diff --git a/app/controllers/goals_controller.rb b/app/controllers/goals_controller.rb
index aeadeef08..ae291283b 100644
--- a/app/controllers/goals_controller.rb
+++ b/app/controllers/goals_controller.rb
@@ -47,6 +47,7 @@ class GoalsController < ApplicationController
)
@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 @@ class GoalsController < ApplicationController
# 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 @@ class GoalsController < ApplicationController
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 @@ class GoalsController < ApplicationController
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 704dfdddb..13eb4d4e3 100644
--- a/app/helpers/goals_helper.rb
+++ b/app/helpers/goals_helper.rb
@@ -19,4 +19,24 @@ module GoalsHelper
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 000000000..6e2aa855a
--- /dev/null
+++ b/app/javascript/controllers/goal_earmark_controller.js
@@ -0,0 +1,97 @@
+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,
+ 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()
+ }
+
+ 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)
+ }
+
+ 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)
+
+ 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(this.localeValue || undefined, {
+ style: "currency",
+ currency: this.currencyValue || "USD",
+ maximumFractionDigits: 0,
+ }).format(value)
+ } catch {
+ return `${this.currencyValue || "$"}${Math.round(value).toLocaleString()}`
+ }
+ }
+}
diff --git a/app/models/assistant/function/create_goal.rb b/app/models/assistant/function/create_goal.rb
index 9452ccf27..18c679dba 100644
--- a/app/models/assistant/function/create_goal.rb
+++ b/app/models/assistant/function/create_goal.rb
@@ -55,6 +55,11 @@ class Assistant::Function::CreateGoal < Assistant::Function
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 @@ class Assistant::Function::CreateGoal < Assistant::Function
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 @@ class Assistant::Function::CreateGoal < Assistant::Function
)
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 @@ class Assistant::Function::CreateGoal < Assistant::Function
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 @@ class Assistant::Function::CreateGoal < Assistant::Function
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/app/views/goals/_form.html.erb b/app/views/goals/_form.html.erb
index f4fe92b47..45195af57 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,22 @@
+ <%# 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. %>
+
<% end %>
diff --git a/config/locales/views/goals/en.yml b/config/locales/views/goals/en.yml
index c35a51da1..2f3dba0a2 100644
--- a/config/locales/views/goals/en.yml
+++ b/config/locales/views/goals/en.yml
@@ -300,6 +300,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 03642562e..63988daba 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 f95ed0c40..b850afe74 100644
--- a/test/controllers/goals_controller_test.rb
+++ b/test/controllers/goals_controller_test.rb
@@ -523,6 +523,57 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest
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
+ 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
+
# --- Lot B4: recording a partial spend ---
test "recording a spend keeps the goal at full progress and frees the earmark" do
@@ -575,6 +626,21 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest
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.
diff --git a/test/helpers/goals_helper_test.rb b/test/helpers/goals_helper_test.rb
new file mode 100644
index 000000000..27c349b27
--- /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
diff --git a/test/models/assistant/function/create_goal_test.rb b/test/models/assistant/function/create_goal_test.rb
index 32ecd6853..2f999a8d6 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