SCC-3762 - Patron Eligibility Errors #415

dgcohen · 2024-12-10T16:34:55Z

Ticket:

JIRA ticket SCC-3762

This PR does the following:

Server function fetchPatronEligibility that fetches a patron's eligibility status from Discovery API
HoldRequestErrorBanner component responsible for showing the user request errors based on the request error status (including ineligibilty reasons).
Eligibility validation on the server side.

How has this been tested?

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.

vercel · 2024-12-10T16:34:59Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
research-catalog	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 16, 2024 9:30pm

EdwinGuzman

Some minor comments. Can you share a few test patron accounts to try to get all use cases locally? (this can be done on slack)

pages/api/hold/request/[id]/edd.ts

pages/api/hold/request/[id]/index.ts

pages/hold/request/[id]/edd.tsx

pages/hold/request/[id]/index.tsx

src/components/HoldPages/HoldRequestErrorBanner.tsx

EdwinGuzman · 2024-12-12T17:53:20Z

pages/api/hold/request/[id]/edd.ts

+          })
+
+        // Server side redirect on ineligibility when Js is disabled
+        // TODO: Move this to seaprate API route


Why do you want to move this to a separate API route?

To be able to remove the jsEnabled flag from the client call.

EdwinGuzman

Latest round looks good. Some comments but I think these should be fixed:

When going to an EDD page, the exact reason does not come up. Shouldn't it? It does come up for the hold request form:
When a patron cannot place holds on either Holds or EDD, disable the form. I can still submit a request on the on-site use page and can fill out the EDD form:
The following is low priority so not necessary for this PR. The no-js form submission redirects to a broken result:

EdwinGuzman · 2024-12-12T17:54:33Z

pages/api/hold/request/[id]/edd.ts

+
+        // Server side redirect on ineligibility when Js is disabled
+        // TODO: Move this to seaprate API route
+        // TODO: Parse these query params in getServerSideProps


There are no query params. This brings up a good point. I was a bit worried about how this was being done, but now I see the POST request is doing the eligibility check, then the redirect happens, then the page does the eligibility check again in getServerSideProps. I don't think there's a harm in checking twice, but ideally, this would only happen once.

Although, I have no good suggestions but I think this is a problem you can think about and recommend solution(s) later.

EdwinGuzman · 2024-12-12T18:06:11Z

src/server/api/hold.ts

+      error.message
+    )
+
+    return { status: 500 }


Can you add a message and say it was a server issue? It seems this would be the case when the NYPL Platform eligibility endpoint is down, right?

charmingduchess · 2024-12-12T15:27:22Z

pages/api/hold/request/[id]/edd.ts

@@ -40,7 +60,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
    // JS-Disabled functionality

    // Redirect to confirmation page
-    res.redirect(
+    return res.redirect(


was this not redirecting without the return statement here?

it was but i added it for consistency after adding it elsewhere as one of Edwin's suggestions.

charmingduchess · 2024-12-12T15:29:27Z

pages/api/hold/request/[id]/edd.ts

Can we combine the edd and physical hold request api routes?

yes but i think we should do this as a follow-up optimization ticket since it'll involve potentially refactoring the posting functions as well.

charmingduchess · 2024-12-12T20:17:20Z

pages/hold/request/[id]/index.tsx

-          patronId: patronId.value,
-          source: source.value,
-          pickupLocation: pickupLocation.value,
+          patronId: patronId?.value,


What case would allow for this request to be made with undefined values for these required fields? I think the optional chaining implies that case is expected.

This maybe was unclear. Can you remove the optional chaining? I don't see why we would want to send a hold request that didn't have these values defined.

pages/hold/request/[id]/index.tsx

src/components/HoldPages/HoldRequestErrorBanner.tsx

src/components/HoldPages/PatronIneligibilityErrors.tsx

__test__/pages/hold/eddRequestPage.test.tsx

dgcohen · 2024-12-16T19:10:49Z

Latest round looks good. Some comments but I think these should be fixed:

When going to an EDD page, the exact reason does not come up. Shouldn't it? It does come up for the hold request form:

When a patron cannot place holds on either Holds or EDD, disable the form. I can still submit a request on the on-site use page and can fill out the EDD form:

The following is low priority so not necessary for this PR. The no-js form submission redirects to a broken result:

Fixed both of the issues you pointed out except for the No-js one, opening a follow up ticket for that.

EdwinGuzman

The updates look good but tbh I cannot test this locally. I get merge conflicts when I pull locally. I think it's because of a force push in the history. So I will approve based on the updates but will have to test on train-research-catalog once it's deployed.

charmingduchess · 2024-12-17T15:44:25Z

pages/hold/request/[id]/index.tsx

-          patronId: patronId.value,
-          source: source.value,
-          pickupLocation: pickupLocation.value,
+          patronId: patronId?.value,


This maybe was unclear. Can you remove the optional chaining? I don't see why we would want to send a hold request that didn't have these values defined.

dgcohen changed the base branch from main to hold-pages December 10, 2024 16:35

dgcohen changed the title ~~Scc 3762/patron eligibility errors~~ SCC-3762 - Patron Eligibility Errors Dec 10, 2024

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

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

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

dgcohen requested review from EdwinGuzman and charmingduchess and removed request for EdwinGuzman December 11, 2024 14:47

dgcohen marked this pull request as ready for review December 11, 2024 14:47

dgcohen requested a review from EdwinGuzman December 11, 2024 14:47

vercel bot deployed to Preview December 11, 2024 15:57 View deployment

EdwinGuzman reviewed Dec 11, 2024

View reviewed changes

vercel bot deployed to Preview December 11, 2024 21:09 View deployment

EdwinGuzman reviewed Dec 12, 2024

View reviewed changes

charmingduchess requested changes Dec 12, 2024

View reviewed changes

dgcohen added 2 commits December 12, 2024 16:42

Manually fix merge conflicts

4fb4f0e

Fix eligibility type

dc86933

dgcohen force-pushed the SCC-3762/patron-eligibility-errors branch from 85187f9 to dc86933 Compare December 12, 2024 21:50

vercel bot deployed to Preview December 12, 2024 21:51 View deployment

dgcohen added 2 commits December 16, 2024 09:48

Use constant for copy in test

5915469

Remove optional chaining from patronIds

90427b0

vercel bot deployed to Preview December 16, 2024 14:53 View deployment

dgcohen added 2 commits December 16, 2024 11:40

Remove destructuring of hold request id to trigger useful error logging

6d90c08

Restore null check on patron ID for tests

8b9e05b

vercel bot deployed to Preview December 16, 2024 17:55 View deployment

dgcohen added 2 commits December 16, 2024 14:14

Change ineligibiltiy reasons to children rather than list item props

e1ea60a

Remove unused imports

1d8a265

dgcohen added 3 commits December 16, 2024 16:14

Extract failed request error state function

f662bb1

Extract server error handler

565582e

Remove ternary in hold error banner

ca74cf4

vercel bot deployed to Preview December 16, 2024 21:30 View deployment

EdwinGuzman approved these changes Dec 16, 2024

View reviewed changes

dgcohen requested a review from charmingduchess December 17, 2024 15:01

charmingduchess requested changes Dec 17, 2024

View reviewed changes

charmingduchess approved these changes Dec 17, 2024

View reviewed changes

dgcohen merged commit 429a2eb into hold-pages Dec 17, 2024
3 checks passed

dgcohen deleted the SCC-3762/patron-eligibility-errors branch December 17, 2024 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCC-3762 - Patron Eligibility Errors #415

SCC-3762 - Patron Eligibility Errors #415

dgcohen commented Dec 10, 2024 •

edited

Loading

vercel bot commented Dec 10, 2024 •

edited

Loading

EdwinGuzman left a comment

EdwinGuzman Dec 12, 2024

dgcohen Dec 12, 2024

EdwinGuzman left a comment

EdwinGuzman Dec 12, 2024

EdwinGuzman Dec 12, 2024

EdwinGuzman Dec 12, 2024

charmingduchess Dec 12, 2024

dgcohen Dec 16, 2024

charmingduchess Dec 12, 2024

dgcohen Dec 16, 2024 •

edited

Loading

charmingduchess Dec 12, 2024

charmingduchess Dec 17, 2024

charmingduchess Dec 17, 2024

dgcohen commented Dec 16, 2024

EdwinGuzman left a comment

charmingduchess Dec 17, 2024

SCC-3762 - Patron Eligibility Errors #415

SCC-3762 - Patron Eligibility Errors #415

Conversation

dgcohen commented Dec 10, 2024 • edited Loading

Ticket:

This PR does the following:

How has this been tested?

Accessibility concerns or updates

Checklist:

vercel bot commented Dec 10, 2024 • edited Loading

EdwinGuzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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 Dec 16, 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

dgcohen commented Dec 16, 2024

EdwinGuzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgcohen commented Dec 10, 2024 •

edited

Loading

vercel bot commented Dec 10, 2024 •

edited

Loading

dgcohen Dec 16, 2024 •

edited

Loading