Improve error handling for creating valkyrie collections

app/controllers/hyrax/dashboard/collections_controller.rb

@@ -84,26 +84,13 @@ def edit
       end
 
       def after_create
-        if @collection.is_a?(ActiveFedora::Base)
-          form
-          set_default_permissions
-          # if we are creating the new collection as a subcollection (via the nested collections controller),
-          # we pass the parent_id through a hidden field in the form and link the two after the create.
-          link_parent_collection(params[:parent_id]) unless params[:parent_id].nil?
-        end
-        respond_to do |format|
-          Hyrax::SolrService.commit
-          format.html { redirect_to edit_dashboard_collection_path(@collection), notice: t('hyrax.dashboard.my.action.collection_create_success') }
-          format.json { render json: @collection, status: :created, location: dashboard_collection_path(@collection) }
-        end
+        Deprecation.warn("Method `#after_create` will be removed in Hyrax 4.0.")
+        after_create_response # call private method for processing
       end
 
       def after_create_error
-        form
-        respond_to do |format|
-          format.html { render action: 'new' }
-          format.json { render json: @collection.errors, status: :unprocessable_entity }
-        end
+        Deprecation.warn("Method `#after_create_error` will be removed in Hyrax 4.0.")
+        after_create_errors("") # call private method for processing
       end
 
       def create
@@ -112,19 +99,12 @@ def create
         # collection is saved without a value for `has_model.`
         @collection = Hyrax.config.collection_class.new
         authorize! :create, @collection
-        return valkyrie_create if @collection.is_a?(Valkyrie::Resource)
 
-        # Coming from the UI, a collection type gid should always be present.  Coming from the API, if a collection type gid is not specified,
-        # use the default collection type (provides backward compatibility with versions < Hyrax 2.1.0)
-        @collection.collection_type_gid = params[:collection_type_gid].presence || default_collection_type.to_global_id
-        @collection.attributes = collection_params.except(:members, :parent_id, :collection_type_gid)
-        @collection.apply_depositor_metadata(current_user.user_key)
-        @collection.visibility = Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE unless @collection.discoverable?
-        if @collection.save
-          after_create
-          add_members_to_collection unless batch.empty?
+        case @collection
+        when ActiveFedora::Base
+          create_active_fedora_collection
         else
-          after_create_error
+          create_valkyrie_collection
         end
       end
 
@@ -215,20 +195,33 @@ def files
 
       private
 
-      def valkyrie_create
-        form.validate(collection_params) &&
-          @collection = transactions['change_set.create_collection']
-                        .with_step_args(
-                          'change_set.set_user_as_depositor' => { user: current_user },
-                          'change_set.add_to_collections' => { collection_ids: Array(params[:parent_id]) },
-                          'collection_resource.apply_collection_type_permissions' => { user: current_user }
-                        )
-                        .call(form)
-                        .value_or { return after_create_error }
+      def create_active_fedora_collection
+        # Coming from the UI, a collection type gid should always be present.  Coming from the API, if a collection type gid is not specified,
+        # use the default collection type (provides backward compatibility with versions < Hyrax 2.1.0)
+        @collection.collection_type_gid = params[:collection_type_gid].presence || default_collection_type.to_global_id
+        @collection.attributes = collection_params.except(:members, :parent_id, :collection_type_gid)
+        @collection.apply_depositor_metadata(current_user.user_key)
+        @collection.visibility = Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE unless @collection.discoverable?
+        if @collection.save
+          after_create_response
+        else
+          after_create_errors(@collection.errors)
+        end
+      end
 
-        after_create
-        add_members_to_collection unless batch.empty?
-        @collection
+      def create_valkyrie_collection
+        return after_create_errors(form_err_msg(form)) unless form.validate(collection_params)
+
+        result =
+          transactions['change_set.create_collection']
+          .with_step_args(
+            'change_set.set_user_as_depositor' => { user: current_user },
+            'change_set.add_to_collections' => { collection_ids: Array(params[:parent_id]) },
+            'collection_resource.apply_collection_type_permissions' => { user: current_user }
+          )
+          .call(form)
+        @collection = result.value_or { return after_create_errors(result.failure.first) }
+        after_create_response
       end
 
       def valkyrie_update
