Skip to content

Do not count hidden categories in tracking budgets + cleanup table math - #4567

Merged
lelemm merged 28 commits into
actualbudget:masterfrom
lelemm:budget-nohidden-alt-version
Apr 16, 2025
Merged

Do not count hidden categories in tracking budgets + cleanup table math#4567
lelemm merged 28 commits into
actualbudget:masterfrom
lelemm:budget-nohidden-alt-version

Conversation

@lelemm

@lelemm lelemm commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

This changes the budget table calculations to exclude hidden categories from totals. To test, hide and unhide categories and groups and compare the group and budget totals.

This also changes the summation for balance and spending to include all amounts that month regardless of a category balance being carried forward. The balance will still carry to the next month for now.

fixes #4116
resolves #2400

@netlify

netlify Bot commented Mar 7, 2025

Copy link
Copy Markdown

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit a61df34
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/68001ef1cb305f0008ba0f7b
😎 Deploy Preview https://deploy-preview-4567.demo.actualbudget.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions

github-actions Bot commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
18 9.01 MB → 9.01 MB (+95 B) +0.00%
Changeset
File Δ Size
locale/en.json 📈 +95 B (+0.08%) 111.04 kB → 111.13 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/en.js 111.04 kB → 111.13 kB (+95 B) +0.08%

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/en-GB.js 5.92 kB 0%
static/js/de.js 120.88 kB 0%
static/js/es.js 66.77 kB 0%
static/js/fr.js 125.94 kB 0%
static/js/nl.js 101.15 kB 0%
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/workbox-window.prod.es5.js 5.69 kB 0%
static/js/uk.js 125.2 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/pt-BR.js 119.46 kB 0%
static/js/resize-observer.js 18.37 kB 0%
static/js/useAccountPreviewTransactions.js 1.69 kB 0%
static/js/AppliedFilters.js 10.87 kB 0%
static/js/wide.js 112.65 kB 0%
static/js/narrow.js 389.39 kB 0%
static/js/ReportRouter.js 1.65 MB 0%
static/js/index.js 5.94 MB 0%

@github-actions

github-actions Bot commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
1 2.29 MB → 2.29 MB (+3.43 kB) +0.15%
Changeset
File Δ Size
packages/loot-core/src/server/budget/report.ts 📈 +5.73 kB (+153.79%) 3.73 kB → 9.46 kB
packages/loot-core/src/server/budget/envelope.ts 📈 +5.25 kB (+86.84%) 6.05 kB → 11.3 kB
packages/loot-core/src/server/budget/base.ts 📉 -4.83 kB (-32.03%) 15.08 kB → 10.25 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
kcab.worker.js 2.29 MB → 2.29 MB (+3.43 kB) +0.15%

Smaller

No assets were smaller

Unchanged

No assets were unchanged

@youngcw

youngcw commented Mar 8, 2025

Copy link
Copy Markdown
Member

Working for everything except income categories. Hiding/unhiding those is broken

Comment thread packages/loot-core/src/server/budget/base.ts Outdated
@youngcw

youngcw commented Mar 8, 2025

Copy link
Copy Markdown
Member

@lelemm @psybers Want to test this out? I think its all working as expected now

@youngcw

youngcw commented Mar 8, 2025

Copy link
Copy Markdown
Member

/update-vrt

@youngcw

youngcw commented Mar 8, 2025

Copy link
Copy Markdown
Member

Looks like I broke the envelope budget math

@lelemm
lelemm marked this pull request as ready for review March 8, 2025 18:46
@lelemm lelemm changed the title Alternative solution for Do not count hidden categories in tracking budgets [WIP] Alternative solution for Do not count hidden categories in tracking budgets Mar 8, 2025
@coderabbitai

coderabbitai Bot commented Mar 8, 2025

Copy link
Copy Markdown
Contributor

"""

Walkthrough

The changes involve significant modifications to the budget management system across multiple files. In base.ts, the createCategory function is now exported, while the createCategoryGroup function and category/group change handlers were removed. The handling of category and category group changes is now delegated based on the budget type to either the envelope.ts or report.ts modules. In envelope.ts, new functions handleCategoryChange, handleCategoryGroupChange, and createCategoryGroup were added to manage dependencies and dynamic sheet creation for category groups. Similarly, in report.ts, the createSummary function was updated to use groups instead of categories for expense calculations, and new functions were added to handle category and group changes, including visibility and tombstone status. The test suite in base.test.ts was extended with tests verifying that hidden categories and groups are excluded from totals in report budgets but included in rollover budgets. These changes modularize category and group handling by budget type and introduce correct handling of hidden entities in budget calculations.

Assessment against linked issues

Objective Addressed Explanation
Fix incorrect totals in spent and balance columns for rollover budgets (#4116) Refactored logic separates rollover budget handling and includes tests verifying correct totals.
Exclude hidden categories from totals in tracking (report) budgets (#2400) Tests confirm hidden categories/groups are excluded from totals in report budgets but included in rollover budgets.

Suggested labels

:sparkles: Merged
"""

Tip

⚡💬 Agentic Chat (Pro Plan, General Availability)
  • We're introducing multi-step agentic chat in review comments and issue comments, within and outside of PR's. This feature enhances review and issue discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments and add commits to existing pull requests.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9ab42fd and a61df34.

📒 Files selected for processing (1)
  • packages/loot-core/src/server/budget/base.ts (5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Visual regression
  • GitHub Check: Functional
  • GitHub Check: build (ubuntu-latest)
🔇 Additional comments (6)
packages/loot-core/src/server/budget/base.ts (6)

41-41: LGTM! Exposing the createCategory function is appropriate.

The function is now properly exported, allowing it to be used by other modules while maintaining the existing functionality.


150-151: LGTM! Good variable extraction.

Extracting the budget type into a variable before using it in multiple conditions improves code readability.


176-184: Good refactoring to handle category changes by budget type.

This change appropriately delegates category change handling to specialized modules based on the budget type, which supports the PR objective of excluding hidden categories from report budget totals while including them in rollover budgets.


186-194: Good refactoring to handle category group changes by budget type.

Similar to the category changes, this appropriately delegates category group change handling to the specialized modules based on budget type, maintaining consistency in the refactoring approach.


255-259: Proper delegation of category group creation based on budget type.

This change aligns with the overall refactoring approach, creating category groups through the appropriate module based on the budget type.


270-270:

Details

✅ Verification successful

Signature change for report.createSummary

The report.createSummary function now only requires the groups and sheetName parameters, suggesting it's been refactored to calculate totals differently. This supports the PR objective of handling hidden categories in report budgets.


🏁 Script executed:

#!/bin/bash
# Verify that the report.createSummary function signature matches this call
rg -A 5 "createSummary.*\(" packages/loot-core/src/server/budget/report.ts

Length of output: 286


Verified createSummary signature

The report.createSummary function in packages/loot-core/src/server/budget/report.ts is declared as:

export function createSummary(groups, sheetName) {  }

which exactly matches the call in base.ts:

report.createSummary(groups, sheetName);

This aligns with the refactor for handling hidden categories in budget reports. Approving these changes.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@lelemm

lelemm commented Mar 8, 2025

Copy link
Copy Markdown
Contributor Author

@youngcw I moved the new logic to ./report.ts and moved the old logic to ./envelope.ts this way we can avoid changing envelop logic.
I did some testing with report. Looks like working. Still need to test envelope

@lelemm lelemm changed the title [WIP] Alternative solution for Do not count hidden categories in tracking budgets Do not count hidden categories in tracking budgets - Alternative solution Mar 8, 2025
@kevingatera

Copy link
Copy Markdown
Contributor

Yes those last 2 are already happening but untested afaik. I will open a PR against your branch @lelemm

@youngcw youngcw added this to the v25.5.0 milestone Apr 6, 2025
youngcw
youngcw previously approved these changes Apr 16, 2025
Add tests for handling hidden categories and groups in budget totals
@lelemm

lelemm commented Apr 16, 2025

Copy link
Copy Markdown
Contributor Author

@youngcw added the tests here

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/loot-core/src/server/budget/base.test.ts (1)

68-339: Consider adding tests for the reverse visibility change.

The current tests verify behavior when changing from hidden to visible, but don't test the opposite - when categories or groups change from visible to hidden. Adding these tests would provide more complete coverage.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between af79220 and 9ab42fd.

📒 Files selected for processing (1)
  • packages/loot-core/src/server/budget/base.test.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Visual regression
  • GitHub Check: Functional
🔇 Additional comments (4)
packages/loot-core/src/server/budget/base.test.ts (4)

68-142: Well-structured test for hidden categories in Report Budget!

This test effectively verifies that hidden categories are excluded from group totals in Report Budget, and that toggling visibility correctly updates the totals. This aligns with the PR objective to exclude hidden categories from budget totals.


144-229: Good coverage of hidden category groups behavior in Report Budget.

This test properly verifies that hidden category groups are excluded from total budget calculations in the Report Budget type. The test clearly demonstrates the toggling behavior, showing that previously hidden amounts are included when made visible.


231-281: Good test for Rollover Budget hidden categories behavior.

This test effectively confirms that in Rollover Budget, hidden categories are still included in group totals, contrasting with the Report Budget behavior. This distinction is key to the PR objectives.


283-339: Comprehensive test for hidden groups in Rollover Budget.

The test properly verifies that hidden category groups remain included in budget totals for Rollover Budget type, maintaining the expected behavior distinction between budget types.

@youngcw

youngcw commented Apr 16, 2025

Copy link
Copy Markdown
Member

@lelemm if you think this is ready then we can call it good and merge it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Spent/Balance Column Totals [Feature] Don't have hidden categories count toward tracking budgets

3 participants