diff --git a/app/controllers/holdings_controller.rb b/app/controllers/holdings_controller.rb index 96ce2cd49..cd6d6260c 100644 --- a/app/controllers/holdings_controller.rb +++ b/app/controllers/holdings_controller.rb @@ -1,5 +1,5 @@ class HoldingsController < ApplicationController - before_action :set_holding, only: %i[show destroy] + before_action :set_holding, only: %i[show update destroy unlock_cost_basis] def index @account = Current.family.accounts.find(params[:account_id]) @@ -8,6 +8,31 @@ class HoldingsController < ApplicationController def show end + def update + total_cost_basis = holding_params[:cost_basis].to_d + + if total_cost_basis >= 0 && @holding.qty.positive? + # Convert total cost basis to per-share cost (the cost_basis field stores per-share) + # Zero is valid for gifted/inherited shares + per_share_cost = total_cost_basis / @holding.qty + @holding.set_manual_cost_basis!(per_share_cost) + flash[:notice] = t(".success") + else + flash[:alert] = t(".error") + end + + # Redirect to account page holdings tab to refresh list and close drawer + redirect_to account_path(@holding.account, tab: "holdings") + end + + def unlock_cost_basis + @holding.unlock_cost_basis! + flash[:notice] = t(".success") + + # Redirect to account page holdings tab to refresh list and close drawer + redirect_to account_path(@holding.account, tab: "holdings") + end + def destroy if @holding.account.can_delete_holdings? @holding.destroy_holding_and_entries! @@ -26,4 +51,8 @@ class HoldingsController < ApplicationController def set_holding @holding = Current.family.holdings.find(params[:id]) end + + def holding_params + params.require(:holding).permit(:cost_basis) + end end diff --git a/app/javascript/controllers/cost_basis_form_controller.js b/app/javascript/controllers/cost_basis_form_controller.js new file mode 100644 index 000000000..467cd5a9a --- /dev/null +++ b/app/javascript/controllers/cost_basis_form_controller.js @@ -0,0 +1,30 @@ +import { Controller } from "@hotwired/stimulus" + +// Handles bidirectional conversion between total cost basis and per-share cost +// in the manual cost basis entry form. +export default class extends Controller { + static targets = ["total", "perShare", "perShareValue"] + static values = { qty: Number } + + // Called when user types in the total cost basis field + // Updates the per-share display and input to show the calculated value + updatePerShare() { + const total = Number.parseFloat(this.totalTarget.value) || 0 + const qty = this.qtyValue || 1 + const perShare = qty > 0 ? (total / qty).toFixed(2) : "0.00" + this.perShareValueTarget.textContent = perShare + if (this.hasPerShareTarget) { + this.perShareTarget.value = perShare + } + } + + // Called when user types in the per-share field + // Updates the total cost basis field with the calculated value + updateTotal() { + const perShare = Number.parseFloat(this.perShareTarget.value) || 0 + const qty = this.qtyValue || 1 + const total = (perShare * qty).toFixed(2) + this.totalTarget.value = total + this.perShareValueTarget.textContent = perShare.toFixed(2) + } +} diff --git a/app/javascript/controllers/drawer_cost_basis_controller.js b/app/javascript/controllers/drawer_cost_basis_controller.js new file mode 100644 index 000000000..6091bc9cd --- /dev/null +++ b/app/javascript/controllers/drawer_cost_basis_controller.js @@ -0,0 +1,33 @@ +import { Controller } from "@hotwired/stimulus" + +// Handles the inline cost basis editor in the holding drawer. +// Shows/hides the form and handles bidirectional total <-> per-share conversion. +export default class extends Controller { + static targets = ["form", "total", "perShare", "perShareValue"] + static values = { qty: Number } + + toggle(event) { + event.preventDefault() + this.formTarget.classList.toggle("hidden") + } + + // Called when user types in total cost basis field + updatePerShare() { + const total = Number.parseFloat(this.totalTarget.value) || 0 + const qty = this.qtyValue || 1 + const perShare = qty > 0 ? (total / qty).toFixed(2) : "0.00" + this.perShareValueTarget.textContent = perShare + if (this.hasPerShareTarget) { + this.perShareTarget.value = perShare + } + } + + // Called when user types in per-share field + updateTotal() { + const perShare = Number.parseFloat(this.perShareTarget.value) || 0 + const qty = this.qtyValue || 1 + const total = (perShare * qty).toFixed(2) + this.totalTarget.value = total + this.perShareValueTarget.textContent = perShare.toFixed(2) + } +} diff --git a/app/models/account/provider_import_adapter.rb b/app/models/account/provider_import_adapter.rb index d247e09e0..e74d1f5e6 100644 --- a/app/models/account/provider_import_adapter.rb +++ b/app/models/account/provider_import_adapter.rb @@ -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 diff --git a/app/models/holding.rb b/app/models/holding.rb index c2297db50..fd7fe4284 100644 --- a/app/models/holding.rb +++ b/app/models/holding.rb @@ -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 diff --git a/app/models/holding/cost_basis_reconciler.rb b/app/models/holding/cost_basis_reconciler.rb new file mode 100644 index 000000000..88bd9b635 --- /dev/null +++ b/app/models/holding/cost_basis_reconciler.rb @@ -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 diff --git a/app/models/holding/materializer.rb b/app/models/holding/materializer.rb index da2950fa3..3fb704ca7 100644 --- a/app/models/holding/materializer.rb +++ b/app/models/holding/materializer.rb @@ -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 diff --git a/app/views/holdings/_cost_basis_cell.html.erb b/app/views/holdings/_cost_basis_cell.html.erb new file mode 100644 index 000000000..24401b8bb --- /dev/null +++ b/app/views/holdings/_cost_basis_cell.html.erb @@ -0,0 +1,109 @@ +<%# locals: (holding:, editable: true) %> + +<% + # Pre-calculate values for the form + # Note: cost_basis field stores per-share cost, so calculate total for display + current_per_share = holding.cost_basis.present? && holding.cost_basis.positive? ? holding.cost_basis : nil + current_total = current_per_share && holding.qty.positive? ? (current_per_share * holding.qty).round(2) : nil + currency = Money::Currency.new(holding.currency) +%> + +<%= turbo_frame_tag dom_id(holding, :cost_basis) do %> + <% if holding.cost_basis_locked? && !editable %> + <%# Locked and not editable (from holdings list) - just show value, right-aligned %> +
+ <%= tag.span format_money(holding.avg_cost) %> + <%= icon "lock", size: "xs", class: "text-secondary" %> +
+ <% else %> + <%# Unlocked OR editable context (drawer) - show clickable menu %> + <%= render DS::Menu.new(variant: :button, placement: "bottom-end") do |menu| %> + <% menu.with_button(class: "hover:text-primary cursor-pointer group") do %> + <% if holding.avg_cost %> +
+ <%= tag.span format_money(holding.avg_cost) %> + <% if holding.cost_basis_locked? %> + <%= icon "lock", size: "xs", class: "text-secondary" %> + <% end %> + <%= icon "pencil", size: "xs", class: "text-secondary opacity-0 group-hover:opacity-100 transition-opacity" %> +
+ <% else %> +
+ <%= icon "pencil", size: "xs" %> + Set +
+ <% end %> + <% end %> + <% menu.with_custom_content do %> +
+

