-
Notifications
You must be signed in to change notification settings - Fork 530
feat: add "isInReviewState" to API responses about dataset versions #12008
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
base: develop
Are you sure you want to change the base?
Conversation
| .add("internalVersionNumber", dsv.getVersion()) | ||
| .add("versionMinorNumber", dsv.getMinorVersionNumber()) | ||
| .add("versionState", dsv.getVersionState().name()) | ||
| .add("isInReviewState", dsv.isInReview()) |
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.
Change to isInReview since this is not a state. Being in review is a lock
stevenwinship
left a comment
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.
Change isInReviewState to isInReview since this is not a state. Being in review is a lock
|
@vera - FWIW: I think we have design questions about this. Being in review is because of a review lock and we already have an API for locks: https://guides.dataverse.org/en/latest/api/native-api.html#dataset-locks . While understanding that it can be convenient to get all useful info in a single call, having multiple ways to get the same info can be confusing. Further, making the output of a GET different from what's allowed in a PUT can also be confusing and make it harder for API users to deal with round trip changes. We certainly have cases where we've allowed extra info in GETs for convenience already, but we're starting to question doing more of that. In this case, I think there's an additional question too: are we starting down a road where we will also have entries for all the other possible locks in the version response? I think the things were discussing are:
|
|
I think it useful to add isInReview to the API response - although we haven't implemented the Submit for Review feature yet in the SPA, we do need to display an 'In Review' label in the View Dataset page, and we already are returning 'In Review' in the search results on the Collection Page. |
|
@ekraffmiller Does it make sense for the localization to be done in the backend? Wouldn't it make more sense to return a fixed value that can be localized by the UI? |
|
Hi @vera, thanks for your PR! We discussed it and think that for maintainability, it would be better to return a list of current locks on the dataset, rather than having a flag for the existence of a particular lock. That way if more locks are added in the future, the API code won't need to be updated. |
This is a good point. We have discussed handling multiple languages in general by adding a requested language in the API header. We we implement that, this data could be returned in the requested language, rather than translated in the frontend. |
Yes, here's an (unmerged) example: 99fcede And here's the (merged) language switcher: |
What this PR does / why we need it:
This PR adds "isInReviewState" to API responses about dataset versions, which is useful information.
Which issue(s) this PR closes:
Not aware of any issue.
Special notes for your reviewer:
/
Suggestions on how to test this:
I've extended the API test:
mvn test -Dtest="DatasetsIT#testDatasetVersionsAPI"Does this PR introduce a user interface change? If mockups are available, please link/include them here:
/
Is there a release notes update needed for this change?:
I've added a short release note.
Additional documentation:
/