-
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
Correctly process Adyen risk_data option #3161
Correctly process Adyen risk_data option #3161
Conversation
@molbrown I noticed the failing tests wrt 'Refused' vs 'CVC Declined', but that you had fixed those in your recent Adyen commit that you rolled back. Should I go ahead and include those fixes in my branch here, or leave them for when your revisions get merged back in? |
Those updates weren't related to my ticket, just an error message update. Ok for you to add the change 👍 |
Looking good. Don't forget test run output in the commit message. |
On that, is the "draft-final" commit something I should include with my pull request, or wait until all it said and done to prepare? For some reason I was thinking I'd add that as part of the final squash-commit preparation phase post-review, but should I include that up front (and then retain and/or update the commit message when doing the final squashing)? |
@bayprogrammer Ah, not a big deal, I'm just used to reviewing theoretically final commits/messages (this was not submitted as a draft AFAICT), and changes due to review being amended to it. |
@curiousepic Yah, didn't do as a draft PR this time. I'll squash and include the proper commit message for future PRs (and will do that shortly for this one). Thanks for the heads up on our process! :) |
@bayprogrammer The standard process is to include test run output as reassurance that tests were executed. It also serves as an indicator that the PR is ready to be reviewed and merged once approved. This is especially true in Core where the absence of test output in a PR is a code smell. |
I've properly prepared my commit (squashed, with test run information included). I also tweaked The remote tests that were failing yesterday due to mismatched expectations with the expected failure message string suddenly stopped failing, so I did not change the remote test as I had expected I would. |
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.
Nice, LGTM!
This fixes how we pass risk data to Adyen via the `risk_data` option. Adyen requires provided risk data elements to be included directly under `additionalData`, not in a sub-hash, with keys prefixed with `riskdata.`. ECS-200 Unit: 29 tests, 140 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 47 tests, 133 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed closes #3161
This fixes how we pass risk data to Adyen via the `risk_data` option. Adyen requires provided risk data elements to be included directly under `additionalData`, not in a sub-hash, with keys prefixed with `riskdata.`. ECS-200 Unit: 29 tests, 140 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 47 tests, 133 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed closes activemerchant#3161
No description provided.