diff --git a/packages/ra-core/src/controller/button/useDeleteController.tsx b/packages/ra-core/src/controller/button/useDeleteController.tsx index 486178d3f99..9a3d400f3e8 100644 --- a/packages/ra-core/src/controller/button/useDeleteController.tsx +++ b/packages/ra-core/src/controller/button/useDeleteController.tsx @@ -1,4 +1,5 @@ import { useCallback, useMemo } from 'react'; +import { useFormContext } from 'react-hook-form'; import { useDelete, UseDeleteOptions } from '../../dataProvider/useDelete'; import { useUnselect } from '../list/useUnselect'; @@ -75,6 +76,8 @@ export const useDeleteController = < const unselect = useUnselect(resource); const redirect = useRedirect(); const translate = useTranslate(); + // null when the delete button is used outside a form (e.g. in a list) + const form = useFormContext(); const [deleteOne, { isPending }] = useDelete( resource, @@ -96,6 +99,11 @@ export const useDeleteController = < } ); record && unselect([record.id], true); + // Clear the form dirty state so warnWhenUnsavedChanges doesn't + // block the redirect: the record is gone, so the unsaved + // changes are moot. keepValues avoids a flash of reverted + // fields before the redirect commits. + form?.reset(undefined, { keepValues: true }); redirect(redirectTo, resource); }, onError: (error: any) => { diff --git a/packages/ra-core/src/form/Form.spec.tsx b/packages/ra-core/src/form/Form.spec.tsx index 325c09df18e..ca632ee7ede 100644 --- a/packages/ra-core/src/form/Form.spec.tsx +++ b/packages/ra-core/src/form/Form.spec.tsx @@ -22,6 +22,7 @@ import { InNonDataRouter, ServerSideValidation, MultiRoutesForm, + WarnWhenUnsavedChangesWithDelete, } from './Form.stories'; import { mergeTranslations } from '../i18n'; @@ -1008,4 +1009,37 @@ describe('Form', () => { ).toEqual(false); } ); + + describe('warnWhenUnsavedChanges with a delete button', () => { + it.each(['undoable', 'pessimistic', 'optimistic'] as const)( + 'should not warn about unsaved changes after deleting a dirty record (%s)', + async mutationMode => { + // spy on "cancel": if the dialog were shown the navigation + // would be cancelled, failing the redirect assertion + const confirmSpy = jest + .spyOn(window, 'confirm') + .mockReturnValue(false); + render( + + ); + // wait for the record to load + const input = + await screen.findByDisplayValue('Hello'); + // make the form dirty + fireEvent.change(input, { + target: { value: 'Hello modified' }, + }); + fireEvent.blur(input); + // delete the record (the headless DeleteButton has no + // confirmation dialog and always uses the controller onSuccess) + fireEvent.click(screen.getByText('Delete')); + // the app should redirect to the list without warning + await screen.findByText('Post list'); + expect(confirmSpy).not.toHaveBeenCalled(); + confirmSpy.mockRestore(); + } + ); + }); }); diff --git a/packages/ra-core/src/form/Form.stories.tsx b/packages/ra-core/src/form/Form.stories.tsx index 46e79f87c6c..dcc1ba93d51 100644 --- a/packages/ra-core/src/form/Form.stories.tsx +++ b/packages/ra-core/src/form/Form.stories.tsx @@ -10,6 +10,7 @@ import { yupResolver } from '@hookform/resolvers/yup'; import * as yup from 'yup'; import polyglotI18nProvider from 'ra-i18n-polyglot'; import englishMessages from 'ra-language-english'; +import fakeRestDataProvider from 'ra-data-fakerest'; import { Route, Routes, @@ -20,7 +21,12 @@ import { } from 'react-router-dom'; import { CoreAdminContext } from '../core'; -import { RecordContextProvider, SaveContextProvider } from '../controller'; +import { + EditBase, + RecordContextProvider, + SaveContextProvider, +} from '../controller'; +import { DeleteButton } from '../test-ui/DeleteButton'; import { Form, FormProps } from './Form'; import { useInput } from './useInput'; import { required, ValidationError } from './validation'; @@ -415,6 +421,69 @@ export const InNonDataRouter = ({ ); +const PostEditWithDelete = ({ + mutationMode, +}: { + mutationMode: 'undoable' | 'optimistic' | 'pessimistic'; +}) => ( + +
+ + + + + +
+); + +/** + * Edit a field (making the form dirty), then click Delete. The record is + * deleted and the app redirects to the list, but the unsaved-changes blocker + * pops a spurious "unsaved changes" confirm dialog even though the record is + * already gone. + */ +export const WarnWhenUnsavedChangesWithDelete = ({ + i18nProvider = defaultI18nProvider, + mutationMode = 'undoable', +}: { + i18nProvider?: I18nProvider; + mutationMode?: 'undoable' | 'optimistic' | 'pessimistic'; +}) => ( + + + + } + /> + Post list

} /> + + + +); + +WarnWhenUnsavedChangesWithDelete.argTypes = { + mutationMode: { + options: ['undoable', 'optimistic', 'pessimistic'], + control: { type: 'select' }, + }, +}; + const Notifications = () => { const { notifications } = useNotificationContext(); return ( diff --git a/packages/ra-core/src/form/useWarnWhenUnsavedChanges.tsx b/packages/ra-core/src/form/useWarnWhenUnsavedChanges.tsx index 243cecdf977..deb117850ab 100644 --- a/packages/ra-core/src/form/useWarnWhenUnsavedChanges.tsx +++ b/packages/ra-core/src/form/useWarnWhenUnsavedChanges.tsx @@ -40,21 +40,24 @@ export const useWarnWhenUnsavedChanges = ( }); useEffect(() => { - if (blocker.state === 'blocked') { - // Corner case: the blocker might be triggered by a redirect in the onSuccess side effect, - // happening during the same tick the form is reset after a successful save. - // In that case, the blocker will block but shouldNotBlock will be true one tick after. - // If we are in that case, we can proceed immediately. - if (shouldNotBlock) { - blocker.proceed(); - return; - } + if (blocker.state !== 'blocked') return; - setShouldNotify(true); + // Proceed automatically when the form becomes safe to leave while the + // navigation is blocked. This covers two cases where the redirect and + // the form reset happen during the same tick (and the reset may only be + // reflected one tick after the block): + // - a successful save resets the form; + // - a successful delete resets the form (the record no longer exists, + // so the unsaved changes are moot). + if (shouldNotBlock) { + blocker.proceed && blocker.proceed(); + return; } - // This effect should only run when the blocker state changes, not when shouldNotBlock changes. + + setShouldNotify(true); + // Can't use blocker in the dependency array because it is not stable across rerenders // eslint-disable-next-line react-hooks/exhaustive-deps - }, [blocker.state]); + }, [blocker.state, shouldNotBlock]); useEffect(() => { if (shouldNotify) { diff --git a/packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.spec.tsx b/packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.spec.tsx index d4cd250adce..a40bc94e396 100644 --- a/packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.spec.tsx +++ b/packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.spec.tsx @@ -11,6 +11,7 @@ import { CoreAdminContext, MutationMode, testDataProvider, + TestMemoryRouter, useNotificationContext, } from 'ra-core'; import { createTheme, ThemeProvider } from '@mui/material/styles'; @@ -453,4 +454,109 @@ describe('', () => { const buttons = await screen.findAllByTestId('themed'); expect(buttons[0].classList).toContain('MuiButton-outlined'); }); + + describe('with warnWhenUnsavedChanges and a dirty form', () => { + // Use the router-agnostic TestMemoryRouter + locationCallback (instead of + // react-router's Routes) so the test does not depend on the router adapter. + const renderDirtyEditForm = ( + mutationMode: MutationMode, + locationCallback: (l: any) => void + ) => { + const dataProvider = testDataProvider({ + getOne: () => + // @ts-ignore + Promise.resolve({ data: { id: 123, title: 'lorem' } }), + delete: jest.fn().mockResolvedValue({ data: { id: 123 } }), + }); + const EditToolbar = props => ( + + + + ); + render( + + + + + } + > + + + + + + + ); + return dataProvider; + }; + + const makeFormDirty = async () => { + const input = + await screen.findByDisplayValue('lorem'); + fireEvent.change(input, { target: { value: 'lorem modified' } }); + fireEvent.blur(input); + }; + + it('should delete the record and redirect without warning when confirming the dialog', async () => { + // spy on "cancel": if the unsaved-changes dialog were shown, the + // navigation would be cancelled and the redirect assertion would fail + const confirmSpy = jest + .spyOn(window, 'confirm') + .mockReturnValue(false); + let location; + const dataProvider = renderDirtyEditForm('pessimistic', l => { + location = l; + }); + await makeFormDirty(); + // open the confirmation dialog and confirm + fireEvent.click( + await screen.findByLabelText('resources.posts.action.delete') + ); + fireEvent.click(screen.getByText('ra.action.confirm')); + // the record is deleted and the app redirects to the list + await waitFor(() => { + expect(location.pathname).toEqual('/posts'); + }); + expect(dataProvider.delete).toHaveBeenCalledWith('posts', { + id: 123, + previousData: { id: 123, title: 'lorem' }, + }); + // no spurious unsaved-changes warning + expect(confirmSpy).not.toHaveBeenCalled(); + confirmSpy.mockRestore(); + }); + + it('should not delete the record nor leave the form when cancelling the dialog (optimistic)', async () => { + const confirmSpy = jest + .spyOn(window, 'confirm') + .mockReturnValue(false); + let location; + const dataProvider = renderDirtyEditForm('optimistic', l => { + location = l; + }); + await makeFormDirty(); + // open the confirmation dialog and cancel + fireEvent.click( + await screen.findByLabelText('resources.posts.action.delete') + ); + fireEvent.click(screen.getByText('ra.action.cancel')); + // the dialog closes + await waitFor(() => { + expect(screen.queryByText('ra.action.confirm')).toBeNull(); + }); + // the deletion never happened (no optimistic removal to roll back) + expect(dataProvider.delete).not.toHaveBeenCalled(); + // the user stays on the dirty form, edits preserved + expect(location.pathname).toEqual('/posts/123'); + expect(screen.getByDisplayValue('lorem modified')).not.toBeNull(); + // no unsaved-changes warning was triggered either + expect(confirmSpy).not.toHaveBeenCalled(); + confirmSpy.mockRestore(); + }); + }); }); diff --git a/packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.tsx b/packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.tsx index 403c8ad2219..42551645a35 100644 --- a/packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.tsx +++ b/packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.tsx @@ -1,4 +1,5 @@ import React, { Fragment, isValidElement, ReactEventHandler } from 'react'; +import { useFormContext } from 'react-hook-form'; import ActionDelete from '@mui/icons-material/Delete'; import { ComponentsOverrides, @@ -59,6 +60,8 @@ export const DeleteWithConfirmButton = React.forwardRef( const notify = useNotify(); const unselect = useUnselect(resource); const redirect = useRedirect(); + // null when the delete button is used outside a form (e.g. in a list) + const form = useFormContext(); const [open, setOpen] = React.useState(false); if (!resource) { throw new Error( @@ -95,6 +98,10 @@ export const DeleteWithConfirmButton = React.forwardRef( } ); record && unselect([record.id]); + // Clear the form dirty state so warnWhenUnsavedChanges + // doesn't block the redirect: the record is gone, so the + // unsaved changes are moot. + form?.reset(undefined, { keepValues: true }); redirect(redirectTo, resource); } }, diff --git a/packages/ra-ui-materialui/src/button/DeleteWithUndoButton.spec.tsx b/packages/ra-ui-materialui/src/button/DeleteWithUndoButton.spec.tsx index 38d414fc6ac..f7061bd9762 100644 --- a/packages/ra-ui-materialui/src/button/DeleteWithUndoButton.spec.tsx +++ b/packages/ra-ui-materialui/src/button/DeleteWithUndoButton.spec.tsx @@ -5,6 +5,7 @@ import { MutationMode, CoreAdminContext, testDataProvider, + TestMemoryRouter, useNotificationContext, } from 'ra-core'; import { createTheme, ThemeProvider } from '@mui/material/styles'; @@ -182,4 +183,62 @@ describe('', () => { const buttons = await screen.findAllByTestId('themed'); expect(buttons[0].classList).toContain('MuiButton-outlined'); }); + + it('should not warn about unsaved changes after deleting a dirty record', async () => { + // spy on "cancel": if the unsaved-changes dialog were shown, the + // navigation would be cancelled and the redirect assertion would fail + const confirmSpy = jest.spyOn(window, 'confirm').mockReturnValue(false); + const dataProvider = testDataProvider({ + getOne: () => + // @ts-ignore + Promise.resolve({ data: { id: 123, title: 'lorem' } }), + delete: jest.fn().mockResolvedValue({ data: { id: 123 } }), + }); + const EditToolbar = props => ( + + + + ); + // Use the router-agnostic TestMemoryRouter + locationCallback (instead of + // react-router's Routes) so the test does not depend on the router adapter. + let location; + render( + { + location = l; + }} + > + + + + } + > + + + + + + + ); + // wait for the record to load + const input = + await screen.findByDisplayValue('lorem'); + // make the form dirty + fireEvent.change(input, { target: { value: 'lorem modified' } }); + fireEvent.blur(input); + // the undoable delete button has no confirmation dialog: clicking it + // deletes (optimistically) and redirects right away + fireEvent.click( + await screen.findByLabelText('resources.posts.action.delete') + ); + // the app should redirect to the list without warning + await waitFor(() => { + expect(location.pathname).toEqual('/posts'); + }); + expect(confirmSpy).not.toHaveBeenCalled(); + confirmSpy.mockRestore(); + }); });