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.

Sign up for GitHub

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 checks for required fields for primary article #1258

Merged
merged 5 commits into from
Jul 5, 2023
Merged

Fix checks for required fields for primary article #1258

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
44fbd9e
import button sentence casing
ahamelers Jun 29, 2023
bfde155
Closes https://github.com/CDL-Dryad/dryad-product-roadmap/issues/2585
ahamelers Jun 29, 2023
a99596b
use DOI placeholder to demonstrate actual DOI format
ahamelers Jun 29, 2023
dc20fe2
Fixing problems with primary article identifier and validation.
sfisher Jul 3, 2023
924c3fb
rubocop
sfisher Jul 3, 2023
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
6 changes: 3 additions & 3 deletions app/javascript/react/components/MetadataEntry/PrelimArticle.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ function PrelimArticle({
{
htmlId: 'publication',
labelText: 'Journal name',
isRequired: false,
isRequired: true,
}
}
/>
Expand All @@ -105,7 +105,7 @@ function PrelimArticle({
</label>
<Field
className="c-input__text"
placeholder="5702.125/qlm.1266rr"
placeholder="10.5702/qlm.1266rr"
type="text"
name="primary_article_doi"
id="primary_article_doi"
Expand All @@ -128,7 +128,7 @@ function PrelimArticle({
}}
disabled={(acText === '' || formRef?.current?.values.primary_article_doi === '')}
>
Import Article Metadata
Import article metadata
</button>
</div>
<div id="population-warnings" className="o-metadata__autopopulate-message">
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/react/components/MetadataEntry/PrelimManu.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ function PrelimManu({
}}
disabled={(acText === '' || acID === '' || formRef?.current?.values.msId === '')}
>
Import Manuscript Metadata
Import manuscript metadata
</button>
</div>
<div id="population-warnings" className="o-metadata__autopopulate-message">
Expand Down
41 changes: 24 additions & 17 deletions app/models/stash_datacite/resource/dataset_validations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,30 @@ def loose_errors
err.flatten
end

def article_id
err = []

if @resource.identifier.import_info != 'other' && @resource.identifier.publication_name.blank?
err << ErrorItem.new(message: 'Fill in the {journal of the related publication}',
page: metadata_page(@resource),
ids: ['publication'])
end

journal = " from #{@resource.identifier.publication_name}" if @resource.identifier.publication_name.present?

if @resource.identifier.import_info == 'published' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Audrey, this is the part that wasn't working and it was changed in this pull request. Before it had some conditions and returned at the first of the article_id method. If it got past returning early then it simply checked that a primary article existed.

The logic in here lines 102-103 seem to be the following:

IF import_info == 'published' AND (the identifier has a publication DOI OR there is a primary article) THEN give an error.

I'm making another commit to fix this and another bug I found if someone puts in a bad identifier for the primary article first.

My new code is

        primary_article = @resource.related_identifiers.where(work_type: 'primary_article').first
        if @resource.identifier.import_info == 'published' && (
                      primary_article.nil? || primary_article.related_identifier.blank? || !primary_article.valid_doi_format?
                    )

It also just looks up the primary article from this resource. The identifier version looks through the whole history for a primary article, but we probably don't need to do that and if their primary article is gone then they probably should change the options from those 3 choices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sfisher This part that you have quoted is just a variable to show a journal name in the error message if one is present, because the error message previously had a journal name in it and I wanted to maintain that, but now I'm showing the same error even if the journal name is blank. What's not working about it?

Copy link
Collaborator Author

@ahamelers ahamelers Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, I think I can see in the old code, that previously the primary_article DOI check was only triggered if the publication_doi field was blank. I guess when I was reconfiguring these checks I thought that was a bug, because this new change is a check that ensures the publication_doi field will never be left blank if the published article import option is selected, which means the primary_article DOI check will never be triggered—and there's an rspec test for it and everything! If it never being triggered is preferred, should I not just remove that primary_article check entirely?

(@resource.identifier.publication_article_doi.blank? || @resource.related_identifiers.where(work_type: 'primary_article').count.positive?)
err << ErrorItem.new(message: "Fill in {a correctly formatted DOI} for the article#{journal}",
page: metadata_page(@resource),
ids: %w[primary_article_doi])
elsif @resource.identifier.import_info == 'manuscript' && @resource.identifier.manuscript_number.blank?
err << ErrorItem.new(message: "Fill in the {manuscript number} for the article#{journal}",
page: metadata_page(@resource),
ids: ['msId'])
end
err
end

def title
if @resource.title.blank?
return ErrorItem.new(message: 'Fill in a {dataset title}',
Expand Down Expand Up @@ -167,23 +191,6 @@ def abstract
[]
end

def article_id
return [] unless @resource.identifier.publication_name.present? &&
@resource.identifier.manuscript_number.blank? &&
@resource.identifier.publication_article_doi.blank?

if @resource.related_identifiers.where(work_type: 'primary_article').count.positive? # has primary, but not doi
ErrorItem.new(message: "Fill in {a correctly formatted DOI} for your article from #{@resource.identifier
.publication_name}",
page: metadata_page(@resource),
ids: %w[primary_article_doi])
else
ErrorItem.new(message: "Fill in a {manuscript number or DOI} for the article from #{@resource.identifier.publication_name}",
page: metadata_page(@resource),
ids: %w[msid primary_article_doi])
end
end

def s3_error_uploads
return [] if @resource.submitted?

Expand Down
8 changes: 4 additions & 4 deletions spec/features/stash_datacite/manuscript_populate_metadata_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@
xit "gives message when journal isn't selected" do
find('input[value="manuscript"]').click
fill_manuscript_info(name: 'European Journal of Plant Pathology', issn: nil, msid: nil)
click_button 'Import Manuscript Metadata'
click_button 'Import manuscript metadata'
expect(page.find('div#population-warnings')).to have_content('Please select your journal from the autocomplete drop-down list')
end

it 'gives disable submit manuscript not filled' do
choose('a manuscript in progress', allow_label_click: true)
expect(page).to have_button('Import Manuscript Metadata', disabled: true)
expect(page).to have_button('Import manuscript metadata', disabled: true)
end

end
Expand Down Expand Up @@ -68,7 +68,7 @@
journal = ''
doi = ''
fill_crossref_info(name: journal, doi: doi)
expect(page).to have_button('Import Article Metadata', disabled: true)
expect(page).to have_button('Import article metadata', disabled: true)
end

it "gives a message when it can't find a doi" do
Expand Down Expand Up @@ -97,7 +97,7 @@ def click_import_article_metadata
# so we force it here.
page.execute_script("$('#do_import').val('true')")

click_button 'Import Article Metadata'
click_button 'Import article metadata'
end

end
Expand Down
2 changes: 1 addition & 1 deletion spec/javascript/react/components/MetadataEntry/PrelimArticle.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ describe('PrelimArticle', () => {

userEvent.tab(); // tab out of element, should trigger save on blur

await waitFor(() => expect(screen.getByText('Import Article Metadata')).toHaveFocus());
await waitFor(() => expect(screen.getByText('Import article metadata')).toHaveFocus());
await waitFor(() => promise); // waits for the axios promise to fulfil
})

Expand Down
2 changes: 1 addition & 1 deletion spec/javascript/react/components/MetadataEntry/PrelimManu.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('PrelimManu', () => {

userEvent.tab(); // tab out of element, should trigger save on blur

await waitFor(() => expect(screen.getByText('Import Manuscript Metadata')).toHaveFocus());
await waitFor(() => expect(screen.getByText('Import manuscript metadata')).toHaveFocus());
await waitFor(() => promise); // waits for the axios promise to fulfil
})

Expand Down
35 changes: 30 additions & 5 deletions spec/models/stash_datacite/dataset_validations_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,26 +97,51 @@ module Resource
end

describe :article_id do
it 'gives error for unfilled manuscript number or publication doi if publication' do
it 'gives error for unfilled publication' do
@resource.identifier.update(import_info: 'published')

validations = DatasetValidations.new(resource: @resource)
error = validations.article_id.first

expect(error.message).to include('journal of the related publication')
expect(error.ids).to eq(%w[publication])
end

it 'gives error for unfilled publication doi if publication' do
@resource.identifier.update(import_info: 'published')
StashEngine::InternalDatum.create(data_type: 'publicationName',
value: 'Barrel of Monkeys: the Primate Journal',
identifier_id: @resource.identifier_id)

validations = DatasetValidations.new(resource: @resource)
error = validations.article_id.first

expect(error.message).to include('formatted DOI')
expect(error.ids).to eq(%w[primary_article_doi])
end

it 'gives error for unfilled manuscript number if manuscript' do
@resource.identifier.update(import_info: 'manuscript')
StashEngine::InternalDatum.create(data_type: 'publicationName',
value: 'Barrel of Monkeys: the Primate Journal',
identifier_id: @resource.identifier_id)

validations = DatasetValidations.new(resource: @resource)
error = validations.article_id
error = validations.article_id.first

expect(error.message).to include('manuscript number or DOI')
expect(error.ids).to eq(%w[msid primary_article_doi])
expect(error.message).to include('manuscript number')
expect(error.ids).to eq(%w[msId])
end

it 'gives a formatting error when someone puts in a URL instead of a DOI' do
@resource.identifier.update(import_info: 'published')
StashEngine::InternalDatum.create(data_type: 'publicationName',
value: 'Barrel of Monkeys: the Primate Journal',
identifier_id: @resource.identifier_id)
create(:related_identifier, resource_id: @resource.id, related_identifier_type: 'url', work_type:
'primary_article')
validations = DatasetValidations.new(resource: @resource)
error = validations.article_id
error = validations.article_id.first

expect(error.message).to include('formatted DOI')
expect(error.ids).to eq(%w[primary_article_doi])
Expand Down
1 change: 1 addition & 0 deletions spec/support/helpers/dataset_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ def fill_required_metadata
# make sure we're on the right page
navigate_to_metadata

choose('choose_other')
fill_in 'title', with: Faker::Lorem.sentence
fill_in_author
fill_in_research_domain
Expand Down