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