DOI minting for data and code pathway #1584
Conversation
app/forms/work_deposit_pathway.rb
Outdated
| @@ -33,6 +33,10 @@ def allows_curation_request? | |||
| data_and_code? && !@resource.draft_curation_requested | |||
| end | |||
|
|
|||
| def allows_mint_doi_request? | |||
| data_and_code? && @resource.work.doi.blank? && !DoiMintingStatus.new(@resource.work).present? | |||
There was a problem hiding this comment.
I'd suggest two refactorings here.
The first one is trivial and also much less important, but I think that the code would read a little better if we implemented DoiMintingStatus#blank? and used that here instead of ! and #present?.
The second and more important thing is that I'd try to get rid of the method chaining here. WorkDepositPathway needs to know too much about the resource object that it's holding. It needs to know that the resource has a work and that the work in turn can have a DOI. It would be better if each type of thing that might act as a resource here (maybe it's only WorkVersion? I can't remember..) could handle telling the work deposit pathway whether or not it has a DOI directly - i.e. @resource.doi_blank?. That reads better too, but it also relieves WorkDepositPathway of needing to know about the structure/hierarchy of that other information.
There was a problem hiding this comment.
First one I agree though as a note, #present? here is not a normal present. It's actually a custom method of DoiMintingStatus (threw me off for a long time trying to troubleshoot because #present? and #blank? were both returning false). But thinking on it, I can also just do a custom #blank? on DoiMintingStatus so we can still follow typical conventions.
Second sounds good & I'll get working on that.
There was a problem hiding this comment.
Does this information actually need to be saved to the database? In other words, do we ever need to recall it from the database after it has been saved? Or do we only care about it for the duration of the request where this form field is submitted? Could it simply be added as a transient attribute on WorkVersion instead of a persistent database column?
Or maybe it doesn't even need to be an attribute of WorkVersion at all. Is it enough for it to just be an attribute of the form object so that we only use it to determine whether or not to queue up the DOI minting and then maybe we don't even pass it along to the WorkVersion?
I'm asking as if I actually know the answer, but I don't. I don't immediately see why this would need to be saved, but there might be a reason that I don't know about. I just wanted to make sure that it was something that you considered.
There was a problem hiding this comment.
So I was debating about that as well. I landed on saving it since users can save & exit the form and come back later to publish. My instinct was that users who previously checked the box might assume it was still checked, submit without rechecking, and incorrectly think they're minting a doi upon publish. I could still go either way though & am open to suggestion.
There was a problem hiding this comment.
Ah! I hadn't thought of that, and it totally makes sense. You're way ahead of me. 🤐
app/jobs/doi_minting_job.rb
Outdated
| @@ -4,6 +4,7 @@ class DoiMintingJob < ApplicationJob | |||
| queue_as :doi | |||
|
|
|||
| def perform(resource) | |||
| sleep(2) | |||
There was a problem hiding this comment.
What was the difference in behavior that you said that you saw when you omitted this sleep call? Did you see it in both your development and test environments? Or was it in just one or the other?
There was a problem hiding this comment.
So test environment is fine either way, but in local dev & the preview branch, without the sleep, v1 work versions do not successfully mint a doi. It will mint one on v2 of the same work successfully though. I had trouble figuring out why this is because it is hitting the DoiMintingJob but not throwing any type of error that I can tell. I only found out the sleep works because I put in a byebug to dig further & it worked that time so I replaced it with a sleep & then everything worked as expected. Also why my suspicion is that it's something around timing with when it sends the record data to data cite.
There was a problem hiding this comment.
Though that being said, this is the only spot the sleep made a difference. I also tried one before if @resource.mint_doi_requested in publish_controller and at the beginning of the call method for MintDoiAsync and neither changed anything
There was a problem hiding this comment.
I think I was able to get this to work locally without the sleep by disabling ActiveJob in the config for the dev environments:
Looks like we override SideKiq in the development environment and just use ActiveJob instead. The minting still failed for me, but I think it's rejecting my url because it's a localhost url. But the job queued and ran. This doesn't explain why it's not working on the preview branch, though. It should be using SideKiq there.
There was a problem hiding this comment.
Also, I don't know why it works with SideKiq but not ActiveJob.
There was a problem hiding this comment.
Ah I see Eric has a fix for this. Using SideKiq probably just delayed the job enough to mimic what the sleep was doing. My comments can be ignored.
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
fixes #1492
When eligible, adds checkbox to publish page of work version form to request doi minting upon publish of the work. Eligible work versions are in the data & code pathway, do not already have a doi on the work, and a doi is not in the process of being minted for the work.