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

Create mandate for subscriptions which do not have a mandate created previously. #2633

Merged
merged 16 commits into from
Dec 13, 2023

Conversation

Mayisha
Copy link
Contributor

@Mayisha Mayisha commented Jun 6, 2023

Fixes #2607

The changes in this PR fix a few issues related to mandate in a subscription and renewal. The main target of this PR is to fix the error that happens while processing a 0 amount subscription which needs a mandate. A mandate is required only for the cards issued in India.

The main issues handled in this PR are-

  1. Unable to process 0/month subscriptions. We add the mandate data to the request and on Stripe end it creates a mandate if required by attaching a payment method to it. The amount needs to be a positive integer and so it fails when the amount is 0.
  2. If we just bypass the mandate options for 0 amount subscriptions and later update the subscription amount to a positive value to be charged, we need to create a mandate and save it for later.
  3. If we change the subscription amount or the payment method, the existing mandate won't work and we need to create a new mandate for the amount and/or the payment method.
  4. When UPE is enabled, if we buy a subscription with a saved card it does not create a mandate.
  5. When UPE is disabled, if we buy a subscription with a saved card it creates a mandate and use the same mandate in renewal, But the manual renewal fails.

Changes proposed in this Pull Request:

  • avoid setting mandate options when the subscription amount is 0. (fixes 1)
  • add mandate data to renewal if not present in any previous renewal orders. (fixes 2)
  • Save mandate id in the order meta, in case it was created in some later renewals, and reuse it on the later renewals. (fixes 2)
  • If the saved mandate is for a different amount and/or different payment method, create a mandate again and save it to the current order meta (fixes 3)
  • Pass confirm and setup_future_usage attributes correctly when an existing mandate is passed with the request. (fixes 4, 5) 947acf1 b8d987d

Related resources for reading

Testing instructions

Prerequisite

  • Change the store currency to INR
  • Make sure the webhooks are working

Testing 0 amount subscription (Test with both UPE enabled and disabled)

  • Create a subscription with 0 amount and a sign-up fee.
  • As a shopper go to the shop page and add the subscription to your cart.
  • Use card 4000003560000123 to complete the purchase.
  • Make sure that there is no error and the subscription is active.
  • Check logs in your Stripe dashboard also to ensure there is no error.
  • Find Id of the order, check your DB and make sure that there is no _stripe_mandate_id meta set for the order.
  • In admin dashboard, go to the subscription edit page and edit the subscription amount to a positive value (500/1000/5000 etc)
  • Go to the my account page as the shopper and renew the subscription manually.
  • Complete the renewal and make sure there is no error.
  • Check your DB and make sure that there is now a _stripe_mandate_id meta set for the current order.
  • Manually renew the order again and check your logs (in Stripe dashboard or in WooCommerce -> Status -> Logs). Make sure that the mandate saved in the previous order is sent with the request.

Testing saved card with UPE enabled

  • Disable UPE
  • Create a subscription product with a positive amount
  • As a shopper, go to the shop page as a logged in user, make sure you have the 4000003560000123 indian card saved in the payment methods and select the subscription product.
  • Complete the purchase
  • Make sure a mandate is created. Check your DB and make sure that there is a _stripe_mandate_id meta set for this order.
  • Manually renew the order again and check your logs (in Stripe dashboard or in WooCommerce -> Status -> Logs). Make sure that the mandate saved in the previous order is sent with the request.

Testing saved card with UPE disabled

  • Enable UPE
  • Create a subscription product with a positive amount
  • As a shopper, go to the shop page as a logged in user, make sure you have the 4000003560000123 indian card saved in the payment methods and select the subscription product.
  • Complete the purchase
  • Make sure a mandate is created. Check your DB and make sure that there is a _stripe_mandate_id meta set for this order.
  • Manually renew the order again and check your logs (in Stripe dashboard or in WooCommerce -> Status -> Logs). Make sure that the mandate saved in the previous order is sent with the request.

Testing with a different amount (Skip this section)

  • Create a subscription with a positive value (500/1000/2000 etc)
  • As a shopper go to the shop page and add the subscription to your cart.
  • Use card 4000003560000123 to complete the purchase.
  • Make sure that there is no error and the subscription is active.
  • Find Id of the order, check your DB and make sure that there is a _stripe_mandate_id meta set for the order.
  • Renew this order and check the request body in the logs (in Stripe dashboard or in WooCommerce -> Status -> Logs) to ensure that the mandate created in the previous order is sent with the request.
  • Find Id of the order, check your DB and make sure that there is no _stripe_mandate_id meta set for this renewal order.
  • In admin dashboard, go to the subscription edit page and edit the subscription amount to a different positive value (1500/3000/5000 etc)
  • Go to the my account page as the shopper and renew the subscription manually.
  • Find Id of the order, check your DB and make sure that there is a _stripe_mandate_id meta set for the order with a different mandate id than before.

Testing critical flows

Make sure all critical flows work as intended with both the Classic and UPE checkouts. Make sure to test both Shortcode and Block-based flows. Test with a normal card, 3DS card and Indian card 4000003560000123

Testing automatic renewal

Follow and test the steps mentioned in the original PR of implementing e-mandate.


  • 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

    - avoid setting mandate options when subscription amount is 0
    - add mandate data to renewal if not present in parent order
@Mayisha Mayisha marked this pull request as draft June 6, 2023 04:57
@Mayisha Mayisha requested a review from reykjalin June 6, 2023 04:58
Copy link
Contributor

@reykjalin reykjalin left a comment

Choose a reason for hiding this comment

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

I think the fix is fine (didn't test the code), but I don't think it's in the right place (see comment). I'm also unsure if it's complete 🤔

The tricky part here is that, with the current implementation (both before and after this PR), if the parent order is a $0 order -- or if there is no parent order -- we're always creating a new mandate because we either (a) never created a mandate and saved it to the parent order; or (b) don't have a parent order in the first place.

So along with the fix that allows for $0 orders we also need to include a fix that can look up a mandate ID from previous orders. The recurring payments for a subscription all need to use the same mandate; if you create a new one the customer has to go through the "sign-up" process for the subscription all over again instead of just confirming the renewal payment.

I'm not exactly sure what the best way to achieve this is. Maybe to always save the mandate on the first order in the subscription, even if the mandate was generated on a different order (e.g. $0 payment first, renewal has an actual payment)? Maybe we save the mandate ID on the order where a payment was first made and then every subsequent order as well so we only have to check the previous renewal?

Honestly, this is probably going to need some exploration and I'm happy to provide more advice or discuss more in-depth either here or in Slack 🙂

includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
…ndate`

- Without `confirm=true` and with `setup_future_usage` set, passing mandate does not work
- for one time order, add mandate to the current order
- for subscription, add mandate to the parent order
@Mayisha Mayisha marked this pull request as ready for review June 13, 2023 08:13
@Mayisha Mayisha requested review from reykjalin, a team and a-danae and removed request for a team June 13, 2023 08:20
@Mayisha Mayisha requested review from a team and removed request for a-danae July 13, 2023 08:50
@a-danae a-danae requested review from a-danae and removed request for a team July 13, 2023 14:10
Copy link
Contributor

@a-danae a-danae left a comment

Choose a reason for hiding this comment

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

Thank you very much for working on this, Mayisha!

I tested these scenarios that had errors before and worked fine:
✅ Purchase a 0-subscription with a sign-up fee. EUR. UPE
✅ Purchase a 0-subscription with a sign-up fee. EUR. Non-UPE

I also went through almost all the testing steps from the PR description:
✅ Testing 0 amount subscription. INR. Non-UPE
✅ Testing saved card with UPE enabled
✅ Testing saved card with UPE disabled
✅ Testing with a different amount

❓ Testing 0 amount subscription. INR. UPE
This flow worked fine with the suggested steps. But I had a problem using a new payment method instead of a saved one for the manual renewal. These are the steps:

  • In the checkout page, select “Use a new payment method”
  • Entered 4000 0035 6000 0123
  • Clicked on “Renew subscription”
  • An error message shows up saying
    “stripe.confirmSetup(): the `confirmParams.return_url` argument is required unless passing `redirect: 'if_required'`"
    There's an entry in the debug.log file saying
    PHP Notice: Undefined index: payment_method in /var/www/html/wp-content/plugins/woocommerce-gateway-stripe/includes/compat/trait-wc-stripe-subscriptions.php on line 573
  • It works fine when using the saved card. I couldn't replicate this with non-UPE.

Mind checking if you get the same behavior?

I have the following pending, but I wanted to leave the feedback I got in the meantime.

  • Testing critical flows
  • Testing automatic renewal
  • Checking out the code

Copy link
Contributor

@a-danae a-danae left a comment

Choose a reason for hiding this comment

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

✅ I've reviewed the code. Looking good! I left a few questions and comments about some behaviors we may want to double-check.

Update: I also ran the e2e tests against a JN site, and they all passed. I was about to test the other subscription-related flows, but it may be better to address Kristófer's comments first.

includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
Comment on lines 589 to 595
$request['payment_method_options']['card']['mandate_options']['amount_type'] = 'fixed';
$request['payment_method_options']['card']['mandate_options']['interval'] = $renewal_order->get_billing_period();
$request['payment_method_options']['card']['mandate_options']['interval_count'] = $renewal_order->get_billing_interval();
$request['payment_method_options']['card']['mandate_options']['amount'] = $renewal_amount;
$request['payment_method_options']['card']['mandate_options']['reference'] = $order->get_id();
$request['payment_method_options']['card']['mandate_options']['start_date'] = $renewal_order->get_time( 'start' );
$request['payment_method_options']['card']['mandate_options']['supported_types'] = [ 'india' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work when there are multiple subscriptions in the renewal order? I'm wondering if we need to add a check to see if amount_type should be set to maximum and the interval to sporadic, similar to what we do below.

Better yet, maybe a helper function would be better here that we could use in both places?

function create_mandate_options_for_order( $order ): array {
	$mandate_options = [];
	$subscriptions   = wcs_get_subscriptions_for_order( $order );

	$sub_amount = 0;
	foreach ( $subscriptions as $sub ) {
		$sub_amount += WC_Stripe_Helper::get_stripe_amount( $sub->get_total() );
	}

	// Note: we're making an assumption here that we should be creating the
	// mandate options even if the `$sub_amount` is 0, which we might not want
	// to? Not sure at the time of writing.

	// Get the first subscription associated with this order.
	$sub = reset( $subscriptions );

	if ( 1 === count( $subscriptions ) ) {
		$mandate_options['amount_type']    = 'fixed';
		$mandate_options['interval']       = $sub->get_billing_period();
		$mandate_options['interval_count'] = $sub->get_billing_interval();
	} else {
		// If there are multiple subscriptions the amount_type becomes 'maximum' so we can charge anything
		// less than the order total, and the interval is sporadic so we don't have to follow a set interval.
		$mandate_options['amount_type'] = 'maximum';
		$mandate_options['interval']    = 'sporadic';
	}

	$mandate_options['amount']          = $sub_amount;
	$mandate_options['reference']       = $order->get_id();
	$mandate_options['start_date']      = $sub->get_time( 'start' );
	$mandate_options['supported_types'] = [ 'india' ];

	return $mandate_options;
}

That function can then be used in both places like $request['payment_method_options']['card']['mandate_options'] = $this->create_mandate_options_for_order( $order );.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to a separate function as suggested.

Comment on lines 669 to 670
// Return from the most recent renewal order with a valid mandate. Mandate is created against a payment method
// and for a specific amount in Stripe so the payment method and amount should also match to reuse the mandate.
Copy link
Contributor

Choose a reason for hiding this comment

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

the payment method and amount should also match to reuse the mandate

Payment method, yes, amount, not necessarily. Please correct me if I'm wrong, but I think part of what you're trying to guard against here is when a customer signs up to more than 1 subscription with different renewal periods in the same order?

For example:

  • Sub A: renews monthly.
  • Sub B: renews weekly.

First week of every month will be more expensive because the renewal order will be paying for both subscriptions. If I understand this current section correctly a different mandate will be created for when only Sub B is renewed, and the initial mandate will be used for when Sub A and Sub B renew at the same time.

I think this would create a confusing experience for the customer because they'll get a sign-up message twice for what they may think of as the same subscription.

Stripe solves this by allowing the amount_type to be maximum and the interval to be sporadic, meaning for this order of Sub A + Sub B we can set the amount_type to maximum, the amount to Sub A + Sub B price, and the interval to sporadic (in this example we could probably use weekly, but that may not be the case for every mix of renewal intervals and more difficult to implement).

Ultimately that means the price may not match even though the mandate is valid for the order, so I think — based on the comment — that this section may need to be rethought a bit? Granted, I don't really have the full context of when this code is executed so I may be wrong 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scenario I considered here was editing and updating the subscription amount from wp-admin. As we discussed in slack, this scenario is not supported. So I removed the amount check and kept the payment method check only. [3476211]

@Mayisha Mayisha changed the title Fix processing a subscription of amount 0 Create mandate for subscriptions which do not have a mandate created previously. Jul 26, 2023
@Mayisha
Copy link
Contributor Author

Mayisha commented Jul 31, 2023

From the last round of review, there were 3 issues discovered,

  1. Adding tax and updating the subscription gives an error payment_intent_mandate_invalid [comment]
  2. When the store has different taxes for different states the same payment_intent_mandate_invalid happens [comment]
  3. I added a comparison between the current order amount and the previous order amount that has a saved mandate. This check was added to support a specific scenario. Suppose a renewal order A has a saved mandate M for amount 1200 and the merchant edits the subscription to change the price to 1500. Now when shopper tries to renew the order, the renewal order B fails with an error payment_intent_mandate_invalid because mandate M was created for amount 1200, not 1500. But this amount comparison will be conflicting for another scenario mentioned here (multi sub order)

I had a discussion with @reykjalin and figured that we can ignore the tax update scenario (point 1 above) and the subscription amount update scenario (which I was trying to cover with the amount comparison mentioned in point 3 above) because changing the subscription price isn’t supported. The merchant has to either:

  • Ask the customer to repurchase the subscription if the price is changed; or
  • Create a new subscription and have the customer come back to the store to process the payment.

As mandates are only created for Indian card and INR currency, and India does not have state based different tax rules we can ignore the scenario in point 2 also.

As changing the subscription price and continuing with the same subscription is not supported, the testing instruction I added in the description under the Testing with a different amount becomes invalid. I am mentioning that in the description.

cc: @a-danae @reykjalin

@broadeye
Copy link

broadeye commented Aug 3, 2023

Ability for customer to upgrade from a free variation to a paid one on the same subscription is a fundemental feature of WC Subscriptions. Being unable to do so would cripple real world merchant business models.

@Mayisha
Copy link
Contributor Author

Mayisha commented Aug 4, 2023

Ability for customer to upgrade from a free variation to a paid one on the same subscription is a fundemental feature of WC Subscriptions. Being unable to do so would cripple real world merchant business models.

The functionality for customer's to upgrade from a free to paid option is not changed here. It will still work the same as before.

Copy link
Contributor

@a-danae a-danae left a comment

Choose a reason for hiding this comment

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

Loving how clean it feels with the mandate properties being handled in a separate method 🙌

I only reviewed the code, I still need to test it. I left some comments but they're not blockers.

includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
includes/compat/trait-wc-stripe-subscriptions.php Outdated Show resolved Hide resolved
Copy link
Contributor

@a-danae a-danae left a comment

Choose a reason for hiding this comment

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

I only noticed one behavior we might want to double-check, but other than that, this looks good to me! Thanks @Mayisha !

✅ Testing saved card with UPE enabled
✅ Testing 0 amount subscription with UPE enabled
✅ Testing saved card with UPE disabled

❓Testing 0 amount subscription with UPE disabled
I get this error when following the steps:

7UE4Bz.mov
Error: stdClass Object
(
    [error] => stdClass Object
        (
            [message] => Mandate or subscription exists with given reference: 262
            [request_log_url] => https://dashboard.stripe.com/test/logs/req_vnyH6S5E94pPqB?t=1701725656
            [type] => invalid_request_error
        )

)

@Mayisha
Copy link
Contributor Author

Mayisha commented Dec 13, 2023

Thanks @a-danae for testing this again.

I could not reproduce the error that you are having for '0 amount subscription with UPE disabled'. @james-allan also had a look and could not reproduce it either. I have created a new issue for this bug #2800 and planning to handle it separately as it is not always reproducible.

Copy link
Contributor

@a-danae a-danae left a comment

Choose a reason for hiding this comment

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

Things look good to me 👍 Thanks for double-checking it and for creating an issue 🙂

Glad to approve this since the scenario is very specific and we couldn't replicate this behavior consistently. I'll dive further into it. Thanks again!

@Mayisha Mayisha merged commit 0b0712e into develop Dec 13, 2023
30 of 32 checks passed
@Mayisha Mayisha deleted the fix/2607-0-sub branch December 13, 2023 13:41
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.

$0/month Subscriptions unable to process on Stripe 7.3
4 participants