From 070de12ed7b2d7aded4d4df612182a2fd9ed2c02 Mon Sep 17 00:00:00 2001 From: JulienM Date: Thu, 4 Jun 2020 18:30:30 +0200 Subject: [PATCH 01/21] Fix pagination on bulk delete --- packages/ra-core/src/dataProvider/useGetList.ts | 8 +++++++- .../ra-ui-materialui/src/list/Pagination.tsx | 16 ++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/ra-core/src/dataProvider/useGetList.ts b/packages/ra-core/src/dataProvider/useGetList.ts index affa51937a7..ae5b30ceced 100644 --- a/packages/ra-core/src/dataProvider/useGetList.ts +++ b/packages/ra-core/src/dataProvider/useGetList.ts @@ -71,11 +71,17 @@ const useGetList = ( { type: 'getList', resource, payload: { pagination, sort, filter } }, options, // data selector (may return []) + // We don't want the list to reset to first page after a bulk delete if the current page still exists + // But CrudDeleteMany reset all "cachedRequests" for this resource. + // It caused ids to be empty, and useListController to automatically reset to page 1 + // Now, it "cachedRequests" is undefined, we try default ids before setting an empty ids array (state: ReduxState): Identifier[] => get( state.admin.resources, [resource, 'list', 'cachedRequests', requestSignature, 'ids'], - [] + state.admin.resources[resource] + ? state.admin.resources[resource].list.ids + : [] ), // total selector (may return undefined) (state: ReduxState): number => diff --git a/packages/ra-ui-materialui/src/list/Pagination.tsx b/packages/ra-ui-materialui/src/list/Pagination.tsx index 7b12a799bb1..bc699c6f561 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.tsx +++ b/packages/ra-ui-materialui/src/list/Pagination.tsx @@ -30,18 +30,25 @@ const Pagination: FC = props => { setPage, setPerPage, } = useListContext(props); + + const getNbPages = useCallback(() => Math.ceil(total / perPage) || 1, [ + perPage, + total, + ]); + useEffect(() => { if (page < 1 || isNaN(page)) { setPage(1); + } else if (page > getNbPages()) { + setPage(getNbPages()); } - }, [page, setPage]); + }, [page, setPage, getNbPages, total, perPage]); + const translate = useTranslate(); const isSmall = useMediaQuery((theme: Theme) => theme.breakpoints.down('sm') ); - const getNbPages = () => Math.ceil(total / perPage) || 1; - /** * Warning: material-ui's page is 0-based */ @@ -77,7 +84,8 @@ const Pagination: FC = props => { [translate] ); - if (total === 0) { + // Avoid rendering TablePagination if "page" value is invalid + if (total === 0 || page > getNbPages()) { return loading ? : limit; } From 8fcdfe887df423d46c44f12e1a6585351157e27f Mon Sep 17 00:00:00 2001 From: JulienM Date: Thu, 4 Jun 2020 18:43:06 +0200 Subject: [PATCH 02/21] fix tests --- packages/ra-core/src/dataProvider/useGetList.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/ra-core/src/dataProvider/useGetList.ts b/packages/ra-core/src/dataProvider/useGetList.ts index ae5b30ceced..0b65d863776 100644 --- a/packages/ra-core/src/dataProvider/useGetList.ts +++ b/packages/ra-core/src/dataProvider/useGetList.ts @@ -79,7 +79,8 @@ const useGetList = ( get( state.admin.resources, [resource, 'list', 'cachedRequests', requestSignature, 'ids'], - state.admin.resources[resource] + state.admin.resources[resource] && + state.admin.resources[resource].list ? state.admin.resources[resource].list.ids : [] ), From 3d2b2efb16eb3d7268ec6bfe8bc9ce597329e4dc Mon Sep 17 00:00:00 2001 From: JulienM Date: Thu, 4 Jun 2020 19:30:11 +0200 Subject: [PATCH 03/21] fix unit tests --- .../src/controller/useListController.spec.tsx | 3 ++- .../ra-ui-materialui/src/list/Pagination.spec.js | 13 ++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/ra-core/src/controller/useListController.spec.tsx b/packages/ra-core/src/controller/useListController.spec.tsx index be426066d15..8a4f2b28560 100644 --- a/packages/ra-core/src/controller/useListController.spec.tsx +++ b/packages/ra-core/src/controller/useListController.spec.tsx @@ -148,7 +148,7 @@ describe('useListController', () => { ).toEqual({ q: true }); }); - it('should update data if permanent filters change', () => { + it.only('should update data if permanent filters change', () => { const children = jest.fn().mockReturnValue(children); const props = { ...defaultProps, @@ -166,6 +166,7 @@ describe('useListController', () => { list: { params: {}, cachedRequests: {}, + ids: [], }, }, }, diff --git a/packages/ra-ui-materialui/src/list/Pagination.spec.js b/packages/ra-ui-materialui/src/list/Pagination.spec.js index fee227d97a3..177458eb0b3 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.spec.js +++ b/packages/ra-ui-materialui/src/list/Pagination.spec.js @@ -1,6 +1,6 @@ import * as React from 'react'; import expect from 'expect'; -import { render, cleanup } from '@testing-library/react'; +import { render, cleanup, wait } from '@testing-library/react'; import { ThemeProvider } from '@material-ui/styles'; import { createMuiTheme } from '@material-ui/core/styles'; import { ListContext } from 'ra-core'; @@ -43,19 +43,22 @@ describe('', () => { expect(queryByText('ra.navigation.no_results')).toBeNull(); }); - it('should not display a pagination limit on an out of bounds page', () => { + it('should display a pagination limit on an out of bounds page', async () => { jest.spyOn(console, 'error').mockImplementationOnce(() => {}); + const setPage = jest.fn().mockReturnValue(null); const { queryByText } = render( ); - // mui TablePagination displays a warning in that case, and that's normal - expect(queryByText('ra.navigation.no_results')).toBeNull(); + // mui TablePagination displays no more a warning in that case + // Then useEffect fallback on a valid page + expect(queryByText('ra.navigation.no_results')).not.toBeNull(); + await wait(expect(setPage).toBeCalledWith(1), 1000); }); }); From e70acd685ead7bfb793eda56bf44f8b2eebaf359 Mon Sep 17 00:00:00 2001 From: JulienM Date: Thu, 18 Jun 2020 17:01:24 +0200 Subject: [PATCH 04/21] little fixes on review --- packages/ra-core/src/controller/useListController.spec.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ra-core/src/controller/useListController.spec.tsx b/packages/ra-core/src/controller/useListController.spec.tsx index 8a4f2b28560..305ecf83061 100644 --- a/packages/ra-core/src/controller/useListController.spec.tsx +++ b/packages/ra-core/src/controller/useListController.spec.tsx @@ -148,7 +148,7 @@ describe('useListController', () => { ).toEqual({ q: true }); }); - it.only('should update data if permanent filters change', () => { + it('should update data if permanent filters change', () => { const children = jest.fn().mockReturnValue(children); const props = { ...defaultProps, From 809f6698106c31ee18b89949612dee76951fb9b7 Mon Sep 17 00:00:00 2001 From: JulienM Date: Thu, 18 Jun 2020 17:30:19 +0200 Subject: [PATCH 05/21] refactor test (review) --- packages/ra-core/src/dataProvider/useGetList.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/ra-core/src/dataProvider/useGetList.ts b/packages/ra-core/src/dataProvider/useGetList.ts index 0b65d863776..affa51937a7 100644 --- a/packages/ra-core/src/dataProvider/useGetList.ts +++ b/packages/ra-core/src/dataProvider/useGetList.ts @@ -71,18 +71,11 @@ const useGetList = ( { type: 'getList', resource, payload: { pagination, sort, filter } }, options, // data selector (may return []) - // We don't want the list to reset to first page after a bulk delete if the current page still exists - // But CrudDeleteMany reset all "cachedRequests" for this resource. - // It caused ids to be empty, and useListController to automatically reset to page 1 - // Now, it "cachedRequests" is undefined, we try default ids before setting an empty ids array (state: ReduxState): Identifier[] => get( state.admin.resources, [resource, 'list', 'cachedRequests', requestSignature, 'ids'], - state.admin.resources[resource] && - state.admin.resources[resource].list - ? state.admin.resources[resource].list.ids - : [] + [] ), // total selector (may return undefined) (state: ReduxState): number => From 76740a57492130977ddc7859105f76d7aef4f6fc Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 14:40:54 +0200 Subject: [PATCH 06/21] Use a useMemo instead of a useCallback to compute the nb of pages in Pagination --- .../ra-ui-materialui/src/list/Pagination.tsx | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/ra-ui-materialui/src/list/Pagination.tsx b/packages/ra-ui-materialui/src/list/Pagination.tsx index bc699c6f561..7676d083a26 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.tsx +++ b/packages/ra-ui-materialui/src/list/Pagination.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { useEffect, useCallback, FC, ReactElement } from 'react'; +import { useEffect, useMemo, useCallback, FC, ReactElement } from 'react'; import PropTypes from 'prop-types'; import { TablePagination, @@ -31,18 +31,17 @@ const Pagination: FC = props => { setPerPage, } = useListContext(props); - const getNbPages = useCallback(() => Math.ceil(total / perPage) || 1, [ - perPage, - total, - ]); + const nbPages = useMemo(() => { + return Math.ceil(total / perPage) || 1; + }, [perPage, total]); useEffect(() => { if (page < 1 || isNaN(page)) { setPage(1); - } else if (page > getNbPages()) { - setPage(getNbPages()); + } else if (page > nbPages) { + setPage(nbPages); } - }, [page, setPage, getNbPages, total, perPage]); + }, [page, nbPages, total, perPage, setPage]); const translate = useTranslate(); const isSmall = useMediaQuery((theme: Theme) => @@ -55,7 +54,7 @@ const Pagination: FC = props => { const handlePageChange = useCallback( (event, page) => { event && event.stopPropagation(); - if (page < 0 || page > getNbPages() - 1) { + if (page < 0 || page > nbPages - 1) { throw new Error( translate('ra.navigation.page_out_of_boundaries', { page: page + 1, @@ -85,7 +84,7 @@ const Pagination: FC = props => { ); // Avoid rendering TablePagination if "page" value is invalid - if (total === 0 || page > getNbPages()) { + if (total === 0 || page > nbPages) { return loading ? : limit; } From 3637749ccbaa9d519a237e5d86063a73c25b4de3 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 15:47:53 +0200 Subject: [PATCH 07/21] Fix the getNumberOrDefault method that returns the default value instead of 0 when parsing "0" --- .../src/controller/useListParams.spec.ts | 28 ++++++++++++++++++- .../ra-core/src/controller/useListParams.ts | 19 +++++++++---- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/packages/ra-core/src/controller/useListParams.spec.ts b/packages/ra-core/src/controller/useListParams.spec.ts index ad258c86a37..e878d49b665 100644 --- a/packages/ra-core/src/controller/useListParams.spec.ts +++ b/packages/ra-core/src/controller/useListParams.spec.ts @@ -1,4 +1,4 @@ -import { getQuery } from './useListParams'; +import { getQuery, getNumberOrDefault } from './useListParams'; import { SORT_DESC, SORT_ASC, @@ -156,4 +156,30 @@ describe('useListParams', () => { }); }); }); + + describe('getNumberOrDefault', () => { + it('should return the parsed number', () => { + const result = getNumberOrDefault('29', 2); + + expect(result).toEqual(29); + }); + + it('should return the default number when passing a not valid number', () => { + const result = getNumberOrDefault('covfefe', 2); + + expect(result).toEqual(2); + }); + + it('should return the default number when passing an undefined number', () => { + const result = getNumberOrDefault(undefined, 2); + + expect(result).toEqual(2); + }); + + it('should not return the default number when passing "0"', () => { + const result = getNumberOrDefault('0', 2); + + expect(result).toEqual(0); + }); + }); }); diff --git a/packages/ra-core/src/controller/useListParams.ts b/packages/ra-core/src/controller/useListParams.ts index b3daf6d9a39..f49fe3864d7 100644 --- a/packages/ra-core/src/controller/useListParams.ts +++ b/packages/ra-core/src/controller/useListParams.ts @@ -342,12 +342,15 @@ export const getQuery = ({ query.sort = sort.field; query.order = sort.order; } - if (!query.perPage) { + if (query.perPage == null) { + console.log('perPage'); query.perPage = perPage; } - if (!query.page) { + if (query.page == null) { + console.log('page'); query.page = 1; } + return { ...query, page: getNumberOrDefault(query.page, 1), @@ -358,9 +361,13 @@ export const getQuery = ({ export const getNumberOrDefault = ( possibleNumber: string | number | undefined, defaultValue: number -) => - (typeof possibleNumber === 'string' - ? parseInt(possibleNumber, 10) - : possibleNumber) || defaultValue; +) => { + const parsedNumber = + typeof possibleNumber === 'string' + ? parseInt(possibleNumber, 10) + : possibleNumber; + + return isNaN(parsedNumber) ? defaultValue : parsedNumber; +}; export default useListParams; From e6357bcc95f53d588a8b44073c1dc22fcbcc9b85 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 16:09:07 +0200 Subject: [PATCH 08/21] Compute the total pages for a list and pass it to the list context --- packages/ra-core/src/controller/ListContext.tsx | 2 ++ .../ra-core/src/controller/useListController.ts | 6 ++++++ packages/ra-ui-materialui/src/list/Pagination.tsx | 15 ++------------- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/packages/ra-core/src/controller/ListContext.tsx b/packages/ra-core/src/controller/ListContext.tsx index 57bb3bec3a9..94f8062009b 100644 --- a/packages/ra-core/src/controller/ListContext.tsx +++ b/packages/ra-core/src/controller/ListContext.tsx @@ -14,6 +14,7 @@ import { ListControllerProps } from './useListController'; * @prop {boolean} loaded boolean that is false until the data is available * @prop {boolean} loading boolean that is true on mount, and false once the data was fetched * @prop {integer} page the current page. Starts at 1 + * @prop {integer} totalPages the total number of pages for the current filters. Corresponds to total / perPage. * @prop {Function} setPage a callback to change the page, e.g. setPage(3) * @prop {integer} perPage the number of results per page. Defaults to 25 * @prop {Function} setPerPage a callback to change the number of results per page, e.g. setPerPage(25) @@ -78,6 +79,7 @@ const ListContext = createContext({ setSort: null, showFilter: null, total: null, + totalPages: null, }); ListContext.displayName = 'ListContext'; diff --git a/packages/ra-core/src/controller/useListController.ts b/packages/ra-core/src/controller/useListController.ts index 4c383b91df3..5185b81ce88 100644 --- a/packages/ra-core/src/controller/useListController.ts +++ b/packages/ra-core/src/controller/useListController.ts @@ -184,6 +184,10 @@ const useListController = ( const finalIds = typeof total === 'undefined' ? defaultIds : ids; + const totalPages = useMemo(() => { + return Math.ceil(total / query.perPage) || 1; + }, [query, total]); + useEffect(() => { if ( query.page <= 0 || @@ -237,6 +241,7 @@ const useListController = ( setSort: queryModifiers.setSort, showFilter: queryModifiers.showFilter, total: typeof total === 'undefined' ? defaultTotal : total, + totalPages, }; }; @@ -268,6 +273,7 @@ export const injectedProps = [ 'setSort', 'showFilter', 'total', + 'totalPages', 'version', ]; diff --git a/packages/ra-ui-materialui/src/list/Pagination.tsx b/packages/ra-ui-materialui/src/list/Pagination.tsx index 7676d083a26..78794ba134b 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.tsx +++ b/packages/ra-ui-materialui/src/list/Pagination.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { useEffect, useMemo, useCallback, FC, ReactElement } from 'react'; +import { useCallback, FC, ReactElement } from 'react'; import PropTypes from 'prop-types'; import { TablePagination, @@ -25,24 +25,13 @@ const Pagination: FC = props => { const { loading, page, + nbPages, perPage, total, setPage, setPerPage, } = useListContext(props); - const nbPages = useMemo(() => { - return Math.ceil(total / perPage) || 1; - }, [perPage, total]); - - useEffect(() => { - if (page < 1 || isNaN(page)) { - setPage(1); - } else if (page > nbPages) { - setPage(nbPages); - } - }, [page, nbPages, total, perPage, setPage]); - const translate = useTranslate(); const isSmall = useMediaQuery((theme: Theme) => theme.breakpoints.down('sm') From 0774a434bfc3ae0fc435b843221110d34ab16660 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 16:19:57 +0200 Subject: [PATCH 09/21] Set the page to the last existing page in the useListController if querying a page out of bounds --- .../ra-core/src/controller/useListController.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/ra-core/src/controller/useListController.ts b/packages/ra-core/src/controller/useListController.ts index 5185b81ce88..96e196bd652 100644 --- a/packages/ra-core/src/controller/useListController.ts +++ b/packages/ra-core/src/controller/useListController.ts @@ -193,10 +193,22 @@ const useListController = ( query.page <= 0 || (!loading && query.page > 1 && (finalIds || []).length === 0) ) { - // query for a page that doesn't exist, set page to 1 + // Query for a page that doesn't exist, set page to 1 queryModifiers.setPage(1); + } else if (!loading && query.page > totalPages) { + // Query for a page out of bounds, set page to the last existing page + // It occurs when deleting the last element of the last page + queryModifiers.setPage(totalPages); } - }, [loading, query.page, finalIds, queryModifiers, total, defaultIds]); + }, [ + loading, + query, + finalIds, + queryModifiers, + total, + totalPages, + defaultIds, + ]); const currentSort = useMemo( () => ({ From 8850de60602b4cae33fa724b7c066902739f5b1f Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 16:32:22 +0200 Subject: [PATCH 10/21] Use totalPages in Pagination --- packages/ra-ui-materialui/src/list/Pagination.spec.js | 11 ++++++++--- packages/ra-ui-materialui/src/list/Pagination.tsx | 6 +++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/ra-ui-materialui/src/list/Pagination.spec.js b/packages/ra-ui-materialui/src/list/Pagination.spec.js index 177458eb0b3..4e2f6c9ac06 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.spec.js +++ b/packages/ra-ui-materialui/src/list/Pagination.spec.js @@ -49,16 +49,21 @@ describe('', () => { const { queryByText } = render( ); // mui TablePagination displays no more a warning in that case - // Then useEffect fallback on a valid page + // Then useEffect fallbacks on a valid page expect(queryByText('ra.navigation.no_results')).not.toBeNull(); - await wait(expect(setPage).toBeCalledWith(1), 1000); }); }); diff --git a/packages/ra-ui-materialui/src/list/Pagination.tsx b/packages/ra-ui-materialui/src/list/Pagination.tsx index 78794ba134b..be45ee660ce 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.tsx +++ b/packages/ra-ui-materialui/src/list/Pagination.tsx @@ -25,7 +25,7 @@ const Pagination: FC = props => { const { loading, page, - nbPages, + totalPages, perPage, total, setPage, @@ -43,7 +43,7 @@ const Pagination: FC = props => { const handlePageChange = useCallback( (event, page) => { event && event.stopPropagation(); - if (page < 0 || page > nbPages - 1) { + if (page < 0 || page >= totalPages) { throw new Error( translate('ra.navigation.page_out_of_boundaries', { page: page + 1, @@ -73,7 +73,7 @@ const Pagination: FC = props => { ); // Avoid rendering TablePagination if "page" value is invalid - if (total === 0 || page > nbPages) { + if (total === 0 || page > totalPages) { return loading ? : limit; } From fa8ec886f2c39d6184efb8c900a8502e4db93a4d Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 16:44:46 +0200 Subject: [PATCH 11/21] Remove console.log --- packages/ra-core/src/controller/useListParams.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/ra-core/src/controller/useListParams.ts b/packages/ra-core/src/controller/useListParams.ts index f49fe3864d7..6150dfe61cf 100644 --- a/packages/ra-core/src/controller/useListParams.ts +++ b/packages/ra-core/src/controller/useListParams.ts @@ -343,11 +343,9 @@ export const getQuery = ({ query.order = sort.order; } if (query.perPage == null) { - console.log('perPage'); query.perPage = perPage; } if (query.page == null) { - console.log('page'); query.page = 1; } From e3c9cf410b6e573d3ff144985687690cbbb87fa0 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 16:53:40 +0200 Subject: [PATCH 12/21] Sanitize the total pages prop in the ListView --- packages/ra-ui-materialui/src/list/ListView.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ra-ui-materialui/src/list/ListView.tsx b/packages/ra-ui-materialui/src/list/ListView.tsx index 048b52c7dd6..50b15e912f0 100644 --- a/packages/ra-ui-materialui/src/list/ListView.tsx +++ b/packages/ra-ui-materialui/src/list/ListView.tsx @@ -253,6 +253,7 @@ const sanitizeRestProps: ( showFilter, sort, total, + totalPages, ...rest }) => rest; From f08f96f2fe69d73b64e1780005715e74188fafc6 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 16:54:50 +0200 Subject: [PATCH 13/21] Fix total page out of bound limit for material ui in Pagination --- packages/ra-ui-materialui/src/list/Pagination.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ra-ui-materialui/src/list/Pagination.tsx b/packages/ra-ui-materialui/src/list/Pagination.tsx index be45ee660ce..20fc16b1e6d 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.tsx +++ b/packages/ra-ui-materialui/src/list/Pagination.tsx @@ -43,7 +43,7 @@ const Pagination: FC = props => { const handlePageChange = useCallback( (event, page) => { event && event.stopPropagation(); - if (page < 0 || page >= totalPages) { + if (page < 0 || page > totalPages - 1) { throw new Error( translate('ra.navigation.page_out_of_boundaries', { page: page + 1, From 9e499ad18acfe5224d9f782357b11c08141c7934 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 17:02:41 +0200 Subject: [PATCH 14/21] Fix a problem with Pagination when trying to pass negative pages through the URL --- packages/ra-ui-materialui/src/list/Pagination.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ra-ui-materialui/src/list/Pagination.tsx b/packages/ra-ui-materialui/src/list/Pagination.tsx index 20fc16b1e6d..fa8eded343e 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.tsx +++ b/packages/ra-ui-materialui/src/list/Pagination.tsx @@ -52,7 +52,7 @@ const Pagination: FC = props => { } setPage(page + 1); }, - [total, perPage, setPage, translate] // eslint-disable-line react-hooks/exhaustive-deps + [totalPages, setPage, translate] ); const handlePerPageChange = useCallback( @@ -73,7 +73,7 @@ const Pagination: FC = props => { ); // Avoid rendering TablePagination if "page" value is invalid - if (total === 0 || page > totalPages) { + if (total === 0 || page < 0 || page > totalPages) { return loading ? : limit; } From 956f627c18e92d16b162deae89c483a5109a2315 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 17:13:55 +0200 Subject: [PATCH 15/21] Write a new test for negative pages --- .../src/list/Pagination.spec.js | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/ra-ui-materialui/src/list/Pagination.spec.js b/packages/ra-ui-materialui/src/list/Pagination.spec.js index 4e2f6c9ac06..fb18189bd9f 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.spec.js +++ b/packages/ra-ui-materialui/src/list/Pagination.spec.js @@ -43,7 +43,7 @@ describe('', () => { expect(queryByText('ra.navigation.no_results')).toBeNull(); }); - it('should display a pagination limit on an out of bounds page', async () => { + it('should display a pagination limit on an out of bounds page (more than total pages)', async () => { jest.spyOn(console, 'error').mockImplementationOnce(() => {}); const setPage = jest.fn().mockReturnValue(null); const { queryByText } = render( @@ -65,6 +65,29 @@ describe('', () => { // Then useEffect fallbacks on a valid page expect(queryByText('ra.navigation.no_results')).not.toBeNull(); }); + + it('should display a pagination limit on an out of bounds page (less than 0)', async () => { + jest.spyOn(console, 'error').mockImplementationOnce(() => {}); + const setPage = jest.fn().mockReturnValue(null); + const { queryByText } = render( + + + + + + ); + // mui TablePagination displays no more a warning in that case + // Then useEffect fallbacks on a valid page + expect(queryByText('ra.navigation.no_results')).not.toBeNull(); + }); }); describe('Pagination buttons', () => { From 3d33bfb55252029f11541a87754eb1eba14ecfc1 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Thu, 23 Jul 2020 17:27:18 +0200 Subject: [PATCH 16/21] Fix the ListControllerProps TypeScript type --- packages/ra-core/src/controller/useListController.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ra-core/src/controller/useListController.ts b/packages/ra-core/src/controller/useListController.ts index 96e196bd652..f4c060a9ac8 100644 --- a/packages/ra-core/src/controller/useListController.ts +++ b/packages/ra-core/src/controller/useListController.ts @@ -79,6 +79,7 @@ export interface ListControllerProps { setSort: (sort: string, order?: string) => void; showFilter: (filterName: string, defaultValue: any) => void; total: number; + totalPages: number; } /** From 446ed85df6692e868d0f006a75149c04df11be2c Mon Sep 17 00:00:00 2001 From: Luwangel Date: Fri, 24 Jul 2020 11:45:14 +0200 Subject: [PATCH 17/21] Make the total pages optionnal in TypeScript types --- packages/ra-core/src/controller/useListController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ra-core/src/controller/useListController.ts b/packages/ra-core/src/controller/useListController.ts index f4c060a9ac8..0e5ac25224e 100644 --- a/packages/ra-core/src/controller/useListController.ts +++ b/packages/ra-core/src/controller/useListController.ts @@ -79,7 +79,7 @@ export interface ListControllerProps { setSort: (sort: string, order?: string) => void; showFilter: (filterName: string, defaultValue: any) => void; total: number; - totalPages: number; + totalPages?: number; } /** From 8116dbbd9021133c4ff57323321f7d0796c358c2 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Fri, 24 Jul 2020 15:02:27 +0200 Subject: [PATCH 18/21] Count the total pages everywhere and not make it optional --- .../field/useReferenceArrayFieldController.ts | 11 +++++++++-- .../field/useReferenceManyFieldController.ts | 7 ++++++- packages/ra-core/src/controller/useListController.ts | 2 +- packages/ra-ui-materialui/src/field/ArrayField.tsx | 2 +- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts b/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts index 0c33075478e..3c83dc1f3a2 100644 --- a/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts +++ b/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useRef } from 'react'; +import { useCallback, useMemo, useEffect, useRef } from 'react'; import get from 'lodash/get'; import isEqual from 'lodash/isEqual'; @@ -194,6 +194,12 @@ const useReferenceArrayFieldController = ({ sort.order, ]); + const total = finalIds.length; + + const totalPages = useMemo(() => { + return Math.ceil(total / perPage) || 1; + }, [perPage, total]); + return { basePath: basePath.replace(resource, reference), currentSort: sort, @@ -219,7 +225,8 @@ const useReferenceArrayFieldController = ({ setPerPage, setSort, showFilter, - total: finalIds.length, + total, + totalPages, }; }; diff --git a/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts b/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts index 10c7868365f..e3ccf6a731c 100644 --- a/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts +++ b/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts @@ -1,5 +1,5 @@ import get from 'lodash/get'; -import { useCallback, useEffect, useRef } from 'react'; +import { useCallback, useMemo, useEffect, useRef } from 'react'; import isEqual from 'lodash/isEqual'; import { useSafeSetState, removeEmpty } from '../../util'; @@ -173,6 +173,10 @@ const useReferenceManyFieldController = ({ } ); + const totalPages = useMemo(() => { + return Math.ceil(total / perPage) || 1; + }, [perPage, total]); + return { basePath: basePath.replace(resource, reference), currentSort: sort, @@ -199,6 +203,7 @@ const useReferenceManyFieldController = ({ setSort, showFilter, total, + totalPages, }; }; diff --git a/packages/ra-core/src/controller/useListController.ts b/packages/ra-core/src/controller/useListController.ts index 0e5ac25224e..f4c060a9ac8 100644 --- a/packages/ra-core/src/controller/useListController.ts +++ b/packages/ra-core/src/controller/useListController.ts @@ -79,7 +79,7 @@ export interface ListControllerProps { setSort: (sort: string, order?: string) => void; showFilter: (filterName: string, defaultValue: any) => void; total: number; - totalPages?: number; + totalPages: number; } /** diff --git a/packages/ra-ui-materialui/src/field/ArrayField.tsx b/packages/ra-ui-materialui/src/field/ArrayField.tsx index 45c47b8a2d5..0853fc0894a 100644 --- a/packages/ra-ui-materialui/src/field/ArrayField.tsx +++ b/packages/ra-ui-materialui/src/field/ArrayField.tsx @@ -135,7 +135,6 @@ export const ArrayField: FC = memo( setData(data); }, [record, source, fieldKey]); - // @ts-ignore return ( = memo( setSort: null, showFilter: null, total: null, + totalPages: null, }} > {cloneElement(Children.only(children), { From 7c75b3090df2e98748ba9f18714563ec78050469 Mon Sep 17 00:00:00 2001 From: Luwangel Date: Fri, 24 Jul 2020 15:20:02 +0200 Subject: [PATCH 19/21] Remove the totalPages from contexts because it could be recomputed using perPage and total --- packages/ra-core/src/controller/ListContext.tsx | 2 -- .../controller/field/useReferenceArrayFieldController.ts | 7 +------ .../controller/field/useReferenceManyFieldController.ts | 5 ----- packages/ra-core/src/controller/useListController.ts | 2 -- packages/ra-ui-materialui/src/field/ArrayField.tsx | 1 - packages/ra-ui-materialui/src/list/ListView.tsx | 1 - packages/ra-ui-materialui/src/list/Pagination.spec.js | 4 ++-- packages/ra-ui-materialui/src/list/Pagination.tsx | 7 +++++-- 8 files changed, 8 insertions(+), 21 deletions(-) diff --git a/packages/ra-core/src/controller/ListContext.tsx b/packages/ra-core/src/controller/ListContext.tsx index 94f8062009b..57bb3bec3a9 100644 --- a/packages/ra-core/src/controller/ListContext.tsx +++ b/packages/ra-core/src/controller/ListContext.tsx @@ -14,7 +14,6 @@ import { ListControllerProps } from './useListController'; * @prop {boolean} loaded boolean that is false until the data is available * @prop {boolean} loading boolean that is true on mount, and false once the data was fetched * @prop {integer} page the current page. Starts at 1 - * @prop {integer} totalPages the total number of pages for the current filters. Corresponds to total / perPage. * @prop {Function} setPage a callback to change the page, e.g. setPage(3) * @prop {integer} perPage the number of results per page. Defaults to 25 * @prop {Function} setPerPage a callback to change the number of results per page, e.g. setPerPage(25) @@ -79,7 +78,6 @@ const ListContext = createContext({ setSort: null, showFilter: null, total: null, - totalPages: null, }); ListContext.displayName = 'ListContext'; diff --git a/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts b/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts index 3c83dc1f3a2..ed83a45de79 100644 --- a/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts +++ b/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts @@ -1,4 +1,4 @@ -import { useCallback, useMemo, useEffect, useRef } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import get from 'lodash/get'; import isEqual from 'lodash/isEqual'; @@ -196,10 +196,6 @@ const useReferenceArrayFieldController = ({ const total = finalIds.length; - const totalPages = useMemo(() => { - return Math.ceil(total / perPage) || 1; - }, [perPage, total]); - return { basePath: basePath.replace(resource, reference), currentSort: sort, @@ -226,7 +222,6 @@ const useReferenceArrayFieldController = ({ setSort, showFilter, total, - totalPages, }; }; diff --git a/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts b/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts index e3ccf6a731c..c02a5b1e848 100644 --- a/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts +++ b/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts @@ -173,10 +173,6 @@ const useReferenceManyFieldController = ({ } ); - const totalPages = useMemo(() => { - return Math.ceil(total / perPage) || 1; - }, [perPage, total]); - return { basePath: basePath.replace(resource, reference), currentSort: sort, @@ -203,7 +199,6 @@ const useReferenceManyFieldController = ({ setSort, showFilter, total, - totalPages, }; }; diff --git a/packages/ra-core/src/controller/useListController.ts b/packages/ra-core/src/controller/useListController.ts index f4c060a9ac8..b1340bf65ac 100644 --- a/packages/ra-core/src/controller/useListController.ts +++ b/packages/ra-core/src/controller/useListController.ts @@ -79,7 +79,6 @@ export interface ListControllerProps { setSort: (sort: string, order?: string) => void; showFilter: (filterName: string, defaultValue: any) => void; total: number; - totalPages: number; } /** @@ -254,7 +253,6 @@ const useListController = ( setSort: queryModifiers.setSort, showFilter: queryModifiers.showFilter, total: typeof total === 'undefined' ? defaultTotal : total, - totalPages, }; }; diff --git a/packages/ra-ui-materialui/src/field/ArrayField.tsx b/packages/ra-ui-materialui/src/field/ArrayField.tsx index 0853fc0894a..ef956bd78f8 100644 --- a/packages/ra-ui-materialui/src/field/ArrayField.tsx +++ b/packages/ra-ui-materialui/src/field/ArrayField.tsx @@ -161,7 +161,6 @@ export const ArrayField: FC = memo( setSort: null, showFilter: null, total: null, - totalPages: null, }} > {cloneElement(Children.only(children), { diff --git a/packages/ra-ui-materialui/src/list/ListView.tsx b/packages/ra-ui-materialui/src/list/ListView.tsx index 50b15e912f0..048b52c7dd6 100644 --- a/packages/ra-ui-materialui/src/list/ListView.tsx +++ b/packages/ra-ui-materialui/src/list/ListView.tsx @@ -253,7 +253,6 @@ const sanitizeRestProps: ( showFilter, sort, total, - totalPages, ...rest }) => rest; diff --git a/packages/ra-ui-materialui/src/list/Pagination.spec.js b/packages/ra-ui-materialui/src/list/Pagination.spec.js index fb18189bd9f..6d540690594 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.spec.js +++ b/packages/ra-ui-materialui/src/list/Pagination.spec.js @@ -53,7 +53,7 @@ describe('', () => { ...defaultProps, total: 10, page: 2, // Query the page 2 but there is only 1 page - totalPages: 1, + perPage: 10, setPage, }} > @@ -76,7 +76,7 @@ describe('', () => { ...defaultProps, total: 10, page: -2, // Query the page -2 😱 - totalPages: 1, + perPage: 10, setPage, }} > diff --git a/packages/ra-ui-materialui/src/list/Pagination.tsx b/packages/ra-ui-materialui/src/list/Pagination.tsx index fa8eded343e..dca635f6359 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.tsx +++ b/packages/ra-ui-materialui/src/list/Pagination.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { useCallback, FC, ReactElement } from 'react'; +import { useCallback, useMemo, FC, ReactElement } from 'react'; import PropTypes from 'prop-types'; import { TablePagination, @@ -25,7 +25,6 @@ const Pagination: FC = props => { const { loading, page, - totalPages, perPage, total, setPage, @@ -37,6 +36,10 @@ const Pagination: FC = props => { theme.breakpoints.down('sm') ); + const totalPages = useMemo(() => { + return Math.ceil(total / perPage) || 1; + }, [perPage, total]); + /** * Warning: material-ui's page is 0-based */ From 6e46f4119255d63a55ac756b551b8f2a7e25778b Mon Sep 17 00:00:00 2001 From: Luwangel Date: Fri, 24 Jul 2020 17:40:19 +0200 Subject: [PATCH 20/21] Apply reviews --- .../src/controller/field/useReferenceArrayFieldController.ts | 4 +--- .../src/controller/field/useReferenceManyFieldController.ts | 2 +- packages/ra-core/src/controller/useListController.ts | 2 +- packages/ra-ui-materialui/src/list/Pagination.tsx | 2 +- 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts b/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts index ed83a45de79..0c33075478e 100644 --- a/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts +++ b/packages/ra-core/src/controller/field/useReferenceArrayFieldController.ts @@ -194,8 +194,6 @@ const useReferenceArrayFieldController = ({ sort.order, ]); - const total = finalIds.length; - return { basePath: basePath.replace(resource, reference), currentSort: sort, @@ -221,7 +219,7 @@ const useReferenceArrayFieldController = ({ setPerPage, setSort, showFilter, - total, + total: finalIds.length, }; }; diff --git a/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts b/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts index c02a5b1e848..10c7868365f 100644 --- a/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts +++ b/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts @@ -1,5 +1,5 @@ import get from 'lodash/get'; -import { useCallback, useMemo, useEffect, useRef } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import isEqual from 'lodash/isEqual'; import { useSafeSetState, removeEmpty } from '../../util'; diff --git a/packages/ra-core/src/controller/useListController.ts b/packages/ra-core/src/controller/useListController.ts index b1340bf65ac..68e2bad54b0 100644 --- a/packages/ra-core/src/controller/useListController.ts +++ b/packages/ra-core/src/controller/useListController.ts @@ -186,7 +186,7 @@ const useListController = ( const totalPages = useMemo(() => { return Math.ceil(total / query.perPage) || 1; - }, [query, total]); + }, [query.perPage, total]); useEffect(() => { if ( diff --git a/packages/ra-ui-materialui/src/list/Pagination.tsx b/packages/ra-ui-materialui/src/list/Pagination.tsx index dca635f6359..fbd7b5ce401 100644 --- a/packages/ra-ui-materialui/src/list/Pagination.tsx +++ b/packages/ra-ui-materialui/src/list/Pagination.tsx @@ -76,7 +76,7 @@ const Pagination: FC = props => { ); // Avoid rendering TablePagination if "page" value is invalid - if (total === 0 || page < 0 || page > totalPages) { + if (total === 0 || page < 1 || page > totalPages) { return loading ? : limit; } From 8223ab03299903cf5867c3efaec0ba3c045b5d3e Mon Sep 17 00:00:00 2001 From: Luwangel Date: Fri, 24 Jul 2020 17:46:57 +0200 Subject: [PATCH 21/21] Fix useEffect in useListController --- packages/ra-core/src/controller/useListController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ra-core/src/controller/useListController.ts b/packages/ra-core/src/controller/useListController.ts index 68e2bad54b0..1d4c91fafea 100644 --- a/packages/ra-core/src/controller/useListController.ts +++ b/packages/ra-core/src/controller/useListController.ts @@ -202,7 +202,7 @@ const useListController = ( } }, [ loading, - query, + query.page, finalIds, queryModifiers, total,