-
Notifications
You must be signed in to change notification settings - Fork 206
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
Conversation
- avoid setting mandate options when subscription amount is 0 - add mandate data to renewal if not present in parent order
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.
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 🙂
…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
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.
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
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.
✅ 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.
$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' ]; |
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.
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 );
.
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.
Moved it to a separate function as suggested.
// 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. |
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.
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 🤔
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.
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]
From the last round of review, there were 3 issues discovered,
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:
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 cc: @a-danae @reykjalin |
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. |
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.
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.
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.
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
)
)
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. |
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.
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!
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-
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 is0
.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.Changes proposed in this Pull Request:
confirm
andsetup_future_usage
attributes correctly when an existingmandate
is passed with the request. (fixes 4, 5) 947acf1 b8d987dRelated resources for reading
Testing instructions
Prerequisite
Testing 0 amount subscription (Test with both UPE enabled and disabled)
4000003560000123
to complete the purchase._stripe_mandate_id
meta set for the order.my account
page as the shopper and renew the subscription manually._stripe_mandate_id
meta set for the current order.Testing saved card with UPE enabled
4000003560000123
indian card saved in the payment methods and select the subscription product._stripe_mandate_id
meta set for this order.Testing saved card with UPE disabled
4000003560000123
indian card saved in the payment methods and select the subscription product._stripe_mandate_id
meta set for this order.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 card4000003560000123
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 themy 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.
changelog.txt
andreadme.txt
(or does not apply)Post merge