Validate document attachments at time of upload #19532

danlim715 · 2024-11-20T16:49:12Z

Summary

This work is behind a feature toggle (flipper): NO
Added benefits intake PDF validation logic for all attachment to run at the time of upload. This change should help catch PDF validation failures before application submission and mitigate failures downstream.
Pension benefits team to own this work.

Related issue(s)

Link to ticket created in va.gov-team repo

Testing done

New code is covered by unit tests
Describe what the old behavior was prior to the change
Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing
If this work is behind a flipper:
- Tests need to be written for both the flipper on and flipper off scenarios. Docs.
- What is the testing plan for rolling out the feature?

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas)

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?

TaiWilkin

Looks like some tests are failing, and one question

lib/claim_documents/monitor.rb

github-actions · 2024-12-03T21:48:17Z

	1 Warning
⚠️	This PR changes `292` 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 .github/CODEOWNERS (+3/-0) app/controllers/v0/claim_documents_controller.rb (+47/-10) app/models/persistent_attachment.rb (+2/-0) config/features.yml (+4/-0) lib/claim_documents/monitor.rb (+36/-0) spec/lib/claim_documents/monitor_spec.rb (+41/-0) spec/requests/swagger_spec.rb (+20/-18) spec/requests/v0/claim_documents_spec.rb (+62/-49) 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

wayne-weibel

a few more changes and some questions

wayne-weibel · 2024-12-18T20:08:01Z

app/controllers/v0/claim_documents_controller.rb


-      raise Common::Exceptions::ValidationErrors, attachment unless attachment.valid?
+      if Flipper.enabled?(:document_upload_validation_enabled) && !stamped_pdf_valid? &&
+         %w[21P-527EZ 21P-530 21P-530V2].include?(form_id)


the form_id check should come first in the conditional list, so if that fails the later ones will not be checked. will prevent an unneeded stamped_pdf_valid? call

wayne-weibel · 2024-12-18T20:09:19Z

app/controllers/v0/claim_documents_controller.rb

