2471 destroy command should unregister doi #6060
Conversation
There was a problem hiding this comment.
Please excuse my intrusion on this issue. Should your comment #2471 (comment) be included in the documentation?
| @@ -189,6 +188,32 @@ public static String getMetadataFromDvObject(String identifier, Map<String, Stri | |||
| return xmlMetadata; | |||
| } | |||
|
|
|||
| public static String getMetadataForDeactivateIdentifier(String identifier, Map<String, String> metadata, DvObject dvObject) { | |||
|
|
|||
| System.out.print("Start of getMetadataForDeactivateIdentifier: "); | |||
There was a problem hiding this comment.
Why would you not use logging?
There was a problem hiding this comment.
This is debug code that should have been removed altogether. I will delete it.
| metadataTemplate.setIdentifier(identifier.substring(identifier.indexOf(':') + 1)); | ||
| metadataTemplate.setCreators(Util.getListFromStr(metadata.get("datacite.creator"))); | ||
|
|
||
| metadataTemplate.setDescription(":unav"); |
There was a problem hiding this comment.
Where does this special value come from?
There was a problem hiding this comment.
The special values here come from the DataCite Metadata schema https://support.datacite.org/docs/schema-values-unknown-information-v41
My intent for this is to mask any identifying information since the Dataset in question has been destroyed by Dataverse and in the opinion of the Dataverse admin should never have been created/published.
| } | ||
| } | ||
|
|
||
| protected HashMap<String, String> addMetadataForDestroyedDataset(DvObject dvObjectIn) { |
There was a problem hiding this comment.
Should the return type be Map instead of the more specific HashMap?
There was a problem hiding this comment.
Sure. Will change.
|
|
||
| protected HashMap<String, String> addMetadataForDestroyedDataset(DvObject dvObjectIn) { | ||
| HashMap<String, String> metadata = new HashMap<>(); | ||
| String authorString = ":unav"; |
There was a problem hiding this comment.
Where is this special string from?
| @@ -279,6 +304,8 @@ public boolean testDOIExists(String identifier) { | |||
| DOIDataCiteRegisterCache rc = findByDOI(identifier); | |||
| if (rc != null) { | |||
| metadata.put("_status", rc.getStatus()); | |||
| } else { | |||
| metadata.put("_status", "public"); | |||
There was a problem hiding this comment.
Should this public be a constant?
| metadata.put("datacite.creator", authorString); | ||
| metadata.put("datacite.title", titleString); | ||
| metadata.put("datacite.publisher", producerString); | ||
| metadata.put("datacite.publicationyear", "9999"); |
There was a problem hiding this comment.
I agree with @bencomp, that special strings should generally be defined as constants somewhere.
Also, I find this a little confusing that this method is in AbstractGlobalIdServiceBean - but it does something that's very specific to the Datacite metadata. I understand you are modeling this method after the already existing method above (addBasicMetadata()) that does the same thing... I'm assuming we do this in the "abstract" bean because this datacite metadata format is used by both the Datacite and EZID DOI implementations. I still feel this could be confusing for the next person working with this code; could you rename these methods, to somehow indicate that they are specific to the DOI implementations - but not to Handles (that extends the same Abstract service class); something like addDOIMetadataForDestroyedDataset() etc.?
There was a problem hiding this comment.
In addition to @bencomp's comments/requests, and what I added there, one more general question: Did we specifically decide that we were only implementing this for Datacite? (and not for EZID and/or Handles?)
"Out of scope" is a perfectly good answer - I just want to know if this was discussed at all.
|
@landreev there's a bit of discussion about EZID/Handles in the comment from @sekmiller at #2471 (comment). I think they are handled. If not, I'm OK with scoping this to just Datacite DOIs for now. |
|
... and belated thanks to @bencomp for the Code Review! |
Thanks @djbrooke - I should've read the ticket, yes. |
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
Pull Request Checklist