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.

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

Revert AdyenPaymentMethod#cancel! method signature #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spaghetticode
Copy link

@spaghetticode spaghetticode commented Jun 13, 2024

The method signature was originally changed in #15 that made the extension compatible with Ruby 3.x.

The previous version is compatible with Ruby 3.x, while the new one was not equivalent to the old one as it requires the second param, which is irrelevant in the context of the method, to be present.

The new signature poses an issues at least on Rowan, as it prevents payments to be cancelled. This is from Rowan staging:

Spree::Order.find_by(number: 'KR700391505').payments.first.cancel!
D, [2024-06-13T08:17:47.239828 #2] DEBUG -- :   TRANSACTION (1.1ms)  ROLLBACK
/app/vendor/bundle/ruby/3.0.0/bundler/gems/solidus-adyen-f654fd0d419c/app/models/concerns/spree/payment_method/adyen_payment_method.rb:41:in `cancel': wrong number of arguments (given 1, expected 2) (ArgumentError)

The method signature was originally changed in #15 that made the
extension compatible with Ruby 3.x.

The previous version is compatible with Ruby 3.x, while the new one
was not equivalent to the old one as it requires the second param,
which is irrelevant in the context of the method, to be present.

The new signature poses an issues at least on Rowan, as it prevents
payments to be cancelled:

    Spree::Order.find_by(number: 'KR700391505').payments.first.cancel!
    D, [2024-06-13T08:17:47.239828 #2] DEBUG -- :   TRANSACTION (1.1ms)  ROLLBACK
    /app/vendor/bundle/ruby/3.0.0/bundler/gems/solidus-adyen-f654fd0d419c/app/models/concerns/spree/payment_method/adyen_payment_method.rb:41:in `cancel': wrong number of arguments (given 1, expected 2) (ArgumentError)
@cjreeve
Copy link

cjreeve commented Jun 13, 2024

I haven't noticed an error on WATG for this and we appear to be using the code. I don't know about the possible impact this change could have. Hopefully @MadelineCollier will remember previous issue and have an idea if something else is needed.

Copy link

@MadelineCollier MadelineCollier left a comment

Choose a reason for hiding this comment

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

Approved as I understand the general gist of the change and how the Ruby 3x work altered the method signature, however this change definitely requires manual testing across all the stores to ensure this:

a) Fixes the issue on Rowan
b) Doesn't introduce new errors for the other brands.

Thanks for taking this on :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants