mirror of
https://github.com/we-promise/sure.git
synced 2026-09-07 15:44:21 +00:00
* fix(goals): stop two goals from each claiming the same account in full
A GoalAccount with a NULL `allocated_amount` means "dedicate the whole
balance". Two of them on one account each claimed all of it, so the money
was counted twice:
Livret A, 6,000 precaution 6,000 vacances 6,000
progress: 100% progress: 100%
`Goal#backing_share_for` cannot catch this. Its pro-rata haircut only
scales FIXED earmarks, and an unallocated link contributes `nil.to_d` —
zero — to `others_fixed`, so the two links never see each other. The
invariant "shares never sum past the balance" held for every earmark
except the one that claims everything.
Enforce it at the door: GoalAccount now refuses a second whole-balance
link on an account another non-archived goal already claims in full, and
asks for an amount instead. The scope matches
`Goal.pooled_allocations_for` — archived goals are excluded from the
backing math, so they do not block; completed goals still hold their
money, so they do.
Rows written before this guard stay readable and editable. Autosave
revalidates every loaded goal_account on `goal.save`, so validating
untouched links would make a goal that merely holds a legacy overlap
impossible to rename. Only a new link, or one whose amount is being
cleared onto a contested account, is checked.
The goal fixtures encoded exactly the forbidden state — three goals
claiming `depository` in full — so tests that built a fourth whole
claim now use accounts of their own. `build_goal` mirrors the old
balance, leaving every KPI figure unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4
* fix(goals): keep a restored goal from re-claiming an account in full
Addresses review feedback on #3160, raised independently on #3165, #3166 and
#3167 — one bug seen four times, because those branches stack.
Archiving a goal releases the accounts it claimed in full, so another goal can
legitimately claim one while it is away. Restoring it then put two
whole-account links back on the same account and reopened the double-counting
`whole_account_link_must_be_exclusive` closes: that check only fires when a
link is written, and a state change writes none.
A validation rather than an AASM guard. `may_fire_event?` stays true, the save
fails, and GoalsController#perform_transition! already surfaces
`errors.full_messages` — the user reads which goal holds the account instead of
a generic "can't do that in this state".
The conflict lookup now lives in one place, `Goal#whole_account_conflicts_on`,
read both by the door (writing a link) and by the restore, so the two cannot
drift into disagreeing about which goals still hold their money. The
`new_record? || will_save_change_to_allocated_amount?` bound stays on the
validation rather than moving into the shared lookup: it exists so a goal
merely holding a legacy overlap can still be renamed.
`Goal::RELEASED_STATES` replaces the repeated "archived" literal. The old
comment already said this scope had to move with the pool's; the constant makes
that true rather than hoped for, and the restore guard will follow the day the
set grows.
Restores from a released state are guarded; `resume` from `paused` is not. A
paused goal never let go of its accounts, so nothing can legitimately have
claimed one meanwhile, and blocking it would strand a user on a goal they
merely shelved.
Two things this surfaced in the test data:
- The fixtures had three goals each claiming `depository` in full — the exact
state the rule forbids. `test "AASM transitions"` failed on it, a true
positive. Two of them now take a fixed 1,000 slice.
- `current_balance sums linked account balances` asserted the gross balance,
which only held because of that overlap. A whole-account link takes what is
left after other goals' fixed earmarks; the test now says so, and computes it
from the data rather than a constant.
Each guard was confirmed load-bearing by removing it and watching its test
fail. bin/rails test: 6936 runs, 27911 assertions, 0 failures. RuboCop,
erb_lint and Brakeman clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): treat moving a whole-account link as the fresh claim it is
Addresses review feedback on #3160.
The exclusivity check was bounded to `new_record? ||
will_save_change_to_allocated_amount?`. A persisted whole-account row whose
`account_id` or `goal_id` changes is neither, so moving one landed it on an
account nobody had checked — the same double-counting hole a restore opened,
through a different door.
The bound is widened rather than dropped. It exists because `Goal has_many
:goal_accounts, autosave: true` revalidates every loaded child on `goal.save`,
so an unguarded check makes a goal that merely holds a legacy overlap
impossible to rename. That reason still holds for every row along for the ride;
it does not hold for a row being moved. A test pins both faces.
bin/rails test: 6939 runs, 27916 assertions, 0 failures. RuboCop and Brakeman
clean. Confirmed load-bearing by narrowing the bound back and watching the move
test fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): stop a link that is changing goals from conflicting with itself
Addresses review feedback on #3160, on the widening that landed in 00894c16.
Guarding ownership changes was right, but the conflict query excluded the
current record by GOAL, not by row. A link changing `goal_id` still carries the
old one in the database, so the query handed the moving row straight back and
the link was refused as its own conflict — the guard blocked the exact case it
had just been widened to cover.
`whole_account_conflicts_on` now takes the row being written and excludes it by
id. The restore guard passes nothing, which is correct: exclusion by goal
already covers every link the goal being restored owns.
Confirmed load-bearing by dropping the id exclusion and watching the new test
fail. bin/rails test: 6940 runs, 27917 assertions, 0 failures. RuboCop and
Brakeman clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): keep the ticked accounts when a creation is rejected
Addresses review feedback on #3160.
`new.html.erb` rendered the form without `currently_linked_account_ids`, whose
default is `[]`, so a rejected create came back with every account unchecked.
The amounts survived — the form reads those off the same built links — which
made it worse rather than better: the user faced an error telling them to enter
an amount, on a form whose account selection had silently cleared, and a
multi-account selection was gone entirely.
The failed path now derives the ticks from the in-memory links rather than
`pluck`: nothing is persisted on a rejected create, so a query would come back
empty and change nothing.
Confirmed load-bearing by emptying the list again and watching the new
controller test fail. bin/rails test: 6941 runs, 27920 assertions, 0 failures.
RuboCop and erb_lint clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): hold the account while checking whether it is already claimed
The exclusivity validation is a read followed by a write, so two requests
could both find no conflict and both commit a whole-account claim — the
double-count the validation exists to prevent, recreated by timing alone.
`whole_account_conflicts_on` now takes a transaction-scoped advisory lock
per account before reading. An advisory lock rather than a row lock
because the conflicting write may be an INSERT, so there is no row to
lock; transaction-scoped so it is released whichever way the enclosing
transaction ends. Accounts are locked in id order, so two goals claiming
the same pair in opposite orders cannot deadlock against each other.
Both doors go through this method — the link validation and the restore
guard — so both are covered by the one change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): bind the advisory lock key instead of interpolating it
Brakeman flagged the hand-built SQL, correctly. The key is a digest of an
id and could not carry a payload, but a raw interpolated string in a model
is the shape a reader has to stop and verify.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): stop the advisory lock warning on every acquisition
`pg_advisory_xact_lock` returns `void`, which the adapter cannot type, so
each call logged "unknown OID 2278". Projected through a subquery so the
result set is a plain integer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): claim every account a goal touches in one deterministic order
Follow-up on the advisory lock: autosave validates each link separately, so
each was taking its own account lock in association order. Two goals saving
links on the same two accounts in opposite orders would then hold one lock
each and wait on the other.
The goal takes the whole set up front, sorted, before any child validates.
The per-account lock inside `whole_account_conflicts_on` stays for a link
saved on its own, and re-taking a lock the transaction already holds costs
nothing.
The ordering test fixes the account ids rather than generating them: the
assertion is entirely about order, and random UUIDs would have let it pass
half the time on association order alone.
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>
335 lines
13 KiB
Ruby
335 lines
13 KiB
Ruby
class GoalsController < ApplicationController
|
||
before_action :require_preview_features!
|
||
before_action :set_goal, only: %i[show edit update destroy pause resume complete archive unarchive reopen]
|
||
|
||
FUNDABLE_TYPES = Goal::FUNDABLE_ACCOUNT_TYPES
|
||
rescue_from ActiveRecord::RecordNotFound, with: :goal_not_found
|
||
|
||
STATE_FILTERS = %w[all active paused completed archived].freeze
|
||
|
||
def index
|
||
state_counts = Current.family.goals.group(:state).count
|
||
@counts = STATE_FILTERS.each_with_object({}) do |state, h|
|
||
h[state] = state == "all" ? state_counts.values.sum : (state_counts[state] || 0)
|
||
end
|
||
|
||
# Preloads + the family-wide backing-math injection (N+1 guard) live in
|
||
# Goal.prepared_for, shared with the Plan hub's active_prepared_for.
|
||
all_goals = Goal.prepared_for(Current.family)
|
||
@active_goals = Goal.active_display_sort(all_goals.reject { |g| %w[completed archived].include?(g.state) })
|
||
@completed_goals = all_goals.select { |g| g.state == "completed" }.sort_by { |g| g.name.downcase }
|
||
@archived_goals = all_goals.select { |g| g.state == "archived" }
|
||
# Completed goals join the chip-filterable grid below the active ones
|
||
# so the `completed` chip can isolate them. Archived stays in a
|
||
# separate collapsed-by-default section, opted out of the filter
|
||
# entirely (rendered with filterable: false).
|
||
@grid_goals = @active_goals + @completed_goals
|
||
|
||
@linkable_account_count = Current.user.accessible_accounts.where(accountable_type: FUNDABLE_TYPES).visible.count
|
||
@kpi = kpi_payload(@active_goals)
|
||
@any_pending_pledge = @active_goals.any? { |g| g.open_pledges.any? }
|
||
@show_search = @grid_goals.size > 6
|
||
@breadcrumbs = plan_breadcrumb_prefix + [ [ t("goals.index.title"), nil ] ]
|
||
end
|
||
|
||
def show
|
||
@open_pledges = @goal.open_pledges.reverse_chronological.to_a
|
||
@breadcrumbs = plan_breadcrumb_prefix + [
|
||
[ t("goals.index.title"), goals_path ],
|
||
[ @goal.name, nil ]
|
||
]
|
||
end
|
||
|
||
def new
|
||
@goal = Current.family.goals.new(
|
||
color: Goal::COLORS.sample,
|
||
currency: Current.family.primary_currency_code
|
||
)
|
||
@linkable_accounts = linkable_accounts_for_new
|
||
@currently_linked_account_ids = []
|
||
@breadcrumbs = plan_breadcrumb_prefix + [
|
||
[ t("goals.index.title"), goals_path ],
|
||
[ t("goals.new.heading"), nil ]
|
||
]
|
||
end
|
||
|
||
def create
|
||
@goal = Current.family.goals.new(goal_params)
|
||
accounts = lookup_accounts(params.dig(:goal, :account_ids))
|
||
@goal.currency = (accounts.first&.currency || Current.family.primary_currency_code) if @goal.currency.blank?
|
||
|
||
allocations = submitted_allocations
|
||
Goal.transaction do
|
||
accounts.each { |a| @goal.goal_accounts.build(account: a, allocated_amount: allocations[a.id.to_s]) }
|
||
@goal.save!
|
||
end
|
||
|
||
flash[:notice] = t(".success")
|
||
respond_to do |format|
|
||
format.html { redirect_to goal_path(@goal) }
|
||
format.turbo_stream do
|
||
render turbo_stream: turbo_stream.action(:redirect, goal_path(@goal))
|
||
end
|
||
end
|
||
rescue ActiveRecord::RecordInvalid
|
||
@linkable_accounts = linkable_accounts_for_new
|
||
# From the in-memory links, not `pluck`: nothing is persisted on a failed
|
||
# create, so a query would come back empty and every box the user ticked
|
||
# would render unchecked. The amounts survived — the form reads those off
|
||
# 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 }
|
||
render :new, status: :unprocessable_entity
|
||
end
|
||
|
||
def edit
|
||
@linkable_accounts = linkable_accounts_for_new
|
||
@currently_linked_account_ids = @goal.goal_accounts.pluck(:account_id).map(&:to_s)
|
||
end
|
||
|
||
def update
|
||
account_ids = params.dig(:goal, :account_ids)
|
||
accounts_supplied = !account_ids.nil?
|
||
accounts = accounts_supplied ? lookup_accounts(account_ids) : []
|
||
|
||
if accounts_supplied && accounts.empty?
|
||
@goal.errors.add(:base, :at_least_one_linked_account_required)
|
||
@linkable_accounts = linkable_accounts_for_new
|
||
@currently_linked_account_ids = @goal.goal_accounts.pluck(:account_id).map(&:to_s)
|
||
render :edit, status: :unprocessable_entity
|
||
return
|
||
end
|
||
|
||
# Assign first, sync the links, then persist once. Saving the attributes
|
||
# up front ran `must_have_at_least_one_linked_account` against the goal's
|
||
# OLD links, so a goal orphaned by account deletion (Account has_many
|
||
# :goal_accounts, dependent: :destroy) could never be edited back to
|
||
# health: the save raised before the submitted accounts were attached.
|
||
# One save over the fully-assembled goal validates what the user actually
|
||
# submitted.
|
||
Goal.transaction do
|
||
@goal.assign_attributes(goal_update_params)
|
||
if accounts_supplied
|
||
sync_linked_accounts!(@goal, accounts, submitted_allocations)
|
||
else
|
||
@goal.save!
|
||
end
|
||
end
|
||
|
||
flash[:notice] = t(".success")
|
||
respond_to do |format|
|
||
format.html { redirect_to goal_path(@goal) }
|
||
format.turbo_stream do
|
||
render turbo_stream: turbo_stream.action(:redirect, goal_path(@goal))
|
||
end
|
||
end
|
||
rescue ActiveRecord::RecordInvalid
|
||
@linkable_accounts = linkable_accounts_for_new
|
||
@currently_linked_account_ids = @goal.goal_accounts.pluck(:account_id).map(&:to_s)
|
||
render :edit, status: :unprocessable_entity
|
||
end
|
||
|
||
# Deletable from any state. Destroying a goal cascades only to its own
|
||
# goal_accounts / goal_pledges — and GoalPledge#clear_matched_transaction_extra
|
||
# unstamps the pledge id it wrote onto a matched transaction. No account,
|
||
# balance, entry or transaction is removed, so the archive-first gate this
|
||
# used to enforce bought no safety; it only hid the action behind a two-step
|
||
# flow no other Sure resource requires.
|
||
def destroy
|
||
@goal.destroy!
|
||
redirect_to goals_path, notice: t(".success")
|
||
end
|
||
|
||
def pause
|
||
perform_transition!(:pause)
|
||
end
|
||
|
||
def resume
|
||
perform_transition!(:resume)
|
||
end
|
||
|
||
def complete
|
||
perform_transition!(:complete)
|
||
end
|
||
|
||
def archive
|
||
perform_transition!(:archive)
|
||
end
|
||
|
||
def unarchive
|
||
perform_transition!(:unarchive)
|
||
end
|
||
|
||
def reopen
|
||
perform_transition!(:reopen)
|
||
end
|
||
|
||
private
|
||
def set_goal
|
||
@goal = Current.family.goals
|
||
.includes(:open_pledges, linked_accounts: :account_providers)
|
||
.find(params[:id])
|
||
end
|
||
|
||
def goal_not_found
|
||
redirect_to goals_path, alert: t("goals.errors.not_found")
|
||
end
|
||
|
||
def goal_params
|
||
params.require(:goal).permit(:name, :target_amount, :target_date, :color, :icon, :notes)
|
||
end
|
||
|
||
def goal_update_params
|
||
params.require(:goal).permit(:name, :target_amount, :target_date, :color, :icon, :notes)
|
||
end
|
||
|
||
def lookup_accounts(ids)
|
||
return [] if ids.blank?
|
||
|
||
ids = Array(ids).reject(&:blank?)
|
||
Current.user.accessible_accounts.where(accountable_type: FUNDABLE_TYPES).visible.where(id: ids).to_a
|
||
end
|
||
|
||
def linkable_accounts_for_new
|
||
Current.user.accessible_accounts.where(accountable_type: FUNDABLE_TYPES).visible.alphabetically.to_a
|
||
end
|
||
|
||
def sync_linked_accounts!(goal, accounts, allocations = {})
|
||
desired_ids = accounts.map(&:id).to_set
|
||
current_ids = goal.goal_accounts.pluck(:account_id).to_set
|
||
|
||
# Only unlink accounts the current user can actually see in the picker.
|
||
# A family goal may be linked to another member's private account, which
|
||
# never renders as a checkbox — so its absence from the submitted set is
|
||
# not an intentional removal and must not destroy the link.
|
||
removable_ids = Current.user.accessible_accounts.where(id: current_ids.to_a).pluck(:id).to_set
|
||
|
||
((current_ids & removable_ids) - desired_ids).each do |id|
|
||
goal.goal_accounts.where(account_id: id).destroy_all
|
||
end
|
||
goal.goal_accounts.reload
|
||
|
||
# Add new links and refresh the earmark on kept links. Only touch the
|
||
# allocation when the form actually submitted a value for that account
|
||
# (allocations.key?), so a caller that omits the hash leaves earmarks
|
||
# untouched. Save through the goal so currency / depository / family
|
||
# validations fire — create! on goal_accounts bypasses them.
|
||
accounts.each do |account|
|
||
existing = goal.goal_accounts.find { |ga| ga.account_id == account.id }
|
||
if existing
|
||
existing.allocated_amount = allocations[account.id.to_s] if allocations.key?(account.id.to_s)
|
||
else
|
||
goal.goal_accounts.build(account: account, allocated_amount: allocations[account.id.to_s])
|
||
end
|
||
end
|
||
goal.save!
|
||
end
|
||
|
||
# { account_id_string => amount_string_or_nil } from goal[allocations].
|
||
# A blank amount means "dedicate the whole balance" (NULL allocated_amount).
|
||
def submitted_allocations
|
||
raw = params.dig(:goal, :allocations)
|
||
return {} if raw.blank?
|
||
|
||
hash = raw.respond_to?(:to_unsafe_h) ? raw.to_unsafe_h : raw
|
||
hash.each_with_object({}) do |(account_id, amount), memo|
|
||
memo[account_id.to_s] = amount.to_s.strip.presence
|
||
end
|
||
end
|
||
|
||
def kpi_payload(active_goals)
|
||
family = Current.family
|
||
currency = family.primary_currency_code
|
||
today = Date.current
|
||
|
||
windows = family.savings_inflow_windows(window_days: 30, now: today)
|
||
velocity_30d = windows[:current]
|
||
velocity_prior_30d = windows[:prior]
|
||
delta_amount = velocity_30d - velocity_prior_30d
|
||
delta_percent = velocity_prior_30d.zero? ? nil : ((delta_amount / velocity_prior_30d.abs) * 100).round(1)
|
||
|
||
# Sign decoupling: the headline-amount sign reflects this month's
|
||
# direction ("−$200 last 30d" = net outflow); the delta direction
|
||
# (↑/↓ vs prior 30d) goes on the subline. Conflating them produced the
|
||
# "−$1234" + "↓ 27%" tile where the minus looked like a loss but the
|
||
# $1234 was actually the (positive) amount contributed.
|
||
headline_sign = velocity_30d.negative? ? "−" : ""
|
||
delta_direction = if delta_amount.positive? then :up
|
||
elsif delta_amount.negative? then :down
|
||
else :flat
|
||
end
|
||
|
||
# behind_pace? excludes paused goals — pausing stops the pace clock,
|
||
# so a paused-but-behind goal belongs in the paused count (below),
|
||
# not in "N behind" or the "needs this month" sum. Keeps this tile
|
||
# consistent with the Plan hub's summary (Goal.summary_for).
|
||
needs = active_goals
|
||
.select(&:behind_pace?)
|
||
.sum { |g| g.monthly_target_amount.to_d }
|
||
behind = active_goals.count(&:behind_pace?)
|
||
# Paused goals are excluded from the on-track numerator for the same
|
||
# reason they're excluded from tracked_total below — otherwise the
|
||
# "X of Y" fraction could exceed its own denominator.
|
||
on_track = active_goals.count { |g| !g.paused? && g.status == :on_track }
|
||
reached = active_goals.count { |g| g.status == :reached }
|
||
no_date = active_goals.count { |g| g.status == :no_target_date }
|
||
paused = active_goals.count(&:paused?)
|
||
|
||
# Denominator of the "Goals on track" tile. A goal only belongs in
|
||
# the fraction if there is a benchmark to compare against:
|
||
# - reached → target already hit, no longer tracked toward pace
|
||
# - paused → user stopped the pace clock on purpose
|
||
# - no_target_date → open-ended saving (emergency fund, sabbatical
|
||
# fund, etc.) has no required monthly pace, so "on track" is
|
||
# undefined. Counting it would penalise the user for having
|
||
# open-ended goals — they'd never improve the ratio.
|
||
# When this hits zero the tile swaps to a celebration / empty
|
||
# state in the view.
|
||
tracked_total = active_goals.count do |g|
|
||
!g.paused? && g.status != :reached && g.status != :no_target_date
|
||
end
|
||
|
||
{
|
||
currency: currency,
|
||
velocity_30d_money: Money.new(velocity_30d.abs, currency),
|
||
velocity_prior_30d_money: Money.new(velocity_prior_30d.abs, currency),
|
||
velocity_30d_sign: headline_sign,
|
||
velocity_delta_percent: delta_percent,
|
||
velocity_direction: delta_direction,
|
||
needs_this_month_money: Money.new(needs, currency),
|
||
on_track_count: on_track,
|
||
reached_count: reached,
|
||
behind_count: behind,
|
||
no_date_count: no_date,
|
||
paused_count: paused,
|
||
tracked_total: tracked_total,
|
||
active_total: active_goals.size
|
||
}
|
||
end
|
||
|
||
def perform_transition!(event)
|
||
unless @goal.aasm.may_fire_event?(event)
|
||
redirect_to goal_path(@goal), alert: t(".invalid_transition")
|
||
return
|
||
end
|
||
|
||
# AASM's bang event returns false — it does NOT raise — when the save
|
||
# that persists the new state fails validation. The return value used to
|
||
# be discarded, so an invalid goal (e.g. orphaned by account deletion)
|
||
# flashed "Goal archived." while the state never moved. Surface the
|
||
# validation error instead of claiming success.
|
||
unless @goal.public_send("#{event}!")
|
||
redirect_to goal_path(@goal),
|
||
alert: @goal.errors.full_messages.to_sentence.presence || t(".invalid_transition")
|
||
return
|
||
end
|
||
|
||
respond_to do |format|
|
||
format.html { redirect_to goal_path(@goal), notice: t(".success") }
|
||
format.turbo_stream do
|
||
render turbo_stream: turbo_stream.action(:redirect, goal_path(@goal))
|
||
end
|
||
end
|
||
end
|
||
end
|