From d82ab7a16f89d9113584e998b15ab6c252589f34 Mon Sep 17 00:00:00 2001 From: philipkukulak Date: Fri, 29 May 2026 11:57:28 -0400 Subject: [PATCH] Fix navigation warning on autotest manager and starter files manager pages, closes 7641 --- Changelog.md | 1 + .../__tests__/autotest_manager.test.jsx | 114 ++++++++++++++++++ .../__tests__/starter_file_manager.test.jsx | 59 ++++++++- .../Components/autotest_manager.jsx | 21 +++- .../Components/starter_file_manager.jsx | 10 ++ app/views/assignments/starter_file.html.erb | 4 - app/views/automated_tests/_form.html.erb | 3 - 7 files changed, 201 insertions(+), 11 deletions(-) create mode 100644 app/javascript/Components/__tests__/autotest_manager.test.jsx diff --git a/Changelog.md b/Changelog.md index d720663548..14534475b5 100644 --- a/Changelog.md +++ b/Changelog.md @@ -45,6 +45,7 @@ - Fixed filter Canvas Test Student from roster sync (#7926) - Fix: include original total mark in JSON response for remark requests (#7945) - Fixed `(hidden)` assignment labeling for assignments with `visible_on` and/or `visible_until` set (#7944) +- Fixed spurious navigation warnings on autotest manager and starter file manager pages (#7968) ### 🔧 Internal changes - Fixed flaky test `can bulk assign duplicated TAs to grade entry students` in `/spec/models/grade_entry_student_spec.rb` (#7958) diff --git a/app/javascript/Components/__tests__/autotest_manager.test.jsx b/app/javascript/Components/__tests__/autotest_manager.test.jsx new file mode 100644 index 0000000000..f1cfbd2a3c --- /dev/null +++ b/app/javascript/Components/__tests__/autotest_manager.test.jsx @@ -0,0 +1,114 @@ +import React from "react"; +import {render, screen, fireEvent, act} from "@testing-library/react"; +import {AutotestManager} from "../autotest_manager"; + +jest.mock("@fortawesome/react-fontawesome", () => ({ + FontAwesomeIcon: () => null, +})); + +let mockFormOnChange; +jest.mock("@rjsf/core", () => { + const React = require("react"); + const MockForm = React.forwardRef((props, ref) => { + mockFormOnChange = props.onChange; + React.useImperativeHandle(ref, () => ({validateForm: () => true})); + return
{props.children}
; + }); + MockForm.displayName = "MockForm"; + return {__esModule: true, default: MockForm}; +}); + +jest.mock("@rjsf/utils", () => ({TranslatableString: {}})); +jest.mock("@rjsf/validator-ajv8", () => ({customizeValidator: () => ({})})); + +jest.mock("react-flatpickr", () => { + const React = require("react"); + const MockFlatpickr = ({id, value}) => ; + MockFlatpickr.displayName = "MockFlatpickr"; + return {__esModule: true, default: MockFlatpickr}; +}); + +jest.mock("flatpickr/dist/plugins/labelPlugin/labelPlugin", () => () => ({})); + +jest.mock("../markus_file_manager", () => ({__esModule: true, default: () => null})); +jest.mock("../Modals/file_upload_modal", () => ({__esModule: true, default: () => null})); +jest.mock("../Modals/autotest_specs_upload_modal", () => ({__esModule: true, default: () => null})); +jest.mock("../../common/flash", () => ({flashMessage: jest.fn()})); + +const fetchPayload = { + files: [], + schema: {}, + formData: {}, + enable_test: true, + enable_student_tests: true, + token_start_date: "", + token_end_date: "", + tokens_per_period: 0, + token_period: 0, + non_regenerating_tokens: false, + unlimited_tokens: false, +}; + +function renderManager() { + return render(); +} + +describe("AutotestManager navigation warning", () => { + beforeEach(() => { + fetch.mockReset(); + fetch.mockResolvedValue({ok: true, json: jest.fn().mockResolvedValue(fetchPayload)}); + }); + + afterEach(() => { + window.onbeforeunload = null; + }); + + it("does not warn when no changes have been made", () => { + renderManager(); + expect(window.onbeforeunload()).toBeUndefined(); + }); + + it("warns after a field is changed", () => { + renderManager(); + fireEvent.click(screen.getAllByRole("checkbox")[0]); + expect(window.onbeforeunload()).toBe(I18n.t("uncommitted_changes_warning")); + }); + + it("clears the warning after form is saved", async () => { + renderManager(); + fireEvent.click(screen.getAllByRole("checkbox")[0]); + expect(window.onbeforeunload()).toBe(I18n.t("uncommitted_changes_warning")); + + fetch.mockResolvedValueOnce({ok: true, json: jest.fn().mockResolvedValue({job_id: "1"})}); + await act(async () => { + fireEvent.click(screen.getByRole("button", {name: I18n.t("save")})); + }); + expect(window.onbeforeunload()).toBeUndefined(); + }); + + it("does not mark dirty when rjsf fires onChange before fetch resolves", () => { + renderManager(); + // formData is null until fetch resolves, so onChange is suppressed + act(() => { + mockFormOnChange({formData: {}}); + }); + expect(window.onbeforeunload()).toBeUndefined(); + }); + + it("marks dirty when rjsf fires onChange after fetch resolves", async () => { + renderManager(); + await act(async () => {}); // flush fetch promise, formData is now non-null + + act(() => { + mockFormOnChange({formData: {testers: []}}); + }); + expect(window.onbeforeunload()).toBe(I18n.t("uncommitted_changes_warning")); + }); + + it("restores window.onbeforeunload to null on unmount", () => { + const {unmount} = renderManager(); + expect(window.onbeforeunload).not.toBeNull(); + unmount(); + expect(window.onbeforeunload).toBeNull(); + }); +}); diff --git a/app/javascript/Components/__tests__/starter_file_manager.test.jsx b/app/javascript/Components/__tests__/starter_file_manager.test.jsx index 4e381955e0..6a38b6ca7a 100644 --- a/app/javascript/Components/__tests__/starter_file_manager.test.jsx +++ b/app/javascript/Components/__tests__/starter_file_manager.test.jsx @@ -1,5 +1,5 @@ import {StarterFileManager} from "../starter_file_manager"; -import {render, screen} from "@testing-library/react"; +import {render, screen, fireEvent, act} from "@testing-library/react"; import React from "react"; @@ -129,3 +129,60 @@ jest.mock("@fortawesome/react-fontawesome", () => ({ }); }); }); + +const minimalFetchPayload = { + files: [], + sections: {}, + available_after_due: true, + starterfileType: "simple", + defaultStarterFileGroup: "", +}; + +function renderManager() { + return render(); +} + +describe("StarterFileManager navigation warning", () => { + beforeEach(() => { + fetch.resetMocks(); + fetch.mockResponseOnce(JSON.stringify(minimalFetchPayload)); + }); + + afterEach(() => { + window.onbeforeunload = null; + }); + + it("does not warn when no changes have been made", () => { + renderManager(); + expect(window.onbeforeunload()).toBeUndefined(); + }); + + it("warns after a field is changed", async () => { + renderManager(); + await screen.findByTestId("available_after_due_checkbox"); + fireEvent.click(screen.getByTestId("available_after_due_checkbox")); + expect(window.onbeforeunload()).toBe(I18n.t("uncommitted_changes_warning")); + }); + + it("clears the warning after form is saved", async () => { + renderManager(); + await screen.findByTestId("available_after_due_checkbox"); + fireEvent.click(screen.getByTestId("available_after_due_checkbox")); + expect(window.onbeforeunload()).toBe(I18n.t("uncommitted_changes_warning")); + + jest.spyOn($, "ajax").mockResolvedValue({}); + fetch.resetMocks(); + fetch.mockResponseOnce(JSON.stringify(minimalFetchPayload)); + await act(async () => { + fireEvent.click(screen.getByTestId("save_button")); + }); + expect(window.onbeforeunload()).toBeUndefined(); + }); + + it("restores window.onbeforeunload to null on unmount", () => { + const {unmount} = renderManager(); + expect(window.onbeforeunload).not.toBeNull(); + unmount(); + expect(window.onbeforeunload).toBeNull(); + }); +}); diff --git a/app/javascript/Components/autotest_manager.jsx b/app/javascript/Components/autotest_manager.jsx index 98dea8786d..f279bff7e6 100644 --- a/app/javascript/Components/autotest_manager.jsx +++ b/app/javascript/Components/autotest_manager.jsx @@ -77,7 +77,7 @@ class AutotestManager extends React.Component { }, }, }, - formData: {}, + formData: null, enable_test: true, enable_student_tests: true, token_start_date: "", @@ -98,6 +98,16 @@ class AutotestManager extends React.Component { componentDidMount() { this.fetchData(); + // takes over from _navigation_warning.js.erb to use React state as the source of truth + window.onbeforeunload = () => { + if (this.state.form_changed) { + return I18n.t("uncommitted_changes_warning"); + } + }; + } + + componentWillUnmount() { + window.onbeforeunload = null; } fetchData = () => { @@ -205,7 +215,10 @@ class AutotestManager extends React.Component { }; handleFormChange = data => { - this.setState({formData: data.formData}, () => this.toggleFormChanged(true)); + this.setState({formData: data.formData}); + if (this.state.formData !== null) { + this.toggleFormChanged(true); + } }; toggleEnableTest = () => { @@ -488,7 +501,7 @@ class AutotestManager extends React.Component { disabled={!this.state.enable_test} schema={this.state.schema} uiSchema={this.state.uiSchema} - formData={this.state.formData} + formData={this.state.formData ?? {}} onChange={this.handleFormChange} validator={this.props.validator} templates={{ @@ -626,6 +639,8 @@ function MoveUpButton(props) { ); } +export {AutotestManager}; + export function makeAutotestManager(elem, props) { const validator = customizeValidator({ajvOptionsOverrides: {discriminator: true}}); const root = createRoot(elem); diff --git a/app/javascript/Components/starter_file_manager.jsx b/app/javascript/Components/starter_file_manager.jsx index 17e39618c7..0c1136b2f9 100644 --- a/app/javascript/Components/starter_file_manager.jsx +++ b/app/javascript/Components/starter_file_manager.jsx @@ -31,6 +31,16 @@ class StarterFileManager extends React.Component { componentDidMount() { this.fetchData(); + // takes over from _navigation_warning.js.erb to use React state as the source of truth + window.onbeforeunload = () => { + if (this.state.form_changed) { + return I18n.t("uncommitted_changes_warning"); + } + }; + } + + componentWillUnmount() { + window.onbeforeunload = null; } toggleFormChanged = value => { diff --git a/app/views/assignments/starter_file.html.erb b/app/views/assignments/starter_file.html.erb index 1e558801b0..ceae9272d4 100644 --- a/app/views/assignments/starter_file.html.erb +++ b/app/views/assignments/starter_file.html.erb @@ -11,10 +11,6 @@ ); }); <% end %> - - <%= render partial: 'shared/navigation_warning', - formats: [:js], - handlers: [:erb] %> <% end %>
diff --git a/app/views/automated_tests/_form.html.erb b/app/views/automated_tests/_form.html.erb index 43e183ae9d..f595b44841 100644 --- a/app/views/automated_tests/_form.html.erb +++ b/app/views/automated_tests/_form.html.erb @@ -5,9 +5,6 @@ { course_id: <%= @current_course.id %>, assignment_id: <%= @assignment.id %>}); }); <% end %> - <%= render partial: 'shared/navigation_warning', - formats: [:js], - handlers: [:erb] %> <% end %>