Improve handling of cost_basis during holding materialization and display (#619)

- Refactored `persist_holdings` to separate and conditionally upsert holdings with and without cost_basis.
- Updated `avg_cost` logic to treat 0 cost_basis as unknown and return nil when cost_basis cannot be determined.
- Modified trend and investment calculation to exclude holdings with unknown cost_basis.
- Adjusted `average_cost` formatting to handle nil values in API responses and views.
- Added comprehensive tests to ensure cost_basis preservation and fallback behavior.
- Localized `unknown` label for display when cost_basis is unavailable.

Co-authored-by: Josh Waldrep <joshua.waldrep5+github@gmail.com>

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
       }

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

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

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

app/views/holdings/_holding.html.erb

@@ -31,7 +31,7 @@
     </div>
 
     <div class="col-span-2 text-right">
-      <%= 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" %>
     </div>
 

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

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

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