feat: fix categories being set on offbudget transactions #3705

UnderKoen · 2024-10-21T14:55:49Z

No description provided.

netlify · 2024-10-21T14:56:06Z

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	`f1e92ff`
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/6716c41ec064d90008e369be
😎 Deploy Preview	https://deploy-preview-3705.demo.actualbudget.org
📱 Preview on mobile	Toggle 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 · 2024-10-21T14:58:29Z

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
9	5.32 MB → 5.32 MB (+237 B)	+0.00%

Changeset

File	Δ	Size
`node_modules/es-define-property/index.js`	📈 +210 B (+60.00%)	350 B → 560 B
`node_modules/has-property-descriptors/index.js`	📈 +9 B (+1.57%)	573 B → 582 B
`node_modules/call-bind/index.js`	📈 +9 B (+0.85%)	1.04 kB → 1.05 kB
`node_modules/define-data-property/index.js`	📈 +9 B (+0.39%)	2.24 kB → 2.25 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
static/js/index.js	3.33 MB → 3.33 MB (+237 B)	+0.01%

Smaller

No assets were smaller

Unchanged

Asset	File Size	% Changed
static/js/usePreviewTransactions.js	1.64 kB	0%
static/js/indexeddb-main-thread-worker-e59fee74.js	13.5 kB	0%
static/js/AppliedFilters.js	21.3 kB	0%
static/js/resize-observer.js	18.37 kB	0%
static/js/BackgroundImage.js	122.29 kB	0%
static/js/narrow.js	82.57 kB	0%
static/js/wide.js	232.12 kB	0%
static/js/ReportRouter.js	1.51 MB	0%

youngcw · 2024-10-21T14:58:30Z

This will be fine for now, but another thing that would be good would be for transactions to get stripped of categories when being added to off budget accounts. People occasionally get categories is their offbudget accounts and it causes issues.

github-actions · 2024-10-21T14:58:30Z

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	1.27 MB → 1.27 MB (+121 B)	+0.01%

Changeset

File	Δ	Size
`packages/loot-core/src/server/accounts/transactions.ts`	📈 +222 B (+4.00%)	5.42 kB → 5.63 kB
`packages/loot-core/src/server/accounts/transfer.ts`	📈 +137 B (+2.23%)	5.99 kB → 6.13 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
kcab.worker.js	1.27 MB → 1.27 MB (+121 B)	+0.01%

Smaller

No assets were smaller

Unchanged

No assets were unchanged

coderabbitai · 2024-10-21T16:16:54Z

Caution

Review failed

The head commit changed during the review from 2b239f6 to f1e92ff.

Walkthrough

The pull request modifies the batchUpdateTransactions function in the transactions.ts file, enhancing the logic for handling transaction categories based on account status. The updates include a revised approach for setting the category property of transactions when accounts are marked as off-budget, ensuring that categories are cleared appropriately during both the addition and update of transactions. The function now retrieves account information at the start, which reduces redundant database queries. While the batch processing of transactions remains intact, the handling of added transactions has been optimized to incorporate the new category assignment checks. Additionally, the clearCategory function in the transfer.ts file has been updated to nullify the category of associated transactions under certain conditions. Other functions related to transfers have been adjusted to maintain the integrity of transaction relationships without altering their core logic.

Possibly related PRs

Fix rules not checking "category is nothing" when reconciling #3572: This PR modifies the normalizeTransactions and reconcileTransactions functions to ensure that the category property is properly initialized and updated, which is directly related to the changes made in the batchUpdateTransactions function regarding category assignment based on account status.

Suggested labels

:sparkles: Merged

Suggested reviewers

jfdoming

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

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

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)

packages/loot-core/src/mocks/budget.ts (1)
111-116: LGTM! Consider adding an else clause for clarity.

The changes effectively address the issue of adding categories to off-budget transactions. By wrapping the category assignment logic inside the if (!account.offBudget) condition, we ensure that categories are only assigned to on-budget accounts.

For improved readability, consider adding an explicit else clause to handle off-budget accounts:
 if (!account.offBudget) {
   if (payee.name === 'Deposit') {
     category = incomeGroup.categories.find(c => c.name === 'Income');
   } else {
     category = pickRandom(expenseCategories);
   }
+} else {
+  category = undefined;
 }
This makes it clear that off-budget accounts intentionally have no category assigned.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 715bc00 and 1f1dee9.

⛔ Files ignored due to path filters (1)

upcoming-release-notes/3705.md is excluded by !**/*.md

📒 Files selected for processing (1)

packages/loot-core/src/mocks/budget.ts (2 hunks)

🧰 Additional context used

🔇 Additional comments (2)

packages/loot-core/src/mocks/budget.ts (2)
137-137: Good use of optional chaining!

The use of optional chaining (category?.id) is a good practice here. It prevents potential errors when category is undefined for off-budget accounts, aligning well with the changes made earlier in the function.

Line range hint 111-137: Verify consistency across other account filling functions

The changes in fillPrimaryChecking effectively address the issue of adding categories to off-budget transactions. However, it's worth verifying if similar changes are needed in other account filling functions (e.g., fillChecking, fillSavings, etc.) to maintain consistency across the codebase.

✅ Verification successful

Consistency Verified Across Account Filling Functions

No other account filling functions requiring similar off-budget checks were found. The changes are isolated to fillPrimaryChecking, ensuring consistency within the codebase.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for other account filling functions that might need similar off-budget checks
rg -A 5 "function fill.*$handlers, account," --glob '!packages/loot-core/src/mocks/budget.ts'
Length of output: 96

Script:
#!/bin/bash
# Search for other account filling functions that include 'account' as a parameter
rg -A 5 "function fill\w*\(.*account.*$" --glob '!packages/loot-core/src/mocks/budget.ts'
Length of output: 92

UnderKoen · 2024-10-21T16:49:31Z

This will be fine for now, but another thing that would be good would be for transactions to get stripped of categories when being added to off budget accounts. People occasionally get categories is their offbudget accounts and it causes issues.

@youngcw Fixxed!

coderabbitai

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1f1dee9 and 49fd364.

⛔ Files ignored due to path filters (1)

upcoming-release-notes/3705.md is excluded by !**/*.md

📒 Files selected for processing (1)

packages/loot-core/src/server/accounts/transactions.ts (2 hunks)

🧰 Additional context used

packages/loot-core/src/server/accounts/transactions.ts

youngcw · 2024-10-21T17:05:55Z

sorry for the feature creep, but while we are here can we try to fix #2266? Its pretty similar.

UnderKoen · 2024-10-21T21:06:02Z

@youngcw Fixxed it!

youngcw · 2024-10-21T21:07:19Z

found a bug and I have not been able to recreate this bug on edge

Change an existing transaction to a transfer (Ive only used on budget accounts so far)
change the transaction back to not a transfer
Try setting the category of that transaction (the category will show what it used to be, but I think that is just a caching bug, it is actually uncategorized refresh the page to see that)
you will get an error

edit: Hah! looks like you also fixed this just now

packages/loot-core/src/server/accounts/transfer.ts

youngcw · 2024-10-21T21:22:16Z

I like how you started trying to fix the reports 😄

actual-github-bot bot changed the title ~~fix: test budget don't add categories to offbudget transactions~~ [WIP] fix: test budget don't add categories to offbudget transactions Oct 21, 2024

actual-github-bot bot added the 🚧 WIP label Oct 21, 2024

UnderKoen changed the title ~~[WIP] fix: test budget don't add categories to offbudget transactions~~ fix: test budget don't add categories to offbudget transactions Oct 21, 2024

actual-github-bot bot added 🔍 Ready for review and removed 🚧 WIP labels Oct 21, 2024

coderabbitai bot reviewed Oct 21, 2024

View reviewed changes

UnderKoen added 2 commits October 21, 2024 18:45

chore: release note

7105170

feat: fix categories being set on offbudget transactions

49fd364

UnderKoen force-pushed the fix/categories-on-offbudget branch from 1f1dee9 to 49fd364 Compare October 21, 2024 16:47

UnderKoen changed the title ~~fix: test budget don't add categories to offbudget transactions~~ feat: fix categories being set on offbudget transactions Oct 21, 2024

coderabbitai bot reviewed Oct 21, 2024

View reviewed changes

packages/loot-core/src/server/accounts/transactions.ts Show resolved Hide resolved

packages/loot-core/src/server/accounts/transactions.ts Outdated Show resolved Hide resolved

packages/loot-core/src/server/accounts/transactions.ts Show resolved Hide resolved

youngcw reviewed Oct 21, 2024

View reviewed changes

packages/loot-core/src/server/accounts/transactions.ts Outdated Show resolved Hide resolved

fix: actualbudget#2266

1059b17

fix: small mistake

2b239f6

youngcw reviewed Oct 21, 2024

View reviewed changes

packages/loot-core/src/server/accounts/transfer.ts Outdated Show resolved Hide resolved

chore: update comment

f1e92ff

youngcw linked an issue Oct 21, 2024 that may be closed by this pull request

[Bug]: Moving transactions between accounts can create transfers with unchangable categories #2266

Closed

5 tasks

youngcw approved these changes Oct 21, 2024

View reviewed changes

actual-github-bot bot added ✅ Approved and removed 🔍 Ready for review labels Oct 21, 2024

youngcw merged commit a91a885 into actualbudget:master Oct 21, 2024
20 checks passed

actual-github-bot bot added ✨ Merged and removed ✅ Approved labels Oct 21, 2024

This was referenced Oct 23, 2024

fix: Custom Report Total Mode not showing offbudget transactions #3627 #3633

Merged

Apply Template to All Categories in Group for Web #3666

Merged

UnderKoen deleted the fix/categories-on-offbudget branch October 24, 2024 06:49

coderabbitai bot mentioned this pull request Nov 3, 2024

Sub category groups #3772

Closed

coderabbitai bot mentioned this pull request Nov 26, 2024

Consistent accounts terminology (For budget / Budgeted --> On budget) #3903

Merged

coderabbitai bot mentioned this pull request Dec 11, 2024

Dkhalife/ts/categorytransactions #3959

Merged

This was referenced Dec 31, 2024

(typescript) Refactoring the mobile TransactionListWithBalance component into typescript. #4061

Merged

enhance: add ability to control category learning per payee and globally #4081

Merged

This was referenced Jan 21, 2025

Make loot-core compatible with exactOptionalPropertyTypes #4214

Open

Identify and fix broken transfers #4216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fix categories being set on offbudget transactions #3705

feat: fix categories being set on offbudget transactions #3705

UnderKoen commented Oct 21, 2024

netlify bot commented Oct 21, 2024 •

edited

Loading

github-actions bot commented Oct 21, 2024 •

edited

Loading

youngcw commented Oct 21, 2024

github-actions bot commented Oct 21, 2024 •

edited

Loading

coderabbitai bot commented Oct 21, 2024 •

edited

Loading

Review failed

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

coderabbitai bot left a comment

UnderKoen commented Oct 21, 2024

coderabbitai bot left a comment

youngcw commented Oct 21, 2024 •

edited

Loading

UnderKoen commented Oct 21, 2024

youngcw commented Oct 21, 2024 •

edited

Loading

youngcw commented Oct 21, 2024

feat: fix categories being set on offbudget transactions #3705

feat: fix categories being set on offbudget transactions #3705

Conversation

UnderKoen commented Oct 21, 2024

netlify bot commented Oct 21, 2024 • edited Loading

✅ Deploy Preview for actualbudget ready!

github-actions bot commented Oct 21, 2024 • edited Loading

Bundle Stats — desktop-client

youngcw commented Oct 21, 2024

github-actions bot commented Oct 21, 2024 • edited Loading

Bundle Stats — loot-core

coderabbitai bot commented Oct 21, 2024 • edited Loading

Review failed

Walkthrough

Possibly related PRs

Suggested labels

Suggested reviewers

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

UnderKoen commented Oct 21, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

youngcw commented Oct 21, 2024 • edited Loading

UnderKoen commented Oct 21, 2024

youngcw commented Oct 21, 2024 • edited Loading

youngcw commented Oct 21, 2024

netlify bot commented Oct 21, 2024 •

edited

Loading

github-actions bot commented Oct 21, 2024 •

edited

Loading

github-actions bot commented Oct 21, 2024 •

edited

Loading

coderabbitai bot commented Oct 21, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

youngcw commented Oct 21, 2024 •

edited

Loading

youngcw commented Oct 21, 2024 •

edited

Loading