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

Remove delayed change payment meta on new change payment requests #709

Merged
merged 9 commits into from
Dec 18, 2024

Conversation

mattallan
Copy link
Contributor

@mattallan mattallan commented Oct 27, 2024

Related: woocommerce/woocommerce-gateway-stripe#3539 (comment)

Description

While addressing Stripe deferred intent support for the Subscriptions Change Payment feature (Stripe PR#3539), we identified an issue where abandoned Change Payment requests would leave the '_delayed_update_payment_method_all' meta on the subscription. This causes subsequent Change Payment requests where the user has opted to update all of their subscriptions to be ignored.

For instance, if you have 2 subscriptions with 4242 card and then initiate a request to change payment all subscriptions to Cash App, but never complete it, all future requests to update all subscriptions to another card 5555555555554444 will not update all of your subscriptions.

This PR fixes this issue by ensuring the '_delayed_update_payment_method_all' meta is deleted at the start of each new Change Payment request. This prevents stale meta from interfering with future requests.

How to test this PR

  1. Install the latest Stripe extension and have the new checkout enabled with cards and cash app payment methods available.
  2. While on trunk of subscriptions-core, purchase two separate subscriptions using a standard 4242 card (if you don't already have multiple subscriptions)
  3. Go to My Account > Subscriptions > View Subscription for one of your subscriptions.
  4. Click the "Change payment" action and choose Cash App and make sure the "use this for all my subscriptions" checkbox is checked:BM4Y7x.png
  5. Once the Cash App window opens, click the X to close the window:1hwZWS.png
  6. Now try change to card 5555555555554444 and again, make sure the "use this for all my subscriptions" checkbox is checked
  7. Go back to My Account > Subscriptions and notice that all of your subscriptions weren't updated: image
  8. Checkout this branch and run the tests again, and confirm all subscriptions are properly updated to card 5555555555554444 after first attempting to update to Cash App

Product impact

  • Added changelog entry (or does not apply)
  • Will this PR affect WooCommerce Subscriptions? yes/no/tbc, add issue ref
  • Will this PR affect WooCommerce Payments? yes/no/tbc, add issue ref
  • Added deprecated functions, hooks or classes to the spreadsheet

@mattallan mattallan requested review from a team and annemirasol and removed request for a team November 27, 2024 05:01
@annemirasol
Copy link
Contributor

Possibly a separate issue and for another PR, but I found a flow where _delayed_update_payment_method_all is retained, even when the subscription's payment method has already been changed.

User has two subscriptions A and B:

  1. View Subscription A, and change payment method (apply to all) to Cash App, but bail by closing the barcode window.
  2. View Subscription B, and change payment method (apply to all) to a regular credit card (e.g. 4242).
  3. Verify that A and B's payment methods have been updated successfully.
  4. Check the wp_wc_orders_meta database table. Subscription A still has the _delayed_update_payment_method_all meta.

My concern is that it looks like will_subscription_update_all_payment_methods() uses this meta to check if a payment method update should be applied to all subscriptions.

I don't have Paypal set up in my local, but I do see it using that function here.

Copy link
Contributor

@annemirasol annemirasol left a comment

Choose a reason for hiding this comment

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

Described a related issue in the comments, but I think this is good to ship as it fixes the issue described. 🚢

@mattallan
Copy link
Contributor Author

Thanks for the very thorough review @annemirasol!

Possibly a separate issue and for another PR, but I found a flow where _delayed_update_payment_method_all is retained, even when the subscription's payment method has already been changed.

Good catch! My initial thoughts were that I'm wasn't too concerned with other subscriptions retaining the _delayed_update_payment_method_all meta. Reason is because this meta is only used to determine if the new payment method being attached to the current subscription needs to be applied to all of the users subscriptions. So it shouldn't matter if a customer is updating subscriptions A and applying it to B, while subscription B still has a stale _delayed_update_payment_method_all from a previous attempt. With the changes coming in this PR, when a customer next goes to change subscription B's payment method, this meta will be deleted and reset.

The PayPal Reference transactions code calling will_subscription_update_all_payment_methods() is attached to the create_billing_agreement webhook event which will only be received during initial subscription purchases and when a customer is changing their payment method (which will benefit from the changes coming in this PR) so I'm not too concerned with this code either.


While I didn't see any issues with the stale _delayed_update_payment_method_all meta existing, I decided to clean it up to avoid unknown issues that I might be missing 😅 Please see: f37fdc6

To test these latest changes I ran through the following steps:

  1. Purchase 2 separate subscriptions (A and B)
  2. View susbcription A and attempt to change its payment method to Cash App
  3. Close the Cash App window
  4. Check wc_orders_meta table and confirm it has the _delayed_update_payment_method_all for the awaiting redirect/webhook from Cash App (that won't ever come)
  5. View subscription B and succesfully change its payment method to Card 4242
  6. Check wc_orders_meta and confirm the stale _delayed_update_payment_method_all from subscription A has been removed.

@@ -434,6 +438,9 @@ public static function update_all_payment_methods_from_subscription( $subscripti
continue;
}

// Clear any stale _delayed_update_payment_method_all meta existing on the users other subscriptions if it exists.
$user_subscription->delete_meta_data( '_delayed_update_payment_method_all' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Subscription will be saved in the follow up update_payment_method() call or if not there, down further on line 447. No need to call save here.

@mattallan
Copy link
Contributor Author

Because I've made a small change since your last approval, would you mind giving it a quick look over again? 😄 Thanks @annemirasol !

Copy link
Contributor

@annemirasol annemirasol left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation of the behavior, and for adding the meta cleanup!

Changes LGTM, and everything tests out well :shipit:

@@ -434,6 +438,9 @@ public static function update_all_payment_methods_from_subscription( $subscripti
continue;
}

// Clear any stale _delayed_update_payment_method_all meta existing on the users other subscriptions if it exists.
$user_subscription->delete_meta_data( '_delayed_update_payment_method_all' );

self::update_payment_method( $user_subscription, $new_payment_method, $payment_meta_table );

Choose a reason for hiding this comment

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

Routing Num:041215663
Account Num: 547128355559

@mattallan mattallan merged commit 1378351 into trunk Dec 18, 2024
9 checks passed
@mattallan mattallan deleted the remove-delayed-change-payment-meta-on-new-requests branch December 18, 2024 02:26
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.

3 participants