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

allow for extensions of valkyrie indexer's to_solr that depends on resource custom metadata #4463

Merged
merged 2 commits into from
Jul 23, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions app/indexers/hyrax/valkyrie_collection_indexer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class ValkyrieCollectionIndexer < Hyrax::ValkyrieIndexer
include Hyrax::ResourceIndexer
include Hyrax::PermissionIndexer
include Hyrax::VisibilityIndexer
include Hyrax::Indexer(:core_metadata)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Collection resource defines core metadata, so seems like the collection indexer should index it.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


def to_solr
super.tap do |index_document|
Expand Down
2 changes: 1 addition & 1 deletion app/services/hyrax/thumbnail_path_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class << self
# @param [#id] object - to get the thumbnail for
# @return [String] a path to the thumbnail
def call(object)
return default_image unless object.try(:thumbnail_id)
return default_image if object.try(:thumbnail_id).blank?

thumb = fetch_thumbnail(object)

Expand Down
134 changes: 91 additions & 43 deletions lib/hyrax/specs/shared_specs/indexers.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,19 @@
# frozen_string_literal: true

# These shared specs test various aspects of valkyrie based indexers by calling the #to_solr method.
Copy link
Contributor

Choose a reason for hiding this comment

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

the docs are really appreciated!

# All tests require two variables to be set in the caller using let statements:
# * indexer_class - class of the indexer being tested
# * resource - a Hyrax::Resource that defines attributes and values consistent with the indexer
#
# NOTE: It is important that the resource has required values that the indexer #to_solr customizations expects to be available.
RSpec.shared_examples 'a Hyrax::Resource indexer' do
subject(:indexer) { indexer_class.new(resource: resource) }
let(:ids) { ['id1', 'id2'] }
let(:resource) { Hyrax::Resource.new(alternate_ids: ids) }
before do
raise 'indexer_class must be set with `let(:indexer_class)`' unless defined? indexer_class
raise 'resource must be set with `let(:resource)` and is expected to be a kind of Hyrax::Resource' unless defined?(resource) && resource.kind_of?(Hyrax::Resource)
Copy link
Contributor

Choose a reason for hiding this comment

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

i feel like the type checks might be heavy-handed, but let's keep them for now.

resource.alternate_ids = ids
end
subject(:indexer) { indexer_class.new(resource: resource) }
let(:ids) { ['id1', 'id2'] }

describe '#to_solr' do
it 'indexes alternate_ids' do
Expand All @@ -14,19 +24,22 @@
end

RSpec.shared_examples 'a permission indexer' do
before do
raise 'indexer_class must be set with `let(:indexer_class)`' unless defined? indexer_class
# NOTE: resource must be persisted for these tests to pass
raise 'resource must be set with `let(:resource)` and is expected to be a kind of Hyrax::Resource' unless defined?(resource) && resource.kind_of?(Hyrax::Resource)
Hyrax::VisibilityWriter.new(resource: resource)
.assign_access_for(visibility: Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PUBLIC)
resource.permission_manager.edit_groups = edit_groups
resource.permission_manager.edit_users = edit_users
resource.permission_manager.read_users = read_users
resource.permission_manager.acl.save
end
subject(:indexer) { indexer_class.new(resource: resource) }
let(:edit_groups) { [:managers] }
let(:edit_users) { [FactoryBot.create(:user)] }
let(:read_users) { [FactoryBot.create(:user)] }

let(:resource) do
FactoryBot.valkyrie_create(:hyrax_work, :public,
read_users: read_users,
edit_groups: edit_groups,
edit_users: edit_users)
end


describe '#to_solr' do
it 'indexes read permissions' do
expect(indexer.to_solr)
Expand All @@ -43,16 +56,25 @@
end

RSpec.shared_examples 'a visibility indexer' do
subject(:indexer) { indexer_class.new(resource: resource) }
let(:resource) { FactoryBot.build(:hyrax_work) }
before do
raise 'indexer_class must be set with `let(:indexer_class)`' unless defined? indexer_class
raise 'resource must be set with `let(:resource)` and is expected to be a kind of Hyrax::Resource' unless defined?(resource) && resource.kind_of?(Hyrax::Resource)
# optionally can pass in default_visibility by setting it with a let statement if your application changes the default; Hyrax defines this as 'restricted'
# See samvera/hyrda-head hydra-access-controls/app/models/concerns/hydra/access_controls/access_rights.rb for possible VISIBILITY_TEXT_VALUE_...'
end
subject(:indexer) { indexer_class.new(resource: resource) }

describe '#to_solr' do
it 'indexes visibility' do
expect(indexer.to_solr).to include(visibility_ssi: 'restricted')
it 'indexes default visibility as restricted or passed in default' do
expected_value = defined?(default_visibility) ? default_visibility : 'restricted'
expect(indexer.to_solr).to include(visibility_ssi: expected_value)
end

context 'when resource is public' do
let(:resource) { FactoryBot.valkyrie_create(:hyrax_work, :public) }
before do
Hyrax::VisibilityWriter.new(resource: resource)
.assign_access_for(visibility: Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PUBLIC)
end

