Files
sure/test/models/holding_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

519 lines
19 KiB
Ruby

require "test_helper"
require "ostruct"
require "bigdecimal"
class HoldingTest < ActiveSupport::TestCase
include EntriesTestHelper, SecuritiesTestHelper
setup do
@account = families(:empty).accounts.create!(name: "Test Brokerage", balance: 20000, cash_balance: 0, currency: "USD", accountable: Investment.new)
# Current day holding instances
@amzn, @nvda = load_holdings
end
test "calculates portfolio weight" do
expected_amzn_weight = 3240.0 / @account.balance * 100
expected_nvda_weight = 3720.0 / @account.balance * 100
assert_in_delta expected_amzn_weight, @amzn.weight, 0.001
assert_in_delta expected_nvda_weight, @nvda.weight, 0.001
end
test "calculates portfolio weight after converting foreign-currency holdings" do
ExchangeRate.create!(from_currency: "EUR", to_currency: "USD", date: Date.current, rate: 1.5)
foreign_security = Security.create!(ticker: "ASML", name: "ASML")
foreign_holding = @account.holdings.create!(
security: foreign_security,
date: Date.current,
qty: 1,
price: 100,
amount: 100,
currency: "EUR"
)
assert_in_delta 0.75, foreign_holding.weight, 0.001
end
test "calculates average cost basis" do
create_trade(@amzn.security, account: @account, qty: 10, price: 212.00, date: 1.day.ago.to_date)
create_trade(@amzn.security, account: @account, qty: 15, price: 216.00, date: Date.current)
create_trade(@nvda.security, account: @account, qty: 5, price: 128.00, date: 1.day.ago.to_date)
create_trade(@nvda.security, account: @account, qty: 30, price: 124.00, date: Date.current)
# expected weighted averages (quantity-weighted)
amzn_total = BigDecimal("10") * BigDecimal("212.00") + BigDecimal("15") * BigDecimal("216.00")
amzn_qty = BigDecimal("10") + BigDecimal("15")
expected_amzn = amzn_total / amzn_qty
nvda_total = BigDecimal("5") * BigDecimal("128.00") + BigDecimal("30") * BigDecimal("124.00")
nvda_qty = BigDecimal("5") + BigDecimal("30")
expected_nvda = nvda_total / nvda_qty
assert_equal Money.new(expected_amzn), @amzn.avg_cost
assert_equal Money.new(expected_nvda), @nvda.avg_cost
end
test "calculates average cost basis from another currency" do
create_trade(@amzn.security, account: @account, qty: 10, price: 212.00, date: 1.day.ago.to_date, currency: "CAD")
create_trade(@amzn.security, account: @account, qty: 15, price: 216.00, date: Date.current, currency: "CAD")
create_trade(@nvda.security, account: @account, qty: 5, price: 128.00, date: 1.day.ago.to_date, currency: "CAD")
create_trade(@nvda.security, account: @account, qty: 30, price: 124.00, date: Date.current, currency: "CAD")
# compute expected: sum(price * qty * rate) / sum(qty)
amzn_total_usd = BigDecimal("10") * BigDecimal("212.00") * BigDecimal("1") +
BigDecimal("15") * BigDecimal("216.00") * BigDecimal("1")
amzn_qty = BigDecimal("10") + BigDecimal("15")
expected_amzn_usd = amzn_total_usd / amzn_qty
nvda_total_usd = BigDecimal("5") * BigDecimal("128.00") * BigDecimal("1") +
BigDecimal("30") * BigDecimal("124.00") * BigDecimal("1")
nvda_qty = BigDecimal("5") + BigDecimal("30")
expected_nvda_usd = nvda_total_usd / nvda_qty
ExchangeRate.stubs(:find_or_fetch_rate).returns(OpenStruct.new(rate: 1))
assert_equal Money.new(expected_amzn_usd, "CAD").exchange_to("USD"), @amzn.avg_cost
assert_equal Money.new(expected_nvda_usd, "CAD").exchange_to("USD"), @nvda.avg_cost
end
test "calculates total return trend" do
@amzn.stubs(:avg_cost).returns(Money.new(214.00))
@nvda.stubs(:avg_cost).returns(Money.new(126.00))
# Gained $30, or 0.93%
assert_equal Money.new(30), @amzn.trend.value
assert_in_delta 0.9, @amzn.trend.percent, 0.001
# Lost $60, or -1.59%
assert_equal Money.new(-60), @nvda.trend.value
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 when not locked" 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, cost_basis_locked: false)
assert_nil @amzn.avg_cost
end
test "avg_cost returns zero cost_basis when locked (e.g., airdrops)" do
# User-set $0 cost basis is valid for airdrops and should be honored
@amzn.update!(cost_basis: 0, cost_basis_source: "manual", cost_basis_locked: true)
assert_equal Money.new(0, "USD"), @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
# Cost basis source tracking tests
test "cost_basis_replaceable_by? returns false when locked" do
@amzn.update!(cost_basis: 200, cost_basis_source: "manual", cost_basis_locked: true)
assert_not @amzn.cost_basis_replaceable_by?("calculated")
assert_not @amzn.cost_basis_replaceable_by?("provider")
assert_not @amzn.cost_basis_replaceable_by?("manual")
end
test "cost_basis_replaceable_by? respects priority hierarchy and allows refreshes" do
# Provider data can be replaced by higher-priority sources (calculated/manual)
# and can be refreshed by provider again.
@amzn.update!(cost_basis: 200, cost_basis_source: "provider", cost_basis_locked: false)
assert @amzn.cost_basis_replaceable_by?("calculated")
assert @amzn.cost_basis_replaceable_by?("manual")
assert @amzn.cost_basis_replaceable_by?("provider")
# Calculated data can be replaced by manual and can be refreshed by calculated again.
@amzn.update!(cost_basis: 200, cost_basis_source: "calculated", cost_basis_locked: false)
assert @amzn.cost_basis_replaceable_by?("manual")
assert @amzn.cost_basis_replaceable_by?("calculated")
assert_not @amzn.cost_basis_replaceable_by?("provider")
# Manual data when LOCKED cannot be replaced by anything
@amzn.update!(cost_basis: 200, cost_basis_source: "manual", cost_basis_locked: true)
assert_not @amzn.cost_basis_replaceable_by?("manual")
assert_not @amzn.cost_basis_replaceable_by?("calculated")
assert_not @amzn.cost_basis_replaceable_by?("provider")
# Manual data when UNLOCKED can be replaced by calculated (enables recalculation)
@amzn.update!(cost_basis: 200, cost_basis_source: "manual", cost_basis_locked: false)
assert_not @amzn.cost_basis_replaceable_by?("manual")
assert @amzn.cost_basis_replaceable_by?("calculated")
assert_not @amzn.cost_basis_replaceable_by?("provider")
end
test "set_manual_cost_basis! sets value and locks" do
@amzn.set_manual_cost_basis!(BigDecimal("175.50"))
assert_equal BigDecimal("175.50"), @amzn.cost_basis
assert_equal "manual", @amzn.cost_basis_source
assert @amzn.cost_basis_locked?
end
test "unlock_cost_basis! allows future updates" do
@amzn.set_manual_cost_basis!(BigDecimal("175.50"))
@amzn.unlock_cost_basis!
assert_not @amzn.cost_basis_locked?
# Source remains manual but since unlocked, calculated could now overwrite
assert @amzn.cost_basis_replaceable_by?("calculated")
end
test "cost_basis_source_label returns correct translation" do
@amzn.update!(cost_basis_source: "manual")
assert_equal I18n.t("holdings.cost_basis_sources.manual"), @amzn.cost_basis_source_label
@amzn.update!(cost_basis_source: "calculated")
assert_equal I18n.t("holdings.cost_basis_sources.calculated"), @amzn.cost_basis_source_label
@amzn.update!(cost_basis_source: "provider")
assert_equal I18n.t("holdings.cost_basis_sources.provider"), @amzn.cost_basis_source_label
@amzn.update!(cost_basis_source: nil)
assert_nil @amzn.cost_basis_source_label
end
test "cost_basis_known? returns true only when source and positive value exist" do
@amzn.update!(cost_basis: nil, cost_basis_source: nil)
assert_not @amzn.cost_basis_known?
@amzn.update!(cost_basis: 200, cost_basis_source: nil)
assert_not @amzn.cost_basis_known?
@amzn.update!(cost_basis: nil, cost_basis_source: "provider")
assert_not @amzn.cost_basis_known?
@amzn.update!(cost_basis: 0, cost_basis_source: "provider")
assert_not @amzn.cost_basis_known?
@amzn.update!(cost_basis: 200, cost_basis_source: "provider")
assert @amzn.cost_basis_known?
end
# Precision and edge case tests
test "cost_basis precision is maintained with fractional shares" do
@amzn.update!(qty: BigDecimal("0.123456"))
@amzn.set_manual_cost_basis!(BigDecimal("100.123456"))
@amzn.reload
assert_in_delta 100.123456, @amzn.cost_basis.to_f, 0.0001
end
test "set_manual_cost_basis! with zero qty does not raise but saves the value" do
@amzn.update!(qty: 0)
@amzn.set_manual_cost_basis!(BigDecimal("100"))
# Value is stored but effectively meaningless with zero qty
assert_equal BigDecimal("100"), @amzn.cost_basis
assert @amzn.cost_basis_locked?
end
test "cost_basis_locked prevents all sources from overwriting" do
@amzn.set_manual_cost_basis!(BigDecimal("100"))
assert @amzn.cost_basis_locked?
# Verify all sources are blocked when locked
assert_not @amzn.cost_basis_replaceable_by?("provider")
assert_not @amzn.cost_basis_replaceable_by?("calculated")
assert_not @amzn.cost_basis_replaceable_by?("manual")
# Value should remain unchanged
assert_equal BigDecimal("100"), @amzn.cost_basis
end
test "unlocked manual allows only calculated to replace" do
@amzn.set_manual_cost_basis!(BigDecimal("100"))
@amzn.unlock_cost_basis!
assert_not @amzn.cost_basis_locked?
assert @amzn.cost_basis_replaceable_by?("calculated")
assert_not @amzn.cost_basis_replaceable_by?("provider")
assert_not @amzn.cost_basis_replaceable_by?("manual")
end
# Security remapping tests
test "security_replaceable_by_provider? returns false when locked" do
@amzn.update!(security_locked: true)
assert_not @amzn.security_replaceable_by_provider?
end
test "security_replaceable_by_provider? returns true when not locked" do
@amzn.update!(security_locked: false)
assert @amzn.security_replaceable_by_provider?
end
test "security_remapped? returns true when provider_security differs from security" do
other_security = create_security("GOOG", prices: [ { date: Date.current, price: 100.00 } ])
@amzn.update!(provider_security: other_security)
assert @amzn.security_remapped?
end
test "security_remapped? returns false when provider_security is nil" do
assert_nil @amzn.provider_security_id
assert_not @amzn.security_remapped?
end
test "security_remapped? returns false when provider_security equals security" do
@amzn.update!(provider_security: @amzn.security)
assert_not @amzn.security_remapped?
end
test "remap_security! changes holding security and locks it" do
old_security = @amzn.security
new_security = create_security("GOOG", prices: [ { date: Date.current, price: 100.00 } ])
@amzn.remap_security!(new_security)
assert_equal new_security, @amzn.security
assert @amzn.security_locked?
assert_equal old_security, @amzn.provider_security
end
test "remap_security! updates all holdings for the same security" do
old_security = @amzn.security
new_security = create_security("GOOG", prices: [ { date: Date.current, price: 100.00 } ])
# There are 2 AMZN holdings (from load_holdings) - yesterday and today
amzn_holdings_count = @account.holdings.where(security: old_security).count
assert_equal 2, amzn_holdings_count
@amzn.remap_security!(new_security)
# All holdings should now be for the new security
assert_equal 0, @account.holdings.where(security: old_security).count
assert_equal 2, @account.holdings.where(security: new_security).count
# All should be locked with provider_security set
@account.holdings.where(security: new_security).each do |h|
assert h.security_locked?
assert_equal old_security, h.provider_security
end
end
test "remap_security! moves trades to new security" do
old_security = @amzn.security
new_security = create_security("GOOG", prices: [ { date: Date.current, price: 100.00 } ])
# Create a trade for the old security
create_trade(old_security, account: @account, qty: 5, price: 100.00, date: Date.current)
assert_equal 1, @account.trades.where(security: old_security).count
@amzn.remap_security!(new_security)
# Trade should have moved to the new security
assert_equal 0, @account.trades.where(security: old_security).count
assert_equal 1, @account.trades.where(security: new_security).count
end
test "remap_security! does nothing when security is same" do
current_security = @amzn.security
@amzn.remap_security!(current_security)
assert_equal current_security, @amzn.security
assert_not @amzn.security_locked?
assert_nil @amzn.provider_security_id
end
test "remap_security! merges holdings on collision by combining qty and amount" do
new_security = create_security("GOOG", prices: [ { date: Date.current, price: 100.00 } ])
# Create an existing holding for the new security on the same date
existing_goog = @account.holdings.create!(
date: @amzn.date,
security: new_security,
qty: 5,
price: 100,
amount: 500,
currency: "USD"
)
amzn_security = @amzn.security
amzn_qty = @amzn.qty
amzn_amount = @amzn.amount
initial_count = @account.holdings.count
# Remap should merge by combining qty and amount
@amzn.remap_security!(new_security)
# The AMZN holding on collision date should be deleted, merged into GOOG
assert_equal initial_count - 1, @account.holdings.count
# The existing GOOG holding should have merged values
existing_goog.reload
assert_equal 5 + amzn_qty, existing_goog.qty
assert_equal 500 + amzn_amount, existing_goog.amount
# Merged holding should be locked to prevent provider overwrites
assert existing_goog.security_locked, "Merged holding should be locked"
# No holdings should remain for the old AMZN security
assert_equal 0, @account.holdings.where(security: amzn_security).count
end
test "reset_security_to_provider! restores original security" do
old_security = @amzn.security
new_security = create_security("GOOG", prices: [ { date: Date.current, price: 100.00 } ])
@amzn.remap_security!(new_security)
assert_equal new_security, @amzn.security
assert @amzn.security_locked?
@amzn.reset_security_to_provider!
assert_equal old_security, @amzn.security
assert_not @amzn.security_locked?
assert_nil @amzn.provider_security_id
end
test "reset_security_to_provider! moves trades back" do
old_security = @amzn.security
new_security = create_security("GOOG", prices: [ { date: Date.current, price: 100.00 } ])
create_trade(old_security, account: @account, qty: 5, price: 100.00, date: Date.current)
@amzn.remap_security!(new_security)
assert_equal 1, @account.trades.where(security: new_security).count
@amzn.reset_security_to_provider!
assert_equal 0, @account.trades.where(security: new_security).count
assert_equal 1, @account.trades.where(security: old_security).count
end
test "reset_security_to_provider! does nothing if not remapped" do
old_security = @amzn.security
@amzn.reset_security_to_provider!
assert_equal old_security, @amzn.security
assert_nil @amzn.provider_security_id
end
private
def load_holdings
security1 = create_security("AMZN", prices: [
{ date: 1.day.ago.to_date, price: 212.00 },
{ date: Date.current, price: 216.00 }
])
security2 = create_security("NVDA", prices: [
{ date: 1.day.ago.to_date, price: 128.00 },
{ date: Date.current, price: 124.00 }
])
create_holding(security1, 1.day.ago.to_date, 10)
amzn = create_holding(security1, Date.current, 15)
create_holding(security2, 1.day.ago.to_date, 5)
nvda = create_holding(security2, Date.current, 30)
[ amzn, nvda ]
end
def create_holding(security, date, qty)
price = Security::Price.find_by(date: date, security: security).price
@account.holdings.create! \
date: date,
security: security,
qty: qty,
price: price,
amount: qty * price,
currency: "USD"
end
# A coin bought elsewhere at one price and moved in at another was never
# bought here, so counting the day it arrived as its cost reports a gain of
# zero on a position that may have doubled.
test "a transfer does not set the cost basis" do
holding = holdings(:one)
holding.account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) }
assert_nil holding.avg_cost,
"a transferred position has no cost basis this app can know"
end
# `!=` is NULL for an unlabelled row, so a naive exclusion drops the ordinary
# purchases that carry no label — which is most of them.
test "an unlabelled purchase still sets it" do
holding = holdings(:one)
holding.account.trades.each { |t| t.update!(investment_activity_label: nil) }
assert_not_nil holding.avg_cost
end
# Averaging the purchases alone and applying that to every unit is the same
# fabrication in a quieter form.
test "a position mixing a purchase and a transfer has no knowable cost" do
holding = holdings(:one)
holding.account.trades.each { |t| t.update!(investment_activity_label: "Buy") }
holding.account.entries.create!(
date: holding.date - 1,
name: "Received 1 unit",
amount: -100,
currency: holding.currency,
entryable: Trade.new(
security: holding.security,
qty: 1,
price: 100,
currency: holding.currency,
investment_activity_label: Trade::TRANSFER_LABEL
)
)
assert_nil holding.avg_cost
end
# A figure the user typed is theirs, not ours to discard: they are saying
# what the position cost them, which is exactly what the app cannot work
# out on its own for a transfer.
test "a cost basis the user set survives a transfer" do
holding = holdings(:one)
holding.account.trades.each { |t| t.update!(investment_activity_label: Trade::TRANSFER_LABEL) }
holding.update_columns(cost_basis: 100, cost_basis_source: "manual")
assert_equal 100, holding.reload.avg_cost.amount.to_d
end
test "a purchase still sets it" do
holding = holdings(:one)
holding.account.trades.each { |t| t.update!(investment_activity_label: "Buy") }
assert_not_nil holding.avg_cost
end
end