Skip to content
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

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

yunnydang
Copy link
Contributor

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

@yunnydang yunnydang requested a review from a team July 17, 2023 19:24
@yunnydang yunnydang self-assigned this Jul 17, 2023
Copy link
Contributor

@jcreiff jcreiff left a 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
Copy link
Contributor

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

Copy link
Contributor Author

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!

@yunnydang yunnydang force-pushed the add_field_unreferenced_refund_safe_charge branch from f490c77 to c91b917 Compare July 17, 2023 19:41
@yunnydang
Copy link
Contributor Author

refuting that the sg_TransactionID is present in the request when unreferenced_refund is present in the options

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!

@yunnydang yunnydang force-pushed the add_field_unreferenced_refund_safe_charge branch 2 times, most recently from 8ae538c to 448ab0b Compare July 18, 2023 01:12
@yunnydang yunnydang requested a review from jcreiff July 18, 2023 01:12
@yunnydang yunnydang force-pushed the add_field_unreferenced_refund_safe_charge branch 2 times, most recently from 775bca5 to 011e4d6 Compare July 18, 2023 01:45
Copy link
Contributor

@jcreiff jcreiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yunnydang yunnydang force-pushed the add_field_unreferenced_refund_safe_charge branch from 011e4d6 to 8dd0d54 Compare July 19, 2023 18:56
@yunnydang yunnydang merged commit 8dd0d54 into master Jul 19, 2023
13 checks passed
@yunnydang yunnydang deleted the add_field_unreferenced_refund_safe_charge branch July 19, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants