SCC-3762 - Hold Confirmation Details #413

dgcohen · 2024-12-09T17:03:49Z

Ticket:

JIRA ticket SCC-3762

This PR does the following:

Adds the function fetchHoldDetails that returns a patron ID (used to redirect to 404 on logged-in patron ID mismatch) and the pickup location (rendered in the item table).
Renders a link to the bib in the confirmation message.
Renders a search link beneath the FAQ
Server-side redirect handling on confirmation page
Adds a distinct Item details table for confirmation (and renames the request version)

How has this been tested?

Added unit tests

Accessibility concerns or updates

NA

Checklist:

I updated the CHANGELOG with the appropriate information and JIRA ticket number (if applicable).
I have added relevant accessibility documentation for this pull request.
All new and existing tests passed.

…/hold-confirmation-server-functions

…C-3762/hold-confirmation-server-functions

charmingduchess · 2024-12-10T18:48:56Z

src/components/HoldPages/HoldConfirmationItemDetails.tsx

Can this be refactored to not use ternaries? Instead, something like {pickupLocationLabel && <.../>}.

I tried to remove the ternary but it ended up breaking the page due to a DS issue where the List component errors out with no useful message when it receives a null child instead of a fragment. I restored the ternaries and will bring this up with the DS team.

Can you provide a bib id to replicate this?

For now, can you use a regular dl tag to remove the tertiaries if it's easy to replicate the styles?

I was able to fix this by passing the list children as a listItems prop in DetailElement instead of as children.

Actually nevermind, that didn't work. There's no example bib since it always errors out whenever null is rendered in a DS component (you can try it out by replacing the ternaries here with the && syntax Vera suggested). I believe the error should be addressed in the DS, but I could be wrong.

EdwinGuzman

Looks good but some comments.

The description List issue is interesting. I think the issue is with DetailElement not returning a proper default value. Share the bib where this error is happening to replicate and test, thanks.

EdwinGuzman · 2024-12-11T01:30:00Z

src/components/HoldPages/HoldConfirmationItemDetails.tsx

Can you provide a bib id to replicate this?

EdwinGuzman · 2024-12-11T01:32:48Z

src/components/HoldPages/HoldConfirmationItemDetails.tsx

For now, can you use a regular dl tag to remove the tertiaries if it's easy to replicate the styles?

EdwinGuzman · 2024-12-11T01:52:07Z

pages/hold/confirmation/[id].tsx

