-
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
Rapyd: Add fields and update stored credential method #4877
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.
This looks good, just had a few small suggestions.
else | ||
network_reference_id = options[:stored_credential][:network_transaction_id] | ||
end | ||
|
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.
Two things here:
- I would create this NTID override as options[:network_transaction_id] to keep it consistent across other gateways that have this override (even though Rapyd calls it something else)
- I would use the ternary operator here. Something like
network_reference_id = options[:network_transaction_id] ? options[:network_transaction_id] : options[:stored_credential][:network_transaction_id]
. That might be a little cleaner. You could also use this in the add_initiation_type method below.
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.
Thanks for the suggestions, it totally looks cleaner this way!
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.
I was running into rubocop errors when writing it network_reference_id = options[:network_transaction_id] ? options[:network_transaction_id] : options[:stored_credential][:network_transaction_id]
as it's stating it was an unneeded conditional. The new way I wrote is the same as it'll prioritize the GSF if it's present and then default to the stored_credential hash. Also updated the tests and variable names to reflect network_transaction_id like you suggested and also added another assertion to the remote test when sending both stored credentials and options, thanks a bunch!
3337811
to
3221c95
Compare
f45211d
to
a07eb5f
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.
Looks good! Nice refactoring! (Approving, but you need to fix up the conflict in your unit test file)
a07eb5f
to
5d84afe
Compare
Local: 5588 tests, 77761 assertions, 0 failures, 19 errors, 0 pendings, 0 omissions, 0 notifications 99.66% passed Unit: 27 tests, 137 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 34 tests, 95 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
5d84afe
to
f2b2fcb
Compare
This change is allow users to manage NTID themselves by adding an option for them to pass in the fields network_reference_id and initiation_type directly.
Local:
5588 tests, 77761 assertions, 0 failures, 19 errors, 0 pendings, 0 omissions, 0 notifications 99.66% passed
Unit:
27 tests, 137 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
Remote:
34 tests, 95 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed