deprecate CollectionBehavior #add_member_objects

DO NOT MERGE - pending merge of PR #5055

----
* deprecates `Hyrax::CollectionBehavior#add_member_objects`
* updates all places that call it to `Hyrax::Collections::CollectionMemberService.add_members`

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
 

app/controllers/hyrax/dashboard/collections_controller.rb

@@ -112,10 +112,10 @@ def create
         @collection.collection_type_gid = params[:collection_type_gid].presence || default_collection_type.to_global_id
         @collection.attributes = collection_params.except(:members, :parent_id, :collection_type_gid)
         @collection.apply_depositor_metadata(current_user.user_key)
-        add_members_to_collection unless batch.empty?
         @collection.visibility = Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE unless @collection.discoverable?
         if @collection.save
           after_create
+          add_members_to_collection unless batch.empty?
         else
           after_create_error
         end
@@ -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

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<String>] 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<ActiveFedora::Base>] an enumerable over the children of this collection

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<Hyrax::Resource>] the new child collections and/or child works
         # @return [Enumerable<Hyrax::Resource>] 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)

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

spec/controllers/hyrax/dashboard/collections_controller_spec.rb

@@ -4,7 +4,7 @@
 RSpec.describe Hyrax::Dashboard::CollectionsController, :clean_repo do
   routes { Hyrax::Engine.routes }
   let(:user)  { create(:user) }
-  let(:other) { build(:user) }
+  let(:other) { create(:user) }
   let(:collection_type_gid) { FactoryBot.create(:user_collection_type).to_global_id.to_s }
 
   let(:collection) do

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,26 +80,30 @@
         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
   end
 
   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)

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)

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