Merge pull request #427 from NYPL/SCC-4427/vqa-round-2-fixes

SCC-4427 - VQA Round 2 Fixes
NYPL · Dec 23, 2024 · 4ec7c43 · 4ec7c43
2 parents 2d116fe + 8b2d38d
commit 4ec7c43
Show file tree

Hide file tree

Showing 11 changed files with 151 additions and 118 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -14,12 +14,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - 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)
+- Added HoldContactButton component and update error copy per VQA requests (SCC-4427)
 - Added email address field prepopulation on EDD Request form (SCC-4407)
 
 ### Updated
 
 - Updated phone, email, notification preference and home library to be individually editable in Account Settings (SCC-4337, SCC-4254, SCC-4253)
 - Updated username to be editable in My Account header (SCC-4236)
+- Format patron ineligibility reasons as list only when more than one (SCC-4427)
+- Disable Hold Request submit button when patron is ineligible (SCC-4427)
 
 ## [1.3.6] 2024-11-6
 

diff --git a/__test__/pages/hold/eddRequestPage.test.tsx b/__test__/pages/hold/eddRequestPage.test.tsx
@@ -16,8 +16,6 @@ import initializePatronTokenAuth, {
 import { fetchBib } from "../../../src/server/api/bib"
 import { bibWithItems, bibWithSingleAeonItem } from "../../fixtures/bibFixtures"
 import {
-  BASE_URL,
-  PATHS,
   EDD_FORM_FIELD_COPY,
   HOLD_PAGE_ERROR_HEADINGS,
 } from "../../../src/config/constants"
@@ -255,18 +253,14 @@ describe("EDD Request page", () => {
 
       expect(
         screen.queryByText(
-          "We were unable to process your request at this time. Please try again, ",
+          "We were unable to process your request at this time. Please ",
           { exact: false }
         )
       ).toBeInTheDocument()
 
       expect(
         screen.getByRole("button", { name: "contact us" })
       ).toBeInTheDocument()
-
-      expect(
-        screen.getByText("start a new search", { exact: false })
-      ).toHaveAttribute("href", `${BASE_URL}${PATHS.SEARCH}`)
     })
 
     it("populates the feedback form with the call number and appropriate copy when the request fails", async () => {
@@ -405,12 +399,6 @@ describe("EDD Request page", () => {
         })
       ).toBeInTheDocument()
 
-      expect(
-        screen.getByText("This is because:", {
-          exact: false,
-        })
-      ).toBeInTheDocument()
-
       expect(
         screen.getByText("Your account has expired", {
           exact: false,
@@ -437,6 +425,8 @@ describe("EDD Request page", () => {
           exact: false,
         })
       ).toBeInTheDocument()
+
+      expect(screen.getByText("Submit request")).toBeDisabled()
     })
   })
 })
diff --git a/__test__/pages/hold/requestPage.test.tsx → __test__/pages/hold/holdRequestPage.test.tsx b/__test__/pages/hold/requestPage.test.tsx → __test__/pages/hold/holdRequestPage.test.tsx
@@ -15,8 +15,6 @@ import initializePatronTokenAuth, {
 import { fetchBib } from "../../../src/server/api/bib"
 import { bibWithItems, bibWithSingleAeonItem } from "../../fixtures/bibFixtures"
 import {
-  BASE_URL,
-  PATHS,
   NYPL_LOCATIONS,
   HOLD_PAGE_ERROR_HEADINGS,
 } from "../../../src/config/constants"
@@ -248,18 +246,14 @@ describe("Hold Request page", () => {
 
       expect(
         screen.queryByText(
-          "We were unable to process your request at this time. Please try again, ",
+          "We were unable to process your request at this time. Please ",
           { exact: false }
         )
       ).toBeInTheDocument()
 
       expect(
         screen.getByRole("button", { name: "contact us" })
       ).toBeInTheDocument()
-
-      expect(
-        screen.getByText("start a new search", { exact: false })
-      ).toHaveAttribute("href", `${BASE_URL}${PATHS.SEARCH}`)
     })
 
     it("populates the feedback form with the call number and appropriate copy when the request fails", async () => {
@@ -339,12 +333,6 @@ describe("Hold Request page", () => {
         })
       ).toBeInTheDocument()
 
-      expect(
-        screen.getByText("This is because:", {
-          exact: false,
-        })
-      ).toBeInTheDocument()
-
       expect(
         screen.getByText("Your account has expired", {
           exact: false,
@@ -371,6 +359,8 @@ describe("Hold Request page", () => {
           exact: false,
         })
       ).toBeInTheDocument()
+
+      expect(screen.getByText("Submit request")).toBeDisabled()
     })
   })
 })
diff --git a/pages/hold/request/[id]/edd.tsx b/pages/hold/request/[id]/edd.tsx
@@ -179,6 +179,7 @@ export default function EDDRequestPage({
             setEddFormState={setEddFormState}
             handleSubmit={postEDDRequest}
             setErrorStatus={setErrorStatus}
+            errorStatus={errorStatus}
             holdId={holdId}
           />
         ) : null}

diff --git a/pages/hold/request/[id]/index.tsx b/pages/hold/request/[id]/index.tsx
@@ -180,6 +180,7 @@ export default function HoldRequestPage({
               handleSubmit={handleSubmit}
               holdId={holdId}
               patronId={patronId}
+              errorStatus={errorStatus}
               source={item.formattedSourceForHoldRequest}
             />
           </>

diff --git a/src/components/HoldPages/EDDRequestForm.tsx b/src/components/HoldPages/EDDRequestForm.tsx
@@ -20,6 +20,7 @@ import {
   validateEDDForm,
   initialEDDInvalidFields,
   isInvalidField,
+  holdButtonDisabledStatuses,
 } from "../../utils/holdPageUtils"
 import type {
   EDDRequestParams,
@@ -32,6 +33,7 @@ interface EDDRequestFormProps {
   handleSubmit: (eddParams: EDDRequestParams) => void
   setErrorStatus: (errorStatus: HoldErrorStatus) => void
   holdId: string
+  errorStatus?: HoldErrorStatus
 }
 
 /**
@@ -43,6 +45,7 @@ const EDDRequestForm = ({
   handleSubmit,
   setErrorStatus,
   holdId,
+  errorStatus,
 }: EDDRequestFormProps) => {
   // Set the invalid fields as an array in state to keep track of the first invalid field for focus on submit
   const [invalidFields, setInvalidFields] = useState(initialEDDInvalidFields)
@@ -76,15 +79,14 @@ const EDDRequestForm = ({
 
   const validateAndSubmit = async (e: React.FormEvent<HTMLFormElement>) => {
     e.preventDefault()
+    const newValidatedFields = validateEDDForm(eddFormState, invalidFields)
 
     // Validate the form on submission in case the user hasn't typed in all the required fields
-    setInvalidFields((prevInvalidFields) =>
-      validateEDDForm(eddFormState, prevInvalidFields)
-    )
+    setInvalidFields(newValidatedFields)
 
     // Find the first invalid field and focus on it
-    const firstInvalidField = invalidFields.find(
-      (firstInvalidFieldKey) => firstInvalidFieldKey.isInvalid
+    const firstInvalidField = newValidatedFields.find(
+      (validatedFieldKey) => validatedFieldKey.isInvalid
     )
 
     // Prevent form submission and focus on first invalid field if there is one
@@ -121,7 +123,7 @@ const EDDRequestForm = ({
         value={eddFormState.source}
       />
       <Box>
-        <Heading level="h3" size="heading4" mb="m">
+        <Heading level="h3" size="heading4" mb="xs">
           Required information
         </Heading>
         <Text noSpace>
@@ -138,7 +140,6 @@ const EDDRequestForm = ({
           name="emailAddress"
           value={eddFormState.emailAddress}
           labelText={EDD_FORM_FIELD_COPY.emailAddress.label}
-          isRequired
           placeholder={EDD_FORM_FIELD_COPY.emailAddress.placeholder}
           helperText={EDD_FORM_FIELD_COPY.emailAddress.helperText}
           invalidText={EDD_FORM_FIELD_COPY.emailAddress.invalidText}
@@ -157,7 +158,6 @@ const EDDRequestForm = ({
             name="startPage"
             value={eddFormState.startPage}
             labelText={EDD_FORM_FIELD_COPY.startPage.label}
-            isRequired
             placeholder={EDD_FORM_FIELD_COPY.startPage.placeholder}
             helperText={EDD_FORM_FIELD_COPY.startPage.helperText}
             invalidText={EDD_FORM_FIELD_COPY.startPage.invalidText}
@@ -173,7 +173,6 @@ const EDDRequestForm = ({
             name="endPage"
             value={eddFormState.endPage}
             labelText={EDD_FORM_FIELD_COPY.endPage.label}
-            isRequired
             placeholder={EDD_FORM_FIELD_COPY.endPage.placeholder}
             helperText={EDD_FORM_FIELD_COPY.endPage.helperText}
             invalidText={EDD_FORM_FIELD_COPY.endPage.invalidText}
@@ -184,13 +183,12 @@ const EDDRequestForm = ({
           />
         </FormField>
       </FormRow>
-      <FormField>
+      <FormField mb="xs">
         <TextInput
           id="chapterTitle"
           name="chapterTitle"
           value={eddFormState.chapterTitle}
           labelText={EDD_FORM_FIELD_COPY.chapterTitle.label}
-          isRequired
           placeholder={EDD_FORM_FIELD_COPY.chapterTitle.placeholder}
           helperText={EDD_FORM_FIELD_COPY.chapterTitle.helperText}
           invalidText={EDD_FORM_FIELD_COPY.chapterTitle.invalidText}
@@ -251,7 +249,7 @@ const EDDRequestForm = ({
           />
         </FormField>
       </FormRow>
-      <FormField>
+      <FormField mb="xs">
         <TextInput
           id="requestNotes"
           name="requestNotes"
@@ -265,7 +263,11 @@ const EDDRequestForm = ({
       </FormField>
       <CopyrightRestrictionsBanner />
       <ButtonGroup>
-        <Button id="edd-request-submit" type="submit">
+        <Button
+          id="edd-request-submit"
+          type="submit"
+          isDisabled={holdButtonDisabledStatuses.includes(errorStatus)}
+        >
           Submit request
         </Button>
       </ButtonGroup>

diff --git a/src/components/HoldPages/HoldContactButton.tsx b/src/components/HoldPages/HoldContactButton.tsx
@@ -0,0 +1,54 @@
+import { Button } from "@nypl/design-system-react-components"
+import { useContext, type ReactNode } from "react"
+
+import type Item from "../../models/Item"
+import type { ItemMetadata } from "../../types/itemTypes"
+import { FeedbackContext } from "../../context/FeedbackContext"
+
+interface HoldContactButtonProps {
+  item: Item
+  children: ReactNode
+}
+
+export const HoldContactButton = ({
+  item,
+  children,
+}: HoldContactButtonProps) => {
+  const { onOpen, setItemMetadata } = useContext(FeedbackContext)
+
+  const onContact = (metadata: ItemMetadata) => {
+    setItemMetadata(metadata)
+    onOpen()
+  }
+
+  return (
+    <Button
+      id="hold-contact"
+      onClick={() =>
+        onContact({
+          id: item.id,
+          barcode: item.barcode,
+          callNumber: item.callNumber,
+          bibId: item.bibId,
+          notificationText: `Request failed for call number ${item.callNumber}`,
+        })
+      }
+      buttonType="link"
+      // TODO: Ask DS team to make button link variant match the default link styles
+      sx={{
+        display: "inline",
+        fontWeight: "inherit",
+        fontSize: "inherit",
+        p: 0,
+        height: "auto",
+        textAlign: "left",
+        minHeight: "auto",
+        textDecorationStyle: "dotted",
+        textDecorationThickness: "1px",
+        textUnderlineOffset: "2px",
+      }}
+    >
+      {children}
+    </Button>
+  )
+}
diff --git a/src/components/HoldPages/HoldRequestErrorBanner.tsx b/src/components/HoldPages/HoldRequestErrorBanner.tsx
@@ -1,17 +1,15 @@
-import { useContext } from "react"
-import { Text, Box, Banner, Button } from "@nypl/design-system-react-components"
+import { Text, Box, Banner } 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 { HoldContactButton } from "./HoldContactButton"
 import {
-  PATHS,
   HOLD_PAGE_ERROR_HEADINGS,
   HOLD_PAGE_CONTACT_PREFIXES,
 } from "../../config/constants"
@@ -28,16 +26,9 @@ interface HoldRequestErrorBannerProps {
  */
 const HoldRequestErrorBanner = ({
   item,
-  errorStatus = "failed",
+  errorStatus = "patronIneligible",
   patronEligibilityStatus,
 }: HoldRequestErrorBannerProps) => {
-  const { onOpen, setItemMetadata } = useContext(FeedbackContext)
-
-  const onContact = (metadata: ItemMetadata) => {
-    setItemMetadata(metadata)
-    onOpen()
-  }
-
   return (
     <Banner
       type="negative"
@@ -54,39 +45,11 @@ const HoldRequestErrorBanner = ({
       content={
         <Box>
           {HOLD_PAGE_CONTACT_PREFIXES?.[errorStatus] && (
-            <Text>
+            <Text noSpace mt="xs">
               {HOLD_PAGE_CONTACT_PREFIXES?.[errorStatus]}
-              {" Please try again, "}
-              <Button
-                id="hold-contact"
-                onClick={() =>
-                  onContact({
-                    id: item.id,
-                    barcode: item.barcode,
-                    callNumber: item.callNumber,
-                    bibId: item.bibId,
-                    notificationText: `Request failed for call number ${item.callNumber}`,
-                  })
-                }
-                buttonType="link"
-                // TODO: Ask DS team to make button link variant match the default link styles
-                sx={{
-                  display: "inline",
-                  fontWeight: "inherit",
-                  fontSize: "inherit",
-                  p: 0,
-                  height: "auto",
-                  textAlign: "left",
-                  minHeight: "auto",
-                  textDecorationStyle: "dotted",
-                  textDecorationThickness: "1px",
-                  textUnderlineOffset: "2px",
-                }}
-              >
-                contact us
-              </Button>{" "}
-              for assistance, or{" "}
-              <RCLink href="/search">start a new search.</RCLink>
+              {" Please "}
+              <HoldContactButton item={item}>contact us</HoldContactButton> for
+              assistance.
             </Text>
           )}
           {(() => {
@@ -97,6 +60,7 @@ const HoldRequestErrorBanner = ({
                 return patronEligibilityStatus ? (
                   <PatronIneligibilityErrors
                     patronEligibilityStatus={patronEligibilityStatus}
+                    item={item}
                   />
                 ) : null
               default: