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!(