Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Added import id for YNAB #626

Merged
merged 5 commits into from
Dec 5, 2024
Merged

Fix: Added import id for YNAB #626

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
28f9e84
Added import id for YNAB
Nov 23, 2024
460aef8
added YNAB max import id const
Nov 23, 2024
bdc593f
break condition into a variable
Nov 26, 2024
008131c
added description to ynab import_id test
Nov 29, 2024
df479a8
refactor: YNAB isSameTransaction tests
Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions packages/main/src/backend/export/outputVendors/ynab/ynab.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,42 @@ import * as ynab from './ynab';
import ClearedEnum = SaveTransaction.ClearedEnum;

describe('ynab', () => {
describe('isSameTransaction(Payee changed)', () => {
abirstolov marked this conversation as resolved.
Show resolved Hide resolved
test('Two transactions with different payee names should be considered the same if they have the same import id', () => {
const transferTransactionFromYnab: TransactionDetail = {
abirstolov marked this conversation as resolved.
Show resolved Hide resolved
id: '579ae642-d161-4bbe-9d54-ae3322c93cf7',
date: '2019-06-27',
amount: -1000000,
memo: null,
cleared: SaveTransaction.ClearedEnum.Cleared,
approved: true,
flag_color: null,
account_id: 'SOME_ACCOUNT_ID',
account_name: 'My great account',
payee_id: 'fd7f187c-0633-434f-aaxe-1fevd68492cb',
payee_name: 'שיק',
category_id: null,
category_name: null,
transfer_account_id: null,
transfer_transaction_id: null,
matched_transaction_id: null,
import_id: '2019-06-27-1000000שיק',
deleted: false,
subtransactions: [],
};
const transactionFromFinancialAccount: SaveTransaction = {
account_id: 'SOME_ACCOUNT_ID',
date: '2019-06-27',
amount: -1000000,
payee_name: 'גן',
category_id: '4e0ttc69-b4f6-420b-8d07-986c8225a3d4',
cleared: ClearedEnum.Cleared,
import_id: '2019-06-27-1000000שיק',
};

expect(ynab.isSameTransaction(transactionFromFinancialAccount, transferTransactionFromYnab)).toBeTruthy();
});
});
describe('isSameTransaction', () => {
test('Two transactions with different payee names should be considered the same if one of them is a transfer transaction', () => {
const transferTransactionFromYnab: TransactionDetail = {
Expand Down
21 changes: 19 additions & 2 deletions packages/main/src/backend/export/outputVendors/ynab/ynab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import * as ynab from 'ynab';
const YNAB_DATE_FORMAT = 'YYYY-MM-DD';
const NOW = moment();
const MIN_YNAB_ACCESS_TOKEN_LENGTH = 43;
const MAX_YNAB_IMPORT_ID_LENGTH = 36;

const categoriesMap = new Map<string, Pick<ynab.Category, 'id' | 'name' | 'category_group_id'>>();
const transactionsFromYnab = new Map<Date, ynab.TransactionDetail[]>();
Expand Down Expand Up @@ -109,7 +110,6 @@ export function getPayeeName(transaction: EnrichedTransaction, payeeNameMaxLengt
function convertTransactionToYnabFormat(originalTransaction: EnrichedTransaction): ynab.SaveTransaction {
const amount = Math.round(originalTransaction.chargedAmount * 1000);
const date = convertTimestampToYnabDateFormat(originalTransaction);

return {
account_id: getYnabAccountIdByAccountNumberFromTransaction(originalTransaction.accountNumber),
date, // "2019-01-17",
Expand All @@ -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]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

// "approved": true,
// "flag_color": "red",
// "import_id": buildImportId(originalTransaction.description, amount, date) // 'YNAB:[milliunit_amount]:[iso_date]:[occurrence]'
};
}

function buildImportId(transaction: EnrichedTransaction): string {
return `${transaction.date.substring(0, 10)}${transaction.chargedAmount}${transaction.description}`.substring(
0,
MAX_YNAB_IMPORT_ID_LENGTH,
);
}
Comment on lines +129 to +134
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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);
}


Comment on lines +129 to +135
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you not using the calculateTransactionHash function on packages/main/src/backend/transactions/transactions.ts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because YNAB import_id is 36 characters max and should be unique
calculateTransactionHash is variable length and truncated will miss some information

function getYnabAccountIdByAccountNumberFromTransaction(transactionAccountNumber: string): string {
const ynabAccountId = ynabConfig!.options.accountNumbersToYnabAccountIds[transactionAccountNumber];
if (!ynabAccountId) {
Expand Down Expand Up @@ -189,14 +197,16 @@ export function isSameTransaction(
transactionFromYnab: ynab.TransactionDetail,
) {
const isATransferTransaction = !!transactionFromYnab.transfer_account_id;
const isTransactionsImportIdEqual = isSameImportId(transactionToCreate, transactionFromYnab);
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 &&
// In a transfer transaction the payee name changes, but we still consider this the same transaction
brafdlog marked this conversation as resolved.
Show resolved Hide resolved
(areStringsEqualIgnoreCaseAndWhitespace(transactionToCreate.payee_name, transactionFromYnab.payee_name) ||
Comment on lines 205 to 207
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

isATransferTransaction)
isATransferTransaction ||
isTransactionsImportIdEqual)
);
}

Expand Down Expand Up @@ -321,3 +331,10 @@ export const ynabOutputVendor: OutputVendor = {
init,
exportTransactions: createTransactions,
};

function isSameImportId(
transactionToCreate: ynab.SaveTransaction,
transactionFromYnab: ynab.TransactionDetail,
): boolean {
return !!transactionToCreate.import_id && transactionToCreate.import_id === transactionFromYnab.import_id;
}
Comment on lines +335 to +340
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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
);
}

Loading