Split UPE: Correct token behavior

[mdmoore]

Fixes #4530

Changes proposed in this Pull Request

This PR ensures that tokens use the correct gateway ID and are displayed under the correct gateway throughout WCPay. Some checks have also been added to ensure that tokens aren't duplicated per each gateway.

Testing instructions

• Enable SEPA in the UPE settings
• From My Account > Payment Methods > Add Payment Method, add a credit card and SEPA method
• Return to My Account > Payment Methods > Add Payment Method and refresh the page a few times to make sure tokens aren't being duplicated
• Check that SEPA tokens are appearing under SEPA Direct Debit and card tokens are appearing under Credit card / Debit card

---

• Run npm run changelog to add a changelog file, choose patch to 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.
• Covered with tests (or have a good reason not to test in description ☝️)
• Tested on mobile (or does not apply)

Post merge

• Link to testing instructions from release testing doc following these instructions : Add link here / 'QA Testing Not Applicable'
• Add or update critical flows and testing instructions for critical flows, if applicable.
• Add what's changed (description, screenshot, demo videos etc.) to the release announcement post, if applicable.

[harriswong]

👀

[harriswong]

I get a processing error when I tried creating a SEPA direct debit

The request returned a 400 code. The request URL is https://api.stripe.com/v1/setup_intents/xyzxyzxyzxyzxyzxyz/confirm

The response body has this error:

 "error": {
    "code": "setup_intent_unexpected_state",
    "doc_url": "https://stripe.com/docs/error-codes/setup-intent-unexpected-state",
    "message": "You cannot update this SetupIntent because it has already succeeded.",
    "setup_intent": {
     ...
    }
}

[tommyshellberg]

It sounds like this unit test was written to make sure that deleting a payment token from another gateway doesn't happen. Setting this to once() seems like it kinda breaks the initial intention of the test.
I wonder if we can write perhaps a more specific test for split UPE gateways?
For instance, if we delete a SEPA gateway we test that only a SEPA token type is deleted(and only once)? 🤔

[tommyshellberg]

The request returned a 400 code.
The response body has this error:

 "error": {
    "code": "setup_intent_unexpected_state",
    "doc_url": "https://stripe.com/docs/error-codes/setup-intent-unexpected-state",
    "message": "You cannot update this SetupIntent because it has already succeeded.",
    "setup_intent": {
     ...
    }
}

I'm getting the same error after creating a second SEPA saved token. The same setup_intent ID is being re-used over and over, even if you refresh the page. My guess is that the setup intent is cached, perhaps as a result of this PR?

Edit: Yeah, it looks like it's grabbing a setup_intent ID from the session:

Perhaps we are not removing the UPE setup intent from the session after creating a payment method.

Hope that helps!

[mdmoore]

@harriswong @tommyshellberg Nice catch! It looks like we were trying to remove the intent from the session using the woocommerce_after_account_payment_methods hook, but that code wasn't being reached. Thanks for finding that PR!

[tommyshellberg]

This is working for me now 🥳 🎉 thanks for addressing the comments I had! I just have some really nitpicky stuff leftover but I'm approving it.
Tests are passing as well as my manual tests.

Even though it's not a huge PR in terms of lines written it deals with complex, abstract and critical code, I don't envy you for having to write this stuff 😬
Great job on this!

[FangedParakeet]

I've left a few comments here, but nothing serious, for better or worse.

I was unable to save payment methods at checkout sadly. When using the card gateway or the SEPA gateway, the attempt to confirm the intent returned the following error.

2022-10-07T16:12:43+00:00 ERROR `setup_future_usage` cannot be used with one or more of the values you specified in `payment_method_types`. Please remove `setup_future_usage` or remove these types from `payment_method_types`: ["eps", "giropay", "p24"]. (invalid_request_error)

(I have all the available payment methods enabled, by the way.)

I was a little surprised by this, but presume this was due to this line returning all enabled payment methods instead of just the current.

Looking at that line it does make a little more sense.

$payment_methods = WC_Payments::get_gateway()->get_payment_method_ids_enabled_at_checkout( null, true );

When the UPE is enabled, WC_Payments::get_gateway() will return the UPE_Payment_Gateway instance, not the legacy card gateway instance of WC_Payment_Gateway_WCPay. Perhaps you might want to modify this to $this->get_payment_method_ids_enabled_at_checkout( null, true ); however, I've left another comment about why this might not have a desired effect when saved SEPA tokens are used at checkout.

Actually, thinking further on this, I think using $this would not allow you to save a SEPA payment method at checkout either, because all of the UPE payment methods enabled at checkout would be returned. We only want to fill that parameter with a single, selected payment method.

I was able to add both cards and SEPA payment methods from the My Account page however. In fact, doing so actually triggered a forgotten SEPA payment method to appear as a saved payment method, so something is working here. The payment tokens look normal in the database and the saved payment methods appear where they should do at checkout, so that's great news!

Halfway there, so let me know if I can help out with anything in the meantime. Cheers!

[mdmoore]

@FangedParakeet I appreciate your attention to detail! I've addressed the feedback from your previous review if you don't mind giving it another look when you have a chance.

[FangedParakeet]

All the code here looks pretty good to me and I don't have too many comments on modifications to make on your additions...That being said I did encounter some misfortunes while testing these changes.

When I was able to add payment methods, they ended up in the correct gateway, which was cool to see and I was able to checkout successfully using both saved card and SEPA payment methods. However, I faced quite a few issues while attempting to add new payment methods, mostly due to clashing caching.

Adding payment method from My Account page

When adding a payment method from the My Account page, after entering my payment information, submitting the form, and returning to the list of payment methods at /my-account/payment-methods, I found that my newly added payment method did not appear in the list.

Checking my Stripe dashboard, I could find that the payment method had been added successfully, but by checking the debug logs, I could see that no request was being made to GET /wcpay/payment_methods to refresh the local database cache of payment methods. For some reason, either this function was not firing or it was not doing its job as expected. I was able to trigger the cache to clear by either reopening the Add Payment Method form (/my-account/add-payment-method) or visiting the checkout, where suddenly all my new saved payment methods would appear at last.

Also, I was able to often inconsistently trigger the below ugly looking PHP warning.

Warning: Illegal string offset 'data' in /var/www/html/wp-content/plugins/woocommerce-payments/includes/class-wc-payments-customer-service.php on line 223 

This didn't happen consistently, but I could trigger this pretty regularly by adding, deleting, or listing payment methods from My Account dashboard. This warning refers to this line from the Customer Service, where we parse the response from the GET /wcpay/payment_methods request containing the user's Stripe stored payment methods.

However, this actually originates from the Token Service: the issue is that in this loop, we make requests to Stripe for payment methods of all enabled payment method types (e.g. including Bancontact, Sofort, etc. when enabled). Stripe stored payment methods can only be of the type card or sepa_debit (be they either saved SEPA payment methods or other payment methods--Bancontact, Ideal, etc.--that are converted to SEPA when saved), so we should only query these two payment method types.

On the Stripe gateway, we created a new retrievable_type property to gather these underlying storable payment method types. However, that might be overkill here, since for now the only reusable payment methods are card and sepa_debit, so we don't need to worry about deciphering which stored SEPA methods were actually converted from other payment methods. We should still restrict the payment method types that we request saved payment methods to a maximum of just card and sepa_debit.

Adding payment methods from checkout

The UPE saved payment method flow works by using the setup_future_usage flag to ensure that a payment method is stored on Stripe and clearing our local payment methods cache, so that we will, in the future, request a fresh list of customer payment methods from Stripe, when it comes time to display a customer's saved payment tokens. However, our redirection flow is not currently working as it should when SEPA is chosen as a payment gateway at checkout.

For some reason, large sections of the UPE redirection code are not executing for SEPA. I can see in the debug logs that we reach this point, where we collect and examine the confirmed payment intent; however, at this point--without any errors--the trail goes cold.

A new payment method may be stored with Stripe, but we don't clear our local cache, so a WC token is not created. The status of the order does not update and there are no notes left on the order, other than that of a payment being authorised. Worst of all, it seems like this line is either ignored or ineffective: the SEPA payment intent cached to the session does not clear, so that if we begin a new checkout, when we try to place our order, we discover that the UPE is still using the already confirmed intent from the previous transaction and we are rightfully shown the following error!

The parameter application_fee_amount cannot be updated on a PaymentIntent after a capture has already been made.

I wasn't able to fully investigate these cascading catastrophes, but it's worth noting that SEPA is a "delayed notification" payment method, meaning that upon confirming the intent, the payment does not succeed immediately. Since the intent status might not be exactly what we would expect, this could cause us to mishandle these transactions. Let me know if you have any ideas here, because I think I would need to dig into this deeper to firgure what's going wrong (perhaps in a separate issue)...

Lowercase SEPA labelling

This is a smaller issue, but is it just me or do saved SEPA tokens always appear styled like this in WooCommerce?

"Sepa iban"?

SEPA IBAN should definitely be capitalised and I can't find anywhere where we outwardly refer to this payment method in lowercase, so I'm not fully sure where this formatting comes from. It's a small issue, but let me know if you see the same display, since we probably should figure out how to fix this.

---

Sorry, I've left you another novel to go through here, but let me know if I'm testing anything incorrectly or if you're able to casually explain away any of my erroneous encounters! If you are able to share the pain of any of these errors with me, let me know if you'd like to move any of these to separate issues. I think the checkout errors and probably also the labelling issues can definitely be moved to their own issues, if you're able to sort out everything mentioned on the Account pages.

Anyways, looking forward to hearing your responses to my exhausting feedback (sorry again!), let me know if I've managed to muddle up anything else along the way. Cheers!

[mdmoore]

@FangedParakeet - Thanks once again for the thorough review.

Adding payment method from My Account page

When adding a payment method from the My Account page, after entering my payment information, submitting the form, and returning to the list of payment methods at /my-account/payment-methods, I found that my newly added payment method did not appear in the list.

It turns out this wasn't due to caching after all. When we list tokens, we also decide whether to add tokens to the user or not. To prevent tokens from being duplicated across gateways, we check that the payment method type matches the gateway_id on which the code is currently running, in the case of the Add Payment Method and Checkout pages. But, on My Account > Payment Methods, there is no gateway ID, so the token was never added to the user. I handled this previously by using this empty( $gateway_id ) check. But when that line was brought into question, I couldn't remember or find it's purpose even though I felt like it was doing something important. Added back, newly added UPE methods immediately appear on the Payment Methods page upon redirect.

Also, I was able to often inconsistently trigger the below ugly looking PHP warning.
Warning: Illegal string offset 'data' in /var/www/html/wp-content/plugins/woocommerce-payments/includes/class-wc-payments-customer-service.php on line 223
the issue is that in this loop, we make requests to Stripe for payment methods of all enabled payment method types (e.g. including Bancontact, Sofort, etc. when enabled)

This is a great catch! Unfortunately, even after restricting the payment method types to card and sepa_debit, the warning still pops up at random. Sometimes it's very easy to reproduce the warning and other times it just won't happen. I was able to catch it one time while debugging the $payment_methods array. This array should contain 2 arrays: one for all card and one for all sepa_debit payment methods stored in Stripe. Instead, one array might be returned as a string.

