QT/sure
1
0
Fork 0
mirror of https://github.com/we-promise/sure.git synced 2026-08-03 23:52:13 +00:00
Code Issues Packages Projects Releases Wiki Activity

Don’t re-create pending SimpleFIN transactions when pending sync is disabled (#2835)

Browse Source 
* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>
This commit is contained in:
PrplHaz4
2026-07-29 18:13:02 -05:00
committed by Juan José Mata
parent 5ca49392c4
commit 5fb8f4f81a
4 changed files with 342 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

63
app/models/simplefin_entry/processor.rb
View File

@@ -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

105
lib/tasks/simplefin_prune_pending.rake Normal file
View File

@@ -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

79
test/lib/tasks/simplefin_prune_pending_test.rb Normal file
View File

@@ -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

116
test/models/simplefin_entry/processor_test.rb
View File

@@ -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