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
Lookup dataverse by alias or ID when downloading guestbook responses via API #10020
Conversation
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.
@bencomp thanks for the PR! 🎉 I left a comment about WrappedResponse.
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.
Asking about tests.
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
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.
Do you feel like writing some tests? If so, I'd suggest adding a method for downloading guestbook responses to UtilIT.java (I couldn't find one) and then calling it from DataversesIT.java.
Working on it. |
I couldn't test locally – maybe because I have en_GB as locale – and the Maven integration test job doesn't run the new test either. Does that require something else to happen, @pdurbin? |
@bencomp thanks for adding tests! To run tests locally, I recommend running Dataverse in Docker using https://guides.dataverse.org/en/6.0/developers/dev-environment.html#quickstart and then running just the tests in that class with Jenkins is failing but not due to your code. I just kicked off a manual run at https://github.com/gdcc/api-test-runner/actions/runs/6554508063 to get a second opinion. |
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.
This should help resolve the 403.
I just looked over https://guides.dataverse.org/en/6.0/api/native-api.html#retrieve-guestbook-responses-for-a-dataverse-collection and it seems fine. It already claims to support a collection alias, which is what @bencomp is enabling in this PR. |
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
Well, now it works on my machine 🎉 |
Jenkins is still broken right now but I'm getting a second opinion about if API tests are passing here: https://github.com/gdcc/api-test-runner/actions/runs/6561459544 Should take about 20 minutes. |
Ok, it passed. @scolapasta can we put this on https://github.com/orgs/IQSS/projects/2 ? |
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.
Tests are passing on GitHub Actions and the docs at https://guides.dataverse.org/en/6.0/api/native-api.html#retrieve-guestbook-responses-for-a-dataverse-collection look good. They already mention alias even though it's newly supported in this PR.
Thanks, @bencomp! 🎉
What this PR does / why we need it:
Replaces a dataverse by-alias lookup with
findDataverseOrDie
to allow looking up a dataverse by both its alias and its ID, as the documentation for 5.12.1 mentions. The same section for the latest version says the same.Which issue(s) this PR closes:
Special notes for your reviewer: I would like this PR to count towards my Hacktoberfest goal, so if this is useful, please add the hacktoberfest-accepted label or an approving review.
My editor VSCodium with Java extensions made me handle the
WrappedResponse
thatfindDataverseOrDie
may throw. Throwing aWebApplicationException
is appropriate for the context, according to its Javadoc description.Suggestions on how to test this: check that the commands in the issue work after applying the change.
Does this PR introduce a user interface change? If mockups are available, please link/include them here: no
Is there a release notes update needed for this change?:
I suppose this may fix a bug in the API, but I don't consider this a big change otherwise.
Additional documentation: