Conversation
mikejolley added the skip-changelog 
woocommercebot requested review from a team and tarhi-saad and removed request for a team 
| top: 0; | ||
| text-align: center; | ||
| transform: translateX(-50%); | ||
| white-space: nowrap; |
There was a problem hiding this comment.
Small fix for to stop these periods wrapping
| 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
Detailsassets/js/base/components/cart-checkout/shipping-rates-control/index.tsxassets/js/base/context/hooks/payment-methods/use-payment-method-interface.ts assets/js/base/context/hooks/shipping/use-shipping-data.ts assets/js/base/context/providers/cart-checkout/shipping/index.tsx assets/js/base/context/providers/cart-checkout/shipping/reducers.js assets/js/blocks/checkout/inner-blocks/checkout-pickup-options-block/block.tsx packages/checkout/components/store-notices-container/test/index.tsx |
| } | ||
| onSelectRate={ ( newShippingRateId ) => { | ||
| selectShippingRate( newShippingRateId, packageId ); | ||
| dispatchCheckoutEvent( 'set-selected-shipping-rate', { |
There was a problem hiding this comment.
Moved here from useSelectShippingRate which I've merged with useShippingData. The 2 hooks did very similar things.
| Size Change: +519 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
| | ||
| const findParentContainer = ( container: string ): string => { | ||
| if ( container.includes( noticeContexts.CHECKOUT + '/' ) ) { | ||
| if ( container.includes( noticeContexts.CHECKOUT ) ) { |
There was a problem hiding this comment.
Removed the slash do that wc/cart errors surface in wc/checkout if the StoreNoticesContainer is not present.
There was a problem hiding this comment.
Could you explain this a bit more, not sure I understand fully what the slash did and why removing it is needed
There was a problem hiding this comment.
The logic was looking for a prefix of wc/cart/ and then selecting either the parent container, or switching to wc/checkout.
This worked for children of wc/cart/ but not for wc/cart itself. Removing the slash lets it change the container if the wc/cart container was passed in.
There was a problem hiding this comment.
Ahhh right yes because we add errors to inner blocks. Nice one thanks!
| } | ||
| | ||
| // Re-throw the error. | ||
| throw error; |
There was a problem hiding this comment.
These did nothing. We cannot catch them because the function is async.
| activeElement && | ||
| inputs.indexOf( activeElement.tagName.toLowerCase() ) !== -1 | ||
| inputs.indexOf( activeElement.tagName.toLowerCase() ) !== -1 && | ||
| activeElement.getAttribute( 'type' ) !== 'radio' |
There was a problem hiding this comment.
Small change allows us to scroll to errors after clicking a radio input.
| @@ -0,0 +1,42 @@ | |||
| /** | |||
There was a problem hiding this comment.
These types were all moved from the old type-defs
| dispatch.receiveError( error ); | ||
| | ||
| // If updated cart state was returned, also update that. | ||
| if ( error.data?.cart ) { |
There was a problem hiding this comment.
Moved this logic into receiveError thunk.
| code: string; | ||
| message: string; | ||
| data: ApiErrorResponseData; | ||
| data?: ApiErrorResponseData | undefined; |
There was a problem hiding this comment.
Not all API errors have data - it's optional.
opr left a comment
opr left a comment There was a problem hiding this comment.
This looks good, when testing with die( 1 ) it shows
Could we make this more user-friendly, like "Something went wrong, please contact us for assistance." instead?
I also noticed that when retrying on the same page, after causing the generic error, and then trying with the thrown named error, the generic error still remains. Is it possible to get around this? I think this is the same problem as we had with the quantity messages, vs auto-dismissed and persistent errors isn't it?
| | ||
| const findParentContainer = ( container: string ): string => { | ||
| if ( container.includes( noticeContexts.CHECKOUT + '/' ) ) { | ||
| if ( container.includes( noticeContexts.CHECKOUT ) ) { |
There was a problem hiding this comment.
Could you explain this a bit more, not sure I understand fully what the slash did and why removing it is needed
Similar issue @opr Will the PR you're working on which dismisses notices work here too? |
mikejolley force-pushed the fix/expose-cart-errors-as-notices branch 4 times, most recently from 887b3fe to be006fa Compare opr left a comment
opr left a comment There was a problem hiding this comment.
Functionality-wise this is working ok according to test instructions, but I added a few comments please take a look
assets/js/base/context/providers/cart-checkout/shipping/index.tsx Outdated Show resolved Hide resolved
opr left a comment
opr left a comment There was a problem hiding this comment.
Looks good now, the test comment could maybe be updated to reflect that it's the number of elements containing matching text, rather than instances of the text occurring?
Besides that, nice one. Thanks for working on this!
Overlapping functionality and responsibility easily merged into a single hook.
… original context
Co-authored-by: Thomas Roberts <5656702+opr@users.noreply.github.com>
Co-authored-by: Thomas Roberts <5656702+opr@users.noreply.github.com>
mikejolley force-pushed the fix/expose-cart-errors-as-notices branch from 6daf0cf to 1154fc2 Compare | e2e fails seem to be fse related. JS tests are passing here and for me locally, so merging. |
2 participants
We identified an issue that API responses that returned errors were being stored to the data stores, but not converted to notices.
To resolve this, I've converted
receiveErrorto a thunk that creates error notices, following the pattern ofreceiveCart/notifyQuantityChangesthat @opr implemented.When detangling this problem I converted a few files to Typescript and merged some hooks. Ill add notes in the diff.
Fixes #7721
Other Checks
Testing
Automated Tests
User Facing Testing
We need to test this by throwing errors from the API. Add some error code here:
woocommerce-blocks/src/StoreApi/Routes/V1/CartSelectShippingRate.php
Line 74 in 95e4604
You can try:
die( 1 );to test a generic errorthrow new RouteException( 'test-error', 'Test', 400 );to test a named errorThere is no other error handling or retry mechanism beyond this (out of scope).
Since this revises the changes in #7363, repeat these testing steps also:
WooCommerce Visibility