This repository was archived by the owner on Feb 23, 2024. It is now read-only.
Refactor the payment status #7666
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
November 10, 2022 15:14
rubikuserbot
requested review from
a team and
tarhi-saad
and removed request for
a team
|
The release ZIP for this PR is accessible via: |
TypeScript Errors ReportFiles with errors: 438
assets/js/atomic/blocks/product-elements/price/index.js
assets/js/atomic/blocks/product-elements/stock-indicator/block.js assets/js/atomic/blocks/product-elements/tag-list/index.ts assets/js/base/components/cart-checkout/product-details/index.tsx assets/js/blocks/cart/block.js assets/js/blocks/checkout/block.tsx assets/js/blocks/mini-cart/frontend.ts assets/js/blocks/product-query/constants.ts |
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the |
|
Size Change: +723 B (0%) Total Size: 991 kB
ℹ️ View Unchanged
|
alexflorisca
added
type: refactor
opr
suggested changes
Nov 14, 2022
There was a problem hiding this comment.
Hey @alexflorisca nice work! Just a couple of things:
In the testing instructions:
You should see 2 calls to SET_PAYMENT_PRISTINE. One comes after we set the default payment method and the other because the checkout is idle and we haven’t completed the payment yet
I only see this once. Just want to double check that this doesn't indicate something being broken?
Go through the checkout flow with a stripe CC and check the SET_PAYMENT_PRISTINE action is fired and status changed to pristine and the console log shows. This is a really unlikely (impossible maybe?) scenario that resets the payment status to pristine, given an error with the checkout, after the payment has been successful.
I see this, and I see the error briefly before being redirected to the success page, is this the expected behaviour?
docs/third-party-developers/extensibility/data-store/payment.md
Outdated
Show resolved
Hide resolved
docs/third-party-developers/extensibility/data-store/payment.md
Outdated
Show resolved
Hide resolved
docs/third-party-developers/extensibility/data-store/payment.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Thomas Roberts <5656702+opr@users.noreply.github.com>
|
Thanks for the review @opr , I've added your suggestions in now.
Yeah it's fine, it's probably because you have some saved payment methods, in which case it will skip running one of them.
Yep that's expected. I forgot to say in the testing instructions you can comment out the |
opr
reviewed
Nov 14, 2022
opr
approved these changes
Nov 15, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The payment status in the payment data store is composed of an object with a bunch of boolean properties that don't always make sense. For example
isProcessingcan be true at the the same time asisPristine. See #6998 for more details.This PR refactors the payment status to have one string value with the status. The various boolean helpers are now selectors. Things to note:
currentStatusobject with selectors for querying the payment statususePaymentMethodInterfacehook exposes thepaymentStatusas it was before.__internalSetCurrentStatusaction because it is marked as internal, hasn't been advertised to third party devs and hasn't been around that long (a month or two since we merged the data store work);getCurrentStatusselector as it is documented and available to third parties, even though this is fairly new so I doubt anyone has had a chance to implement it.Fixes #6998
Other Checks
Testing
Automated Tests
User Facing Testing
SET_PAYMENT_PRISTINE. One comes after we set the default payment method and the other because the checkout is idle and we haven’t completed the payment yetSET_PAYMENT_STARTEDaction fired and thestatuschange tostartedSET_PAYMENT_PRISTINEaction fired and status changed topristinewoocommerce-blocks/assets/js/data/checkout/utils.ts
Line 168 in d295a71
dispatch.__internalSetHasError( true );(and maybe a console log) on line 167.SET_PAYMENT_PRISTINEaction is fired and status changed topristineand the console log shows. This is a really unlikely (impossible maybe?) scenario that resets the payment status to pristine, given an error with the checkout, after the payment has been successful.SET_PAYMENT_PROCESSINGaction is fired and the status changed toprocessingand theSET_PAYMENT_METHOD_DATAandSET_PAYMENT_SUCCESSactions are fired and the status and paymentMethodData keys are updated in the storeSET_PAYMENT_FAILEDandSET_PAYMENT_ERRORare difficult to test as it requires modifying code in the stripe plugin. They are also not accurate representations of the state which I have explained in The ERROR and FAIL payment status values are not accurate #7667. For now I think it’s ok to check the code and make sure these set the correct value on the status.WooCommerce Visibility
Changelog