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.

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 }

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

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

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)