Merge pull request #5610 from samvera/val_col_update

Improve error handling for updating valkyrie collections

app/controllers/hyrax/dashboard/collections_controller.rb

@@ -109,37 +109,21 @@ def create
       end
 
       def after_update
-        respond_to do |format|
-          format.html { redirect_to update_referer, notice: t('hyrax.dashboard.my.action.collection_update_success') }
-          format.json { render json: @collection, status: :updated, location: dashboard_collection_path(@collection) }
-        end
+        Deprecation.warn("Method `#after_update` will be removed in Hyrax 4.0.")
+        after_update_response # call private method for processing
       end
 
       def after_update_error
-        form
-        respond_to do |format|
-          format.html { render action: 'edit' }
-          format.json { render json: @collection.errors, status: :unprocessable_entity }
-        end
+        Deprecation.warn("Method `#after_update_error` will be removed in Hyrax 4.0.")
+        after_update_errors(@collection.errors) # call private method for processing
       end
 
       def update
-        unless params[:update_collection].nil?
-          process_banner_input
-          process_logo_input
-        end
-
-        process_member_changes
-
-        return valkyrie_update if @collection.is_a?(Valkyrie::Resource)
-
-        @collection.visibility = Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE unless @collection.discoverable?
-        # we don't have to reindex the full graph when updating collection
-        @collection.reindex_extent = Hyrax::Adapters::NestingIndexAdapter::LIMITED_REINDEX
-        if @collection.update(collection_params.except(:members))
-          after_update
+        case @collection
+        when ActiveFedora::Base
+          update_active_fedora_collection
         else
-          after_update_error
+          update_valkyrie_collection
         end
       end
 
@@ -224,12 +208,39 @@ def create_valkyrie_collection
         after_create_response
       end
 
-      def valkyrie_update
-        form.validate(collection_params) &&
-          @collection = transactions['change_set.update_collection']
-                        .call(form)
-                        .value_or { return after_update_error }
-        after_update
+      def update_active_fedora_collection
+        unless params[:update_collection].nil?
+          process_banner_input
+          process_logo_input
+        end
+
+        process_member_changes
+
+        @collection.visibility = Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE unless @collection.discoverable?
+        # we don't have to reindex the full graph when updating collection
+        @collection.reindex_extent = Hyrax::Adapters::NestingIndexAdapter::LIMITED_REINDEX
+        if @collection.update(collection_params.except(:members))
+          after_update_response
+        else
+          after_update_errors(@collection.errors)
+        end
+      end
+
+      def update_valkyrie_collection
+        return after_update_errors(form_err_msg(form)) unless form.validate(collection_params)
+
+        result = transactions['change_set.update_collection']
+                 .call(form)
+        @collection = result.value_or { return after_update_errors(result.failure.first) }
+
+        process_member_changes
+
+        unless params[:update_collection].nil?
+          process_banner_input
+          process_logo_input
+        end
+
+        after_update_response
       end
 
       def valkyrie_destroy
@@ -574,12 +585,55 @@ def after_create_response
         add_members_to_collection unless batch.empty?
       end
 
-      def after_create_errors(errors)
+      def after_create_errors_for_active_fedora(errors)
+        form
+        respond_to do |format|
+          format.html do
+            flash[:error] = errors.to_s
+            render action: 'new'
+          end
+          format.json { render json: @collection.errors, status: :unprocessable_entity }
+        end
+      end
+
+      def after_create_errors(errors) # for valkyrie
+        return after_create_errors_for_active_fedora(errors) if @collection.is_a? ActiveFedora::Base
         respond_to do |wants|
           wants.html do
             flash[:error] = errors.to_s
             render 'new', status: :unprocessable_entity
           end
+          wants.json do
+            render_json_response(response_type: :unprocessable_entity, options: { errors: errors })
+          end
+        end
+      end
+
+      def after_update_response
+        respond_to do |format|
+          format.html { redirect_to update_referer, notice: t('hyrax.dashboard.my.action.collection_update_success') }
+          format.json { render json: @collection, status: :updated, location: dashboard_collection_path(@collection) }
+        end
+      end
+
+      def after_update_errors_for_active_fedora(errors)
+        form
+        respond_to do |format|
+          format.html do
+            flash[:error] = errors.to_s
+            render action: 'edit'
+          end
+          format.json { render json: @collection.errors, status: :unprocessable_entity }
+        end
+      end
+
+      def after_update_errors(errors) # for valkyrie
+        return after_update_errors_for_active_fedora(errors) if @collection.is_a? ActiveFedora::Base
+        respond_to do |wants|
+          wants.html do
+            flash[:error] = errors.to_s
+            render 'edit', status: :unprocessable_entity
+          end
           wants.json { render_json_response(response_type: :unprocessable_entity, options: { errors: errors }) }
         end
       end

spec/controllers/hyrax/dashboard/collections_controller_spec.rb

@@ -184,23 +184,29 @@
 
     context "when create fails" do
       let(:collection) { Collection.new }
+      let(:error) { "Failed to save collection" }
 
       before do
         allow(controller).to receive(:authorize!)
         allow(Collection).to receive(:new).and_return(collection)
         allow(collection).to receive(:save).and_return(false)
-
-        allow(Hyrax.persister)
-          .to receive(:save)
-          .with(any_args)
-          .and_raise(StandardError, 'Failed to save collection')
+        allow(collection).to receive(:errors).and_return(error)
       end
 
       it "renders the form again" do
         post :create, params: { collection: collection_attrs }
 
-        expect(response).to have_http_status(:unprocessable_entity)
+        expect(response).to have_http_status(:ok)
         expect(response).to render_template(:new)
+        expect(flash[:error]).to eq error
+      end
+
+      it "renders json" do
+        post :create, params: { collection: collection_attrs, format: :json }
+
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(response.content_type).to eq "application/json"
+        expect(response.body).to eq error
       end
     end
   end
@@ -339,29 +345,39 @@
     end
 
     context "when update fails" do
-      let(:collection) { FactoryBot.valkyrie_create(:hyrax_collection) }
+      let(:collection) { FactoryBot.create(:collection_lw) }
       let(:repository) { instance_double(Blacklight::Solr::Repository, search: result) }
       let(:result) { double(documents: [], total: 0) }
+      let(:error) { "Failed to save collection" }
 
       before do
         allow(controller).to receive(:authorize!)
         allow(Collection).to receive(:find).and_return(collection)
         allow(collection).to receive(:update).and_return(false)
         allow(controller).to receive(:repository).and_return(repository)
-        allow(Hyrax.persister)
-          .to receive(:save)
-          .with(any_args)
-          .and_raise(StandardError, 'Failed to save collection')
+        allow_any_instance_of(::Collection).to receive(:save).and_return(false)
+        allow(collection).to receive(:errors).and_return(error)
       end
 
       it "renders the form again" do
         put :update, params: {
           id: collection,
           collection: collection_attrs
         }
-        expect(response).to be_successful
+        expect(response).to have_http_status(:ok)
         expect(response).to render_template(:edit)
       end
+
+      it "renders json" do
+        put :update, params: {
+          id: collection,
+          collection: collection_attrs,
+          format: :json
+        }
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(response.content_type).to eq "application/json"
+        expect(response.body).to eq error
+      end
     end
 
     context "updating a collections branding metadata" do

spec/controllers/hyrax/dashboard/collections_controller_with_resource_spec.rb

@@ -189,12 +189,26 @@
           expect(flash[:error]).to match(/Failed to save collection/)
           expect(response).to render_template(:new)
         end
+
+        it "renders json" do
+          post :create, params: { collection: collection_attrs, format: :json }
+
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(response.content_type).to eq "application/json"
+
+          json_response = JSON.parse(response.body)
+          expect(json_response["code"]).to eq 422
+          expect(json_response["message"]).to eq "Unprocessable Entity"
+          expect(json_response["description"]).to eq "The resource you attempted to modify cannot be modified according to your request."
+          expect(json_response["errors"]).to match(/Failed to save collection/)
+        end
       end
 
       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" } }
+        let(:messages) { { "error" => errmsg } }
+        let(:errmsg) { "Validation error" }
         before do
           allow(controller).to receive(:authorize!)
           allow(Hyrax::Forms::ResourceForm).to receive(:for).with(collection).and_return(form)
@@ -205,10 +219,24 @@
 
         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(flash[:error]).to eq errmsg
           expect(response).to render_template(:new)
         end
+
+        it "renders json" do
+          post :create, params: { collection: collection_attrs, format: :json }
+
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(response.content_type).to eq "application/json"
+
+          json_response = JSON.parse(response.body)
+          expect(json_response["code"]).to eq 422
+          expect(json_response["message"]).to eq "Unprocessable Entity"
+          expect(json_response["description"]).to eq "The resource you attempted to modify cannot be modified according to your request."
+          expect(json_response["errors"]).to eq errmsg
+        end
       end
     end
   end
@@ -360,20 +388,76 @@
 
     context "when update fails" do
       before do
+        allow(controller).to receive(:authorize!)
         collection # ensure the collection is loaded before we stub the persister save
-        allow(Hyrax.persister)
-          .to receive(:save)
-          .with(any_args)
-          .and_raise(StandardError, 'Failed to save collection')
       end
 
-      it "renders the form again" do
-        put :update, params: {
-          id: collection,
-          collection: collection_attrs
-        }
-        expect(response).to be_successful
-        expect(response).to render_template(:edit)
+      context "in transaction processing" do
+        before do
+          allow(Hyrax.persister)
+            .to receive(:save)
+            .with(any_args)
+            .and_raise(StandardError, 'Failed to save collection')
+        end
+
+        it "renders the form again" do
+          put :update, params: { id: collection, 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(:edit)
+        end
+
+        it "renders json" do
+          put :update, params: {
+            id: collection,
+            collection: collection_attrs,
+            format: :json
+          }
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(response.content_type).to eq "application/json"
+
+          json_response = JSON.parse(response.body)
+          expect(json_response["code"]).to eq 422
+          expect(json_response["message"]).to eq "Unprocessable Entity"
+          expect(json_response["description"]).to eq "The resource you attempted to modify cannot be modified according to your request."
+          expect(json_response["errors"]).to match(/Failed to save collection/)
+        end
+      end
+
+      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" => errmsg } }
+        let(:errmsg) { "Validation error" }
+        before do
+          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
+          put :update, params: { id: collection, collection: collection_attrs }
+
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(flash[:error]).to eq errmsg
+          expect(response).to render_template(:edit)
+        end
+
+        it "renders json" do
+          put :update, params: {
+            id: collection,
+            collection: collection_attrs,
+            format: :json
+          }
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(response.content_type).to eq "application/json"
+
+          json_response = JSON.parse(response.body)
+          expect(json_response["code"]).to eq 422
+          expect(json_response["message"]).to eq "Unprocessable Entity"
+          expect(json_response["description"]).to eq "The resource you attempted to modify cannot be modified according to your request."
+          expect(json_response["errors"]).to eq errmsg
+        end
       end
     end