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

feat: handle unsaved changes in text & problem editors #1444

Merged
merged 6 commits into from
Nov 4, 2024
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
22 changes: 14 additions & 8 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@
"react-onclickoutside": "^6.13.0",
"react-redux": "7.2.9",
"react-responsive": "9.0.2",
"react-router": "6.23.1",
"react-router-dom": "6.23.1",
"react-router": "6.27.0",
"react-router-dom": "6.27.0",
Comment on lines +96 to +97
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intended?
Not a problem, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was checking if useBlocker is fixed in the latest version and then left it as is.

"react-select": "5.8.0",
"react-textarea-autosize": "^8.5.3",
"react-transition-group": "4.4.5",
Expand Down
10 changes: 7 additions & 3 deletions src/course-unit/course-xblock/CourseXBlock.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
} from '@openedx/paragon';
import { EditOutline as EditIcon, MoreVert as MoveVertIcon } from '@openedx/paragon/icons';
import { useIntl } from '@edx/frontend-platform/i18n';
import { useNavigate, useSearchParams } from 'react-router-dom';
import { useSearchParams } from 'react-router-dom';

import { getCanEdit, getCourseId } from 'CourseAuthoring/course-unit/data/selectors';
import DeleteModal from '../../generic/delete-modal/DeleteModal';
Expand All @@ -19,6 +19,7 @@ import { copyToClipboard } from '../../generic/data/thunks';
import { COMPONENT_TYPES } from '../../generic/block-type-utils/constants';
import XBlockMessages from './xblock-messages/XBlockMessages';
import messages from './messages';
import { createCorrectInternalRoute } from '../../utils';

