-
Notifications
You must be signed in to change notification settings - Fork 69
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
POC: Multiple instances of single-method UPE at checkout #3968
Conversation
… upe payment methods as separate gateways
<input id="wcpay-payment-method-upe" type="hidden" name="wcpay-payment-method-upe" /> | ||
<input id="wcpay_selected_upe_payment_type" type="hidden" name="wcpay_selected_upe_payment_type" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these two inputs used or relied upon anywhere in the codebase? I couldn't find any reference to their names nor ids.
Tracking the "Side Effect" type issues for this POC here for now: ✅ Saving payment methods / using saved payment methodsConfirmed that cards can't be yet saved. We get this error:
✅ SubscriptionsSubscription purchases and renewals are processing as expected. Only gateways that support subscriptions are appearing on checkout 👍. ✅ Notice PollutionSee the image above. Issue: #4394 ✅ Pay for Order pageCheckout can't be completed from the Pay for Order page. This error is appearing in the console:
This seems to be related to the change in the way the payment method type is handled. Issue: #4395 ✅ Add Payment Methods from My AccountThis error appears on the page:
This is due to all payment gateways appearing on the page when only the card element should appear. We'll need to filter out gateways that don't support tokenization when register gateways. We can check if we're on the Add Payment Page and if the payment method is reusable. Issue: #4396 ✅ Multi-currencyDouble check that multi-currency is still working correctly. Issue: #4397 Payment Request ButtonsDouble check Payment Request Buttons Issue: #4398 |
@harriswong I don't have the same error when adding payment methods. I've just tried again, enabling those specific methods and the settings save as expected. |
* Display the platform checkout block when necessary * Run necessary checks for CC running in Blocks * Increase test coverage * Cosmetic updates Co-authored-by: Timur Karimov <timurkarimov@timurs-macbook-pro.home>
* Set the margin area of the card element to be the same as in the upe element * Avoid displaying saved methods empty HTML element when there are no saved tokens * Align the padding of UPE fieldset with the CC * Cosmetic changes * Cross-browser checkbox-label alignment Co-authored-by: Timur Karimov <timurkarimov@timurs-macbook-pro.home>
…hould be displayed for a UPE payment method. (#5494) * Improve the decision making if a payment method should show the save option * Include the test to cover the scenario where SEPA has saved cards disabled * Fix the property supply and improve the if statement for reusable methods * Simplify UPE component rerendering by using the existing flag * Fix method description --------- Co-authored-by: Timur Karimov <timurkarimov@timurs-macbook-pro.home>
* Use a single separator for express checkout methods * Consolidate express checkout enablement checks inline
…5496) * adds new split upe payment gateway child class * fixes classic checkout client scripts for regular and split upe * amends legacy gateway and upe styles for all available upe * fixes hooks for legacy upe gateway * fixes token service * upe checkout compatibility fix * fixes upe checkout class * adds compatibility to wc_payments class * adds new split upe scripts to webpack, fix php errors * adds feature flag compatibility, fixes legacy card gateway checkout * adds compatibility fixes, all three gateways function properly in classic checkout behind separate flags * adds tri-gateway support for blocks checkout * fixes token service to set correct gateway id for relevant upe flavour * fixes e2e tests for both upe feature flags * amends unit tests * removes useless use statement * fixes php unit test errors * fixes saved payment method test * fixes failing unit tests due to missing feature flag * adds missing parameter to request to create setup intent * adds missing session intent function to parent upe class * fixes e2e suite test groups parameter Co-authored-by: Timur Karimov <timur.karimow.95@gmail.com> * fixes doc comment typo Co-authored-by: Timur Karimov <timur.karimow.95@gmail.com> * fixes e2e test branches typo Co-authored-by: Timur Karimov <timur.karimow.95@gmail.com> * adds consistency to stripe api js paymentMethodType checks * split upe blocks js uses showSaveOption parameter appropriately * removes redundant and repetitive looping when initialising legacy and split upe * adds consistent logic to if branches in register_gateway hook * fixes webpack typo * fixes bug that removes all additional payment gateways --------- Co-authored-by: Timur Karimov <timur.karimow.95@gmail.com>
Co-authored-by: Brett Shumaker <brettshumaker@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting this in, after a long time
Changes proposed in this Pull Request
UPE Payment Methods as Separate Payment Gateways
The changes in this PR show a glimpse of a radical yet visionary future, where the enabled UPE payment methods exist as separate payment gateways. Please read this P2 post for more context on the reasoning for this. Essentially, this makes payment options at checkout as more visually understandable for customers: no more are all existing payment methods crammed under the WooCommerce Payments' "Popular Payment Methods" umbrella at checkout, but now they can be hosted as separate payment gateways, in the same way that any other payment method would be.
UPE reimagined and reloaded
In order to achieve this I have broken the existing UPE Payment Gateway into a class that supports a single payment method--as opposed to multiple--in its constructor, managing the lifetime of payment intents for only a single payment method at a time. This creates a new gateway at checkout for each payment method with a separate payment element; however, since payment elements must be primed with the client secret from a valid payment intent, this implies that for each checkout we must create a separate payment intent for each payment method, necessitating separate requests to the Payments Server to create each intent. Furthermore, once the checkout is completed, only one payment intent is updated and the rest will remain in their initial state in limbo for all eternity. Since payment intents are now tied to the WC Session, a tradition I have continued in this PR, this reduces the number of excess payment intents somewhat, as refreshing the page will not create new payment intents; but they will still now always be unused payment intents at checkout.
To maintain the previous gateway for the card method, so that this gateway can use Stripe Elements instead of the UPE, when the UPE is enabled I create an instance of both the
UPE_Payment_Gateway
andWC_Payment_Gateway_WCPay
, the latter representing the legacy card gateway that uses Stripe Elements. Whenever the$card_gateway
is called upon, I use theUPE_Payment_Gateway
instance, so that the new UPE settings are used; however, I insert the legacyWC_Payment_Gateway_WCPay
instance instead into the array of available gateways when registering the gateway. This allows us to use the UPE settings while in the admin pages, while using the Stripe Elements from the previous gateway at checkout. This switcharoo probably breaks a few things, but I'm not looking for them just yet--the work here is just to prove that this is possible!Side-Effects
So far I have only really tested these changes with the regular checkout flow. Here are a few more things that I have either not fixed or not really tested with this refactoring of the UPE.
This is not to say that these things are not possible--on the contrary, I think they are all definitely very achievable! However, they are unique processes that require specific flows, which I have not aimed to cater towards in this PR, since I only wanted to prove that we could complete a regular checkout with the UPE existing in separate payment gateways.
Additionally, adding the UPE payment gateways as separate gateways does not add the payment gateways to the main settings screen. This means that we can continue to manage these payment methods using the new settings within WCPay's own settings. Note that I haven't yet implemented those latter changes here, so if that works currently, it is another accident.
Settings for
All payment methods
only contains WooCommerce PaymentsWCPay main settings can function as normal
However, the
UPE_Payment_Gateway
class inherits from the mainWC_Payment_Gateway_WCPay
class and usually there is only expected to be one of these gateways present at any time. In this implementation, each payment method is a separate gateway, which is in turn a separate instance ofUPE_Payment_Gateway
, continuing to extend its parent, so that I can make use of theprocess_payment
function, as well as of the settings fromWC_Payment_Gateway_WCPay
. This means that many features become duplicated, since there will now always be more than one instance of this class at any time: this includes things such as hooks, admin notices, and AJAX functions fromWC_Payment_Gateway_WCPay
.You will find the most obvious occurence of this redundant replication inside the WCPay settings, where the top of the page is filled up with all of these multiplied notices.
Notice pollution
This is a side-effect of the main design choice of this PR in inheriting and multiplying these classes, so this will need to be solved somehow.
Also, there are no tests here and all the existing tests are probably going to be broken. Boo!
Testing instructions
npm run changelog
to add a changelog file, choosepatch
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.Post merge