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

[HOLD] Fix validation of description values including title #681

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[HOLD] Fix validation of description values including title #681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 12 additions & 4 deletions lib/cocina/models/validators/description_values_validator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,22 +62,30 @@ def validate_values_for_blanks(hash, path)
end

def validate_values_for_multiples(hash, path)
return unless hash.count { |key, value| %i[value groupedValue structuredValue parallelValue].include?(key) && value.present? } > 1
return unless hash.count { |key, value| %w[value groupedValue structuredValue parallelValue].include?(key) && value.present? } > 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keys are strings, rather than symbols.

Copy link
Member

@mjgiarlo mjgiarlo Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How do we know these keys are strings?

The entrypoint to the DescriptionValuesValidator class is the Validator class, which converts the attributes to be validated into a HashWithIndifferentAccess instance.

The HashWithIndifferentAccess class maps symbol keys to string keys in its writing interface. Thus:

[y]ou are guaranteed that the key is returned as a string

(see https://api.rubyonrails.org/classes/ActiveSupport/HashWithIndifferentAccess.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for explaining/documenting this in detail!


error_paths_multiple << path_to_s(path)
end

def validate_title_type(hash, path)
# only apply to title.structuredValue, title.parallelValue.structuredValue, or relatedResource.title with a value
return unless hash[:value] && (path.first == :title || related_resource_title?(path)) && path.include?(:structuredValue)
return unless hash[:value] && (structured_value_title?(path) || related_resource_title?(path))

# if there is a "value" key, make sure there is also a "type" key, only for title.structuredValue
error_paths_missing_title_type << path_to_s(path) unless hash[:type]
end

def related_resource_title?(path)
# title is directly within relatedResource, e.g [:relatedResource, 0, :title, 0, :structuredValue, 0])
path.first == :relatedResource && path[2] == :title
# title is within relatedResource, e.g ["relatedResource", 0, "title", 0, "structuredValue", 0])
structured_value_path = path[4] == 'structuredValue' || (path[4] == 'parallelValue' && path[6] == 'structuredValue')
path.first == 'relatedResource' && path[2] == 'title' && structured_value_path
end

def structured_value_title?(path)
# title path includes a structuredValue directly or within a parallelValue
# e.g. ["title", 0, "structuredValue", 0] or ["title", 0, "parallelValue", 0, "structuredValue", 0])
structured_value_path = path[2] == 'structuredValue' || (path[2] == 'parallelValue' && path[4] == 'structuredValue')
path.first == 'title' && structured_value_path
end

def path_to_s(path)
Expand Down
20 changes: 9 additions & 11 deletions spec/cocina/models/validators/description_values_validator_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@

RSpec.describe Cocina::Models::Validators::DescriptionValuesValidator do
let(:clazz) { Cocina::Models::Description }

let(:props) { desc_props }

let(:props) { desc_props.with_indifferent_access }
let(:desc_props) do
{
title: [
Expand Down Expand Up @@ -166,7 +164,7 @@
end

describe 'when a valid RequestDescription' do
let(:props) { request_desc_props }
let(:props) { request_desc_props.with_indifferent_access }
let(:clazz) { Cocina::Models::RequestDescription }

it 'does not raise' do
Expand All @@ -175,7 +173,7 @@
end

describe 'when an invalid Description' do
let(:props) { invalid_desc_props }
let(:desc_props) { invalid_desc_props }

it 'is not valid' do
expect do
Expand All @@ -185,7 +183,7 @@
end

describe 'when a blank value in description' do
let(:props) { blank_title_props }
let(:desc_props) { blank_title_props }

it 'is not valid' do
expect do
Expand All @@ -195,7 +193,7 @@
end

describe 'when a no type for title structuredValue in description' do
let(:props) { no_title_type_props }
let(:desc_props) { no_title_type_props }

it 'is not valid' do
expect do
Expand All @@ -205,7 +203,7 @@
end

describe 'when no type for second structuredValue title in description' do
let(:props) { multiple_title_structured_value_props }
let(:desc_props) { multiple_title_structured_value_props }

it 'is not valid' do
expect do
Expand All @@ -215,7 +213,7 @@
end

describe 'when no type for related resource structured value title in description' do
let(:props) { related_resource_no_title_type_props }
let(:desc_props) { related_resource_no_title_type_props }

it 'is not valid' do
expect do
Expand All @@ -225,7 +223,7 @@
end

describe 'when no type for related resource parallelValue structured value title in description' do
let(:props) { related_resource_parallel_value_no_type_props }
let(:desc_props) { related_resource_parallel_value_no_type_props }

it 'is not valid' do
expect do
Expand All @@ -235,7 +233,7 @@
end

describe 'when structuredValue with no type within parallelValue' do
let(:props) { parallel_structured_props }
let(:desc_props) { parallel_structured_props }

it 'is not valid' do
expect do
Expand Down