From c28c69d64a62963ac1038433e2cae03b20948138 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Thu, 17 Sep 2026 09:22:08 -0400 Subject: [PATCH 1/8] Wait & show spinner when associated libraries still loading (PP-5030) --- src/components/CatalogServices.tsx | 13 ++- src/components/Collections.tsx | 26 ++--- src/components/DiscoveryServices.tsx | 13 ++- src/components/IndividualAdmins.tsx | 17 +++- src/components/MetadataServices.tsx | 13 ++- src/components/PatronAuthServices.tsx | 13 ++- src/components/ServiceEditForm.tsx | 20 +++- src/interfaces.ts | 8 ++ src/reducers/libraries.ts | 26 ++++- src/utils/allLibraries.ts | 49 ++++++++++ .../jest/components/IndividualAdmins.test.tsx | 11 ++- .../components/PatronAuthServices.test.tsx | 15 +++ .../jest/components/ServiceEditForm.test.tsx | 46 +++++++++ tests/jest/components/SetupPage.test.tsx | 17 ++-- tests/jest/reducers/libraries.test.ts | 41 ++++++++ tests/jest/utils/allLibraries.test.ts | 97 +++++++++++++++++++ 16 files changed, 379 insertions(+), 46 deletions(-) create mode 100644 src/utils/allLibraries.ts create mode 100644 tests/jest/reducers/libraries.test.ts create mode 100644 tests/jest/utils/allLibraries.test.ts diff --git a/src/components/CatalogServices.tsx b/src/components/CatalogServices.tsx index ff039670d5..9b9028860f 100644 --- a/src/components/CatalogServices.tsx +++ b/src/components/CatalogServices.tsx @@ -5,6 +5,10 @@ import EditableConfigList, { } from "./EditableConfigList"; import { connect } from "react-redux"; import ActionCreator from "../actions"; +import { + fetchLibrariesIfNeeded, + settledAllLibraries, +} from "../utils/allLibraries"; import { CatalogServicesData, CatalogServiceData } from "../interfaces"; import ServiceEditForm from "./ServiceEditForm"; @@ -37,9 +41,7 @@ function mapStateToProps(state) { {}, (state.editor.catalogServices && state.editor.catalogServices.data) || {} ); - if (state.editor.libraries && state.editor.libraries.data) { - data.allLibraries = state.editor.libraries.data.libraries; - } + Object.assign(data, settledAllLibraries(state)); // fetchError = an error involving loading the list of catalog services; formError = an error upon submission // of the create/edit form. return { @@ -58,7 +60,10 @@ function mapStateToProps(state) { function mapDispatchToProps(dispatch, ownProps) { const actions = new ActionCreator(null, ownProps.csrfToken); return { - fetchData: () => dispatch(actions.fetchCatalogServices()), + fetchData: () => { + fetchLibrariesIfNeeded(dispatch, actions); + return dispatch(actions.fetchCatalogServices()); + }, editItem: (data: FormData) => dispatch(actions.editCatalogService(data)), deleteItem: (identifier: string | number) => dispatch(actions.deleteCatalogService(identifier)), diff --git a/src/components/Collections.tsx b/src/components/Collections.tsx index d80531bd3d..4408ba6b23 100644 --- a/src/components/Collections.tsx +++ b/src/components/Collections.tsx @@ -9,6 +9,10 @@ import { import { connect } from "react-redux"; import * as PropTypes from "prop-types"; import ActionCreator from "../actions"; +import { + fetchLibrariesIfNeeded, + settledAllLibraries, +} from "../utils/allLibraries"; import { CollectionsData, CollectionData, @@ -21,13 +25,11 @@ import CollectionReapButton from "./CollectionReapButton"; import ServiceWithRegistrationsEditForm from "./ServiceWithRegistrationsEditForm"; import TrashIcon from "./icons/TrashIcon"; -export interface CollectionsStateProps - extends EditableConfigListStateProps { +export interface CollectionsStateProps extends EditableConfigListStateProps { isFetchingLibraryRegistrations?: boolean; } -export interface CollectionsDispatchProps - extends EditableConfigListDispatchProps { +export interface CollectionsDispatchProps extends EditableConfigListDispatchProps { registerLibrary: (data: FormData) => Promise; fetchLibraryRegistrations?: () => Promise; importCollection: ( @@ -38,13 +40,12 @@ export interface CollectionsDispatchProps } export interface CollectionsProps - extends CollectionsStateProps, + extends + CollectionsStateProps, CollectionsDispatchProps, EditableConfigListOwnProps {} -export class CollectionEditForm extends ServiceWithRegistrationsEditForm< - CollectionsData -> { +export class CollectionEditForm extends ServiceWithRegistrationsEditForm { context: ServiceWithRegistrationsEditForm["context"] & { importCollection: ( collectionId: string | number, @@ -209,9 +210,7 @@ function mapStateToProps(state) { {}, (state.editor.collections && state.editor.collections.data) || {} ); - if (state.editor.libraries && state.editor.libraries.data) { - data.allLibraries = state.editor.libraries.data.libraries; - } + Object.assign(data, settledAllLibraries(state)); // fetchError = an error involving loading the list of collections; formError = an error upon // submission of the create/edit form. return { @@ -228,7 +227,10 @@ function mapStateToProps(state) { function mapDispatchToProps(dispatch, ownProps) { const actions = new ActionCreator(null, ownProps.csrfToken); return { - fetchData: () => dispatch(actions.fetchCollections()), + fetchData: () => { + fetchLibrariesIfNeeded(dispatch, actions); + return dispatch(actions.fetchCollections()); + }, editItem: (data: FormData) => dispatch(actions.editCollection(data)), deleteItem: (identifier: string | number) => dispatch(actions.deleteCollection(identifier)), diff --git a/src/components/DiscoveryServices.tsx b/src/components/DiscoveryServices.tsx index 87ace37071..e697c2cff9 100644 --- a/src/components/DiscoveryServices.tsx +++ b/src/components/DiscoveryServices.tsx @@ -8,6 +8,10 @@ import { import { connect } from "react-redux"; import * as PropTypes from "prop-types"; import ActionCreator from "../actions"; +import { + fetchLibrariesIfNeeded, + settledAllLibraries, +} from "../utils/allLibraries"; import { DiscoveryServicesData, DiscoveryServiceData, @@ -120,9 +124,7 @@ function mapStateToProps(state) { (state.editor.discoveryServices && state.editor.discoveryServices.data) || {} ); - if (state.editor.libraries && state.editor.libraries.data) { - data.allLibraries = state.editor.libraries.data.libraries; - } + Object.assign(data, settledAllLibraries(state)); if ( state.editor.discoveryServiceLibraryRegistrations && state.editor.discoveryServiceLibraryRegistrations.data @@ -156,7 +158,10 @@ function mapStateToProps(state) { function mapDispatchToProps(dispatch, ownProps) { const actions = new ActionCreator(null, ownProps.csrfToken); return { - fetchData: () => dispatch(actions.fetchDiscoveryServices()), + fetchData: () => { + fetchLibrariesIfNeeded(dispatch, actions); + return dispatch(actions.fetchDiscoveryServices()); + }, editItem: (data: FormData) => dispatch(actions.editDiscoveryService(data)), deleteItem: (identifier: string | number) => dispatch(actions.deleteDiscoveryService(identifier)), diff --git a/src/components/IndividualAdmins.tsx b/src/components/IndividualAdmins.tsx index 2ee0f6d8f1..c02ea8cc58 100644 --- a/src/components/IndividualAdmins.tsx +++ b/src/components/IndividualAdmins.tsx @@ -6,6 +6,10 @@ import EditableConfigList, { } from "./EditableConfigList"; import { connect } from "react-redux"; import ActionCreator from "../actions"; +import { + fetchLibrariesIfNeeded, + settledAllLibraries, +} from "../utils/allLibraries"; import { IndividualAdminsData, IndividualAdminData, @@ -176,9 +180,7 @@ function mapStateToProps(state) { {}, (state.editor.individualAdmins && state.editor.individualAdmins.data) || {} ); - if (state.editor.libraries && state.editor.libraries.data) { - data.allLibraries = state.editor.libraries.data.libraries; - } + Object.assign(data, settledAllLibraries(state)); // fetchError = an error involving loading the list of individual admins; formError = an error upon submission of the // create/edit form. return { @@ -197,7 +199,14 @@ function mapStateToProps(state) { function mapDispatchToProps(dispatch, ownProps) { const actions = new ActionCreator(null, ownProps.csrfToken); return { - fetchData: () => dispatch(actions.fetchIndividualAdmins()), + fetchData: () => { + // The pre-auth setup page has no admin yet, so a libraries request + // could only fail; skip it there. + if (!ownProps.settingUp) { + fetchLibrariesIfNeeded(dispatch, actions); + } + return dispatch(actions.fetchIndividualAdmins()); + }, editItem: (data: FormData) => dispatch(actions.editIndividualAdmin(data)), deleteItem: (identifier: string | number) => dispatch(actions.deleteIndividualAdmin(identifier)), diff --git a/src/components/MetadataServices.tsx b/src/components/MetadataServices.tsx index 59af0b3569..e9a9930de6 100644 --- a/src/components/MetadataServices.tsx +++ b/src/components/MetadataServices.tsx @@ -6,6 +6,10 @@ import EditableConfigList, { } from "./EditableConfigList"; import { connect } from "react-redux"; import ActionCreator from "../actions"; +import { + fetchLibrariesIfNeeded, + settledAllLibraries, +} from "../utils/allLibraries"; import { MetadataServicesData, MetadataServiceData } from "../interfaces"; import ServiceEditForm from "./ServiceEditForm"; @@ -49,9 +53,7 @@ function mapStateToProps(state) { {}, (state.editor.metadataServices && state.editor.metadataServices.data) || {} ); - if (state.editor.libraries && state.editor.libraries.data) { - data.allLibraries = state.editor.libraries.data.libraries; - } + Object.assign(data, settledAllLibraries(state)); // fetchError = an error involving loading the list of metadata services; formError = an error upon submission of the // create/edit form. return { @@ -70,7 +72,10 @@ function mapStateToProps(state) { function mapDispatchToProps(dispatch, ownProps) { const actions = new ActionCreator(null, ownProps.csrfToken); return { - fetchData: () => dispatch(actions.fetchMetadataServices()), + fetchData: () => { + fetchLibrariesIfNeeded(dispatch, actions); + return dispatch(actions.fetchMetadataServices()); + }, editItem: (data: FormData) => dispatch(actions.editMetadataService(data)), deleteItem: (identifier: string | number) => dispatch(actions.deleteMetadataService(identifier)), diff --git a/src/components/PatronAuthServices.tsx b/src/components/PatronAuthServices.tsx index aee6770bce..39f686cfda 100644 --- a/src/components/PatronAuthServices.tsx +++ b/src/components/PatronAuthServices.tsx @@ -6,6 +6,10 @@ import EditableConfigList, { } from "./EditableConfigList"; import { connect } from "react-redux"; import ActionCreator from "../actions"; +import { + fetchLibrariesIfNeeded, + settledAllLibraries, +} from "../utils/allLibraries"; import { PatronAuthServicesData, PatronAuthServiceData } from "../interfaces"; import PatronAuthServiceEditForm from "./PatronAuthServiceEditForm"; import NeighborhoodAnalyticsForm from "./NeighborhoodAnalyticsForm"; @@ -63,9 +67,7 @@ function mapStateToProps(state, ownProps) { (state.editor.patronAuthServices && state.editor.patronAuthServices.data) || {} ); - if (state.editor.libraries && state.editor.libraries.data) { - data.allLibraries = state.editor.libraries.data.libraries; - } + Object.assign(data, settledAllLibraries(state)); // fetchError = an error involving loading the list of patron auth services; formError = an error upon submission // of the create/edit form. return { @@ -85,7 +87,10 @@ function mapStateToProps(state, ownProps) { function mapDispatchToProps(dispatch, ownProps) { const actions = new ActionCreator(null, ownProps.csrfToken); return { - fetchData: () => dispatch(actions.fetchPatronAuthServices()), + fetchData: () => { + fetchLibrariesIfNeeded(dispatch, actions); + return dispatch(actions.fetchPatronAuthServices()); + }, editItem: (data: FormData) => dispatch(actions.editPatronAuthService(data)), deleteItem: (identifier: string | number) => dispatch(actions.deletePatronAuthService(identifier)), diff --git a/src/components/ServiceEditForm.tsx b/src/components/ServiceEditForm.tsx index 5db07d13ef..b4c6044002 100644 --- a/src/components/ServiceEditForm.tsx +++ b/src/components/ServiceEditForm.tsx @@ -14,6 +14,7 @@ import { import { clearForm, libraryLabel } from "../utils/sharedFunctions"; import LibraryConfigLink from "./LibraryConfigLink"; import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; +import { Alert } from "react-bootstrap"; export interface ServiceEditFormProps { data: T; @@ -372,9 +373,20 @@ export default class ServiceEditForm< } renderLibrariesForm(protocol: ProtocolData, disabled: boolean) { - return ( + // allLibraries is undefined until the library list request settles; + // wait rather than flash unlinked short names that change on arrival. + // The status line stays mounted and only its text changes, so screen + // readers announce both the wait and its end. + const loading = !this.props.data?.allLibraries; + const librariesFieldset = !loading && (
Libraries + {this.props.data.allLibrariesError && ( + + The library list failed to load. Associated libraries are shown by + short name only, and libraries cannot be added. + + )}
{this.state.libraries.map((library) => (
@@ -497,6 +509,12 @@ export default class ServiceEditForm< )}
); + return ( + <> +

{loading ? "Loading libraries..." : ""}

+ {librariesFieldset} + + ); } availableProtocols(props?): ProtocolData[] { diff --git a/src/interfaces.ts b/src/interfaces.ts index babcdb8a01..d5845c6242 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -1,5 +1,7 @@ /* eslint-disable */ +import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; + export interface ConfigurationSettings { /** A token generated by the server to prevent Cross-Site Request Forgery. The token should be included in an 'X-CSRF-Token' header in any non-GET @@ -386,7 +388,10 @@ export interface ServiceData { export interface ServicesData { protocols: ProtocolData[]; + /** The sitewide library list; undefined until its request settles. */ allLibraries?: LibraryData[]; + /** Set when the sitewide library list failed to load. */ + allLibrariesError?: FetchErrorData; } export interface ServicesWithRegistrationsData extends ServicesData { @@ -425,7 +430,10 @@ export interface IndividualAdminData { export interface IndividualAdminsData { individualAdmins?: IndividualAdminData[]; + /** The sitewide library list; undefined until its request settles. */ allLibraries?: LibraryData[]; + /** Set when the sitewide library list failed to load. */ + allLibrariesError?: FetchErrorData; } export interface PatronAuthServiceData extends ServiceData {} diff --git a/src/reducers/libraries.ts b/src/reducers/libraries.ts index 3f94ea55a8..5dac98a958 100644 --- a/src/reducers/libraries.ts +++ b/src/reducers/libraries.ts @@ -1,8 +1,30 @@ import { LibrariesData } from "../interfaces"; import ActionCreator from "../actions"; -import createFetchEditReducer from "./createFetchEditReducer"; +import createFetchEditReducer, { + FetchEditState, +} from "./createFetchEditReducer"; -export default createFetchEditReducer( +const fetchEditReducer = createFetchEditReducer( ActionCreator.LIBRARIES, ActionCreator.EDIT_LIBRARY ); + +/** + * The standard fetch-edit reducer, except that a refetch after a failure + * keeps the failed state (fetchError, isLoaded) visible while the retry is + * in flight. The plain REQUEST handler clears both, which would flip + * consumers from "failed" back to "loading" on every retry. + */ +export default ( + state: FetchEditState | undefined, + action +): FetchEditState => { + const next = fetchEditReducer(state, action); + if ( + action.type === `${ActionCreator.LIBRARIES}_${ActionCreator.REQUEST}` && + state?.fetchError + ) { + return { ...next, fetchError: state.fetchError, isLoaded: state.isLoaded }; + } + return next; +}; diff --git a/src/utils/allLibraries.ts b/src/utils/allLibraries.ts new file mode 100644 index 0000000000..5557cdff3c --- /dev/null +++ b/src/utils/allLibraries.ts @@ -0,0 +1,49 @@ +// Helpers shared by the config pages whose "Libraries" sections resolve +// associated-library short names against the sitewide library list kept in +// state.editor.libraries. + +import ActionCreator from "../actions"; +import { LibraryData } from "../interfaces"; +import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; + +/** + * Returns the sitewide library list once its request has settled, or an + * empty object while the request is still pending. A failed request settles + * to an empty list plus the error. Consumers can therefore tell "still + * loading" (allLibraries undefined) apart from "no libraries" ([]), and can + * report a failure. + * + * Merge the result into the `data` prop built by a config page's + * mapStateToProps. + */ +export function settledAllLibraries(state): { + allLibraries?: LibraryData[]; + allLibrariesError?: FetchErrorData; +} { + const libraries = state.editor.libraries; + if (!libraries?.data && !libraries?.isLoaded) { + return {}; + } + return { + allLibraries: libraries.data?.libraries ?? [], + allLibrariesError: libraries.fetchError ?? undefined, + }; +} + +/** + * Fetches the sitewide library list unless a copy is already loaded or a + * request is already in flight (the app header also fetches it on mount). + * A previously failed request is retried. A failure lands in Redux state; + * the catch only avoids an unhandled rejection. + */ +export function fetchLibrariesIfNeeded(dispatch, actions: ActionCreator): void { + dispatch((thunkDispatch, getState) => { + const libraries = getState().editor.libraries; + if ( + !libraries?.isFetching && + (!libraries?.isLoaded || libraries?.fetchError) + ) { + thunkDispatch(actions.fetchLibraries()).catch(() => {}); + } + }); +} diff --git a/tests/jest/components/IndividualAdmins.test.tsx b/tests/jest/components/IndividualAdmins.test.tsx index 9a2406008a..3b30f758b6 100644 --- a/tests/jest/components/IndividualAdmins.test.tsx +++ b/tests/jest/components/IndividualAdmins.test.tsx @@ -498,10 +498,13 @@ describe("IndividualAdmins - connect wiring", () => { afterEach(() => jest.restoreAllMocks()); it("renders the connected default export, fetching on mount", async () => { - jest.spyOn(globalThis, "fetch").mockResolvedValue( - new Response(JSON.stringify({ individualAdmins: [] }), { - headers: { "Content-Type": "application/json" }, - }) + // A Response body can only be read once, so build one per fetch call. + jest.spyOn(globalThis, "fetch").mockImplementation(() => + Promise.resolve( + new Response(JSON.stringify({ individualAdmins: [] }), { + headers: { "Content-Type": "application/json" }, + }) + ) ); renderWithProviders( diff --git a/tests/jest/components/PatronAuthServices.test.tsx b/tests/jest/components/PatronAuthServices.test.tsx index c33180e8f2..c3dcd30659 100644 --- a/tests/jest/components/PatronAuthServices.test.tsx +++ b/tests/jest/components/PatronAuthServices.test.tsx @@ -77,6 +77,21 @@ describe("PatronAuthServices", () => { ).not.toBeInTheDocument(); }); + it("fetches the libraries list along with the services on mount", async () => { + const fetchSpy = stubFetch(listData); + renderConnected(); + + await waitFor(() => { + const urls = fetchSpy.mock.calls.map((call) => String(call[0])); + expect(urls).toEqual( + expect.arrayContaining([ + expect.stringContaining("/admin/patron_auth_services"), + expect.stringContaining("/admin/libraries"), + ]) + ); + }); + }); + it("shows the neighborhood analytics panel when creating a service whose protocol has a neighborhood_mode setting", async () => { const neighborhoodSetting = { key: "neighborhood_mode", diff --git a/tests/jest/components/ServiceEditForm.test.tsx b/tests/jest/components/ServiceEditForm.test.tsx index d2a9ed74cd..1dee006973 100644 --- a/tests/jest/components/ServiceEditForm.test.tsx +++ b/tests/jest/components/ServiceEditForm.test.tsx @@ -484,6 +484,52 @@ describe("ServiceEditForm", () => { expect(container.querySelector(".with-edit-button a")).toBeNull(); }); + it("shows a loading indicator in the Libraries panel until allLibraries arrives", () => { + // Undefined allLibraries means the library list is still loading. + const dataStillLoading = Object.assign({}, servicesData, { + allLibraries: undefined, + }); + const { container, rerender } = renderForm({ + data: dataStillLoading, + item: serviceData, + }); + expect(container.querySelector(".update-libraries")).toBeNull(); + expect(container.querySelector('[role="status"]')).toHaveTextContent( + "Loading libraries..." + ); + + rerenderForm(rerender, { item: serviceData }); + // The status line stays mounted (so screen readers announce the text + // change) but empties out. + expect(container.querySelector('[role="status"]')).toBeEmptyDOMElement(); + const editable = container.querySelectorAll(".with-edit-button"); + expect(editable).toHaveLength(1); + expect(editable[0]).toHaveTextContent("New York Public Library - nypl"); + }); + + it("explains a failed library list load in the Libraries panel", () => { + // On failure allLibraries settles to [] and allLibrariesError is set. + const dataWithError = Object.assign({}, servicesData, { + allLibraries: [], + allLibrariesError: { + status: 500, + response: "nope", + url: "/admin/libraries", + }, + }); + const { container } = renderForm({ + data: dataWithError, + item: serviceData, + }); + expect(container.querySelector(".alert-danger")).toHaveTextContent( + "The library list failed to load" + ); + // The associated library still renders, by short name. + const editable = container.querySelectorAll(".with-edit-button"); + expect(editable).toHaveLength(1); + expect(editable[0]).toHaveTextContent("nypl"); + }); + it("renders removable and editable libraries", () => { const { container, unmount } = renderForm(); expect(container.querySelectorAll(".with-remove-button")).toHaveLength(0); diff --git a/tests/jest/components/SetupPage.test.tsx b/tests/jest/components/SetupPage.test.tsx index 58b5ef9912..b287ae639e 100644 --- a/tests/jest/components/SetupPage.test.tsx +++ b/tests/jest/components/SetupPage.test.tsx @@ -6,13 +6,16 @@ import SetupPage from "../../../src/components/SetupPage"; describe("SetupPage", () => { beforeEach(() => { - // SetupPage renders the connected IndividualAdmins list, which fetches the - // admin list on mount. Stub fetch so mounting does not hit the network. - jest.spyOn(globalThis, "fetch").mockResolvedValue( - new Response(JSON.stringify({ individualAdmins: [] }), { - status: 200, - headers: { "Content-Type": "application/json" }, - }) + // SetupPage renders the connected IndividualAdmins list, which fetches on + // mount. Stub fetch so mounting does not hit the network; a Response body + // can only be read once, so build one per call. + jest.spyOn(globalThis, "fetch").mockImplementation(() => + Promise.resolve( + new Response(JSON.stringify({ individualAdmins: [] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }) + ) ); }); diff --git a/tests/jest/reducers/libraries.test.ts b/tests/jest/reducers/libraries.test.ts new file mode 100644 index 0000000000..0cf73a1154 --- /dev/null +++ b/tests/jest/reducers/libraries.test.ts @@ -0,0 +1,41 @@ +import libraries from "../../../src/reducers/libraries"; +import ActionCreator from "../../../src/actions"; + +const REQUEST = `${ActionCreator.LIBRARIES}_${ActionCreator.REQUEST}`; +const FAILURE = `${ActionCreator.LIBRARIES}_${ActionCreator.FAILURE}`; +const SUCCESS = `${ActionCreator.LIBRARIES}_${ActionCreator.SUCCESS}`; +const LOAD = `${ActionCreator.LIBRARIES}_${ActionCreator.LOAD}`; + +describe("libraries reducer", () => { + const fetchError = { status: 500, response: "nope", url: "/admin/libraries" }; + + it("clears state on a first-load request", () => { + const state = libraries(undefined, { type: REQUEST }); + expect(state.isFetching).toBe(true); + expect(state.isLoaded).toBe(false); + expect(state.fetchError).toBeNull(); + }); + + it("keeps the failed state visible while a retry is in flight", () => { + const failed = libraries(undefined, { type: FAILURE, error: fetchError }); + expect(failed.fetchError).toEqual(fetchError); + expect(failed.isLoaded).toBe(true); + + const retrying = libraries(failed, { type: REQUEST }); + expect(retrying.isFetching).toBe(true); + expect(retrying.fetchError).toEqual(fetchError); + expect(retrying.isLoaded).toBe(true); + }); + + it("clears the failure once a retry succeeds", () => { + const data = { libraries: [{ short_name: "nypl" }] }; + let state = libraries(undefined, { type: FAILURE, error: fetchError }); + state = libraries(state, { type: REQUEST }); + state = libraries(state, { type: SUCCESS }); + state = libraries(state, { type: LOAD, data }); + + expect(state.fetchError).toBeNull(); + expect(state.isLoaded).toBe(true); + expect(state.data).toEqual(data); + }); +}); diff --git a/tests/jest/utils/allLibraries.test.ts b/tests/jest/utils/allLibraries.test.ts new file mode 100644 index 0000000000..3c6598ef50 --- /dev/null +++ b/tests/jest/utils/allLibraries.test.ts @@ -0,0 +1,97 @@ +import { + fetchLibrariesIfNeeded, + settledAllLibraries, +} from "../../../src/utils/allLibraries"; + +describe("settledAllLibraries", () => { + const libraries = [{ short_name: "nypl", name: "New York Public Library" }]; + const stateWith = (librariesState) => ({ + editor: { libraries: librariesState }, + }); + + it("returns nothing while the request has not settled", () => { + expect(settledAllLibraries(stateWith(undefined))).toEqual({}); + expect( + settledAllLibraries( + stateWith({ data: null, isLoaded: false, isFetching: true }) + ) + ).toEqual({}); + }); + + it("returns the loaded list", () => { + expect( + settledAllLibraries( + stateWith({ data: { libraries }, isLoaded: true, fetchError: null }) + ) + ).toEqual({ allLibraries: libraries, allLibrariesError: undefined }); + }); + + it("settles to an empty list plus the error on failure", () => { + const fetchError = { + status: 500, + response: "nope", + url: "/admin/libraries", + }; + expect( + settledAllLibraries(stateWith({ data: null, isLoaded: true, fetchError })) + ).toEqual({ allLibraries: [], allLibrariesError: fetchError }); + }); +}); + +describe("fetchLibrariesIfNeeded", () => { + const fetchThunk = "libraries thunk"; + const actions = { fetchLibraries: jest.fn(() => fetchThunk) } as any; + + // A dispatch that runs thunks against the given libraries state and + // records every other dispatched action. + const makeDispatch = (librariesState) => { + const dispatched = []; + const getState = () => ({ editor: { libraries: librariesState } }); + const dispatch = (action) => { + if (typeof action === "function") { + return action(dispatch, getState); + } + dispatched.push(action); + return Promise.resolve(); + }; + return { dispatch, dispatched }; + }; + + beforeEach(() => jest.clearAllMocks()); + + it("fetches when the list has never loaded", () => { + const { dispatch, dispatched } = makeDispatch(undefined); + fetchLibrariesIfNeeded(dispatch, actions); + expect(dispatched).toEqual([fetchThunk]); + }); + + it("does not fetch when the list is already loaded", () => { + const { dispatch, dispatched } = makeDispatch({ + isLoaded: true, + isFetching: false, + fetchError: null, + }); + fetchLibrariesIfNeeded(dispatch, actions); + expect(dispatched).toEqual([]); + }); + + it("does not fetch while a request is already in flight", () => { + const { dispatch, dispatched } = makeDispatch({ + isLoaded: false, + isFetching: true, + fetchError: null, + }); + fetchLibrariesIfNeeded(dispatch, actions); + expect(dispatched).toEqual([]); + }); + + it("retries after a failed request", () => { + const { dispatch, dispatched } = makeDispatch({ + isLoaded: true, + isFetching: false, + fetchError: { status: 500, response: "nope", url: "/admin/libraries" }, + }); + fetchLibrariesIfNeeded(dispatch, actions); + expect(dispatched).toEqual([fetchThunk]); + }); +}); From 8c26bfa3f7ad9660735119dc3ee2918bf892ce2a Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Thu, 17 Sep 2026 20:04:13 -0400 Subject: [PATCH 2/8] Clarify who owns linting / formatting --- CLAUDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index d80dc71119..d3424b31e3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,7 +91,7 @@ Helpers in `tests/jest/testUtils/`: ## Code Style -- Prettier: double quotes, semicolons, trailing commas (es5), 80 char width +- Prettier (double quotes, semicolons, trailing commas (es5), printWidth 80) and ESLint are authoritative for formatting and style. The husky pre-commit hook auto-formats staged files with Prettier, and CI runs ESLint (`npm run lint:js`), so do not flag or hand-verify formatting. printWidth is a soft target, not a hard limit; lines Prettier leaves longer than 80 (e.g., interface `extends` clauses) are fine, and there is no ESLint max-len rule. - ESLint (flat config, `eslint.config.mjs`) with `jsx-a11y/strict` — runs on the whole tree in CI via `npm run lint:js` (`eslint . --max-warnings 0`, so warnings fail the build) and on staged files via the husky pre-commit hook. `npm run lint` runs ESLint (`lint:js`) followed by sass-lint. - `@typescript-eslint/no-explicit-any` is disabled (any is allowed) - Prefer template literals over string concatenation for building strings with variables From c083cbc381b2b8dc208a8859f8b2839052f43e16 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Thu, 17 Sep 2026 20:04:42 -0400 Subject: [PATCH 3/8] CI AI code review feedback --- src/components/IndividualAdminEditForm.tsx | 184 +++++++++++------- src/components/LibrariesLoadStatus.tsx | 30 +++ src/components/ServiceEditForm.tsx | 14 +- src/reducers/index.ts | 9 +- src/reducers/libraries.ts | 37 ++-- src/utils/allLibraries.ts | 14 +- .../IndividualAdminEditForm.test.tsx | 61 ++++++ .../jest/components/IndividualAdmins.test.tsx | 9 +- .../jest/components/ServiceEditForm.test.tsx | 20 +- tests/jest/components/SetupPage.test.tsx | 14 ++ tests/jest/reducers/libraries.test.ts | 37 +++- tests/jest/utils/allLibraries.test.ts | 20 ++ 12 files changed, 343 insertions(+), 106 deletions(-) create mode 100644 src/components/LibrariesLoadStatus.tsx diff --git a/src/components/IndividualAdminEditForm.tsx b/src/components/IndividualAdminEditForm.tsx index 1877698456..9a4e50356a 100644 --- a/src/components/IndividualAdminEditForm.tsx +++ b/src/components/IndividualAdminEditForm.tsx @@ -2,9 +2,15 @@ import * as React from "react"; import * as PropTypes from "prop-types"; import EditableInput from "./EditableInput"; import { clearForm, libraryLabel } from "../utils/sharedFunctions"; -import { IndividualAdminsData, IndividualAdminData } from "../interfaces"; +import { + IndividualAdminsData, + IndividualAdminData, + LibraryData, +} from "../interfaces"; import Admin from "../models/Admin"; import { Panel, Form } from "library-simplified-reusable-components"; +import { Alert } from "react-bootstrap"; +import LibrariesLoadStatus from "./LibrariesLoadStatus"; import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; @@ -58,6 +64,7 @@ export default class IndividualAdminEditForm extends React.Component< this.submit = this.submit.bind(this); this.renderForm = this.renderForm.bind(this); this.renderRoleForm = this.renderRoleForm.bind(this); + this.renderRolesTable = this.renderRolesTable.bind(this); } UNSAFE_componentWillReceiveProps(nextProps) { @@ -143,6 +150,9 @@ export default class IndividualAdminEditForm extends React.Component< } renderRoleForm() { + // Wait for the sitewide library list before showing per-library roles, + // mirroring the Libraries panel in ServiceEditForm. + const { allLibraries, allLibrariesError } = this.props.data; return (
Roles @@ -156,86 +166,109 @@ export default class IndividualAdminEditForm extends React.Component< checked={this.isSelected("system")} onChange={() => this.handleRoleChange("system")} /> - - + + {allLibrariesError && ( + + {this.props.item + ? "The library list failed to load. This admin's library roles cannot be shown, and no roles can be changed." + : "The library list failed to load. Sitewide roles can still be assigned, but per-library roles cannot."} + + )} + {/* For an existing admin a failed load disables all role edits, so + an empty table shell would only add noise; drop it. */} + {allLibraries && + !(this.props.item && allLibrariesError) && + this.renderRolesTable(allLibraries, allLibrariesError)} + + ); + } + + renderRolesTable( + allLibraries: LibraryData[], + allLibrariesError?: FetchErrorData + ) { + return ( +
+ + + + + + + + + {allLibraries.length === 0 && !allLibrariesError && ( - - + + )} + {allLibraries.map((library) => ( + + + - - - {this.props.data && - this.props.data.allLibraries && - this.props.data.allLibraries.map((library) => ( - - - - - - ))} - -
+ this.handleRoleChange("manager-all")} + /> + + this.handleRoleChange("librarian-all")} + /> +
+ No libraries are configured.
{libraryLabel(library.name, library.short_name)} this.handleRoleChange("manager-all")} + disabled={this.isDisabled("manager", library.short_name)} + name={`manager-${library.short_name}`} + ref={(componentInstance) => { + this.libraryManagerRefs[library.short_name] = + componentInstance; + }} + label="" + aria-label={`Administrator of ${library.short_name}`} + checked={this.isSelected("manager", library.short_name)} + onChange={() => + this.handleRoleChange("manager", library.short_name) + } /> - - + + this.handleRoleChange("librarian-all")} + disabled={this.isDisabled("librarian", library.short_name)} + name={`librarian-${library.short_name}`} + ref={(componentInstance) => { + this.librarianRefs[library.short_name] = componentInstance; + }} + label="" + aria-label={`User of ${library.short_name}`} + checked={this.isSelected("librarian", library.short_name)} + onChange={() => + this.handleRoleChange("librarian", library.short_name) + } /> - +
{libraryLabel(library.name, library.short_name)} - { - this.libraryManagerRefs[library.short_name] = - componentInstance; - }} - label="" - aria-label={`Administrator of ${library.short_name}`} - checked={this.isSelected("manager", library.short_name)} - onChange={() => - this.handleRoleChange("manager", library.short_name) - } - /> - - { - this.librarianRefs[library.short_name] = - componentInstance; - }} - label="" - aria-label={`User of ${library.short_name}`} - checked={this.isSelected("librarian", library.short_name)} - onChange={() => - this.handleRoleChange("librarian", library.short_name) - } - /> -
-
+ ))} + + ); } @@ -269,6 +302,15 @@ export default class IndividualAdminEditForm extends React.Component< if (this.props.disabled) { return true; } + // Sitewide toggles rewrite an existing admin's (hidden) per-library + // roles wholesale, so edits stay disabled until the library list is + // available. A new admin has no roles yet, so nothing can be clobbered. + if ( + this.props.item && + (!this.props.data.allLibraries || this.props.data.allLibrariesError) + ) { + return true; + } if (role === "system" || this.isSelected("system")) { return !this.context.admin.isSystemAdmin(); } diff --git a/src/components/LibrariesLoadStatus.tsx b/src/components/LibrariesLoadStatus.tsx new file mode 100644 index 0000000000..097a3ce4e3 --- /dev/null +++ b/src/components/LibrariesLoadStatus.tsx @@ -0,0 +1,30 @@ +import * as React from "react"; +import { LibraryData } from "../interfaces"; +import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; + +export interface LibrariesLoadStatusProps { + allLibraries?: LibraryData[]; + allLibrariesError?: FetchErrorData; +} + +/** + * Live status line for the sitewide library list load. It stays mounted so + * that the settled text is a content change, which screen readers announce + * (content present at mount is not announced). The settled text is visually + * hidden; the surrounding UI shows the outcome. + */ +export default function LibrariesLoadStatus({ + allLibraries, + allLibrariesError, +}: LibrariesLoadStatusProps): JSX.Element { + const loading = !allLibraries; + return ( +

+ {loading + ? "Loading libraries..." + : allLibrariesError + ? "Libraries failed to load." + : "Libraries loaded."} +

+ ); +} diff --git a/src/components/ServiceEditForm.tsx b/src/components/ServiceEditForm.tsx index b4c6044002..9d76bc3bff 100644 --- a/src/components/ServiceEditForm.tsx +++ b/src/components/ServiceEditForm.tsx @@ -13,6 +13,7 @@ import { } from "../interfaces"; import { clearForm, libraryLabel } from "../utils/sharedFunctions"; import LibraryConfigLink from "./LibraryConfigLink"; +import LibrariesLoadStatus from "./LibrariesLoadStatus"; import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; import { Alert } from "react-bootstrap"; @@ -375,9 +376,7 @@ export default class ServiceEditForm< renderLibrariesForm(protocol: ProtocolData, disabled: boolean) { // allLibraries is undefined until the library list request settles; // wait rather than flash unlinked short names that change on arrival. - // The status line stays mounted and only its text changes, so screen - // readers announce both the wait and its end. - const loading = !this.props.data?.allLibraries; + const loading = !this.props.data.allLibraries; const librariesFieldset = !loading && (
Libraries @@ -387,6 +386,10 @@ export default class ServiceEditForm< short name only, and libraries cannot be added. )} + {this.props.data.allLibraries.length === 0 && + !this.props.data.allLibrariesError && ( +

No libraries are configured.

+ )}
{this.state.libraries.map((library) => (
@@ -511,7 +514,10 @@ export default class ServiceEditForm< ); return ( <> -

{loading ? "Loading libraries..." : ""}

+ {librariesFieldset} ); diff --git a/src/reducers/index.ts b/src/reducers/index.ts index 8f41f0aa19..08dfc72d93 100644 --- a/src/reducers/index.ts +++ b/src/reducers/index.ts @@ -5,7 +5,7 @@ import bookCoverPreview, { BookCoverPreviewState } from "./bookCoverPreview"; import bookCover from "./bookCover"; import customListsForBook from "./customListsForBook"; import diagnostics from "./diagnostics"; -import libraries from "./libraries"; +import libraries, { LibrariesState } from "./libraries"; import collections from "./collections"; import individualAdmins from "./individualAdmins"; import patronAuthServices from "./patronAuthServices"; @@ -38,7 +38,6 @@ import { FetchEditState } from "./createFetchEditReducer"; import { RegisterLibraryState } from "./createRegisterLibraryReducer"; import patronManager from "./managePatrons"; import { - LibrariesData, CollectionsData, IndividualAdminsData, PatronAuthServicesData, @@ -65,7 +64,7 @@ export interface State { bookCover: FetchEditState; customListsForBook: FetchEditState; diagnostics: FetchEditState; - libraries: FetchEditState; + libraries: LibrariesState; collections: FetchEditState; individualAdmins: FetchEditState; patronAuthServices: FetchEditState; @@ -74,9 +73,7 @@ export interface State { catalogServices: FetchEditState; discoveryServices: FetchEditState; registerLibraryWithDiscoveryService: RegisterLibraryState; - discoveryServiceLibraryRegistrations: FetchEditState< - LibraryRegistrationsData - >; + discoveryServiceLibraryRegistrations: FetchEditState; customLists: FetchEditState; customListDetails: FetchMoreCustomListDetails; customListEditor: CustomListEditorState; diff --git a/src/reducers/libraries.ts b/src/reducers/libraries.ts index 5dac98a958..78b19e4e10 100644 --- a/src/reducers/libraries.ts +++ b/src/reducers/libraries.ts @@ -1,30 +1,39 @@ import { LibrariesData } from "../interfaces"; +import { RequestError } from "@thepalaceproject/web-opds-client/lib/DataFetcher"; import ActionCreator from "../actions"; import createFetchEditReducer, { FetchEditState, } from "./createFetchEditReducer"; +export interface LibrariesState extends FetchEditState { + /** + * The failure that a retry now in flight is retrying. Kept under its own + * key so that fetchError keeps meaning "the current request failed" for + * direct consumers (e.g. the Libraries config page); consumers that want + * to keep showing the old failure during the retry read this instead. + * Cleared when the retry settles (FAILURE or LOAD). + */ + lastFetchError?: RequestError | null; +} + const fetchEditReducer = createFetchEditReducer( ActionCreator.LIBRARIES, ActionCreator.EDIT_LIBRARY ); -/** - * The standard fetch-edit reducer, except that a refetch after a failure - * keeps the failed state (fetchError, isLoaded) visible while the retry is - * in flight. The plain REQUEST handler clears both, which would flip - * consumers from "failed" back to "loading" on every retry. - */ -export default ( - state: FetchEditState | undefined, - action -): FetchEditState => { - const next = fetchEditReducer(state, action); +const librariesAction = (action: string) => + `${ActionCreator.LIBRARIES}_${action}`; + +export default (state: LibrariesState | undefined, action): LibrariesState => { + const next: LibrariesState = fetchEditReducer(state, action); + if (action.type === librariesAction(ActionCreator.REQUEST)) { + return { ...next, lastFetchError: state?.fetchError ?? null }; + } if ( - action.type === `${ActionCreator.LIBRARIES}_${ActionCreator.REQUEST}` && - state?.fetchError + action.type === librariesAction(ActionCreator.FAILURE) || + action.type === librariesAction(ActionCreator.LOAD) ) { - return { ...next, fetchError: state.fetchError, isLoaded: state.isLoaded }; + return { ...next, lastFetchError: null }; } return next; }; diff --git a/src/utils/allLibraries.ts b/src/utils/allLibraries.ts index 5557cdff3c..7982f3a5b1 100644 --- a/src/utils/allLibraries.ts +++ b/src/utils/allLibraries.ts @@ -11,7 +11,9 @@ import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces * empty object while the request is still pending. A failed request settles * to an empty list plus the error. Consumers can therefore tell "still * loading" (allLibraries undefined) apart from "no libraries" ([]), and can - * report a failure. + * report a failure. A retry after a failure also counts as settled (the + * reducer keeps the old failure as lastFetchError), so the previous error + * stays visible while the retry runs. * * Merge the result into the `data` prop built by a config page's * mapStateToProps. @@ -21,12 +23,18 @@ export function settledAllLibraries(state): { allLibrariesError?: FetchErrorData; } { const libraries = state.editor.libraries; - if (!libraries?.data && !libraries?.isLoaded) { + if ( + !libraries?.data && + !libraries?.isLoaded && + !libraries?.fetchError && + !libraries?.lastFetchError + ) { return {}; } return { allLibraries: libraries.data?.libraries ?? [], - allLibrariesError: libraries.fetchError ?? undefined, + allLibrariesError: + libraries.fetchError ?? libraries.lastFetchError ?? undefined, }; } diff --git a/tests/jest/components/IndividualAdminEditForm.test.tsx b/tests/jest/components/IndividualAdminEditForm.test.tsx index 4c73a49d2c..a1d8e05b7b 100644 --- a/tests/jest/components/IndividualAdminEditForm.test.tsx +++ b/tests/jest/components/IndividualAdminEditForm.test.tsx @@ -202,6 +202,67 @@ describe("IndividualAdminEditForm - rendered inputs and role changes", () => { const roleCheckbox = (role: string) => screen.getByRole("checkbox", { name: roleNames[role] }); + const failureData = { + individualAdmins: [adminData], + allLibraries: [], + allLibrariesError: { + status: 500, + response: "nope", + url: "/admin/libraries", + }, + }; + + it("waits for the library list before showing per-library roles", () => { + const { container } = renderForm({ + item: adminData, + data: { individualAdmins: [adminData] }, + }); + expect(container.querySelector("table.library-admin-roles")).toBeNull(); + expect(container.querySelector('[role="status"]')).toHaveTextContent( + "Loading libraries..." + ); + // Sitewide toggles rewrite an existing admin's hidden per-library roles + // wholesale, so role edits are disabled until the list is available. + expect(roleCheckbox("system")).toBeDisabled(); + }); + + it("explains a failed library list load when editing an existing admin", () => { + // On failure allLibraries settles to [] and allLibrariesError is set. + const { container } = renderForm({ item: adminData, data: failureData }); + expect( + screen.getByText( + /library roles cannot be shown, and no roles can be changed/ + ) + ).toBeInTheDocument(); + expect(container.querySelector('[role="status"]')).toHaveTextContent( + "Libraries failed to load." + ); + expect(roleCheckbox("system")).toBeDisabled(); + // All role edits are disabled, so the empty table shell is dropped. + expect(container.querySelector("table.library-admin-roles")).toBeNull(); + }); + + it("still allows sitewide roles for a new admin when the library list failed", () => { + // A new admin has no hidden per-library roles to clobber. + const { container } = renderForm({ data: failureData }); + expect( + screen.getByText(/Sitewide roles can still be assigned/) + ).toBeInTheDocument(); + expect(roleCheckbox("system")).toBeEnabled(); + expect(roleCheckbox("manager-all")).toBeEnabled(); + // No misleading "no libraries" row under a failure. + expect(container.querySelector("tbody")).toBeEmptyDOMElement(); + }); + + it("says when no libraries are configured", () => { + renderForm({ + data: { individualAdmins: [adminData], allLibraries: [] }, + }); + expect( + screen.getByText("No libraries are configured.") + ).toBeInTheDocument(); + }); + const expectRoles = (expected: string[]) => { for (const role of allRoles) { if (expected.includes(role)) { diff --git a/tests/jest/components/IndividualAdmins.test.tsx b/tests/jest/components/IndividualAdmins.test.tsx index 3b30f758b6..801680fdce 100644 --- a/tests/jest/components/IndividualAdmins.test.tsx +++ b/tests/jest/components/IndividualAdmins.test.tsx @@ -499,7 +499,7 @@ describe("IndividualAdmins - connect wiring", () => { it("renders the connected default export, fetching on mount", async () => { // A Response body can only be read once, so build one per fetch call. - jest.spyOn(globalThis, "fetch").mockImplementation(() => + const fetchSpy = jest.spyOn(globalThis, "fetch").mockImplementation(() => Promise.resolve( new Response(JSON.stringify({ individualAdmins: [] }), { headers: { "Content-Type": "application/json" }, @@ -520,5 +520,12 @@ describe("IndividualAdmins - connect wiring", () => { expect( await screen.findByText("Create new individual admin") ).toBeInTheDocument(); + + // Outside setup mode, fetchData also requests the libraries list (the + // settingUp half of that guard is pinned in SetupPage.test.tsx). + const urls = fetchSpy.mock.calls.map((call) => String(call[0])); + expect(urls).toEqual( + expect.arrayContaining([expect.stringContaining("/admin/libraries")]) + ); }); }); diff --git a/tests/jest/components/ServiceEditForm.test.tsx b/tests/jest/components/ServiceEditForm.test.tsx index 1dee006973..81eb392a23 100644 --- a/tests/jest/components/ServiceEditForm.test.tsx +++ b/tests/jest/components/ServiceEditForm.test.tsx @@ -499,9 +499,11 @@ describe("ServiceEditForm", () => { ); rerenderForm(rerender, { item: serviceData }); - // The status line stays mounted (so screen readers announce the text - // change) but empties out. - expect(container.querySelector('[role="status"]')).toBeEmptyDOMElement(); + // The status line stays mounted and announces completion; the + // completion text is visually hidden. + const status = container.querySelector('[role="status"]'); + expect(status).toHaveTextContent("Libraries loaded."); + expect(status).toHaveClass("visuallyHidden"); const editable = container.querySelectorAll(".with-edit-button"); expect(editable).toHaveLength(1); expect(editable[0]).toHaveTextContent("New York Public Library - nypl"); @@ -524,12 +526,24 @@ describe("ServiceEditForm", () => { expect(container.querySelector(".alert-danger")).toHaveTextContent( "The library list failed to load" ); + // The live status must not claim success on failure. + expect(container.querySelector('[role="status"]')).toHaveTextContent( + "Libraries failed to load." + ); // The associated library still renders, by short name. const editable = container.querySelectorAll(".with-edit-button"); expect(editable).toHaveLength(1); expect(editable[0]).toHaveTextContent("nypl"); }); + it("says when no libraries are configured", () => { + const emptyData = Object.assign({}, servicesData, { allLibraries: [] }); + const { container } = renderForm({ data: emptyData }); + expect(container.querySelector(".update-libraries")).toHaveTextContent( + "No libraries are configured." + ); + }); + it("renders removable and editable libraries", () => { const { container, unmount } = renderForm(); expect(container.querySelectorAll(".with-remove-button")).toHaveLength(0); diff --git a/tests/jest/components/SetupPage.test.tsx b/tests/jest/components/SetupPage.test.tsx index b287ae639e..a1c9bbd557 100644 --- a/tests/jest/components/SetupPage.test.tsx +++ b/tests/jest/components/SetupPage.test.tsx @@ -40,5 +40,19 @@ describe("SetupPage", () => { name: "Set up your system admin account", }) ).toBeInTheDocument(); + + // settingUp skips the libraries request, which cannot succeed before an + // admin exists. The positive check keeps the negative one honest. + const urls = (globalThis.fetch as jest.Mock).mock.calls.map((call) => + String(call[0]) + ); + expect(urls).toEqual( + expect.arrayContaining([ + expect.stringContaining("/admin/individual_admins"), + ]) + ); + expect(urls).not.toEqual( + expect.arrayContaining([expect.stringContaining("/admin/libraries")]) + ); }); }); diff --git a/tests/jest/reducers/libraries.test.ts b/tests/jest/reducers/libraries.test.ts index 0cf73a1154..f1035cb38f 100644 --- a/tests/jest/reducers/libraries.test.ts +++ b/tests/jest/reducers/libraries.test.ts @@ -16,18 +16,32 @@ describe("libraries reducer", () => { expect(state.fetchError).toBeNull(); }); - it("keeps the failed state visible while a retry is in flight", () => { + it("moves the previous failure to lastFetchError while a retry is in flight", () => { const failed = libraries(undefined, { type: FAILURE, error: fetchError }); expect(failed.fetchError).toEqual(fetchError); expect(failed.isLoaded).toBe(true); const retrying = libraries(failed, { type: REQUEST }); expect(retrying.isFetching).toBe(true); - expect(retrying.fetchError).toEqual(fetchError); - expect(retrying.isLoaded).toBe(true); + // fetchError keeps meaning "the current request failed"; the old + // failure moves to lastFetchError so consumers can keep showing it. + expect(retrying.fetchError).toBeNull(); + expect(retrying.lastFetchError).toEqual(fetchError); + expect(retrying.isLoaded).toBe(false); }); - it("clears the failure once a retry succeeds", () => { + it("keeps lastFetchError through a retry's SUCCESS until LOAD", () => { + let state = libraries(undefined, { type: FAILURE, error: fetchError }); + state = libraries(state, { type: REQUEST }); + state = libraries(state, { type: SUCCESS }); + + expect(state.data).toBeNull(); + expect(state.isLoaded).toBe(false); + expect(state.fetchError).toBeNull(); + expect(state.lastFetchError).toEqual(fetchError); + }); + + it("clears the old failure once a retry succeeds", () => { const data = { libraries: [{ short_name: "nypl" }] }; let state = libraries(undefined, { type: FAILURE, error: fetchError }); state = libraries(state, { type: REQUEST }); @@ -35,7 +49,22 @@ describe("libraries reducer", () => { state = libraries(state, { type: LOAD, data }); expect(state.fetchError).toBeNull(); + expect(state.lastFetchError).toBeNull(); expect(state.isLoaded).toBe(true); expect(state.data).toEqual(data); }); + + it("reports only the new failure when a retry fails again", () => { + const newError = { + status: 502, + response: "worse", + url: "/admin/libraries", + }; + let state = libraries(undefined, { type: FAILURE, error: fetchError }); + state = libraries(state, { type: REQUEST }); + state = libraries(state, { type: FAILURE, error: newError }); + + expect(state.fetchError).toEqual(newError); + expect(state.lastFetchError).toBeNull(); + }); }); diff --git a/tests/jest/utils/allLibraries.test.ts b/tests/jest/utils/allLibraries.test.ts index 3c6598ef50..4a36a17e18 100644 --- a/tests/jest/utils/allLibraries.test.ts +++ b/tests/jest/utils/allLibraries.test.ts @@ -36,6 +36,26 @@ describe("settledAllLibraries", () => { settledAllLibraries(stateWith({ data: null, isLoaded: true, fetchError })) ).toEqual({ allLibraries: [], allLibrariesError: fetchError }); }); + + it("keeps a previous failure settled while a retry is in flight", () => { + // The reducer moves the old failure to lastFetchError during a retry. + const lastFetchError = { + status: 500, + response: "nope", + url: "/admin/libraries", + }; + expect( + settledAllLibraries( + stateWith({ + data: null, + isLoaded: false, + isFetching: true, + fetchError: null, + lastFetchError, + }) + ) + ).toEqual({ allLibraries: [], allLibrariesError: lastFetchError }); + }); }); describe("fetchLibrariesIfNeeded", () => { From 35734bcbe53d30275117b870023570bf5a183979 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Fri, 18 Sep 2026 12:49:31 -0400 Subject: [PATCH 4/8] Improve test coverage --- src/utils/allLibraries.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/utils/allLibraries.ts b/src/utils/allLibraries.ts index 7982f3a5b1..40b5c2fce7 100644 --- a/src/utils/allLibraries.ts +++ b/src/utils/allLibraries.ts @@ -47,10 +47,9 @@ export function settledAllLibraries(state): { export function fetchLibrariesIfNeeded(dispatch, actions: ActionCreator): void { dispatch((thunkDispatch, getState) => { const libraries = getState().editor.libraries; - if ( - !libraries?.isFetching && - (!libraries?.isLoaded || libraries?.fetchError) - ) { + const inFlight = libraries?.isFetching; + const settledCleanly = libraries?.isLoaded && !libraries.fetchError; + if (!inFlight && !settledCleanly) { thunkDispatch(actions.fetchLibraries()).catch(() => {}); } }); From 8e15f86b0dae8ce26429ffae32e5542bccf31d3a Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Sat, 19 Sep 2026 18:26:53 -0400 Subject: [PATCH 5/8] CI AI code review feedback --- src/reducers/libraries.ts | 13 ++++++++++++- src/utils/allLibraries.ts | 11 +++++++---- tests/jest/reducers/libraries.test.ts | 20 ++++++++++++++++++++ tests/jest/utils/allLibraries.test.ts | 15 +++++++++++++++ 4 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/reducers/libraries.ts b/src/reducers/libraries.ts index 78b19e4e10..72135d9a37 100644 --- a/src/reducers/libraries.ts +++ b/src/reducers/libraries.ts @@ -27,7 +27,18 @@ const librariesAction = (action: string) => export default (state: LibrariesState | undefined, action): LibrariesState => { const next: LibrariesState = fetchEditReducer(state, action); if (action.type === librariesAction(ActionCreator.REQUEST)) { - return { ...next, lastFetchError: state?.fetchError ?? null }; + // Keep the already-loaded list visible while a refetch is in flight + // (the Libraries tab refetches on every config-page mount and after + // every save), so consumers do not flip back to "loading". Also fall + // back to the already-retained failure so that a second request + // starting before the first settles (e.g. the header's fetch and the + // Libraries tab's fetch overlap) does not discard it. + return { + ...next, + data: state?.data ?? null, + isLoaded: !!state?.data, + lastFetchError: state?.fetchError ?? state?.lastFetchError ?? null, + }; } if ( action.type === librariesAction(ActionCreator.FAILURE) || diff --git a/src/utils/allLibraries.ts b/src/utils/allLibraries.ts index 40b5c2fce7..8a5d5c36b6 100644 --- a/src/utils/allLibraries.ts +++ b/src/utils/allLibraries.ts @@ -24,17 +24,20 @@ export function settledAllLibraries(state): { } { const libraries = state.editor.libraries; if ( - !libraries?.data && !libraries?.isLoaded && !libraries?.fetchError && !libraries?.lastFetchError ) { return {}; } + // With a loaded list in hand, a failure recorded by a concurrent or + // later request is not worth blocking the UI over; show the list. + const loaded = libraries.data?.libraries; return { - allLibraries: libraries.data?.libraries ?? [], - allLibrariesError: - libraries.fetchError ?? libraries.lastFetchError ?? undefined, + allLibraries: loaded ?? [], + allLibrariesError: loaded + ? undefined + : (libraries.fetchError ?? libraries.lastFetchError ?? undefined), }; } diff --git a/tests/jest/reducers/libraries.test.ts b/tests/jest/reducers/libraries.test.ts index f1035cb38f..5223b8036c 100644 --- a/tests/jest/reducers/libraries.test.ts +++ b/tests/jest/reducers/libraries.test.ts @@ -30,6 +30,26 @@ describe("libraries reducer", () => { expect(retrying.isLoaded).toBe(false); }); + it("keeps the loaded list while a refetch is in flight", () => { + const data = { libraries: [{ short_name: "nypl" }] }; + let state = libraries(undefined, { type: LOAD, data }); + state = libraries(state, { type: REQUEST }); + + expect(state.isFetching).toBe(true); + expect(state.data).toEqual(data); + expect(state.isLoaded).toBe(true); + }); + + it("keeps lastFetchError when a second request starts before the retry settles", () => { + // The header's fetch and the Libraries tab's fetch can overlap. + let state = libraries(undefined, { type: FAILURE, error: fetchError }); + state = libraries(state, { type: REQUEST }); + state = libraries(state, { type: REQUEST }); + + expect(state.fetchError).toBeNull(); + expect(state.lastFetchError).toEqual(fetchError); + }); + it("keeps lastFetchError through a retry's SUCCESS until LOAD", () => { let state = libraries(undefined, { type: FAILURE, error: fetchError }); state = libraries(state, { type: REQUEST }); diff --git a/tests/jest/utils/allLibraries.test.ts b/tests/jest/utils/allLibraries.test.ts index 4a36a17e18..02987ac078 100644 --- a/tests/jest/utils/allLibraries.test.ts +++ b/tests/jest/utils/allLibraries.test.ts @@ -26,6 +26,21 @@ describe("settledAllLibraries", () => { ).toEqual({ allLibraries: libraries, allLibrariesError: undefined }); }); + it("does not report an error when a loaded list is present", () => { + // Two overlapping requests can leave a loaded list next to a recorded + // failure; the list wins. + const fetchError = { + status: 500, + response: "nope", + url: "/admin/libraries", + }; + expect( + settledAllLibraries( + stateWith({ data: { libraries }, isLoaded: true, fetchError }) + ) + ).toEqual({ allLibraries: libraries, allLibrariesError: undefined }); + }); + it("settles to an empty list plus the error on failure", () => { const fetchError = { status: 500, From 059541f5389af3c83722648fcc1e3956de7d8298 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Sat, 19 Sep 2026 19:15:31 -0400 Subject: [PATCH 6/8] Hint at library refresh fetch error --- src/components/IndividualAdminEditForm.tsx | 9 +++++++- src/components/ServiceEditForm.tsx | 6 ++++++ src/interfaces.ts | 4 ++++ src/utils/allLibraries.ts | 14 +++++++++---- .../IndividualAdminEditForm.test.tsx | 21 +++++++++++++++++++ .../jest/components/ServiceEditForm.test.tsx | 19 +++++++++++++++++ tests/jest/utils/allLibraries.test.ts | 13 ++++++++---- 7 files changed, 77 insertions(+), 9 deletions(-) diff --git a/src/components/IndividualAdminEditForm.tsx b/src/components/IndividualAdminEditForm.tsx index 9a4e50356a..c42ecd294f 100644 --- a/src/components/IndividualAdminEditForm.tsx +++ b/src/components/IndividualAdminEditForm.tsx @@ -152,7 +152,8 @@ export default class IndividualAdminEditForm extends React.Component< renderRoleForm() { // Wait for the sitewide library list before showing per-library roles, // mirroring the Libraries panel in ServiceEditForm. - const { allLibraries, allLibrariesError } = this.props.data; + const { allLibraries, allLibrariesError, allLibrariesRefreshError } = + this.props.data; return (
Roles @@ -177,6 +178,12 @@ export default class IndividualAdminEditForm extends React.Component< : "The library list failed to load. Sitewide roles can still be assigned, but per-library roles cannot."} )} + {allLibrariesRefreshError && ( + + The library list could not be refreshed. Showing the last loaded + list, which may be out of date. + + )} {/* For an existing admin a failed load disables all role edits, so an empty table shell would only add noise; drop it. */} {allLibraries && diff --git a/src/components/ServiceEditForm.tsx b/src/components/ServiceEditForm.tsx index 9d76bc3bff..a283df4a4e 100644 --- a/src/components/ServiceEditForm.tsx +++ b/src/components/ServiceEditForm.tsx @@ -386,6 +386,12 @@ export default class ServiceEditForm< short name only, and libraries cannot be added. )} + {this.props.data.allLibrariesRefreshError && ( + + The library list could not be refreshed. Showing the last loaded + list, which may be out of date. + + )} {this.props.data.allLibraries.length === 0 && !this.props.data.allLibrariesError && (

No libraries are configured.

diff --git a/src/interfaces.ts b/src/interfaces.ts index d5845c6242..dd841ee5ee 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -392,6 +392,8 @@ export interface ServicesData { allLibraries?: LibraryData[]; /** Set when the sitewide library list failed to load. */ allLibrariesError?: FetchErrorData; + /** Set when the list is loaded but a later refresh of it failed. */ + allLibrariesRefreshError?: FetchErrorData; } export interface ServicesWithRegistrationsData extends ServicesData { @@ -434,6 +436,8 @@ export interface IndividualAdminsData { allLibraries?: LibraryData[]; /** Set when the sitewide library list failed to load. */ allLibrariesError?: FetchErrorData; + /** Set when the list is loaded but a later refresh of it failed. */ + allLibrariesRefreshError?: FetchErrorData; } export interface PatronAuthServiceData extends ServiceData {} diff --git a/src/utils/allLibraries.ts b/src/utils/allLibraries.ts index 8a5d5c36b6..12cc3bad0c 100644 --- a/src/utils/allLibraries.ts +++ b/src/utils/allLibraries.ts @@ -15,12 +15,17 @@ import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces * reducer keeps the old failure as lastFetchError), so the previous error * stays visible while the retry runs. * + * A failure with no list at all is blocking (allLibrariesError); a failure + * recorded while a loaded list is in hand only means the list may be out of + * date (allLibrariesRefreshError). + * * Merge the result into the `data` prop built by a config page's * mapStateToProps. */ export function settledAllLibraries(state): { allLibraries?: LibraryData[]; allLibrariesError?: FetchErrorData; + allLibrariesRefreshError?: FetchErrorData; } { const libraries = state.editor.libraries; if ( @@ -31,13 +36,14 @@ export function settledAllLibraries(state): { return {}; } // With a loaded list in hand, a failure recorded by a concurrent or - // later request is not worth blocking the UI over; show the list. + // later request is not worth blocking the UI over; show the list and + // report the failure as a non-blocking refresh error instead. const loaded = libraries.data?.libraries; + const error = libraries.fetchError ?? libraries.lastFetchError ?? undefined; return { allLibraries: loaded ?? [], - allLibrariesError: loaded - ? undefined - : (libraries.fetchError ?? libraries.lastFetchError ?? undefined), + allLibrariesError: loaded ? undefined : error, + allLibrariesRefreshError: loaded ? error : undefined, }; } diff --git a/tests/jest/components/IndividualAdminEditForm.test.tsx b/tests/jest/components/IndividualAdminEditForm.test.tsx index a1d8e05b7b..13da572674 100644 --- a/tests/jest/components/IndividualAdminEditForm.test.tsx +++ b/tests/jest/components/IndividualAdminEditForm.test.tsx @@ -254,6 +254,27 @@ describe("IndividualAdminEditForm - rendered inputs and role changes", () => { expect(container.querySelector("tbody")).toBeEmptyDOMElement(); }); + it("warns when the library list could not be refreshed, without blocking edits", () => { + renderForm({ + item: adminData, + data: { + individualAdmins: [adminData], + allLibraries, + allLibrariesRefreshError: { + status: 500, + response: "nope", + url: "/admin/libraries", + }, + }, + }); + expect( + screen.getByText(/The library list could not be refreshed/) + ).toBeInTheDocument(); + // The stale list still renders and roles stay editable. + expect(roleCheckbox("system")).toBeEnabled(); + expect(roleCheckbox("manager-nypl")).toBeEnabled(); + }); + it("says when no libraries are configured", () => { renderForm({ data: { individualAdmins: [adminData], allLibraries: [] }, diff --git a/tests/jest/components/ServiceEditForm.test.tsx b/tests/jest/components/ServiceEditForm.test.tsx index 81eb392a23..0804ce3094 100644 --- a/tests/jest/components/ServiceEditForm.test.tsx +++ b/tests/jest/components/ServiceEditForm.test.tsx @@ -536,6 +536,25 @@ describe("ServiceEditForm", () => { expect(editable[0]).toHaveTextContent("nypl"); }); + it("warns when the library list could not be refreshed", () => { + const staleData = Object.assign({}, servicesData, { + allLibrariesRefreshError: { + status: 500, + response: "nope", + url: "/admin/libraries", + }, + }); + const { container } = renderForm({ data: staleData, item: serviceData }); + expect(container.querySelector(".alert-warning")).toHaveTextContent( + "The library list could not be refreshed" + ); + // The stale list still renders in full, links included. + const editable = container.querySelectorAll(".with-edit-button"); + expect(editable).toHaveLength(1); + expect(editable[0]).toHaveTextContent("New York Public Library - nypl"); + expect(container.querySelector(".alert-danger")).toBeNull(); + }); + it("says when no libraries are configured", () => { const emptyData = Object.assign({}, servicesData, { allLibraries: [] }); const { container } = renderForm({ data: emptyData }); diff --git a/tests/jest/utils/allLibraries.test.ts b/tests/jest/utils/allLibraries.test.ts index 02987ac078..fd74be94d7 100644 --- a/tests/jest/utils/allLibraries.test.ts +++ b/tests/jest/utils/allLibraries.test.ts @@ -26,9 +26,10 @@ describe("settledAllLibraries", () => { ).toEqual({ allLibraries: libraries, allLibrariesError: undefined }); }); - it("does not report an error when a loaded list is present", () => { - // Two overlapping requests can leave a loaded list next to a recorded - // failure; the list wins. + it("reports a failure next to a loaded list as a refresh error", () => { + // Two overlapping requests, or a failed refresh of a loaded list, can + // leave a list next to a recorded failure; the list wins and the + // failure downgrades to a non-blocking refresh error. const fetchError = { status: 500, response: "nope", @@ -38,7 +39,11 @@ describe("settledAllLibraries", () => { settledAllLibraries( stateWith({ data: { libraries }, isLoaded: true, fetchError }) ) - ).toEqual({ allLibraries: libraries, allLibrariesError: undefined }); + ).toEqual({ + allLibraries: libraries, + allLibrariesError: undefined, + allLibrariesRefreshError: fetchError, + }); }); it("settles to an empty list plus the error on failure", () => { From 1df9580466519637fad40c477d03aeb566523af6 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Sun, 20 Sep 2026 14:34:59 -0400 Subject: [PATCH 7/8] CI AI code review feedback --- src/components/CatalogServices.tsx | 1 + src/components/Collections.tsx | 3 +- src/components/DiscoveryServices.tsx | 8 +- src/components/EditableConfigList.tsx | 53 ++++++++- src/components/IndividualAdminEditForm.tsx | 29 +++-- src/components/IndividualAdmins.tsx | 25 +++- src/components/LibrariesLoadStatus.tsx | 21 ++-- src/components/LibrariesRefreshWarning.tsx | 30 +++++ src/components/LibraryRegistration.tsx | 14 +++ src/components/MetadataServices.tsx | 1 + src/components/PatronAuthServices.tsx | 1 + src/components/ServiceEditForm.tsx | 36 +++--- src/interfaces.ts | 20 ++-- src/reducers/libraries.ts | 53 +++++++-- src/utils/allLibraries.ts | 57 +++++---- .../components/DiscoveryServices.test.tsx | 16 +++ .../components/EditableConfigList.test.tsx | 76 +++++++++++- .../IndividualAdminEditForm.test.tsx | 58 ++++++---- .../jest/components/IndividualAdmins.test.tsx | 78 +++++++++++++ .../components/LibraryRegistration.test.tsx | 18 +++ .../jest/components/ServiceEditForm.test.tsx | 16 ++- tests/jest/reducers/libraries.test.ts | 85 +++++++++++++- tests/jest/utils/allLibraries.test.ts | 108 +++++++++++++++++- 23 files changed, 682 insertions(+), 125 deletions(-) create mode 100644 src/components/LibrariesRefreshWarning.tsx diff --git a/src/components/CatalogServices.tsx b/src/components/CatalogServices.tsx index 9b9028860f..1675df21e3 100644 --- a/src/components/CatalogServices.tsx +++ b/src/components/CatalogServices.tsx @@ -21,6 +21,7 @@ export class CatalogServices extends EditableConfigList< > { EditForm = ServiceEditForm; listDataKey = "catalog_services"; + usesLibraryList = true; itemTypeName = "catalog service"; urlBase = "/admin/web/config/catalogServices/"; identifierKey = "id"; diff --git a/src/components/Collections.tsx b/src/components/Collections.tsx index 4408ba6b23..70d009ca93 100644 --- a/src/components/Collections.tsx +++ b/src/components/Collections.tsx @@ -115,6 +115,7 @@ export class Collections extends GenericEditableConfigList< > { EditForm = CollectionEditForm; listDataKey = "collections"; + usesLibraryList = true; itemTypeName = "collection"; urlBase = "/admin/web/config/collections/"; identifierKey = "id"; @@ -147,7 +148,7 @@ export class Collections extends GenericEditableConfigList< } protected getAllLibraries() { - return this.props.data?.allLibraries ?? []; + return this.props.data.allLibraries; } componentDidMount() { diff --git a/src/components/DiscoveryServices.tsx b/src/components/DiscoveryServices.tsx index e697c2cff9..0b9067ef39 100644 --- a/src/components/DiscoveryServices.tsx +++ b/src/components/DiscoveryServices.tsx @@ -49,6 +49,7 @@ export class DiscoveryServices extends GenericEditableConfigList< > { EditForm = DiscoveryServiceEditForm; listDataKey = "discovery_services"; + usesLibraryList = true; itemTypeName = "discovery service"; urlBase = "/admin/web/config/discovery/"; identifierKey = "id"; @@ -80,7 +81,7 @@ export class DiscoveryServices extends GenericEditableConfigList< private registeredLibraries( item: DiscoveryServiceData ): LibraryDataWithStatus[] | undefined { - const registrations = this.props.data?.libraryRegistrations; + const registrations = this.props.data.libraryRegistrations; if (!registrations) return undefined; const serviceReg = registrations.find((r) => r.id === item.id); return (serviceReg?.libraries ?? []).filter((l) => l.status === "success"); @@ -99,7 +100,10 @@ export class DiscoveryServices extends GenericEditableConfigList< ): Array<{ label: string; suffix?: string; href?: string }> | undefined { const registered = this.registeredLibraries(item); if (registered === undefined) return undefined; - const allLibraries = this.props.data?.allLibraries ?? []; + const allLibraries = this.getAllLibraries(); + // Hold the panel until the sitewide list settles, so labels render + // once, in their final linked form. + if (!allLibraries) return undefined; return registered.map((lib) => { const meta = allLibraries.find((l) => l.short_name === lib.short_name); return { diff --git a/src/components/EditableConfigList.tsx b/src/components/EditableConfigList.tsx index 7f7c3474fb..bffadc37cd 100644 --- a/src/components/EditableConfigList.tsx +++ b/src/components/EditableConfigList.tsx @@ -15,6 +15,8 @@ import Admin from "../models/Admin"; import * as PropTypes from "prop-types"; import { navigateTo } from "../utils/navigate"; import { libraryConfigHref, libraryLabel } from "../utils/sharedFunctions"; +import LibrariesRefreshWarning from "./LibrariesRefreshWarning"; +import LibrariesLoadStatus from "./LibrariesLoadStatus"; export interface EditableConfigListStateProps { data?: T; @@ -111,6 +113,9 @@ export abstract class GenericEditableConfigList< abstract labelKey: string; adminLevel?: number; limitOne = false; + /** True on lists whose data merges settledAllLibraries; gates the + * library-list status line in list mode. */ + usesLibraryList = false; links?: { [key: string]: JSX.Element }; AdditionalContent?: new ( props: AdditionalContentProps @@ -177,6 +182,30 @@ export abstract class GenericEditableConfigList< {this.props.fetchError && !this.props.editOrCreate && ( )} + {/* In list mode only; the edit and create forms raise their own + library-list status line and failure alerts. The status line is + additionally gated on usesLibraryList, since lists that never + merge settledAllLibraries would read as loading forever. */} + {!this.props.editOrCreate && this.usesLibraryList && ( + + )} + {(this.props.data as any)?.allLibrariesError && + !this.props.editOrCreate && ( + {this.librariesUnavailableMessage()} + )} + {!this.props.editOrCreate && ( + + )} {this.props.formError && this.props.editOrCreate && ( )} @@ -267,7 +296,8 @@ export abstract class GenericEditableConfigList< /** * Returns the full list of libraries known to the server, used to resolve - * short names to display names and UUIDs for the associated-items panel. + * short names to display names and UUIDs for the associated-items panel, + * or undefined while that list has not settled yet. * * The base implementation accesses `data.allLibraries` via an `any` cast * because the generic `T` is not constrained to include that field (e.g. @@ -275,8 +305,8 @@ export abstract class GenericEditableConfigList< * `allLibraries` (e.g. `Collections`, `IndividualAdmins`) should override * this method with a type-safe accessor to avoid the cast. */ - protected getAllLibraries(): LibraryData[] { - return (this.props.data as any)?.allLibraries ?? []; + protected getAllLibraries(): LibraryData[] | undefined { + return (this.props.data as any).allLibraries; } /** @@ -285,6 +315,15 @@ export abstract class GenericEditableConfigList< * subclasses that use different terminology (e.g. "registered libraries", * "roles"). */ + /** + * Message for the list-mode alert shown when the sitewide library list + * failed to load. Override where the disclosure panel lists something + * other than libraries (see IndividualAdmins). + */ + protected librariesUnavailableMessage(): string { + return "The library list failed to load. Associated libraries are shown by short name only."; + } + protected formatAssociatedCount(count: number): string { return count === 0 ? "no libraries" @@ -298,7 +337,9 @@ export abstract class GenericEditableConfigList< * for a given item, or `undefined` if the panel does not apply to this item. * * Return semantics (used by `renderLi` to drive toggle visibility): - * - `undefined` → the feature does not apply; no toggle is rendered. + * - `undefined` → the feature does not apply to this item, or the + * sitewide library list has not settled yet; no toggle + * or count is rendered. * - `[]` → the feature applies but there are no associations; * a disabled toggle is rendered. * - `[…entries]` → associations exist; an enabled toggle is rendered. @@ -318,6 +359,10 @@ export abstract class GenericEditableConfigList< ?.libraries; if (libraries === undefined) return undefined; const allLibraries = this.getAllLibraries(); + // Hold the panel until the sitewide list settles, so labels render + // once, in their final linked form, instead of flashing bare short + // names that get rewritten when the list arrives. + if (!allLibraries) return undefined; return libraries.map((lib) => { const libraryData = allLibraries.find( (l) => l.short_name === lib.short_name diff --git a/src/components/IndividualAdminEditForm.tsx b/src/components/IndividualAdminEditForm.tsx index c42ecd294f..18831a8fdd 100644 --- a/src/components/IndividualAdminEditForm.tsx +++ b/src/components/IndividualAdminEditForm.tsx @@ -11,6 +11,7 @@ import Admin from "../models/Admin"; import { Panel, Form } from "library-simplified-reusable-components"; import { Alert } from "react-bootstrap"; import LibrariesLoadStatus from "./LibrariesLoadStatus"; +import LibrariesRefreshWarning from "./LibrariesRefreshWarning"; import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; @@ -170,6 +171,7 @@ export default class IndividualAdminEditForm extends React.Component< {allLibrariesError && ( @@ -178,12 +180,10 @@ export default class IndividualAdminEditForm extends React.Component< : "The library list failed to load. Sitewide roles can still be assigned, but per-library roles cannot."} )} - {allLibrariesRefreshError && ( - - The library list could not be refreshed. Showing the last loaded - list, which may be out of date. - - )} + {/* For an existing admin a failed load disables all role edits, so an empty table shell would only add noise; drop it. */} {allLibraries && @@ -309,15 +309,26 @@ export default class IndividualAdminEditForm extends React.Component< if (this.props.disabled) { return true; } - // Sitewide toggles rewrite an existing admin's (hidden) per-library - // roles wholesale, so edits stay disabled until the library list is - // available. A new admin has no roles yet, so nothing can be clobbered. + // An existing admin is fully locked while the list is missing or + // failed to load: their per-library roles cannot be shown, and the + // sitewide toggles would rewrite those hidden roles wholesale. if ( this.props.item && (!this.props.data.allLibraries || this.props.data.allLibrariesError) ) { return true; } + // Only the per-library toggles rebuild roles from the list (their + // un-check branches expand sitewide roles using it), so a stale list + // locks just those. The sitewide toggles replace the role set without + // consulting the list and stay assignable, so the form cannot be + // reduced to submitting a roleless admin. + if ( + this.props.data.allLibrariesRefreshError && + (role === "manager" || role === "librarian") + ) { + return true; + } if (role === "system" || this.isSelected("system")) { return !this.context.admin.isSystemAdmin(); } diff --git a/src/components/IndividualAdmins.tsx b/src/components/IndividualAdmins.tsx index c02ea8cc58..ff25f0342b 100644 --- a/src/components/IndividualAdmins.tsx +++ b/src/components/IndividualAdmins.tsx @@ -14,6 +14,7 @@ import { IndividualAdminsData, IndividualAdminData, AdminRoleData, + LibraryData, } from "../interfaces"; import Admin from "../models/Admin"; import { libraryConfigHref, libraryLabel } from "../utils/sharedFunctions"; @@ -28,6 +29,7 @@ export class IndividualAdmins extends EditableConfigList< > { EditForm = IndividualAdminEditForm; listDataKey = "individualAdmins"; + usesLibraryList = true; itemTypeName = "individual admin"; urlBase = "/admin/web/config/individualAdmins/"; identifierKey = "email"; @@ -38,14 +40,16 @@ export class IndividualAdmins extends EditableConfigList< admin: PropTypes.object.isRequired, }; - private getRolesSummary(item: IndividualAdminData): Array<{ + private getRolesSummary( + item: IndividualAdminData, + allLibraries: LibraryData[] + ): Array<{ label: string; suffix?: string; href?: string; pinned?: boolean; }> { const roles: AdminRoleData[] = item.roles || []; - const allLibraries = this.getAllLibraries(); const getLibraryLabel = (shortName: string) => libraryLabel( @@ -105,7 +109,12 @@ export class IndividualAdmins extends EditableConfigList< } protected getAllLibraries() { - return this.props.data?.allLibraries ?? []; + return this.props.data.allLibraries; + } + + // This tab's disclosure lists roles, not libraries. + protected librariesUnavailableMessage(): string { + return "The library list failed to load. Roles are shown by library short name only."; } protected formatAssociatedCount(count: number): string { @@ -118,12 +127,16 @@ export class IndividualAdmins extends EditableConfigList< | Array<{ label: string; suffix?: string; href?: string; pinned?: boolean }> | undefined { if (!item.roles) return undefined; - // System admins have a single implicit role that isn't library-scoped; - // show a synthetic "sysadmin" entry rather than the library-role summary. + // System admins have a single implicit role that isn't library-scoped + // and can never be rewritten by the list; render it immediately. if (item.roles.some((r) => r.role === "system")) { return [{ label: "sysadmin" }]; } - return this.getRolesSummary(item); + // Hold library-scoped rows until the sitewide list settles, so labels + // render once, in their final linked form. + const allLibraries = this.getAllLibraries(); + if (!allLibraries) return undefined; + return this.getRolesSummary(item, allLibraries); } canCreate() { diff --git a/src/components/LibrariesLoadStatus.tsx b/src/components/LibrariesLoadStatus.tsx index 097a3ce4e3..47e8eab662 100644 --- a/src/components/LibrariesLoadStatus.tsx +++ b/src/components/LibrariesLoadStatus.tsx @@ -1,29 +1,30 @@ import * as React from "react"; -import { LibraryData } from "../interfaces"; -import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; +import { AllLibrariesData } from "../interfaces"; -export interface LibrariesLoadStatusProps { - allLibraries?: LibraryData[]; - allLibrariesError?: FetchErrorData; -} +export type LibrariesLoadStatusProps = AllLibrariesData; /** * Live status line for the sitewide library list load. It stays mounted so * that the settled text is a content change, which screen readers announce - * (content present at mount is not announced). The settled text is visually - * hidden; the surrounding UI shows the outcome. + * (content present at mount is not announced). The settled text is + * visually hidden; the surrounding UI shows the outcome. On the failure + * paths the text empties instead: the adjacent Alert renders role="alert" + * and announces the outcome itself, so a status text there would be read + * twice. This region covers the one transition nothing else announces, + * loading to cleanly loaded. */ export default function LibrariesLoadStatus({ allLibraries, allLibrariesError, + allLibrariesRefreshError, }: LibrariesLoadStatusProps): JSX.Element { const loading = !allLibraries; return (

{loading ? "Loading libraries..." - : allLibrariesError - ? "Libraries failed to load." + : allLibrariesError || allLibrariesRefreshError + ? "" : "Libraries loaded."}

); diff --git a/src/components/LibrariesRefreshWarning.tsx b/src/components/LibrariesRefreshWarning.tsx new file mode 100644 index 0000000000..5eafb2dba1 --- /dev/null +++ b/src/components/LibrariesRefreshWarning.tsx @@ -0,0 +1,30 @@ +import * as React from "react"; +import { Alert } from "react-bootstrap"; +import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; + +export interface LibrariesRefreshWarningProps { + allLibrariesRefreshError?: FetchErrorData; + detail?: string; +} + +/** + * Warning that the sitewide library list is being served from its last + * loaded copy because a refresh failed. Renders nothing while there is no + * refresh error. `detail` appends a panel-specific consequence to the + * shared wording. + */ +export default function LibrariesRefreshWarning({ + allLibrariesRefreshError, + detail, +}: LibrariesRefreshWarningProps): JSX.Element | null { + if (!allLibrariesRefreshError) { + return null; + } + return ( + + The library list could not be refreshed. Showing the last loaded list, + which may be out of date. + {detail ? ` ${detail}` : ""} + + ); +} diff --git a/src/components/LibraryRegistration.tsx b/src/components/LibraryRegistration.tsx index b5e9fc4f4d..99df644296 100644 --- a/src/components/LibraryRegistration.tsx +++ b/src/components/LibraryRegistration.tsx @@ -9,6 +9,7 @@ import { LibraryData, } from "../interfaces"; import LibraryConfigLink from "./LibraryConfigLink"; +import { Alert } from "react-bootstrap"; export interface LibraryRegistrationState { registration_stage?: { [key: string]: string } | null; @@ -56,6 +57,19 @@ export default class LibraryRegistration extends React.Component< } render(): JSX.Element { + // The Libraries panel above this section explains a failed list load; + // say why registration is unavailable too rather than vanishing. + if ( + this.props.item && + this.protocolSupportsType("supports_registration") && + this.props.data.allLibrariesError + ) { + return ( + + Libraries cannot be registered: the library list is unavailable. + + ); + } if ( this.props.item && this.protocolSupportsType("supports_registration") && diff --git a/src/components/MetadataServices.tsx b/src/components/MetadataServices.tsx index e9a9930de6..599fd0b57c 100644 --- a/src/components/MetadataServices.tsx +++ b/src/components/MetadataServices.tsx @@ -22,6 +22,7 @@ export class MetadataServices extends EditableConfigList< > { EditForm = ServiceEditForm; listDataKey = "metadata_services"; + usesLibraryList = true; itemTypeName = "metadata service"; urlBase = "/admin/web/config/metadata/"; identifierKey = "id"; diff --git a/src/components/PatronAuthServices.tsx b/src/components/PatronAuthServices.tsx index 39f686cfda..e85610b388 100644 --- a/src/components/PatronAuthServices.tsx +++ b/src/components/PatronAuthServices.tsx @@ -26,6 +26,7 @@ export class PatronAuthServices extends EditableConfigList< ExtraFormSection = NeighborhoodAnalyticsForm; extraFormKey = "neighborhood_mode"; listDataKey = "patron_auth_services"; + usesLibraryList = true; itemTypeName = "patron authentication service"; urlBase = "/admin/web/config/patronAuth/"; identifierKey = "id"; diff --git a/src/components/ServiceEditForm.tsx b/src/components/ServiceEditForm.tsx index a283df4a4e..bf72af55f5 100644 --- a/src/components/ServiceEditForm.tsx +++ b/src/components/ServiceEditForm.tsx @@ -14,6 +14,7 @@ import { import { clearForm, libraryLabel } from "../utils/sharedFunctions"; import LibraryConfigLink from "./LibraryConfigLink"; import LibrariesLoadStatus from "./LibrariesLoadStatus"; +import LibrariesRefreshWarning from "./LibrariesRefreshWarning"; import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; import { Alert } from "react-bootstrap"; @@ -383,15 +384,13 @@ export default class ServiceEditForm< {this.props.data.allLibrariesError && ( The library list failed to load. Associated libraries are shown by - short name only, and libraries cannot be added. - - )} - {this.props.data.allLibrariesRefreshError && ( - - The library list could not be refreshed. Showing the last loaded - list, which may be out of date. + short name only, and library associations cannot be added or + removed. )} + {this.props.data.allLibraries.length === 0 && !this.props.data.allLibrariesError && (

No libraries are configured.

@@ -400,13 +399,14 @@ export default class ServiceEditForm< {this.state.libraries.map((library) => (
this.removeLibrary(library)} confirmRemoval={() => this.isLibraryRemovalPermitted(library)} ref={library.short_name} > - {this.props.data && - this.props.data.protocols && + {this.props.data.protocols && this.protocolHasLibrarySettings(protocol) && ( )} {!( - this.props.data && this.props.data.protocols && this.protocolHasLibrarySettings(protocol) ) && this.renderLibraryLabel(library.short_name)} {this.isExpanded(library) && (
- {this.props.data && - this.props.data.protocols && + {this.props.data.protocols && this.protocolLibrarySettings(protocol) && this.protocolLibrarySettings(protocol).map((setting) => ( {this.state.selectedLibrary && (
- {this.props.data && - this.props.data.protocols && + {this.props.data.protocols && this.protocolLibrarySettings(protocol) && this.protocolLibrarySettings(protocol).map((setting) => ( {librariesFieldset} @@ -630,7 +628,9 @@ export default class ServiceEditForm< } getLibrary(shortName: string): LibraryData { - const libraries = (this.props.data && this.props.data.allLibraries) || []; + // Only called from inside the Libraries panel, which renders after + // allLibraries has settled. + const libraries = this.props.data.allLibraries; for (const library of libraries) { if (library.short_name === shortName) { return library; @@ -653,7 +653,9 @@ export default class ServiceEditForm< } availableLibraries(): LibraryData[] { - const libraries = (this.props.data && this.props.data.allLibraries) || []; + // Only called from inside the Libraries panel, which renders after + // allLibraries has settled. + const libraries = this.props.data.allLibraries; return libraries.filter((library) => { for (const stateLibrary of this.state.libraries) { if (stateLibrary.short_name === library.short_name) { diff --git a/src/interfaces.ts b/src/interfaces.ts index dd841ee5ee..23f31350b2 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -386,8 +386,12 @@ export interface ServiceData { goal?: string; } -export interface ServicesData { - protocols: ProtocolData[]; +/** + * The sitewide library list fields produced by settledAllLibraries and + * merged into a config page's data. Extended by each data type whose page + * resolves library short names against the list. + */ +export interface AllLibrariesData { /** The sitewide library list; undefined until its request settles. */ allLibraries?: LibraryData[]; /** Set when the sitewide library list failed to load. */ @@ -396,6 +400,10 @@ export interface ServicesData { allLibrariesRefreshError?: FetchErrorData; } +export interface ServicesData extends AllLibrariesData { + protocols: ProtocolData[]; +} + export interface ServicesWithRegistrationsData extends ServicesData { libraryRegistrations?: LibraryRegistrationData[]; } @@ -430,14 +438,8 @@ export interface IndividualAdminData { roles?: AdminRoleData[]; } -export interface IndividualAdminsData { +export interface IndividualAdminsData extends AllLibrariesData { individualAdmins?: IndividualAdminData[]; - /** The sitewide library list; undefined until its request settles. */ - allLibraries?: LibraryData[]; - /** Set when the sitewide library list failed to load. */ - allLibrariesError?: FetchErrorData; - /** Set when the list is loaded but a later refresh of it failed. */ - allLibrariesRefreshError?: FetchErrorData; } export interface PatronAuthServiceData extends ServiceData {} diff --git a/src/reducers/libraries.ts b/src/reducers/libraries.ts index 72135d9a37..0e1cf5106d 100644 --- a/src/reducers/libraries.ts +++ b/src/reducers/libraries.ts @@ -14,6 +14,15 @@ export interface LibrariesState extends FetchEditState { * Cleared when the retry settles (FAILURE or LOAD). */ lastFetchError?: RequestError | null; + /** + * The last loaded list while a refetch is in flight. Kept under its own + * key so that data keeps its normal request lifecycle for the Libraries + * config page (whose edit form must unmount during the post-save + * refetch); settledAllLibraries serves this copy so the other config + * tabs do not flip back to "loading". Kept through a FAILURE so a stale + * copy stays available; cleared when a fresh list LOADs. + */ + lastData?: LibrariesData | null; } const fetchEditReducer = createFetchEditReducer( @@ -24,27 +33,49 @@ const fetchEditReducer = createFetchEditReducer( const librariesAction = (action: string) => `${ActionCreator.LIBRARIES}_${action}`; +const editLibraryAction = (action: string) => + `${ActionCreator.EDIT_LIBRARY}_${action}`; + export default (state: LibrariesState | undefined, action): LibrariesState => { const next: LibrariesState = fetchEditReducer(state, action); if (action.type === librariesAction(ActionCreator.REQUEST)) { - // Keep the already-loaded list visible while a refetch is in flight - // (the Libraries tab refetches on every config-page mount and after - // every save), so consumers do not flip back to "loading". Also fall - // back to the already-retained failure so that a second request + // Fall back to the already-retained copies so that a second request // starting before the first settles (e.g. the header's fetch and the - // Libraries tab's fetch overlap) does not discard it. + // Libraries tab's fetch overlap) does not discard them. A failure + // recorded beside current data is dropped, not retained: consumers + // already ignore it (see settledAllLibraries), so carrying it would + // resurface it against the retained copy during the refetch. return { ...next, - data: state?.data ?? null, - isLoaded: !!state?.data, - lastFetchError: state?.fetchError ?? state?.lastFetchError ?? null, + lastData: state?.data ?? state?.lastData ?? null, + lastFetchError: state?.data + ? null + : (state?.fetchError ?? state?.lastFetchError ?? null), }; } + if (action.type === librariesAction(ActionCreator.FAILURE)) { + // fetchError now carries the new failure; lastData rides along in + // `next` so consumers can keep serving the stale list. + return { ...next, lastFetchError: null }; + } + if (action.type === librariesAction(ActionCreator.LOAD)) { + return { ...next, lastData: null, lastFetchError: null }; + } if ( - action.type === librariesAction(ActionCreator.FAILURE) || - action.type === librariesAction(ActionCreator.LOAD) + action.type === editLibraryAction(ActionCreator.REQUEST) || + action.type === editLibraryAction(ActionCreator.SUCCESS) ) { - return { ...next, lastFetchError: null }; + // The base reducer clears fetchError when a library form submit starts + // or succeeds; keep a pending list-fetch failure as lastFetchError so + // consumers do not mistake the state for a cleanly loaded list. As in + // the LIBRARIES_REQUEST branch, a failure beside current data is + // dropped, since consumers ignore it. + return { + ...next, + lastFetchError: state?.data + ? null + : (state?.fetchError ?? state?.lastFetchError ?? null), + }; } return next; }; diff --git a/src/utils/allLibraries.ts b/src/utils/allLibraries.ts index 12cc3bad0c..d3cbf5e4a1 100644 --- a/src/utils/allLibraries.ts +++ b/src/utils/allLibraries.ts @@ -3,8 +3,8 @@ // state.editor.libraries. import ActionCreator from "../actions"; -import { LibraryData } from "../interfaces"; -import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces"; +import { AllLibrariesData } from "../interfaces"; +import { LibrariesState } from "../reducers/libraries"; /** * Returns the sitewide library list once its request has settled, or an @@ -15,35 +15,27 @@ import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces * reducer keeps the old failure as lastFetchError), so the previous error * stays visible while the retry runs. * - * A failure with no list at all is blocking (allLibrariesError); a failure - * recorded while a loaded list is in hand only means the list may be out of - * date (allLibrariesRefreshError). + * A failure with no list at all is blocking (allLibrariesError). A failure + * beside the retained copy of the list means the copy may be out of date + * (allLibrariesRefreshError). A failure beside a current list is reported + * as neither: the list on screen is up to date, and concurrent requests + * with mixed outcomes must not degrade a working page. * * Merge the result into the `data` prop built by a config page's * mapStateToProps. */ -export function settledAllLibraries(state): { - allLibraries?: LibraryData[]; - allLibrariesError?: FetchErrorData; - allLibrariesRefreshError?: FetchErrorData; -} { +export function settledAllLibraries(state): AllLibrariesData { const libraries = state.editor.libraries; - if ( - !libraries?.isLoaded && - !libraries?.fetchError && - !libraries?.lastFetchError - ) { + const current = currentLibraries(libraries); + const loaded = retainedLibraries(libraries); + const error = retainedError(libraries); + if (!loaded && !error) { return {}; } - // With a loaded list in hand, a failure recorded by a concurrent or - // later request is not worth blocking the UI over; show the list and - // report the failure as a non-blocking refresh error instead. - const loaded = libraries.data?.libraries; - const error = libraries.fetchError ?? libraries.lastFetchError ?? undefined; return { allLibraries: loaded ?? [], allLibrariesError: loaded ? undefined : error, - allLibrariesRefreshError: loaded ? error : undefined, + allLibrariesRefreshError: loaded && !current ? error : undefined, }; } @@ -57,9 +49,30 @@ export function fetchLibrariesIfNeeded(dispatch, actions: ActionCreator): void { dispatch((thunkDispatch, getState) => { const libraries = getState().editor.libraries; const inFlight = libraries?.isFetching; - const settledCleanly = libraries?.isLoaded && !libraries.fetchError; + const settledCleanly = + !!retainedLibraries(libraries) && !retainedError(libraries); if (!inFlight && !settledCleanly) { thunkDispatch(actions.fetchLibraries()).catch(() => {}); } }); } + +// The predicates settled-ness is derived from, feeding both functions +// above. The two deliberately differ on one state: a failure beside a +// current list is nothing to report for settledAllLibraries, while +// fetchLibrariesIfNeeded still retries it to clear the stray fetchError. +// isLoaded is deliberately not consulted: actions from the shared +// EDIT_LIBRARY prefix can clear fetchError while leaving isLoaded true, +// and isLoaded alone proves neither a list nor an error worth showing. + +/** The current list, from data. */ +const currentLibraries = (libraries?: LibrariesState) => + libraries?.data?.libraries; + +/** The list in hand: the current one, or the copy retained during a refetch. */ +const retainedLibraries = (libraries?: LibrariesState) => + currentLibraries(libraries) ?? libraries?.lastData?.libraries; + +/** The failure in hand: the current one, or the one a retry is retrying. */ +const retainedError = (libraries?: LibrariesState) => + libraries?.fetchError ?? libraries?.lastFetchError ?? undefined; diff --git a/tests/jest/components/DiscoveryServices.test.tsx b/tests/jest/components/DiscoveryServices.test.tsx index 4b64177e9d..f5b2e638ae 100644 --- a/tests/jest/components/DiscoveryServices.test.tsx +++ b/tests/jest/components/DiscoveryServices.test.tsx @@ -52,6 +52,22 @@ describe("DiscoveryServices - registered library disclosure", () => { // ── Toggle visibility ───────────────────────────────────────────────────── + it("shows no toggle while allLibraries has not settled", () => { + // Labels must render once, in final linked form, not flash bare short + // names that get rewritten when the sitewide list arrives. + const { container } = renderServices({ + discovery_services: [{ id: 1, protocol: "p", name: "Service A" } as any], + allLibraries: undefined, + libraryRegistrations: [ + { + id: 1, + libraries: [{ short_name: "alpha", status: "success" }], + }, + ] as any, + }); + expect(container.querySelector(".association-toggle")).toBeNull(); + }); + it("shows no toggle when libraryRegistrations data has not yet loaded", () => { const { container } = renderServices({ discovery_services: [{ id: 1, protocol: "p", name: "Service A" } as any], diff --git a/tests/jest/components/EditableConfigList.test.tsx b/tests/jest/components/EditableConfigList.test.tsx index 73bf1fa6d7..05f4f1df88 100644 --- a/tests/jest/components/EditableConfigList.test.tsx +++ b/tests/jest/components/EditableConfigList.test.tsx @@ -23,6 +23,12 @@ describe("EditableConfigList - library association disclosure", () => { interface ServicesData { services: ServiceItem[]; allLibraries?: Array<{ short_name: string; name?: string; uuid?: string }>; + allLibrariesError?: { status: number; response: string; url: string }; + allLibrariesRefreshError?: { + status: number; + response: string; + url: string; + }; } class TestEditForm extends React.Component< @@ -36,6 +42,7 @@ describe("EditableConfigList - library association disclosure", () => { class TestServiceList extends EditableConfigList { EditForm = TestEditForm; listDataKey = "services"; + usesLibraryList = true; itemTypeName = "service"; urlBase = "/admin/services/"; identifierKey = "id"; @@ -377,13 +384,19 @@ describe("EditableConfigList - library association disclosure", () => { expect(items[1].textContent).toBe("Beta Library - beta"); }); - it("falls back to short_name when allLibraries is absent from the data", () => { + it("shows an alert and bare short names when the library list failed to load", () => { const { container } = renderWithContext( { />, config ); + expect(container.querySelector(".alert-danger")).toHaveTextContent( + "The library list failed to load" + ); + // The associations still render, by short name, below the alert. fireEvent.click(container.querySelector(".association-toggle")); expect(container.querySelector(".associated-items li").textContent).toBe( "nypl" ); }); + + it("shows a warning when the library list is stale", () => { + const { container } = renderWithContext( + , + config + ); + expect(container.querySelector(".alert-warning")).toHaveTextContent( + "The library list could not be refreshed" + ); + // The associations still render, from the retained list. + fireEvent.click(container.querySelector(".association-toggle")); + expect(container.querySelector(".associated-items li").textContent).toBe( + "NYPL - nypl" + ); + }); + + it("holds the association panel until allLibraries settles", () => { + const { container } = renderWithContext( + , + config + ); + // With the sitewide list unsettled, no toggle renders at all, so the + // panel cannot flash bare short names that get rewritten when the + // list arrives. The status line explains and announces the gap. + expect(container.querySelector(".association-toggle")).toBeNull(); + expect(container.querySelector('[role="status"]')).toHaveTextContent( + "Loading libraries..." + ); + }); }); }); diff --git a/tests/jest/components/IndividualAdminEditForm.test.tsx b/tests/jest/components/IndividualAdminEditForm.test.tsx index 13da572674..181afa4da9 100644 --- a/tests/jest/components/IndividualAdminEditForm.test.tsx +++ b/tests/jest/components/IndividualAdminEditForm.test.tsx @@ -234,9 +234,9 @@ describe("IndividualAdminEditForm - rendered inputs and role changes", () => { /library roles cannot be shown, and no roles can be changed/ ) ).toBeInTheDocument(); - expect(container.querySelector('[role="status"]')).toHaveTextContent( - "Libraries failed to load." - ); + // The role="alert" danger Alert announces the failure itself, so the + // status region empties rather than duplicating the announcement. + expect(container.querySelector('[role="status"]')).toBeEmptyDOMElement(); expect(roleCheckbox("system")).toBeDisabled(); // All role edits are disabled, so the empty table shell is dropped. expect(container.querySelector("table.library-admin-roles")).toBeNull(); @@ -254,25 +254,41 @@ describe("IndividualAdminEditForm - rendered inputs and role changes", () => { expect(container.querySelector("tbody")).toBeEmptyDOMElement(); }); - it("warns when the library list could not be refreshed, without blocking edits", () => { - renderForm({ - item: adminData, - data: { - individualAdmins: [adminData], - allLibraries, - allLibrariesRefreshError: { - status: 500, - response: "nope", - url: "/admin/libraries", - }, - }, - }); - expect( - screen.getByText(/The library list could not be refreshed/) - ).toBeInTheDocument(); - // The stale list still renders and roles stay editable. + const staleData = { + individualAdmins: [adminData], + allLibraries, + allLibrariesRefreshError: { + status: 500, + response: "nope", + url: "/admin/libraries", + }, + }; + + it("locks only per-library roles for an existing admin while the list is stale", () => { + const { container } = renderForm({ item: adminData, data: staleData }); + expect(container.querySelector(".alert-warning")).toHaveTextContent( + "Per-library roles cannot be changed until the list can be refreshed." + ); + // The role="alert" warning announces the refresh failure itself, so + // the status region empties rather than duplicating the announcement. + expect(container.querySelector('[role="status"]')).toBeEmptyDOMElement(); + // Only the per-library un-check branches expand sitewide roles from + // the (stale) list; sitewide toggles are list-independent and stay + // assignable, so submitting cannot be forced into a roleless admin. expect(roleCheckbox("system")).toBeEnabled(); - expect(roleCheckbox("manager-nypl")).toBeEnabled(); + expect(roleCheckbox("manager-all")).toBeEnabled(); + expect(roleCheckbox("manager-nypl")).toBeDisabled(); + expect(roleCheckbox("librarian-nypl")).toBeDisabled(); + }); + + it("locks only per-library roles for a new admin while the list is stale", () => { + const { container } = renderForm({ data: staleData }); + expect(container.querySelector(".alert-warning")).toHaveTextContent( + "Per-library roles cannot be changed" + ); + expect(roleCheckbox("system")).toBeEnabled(); + expect(roleCheckbox("manager-all")).toBeEnabled(); + expect(roleCheckbox("manager-nypl")).toBeDisabled(); }); it("says when no libraries are configured", () => { diff --git a/tests/jest/components/IndividualAdmins.test.tsx b/tests/jest/components/IndividualAdmins.test.tsx index 801680fdce..cd62f37f37 100644 --- a/tests/jest/components/IndividualAdmins.test.tsx +++ b/tests/jest/components/IndividualAdmins.test.tsx @@ -46,6 +46,84 @@ describe("IndividualAdmins - role association disclosure", () => { // ── Toggle visibility ───────────────────────────────────────────────────── + it("shows no toggle while allLibraries has not settled", () => { + // Labels must render once, in final linked form, not flash bare short + // names that get rewritten when the sitewide list arrives. + const { container } = renderWithContext( + , + sysAdminConfig + ); + expect(container.querySelector(".association-toggle")).toBeNull(); + }); + + it("shows the sysadmin entry before allLibraries settles", () => { + // The synthetic sysadmin entry never consults the library list and can + // never be rewritten by it, so it is not held back. + const { container } = renderWithContext( + , + sysAdminConfig + ); + fireEvent.click(container.querySelector(".association-toggle")); + expect(container.querySelector(".associated-items li").textContent).toBe( + "sysadmin" + ); + }); + + it("names roles, not libraries, when the library list failed to load", () => { + const { container } = renderWithContext( + , + sysAdminConfig + ); + expect(container.querySelector(".alert-danger")).toHaveTextContent( + "Roles are shown by library short name only." + ); + }); + it("shows no toggle for an admin with no roles field", () => { const { container } = renderAdmins([{ email: "noroles@example.com" }]); expect(container.querySelector(".association-toggle")).toBeNull(); diff --git a/tests/jest/components/LibraryRegistration.test.tsx b/tests/jest/components/LibraryRegistration.test.tsx index 559f8bb057..45866967ef 100644 --- a/tests/jest/components/LibraryRegistration.test.tsx +++ b/tests/jest/components/LibraryRegistration.test.tsx @@ -87,6 +87,24 @@ describe("LibraryRegistration", () => { expect(libraries(container)).toHaveLength(0); }); + it("explains instead of vanishing when the library list failed to load", () => { + const { container } = renderReg({ + item: serviceData, + data: makeData({ + allLibraries: [], + allLibrariesError: { + status: 500, + response: "nope", + url: "/admin/libraries", + }, + }), + }); + expect(libraries(container)).toHaveLength(0); + expect(container.querySelector(".alert-warning")).toHaveTextContent( + "Libraries cannot be registered: the library list is unavailable." + ); + }); + it("doesn't render libraries in edit form if protocol doesn't support registration", () => { const { container } = renderReg({ item: serviceData, diff --git a/tests/jest/components/ServiceEditForm.test.tsx b/tests/jest/components/ServiceEditForm.test.tsx index 0804ce3094..f77689b75b 100644 --- a/tests/jest/components/ServiceEditForm.test.tsx +++ b/tests/jest/components/ServiceEditForm.test.tsx @@ -526,14 +526,15 @@ describe("ServiceEditForm", () => { expect(container.querySelector(".alert-danger")).toHaveTextContent( "The library list failed to load" ); - // The live status must not claim success on failure. - expect(container.querySelector('[role="status"]')).toHaveTextContent( - "Libraries failed to load." - ); + // The role="alert" danger Alert announces the failure itself, so the + // status region empties rather than duplicating the announcement. + expect(container.querySelector('[role="status"]')).toBeEmptyDOMElement(); // The associated library still renders, by short name. const editable = container.querySelectorAll(".with-edit-button"); expect(editable).toHaveLength(1); expect(editable[0]).toHaveTextContent("nypl"); + // A removal could not be undone in-session, so it is disabled too. + expect(container.querySelector("button.remove-btn")).toBeDisabled(); }); it("warns when the library list could not be refreshed", () => { @@ -548,11 +549,16 @@ describe("ServiceEditForm", () => { expect(container.querySelector(".alert-warning")).toHaveTextContent( "The library list could not be refreshed" ); - // The stale list still renders in full, links included. + // The role="alert" warning announces the refresh failure itself, so + // the status region empties rather than duplicating the announcement. + expect(container.querySelector('[role="status"]')).toBeEmptyDOMElement(); + // The stale list still renders in full, links included, and the + // panel stays editable; only the blocking error disables removal. const editable = container.querySelectorAll(".with-edit-button"); expect(editable).toHaveLength(1); expect(editable[0]).toHaveTextContent("New York Public Library - nypl"); expect(container.querySelector(".alert-danger")).toBeNull(); + expect(container.querySelector("button.remove-btn")).toBeEnabled(); }); it("says when no libraries are configured", () => { diff --git a/tests/jest/reducers/libraries.test.ts b/tests/jest/reducers/libraries.test.ts index 5223b8036c..2f9766cffa 100644 --- a/tests/jest/reducers/libraries.test.ts +++ b/tests/jest/reducers/libraries.test.ts @@ -5,6 +5,8 @@ const REQUEST = `${ActionCreator.LIBRARIES}_${ActionCreator.REQUEST}`; const FAILURE = `${ActionCreator.LIBRARIES}_${ActionCreator.FAILURE}`; const SUCCESS = `${ActionCreator.LIBRARIES}_${ActionCreator.SUCCESS}`; const LOAD = `${ActionCreator.LIBRARIES}_${ActionCreator.LOAD}`; +const EDIT_REQUEST = `${ActionCreator.EDIT_LIBRARY}_${ActionCreator.REQUEST}`; +const EDIT_SUCCESS = `${ActionCreator.EDIT_LIBRARY}_${ActionCreator.SUCCESS}`; describe("libraries reducer", () => { const fetchError = { status: 500, response: "nope", url: "/admin/libraries" }; @@ -30,14 +32,47 @@ describe("libraries reducer", () => { expect(retrying.isLoaded).toBe(false); }); - it("keeps the loaded list while a refetch is in flight", () => { + it("retains the loaded list as lastData while a refetch is in flight", () => { const data = { libraries: [{ short_name: "nypl" }] }; let state = libraries(undefined, { type: LOAD, data }); state = libraries(state, { type: REQUEST }); expect(state.isFetching).toBe(true); + // data keeps its normal lifecycle (nulled by REQUEST) for the Libraries + // config page; the retained copy moves to lastData. + expect(state.data).toBeNull(); + expect(state.isLoaded).toBe(false); + expect(state.lastData).toEqual(data); + }); + + it("keeps lastData through a failed refetch and clears it on a fresh load", () => { + const data = { libraries: [{ short_name: "nypl" }] }; + let state = libraries(undefined, { type: LOAD, data }); + state = libraries(state, { type: REQUEST }); + state = libraries(state, { type: FAILURE, error: fetchError }); + expect(state.lastData).toEqual(data); + expect(state.fetchError).toEqual(fetchError); + + state = libraries(state, { type: REQUEST }); + state = libraries(state, { type: SUCCESS }); + state = libraries(state, { type: LOAD, data }); + expect(state.lastData).toBeNull(); expect(state.data).toEqual(data); - expect(state.isLoaded).toBe(true); + }); + + it("drops a failure recorded beside current data when a refetch starts", () => { + // Overlapping requests with mixed outcomes leave data plus fetchError. + // Consumers ignore that failure, so it must not resurface against the + // retained copy while the next refetch runs. + const data = { libraries: [{ short_name: "nypl" }] }; + let state = libraries(undefined, { type: LOAD, data }); + state = libraries(state, { type: FAILURE, error: fetchError }); + expect(state.data).toEqual(data); + expect(state.fetchError).toEqual(fetchError); + + state = libraries(state, { type: REQUEST }); + expect(state.lastData).toEqual(data); + expect(state.lastFetchError).toBeNull(); }); it("keeps lastFetchError when a second request starts before the retry settles", () => { @@ -74,6 +109,52 @@ describe("libraries reducer", () => { expect(state.data).toEqual(data); }); + it("keeps a list-fetch failure when a library form submit starts", () => { + // EDIT_LIBRARY_REQUEST shares this reducer and clears fetchError; the + // failure must survive as lastFetchError so consumers do not mistake + // the state for a cleanly loaded list. + let state = libraries(undefined, { type: FAILURE, error: fetchError }); + state = libraries(state, { type: EDIT_REQUEST }); + + expect(state.fetchError).toBeNull(); + expect(state.lastFetchError).toEqual(fetchError); + }); + + it("keeps an already-retained failure when a library form submit starts", () => { + let state = libraries(undefined, { type: FAILURE, error: fetchError }); + state = libraries(state, { type: REQUEST }); + state = libraries(state, { type: EDIT_REQUEST }); + + expect(state.lastFetchError).toEqual(fetchError); + }); + + it("does not invent a failure when a library form submit starts cleanly", () => { + const state = libraries(undefined, { type: EDIT_REQUEST }); + expect(state.lastFetchError).toBeNull(); + }); + + it("keeps a list-fetch failure when a library form submit succeeds", () => { + // A list refetch can fail between EDIT_LIBRARY_REQUEST and its + // SUCCESS, and the base reducer's SUCCESS also clears fetchError. + let state = libraries(undefined, { type: REQUEST }); + state = libraries(state, { type: EDIT_REQUEST }); + state = libraries(state, { type: FAILURE, error: fetchError }); + state = libraries(state, { type: EDIT_SUCCESS }); + + expect(state.fetchError).toBeNull(); + expect(state.lastFetchError).toEqual(fetchError); + }); + + it("drops a failure beside current data when a library form submit starts", () => { + // Same rule as LIBRARIES_REQUEST: consumers ignore that failure. + const data = { libraries: [{ short_name: "nypl" }] }; + let state = libraries(undefined, { type: LOAD, data }); + state = libraries(state, { type: FAILURE, error: fetchError }); + state = libraries(state, { type: EDIT_REQUEST }); + + expect(state.lastFetchError).toBeNull(); + }); + it("reports only the new failure when a retry fails again", () => { const newError = { status: 502, diff --git a/tests/jest/utils/allLibraries.test.ts b/tests/jest/utils/allLibraries.test.ts index fd74be94d7..e14242c2b3 100644 --- a/tests/jest/utils/allLibraries.test.ts +++ b/tests/jest/utils/allLibraries.test.ts @@ -18,6 +18,24 @@ describe("settledAllLibraries", () => { ).toEqual({}); }); + it("reports not settled when isLoaded is set without a list or error", () => { + // EDIT_LIBRARY actions can clear fetchError while leaving isLoaded + // true; that state proves nothing worth showing. + expect(settledAllLibraries(stateWith({ isLoaded: true }))).toEqual({}); + }); + + it("settles an empty loaded list", () => { + expect( + settledAllLibraries( + stateWith({ data: { libraries: [] }, isLoaded: true }) + ) + ).toEqual({ + allLibraries: [], + allLibrariesError: undefined, + allLibrariesRefreshError: undefined, + }); + }); + it("returns the loaded list", () => { expect( settledAllLibraries( @@ -26,10 +44,24 @@ describe("settledAllLibraries", () => { ).toEqual({ allLibraries: libraries, allLibrariesError: undefined }); }); - it("reports a failure next to a loaded list as a refresh error", () => { - // Two overlapping requests, or a failed refresh of a loaded list, can - // leave a list next to a recorded failure; the list wins and the - // failure downgrades to a non-blocking refresh error. + it("serves the retained list while a refetch is in flight", () => { + expect( + settledAllLibraries( + stateWith({ + data: null, + isLoaded: false, + isFetching: true, + lastData: { libraries }, + }) + ) + ).toEqual({ + allLibraries: libraries, + allLibrariesError: undefined, + allLibrariesRefreshError: undefined, + }); + }); + + it("serves the retained list plus a refresh error after a failed refetch", () => { const fetchError = { status: 500, response: "nope", @@ -37,7 +69,12 @@ describe("settledAllLibraries", () => { }; expect( settledAllLibraries( - stateWith({ data: { libraries }, isLoaded: true, fetchError }) + stateWith({ + data: null, + isLoaded: true, + fetchError, + lastData: { libraries }, + }) ) ).toEqual({ allLibraries: libraries, @@ -46,6 +83,26 @@ describe("settledAllLibraries", () => { }); }); + it("ignores a failure recorded beside a current list", () => { + // Overlapping requests with mixed outcomes (one LOAD, one FAILURE) can + // leave a current list next to a recorded failure. The list on screen + // is up to date, so no error of either kind is reported. + const fetchError = { + status: 500, + response: "nope", + url: "/admin/libraries", + }; + expect( + settledAllLibraries( + stateWith({ data: { libraries }, isLoaded: true, fetchError }) + ) + ).toEqual({ + allLibraries: libraries, + allLibrariesError: undefined, + allLibrariesRefreshError: undefined, + }); + }); + it("settles to an empty list plus the error on failure", () => { const fetchError = { status: 500, @@ -107,6 +164,7 @@ describe("fetchLibrariesIfNeeded", () => { it("does not fetch when the list is already loaded", () => { const { dispatch, dispatched } = makeDispatch({ + data: { libraries: [] }, isLoaded: true, isFetching: false, fetchError: null, @@ -115,6 +173,35 @@ describe("fetchLibrariesIfNeeded", () => { expect(dispatched).toEqual([]); }); + it("fetches when isLoaded is set without a list or error", () => { + // EDIT_LIBRARY actions can clear fetchError while leaving isLoaded + // true; there is nothing to show, so fetch. + const { dispatch, dispatched } = makeDispatch({ + isLoaded: true, + isFetching: false, + fetchError: null, + }); + fetchLibrariesIfNeeded(dispatch, actions); + expect(dispatched).toEqual([fetchThunk]); + }); + + it("retries when a retained list carries a retained failure", () => { + const { dispatch, dispatched } = makeDispatch({ + data: null, + isLoaded: false, + isFetching: false, + fetchError: null, + lastData: { libraries: [] }, + lastFetchError: { + status: 500, + response: "nope", + url: "/admin/libraries", + }, + }); + fetchLibrariesIfNeeded(dispatch, actions); + expect(dispatched).toEqual([fetchThunk]); + }); + it("does not fetch while a request is already in flight", () => { const { dispatch, dispatched } = makeDispatch({ isLoaded: false, @@ -125,6 +212,17 @@ describe("fetchLibrariesIfNeeded", () => { expect(dispatched).toEqual([]); }); + it("retries when a loaded list has a recorded failure", () => { + const { dispatch, dispatched } = makeDispatch({ + data: { libraries: [] }, + isLoaded: true, + isFetching: false, + fetchError: { status: 500, response: "nope", url: "/admin/libraries" }, + }); + fetchLibrariesIfNeeded(dispatch, actions); + expect(dispatched).toEqual([fetchThunk]); + }); + it("retries after a failed request", () => { const { dispatch, dispatched } = makeDispatch({ isLoaded: true, From 13cfa8e2a6cc9b7ff5488994c7682a56a6ab9b17 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Mon, 21 Sep 2026 12:58:40 -0400 Subject: [PATCH 8/8] CI AI code review feedback --- src/components/DiscoveryServices.tsx | 6 +++++ src/utils/allLibraries.ts | 25 ++++++++++++------- .../components/DiscoveryServices.test.tsx | 20 +++++++++++++++ 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/src/components/DiscoveryServices.tsx b/src/components/DiscoveryServices.tsx index 0b9067ef39..2d9659f523 100644 --- a/src/components/DiscoveryServices.tsx +++ b/src/components/DiscoveryServices.tsx @@ -87,6 +87,12 @@ export class DiscoveryServices extends GenericEditableConfigList< return (serviceReg?.libraries ?? []).filter((l) => l.status === "success"); } + // This tab's disclosure lists registered libraries, whose names and links + // fall back to the registration records themselves. + protected librariesUnavailableMessage(): string { + return "The library list failed to load. Registered libraries are shown using registration data, which may be out of date."; + } + protected formatAssociatedCount(count: number): string { return count === 0 ? "no registered libraries" diff --git a/src/utils/allLibraries.ts b/src/utils/allLibraries.ts index d3cbf5e4a1..6882480b94 100644 --- a/src/utils/allLibraries.ts +++ b/src/utils/allLibraries.ts @@ -24,7 +24,9 @@ import { LibrariesState } from "../reducers/libraries"; * Merge the result into the `data` prop built by a config page's * mapStateToProps. */ -export function settledAllLibraries(state): AllLibrariesData { +export function settledAllLibraries(state: { + editor: { libraries?: LibrariesState }; +}): AllLibrariesData { const libraries = state.editor.libraries; const current = currentLibraries(libraries); const loaded = retainedLibraries(libraries); @@ -46,15 +48,20 @@ export function settledAllLibraries(state): AllLibrariesData { * the catch only avoids an unhandled rejection. */ export function fetchLibrariesIfNeeded(dispatch, actions: ActionCreator): void { - dispatch((thunkDispatch, getState) => { - const libraries = getState().editor.libraries; - const inFlight = libraries?.isFetching; - const settledCleanly = - !!retainedLibraries(libraries) && !retainedError(libraries); - if (!inFlight && !settledCleanly) { - thunkDispatch(actions.fetchLibraries()).catch(() => {}); + dispatch( + ( + thunkDispatch, + getState: () => { editor: { libraries?: LibrariesState } } + ) => { + const libraries = getState().editor.libraries; + const inFlight = libraries?.isFetching; + const settledCleanly = + !!retainedLibraries(libraries) && !retainedError(libraries); + if (!inFlight && !settledCleanly) { + thunkDispatch(actions.fetchLibraries()).catch(() => {}); + } } - }); + ); } // The predicates settled-ness is derived from, feeding both functions diff --git a/tests/jest/components/DiscoveryServices.test.tsx b/tests/jest/components/DiscoveryServices.test.tsx index f5b2e638ae..5bbdf35990 100644 --- a/tests/jest/components/DiscoveryServices.test.tsx +++ b/tests/jest/components/DiscoveryServices.test.tsx @@ -52,6 +52,26 @@ describe("DiscoveryServices - registered library disclosure", () => { // ── Toggle visibility ───────────────────────────────────────────────────── + it("names registration data in the failed-library-list alert", () => { + // This tab's entries fall back to registration records, so the generic + // "shown by short name only" message would be wrong here. + const { container } = renderServices({ + discovery_services: [{ id: 1, protocol: "p", name: "Service A" } as any], + allLibraries: [], + allLibrariesError: { + status: 500, + response: "nope", + url: "/admin/libraries", + }, + libraryRegistrations: [ + { id: 1, libraries: [{ short_name: "alpha", status: "success" }] }, + ] as any, + }); + expect(container.querySelector(".alert-danger")).toHaveTextContent( + "Registered libraries are shown using registration data" + ); + }); + it("shows no toggle while allLibraries has not settled", () => { // Labels must render once, in final linked form, not flash bare short // names that get rewritten when the sitewide list arrives.