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

Resolve renewal failures for subscription orders with the UPE #6965

Merged
merged 12 commits into from
Aug 15, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/fix-upe-subscriptions-schisms
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fix

Fixes subscription renewals with the UPE enabled.
16 changes: 8 additions & 8 deletions includes/class-payment-information.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@ class Payment_Information {
/**
* The order object.
*
* @var \WC_Order/NULL
* @var ?\WC_Order
*/
private $order;

/**
* The payment token used for this payment.
*
* @var \WC_Payment_Token/NULL
* @var ?\WC_Payment_Token
*/
private $token;

Expand All @@ -52,14 +52,14 @@ class Payment_Information {
/**
* Indicates whether the payment is merchant-initiated (true) or customer-initiated (false).
*
* @var Payment_Initiated_By
* @var ?Payment_Initiated_By
*/
private $payment_initiated_by;

/**
* Indicates whether the payment will be only authorized (true) or captured immediately (false).
*
* @var Payment_Capture_Type
* @var ?Payment_Capture_Type
*/
private $manual_capture;

Expand Down Expand Up @@ -175,9 +175,9 @@ public function get_payment_method(): string {
/**
* Returns the order object.
*
* @return \WC_Order The order object.
* @return ?\WC_Order The order object.
*/
public function get_order(): \WC_Order {
public function get_order(): ?\WC_Order {
return $this->order;
}

Expand All @@ -188,9 +188,9 @@ public function get_order(): \WC_Order {
* since the return type is nullable, as per
* https://www.php.net/manual/en/functions.returning-values.php#functions.returning-values.type-declaration
*
* @return \WC_Payment_Token/NULL The payment token.
* @return ?\WC_Payment_Token The payment token.
*/
public function get_payment_token(): \WC_Payment_Token {
public function get_payment_token(): ?\WC_Payment_Token {
return $this->token;
}

Expand Down
42 changes: 33 additions & 9 deletions includes/class-wc-payment-gateway-wcpay.php
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,7 @@ public function process_payment_for_order( $cart, $payment_information, $schedul
throw new Exception( WC_Payments_Utils::get_filtered_error_message( $e ) );
}

$payment_methods = $this->get_payment_methods_from_request();
$payment_methods = $this->get_payment_method_types( $payment_information );
// The sanitize_user call here is deliberate: it seems the most appropriate sanitization function
// for a string that will only contain latin alphanumeric characters and underscores.
// phpcs:ignore WordPress.Security.NonceVerification.Missing
Expand Down Expand Up @@ -1231,7 +1231,7 @@ public function process_payment_for_order( $cart, $payment_information, $schedul
Payment_Method::CARD === $this->get_selected_stripe_payment_type_id() &&
in_array( Payment_Method::LINK, $this->get_upe_enabled_payment_method_ids(), true )
) {
$request->set_payment_method_types( $this->get_payment_methods_from_request() );
$request->set_payment_method_types( $this->get_payment_method_types( $payment_information ) );
$request->set_mandate_data( $this->get_mandate_data() );
}

Expand Down Expand Up @@ -1375,28 +1375,52 @@ public function process_payment_for_order( $cart, $payment_information, $schedul
*/
public function get_payment_method_to_use_for_intent() {
if ( WC_Payments_Features::is_upe_deferred_intent_enabled() ) {
return $this->get_payment_methods_from_request()[0];
$request_payment_method = sanitize_text_field( wp_unslash( $_POST['payment_method'] ?? '' ) ); // phpcs:ignore WordPress.Security.NonceVerification
return $this->get_payment_methods_from_gateway_id( $request_payment_method )[0];
}
}

/**
* Get the payment methods used in the request.
* Get payment method types to attach to intention request.
*
* @param Payment_Information $payment_information Payment information object for transaction.
* @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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing--added in 178d076.

$request_payment_method = sanitize_text_field( wp_unslash( $_POST['payment_method'] ?? '' ) ); // phpcs:ignore WordPress.Security.NonceVerification
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming proposal: $requested_payment_method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense--added in 178d076.

$token = $payment_information->get_payment_token();

if ( ! empty( $request_payment_method ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we prioritize posted PM over token? Can we add a comment explaining the order in which are trying to pick PMs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use $order instanceof WC_Order - a better practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call--updated in 178d076.

$payment_methods = $this->get_payment_methods_from_gateway_id( $token->get_gateway_id(), $order_id );
} else {
$payment_methods = WC_Payments::get_gateway()->get_payment_method_ids_enabled_at_checkout( null, true );
}

return $payment_methods;
}

if ( ! empty( $upe_payment_method ) && 'woocommerce_payments' !== $upe_payment_method ) {
$payment_methods = [ str_replace( 'woocommerce_payments_', '', $upe_payment_method ) ];
/**
* Get the payment methods used in the request.
*
* @param string $gateway_id ID of processing payment gateway.
* @param int $order_id ID of related order, if applicable.
* @return array List of payment methods.
*/
public function get_payment_methods_from_gateway_id( $gateway_id, $order_id = null ) {
if ( 'woocommerce_payments' !== $gateway_id ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should check here is the id start with woocommerce_payments_ as well. WDYT?

Copy link
Contributor

@kalessil kalessil Aug 11, 2023

Choose a reason for hiding this comment

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

Also, I lack a comment here explaining which gateways have this sort of IDs. Can you add it, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

$payment_methods = [ str_replace( 'woocommerce_payments_', '', $gateway_id ) ];
} elseif ( WC_Payments_Features::is_upe_split_enabled() || WC_Payments_Features::is_upe_deferred_intent_enabled() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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() )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate the clauses in 178d076.

$payment_methods = [ 'card' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a constant for card PM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout--amended in 178d076.

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 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment explaining when we get into this branch please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment explaining the logic a bit better in 178d076 and 08e0daa. We will get here if the $gateway_id is woocommerce_payments and neither split nor deferred intent UPE is enabled--so this is either the legacy UPE or legacy card gateway.

}

return $payment_methods;
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/payment-methods/test-class-upe-payment-gateway.php
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ public function set_up() {
// so that get_payment_method_from_request() does not throw error.
$_POST = [
'wcpay-payment-method' => 'pm_mock',
'payment_method' => UPE_Payment_Gateway::GATEWAY_ID,
];
}

Expand Down Expand Up @@ -834,7 +835,6 @@ public function test_create_setup_intent_no_customer() {

public function test_process_payment_returns_correct_redirect_url() {
$order = WC_Helper_Order::create_order();
$order_id = $order->get_id();
$_POST['wc_payment_intent_id'] = 'pi_mock';

$payment_intent = WC_Helper_Intention::create_intention( [ 'status' => Payment_Intent_Status::PROCESSING ] );
Expand All @@ -859,7 +859,6 @@ public function test_process_payment_returns_correct_redirect_url() {

public function test_process_payment_passes_save_payment_method_to_store() {
$order = WC_Helper_Order::create_order();
$order_id = $order->get_id();
$gateway_id = UPE_Payment_Gateway::GATEWAY_ID;
$save_payment_param = "wc-$gateway_id-new-payment-method";
$_POST[ $save_payment_param ] = 'yes';
Expand Down Expand Up @@ -919,11 +918,12 @@ public function test_process_payment_returns_correct_redirect_when_using_saved_p

$this->set_cart_contains_subscription_items( false );

$result = $this->mock_upe_gateway->process_payment( $order->get_id() );

$this->mock_upe_gateway
->expects( $this->never() )
->method( 'manage_customer_details_for_order' );

$result = $this->mock_upe_gateway->process_payment( $order->get_id() );

$this->assertEquals( 'success', $result['result'] );
$this->assertMatchesRegularExpression( '/key=mock_order_key/', $result['redirect'] );
}
Expand Down
Loading