-
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
SumUp Gateway: Fix refund method #4924
Conversation
89c79e1
to
76330f4
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 just I left some comments for you consideration. 👍🏻
@@ -123,6 +123,26 @@ def test_failed_void_invalid_checkout_id | |||
assert_equal 'Resource not found', response.message | |||
end | |||
|
|||
def test_successful_refund | |||
gateway = SumUpGateway.new(fixtures(:sum_up_testing)) |
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.
Maybe we can find a better name and include it in the fixtures.yml file, to keep in mind that we need to use this credentials as well to run remote test.
@@ -36,8 +36,7 @@ def void(authorization, options = {}) | |||
|
|||
def refund(money, authorization, options = {}) | |||
transaction_id = authorization.split('#')[-1] |
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.
Split is not necessary here
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.
In fact is actually needed due the way that authorization_from
builds the reference, now is that the better way ?, maybe not:
transaction_id = authorization.split('#').last
return false unless %w(PENDING EXPIRED PAID).include?(response[:status]) | ||
return false unless %w(PENDING EXPIRED PAID REFUND).include?(response[:status]) | ||
|
||
return true if response[:transactions].blank? |
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.
since you are returning a proper response for refund
, I think this return true
is not needed, unless for other transaction we have empty responses.
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.
@sinourain left some complementary PR comments
@@ -36,8 +36,7 @@ def void(authorization, options = {}) | |||
|
|||
def refund(money, authorization, options = {}) | |||
transaction_id = authorization.split('#')[-1] |
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.
In fact is actually needed due the way that authorization_from
builds the reference, now is that the better way ?, maybe not:
transaction_id = authorization.split('#').last
gateway = SumUpGateway.new(fixtures(:sum_up_testing)) | ||
purchase = gateway.purchase(@amount, @credit_card, @options) | ||
assert_success purchase | ||
assert_equal 'PAID', purchase.message |
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.
💬 Comment:
Do we need all those assertions on the part of the code that is just the context/preparation for the actual test that is going to be refund
?, there are 7 assertions before the refund request is actually fired and that request only has 2 assertions, are we testing purchase
or refund
?
@@ -123,6 +123,26 @@ def test_failed_void_invalid_checkout_id | |||
assert_equal 'Resource not found', response.message | |||
end | |||
|
|||
def test_successful_refund |
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.
💬 Comment:
Would be great to actually a test that covers a partial refund.
76330f4
to
479e803
Compare
479e803
to
392dffa
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.
Just a few comments, generally looks good though 👍
# | ||
# For this example configure in the fixtures => :sum_up_account_for_successful_purchases | ||
def test_successful_refund | ||
gateway = SumUpGateway.new(fixtures(:sum_up_account_for_successful_purchases)) |
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.
Can you share the credentials to test this and also create an entry in the fixtures for sum_up_account_for_successful_purchases
?
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.
Done!
return response[:status] if success_from(response) | ||
def message_from(succeeded, response) | ||
if succeeded | ||
return 'SUCCESSFUL' if response.is_a?(Integer) |
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.
Change 'SUCCSESSFUL'
to 'Succeeded'
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.
Done!
@sinourain - just a nudge for you to respond to my review |
thank you. With so many things I had not seen your comments. I'm going to review them. |
392dffa
to
4c05cb6
Compare
@@ -1,3 +1,4 @@ | |||
require 'pry' |
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.
You can remove this require
statement
def test_successful_refund | ||
gateway = SumUpGateway.new(fixtures(:sum_up_account_for_successful_purchases)) | ||
purchase = gateway.purchase(@amount, @credit_card, @options) | ||
transaction_id = purchase.params['transaction_id'] |
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.
We should assert
the transaction_id here, otherwise the test throws an error if the value is nil
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.
Done!
def test_successful_partial_refund | ||
gateway = SumUpGateway.new(fixtures(:sum_up_account_for_successful_purchases)) | ||
purchase = gateway.purchase(@amount * 10, @credit_card, @options) | ||
transaction_id = purchase.params['transaction_id'] |
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.
Same as above
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.
Done!
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.
Just a few requests to change in the tests, but everything else looks good!
4c05cb6
to
bd2d111
Compare
Description ------------------------- Fix refund method to SumUp Gateway adapter. This are the relevant links to review the implementation: - [Refund a transaction](https://developer.sumup.com/docs/api/refund-transaction/) Tickets for Spreedly reference SER-836 Note: SumUp has shared with us an account to test with which you can refund a purchase Unit test ------------------------- Finished in 32.469516 seconds. 5638 tests, 78183 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote test ------------------------- 173.64 tests/s, 2407.89 assertions/s Rubocop ------------------------- 773 files inspected, no offenses detected
bd2d111
to
781d12f
Compare
Description
Fix refund method to SumUp Gateway adapter.
This are the relevant links to review the implementation:
Tickets for Spreedly reference
SER-836
Note: SumUp has shared with us an account to test with which we can refund a purchase
Unit test
Finished in 32.469516 seconds.
5638 tests, 78183 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
Remote test
173.64 tests/s, 2407.89 assertions/s
Rubocop
773 files inspected, no offenses detected