it 'indexes as open' do
expect(indexer.to_solr).to include(visibility_ssi: 'open')
Expand All @@ -61,9 +83,37 @@
end
end

RSpec.shared_examples 'a Core metadata indexer' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly the same with the similar changes for variable validation and removing the default resource. It was moved to make the ordering of the various shared specs easier to follow.

before do
raise 'indexer_class must be set with `let(:indexer_class)`' unless defined? indexer_class
# NOTE: The resource's class is expected to have or inherit `include Hyrax::Schema(:core_metadata)`
raise 'resource must be set with `let(:resource)` and is expected to be a kind of Hyrax::Resource' unless defined?(resource) && resource.kind_of?(Hyrax::Resource)
resource.title = titles
end
subject(:indexer) { indexer_class.new(resource: resource) }
let(:titles) { ['Comet in Moominland', 'Finn Family Moomintroll'] }

describe '#to_solr' do
it 'indexes title as text' do
expect(indexer.to_solr)
.to include(title_tesim: a_collection_containing_exactly(*titles))
end

it 'indexes title as string' do
expect(indexer.to_solr)
.to include(title_sim: a_collection_containing_exactly(*titles))
end
end
end

RSpec.shared_examples 'a Basic metadata indexer' do
before do
raise 'indexer_class must be set with `let(:indexer_class)`' unless defined? indexer_class
# NOTE: The resource's class is expected to to have or inherit `include Hyrax::Schema(:basic_metadata)`
raise 'resource must be set with `let(:resource)` and is expected to be a kind of Hyrax::Resource' unless defined?(resource) && resource.kind_of?(Hyrax::Resource)
attributes.each { |k, v| resource.set_value(k, v) }
end
subject(:indexer) { indexer_class.new(resource: resource) }
let(:resource) { resource_class.new(**attributes) }

let(:attributes) do
{
Expand All @@ -72,26 +122,42 @@
}
end

let(:resource_class) do
Class.new(Hyrax::Work) do
include Hyrax::Schema(:basic_metadata)
end
end

describe '#to_solr' do
it 'indexes basic metadata' do
expect(indexer.to_solr)
.to include(keyword_sim: a_collection_containing_exactly(*attributes[:keyword]),
.to include(keyword_sim: a_collection_containing_exactly(*attributes[:keyword]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change just makes the a_collection... align with a_collection... in the following 2 lines. No real code change.

subject_tesim: a_collection_containing_exactly(*attributes[:subject]),
subject_sim: a_collection_containing_exactly(*attributes[:subject]))
end
end
end

RSpec.shared_examples 'a Work indexer' do
before do
raise 'indexer_class must be set with `let(:indexer_class)`' unless defined? indexer_class
# NOTE: resource must be persisted for permission tests to pass
raise 'resource must be set with `let(:resource)` and is expected to be a kind of Hyrax::Work' unless defined?(resource) && resource.kind_of?(Hyrax::Work)
# optionally can pass in default_visibility by setting it with a let statement if your application changes the default; Hyrax defines this as 'restricted'
# See samvera/hyrda-head hydra-access-controls/app/models/concerns/hydra/access_controls/access_rights.rb for possible VISIBILITY_TEXT_VALUE_...'
end
subject(:indexer) { indexer_class.new(resource: resource) }

it_behaves_like 'a Hyrax::Resource indexer'
it_behaves_like 'a Core metadata indexer'
it_behaves_like 'a permission indexer'
it_behaves_like 'a visibility indexer'
Copy link
Contributor Author

@elrayle elrayle Jul 22, 2020

Choose a reason for hiding this comment

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

The shared spec defines the minimal requirements for a work instead of expecting the individual work tests to know which parts are required.

end

RSpec.shared_examples 'a Collection indexer' do
before do
raise 'indexer_class must be set with `let(:indexer_class)`' unless defined? indexer_class
# NOTE: resource must be persisted for permission tests to pass
raise 'resource must be set with `let(:resource)` and is expected to be a kind of Hyrax::PcdmCollection' unless defined?(resource) && resource.kind_of?(Hyrax::PcdmCollection)
end
subject(:indexer) { indexer_class.new(resource: resource) }
let(:resource) { Hyrax::PcdmCollection.new }

it_behaves_like 'a Hyrax::Resource indexer'
it_behaves_like 'a Core metadata indexer'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The shared spec defines the minimal requirements for a collection instead of expecting the individual collection tests to know which parts are required.

it_behaves_like 'a permission indexer'
it_behaves_like 'a visibility indexer'

Expand All @@ -107,21 +173,3 @@
end
end
end

RSpec.shared_examples 'a Core metadata indexer' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved higher in the file

subject(:indexer) { indexer_class.new(resource: resource) }
let(:titles) { ['Comet in Moominland', 'Finn Family Moomintroll'] }
let(:resource) { Hyrax::Work.new(title: titles) }

describe '#to_solr' do
it 'indexes title as text' do
expect(indexer.to_solr)
.to include(title_tesim: a_collection_containing_exactly(*titles))
end

it 'indexes title as string' do
expect(indexer.to_solr)
.to include(title_sim: a_collection_containing_exactly(*titles))
end
end
end
1 change: 1 addition & 0 deletions spec/hyrax/indexer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
end

context 'with core metadata schema' do
let(:resource) { work }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

resource is required for shared indexer specs

it_behaves_like 'a Core metadata indexer'
end

Expand Down
1 change: 1 addition & 0 deletions spec/indexers/hyrax/permission_indexer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
include Hyrax::PermissionIndexer
end
end
let(:resource) { FactoryBot.valkyrie_create(:hyrax_resource) }

it_behaves_like 'a permission indexer'
end
1 change: 1 addition & 0 deletions spec/indexers/hyrax/resource_indexer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require 'hyrax/specs/shared_specs'

RSpec.describe Hyrax::ResourceIndexer do
let(:resource) { Hyrax::Resource.new }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is added to clarify and ensure the proper behavior. Prior to this, we had misleading successful test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resource is now required

let(:indexer_class) do
Class.new(Hyrax::ValkyrieIndexer) do
include Hyrax::ResourceIndexer
Expand Down
3 changes: 1 addition & 2 deletions spec/indexers/hyrax/valkyrie_collection_indexer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
require 'hyrax/specs/shared_specs'

RSpec.describe Hyrax::ValkyrieCollectionIndexer do
subject(:indexer) { described_class.new(resource: resource) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The subject is not used anywhere, so it was removed.

let(:resource) { FactoryBot.valkyrie_create(:hyrax_collection) }
let(:indexer_class) { described_class }

it_behaves_like 'a Hyrax::Resource indexer'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All shared specs representing the minimal required parts of a collection are now defined and tested in the shared spec 'a Collection indexer'.

it_behaves_like 'a Collection indexer'
end
36 changes: 31 additions & 5 deletions spec/indexers/hyrax/valkyrie_work_indexer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,48 @@
require 'hyrax/specs/shared_specs'

RSpec.describe Hyrax::ValkyrieWorkIndexer do
let(:indexer) { described_class.new(resource: resource) }
let(:resource) { FactoryBot.valkyrie_create(:hyrax_work) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

indexer and resource were not used by this test, so they were removed.

let(:indexer_class) { described_class }

it_behaves_like 'a Hyrax::Resource indexer'
it_behaves_like 'a Core metadata indexer'
it_behaves_like 'a permission indexer'
it_behaves_like 'a visibility indexer'
it_behaves_like 'a Work indexer'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All shared specs representing the minimal required parts of a work are now defined and tested in the shared spec 'a Work indexer'.


context 'when extending with basic metadata' do
let(:indexer_class) do
Class.new(described_class) do
include Hyrax::Indexer(:basic_metadata)
end
end
let(:resource) do
Class.new(Hyrax::Work) do
include Hyrax::Schema(:basic_metadata)
end.new
end

it_behaves_like 'a Basic metadata indexer'
end

context 'when extending with custom metadata' do
Copy link
Contributor Author

@elrayle elrayle Jul 20, 2020

Choose a reason for hiding this comment

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

This context is the spec that tests the described behavior of an indexer that uses custom metadata in the resource. Without the fix that allows the resource_class to be passed to the shared spec, this will fail with the Actual Behavior described in the PR comments. With the fix it passes with the Expected Behavior.

before do
module Hyrax::Test
module Custom
class Work < Hyrax::Work
attribute :broader, Valkyrie::Types::Array.of(Valkyrie::Types::String)
end
class WorkIndexer < Hyrax::ValkyrieWorkIndexer
def to_solr
super.tap do |solr_doc|
solr_doc['broader_ssim'] = resource.broader.first
end
end
end
end
end
end
after { Hyrax::Test.send(:remove_const, :Custom) }

let(:resource) { Hyrax.persister.save(resource: Hyrax::Test::Custom::Work.new(broader: ['term1', 'term2'])) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Per tech call, this is the line that the PR is supporting.

let(:indexer_class) { Hyrax::Test::Custom::WorkIndexer }
Copy link
Contributor

Choose a reason for hiding this comment

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

Per conversation, this was already supported.


it_behaves_like 'a Work indexer'
end
end
1 change: 1 addition & 0 deletions spec/indexers/hyrax/visibility_indexer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
include Hyrax::VisibilityIndexer
end
end
let(:resource) { Hyrax::Resource.new }

it_behaves_like 'a visibility indexer'
end
10 changes: 5 additions & 5 deletions spec/support/book_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ module Test
# Use this for testing valkyrie models generically, with Hyrax assumptions
# but no PCDM modelling behavior.
class BookResource < Hyrax::Resource
attribute :author, Valkyrie::Types::String
attribute :created, Valkyrie::Types::Date
attribute :isbn, Valkyrie::Types::String
attribute :pubisher, Valkyrie::Types::String
attribute :title, Valkyrie::Types::String
attribute :author, Valkyrie::Types::String
attribute :created, Valkyrie::Types::Date
attribute :isbn, Valkyrie::Types::String
attribute :publisher, Valkyrie::Types::String
attribute :title, Valkyrie::Types::String
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this is just an alignment change. The only real change is fixing the typo from pubisher to publisher.

end

class Book < ActiveFedora::Base
Expand Down