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

fix: insert correct identifierType into DataCite export XML #10759

Closed
wants to merge 2 commits into from

Conversation

vera
Copy link
Contributor

@vera vera commented Aug 8, 2024

What this PR does / why we need it:

I noticed when exporting the metadata of a dataset with a Permalink PID that the DataCite XML export contained the wrong identifierType:

<identifier identifierType="DOI">https://example.com/mypermalink1234</identifier>

This PR fixes the bug by using getProviderType to insert the correct identifier type.

I also fixed some typos and added a missing return value for getProviderType.

Which issue(s) this PR closes:

not aware of any issue

Special notes for your reviewer:

-

Suggestions on how to test this:

I tested this locally by creating a dataset with a Permalink PID and then using the "Export Metadata" > "DataCite" button in the UI.

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?:

I think not

Additional documentation:

-

@pdurbin
Copy link
Member

pdurbin commented Aug 8, 2024

@vera thanks for the pull request!

Just a heads up that there is a significant refactoring by @qqmyers of the DataCite code in the works:

@qqmyers
Copy link
Member

qqmyers commented Aug 8, 2024

FWIW: The new code does set the identifierType (to "DOI", "Handle", or "URL" (for perma)) which should be allowed values for the DataCite schema. (Maybe the original code was written before exporters/when the output was only sent to DataCite and DOI was always true?) I'm not sure how fast #10615 is moving, so if this gets merged first (so we get the other fixes), I think it's fine.

@vera
Copy link
Contributor Author

vera commented Aug 9, 2024

Ah, that's great. Sorry, I wasn't aware of that.

Yes, I also checked the DataCite schema. I didn't find a list of allowed values, only that "identifierType" should be a non-empty string, so any string should be fine I think.

@qqmyers
Copy link
Member

qqmyers commented Aug 9, 2024

You're right. In this case it wasn't the DataCiteSchema but the OpenAire Guildelines that had a list.

@johannes-darms
Copy link
Contributor

Can we integrate this into version 6.4 if #10615 is not integrated?

@pdurbin
Copy link
Member

pdurbin commented Sep 27, 2024

@vera can you please resolve merge conflicts in this pull request? They are due to us merging this PR:

# Conflicts:
#	src/main/java/edu/harvard/iq/dataverse/pidproviders/doi/XmlMetadataTemplate.java
#	src/main/java/edu/harvard/iq/dataverse/pidproviders/doi/datacite/DOIDataCiteRegisterService.java
#	src/main/resources/edu/harvard/iq/dataverse/pidproviders/doi/datacite_metadata_template.xml
@vera
Copy link
Contributor Author

vera commented Sep 27, 2024

Hmm, I actually think this PR is not necessary anymore. The main bug this PR fixes (wrong identifierType in the DataCite export) is already fixed by #10632 (here: https://github.com/IQSS/dataverse/pull/10632/files#diff-ceea58ab3cfad4c5eb5265ad85516df9af3b75f31b32b764367989d5c59c84a0R225-R237).

I've tested the develop branch with a PermaLink dataset and it's now correct:

<identifier identifierType="URL">https://csh.nfdi4health.de/resource/health-IP89GF</identifier>

The rest of the changes in this PR don't need to be merged I think.

@coveralls
Copy link

Coverage Status

coverage: 20.872%. remained the same
when pulling 4f38834 on vera:fix/datacite-export-identifier-type
into 1b2a52b on IQSS:develop.

@pdurbin
Copy link
Member

pdurbin commented Sep 27, 2024

I'm confused why this pull request doesn't have a merge conflict because src/main/resources/edu/harvard/iq/dataverse/pidproviders/doi/datacite_metadata_template.xml was removed in b1e5020 🤔

@vera anyway, I think this PR will need to be adjusted to the new way added here:

@pdurbin
Copy link
Member

pdurbin commented Sep 27, 2024

Hmm, I actually think this PR is not necessary anymore.

Oh! @vera please feel free to close this PR if you don't need it. 😄

@vera vera closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants