update collection membership for works during edit

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}" }

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

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,27 +62,33 @@ 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<String] global ids of collection types that restrict membership
     def single_membership_collection_types_gids
       @single_membership_collection_types_gids ||=
         Hyrax::CollectionType.gids_that_do_not_allow_multiple_membership&.map(&:to_s)
     end
 
+    # @return [Array<Hyrax::PcdmCollection>] list of collection instances for single membership collections
     def filter_to_single_membership_collections(collection_ids)
       return [] if collection_ids.blank?
       field_pairs = {
         Hyrax.config.collection_type_index_field.to_sym => single_membership_collection_types_gids
       }
       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<Hyrax::PcdmCollection>] 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<Hyrax::PcdmCollection>] 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<Hyrax::PcdmCollection>] collections with restricted membership
+    # @return [Array<Array>] collections groups by collection type
+    # @example example return result
+    #   [
+    #     [
+    #       <Collection(id: 1, collection_type_gid: 1, ...)>,
+    #       <Collection(id: 4, collection_type_gid: 1, ...)>
+    #     ],
+    #     [
+    #       <Collection(id: 13, collection_type_gid: 8, ...)>,
+    #       <Collection(id: 26, collection_type_gid: 8, ...)>
+    #     ]
+    #   ]
     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

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

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',

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