-
Notifications
You must be signed in to change notification settings - Fork 490
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
4828 - handle pre-registering providers when creating file pids #4830
4828 - handle pre-registering providers when creating file pids #4830
Conversation
Apologies for the whitespace changes - the real change is just to set the identifier as registered and update the date prior to calling publicizeIdentifier for those providers that pre-register in two places - the first handling the files (which I've ~tested) and again in the file loop for datasets (which I haven't looked into). |
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.
I'm sorry to do this but can you please either fix up this pull request or make a new one that doesn't have all the whitespace changes?
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.
Please resolve the merge conflicts.
4828-DOI_registration_with_EZID_does_not_make_DOIs_public Conflicts: src/main/java/edu/harvard/iq/dataverse/engine/command/impl/RegisterDvObjectCommand.java
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.
Thanks for resolving the merge conflicts!
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.
I'm not very familiar with this part of the code base but I left a comment about a null check.
// publicizeIdentifier is called | ||
target.setIdentifierRegistered(true); | ||
target.setGlobalIdCreateTime(new Timestamp(new Date().getTime())); | ||
} |
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.
Is there a reason why the if
block above is not within the doiRetString != null
null check below?
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.
Hmm - no. Just has to happen before publicizeIdentifier is called and shouldn't happen if the doiRetString is null/doesn't match. Good catch. I'll change that and do the same in the files loop.
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.
Aside from Phil's note about moving the new logic inside the "null checks", the logic checks out to me. Aside from that I feel it is good to go.
of https://github.com/QualitativeDataRepository/dataverse.git into 4828-DOI_registration_with_EZID_does_not_make_DOIs_public Conflicts: src/main/java/edu/harvard/iq/dataverse/engine/command/impl/RegisterDvObjectCommand.java
Agree with Matthew. I think this is ready for QA. |
IdServiceBean did - loading the SettingsServiceBean.Key.Protocol if the protocol parameter was null. See Issue 4828 discussion.
…n_with_EZID_does_not_make_DOIs_public
Related Issues
Pull Request Checklist