patch: file upload errors

[kitttang]

Note

This template is designed to help both contributors and maintainers. It is a checklist to ensure all necessary
information is provided, and prompts contributors on any contribution guidelines they have missed.

Do not remove sections.
They are important for the review process and help maintainers ensure quality and good documentation across the
project.

Some checkboxes will not apply to every change, so feel free to leave them unchecked if they are not relevant.

Description

Context

Changes

Repairing ability for errors received from the upload response to show on the page rather than going to the 500 page
Stopping that error from being shown twice
Adding a new error for when too many files are uploaded
Changing mention of file to files and removing the field name to match GDS

Type of change

What is the type of change you are making?

• Chore or documentation (non-breaking change that does not add functionality)
• ADR (Architectural Decision Record, non-breaking change that documents or proposes a decision)
• Refactor (non-breaking change that improves code quality)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

PR title

PR titles should be prefixed with the type of change you are making, based on the README.md#versioning.
This is so that when performing a squash merge, the PR title is automatically used as the commit message.

Have you updated the PR title to match the type of change you are making?

• Yes
• No, I need help or guidance

Testing

Automated tests

Have you added automated tests?

• Yes, unit or integration tests
• Yes, end-to-end (cypress) tests
• No, tests are not required for this change
• No, I need help or guidance
• No (explain why tests are not required or can't be added at this time)

Manual tests

Have you manually tested your changes?

• Yes
• No, manual tests are not required or sufficiently covered by automated tests

Have you attached an example form JSON or snippet for the reviewer in this PR?

• Yes
• No, any existing form can be used
• No, it is not required or not applicable

Steps to test

1. Use file upload field to upload too many files
2. Return a json containing errorCode: TOO_MANY_FILES and optionally maxFilesPerUpload: 10
3. Press the submit button
4. An error should display on the page saying 'You can only select up to 10 files at the same time' if you provided maxFilesPerUpload, otherwise 'You have selected too many files'

Documentation

Have you updated the documentation?

• Yes, I have updated ./docs for this change since additional explanation or steps to use/configure the feature is required
• Yes, I have added or updated an ADR for this change since it is large, complex, or has significant architectural implications
• Yes, I have added inline comments for hard-to-understand areas
• No, I am not sure if documentation is required
• No, documentation is not required for this change

Discussion

Warning

Large or complex changes may require discussion with the maintainers before they can be merged. If it has not yet been discussed, it may delay the review process

Have you discussed this change with the maintainers?

• Yes, I have discussed this change with the maintainers on slack, email or via GitHub issues
• Yes, this change is an ADR to help kick-off discussion
• No, this change is small and does not require discussion
• No, I am not sure if one is required

[jenbutongit]

please keep since we don't want to lose any errors that have accumulated up to this point

[kitttang]

@jenbutongit Is it safe to delete error writing in the if (error) on line 102 instead?

And is keeping "response = uploadService.parsedDocumentUploadResponse(err.data);" okay? I've done this so "const { location, warning, error } = response;" can unpack

[superafroman]

Doing it in this way will result in the error being added to request.pre.errors twice (when an error is thrown) - you add response.error in the catch, then unpack the response and add it again in the if(error) block. Do you need the response when an error is thrown? (e.g. are you using location or warning?)

[jenbutongit]

@superafroman Do you want to resolve/review since the implementation has changed? My initial comment is also redundant.

As a side note - I've noticed an issue with the code (not introduced by this change). uploadService.parsedDocumentUploadResponse is called internally in uploadService, but is called again on err.data. We have httpService.request (and httpService.post) which looks like it wants to suppress the thrown error and return it in an object instead

Looking at uploadService.uploadDocuments - I think the intention is that it returns an object containing { location, error, warning } if there was a document upload API error but is never called since post request throws.

I'm fine to resolve this in another PR/issue though.

[jenbutongit]

I think this can be omitted, since it's handled in l51-53. I think that should remove the need for the set as well?

[kitttang]

if you have an error which doesn't go into the catch block, then it will not be added to the errors list (which is how the file upload tests work) so it needs to be added here

[jenbutongit]

as an fyi - attaching the file I'm using to test different error cases files.json

I'm also testing with https://github.com/UKForeignOffice/document-upload

I'm testing these regressions

1. if the file exceeds the runner's limits (see default.js)
2. if the file exceeds the document upload api's limit
3. if the file sends a warning