96358 Fix for out of order attachment ids - IVC CHAMPVA forms #19647
Conversation
va-vfs-bot
temporarily deployed
to
fix-upload-order-attachment-id-bug-ivc-forms/main/main
va-vfs-bot
temporarily deployed
to
fix-upload-order-attachment-id-bug-ivc-forms/main/main
va-vfs-bot
temporarily deployed
to
fix-upload-order-attachment-id-bug-ivc-forms/main/main
| parsed_form_data['supporting_docs']&.pluck('attachment_id')&.compact.presence || | ||
| parsed_form_data['supporting_docs']&.pluck('claim_id')&.compact.presence || [] | ||
| cached_uploads = [] | ||
| parsed_form_data['supporting_docs']&.each do |d| | ||
| # Get the database record that corresponds to this file upload: | ||
| record = PersistentAttachments::MilitaryRecords.find_by(guid: d['confirmation_code']) | ||
| # Push to our array with some extra information so we can sort by date uploaded: | ||
| cached_uploads.push({ attachment_id: d['attachment_id'], | ||
| created_at: record.created_at, | ||
| file_name: record.file.id }) | ||
| end | ||
|
|
||
| # Sort by date created so we have the file's upload order and | ||
| # reduce down to just the attachment id strings: | ||
| attachment_ids = cached_uploads.sort_by { |h| h[:created_at] }.pluck(:attachment_id)&.compact.presence | ||
|
|
||
| # Return either the attachment IDs or `claim_id`s (in the case of form 10-7959a): | ||
| attachment_ids || parsed_form_data['supporting_docs']&.pluck('claim_id')&.compact.presence || [] |
There was a problem hiding this comment.
The order of the attachment IDs has always mattered, so this is ensuring we have things in a sane order (date created) rather than the previous arbitrary order.
In future we will probably want to look into a wider refactor. A direct mapping of files to doc types rather than relying on the order of this array would be better, but this is a necessary first step to make sure the MVP works.
| allow(PersistentAttachments::MilitaryRecords).to receive(:find_by) | ||
| .and_return(double('Record1', created_at: 1.day.ago, id: 'some_uuid', file: double(id: 'file0'))) |
There was a problem hiding this comment.
Mock added to account for new logic in supporting_document_ids that attempts to read PersistentAttachments::MilitaryRecords.
va-vfs-bot
temporarily deployed
to
fix-upload-order-attachment-id-bug-ivc-forms/main/main
github-actions
bot
added
require-backend-approval
and removed
require-backend-approval
labels
| cached_uploads = [] | ||
| parsed_form_data['supporting_docs']&.each do |d| | ||
| # Get the database record that corresponds to this file upload: | ||
| record = PersistentAttachments::MilitaryRecords.find_by(guid: d['confirmation_code']) |
There was a problem hiding this comment.
OOC what would happen if this query failed? Any possibility of a silent failure? I don't want to hold this PR up but it may be worth a follow-on to investigate and make it more defensive if necessary.
There was a problem hiding this comment.
If the query returns nil on l95, we'll get an error when we try to pull the created_at and file.id on lines 98 & 99. This will bubble back up to the user as a 500, so they'll get an error screen and the submission won't go through.
I can definitely make a follow up to probe this a little more - thanks for looking this over!
There was a problem hiding this comment.
thanks @michaelclement! maybe the follow-up could be to make a quick couple of request specs that confirm an error is returned when the query returns nil or throws.
There was a problem hiding this comment.
Definitely a good idea - I'm going to put a couple more tests into this PR while it's top of mind. Thanks!
There was a problem hiding this comment.
@acrollet Tests added in this commit - please let me know if there's anything else you think I should take a look at!
|
Backend-review-group approval confirmed. |
michaelclement
dismissed stale reviews from rjohnson2011, cloudmagic80, acrollet, and reiting
via
fe2da6a
michaelclement
deleted the
fix-upload-order-attachment-id-bug-ivc-forms
branch
Summary
This PR fixes an issue with how attachment metadata is prepared before being sent to Pega for ingestion. Previously, the document types (
attachment_id, e.g.: 'Birth certificate') were rolled into a list based solely on the order of the front end configuration.These values were then matched up with the list of attachments in the cache on the backend. However, if a user uploaded files in any order other than the order in which they encounter them, the alignment between attachment_id and the file itself would be thrown off. This caused files to be mis-categorized.
By reorganizing the
attachment_idarray to match with the order files were uploaded, metadata gets matched correctly regardless of how the user uploaded their files.Related issue(s)
Testing done
Screenshots
NA
What areas of the site does it impact?
IVC CHAMPVA forms
Acceptance criteria
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?