+ <%= t(".set_cost_basis_header", ticker: holding.ticker, qty: number_with_precision(holding.qty, precision: 2)) %> +

+ <% + form_data = { turbo: false } + if holding.avg_cost + form_data[:turbo_confirm] = { + title: t(".overwrite_confirm_title"), + body: t(".overwrite_confirm_body", current: format_money(holding.avg_cost)) + } + end + %> + <%= styled_form_with model: holding, + url: holding_path(holding), + method: :patch, + class: "space-y-3", + data: form_data do |f| %> + +
+
+ +
+ <%= currency.symbol %> + + <%= currency.iso_code %> +
+
+
+

+ = <%= currency.symbol %><%= number_with_precision(current_per_share, precision: 2) || "0.00" %> <%= t(".per_share") %> +

+ + +
+ +
+ <%= currency.symbol %> + + <%= currency.iso_code %> +
+
+ +
+ + <%= f.submit t(".save"), class: "inline-flex items-center gap-1 px-2 py-1 rounded-md text-sm font-medium text-inverse bg-inverse hover:bg-inverse-hover" %> +
+ <% end %> +
+ <% end %> + <% end %> + <% end %> +<% end %> diff --git a/app/views/holdings/_holding.html.erb b/app/views/holdings/_holding.html.erb index 45ff4bf81..85d9474ac 100644 --- a/app/views/holdings/_holding.html.erb +++ b/app/views/holdings/_holding.html.erb @@ -31,7 +31,7 @@
- <%= tag.p holding.avg_cost ? format_money(holding.avg_cost) : t(".unknown"), class: holding.avg_cost ? nil : "text-secondary" %> + <%= render "holdings/cost_basis_cell", holding: holding, editable: false %> <%= tag.p t(".per_share"), class: "font-normal text-secondary" %>
@@ -45,13 +45,13 @@
- <%# Show Total Return (unrealized G/L) when cost basis exists %> - <% if holding.trades.any? && holding.trend %> + <%# Show Total Return (unrealized G/L) when cost basis exists (from trades or manual) %> + <% if holding.trend %> <%= tag.p format_money(holding.trend.value), style: "color: #{holding.trend.color};" %> <%= tag.p "(#{number_to_percentage(holding.trend.percent, precision: 1)})", style: "color: #{holding.trend.color};" %> <% else %> <%= tag.p "--", class: "text-secondary" %> - <%= tag.p "No cost basis", class: "text-xs text-secondary" %> + <%= tag.p t(".no_cost_basis"), class: "text-xs text-secondary" %> <% end %>
diff --git a/app/views/holdings/show.html.erb b/app/views/holdings/show.html.erb index 32918b49e..868b77f1d 100644 --- a/app/views/holdings/show.html.erb +++ b/app/views/holdings/show.html.erb @@ -35,16 +35,107 @@
<%= @holding.weight ? number_to_percentage(@holding.weight, precision: 2) : t(".unknown") %>
-
-
<%= t(".avg_cost_label") %>
-
<%= @holding.avg_cost ? format_money(@holding.avg_cost) : t(".unknown") %>
+ <%# Average Cost with inline editor %> + <% + currency = Money::Currency.new(@holding.currency) + current_per_share = @holding.cost_basis.present? && @holding.cost_basis.positive? ? @holding.cost_basis : nil + current_total = current_per_share && @holding.qty.positive? ? (current_per_share * @holding.qty).round(2) : nil + %> +
+
+
<%= t(".avg_cost_label") %>
+
+ <%= @holding.avg_cost ? format_money(@holding.avg_cost) : t(".unknown") %> + <% if @holding.cost_basis_locked? %> + <%= icon "lock", size: "xs", class: "text-secondary" %> + <% end %> + <% if @holding.cost_basis_source.present? %> + (<%= @holding.cost_basis_source_label %>) + <% end %> + +
+
+ + <%# Inline cost basis editor (hidden by default) %> +
<%= t(".total_return_label") %>
-
- <%= @holding.trend ? render("shared/trend_change", trend: @holding.trend) : t(".unknown") %> -
+ <% if @holding.trend %> +
+ <%= render("shared/trend_change", trend: @holding.trend) %> +
+ <% else %> +
<%= t(".unknown") %>
+ <% end %>
@@ -85,21 +176,39 @@ <% end %> - <% if @holding.account.can_delete_holdings? %> + <% if @holding.cost_basis_locked? || @holding.account.can_delete_holdings? %> <% dialog.with_section(title: t(".settings"), open: true) do %>
-
-
-

