From b8ebb24e8b51034f60b614ddade3463de9deef81 Mon Sep 17 00:00:00 2001 From: CrossDrain <32982516+CrossDrain@users.noreply.github.com> Date: Wed, 27 May 2026 21:33:08 +0000 Subject: [PATCH] fix(holdings): carry provider cost_basis forward to calculated rows past snapshot date (#1818) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(holdings): carry provider cost_basis forward to calculated rows Providers like IBKR Flex emit holdings on report_date and only include trades within the query window. The reverse calculator + gapfill therefore produces rows past report_date with nil cost_basis, even though the provider supplied a basis on the snapshot. That nil basis silently blanks `Trend`, the Reports "Total Return" card, the Top Holdings return column, and Gains by Tax Treatment, because every one of them gates on `holding.avg_cost`. When a calculated row would otherwise have no usable cost_basis, backfill it with the most recent provider-supplied cost_basis for the same (security, currency) on or before the holding date. Existing calculated/manual values are preserved (they outrank a provider carry-forward), and existing provider carry-forwards are refreshed when a newer snapshot supersedes them. * - Fix currency mismatch: provider snapshots were keyed by (security_id, currency) but calculated rows use account currency while IBKR provider rows use the security's native currency (e.g., USD vs EUR). Now keyed by security_id only; carry_forward_provider_cost_basis converts via Money#exchange_to at the snapshot date (same convention as ReverseCalculator for trade prices), with a ConversionError fallback. - Trim long inline comment to three lines - Fix safe-nav inconsistency: existing.cost_basis.positive? -> existing&.cost_basis&.positive? - Add test: refreshes stale carry-forward when a newer provider snapshot arrives - Add test: carry-forward is a no-op for forward-strategy accounts with no provider holdings * fix(holdings): prevent overwriting zero-valued manual cost basis Ensure that manual cost basis entries with a value of zero (e.g., for free shares) are not overwritten by provider carry-forward values during materialization. Additionally, updated the logic to allow zero-valued manual or calculated cost bases to be preserved, and added tests to verify currency conversion and error handling during cost basis carry-forward. * refactor(holdings): allow zero-valued cost basis in provider snapshots Remove the filter that restricted provider cost basis snapshots to values greater than zero. This ensures that manual cost basis entries with a value of zero (e.g., for free shares) are correctly captured and available for carry-forward logic. * perf(holdings): optimize provider cost basis snapshot lookup Filter provider cost basis snapshots by the security IDs present in the current holdings set to reduce the amount of data loaded into memory. * refactor(holdings): move PortfolioCache FX fix to dedicated branch Remove date-accurate exchange rate fix from this branch — it has been split into fix/portfolio-cache-historical-fx-rate to keep concerns separate. Co-Authored-By: Claude Sonnet 4.6 * revert(portfolio_cache): restore date-accurate FX in get_price 36676784 removed date: date from exchange_to intending to move it to fix/portfolio-cache-historical-fx-rate, but that branch was a duplicate of db1051d2 which was already in main. The revert therefore regressed portfolio_cache.rb below main's state. Restore the historical exchange rate lookup so this branch no longer removes a fix already present in main. * fix(portfolio_cache): restore date-accurate FX and its test 36676784 removed date: date from exchange_to and deleted the historical FX test, intending to carry them in fix/portfolio-cache-historical-fx-rate. That branch was a duplicate of db1051d2 already in main, so the removal regressed portfolio_cache.rb below main's state. Restore both. --------- Co-authored-by: Claude Sonnet 4.6 --- app/models/holding/materializer.rb | 67 +++++++- test/models/holding/materializer_test.rb | 197 +++++++++++++++++++++++ 2 files changed, 262 insertions(+), 2 deletions(-) diff --git a/app/models/holding/materializer.rb b/app/models/holding/materializer.rb index 5832e2033..689a43ad1 100644 --- a/app/models/holding/materializer.rb +++ b/app/models/holding/materializer.rb @@ -88,8 +88,27 @@ class Holding::Materializer "cost_basis_source" => reconciled[:cost_basis_source] ) else - # No cost_basis to set, or existing is better - don't touch cost_basis fields - holdings_to_upsert_without_cost << base_attrs + # No new calculated value — fall back to the most recent provider + # cost_basis for this security on or before the holding date. + # Calculated/manual values outrank a provider carry-forward. + existing_source = existing&.cost_basis_source + preserve_existing = existing&.cost_basis.present? && %w[calculated manual].include?(existing_source) + + if preserve_existing + holdings_to_upsert_without_cost << base_attrs + else + carried = carry_forward_provider_cost_basis(holding) + + if carried && (existing&.cost_basis != carried || existing_source != "provider") + holdings_to_upsert_with_cost << base_attrs.merge( + "cost_basis" => carried, + "cost_basis_source" => "provider" + ) + else + # No cost_basis to set, or existing is better - don't touch cost_basis fields + holdings_to_upsert_without_cost << base_attrs + end + end end end @@ -165,6 +184,50 @@ class Holding::Materializer [ holding.account_id || account.id, holding.security_id, holding.date, holding.currency ] end + # Returns the most recent provider-supplied cost_basis for the given holding's + # security on or before its date, converted to the holding's currency. + # Used to backfill calculated rows past the provider's last snapshot so + # reports keep showing trend data. + # + # Provider and calculated rows can be denominated in different currencies + # (e.g., IBKR reports USD holdings while the reverse calculator converts to + # the account's base currency). When they differ, the cost_basis is converted + # at the snapshot date — the same convention ReverseCalculator uses for trade + # prices — so the result is consistent with trade-derived cost_basis values. + def carry_forward_provider_cost_basis(holding) + snapshots = provider_cost_basis_snapshots[holding.security_id] + return nil if snapshots.blank? + + result = nil + snapshots.each do |snap_date, cost_basis, snap_currency| + break if snap_date > holding.date + result = [ cost_basis, snap_currency, snap_date ] + end + return nil unless result + + cost_basis, snap_currency, snap_date = result + return cost_basis if snap_currency == holding.currency + + Money.new(cost_basis, snap_currency).exchange_to(holding.currency, date: snap_date).amount + rescue Money::ConversionError + nil + end + + def provider_cost_basis_snapshots + @provider_cost_basis_snapshots ||= begin + ids = @holdings.map(&:security_id).uniq + account.holdings + .where.not(account_provider_id: nil) + .where.not(cost_basis: nil) + .where(security_id: ids) + .order(:date) # ascending required: carry_forward_provider_cost_basis scans and breaks on snap_date > holding.date + .pluck(:security_id, :currency, :date, :cost_basis) + .each_with_object(Hash.new { |h, k| h[k] = [] }) do |(security_id, currency, date, cost_basis), memo| + memo[security_id] << [ date, cost_basis, currency ] + end + end + end + def purge_stale_holdings portfolio_security_ids = account.trades.distinct.pluck(:security_id) diff --git a/test/models/holding/materializer_test.rb b/test/models/holding/materializer_test.rb index 605f00478..e975de3cd 100644 --- a/test/models/holding/materializer_test.rb +++ b/test/models/holding/materializer_test.rb @@ -155,6 +155,203 @@ class Holding::MaterializerTest < ActiveSupport::TestCase assert_equal [ account_provider.id ], today_holdings.pluck(:account_provider_id) end + test "carries forward provider cost_basis to calculated rows past the provider snapshot date" do + coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key") + coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "USD") + account_provider = AccountProvider.create!(account: @account, provider: coinstats_account) + + # Provider snapshot two days ago with known cost basis, but no trades. + # This mirrors IBKR Flex where the export ends on Friday but today is Sunday. + Holding.create!( + account: @account, + security: @aapl, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + date: 2.days.ago.to_date, + account_provider: account_provider, + cost_basis: BigDecimal("125.50"), + cost_basis_source: "provider" + ) + + Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings + + today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD") + assert_nil today_holding.account_provider_id, + "Today's row is calculated, not a provider snapshot" + assert_equal BigDecimal("125.50"), today_holding.cost_basis, + "Today's calculated row should inherit the provider's cost_basis so trend/return calcs work" + assert_equal "provider", today_holding.cost_basis_source + end + + test "does not overwrite an existing calculated cost_basis with provider carry-forward" do + coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key") + coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "USD") + account_provider = AccountProvider.create!(account: @account, provider: coinstats_account) + + Holding.create!( + account: @account, + security: @aapl, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + date: 2.days.ago.to_date, + account_provider: account_provider, + cost_basis: BigDecimal("125.50"), + cost_basis_source: "provider" + ) + + # Pre-existing calculated row for today (e.g., from a prior trade-derived run) + Holding.create!( + account: @account, + security: @aapl, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + date: Date.current, + cost_basis: BigDecimal("180.00"), + cost_basis_source: "calculated" + ) + + Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings + + today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD") + assert_equal BigDecimal("180.00"), today_holding.cost_basis, + "Existing calculated cost_basis must beat provider carry-forward" + assert_equal "calculated", today_holding.cost_basis_source + end + + test "refreshes stale provider carry-forward when a newer provider snapshot arrives" do + coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key") + coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "USD") + account_provider = AccountProvider.create!(account: @account, provider: coinstats_account) + + # With no entries, start_date = yesterday, so materializer only descends to + # yesterday. Use an older date so the second snapshot doesn't land on a date + # the materializer already owns. + Holding.create!( + account: @account, security: @aapl, qty: 10, price: 200, amount: 2000, + currency: "USD", date: 5.days.ago.to_date, + account_provider: account_provider, + cost_basis: BigDecimal("100.00"), cost_basis_source: "provider" + ) + + Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings + + today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD") + assert_equal BigDecimal("100.00"), today_holding.cost_basis + + # Provider publishes a newer snapshot with an updated cost_basis on a date + # that falls outside the materializer's window (older than start_date). + Holding.create!( + account: @account, security: @aapl, qty: 10, price: 210, amount: 2100, + currency: "USD", date: 3.days.ago.to_date, + account_provider: account_provider, + cost_basis: BigDecimal("150.00"), cost_basis_source: "provider" + ) + + Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings + + today_holding.reload + assert_equal BigDecimal("150.00"), today_holding.cost_basis, + "Carry-forward should update to the newer provider snapshot value" + assert_equal "provider", today_holding.cost_basis_source + end + + test "carry-forward is a no-op for forward-strategy accounts without provider holdings" do + create_trade(@aapl, account: @account, qty: 5, price: 200, date: Date.current) + + assert_nothing_raised do + Holding::Materializer.new(@account, strategy: :forward).materialize_holdings + end + + today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD") + assert_equal "calculated", today_holding.cost_basis_source + assert_equal BigDecimal("200.00"), today_holding.cost_basis, + "Forward strategy with no provider rows should compute cost_basis from trades normally" + end + + test "does not overwrite a zero-valued manual cost_basis with provider carry-forward" do + coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key") + coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "USD") + account_provider = AccountProvider.create!(account: @account, provider: coinstats_account) + + Holding.create!( + account: @account, security: @aapl, + qty: 10, price: 200, amount: 2000, currency: "USD", + date: 2.days.ago.to_date, + account_provider: account_provider, + cost_basis: BigDecimal("125.50"), cost_basis_source: "provider" + ) + + # Free shares: legitimate zero-cost basis recorded manually + Holding.create!( + account: @account, security: @aapl, + qty: 10, price: 200, amount: 2000, currency: "USD", + date: Date.current, + cost_basis: BigDecimal("0"), cost_basis_source: "manual" + ) + + Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings + + today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD") + assert_equal BigDecimal("0"), today_holding.cost_basis, + "Zero-valued manual cost_basis (e.g., free shares) must not be overwritten by provider carry-forward" + assert_equal "manual", today_holding.cost_basis_source + end + + test "carry-forward converts provider cost_basis currency when provider and calculated currencies differ" do + snap_date = 2.days.ago.to_date + ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: snap_date, rate: 1.2) + + coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key") + coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "EUR") + account_provider = AccountProvider.create!(account: @account, provider: coinstats_account) + + Holding.create!( + account: @account, security: @aapl, + qty: 10, price: 200, amount: 2000, currency: "EUR", + date: snap_date, + account_provider: account_provider, + cost_basis: BigDecimal("100.00"), cost_basis_source: "provider" + ) + + Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings + + today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD") + assert_in_delta BigDecimal("120.00"), today_holding.cost_basis, BigDecimal("0.01"), + "Provider cost_basis in EUR should be converted to USD at the snapshot-date exchange rate" + assert_equal "provider", today_holding.cost_basis_source + end + + test "carry-forward skips provider cost_basis when FX conversion raises Money::ConversionError" do + snap_date = 2.days.ago.to_date + # No ExchangeRate created — EUR→USD conversion will raise Money::ConversionError + + coinstats_item = @family.coinstats_items.create!(name: "CoinStats", api_key: "test-key") + coinstats_account = coinstats_item.coinstats_accounts.create!(name: "Brokerage", currency: "EUR") + account_provider = AccountProvider.create!(account: @account, provider: coinstats_account) + + Holding.create!( + account: @account, security: @aapl, + qty: 10, price: 200, amount: 2000, currency: "EUR", + date: snap_date, + account_provider: account_provider, + cost_basis: BigDecimal("100.00"), cost_basis_source: "provider" + ) + + assert_nothing_raised do + Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings + end + + today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD") + assert_nil today_holding.cost_basis, + "Carry-forward should be skipped gracefully when currency conversion fails" + end + test "preserves same-day non-provider holdings for securities absent from the provider snapshot" do ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.2)