Fix Calendar Widget tracking transfers as income and expense #4143
Conversation
✅ 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
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 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
|
AntoineTA
changed the title
WalkthroughThe pull request introduces a modification to the transaction filtering logic in two components: the Calendar component and the calendar spreadsheet function. In both cases, a new filter condition is added to exclude transactions with a Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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: 0
🧹 Nitpick comments (1)
packages/desktop-client/src/components/reports/reports/Calendar.tsx (1)
206-206: LGTM! Consider adding a test case.The filter condition is correctly implemented and consistent with the spreadsheet implementation. To ensure the change is properly tested:
Would you like me to generate a test case that verifies transfer transactions are excluded from the calendar widget calculations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4143.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts(1 hunks)
🔇 Additional comments (1)
packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.ts (1)
83-83: LGTM! Verify the impact on transfer transactions.The filter condition correctly excludes transfer transactions from income and expense calculations. Let's verify the implementation:
✅ Verification successful
Implementation verified - transfer filtering is consistent
The filter condition
{ transfer_id: { $eq: null } }follows the established pattern used throughout the codebase for excluding transfer transactions from reports. This implementation is consistent with:
- Other reporting components (Calendar.tsx)
- Transaction rule handling
- Core transfer management logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that transfer transactions are properly excluded. # Test: Check if there are any transfer transactions that might still be counted. # Expect: No results as transfer transactions should be excluded. rg -A 5 "transfer_id.*null.*amount.*sum" .Length of output: 44
Script:
#!/bin/bash # Check the full query structure and transfer handling ast-grep --pattern 'makeRootQuery($$$) { $$$ }' # Look for similar transfer filtering patterns rg -B 3 -A 3 'transfer_id.*null' . # Check for other query-related functions in the file rg -B 2 -A 5 'function.*Query' packages/desktop-client/src/components/reports/spreadsheets/calendar-spreadsheet.tsLength of output: 12297
|
👋 Hi! Thank you for the contribution! Sorry I didn't see this issue earlier before your change, but it's better if we retain the maximum functionality of the calendar report. Some people might use it to track when money is leaving and entering their budget as a whole and would quite like to see transfers in there. I've put a comment on the original issue explaining how you/others in the future can customise the calendar report to exclude transfers if that's what you'd like on your reports page. We'd welcome more contributions, and if you're interested on carrying on within the report code then there are plenty of issues open in that area https://github.com/actualbudget/actual/issues?q=is%3Aissue+is%3Aopen+label%3Areports Sorry again for stepping on this one - especially as it's your first contribution. |
|
No worry, I totally understand. It's not like I spent a lot of time on it, and it allowed me to get started with the project which was the main goal anyway. |
Fixes #4136
When fetching transactions for the calendar widget, ignore transactions that are transfers by only getting transactions whose
transfer_idis null.