Resolve renewal failures for subscription orders with the UPE #6965
Conversation
…s for intent when appropriate
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.41 MB ℹ️ View Unchanged
|
|
Once this is reviewed/tested/merged, how would we feel about sharing a "beta" build with 6430073-zen? They are greatly affected by #4492:
|
| * @return array List of payment methods. | ||
| */ | ||
| private function get_payment_methods_from_request() { | ||
| $upe_payment_method = sanitize_text_field( wp_unslash( $_POST['payment_method'] ?? '' ) ); // phpcs:ignore WordPress.Security.NonceVerification | ||
| public function get_payment_method_types( $payment_information ) { |
There was a problem hiding this comment.
I guess for the supported PHP versions we can harden this method a bit: public function get_payment_method_types( Payment_Information $payment_information ): array
There was a problem hiding this comment.
Sure thing--added in 178d076.
| private function get_payment_methods_from_request() { | ||
| $upe_payment_method = sanitize_text_field( wp_unslash( $_POST['payment_method'] ?? '' ) ); // phpcs:ignore WordPress.Security.NonceVerification | ||
| public function get_payment_method_types( $payment_information ) { | ||
| $request_payment_method = sanitize_text_field( wp_unslash( $_POST['payment_method'] ?? '' ) ); // phpcs:ignore WordPress.Security.NonceVerification |
There was a problem hiding this comment.
Naming proposal: $requested_payment_method
There was a problem hiding this comment.
Makes sense--added in 178d076.
| $request_payment_method = sanitize_text_field( wp_unslash( $_POST['payment_method'] ?? '' ) ); // phpcs:ignore WordPress.Security.NonceVerification | ||
| $token = $payment_information->get_payment_token(); | ||
|
|
||
| if ( ! empty( $request_payment_method ) ) { |
There was a problem hiding this comment.
Why do we prioritize posted PM over token? Can we add a comment explaining the order in which are trying to pick PMs?
There was a problem hiding this comment.
I'm basically prioritising checkout transactions over subscription renewals, since I believe the former will be more common: explained over here.
| $payment_methods = $this->get_payment_methods_from_gateway_id( $request_payment_method ); | ||
| } elseif ( ! is_null( $token ) ) { | ||
| $order = $payment_information->get_order(); | ||
| $order_id = is_a( $order, 'WC_Order' ) ? $order->get_id() : null; |
There was a problem hiding this comment.
Let's use $order instanceof WC_Order - a better practice.
There was a problem hiding this comment.
Good call--updated in 178d076.
| * @return array List of payment methods. | ||
| */ | ||
| public function get_payment_methods_from_gateway_id( $gateway_id, $order_id = null ) { | ||
| if ( 'woocommerce_payments' !== $gateway_id ) { |
There was a problem hiding this comment.
I believe we should check here is the id start with woocommerce_payments_ as well. WDYT?
There was a problem hiding this comment.
Also, I lack a comment here explaining which gateways have this sort of IDs. Can you add it, please?
There was a problem hiding this comment.
At this point, we know that the ID will start with woocommerce_payments; if the ID is anything other than woocommerce_payments, we presume that it will begin with woocommerce_payments_ as this should be the only remaining available option.
I simplified this (with a few explanatory comments) in 178d076 by simply checking whether the ID begins with woocommerce_payments_ first instead. However, I don't really feel a need to also confirm that the ID begins with woocommerce_payments, as I don't believe that it's possible for neither of those cases to be true.
| */ | ||
| public function get_payment_methods_from_gateway_id( $gateway_id, $order_id = null ) { | ||
| if ( 'woocommerce_payments' !== $gateway_id ) { | ||
| $payment_methods = [ str_replace( 'woocommerce_payments_', '', $gateway_id ) ]; | ||
| } elseif ( WC_Payments_Features::is_upe_split_enabled() || WC_Payments_Features::is_upe_deferred_intent_enabled() ) { |
There was a problem hiding this comment.
That's difficult to understand can we introduce separate ifs for WC_Payments_Features::is_upe_split_enabled() and WC_Payments_Features::is_upe_deferred_intent_enabled() )?
There was a problem hiding this comment.
Separate the clauses in 178d076.
| */ | ||
| public function get_payment_methods_from_gateway_id( $gateway_id, $order_id = null ) { | ||
| if ( 'woocommerce_payments' !== $gateway_id ) { | ||
| $payment_methods = [ str_replace( 'woocommerce_payments_', '', $gateway_id ) ]; | ||
| } elseif ( WC_Payments_Features::is_upe_split_enabled() || WC_Payments_Features::is_upe_deferred_intent_enabled() ) { | ||
| $payment_methods = [ 'card' ]; |
There was a problem hiding this comment.
There is a constant for card PM.
There was a problem hiding this comment.
Good shout--amended in 178d076.
| } elseif ( WC_Payments_Features::is_upe_split_enabled() || WC_Payments_Features::is_upe_deferred_intent_enabled() ) { | ||
| $payment_methods = [ 'card' ]; | ||
| if ( WC_Payments_Features::is_upe_deferred_intent_enabled() && | ||
| in_array( Payment_Method::LINK, $this->get_upe_enabled_payment_method_ids(), true ) ) { | ||
| $payment_methods[] = Payment_Method::LINK; | ||
| } | ||
| } else { | ||
| $payment_methods = WC_Payments::get_gateway()->get_payment_method_ids_enabled_at_checkout( null, true ); | ||
| $payment_methods = WC_Payments::get_gateway()->get_payment_method_ids_enabled_at_checkout( $order_id, true ); |
There was a problem hiding this comment.
Can we add a comment explaining when we get into this branch please?
FangedParakeet
requested review from
a team,
timur27 and
gpressutto5
and removed request for
a team and
timur27
|
@kalessil, just seen your comment over here. I tested this locally and I think this does enable us to process renewals for subscriptions using SEPA as a payment method...but this wasn't particularly intended by me when I authored this PR. 😅 I was actually under the impression that we would continue proceeding to keep SEPA subscriptions disabled--consequently we have closed #5517 and #5531. Can you confirm whether this is still the case or whether we intend to give SEPA subscriptions another chance? In the latter case, I would have to reassess whether SEPA subscriptions integration should be included in our upcoming deferred intent UPE feature release. |
Fixes #4492
Fixes #6291
Changes proposed in this Pull Request
Fixing dysfunctional subscriptions with split and legacy UPE
This PR simplifies and clarifies the logic used to select an array of payment method type IDs to attach to a payment intent. We first check whether
$_POSTparameters are present--in which case, the request is from a standard checkout form. If the$_POSTparameters are missing, this may be a subscription renewal--in which case, we use the attached payment token to ascertain the payment gateway and ensure we use the currency on the order to filter payment methods. If the scenario fits neither case, we return the payment methods enabled by the current gateway.Hopefully this should fix...
$_POST[ 'payment_method' ]) is not present.Testing instructions
Testing subscription renewals
This is the main feature that needs to be tested in this PR and can generally be performed in two ways.
First ensure your store contains at least one subscription product, add it to cart, and complete a checkout for this product.
Trigger subscription renewal from order page
Trigger subscription renewal from scheduled actions
Ideally, we should test subscription renewals with all UPEs disabled, legacy UPE enabled, split UPE enabled, and deferred intent UPE enabled. You can toggle these features using the dev tools.
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge