nYNAB import - properly handle hidden categories and groups #4294
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 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 |
| "id": "F5751985-3290-41E7-B17F-6DBE979F315D", | ||
| "name": "Bills", | ||
| "hidden": true, | ||
| "deleted": false |
There was a problem hiding this comment.
for testing duplicate groups
| "goal_overall_funded": null, | ||
| "goal_overall_left": null, | ||
| "deleted": false | ||
| }, |
There was a problem hiding this comment.
for testing duplicate categories
youngcw
changed the title
WalkthroughThis pull request extends the category management functionality by introducing and propagating a Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ 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 (2)
packages/loot-core/src/server/importers/ynab5.ts (2)
85-106: Consider extracting retry logic to a reusable function.The retry logic is duplicated between category group and category creation. Consider extracting it to a reusable function to improve maintainability and reduce code duplication.
+async function createWithRetry<T>( + name: string, + createFn: (name: string) => Promise<T>, + maxRetries: number = 10 +): Promise<T> { + let count = 1; + const origName = name; + + while (true) { + try { + return await createFn(name); + } catch (e) { + if (count >= maxRetries) { + throw Error(e.message); + } + name = origName + '-' + count.toString(); + count++; + } + } +} // Usage in createCategoryGroup -let run = true; -const MAX_RETRY = 10; -let count = 1; -const origName = group.name; -while (run) { - try { - groupId = await actual.createCategoryGroup({ - name: group.name, - is_income: false, - hidden: group.hidden, - }); - entityIdMap.set(group.id, groupId); - run = false; - } catch (e) { - group.name = origName + '-' + count.toString(); - count += 1; - if (count >= MAX_RETRY) { - run = false; - throw Error(e.message); - } - } -} +groupId = await createWithRetry( + group.name, + async (name) => { + const id = await actual.createCategoryGroup({ + name, + is_income: false, + hidden: group.hidden, + }); + entityIdMap.set(group.id, id); + return id; + } +);
133-154: Add logging for retry attempts.Consider adding logging to help track retry attempts during category creation. This will be valuable for debugging import issues.
+import { logger } from '../logger'; // In retry logic } catch (e) { + logger.debug( + `Retry ${count} for category "${origName}": ${e.message}` + ); cat.name = origName + '-' + count.toString();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
upcoming-release-notes/4293.mdis excluded by!**/*.mdupcoming-release-notes/4294.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/e2e/data/ynab5-demo-budget.json(3 hunks)packages/loot-core/src/server/api.ts(1 hunks)packages/loot-core/src/server/importers/ynab5-types.d.ts(1 hunks)packages/loot-core/src/server/importers/ynab5.ts(2 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
🔇 Additional comments (9)
packages/loot-core/src/server/importers/ynab5-types.d.ts (1)
33-33: LGTM! Type definitions for hidden property added correctly.The addition of the
hiddenboolean property to bothCategoryGroupandCategoryinterfaces aligns with the PR objective of preserving hidden status during YNAB import.Also applies to: 41-41
packages/loot-core/src/types/server-handlers.d.ts (1)
96-96: LGTM! Server handler type updated correctly.The addition of the optional
hiddenproperty to thecategory-group-createhandler's argument type is consistent with the YNAB import requirements.packages/loot-core/src/server/importers/ynab5.ts (1)
82-82: LGTM! Hidden Categories group exclusion added.The addition of
Hidden Categoriesto the excluded groups list is correct and aligns with the PR objective.Also applies to: 82-82
packages/loot-core/src/server/api.ts (1)
631-634: LGTM! API handler updated correctly.The addition of the
hiddenproperty to the category group creation handler is consistent with the type definitions and YNAB import requirements.packages/loot-core/src/server/main.ts (1)
371-383: LGTM! The changes properly handle hidden category groups.The handler now correctly accepts and propagates the
hiddenproperty when creating category groups, which is essential for preserving the visibility state during YNAB imports.packages/desktop-client/e2e/data/ynab5-demo-budget.json (4)
169-174: LGTM! Good test case for hidden category groups.The test data includes a hidden category group that shares a name with an existing visible group, providing good coverage for both the hidden property and duplicate name handling.
177-199: LGTM! Good test case for category visibility independence.The test data verifies that a category's visibility is independent of its parent group's visibility by including a visible category under a hidden group.
273-296: LGTM! Good test case for hidden categories under visible groups.The test data verifies that categories can be hidden independently of their parent group's visibility by including a hidden category under a visible group.
633-656: LGTM! Good test case for duplicate categories with different visibility.The test data includes a hidden category that shares a name with an existing visible category, providing good coverage for both the hidden property and duplicate name handling.
Pass along YNAB hidden attribute so categories and groups can retain hidden status after migration.