-
Notifications
You must be signed in to change notification settings - Fork 66
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
Adding zip backup #12445
Adding zip backup #12445
Conversation
@kylesoskin is there a ticket associated with this work that would give me context as to why this work was done? |
@jeremy6d ... sort of. I have added it to the PR description and pasting in this comment as well. department-of-veterans-affairs/va.gov-team#56057 Specifically it is related to an error mentioned in the comments of that ticket. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't block on little things, but I did offer some food for thought. Thanks for the increased context in the ticket. 👍
@@ -226,11 +226,11 @@ def get_notice_of_disagreement_upload(guid:) | |||
end | |||
end | |||
|
|||
def self.file_upload_metadata(user) | |||
def self.file_upload_metadata(user, backup_zip = nil) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only change I might suggest is to make backup_zip
a named parameter, since on the surface it doesn't seem to have a lot of context for a method named file_upload_metadata
but this is by no means a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have considered a named parameter here instead. I often use named parameters, just did not feel right here for some reason, but it can always be changed for sure.
{ | ||
'veteranFirstName' => transliterate_name(user.first_name), | ||
'veteranLastName' => transliterate_name(user.last_name), | ||
'zipCode' => user.postal_code, | ||
'zipCode' => user.postal_code || backup_zip, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible backup_zip
could ever be nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It SHOULD never be, as the veteran is required to have it populated from the frontend as a part of their submission (IE the frontend should reject the request before it makes it to the backend, if it is nil
). HOWEVER even if it is nil
somehow (it shouldnt be), we are no worse off than previously. user.postal_code
takes precedence, and when that is nil
either use the passed in backup_zip (if one is passed in), or yea it just stays nil
. as it would have if this code were not here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IE nil || nil => nil
, so it should be fine.
@@ -18,17 +18,17 @@ def create | |||
req_body_obj = request_body_hash.is_a?(String) ? JSON.parse(request_body_hash) : request_body_hash | |||
form4142 = req_body_obj.delete('form4142') | |||
sc_evidence = req_body_obj.delete('additionalDocuments') | |||
zip_from_frontend = req_body_obj.dig('data', 'attributes', 'veteran', 'address', 'zipCode5') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there ever be a reasonable situation where there would be a nil
zip code in the request body object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same response as above pretty much. "no" there should not be, it is a requirement of the schema and frontend validation would error/not allow submission at a much earlier point that would never get to this code.... BUT even in the event there was, we are no worse off.
Summary
This uses the zip from the frontend payload as a backup if the users MPI zip is not populated (rather than null)
Benefits Team - Decision Reviews
no flipper used for this
Related issue(s)
Testing done
What areas of the site does it impact?
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?