+      unless allowed_types.include?(extension)
+        raise Common::Exceptions::UnprocessableEntity.new(
+          detail: I18n.t('errors.messages.extension_allowlist_error', extension:, allowed_types:),
+          source: 'PersistentAttachment.unlock_file'


this need to be updated?
also, add a separate check here for an empty file?

Yes, I found from running our specs that trying to convert txt files into PDF caused issues within pdftk, hence the extra enforcement of file type here. i'll add a check for empty file here now

app/controllers/v0/claim_documents_controller.rb

wayne-weibel · 2024-12-18T20:14:32Z

lib/claim_documents/monitor.rb

+  #
+  class Monitor < ::Logging::Monitor
+    # statsd key for document uploads
+    DOCUMENT_STATS_KEY = 'api.document_upload'


api.claim_documents instead?

@wayne-weibel sure, i can rename it. But we'll also need to remember to update the key name on the widget @balexandr created in Datadog as well.

lib/claim_documents/monitor.rb

wayne-weibel · 2024-12-18T20:26:18Z

spec/controllers/v0/claim_documents_controller_spec.rb

the unit tests do not hit what is intended to be tested

$ rspec spec/controllers/v0/claim_documents_controller_spec.rb spec/requests/v0/claim_documents_spec.rb
browse to local file for vets-api coverage path-to/vets-api/coverage/index.html
search for claim_documents_controller

removing in favor of spec/requests/v0/claim_documents_spec.rb

spec/lib/claim_documents/monitor_spec.rb

wayne-weibel · 2024-12-18T20:53:58Z

spec/requests/v0/claim_documents_spec.rb

should this be a separate spec? it seems redundant to spec/controllers/v0/claim_documents_controller_spec.rb

@wayne-weibel that could be why spec/controllers/v0/claim_documents_controller_spec.rb didn't exist in the first place. I can remove that spec file then, to reduce redundancy.

* Validate document attachments at time of upload * Added flipper for extra validation steps --------- Co-authored-by: Wayne Weibel <wayne.weibel@adhocteam.us>

danlim715 added NOT_YET Don't merge this PR w/o the authors permission pension-benefits Used for PRs that impact Pensions labels Nov 20, 2024

github-actions bot added codeowners-addition-failure lint-failure test-failure labels Nov 20, 2024

va-vfs-bot temporarily deployed to zsf/validate-evidence-at-upload/main/main November 20, 2024 17:30 Inactive

danlim715 force-pushed the zsf/validate-evidence-at-upload branch from 4fa46a2 to 207c8df Compare November 22, 2024 01:41

github-actions bot removed the lint-failure label Nov 22, 2024

va-vfs-bot temporarily deployed to zsf/validate-evidence-at-upload/main/main November 25, 2024 14:37 Inactive

danlim715 removed the NOT_YET Don't merge this PR w/o the authors permission label Dec 2, 2024

danlim715 force-pushed the zsf/validate-evidence-at-upload branch 2 times, most recently from 90b62ce to f6e2287 Compare December 2, 2024 15:46

va-vfs-bot temporarily deployed to zsf/validate-evidence-at-upload/main/main December 2, 2024 15:46 Inactive

danlim715 force-pushed the zsf/validate-evidence-at-upload branch 2 times, most recently from afbc386 to ecd15ee Compare December 2, 2024 15:49

github-actions bot removed the codeowners-addition-failure label Dec 2, 2024

va-vfs-bot temporarily deployed to zsf/validate-evidence-at-upload/main/main December 2, 2024 16:03 Inactive

danlim715 force-pushed the zsf/validate-evidence-at-upload branch from ecd15ee to bc56c8b Compare December 2, 2024 16:06

va-vfs-bot temporarily deployed to zsf/validate-evidence-at-upload/main/main December 2, 2024 16:21 Inactive

TaiWilkin requested changes Dec 2, 2024

View reviewed changes

lib/claim_documents/monitor.rb Outdated Show resolved Hide resolved

github-actions bot added the codeowners-addition-failure label Dec 3, 2024

danlim715 force-pushed the zsf/validate-evidence-at-upload branch from 3af220b to b09b70c Compare December 3, 2024 21:51

github-actions bot removed the codeowners-addition-failure label Dec 3, 2024

danlim715 requested a review from TaiWilkin December 3, 2024 22:00

va-vfs-bot temporarily deployed to zsf/validate-evidence-at-upload/main/main December 3, 2024 22:12 Inactive

danlim715 force-pushed the zsf/validate-evidence-at-upload branch from b09b70c to 39fff39 Compare December 4, 2024 16:36

github-actions bot added the lint-failure label Dec 4, 2024

va-vfs-bot temporarily deployed to zsf/validate-evidence-at-upload/main/main December 4, 2024 17:05 Inactive

github-actions bot removed the lint-failure label Dec 6, 2024

danlim715 enabled auto-merge (squash) December 17, 2024 22:57

wayne-weibel requested changes Dec 18, 2024

View reviewed changes

github-actions bot added the ready-for-backend-review label Dec 18, 2024

va-vfs-bot temporarily deployed to zsf/validate-evidence-at-upload/main/main December 18, 2024 23:03 Inactive

github-actions bot added test-failure and removed test-passing ready-for-backend-review labels Dec 18, 2024

danlim715 force-pushed the zsf/validate-evidence-at-upload branch from 2a528d9 to 0319295 Compare December 19, 2024 16:53

github-actions bot added test-passing ready-for-backend-review and removed test-failure labels Dec 19, 2024

danlim715 and others added 9 commits December 19, 2024 11:15

Validate document attachments at time of upload

2961be4

Added flipper for extra validation steps

d2f1980

Added specs

0e3af31

updated CODEOWNERS

9ac466d

Added file extension type restrictions and updated specs

484bd30

lint

cb2738e

Fixed claim_documents_spec.rb

0501e9f

Removed ZSF monitoring and updated specs

6a6df0d

remove redundant specs

243dbba

danlim715 force-pushed the zsf/validate-evidence-at-upload branch from 0319295 to 243dbba Compare December 19, 2024 17:16

danlim715 requested a review from wayne-weibel December 19, 2024 17:51

wayne-weibel approved these changes Dec 19, 2024

View reviewed changes

TaiWilkin approved these changes Dec 19, 2024

View reviewed changes

ryan-mcneil approved these changes Dec 19, 2024

View reviewed changes

github-actions bot removed the ready-for-backend-review label Dec 19, 2024

danlim715 merged commit ebf58a6 into master Dec 19, 2024
25 of 27 checks passed

danlim715 deleted the zsf/validate-evidence-at-upload branch December 19, 2024 22:20

danlim715 mentioned this pull request Jan 15, 2025

updated expected form id for burial applications #20288

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate document attachments at time of upload #19532

Validate document attachments at time of upload #19532

danlim715 commented Nov 20, 2024

TaiWilkin left a comment

github-actions bot commented Dec 3, 2024 •

edited

Loading

Files

wayne-weibel left a comment

wayne-weibel Dec 18, 2024

wayne-weibel Dec 18, 2024

danlim715 Dec 18, 2024

wayne-weibel Dec 18, 2024

danlim715 Dec 18, 2024

wayne-weibel Dec 18, 2024

danlim715 Dec 18, 2024

wayne-weibel Dec 18, 2024

danlim715 Dec 18, 2024

Validate document attachments at time of upload #19532

Validate document attachments at time of upload #19532

Conversation

danlim715 commented Nov 20, 2024

Summary

Related issue(s)

Testing done

Screenshots

What areas of the site does it impact?

Acceptance criteria

Requested Feedback

TaiWilkin left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2024 • edited Loading

Files

wayne-weibel 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2024 •

edited

Loading