From aaa73d30dbb5327dfe5a1a1d30c3a795a079db0a Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 7 May 2020 08:42:49 -0400 Subject: [PATCH 1/3] Addressing case where children are added again Prior to this commit, there was no guard in place to prevent adding children again. The logic would check "is something marked for delete" if not, then add it. That needed to change, because when a user would submit a form changing keywords, then we would re-add all of the already added members. Closes #4301 (which relates to #4193 and #4278) Related to #4302 --- app/actors/hyrax/actors/attach_members_actor.rb | 1 + app/actors/hyrax/actors/collections_membership_actor.rb | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) 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 ## From b201f7310f805b7e9ecd48ec0bb6838cfa7f49cd Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 7 May 2020 10:33:39 -0400 Subject: [PATCH 2/3] Reworking spec to include UI scenario Prior to this commit, there were no specs handling the two axis of variance: Axis 1: A work is or is not already an existing member of the parent work. Axis 1: The passed attributes hash has marked an object for destroy, or for addition. This spec is hear to capture that relationshp. --- .../hyrax/actors/attach_members_actor_spec.rb | 36 ++++++++++++++++--- .../collections_membership_actor_spec.rb | 27 ++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) 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) } From de2d1aa6f110500cbf85b4f0d7516d118b728522 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 7 May 2020 10:48:29 -0400 Subject: [PATCH 3/3] Addressing failure to load CSV constant --- app/services/hyrax/file_set_csv_service.rb | 1 + 1 file changed, 1 insertion(+) 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