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