diff --git a/app/components/goals/account_stack_component.rb b/app/components/goals/account_stack_component.rb index f0fddc00d..c1892e100 100644 --- a/app/components/goals/account_stack_component.rb +++ b/app/components/goals/account_stack_component.rb @@ -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) diff --git a/app/components/goals/funding_accounts_breakdown_component.rb b/app/components/goals/funding_accounts_breakdown_component.rb index 88995a00e..e63d7695c 100644 --- a/app/components/goals/funding_accounts_breakdown_component.rb +++ b/app/components/goals/funding_accounts_breakdown_component.rb @@ -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 diff --git a/app/components/goals/status_pill_component.rb b/app/components/goals/status_pill_component.rb index 03822722c..d1ec2179f 100644 --- a/app/components/goals/status_pill_component.rb +++ b/app/components/goals/status_pill_component.rb @@ -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 diff --git a/app/controllers/goal_pledges_controller.rb b/app/controllers/goal_pledges_controller.rb index e7e0bf398..c3c9c438d 100644 --- a/app/controllers/goal_pledges_controller.rb +++ b/app/controllers/goal_pledges_controller.rb @@ -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 diff --git a/app/controllers/goals_controller.rb b/app/controllers/goals_controller.rb index efe19012f..afba329b6 100644 --- a/app/controllers/goals_controller.rb +++ b/app/controllers/goals_controller.rb @@ -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) diff --git a/app/javascript/controllers/goal_pledge_preview_controller.js b/app/javascript/controllers/goal_pledge_preview_controller.js index 9246b6503..625ee4af7 100644 --- a/app/javascript/controllers/goal_pledge_preview_controller.js +++ b/app/javascript/controllers/goal_pledge_preview_controller.js @@ -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()}`; } } } diff --git a/app/javascript/controllers/goal_projection_chart_controller.js b/app/javascript/controllers/goal_projection_chart_controller.js index a52d0cc1e..bdb791ac8 100644 --- a/app/javascript/controllers/goal_projection_chart_controller.js +++ b/app/javascript/controllers/goal_projection_chart_controller.js @@ -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 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)}`); diff --git a/app/javascript/controllers/goal_stepper_controller.js b/app/javascript/controllers/goal_stepper_controller.js index e67f27853..ceb2b1326 100644 --- a/app/javascript/controllers/goal_stepper_controller.js +++ b/app/javascript/controllers/goal_stepper_controller.js @@ -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; diff --git a/app/jobs/sweep_expired_goal_pledges_job.rb b/app/jobs/sweep_expired_goal_pledges_job.rb index 1557fad67..29a327683 100644 --- a/app/jobs/sweep_expired_goal_pledges_job.rb +++ b/app/jobs/sweep_expired_goal_pledges_job.rb @@ -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 diff --git a/app/models/account.rb b/app/models/account.rb index 0bb487daf..5f9ad5e68 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -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 diff --git a/app/models/assistant/function/create_goal.rb b/app/models/assistant/function/create_goal.rb index 983402f8a..9452ccf27 100644 --- a/app/models/assistant/function/create_goal.rb +++ b/app/models/assistant/function/create_goal.rb @@ -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) diff --git a/app/models/demo/generator.rb b/app/models/demo/generator.rb index ad413890e..f7c317148 100644 --- a/app/models/demo/generator.rb +++ b/app/models/demo/generator.rb @@ -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 diff --git a/app/models/family.rb b/app/models/family.rb index a117f6cc8..937e03e83 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -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) diff --git a/app/models/goal.rb b/app/models/goal.rb index e71a4b0d9..8274a1778 100644 --- a/app/models/goal.rb +++ b/app/models/goal.rb @@ -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 diff --git a/app/models/goal_pledge.rb b/app/models/goal_pledge.rb index e94f3cc40..a72b2cb5b 100644 --- a/app/models/goal_pledge.rb +++ b/app/models/goal_pledge.rb @@ -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? diff --git a/app/models/goal_pledge/reconciler.rb b/app/models/goal_pledge/reconciler.rb index 0a1afd837..f3309dc9e 100644 --- a/app/models/goal_pledge/reconciler.rb +++ b/app/models/goal_pledge/reconciler.rb @@ -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 diff --git a/app/views/categories/_form.html.erb b/app/views/categories/_form.html.erb index 115a7d61e..6071368fc 100644 --- a/app/views/categories/_form.html.erb +++ b/app/views/categories/_form.html.erb @@ -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> diff --git a/app/views/goals/_color_picker.html.erb b/app/views/goals/_color_picker.html.erb index 5ccba3b92..d3805d466 100644 --- a/app/views/goals/_color_picker.html.erb +++ b/app/views/goals/_color_picker.html.erb @@ -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> diff --git a/app/views/goals/index.html.erb b/app/views/goals/index.html.erb index 53b5a6074..3411389bf 100644 --- a/app/views/goals/index.html.erb +++ b/app/views/goals/index.html.erb @@ -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> diff --git a/app/views/goals/show.html.erb b/app/views/goals/show.html.erb index 6ad61838a..8411afc33 100644 --- a/app/views/goals/show.html.erb +++ b/app/views/goals/show.html.erb @@ -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"> diff --git a/test/controllers/goals_controller_test.rb b/test/controllers/goals_controller_test.rb index 984b0afad..7600d4fd9 100644 --- a/test/controllers/goals_controller_test.rb +++ b/test/controllers/goals_controller_test.rb @@ -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) diff --git a/test/jobs/sweep_expired_goal_pledges_job_test.rb b/test/jobs/sweep_expired_goal_pledges_job_test.rb index ab4811575..43660de41 100644 --- a/test/jobs/sweep_expired_goal_pledges_job_test.rb +++ b/test/jobs/sweep_expired_goal_pledges_job_test.rb @@ -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 diff --git a/test/models/goal_pledge_test.rb b/test/models/goal_pledge_test.rb index 88e2dabdd..a4626b3e5 100644 --- a/test/models/goal_pledge_test.rb +++ b/test/models/goal_pledge_test.rb @@ -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) diff --git a/test/models/goal_test.rb b/test/models/goal_test.rb index 092766906..fa3616040 100644 --- a/test/models/goal_test.rb +++ b/test/models/goal_test.rb @@ -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