-
Notifications
You must be signed in to change notification settings - Fork 495
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
return deaccessioned version of dataset in more cases #10191
Conversation
@landreev can you hear me now? |
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.
I haven't wrapped my head around the substantive change in this PR but I'm leaving comments about docs and tests, at least.
I would still like to check with @ekraffmiller about the citation API, on dev branch I was able to access to the citation of a deaccessioned dataset when the parameter is provided, but I found a bug(?) that this parameter will also give the citation of a draft. |
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.
A couple minor suggestions for changes. Overall - looks good. Very clear testing instructions.
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.
@jp-tosca can you please resolve merge conflicts?
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 - just the merge issues to fix.
Merge conflicts have been solved. 🥳 |
This comment has been minimized.
This comment has been minimized.
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
What this PR does / why we need it:
API only returns Deaccessioned Dataset Version if user has edit privileges
Which issue(s) this PR closes:
Closes #10164
Special notes for your reviewer:
We agreed to change the parameter name from includeFiles with excludeFiles, this is the default and the lack of the parameter will default to false so files are expected to be included by default if this parameter is not provided.
getVersionFiles
,getVersionFileCounts
andgetDownloadSize
act exactly as before, this check would execute every time includeDeaccessioned was set to true,getDatasetVersionCitation
will skip this test when includeDeaccessioned is true andgetVersion
will check for when includeDeaccessioned is true and excludeFiles is false. This change also replaced includeFiles with excludeFiles which is the default behaviour.This change required to update Datasets.getDatasetVersionOrDie which impacted the following methods:
{id}/versions/{versionId
: Will check file permissions when files when excludeFiles and includeDeaccessioned are true.{id}/versions/{versionId}/files
: Will check file permissions when includeDeaccessioned is true.{id}/versions/{versionId}/files/counts
: Will check file permissions when includeDeaccessioned is true.{identifier}/versions/{versionId}/downloadsize
: Will check file permissions when includeDeaccessioned is true.{id}/versions/{versionId}/citation
: Won't check file permissions.{id}/versions
: replaced includeFiles with excludeFiles which is the default.Suggestions on how to test this:
Have a Dataset with one deaccessioned version, here is the test matrix that I used and a flow chart that i made: