Scc 4405/hold pages js disabled

[dgcohen]

Ticket:

• JIRA ticket SCC-4405

This PR does the following:

• Adds server-side validation on EDD hold requests
• Fixes bugs causing errors on No-js hold requests (related to JSON parsing/query string formatting)
• Adds formState and validatedFields query params to populate field values/validation state on refreshes/js-disabled redirects.
• Renames initialEDDInvalidFields to defaultValidatedEDDFields

How has this been tested?

• Added unit tests

Accessibility concerns or updates

• I thought about adding autofocus when js is disabled but it ended up adding a bunch of code, so not sure if it's worth it. Let me know if you think this is important (will check with Clare as well) and I'll add it back.

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]

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	Jan 3, 2025 6:59pm

[EdwinGuzman]

This works very well. I left a comment about the general architecture but some questions:

• shouldn't the query params be escaped?
• it feels a bit unsafe having the patron's email in the query params. I don't think there's anything that can be done.
• What if a value like requestNotes is very long? Thankfully, I think the limit is big so there's no need to worry but something to keep in mind.

example: validatedFields=[{"key":"emailAddress","isInvalid":false},{"key":"startPage","isInvalid":true},{"key":"endPage","isInvalid":true},{"key":"chapterTitle","isInvalid":true}]&formState={"source":"sierra-nypl","emailAddress":"[email protected]","startPage":"","endPage":"","chapterTitle":"","author":"","date":"","volume":"","issue":"","requestNotes":""}

[EdwinGuzman]

Is it possible to pass the formInvalid object to the component so you can then remove the router.query?.formState call? The values are already being parsed in the server so it doesn't seem necessary to do it again in the client.

[dgcohen]

The router.query?.formState call is responsible for pre-populating the form fields, whereas the formInvalid query param is just responsible for setting the page's status. I could derive the "invalid" status using the form's state, but it would require some weird changes to the component (moving the error status setting from the submit handler to a useEffect), and I felt that this was cleaner since there was already a switch statement in getServerSideProps that sets the page status before the component renders.

[charmingduchess]

Just cause you mentioned there being a lot happening in the getServerSideProps, maybe the email fetch can be moved to the server/api/hold file with the other fetchers

[dgcohen]

This works very well. I left a comment about the general architecture but some questions:

• shouldn't the query params be escaped?
• it feels a bit unsafe having the patron's email in the query params. I don't think there's anything that can be done.
• What if a value like requestNotes is very long? Thankfully, I think the limit is big so there's no need to worry but something to keep in mind.

example: validatedFields=[{"key":"emailAddress","isInvalid":false},{"key":"startPage","isInvalid":true},{"key":"endPage","isInvalid":true},{"key":"chapterTitle","isInvalid":true}]&formState={"source":"sierra-nypl","emailAddress":"[email protected]","startPage":"","endPage":"","chapterTitle":"","author":"","date":"","volume":"","issue":"","requestNotes":""}

The query params should definitely be escaped. I'm seeing them escaped in chrome's address bar (same as in the unit test), where are you seeing them unescaped?

Not sure if I understand the security concern about the email address in the url. Wouldn't the patron who submitted the form be the only one who sees the redirect url (not including NYPL staff)?