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

Worlpay: Fix Google Pay #4774

Merged
merged 1 commit into from
May 31, 2023
Merged

Worlpay: Fix Google Pay #4774

merged 1 commit into from
May 31, 2023

Conversation

almalee24
Copy link
Contributor

Ensure that we don't send cardHolderName if empty and that Google Pay and Apple Pay fall into the network tokenization code path and not the credit card path.

Remote Tests:
100 tests, 416 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed

Unit Tests:
107 tests, 633 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed

@almalee24 almalee24 requested a review from a team May 11, 2023 18:22
@@ -6,6 +6,7 @@ def setup
@cftgateway = WorldpayGateway.new(fixtures(:world_pay_gateway_cft))

@amount = 100
@year = (Time.now.year + 2).to_s[-2..-1].to_i
Copy link
Contributor

@BritneyS BritneyS May 12, 2023

Choose a reason for hiding this comment

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

Nice. It may be controversial, but I'm a fan of negative indices in Ruby. Very straightforward and readable.

else
{ payment_type: :credit }
type = payment_method.respond_to?(:source) ? :network_token : :credit
Copy link
Contributor

@BritneyS BritneyS May 12, 2023

Choose a reason for hiding this comment

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

Can you add an assertion to existing tests for Google Pay/Apple Pay that confirms that they are now going through the network tokenization path?

Copy link
Contributor Author

@almalee24 almalee24 May 12, 2023

Choose a reason for hiding this comment

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

Yes I can. The weird thing is that we are going through the network tokenization path with the current way which makes me think that we are saving Google or Apple Pay as NetworkTokenizationCreditCard but as CreditCard

def test_successfully_attaching_payment_type
[@credit_card, @nt_credit_card].each do |pm|
payment = @gateway.send(:payment_details, pm)[:payment_type]
if pm.respond_to?(:source)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to split these into separate tests to keep the passing conditions clear. This test may result in a false positive later on if, for example, a payment method contains a source field and it shouldn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wouldn't be possible for CreditCard to have the source field but I can still separate.

Copy link
Contributor

@BritneyS BritneyS left a comment

Choose a reason for hiding this comment

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

🚀

Ensure that we don't send cardHolderName if empty and that Google Pay
and Apple Pay fall into the network tokenization code path and not
the credit card path.

Remote Tests:
100 tests, 416 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Unit Tests:
107 tests, 633 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
@almalee24 almalee24 merged commit 51c1f2d into master May 31, 2023
@almalee24 almalee24 deleted the worldpay_network_tokens branch May 31, 2023 17:48
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