-
Notifications
You must be signed in to change notification settings - Fork 683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expired Guest Cart Errors #1150
Conversation
* This prevents the backstop reducer from triggering an error toast.
* Also updates unit tests.
This pull request is automatically deployed with Now. |
@@ -529,24 +532,9 @@ test('updateItemInCart thunk dispatches special failure if guestCartId is not pr | |||
2, | |||
actions.updateItem.receive(error) | |||
); | |||
// and now, the createGuestCart thunk | |||
expect(dispatch).toHaveBeenNthCalledWith(3, expect.any(Function)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually care about all the other calls to dispatch
in this test ... the previous assertion is the only thing we're after here.
.mockImplementationOnce(() => ({ | ||
cart: {}, | ||
user: { isSignedIn: false } | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getState
is now only called once and cart
and user
are destructured from there.
The tweak to the retry logic made it so that we don't need this third mock implementation.
|
||
// if a guest cart exists in storage, act like we just received it | ||
const guestCartId = await retrieveGuestCartId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't anything special, I just rearranged the dispatches for consistency with other actions.
Previously we'd get an actions.getGuestCart.receive
without a preceeding actions.getGuestCart.request
in the case where we already had a guestCartId
.
// complete before dispatching the error--you don't want an | ||
// upstream action to try and reuse the known-bad ID. | ||
await clearGuestCartId(); | ||
await dispatch(removeCart()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeCart
is new - it calls clearGuestCartId
AND actions.reset
to remove the guestCartId
from redux.
// if so, then delete the cached ID... | ||
// in contrast to the save, make sure storage deletion is | ||
// Delete the cached ID from local storage. | ||
// The reducer handles clearing out the bad ID from Redux. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeItemFromCart
is a little different in that the reducer handles clearing out the bad ID from redux, so we don't have to do it here.
// complete before dispatching the error--you don't want an | ||
// upstream action to try and reuse the known-bad ID. | ||
await clearGuestCartId(); | ||
// then create a new one | ||
await dispatch(createGuestCart()); | ||
// then retry this operation | ||
return thunk(...arguments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sense attempting to retry removing an item - we just got a brand new cart with nothing in it!
// if so, then delete the cached ID... | ||
// in contrast to the save, make sure storage deletion is | ||
// if so, then delete the cached ID from local storage. | ||
// The reducer handles clearing out the bad ID from Redux. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing changed here, I just updated the comment to make this part clear because this function and removeItemFromCart
differ from the others (they don't call removeCart
).
shippingMethods: [], | ||
totals: {} | ||
totals: {}, | ||
updateItemError: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We capture all these errors now to stop the backstop reducer from kicking in.
expect(result).toEqual({ | ||
...initialState, | ||
removeItemError: action.payload | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other tests here pass but should probably be updated.
// then retry this operation | ||
return thunk(...arguments); | ||
// and add the updated item to the new cart. | ||
await dispatch(addItemToCart(payload)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may or may not be controversial so I'll point it out:
Here we've failed to update an item in the cart due to the cart having expired. We've just gotten a brand new (empty) cart, so there's no sense in retrying the update to this item because it won't exist. But we do know what the user wanted, so we add the updated item to the new cart.
Sucks that they lose anything else they had in the cart, but 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we'll have to think this one through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. Let's discuss what we want to do about existing cart items (and pending cart modifications) when the cart expires.
// then retry this operation | ||
return thunk(...arguments); | ||
// and add the updated item to the new cart. | ||
await dispatch(addItemToCart(payload)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we'll have to think this one through.
.set('notFound', '404 Not Found') | ||
.set('internalError', '500 Internal Server Error'); | ||
.set('notFound', 'That page could not be found. Please try again.') | ||
.set('internalError', 'Something went wrong. Please try again.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I don't think this change helps much—if anything, it makes it more obscure. If we want to make the message friendlier, let's do it right with a real design and some imagery. Until then, I'd rather leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I do think the 500 Internal Server Error
is misleading though, especially since that codepath can be executed without a 500
actually occurring.
Also misleading is that these are server-side error codes, when the problem (aside from the 404) can be in the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. They're not literally server error codes; they're intended to mimic what you'd see if this were a server-rendered app, so we know what case we're in. Ultimately, I don't mind changing the copy if it bothers you.
.set('notFound', '404 Not Found') | ||
.set('internalError', '500 Internal Server Error'); | ||
.set('notFound', 'That page could not be found. Please try again.') | ||
.set('internalError', 'Something went wrong. Please try again.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. They're not literally server error codes; they're intended to mimic what you'd see if this were a server-rendered app, so we know what case we're in. Ultimately, I don't mind changing the copy if it bothers you.
…o-research/pwa-studio into supernova/1143-expired_guest_cart
PR Updated:
|
PR Updated:
|
QA Pass. @jimbo Can you please review the new changes and approve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes make sense. If this passes QA, thumbs up from me. 👍
19ec0ba
to
5c3e147
Compare
pr-test and pr-build both pass bundlesize check, seems like webhook update is pending. Merging this. |
Description
There were a handful of errors relating to stale data, specifically the
cartId
. This PR:addItemToCart
,updateItemInCart
, andremoveItemFromCart
so that the backstop reducer doesn't display an error toastupdateItemInCart
andremoveItemFromCart
to not actually retry these operations because when we determine that the guest cart has expired we get a new cart. Retrying these operations on a new cart doesn't make sense, as the cart will be empty.updateItemInCart
instead attempts toaddItemToCart
to the new cart.Related Issue
Closes #1143 .
Verification Steps
For each test case below, ensure that:
We will be testing a bad
cartId
in Local Storage and then in Redux.For each, we will be testing adding, updating, and removing items from the cart.
We will also test the flows for both guest users and authenticated users.
Guest Users (Not Signed In)
Perform the following tests as a guest user. You may have to sign out first.
Test Case: Bad
cartId
in Local StoragePage Load
cartId
cookie to something boguscartId
and empty cartAdd Item
cartId
cookie to something bogusUpdate Item
cartId
cookie to something bogusRemove Item
cartId
cookie to something bogusTest Case: Bad
cartId
in ReduxPrerequisite
Add Item
CART/GET_DETAILS/RECEIVE
message in the console)and click the small
dispatch
button in the lower right to dispatch an action that will setcart.cartId
to "Bogus!"Update Item
and click the small
dispatch
button in the lower right to dispatch an action that will setcart.cartId
to "Bogus!"Update Cart
buttonRemove Item
and click the small
dispatch
button in the lower right to dispatch an action that will setcart.cartId
to "Bogus!"Remove Item
for any item in the cartAuthenticated Users (Signed In)
Perform the following tests as an authenticated user. You may have to sign in first.
You may also want to add an item to your cart so that you can be sure you're getting "your" cart back.
Test Case: Bad
cartId
in Local StorageThe
cartId
for authenticated users is aNumber
(ex: "2209"). For these steps either change it to another number or be sure to include escaped quotes if you change it to a string. Ex:{"value":"\"bogus\"" ...}
. If you change it to the literal string "bogus" you will get JSON parse errors in the console and will invalidate the tests.Page Load
cartId
cookie to something boguscartId
in local storage is updated back to the correct ID (yours).Add Item
cartId
cookie to something bogusUpdate Item
cartId
cookie to something bogusRemove Item
cartId
cookie to something bogusTest Case: Bad
cartId
in ReduxAdd Item
CART/GET_DETAILS/RECEIVE
message in the console)and click the small
dispatch
button in the lower right to dispatch an action that will setcart.cartId
to "Bogus!"Update Item
and click the small
dispatch
button in the lower right to dispatch an action that will setcart.cartId
to "Bogus!"Update Cart
buttonRemove Item
and click the small
dispatch
button in the lower right to dispatch an action that will setcart.cartId
to "Bogus!"Remove Item
for any item in the cartHow Have YOU Tested this?
yarn test
Screenshots / Screen Captures (if appropriate):
This is what I'm referring to when I say "the red error toast":
Proposed Labels for Change Type/Package
Checklist: