Merge pull request #4303 from samvera/gh-4301

Addressing case where children are added again

app/actors/hyrax/actors/attach_members_actor.rb

@@ -41,6 +41,7 @@ def assign_nested_attributes_for_collection(env, attributes_collection)
               next unless existing_works.include?(attributes['id'])
               remove(env.curation_concern, attributes['id'])
             else
+              next if existing_works.include?(attributes['id'])
               add(env, attributes['id'])
             end
           end

app/actors/hyrax/actors/collections_membership_actor.rb

@@ -31,7 +31,7 @@ def update(env)
         # @param env [Hyrax::Actors::Enviornment]
         # @return [Boolean]
         #
-        # rubocop:disable Metrics/MethodLength
+        # rubocop:disable Metrics/MethodLength, Metrics/AbcSize
         # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
         def assign_nested_attributes_for_collection(env)
           attributes_collection = env.attributes.delete(:member_of_collections_attributes)
@@ -56,13 +56,14 @@ def assign_nested_attributes_for_collection(env)
               next unless existing_collections.include?(attributes['id'])
               remove(env.curation_concern, attributes['id'])
             else
+              next if existing_collections.include?(attributes['id'])
               add(env, attributes['id'])
             end
           end
 
           true
         end
-        # rubocop:enable Metrics/MethodLength
+        # rubocop:enable Metrics/MethodLength, Metrics/AbcSize
         # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
 
         ##

app/services/hyrax/file_set_csv_service.rb

@@ -1,3 +1,4 @@
+require 'csv'
 module Hyrax
   #
   # Generates CSV from a FileSet

spec/actors/hyrax/actors/attach_members_actor_spec.rb

@@ -4,7 +4,6 @@
   let(:terminator) { Hyrax::Actors::Terminator.new }
   let(:depositor) { create(:user) }
   let(:work) { create(:work) }
-  let(:attributes) { HashWithIndifferentAccess.new(work_members_attributes: { '0' => { id: id } }) }
 
   subject(:middleware) do
     stack = ActionDispatch::MiddlewareStack.new.tap do |middleware|
@@ -20,7 +19,6 @@
       work.ordered_members << existing_child_work
     end
     let(:existing_child_work) { create(:work) }
-    let(:id) { existing_child_work.id }
 
     context "without useful attributes" do
       let(:attributes) { {} }
@@ -29,12 +27,14 @@
     end
 
     context "when the id already exists in the members" do
+      let(:attributes) { HashWithIndifferentAccess.new(work_members_attributes: { '0' => { id: existing_child_work.id } }) }
+
       it "does nothing" do
         expect { subject }.not_to change { work.ordered_members.to_a }
       end
 
       context "and the _destroy flag is set" do
-        let(:attributes) { HashWithIndifferentAccess.new(work_members_attributes: { '0' => { id: id, _destroy: 'true' } }) }
+        let(:attributes) { HashWithIndifferentAccess.new(work_members_attributes: { '0' => { id: existing_child_work.id, _destroy: 'true' } }) }
 
         it "removes from the member and the ordered members" do
           expect { subject }.to change { work.ordered_members.to_a }
@@ -44,9 +44,37 @@
       end
     end
 
+    context "when working through Rails nested attribute scenarios" do
+      before do
+        allow(ability).to receive(:can?).with(:edit, GenericWork).and_return(true)
+        work.ordered_members << work_to_remove
+      end
+
+      let(:work_to_remove) { create(:work, title: ['Already Member and Remove']) }
+      let(:work_to_skip) { create(:work, title: ['Not a Member']) }
+      let(:work_to_add) { create(:work, title: ['Not a Member but want to add']) }
+
+      let(:attributes) do
+        HashWithIndifferentAccess.new(
+          work_members_attributes: {
+            '0' => { id: work_to_remove.id, _destroy: 'true' }, # colleciton is a member and we're removing it
+            '1' => { id: work_to_skip.id, _destroy: 'true' }, # colleciton is a NOT member and is marked for deletion; This is a UI introduced option
+            '2' => { id: existing_child_work.id },
+            '3' => { id: work_to_add.id }
+          }
+        )
+      end
+
+      it "handles destroy/non-destroy and keep/add behaviors" do
+        expect { subject }.to change { work.ordered_members.to_a }
+        expect(work.ordered_member_ids).to match_array [existing_child_work.id, work_to_add.id]
+        expect(work.member_ids).to match_array [existing_child_work.id, work_to_add.id]
+      end
+    end
+
     context "when the id does not exist in the members" do
       let(:another_work) { create(:work) }
-      let(:id) { another_work.id }
+      let(:attributes) { HashWithIndifferentAccess.new(work_members_attributes: { '0' => { id: another_work.id } }) }
 
       context "and I can edit that object" do
         before do

spec/actors/hyrax/actors/collections_membership_actor_spec.rb

@@ -84,6 +84,33 @@
       end
     end
 
+    context "when working through Rails nested attribute scenarios" do
+      let(:collection_to_remove) { create(:collection_lw, user: user, title: ['Already Member and Remove'], with_permission_template: true) }
+      let(:collection_to_skip) { create(:collection_lw, user: user, title: ['Not a Member'], with_permission_template: true) }
+      let(:collection_to_keep) { create(:collection_lw, user: user, title: ['Is a member and we want to keep'], with_permission_template: true) }
+      let(:collection_to_add) { create(:collection_lw, user: user, title: ['Not a Member but want to add'], with_permission_template: true) }
+      let(:attributes) do
+        {
+          member_of_collections_attributes: {
+            '0' => { id: collection_to_remove.id, _destroy: 'true' }, # colleciton is a member and we're removing it
+            '1' => { id: collection_to_skip.id, _destroy: 'true' }, # colleciton is a NOT member and is marked for deletion; This is a UI introduced option
+            '2' => { id: collection_to_keep.id },
+            '3' => { id: collection_to_add.id }
+          }
+        }
+      end
+
+      before do
+        curation_concern.member_of_collections = [collection_to_remove, collection_to_keep]
+        curation_concern.save!
+      end
+
+      it "removes the work from that collection" do
+        expect(subject.create(env)).to be true
+        expect(curation_concern.member_of_collections).to match_array [collection_to_keep, collection_to_add]
+      end
+    end
+
     context "when work is in another user's collection" do
       let(:other_user) { create(:user) }
       let(:other_collection) { build(:collection_lw, user: other_user, title: ['A good title'], with_permission_template: true) }