mirror of
https://github.com/we-promise/sure.git
synced 2026-04-19 12:04:08 +00:00
Add cost basis source tracking with manual override and lock protection (#623)
* Add cost basis tracking and management to holdings - Added migration to introduce `cost_basis_source` and `cost_basis_locked` fields to `holdings`. - Implemented backfill for existing holdings to set `cost_basis_source` based on heuristics. - Introduced `Holding::CostBasisReconciler` to manage cost basis resolution logic. - Added user interface components for editing and locking cost basis in holdings. - Updated `materializer` to integrate reconciliation logic and respect locked holdings. - Extended tests for cost basis-related workflows to ensure accuracy and reliability. * Fix cost basis calculation in holdings controller - Ensure `cost_basis` is converted to decimal for accurate arithmetic. - Fix conditional check to properly validate positive `cost_basis`. * Improve cost basis validation and error handling in holdings controller - Allow zero as a valid cost basis for gifted/inherited shares. - Add error handling with user feedback for invalid cost basis values. --------- Co-authored-by: Josh Waldrep <joshua.waldrep5+github@gmail.com>
This commit is contained in:
@@ -313,17 +313,32 @@ class Account::ProviderImportAdapter
|
||||
end
|
||||
end
|
||||
|
||||
holding.assign_attributes(
|
||||
# Reconcile cost_basis to respect priority hierarchy
|
||||
reconciled = Holding::CostBasisReconciler.reconcile(
|
||||
existing_holding: holding.persisted? ? holding : nil,
|
||||
incoming_cost_basis: cost_basis,
|
||||
incoming_source: "provider"
|
||||
)
|
||||
|
||||
# Build base attributes
|
||||
attributes = {
|
||||
security: security,
|
||||
date: date,
|
||||
currency: currency,
|
||||
qty: quantity,
|
||||
price: price,
|
||||
amount: amount,
|
||||
cost_basis: cost_basis,
|
||||
account_provider_id: account_provider_id,
|
||||
external_id: external_id
|
||||
)
|
||||
}
|
||||
|
||||
# Only update cost_basis if reconciliation says to
|
||||
if reconciled[:should_update]
|
||||
attributes[:cost_basis] = reconciled[:cost_basis]
|
||||
attributes[:cost_basis_source] = reconciled[:cost_basis_source]
|
||||
end
|
||||
|
||||
holding.assign_attributes(attributes)
|
||||
|
||||
begin
|
||||
Holding.transaction(requires_new: true) do
|
||||
@@ -353,11 +368,22 @@ class Account::ProviderImportAdapter
|
||||
updates = {
|
||||
qty: quantity,
|
||||
price: price,
|
||||
amount: amount,
|
||||
cost_basis: cost_basis
|
||||
amount: amount
|
||||
}
|
||||
|
||||
# Adopt the row to this provider if it’s currently unowned
|
||||
# Reconcile cost_basis to respect priority hierarchy
|
||||
collision_reconciled = Holding::CostBasisReconciler.reconcile(
|
||||
existing_holding: existing,
|
||||
incoming_cost_basis: cost_basis,
|
||||
incoming_source: "provider"
|
||||
)
|
||||
|
||||
if collision_reconciled[:should_update]
|
||||
updates[:cost_basis] = collision_reconciled[:cost_basis]
|
||||
updates[:cost_basis_source] = collision_reconciled[:cost_basis_source]
|
||||
end
|
||||
|
||||
# Adopt the row to this provider if it's currently unowned
|
||||
if account_provider_id.present? && existing.account_provider_id.nil?
|
||||
updates[:account_provider_id] = account_provider_id
|
||||
end
|
||||
|
||||
@@ -3,6 +3,16 @@ class Holding < ApplicationRecord
|
||||
|
||||
monetize :amount
|
||||
|
||||
# Cost basis source priority (higher = takes precedence)
|
||||
COST_BASIS_SOURCE_PRIORITY = {
|
||||
nil => 0,
|
||||
"provider" => 1,
|
||||
"calculated" => 2,
|
||||
"manual" => 3
|
||||
}.freeze
|
||||
|
||||
COST_BASIS_SOURCES = %w[manual calculated provider].freeze
|
||||
|
||||
belongs_to :account
|
||||
belongs_to :security
|
||||
belongs_to :account_provider, optional: true
|
||||
@@ -10,9 +20,12 @@ class Holding < ApplicationRecord
|
||||
validates :qty, :currency, :date, :price, :amount, presence: true
|
||||
validates :qty, :price, :amount, numericality: { greater_than_or_equal_to: 0 }
|
||||
validates :external_id, uniqueness: { scope: :account_id }, allow_blank: true
|
||||
validates :cost_basis_source, inclusion: { in: COST_BASIS_SOURCES }, allow_nil: true
|
||||
|
||||
scope :chronological, -> { order(:date) }
|
||||
scope :for, ->(security) { where(security_id: security).order(:date) }
|
||||
scope :with_locked_cost_basis, -> { where(cost_basis_locked: true) }
|
||||
scope :with_unlocked_cost_basis, -> { where(cost_basis_locked: false) }
|
||||
|
||||
delegate :ticker, to: :security
|
||||
|
||||
@@ -76,6 +89,53 @@ class Holding < ApplicationRecord
|
||||
account.sync_later
|
||||
end
|
||||
|
||||
# Returns the priority level for the current source (higher = better)
|
||||
def cost_basis_source_priority
|
||||
COST_BASIS_SOURCE_PRIORITY[cost_basis_source] || 0
|
||||
end
|
||||
|
||||
# Check if this holding's cost_basis can be overwritten by the given source
|
||||
def cost_basis_replaceable_by?(new_source)
|
||||
return false if cost_basis_locked?
|
||||
|
||||
new_priority = COST_BASIS_SOURCE_PRIORITY[new_source] || 0
|
||||
|
||||
# Special case: when user unlocks a manual cost_basis, they're opting into
|
||||
# recalculation. Allow only "calculated" source to replace it (from trades).
|
||||
# This is the whole point of the unlock action.
|
||||
if cost_basis_source == "manual"
|
||||
return new_source == "calculated"
|
||||
end
|
||||
|
||||
new_priority > cost_basis_source_priority
|
||||
end
|
||||
|
||||
# Set cost_basis from user input (locks the value)
|
||||
def set_manual_cost_basis!(value)
|
||||
update!(
|
||||
cost_basis: value,
|
||||
cost_basis_source: "manual",
|
||||
cost_basis_locked: true
|
||||
)
|
||||
end
|
||||
|
||||
# Unlock cost_basis to allow provider/calculated updates
|
||||
def unlock_cost_basis!
|
||||
update!(cost_basis_locked: false)
|
||||
end
|
||||
|
||||
# Check if cost_basis is known (has a source and positive value)
|
||||
def cost_basis_known?
|
||||
cost_basis.present? && cost_basis.positive? && cost_basis_source.present?
|
||||
end
|
||||
|
||||
# Human-readable source label for UI display
|
||||
def cost_basis_source_label
|
||||
return nil unless cost_basis_source.present?
|
||||
|
||||
I18n.t("holdings.cost_basis_sources.#{cost_basis_source}")
|
||||
end
|
||||
|
||||
private
|
||||
def calculate_trend
|
||||
return nil unless amount_money
|
||||
|
||||
58
app/models/holding/cost_basis_reconciler.rb
Normal file
58
app/models/holding/cost_basis_reconciler.rb
Normal file
@@ -0,0 +1,58 @@
|
||||
# Determines the appropriate cost_basis value and source when updating a holding.
|
||||
#
|
||||
# Used by both Materializer (for trade-derived calculations) and
|
||||
# ProviderImportAdapter (for provider-supplied values) to ensure consistent
|
||||
# reconciliation logic across all data sources.
|
||||
#
|
||||
# Priority hierarchy: manual > calculated > provider > unknown
|
||||
#
|
||||
class Holding::CostBasisReconciler
|
||||
# Determines the appropriate cost_basis value and source for a holding update
|
||||
#
|
||||
# @param existing_holding [Holding, nil] The existing holding record (nil for new)
|
||||
# @param incoming_cost_basis [BigDecimal, nil] The incoming cost_basis value
|
||||
# @param incoming_source [String] The source of incoming data ('calculated', 'provider')
|
||||
# @return [Hash] { cost_basis: value, cost_basis_source: source, should_update: boolean }
|
||||
def self.reconcile(existing_holding:, incoming_cost_basis:, incoming_source:)
|
||||
# Treat zero cost_basis from provider as unknown
|
||||
if incoming_source == "provider" && (incoming_cost_basis.nil? || incoming_cost_basis.zero?)
|
||||
incoming_cost_basis = nil
|
||||
end
|
||||
|
||||
# New holding - use whatever we have
|
||||
if existing_holding.nil?
|
||||
return {
|
||||
cost_basis: incoming_cost_basis,
|
||||
cost_basis_source: incoming_cost_basis.present? ? incoming_source : nil,
|
||||
should_update: true
|
||||
}
|
||||
end
|
||||
|
||||
# Locked - never overwrite
|
||||
if existing_holding.cost_basis_locked?
|
||||
return {
|
||||
cost_basis: existing_holding.cost_basis,
|
||||
cost_basis_source: existing_holding.cost_basis_source,
|
||||
should_update: false
|
||||
}
|
||||
end
|
||||
|
||||
# Check priority - can the incoming source replace the existing?
|
||||
if existing_holding.cost_basis_replaceable_by?(incoming_source)
|
||||
if incoming_cost_basis.present?
|
||||
return {
|
||||
cost_basis: incoming_cost_basis,
|
||||
cost_basis_source: incoming_source,
|
||||
should_update: true
|
||||
}
|
||||
end
|
||||
end
|
||||
|
||||
# Keep existing (equal or lower priority, or incoming is nil)
|
||||
{
|
||||
cost_basis: existing_holding.cost_basis,
|
||||
cost_basis_source: existing_holding.cost_basis_source,
|
||||
should_update: false
|
||||
}
|
||||
end
|
||||
end
|
||||
@@ -31,36 +31,73 @@ class Holding::Materializer
|
||||
|
||||
current_time = Time.now
|
||||
|
||||
# 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? }
|
||||
# Load existing holdings to check locked status and source priority
|
||||
existing_holdings_map = load_existing_holdings_map
|
||||
|
||||
# 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?
|
||||
# Separate holdings into categories based on cost_basis reconciliation
|
||||
holdings_to_upsert_with_cost = []
|
||||
holdings_to_upsert_without_cost = []
|
||||
|
||||
@holdings.each do |holding|
|
||||
key = holding_key(holding)
|
||||
existing = existing_holdings_map[key]
|
||||
|
||||
reconciled = Holding::CostBasisReconciler.reconcile(
|
||||
existing_holding: existing,
|
||||
incoming_cost_basis: holding.cost_basis,
|
||||
incoming_source: "calculated"
|
||||
)
|
||||
|
||||
base_attrs = holding.attributes
|
||||
.slice("date", "currency", "qty", "price", "amount", "security_id")
|
||||
.merge("account_id" => account.id, "updated_at" => current_time)
|
||||
|
||||
if existing&.cost_basis_locked?
|
||||
# For locked holdings, preserve ALL cost_basis fields
|
||||
holdings_to_upsert_without_cost << base_attrs
|
||||
elsif reconciled[:should_update] && reconciled[:cost_basis].present?
|
||||
# Update with new cost_basis and source
|
||||
holdings_to_upsert_with_cost << base_attrs.merge(
|
||||
"cost_basis" => reconciled[:cost_basis],
|
||||
"cost_basis_source" => reconciled[:cost_basis_source]
|
||||
)
|
||||
else
|
||||
# No cost_basis to set, or existing is better - don't touch cost_basis fields
|
||||
holdings_to_upsert_without_cost << base_attrs
|
||||
end
|
||||
end
|
||||
|
||||
# Upsert with cost_basis updates
|
||||
if holdings_to_upsert_with_cost.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)
|
||||
},
|
||||
holdings_to_upsert_with_cost,
|
||||
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?
|
||||
# Upsert without cost_basis (preserves existing)
|
||||
if holdings_to_upsert_without_cost.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)
|
||||
},
|
||||
holdings_to_upsert_without_cost,
|
||||
unique_by: %i[account_id security_id date currency]
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
def load_existing_holdings_map
|
||||
# Load holdings that might affect reconciliation:
|
||||
# - Locked holdings (must preserve their cost_basis)
|
||||
# - Holdings with a source (need to check priority)
|
||||
account.holdings
|
||||
.where(cost_basis_locked: true)
|
||||
.or(account.holdings.where.not(cost_basis_source: nil))
|
||||
.index_by { |h| holding_key(h) }
|
||||
end
|
||||
|
||||
def holding_key(holding)
|
||||
[ holding.account_id || account.id, holding.security_id, holding.date, holding.currency ]
|
||||
end
|
||||
|
||||
def purge_stale_holdings
|
||||
portfolio_security_ids = account.entries.trades.map { |entry| entry.entryable.security_id }.uniq
|
||||
|
||||
|
||||
Reference in New Issue
Block a user