Implement PE with deferred intent for the card payment method in the classic checkout #2753

a-danae · 2023-11-12T17:50:14Z

Fixes #2747

This PR doesn't point to develop as we'll require further work before these changes get shipped.

Changes proposed in this Pull Request:

Collect the payment details before creating the payment intent (aka, use deferred intents). Previously, a payment intent was created when the checkout page was loaded.
When processing the payment, the payment intent gets created already with the payment details and confirmed.
Processing UPE payments after the redirection to the "Order confirmation" page isn't needed anymore.
When the payment processing fails, the failed response gets added to the debug log

This PR only covers:

Processing payments from the classic checkout page. The block checkout, Pay for order page, and adding a new payment method from my account aren't covered.
Automatic captures. Authorizing the transaction and capturing later will be handled separately.
Using new cards regular. Other payment methods, 3DS cards, saving a card, or reusing an already saved card will be handled separately.

Testing instructions

Enable UPE
As a shopper, process payments under the classic checkout page for all tests

Happy path

Check out develop
Pay for an order using a new regular card, not saving it. Flow: Shopper > Checkout with normal credit card
Check out this branch and pay for another order
Open both WC orders in the database
Confirm these meta exist and match between both orders:
- _stripe_charge_captured
- _stripe_currency
- _stripe_fee
- _stripe_intent_id
- _stripe_net
- _stripe_upe_payment_type
Confirm that the meta _stripe_upe_redirect_processed, isn't set in the order processed using this branch
Open the payment intents in Stripe for both orders
Confirm that the information for the payment intents created for each order is consistent with each other:
- Customer data. You could try checking out as:
  - A guest -> A new Stripe customer should be created
  - A customer with no associated Stripe user -> A new Stripe customer should be created. It'll be reused
  - A customer with an associated Stripe user -> No new Stripe customer should be created. The associated one should be reused
- Payment details
  - For the statement descriptor, it must reflect the short or long version depending on the options in the plugin dashboard
- Payment method data
- Metadata:
  - customer_email
  - customer_name
  - order_id
  - order_key
  - payment_type
  - site_url

Confirm the intent creation flow is deferred

Add a product to the cart and go to the checkout page. Don't continue
In the Stripe dashboard, confirm no payment intent was created
Proceed with the checkout
Confirm that the payment intent was created and confirmed

Card failures

Confirm the following flow behaves as expected Shopper > Checkout failures (with various cards)

Note: With the card failures, a new payment intent will be created for each retry. In the future, the same intent will be reused as possible. This will be fixed by #2761.

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

Added testing instructions to the Release Testing Instructions wiki page (or does not apply)

…oller

…ing an intention

includes/payment-methods/class-wc-stripe-upe-payment-gateway.php

mattallan

Niiiice changes @a-danae !! I'm excited to see the work being started in this PR.

I've only just performed a quick code review so far but I'll have play with the front-end changes and then look over the changes with more attention to detail. 🙌

client/classic/upe/payment-processing.js

mattallan · 2023-11-21T05:07:12Z

client/classic/upe/payment-processing.js

+	const paymentMethodType = domElement.dataset.paymentMethodType;
+	let upeElement;
+
+	// Non-split PE. To be removed.


To be removed.

Just making sure I understand this and other similar in-line comments. This section can be removed when we stop supporting the legacy PE?

This section can be removed when we stop supporting the legacy PE?

Exactly. More specifically, this can be removed when we implement using split PE.

We could fully switch to using deferred intents, but we'll still need this until we split the payment elements in the frontend in separate WooCommerce gateways.

mattallan · 2023-11-21T06:37:22Z

includes/payment-methods/class-wc-stripe-upe-payment-gateway.php

+	 * @return array An array containing the payment information for processing a payment intent.
+	 */
+	private function prepare_payment_information_from_request( WC_Order $order ) {
+		// TODO: throw exception if any required information is missing.


Just checking, are the TODO's added in this PR going to be addressed in this PR or later?

This one specifically was already solved. Removed in 8098d58

But, yup, it's probably confusing to have them around and we already have tasks for working on them. I'm removing the ones that aren't meant to be solved in this PR, which is all but one.

Thanks @a-danae !

It's probably confusing to have them around and we already have tasks for working on them

I personally think they're fine to have in this base PR to help indicate where we expect code from another issue to go. I just wanted to avoid shipping the feature with a bunch of TODO's that we added early on and then never got around to addressing 😄

Happy to go with whatever you think is best!

Makes complete sense 😄

I've removed the TODO comments in cf90c3c. We can decide where's the best place to handle the behaviors mentioned in those comments when working on the issues.

includes/payment-methods/class-wc-stripe-upe-payment-gateway.php

mattallan · 2023-11-21T07:36:44Z

includes/class-wc-stripe-intent-controller.php

+			$order->update_meta_data( '_stripe_upe_payment_type', $selected_payment_type );
+		}
+
+		$order->update_status( 'pending', __( 'Awaiting payment.', 'woocommerce-gateway-stripe' ) );


