Improve budget median: use last 6 months instead of all-time data#1056
Improve budget median: use last 6 months instead of all-time data#1056ChakibMoMi wants to merge 2 commits intowe-promise:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a 6-month lookback window to income-statement stats (wired into queries and cache keys) and updates budget daily-spending logic to use explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IncomeStatement as IncomeStatement Service
participant Cache
participant DB as Database
Client->>IncomeStatement: request family/category stats
IncomeStatement->>Cache: build cache key (includes stats_lookback_key)
Cache-->>IncomeStatement: miss / hit
alt cache miss
IncomeStatement->>IncomeStatement: compute lookback_start/end, visible_statuses
IncomeStatement->>DB: query transactions with lookback date bounds and status filter
DB-->>IncomeStatement: rows
IncomeStatement->>Cache: store serialized result with key
IncomeStatement-->>Client: return stats
else cache hit
Cache-->>IncomeStatement: cached result
IncomeStatement-->>Client: return stats
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![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: 49a0bcdc2d
ℹ️ 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".
| def lookback_start_date | ||
| STATS_LOOKBACK_MONTHS.months.ago.beginning_of_month.to_date |
There was a problem hiding this comment.
These lookback dates are derived from Date.current, so the stats query result changes automatically when the month rolls over, but IncomeStatement#family_stats/#category_stats cache entries are keyed only by family, interval, and entries_cache_version (no date component). That means a cache populated on the last day of a month can continue serving the previous 6-month window on the first day of the next month until any entry is updated, yielding stale medians/averages for budgeting.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/models/income_statement/family_stats.rb (1)
24-30:lookback_start_date/lookback_end_dateare duplicated verbatim inCategoryStatsThe two private helpers at lines 24–30 are copy-pasted identically into
IncomeStatement::CategoryStats(lines 23–29). Per the project's coding guidelines, shared logic should live in a Rails concern or PORO rather than being duplicated across classes.♻️ Suggested extraction into a shared module
# app/models/income_statement/stats_lookback.rb (new file) module IncomeStatement::StatsLookback STATS_LOOKBACK_MONTHS = 6 private def lookback_start_date STATS_LOOKBACK_MONTHS.months.ago.beginning_of_month.to_date end def lookback_end_date 1.month.ago.end_of_month.to_date end endThen in both
FamilyStatsandCategoryStats:+include IncomeStatement::StatsLookback -STATS_LOOKBACK_MONTHS = 6 # FamilyStats -STATS_LOOKBACK_MONTHS = IncomeStatement::FamilyStats::STATS_LOOKBACK_MONTHS # CategoryStats - -def lookback_start_date - STATS_LOOKBACK_MONTHS.months.ago.beginning_of_month.to_date -end - -def lookback_end_date - 1.month.ago.end_of_month.to_date -endAs per coding guidelines: "Use Rails concerns and POROs for organizing business logic instead of services folder."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/income_statement/family_stats.rb` around lines 24 - 30, Extract the duplicated lookback helpers into a shared module (e.g. IncomeStatement::StatsLookback) that defines STATS_LOOKBACK_MONTHS and the private methods lookback_start_date and lookback_end_date, then include that concern into both IncomeStatement::FamilyStats and IncomeStatement::CategoryStats and remove the duplicate method definitions from those classes so they use the shared implementations.app/models/income_statement/category_stats.rb (1)
2-2:STATS_LOOKBACK_MONTHSaliasing creates a cross-class load-order dependencyAliasing
IncomeStatement::FamilyStats::STATS_LOOKBACK_MONTHSat the class level meansCategoryStatscan only be loaded afterFamilyStats. While Zeitwerk normally handles this, the shared-concern extraction suggested forfamily_stats.rbwould eliminate the coupling entirely — both classes wouldinclude IncomeStatement::StatsLookbackand getSTATS_LOOKBACK_MONTHSfrom a single authoritative source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/income_statement/category_stats.rb` at line 2, Replace the class-level aliasing of STATS_LOOKBACK_MONTHS by extracting the constant into a shared module (e.g., define module IncomeStatement::StatsLookback with STATS_LOOKBACK_MONTHS) and have both IncomeStatement::FamilyStats and CategoryStats include IncomeStatement::StatsLookback; remove the direct reference/alias STATS_LOOKBACK_MONTHS = IncomeStatement::FamilyStats::STATS_LOOKBACK_MONTHS from CategoryStats and stop relying on IncomeStatement::FamilyStats for the constant so load-order coupling is eliminated.app/models/budget_category.rb (1)
160-166: Redundant guard on line 166 after the new inclusive range checkSince line 163 already guarantees
Date.current <= budget.end_date,days_remainingwill always be≥ 1((end_date - current).to_i + 1 ≥ 1). Thereturn nil unless days_remaining > 0on line 166 is now dead code.🧹 Proposed clean-up
- days_remaining = (budget.end_date - Date.current).to_i + 1 - return nil unless days_remaining > 0 - - { + days_remaining = (budget.end_date - Date.current).to_i + 1 + {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/budget_category.rb` around lines 160 - 166, The guard `return nil unless days_remaining > 0` is redundant because you already ensure Date.current is between budget.start_date and budget.end_date (inclusive) earlier; remove the redundant check and keep the days_remaining calculation (`days_remaining = (budget.end_date - Date.current).to_i + 1`) as-is so later code can use it without an unreachable early return. Ensure this change affects the method that references available_to_spend, budget.start_date, budget.end_date, and days_remaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/models/income_statement/category_stats.rb`:
- Around line 23-29: The lookback_start_date and lookback_end_date methods in
IncomeStatement::CategoryStats are duplicated from IncomeStatement::FamilyStats;
extract these two helpers into a shared module (e.g.,
IncomeStatement::LookbackHelpers or a Concern) and include it into both
IncomeStatement::CategoryStats and IncomeStatement::FamilyStats so both classes
call the same methods (lookback_start_date and lookback_end_date) from the
shared module instead of defining them twice.
- Around line 83-85: The SQL in CategoryStats is using hardcoded account status
strings ("draft","active") like FamilyStats; replace that literal list with the
shared source used in FamilyStats (e.g., the Account model constant/scope
referenced there) so both CategoryStats and FamilyStats derive allowed statuses
from the same symbol (for example Account::ALLOWED_STATUSES or
Account.active_and_draft scope) and update the query in CategoryStats (the
a.status IN (...) clause) to use that constant/scope or bound parameter instead
of hardcoded strings.
---
Nitpick comments:
In `@app/models/budget_category.rb`:
- Around line 160-166: The guard `return nil unless days_remaining > 0` is
redundant because you already ensure Date.current is between budget.start_date
and budget.end_date (inclusive) earlier; remove the redundant check and keep the
days_remaining calculation (`days_remaining = (budget.end_date -
Date.current).to_i + 1`) as-is so later code can use it without an unreachable
early return. Ensure this change affects the method that references
available_to_spend, budget.start_date, budget.end_date, and days_remaining.
In `@app/models/income_statement/category_stats.rb`:
- Line 2: Replace the class-level aliasing of STATS_LOOKBACK_MONTHS by
extracting the constant into a shared module (e.g., define module
IncomeStatement::StatsLookback with STATS_LOOKBACK_MONTHS) and have both
IncomeStatement::FamilyStats and CategoryStats include
IncomeStatement::StatsLookback; remove the direct reference/alias
STATS_LOOKBACK_MONTHS = IncomeStatement::FamilyStats::STATS_LOOKBACK_MONTHS from
CategoryStats and stop relying on IncomeStatement::FamilyStats for the constant
so load-order coupling is eliminated.
In `@app/models/income_statement/family_stats.rb`:
- Around line 24-30: Extract the duplicated lookback helpers into a shared
module (e.g. IncomeStatement::StatsLookback) that defines STATS_LOOKBACK_MONTHS
and the private methods lookback_start_date and lookback_end_date, then include
that concern into both IncomeStatement::FamilyStats and
IncomeStatement::CategoryStats and remove the duplicate method definitions from
those classes so they use the shared implementations.
ChakibMoMi
force-pushed
the
feature/bugfix-budget-median
branch
from
49a0bcd to
6b4858c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/models/income_statement.rb (1)
133-135:stats_lookback_keyduplicates the formula fromStatsLookback#lookback_start_date.If the lookback start formula in
StatsLookbackis ever updated,stats_lookback_keywill silently diverge, producing stale cache keys. Including the module eliminates the duplication:♻️ Proposed refactor
class IncomeStatement include Monetizable + include IncomeStatement::StatsLookback ... def stats_lookback_key - StatsLookback::STATS_LOOKBACK_MONTHS.months.ago.beginning_of_month.to_date.to_s + lookback_start_date.to_s end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/income_statement.rb` around lines 133 - 135, Replace the duplicated date formula in the stats_lookback_key method by delegating to the canonical implementation in StatsLookback (e.g. call StatsLookback#lookback_start_date or include the StatsLookback module) so the cache key is derived from that single source of truth; ensure you convert the returned value to the same string format (to_date.to_s) so stats_lookback_key returns an identical string as before.app/models/income_statement/stats_lookback.rb (1)
10-12: Consider documenting whylookback_end_datedoesn't referenceSTATS_LOOKBACK_MONTHS.
lookback_end_datealways returns1.month.ago.end_of_monthregardless ofSTATS_LOOKBACK_MONTHS. This is intentional — the window always ends at the last completed month andSTATS_LOOKBACK_MONTHSonly controls how far back the start reaches. A brief comment would prevent a future maintainer from "fixing" this by tying the end to the constant.📝 Suggested inline documentation
def lookback_end_date + # Always end at the last completed month, regardless of how far back the + # window extends — ensures the current in-progress month is never included. 1.month.ago.end_of_month.to_date end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/income_statement/stats_lookback.rb` around lines 10 - 12, Add a short inline comment above the lookback_end_date method clarifying that the end of the window is intentionally fixed to the last completed month (1.month.ago.end_of_month) and should not be tied to STATS_LOOKBACK_MONTHS because that constant only controls how far back the start date goes; reference the method name lookback_end_date and the constant STATS_LOOKBACK_MONTHS in the comment so future maintainers understand the intent.test/models/income_statement_test.rb (1)
231-260: Week-median comments assumebase_dateis a Monday; consider asserting exact values.The comments claim
week_median = 800(from groups[300, 1000, 800]), but PostgreSQLdate_trunc('week', ...)truncates to Monday. When2.months.ago.beginning_of_monthfalls on a Sunday,base_dateandbase_date + 1.dayland in different ISO weeks, changing the groupings and the resulting median. Therefute_equalassertion still holds in all cases (the monthly total 2100 can never equal a per-week median), but the specific numeric comments become misleading. Asserting both concrete values would make the test self-documenting and catch regressions:♻️ Stronger assertions (values valid when base_date is a Monday)
- refute_equal month_median, week_median, "Different intervals should return different statistical results when data spans multiple time periods" - - # Both should still be numeric - assert month_median.is_a?(Numeric) - assert week_median.is_a?(Numeric) - assert month_median > 0 - assert week_median > 0 + assert_equal 2100.0, month_median + assert_equal 800.0, week_medianIf the day-of-week variance is a concern, anchor
base_dateto a known Monday:- base_date = 2.months.ago.beginning_of_month.to_date + # Pin to the most recent Monday in a past completed month so week groupings are deterministic + raw = 2.months.ago.beginning_of_month.to_date + base_date = raw + ((1 - raw.wday) % 7) # advance to the first Monday of that month🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/income_statement_test.rb` around lines 231 - 260, The test's week grouping is flaky because base_date can fall on a Sunday and PostgreSQL date_trunc('week') groups by Monday; update the test so it is deterministic by anchoring base_date to a known weekday (e.g., force base_date to the first Monday of the month) or replace the loose refute_equal with concrete assertions for IncomeStatement#median_expense for both intervals (month and week) using the expected numeric medians; locate the test setup using base_date and the calls to create_transaction and IncomeStatement#median_expense to implement the change.app/models/income_statement/family_stats.rb (1)
72-74: Use the existingAccount.visiblescope instead of hardcoding status values.The Account model already defines a
visiblescope with identical status logic:where(status: [ "draft", "active" ]). For consistency and maintainability, reference this scope or extract the status values to a constant (similar toTransaction::BUDGET_EXCLUDED_KINDS) rather than duplicating hardcoded strings.Note: Account uses AASM state machine, not a Rails enum. The default status is
:active, so test accounts created without an explicit status won't be silently dropped by this filter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/income_statement/family_stats.rb` around lines 72 - 74, Replace the hardcoded status filter "a.status IN ('draft', 'active')" with the existing Account.visible scope (or use a shared constant for statuses) so we don't duplicate logic; locate the SQL fragment in family_stats.rb where the query uses the alias "a" and either switch this raw SQL to use Account.visible (or interpolate Account.visible.pluck(:status) / a constant) or refactor the query to join/filter via Account.visible to ensure consistent status logic with the Account model.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/models/income_statement/category_stats.rb`:
- Line 2: The SQL in IncomeStatement::CategoryStats currently hardcodes the
status strings 'draft' and 'active' (same issue as in FamilyStats); update the
SQL to reference the canonical status values instead of literals by using the
model/enum constants used in family_stats (e.g., the Category model's status
enum or a STATUS_DRAFT/STATUS_ACTIVE constants) and pass them as bound
parameters (or via Arel/ActiveRecord helpers) in the query generation method
inside IncomeStatement::CategoryStats (the method that builds the SQL for
lookback stats included via IncomeStatement::StatsLookback) so the code uses the
shared constants and avoids hardcoded strings.
---
Nitpick comments:
In `@app/models/income_statement.rb`:
- Around line 133-135: Replace the duplicated date formula in the
stats_lookback_key method by delegating to the canonical implementation in
StatsLookback (e.g. call StatsLookback#lookback_start_date or include the
StatsLookback module) so the cache key is derived from that single source of
truth; ensure you convert the returned value to the same string format
(to_date.to_s) so stats_lookback_key returns an identical string as before.
In `@app/models/income_statement/family_stats.rb`:
- Around line 72-74: Replace the hardcoded status filter "a.status IN ('draft',
'active')" with the existing Account.visible scope (or use a shared constant for
statuses) so we don't duplicate logic; locate the SQL fragment in
family_stats.rb where the query uses the alias "a" and either switch this raw
SQL to use Account.visible (or interpolate Account.visible.pluck(:status) / a
constant) or refactor the query to join/filter via Account.visible to ensure
consistent status logic with the Account model.
In `@app/models/income_statement/stats_lookback.rb`:
- Around line 10-12: Add a short inline comment above the lookback_end_date
method clarifying that the end of the window is intentionally fixed to the last
completed month (1.month.ago.end_of_month) and should not be tied to
STATS_LOOKBACK_MONTHS because that constant only controls how far back the start
date goes; reference the method name lookback_end_date and the constant
STATS_LOOKBACK_MONTHS in the comment so future maintainers understand the
intent.
In `@test/models/income_statement_test.rb`:
- Around line 231-260: The test's week grouping is flaky because base_date can
fall on a Sunday and PostgreSQL date_trunc('week') groups by Monday; update the
test so it is deterministic by anchoring base_date to a known weekday (e.g.,
force base_date to the first Monday of the month) or replace the loose
refute_equal with concrete assertions for IncomeStatement#median_expense for
both intervals (month and week) using the expected numeric medians; locate the
test setup using base_date and the calls to create_transaction and
IncomeStatement#median_expense to implement the change.
Budget statistics now use a 6-month lookback window for median/average calculations, producing more realistic budget suggestions based on recent spending patterns rather than all-time historical data. Changes: - Extract StatsLookback concern to DRY up lookback logic between FamilyStats and CategoryStats - Add lookback date to cache key so stats invalidate on month rollover - Filter stats queries to completed months within 6-month window - Filter to active/draft accounts only - Use Date.current.between? for budget period check - Fix budget_category daily spending to use budget period dates - Update tests to create transactions within the lookback window
ChakibMoMi
force-pushed
the
feature/bugfix-budget-median
branch
from
6b4858c to
5f74a91
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/models/income_statement/stats_lookback.rb (1)
4-5:VISIBLE_ACCOUNT_STATUSESduplicatesAccount.visiblescope — silent drift riskThe constant at line 4-5 exactly mirrors
Account.visible(line 21 of app/models/account.rb), which definesstatus: ["draft", "active"]. IfAccount.visiblegains a new status (e.g.,"syncing"), stats queries will silently exclude those accounts with no error or test failure.Safer approach: expose
Account::VISIBLE_STATUSESon the Account model and derive from it:♻️ Suggested refactor: single source of truth for account statuses
- # Matches Account.visible scope: where(status: ["draft", "active"]) - VISIBLE_ACCOUNT_STATUSES = %w[draft active].freeze - - ... - - def visible_account_statuses_sql - `@visible_account_statuses_sql` ||= VISIBLE_ACCOUNT_STATUSES.map { |s| "'#{s}'" }.join(", ") - end + def visible_account_statuses_sql + `@visible_account_statuses_sql` ||= + Account::VISIBLE_STATUSES.map { |s| ActiveRecord::Base.connection.quote(s) }.join(", ") + endThis requires adding
VISIBLE_STATUSES = %w[draft active].freezeto the Account model — eliminates the duplication and uses proper SQL quoting.Also applies to: 19-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/income_statement/stats_lookback.rb` around lines 4 - 5, The VISIBLE_ACCOUNT_STATUSES constant duplicates Account.visible and risks drifting; remove or replace VISIBLE_ACCOUNT_STATUSES in stats_lookback.rb and derive statuses from Account by referencing a single source like Account::VISIBLE_STATUSES (and update Account model to define VISIBLE_STATUSES = %w[draft active].freeze if not present) so queries use Account::VISIBLE_STATUSES instead of a hardcoded array; update any other occurrences (e.g., the similar block at lines 19-21) to use Account::VISIBLE_STATUSES to ensure a single source of truth.test/models/income_statement_test.rb (1)
67-78: The status filter concern is not an issue—Account.statusdefaults to"active"(confirmed in both the AASM state machine and database schema), which matches thevisiblescope filter of['draft', 'active']. The test setup will work correctly.However, explicitly setting
status: :activewhen creating@checking_accountin the test setup remains a good practice for clarity and defensive programming against future default changes:- `@checking_account` = `@family.accounts.create`! name: "Checking", currency: `@family.currency`, balance: 5000, accountable: Depository.new + `@checking_account` = `@family.accounts.create`! name: "Checking", currency: `@family.currency`, balance: 5000, accountable: Depository.new, status: :active🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/income_statement_test.rb` around lines 67 - 78, The test setup should explicitly set the checking account's status to active to make the intent clear and protect against future default changes; update the setup where `@checking_account` is created (the Account instantiation used by the test) to include status: :active so it clearly satisfies the visible scope and aligns with assertions in IncomeStatement#median_expense (used in the test), ensuring the created `@checking_account` is unambiguously active during the test run.
🤖 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/income_statement/stats_lookback.rb`:
- Around line 4-5: The VISIBLE_ACCOUNT_STATUSES constant duplicates
Account.visible and risks drifting; remove or replace VISIBLE_ACCOUNT_STATUSES
in stats_lookback.rb and derive statuses from Account by referencing a single
source like Account::VISIBLE_STATUSES (and update Account model to define
VISIBLE_STATUSES = %w[draft active].freeze if not present) so queries use
Account::VISIBLE_STATUSES instead of a hardcoded array; update any other
occurrences (e.g., the similar block at lines 19-21) to use
Account::VISIBLE_STATUSES to ensure a single source of truth.
In `@test/models/income_statement_test.rb`:
- Around line 67-78: The test setup should explicitly set the checking account's
status to active to make the intent clear and protect against future default
changes; update the setup where `@checking_account` is created (the Account
instantiation used by the test) to include status: :active so it clearly
satisfies the visible scope and aligns with assertions in
IncomeStatement#median_expense (used in the test), ensuring the created
`@checking_account` is unambiguously active during the test run.
PR Review: "Improve budget median: use last 6 months instead of all-time data" (#1056)SummaryThis PR introduces a 6-month lookback window for budget statistics (median/average expense and income calculations), replacing the previous all-time approach. It also improves the What's Good
Issues to Address1. SQL Injection Risk in
|
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Please see my last comment for a Claude Code review of this PR.
…estore guard - Replace manual SQL string interpolation in visible_account_statuses_sql with parameterized bind params (:visible_statuses) to prevent SQL injection - Derive visible statuses from Account.visible scope instead of hardcoded VISIBLE_ACCOUNT_STATUSES constant to avoid divergence - Restore days_remaining > 0 guard in budget_category#suggested_daily_spending as safety net against potential ZeroDivisionError - Remove unnecessary instance variable memoization in StatsLookback - Add test verifying transactions outside 6-month lookback window are excluded
2 participants
Budget statistics now use a 6-month lookback window for median/average calculations, producing more realistic budget suggestions based on recent spending patterns rather than all-time historical data.
Changes:
Summary by CodeRabbit
Bug Fixes
New Features
Tests