From cfa27c8922bb5c750bd6ed75e27edb5cb03d0399 Mon Sep 17 00:00:00 2001 From: buzzromain <18685603+buzzromain@users.noreply.github.com> Date: Mon, 24 Aug 2026 18:34:26 +0000 Subject: [PATCH 1/4] feat(goals): show what each account still has room to earmark MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Account#free_to_earmark` has existed, unused, since earmarks shipped — its own comment said the UI was a follow-up. This is that follow-up, and the wording is the substance of it. It does not say "over-allocated". `free_to_earmark` is negative for as long as the saving is unfinished, which is the normal condition of anyone with goals in progress: a 6,000 account backing two goals of 5,000 gives −4,000 and is a perfectly correct setup. A warning phrased as a fault would fire permanently and teach people to ignore it. The message states the consequence instead — the goals come to X for a balance of Y, so they progress pro rata — and is never styled as an error. The trap is the goal being edited. `goal_earmarked_total` counts every goal including that one, so reopening a goal that earmarks 5,000 on a 6,000 account shows 1,000 of headroom, and re-entering the same 5,000 trips a message about a setup the user has not touched. `earmarked_by_other_goals` excludes it, and only when it is persisted — a goal being created has nothing to exclude. The pool is read once per render and passed down, never per account: the form lists every fundable account the user can see. A test counts the query and fails at two. The Stimulus controller is its own, with 3 targets. goal_form_controller is at 10 against the 7 the project guidelines suggest, needs none of this state, and is untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 --- app/controllers/goals_controller.rb | 5 ++ app/helpers/goals_helper.rb | 20 +++++ .../controllers/goal_earmark_controller.js | 87 +++++++++++++++++++ app/views/goals/_form.html.erb | 26 +++++- app/views/goals/edit.html.erb | 2 +- app/views/goals/new.html.erb | 4 +- config/locales/views/goals/en.yml | 4 + config/locales/views/goals/fr.yml | 4 + test/controllers/goals_controller_test.rb | 63 ++++++++++++++ test/helpers/goals_helper_test.rb | 74 ++++++++++++++++ 10 files changed, 282 insertions(+), 7 deletions(-) create mode 100644 app/javascript/controllers/goal_earmark_controller.js create mode 100644 test/helpers/goals_helper_test.rb diff --git a/app/controllers/goals_controller.rb b/app/controllers/goals_controller.rb index d20e46f57c..bde72fae6b 100644 --- a/app/controllers/goals_controller.rb +++ b/app/controllers/goals_controller.rb @@ -47,6 +47,7 @@ def new ) @linkable_accounts = linkable_accounts_for_new @currently_linked_account_ids = [] + @pooled_allocations = Goal.pooled_allocations_for(Current.family) @breadcrumbs = plan_breadcrumb_prefix + [ [ t("goals.index.title"), goals_path ], [ t("goals.new.heading"), nil ] @@ -79,11 +80,13 @@ def create # the same built records — so the user was left staring at an error telling # them to enter an amount, on a form whose accounts had silently cleared. @currently_linked_account_ids = @goal.goal_accounts.map { |ga| ga.account_id.to_s } + @pooled_allocations = Goal.pooled_allocations_for(Current.family) render :new, status: :unprocessable_entity end def edit @linkable_accounts = linkable_accounts_for_new + @pooled_allocations = Goal.pooled_allocations_for(Current.family) @currently_linked_account_ids = @goal.goal_accounts.pluck(:account_id).map(&:to_s) end @@ -95,6 +98,7 @@ def update if accounts_supplied && accounts.empty? @goal.errors.add(:base, :at_least_one_linked_account_required) @linkable_accounts = linkable_accounts_for_new + @pooled_allocations = Goal.pooled_allocations_for(Current.family) @currently_linked_account_ids = @goal.goal_accounts.pluck(:account_id).map(&:to_s) render :edit, status: :unprocessable_entity return @@ -125,6 +129,7 @@ def update end rescue ActiveRecord::RecordInvalid @linkable_accounts = linkable_accounts_for_new + @pooled_allocations = Goal.pooled_allocations_for(Current.family) @currently_linked_account_ids = @goal.goal_accounts.pluck(:account_id).map(&:to_s) render :edit, status: :unprocessable_entity end diff --git a/app/helpers/goals_helper.rb b/app/helpers/goals_helper.rb index 704dfdddb4..13eb4d4e35 100644 --- a/app/helpers/goals_helper.rb +++ b/app/helpers/goals_helper.rb @@ -19,4 +19,24 @@ def goal_complete_confirm(goal) btn_text: t("goals.show.confirm_complete_cta") ) end + + # Fixed earmarks on `account` held by goals OTHER than the one being + # edited, read from a pooled map loaded once per render + # (Goal.pooled_allocations_for) rather than per account — the form lists + # every fundable account, so a per-account query is a guaranteed N+1. + # + # Excluding the current goal is what makes the warning trustworthy: + # `Account#goal_earmarked_total` counts every goal, so reopening a goal + # that earmarks 5,000 on a 6,000 account would leave 1,000 of apparent + # headroom, and re-entering the same 5,000 would trip a message although + # nothing changed. + # + # Whole-balance links carry a nil allocated_amount and contribute zero: + # they reserve no fixed slice. That is the right reading here — what they + # do claim is guarded separately, at write time, by GoalAccount. + def earmarked_by_other_goals(account, pooled:, current_goal: nil) + (pooled[account.id] || []) + .reject { |row| current_goal&.persisted? && row[:goal_id] == current_goal.id } + .sum { |row| row[:allocated_amount].to_d } + end end diff --git a/app/javascript/controllers/goal_earmark_controller.js b/app/javascript/controllers/goal_earmark_controller.js new file mode 100644 index 0000000000..a674afdaf4 --- /dev/null +++ b/app/javascript/controllers/goal_earmark_controller.js @@ -0,0 +1,87 @@ +import { Controller } from "@hotwired/stimulus" + +// Tells the user what each funding account still has room for, as they type. +// +// Nothing here is an error, and the wording matters more than the maths: a +// goal in progress legitimately claims more than its account holds. An +// account of 6,000 backing two goals of 5,000 is a correct setup, not an +// over-allocation, so a warning phrased as one would fire permanently. The +// pro-rata message states the consequence instead of scolding. +// +// Deliberately separate from goal-form, which is already at 10 targets +// against the 7 the project guidelines suggest. Three here, and no shared +// state between them. +export default class extends Controller { + static targets = ["allocationInput", "warning", "checkbox"] + static values = { + currency: String, + wholeBalance: String, + prorata: String, + headroom: String, + } + + connect() { + this.refresh() + } + + refresh() { + this.allocationInputTargets.forEach((input) => this.#refreshRow(input)) + } + + #refreshRow(input) { + const row = input.closest("[data-balance]") + if (!row) return + + const warning = row.querySelector('[data-goal-earmark-target="warning"]') + const checkbox = row.querySelector('input[type="checkbox"]') + if (!warning) return + + // An unchecked account funds nothing, so it has nothing to say. + if (checkbox && !checkbox.checked) return this.#hide(warning) + + const balance = Number.parseFloat(row.dataset.balance || "0") + const others = Number.parseFloat(row.dataset.earmarkedByOthers || "0") + const raw = input.value.trim() + + if (raw === "") { + return this.#show(warning, this.wholeBalanceValue) + } + + const entered = Number.parseFloat(raw.replace(",", ".")) + if (Number.isNaN(entered)) return this.#hide(warning) + + const total = others + entered + + if (total > balance) { + this.#show( + warning, + this.prorataValue + .replace("{total}", this.#money(total)) + .replace("{balance}", this.#money(balance)), + ) + } else { + this.#show(warning, this.headroomValue.replace("{left}", this.#money(balance - total))) + } + } + + #show(element, text) { + element.textContent = text + element.classList.remove("hidden") + } + + #hide(element) { + element.classList.add("hidden") + } + + #money(value) { + try { + return new Intl.NumberFormat(undefined, { + style: "currency", + currency: this.currencyValue || "USD", + maximumFractionDigits: 0, + }).format(value) + } catch { + return `${this.currencyValue || "$"}${Math.round(value).toLocaleString()}` + } + } +} diff --git a/app/views/goals/_form.html.erb b/app/views/goals/_form.html.erb index f4fe92b470..052d5e0080 100644 --- a/app/views/goals/_form.html.erb +++ b/app/views/goals/_form.html.erb @@ -1,4 +1,4 @@ -<%# locals: (goal:, linkable_accounts:, currently_linked_account_ids: []) %> +<%# locals: (goal:, linkable_accounts:, currently_linked_account_ids: [], pooled_allocations: {}) %> <%# goal-kind lives on this wrapper, not on the kind selector: its dateField target is a sibling of the radios, and a controller only sees @@ -70,7 +70,15 @@

