QT/sure
1
0
Fork 0
mirror of https://github.com/we-promise/sure.git synced 2026-04-19 20:14:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

Add security remapping for holdings with sync protection (#692)

Browse Source 
* Add security remapping support to holdings

- Introduced `provider_security` tracking for holdings with schema updates.
- Implemented security remap/reset workflows in `Holding` model and UI.
- Updated routes, controllers, and tests to support new functionality.
- Enhanced client-side interaction with Stimulus controller for remapping.

# Conflicts:
#	app/components/UI/account/activity_feed.html.erb
#	db/schema.rb

* Refactor "New transaction" to "New activity" across UI and tests

- Updated localized strings, button labels, and ARIA attributes.
- Improved error handling in holdings' current price display.
- Scoped fallback queries in `provider_import_adapter` to prevent overwrites.
- Added safeguard for offline securities in price fetching logic.

* Update security remapping to merge holdings on collision by deleting duplicates

- Removed error handling for collisions in `remap_security!`.
- Added logic to merge holdings by deleting duplicates on conflicting dates.
- Modified associated test to validate merging behavior.

* Update security remapping to merge holdings on collision by combining qty and amount

- Modified `remap_security!` to merge holdings by summing `qty` and `amount` on conflicting dates.
- Adjusted logic to calculate `price` for merged holdings.
- Updated test to validate new merge behavior.

* Improve DOM handling in Turbo redirect action & enhance holdings merge logic

- Updated Turbo's custom `redirect` action to use the "replace" option for cleaner DOM updates without clearing the cache.
- Enhanced holdings merge logic to calculate weighted average cost basis during security remapping, ensuring more accurate cost_basis updates.

* Track provider_security_id during security updates to support reset workflows

* Fix provider tracking: guard nil ticker lookups and preserve merge attrs

- Guard fallback 1b lookup when security.ticker is blank to avoid matching NULL tickers
- Preserve external_id, provider_security_id, account_provider_id during collision merge

* Fix schema.rb version after merge (includes tax_treatment migration)

* fix: Rename migration to run after schema version

The migration 20260117000001 was skipped in CI because it had a timestamp
earlier than the schema version (2026_01_17_200000). CI loads schema.rb
directly and only runs migrations with versions after the schema version.

Renamed to 20260119000001 so it runs correctly.

* Update schema: remove Coinbase tables, add new fields and indexes

* Update schema: add back `tax_treatment` field with default value "taxable"

* Improve Turbo redirect action: use "replace" to avoid form submission in history

* Lock merged holdings to prevent provider overwrites and fix activity feed template indentation

* Refactor holdings transfer logic: enforce currency checks during collisions and enhance merge handling

---------

Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: luckyPipewrench <luckypipewrench@proton.me>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
This commit is contained in:
LPW
2026-01-23 06:54:55 -05:00
committed by GitHub
parent e0fb585bda
commit c504ba9b99
17 changed files with 522 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
app/models/account/provider_import_adapter.rb
View File

@@ -323,20 +323,46 @@ class Account::ProviderImportAdapter
holding = account.holdings.find_by(external_id: external_id)
unless holding
# Fallback path: match by (security, date, currency) — and when provided,
# also scope by account_provider_id to avoid cross‑provider claiming.
# This keeps behavior symmetric with deletion logic below which filters
# by account_provider_id when present.
find_by_attrs = {
security: security,
# Fallback path 1a: match by provider_security (for remapped holdings)
# This allows re-matching a holding that was remapped to a different security
# Scope by account_provider_id to avoid cross-provider overwrites
fallback_1a_attrs = {
provider_security: security,
date: date,
currency: currency
}
if account_provider_id.present?
find_by_attrs[:account_provider_id] = account_provider_id
fallback_1a_attrs[:account_provider_id] = account_provider_id if account_provider_id.present?
holding = account.holdings.find_by(fallback_1a_attrs)
# Fallback path 1b: match by provider_security ticker (for remapped holdings when
# Security::Resolver returns a different security instance for the same ticker)
# Scope by account_provider_id to avoid cross-provider overwrites
# Skip if ticker is blank to avoid matching NULL tickers
unless holding || security.ticker.blank?
scope = account.holdings
.joins("INNER JOIN securities AS ps ON ps.id = holdings.provider_security_id")
.where(date: date, currency: currency)
.where("ps.ticker = ?", security.ticker)
scope = scope.where(account_provider_id: account_provider_id) if account_provider_id.present?
holding = scope.first
end
holding = account.holdings.find_by(find_by_attrs)
# Fallback path 2: match by (security, date, currency) — and when provided,
# also scope by account_provider_id to avoid cross‑provider claiming.
# This keeps behavior symmetric with deletion logic below which filters
# by account_provider_id when present.
unless holding
find_by_attrs = {
security: security,
date: date,
currency: currency
}
if account_provider_id.present?
find_by_attrs[:account_provider_id] = account_provider_id
end
holding = account.holdings.find_by(find_by_attrs)
end
end
holding ||= account.holdings.new(
@@ -382,7 +408,6 @@ class Account::ProviderImportAdapter
# Build base attributes
attributes = {
security: security,
date: date,
currency: currency,
qty: quantity,
@@ -392,6 +417,14 @@ class Account::ProviderImportAdapter
external_id: external_id
}
# Only update security if not locked by user
if holding.new_record? || holding.security_replaceable_by_provider?
attributes[:security] = security
# Track the provider's original security so reset_security_to_provider! works
# Only set if not already set (preserves original if user remapped then unlocked)
attributes[:provider_security_id] = security.id if holding.provider_security_id.blank?
end
# Only update cost_basis if reconciliation says to
if reconciled[:should_update]
attributes[:cost_basis] = reconciled[:cost_basis]

113
app/models/holding.rb
View File

@@ -16,6 +16,7 @@ class Holding < ApplicationRecord
belongs_to :account
belongs_to :security
belongs_to :account_provider, optional: true
belongs_to :provider_security, class_name: "Security", optional: true
validates :qty, :currency, :date, :price, :amount, presence: true
validates :qty, :price, :amount, numericality: { greater_than_or_equal_to: 0 }
@@ -128,6 +129,118 @@ class Holding < ApplicationRecord
update!(cost_basis_locked: false)
end
# Check if this holding's security can be changed by provider sync
def security_replaceable_by_provider?
!security_locked?
end
# Check if user has remapped this holding to a different security
# Also verifies the provider_security record still exists (FK should prevent deletion, but be safe)
def security_remapped?
provider_security_id.present? && security_id != provider_security_id && provider_security.present?
end
# Remap this holding (and all other holdings for the same security) to a different security
# Also moves all trades for the old security to the new security
# If the target security already has holdings on some dates, merge by combining qty/amount
def remap_security!(new_security)
return if new_security.id == security_id
old_security = security
transaction do
# Find (date, currency) pairs where the new security already has holdings (collision keys)
# Currency must match to merge - can't combine holdings denominated in different currencies
collision_keys = account.holdings
.where(security: new_security)
.where(date: account.holdings.where(security: old_security).select(:date))
.pluck(:date, :currency)
.to_set
# Process each holding for the old security
account.holdings.where(security: old_security).find_each do |holding|
if collision_keys.include?([ holding.date, holding.currency ])
# Collision: merge into existing holding for new_security (same date AND currency)
existing = account.holdings.find_by!(security: new_security, date: holding.date, currency: holding.currency)
merged_qty = existing.qty + holding.qty
merged_amount = existing.amount + holding.amount
# Calculate weighted average cost basis if both holdings have cost_basis
merged_cost_basis = if existing.cost_basis.present? && holding.cost_basis.present? && merged_qty.positive?
((existing.cost_basis * existing.qty) + (holding.cost_basis * holding.qty)) / merged_qty
else
existing.cost_basis # Keep existing if we can't calculate weighted average
end
# Preserve provider tracking from the holding being destroyed
# so subsequent syncs can find the merged holding
merge_attrs = {
qty: merged_qty,
amount: merged_amount,
price: merged_qty.positive? ? merged_amount / merged_qty : 0,
cost_basis: merged_cost_basis
}
merge_attrs[:external_id] ||= holding.external_id if existing.external_id.blank? && holding.external_id.present?
merge_attrs[:provider_security_id] ||= holding.provider_security_id || old_security.id if existing.provider_security_id.blank?
merge_attrs[:account_provider_id] ||= holding.account_provider_id if existing.account_provider_id.blank? && holding.account_provider_id.present?
merge_attrs[:security_locked] = true # Lock merged holding to prevent provider overwrites
existing.update!(merge_attrs)
holding.destroy!
else
# No collision: update to new security
holding.provider_security_id ||= old_security.id
holding.security = new_security
holding.security_locked = true
holding.save!
end
end
# Move all trades for old security to new security
account.trades.where(security: old_security).update_all(security_id: new_security.id)
end
# Reload self to reflect changes (may raise RecordNotFound if self was destroyed)
begin
reload
rescue ActiveRecord::RecordNotFound
nil
end
end
# Reset security (and all related holdings) back to what the provider originally sent
# Note: This moves ALL trades for current_security back to original_security. If the user
# had legitimate trades for the target security before remapping, those would also be moved.
# In practice this is rare since SimpleFIN doesn't provide trades, and Plaid trades would
# typically be for real tickers not CUSTOM: ones. A more robust solution would track which
# trades were moved during remap, but that adds significant complexity for an edge case.
def reset_security_to_provider!
return unless provider_security_id.present?
current_security = security
original_security = provider_security
# Guard against deleted provider_security (shouldn't happen due to FK, but be safe)
return unless original_security.present?
transaction do
# Move trades back (see note above about limitation)
account.trades.where(security: current_security).update_all(security_id: original_security.id)
# Reset ALL holdings that were remapped from this provider_security
account.holdings.where(security: current_security, provider_security: original_security).find_each do |holding|
holding.update!(
security: original_security,
security_locked: false,
provider_security_id: nil
)
end
end
# Reload self to reflect changes
reload
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?

3
app/models/security/provided.rb
View File

@@ -55,6 +55,9 @@ module Security::Provided
return price if price.present?
# Don't fetch prices for offline securities (e.g., custom tickers from SimpleFIN)
return nil if offline?
# Make sure we have a data provider before fetching
return nil unless provider.present?
response = provider.fetch_security_price(

4
app/models/security/resolver.rb
View File

@@ -63,7 +63,7 @@ class Security::Resolver
exchange_matches = s.exchange_operating_mic.upcase.to_s == exchange_operating_mic.upcase.to_s
if country_code && exchange_operating_mic
ticker_matches && exchange_matches && s.country_code.upcase.to_s == country_code.upcase.to_s
ticker_matches && exchange_matches && s.country_code&.upcase.to_s == country_code.upcase.to_s
else
ticker_matches && exchange_matches
end
@@ -79,7 +79,7 @@ class Security::Resolver
# If a country code is specified, we MUST find a match with the same code
if country_code.present?
filtered_candidates = filtered_candidates.select { |s| s.country_code.upcase.to_s == country_code.upcase.to_s }
filtered_candidates = filtered_candidates.select { |s| s.country_code&.upcase.to_s == country_code.upcase.to_s }
end
# 1. Prefer exact ticker matches (MSTR before MSTRX when searching for "MSTR")