Use same endpoint for download results as the results list view #8110
Conversation
|
Ready for review but noting that the Chromatic CI checks apparently detected some difference with dropdown icons where it is saying that Main (seems like some visual bug) PR branch (seems normal as expected) |
|
FYI investigating an error Elisa found that so far only appears on dev6 instead of locally |
|
Tested a variety of different filter configs in dev6 and everything LGTM. All of the new tests are also passing for me locally. Thanks for your great work on this Mike! I agree with you and Merethe that it makes much more sense to be querying these results from the backend. One thing of note that is outside the scope of this ticket: |
mpbrown
force-pushed
the
mike/7948-results-csv-download-query-refactor
branch
from
ed83bed
to
920b24f
Compare
|
@mpbrown Something I'm noticing about these changes is that in the downloaded file if there is a date value in the "Symptom onset" column, it is showing up as "Invalid date". I noticed in other envs we are able to display the correct date if there is a date in that field. Let me know if you can repro. |
Good catch! I'll look into how it's parsing the date |
|
Setting this back to |
|
@bobbywells52 @emyl3 ready for re-review! |
| return new ResolvedSurveyData(result.getSurveyData()); | ||
| } | ||
|
|
||
| @SchemaMapping(typeName = "Result", field = "patientLink") |
There was a problem hiding this comment.
Do we need the patientLink field? I don't think we are using it on the results page or mapping it to the CSV when we download the results 🤔
There was a problem hiding this comment.
I'm pretty sure there was some error it ran into with the tests, but I can't remember what it specifically was so maybe that error was made obsolete by other changes, gonna test out removing that patient link then. patientLink is on the TestResult type which might be on the path to being deprecated in favor of Result now
There was a problem hiding this comment.
For TestResultTest, this line expects patientLink to be there on the result. This old unit test might be left over from when we used to need the patient link on the result but maybe we no longer do? Do you think we should edit/remove the unit test or add patient link for now?
There was a problem hiding this comment.
Ahhhh oops sorry for missing that! Thank you for looking into that again 😓 I think maybe for now we can put the patientLink field back so the tests could pass and this bug fix can be merged in. Thank you for your patience with me on this 😅 🙏
But if you think it'll be helpful, we can make a tech debt ticket to clean this up. That field doesn't seem to be used (based on my rough reading of the codebase) but probably best to dig more thoroughly.
There was a problem hiding this comment.
Sounds good! I'll create a new ticket to address some of that tech debt around TestResult vs Result overall
mpbrown
force-pushed
the
mike/7948-results-csv-download-query-refactor
branch
from
5b89bcc
to
eca5da8
Compare
frontend/src/app/testResults/viewResults/DownloadResultsCsvModal.tsx
Outdated
Show resolved
Hide resolved
|
BACKEND AND FRONTEND PULL REQUEST
Related Issue
Changes Proposed
GetResultsForDownloadquery which hits the same endpoint at the main results list view. This makes the results download consistent with the results list by returning the data based onResultsrather thanTestEventsas the CSV download was previously doing.Additional Information
TestEvents, but rather throughResultsdirectly, this simplifies the logic for the CSV data parsing so we don't have to worry about filtering out any feature disabled diseases from TestEvent data.Testing