Make parent budgets auto-aggregate from subcategory edits (#1312)

* Initial plan

* Auto-sum parent budgets from subcategory edits

Agent-Logs-Url: https://github.com/we-promise/sure/sessions/f1c1b9ef-0e5d-4300-8f1b-e40876abfdcd

Co-authored-by: jjmata <187772+jjmata@users.noreply.github.com>

* Finalize subcategory budget parent aggregation

Agent-Logs-Url: https://github.com/we-promise/sure/sessions/f1c1b9ef-0e5d-4300-8f1b-e40876abfdcd

Co-authored-by: jjmata <187772+jjmata@users.noreply.github.com>

* Address follow-up review on budget aggregation

Agent-Logs-Url: https://github.com/we-promise/sure/sessions/b773decd-69a2-4da9-81ed-3be7d24cbb52

Co-authored-by: jjmata <187772+jjmata@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jjmata <187772+jjmata@users.noreply.github.com>

app/controllers/budget_categories_controller.rb

@@ -23,22 +23,22 @@ class BudgetCategoriesController < ApplicationController
 
   def update
     @budget_category = Current.family.budget_categories.find(params[:id])
+    @budget_category.update_budgeted_spending!(budgeted_spending_param)
 
-    if @budget_category.update(budget_category_params)
-      respond_to do |format|
-        format.turbo_stream
-        format.html { redirect_to budget_budget_categories_path(@budget) }
-      end
-    else
-      render :index, status: :unprocessable_entity
+    respond_to do |format|
+      format.turbo_stream
+      format.html { redirect_to budget_budget_categories_path(@budget) }
     end
+  rescue ActiveRecord::RecordInvalid
+    render :index, status: :unprocessable_entity
   end
 
   private
-    def budget_category_params
-      params.require(:budget_category).permit(:budgeted_spending).tap do |params|
-        params[:budgeted_spending] = params[:budgeted_spending].presence || 0
-      end
+    def budgeted_spending_param
+      params.require(:budget_category)
+        .permit(:budgeted_spending)
+        .fetch(:budgeted_spending, nil)
+        .presence || 0
     end
 
     def set_budget

app/models/budget_category.rb

@@ -53,6 +53,17 @@ class BudgetCategory < ApplicationRecord
     budget.budget_category_actual_spending(self)
   end
 
+  def update_budgeted_spending!(new_budgeted_spending)
+    self.class.transaction do
+      lock!
+
+      previous_budgeted_spending = budgeted_spending || 0
+      update!(budgeted_spending: new_budgeted_spending)
+
+      sync_parent_budgeted_spending!(previous_budgeted_spending:) if subcategory?
+    end
+  end
+
   def avg_monthly_expense
     budget.category_avg_monthly_expense(category)
   end
@@ -192,21 +203,6 @@ class BudgetCategory < ApplicationRecord
     budget.budget_categories.select { |bc| bc.category.parent_id == category.parent_id && bc.id != id }
   end
 
-  def max_allocation
-    return nil unless subcategory?
-
-    parent_budget_cat = budget.budget_categories.find { |bc| bc.category.id == category.parent_id }
-    return nil unless parent_budget_cat
-
-    parent_budget = parent_budget_cat[:budgeted_spending] || 0
-
-    # Sum budgets of siblings that have individual limits (excluding those that inherit)
-    siblings_with_limits = siblings.reject(&:inherits_parent_budget?)
-    siblings_budget = siblings_with_limits.sum { |s| s[:budgeted_spending] || 0 }
-
-    [ parent_budget - siblings_budget, 0 ].max
-  end
-
   def subcategories
     return BudgetCategory.none unless category.parent_id.nil?
     return BudgetCategory.none if category.id.nil?
@@ -215,4 +211,28 @@ class BudgetCategory < ApplicationRecord
       .joins(:category)
       .where(categories: { parent_id: category.id })
   end
+
+  private
+    def sync_parent_budgeted_spending!(previous_budgeted_spending:)
+      parent_budget_category = budget.budget_categories.where(category_id: category.parent_id).lock.first
+      return unless parent_budget_category
+
+      sibling_budgeted_spending = budget.budget_categories
+        .joins(:category)
+        .where(categories: { parent_id: category.parent_id })
+        .where.not(id: id)
+        .sum(:budgeted_spending)
+
+      # Preserve positive parent reserve—the extra budget assigned directly to the parent
+      # beyond the sum of its subcategories—but do not carry forward a negative reserve
+      # that would leave the parent below its subcategory total.
+      parent_budget_reserve = [
+        (parent_budget_category.budgeted_spending || 0) - sibling_budgeted_spending - previous_budgeted_spending,
+        0
+      ].max
+
+      parent_budget_category.update!(
+        budgeted_spending: sibling_budgeted_spending + (budgeted_spending || 0) + parent_budget_reserve
+      )
+    end
 end

app/views/budget_categories/_budget_category_form.html.erb

@@ -22,7 +22,6 @@
                             step: currency.step,
                             id: dom_id(budget_category, :budgeted_spending),
                             min: 0,
-                            max: budget_category.max_allocation,
                             data: { auto_submit_form_target: "auto" },
                             title: budget_category.subcategory? ? "Leave empty to share parent's budget" : nil %>
         </div>

app/views/budget_categories/update.turbo_stream.erb

@@ -5,14 +5,18 @@
 <%= turbo_stream.replace dom_id(@budget, :confirm_button), partial: "budget_categories/confirm_button", locals: { budget: @budget } %>
 
 <% if @budget_category.subcategory? %>
+  <% if (parent_budget_category = @budget_category.parent_budget_category) %>
+    <%= turbo_stream.replace dom_id(parent_budget_category, :form), partial: "budget_categories/budget_category_form", locals: { budget_category: parent_budget_category } %>
+  <% end %>
+
   <%# Update sibling subcategories when a subcategory changes %>
   <% @budget_category.siblings.each do |sibling| %>
-    <%= turbo_stream.update dom_id(sibling, :form), partial: "budget_categories/budget_category_form", locals: { budget_category: sibling } %>
+    <%= turbo_stream.replace dom_id(sibling, :form), partial: "budget_categories/budget_category_form", locals: { budget_category: sibling } %>
   <% end %>
 
 <% else %>
   <%# Update all subcategories when a parent category changes %>
   <% @budget_category.subcategories.each do |subcategory| %>
-    <%= turbo_stream.update dom_id(subcategory, :form), partial: "budget_categories/budget_category_form", locals: { budget_category: subcategory } %>
+    <%= turbo_stream.replace dom_id(subcategory, :form), partial: "budget_categories/budget_category_form", locals: { budget_category: subcategory } %>
   <% end %>
 <% end %>

test/controllers/budget_categories_controller_test.rb

@@ -0,0 +1,97 @@
+require "test_helper"
+
+class BudgetCategoriesControllerTest < ActionDispatch::IntegrationTest
+  include ActionView::RecordIdentifier
+
+  setup do
+    sign_in users(:family_admin)
+
+    @budget = budgets(:one)
+    @family = @budget.family
+
+    @parent_category = Category.create!(
+      name: "Bills controller test",
+      family: @family,
+      color: "#4da568",
+      lucide_icon: "house"
+    )
+
+    @electric_category = Category.create!(
+      name: "Electric controller test",
+      parent: @parent_category,
+      family: @family
+    )
+
+    @water_category = Category.create!(
+      name: "Water controller test",
+      parent: @parent_category,
+      family: @family
+    )
+
+    @parent_budget_category = BudgetCategory.create!(
+      budget: @budget,
+      category: @parent_category,
+      budgeted_spending: 500,
+      currency: "USD"
+    )
+
+    @electric_budget_category = BudgetCategory.create!(
+      budget: @budget,
+      category: @electric_category,
+      budgeted_spending: 100,
+      currency: "USD"
+    )
+
+    @water_budget_category = BudgetCategory.create!(
+      budget: @budget,
+      category: @water_category,
+      budgeted_spending: 50,
+      currency: "USD"
+    )
+  end
+
+  test "updating a subcategory adjusts the parent budget by the same delta" do
+    assert_changes -> { @parent_budget_category.reload.budgeted_spending.to_f }, from: 500.0, to: 550.0 do
+      patch budget_budget_category_path(@budget, @electric_budget_category),
+            params: { budget_category: { budgeted_spending: 150 } },
+            as: :turbo_stream
+    end
+
+    assert_response :success
+    assert_includes @response.body, dom_id(@parent_budget_category, :form)
+  end
+
+  test "manual parent budget remains on top of subcategory changes" do
+    @parent_budget_category.update!(budgeted_spending: 900)
+
+    assert_changes -> { @parent_budget_category.reload.budgeted_spending.to_f }, from: 900.0, to: 975.0 do
+      patch budget_budget_category_path(@budget, @water_budget_category),
+            params: { budget_category: { budgeted_spending: 125 } },
+            as: :turbo_stream
+    end
+  end
+
+  test "sibling subcategory budget form rerenders without a max allocation cap" do
+    patch budget_budget_category_path(@budget, @electric_budget_category),
+          params: { budget_category: { budgeted_spending: 125 } },
+          as: :turbo_stream
+
+    assert_response :success
+
+    fragment = Nokogiri::HTML.fragment(@response.body)
+    input = fragment.at_css("input##{dom_id(@water_budget_category, :budgeted_spending)}")
+
+    assert_not_nil input
+    assert_nil input["max"]
+  end
+
+  test "clearing a subcategory budget switches it back to shared and lowers the parent" do
+    assert_changes -> { @parent_budget_category.reload.budgeted_spending.to_f }, from: 500.0, to: 400.0 do
+      patch budget_budget_category_path(@budget, @electric_budget_category),
+            params: { budget_category: { budgeted_spending: "" } },
+            as: :turbo_stream
+    end
+
+    assert_equal 0.0, @electric_budget_category.reload.budgeted_spending.to_f
+  end
+end

test/models/budget_category_test.rb

@@ -87,43 +87,6 @@ class BudgetCategoryTest < ActiveSupport::TestCase
     assert_equal 200, @subcategory_with_limit_bc.available_to_spend
   end
 
-  test "max_allocation excludes budgets of inheriting siblings" do
-    # Create another inheriting subcategory
-    another_inheriting = Category.create!(
-      name: "Test Coffee #{Time.now.to_f}",
-      parent: @parent_category,
-      family: @family
-    )
-
-    another_inheriting_bc = BudgetCategory.create!(
-      budget: @budget,
-      category: another_inheriting,
-      budgeted_spending: 0,  # Inherits
-      currency: "USD"
-    )
-
-    # Max allocation for new subcategory should only account for the one with explicit limit (300)
-    # 1000 (parent) - 300 (subcategory_with_limit) = 700
-    assert_equal 700, another_inheriting_bc.max_allocation
-
-    # If we add a new subcategory with a limit
-    new_subcategory_cat = Category.create!(
-      name: "Test Fast Food #{Time.now.to_f}",
-      parent: @parent_category,
-      family: @family
-    )
-
-    new_subcategory_bc = BudgetCategory.create!(
-      budget: @budget,
-      category: new_subcategory_cat,
-      budgeted_spending: 0,
-      currency: "USD"
-    )
-
-    # Max should still be 700 because both inheriting subcategories don't count
-    assert_equal 700, new_subcategory_bc.max_allocation
-  end
-
   test "percent_of_budget_spent for inheriting subcategory uses parent budget" do
     # Mock spending
     @budget.stubs(:budget_category_actual_spending).with(@subcategory_inheriting_bc).returns(100)
@@ -179,4 +142,31 @@ class BudgetCategoryTest < ActiveSupport::TestCase
     assert_equal 800, @subcategory_with_limit_bc.available_to_spend
     assert_equal 800, @subcategory_inheriting_bc.available_to_spend
   end
+
+  test "update_budgeted_spending! preserves positive parent reserve when subcategory becomes individual" do
+    @subcategory_inheriting_bc.update_budgeted_spending!(200)
+
+    assert_equal 1200, @parent_budget_category.reload.budgeted_spending
+    assert_equal 200, @subcategory_inheriting_bc.reload.budgeted_spending
+    refute @subcategory_inheriting_bc.reload.inherits_parent_budget?
+  end
+
+  test "update_budgeted_spending! lowers parent when subcategory returns to shared" do
+    @subcategory_with_limit_bc.update_budgeted_spending!(0)
+
+    assert_equal 700, @parent_budget_category.reload.budgeted_spending
+    assert @subcategory_with_limit_bc.reload.inherits_parent_budget?
+  end
+
+  test "update_budgeted_spending! does not preserve a negative parent reserve" do
+    # Create an artificial inconsistent parent total to verify recovery behavior.
+    @parent_budget_category.update!(budgeted_spending: 50)
+    @subcategory_inheriting_bc.update!(budgeted_spending: 50)
+
+    @subcategory_with_limit_bc.update_budgeted_spending!(20)
+
+    assert_equal 70, @parent_budget_category.reload.budgeted_spending
+    assert_equal 20, @subcategory_with_limit_bc.reload.budgeted_spending
+    assert_equal 50, @subcategory_inheriting_bc.reload.budgeted_spending
+  end
 end