Skip to content

Commit bdcee65

Browse files
slax57slax57
authored
Merge pull request #10997 from marmelab/fix-save-button-dirty-check
Fix `<SaveButton>` form dirty status check
2 parents 11af04a + 95da6b1 commit bdcee65

File tree

7 files changed

+310
-8
lines changed

7 files changed

+310
-8
lines changed

‎packages/ra-core/src/form/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,4 @@ export * from './useWarnWhenUnsavedChanges';
1111
export * from './validation';
1212
export * from './WarnWhenUnsavedChanges';
1313
export * from './FilterLiveForm';
14+
export * from './useFormIsDirty';

‎packages/ra-core/src/form/useFormIsDirty.spec.ts‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
import { checkHasDirtyFields } from './useFormIsDirty';
2+
3+
describe('useFormIsDirty', () => {
4+
describe('checkHasDirtyFields', () => {
5+
it('should return true if any field is dirty on simple forms', () => {
6+
const dirtyFields = { name: true, age: false };
7+
expect(checkHasDirtyFields(dirtyFields)).toBe(true);
8+
});
9+
10+
it('should return false if no field is dirty on simple forms', () => {
11+
const dirtyFields = { name: false, age: false };
12+
expect(checkHasDirtyFields(dirtyFields)).toBe(false);
13+
});
14+
15+
it('should return true if any field is dirty on forms with nested fields', () => {
16+
const dirtyFields = {
17+
name: false,
18+
age: false,
19+
address: { street: true, city: false },
20+
};
21+
expect(checkHasDirtyFields(dirtyFields)).toBe(true);
22+
});
23+
24+
it('should return false if no field is dirty on forms with nested fields', () => {
25+
const dirtyFields = {
26+
name: false,
27+
age: false,
28+
address: { street: false, city: false },
29+
};
30+
expect(checkHasDirtyFields(dirtyFields)).toBe(false);
31+
});
32+
33+
it('should return true if any field is dirty on forms with array of scalar fields', () => {
34+
const dirtyFields = {
35+
name: false,
36+
age: false,
37+
hobbies: [true, false],
38+
};
39+
expect(checkHasDirtyFields(dirtyFields)).toBe(true);
40+
});
41+
42+
it('should return false if no field is dirty on forms with array of scalar fields', () => {
43+
const dirtyFields = {
44+
name: false,
45+
age: false,
46+
hobbies: [false, false],
47+
};
48+
expect(checkHasDirtyFields(dirtyFields)).toBe(false);
49+
});
50+
51+
it('should return true if any field is dirty on forms with array of objects', () => {
52+
const dirtyFields = {
53+
name: false,
54+
age: false,
55+
hobbies: [{ name: true }, { name: false }],
56+
};
57+
expect(checkHasDirtyFields(dirtyFields)).toBe(true);
58+
});
59+
60+
it('should return false if no field is dirty on forms with array of objects', () => {
61+
const dirtyFields = {
62+
name: false,
63+
age: false,
64+
hobbies: [{ name: false }, { name: false }],
65+
};
66+
expect(checkHasDirtyFields(dirtyFields)).toBe(false);
67+
});
68+
69+
it('should return true if any field is dirty on forms with nested array of objects', () => {
70+
const dirtyFields = {
71+
name: false,
72+
age: false,
73+
address: {
74+
street: false,
75+
city: [{ name: true }, { name: false }],
76+
},
77+
};
78+
expect(checkHasDirtyFields(dirtyFields)).toBe(true);
79+
});
80+
81+
it('should return false if no field is dirty on forms with nested array of objects', () => {
82+
const dirtyFields = {
83+
name: false,
84+
age: false,
85+
address: {
86+
street: false,
87+
city: [{ name: false }, { name: false }],
88+
},
89+
};
90+
expect(checkHasDirtyFields(dirtyFields)).toBe(false);
91+
});
92+
93+
// nested array of scalar values
94+
it('should return true if any field is dirty on forms with nested array of scalar values', () => {
95+
const dirtyFields = {
96+
name: false,
97+
age: false,
98+
hobbies: [
99+
{ name: false, tags: [true, true] },
100+
{ name: false, tags: [false, false] },
101+
],
102+
};
103+
expect(checkHasDirtyFields(dirtyFields)).toBe(true);
104+
});
105+
106+
it('should return false if no field is dirty on forms with nested array of scalar values', () => {
107+
const dirtyFields = {
108+
name: false,
109+
age: false,
110+
hobbies: [
111+
{ name: false, tags: [false, false] },
112+
{ name: false, tags: [false, false] },
113+
],
114+
};
115+
expect(checkHasDirtyFields(dirtyFields)).toBe(false);
116+
});
117+
118+
it('should return true when an array contains an empty object (new item)', () => {
119+
const dirtyFields = {
120+
name: false,
121+
age: false,
122+
hobbies: [{}], // empty object should be considered dirty
123+
};
124+
expect(checkHasDirtyFields(dirtyFields)).toBe(true);
125+
});
126+
127+
it('should return true when an array contains undefined entries (new item)', () => {
128+
const dirtyFields = {
129+
name: false,
130+
age: false,
131+
hobbies: [undefined], // undefined should be considered dirty
132+
} as any;
133+
expect(checkHasDirtyFields(dirtyFields)).toBe(true);
134+
});
135+
});
136+
});

‎packages/ra-core/src/form/useFormIsDirty.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import { useFormState } from 'react-hook-form';
2+
import isEmpty from 'lodash/isEmpty.js';
3+
4+
// useFormState().isDirty might differ from useFormState().dirtyFields (https://github.com/react-hook-form/react-hook-form/issues/4740)
5+
export const useFormIsDirty = (): boolean => {
6+
const { dirtyFields } = useFormState();
7+
return checkHasDirtyFields(dirtyFields);
8+
};
9+
10+
export const checkHasDirtyFields = (
11+
dirtyFields: Partial<
12+
Readonly<{
13+
[x: string]: any;
14+
}>
15+
>
16+
): boolean => {
17+
// dirtyFields can contains simple keys with boolean values, nested objects or arrays
18+
// We must ignore values that are false
19+
return Object.values(dirtyFields).some(value => {
20+
if (typeof value === 'boolean') {
21+
return value;
22+
} else if (Array.isArray(value)) {
23+
// Some arrays contain only booleans (scalar arrays), some arrays contain objects (object arrays)
24+
for (const item of value) {
25+
if (item === true) {
26+
return true;
27+
}
28+
// FIXME: because we currently don't set default values correctly for arrays,
29+
// new items are either empty objects, or undefined in dirtyFields. Consider them as dirty.
30+
if (
31+
(typeof item === 'object' && isEmpty(item)) ||
32+
item === undefined
33+
) {
34+
return true;
35+
}
36+
if (
37+
typeof item === 'object' &&
38+
item !== null &&
39+
checkHasDirtyFields(item)
40+
) {
41+
return true;
42+
}
43+
}
44+
} else if (typeof value === 'object' && value !== null) {
45+
return checkHasDirtyFields(value);
46+
}
47+
return false;
48+
});
49+
};

‎packages/ra-ui-materialui/src/button/SaveButton.spec.tsx‎

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { AdminContext } from '../AdminContext';
2323
import {
2424
AlwaysEnable,
2525
Basic,
26+
ComplexForm,
2627
EnabledWhenFormIsPrefilled,
2728
} from './SaveButton.stories';
2829

@@ -401,7 +402,7 @@ describe('<SaveButton />', () => {
401402
);
402403
});
403404

404-
it('should render enabled if the form is prefilled', async () => {
405+
it('should be enabled if the form is prefilled', async () => {
405406
render(<EnabledWhenFormIsPrefilled />);
406407
await waitFor(() =>
407408
expect(screen.getByLabelText('ra.action.save')['disabled']).toEqual(
@@ -410,6 +411,73 @@ describe('<SaveButton />', () => {
410411
);
411412
});
412413

414+
it('should enable/disable consistently in a complex form', async () => {
415+
render(<ComplexForm />);
416+
await waitFor(() =>
417+
expect(screen.getByLabelText('ra.action.save')['disabled']).toEqual(
418+
true
419+
)
420+
);
421+
fireEvent.change(await screen.getByDisplayValue('Lorem ipsum'), {
422+
target: { value: 'Lorem ipsum dolor' },
423+
});
424+
await waitFor(() =>
425+
expect(screen.getByLabelText('ra.action.save')['disabled']).toEqual(
426+
false
427+
)
428+
);
429+
fireEvent.click(screen.getByText('ra.action.save'));
430+
await waitFor(() =>
431+
expect(screen.getByLabelText('ra.action.save')['disabled']).toEqual(
432+
true
433+
)
434+
);
435+
fireEvent.change(await screen.getByDisplayValue('bazinga'), {
436+
target: { value: 'bazingaaaa' },
437+
});
438+
439+
await waitFor(() =>
440+
expect(screen.getByLabelText('ra.action.save')['disabled']).toEqual(
441+
false
442+
)
443+
);
444+
fireEvent.click(screen.getByText('ra.action.save'));
445+
await waitFor(() =>
446+
expect(screen.getByLabelText('ra.action.save')['disabled']).toEqual(
447+
true
448+
)
449+
);
450+
fireEvent.click(screen.getByLabelText('ra.action.add'));
451+
await waitFor(() =>
452+
expect(screen.getByLabelText('ra.action.save')['disabled']).toEqual(
453+
false
454+
)
455+
);
456+
await waitFor(() =>
457+
expect(
458+
screen.queryAllByLabelText('resources.posts.fields.tags.name')
459+
.length
460+
).toEqual(2)
461+
);
462+
fireEvent.change(
463+
screen.getAllByLabelText('resources.posts.fields.tags.name')[1],
464+
{
465+
target: { value: 'plop' },
466+
}
467+
);
468+
await waitFor(() =>
469+
expect(screen.getByLabelText('ra.action.save')['disabled']).toEqual(
470+
false
471+
)
472+
);
473+
fireEvent.click(screen.getByText('ra.action.save'));
474+
await waitFor(() =>
475+
expect(screen.getByLabelText('ra.action.save')['disabled']).toEqual(
476+
true
477+
)
478+
);
479+
});
480+
413481
it('should not be enabled if no inputs have changed', async () => {
414482
render(
415483
<AdminContext dataProvider={testDataProvider()}>

‎packages/ra-ui-materialui/src/button/SaveButton.stories.tsx‎

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,15 @@
11
import * as React from 'react';
2-
import { Form, TestMemoryRouter } from 'ra-core';
2+
import { Form, ResourceContextProvider, TestMemoryRouter } from 'ra-core';
33
import { Paper } from '@mui/material';
4+
import fakeRestDataProvider from 'ra-data-fakerest';
5+
import { useFormContext } from 'react-hook-form';
46

57
import { SaveButton } from './SaveButton';
8+
import { ArrayInput } from '../input/ArrayInput/ArrayInput';
9+
import { TextInput } from '../input/TextInput';
10+
import { SimpleFormIterator } from '../input/ArrayInput/SimpleFormIterator';
611
import { AdminContext } from '../AdminContext';
7-
import { useFormContext } from 'react-hook-form';
12+
import { Edit } from '../detail';
813

914
export default {
1015
title: 'ra-ui-materialui/button/SaveButton',
@@ -81,3 +86,45 @@ export const Submitting = () => (
8186
</AdminContext>
8287
</TestMemoryRouter>
8388
);
89+
90+
export const ComplexForm = () => (
91+
<AdminContext
92+
dataProvider={fakeRestDataProvider(
93+
{
94+
posts: [
95+
{
96+
id: 1,
97+
title: 'Lorem ipsum',
98+
tags: [{ name: 'bazinga' }],
99+
},
100+
],
101+
},
102+
process.env.NODE_ENV !== 'test',
103+
300
104+
)}
105+
>
106+
<Paper>
107+
<ResourceContextProvider value="posts">
108+
<Edit id={1} redirect={false}>
109+
<Form>
110+
<TextInput source="title" />
111+
<ArrayInput source="tags">
112+
<SimpleFormIterator>
113+
<TextInput source="name" />
114+
</SimpleFormIterator>
115+
</ArrayInput>
116+
<SaveButton />
117+
<FormInspector />
118+
</Form>
119+
</Edit>
120+
</ResourceContextProvider>
121+
</Paper>
122+
</AdminContext>
123+
);
124+
125+
const FormInspector = () => {
126+
const {
127+
formState: { isDirty, dirtyFields },
128+
} = useFormContext();
129+
return <p>{JSON.stringify({ isDirty, dirtyFields })}</p>;
130+
};

‎packages/ra-ui-materialui/src/button/SaveButton.tsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
warning,
2020
setSubmissionErrors,
2121
useRecordFromLocation,
22+
useFormIsDirty,
2223
} from 'ra-core';
2324

2425
/**
@@ -72,9 +73,9 @@ export const SaveButton = <RecordType extends RaRecord = any>(
7273
const translate = useTranslate();
7374
const form = useFormContext();
7475
const saveContext = useSaveContext();
75-
const { dirtyFields, isValidating, isSubmitting } = useFormState();
76+
const { isValidating, isSubmitting } = useFormState();
7677
// useFormState().isDirty might differ from useFormState().dirtyFields (https://github.com/react-hook-form/react-hook-form/issues/4740)
77-
const isDirty = Object.keys(dirtyFields).length > 0;
78+
const isDirty = useFormIsDirty();
7879
// Use form isDirty, isValidating and form context saving to enable or disable the save button
7980
// if alwaysEnable is undefined and the form wasn't prefilled
8081
const recordFromLocation = useRecordFromLocation();

‎yarn.lock‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19904,11 +19904,11 @@ __metadata:
1990419904
linkType: hard
1990519905

1990619906
"react-hook-form@npm:^7.53.0":
19907-
version: 7.53.0
19908-
resolution: "react-hook-form@npm:7.53.0"
19907+
version: 7.53.2
19908+
resolution: "react-hook-form@npm:7.53.2"
1990919909
peerDependencies:
1991019910
react: ^16.8.0 || ^17 || ^18 || ^19
19911-
checksum: 6d62b150618a833c17d59e669b707661499e2bb516a8d340ca37699f99eb448bbba7b5b78324938c8948014e21efaa32e3510c2ba246fd5e97a96fca0cfa7c98
19911+
checksum: 18336d8e8798a70dcd0af703a0becca2d5dbf82a7b7a3ca334ae0e1f26410490bc3ef2ea51adcf790bb1e7006ed7a763fd00d664e398f71225b23529a7ccf0bf
1991219912
languageName: node
1991319913
linkType: hard
1991419914

0 commit comments

Comments
 (0)