-
Notifications
You must be signed in to change notification settings - Fork 207
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
Update the subscriptions sources to payment methods if they were migrated #3249
Update the subscriptions sources to payment methods if they were migrated #3249
Conversation
…scription The renewal will use the payment method ID stored in the subscription metadata, no token is needed. The token will be created from the payment methods in the Stripe account when we list the tokens for the shopper.
Nice work @a-danae! I left some comments. Some are minor typos comments but the others are critical that we fix. :) I confirmed that signing up to the Subscription using the
I then disabled Legacy checkout mode - as you mentioned the subscription temporarily changes to manual. After processing a renewal again, the payment is successful using the new token and the subscription has migrated to the new
As part of reviewing this PR I also tested 2 other scenarios:
Each time the subscription renews it will continue adding this note (screenshot). Some may consider that overkill but it's good that it records that we've at least attempted to migrate it.
Once again some really good work @a-danae! |
includes/compat/class-wc-stripe-subscriptions-legacy-sepa-token-update.php
Outdated
Show resolved
Hide resolved
includes/migrations/class-wc-stripe-subscriptions-repairer-legacy-sepa-tokens.php
Outdated
Show resolved
Hide resolved
includes/migrations/class-wc-stripe-subscriptions-repairer-legacy-sepa-tokens.php
Show resolved
Hide resolved
includes/migrations/class-wc-stripe-subscriptions-repairer-legacy-sepa-tokens.php
Outdated
Show resolved
Hide resolved
includes/compat/class-wc-stripe-subscriptions-legacy-sepa-token-update.php
Outdated
Show resolved
Hide resolved
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.
To streamline the review I'm going to approve the PR. I've made some suggested code changes which are important that we fix.
…n-update.php Co-authored-by: James Allan <[email protected]>
…P < 8.0 Co-authored-by: James Allan <[email protected]>
Co-authored-by: James Allan <[email protected]>
Critical indeed, great catch on the
Ah yeah, I thought it might be overkill, but exactly. Thinking it'd be good to debug the potential scenario of renewals failing due to Stripe deprecating the use of Sources. Glad to update it if a different approach would be better 🙂 Thanks for the thorough review, as always @james-allan ! |
Changes proposed in this Pull Request:
Stripe accounts will undergo a migration to move their Sources to PaymentMethods. This PR updates the payment method associated with subscriptions from src_ to the migrated pm_, if the migration already took place in the account.
Testing instructions
Setup
Testing the migration and subsequent renewals
If you use my account, use customer cus_PuO841WftotzWl
On the usermeta of a shopper, set wp__stripe_customer_id to cus_PuO841WftotzWl
There are two SEPA payment methods that end in 3201. One of them is a src_ src_1PEHjXGwE7I87pM0KSAsG4If , and the other is a pm_ created by the migration pm_1PG2BmGwE7I87pM0FaxCi8UN
pm_
changelog.txt
andreadme.txt
(or does not apply)Post merge