CyberSource: Add support for issuer additionalData gateway-specific field #3296
Conversation
jasonxp
force-pushed
the
ce-56-cybersource-add-additional-data-gsfs
branch
2 times, most recently
from
192e4dc to
a46b52d
Compare
| assert_success response | ||
| assert response.test? | ||
| assert_successful_response(response) | ||
| assert !response.authorization.blank? |
There was a problem hiding this comment.
Consider changing this to assert_not instead of assert !.
There was a problem hiding this comment.
That's actually not code that I added, so I'll leave it alone for now since assert ! is used consistently (15 times) throughout the tests.
There was a problem hiding this comment.
(I'm slightly bewildered as to why GitHub shows this with a green highlight, but it is indeed in master here: https://github.com/activemerchant/active_merchant/blob/master/test/remote/gateways/remote_cyber_source_test.rb#L107).
There was a problem hiding this comment.
Is this something worth refactoring for readability?
There was a problem hiding this comment.
I do like assert_not better in general too, but I think consistency within the same test case is more important, so I would stick with assert ! if that's what's used throughout the rest of the nearby code.
There was a problem hiding this comment.
Yes, agreed that consistency is most important! My question was whether this is something that would be worth changing throughout this test file.
There was a problem hiding this comment.
Ah, gotcha. Doesn't hurt either way in my opinion, but probably in this case it is a matter of deciding how far to go for the immediate ticket vs. leaving otherwise unrelated changes to a different refactor.
I know I always see more I want to do, but often have to determine what is necessary for the task at hand and constrain myself to that. 🙂
| @@ -590,4 +604,11 @@ def test_verify_credentials | |||
| assert !gateway.verify_credentials | |||
| end | |||
|
|
|||
| private | |||
|
|
|||
| def assert_successful_response(response) | |||
There was a problem hiding this comment.
It looks like this method wasn't extracted out from all of the other tests, leading to some tests still including all three lines (e.g., test_successful_authorization_with_elo) and others using this new private method. This seems like a good thing to make consistent.
There was a problem hiding this comment.
True, there were a lot of them. I refactored them all to DRY things out.
jasonxp
force-pushed
the
ce-56-cybersource-add-additional-data-gsfs
branch
from
a46b52d to
d3dbff2
Compare
| assert_success response | ||
| assert response.test? | ||
| assert_successful_response(response) | ||
| assert !response.authorization.blank? |
There was a problem hiding this comment.
I do like assert_not better in general too, but I think consistency within the same test case is more important, so I would stick with assert ! if that's what's used throughout the rest of the nearby code.
…ield Added support for the issuer additionalData CyberSource-specific field. The CyberSource XML schema has also been updated. Per the gateway's instructions, we are now using version 1.156 in the test environment, and version 1.155 in production. CE-56 Unit tests: 4195 tests, 70144 assertions, 0 failures, 0 errors, 0 pendings, 2 omissions, 0 notifications 100% passed test/remote/gateways/remote_cyber_source_test.rb contains 5 pre-existing, unrelated test failures: - test_successful_3ds_validate_authorize_request - test_successful_3ds_validate_purchase_request - test_successful_pinless_debit_card_puchase - test_successful_tax_calculation - test_successful_validate_pinless_debit_card 59 tests, 260 assertions, 5 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
jasonxp
force-pushed
the
ce-56-cybersource-add-additional-data-gsfs
branch
from
d3dbff2 to
7e5a1c9
Compare
Added support for the issuer additionalData CyberSource-specific field. The CyberSource XML schema has also been updated. Per the gateway's instructions, we are now using version 1.156 in the test environment, and version 1.155 in production.