From a0942f66bfdd8f46646ab06c7b03bab76dad9c60 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 23 May 2021 23:57:28 +0200
Subject: [PATCH] Fix crash destroying budgets with admins/valuators

We don't allow deleting a budget with associated investments. However,
we allow deleting a budget with associated administrators and valuators.
This results in a foreign key violation error:

PG::ForeignKeyViolation: ERROR:  update or delete on table "budgets"
violates foreign key constraint "fk_rails_c847a52b1d" on table
"budget_administrators"

Using the `dependent: :destroy` option when defining the relationship,
we remove the association records when removing the budget.

As a bonus, we reduce the number of Rubocop offenses regarding the
`Rails/HasManyOrHasOneDependent` rule. Only 72 to go! :)
---
 app/models/budget.rb              |  4 ++--
 spec/models/budget_spec.rb        | 22 ++++++++++++++++++++++
 spec/system/admin/budgets_spec.rb | 12 ++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/app/models/budget.rb b/app/models/budget.rb
index d6034c896..c667ada15 100644
--- a/app/models/budget.rb
+++ b/app/models/budget.rb
@@ -34,9 +34,9 @@ class Budget < ApplicationRecord
   has_many :headings, through: :groups
   has_many :lines, through: :ballots, class_name: "Budget::Ballot::Line"
   has_many :phases, class_name: "Budget::Phase"
-  has_many :budget_administrators
+  has_many :budget_administrators, dependent: :destroy
   has_many :administrators, through: :budget_administrators
-  has_many :budget_valuators
+  has_many :budget_valuators, dependent: :destroy
   has_many :valuators, through: :budget_valuators
 
   has_one :poll
diff --git a/spec/models/budget_spec.rb b/spec/models/budget_spec.rb
index 9f734f452..e82485cb7 100644
--- a/spec/models/budget_spec.rb
+++ b/spec/models/budget_spec.rb
@@ -402,4 +402,26 @@ describe Budget do
       end
     end
   end
+
+  describe "#budget_administrators" do
+    it "destroys relation with administrators when destroying the budget" do
+      budget = create(:budget, administrators: [create(:administrator)])
+
+      budget.destroy!
+
+      expect(BudgetAdministrator.count).to be 0
+      expect(Administrator.count).to be 1
+    end
+  end
+
+  describe "#budget_valuators" do
+    it "destroys relation with valuators when destroying the budget" do
+      budget = create(:budget, valuators: [create(:valuator)])
+
+      budget.destroy!
+
+      expect(BudgetValuator.count).to be 0
+      expect(Valuator.count).to be 1
+    end
+  end
 end
diff --git a/spec/system/admin/budgets_spec.rb b/spec/system/admin/budgets_spec.rb
index e6c22ed6b..e6a5ab6a4 100644
--- a/spec/system/admin/budgets_spec.rb
+++ b/spec/system/admin/budgets_spec.rb
@@ -205,6 +205,18 @@ describe "Admin budgets", :admin do
       expect(page).to have_content("There are no budgets.")
     end
 
+    scenario "Destroy a budget without investments but with administrators and valuators" do
+      budget.administrators << Administrator.first
+      budget.valuators << create(:valuator)
+
+      visit admin_budgets_path
+      click_link "Edit budget"
+      click_link "Delete budget"
+
+      expect(page).to have_content "Budget deleted successfully"
+      expect(page).to have_content "There are no budgets."
+    end
+
     scenario "Try to destroy a budget with investments" do
       create(:budget_investment, heading: heading)