feat(splits): add exclusion support for splits and improve rendering#1661
Conversation
…mprove rendering of split transactions
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads a grouped display flag through controllers, views, and forms and adds per-child ChangesExcluded Splits with Grouped Context
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant SplitsController
participant EntryModel
participant TransactionsController
participant Views
User->>Browser: submit split form (splits with excluded, grouped)
Browser->>SplitsController: POST /splits (params + grouped)
SplitsController->>EntryModel: Entry#split!(splits including excluded)
EntryModel-->>SplitsController: creates child entries with excluded flags
SplitsController->>Browser: redirect to entry view
Browser->>TransactionsController: request/update (grouped param preserved)
TransactionsController->>Views: compute in_split_group and render turbo-stream partials (pass in_split_group)
Views-->>Browser: turbo-stream replacements and forms (hidden grouped fields)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Documentation Updates 1 document(s) were updated by changes in this PR: How split transaction child exclusion was implementedrenamed from "Why can't individual parts of a split transaction be excluded from analytics, and what changes are needed to support this?" View Changes@@ -1,31 +1,34 @@
-Currently, individual parts of a split transaction cannot be excluded from analytics due to intentional UI and controller restrictions — not a database limitation.
+Individual parts of a split transaction can now be excluded from analytics. This functionality was implemented in [PR #1661](https://github.com/we-promise/sure/pull/1661).
-## Root Cause
+## Implementation
-1. **View restriction** — The exclusion toggle is hidden for both split parents and split children in the transaction detail view:
+The feature required three key changes, all of which have been completed:
+
+1. **View restriction removed** — The exclusion toggle was previously hidden for both split parents and split children. The conditional has been updated to allow the toggle for split children while still hiding it for split parents:
```erb
- <% unless @entry.split_parent? || @entry.split_child? %>
+ <% unless @entry.split_parent? %>
<%= f.toggle :excluded, { data: { auto_submit_form_target: "auto" } } %>
<% end %>
```
-2. **Controller restriction** — The `SplitsController` only permits `name`, `amount`, and `category_id` in its strong parameters — the `excluded` field is not accepted.
+2. **Controller parameter permitted** — The `SplitsController` now permits the `excluded` field in strong parameters, allowing it to be passed through during split creation and updates:
+ ```ruby
+ def split_params
+ params.require(:split).permit(splits: [ :name, :amount, :category_id, :excluded ])
+ end
+ ```
-## What Already Works
+3. **Model support added** — The `Entry#split!` method now accepts and handles the `excluded` key, properly casting truthy string values (`"true"`, `"1"`) to boolean:
+ ```ruby
+ excluded: TRUTHY_VALUES.include?(split_attrs[:excluded])
+ ```
-- The **database already supports exclusion** per split child, as each split child is a real `Entry` record with its own `excluded` boolean flag.
-- Analytics queries already filter on `ae.excluded = false`, so the underlying logic is in place.
+## How It Works
-## Required Changes
+- The **database already supported exclusion** per split child, as each split child is a real `Entry` record with its own `excluded` boolean flag.
+- Analytics queries filter on `ae.excluded = false`, so excluded split children are automatically removed from balance calculations.
+- Users can toggle exclusion on individual split children via the transaction detail drawer.
-1. **View** — Update the conditional in the form partial to allow the exclusion toggle for `split_child?` entries (while still hiding it for `split_parent?`).
+## User Flow
-2. **Controller** — Allow the `excluded` parameter through the `SplitsController`'s strong params, or route split child updates through `TransactionsController`, which already permits `excluded` in `entry_params`.
-
-3. **Validation** — No model-level changes are needed, as the existing `cannot_unexclude_split_parent` validation only locks exclusion on **parent** entries, not children.
-
-4. **Split editor (optional)** — Add an exclusion checkbox per row in the split creation/edit form so users can set exclusion at split-creation time.
-
-## Known Related Issue
-
-There is a noted issue where `Balance::SyncCache` does not filter `excluded: true` entries in `flows_for_date()`, which could cause double-counting in cash flow calculations. This would also affect excluded split children if not resolved.
+The exclusion toggle is available in the **transaction detail drawer** after a split has been created. The split creation/editing modal does not include exclusion controls — users create the split first, then open any child transaction to toggle exclusion in the settings panel. |
There was a problem hiding this comment.
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)
389-397:⚠️ Potential issue | 🟠 Major
split_attrs[:excluded]can benil, overriding the column default and storingNULLin the database.The
excludedcolumn hasdefault: false(no explicitNOT NULLconstraint), so the database will accept theNULLvalue. However, this is still a bug: unchecked HTML checkboxes don't submit a value, causings[:excluded]to benilin the controller's split params (line 22 insplits_controller.rb). Thisnilthen reaches line 395 and overrides the intended default, storingNULLinstead offalse. Boolean columns with defaults should never beNULL—this will cause issues with boolean predicates likeexcluded?and make the value indistinguishable from an intentionalfalse.All test cases (and any other callers of
split!) omit the:excludedkey entirely, so they will be affected by this bug once users interact with the form.Proposed fix
child_entries.create!( account: account, date: date, name: split_attrs[:name], amount: split_attrs[:amount], currency: currency, - excluded: split_attrs[:excluded], + excluded: split_attrs[:excluded] || false, entryable: child_transaction )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/entry.rb` around lines 389 - 397, The child_entries.create! call is passing split_attrs[:excluded] which can be nil and override the DB default; change the argument to ensure a non-nil boolean (e.g. use split_attrs.fetch(:excluded, false) or otherwise coerce to a boolean) so child_entries.create! (in Entry#split! / the block creating child entries) always writes true/false rather than NULL for the excluded column.
🧹 Nitpick comments (2)
app/views/category/dropdowns/show.html.erb (1)
88-88: 💤 Low valueUse
hidden_field_tagfor consistency with every othergroupedfield in this PR.
f.hidden_field :groupedin aform_with url:(no model/scope) does producename="grouped"— so this is functionally correct — but the entire PR consistently useshidden_field_tag :grouped, params[:grouped]for this purpose (seeshow.html.erblines 57, 99, 273, 290, 316, and_notes.html.erbline 6).♻️ Proposed change
- <%= f.hidden_field :grouped, value: params[:grouped] %> + <%= hidden_field_tag :grouped, params[:grouped] %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/category/dropdowns/show.html.erb` at line 88, Replace the form builder hidden field call f.hidden_field :grouped with the standalone helper hidden_field_tag :grouped, params[:grouped] to match the rest of the PR; locate the occurrence where f.hidden_field :grouped is used (inside the form_with block) and change it to hidden_field_tag :grouped, params[:grouped] so the generated input is consistent with other uses like the existing hidden_field_tag :grouped, params[:grouped] instances.app/views/transactions/_transaction.html.erb (1)
74-74: 💤 Low valueOptional: avoid appending
grouped=falseto every non-split-group transaction URL.
entry_path(entry, grouped: in_split_group)always passes the param, so normal-context transaction links get?grouped=falseappended even though the controller treats absent andfalseidentically (params[:grouped] == "true").♻️ Proposed fix
- entry_path(entry, grouped: in_split_group), + in_split_group ? entry_path(entry, grouped: true) : entry_path(entry),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/transactions/_transaction.html.erb` at line 74, The link helper always emits grouped=false because entry_path(entry, grouped: in_split_group) passes the param even when in_split_group is false; change the view so the grouped param is only included when in_split_group is true (i.e., call entry_path without the grouped arg for the normal case and only pass grouped: true when in_split_group is truthy) so URLs don't get ?grouped=false; update the usage of entry_path in _transaction.html.erb to conditionally include the grouped parameter based on in_split_group.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/models/entry.rb`:
- Around line 389-397: The child_entries.create! call is passing
split_attrs[:excluded] which can be nil and override the DB default; change the
argument to ensure a non-nil boolean (e.g. use split_attrs.fetch(:excluded,
false) or otherwise coerce to a boolean) so child_entries.create! (in
Entry#split! / the block creating child entries) always writes true/false rather
than NULL for the excluded column.
---
Nitpick comments:
In `@app/views/category/dropdowns/show.html.erb`:
- Line 88: Replace the form builder hidden field call f.hidden_field :grouped
with the standalone helper hidden_field_tag :grouped, params[:grouped] to match
the rest of the PR; locate the occurrence where f.hidden_field :grouped is used
(inside the form_with block) and change it to hidden_field_tag :grouped,
params[:grouped] so the generated input is consistent with other uses like the
existing hidden_field_tag :grouped, params[:grouped] instances.
In `@app/views/transactions/_transaction.html.erb`:
- Line 74: The link helper always emits grouped=false because entry_path(entry,
grouped: in_split_group) passes the param even when in_split_group is false;
change the view so the grouped param is only included when in_split_group is
true (i.e., call entry_path without the grouped arg for the normal case and only
pass grouped: true when in_split_group is truthy) so URLs don't get
?grouped=false; update the usage of entry_path in _transaction.html.erb to
conditionally include the grouped parameter based on in_split_group.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8a2e6e9-3255-4b2a-a88c-4ae9d398cdb0
📒 Files selected for processing (12)
app/controllers/splits_controller.rbapp/controllers/transaction_categories_controller.rbapp/controllers/transactions_controller.rbapp/models/entry.rbapp/views/categories/_menu.html.erbapp/views/category/dropdowns/_row.html.erbapp/views/category/dropdowns/show.html.erbapp/views/entries/_entry.html.erbapp/views/transactions/_notes.html.erbapp/views/transactions/_transaction.html.erbapp/views/transactions/_transaction_category.html.erbapp/views/transactions/show.html.erb
jjmata
left a comment
There was a problem hiding this comment.
Code Review
Thanks for this PR — the UX problem is real and the overall approach is sensible. A few issues worth addressing before merge.
🔴 Bug: string "false" is truthy — boolean coercion is broken
In app/models/entry.rb:
excluded: split_attrs[:excluded] || false,split_attrs[:excluded] comes from a form param, so it will be the string "false" when unchecked — which is truthy in Ruby. "false" || false evaluates to "false", and ActiveRecord will cast that string as true for a boolean column. Every split child will be marked excluded unless the field is absent entirely.
The fix is explicit casting:
excluded: ActiveModel::Type::Boolean.new.cast(split_attrs[:excluded]) || false,Or at the controller layer before the hash is built.
🔴 Missing test coverage for the new excluded feature
test/models/entry_split_test.rb and test/controllers/splits_controller_test.rb both exist but neither tests the new excluded parameter on split children. At minimum, add:
- A model test asserting that
split!withexcluded: trueon a child creates that child withexcluded: true. - A controller test that passes
excluded: "true"in the split params and asserts the resulting child entry is excluded.
Without these, the string-coercion bug above (or any future regression) would not be caught.
🟡 DRY violation: in_split_group computed identically in two controllers
transactions_controller.rb:143 and transaction_categories_controller.rb:24 both compute:
in_split_group = @entry.split_child? && Current.user.show_split_grouped? && params[:grouped] == "true"This belongs in a shared helper (e.g. TransactionsHelper or a before_action on a shared concern). One change to the logic currently requires two edits.
🟡 Redundant local variable default in views
Several partials now have both a locals: magic comment with a default and an explicit local_assigns.fetch call:
<%# locals: (entry:, balance_trend: nil, view_ctx: "global", in_split_group: false) %>
<% in_split_group = local_assigns.fetch(:in_split_group, false) %>The magic comment (locals:) is the Rails 7.1 way to declare and default locals — the fetch line is redundant when the comment is present. Pick one approach and be consistent. The other partials in this codebase only use the magic comment.
🟡 Ternary can be simplified
In app/views/transactions/_transaction.html.erb:
in_split_group ? entry_path(entry, grouped: true) : entry_path(entry)Since in_split_group is already a boolean, this simplifies to:
entry_path(entry, grouped: in_split_group)🟢 Behavioural concern: what happens when an excluded split child is summed?
The PR enables marking individual split children as excluded. The parent is always excluded (that's how splits work — the parent is excluded and its children represent the breakdown). If a child is also excluded, does the balance/account calculation still hold? The Entry.where(excluded: false) scope used for balance queries (e.g. entry.rb:102) would then drop the child too, potentially under-counting. Please confirm the intent here and add a test if this is a supported use case.
🟢 hidden_field_tag :grouped, nil sends grouped= rather than omitting the param
When params[:grouped] is nil (the common case on non-split pages), every form that does:
<%= hidden_field_tag :grouped, params[:grouped] %>will submit grouped= (empty string) rather than omitting the param entirely. The controller guard params[:grouped] == "true" handles this correctly (empty string ≠ "true"), but the extra param adds noise. A guard like hidden_field_tag :grouped, "true" if params[:grouped] == "true" would be cleaner.
Generated by Claude Code
Fix boolean coercion bug where string "false" from form params was truthy in Ruby, causing all split children to be marked excluded. Use ActiveModel::Type::Boolean for explicit casting in Entry#split!. Additional changes addressing code review feedback: - Extract duplicated in_split_group logic from TransactionsController and TransactionCategoriesController into TransactionsHelper - Remove redundant local_assigns.fetch calls in partials that already declare defaults via the Rails 7.1 locals: magic comment - Simplify ternary in _transaction.html.erb to pass grouped directly - Guard hidden_field_tag :grouped to only emit when value is "true" - Add model tests for excluded on split children (boolean and string) - Add controller test for excluded param through full HTTP stack - Add test confirming excluded children are dropped from balance queries
|
Can you review @sokie? 🙏 |
|
The approach is clean and the test coverage hits the important cases (boolean casting, per-child persistence, filtering). A couple of things worth noting: Split editor UI gap
excluded: ActiveModel::Type::Boolean.new.cast(split_attrs[:excluded]) || falseThis allocates a new type object for every split row in the loop. Not a meaningful performance issue at typical split counts, but
The Generated by Claude Code |
Let me know if anything else is needed before merging! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/entry.rb (1)
395-395: ⚡ Quick winMinor: hoist the truthy-values array to a frozen constant.
[true, "true", "1", 1]is a new heap object on everysplits.mapiteration. For typical split counts this is negligible, but extracting it to a frozen constant removes the repeated allocation and makes the intent self-documenting.♻️ Proposed refactor
Add a private constant near the top of the private section (or alongside the class constants):
private + BOOLEAN_TRUE_VALUES = [ true, "true", "1", 1 ].freeze + def cannot_unexclude_split_parentThen reference it in
split!:- excluded: [true, "true", "1", 1].include?(split_attrs[:excluded]), + excluded: BOOLEAN_TRUE_VALUES.include?(split_attrs[:excluded]),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/entry.rb` at line 395, Hoist the repeated truthy array into a frozen private constant (e.g. TRUTHY_VALUES = [true, "true", "1", 1].freeze) declared near the other private/class constants, then replace the inline array in the split handling (the line building excluded: ... inside the split!/splits mapping) with TRUTHY_VALUES.include?(split_attrs[:excluded]) to avoid allocating the array on each iteration and make intent clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/models/entry.rb`:
- Line 395: Hoist the repeated truthy array into a frozen private constant (e.g.
TRUTHY_VALUES = [true, "true", "1", 1].freeze) declared near the other
private/class constants, then replace the inline array in the split handling
(the line building excluded: ... inside the split!/splits mapping) with
TRUTHY_VALUES.include?(split_attrs[:excluded]) to avoid allocating the array on
each iteration and make intent clearer.
…d-split-group-rendering
Extract the array of truthy values used for excluded attribute check into a private constant to improve code maintainability and avoid duplication of the magic array.
|
Thank you, @CrossDrain ... can you include before/after screenshot in the first PR comment to help reviewers? 🙏 |
|
@jjmata The screenshots show:
Let me know if there's anything final I need to touch on. |
…d-split-group-rendering
|
I've resolved a merge conflict which was caused by changes from PR #1671 ( |
So that was actually a "no human interaction" merge! 🙄 I'm backing it out and you will probably have to update here as well, @CrossDrain. Sorry. |
|
@jjmata Oh okay, thanks for letting me know, no worries! Should I roll back my merge as well, or should I just let it as is for it to be ready to merge if the feat/search-highlight PR gets re-opened and merged first? |
Roll it back, please. Not planning on taking those other PRs until they are changed for Design System-type support. That way you can also take care of the current conflict with |
sure-design
left a comment
There was a problem hiding this comment.
Re-review — independent read of the current diff
The feedback from the previous round was substantially addressed. Here's what I confirmed is fixed and what still needs attention before merge.
✅ Previously-raised issues — confirmed resolved
| Issue | Fix confirmed |
|---|---|
"false" || false evaluates to truthy string (every child marked excluded) |
TRUTHY_VALUES.include?(split_attrs[:excluded]) — correctly returns false for nil, "false", 0, etc. |
nil stored as NULL for excluded column |
Same fix; TRUTHY_VALUES.include?(nil) → false, always writes true/false |
ActiveModel::Type::Boolean.new allocated per loop iteration |
Hoisted to frozen TRUTHY_VALUES constant with private_constant |
Missing tests for excluded |
3 model tests (boolean persistence, string casting, balance filtering) + 1 controller test — all present |
in_split_group logic duplicated across two controllers |
Extracted to TransactionsHelper#in_split_group?; both controllers call helpers.in_split_group? |
hidden_field_tag :grouped, nil submitting empty param |
All hidden fields now use hidden_field_tag :grouped, "true" if params[:grouped] == "true" |
f.hidden_field :grouped inconsistency |
Changed to hidden_field_tag throughout |
🔴 Blocker — merge conflict, rollback requested
mergeable_state is dirty. @jjmata explicitly asked on 2026-05-06 for the feat/search-highlight merge to be rolled back and the conflict with main resolved properly. That hasn't happened yet — this PR can't merge as-is.
🟡 update action untested for excluded
splits_controller.rb threads excluded through both create (line 22) and update (line 54), but only the create path has a test. A regression in the update path would go undetected. Please add a parallel test alongside the existing create test:
test "update with excluded parameter sets child as excluded" do
@entry.split!([
{ name: "Groceries", amount: 70, category_id: nil },
{ name: "Household", amount: 30, category_id: nil }
])
patch transaction_split_path(@entry), params: {
split: {
splits: [
{ name: "Groceries", amount: "-70", category_id: "", excluded: "true" },
{ name: "Household", amount: "-30", category_id: "", excluded: "false" }
]
}
}
assert_redirected_to transactions_url
children = @entry.child_entries.order(:amount)
refute children.first.excluded?
assert children.last.excluded?
end🟢 ?grouped=false appended to every non-split transaction URL
In app/views/transactions/_transaction.html.erb:74:
entry_path(entry, grouped: in_split_group)When in_split_group is false (the common case), this produces ?grouped=false on every transaction link. Behavior is correct (the controller checks params[:grouped] == "true"), but it's URL noise. Quick fix:
in_split_group ? entry_path(entry, grouped: true) : entry_path(entry)🟢 Comment on helper violates project convention
# Returns true if the entry is a split child and the user has split grouping enabled.
# Used to determine whether to show grouped split display.
def in_split_group?(entry, params_grouped)Per CLAUDE.md: "Default to writing no comments. Only add one when the WHY is non-obvious." This comment explains what the method does (which the name already communicates), not why it exists. Please remove it.
Balance math — design confirmed sound
Excluding an individual split child intentionally under-counts that portion of the transaction from balances. This is the stated use case ("track separately but not count toward totals") and is confirmed by the new model test. No issue here.
Generated by Claude Code
Generated by Claude Code
f16caf5 to
0d514d7
Compare
…ge for excluded split parameters
e475dd9 to
2e6d8de
Compare
…d-split-group-rendering
|
Apologies for all the confusion and the messy commit history! I got a little confused with all the merges, which led to an even more confusing loop of conflict resolutions and reverts. To clean everything up and ensure the branch is in a perfect state, I force-pushed it back to So, currently all of my original changes are intact, and I have just pushed one new commit (
I also merged main to this branch and it is now completely in sync and ready to be merged! If you need any further clarification feel free to reach out to me here on GitHub or on Discord. |
…d-split-group-rendering
…d-split-group-rendering
Split Children Exclusion & Rendering Fixes
Overview
This PR introduces two major improvements to split transaction handling:
groupedstate across the UI.Feature 1: Exclusion Function for Split Children
Problem
Previously, when splitting a transaction, all child entries were always included in balance calculations. There was no way to exclude specific parts of a split (e.g., a personal expense from a shared bill that you want to track separately but not count toward your totals).
Solution
Added an
excludedboolean attribute to split child entries. This allows users to:Implementation
app/models/entry.rb: AddedTRUTHY_VALUESconstant andexcluded: TRUTHY_VALUES.include?(split_attrs[:excluded])to handle string/boolean valuesapp/controllers/splits_controller.rb: Permitted:excludedparam and passed it throughapp/views/transactions/show.html.erb: Added exclusion toggle UI for split children (previously only shown for parent)Screenshots: Exclusion Function for Split Children
🔄 Before
All split children always included in balances. No exclusion toggle available.
✅ After
Each child has an "Exclude" toggle. Excluded children are dimmed and filtered from balance calculations.
The exclusion toggle is currently available only in the transaction detail drawer (after a split has been created). The split creation/editing modal does not yet include exclusion controls.
This is intentional. This PR focuses on establishing the core exclusion architecture (model, controller, detail drawer)
Adding exclusion to the split editor may create duplication later, as this functionality could become obsolete if a proper reimbursements feature is developed.
Current user flow: Create split → Open any child transaction → Toggle "Exclude" in settings panel.
Feature 2: Split Transaction Rendering Fixes
Problem
Split children were losing their visual indentation and context in several scenarios:
groupedparameter wasn't preserved, causing children to lose their indentation when returning to the listSolution
in_split_grouplocal parameter propagated through all transaction rendering partialspl-8 lg:pl-12) whenin_split_groupis truegroupedparameter in all links and forms within split childrenChanges
app/views/transactions/_transaction.html.erb: Addedin_split_grouplocal, applied indentation class, passed to category partial, and addedgroupedparam to entry linkapp/views/transactions/_transaction_category.html.erb: Addedin_split_grouplocal and passed to category menuapp/views/categories/_menu.html.erb: Addedin_split_grouplocal and passedgroupedparam to dropdown srcapp/views/transactions/show.html.erb: Removedsplit_child?from settings condition, added hiddengroupedfield to all formsapp/views/entries/_entry.html.erb: Addedin_split_grouplocal and propagated to entryable partialScreenshots: Split Transaction Rendering Fixes
🔄 Before
Indentation lost on navigation. Category dropdown loses context.
✅ After
Indentation preserved throughout. All links/forms maintain grouped state
Technical Details
Data Model
Balance Calculations
Excluded split children are automatically filtered:
Testing
test/models/entry_split_test.rb: Added tests for exclusion casting and balance filteringtest/controllers/splits_controller_test.rb: Added test for excluded param handlingSummary by CodeRabbit
New Features
UI Improvements
Tests