-
Notifications
You must be signed in to change notification settings - Fork 210
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
Fix syncing of local tokens with remote payment methods #2897
Conversation
7688801
to
c1257be
Compare
b8fd29d
to
70ae700
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.
I had already half reviewed this PR last Friday so decided to finish my review. I have tested the scenarios mentioned in the description and got the following outcomes.
- Pulling remote payment methods ✔️
- Deleting local payment methods when removed remotely ✔️
- Regression tests (tested with WooPayments by saving a card) ✔️
- Listing saved APMs (tested by purchasing a subscription using SEPA) ✔️
- Disabling specific payment method types with saved payment methods (for iDeal and Bancontact) ✔️
I found 2 problems.
- When sepa is enabled, I see it on the checkout page and even save the method while purchasing a subscription but I don't see it on
My Account
page.
- I can not add a payment method from
My Account
page. I get the following error
Notice: Undefined index: return_url in /var/www/html/wp-content/plugins/woocommerce-gateway-stripe/includes/class-wc-stripe-intent-controller.php on line 959 {"success":true,"data":{"status":"succeeded","id":"seti_1OkQJ2CBKj0yxC0ZfaFbFojV","client_secret":"seti_1OkQJ2CBKj0yxC0ZfaFbFojV_secret_PZZRODQG5VdX2jq6fUlByCTtxWcipL0"}}
The issues seem unrelated to this PR. Let me know your thoughts.
Note: This one is not important but thought I would add it discovered during testing. Adding/deleting a payment method works find both ways to/from Stripe Dashboard and My Account page. When I set a default method on My account page it's reflected on the Stripe Dashboard. But setting default from Stripe does not reflect on the My account page. It's a unlikely scenario though, as a merchant won't be setting a default for a shopper.
…_gateway Using this instead of creating a new instance of WC_Stripe_UPE_Payment_Gateway to avoid potential side-effects.
Thanks for the thorough review, @Mayisha !
There's a GH issue for this over here #2907
Good catch. I can replicate in
I can replicate this in |
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 Danae for the fix. Addressing the two issues with their separate tickets sounds good.
Do you see any potential problems with keeping it as is?
Also I don't see any problems here as it is not anything that significaant.
Fixes #2896
Changes proposed in this Pull Request:
Testing instructions
Pulling remote payment methods
https://dashboard.stripe.com/test/customers/cus_xxxx
4000056655665556
LIKE '%stripe_payment_methods_%'
, or assignfalse
to this variableDeleting local payment methods when removed remotely
https://dashboard.stripe.com/test/customers/cus_xxxx
LIKE '%stripe_payment_methods_%'
, or assignfalse
to this variableRegression tests
Listing of payment methods from other extensions
Listing saved APMs
Disabling specific payment method types with saved payment methods
changelog.txt
andreadme.txt
(or does not apply)Post merge