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

Rapyd: Add fields and update stored credential method #4877

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

yunnydang
Copy link
Contributor

@yunnydang yunnydang commented Aug 29, 2023

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

@yunnydang yunnydang requested a review from a team August 29, 2023 02:25
@yunnydang yunnydang self-assigned this Aug 29, 2023
Copy link
Contributor

@rachelkirk rachelkirk left a 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Two things here:

  1. 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)
  2. 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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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!

@yunnydang yunnydang force-pushed the add_new_fields_to_rapyd branch 2 times, most recently from f45211d to a07eb5f Compare August 31, 2023 20:31
Copy link
Contributor

@rachelkirk rachelkirk left a 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)

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
@yunnydang yunnydang merged commit f2b2fcb into master Sep 6, 2023
5 checks passed
@yunnydang yunnydang deleted the add_new_fields_to_rapyd branch September 6, 2023 18:30
This pull request was closed.
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