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

Fix manual renewal subscriptions that use SEPA when disabling the Legacy experience #3139

Merged
merged 57 commits into from
Jun 23, 2024

Conversation

a-danae
Copy link
Contributor

@a-danae a-danae commented May 20, 2024

Fixes #3133

Changes proposed in this Pull Request:

  • Introduce a migrator class extending WCS_Background_Repairer
  • Select the subscriptions using legacy SEPA tokens (those with the stripe_sepa gateway ID) to be migrated
  • For each subscription, create an updated SEPA gateway token if none exists.
    This isn't needed to fix the problem, but doing it for consistency for the shoppers. Assuming it could be strange for shoppers to see an active subscription but no tokens.
  • Update the subscription payment method type to use the updated SEPA stripe_sepa_debit gateway ID.
    This is what fixes the actual problem

Testing instructions

ℹ️ Since 8.2.0, legacy SEPA payment methods aren't attached to the customer for some reason. Check out 8.1.0 to add legacy SEPA payment methods if you want to test processing renewals. I'll create an issue to check this out.

Setup

  • Install and activate WC Subscriptions
  • Create a subscription product
  • Create a customer user to be used as the shopper
  • Set the store currency to Euro
  • Create a page using the classic checkout if you don't have one already
  • Set up webhooks
  • Under the payment methods tab, enable SEPA. At wp-admin/admin.php?page=wc-settings&tab=checkout&section=stripe&panel=methods

Create subscriptions with the Legacy SEPA gateway

  1. Log in as the shopper
  2. Go to My Account > Payment methods > Add payment method, at /my-account/add-payment-method/
  3. Add three SEPA payment methods.
    Find test IBAN numbers here. AT611904300234573201, DE89370400440532013000, EE382200221020145685
  4. Add a card payment method
  5. As the shopper, purchase the subscription product four times - one with each payment method.
    You'll need to use the shortcode checkout. Legacy SEPA doesn't show up in Blocks
  6. Confirm that
    • All subscriptions are active
    • Each of them display the selected payment method
    image
  7. Go to the Edit page for each subscription at /wp-admin/admin.php?page=wc-orders--shop_subscription&action=edit&id=< sub ID >
  8. Under "Payment method", confirm SEPA Direct Debit is displayed
    image

At this point:

  • Going into the database > the wc_orders table > these SEPA subscriptions will have stripe_sepa under the payment_method column
  • The Legacy SEPA payment gateway is enabled on the Stripe extension, which uses the stripe_sepa gateway ID

See the issue

  1. Disable the Legacy experience
  2. As a merchant, go to the Edit page for each subscription
  3. Notice that under "Payment method", it now says "Manual renewal"

At this point:

  • The Legacy SEPA payment gateway (stripe_sepa) is disabled
  • The Updated SEPA payment gateway (stripe_sepa_debit) is enabled instead
  • WooCommerce flags the payment gateway used for the subscription is disabled, thus Subscription changes the subscription to Manual renewal

Confirm the right subscriptions are queued to be updated - actually testing the fix now

  1. As a merchant, go to the Scheduled Action page
  2. Confirm that there's a scheduled action called wc_stripe_schedule_subscriptions_legacy_sepa_token_repairs
    image
  3. Click on "Run now", or wait for the action to run
    This action will schedule a new action for each subscription that needs to be updated
  4. If you let it execute on its own, confirm that another wc_stripe_schedule_subscriptions_legacy_sepa_token_repairs doesn't get scheduled for later
  5. Search for actions called wc_stripe_subscriptions_legacy_sepa_token_repair
  6. Confirm there are three actions scheduled.
  7. Confirm the ID of the subscription each one of them targets belongs to a subscription purchased with the Legacy SEPA gateway
  8. Confirm that the subscription purchased with the card isn't listed
    image

Confirm the subscriptions' payment method gets updated

  1. As a merchant, go to the Scheduled Action page
  2. Search for actions called wc_stripe_subscriptions_legacy_sepa_token_repair
  3. Run the actions for each subscription. Or, preferably, wait for them to run
  4. As a merchant, go to the Edit page for each subscription
  5. Confirm that under "Payment method", it now says "SEPA Direct Debit"
  6. On the WooCommerce logs, at wp-admin/admin.php?page=wc-status&tab=logs, confirm that there are entries for each subscription that was updated

