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

Publishable write #101

Merged
merged 11 commits into from
Jun 28, 2024
Merged

Publishable write #101

merged 11 commits into from
Jun 28, 2024

Conversation

lmd59
Copy link
Contributor

@lmd59 lmd59 commented Jun 13, 2024

Summary

Implement/ensure publishable artifact repository minimum write capabilities (Publish, Retire, Archive) as defined in the CRMI IG here.

New behavior

  • Checks conditions for create/update to ensure the data matches the requirements for publish/submit and retire/revise capabilities in the spec.
  • Adds delete operation and checks conditions for archive/withdraw capabilities in the spec
  • Adds a transaction bundle script that coerces the resource statuses to active before posting the bundle to the server

Code changes

  • Adds an AUTHORING environment variable to toggle between an authoring and publishable artifact repository
  • Updates the README CRUD section
  • Adds the db:postBundle script for a POST transaction bundle upload option, and refactors dbSetup.ts to share functionality so that the db:postBundle script can work similarly to the db:loadBundle script.
    • Does some refactoring of the use of the DetailedEntry type for this
  • Updates capability statement to show delete archive/withdraw capabilities
  • Adds db operation and server endpoint for DELETE operations for both LibraryService and MeasureService
  • Updates BaseService to enforce spec requirements on transaction bundle entries
  • Adds checkFieldsFor* check functions to inputUtils to enforce spec requirements
    • Question: The spec states for retire "The artifact must be in active status and update is only allowed to change the status to retired and update the date (and other metadata appropriate to indicate retired status)." Is there any other metadata we should allow to update for the retire functionality?
    • Adds and updates tests for BaseService (transaction bundle handling), LibraryService, and MeasureService

Testing guidance

  • npm run check
  • Set the AUTHORING environment variable to false in your local .env file to test publishable-limited capabilities.
  • npm run start:service
  • Run db:postBundle, example npm run db:postBundle {bundle path}
    • Default assumes that the server is running on http://localhost:3000/4_0_1, or you can specify a server path as an additional argument on the end
  • Test out the capabilities and limitations listed in the README for the publishable repository server (and authoring if it makes sense) using Insomnia
    • Can see added jest tests for more specific examples of things to test

@hossenlopp hossenlopp self-requested a review June 17, 2024 13:17
@hossenlopp hossenlopp self-assigned this Jun 17, 2024
@elsaperelli elsaperelli self-requested a review June 19, 2024 15:44
@elsaperelli elsaperelli self-assigned this Jun 19, 2024
Copy link
Contributor

@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.

I still need to test the CRUD operations, but I am finding a potential issue with the postBundle functionality. When I do npm run db:postBundle {may-connectathon-2024 bundle directory}, it seems to upload significantly less Libraries than when I use our old ./directory-upload script (83 vs. around 400 Libraries). I am pretty sure we want around 400 Libraries because of the whole "child artifacts do not have a lifecycle outside of their parents" specification.

service/README.md Outdated Show resolved Hide resolved
service/README.md Outdated Show resolved Hide resolved
service/src/services/BaseService.ts Outdated Show resolved Hide resolved
service/src/services/BaseService.ts Outdated Show resolved Hide resolved
service/src/util/inputUtils.ts Outdated Show resolved Hide resolved
service/src/services/LibraryService.ts Outdated Show resolved Hide resolved
service/src/services/MeasureService.ts Outdated Show resolved Hide resolved
service/test/services/BaseService.test.ts Show resolved Hide resolved
service/scripts/dbSetup.ts Outdated Show resolved Hide resolved
service/scripts/dbSetup.ts Show resolved Hide resolved
@lmd59 lmd59 removed the request for review from hossenlopp June 25, 2024 13:22
@lmd59
Copy link
Contributor Author

lmd59 commented Jun 25, 2024

https://hl7.org/fhir/uv/crmi/artifact-lifecycle.html#artifact-identity has a great rundown of artifact identity (thanks for sharing it @elsaperelli!)
This is making me think about how the distinctions between publish and retire should be applied (or other create vs update distinctions).

If we do a PUT to [url base]/id and something with that id already exists, then we currently enforce the retire checks. If not, it enforces the publish checks. But what if there's a FHIRHelpers version 1 and a user tries to publish a FHIRHelpers version 2 using PUT. These have different canonical identities because they have different versions, but they have the same id, so the server would end up running the retire checks. Should we allow a user to PUT to publish that version 2? The FHIR spec generally seems to allow PUTs for creates, so it feels like we should support that. But the /id path format implies checking of the id and not the canonical identifier.

Copy link
Contributor

@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.

Some small comments and I noticed that the transaction bundle upload still isn't working correctly- looks like the Measures are not being uploaded because they have draft status. Do we want to coerce them to active like we do in dbSetup?

service/scripts/dbSetup.ts Outdated Show resolved Hide resolved
service/test/services/LibraryService.test.ts Outdated Show resolved Hide resolved
beforeAll(() => {
process.env.AUTHORING = 'false';
createTestResource(
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of these are reused, maybe define them at the top of this file like the others?

@lmd59
Copy link
Contributor Author

lmd59 commented Jun 28, 2024

Because artifacts must conform to the Shareable spec https://hl7.org/fhir/uv/crmi/StructureDefinition-crmi-shareablemeasure.html with url, version, description, and title at cardinality 1..1 for CRMIShareableMeasure and CRMIShareableLibrary...
Should our _elements implementation also be keeping those as required elements?

Copy link
Contributor

@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! Lgtm!

@lmd59 lmd59 merged commit 299f778 into main Jun 28, 2024
2 checks passed
@elsaperelli elsaperelli deleted the publishable-write branch June 28, 2024 16:10
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