Wompi: allow partial refund amount on void_sync #4535
Conversation
| def void(authorization, options = {}) | ||
| commit('void', {}, "/transactions/#{authorization}/void_sync") | ||
| def void(authorization, options = {}, money = nil) | ||
| post = money ? { amount_in_cents: amount(money).to_i } : {} |
There was a problem hiding this comment.
This logic is an attempt to preserve the functionality of an actual request to the void endpoint (rather than a refund redirected to a void)
I did some local testing (including integration tests) and realized that a /void request would break without this default nil amount for money, because usually no amount argument is expected for a true void
There was a problem hiding this comment.
Thanks for that explanation, the nil did stick out!
There was a problem hiding this comment.
Looks good to me! Can you add a new comment in refund on line 64 above where post is created? Reading through this again I don't think my original comment is clear enough.
I'm thinking # All refunds will be temporarily sent to the void endpoint. The below comments preserve the original refund logic in case needed once the refund endpoint is restored.
| end.check_request do |endpoint, data, _headers| | ||
| assert_match 'void_sync', endpoint | ||
| assert_match '{}', data | ||
| end.respond_with(successful_void_response) |
There was a problem hiding this comment.
Would appreciate any feedback on these test cases to make sure they are doing what I think they are doing 😁
There was a problem hiding this comment.
Tests look great to me! Very thorough. At first I was wondering why you didn't explicitly call out the amount in the assertions, but after a pause, this is better because it makes your test more resilient in case someone changes the @amount again.
|
@drkjc I tagged you just because I though you might be familiar with this, since you did the initial work to re-direct refund to void. 🙏🏻 |
There was a problem hiding this comment.
✨ Looks good and tests are passing. I had a couple of comments about the remote tests. When I ran the whole suite some of them failed but when ran individually those failing tests passed. Wompi's sandbox is still unpredictable it seems. I'm approving but I think it's wise to wait to hear back from them about the partial refund amount before you merge.
| def void(authorization, options = {}) | ||
| commit('void', {}, "/transactions/#{authorization}/void_sync") | ||
| def void(authorization, options = {}, money = nil) | ||
| post = money ? { amount_in_cents: amount(money).to_i } : {} |
There was a problem hiding this comment.
Thanks for that explanation, the nil did stick out!
| end.check_request do |endpoint, data, _headers| | ||
| assert_match 'void_sync', endpoint | ||
| assert_match '{}', data | ||
| end.respond_with(successful_void_response) |
There was a problem hiding this comment.
Tests look great to me! Very thorough. At first I was wondering why you didn't explicitly call out the amount in the assertions, but after a pause, this is better because it makes your test more resilient in case someone changes the @amount again.
| # assert refund = @gateway.refund(@amount - 1, purchase.authorization) | ||
| # assert_success refund | ||
| # end | ||
| assert refund = @gateway.refund(@amount - 50000, purchase.authorization) |
There was a problem hiding this comment.
I pryed into this test as well to check the @amount, also not seeing a change. I guess if you wanted to be more explicit with the test, you could add a couple of lines like
response_data = refund.params['data']
assert_equal response_data.dig('transaction', 'amount_in_cents'), 100000
But as the gateway is currently behaving that assert_equal will fail.
There was a problem hiding this comment.
Thanks @rachelkirk ! I also experienced some intermittent failure on tests that seemed related to connection issues with the sandbox.
I will definitely wait on Wompi's feedback before merging, and if they indicate that we should be seeing the partial amount in their response, I'll dig into that further and also update the test with the assert_equal tip you suggested
There was a problem hiding this comment.
@jcreiff @rachelkirk Everything looks good! Though, for some reason I can't get the partial refund remote test passing.
I'm consistently getting a bad request error, what errors where you seeing? Seems like a problem on my end so I'm going to approve, just don't forget a CHANGELOG entry.
| def void(authorization, options = {}) | ||
| commit('void', {}, "/transactions/#{authorization}/void_sync") | ||
| def void(authorization, options = {}, money = nil) | ||
| post = money ? { amount_in_cents: amount(money).to_i } : {} |
There was a problem hiding this comment.
Looks good to me! Can you add a new comment in refund on line 64 above where post is created? Reading through this again I don't think my original comment is clear enough.
I'm thinking # All refunds will be temporarily sent to the void endpoint. The below comments preserve the original refund logic in case needed once the refund endpoint is restored.
Previous changes to this gateway redirected all refund requests to the void endpoint. This meant that any partial refund attempts were processed as full refunds, because the void method did not accept an amount. Wompi has advised that it is possible to send amount_in_cents to this void_sync endpoint to allow for partial refunds. CER-152 LOCAL 5276 tests, 76199 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 746 files inspected, no offenses detected UNIT 12 tests, 58 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed REMOTE 13 tests, 34 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
jcreiff
force-pushed
the
CER-152_wompi_void_amount
branch
from
caeb9d7
to
af17e54
Compare
Previous changes to this gateway redirected all refund requests to the
void endpoint. This meant that any partial refund attempts were processed
as full refunds, because the void method did not accept an amount.
Wompi has advised that it is now possible to send amount_in_cents to this
void_sync endpoint to allow for partial refunds.
CER-152
LOCAL
5276 tests, 76199 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
746 files inspected, no offenses detected
UNIT
12 tests, 58 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
REMOTE
13 tests, 34 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed