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

return deaccessioned version of dataset in more cases #10191

Merged
merged 18 commits into from Jan 2, 2024

Conversation

jp-tosca
Copy link
Contributor

@jp-tosca jp-tosca commented Dec 14, 2023

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 and getDownloadSize act exactly as before, this check would execute every time includeDeaccessioned was set to true, getDatasetVersionCitation will skip this test when includeDeaccessioned is true and getVersion 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:

  • getVersion {id}/versions/{versionId: Will check file permissions when files when excludeFiles and includeDeaccessioned are true.
  • getVersionFiles {id}/versions/{versionId}/files: Will check file permissions when includeDeaccessioned is true.
  • getVersionFileCounts {id}/versions/{versionId}/files/counts: Will check file permissions when includeDeaccessioned is true.
  • getDownloadSize {identifier}/versions/{versionId}/downloadsize: Will check file permissions when includeDeaccessioned is true.
  • getDatasetVersionCitation {id}/versions/{versionId}/citation: Won't check file permissions.
  • listVersions {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:

Deaccesioned dataset Include Deacessioned Exclude files User can edit Result
N Y Y Y OK
N Y Y N OK
N Y N Y OK
N Y N N OK
N N Y Y OK
N N Y N OK
N N N Y OK
N N N N OK
Y Y N Y OK
Y Y N N ERR
Y Y Y Y OK
Y Y Y N OK
Y N N Y ERR
Y N N N ERR
Y N Y Y ERR
Y N Y N ERR

image

@jp-tosca jp-tosca self-assigned this Dec 14, 2023
@coveralls
Copy link

coveralls commented Dec 15, 2023

Coverage Status

coverage: 20.165%. remained the same
when pulling 43855c3 on 10164-bug-api-datasets-versions-fix
into 5cbb895 on develop.

@jp-tosca jp-tosca marked this pull request as ready for review December 15, 2023 20:19
@jp-tosca jp-tosca removed their assignment Dec 15, 2023
@sekmiller
Copy link
Contributor

@landreev can you hear me now?

Copy link
Member

@pdurbin pdurbin left a 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.

doc/sphinx-guides/source/api/changelog.rst Outdated Show resolved Hide resolved
doc/sphinx-guides/source/api/changelog.rst Outdated Show resolved Hide resolved
src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java Outdated Show resolved Hide resolved
src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java Outdated Show resolved Hide resolved
@jp-tosca
Copy link
Contributor Author

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.

@cmbz cmbz added the Size: 10 A percentage of a sprint. 7 hours. label Dec 15, 2023
@sekmiller sekmiller self-assigned this Dec 18, 2023
Copy link
Member

@qqmyers qqmyers left a 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.

@sekmiller sekmiller removed their assignment Dec 19, 2023
Copy link
Member

@pdurbin pdurbin left a 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?

@pdurbin pdurbin changed the title 10164 bug api datasets versions fix return deaccessioned version of dataset in more cases Dec 19, 2023
Copy link
Member

@qqmyers qqmyers left a 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.

@jp-tosca
Copy link
Contributor Author

Merge conflicts have been solved. 🥳

This comment has been minimized.

@jp-tosca jp-tosca removed their assignment Dec 19, 2023
@sekmiller sekmiller self-assigned this Dec 20, 2023
Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:10164-bug-api-datasets-versions-fix
ghcr.io/gdcc/configbaker:10164-bug-api-datasets-versions-fix

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 10 A percentage of a sprint. 7 hours.
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

API only returns Deaccessioned Dataset Version if user has edit privileges on the version
6 participants