VFMP - Added the ability for CST providers to upload supporting docs to existing cases#27798
VFMP - Added the ability for CST providers to upload supporting docs to existing cases#27798ksantiagoBAH merged 9 commits intomasterfrom
Conversation
github-actions
Bot
added
the
missing-service-tag
Generated by 🚫 Danger |
github-actions
Bot
removed
missing-service-tag
There was a problem hiding this comment.
Pull request overview
Adds CHAMPVA/CST support for uploading supporting documents to existing cases by exposing upload-routing metadata on benefits claims, introducing a docs-only resubmission endpoint, and correcting evidence-submission status progression after finalize.
Changes:
- Add
uploadMetadatatov0/benefits_claimsclaim payloads (provider-aware destination + CHAMPVA doc-type options; docs-only finalize metadata gated byform1010d_enhanced_flow_enabled). - Introduce
POST /ivc_champva/v1/forms/docs_only_resubmissionto submit supporting docs to existing CHAMPVA cases (bypasses VES) and mark matchingEvidenceSubmissionrows asSUCCESS. - Persist
EvidenceSubmissionrows for CHAMPVA supporting-document uploads whenclaim_idis provided.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/controllers/v0/benefits_claims_controller.rb | Adds upload routing metadata to claim responses and improves evidence-submission lookup for CHAMPVA UUID claim IDs. |
| app/swagger/swagger/schemas/benefits_claims.rb | Documents the new uploadMetadata shape in Swagger. |
| config/features.yml | Adds form1010d_enhanced_flow_enabled flipper for enhanced docs-only flow gating. |
| config/benefits_claims/claim_status_meta/ivc_champva/default.json | Adjusts CHAMPVA claim status meta UI config (simpleLayout). |
| modules/ivc_champva/config/routes.rb | Adds the docs-only resubmission route. |
| modules/ivc_champva/app/controllers/ivc_champva/v1/uploads_controller.rb | Implements docs-only resubmission flow, bypasses VES for that path, persists evidence submissions, and updates statuses post-finalize. |
| spec/controllers/v0/benefits_claims_controller_spec.rb | Updates evidence-submission query expectations and adds unit coverage for upload metadata building. |
| modules/ivc_champva/spec/requests/ivc_champva/v1/forms/uploads_spec.rb | Adds request coverage for evidence-submission persistence and the docs-only resubmission endpoint/status transition. |
| validate_docs_only_resubmission!(parsed_form_data) | ||
| hydrate_docs_only_resubmission_data!(parsed_form_data) | ||
|
|
||
| form_id = get_form_id |
There was a problem hiding this comment.
In submit_docs_only_resubmission, parsed_form_data['form_number'] ||= '10-10D-EXTENDED' won’t prevent get_form_id from raising because get_form_id reads params[:form_number] (not parsed_form_data). If form_number is omitted from the request body, this will 500 despite the default. Consider deriving form_id from parsed_form_data['form_number'] (or setting params[:form_number] when defaulting) so the default actually works.
| form_id = get_form_id | |
| form_number = parsed_form_data['form_number'] | |
| form_id = FORM_NUMBER_MAP[form_number] | |
| raise ArgumentError, "Unsupported form number: #{form_number}" if form_id.blank? |
There was a problem hiding this comment.
This is worth addressing. The spec masks the bug by always passing form_number. The default assignment on line 104 (parsed_form_data['form_number'] ||= '10-10D-EXTENDED') is dead code because get_form_id reads from params[:form_number], not parsed_form_data. If a client omits form_number, the raise on line 1326 gets caught by the generic rescue => e and returns a 500.
| if doc['confirmation_code'].blank? | ||
| raise ArgumentError, "supporting_docs[#{index}] is missing confirmation_code" | ||
| end |
There was a problem hiding this comment.
validate_docs_only_resubmission! only checks supporting_docs[*].confirmation_code, but later code assumes additional fields exist and are valid: supporting_document_ids will raise if the referenced PersistentAttachments::MilitaryRecords record can’t be found, and mark_docs_only_evidence_submissions_received! relies on supporting_docs[*].name to match EvidenceSubmissions. Consider validating supporting_docs[*].name presence and that each confirmation_code resolves to an existing persistent attachment (returning a 422 with a clear message instead of a 500).
| if doc['confirmation_code'].blank? | |
| raise ArgumentError, "supporting_docs[#{index}] is missing confirmation_code" | |
| end | |
| unless doc.respond_to?(:[]) | |
| raise ArgumentError, "supporting_docs[#{index}] must be an object" | |
| end | |
| if doc['name'].blank? | |
| raise ArgumentError, "supporting_docs[#{index}] is missing name" | |
| end | |
| confirmation_code = doc['confirmation_code'] | |
| if confirmation_code.blank? | |
| raise ArgumentError, "supporting_docs[#{index}] is missing confirmation_code" | |
| end | |
| next if PersistentAttachments::MilitaryRecords.exists?(guid: confirmation_code) | |
| raise ArgumentError, | |
| "supporting_docs[#{index}] confirmation_code could not be resolved to an existing attachment" |
| claim_id = resolve_claim_record_id(params[:claim_id]) | ||
| if claim_id.blank? | ||
| Rails.logger.warn('Skipping CHAMPVA evidence submission persistence: missing/unresolvable claim_id') | ||
| return | ||
| end | ||
|
|
||
| user_account = current_user_account_for_evidence_submission | ||
| if user_account.blank? | ||
| Rails.logger.warn('Skipping CHAMPVA evidence submission persistence: missing user_account') | ||
| return | ||
| end | ||
|
|
||
| file_name = uploaded_file_name(params['file'], attachment) | ||
| if file_name.blank? | ||
| Rails.logger.warn('Skipping CHAMPVA evidence submission persistence: missing file_name') | ||
| return |
There was a problem hiding this comment.
persist_claim_evidence_submission logs warnings when claim_id/user_account/file_name are missing, but claim_id is explicitly optional for legacy upload paths. As written, calls to submit_supporting_documents that don’t include claim_id will emit a warn on every upload, which is likely to create noisy logs. Consider only warning when a claim_id param is present but unresolvable (or downgrade to debug/info for the expected legacy case).
| DOCS_ONLY_RESUBMISSION_SUBMISSION_TYPES.include?(submission_type) | ||
| end | ||
|
|
||
| def docs_only_resubmission_flow_enabled?(parsed_form_data) |
There was a problem hiding this comment.
This controller’s authenticate method rescues Common::Exceptions::Unauthorized and allows the request to continue unauthenticated, which means POST /forms/docs_only_resubmission can be called without a logged-in user. Since this endpoint hydrates data from an existing claim and can update EvidenceSubmission statuses, consider requiring @current_user (or other auth/authorization) as part of docs_only_resubmission_flow_enabled? / submit_docs_only_resubmission so the docs-only finalize path can’t be exercised anonymously when the flipper is enabled broadly.
| def docs_only_resubmission_flow_enabled?(parsed_form_data) | |
| def docs_only_resubmission_flow_enabled?(parsed_form_data) | |
| return false unless @current_user |
rmtolmach
left a comment
rmtolmach
left a comment
There was a problem hiding this comment.
@ksantiagoBAH Can you do the following?
- Resolve the Reek
DuplicatesMethodCallwarnings? There's some room for improvement there. - Refactor so you're not disabling rubocop? I see you're disabling it in 6 places in this PR. We try to avoid that if at all possible.
- Take a look at Copilot's comments and see if any of them are relevant? (I haven't read them, but sometimes they're helpful, sometimes not 😄)
| end | ||
|
|
||
| def hydrate_docs_only_resubmission_data(parsed_form_data) | ||
| source_form = IvcChampvaForm.where(form_uuid: parsed_form_data['claim_id'].to_s).order(updated_at: :desc).first |
There was a problem hiding this comment.
If ownership enforcement isn't occurring elsewhere we might want to add a check to ensure the UUID belongs to the requesting user.
There was a problem hiding this comment.
Totally fair callout. I wanted to add ownership checks too, but IvcChampvaForm doesn’t currently have a reliable user ownership field in this path, so I didn’t want to add a fake/partial check.
I kept this PR scoped to the docs-only flow fix behind the flag, and I’ll open a follow-up to add real ownership enforcement once we lock down the right ownership signal for claim_id -> IvcChampvaForm.
There was a problem hiding this comment.
Makes sense. Worth capturing that as a ticket so it doesn't slip.
| end | ||
|
|
||
| # rubocop:disable Metrics/MethodLength | ||
| def add_evidence_submissions_to_claims(claims, all_evidence_submissions, endpoint) |
There was a problem hiding this comment.
@ksantiagoBAH it looks like you can tighten up this method a little bit to avoid disabling this rubocop. See what I did here:
|
|
||
| module V0 | ||
| # rubocop:disable Metrics/ClassLength | ||
| class BenefitsClaimsController < ApplicationController |
There was a problem hiding this comment.
Has your team considered a plan to refactor this class so it's not so long?
There was a problem hiding this comment.
Yeah, I'm going to refactor these real quick
There was a problem hiding this comment.
Refactored the method level ones, but I will bring this to the team as some tech debt for us to refactor.
nathangthomas
left a comment
nathangthomas
left a comment
There was a problem hiding this comment.
I left some comments. Some you've already addressed or commented on. Getting close!
Here’s a PR description you can paste for the vets-api changes:
Summary
This work is behind a feature toggle (flipper): YES
Primary flipper for docs-only flow:
form1010d_enhanced_flow_enabledAdditional optional behavior flipper used in this flow:
champva_convert_to_pdf_on_upload(non-PDF -> PDF conversion at upload)What changed
This PR adds/updates backend support for CST CHAMPVA supporting-document uploads using a data-driven destination model and fixes file status progression for docs-only resubmissions.
Changes include:
uploadMetadatato claim payloads inV0::BenefitsClaimsController.uploadDestinationKeyformIdacceptedFileTypesdocumentTypeOptionsfinalizeDestinationKey+submissionType(for 10-10D-EXTENDED docs-only finalize path)POST /ivc_champva/v1/forms/docs_only_resubmissionvalidates payload, hydrates required veteran/contact fields from existing CHAMPVA claim record, and submits supporting docs via existing uploader path.form1010d_enhanced_flow_enabled.EvidenceSubmissionrows for the claim are moved from pending statuses (CREATED/QUEUED/PENDING) toSUCCESSso CST can show files in “Files received.”Bug / repro
Prior behavior:
EvidenceSubmissionremainedCREATED, so CST continued showing item under “File submissions in progress” instead of “Files received.”Why this solution
Team ownership
Flipper success criteria
For
form1010d_enhanced_flow_enabled:SUCCESSevidence status after successful finalize.Related issue(s)
department-of-veterans-affairs/vets-website#44126
department-of-veterans-affairs/va.gov-team#139480
Testing done
Automated
bundle exec rspec modules/ivc_champva/spec/requests/ivc_champva/v1/forms/uploads_spec.rb:857bundle exec rubocop --parallel --format github(clean)Manual verification steps
vets-apiandvets-website.submit_supporting_documents).docs_only_resubmission) withsubmission_type=existing.EvidenceSubmissiontransitions toSUCCESSFlipper rollout test plan
form1010d_enhanced_flow_enabledfor lower env/internal users first.Screenshots
(Optional)
What areas of the site does it impact?
Backend/API impact:
v0/benefits_claimsresponse shape (addsuploadMetadatafor provider-aware uploads)ivc_champvauploads controller docs-only flow and evidence status updatesNo direct non-CHAMPVA upload behavior changes intended; default upload destination remains unchanged for non-CHAMPVA providers.
Acceptance criteria
Requested Feedback
Please focus review on:
CREATED/QUEUED/PENDING -> SUCCESS) and matching strategy.