Refactor payment status #8110
Refactor payment status #8110
Conversation
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
assets/js/data/checkout/default-state.ts
|
|
Size Change: +206 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
woocommercebot
requested review from
a team and
tarhi-saad
and removed request for
a team
alexflorisca
requested review from
mikejolley,
opr and
senadir
and removed request for
tarhi-saad
As mentioned when pairing on this, I don't think "client-side only" payments are something we would ever have wanted to do, Darren please correct me if I'm wrong, but it seems insecure. Given a payment gateway often has a "secret" key that allows an application to interact with it, allowing this so take place on the client-side seems unreasonable. Perhaps it was made to allow async processing of payments, before the submit button was actually pressed?
I wonder if it would be beneficial to set the status to |
There was a problem hiding this comment.
I won't add an approval or rejection here since I worked on it with you, and I commented above with my thoughts on success/failed.
I just added some inline comments about following the WP Coding guidelines re comments (They should begin with a capital letter and end in a full stop).
I wasn't very clear when giving some historical context. What I meant is that there are some payment methods that have preliminary processing to do client side (that have nothing necessarily to do with completing the payment) usually involving contacting the payment processor (for example getting a token). These statuses provided a way for payment methods to signal to the checkout flow that they were busy doing this so checkout won't submit to the server yet. My memory is blurry but I think this is especially important for express payment methods. So think of this less as "payment status" and more as "payment method readiness/processing status". I also can't remember if these statuses were also used during payment method initialization. |
|
Thanks @nerrad that's super helpful! |
Thanks Darren that makes sense. So in this case, I think having the payment status change to
Yeah I was thinking the same. I'm not even sure we need |
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
opr
removed
the
status: stale
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
alexflorisca
added
type: refactor
| // allows for other observers that return true for continuing through | ||
| // to the next observer (or bailing if there's a problem). | ||
| if ( isPaymentProcessing ) { | ||
| __internalEmitPaymentProcessingEvent( |
There was a problem hiding this comment.
This was moved in a useEffect above, the same one where we set the payment status to processing
| const expectedState = { | ||
| ...defaultState, | ||
| redirectUrl: 'https://example.com', | ||
| status: STATUS.IDLE, |
There was a problem hiding this comment.
The reason for removing all the STATUS.IDLEs here is because we got rid of the reducer code that sets the status to IDLE for the first action performed on the store. That was a sideeffect essentially so we don't need to mock that in the test anymore
| ).toEqual( expectedState ); | ||
| } ); | ||
|
|
||
| it( 'should handle SET_HAS_ERROR when status is anything else', () => { |
There was a problem hiding this comment.
anything else here means not PROCESSING or BEFORE_PROCESSING
assets/js/base/context/hooks/payment-methods/use-payment-method-interface.ts
Outdated
Show resolved
Hide resolved
assets/js/base/context/providers/cart-checkout/payment-events/index.tsx
Outdated
Show resolved
Hide resolved
assets/js/blocks/cart-checkout-shared/payment-methods/express-payment-methods.js
Show resolved
Hide resolved
|
Just a note on deprecating. I've assumed that this PR will be merged before 9.6.0 is released. If it isn't, the version number inside the |
This reverts commit 38d66ed.
…index.tsx Co-authored-by: Mike Jolley <mike.jolley@me.com>
mikejolley
force-pushed
the
fix/payment-status
branch
from
294c203 to
a7bb488
Compare
I've refactored the payment status to make it less confusing and more inline with current Checkout flow.
Payment status was being set to
SUCCESSorFAILbefore the payment actually gets made.We fire an event
PAYMENT_PROCESSING, which enables third party plugins to subscribe to and run some code, returning either aFAIL,SUCCESSorERRORresponse. We then set the payment status in the data store accordingly.Historically (from speaking to @nerrad briefly), this was done to provide an API to allow third party plugins to make payments client side. I think this isn't the case anymore and all payments are actually made server side. In which case, the client side API we provide via the
PAYMENT_PROCESSINGevent, is mainly for creating payment intents and running any code BEFORE the actual payment gets made. This is how stripe works. So while it makes sense to return anERROR, it doesn't make sense to returnFAILorSUCCESS, which implies the payment has been attempted.The
FAILresponse from theonPaymentProcessingobserver was also throwing the Checkout into a weird state, an overlay and loading indicator on the Place Order button, so this wasn't being handled well anyway. To keep backwards compatibility with any plugins which may return aFAILresponse, we now set the payment status in the data store toERRORwhen we receive either anERRORorFAILresponse from the observers.SUCCESSandFAILstatuses and associated actions, action types, selectors and reducers.READYinstead ofSUCCESS, which better indicates that we are in a state ready to make the payment.isPaymentFinishedselector was relying on theSUCCESSandFAILpayment statuses which were being set before the payment had even startedisPaymentFinishedselectorWe reset to
PRISTINEmultiple timesPRISTINEimplies a clean slate, before anything runs, yet we set the status toPRISTINEmultiple times throughout the payment lifecycle.PRISTINEstatus withIDLEThe
STARTEDstatus only applies to express payment methodsSTARTEDstatus toEXPRESS_STARTEDand the associated selectors and actionsCheckout status
PRISTINEis not neededThere is nowhere in the codebase that we check wether the checkout status is PRISTINE or not. There is also some unnecessary logic in the reducer to set the status to IDLE after the first action is performed on the checkout store. If we ever need to check that state of the checkout, it can be done with the data that is in the data store quite simply.
PRISTINEHere is what the happy path will look like after this PR and #8381
Fixes #7667
Testing
User Facing Testing
4000 0000 0000 0002Internal Testing
npm start.4000 0000 0000 0002wc/store/paymentstoreuse-payment-processing.jsand before the return statement on line 123, addFail.has_errorError.has_errorWooCommerce Visibility
Performance Impact
Changelog