+  pickupLocationLabel,
+  discoveryBibResult,
 }: HoldConfirmationPageProps) {
  const metadataTitle = `Request Confirmation | ${SITE_NAME}`
+
+  const bib = new Bib(discoveryBibResult)
+  const item = bib?.items[0]
+


Lines 50-51 is already performed in getServerSideProps. You can remove discoveryBibResult and instead return item from the server. When you do so, you might get a serialization issue but you can fix it with JSON.parse(JSON.stringify(item)).

We've run into this issue before where models can't be properly serialized in getServerSideProps since they contain functions. What we can do is pass just the needed fields but this would slightly limit the flexibility and increase the number of props passed to the component.

The alternative is to remove the model initialization on the server side to avoid running it twice, but we would need to call the discovery response parameters directly which might be fine.

You don't need bib in this component so you can only return item. It would limit to just the current fields you need but you don't need anything else according to the designs (at least for now).

EdwinGuzman · 2024-12-11T01:52:44Z

pages/hold/confirmation/[id].tsx

-              You&apos;re all set! We have received your on-site request for
-              TODO replace with bib link
-            </Text>
+            <>


nit: don't need the fragment here

EdwinGuzman · 2024-12-11T02:00:30Z

pages/hold/confirmation/[id].tsx


    if (patronId !== patronIdFromResponse) {
      throw new Error(
        "Error in HoldConfirmationPage getServerSideProps: Logged in patron Id doesn't match the patron Id in the hold request."
      )
    }

+    // TODO: Determine error state when there are errors in hold details endpoint reponse


Yeah, as we discussed in person, throwing an error here will redirect the user to a 404 page which isn't correct. From the user flow:

the patron submits a POST request and it succeeds

they get sent to the confirmation page,

the GET request with the requestId fails

Instead of redirecting to the 404 page, the patron should still be sent to the confirmation page with a message that the hold confirmation details could not be fetched. This is slightly out of scope so the TODO is correct. Create a ticket to work on proper error handling and redirecting.

EdwinGuzman · 2024-12-11T02:01:34Z

pages/hold/confirmation/[id].tsx

+    const { discoveryBibResult } = await fetchBib(bibId, {}, itemId)
+
+    if (!discoveryBibResult?.items?.length) {
+      throw new Error("Hold Confirmation Page - Item not found")


Similar idea here as to the above, so include this.

dgcohen · 2024-12-11T16:55:32Z

Looks good but some comments.

The description List issue is interesting. I think the issue is with DetailElement not returning a proper default value. Share the bib where this error is happening to replicate and test, thanks.

I'm a little reluctant to replace with a dl element since this doesn't really address the root issue of the List component not being able to accept null children and would involve duplication of styles when the fragment ternary syntax works.

EdwinGuzman · 2024-12-11T18:53:20Z

I'm a little reluctant to replace with a dl element since this doesn't really address the root issue of the List component not being able to accept null children and would involve duplication of styles when the fragment ternary syntax works.

Yeah, I agree. It's not a blocker so it can wait for a better DS fix, but you'll just have to live with the not-so-great-looking syntax for now.

EdwinGuzman · 2024-12-12T16:49:16Z

src/components/BibPage/BibDetail.tsx

@@ -14,6 +14,7 @@ import type {
 } from "../../types/bibDetailsTypes"
 import { rtlOrLtr, isItTheLastElement } from "../../utils/bibUtils"
 import type { ReactNode } from "react"
+import { child } from "winston"


Don't think you meant this but it doesn't seemed used.

EdwinGuzman

Make sure to remove the "winston" import. I'll create a ticket for the DS List issue.

dgcohen and others added 30 commits December 2, 2024 11:53

Merge branch 'main' of github.com:NYPL/research-catalog into SCC-3762…

d9fff88

…/hold-confirmation-server-functions

Merge branch 'hold-pages' of github.com:NYPL/research-catalog into SC…

ddaa479

…C-3762/hold-confirmation-server-functions

Initialize HoldPageStatusMessage component

4a78c27

Rename to HoldPageStatusMessage

01bc099

Update renamed variable references

2ba3e42

More reference name changes

584843d

Start error message refactor

a8121d1

Add patron error messages

faac548

Rename pageStatus to errorStatus

7e77b23

Check patron eligibility in hold request getServerSideProps

a1cdffa

Rename HoldRequestBanner to HoldRequestErrorBanner

dd94ac1

Make patron errors a DS list component

bfb4769

Remove 404 redirect on patron eligibility fetch failure

d5f09ef

Remove patron error fallback from body

eeebb65

Move error copy to constants file

a1d35f5

Fix error handling on edd page

0582787

Remove brackets from redirect path

9317001

Remove check on eligibility boolean

c186bc9

Add top margin to banner body

c15b8f5

Add switch statement to client side fetch

69e359b

Add eligibility check to api route

b14187d

Add variable with failed edd server responses

87d3528

Don't show reasons list when none is active

2911eb9

Don't return reason when there isn't a specific eligibility reason

3872336

Add top margin to reasons box

cf3bda6

Render item details on confirmation page

7f5e309

Remove console logs

8d9b937

Render back to search link

580b160

Add margin to search link

45b8db3

Add separate item table for confirmation page

50e4b4e

Update changelog

c2cf8c3

vercel bot deployed to Preview December 10, 2024 18:21 View deployment

dgcohen marked this pull request as ready for review December 10, 2024 18:25

dgcohen requested review from EdwinGuzman and charmingduchess December 10, 2024 18:26

charmingduchess requested changes Dec 10, 2024

View reviewed changes

Remove ternaries from HoldConfirmationItemDetails

122cded

vercel bot deployed to Preview December 10, 2024 20:07 View deployment

Comment out eligibility fetch

82533e8

vercel bot deployed to Preview December 10, 2024 22:24 View deployment

Restore fragment syntax

52a1e65

vercel bot deployed to Preview December 10, 2024 23:32 View deployment

EdwinGuzman reviewed Dec 11, 2024

View reviewed changes

Pass list children as props in detail element

5ba879f

vercel bot deployed to Preview December 11, 2024 16:28 View deployment

dgcohen added 2 commits December 11, 2024 11:34

Remove fragment

702b3ec

Restore fragments

1645a55

vercel bot deployed to Preview December 11, 2024 16:48 View deployment

dgcohen added 2 commits December 11, 2024 11:52

Remove bib and item initialization from getServerSideProps

dbc84d1

Fix comment

fdf92d6

vercel bot deployed to Preview December 11, 2024 16:54 View deployment

charmingduchess approved these changes Dec 12, 2024

View reviewed changes

EdwinGuzman reviewed Dec 12, 2024

View reviewed changes

EdwinGuzman approved these changes Dec 12, 2024

View reviewed changes

Remove winston line

e89a976

vercel bot deployed to Preview December 12, 2024 19:11 View deployment

dgcohen merged commit 8afc6fe into hold-pages Dec 12, 2024
3 checks passed

dgcohen deleted the SCC-3762/hold-confirmation-server-functions branch December 12, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCC-3762 - Hold Confirmation Details #413

SCC-3762 - Hold Confirmation Details #413

dgcohen commented Dec 9, 2024 •

edited by jira bot

Loading

charmingduchess Dec 10, 2024

dgcohen Dec 10, 2024 •

edited

Loading

EdwinGuzman Dec 11, 2024

EdwinGuzman Dec 11, 2024 •

edited by dgcohen

Loading

dgcohen Dec 11, 2024 •

edited

Loading

dgcohen Dec 11, 2024 •

edited

Loading

EdwinGuzman left a comment

EdwinGuzman Dec 11, 2024

EdwinGuzman Dec 11, 2024 •

edited by dgcohen

Loading

EdwinGuzman Dec 11, 2024

dgcohen Dec 11, 2024 •

edited

Loading

EdwinGuzman Dec 11, 2024

EdwinGuzman Dec 11, 2024

EdwinGuzman Dec 11, 2024

EdwinGuzman Dec 11, 2024

dgcohen commented Dec 11, 2024 •

edited

Loading

EdwinGuzman commented Dec 11, 2024

EdwinGuzman Dec 12, 2024

EdwinGuzman left a comment

SCC-3762 - Hold Confirmation Details #413

SCC-3762 - Hold Confirmation Details #413

Conversation

dgcohen commented Dec 9, 2024 • edited by jira bot Loading

Ticket:

This PR does the following:

How has this been tested?

Accessibility concerns or updates

Checklist:

Choose a reason for hiding this comment

dgcohen Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwinGuzman Dec 11, 2024 • edited by dgcohen Loading

Choose a reason for hiding this comment

dgcohen Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

dgcohen Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

EdwinGuzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwinGuzman Dec 11, 2024 • edited by dgcohen Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgcohen Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgcohen commented Dec 11, 2024 • edited Loading

EdwinGuzman commented Dec 11, 2024

Choose a reason for hiding this comment

EdwinGuzman left a comment

Choose a reason for hiding this comment

dgcohen commented Dec 9, 2024 •

edited by jira bot

Loading

dgcohen Dec 10, 2024 •

edited

Loading

EdwinGuzman Dec 11, 2024 •

edited by dgcohen

Loading

dgcohen Dec 11, 2024 •

edited

Loading

dgcohen Dec 11, 2024 •

edited

Loading

EdwinGuzman Dec 11, 2024 •

edited by dgcohen

Loading

dgcohen Dec 11, 2024 •

edited

Loading

dgcohen commented Dec 11, 2024 •

edited

Loading