Skip to content

Commit feb7769

Browse files
djhidjhi
authoredApr 30, 2021
Merge pull request #6226 from marmelab/fix-multiple-hide-filter
Fix hideFilter called repeatedly only registers the last call
·
v5.6.4v3.15.0
2 parents dec80a5 + 03d4931 commit feb7769

File tree

5 files changed

+311
-95
lines changed

5 files changed

+311
-95
lines changed
 

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

+44
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,50 @@ describe('useListController', () => {
350350
});
351351
});
352352
});
353+
354+
it('should support to sync calls', async () => {
355+
const { getByLabelText, queryByText } = renderWithRedux(
356+
<ListController {...defaultProps}>
357+
{({ displayedFilters, showFilter }) => (
358+
<>
359+
<button
360+
aria-label="Show filters"
361+
onClick={() => {
362+
showFilter('filter1.subdata', 'bob');
363+
showFilter('filter2', '');
364+
}}
365+
/>
366+
{Object.keys(displayedFilters).map(
367+
(displayedFilter, index) => (
368+
<div key={index}>{displayedFilter}</div>
369+
)
370+
)}
371+
</>
372+
)}
373+
</ListController>,
374+
{
375+
admin: {
376+
resources: {
377+
posts: {
378+
list: {
379+
params: {
380+
filter: { q: 'hello' },
381+
},
382+
cachedRequests: {},
383+
},
384+
},
385+
},
386+
},
387+
}
388+
);
389+
390+
fireEvent.click(getByLabelText('Show filters'));
391+
392+
await waitFor(() => {
393+
expect(queryByText('filter1.subdata')).not.toBeNull();
394+
expect(queryByText('filter2')).not.toBeNull();
395+
});
396+
});
353397
});
354398

