Fix budget cache invalidation after transaction deletion

[sure-admin]

Summary

• invalidate entry aggregation caches when an older transaction entry is deleted
• simplify Family#entries_cache_version to include both entry count and latest updated_at
• add a regression test covering deletion of an older entry

Tests

• bin/rails test test/models/family_test.rb test/controllers/transactions/bulk_deletions_controller_test.rb test/models/budget_test.rb

Summary by CodeRabbit

• Bug Fixes
  • Improved cache version tracking so changes to entries, including deletions of older entries, are detected reliably.
• Tests
  • Added coverage verifying that removing an older entry updates the family’s cache version.

[coderabbitai]

📝 Walkthrough

Walkthrough

Family#entries_cache_version now recomputes its value from the current entry count and latest update timestamp. A model test verifies that deleting an older entry changes the cache version.

Changes

Family entry cache invalidation

Layer / File(s)	Summary
Recompute entry cache versions app/models/family.rb, test/models/family_test.rb	entries_cache_version no longer memoizes a timestamp-only value; it combines the current entry count with the latest updated_at, and the test verifies deletion changes the result.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: erkdgn

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% 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 reflects the main change: fixing cache invalidation when transactions are deleted.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/budget-cache-delete

---

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.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 392d264524

ℹ️ 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".

[chatgpt-codex-connector]

Avoid two full-ledger scans per cache lookup

When a report spans multiple periods, every distinct IncomeStatement#totals_query cache lookup invokes this method; removing memoization makes each invocation issue separate COUNT and MAX queries across all entries in the family. For example, the monthly loop in ReportsController#build_trends_data now adds two family-wide aggregate queries per month even when every report result is already cached, so long-range reports for large ledgers can become substantially slower or time out. Preserve deletion-aware invalidation without recomputing both aggregates for every cache fetch, such as by maintaining a mutation-driven version or safely reusing one aggregate snapshot.

Useful? React with 👍 / 👎.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

app/models/family.rb (1)

432-432: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Avoid two aggregate queries per cache-version read.

This now issues separate COUNT and MAX(updated_at) queries on every invocation, while IncomeStatement reads the version for multiple cache keys. Combine the aggregates into one query or reuse a maintained invalidation token. As per coding guidelines, database access in models should be optimized.

🤖 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/family.rb` at line 432, Optimize the cache-version calculation in
the surrounding Family model method so each invocation does not issue separate
COUNT and MAX(updated_at) aggregate queries. Fetch both aggregate values with
one database query, or reuse an existing maintained invalidation token, while
preserving the current version format and behavior.

Source: Coding guidelines

🤖 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/family.rb`:
- Line 432: Replace the cache-key fragment using entries.count and maximum
updated_at with a family-level invalidation token that changes on every entry
create, update, and destroy. Update the relevant Family and entry lifecycle
logic so IncomeStatement cache keys always change after any material entry
mutation, including edits to non-latest entries.

---

Nitpick comments:
In `@app/models/family.rb`:
- Line 432: Optimize the cache-version calculation in the surrounding Family
model method so each invocation does not issue separate COUNT and
MAX(updated_at) aggregate queries. Fetch both aggregate values with one database
query, or reuse an existing maintained invalidation token, while preserving the
current version format and behavior.

🪄 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 Plus

Run ID: 973399fb-6e70-4807-bebe-e34796fd2816

📥 Commits

Reviewing files that changed from the base of the PR and between 1fb5202 and 392d264.

📒 Files selected for processing (2)

• app/models/family.rb
• test/models/family_test.rb

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Invalidate on every material entry mutation.

entries.count plus the maximum updated_at is unchanged when a non-latest entry is edited, such as changing its amount, category, or date. Because IncomeStatement uses this value in cache keys, stale aggregation results can be served. Use a family-level invalidation token/timestamp updated on every entry create, update, and destroy, or another mechanism with that guarantee.

🤖 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/family.rb` at line 432, Replace the cache-key fragment using
entries.count and maximum updated_at with a family-level invalidation token that
changes on every entry create, update, and destroy. Update the relevant Family
and entry lifecycle logic so IncomeStatement cache keys always change after any
material entry mutation, including edits to non-latest entries.

[jjmata]

Reviewed the diff (app/models/family.rb + test/models/family_test.rb).

Correctness: This does fix a real gap. The old entries_cache_version only tracked entries.maximum(:updated_at), so deleting an entry that wasn't the most-recently-updated one left the max timestamp unchanged and the cache key identical — stale budget/income-statement figures after deletion. Folding entries.count into the version string catches that case correctly (count changes on any create/destroy, regardless of which entry). Creates/updates already worked before since updated_at is monotonic, so the fix is correctly scoped to the actual bug rather than over-engineering a new invalidation token.

Test coverage: test "entries_cache_version changes when an older entry is destroyed" (family_test.rb:168-186) is well-targeted — it specifically constructs the scenario where the destroyed entry is not the one with the max updated_at, which is exactly the case the old code got wrong. Good use of fixtures per repo convention.

One concern worth addressing: dropping the @entries_cache_version ||= memoization means every call now issues two aggregate queries (COUNT + MAX) instead of reusing a cached value. entries_cache_version is called from multiple sites per request — e.g. income_statement.rb:230,237,253 (three separate call sites, each invoked per interval/period) and investment_statement.rb:315 — so a single page render could now hit the DB 6+ times where it previously hit it once per Family instance. CodeRabbit's automated review already flagged this same point; worth combining into one query (e.g. entries.pick(Arel.sql("COUNT(*), MAX(updated_at)"))) or re-memoizing now that the key includes count, since Family instances are typically scoped to a single request via Current.family.

Otherwise this looks correct, minimal, and in line with repo conventions (fat model, no new deps, no i18n surface).

---

Generated by Claude Code