Fix missing SimpleFIN investment account transactions (#562)

* Add tests and update logic for processing SimpleFIN investment transactions

- Added `SimplefinAccount::Transactions::ProcessorInvestmentTest` to validate dividend transaction processing, transaction linking, and stale linkage repairs.
- Enhanced `SimplefinItem#process_accounts` with stale linkage repair logic and detailed logging for unlinked accounts with transactions.
- Updated `SimplefinAccount::Transactions::Processor` for improved logging and error handling during transaction processing.
- Adjusted `SimplefinItem::Importer` to log detailed account and transaction information and use extended sync windows for investment accounts.

* Refactor `SimplefinItem#process_accounts` to use direct queries for fresh data and streamline stale linkage repair logic; update tests for improved coverage and clarity.

* Improve stale linkage repair logic in `SimplefinItem#repair_stale_linkages`

- Updated to handle multiple linked accounts matching the same unlinked account by selecting the first match.
- Added detailed logging to warn about multiple matches for easier debugging.

* Include `:linked_account` in `SimplefinItem#process_accounts` queries for more comprehensive account data processing.

* Expand `merge_transactions` logic with composite key fallback for deduplication; document edge cases.

---------

Co-authored-by: Josh Waldrep <joshua.waldrep5+github@gmail.com>
This commit is contained in:
LPW
2026-01-07 10:15:28 -05:00
committed by GitHub
parent 3b4ab735b0
commit 3f97f316e0
5 changed files with 528 additions and 87 deletions

View File

@@ -1,85 +1,33 @@
# SimpleFin Investment transactions processor
# Processes investment-specific transactions like trades, dividends, etc.
#
# NOTE: SimpleFIN transactions (dividends, contributions, etc.) for investment accounts
# are already processed by SimplefinAccount::Transactions::Processor, which handles ALL
# account types including investments. That processor uses SimplefinEntry::Processor
# which captures full metadata (merchant, notes, extra data).
#
# This processor is intentionally a no-op for transactions to avoid:
# 1. Duplicate processing of the same transactions
# 2. Overwriting richer data with less complete data
#
# Unlike Plaid (which has a separate investment_transactions endpoint), SimpleFIN returns
# all transactions in a single `transactions` array regardless of account type.
#
# Holdings are processed separately by SimplefinAccount::Investments::HoldingsProcessor.
class SimplefinAccount::Investments::TransactionsProcessor
def initialize(simplefin_account)
@simplefin_account = simplefin_account
end
def process
return unless simplefin_account.current_account&.accountable_type == "Investment"
return unless simplefin_account.raw_transactions_payload.present?
transactions_data = simplefin_account.raw_transactions_payload
transactions_data.each do |transaction_data|
process_investment_transaction(transaction_data)
end
# Intentionally a no-op for transactions.
# SimpleFIN investment transactions are already processed by the regular
# SimplefinAccount::Transactions::Processor which handles all account types.
#
# This avoids duplicate processing and ensures the richer metadata from
# SimplefinEntry::Processor (merchant, notes, extra) is preserved.
Rails.logger.debug "SimplefinAccount::Investments::TransactionsProcessor - Skipping (transactions handled by SimplefinAccount::Transactions::Processor)"
end
private
attr_reader :simplefin_account
def account
simplefin_account.current_account
end
def process_investment_transaction(transaction_data)
data = transaction_data.with_indifferent_access
amount = parse_amount(data[:amount])
posted_date = parse_date(data[:posted])
external_id = "simplefin_#{data[:id]}"
# Use the unified import adapter for consistent handling
import_adapter.import_transaction(
external_id: external_id,
amount: amount,
currency: account.currency,
date: posted_date,
name: data[:description] || "Investment transaction",
source: "simplefin"
)
rescue => e
Rails.logger.error("Failed to process SimpleFin investment transaction #{data[:id]}: #{e.message}")
end
def import_adapter
@import_adapter ||= Account::ProviderImportAdapter.new(account)
end
def parse_amount(amount_value)
parsed_amount = case amount_value
when String
BigDecimal(amount_value)
when Numeric
BigDecimal(amount_value.to_s)
else
BigDecimal("0")
end
# SimpleFin uses banking convention, Maybe expects opposite
-parsed_amount
rescue ArgumentError => e
Rails.logger.error "Failed to parse SimpleFin investment transaction amount: #{amount_value.inspect} - #{e.message}"
BigDecimal("0")
end
def parse_date(date_value)
case date_value
when String
Date.parse(date_value)
when Integer, Float
Time.at(date_value).to_date
when Time, DateTime
date_value.to_date
when Date
date_value
else
Rails.logger.error("SimpleFin investment transaction has invalid date value: #{date_value.inspect}")
raise ArgumentError, "Invalid date format: #{date_value.inspect}"
end
rescue ArgumentError, TypeError => e
Rails.logger.error("Failed to parse SimpleFin investment transaction date '#{date_value}': #{e.message}")
raise ArgumentError, "Unable to parse transaction date: #{date_value.inspect}"
end
end

View File

@@ -6,18 +6,40 @@ class SimplefinAccount::Transactions::Processor
end
def process
return unless simplefin_account.raw_transactions_payload.present?
transactions = simplefin_account.raw_transactions_payload.to_a
acct = simplefin_account.current_account
acct_info = acct ? "Account id=#{acct.id} name='#{acct.name}' type=#{acct.accountable_type}" : "NO LINKED ACCOUNT"
if transactions.empty?
Rails.logger.info "SimplefinAccount::Transactions::Processor - No transactions in raw_transactions_payload for simplefin_account #{simplefin_account.id} (#{simplefin_account.name}) - #{acct_info}"
return
end
Rails.logger.info "SimplefinAccount::Transactions::Processor - Processing #{transactions.count} transactions for simplefin_account #{simplefin_account.id} (#{simplefin_account.name}) - #{acct_info}"
# Log first few transaction IDs for debugging
sample_ids = transactions.first(3).map { |t| t.is_a?(Hash) ? (t[:id] || t["id"]) : nil }.compact
Rails.logger.info "SimplefinAccount::Transactions::Processor - Sample transaction IDs: #{sample_ids.inspect}"
processed_count = 0
error_count = 0
# Each entry is processed inside a transaction, but to avoid locking up the DB when
# there are hundreds or thousands of transactions, we process them individually.
simplefin_account.raw_transactions_payload.each do |transaction_data|
transactions.each do |transaction_data|
SimplefinEntry::Processor.new(
transaction_data,
simplefin_account: simplefin_account
).process
processed_count += 1
rescue => e
Rails.logger.error "Error processing SimpleFin transaction: #{e.message}"
error_count += 1
tx_id = transaction_data.is_a?(Hash) ? (transaction_data[:id] || transaction_data["id"]) : nil
Rails.logger.error "SimplefinAccount::Transactions::Processor - Error processing transaction #{tx_id}: #{e.class} - #{e.message}"
Rails.logger.error e.backtrace.first(5).join("\n") if e.backtrace
end
Rails.logger.info "SimplefinAccount::Transactions::Processor - Completed for simplefin_account #{simplefin_account.id}: #{processed_count} processed, #{error_count} errors"
end
private

View File

@@ -57,11 +57,163 @@ class SimplefinItem < ApplicationRecord
def process_accounts
# Process accounts linked via BOTH legacy FK and AccountProvider
simplefin_accounts.includes(:account, account_provider: :account).each do |simplefin_account|
# Only process if there's a linked account (via either system)
next unless simplefin_account.current_account.present?
# Use direct query to ensure fresh data from DB, bypassing any association cache
all_accounts = SimplefinAccount.where(simplefin_item_id: id).includes(:account, :linked_account, account_provider: :account).to_a
Rails.logger.info "=" * 60
Rails.logger.info "SimplefinItem#process_accounts START - Item #{id} (#{name})"
Rails.logger.info " Total SimplefinAccounts: #{all_accounts.count}"
# Log all accounts for debugging
all_accounts.each do |sfa|
acct = sfa.current_account
Rails.logger.info " - SimplefinAccount id=#{sfa.id} sf_account_id=#{sfa.account_id} name='#{sfa.name}'"
Rails.logger.info " linked_account: #{sfa.linked_account&.id || 'nil'}, account: #{sfa.account&.id || 'nil'}, current_account: #{acct&.id || 'nil'}"
Rails.logger.info " raw_transactions_payload count: #{sfa.raw_transactions_payload.to_a.count}"
end
# First, try to repair stale linkages (old SimplefinAccount linked but new one has data)
repair_stale_linkages(all_accounts)
# Re-fetch after repairs - use direct query for fresh data
all_accounts = SimplefinAccount.where(simplefin_item_id: id).includes(:account, :linked_account, account_provider: :account).to_a
linked = all_accounts.select { |sfa| sfa.current_account.present? }
unlinked = all_accounts.reject { |sfa| sfa.current_account.present? }
Rails.logger.info "SimplefinItem#process_accounts - After repair: #{linked.count} linked, #{unlinked.count} unlinked"
# Log unlinked accounts with transactions for debugging
unlinked_with_txns = unlinked.select { |sfa| sfa.raw_transactions_payload.to_a.any? }
if unlinked_with_txns.any?
Rails.logger.warn "SimplefinItem#process_accounts - #{unlinked_with_txns.count} UNLINKED account(s) have transactions that won't be processed:"
unlinked_with_txns.each do |sfa|
Rails.logger.warn " - SimplefinAccount id=#{sfa.id} name='#{sfa.name}' sf_account_id=#{sfa.account_id} txn_count=#{sfa.raw_transactions_payload.to_a.count}"
end
end
linked.each do |simplefin_account|
acct = simplefin_account.current_account
Rails.logger.info "SimplefinItem#process_accounts - Processing: SimplefinAccount id=#{simplefin_account.id} name='#{simplefin_account.name}' -> Account id=#{acct.id} name='#{acct.name}' type=#{acct.accountable_type}"
SimplefinAccount::Processor.new(simplefin_account).process
end
Rails.logger.info "SimplefinItem#process_accounts END"
Rails.logger.info "=" * 60
end
# Repairs stale linkages when user re-adds institution in SimpleFIN.
# When a user deletes and re-adds an institution in SimpleFIN, new account IDs are generated.
# This causes old SimplefinAccounts to remain "linked" but stale (no new data),
# while new SimplefinAccounts have data but are unlinked.
# This method detects such cases and transfers the linkage from old to new.
def repair_stale_linkages(all_accounts)
linked = all_accounts.select { |sfa| sfa.current_account.present? }
unlinked = all_accounts.reject { |sfa| sfa.current_account.present? }
Rails.logger.info "SimplefinItem#repair_stale_linkages - #{linked.count} linked, #{unlinked.count} unlinked SimplefinAccounts"
# Find unlinked accounts that have transactions
unlinked_with_data = unlinked.select { |sfa| sfa.raw_transactions_payload.to_a.any? }
if unlinked_with_data.any?
Rails.logger.info "SimplefinItem#repair_stale_linkages - Found #{unlinked_with_data.count} unlinked SimplefinAccount(s) with transactions:"
unlinked_with_data.each do |sfa|
Rails.logger.info " - id=#{sfa.id} name='#{sfa.name}' account_id=#{sfa.account_id} txn_count=#{sfa.raw_transactions_payload.to_a.count}"
end
end
return if unlinked_with_data.empty?
# For each unlinked account with data, try to find a matching linked account
unlinked_with_data.each do |new_sfa|
# Find linked SimplefinAccount with same name (case-insensitive).
stale_matches = linked.select do |old_sfa|
old_sfa.name.to_s.downcase.strip == new_sfa.name.to_s.downcase.strip
end
if stale_matches.size > 1
Rails.logger.warn "SimplefinItem#repair_stale_linkages - Multiple linked accounts match '#{new_sfa.name}': #{stale_matches.map(&:id).join(', ')}. Using first match."
end
stale_match = stale_matches.first
next unless stale_match
account = stale_match.current_account
Rails.logger.info "SimplefinItem#repair_stale_linkages - Found matching accounts:"
Rails.logger.info " - OLD: SimplefinAccount id=#{stale_match.id} account_id=#{stale_match.account_id} txn_count=#{stale_match.raw_transactions_payload.to_a.count}"
Rails.logger.info " - NEW: SimplefinAccount id=#{new_sfa.id} account_id=#{new_sfa.account_id} txn_count=#{new_sfa.raw_transactions_payload.to_a.count}"
Rails.logger.info " - Linked to Account: '#{account.name}' (id=#{account.id})"
# Transfer the linkage from old to new
begin
# Merge transactions from old to new before transferring
old_transactions = stale_match.raw_transactions_payload.to_a
new_transactions = new_sfa.raw_transactions_payload.to_a
if old_transactions.any?
Rails.logger.info "SimplefinItem#repair_stale_linkages - Merging #{old_transactions.count} transactions from old SimplefinAccount"
merged = merge_transactions(old_transactions, new_transactions)
new_sfa.update!(raw_transactions_payload: merged)
end
# Check if linked via legacy FK (use to_s for UUID comparison safety)
if account.simplefin_account_id.to_s == stale_match.id.to_s
account.simplefin_account_id = new_sfa.id
account.save!
end
# Check if linked via AccountProvider
if stale_match.account_provider.present?
Rails.logger.info "SimplefinItem#repair_stale_linkages - Transferring AccountProvider linkage from SimplefinAccount #{stale_match.id} to #{new_sfa.id}"
stale_match.account_provider.update!(provider: new_sfa)
end
# If the new one doesn't have an AccountProvider yet, create one
new_sfa.ensure_account_provider!
Rails.logger.info "SimplefinItem#repair_stale_linkages - Successfully transferred linkage for Account '#{account.name}' to SimplefinAccount id=#{new_sfa.id}"
# Clear transactions from stale SimplefinAccount and leave it orphaned
# We don't destroy it because has_one :account, dependent: :nullify would nullify the FK we just set
# IMPORTANT: Use update_all to bypass AR associations - stale_match.update! would
# trigger autosave on the preloaded account association, reverting the FK we just set!
SimplefinAccount.where(id: stale_match.id).update_all(raw_transactions_payload: [], raw_holdings_payload: [])
Rails.logger.info "SimplefinItem#repair_stale_linkages - Cleared data from stale SimplefinAccount id=#{stale_match.id} (leaving orphaned)"
rescue => e
Rails.logger.error "SimplefinItem#repair_stale_linkages - Failed to transfer linkage: #{e.class} - #{e.message}"
Rails.logger.error e.backtrace.first(5).join("\n") if e.backtrace
end
end
end
# Merge two arrays of transactions, deduplicating by ID.
# Fallback: uses composite key [posted, amount, description] when ID/fitid missing.
#
# Known edge cases with composite key fallback:
# 1. False positives: Two distinct transactions with identical posted/amount/description
# will be incorrectly merged (rare but possible).
# 2. Type inconsistency: If posted varies in type (String vs Integer), keys won't match.
# 3. Description variations: Minor differences (whitespace, case) prevent matching.
#
# SimpleFIN typically provides transaction IDs, so this fallback is rarely needed.
def merge_transactions(old_txns, new_txns)
by_id = {}
# Add old transactions first
old_txns.each do |tx|
t = tx.with_indifferent_access
key = t[:id] || t[:fitid] || [ t[:posted], t[:amount], t[:description] ]
by_id[key] = tx
end
# Add new transactions (overwrite old with same ID)
new_txns.each do |tx|
t = tx.with_indifferent_access
key = t[:id] || t[:fitid] || [ t[:posted], t[:amount], t[:description] ]
by_id[key] = tx
end
by_id.values
end
def schedule_account_syncs(parent_sync: nil, window_start_date: nil, window_end_date: nil)

View File

@@ -36,11 +36,11 @@ class SimplefinItem::Importer
if simplefin_item.last_synced_at.nil? || no_txns_yet
# First sync (or balances-only pre-run) — use chunked approach to get full history
Rails.logger.info "SimplefinItem::Importer - Using chunked history import"
Rails.logger.info "SimplefinItem::Importer - Using CHUNKED HISTORY import (last_synced_at=#{simplefin_item.last_synced_at.inspect}, no_txns_yet=#{no_txns_yet})"
import_with_chunked_history
else
# Regular sync - use single request with buffer
Rails.logger.info "SimplefinItem::Importer - Using regular sync"
Rails.logger.info "SimplefinItem::Importer - Using REGULAR SYNC (last_synced_at=#{simplefin_item.last_synced_at&.strftime('%Y-%m-%d %H:%M')})"
import_regular_sync
end
rescue RateLimitedError => e
@@ -331,6 +331,7 @@ class SimplefinItem::Importer
# Step 2: Fetch transactions/holdings using the regular window.
start_date = determine_sync_start_date
Rails.logger.info "SimplefinItem::Importer - import_regular_sync: last_synced_at=#{simplefin_item.last_synced_at&.strftime('%Y-%m-%d %H:%M')} => start_date=#{start_date&.strftime('%Y-%m-%d')}"
accounts_data = fetch_accounts_data(start_date: start_date, pending: true)
return if accounts_data.nil? # Error already handled
@@ -372,6 +373,7 @@ class SimplefinItem::Importer
#
# Returns nothing; side-effects are snapshot + account upserts.
def perform_account_discovery
Rails.logger.info "SimplefinItem::Importer - perform_account_discovery START (no date params - transactions may be empty)"
discovery_data = fetch_accounts_data(start_date: nil)
discovered_count = discovery_data&.dig(:accounts)&.size.to_i
Rails.logger.info "SimpleFin discovery (no params) returned #{discovered_count} accounts"
@@ -547,6 +549,32 @@ class SimplefinItem::Importer
transactions = account_data[:transactions]
holdings = account_data[:holdings]
# Log detailed info for accounts with holdings (investment accounts) to debug missing transactions
# Note: SimpleFIN doesn't include a 'type' field, so we detect investment accounts by presence of holdings or name
acct_name = account_data[:name].to_s.downcase
has_holdings = holdings.is_a?(Array) && holdings.any?
is_investment = has_holdings || acct_name.include?("ira") || acct_name.include?("401k") || acct_name.include?("retirement") || acct_name.include?("brokerage")
# Always log for all accounts to trace the import flow
Rails.logger.info "SimplefinItem::Importer#import_account - account_id=#{account_id} name='#{account_data[:name]}' txn_count=#{transactions&.count || 0} holdings_count=#{holdings&.count || 0}"
if is_investment
Rails.logger.info "SimpleFIN Investment Account Debug - account_id=#{account_id} name='#{account_data[:name]}'"
Rails.logger.info " - API response keys: #{account_data.keys.inspect}"
Rails.logger.info " - transactions count: #{transactions&.count || 0}"
Rails.logger.info " - holdings count: #{holdings&.count || 0}"
Rails.logger.info " - existing raw_transactions_payload count: #{simplefin_account.raw_transactions_payload.to_a.count}"
# Log transaction data
if transactions.is_a?(Array) && transactions.any?
Rails.logger.info " - Transaction IDs: #{transactions.map { |t| t[:id] || t["id"] }.inspect}"
else
Rails.logger.warn " - NO TRANSACTIONS in API response for investment account!"
# Log what the transactions field actually contains
Rails.logger.info " - transactions raw value: #{account_data[:transactions].inspect}"
end
end
# Update all attributes; only update transactions if present to avoid wiping prior data
attrs = {
name: account_data[:name],
@@ -565,6 +593,8 @@ class SimplefinItem::Importer
if transactions.is_a?(Array) && transactions.any?
existing_transactions = simplefin_account.raw_transactions_payload.to_a
Rails.logger.info "SimplefinItem::Importer#import_account - Merging transactions for account_id=#{account_id}: #{existing_transactions.count} existing + #{transactions.count} new"
# Build a map of key => best_tx
best_by_key = {}
@@ -623,6 +653,8 @@ class SimplefinItem::Importer
merged_transactions = best_by_key.values
attrs[:raw_transactions_payload] = merged_transactions
Rails.logger.info "SimplefinItem::Importer#import_account - Merged result for account_id=#{account_id}: #{merged_transactions.count} total transactions"
# NOTE: Reconciliation disabled - it analyzes the SimpleFin API response
# which only contains ~90 days of history, creating misleading "gap" warnings
# that don't reflect actual database state. Re-enable if we improve it to
@@ -632,6 +664,8 @@ class SimplefinItem::Importer
# rescue => e
# Rails.logger.warn("SimpleFin: reconciliation failed for sfa=#{simplefin_account.id || account_id}: #{e.class} - #{e.message}")
# end
else
Rails.logger.info "SimplefinItem::Importer#import_account - No transactions in API response for account_id=#{account_id} (transactions=#{transactions.inspect.first(100)})"
end
# Track whether incoming holdings are new/changed so we can materialize and refresh balances
@@ -666,6 +700,11 @@ class SimplefinItem::Importer
begin
simplefin_account.save!
# Log final state after save for debugging
if is_investment
Rails.logger.info "SimplefinItem::Importer#import_account - SAVED account_id=#{account_id}: raw_transactions_payload now has #{simplefin_account.reload.raw_transactions_payload.to_a.count} transactions"
end
# Post-save side effects
acct = simplefin_account.current_account
if acct
@@ -835,15 +874,18 @@ class SimplefinItem::Importer
end
def initial_sync_lookback_period
# Default to 7 days for initial sync. Providers that support deeper
# history will supply it via chunked fetches, and users can optionally
# set a custom `sync_start_date` to go further back.
7
# Default to 60 days for initial sync to capture recent investment
# transactions (dividends, contributions, etc.). Providers that support
# deeper history will supply it via chunked fetches, and users can
# optionally set a custom `sync_start_date` to go further back.
60
end
def sync_buffer_period
# Default to 7 days buffer for subsequent syncs
7
# Default to 30 days buffer for subsequent syncs
# Investment accounts often have infrequent transactions (dividends, etc.)
# that would be missed with a shorter window
30
end
# Transaction reconciliation: detect potential data gaps or missing transactions