-
Notifications
You must be signed in to change notification settings - Fork 4
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
[DUOS-1905][risk=no] Update Researcher Console to use Summary Endpoint #1708
Conversation
Co-authored-by: Gregory Rushton <rushtong@users.noreply.github.com>
Co-authored-by: Gregory Rushton <rushtong@users.noreply.github.com>
Not sure where the bug originates, but if I set the page count to 100 on the researcher console, I get the following error:
|
I'm not sure how to recreate this. I ran into the same bug when there was a bug with the consent backend; this happens in the case that there is a row which is both a draft DAR and the backend does not send a referenceId alongside it. When I create a new draft DAR locally, this doesn't happen to me. Would it be possible that there is a draft DAR in your database which does not have a referenceId? If so, that would cause this error, and I can add some guards to ensure legacy drafts don't break the UI. |
This scenario should be technically impossible since all dars, draft or not, are saved with a reference id. I'll spend some time digging further now that I have some more time. |
|
||
useEffect(() => { | ||
const isCancelable = isCollectionCancelable(dars); | ||
const isRevisable = isCollectionRevisable(dars); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im seeing some things here that i would recommend changing after merging in 1904, but we can handle that then? These variables for cancelEnabled+ were only ever used once, so I moved them to where the render was called. We can address that post merge tho! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me!
@@ -144,7 +114,7 @@ export default function ResearcherActions(props) { | |||
}; | |||
|
|||
const reviseButtonAttributes = { | |||
keyProp: `revise-collection-${collectionId}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you choose to change these from collectionId to uniqueId? Was the collectionId not unique?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there isn't always a collection id. For example, if the dar collection is a draft, it doesn't have a collection id, so it would be just 'undefined' here. Thus, we need to use a referenceId in the draft case. There might be another solution here, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry meant to approve this earlier, i'm working with it already in a diff branch, so it's all good :P
There was a problem hiding this 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. One possible improvement to consider. 👍🏽
const cancelEnabled = collection.actions.includes('Cancel'); | ||
const reviseEnabled = collection.actions.includes('Revise'); | ||
const reviewEnabled = collection.actions.includes('Review'); | ||
const deleteEnabled = collection.actions.includes('Delete'); | ||
const resumeEnabled = collection.actions.includes('Resume'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional suggestion. It would be nice if these string constants were part of an object/enum somewhere useful for all action-related components. Not sure where that would be for now, so definitely not a blocker for this PR.
Closing, changes already incorporated in #1699 |
Addresses
https://broadworkbench.atlassian.net/browse/DUOS-1905
In dev, the summary endpoint does not return reference ids. We need them so that draft dar collections can be resumed. The backend change to add that is here: DataBiosphere/consent#1670
Have you read Terra's Contributing Guide lately? If not, do that first.