mirror of
https://github.com/we-promise/sure.git
synced 2026-05-29 23:39:03 +00:00
fix(goals): address AI review on PR #1798 (CodeRabbit + Codex)
Correctness: - GoalPledge#matches? rejects outflows on transfer pledges so a +$200 purchase no longer satisfies a $200 deposit pledge after .abs - GoalsController#sync_linked_accounts! saves through the goal so currency/depository/family validations actually run on update - AlreadyClaimedError replaces empty RecordInvalid in resolve_with! and reconciler rescues the dedicated class - SweepExpiredGoalPledgesJob wraps each expire! in a per-record rescue - Assistant::Function::CreateGoal disambiguates duplicate account names and returns an absolute URL via mailer host config - Family#savings_inflow_velocity defensively scopes from the family's accounts (was Account.joins(:goal_accounts).where(goal_id: ...)) - GoalPledgesController#set_goal preloads linked_accounts + providers to drop the N+1 on any_connected_account? - Stepper subtitle update walks to the enclosing dialog before querySelector so two stepper instances don't fight over one header - categories/_form.html.erb data-action targets color-icon-picker, not the non-existent "category" controller UX / visual: - Projection chart drops preserveAspectRatio="none" and pins endDate at today for past-due goals so the today marker stays in-domain - _color_picker / categories form swap non-standard border-1 for border - Goals index search input uses ring-alpha-black-100 (was raw gray-500) Refactors: - Goal#header_summary extracts the multi-line ERB header block - Goal#catch_up_delta_money sums open_pledges in SQL - Goal#projection_summary uses I18n.l for the on-track month label - Account#default_pledge_kind moves the manual/transfer decision out of GoalPledgesController - GoalPledge::Reconciler iterates ordered (created_at, id) so first-claim wins is deterministic under non-sequential PKs - Goals::FundingAccountsBreakdownComponent + Goals::AccountStackComponent use clamp(0..) instead of Float::INFINITY / [x, 0].max - Goals::StatusPillComponent#label provides a titleize fallback - Goal projection chart skips the redundant initial _draw and reuses the snapped point in the past branch (no double-bisect) - Goal pledge preview drops maximumFractionDigits: 0 so USD/EUR show cents while JPY/KRW stay whole-unit - Demo generator captures the Wedding fund goal in the seed loop instead of looking it up by hardcoded name Tests: - GoalPledgeTest: outflow rejection - GoalsControllerTest: cross-currency attachment rejected on update - SweepExpiredGoalPledgesJobTest: cancelled coverage + per-record rescue - GoalTest: pledge_action_label_key flips to manual_save without an unconditional guard
This commit is contained in:
Guillem Arias
@@ -10,7 +10,7 @@ class Goals::AccountStackComponent < ApplicationComponent
|
||||
end
|
||||
|
||||
def extra_count
|
||||
[ @accounts.size - @max, 0 ].max
|
||||
(@accounts.size - @max).clamp(0..)
|
||||
end
|
||||
|
||||
def initial_for(account)
|
||||
|
||||
@@ -75,7 +75,7 @@ class Goals::FundingAccountsBreakdownComponent < ApplicationComponent
|
||||
|
||||
result = Hash.new { |h, k| h[k] = { last_30: 0.to_d, last_90: 0.to_d } }
|
||||
rows.each do |aid, date, amount|
|
||||
inflow = (-amount.to_d).clamp(0, Float::INFINITY)
|
||||
inflow = (-amount.to_d).clamp(0..)
|
||||
result[aid][:last_90] += inflow
|
||||
result[aid][:last_30] += inflow if date >= cutoff_30
|
||||
end
|
||||
|
||||
@@ -29,7 +29,7 @@ class Goals::StatusPillComponent < ApplicationComponent
|
||||
end
|
||||
|
||||
def label
|
||||
I18n.t("goals.status.#{status_key}")
|
||||
I18n.t("goals.status.#{status_key}", default: status_key.to_s.titleize)
|
||||
end
|
||||
|
||||
def classes
|
||||
|
||||
@@ -16,7 +16,7 @@ class GoalPledgesController < ApplicationController
|
||||
@pledge = @goal.goal_pledges.new(
|
||||
currency: @goal.currency,
|
||||
account: account,
|
||||
kind: kind_for_account(account),
|
||||
kind: account&.default_pledge_kind || "transfer",
|
||||
amount: params[:amount].presence
|
||||
)
|
||||
end
|
||||
@@ -24,7 +24,7 @@ class GoalPledgesController < ApplicationController
|
||||
def create
|
||||
@pledge = @goal.goal_pledges.new(pledge_params)
|
||||
@pledge.account = lookup_account(params.dig(:goal_pledge, :account_id))
|
||||
@pledge.kind = kind_for_account(@pledge.account)
|
||||
@pledge.kind = @pledge.account&.default_pledge_kind || "transfer"
|
||||
@pledge.currency = @goal.currency
|
||||
|
||||
if @pledge.save
|
||||
@@ -56,7 +56,12 @@ class GoalPledgesController < ApplicationController
|
||||
|
||||
private
|
||||
def set_goal
|
||||
@goal = Current.family.goals.find(params[:goal_id])
|
||||
# Preload linked accounts + their providers so any_connected_account?
|
||||
# and the new-pledge form's per-account helpers don't trigger N+1
|
||||
# queries on account_providers.
|
||||
@goal = Current.family.goals
|
||||
.includes(:open_pledges, linked_accounts: :account_providers)
|
||||
.find(params[:goal_id])
|
||||
end
|
||||
|
||||
def set_pledge
|
||||
@@ -77,17 +82,6 @@ class GoalPledgesController < ApplicationController
|
||||
requested || @goal.linked_accounts.first
|
||||
end
|
||||
|
||||
# Per-account: manual accounts get a `manual_save` pledge (resolves on the
|
||||
# user's next valuation), connected accounts get a `transfer` pledge
|
||||
# (resolves when the synced deposit posts). Account-level avoids the
|
||||
# mixed-funding goal bug where the goal-level toggle picked one kind for
|
||||
# all pledges regardless of which account the user actually moved money
|
||||
# into.
|
||||
def kind_for_account(account)
|
||||
return "transfer" if account.nil?
|
||||
account.manual? ? "manual_save" : "transfer"
|
||||
end
|
||||
|
||||
def record_not_found
|
||||
redirect_to goals_path, alert: t("goals.errors.not_found")
|
||||
end
|
||||
|
||||
@@ -175,15 +175,18 @@ class GoalsController < ApplicationController
|
||||
end
|
||||
|
||||
def sync_linked_accounts!(goal, accounts)
|
||||
desired = accounts.map(&:id).to_set
|
||||
current = goal.goal_accounts.pluck(:account_id).to_set
|
||||
desired_ids = accounts.map(&:id).to_set
|
||||
current_ids = goal.goal_accounts.pluck(:account_id).to_set
|
||||
|
||||
(current - desired).each do |id|
|
||||
(current_ids - desired_ids).each do |id|
|
||||
goal.goal_accounts.where(account_id: id).destroy_all
|
||||
end
|
||||
(desired - current).each do |id|
|
||||
goal.goal_accounts.create!(account_id: id)
|
||||
end
|
||||
additions = accounts.reject { |a| current_ids.include?(a.id) }
|
||||
additions.each { |a| goal.goal_accounts.build(account: a) }
|
||||
# Save through the goal so currency / depository / family
|
||||
# validations fire. `create!` on goal_accounts directly bypasses them
|
||||
# and let cross-currency / non-depository attachments through.
|
||||
goal.save!
|
||||
end
|
||||
|
||||
def kpi_payload(active_goals)
|
||||
|
||||
@@ -75,13 +75,16 @@ export default class extends Controller {
|
||||
|
||||
#money(value) {
|
||||
try {
|
||||
// Let Intl pick the currency-specific default fraction digits so
|
||||
// USD/EUR previews show cents while JPY/KRW stay whole-unit. The
|
||||
// server saves the user-entered amount verbatim; the preview must
|
||||
// not silently round it.
|
||||
return new Intl.NumberFormat(undefined, {
|
||||
style: "currency",
|
||||
currency: this.currencyValue || "USD",
|
||||
maximumFractionDigits: 0,
|
||||
}).format(value);
|
||||
} catch {
|
||||
return `${this.currencyValue || "$"}${Math.round(value).toLocaleString()}`;
|
||||
return `${this.currencyValue || "$"}${value.toLocaleString()}`;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -14,15 +14,16 @@ export default class extends Controller {
|
||||
static values = { data: Object, ariaLabel: String, ariaDescription: String };
|
||||
|
||||
connect() {
|
||||
this._draw();
|
||||
this._resize = this._draw.bind(this);
|
||||
window.addEventListener("resize", this._resize);
|
||||
// Container may have 0 width on initial connect (Turbo restoration,
|
||||
// hidden parent, etc). Re-draw whenever the box settles into a real
|
||||
// size.
|
||||
// size. The first observer callback also performs the initial paint.
|
||||
if (typeof ResizeObserver !== "undefined") {
|
||||
this._observer = new ResizeObserver(() => this._draw());
|
||||
this._observer.observe(this.element);
|
||||
} else {
|
||||
this._draw();
|
||||
}
|
||||
// Repaint when the user toggles theme so SVG attributes (which bake
|
||||
// light/dark hex values at draw time) follow data-theme. Lives here
|
||||
@@ -87,7 +88,11 @@ export default class extends Controller {
|
||||
const currentAmount = data.current_amount || 0;
|
||||
const avgMonthly = data.avg_monthly || 0;
|
||||
|
||||
const endDate = target || new Date(today.getTime() + 30 * 24 * 60 * 60 * 1000);
|
||||
// Past-due goals: pin endDate at today so the "today" marker stays inside
|
||||
// the x-domain instead of clipping right at the edge.
|
||||
const endDate = target
|
||||
? new Date(Math.max(target.getTime(), today.getTime()))
|
||||
: new Date(today.getTime() + 30 * 24 * 60 * 60 * 1000);
|
||||
|
||||
// Drop any same-day-or-later points from the balance series: we own the
|
||||
// endpoint with `currentAmount` (live `linked_accounts.sum(:balance)`)
|
||||
@@ -144,8 +149,7 @@ export default class extends Controller {
|
||||
.append("svg")
|
||||
.attr("width", width)
|
||||
.attr("height", height)
|
||||
.attr("viewBox", `0 0 ${width} ${height}`)
|
||||
.attr("preserveAspectRatio", "none");
|
||||
.attr("viewBox", `0 0 ${width} ${height}`);
|
||||
|
||||
// Drop the <title> child; browsers render it as a native hover tooltip
|
||||
// that fights with our own crosshair tooltip. aria-label gives the same
|
||||
@@ -502,12 +506,10 @@ export default class extends Controller {
|
||||
hoverSavedDot.style("display", "none");
|
||||
lines.push(`Projected: ${this._fmtMoney(projValue, data.currency)}`);
|
||||
} else {
|
||||
// Saved segment: snap saved dot to the nearest contribution; no
|
||||
// projection dot in the past.
|
||||
const i = bisectDate(savedSeries, hoverDate);
|
||||
const a = savedSeries[Math.max(0, i - 1)];
|
||||
const b = savedSeries[Math.min(savedSeries.length - 1, i)];
|
||||
const savedPoint = !a ? b : !b ? a : (hoverDate - a.date < b.date - hoverDate ? a : b);
|
||||
// Saved segment: hoverDate is already snapped to nearest savedSeries
|
||||
// entry above, so reuse that entry directly instead of running
|
||||
// bisectDate a second time.
|
||||
const savedPoint = savedSeries.find((p) => p.date.getTime() === hoverDate.getTime()) || savedSeries[savedSeries.length - 1];
|
||||
hoverSavedDot.attr("cx", x(savedPoint.date)).attr("cy", y(savedPoint.value)).style("display", null);
|
||||
hoverProjDot.style("display", "none");
|
||||
lines.push(`Saved: ${this._fmtMoney(savedPoint.value, data.currency)}`);
|
||||
|
||||
@@ -204,8 +204,11 @@ export default class extends Controller {
|
||||
this.stepperLineTarget.classList.toggle("border-subdued", this.currentStep === 1);
|
||||
}
|
||||
// Modal subtitle lives in the dialog header, outside this controller's
|
||||
// DOM scope. Locate it by attribute and update directly.
|
||||
const subtitle = document.querySelector('[data-goal-stepper-modal-subtitle]');
|
||||
// DOM scope. Walk up to the enclosing dialog first so two stepper
|
||||
// instances on the same page (eg. edit modal opened over the new modal)
|
||||
// each update their own header rather than the first match in the DOM.
|
||||
const dialog = this.element.closest("dialog, [role='dialog']");
|
||||
const subtitle = (dialog || document).querySelector("[data-goal-stepper-modal-subtitle]");
|
||||
if (subtitle) {
|
||||
subtitle.textContent =
|
||||
this.currentStep === 1 ? this.step1SubtitleValue : this.step2SubtitleValue;
|
||||
|
||||
@@ -1,7 +1,14 @@
|
||||
class SweepExpiredGoalPledgesJob < ApplicationJob
|
||||
queue_as :scheduled
|
||||
|
||||
# Per-record rescue so one bad pledge (lock contention, missing FK,
|
||||
# stale row) doesn't abort the sweep and leave the rest open forever.
|
||||
def perform
|
||||
GoalPledge.open_and_expired_now.find_each(&:expire!)
|
||||
GoalPledge.open_and_expired_now.find_each do |pledge|
|
||||
pledge.expire!
|
||||
rescue => e
|
||||
Rails.logger.error("SweepExpiredGoalPledgesJob: pledge ##{pledge.id} expire failed: #{e.class}: #{e.message}")
|
||||
Sentry.capture_exception(e) if defined?(Sentry)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -355,6 +355,15 @@ class Account < ApplicationRecord
|
||||
simplefin_account_id.blank?
|
||||
end
|
||||
|
||||
# Default GoalPledge kind for this account. Manual accounts get
|
||||
# `manual_save` (resolves on the next valuation), live-synced accounts
|
||||
# get `transfer` (resolves when the synced deposit posts). Keeps the
|
||||
# decision in one place so the new-pledge controller / preview helper
|
||||
# can't disagree on what they're going to save.
|
||||
def default_pledge_kind
|
||||
manual? ? "manual_save" : "transfer"
|
||||
end
|
||||
|
||||
def logo_url
|
||||
if institution_domain.present? && Setting.brand_fetch_client_id.present?
|
||||
logo_size = Setting.brand_fetch_logo_size
|
||||
|
||||
@@ -82,8 +82,8 @@ class Assistant::Function::CreateGoal < Assistant::Function
|
||||
)
|
||||
end
|
||||
|
||||
matched = family.accounts.where(accountable_type: "Depository").visible.where(name: linked_account_names).to_a
|
||||
missing = linked_account_names - matched.map(&:name)
|
||||
available = family.accounts.where(accountable_type: "Depository").visible.where(name: linked_account_names)
|
||||
missing = linked_account_names - available.pluck(:name).uniq
|
||||
if missing.any?
|
||||
return error(
|
||||
"unknown_accounts",
|
||||
@@ -93,6 +93,22 @@ class Assistant::Function::CreateGoal < Assistant::Function
|
||||
)
|
||||
end
|
||||
|
||||
# Multiple accounts can share a name. Block silent over-linking by
|
||||
# surfacing the ambiguity so the assistant re-asks with disambiguated
|
||||
# input rather than attaching every same-named account to the goal.
|
||||
grouped = available.group_by(&:name)
|
||||
ambiguous_names = grouped.select { |_, accts| accts.size > 1 }.keys
|
||||
if ambiguous_names.any?
|
||||
return error(
|
||||
"ambiguous_accounts",
|
||||
"Multiple accounts share a name. Ask the user which one to use.",
|
||||
ambiguous_names: ambiguous_names,
|
||||
available_accounts: depository_account_payload
|
||||
)
|
||||
end
|
||||
|
||||
matched = linked_account_names.map { |name| grouped[name].first }
|
||||
|
||||
currencies = matched.map(&:currency).uniq
|
||||
if currencies.size > 1
|
||||
return error(
|
||||
@@ -122,15 +138,28 @@ class Assistant::Function::CreateGoal < Assistant::Function
|
||||
target_amount_formatted: goal.target_amount_money.format,
|
||||
currency: goal.currency,
|
||||
target_date: goal.target_date&.iso8601,
|
||||
url: Rails.application.routes.url_helpers.goal_path(goal),
|
||||
url: absolute_url_for(goal),
|
||||
linked_account_names: matched.map(&:name),
|
||||
message: "Created goal '#{goal.name}' (target #{goal.target_amount_money.format}). View it at #{Rails.application.routes.url_helpers.goal_path(goal)}."
|
||||
message: "Created goal '#{goal.name}' (target #{goal.target_amount_money.format}). View it at #{absolute_url_for(goal)}."
|
||||
}
|
||||
rescue ActiveRecord::RecordInvalid => e
|
||||
error("validation_failed", e.record.errors.full_messages.join("; "))
|
||||
end
|
||||
|
||||
private
|
||||
# Build an absolute URL for the new goal so chat clients (which render
|
||||
# outside the request that produced the goal) can link directly. Falls
|
||||
# back to the relative path when no host is configured (e.g. self-hosted
|
||||
# in a job without ENV).
|
||||
def absolute_url_for(goal)
|
||||
host_opts = Rails.application.config.action_mailer.default_url_options || {}
|
||||
if host_opts[:host].present?
|
||||
Rails.application.routes.url_helpers.goal_url(goal, host_opts)
|
||||
else
|
||||
Rails.application.routes.url_helpers.goal_path(goal)
|
||||
end
|
||||
end
|
||||
|
||||
def parse_decimal(value)
|
||||
return nil if value.nil?
|
||||
BigDecimal(value.to_s)
|
||||
|
||||
@@ -1285,63 +1285,68 @@ class Demo::Generator
|
||||
currency = depository_accounts.first.currency
|
||||
eligible = depository_accounts.select { |a| a.currency == currency }
|
||||
primary = eligible.first
|
||||
secondary = (eligible - [ primary ]).first || primary
|
||||
|
||||
# Demo coverage matrix. Picks targets + target_dates so every visible
|
||||
# goal surface fires on at least one card:
|
||||
# Demo coverage matrix. The demo seeds a heavy primary checking
|
||||
# balance (~$150k) plus a smaller secondary account (~$10k). To
|
||||
# surface every goal state we deliberately route goals to different
|
||||
# account pools so progress lands above or below the target:
|
||||
#
|
||||
# AASM states: active, paused, completed, archived
|
||||
# Computed status (on active goals):
|
||||
# :reached, :on_track, :behind, :no_target_date
|
||||
# Edge surfaces: past-due target_date ("was due"), open pledge
|
||||
# banner, matched pledge ("last pledge matched")
|
||||
goals = [
|
||||
# active · behind (short timeline + non-trivial target)
|
||||
# active · behind — secondary account only, target above its balance
|
||||
{
|
||||
name: "Vacation in Italy",
|
||||
target: 5_000,
|
||||
target: 20_000,
|
||||
target_date: 4.months.from_now.to_date,
|
||||
accounts: eligible.first(2),
|
||||
accounts: [ secondary ],
|
||||
pledges: [
|
||||
{ account: primary, amount: 250, kind: "transfer", status: "open", expires_at: 5.days.from_now }
|
||||
{ account: secondary, amount: 250, kind: "transfer", status: "open", expires_at: 5.days.from_now }
|
||||
]
|
||||
},
|
||||
# active · on_track-ish (small target, year out — required rate fits any reasonable pace)
|
||||
# active · reached — primary balance comfortably above target
|
||||
{
|
||||
name: "Wedding fund",
|
||||
target: 2_400,
|
||||
target_date: 12.months.from_now.to_date,
|
||||
accounts: eligible.first(2)
|
||||
},
|
||||
# active · no_target_date — exercises the open-ended branch
|
||||
{
|
||||
name: "Emergency fund",
|
||||
target: 10_000,
|
||||
target_date: nil,
|
||||
accounts: [ primary ]
|
||||
},
|
||||
# active · behind (large multi-year target — no realistic pace covers $50k/24mo)
|
||||
# active · no_target_date — secondary so progress doesn't auto-cap at 100%
|
||||
{
|
||||
name: "Emergency fund",
|
||||
target: 30_000,
|
||||
target_date: nil,
|
||||
accounts: [ secondary ]
|
||||
},
|
||||
# active · behind big — combined pools still well short of the target
|
||||
{
|
||||
name: "House downpayment",
|
||||
target: 50_000,
|
||||
target: 500_000,
|
||||
target_date: 24.months.from_now.to_date,
|
||||
accounts: eligible.first(2),
|
||||
pledges: [
|
||||
{ account: primary, amount: 2_000, kind: "transfer", status: "open", expires_at: 4.days.from_now }
|
||||
]
|
||||
},
|
||||
# active · reached (target intentionally below any plausible primary balance)
|
||||
# active · on_track — primary balance close to target, long horizon makes
|
||||
# the required monthly rate small enough for the demo's pace to cover
|
||||
{
|
||||
name: "Coffee gear",
|
||||
target: 150,
|
||||
target_date: 8.months.from_now.to_date,
|
||||
name: "Long-term portfolio",
|
||||
target: 200_000,
|
||||
target_date: 60.months.from_now.to_date,
|
||||
accounts: [ primary ]
|
||||
},
|
||||
# active · past-due (target_date in the past — exercises "was due" header copy
|
||||
# and the months_remaining = 0 branch in monthly_target_amount)
|
||||
# active · past-due — exercises "was due" header copy + the
|
||||
# months_remaining = 0 branch in monthly_target_amount
|
||||
{
|
||||
name: "Tax prep buffer",
|
||||
target: 1_200,
|
||||
target_date: 2.months.ago.to_date,
|
||||
accounts: [ primary ]
|
||||
accounts: [ secondary ]
|
||||
},
|
||||
# AASM paused
|
||||
{
|
||||
@@ -1369,6 +1374,7 @@ class Demo::Generator
|
||||
}
|
||||
]
|
||||
|
||||
wedding_goal = nil
|
||||
goals.each do |goal_spec|
|
||||
goal = family.goals.new(
|
||||
name: goal_spec[:name],
|
||||
@@ -1380,6 +1386,7 @@ class Demo::Generator
|
||||
)
|
||||
goal_spec[:accounts].uniq.each { |a| goal.goal_accounts.build(account: a) }
|
||||
goal.save!
|
||||
wedding_goal = goal if goal_spec[:name] == "Wedding fund"
|
||||
|
||||
Array(goal_spec[:pledges]).each do |pledge_spec|
|
||||
goal.goal_pledges.create!(
|
||||
@@ -1393,7 +1400,7 @@ class Demo::Generator
|
||||
end
|
||||
end
|
||||
|
||||
seed_matched_pledge_demo_for_wedding!(family, currency, primary)
|
||||
seed_matched_pledge_demo_for_wedding!(wedding_goal, currency, primary) if wedding_goal && primary
|
||||
|
||||
puts " ✅ Seeded #{goals.size} goals"
|
||||
end
|
||||
@@ -1402,8 +1409,7 @@ class Demo::Generator
|
||||
# inflow Transaction. Surfaces the "Last pledge matched N days ago"
|
||||
# header copy + exercises the partial-unique index on
|
||||
# transactions.extra->'goal'->>'pledge_id'.
|
||||
def seed_matched_pledge_demo_for_wedding!(family, currency, primary)
|
||||
wedding = family.goals.find_by(name: "Wedding fund")
|
||||
def seed_matched_pledge_demo_for_wedding!(wedding, currency, primary)
|
||||
return unless wedding && primary
|
||||
|
||||
recent_inflow_entry = Entry
|
||||
|
||||
@@ -59,7 +59,11 @@ class Family < ApplicationRecord
|
||||
# Result is allowed to go negative (net outflow last 30d) so the headline
|
||||
# reflects reality; the controller decides how to render.
|
||||
def savings_inflow_velocity(range: 30.days.ago.to_date..Date.current)
|
||||
account_ids = Account
|
||||
# Defensive scope: goal_id is already family-bound (this family's
|
||||
# goals), but pinning family_id keeps cross-family bleed-through
|
||||
# impossible if a goal_account ever ends up pointing at a foreign
|
||||
# account through a future bug.
|
||||
account_ids = accounts
|
||||
.joins(:goal_accounts)
|
||||
.where(goal_accounts: { goal_id: goals.select(:id) })
|
||||
.where(currency: primary_currency_code)
|
||||
|
||||
@@ -283,6 +283,33 @@ class Goal < ApplicationRecord
|
||||
end
|
||||
end
|
||||
|
||||
# Header copy under the goal title on show. Used to live as a multi-line
|
||||
# if/elsif block in show.html.erb. Keeps the view template free of date
|
||||
# math + i18n key picking.
|
||||
def header_summary
|
||||
parts = []
|
||||
if target_date
|
||||
days = (target_date - Date.current).to_i
|
||||
past_due = days < 0 && !(completed? || status == :reached)
|
||||
if past_due
|
||||
parts << I18n.t("goals.show.header.target_by_past",
|
||||
amount: target_amount_money.format(precision: 0),
|
||||
date: I18n.l(target_date, format: :long))
|
||||
else
|
||||
parts << I18n.t("goals.show.header.target_by",
|
||||
amount: target_amount_money.format(precision: 0),
|
||||
date: I18n.l(target_date, format: :long))
|
||||
if days > 0 && !(completed? || status == :reached)
|
||||
parts << I18n.t("goals.goal_card.days_left", count: days).split(" · ").first
|
||||
end
|
||||
end
|
||||
else
|
||||
parts << I18n.t("goals.show.header.target",
|
||||
amount: target_amount_money.format(precision: 0))
|
||||
end
|
||||
parts.join(" · ")
|
||||
end
|
||||
|
||||
# Single source of truth for the projection-chart subtitle / chart-aria
|
||||
# description. Used to live inline in show.html.erb as a 17-line if/elsif
|
||||
# chain. Returns an `html_safe` string when it picks the `_html` variant.
|
||||
@@ -300,7 +327,7 @@ class Goal < ApplicationRecord
|
||||
months = (remaining_amount.to_d / pace.to_d).ceil
|
||||
I18n.t(
|
||||
"goals.show.projection.on_track_html",
|
||||
date: (Date.current >> months.to_i).strftime("%b %Y")
|
||||
date: I18n.l(Date.current >> months.to_i, format: "%b %Y")
|
||||
)
|
||||
else
|
||||
I18n.t("goals.show.projection.no_pace")
|
||||
@@ -316,7 +343,7 @@ class Goal < ApplicationRecord
|
||||
def catch_up_delta_money
|
||||
return Money.new(0, currency) if monthly_target_amount.nil?
|
||||
|
||||
pending = open_pledges.to_a.sum { |p| p.amount.to_d }
|
||||
pending = open_pledges.sum(:amount).to_d
|
||||
delta = [ monthly_target_amount.to_d - pace.to_d - pending, 0 ].max
|
||||
Money.new(delta, currency)
|
||||
end
|
||||
|
||||
@@ -38,9 +38,13 @@ class GoalPledge < ApplicationRecord
|
||||
|
||||
# Tolerance check: entry date within [created_at − 5d, expires_at] (so
|
||||
# extend! widens the upper bound) and amount within ±$0.50 OR ±1%.
|
||||
# Transfer pledges only fire on inflows (Sure convention: inflow < 0).
|
||||
# Without this guard, .abs below lets a $200 outflow satisfy a $200
|
||||
# transfer pledge as readily as a $200 deposit.
|
||||
def matches?(entry)
|
||||
return false unless status_open?
|
||||
return false unless entry.account_id == account_id
|
||||
return false if kind_transfer? && !entry.amount.to_d.negative?
|
||||
|
||||
earliest = created_at.to_date - MATCH_DATE_TOLERANCE_DAYS.days
|
||||
latest = [ created_at.to_date + MATCH_DATE_TOLERANCE_DAYS.days, expires_at.to_date ].max
|
||||
@@ -62,7 +66,9 @@ class GoalPledge < ApplicationRecord
|
||||
|
||||
transaction.with_lock do
|
||||
pledge_id_in_extra = transaction.extra.dig("goal", "pledge_id")
|
||||
raise ActiveRecord::RecordInvalid if pledge_id_in_extra.present? && pledge_id_in_extra != id
|
||||
if pledge_id_in_extra.present? && pledge_id_in_extra != id
|
||||
raise AlreadyClaimedError, "Transaction ##{transaction.id} already claimed by pledge ##{pledge_id_in_extra}"
|
||||
end
|
||||
|
||||
extra = transaction.extra || {}
|
||||
extra["goal"] = (extra["goal"] || {}).merge("pledge_id" => id)
|
||||
@@ -83,6 +89,10 @@ class GoalPledge < ApplicationRecord
|
||||
end
|
||||
|
||||
class NotOpenError < StandardError; end
|
||||
# Raised when a Transaction is already claimed by a different open
|
||||
# pledge. Lets the reconciler distinguish a known race ("another worker
|
||||
# got there first") from a generic validation failure.
|
||||
class AlreadyClaimedError < StandardError; end
|
||||
|
||||
def extend!(days: EXTEND_DAYS)
|
||||
raise NotOpenError, "Only open pledges can be extended" unless status_open?
|
||||
|
||||
@@ -9,10 +9,14 @@ class GoalPledge::Reconciler
|
||||
return unless eligible_entry?
|
||||
return if already_stamped?
|
||||
|
||||
# Older pledges resolve first. Deterministic so "first claim wins"
|
||||
# under ties doesn't depend on PK ordering (which find_each batches
|
||||
# by) — relevant the day Sure adopts ULID/UUID PKs.
|
||||
GoalPledge
|
||||
.where(account_id: entry.account_id, status: "open", kind: expected_kind)
|
||||
.where("expires_at >= ?", Time.current)
|
||||
.find_each do |pledge|
|
||||
.order(:created_at, :id)
|
||||
.each do |pledge|
|
||||
next unless pledge.matches?(entry)
|
||||
|
||||
begin
|
||||
@@ -24,6 +28,7 @@ class GoalPledge::Reconciler
|
||||
Rails.logger.info("GoalPledge ##{pledge.id} matched entry ##{entry.id}")
|
||||
return
|
||||
rescue GoalPledge::NotOpenError,
|
||||
GoalPledge::AlreadyClaimedError,
|
||||
ActiveRecord::RecordInvalid,
|
||||
ActiveRecord::RecordNotUnique => e
|
||||
# Race vs another worker (this pledge got claimed, or this txn got
|
||||
|
||||
@@ -35,7 +35,7 @@
|
||||
</div>
|
||||
<div data-color-icon-picker-target="validationMessage" class="hidden self-start flex gap-1 items-center text-xs text-destructive ">
|
||||
<span>Poor contrast, choose darker color or</span>
|
||||
<button type="button" class="underline cursor-pointer" data-action="category#autoAdjust">auto-adjust.</button>
|
||||
<button type="button" class="underline cursor-pointer" data-action="color-icon-picker#autoAdjust">auto-adjust.</button>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
@@ -46,7 +46,7 @@
|
||||
<% Category.icon_codes.each do |icon| %>
|
||||
<label class="relative">
|
||||
<%= f.radio_button :lucide_icon, icon, class: "sr-only peer", data: { action: "change->color-icon-picker#handleIconChange change->color-icon-picker#handleIconColorChange", color_icon_picker_target:"icon" } %>
|
||||
<div class="text-secondary w-7 h-7 flex m-0.5 items-center justify-center rounded-full cursor-pointer hover:bg-container-inset-hover peer-checked:bg-container-inset border-1 border-transparent">
|
||||
<div class="text-secondary w-7 h-7 flex m-0.5 items-center justify-center rounded-full cursor-pointer hover:bg-container-inset-hover peer-checked:bg-container-inset border border-transparent">
|
||||
<%= icon(icon, size: "sm", color: "current") %>
|
||||
</div>
|
||||
</label>
|
||||
|
||||
@@ -55,7 +55,7 @@
|
||||
<% icons.each do |icon_name| %>
|
||||
<label class="relative">
|
||||
<%= form.radio_button :icon, icon_name, class: "sr-only peer", data: { action: "change->color-icon-picker#handleIconChange change->color-icon-picker#handleIconColorChange", color_icon_picker_target: "icon" } %>
|
||||
<div class="text-secondary w-7 h-7 flex m-0.5 items-center justify-center rounded-full cursor-pointer hover:bg-container-inset-hover peer-checked:bg-container-inset border-1 border-transparent">
|
||||
<div class="text-secondary w-7 h-7 flex m-0.5 items-center justify-center rounded-full cursor-pointer hover:bg-container-inset-hover peer-checked:bg-container-inset border border-transparent">
|
||||
<%= icon(icon_name, size: "sm", color: "current") %>
|
||||
</div>
|
||||
</label>
|
||||
|
||||
@@ -96,7 +96,7 @@
|
||||
data-action="input->goals-filter#filter"
|
||||
aria-label="<%= t(".search.aria_label") %>"
|
||||
placeholder="<%= t(".search.placeholder") %>"
|
||||
class="block w-full border border-secondary rounded-md py-2.5 pl-10 pr-3 bg-container focus:ring-gray-500 sm:text-sm">
|
||||
class="block w-full border border-secondary rounded-md py-2.5 pl-10 pr-3 bg-container focus:outline-none focus:ring-2 focus:ring-alpha-black-100 sm:text-sm">
|
||||
<div class="absolute inset-y-0 left-0 flex items-center pl-3 pointer-events-none">
|
||||
<%= icon "search", class: "text-secondary" %>
|
||||
</div>
|
||||
|
||||
@@ -7,26 +7,7 @@
|
||||
<h1 class="text-2xl font-semibold text-primary min-w-0 break-words"><%= @goal.name %></h1>
|
||||
<%= render Goals::StatusPillComponent.new(goal: @goal) %>
|
||||
</div>
|
||||
<p class="text-sm text-secondary">
|
||||
<%
|
||||
primary_parts = []
|
||||
if @goal.target_date
|
||||
days = (@goal.target_date - Date.current).to_i
|
||||
past_due = days < 0 && !(@goal.completed? || @goal.status == :reached)
|
||||
if past_due
|
||||
primary_parts << t(".header.target_by_past", amount: @goal.target_amount_money.format(precision: 0), date: I18n.l(@goal.target_date, format: :long))
|
||||
else
|
||||
primary_parts << t(".header.target_by", amount: @goal.target_amount_money.format(precision: 0), date: I18n.l(@goal.target_date, format: :long))
|
||||
if days > 0 && !(@goal.completed? || @goal.status == :reached)
|
||||
primary_parts << t("goals.goal_card.days_left", count: days, date: I18n.l(@goal.target_date, format: :long)).split(" · ").first
|
||||
end
|
||||
end
|
||||
else
|
||||
primary_parts << t(".header.target", amount: @goal.target_amount_money.format(precision: 0))
|
||||
end
|
||||
%>
|
||||
<%= primary_parts.join(" · ") %>
|
||||
</p>
|
||||
<p class="text-sm text-secondary"><%= @goal.header_summary %></p>
|
||||
<% last_days = @goal.last_matched_pledge_days_ago %>
|
||||
<% unless last_days.nil? %>
|
||||
<p class="text-xs text-subdued mt-0.5">
|
||||
|
||||
@@ -104,6 +104,24 @@ class GoalsControllerTest < ActionDispatch::IntegrationTest
|
||||
assert_not_empty @goal.reload.goal_accounts
|
||||
end
|
||||
|
||||
test "update rejects a cross-currency account attachment" do
|
||||
# Regression: sync_linked_accounts! used to call goal_accounts.create!
|
||||
# directly, bypassing Goal#linked_accounts_must_match_goal_currency.
|
||||
eur_account = Account.create!(
|
||||
family: @goal.family,
|
||||
accountable: Depository.new,
|
||||
name: "EUR Checking",
|
||||
currency: "EUR",
|
||||
balance: 100
|
||||
)
|
||||
before_ids = @goal.goal_accounts.pluck(:account_id).sort
|
||||
|
||||
patch goal_url(@goal), params: { goal: { account_ids: [ eur_account.id ] } }
|
||||
|
||||
assert_response :unprocessable_entity
|
||||
assert_equal before_ids, @goal.reload.goal_accounts.pluck(:account_id).sort
|
||||
end
|
||||
|
||||
test "pause/resume/complete/archive/unarchive flow" do
|
||||
fresh = goals(:emergency_fund)
|
||||
patch pause_goal_url(fresh)
|
||||
|
||||
@@ -19,13 +19,32 @@ class SweepExpiredGoalPledgesJobTest < ActiveJob::TestCase
|
||||
assert pledge.reload.status_open?
|
||||
end
|
||||
|
||||
test "ignores already-matched or cancelled pledges" do
|
||||
test "ignores already-matched, cancelled, or already-expired pledges" do
|
||||
matched = goal_pledges(:matched_transfer)
|
||||
expired = goal_pledges(:expired_transfer)
|
||||
# Build the cancelled pledge inline rather than baking it into fixtures
|
||||
# so the cancelled-path coverage stays test-local.
|
||||
cancelled = matched.goal.goal_pledges.create!(
|
||||
account: matched.account,
|
||||
amount: 25,
|
||||
currency: matched.currency,
|
||||
kind: matched.kind,
|
||||
status: "cancelled",
|
||||
expires_at: 2.days.ago
|
||||
)
|
||||
|
||||
SweepExpiredGoalPledgesJob.perform_now
|
||||
|
||||
assert matched.reload.status_matched?
|
||||
assert expired.reload.status_expired?
|
||||
assert cancelled.reload.status_cancelled?
|
||||
end
|
||||
|
||||
test "logs and continues when a single pledge fails to expire" do
|
||||
pledge = goal_pledges(:open_transfer)
|
||||
pledge.update_columns(expires_at: 1.day.ago)
|
||||
GoalPledge.any_instance.stubs(:expire!).raises(StandardError, "boom")
|
||||
|
||||
assert_nothing_raised { SweepExpiredGoalPledgesJob.perform_now }
|
||||
end
|
||||
end
|
||||
|
||||
@@ -65,6 +65,13 @@ class GoalPledgeTest < ActiveSupport::TestCase
|
||||
assert_not @pledge.matches?(entry)
|
||||
end
|
||||
|
||||
test "matches? rejects outflows of the same magnitude on transfer pledges" do
|
||||
# Sure convention: outflow > 0, inflow < 0. A +$200 purchase must not
|
||||
# satisfy a $200 transfer pledge after the .abs amount-tolerance step.
|
||||
entry = build_entry(account: @account, amount: 200, date: @pledge.created_at.to_date)
|
||||
assert_not @pledge.matches?(entry)
|
||||
end
|
||||
|
||||
test "matches? returns false on already-matched pledge" do
|
||||
matched = goal_pledges(:matched_transfer)
|
||||
entry = build_entry(account: matched.account, amount: -matched.amount.to_d, date: matched.created_at.to_date)
|
||||
|
||||
@@ -187,6 +187,9 @@ class GoalTest < ActiveSupport::TestCase
|
||||
@goal.goal_accounts.where(account_id: @connected.id).destroy_all
|
||||
@goal.reload
|
||||
@goal.instance_variable_set(:@current_balance, nil)
|
||||
assert_equal "goals.show.pledge_just_transferred", @goal.pledge_action_label_key if @goal.linked_accounts.any?(&:plaid_account)
|
||||
# After removing the only connected account, the goal is manual-only;
|
||||
# the copy must flip to "pledge_just_saved" so users aren't told to
|
||||
# wait for a sync that won't run.
|
||||
assert_equal "goals.show.pledge_just_saved", @goal.pledge_action_label_key
|
||||
end
|
||||
end
|
||||
|
||||
Reference in New Issue
Block a user