Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix List view error after delete when using a field with no record test #5900

Merged
merged 7 commits into from
Feb 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 71 additions & 3 deletions packages/ra-core/src/controller/useListController.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import * as React from 'react';
import expect from 'expect';
import { fireEvent, waitFor } from '@testing-library/react';
import { fireEvent, waitFor, act } from '@testing-library/react';
import lolex from 'lolex';
import TextField from '@material-ui/core/TextField/TextField';

import { DataProviderContext } from '../dataProvider';
import ListController from './ListController';
import {
getListControllerProps,
Expand Down Expand Up @@ -41,6 +42,73 @@ describe('useListController', () => {
debounce: 200,
};

describe('data', () => {
it('should be synchronized with ids after delete', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test fails on master

const FooField = ({ record }) => <span>{record.foo}</span>;
const dataProvider = {
getList: () =>
Promise.resolve({
data: [
{ id: 1, foo: 'foo1' },
{ id: 2, foo: 'foo2' },
],
total: 2,
}),
};
const { dispatch, queryByText } = renderWithRedux(
<DataProviderContext.Provider value={dataProvider}>
<ListController
{...defaultProps}
resource="comments"
filter={{ foo: 1 }}
>
{({ data, ids }) => (
<>
{ids.map(id => (
<FooField key={id} record={data[id]} />
))}
</>
)}
</ListController>
</DataProviderContext.Provider>,
{
admin: {
resources: {
comments: {
list: {
params: {},
cachedRequests: {},
ids: [],
selectedIds: [],
total: null,
},
data: {},
},
},
},
}
);
await act(async () => await new Promise(r => setTimeout(r)));

// delete one post
act(() => {
dispatch({
type: 'RA/CRUD_DELETE_OPTIMISTIC',
payload: { id: 1 },
meta: {
resource: 'comments',
fetch: 'DELETE',
optimistic: true,
},
});
});
await act(async () => await new Promise(r => setTimeout(r)));

expect(queryByText('foo1')).toBeNull();
expect(queryByText('foo2')).not.toBeNull();
});
});

describe('setFilters', () => {
let clock;
let fakeComponent = ({ setFilters, filterValues }) => (
Expand Down Expand Up @@ -153,7 +221,6 @@ describe('useListController', () => {
const props = {
...defaultProps,
debounce: 200,
crudGetList: jest.fn(),
children,
};

Expand All @@ -171,13 +238,14 @@ describe('useListController', () => {
params: {},
cachedRequests: {},
ids: [],
selectedIds: [],
total: null,
},
},
},
},
}
);

const crudGetListCalls = dispatch.mock.calls.filter(
call => call[0].type === 'RA/CRUD_GET_LIST'
);
Expand Down
60 changes: 10 additions & 50 deletions packages/ra-core/src/controller/useListController.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
import { isValidElement, ReactElement, useEffect, useMemo } from 'react';
import inflection from 'inflection';
import { Location } from 'history';
import { useSelector } from 'react-redux';
import get from 'lodash/get';

import { useCheckMinimumRequiredProps } from './checkMinimumRequiredProps';
import useListParams from './useListParams';
import useRecordSelection from './useRecordSelection';
import useTranslate from '../i18n/useTranslate';
import useNotify from '../sideEffect/useNotify';
import useGetList from '../dataProvider/useGetList';
import { useGetMainList } from '../dataProvider/useGetMainList';
import { SORT_ASC } from '../reducer/admin/resource/list/queryReducer';
import { CRUD_GET_LIST } from '../actions';
import defaultExporter from '../export/defaultExporter';
Expand All @@ -18,7 +16,6 @@ import {
SortPayload,
RecordMap,
Identifier,
ReduxState,
Record,
Exporter,
} from '../types';
Expand Down Expand Up @@ -53,8 +50,6 @@ const defaultSort = {
order: SORT_ASC,
};

const defaultData = {};

export interface ListControllerProps<RecordType extends Record = Record> {
basePath: string;
currentSort: SortPayload;
Expand Down Expand Up @@ -153,7 +148,9 @@ const useListController = <RecordType extends Record = Record>(
* We want the list of ids to be always available for optimistic rendering,
* and therefore we need a custom action (CRUD_GET_LIST) that will be used.
*/
const { ids, total, error, loading, loaded } = useGetList<RecordType>(
const { ids, data, total, error, loading, loaded } = useGetMainList<
RecordType
>(
resource,
{
page: query.page,
Expand Down Expand Up @@ -181,41 +178,12 @@ const useListController = <RecordType extends Record = Record>(
}
);

const data = useSelector(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic moved to a special hook, useGetMainList

(state: ReduxState): RecordMap<RecordType> =>
get(
state.admin.resources,
[resource, 'data'],
defaultData
) as RecordMap<RecordType>
);

// When the user changes the page/sort/filter, this controller runs the
// useGetList hook again. While the result of this new call is loading,
// the ids and total are empty. To avoid rendering an empty list at that
// moment, we override the ids and total with the latest loaded ones.
const defaultIds = useSelector((state: ReduxState): Identifier[] =>
get(state.admin.resources, [resource, 'list', 'ids'], [])
);
const defaultTotal = useSelector((state: ReduxState): number =>
get(state.admin.resources, [resource, 'list', 'total'])
);

// Since the total can be empty during the loading phase
// We need to override that total with the latest loaded one
// This way, the useEffect bellow won't reset the page to 1
const finalTotal = typeof total === 'undefined' ? defaultTotal : total;

const finalIds = typeof total === 'undefined' ? defaultIds : ids;

const totalPages = useMemo(() => {
return Math.ceil(finalTotal / query.perPage) || 1;
}, [query.perPage, finalTotal]);
const totalPages = Math.ceil(total / query.perPage) || 1;

useEffect(() => {
if (
query.page <= 0 ||
(!loading && query.page > 1 && (finalIds || []).length === 0)
(!loading && query.page > 1 && ids.length === 0)
) {
// Query for a page that doesn't exist, set page to 1
queryModifiers.setPage(1);
Expand All @@ -224,15 +192,7 @@ const useListController = <RecordType extends Record = Record>(
// It occurs when deleting the last element of the last page
queryModifiers.setPage(totalPages);
}
}, [
loading,
query.page,
finalIds,
queryModifiers,
total,
totalPages,
defaultIds,
]);
}, [loading, query.page, ids, queryModifiers, total, totalPages]);

const currentSort = useMemo(
() => ({
Expand Down Expand Up @@ -262,8 +222,8 @@ const useListController = <RecordType extends Record = Record>(
filterValues: query.filterValues,
hasCreate,
hideFilter: queryModifiers.hideFilter,
ids: finalIds,
loaded: loaded || defaultIds.length > 0,
ids,
loaded: loaded || ids.length > 0,
loading,
onSelect: selectionModifiers.select,
onToggleItem: selectionModifiers.toggle,
Expand All @@ -277,7 +237,7 @@ const useListController = <RecordType extends Record = Record>(
setPerPage: queryModifiers.setPerPage,
setSort: queryModifiers.setSort,
showFilter: queryModifiers.showFilter,
total: finalTotal,
total: total,
};
};

Expand Down
2 changes: 2 additions & 0 deletions packages/ra-core/src/dataProvider/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import useQueryWithStore, { QueryOptions } from './useQueryWithStore';
import withDataProvider from './withDataProvider';
import useGetOne, { UseGetOneHookValue } from './useGetOne';
import useGetList from './useGetList';
import { useGetMainList } from './useGetMainList';
import useGetMany from './useGetMany';
import useGetManyReference from './useGetManyReference';
import useGetMatching from './useGetMatching';
Expand Down Expand Up @@ -45,6 +46,7 @@ export {
useQuery,
useGetOne,
useGetList,
useGetMainList,
useGetMany,
useGetManyReference,
useGetMatching,
Expand Down
Loading