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

CheckoutV2: add sender object for purchase and auth #5124

Merged
merged 1 commit into from
May 21, 2024

Conversation

yunnydang
Copy link
Contributor

This allow us to add the sender object to auth and purchase.

Note: There is a discrepancy with the address fields for recipient and sender. Recipient is expecting address_line1/address_line2. Sender is expecting address1/address2 since sender for for payout is already using those field names. Will have to document this.

Local:
5903 tests, 79624 assertions, 0 failures, 7 errors, 0 pendings, 0 omissions, 0 notifications
99.8814% passed

Unit:
67 tests, 421 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remote:
106 tests, 255 assertions, 4 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
96.2264% passed

@yunnydang yunnydang requested a review from a team May 17, 2024 22:49
@yunnydang yunnydang self-assigned this May 17, 2024
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.

I left a comment, but not sure how quickly this fix needs to be addressed.

@@ -187,6 +188,36 @@ def add_processing_data(post, options)
post[:processing] = options[:processing]
end

def add_sender_data(post, options)
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of the fields in this sender method overlap with the fields I added in add_payout_sender_data. Would it be worth it to refactor that instead of creating another large method?

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 @rachelkirk I was thinking of implementing a refactor to your work you did for payouts but didnt want to take the risk of mucking about with it. My line of thinking was that I wanted to keep payout sender data separate from regular auth/purchase as it's less risky but Im happy to weigh in your thoughts! As for priority, its kinda high since this is blocking AFT transactions :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Squinting at the Checkout docs, there are some odd discrepancies between what they accept (or expect, even) on the payouts version of this sender object vs the payment version.

The strangest example is that in the payment scenario, the field is dob, while in the payout scenario, it's date_of_birth.

Then there are some fields marked as required for payouts that aren't evidently available in the case of a payment. (like reference_type and source_of_funds).

We'd also have to tinker with the payout == true logic that guards the payout sender method in order to combine these; even if it's an additional large method it might be cleaner to keep them separated. If we do it that way, I think I'd want to see this method renamed to add_payment_sender_data to distinguish it from payout (using the naming conventions in the CKO docs)

Copy link
Contributor

Choose a reason for hiding this comment

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

@yunnydang I think you'd also need to add the option for company_name in this method, since it's one of the fields marked required if the type is set to corporate

Copy link
Contributor

Choose a reason for hiding this comment

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

@yunnydang could you move the payout specific fields further down in the method and add the return unless there above them to guard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rachelkirk I could try to do that but as Joe mentioned above, theres some overlapping and non-overlapping fields (with differing field names yet expecting the same values) that I'd like to not tangle in with the payout method. With purchase and auth, I think it's easier/cleaner to have them separated since payout is a specific use case compared to opening up the various transaction cases on auth and purchase.

@yunnydang yunnydang force-pushed the checkout_add_sender_to_auth_purchase branch from d5e503a to 01e5b62 Compare May 20, 2024 22:06
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.

Looks good to me!

@yunnydang yunnydang force-pushed the checkout_add_sender_to_auth_purchase branch from 01e5b62 to 52f2295 Compare May 21, 2024 17:08
@yunnydang yunnydang merged commit 52f2295 into master May 21, 2024
0 of 5 checks passed
@yunnydang yunnydang deleted the checkout_add_sender_to_auth_purchase branch May 21, 2024 17:12
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.

None yet

3 participants