Refactor state updates to EditorContext and fix bugs.

CHANGELOG.md

@@ -2,6 +2,28 @@
 
 All notable changes to this project will be documented in this file, in reverse chronological order by release.
 
+## Next Release - TBD
+
+### Added
+
+- Nothing.
+
+### Changed
+
+- Nothing.
+
+### Deprecated
+
+- Nothing.
+
+### Removed
+
+- Nothing.
+
+### Fixed
+
+- Changing the state of an object with mixed-state children no longer causes duplicate Fedora writes, and is no longer blocked if the desired state is the same as the existing top-level object state.
+
 ## 2.3 - 2024-07-24
 
 ### Added

api/src/routes/edit.test.ts

@@ -1049,6 +1049,20 @@ describe("edit", () => {
     });
 
     describe("put /object/:pid/state", () => {
+        let getObjectSpy;
+        let stateSpy;
+
+        beforeEach(() => {
+            const collector = FedoraDataCollector.getInstance();
+            getObjectSpy = jest.spyOn(collector, "getObjectData").mockImplementation(jest.fn());
+            const fedora = Fedora.getInstance();
+            stateSpy = jest.spyOn(fedora, "modifyObjectState").mockImplementation(jest.fn());
+        });
+
+        afterEach(() => {
+            jest.clearAllMocks();
+        });
+
         it("will reject invalid states", async () => {
             const response = await request(app)
                 .put(`/edit/object/${pid}/state`)
@@ -1057,21 +1071,52 @@ describe("edit", () => {
                 .send("Illegal")
                 .expect(StatusCodes.BAD_REQUEST);
             expect(response.error.text).toEqual("Illegal state: Illegal");
+            expect(getObjectSpy).not.toHaveBeenCalled();
+            expect(stateSpy).not.toHaveBeenCalled();
         });
 
         it("will accept a valid state", async () => {
-            const fedora = Fedora.getInstance();
-            const stateSpy = jest.spyOn(fedora, "modifyObjectState").mockImplementation(jest.fn());
-
+            getObjectSpy.mockResolvedValue(FedoraDataCollection.build(pid));
             await request(app)
                 .put(`/edit/object/${pid}/state`)
                 .set("Authorization", "Bearer test")
                 .set("Content-Type", "text/plain")
                 .send("Active")
                 .expect(StatusCodes.OK);
 
+            expect(getObjectSpy).toHaveBeenCalledWith(pid);
             expect(stateSpy).toHaveBeenCalledWith(pid, "Active");
         });
+
+        it("will handle unexpected errors", async () => {
+            const kaboom = new Error("kaboom");
+            getObjectSpy.mockImplementation(() => {
+                throw kaboom;
+            });
+            const consoleSpy = jest.spyOn(console, "error").mockImplementation(jest.fn());
+            const response = await request(app)
+                .put(`/edit/object/${pid}/state`)
+                .set("Authorization", "Bearer test")
+                .set("Content-Type", "text/plain")
+                .send("Active")
+                .expect(StatusCodes.INTERNAL_SERVER_ERROR);
+
+            expect(consoleSpy).toHaveBeenCalledWith(kaboom);
+            expect(response.text).toEqual("kaboom");
+        });
+
+        it("will not write an existing state to Fedora", async () => {
+            getObjectSpy.mockResolvedValue(FedoraDataCollection.build(pid, {}, { state: ["Active"] }));
+            await request(app)
+                .put(`/edit/object/${pid}/state`)
+                .set("Authorization", "Bearer test")
+                .set("Content-Type", "text/plain")
+                .send("Active")
+                .expect(StatusCodes.OK);
+
+            expect(getObjectSpy).toHaveBeenCalledWith(pid);
+            expect(stateSpy).not.toHaveBeenCalled();
+        });
     });
 
     describe("put /object/:pid/sortOn", () => {

api/src/routes/edit.ts

@@ -500,7 +500,11 @@ edit.put("/object/:pid/state", requireToken, pidSanitizer, bodyParser.text(), as
             res.status(400).send(`Illegal state: ${state}`);
             return;
         }
-        await fedora.modifyObjectState(pid, state);
+        // Only update state if it's different from the existing one:
+        const existing = await FedoraDataCollector.getInstance().getObjectData(pid);
+        if (existing.state !== state) {
+            await fedora.modifyObjectState(pid, state);
+        }
         res.status(200).send("ok");
     } catch (error) {
         console.error(error);

client/components/edit/StateModal.test.tsx

@@ -67,13 +67,12 @@ describe("StateModal", () => {
                 objectDetailsStorage: {},
             },
             action: {
-                removeFromObjectDetailsStorage: jest.fn(),
+                updateObjectState: jest.fn(),
             },
         };
         fetchContextValues = {
             action: {
                 fetchJSON: jest.fn(),
-                fetchText: jest.fn(),
             },
         };
         mockUseGlobalContext.mockReturnValue(globalValues);
@@ -131,8 +130,8 @@ describe("StateModal", () => {
 
     it("saves data correctly", async () => {
         editorValues.state.objectDetailsStorage[pid] = { pid, state: "Inactive" };
+        editorValues.action.updateObjectState.mockResolvedValue(["Status saved successfully.", "success"]);
         fetchContextValues.action.fetchJSON.mockResolvedValue({ numFound: 0 });
-        fetchContextValues.action.fetchText.mockResolvedValue("ok");
         await act(async () => {
             render(<StateModal />);
         });
@@ -142,24 +141,24 @@ describe("StateModal", () => {
         await user.click(screen.getByText("Active"));
         await user.click(screen.getByText("Save"));
         await waitFor(() =>
-            expect(fetchContextValues.action.fetchText).toHaveBeenCalledWith(
-                "http://localhost:9000/api/edit/object/foo%3A123/state",
-                { body: "Active", method: "PUT" },
+            expect(editorValues.action.updateObjectState).toHaveBeenCalledWith(
+                "foo:123",
+                "Active",
+                0,
+                expect.anything(),
             ),
         );
         expect(globalValues.action.setSnackbarState).toHaveBeenCalledWith({
             message: "Status saved successfully.",
             open: true,
             severity: "success",
         });
-        expect(editorValues.action.removeFromObjectDetailsStorage).toHaveBeenCalledWith(pid);
         expect(globalValues.action.closeModal).toHaveBeenCalledWith("state");
     });
 
     it("does not save when nothing changes", async () => {
         editorValues.state.objectDetailsStorage[pid] = { pid, state: "Inactive" };
         fetchContextValues.action.fetchJSON.mockResolvedValue({ numFound: 0 });
-        fetchContextValues.action.fetchText.mockResolvedValue("ok");
         await act(async () => {
             render(<StateModal />);
         });
@@ -173,70 +172,44 @@ describe("StateModal", () => {
                 severity: "info",
             }),
         );
-        expect(fetchContextValues.action.fetchText).not.toHaveBeenCalled();
-        expect(editorValues.action.removeFromObjectDetailsStorage).not.toHaveBeenCalled();
+        expect(editorValues.action.updateObjectState).not.toHaveBeenCalled();
         expect(globalValues.action.openModal).not.toHaveBeenCalled();
     });
 
     it("handles save failure gracefully", async () => {
         editorValues.state.objectDetailsStorage[pid] = { pid, state: "Inactive" };
         fetchContextValues.action.fetchJSON.mockResolvedValue({ numFound: 0 });
-        fetchContextValues.action.fetchText.mockResolvedValue("not ok");
-        await act(async () => {
-            render(<StateModal />);
-        });
-        await waitFor(() => expect(fetchContextValues.action.fetchJSON).toHaveBeenCalled());
-        expect(globalValues.action.isModalOpen).toHaveBeenCalledWith("state");
-        const user = userEvent.setup();
-        await user.click(screen.getByText("Active"));
-        await user.click(screen.getByText("Save"));
-        await waitFor(() =>
-            expect(fetchContextValues.action.fetchText).toHaveBeenCalledWith(
-                "http://localhost:9000/api/edit/object/foo%3A123/state",
-                { body: "Active", method: "PUT" },
-            ),
-        );
-        expect(globalValues.action.setSnackbarState).toHaveBeenCalledWith({
-            message: 'Status failed to save; "not ok"',
-            open: true,
-            severity: "error",
+        editorValues.action.updateObjectState.mockImplementation(() => {
+            throw new Error('Status failed to save; "not ok"');
         });
-        expect(editorValues.action.removeFromObjectDetailsStorage).not.toHaveBeenCalled();
-        expect(globalValues.action.closeModal).toHaveBeenCalledWith("state");
-    });
-
-    it("handles child save failure gracefully", async () => {
-        editorValues.state.objectDetailsStorage[pid] = { pid, state: "Inactive" };
-        fetchContextValues.action.fetchJSON.mockResolvedValue({ numFound: 1, docs: [{ id: "foo:125" }] });
-        fetchContextValues.action.fetchText.mockResolvedValue("not ok");
         await act(async () => {
             render(<StateModal />);
         });
         await waitFor(() => expect(fetchContextValues.action.fetchJSON).toHaveBeenCalled());
         expect(globalValues.action.isModalOpen).toHaveBeenCalledWith("state");
         const user = userEvent.setup();
         await user.click(screen.getByText("Active"));
-        await user.click(screen.getByText("Update 1 children to match"));
         await user.click(screen.getByText("Save"));
         await waitFor(() =>
-            expect(fetchContextValues.action.fetchText).toHaveBeenCalledWith(
-                "http://localhost:9000/api/edit/object/foo%3A125/state",
-                { body: "Active", method: "PUT" },
+            expect(editorValues.action.updateObjectState).toHaveBeenCalledWith(
+                "foo:123",
+                "Active",
+                0,
+                expect.anything(),
             ),
         );
         expect(globalValues.action.setSnackbarState).toHaveBeenCalledWith({
             message: 'Status failed to save; "not ok"',
             open: true,
             severity: "error",
         });
-        expect(editorValues.action.removeFromObjectDetailsStorage).not.toHaveBeenCalled();
         expect(globalValues.action.closeModal).toHaveBeenCalledWith("state");
     });
 
     it("updates children correctly", async () => {
         editorValues.state.objectDetailsStorage[pid] = { pid, state: "Inactive" };
         fetchContextValues.action.fetchJSON.mockResolvedValue({ numFound: 1, docs: [{ id: "foo:125" }] });
-        fetchContextValues.action.fetchText.mockResolvedValue("ok");
+        editorValues.action.updateObjectState.mockResolvedValue(["Status saved successfully.", "success"]);
         await act(async () => {
             render(<StateModal />);
         });
@@ -247,24 +220,18 @@ describe("StateModal", () => {
         await user.click(screen.getByText("Update 1 children to match"));
         await user.click(screen.getByText("Save"));
         await waitFor(() =>
-            expect(fetchContextValues.action.fetchText).toHaveBeenCalledWith(
-                "http://localhost:9000/api/edit/object/foo%3A123/state",
-                { body: "Active", method: "PUT" },
+            expect(editorValues.action.updateObjectState).toHaveBeenCalledWith(
+                "foo:123",
+                "Active",
+                1,
+                expect.anything(),
             ),
         );
         expect(globalValues.action.setSnackbarState).toHaveBeenCalledWith({
             message: "Status saved successfully.",
             open: true,
             severity: "success",
         });
-        expect(fetchContextValues.action.fetchText).toHaveBeenNthCalledWith(
-            1,
-            "http://localhost:9000/api/edit/object/foo%3A125/state",
-            { body: "Active", method: "PUT" },
-        );
-        expect(fetchContextValues.action.fetchText).toHaveBeenCalledTimes(2);
-        expect(editorValues.action.removeFromObjectDetailsStorage).toHaveBeenNthCalledWith(1, "foo:125");
-        expect(editorValues.action.removeFromObjectDetailsStorage).toHaveBeenNthCalledWith(2, pid);
         expect(globalValues.action.closeModal).toHaveBeenCalledWith("state");
     });
 });

client/components/edit/StateModal.tsx

@@ -13,7 +13,7 @@ import IconButton from "@mui/material/IconButton";
 import CloseIcon from "@mui/icons-material/Close";
 import { useGlobalContext } from "../../context/GlobalContext";
 import { useEditorContext } from "../../context/EditorContext";
-import { getObjectRecursiveChildPidsUrl, getObjectStateUrl } from "../../util/routes";
+import { getObjectRecursiveChildPidsUrl } from "../../util/routes";
 import { useFetchContext } from "../../context/FetchContext";
 import ObjectLoader from "./ObjectLoader";
 
@@ -23,10 +23,10 @@ const StateModal = (): React.ReactElement => {
     } = useGlobalContext();
     const {
         state: { objectDetailsStorage, stateModalActivePid },
-        action: { removeFromObjectDetailsStorage },
+        action: { updateObjectState },
     } = useEditorContext();
     const {
-        action: { fetchJSON, fetchText },
+        action: { fetchJSON },
     } = useFetchContext();
 
     function closeStateModal() {
@@ -39,12 +39,11 @@ const StateModal = (): React.ReactElement => {
     const [childPidResponse, setChildPidResponse] = useState({ loading: true });
     const loaded = Object.prototype.hasOwnProperty.call(objectDetailsStorage, stateModalActivePid);
     const details = loaded ? objectDetailsStorage[stateModalActivePid] : {};
-    const childPageSize = 1000;
     useEffect(() => {
         async function loadChildren() {
             setChildPidResponse({ loading: true });
             setIncludeChildren(false);
-            const url = getObjectRecursiveChildPidsUrl(details.pid, 0, childPageSize);
+            const url = getObjectRecursiveChildPidsUrl(details.pid, 0, 0);
             const response = await fetchJSON(url);
             setChildPidResponse(response);
         }
@@ -70,59 +69,20 @@ const StateModal = (): React.ReactElement => {
         });
     };
 
-    const updateStatus = async (pid: string, remaining: number = 0): Promise<string> => {
-        setStatusMessage(`Saving status for ${pid} (${remaining} more remaining)...`);
-        const target = getObjectStateUrl(pid);
-        const result = await fetchText(target, { method: "PUT", body: selectedValue });
-        if (result === "ok") {
-            // Clear and reload the cached object, since it has now changed!
-            removeFromObjectDetailsStorage(pid);
-        }
-        return result;
-    };
-
-    const saveChildPage = async (response, found: number, total: number): Promise<boolean> => {
-        for (let i = 0; i < response.docs.length; i++) {
-            const result = await updateStatus(response.docs[i].id, total - (found + i));
-            if (result !== "ok") {
-                showSnackbarMessage(`Status failed to save; "${result}"`, "error");
-                closeStateModal();
-                setStatusMessage("");
-                return false;
-            }
-        }
-        return true;
-    };
-
-    const saveChildren = async (): Promise<boolean> => {
-        const expectedTotal = childPidResponse.numFound;
-        let found = 0;
-        let nextResponse = childPidResponse;
-        while (found < expectedTotal) {
-            if (!(await saveChildPage(nextResponse, found, expectedTotal))) {
-                return false;
-            }
-            found += nextResponse.docs.length;
-            if (found < expectedTotal) {
-                const url = getObjectRecursiveChildPidsUrl(details.pid, found, childPageSize);
-                nextResponse = await fetchJSON(url);
-            }
-        }
-        return true;
-    };
-
     const save = async () => {
-        if (selectedValue !== details.state) {
-            if (includeChildren) {
-                if (!(await saveChildren())) {
-                    return;
-                }
-            }
-            const result = await updateStatus(stateModalActivePid);
-            if (result === "ok") {
-                showSnackbarMessage("Status saved successfully.", "success");
-            } else {
-                showSnackbarMessage(`Status failed to save; "${result}"`, "error");
+        // Don't allow the user to set the state to the existing state (unless)
+        // children are involved, since it may be necessary to update mixed-status items.
+        if (selectedValue !== details.state || includeChildren) {
+            try {
+                const result = await updateObjectState(
+                    stateModalActivePid,
+                    selectedValue,
+                    includeChildren ? childPidResponse.numFound : 0,
+                    setStatusMessage,
+                );
+                showSnackbarMessage(...result);
+            } catch (e) {
+                showSnackbarMessage(e.message, "error");
             }
             closeStateModal();
             setStatusMessage("");