-
Notifications
You must be signed in to change notification settings - Fork 494
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
rename Private URL to Preview URL and other changes #10961
base: develop
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Hey @sekmiller. I haven't seen what you've done but from your comments in this PR, shouldn't this PR close the GitHub issue Some researchers unsure of difference between "Private URL" and "Anonymous Private URL" #8185, instead of the GitHub issue it's set to close now, Reviewers using anonymous private URL might learn dataset author's identity from information about the Dataverse installation or collection #8184? |
Hey @jggautier, the reason that I marked it with 8184 was because that is the one that was in the queue- and it has the references to changing the name to "Preview URL". 8185 has "no status", but I can add it to the "Closes" above. |
This comment has been minimized.
This comment has been minimized.
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.
Overall, looks good. I found one place where I think we can remove some minor code duplication. I also have some doc suggestions.
p.s. Tests are passing. 🎉 https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10961/15/testReport/
@@ -2171,6 +2171,7 @@ public Response getAssignments(@Context ContainerRequestContext crc, @PathParam( | |||
|
|||
@GET | |||
@AuthRequired | |||
@Deprecated(forRemoval = true, since = "2024-10-17") | |||
@Path("{id}/privateUrl") | |||
public Response getPrivateUrlData(@Context ContainerRequestContext crc, @PathParam("id") String idSupplied) { | |||
return response( req -> { |
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.
Untested but you should be able to replace the whole body with this...
return getPreviewUrlData(crc, idSupplied);
... and the same pattern should work for the other two methods below. That way we don't have to maintain code in two places.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Philip Durbin <[email protected]>
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I'm ready to approve this but API tests didn't run. I'm kicking of another attempt manually: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10961/22/ |
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.
API tests are passing: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10961/22/testReport/
Approved.
I didn't run the code myself but it looks good.
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. |
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
Hey @sekmiller: Found a typo: published ot if the URL |
@sekmiller Another observation I had during testing: "Disable General Preview URL" button isn't working as expected when I modify an existing unpublished dataset file. I compared the 2 behaviors in Internal with the PR and Demo with previous code. To test this, open up an existing dataset thats unpublished and go to Preview URL and click on disable general preview url button. Then from dataset page, go back to Preview URL screen. Here is where the issue lies: "Preview URL has not been created". should be displayed instead of showing a URL as it is doing now. Also noticed that the green success disabling message wasn't displayed either: Screen.Recording.2024-11-15.at.12.17.56.PM.mov |
What this PR does / why we need it: Updates the Private URL to Preview URL in Edit Dataset Popup and native api calls. The request was due to confusion over the use of Private URLs. Also hides Collection info when there is an Anonymous Preview
Which issue(s) this PR closes:
Closes Reviewers using anonymous private URL might learn dataset author's identity from information about the Dataverse installation or collection #8184 Reviewers using anonymous private URL might learn dataset author's identity from information about the Dataverse installation or collection
#8184
Closes Some researchers unsure of difference between "Private URL" and "Anonymous Private URL" #8185 Some researches unsure of differences between "Private URL" and "Anonymous Private URL
Closes Anonymous peer Review: Anonymize "breadcrumbs" and "collection" level information for this feature in HDV and install in Production #10950 Anonymous peer Review: Anonymize "breadcrumbs" and "collection" level information for this feature in HDV and install in Production
Special notes for your reviewer: the native api references to privateurl remain and have been marked as deprecated. The classes and service beans, etc. remain with the private url nomenclature.
With respect to 10950 I ran into some jsf weirdness for hiding the breadcrumbs. there may be a better way to handle it.
Suggestions on how to test this:
This is the doc I used to guide the update to the Edit Dataset Popup: https://user-images.githubusercontent.com/18374574/151025261-ed70e998-9341-4013-9e0f-3c1d4d2c83e4.jpg
I did make one change to the popup to show the resulting url on create. Julian signed off on showing the URL in the popup. He is also reviewing the UI/UX for acceptance testing.
Creation of the "general" preview url will always be available to those that have manage Dataset permission permissions via the "Preview URL" option under the Edit Dataset menu. "Anonymous Preview" is only available if the AnonymizedFieldTypeNames setting has been added to the installation. (Use curl -X PUT -d 'author, datasetContact, contributor, depositor, grantNumber, publication' http://localhost:8080/api/admin/settings/:AnonymizedFieldTypeNames to add the setting)
Verify that the newly created preview url works and indicates to the user that they are viewing an unpublished dataset. Also if the "Anonymous Preview" is used that the Anonymized metadata are displayed as "withheld". Also in the anonymous preview you should not be able to determine the Dataverse Collection that hosts the dataset. (That is, the "breadcrumbs" that indicate the Collections in the ownership tree should not be visible, nor should the header showing the immediate owner of the dataset.)
In the native api verify that previewUrl and privateUrl may be used interchangeably.(curl -H "X-Dataverse-key:35f79221-0fc4-433b-956c-cf20f7f13d87" -X POST "http://localhost:8080/api/datasets/:persistentId/privateUrl?anonymizedAccess=true&persistentId=doi:10.5072/FK2/GNJY26")
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Yes. See doc above.
Is there a release notes update needed for this change?:
Included.
Additional documentation:
The app documentation has been updated for this change