array(2) { [0] => string(1) " " [1] => array(3) { [0] => array(9)... }

This happens here, before we flatten the array. It feels like an error in the response.

The parameter application_fee_amount cannot be updated on a PaymentIntent after a capture has already been made.

I encountered this before as well but thought it to be solved. I am able to reproduce it again now though.

Lowercase SEPA labeling

This isn't something I've encountered yet. I've always seen the all caps, SEPA IBAN. I'm not sure what might cause this unless it could be a browser extension perhaps?

@FangedParakeet - How do you feel about moving to merge this now that tokens are working on the My Account > Payment Methods page, then creating new issues for the intermittent warning and checkout errors?

[FangedParakeet]

But, on My Account > Payment Methods, there is no gateway ID, so the token was never added to the user. I handled this previously by using #4670 (comment).

Whoops, sorry for making you remove that line! That makes perfect sense, even though it is a little irritating that the $gateway_id parameter is always missing on the Accounts page.

Unfortunately, even after restricting the payment method types to card and sepa_debit, the warning still pops up at random.

😢

This happens here, before we flatten the array. It feels like an error in the response.

Yep, agreed. I think the error is in the response handling and should be handled in the WPay Server API client, before it reaches the token service.

How do you feel about moving to merge this now that tokens are working on the My Account > Payment Methods page, then creating new issues for the intermittent warning and checkout errors?

This sounds like a good plan. Right now my local environment is all twisted up for some reason and not returning any WCPay gateways at checkout on this branch, but I'm going to try to fix that and retest tomorrow morning. Here are a few things that I think we can probably handle in separate issues, so let me know if you agree.

• Saved payments functionality from shortcode checkout
• Saved payments functionality from blocks checkout
• API error handling for payment method requests

[mdmoore]

Here are a few things that I think we can probably handle in separate issues, so let me know if you agree.

Saved payments functionality from shortcode checkout
Saved payments functionality from blocks checkout
API error handling for payment method requests

Yep, these are all good issues to pull from the discovery in this PR. I'll get these added.

[FangedParakeet]

I've left a few comments, but this is generally working pretty well for me from the My Accounts page now!

On one occasion, I was met with this ugly error message.

Wtf?

But checking the error log, I think my local WCPay Server was just throwing a tantrum, because the setup intent was created successfully, a new payment method was added by Stripe, but for some reason the response came back empty, causing this nonsense. I think we need better error handling for these such cases, but we've discussed that already and this can definitely be move to a new issue.

Also, I had to downgrade npm to ~v6 to get build the packages via npm install. Dunno why that happened or whether that could have caused anything above. Anyways, let's just forget about that for now.

TL;DR: I'm going to give this PR my blessings!...But first, let me describe the things that worked, the things that didn't, and the things we can move to new issues.

Things I tested

• Listing saved payment methods on My Accounts > Payment methods ✅
• Listing saved payment methods on Add payment methods page ✅
• Listing saved payment methods on shortcode checkout ✅
• Listing saved payment methods on blocks checkout ✅
• Adding saved payment methods (UPE & non-UPE) from My Accounts ✅
• Adding saved payment methods (UPE & non-UPE) from shortcode checkout ✅
• Adding saved payment methods (UPE and non-UPE) from blocks checkout ❌
• Completing checkout using non-UPE saved payment method from shortcode checkout ✅
• Completing checkout using UPE saved payment method from shortcode checkout ✅
• Completing checkout using non-UPE saved payment method from shortcode checkout ✅
• Completing checkout using UPE saved payment method from blocks checkout ✅
• Completing checkout using non-UPE saved payment method from blocks checkout ❌

Things that didn't work quite right

Blocks checkout

This is generally quite broken, but #4681 has just been merged into the feature branch and not yet updated here, so I'm optimistic that some of these issues will be resolved automatically by merging...though, after also reviewing that PR, I think a few of them might remain as well. 😅

Anyways, no matter for now. We'll solve this elsewhere.

SEPA Tokens

I left a comment about how to make duplicate SEPA tokens unintentionally and get them to appear at on the Accounts page when SEPA is disabled as a payment method...but I think you solved it right as I've been writing this review, so I'll count that resolved. 😁

SEPA Token labelling

I found that on the form pages (i.e. add payment method, shortcode checkout) the labelling was capitalised correctly...

SEPA IBAN: excellent

...But on the main payment methods list page, where all payment methods are shown together and on the blocks checkout page, the payment method title is in lowercase.

Sepa iban??? My eyes!

Sepa iban again??? Someone please make it stop!

I honestly have no idea why this is happening and hope it isn't a problem that needs to be solved in WC Core. Anyways, this can definitely be moved to a new issue.

WCPay Server API Errors

From time to time these random hideous PHP errors about missing data properties fill up the entire screen, usually solvable by simply refreshing the page. This is basically happening because for some reason the response from the WCPay Server is empty. I hope this happens more frequently locally, due to some Docker temperamentality, but either way, I think we could perhaps handle this better by swallowing the message or retrying such requests.

Things that can be solved in new issues

• Blocks checkout compatibility
• SEPA labelling
• API error handling

Conclusion

But in general, despite all of that, this PR is working really well in pretty much all use-cases! Thanks for running this marathon with me and responding to all of my cluster-bombed feedback along the way.

I think this is a huge step forward and definitely now ready to join the rest of the party!

[github-actions]

Size Change: 0 B

Total Size: 1.7 MB

ℹ️ View Unchanged

Filename	Size
release/woocommerce-payments/assets/css/admin.css	913 B
release/woocommerce-payments/assets/css/success.css	401 B
release/woocommerce-payments/dist/blocks-checkout.css	1.77 kB
release/woocommerce-payments/dist/blocks-checkout.js	34.4 kB
release/woocommerce-payments/dist/checkout.css	1.85 kB
release/woocommerce-payments/dist/checkout.js	27.1 kB
release/woocommerce-payments/dist/index.css	35.8 kB
release/woocommerce-payments/dist/index.js	640 kB
release/woocommerce-payments/dist/multi-currency-analytics.js	8.88 kB
release/woocommerce-payments/dist/multi-currency-switcher-block.js	79 kB
release/woocommerce-payments/dist/multi-currency.css	14.7 kB
release/woocommerce-payments/dist/multi-currency.js	117 kB
release/woocommerce-payments/dist/onboarding.css	1.12 kB
release/woocommerce-payments/dist/onboarding.js	38.1 kB
release/woocommerce-payments/dist/order.js	299 B
release/woocommerce-payments/dist/payment-gateways.css	1.2 kB
release/woocommerce-payments/dist/payment-gateways.js	91.1 kB
release/woocommerce-payments/dist/payment-request.js	23.1 kB
release/woocommerce-payments/dist/platform-checkout.css	3.73 kB
release/woocommerce-payments/dist/platform-checkout.js	76.5 kB
release/woocommerce-payments/dist/settings.css	47.3 kB
release/woocommerce-payments/dist/settings.js	157 kB
release/woocommerce-payments/dist/subscription-edit-page.js	3.66 kB
release/woocommerce-payments/dist/subscription-product-onboarding-modal.css	7.18 kB
release/woocommerce-payments/dist/subscription-product-onboarding-modal.js	16.2 kB
release/woocommerce-payments/dist/subscription-product-onboarding-toast.js	428 B
release/woocommerce-payments/dist/subscriptions-empty-state.css	298 B
release/woocommerce-payments/dist/subscriptions-empty-state.js	48.4 kB
release/woocommerce-payments/dist/tos.css	236 B
release/woocommerce-payments/dist/tos.js	31.5 kB
release/woocommerce-payments/dist/upe_checkout.css	1.85 kB
release/woocommerce-payments/dist/upe_checkout.js	38.5 kB
release/woocommerce-payments/dist/upe-blocks-checkout.css	1.77 kB
release/woocommerce-payments/dist/upe-blocks-checkout.js	45.3 kB
release/woocommerce-payments/includes/subscriptions/assets/css/plugin-page.css	633 B
release/woocommerce-payments/includes/subscriptions/assets/js/plugin-page.js	355 B
release/woocommerce-payments/vendor/automattic/jetpack-admin-ui/src/css/jetpack-icon.css	224 B
release/woocommerce-payments/vendor/automattic/jetpack-assets/build/i18n-loader.js	2.43 kB
release/woocommerce-payments/vendor/automattic/jetpack-assets/src/js/i18n-loader.js	1.01 kB
release/woocommerce-payments/vendor/automattic/jetpack-identity-crisis/babel.config.js	160 B
release/woocommerce-payments/vendor/automattic/jetpack-identity-crisis/build/index.css	2.27 kB
release/woocommerce-payments/vendor/automattic/jetpack-identity-crisis/build/index.js	13.3 kB
release/woocommerce-payments/vendor/automattic/jetpack-identity-crisis/build/index.rtl.css	2.27 kB
release/woocommerce-payments/vendor/automattic/jetpack-tracking/src/js/tracks-ajax.js	789 B
release/woocommerce-payments/vendor/automattic/jetpack-tracking/src/js/tracks-callables.js	925 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/about.css	1.2 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/admin-order-statuses.css	403 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/admin.css	3.43 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/checkout.css	299 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/modal.css	742 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/view-subscription.css	572 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/wcs-upgrade.css	411 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/admin-pointers.js	544 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/admin.js	8.97 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/jstz.js	6.8 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/jstz.min.js	3.83 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/meta-boxes-coupon.js	544 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/meta-boxes-subscription.js	2.35 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/moment.js	22.1 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/moment.min.js	11.6 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/payment-method-restrictions.js	1.29 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/wcs-meta-boxes-order.js	502 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/frontend/payment-methods.js	355 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/frontend/single-product.js	429 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/frontend/view-subscription.js	1.38 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/frontend/wcs-cart.js	387 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/modal.js	1.1 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/wcs-upgrade.js	1.27 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/build/index.css	392 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/build/index.js	3.06 kB

_{compressed-size-action}

[mdmoore]

@FangedParakeet Thanks a ton for sticking with me on this. Your comments and reviews have been invaluable. The SEPA token labeling problem is a strange one that I haven't encountered anywhere that the label is used. This is what I see on the My Account > Payment Methods page:

It'll be good to have some deeper investigation on this. I could see it being due to some difference in version among WooCommerce, WooCommerce Blocks, or WCPay.

After merging poc/upe-instances-multiplied, I notice a couple new issues when adding new payment methods.

1. Adding a new card is failing now that card has been removed from the UPE payment method config here
2. When adding a SEPA payment method, success notices are now duplicated.

With that, our list of new issues to be created will be:

1. Blocks checkout compatibility
2. SEPA labeling
3. API error handling
4. Saving card payment methods
5. Duplicate notices when saving SEPA methods

I'll add these ASAP 👍