-
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
Nuvie/SafeCharge: Add unreferenced refund field #4831
Conversation
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.
One small suggestion for refactoring -- I also think you could add a unit test here; the test expectation would be refuting that the sg_TransactionID
is present in the request when unreferenced_refund
is present in the options
post[:sg_CCToken] = token | ||
post[:sg_ExpMonth] = exp_month | ||
post[:sg_ExpYear] = exp_year | ||
|
||
commit(post) | ||
if options[:unreferenced_refund] == true |
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.
Instead of duplicating the commit(post)
statement with this logic, could you instead add an unless
to the end of line 76?
post[:sg_TransactionID] = transaction_id unless options[:unreferenced_refund]
I think this would make the change a little bit more DRY
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.
yep I totally agree! I was completely brain fogged and was already fixing it when the linter failed lol, thanks!
f490c77
to
c91b917
Compare
I was playing around with trying to do something similar to this but didnt get far. After stepping away for a bit, I figured it out, thanks! |
8ae538c
to
448ab0b
Compare
775bca5
to
011e4d6
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!
011e4d6
to
8dd0d54
Compare
Note:
I didn't include a unit test because this fields purpose is to exclude sending the sg_TransactionID and not meant to return anything.
Local:
5545 tests, 77509 assertions, 0 failures, 19 errors, 0 pendings, 0 omissions, 0 notifications
99.6573% passed
Unit:
25 tests, 144 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
Remote:
32 tests, 89 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed