Skip to content
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

By patient #48

Merged
merged 6 commits into from
Aug 6, 2024
Merged

By patient #48

merged 6 commits into from
Aug 6, 2024

Conversation

lmd59
Copy link
Contributor

@lmd59 lmd59 commented Jul 30, 2024

Summary

Support option for exporting files by patient (rather than by type)

New behavior

POST or GET requests sent to either the Patient or Group (but not System) $export endpoint may include a parameter _bySubject with value Patient which causes an export kickoff that will create files with resources grouped by patient. Each file is named based on the patient id, and the resulting stored bulk export status has a byPatient flag set to true. This may be used in combination with the patient parameter to create files for specified patients.

Code changes

  • Update readme
  • Update exportWorker to pass bySubject option through job data
  • Update export service to handle _bySubject validation and pass it to the job and bulk export request.
  • Update exportToNDJSON to create byPatient ndjson files with subject headers and to take in job data as options rather than individual parameters
  • Update mongo controller to create bulk export request with byPatient flag
  • Update tests for new functionality as well as fix some testing issues with not clearing tmp files in the exportToNDJson test file.
  • Update: Update bulkstatus service to add appropriate inputDetails to the import manifest for the kickoff import endpoint

Testing guidance

  • npm run check
  • npm run start
  • Test the _bySubject=Patient parameter with the typical kickoff/status/file-fetch $export workflow. Try some other parameter combinations. Attached Insomnia package has a couple of requests as a jumping off point.
    _bySubject-Insomnia.json

Update: Also test kickoff import as described here: #47 . Test with deqm-test-server branch here: projecttacoma/deqm-test-server#156

Note: There are a couple of TODO questions left that aren't really to do with this task but are potentially small things we might want to spruce up in other parts of the code. Interested in opinions on whether we want to address those things as well either in this PR or maybe a future general code cleanup task.

Copy link

github-actions bot commented Jul 30, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.59% (+1.24% 🔼)
573/758
🟡 Branches
63.19% (+2.22% 🔼)
206/326
🟡 Functions
75.63% (+1.49% 🔼)
90/119
🟡 Lines
75.87% (+1.25% 🔼)
566/746
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / bulkstatus.service.js
61.76% (-1.87% 🔻)
59.38% (-3.96% 🔻)
66.67%
61.76% (-1.87% 🔻)

Test suite run success

95 tests passing in 8 suites.

Report generated by 🧪jest coverage report action from 28e5af2

@elsaperelli elsaperelli self-requested a review July 30, 2024 13:21
@elsaperelli elsaperelli self-assigned this Jul 30, 2024
Copy link
Collaborator

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woo great functionality! Just some small comments :)

README.md Show resolved Hide resolved
src/services/export.service.js Show resolved Hide resolved
src/util/mongo.controller.js Show resolved Hide resolved
src/util/mongo.controller.js Outdated Show resolved Hide resolved

const types = request.query._type?.split(',') || parameters._type?.split(',');
const types = request.query._type?.split(',') || parameters._type?.split(','); //TODO, does gatherParams not pull from the query as well? Why is this OR required?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good call, looks like we can remove it and the one below but I would do some quick testing to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this and did some testing across POST/GET for _type and _elements

src/services/export.service.js Outdated Show resolved Hide resolved
test/util/exportToNDJson.test.js Outdated Show resolved Hide resolved
src/services/export.service.js Outdated Show resolved Hide resolved
lmd59 and others added 2 commits July 31, 2024 17:28
Typo and remove unnecessary parameter

Co-authored-by: Elsa Perelli <[email protected]>
Copy link
Collaborator

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@elsaperelli elsaperelli merged commit 2824aae into main Aug 6, 2024
4 checks passed
@elsaperelli elsaperelli deleted the by-patient branch August 6, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants