diff --git a/app/models/assistant/function/get_holdings.rb b/app/models/assistant/function/get_holdings.rb index 515888e8a..4cc277072 100644 --- a/app/models/assistant/function/get_holdings.rb +++ b/app/models/assistant/function/get_holdings.rb @@ -105,8 +105,8 @@ class Assistant::Function::GetHoldings < Assistant::Function amount: holding.amount.to_f, formatted_amount: holding.amount_money.format, weight: holding.weight&.round(2), - average_cost: holding.avg_cost.to_f, - formatted_average_cost: holding.avg_cost.format, + average_cost: holding.avg_cost&.to_f, + formatted_average_cost: holding.avg_cost&.format, account: holding.account.name, date: holding.date } diff --git a/app/models/holding.rb b/app/models/holding.rb index 0b7f602af..c2297db50 100644 --- a/app/models/holding.rb +++ b/app/models/holding.rb @@ -27,12 +27,16 @@ class Holding < ApplicationRecord account.balance.zero? ? 1 : amount / account.balance * 100 end - # Basic approximation of cost-basis + # Returns average cost per share, or nil if unknown. + # # Uses pre-computed cost_basis if available (set during materialization), - # otherwise falls back to calculating from trades + # otherwise falls back to calculating from trades. Returns nil when cost + # basis cannot be determined (no trades and no provider cost_basis). def avg_cost - # Use stored cost_basis if available (eliminates N+1 queries) - return Money.new(cost_basis, currency) if cost_basis.present? + # Use stored cost_basis if available and positive (eliminates N+1 queries) + # Note: cost_basis of 0 is treated as "unknown" since providers sometimes + # return 0 when they don't have the data + return Money.new(cost_basis, currency) if cost_basis.present? && cost_basis.positive? # Fallback to calculation for holdings without pre-computed cost_basis calculate_avg_cost @@ -75,6 +79,7 @@ class Holding < ApplicationRecord private def calculate_trend return nil unless amount_money + return nil unless avg_cost # Can't calculate trend without cost basis start_amount = qty * avg_cost @@ -83,6 +88,8 @@ class Holding < ApplicationRecord previous: start_amount end + # Calculates weighted average cost from buy trades. + # Returns nil if no trades exist (cost basis is unknown). def calculate_avg_cost trades = account.trades .with_entry @@ -101,13 +108,10 @@ class Holding < ApplicationRecord Arel.sql("SUM(trades.qty)") ) - weighted_avg = - if total_qty && total_qty > 0 - total_cost / total_qty - else - price - end + # Return nil when no trades exist - cost basis is genuinely unknown + # Previously this fell back to current market price, which was misleading + return nil unless total_qty && total_qty > 0 - Money.new(weighted_avg || price, currency) + Money.new(total_cost / total_qty, currency) end end diff --git a/app/models/holding/materializer.rb b/app/models/holding/materializer.rb index fee335fd6..da2950fa3 100644 --- a/app/models/holding/materializer.rb +++ b/app/models/holding/materializer.rb @@ -27,14 +27,38 @@ class Holding::Materializer end def persist_holdings + return if @holdings.empty? + current_time = Time.now - account.holdings.upsert_all( - @holdings.map { |h| h.attributes - .slice("date", "currency", "qty", "price", "amount", "security_id", "cost_basis") - .merge("account_id" => account.id, "updated_at" => current_time) }, - unique_by: %i[account_id security_id date currency] - ) + # Separate holdings into those with and without computed cost_basis + holdings_with_cost_basis, holdings_without_cost_basis = @holdings.partition { |h| h.cost_basis.present? } + + # Upsert holdings that have computed cost_basis (from trades) + # These will overwrite any existing provider cost_basis with the trade-derived value + if holdings_with_cost_basis.any? + account.holdings.upsert_all( + holdings_with_cost_basis.map { |h| + h.attributes + .slice("date", "currency", "qty", "price", "amount", "security_id", "cost_basis") + .merge("account_id" => account.id, "updated_at" => current_time) + }, + unique_by: %i[account_id security_id date currency] + ) + end + + # Upsert holdings WITHOUT cost_basis column - preserves existing provider cost_basis + # This handles securities that have no trades (e.g., SimpleFIN-only holdings) + if holdings_without_cost_basis.any? + account.holdings.upsert_all( + holdings_without_cost_basis.map { |h| + h.attributes + .slice("date", "currency", "qty", "price", "amount", "security_id") + .merge("account_id" => account.id, "updated_at" => current_time) + }, + unique_by: %i[account_id security_id date currency] + ) + end end def purge_stale_holdings diff --git a/app/models/investment_statement.rb b/app/models/investment_statement.rb index ee8daacfa..11cfdffa4 100644 --- a/app/models/investment_statement.rb +++ b/app/models/investment_statement.rb @@ -133,8 +133,12 @@ class InvestmentStatement holdings = current_holdings.to_a return nil if holdings.empty? - current = holdings.sum(&:amount) - previous = holdings.sum { |h| h.qty * h.avg_cost.amount } + # Only include holdings with known cost basis in the calculation + holdings_with_cost_basis = holdings.select(&:avg_cost) + return nil if holdings_with_cost_basis.empty? + + current = holdings_with_cost_basis.sum(&:amount) + previous = holdings_with_cost_basis.sum { |h| h.qty * h.avg_cost.amount } Trend.new(current: current, previous: previous) end diff --git a/app/views/holdings/_holding.html.erb b/app/views/holdings/_holding.html.erb index 4865af8ee..45ff4bf81 100644 --- a/app/views/holdings/_holding.html.erb +++ b/app/views/holdings/_holding.html.erb @@ -31,7 +31,7 @@
- <%= tag.p format_money holding.avg_cost %> + <%= tag.p holding.avg_cost ? format_money(holding.avg_cost) : t(".unknown"), class: holding.avg_cost ? nil : "text-secondary" %> <%= tag.p t(".per_share"), class: "font-normal text-secondary" %>
diff --git a/config/locales/views/holdings/en.yml b/config/locales/views/holdings/en.yml index 933a8b839..10b648965 100644 --- a/config/locales/views/holdings/en.yml +++ b/config/locales/views/holdings/en.yml @@ -8,6 +8,7 @@ en: holding: per_share: per share shares: "%{qty} shares" + unknown: "--" index: average_cost: Average cost holdings: Holdings diff --git a/test/models/holding/materializer_test.rb b/test/models/holding/materializer_test.rb index 41ae8ff62..5130743e8 100644 --- a/test/models/holding/materializer_test.rb +++ b/test/models/holding/materializer_test.rb @@ -26,4 +26,53 @@ class Holding::MaterializerTest < ActiveSupport::TestCase Holding::Materializer.new(@account, strategy: :forward).materialize_holdings end end + + test "preserves provider cost_basis when trade-derived cost_basis is nil" do + # Simulate a provider-imported holding with cost_basis (e.g., from SimpleFIN) + # This is the realistic scenario: linked account with provider holdings but no trades + provider_cost_basis = BigDecimal("150.00") + holding = Holding.create!( + account: @account, + security: @aapl, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + date: Date.current, + cost_basis: provider_cost_basis + ) + + # Use :reverse strategy (what linked accounts use) - doesn't purge holdings + # The AAPL holding has no trades, so computed cost_basis is nil + # The materializer should preserve the provider cost_basis, not overwrite with nil + Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings + + holding.reload + assert_equal provider_cost_basis, holding.cost_basis, + "Provider cost_basis should be preserved when no trades exist for this security" + end + + test "updates cost_basis when trade-derived cost_basis is available" do + # Create a holding with provider cost_basis + Holding.create!( + account: @account, + security: @aapl, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + date: Date.current, + cost_basis: BigDecimal("150.00") # Provider says $150 + ) + + # Create a trade that gives us a different cost basis + create_trade(@aapl, account: @account, qty: 10, price: 180, date: Date.current) + + # Use :reverse strategy - with trades, it should compute cost_basis from them + Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings + + holding = @account.holdings.find_by(security: @aapl, date: Date.current) + assert_equal BigDecimal("180.00"), holding.cost_basis, + "Trade-derived cost_basis should override provider cost_basis when available" + end end diff --git a/test/models/holding_test.rb b/test/models/holding_test.rb index ea44d918b..a3f4e2039 100644 --- a/test/models/holding_test.rb +++ b/test/models/holding_test.rb @@ -75,6 +75,43 @@ class HoldingTest < ActiveSupport::TestCase assert_in_delta -1.6, @nvda.trend.percent, 0.001 end + test "avg_cost returns nil when no trades exist and no stored cost_basis" do + # Holdings created without trades should return nil for avg_cost + # This prevents displaying fake $0 gain/loss based on current market price + assert_nil @amzn.avg_cost + assert_nil @nvda.avg_cost + end + + test "avg_cost uses stored cost_basis when available" do + # Simulate provider-supplied cost_basis (e.g., from SimpleFIN) + @amzn.update!(cost_basis: 200.00) + + assert_equal Money.new(200.00, "USD"), @amzn.avg_cost + end + + test "avg_cost treats zero cost_basis as unknown" do + # Some providers return 0 when they don't have cost basis data + # This should be treated as "unknown" (return nil), not as $0 cost + @amzn.update!(cost_basis: 0) + + assert_nil @amzn.avg_cost + end + + test "trend returns nil when cost basis is unknown" do + # Without cost basis, we can't calculate unrealized gain/loss + assert_nil @amzn.trend + assert_nil @nvda.trend + end + + test "trend works when avg_cost is available" do + @amzn.update!(cost_basis: 214.00) + + # Current price is 216, cost basis is 214 + # Qty is 15, so gain = 15 * (216 - 214) = $30 + assert_not_nil @amzn.trend + assert_equal Money.new(30), @amzn.trend.value + end + private def load_holdings