fix: Custom Report Total Mode not showing offbudget transactions #3627 #3633
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger 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 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
|
UnderKoen
changed the title
WalkthroughThe pull request introduces significant updates across several files to improve the handling of uncategorized entities in the reporting features of the application. A new type, Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🔇 Additional comments (5)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/desktop-client/src/components/reports/spreadsheets/recalculate.ts (2)
54-54: LGTM: Enhanced filtering logicPassing
groupsByCategorytofilterHiddenItemsis a good improvement. It allows the filtering logic to consider the new grouping condition, which is in line with the PR objective.Consider adding a comment explaining the purpose of this new parameter for better code readability:
filterHiddenItems( item, assets, showOffBudget, showHiddenCategories, showUncategorized, - groupsByCategory, + groupsByCategory, // Consider category/categoryGroup grouping in filtering )
59-60: LGTM: Improved handling of uncategorized itemsThe updated filtering condition correctly addresses the PR objective by including uncategorized items when grouping by category or category group. This should resolve the issue with offbudget transactions and uncategorized entries not showing up in reports.
Consider adding a type guard or null check for
item.uncategorized_idto improve type safety:asset => asset.date === intervalItem && - (asset[groupByLabel] === (item.id ?? null) || - (item.uncategorized_id && groupsByCategory)), + (asset[groupByLabel] === (item.id ?? null) || + (item.uncategorized_id !== undefined && item.uncategorized_id !== null && groupsByCategory)),packages/desktop-client/src/components/reports/spreadsheets/filterHiddenItems.ts (1)
24-46: Consider adding unit tests for the new filtering logicThe addition of the
groupByCategoryparameter introduces new logic paths within thefilterHiddenItemsfunction. To ensure all scenarios are handled correctly and to prevent future regressions, it's recommended to add unit tests covering:
- Different values of
uncategorized_id('off_budget','transfer','other','all', andnull).- Various combinations of
queryproperties (hasCategory,isOffBudget,isTransfer).- Both cases where
groupByCategoryistrueandfalse.Would you like assistance in creating unit tests for this function or setting up coverage reports to ensure all new logic paths are tested?
packages/desktop-client/src/components/reports/ReportOptions.ts (3)
239-239: Consider Makinguncategorized_ida Required PropertySince
uncategorized_idis assigned in all instances ofUncategorizedEntity, consider making it a required property instead of optional. This ensures that the property is always present, enhancing type safety.Apply this diff to make
uncategorized_idrequired:-export type UncategorizedEntity = Pick< - 'id' | 'name' | 'hidden' | 'cat_group' -> & { - uncategorized_id?: UncategorizedId; +export type UncategorizedEntity = Pick< + 'id' | 'name' | 'hidden' | 'cat_group' +> & { + uncategorized_id: UncategorizedId;Also applies to: 241-241
268-268: Consider Makinguncategorized_idRequired inUncategorizedGroupEntityIf
uncategorized_idis always assigned forUncategorizedGroupEntity, consider making it a required property to maintain consistency and improve type safety.Apply this diff to make
uncategorized_idrequired:-type UncategorizedGroupEntity = Pick< - 'name' | 'id' | 'hidden' -> & { - categories?: UncategorizedEntity[]; - uncategorized_id?: UncategorizedId; +type UncategorizedGroupEntity = Pick< + 'name' | 'id' | 'hidden' +> & { + categories?: UncategorizedEntity[]; + uncategorized_id: UncategorizedId;
330-335: Simplify Redundant Property Assignments After SpreadingAfter spreading
...group, reassigningid,name, andhiddenis redundant because these properties are already included. Simplify the return statement to improve code readability.Apply this diff to simplify the code:
return { ...group, - id: group.id, - name: group.name, - hidden: group.hidden, };Alternatively, if you intend to select specific properties, you can destructure
groupand return only the needed properties.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/desktop-client/src/components/reports/ReportOptions.ts (3 hunks)
- packages/desktop-client/src/components/reports/spreadsheets/custom-spreadsheet.ts (3 hunks)
- packages/desktop-client/src/components/reports/spreadsheets/filterHiddenItems.ts (1 hunks)
- packages/desktop-client/src/components/reports/spreadsheets/recalculate.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (13)
packages/desktop-client/src/components/reports/spreadsheets/recalculate.ts (2)
46-47: LGTM: Grouping logic enhancementThe introduction of
groupsByCategoryvariable is a good addition. It clearly defines when the grouping is based on category or category group, which aligns with the PR objective to fix issues related to grouping by category.
Line range hint
1-124: Overall assessment: Implementation addresses the reported issuesThe changes made to the
recalculatefunction effectively address the PR objectives, particularly in fixing issues related to offbudget transactions and uncategorized entries when grouped by category. The introduction ofgroupsByCategoryand the updates to filtering logic should resolve the reported problems.To ensure the changes work as expected, consider adding or updating unit tests to cover the following scenarios:
- Grouping by category with uncategorized items
- Grouping by category group with offbudget transactions
- Edge cases with various combinations of
showOffBudget,showHiddenCategories, andshowUncategorizedAdditionally, perform manual testing with the steps provided in the original issue #3627 to verify that the bug has been resolved.
🧰 Tools
🪛 Biome
[error] 62-62: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/desktop-client/src/components/reports/spreadsheets/filterHiddenItems.ts (3)
12-12: Addition ofgroupByCategoryparameter looks goodThe introduction of the optional
groupByCategoryparameter to thefilterHiddenItemsfunction effectively enhances its flexibility, allowing for additional grouping logic based on categories.
15-21: Refined filtering logic improves clarityThe updates to the filtering logic using chained
.filter()methods improve readability and maintain the clarity of the criteria being applied. This makes the code easier to understand and maintain.
24-46: Ensure all cases are covered in the filtering logicThe switch statement effectively handles different cases of
uncategorized_id, ensuring that items are correctly filtered based on their uncategorized type. This enhances the correctness of the filtering process whengroupByCategoryistrue.packages/desktop-client/src/components/reports/spreadsheets/custom-spreadsheet.ts (4)
148-149: Correctly determine grouping by category or category groupThe
groupsByCategoryvariable is accurately set based on thegroupByLabel, effectively identifying whether the data is grouped by category or category group. This aids in appropriate data processing later in the code.
173-174: Inclusion of uncategorized assets when grouping by category is correctly implementedThe condition ensures that uncategorized assets are included in the results when grouping by category or category group. This addresses the issue where off-budget transactions and uncategorized entries were previously not displayed.
190-191: Correctly include uncategorized debts in groupingThe updated condition appropriately includes uncategorized debts when the data is grouped by category or category group, ensuring comprehensive reporting of all relevant transactions.
168-168: EnsurefilterHiddenItemsfunction handlesgroupsByCategoryparameterVerify that the
filterHiddenItemsfunction has been updated to accept the newgroupsByCategoryparameter and that it properly utilizes this parameter to filter items based on the grouping criteria.Run the following script to confirm that the
filterHiddenItemsfunction signature includesgroupsByCategoryas a parameter:packages/desktop-client/src/components/reports/ReportOptions.ts (4)
235-236: Definition ofUncategorizedIdEnhances Type SafetyThe
UncategorizedIdtype is well-defined and provides better type safety for uncategorized entities.
247-247: Assignment ofuncategorized_idIs Consistent and CorrectThe
uncategorized_idproperties inuncategorizedCategory,transferCategory, andoffBudgetCategoryare correctly assigned using the newly definedUncategorizedIdtype.Also applies to: 253-253, 259-259
275-275:uncategorized_idinuncategorizedGroupAssigned AppropriatelyThe
uncategorized_idproperty inuncategorizedGroupis correctly set to'all', aligning with the newUncategorizedIdtype.
Line range hint
290-298: Ensure Sorting Logic Handles Undefined Values SafelyThe sorting function relies on properties like
is_incomeandsort_order, which may beundefined. To prevent potential runtime errors, ensure these properties are defined for all categories and groups or adjust the logic to handleundefinedvalues gracefully.Consider enhancing the code to handle undefined values:
return categories.list.sort((a, b) => { - return a.sort_order && b.sort_order && catGroupA && catGroupB + return (a.sort_order !== undefined && b.sort_order !== undefined && catGroupA && catGroupB) ? Number(catGroupA.is_income ?? 0) - Number(catGroupB.is_income ?? 0) || (catGroupA.sort_order ?? 0) - (catGroupB.sort_order ?? 0) || a.sort_order - b.sort_order : 0; });To verify that all categories and groups have
sort_orderandis_incomedefined, you can review the data initialization or run tests to ensure noundefinedvalues are present.
packages/desktop-client/src/components/reports/spreadsheets/recalculate.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/filterHiddenItems.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/reports/spreadsheets/filterHiddenItems.ts (1)
24-45: New groupByCategory logic effectively addresses PR objectivesThe added logic for
groupByCategorysuccessfully handles different cases of uncategorized items, addressing the PR's main objective. This implementation should correctly display off-budget transactions, transfers, and uncategorized entries when grouped by category.However, there's a minor suggestion for improvement:
Consider logging or throwing an error in the default case of the switch statement, as it currently silently filters out items with unexpected
uncategorized_idvalues. This could help with debugging and ensuring data integrity. For example:default: - return false; + console.warn(`Unexpected uncategorized_id: ${item.uncategorized_id}`); + return false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3633.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/reports/spreadsheets/filterHiddenItems.ts (1 hunks)
- packages/desktop-client/src/components/reports/spreadsheets/recalculate.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/reports/spreadsheets/recalculate.ts
🧰 Additional context used
🔇 Additional comments (3)
packages/desktop-client/src/components/reports/spreadsheets/filterHiddenItems.ts (3)
12-12: LGTM: Function signature updated appropriatelyThe addition of the optional
groupByCategoryparameter aligns well with the PR objective to address issues related to grouping by category. This change maintains backward compatibility while allowing for new functionality.
15-21: Improved filtering logic enhances clarity and functionalityThe restructured filtering logic now explicitly handles category hiding, off-budget accounts, and uncategorized items. This change improves readability and directly addresses the issues mentioned in the PR objectives, particularly the handling of off-budget transactions.
Line range hint
1-47: Overall changes successfully address PR objectivesThe modifications to the
filterHiddenItemsfunction effectively address the issues raised in PR #3627. The function now correctly handles:
- Off-budget transactions
- Transfers
- Uncategorized entries
- Grouping by category
These changes should resolve the reported problems with custom reports, particularly in the Total Mode. The code is well-structured and maintainable, with clear logic for different filtering scenarios.
To ensure the changes are correctly implemented across the codebase, please run the following verification script:
This script will help verify that the
filterHiddenItemsfunction is used correctly throughout the codebase and that thegroupByCategoryparameter is passed where necessary.✅ Verification successful
Verification Successful: All concerns addressed
The
filterHiddenItemsfunction is correctly utilized across the codebase, and thegroupByCategoryparameter is consistently passed where necessary. Additionally, there are no remainingTODOorFIXMEcomments related to "report," "filter," "offbudget," or "uncategorized."These findings confirm that the modifications effectively resolve the issues outlined in PR #3627. The code is both robust and maintainable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of filterHiddenItems function and its parameters # Test: Search for all usages of filterHiddenItems echo "Usages of filterHiddenItems:" rg --type typescript "filterHiddenItems\(" -A 3 # Test: Check if groupByCategory is passed in relevant places echo "\nUsages of groupByCategory:" rg --type typescript "groupByCategory" # Test: Look for any remaining TODOs or FIXMEs related to this issue echo "\nRelated TODOs or FIXMEs:" rg --type typescript -i "TODO|FIXME" -C 2 | rg -i "report|filter|offbudget|uncategorized"Length of output: 482
Script:
#!/bin/bash # Description: Verify the usage of filterHiddenItems function and its parameters by searching TypeScript files # Test: Search for all usages of filterHiddenItems echo "Usages of filterHiddenItems:" rg "filterHiddenItems\(" -g "*.ts" -g "*.tsx" -A 3 # Test: Check if groupByCategory is passed in relevant places echo "\nUsages of groupByCategory:" rg "groupByCategory" -g "*.ts" -g "*.tsx" # Test: Look for any remaining TODOs or FIXMEs related to this issue echo "\nRelated TODOs or FIXMEs:" rg -i "TODO|FIXME" -C 2 "report|filter|offbudget|uncategorized" -g "*.ts" -g "*.tsx"Length of output: 3762
Script:
#!/bin/bash # Description: Verify the usage of filterHiddenItems function and its parameters by searching TypeScript files # Test: Search for all usages of filterHiddenItems echo "Usages of filterHiddenItems:" rg "filterHiddenItems\(" -g "*.ts" -g "*.tsx" -A 3 # Test: Check if groupByCategory is passed in relevant places echo "\nUsages of groupByCategory:" rg "groupByCategory" -g "*.ts" -g "*.tsx" # Test: Look for any remaining TODOs or FIXMEs related to this issue echo "\nRelated TODOs or FIXMEs:" rg -i "(TODO|FIXME).*?(report|filter|offbudget|uncategorized)" -C 2 -g "*.ts" -g "*.tsx"Length of output: 3688
Script:
#!/bin/bash # Description: Verify the usage of filterHiddenItems function and its parameters by searching TypeScript files # Test: Search for all usages of filterHiddenItems echo "Usages of filterHiddenItems:" rg "filterHiddenItems\(" -g "*.ts" -g "*.tsx" -A 3 # Test: Check if groupByCategory is passed in relevant places echo "\nUsages of groupByCategory:" rg "groupByCategory" -g "*.ts" -g "*.tsx" # Test: Look for any remaining TODOs or FIXMEs related to this issue echo "\nRelated TODOs or FIXMEs:" rg -i '(TODO|FIXME).*?(report|filter|offbudget|uncategorized)' -C 2 -g "*.ts" -g "*.tsx"Length of output: 3688
|
This is really clean - thanks for tidying up my mess. It definitely addresses the issue and makes the code a lot easier to read. Can you check this? mode=total, split=category, type=net, options=tick all options on and switch to table graph - The "Uncategorized & Off Budget" group is showing 0s. Also, these categories only show when the "Show empty rows" Option is ticked, but that should not be the case. |
|
@carkom fixxed that! |
fixes #3627
Offbudget, transfers and uncategorized works again on the group by category