diff --git a/app/controllers/hyrax/dashboard/collection_members_controller.rb b/app/controllers/hyrax/dashboard/collection_members_controller.rb index 0d59057c22..5481f49df7 100644 --- a/app/controllers/hyrax/dashboard/collection_members_controller.rb +++ b/app/controllers/hyrax/dashboard/collection_members_controller.rb @@ -21,21 +21,25 @@ def after_update_error(err_msg) end end - def update_members + def update_members # rubocop:disable Metrics/MethodLength err_msg = validate after_update_error(err_msg) if err_msg.present? return if err_msg.present? collection.reindex_extent = Hyrax::Adapters::NestingIndexAdapter::LIMITED_REINDEX - members = collection.add_member_objects batch_ids - messages = members.collect { |member| member.errors.full_messages }.flatten - if messages.size == members.size - after_update_error(messages.uniq.join(', ')) - elsif messages.present? - flash[:error] = messages.uniq.join(', ') - after_update - else + begin + Hyrax::Collections::CollectionMemberService.add_members_by_ids(collection: collection.valkyrie_resource, + new_member_ids: batch_ids, + user: current_user) after_update + rescue Hyrax::SingleMembershipError => err + messages = JSON.parse(err.message) + if messages.size == batch_ids.size + after_update_error(messages.uniq.join(', ')) + elsif messages.present? + flash[:error] = messages.uniq.join(', ') + after_update + end end end diff --git a/app/controllers/hyrax/dashboard/collections_controller.rb b/app/controllers/hyrax/dashboard/collections_controller.rb index a98ea39c7b..40bd9e7be8 100644 --- a/app/controllers/hyrax/dashboard/collections_controller.rb +++ b/app/controllers/hyrax/dashboard/collections_controller.rb @@ -379,7 +379,9 @@ def process_member_changes def add_members_to_collection(collection = nil) collection ||= @collection - collection.add_member_objects batch + Hyrax::Collections::CollectionMemberService.add_members_by_ids(collection: collection.valkyrie_resource, + new_member_ids: batch, + user: current_user) end def remove_members_from_collection diff --git a/app/models/concerns/hyrax/collection_behavior.rb b/app/models/concerns/hyrax/collection_behavior.rb index c1f15c7126..9b8a87cfdb 100644 --- a/app/models/concerns/hyrax/collection_behavior.rb +++ b/app/models/concerns/hyrax/collection_behavior.rb @@ -51,27 +51,22 @@ def collection_type=(new_collection_type) end # Add members using the members association. - # TODO: Confirm if this is ever used. I believe all relationships are done through - # add_member_objects using the member_of_collections relationship. Deprecate? def add_members(new_member_ids) - return if new_member_ids.blank? - members << Hyrax.custom_queries.find_many_by_alternate_ids(alternate_ids: new_member_ids, use_valkyrie: false) + Deprecation.warn("'##{__method__}' will be removed in Hyrax 4.0. " \ + "Instead, use Hyrax::Collections::CollectionMemberService.add_members_by_ids.") + Hyrax::Collections::CollectionMemberService.add_members_by_ids(collection: valkyrie_resource, + new_member_ids: new_member_ids, + user: nil) end # Add member objects by adding this collection to the objects' member_of_collection association. # @param [Enumerable] the ids of the new child collections and works collection ids def add_member_objects(new_member_ids) - Array(new_member_ids).collect do |member_id| - member = Hyrax.query_service.find_by_alternate_identifier(alternate_identifier: member_id, use_valkyrie: false) - message = Hyrax::MultipleMembershipChecker.new(item: member).check(collection_ids: id, include_current_members: true) - if message - member.errors.add(:collections, message) - else - member.member_of_collections << self - member.save! - end - member - end + Deprecation.warn("'##{__method__}' will be removed in Hyrax 4.0. " \ + "Instead, use Hyrax::Collections::CollectionMemberService.add_members_by_ids.") + Hyrax::Collections::CollectionMemberService.add_members_by_ids(collection: valkyrie_resource, + new_member_ids: new_member_ids, + user: nil) end # @return [Enumerable] an enumerable over the children of this collection diff --git a/app/services/hyrax/collections/collection_member_service.rb b/app/services/hyrax/collections/collection_member_service.rb index 0fbfd9bfb5..424b0a1931 100644 --- a/app/services/hyrax/collections/collection_member_service.rb +++ b/app/services/hyrax/collections/collection_member_service.rb @@ -81,7 +81,15 @@ def add_members_by_ids(collection:, new_member_ids:, user:) # @param new_members [Enumerable] the new child collections and/or child works # @return [Enumerable] updated member resources def add_members(collection:, new_members:, user:) - new_members.map { |new_member| add_member(collection: collection, new_member: new_member, user: user) } + messages = [] + new_members.map do |new_member| + begin + add_member(collection: collection, new_member: new_member, user: user) + rescue Hyrax::SingleMembershipError => err + messages += [err.message] + end + end + raise Hyrax::SingleMembershipError, messages if messages.present? end # Add a work or collection as a member of a collection @@ -99,7 +107,7 @@ def add_member_by_id(collection:, new_member_id:, user:) # @return [Hyrax::Resource] updated member resource def add_member(collection:, new_member:, user:) message = Hyrax::MultipleMembershipChecker.new(item: new_member).check(collection_ids: [collection.id], include_current_members: true) - raise Hyrax::SingleMembershipError, message if message + raise Hyrax::SingleMembershipError, message if message.present? new_member.member_of_collection_ids += [collection.id] # only populate this direction new_member = Hyrax.persister.save(resource: new_member) Hyrax.publisher.publish('object.metadata.updated', object: new_member, user: user) diff --git a/spec/controllers/hyrax/dashboard/collection_members_controller_spec.rb b/spec/controllers/hyrax/dashboard/collection_members_controller_spec.rb index e608ab7a41..36e57ba168 100644 --- a/spec/controllers/hyrax/dashboard/collection_members_controller_spec.rb +++ b/spec/controllers/hyrax/dashboard/collection_members_controller_spec.rb @@ -2,7 +2,7 @@ RSpec.describe Hyrax::Dashboard::CollectionMembersController, :clean_repo do routes { Hyrax::Engine.routes } let(:user) { create(:user) } - let(:other) { build(:user) } + let(:other) { create(:user) } let(:work_1_own) { create(:work, id: 'work-1-own', title: ['First of the Assets'], user: user) } let(:work_2_own) { create(:work, id: 'work-2-own', title: ['Second of the Assets'], user: user) } @@ -408,5 +408,35 @@ end end end + + context 'when members violate the multi-membership checker for single membership collections' do + let!(:sm_collection_type) { create(:collection_type, title: 'Single Membership', allow_multiple_membership: false) } + let(:coll_1_sm) { create(:collection_lw, id: 'coll_1_sm', title: ['SM1'], collection_type: sm_collection_type, user: user) } + let!(:coll_2_sm) { create(:collection_lw, id: 'coll_2_sm', title: ['SM2'], collection_type: sm_collection_type, user: user) } + let(:base_errmsg) { "Error: You have specified more than one of the same single-membership collection type" } + let(:regexp) { /#{base_errmsg} \(type: Single Membership, collections: (SM1 and SM2|SM2 and SM1)\)/ } + + before do + sign_in user + [work_1_own, work_2_own].each do |asset| + asset.member_of_collections << coll_1_sm + asset.save! + Hyrax.publisher.publish('object.metadata.updated', object: asset.valkyrie_resource, user: user) + end + Hyrax.publisher.publish('object.metadata.updated', object: coll_1_sm.valkyrie_resource, user: user) + Hyrax.publisher.publish('object.metadata.updated', object: coll_2_sm.valkyrie_resource, user: user) + end + + it "displays error message and deposits works not in violation" do + expect do + post :update_members, params: { id: coll_2_sm, + collection: { members: 'add' }, + batch_document_ids: [work_1_own.id, work_2_own.id, work_3_own.id] } + end.to change { coll_2_sm.reload.member_objects.size }.by(1) + expect(flash[:error]).to match regexp + expect(response).to redirect_to routes.url_helpers.dashboard_collection_path(coll_2_sm, locale: 'en') + expect(coll_2_sm.member_objects).to match_array [work_3_own] + end + end end end diff --git a/spec/models/collection_spec.rb b/spec/models/collection_spec.rb index 1092eea1ef..1948707a29 100644 --- a/spec/models/collection_spec.rb +++ b/spec/models/collection_spec.rb @@ -55,14 +55,15 @@ end context "when adding members" do - let(:work1) { create(:work) } - let(:work2) { create(:work) } - let(:work3) { create(:work) } - - it "allows multiple files to be added" do - collection.add_member_objects [work1.id, work2.id] - collection.save! - expect(collection.reload.member_objects).to match_array [work1, work2] + let(:work1) { valkyrie_create(:hyrax_work, title: 'Work 1') } + let(:work2) { valkyrie_create(:hyrax_work, title: 'Work 2') } + let(:work3) { valkyrie_create(:hyrax_work, title: 'Work 3') } + + it "allows multiple works to be added" do + Hyrax::Collections::CollectionMemberService.add_members(collection: collection.valkyrie_resource, + new_members: [work1, work2], + user: nil) + expect(collection.reload.member_objects.map(&:id)).to match_array [work1.id.to_s, work2.id.to_s] end context 'when multiple membership checker returns a non-nil value' do @@ -79,9 +80,12 @@ let(:error_message) { 'Error: foo bar' } it 'fails to add the member' do - collection.add_member_objects [work1.id, work2.id, work3.id] - collection.save! - expect(collection.reload.member_objects).to match_array [work1, work3] + begin + Hyrax::Collections::CollectionMemberService.add_members(collection: collection.valkyrie_resource, + new_members: [work1, work2, work3], + user: nil) + rescue; end # rubocop:disable Lint/SuppressedException + expect(collection.reload.member_objects.map(&:id)).to match_array [work1.id.to_s, work3.id.to_s] end end end @@ -89,16 +93,17 @@ describe "#destroy", clean_repo: true do let(:collection) { build(:collection_lw) } - let(:work1) { create(:work) } + let(:work1) { valkyrie_create(:hyrax_work) } before do - collection.add_member_objects [work1.id] - collection.save! + Hyrax::Collections::CollectionMemberService.add_members(collection: collection.valkyrie_resource, + new_members: [work1], + user: nil) collection.destroy end - it "does not delete member files when deleted" do - expect(GenericWork.exists?(work1.id)).to be true + it "does not delete member works when deleted" do + expect(Hyrax::Test::SimpleWorkLegacy.exists?(work1.id.to_s)).to be true end end @@ -111,7 +116,10 @@ class OtherCollection < ActiveFedora::Base class Member < ActiveFedora::Base include Hydra::Works::WorkBehavior end - collection.add_member_objects member.id + + Hyrax::Collections::CollectionMemberService.add_members(collection: collection.valkyrie_resource, + new_members: [member.valkyrie_resource], + user: nil) end after do Object.send(:remove_const, :OtherCollection) diff --git a/spec/models/concerns/hyrax/collection_behavior_spec.rb b/spec/models/concerns/hyrax/collection_behavior_spec.rb index 7da3216c5e..fa6a5083ba 100644 --- a/spec/models/concerns/hyrax/collection_behavior_spec.rb +++ b/spec/models/concerns/hyrax/collection_behavior_spec.rb @@ -6,7 +6,9 @@ describe "#destroy" do it "removes the collection id from associated members" do - collection.add_member_objects([work.id]) + Hyrax::Collections::CollectionMemberService.add_members(collection: collection.valkyrie_resource, + new_members: [work], + user: nil) collection.save collection_via_query = Hyrax.query_service.find_by_alternate_identifier(alternate_identifier: collection.id, use_valkyrie: false) diff --git a/spec/services/hyrax/collections/collection_member_service_spec.rb b/spec/services/hyrax/collections/collection_member_service_spec.rb index 75a79ebfa1..0f6f68c8cc 100644 --- a/spec/services/hyrax/collections/collection_member_service_spec.rb +++ b/spec/services/hyrax/collections/collection_member_service_spec.rb @@ -40,12 +40,18 @@ before { described_class.add_members_by_ids(collection: collection, new_member_ids: new_member_ids, user: user) } - context 'when no members' do + context 'when ids is empty' do + let(:new_member_ids) { [] } + it "returns without making changes" do + expect(custom_query_service.find_members_of(collection: collection).map(&:id)).to match_array new_member_ids + end + end + context 'when collection currently has no members' do it "updates the collection member set to contain only the new members" do expect(custom_query_service.find_members_of(collection: collection).map(&:id)).to match_array new_member_ids end end - context 'when has members' do + context 'when collection already has members' do let(:existing_work) { FactoryBot.valkyrie_create(:hyrax_work) } let(:existing_members) { [existing_work] } context 'and one of the new members already exists in the member set' do @@ -169,13 +175,16 @@ let(:collection_ids) { collections.map(&:id) } before do + Hyrax.publisher.publish('object.metadata.updated', object: collection, user: user) + Hyrax.publisher.publish('object.metadata.updated', object: collection2, user: user) allow(Hyrax::CollectionType).to receive(:gids_that_do_not_allow_multiple_membership).and_return([collection_type.to_global_id]) end it 'raises an error' do - errmsg = 'Error: You have specified more than one of the same single-membership collection type (type: Greedy, collections: Col-2 and Col-1)' + base_errmsg = "Error: You have specified more than one of the same single-membership collection type" + regexp = /#{base_errmsg} \(type: Greedy, collections: (Col-1 and Col-2|Col-2 and Col-1)\)/ expect { described_class.add_member(collection: collection2, new_member: new_member, user: user) } - .to raise_error Hyrax::SingleMembershipError, errmsg + .to raise_error Hyrax::SingleMembershipError, regexp end end end