diff --git a/app/actors/hyrax/actors/attach_members_actor.rb b/app/actors/hyrax/actors/attach_members_actor.rb index 71d6a539c1..b5d9489aec 100644 --- a/app/actors/hyrax/actors/attach_members_actor.rb +++ b/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 diff --git a/app/actors/hyrax/actors/collections_membership_actor.rb b/app/actors/hyrax/actors/collections_membership_actor.rb index 9c66532c4a..3171d597f8 100644 --- a/app/actors/hyrax/actors/collections_membership_actor.rb +++ b/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 ## diff --git a/app/services/hyrax/file_set_csv_service.rb b/app/services/hyrax/file_set_csv_service.rb index 1a879f0803..a01929fee4 100644 --- a/app/services/hyrax/file_set_csv_service.rb +++ b/app/services/hyrax/file_set_csv_service.rb @@ -1,3 +1,4 @@ +require 'csv' module Hyrax # # Generates CSV from a FileSet diff --git a/spec/actors/hyrax/actors/attach_members_actor_spec.rb b/spec/actors/hyrax/actors/attach_members_actor_spec.rb index 1c6e4f1754..5dfd722068 100644 --- a/spec/actors/hyrax/actors/attach_members_actor_spec.rb +++ b/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 diff --git a/spec/actors/hyrax/actors/collections_membership_actor_spec.rb b/spec/actors/hyrax/actors/collections_membership_actor_spec.rb index 840812d533..14f9abf160 100644 --- a/spec/actors/hyrax/actors/collections_membership_actor_spec.rb +++ b/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) }