@@ -247,6 +240,10 @@ def valkyrie_destroy
         end
       end
 
+      def form_err_msg(form)
+        form.errors.messages.values.flatten.to_sentence
+      end
+
       def default_collection_type
         Hyrax::CollectionType.find_or_create_default_collection_type
       end
@@ -560,6 +557,32 @@ def verify_linkurl(linkurl)
       def valid_url?(url)
         (url =~ URI.regexp(['http', 'https']))
       end
+
+      def after_create_response
+        if @collection.is_a?(ActiveFedora::Base)
+          form
+          set_default_permissions
+          # if we are creating the new collection as a subcollection (via the nested collections controller),
+          # we pass the parent_id through a hidden field in the form and link the two after the create.
+          link_parent_collection(params[:parent_id]) unless params[:parent_id].nil?
+        end
+        respond_to do |format|
+          Hyrax::SolrService.commit
+          format.html { redirect_to edit_dashboard_collection_path(@collection), notice: t('hyrax.dashboard.my.action.collection_create_success') }
+          format.json { render json: @collection, status: :created, location: dashboard_collection_path(@collection) }
+        end
+        add_members_to_collection unless batch.empty?
+      end
+
+      def after_create_errors(errors)
+        respond_to do |wants|
+          wants.html do
+            flash[:error] = errors.to_s
+            render 'new', status: :unprocessable_entity
+          end
+          wants.json { render_json_response(response_type: :unprocessable_entity, options: { errors: errors }) }
+        end
+      end
     end
   end
 end

spec/controllers/hyrax/dashboard/collections_controller_spec.rb

@@ -199,7 +199,7 @@
       it "renders the form again" do
         post :create, params: { collection: collection_attrs }
 
-        expect(response).to be_successful
+        expect(response).to have_http_status(:unprocessable_entity)
         expect(response).to render_template(:new)
       end
     end

spec/controllers/hyrax/dashboard/collections_controller_with_resource_spec.rb

@@ -170,22 +170,45 @@
     end
 
     context "when create fails" do
-      before do
-        allow(controller).to receive(:authorize!)
-        allow(Hyrax::PcdmCollection).to receive(:new).and_return(collection)
-        allow(Hyrax.persister)
-          .to receive(:save)
-          .with(resource: collection)
-          .and_raise(StandardError, 'Failed to save collection')
+      context "in transaction processing" do
+        before do
+          allow(controller).to receive(:authorize!)
+          allow(Hyrax::PcdmCollection).to receive(:new).and_return(collection)
+          allow(Hyrax.persister)
+            .to receive(:save)
+            .with(resource: collection)
+            .and_raise(StandardError, 'Failed to save collection')
+        end
+
+        let(:collection) { Hyrax::PcdmCollection.new }
+
+        it "renders the form again" do
+          post :create, params: { collection: collection_attrs }
+
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(flash[:error]).to match(/Failed to save collection/)
+          expect(response).to render_template(:new)
+        end
       end
 
-      let(:collection) { Hyrax::PcdmCollection.new }
+      context "in validations" do
+        let(:form) { instance_double(Hyrax::Forms::PcdmCollectionForm, errors: errors) }
+        let(:errors) { instance_double(Reform::Contract::CustomError, messages: messages) }
+        let(:messages) { { "error" => "Validation error" } }
+        before do
+          allow(controller).to receive(:authorize!)
+          allow(Hyrax::Forms::ResourceForm).to receive(:for).with(collection).and_return(form)
+          allow(form).to receive(:validate).with(any_args).and_return(false)
+        end
 
-      it "renders the form again" do
-        post :create, params: { collection: collection_attrs }
+        let(:collection) { Hyrax::PcdmCollection.new }
 
-        expect(response).to be_successful
-        expect(response).to render_template(:new)
+        it "renders the form again" do
+          post :create, params: { collection: collection_attrs }
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(flash[:error]).to eq "Validation error"
+          expect(response).to render_template(:new)
+        end
       end
     end
   end