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

Map AVS codes for Adyen #3119

Merged
merged 1 commit into from
Jan 31, 2019
Merged

Map AVS codes for Adyen #3119

merged 1 commit into from
Jan 31, 2019

Conversation

therufs
Copy link
Contributor

@therufs therufs commented Jan 18, 2019

Adding more mappings to AVSResult makes me a little ⁉️but it didn't look like there were good explicit corresponding options for the codes Adyen is returning.

@therufs therufs self-assigned this Jan 18, 2019
@therufs therufs requested a review from a team January 18, 2019 17:03
@curiousepic
Copy link
Contributor

So, (sorry I didn't think to mention this earlier) but I think we want to keep the AM-level codes to the standardized list: https://en.wikipedia.org/wiki/Address_Verification_System
Let me think about how to reconcile that.

@curiousepic
Copy link
Contributor

curiousepic commented Jan 18, 2019 •
edited
Loading

It may be possible and desirable to override the AVSResult module's MESSAGES hash in the adapter to include the Adyen-specific codes and messages, instead of adding them directly to the module.

@therufs
Copy link
Contributor Author

therufs commented Jan 31, 2019 •
edited
Loading

hey @activemerchant/spreedly-connect,

The possibility of breaking out AVS messages per card provider has been brought up with product, but we think it's outside the scope of this ticket, so the above is what we're going to go with for now: just populating existing AVS codes/messages where they match. (So, this is ready for actual review!)

curiousepic
curiousepic approved these changes Jan 31, 2019
View reviewed changes
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.

👍 LGTM

@therufs
Copy link
Contributor Author

therufs commented Jan 31, 2019

Remote: 39 tests, 100 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Unit: 26 tests, 127 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

@therufs therufs merged commit 45d43b8 into activemerchant:master Jan 31, 2019
therufs added a commit that referenced this pull request Jan 31, 2019
@therufs
Update changelog with #3119
cb3a1f7
whitby3001 pushed a commit to whitby3001/active_merchant that referenced this pull request Sep 3, 2019
@therufs @whitby3001
Update changelog with activemerchant#3119
29b35c6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@curiousepic curiousepic curiousepic approved these changes

Assignees

@therufs therufs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@therufs @curiousepic