-
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
Mundipagg: Make gateway_affiliation_id an option #3219
Mundipagg: Make gateway_affiliation_id an option #3219
Conversation
e94dec2
to
506dbaf
Compare
506dbaf
to
e71451c
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.
Looks good, but had one question left as comment).
@@ -155,7 +155,7 @@ def add_payment(post, payment, options) | |||
post[:customer][:name] = payment.name if post[:customer] | |||
post[:customer_id] = parse_auth(payment)[0] if payment.is_a?(String) | |||
post[:payment] = {} | |||
post[:payment][:gateway_affiliation_id] = @options[:gateway_id] if @options[:gateway_id] | |||
post[:payment][:gateway_affiliation_id] = options[:gateway_affiliation_id] if options[:gateway_affiliation_id] |
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.
Would there be any value to falling back to the GW option if the txn-level option is omitted (esp. if there's a chance of impacting other users of AM who might benefit from a backwards-compatible fallback)?
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.
That's a good point. Since it was never required on initialize and we aren't messing with that signature anyway, there's no harm in leaving a naive fallback as you suggest.
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.
Updated with fallback
gateway_affiliation_id was originally implemented as a gateway-level credential (named gateway_id). However, this field makes more sense as a transaction-level field. Retains the pointer to @options[:gateway_id] as a backward compatible fallback. Remote: 24 tests, 59 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Unit: 18 tests, 78 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
e71451c
to
50860a5
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.
👍 Looks good!
gateway_affiliation_id was originally implemented as a gateway-level credential (named gateway_id). However, this field makes more sense as a transaction-level field. Retains the pointer to @options[:gateway_id] as a backward compatible fallback. Closes activemerchant#3219 Remote: 24 tests, 59 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Unit: 18 tests, 78 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
gateway_affiliation_id was originally implemented as a gateway-level
credential (named gateway_id). However, this field makes more sense as a
transaction-level field.
Remote:
23 tests, 58 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
Unit:
17 tests, 76 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed