Merge develop into poc/upe instances multiplied

[timur27]

Description in progress

Fixes #5109

Changes proposed in this Pull Request

Merge the latest changes from develop and ensure, that poc/upe-instances-multiplied with all the changes on it are stable.

#4850 is the PR that affects our split UPE implementation. With this refactor, we now have two new classes, which represent the checkout - WC_Payments_Checkout and WC_Payments_UPE_Checkout. These classes are encapsulating the UI field rendering.

Following commits are introduced besides the regular merging and conflict resolving:

Minor fixes without any logic change

Minor | fixes | to avoid regression

Fixing | tests | suite

Resolving conflicts and ensuring tests pass after both token behavior was corrected and the logic for LINK tokens was updated. ⚠️ There is an additional logic introduced, basically a copy-paste from the SEPA tokes behavior. We need to make sure, that tokens for LINK are still working as expected. I think we can do it after #4720.

Initializing payment checkouts in class-wc-payments to reflect the latest changes from develop which introduce WC_Payments_Checkout and WC_Payments_UPE_Checkout classes.

15f08c7 to let the WC_Payments_UPE_Checkout store the list of payment gateways to reflect the split UPE work and be able to render the UI for each split payment method.

---

WC_Payments_Checkout and WC_Payments_UPE_Checkout are now responsible for rendering the UI for a gateway. Since the refactor was done before the UPE split, it takes a single method into the account even for WC_Payments_UPE_Checkout.

The current logic of rendering gateways on develop (after the refactor, but before this PR) keeps the gateway in the checkout object. So, when UPE is turned on, WC_Payments_UPE_Checkout with the $card_gateway is being initialized and when the time comes, the $card_gateway is being successfully rendered by WC_Payments_UPE_Checkout as a single payment element with all the bits and pieces inside of it.

Our mission was to use this brand new checkout class in order to render all the registered payment methods, which are now represented by a group of split payments, instead of only $card_gateway.

There were multiple options to achieve this. First of all, we could create a WC_Payments_UPE_Checkout object for each gateway. In practice, with ten methods available, doing this seems not optimal. I think that the reason for the refactor was to abstract the payment fields UI rendering part to a separate class, and with the approach I describe, we would multiply the class with the same purpose ten times and use the memory without any meaningful reason.

Another option was to ensure, that only one instance of the WC_Payments_UPE_Checkout is created, and each $gateway provides itself as a callback parameter of the action hook in this place. Then, we could set the $gateway in the checkout class and use it while rendering. However, this approach most probably leads to the violation of Liskov substitution principle, since payment_fields() method's definition will be updated by adding a new parameter ($gateway). We then would need to set_gateway in the payment_fields() method and thus change the state of the class. All this seemed a bit "unusual" to me.

Finally, I went with the approach, which allows us to store all the payment gateways in the WC_Payments_UPE_Checkout class. When rendering the payment fields, we invoke the callback from a new wc_payments_set_gateway hook I added and then proceed with the regular flow. This allowed us to keep the purpose of the WC_Payments_UPE_Checkout class by creating only one instance of it, and at the same time, made the operation of setting the gateway loosely coupled with the object itself (if $gateway won't be set, then $card_gateway Stripe element will be used).

Another thing worth mentioning is, that the non-UPE checkout should be initialized even when we are dealing with the UPE setup, since it defines the hook which is crucial for the legacy card to be rendered properly. This is why it's initialized regardless of wether users have UPE turned on or not.

Note: Please keep in mind, that the legacy card gateway is not working with the Blocks checkout after this change. I assume that it was a part of discussion here and it will be resolved in this issue.

Testing instructions

1. Ensure that the classic UPE checkout works as expected with both the legacy card and UPE elements.
2. Ensure that the non-UPE checkout works as expected (turn off UPE in the admin panel).
3. Ensure that blocks checkout works for split UPE elements. (please see the note right above the testing instructions).
4. Ensure that SEPA tokens are managed as expected

---

• 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. (was not needed, since the PR will be merged to the feature branch).
• Covered with tests (or have a good reason not to test in description ☝️) - existing tests were updated.
• 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.

[FangedParakeet]

There were about ten trillion files changed here, so I tried my best to focus on the most important ones. 😅 This was truly a rhinocerous-sized refactor, so nice one resolving all these changes as best as you could!

In general, I think everything is fine here. I've left a few comments, but almost all of them are pretty inconsequential. I did encounter a few new errors, but I'll describe them below and I think almost all can be moved to new issues.

Let me just get into it, by stepping through your testing instructions one-by-one.

1. Ensure that the classic UPE checkout works as expected with both the legacy card and UPE elements.

Using the shortcode checkout, I tested the following with both the non-UPE card gateway and other UPE gateways.

• Standard checkout
• Failed checkout
• Checkout while saving payment method
• Checkout with saved payment method
• Checkout with card requiring 3DS authentication

...And all of the above, appeared to be working normally for me! Huzzah.

2. Ensure that the non-UPE checkout works as expected (turn off UPE in the admin panel).

Since the non-UPE card gateway was working properly in the previous tests, I wasn't expecting there to be much difference with the UPE disabled. Nonetheless, without testing as many flows as in the previous step, I did test the basic regular shortcode and blocks flows and discovered that the shortcode checkout appeared to be working as expected (🎉), while the blocks checkout was not working at all, which leads neatly into the next section.

3. Ensure that blocks checkout works for split UPE elements. (please see the note right above the testing instructions).

The legacy non-UPE gateway appeared broken for me. I was able to checkout with a saved payment method, but could not complete a blocks checkout using a new credit card. I don't think this is related to the issues I raised here, as you might have opined.

Digging a little further I found that the following error was returned to the checkout.

Error: You passed an empty string for 'payment_method_types[0]'. We assume empty values are an attempt to unset a parameter; however 'payment_method_types[0]' cannot be unset. You should remove 'payment_method_types[0]' from your request or supply a non-empty value.

I believe this error refers to this parameter, which @mdmoore also grappled with while resolving the saved payment method functionality. I'm not entirely sure how this occurred, but checking my error logs, this payment_method_types parameter was filled with a single empty string in our request to Stripe.

2022-11-15T01:30:42+00:00 INFO REQUEST POST https://public-api.wordpress.com/wpcom/v2/sites/%s/wcpay/intentions
2022-11-15T01:30:42+00:00 INFO HEADERS: array (
  'Content-Type' => 'application/json; charset=utf-8',
  'User-Agent' => 'WooCommerce Payments/5.0.1',
  'Idempotency-Key' => '3afc322e-7bad-4b22-89b8-1c1c8e33866c',
)
2022-11-15T01:30:42+00:00 INFO BODY: array (
  'test_mode' => true,
  'amount' => 2779,
  'currency' => 'eur',
  'payment_method' => 'pm_1M4Dz12H1rYvbvjIUQiWe4Us',
  'customer' => 'cus_LXS3WAVT5O8VZT',
  // ...
  'description' => 'Online Payment for Order #244 for fangedparakeet.jurassic.tube blog_id 191029001',
  'payment_method_types' => 
  array (
    0 => '',
  ),
  'is_platform_payment_method' => false,
)

I'm pretty sure this is a new issue, but I'm happy to resolve this in a separate issue, because it does seem that something uniquely unusual is afoot here.

For the rest, I believe things were working similarly to my experiences noted here. I was able to complete a checkout with a new UPE payment method and a saved non-UPE payment method, but not a saved UPE payment method. There is already an issue to resolve this limitation, so I'm happy to ignore that here.

4. Ensure that SEPA tokens are managed as expected

Everything here seemed to be in proper working order! I wasn't really expecting anything here to go up in flames, but from the shortcode checkout and the My Account pages, managing SEPA tokens seemed to be doing it was supposed to.

Those are my thoughts, let me know if you have any immediate comments! I am still approving this PR, because I think the main refactor is fine! The only real issue is using cards in the blocks checkout, but, as stated before, more than happy to resolve that elsewhere.

I think it could be beneficial to also add some more comprehensive confidence checking to checkouts from the Order Pay page, Subscriptions--especially free-trial subscriptions, that rely on setup intents rather than payment intents--and combinations of these two checkout flows, since there are a few uncertainties surrounding these areas that I've raised in separate comments. That being said, I think these examinations can be done another day and I'm eager to catch up with the elusive develop branch in the meantime.

Great work, @timur27! For the most part, LGTM. 👍

[timur27]

Thanks for the review, @FangedParakeet!

I don't think this is related to the issues I raised #4670 (review), as you might have opined.

To be more specific, I was referring to this paragraph.

Before making this PR ready for the review, I was able to make the use case with a new credit card work by rolling back the change for this line of code. So, when I provide

$selected_payment_method_type = [ WC_Payments::get_gateway()->get_selected_stripe_payment_type_id() ];

instead of

$upe_payment_method = sanitize_text_field( wp_unslash( $_POST['payment_method'] ?? '' ) );

To the payments_api_client->create_and_confirm_intention, Blocks checkout with a legacy card works like a charm. However, I was skeptical about fixing this here and in this way because I felt this might break tokens for SEPA again. At the same time, I noticed the issue with the acceptance criteria While UPE is enabled, completing checkout from blocks checkout with a credit card should process without error. I thought then that the issue could be fixed as part of it and thus linked it in the description.

[FangedParakeet]

Before making this PR ready for the review, I was able to make the use case with a new credit card work by rolling back the change for this line of code. So, when I provide

$selected_payment_method_type = [ WC_Payments::get_gateway()->get_selected_stripe_payment_type_id() ];

You would first have to change the return value of WC_Payment_Gateway_WCPay::get_selected_stripe_payment_type_id() to a single string ('card'), but I think this could work. I actually think this would work in all situations, but there are definitely a few more edge cases we'd need to test out first, so--as you offered--I think it's best to resolve this in a new issue.

Also, I just noticed one more thing! (Sorry.)

In this merge branch, for some reason, the WCPay gateway seems to have disappeared from the main WC Payments Settings screen (WooCommerce > Settings > Payments > All payment methods).

O brother where art thou?

It's still there on both the develop branch and the poc/upe-instances-multiplied branch.

Regular correct settings page

If you think there's a quick fix for this, let me know: otherwise I'll create a new issue to resolve this missing method, once you've completed this merge.

Also, we had some discussion above about the gateway description field: I just noticed that these description values are actually incorrect in our PoC branch (see above screenshot). I'll create a new issue to fix this as well shortly...

[timur27]

Thanks for another round of review, @FangedParakeet ! I fixed the failing test (a4222e7) and the hidden card gateway in the admin panel (1bebfee).

[FangedParakeet]

LGTM! I think pretty much everything pending from here has an open issue already ready to capture further remaining work--if not, please feel free to pop any open, if I missed anything!

Otherwise let's get this merged and finally get our branch up to date.