From e4cfee0267cf13ea55da61cd7ea8cce9b40594f4 Mon Sep 17 00:00:00 2001 From: StalkerSea <35916410+StalkerSea@users.noreply.github.com> Date: Mon, 13 Jul 2026 18:33:35 -0500 Subject: [PATCH] Fix income trades (interest/dividend) wiping security holdings to $0 (#2673) Income trades created via Trade::CreateForm#create_income_trade set price: 0 on the Trade record. This zero price was ingested by PortfolioCache#load_prices as a valid price source, causing ForwardCalculator#build_holdings to compute holding amount = qty * 0 = $0, and overriding the security's market price on that date. Additionally, Balance::BaseCalculator#flows_for_date misclassified income trades (qty=0) as sells, producing spurious non_cash_outflows. Fixes: - PortfolioCache#load_prices: filter out zero-price trades from trade price sources using .select { |t| t.entryable.price&.positive? } - Balance::BaseCalculator#flows_for_date: separate income trades from regular trades using their qty (income trades have qty=0), so they contribute only to cash flows, not non-cash flows - Balance::SyncCache#converted_entries: preserve the entryable association target on duped entries so that e.entryable.qty access doesn't N+1 Fixes #2672 --- app/models/balance/base_calculator.rb | 17 +++++-- app/models/balance/sync_cache.rb | 3 ++ app/models/holding/portfolio_cache.rb | 7 +++ .../models/balance/forward_calculator_test.rb | 51 +++++++++++++++++++ test/models/holding/portfolio_cache_test.rb | 27 ++++++++++ test/support/ledger_testing_helper.rb | 23 +++++++++ 6 files changed, 124 insertions(+), 4 deletions(-) diff --git a/app/models/balance/base_calculator.rb b/app/models/balance/base_calculator.rb index 319362e0d..a980cffd9 100644 --- a/app/models/balance/base_calculator.rb +++ b/app/models/balance/base_calculator.rb @@ -97,15 +97,24 @@ class Balance::BaseCalculator txn_inflow_sum = entries.select { |e| e.amount < 0 && e.transaction? }.sum(&:amount) txn_outflow_sum = entries.select { |e| e.amount >= 0 && e.transaction? }.sum(&:amount) - trade_cash_inflow_sum = entries.select { |e| e.amount < 0 && e.trade? }.sum(&:amount) - trade_cash_outflow_sum = entries.select { |e| e.amount >= 0 && e.trade? }.sum(&:amount) + # Separate regular trades (buy/sell, affecting holdings) from income-only + # trades (interest/dividend with qty=0, which are cash-only events and + # must not produce spurious non_cash_outflows in the flow breakdown). + regular_trades = entries.select { |e| e.trade? && e.entryable.qty != 0 } + income_trades = entries.select { |e| e.trade? && e.entryable.qty == 0 } + + trade_cash_inflow_sum = regular_trades.select { |e| e.amount < 0 }.sum(&:amount) + trade_cash_outflow_sum = regular_trades.select { |e| e.amount >= 0 }.sum(&:amount) + + income_inflow_sum = income_trades.select { |e| e.amount < 0 }.sum(&:amount) + income_outflow_sum = income_trades.select { |e| e.amount >= 0 }.sum(&:amount) if account.balance_type == :non_cash && account.accountable_type == "Loan" non_cash_inflows = txn_inflow_sum.abs non_cash_outflows = txn_outflow_sum elsif account.balance_type != :non_cash - cash_inflows = txn_inflow_sum.abs + trade_cash_inflow_sum.abs - cash_outflows = txn_outflow_sum + trade_cash_outflow_sum + cash_inflows = txn_inflow_sum.abs + trade_cash_inflow_sum.abs + income_inflow_sum.abs + cash_outflows = txn_outflow_sum + trade_cash_outflow_sum + income_outflow_sum # Trades are inverse (a "buy" is outflow of cash, but "inflow" of non-cash, aka "holdings") non_cash_outflows = trade_cash_inflow_sum.abs diff --git a/app/models/balance/sync_cache.rb b/app/models/balance/sync_cache.rb index 58caf70ad..2bbc65d36 100644 --- a/app/models/balance/sync_cache.rb +++ b/app/models/balance/sync_cache.rb @@ -36,6 +36,9 @@ class Balance::SyncCache def converted_entries @converted_entries ||= account.entries.excluding_split_parents.includes(:entryable).order(:date).to_a.map do |e| converted_entry = e.dup + # dup does not copy the association cache, so the entryable would + # be re-fetched on access. Copy it to keep the preload active. + converted_entry.association(:entryable).target = e.entryable custom_rate = e.entryable.exchange_rate if e.entryable.respond_to?(:exchange_rate) diff --git a/app/models/holding/portfolio_cache.rb b/app/models/holding/portfolio_cache.rb index 4ea52920e..014c7ceb7 100644 --- a/app/models/holding/portfolio_cache.rb +++ b/app/models/holding/portfolio_cache.rb @@ -118,7 +118,14 @@ class Holding::PortfolioCache end # Medium priority prices from trades + # Exclude income entries (interest/dividend) — they are non-trading + # events with qty=0 and their zero price would clobber the security's + # market price on that date in the ForwardCalculator, producing a + # zero-amount holding. Use qty (the same heuristic as + # Balance::BaseCalculator) rather than price to avoid blocking + # non-income zero-price trades such as Questrade journal transfers. trade_prices = (trades_by_security_id[security.id] || []) + .reject { |t| t.entryable.qty == 0 } .map do |trade| PriceWithPriority.new( price: Security::Price.new( diff --git a/test/models/balance/forward_calculator_test.rb b/test/models/balance/forward_calculator_test.rb index b4f30c7c5..bdd24dc81 100644 --- a/test/models/balance/forward_calculator_test.rb +++ b/test/models/balance/forward_calculator_test.rb @@ -534,6 +534,57 @@ class Balance::ForwardCalculatorTest < ActiveSupport::TestCase ) end + test "investment account interest trade is classified as cash-only, not non-cash outflow" do + account = create_account_with_ledger( + account: { type: Investment, currency: "USD" }, + entries: [ + { type: "opening_anchor", date: 3.days.ago.to_date, balance: 5000 }, + { type: "trade", date: 1.day.ago.to_date, ticker: "AAPL", qty: 10, price: 100 }, + { type: "income_trade", date: Date.current, ticker: "AAPL", amount: 5, label: "Interest" } + ], + holdings: [ + { date: 1.day.ago.to_date, ticker: "AAPL", qty: 10, price: 100, amount: 1000 }, + { date: Date.current, ticker: "AAPL", qty: 10, price: 110, amount: 1100 } + ] + ) + + calculated = Balance::ForwardCalculator.new(account).calculate + + assert_calculated_ledger_balances( + calculated_data: calculated, + expected_data: [ + { + date: 3.days.ago.to_date, + legacy_balances: { balance: 5000, cash_balance: 5000 }, + balances: { start: 5000, start_cash: 5000, start_non_cash: 0, end_cash: 5000, end_non_cash: 0, end: 5000 }, + flows: 0, + adjustments: 0 + }, + { + date: 2.days.ago.to_date, + legacy_balances: { balance: 5000, cash_balance: 5000 }, + balances: { start: 5000, start_cash: 5000, start_non_cash: 0, end_cash: 5000, end_non_cash: 0, end: 5000 }, + flows: 0, + adjustments: 0 + }, + { + date: 1.day.ago.to_date, + legacy_balances: { balance: 5000, cash_balance: 4000 }, + balances: { start: 5000, start_cash: 5000, start_non_cash: 0, end_cash: 4000, end_non_cash: 1000, end: 5000 }, + flows: { cash_inflows: 0, cash_outflows: 1000, non_cash_inflows: 1000, non_cash_outflows: 0, net_market_flows: 0 }, + adjustments: 0 + }, + { + date: Date.current, + legacy_balances: { balance: 5105, cash_balance: 4005 }, + balances: { start: 5000, start_cash: 4000, start_non_cash: 1000, end_cash: 4005, end_non_cash: 1100, end: 5105 }, + flows: { cash_inflows: 5, cash_outflows: 0, non_cash_inflows: 0, non_cash_outflows: 0, net_market_flows: 100 }, + adjustments: 0 + } + ] + ) + end + test "investment account can have valuations that override balance" do account = create_account_with_ledger( account: { type: Investment, currency: "USD" }, diff --git a/test/models/holding/portfolio_cache_test.rb b/test/models/holding/portfolio_cache_test.rb index fed652165..5ea355395 100644 --- a/test/models/holding/portfolio_cache_test.rb +++ b/test/models/holding/portfolio_cache_test.rb @@ -57,6 +57,33 @@ class Holding::PortfolioCacheTest < ActiveSupport::TestCase assert_equal holding.price, cache.get_price(@security.id, holding.date).price end + test "excludes income trades with zero price from trade price sources" do + Security::Price.destroy_all + + @account.entries.create!( + name: "Interest: TEST", + date: Date.current, + amount: -50, + currency: "USD", + entryable: Trade.new( + qty: 0, + security: @security, + price: 0, + currency: "USD", + investment_activity_label: "Interest" + ) + ) + + cache = Holding::PortfolioCache.new(@account) + + # Income trade price=0 should be excluded; with no DB price for today, + # get_price returns nil instead of 0. + assert_nil cache.get_price(@security.id, Date.current) + + # The buy trade's price is still available on its own date. + assert_equal @trade.price, cache.get_price(@security.id, @trade.entry.date).price + end + test "converts historical prices using the requested date exchange rate" do account = families(:empty).accounts.create!( name: "CHF Brokerage", diff --git a/test/support/ledger_testing_helper.rb b/test/support/ledger_testing_helper.rb index a2266b679..8194979a0 100644 --- a/test/support/ledger_testing_helper.rb +++ b/test/support/ledger_testing_helper.rb @@ -88,6 +88,29 @@ module LedgerTestingHelper currency: currency, entryable: trade ) + when "income_trade" + security = Security.find_or_create_by!(ticker: entry_data[:ticker]) do |s| + s.name = entry_data[:ticker] + end + + currency = entry_data[:currency] || created_account.currency + + trade = Trade.new( + qty: 0, + security: security, + price: 0, + fee: 0, + currency: currency, + investment_activity_label: entry_data[:label] || "Interest" + ) + + created_account.entries.create!( + name: "#{entry_data[:label] || 'Interest'}: #{entry_data[:ticker]}", + date: entry_data[:date], + amount: -(entry_data[:amount].to_d), + currency: currency, + entryable: trade + ) end end