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

Add clone operation for draft artifacts #95

Merged
merged 4 commits into from
May 7, 2024
Merged

Conversation

elsaperelli
Copy link
Contributor

@elsaperelli elsaperelli commented Apr 30, 2024

Summary

This PR implements the $clone operation as described by the Quality Measure IG here. There is currently no OperationDefinition for $clone like there is for $draft and $release in the CRMI IG, so the functionality for this operation is subject to change. For now, this PR implements functionality to clone a draft artifact in the draft-repository (authoring measure repository).

New behavior

There is now a "Clone" button amongst the "Review", "Edit", and "Delete" buttons for draft artifacts in the draft-repository frontend. When the user clicks on the "Clone" button, a confirmation modal appears notifying the user that the action will clone that artifact and any of its children. The act of cloning (for now) means that a draft artifact and its children are given a new ID and their versions are incremented (tried 10 times and errors out if it does not have a unique URL and version combination), but the URL and status: "draft" remains the same. Again, this is subject to change as the definition of clone adapts and is potentially discussed at the Connectathon.

Code changes

  • app/src/components/DeletionConfirmationModal.tsx -> app/src/components/ConfirmationModal.tsx - made the DeletionConfirmationModal reusable for clone confirmation as well.
  • app/src/components/ResourceInfoCard.tsx - made success and error notifications reusable for delete and clone, added trpc cloneMutation, ConfirmationModal for clone, and "clone" button.
  • app/src/pages/authoring/index.tsx - trpc.useContext() is now deprecated so it has to be replaced with useUtils().
  • app/src/server/db/dbOperations.ts - add batchCloneDraft function.
  • app/src/server/trpc/routers/draft.ts - consolidate code into helper function getParentDraftArtifactAndChildren and add cloneParent procedure.
  • app/src/server/trpc/routers/service.ts - reusable modifyResource for draft and clone.
  • app/src/util/draftHelper.ts - move reused code for getting a parent artifact and its children from routers/draft.ts.
  • app/src/util/modifyResourceFields.ts - make modifyResourceToDraft reusable for both modifying to draft and modifying for clone.

Testing guidance

  • npm run check:all
  • cd service
  • npm run db:loadBundle <path to bundle> (recommended to use the one I edited for recursive delete testing.
  • cd ..
  • npm run start:all
  • Try drafting a parent artifact and then try combinations of cloning them and deleting them and make sure that there is no unexpected behavior. Make sure the user cannot directly clone or delete a child artifact.
  • Make sure the formatting for the ResourceCards and their buttons were not messed up with the addition of the "clone" button.

Question for Reviewer

  • I was randomly having a lot of trouble with getting the clone confirmation modal to close even after the clone mutation was successful. This was strange because I had no problem with the delete confirmation modal. I spent a ton of time on this and just decided to add the setIsCloneConfirmationModalOpen(false) and setIsDeleteConfirmationModalOpen(false), but if you comment out both of those lines, you will see that the behavior is unchanged for delete but does not work for clone. I am very curious why this behavior is happening if anyone has any ideas!

@zacharyrobin zacharyrobin self-requested a review April 30, 2024 14:20
@zacharyrobin zacharyrobin self-assigned this Apr 30, 2024
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.

Since we're making some README updates, currently the docker compose example listed in the README is out of date. I would recommend just getting rid of the copied docker example text and just linking to the docker example file instead so they don't both need to be changed for a given update.

@elsaperelli elsaperelli requested a review from lmd59 May 6, 2024 16:13
@zacharyrobin
Copy link
Contributor

Looks great. Went through some of the workflow and appears to function as expected. One thing of note is that the IDs when cloning on the authoring page appears to be the same when all are listed out. The individual ids when you go into edit are unique but the preview may need to be updated so that it displays the correct ID. Screenshot 2024-05-06 at 11 56 57 AM

@zacharyrobin
Copy link
Contributor

zacharyrobin commented May 6, 2024

This is super nitpicky and not a necessary change but may want to reconsider the naming on hover from "Edit Draft Resource" to "Revise Draft Resource" to match the to match the language used in the QMIG.

Screenshot 2024-05-06 at 12 02 12 PM

@zacharyrobin
Copy link
Contributor

changes to the READMEs look great ✅

@elsaperelli
Copy link
Contributor Author

Looks great. Went through some of the workflow and appears to function as expected. One thing of note is that the IDs when cloning on the authoring page appears to be the same when all are listed out. The individual ids when you go into edit are unique but the preview may need to be updated so that it displays the correct ID. Screenshot 2024-05-06 at 11 56 57 AM

I did a little bit more digging into this, and I realized I had not really done a full investigation on id vs. identifier before, but to my understanding (which could be incorrect) is that they are unrelated. For example, the ColorectalCancerScreeningsFHIR bundle in ecqm-content-r4-2021 has a Measure with id ColorectalCancerScreeningsFHIR and identifier system "http://hl7.org/fhir/cqi/ecqm/Measure/Identifier/guid and identifier value "e9142f0e-3fc3-4d85-b29e-33ab87ee39c7". Since we only want to change the id when cloning an artifact (for now), I think we could keep the identifier the same? This is certainly something I will include in the Connectathon questions document, but does that make sense for now? Or do we think that the identifier.value needs a new value?

@elsaperelli elsaperelli merged commit cfe2ba5 into main May 7, 2024
2 checks passed
@elsaperelli elsaperelli deleted the clone-operation branch May 7, 2024 19:09
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