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

10207 bug api datasets versions latest fix #10212

Merged
merged 7 commits into from
Feb 1, 2024

Conversation

jp-tosca
Copy link
Contributor

@jp-tosca jp-tosca commented Jan 5, 2024

What this PR does / why we need it:

It was reported on #10164 that the API to get a dataset version was not working as intended. Part of this was fixed on #10191 but this PR didn't address when the request is made using the parameter ":latest" Some other cases like "latest-published" were addressed because of how the code was structured.

Which issue(s) this PR closes:

Closes #10207

Special notes for your reviewer:

As part of the fix several test cases were added to verify the right behavior of the API, some of them can seem redundant but there are slight differences, for example, if you use ":latest" on a request that includes deaccessioned but there is a Draft and you will get the Draft version and depending if you requested the files you may get them or not. I am happy to get on a call and explain the cases but also I am open to suggestions.

Suggestions on how to test this:

Similar to the testing done on #10164 but besides targeting a specific version replace with :latest

As an example curl -X GET "https://beta.dataverse.org/api/v1/datasets/:persistentId/versions/:latest?persistentId=doi:10.5072/FK2/UBKVYE&includeDeaccessioned=true&excludeFiles=true"

@coveralls
Copy link

coveralls commented Jan 5, 2024

Coverage Status

coverage: 20.147% (+0.004%) from 20.143%
when pulling 3a4cab1 on 10207-bug-api-datasets-versions-latest-fix
into 274c6f0 on develop.

This comment has been minimized.

@sekmiller sekmiller assigned sekmiller and unassigned sekmiller Jan 5, 2024
@cmbz cmbz added the Size: 3 A percentage of a sprint. 2.1 hours. label Jan 8, 2024
Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

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

I suggest we update the GetLatestPublishedDatasetVersionCommand to make the logic clearer and to comment on the derivation of the checkPerms param. Thanks!

This comment has been minimized.

Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Looks good. Passing to ready for QA

@sekmiller sekmiller removed their assignment Jan 10, 2024
@GPortas GPortas added the SPA These changes are required for the Dataverse SPA label Jan 17, 2024
@GPortas GPortas self-assigned this Jan 25, 2024
Copy link
Contributor

@GPortas GPortas 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

Looks good!

I have tested all version identifiers and it works properly.

In particular, the following call using :latest, which was the one problematic, now works as expected:

curl -X GET "http://localhost:8080/api/v1/datasets/:persistentId/versions/:latest?persistentId=doi:10.5072/FK2/EFWQDI&includeDeaccessioned=true&excludeFiles=true"

I wanted to add a refactor suggestion to my PR review, but GitHub doesn't seem to allow adding suggestions when they involve big changes to the file. So I created a PR pointing to your branch: #10267

This comment has been minimized.

…ed-dsv-cmd

#10212 PR refactor proposal for GetLatestPublishedDatasetVersionCommand
Copy link

📦 Pushed preview images as

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

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

@sekmiller sekmiller self-assigned this Jan 31, 2024
@sekmiller sekmiller removed their assignment Jan 31, 2024
@GPortas GPortas self-assigned this Feb 1, 2024
Copy link
Contributor

@GPortas GPortas left a comment

Choose a reason for hiding this comment

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

Thank you for applying the refactor @jp-tosca!

Approving and merging.

@GPortas GPortas merged commit 020ee02 into develop Feb 1, 2024
19 checks passed
@GPortas GPortas deleted the 10207-bug-api-datasets-versions-latest-fix branch February 1, 2024 16:00
@pdurbin pdurbin added this to the 6.2 milestone Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 3 A percentage of a sprint. 2.1 hours. SPA These changes are required for the Dataverse SPA
Projects
None yet
6 participants