From 5fb8f4f81adb4b75fa9cfc8e6be5c26c2b41bf03 Mon Sep 17 00:00:00 2001 From: PrplHaz4 Date: Wed, 29 Jul 2026 18:13:02 -0500 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20re-create=20pending=20SimpleFIN?= =?UTF-8?q?=20transactions=20when=20pending=20sync=20is=20disabled=20(#283?= =?UTF-8?q?5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(simplefin): skip pending entries in processor when pending is disabled When SIMPLEFIN_INCLUDE_PENDING/syncs_include_pending is off, pending rows already stored in raw_transactions_payload were still (re)created as entries on every sync - including ones the user manually deleted - because the setting only affected the API request, not reprocessing of the stored payload. Co-Authored-By: Claude Fable 5 * chore(simplefin): add rake task to prune stale pending rows from payload store raw_transactions_payload accumulates transactions across syncs and is never pruned, so pending rows fetched before pending inclusion was disabled keep getting re-imported. This one-time maintenance task removes them (dry-run by default; scope by item_id/account_id). Co-Authored-By: Claude Fable 5 * fix(simplefin): address PR #2835 review feedback on pending detection Fix epoch-zero pending check treating non-numeric posted strings (e.g. "unavailable") as pending via String#to_i coercion; compare against explicit zero representations instead, matching posted_date. Dedupe the prune_pending rake task's copy of this logic by delegating to a new public SimplefinEntry::Processor.pending? class method. Also close a test gap where SIMPLEFIN_INCLUDE_PENDING env var precedence over the Setting wasn't actually exercised. * test(simplefin): cover pending-guard precedence and add rake task tests Add the missing mirror case for pending_enabled? precedence (env var disabling pending over a permissive Setting) and add test coverage for the prune_pending rake task, which previously had none: dry_run safety default, correct pruning via the shared Processor.pending? predicate (including the malformed-posted regression), and that it never touches Entry/Transaction rows. --------- Co-authored-by: Claude Fable 5 --- app/models/simplefin_entry/processor.rb | 63 ++++++---- lib/tasks/simplefin_prune_pending.rake | 105 ++++++++++++++++ .../lib/tasks/simplefin_prune_pending_test.rb | 79 ++++++++++++ test/models/simplefin_entry/processor_test.rb | 116 ++++++++++++++++++ 4 files changed, 342 insertions(+), 21 deletions(-) create mode 100644 lib/tasks/simplefin_prune_pending.rake create mode 100644 test/lib/tasks/simplefin_prune_pending_test.rb diff --git a/app/models/simplefin_entry/processor.rb b/app/models/simplefin_entry/processor.rb index 7cb5f9da9..419fe14bc 100644 --- a/app/models/simplefin_entry/processor.rb +++ b/app/models/simplefin_entry/processor.rb @@ -10,7 +10,29 @@ class SimplefinEntry::Processor @shared_import_adapter = import_adapter end + # Pending detection: explicit flag OR inferred from posted=0 (epoch) + transacted_at. + # Public so callers like the prune_pending rake task share this definition instead of + # reimplementing it. + def self.pending?(simplefin_transaction) + data = simplefin_transaction.with_indifferent_access + return true if ActiveModel::Type::Boolean.new.cast(data[:pending]) + + posted_val = data[:posted] + transacted_val = data[:transacted_at] + # Compare against explicit zero representations (mirrors posted_date) rather than + # posted_val.to_i.zero?, which would also match non-numeric junk like "unavailable". + posted_is_epoch_zero = posted_val == 0 || posted_val == "0" + transacted_present = transacted_val.present? && transacted_val.to_i > 0 + posted_is_epoch_zero && transacted_present + end + def process + # Skip pending transactions when pending inclusion is disabled. Without this guard + # the SIMPLEFIN_INCLUDE_PENDING/syncs_include_pending setting only affects the API + # request, while pending rows already stored in raw_transactions_payload would still + # be (re)created here on every sync - including ones the user manually deleted. + return if pending? && !pending_enabled? + import_adapter.import_transaction( external_id: external_id, amount: amount, @@ -27,6 +49,23 @@ class SimplefinEntry::Processor private attr_reader :simplefin_transaction, :simplefin_account + # Whether pending transactions should be imported. Mirrors the resolution order used + # by SimplefinItem::Importer#fetch_accounts_data: env var (when set) over runtime Setting. + def pending_enabled? + if ENV["SIMPLEFIN_INCLUDE_PENDING"].present? + Rails.configuration.x.simplefin.include_pending + else + Setting.syncs_include_pending + end + end + + # We only infer pending from posted=0, NOT from posted=nil/blank, because some + # providers omit posted dates even for settled transactions (which would cause + # false positives). + def pending? + self.class.pending?(data) + end + def extra_metadata sf = {} # Preserve raw strings from provider so nothing is lost @@ -36,27 +75,9 @@ class SimplefinEntry::Processor # Include provider-supplied extra hash if present sf["extra"] = data[:extra] if data[:extra].is_a?(Hash) - # Pending detection: explicit flag OR inferred from posted=0 + transacted_at - # SimpleFIN indicates pending via: - # 1. pending: true (explicit flag) - # 2. posted=0 (epoch zero) + transacted_at present (implicit - some banks use this pattern) - # - # Note: We only infer from posted=0, NOT from posted=nil/blank, because some providers - # don't supply posted dates even for settled transactions (would cause false positives). - # We always set the key (true or false) to ensure deep_merge overwrites any stale value - is_pending = if ActiveModel::Type::Boolean.new.cast(data[:pending]) - true - else - # Infer pending ONLY when posted is explicitly 0 (epoch) AND transacted_at is present - # posted=nil/blank is NOT treated as pending (some providers omit posted for settled txns) - posted_val = data[:posted] - transacted_val = data[:transacted_at] - posted_is_epoch_zero = posted_val.present? && posted_val.to_i.zero? - transacted_present = transacted_val.present? && transacted_val.to_i > 0 - posted_is_epoch_zero && transacted_present - end - - if is_pending + # Pending detection handled by #pending?. We always set the key (true or false) to + # ensure deep_merge overwrites any stale value. + if pending? sf["pending"] = true Rails.logger.debug("SimpleFIN: flagged pending transaction #{external_id}") else diff --git a/lib/tasks/simplefin_prune_pending.rake b/lib/tasks/simplefin_prune_pending.rake new file mode 100644 index 000000000..c885bcf44 --- /dev/null +++ b/lib/tasks/simplefin_prune_pending.rake @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +# Maintenance task to prune pending transactions from the SimpleFin cumulative +# raw_transactions_payload store. +# +# Why: SimplefinAccount#raw_transactions_payload accumulates transactions across syncs +# and is never pruned. When pending inclusion is disabled, the API stops returning pending +# rows but ones already stored here keep getting (re)created as entries on every sync - +# including ones a user manually deleted. SimplefinEntry::Processor now skips pending rows +# while pending is disabled, but the stale rows remain in the store. This task removes them. +# +# Pending detection delegates to SimplefinEntry::Processor.pending?: +# - pending: true (explicit flag), OR +# - posted == 0 (epoch) AND transacted_at present (implicit pattern from some banks) +# +# Usage examples: +# # Preview (no writes) across all SimpleFin accounts +# bin/rails 'sure:simplefin:prune_pending[dry_run=true]' +# +# # Execute across all SimpleFin accounts (writes enabled) +# bin/rails 'sure:simplefin:prune_pending[dry_run=false]' +# +# # Limit to one item or one linked account +# bin/rails 'sure:simplefin:prune_pending[item_id=ec255931-62ff-4a68-abda-16067fad0429,dry_run=false]' +# bin/rails 'sure:simplefin:prune_pending[account_id=8b46387c-5aa4-4a92-963a-4392c10999c9,dry_run=false]' + +namespace :sure do + namespace :simplefin do + desc "Prune pending transactions from SimpleFin raw_transactions_payload. Args (named): item_id, account_id, dry_run=true" + task :prune_pending, [ :item_id, :account_id, :dry_run ] => :environment do |_, args| + # Support both positional and named (key=value) args; prefer named. + kv = {} + [ args[:item_id], args[:account_id], args[:dry_run] ].each do |raw| + next unless raw.is_a?(String) && raw.include?("=") + k, v = raw.split("=", 2) + kv[k.to_s] = v + end + + # A key=value string only carries a named arg, so it must not also be reused as a + # positional fallback (otherwise `prune_pending[dry_run=true]` lands "dry_run=true" + # in the :item_id slot and fails UUID validation). + positional = ->(raw) { raw.is_a?(String) && raw.include?("=") ? nil : raw } + + item_id = (kv["item_id"] || positional.call(args[:item_id])).presence + account_id = (kv["account_id"] || positional.call(args[:account_id])).presence + dry_raw = (kv["dry_run"] || positional.call(args[:dry_run])).to_s.downcase + + # Default to dry_run=true unless explicitly disabled, and validate input strictly + if dry_raw.blank? || %w[1 true yes y].include?(dry_raw) + dry_run = true + elsif %w[0 false no n].include?(dry_raw) + dry_run = false + else + puts({ ok: false, error: "invalid_argument", message: "dry_run must be one of: true/yes/1 or false/no/0" }.to_json) + exit 1 + end + + # Basic UUID validation when provided + uuid_rx = /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/i + if item_id.present? && !item_id.match?(uuid_rx) + puts({ ok: false, error: "invalid_argument", message: "item_id must be a hyphenated UUID" }.to_json) + exit 1 + end + if account_id.present? && !account_id.match?(uuid_rx) + puts({ ok: false, error: "invalid_argument", message: "account_id must be a hyphenated UUID" }.to_json) + exit 1 + end + + # Select SimplefinAccounts to process + sfas = if item_id.present? + SimplefinItem.find(item_id).simplefin_accounts + elsif account_id.present? + acct = Account.find(account_id) + # Prefer new provider linkage, fallback to legacy foreign key + sfa = if acct.account_providers.where(provider_type: "SimplefinAccount").exists? + AccountProvider.find_by(account: acct, provider_type: "SimplefinAccount")&.provider + else + SimplefinAccount.find_by(account: acct) + end + SimplefinAccount.where(id: Array.wrap(sfa).compact.map(&:id)) + else + SimplefinAccount.all + end + + total_accounts = 0 + total_removed = 0 + + sfas.find_each do |sfa| + txns = sfa.raw_transactions_payload.to_a + kept = txns.reject { |tx| SimplefinEntry::Processor.pending?(tx) } + removed = txns.size - kept.size + next if removed.zero? + + total_accounts += 1 + total_removed += removed + + sfa.update!(raw_transactions_payload: kept) unless dry_run + + puts({ sfa_id: sfa.id, name: sfa.name, total: txns.size, removed: removed, kept: kept.size, dry_run: dry_run }.to_json) + end + + puts({ ok: true, accounts_pruned: total_accounts, transactions_removed: total_removed, dry_run: dry_run }.to_json) + end + end +end diff --git a/test/lib/tasks/simplefin_prune_pending_test.rb b/test/lib/tasks/simplefin_prune_pending_test.rb new file mode 100644 index 000000000..4fedc7093 --- /dev/null +++ b/test/lib/tasks/simplefin_prune_pending_test.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require "test_helper" + +class SimplefinPrunePendingTest < ActiveSupport::TestCase + setup do + Rails.application.load_tasks unless Rake::Task.task_defined?("sure:simplefin:prune_pending") + Rake::Task["sure:simplefin:prune_pending"].reenable + + @family = families(:dylan_family) + @account = accounts(:depository) + @simplefin_item = SimplefinItem.create!( + family: @family, + name: "Test SimpleFin Bank", + access_url: "https://example.com/access_token" + ) + @simplefin_account = SimplefinAccount.create!( + simplefin_item: @simplefin_item, + name: "SF Checking", + account_id: "sf_acc_1", + account_type: "checking", + currency: "USD", + current_balance: 1000, + available_balance: 1000, + account: @account + ) + end + + test "dry_run defaults to true and leaves the payload untouched" do + payload = [ + { "id" => "tx_pending", "pending" => true, "posted" => Date.current.to_s, "transacted_at" => (Date.current - 1).to_s }, + { "id" => "tx_posted", "posted" => Date.current.to_s, "transacted_at" => (Date.current - 1).to_s } + ] + @simplefin_account.update!(raw_transactions_payload: payload) + + capture_io { Rake::Task["sure:simplefin:prune_pending"].invoke } + + assert_equal payload, @simplefin_account.reload.raw_transactions_payload, + "dry_run must default to true and never write to raw_transactions_payload" + end + + test "removes pending rows and keeps non-pending rows when dry_run=false" do + # Uses the same three shapes covered in SimplefinEntry::ProcessorTest: an explicit + # pending flag, a settled row, and a malformed non-numeric posted value that must NOT + # be swept up as pending (regression: SimplefinEntry::Processor.pending? is the single + # shared definition this task delegates to, so this also guards against the task + # reintroducing its own posted_val.to_i.zero? bug). + payload = [ + { "id" => "tx_pending", "pending" => true, "posted" => Date.current.to_s, "transacted_at" => (Date.current - 1).to_s }, + { "id" => "tx_posted", "posted" => Date.current.to_s, "transacted_at" => (Date.current - 1).to_s }, + { "id" => "tx_malformed_posted", "posted" => "unavailable", "transacted_at" => (Date.current - 1).to_s } + ] + @simplefin_account.update!(raw_transactions_payload: payload) + + capture_io { Rake::Task["sure:simplefin:prune_pending"].invoke(nil, nil, "false") } + + remaining_ids = @simplefin_account.reload.raw_transactions_payload.map { |tx| tx["id"] } + assert_equal %w[tx_posted tx_malformed_posted], remaining_ids + end + + test "never touches Entry/Transaction rows, only the raw payload cache" do + entry = @account.entries.create!( + name: "Pre-existing entry", + date: Date.current, + amount: 10, + currency: "USD", + entryable: Transaction.new + ) + + payload = [ { "id" => "tx_pending", "pending" => true, "posted" => Date.current.to_s, "transacted_at" => (Date.current - 1).to_s } ] + @simplefin_account.update!(raw_transactions_payload: payload) + + assert_no_difference [ "Entry.count", "Transaction.count" ] do + capture_io { Rake::Task["sure:simplefin:prune_pending"].invoke(nil, nil, "false") } + end + + assert entry.reload.persisted? + end +end diff --git a/test/models/simplefin_entry/processor_test.rb b/test/models/simplefin_entry/processor_test.rb index 87061d04d..52e1a0ad1 100644 --- a/test/models/simplefin_entry/processor_test.rb +++ b/test/models/simplefin_entry/processor_test.rb @@ -142,6 +142,99 @@ class SimplefinEntry::ProcessorTest < ActiveSupport::TestCase assert_equal true, sf["pending"], "expected pending flag to be true when posted==0 and/or pending=true" end + test "skips pending transactions when pending inclusion is disabled" do + Setting.stubs(:syncs_include_pending).returns(false) + + tx = { + id: "tx_pending_disabled_1", + amount: "-30.00", + currency: "USD", + payee: "Test Store", + description: "Auth hold", + posted: Date.current.to_s, + transacted_at: (Date.current - 1).to_s, + pending: true + } + + # Clear the env var so this only exercises the Setting fallback branch of pending_enabled? + with_env_overrides SIMPLEFIN_INCLUDE_PENDING: nil do + assert_no_difference "@account.entries.count" do + SimplefinEntry::Processor.new(tx, simplefin_account: @simplefin_account).process + end + end + end + + test "still imports posted transactions when pending inclusion is disabled" do + Setting.stubs(:syncs_include_pending).returns(false) + + tx = { + id: "tx_posted_disabled_1", + amount: "-30.00", + currency: "USD", + payee: "Test Store", + description: "Settled", + posted: Date.current.to_s, + transacted_at: (Date.current - 1).to_s, + pending: false + } + + with_env_overrides SIMPLEFIN_INCLUDE_PENDING: nil do + assert_difference "@account.entries.count", 1 do + SimplefinEntry::Processor.new(tx, simplefin_account: @simplefin_account).process + end + end + end + + test "SIMPLEFIN_INCLUDE_PENDING env var takes precedence over Setting" do + # Setting says "skip pending", but the env var (mirrored via the boot-time config it + # populates) says "include pending" - env var must win, matching + # SimplefinItem::Importer#fetch_accounts_data's effective_pending resolution. + Setting.stubs(:syncs_include_pending).returns(false) + Rails.configuration.x.simplefin.stubs(:include_pending).returns(true) + + tx = { + id: "tx_pending_env_override_1", + amount: "-30.00", + currency: "USD", + payee: "Test Store", + description: "Auth hold", + posted: Date.current.to_s, + transacted_at: (Date.current - 1).to_s, + pending: true + } + + with_env_overrides SIMPLEFIN_INCLUDE_PENDING: "1" do + assert_difference "@account.entries.count", 1 do + SimplefinEntry::Processor.new(tx, simplefin_account: @simplefin_account).process + end + end + end + + test "SIMPLEFIN_INCLUDE_PENDING env var disabling pending takes precedence over a permissive Setting" do + # Mirror of the test above: this is the actual real-world guard scenario the PR + # fixes - a self-hoster sets SIMPLEFIN_INCLUDE_PENDING=0 while the Setting (UI + # toggle) still says "include pending". The env var must win and skip the row. + Setting.stubs(:syncs_include_pending).returns(true) + Rails.configuration.x.simplefin.stubs(:include_pending).returns(false) + + tx = { + id: "tx_pending_env_disable_1", + amount: "-30.00", + currency: "USD", + payee: "Test Store", + description: "Auth hold", + posted: Date.current.to_s, + transacted_at: (Date.current - 1).to_s, + pending: true + } + + with_env_overrides SIMPLEFIN_INCLUDE_PENDING: "0" do + assert_no_difference "@account.entries.count" do + SimplefinEntry::Processor.new(tx, simplefin_account: @simplefin_account).process + end + end + end + test "infers pending when posted is explicitly 0 and transacted_at present (no explicit pending flag)" do # Some SimpleFIN banks indicate pending by sending posted=0 + transacted_at, without pending flag t_epoch = (Date.current - 1).to_time.to_i @@ -163,4 +256,27 @@ class SimplefinEntry::ProcessorTest < ActiveSupport::TestCase sf = entry.transaction.extra.fetch("simplefin") assert_equal true, sf["pending"], "expected pending to be inferred from posted=0 + transacted_at present" end + + test "does not treat a non-numeric posted value as epoch-zero pending" do + # Regression: `posted_val.to_i.zero?` would also match malformed strings like + # "unavailable" (String#to_i coerces non-numeric input to 0), wrongly flagging a + # settled transaction as pending. Only literal 0 / "0" should count as epoch-zero. + tx = { + id: "tx_malformed_posted_1", + amount: "-11.00", + currency: "USD", + payee: "Test Store", + description: "Settled", + memo: "", + posted: "unavailable", + transacted_at: (Date.current - 1).to_s + # Note: NO pending flag set + } + + SimplefinEntry::Processor.new(tx, simplefin_account: @simplefin_account).process + + entry = @account.entries.find_by!(external_id: "simplefin_tx_malformed_posted_1", source: "simplefin") + sf = entry.transaction.extra.fetch("simplefin") + assert_equal false, sf["pending"], "expected a non-numeric posted value to not be inferred as pending" + end end