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

Correctly process Adyen risk_data option #3161

Merged
merged 1 commit into from
Mar 6, 2019
Merged

Correctly process Adyen risk_data option #3161

merged 1 commit into from
Mar 6, 2019

Conversation

bayprogrammer
Copy link
Contributor

No description provided.

@bayprogrammer bayprogrammer requested a review from a team March 5, 2019 22:03
@bayprogrammer
Copy link
Contributor Author

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

@molbrown
Copy link
Contributor

molbrown commented Mar 6, 2019

Those updates weren't related to my ticket, just an error message update. Ok for you to add the change 👍

@curiousepic
Copy link
Contributor

curiousepic commented Mar 6, 2019

Looking good. Don't forget test run output in the commit message.

@bayprogrammer
Copy link
Contributor Author

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)?

@curiousepic
Copy link
Contributor

curiousepic commented Mar 6, 2019

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

@bayprogrammer
Copy link
Contributor Author

@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! :)

@jknipp
Copy link
Member

jknipp commented Mar 6, 2019

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

@bayprogrammer
Copy link
Contributor Author

I've properly prepared my commit (squashed, with test run information included). I also tweaked #add_risk_data per my comment earlier.

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.

Copy link
Contributor

@curiousepic curiousepic left a 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
@bayprogrammer bayprogrammer merged commit 825008a into activemerchant:master Mar 6, 2019
@bayprogrammer bayprogrammer deleted the ECS-200-adyen-riskdata-gsf-invalid-request branch March 6, 2019 21:46
whitby3001 pushed a commit to whitby3001/active_merchant that referenced this pull request Sep 3, 2019
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
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.

4 participants