Files
sure/test/models/holding/materializer_test.rb
T
buzzromainandClaude Opus 5 07524d0d84 fix(holdings): a transfer must not set a cost basis (#3154)
* fix(holdings): a transfer must not set a cost basis

calculate_avg_cost sums every trade with a positive quantity, so an asset moved
in from elsewhere is counted as bought on the day it arrived. A coin acquired at
30k and transferred in at 60k reports a cost of 60k and no gain at all — a
number that looks authoritative and is wrong.

Nothing here can know what a transferred asset cost: the purchase happened
somewhere this app never saw. Leaving the cost unknown is what the method
already does when it has nothing to work from, and for the same stated reason
the fallback to market price was removed from it: "Previously this fell back to
current market price, which was misleading."

Two things it would be easy to get wrong, and both are covered:

- **One transfer makes the whole position unknown**, not just its own row.
  Averaging the purchases alone and applying that to every unit is the same
  fabrication in a quieter form: buy one at 30k, receive one, and the position
  reports 30k a unit for two units that did not cost that.
- **Unlabelled purchases are preserved.** `!=` is NULL for a row with no label,
  so a naive exclusion would drop the ordinary trades that carry none — which
  is most of them. Hence IS DISTINCT FROM.

Balances and value are unaffected: they come from holdings, which providers
import from the position itself rather than from trade history.

This reaches every integration that labels a movement as a transfer. Questrade
journals already did; the self-custody wallets do as of #3153.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(holdings): stop a stored figure outranking the transfer guard

Review on #3154, and the reviewers were right that the first pass only
covered half the path.

`Holding#avg_cost` returns a stored `cost_basis` before it ever calls
`calculate_avg_cost`, so the transfer guard was protecting only holdings
that had nothing stored. Worse, the stored value was itself wrong: both
calculators counted every positive-quantity trade toward the running
average, transfers included, and the materializer persisted that as a
`calculated` basis. A coin bought elsewhere at 30k and moved in at 60k
reported no gain at all, and said so with a figure that looks derived.

Fixed in the write path rather than the read one. Adding an `exists?` per
holding to `avg_cost` would have reintroduced exactly the N+1 the stored
value exists to avoid; clearing the stored value instead lets the read
path fall through to the guard that was already there.

Both calculators now exclude transfers from the average and mark the
security's basis unknown — the forward one for good, the reverse one from
the transfer's date onward, since the purchases before it still stand on
their own. `cost_basis_unknown` is carried separately from a nil
`cost_basis` because the materializer treats them differently: nil means
"nothing computed, leave what is there", unknown means "this cannot be
known, clear what is there".

A `manual` or `provider` basis survives. That is somebody asserting what
the position cost them, which is precisely the thing the app cannot derive
for a transfer.

The migration clears figures already stored. Positions heal on the next
materialization anyway, but a manual or disconnected account may not
materialize again for a long time, and the wrong number is not visibly
wrong.

The regression test materializes first and relabels after, because that is
the case that matters: a position already carrying a figure worked out
before anyone knew the movement was a transfer.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye

* fix(holdings): clear a transferred basis that recorded no source

Follow-up on the same review. `load_existing_holdings_map` loaded holdings
that were locked, sourced, or provider-owned — so a row carrying a
`cost_basis` with no `cost_basis_source` was invisible to it. The clearing
then saw no existing holding and left the figure standing, which meant the
rows least able to justify the number they hold were the ones that kept it.

The migration takes `[7.2]` to match `schema.rb` and the other 398
migrations, rather than the `[8.0]` I had written.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye

* fix(holdings): renumber the migration off a colliding version

`20260825120000` is already taken by `add_consumed_amount_to_goals` on the
goals stack. Two migrations sharing a version is not a merge conflict —
`schema_migrations` is keyed by it, so whichever landed second would be
recorded as already run and skipped in silence. For a data migration that
means transferred positions quietly keeping the cost basis this branch
exists to clear.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye

* docs(holdings): say why the basis guard keys on one label, not the set

#3192 landed Trade::INTERNAL_MOVEMENT_LABELS in this file, next to the
TRANSFER_LABEL this branch adds. Both rest on ownership being preserved, so
two constants sitting together invite the question of why the basis guard does
not simply use the broader one.

It could, and that would be a behaviour change: the sweep labels would start
clearing a cost basis too. Nothing has shown a sweep landing on a security, and
widening a guard that erases figures on the strength of a guess is the wrong
direction, so it stays narrow and now says so.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-08-28 07:53:33 +02:00

470 lines
19 KiB
Ruby

require "test_helper"
class Holding::MaterializerTest < ActiveSupport::TestCase
include EntriesTestHelper
setup do
@family = families(:empty)
@account = @family.accounts.create!(name: "Test", balance: 20000, cash_balance: 20000, currency: "USD", accountable: Investment.new)
@aapl = securities(:aapl)
@msft = securities(:msft)
end
test "syncs holdings" do
create_trade(@aapl, account: @account, qty: 1, price: 200, date: Date.current)
# Should have yesterday's and today's holdings
assert_difference "@account.holdings.count", 2 do
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
end
end
# A position that takes in a transfer has no cost basis this app can know.
# A nil from the calculator alone left the previous calculated figure
# standing — the stale number reporting a transferred coin as if it had been
# bought on the day it arrived. `avg_cost` reads that stored value before it
# ever reaches the transfer guard, so the guard was only protecting holdings
# that had nothing stored at all.
#
# Materialised first and relabelled after, because that is the case that
# matters: a position already carrying a figure worked out before anyone
# knew the movement was a transfer.
test "a transfer clears a cost basis this app had worked out" do
create_trade(@aapl, account: @account, qty: 1, price: 200, date: Date.current)
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
assert_equal 200, latest_holding.cost_basis.to_d, "nothing was stored to clear"
@account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) }
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
assert_nil latest_holding.cost_basis, "the stale calculated figure survived the transfer"
assert_nil latest_holding.cost_basis_source
assert_nil latest_holding.avg_cost
end
# A row carrying a figure with no `cost_basis_source` was invisible to
# `load_existing_holdings_map`, so the clearing above saw no existing holding
# and left the stale basis standing — the rows least able to justify the
# number they hold being the ones that kept it.
test "a transfer clears a stored basis that never recorded where it came from" do
create_trade(@aapl, account: @account, qty: 1, price: 200, date: Date.current)
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
@account.holdings.where(security: @aapl).update_all(cost_basis: 200, cost_basis_source: nil)
@account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) }
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
assert_nil latest_holding.cost_basis, "a source-less figure outlived the transfer"
end
# Somebody asserted what this position cost them, which is exactly what the
# app cannot work out for a transfer. Theirs to keep.
test "a transfer leaves a provider cost basis alone" do
create_trade(@aapl, account: @account, qty: 1, price: 200, date: Date.current)
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
@account.holdings.where(security: @aapl).update_all(cost_basis: 150, cost_basis_source: "provider")
@account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) }
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
assert_equal 150, latest_holding.cost_basis.to_d
end
# An ordinary purchase is untouched: the exclusion keys off the label, and
# most rows carry none at all.
test "an ordinary purchase still gets its calculated basis" do
create_trade(@aapl, account: @account, qty: 1, price: 200, date: Date.current)
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
assert_equal 200, latest_holding.cost_basis.to_d
end
test "purges stale holdings for unlinked accounts" do
# Since the account has no entries, there should be no holdings
Holding.create!(account: @account, security: @aapl, qty: 1, price: 100, amount: 100, currency: "USD", date: Date.current)
assert_difference "Holding.count", -1 do
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
test "recalculates calculated cost_basis when new trades are added" do
date = Date.current
create_trade(@aapl, account: @account, qty: 1, price: 3000, date: date)
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
holding = @account.holdings.find_by!(security: @aapl, date: date, currency: "USD")
assert_equal "calculated", holding.cost_basis_source
assert_equal BigDecimal("3000.0"), holding.cost_basis
create_trade(@aapl, account: @account, qty: 1, price: 2500, date: date)
Holding::Materializer.new(@account, strategy: :forward).materialize_holdings
holding.reload
assert_equal "calculated", holding.cost_basis_source
assert_equal BigDecimal("2750.0"), holding.cost_basis
end
test "preserves calculated history for provider-sourced holdings on reverse materialization" 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: Date.current,
account_provider: account_provider
)
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
today_holding = @account.holdings.find_by!(security: @aapl, date: Date.current, currency: "USD")
yesterday_holding = @account.holdings.find_by!(security: @aapl, date: Date.yesterday, currency: "USD")
assert_equal account_provider.id, today_holding.account_provider_id
assert_nil yesterday_holding.account_provider_id
assert_equal BigDecimal("10"), yesterday_holding.qty
assert_equal yesterday_holding.qty * yesterday_holding.price, yesterday_holding.amount
end
test "cleans up calculated current-day holdings when a provider snapshot exists in another currency" do
ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, 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: "USD"
)
account_provider = AccountProvider.create!(account: @account, provider: coinstats_account)
Holding.create!(
account: @account,
security: @aapl,
qty: 10,
price: 200,
amount: 2000,
currency: "EUR",
date: Date.current,
account_provider: account_provider,
cost_basis: 150
)
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
today_holdings = @account.holdings.where(security: @aapl, date: Date.current).order(:currency)
assert_equal [ "EUR" ], today_holdings.pluck(:currency)
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)
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: "EUR",
date: Date.current,
account_provider: account_provider,
cost_basis: 150
)
manual_holding = Holding.create!(
account: @account,
security: @msft,
qty: 3,
price: 250,
amount: 750,
currency: "USD",
date: Date.current,
cost_basis: 225,
cost_basis_source: "manual",
cost_basis_locked: true
)
Holding::Materializer.new(@account, strategy: :reverse).materialize_holdings
assert_equal manual_holding.id, manual_holding.reload.id
assert_equal @msft.id, manual_holding.security_id
assert_nil manual_holding.account_provider_id
today_holdings = @account.holdings.where(date: Date.current)
assert_equal(
[ [ @aapl.id, "EUR" ], [ @msft.id, "USD" ] ].sort,
today_holdings.pluck(:security_id, :currency).sort
)
end
private
def latest_holding
@account.holdings.where(security: @aapl).order(:date).last
end
end