fix(reports): exclude investment_contribution from expense totals (#1750)

[Rene0422]

Summary

Closes #1750.

When a user explicitly marks a transaction as a Transfer to an investment account, the outflow leg gets the investment_contribution kind. The Reports page was still counting that leg as an expense, inflating the spending total. The reporter's case (Mutual Fund / Other Investment, self-hosted, transaction explicitly marked as Transfer with From/To fields) reproduces the bug exactly.

Root cause

Transaction::BUDGET_EXCLUDED_KINDS listed funds_movement, one_time, and cc_payment — the kinds that get filtered out of expense totals. investment_contribution was deliberately not in the list, and the inline comment said so:

"loan_payment and investment_contribution are intentionally NOT here — they represent real cash outflow from a budgeting perspective."

That's one valid framing, but it conflicts with the user's mental model (they classified the transaction as a transfer, not an expense) and with how regular funds_movement transfers are already handled. Tax-advantaged investment accounts (Roth IRA, 401k, HSA) are already excluded by a separate path (tax_advantaged_account_ids), which shows the system already recognises "money into an investment account is not necessarily spending."

This PR flips the choice for investment_contribution only. loan_payment keeps its current treatment — reducing loan principal is a different conversation and wasn't reported.

Changes

• app/models/transaction.rb — BUDGET_EXCLUDED_KINDS now includes investment_contribution. Comment rewritten to explain the rationale and to call out that loan_payment is intentionally still treated as an expense.
• app/models/income_statement/totals.rb — both SQL queries had CASE WHEN at.kind IN ('investment_contribution', 'loan_payment') THEN 'expense' … to force investment_contribution to classify as expense regardless of entry sign. Since it's now filtered out by the WHERE NOT IN clause before reaching the CASE, the branch is dead code; simplified to CASE WHEN at.kind = 'loan_payment' (also in the matching GROUP BY).
• test/models/income_statement_test.rb — flipped both "includes investment_contribution … as expenses" tests to assert exclusion. One covers an outflow on the source side; the other covers a provider-imported negative-amount inflow on the destination side. The negative-sign case is important because it confirms the kind is the load-bearing signal, not the entry sign.

Propagation / call sites checked

The change propagates automatically through every site that already filters on BUDGET_EXCLUDED_KINDS:

• BudgetCategoriesController (budget category totals)
• ReportsController#build_transactions_breakdown
• ReportsController#build_transactions_breakdown_for_export
• ReportsController#build_monthly_breakdown_for_export

So the breakdown table, CSV export, and monthly breakdown all stay consistent with the totals.

Transaction::Search uses TRANSFER_KINDS (not BUDGET_EXCLUDED_KINDS) and already excluded investment_contribution from the "expense" filter — test/models/transaction/search_test.rb:262 locks that in. No change needed there.

Tax-advantaged account exclusion is independent (it filters on accounts.id NOT IN (:tax_advantaged_account_ids)) and is unaffected; its tests use standard-kind transactions.

Test plan

• bin/rails test test/models/income_statement_test.rb
• bin/rails test test/models/transaction/search_test.rb
• bin/rails test test/controllers/reports_controller_test.rb
• bin/rails test
• bin/rubocop -f github -a
• bundle exec erb_lint ./app/**/*.erb -a
• bin/brakeman --no-pager
• Manual repro per the issue: create an Investment account (Mutual Fund / Other subtype), mark a transaction as "Transfer to [Investment account]", open Reports → confirm the transfer no longer appears in the expense total. Confirm it still appears in the Transactions list under the Transfer filter.

Summary by CodeRabbit

• Bug Fixes

  • Investment contribution transactions are excluded from income statement and budget analytics; only loan payments remain a special-case for classification and totals.

• Tests

  • Tests updated to verify investment contributions do not change transaction counts or income/expense totals and to cover provider-imported contribution cases.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR adds investment_contribution to the budget-exclusion constant, removes its forced-expense treatment from IncomeStatement totals SQL (leaving only loan_payment as a forced expense), and updates tests to assert investment_contribution is excluded from income-statement totals.

Changes

Investment contribution exclusion from budget analytics

Layer / File(s)	Summary
Budget exclusion constant update app/models/transaction.rb	BUDGET_EXCLUDED_KINDS is updated to include investment_contribution; enum/docs amended to state this exclusion and note loan_payment remains handled specially.
Income statement query classification logic app/models/income_statement/totals.rb	transactions_only_query_sql and transactions_subquery_sql remove investment_contribution from forced-expense CASE logic (leaving only loan_payment) and align GROUP BY clauses with the revised classification expression.
Test coverage for exclusion behavior test/models/income_statement_test.rb	Tests updated to assert investment_contribution transactions are excluded from income-statement totals, including provider-imported inflow cases for investment accounts.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• #1313: Matches the same objective of excluding investment_contribution from budget/report expense calculations and removing CASE-based expense forcing.

Possibly related PRs

• we-promise/sure#874: Related adjustments affecting loan_payment classification that interact with the updated CASE logic.
• we-promise/sure#987: Prior adjustments to totals SQL and budget exclusion logic touching the same areas.
• we-promise/sure#1588: Related changes to using BUDGET_EXCLUDED_KINDS for budget drilldowns and filtering.

Suggested labels

pr:verified

Suggested reviewers

• jjmata
• luckyPipewrench

Poem

🐰 I hopped through ledgers, coins in tow,
Found contributions that shouldn't show,
Loans still count, investments stay light,
Totals now tally what’s true and right—
A tidy report, and a happy night.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: excluding investment_contribution from expense totals in reports, directly addressing the core fix in the PR.
Linked Issues check	✅ Passed	The PR fully addresses issue #1750 by adding investment_contribution to BUDGET_EXCLUDED_KINDS, removing it from expense classification logic, and updating tests to verify exclusion from Reports.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the investment_contribution exclusion issue; no unrelated modifications to other features or systems.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/transaction.rb`:
- Around line 83-90: Update the stale enum comment for the transaction kind
`investment_contribution` so it matches current behavior where such transactions
are excluded from budgets via BUDGET_EXCLUDED_KINDS and handled by
IncomeStatement::Totals (i.e., not treated as a budget expense); locate the
enum/kind documentation near the transaction kind definitions in transaction.rb
(where `investment_contribution` is declared) and rewrite the inline comment to
state that investment_contribution is excluded from budget calculations and
handled separately by tax‑advantaged account logic.

🪄 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: b9513e9f-2ae8-4646-92b6-9b807d719bd4

📥 Commits

Reviewing files that changed from the base of the PR and between e0537a4 and dd636a2.

📒 Files selected for processing (3)

• app/models/income_statement/totals.rb
• app/models/transaction.rb
• test/models/income_statement_test.rb

[Rene0422]

@jjmata please review this PR, and let me know the result.

[jjmata]

Code review — PR #1999

The fix is well-reasoned and the implementation is clean. A few things worth considering before merge:

---

✅ What looks good

• Root cause is clearly identified and the decision to change BUDGET_EXCLUDED_KINDS instead of patching SQL is the right abstraction point — every downstream consumer gets the fix for free.
• Simplifying the CASE WHEN at.kind IN ('investment_contribution', 'loan_payment') to CASE WHEN at.kind = 'loan_payment' in both queries plus the GROUP BY is logically correct once investment_contribution rows are pre-filtered by the WHERE NOT IN clause.
• The second test (negative-amount inflow on the investment-account side) is a good edge-case lock — it confirms the kind signal takes precedence over the sign of the entry, which is important for provider-imported data.
• Propagation analysis across BudgetCategoriesController, ReportsController, and the export paths is thorough.

---

⚠️ Things worth discussing

1. loan_payment asymmetry

The PR explicitly calls this out but I think it deserves explicit sign-off: after this change, a transfer to an investment account is "not an expense", but a transfer to a loan account is still "an expense". Both move money between accounts the user owns. The distinction makes sense from a net-worth perspective (loan repayments reduce a liability, so they're directionally real cash flow), but from a cash-flow budgeting perspective users may expect symmetry. Fine to keep as-is, but worth confirming the team is aligned so a follow-up issue (#1313 per CodeRabbit) doesn't create confusion.

2. Interaction with PR #1995

PR #1995 (open, ~13 h older) also touches reports and one_time filtering. If both land close together, there's a risk of merge conflicts in income_statement/totals.rb or diverging test fixtures. Suggest rebasing whichever lands second on top of the other before merge.

3. One-sided provider import edge case

The second test creates only the investment-account leg of a contribution (investment_contribution, -$500). In practice, when a provider imports a payroll-deducted 401k contribution it often creates only the destination leg (inflow to the investment account) — there is no corresponding checking-account outflow. This PR correctly excludes that leg from expenses.

However, if a user also manually tagged the payroll line as a transfer (creating an investment_contribution outflow on the checking side), both legs are now excluded, which is also correct. The case that could silently break is if the checking-account outflow was imported as a plain non-investment_contribution transaction (e.g. kind nil, treated as a regular expense) before this deploy. After deploy, the investment-account inflow gets excluded but the checking-account outflow is still counted as an expense — so expenses would be double-counted in the opposite direction for those existing records. This is an edge case, but worth verifying: does PlaidEntry::Processor always set investment_contribution on both legs of such a transfer, or only on the destination leg?

---

Overall this is a solid fix for the reported issue. The three points above are questions/risks rather than blockers — happy to see this land once the loan_payment decision is confirmed intentional and the Plaid processor question is answered.

---

Generated by Claude Code

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/models/income_statement_test.rb (1)

305-320: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Test name contradicts assertions — rename or remove.

The test is named "includes investment_contribution transactions as expenses..." but the assertions at lines 317-319 verify that investment_contribution is excluded (transaction count stays at 4, expense stays at 900). The duplicate test at lines 322-340 correctly names this behavior.

Either rename this test to reflect exclusion or remove it since lines 322-340 cover the same scenario with correct naming.

Suggested fix: rename to match behavior

-  test "includes investment_contribution transactions as expenses in income statement" do
+  test "excludes investment_contribution transactions from income statement totals (outflow leg)" do

Or remove this test entirely and keep only lines 322-340.

🤖 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 `@test/models/income_statement_test.rb` around lines 305 - 320, The test named
"includes investment_contribution transactions as expenses..." contradicts its
assertions (it actually asserts exclusion) — either rename the test string to
reflect exclusion (e.g., "excludes investment_contribution transactions from
income statement") or remove this duplicate test; locate the block that calls
create_transaction(..., kind: "investment_contribution") and constructs
IncomeStatement.new(`@family`) and adjust the test name or delete the entire test
to avoid duplicating the correct test that asserts exclusion.

🤖 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 `@test/models/income_statement_test.rb`:
- Line 321: Remove the trailing whitespace in the test file by editing the
IncomeStatementTest (income_statement_test.rb) and deleting the extra space
characters at the end of the flagged line so the line ends cleanly with no
trailing spaces; save the file and re-run static checks.

---

Outside diff comments:
In `@test/models/income_statement_test.rb`:
- Around line 305-320: The test named "includes investment_contribution
transactions as expenses..." contradicts its assertions (it actually asserts
exclusion) — either rename the test string to reflect exclusion (e.g., "excludes
investment_contribution transactions from income statement") or remove this
duplicate test; locate the block that calls create_transaction(..., kind:
"investment_contribution") and constructs IncomeStatement.new(`@family`) and
adjust the test name or delete the entire test to avoid duplicating the correct
test that asserts exclusion.

🪄 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: c390a87d-2b1d-433f-b3de-ed890bb348cf

📥 Commits

Reviewing files that changed from the base of the PR and between 398d3b4 and 525df92.

📒 Files selected for processing (3)

• app/models/income_statement/totals.rb
• app/models/transaction.rb
• test/models/income_statement_test.rb

🚧 Files skipped from review as they are similar to previous changes (1)

• app/models/transaction.rb

[zachryall]

Hey @Rene0422 , looks like you have done some good work on this. Any idea when it might get merged in, ready for a release?

[rickdgray]

Please add this change. I would love to turn this off. My Sankey is totally broken.

[sure-design]

Triage note: CI is green and the scope is tight, but @jjmata's review questions from 2026-05-27 (loan_payment/investment_contribution asymmetry sign-off, potential conflict with #1995, and whether Plaid tags both legs of a contribution as investment_contribution) appear unanswered by the author. Meanwhile several users (including one reporting a broken Sankey chart) have asked for this to be merged. Flagging for maintainer attention since it's otherwise ready modulo those open questions.

---

Generated by Claude Code

[sure-design]

This fix is clean and CI is green, but jjmata's review from 05-27 raised three questions that don't appear to have been answered: is the loan_payment vs investment_contribution asymmetry intentional/signed-off, does this conflict with #1995 (which also touches income_statement/totals.rb), and does Plaid ever tag only one leg of a contribution as investment_contribution (which could double-count in the other direction)? A few users are now asking for this to merge (one with a broken Sankey chart) — answering these would unblock it.

---

Generated by Claude Code