-
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 result and condition filters to test results list #6682
Add result and condition filters to test results list #6682
Conversation
data - Adds resolver method to return paginated test result data - Adds a utility `TestResultsListItem` class - this is the object returned by the server - Adds a data resolver to correctly fetch facility data for a test result
…; updates to expected shape of incoming test result in result table UI component
backend/src/test/java/gov/cdc/usds/simplereport/service/ResultServiceTest.java
Fixed
Show fixed
Hide fixed
backend/src/test/java/gov/cdc/usds/simplereport/service/ResultServiceTest.java
Fixed
Show fixed
Hide fixed
backend/src/test/java/gov/cdc/usds/simplereport/service/ResultServiceTest.java
Fixed
Show fixed
Hide fixed
…hings to build (for now)
…L query from Cypress
941c343
to
a101869
Compare
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.
LGTU, paired on the review with @mpbrown, some minor suggestions and we believe we need a follow up pr to no hard code the diseases on the disease filter list
backend/src/main/java/gov/cdc/usds/simplereport/service/ResultService.java
Show resolved
Hide resolved
SupportedDisease supportedDisease = | ||
disease != null ? diseaseService.getDiseaseByName(disease) : null; | ||
|
||
if (facilityId == null) { |
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.
remove if statement and only use getFacilityResults
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 there a clean way to move this null conditional into the service instead of having it in the resolver? This was related to our other comment about combining getOrganizationResults
and getFacilityResults
, although not sure whether the different auth requirements complicates this then
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 think having this conditional in the resolver makes sense for the auth reasons discussed elsewhere.
Fwiw, this pattern (and the associated auth requirements) are lifted one-to-one from the existing TestResultResolver
and TestOrderService
classes, which these changes largely supplant:
Lines 46 to 65 in 5140abc
if (facilityId == null) { | |
return tos.getOrganizationTestEventsResults( | |
patientId, | |
Translators.parseTestResult(result), | |
Translators.parsePersonRole(role, true), | |
startDate, | |
endDate, | |
pageNumber, | |
pageSize); | |
} | |
return tos.getFacilityTestEventsResults( | |
facilityId, | |
patientId, | |
Translators.parseTestResult(result), | |
Translators.parsePersonRole(role, true), | |
startDate, | |
endDate, | |
pageNumber, | |
pageSize); | |
} |
]} | ||
defaultSelect | ||
onChange={setFilterParams("disease")} | ||
/> |
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.
Per story AC, the values here should be loaded from the backend so we dont have to keep adding more values as we add more diseases, doesnt have to block this PR, but it could be added as a followup pr as part of the same 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'm honestly not sure if I'm sold on pulling those values from the backend here to begin with. Adding new diseases to SimpleReport will already involve touchpoints in the frontend code, like our constants.
Also, HIV
and RSV
have already been added to the database but we're not ready to expose those values to the disease filter, so we'd need to turn around and hardcode those values somewhere instead.
Basically I'm leaning towards there being a logical distinction between diseases that SR "knows about"
and diseases that users can submit for
, at least at this time.
Whatchy'all think?
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.
Hmm that's a good point where the frontend already requires some hardcoded reference for the diseases. At our next pairing session I'm curious to get your thoughts on this as it relates to what I'm working on with adding HIV support since it seems like we'll always need some way to distinguish which AOE questions to show
frontend/src/app/testResults/resultsTable/ResultsTable.test.tsx
Outdated
Show resolved
Hide resolved
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! Nice work on this!
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
Kudos, SonarCloud Quality Gate passed! |
@nathancrtr I noticed something interesting with this new functionality... if we correct a result is it now expected that we see the previously corrected test in the results page along with the newly submitted result? To repro:
|
Related Issue
Changes Proposed
condition
andresult
filters for test results listAdditional Information
result
andcondition
vary within a single test event, these filters had to be updated for more granular accessSpecification
s to apply filters from user to database retrievalTesting
Name filter
Screen.Recording.2023-10-05.at.4.22.04.PM.mov
Date filters
Screen.Recording.2023-10-05.at.4.22.20.PM.mov
Condition filter
Screen.Recording.2023-10-05.at.4.20.59.PM.mov
Result filter
Screen.Recording.2023-10-05.at.4.21.18.PM.mov
Role filter
Screen.Recording.2023-10-05.at.4.21.34.PM.mov
Facility filter
Screen.Recording.2023-10-05.at.4.25.58.PM.mov
Multiple filters applied; clearing filters
Screen.Recording.2023-10-05.at.4.22.38.PM.mov