What we're doing for each subscription:

  • We retrieve all of the stripe_sepa_debit tokens for the user that owns the subscription.
  • When calling WC_Payment_Tokens::get_customer_tokens(), we create tokens using the Updated SEPA gateway (stripe_sepa_debit) from the Legacy tokens (stripe_sepa), over here
  • We update the payment_method of the subscription, from stripe_sepa to stripe_sepa_debit, which matches the gateway ID of the Updated SEPA gateway
  • WooCommerce no longer flags the payment gateway used for the subscription as disabled; thus Subscription is no longer flagged under "Manual renewal"

Unhappy paths

  • No action must be scheduled when WC_Subscriptions isn't active
  • No action must be scheduled when the Legacy experience is enabled

  • Covered with tests (or have a good reason not to test in description ☝️)
  • Added changelog entry in both changelog.txt and readme.txt (or does not apply)
  • Tested on mobile (or does not apply)

Post merge

a-danae added 17 commits May 31, 2024 20:45
With this change, the subscription meta for flagging that the migration took place won't skip the migration. Also, we're including the check for whether the Updated checkout experience is enabled into the method that would trigger an exception when the migration is skipped, so the reason gets logged.
@a-danae a-danae marked this pull request as draft June 18, 2024 17:59
@a-danae
Copy link
Contributor Author

a-danae commented Jun 18, 2024

@james-allan , just a heads up that I re-requested your review, but I'd like to adjust if following your comments before reviewing it again. I marked this as a draft in the meantime. Ok, marking this as ready to review. Planning to address the comments in separate PRs.

@a-danae a-danae marked this pull request as ready for review June 18, 2024 22:26
*
* This class extends the WCS_Background_Repairer for scheduling and running the individual migration actions.
*/
class WC_Stripe_Subscriptions_Legacy_SEPA_Token_Update {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm separating the logic for updating the token from the background repairer class to be reused in the woocommerce_scheduled_subscription_payment action. Planning to do that in a separate PR.

@james-allan
Copy link
Contributor

I also want to consider if there's any improvements we can make to the loading of this class. ie when we know we're done, stop loading it.

Makes sense. Checking. Would this be a blocker?

Nah, not a blocker.

Hadn't thought of that, sounds great. Would it be okay to implement this approach in a separate PR? Thinking it'd be an scenario that could be tested separately, and thinking of not making this PR bigger and not having more testing steps for it.

Yeah that sounds like a good idea, this PR is tough to test so it would be good to move those additional things off to separate PRs.

Copy link
Contributor

@james-allan james-allan left a comment

Choose a reason for hiding this comment

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

The changes tested well again today. I left a minor comment but I'm approving this PR as it's not blocking and it would be good to progress this further.

@james-allan
Copy link
Contributor

@a-danae I also wanted to flag that after the payment method is migrated, I tried to renew the subscription, however I got this error from Stripe.

Screenshot 2024-06-21 at 1 11 53 PM

What I think is causing that is we're using the payment intents API to try collect a payment with a src payment method.

But I'm not 100% sure. Just wanted to flag so you were aware.

@james-allan
Copy link
Contributor

But I'm not 100% sure. Just wanted to flag so you were aware.

It could also have something to do with the fact that my src_ payment methods have a status of consumed and so they aren't chargeable anymore. I'm not sure what's behind that.

a-danae and others added 2 commits June 21, 2024 15:37
…n-update.php

Co-authored-by: James Allan <james.allan@automattic.com>
@a-danae
Copy link
Contributor Author

a-danae commented Jun 23, 2024

Summarizing here the pending items mentioned in the review that will be addressed separately:

@a-danae a-danae merged commit d47cdf6 into develop Jun 23, 2024
33 checks passed
@a-danae a-danae deleted the fix/manual-renewal-subscriptions-with-sepa branch June 23, 2024 11:12
@james-allan
Copy link
Contributor

Summarizing here the pending items mentioned in the review that will be addressed separately:

Are there any you want some help with @a-danae?

@agenceKanvas
Copy link

I'm a little scared to try it actually. Is there a way to launch the migrator on 4 ou 5 subscriptions and see if it's working on production before launching it globally ?

@a-danae
Copy link
Contributor Author

a-danae commented Jul 2, 2024

Are there any you want some help with @a-danae?

That'd be great! I'm creating issues for these points now so we can distribute them. Big thanks @james-allan !

Is there a way to launch the migrator on 4 ou 5 subscriptions and see if it's working on production before launching it globally ?

@agenceKanvas sure, it makes sense to test with a few of them first. We don't have a way to control them right now, but adding a filter when retrieving the items to repair would do the trick; it'd allow merchants to control this if needed and it would be an easy addition.

@james-allan happy to go differently if you disagree! Thinking of adding a filter to the return value of get_items_to_update()

@james-allan
Copy link
Contributor

@james-allan happy to go differently if you disagree! Thinking of adding a filter to the return value of get_items_to_update()

We'd need to be careful not to set the option that then prevented it from running for the remaining subscriptions or I guess we could just include a note that after running you're test you need to delete the option to trigger it to run for the remaining ones.

@agenceKanvas
Copy link

@a-danae , I've tried the testing scenario on my staging site and the wc_stripe_schedule_subscriptions_legacy_sepa_token_repairs did not appear (v8.5.0-test).
Is there a way to call it manually ?

@james-allan
Copy link
Contributor

james-allan commented Jul 11, 2024

Is there a way to call it manually ?

@agenceKanvas if you have access to run code directly, something like this will kick it off.

$logger  = wc_get_logger();
$updater = new WC_Stripe_Subscriptions_Repairer_Legacy_SEPA_Tokens( $logger );
$updater->schedule_repair();

Important

Once you run it, you will want to remove this code because you don't want multiple instances of the WC_Stripe_Subscriptions_Repairer_Legacy_SEPA_Tokens class running on your site.

@a-danae
Copy link
Contributor Author

a-danae commented Jul 14, 2024

(v8.5.0-test).

Hey @agenceKanvas, just a heads up that Stripe 8.5.1 is out and contains this fix, so you can use that instead of the test release in GH.

@agenceKanvas
Copy link

Hi @a-danae !
I've tried to run the commands. This is the result :

  • Commands runned with no error displayed
  • Event listed in the scheduled one, run manually and finished
  • The 3 subscriptions that I created following the procedure were still in manual renewal state
    I'm going to import the db export that I made before the command, update the plugin and check again.
    If that's not working, what data would you need to see what could have happen ?
    It's a staging server so if you need access I can provide one.
    Thanks for your help !

@agenceKanvas
Copy link

agenceKanvas commented Jul 15, 2024

So, I reinstalled the backup of the database, before the plugin update, I installed the 8.5.1, and the event was not triggered again.
I tried to run it manually with success :
Capture d’écran 2024-07-15 à 11 09 34

But the subs are sill in manual renewal :
Capture d’écran 2024-07-15 à 11 12 11

** Update **
I spoke too fast. When I go in the DB and query the post meta related to payment of the sub, I have what I'm supposed to have :
Capture d’écran 2024-07-15 à 15 41 40
The only problem, I think, is that the next payment date outdated.
Because this is a staging website, the payments are locked in manual renewal.
When it goes live, will the date be updated automatically ?
Thanks a lot !

@a-danae
Copy link
Contributor Author

a-danae commented Aug 28, 2024

Hi there @agenceKanvas ! Glad to hear the meta was updated as expected

Not sure if you got this sorted, but just in case...

When it goes live, will the date be updated automatically ?

The date wouldn't be updated as we only change the payment-related meta for the migration. Have you encountered any issues with payment renewal dates having passed? We also hook the migration on renewals, so if it's on Manual renewal and the renewal date comes, the payment will go through and the payment method will be updated for upcoming renewals.

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.

Subscriptions using SEPA change to Manual Renewal after disabling the Legacy checkout experience
4 participants