const CourseXBlock = ({
id, title, type, unitXBlockActions, shouldScroll, userPartitionInfo,
Expand All @@ -28,7 +29,6 @@ const CourseXBlock = ({
const [isDeleteModalOpen, openDeleteModal, closeDeleteModal] = useToggle(false);
const [isConfigureModalOpen, openConfigureModal, closeConfigureModal] = useToggle(false);
const dispatch = useDispatch();
const navigate = useNavigate();
const canEdit = useSelector(getCanEdit);
const courseId = useSelector(getCourseId);
const intl = useIntl();
Expand Down Expand Up @@ -58,7 +58,11 @@ const CourseXBlock = ({
case COMPONENT_TYPES.html:
case COMPONENT_TYPES.problem:
case COMPONENT_TYPES.video:
navigate(`/course/${courseId}/editor/${type}/${id}`);
// Not using useNavigate from react router to use browser navigation
// which allows us to block back button if unsaved changes in editor are present.
window.location.assign(
createCorrectInternalRoute(`/course/${courseId}/editor/${type}/${id}`),
);
Comment on lines +61 to +65
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the part that replaces use of useNavigate from react-router to normal browser navigation.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be possible to implement with react-router if you want, though: https://reactrouter.com/en/main/hooks/use-blocker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradenmacdonald I did try it out but it has some issues as mentioned in the description:

React router has support for blocking navigation when required but it seems to refresh the page regardless of user clicking cancel or stay which keeps the user in the page but the changes are lost. So I had to use browser navigation to take user to editors even from new MFE unit page which can then make use of beforeunload event to block navigation when required. Not using react router to navigate to editor results in slow navigation although it is the default way of going back to unit page on clicking cancel button.

@rpenido also tried it out with same results.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry I missed that. Thanks for investigating it.

break;
default:
}
Expand Down
28 changes: 16 additions & 12 deletions src/course-unit/course-xblock/CourseXBlock.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ const blockId = '567890';
const handleDeleteMock = jest.fn();
const handleDuplicateMock = jest.fn();
const handleConfigureSubmitMock = jest.fn();
const mockedUsedNavigate = jest.fn();
const {
name,
block_id: id,
Expand All @@ -42,11 +41,6 @@ const unitXBlockActionsMock = {
handleDuplicate: handleDuplicateMock,
};

jest.mock('react-router-dom', () => ({
...jest.requireActual('react-router-dom'),
useNavigate: () => mockedUsedNavigate,
}));

jest.mock('react-redux', () => ({
...jest.requireActual('react-redux'),
useSelector: jest.fn(),
Expand Down Expand Up @@ -78,6 +72,16 @@ useSelector.mockImplementation((selector) => {
});

describe('<CourseXBlock />', () => {
const locationTemp = window.location;
beforeAll(() => {
delete window.location;
window.location = {
assign: jest.fn(),
};
});
afterAll(() => {
window.location = locationTemp;
});
beforeEach(async () => {
initializeMockApp({
authenticatedUser: {
Expand Down Expand Up @@ -168,8 +172,8 @@ describe('<CourseXBlock />', () => {
expect(editButton).toBeInTheDocument();

userEvent.click(editButton);
expect(mockedUsedNavigate).toHaveBeenCalled();
expect(mockedUsedNavigate).toHaveBeenCalledWith(`/course/${courseId}/editor/html/${id}`);
expect(window.location.assign).toHaveBeenCalled();
expect(window.location.assign).toHaveBeenCalledWith(`/course/${courseId}/editor/html/${id}`);
});

it('navigates to editor page on edit Video xblock', () => {
Expand All @@ -182,8 +186,8 @@ describe('<CourseXBlock />', () => {
expect(editButton).toBeInTheDocument();

userEvent.click(editButton);
expect(mockedUsedNavigate).toHaveBeenCalled();
expect(mockedUsedNavigate).toHaveBeenCalledWith(`/course/${courseId}/editor/video/${id}`);
expect(window.location.assign).toHaveBeenCalled();
expect(window.location.assign).toHaveBeenCalledWith(`/course/${courseId}/editor/video/${id}`);
});

it('navigates to editor page on edit Problem xblock', () => {
Expand All @@ -196,8 +200,8 @@ describe('<CourseXBlock />', () => {
expect(editButton).toBeInTheDocument();

userEvent.click(editButton);
expect(mockedUsedNavigate).toHaveBeenCalled();
expect(mockedUsedNavigate).toHaveBeenCalledWith(`/course/${courseId}/editor/problem/${id}`);
expect(window.location.assign).toHaveBeenCalled();
expect(window.location.assign).toHaveBeenCalledWith(`/course/${courseId}/editor/problem/${id}`);
expect(handleDeleteMock).toHaveBeenCalledWith(id);
});
});
Expand Down
73 changes: 72 additions & 1 deletion src/editors/containers/EditorContainer/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ jest.spyOn(editorCmsApi, 'fetchByUnitId').mockImplementation(async () => ({
},
}));

const isDirtyMock = jest.fn();
jest.mock('../TextEditor/hooks', () => ({
...jest.requireActual('../TextEditor/hooks'),
isDirty: () => isDirtyMock,
}));

const defaultPropsHtml = {
blockId: 'block-v1:Org+TS100+24+type@html+block@123456html',
blockType: 'html',
Expand All @@ -45,15 +51,27 @@ const fieldsHtml = {
};

describe('EditorContainer', () => {
let mockEvent: Event;

beforeEach(() => {
initializeMocks();
mockEvent = new Event('beforeunload');
jest.spyOn(window, 'addEventListener');
jest.spyOn(window, 'removeEventListener');
jest.spyOn(mockEvent, 'preventDefault');
Object.defineProperty(mockEvent, 'returnValue', { writable: true });
});

afterEach(() => {
jest.restoreAllMocks();
});

test('it displays a confirmation dialog when closing the editor modal', async () => {
test('it displays a confirmation dialog when closing the editor modal if data is changed', async () => {
jest.spyOn(editorCmsApi, 'fetchBlockById').mockImplementationOnce(async () => (
{ status: 200, data: snakeCaseObject(fieldsHtml) }
));

isDirtyMock.mockReturnValue(true);
render(<EditorPage {...defaultPropsHtml} />);

// Then the editor should open
Expand All @@ -68,12 +86,48 @@ describe('EditorContainer', () => {
fireEvent.click(closeButton);
// Now we should see the confirmation message:
expect(await screen.findByText(confirmMessage)).toBeInTheDocument();
expect(defaultPropsHtml.onClose).not.toHaveBeenCalled();

// Should close modal if cancelled
const cancelBtn = await screen.findByRole('button', { name: 'Cancel' });
fireEvent.click(cancelBtn);
expect(defaultPropsHtml.onClose).not.toHaveBeenCalled();

// open modal again
fireEvent.click(closeButton);
// And can confirm the cancelation:
const confirmButton = await screen.findByRole('button', { name: 'OK' });
fireEvent.click(confirmButton);
expect(defaultPropsHtml.onClose).toHaveBeenCalled();
window.dispatchEvent(mockEvent);
// should not be blocked by beforeunload event as the page was unloaded using close/cancel option
expect(window.removeEventListener).toHaveBeenCalledWith('beforeunload', expect.any(Function));
expect(mockEvent.preventDefault).not.toHaveBeenCalled();
});

test('it does not display any confirmation dialog when closing the editor modal if data is not changed', async () => {
jest.spyOn(editorCmsApi, 'fetchBlockById').mockImplementationOnce(async () => (
{ status: 200, data: snakeCaseObject(fieldsHtml) }
));

isDirtyMock.mockReturnValue(false);
render(<EditorPage {...defaultPropsHtml} />);

// Then the editor should open
expect(await screen.findByRole('heading', { name: /Introduction to Testing/ })).toBeInTheDocument();

// Assert the "are you sure?" message isn't visible yet
const confirmMessage = /Are you sure you want to exit the editor/;
expect(screen.queryByText(confirmMessage)).not.toBeInTheDocument();

// Find and click the close button
const closeButton = await screen.findByRole('button', { name: 'Exit the editor' });
fireEvent.click(closeButton);
// Even now we should not see the confirmation message as data is not dirty, i.e. not changed:
expect(screen.queryByText(confirmMessage)).not.toBeInTheDocument();

// And onClose is directly called
expect(defaultPropsHtml.onClose).toHaveBeenCalled();
});

test('it disables the save button until the fields have been loaded', async () => {
Expand All @@ -94,4 +148,21 @@ describe('EditorContainer', () => {
// Now the save button should be active:
await waitFor(() => expect(saveButton).not.toBeDisabled());
});

test('beforeunload event is triggered on page unload if data is changed', async () => {
jest.spyOn(editorCmsApi, 'fetchBlockById').mockImplementationOnce(async () => (
{ status: 200, data: snakeCaseObject(fieldsHtml) }
));

isDirtyMock.mockReturnValue(true);
render(<EditorPage {...defaultPropsHtml} />);

// Then the editor should open
expect(await screen.findByRole('heading', { name: /Introduction to Testing/ })).toBeInTheDocument();
// on beforeunload event block user
window.dispatchEvent(mockEvent);
expect(window.removeEventListener).toHaveBeenCalledWith('beforeunload', expect.any(Function));
expect(mockEvent.preventDefault).toHaveBeenCalled();
expect(mockEvent.returnValue).toBe(true);
});
});
38 changes: 33 additions & 5 deletions src/editors/containers/EditorContainer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import TitleHeader from './components/TitleHeader';
import * as hooks from './hooks';
import messages from './messages';
import './index.scss';
import usePromptIfDirty from '../../../generic/promptIfDirty/usePromptIfDirty';

interface WrapperProps {
children: React.ReactNode;
Expand Down Expand Up @@ -61,32 +62,57 @@ export const FooterWrapper: React.FC<WrapperProps> = ({ children }) => {
interface Props extends EditorComponent {
children: React.ReactNode;
getContent: Function;
isDirty: () => boolean;
validateEntry?: Function | null;
}

const EditorContainer: React.FC<Props> = ({
children,
getContent,
isDirty,
onClose = null,
validateEntry = null,
returnFunction = null,
}) => {
const intl = useIntl();
const dispatch = useDispatch();
// Required to mark data as not dirty on save
const [saved, setSaved] = React.useState(false);
const isInitialized = hooks.isInitialized();
const { isCancelConfirmOpen, openCancelConfirmModal, closeCancelConfirmModal } = hooks.cancelConfirmModalToggle();
const handleCancel = hooks.handleCancel({ onClose, returnFunction });
const disableSave = !isInitialized;
const saveFailed = hooks.saveFailed();
const clearSaveFailed = hooks.clearSaveError({ dispatch });
const onSave = hooks.handleSaveClicked({
const handleSave = hooks.handleSaveClicked({
dispatch,
getContent,
validateEntry,
returnFunction,
});

const onSave = () => {
setSaved(true);
handleSave();
};
// Stops user from navigating away if they have unsaved changes.
usePromptIfDirty(() => {
// Do not block if cancel modal is used or data is saved.
if (isCancelConfirmOpen || saved) {
return false;
}
return isDirty();
});

const confirmCancelIfDirty = () => {
if (isDirty()) {
openCancelConfirmModal();
} else {
handleCancel();
}
};
return (
<EditorModalWrapper onClose={openCancelConfirmModal}>
<EditorModalWrapper onClose={confirmCancelIfDirty}>
{saveFailed && (
<Toast show onClose={clearSaveFailed}>
<FormattedMessage {...messages.contentSaveFailed} />
Expand All @@ -108,7 +134,9 @@ const EditorContainer: React.FC<Props> = ({
</Button>
)}
isOpen={isCancelConfirmOpen}
close={closeCancelConfirmModal}
close={() => {
closeCancelConfirmModal();
}}
title={intl.formatMessage(messages.cancelConfirmTitle)}
>
<FormattedMessage {...messages.cancelConfirmDescription} />
Expand All @@ -121,7 +149,7 @@ const EditorContainer: React.FC<Props> = ({
<IconButton
src={Close}
iconAs={Icon}
onClick={openCancelConfirmModal}
onClick={confirmCancelIfDirty}
alt={intl.formatMessage(messages.exitButtonAlt)}
/>
</div>
Expand All @@ -135,7 +163,7 @@ const EditorContainer: React.FC<Props> = ({
<Button
aria-label={intl.formatMessage(messages.cancelButtonAriaLabel)}
variant="tertiary"
onClick={openCancelConfirmModal}
onClick={confirmCancelIfDirty}
>
<FormattedMessage {...messages.cancelButtonLabel} />
</Button>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
exports[`EditorProblemView component renders raw editor 1`] = `
<EditorContainer
getContent={[Function]}
isDirty={[Function]}
returnFunction={null}
>
<AlertModal
Expand Down Expand Up @@ -72,6 +73,7 @@ exports[`EditorProblemView component renders raw editor 1`] = `
exports[`EditorProblemView component renders simple view 1`] = `
<EditorContainer
getContent={[Function]}
isDirty={[Function]}
returnFunction={null}
>
<AlertModal
Expand Down
Loading