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

Use deferred intents on the Pay for Order page #2801

Merged

Conversation

james-allan
Copy link
Contributor

@james-allan james-allan commented Dec 15, 2023

Fixes #2766

Changes proposed in this Pull Request:

This PR utilises the changes introduced in add/my-account-add-card-with-deferred-intent to add the deferred intent payment elements to the Pay for Order page.

These payment elements are the same ones used on the classic checkout.

Testing instructions

  1. Go to WooCommerce → Orders → Add new
  2. Add items, a customer and save. Leave the order as pending payment.
  3. Click the Customer payment page → link.
  4. On your Stripe dashboard go to the payments table and note that a payment intent has been created and is in-complete.
Screenshot 2023-12-15 at 10 06 00 am
  1. Checkout this branch (issue/2766-deferred-intent-pay-for-order-m2) and re-build
  2. Repeat from step 3.
    • Optional, add more items or change the order so the total is different so the two intents are distinguishable.
  3. This time, when you view the Stripe Dashboard, there won't be a new payment intent.
  4. Go back to the Pay for Order page and complete the checkout.
  5. Note that order is paid and the payment was taken in Stripe.
Screenshot 2023-12-15 at 10 22 37 am Screenshot 2023-12-15 at 10 22 54 am

Other tests

SCA/3DS

Alternative payment methods

Because these payment elements use the same ones implemented in the original classic checkout PR, it's still a unified payment element and all alternative methods appear like this:

Screenshot 2023-12-15 at 1 10 49 pm

Auth and capture

  1. Go to WooCommerce → Settings → Payments → Stripe → Settings
  2. Enable Auth and capture.
  3. Repeat the steps above to pay for an order via the Pay for Order page.
  4. View the admin edit order screen and note the order is on-hold and has an order note about auth and capture.
  5. Capture the payment by changing the order status to processing or completed.
  6. Verify the payment was taken authorised and captured in your Stripe dashboard.
Screenshot 2023-12-15 at 1 14 07 pm
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Added changelog entry in both changelog.txt and readme.txt (or does not apply)
  • Tested on mobile (or does not apply)

Post merge

mattallan and others added 18 commits November 28, 2023 15:56
@james-allan james-allan added pr: needs review status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. labels Dec 15, 2023
@james-allan james-allan requested review from a team and a-danae and removed request for a team December 15, 2023 03:21
Base automatically changed from add/my-account-add-card-with-deferred-intent to add/deferred-intent December 20, 2023 03:13
@james-allan james-allan removed the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Dec 20, 2023
Copy link
Contributor

@mattallan mattallan left a comment

Choose a reason for hiding this comment

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

Nice fix @james-allan !!

I've tested the following flows/cases on this PR and didn't find any issues:

  • Confirm intent isn't created on pay for order page load
  • Pay for an order with a success card
  • Pay for an order with a fail card (confirmed error was present)
  • Pay for an order with a 3DS card (4000000000003220)
  • Pay for an order with a 3DS card and fail the auth check
  • Pay for an order with Auth and Capture turn on/off

I couldn't test with a non-credit card like SEPA (pending order goes on-hold instead of processing, but I'm assuming this will addressed by #2772 ?

The other thing I noticed while testing this PR is when you use a failed card like 4000000000009995, you see the correct error message on the checkout, but if you then refresh the page and resubmit the form, you see the following:
image

I couldn't reproduce this on develop so I'm not sure if this is something we should fix now or fix it later as I don't think it's related to this PR specifically.
My guess is we need to do something like clear the form on submit or something so that when we display an error, the same form can't be resubmitted by the customer.

@james-allan
Copy link
Contributor Author

I couldn't test with a non-credit card like SEPA (pending order goes on-hold instead of processing, but I'm assuming this will addressed by #2772 ?

I think that's intended. SEPA is a direct debit kind of payment method so can take up to 6 days to confirm.

Having said that if I view the payment in the Stripe dashboard it is complete and confirmed which is probably a test mode thing. I'm going to test on a JN site to rule out a webhook issue.

The other thing I noticed while testing this PR is when you use a failed card like 4000000000009995, you see the correct error message on the checkout, but if you then refresh the page and resubmit the form, you see the following:

I cannot replicate this. Were you resubmitting the form after the original failure with a different card or the same failing card?

@james-allan
Copy link
Contributor Author

I'm going to test on a JN site to rule out a webhook issue.

So I've been able to confirm on a JN site running this branch, when I pay for an order using SEPA it goes pending > on-hold > processing.

Screenshot 2024-01-02 at 2 24 20 pm

@mattallan
Copy link
Contributor

I'm going to test on a JN site to rule out a webhook issue.

Aha! Yeah you're right, I think it might be setup/webhook related. With #2803 merged, I tested another UPE method like giropay and can confirm the Pay for Order page works correctly.

I cannot replicate this. Were you resubmitting the form after the original failure with a different card or the same failing card?

Yeah, after landing on the checkout page with the error message, I'm refreshing the page and pressing "Continue" on the resubmit option. Here's a recording:

Screen.Capture.on.2024-01-02.at.14-21-45.mp4

@james-allan
Copy link
Contributor Author

Yeah, after landing on the checkout page with the error message, I'm refreshing the page and pressing "Continue" on the resubmit option. Here's a recording

Ah thanks I can replicate this error now. When I try replicate on develop the page doesn't attempt to resubmit the form at all, the page just refreshes. The checkout page works the same so I'm going to look into how I can replicate that kind of behaviour here too.

@mattallan mattallan self-assigned this Jan 3, 2024
@james-allan
Copy link
Contributor Author

So I was comparing our code to WooPayments and I noticed it uses the same approach of submitting order pay form which I believe is what causes the difference between this branch and develop.

The difference is that on WooPayments it displays a more generic error message after you hit refresh and continue:

Screenshot 2024-01-03 at 10 59 38 am

As I've thought about it isn't technically incorrect that the error message gets displayed, you are reattempting to submit the form with the same data. The second time around the error message is different given you're attempting to reuse the last payment method. However, it's not ideal and we'd prefer the page would just refresh like it does on develop.

I have a feeling though that I'll need to do another redirect to fix that.

@james-allan
Copy link
Contributor Author

However, it's not ideal and we'd prefer the page would just refresh like it does on develop.

I have a feeling though that I'll need to do another redirect to fix that.

So I've been able to prevent the resubmit popup after a failure but to achieve that I needed to put in a redirect at the end of the process_payment_with_deferred_intent() function which redirects the user back to a clean pay for order page.

eg

@@ -742,6 +742,11 @@ class WC_Stripe_UPE_Payment_Gateway extends WC_Gateway_Stripe {
                                sprintf( __( 'Payment failed: %s', 'woocommerce-gateway-stripe' ), $e->getLocalizedMessage() )
                        );

+                       if ( isset( $_POST['wc-stripe-is-order-pay'] ) ) {
+                               wp_safe_redirect( $order->get_checkout_payment_url() );
+                               exit;
+                       }
+
                        return [ 'result' => 'failure' ];
                }
        }

This works, however I'm not sure if it's the right approach because that cuts the request short and prevents the rest of the WC_Form_Handler::pay_action() from running. eg the woocommerce_after_pay_action hook won't fire because I've cut the request short and done a redirect.

So I'm a little stuck now on how we want to proceed here. The reason this doesn't happen on develop is because it uses the handleUPEOrderPay() JS function which does all the processing on the pay for order page itself via JS and doesn't submit the form at all. It creates the intent, confirms the payment and either redirects or shows the error message all within the JS without needing to submit the page.

Given WooPayments works the same way as this branch with a different error message, what are your thoughts on leaving this as it is. I have a feeling that this would affect all similar payment methods and WC could resolve it by changing the WC_Form_Handler::pay_action() to include a redirect and prevent the risk of a form resubmission on refresh if they wanted to.

@mattallan
Copy link
Contributor

Given WooPayments works the same way as this branch with a different error message, what are your thoughts on leaving this as it is

I think this is a reasonable way forward. Thanks for looking into this @james-allan

Approving this one for merge!

@james-allan james-allan merged commit 0e1e60d into add/deferred-intent Jan 5, 2024
@james-allan james-allan deleted the issue/2766-deferred-intent-pay-for-order-m2 branch January 5, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants