Fix flaw with event emitters and complete checkout flow for the cheque payment method. #2108
Conversation
nerrad
requested review from
Aljullu and
mikejolley
and removed request for
a team
nerrad
added
skip-changelog
|
Size Change: -108 kB (5%) ✅ Total Size: 2.06 MB
ℹ️ View Unchanged
|
|
I have no idea why bundlesize is reporting 0B for the payment method builds. I tried a production build of them locally and didn't have any issue. |
There was a problem hiding this comment.
Changes look good to me and Check payment works like a charm! 🎉 I left some minor comments below but I don't think they should be blocking.
Validation errors after submit button is clicked may have regressed. Please do test, but we can fix in follow-ups unless the cause is clear.
I could verify this. Validation errors do not allow the checkout process to complete, but they don't show up.
| const getObserversByPriority = ( observers, eventType ) => { | ||
| return observers[ eventType ] | ||
| ? Array.from( observers[ eventType ].values() ).sort( ( a, b ) => { | ||
| return a.priority > b.priority ? -1 : 1; |
There was a problem hiding this comment.
This could be rewritten as a subtraction, if you want:
| return a.priority > b.priority ? -1 : 1; | |
| return b.priority - a.priority; |
| @@ -56,4 +62,20 @@ describe( 'Testing emitters', () => { | |||
| } | |||
| ); | |||
| } ); | |||
| describe( 'Test Priority', () => { | |||
| it( 'Executes observers in expected order by priority', async () => { | |||
There was a problem hiding this comment.
Nit: we usually write test descriptions in lowercase:
| it( 'Executes observers in expected order by priority', async () => { | |
| it( 'executes observers in expected order by priority', async () => { |
| ) | ||
| ); | ||
| }; | ||
| }, |
There was a problem hiding this comment.
No need to do it in this pull, but this looks like something that could be abstracted. These four methods are the same just changing the type. Same for checkout-state/event-emit.js and shipping/event-emit.js.
There was a problem hiding this comment.
Ya this is a good idea. Done in 01eb74c
| */ | ||
| const Content = ( { activePaymentMethod, eventRegistration } ) => { | ||
| // hook into payment processing event. | ||
| useEffect( () => { |
There was a problem hiding this comment.
Can you help me understand what this does exactly?
Can we abstract/import it so gateways have less to worry about?
There was a problem hiding this comment.
Basically every gateway will have to at a minimum register an observer on the onPaymentProcessing event. This event fires when checkout has initiated payment processing as a part of the flow. What gateways return from this callback when it's invoked determines whether the gateway was succesful, failed, or errored for it's processing. The possible shapes of the return value are:
Success
Can return:
true- OR an object:
{ paymentMethodData, billingData }
Fail:
Return an object: { fail: { errorMessage, paymentMethodData } }
Error:
Return an object: { errorMessage, validationErrors }
- errorMessage is used as just a general error message to display in the payment method step.
- validationErrors is an object where keys are field ids and values are the error message for that field (to help with displaying field validation errors for for the payment method).
There was a problem hiding this comment.
Can we abstract/import it so gateways have less to worry about?
So one thing I was thinking we could do is have a function for registering really simple gateways that are similar to cheque in that they all the processing happens server side. So it'd have a fairly simple signature (gateway name, maybe icon, and text for the content window). Then this function would take care of the payment method registration (with Config etc) using components common to all these payment methods. We could export this as a helper on the block-registry global.
With master having the necessary pieces in place to theoretically complete a payment using the cheque payment method and fulfill the checkout flow (including a redirect to order receipt page), I took some time to test things out and fix bugs that popped up.
In the process I discovered a serious flaw with the event emitters that I overlooked when designing the system. The flaw is that all the registered observers on the event emitters cannot react to any state changes because they are all invoked in one render process. This also means that any state changes in observers can be problematic because subsequent observers would not know of those state changes.
The solution involves recognizing that we are using the event emitters for tracking flow through the checkout and to limit status and state changes to after the event has emitted. By adding event emitters to the PaymentMethodData context, and adding another Checkout status it actually ended up simplifying the flow logic because now there is a stable point for payment methods to hook in their behaviour and for the
CheckoutProcessorcomponent to hook in it's logic.With the changes in this pull:
Here's a rough diagram of the flow (not including shipping events):
In this pull:
onPaymentProcessingevent emitter.To Test
It is expected that Cart/Checkout behaviour up to the point of clicking the related submit button should continue to work as expected outside what was done in this branch.
You should be able to finish a complete checkout flow using the cheque payment method without errors.
There should be no errors in the console while testing.
Stripe is not expected to work.
Validation errors after submit button is clicked may have regressed. Please do test, but we can fix in follow-ups unless the cause is clear.
Followups:
usePaymentMethodInterfacehook. That forces payment methods to use the appropriate api for hooking into the checkout flow (events). (Remove payment status setters from payment method extension interface. #2110)