<%= t("goals.form.fields.funding_accounts_hint") %>

<%= t("goals.form.fields.earmark_hint") %>

-
+ <%# Its own controller, not goal-form: that one is already at 10 targets + against the 7 the project guidelines suggest, and this needs none of + its state. Three targets here. %> +
" + data-goal-earmark-prorata-value="<%= t("goals.form.earmark.prorata") %>" + data-goal-earmark-headroom-value="<%= t("goals.form.earmark.headroom") %>"> <% linked_allocation_by_account = goal.goal_accounts.index_by(&:account_id) %> <% grouped = linkable_accounts.group_by { |a| a.subtype.to_s.presence || "other" } %> <% grouped.each_with_index do |(subtype, accts), group_idx| %> @@ -78,7 +86,10 @@
"> <% accts.each_with_index do |account, idx| %> <% linked_ga = linked_allocation_by_account[account.id] %> -
0 %>"> +
0 %>" + data-balance="<%= account.balance.to_d %>" + data-earmarked-by-others="<%= earmarked_by_other_goals(account, pooled: pooled_allocations, current_goal: goal) %>"> +
<% end %>
diff --git a/app/views/goals/edit.html.erb b/app/views/goals/edit.html.erb index de4628c7c1..9420ded790 100644 --- a/app/views/goals/edit.html.erb +++ b/app/views/goals/edit.html.erb @@ -1,6 +1,6 @@ <%= render DS::Dialog.new do |dialog| %> <% dialog.with_header(title: t(".heading")) %> <% dialog.with_body do %> - <%= render "form", goal: @goal, linkable_accounts: @linkable_accounts, currently_linked_account_ids: @currently_linked_account_ids %> + <%= render "form", goal: @goal, linkable_accounts: @linkable_accounts, currently_linked_account_ids: @currently_linked_account_ids, pooled_allocations: @pooled_allocations %> <% end %> <% end %> diff --git a/app/views/goals/new.html.erb b/app/views/goals/new.html.erb index 758727244e..391d0ddd94 100644 --- a/app/views/goals/new.html.erb +++ b/app/views/goals/new.html.erb @@ -13,7 +13,7 @@
<% end %> <% dialog.with_body do %> - <%= render "form", goal: @goal, linkable_accounts: @linkable_accounts, currently_linked_account_ids: @currently_linked_account_ids %> + <%= render "form", goal: @goal, linkable_accounts: @linkable_accounts, currently_linked_account_ids: @currently_linked_account_ids, pooled_allocations: @pooled_allocations %> <% end %> <% end %> <% else %> @@ -25,6 +25,6 @@

