Merge pull request #4894 from marmelab/fix-pagination-on-delete

Fix pagination on bulk delete

packages/ra-core/src/controller/useListController.spec.tsx

@@ -166,6 +166,7 @@ describe('useListController', () => {
                                 list: {
                                     params: {},
                                     cachedRequests: {},
+                                    ids: [],
                                 },
                             },
                         },

packages/ra-core/src/controller/useListController.ts

@@ -184,15 +184,31 @@ const useListController = <RecordType = Record>(
 
     const finalIds = typeof total === 'undefined' ? defaultIds : ids;
 
+    const totalPages = useMemo(() => {
+        return Math.ceil(total / query.perPage) || 1;
+    }, [query.perPage, total]);
+
     useEffect(() => {
         if (
             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.page,
+        finalIds,
+        queryModifiers,
+        total,
+        totalPages,
+        defaultIds,
+    ]);
 
     const currentSort = useMemo(
         () => ({
@@ -268,6 +284,7 @@ export const injectedProps = [
     'setSort',
     'showFilter',
     'total',
+    'totalPages',
     'version',
 ];
 

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);
+        });
+    });
 });

packages/ra-core/src/controller/useListParams.ts

@@ -342,12 +342,13 @@ export const getQuery = ({
         query.sort = sort.field;
         query.order = sort.order;
     }
-    if (!query.perPage) {
+    if (query.perPage == null) {
         query.perPage = perPage;
     }
-    if (!query.page) {
+    if (query.page == null) {
         query.page = 1;
     }
+
     return {
         ...query,
         page: getNumberOrDefault(query.page, 1),
@@ -358,9 +359,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;

packages/ra-ui-materialui/src/field/ArrayField.tsx

@@ -135,7 +135,6 @@ export const ArrayField: FC<ArrayFieldProps> = memo<ArrayFieldProps>(
             setData(data);
         }, [record, source, fieldKey]);
 
-        // @ts-ignore
         return (
             <ListContext.Provider
                 value={{

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,50 @@ describe('<Pagination />', () => {
             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 (more than total pages)', async () => {
             jest.spyOn(console, 'error').mockImplementationOnce(() => {});
+            const setPage = jest.fn().mockReturnValue(null);
             const { queryByText } = render(
                 <ThemeProvider theme={theme}>
                     <ListContext.Provider
-                        value={{ ...defaultProps, total: 10, page: 2 }}
+                        value={{
+                            ...defaultProps,
+                            total: 10,
+                            page: 2, // Query the page 2 but there is only 1 page
+                            perPage: 10,
+                            setPage,
+                        }}
                     >
                         <Pagination />
                     </ListContext.Provider>
                 </ThemeProvider>
             );
-            // 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 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(
+                <ThemeProvider theme={theme}>
+                    <ListContext.Provider
+                        value={{
+                            ...defaultProps,
+                            total: 10,
+                            page: -2, // Query the page -2 😱
+                            perPage: 10,
+                            setPage,
+                        }}
+                    >
+                        <Pagination />
+                    </ListContext.Provider>
+                </ThemeProvider>
+            );
+            // 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();
         });
     });
 

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 { useCallback, useMemo, FC, ReactElement } from 'react';
 import PropTypes from 'prop-types';
 import {
     TablePagination,
@@ -30,25 +30,23 @@ const Pagination: FC<PaginationProps> = props => {
         setPage,
         setPerPage,
     } = useListContext(props);
-    useEffect(() => {
-        if (page < 1 || isNaN(page)) {
-            setPage(1);
-        }
-    }, [page, setPage]);
+
     const translate = useTranslate();
     const isSmall = useMediaQuery((theme: Theme) =>
         theme.breakpoints.down('sm')
     );
 
-    const getNbPages = () => Math.ceil(total / perPage) || 1;
+    const totalPages = useMemo(() => {
+        return Math.ceil(total / perPage) || 1;
+    }, [perPage, total]);
 
     /**
      * Warning: material-ui's page is 0-based
      */
     const handlePageChange = useCallback(
         (event, page) => {
             event && event.stopPropagation();
-            if (page < 0 || page > getNbPages() - 1) {
+            if (page < 0 || page > totalPages - 1) {
                 throw new Error(
                     translate('ra.navigation.page_out_of_boundaries', {
                         page: page + 1,
@@ -57,7 +55,7 @@ const Pagination: FC<PaginationProps> = props => {
             }
             setPage(page + 1);
         },
-        [total, perPage, setPage, translate] // eslint-disable-line react-hooks/exhaustive-deps
+        [totalPages, setPage, translate]
     );
 
     const handlePerPageChange = useCallback(
@@ -77,7 +75,8 @@ const Pagination: FC<PaginationProps> = props => {
         [translate]
     );
 
-    if (total === 0) {
+    // Avoid rendering TablePagination if "page" value is invalid
+    if (total === 0 || page < 1 || page > totalPages) {
         return loading ? <Toolbar variant="dense" /> : limit;
     }