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

Mundipagg: Make gateway_affiliation_id an option #3219

Conversation

curiousepic
Copy link
Contributor

@curiousepic curiousepic commented May 14, 2019

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

@curiousepic curiousepic requested a review from a team May 14, 2019 14:31
@curiousepic curiousepic force-pushed the ECS_318_mundipagg_gateway_id branch from e94dec2 to 506dbaf Compare May 14, 2019 15:26
@curiousepic curiousepic changed the title Mundipagg: Allow gateway_affiliation_id override Mundipagg: Make gateway_affiliation_id an option May 14, 2019
@curiousepic curiousepic force-pushed the ECS_318_mundipagg_gateway_id branch from 506dbaf to e71451c Compare May 14, 2019 19:09
Copy link
Contributor

@bayprogrammer bayprogrammer left a 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]
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
@curiousepic curiousepic force-pushed the ECS_318_mundipagg_gateway_id branch from e71451c to 50860a5 Compare May 14, 2019 19:51
Copy link
Contributor

@bayprogrammer bayprogrammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good!

@curiousepic curiousepic deleted the ECS_318_mundipagg_gateway_id branch May 14, 2019 20:22
whitby3001 pushed a commit to whitby3001/active_merchant that referenced this pull request Sep 3, 2019
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
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.

2 participants