Fix navigation warnings on autotest manager and starter file manager pages, closes 7641

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)

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 <form>{props.children}</form>;
+  });
+  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}) => <input id={id} type="text" readOnly value={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(<AutotestManager course_id={1} assignment_id={1} validator={{}} />);
+}
+
+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();
+  });
+});

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(<StarterFileManager course_id={1} assignment_id={1} read_only={false} />);
+}
+
+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();
+  });
+});

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);

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 => {

app/views/assignments/starter_file.html.erb

@@ -11,10 +11,6 @@
       );
     });
   <% end %>
-
-  <%= render partial: 'shared/navigation_warning',
-             formats: [:js],
-             handlers: [:erb] %>
 <% end %>
 
 <div id='starter_file_manager'></div>

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 %>
 
 <div id='autotest-manager'></div>