From 4fb4f0efc33f2ae6b9ad3b593c1260d644b88a3c Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Thu, 12 Dec 2024 16:42:29 -0500 Subject: [PATCH 01/11] Manually fix merge conflicts --- CHANGELOG | 3 + __test__/pages/hold/eddRequestPage.test.tsx | 151 +++++++++++++----- __test__/pages/hold/requestPage.test.tsx | 98 +++++++++++- pages/api/hold/request/[id]/edd.ts | 28 +++- pages/api/hold/request/[id]/index.ts | 29 +++- pages/hold/request/[id]/edd.tsx | 105 +++++++----- pages/hold/request/[id]/index.tsx | 108 +++++++------ src/components/HoldPages/EDDRequestForm.tsx | 19 ++- .../HoldPages/HoldRequestBanner.tsx | 95 ----------- .../HoldPages/HoldRequestErrorBanner.tsx | 112 +++++++++++++ .../HoldPages/PatronIneligibilityErrors.tsx | 73 +++++++++ src/config/config.ts | 1 + src/server/api/hold.ts | 115 +++++++------ src/server/api/tests/hold.test.ts | 68 ++++++++ src/types/appTypes.ts | 2 +- src/types/holdPageTypes.ts | 15 +- src/utils/holdPageUtils.ts | 17 -- 17 files changed, 723 insertions(+), 316 deletions(-) delete mode 100644 src/components/HoldPages/HoldRequestBanner.tsx create mode 100644 src/components/HoldPages/HoldRequestErrorBanner.tsx create mode 100644 src/components/HoldPages/PatronIneligibilityErrors.tsx diff --git a/CHANGELOG b/CHANGELOG index 62d3b8fb0..45ea35299 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -11,6 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added and integrated hold confirmation details fetcher function (SCC-3762) - Added server-side auth redirect to hold confirmation page (SCC-3762) +- Added fetchPatronEligibility server function that requests a patron's hold request eligibility status from Discovery API (SCC-3762) +- Added patron ineligibility error messaging to HoldRequestErrorBanner component (SCC-3762) +- Added eligibility checks to EDD and on-site request hold pages and API routes (SCC-3762) ### Updated diff --git a/__test__/pages/hold/eddRequestPage.test.tsx b/__test__/pages/hold/eddRequestPage.test.tsx index d1ec932fa..f1692d224 100644 --- a/__test__/pages/hold/eddRequestPage.test.tsx +++ b/__test__/pages/hold/eddRequestPage.test.tsx @@ -19,9 +19,9 @@ import { BASE_URL, PATHS, EDD_FORM_FIELD_COPY, + HOLD_PAGE_ERROR_HEADINGS, } from "../../../src/config/constants" import { fetchDeliveryLocations } from "../../../src/server/api/hold" -import { EDDPageStatusMessages } from "../../../src/utils/holdPageUtils" jest.mock("../../../src/server/auth") jest.mock("../../../src/server/api/bib") @@ -44,6 +44,26 @@ const mockReq = { }, } +const fillRequiredEDDFormFields = async () => { + // Fill in all required form fields + await userEvent.type( + screen.getByPlaceholderText(EDD_FORM_FIELD_COPY.emailAddress.placeholder), + EDD_FORM_FIELD_COPY.emailAddress.placeholder + ) + await userEvent.type( + screen.getByPlaceholderText(EDD_FORM_FIELD_COPY.startPage.placeholder), + EDD_FORM_FIELD_COPY.startPage.placeholder + ) + await userEvent.type( + screen.getByPlaceholderText(EDD_FORM_FIELD_COPY.endPage.placeholder), + EDD_FORM_FIELD_COPY.endPage.placeholder + ) + await userEvent.type( + screen.getByPlaceholderText(EDD_FORM_FIELD_COPY.chapterTitle.placeholder), + EDD_FORM_FIELD_COPY.chapterTitle.placeholder + ) +} + describe("EDD Request page", () => { describe("logout redirect handling", () => { beforeEach(() => { @@ -74,14 +94,12 @@ describe("EDD Request page", () => { params: { id }, req: mockReq, res: mockRes, - query: {}, }) expect(responseWithZeroRedirects.redirect).toBeDefined() const responseWithTwoRedirects = await getServerSideProps({ params: { id: "123-456" }, req: { ...mockReq, cookies: { nyplAccountRedirects: 2 } }, res: mockRes, - query: {}, }) expect(responseWithTwoRedirects.redirect).toBeDefined() }) @@ -98,7 +116,6 @@ describe("EDD Request page", () => { params: { id }, req: mockReq, res: mockRes, - query: {}, }) expect(responseWithoutRedirect.redirect).not.toBeDefined() }) @@ -107,7 +124,6 @@ describe("EDD Request page", () => { params: { id }, req: mockReq, res: mockRes, - query: {}, }) expect(response.redirect).toBeUndefined() }) @@ -123,7 +139,6 @@ describe("EDD Request page", () => { params: { id }, res: mockRes, req: mockReq, - query: {}, }) expect(mockRes.setHeader.mock.calls[0]).toStrictEqual([ "Set-Cookie", @@ -149,7 +164,6 @@ describe("EDD Request page", () => { params: { id }, res: mockRes, req: mockReq, - query: {}, }) expect(responseWithAeonRedirect.redirect).toStrictEqual({ destination: bibWithSingleAeonItem.resource.items[0].aeonUrl[0], @@ -204,32 +218,12 @@ describe("EDD Request page", () => { global.fetch = jest.fn().mockImplementationOnce(() => Promise.resolve({ - status: 404, + status: 500, json: () => Promise.resolve({ success: true }), }) ) - // Fill in all required form fields - await userEvent.type( - screen.getByPlaceholderText( - EDD_FORM_FIELD_COPY.emailAddress.placeholder - ), - EDD_FORM_FIELD_COPY.emailAddress.placeholder - ) - await userEvent.type( - screen.getByPlaceholderText(EDD_FORM_FIELD_COPY.startPage.placeholder), - EDD_FORM_FIELD_COPY.startPage.placeholder - ) - await userEvent.type( - screen.getByPlaceholderText(EDD_FORM_FIELD_COPY.endPage.placeholder), - EDD_FORM_FIELD_COPY.endPage.placeholder - ) - await userEvent.type( - screen.getByPlaceholderText( - EDD_FORM_FIELD_COPY.chapterTitle.placeholder - ), - EDD_FORM_FIELD_COPY.chapterTitle.placeholder - ) + await fillRequiredEDDFormFields() }) it("shows an error when the request fails", async () => { @@ -238,7 +232,9 @@ describe("EDD Request page", () => { expect(screen.getByTestId("hold-request-error")).toBeInTheDocument() }) - expect(screen.getByText("Request failed")).toBeInTheDocument() + expect( + screen.getByText("Request failed.", { exact: false }) + ).toBeInTheDocument() expect( screen.queryByText( @@ -307,11 +303,11 @@ describe("EDD Request page", () => { discoveryItemResult={bibWithItems.resource.items[0]} patronId="123" isAuthenticated={true} - pageStatus="unavailable" + errorStatus="eddUnavailable" /> ) expect( - screen.getByText(EDDPageStatusMessages.unavailable.heading) + screen.getByText(HOLD_PAGE_ERROR_HEADINGS.eddUnavailable) ).toBeInTheDocument() }) it("shows a failed error message when the page loads with an failed status", async () => { @@ -321,11 +317,11 @@ describe("EDD Request page", () => { discoveryItemResult={bibWithItems.resource.items[0]} patronId="123" isAuthenticated={true} - pageStatus="failed" + errorStatus="failed" /> ) expect( - screen.getByText(EDDPageStatusMessages.failed.heading) + screen.getByText(HOLD_PAGE_ERROR_HEADINGS.failed) ).toBeInTheDocument() }) it("shows an invalid error message when the page loads with an invalid status", async () => { @@ -335,11 +331,94 @@ describe("EDD Request page", () => { discoveryItemResult={bibWithItems.resource.items[0]} patronId="123" isAuthenticated={true} - pageStatus="invalid" + errorStatus="invalid" /> ) expect( - screen.getByText(EDDPageStatusMessages.invalid.message) + screen.getByText( + "Some fields contain errors. Please correct and submit again." + ) + ).toBeInTheDocument() + }) + }) + describe("EDD Request patron ineligibility messaging", () => { + beforeEach(async () => { + render( + + ) + + global.fetch = jest.fn().mockImplementationOnce(() => + Promise.resolve({ + status: 401, + json: () => + Promise.resolve({ + success: false, + patronEligibilityStatus: { + eligibility: false, + expired: true, + moneyOwed: true, + ptypeDisallowsHolds: true, + reachedHoldLimit: true, + }, + }), + }) + ) + + await fillRequiredEDDFormFields() + + await fireEvent( + screen.getByText("Submit request"), + new MouseEvent("click") + ) + }) + + it("shows an error listing ineligibility reasons when the patron is ineligibile to place holds", async () => { + await waitFor(() => { + expect(screen.getByTestId("hold-request-error")).toBeInTheDocument() + }) + + expect( + screen.getByText("There is a problem with your library account.", { + exact: false, + }) + ).toBeInTheDocument() + + expect( + screen.getByText("This is because:", { + exact: false, + }) + ).toBeInTheDocument() + + expect( + screen.getByText("Your account has expired", { + exact: false, + }) + ).toBeInTheDocument() + + expect( + screen.getByText("Your fines have exceeded the limit", { + exact: false, + }) + ).toBeInTheDocument() + + expect( + screen.getByText( + "Your card does not permit placing holds on ReCAP materials.", + { + exact: false, + } + ) + ).toBeInTheDocument() + + expect( + screen.getByText("You have reached the allowed number of holds.", { + exact: false, + }) ).toBeInTheDocument() }) }) diff --git a/__test__/pages/hold/requestPage.test.tsx b/__test__/pages/hold/requestPage.test.tsx index 554d9600b..3773ac766 100644 --- a/__test__/pages/hold/requestPage.test.tsx +++ b/__test__/pages/hold/requestPage.test.tsx @@ -200,7 +200,7 @@ describe("Hold Request page", () => { expect(screen.getByTestId("hold-request-form")).toBeInTheDocument() }) }) - describe("Hold Request error handling", () => { + describe("Hold Request server error handling", () => { beforeEach(async () => { render( { global.fetch = jest.fn().mockImplementationOnce(() => Promise.resolve({ - status: 404, - json: () => Promise.resolve({ success: true }), + status: 500, + json: () => Promise.resolve({ success: false }), }) ) @@ -237,7 +237,9 @@ describe("Hold Request page", () => { expect(screen.getByTestId("hold-request-error")).toBeInTheDocument() }) - expect(screen.getByText("Request failed")).toBeInTheDocument() + expect( + screen.getByText("Request failed.", { exact: false }) + ).toBeInTheDocument() expect( screen.queryByText( @@ -278,4 +280,92 @@ describe("Hold Request page", () => { }) }) }) + + describe("Hold Request patron ineligibility messaging", () => { + beforeEach(async () => { + render( + + ) + + global.fetch = jest.fn().mockImplementationOnce(() => + Promise.resolve({ + status: 401, + json: () => + Promise.resolve({ + success: false, + patronEligibilityStatus: { + eligibility: false, + expired: true, + moneyOwed: true, + ptypeDisallowsHolds: true, + reachedHoldLimit: true, + }, + }), + }) + ) + + await fireEvent( + screen.getByText("Submit request"), + new MouseEvent("click") + ) + }) + + it("shows an error listing ineligibility reasons when the patron is ineligibile to place holds", async () => { + await waitFor(() => { + expect(screen.getByTestId("hold-request-error")).toBeInTheDocument() + }) + + expect( + screen.getByText("There is a problem with your library account.", { + exact: false, + }) + ).toBeInTheDocument() + + expect( + screen.getByText("This is because:", { + exact: false, + }) + ).toBeInTheDocument() + + expect( + screen.getByText("Your account has expired", { + exact: false, + }) + ).toBeInTheDocument() + + expect( + screen.getByText("Your fines have exceeded the limit", { + exact: false, + }) + ).toBeInTheDocument() + + expect( + screen.getByText( + "Your card does not permit placing holds on ReCAP materials.", + { + exact: false, + } + ) + ).toBeInTheDocument() + + expect( + screen.getByText("You have reached the allowed number of holds.", { + exact: false, + }) + ).toBeInTheDocument() + }) + }) }) diff --git a/pages/api/hold/request/[id]/edd.ts b/pages/api/hold/request/[id]/edd.ts index b9cd13412..29790c6b7 100644 --- a/pages/api/hold/request/[id]/edd.ts +++ b/pages/api/hold/request/[id]/edd.ts @@ -1,6 +1,9 @@ import type { NextApiRequest, NextApiResponse } from "next" -import { postEDDRequest } from "../../../../../src/server/api/hold" +import { + postEDDRequest, + fetchPatronEligibility, +} from "../../../../../src/server/api/hold" import { BASE_URL, PATHS } from "../../../../../src/config/constants" /** @@ -8,16 +11,33 @@ import { BASE_URL, PATHS } from "../../../../../src/config/constants" */ async function handler(req: NextApiRequest, res: NextApiResponse) { if (req.method !== "POST") { - res.status(500).json({ + return res.status(500).json({ error: "Please use a POST request for the EDD Request API endpoint", }) } try { + const { patronId, jsEnabled, ...rest } = JSON.parse(req.body) const holdId = req.query.id as string + const [, itemId] = holdId.split("-") - const { jsEnabled, ...rest } = JSON.parse(req.body) + const patronEligibilityStatus = await fetchPatronEligibility(patronId) + + if (patronEligibilityStatus.status === 401) { + switch (jsEnabled) { + case true: + return res.status(401).json({ + patronEligibilityStatus, + }) + + // Server side redirect on ineligibility when Js is disabled + // TODO: Move this to seaprate API route + // TODO: Parse these query params in getServerSideProps + default: + return res.redirect(`${BASE_URL}${PATHS.HOLD_REQUEST}/${holdId}/edd`) + } + } const holdRequestResponse = await postEDDRequest({ ...rest, @@ -40,7 +60,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) { // JS-Disabled functionality // Redirect to confirmation page - res.redirect( + return res.redirect( `${BASE_URL}${PATHS.HOLD_CONFIRMATION}/${holdId}?pickupLocation=edd?requestId=${requestId}` ) } catch (error) { diff --git a/pages/api/hold/request/[id]/index.ts b/pages/api/hold/request/[id]/index.ts index bc5111f48..5c7c51091 100644 --- a/pages/api/hold/request/[id]/index.ts +++ b/pages/api/hold/request/[id]/index.ts @@ -1,6 +1,9 @@ import type { NextApiRequest, NextApiResponse } from "next" -import { postHoldRequest } from "../../../../../src/server/api/hold" +import { + postHoldRequest, + fetchPatronEligibility, +} from "../../../../../src/server/api/hold" import { BASE_URL, PATHS } from "../../../../../src/config/constants" /** @@ -8,16 +11,33 @@ import { BASE_URL, PATHS } from "../../../../../src/config/constants" */ async function handler(req: NextApiRequest, res: NextApiResponse) { if (req.method !== "POST") { - res.status(500).json({ + return res.status(500).json({ error: "Please use a POST request for the Hold Request API endpoint", }) } try { + const { patronId, source, pickupLocation, jsEnabled } = JSON.parse(req.body) + const holdId = req.query.id as string const [, itemId] = holdId.split("-") - const { patronId, source, pickupLocation, jsEnabled } = JSON.parse(req.body) + const patronEligibilityStatus = await fetchPatronEligibility(patronId) + + if (patronEligibilityStatus.status === 401) { + switch (jsEnabled) { + case true: + return res.status(401).json({ + patronEligibilityStatus, + }) + + // Server side redirect on ineligibility when Js is disabled + // TODO: Move this to seaprate API route + // TODO: Parse these query params in getServerSideProps + default: + return res.redirect(`${BASE_URL}${PATHS.HOLD_REQUEST}/${holdId}`) + } + } const holdRequestResponse = await postHoldRequest({ itemId, @@ -34,6 +54,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) { } // Return a 200 status when the hold request is posted successfully via JS fetch + // TODO: Make this a separate API route instead of a fetch attribute if (jsEnabled) { return res.status(200).json({ pickupLocation: pickupLocationFromResponse, @@ -42,7 +63,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) { } // Server side redirect in case user has JS disabled - res.redirect( + return res.redirect( `${BASE_URL}${PATHS.HOLD_CONFIRMATION}/${holdId}?pickupLocation=${pickupLocationFromResponse}?requestId=${requestId}` ) } catch (error) { diff --git a/pages/hold/request/[id]/edd.tsx b/pages/hold/request/[id]/edd.tsx index 058e3c3b4..421c3dfc1 100644 --- a/pages/hold/request/[id]/edd.tsx +++ b/pages/hold/request/[id]/edd.tsx @@ -9,18 +9,18 @@ import { import Layout from "../../../../src/components/Layout/Layout" import EDDRequestForm from "../../../../src/components/HoldPages/EDDRequestForm" -import HoldRequestBanner from "../../../../src/components/HoldPages/HoldRequestBanner" +import HoldRequestErrorBanner from "../../../../src/components/HoldPages/HoldRequestErrorBanner" import HoldRequestItemDetails from "../../../../src/components/HoldPages/HoldRequestItemDetails" import { SITE_NAME, BASE_URL, PATHS } from "../../../../src/config/constants" import useLoading from "../../../../src/hooks/useLoading" import { fetchBib } from "../../../../src/server/api/bib" -import { fetchDeliveryLocations } from "../../../../src/server/api/hold" import { - EDDPageStatusMessages, - initialEDDFormState, -} from "../../../../src/utils/holdPageUtils" + fetchDeliveryLocations, + fetchPatronEligibility, +} from "../../../../src/server/api/hold" +import { initialEDDFormState } from "../../../../src/utils/holdPageUtils" import initializePatronTokenAuth, { doRedirectBasedOnNyplAccountRedirects, @@ -35,7 +35,8 @@ import type { DiscoveryItemResult } from "../../../../src/types/itemTypes" import type { EDDRequestParams, - EDDPageStatus, + HoldErrorStatus, + PatronEligibilityStatus, } from "../../../../src/types/holdPageTypes" interface EDDRequestPropsType { @@ -43,8 +44,8 @@ interface EDDRequestPropsType { discoveryItemResult: DiscoveryItemResult patronId: string isAuthenticated?: boolean - eddRequestable?: boolean - pageStatus?: EDDPageStatus + errorStatus?: HoldErrorStatus + patronEligibilityStatus?: PatronEligibilityStatus } /** @@ -55,7 +56,8 @@ export default function EDDRequestPage({ discoveryItemResult, patronId, isAuthenticated, - pageStatus: defaultPageStatus, + errorStatus: defaultErrorStatus, + patronEligibilityStatus: defaultEligibilityStatus, }: EDDRequestPropsType) { const metadataTitle = `Electronic Delivery Request | ${SITE_NAME}` @@ -64,7 +66,10 @@ export default function EDDRequestPage({ const holdId = `${item.bibId}-${item.id}` - const [pageStatus, setPageStatus] = useState(defaultPageStatus) + const [errorStatus, setErrorStatus] = useState(defaultErrorStatus) + const [patronEligibilityStatus, setPatronEligibilityStatus] = useState( + defaultEligibilityStatus + ) const [eddFormState, setEddFormState] = useState({ ...initialEDDFormState, @@ -79,10 +84,14 @@ export default function EDDRequestPage({ const isLoading = useLoading() useEffect(() => { - if (pageStatus && pageStatus !== "invalid" && bannerContainerRef.current) { + if ( + errorStatus && + errorStatus !== "invalid" && + bannerContainerRef.current + ) { bannerContainerRef.current.focus() } - }, [pageStatus]) + }, [errorStatus, patronEligibilityStatus]) const postEDDRequest = async (eddParams: EDDRequestParams) => { try { @@ -97,30 +106,35 @@ export default function EDDRequestPage({ ) const responseJson = await response.json() - if (response.status !== 200) { - console.error( - "HoldRequestPage: Error in edd request api response", - responseJson.error - ) - setPageStatus("failed") - setFormPosting(false) - return + switch (response.status) { + // Patron is ineligible to place holds + case 401: + setFormPosting(false) + setErrorStatus("patronIneligible") + setPatronEligibilityStatus(responseJson?.patronEligibilityStatus) + break + // Server side error placing the hold request + case 500: + setFormPosting(false) + console.error( + "EDDRequestPage: Error in EDD request api response", + responseJson.error + ) + setErrorStatus("failed") + break + default: + setFormPosting(false) + // Success state + await router.push( + `${PATHS.HOLD_CONFIRMATION}/${holdId}?pickupLocation=edd&requestId=${responseJson?.requestId}` + ) } - const { requestId } = responseJson - - setPageStatus(null) - setFormPosting(false) - - // Success state - await router.push( - `${PATHS.HOLD_CONFIRMATION}/${holdId}?pickupLocation=edd&requestId=${requestId}` - ) } catch (error) { console.error( "HoldRequestPage: Error in hold request api response", error ) - setPageStatus("failed") + setErrorStatus("failed") setFormPosting(false) } } @@ -141,12 +155,11 @@ export default function EDDRequestPage({ {/* Always render the wrapper element that will display the dynamically rendered notification for focus management */} - {pageStatus && ( - )} @@ -156,12 +169,12 @@ export default function EDDRequestPage({ {isLoading || formPosting ? ( - ) : pageStatus !== "unavailable" ? ( + ) : errorStatus !== "eddUnavailable" ? ( ) : null} @@ -170,7 +183,7 @@ export default function EDDRequestPage({ ) } -export async function getServerSideProps({ params, req, res, query }) { +export async function getServerSideProps({ params, req, res }) { const { id } = params // authentication redirect @@ -232,18 +245,30 @@ export async function getServerSideProps({ params, req, res, query }) { await fetchDeliveryLocations(item.barcode, patronId) if (locationStatus !== 200) { - throw new Error("EDD Page - Error fetching edd in getServerSideProps") + console.error("EDD Page - Error fetching edd in getServerSideProps") } const isEddAvailable = eddRequestable && item.isAvailable + const patronEligibilityStatus = await fetchPatronEligibility(patronId) + + const locationOrEligibilityFetchFailed = + locationStatus !== 200 || + ![200, 401].includes(patronEligibilityStatus?.status) + return { props: { discoveryBibResult, discoveryItemResult, patronId, isAuthenticated, - pageStatus: !isEddAvailable ? "unavailable" : null, + errorStatus: locationOrEligibilityFetchFailed + ? "failed" + : patronEligibilityStatus.status === 401 + ? "patronIneligible" + : !isEddAvailable + ? "eddUnavailable" + : null, }, } } catch (error) { diff --git a/pages/hold/request/[id]/index.tsx b/pages/hold/request/[id]/index.tsx index 5bbc6ada8..bbc19b47b 100644 --- a/pages/hold/request/[id]/index.tsx +++ b/pages/hold/request/[id]/index.tsx @@ -10,7 +10,7 @@ import { import Layout from "../../../../src/components/Layout/Layout" import HoldRequestForm from "../../../../src/components/HoldPages/HoldRequestForm" -import HoldRequestBanner from "../../../../src/components/HoldPages/HoldRequestBanner" +import HoldRequestErrorBanner from "../../../../src/components/HoldPages/HoldRequestErrorBanner" import HoldRequestItemDetails from "../../../../src/components/HoldPages/HoldRequestItemDetails" import { SITE_NAME, BASE_URL, PATHS } from "../../../../src/config/constants" @@ -19,7 +19,7 @@ import useLoading from "../../../../src/hooks/useLoading" import { fetchBib } from "../../../../src/server/api/bib" import { fetchDeliveryLocations, - fetchHoldRequestEligibility, + fetchPatronEligibility, } from "../../../../src/server/api/hold" import initializePatronTokenAuth, { @@ -33,6 +33,10 @@ import Item from "../../../../src/models/Item" import type { DiscoveryBibResult } from "../../../../src/types/bibTypes" import type { DiscoveryItemResult } from "../../../../src/types/itemTypes" import type { DeliveryLocation } from "../../../../src/types/locationTypes" +import type { + HoldErrorStatus, + PatronEligibilityStatus, +} from "../../../../src/types/holdPageTypes" interface HoldRequestPropsType { discoveryBibResult: DiscoveryBibResult @@ -40,6 +44,8 @@ interface HoldRequestPropsType { deliveryLocations?: DeliveryLocation[] patronId: string isAuthenticated?: boolean + errorStatus?: HoldErrorStatus + patronEligibilityStatus?: PatronEligibilityStatus } /** @@ -53,6 +59,8 @@ export default function HoldRequestPage({ deliveryLocations = [], patronId, isAuthenticated, + errorStatus: defaultErrorStatus, + patronEligibilityStatus: defaultEligibilityStatus, }: HoldRequestPropsType) { const metadataTitle = `Item Request | ${SITE_NAME}` @@ -61,9 +69,10 @@ export default function HoldRequestPage({ const holdId = `${item.bibId}-${item.id}` - // Initialize alert to true if item is not available. This will show the error banner. - const [alert, setAlert] = useState(!item.isAvailable) - const [errorDetail, setErrorDetail] = useState("") + const [errorStatus, setErrorStatus] = useState(defaultErrorStatus) + const [patronEligibilityStatus, setPatronEligibilityStatus] = useState( + defaultEligibilityStatus + ) const [formPosting, setFormPosting] = useState(false) const bannerContainerRef = useRef() @@ -71,10 +80,10 @@ export default function HoldRequestPage({ const isLoading = useLoading() useEffect(() => { - if (alert && bannerContainerRef.current) { + if (errorStatus && bannerContainerRef.current) { bannerContainerRef.current.focus() } - }, [alert]) + }, [errorStatus, patronEligibilityStatus]) const handleSubmit = async (e) => { e.preventDefault() @@ -85,40 +94,44 @@ export default function HoldRequestPage({ const response = await fetch(`${BASE_URL}/api/hold/request/${holdId}`, { method: "POST", body: JSON.stringify({ - patronId: patronId.value, - source: source.value, - pickupLocation: pickupLocation.value, + patronId: patronId?.value, + source: source?.value, + pickupLocation: pickupLocation?.value, jsEnabled: true, }), }) const responseJson = await response.json() - if (response.status !== 200) { - console.error( - "HoldRequestPage: Error in hold request api response", - responseJson.error - ) - setAlert(true) - setErrorDetail(responseJson?.error?.detail || "") - setFormPosting(false) - return + switch (response.status) { + // Patron is ineligible to place holds + case 401: + setFormPosting(false) + setErrorStatus("patronIneligible") + setPatronEligibilityStatus(responseJson?.patronEligibilityStatus) + break + // Server side error placing the hold request + case 500: + setFormPosting(false) + console.error( + "HoldRequestPage: Error in hold request api response", + responseJson.error + ) + setErrorStatus("failed") + break + default: + setFormPosting(false) + // Success state + await router.push( + `${PATHS.HOLD_CONFIRMATION}/${holdId}?pickupLocation=${responseJson.pickupLocation}&requestId=${responseJson.requestId}` + ) } - const { pickupLocation: pickupLocationFromResponse, requestId } = - responseJson - - setFormPosting(false) - - // Success state - await router.push( - `${PATHS.HOLD_CONFIRMATION}/${holdId}?pickupLocation=${pickupLocationFromResponse}&requestId=${requestId}` - ) } catch (error) { console.error( "HoldRequestPage: Error in hold request api response", error ) + setErrorStatus("failed") setFormPosting(false) - setAlert(true) } } @@ -138,18 +151,11 @@ export default function HoldRequestPage({ {/* Always render the wrapper element that will display the dynamically rendered notification for focus management */} - {alert && ( - )} @@ -200,7 +206,7 @@ export async function getServerSideProps({ params, req, res }) { redirectCount + 1 }; Max-Age=10; path=/; domain=.nypl.org;` ) - const redirect = getLoginRedirect(req, `${PATHS.HOLD_REQUEST}[${id}]`) + const redirect = getLoginRedirect(req, `${PATHS.HOLD_REQUEST}/${id}`) return { redirect: { @@ -213,10 +219,6 @@ export async function getServerSideProps({ params, req, res }) { try { const patronId = patronTokenResponse?.decodedPatron?.sub - // TODO: implement this function - // const holdRequestEligibility = await fetchHoldRequestEligibility(patronId) - // console.log("holdRequestEligibility", holdRequestEligibility) - // fetch bib and item const [bibId, itemId] = id.split("-") @@ -247,11 +249,17 @@ export async function getServerSideProps({ params, req, res }) { await fetchDeliveryLocations(item.barcode, patronId) if (locationStatus !== 200) { - throw new Error( - "Hold Page - Error fetching delivery locations in getServerSideProps" + console.error( + "HoldRequest Page - Error fetching deliveryLocations in getServerSideProps" ) } + const patronEligibilityStatus = await fetchPatronEligibility(patronId) + + const locationOrEligibilityFetchFailed = + locationStatus !== 200 || + ![200, 401].includes(patronEligibilityStatus?.status) + return { props: { discoveryBibResult, @@ -259,6 +267,12 @@ export async function getServerSideProps({ params, req, res }) { deliveryLocations, patronId, isAuthenticated, + patronEligibilityStatus, + errorStatus: locationOrEligibilityFetchFailed + ? "failed" + : patronEligibilityStatus.status === 401 + ? "patronIneligible" + : null, }, } } catch (error) { diff --git a/src/components/HoldPages/EDDRequestForm.tsx b/src/components/HoldPages/EDDRequestForm.tsx index c4857a80a..0a6097ab2 100644 --- a/src/components/HoldPages/EDDRequestForm.tsx +++ b/src/components/HoldPages/EDDRequestForm.tsx @@ -21,13 +21,16 @@ import { initialEDDInvalidFields, isInvalidField, } from "../../utils/holdPageUtils" -import type { EDDRequestParams, EDDPageStatus } from "../../types/holdPageTypes" +import type { + EDDRequestParams, + HoldErrorStatus, +} from "../../types/holdPageTypes" interface EDDRequestFormProps { eddFormState: EDDRequestParams setEddFormState: React.Dispatch> handleSubmit: (eddParams: EDDRequestParams) => void - setPageStatus: (status: EDDPageStatus) => void + setErrorStatus: (errorStatus: HoldErrorStatus) => void holdId: string } @@ -38,7 +41,7 @@ const EDDRequestForm = ({ eddFormState, setEddFormState, handleSubmit, - setPageStatus, + setErrorStatus, holdId, }: EDDRequestFormProps) => { // Set the invalid fields as an array in state to keep track of the first invalid field for focus on submit @@ -86,10 +89,10 @@ const EDDRequestForm = ({ // Prevent form submission and focus on first invalid field if there is one if (firstInvalidField) { - setPageStatus("invalid") + setErrorStatus("invalid") validatedInputRefs?.[firstInvalidField.key]?.current.focus() } else { - setPageStatus(null) + setErrorStatus(null) handleSubmit(eddFormState) } } @@ -118,7 +121,7 @@ const EDDRequestForm = ({ value={eddFormState.source} /> - + Required information @@ -197,8 +200,8 @@ const EDDRequestForm = ({ ref={validatedInputRefs["chapterTitle"]} /> - - + + Optional information diff --git a/src/components/HoldPages/HoldRequestBanner.tsx b/src/components/HoldPages/HoldRequestBanner.tsx deleted file mode 100644 index afaaac058..000000000 --- a/src/components/HoldPages/HoldRequestBanner.tsx +++ /dev/null @@ -1,95 +0,0 @@ -import { useContext } from "react" -import { Box, Banner, Button } from "@nypl/design-system-react-components" - -import { FeedbackContext } from "../../../src/context/FeedbackContext" -import type { ItemMetadata } from "../../../src/types/itemTypes" -import type Item from "../../../src/models/Item" -import RCLink from "../Links/RCLink/RCLink" - -interface HoldRequestBannerProps { - item: Item - heading: string - errorMessage: string - errorDetail?: string - pageStatus?: string -} - -/** - * The HoldRequestBanner renders an error notification on the hold page that includes a button to - * open the feedback form, pre-populated with item metadata. - */ -const HoldRequestBanner = ({ - item, - heading, - errorMessage, - errorDetail, - pageStatus = "failed", -}: HoldRequestBannerProps) => { - const { onOpen, setItemMetadata } = useContext(FeedbackContext) - - const onContact = (metadata: ItemMetadata) => { - setItemMetadata(metadata) - onOpen() - } - - return ( - - - {errorMessage} - {pageStatus !== "invalid" ? ( - <> - {" Please try again, "} - {" "} - for assistance, or{" "} - start a new search. - - ) : null} - - {errorDetail ? {errorDetail} : null} - - } - mb="s" - /> - ) -} - -export default HoldRequestBanner diff --git a/src/components/HoldPages/HoldRequestErrorBanner.tsx b/src/components/HoldPages/HoldRequestErrorBanner.tsx new file mode 100644 index 000000000..0098077a5 --- /dev/null +++ b/src/components/HoldPages/HoldRequestErrorBanner.tsx @@ -0,0 +1,112 @@ +import { useContext } from "react" +import { Text, Box, Banner, Button } from "@nypl/design-system-react-components" + +import type { + HoldErrorStatus, + PatronEligibilityStatus, +} from "../../types/holdPageTypes" +import { FeedbackContext } from "../../context/FeedbackContext" +import type { ItemMetadata } from "../../types/itemTypes" +import type Item from "../../models/Item" +import RCLink from "../Links/RCLink/RCLink" +import PatronIneligibilityErrors from "./PatronIneligibilityErrors" +import { + PATHS, + HOLD_PAGE_ERROR_HEADINGS, + HOLD_PAGE_CONTACT_PREFIXES, +} from "../../config/constants" + +interface HoldRequestErrorBannerProps { + item: Item + errorStatus?: HoldErrorStatus + patronEligibilityStatus?: PatronEligibilityStatus +} + +/** + * The HoldRequestErrorBanner renders an error notification on the hold page that includes a button to + * open the feedback form, pre-populated with item metadata. + */ +const HoldRequestErrorBanner = ({ + item, + errorStatus = "failed", + patronEligibilityStatus, +}: HoldRequestErrorBannerProps) => { + const { onOpen, setItemMetadata } = useContext(FeedbackContext) + + const onContact = (metadata: ItemMetadata) => { + setItemMetadata(metadata) + onOpen() + } + + return ( + + {HOLD_PAGE_CONTACT_PREFIXES?.[errorStatus] ? ( + + {HOLD_PAGE_CONTACT_PREFIXES?.[errorStatus]} + {" Please try again, "} + {" "} + for assistance, or{" "} + start a new search. + + ) : null} + {(() => { + switch (errorStatus) { + case "invalid": + return "Some fields contain errors. Please correct and submit again." + case "patronIneligible": + return patronEligibilityStatus ? ( + + ) : null + default: + return null + } + })()} + + } + /> + ) +} + +export default HoldRequestErrorBanner diff --git a/src/components/HoldPages/PatronIneligibilityErrors.tsx b/src/components/HoldPages/PatronIneligibilityErrors.tsx new file mode 100644 index 000000000..9ca756486 --- /dev/null +++ b/src/components/HoldPages/PatronIneligibilityErrors.tsx @@ -0,0 +1,73 @@ +import { Text, Box, List } from "@nypl/design-system-react-components" + +import type { + HoldErrorStatus, + PatronEligibilityStatus, +} from "../../types/holdPageTypes" +import type Item from "../../models/Item" +import RCLink from "../Links/RCLink/RCLink" +import ExternalLink from "../Links/ExternalLink/ExternalLink" +import { PATHS } from "../../config/constants" +import { appConfig } from "../../config/config" + +interface PatronIneligibilityErrorsProps { + patronEligibilityStatus: PatronEligibilityStatus +} + +/** + * The PatronIneligibilityErrors component is responsible for rendering a list of reasons why + * a patron is ineligible to place holds within the HoldRequestErrorBanner component. + */ +const PatronIneligibilityErrors = ({ + patronEligibilityStatus, +}: PatronIneligibilityErrorsProps) => { + const { expired, moneyOwed, ptypeDisallowsHolds, reachedHoldLimit } = + patronEligibilityStatus + + const hasSpecificReason = + expired || moneyOwed || ptypeDisallowsHolds || reachedHoldLimit + + // Generic patron error displayed in heading, don't show reasons list if there isn't one + if (!hasSpecificReason) return null + + return ( + + This is because: + + Your account has expired -- Please see{" "} + + Library Terms and Conditions -- Renewing or Validating Your + Library Card + {" "} + about renewing your card. + , + ] + : []), + ...(moneyOwed + ? [ + <> + Your fines have exceeded the limit — you can pay your fines in + a branch or online from the links under{" "} + My Account. + , + ] + : []), + ...(ptypeDisallowsHolds + ? ["Your card does not permit placing holds on ReCAP materials."] + : []), + ...(reachedHoldLimit + ? ["You have reached the allowed number of holds."] + : []), + ]} + /> + + ) +} + +export default PatronIneligibilityErrors diff --git a/src/config/config.ts b/src/config/config.ts index fe46b743e..a9bbe1892 100644 --- a/src/config/config.ts +++ b/src/config/config.ts @@ -69,6 +69,7 @@ export const appConfig: AppConfig = { researchMaterialsHelp: "https://www.nypl.org/help/request-research-materials", tokenUrl: "https://isso.nypl.org/", + renewCard: "https://www.nypl.org/help/library-card/terms-conditions#renew", }, // Array of closed nypl location keys (available options for NYPL locations: all, schwarzman, schomburg, lpa) closedLocations: [] as (NYPLocationKey | "all")[], diff --git a/src/server/api/hold.ts b/src/server/api/hold.ts index cc3ade36f..d359fe0cb 100644 --- a/src/server/api/hold.ts +++ b/src/server/api/hold.ts @@ -124,59 +124,6 @@ export async function postHoldRequest( } } -/** - * Getter function for hold request details. - */ -// TODO: Add return type -export async function fetchHoldDetails( - requestId: string -): Promise { - try { - const client = await nyplApiClient() - const holdDetailsResult = await client.get(`/hold-requests/${requestId}`) - const { id, pickupLocation, patron } = holdDetailsResult.data - - return { - requestId: id, - patronId: patron, - pickupLocation, - status: 200, - } - } catch (error) { - console.error( - `Error fetching hold request details in fetchHoldRequestDetails server function, requestId: ${requestId}`, - error.message - ) - - return { status: 500 } - } -} - -/** - * Getter function for hold request eligibility for patrons. - */ -export async function fetchHoldRequestEligibility( - patronId: string -): Promise { - const eligibilityEndpoint = `/patrons/${patronId}/hold-request-eligibility` - - try { - const client = await nyplApiClient() - const eligibilityResult = await client.get(eligibilityEndpoint, { - cache: false, - }) - - return eligibilityResult as PatronEligibilityStatus - } catch (error) { - console.error( - `Error fetching hold request eligibility in fetchHoldRequestEligibility server function, patronId: ${patronId}`, - error.message - ) - - return { eligibility: false } - } -} - /** * Post EDD requests to discovery API. */ @@ -230,3 +177,65 @@ export async function postEDDRequest( } } } + +/** + * Getter function for hold request details. + */ +// TODO: Add return type +export async function fetchHoldDetails( + requestId: string +): Promise { + try { + const client = await nyplApiClient() + const holdDetailsResult = await client.get(`/hold-requests/${requestId}`) + const { id, pickupLocation, patron } = holdDetailsResult.data + + return { + requestId: id, + patronId: patron, + pickupLocation, + status: 200, + } + } catch (error) { + console.error( + `Error fetching hold request details in fetchHoldRequestDetails server function, requestId: ${requestId}`, + error.message + ) + + return { status: 500 } + } +} + +/** + * Getter function for hold request eligibility for patrons. + */ +export async function fetchPatronEligibility( + patronId: string +): Promise { + const eligibilityEndpoint = `/patrons/${patronId}/hold-request-eligibility` + + try { + const client = await nyplApiClient() + const eligibilityResult = await client.get(eligibilityEndpoint, { + cache: false, + }) + + // There should always be en eligibilty boolean attribute returned from Discovery API + if (eligibilityResult.eligibility === undefined) { + throw new Error("Improperly formatted eligibility from Discovery API") + } + + if (eligibilityResult.eligibility === false) { + return { status: 401, ...eligibilityResult } + } + + return { status: 200, ...eligibilityResult } + } catch (error) { + console.error( + `Error fetching hold request eligibility in fetchPatronEligibility server function, patronId: ${patronId}`, + error.message + ) + + return { status: 500 } + } +} diff --git a/src/server/api/tests/hold.test.ts b/src/server/api/tests/hold.test.ts index 6a310653e..167c3efac 100644 --- a/src/server/api/tests/hold.test.ts +++ b/src/server/api/tests/hold.test.ts @@ -3,11 +3,13 @@ import { postHoldRequest, postEDDRequest, fetchHoldDetails, + fetchPatronEligibility, } from "../hold" import type { DeliveryLocationsResult } from "../../../types/locationTypes" import type { HoldPostResult, HoldDetailsResult, + PatronEligibilityStatus, } from "../../../types/holdPageTypes" jest.mock("../../nyplApiClient", () => { @@ -111,6 +113,41 @@ jest.mock("../../nyplApiClient", () => { }) }) }) + .mockImplementationOnce(async () => { + return await new Promise((resolve) => { + resolve({ + get: jest.fn().mockReturnValueOnce({ + eligibility: true, + expired: false, + moneyOwed: false, + ptypeDisallowsHolds: false, + reachedHoldLimit: false, + }), + }) + }) + }) + .mockImplementationOnce(async () => { + return await new Promise((resolve) => { + resolve({ + get: jest.fn().mockReturnValueOnce({ + eligibility: false, + expired: true, + moneyOwed: true, + ptypeDisallowsHolds: false, + reachedHoldLimit: false, + }), + }) + }) + }) + .mockImplementationOnce(async () => { + return await new Promise((resolve) => { + resolve({ + get: () => { + throw new Error("Error getting patron eligibility status") + }, + }) + }) + }) }) describe("fetchDeliveryLocations", () => { @@ -216,3 +253,34 @@ describe("fetchHoldDetails", () => { expect(holdDetails.status).toEqual(500) }) }) + +describe("fetchPatronEligibility", () => { + it("should return a patron's hold eligibility status from Discovery API", async () => { + const patonEligibility = (await fetchPatronEligibility( + "123" + )) as PatronEligibilityStatus + + expect(patonEligibility).toEqual({ + status: 200, + eligibility: true, + expired: false, + moneyOwed: false, + ptypeDisallowsHolds: false, + reachedHoldLimit: false, + }) + }) + it("should return a 401 status if the patron is ineligibile", async () => { + const patonEligibility = (await fetchPatronEligibility( + "123" + )) as PatronEligibilityStatus + + expect(patonEligibility.status).toEqual(401) + }) + it("should return a 500 status if there was an error", async () => { + const patonEligibility = (await fetchPatronEligibility( + "123" + )) as PatronEligibilityStatus + + expect(patonEligibility.status).toEqual(500) + }) +}) diff --git a/src/types/appTypes.ts b/src/types/appTypes.ts index 4134d650d..89ac8425c 100644 --- a/src/types/appTypes.ts +++ b/src/types/appTypes.ts @@ -23,7 +23,7 @@ export interface Features { export type Environment = "development" | "qa" | "production" -export type HTTPStatusCode = 200 | 307 | 400 | 404 | 500 +export type HTTPStatusCode = 200 | 307 | 400 | 401 | 404 | 500 export type HTTPResponse = { status: HTTPStatusCode diff --git a/src/types/holdPageTypes.ts b/src/types/holdPageTypes.ts index 60e9fa2d0..e7e9057fb 100644 --- a/src/types/holdPageTypes.ts +++ b/src/types/holdPageTypes.ts @@ -44,7 +44,7 @@ export interface PatronEligibilityStatus { moneyOwed?: boolean ptypeDisallowsHolds?: boolean reachedHoldLimit?: boolean - hasIssues?: boolean + status: HTTPStatusCode } export interface DiscoveryHoldPostParams { @@ -59,12 +59,13 @@ export interface DiscoveryHoldPostParams { docDeliveryData?: EDDRequestParams } -export type EDDPageStatus = null | "failed" | "unavailable" | "invalid" - -export interface EDDStatusMessage { - heading?: string - message: string -} +export type HoldErrorStatus = + | null + | "failed" + | "eddUnavailable" + | "invalid" + | "patronIneligible" + | "serverError" export interface EDDFormAction { type: EDDFormActionType diff --git a/src/utils/holdPageUtils.ts b/src/utils/holdPageUtils.ts index 02209ffa1..d5161c033 100644 --- a/src/utils/holdPageUtils.ts +++ b/src/utils/holdPageUtils.ts @@ -1,8 +1,6 @@ import type { EDDRequestParams, EDDFormValidatedField, - EDDPageStatus, - EDDStatusMessage, } from "../types/holdPageTypes" import { EMAIL_REGEX } from "../config/constants" @@ -23,21 +21,6 @@ export const initialEDDFormState: EDDRequestParams = { pickupLocation: "edd", } -export const EDDPageStatusMessages: Record = { - failed: { - heading: "Request failed", - message: "We were unable to process your request at this time.", - }, - unavailable: { - heading: "Electronic delivery unavailable", - message: - "Electronic delivery options for this item are currently unavailable.", - }, - invalid: { - message: "Some fields contain errors. Please correct and submit again.", - }, -} - // Initial state for invalid fields in the EDD form to keep track of the first invalid field for focus on submit export const initialEDDInvalidFields: EDDFormValidatedField[] = [ { key: "emailAddress", isInvalid: false }, From dc86933b62e4062b9285e9063c0a2737782a18e7 Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Thu, 12 Dec 2024 16:47:32 -0500 Subject: [PATCH 02/11] Fix eligibility type --- src/types/holdPageTypes.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/types/holdPageTypes.ts b/src/types/holdPageTypes.ts index e7e9057fb..042de74c6 100644 --- a/src/types/holdPageTypes.ts +++ b/src/types/holdPageTypes.ts @@ -38,9 +38,7 @@ export interface HoldDetailsResult { } export interface PatronEligibilityStatus { - eligibility: boolean expired?: boolean - blocked?: boolean moneyOwed?: boolean ptypeDisallowsHolds?: boolean reachedHoldLimit?: boolean From 5915469060b746d96932ff3f95493c4a27777edb Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Mon, 16 Dec 2024 09:48:30 -0500 Subject: [PATCH 03/11] Use constant for copy in test --- __test__/pages/hold/eddRequestPage.test.tsx | 2 +- __test__/pages/hold/requestPage.test.tsx | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/__test__/pages/hold/eddRequestPage.test.tsx b/__test__/pages/hold/eddRequestPage.test.tsx index f1692d224..68e05bb6a 100644 --- a/__test__/pages/hold/eddRequestPage.test.tsx +++ b/__test__/pages/hold/eddRequestPage.test.tsx @@ -383,7 +383,7 @@ describe("EDD Request page", () => { }) expect( - screen.getByText("There is a problem with your library account.", { + screen.getByText(HOLD_PAGE_ERROR_HEADINGS.patronIneligible, { exact: false, }) ).toBeInTheDocument() diff --git a/__test__/pages/hold/requestPage.test.tsx b/__test__/pages/hold/requestPage.test.tsx index 3773ac766..9ef1ec3c8 100644 --- a/__test__/pages/hold/requestPage.test.tsx +++ b/__test__/pages/hold/requestPage.test.tsx @@ -14,7 +14,12 @@ import initializePatronTokenAuth, { } from "../../../src/server/auth" import { fetchBib } from "../../../src/server/api/bib" import { bibWithItems, bibWithSingleAeonItem } from "../../fixtures/bibFixtures" -import { BASE_URL, PATHS, NYPL_LOCATIONS } from "../../../src/config/constants" +import { + BASE_URL, + PATHS, + NYPL_LOCATIONS, + HOLD_PAGE_ERROR_HEADINGS, +} from "../../../src/config/constants" import { fetchDeliveryLocations } from "../../../src/server/api/hold" jest.mock("../../../src/server/auth") @@ -329,7 +334,7 @@ describe("Hold Request page", () => { }) expect( - screen.getByText("There is a problem with your library account.", { + screen.getByText(HOLD_PAGE_ERROR_HEADINGS.patronIneligible, { exact: false, }) ).toBeInTheDocument() From 90427b0003832d9aae427926d4c39a0c8ec7394e Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Mon, 16 Dec 2024 09:51:23 -0500 Subject: [PATCH 04/11] Remove optional chaining from patronIds --- pages/hold/request/[id]/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pages/hold/request/[id]/index.tsx b/pages/hold/request/[id]/index.tsx index bbc19b47b..2f616f958 100644 --- a/pages/hold/request/[id]/index.tsx +++ b/pages/hold/request/[id]/index.tsx @@ -94,7 +94,7 @@ export default function HoldRequestPage({ const response = await fetch(`${BASE_URL}/api/hold/request/${holdId}`, { method: "POST", body: JSON.stringify({ - patronId: patronId?.value, + patronId: patronId.value, source: source?.value, pickupLocation: pickupLocation?.value, jsEnabled: true, From 6d90c08cc3861179820327935c62d36e4dd903c2 Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Mon, 16 Dec 2024 11:40:44 -0500 Subject: [PATCH 05/11] Remove destructuring of hold request id to trigger useful error logging --- src/server/api/hold.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/api/hold.ts b/src/server/api/hold.ts index d359fe0cb..8f6c5ee92 100644 --- a/src/server/api/hold.ts +++ b/src/server/api/hold.ts @@ -150,7 +150,7 @@ export async function postEDDRequest( try { const client = await nyplApiClient() const eddPostResult = await client.post("/hold-requests", eddPostParams) - const { id: requestId } = eddPostResult.data + const requestId = eddPostResult?.data?.id if (!requestId) { console.error( From 8b9e05b730b3b897e2d341271e870e56699bd127 Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Mon, 16 Dec 2024 12:53:49 -0500 Subject: [PATCH 06/11] Restore null check on patron ID for tests --- pages/api/hold/request/[id]/edd.ts | 3 ++- pages/hold/request/[id]/edd.tsx | 3 ++- pages/hold/request/[id]/index.tsx | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/pages/api/hold/request/[id]/edd.ts b/pages/api/hold/request/[id]/edd.ts index 29790c6b7..c1617b4e1 100644 --- a/pages/api/hold/request/[id]/edd.ts +++ b/pages/api/hold/request/[id]/edd.ts @@ -40,8 +40,9 @@ async function handler(req: NextApiRequest, res: NextApiResponse) { } const holdRequestResponse = await postEDDRequest({ - ...rest, itemId, + patronId, + ...rest, }) const { requestId } = holdRequestResponse diff --git a/pages/hold/request/[id]/edd.tsx b/pages/hold/request/[id]/edd.tsx index 421c3dfc1..3eb6dc46b 100644 --- a/pages/hold/request/[id]/edd.tsx +++ b/pages/hold/request/[id]/edd.tsx @@ -248,7 +248,7 @@ export async function getServerSideProps({ params, req, res }) { console.error("EDD Page - Error fetching edd in getServerSideProps") } - const isEddAvailable = eddRequestable && item.isAvailable + const isEddAvailable = eddRequestable && item.isEDDRequestable const patronEligibilityStatus = await fetchPatronEligibility(patronId) @@ -262,6 +262,7 @@ export async function getServerSideProps({ params, req, res }) { discoveryItemResult, patronId, isAuthenticated, + patronEligibilityStatus, errorStatus: locationOrEligibilityFetchFailed ? "failed" : patronEligibilityStatus.status === 401 diff --git a/pages/hold/request/[id]/index.tsx b/pages/hold/request/[id]/index.tsx index 2f616f958..bbc19b47b 100644 --- a/pages/hold/request/[id]/index.tsx +++ b/pages/hold/request/[id]/index.tsx @@ -94,7 +94,7 @@ export default function HoldRequestPage({ const response = await fetch(`${BASE_URL}/api/hold/request/${holdId}`, { method: "POST", body: JSON.stringify({ - patronId: patronId.value, + patronId: patronId?.value, source: source?.value, pickupLocation: pickupLocation?.value, jsEnabled: true, From e1ea60a00627628aa5eeeaf31ec311fd79b9d4a4 Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Mon, 16 Dec 2024 14:14:22 -0500 Subject: [PATCH 07/11] Change ineligibiltiy reasons to children rather than list item props --- .../HoldPages/PatronIneligibilityErrors.tsx | 66 +++++++++---------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/src/components/HoldPages/PatronIneligibilityErrors.tsx b/src/components/HoldPages/PatronIneligibilityErrors.tsx index 9ca756486..af86c0a10 100644 --- a/src/components/HoldPages/PatronIneligibilityErrors.tsx +++ b/src/components/HoldPages/PatronIneligibilityErrors.tsx @@ -33,39 +33,39 @@ const PatronIneligibilityErrors = ({ return ( This is because: - - Your account has expired -- Please see{" "} - - Library Terms and Conditions -- Renewing or Validating Your - Library Card - {" "} - about renewing your card. - , - ] - : []), - ...(moneyOwed - ? [ - <> - Your fines have exceeded the limit — you can pay your fines in - a branch or online from the links under{" "} - My Account. - , - ] - : []), - ...(ptypeDisallowsHolds - ? ["Your card does not permit placing holds on ReCAP materials."] - : []), - ...(reachedHoldLimit - ? ["You have reached the allowed number of holds."] - : []), - ]} - /> + + {expired ? ( + <> + Your account has expired -- Please see{" "} + + Library Terms and Conditions -- Renewing or Validating Your + Library Card + {" "} + about renewing your card. + + ) : ( + <> + )} + {moneyOwed ? ( + <> + Your fines have exceeded the limit — you can pay your fines in a + branch or online from the links under{" "} + My Account. + + ) : ( + <> + )} + {ptypeDisallowsHolds ? ( + "Your card does not permit placing holds on ReCAP materials." + ) : ( + <> + )} + {reachedHoldLimit ? ( + "You have reached the allowed number of holds." + ) : ( + <> + )} + ) } From 1d8a2653d98855ccd664d5a689c60bd4a993b734 Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Mon, 16 Dec 2024 14:16:47 -0500 Subject: [PATCH 08/11] Remove unused imports --- src/components/HoldPages/PatronIneligibilityErrors.tsx | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/components/HoldPages/PatronIneligibilityErrors.tsx b/src/components/HoldPages/PatronIneligibilityErrors.tsx index af86c0a10..e54157feb 100644 --- a/src/components/HoldPages/PatronIneligibilityErrors.tsx +++ b/src/components/HoldPages/PatronIneligibilityErrors.tsx @@ -1,10 +1,6 @@ import { Text, Box, List } from "@nypl/design-system-react-components" -import type { - HoldErrorStatus, - PatronEligibilityStatus, -} from "../../types/holdPageTypes" -import type Item from "../../models/Item" +import type { PatronEligibilityStatus } from "../../types/holdPageTypes" import RCLink from "../Links/RCLink/RCLink" import ExternalLink from "../Links/ExternalLink/ExternalLink" import { PATHS } from "../../config/constants" From f662bb151196cc87c48638297b9804833a18bf6b Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Mon, 16 Dec 2024 16:14:21 -0500 Subject: [PATCH 09/11] Extract failed request error state function --- pages/hold/request/[id]/index.tsx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pages/hold/request/[id]/index.tsx b/pages/hold/request/[id]/index.tsx index bbc19b47b..507c75e2e 100644 --- a/pages/hold/request/[id]/index.tsx +++ b/pages/hold/request/[id]/index.tsx @@ -85,6 +85,11 @@ export default function HoldRequestPage({ } }, [errorStatus, patronEligibilityStatus]) + const setFailedRequestErrorState = () => { + setFormPosting(false) + setErrorStatus("failed") + } + const handleSubmit = async (e) => { e.preventDefault() try { @@ -111,12 +116,11 @@ export default function HoldRequestPage({ break // Server side error placing the hold request case 500: - setFormPosting(false) console.error( "HoldRequestPage: Error in hold request api response", responseJson.error ) - setErrorStatus("failed") + setFailedRequestErrorState() break default: setFormPosting(false) @@ -130,8 +134,7 @@ export default function HoldRequestPage({ "HoldRequestPage: Error in hold request api response", error ) - setErrorStatus("failed") - setFormPosting(false) + setFailedRequestErrorState() } } From 565582ea0669e87bb44166a9384f887adbcc6699 Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Mon, 16 Dec 2024 16:19:07 -0500 Subject: [PATCH 10/11] Extract server error handler --- pages/hold/request/[id]/edd.tsx | 24 +++++++++++------------- pages/hold/request/[id]/index.tsx | 15 +++++++-------- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/pages/hold/request/[id]/edd.tsx b/pages/hold/request/[id]/edd.tsx index 3eb6dc46b..5db829334 100644 --- a/pages/hold/request/[id]/edd.tsx +++ b/pages/hold/request/[id]/edd.tsx @@ -93,6 +93,15 @@ export default function EDDRequestPage({ } }, [errorStatus, patronEligibilityStatus]) + const handleServerHoldPostError = (errorMessage: string) => { + console.error( + "EDDRequestPage: Error in EDD request api response", + errorMessage + ) + setFormPosting(false) + setErrorStatus("failed") + } + const postEDDRequest = async (eddParams: EDDRequestParams) => { try { setFormPosting(true) @@ -113,14 +122,8 @@ export default function EDDRequestPage({ setErrorStatus("patronIneligible") setPatronEligibilityStatus(responseJson?.patronEligibilityStatus) break - // Server side error placing the hold request case 500: - setFormPosting(false) - console.error( - "EDDRequestPage: Error in EDD request api response", - responseJson.error - ) - setErrorStatus("failed") + handleServerHoldPostError(responseJson.error) break default: setFormPosting(false) @@ -130,12 +133,7 @@ export default function EDDRequestPage({ ) } } catch (error) { - console.error( - "HoldRequestPage: Error in hold request api response", - error - ) - setErrorStatus("failed") - setFormPosting(false) + handleServerHoldPostError(error) } } diff --git a/pages/hold/request/[id]/index.tsx b/pages/hold/request/[id]/index.tsx index 507c75e2e..c040feb8b 100644 --- a/pages/hold/request/[id]/index.tsx +++ b/pages/hold/request/[id]/index.tsx @@ -85,7 +85,11 @@ export default function HoldRequestPage({ } }, [errorStatus, patronEligibilityStatus]) - const setFailedRequestErrorState = () => { + const handleServerHoldPostError = (errorMessage: string) => { + console.error( + "HoldRequestPage: Error in hold request api response", + errorMessage + ) setFormPosting(false) setErrorStatus("failed") } @@ -114,13 +118,8 @@ export default function HoldRequestPage({ setErrorStatus("patronIneligible") setPatronEligibilityStatus(responseJson?.patronEligibilityStatus) break - // Server side error placing the hold request case 500: - console.error( - "HoldRequestPage: Error in hold request api response", - responseJson.error - ) - setFailedRequestErrorState() + handleServerHoldPostError(responseJson.error) break default: setFormPosting(false) @@ -134,7 +133,7 @@ export default function HoldRequestPage({ "HoldRequestPage: Error in hold request api response", error ) - setFailedRequestErrorState() + handleServerHoldPostError(error) } } From ca74cf438e551dd208cc3fdff074be2e4c46a21f Mon Sep 17 00:00:00 2001 From: Diego Cohen Date: Mon, 16 Dec 2024 16:21:25 -0500 Subject: [PATCH 11/11] Remove ternary in hold error banner --- src/components/HoldPages/HoldRequestErrorBanner.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/HoldPages/HoldRequestErrorBanner.tsx b/src/components/HoldPages/HoldRequestErrorBanner.tsx index 0098077a5..f85ec1dca 100644 --- a/src/components/HoldPages/HoldRequestErrorBanner.tsx +++ b/src/components/HoldPages/HoldRequestErrorBanner.tsx @@ -53,7 +53,7 @@ const HoldRequestErrorBanner = ({ }} content={ - {HOLD_PAGE_CONTACT_PREFIXES?.[errorStatus] ? ( + {HOLD_PAGE_CONTACT_PREFIXES?.[errorStatus] && ( {HOLD_PAGE_CONTACT_PREFIXES?.[errorStatus]} {" Please try again, "} @@ -88,7 +88,7 @@ const HoldRequestErrorBanner = ({ for assistance, or{" "} start a new search. - ) : null} + )} {(() => { switch (errorStatus) { case "invalid":