QT/sure
1
0
Fork 0
mirror of https://github.com/we-promise/sure.git synced 2026-09-05 14:51:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(goals): let a months-of-expenses reserve be created at all (#3229)

Browse Source 
* 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
buzzromain
2026-08-28 06:59:57 +02:00
committed by GitHub
co-authored by Claude Opus 5
parent e7619cac89
commit e269d7f6f3
2 changed files with 120 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/models/goal.rb
+16 -10
View File
@@ -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.
test/controllers/goals_controller_test.rb
+104 -63
View File
@@ -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!(