-
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
Orbital: Add support for L2 and L3 fields, update XSD file #5123
Conversation
@@ -1232,7 +1243,13 @@ def setup | |||
tax_indicator: '1', | |||
tax: '75', | |||
purchase_order: '123abc', | |||
zip: address[:zip] | |||
zip: address[:zip], |
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.
It's not super clear by looking at the diff, but these are in a separate Tandem Class that uses different credentials. When adding all fields to level_2_options, I got errors about fields being restricted to Canadian merchants which is the reason for the new remote test with that currency and those fields are only added in that test.
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.
😮💨 great work parsing through the XSD; I tried to squint at it and find anything that might have been overlooked, but I think it's looking good!
Left one small comment about one of the tests.
I think my larger question is about whether or not updating the XSD that we are testing against means that we also need to update the API_VERSION
declared at the top of the gateway model. I notice that in the previous commits that changed the XSD, there was also a change to that line.
https://github.com/activemerchant/active_merchant/pull/3850/files
https://github.com/activemerchant/active_merchant/pull/3117/files
@@ -1953,4 +1971,11 @@ def post_scrubbed_echeck | |||
Conn close | |||
REQUEST | |||
end | |||
|
|||
def assert_xml_valid_to_xsd(data) |
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.
nice bit of refactoring here 🌟
89a4e33
to
d159d00
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.
LGTM!
@@ -269,6 +291,9 @@ def test_network_tokenization_credit_card_data | |||
|
|||
def test_schema_for_soft_descriptors_with_network_tokenization_credit_card_data |
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.
@jknipp I updated this unit test to bring in the fields with L2/L3 data. Refactored the assertion to ensure it is valid to XSD instead of just matching specific fields.
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.
This test could probably be renamed since it's no longer specific to network tokenization and soft descriptors
d28e055
to
d678a6c
Compare
@rachelkirk I think this looks fine? I'd prefer another set of 👀 from a member of Pathfinder to be doubly sure on the specific field modifications. |
@DeeMeyers reviewed it earlier but I can reach out to their team again if you meant you wanted more than one Pathfinder team member to review. |
d678a6c
to
a9f6107
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.
Thanks for adding the additional xsd assertions!
A bit of refactoring was needed in the gateway file to support these fields and get things in better shape with adhering to the XSD. Unit tests are now more comprehensive to check schema. I added a remote test and left comments to indicate which fields are restricted to Canadian merchants. Unit Tests: 148 tests, 850 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote Tests: 134 tests, 504 assertions, 8 failures, 11 errors, 0 pendings, 0 omissions, 0 notifications 85.8209% passed *Same number of failures and errors on master. Local Tests: 5899 tests, 79647 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
a9f6107
to
f011897
Compare
A bit of refactoring was needed in the gateway file to support these fields and get things in better shape with adhering to the XSD. Unit tests are now more comprehensive to check schema. I added a remote test and left comments to indicate which fields are restricted to Canadian merchants.
Unit Tests:
148 tests, 850 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
Remote Tests:
134 tests, 504 assertions, 8 failures, 11 errors, 0 pendings, 0 omissions, 0 notifications 85.8209% passed
*Same number of failures and errors on master.
Local Tests:
5899 tests, 79647 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed