Add security remapping for holdings with sync protection #692
+522
−53
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds provider-backed security remapping: DB migration and holding fields, Holding model remap/reset/lock logic, controller routes/actions and Stimulus UI, import-adapter/resolver adjustments, trade movement/merge on remap, Turbo redirect tweak, locales, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Controller as HoldingsController
participant Model as Holding
participant Trade as Trade
participant DB as Database
User->>Controller: PATCH /holdings/:id/remap_security (security_id)
Controller->>Model: remap_security!(new_security)
Model->>DB: load holdings for old security (account scope)
Model->>Model: detect collisions by date
alt Collision exists
Model->>DB: fetch target holding (new security) and merge qty/amount
Model->>DB: update weighted cost_basis and attributes
Model->>DB: destroy old holding row
else No collision
Model->>DB: update holding.security_id -> new_security.id
Model->>Model: set provider_security_id, security_locked = true
Model->>DB: save holding
end
Model->>Trade: move trades from old security to new_security
Model->>DB: reload affected records
Controller->>User: redirect (success)
sequenceDiagram
participant User as User
participant Controller as HoldingsController
participant Model as Holding
participant Trade as Trade
participant DB as Database
User->>Controller: POST /holdings/:id/reset_security
Controller->>Model: reset_security_to_provider!
alt provider_security_id present
Model->>Trade: move trades back to provider_security
Model->>DB: update trades and holdings remapped from provider_security
Model->>Model: restore original security_id, clear provider_security_id, security_locked = false
Model->>DB: save updates
else no provider_security_id
Model->>Model: no-op
end
Controller->>User: redirect (success)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/models/account/provider_import_adapter.rb`:
- Around line 320-353: Fallback paths 1a
(account.holdings.find_by(provider_security: security, date: date, currency:
currency)) and 1b (the .joins("INNER JOIN securities AS ps
...").where(...).where("ps.ticker = ?", security.ticker).first) do not scope by
account_provider_id and can match holdings from another provider; update both
lookups to include account_provider_id when account_provider_id.present? (same
pattern used in the fallback 2 find_by_attrs and the deletion logic) so they
only match holdings with the same account_provider_id, preserving provider
scoping.
In `@app/views/holdings/show.html.erb`:
- Around line 34-38: The icon-only edit toggle button
(data-action="click->holding-security-remap#toggle") lacks an accessible label;
update the button to include a localized accessible label by adding an
i18n-backed aria-label (or an sr-only span with t(".edit")/t("holdings.edit")
key) so screen readers get meaningful text, ensuring you use the translation
helper (t) for the label and add the corresponding entry to your locale file.
🧹 Nitpick comments (1)
app/controllers/holdings_controller.rb (1)
50-73: Trim ticker/exchange tokens before resolving.Defensive trimming avoids resolver misses if the combobox returns whitespace or extra separators.
♻️ Proposed refactor
- ticker, exchange = params[:security_id].to_s.split("|") + ticker, exchange = params[:security_id].to_s.split("|", 2).map(&:strip)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/models/holding.rb`:
- Around line 186-188: The current reset_security_to_provider! reassigns all
trades with security == current_security, which can move unrelated legitimate
trades; modify the flow to only touch trades that were remapped by introducing a
remapped_from_security_id (or similar) on Trade, set that field when performing
the remap (e.g., in your remap method) and then change the reset logic (the
transaction that calls account.trades.where(security:
current_security).update_all(...)) to filter by remapped_from_security_id (and
security: current_security) so only remapped trades are moved back, and clear
the remapped_from_security_id after the move. Ensure the new attribute is
persisted on Trade, the remap path sets it, and the reset_security_to_provider!
uses that attribute and clears it inside the same transaction.
- Around line 172-173: Replace the bare rescue around reload with an explicit
exception handler: call reload as before but rescue only
ActiveRecord::RecordNotFound so that unexpected errors are not swallowed; update
the code around the reload call in app/models/holding.rb (the place where reload
is invoked on self) to catch ActiveRecord::RecordNotFound and return/ignore nil,
leaving other exceptions to propagate.
- Around line 149-165: The current remap logic (involving collision_dates,
account.holdings and the loop that may call holding.destroy!) unconditionally
deletes the old holding on collision; change it to first find the corresponding
target holding (account.holdings.where(security: new_security, date:
holding.date).first) and validate equivalence of key fields (qty/amount/price)
before destroy: if identical, destroy the old record; if different, either merge
values explicitly (e.g., combine qty/amount and reconcile price) or raise/record
a validation error so manual review occurs; also ensure provider_security_id,
security_locked and save! logic runs only after successful merge/validation and
update tests for remap_security! to assert merge/validation behavior rather than
just count changes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
added 5 commits
January 18, 2026 09:54
- 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
- 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.
…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.
… 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.
…e 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.
luckyPipewrench
force-pushed
the
enhanced-holdings
branch
from
ecad534 to
4a36bd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/models/account/provider_import_adapter.rb`:
- Around line 413-416: The conditional that sets attributes[:security] when
holding.new_record? || holding.security_replaceable_by_provider? must also set
attributes[:provider_security_id] for new imported holdings so
reset_security_to_provider! can work; update the block around the security
assignment to add attributes[:provider_security_id] = security.id when
holding.new_record? (follow the same remapping pattern used elsewhere to track
the provider's original security), ensuring provider_security_id is populated
for new records while preserving the existing branch for
security_replaceable_by_provider?.
🧹 Nitpick comments (3)
test/models/holding_test.rb (1)
326-357: Collision merge test could verify cost_basis calculation.The test validates qty and amount merging but doesn't verify the weighted average cost_basis calculation. Consider adding assertions for cost_basis when both holdings have values.
♻️ Suggested enhancement for cost_basis verification
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" + currency: "USD", + cost_basis: 90.00, + cost_basis_source: "provider" ) + `@amzn.update`!(cost_basis: 200.00, cost_basis_source: "provider") + 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) # ... existing assertions ... + # Verify weighted average cost basis: (90*5 + 200*15) / 20 = 172.50 + expected_cost_basis = (90.00 * 5 + 200.00 * amzn_qty) / (5 + amzn_qty) + assert_in_delta expected_cost_basis, existing_goog.cost_basis.to_f, 0.01 endapp/models/holding.rb (1)
197-228: Reset implementation with documented trade limitation.The detailed comment (lines 197-202) clearly explains the edge case where legitimate trades could be incorrectly moved. This is a reasonable trade-off given:
- SimpleFIN doesn't provide trades
- Plaid trades typically use real tickers, not
CUSTOM:prefixed onesThe filter on line 217 correctly scopes to holdings remapped from this specific provider security.
One minor consistency note:
reloadon line 227 doesn't have explicit exception handling likeremap_security!does. WhileRecordNotFoundshouldn't occur during reset (unlike remap where the record may be destroyed via merge), adding the same pattern would be more defensive.app/views/holdings/show.html.erb (1)
54-56: Use design‑system tokens instead of raw gray utilities.
Lines 55–56 and Line 238 usebg-gray-*/hover:bg-gray-*. Please swap these for functional tokens frommaybe-design-system.cssto keep theming consistent. As per coding guidelines, prefer design‑system tokens over raw Tailwind colors.Also applies to: 235-239
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/models/account/provider_import_adapter.rb`:
- Around line 331-340: The fallback "1b" lookup that builds scope using
account.holdings and joins on provider_security_id should be skipped when
security.ticker is nil/blank to avoid matching NULL tickers; update the logic
around the unless holding block so the join/where on "ps.ticker = ?" (the
scope/query that assigns holding) only runs when security.ticker.present? (i.e.,
guard the scope creation/where call with a check on security.ticker) and
preserve the existing account_provider_id scoping and assignment to holding.
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Collaborator
|
I tried to fix a |
- 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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/models/holding.rb`:
- Around line 169-182: The merge path currently builds merge_attrs and updates
the existing holding without ensuring it is locked; add security_locked: true to
merge_attrs before calling existing.update! so that the merged holding is marked
locked (matching the non-collision path) and thus prevents provider syncs from
overwriting the user remap; update the code that constructs merge_attrs in the
collision branch (the hash used by existing.update!) to include security_locked:
true.
🧹 Nitpick comments (1)
app/models/holding.rb (1)
177-179: Redundant||=on freshly initialized hash keys.
merge_attrsis created on lines 171-176, somerge_attrs[:external_id]et al. are alwaysnil. The||=operator is superfluous here—direct assignment is clearer.♻️ Suggested simplification
- 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[: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?
added 2 commits
January 19, 2026 11:01
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.
Collaborator
Author
|
Its good now @jjmata |
jjmata
reviewed
Jan 19, 2026
added 5 commits
January 19, 2026 17:27
Resolves schema.rb conflict by keeping upstream's version number while preserving this branch's holdings table changes (provider_security_id, security_locked columns).
…feed template indentation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/models/holding.rb`:
- Around line 151-186: Collision detection and the subsequent lookup use only
date and security, which can merge holdings across different currencies; update
the collision calculation (collision_dates) and the find_by! call used to fetch
existing (existing = account.holdings.find_by!(...)) to also filter by currency
(e.g., include currency or currency_id field) so only holdings with matching
currency are considered, and skip or raise if currencies differ before computing
merged_qty/merged_amount/cost_basis; ensure any other attribute merges
(external_id, provider_security_id, account_provider_id) respect the same
currency check.
♻️ Duplicate comments (1)
app/models/holding.rb (1)
209-226: Reset still reassigns unrelated trades.The reset path still moves all trades for
current_security, which can reassign legitimate, unrelated trades. Consider tracking remapped trades and only reverting those.
🧹 Nitpick comments (1)
app/views/holdings/show.html.erb (1)
54-59: Use design tokens for the new remap action buttons.These new buttons use raw gray utilities (
bg-gray-200,hover:bg-gray-300). Consider swapping to design-system tokens to keep themes consistent. As per coding guidelines, please prefer functional tokens from the design system.
…isions and enhance merge handling
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
security_lockedflagTechnical Changes
Database:
provider_security_idto track original provider securitysecurity_lockedboolean to protect user overridesModel (
Holding):remap_security!updates ALL holdings for that security across all dates, moves trades, sets lockreset_security_to_provider!reverts all related holdings back to originalSync Protection (
ProviderImportAdapter):security_lockedflag during syncTesting
Summary by CodeRabbit
New Features
Bug Fixes
Localization
✏️ Tip: You can customize this high-level summary in your review settings.