Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Oct 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions app/models/account/credit_card.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,12 @@ def delete_cc_details
end

def unstore_credit_card!
response = provider_payment_gateway.try!(:threescale_unstore, credit_card_auth_code)
log_gateway_response(response, "unstore [auth: #{credit_card_auth_code}]")
begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

response = provider_payment_gateway.try!(:threescale_unstore, credit_card_auth_code)
log_gateway_response(response, "unstore [auth: #{credit_card_auth_code}]")
rescue Account::Gateway::InvalidSettingsError => error
Rails.logger.warn("Couldn't unstore credit card details from the payment gateway: #{error.message}")
end

return if payment_detail.destroyed?

Expand Down
4 changes: 4 additions & 0 deletions app/models/account/gateway.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
module Account::Gateway
extend ActiveSupport::Concern

class InvalidSettingsError < StandardError; end

included do
has_many :payment_transactions
has_one :payment_gateway_setting, dependent: :destroy, inverse_of: :account
Expand All @@ -18,6 +20,8 @@ def payment_gateway(**options)
return if payment_gateway_type.blank?

PaymentGateway.implementation(payment_gateway_type, **options).new(payment_gateway_options || {})
rescue ArgumentError => exception
raise InvalidSettingsError, exception.message
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

def payment_gateway_configured?
Expand Down
18 changes: 18 additions & 0 deletions test/unit/account/credit_card_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,24 @@ class Account::CreditCardTest < ActiveSupport::TestCase
assert_nil provider.credit_card_partial_number
end

test 'unstore credit card succeeds if gateway settings are invalid' do
provider = FactoryBot.create(:simple_account, payment_gateway_type: :bogus)
buyer = FactoryBot.create(:simple_buyer, provider_account: provider,
credit_card_auth_code: "fdsa",
credit_card_expires_on: Date.new(2020, 4, 2),
credit_card_partial_number: "0989")

ActiveMerchant::Billing::BogusGateway.expects(:new).raises(ArgumentError.new("Missing required parameter: whatever"))

assert_nothing_raised do
buyer.unstore_credit_card!
end

assert_nil buyer.credit_card_auth_code
assert_equal Time.zone.today.change(:day => 1), buyer.credit_card_expires_on_with_default
assert_nil buyer.credit_card_partial_number
end

test '#credit_card_editable? returns false for master account' do
refute master_account.credit_card_editable?
end
Expand Down