103296 update uploads controller unit tests

[brostk]

Note: Delete the description statements, complete each step. None are optional, but can be justified as to why they cannot be completed as written. Provide known gaps to testing that may raise the risk of merging to production.

Summary

• This work is behind a feature toggle (flipper): NO
• added an else condition, with corresponding unit tests, for invalid form IDs being passed to submit_supporting_documents
• added tests to cover when the require_all_s3_success feature is enabled
• added additional handle_uploads tests for other error conditions
• added submit_supporting_documents tests for error conditions
• rewrote get_attachment_ids_and_forms tests to test method functionality, not mocks
• rewrote supporting_document_ids tests to test method functionality, not mocks
• removed generate_attachment_ids test, since the method does not exist and only tested mock functionality

Related issue(s)

• Implement unit tests for uploads_controller.rb va.gov-team#103296

Testing done

• New code is covered by unit tests
• The new else condition on submit_supporting_documents is validated with new unit tests, which test both successful and failure cases
• All unit tests are passing

Screenshots

Note: Optional

What areas of the site does it impact?

• The uploads_controller change now throws an error when an invalid form ID is passed to the controller, when previously we were returning a 204 even though nothing was done on the server; arguably this should be an error condition because the form ID is not submitted in this case. This would occur when submitting IVC CHAMPVA forms to the backend with an invalid form ID, which should not occur due to the way the front end is coded.

Acceptance criteria

• I fixed|updated|added unit tests and integration tests for each feature (if applicable).
• No error nor warning in the console.
• Events are being sent to the appropriate logging solution
• Documentation has been updated (link to documentation)
• No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
• Feature/bug has a monitor built into Datadog (if applicable)
• If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
• I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

[github-actions]

1 Warning

⚠️

This PR changes 314 LoC (not counting whitespace/newlines). In order to ensure each PR receives the proper attention it deserves, we recommend not exceeding 200. Expect some delays getting reviews. File Summary Files modules/ivc_champva/app/controllers/ivc_champva/v1/uploads_controller.rb (+5/-0) modules/ivc_champva/spec/requests/ivc_champva/v1/forms/uploads_spec.rb (+231/-78) Note: We exclude files matching the following when considering PR size: *.csv, *.json, *.tsv, *.txt, *.md, Gemfile.lock, app/swagger, modules/mobile/docs, spec/fixtures/, spec/support/vcr_cassettes/, modules/mobile/spec/support/vcr_cassettes/, db/seeds, modules/vaos/app/docs, modules/meb_api/app/docs, modules/appeals_api/app/swagger/, *.bru, *.pdf Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

[michaelclement]

I suspect this is because the retry logic inside handle_file_uploads fails - that might cause [statuses, error_messages] to be undefined, which is probably the source of these undefined method 'uniq' for nil errors we see from time to time.

Not sure we need to do anything about this right now, but this is a good lead for a followup ticket to fix that bug.

[michaelclement]

(To answer the question, I believe it should be 400/'Upload failed')

[brostk]

Agreed. You're right - inside handle_file_uploads, we're returning [statuses, error_messages] without them necessarily having been initialized. 400/'Upload failed' would definitely be more appropriate.

[michaelclement]

Nice work!