From c526e72539cfda13a439ce0fd8a1f593af501b11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <2887858+deivid-rodriguez@users.noreply.github.com> Date: Thu, 23 Oct 2025 15:39:01 +0200 Subject: [PATCH] Improve enterprise removal (failure case) Make sure failure to delete due to dependent objects is handled through activemodel errors and not by rescuing `ActiveRecord::DeleteRestrictionError` exceptions. Previously we would display two alert prompts, and we would weirdly display the content of our 500 error page on top of the screen. Now, we display a flash error message explaining the reason to fail to remove it. --- .../admin/enterprises_controller.rb | 7 ++-- app/models/enterprise.rb | 10 +++--- spec/models/enterprise_spec.rb | 35 +++++++++++-------- spec/system/admin/enterprises_spec.rb | 22 ++++++++++++ 4 files changed, 52 insertions(+), 22 deletions(-) diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index f9e8fedcf9..de96364445 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -163,8 +163,11 @@ module Admin end def destroy - @object.destroy - flash.now[:success] = flash_message_for(@object, :successfully_removed) + if @object.destroy + flash.now[:success] = flash_message_for(@object, :successfully_removed) + else + flash.now[:error] = @object.errors.full_messages.to_sentence + end respond_to do |format| format.turbo_stream { render :destroy, status: :ok } diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index b36c3cb33e..667b914512 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -50,11 +50,11 @@ class Enterprise < ApplicationRecord has_many :distributed_orders, class_name: 'Spree::Order', foreign_key: 'distributor_id', inverse_of: :distributor, - dependent: :restrict_with_exception + dependent: :restrict_with_error belongs_to :address, class_name: 'Spree::Address' belongs_to :business_address, optional: true, class_name: 'Spree::Address', dependent: :destroy - has_many :enterprise_fees, dependent: :restrict_with_exception + has_many :enterprise_fees, dependent: :restrict_with_error has_many :enterprise_roles, dependent: :destroy has_many :users, through: :enterprise_roles belongs_to :owner, class_name: 'Spree::User', @@ -62,18 +62,18 @@ class Enterprise < ApplicationRecord has_many :distributor_payment_methods, inverse_of: :distributor, foreign_key: :distributor_id, - dependent: :restrict_with_exception + dependent: :restrict_with_error has_many :distributor_shipping_methods, inverse_of: :distributor, foreign_key: :distributor_id, - dependent: :restrict_with_exception + dependent: :restrict_with_error has_many :payment_methods, through: :distributor_payment_methods has_many :shipping_methods, through: :distributor_shipping_methods has_many :customers, dependent: :destroy has_many :inventory_items, dependent: :destroy has_many :tag_rules, dependent: :destroy has_one :stripe_account, dependent: :destroy - has_many :vouchers, dependent: :restrict_with_exception + has_many :vouchers, dependent: :restrict_with_error has_many :connected_apps, dependent: :destroy has_many :dfc_permissions, dependent: :destroy has_one :custom_tab, dependent: :destroy diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb index 825a7e3941..f53e72b021 100644 --- a/spec/models/enterprise_spec.rb +++ b/spec/models/enterprise_spec.rb @@ -64,9 +64,10 @@ RSpec.describe Enterprise do expect do enterprise.destroy - end.to raise_error(ActiveRecord::DeleteRestrictionError, - /Cannot delete record because of dependent distributed_orders/) - .and change { Spree::Order.count }.by(0) + expect(enterprise.errors.full_messages).to eq( + ["Cannot delete record because dependent distributed orders exist"] + ) + end.to change { Spree::Order.count }.by(0) end it "does not destroy distributor_payment_methods upon destroy" do @@ -75,9 +76,10 @@ RSpec.describe Enterprise do expect do enterprise.destroy - end.to raise_error(ActiveRecord::DeleteRestrictionError, - /Cannot delete record because of dependent distributor_payment_methods/) - .and change { DistributorPaymentMethod.count }.by(0) + expect(enterprise.errors.full_messages).to eq( + ["Cannot delete record because dependent distributor payment methods exist"] + ) + end.to change { Spree::Order.count }.by(0) end it "does not destroy distributor_shipping_methods upon destroy" do @@ -86,9 +88,10 @@ RSpec.describe Enterprise do expect do enterprise.destroy - end.to raise_error(ActiveRecord::DeleteRestrictionError, - /Cannot delete record because of dependent distributor_shipping_methods/) - .and change { DistributorShippingMethod.count }.by(0) + expect(enterprise.errors.full_messages).to eq( + ["Cannot delete record because dependent distributor shipping methods exist"] + ) + end.to change { Spree::Order.count }.by(0) end it "does not destroy enterprise_fees upon destroy" do @@ -97,9 +100,10 @@ RSpec.describe Enterprise do expect do enterprise.destroy - end.to raise_error(ActiveRecord::DeleteRestrictionError, - /Cannot delete record because of dependent enterprise_fees/) - .and change { EnterpriseFee.count }.by(0) + expect(enterprise.errors.full_messages).to eq( + ["Cannot delete record because dependent enterprise fees exist"] + ) + end.to change { Spree::Order.count }.by(0) end it "does not destroy vouchers upon destroy" do @@ -110,9 +114,10 @@ RSpec.describe Enterprise do expect do enterprise.destroy - end.to raise_error(ActiveRecord::DeleteRestrictionError, - /Cannot delete record because of dependent vouchers/) - .and change { Voucher.count }.by(0) + expect(enterprise.errors.full_messages).to eq( + ["Cannot delete record because dependent vouchers exist"] + ) + end.to change { Spree::Order.count }.by(0) end describe "relationships to other enterprises" do diff --git a/spec/system/admin/enterprises_spec.rb b/spec/system/admin/enterprises_spec.rb index bcca16ebf4..08dc187592 100644 --- a/spec/system/admin/enterprises_spec.rb +++ b/spec/system/admin/enterprises_spec.rb @@ -88,6 +88,28 @@ RSpec.describe ' end.to change{ Enterprise.count }.by(-1) end + it "deleting an existing enterprise unsuccessfully" do + enterprise = create(:enterprise) + create(:order, distributor: enterprise) + + user = create(:user) + + admin = login_as_admin + + visit '/admin/enterprises' + + expect do + accept_alert do + within "tr.enterprise-#{enterprise.id}" do + first("a", text: 'Delete').click + end + end + + expect(page).to have_content("Cannot delete record because dependent distributed order") + expect(page).to have_content(enterprise.name) + end.to change{ Enterprise.count }.by(0) + end + it "editing an existing enterprise" do @enterprise = create(:enterprise) e2 = create(:enterprise)