mirror of
https://github.com/we-promise/sure.git
synced 2026-07-16 06:45:21 +00:00
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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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" },
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user