New: Skip YNAB export for unmapped accounts #646
Conversation
WalkthroughThis pull request introduces account-specific export functionality for transactions. In the common types file, a new interface extends export parameters with an account number, and a function type is added to handle these parameters. In the YNAB exporter file, the transaction creation functions are modified to accept an additional configuration parameter, perform account-specific transaction grouping, handle unmapped accounts by emitting progress events, and adjust error reporting. These changes enable the exporter to skip unmapped accounts while processing transactions by account. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Exporter as ExportTransactions
participant AccountExporter as createTransactionsForAccount
participant YNABService
Client->>Exporter: Request export(transactionsToCreate, outputVendorsConfig, startDate)
Exporter->>Exporter: Check and init categories if needed
Exporter->>Exporter: Group transactions by account number
loop For each account
Exporter->>AccountExporter: Process account-specific transactions
AccountExporter->>YNABService: Get YnabAccountId by account number
alt Account is mapped
YNABService-->>AccountExporter: Return YnabAccountId
AccountExporter-->>Exporter: Return processed transaction count
else Account is unmapped
AccountExporter-->>Exporter: Emit progress event "unmapped account", return 0
end
end
Exporter->>Client: Return aggregated export result
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/main/src/backend/export/outputVendors/ynab/ynab.ts (1)
237-238: Remove @ts-expect-error by adding proper type guards.The type error suppression could be replaced with proper null checks to ensure type safety.
- // @ts-expect-error error TS18049: 'transactionToCreate.amount' is possibly 'null' or 'undefined' - Math.abs(transactionToCreate.amount - transactionFromYnab.amount) < 1000 && + typeof transactionToCreate.amount === 'number' && + typeof transactionFromYnab.amount === 'number' && + Math.abs(transactionToCreate.amount - transactionFromYnab.amount) < 1000 &&
🧹 Nitpick comments (2)
packages/main/src/backend/export/outputVendors/ynab/ynab.ts (2)
41-65: Consider moving categories initialization to the init function.The categories initialization could be moved to the init function to ensure it's ready before any transactions are processed, reducing potential delays during transaction creation.
export async function init(outputVendorsConfig: Config['outputVendors']) { ynabConfig = outputVendorsConfig.ynab; initFromToken(outputVendorsConfig[OutputVendorName.YNAB]?.options.accessToken); + await initCategories(); } const createTransactions: ExportTransactionsFunction = async ( { transactionsToCreate, startDate, outputVendorsConfig }, eventPublisher, ) => { - if (!categoriesMap.size) { - await initCategories(); - } const accountNumbers = _.uniq(transactionsToCreate.map((t) => t.accountNumber));
77-77: Enhance the unmapped account message with more context.The current message could be more informative by including guidance on how to map the account.
- await emitProgressEvent(eventPublisher, allTransactions, `Account ${accountNumber} is unmapped. Skipping.`); + await emitProgressEvent(eventPublisher, allTransactions, `Account ${accountNumber} is not mapped to any YNAB account. Please add the mapping in the YNAB configuration. Skipping.`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/main/src/backend/commonTypes.ts(1 hunks)packages/main/src/backend/export/outputVendors/ynab/ynab.ts(4 hunks)
🔇 Additional comments (2)
packages/main/src/backend/commonTypes.ts (2)
103-105: LGTM! Well-structured interface for account-specific exports.The interface correctly extends ExportTransactionsParams and adds the necessary accountNumber property for per-account operations.
107-110: LGTM! Well-defined function type for account-specific exports.The type definition maintains consistency with existing patterns while adding account-specific functionality.
| try { | ||
| getYnabAccountIdByAccountNumberFromTransaction(accountNumber); | ||
| } catch (e) { | ||
| await emitProgressEvent(eventPublisher, allTransactions, `Account ${accountNumber} is unmapped. Skipping.`); | ||
| return { | ||
| exportedTransactionsNum: 0, | ||
| }; |
There was a problem hiding this comment.
As mentioned in the linked issue, let me know if you'd prefer to have an option in the config for enabling the skipping logic.
| } | ||
| const transactionsFromFinancialAccount = transactionsToCreate.map(convertTransactionToYnabFormat); | ||
| const transactionsFromFinancialAccount = allTransactions | ||
| .filter((v) => v.accountNumber === accountNumber) |
There was a problem hiding this comment.
I decided to do the filter here instead of in the outer function (createTransactions) for 2 reasons:
- The
emitProgressEventfunction (used below) expects to receiveallTransactions. I'm not sure how important that is, so I didn't want to change that. - This way there's no dependency on the caller.
|
Pull request has been marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
sorry it is in my todolist |
|
Pull request has been marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Closes #644. Please read the issue for more details.
Summary by CodeRabbit