Identify and fix broken transfers #4216
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
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 |
…t should not be there
|
🎉 That seems to work! We should probably rename that button to be something more generic than about splits, and add a comment that transfers will get fixed. |
|
Should we make the needs repair text clickable to directly fix it? |
If you can, then yes please! |
youngcw
changed the title
WalkthroughThe pull request introduces changes to the transaction repair functionality across multiple files in the desktop client and server tools. The primary modifications include renaming the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 1
🧹 Nitpick comments (5)
packages/loot-core/src/server/tools/app.ts (1)
77-85: Add NULL check for transfer_id in SQL query.The query should handle cases where
transfer_idis NULL to avoid potential issues.SELECT t1.id FROM v_transactions_internal t1 JOIN accounts a1 ON t1.account = a1.id - JOIN v_transactions_internal t2 ON t1.transfer_id = t2.id + JOIN v_transactions_internal t2 ON t1.transfer_id = t2.id AND t1.transfer_id IS NOT NULL JOIN accounts a2 ON t2.account = a2.id WHERE a1.offbudget = a2.offbudget AND t1.category IS NOT NULLpackages/desktop-client/src/components/settings/RepairTransactions.tsx (2)
157-158: Fix typo in description text.The word "erroniously" is misspelled.
- Check if you have any budget transfers that erroniously contain a + Check if you have any budget transfers that erroneously contain a
62-64: Improve the fixed transfers message.The message could be more descriptive about what was fixed.
- t('Fixed {{count}} transfers.', { + t('Fixed {{count}} off-budget transfers by removing incorrect categories.', {packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
1445-1447: LGTM! The changes effectively identify broken transfers.The modification correctly implements the PR's objective by displaying 'Needs Repair' when a budget transfer incorrectly has a category assigned, helping users identify broken transfers that need attention.
Consider adding a comment explaining this logic for future maintainers:
: isBudgetTransfer + // For budget transfers, display 'Needs Repair' if a category is incorrectly assigned + // otherwise display 'Transfer' as expected ? categoryId != null ? 'Needs Repair' : 'Transfer'
1445-1447: Consider making the 'Needs Repair' text actionable.Based on the PR discussion, particularly UnderKoen's suggestion, consider making the 'Needs Repair' text clickable to help users directly fix the broken transfers. This would improve the user experience by providing a clear path to resolution.
Example implementation approach:
: isBudgetTransfer ? categoryId != null - ? 'Needs Repair' + ? <Button + variant="bare" + style={{ + color: theme.errorText, + textDecoration: 'underline', + cursor: 'pointer' + }} + onPress={() => onRepairTransfer(transaction.id)} + > + Needs Repair + </Button> : 'Transfer'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4216.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/settings/RepairTransactions.tsx(5 hunks)packages/desktop-client/src/components/settings/index.tsx(2 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(1 hunks)packages/loot-core/src/server/tools/app.ts(1 hunks)packages/loot-core/src/server/tools/types/handlers.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: compare
🔇 Additional comments (3)
packages/loot-core/src/server/tools/types/handlers.d.ts (1)
8-8: LGTM!The type definition for
numTransfersFixedis correctly added to track the number of fixed transfers.packages/desktop-client/src/components/settings/index.tsx (1)
34-34: LGTM!The component import is correctly updated to use the renamed
RepairTransactionscomponent.packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
1445-1447: Strengthen the null check for better error handling.The current check
categoryId != nullcould be made more explicit to handle all edge cases properly.Consider this more robust implementation:
: isBudgetTransfer - ? categoryId != null + ? categoryId !== null && categoryId !== undefined ? 'Needs Repair' : 'Transfer'Let's verify if there are any instances where categoryId might be undefined:
| await runMutator(async () => { | ||
| const updated = brokenTransfers.map(row => ({ | ||
| id: row.id, | ||
| category: null, | ||
| })); | ||
| await batchUpdateTransactions({ updated }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for failed updates.
The update operation should include error handling to ensure all transfers are properly fixed.
await runMutator(async () => {
+ try {
const updated = brokenTransfers.map(row => ({
id: row.id,
category: null,
}));
- await batchUpdateTransactions({ updated });
+ const results = await batchUpdateTransactions({ updated });
+ if (results.length !== updated.length) {
+ throw new Error('Not all transfers were updated successfully');
+ }
+ } catch (error) {
+ console.error('Failed to update transfers:', error);
+ throw error;
+ }
});Committable suggestion skipped: line range outside the PR's diff.
|
Nice! |
|
@UnderKoen Im going to merge this now. Feel free to make another PR that makes the error text clickable. |
There is currently nothing in the code that automatically fixes an on budget transfer that ended up with a category. This adds that function to the "fix splits" script, and renames that script to "Repair Transactions".
Ive attached an example file that will show the "BROKEN" category label.
test1.zip
View the "test" account to see the broken transfer, or open the "Food" category spending
Thanks @UnderKoen for adding in the actual fix part.