diff --git a/app/jobs/simplefin_holdings_apply_job.rb b/app/jobs/simplefin_holdings_apply_job.rb new file mode 100644 index 000000000..88022b822 --- /dev/null +++ b/app/jobs/simplefin_holdings_apply_job.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class SimplefinHoldingsApplyJob < ApplicationJob + queue_as :default + + # Idempotently materializes holdings for a SimplefinAccount by reading + # `raw_holdings_payload` and upserting Holding rows by (external_id) or + # (security,date,currency) via the ProviderImportAdapter used by the + # SimplefinAccount::Investments::HoldingsProcessor. + # + # Safe no-op when: + # - the SimplefinAccount is missing + # - there is no current linked Account + # - the linked Account is not an Investment/Crypto + # - there is no raw holdings payload + def perform(simplefin_account_id) + sfa = SimplefinAccount.find_by(id: simplefin_account_id) + return unless sfa + + account = sfa.current_account + return unless account + return unless [ "Investment", "Crypto" ].include?(account.accountable_type) + + holdings = Array(sfa.raw_holdings_payload) + return if holdings.empty? + + begin + SimplefinAccount::Investments::HoldingsProcessor.new(sfa).process + rescue => e + Rails.logger.warn("SimpleFin HoldingsApplyJob failed for SFA=#{sfa.id}: #{e.class} - #{e.message}") + end + end +end diff --git a/app/models/account/provider_import_adapter.rb b/app/models/account/provider_import_adapter.rb index f1cd3e70b..83f483fac 100644 --- a/app/models/account/provider_import_adapter.rb +++ b/app/models/account/provider_import_adapter.rb @@ -136,20 +136,63 @@ class Account::ProviderImportAdapter # Two strategies for finding/creating holdings: # 1. By external_id (SimpleFin approach) - tracks each holding uniquely # 2. By security+date+currency (Plaid approach) - overwrites holdings for same security/date - holding = if external_id.present? - account.holdings.find_or_initialize_by(external_id: external_id) do |h| - h.security = security - h.date = date - h.currency = currency + holding = nil + + if external_id.present? + # Preferred path: match by provider's external_id + holding = account.holdings.find_by(external_id: external_id) + + unless holding + # Fallback path: match by (security, date, currency) — and when provided, + # also scope by account_provider_id to avoid cross‑provider claiming. + # This keeps behavior symmetric with deletion logic below which filters + # by account_provider_id when present. + find_by_attrs = { + security: security, + date: date, + currency: currency + } + if account_provider_id.present? + find_by_attrs[:account_provider_id] = account_provider_id + end + + holding = account.holdings.find_by(find_by_attrs) end + + holding ||= account.holdings.new( + security: security, + date: date, + currency: currency, + account_provider_id: account_provider_id + ) else - account.holdings.find_or_initialize_by( + holding = account.holdings.find_or_initialize_by( security: security, date: date, currency: currency ) end + # Early cross-provider composite-key conflict guard: avoid attempting a write + # that would violate a unique index on (account_id, security_id, date, currency). + if external_id.present? + existing_composite = account.holdings.find_by( + security: security, + date: date, + currency: currency + ) + + if existing_composite && + account_provider_id.present? && + existing_composite.account_provider_id.present? && + existing_composite.account_provider_id != account_provider_id + Rails.logger.warn( + "ProviderImportAdapter: cross-provider holding collision for account=#{account.id} security=#{security.id} date=#{date} currency=#{currency}; returning existing id=#{existing_composite.id}" + ) + return existing_composite + end + end + holding.assign_attributes( security: security, date: date, @@ -158,10 +201,66 @@ class Account::ProviderImportAdapter price: price, amount: amount, cost_basis: cost_basis, - account_provider_id: account_provider_id + account_provider_id: account_provider_id, + external_id: external_id ) - holding.save! + begin + Holding.transaction(requires_new: true) do + holding.save! + end + rescue ActiveRecord::RecordNotUnique => e + # Handle unique index collisions on (account_id, security_id, date, currency) + # that can occur when another provider (or concurrent import) already + # created a row for this composite key. Use the existing row and keep + # the outer transaction valid by isolating the error in a savepoint. + existing = account.holdings.find_by( + security: security, + date: date, + currency: currency + ) + + if existing + # If an existing row belongs to a different provider, do NOT claim it. + # Keep cross-provider isolation symmetrical with deletion logic. + if account_provider_id.present? && existing.account_provider_id.present? && existing.account_provider_id != account_provider_id + Rails.logger.warn( + "ProviderImportAdapter: cross-provider holding collision for account=#{account.id} security=#{security.id} date=#{date} currency=#{currency}; returning existing id=#{existing.id}" + ) + holding = existing + else + # Same provider (or unowned). Apply latest snapshot and attach external_id for idempotency. + updates = { + qty: quantity, + price: price, + amount: amount, + cost_basis: cost_basis + } + + # Adopt the row to this provider if it’s currently unowned + if account_provider_id.present? && existing.account_provider_id.nil? + updates[:account_provider_id] = account_provider_id + end + + # Attach external_id if provided and missing + if external_id.present? && existing.external_id.blank? + updates[:external_id] = external_id + end + + begin + # Use update_columns to avoid validations and keep this collision handler best-effort. + existing.update_columns(updates.compact) + rescue => _ + # Best-effort only; avoid raising in collision handler + end + + holding = existing + end + else + # Could not find an existing row; re-raise original error + raise e + end + end # Optionally delete future holdings for this security (Plaid behavior) # Only delete if ALL providers allow deletion (cross-provider check) diff --git a/app/models/simplefin_account/investments/holdings_processor.rb b/app/models/simplefin_account/investments/holdings_processor.rb index ee942fac2..ae7723206 100644 --- a/app/models/simplefin_account/investments/holdings_processor.rb +++ b/app/models/simplefin_account/investments/holdings_processor.rb @@ -101,11 +101,17 @@ class SimplefinAccount::Investments::HoldingsProcessor if !sym.include?(":") && (is_crypto_account || is_crypto_symbol || mentions_crypto) sym = "CRYPTO:#{sym}" end - # Use Security::Resolver to find or create the security - Security::Resolver.new(sym).resolve - rescue ArgumentError => e - Rails.logger.error "Failed to resolve SimpleFin security #{symbol}: #{e.message}" - nil + # Use Security::Resolver to find or create the security, but be resilient + begin + Security::Resolver.new(sym).resolve + rescue => e + # If provider search fails or any unexpected error occurs, fall back to an offline security + Rails.logger.warn "SimpleFin: resolver failed for symbol=#{sym}: #{e.class} - #{e.message}; falling back to offline security" + Security.find_or_initialize_by(ticker: sym).tap do |sec| + sec.offline = true if sec.respond_to?(:offline) && sec.offline != true + sec.save! if sec.changed? + end + end end def parse_holding_date(created_timestamp) diff --git a/app/models/simplefin_item/importer.rb b/app/models/simplefin_item/importer.rb index 95b5fc307..0e36af726 100644 --- a/app/models/simplefin_item/importer.rb +++ b/app/models/simplefin_item/importer.rb @@ -7,6 +7,7 @@ class SimplefinItem::Importer @simplefin_item = simplefin_item @simplefin_provider = simplefin_provider @sync = sync + @enqueued_holdings_job_ids = Set.new end def import @@ -496,10 +497,21 @@ class SimplefinItem::Importer attrs[:raw_transactions_payload] = merged_transactions end - # Preserve most recent holdings (don't overwrite current positions with older data) - if holdings.is_a?(Array) && holdings.any? && simplefin_account.raw_holdings_payload.blank? - attrs[:raw_holdings_payload] = holdings + # Track whether incoming holdings are new/changed so we can materialize and refresh balances + holdings_changed = false + if holdings.is_a?(Array) && holdings.any? + prior = simplefin_account.raw_holdings_payload.to_a + if prior != holdings + attrs[:raw_holdings_payload] = holdings + # Also mirror into raw_payload['holdings'] so downstream calculators can use it + raw = simplefin_account.raw_payload.is_a?(Hash) ? simplefin_account.raw_payload.deep_dup : {} + raw = raw.with_indifferent_access + raw[:holdings] = holdings + attrs[:raw_payload] = raw + holdings_changed = true + end end + simplefin_account.assign_attributes(attrs) # Inactive detection/toggling (non-blocking) @@ -516,6 +528,37 @@ class SimplefinItem::Importer begin simplefin_account.save! + + # Post-save side effects + acct = simplefin_account.current_account + if acct + # Refresh credit attributes when available-balance present + if acct.accountable_type == "CreditCard" && account_data[:"available-balance"].present? + begin + SimplefinAccount::Liabilities::CreditProcessor.new(simplefin_account).process + rescue => e + Rails.logger.warn("SimpleFin: credit post-import refresh failed for sfa=#{simplefin_account.id}: #{e.class} - #{e.message}") + end + end + + # If holdings changed for an investment/crypto account, enqueue holdings apply job and recompute cash balance + if holdings_changed && [ "Investment", "Crypto" ].include?(acct.accountable_type) + # Debounce per importer run per SFA + unless @enqueued_holdings_job_ids.include?(simplefin_account.id) + SimplefinHoldingsApplyJob.perform_later(simplefin_account.id) + @enqueued_holdings_job_ids << simplefin_account.id + end + + # Recompute cash balance using existing calculator; avoid altering canonical ledger balances + begin + calculator = SimplefinAccount::Investments::BalanceCalculator.new(simplefin_account) + new_cash = calculator.cash_balance + acct.update!(cash_balance: new_cash) + rescue => e + Rails.logger.warn("SimpleFin: cash balance recompute failed for sfa=#{simplefin_account.id}: #{e.class} - #{e.message}") + end + end + end rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e # Treat duplicates/validation failures as partial success: count and surface friendly error, then continue stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1 diff --git a/lib/tasks/simplefin_holdings_backfill.rake b/lib/tasks/simplefin_holdings_backfill.rake index 262f29811..1129866e1 100644 --- a/lib/tasks/simplefin_holdings_backfill.rake +++ b/lib/tasks/simplefin_holdings_backfill.rake @@ -96,7 +96,6 @@ namespace :sure do sfa = ap&.provider || SimplefinAccount.find_by(account: acct) sfas = Array.wrap(sfa).compact else - success = errors.empty? puts({ ok: false, error: "usage", message: "Provide one of item_id, account_id, or account_name" }.to_json) exit 1 end @@ -124,9 +123,9 @@ namespace :sure do if dry_run puts({ dry_run: true, sfa_id: sfa.id, account_id: account.id, name: sfa.name, would_process: count }.to_json) else - SimplefinAccount::Investments::HoldingsProcessor.new(sfa).process + SimplefinHoldingsApplyJob.perform_later(sfa.id) total_holdings_written += count - puts({ ok: true, sfa_id: sfa.id, account_id: account.id, name: sfa.name, processed: count }.to_json) + puts({ ok: true, sfa_id: sfa.id, account_id: account.id, name: sfa.name, enqueued: true, estimated_holdings: count }.to_json) end sleep(sleep_ms / 1000.0) if sleep_ms.positive? diff --git a/test/jobs/simplefin_holdings_apply_job_test.rb b/test/jobs/simplefin_holdings_apply_job_test.rb new file mode 100644 index 000000000..c480f0396 --- /dev/null +++ b/test/jobs/simplefin_holdings_apply_job_test.rb @@ -0,0 +1,63 @@ +require "test_helper" + +class SimplefinHoldingsApplyJobTest < ActiveSupport::TestCase + setup do + @family = families(:dylan_family) + @item = SimplefinItem.create!(family: @family, name: "SF", access_url: "https://example.com/x") + + @account = accounts(:investment) + + # Link SFA to existing investment account via legacy association for simplicity + @sfa = @item.simplefin_accounts.create!( + name: "Invest", + account_id: "sf_invest_1", + currency: "USD", + account_type: "investment", + current_balance: 10_000 + ) + @account.update!(simplefin_account_id: @sfa.id) + end + + test "materializes holdings from raw_holdings_payload and is idempotent" do + # Two holdings: one AAPL (existing security), one NEWCO (should be created) + @sfa.update!( + raw_holdings_payload: [ + { + "id" => "h1", + "symbol" => "AAPL", + "quantity" => 10, + "market_value" => 2000, + "currency" => "USD", + "as_of" => (Date.current - 2.days).to_s + }, + { + "id" => "h2", + "symbol" => "NEWCO", + "quantity" => 5, + "market_value" => 500, + "currency" => "USD", + "as_of" => Date.current.to_s + } + ] + ) + + assert_difference "Holding.where(account: @account).count", 2 do + SimplefinHoldingsApplyJob.perform_now(@sfa.id) + end + + + # Running again should not create duplicates (external_id uniqueness) + assert_no_difference "Holding.where(account: @account).count" do + SimplefinHoldingsApplyJob.perform_now(@sfa.id) + end + + holdings = @account.holdings.order(:external_id) + aapl = holdings.find { |h| h.security.ticker == "AAPL" } + refute_nil aapl + assert_equal 10, aapl.qty + assert_equal Money.new(2000, "USD"), aapl.amount_money + + newco_sec = Security.find_by(ticker: "NEWCO") + refute_nil newco_sec, "should create NEWCO security via resolver when missing" + end +end diff --git a/test/models/account/provider_import_adapter_cross_provider_test.rb b/test/models/account/provider_import_adapter_cross_provider_test.rb new file mode 100644 index 000000000..e78f55237 --- /dev/null +++ b/test/models/account/provider_import_adapter_cross_provider_test.rb @@ -0,0 +1,95 @@ +require "test_helper" + +# Ensures a provider cannot "claim" another provider's holdings when external_id lookup misses +# and a (security, date, currency) match exists. The fallback path must be scoped by account_provider_id. +class Account::ProviderImportAdapterCrossProviderTest < ActiveSupport::TestCase + test "does not claim holdings from a different provider when external_id is present and fallback kicks in" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + # Create two different account providers for the SAME account + # Provider A (e.g., Plaid) + ap_a = AccountProvider.create!( + account: investment_account, + provider: plaid_accounts(:one) + ) + + # Provider B (e.g., SimpleFin) + item = SimplefinItem.create!(family: families(:dylan_family), name: "SF Conn", access_url: "https://example.com/access") + sfa_b = SimplefinAccount.create!( + simplefin_item: item, + name: "SF Invest", + account_id: "sf_inv_cross_provider", + currency: "USD", + account_type: "investment", + current_balance: 1000 + ) + ap_b = AccountProvider.create!( + account: investment_account, + provider: sfa_b + ) + + # Use a date that will not collide with existing fixture holdings for this account + holding_date = Date.today - 3.days + + # Existing holding created by Provider A for (security, date, currency) + existing_a = investment_account.holdings.create!( + security: security, + date: holding_date, + qty: 1, + price: 100, + amount: 100, + currency: "USD", + account_provider_id: ap_a.id + ) + + # Now import for Provider B with an external_id that doesn't exist yet. + # Fallback should NOT "claim" Provider A's row because account_provider_id differs. + # Attempt import for Provider B with a conflicting composite key. + # Policy: do NOT create a duplicate row and do NOT claim Provider A's row. + assert_no_difference "investment_account.holdings.count" do + @result_b = adapter.import_holding( + security: security, + quantity: 2, + amount: 220, + currency: "USD", + date: holding_date, + price: 110, + cost_basis: nil, + external_id: "ext-b-1", + source: "simplefin", + account_provider_id: ap_b.id, + delete_future_holdings: false + ) + end + + # Provider A's holding remains unclaimed (no external_id added) and still owned by A + existing_a.reload + assert_nil existing_a.external_id + assert_equal ap_a.id, existing_a.account_provider_id + + # Adapter returns the existing A row for transparency + assert_equal existing_a.id, @result_b.id + + # Idempotency: importing again with the same external_id should not create another row + assert_no_difference "investment_account.holdings.count" do + again = adapter.import_holding( + security: security, + quantity: 2, + amount: 220, + currency: "USD", + date: holding_date, + price: 110, + cost_basis: nil, + external_id: "ext-b-1", + source: "simplefin", + account_provider_id: ap_b.id, + delete_future_holdings: false + ) + assert_equal existing_a.id, again.id + # Ensure external_id was NOT attached to A's row (no cross-provider claim) + assert_nil existing_a.reload.external_id + end + end +end diff --git a/test/models/account/provider_import_adapter_unowned_adoption_test.rb b/test/models/account/provider_import_adapter_unowned_adoption_test.rb new file mode 100644 index 000000000..3b84f9d18 --- /dev/null +++ b/test/models/account/provider_import_adapter_unowned_adoption_test.rb @@ -0,0 +1,90 @@ +require "test_helper" + +# Ensures an "unowned" composite row (no account_provider_id) is fully adopted on +# first collision: attributes are updated, external_id attached, and +# account_provider_id set to the importing provider. +class Account::ProviderImportAdapterUnownedAdoptionTest < ActiveSupport::TestCase + test "adopts unowned holding on unique-index collision by updating attrs and provider ownership" do + investment_account = accounts(:investment) + adapter = Account::ProviderImportAdapter.new(investment_account) + security = securities(:aapl) + + # Create a SimpleFin provider for this account (the importer) + item = SimplefinItem.create!(family: families(:dylan_family), name: "SF Conn", access_url: "https://example.com/access") + sfa = SimplefinAccount.create!( + simplefin_item: item, + name: "SF Invest", + account_id: "sf_inv_unowned_claim", + currency: "USD", + account_type: "investment", + current_balance: 1000 + ) + ap = AccountProvider.create!(account: investment_account, provider: sfa) + + holding_date = Date.today - 4.days + + # Existing composite row without provider ownership (unowned) + existing_unowned = investment_account.holdings.create!( + security: security, + date: holding_date, + qty: 1, + price: 100, + amount: 100, + currency: "USD", + account_provider_id: nil + ) + + # Import for SimpleFin with an external_id that will collide on composite key + # Adapter should NOT create a new row, but should update the existing one: + # - qty/price/amount/cost_basis updated + # - external_id attached + # - account_provider_id adopted to ap.id + assert_no_difference "investment_account.holdings.count" do + @result = adapter.import_holding( + security: security, + quantity: 2, + amount: 220, + currency: "USD", + date: holding_date, + price: 110, + cost_basis: nil, + external_id: "ext-unowned-1", + source: "simplefin", + account_provider_id: ap.id, + delete_future_holdings: false + ) + end + + existing_unowned.reload + + # Attributes updated + assert_equal 2, existing_unowned.qty + assert_equal 110, existing_unowned.price + assert_equal 220, existing_unowned.amount + + # Ownership and external_id adopted + assert_equal ap.id, existing_unowned.account_provider_id + assert_equal "ext-unowned-1", existing_unowned.external_id + + # Adapter returns the same row + assert_equal existing_unowned.id, @result.id + + # Idempotency: re-import should not create a duplicate and should return the same row + assert_no_difference "investment_account.holdings.count" do + again = adapter.import_holding( + security: security, + quantity: 2, + amount: 220, + currency: "USD", + date: holding_date, + price: 110, + cost_basis: nil, + external_id: "ext-unowned-1", + source: "simplefin", + account_provider_id: ap.id, + delete_future_holdings: false + ) + assert_equal existing_unowned.id, again.id + end + end +end diff --git a/test/models/simplefin_item/importer_post_import_test.rb b/test/models/simplefin_item/importer_post_import_test.rb new file mode 100644 index 000000000..9f722d47c --- /dev/null +++ b/test/models/simplefin_item/importer_post_import_test.rb @@ -0,0 +1,74 @@ +require "test_helper" + +class SimplefinItem::ImporterPostImportTest < ActiveSupport::TestCase + setup do + @family = families(:dylan_family) + @item = SimplefinItem.create!(family: @family, name: "SF Conn", access_url: "https://example.com/access") + @sync = Sync.create!(syncable: @item) + end + + test "credit account import updates available_credit when available-balance provided" do + credit_acct = accounts(:credit_card) + + sfa = @item.simplefin_accounts.create!( + name: "CC", + account_id: "sf_cc_1", + currency: "USD", + account_type: "credit", + available_balance: 0 + ) + # Link via legacy association + credit_acct.update!(simplefin_account_id: sfa.id) + + importer = SimplefinItem::Importer.new(@item, simplefin_provider: mock(), sync: @sync) + + account_data = { + id: sfa.account_id, + name: "CC", + balance: -1200.0, # liabilities often negative from provider + currency: "USD", + "available-balance": 5000.0 + } + + # Call private method for focused unit test + importer.send(:import_account, account_data) + + assert_equal 5000.0, credit_acct.reload.credit_card.available_credit + end + + test "investment import recalculates cash_balance when holdings payload changes" do + invest_acct = accounts(:investment) + + sfa = @item.simplefin_accounts.create!( + name: "Invest", + account_id: "sf_inv_1", + currency: "USD", + account_type: "investment", + current_balance: 0 + ) + invest_acct.update!(simplefin_account_id: sfa.id) + + importer = SimplefinItem::Importer.new(@item, simplefin_provider: mock(), sync: @sync) + + holdings = [ + { "id" => "h1", "symbol" => "AAPL", "quantity" => 10, "market_value" => 2000, "currency" => "USD", "as_of" => Date.current.to_s }, + { "id" => "h2", "symbol" => "MSFT", "quantity" => 20, "market_value" => 4000, "currency" => "USD", "as_of" => Date.current.to_s } + ] + + account_data = { + id: sfa.account_id, + name: "Invest", + balance: 10000.0, + currency: "USD", + holdings: holdings + } + + # Prevent the job from running in this unit test; we only care about cash balance recompute + SimplefinHoldingsApplyJob.expects(:perform_later).once + + importer.send(:import_account, account_data) + + # Cash balance should be total balance (10_000) minus market_value sum (6_000) = 4_000 + assert_equal 4000.0, invest_acct.reload.cash_balance + end +end