-
Notifications
You must be signed in to change notification settings - Fork 55
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
add notes and gender to patient bulk upload #6778
add notes and gender to patient bulk upload #6778
Conversation
@@ -163,6 +164,56 @@ | |||
this.notes = notes; | |||
} | |||
|
|||
@Builder |
Check notice
Code scanning / CodeQL
Use of default toString()
7f3176c
to
38a4807
Compare
null, // preferredLanguage | ||
null // testResultDeliveryPreference | ||
); | ||
Person.builder() |
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 was super annoying to deal with the multi-param constructor off the person object so I added a new one with the Builder annotation to make it more manageable. Do recognize that we're adding an entirely new constructor to enable this though, so if folks have a better way to work with this large object lmk.
.middleName(extractedData.getMiddleName().getValue()) | ||
.lastName(extractedData.getLastName().getValue()) | ||
.suffix(extractedData.getSuffix().getValue()) | ||
.notes(extractedData.getNotes().getValue()) |
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.
Right now since we're not doing any string interpolation this column can either get filled with null
(if the columns aren't specified) or the empty string (if the column is specified but there isn't any value for that row.) I left it this way since we're doing the same thing in other places.
lmk if anyone prefers null or ""
explicitly though I can go back and clean it up here / in other settings
parseYesNoUnk(extractedData.getResidentCongregateSetting().getValue()), | ||
parseYesNoUnk(extractedData.getEmployedInHealthcare().getValue()), | ||
null, // preferredLanguage | ||
null // testResultDeliveryPreference |
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.
Removed these explicit nulls by not specifying them in the builder, but lmk if we want to add them back in.
backend/src/main/java/gov/cdc/usds/simplereport/service/PatientBulkUploadServiceAsync.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/gov/cdc/usds/simplereport/validators/CsvValidatorUtils.java
Outdated
Show resolved
Hide resolved
fb369e0
to
7c2feb5
Compare
Kudos, SonarCloud Quality Gate passed! |
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.
Nice work! 👍
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.
Thank you for your work on this!
BACKEND PULL REQUEST
Related Issue
Changes Proposed
Testing
Happy Path
Example csv below. Note the
Screen.Recording.2023-10-23.at.11.27.48.AM.mov
Bad string value for gender identity
Upload without extra columns still works
Screen.Recording.2023-10-23.at.11.51.37.AM.mov