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

Transition to use CRMI definitions for Measure and Library #105

Merged
merged 6 commits into from
Aug 14, 2024

Conversation

elsaperelli
Copy link
Contributor

Summary

This PR updates the typing of Libraries and Measures by using (when possible) CRMIShareableMeasure and CRMIShareableLibrary type definitions.

New behavior

The CRMIShareableMeasure and CRMIShareableLibrary definitions only vary slightly from that of fhir4.Measure and fhir4.Library- they are both extended from them but also require version, url, title, status and description.

Code changes

  • A lot of changing fhir4.FhirResource to fhir4.Measure and fhir4.Library to CRMIShareableMeasure and CRMIShareableLibrary.
  • A lot of making sure test fixtures have additional attributes (url, version, title, description)
  • Deleting some no longer relevant unit tests

Testing guidance

  • npm run check:all
  • npm run build:all
  • npm run start:all
  • Make sure the functionality is the same.
  • Go through the code to make sure I didn't miss any places where the new types can be used. Note: I purposely did not change anything related to uploading bundles because I did not want to mess with the existing error handling there. Open to alternative ways to do this though.
  • Make sure that all unit tests are still relevant.

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.

Looks good to me.

Copy link
Contributor

@lmd59 lmd59 left a comment

Choose a reason for hiding this comment

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

Looks good! A couple of small recommendations in line as well as recommend replace fhir4.Library with CRMIShareableLibrary in app/src/server/trpc/routers/service.ts and app/src/pages/[resourceType]/[id].tsx

Also, I don't know if we necessarily need to, but thought I'd double check if this warrants an update to the service README. There's a lot of mention of Measure and Library related support, so do you think it's worth adding a bit about compliance with the CRMI versions of those resources?

service/src/util/inputUtils.ts Outdated Show resolved Hide resolved
app/src/util/types/fhir.ts Show resolved Hide resolved
app/src/util/types/fhir.ts Show resolved Hide resolved
Copy link
Contributor

@lmd59 lmd59 left a comment

Choose a reason for hiding this comment

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

Nice!

@elsaperelli elsaperelli merged commit aa72fd4 into main Aug 14, 2024
2 checks passed
@elsaperelli elsaperelli deleted the crmi-defs branch August 14, 2024 15:01
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