-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
ECS-3530 Adyen Format error fix #5155
Conversation
There's also a Rubocop error which is why the GitHub actions tests are failing |
69c643a
to
538b85d
Compare
538b85d
to
30babca
Compare
30babca
to
712d57c
Compare
I added the conditional back into the Then we also need to continue to pass the existing recurring_contract info for apple_pay and google pay and only update that when it is a pure network token. cc: @almalee24 @jherreraa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of individual comments on this one.
We may not need to reach out to Adyen if we handle the situations we left in my second comment.
712d57c
to
634bc81
Compare
634bc81
to
46d3a6e
Compare
Great work on this one Johan, I know this framework can be a bit tricky! |
I found another edge case that needs to be handled before we deploy this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jherreraa, Hey, I found one more issue that we need to resolve before merging this. It should be a quick fix though.
Also maybe add at least one test on apple pay and google pay where the new flag is set to false so we make sure that it passes in both cases.
46d3a6e
to
26eac3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the last thing that we need to solidify and then we are good to go on this. I know this has been a long one, thanks for sticking with it!
26eac3c
to
8b82d84
Compare
8b82d84
to
38b395f
Compare
def add_recurring_contract(post, options = {}) | ||
return unless options[:recurring_contract_type] | ||
def add_recurring_contract(post, options = {}, payment = nil) | ||
return unless options[:recurring_contract_type] || (payment.try(:source) && options[:switch_cryptogram_mapping_nt]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be payment.try(:source) == :network_token && options[:switch_cryptogram_mapping_nt]
38b395f
to
dea3ee6
Compare
180d6e6
to
51d2123
Compare
[ECS-3530](https://spreedly.atlassian.net/browse/ECS-3530) This PR fix format error using NT Unit tests ---------------- Finished in 0.126432 seconds. 121 tests, 641 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote tests ---------------- Finished in 609.957316 seconds. 143 tests, 464 assertions, 11 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 92.3077% passed 0.23 tests/s, 0.76 assertions/s -> failures not related to change
51d2123
to
19370e2
Compare
ECS-3530
Summary:
This PR fix format error using NT
Unit tests
Finished in 0.126432 seconds.
121 tests, 641 assertions, 0 failures,
0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
Remote tests
Finished in 609.957316 seconds.
143 tests, 464 assertions, 11 failures,
0 errors, 0 pendings, 0 omissions, 0 notifications 92.3077% passed
0.23 tests/s, 0.76 assertions/s
-> failures not related to change