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

Split PE - Fixes Woo Subscriptions support when using card payments with dPE #2866

Merged
merged 14 commits into from
Feb 6, 2024

Conversation

mattallan
Copy link
Contributor

@mattallan mattallan commented Jan 29, 2024

For #2777

Changes proposed in this Pull Request:

This PR fixes compatibility issues with Woo Subscriptions and our new Split PE with deferred intent changes. In my testing there were three main issues I found:

  1. Subscriptions purchased with an existing saved card remain on-hold after processing a renewal.
  2. Attempting to purchase a subscription that had a free trial or was synced, resulted in a generic error on the checkout.
  3. Error when attempting to change a subscription's payment method to an existing card.

I have addressed the above issues in the following commits:

  • e2fa15d - fixes the first issue by making sure we call maybe_update_source_on_subscription_order() whenever we're using an existing saved payment method so that the pm_ and cus_ tokens are saved on the subscription. This function was already being handled when a new card was being saved (see inside handle_saving_payment_method()), but not when an existing card was used.
  • 9bc5fc6 - fixes the second issue by adding support for creating setup intents inside process_payment_with_deferred_intent() for purchases that don't have an upfront payment but still require the card to be taken for future payments i.e. a subscription with a free trial. This single commit ended up being a bit larger than I typically like however, it can be broken down into the following changes:
    • Refactor the intent_controller->create_and_confirm_setup_intent() function so that it can handle requests from the front-end (My Account > Payment methods > Add new) and our process_payment_with_deferred_intent() function.
    • Update the process_payment_with_deferred_intent() to add functionality when $payment_needed is false. When no payment needed and we're inside the process_payment flow, it's expected for a setup intent to be created.
    • I moved the code that saves the stripe meta to the order and the handling of saving a new card outside of the needs_payment condition so that it's run when we're using payment intents and setup intents.
    • When payment is not needed, don't process the charge and instead mark the order as complete.
  • 86d3d62 - fixes the final issue by not calling processPayment when a customer is changing payment method to an existing saved card.

Testing instructions

Before testing, make sure you have:

  • the latest Woo Subscriptions installed and activated,
  • a simple subscription product and another with a free trial period

There are a lot of subscription flows to cover and rather than go through them all here, I'll link to our testing instructions for the critical subscription flows:

Purchasing and renewing subscriptions

  1. Add the simple subscription product to your cart
  2. Navigate to the checkout page and confirm only reusable payment methods are visible
  3. Complete the checkout using a new card
  4. Visit WP Admin > WooCommerce > Subscriptions and view the newly purchased subscription
  5. On the Edit Subscription admin page, click on the pencil icon in the Billing Details section and confirm the Stripe customer ID and source ID (pm_1234) are correctly saved on the subscription image
  6. Use the Order Actions to process an automatic renewal for the subscription: image
  7. Confirm the renewal order is created and is processing, and confirm the subscription is active.
  8. Confirm the payment in Stripe

Purchasing and renewing subscriptions with setup intents

  1. Follow the above steps except this time add a subscription product with a trial period
  2. Confirm the new card you added above, is now in your list of saved cards (to confirm new cards used to purchase subscriptions are always saved)
  3. Use a saved card this time instead of adding a new one
  4. Confirm there's no payment for the initial purchase.
  5. After processing a renewal order, confirm the charge in Stripe.

Changing subscriptions payment method

Note

Before running this test, make sure you have at least 2 active subscriptions to test updating all payment methods.

  1. From the frontend of your store, go to My Account > Subscriptions and click to view one of your active subscriptions
  2. Click on the Change payment buttonimage
  3. Choose a new card number that differs from your existing card i.e. if your subscription has the visa 4242424242424242, change the payment to mastercard 5555555555554444 so it's easier to notice the difference
  4. Check the "Update the payment method used for all of my current subscriptions (optional)" box and submit the form
  5. Go back to My Account > Subscriptions to confirm all of your active subscriptions now have the updated payment method image
  6. Process a subscription renewal using the Edit Subscription actions shown in the previous tests and confirm the new/changed card is used to process the subscription.

  • 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

@mattallan mattallan requested review from a team and james-allan and removed request for a team January 29, 2024 06:32
@mattallan mattallan marked this pull request as ready for review January 29, 2024 06:33
@mattallan
Copy link
Contributor Author

mattallan commented Jan 29, 2024

I've performed the following tests on this PR:

Classic checkout/shortcode

  • Purchase a simple subscription 👍
    • Using an existing saved card
    • Using a new card (confirmed card is saved)
  • Purchase trial/synced subscription 👍
    • Using an existing saved card
    • Using a new card (confirmed card is saved)
    • Confirm setup intent is created
  • Purchase multiple subscriptions 👍
    • Confirm pm_ and cus_ ids are present on all active subscriptions
  • Confirm payment methods displayed on checkout when a subscription is in the cart 👍
  • Manually pay for a pending renewal order 👍
  • Renew early via shortcode checkout from My Account > Subscriptions > View Subscription > Renew now 👍
  • Automatic subscription renewal for subscriptions purchased on classic checkout (both simple & trial products) 👍

Checkout Blocks

  • Purchase a simple subscription 👍
    • Using an existing saved card
    • Using a new card (confirmed card is saved)
  • Purchase trial/synced subscription 👍
    • Using an existing saved card
    • Using a new card (confirmed card is saved)
  • Purchase multiple subscriptions 👍
    • Confirm pm_ and cus_ ids are present on all active subscriptions
  • Confirm payment methods displayed on checkout when a subscription is in the cart 👍
  • Manually pay for a pending renewal order 👍
  • Renew early via checkout block from My Account > Subscriptions > View Subscription > Renew now 👍
  • Renew subscriptions automatically for subscriptions that were purchased on checkout block 👍

Other subscription-related tests

  • Change payment method 👍
    • New card
    • Existing card
    • Update all existing subscriptions with new card
    • Update all existing subscriptions with existing card
  • Renew early via modal 👍

@james-allan
Copy link
Contributor

Hey @mattallan I've been trying to make some subscription purchases using other payment methods like Bancontact. Stripe's doc suggest that Bancontact can be used for recurring revenue via SEPA Debit. Stipe will do the Bancontact -> SEPA stuff under the hood as long you send the required setup intent, off session args etc.

However when I attempted to purchase a subscription using Bancontact I got a number of rolling issues:

  1. We need to send mandate data when using Bancontact with setup intents for off session payments. See stripe error below.
  2. When I resolved that issue, I then had a different issue when create_payment_token_for_user() is called. The $payment_method doesn't have any last and so the creating of a WC_Payment_Token_SEPA fails.
Screenshot 2024-02-01 at 3 53 30 pm

I'm not sure how you think we should proceed here. If theres a lot of unknowns here perhaps we limit the scope of this PR to card payments and work on other APMs in a separate PR. Thoughts?

@james-allan
Copy link
Contributor

james-allan commented Feb 1, 2024

Stripe's doc suggest that Bancontact can be used for recurring revenue via SEPA Debit.

WooPayments have Bancontact as not a reusable payment method so perhaps there's more to this that makes it not possible. 🤔

@james-allan james-allan closed this Feb 1, 2024
@james-allan james-allan reopened this Feb 1, 2024
@james-allan
Copy link
Contributor

james-allan commented Feb 1, 2024

I've been trying to make some subscription purchases using other payment methods like Bancontact.

So it turns out I had manual payments enabled with Subscriptions which is why Bancontact was showing up originally. If I disable manual payments, it is no longer shown.

That changes the question slightly because we do still need to resolve those errors but I wonder if we need to change it so we only create set up intents for off session payments if we're creating an automatic subscription. 🤔

@mattallan
Copy link
Contributor Author

@james-allan Thanks for taking a look at this one.

Since some of our APMs are listed as reusable, they should still technically work for subscription purchases and so I had a look at what is stopping us from accepting an APM like Bancontact to purchase a subscription product.

Here's what I found needs to change:

  • add 'subscriptions' to list of $supports flags on each UPE payment method that is reusable.
  • update our is_mandate_data_required() function so that it also includes bancontact (which uses SEPA for tokenization)
  • Need to update our process_payment_with_deferred_intent() function so that it doesn't try to save a payment token until after the customer has gone through the confirmation/authorization pop-up. This is just a theory but, to add a WC token we need the last4 value and we don't have the last4 value for our redirecting payment methods until the customer has been redirected. I think we have to add/save the payment token on redirect for these.

There's a fair bit to change here but I'm happy to continue adding those changes to this PR unless you think it's best to separate them?
Smaller PRs might be better but I don't mind either way! Do you have a preference?

@james-allan
Copy link
Contributor

Need to update our process_payment_with_deferred_intent() function so that it doesn't try to save a payment token until after the customer has gone through the confirmation/authorization pop-up. This is just a theory but, to add a WC token we need the last4 value and we don't have the last4 value for our redirecting payment methods until the customer has been redirected. I think we have to add/save the payment token on redirect for these.

I'm not sure how far down this rabbit hole you went but from my testing payment methods like Bancontact don't return the last 4 even once you've approved the payment and returned to the store.

Bancontact SEPA
Screenshot 2024-02-02 at 11 00 53 am Screenshot 2024-02-02 at 11 00 26 am

Above is the response from the /payment_methods endpoint via the WC_Stripe_API::get_payment_method() function.

You'll notice that the SEPA PM response includes additional details like last4 whereas the Bancontact has essentially no information. Looking at Stripe docs, this appears to be the correct response. The Bancontact arg has no child args, but SEPA does. I think (I could be wrong) but this could mean Bancontact PMs cannot ever return a last4.

Smaller PRs might be better but I don't mind either way! Do you have a preference?

I think its fair to move this to a separate issue given there's additional research we'll need to do to investigate if Bancontact can be used as a saved token. As I mentioned, WooPayments have it as a non-resuable method, our Stripe plugin is torn on the topic because the WC_Gateway_Stripe_Bancontact class doesn't list tokenization or subscriptions as a feature of that gateway, but the WC_Stripe_UPE_Payment_Method_Bancontact class says it is reusable. 🤷

@mattallan
Copy link
Contributor Author

Thanks @james-allan for going further than I did! I didn't get that far to confirm whether the last4 would be on the payment method object after finishing/confirming the Bancontact payment.

I think its fair to move this to a separate issue given there's additional research we'll need to do to investigate if Bancontact can be used as a saved token. As I mentioned, WooPayments have it as a non-resuable method, our Stripe plugin is torn on the topic because the WC_Gateway_Stripe_Bancontact class doesn't list tokenization or subscriptions as a feature of that gateway, but the WC_Stripe_UPE_Payment_Method_Bancontact class says it is reusable.

I agree with this comment and happy to handle this in a separate PR as it sounds like we'll need to do a bit more investigation into supporting automatic subscription renewals using our APMs.

@mattallan mattallan changed the title Split PE - Fixes Woo Subscriptions support when using dPE Split PE - Fixes Woo Subscriptions support when using card payments with dPE Feb 2, 2024
@james-allan
Copy link
Contributor

I agree with this comment and happy to handle this in a separate PR as it sounds like we'll need to do a bit more investigation into supporting automatic subscription renewals using our APMs.

Ok, I've created the issue to add the other APMs here: #2872

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.

Changes look good @mattallan.

I've tested the following scenarios:

  1. Standard subscription purchase. 👍
  2. Free trial subscription. 👍
  3. Signing up using a saved card. 👍
  4. Early renewal order processed 👍
  5. Sign up to free trial using 3DS card ❌ I got an invalid_intent_id error message on checkout and the following error in my logs:
PHP Warning:  Undefined property: stdClass::$latest_charge in /wp-content/plugins/woocommerce-gateway-stripe/includes/abstracts/abstract-wc-stripe-payment-gateway.php on line 717
Screenshot 2024-02-02 at 5 32 43 pm

FWIW the invalid intent error appears to be caused because the order doesn't have an intent stored on it but the update_order_status_ajax() request receives a setup intent id. ie seti_1OfHMMCM0qfKgPGttSXXo0ZI !== ''.

I think this means update_order_status_ajax() needs to be changed to handle receiving setup intents and intents. eg if it recevies a set up intent, see if the order has a corresponding set up intent. At the moment it just pulls the payment intent order meta which is empty in this case.

  1. Standard sign up using 3DS card 👍
  2. Renewal run via Action scheduler. 👍
  3. Multiple subscriptions (weekly and monthly) 👍

mattallan and others added 2 commits February 5, 2024 10:49
Co-authored-by: James Allan <james.allan@automattic.com>
Co-authored-by: James Allan <james.allan@automattic.com>
@mattallan
Copy link
Contributor Author

Thanks for taking a look and testing this @james-allan

Sign up to free trial using 3DS card ❌

Good catch! I'll look into this one!

@mattallan
Copy link
Contributor Author

@james-allan I've just pushed up 1668359 to fix the issue with using a 3DS card to purchase subscription product with no upfront cost (i.e. trial/synced).

Something I also noticed while reviewing the code is that I was completing orders (and activating subscriptions) before the customer is redirected to an authentication modal. I pushed up 3eecfc4 to fix this.

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.

Changes look good and work well. I just noticed a slightly confusing sentence in the new function comment you introduced. Otherwise this looks good to go.

Co-authored-by: James Allan <james.allan@automattic.com>
@mattallan mattallan merged commit d86142c into add/deferred-intent Feb 6, 2024
32 checks passed
@mattallan mattallan deleted the fix/2777-dPE-subscription-support branch February 6, 2024 05:14
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.

2 participants