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

Cybersource: Add merchant_id #4844

Merged
merged 1 commit into from
Aug 4, 2023
Merged

Cybersource: Add merchant_id #4844

merged 1 commit into from
Aug 4, 2023

Conversation

almalee24
Copy link

Set the merchantId filed to options[:merchant_id] instead of @options[:login] if available.

Unit: 136 tests, 641 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed

@almalee24 almalee24 requested a review from a team July 28, 2023 14:56
Copy link
Contributor

@aenand aenand left a comment

Choose a reason for hiding this comment

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

This looks good! Can you also make this change to the CyberSource REST gateway?

@almalee24
Copy link
Author

This looks good! Can you also make this change to the CyberSource REST gateway?

Yes, I'll make those changes. I just realized that @options[:merchant_id] was different than this new field

Copy link
Contributor

@BritneyS BritneyS 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!

@@ -556,7 +556,7 @@ def add_line_item_data(xml, options)
end

def add_merchant_data(xml, options)
xml.tag! 'merchantID', @options[:login]
xml.tag! 'merchantID', options[:merchant_id] || @options[:login]
Copy link
Contributor

Choose a reason for hiding this comment

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

options vs @options here is interesting. This makes sense, since it looks like @options is a hash for "gateway options" and options is a hash for "request options", so merchant_id is passed after the gateway was originally created.

Copy link
Contributor

@aenand aenand left a comment

Choose a reason for hiding this comment

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

Nice work!

string_to_sign = {
host: host,
date: gmtdatetime,
"(request-target)": "#{http_method} /pts/v2/#{resource}",
digest: digest,
"v-c-merchant-id": @options[:merchant_id]
"v-c-merchant-id": options[:merchant_id] || @options[:merchant_id]
Copy link
Contributor

Choose a reason for hiding this comment

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

After reviewing the REST docs I made a mistake in the requirements. I believe we should only override it for the HTTP header, not the Signature.

Copy link
Contributor

@aenand aenand left a comment

Choose a reason for hiding this comment

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

Nice work!

Set the merchantId filed to options[:merchant_id] instead of @options[:login]
if available.

Cybersource Unit:
136 tests, 641 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

CybersourceRest Unit:
30 tests, 144 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
@almalee24 almalee24 merged commit 21d987d into master Aug 4, 2023
5 checks passed
@almalee24 almalee24 deleted the cybersource_merchant_id branch August 4, 2023 15:09
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.

3 participants