diff --git a/app/models/account/provider_import_adapter.rb b/app/models/account/provider_import_adapter.rb index b4f8e17dc..d2bf6e990 100644 --- a/app/models/account/provider_import_adapter.rb +++ b/app/models/account/provider_import_adapter.rb @@ -726,26 +726,9 @@ def find_duplicate_transaction(date:, amount:, currency:, name: nil, exclude_ent query.order(created_at: :asc).first end - # Finds a pending transaction that likely matches a newly posted transaction - # Used to reconcile pending→posted when SimpleFIN gives different IDs for the same transaction - # - # @param date [Date, String] Posted transaction date - # @param amount [BigDecimal, Numeric] Transaction amount (must match exactly) - # @param currency [String] Currency code - # @param source [String] Provider name (e.g., "simplefin") - # @param date_window [Integer] Days to search around the posted date (default: 8) - # @return [Entry, nil] The pending entry or nil if not found def find_pending_transaction(date:, amount:, currency:, source:, date_window: 8) date = Date.parse(date.to_s) unless date.is_a?(Date) - # Look for entries that: - # 1. Same account (implicit via account.entries) - # 2. Same source (simplefin) - # 3. Same amount (exact match - this is the strongest signal) - # 4. Same currency - # 5. Date within window (pending can post days later) - # 6. Is a Transaction (not Trade or Valuation) - # 7. Has pending=true in transaction.extra["simplefin"]["pending"] or extra["plaid"]["pending"] candidates = account.entries .joins("INNER JOIN transactions ON transactions.id = entries.entryable_id AND entries.entryable_type = 'Transaction'") .where(source: source) @@ -759,6 +742,10 @@ def find_pending_transaction(date:, amount:, currency:, source:, date_window: 8) OR (transactions.extra -> 'enable_banking' ->> 'pending')::boolean = true SQL .order(date: :desc) # Prefer most recent pending transaction + .limit(2) + .to_a + + return nil if candidates.size != 1 # ambiguous same-amount pendings must not be auto-claimed (#2013) candidates.first end diff --git a/test/models/account/provider_import_adapter_test.rb b/test/models/account/provider_import_adapter_test.rb index 29ab62fb8..875f4f982 100644 --- a/test/models/account/provider_import_adapter_test.rb +++ b/test/models/account/provider_import_adapter_test.rb @@ -1179,8 +1179,9 @@ class Account::ProviderImportAdapterTest < ActiveSupport::TestCase end end - test "reconciles most recent pending when multiple exist" do - # Create two pending transactions with same amount + test "does not auto-claim pending when multiple same-amount candidates exist" do + # Two pending transactions with the same amount — this is the ambiguous case. + # Claiming either one could silently destroy a distinct transaction (issue #2013). older_pending = @adapter.import_transaction( external_id: "simplefin_older_pending", amount: 60.00, @@ -1201,8 +1202,8 @@ class Account::ProviderImportAdapterTest < ActiveSupport::TestCase extra: { "simplefin" => { "pending" => true } } ) - # Import posted - should match the most recent pending (by date) - assert_no_difference "@account.entries.count" do + # Posted with same amount — ambiguous, so a NEW entry must be created + assert_difference "@account.entries.count", 1 do posted_entry = @adapter.import_transaction( external_id: "simplefin_posted_recurring", amount: 60.00, @@ -1213,11 +1214,14 @@ class Account::ProviderImportAdapterTest < ActiveSupport::TestCase extra: { "simplefin" => { "pending" => false } } ) - # Should match the newer pending entry - assert_equal newer_pending.id, posted_entry.id - # Older pending should remain untouched - assert_equal "simplefin_older_pending", older_pending.reload.external_id + # Must be a brand-new entry, not a hijacked pending + assert_not_equal older_pending.id, posted_entry.id + assert_not_equal newer_pending.id, posted_entry.id end + + # Both original pendings must be untouched + assert_equal "simplefin_older_pending", older_pending.reload.external_id + assert_equal "simplefin_newer_pending", newer_pending.reload.external_id end test "find_pending_transaction returns nil when no pending transactions exist" do @@ -1242,6 +1246,87 @@ class Account::ProviderImportAdapterTest < ActiveSupport::TestCase assert_nil result end + # ============================================================================ + # Same-amount collision guard (issue #2013) + # ============================================================================ + + test "two same-amount ATM withdrawals both pending — posting one does not steal the other" do + # Exact scenario from issue #2013: + # Two $20 ATM withdrawals are both pending at the same time. When the first + # one posts, the system must NOT auto-claim the second pending (ambiguous), + # so both pending entries survive intact and a new posted entry is created. + pending_atm1 = @adapter.import_transaction( + external_id: "simplefin_pending_atm_1", + amount: 20.00, + currency: "USD", + date: Date.today - 5.days, + name: "ATM Withdrawal", + source: "simplefin", + extra: { "simplefin" => { "pending" => true } } + ) + + pending_atm2 = @adapter.import_transaction( + external_id: "simplefin_pending_atm_2", + amount: 20.00, + currency: "USD", + date: Date.today - 2.days, + name: "ATM Withdrawal", + source: "simplefin", + extra: { "simplefin" => { "pending" => true } } + ) + + # First ATM withdrawal posts — two same-amount pendings in window → ambiguous → new entry + assert_difference "@account.entries.count", 1 do + posted_entry = @adapter.import_transaction( + external_id: "simplefin_posted_atm_1", + amount: 20.00, + currency: "USD", + date: Date.today, + name: "ATM Withdrawal", + source: "simplefin", + extra: { "simplefin" => { "pending" => false } } + ) + + assert_not_equal pending_atm1.id, posted_entry.id + assert_not_equal pending_atm2.id, posted_entry.id + end + + # Both pending entries must remain untouched — no silent destruction + assert_equal "simplefin_pending_atm_1", pending_atm1.reload.external_id + assert pending_atm1.transaction.pending? + assert_equal "simplefin_pending_atm_2", pending_atm2.reload.external_id + assert pending_atm2.transaction.pending? + end + + test "posted transaction still claims the pending when it is the only same-amount candidate" do + # When there is exactly ONE matching pending, auto-claim must still work. + pending_entry = @adapter.import_transaction( + external_id: "simplefin_pending_coffee", + amount: 5.50, + currency: "USD", + date: Date.today - 2.days, + name: "Coffee Shop", + source: "simplefin", + extra: { "simplefin" => { "pending" => true } } + ) + + assert_no_difference "@account.entries.count" do + posted_entry = @adapter.import_transaction( + external_id: "simplefin_posted_coffee", + amount: 5.50, + currency: "USD", + date: Date.today, + name: "Coffee Shop", + source: "simplefin", + extra: { "simplefin" => { "pending" => false } } + ) + + assert_equal pending_entry.id, posted_entry.id + assert_equal "simplefin_posted_coffee", posted_entry.external_id + assert_not posted_entry.transaction.pending? + end + end + # ============================================================================ # Critical Direction Fix Tests (CITGO Bug Prevention) # ============================================================================