From ec1562782b27dee1ea24cd8d4eddb7e347727139 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Apr 2026 16:41:45 +0200 Subject: [PATCH] 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> --- .../budget_categories_controller.rb | 22 ++--- app/models/budget_category.rb | 50 +++++++--- .../_budget_category_form.html.erb | 1 - .../budget_categories/update.turbo_stream.erb | 8 +- .../budget_categories_controller_test.rb | 97 +++++++++++++++++++ test/models/budget_category_test.rb | 64 ++++++------ 6 files changed, 176 insertions(+), 66 deletions(-) create mode 100644 test/controllers/budget_categories_controller_test.rb diff --git a/app/controllers/budget_categories_controller.rb b/app/controllers/budget_categories_controller.rb index b779a0224..0490a5f61 100644 --- a/app/controllers/budget_categories_controller.rb +++ b/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 diff --git a/app/models/budget_category.rb b/app/models/budget_category.rb index 85c355861..764e3d744 100644 --- a/app/models/budget_category.rb +++ b/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 diff --git a/app/views/budget_categories/_budget_category_form.html.erb b/app/views/budget_categories/_budget_category_form.html.erb index f4fce9d86..59a564a8b 100644 --- a/app/views/budget_categories/_budget_category_form.html.erb +++ b/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 %> diff --git a/app/views/budget_categories/update.turbo_stream.erb b/app/views/budget_categories/update.turbo_stream.erb index bf44fea3c..0991695cd 100644 --- a/app/views/budget_categories/update.turbo_stream.erb +++ b/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 %> diff --git a/test/controllers/budget_categories_controller_test.rb b/test/controllers/budget_categories_controller_test.rb new file mode 100644 index 000000000..96c6348a5 --- /dev/null +++ b/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 diff --git a/test/models/budget_category_test.rb b/test/models/budget_category_test.rb index 5814fe9d8..d0f0b71a6 100644 --- a/test/models/budget_category_test.rb +++ b/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