-
Notifications
You must be signed in to change notification settings - Fork 501
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
SPO/7786 Add holdings element to DDI and use PID URIs #7984
SPO/7786 Add holdings element to DDI and use PID URIs #7984
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.
Overall, this looks fine but it does represent a backward-incompatible change since <IDNo agency="DOI">
becomes a URL instead of a DOI. (I can imagine scripts or systems breaking.) @qqmyers can you please add a release note?
I was about to move this to QA when I noticed a test failure at https://jenkins.dataverse.org/blue/organizations/jenkins/IQSS-Dataverse-Develop-PR/detail/PR-7984/3/tests Digging a little more, it seems like this is one of the tests that fails intermittently:
So it should be safe to move to QA. |
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.
The release notes seems to warn well enough that this is a backward incompatible change. The code looks fine.
Passes smoketest holdings element in export verified, harvest between two dataverses of same pr works but when harvest client is 5.5 it fails due to parsing error of doi (IDno) Harvest shows it successful but when you attempt to view the page where the dataset card it is throws an exception (parsing error). |
@kcondon - I reverted the change to the IDNo element so the harvesting-related compatibility issue should be gone. |
What this PR does / why we need it: This PR adds a studydsc/citation/holdings element with a URI attribute whose value is the full PID/DOI URI for the dataset. It no longer updates the studydsc/citation/titlestmt/IDNo element to use the URI form of the DOI as this causes a compatibility issue when older servers harvest from newer ones using the ddi export and because using the URI form does not appear to be required/recommended by the CESSDA validator. (which does check the holdings element).
Which issue(s) this PR closes:
Closes #7786
Special notes for your reviewer: This seems like an improvement that addresses #7786 but I think further work to understand best-practices for what should be in these elements might be worthwhile. Examples I've seen use the local URL (i.e. what the DOI resolves to) in the holdings element, and do not use the dataset DOI in the docdscr/citation/titlestmt/IDNo (not changed in this PR) as Dataverse does. Further, per the issue discussion, it seems like holdings could reference the individual files that are part of the dataset. In any case, I think this PR should improve compliance and doesn't interfere with making further changes going forward.
Suggestions on how to test this: the DDIExportUtilTests check the changes. Manually I think you can check the DDI metadata export to see the changes as well. One could also test the OAI-PMH outputs directly (I haven't but afaik they use the same ddi generation code.
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?:
Additional documentation: