Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip fileno before validating #18497

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

michelpmcdonald
Copy link
Contributor

@michelpmcdonald michelpmcdonald commented Sep 18, 2024

Summary

Consumers are submitting multipart files with leading and trailing whitespace for the veteran's metadata fileNumber.

They should not be doing this, and we do reject those submissions, but it's easy enough for us to strip and keep on truck'in.

Team banana-peels, we own this code

Related issue(s)

API-40166

Testing done

  • New code is covered by unit tests
    Unit test add leading and trailing whitespace to fileNumber and confirm that the whitespace is removed and the submission passes our validation

What areas of the site does it impact?

Benefits Intake PDF upload submissions

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.

@va-vfs-bot va-vfs-bot temporarily deployed to API-40166-trim_fileno_bi/main/main September 18, 2024 15:29 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to API-40166-trim_fileno_bi/main/main September 18, 2024 17:34 Inactive
@@ -52,7 +52,7 @@ def validate_metadata(metadata_input, consumer_id, upload_guid, submission_versi
if rejected.present?
raise VBADocuments::UploadError.new(code: 'DOC102', detail: "Non-string values for keys: #{rejected.join(',')}")
end
if (FILE_NUMBER_REGEX =~ metadata['fileNumber']).nil?
if (FILE_NUMBER_REGEX =~ metadata['fileNumber']&.strip).nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The safe navigation operator (&.) isn't needed here because L48 will raise an exception if there are missing keys, and L53 will raise an exception if the value isn't a string. Therefore metadata['fileNumber'] won't ever be nil if L55 is reached.

@@ -76,6 +76,7 @@ def validate_documents(parts, pdf_validator_options = VBADocuments::DocumentRequ

def perfect_metadata(model, parts, timestamp)
metadata = JSON.parse(parts['metadata'])
metadata['fileNumber'] = metadata['fileNumber']&.strip if metadata.key?('fileNumber')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since execution won't reach perfect_metadata if a fileNumber key is not provided and confirmed valid in the metadata (validate_metadata runs before perfect_metadata), if metadata.key?('fileNumber') and the safe navigation operator (&.) aren't needed and this can be simplified to:

metadata['fileNumber'] = metadata['fileNumber'].strip

@@ -1,7 +1,7 @@
{
"veteranFirstName": "Jane",
"veteranLastName": "Doe",
"fileNumber": "012345678",
"fileNumber": " 012345678 ",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than modifying this file which is intended to be the "model" version of metadata, have you considered modifying the value in the spec file prior to using it instead? Something like this:

# code that sets up the test, including the metadata (using valid_metadata.json)
metadata['fileNumber'] = "  012345678 "
# code that runs the UploadProcessor and confirms after that the fileNumber was sent as "012345678"

@@ -191,6 +191,8 @@
expect(capture_body).to have_key('document')
metadata = JSON.parse(capture_body['metadata'])
expect(metadata['uuid']).to eq(upload.guid)
# leading\trailing whitespace should have been removed
expect(metadata['fileNumber']).to eq(metadata['fileNumber']&.strip)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since having whitespace in the fileNumber is an edge case and the test here is for a happy path, I think that testing the edge case deserves its own it block (and context).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants