From e269d7f6f347b24e290e4e26b81cea40ff5a4b5a Mon Sep 17 00:00:00 2001 From: buzzromain <18685603+buzzromain@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:59:57 +0200 Subject: [PATCH] fix(goals): let a months-of-expenses reserve be created at all (#3229) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(goals): let a months-of-expenses reserve be created at all The mode could not be used from the UI. The form makes the amount field read-only in months mode — correctly, since the figure is derived — so the form submits it empty. `target_amount` is required and positive, and validations run before every save callback, so the derivation that fills it never got the chance. Creation came back 422 with "can't be blank" on a field the user is not allowed to type in. Reproduced through the controller before changing anything: response 422, no goal created. Every existing test set `target_amount` explicitly, which is why the model looked healthy — the gap was entirely on the path a user actually takes. The derivation moves to `before_validation`, where a derived value belongs: it is computed, then validated like any other. The dirty-state predicates change with it, since `will_save_change_to_*` describes a save that has not been decided on yet at that point. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(goals): move the new tests out of the private section Review flagged them as never running. They do — Rails' `test` macro goes through `define_method` from a class method, which defines a public method whatever the surrounding visibility, and `-n` confirms Minitest picks both up. Verified before touching anything: 2 runs, 7 assertions. Moved anyway. My insertion targeted the file's last `private` rather than its first, so they landed among the helper methods, where they read as a mistake whether or not they behave like one — three separate reviewers have now stopped on it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(goals): put the page tests where nobody has to check they run Review on #3229. The four page tests sat between `private` and a later `public`, and every reader so far has stopped to work out whether they run. They do — Rails' `test` macro calls `define_method` from a class method, and a method defined that way is public whatever the surrounding visibility — but a test whose behaviour has to be reasoned about is a test nobody trusts. They move above the first `private`, where the question does not come up. The second `private` goes with them: everything between it and the first was already private, so it did nothing. The fixed-amount test also gained the assertion it was missing. It named the guard it was protecting and then checked only the status, so a 422 arriving for any other reason would have kept it green. It now asserts the error the form actually puts in front of the user; flipping that paragraph's condition makes it fail, which is the point. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 --- app/models/goal.rb | 26 ++-- test/controllers/goals_controller_test.rb | 167 ++++++++++++++-------- 2 files changed, 120 insertions(+), 73 deletions(-) diff --git a/app/models/goal.rb b/app/models/goal.rb index c5e42e308..fb3d2f8a9 100644 --- a/app/models/goal.rb +++ b/app/models/goal.rb @@ -46,15 +46,21 @@ class Goal < ApplicationRecord # A target_amount edit is in the list because in this mode the amount is # derived, not typed: without it the form could persist an arbitrary figure # under a "six months of expenses" label until the next monthly refresh. - before_save :apply_months_of_expenses_target, - if: -> { - months_of_expenses_target? && ( - new_record? || - will_save_change_to_target_months? || - will_save_change_to_target_mode? || - will_save_change_to_target_amount? - ) - } + # before_VALIDATION, not before_save. `target_amount` is required and must be + # positive, and those run before any save callback — so a reserve whose + # amount is derived arrived at validation empty and was refused before the + # callback that fills it ever ran. The form makes the field read-only in this + # mode, so there was no way to satisfy the validation by hand either: the + # months mode could not be created from the UI at all. + before_validation :apply_months_of_expenses_target, + if: -> { + months_of_expenses_target? && ( + new_record? || + target_months_changed? || + target_mode_changed? || + target_amount_changed? + ) + } validate :must_have_at_least_one_linked_account validate :linked_accounts_must_be_fundable @@ -1175,7 +1181,7 @@ class Goal < ApplicationRecord # the reserve already had. Same reasoning as the refresh job — a stale # floor beats a wrong one, and this one would be wearing a label saying # it was computed. - self.target_amount = target_amount_in_database if will_save_change_to_target_amount? && !new_record? + self.target_amount = target_amount_in_database if target_amount_changed? && !new_record? end # target_months only means something for a reserve on the months basis. diff --git a/test/controllers/goals_controller_test.rb b/test/controllers/goals_controller_test.rb index fea620785..750e11e38 100644 --- a/test/controllers/goals_controller_test.rb +++ b/test/controllers/goals_controller_test.rb @@ -679,6 +679,110 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest assert_equal I18n.t("goals.consume.errors.transaction_not_found"), flash[:alert] end + # The months mode could not be created from the UI at all. The form makes the + # amount read-only, so it submits empty; `target_amount` is required and + # validations run before any save callback, so the derivation that fills it + # never ran. Every existing test set the amount explicitly, which is why the + # model looked fine. + test "a months-of-expenses reserve can be created without typing an amount" do + account = unclaimed_account("Reserve Pot") + IncomeStatement.any_instance.stubs(:median_expense).returns(500) + + assert_difference -> { Goal.count }, 1 do + post goals_url, params: { goal: { + name: "Precaution", color: "#4da568", kind: "maintained", + target_mode: "months_of_expenses", target_months: "6", + target_amount: "", account_ids: [ account.id ] + } } + end + + goal = Goal.order(created_at: :desc).first + assert_equal 3_000, goal.target_amount.to_d, "the floor was not derived from the months" + assert goal.maintained? + end + + # A fixed reserve still needs one, and still says so. Asserted on the error the + # form puts in front of the user, not on the status alone: a 422 for some + # unrelated reason would otherwise keep this green while the guard it names + # had quietly gone. + test "a fixed-amount goal still requires the amount" do + account = unclaimed_account("Fixed Pot") + + assert_no_difference -> { Goal.count } do + post goals_url, params: { goal: { + name: "No amount", color: "#4da568", + target_amount: "", account_ids: [ account.id ] + } } + end + + assert_response :unprocessable_entity + # The paragraph is in the markup either way; `hidden` is what decides + # whether it is shown, so its absence is the assertion. + assert_select "[data-goal-form-target=amountError]:not(.hidden)", 1, + "the form did not surface the missing-amount error" + end + + # The surface the user actually reads: the figure beside the ring, and the + # line saying part of it has been used. Asserted on the rendered page rather + # than the model, because the contradiction was a display bug — the model + # has always counted both halves. + test "the goal page reports the used portion as part of the total" do + goal = spent_goal_for_display + + get goal_url(goal) + + assert_response :success + assert_includes response.body, I18n.t("goals.show.ring.including_used", + amount: goal.consumed_amount_money.format(precision: 0)) + + # The headline figure specifically, not "somewhere on the page": the + # account balance still appears in the funding breakdown below, which is + # where that question belongs. + headline = css_select("p.text-xl.font-medium.text-primary").map(&:text).map(&:strip) + assert_includes headline, goal.progress_amount_money.format(precision: 0) + assert_not_includes headline, goal.current_balance_money.format(precision: 0) + end + + test "a goal that has spent nothing says nothing about it" do + account = unclaimed_account("Quiet Pot") + goal = @user.family.goals.create!(name: "Quiet", target_amount: 1_000, currency: "USD") do |g| + g.goal_accounts.build(account: account, allocated_amount: 1_000) + end + + get goal_url(goal) + + assert_response :success + assert_no_match(/already used|déjà utilisés/, response.body) + end + + + # The ring announced the account balance while the visible headline beside it + # reported the progress total: same ring, two different numbers depending on + # whether you could see it. + test "the ring announces the same total the headline shows" do + goal = spent_goal_for_display + + get goal_url(goal) + + assert_response :success + label = css_select("[role=progressbar]").first["aria-label"] + assert_includes label, goal.progress_amount_money.format + assert_not_includes label, goal.current_balance_money.format + assert_includes label, goal.consumed_amount_money.format + end + + test "a goal with nothing used keeps the plain wording" do + account = unclaimed_account("Plain Pot") + goal = @user.family.goals.create!(name: "Plain", target_amount: 1_000, currency: "USD") do |g| + g.goal_accounts.build(account: account, allocated_amount: 1_000) + end + + get goal_url(goal) + + label = css_select("[role=progressbar]").first["aria-label"] + assert_no_match(/already used|déjà utilisés/, label) + end + private def spent_goal_for_display @@ -707,69 +811,6 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest end - # The surface the user actually reads: the figure beside the ring, and the - # line saying part of it has been used. Asserted on the rendered page rather - # than the model, because the contradiction was a display bug — the model - # has always counted both halves. - test "the goal page reports the used portion as part of the total" do - goal = spent_goal_for_display - - get goal_url(goal) - - assert_response :success - assert_includes response.body, I18n.t("goals.show.ring.including_used", - amount: goal.consumed_amount_money.format(precision: 0)) - - # The headline figure specifically, not "somewhere on the page": the - # account balance still appears in the funding breakdown below, which is - # where that question belongs. - headline = css_select("p.text-xl.font-medium.text-primary").map(&:text).map(&:strip) - assert_includes headline, goal.progress_amount_money.format(precision: 0) - assert_not_includes headline, goal.current_balance_money.format(precision: 0) - end - - test "a goal that has spent nothing says nothing about it" do - account = unclaimed_account("Quiet Pot") - goal = @user.family.goals.create!(name: "Quiet", target_amount: 1_000, currency: "USD") do |g| - g.goal_accounts.build(account: account, allocated_amount: 1_000) - end - - get goal_url(goal) - - assert_response :success - assert_no_match(/already used|déjà utilisés/, response.body) - end - - - # The ring announced the account balance while the visible headline beside it - # reported the progress total: same ring, two different numbers depending on - # whether you could see it. - test "the ring announces the same total the headline shows" do - goal = spent_goal_for_display - - get goal_url(goal) - - assert_response :success - label = css_select("[role=progressbar]").first["aria-label"] - assert_includes label, goal.progress_amount_money.format - assert_not_includes label, goal.current_balance_money.format - assert_includes label, goal.consumed_amount_money.format - end - - test "a goal with nothing used keeps the plain wording" do - account = unclaimed_account("Plain Pot") - goal = @user.family.goals.create!(name: "Plain", target_amount: 1_000, currency: "USD") do |g| - g.goal_accounts.build(account: account, allocated_amount: 1_000) - end - - get goal_url(goal) - - label = css_select("[role=progressbar]").first["aria-label"] - assert_no_match(/already used|déjà utilisés/, label) - end - - private - # A private account of another member, linked to the goal under test. def private_linked_account account = Account.create!(