From 02663504277989edc04b4baeb0a7aa7201a81a8c Mon Sep 17 00:00:00 2001 From: zetlen Date: Fri, 28 Sep 2018 11:02:20 -0500 Subject: [PATCH] fix: reducer removes guestCartId when it fails chore: add test coverage too --- .../cart/__tests__/asyncActions.spec.js | 391 +++++++++++++++++- .../src/actions/cart/asyncActions.js | 33 +- .../src/reducers/__tests__/cart.spec.js | 91 ++++ packages/venia-concept/src/reducers/cart.js | 2 +- 4 files changed, 493 insertions(+), 24 deletions(-) create mode 100644 packages/venia-concept/src/reducers/__tests__/cart.spec.js 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 09dc204938..39b14d3b1c 100644 --- a/packages/venia-concept/src/actions/cart/__tests__/asyncActions.spec.js +++ b/packages/venia-concept/src/actions/cart/__tests__/asyncActions.spec.js @@ -2,7 +2,11 @@ import { RestApi } from '@magento/peregrine'; import { dispatch, getState } from 'src/store'; import checkoutActions from 'src/actions/checkout'; -import { mockGetItem, mockSetItem } from 'src/util/simplePersistence'; +import { + mockGetItem, + mockSetItem, + mockRemoveItem +} from 'src/util/simplePersistence'; import actions from '../actions'; import { addItemToCart, @@ -27,6 +31,10 @@ beforeAll(() => { afterEach(() => { dispatch.mockClear(); request.mockClear(); + getState.mockClear(); + mockGetItem.mockClear(); + mockSetItem.mockClear(); + mockRemoveItem.mockClear(); }); afterAll(() => { @@ -43,7 +51,7 @@ test('createGuestCart thunk returns undefined', async () => { expect(result).toBeUndefined(); }); -test('createGuestCart thunk does nothing if a guest cart exists', async () => { +test('createGuestCart thunk does nothing if a guest cart exists in state', async () => { await createGuestCart()(...thunkArgs); expect(dispatch).not.toHaveBeenCalled(); @@ -57,6 +65,7 @@ test('createGuestCart thunk uses the guest cart from storage', async () => { await createGuestCart()(...thunkArgs); + expect(mockGetItem).toHaveBeenCalledWith('guestCartId'); expect(dispatch).toHaveBeenNthCalledWith(1, checkoutActions.reset()); expect(dispatch).toHaveBeenNthCalledWith( 2, @@ -87,8 +96,9 @@ test('createGuestCart thunk dispatches actions on success', async () => { test('createGuestCart thunk dispatches actions on failure', async () => { const error = new Error('ERROR'); - request.mockResolvedValueOnce(error); + request.mockRejectedValueOnce(error); getState.mockImplementationOnce(() => ({ cart: {} })); + mockGetItem.mockImplementationOnce(() => {}); await createGuestCart()(...thunkArgs); @@ -131,21 +141,259 @@ test('addItemToCart thunk dispatches actions on success', async () => { expect(dispatch).toHaveBeenCalledTimes(4); }); -test('addItemToCart thunk dispatches actions on failure', async () => { - const payload = { item: 'ITEM', quantity: 1 }; - const error = new Error('ERROR'); +test('addItemToCart thunk skips image cache if no sku or image', async () => { + const noSku = { + quantity: 1, + item: { + media_gallery_entries: [ + { + position: 1, + url: 'http://example.com' + } + ] + } + }; + await addItemToCart(noSku)(...thunkArgs); + expect(mockGetItem).not.toHaveBeenCalled; + const noImages = { + quantity: 1, + item: { + sku: 'INVISIBLE' + } + }; + await addItemToCart(noImages)(...thunkArgs); + expect(mockGetItem).not.toHaveBeenCalled; + const emptyImages = { + quantity: 1, + item: { + sku: 'INVISIBLE', + media_gallery_entries: [] + } + }; + await addItemToCart(emptyImages)(...thunkArgs); + expect(mockGetItem).not.toHaveBeenCalled; +}); - request.mockRejectedValueOnce(error); - await addItemToCart(payload)(...thunkArgs); +test('addItemToCart stores product images in local cache for use in cart', async () => { + const itemWithImages = { + quantity: 1, + item: { + sku: 'HELLO', + media_gallery_entries: [ + { + position: 2, + url: 'http://example.com/second' + }, + { + position: 1, + url: 'http://example.com/first' + } + ] + } + }; + await addItemToCart(itemWithImages)(...thunkArgs); + expect(mockGetItem).toHaveBeenCalledWith('imagesBySku'); + expect(mockSetItem).toHaveBeenCalledWith( + 'imagesBySku', + expect.objectContaining({ + HELLO: { position: 1, url: 'http://example.com/first' } + }) + ); + + const itemWithUnpositionedImages = { + quantity: 1, + item: { + sku: 'GOODBYE', + media_gallery_entries: [ + { + url: 'http://example.com' + } + ] + } + }; + await addItemToCart(itemWithUnpositionedImages)(...thunkArgs); + expect(mockGetItem).toHaveBeenCalledTimes(2); + expect(mockSetItem).toHaveBeenCalledWith( + 'imagesBySku', + expect.objectContaining({ + GOODBYE: { url: 'http://example.com' } + }) + ); +}); + +test('addItemToCart reuses product images from cache', async () => { + const sameItem = { + sku: 'SAME_ITEM', + media_gallery_entries: [{ url: 'http://example.com/same/item' }] + }; + const fakeImageCache = {}; + mockGetItem.mockReturnValueOnce(fakeImageCache); + await addItemToCart({ quantity: 1, item: sameItem })(...thunkArgs); + mockGetItem.mockReturnValueOnce(fakeImageCache); + expect(mockSetItem).toHaveBeenCalledTimes(1); + await addItemToCart({ quantity: 4, item: sameItem })(...thunkArgs); + expect(mockSetItem).toHaveBeenCalledTimes(1); +}); +test('addItemToCart thunk dispatches special failure if guestCartId is not present', async () => { + const payload = { item: 'ITEM', quantity: 1 }; + const error = new Error('Missing required information: guestCartId'); + error.noGuestCartId = true; + getState.mockImplementationOnce(() => ({ cart: {} })); + await addItemToCart(payload)(...thunkArgs); + expect(mockRemoveItem).toHaveBeenCalledWith('guestCartId'); expect(dispatch).toHaveBeenNthCalledWith( 1, actions.addItem.request(payload) ); expect(dispatch).toHaveBeenNthCalledWith(2, actions.addItem.receive(error)); + // and now, the the createGuestCart thunk expect(dispatch).toHaveBeenNthCalledWith(3, expect.any(Function)); - expect(dispatch).toHaveBeenNthCalledWith(4, expect.any(Function)); - expect(dispatch).toHaveBeenCalledTimes(4); +}); + +test('addItemToCart tries to recreate a guest cart on 404 failure', async () => { + getState + .mockImplementationOnce(() => ({ + cart: { guestCartId: 'OLD_AND_BUSTED' } + })) + .mockImplementationOnce(() => ({ + cart: { guestCartId: 'CACHED_CART' } + })) + .mockImplementationOnce(() => ({ cart: {} })); + const payload = { item: 'ITEM', quantity: 1 }; + const error = new Error('ERROR'); + error.response = { + status: 404 + }; + // image cache + mockGetItem.mockResolvedValueOnce('CACHED_CART'); + + request.mockRejectedValueOnce(error); + + await addItemToCart(payload)(...thunkArgs); + + expect(dispatch.mock.calls).toMatchInlineSnapshot(` +Array [ + Array [ + Object { + "payload": Object { + "item": "ITEM", + "quantity": 1, + }, + "type": "CART/ADD_ITEM/REQUEST", + }, + ], + Array [ + Object { + "error": true, + "payload": [Error: ERROR], + "type": "CART/ADD_ITEM/RECEIVE", + }, + ], + Array [ + [Function], + ], + Array [ + Object { + "payload": Object { + "item": "ITEM", + "quantity": 1, + }, + "type": "CART/ADD_ITEM/REQUEST", + }, + ], + Array [ + Object { + "payload": Object { + "cartItem": undefined, + "item": "ITEM", + "quantity": 1, + }, + "type": "CART/ADD_ITEM/RECEIVE", + }, + ], + Array [ + [Function], + ], + Array [ + [Function], + ], +] +`); +}); + +test('addItemToCart opens drawer and gets cart details on success', async () => { + const payload = { item: 'ITEM', quantity: 1 }; + const fakeCart = { cart: { guestCartId: 'NEW_GUEST_CART_ID' } }; + const cartItem = 'CART_ITEM'; + + getState.mockReturnValueOnce(fakeCart).mockReturnValueOnce(fakeCart); + const fakeDispatch = fn => + typeof fn === 'function' && fn(dispatch, getState); + dispatch + .mockImplementationOnce(fakeDispatch) + .mockImplementationOnce(fakeDispatch) + .mockImplementationOnce(fakeDispatch) + .mockImplementationOnce(fakeDispatch) + .mockImplementationOnce(fakeDispatch); + + request.mockResolvedValueOnce(cartItem).mockResolvedValueOnce(cartItem); + await addItemToCart(payload)(...thunkArgs); + + expect(getState).toHaveBeenCalledTimes(2); + expect(dispatch).toHaveBeenCalledTimes(7); + expect(request).toHaveBeenCalledTimes(3); + + expect(dispatch.mock.calls).toMatchInlineSnapshot(` +Array [ + Array [ + Object { + "payload": Object { + "item": "ITEM", + "quantity": 1, + }, + "type": "CART/ADD_ITEM/REQUEST", + }, + ], + Array [ + Object { + "payload": Object { + "cartItem": "CART_ITEM", + "item": "ITEM", + "quantity": 1, + }, + "type": "CART/ADD_ITEM/RECEIVE", + }, + ], + Array [ + [Function], + ], + Array [ + Object { + "payload": "cart", + "type": "APP/TOGGLE_DRAWER", + }, + ], + Array [ + [Function], + ], + Array [ + Object { + "payload": "NEW_GUEST_CART_ID", + "type": "CART/GET_DETAILS/REQUEST", + }, + ], + Array [ + Object { + "payload": Object { + "details": "CART_ITEM", + "totals": undefined, + }, + "type": "CART/GET_DETAILS/RECEIVE", + }, + ], +] +`); }); test('getCartDetails() returns a thunk', () => { @@ -158,6 +406,109 @@ test('getCartDetails thunk returns undefined', async () => { expect(result).toBeUndefined(); }); +test('getCartDetails thunk creates a guest cart if no ID is found', async () => { + getState + // for the getCartDetails state check + .mockImplementationOnce(() => ({ cart: {} })) + // for the createGuestCart check + .mockImplementationOnce(() => ({ cart: {} })) + // for the subsequent getCartDetails re-check + .mockImplementationOnce(() => ({ + cart: { guestCartId: 'NEW_GUEST_CART_ID' } + })); + dispatch + // for not dispatching the sync notifier action + .mockImplementationOnce(() => {}) + // for actually dispatching the createGuestCart action + .mockImplementationOnce(fn => fn(...thunkArgs)); + mockGetItem.mockImplementationOnce(() => {}); + request + // for createGuestCart + .mockResolvedValueOnce('NEW_GUEST_CART_ID') + // for getCartDetails + .mockResolvedValueOnce({ + id: 1, + guestCartId: 'NEW_GUEST_CART_ID', + items: [] + }); + + await getCartDetails()(...thunkArgs); + + expect(getState).toHaveBeenCalledTimes(3); + expect(mockGetItem).toHaveBeenCalled(); + const createCallArgs = request.mock.calls[0]; + const retrieveCallArgs = request.mock.calls[1]; + expect(createCallArgs[0]).toBe('/rest/V1/guest-carts'); + expect(createCallArgs[1]).toHaveProperty('method', 'POST'); + expect(retrieveCallArgs[0]).toBe('/rest/V1/guest-carts/NEW_GUEST_CART_ID/'); +}); + +test('getCartDetails thunk deletes an old cart id and recreates a guest cart if cart ID is expired', async () => { + let tempStorage = {}; + mockSetItem.mockImplementation((key, value) => { + tempStorage[key] = value; + }); + mockGetItem.mockImplementation(key => { + return tempStorage[key]; + }); + getState + // for the getCartDetails state check + .mockImplementationOnce(() => ({ + cart: { guestCartId: 'EXPIRED_CART_ID' } + })) + // for the createGuestCart check + .mockImplementationOnce(() => ({ + cart: { guestCartId: tempStorage.guestCartId } + })) + // for the subsequent getCartDetails re-check + .mockImplementationOnce(() => ({ + cart: { guestCartId: 'BRAND_NEW_CART' } + })); + dispatch + // for not dispatching the sync notifier action + .mockImplementationOnce(() => {}) + // for not dispatching the create notifier action + .mockImplementationOnce(() => {}) + // for actually dispatching the createGuestCart action + .mockImplementationOnce(fn => fn(...thunkArgs)); + // mockGetItem.mockImplementationOnce(() => 'EXPIRED_CART_ID'); + request + // for for getting expired cart + .mockRejectedValueOnce({ response: { status: 404 } }) + // for for getting expired cart totals + .mockRejectedValueOnce({ response: { status: 404 } }) + // for createNewCart + .mockResolvedValueOnce('BRAND_NEW_CART') + // for getCartDetails + .mockResolvedValueOnce({ + id: 1, + guestCartId: 'BRAND_NEW_CART', + items: [] + }); + + await getCartDetails()(...thunkArgs); + + expect(getState).toHaveBeenCalledTimes(3); + expect(mockGetItem).toHaveBeenCalledWith('guestCartId'); + expect(mockRemoveItem).toHaveBeenCalledWith('guestCartId'); + expect(mockSetItem).toHaveBeenCalledWith('guestCartId', 'BRAND_NEW_CART'); + const [ + retrieveExpiredCallArgs, + retrieveExpiredTotalsArgs, + createCallArgs, + retrieveCallArgs + ] = request.mock.calls; + expect(retrieveExpiredCallArgs[0]).toBe( + '/rest/V1/guest-carts/EXPIRED_CART_ID/' + ); + expect(retrieveExpiredTotalsArgs[0]).toBe( + '/rest/V1/guest-carts/EXPIRED_CART_ID/totals' + ); + expect(createCallArgs[0]).toBe('/rest/V1/guest-carts'); + expect(createCallArgs[1]).toHaveProperty('method', 'POST'); + expect(retrieveCallArgs[0]).toBe('/rest/V1/guest-carts/BRAND_NEW_CART/'); +}); + test('getCartDetails thunk dispatches actions on success', async () => { request.mockResolvedValueOnce(1); request.mockResolvedValueOnce(2); @@ -194,8 +545,16 @@ test('getCartDetails thunk dispatches actions on failure', async () => { test('getCartDetails thunk merges cached item images into details', async () => { const cache = { SKU_1: 'IMAGE_1' }; - const items = [{ image: 'IMAGE_0', sku: 'SKU_0' }, { sku: 'SKU_1' }]; - const expected = [items[0], { ...items[1], image: cache.SKU_1 }]; + const items = [ + { image: 'IMAGE_0', sku: 'SKU_0' }, + { sku: 'SKU_1' }, + { sku: 'SKU_2' } + ]; + const expected = [ + items[0], + { ...items[1], image: cache.SKU_1 }, + { ...items[2], image: {} } + ]; mockGetItem.mockResolvedValueOnce(cache); request.mockResolvedValueOnce({ items }); @@ -242,3 +601,11 @@ test('toggleCart thunk opens the drawer and refreshes the cart', async () => { expect(dispatch).toHaveBeenNthCalledWith(2, expect.any(Function)); expect(dispatch).toHaveBeenCalledTimes(2); }); + +test('toggleCart thunk closes the drawer', async () => { + getState.mockReturnValueOnce({ app: { drawer: 'cart' }, cart: {} }); + await toggleCart()(...thunkArgs); + + expect(dispatch).toHaveBeenNthCalledWith(1, expect.any(Function)); + expect(dispatch).toHaveBeenCalledTimes(1); +}); diff --git a/packages/venia-concept/src/actions/cart/asyncActions.js b/packages/venia-concept/src/actions/cart/asyncActions.js index 86bab0ef94..98bc8f16fb 100644 --- a/packages/venia-concept/src/actions/cart/asyncActions.js +++ b/packages/venia-concept/src/actions/cart/asyncActions.js @@ -48,9 +48,10 @@ export const createGuestCart = () => export const addItemToCart = (payload = {}) => { const { item, quantity } = payload; - writeImageToCache(item); + const writingImageToCache = writeImageToCache(item); return async function thunk(dispatch, getState) { + await writingImageToCache; dispatch(actions.addItem.request(payload)); try { @@ -58,7 +59,11 @@ export const addItemToCart = (payload = {}) => { const { guestCartId } = cart; if (!guestCartId) { - throw new Error('Missing required information: guestCartId'); + const missingGuestCartError = new Error( + 'Missing required information: guestCartId' + ); + missingGuestCartError.noGuestCartId = true; + throw missingGuestCartError; } const cartItem = await request( @@ -78,13 +83,18 @@ export const addItemToCart = (payload = {}) => { dispatch(actions.addItem.receive({ cartItem, item, quantity })); } catch (error) { - const { response } = error; + const { response, noGuestCartId } = error; dispatch(actions.addItem.receive(error)); // check if the guest cart has expired - if (response && response.status === 404) { - // if so, create a new one + if (noGuestCartId || (response && response.status === 404)) { + // if so, then delete the cached ID... + // 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 clearGuestCartId(); + // then create a new one await dispatch(createGuestCart()); // then retry this operation return thunk(...arguments); @@ -143,7 +153,12 @@ export const getCartDetails = (payload = {}) => { // check if the guest cart has expired if (response && response.status === 404) { - // if so, create a new one + // if so, then delete the cached ID... + // 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 clearGuestCartId(); + // then create a new one await dispatch(createGuestCart()); // then retry this operation return thunk(...arguments); @@ -176,10 +191,6 @@ export const toggleCart = () => /* helpers */ async function fetchCartPart({ guestCartId, forceRefresh, subResource = '' }) { - if (!guestCartId) { - throw new Error('Missing required information: guestCartId'); - } - return request(`/rest/V1/guest-carts/${guestCartId}/${subResource}`, { cache: forceRefresh ? 'reload' : 'default' }); @@ -209,7 +220,7 @@ async function writeImageToCache(item = {}) { const { media_gallery_entries: media, sku } = item; if (sku) { - const image = media.find(m => m.position === 1) || media[0]; + const image = media && (media.find(m => m.position === 1) || media[0]); if (image) { const imageCache = await retrieveImageCache(); diff --git a/packages/venia-concept/src/reducers/__tests__/cart.spec.js b/packages/venia-concept/src/reducers/__tests__/cart.spec.js new file mode 100644 index 0000000000..a238e7a24e --- /dev/null +++ b/packages/venia-concept/src/reducers/__tests__/cart.spec.js @@ -0,0 +1,91 @@ +import cartReducers from 'src/reducers/cart'; +import actions from 'src/actions/cart'; +import checkoutActions from 'src/actions/checkout'; + +test('getGuestCart.receive: adds guestCartId to state', () => { + expect( + cartReducers( + { other: 'stuff' }, + { type: actions.getGuestCart.receive, payload: 'A_CART' } + ) + ).toEqual({ + other: 'stuff', + guestCartId: 'A_CART' + }); +}); + +test('getGuestCart.receive: restores initial state on error', () => { + expect( + cartReducers( + { guestCartId: 'AN_EXPIRED_CART', other: 'stuff' }, + { + type: actions.getGuestCart.receive, + payload: new Error('Failed to get a guest cart!'), + error: true + } + ) + ).toEqual({ + details: {}, + guestCartId: null, + totals: {} + }); +}); + +test('getGuestCart.receive: adds guestCartId to state', () => { + expect( + cartReducers( + { other: 'stuff' }, + { type: actions.getGuestCart.receive, payload: 'A_CART' } + ) + ).toEqual({ + other: 'stuff', + guestCartId: 'A_CART' + }); +}); + +test('getDetails.receive: merges payload with state', () => { + expect( + cartReducers( + { other: 'stuff', totals: { total: 100 } }, + { + type: actions.getDetails.receive, + payload: { + totals: { total: 200 }, + details: { items: ['woah'] } + } + } + ) + ).toEqual({ + other: 'stuff', + totals: { + total: 200 + }, + details: { + items: ['woah'] + } + }); +}); + +test('getDetails.receive: returns same state on error, letting the app slice handle error display', () => { + const state = { other: 'stuff', totals: { total: 100 } }; + expect( + cartReducers(state, { + type: actions.getDetails.receive, + payload: new Error('That did not work at all'), + error: true + }) + ).toEqual(state); +}); + +test('checkoutActions.order.accept: cart resets to initial state', () => { + expect( + cartReducers( + { guestCartId: 'SOME_CART', details: { items: ['done'] } }, + { type: checkoutActions.order.accept } + ) + ).toEqual({ + details: {}, + guestCartId: null, + totals: {} + }); +}); diff --git a/packages/venia-concept/src/reducers/cart.js b/packages/venia-concept/src/reducers/cart.js index d49bfc8c1c..18a0644277 100644 --- a/packages/venia-concept/src/reducers/cart.js +++ b/packages/venia-concept/src/reducers/cart.js @@ -14,7 +14,7 @@ const initialState = { const reducerMap = { [actions.getGuestCart.receive]: (state, { payload, error }) => { if (error) { - return state; + return initialState; } return {