<%= t(".delete_title") %>

-

<%= t(".delete_subtitle") %>

-
+ <% if @holding.cost_basis_locked? %> +
+
+

<%= t(".cost_basis_locked_label") %>

+

<%= t(".cost_basis_locked_description") %>

+
- <%= button_to t(".delete"), - holding_path(@holding), - method: :delete, - class: "rounded-lg px-3 py-2 text-red-500 text-sm font-medium border border-secondary", - data: { turbo_confirm: true } %> -
+ <%= button_to t(".unlock_cost_basis"), + unlock_cost_basis_holding_path(@holding), + method: :post, + class: "inline-flex items-center gap-1 px-3 py-2 rounded-lg text-sm font-medium text-primary bg-gray-200 hover:bg-gray-300 theme-dark:bg-gray-700 theme-dark:hover:bg-gray-600", + form: { data: { turbo: false } }, + data: { turbo_confirm: { title: t(".unlock_confirm_title"), body: t(".unlock_confirm_body") } } %> +
+ <% end %> + + <% if @holding.account.can_delete_holdings? %> +
+
+

<%= t(".delete_title") %>

+

<%= t(".delete_subtitle") %>

+
+ + <%= button_to t(".delete"), + holding_path(@holding), + method: :delete, + class: "rounded-lg px-3 py-2 text-red-500 text-sm font-medium border border-secondary", + data: { turbo_confirm: true } %> +
+ <% end %>
<% end %> <% end %> diff --git a/app/views/reports/_investment_performance.html.erb b/app/views/reports/_investment_performance.html.erb index f88fa9fd2..c727c819a 100644 --- a/app/views/reports/_investment_performance.html.erb +++ b/app/views/reports/_investment_performance.html.erb @@ -119,7 +119,7 @@
<% investment_metrics[:accounts].each do |account| %> -
+ <%= link_to account_path(account), class: "bg-container-inset rounded-lg p-4 flex items-center justify-between hover:bg-container-hover transition-colors" do %>
<%= render "accounts/logo", account: account, size: "sm" %>
@@ -128,7 +128,7 @@

<%= format_money(account.balance_money) %>

-
+ <% end %> <% end %>
diff --git a/config/locales/views/holdings/en.yml b/config/locales/views/holdings/en.yml index 10b648965..0e6d588d4 100644 --- a/config/locales/views/holdings/en.yml +++ b/config/locales/views/holdings/en.yml @@ -5,10 +5,30 @@ en: brokerage_cash: Brokerage cash destroy: success: Holding deleted + update: + success: Cost basis saved. + error: Invalid cost basis value. + unlock_cost_basis: + success: Cost basis unlocked. It may be updated on next sync. + cost_basis_sources: + manual: User set + calculated: From trades + provider: From provider + cost_basis_cell: + unknown: "--" + set_cost_basis_header: "Set cost basis for %{ticker} (%{qty} shares)" + total_cost_basis_label: Total cost basis + or_per_share_label: "Or enter per share:" + per_share: per share + cancel: Cancel + save: Save + overwrite_confirm_title: Overwrite cost basis? + overwrite_confirm_body: "This will replace the current cost basis of %{current}." holding: per_share: per share shares: "%{qty} shares" unknown: "--" + no_cost_basis: No cost basis index: average_cost: Average cost holdings: Holdings @@ -36,3 +56,8 @@ en: trade_history_entry: "%{qty} shares of %{security} at %{price}" total_return_label: Total Return unknown: Unknown + cost_basis_locked_label: Cost basis is locked + cost_basis_locked_description: Your manually set cost basis won't be changed by syncs. + unlock_cost_basis: Unlock + unlock_confirm_title: Unlock cost basis? + unlock_confirm_body: This will allow the cost basis to be updated by provider syncs or trade calculations. diff --git a/config/routes.rb b/config/routes.rb index f6f6fdd66..ced0f676a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -168,7 +168,11 @@ Rails.application.routes.draw do resources :mappings, only: :update, module: :import end - resources :holdings, only: %i[index new show destroy] + resources :holdings, only: %i[index new show update destroy] do + member do + post :unlock_cost_basis + end + end resources :trades, only: %i[show new create update destroy] resources :valuations, only: %i[show new create update destroy] do post :confirm_create, on: :collection diff --git a/db/migrate/20260112011546_add_cost_basis_source_tracking_to_holdings.rb b/db/migrate/20260112011546_add_cost_basis_source_tracking_to_holdings.rb new file mode 100644 index 000000000..f805dfbe4 --- /dev/null +++ b/db/migrate/20260112011546_add_cost_basis_source_tracking_to_holdings.rb @@ -0,0 +1,6 @@ +class AddCostBasisSourceTrackingToHoldings < ActiveRecord::Migration[7.2] + def change + add_column :holdings, :cost_basis_source, :string + add_column :holdings, :cost_basis_locked, :boolean, default: false, null: false + end +end diff --git a/db/migrate/20260112065106_backfill_cost_basis_source_for_holdings.rb b/db/migrate/20260112065106_backfill_cost_basis_source_for_holdings.rb new file mode 100644 index 000000000..b81f92156 --- /dev/null +++ b/db/migrate/20260112065106_backfill_cost_basis_source_for_holdings.rb @@ -0,0 +1,42 @@ +class BackfillCostBasisSourceForHoldings < ActiveRecord::Migration[7.2] + disable_ddl_transaction! + + def up + # Backfill cost_basis_source for existing holdings that have cost_basis but no source + # This is safe - it only adds metadata, doesn't change actual cost_basis values + # Locks existing data by default to protect it - users can unlock if they want syncs to update + + say_with_time "Backfilling cost_basis_source for holdings" do + updated = 0 + + # Process in batches to avoid locking issues + Holding.where.not(cost_basis: nil) + .where(cost_basis_source: nil) + .where("cost_basis > 0") + .find_each do |holding| + # Heuristic: If holding's account has buy trades for this security, likely calculated + # Otherwise, likely from provider (SimpleFIN/Plaid/Lunchflow) + has_trades = holding.account.trades + .where(security_id: holding.security_id) + .where("qty > 0") + .exists? + + source = has_trades ? "calculated" : "provider" + + # Lock existing data to protect it - users can unlock via UI if they want syncs to update + holding.update_columns(cost_basis_source: source, cost_basis_locked: true) + updated += 1 + end + + updated + end + end + + def down + # Reversible: clear the source and unlock for holdings that were backfilled + # We can't know for sure which ones were backfilled vs manually set, + # but clearing all non-manual sources is safe since they'd be re-detected + Holding.where(cost_basis_source: %w[calculated provider]) + .update_all(cost_basis_source: nil, cost_basis_locked: false) + end +end diff --git a/db/schema.rb b/db/schema.rb index 6465a2c16..067d2c9f6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_01_10_122603) do +ActiveRecord::Schema[7.2].define(version: 2026_01_12_065106) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -49,6 +49,8 @@ ActiveRecord::Schema[7.2].define(version: 2026_01_10_122603) do t.string "institution_name" t.string "institution_domain" t.text "notes" + t.jsonb "holdings_snapshot_data" + t.datetime "holdings_snapshot_at" t.index ["accountable_id", "accountable_type"], name: "index_accounts_on_accountable_id_and_accountable_type" t.index ["accountable_type"], name: "index_accounts_on_accountable_type" t.index ["currency"], name: "index_accounts_on_currency" @@ -340,12 +342,14 @@ ActiveRecord::Schema[7.2].define(version: 2026_01_10_122603) do t.jsonb "locked_attributes", default: {} t.string "external_id" t.string "source" + t.boolean "exclude_from_cashflow", default: false, null: false t.index "lower((name)::text)", name: "index_entries_on_lower_name" t.index ["account_id", "date"], name: "index_entries_on_account_id_and_date" t.index ["account_id", "source", "external_id"], name: "index_entries_on_account_source_and_external_id", unique: true, where: "((external_id IS NOT NULL) AND (source IS NOT NULL))" t.index ["account_id"], name: "index_entries_on_account_id" t.index ["date"], name: "index_entries_on_date" t.index ["entryable_type"], name: "index_entries_on_entryable_type" + t.index ["exclude_from_cashflow"], name: "index_entries_on_exclude_from_cashflow" t.index ["import_id"], name: "index_entries_on_import_id" end @@ -485,6 +489,8 @@ ActiveRecord::Schema[7.2].define(version: 2026_01_10_122603) do t.string "external_id" t.decimal "cost_basis", precision: 19, scale: 4 t.uuid "account_provider_id" + t.string "cost_basis_source" + t.boolean "cost_basis_locked", default: false, null: false t.index ["account_id", "external_id"], name: "idx_holdings_on_account_id_external_id_unique", unique: true, where: "(external_id IS NOT NULL)" t.index ["account_id", "security_id", "date", "currency"], name: "idx_on_account_id_security_id_date_currency_5323e39f8b", unique: true t.index ["account_id"], name: "index_holdings_on_account_id" @@ -1125,7 +1131,14 @@ ActiveRecord::Schema[7.2].define(version: 2026_01_10_122603) do t.string "currency" t.jsonb "locked_attributes", default: {} t.uuid "category_id" + t.decimal "realized_gain", precision: 19, scale: 4 + t.decimal "cost_basis_amount", precision: 19, scale: 4 + t.string "cost_basis_currency" + t.integer "holding_period_days" + t.string "realized_gain_confidence" + t.string "realized_gain_currency" t.index ["category_id"], name: "index_trades_on_category_id" + t.index ["realized_gain"], name: "index_trades_on_realized_gain_not_null", where: "(realized_gain IS NOT NULL)" t.index ["security_id"], name: "index_trades_on_security_id" end @@ -1138,9 +1151,11 @@ ActiveRecord::Schema[7.2].define(version: 2026_01_10_122603) do t.string "kind", default: "standard", null: false t.string "external_id" t.jsonb "extra", default: {}, null: false + t.string "investment_activity_label" t.index ["category_id"], name: "index_transactions_on_category_id" t.index ["external_id"], name: "index_transactions_on_external_id" t.index ["extra"], name: "index_transactions_on_extra", using: :gin + t.index ["investment_activity_label"], name: "index_transactions_on_investment_activity_label" t.index ["kind"], name: "index_transactions_on_kind" t.index ["merchant_id"], name: "index_transactions_on_merchant_id" end diff --git a/test/controllers/holdings_controller_test.rb b/test/controllers/holdings_controller_test.rb index 2e03b9f46..a73680d54 100644 --- a/test/controllers/holdings_controller_test.rb +++ b/test/controllers/holdings_controller_test.rb @@ -27,4 +27,38 @@ class HoldingsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to account_path(@holding.account) assert_empty @holding.account.entries.where(entryable: @holding.account.trades.where(security: @holding.security)) end + + test "updates cost basis with total amount divided by qty" do + # Given: holding with 10 shares + @holding.update!(qty: 10, cost_basis: nil, cost_basis_source: nil, cost_basis_locked: false) + + # When: user submits total cost basis of $100 (should become $10 per share) + patch holding_path(@holding), params: { holding: { cost_basis: "100.00" } } + + # Redirects to account page holdings tab to refresh list + assert_redirected_to account_path(@holding.account, tab: "holdings") + @holding.reload + + # Then: cost_basis should be per-share ($10), not total + assert_equal 10.0, @holding.cost_basis.to_f + assert_equal "manual", @holding.cost_basis_source + assert @holding.cost_basis_locked? + end + + test "unlock_cost_basis removes lock" do + # Given: locked holding + @holding.update!(cost_basis: 50.0, cost_basis_source: "manual", cost_basis_locked: true) + + # When: user unlocks + post unlock_cost_basis_holding_path(@holding) + + # Redirects to account page holdings tab to refresh list + assert_redirected_to account_path(@holding.account, tab: "holdings") + @holding.reload + + # Then: lock is removed but cost_basis and source remain + assert_not @holding.cost_basis_locked? + assert_equal 50.0, @holding.cost_basis.to_f + assert_equal "manual", @holding.cost_basis_source + end end diff --git a/test/models/holding/cost_basis_reconciler_test.rb b/test/models/holding/cost_basis_reconciler_test.rb new file mode 100644 index 000000000..c8413b0c1 --- /dev/null +++ b/test/models/holding/cost_basis_reconciler_test.rb @@ -0,0 +1,171 @@ +require "test_helper" + +class Holding::CostBasisReconcilerTest < ActiveSupport::TestCase + setup do + @family = families(:empty) + @account = @family.accounts.create!( + name: "Test Investment", + balance: 20000, + currency: "USD", + accountable: Investment.new + ) + @security = securities(:aapl) + end + + test "new holding uses incoming cost_basis" do + result = Holding::CostBasisReconciler.reconcile( + existing_holding: nil, + incoming_cost_basis: BigDecimal("150"), + incoming_source: "provider" + ) + + assert result[:should_update] + assert_equal BigDecimal("150"), result[:cost_basis] + assert_equal "provider", result[:cost_basis_source] + end + + test "new holding with nil cost_basis gets nil source" do + result = Holding::CostBasisReconciler.reconcile( + existing_holding: nil, + incoming_cost_basis: nil, + incoming_source: "provider" + ) + + assert result[:should_update] + assert_nil result[:cost_basis] + assert_nil result[:cost_basis_source] + end + + test "locked holding is never overwritten" do + holding = @account.holdings.create!( + security: @security, + date: Date.current, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + cost_basis: BigDecimal("175"), + cost_basis_source: "manual", + cost_basis_locked: true + ) + + result = Holding::CostBasisReconciler.reconcile( + existing_holding: holding, + incoming_cost_basis: BigDecimal("200"), + incoming_source: "calculated" + ) + + assert_not result[:should_update] + assert_equal BigDecimal("175"), result[:cost_basis] + assert_equal "manual", result[:cost_basis_source] + end + + test "calculated overwrites provider" do + holding = @account.holdings.create!( + security: @security, + date: Date.current, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + cost_basis: BigDecimal("150"), + cost_basis_source: "provider", + cost_basis_locked: false + ) + + result = Holding::CostBasisReconciler.reconcile( + existing_holding: holding, + incoming_cost_basis: BigDecimal("175"), + incoming_source: "calculated" + ) + + assert result[:should_update] + assert_equal BigDecimal("175"), result[:cost_basis] + assert_equal "calculated", result[:cost_basis_source] + end + + test "provider does not overwrite calculated" do + holding = @account.holdings.create!( + security: @security, + date: Date.current, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + cost_basis: BigDecimal("175"), + cost_basis_source: "calculated", + cost_basis_locked: false + ) + + result = Holding::CostBasisReconciler.reconcile( + existing_holding: holding, + incoming_cost_basis: BigDecimal("150"), + incoming_source: "provider" + ) + + assert_not result[:should_update] + assert_equal BigDecimal("175"), result[:cost_basis] + assert_equal "calculated", result[:cost_basis_source] + end + + test "provider does not overwrite manual" do + holding = @account.holdings.create!( + security: @security, + date: Date.current, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + cost_basis: BigDecimal("175"), + cost_basis_source: "manual", + cost_basis_locked: false + ) + + result = Holding::CostBasisReconciler.reconcile( + existing_holding: holding, + incoming_cost_basis: BigDecimal("150"), + incoming_source: "provider" + ) + + assert_not result[:should_update] + assert_equal BigDecimal("175"), result[:cost_basis] + assert_equal "manual", result[:cost_basis_source] + end + + test "zero provider cost_basis treated as unknown" do + result = Holding::CostBasisReconciler.reconcile( + existing_holding: nil, + incoming_cost_basis: BigDecimal("0"), + incoming_source: "provider" + ) + + assert result[:should_update] + assert_nil result[:cost_basis] + assert_nil result[:cost_basis_source] + end + + test "nil incoming cost_basis does not overwrite existing" do + holding = @account.holdings.create!( + security: @security, + date: Date.current, + qty: 10, + price: 200, + amount: 2000, + currency: "USD", + cost_basis: BigDecimal("175"), + cost_basis_source: "provider", + cost_basis_locked: false + ) + + result = Holding::CostBasisReconciler.reconcile( + existing_holding: holding, + incoming_cost_basis: nil, + incoming_source: "calculated" + ) + + # Even though calculated > provider, nil incoming shouldn't overwrite existing value + assert_not result[:should_update] + assert_equal BigDecimal("175"), result[:cost_basis] + assert_equal "provider", result[:cost_basis_source] + end +end diff --git a/test/models/holding_test.rb b/test/models/holding_test.rb index a3f4e2039..7e49fe6ba 100644 --- a/test/models/holding_test.rb +++ b/test/models/holding_test.rb @@ -112,6 +112,132 @@ class HoldingTest < ActiveSupport::TestCase 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" do + # Provider data can be replaced by calculated or manual + @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_not @amzn.cost_basis_replaceable_by?("provider") + + # Calculated data can be replaced by manual only + @amzn.update!(cost_basis: 200, cost_basis_source: "calculated", cost_basis_locked: false) + assert @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 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 + private def load_holdings