Bond Core: add account and lot support with manual inflation flow#1532
Draft
UberDudePL wants to merge 15 commits intowe-promise:mainfrom
Draft
Bond Core: add account and lot support with manual inflation flow#1532UberDudePL wants to merge 15 commits intowe-promise:mainfrom
UberDudePL wants to merge 15 commits intowe-promise:mainfrom
Conversation
brin-security-scanner
Bot
added
pr:verified
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Bond and BondLot domain models, DB schema and migrations, controllers and views for bond account and lot management, Stimulus front-end controllers, balance integration, a scheduled settlement job, and extensive tests and i18n for bond-related UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant BondLotsCtrl as BondLotsController
participant BondLotModel as BondLot
participant EntryModel as Entry/Transaction
participant Account
participant SyncJob
User->>Browser: Submit bond lot create form
Browser->>BondLotsCtrl: POST /bond_lots (params)
BondLotsCtrl->>Account: require_account_permission!(account_id)
BondLotsCtrl->>BondLotModel: build(params)
BondLotModel->>EntryModel: create_purchase_entry! (entry + transaction.extra with bond_lot_id)
EntryModel-->>BondLotModel: associate entry
BondLotModel-->>BondLotsCtrl: saved
BondLotsCtrl->>Account: sync_later(purchased_on)
Account->>SyncJob: enqueue(sync date)
BondLotsCtrl-->>Browser: redirect with notice
Browser-->>User: show success
sequenceDiagram
participant Scheduler
participant Job as SettleMaturedBondLotsJob
participant BondLotModel as BondLot
participant EntryModel as Entry/Transaction
participant Account
Scheduler->>Job: perform(on:)
loop each auto_close_on_maturity lot with maturity_date <= on
Job->>BondLotModel: settle_if_matured!(on:)
BondLotModel->>EntryModel: create_settlement_entry!
EntryModel->>Account: persist settlement transaction
BondLotModel->>BondLotModel: set closed_on, update fields
Job->>Account: sync_later(original_purchased_on)
end
Job-->>Scheduler: complete (or re-raise first error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Introduces a new Bond accountable type with BondLot lifecycle support (purchase/edit/delete, maturity settlement with tax handling, and manual inflation-linked valuation), integrating bond positions into balance calculations and account UI flows.
Changes:
- Add
Bond/BondLotmodels + settlement job and supporting database schema/migrations. - Implement bond lot CRUD + drawer UI, plus account creation/edit flow for bond accounts.
- Extend balance/ledger logic and tests/fixtures to treat bond lot purchases as holdings flows.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/support/ledger_testing_helper.rb | Allow transaction kind/extra metadata in ledger fixtures (bond lot tagging). |
| test/models/bond_test.rb | Unit tests for bond balance/maturity/subtype normalization. |
| test/models/bond_lot_test.rb | Extensive unit tests for bond lot valuation, defaults, settlement, reinvestment, inflation flow. |
| test/models/balance/reverse_calculator_test.rb | Ensures bond lot purchases are treated as holdings flows (not market change). |
| test/jobs/settle_matured_bond_lots_job_test.rb | Job test for auto-settling matured lots. |
| test/fixtures/bonds.yml | Bond fixture(s) for tests. |
| test/fixtures/bond_lots.yml | Bond lot fixtures for tests. |
| test/fixtures/accounts.yml | Add a bond account fixture. |
| test/controllers/transactions/bulk_deletions_controller_test.rb | Ensures bulk deletion removes linked bond lots. |
| test/controllers/bonds_controller_test.rb | Controller tests for creating/updating bond accounts. |
| test/controllers/bond_lots_controller_test.rb | Controller tests for bond lot CRUD and validations. |
| test/controllers/accounts_controller_test.rb | Coverage for bond account tabs/actions with matured lots present. |
| db/schema.rb | Schema snapshot updated for bond tables + related changes. |
| db/migrate/20260407183000_add_bond_lot_enum_check_constraints.rb | Adds/ensures enum-like check constraints on bond_lots. |
| db/migrate/20260407170000_fix_bond_lots_non_inflation_check_constraint.rb | No-op migration documenting constraint state. |
| db/migrate/20260407162000_add_holdings_snapshot_columns_to_accounts.rb | Add holdings snapshot columns to accounts. |
| db/migrate/20260407151000_change_bond_lots_entry_fk_to_cascade.rb | Change bond_lots→entries FK to cascade delete. |
| db/migrate/20260331120000_create_bond_lots.rb | Create bond_lots table with indexes/constraints. |
| db/migrate/20260330233052_create_bonds.rb | Create bonds table. |
| config/routes.rb | Add routes for bonds and bond_lots resources. |
| config/locales/views/bonds/pl.yml | Polish UI strings for bond/bond lot flows and inflation copy. |
| config/locales/views/bonds/en.yml | English UI strings for bond/bond lot flows and inflation copy. |
| config/locales/views/accounts/en.yml | Add “Bond” account type label. |
| app/views/bonds/new.html.erb | Bond account “new” dialog + method select path. |
| app/views/bonds/edit.html.erb | Bond account “edit” dialog. |
| app/views/bonds/_form.html.erb | Bond account form fields (initial balance, wrapper, auto-buy). |
| app/views/bond_lots/show.html.erb | Drawer show/edit + history + delete for bond lots. |
| app/views/bond_lots/new.html.erb | New bond lot dialog. |
| app/views/bond_lots/edit.html.erb | Edit bond lot dialog. |
| app/views/bond_lots/_form.html.erb | Bond lot form with product presets + inflation-linked fields. |
| app/views/accounts/show/_activity.html.erb | Activity menu updated to include “New activity” for bonds. |
| app/views/accounts/new.html.erb | Add bond to account type selector. |
| app/models/entry.rb | Associate entries to bond lots (dependent destroy). |
| app/models/enable_banking_item.rb | Guard psu_type validation when column absent. |
| app/models/concerns/accountable.rb | Register Bond as an Accountable type. |
| app/models/bond_lot.rb | Core bond lot domain logic: defaults, valuation, settlement, rate review. |
| app/models/bond.rb | Bond accountable model: holdings/original balance, wrapper logic, product presets. |
| app/models/balance/sync_cache.rb | Preserve preloaded entryable on duplicated entries (avoid extra loads). |
| app/models/balance/base_calculator.rb | Bond holdings value/flows integration for balance calculations. |
| app/models/account.rb | Add bond wrapper label helper + treat Bond as investment-like balance type. |
| app/jobs/settle_matured_bond_lots_job.rb | Scheduled job to auto-settle matured bond lots. |
| app/javascript/controllers/bond_lot_inflation_controller.js | Stimulus toggling for inflation-linked form fields. |
| app/javascript/controllers/bond_lot_form_controller.js | Stimulus syncing subtype/term from product presets. |
| app/javascript/controllers/bond_account_form_controller.js | Stimulus toggling for wrapper-driven bond account fields. |
| app/controllers/bonds_controller.rb | AccountableResource-based controller for Bond accounts. |
| app/controllers/bond_lots_controller.rb | CRUD controller for bond lots with permission checks and sync triggers. |
| app/components/UI/account/activity_feed.html.erb | Add “New activity” action entry point for bond accounts. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b4e99086d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
app/views/bonds/edit.html.erb (1)
2-2: Minor: i18n key convention inconsistency.Other new dialog views in this PR (
bond_lots/new.html.erb,bond_lots/edit.html.erb) uset(".title", ...), but this file usest(".edit", ...). Consider standardizing to.titlefor both the new/edit dialog headers to keep locale keys predictable.✏️ Suggested change
- <% dialog.with_header(title: t(".edit", account: `@account.name`)) %> + <% dialog.with_header(title: t(".title", account: `@account.name`)) %>Update
config/locales/*.ymlaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/bonds/edit.html.erb` at line 2, The dialog header in edit view uses t(".edit", account: `@account.name`) which is inconsistent with other dialog views using t(".title"); update the call in the view to use t(".title", account: `@account.name`) (i.e., change dialog.with_header(title: t(".edit", ...)) to dialog.with_header(title: t(".title", ...))) and then update the corresponding locale entries (the relevant YAML keys currently providing ".edit") to provide a ".title" key for both the bond edit and bond_lot new/edit locales so all dialog headers use the same i18n key.app/javascript/controllers/bond_lot_form_controller.js (1)
35-47: Prefer targets over manual input queries here.These selectors depend on exact Rails input names. Since the controller would still stay under 7 targets, consider making
purchasedOnInput,issueDateInput, andtermInputStimulus targets.As per coding guidelines, “Use Stimulus targets to reference DOM elements, not manual getElementById calls.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/bond_lot_form_controller.js` around lines 35 - 47, The methods syncIssueDateWithPurchase and `#syncTermWithProduct` currently query inputs via querySelector (purchasedOnInput, issueDateInput, termInput); convert these to Stimulus targets by declaring targets (e.g., purchasedOn, issueDate, term) on the controller and replace the manual queries with the corresponding target references (this.purchasedOnTarget, this.issueDateTarget, this.termTarget) or this.targets.find(...) so the controller uses Stimulus targets instead of hard-coded Rails input name selectors; update the controller’s static targets array and all usages in syncIssueDateWithPurchase and `#syncTermWithProduct` accordingly.app/views/bond_lots/show.html.erb (1)
85-89: Use a design-system text token for the destructive action.
text-red-500bypasses the functional tokens used elsewhere in this view; prefer the existing destructive/danger token if available.As per coding guidelines, “Use functional tokens from design system (e.g.,
text-primary,bg-container) instead of raw Tailwind classes (e.g.,text-white,bg-gray-200).”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/bond_lots/show.html.erb` around lines 85 - 89, The delete button currently uses a raw Tailwind class ("text-red-500") which bypasses the design-system functional tokens; update the class on the button_to for t(".delete") (the element calling bond_lot_path(`@bond_lot`) with method: :delete) to use the project’s destructive/danger token instead of "text-red-500" (for example replace with the design-system token such as "text-destructive" or "text-danger" used elsewhere), keeping the other classes intact and preserving the data: { turbo_confirm: t(".delete_confirm") } behavior.app/javascript/controllers/bond_lot_inflation_controller.js (1)
61-70: Use Stimulus targets instead ofquerySelectorfor form field access.Lines 61–70 manually query form controls by name attribute, which couples the controller to the form structure. Add targets for
subtype,purchasedOnInput, andissueDateInputin the controller, declare them in the view withdata-bond-lot-inflation-targetattributes, and read fromthis.subtypeInputTarget,this.purchasedOnInputTarget, andthis.issueDateInputTargetinstead. This follows the Stimulus convention and makes the controller resilient to form changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/bond_lot_inflation_controller.js` around lines 61 - 70, Replace direct DOM queries inside the private methods `#subtypeValue` and `#firstPeriodRateRequired` with Stimulus targets: declare targets = ["subtypeInput","purchasedOnInput","issueDateInput"] on the controller, update the view to add data-bond-lot-inflation-target="subtypeInput|purchasedOnInput|issueDateInput" on the corresponding form fields, and read values from this.subtypeInputTarget, this.purchasedOnInputTarget, and this.issueDateInputTarget (and their .value) instead of using querySelector and optional chaining; ensure `#subtypeValue` returns the stringified value from this.subtypeInputTarget and `#firstPeriodRateRequired` parses dates from the purchasedOn and issueDate targets via this.#parseDate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/UI/account/activity_feed.html.erb`:
- Around line 29-35: The code uses a hardcoded i18n fallback in the
menu.with_item call (t("accounts.show.activity.new_activity", default: "New
activity")); remove the default argument so the view calls
t("accounts.show.activity.new_activity") only, and add/verify the key
accounts.show.activity.new_activity is present in your locale files (e.g.,
config/locales/*.yml) to prevent missing-translation fallbacks; update the
locale files where necessary and ensure the menu.with_item invocation still
passes variant/icon/href/data unchanged.
In `@app/controllers/bond_lots_controller.rb`:
- Around line 24-27: The show action is incorrectly invoking
require_account_permission! which enforces default write permission and blocks
read-only users; remove the call from show so it relies on the scoped lookup
(set_bond_lot) or replace it with a read-level check if needed. Locate the show
method in BondLotsController (method name: show) and either delete the
require_account_permission!(`@account`) line or change it to a read-specific
helper (e.g., require_account_read_permission!(`@account`)); keep the existing
require_account_permission! check on edit, update, and destroy actions
unchanged.
In `@app/models/enable_banking_item.rb`:
- Around line 56-59: The write path for psu_type in start_authorization is not
guarded like the read path, so when validated_psu_type is present but the
psu_type column is missing update!(attributes) can raise
ActiveRecord::UnknownAttributeError; modify start_authorization to only set
attributes[:psu_type] (or include psu_type in the attributes hash passed to
update!) when the model actually has the psu_type attribute (use
has_attribute?(:psu_type) or similar) and validated_psu_type is present,
ensuring you check both validated_psu_type and has_attribute?(:psu_type) before
assigning to attributes[:psu_type] and calling update!.
In `@app/models/entry.rb`:
- Line 14: The Entry model's has_one :bond_lot allows entry.destroy! to
cascade-delete a BondLot and its dependents, risking silent data loss; add a
before_destroy guard on Entry (e.g., before_destroy
:prevent_destroy_if_bond_lot_present) that checks for bond_lot and raises or
halts destruction, and update EntryableResource#destroy and any API/UI flows to
either surface that validation error or route removals through
BondLotsController#destroy which calls destroy_with_purchase_entry! for safe,
ordered deletion; ensure the guard references the bond_lot association and that
tests asserting successful deletion are adjusted to use the bonded-lot-specific
destroy path.
In `@app/views/accounts/show/_activity.html.erb`:
- Around line 19-32: Remove the hardcoded default fallback from the translation
calls so translations always resolve: in the account activity partial, update
both menu.with_item calls that use t(".new_activity", default: "New activity")
(the branches guarded by account.investment? which builds the link with
new_trade_path(account_id: account.id) and account.bond? which builds the link
with new_bond_lot_path(account_id: account.id)) to call t(".new_activity") with
no default option.
In `@app/views/bond_lots/_form.html.erb`:
- Around line 21-29: Add the inflation controller action to both date inputs so
the bond-lot-inflation controller recalculates requirement state whenever dates
change: for the purchased_on field append a second action (e.g.
change->bond-lot-inflation#recalculate) alongside
change->bond-lot-form#syncIssueDateWithPurchase, and for the issue_date
date_field add the same change->bond-lot-inflation#recalculate action; keep the
existing data-bond-lot-inflation-target attributes and ensure the action name
matches the method on the bond-lot-inflation controller.
In `@app/views/bond_lots/show.html.erb`:
- Around line 57-65: The view precomputes inflation_text and margin_text
unconditionally which raises when event[:inflation_source] == "first_period" and
component percents are missing; move the computations so they only run in the
branch that renders the manual inflation copy (i.e., compute inflation_text and
margin_text inside the else for event[:inflation_source] != "first_period"),
leaving the first_period branch to render the fixed-rate translation without
touching event[:inflation_component_percent] or
event[:margin_component_percent].
In `@db/migrate/20260330233052_create_bonds.rb`:
- Around line 7-13: Add PostgreSQL CHECK constraints in the create_bonds
migration to enforce the Bond model's allowed values for rate_type,
coupon_frequency, subtype, and tax_wrapper (mirror the model constants/enums
exactly) and ensure nullability rules match the model; update the migration to
add CHECKs for each column (e.g., CHECK (rate_type IN (...))), include the
existing tax_wrapper constraint (instead of only indexing it), and ensure
defaults/null settings remain consistent with the Bond model so invalid rows
cannot bypass ActiveRecord validations.
In `@db/migrate/20260331120000_create_bond_lots.rb`:
- Around line 20-29: Add DB CHECK constraints to enforce non-negative
money/quantity fields and a valid tax percentage: for the bond_lots migration
add check constraints on early_redemption_fee >= 0, settlement_amount >= 0,
tax_withheld >= 0, units >= 0 (or > 0 if business requires), nominal_per_unit >=
0, and tax_rate BETWEEN 0 AND 100; implement these using add_check_constraint on
the bond_lots table (referencing the column names early_redemption_fee,
settlement_amount, tax_withheld, units, nominal_per_unit, tax_rate) in the same
migration (and mirror the same constraints in the later block noted at lines
49-68) so the schema enforces these invariants at the DB level.
In `@db/schema.rb`:
- Around line 269-283: Add DB-level check constraints to the bonds table to
restrict tax_wrapper, subtype, rate_type, and coupon_frequency to the allowed
enum/range values (matching bond_lots expectations) by creating a new migration
that alters the bonds table and adds explicit CHECK constraints for each column
(or a single combined CHECK) using add_check_constraint or raw SQL in the
migration's change method; ensure the constraint names are descriptive (e.g.,
check_bonds_tax_wrapper) and run the migration to regenerate db/schema.rb so the
schema reflects the new constraints.
---
Nitpick comments:
In `@app/javascript/controllers/bond_lot_form_controller.js`:
- Around line 35-47: The methods syncIssueDateWithPurchase and
`#syncTermWithProduct` currently query inputs via querySelector (purchasedOnInput,
issueDateInput, termInput); convert these to Stimulus targets by declaring
targets (e.g., purchasedOn, issueDate, term) on the controller and replace the
manual queries with the corresponding target references (this.purchasedOnTarget,
this.issueDateTarget, this.termTarget) or this.targets.find(...) so the
controller uses Stimulus targets instead of hard-coded Rails input name
selectors; update the controller’s static targets array and all usages in
syncIssueDateWithPurchase and `#syncTermWithProduct` accordingly.
In `@app/javascript/controllers/bond_lot_inflation_controller.js`:
- Around line 61-70: Replace direct DOM queries inside the private methods
`#subtypeValue` and `#firstPeriodRateRequired` with Stimulus targets: declare
targets = ["subtypeInput","purchasedOnInput","issueDateInput"] on the
controller, update the view to add
data-bond-lot-inflation-target="subtypeInput|purchasedOnInput|issueDateInput" on
the corresponding form fields, and read values from this.subtypeInputTarget,
this.purchasedOnInputTarget, and this.issueDateInputTarget (and their .value)
instead of using querySelector and optional chaining; ensure `#subtypeValue`
returns the stringified value from this.subtypeInputTarget and
`#firstPeriodRateRequired` parses dates from the purchasedOn and issueDate targets
via this.#parseDate.
In `@app/views/bond_lots/show.html.erb`:
- Around line 85-89: The delete button currently uses a raw Tailwind class
("text-red-500") which bypasses the design-system functional tokens; update the
class on the button_to for t(".delete") (the element calling
bond_lot_path(`@bond_lot`) with method: :delete) to use the project’s
destructive/danger token instead of "text-red-500" (for example replace with the
design-system token such as "text-destructive" or "text-danger" used elsewhere),
keeping the other classes intact and preserving the data: { turbo_confirm:
t(".delete_confirm") } behavior.
In `@app/views/bonds/edit.html.erb`:
- Line 2: The dialog header in edit view uses t(".edit", account: `@account.name`)
which is inconsistent with other dialog views using t(".title"); update the call
in the view to use t(".title", account: `@account.name`) (i.e., change
dialog.with_header(title: t(".edit", ...)) to dialog.with_header(title:
t(".title", ...))) and then update the corresponding locale entries (the
relevant YAML keys currently providing ".edit") to provide a ".title" key for
both the bond edit and bond_lot new/edit locales so all dialog headers use the
same i18n key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97bcb6a1-b628-4b8a-a861-d6cf30d29387
📒 Files selected for processing (47)
app/components/UI/account/activity_feed.html.erbapp/controllers/bond_lots_controller.rbapp/controllers/bonds_controller.rbapp/javascript/controllers/bond_account_form_controller.jsapp/javascript/controllers/bond_lot_form_controller.jsapp/javascript/controllers/bond_lot_inflation_controller.jsapp/jobs/settle_matured_bond_lots_job.rbapp/models/account.rbapp/models/balance/base_calculator.rbapp/models/balance/sync_cache.rbapp/models/bond.rbapp/models/bond_lot.rbapp/models/concerns/accountable.rbapp/models/enable_banking_item.rbapp/models/entry.rbapp/views/accounts/new.html.erbapp/views/accounts/show/_activity.html.erbapp/views/bond_lots/_form.html.erbapp/views/bond_lots/edit.html.erbapp/views/bond_lots/new.html.erbapp/views/bond_lots/show.html.erbapp/views/bonds/_form.html.erbapp/views/bonds/edit.html.erbapp/views/bonds/new.html.erbconfig/locales/views/accounts/en.ymlconfig/locales/views/bonds/en.ymlconfig/locales/views/bonds/pl.ymlconfig/routes.rbdb/migrate/20260330233052_create_bonds.rbdb/migrate/20260331120000_create_bond_lots.rbdb/migrate/20260407151000_change_bond_lots_entry_fk_to_cascade.rbdb/migrate/20260407162000_add_holdings_snapshot_columns_to_accounts.rbdb/migrate/20260407170000_fix_bond_lots_non_inflation_check_constraint.rbdb/migrate/20260407183000_add_bond_lot_enum_check_constraints.rbdb/schema.rbtest/controllers/accounts_controller_test.rbtest/controllers/bond_lots_controller_test.rbtest/controllers/bonds_controller_test.rbtest/controllers/transactions/bulk_deletions_controller_test.rbtest/fixtures/accounts.ymltest/fixtures/bond_lots.ymltest/fixtures/bonds.ymltest/jobs/settle_matured_bond_lots_job_test.rbtest/models/balance/reverse_calculator_test.rbtest/models/bond_lot_test.rbtest/models/bond_test.rbtest/support/ledger_testing_helper.rb
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/entry.rb (1)
14-28:⚠️ Potential issue | 🟡 MinorAdd
prepend: trueto the settled-lot guard to prevent temporary association destruction.The
has_one :bond_lot, dependent: :destroyassociation (line 14) registers a destroy callback that executes before explicitbefore_destroyhooks. Althoughprevent_deletion_when_linked_bond_lot_settled(line 28) correctly aborts deletion viathrow :abort, the bond lot record gets destroyed during the callback chain before the guard runs. Useprepend: trueto ensure the guard executes first, preventing any intermediate state changes.🛡️ Proposed adjustment
- before_destroy :prevent_deletion_when_linked_bond_lot_settled + before_destroy :prevent_deletion_when_linked_bond_lot_settled, prepend: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/entry.rb` around lines 14 - 28, The has_one :bond_lot association should be changed to run its before_destroy guard first: update the association declaration (has_one :bond_lot, dependent: :destroy) to include prepend: true (e.g., has_one :bond_lot, dependent: :destroy, prepend: true) so the prevent_deletion_when_linked_bond_lot_settled before_destroy callback executes before the bond_lot destroy callback and prevents intermediate destruction.
♻️ Duplicate comments (1)
db/schema.rb (1)
1730-1731:⚠️ Potential issue | 🟠 MajorDon’t cascade-delete bond lots from the database side.
ON DELETE CASCADElets PostgreSQL deletebond_lotswhen anentriesrow is removed by database cascade ordelete/delete_all, bypassing the Rails guard that blocks deletion of settled lots. Prefernullify/restrict here and let the explicit Rails destroy path decide when a bond lot may be removed.🔎 Read-only verification
#!/bin/bash # Verify the FK action and the Rails-side guard that it can bypass. rg -nP -C3 'add_foreign_key "bond_lots", "entries"|add_foreign_key :bond_lots, :entries|on_delete: :cascade|on_delete: :nullify' db/schema.rb db/migrate rg -nP -C4 'before_destroy :prevent_deletion_when_linked_bond_lot_settled|def prevent_deletion_when_linked_bond_lot_settled' app/models/entry.rbExpected after the fix: the
bond_lots -> entriesFK should no longer use database cascade if settled-lot preservation depends on Rails callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/schema.rb` around lines 1730 - 1731, The bond_lots→entries foreign key currently uses ON DELETE CASCADE which lets Postgres bypass Rails' before_destroy guard (prevent_deletion_when_linked_bond_lot_settled) — change the FK so database does not cascade-delete: replace the cascade action with a safer option (e.g., on_delete: :nullify or on_delete: :restrict) for the add_foreign_key "bond_lots", "entries" entry in the schema and, if needed, create a migration that alters the existing constraint rather than relying on schema.rb alone so Rails callbacks on Entry/Entry#prevent_deletion_when_linked_bond_lot_settled can enforce settled-lot preservation.
🧹 Nitpick comments (2)
app/javascript/controllers/bond_lot_inflation_controller.js (2)
51-66: MaketoggleManualInflationFieldprivate.This method is only called internally, while
toggleSubtypeFieldsandrecalculateare the public Stimulus actions. Keeping the helper private makes the controller API clearer.Proposed refactor
- this.toggleManualInflationField() + this.#toggleManualInflationField() } recalculate() { this.toggleSubtypeFields() } - toggleManualInflationField() { + `#toggleManualInflationField`() { if (!this.hasManualInflationFieldTarget || !this.hasManualInflationInputTarget) return const inflationLinked = this.inflationSubtypesValue.includes(this.#subtypeValue())As per coding guidelines, “Use private methods in Stimulus controllers and expose a clear public API.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/bond_lot_inflation_controller.js` around lines 51 - 66, Rename the helper method toggleManualInflationField to a private method using JS private syntax (e.g. `#toggleManualInflationField`) and update all internal calls to it (for example the call this.toggleManualInflationField() at the end of connect and any other internal uses) so they call this.#toggleManualInflationField() instead; keep public actions unchanged (recalculate, toggleSubtypeFields) and ensure the method is not referenced from templates or Stimulus action strings so it remains internal to the controller.
4-14: Trim or split the controller targets.This controller declares 9 targets, which is already past the project’s Stimulus complexity threshold. Consider splitting manual-inflation behavior into a small companion controller, or reducing target categories by using wrapper fieldsets plus values/classes.
As per coding guidelines, “Keep Stimulus controllers lightweight and simple - aim for fewer than 7 controller targets as a sign of excessive complexity.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/bond_lot_inflation_controller.js` around lines 4 - 14, The controller bond_lot_inflation_controller.js currently declares nine static targets (inflationFields, otherFields, inflationInput, otherRequiredInput, manualInflationField, manualInflationInput, subtypeInput, purchasedOnInput, issueDateInput), exceeding the project's Stimulus complexity threshold; split or trim it by moving the manual-inflation responsibilities into a small companion controller (e.g., ManualInflationController) that owns manualInflationField, manualInflationInput, purchasedOnInput and issueDateInput, then remove those four names from bond_lot_inflation_controller.js static targets and add data-controller="manual-inflation" to the relevant markup, or alternatively reduce targets by grouping related elements under a wrapper target (use one wrapper target like manualInflationWrapper and access children via querySelector/value classes) and update the HTML data-targets accordingly so bond_lot_inflation_controller.js keeps fewer than seven targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/javascript/controllers/bond_lot_inflation_controller.js`:
- Around line 37-43: The loop over inflationInputTargets uses dataset flags as
booleans but HTML data attributes are strings; change the checks to parse them
explicitly (e.g., use input.dataset.requiresFirstPeriodCheck === "true" and
input.dataset.optional === "true") so required is computed correctly; update the
logic in the forEach that sets input.disabled, input.required and the branch
that tests input.dataset.requiresFirstPeriodCheck to use these explicit string
comparisons alongside the existing inflationLinked and firstPeriodRateRequired
variables.
In `@config/locales/views/bonds/pl.yml`:
- Around line 135-151: Replace the English phrase "Auto-fetch" in the Polish
locale value for the key auto_fetch_disabled_hint with Polish wording consistent
with auto_fetch_inflation (use "Pobieraj inflację automatycznie od dostawcy");
update the string for auto_fetch_disabled_hint so it reads similarly to line
with auto_fetch_inflation and keeps the same meaning about global auto-import
being disabled and requiring manual CPI entry.
---
Outside diff comments:
In `@app/models/entry.rb`:
- Around line 14-28: The has_one :bond_lot association should be changed to run
its before_destroy guard first: update the association declaration (has_one
:bond_lot, dependent: :destroy) to include prepend: true (e.g., has_one
:bond_lot, dependent: :destroy, prepend: true) so the
prevent_deletion_when_linked_bond_lot_settled before_destroy callback executes
before the bond_lot destroy callback and prevents intermediate destruction.
---
Duplicate comments:
In `@db/schema.rb`:
- Around line 1730-1731: The bond_lots→entries foreign key currently uses ON
DELETE CASCADE which lets Postgres bypass Rails' before_destroy guard
(prevent_deletion_when_linked_bond_lot_settled) — change the FK so database does
not cascade-delete: replace the cascade action with a safer option (e.g.,
on_delete: :nullify or on_delete: :restrict) for the add_foreign_key
"bond_lots", "entries" entry in the schema and, if needed, create a migration
that alters the existing constraint rather than relying on schema.rb alone so
Rails callbacks on Entry/Entry#prevent_deletion_when_linked_bond_lot_settled can
enforce settled-lot preservation.
---
Nitpick comments:
In `@app/javascript/controllers/bond_lot_inflation_controller.js`:
- Around line 51-66: Rename the helper method toggleManualInflationField to a
private method using JS private syntax (e.g. `#toggleManualInflationField`) and
update all internal calls to it (for example the call
this.toggleManualInflationField() at the end of connect and any other internal
uses) so they call this.#toggleManualInflationField() instead; keep public
actions unchanged (recalculate, toggleSubtypeFields) and ensure the method is
not referenced from templates or Stimulus action strings so it remains internal
to the controller.
- Around line 4-14: The controller bond_lot_inflation_controller.js currently
declares nine static targets (inflationFields, otherFields, inflationInput,
otherRequiredInput, manualInflationField, manualInflationInput, subtypeInput,
purchasedOnInput, issueDateInput), exceeding the project's Stimulus complexity
threshold; split or trim it by moving the manual-inflation responsibilities into
a small companion controller (e.g., ManualInflationController) that owns
manualInflationField, manualInflationInput, purchasedOnInput and issueDateInput,
then remove those four names from bond_lot_inflation_controller.js static
targets and add data-controller="manual-inflation" to the relevant markup, or
alternatively reduce targets by grouping related elements under a wrapper target
(use one wrapper target like manualInflationWrapper and access children via
querySelector/value classes) and update the HTML data-targets accordingly so
bond_lot_inflation_controller.js keeps fewer than seven targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 766bf906-00b3-4e11-87f7-90ba2298bf05
📒 Files selected for processing (17)
CONTRIBUTING.mdapp/components/UI/account/activity_feed.html.erbapp/controllers/bond_lots_controller.rbapp/javascript/controllers/bond_lot_form_controller.jsapp/javascript/controllers/bond_lot_inflation_controller.jsapp/models/bond_lot.rbapp/models/enable_banking_item.rbapp/models/entry.rbapp/views/accounts/show/_activity.html.erbapp/views/bond_lots/_form.html.erbapp/views/bond_lots/show.html.erbapp/views/bonds/edit.html.erbconfig/locales/views/bonds/en.ymlconfig/locales/views/bonds/pl.ymldb/migrate/20260420100000_reconcile_bond_core_schema_and_constraints.rbdb/schema.rbtest/models/bond_lot_test.rb
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (8)
- app/views/bonds/edit.html.erb
- app/models/enable_banking_item.rb
- app/views/bond_lots/_form.html.erb
- app/views/accounts/show/_activity.html.erb
- app/views/bond_lots/show.html.erb
- app/javascript/controllers/bond_lot_form_controller.js
- config/locales/views/bonds/en.yml
- app/models/bond_lot.rb
Collaborator
|
Lots of open PR comments, will wait until they are addressed - you ask for review @UberDudePL. |
Author
|
@coderabbitai review |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
Author
|
@coderabbitai review |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
…gration reversibility
…iew, add inflation-linked defaults fallback
Author
|
@jjmata working them through, but cannot run Coderabbit anymore :( |
Comment on lines
+3
to
+33
| create_table :bond_lots, id: :uuid do |t| | ||
| t.references :bond, null: false, foreign_key: { on_delete: :cascade }, type: :uuid | ||
| t.date :purchased_on, null: false | ||
| t.decimal :amount, precision: 19, scale: 4, null: false | ||
| t.integer :term_months, null: false | ||
| t.date :maturity_date, null: false | ||
| t.decimal :interest_rate, precision: 10, scale: 3 | ||
| t.string :subtype, null: false, default: "other" | ||
| t.string :rate_type | ||
| t.string :coupon_frequency | ||
| t.references :entry, type: :uuid, foreign_key: { to_table: :entries, on_delete: :nullify }, index: false | ||
|
|
||
| t.date :issue_date | ||
| t.decimal :first_period_rate, precision: 10, scale: 3 | ||
| t.decimal :inflation_margin, precision: 10, scale: 3 | ||
| t.decimal :inflation_rate_assumption, precision: 10, scale: 3 | ||
| t.integer :cpi_lag_months | ||
| t.decimal :early_redemption_fee, precision: 19, scale: 4 | ||
| t.decimal :units, precision: 12, scale: 2 | ||
| t.decimal :nominal_per_unit, precision: 19, scale: 4 | ||
|
|
||
| t.boolean :auto_close_on_maturity, null: false, default: true | ||
| t.date :closed_on | ||
| t.decimal :settlement_amount, precision: 19, scale: 4 | ||
| t.decimal :tax_withheld, precision: 19, scale: 4 | ||
| t.string :tax_strategy, null: false, default: "standard" | ||
| t.decimal :tax_rate, precision: 6, scale: 3, null: false, default: 19.0 | ||
| t.boolean :requires_rate_review, null: false, default: false | ||
|
|
||
| t.string :product_code | ||
|
|
There was a problem hiding this comment.
db/schema.rb shows bond_lots has auto_fetch_inflation and inflation_provider columns (and an index on inflation_provider), but 20260331120000_create_bond_lots.rb never creates them and there’s no later migration adding them. This will cause schema drift for new environments that run migrations (they won’t have these columns even though schema.rb does). Either add these columns (+ index/defaults) via migrations (preferred: in the create table migration, since the table is new) or re-dump schema.rb after removing them from the database.
…y instead of failing validation
…mulus targets to 6, improve bulk delete notice
…nflation schema drift
| end | ||
|
|
||
| def show | ||
| @account = @bond_lot.account |
| <div class="rounded-xl border border-secondary bg-container-inset p-3 text-sm"> | ||
| <div class="grid gap-2 md:grid-cols-2"> | ||
| <p class="text-primary"><strong><%= t(".overview_principal") %>:</strong> <span class="privacy-sensitive"><%= format_money(Money.new(@bond_lot.amount, @account.currency)) %></span></p> | ||
| <p class="text-primary"><strong><%= t(".overview_settlement") %>:</strong> <span class="privacy-sensitive"><%= format_money(Money.new(@bond_lot.settlement_amount.to_d, @account.currency)) %></span></p> |
Comment on lines
+1
to
+5
| class ChangeBondLotsEntryFkToCascade < ActiveRecord::Migration[7.2] | ||
| def up | ||
| remove_foreign_key :bond_lots, :entries | ||
| add_foreign_key :bond_lots, :entries, on_delete: :cascade | ||
| end |
Comment on lines
+1
to
+3
| import { Controller } from "@hotwired/stimulus" | ||
|
|
||
| export default class extends Controller { |
Comment on lines
+1
to
+3
| import { Controller } from "@hotwired/stimulus" | ||
|
|
||
| export default class extends Controller { |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
This PR introduces the core Bond foundation for Sure.
It adds bond account support and bond lot lifecycle management, including creation and editing flows, maturity settlement, tax handling, and manual inflation-linked support. The scope is intentionally limited to keep the first Bond PR reviewable and easy to reason about.
Included in this PR
Explicitly out of scope
Why this split
The goal is to ship a solid Bond Core first, without bundling extra infrastructure and presentation work into the same review.
This keeps the PR smaller, lower risk, and easier to merge.
Verification
Targeted tests and RuboCop checks passed locally.
Summary by CodeRabbit