mirror of
https://github.com/we-promise/sure.git
synced 2026-09-06 07:11:14 +00:00
* feat(goals): let a goal be spent without looking like it fell behind Coming home from the holiday a goal paid for dropped it from 100% to 20%. The money went where it was meant to go, and the app read that as failure. The only way back was to edit the target — falsifying what the user had actually set out to save. `consumed_amount` records what was spent ON the thing the goal was for, and progress reads `(backing + consumed) / target`. Spending the money is no longer indistinguishable from losing it. **The part that is easy to miss.** `consume!` also shrinks the earmark on the account by the same amount. Without that, money the user has already spent stays reserved and keeps its share away from every sibling goal — the exact double-counting the exclusivity rules exist to prevent, arriving through the back door. A test pins it through the pro-rata haircut, where the effect is visible: a sibling's backing grows as the spent share is released. **Kept separate from `completed_amount`.** That one freezes the BACKING at closure; folding consumption into it would count the same money twice on a goal partly spent and then closed. A test asserts each side is counted once. **A reserve refuses consumption outright.** It is drawn down and refilled, not spent, and recording a withdrawal as consumption would erase the shortfall the reserve exists to report. `account:` may be omitted only when the goal has one link — with several, guessing would silently pick a side. The controller refuses an account id that resolves to nothing rather than falling back to nil, which on a single-link goal would have recorded the spend against an account the user never named. The write is its own action rather than a verb branch inside `consume`: HEAD routes like GET but `request.get?` is false for it, so a branch would send a HEAD request down the write path. Brakeman caught that. bin/rails test: 7088 runs, 28510 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): make a spend either happen entirely or not at all Review on #3176 found the recorded spend and the released earmark could drift apart, and that nothing stopped the two figures being edited out of agreement afterwards. The lock was on the link, not the goal. `consumed_amount` lives on the goal, so two concurrent requests locking only their own links both read the same old value, both passed the target check, and both added to it. The whole check-and-write now runs under `with_lock` on the goal. Consuming more than the chosen link held was silently clamped: the link released what it had while `consumed_amount` took the full figure, so money counted as spent stayed reserved against every sibling goal. It is refused now — `:exceeds_earmark` — rather than half-applied. A dialog left open in another tab could still post to a goal that had since been completed or archived; `:not_active` closes that. Two validations stop the pair being separated after the fact: a goal that has recorded a spend cannot become a reserve (reserves refuse consumption, so the figure would count toward progress on an object whose model treats spending as a shortfall), and the target cannot be lowered below what was already spent. Consuming cleared the columns and left the memos standing, so an instance that had already read its backing kept reporting the pre-spend figure. Progress holding steady is the feature — the earmark shrinks by what consumption grows by — which is exactly what hid the stale backing. Also from review: `accessible_accounts` rather than the whole family for the account picker, `DS::Select` rather than a bare `select_tag`, the flash amount through `Money#format`, and the French label for the menu entry, which I had left untranslated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * refactor(goals): reuse the cache reset the class already owns Review pointed out `reset_state_dependent_caches!` exists for exactly this, and that hand-rolling a second ivar list was the wrong shape. It was also wrong in substance: mine omitted `@pooled_allocations`, and consuming shrinks a link's allocation, which is precisely what the pool is computed from. One list, kept in one place, stays right when a memo is added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * docs(goals): put four comments back on the methods they describe Rebases had stranded them: a paragraph about clearing memos on an AASM transition, two about what reopening does to a frozen figure, and one about `reload` leaving memos standing had all piled up in front of `consumption_link_for`, which does none of those things. The last is dropped rather than moved — its explanation now sits at the call site in `consume!`, where the reset actually happens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): keep a private backing account out of the consume dialog A goal can be backed by an account the viewer is not allowed to see, and the first pass only guarded the named-account path. Two ways round it remained. The dialog listed every link, so it named private accounts outright. And with `account_id` left blank the model picked the sole link on its own, without anyone having checked the viewer could reach it — so a direct POST reduced a private account's earmark, the figures moving afterwards saying roughly how much was in it. The controller now derives the eligible links from `Current.user.accessible_accounts`, the dialog renders those, and a blank id resolves only to a sole *eligible* account. With none the request is refused; with several it stays nil and the model asks, as before. Naming the account explicitly matters even when the goal has several links: the one the viewer can reach is not necessarily the one the model would have picked. 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>
390 lines
16 KiB
Ruby
390 lines
16 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 consume record_consumption]
|
||
|
||
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
|
||
|
||
# Renders the dialog. The write lives in its own action below.
|
||
def consume
|
||
@consumption_accounts = eligible_consumption_accounts
|
||
end
|
||
|
||
def record_consumption
|
||
amount = params[:amount].to_d
|
||
@goal.consume!(amount, account: consumption_account)
|
||
redirect_to goal_path(@goal),
|
||
notice: t("goals.consume.success", amount: Money.new(amount, @goal.currency).format)
|
||
rescue Goal::ConsumptionRefused => e
|
||
redirect_to goal_path(@goal), alert: t("goals.consume.errors.#{e.reason}")
|
||
rescue ActiveRecord::RecordInvalid => e
|
||
redirect_to goal_path(@goal), alert: e.record.errors.full_messages.to_sentence
|
||
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, :kind)
|
||
end
|
||
|
||
def goal_update_params
|
||
params.require(:goal).permit(:name, :target_amount, :target_date, :color, :icon, :notes, :kind)
|
||
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.
|
||
# - maintained → a reserve has no deadline and no pace benchmark at
|
||
# all. Its statuses are `funded`/`depleted`, which match none of the
|
||
# exclusions above, so without this it would land in the denominator
|
||
# and never in the numerator: a family with one reserve read
|
||
# "0 of 1 on track" for a goal that is working exactly as intended.
|
||
# 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.maintained? && 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
|
||
|
||
# A blank id means "the goal has one link, use it" and the model decides
|
||
# whether that is true. An id that resolves to nothing is refused here
|
||
# rather than falling back to nil: on a single-link goal, nil would consume
|
||
# from that link and the user would see a spend recorded against an account
|
||
# they did not name.
|
||
# The goal's links narrowed to what the VIEWER may see. A goal can be
|
||
# backed by a private account, and both halves of that leak matter: the
|
||
# dialog would name an account the reader is not allowed to know exists,
|
||
# and a direct POST would reduce its earmark — the figures moving
|
||
# afterwards saying how much was in it.
|
||
def eligible_consumption_accounts
|
||
@eligible_consumption_accounts ||= begin
|
||
linked = @goal.linked_accounts
|
||
visible_ids = Current.user.accessible_accounts.where(id: linked.map(&:id)).pluck(:id).to_set
|
||
linked.select { |account| visible_ids.include?(account.id) }
|
||
end
|
||
end
|
||
|
||
def consumption_account
|
||
eligible = eligible_consumption_accounts
|
||
raise Goal::ConsumptionRefused.new(:account_not_linked) if eligible.empty?
|
||
|
||
if params[:account_id].present?
|
||
return eligible.find { |account| account.id.to_s == params[:account_id].to_s } ||
|
||
raise(Goal::ConsumptionRefused.new(:account_not_linked))
|
||
end
|
||
|
||
# Named explicitly even when the goal has several links, because the one
|
||
# the viewer can reach is not necessarily the one the model would pick on
|
||
# its own. With more than one eligible link it stays nil, and the model
|
||
# refuses rather than guessing.
|
||
eligible.size == 1 ? eligible.first : nil
|
||
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
|