From e7619cac89162ab128e3dd679938fb6487fce3d4 Mon Sep 17 00:00:00 2001 From: buzzromain <18685603+buzzromain@users.noreply.github.com> Date: Fri, 28 Aug 2026 02:06:13 +0200 Subject: [PATCH] fix(goals): make the figure beside the ring agree with the ring (#3213) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(goals): make the figure beside the ring agree with the ring Recording a spend left the goal page contradicting itself. The ring is drawn from `progress_percent`, which counts money still held plus money already spent on the goal; the figure beside it showed only the first half. A goal that had saved 5,000 and spent 2,000 of it rendered a 100% ring next to "3,000 of 5,000" — two answers on one card, with nothing to say which to believe. The model was never wrong: `progress_percent` and `remaining_amount` have both counted the two halves since the spend feature landed. Only the display took one of them. `progress_amount` names what progress actually counts, and both surfaces now read from it. The amount already used is reported as part of that total rather than beside it — "Including 2,000 already used" — so a reader has nothing to add up and no reason to read a completed goal as a shortfall. "Used" rather than "spent", matching the menu entry the user came through: it is the same gesture, and spending on the thing you saved for is the goal working, not failing. Shown only where there is something to show. The overwhelming majority of goals never record a spend, and a permanent "0 used" line would be noise on every card. A reserve refuses consumption outright, so this never appears on one. What is still sitting in each account keeps its place in the funding breakdown, which is where that question belongs — the view test asserts the headline specifically rather than the whole page, for exactly that reason. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): make the ring announce what it shows Review on #3213. The headline moved to the progress total; the ring's `aria-label` still read `current_balance_money`. A screen reader announced "$3,000 of $5,000 saved" while the line beside it said "$5,000, including $2,000 already used" — the same ring, two different numbers depending on whether you could see it. The wording moves with the figure. "Saved" stops being the whole truth once part of the total has been spent on the goal, so a goal that has recorded one gets the sentence that says so, and every other goal keeps the wording it had. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * style(goals): use the component's t() for the ring's labels Review on #3213. `I18n.t` works, but the component helper is what the rest of the codebase reaches for and it carries the view's locale context. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 --- app/components/goals/card_component.html.erb | 12 ++- .../goals/progress_ring_component.html.erb | 2 +- .../goals/progress_ring_component.rb | 20 ++++- app/models/goal.rb | 28 +++++++ app/views/goals/show.html.erb | 10 ++- config/locales/views/goals/en.yml | 3 + config/locales/views/goals/fr.yml | 3 + test/controllers/goals_controller_test.rb | 74 +++++++++++++++++++ test/models/goal_test.rb | 47 ++++++++++++ 9 files changed, 195 insertions(+), 4 deletions(-) diff --git a/app/components/goals/card_component.html.erb b/app/components/goals/card_component.html.erb index 6c38e7921..f5b10fe28 100644 --- a/app/components/goals/card_component.html.erb +++ b/app/components/goals/card_component.html.erb @@ -29,10 +29,20 @@
+ <%# The figure the ring is drawn from, not the account balance: money still + held plus money already spent on the goal itself. Showing the balance + alone put a 100% ring beside "3,000 of 5,000". %>
- <%= goal.current_balance_money.format(precision: 0) %> + <%= goal.progress_amount_money.format(precision: 0) %> / <%= goal.target_amount_money.format(precision: 0) %>
+ <% if goal.any_consumption? %> + <%# "Including", not a second figure beside the first: the amount is part + of the total above, and a reader should not have to add anything. %> +

+ <%= t("goals.goal_card.including_used", amount: goal.consumed_amount_money.format(precision: 0)) %> +

+ <% end %> <% if pace_line %>

<%= pace_line %>

<% end %> diff --git a/app/components/goals/progress_ring_component.html.erb b/app/components/goals/progress_ring_component.html.erb index 273555103..2e12cd47f 100644 --- a/app/components/goals/progress_ring_component.html.erb +++ b/app/components/goals/progress_ring_component.html.erb @@ -5,7 +5,7 @@ aria-valuenow="<%= percent %>" aria-valuemin="0" aria-valuemax="100" - aria-label="<%= t("goals.show.ring.aria_label", percent: percent, amount: amount_label, target: target_label) %>" + aria-label="<%= aria_label %>" class="relative mx-auto" style="width: <%= size %>px; height: <%= size %>px;">
diff --git a/app/components/goals/progress_ring_component.rb b/app/components/goals/progress_ring_component.rb index edbfbf1e5..f9a4a59db 100644 --- a/app/components/goals/progress_ring_component.rb +++ b/app/components/goals/progress_ring_component.rb @@ -10,8 +10,26 @@ class Goals::ProgressRingComponent < ApplicationComponent goal.progress_percent end + # The same total the headline beside this ring reports, and the same one the + # ring is drawn from. Announcing the account balance while the visible text + # said something else left a screen reader and a sighted reader looking at + # the same ring and hearing two different numbers. def amount_label - goal.current_balance_money.format + goal.progress_amount_money.format + end + + # "saved" stops being the whole truth once part of the total has been spent + # on the goal. The sighted reader gets that from the line under the figure; + # this is the same sentence for someone who cannot see it. + def aria_label + if goal.any_consumption? + t("goals.show.ring.aria_label_with_used", + percent: percent, amount: amount_label, target: target_label, + used: goal.consumed_amount_money.format) + else + t("goals.show.ring.aria_label", + percent: percent, amount: amount_label, target: target_label) + end end def target_label diff --git a/app/models/goal.rb b/app/models/goal.rb index 1628c6d6e..c5e42e308 100644 --- a/app/models/goal.rb +++ b/app/models/goal.rb @@ -601,6 +601,33 @@ class Goal < ApplicationRecord @remaining_amount_money ||= Money.new(remaining_amount, currency) end + # What progress actually counts: money still held for the goal, plus money + # already taken out of it and spent on the thing it was for. + # + # `progress_percent` and `remaining_amount` have always been computed from + # both. Only the headline figure showed the first half, so a goal that had + # spent part of its savings sat at a 100% ring beside "3,000 of 5,000" — the + # ring and the numbers disagreeing on the same card, with nothing to explain + # which one to believe. + def progress_amount + current_balance.to_d + consumed_amount.to_d + end + + def progress_amount_money + @progress_amount_money ||= Money.new(progress_amount, currency) + end + + def consumed_amount_money + @consumed_amount_money ||= Money.new(consumed_amount.to_d, currency) + end + + # Only a goal that has actually recorded a spend says anything about it: the + # overwhelming majority never do, and a permanent "0 used" line would be + # noise on every card. + def any_consumption? + consumed_amount.to_d.positive? + end + def progress_percent return @progress_percent if defined?(@progress_percent) @@ -1182,6 +1209,7 @@ class Goal < ApplicationRecord @current_balance @current_balance_money @remaining_amount @remaining_amount_money @progress_percent @monthly_target_amount + @progress_amount_money @consumed_amount_money @pace @pace_money @status @pooled_allocations ].each do |ivar| remove_instance_variable(ivar) if instance_variable_defined?(ivar) diff --git a/app/views/goals/show.html.erb b/app/views/goals/show.html.erb index ff77c2423..204c8c7e3 100644 --- a/app/views/goals/show.html.erb +++ b/app/views/goals/show.html.erb @@ -133,7 +133,15 @@
<%= render Goals::ProgressRingComponent.new(goal: @goal, size: 180) %> -

<%= @goal.current_balance_money.format(precision: 0) %>

+ <%# The figure the ring is drawn from — held plus already spent on the + goal — so the two agree. What is still sitting in each account is in + the funding breakdown below, which is where that question belongs. %> +

<%= @goal.progress_amount_money.format(precision: 0) %>

+ <% if @goal.any_consumption? %> +

+ <%= t(".ring.including_used", amount: @goal.consumed_amount_money.format(precision: 0)) %> +

+ <% end %> <% if @goal.contributions_basis? %>

<%= t(".ring.market_value", amount: @goal.market_value_money.format(precision: 0)) %>

<% end %> diff --git a/config/locales/views/goals/en.yml b/config/locales/views/goals/en.yml index 00f8a7a19..de0543d71 100644 --- a/config/locales/views/goals/en.yml +++ b/config/locales/views/goals/en.yml @@ -169,12 +169,14 @@ en: target_by: "Target %{amount} by %{date}" target_by_past: "Target %{amount} · was due %{date}" ring: + including_used: "Including %{amount} already used" saved: Saved of: "of %{target}" to_go: "%{amount} to go" of_target: of target market_value: "Market value %{amount}" aria_label: "Goal %{percent}% complete. %{amount} of %{target} saved." + aria_label_with_used: "Goal %{percent}% complete. %{amount} of %{target}, including %{used} already used." projection: heading: Projection legend_saved: Saved @@ -259,6 +261,7 @@ en: no_depository_accounts: You need at least one depository account (checking, savings, HSA, CD, money-market) before creating a goal. add_account: Add an account goal_card: + including_used: "Including %{amount} already used" no_accounts: No linked accounts n_accounts: "%{first} +%{count}" left: left diff --git a/config/locales/views/goals/fr.yml b/config/locales/views/goals/fr.yml index b204d86e3..2b338813e 100644 --- a/config/locales/views/goals/fr.yml +++ b/config/locales/views/goals/fr.yml @@ -76,6 +76,7 @@ fr: suggested_no_date: Définissez une date cible pour projeter la fin. suggested_with_date: Épargnez {monthly}/mois sur {accounts} pour l'atteindre à temps. goal_card: + including_used: "Dont %{amount} déjà utilisés" accounts: one: 1 compte other: "%{count} comptes" @@ -302,7 +303,9 @@ fr: reopen: Réouvrir l'objectif resume: Reprendre ring: + including_used: "Dont %{amount} déjà utilisés" aria_label: Objectif complété à %{percent}%. %{amount} sur %{target} épargnés. + aria_label_with_used: "Objectif à %{percent}%. %{amount} sur %{target}, dont %{used} déjà utilisés." market_value: Valeur de marché %{amount} of: sur %{target} of_target: de l'objectif diff --git a/test/controllers/goals_controller_test.rb b/test/controllers/goals_controller_test.rb index 650bc946c..fea620785 100644 --- a/test/controllers/goals_controller_test.rb +++ b/test/controllers/goals_controller_test.rb @@ -680,6 +680,18 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest end private + + def spent_goal_for_display + account = Account.create!( + family: @user.family, accountable: Depository.new, + name: "Spent Pot", currency: "USD", balance: 5_000 + ) + goal = @user.family.goals.create!(name: "Trip", target_amount: 5_000, currency: "USD") do |g| + g.goal_accounts.build(account: account, allocated_amount: 5_000) + end + goal.consume!(2_000) + goal + end # SQL the pooled-allocation read issues, and nothing else: goal_accounts # joined to goals. def count_pool_queries @@ -694,6 +706,68 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest ActiveSupport::Notifications.unsubscribe(sub) 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. diff --git a/test/models/goal_test.rb b/test/models/goal_test.rb index 6bd5d1b68..06d5d26a1 100644 --- a/test/models/goal_test.rb +++ b/test/models/goal_test.rb @@ -1196,8 +1196,55 @@ class GoalTest < ActiveSupport::TestCase assert_equal 3_000, goal.reload.target_amount.to_d end + + # The ring is drawn from `progress_percent`, which counts money held plus + # money already spent on the goal. The headline figure showed only the first + # half, so a goal that had spent part of its savings sat at a 100% ring + # beside "3,000 of 5,000" — the two disagreeing on the same card, with + # nothing to say which one to believe. + test "the headline figure agrees with the ring after a spend" do + goal = spent_goal + + assert_equal 100, goal.progress_percent + assert_equal 5_000, goal.progress_amount_money.amount.to_d, + "the figure beside the ring still reported the account balance" + end + + # The amount is part of the total above it, never a second figure beside it: + # a reader should have nothing to add up. + test "what was used is reported as part of the total, and only when there is some" do + goal = spent_goal + + assert goal.any_consumption? + assert_equal 2_000, goal.consumed_amount_money.amount.to_d + assert_equal goal.progress_amount_money.amount.to_d, + goal.current_balance.to_d + goal.consumed_amount_money.amount.to_d + end + + test "a goal that has spent nothing says nothing about it" do + account = Account.create!(family: @family, accountable: Depository.new, + name: "Quiet pot", currency: @family.currency, balance: 1_000) + goal = @family.goals.create!(name: "Quiet", target_amount: 1_000, currency: @family.currency) do |g| + g.goal_accounts.build(account: account, allocated_amount: 1_000) + end + + assert_not goal.any_consumption? + end + private + # 5,000 saved, 2,000 of it since spent on the thing itself. + def spent_goal + account = Account.create!(family: @family, accountable: Depository.new, + name: "Spent pot #{SecureRandom.hex(3)}", + currency: @family.currency, balance: 5_000) + goal = @family.goals.create!(name: "Trip", target_amount: 5_000, currency: @family.currency) do |g| + g.goal_accounts.build(account: account, allocated_amount: 5_000) + end + goal.consume!(2_000) + goal + end + # Its own account, so the shared-pool haircut does not make the frozen # figure depend on what the fixtures happen to claim. def completable_goal