Have budget category reflect income and expenses #315
Conversation
WalkthroughUpdated Budget model logic: per-category and overall spending are now computed as net totals (total incomes − total expenses); the methods return 0 when net is non-negative and the absolute value when net is negative. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Controller/Consumer
participant Budget as Budget model
participant DB as Transactions/Records
rect `#DFF3E0`
Caller->>Budget: request actual_spending / budget_category_actual_spending
end
Budget->>DB: fetch income_totals (filtered)
Budget->>DB: fetch expense_totals (filtered)
DB-->>Budget: income_totals, expense_totals
rect `#FFF3D9`
Note over Budget: compute net = income_totals.total - expense_totals.total
end
alt net >= 0
Budget-->>Caller: return 0
else net < 0
Budget-->>Caller: return net.abs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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: 0
🧹 Nitpick comments (1)
app/models/budget.rb (1)
162-167: Consider optimizing for loop usage to avoid O(n×m) lookups.The logic correctly implements per-category net spending, but since this method is called in a loop at line 140 (
to_donut_segments_json), the repeatedfindoperations oncategory_totalsarrays could be inefficient when there are many budget categories.Consider building hash maps once and reusing them:
def budget_category_actual_spending(budget_category, expense_map: nil, income_map: nil) expense_map ||= expense_totals.category_totals.index_by { |ct| ct.category.id } income_map ||= income_totals.category_totals.index_by { |ct| ct.category.id } total_expenses = expense_map[budget_category.category.id]&.total || 0 total_incomes = income_map[budget_category.category.id]&.total || 0 total = total_incomes - total_expenses total.negative? ? total.abs : 0 endThen update line 140 to pass the maps:
expense_map = expense_totals.category_totals.index_by { |ct| ct.category.id } income_map = income_totals.category_totals.index_by { |ct| ct.category.id } segments = budget_categories.map do |bc| { color: bc.category.color, amount: budget_category_actual_spending(bc, expense_map: expense_map, income_map: income_map), id: bc.id } end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/budget.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/models/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-design.mdc)
Domain models should not call Provider::Registry directly; use a Provided concern within the model’s namespace to select providers and expose convenience methods
Use ActiveRecord validations for forms and complex domain constraints.
app/models/**/*.rb: Place business logic in POROs and model classes under app/models
Models should answer questions about themselves (e.g., prefer account.balance_series over service objects)
Implement complex validations and business logic with ActiveRecord validations
Model-level validations may mirror DB constraints but are not strictly required
Files:
app/models/budget.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Place Rails application Ruby code (models, controllers, services, jobs, mailers, components) under app/
Files:
app/models/budget.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby style: 2-space indentation; snake_case for methods/variables; CamelCase for classes/modules
Files:
app/models/budget.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{rb,js,jsx,ts,tsx}: Make changes atomic, testable, and explain their impact briefly in code suggestions.
Respect existing tests and add tests when changing critical logic.
Files:
app/models/budget.rb
app/{helpers,models}/**/*.rb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, and dates on the server side (Ruby) before sending to the client
Files:
app/models/budget.rb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/models/budget.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: sokie
Repo: we-promise/sure PR: 276
File: app/controllers/reports_controller.rb:159-169
Timestamp: 2025-11-04T15:16:23.341Z
Learning: In the Budget model, the methods `actual_spending` and `allocated_spending` return BigDecimal values (not Money objects), so division operations like `budget.actual_spending / budget.allocated_spending` work correctly without needing conversion to fractional units.
📚 Learning: 2025-11-04T15:16:23.341Z
Learnt from: sokie
Repo: we-promise/sure PR: 276
File: app/controllers/reports_controller.rb:159-169
Timestamp: 2025-11-04T15:16:23.341Z
Learning: In the Budget model, the methods `actual_spending` and `allocated_spending` return BigDecimal values (not Money objects), so division operations like `budget.actual_spending / budget.allocated_spending` work correctly without needing conversion to fractional units.
Applied to files:
app/models/budget.rb
📚 Learning: 2025-09-16T13:15:56.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-09-16T13:15:56.406Z
Learning: Applies to app/models/{entry,valuation,transaction,trade}.rb : Entry amounts are signed: negative = inflow, positive = outflow (applies to Entry and its types: Valuation, Transaction, Trade)
Applied to files:
app/models/budget.rb
🧬 Code graph analysis (1)
app/models/budget.rb (3)
app/models/income_statement.rb (2)
income_totals(33-35)expense_totals(29-31)lib/money/arithmetic.rb (2)
negative?(49-51)abs(41-43)app/models/budget_category.rb (1)
category(44-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (1)
app/models/budget.rb (1)
157-160: Net spending implementation verified—logic is sound across all dependent calculations.The semantic change from gross to net spending is correctly reflected throughout:
actual_spending: Returns expense overage only (net = income - expenses, clamped to ≥0) ✓available_to_spend: Still correctly computed asbudgeted - actual_spending✓percent_of_budget_spent: Correctly divides net spending by budgeted amount ✓overage_percent: Safely handles negative available_to_spend scenario (guards with.negative?check before division) ✓reports_controllerusage: Division of two numeric types works correctly ✓Type mixing via
|| 0fallbacks is safe—themonetizewrapper (line 14) handles conversion automatically.Verify test coverage: No test files were found in the repository. Please manually confirm that existing tests pass with the net spending calculation and that any tests covering the semantic change are in place.
jjmata
changed the title
app/models/budget.rb
Outdated
|
|
||
| def actual_spending | ||
| expense_totals.total | ||
| total = income_totals.total - expense_totals.total |
There was a problem hiding this comment.
Wouldn't this also include salary income? Needs to only take "expense income" for this to work correctly, @Esteban-Bermudez.
There was a problem hiding this comment.
Current behavior with PR:
income_totals.total = $5,000 (salary) + $200 (healthcare refund) = $5,200
expense_totals.total = $500 (healthcare) + $300 (groceries) = $800
actual_spending = $5,200 - $800 = $4,400 (positive)
# Returns: 0 (because total is positive)
Expected behavior:
actual_spending should be $600 ($500 healthcare + $300 groceries - $200 refund)
There was a problem hiding this comment.
I will remove the check for 0, as it appears the caller is checking for a negative value, so there is no need to include it in the method.
And yes, I see what you mean that the income totals would drastically change the value of spending, and it does not make sense to include income in spending.
I can revert this behaviour, however I am curious to know how the actual_spending is used in other calculations and view?
There was a problem hiding this comment.
@jjmata The main thing I could see this affecting is if the 200 refund in your example is included in the income_totals or the expense_totals?
For example, if it is in income_total, then the actual_spending will be 800 even though it is 600 due to the reimbursement.
I guess we would need to account for returns/income in expense categories, but not account for income categories like salary.
Currently, budget categories will only check if your transaction expenses exceed the threshold and do not take into account the income (gross spending vs net spending). This change will allow the user to see how much they have spent, taking into account situations like healthcare reimbursements or product returns.
Esteban-Bermudez
force-pushed
the
esteban/have-budget-categories-reflect-combined-income-and-expenses
branch
from
94eec93 to
a4230df
Compare
|
@jjmata Would you be able to take a look at this again with my changes? |
2 participants
Closes issue #314
Currently, budget categories will only check if your transaction expenses exceed the threshold and do not take into account the income (gross spending vs net spending). This change will allow the user to see how much they have spent, taking into account situations like healthcare reimbursements or product returns.
This might not be the best implementation of this, so I am happy to make changes for what works best for this calculation.
We resolve to a 0 value when the value is positive, as this means there is only an income.
Summary by CodeRabbit