<%= t(".subtitle") %>

- <%= render "form", goal: @goal, linkable_accounts: @linkable_accounts, currently_linked_account_ids: @currently_linked_account_ids %> + <%= render "form", goal: @goal, linkable_accounts: @linkable_accounts, currently_linked_account_ids: @currently_linked_account_ids, pooled_allocations: @pooled_allocations %>
<% end %> diff --git a/config/locales/views/goals/en.yml b/config/locales/views/goals/en.yml index 871b28fd34..4a441e71c6 100644 --- a/config/locales/views/goals/en.yml +++ b/config/locales/views/goals/en.yml @@ -280,6 +280,10 @@ en: name_required: Give your goal a name. amount_required: Set a target above zero. accounts_required: Pick at least one funding account. + earmark: + headroom: "{left} left to earmark on this account." + prorata: "Your goals on this account come to {total} for a balance of {balance} — they will progress pro rata." + whole_balance: This account will fund whatever is left after the other earmarks. fields: name: Name name_placeholder: Emergency fund, House down payment… diff --git a/config/locales/views/goals/fr.yml b/config/locales/views/goals/fr.yml index 3552265b39..1454983291 100644 --- a/config/locales/views/goals/fr.yml +++ b/config/locales/views/goals/fr.yml @@ -42,6 +42,10 @@ fr: accounts_required: Sélectionnez au moins un compte de financement. amount_required: Définissez une cible supérieure à zéro. name_required: Donnez un nom à votre objectif. + earmark: + headroom: "Il reste {left} affectables sur ce compte." + prorata: "Vos objectifs sur ce compte totalisent {total} pour un solde de {balance} : ils progresseront au prorata." + whole_balance: Ce compte financera le solde restant après les autres affectations. fields: color: Couleur earmark_for: Affecter un montant pour %{account} diff --git a/test/controllers/goals_controller_test.rb b/test/controllers/goals_controller_test.rb index 4b438e51b7..dd70d12d06 100644 --- a/test/controllers/goals_controller_test.rb +++ b/test/controllers/goals_controller_test.rb @@ -497,9 +497,72 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_match I18n.t("goals.show.reserve_shortfall.heading"), response.body assert_no_match(/#{Regexp.escape(I18n.t("goals.show.empty.heading"))}/, response.body) + # --- Lot B6: earmark headroom in the form --- + + test "the new form renders each account's balance and what other goals hold" do + account = unclaimed_account("Headroom Pot") + @user.family.goals.create!(name: "Neighbour", target_amount: 5_000, currency: "USD") do |g| + g.goal_accounts.build(account: account, allocated_amount: 400) + end + + get new_goal_url + + assert_response :success + assert_select "[data-balance][data-earmarked-by-others]", minimum: 1 + assert_match 'data-earmarked-by-others="400.0"', response.body + end + + # The N+1 this lot exists to avoid: the form lists every fundable account, + # so reading the pool per account would scale with the account list. + test "the form reads the shared pool exactly once, however many accounts" do + 3.times { |i| unclaimed_account("Pool Pot #{i}") } + + assert_equal 1, count_pool_queries { get new_goal_url } + assert_response :success + end + + # The acceptance criterion of this lot: reopening a goal must not count its + # own earmark, or re-entering the same figure would trip a message about a + # setup the user has not touched. + test "the edit form does not count the edited goal's own earmark" do + account = unclaimed_account("Reopen Pot") + goal = @user.family.goals.create!(name: "Reopened", target_amount: 5_000, currency: "USD") do |g| + g.goal_accounts.build(account: account, allocated_amount: 5_000) + end + + get edit_goal_url(goal) + + assert_response :success + assert_match 'data-earmarked-by-others="0.0"', response.body + assert_no_match(/data-earmarked-by-others="5000/, response.body) + end + + # The error paths re-render the same form, so they need the pool too — + # without it the row helper is handed nil and the render blows up. + test "the pool is available again when create re-renders after an error" do + unclaimed_account("Error Pot") + + post goals_url, params: { goal: { name: "No accounts", target_amount: "1000", color: "#4da568" } } + + assert_response :unprocessable_entity + assert_select "[data-balance][data-earmarked-by-others]", minimum: 1 end private + # SQL the pooled-allocation read issues, and nothing else: goal_accounts + # joined to goals. + def count_pool_queries + count = 0 + sub = ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload| + sql = payload[:sql].to_s + count += 1 if sql.include?("FROM \"goal_accounts\"") && sql.include?("INNER JOIN \"goals\"") + end + yield + count + ensure + ActiveSupport::Notifications.unsubscribe(sub) + end + # An active one_off goal sitting exactly at its target, on an account no # other goal claims. def fully_funded_goal diff --git a/test/helpers/goals_helper_test.rb b/test/helpers/goals_helper_test.rb new file mode 100644 index 0000000000..27c349b27a --- /dev/null +++ b/test/helpers/goals_helper_test.rb @@ -0,0 +1,74 @@ +require "test_helper" + +class GoalsHelperTest < ActionView::TestCase + include GoalsHelper + + setup do + @family = families(:dylan_family) + @account = Account.create!( + family: @family, accountable: Depository.new, + name: "Helper Savings", currency: "USD", balance: 6_000 + ) + end + + test "sums the fixed earmarks other goals hold on the account" do + build_goal("A", 2_000) + build_goal("B", 1_500) + + assert_equal BigDecimal("3500"), earmarked_by_other_goals(@account, pooled: pooled) + end + + # The whole point of the helper: reopening a goal must not count itself. + test "excludes the goal currently being edited" do + mine = build_goal("Mine", 5_000) + build_goal("Theirs", 500) + + assert_equal BigDecimal("500"), earmarked_by_other_goals(@account, pooled: pooled, current_goal: mine) + end + + test "a goal that does not exist yet excludes nothing" do + build_goal("Existing", 2_000) + unsaved = @family.goals.new(name: "New", target_amount: 1_000, currency: "USD") + + assert_equal BigDecimal("2000"), earmarked_by_other_goals(@account, pooled: pooled, current_goal: unsaved) + end + + test "whole-balance links reserve no fixed slice" do + build_goal("Whole", nil) + + assert_equal BigDecimal("0"), earmarked_by_other_goals(@account, pooled: pooled) + end + + test "an account no goal touches has nothing earmarked" do + untouched = Account.create!( + family: @family, accountable: Depository.new, + name: "Untouched", currency: "USD", balance: 1_000 + ) + + assert_equal BigDecimal("0"), earmarked_by_other_goals(untouched, pooled: pooled) + end + + # Depends on Lot B1: both released states drop out of the pool, so neither + # can inflate the headroom warning. + test "archived and completed goals are absent from the pool" do + archived = build_goal("Archived", 1_000) + completed = build_goal("Completed", 1_000) + build_goal("Live", 750) + + archived.archive! + completed.complete! + + assert_equal BigDecimal("750"), earmarked_by_other_goals(@account, pooled: pooled) + end + + private + def build_goal(name, allocated) + @family.goals.create!(name: name, target_amount: 10_000, currency: "USD") do |g| + g.goal_accounts.build(account: @account, allocated_amount: allocated) + end + end + + def pooled + Goal.pooled_allocations_for(@family) + end +end From 98fb2fadbbe59f8d60ae56b9e15b09759bffaa20 Mon Sep 17 00:00:00 2001 From: buzzromain <18685603+buzzromain@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:02:13 +0000 Subject: [PATCH 2/4] fix(goals): read the typed amount strictly, and format it in the app's locale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on #3166. `Number.parseFloat` accepts prefixes, so "500abc" became 500, and the bare comma-to-dot swap turned a thousands-separated "1,500" into 1.5. Either way the preview described an amount the user had not typed — and the second case is a habit from another locale, not a typo, so it would have gone unnoticed. The value now has to match a complete number before anything is computed. `Intl.NumberFormat(undefined, ...)` let the BROWSER pick the locale, so a French user on an English-locale browser read separators and symbol placement matching nothing else on the page. The amounts cannot be formatted server-side — they change with every keystroke — so the server passes `I18n.locale` and the client applies it. That puts the decision where the rest of the app's formatting already lives. bin/rails test: 6954 runs, 0 failures. RuboCop, erb_lint and biome clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --- .../controllers/goal_earmark_controller.js | 12 +++++++++++- app/views/goals/_form.html.erb | 7 +++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/javascript/controllers/goal_earmark_controller.js b/app/javascript/controllers/goal_earmark_controller.js index a674afdaf4..6e2aa855a6 100644 --- a/app/javascript/controllers/goal_earmark_controller.js +++ b/app/javascript/controllers/goal_earmark_controller.js @@ -15,11 +15,19 @@ export default class extends Controller { static targets = ["allocationInput", "warning", "checkbox"] static values = { currency: String, + locale: String, wholeBalance: String, prorata: String, headroom: String, } + // A complete number, optionally with one decimal separator and digits after + // it. `Number.parseFloat` alone accepts prefixes — "500abc" becomes 500 — + // and a bare comma-to-dot swap turns the thousands-separated "1,500" into + // 1.5, so a typo or a habit from another locale silently changed the amount + // the preview was based on. + static ALLOCATION_PATTERN = /^\d+(?:[.,]\d+)?$/ + connect() { this.refresh() } @@ -47,6 +55,8 @@ export default class extends Controller { return this.#show(warning, this.wholeBalanceValue) } + if (!this.constructor.ALLOCATION_PATTERN.test(raw)) return this.#hide(warning) + const entered = Number.parseFloat(raw.replace(",", ".")) if (Number.isNaN(entered)) return this.#hide(warning) @@ -75,7 +85,7 @@ export default class extends Controller { #money(value) { try { - return new Intl.NumberFormat(undefined, { + return new Intl.NumberFormat(this.localeValue || undefined, { style: "currency", currency: this.currencyValue || "USD", maximumFractionDigits: 0, diff --git a/app/views/goals/_form.html.erb b/app/views/goals/_form.html.erb index 052d5e0080..45195af57e 100644 --- a/app/views/goals/_form.html.erb +++ b/app/views/goals/_form.html.erb @@ -76,6 +76,13 @@
+ data-goal-earmark-locale-value="<%= I18n.locale %>" data-goal-earmark-whole-balance-value="<%= t("goals.form.earmark.whole_balance") %>" data-goal-earmark-prorata-value="<%= t("goals.form.earmark.prorata") %>" data-goal-earmark-headroom-value="<%= t("goals.form.earmark.headroom") %>"> From f9f813568c3951978e22d0f90f6e5cb190467771 Mon Sep 17 00:00:00 2001 From: buzzromain <18685603+buzzromain@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:53:59 +0000 Subject: [PATCH 3/4] fix(goals): let the assistant create a second goal on a claimed account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #3166. The function always built whole-account links and had no way to express an earmark, so once exclusivity landed, asking for a second goal on an account another goal already claimed came back as a bare `validation_failed` — while the account list still advertised the account as available. A common request became an unexplained refusal. Three changes, and the list is the important one: it now says what is left on each account and which are claimed in full, because the assistant reasons from that list and had no way to know otherwise. `earmarks` is an optional map of account name to amount, so the assistant can reserve a slice rather than the whole balance. Accounts left out keep the previous behaviour and take whatever is spare. The refusal is named before the save — `account_claimed_in_full`, with the account names — so the assistant gets a reason it can act on and ask about, rather than a validation message it can only relay. Checked after the currency check, which is the more fundamental of the two. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --- app/models/assistant/function/create_goal.rb | 60 ++++++++++++++++++- test/controllers/goals_controller_test.rb | 2 + .../assistant/function/create_goal_test.rb | 52 ++++++++++++++++ 3 files changed, 112 insertions(+), 2 deletions(-) diff --git a/app/models/assistant/function/create_goal.rb b/app/models/assistant/function/create_goal.rb index 9452ccf27f..18c679dba8 100644 --- a/app/models/assistant/function/create_goal.rb +++ b/app/models/assistant/function/create_goal.rb @@ -55,6 +55,11 @@ def params_schema items: { type: "string" }, description: "Names of the user's Depository accounts to link. Must contain at least one. Use names exactly as they appear in the available accounts list. The goal's balance is the balance of these accounts." }, + earmarks: { + type: "object", + description: "Optional map of account name to the amount to reserve from that account, e.g. {\"Livret A\": 2000}. Required for an account already claimed in full by another goal — the available accounts list says which, and how much room is left. Accounts left out of this map reserve whatever the account has spare.", + additionalProperties: { type: "number" } + }, notes: { type: "string", description: "Optional freeform notes." @@ -69,6 +74,7 @@ def call(params = {}) target_date = parse_date(params["target_date"]) linked_account_names = Array(params["linked_account_names"]).map { |n| n.to_s.strip }.reject(&:blank?) notes = params["notes"].to_s.strip + earmarks = parse_earmarks(params["earmarks"]) return error("name_required", "Please provide a name for the goal.") if name.blank? @@ -117,6 +123,21 @@ def call(params = {}) ) end + # Named before the save, so the assistant gets a reason it can act on + # rather than a generic validation failure it can only relay. Claiming an + # account in full is exclusive; joining one that is already claimed needs + # an explicit earmark, and the assistant can ask for one. + over_claimed = matched.select { |a| whole_account_claimed_ids.include?(a.id) && earmarks[a.name].nil? } + if over_claimed.any? + return error( + "account_claimed_in_full", + "Another goal already claims #{over_claimed.map(&:name).to_sentence} in full. " \ + "Ask the user how much to reserve from #{'it'.pluralize(over_claimed.size)}, then pass it in `earmarks`.", + claimed_account_names: over_claimed.map(&:name), + available_accounts: depository_account_payload + ) + end + goal = nil Goal.transaction do goal = family.goals.new( @@ -127,7 +148,7 @@ def call(params = {}) notes: notes.presence, color: Goal::COLORS.sample ) - matched.each { |a| goal.goal_accounts.build(account: a) } + matched.each { |a| goal.goal_accounts.build(account: a, allocated_amount: earmarks[a.name]) } goal.save! end @@ -174,8 +195,43 @@ def parse_date(value) nil end + # Says what is left, not just what exists. A goal that claims an account in + # full is exclusive, so an account already claimed can only be joined with + # an explicit earmark — and the assistant has no way to know that unless + # the list says so. def depository_account_payload - family.accounts.where(accountable_type: "Depository").visible.pluck(:name, :currency).map { |n, c| { name: n, currency: c } } + claimed = whole_account_claimed_ids + + family.accounts.where(accountable_type: "Depository").visible.map do |account| + { + name: account.name, + currency: account.currency, + free_to_earmark: Money.new(account.free_to_earmark, account.currency).format, + claimed_in_full: claimed.include?(account.id) + } + end + end + + def whole_account_claimed_ids + @whole_account_claimed_ids ||= GoalAccount.joins(:goal) + .where(allocated_amount: nil) + .where(goals: { family_id: family.id }) + .where.not(goals: { state: Goal::RELEASED_STATES }) + .pluck(:account_id) + .to_set + end + + # Names are the assistant's handle on an account, so the map is keyed by + # them. Non-positive amounts are dropped rather than refused: a zero + # earmark and no earmark mean different things to the model, and neither + # is what the user asked for. + def parse_earmarks(raw) + return {} unless raw.is_a?(Hash) + + raw.each_with_object({}) do |(account_name, amount), acc| + value = parse_decimal(amount) + acc[account_name.to_s.strip] = value if value && value.positive? + end end def error(key, message, extras = {}) diff --git a/test/controllers/goals_controller_test.rb b/test/controllers/goals_controller_test.rb index dd70d12d06..0cce094fd0 100644 --- a/test/controllers/goals_controller_test.rb +++ b/test/controllers/goals_controller_test.rb @@ -497,6 +497,8 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_match I18n.t("goals.show.reserve_shortfall.heading"), response.body assert_no_match(/#{Regexp.escape(I18n.t("goals.show.empty.heading"))}/, response.body) + end + # --- Lot B6: earmark headroom in the form --- test "the new form renders each account's balance and what other goals hold" do diff --git a/test/models/assistant/function/create_goal_test.rb b/test/models/assistant/function/create_goal_test.rb index 32ecd68531..2f999a8d6a 100644 --- a/test/models/assistant/function/create_goal_test.rb +++ b/test/models/assistant/function/create_goal_test.rb @@ -93,4 +93,56 @@ class Assistant::Function::CreateGoalTest < ActiveSupport::TestCase assert_equal false, result[:success] assert_equal "unknown_accounts", result[:error] end + + # --- Review follow-up (#3166) --- + + # The function always built whole-account links, so once exclusivity landed a + # second goal on the same account failed with a generic validation error — + # while the account list still advertised it as available. A common request + # ("save for a holiday too") became an unexplained refusal. + test "an account another goal claims in full is refused with a reason, not a validation failure" do + account = Account.create!(family: @family, accountable: Depository.new, + name: "Claimed Pot", currency: "USD", balance: 5_000) + @family.goals.create!(name: "Precaution", target_amount: 5_000, currency: "USD") do |g| + g.goal_accounts.build(account: account) + end + + result = @fn.call("name" => "Holiday", "target_amount" => 1_000, + "linked_account_names" => [ account.name ]) + + assert_equal false, result[:success] + assert_equal "account_claimed_in_full", result[:error] + assert_includes result[:claimed_account_names], account.name + end + + test "the same account is accepted once an earmark says how much to take" do + account = Account.create!(family: @family, accountable: Depository.new, + name: "Claimed Pot", currency: "USD", balance: 5_000) + @family.goals.create!(name: "Precaution", target_amount: 5_000, currency: "USD") do |g| + g.goal_accounts.build(account: account) + end + + result = @fn.call("name" => "Holiday", "target_amount" => 1_000, + "linked_account_names" => [ account.name ], + "earmarks" => { account.name => 1_000 }) + + assert_equal true, result[:success] + assert_equal 1_000, Goal.find(result[:goal_id]).goal_accounts.first.allocated_amount.to_d + end + + # The list is what the assistant reasons from; without this it had no way to + # know an account could not be taken whole. + test "the account list says what is left and what is already claimed" do + account = Account.create!(family: @family, accountable: Depository.new, + name: "Claimed Pot", currency: "USD", balance: 5_000) + @family.goals.create!(name: "Precaution", target_amount: 5_000, currency: "USD") do |g| + g.goal_accounts.build(account: account) + end + + result = @fn.call("name" => "X", "target_amount" => 100, "linked_account_names" => []) + listed = result[:available_accounts].find { |a| a[:name] == account.name } + + assert listed[:claimed_in_full] + assert listed.key?(:free_to_earmark) + end end From 3910f1899539a89a82939e26c0d372c2e33de1b7 Mon Sep 17 00:00:00 2001 From: buzzromain <18685603+buzzromain@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:06:04 +0000 Subject: [PATCH 4/4] test(goals): move the spend tests back out of the private section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merge of `main` into this branch landed #3176's tests between `count_pool_queries` and the helpers below it, inside the `private` section and at the wrong indentation. `ci / lint` has been failing on `Layout/IndentationConsistency` since. They still ran — `test` is a class method, so `private` does not hide them — which is why the unit job stayed green while lint went red. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --- test/controllers/goals_controller_test.rb | 30 +++++++++++------------ 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/test/controllers/goals_controller_test.rb b/test/controllers/goals_controller_test.rb index 06242f345c..b850afe745 100644 --- a/test/controllers/goals_controller_test.rb +++ b/test/controllers/goals_controller_test.rb @@ -574,21 +574,6 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest assert_select "[data-balance][data-earmarked-by-others]", minimum: 1 end - private - # SQL the pooled-allocation read issues, and nothing else: goal_accounts - # joined to goals. - def count_pool_queries - count = 0 - sub = ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload| - sql = payload[:sql].to_s - count += 1 if sql.include?("FROM \"goal_accounts\"") && sql.include?("INNER JOIN \"goals\"") - end - yield - count - ensure - ActiveSupport::Notifications.unsubscribe(sub) - end - # --- Lot B4: recording a partial spend --- test "recording a spend keeps the goal at full progress and frees the earmark" do @@ -641,6 +626,21 @@ def count_pool_queries assert_equal 0, goal.reload.consumed_amount end + private + # SQL the pooled-allocation read issues, and nothing else: goal_accounts + # joined to goals. + def count_pool_queries + count = 0 + sub = ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload| + sql = payload[:sql].to_s + count += 1 if sql.include?("FROM \"goal_accounts\"") && sql.include?("INNER JOIN \"goals\"") + end + yield + count + ensure + ActiveSupport::Notifications.unsubscribe(sub) + end + private # A private account of another member, linked to the goal under test.