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

DOI minting for data and code pathway #1584

Merged
merged 7 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ Rails/ApplicationController:
Exclude:
- 'app/controllers/api/v1/rest_controller.rb'

Rails/Blank:
Exclude:
- 'app/forms/work_deposit_pathway.rb'

Rails/SkipsModelValidations:
Exclude:
- 'app/controllers/dashboard/form/publish_controller.rb'
Expand Down
5 changes: 5 additions & 0 deletions app/controllers/dashboard/form/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ def allow_curation?
deposit_pathway.allows_curation_request? && in_publish_edit_action?
end

helper_method :allow_mint_doi?
def allow_mint_doi?
deposit_pathway.allows_mint_doi_request?
end

helper_method :allow_publish?
def allow_publish?
if deposit_pathway.work?
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/dashboard/form/publish_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
module Dashboard
module Form
class PublishController < BaseController
class DoiIsMintingError < RuntimeError; end

def edit
@work_version = WorkVersion
.includes(file_version_memberships: [:file_resource])
Expand Down Expand Up @@ -32,6 +34,9 @@ def update
@resource.indexing_source = Proc.new { nil }
@resource.save
@resource.publish
if @resource.mint_doi_requested
allow_mint_doi? ? MintDoiAsync.call(@resource.work) : flash[:error] = t('dashboard.form.publish.doi.error')
end
elsif request_curation? && [email protected]_curation_requested
@resource.update_column(:draft_curation_requested, true)
# We want validation errors to block curation requests and keep users on the edit page
Expand Down Expand Up @@ -115,6 +120,7 @@ def work_version_params
:published_date,
:depositor_agreement,
:draft_curation_requested,
:mint_doi_requested,
keyword: [],
contributor: [],
publisher: [],
Expand Down
6 changes: 6 additions & 0 deletions app/forms/work_deposit_pathway.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ def allows_curation_request?
data_and_code? && [email protected]_curation_requested
end

def allows_mint_doi_request?
data_and_code? && @resource.work.doi.blank? && !DoiMintingStatus.new(@resource.work).present?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

def work?
Work::Types.all.include?(work_type)
end
Expand Down Expand Up @@ -117,6 +121,7 @@ def imported_metadata_from_rmd?
:update_doi=,
:work_type,
:draft_curation_requested,
:mint_doi_requested,
to: :work_version, prefix: false

private
Expand Down Expand Up @@ -258,6 +263,7 @@ def form_partial
:aasm,
:update_column,
:draft_curation_requested=,
:mint_doi_requested=,
:set_thumbnail_selection,
to: :work_version,
prefix: false
Expand Down
1 change: 1 addition & 0 deletions app/jobs/doi_minting_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ class DoiMintingJob < ApplicationJob
queue_as :doi

def perform(resource)
sleep(2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Smullz622 Smullz622 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was able to get this to work locally without the sleep by disabling ActiveJob in the config for the dev environments:

https://github.com/psu-libraries/scholarsphere/blob/main/config/environments/development.rb#L19-L20

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't know why it works with SideKiq but not ActiveJob.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

status = DoiMintingStatus.new(resource)

status.minting!
Expand Down
20 changes: 20 additions & 0 deletions app/views/dashboard/form/publish/_doi.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<div class="form-group mt-5">
<div class="keyline keyline--left">
<h4><%= t '.header' %></h4>
</div>
<div>
<div>
<%= t '.request_description' %>
</div>
<br>
<div class="form-check">
<%= form.check_box :mint_doi_requested,
required: false,
class: 'form-check-input' %>

<%= form.label :mint_doi_requested, class: 'form-check-label' do %>
<%= t '.label' %>
<% end %>
</div>
</div>
</div>
4 changes: 4 additions & 0 deletions app/views/dashboard/form/publish/_fields.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
<%= render 'curation', form: form %>
<% end %>

<% if allow_mint_doi? %>
<%= render 'doi', form: form %>
<% end %>

<div class="form-group mt-5">
<div class="keyline keyline--left">
<h2 id="<%= WorkVersion.human_attribute_name(:depositor_agreement).parameterize %>" class="h4">
Expand Down
5 changes: 5 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,11 @@ en:
curation_requested: Curation has been requested. We will notify you when curation is complete and your work is ready to be published. If you have any questions in the meantime, please contact ScholarSphere curators via our
error: There was an error with your curation request. Please try again later or contact us if the problem persists.
contact: contact form
doi:
header: Mint DOI (Optional)
request_description: Placeholder for mint doi description
label: I would like to mint a DOI upon publish.
error: There was an error minting your doi. Please try again later from Work Settings or contact us if the problem persists.
members:
heading: Works
badge: Work
Expand Down
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I hadn't thought of that, and it totally makes sense. You're way ahead of me. 🤐

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddMintDoiRequestedToWorkVersion < ActiveRecord::Migration[6.1]
def change
add_column :work_versions, :mint_doi_requested, :boolean
end
end
4 changes: 3 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2024_03_21_203236) do
ActiveRecord::Schema.define(version: 2024_09_10_195820) do

# These are extensions that must be enabled in order to support this database
enable_extension "pg_stat_statements"
enable_extension "plpgsql"
Expand Down Expand Up @@ -263,6 +264,7 @@
t.boolean "imported_metadata_from_rmd"
t.boolean "draft_curation_requested"
t.datetime "sent_for_curation_at"
t.boolean "mint_doi_requested"
t.index ["external_app_id"], name: "index_work_versions_on_external_app_id"
t.index ["work_id", "version_number"], name: "index_work_versions_on_work_id_and_version_number", unique: true
t.index ["work_id"], name: "index_work_versions_on_work_id"
Expand Down
66 changes: 66 additions & 0 deletions spec/features/dashboard/work_version_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1242,4 +1242,70 @@ def mock_solr_indexing_job
end
end
end

describe 'minting a doi', js: true do
let(:user) { work_version.work.depositor.user }

context 'with a draft eligible for doi minting' do
let(:work) { create :work, work_type: 'dataset', versions_count: 1, has_draft: true, doi: nil }
let(:work_version) { work.versions.first }

it 'renders a checkbox requesting a doi be minted upon publish' do
visit dashboard_form_publish_path(work_version)

expect(page).to have_content(I18n.t('dashboard.form.publish.doi.label'))
end
end

context 'with a draft not eligible for doi minting' do
let(:work) { create :work, work_type: 'dataset', versions_count: 1, has_draft: true, doi: '10.26207/x8qe-xp35' }
let(:work_version) { work.versions.first }

it 'does not render a checkbox requesting a doi be minted upon publish' do
visit dashboard_form_publish_path(work_version)

expect(page).not_to have_content(I18n.t('dashboard.form.publish.doi.label'))
end
end

context 'when minting doi is successfully requested' do
let(:work_version) { create :work_version, :dataset_able_to_be_published }
let(:work) { create :work, work_type: 'dataset', versions_count: 1, has_draft: true, doi: nil, versions: [work_version] }

before do
allow(MintDoiAsync).to receive(:call).with(work_version.work)
end

it 'calls the MintDoi job' do
visit dashboard_form_publish_path(work_version)

check I18n.t('dashboard.form.publish.doi.label')
check 'I have read and agree to the deposit agreement.'

click_on 'Publish'

expect(MintDoiAsync).to have_received(:call).with(work_version.work)
end
end

context 'when an error occurs and the work version does not publish' do
let(:work_version) { create :work_version, :dataset_able_to_be_published }
let(:work) { create :work, work_type: 'dataset', versions_count: 1, has_draft: true, doi: nil, versions: [work_version] }

before do
work_version.work.doi = nil
allow(MintDoiAsync).to receive(:call).with(work_version.work)
end

it 'does not mint a doi' do
visit dashboard_form_publish_path(work_version)

check I18n.t('dashboard.form.publish.doi.label')

click_on 'Publish'

expect(MintDoiAsync).not_to have_received(:call).with(work_version.work)
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@
end
end

describe '#mint_doi_requested=' do
before { allow(wv).to receive(:mint_doi_requested=) }

let(:arg) { double }

it 'delegates to the given work version' do
form.mint_doi_requested = arg
expect(wv).to have_received(:mint_doi_requested=).with(arg)
end
end

describe '.form_fields' do
it "returns a frozen array of the names of the form's fields" do
expect(described_class.form_fields).to match_array %w{
Expand Down
1 change: 1 addition & 0 deletions spec/models/work_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,7 @@
keyword_sim
keyword_tesim
language_tesim
mint_doi_requested_tesim
model_ssi
proxy_id_isi
published_date_dtrsi
Expand Down