Add ACSS support for WC Subscriptions #4051
Conversation
| 'payment_schedule' => 'combined', | ||
| 'interval_description' => __( 'Payments as per agreement', 'woocommerce-gateway-stripe' ), |
There was a problem hiding this comment.
I figured it's best to set the payment schedule to combined and the interval description to something that can fit both one-time payments and subscriptions, because the same payment method (and consequently the mandate) can be used for both recurring and non-recurring payments, depending on how the payment method is saved in WooCommerce.
Reference: https://docs.stripe.com/payments/acss-debit#payment-schedule
| @@ -436,7 +437,7 @@ public function update_payment_intent_ajax() { | |||
| * @throws Exception If the update intent call returns with an error. | |||
| * @return array|null An array with result of the update, or nothing | |||
| */ | |||
| public function update_payment_intent( $payment_intent_id = '', $order_id = null, $save_payment_method = false, $selected_upe_payment_type = '' ) { | |||
| public function update_intent( $payment_intent_id = '', $order_id = null, $save_payment_method = false, $selected_upe_payment_type = '' ) { | |||
There was a problem hiding this comment.
This method now handles setup intents as well. Looking at the Stripe logs, there was an error where it was incorrectly calling payment_intents/seti_... instead of setup_intents/seti....
Given the similarity of a potential new method for handling only setup intents, I decided to unify both needs into the same function.
I'd appreciate the reviewer double-checking this approach, but I believe it shouldn't affect existing flows, given how minimal the change is.
| if ( isset( $intent->mandate ) ) { | ||
| $order->update_meta_data( '_stripe_mandate_id', $intent->mandate ); | ||
| } |
There was a problem hiding this comment.
For payment intents, the mandate ID is only available from the charge object, however for setup intents, the mandate ID can be accessed directly from the intent object.
| // TODO: Refactor and add mandate ID support for other payment methods, if necessary. | ||
| // The mandate ID is not available for the intent object, so we need to fetch the charge. | ||
| // Mandate ID is necessary for renewal payments for certain payment methods and Indian cards. |
There was a problem hiding this comment.
The mandate ID here is necessary for renewal orders. Without it, renewal orders will fail.
I think it's best to keep this as a TODO just to indicate that the approach should be refactored if adding new payment methods, and also to make sure we test other payment methods because I figured this is necessary for at least card and ACSS.
@asumaran I couldn't test BACS subscriptions, but this doesn't seem necessary for ACH as far as I have tested. Can you try processing a renewal order for BACS and make sure this is not needed for BACS? There's a similar check in line 1631.
ricardo
requested review from
a team and
cesarcosta99
and removed request for
a team
|
@cesarcosta99 I fixed the change payment method flows in 5ccf8f8. So the PR is finally ready to be reviewed. IMO that method shouldn't be used to update the order, but only the intent. It would be great to have it refactored, but that method became slightly convoluted a long time before these changes, so I've just adapted it to prevent a bug when changing the method for a subscription. |
There was a problem hiding this comment.
Awesome work! Code LGTM with just one minor request and it also tested well overall. I spotted one issue we may want to fix before merging this.
⚠️ Purchase subscription product- ✅ Classic and Blocks checkouts
⚠️ Minor issue found: the Next Payment column don't display the selected payment method, neither in the list or in the subscription view. In the screenshot below, the 200 and 198 IDs are free trial purchases, while 194 and 191 are simple subscription purchases. Note: After manually renewing 194 and 191, the payment method is now displaying, but initially it wasn't.
- ✅ Purchase free trial subscription
- Classic and Blocks checkouts
- ✅ Renew subscription
- Classic and Blocks checkouts
- ✅ Automatically renew a subscription
- ✅ Change payment method
- ✅ Change payment method to saved token
- ✅ Change default payment method
| * | ||
| * @since 5.6.0 | ||
| * @version x.x.x | ||
| * | ||
| * @param {string} $payment_intent_id The id of the payment intent to update. |
There was a problem hiding this comment.
Can we update this param to $intent_id (and its description) to avoid any confusion now that we're expanding support to setup intents as well?
Fixes #3831
Changes proposed in this Pull Request:
Testing instructions
K1A 0B1Reproduce the following critical flows using ACSS for payments:
woocommerce_scheduled_subscription_paymentaction related to the subscription.Changelog entry
Changelog Entry Comment
Comment
Post merge