355399
describe('getListControllerProps', () => {

‎packages/ra-core/src/controller/useListParams.spec.tsx

+54-43
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ describe('useListParams', () => {
229229
});
230230
});
231231
});
232-
it('should initialize filters', () => {
232+
it('should initialize filters', async () => {
233233
const TestedComponent = () => {
234234
const [, { showFilter }] = useListParams({
235235
resource: 'foo',
@@ -250,22 +250,24 @@ describe('useListParams', () => {
250250
</TestContext>
251251
</Router>
252252
);
253-
expect(history.push).toBeCalledWith({
254-
search:
255-
'?' +
256-
stringify({
257-
displayedFilters: JSON.stringify({ foo: true }),
258-
filter: JSON.stringify({ foo: 'bar' }),
259-
sort: 'id',
260-
order: 'ASC',
261-
page: 1,
262-
perPage: 10,
263-
}),
264-
state: { _scrollToTop: false },
253+
await waitFor(() => {
254+
expect(history.push).toBeCalledWith({
255+
search:
256+
'?' +
257+
stringify({
258+
displayedFilters: JSON.stringify({ foo: true }),
259+
filter: JSON.stringify({ foo: 'bar' }),
260+
sort: 'id',
261+
order: 'ASC',
262+
page: 1,
263+
perPage: 10,
264+
}),
265+
state: { _scrollToTop: false },
266+
});
265267
});
266268
});
267269

268-
it('should initialize displayed filters on compound filters', () => {
270+
it('should initialize displayed filters on compound filters', async () => {
269271
const TestedComponent = () => {
270272
const [, { showFilter }] = useListParams({
271273
resource: 'foo',
@@ -286,22 +288,26 @@ describe('useListParams', () => {
286288
</TestContext>
287289
</Router>
288290
);
289-
expect(history.push).toBeCalledWith({
290-
search:
291-
'?' +
292-
stringify({
293-
displayedFilters: JSON.stringify({ 'foo.bar': true }),
294-
filter: JSON.stringify({ foo: { bar: 'baz' } }),
295-
sort: 'id',
296-
order: 'ASC',
297-
page: 1,
298-
perPage: 10,
299-
}),
300-
state: { _scrollToTop: false },
291+
await waitFor(() => {
292+
expect(history.push).toBeCalledWith({
293+
search:
294+
'?' +
295+
stringify({
296+
displayedFilters: JSON.stringify({
297+
'foo.bar': true,
298+
}),
299+
filter: JSON.stringify({ foo: { bar: 'baz' } }),
300+
sort: 'id',
301+
order: 'ASC',
302+
page: 1,
303+
perPage: 10,
304+
}),
305+
state: { _scrollToTop: false },
306+
});
301307
});
302308
});
303309

304-
it('should initialize filters on compound filters', () => {
310+
it('should initialize filters on compound filters', async () => {
305311
const TestedComponent = () => {
306312
const [, { showFilter }] = useListParams({
307313
resource: 'foo',
@@ -322,18 +328,22 @@ describe('useListParams', () => {
322328
</TestContext>
323329
</Router>
324330
);
325-
expect(history.push).toBeCalledWith({
326-
search:
327-
'?' +
328-
stringify({
329-
displayedFilters: JSON.stringify({ 'foo.bar': true }),
330-
filter: JSON.stringify({ foo: { bar: 'baz' } }),
331-
sort: 'id',
332-
order: 'ASC',
333-
page: 1,
334-
perPage: 10,
335-
}),
336-
state: { _scrollToTop: false },
331+
await waitFor(() => {
332+
expect(history.push).toBeCalledWith({
333+
search:
334+
'?' +
335+
stringify({
336+
displayedFilters: JSON.stringify({
337+
'foo.bar': true,
338+
}),
339+
filter: JSON.stringify({ foo: { bar: 'baz' } }),
340+
sort: 'id',
341+
order: 'ASC',
342+
page: 1,
343+
perPage: 10,
344+
}),
345+
state: { _scrollToTop: false },
346+
});
337347
});
338348
});
339349
});
@@ -351,7 +361,7 @@ describe('useListParams', () => {
351361
return <button onClick={handleClick}>update</button>;
352362
};
353363

354-
test('should synchronize parameters with location and redux state when sync is enabled', async () => {
364+
it('should synchronize parameters with location and redux state when sync is enabled', async () => {
355365
const history = createMemoryHistory();
356366
jest.spyOn(history, 'push');
357367
let dispatch;
@@ -366,9 +376,10 @@ describe('useListParams', () => {
366376
);
367377

368378
fireEvent.click(getByText('update'));
369-
370-
expect(history.push).toHaveBeenCalled();
371-
expect(dispatch).toHaveBeenCalled();
379+
await waitFor(() => {
380+
expect(history.push).toHaveBeenCalled();
381+
expect(dispatch).toHaveBeenCalled();
382+
});
372383
});
373384

374385
test('should not synchronize parameters with location and redux state when sync is not enabled', async () => {

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

+33-42
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
1-
import { useCallback, useMemo, useEffect, useState } from 'react';
1+
import { useCallback, useMemo, useEffect, useState, useRef } from 'react';
22
import { useSelector, useDispatch, shallowEqual } from 'react-redux';
33
import { parse, stringify } from 'query-string';
44
import lodashDebounce from 'lodash/debounce';
5-
import set from 'lodash/set';
65
import pickBy from 'lodash/pickBy';
76
import { useHistory, useLocation } from 'react-router-dom';
87

98
import queryReducer, {
109
SET_FILTER,
10+
HIDE_FILTER,
11+
SHOW_FILTER,
1112
SET_PAGE,
1213
SET_PER_PAGE,
1314
SET_SORT,
@@ -16,7 +17,6 @@ import queryReducer, {
1617
import { changeListParams, ListParams } from '../actions/listActions';
1718
import { SortPayload, ReduxState, FilterPayload } from '../types';
1819
import removeEmpty from '../util/removeEmpty';
19-
import removeKey from '../util/removeKey';
2020

2121
interface ListParamsOptions {
2222
resource: string;
@@ -125,6 +125,7 @@ const useListParams = ({
125125
: defaultParams,
126126
shallowEqual
127127
);
128+
const tempParams = useRef<ListParams>();
128129

129130
const requestSignature = [
130131
location.search,
@@ -163,21 +164,31 @@ const useListParams = ({
163164
}, []); // eslint-disable-line
164165

165166
const changeParams = useCallback(action => {
166-
const newParams = queryReducer(query, action);
167-
if (syncWithLocation) {
168-
history.push({
169-
search: `?${stringify({
170-
...newParams,
171-
filter: JSON.stringify(newParams.filter),
172-
displayedFilters: JSON.stringify(
173-
newParams.displayedFilters
174-
),
175-
})}`,
176-
state: { _scrollToTop: action.type === SET_PAGE },
177-
});
178-
dispatch(changeListParams(resource, newParams));
167+
if (!tempParams.current) {
168+
// no other changeParams action dispatched this tick
169+
tempParams.current = queryReducer(query, action);
170+
// schedule side effects for next tick
171+
setTimeout(() => {
172+
if (syncWithLocation) {
173+
history.push({
174+
search: `?${stringify({
175+
...tempParams.current,
176+
filter: JSON.stringify(tempParams.current.filter),
177+
displayedFilters: JSON.stringify(
178+
tempParams.current.displayedFilters
179+
),
180+
})}`,
181+
state: { _scrollToTop: action.type === SET_PAGE },
182+
});
183+
dispatch(changeListParams(resource, tempParams.current));
184+
} else {
185+
setLocalParams(tempParams.current);
186+
}
187+
tempParams.current = undefined;
188+
}, 0);
179189
} else {
180-
setLocalParams(newParams);
190+
// side effects already scheduled, just change the params
191+
tempParams.current = queryReducer(tempParams.current, action);
181192
}
182193
}, requestSignature); // eslint-disable-line react-hooks/exhaustive-deps
183194

@@ -229,38 +240,18 @@ const useListParams = ({
229240
);
230241

231242
const hideFilter = useCallback((filterName: string) => {
232-
// we don't use lodash.set() for displayed filters
233-
// to avoid problems with compound filter names (e.g. 'author.name')
234-
const displayedFilters = Object.keys(displayedFilterValues).reduce(
235-
(filters, filter) => {
236-
return filter !== filterName
237-
? { ...filters, [filter]: true }
238-
: filters;
239-
},
240-
{}
241-
);
242-
const filter = removeEmpty(removeKey(filterValues, filterName));
243243
changeParams({
244-
type: SET_FILTER,
245-
payload: { filter, displayedFilters },
244+
type: HIDE_FILTER,
245+
payload: filterName,
246246
});
247247
}, requestSignature); // eslint-disable-line react-hooks/exhaustive-deps
248248

249249
const showFilter = useCallback((filterName: string, defaultValue: any) => {
250-
// we don't use lodash.set() for displayed filters
251-
// to avoid problems with compound filter names (e.g. 'author.name')
252-
const displayedFilters = {
253-
...displayedFilterValues,
254-
[filterName]: true,
255-
};
256-
const filter = defaultValue
257-
? set(filterValues, filterName, defaultValue)
258-
: filterValues;
259250
changeParams({
260-
type: SET_FILTER,
251+
type: SHOW_FILTER,
261252
payload: {
262-
filter,
263-
displayedFilters,
253+
filterName,
254+
defaultValue,
264255
},
265256
});
266257
}, requestSignature); // eslint-disable-line react-hooks/exhaustive-deps

‎packages/ra-core/src/reducer/admin/resource/list/queryReducer.spec.ts

+87
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,93 @@ describe('Query Reducer', () => {
8181
expect(updatedState.page).toEqual(1);
8282
});
8383
});
84+
describe('SHOW_FILTER action', () => {
85+
it('should add the filter to the displayed filters and set the filter value', () => {
86+
const updatedState = queryReducer(
87+
{
88+
filter: { bar: 1 },
89+
displayedFilters: { bar: true },
90+
},
91+
{
92+
type: 'SHOW_FILTER',
93+
payload: { filterName: 'foo', defaultValue: 'bar' },
94+
}
95+
);
96+
expect(updatedState.filter).toEqual({ bar: 1, foo: 'bar' });
97+
expect(updatedState.displayedFilters).toEqual({
98+
bar: true,
99+
foo: true,
100+
});
101+
});
102+
103+
it('should work without default value', () => {
104+
const updatedState = queryReducer(
105+
{
106+
filter: { bar: 1 },
107+
displayedFilters: { bar: true },
108+
},
109+
{
110+
type: 'SHOW_FILTER',
111+
payload: { filterName: 'foo' },
112+
}
113+
);
114+
expect(updatedState.filter).toEqual({ bar: 1 });
115+
expect(updatedState.displayedFilters).toEqual({
116+
bar: true,
117+
foo: true,
118+
});
119+
});
120+
121+
it('should not change an already shown filter', () => {
122+
const updatedState = queryReducer(
123+
{
124+
filter: { foo: 1 },
125+
displayedFilters: { foo: true },
126+
},
127+
{
128+
type: 'SHOW_FILTER',
129+
payload: { filterName: 'foo', defaultValue: 'bar' },
130+
}
131+
);
132+
expect(updatedState.filter).toEqual({ foo: 1 });
133+
expect(updatedState.displayedFilters).toEqual({ foo: true });
134+
});
135+
});
136+
describe('HIDE_FILTER action', () => {
137+
it('should remove the filter from the displayed filters and reset the filter value', () => {
138+
const updatedState = queryReducer(
139+
{
140+
filter: { foo: 2, bar: 1 },
141+
displayedFilters: { foo: true, bar: true },
142+
},
143+
{
144+
type: 'HIDE_FILTER',
145+
payload: 'bar',
146+
}
147+
);
148+
expect(updatedState.filter).toEqual({ foo: 2 });
149+
expect(updatedState.displayedFilters).toEqual({
150+
foo: true,
151+
});
152+
});
153+
154+
it('should do nothing if the filter is already hidden', () => {
155+
const updatedState = queryReducer(
156+
{
157+
filter: { foo: 2 },
158+
displayedFilters: { foo: true },
159+
},
160+
{
161+
type: 'HIDE_FILTER',
162+
payload: 'bar',
163+
}
164+
);
165+
expect(updatedState.filter).toEqual({ foo: 2 });
166+
expect(updatedState.displayedFilters).toEqual({
167+
foo: true,
168+
});
169+
});
170+
});
84171
describe('SET_SORT action', () => {
85172
it('should set SORT_ASC order by default when sort value is new', () => {
86173
const updatedState = queryReducer(

‎packages/ra-core/src/reducer/admin/resource/list/queryReducer.ts

+93-10
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
import { Reducer } from 'redux';
2+
import set from 'lodash/set';
3+
4+
import removeEmpty from '../../../../util/removeEmpty';
5+
import removeKey from '../../../../util/removeKey';
26
import { ListParams } from '../../../../actions';
7+
38
export const SET_SORT = 'SET_SORT';
49
export const SORT_ASC = 'ASC';
510
export const SORT_DESC = 'DESC';
@@ -8,20 +13,51 @@ export const SET_PAGE = 'SET_PAGE';
813
export const SET_PER_PAGE = 'SET_PER_PAGE';
914

1015
export const SET_FILTER = 'SET_FILTER';
16+
export const SHOW_FILTER = 'SHOW_FILTER';
17+
export const HIDE_FILTER = 'HIDE_FILTER';
1118

1219
const oppositeOrder = direction =>
1320
direction === SORT_DESC ? SORT_ASC : SORT_DESC;
1421

22+
type ActionTypes =
23+
| {
24+
type: typeof SET_SORT;
25+
payload: { sort: string; order?: typeof SORT_ASC | typeof SORT_DESC };
26+
}
27+
| {
28+
type: typeof SET_PAGE;
29+
payload: number;
30+
}
31+
| {
32+
type: typeof SET_PER_PAGE;
33+
payload: number;
34+
}
35+
| {
36+
type: typeof SET_FILTER;
37+
payload: {
38+
filter: any;
39+
displayedFilters?: { [key: string]: boolean };
40+
};
41+
}
42+
| {
43+
type: typeof SHOW_FILTER;
44+
payload: { filterName: string; defaultValue?: any };
45+
}
46+
| {
47+
type: typeof HIDE_FILTER;
48+
payload: string;
49+
};
50+
1551
/**
1652
* This reducer is for the react-router query string, NOT for redux.
1753
*/
1854
const queryReducer: Reducer<ListParams> = (
1955
previousState,
20-
{ type, payload }
56+
action: ActionTypes
2157
) => {
22-
switch (type) {
58+
switch (action.type) {
2359
case SET_SORT:
24-
if (payload.sort === previousState.sort) {
60+
if (action.payload.sort === previousState.sort) {
2561
return {
2662
...previousState,
2763
order: oppositeOrder(previousState.order),
@@ -31,24 +67,71 @@ const queryReducer: Reducer<ListParams> = (
3167

3268
return {
3369
...previousState,
34-
sort: payload.sort,
35-
order: payload.order || SORT_ASC,
70+
sort: action.payload.sort,
71+
order: action.payload.order || SORT_ASC,
3672
page: 1,
3773
};
3874

3975
case SET_PAGE:
40-
return { ...previousState, page: payload };
76+
return { ...previousState, page: action.payload };
4177

4278
case SET_PER_PAGE:
43-
return { ...previousState, page: 1, perPage: payload };
79+
return { ...previousState, page: 1, perPage: action.payload };
4480

4581
case SET_FILTER: {
4682
return {
4783
...previousState,
4884
page: 1,
49-
filter: payload.filter,
50-
displayedFilters: payload.displayedFilters
51-
? payload.displayedFilters
85+
filter: action.payload.filter,
86+
displayedFilters: action.payload.displayedFilters
87+
? action.payload.displayedFilters
88+
: previousState.displayedFilters,
89+
};
90+
}
91+
92+
case SHOW_FILTER: {
93+
if (
94+
previousState.displayedFilters &&
95+
previousState.displayedFilters[action.payload.filterName]
96+
) {
97+
// the filter is already shown
98+
return previousState;
99+
}
100+
return {
101+
...previousState,
102+
filter: action.payload.defaultValue
103+
? set(
104+
previousState.filter,
105+
action.payload.filterName,
106+
action.payload.defaultValue
107+
)
108+
: previousState.filter,
109+
// we don't use lodash.set() for displayed filters
110+
// to avoid problems with compound filter names (e.g. 'author.name')
111+
displayedFilters: {
112+
...previousState.displayedFilters,
113+
[action.payload.filterName]: true,
114+
},
115+
};
116+
}
117+
118+
case HIDE_FILTER: {
119+
return {
120+
...previousState,
121+
filter: removeEmpty(
122+
removeKey(previousState.filter, action.payload)
123+
),
124+
// we don't use lodash.set() for displayed filters
125+
// to avoid problems with compound filter names (e.g. 'author.name')
126+
displayedFilters: previousState.displayedFilters
127+
? Object.keys(previousState.displayedFilters).reduce(
128+
(filters, filter) => {
129+
return filter !== action.payload
130+
? { ...filters, [filter]: true }
131+
: filters;
132+
},
133+
{}
134+
)
52135
: previousState.displayedFilters,
53136
};
54137
}

0 commit comments

Comments
 (0)
Please sign in to comment.