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

Release draft #58

Merged
merged 12 commits into from
Sep 18, 2023
Merged

Release draft #58

merged 12 commits into from
Sep 18, 2023

Conversation

elsaperelli
Copy link
Contributor

Summary

This PR adds Release functionality to the Authoring Repository. The user can now "Release" a draft artifact to the central Measure Repository.

New behavior

The user can now "Release" a draft artifact to the central Measure Repository from the Authoring Repository. When a draft artifact is released, the artifact status changes from draft to active. The spec specifies the following for the Release operation: "To be released, an artifact is required to have a version specified." Therefore, when the user releases a draft, they are prompted to input a version. The draft cannot be released without a version specified. When the user adds a version and confirms the release, the active artifact will now appear in the Measure Repository and the draft artifact will be deleted from the Authoring Repository.

Code changes

  • ReleaseModal.tsx - Modal when the user clicks "Release" to confirm release of the draft artifact and also input for a version.
  • pages/authoring/[resourceType]/[id].tsx - Add modal
  • app/package.json - add luxon
  • service/package.json - add cors
  • app.ts - expose headers using cors

Testing guidance

  • npm run check:all
  • npm run start:all
  • Navigate to the Authoring tab. Make sure you have a couple draft Measures and Libraries.
  • Navigate to one of the draft artifacts. Click "Release".
  • Add a version and confirm release.
  • Check the following things:
    • The draft artifact is no longer in the Authoring repository
    • The resourceType count in the Authoring repository is decremented by one
    • The active artifact is now in the Measure repository
    • The resourceType count in the Measure repository is incremented by one

A few notes/questions for the reviewer:

  • One of the comments/questions Lauren left when she worked on this PR was the following: "TODO/question: -> should we check service for whether artifact already exists in some way for PUT update?" I don't think we need this since the artifact IDs should theoretically all be the same? Would love some feedback on that though.
  • Bree added some awesome UI for the modal when she worked on this, but it wasn't fully functional quite yet and in an effort to get this functionality up before the connectathon, I put her stuff on a separate branch (release_draft_UI) and just did some simple UI.
  • I added some descriptions of what the release does in the Modal, but should I add descriptions to the README.md and/or the homepage of the app?

@sarahmcdougall sarahmcdougall self-requested a review September 12, 2023 13:22
@sarahmcdougall sarahmcdougall self-assigned this Sep 12, 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.

Good work! Most of my feedback is related to future tasking and clarifying the spec.
In addition to in-line comments, it might be worth updating the "edit draft resource" tooltip to say something along the lines of "edit/release" since we can't do a release without being on the edit page.

app/src/components/ReleaseModal.tsx Outdated Show resolved Hide resolved
app/src/components/ReleaseModal.tsx Outdated Show resolved Hide resolved
app/src/components/ReleaseModal.tsx Outdated Show resolved Hide resolved
onClick={() => setIsModalOpen(true)}
//TODO/question: disabled={} any disable needed? any necessary fields?
>
Release
Copy link
Contributor

Choose a reason for hiding this comment

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

This falls out of scope of "release" but interested in your thoughts on this - should we apply the updates to the draft measure automatically instead of having an "update" button? There's some pros/cons of both approaches, but it might be nice to automatically update the artifact content similar to how we automatically re-run calculation in fqm-testify whenever changes are made.

As an example, I made an update to my draft artifact (removed the description) without clicking the "update" button, and then released the artifact. The resulting active artifact had the old description, which could cause issues if someone were to accidentally forget to click the "update" button in practice. If we want to reduce burden on the user, it would be nice to automatically update the artifact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I also noticed this! I think we should make a task for this. However, in the meantime, do you think it would be useful to at least have some sort of indication that changes have not been saved? So the user knows to update before releasing? Or should we just wait to do this task?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can hold off on this for now - I feel like this is a rare case and in general I feel like a user is not likely to make updates and then release directly afterward

app/src/components/ReleaseModal.tsx Outdated Show resolved Hide resolved
body: JSON.stringify(resource)
});

const location = res.headers.get('Location')?.substring(5); // remove 4_0_1 (version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way that we could generalize this in some way? For our backend service, the location will be 4_0_1/<resource type>/<id>. At future Connectathons we may want to use a public sandbox endpoint instead of localhost, and so I don't think we are guaranteed to have 4_0_1 included in the url. Maybe we could conditionally remove the base version if a base version is present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully the approach I did makes sense for now- we can update in the future when we start using other endpoints.

});

// direct user to published artifact detail page
router.push(location);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could address this in a follow-up task, but this app is getting pretty slow due to the redirects.
Screenshot below is the logs that I see on the backend. Looks like the data requirements are being calculated automatically, which we expect, but seems like dependent libraries are being found multiple times for the same libraries?
Screenshot 2023-09-14 at 10 45 22 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this needs to be looked at. I think this should be done in a separate task since it looks like this is happening when the user just navigates to an artifact on the publishable measure repository, so it is a bit out of scope for this task.

app/src/components/ReleaseModal.tsx Outdated Show resolved Hide resolved
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.

I'm running into the following runtime error when I release an artifact:
Screenshot 2023-09-18 at 11 39 50 AM

Looks like the actual release is still successful. I think the issue may be related to the base version handling? Wanted to confirm that you also receive this error before digging deeper.

@sarahmcdougall sarahmcdougall merged commit 8c79302 into main Sep 18, 2023
2 checks passed
@sarahmcdougall sarahmcdougall deleted the release_draft branch September 18, 2023 20:00
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.

4 participants