diff --git a/packages/venia-concept/src/actions/cart/__tests__/asyncActions.spec.js b/packages/venia-concept/src/actions/cart/__tests__/asyncActions.spec.js index bffcae5f59..777b29e2d3 100644 --- a/packages/venia-concept/src/actions/cart/__tests__/asyncActions.spec.js +++ b/packages/venia-concept/src/actions/cart/__tests__/asyncActions.spec.js @@ -79,15 +79,40 @@ describe('createCart', () => { await createCart()(...thunkArgs); expect(mockGetItem).toHaveBeenCalledWith('cartId'); + expect(dispatch).toHaveBeenCalledTimes(3); expect(dispatch).toHaveBeenNthCalledWith(1, checkoutActions.reset()); + expect(dispatch).toHaveBeenNthCalledWith(2, actions.getCart.request()); expect(dispatch).toHaveBeenNthCalledWith( - 2, + 3, actions.getCart.receive(storedCartId) ); - expect(dispatch).toHaveBeenCalledTimes(2); + expect(request).not.toHaveBeenCalled(); }); + test('its thunk does not use the cart id from storage if the user is logged in', async () => { + getState.mockImplementationOnce(() => ({ + cart: {}, + user: { isSignedIn: true } + })); + const storedCartId = 'STORED_CART_ID'; + mockGetItem.mockImplementationOnce(() => storedCartId); + const responseMock = 1234; + request.mockResolvedValueOnce(responseMock); + + await createCart()(...thunkArgs); + + expect(dispatch).toHaveBeenCalledTimes(3); + expect(dispatch).toHaveBeenNthCalledWith(1, checkoutActions.reset()); + expect(dispatch).toHaveBeenNthCalledWith(2, actions.getCart.request()); + expect(dispatch).toHaveBeenNthCalledWith( + 3, + actions.getCart.receive(responseMock) + ); + + expect(request).toHaveBeenCalled(); + }); + test('its thunk dispatches actions on success', async () => { getState.mockImplementationOnce(() => ({ cart: {}, @@ -259,6 +284,12 @@ describe('addItemToCart', () => { await addItemToCart(payload)(...thunkArgs); + expect(dispatch).toHaveBeenCalledTimes(8); + + /* + * Initial attempt will fail. + */ + expect(dispatch).toHaveBeenNthCalledWith( 1, actions.addItem.request(payload) @@ -267,12 +298,32 @@ describe('addItemToCart', () => { 2, actions.addItem.receive(error) ); - // createCart + // removeCart expect(dispatch).toHaveBeenNthCalledWith(3, expect.any(Function)); + // createCart + expect(dispatch).toHaveBeenNthCalledWith(4, expect.any(Function)); - // And then the thunk is called again. + /* + * And then the thunk is called again. + */ - expect(request).toHaveBeenCalledTimes(2); + expect(dispatch).toHaveBeenNthCalledWith( + 5, + actions.addItem.request(payload) + ); + // getCartDetails + expect(dispatch).toHaveBeenNthCalledWith(6, expect.any(Function)); + // toggleDrawer + expect(dispatch).toHaveBeenNthCalledWith(7, expect.any(Function)); + // addItem.receive + expect(dispatch).toHaveBeenNthCalledWith( + 8, + actions.addItem.receive({ + cartItem: undefined, + item: 'ITEM', + quantity: 1 + }) + ); }); test('its thunk uses the appropriate endpoint when user is signed in', async () => { @@ -328,7 +379,10 @@ describe('removeItemFromCart', () => { }); test('its thunk dispatches special failure if cartId is not present', async () => { - getState.mockImplementationOnce(() => ({ cart: {} })); + getState.mockImplementationOnce(() => ({ + cart: {}, + user: { isSignedIn: false } + })); const error = new Error('Missing required information: cartId'); error.noCartId = true; @@ -358,6 +412,7 @@ describe('removeItemFromCart', () => { await removeItemFromCart(payload)(...thunkArgs); + expect(dispatch).toHaveBeenCalledTimes(4); expect(dispatch).toHaveBeenNthCalledWith( 1, actions.removeItem.request(payload) @@ -368,10 +423,54 @@ describe('removeItemFromCart', () => { ); // createCart expect(dispatch).toHaveBeenNthCalledWith(3, expect.any(Function)); + // getCartDetails + expect(dispatch).toHaveBeenNthCalledWith(4, expect.any(Function)); + }); - // And then the thunk is called again. + test('its thunk retries the operation on 404 error when the user is signed in', async () => { + getState.mockImplementationOnce(() => ({ + app: { drawer: null }, + cart: { cartId: 'CART_ID' }, + user: { isSignedIn: true } + })); + const error = new Error('ERROR'); + error.response = { + status: 404 + }; + request.mockRejectedValueOnce(error); - expect(request).toHaveBeenCalledTimes(2); + await removeItemFromCart(payload)(...thunkArgs); + + expect(dispatch).toHaveBeenCalledTimes(6); + expect(dispatch).toHaveBeenNthCalledWith( + 1, + actions.removeItem.request(payload) + ); + expect(dispatch).toHaveBeenNthCalledWith( + 2, + actions.removeItem.receive(error) + ); + // createCart + expect(dispatch).toHaveBeenNthCalledWith(3, expect.any(Function)); + + /* + * The operation is now retried. + */ + expect(dispatch).toHaveBeenNthCalledWith( + 4, + actions.removeItem.request(payload) + ); + expect(dispatch).toHaveBeenNthCalledWith( + 5, + actions.removeItem.receive({ + cartItem: undefined, + item: payload.item, + cartItemCount: 0 + }) + ); + + // getCartDetails + expect(dispatch).toHaveBeenNthCalledWith(6, expect.any(Function)); }); test('its thunk clears the cartId when removing the last item in the cart', async () => { @@ -481,15 +580,18 @@ describe('updateItemInCart', () => { expect(dispatch).toHaveBeenNthCalledWith(3, expect.any(Function)); }); - test('its thunk tries to recreate a cart on 404 failure', async () => { + test('its thunk tries to recreate a cart and add the updated item to it on 404 failure', async () => { const error = new Error('ERROR'); error.response = { status: 404 }; request.mockRejectedValueOnce(error); + // image cache + //mockGetItem.mockResolvedValueOnce('CACHED_CART'); await updateItemInCart(payload, targetItemId)(...thunkArgs); + expect(dispatch).toHaveBeenCalledTimes(7); expect(dispatch).toHaveBeenNthCalledWith( 1, actions.updateItem.request(payload) @@ -498,12 +600,68 @@ describe('updateItemInCart', () => { 2, actions.updateItem.receive(error) ); + // removeCart + expect(dispatch).toHaveBeenNthCalledWith(3, expect.any(Function)); // createCart + expect(dispatch).toHaveBeenNthCalledWith(4, expect.any(Function)); + // addItemToCart + expect(dispatch).toHaveBeenNthCalledWith(5, expect.any(Function)); + // getCartDetails + expect(dispatch).toHaveBeenNthCalledWith(6, expect.any(Function)); + // closeOptionsDrawer + expect(dispatch).toHaveBeenNthCalledWith(7, expect.any(Function)); + }); + + test('its thunk retries on 404 failure if the user is signed in', async () => { + getState.mockImplementationOnce(() => ({ + app: { drawer: null }, + cart: { cartId: 'CART_ID' }, + user: { isSignedIn: true } + })); + const error = new Error('ERROR'); + error.response = { + status: 404 + }; + request.mockRejectedValueOnce(error); + // image cache + //mockGetItem.mockResolvedValueOnce('CACHED_CART'); + + await updateItemInCart(payload, targetItemId)(...thunkArgs); + + expect(dispatch).toHaveBeenCalledTimes(8); + expect(dispatch).toHaveBeenNthCalledWith( + 1, + actions.updateItem.request(payload) + ); + expect(dispatch).toHaveBeenNthCalledWith( + 2, + actions.updateItem.receive(error) + ); + // removeCart expect(dispatch).toHaveBeenNthCalledWith(3, expect.any(Function)); + // createCart + expect(dispatch).toHaveBeenNthCalledWith(4, expect.any(Function)); - // And then the thunk is called again. + /* + * The operation is now retried. + */ + expect(dispatch).toHaveBeenNthCalledWith( + 5, + actions.updateItem.request(payload) + ); + expect(dispatch).toHaveBeenNthCalledWith( + 6, + actions.updateItem.receive({ + cartItem: undefined, + item: payload.item, + quantity: payload.quantity + }) + ); - expect(request).toHaveBeenCalledTimes(2); + // getCartDetails + expect(dispatch).toHaveBeenNthCalledWith(7, expect.any(Function)); + // closeOptionsDrawer + expect(dispatch).toHaveBeenNthCalledWith(8, expect.any(Function)); }); test('its thunk uses the proper endpoint when the user is signed in', async () => { diff --git a/packages/venia-concept/src/actions/cart/asyncActions.js b/packages/venia-concept/src/actions/cart/asyncActions.js index ecad990dc2..b0dc5cf7c3 100644 --- a/packages/venia-concept/src/actions/cart/asyncActions.js +++ b/packages/venia-concept/src/actions/cart/asyncActions.js @@ -21,16 +21,16 @@ export const createCart = () => // in case the user has already completed an order this session dispatch(checkoutActions.reset()); + // Request a new cart. + dispatch(actions.getCart.request()); + // if a cart exists in storage, act like we just received it const cartId = await retrieveCartId(); - if (cartId) { + if (cartId && !user.isSignedIn) { dispatch(actions.getCart.receive(cartId)); return; } - // otherwise, request a new cart - dispatch(actions.getCart.request()); - try { const guestCartEndpoint = '/rest/V1/guest-carts'; const signedInCartEndpoint = '/rest/V1/carts/mine'; @@ -114,13 +114,13 @@ export const addItemToCart = (payload = {}) => { dispatch(actions.addItem.receive(error)); - // check if the guest cart has expired + // check if the cart has expired if (noCartId || (response && response.status === 404)) { - // if so, then delete the cached ID... - // in contrast to the save, make sure storage deletion is + // Delete the cached ID from local storage and Redux. + // In contrast to the save, make sure storage deletion is // complete before dispatching the error--you don't want an // upstream action to try and reuse the known-bad ID. - await clearCartId(); + await dispatch(removeCart()); // then create a new one await dispatch(createCart()); // then retry this operation @@ -138,8 +138,9 @@ export const updateItemInCart = (payload = {}, targetItemId) => { await writingImageToCache; dispatch(actions.updateItem.request(payload)); + const { cart, user } = getState(); + try { - const { cart, user } = getState(); const { cartId } = cart; if (!cartId) { @@ -176,25 +177,28 @@ export const updateItemInCart = (payload = {}, targetItemId) => { dispatch(actions.updateItem.receive(error)); - // check if the guest cart has expired + // check if the cart has expired if (noCartId || (response && response.status === 404)) { - // if so, then delete the cached ID... - // in contrast to the save, make sure storage deletion is + // Delete the cached ID from local storage and Redux. + // In contrast to the save, make sure storage deletion is // complete before dispatching the error--you don't want an // upstream action to try and reuse the known-bad ID. - await clearCartId(); + await dispatch(removeCart()); // then create a new one await dispatch(createCart()); - // then retry this operation - return thunk(...arguments); + + if (user.isSignedIn) { + // The user is signed in and we just received their cart. + // Retry this operation. + return thunk(...arguments); + } else { + // The user is a guest and just received a brand new (empty) cart. + // Add the updated item to that cart. + await dispatch(addItemToCart(payload)); + } } } - // await Promise.all([ - // dispatch(getCartDetails({ forceRefresh: true })), - // dispatch(toggleDrawer('cart')) - // ]); - await dispatch(getCartDetails({ forceRefresh: true })); // Close the options drawer only after the cart is finished updating. @@ -208,8 +212,9 @@ export const removeItemFromCart = payload => { return async function thunk(dispatch, getState) { dispatch(actions.removeItem.request(payload)); + const { cart, user } = getState(); + try { - const { cart, user } = getState(); const { cartId } = cart; if (!cartId) { @@ -255,25 +260,30 @@ export const removeItemFromCart = payload => { dispatch(actions.removeItem.receive(error)); - // check if the guest cart has expired + // check if the cart has expired if (noCartId || (response && response.status === 404)) { - // 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. + // In contrast to the save, make sure storage deletion is // complete before dispatching the error--you don't want an // upstream action to try and reuse the known-bad ID. await clearCartId(); // then create a new one await dispatch(createCart()); - // then retry this operation - return thunk(...arguments); + + if (user.isSignedIn) { + // The user is signed in and we just received their cart. + // Retry this operation. + return thunk(...arguments); + } + + // Else the user is a guest and just received a brand new (empty) cart. + // We don't retry because we'd be attempting to remove an item + // from an empty cart. } } - await dispatch( - getCartDetails({ - forceRefresh: true - }) - ); + await dispatch(getCartDetails({ forceRefresh: true })); }; }; @@ -360,10 +370,11 @@ export const getCartDetails = (payload = {}) => { dispatch(actions.getDetails.receive(error)); - // check if the guest cart has expired + // check if the cart has expired if (response && response.status === 404) { - // 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. + // In contrast to the save, make sure storage deletion is // complete before dispatching the error--you don't want an // upstream action to try and reuse the known-bad ID. await clearCartId(); diff --git a/packages/venia-concept/src/components/ErrorView/errorView.js b/packages/venia-concept/src/components/ErrorView/errorView.js index de7ca41f88..b75a213735 100644 --- a/packages/venia-concept/src/components/ErrorView/errorView.js +++ b/packages/venia-concept/src/components/ErrorView/errorView.js @@ -4,8 +4,8 @@ import { loadingIndicator } from 'src/components/LoadingIndicator'; const messages = new Map() .set('loading', loadingIndicator) - .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.'); class ErrorView extends Component { render() { diff --git a/packages/venia-concept/src/reducers/__tests__/cart.spec.js b/packages/venia-concept/src/reducers/__tests__/cart.spec.js index 048ed27c81..397700faea 100644 --- a/packages/venia-concept/src/reducers/__tests__/cart.spec.js +++ b/packages/venia-concept/src/reducers/__tests__/cart.spec.js @@ -184,7 +184,10 @@ describe('removeItem.receive', () => { const result = reducer(state, action); - expect(result).toEqual(initialState); + expect(result).toEqual({ + ...initialState, + removeItemError: action.payload + }); }); }); diff --git a/packages/venia-concept/src/reducers/cart.js b/packages/venia-concept/src/reducers/cart.js index f191697134..3c6e30beff 100644 --- a/packages/venia-concept/src/reducers/cart.js +++ b/packages/venia-concept/src/reducers/cart.js @@ -6,15 +6,19 @@ import checkoutActions from 'src/actions/checkout'; export const name = 'cart'; export const initialState = { + addItemError: null, cartId: null, details: {}, + detailsError: null, isLoading: false, isOptionsDrawerOpen: false, isUpdatingItem: false, isAddingItem: false, paymentMethods: [], + removeItemError: null, shippingMethods: [], - totals: {} + totals: {}, + updateItemError: null }; const reducerMap = { @@ -39,6 +43,7 @@ const reducerMap = { if (error) { return { ...state, + detailsError: payload, cartId: null, isLoading: false }; @@ -56,7 +61,15 @@ const reducerMap = { isAddingItem: true }; }, - [actions.addItem.receive]: state => { + [actions.addItem.receive]: (state, { payload, error }) => { + if (error) { + return { + ...state, + addItemError: payload, + isAddingItem: false + }; + } + return { ...state, isAddingItem: false @@ -72,7 +85,15 @@ const reducerMap = { isUpdatingItem: true }; }, - [actions.updateItem.receive]: state => { + [actions.updateItem.receive]: (state, { payload, error }) => { + if (error) { + return { + ...state, + isUpdatingItem: false, + updateItemError: payload + }; + } + // We don't actually have to update any items here // because we force a refresh from the server. return { @@ -82,7 +103,10 @@ const reducerMap = { }, [actions.removeItem.receive]: (state, { payload, error }) => { if (error) { - return initialState; + return { + ...initialState, + removeItemError: payload + }; } // If we are emptying the cart, perform a reset to prevent // a bug where the next item added to cart would have a price of 0