-
Notifications
You must be signed in to change notification settings - Fork 210
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
Add APM support for Subscriptions and tokens with split UPE with deferred intent #2883
Add APM support for Subscriptions and tokens with split UPE with deferred intent #2883
Conversation
…a tokens on the block checkout
…rt contains a subs cription or saved tokens are enabled
I'm placing this one on-hold for a bit. I think there's an issue in the way I'm setting the subscription's payment method ID to |
props.showSaveOption | ||
) { | ||
options.setupFutureUsage = 'off_session'; | ||
} |
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.
*/ | ||
if ( $is_stripe_payment_method && isset( $data['issavedtoken'] ) && $data['issavedtoken'] ) { | ||
$context->set_payment_data( array_merge( $data, [ 'wc-stripe-is-deferred-intent' => true ] ) ); | ||
} |
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.
Using a saved payment token on the block checkout doesn't go via PaymentProcessor
function and so this flag that we use to determine if we should process the request via the deferred intent flow isn't set.
This change makes sure we set this flag early in the request to process the block checkout.
|
||
return $is_stripe_link_enabled || $is_sepa_debit_payment; | ||
return 'card' === $selected_payment_type && in_array( 'link', $this->get_upe_gateway()->get_upe_enabled_payment_method_ids(), true ); |
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.
Because Bancontact and iDEAL resolve to SEPA debit payment tokens, Stripe also requires that mandate data be set when attempting to use Bancontact and save it.
'payment_method' => $payment_information['payment_method'], | ||
'payment_method_types' => [ $payment_information['selected_payment_type'] ], | ||
'customer' => $payment_information['customer'], | ||
'confirm' => 'true', | ||
'return_url' => $payment_information['return_url'], |
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.
Now that Bancontact and iDEAL can be used for set up intents, we need to pass a return URL here.
@@ -712,7 +714,7 @@ private function process_payment_with_deferred_intent( int $order_id ) { | |||
|
|||
// Handle saving the payment method in the store. | |||
// It's already attached to the Stripe customer at this point. | |||
if ( $payment_information['save_payment_method_to_store'] ) { | |||
if ( $payment_information['save_payment_method_to_store'] && $upe_payment_method && $upe_payment_method->get_id() === $upe_payment_method->get_retrievable_type() ) { |
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.
This change can essentially be described as: only save the token if the payment method being used resolves to that same payment method. eg card = card 👍 bancontact = sepa ❌.
When using a Bancontact to purchase a subscription we don't save the bancontact stripe payment method id (ie pm_
) because it's not reusable. After the customer returns to the store after authorising the payment, stripe will create a sepa_debit
token and we save it at that point. See process_upe_redirect_payment()
and process_order_for_confirmed_intent()
.
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.
@james-allan I did some testing today and noticed a couple of things:
- ❌
pm_
token stored on the subscription is different than what gets saved in the wc_payment_tokens table - ❌ Processing a renewal payment with an APM (ideal) results in the following fatal:
[12-Feb-2024 03:23:17 UTC] PHP Fatal error: Uncaught Error: Call to undefined method WC_Stripe_UPE_Payment_Method_Sepa::get_intent_from_order() in /Users/matt/local/woo/wp-content/plugins/woocommerce-gateway-stripe/includes/compat/trait-wc-stripe-subscriptions.php:818
Stack trace:
#0 /Users/matt/local/woo/wp-content/plugins/woocommerce-gateway-stripe/includes/compat/trait-wc-stripe-subscriptions.php(244): WC_Stripe_UPE_Payment_Method_Sepa->has_authentication_already_failed(Object(WC_Order))
#1 /Users/matt/local/woo/wp-content/plugins/woocommerce-gateway-stripe/includes/compat/trait-wc-stripe-subscriptions.php(189): WC_Stripe_UPE_Payment_Method_Sepa->process_subscription_payment('225.00', Object(WC_Order), true, false)
#2 /Users/matt/local/woo/wp-includes/class-wp-hook.php(324): WC_Stripe_UPE_Payment_Method_Sepa->scheduled_subscription_payment('225.00', Object(WC_Order))
#3 /Users/matt/local/woo/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters('', Array)
#4 /Users/matt/local/woo/wp-includes/plugin.php(517): WP_Hook->do_action(Array)
#5 /Users/ in /Users/matt/local/woo/wp-content/plugins/woocommerce-gateway-stripe/includes/compat/trait-wc-stripe-subscriptions.php on line 818
- ❌ On the Edit Subscription page, when you go to edit the payment method, there's two SEPA payment options. Inspecting the elements shows one is actually
'stripe'
but for some reason the title is being listed as SEPA? I couldn't figure this one out.
I've been digging into the second issue a bit and it brings up a bigger question (and potential issues). If the SEPA payment method is going to be using the subscriptions trait, should it extend a gateway class, or should we refactor the subscriptions trait so that it doesn't assume the trait will be used on a gateway class by using things like WC_Stripe::get_instance()->get_main_stripe_gateway()->{gateway_method}
🤔
@mattallan I've pushed 5f76377 in line with the approach we discussed in a Slack huddle. I'm going to put it through some tests to make sure things are working as expected. |
A couple of things I've noticed so far:
|
I noticed my APM tokens aren't getting deleted from Stripe so I've opened a separate issue for this: EDIT: opened another issue |
Another issue:
|
@james-allan I did a bit more testing today and found some issues. Let me know if you'd rather move these out to separate issues so that we can divide and conquer 😄 APM testing (iDeal):
Standard Credit Card testing:
|
Good catch. I've had a look into this one, and the issue stems from this code in the if ( WC_Stripe_Feature_Flags::is_upe_checkout_enabled() ) {
$payment_method_types = $this->get_upe_enabled_at_checkout_payment_method_ids();
} elseif ( isset( $prepared_source->source_object->type ) ) {
$payment_method_types = [ $prepared_source->source_object->type ];
} Given this function has With that in mind, I'm thinking of removing that first if statement entirely. Thoughts? |
Yeah I agree. Looking at where |
I've fixed that with b99580b.
I've fixed that in b13c162.
I still need to look into this one. |
Ok. I've fixed that issue in d1f7541 too. 😓 |
Should we also include Sofort? It's reusable and it continues to be displayed when it was enabled before. |
Here's the full set of tests I'm going to rerun through now that this PR seems a little more stable. I may add to them as I go. Subscription sign up
Renewals
Changing payment methods
Every other payment method will need to be part of: #2905
Tokens
|
It's my understanding the Sofort has been discontinued by Stripe. There are PRs I've seen removing it from the plugin and in Stripe's docs they say that they are in the process of deprecating it but they do leave the door open to existing merchants. So, if I've misunderstood and we do still support Sofort let me know. :) |
Just making a note that Danae has fixed this in #2902 💯 |
…cally (#2902) * Check whether the deleted token belongs to a reusable gateway Before, we were only checking it belonged to the main gateway. Since Split PE, APMs have their own gateway so we needed to change this check * Move the try/catch blocks up in the method * Add Bancontact, Ideal, and Sofort to the static list of reusable gateways
Yup, that's what I've understood: It's been discontinued by Stripe but still functioning for existing merchants. In the plugin, looks like we haven't removed it for merchants that still have it active (ref in UPE, ref in classic). |
Ok, yeah that makes sense. I've updated the Sofort UPE method to now support subscriptions in d6690c9. I've also updated my list of tests above to also test Sofort. FWIW and incase this is helpful to anyone else, because my Stipe settings didn't show Sofort as an available payment method, I just forced it on via the option. $settings = get_option( 'woocommerce_stripe_settings' );
$settings['upe_checkout_experience_accepted_payments'][] = 'sofort';
update_option( 'woocommerce_stripe_settings', $settings ); |
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.
@james-allan I did a bunch more testing on this branch on the latest changes today and I would say this is ready to be merged!!! 👏
I found one issue with not being able to use a saved credit card when paying for a failed subscription renewal. I was able to replicate this on the base branch add/deferred-intent
so I'm going to create a separate issue to fix this.
Question:
- Should iDeal and Bancontact support multiple subscriptions or leave it out for now?
iDeal
- Purchase simple
- Use new card
- Use saved card
- Purchase multiple subscriptions
- iDeal not supported ❓
- Process renewal
- New card
- Saved card
- Confirm successful renewals with iDeal even with the SEPA payment method disabled
- Renew early
- Modal
- Checkout
- Change payment method
- iDeal/Bancontact not listed. Will be fixed separately in [Split PE] Investigate and enable customers to change subscriptions to Bancontact and iDEAL #2900
- Purchase trial
- Use a saved card
- New card
- Process switch
- No switch cost
- With prorated amount
- Refund renewal order
Credit Card
- Purchase simple
- Use new card
- Checkout shortcode
- Checkout block
- Use saved card
- Checkout shortcode
- Checkout block
- Use new card
- Purchase multiple subscriptions
- Use new card
- Checkout shortcode
- Checkout block
- Use saved card
- Checkout block
- Checkout shortcode
- Use new card
- Purchase mixed checkout
- Use new card
- Checkout shortcode
- Checkout block
- Use saved card
- Checkout block
- Checkout shortcode
- Use new card
- Process renewals
- single purchase
- multiple subscriptions
- mixed checkout
- Renew early
- Modal
- Checkout
- Change payment method
- Purchase trial
- Use new card
- Checkout shortcode
- Checkout block
- Use saved card
- Checkout block
- Checkout shortcode
- Use new card
- Process switch
- Refund renewal order
- Pay for a failed renewal
- New card
- Checkout shortcode
- Checkout block
- Saved card
- Checkout shortcode
- Checkout block ❌
- Error found in stripe logs: No such payment_intent: 'seti_XXXXXX'
- Can reproduce on add/deferred-intent so I'll create a separate issue for this
- New card
SEPA Direct Debit
- Purchase subscriptiom
- Use new card
- AT611904300234573201
- AT861904300235473202 (fails)
- Use saved card
- Use new card
- Renew
- Renew early
- Change payment method
- change to different SEPA card
- change to a credit card
- Purchase trial
- Refund renewal order
Bancontact
- Purchase simple
- Use new card
- Use saved card
- Renew
- Renew early
- Purchase trial
- Refund renewal order
@mattallan I'm still going through your latest response and all the tests and questions you've raised. I was just in the middle of my tests too and I found an issue with using a new 3DS card on the block checkout (I haven't tested classic checkout). The purchase goes through but the payment method on the subscriptions says
Can you just check that you can replicate that too and I'll file an issue for it. |
Yep I can reproduce it, but only on the Checkout block page and only when using a new card.
Payment method on My Account > View subscription is N/A, but the payment method in the DB is still set to |
Thanks for confirming. I can also replicate on |
Note I've opened a new issue to address the problem I found to do with paying for a failed renewal using a saved card on the block checkout: #2904 |
I've patched that in e1e3926. :) |
Fixes #2872
Changes proposed in this Pull Request:
This PR adds/fixes the following:
Testing instructions
Subscriptions
add/deferred-intent
only the card payment element will be shown.add/deferred-intent
Important
Keep in mind that no matter which payment method you use (SEPA, Bancontact or iDEAL) they all resolve to SEPA payment tokens as described in Stripe docs here, under "Recurring payments".
Tokens
After you've purchased a subscription using any of the APMs mentioned above, you should now have a SEPA token. These steps will walk through the process of using a SEPA token and saving a new one.
Deleting tokens
wp_woocommerce_payment_tokens
table.changelog.txt
andreadme.txt
(or does not apply)Post merge