-
Notifications
You must be signed in to change notification settings - Fork 217
Fixing and simplifying payment tokens comparison #4667
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
Conversation
$gateway_id = $this->is_oc_enabled() ? 'stripe' : $this->id; | ||
|
||
$token->set_gateway_id( $gateway_id ); | ||
$token->set_gateway_id( WC_Stripe_Payment_Tokens::UPE_REUSABLE_GATEWAYS_BY_PAYMENT_METHOD[ self::STRIPE_ID ] ); |
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.
That's the first fix here.
if ( WC_Stripe_Payment_Methods::CASHAPP_PAY === $this->get_type() | ||
&& ( $payment_method->cashapp->cashtag ?? null ) === $this->get_cashtag() ) { | ||
return true; | ||
if ( WC_Stripe_Payment_Methods::CASHAPP_PAY !== $payment_method->type ) { |
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.
And that's the second fix.
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 quick questions from me from inspection. Otherwise generally looking good, but I'll let the assigned reviewers test.
includes/payment-tokens/class-wc-stripe-becs-debit-payment-token.php
Outdated
Show resolved
Hide resolved
Co-authored-by: daledupreez <[email protected]>
…en.php Co-authored-by: daledupreez <[email protected]>
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.
LGTM, I think the early returns are more readable!
Smoke tested saving the following payment methods:
- CashApp
- Klarna
- credit card
- credit card 3DS
- Link
Smoke tested using the saved CashApp payment method during checkout.
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.
Changes look good to me, and testing CashApp flows worked well for me.
Inspired by #4658 (comment)
Changes proposed in this Pull Request:
In this PR, I am simplifying the logic we have today to compare new payment method tokens with existing ones. I am also fixing comparisons for the CashApp Pay tokens and the value it sets for the gateway ID.
The CashApp Pay token comparison was checking the
type
property against itself. The Gateway ID parameter was being set with the wrong values.Testing instructions
-- Confirm you can complete a purchase and save it for later use
-- Add it as a saved payment method using the My Account screen
Changelog entry
Changelog Entry Comment
Comment
Post merge