diff --git a/app/models/holding/materializer.rb b/app/models/holding/materializer.rb index 3fb704ca7..ae5ea3c1c 100644 --- a/app/models/holding/materializer.rb +++ b/app/models/holding/materializer.rb @@ -16,6 +16,14 @@ class Holding::Materializer purge_stale_holdings end + # Clean up calculated holdings for securities that now have provider-sourced holdings + # This prevents duplicates when a manually-entered account gets linked to a provider + cleanup_calculated_holdings_for_provider_securities + + # Reload holdings association to clear any cached stale data + # This ensures subsequent Balance calculations see the fresh holdings + account.holdings.reload + @holdings end @@ -39,6 +47,9 @@ class Holding::Materializer holdings_to_upsert_without_cost = [] @holdings.each do |holding| + # Skip securities that have provider-sourced holdings - don't overwrite provider data + next if provider_sourced_security_ids.include?(holding.security_id) + key = holding_key(holding) existing = existing_holdings_map[key] @@ -88,12 +99,37 @@ class Holding::Materializer # Load holdings that might affect reconciliation: # - Locked holdings (must preserve their cost_basis) # - Holdings with a source (need to check priority) + # - Provider-sourced holdings (must not be overwritten) account.holdings .where(cost_basis_locked: true) .or(account.holdings.where.not(cost_basis_source: nil)) + .or(account.holdings.where.not(account_provider_id: nil)) .index_by { |h| holding_key(h) } end + # Get security IDs that have provider-sourced holdings (any date) + # These should be preserved and not overwritten by calculated holdings + def provider_sourced_security_ids + @provider_sourced_security_ids ||= account.holdings + .where.not(account_provider_id: nil) + .distinct + .pluck(:security_id) + end + + # Remove calculated holdings (account_provider_id IS NULL) for securities + # that now have provider-sourced holdings. This prevents duplicates when + # a manually-entered account gets linked to a provider. + def cleanup_calculated_holdings_for_provider_securities + return if provider_sourced_security_ids.empty? + + deleted_count = account.holdings + .where(account_provider_id: nil) + .where(security_id: provider_sourced_security_ids) + .delete_all + + Rails.logger.info("Cleaned up #{deleted_count} calculated holdings for provider-sourced securities") if deleted_count > 0 + end + def holding_key(holding) [ holding.account_id || account.id, holding.security_id, holding.date, holding.currency ] end @@ -101,12 +137,16 @@ class Holding::Materializer def purge_stale_holdings portfolio_security_ids = account.entries.trades.map { |entry| entry.entryable.security_id }.uniq - # If there are no securities in the portfolio, delete all holdings + # Never delete provider-sourced holdings - they're authoritative from the provider + # If there are no securities in the portfolio, only delete non-provider holdings if portfolio_security_ids.empty? - Rails.logger.info("Clearing all holdings (no securities)") - account.holdings.delete_all + Rails.logger.info("Clearing non-provider holdings (no securities from trades)") + account.holdings.where(account_provider_id: nil).delete_all else - deleted_count = account.holdings.delete_by("date < ? OR security_id NOT IN (?)", account.start_date, portfolio_security_ids) + # Keep provider holdings and holdings for known securities within date range + deleted_count = account.holdings + .where(account_provider_id: nil) + .delete_by("date < ? OR security_id NOT IN (?)", account.start_date, portfolio_security_ids) Rails.logger.info("Purged #{deleted_count} stale holdings") if deleted_count > 0 end end diff --git a/app/models/simplefin_account.rb b/app/models/simplefin_account.rb index d505d41e9..2a6592317 100644 --- a/app/models/simplefin_account.rb +++ b/app/models/simplefin_account.rb @@ -23,12 +23,17 @@ class SimplefinAccount < ApplicationRecord acct = current_account return nil unless acct - AccountProvider + provider = AccountProvider .find_or_initialize_by(provider_type: "SimplefinAccount", provider_id: id) - .tap do |provider| - provider.account = acct - provider.save! + .tap do |p| + p.account = acct + p.save! end + + # Reload the association so future accesses don't return stale/nil value + reload_account_provider + + provider rescue => e Rails.logger.warn("SimplefinAccount##{id}: failed to ensure AccountProvider link: #{e.class} - #{e.message}") nil diff --git a/app/models/simplefin_account/investments/balance_calculator.rb b/app/models/simplefin_account/investments/balance_calculator.rb index 1e4780bc7..f940ca073 100644 --- a/app/models/simplefin_account/investments/balance_calculator.rb +++ b/app/models/simplefin_account/investments/balance_calculator.rb @@ -1,6 +1,22 @@ # SimpleFin Investment balance calculator # SimpleFin provides clear balance and holdings data, so calculations are simpler than Plaid class SimplefinAccount::Investments::BalanceCalculator + # Common money market fund tickers that should be treated as cash equivalents + # These are settlement funds that users consider "cash available to invest" + MONEY_MARKET_TICKERS = %w[ + VMFXX VMMXX VMRXX VUSXX + SPAXX FDRXX SPRXX FZFXX FDLXX + SWVXX SNVXX SNOXX + TTTXX PRTXX + ].freeze + + # Patterns that indicate money market funds (case-insensitive) + MONEY_MARKET_PATTERNS = [ + /money\s*market/i, + /settlement\s*fund/i, + /cash\s*reserve/i + ].freeze + def initialize(simplefin_account) @simplefin_account = simplefin_account end @@ -11,39 +27,64 @@ class SimplefinAccount::Investments::BalanceCalculator end def cash_balance - # Calculate cash balance as total balance minus holdings value + # Calculate cash balance as total balance minus non-cash holdings value + # Money market funds are treated as cash equivalents (settlement funds) total_balance = balance - holdings_value = total_holdings_value + non_cash_value = non_cash_holdings_value - cash = total_balance - holdings_value + cash = total_balance - non_cash_value - # Ensure non-negative cash balance - [ cash, BigDecimal("0") ].max + # Allow negative cash to represent margin debt (matching Plaid's approach) + # Log a warning for debugging, but don't clamp to zero + if cash.negative? + Rails.logger.info("SimpleFin: negative cash_balance (#{cash}) for account #{simplefin_account.account_id || simplefin_account.id} - may indicate margin usage or stale data") + end + + cash end private attr_reader :simplefin_account - def total_holdings_value - return BigDecimal("0") unless simplefin_account.raw_payload&.dig("holdings") + def holdings_data + @holdings_data ||= simplefin_account.raw_holdings_payload.presence || + simplefin_account.raw_payload&.dig("holdings") || + [] + end - holdings_data = simplefin_account.raw_payload["holdings"] + def non_cash_holdings_value + return BigDecimal("0") unless holdings_data.present? holdings_data.sum do |holding| - market_value = holding["market_value"] - begin - case market_value - when String - BigDecimal(market_value) - when Numeric - BigDecimal(market_value.to_s) - else - BigDecimal("0") - end - rescue ArgumentError => e - Rails.logger.warn "SimpleFin holdings market_value parse error for account #{simplefin_account.account_id || simplefin_account.id}: #{e.message} (value: #{market_value.inspect})" - BigDecimal("0") - end + # Skip money market funds - they're cash equivalents + next BigDecimal("0") if cash_equivalent?(holding) + + parse_market_value(holding["market_value"]) end end + + def cash_equivalent?(holding) + symbol = holding["symbol"].to_s.upcase.strip + description = holding["description"].to_s + + # Check known money market tickers + return true if MONEY_MARKET_TICKERS.include?(symbol) + + # Check description patterns + MONEY_MARKET_PATTERNS.any? { |pattern| description.match?(pattern) } + end + + def parse_market_value(market_value) + case market_value + when String + BigDecimal(market_value) + when Numeric + BigDecimal(market_value.to_s) + else + BigDecimal("0") + end + rescue ArgumentError => e + Rails.logger.warn "SimpleFin holdings market_value parse error for account #{simplefin_account.account_id || simplefin_account.id}: #{e.message} (value: #{market_value.inspect})" + BigDecimal("0") + end end diff --git a/app/models/simplefin_account/investments/holdings_processor.rb b/app/models/simplefin_account/investments/holdings_processor.rb index 052c28cb5..856b2c69d 100644 --- a/app/models/simplefin_account/investments/holdings_processor.rb +++ b/app/models/simplefin_account/investments/holdings_processor.rb @@ -63,8 +63,10 @@ class SimplefinAccount::Investments::HoldingsProcessor 0 end - # Use best-known date: created -> updated_at -> as_of -> date -> today - holding_date = parse_holding_date(any_of(simplefin_holding, %w[created updated_at as_of date])) || Date.current + # SimpleFIN holdings represent a current snapshot, not historical positions. + # Always use today's date regardless of the `created` timestamp (which is when + # the holding was first seen by SimpleFIN, not when we observed it). + holding_date = Date.current # Skip zero positions with no value to avoid invisible rows next if qty.to_d.zero? && computed_amount.to_d.zero? diff --git a/test/jobs/simplefin_holdings_apply_job_test.rb b/test/jobs/simplefin_holdings_apply_job_test.rb index c480f0396..55a4e644e 100644 --- a/test/jobs/simplefin_holdings_apply_job_test.rb +++ b/test/jobs/simplefin_holdings_apply_job_test.rb @@ -19,6 +19,9 @@ class SimplefinHoldingsApplyJobTest < ActiveSupport::TestCase end test "materializes holdings from raw_holdings_payload and is idempotent" do + # Clear existing fixture holdings so we can test clean creation + @account.holdings.delete_all + # Two holdings: one AAPL (existing security), one NEWCO (should be created) @sfa.update!( raw_holdings_payload: [ @@ -27,16 +30,14 @@ class SimplefinHoldingsApplyJobTest < ActiveSupport::TestCase "symbol" => "AAPL", "quantity" => 10, "market_value" => 2000, - "currency" => "USD", - "as_of" => (Date.current - 2.days).to_s + "currency" => "USD" }, { "id" => "h2", "symbol" => "NEWCO", "quantity" => 5, "market_value" => 500, - "currency" => "USD", - "as_of" => Date.current.to_s + "currency" => "USD" } ] )