diff --git a/CHANGELOG.md b/CHANGELOG.md index b67f51eb..ec1c197d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/api/src/routes/edit.test.ts b/api/src/routes/edit.test.ts index 4cc62aaf..9631cf71 100644 --- a/api/src/routes/edit.test.ts +++ b/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,12 +1071,12 @@ 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") @@ -1070,8 +1084,39 @@ describe("edit", () => { .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", () => { diff --git a/api/src/routes/edit.ts b/api/src/routes/edit.ts index fb4d90bc..7d081ef7 100644 --- a/api/src/routes/edit.ts +++ b/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); diff --git a/client/components/edit/StateModal.test.tsx b/client/components/edit/StateModal.test.tsx index 8d7aadc7..75719efe 100644 --- a/client/components/edit/StateModal.test.tsx +++ b/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(); }); @@ -142,9 +141,11 @@ 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({ @@ -152,14 +153,12 @@ describe("StateModal", () => { 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(); }); @@ -173,42 +172,16 @@ 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(); - }); - 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(); }); @@ -216,12 +189,13 @@ describe("StateModal", () => { 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({ @@ -229,14 +203,13 @@ describe("StateModal", () => { 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(); }); @@ -247,9 +220,11 @@ 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({ @@ -257,14 +232,6 @@ describe("StateModal", () => { 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"); }); }); diff --git a/client/components/edit/StateModal.tsx b/client/components/edit/StateModal.tsx index 2848ce7e..9e130f9c 100644 --- a/client/components/edit/StateModal.tsx +++ b/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 => { - 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 => { - 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 => { - 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(""); diff --git a/client/context/EditorContext.test.tsx b/client/context/EditorContext.test.tsx index bf167dd7..c73b31b5 100644 --- a/client/context/EditorContext.test.tsx +++ b/client/context/EditorContext.test.tsx @@ -14,7 +14,8 @@ describe("useEditorContext", () => { beforeEach(() => { fetchValues = { action: { - fetchJSON: jest.fn() + fetchJSON: jest.fn(), + fetchText: jest.fn(), } }; mockUseFetchContext.mockReturnValue( @@ -393,7 +394,7 @@ describe("useEditorContext", () => { }); expect(Object.keys(result.current.state.parentDetailsStorage)).toEqual(["test:123", "test:125"]); }); - + it("handles errors", async () => { const { result } = await renderHook(() => useEditorContext(), { wrapper: EditorContextProvider }); const callback = jest.fn(); @@ -423,4 +424,80 @@ describe("useEditorContext", () => { expect(Object.keys(result.current.state.parentDetailsStorage)).toEqual(["test:123", "test:125"]); }); }); + + describe("updateObjectState", () => { + const pid = "test:123"; + + it("saves data correctly", async () => { + const { result } = await renderHook(() => useEditorContext(), { wrapper: EditorContextProvider }); + // Load an object into storage so we can test that it gets cleared out after updates: + const fakeObjectDetails = { foo: "bar" }; + fetchValues.action.fetchJSON.mockResolvedValue(fakeObjectDetails); + await act(async() => { + await result.current.action.loadObjectDetailsIntoStorage(pid); + }); + expect(result.current.state.objectDetailsStorage[pid]).toEqual(fakeObjectDetails); + const statusCallback = jest.fn(); + fetchValues.action.fetchText.mockResolvedValue("ok"); + let updateResult; + await act(async () => { + updateResult = await result.current.action.updateObjectState(pid, "Active", 0, statusCallback); + }); + expect(statusCallback).toHaveBeenCalledWith("Saving status for test:123 (0 more remaining)..."); + expect(fetchValues.action.fetchText).toHaveBeenCalledWith("http://localhost:9000/api/edit/object/test%3A123/state", {"body": "Active", "method": "PUT"}); + expect(updateResult).toEqual(["Status saved successfully.", "success"]); + // Object storage should now be empty due to clearing of changed data: + expect(result.current.state.objectDetailsStorage).toEqual({}); + }); + + it("handles save failure gracefully", async () => { + const { result } = await renderHook(() => useEditorContext(), { wrapper: EditorContextProvider }); + const statusCallback = jest.fn(); + fetchValues.action.fetchText.mockResolvedValue("not ok"); + let updateResult; + await act(async () => { + updateResult = await result.current.action.updateObjectState(pid, "Active", 0, statusCallback); + }); + expect(statusCallback).toHaveBeenCalledWith("Saving status for test:123 (0 more remaining)..."); + expect(fetchValues.action.fetchText).toHaveBeenCalledWith("http://localhost:9000/api/edit/object/test%3A123/state", {"body": "Active", "method": "PUT"}); + expect(updateResult).toEqual(["Status failed to save; \"not ok\"", "error"]); + }); + + it("handles child save failure gracefully", async () => { + fetchValues.action.fetchJSON.mockResolvedValue({ numFound: 1, docs: [{ id: "foo:125" }] }); + fetchValues.action.fetchText.mockResolvedValue("not ok"); + const { result } = await renderHook(() => useEditorContext(), { wrapper: EditorContextProvider }); + const statusCallback = jest.fn(); + fetchValues.action.fetchText.mockResolvedValue("not ok"); + let updateResult; + await act(async () => { + try { + await result.current.action.updateObjectState(pid, "Active", 1, statusCallback); + } catch (e) { + updateResult = e; + } + }); + expect(statusCallback).toHaveBeenCalledTimes(1); + expect(statusCallback).toHaveBeenCalledWith("Saving status for foo:125 (1 more remaining)..."); + expect(updateResult.message).toEqual("Status failed to save; \"not ok\""); + }); + + it("updates children correctly", async () => { + fetchValues.action.fetchJSON.mockResolvedValue({ numFound: 1, docs: [{ id: "foo:125" }] }); + fetchValues.action.fetchText.mockResolvedValue("ok"); + const { result } = await renderHook(() => useEditorContext(), { wrapper: EditorContextProvider }); + const statusCallback = jest.fn(); + let updateResult; + await act(async () => { + updateResult = await result.current.action.updateObjectState(pid, "Active", 1, statusCallback); + }); + expect(statusCallback).toHaveBeenCalledTimes(2); + expect(statusCallback).toHaveBeenCalledWith("Saving status for foo:125 (1 more remaining)..."); + expect(statusCallback).toHaveBeenCalledWith("Saving status for test:123 (0 more remaining)..."); + expect(fetchValues.action.fetchText).toHaveBeenCalledWith("http://localhost:9000/api/edit/object/test%3A123/state", {"body": "Active", "method": "PUT"}); + expect(updateResult).toEqual(["Status saved successfully.", "success"]); + // Object storage should now be empty due to clearing of changed data: + expect(result.current.state.objectDetailsStorage).toEqual({}); + }); + }); }); diff --git a/client/context/EditorContext.tsx b/client/context/EditorContext.tsx index 95b2492e..01c5c357 100644 --- a/client/context/EditorContext.tsx +++ b/client/context/EditorContext.tsx @@ -1,5 +1,5 @@ import React, { createContext, useContext, useReducer } from "react"; -import { editObjectCatalogUrl, getObjectChildCountsUrl, getObjectChildrenUrl, getObjectDetailsUrl, getObjectParentsUrl } from "../util/routes"; +import { editObjectCatalogUrl, getObjectChildCountsUrl, getObjectChildrenUrl, getObjectDetailsUrl, getObjectParentsUrl, getObjectRecursiveChildPidsUrl, getObjectStateUrl } from "../util/routes"; import { useFetchContext } from "./FetchContext"; import { extractFirstMetadataValue as utilExtractFirstMetadataValue } from "../util/metadata"; import { TreeNode } from "../util/Breadcrumbs"; @@ -237,7 +237,7 @@ export const EditorContextProvider = ({ children }) => { export const useEditorContext = () => { const { action: { - fetchJSON + fetchJSON, fetchText } }= useFetchContext(); const { @@ -529,6 +529,56 @@ export const useEditorContext = () => { return utilExtractFirstMetadataValue(currentMetadata, field, defaultValue); } + const updateSingleObjectState = async (pid: string, newState: string, setStatusMessage: (msg: string) => void, remaining: number = 0): Promise => { + setStatusMessage(`Saving status for ${pid} (${remaining} more remaining)...`); + const target = getObjectStateUrl(pid); + const result = await fetchText(target, { method: "PUT", body: newState }); + if (result === "ok") { + // Clear and reload the cached object, since it has now changed! + removeFromObjectDetailsStorage(pid); + } + return result; + }; + + const saveObjectStateForChildPage = async (response, newState: string, found: number, total: number, setStatusMessage: (msg: string) => void): Promise => { + for (let i = 0; i < response.docs.length; i++) { + const result = await updateSingleObjectState(response.docs[i].id, newState, setStatusMessage, total - (found + i)); + if (result !== "ok") { + throw new Error(`Status failed to save; "${result}"`); + } + } + }; + + const applyObjectStateToChildren = async (pid: string, newState: string, expectedTotal: number, setStatusMessage: (msg: string) => void): Promise => { + const childPageSize = 1000; + let found = 0; + while (found < expectedTotal) { + const url = getObjectRecursiveChildPidsUrl(pid, found, childPageSize); + const nextResponse = await fetchJSON(url); + await saveObjectStateForChildPage(nextResponse, newState, found, expectedTotal, setStatusMessage); + found += nextResponse.docs.length; + } + return true; + }; + + /** + * Update an object's state (and, optionally, the states of its children). + * @param pid The PID to update + * @param newState The new state to set + * @param expectedChildCount The number of children to update (either the actual number of known children, or 0 to skip child updates) + * @param setStatusMessage Callback function to display status messages as we work + * @returns An array for updating snackbar messages (first element = message, second element = level) + */ + const updateObjectState = async function (pid: string, newState: string, expectedChildCount: number = 0, setStatusMessage: (msg: string) => void): Promise> { + if (expectedChildCount > 0) { + await applyObjectStateToChildren(pid, newState, expectedChildCount, setStatusMessage); + } + const result = await updateSingleObjectState(pid, newState, setStatusMessage); + return (result === "ok") + ? ["Status saved successfully.", "success"] + : [`Status failed to save; "${result}"`, "error"]; + } + return { state: { currentAgents, @@ -573,6 +623,7 @@ export const useEditorContext = () => { removeFromParentDetailsStorage, clearPidFromChildCountsStorage, clearPidFromChildListStorage, + updateObjectState, }, }; }