From 7299a6b004f8c04d0c4155d1709307d38c943d54 Mon Sep 17 00:00:00 2001 From: Louise Davies Date: Thu, 21 Apr 2022 09:34:19 +0100 Subject: [PATCH 1/5] #1210 - use addToCart request with remove param to remove multiple items from the cart - also retry any mutations from TopCAT if we get 431 errors --- packages/datagateway-common/src/api/cart.tsx | 35 ++++++++++++++++--- .../src/downloadApiHooks.ts | 16 +++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/packages/datagateway-common/src/api/cart.tsx b/packages/datagateway-common/src/api/cart.tsx index 040d4555c..24674f34d 100644 --- a/packages/datagateway-common/src/api/cart.tsx +++ b/packages/datagateway-common/src/api/cart.tsx @@ -30,12 +30,16 @@ export const fetchDownloadCart = (config: { const addToCart = ( entityType: 'investigation' | 'dataset' | 'datafile', entityIds: number[], - config: { facilityName: string; downloadApiUrl: string } + config: { facilityName: string; downloadApiUrl: string }, + remove?: boolean ): Promise => { const { facilityName, downloadApiUrl } = config; const params = new URLSearchParams(); params.append('sessionId', readSciGatewayToken().sessionId || ''); params.append('items', `${entityType} ${entityIds.join(`, ${entityType} `)}`); + if (remove) { + params.append('remove', remove.toString()); + } return axios .post( @@ -112,6 +116,14 @@ export const useAddToCart = ( onSuccess: (data) => { queryClient.setQueryData('cart', data); }, + retry: (failureCount, error) => { + // if we get 431 we know this is an intermittent error so retry + if (error.code === '431' && failureCount < 3) { + return true; + } else { + return false; + } + }, onError: (error) => { handleICATError(error); }, @@ -132,14 +144,27 @@ export const useRemoveFromCart = ( return useMutation( (entityIds: number[]) => - removeFromCart(entityType, entityIds, { - facilityName, - downloadApiUrl, - }), + addToCart( + entityType, + entityIds, + { + facilityName, + downloadApiUrl, + }, + true + ), { onSuccess: (data) => { queryClient.setQueryData('cart', data); }, + retry: (failureCount, error) => { + // if we get 431 we know this is an intermittent error so retry + if (error.code === '431' && failureCount < 3) { + return true; + } else { + return false; + } + }, onError: (error) => { handleICATError(error); }, diff --git a/packages/datagateway-download/src/downloadApiHooks.ts b/packages/datagateway-download/src/downloadApiHooks.ts index e191516e6..6eb478b1a 100644 --- a/packages/datagateway-download/src/downloadApiHooks.ts +++ b/packages/datagateway-download/src/downloadApiHooks.ts @@ -58,6 +58,14 @@ export const useRemoveAllFromCart = (): UseMutationResult< onSuccess: (data) => { queryClient.setQueryData('cart', []); }, + retry: (failureCount, error) => { + // if we get 431 we know this is an intermittent error so retry + if (error.code === '431' && failureCount < 3) { + return true; + } else { + return false; + } + }, onError: (error) => { handleICATError(error); }, @@ -84,6 +92,14 @@ export const useRemoveEntityFromCart = (): UseMutationResult< onSuccess: (data) => { queryClient.setQueryData('cart', data); }, + retry: (failureCount, error) => { + // if we get 431 we know this is an intermittent error so retry + if (error.code === '431' && failureCount < 3) { + return true; + } else { + return false; + } + }, onError: (error) => { handleICATError(error); }, From a88a626317798f7f3672a67ea7553538166fac81 Mon Sep 17 00:00:00 2001 From: Louise Davies Date: Fri, 22 Apr 2022 10:28:17 +0100 Subject: [PATCH 2/5] #1210 - Add explicit undefined check for remove param --- packages/datagateway-common/src/api/cart.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/datagateway-common/src/api/cart.tsx b/packages/datagateway-common/src/api/cart.tsx index 24674f34d..416afa95b 100644 --- a/packages/datagateway-common/src/api/cart.tsx +++ b/packages/datagateway-common/src/api/cart.tsx @@ -37,7 +37,7 @@ const addToCart = ( const params = new URLSearchParams(); params.append('sessionId', readSciGatewayToken().sessionId || ''); params.append('items', `${entityType} ${entityIds.join(`, ${entityType} `)}`); - if (remove) { + if (typeof remove !== 'undefined') { params.append('remove', remove.toString()); } From 55bb893a33dbfa374c57caefc53638bd6a429e9a Mon Sep 17 00:00:00 2001 From: Louise Davies Date: Fri, 22 Apr 2022 10:38:50 +0100 Subject: [PATCH 3/5] #1210 - fix cart tests for new http method & add tests for retries --- .../datagateway-common/src/api/cart.test.tsx | 53 ++++++++++++------- .../src/downloadApiHooks.test.tsx | 46 +++++++++++----- 2 files changed, 67 insertions(+), 32 deletions(-) diff --git a/packages/datagateway-common/src/api/cart.test.tsx b/packages/datagateway-common/src/api/cart.test.tsx index a09eb0796..2178a1591 100644 --- a/packages/datagateway-common/src/api/cart.test.tsx +++ b/packages/datagateway-common/src/api/cart.test.tsx @@ -119,10 +119,15 @@ describe('Cart api functions', () => { expect(result.current.data).toEqual(mockData.cartItems); }); - it('sends axios request to add item to cart once mutate function is called and calls handleICATError on failure', async () => { - (axios.post as jest.Mock).mockRejectedValue({ - message: 'Test error message', - }); + it('sends axios request to add item to cart once mutate function is called and calls handleICATError on failure, with a retry on code 431', async () => { + (axios.post as jest.MockedFunction) + .mockRejectedValueOnce({ + code: '431', + message: 'Test 431 error message', + }) + .mockRejectedValue({ + message: 'Test error message', + }); const { result, waitFor } = renderHook(() => useAddToCart('dataset'), { wrapper: createReactQueryWrapper(), @@ -133,8 +138,10 @@ describe('Cart api functions', () => { result.current.mutate([1, 2]); - await waitFor(() => result.current.isError); + await waitFor(() => result.current.isError, { timeout: 2000 }); + expect(result.current.failureCount).toBe(2); + expect(handleICATError).toHaveBeenCalledTimes(1); expect(handleICATError).toHaveBeenCalledWith({ message: 'Test error message', }); @@ -143,7 +150,7 @@ describe('Cart api functions', () => { describe('useRemoveFromCart', () => { it('sends axios request to remove item from cart once mutate function is called and returns successful response', async () => { - (axios.delete as jest.Mock).mockResolvedValue({ + (axios.post as jest.Mock).mockResolvedValue({ data: mockData, }); @@ -161,22 +168,28 @@ describe('Cart api functions', () => { await waitFor(() => result.current.isSuccess); - expect(axios.delete).toHaveBeenCalledWith( + const params = new URLSearchParams(); + params.append('sessionId', ''); + params.append('items', 'dataset 1, dataset 2'); + params.append('remove', 'true'); + + expect(axios.post).toHaveBeenCalledWith( 'https://example.com/topcat/user/cart/TEST/cartItems', - { - params: { - sessionId: null, - items: 'dataset 1, dataset 2', - }, - } + + params ); expect(result.current.data).toEqual(mockData.cartItems); }); - it('sends axios request to remove item from cart once mutate function is called and calls handleICATError on failure', async () => { - (axios.delete as jest.Mock).mockRejectedValue({ - message: 'Test error message', - }); + it('sends axios request to remove item from cart once mutate function is called and calls handleICATError on failure, with a retry on code 431', async () => { + (axios.post as jest.MockedFunction) + .mockRejectedValueOnce({ + code: '431', + message: 'Test 431 error message', + }) + .mockRejectedValue({ + message: 'Test error message', + }); const { result, waitFor } = renderHook( () => useRemoveFromCart('dataset'), @@ -185,13 +198,15 @@ describe('Cart api functions', () => { } ); - expect(axios.delete).not.toHaveBeenCalled(); + expect(axios.post).not.toHaveBeenCalled(); expect(result.current.isIdle).toBe(true); result.current.mutate([1, 2]); - await waitFor(() => result.current.isError); + await waitFor(() => result.current.isError, { timeout: 2000 }); + expect(result.current.failureCount).toBe(2); + expect(handleICATError).toHaveBeenCalledTimes(1); expect(handleICATError).toHaveBeenCalledWith({ message: 'Test error message', }); diff --git a/packages/datagateway-download/src/downloadApiHooks.test.tsx b/packages/datagateway-download/src/downloadApiHooks.test.tsx index 5498ddc1e..8de57f1d8 100644 --- a/packages/datagateway-download/src/downloadApiHooks.test.tsx +++ b/packages/datagateway-download/src/downloadApiHooks.test.tsx @@ -181,12 +181,20 @@ describe('Download Cart API react-query hooks test', () => { ); }); - it('logs error upon unsuccessful response', async () => { - axios.delete = jest.fn().mockImplementation(() => - Promise.reject({ - message: 'Test error message', - }) - ); + it('logs error upon unsuccessful response, with a retry on code 431', async () => { + axios.delete = jest + .fn() + .mockImplementationOnce(() => + Promise.reject({ + code: '431', + message: 'Test 431 error message', + }) + ) + .mockImplementation(() => + Promise.reject({ + message: 'Test error message', + }) + ); const { result, waitFor } = renderHook(() => useRemoveAllFromCart(), { wrapper: createReactQueryWrapper(), @@ -194,7 +202,7 @@ describe('Download Cart API react-query hooks test', () => { result.current.mutate(); - await waitFor(() => result.current.isError); + await waitFor(() => result.current.isError, { timeout: 2000 }); expect( axios.delete @@ -202,6 +210,8 @@ describe('Download Cart API react-query hooks test', () => { `${mockedSettings.downloadApiUrl}/user/cart/${mockedSettings.facilityName}/cartItems`, { params: { sessionId: null, items: '*' } } ); + expect(result.current.failureCount).toBe(2); + expect(handleICATError).toHaveBeenCalledTimes(1); expect(handleICATError).toHaveBeenCalledWith({ message: 'Test error message', }); @@ -242,11 +252,19 @@ describe('Download Cart API react-query hooks test', () => { }); it('logs error upon unsuccessful response', async () => { - axios.delete = jest.fn().mockImplementation(() => - Promise.reject({ - message: 'Test error message', - }) - ); + axios.delete = jest + .fn() + .mockImplementationOnce(() => + Promise.reject({ + code: '431', + message: 'Test 431 error message', + }) + ) + .mockImplementation(() => + Promise.reject({ + message: 'Test error message', + }) + ); const { result, waitFor } = renderHook(() => useRemoveEntityFromCart(), { wrapper: createReactQueryWrapper(), @@ -254,7 +272,7 @@ describe('Download Cart API react-query hooks test', () => { result.current.mutate({ entityId: 1, entityType: 'investigation' }); - await waitFor(() => result.current.isError); + await waitFor(() => result.current.isError, { timeout: 2000 }); expect( axios.delete @@ -262,6 +280,8 @@ describe('Download Cart API react-query hooks test', () => { `${mockedSettings.downloadApiUrl}/user/cart/${mockedSettings.facilityName}/cartItems`, { params: { sessionId: null, items: 'investigation 1' } } ); + expect(result.current.failureCount).toBe(2); + expect(handleICATError).toHaveBeenCalledTimes(1); expect(handleICATError).toHaveBeenCalledWith({ message: 'Test error message', }); From 6cad81d3179e12a312e61823c01fe29bad816e26 Mon Sep 17 00:00:00 2001 From: Louise Davies Date: Wed, 27 Apr 2022 13:25:25 +0100 Subject: [PATCH 4/5] #1210 - move removeFromCart to dg-download - also rename addToCart to addOrRemoveFromCart --- packages/datagateway-common/src/api/cart.tsx | 26 +++---------------- .../datagateway-download/src/downloadApi.ts | 22 ++++++++++++++++ .../src/downloadApiHooks.ts | 2 +- 3 files changed, 26 insertions(+), 24 deletions(-) diff --git a/packages/datagateway-common/src/api/cart.tsx b/packages/datagateway-common/src/api/cart.tsx index 416afa95b..56c7521c3 100644 --- a/packages/datagateway-common/src/api/cart.tsx +++ b/packages/datagateway-common/src/api/cart.tsx @@ -27,7 +27,7 @@ export const fetchDownloadCart = (config: { .then((response) => response.data.cartItems); }; -const addToCart = ( +const addOrRemoveFromCart = ( entityType: 'investigation' | 'dataset' | 'datafile', entityIds: number[], config: { facilityName: string; downloadApiUrl: string }, @@ -49,26 +49,6 @@ const addToCart = ( .then((response) => response.data.cartItems); }; -export const removeFromCart = ( - entityType: 'investigation' | 'dataset' | 'datafile', - entityIds: number[], - config: { facilityName: string; downloadApiUrl: string } -): Promise => { - const { facilityName, downloadApiUrl } = config; - - return axios - .delete( - `${downloadApiUrl}/user/cart/${facilityName}/cartItems`, - { - params: { - sessionId: readSciGatewayToken().sessionId, - items: `${entityType} ${entityIds.join(`, ${entityType} `)}`, - }, - } - ) - .then((response) => response.data.cartItems); -}; - export const useCart = (): UseQueryResult => { const downloadApiUrl = useSelector( (state: StateType) => state.dgcommon.urls.downloadApiUrl @@ -108,7 +88,7 @@ export const useAddToCart = ( return useMutation( (entityIds: number[]) => - addToCart(entityType, entityIds, { + addOrRemoveFromCart(entityType, entityIds, { facilityName, downloadApiUrl, }), @@ -144,7 +124,7 @@ export const useRemoveFromCart = ( return useMutation( (entityIds: number[]) => - addToCart( + addOrRemoveFromCart( entityType, entityIds, { diff --git a/packages/datagateway-download/src/downloadApi.ts b/packages/datagateway-download/src/downloadApi.ts index 2c4d7107f..d089388aa 100644 --- a/packages/datagateway-download/src/downloadApi.ts +++ b/packages/datagateway-download/src/downloadApi.ts @@ -6,6 +6,8 @@ import { Download, readSciGatewayToken, handleICATError, + DownloadCart, + DownloadCartItem, } from 'datagateway-common'; export const removeAllDownloadCartItems: (settings: { @@ -30,6 +32,26 @@ export const removeAllDownloadCartItems: (settings: { }); }; +export const removeFromCart = ( + entityType: 'investigation' | 'dataset' | 'datafile', + entityIds: number[], + config: { facilityName: string; downloadApiUrl: string } +): Promise => { + const { facilityName, downloadApiUrl } = config; + + return axios + .delete( + `${downloadApiUrl}/user/cart/${facilityName}/cartItems`, + { + params: { + sessionId: readSciGatewayToken().sessionId, + items: `${entityType} ${entityIds.join(`, ${entityType} `)}`, + }, + } + ) + .then((response) => response.data.cartItems); +}; + export const getIsTwoLevel: (settings: { idsUrl: string; }) => Promise = (settings: { idsUrl: string }) => { diff --git a/packages/datagateway-download/src/downloadApiHooks.ts b/packages/datagateway-download/src/downloadApiHooks.ts index 6eb478b1a..a71dfb3af 100644 --- a/packages/datagateway-download/src/downloadApiHooks.ts +++ b/packages/datagateway-download/src/downloadApiHooks.ts @@ -4,7 +4,6 @@ import { DownloadCartItem, handleICATError, fetchDownloadCart, - removeFromCart, } from 'datagateway-common'; import { DownloadSettingsContext } from './ConfigProvider'; import { @@ -19,6 +18,7 @@ import { import pLimit from 'p-limit'; import { removeAllDownloadCartItems, + removeFromCart, getSize, getDatafileCount, getIsTwoLevel, From bf46afafac13f8ab81b313e25d575b979b73399e Mon Sep 17 00:00:00 2001 From: Sam Glendenning Date: Tue, 3 May 2022 10:16:40 +0100 Subject: [PATCH 5/5] Fixing downloadCartTable unit test #1210 --- .../downloadCart/downloadCartTable.component.test.tsx | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/datagateway-download/src/downloadCart/downloadCartTable.component.test.tsx b/packages/datagateway-download/src/downloadCart/downloadCartTable.component.test.tsx index ef0a7d016..6edd387be 100644 --- a/packages/datagateway-download/src/downloadCart/downloadCartTable.component.test.tsx +++ b/packages/datagateway-download/src/downloadCart/downloadCartTable.component.test.tsx @@ -1,11 +1,7 @@ import React from 'react'; import { createMount } from '@material-ui/core/test-utils'; import DownloadCartTable from './downloadCartTable.component'; -import { - DownloadCartItem, - fetchDownloadCart, - removeFromCart, -} from 'datagateway-common'; +import { DownloadCartItem, fetchDownloadCart } from 'datagateway-common'; import { flushPromises } from '../setupTests'; import { act } from 'react-dom/test-utils'; import { DownloadSettingsContext } from '../ConfigProvider'; @@ -17,6 +13,7 @@ import { getDatafileCount, getSize, removeAllDownloadCartItems, + removeFromCart, } from '../downloadApi'; jest.mock('datagateway-common', () => { @@ -26,7 +23,6 @@ jest.mock('datagateway-common', () => { __esModule: true, ...originalModule, fetchDownloadCart: jest.fn(), - removeFromCart: jest.fn(), }; }); @@ -39,6 +35,7 @@ jest.mock('../downloadApi', () => { getSize: jest.fn(), getDatafileCount: jest.fn(), getIsTwoLevel: jest.fn().mockResolvedValue(true), + removeFromCart: jest.fn(), }; });