This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Cart Action Promises with success/reject handling #8272
Merged
Conversation
This file contains 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
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/base/components/cart-checkout/totals/coupon/index.tsx
assets/js/base/components/cart-checkout/totals/coupon/stories/index.tsx assets/js/base/context/hooks/cart/use-store-cart.ts |
|
Size Change: -295 B (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
mikejolley
added
block: cart
mikejolley
force-pushed
the
add/cart-action-promises-reject-handling
branch
from
69bf5f8 to
c2cb66b
Compare
woocommercebot
requested review from
a team and
nielslange
and removed request for
a team
mikejolley
force-pushed
the
add/cart-action-promises-reject-handling
branch
from
f998874 to
b13ad8c
Compare
Many of the functions that return promises in this file n...Many of the functions that return promises in this file need to be moved to thunks.ts.
woocommerce-blocks/assets/js/data/cart/actions.ts Lines 29 to 32 in 62da5b2 🚀 This comment was generated by the automations bot based on a
|
mikejolley
commented
Jan 25, 2023
| /** | ||
| * This function is used to notify the user of cart errors. | ||
| */ | ||
| export const notifyErrors = ( error: ApiErrorResponse | null = null ) => { |
There was a problem hiding this comment.
This has been consolidated into processErrorResponse.
mikejolley
commented
Jan 25, 2023
| @@ -23,8 +23,10 @@ import { ACTION_TYPES as types } from './action-types'; | |||
| import { apiFetchWithHeaders } from '../shared-controls'; | |||
There was a problem hiding this comment.
Each function here will now work in the way way; returning a promise with either the successful response, or rejecting with the error message so that consumers can handle the error objects.
There was a problem hiding this comment.
This effectively means we fully moved out notice triggering from data stores and into hooks?
There was a problem hiding this comment.
@senadir There is some still in receiveCart, but yeah for errors its back inside hooks.
mikejolley
commented
Jan 25, 2023
| @@ -275,7 +276,8 @@ const CheckoutProcessor = () => { | |||
| ) | |||
| .then( ( response: CheckoutResponseError ) => { | |||
| if ( response.data?.cart ) { | |||
| receiveCart( response.data.cart ); | |||
| // We don't want to receive the address here because it will overwrite fields. | |||
| receiveCartContents( response.data.cart ); | |||
There was a problem hiding this comment.
This fixes the address data being lost after a failed checkout request if data has not yet persisted. https://a8c.slack.com/archives/C8X6Q7XQU/p1674643137575739
There was a problem hiding this comment.
I couldn't find where receiveCartContents is defined so I assume it was already in trunk, I'm trying to understand its role here, at least, I understand that it sets everything except the addresses, but why do we need to do that? Why is the data returned from the server inconsistent? if addresses didn't persist, shouldn't we assume line items and such are going to be incorrect here and just reject the whole thing and show errors?
There was a problem hiding this comment.
It's in trunk already (#7711). Its there because when you're persisting address data to the server, in particular partial address data, it is not desirable to update the stores with the response—the data in the stores may be more current. We needed this change after introducing partial pushes, since the partial push does not include all current data.
There was a problem hiding this comment.
Gotcha! in that case, are partial updates the only place where we deploy receiveCartContents?
There was a problem hiding this comment.
Yes and when editing (updateCustomerData), which is where receiveCartContents was already in use.
mikejolley
commented
Jan 25, 2023
| } ) | ||
| .catch( ( error ) => { | ||
| createErrorNotice( error.message, { | ||
| id: 'coupon-form', | ||
| context, | ||
| } ); | ||
| // Finished handling the coupon. | ||
| receiveApplyingCoupon( '' ); |
There was a problem hiding this comment.
This is handled in the thunks already.
mikejolley
force-pushed
the
add/cart-action-promises-reject-handling
branch
from
1948990 to
976efa0
Compare
nielslange
added
the
status: blocker
senadir
approved these changes
Jan 30, 2023
Comment on lines
+74
to
+83
| if ( onSubmit !== undefined ) { | ||
| onSubmit( couponValue ).then( ( result ) => { | ||
| if ( result ) { | ||
| setCouponValue( '' ); | ||
| setIsCouponFormHidden( true ); | ||
| } | ||
| } ); | ||
| } else { | ||
| setCouponValue( '' ); | ||
| setIsCouponFormHidden( true ); |
There was a problem hiding this comment.
This is possible confusing, why is onSubmit ever undefined?
There was a problem hiding this comment.
The typedef allows for undefined, so this just handles that case.
| @@ -23,8 +23,10 @@ import { ACTION_TYPES as types } from './action-types'; | |||
| import { apiFetchWithHeaders } from '../shared-controls'; | |||
There was a problem hiding this comment.
This effectively means we fully moved out notice triggering from data stores and into hooks?
| @@ -275,7 +276,8 @@ const CheckoutProcessor = () => { | |||
| ) | |||
| .then( ( response: CheckoutResponseError ) => { | |||
| if ( response.data?.cart ) { | |||
| receiveCart( response.data.cart ); | |||
| // We don't want to receive the address here because it will overwrite fields. | |||
| receiveCartContents( response.data.cart ); | |||
There was a problem hiding this comment.
I couldn't find where receiveCartContents is defined so I assume it was already in trunk, I'm trying to understand its role here, at least, I understand that it sets everything except the addresses, but why do we need to do that? Why is the data returned from the server inconsistent? if addresses didn't persist, shouldn't we assume line items and such are going to be incorrect here and just reject the whole thing and show errors?
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
nielslange
approved these changes
Jan 30, 2023
nielslange
added
skip-changelog
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.
This PR revises cart error handling so that consumers of the action thunks can dispatch errors (or choose not to), rather than all errors being raised via
notifyErrors. This allows extensions such as gift cards to circumvent the notice system and creating their own notices.This also ensures that all thunks return a promise. This allows consumers to wait for the action to resolve and then handle success/failure, and avoids them monitoring for changes in the data stores using
useEffect.Finally, this merges the error handling code that was split between
processErrorResponseandnotifyErrors.An example of usage of this pattern was introduced in #8182 The shipping calculated form uses the promises to show the success or failure state, and also creates a notice under the shipping calculator context so it's displayed in the correct place.
Another consumer (updated in this PR) was the coupon form; previously it had to use
useEffectto wait for changes in the data store, but by using promises instead it can just wait for success/fail before toggling itself off after success.The issue with the current system in trunk was that all notices were thrown internally before extensions had a chance to deal with them, and the errors were no longer surfaced due to the removal of
throw error. This was reported here: p1674473844393499-slack-C8X6Q7XQUTesting
This requires smoke testing in a few different areas that utilise notices.
Shipping Calculator
Coupon Form
Cart notices
Repeat these testing steps and ensure there are no regressions:
#7938
And these:
#8162
API Errors
Edit this route and add
die(1): https://github.com/woocommerce/woocommerce-blocks/blob/trunk/src/StoreApi/Routes/V1/CartSelectShippingRate.php#L74Go to cart and select a shipping rate. Confirm you see a top level error notice in the cart.
Repeat this on the checkout page.
WooCommerce Visibility