-
Notifications
You must be signed in to change notification settings - Fork 477
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
DANS - 9707/Second fix for #9677 #9706
DANS - 9707/Second fix for #9677 #9706
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.
@qqmyers thanks for the PR. Would you be able to add a test so it stays fixed?
It's probably a matter of calling this...
UtilIT.exportDataset(datasetPersistentId, "ddi", apiToken);
...like we do in DatasetsIT.
@qqmyers if existing tests are already catching this bug why aren't they failing in the develop branch? Why did the regression need to be discovered manually? The latest API test run for develop shows no failures: https://jenkins.dataverse.org/job/IQSS-dataverse-develop/1406/testReport/edu.harvard.iq.dataverse.api/ |
OK - new test now passes. Not sure what is up with the MoveIT test - that doesn't seem related. Perhaps timing related again? Otherwise, I think this is ready for review. |
Should be fixed here: |
@qqmyers thanks for the test! I'm just recording here the output I see from the new test on develop where I expect it to fail (then I'll go run the test on your branch):
|
I merged develop in and ran the test locally. Works great. Here are the variables being tested:
|
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. Thanks, especially for the tests, @qqmyers!
What this PR does / why we need it: In removing extra fields from the main json export, the fix in #9677 also removed them from file details where they need to be. This PR leverages the boolean flag introduced in #9695 to just add the extra fields when called to create the file details json export.
Which issue(s) this PR closes:
Closes #9707
Special notes for your reviewer: discussed in https://iqss.slack.com/archives/C03R1E7T4KA/p1689347480349959
Suggestions on how to test this: Verify that DDI exports in the exports menu have variable level metadata and that the main json export does not.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: