THREESCALE-11383: Ignore errors on payment gateway initialization when destroying buyer

[mayorova]

What this PR does / why we need it:

If payment gateway configuration is invalid on a provider account, their buyers cannot be deleted because the unstore_credit_card! method is called as before_destroy callback.

Specifically, if the payment gateway type is :stripe, but the gateway settings stored in the database miss the login field, the payment gateway instance cannot be initialized, and the whole operation fails.

This PR catches the error, raises a more specific Account::Gateway::InvalidSettingsError error and "swallows" it the unstore_credit_card!, thus allowing for the destroy to complete successfully.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-11383

Verification steps

Steps to reproduce or to confirm that that issue is not reproducible in the branch:

pgs = PaymentGatewaySetting.new(gateway_type: :stripe, gateway_settings: { test: 1, private_key: "foo", merchant_id: "bar", public_key: "foobar" })

provider = FactoryBot.create(:provider_account, payment_gateway_setting: pgs)

buyer = FactoryBot.create(:buyer_account, provider_account: provider)

buyer.smart_destroy

The destroy operation should succeed and not throw errors.

Special notes for your reviewer:

[akostadinov]

My question is whether we should raise here? Or should we return something like InvalidPaymentGateway object instead? It seems pretty easy to configure an invalid gateway and handling exceptions might be less straightforward than returning an object that can have programmed the desired behavior 🤔

[mayorova]

Well, I'm not sure to be honest whether it's "easy" to misconfigure it... I'm not sure in fact how it is possible 😬 (and how that "bad" configuration got there in the first place)

Looks like the current implementation returns nil if payment_gateway_type is blank. In fact, I am not sure what consequences it might have...

To me throwing an exception seems to be more logical and straightforward than returning a InvalidPaymentGateway... but it could be done also.

[mayorova]

@akostadinov So, what do you think?

I am a bit concerned that we may throw this exception in production. But on the other hand, currently an exception would also be thrown, so the current behavior actually wouldn't change 😅 It would just give us more useful info about the error in BugSnag.

[akostadinov]

I still believe that we should not throw but return an object. Still this fixes a real issue and is a net improvement in behavior so I'm approving. Thank you.

[akostadinov]

this line can be removed

[mayorova]

So, I trusted you and removed the line, but then I realized that this end in line :104 doesn't actually end the method, so I think we can't get rid of begin.

[akostadinov]

my bad, I overlooked that, thought you added by mistake

[akostadinov]

Although then there is a check return if payment_detail.destroyed? which idk how it is supposed to work. Maybe we should make the rescue global for the method.

[mayorova]

I think these are need to be executed in any case:

    self.credit_card_auth_code = nil
    self.credit_card_expires_on = nil
    self.credit_card_partial_number = nil

[akostadinov]

I wouldn't care about these other values. If things work in real life as they are now, then I'm fine with it. I was just worried that by invalid configuration payment_detail may raise. But I think your test ensures it wouldn't.