I haven't tested anything yet but I'm curious why we need to update the status of the order to pending when dealing with deferred intents. What is the order status before this?

I'm wondering if we should have a $order->needs_payment() somewhere so that we're only attempting to create and confirm an intent for orders that have pending/failed status.

Maybe inside process_payment_with_deferred_intent()?

I haven't tested anything yet but I'm curious why we need to update the status of the order to pending when dealing with deferred intents. What is the order status before this?

You're right, no need to update the order status. The order is already "pending" at this point. Fixed in fef7018

I'm wondering if we should have a $order->needs_payment() somewhere so that we're only attempting to create and confirm an intent for orders that have pending/failed status.

Yeah, I agree.

I see there's a more complex logic around this in verify_intent_after_checkout(), but I think it's specific for the legacy PE implementation because it runs between the Checkout and Thank you pages.

Would it be okay with you to have an issue for addressing this, instead of doing it in this PR? I'm thinking that webhooks and cache may also come into play, but would like to dive into it. Also to not delay merging this base PR.

Would it be okay with you to have an issue for addressing this, instead of doing it in this PR? I'm thinking that webhooks and cache may also come into play, but would like to dive into it. Also to not delay merging this base PR.

That sounds good to me!

Yay! GH issue created over here #2785 . Of course feel free to update it!

And thanks as always for pointing these out 😄

Co-authored-by: Matt Allan <matttallan@gmail.com>

… intent Co-authored-by: Matt Allan <matttallan@gmail.com>

mattallan

Amazing work on this one @a-danae !!

As per the testing instructions you shared on the PR, I purchased two orders using a new card on both the develop and add/card-classic-with-deferred-intent and ran through the following checks I've posted below.

The only two issues I found were when I tried to purchase a regular WC product as a guest customer and the other issue was I noticed shipping details were missing from the payment data when looking at the payment from the Stripe dashboard

I'm not sure if you wanted to fix these in this PR or open separate issues for them but I'm happy to approve this PR and leave that decision up to you 😄

I also left a small comment regarding the empty initializeAppearance function

Confirmed the _stripe_upe_redirect_processed meta is not set on the order purchased on this branch
Confirmed all of the stripe processing meta is set on the order (i.e. _stripe_fee & _stripe_intent_id) and is consistent across both orders
Confirmed the payment intent in the Stripe dashabord has the correct customer, payment & meta data and is consistent across both orders created on different branched.
- ⚠️ The customer, payment and metadata all looked correct, however one thing I noticed is the payment on this branch doesn't have a shipping section when looking at Stripe. Looking in the events and logs section, it looks like on develop we send a POST /payment_intents/pi_ request with shipping. Not sure if this is needed but I'm happy to move this to a separate issue if you like.

Tested the above cases as:

Guest customer
- 🚨 I ran into an error trying to purchase a product as a guest customer on this branch. There weren't any PHP errors or gateway/request logs but I noticed this error in the console.
- Existing WC customer without Stripe customer
- Existing WC customer with Stripe csutomer
Tested with a failed card (0431) along with other erroring cases and confirmed new intents were created each time on this branch (confirming the bug that will be fixed in [Deferred intent] When possible, reuse a payment intent associated with the order #2761)

Specific deferred intent checks:

Confirm no intent is created on checkout page load on this branch
Confirm intent is created and confirmed after processing the checkout

client/classic/upe/payment-processing.js

…needed

…on object

…d in the selected country

mattallan · 2023-11-23T23:26:45Z

Thanks for the latest changes @a-danae !

I've confirmed 61d9e34 fixes the issue with shipping details not being sent to Stripe. Here's a screenshot testing the latest:

💃

a-danae · 2023-11-24T00:29:52Z

Thanks for going ahead and re-reviewing this PR! I completely missed the missing shipping information, great catch 🙂 I was planning to leave a comment here after solving the guest customer issue but I'll continue with that in the morning.

a-danae · 2023-11-24T11:36:16Z

Guest customer
🚨 I ran into an error trying to purchase a product as a guest customer on this branch. There weren't any PHP errors or gateway/request logs but I noticed this error in the console.

This should be fixed by 863cfc7. Something similar was happening in another place, and should be fixed by 1c2f308

Also, I introduced this validation that was present in the legacy PE flow 646299c

Back to you. Thanks for such a thorough review, Matt! 🙌

mattallan

Thanks @a-danae for fixing those issues!

I've confirmed the issue with not being able to purchase as a guest customer have been resolved.
I've given the latest changes a code review
Smoke tested the overall changes once again

I'm happy to approve this to be merged 🎉💃
Thanks for all the work you did to lay the foundation for deferred intents!

a-danae added 13 commits November 10, 2023 12:30

Separate adding hooks from the constructor for WC_Stripe_Intent_Contr…

e597d13

…oller

Make wc-stripe-upe-element a class instead of an id

beca67a

Render the Payment element in the classic checkout page without creat…

9104244

…ing an intention

Process payments with deferred intent in the classic checkout

5612e99

Fix expected dom element in unit tests

7fc37e0

Update process_payment_with_deferred_intent

75a876b

Add base uni test for PE with deferred intent processing

93339f5

Move some logic out of create_and_confirm_payment_intent

60c3529

Add validation for the payment information array

df620b9

Update unit tests for processing payments with a deferred intent

f30d1e8

Merge branch 'develop' into add/card-classic-with-deferred-intent

61b31e6

Improve the error messaging on processing failures

fc72a63

Remove unneeded test

bd1ff2d

a-danae changed the base branch from add/split-pe-with-deferred-intent to add/deferred-intent November 17, 2023 10:56

a-danae marked this pull request as ready for review November 20, 2023 08:46

a-danae requested a review from mattallan November 20, 2023 08:46

a-danae commented Nov 20, 2023

View reviewed changes

includes/payment-methods/class-wc-stripe-upe-payment-gateway.php Show resolved Hide resolved

mattallan reviewed Nov 21, 2023

View reviewed changes

a-danae and others added 6 commits November 21, 2023 09:37

Fix typo in doc block

1903089

Co-authored-by: Matt Allan <matttallan@gmail.com>

Remove resolved TODO comment

8098d58

Use the order currency instead of the store currency when creating an…

3c4fb1e

… intent Co-authored-by: Matt Allan <matttallan@gmail.com>

Remove inline TODO comments that already have issues to address them

cf90c3c

Remove unnecesary order status assignment

fef7018

Remove TODO that will be addressed in a separate issue

7a4d634

mattallan approved these changes Nov 23, 2023

View reviewed changes

client/classic/upe/payment-processing.js Show resolved Hide resolved

a-danae mentioned this pull request Nov 23, 2023

[Deferred intent] Prevent duplicated / mistaken payment processing of orders #2785

Open

a-danae added 3 commits November 23, 2023 11:47

Set the selected payment method type as the order's payment method title

36f4f53

Include shipping information for the payment intent when shipping is …

61d9e34

…needed

Pass the appearance and fonts parameters to the Elements initializati…

6697749

…on object

Validate whether the provided payment method type is valid and allowe…

646299c

…d in the selected country

a-danae added 3 commits November 24, 2023 11:17

Check whether the dom elements exist before changing their attributes

863cfc7

Fix console error when checking out as a guest

1c2f308

Adjust unit tests

59c3428

a-danae requested a review from mattallan November 24, 2023 11:36

mattallan approved these changes Nov 27, 2023

View reviewed changes

a-danae merged commit b672036 into add/deferred-intent Nov 27, 2023

a-danae deleted the add/card-classic-with-deferred-intent branch November 27, 2023 08:41

a-danae mentioned this pull request Nov 28, 2023

[Deferred intent] Card - Process payments in the classic checkout #2747

Closed

james-allan mentioned this pull request Dec 6, 2023

[Deferred intent] Card - Handle capture-later transactions #2764

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PE with deferred intent for the card payment method in the classic checkout #2753

Implement PE with deferred intent for the card payment method in the classic checkout #2753

a-danae commented Nov 12, 2023 •

edited

Loading

mattallan left a comment

mattallan Nov 21, 2023

a-danae Nov 21, 2023

mattallan Nov 21, 2023

a-danae Nov 21, 2023

mattallan Nov 22, 2023

a-danae Nov 22, 2023

mattallan Nov 21, 2023

a-danae Nov 22, 2023

mattallan Nov 22, 2023 •

edited

Loading

a-danae Nov 23, 2023

mattallan left a comment

mattallan commented Nov 23, 2023

a-danae commented Nov 24, 2023

a-danae commented Nov 24, 2023

mattallan left a comment

Implement PE with deferred intent for the card payment method in the classic checkout #2753

Implement PE with deferred intent for the card payment method in the classic checkout #2753

Conversation

a-danae commented Nov 12, 2023 • edited Loading

Changes proposed in this Pull Request:

Testing instructions

mattallan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattallan Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattallan left a comment

Choose a reason for hiding this comment

mattallan commented Nov 23, 2023

a-danae commented Nov 24, 2023

a-danae commented Nov 24, 2023

mattallan left a comment

Choose a reason for hiding this comment

a-danae commented Nov 12, 2023 •

edited

Loading

mattallan Nov 22, 2023 •

edited

Loading