From a10af59f423fa61e54eab46dba7cfb7cd37b939b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Juan=20Jos=C3=A9=20Mata?= <juanjo.mata@gmail.com>
Date: Mon, 23 Mar 2026 19:39:32 +0100
Subject: [PATCH] Fix SimpleFIN holdings confusing market value with cost basis
 (#1182) (#1261)

Remove "value" from the market_value fallback chain in the SimpleFIN
HoldingsProcessor and add it to the cost_basis fallback chain instead.
Some brokerages (Vanguard, Fidelity) use "value" to represent cost basis,
causing the system to display average cost per share as the current price
and show massive phantom losses.

https://claude.ai/code/session_01V2gC6BPT3sF7Hu4XQgUQT4

Co-authored-by: Claude <noreply@anthropic.com>
---
 .../investments/holdings_processor.rb         |  7 +-
 .../jobs/simplefin_holdings_apply_job_test.rb | 66 +++++++++++++++++++
 2 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/app/models/simplefin_account/investments/holdings_processor.rb b/app/models/simplefin_account/investments/holdings_processor.rb
index 856b2c69d..3274b4b9d 100644
--- a/app/models/simplefin_account/investments/holdings_processor.rb
+++ b/app/models/simplefin_account/investments/holdings_processor.rb
@@ -42,9 +42,12 @@ class SimplefinAccount::Investments::HoldingsProcessor
         end
 
         # Parse provider data with robust fallbacks across SimpleFin sources
+        # NOTE: "value" is intentionally excluded from the market_value fallback chain
+        # because some brokerages (e.g. Vanguard, Fidelity) use "value" to mean cost basis,
+        # which would cause the system to display average cost as current price. (GH #1182)
         qty = parse_decimal(any_of(simplefin_holding, %w[shares quantity qty units]))
-        market_value = parse_decimal(any_of(simplefin_holding, %w[market_value value current_value]))
-        cost_basis = parse_decimal(any_of(simplefin_holding, %w[cost_basis basis total_cost]))
+        market_value = parse_decimal(any_of(simplefin_holding, %w[market_value current_value]))
+        cost_basis = parse_decimal(any_of(simplefin_holding, %w[cost_basis basis total_cost value]))
 
         # Derive price from market_value when possible; otherwise fall back to any price field
         fallback_price = parse_decimal(any_of(simplefin_holding, %w[purchase_price price unit_price average_cost avg_cost]))
diff --git a/test/jobs/simplefin_holdings_apply_job_test.rb b/test/jobs/simplefin_holdings_apply_job_test.rb
index 55a4e644e..7cc7b4c6c 100644
--- a/test/jobs/simplefin_holdings_apply_job_test.rb
+++ b/test/jobs/simplefin_holdings_apply_job_test.rb
@@ -61,4 +61,70 @@ class SimplefinHoldingsApplyJobTest < ActiveSupport::TestCase
     newco_sec = Security.find_by(ticker: "NEWCO")
     refute_nil newco_sec, "should create NEWCO security via resolver when missing"
   end
+
+  test "uses market_value for price and does not confuse value with market_value" do
+    # Regression test for GH #1182: some brokerages (Vanguard, Fidelity) include a
+    # "value" field that represents cost basis, not market value. The processor must
+    # use "market_value" for price derivation and treat "value" as a cost_basis fallback.
+    @account.holdings.delete_all
+
+    @sfa.update!(
+      raw_holdings_payload: [
+        {
+          "id" => "h_vanguard",
+          "symbol" => "VFIAX",
+          "shares" => 50,
+          "market_value" => 22626.42,
+          "cost_basis" => 22004.40,
+          "value" => 22004.40,
+          "currency" => "USD"
+        }
+      ]
+    )
+
+    assert_difference "Holding.where(account: @account).count", 1 do
+      SimplefinHoldingsApplyJob.perform_now(@sfa.id)
+    end
+
+    holding = @account.holdings.find_by(external_id: "simplefin_h_vanguard")
+    refute_nil holding
+
+    # Price should be derived from market_value / shares, NOT from value / shares
+    expected_price = BigDecimal("22626.42") / BigDecimal("50")
+    assert_in_delta expected_price.to_f, holding.price.to_f, 0.01,
+      "price should be market_value/qty (#{expected_price}), not value/qty"
+
+    # Amount should reflect market_value, not cost basis
+    assert_in_delta 22626.42, holding.amount.to_f, 0.01
+  end
+
+  test "falls back to value for cost_basis when cost_basis field is absent" do
+    @account.holdings.delete_all
+
+    @sfa.update!(
+      raw_holdings_payload: [
+        {
+          "id" => "h_fallback",
+          "symbol" => "FXAIX",
+          "shares" => 100,
+          "market_value" => 50000,
+          "value" => 45000,
+          "currency" => "USD"
+        }
+      ]
+    )
+
+    assert_difference "Holding.where(account: @account).count", 1 do
+      SimplefinHoldingsApplyJob.perform_now(@sfa.id)
+    end
+
+    holding = @account.holdings.find_by(external_id: "simplefin_h_fallback")
+    refute_nil holding
+
+    # Price derived from market_value
+    assert_in_delta 500.0, holding.price.to_f, 0.01
+
+    # cost_basis should fall back to "value" field (45000)
+    assert_in_delta 45000.0, holding.cost_basis.to_f, 0.01
+  end
 end