10001 (and 10002 and 10003) Design Debt: More Granular File Validation Errors #5312
Conversation
…A modal alert would be easier and less clunky.
…including other file types; some tests
… a unique id for each file upload success element
… in attachFile; still need to fix some tests and add tests for non-pdf files and too-big files
Mwindo
changed the title
… a non-pdf.txt file in preparation for more testing
Mwindo
changed the title
| onSuccess({ selectedFile }); | ||
| }; | ||
|
|
||
| export const validateFile = async ({ |
There was a problem hiding this comment.
Is this function called anywhere apart from validateFileOnSelect in this file? In that case, do we necessarily need to export it and put it under test? Just a thought if we want to trim down the test suite for this file.
There was a problem hiding this comment.
Good point--I think you're correct! Thank you :)
There was a problem hiding this comment.
Actually, I remember why I did this now. My thinking basically boils down to the following: validateFile (even if not used anywhere else currently) is the underlying logic, independent of any file selection event. validateFileOnSelect is the way we happen to use it in our UI, and this method (and its tests) alone is what we would replace if we opted to go a more "Cerebral" route; validateFile (and its tests) could be left alone. So this separation decouples the UI implementation from the validation logic.
What do you think about this idea?
|
|
||
| describe('upload court issued document validations', () => { | ||
| const docketNumber = '102-67'; // Any existing docket number works | ||
| const encoding = 'binary'; // Some Cypress tests were not mimicking real app behavior without manually setting the encoding to binary |
There was a problem hiding this comment.
The reason (I think) we need to manually set the encoding: corrupt/encrypted files make it difficult for cypress with cypress-file-upload to determine the right encoding. E.g., we want a corrupt pdf to be treated as a binary file, but because it is corrupt cypress thinks it is something else.
TL;DR: When something goes wrong with a (typically PDF) file upload, the user doesn't know until we try to parse it on the backend, at which point we only display a generic error. This means 1) the error is often not specific enough to be helpful and 2) the error might come at the end of a long form rather than as soon as the user selects the file. This PR addresses the issue.
This should free up the court's time addressing frequent help tickets (multiple per day) regarding failed file uploads. It will also prevent corrupted/invalid files from being uploaded to S3.
N.B.: The PR diff is not as intimidating as it looks. A lot of the diff is refactoring.
Manual step: the usataxcourt website FAQ should be updated at https://ustaxcourt.gov/dawson_faqs_case_management.html#FileUpload
Tickets: 10001 (password-protected PDFs), 10002 (invalid files types), 10003 (invalid/corrupted PDFs). (But note that this PR is best summarized as "implement client-side file validation." )
Figma here.
The Approach in This PR
In order to provide the user immediate feedback (rather than waiting until they try to submit), we use pdf-js client-side on file selection to check for issues. If issues are discovered--e.g., the PDF is password-protected or the wrong file type--we show a modal with an error message and clear the file selection. (While validating--which is usually very quick--we display a loading overlay.) This sidesteps difficulties with joi document validation.
In addition, we send an error message to the backend for logging. This error message is slightly decoupled from what is shown to the user in order for us to have finer-grained control over what is logged. (E.g., while we may display the same generic error message for multiple types of file parsing errors, we log them more granularly for debugging/prod-support purposes.)
While the front-end messaging has been overhauled, this PR does not change our back-end PDF validation implementation at all. In other words, the client-side validation is there for convenience to filter out bad documents, but we still validate on the back end.
The validation has been added to (almost) everywhere we upload files. (I was told to ignore
PractitionerAddEditDocumentation, which might be changed soon, andPetitionQcScanBatchPreview, which, despite some code for uploading, doesn't seem to actually allow uploads.)Examples of New Error Messages
Screen.Recording.2024-09-10.at.3.54.42.PM.mov