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

Check if dataRequirement resourceType does not have patient search params #289

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

elsaperelli
Copy link
Contributor

Summary

We found an issue when running dataRequirements with CMS68. It will error out with the Task resource on CMS68 since the Task resource type is in the compartment definition, but does not have any search parameters so it is not included in PatientParameters.ts (the file that is a result of running the parseCompartmentDefiniton.js script in the deqm-test-server repo). Now, running dataRequirements on this measure no longer errors out.

New behavior

Now, we only do a .forEach on the indexed content of PatientParameters for a resourceType if that resourceType exists in PatientParameters.

Code changes

  • DataRequirementsHelpers.ts - skips adding an extension to a data requirement whose resourceType is not in PatientParameters.ts.

Testing guidance

  • npm run check
  • npm run test:integration
  • Run data requirements on CMS68.

QUESTION: Should we be adding an extension to dataRequirements whose resourceType is not in PatientParameters.ts? I tried looking into the spec for fhirQueryPattern but wasn't sure...

@github-actions
Copy link

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 86.37% 2369/2743
🟡 Branches
73.68% (+0% 🔼)
2197/2982
🟢 Functions 88.89% 424/477
🟢 Lines 86.7% 2289/2640
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / DataRequirementHelpers.ts
85.03%
80.31% (-0.11% 🔻)
78.57% 85.71%

Test suite run success

449 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from 8f0c58d

@sarahmcdougall sarahmcdougall self-assigned this Oct 26, 2023
Copy link
Contributor

@sarahmcdougall sarahmcdougall left a comment

Choose a reason for hiding this comment

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

Tested with CMS68 on the main branch and on this branch, and behavior is expected. Good work!

In regard to the question as to whether we want to add more handling to the compartment definition, I am in favor of the current approach in this PR. The Task resource in particular describes an activity and often exists in parallel with clinical resources. It references who should perform the task, but the task itself does not seem to be concerned with a given patient. Therefore, for resource types like Task, the patient compartment definition will not include these resource types, which is expected. The patient compartment definition includes resource types where the subject is a patient, which is not the case here.

From my understanding, we use the FHIR Query Pattern for specifying the patient context, so we should not be making an extension for resource types that do not appear in the patient compartment definition.

@elsaperelli
Copy link
Contributor Author

patient compartment definition

This all made sense to me until I looked at the compartment definiton for a patient...it looks like Task is included? It was in the compartmentdefinition-patient.json in deqm-test-server, but didn't have any params so it was not added to PatientParameters.ts. But now I am confused because https://build.fhir.org/compartmentdefinition-patient.json.html shows Task not only included in the definition but it also has two params, "patient" and "focus". This leads me to believe that this PR's fix is actually not correct, and that the correct solution is parsing this seemingly new patient compartment definition? Maybe we can also ask @hossenlopp about this.

@elsaperelli
Copy link
Contributor Author

patient compartment definition

This all made sense to me until I looked at the compartment definiton for a patient...it looks like Task is included? It was in the compartmentdefinition-patient.json in deqm-test-server, but didn't have any params so it was not added to PatientParameters.ts. But now I am confused because https://build.fhir.org/compartmentdefinition-patient.json.html shows Task not only included in the definition but it also has two params, "patient" and "focus". This leads me to believe that this PR's fix is actually not correct, and that the correct solution is parsing this seemingly new patient compartment definition? Maybe we can also ask @hossenlopp about this.

Update: It seems like Task was not in the patient compartment definition in R4 (hence why it wasn't in deqm-test-server or PatientParameters.ts) but it is in R5...so perhaps an update to the compartment definition and parsing needs to be done.

@sarahmcdougall
Copy link
Contributor

Looks like our version of the compartment definition in deqm-test-server is from 2019, so it makes sense that recent versions have additional resources. Not sure if we want to take the jump to R5 yet, but I did notice a version from 2022 that we might be able to use to get us up to date? https://hl7.org/fhir/R4B/compartmentdefinition-patient.json.html

Copy link
Contributor

@hossenlopp hossenlopp left a comment

Choose a reason for hiding this comment

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

This fix will do for now. I'm going to just respond to the existing discussions here:

We should stick with using the 4.0.1 definition which does not mention Task. The R4B mentions Task but doesn't have any fields for it to reference to the patient. This will just be something to update in the future for R5.

@elsaperelli elsaperelli merged commit 40b26fb into master Oct 31, 2023
5 checks passed
@elsaperelli elsaperelli deleted the resource-crash-fix branch October 31, 2023 18:08
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