Fix: Added import id for YNAB #626
Conversation
In order to avoid duplicate inserted when a payee is changed, an import_id is added
| function buildImportId(transaction: EnrichedTransaction): string { | ||
| return `${transaction.date.substring(0, 10)}${transaction.chargedAmount}${transaction.description}`.substring( | ||
| 0, | ||
| MAX_YNAB_IMPORT_ID_LENGTH, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are you not using the calculateTransactionHash function on packages/main/src/backend/transactions/transactions.ts?
There was a problem hiding this comment.
because YNAB import_id is 36 characters max and should be unique
calculateTransactionHash is variable length and truncated will miss some information
WalkthroughThe changes in this pull request enhance the functionality of the YNAB integration by adding new transaction handling features and improving error management. A new test suite for the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 5
🧹 Outside diff range and nitpick comments (2)
packages/main/src/backend/export/outputVendors/ynab/ynab.test.ts (1)
11-31: Rename misleading variabletransferTransactionFromYnab.The variable name suggests it's a transfer transaction, but the object represents a regular transaction from YNAB. This could be confusing, especially when compared to the actual transfer transaction test case below.
Consider renaming to something more accurate:
- const transferTransactionFromYnab: TransactionDetail = { + const existingYnabTransaction: TransactionDetail = {packages/main/src/backend/export/outputVendors/ynab/ynab.ts (1)
208-209: Simplify and clarify the complex return condition for better readability.The current return statement in
isSameTransactionis complex and could be refactored to enhance clarity and maintainability.Break down the conditions into meaningful variables:
export function isSameTransaction( transactionToCreate: ynab.SaveTransaction, transactionFromYnab: ynab.TransactionDetail, ) { const isATransferTransaction = !!transactionFromYnab.transfer_account_id; const isTransactionsImportIdEqual = isSameImportId(transactionToCreate, transactionFromYnab); + const isSamePayeeName = areStringsEqualIgnoreCaseAndWhitespace( + transactionToCreate.payee_name, + transactionFromYnab.payee_name, + ); + const isSameAccountId = transactionToCreate.account_id === transactionFromYnab.account_id; + const isSameDate = transactionToCreate.date === transactionFromYnab.date; + const amountDifferenceIsSmall = + transactionToCreate.amount != null && + transactionFromYnab.amount != null && + Math.abs(transactionToCreate.amount - transactionFromYnab.amount) < 1000; - return ( - transactionToCreate.account_id === transactionFromYnab.account_id && - transactionToCreate.date === transactionFromYnab.date && - // @ts-expect-error error TS18049: 'transactionToCreate.amount' is possibly 'null' or 'undefined' - Math.abs(transactionToCreate.amount - transactionFromYnab.amount) < 1000 && - (areStringsEqualIgnoreCaseAndWhitespace(transactionToCreate.payee_name, transactionFromYnab.payee_name) || - isATransferTransaction || - isTransactionsImportIdEqual) - ); + return ( + isSameAccountId && + isSameDate && + amountDifferenceIsSmall && + (isSamePayeeName || isATransferTransaction || isTransactionsImportIdEqual) + ); }This refactoring enhances readability and makes future maintenance easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/main/src/backend/export/outputVendors/ynab/ynab.test.ts(1 hunks)packages/main/src/backend/export/outputVendors/ynab/ynab.ts(4 hunks)
🔇 Additional comments (1)
packages/main/src/backend/export/outputVendors/ynab/ynab.test.ts (1)
9-44: LGTM! Test suite effectively validates the import_id matching.
The new test suite directly addresses the PR objective of preventing duplicate transactions when payees are modified in YNAB. The test case effectively validates that transactions with different payee names are considered identical when they share the same import_id.
A few observations worth noting:
- The test data setup is comprehensive, including all necessary fields for both YNAB and financial account transactions
- The test follows the existing patterns in the codebase
- The assertion is clear and focused on the specific behavior being tested
packages/main/src/backend/export/outputVendors/ynab/ynab.test.ts
Outdated
Show resolved
Hide resolved
| function isSameImportId( | ||
| transactionToCreate: ynab.SaveTransaction, | ||
| transactionFromYnab: ynab.TransactionDetail, | ||
| ): boolean { | ||
| return !!transactionToCreate.import_id && transactionToCreate.import_id === transactionFromYnab.import_id; | ||
| } |
There was a problem hiding this comment.
Ensure safe comparison of import_id by validating both values.
In isSameImportId, comparing import_ids without checking if they are null or undefined might lead to unexpected behavior.
Update the function to handle potential null or undefined values:
function isSameImportId(
transactionToCreate: ynab.SaveTransaction,
transactionFromYnab: ynab.TransactionDetail,
): boolean {
- return !!transactionToCreate.import_id && transactionToCreate.import_id === transactionFromYnab.import_id;
+ return (
+ transactionToCreate.import_id != null &&
+ transactionFromYnab.import_id != null &&
+ transactionToCreate.import_id === transactionFromYnab.import_id
+ );
}This change ensures that the comparison only occurs when both import_ids are valid strings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isSameImportId( | |
| transactionToCreate: ynab.SaveTransaction, | |
| transactionFromYnab: ynab.TransactionDetail, | |
| ): boolean { | |
| return !!transactionToCreate.import_id && transactionToCreate.import_id === transactionFromYnab.import_id; | |
| } | |
| function isSameImportId( | |
| transactionToCreate: ynab.SaveTransaction, | |
| transactionFromYnab: ynab.TransactionDetail, | |
| ): boolean { | |
| return ( | |
| transactionToCreate.import_id != null && | |
| transactionFromYnab.import_id != null && | |
| transactionToCreate.import_id === transactionFromYnab.import_id | |
| ); | |
| } |
| @@ -119,12 +119,20 @@ function convertTransactionToYnabFormat(originalTransaction: EnrichedTransaction | |||
| category_id: getYnabCategoryIdFromCategoryName(originalTransaction.category), | |||
| memo: originalTransaction.memo, | |||
| cleared: ynab.SaveTransaction.ClearedEnum.Cleared, | |||
| import_id: buildImportId(originalTransaction), // [date][amount][description] | |||
There was a problem hiding this comment.
Ensure import_id follows YNAB's recommended format for uniqueness.
The current import_id generation may lead to collisions due to truncation and similarity in transaction data. YNAB recommends using the format YNAB:[milliunit_amount]:[iso_date]:[occurrence] to ensure uniqueness.
Consider updating the buildImportId function to match this format. Here's how you might adjust it:
import_id: buildImportId(originalTransaction),Committable suggestion skipped: line range outside the PR's diff.
| function buildImportId(transaction: EnrichedTransaction): string { | ||
| return `${transaction.date.substring(0, 10)}${transaction.chargedAmount}${transaction.description}`.substring( | ||
| 0, | ||
| MAX_YNAB_IMPORT_ID_LENGTH, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve uniqueness of import_id by adopting YNAB's standard format.
Concatenating date, amount, and description can cause collisions when different transactions produce the same truncated string. To enhance uniqueness within the character limit, it's advisable to follow YNAB's suggested pattern.
Here's a revised implementation:
+import crypto from 'crypto';
function buildImportId(transaction: EnrichedTransaction): string {
- return `${transaction.date.substring(0, 10)}${transaction.chargedAmount}${transaction.description}`.substring(
- 0,
- MAX_YNAB_IMPORT_ID_LENGTH,
- );
+ const amount = Math.round(transaction.chargedAmount * 1000);
+ const date = transaction.date.substring(0, 10);
+ const occurrence = '1'; // Or calculate occurrence if necessary
+ return `YNAB:${amount}:${date}:${occurrence}`.substring(0, MAX_YNAB_IMPORT_ID_LENGTH);
}If you need to handle multiple occurrences, you can increment the occurrence value accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function buildImportId(transaction: EnrichedTransaction): string { | |
| return `${transaction.date.substring(0, 10)}${transaction.chargedAmount}${transaction.description}`.substring( | |
| 0, | |
| MAX_YNAB_IMPORT_ID_LENGTH, | |
| ); | |
| } | |
| function buildImportId(transaction: EnrichedTransaction): string { | |
| const amount = Math.round(transaction.chargedAmount * 1000); | |
| const date = transaction.date.substring(0, 10); | |
| const occurrence = '1'; // Or calculate occurrence if necessary | |
| return `YNAB:${amount}:${date}:${occurrence}`.substring(0, MAX_YNAB_IMPORT_ID_LENGTH); | |
| } |
| Math.abs(transactionToCreate.amount - transactionFromYnab.amount) < 1000 && | ||
| // In a transfer transaction the payee name changes, but we still consider this the same transaction | ||
| (areStringsEqualIgnoreCaseAndWhitespace(transactionToCreate.payee_name, transactionFromYnab.payee_name) || |
There was a problem hiding this comment.
Handle potential null or undefined amounts instead of suppressing TypeScript errors.
Using @ts-expect-error suppresses TypeScript's helpful checks and may hide runtime issues if transactionToCreate.amount is null or undefined.
Consider modifying the code to handle these cases explicitly:
-// @ts-expect-error error TS18049: 'transactionToCreate.amount' is possibly 'null' or 'undefined'
-Math.abs(transactionToCreate.amount - transactionFromYnab.amount) < 1000 &&
+const amountDifferenceIsSmall =
+ transactionToCreate.amount != null &&
+ transactionFromYnab.amount != null &&
+ Math.abs(transactionToCreate.amount - transactionFromYnab.amount) < 1000;
+
+return (
+ transactionToCreate.account_id === transactionFromYnab.account_id &&
+ transactionToCreate.date === transactionFromYnab.date &&
+ amountDifferenceIsSmall &&
+ (areStringsEqualIgnoreCaseAndWhitespace(transactionToCreate.payee_name, transactionFromYnab.payee_name) ||
+ isATransferTransaction ||
+ isTransactionsImportIdEqual)
+);This approach ensures that you handle null or undefined values safely without suppressing errors.
Committable suggestion skipped: line range outside the PR's diff.
|
@abirstolov we're just trying the @coderabbitai service, please read its comments carefully and let us know if it was relevant comments. |
|
Tip For best results, initiate chat on the files or code changes. @baruchiro: Thank you for introducing me to the team. I hope my comments are helpful. If you or @abirstolov have any questions or need further assistance, please feel free to reach out. |
packages/main/src/backend/export/outputVendors/ynab/ynab.test.ts
Outdated
Show resolved
Hide resolved
packages/main/src/backend/export/outputVendors/ynab/ynab.test.ts
Outdated
Show resolved
Hide resolved
|
@abirstolov did you check this version locally to see that it doesn't create duplicates of existing transactions? |
|
@abirstolov if you need any help with GitHub talk with us on Discord |
|
@baruchiro I have tested that case, for the same transaction with a different payee - it didn't add a duplicate, and for a different transaction it did add. |
|
🎉 This PR is included in version 2.4.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
In order to avoid duplicate inserted when a payee is changed, an import_id is added to the YNAB transaction
Fix #625
Summary by CodeRabbit
New Features
Bug Fixes
Tests
isSameTransactionfunction to verify behavior with different payees.