-
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
Johanna/6723 add destination disease columns upload table #6857
Johanna/6723 add destination disease columns upload table #6857
Conversation
…ination_disease_columns_upload_table
…ination_disease_columns_upload_table Merged with latest main
…ination_disease_columns_upload_table Merged with latest main
…ination_disease_columns_upload_table Merged with latest liquibase changes
backend/src/main/java/gov/cdc/usds/simplereport/service/TestResultUploadService.java
Fixed
Show fixed
Hide fixed
@Entity | ||
@Slf4j | ||
@AllArgsConstructor | ||
@NoArgsConstructor | ||
@Builder |
Check notice
Code scanning / CodeQL
Use of default toString() Note
@NoArgsConstructor | ||
@AllArgsConstructor | ||
@Entity | ||
@Builder |
Check notice
Code scanning / CodeQL
Use of default toString() Note
Default toString(): SupportedDisease inherits toString() from Object, and so is not suitable for printing.
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.
Changes look good! just one nit about the codesmells that sonar has identified.
try { | ||
processCovidResponse(covidSubmission.get()).ifPresent(uploadSummary::add); | ||
} catch (CsvProcessingException | ExecutionException | InterruptedException e) { | ||
log.error("Error processing csv in bulk result upload", e); | ||
Thread.currentThread().interrupt(); | ||
} | ||
try { | ||
processUniversalResponse(universalSubmission.get()).ifPresent(uploadSummary::add); | ||
} catch (CsvProcessingException | ExecutionException | InterruptedException e) { | ||
log.error("Error processing FHIR in bulk result upload", e); | ||
Thread.currentThread().interrupt(); | ||
} |
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.
These nested try catches are code smells, could you pull these out into their own private methods to resolve that?
Kudos, SonarCloud Quality Gate passed! |
I have addressed the feedback from Boban and Bob. New code is available in |
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.
😄
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.
LGTM! Tested on dev2! RSV uploads look good and no regression with the upload history view
BACKEND PULL REQUEST
Related Issue
Changes Proposed
UploadDiseaseDetails
and corresponding repository.TestResultUploadService
include:saveSubmissionToDb
now receives the destination pipeline and the disease/count breakdown.FHIRBundleRecord
: contains the serialized fhir bundle plus the diseases/count breakdown as metadata.CovidSummisionSummary
: contains the RS response and other information relevant to covid reporting (e.g.: submissionId, org, destination).UniversalSummisionSummary
: contains the RS response and other information relevant to universal reporting (e.g.: submissionId, org, destination).Pipeline
: new type to represent COVID | UNIVERSAL pipeline names.TestResultUpload
now uses the @builder annotation.Testing
Code is available in dev2.