fix(transactions): include one-time in reports, keep out of budget medians#1995
fix(transactions): include one-time in reports, keep out of budget medians#1995glorydavid03023 wants to merge 17 commits into
Conversation
superagent-security
Bot
added
the
contributor:verified
|
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:
📝 WalkthroughWalkthroughSplit excluded transaction kinds into REPORT_EXCLUDED_KINDS vs BUDGET_EXCLUDED_KINDS, thread a new for_budget flag through IncomeStatement/Totals (memoized by period+mode), adjust Budget to use budget-mode totals, and make ReportsController use report-mode exclusions. ChangesOne-Time vs Excluded Transaction Separation
Sequence Diagram(s)sequenceDiagram
participant User
participant ReportsController
participant IncomeStatement
participant Totals
participant DB
User->>ReportsController: request report/export
ReportsController->>DB: query transactions excluding REPORT_EXCLUDED_KINDS
ReportsController-->>User: report (includes one-time)
User->>IncomeStatement: expense_totals(period, for_budget:true/false)
IncomeStatement->>Totals: totals_query(excluded_kinds)
Totals->>DB: SQL using excluded_kinds_sql
DB-->>Totals: aggregated totals
Totals-->>IncomeStatement: totals (cached by period + for_budget)
IncomeStatement-->>User: memoized totals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
![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
🧹 Nitpick comments (1)
app/controllers/reports_controller.rb (1)
359-360: ⚡ Quick winUpdate stale exclusion comments to match
REPORT_EXCLUDED_KINDS.The three comments still state one-time transactions are excluded, but the new filter now includes one-time transactions in reports. Please update the comments to avoid confusion.
✏️ Suggested comment-only patch
- # Exclude transfers, one-time, and CC payments (matching income_statement logic) + # Exclude transfers and CC payments (reports include one-time transactions)- # Exclude transfers, one-time, and CC payments (matching income_statement logic) + # Exclude transfers and CC payments (reports include one-time transactions)- # Exclude transfers, one-time, and CC payments (matching income_statement logic) + # Exclude transfers and CC payments (reports include one-time transactions)Also applies to: 365-365, 663-664, 669-669, 700-701, 706-706
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/reports_controller.rb` around lines 359 - 360, The comment that reads "Exclude transfers, one-time, and CC payments (matching income_statement logic)" is stale because REPORT_EXCLUDED_KINDS no longer excludes one-time transactions; update the comment near the Transaction query to reflect current behavior (e.g., "Exclude transfers and CC payments; one-time transactions are included per REPORT_EXCLUDED_KINDS") and make the same change for the other occurrences noted (the other comment instances around the Transaction/query blocks). Ensure you reference REPORT_EXCLUDED_KINDS in the updated comment so readers know the exclusion set is driven by that constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/budget.rb`:
- Around line 326-327: actual_income is still calling
family.income_statement.income_totals(period: self.period).total and thus
bypasses the budget-scoped totals; update the actual_income method to use the
budget-scoped totals by calling income_statement.income_totals with for_budget:
true (or reuse the existing income_totals helper) e.g. replace the direct
family.income_statement.income_totals(...) call with
income_statement.income_totals(period: period, for_budget: true).total or call
the model's income_totals to ensure one-time income is excluded.
In `@app/models/income_statement/totals.rb`:
- Around line 164-165: The manual string interpolation in excluded_kinds_sql is
brittle; change it to use the DB adapter's quoting (e.g. connection.quote or
ActiveRecord::Base.connection.quote) when building the list so each kind is
safely quoted (e.g. map each `@excluded_kinds` through connection.quote and join
with ", "), preserving the memoization of `@excluded_kinds_sql` in the
excluded_kinds_sql method.
---
Nitpick comments:
In `@app/controllers/reports_controller.rb`:
- Around line 359-360: The comment that reads "Exclude transfers, one-time, and
CC payments (matching income_statement logic)" is stale because
REPORT_EXCLUDED_KINDS no longer excludes one-time transactions; update the
comment near the Transaction query to reflect current behavior (e.g., "Exclude
transfers and CC payments; one-time transactions are included per
REPORT_EXCLUDED_KINDS") and make the same change for the other occurrences noted
(the other comment instances around the Transaction/query blocks). Ensure you
reference REPORT_EXCLUDED_KINDS in the updated comment so readers know the
exclusion set is driven by that constant.
🪄 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: 96228cc0-d661-45c4-ab7a-2530d49b2bb1
📒 Files selected for processing (7)
app/controllers/reports_controller.rbapp/models/budget.rbapp/models/income_statement.rbapp/models/income_statement/totals.rbapp/models/transaction.rbconfig/locales/views/transactions/en.ymltest/models/income_statement_test.rb
…dians Split transfer exclusions from one-time: historical totals, reports, and period expense/income use REPORT_EXCLUDED_KINDS; budget medians and budget category actuals still use BUDGET_EXCLUDED_KINDS. Fixes we-promise#1994
3abc0d4 to
51eb140
Compare
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 (3)
app/controllers/reports_controller.rb (3)
359-359:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the comment to reflect the new exclusion behavior.
The comment states "Exclude transfers, one-time, and CC payments" but with
REPORT_EXCLUDED_KINDS, only transfers and CC payments are excluded. One-time transactions are now included in reports per the PR objectives.📝 Proposed fix
- # Exclude transfers, one-time, and CC payments (matching income_statement logic) + # Exclude transfers and CC payments (matching income_statement report logic)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/reports_controller.rb` at line 359, Update the outdated comment that currently reads "Exclude transfers, one-time, and CC payments" to reflect the actual exclusion logic: mention only transfers and CC payments are excluded. Locate the comment near the REPORT_EXCLUDED_KINDS usage in reports_controller (the block that references REPORT_EXCLUDED_KINDS) and change it to something like "Exclude transfers and CC payments (matching income_statement logic)" so it matches the implemented behavior.
700-700:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the comment to reflect the new exclusion behavior.
The comment states "Exclude transfers, one-time, and CC payments" but with
REPORT_EXCLUDED_KINDS, only transfers and CC payments are excluded. One-time transactions are now included in monthly breakdown exports per the PR objectives.📝 Proposed fix
- # Exclude transfers, one-time, and CC payments (matching income_statement logic) + # Exclude transfers and CC payments (matching income_statement report logic)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/reports_controller.rb` at line 700, The comment above the monthly breakdown export no longer matches behavior: update the comment that currently reads "Exclude transfers, one-time, and CC payments (matching income_statement logic)" to accurately describe that only transfers and CC payments are excluded based on REPORT_EXCLUDED_KINDS; remove reference to one-time transactions. Locate the comment near the REPORT_EXCLUDED_KINDS usage in reports_controller (e.g., around the monthly breakdown/export logic) and change the text to something like "Exclude transfers and CC payments (matching income_statement logic)" so it reflects the actual exclusion rules.
663-663:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the comment to reflect the new exclusion behavior.
The comment states "Exclude transfers, one-time, and CC payments" but with
REPORT_EXCLUDED_KINDS, only transfers and CC payments are excluded. One-time transactions are now included in exported reports per the PR objectives.📝 Proposed fix
- # Exclude transfers, one-time, and CC payments (matching income_statement logic) + # Exclude transfers and CC payments (matching income_statement report logic)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/reports_controller.rb` at line 663, Update the comment that currently reads "Exclude transfers, one-time, and CC payments (matching income_statement logic)" to reflect the current behavior: only transfers and CC payments are excluded via REPORT_EXCLUDED_KINDS; one-time transactions are included in exported reports. Edit the comment located next to REPORT_EXCLUDED_KINDS / the related filtering logic in ReportsController (reports_controller.rb) to state "Exclude transfers and CC payments; one-time transactions are included."
♻️ Duplicate comments (1)
app/models/budget.rb (1)
326-327:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBudget income actuals still bypass the budget-scoped totals
Line 294 still calls non-budget income totals directly, so one-time income can leak into budget metrics despite this helper now using
for_budget: true.Suggested fix
def actual_income - family.income_statement.income_totals(period: self.period).total + income_totals.total end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/budget.rb` around lines 326 - 327, The code is still calling income_statement.income_totals without scoping to the budget, allowing one-time income to leak; update the direct call (the other location that invokes income_statement.income_totals) to either use the budget-scoped helper income_totals or pass for_budget: true to income_statement.income_totals so the totals are computed with for_budget: true; locate the direct call and replace it with a call to income_totals or income_statement.income_totals(period: period, for_budget: true) to ensure budget-scoped totals are used everywhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/controllers/reports_controller.rb`:
- Line 359: Update the outdated comment that currently reads "Exclude transfers,
one-time, and CC payments" to reflect the actual exclusion logic: mention only
transfers and CC payments are excluded. Locate the comment near the
REPORT_EXCLUDED_KINDS usage in reports_controller (the block that references
REPORT_EXCLUDED_KINDS) and change it to something like "Exclude transfers and CC
payments (matching income_statement logic)" so it matches the implemented
behavior.
- Line 700: The comment above the monthly breakdown export no longer matches
behavior: update the comment that currently reads "Exclude transfers, one-time,
and CC payments (matching income_statement logic)" to accurately describe that
only transfers and CC payments are excluded based on REPORT_EXCLUDED_KINDS;
remove reference to one-time transactions. Locate the comment near the
REPORT_EXCLUDED_KINDS usage in reports_controller (e.g., around the monthly
breakdown/export logic) and change the text to something like "Exclude transfers
and CC payments (matching income_statement logic)" so it reflects the actual
exclusion rules.
- Line 663: Update the comment that currently reads "Exclude transfers,
one-time, and CC payments (matching income_statement logic)" to reflect the
current behavior: only transfers and CC payments are excluded via
REPORT_EXCLUDED_KINDS; one-time transactions are included in exported reports.
Edit the comment located next to REPORT_EXCLUDED_KINDS / the related filtering
logic in ReportsController (reports_controller.rb) to state "Exclude transfers
and CC payments; one-time transactions are included."
---
Duplicate comments:
In `@app/models/budget.rb`:
- Around line 326-327: The code is still calling income_statement.income_totals
without scoping to the budget, allowing one-time income to leak; update the
direct call (the other location that invokes income_statement.income_totals) to
either use the budget-scoped helper income_totals or pass for_budget: true to
income_statement.income_totals so the totals are computed with for_budget: true;
locate the direct call and replace it with a call to income_totals or
income_statement.income_totals(period: period, for_budget: true) to ensure
budget-scoped totals are used everywhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb07d7ee-3468-4704-aeef-f5bb07acb01e
📒 Files selected for processing (7)
app/controllers/reports_controller.rbapp/models/budget.rbapp/models/income_statement.rbapp/models/income_statement/totals.rbapp/models/transaction.rbconfig/locales/views/transactions/en.ymltest/models/income_statement_test.rb
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/transactions/en.yml
|
The A few notes: Cache key bump: Bumping from
Updated locale string: The new description ( Test naming: Splitting into Generated by Claude Code |
|
|
||
| def income_totals | ||
| @income_totals ||= income_statement.income_totals(period: period) | ||
| @income_totals ||= income_statement.income_totals(period: period, for_budget: true) |
There was a problem hiding this comment.
Budget#actual_income bypasses this private helper — asymmetric with actual_spending / estimated_income.
# line 294
def actual_income
family.income_statement.income_totals(period: self.period).total
endactual_spending (line 230) goes through net_totals → for_budget: true and correctly excludes one-time. actual_income calls the public income_totals directly, hits the new default for_budget: false → REPORT_EXCLUDED_KINDS → one-time income is now included.
Concrete effect: estimated_income is the median (no one-time). A one-time bonus inflates actual_income, actual_income_percent, remaining_expected_income, surplus_percent. Same-magnitude one-time expense does not show in actual_spending. This is the same defect the PR fixes — just on the income side.
Fix — route through the private memoized helper so it matches actual_spending:
def actual_income
income_totals.total
endAlso worth adding a Budget-level test pinning both actual_spending and actual_income against a kind: "one_time" transaction. Would catch this and future drift.
…ping Route Budget#actual_income through budget-scoped income_totals, update report exclusion comments, harden excluded_kinds_sql quoting, and add budget tests.
dd99336 to
0afb039
Compare
superagent-security
Bot
removed
contributor:verified
Signed-off-by: glorydavid03023 <glorydavid03023@gmail.com>
gariasf
left a comment
gariasf
left a comment
There was a problem hiding this comment.
Reviewed locally — the logic is correct and well-designed, but there's a blocker: a test fails on the current head.
Failing test: test/models/income_statement_test.rb → "memoizes expense and income period totals across repeated calculations" (≈ line 42).
The mocks are:
income_statement.expects(:build_period_total).with(classification: "expense", period: period)…
income_statement.expects(:build_period_total).with(classification: "income", period: period)…but build_period_total now always receives for_budget:, so the expectations never match:
unexpected invocation: build_period_total(classification: "expense", period: …, for_budget: false)
unsatisfied: build_period_total(classification: "expense", period: …)
Result: 1 failure / 58 runs. (The PR rewrote the one-time tests but missed this memoization mock — the [x] income_statement_test.rb box no longer holds.)
Fix: add for_budget: false to both .with(...) expectations.
Everything else checks out:
- The
BUDGET_EXCLUDED_KINDS = REPORT_EXCLUDED_KINDS + %w[one_time]superset is the right call — the untouched median path (FamilyStats/CategoryStats) referencesBUDGET_EXCLUDED_KINDS, so medians correctly keep excluding one-time. Verified. - Cache key bump
v2 → v3+kinds_keycleanly separates budget vs report totals. actual_income→ budget-scopedincome_totalsis a good consistency fix.- Reports and Budgets both render fine in the browser.
Minor: dead local key = period_cache_key(period) left in expense_totals (income_totals correctly dropped it).
Signed-off-by: glorydavid03023 <glorydavid03023@gmail.com>
…tations
CI test_unit failures were all introduced by this branch's own changes:
- IncomeStatement#build_period_total gained a `for_budget:` kwarg, but two
new memoization tests (IncomeStatementTest and PagesControllerTest) still
mocked build_period_total with the old `(classification:, period:)`
signature, so the real `for_budget: false` call matched no expectation
("invoked never"). Updated both mocks to expect `for_budget: false`.
- "reuses totals_query for income and expense in the same period" expected a
single totals_query call, but build_period_total fetched totals directly and
ran the query once per classification (2 calls). The branch already added a
`totals_for_period` helper for exactly this sharing but never used it, and it
ignored the budget/report kind scoping. Wired build_period_total through
totals_for_period and parameterized it by `for_budget` (memoized per
period+scope), so income and expense reuse one query while budget vs report
exclusions stay correct. No change to computed totals.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gariasf
left a comment
gariasf
left a comment
There was a problem hiding this comment.
Re-reviewed after 666710c2. The blocker from my last review is resolved: both mock sites now pass for_budget: false (including the pages_controller_test.rb one I had missed), and CI is fully green on head, test_unit and test_system included.
The fix also went further than the test alignment, and the refactor is a real improvement: build_period_total now routes through a shared totals_for_period(period, for_budget:), so income and expense for the same period+scope share one fetch. I checked the things that could have gone wrong with it:
- All four instance memo layers are keyed
[period_cache_key, for_budget]consistently (@expense_totals_by_period,@income_totals_by_period,@net_category_totals_by_period,@totals_for_period), so budget and report totals can't cross-contaminate. totals_for_periodhas exactly one caller, and it builds the sametransactions_scopethe inline version used. No semantic drift.- Call sites land where they should: dashboard, reports, and the AI assistant function get report scope; budget internals all pass
for_budget: true;actual_incomegoes through the private helper now. - The median path (
FamilyStats/CategoryStats) still referencesBUDGET_EXCLUDED_KINDS, so one-time stays out of medians. - The budget-level regression test I asked for is in (
budget_test.rb), pinning bothactual_spendingandactual_incomeagainst one-time noise, with the memo-busting reload. Thanks for that. - Bonus:
Totals#excluded_kinds_sqlswitching from raw interpolation toconnection.quoteis a nice hardening touch.
Two small leftovers inline, neither blocking: the dead local from my last review survived, and the non-English locales now describe the old behavior. Worth a final cosmetic commit, but I'm approving as is.
| # Memoized per instance so callers that also invoke `net_category_totals` | ||
| key = period_cache_key(period) |
There was a problem hiding this comment.
Leftover from my last review: key is assigned but never used (the memo builds its own cache_key below), and the comment above it ends mid-sentence.
| # Memoized per instance so callers that also invoke `net_category_totals` | |
| key = period_cache_key(period) | |
| # Memoized per instance so repeat callers (e.g. net_category_totals) reuse the same build |
| activity_type_description: Type of investment activity (Buy, Sell, Dividend, etc.). Auto-detected or set manually. | ||
| one_time_title: One-time %{type} | ||
| one_time_description: One-time transactions will be excluded from certain budgeting calculations and reports to help you see what's really important. | ||
| one_time_description: One-time transactions appear in historical reports and totals but are excluded from budget medians and recurring spending estimates. |
There was a problem hiding this comment.
The English copy is now correct, but the other locales (es, ca, de, hu, pl, and friends) still say one-time transactions are "excluded from certain budgeting calculations and reports", which describes the pre-PR behavior and is now wrong for non-English users. Worth updating them in the same pass, per the repo's i18n guideline of keeping locale files in sync with changed copy.
Signed-off-by: glorydavid03023 <glorydavid03023@gmail.com>
4 participants
Fixes #1994
Summary
Transaction::REPORT_EXCLUDED_KINDS(transfers + CC payments only)expense_totals/income_totalsinclude one-time transactionsfor_budget: trueperiod totals still exclude one-timeBudget#actual_incomenow uses budget-scopedincome_totals(matchesactual_spending)Test plan
bin/rails test test/models/income_statement_test.rbbin/rails test test/models/budget_test.rb:457Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
Tests