diff --git a/app/controllers/concerns/hyrax/works_controller_behavior.rb b/app/controllers/concerns/hyrax/works_controller_behavior.rb index 96ec3559e2..96fd12a63e 100644 --- a/app/controllers/concerns/hyrax/works_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/works_controller_behavior.rb @@ -55,21 +55,12 @@ def new end def create - # Caching the original input params in case the form is not valid - original_input_params_for_form = params[hash_key_for_curation_concern].deep_dup - if create_work - after_create_response + case curation_concern + when ActiveFedora::Base + original_input_params_for_form = params[hash_key_for_curation_concern].deep_dup + actor.create(actor_environment) ? after_create_response : after_create_error(curation_concern.errors, original_input_params_for_form) else - respond_to do |wants| - wants.html do - build_form - # Creating a form object that can re-render most of the - # submitted parameters - @form = Hyrax::Forms::FailedSubmissionFormWrapper.new(form: @form, input_params: original_input_params_for_form) - render 'new', status: :unprocessable_entity - end - wants.json { render_json_response(response_type: :unprocessable_entity, options: { errors: curation_concern.errors }) } - end + create_valkyrie_work end end @@ -101,16 +92,11 @@ def edit end def update - if update_work - after_update_response + case curation_concern + when ActiveFedora::Base + actor.update(actor_environment) ? after_update_response : after_update_error(curation_concern.errors) else - respond_to do |wants| - wants.html do - build_form - render 'edit', status: :unprocessable_entity - end - wants.json { render_json_response(response_type: :unprocessable_entity, options: { errors: curation_concern.errors }) } - end + update_valkyrie_work end end @@ -189,36 +175,38 @@ def actor ## # @return [#errors] - def create_work - case curation_concern - when ActiveFedora::Base - actor.create(actor_environment) - else - form = build_form - - @curation_concern = - form.validate(params[hash_key_for_curation_concern]) && - transactions['change_set.create_work'] - .with_step_args('work_resource.add_to_parent' => { parent_id: params[:parent_id], user: current_user }, - 'work_resource.add_file_sets' => { uploaded_files: uploaded_files, file_set_params: params[hash_key_for_curation_concern][:file_set] }, - 'change_set.set_user_as_depositor' => { user: current_user }) - .call(form).value! - end + def create_valkyrie_work + form = build_form + return after_create_error(form_err_msg(form)) unless form.validate(params[hash_key_for_curation_concern]) + + result = + transactions['change_set.create_work'] + .with_step_args('work_resource.add_to_parent' => { parent_id: params[:parent_id], user: current_user }, + 'work_resource.add_file_sets' => { uploaded_files: uploaded_files, file_set_params: params[hash_key_for_curation_concern][:file_set] }, + 'change_set.set_user_as_depositor' => { user: current_user }) + .call(form) + @curation_concern = result.value_or { return after_create_error(transaction_err_msg(result)) } + after_create_response end - def update_work - case curation_concern - when ActiveFedora::Base - actor.update(actor_environment) - else - form = build_form + def update_valkyrie_work + form = build_form + return after_update_error(form_err_msg(form)) unless form.validate(params[hash_key_for_curation_concern]) - @curation_concern = - form.validate(params[hash_key_for_curation_concern]) && - transactions['change_set.update_work'] - .with_step_args('work_resource.add_file_sets' => { uploaded_files: uploaded_files, file_set_params: params[hash_key_for_curation_concern][:file_set] }) - .call(form).value! - end + result = + transactions['change_set.update_work'] + .with_step_args('work_resource.add_file_sets' => { uploaded_files: uploaded_files, file_set_params: params[hash_key_for_curation_concern][:file_set] }) + .call(form) + @curation_concern = result.value_or { return after_update_error(transaction_err_msg(result)) } + after_update_response + end + + def form_err_msg(form) + form.errors.messages.values.flatten.to_sentence + end + + def transaction_err_msg(result) + result.failure.first end def presenter @@ -377,6 +365,26 @@ def after_create_response end end + def after_create_error(errors, original_input_params_for_form = nil) + respond_to do |wants| + wants.html do + flash[:error] = errors.to_s + rebuild_form(original_input_params_for_form) if original_input_params_for_form.present? + render 'new', status: :unprocessable_entity + end + wants.json { render_json_response(response_type: :unprocessable_entity, options: { errors: errors }) } + end + end + + # Creating a form object that can re-render most of the submitted parameters. + # Required for ActiveFedora::Base objects only. + def rebuild_form(original_input_params_for_form) + build_form + @form = Hyrax::Forms::FailedSubmissionFormWrapper + .new(form: @form, + input_params: original_input_params_for_form) + end + def after_update_response return redirect_to hyrax.confirm_access_permission_path(curation_concern) if permissions_changed? && concern_has_file_sets? @@ -386,6 +394,17 @@ def after_update_response end end + def after_update_error(errors) + respond_to do |wants| + wants.html do + flash[:error] = errors.to_s + build_form unless @form.is_a? Hyrax::ChangeSet + render 'edit', status: :unprocessable_entity + end + wants.json { render_json_response(response_type: :unprocessable_entity, options: { errors: errors }) } + end + end + def after_destroy_response(title) respond_to do |wants| wants.html { redirect_to hyrax.my_works_path, notice: "Deleted #{title}" } diff --git a/app/forms/hyrax/forms/resource_form.rb b/app/forms/hyrax/forms/resource_form.rb index 326b814c81..187de1ea32 100644 --- a/app/forms/hyrax/forms/resource_form.rb +++ b/app/forms/hyrax/forms/resource_form.rb @@ -99,6 +99,8 @@ class ResourceForm < Hyrax::ChangeSet # rubocop:disable Metrics/ClassLength property :in_works_ids, virtual: true, prepopulator: InWorksPrepopulator property :member_ids, default: [], type: Valkyrie::Types::Array property :member_of_collection_ids, default: [], type: Valkyrie::Types::Array + property :member_of_collections_attributes, virtual: true + validates_with CollectionMembershipValidator property :representative_id, type: Valkyrie::Types::String property :thumbnail_id, type: Valkyrie::Types::String diff --git a/app/services/hyrax/multiple_membership_checker.rb b/app/services/hyrax/multiple_membership_checker.rb index 444baad28f..0c080649a8 100644 --- a/app/services/hyrax/multiple_membership_checker.rb +++ b/app/services/hyrax/multiple_membership_checker.rb @@ -11,6 +11,26 @@ def initialize(item:) @item = item end + # @api public + # + # Validate member_of_collection_ids does not have any membership conflicts + # based on the collection types of the members. + # + # Collections that have a collection type declaring + # `allow_multiple_membership` as `false` require that its members do not + # also belong to other collections of the same type. + # + # @return [True, String] true if no conflicts; otherwise, an error message string + def validate + return true if item.member_of_collection_ids.empty? || item.member_of_collection_ids.count <= 1 + return true unless single_membership_collection_types_exist? + + collections_to_check = filter_to_single_membership_collections(item.member_of_collection_ids) + problematic_collections = check_collections(collections_to_check) + errs = build_error_message(problematic_collections) + errs.presence || true + end + # @api public # # Scan a list of collection_ids for multiple single-membership collections. @@ -42,15 +62,18 @@ def check(collection_ids:, include_current_members: false) private + # @return [Boolean] true if there a any collection types that restrict membership def single_membership_collection_types_exist? single_membership_collection_types_gids.present? end + # @return [Array] list of collection instances for single membership collections def filter_to_single_membership_collections(collection_ids) return [] if collection_ids.blank? field_pairs = { @@ -58,11 +81,14 @@ def filter_to_single_membership_collections(collection_ids) } Hyrax::SolrQueryService.new .with_generic_type(generic_type: "Collection") - .with_ids(ids: Array[collection_ids]) + .with_ids(ids: Array(collection_ids).map(&:to_s)) .with_field_pairs(field_pairs: field_pairs, join_with: ' OR ') .get_objects(use_valkyrie: true).to_a end + # @param proposed [Array] collections with restricted membership that are being added + # @param include_current_members [Boolean] true if current collections for item should also be checked + # @return [Array] collections with restricted membership def collections_to_check(proposed, include_current_members) # ActorStack does a wholesale collection membership replacement, such that # proposed collections include existing and new collections. Parameter @@ -72,6 +98,19 @@ def collections_to_check(proposed, include_current_members) proposed | filter_to_single_membership_collections(item.member_of_collection_ids) end + # @param collections_to_check [Array] collections with restricted membership + # @return [Array] collections groups by collection type + # @example example return result + # [ + # [ + # , + # + # ], + # [ + # , + # + # ] + # ] def check_collections(collections_to_check) # uniq insures we include a collection only once when it is in the list multiple # group_by groups collections of the same collection type together @@ -82,6 +121,7 @@ def check_collections(collections_to_check) .select { |_gid, list| list.count > 1 } end + # @return [nil, String] nil if no errors; otherwise, errors appended into a single human readable message def build_error_message(problematic_collections) return if problematic_collections.blank? error_message_clauses = problematic_collections.map do |gid, list| @@ -93,10 +133,13 @@ def build_error_message(problematic_collections) "#{error_message_clauses.join('; ')}" end + # @return [String] title of the collection type def collection_type_title_from_gid(gid) Hyrax::CollectionType.find_by_gid(gid).title end + # @return [String] comma separated (with and before final) list of titles + # @example "Title 1, Title 2, and Title 3" def collection_titles_from_list(collection_list) collection_list.map do |collection| collection.title.first diff --git a/app/validators/hyrax/collection_membership_validator.rb b/app/validators/hyrax/collection_membership_validator.rb new file mode 100644 index 0000000000..f4101da31d --- /dev/null +++ b/app/validators/hyrax/collection_membership_validator.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true +module Hyrax + # Validates that the record passes the multiple membership checker + class CollectionMembershipValidator < ActiveModel::Validator + def validate(record) + update_collections(record) + validation = validate_multi_membership(record) + return if validation == true + record.errors[:member_of_collection_ids] << validation + end + + private + + def validate_multi_membership(record) + # collections-in-collections do not have multi-membership restrictions + return true if record.is_a? Hyrax::Forms::PcdmCollectionForm + + Hyrax::MultipleMembershipChecker.new(item: record).validate + end + + def update_collections(record) + record.member_of_collection_ids = collections_ids(record) + record.member_of_collection_ids.uniq! + end + + def collections_ids(record) + collection_ids = [] + if record.member_of_collections_attributes.present? + record.member_of_collections_attributes + .each do |_k, h| + next if h["_destroy"] == "true" + collection_ids << Valkyrie::ID.new(h["id"]) + end + end + collection_ids + end + end +end diff --git a/lib/hyrax/transactions/work_create.rb b/lib/hyrax/transactions/work_create.rb index 000ba59e93..719e06dd35 100644 --- a/lib/hyrax/transactions/work_create.rb +++ b/lib/hyrax/transactions/work_create.rb @@ -11,7 +11,6 @@ class WorkCreate < Transaction DEFAULT_STEPS = ['change_set.set_default_admin_set', 'change_set.ensure_admin_set', 'change_set.set_user_as_depositor', - 'change_set.add_to_collections', 'change_set.apply', 'work_resource.save_acl', 'work_resource.add_file_sets', diff --git a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb index cb56d79c46..816d827c13 100644 --- a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb +++ b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb @@ -514,6 +514,44 @@ .to include(have_attributes(mode: :read, agent: 'group/public')) end end + + context 'and error occurs' do + let(:update_params) { { title: 'new title', visibility: 'open' } } + + context 'in form validation' do + let(:error_msg) { 'FORM VALIDATION FAILED' } + before do + allow_any_instance_of(Hyrax::Forms::ResourceForm).to receive(:validate).and_return(false) # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(Hyrax::Forms::ResourceForm) # rubocop:disable RSpec/AnyInstance + .to receive_message_chain(:errors, :messages, :values).and_return([error_msg]) # rubocop:disable RSpec/MessageChain + end + + it 'stays on the edit form and flashes an error message' do + patch :update, params: { id: id, test_simple_work: update_params } + + expect(flash[:error]).to eq error_msg + expect(response).to render_template(:edit) + end + end + + context 'in transactions' do + let(:error_msg) { 'TRANSACTION FAILED' } + let(:failure) { Dry::Monads::Failure([error_msg, fake_new_work]) } + let(:fake_new_work) { instance_double(Hyrax::Test::SimpleWork) } + + before do + allow_any_instance_of(Hyrax::Transactions::Steps::Save) # rubocop:disable RSpec/AnyInstance + .to receive(:call).with(any_args).and_return(failure) + end + + it 'stays on the edit form and flashes an error message' do + patch :update, params: { id: id, test_simple_work: update_params } + + expect(flash[:error]).to eq error_msg + expect(response).to render_template(:edit) + end + end + end end end end diff --git a/spec/factories/hyrax_collection.rb b/spec/factories/hyrax_collection.rb index b8b76fb072..d817bdda63 100644 --- a/spec/factories/hyrax_collection.rb +++ b/spec/factories/hyrax_collection.rb @@ -9,6 +9,7 @@ transient do with_permission_template { true } + with_index { true } user { create(:user) } edit_groups { [] } edit_users { [] } @@ -22,7 +23,8 @@ if evaluator.members.present? evaluator.members.map do |member| member.member_of_collection_ids += [collection.id] - Hyrax.persister.save(resource: member) + member = Hyrax.persister.save(resource: member) + Hyrax.index_adapter.save(resource: member) if evaluator.with_index end end if evaluator.with_permission_template @@ -39,6 +41,7 @@ evaluator.read_users collection.permission_manager.acl.save end + Hyrax.index_adapter.save(resource: collection) if evaluator.with_index end trait :public do @@ -56,5 +59,17 @@ members { [valkyrie_create(:hyrax_collection), valkyrie_create(:hyrax_collection)] } end end + + trait :as_collection_member do + member_of_collection_ids { [valkyrie_create(:hyrax_collection).id] } + end + + trait :as_member_of_multiple_collections do + member_of_collection_ids do + [valkyrie_create(:hyrax_collection).id, + valkyrie_create(:hyrax_collection).id, + valkyrie_create(:hyrax_collection).id] + end + end end end diff --git a/spec/forms/hyrax/forms/resource_form_spec.rb b/spec/forms/hyrax/forms/resource_form_spec.rb index b90f6f741a..b45b7056d8 100644 --- a/spec/forms/hyrax/forms/resource_form_spec.rb +++ b/spec/forms/hyrax/forms/resource_form_spec.rb @@ -190,6 +190,56 @@ end end + describe '#member_of_collection_ids' do + it 'for a new object has empty membership' do + expect(form.member_of_collection_ids).to be_empty + end + + context 'when collection membership is updated' do + context 'from none to one' do + let(:work) { FactoryBot.valkyrie_create(:hyrax_work) } + let(:member_of_collections_attributes) do + { "0" => { "id" => "123", "_destroy" => "false" } } + end + + it 'is populated from member_of_collections_attributes' do + expect { form.validate(member_of_collections_attributes: member_of_collections_attributes) } + .to change { form.member_of_collection_ids&.map(&:id) } + .to contain_exactly('123') + end + end + + context 'from 3 down to 2' do + let(:work) { FactoryBot.valkyrie_create(:hyrax_work, member_of_collection_ids: before_collection_ids) } + let(:col1) { FactoryBot.valkyrie_create(:hyrax_collection) } + let(:col2) { FactoryBot.valkyrie_create(:hyrax_collection) } + let(:col3) { FactoryBot.valkyrie_create(:hyrax_collection) } + let(:before_collection_ids) { [col1.id, col2.id, col3.id] } + let(:after_collection_ids) { [col1.id.to_s, col2.id.to_s] } + let(:member_of_collections_attributes) do + { "0" => { "id" => col1.id.to_s, "_destroy" => "false" }, + "1" => { "id" => col2.id.to_s, "_destroy" => "false" }, + "2" => { "id" => col3.id.to_s, "_destroy" => "true" } } + end + + it 'is populated from member_of_collections_attributes' do + expect { form.validate(member_of_collections_attributes: member_of_collections_attributes) } + .to change { form.member_of_collection_ids } + .to contain_exactly(*after_collection_ids) + end + end + end + + context 'when the work is a member of collections' do + let(:work) { FactoryBot.valkyrie_create(:hyrax_work, :as_member_of_multiple_collections) } + + it 'gives collection ids' do + expect(form.member_of_collection_ids) + .to contain_exactly(*work.member_of_collection_ids.map(&:id)) + end + end + end + describe '#model_class' do it 'is the class of the model' do expect(form.model_class).to eq work.class diff --git a/spec/hyrax/transactions/work_create_spec.rb b/spec/hyrax/transactions/work_create_spec.rb index 66e7a6cb4d..c8c3a23a2a 100644 --- a/spec/hyrax/transactions/work_create_spec.rb +++ b/spec/hyrax/transactions/work_create_spec.rb @@ -43,22 +43,6 @@ end end - context 'when adding to collections' do - let(:collections) do - [FactoryBot.valkyrie_create(:hyrax_collection), - FactoryBot.valkyrie_create(:hyrax_collection)] - end - - let(:collection_ids) { collections.map(&:id) } - - it 'adds to the collections' do - tx.with_step_args('change_set.add_to_collections' => { collection_ids: collection_ids }) - - expect(tx.call(change_set).value!) - .to have_attributes member_of_collection_ids: contain_exactly(*collection_ids) - end - end - context 'when attaching uploaded files' do let(:uploaded_files) { FactoryBot.create_list(:uploaded_file, 4) } diff --git a/spec/services/hyrax/multiple_membership_checker_spec.rb b/spec/services/hyrax/multiple_membership_checker_spec.rb index 9435ee9c50..41f7eef903 100644 --- a/spec/services/hyrax/multiple_membership_checker_spec.rb +++ b/spec/services/hyrax/multiple_membership_checker_spec.rb @@ -220,4 +220,106 @@ end end end + + describe '#validate' do + let(:base_errmsg) { "Error: You have specified more than one of the same single-membership collection type" } + + let(:checker) { described_class.new(item: item) } + let(:collection_ids) { ['foobar'] } + let!(:collection_type) { FactoryBot.create(:collection_type, title: 'Greedy', allow_multiple_membership: false) } + let(:collection_types) { [collection_type] } + let(:collection_type_gids) { [collection_type.to_global_id] } + + subject(:validation) { checker.validate } + + context 'when the item has 0 collections' do + let(:item) { FactoryBot.build(:hyrax_work) } + it 'returns true' do + expect(validation).to be true + end + end + + context 'when the item has 1 collection' do + let(:item) { FactoryBot.build(:hyrax_work, :as_collection_member) } + it 'returns true' do + expect(validation).to be true + end + end + + context 'when there are no single-membership collection types' do + before { allow(Hyrax::CollectionType).to receive(:gids_that_do_not_allow_multiple_membership).and_return([]) } + it 'returns true' do + expect(validation).to be true + end + end + + context 'when there are no single-membership collection instances' do + let(:item) { FactoryBot.build(:hyrax_work, :as_member_of_multiple_collections) } + let(:collection_ids) { item.member_of_collection_ids } + it 'returns true' do + expect(validation).to be true + end + end + + context 'when there are single-membership collection instances' do + let!(:sm_collection_type1) { FactoryBot.create(:collection_type, title: 'Single Membership 1', allow_multiple_membership: false) } + let!(:sm_collection_type2) { FactoryBot.create(:collection_type, title: 'Single Membership 2', allow_multiple_membership: false) } + + context 'and the collections are of different single-membership types' do + let(:item) { FactoryBot.build(:hyrax_work, member_of_collection_ids: collection_ids) } + let(:col1) do + FactoryBot.valkyrie_create(:hyrax_collection, + title: ['Foo'], + collection_type_gid: sm_collection_type1.to_global_id, + with_index: true) + end + let(:col2) do + FactoryBot.valkyrie_create(:hyrax_collection, + title: ['Bar'], + collection_type_gid: sm_collection_type2.to_global_id, + with_index: true) + end + let(:collection_ids) { [col1.id, col2.id] } + + it 'returns true' do + expect(validation).to be true + end + end + + context 'and the collections are of same single-membership types' do + let(:item) { FactoryBot.build(:hyrax_work, member_of_collection_ids: collection_ids) } + let(:col1) do + FactoryBot.valkyrie_create(:hyrax_collection, + title: ['Foo'], + collection_type_gid: sm_collection_type1.to_global_id, + with_index: true) + end + let(:col2) do + FactoryBot.valkyrie_create(:hyrax_collection, + title: ['Bar'], + collection_type_gid: sm_collection_type1.to_global_id, + with_index: true) + end + let(:collection_ids) { [col1.id, col2.id] } + + it 'returns an error' do + regexp = /#{base_errmsg} \(type: Single Membership 1, collections: (Foo and Bar|Bar and Foo)\)/ + expect(validation).to match regexp + end + end + + context 'and some collections are of same single-membership types' do + let(:item) { FactoryBot.build(:hyrax_work, member_of_collection_ids: collection_ids) } + let(:col1) { FactoryBot.valkyrie_create(:hyrax_collection, title: ['Foo'], collection_type_gid: sm_collection_type1.to_global_id) } + let(:col2) { FactoryBot.valkyrie_create(:hyrax_collection, title: ['Bar'], collection_type_gid: sm_collection_type1.to_global_id) } + let(:col3) { FactoryBot.valkyrie_create(:hyrax_collection, title: ['Baz']) } + let(:collection_ids) { [col1.id, col2.id, col3.id] } + + it 'returns an error' do + regexp = /#{base_errmsg} \(type: Single Membership 1, collections: (Foo and Bar|Bar and Foo)\)/ + expect(validation).to match regexp + end + end + end + end end diff --git a/spec/validators/collection_membership_validator_spec.rb b/spec/validators/collection_membership_validator_spec.rb new file mode 100644 index 0000000000..2974199603 --- /dev/null +++ b/spec/validators/collection_membership_validator_spec.rb @@ -0,0 +1,254 @@ +# frozen_string_literal: true +RSpec.describe Hyrax::CollectionMembershipValidator, :clean_repo do + describe '#validate' do + subject(:validator) { described_class.new } + + context 'when record is a work form changeset' do + let(:form) { Hyrax::Forms::ResourceForm.new(work) } + let(:work) { FactoryBot.build(:hyrax_work) } + let(:mem_of_cols_attrs) { {} } + + # @note The form sets :member_of_collections_attributes to include + # * all collections (already existing and newly added) that will be the set of collections + # * any collections that were removed + # The validator is only checking collections that will be the set of collections. + before { allow(form).to receive(:member_of_collections_attributes).and_return(mem_of_cols_attrs) } + + context 'and there are no collections' do + let(:mem_of_cols_attrs) { nil } + it 'validates and sets member_of_collection_ids to empty' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to be_empty + end + end + + context 'and work is added to collections' do + let(:col1) { FactoryBot.valkyrie_create(:hyrax_collection) } + let(:col2) { FactoryBot.valkyrie_create(:hyrax_collection) } + let(:mem_of_cols_attrs) do + { "0" => { "id" => col1.id.to_s, "_destroy" => "false" }, + "1" => { "id" => col2.id.to_s, "_destroy" => "false" } } + end + + it 'validates and sets member_of_collection_ids to new collections' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to contain_exactly(col1.id, col2.id) + end + + context 'when it is already in a collection' do + let(:work) { FactoryBot.build(:hyrax_work, :as_collection_member) } + let(:col_id) { work.member_of_collection_ids.first.id } + + it 'validates and replaces member_of_collection_ids with new collection set' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to contain_exactly(col1.id, col2.id) + end + end + + context 'and collection type does not allow multiple membership' do + let(:single_mem_col_type) { FactoryBot.create(:collection_type, allow_multiple_membership: false) } + + context 'and work is in another collection NOT posing a conflict' do + let(:work) { FactoryBot.build(:hyrax_work, :as_collection_member) } + let(:col_id) { work.member_of_collection_ids.first.id } + let(:sm_col) { FactoryBot.valkyrie_create(:hyrax_collection, collection_type_gid: single_mem_col_type.to_global_id) } + let(:mem_of_cols_attrs) do + { "0" => { "id" => col_id.to_s, "_destroy" => "false" }, + "1" => { "id" => sm_col.id.to_s, "_destroy" => "false" } } + end + + it 'validates and appends new collections to member_of_collection_ids' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to contain_exactly(col_id, sm_col.id) + end + end + + context 'and work is in another collection that IS posing a conflict' do + let(:work) { FactoryBot.build(:hyrax_work, member_of_collection_ids: [sm_col1.id]) } + let(:sm_col1) { FactoryBot.valkyrie_create(:hyrax_collection, collection_type_gid: single_mem_col_type.to_global_id) } + let(:sm_col2) { FactoryBot.valkyrie_create(:hyrax_collection, collection_type_gid: single_mem_col_type.to_global_id) } + let(:mem_of_cols_attrs) do + { "0" => { "id" => sm_col1.id.to_s, "_destroy" => "false" }, + "1" => { "id" => sm_col2.id.to_s, "_destroy" => "false" } } + end + + it 'fails validating and sets errors' do + validator.validate(form) + + expect(form.errors).not_to be_blank + expect(form.member_of_collection_ids).to contain_exactly(sm_col1.id, sm_col2.id) + end + end + end + end + + context 'and includes removing work from a collection' do + let(:work) { FactoryBot.build(:hyrax_work, :as_collection_member) } + let(:col_id) { work.member_of_collection_ids.first.id } + let(:mem_of_cols_attrs) do + { "0" => { "id" => col_id, "_destroy" => "true" } } + end + + context 'when only one collections' do + it 'validates and sets member_of_collection_ids to empty' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to be_empty + end + end + + context 'when it was in multiple collections' do + let(:work) { FactoryBot.build(:hyrax_work, :as_member_of_multiple_collections) } + let(:col_ids) { work.member_of_collection_ids } + let(:remove_col_id) { col_ids.first } + let(:keep_col_ids) { col_ids - [remove_col_id] } + let(:mem_of_cols_attrs) do + { "0" => { "id" => remove_col_id, "_destroy" => "true" }, + "1" => { "id" => keep_col_ids.first, "_destroy" => "false" }, + "2" => { "id" => keep_col_ids.second, "_destroy" => "false" } } + end + + it 'validates, removing one collection, and leaving the rest' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to contain_exactly(*keep_col_ids) + end + end + end + end + + context 'when record is a collection form changeset' do + let(:form) { Hyrax::Forms::PcdmCollectionForm.new(col) } + let(:col) { FactoryBot.build(:hyrax_collection) } + let(:mem_of_cols_attrs) { {} } + + # @note Collections do not restrict membership and always pass validation + before { allow(form).to receive(:member_of_collections_attributes).and_return(mem_of_cols_attrs) } + + context 'and there are no collections' do + let(:mem_of_cols_attrs) { {} } + it 'validates and sets member_of_collection_ids to empty' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to be_empty + end + end + + context 'and collection is added to collections' do + let(:col1) { FactoryBot.valkyrie_create(:hyrax_collection) } + let(:col2) { FactoryBot.valkyrie_create(:hyrax_collection) } + let(:mem_of_cols_attrs) do + { "0" => { "id" => col1.id.to_s, "_destroy" => "false" }, + "1" => { "id" => col2.id.to_s, "_destroy" => "false" } } + end + + it 'validates and sets member_of_collection_ids to new collections' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to contain_exactly(col1.id, col2.id) + end + + context 'when it is already in a collection' do + let(:col) { FactoryBot.build(:hyrax_collection, :as_collection_member) } + let(:col_id) { col.member_of_collection_ids.first.id } + + it 'validates and replaces member_of_collection_ids with new collection set' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to contain_exactly(col1.id, col2.id) + end + end + + context 'and collection type of parent collection does not allow multiple membership' do + let(:single_mem_col_type) { FactoryBot.create(:collection_type, allow_multiple_membership: false) } + + context 'and collection is in another collection of a different collection type' do + let(:col) { FactoryBot.build(:hyrax_collection, :as_collection_member) } + let(:col_id) { col.member_of_collection_ids.first.id } + let(:sm_col) { FactoryBot.valkyrie_create(:hyrax_collection, collection_type_gid: single_mem_col_type.to_global_id) } + let(:mem_of_cols_attrs) do + { "0" => { "id" => col_id.to_s, "_destroy" => "false" }, + "1" => { "id" => sm_col.id.to_s, "_destroy" => "false" } } + end + + it 'validates and appends new collections to member_of_collection_ids' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to contain_exactly(col_id, sm_col.id) + end + end + + context 'and collection is in another collection of the same single membership type' do + let(:col) { FactoryBot.build(:hyrax_collection, member_of_collection_ids: [sm_col1.id]) } + let(:sm_col1) { FactoryBot.valkyrie_create(:hyrax_collection, collection_type_gid: single_mem_col_type.to_global_id) } + let(:sm_col2) { FactoryBot.valkyrie_create(:hyrax_collection, collection_type_gid: single_mem_col_type.to_global_id) } + let(:mem_of_cols_attrs) do + { "0" => { "id" => sm_col1.id.to_s, "_destroy" => "false" }, + "1" => { "id" => sm_col2.id.to_s, "_destroy" => "false" } } + end + + it 'validates and appends new collections to member_of_collection_ids' do + # @note This passes because collections are always allowed in any other collection + # as long as all collections involved support nesting. As nesting is not validated + # by this validator, it is not tested here. + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to contain_exactly(sm_col1.id, sm_col2.id) + end + end + end + end + + context 'and includes removing collection from a collection' do + let(:col) { FactoryBot.build(:hyrax_collection, :as_collection_member) } + let(:col_id) { col.member_of_collection_ids.first.id } + let(:mem_of_cols_attrs) do + { "0" => { "id" => col_id, "_destroy" => "true" } } + end + + context 'when only one collections' do + it 'validates and sets member_of_collection_ids to empty' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to be_empty + end + end + + context 'when it was in multiple collections' do + let(:col) { FactoryBot.build(:hyrax_collection, :as_member_of_multiple_collections) } + let(:col_ids) { col.member_of_collection_ids } + let(:remove_col_id) { col_ids.first } + let(:keep_col_ids) { col_ids - [remove_col_id] } + let(:mem_of_cols_attrs) do + { "0" => { "id" => remove_col_id, "_destroy" => "true" }, + "1" => { "id" => keep_col_ids.first, "_destroy" => "false" }, + "2" => { "id" => keep_col_ids.second, "_destroy" => "false" } } + end + + it 'validates, removing one collection, and leaving the rest' do + validator.validate(form) + + expect(form.errors).to be_blank + expect(form.member_of_collection_ids).to contain_exactly(*keep_col_ids) + end + end + end + end + end +end