From 83c64b9e94e0c2584e761bc6c2cc7657aa659762 Mon Sep 17 00:00:00 2001 From: Guillem Arias Date: Thu, 14 May 2026 19:12:28 +0200 Subject: [PATCH] fix(goals): pledge lifecycle + connected-account detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behavioural fixes touching Goal, GoalPledge, the reconciler and the goals controller. No schema change. B5 — connected-account detection covered only Plaid. SimpleFIN, Brex, Enable Banking, IBKR, Kraken, SnapTrade and Lunchflow users got "manual_save" pledges by default; their auto-synced Transactions then failed to match (reconciler matches Transactions to "transfer" pledges only). Pledges sat in the yellow banner until expiry. Switch the detection to !Account#manual?, which mirrors the existing `Account.manual` scope (no account_providers, no plaid_account_id, no simplefin_account_id). Add `Account#manual?` so the per-instance and per-query checks can't drift. B7 — `extend!` widens `expires_at` but `matches?` was anchored on `created_at ± 5d`, so an extension that pushed the expiry past day 5 didn't actually buy any match runway. Widen the upper bound to `max(created_at + 5d, expires_at)`. The lower bound stays at `created_at − 5d`. B8 — `Goal#open_pledges` returned `status: open` regardless of expiry. Between a pledge timing out (day 7) and the 15-min sweep job marking it `expired`, the show page rendered a ghost yellow banner with "0 days left" that the reconciler would no longer touch. Add `expires_at >= NOW` to the scope so the visible state matches the match-eligible state. B9 — Double-click on Record pledge produced two identical open pledges, which then stacked as two yellow banners. Add a create-time validation rejecting duplicates against (goal_id, account_id, amount, status=open, expires_at >= NOW). B10 — The reconciler used `transaction.with_lock` but didn't lock the pledge. Two concurrent reconcile attempts on different transactions could both target the same pledge; one would lose to the partial unique index on `transactions.extra->'goal'->>'pledge_id'` and the RecordNotUnique was caught by the outer StandardError rescue, which silently dropped the other transaction's match attempt entirely. Lock the pledge first, re-check `status_open?` inside the lock, and catch RecordNotUnique alongside RecordInvalid/NotOpenError in the reconciler — so on a lost race we fall through to the next candidate pledge instead of exiting the loop. Extract the Valuation-match path to `GoalPledge#resolve_with_valuation!` so it goes through the same locked status-recheck. B12 — When a goal is destroyed, `dependent: :destroy` reaped pledges but left `transactions.extra["goal"]["pledge_id"]` pointing at the now-deleted UUIDs. The partial unique index on that JSON path then indexed stale references. Add a `before_destroy` on GoalPledge that clears the matching transaction's `extra` if it still points back to the pledge. B6 — `last_matched_pledge_at` used `goal_pledges.maximum(:updated_at)` on matched rows. Any backfill or sync-resync that touches a matched pledge bumped `updated_at`, so a single resync set every goal's "Last saved N days ago" header back to "today". Switch to the entry's `date` via a join through `matched_transaction_id`, which reflects the date the money actually moved. B22 — `scope :chronological` ordered DESC, the opposite of what the name promises. Rename to `:reverse_chronological` and update the one caller in `goals#show`. (Other models' `chronological` scopes are unrelated and ordered correctly.) Also: preload `account_providers` on `linked_accounts` in the index and show controllers so `Account#manual?` walks the in-memory collection instead of triggering N queries. Tests: add fixture-backed coverage for extend-widens-match-window, post-extend rejection beyond expiry, and the duplicate-pledge validation. Existing assertions still hold against the new `matches?` window math. --- app/controllers/goals_controller.rb | 9 ++-- app/models/account.rb | 8 ++- app/models/goal.rb | 27 +++++++--- app/models/goal_pledge.rb | 67 ++++++++++++++++++++---- app/models/goal_pledge/reconciler.rb | 15 ++++-- config/locales/models/goal_pledge/en.yml | 1 + test/models/goal_pledge_test.rb | 29 ++++++++++ 7 files changed, 131 insertions(+), 25 deletions(-) diff --git a/app/controllers/goals_controller.rb b/app/controllers/goals_controller.rb index 747af47b8..e71135eb2 100644 --- a/app/controllers/goals_controller.rb +++ b/app/controllers/goals_controller.rb @@ -11,7 +11,10 @@ class GoalsController < ApplicationController h[state] = state == "all" ? state_counts.values.sum : (state_counts[state] || 0) end - all_goals = Current.family.goals.alphabetically.includes(:linked_accounts, :open_pledges).to_a + all_goals = Current.family.goals + .alphabetically + .includes(:open_pledges, linked_accounts: :account_providers) + .to_a @active_goals = all_goals.reject { |g| %w[completed archived].include?(g.state) } .sort_by { |g| [ g.paused? ? 3 : ACTIVE_STATUS_RANK.fetch(g.status, 4), g.name.downcase ] } @completed_goals = all_goals.select { |g| g.state == "completed" } @@ -28,7 +31,7 @@ class GoalsController < ApplicationController end def show - @open_pledges = @goal.open_pledges.chronological.to_a + @open_pledges = @goal.open_pledges.reverse_chronological.to_a @breadcrumbs = [ [ t("breadcrumbs.home"), root_path ], [ t("goals.index.title"), goals_path ], @@ -140,7 +143,7 @@ class GoalsController < ApplicationController private def set_goal @goal = Current.family.goals - .includes(:linked_accounts, :open_pledges) + .includes(:open_pledges, linked_accounts: :account_providers) .find(params[:id]) end diff --git a/app/models/account.rb b/app/models/account.rb index f83ced876..0bb487daf 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -344,7 +344,13 @@ class Account < ApplicationRecord def manual_crypto_exchange? accountable_type == "Crypto" && accountable&.subtype == "exchange" && - account_providers.none? && + manual? + end + + # True when the account has no live sync provider attached. Mirrors the + # `Account.manual` scope so per-instance checks don't drift from the query. + def manual? + account_providers.none? && plaid_account_id.blank? && simplefin_account_id.blank? end diff --git a/app/models/goal.rb b/app/models/goal.rb index bdb10f228..e3ce6249c 100644 --- a/app/models/goal.rb +++ b/app/models/goal.rb @@ -10,7 +10,9 @@ class Goal < ApplicationRecord has_many :goal_accounts, dependent: :destroy has_many :linked_accounts, through: :goal_accounts, source: :account has_many :goal_pledges, dependent: :destroy - has_many :open_pledges, -> { where(status: "open") }, class_name: "GoalPledge" + has_many :open_pledges, + -> { where(status: "open").where("expires_at >= ?", Time.current) }, + class_name: "GoalPledge" validates :name, presence: true, length: { maximum: 255 } validates :target_amount, presence: true, numericality: { greater_than: 0 } @@ -208,21 +210,34 @@ class Goal < ApplicationRecord end end - # Days since the most recently matched pledge. Used by the show header to - # show "Last saved N days ago". Returns nil if no pledge has resolved yet. + # Date of the most-recently-matched pledge's underlying entry. Used by the + # show header to display "Last saved N days ago". Anchoring on the entry's + # date keeps the readout stable under sync re-runs (which would bump + # pledge#updated_at). Returns nil if no pledge has resolved yet. def last_matched_pledge_at - @last_matched_pledge_at ||= goal_pledges.where(status: "matched").maximum(:updated_at) + return @last_matched_pledge_at if defined?(@last_matched_pledge_at) + + @last_matched_pledge_at = Entry + .where(entryable_type: "Transaction") + .joins("INNER JOIN goal_pledges ON goal_pledges.matched_transaction_id = entries.entryable_id") + .where(goal_pledges: { goal_id: id, status: "matched" }) + .maximum(:date) end def last_matched_pledge_days_ago last = last_matched_pledge_at return nil if last.nil? - (Date.current - last.to_date).to_i + (Date.current - last).to_i end + # True when any linked account is wired to a live sync provider (Plaid, + # SimpleFIN, or any AccountProvider — Brex, Enable Banking, IBKR, Kraken, + # SnapTrade, Lunchflow). Drives the pledge-create copy: connected accounts + # get the "I just transferred…" path; manual-only accounts get "I just + # saved…" so users aren't told to wait for a sync that won't happen. def any_connected_account? - linked_accounts.any? { |a| a.respond_to?(:plaid_account) && a.plaid_account.present? } + linked_accounts.any? { |a| !a.manual? } end # "I just transferred" for bank-connected accounts, "I just saved" for manual-only. diff --git a/app/models/goal_pledge.rb b/app/models/goal_pledge.rb index 915067da5..023d98ec1 100644 --- a/app/models/goal_pledge.rb +++ b/app/models/goal_pledge.rb @@ -22,23 +22,29 @@ class GoalPledge < ApplicationRecord validates :expires_at, presence: true validate :account_must_be_linked_to_goal validate :currency_matches_goal + validate :no_duplicate_open_pledge, on: :create monetize :amount - scope :chronological, -> { order(created_at: :desc) } + # Newest first. Used by the show page to render pending-pledge banners in + # "most-recent on top" order. Not actually chronological — kept for clarity. + scope :reverse_chronological, -> { order(created_at: :desc) } scope :open_and_expired_now, -> { where(status: "open").where("expires_at < ?", Time.current) } before_validation :assign_defaults, on: :create + before_destroy :clear_matched_transaction_extra - # Tolerance check: ±5 days from pledge date, amount within ±$0.50 OR ±1%. + # Tolerance check: entry date within [created_at − 5d, expires_at] (so + # extend! widens the upper bound) and amount within ±$0.50 OR ±1%. def matches?(entry) return false unless status_open? return false unless entry.account_id == account_id - date_diff = (entry.date - created_at.to_date).abs.to_i - return false if date_diff > MATCH_DATE_TOLERANCE_DAYS + 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 + return false unless entry.date >= earliest && entry.date <= latest txn_amount = entry.amount.to_d.abs pledge_amount = amount.to_d @@ -51,15 +57,28 @@ class GoalPledge < ApplicationRecord end def resolve_with!(transaction) - 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 + with_lock do + raise NotOpenError, "Pledge no longer open" unless status_open? - extra = transaction.extra || {} - extra["goal"] = (extra["goal"] || {}).merge("pledge_id" => id) - transaction.update!(extra: extra) + 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 - update!(status: "matched", matched_transaction_id: transaction.id) + extra = transaction.extra || {} + extra["goal"] = (extra["goal"] || {}).merge("pledge_id" => id) + transaction.update!(extra: extra) + + update!(status: "matched", matched_transaction_id: transaction.id) + end + end + end + + # Valuation-backed match: no transaction to stamp, just flip the pledge. + def resolve_with_valuation! + with_lock do + raise NotOpenError, "Pledge no longer open" unless status_open? + + update!(status: "matched") end end @@ -111,4 +130,30 @@ class GoalPledge < ApplicationRecord errors.add(:currency, :must_match_goal) end + + # Guards against a double-click that creates two identical open pledges, + # which would render two yellow banners and leave one orphaned to expiry. + def no_duplicate_open_pledge + return unless goal_id && account_id && amount && status_open? + + exists = GoalPledge + .where(goal_id: goal_id, account_id: account_id, amount: amount, status: "open") + .where("expires_at >= ?", Time.current) + .exists? + + errors.add(:base, :duplicate_open_pledge) if exists + end + + def clear_matched_transaction_extra + return if matched_transaction_id.blank? + + txn = Transaction.find_by(id: matched_transaction_id) + return if txn.nil? + return unless txn.extra.dig("goal", "pledge_id") == id + + new_extra = txn.extra.deep_dup + new_extra["goal"]&.delete("pledge_id") + new_extra.delete("goal") if new_extra["goal"]&.empty? + txn.update!(extra: new_extra) + end end diff --git a/app/models/goal_pledge/reconciler.rb b/app/models/goal_pledge/reconciler.rb index 5253bf54c..f700bc852 100644 --- a/app/models/goal_pledge/reconciler.rb +++ b/app/models/goal_pledge/reconciler.rb @@ -16,12 +16,19 @@ class GoalPledge::Reconciler next unless pledge.matches?(entry) begin - pledge.resolve_with!(entry.transaction) if entry.entryable.is_a?(Transaction) - pledge.update!(status: "matched") if entry.entryable.is_a?(Valuation) + if entry.entryable.is_a?(Transaction) + pledge.resolve_with!(entry.transaction) + elsif entry.entryable.is_a?(Valuation) + pledge.resolve_with_valuation! + end Rails.logger.info("GoalPledge ##{pledge.id} matched entry ##{entry.id}") return - rescue ActiveRecord::RecordInvalid => e - Rails.logger.warn("GoalPledge ##{pledge.id} match failed: #{e.message}") + rescue GoalPledge::NotOpenError, + ActiveRecord::RecordInvalid, + ActiveRecord::RecordNotUnique => e + # Race vs another worker (this pledge got claimed, or this txn got + # stamped by another pledge). Fall through and try the next pledge. + Rails.logger.warn("GoalPledge ##{pledge.id} match failed: #{e.class}: #{e.message}") end end rescue StandardError => e diff --git a/config/locales/models/goal_pledge/en.yml b/config/locales/models/goal_pledge/en.yml index adac9ba25..eda34dbc3 100644 --- a/config/locales/models/goal_pledge/en.yml +++ b/config/locales/models/goal_pledge/en.yml @@ -17,3 +17,4 @@ en: must_be_linked_to_goal: Pick one of the goal's linked accounts. currency: must_match_goal: Pledge currency must match the goal currency. + duplicate_open_pledge: You already have an open pledge for this amount on this account. Cancel or extend the existing one before recording another. diff --git a/test/models/goal_pledge_test.rb b/test/models/goal_pledge_test.rb index 0b9927966..88e2dabdd 100644 --- a/test/models/goal_pledge_test.rb +++ b/test/models/goal_pledge_test.rb @@ -77,6 +77,35 @@ class GoalPledgeTest < ActiveSupport::TestCase assert @pledge.expires_at > before + 6.days end + test "matches? widens upper bound to expires_at after extend!" do + # Day 8 — past the default 5-day creation-anchored window but inside the + # extended expiry window. Without the widening this would be a regression + # of B7 (extend doesn't actually buy match runway). + @pledge.extend! + far_date = @pledge.created_at.to_date + 8.days + assert far_date <= @pledge.expires_at.to_date + entry = build_entry(account: @account, amount: -200, date: far_date) + assert @pledge.matches?(entry) + end + + test "matches? rejects entries past extended expires_at" do + @pledge.extend! + far_date = @pledge.expires_at.to_date + 1.day + entry = build_entry(account: @account, amount: -200, date: far_date) + assert_not @pledge.matches?(entry) + end + + test "duplicate open pledge for same goal+account+amount is rejected on create" do + dup = @goal.goal_pledges.new(account: @account, amount: @pledge.amount, currency: @goal.currency) + assert_not dup.valid? + assert dup.errors[:base].any? { |m| m.include?("open pledge") } + end + + test "duplicate validation does not block different amounts" do + dup = @goal.goal_pledges.new(account: @account, amount: @pledge.amount.to_d + 1, currency: @goal.currency) + assert dup.valid?, dup.errors.full_messages.to_sentence + end + test "extend! raises for non-open pledge" do pledge = goal_pledges(:matched_transfer) assert_raises(GoalPledge::NotOpenError) { pledge.extend! }