CE-2343 Bluesnap Idempotency #4305
Conversation
| @@ -573,4 +573,10 @@ def test_transcript_scrubbing_with_echeck | |||
| assert_scrubbed(@check.routing_number, transcript) | |||
| assert_scrubbed(@gateway.options[:api_password], transcript) | |||
| end | |||
|
|
|||
| def test_successful_purchase_with_idempotency_key | |||
There was a problem hiding this comment.
Thank you for adding a remote test!
test/unit/gateways/blue_snap_test.rb
Outdated
| response = stub_comms(@gateway, :raw_ssl_request) do | ||
| @gateway.purchase(@amount, @credit_card, @options.merge({ idempotency_key: 'test123' })) | ||
| end.check_request do |headers| | ||
| headers && headers['Idempotency-Key'] == 'test123' |
There was a problem hiding this comment.
We could replace 'test123' with any value and this test would still pass.
This is because we aren't asserting anything here. It needs to be assert_equal 'test123', headers['Idempotency-Key'].
Once you fix that you will see that the test fails because check_request hasn't been implemented correctly here. In this instance the headers end up being the fourth value returned by check_request, so the syntax should look like:
end.check_request do |_method, _url, _data, headers|
kind of like you see here.
In my experience, it is not easy to see how check_request works under the hood. It yields values depending on how the gateway adapter formulates the request, so whenever I need to use it to extract a value from a request I usually just declare all four values in the block and add a binding inside the block as well to look at what it has extracted.
There was a problem hiding this comment.
That makes sense. I knew something was a little off here. And thanks for the tip.
test/unit/gateways/blue_snap_test.rb
Outdated
| end.respond_with(successful_authorize_response) | ||
|
|
||
| assert_success response | ||
| assert_equal '1012082893', response.authorization |
There was a problem hiding this comment.
Nit: These two lines are superfluous, so you might as well remove them.
On line 551 you have stubbed the response, and here you are making assertions against the very response you provided.
I know this pattern has been copied and pasted throughout active merchant but I think the test is much nicer if you remove these lines because then it better communicates what you are actually trying to test (which is that the request itself contains the header).
| @@ -433,15 +433,15 @@ def parse_element(parsed, node) | |||
| end | |||
| end | |||
|
|
|||
| def api_request(action, request, verb, payment_method_details) | |||
| ssl_request(verb, url(action, payment_method_details), request, headers) | |||
| def api_request(action, request, verb, payment_method_details, options) | |||
There was a problem hiding this comment.
It's kind of weird to me that we have this api_request method wrapping the ssl_request method. We could just calculate the url and the header inside the commit method and then call ssl_request directly the way we do in most other gateway adapters.
This is just an idle thought on my part, not a request for changes. It has nothing to do with the changes you are making in this PR.
| @@ -1,4 +1,5 @@ | |||
| require 'nokogiri' | |||
| require 'pry' | |||
There was a problem hiding this comment.
If I had a nickel for every time I've done this.
Speaking from experience, you might find it easier to do this inline when you add breakpoints, e.g. (require 'pry'; binding.pry), that way rubocop will prevent you from trying to check this require line into the code.
| @@ -543,6 +543,14 @@ def test_localizes_currencies | |||
| end | |||
| end | |||
|
|
|||
| def test_optional_idempotency_key_header | |||
Add the ability to optionally send Idempotency Key as a header on BlueSnap transactions. ECS-2343 Unit: 41 tests, 237 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 49 tests, 166 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
drkjc
force-pushed
the
CE_2343_bluesnap_idempotency
branch
from
ad6e8e1
to
eb4e30d
Compare
Bluesnap: Add support for optional Idempotency Key field on purchase, authorize, capture, refund, and void. Idempotency Key is passed as a header.
Unit tests:
41 tests, 237 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
Remote tests:
49 tests, 166 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed