Fix rules not checking "category is nothing" when reconciling #3572
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe changes in this pull request primarily involve modifications to the Assessment against linked issues
Possibly related PRs
Suggested labels
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/accounts/sync.ts (2)
299-299: LGTM: Consistent handling of category for bank sync transactionsThis change ensures that bank sync transactions include the
categoryproperty, maintaining consistency with regular transactions.For consistency with the
normalizeTransactionsfunction, consider moving this assignment to a separate line:- category: trans.category || null, + category: null, + }, + }); + normalized[normalized.length - 1].trans.category = trans.category || null;This change would make it easier to add any future normalization logic for the
categoryproperty.
Line range hint
380-380: LGTM: Proper handling of category during reconciliationThis change ensures that the
categoryis included when updating existing transactions during reconciliation. The logic correctly prioritizes the existing category, then the new transaction's category, and finally defaults tonull.For improved clarity, consider using a ternary operator:
- category: existing.category || trans.category || null, + category: existing.category ? existing.category : (trans.category || null),This change makes it more explicit that we're prioritizing the existing category over the new one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3572.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/loot-core/src/server/accounts/sync.ts (2 hunks)
🔇 Additional comments (3)
packages/loot-core/src/server/accounts/sync.ts (3)
241-242: LGTM: Ensures category is always setThis change ensures that all transactions have a
categoryproperty, even if it'snull. This addresses the core issue where rules were not checking 'category is nothing' when reconciling transactions.
Line range hint
724-726: LGTM: No changes needed for addTransactionsThe
addTransactionsfunction doesn't require direct changes related to thecategoryproperty. Thecategoryis already being handled correctly in thenormalizeTransactionsfunction, which is called at the beginning ofaddTransactions. The existing logic preserves thecategoryproperty when creating thefinalTransactionobject.
Line range hint
1-1004: Overall assessment: Changes effectively address the PR objectiveThe modifications in this file consistently handle the
categoryproperty across different functions involved in transaction synchronization and normalization. These changes effectively address the PR objective of fixing the issue where rules were not checking 'category is nothing' when reconciling transactions.Key improvements:
normalizeTransactions: Ensures all transactions have acategoryproperty, even if it'snull.normalizeBankSyncTransactions: Maintains consistency in handling thecategoryproperty for bank sync transactions.reconcileTransactions: Properly includes thecategorywhen updating existing transactions during reconciliation.The implementation is consistent and maintains backwards compatibility. These changes should resolve the issue described in the linked bug report (#2540) where the 'Category is (nothing)' rule was not being respected during account synchronization.
Bundle Stats — desktop-clientHey 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
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey 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
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
davidmartos96
changed the title
Co-authored-by: Matt Fiddaman <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/accounts/sync.ts (1)
Line range hint
1-793: Overall, the changes improve category handling, but a few points need attention.The modifications consistently handle the new
categoryproperty across different functions, addressing the PR objectives. However, consider addressing the following points:
- In the
reconcileTransactionsfunction, consider adding logic to preserve manually set categories that differ from imported ones.- In the
addTransactionsfunction, either implement the usage of the newcategoryparameter or remove it if it's not intended to be used.These adjustments will further enhance the robustness and clarity of the category handling implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3572.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/loot-core/src/server/accounts/sync.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/loot-core/src/server/accounts/sync.ts (4)
241-242: LGTM! Nullish coalescing implemented as suggested.The change ensures that
trans.categoryis always set to either its existing value ornull, addressing the issue mentioned in the PR objectives. This implementation also follows the previous review suggestion to use nullish coalescing.
299-299: LGTM! Consistent handling of category property.The addition of the
categoryproperty to bank sync transactions, with nullish coalescing to default tonull, is consistent with the changes made in thenormalizeTransactionsfunction. This ensures uniform handling of the category across different transaction sources.
Line range hint
686-686: LGTM, but the newcategoryparameter is unused.The addition of the
categoryparameter to theaddTransactionsfunction signature is consistent with the other changes for handling categories. However, it appears that this new parameter is not utilized within the function body.Consider one of the following actions:
- Remove the
categoryparameter if it's not intended to be used.- Implement the logic to use the
categoryparameter within the function.To verify the usage of the
categoryparameter, we can search for its occurrences within theaddTransactionsfunction:
Line range hint
391-391: LGTM, but consider preserving manually set categories.The addition of the
categoryproperty to theupdatesobject ensures proper category handling during reconciliation. The fallback chain (existing category → transaction category → null) is a good approach. However, consider the following:
- This logic might overwrite manually set categories with imported ones.
- It may be beneficial to add a check to preserve manually set categories, especially if they differ from the imported category.
To verify the potential impact on manually set categories, we can check for instances where categories might be overwritten:
Fixes #2540
The call to
runRulespasses a transaction that doesn't include a category (aka undefined), so the rule checking "category is nothing" doesn't work.My solution is to provide a category or null in both
normalizeBankSyncTransactionsandnormalizeTransactionsThis is my first dig into the source of the project, but hopefully this solution or similar is valid.