From 30a70f56270d22577c1f2e3d99a3113e8219a28d Mon Sep 17 00:00:00 2001
From: pmoon <14009133+pmoon00@users.noreply.github.com>
Date: Fri, 9 Aug 2024 12:44:37 +1200
Subject: [PATCH] fix(#2562): Prevent transaction deduplication for imported
transactions (#2991)
* fix(#2562): Prevent transaction deduplication for imported transactions (#2770)
* fix(#2562): Prevent transaction deduplication for imported transactions
* chore(): eslint fixes
* chore(): Add release note file
* fix(#2562): Allow transaction deduplication if transaction being imported is null
* chore: Rename release note, add strazto as author
* test(loot-core): Add test case for new logic
* docs(release-notes.loot-core): Add pmoon00 as author
* test(loot-core): Update test case to not be affected by unrelated bug
* test(loot-core): fix linter
---------
Co-authored-by: Mohamed El Mahdali <mohamed.elmahdali.developer@gmail.com>
Co-authored-by: Matiss Janis Aboltins <matiss@mja.lv>
* Add Handling For goCardless Fuzzy Search
* Rename Release Note File
* Rename Release Notes File
* Fix UseFuzzySearchV2 After Merge Conflict
* Update Fuzzy Search Query To Include New Columns
* Update useFuzzyMatchV2 Variable To useStrictIdChecking
* Update useStrictIdChecking To Only Be Used If It's Not Syncing From External Sources
---------
Co-authored-by: Matthew Strasiotto <39424834+strazto@users.noreply.github.com>
Co-authored-by: Mohamed El Mahdali <mohamed.elmahdali.developer@gmail.com>
Co-authored-by: Matiss Janis Aboltins <matiss@mja.lv>
---
.../src/server/accounts/sync.test.ts | 157 ++++++++++++++++++
.../loot-core/src/server/accounts/sync.ts | 68 ++++++--
upcoming-release-notes/2991.md | 6 +
3 files changed, 217 insertions(+), 14 deletions(-)
create mode 100644 upcoming-release-notes/2991.md
diff --git a/packages/loot-core/src/server/accounts/sync.test.ts b/packages/loot-core/src/server/accounts/sync.test.ts
index 1ea00a4d0..b4356875c 100644
--- a/packages/loot-core/src/server/accounts/sync.test.ts
+++ b/packages/loot-core/src/server/accounts/sync.test.ts
@@ -342,4 +342,161 @@ describe('Account sync', () => {
'bakkerij-renamed',
]);
});
+
+ test('reconcile does not merge transactions with different ‘imported_id’ values', async () => {
+ const { id } = await prepareDatabase();
+
+ let payees = await getAllPayees();
+ expect(payees.length).toBe(0);
+
+ // Add first transaction
+ await reconcileTransactions(id, [
+ {
+ date: '2024-04-05',
+ amount: -1239,
+ imported_payee: 'Acme Inc.',
+ payee_name: 'Acme Inc.',
+ imported_id: 'b85cdd57-5a1c-4ca5-bd54-12e5b56fa02c',
+ notes: 'TEST TRANSACTION',
+ cleared: true,
+ },
+ ]);
+
+ payees = await getAllPayees();
+ expect(payees.length).toBe(1);
+
+ let transactions = await getAllTransactions();
+ expect(transactions.length).toBe(1);
+
+ // Add second transaction
+ await reconcileTransactions(id, [
+ {
+ date: '2024-04-06',
+ amount: -1239,
+ imported_payee: 'Acme Inc.',
+ payee_name: 'Acme Inc.',
+ imported_id: 'ca1589b2-7bc3-4587-a157-476170b383a7',
+ notes: 'TEST TRANSACTION',
+ cleared: true,
+ },
+ ]);
+
+ payees = await getAllPayees();
+ expect(payees.length).toBe(1);
+
+ transactions = await getAllTransactions();
+ expect(transactions.length).toBe(2);
+
+ expect(
+ transactions.find(
+ t => t.imported_id === 'b85cdd57-5a1c-4ca5-bd54-12e5b56fa02c',
+ ).amount,
+ ).toBe(-1239);
+ expect(
+ transactions.find(
+ t => t.imported_id === 'ca1589b2-7bc3-4587-a157-476170b383a7',
+ ).amount,
+ ).toBe(-1239);
+ });
+
+ test(
+ 'given an imported tx with no imported_id, ' +
+ 'when using fuzzy search V2, existing transaction has an imported_id, matches amount, and is within 7 days of imported tx, ' +
+ 'then imported tx should reconcile with existing transaction from fuzzy match',
+ async () => {
+ const { id } = await prepareDatabase();
+
+ let payees = await getAllPayees();
+ expect(payees.length).toBe(0);
+
+ const existingTx = {
+ date: '2024-04-05',
+ amount: -1239,
+ imported_payee: 'Acme Inc.',
+ payee_name: 'Acme Inc.',
+ imported_id: 'b85cdd57-5a1c-4ca5-bd54-12e5b56fa02c',
+ notes: 'TEST TRANSACTION',
+ cleared: true,
+ };
+
+ // Add transaction to represent existing transaction with imoprted_id
+ await reconcileTransactions(id, [existingTx]);
+
+ payees = await getAllPayees();
+ expect(payees.length).toBe(1);
+
+ let transactions = await getAllTransactions();
+ expect(transactions.length).toBe(1);
+
+ // Import transaction similar to existing but with different date and no imported_id
+ await reconcileTransactions(id, [
+ {
+ ...existingTx,
+ date: '2024-04-06',
+ imported_id: null,
+ },
+ ]);
+
+ payees = await getAllPayees();
+ expect(payees.length).toBe(1);
+
+ transactions = await getAllTransactions();
+ expect(transactions.length).toBe(1);
+
+ expect(transactions[0].amount).toBe(-1239);
+ },
+ );
+
+ test(
+ 'given an imported tx has an imported_id, ' +
+ 'when not using fuzzy search V2, existing transaction has an imported_id, matches amount, and is within 7 days of imported tx, ' +
+ 'then imported tx should reconcile with existing transaction from fuzzy match',
+ async () => {
+ const { id } = await prepareDatabase();
+
+ let payees = await getAllPayees();
+ expect(payees.length).toBe(0);
+
+ const existingTx = {
+ date: '2024-04-05',
+ amount: -1239,
+ imported_payee: 'Acme Inc.',
+ payee_name: 'Acme Inc.',
+ imported_id: 'b85cdd57-5a1c-4ca5-bd54-12e5b56fa02c',
+ notes: 'TEST TRANSACTION',
+ cleared: true,
+ };
+
+ // Add transaction to represent existing transaction with imoprted_id
+ await reconcileTransactions(id, [existingTx]);
+
+ payees = await getAllPayees();
+ expect(payees.length).toBe(1);
+
+ let transactions = await getAllTransactions();
+ expect(transactions.length).toBe(1);
+
+ // Import transaction similar to existing but with different date and imported_id
+ await reconcileTransactions(
+ id,
+ [
+ {
+ ...existingTx,
+ date: '2024-04-06',
+ imported_id: 'something-else-entirely',
+ },
+ ],
+ false,
+ false,
+ );
+
+ payees = await getAllPayees();
+ expect(payees.length).toBe(1);
+
+ transactions = await getAllTransactions();
+ expect(transactions.length).toBe(1);
+
+ expect(transactions[0].amount).toBe(-1239);
+ },
+ );
});
diff --git a/packages/loot-core/src/server/accounts/sync.ts b/packages/loot-core/src/server/accounts/sync.ts
index b89969dc4..b83c5ddd6 100644
--- a/packages/loot-core/src/server/accounts/sync.ts
+++ b/packages/loot-core/src/server/accounts/sync.ts
@@ -309,6 +309,7 @@ export async function reconcileTransactions(
acctId,
transactions,
isBankSyncAccount = false,
+ strictIdChecking = true,
isPreview = false,
) {
console.log('Performing transaction reconciliation');
@@ -323,7 +324,12 @@ export async function reconcileTransactions(
transactionsStep1,
transactionsStep2,
transactionsStep3,
- } = await matchTransactions(acctId, transactions, isBankSyncAccount);
+ } = await matchTransactions(
+ acctId,
+ transactions,
+ isBankSyncAccount,
+ strictIdChecking,
+ );
// Finally, generate & commit the changes
for (const { trans, subtransactions, match } of transactionsStep3) {
@@ -416,6 +422,7 @@ export async function matchTransactions(
acctId,
transactions,
isBankSyncAccount = false,
+ strictIdChecking = true,
) {
console.log('Performing transaction reconciliation matching');
@@ -459,20 +466,39 @@ export async function matchTransactions(
// If it didn't match, query data needed for fuzzy matching
if (!match) {
- // Look 7 days ahead and 7 days back when fuzzy matching. This
+ // Fuzzy matching looks 7 days ahead and 7 days back. This
// needs to select all fields that need to be read from the
// matched transaction. See the final pass below for the needed
// fields.
- fuzzyDataset = await db.all(
- `SELECT id, is_parent, date, imported_id, payee, imported_payee, category, notes, reconciled, cleared, amount FROM v_transactions
- WHERE date >= ? AND date <= ? AND amount = ? AND account = ?`,
- [
- db.toDateRepr(monthUtils.subDays(trans.date, 7)),
- db.toDateRepr(monthUtils.addDays(trans.date, 7)),
- trans.amount || 0,
- acctId,
- ],
- );
+ const sevenDaysBefore = db.toDateRepr(monthUtils.subDays(trans.date, 7));
+ const sevenDaysAfter = db.toDateRepr(monthUtils.addDays(trans.date, 7));
+ // strictIdChecking has the added behaviour of only matching on transactions with no import ID
+ // if the transaction being imported has an import ID.
+ if (strictIdChecking) {
+ fuzzyDataset = await db.all(
+ `SELECT id, is_parent, date, imported_id, payee, imported_payee, category, notes, reconciled, cleared, amount
+ FROM v_transactions
+ WHERE
+ -- If both ids are set, and we didn't match earlier then skip dedup
+ (imported_id IS NULL OR ? IS NULL)
+ AND date >= ? AND date <= ? AND amount = ?
+ AND account = ?`,
+ [
+ trans.imported_id || null,
+ sevenDaysBefore,
+ sevenDaysAfter,
+ trans.amount || 0,
+ acctId,
+ ],
+ );
+ } else {
+ fuzzyDataset = await db.all(
+ `SELECT id, is_parent, date, imported_id, payee, imported_payee, category, notes, reconciled, cleared, amount
+ FROM v_transactions
+ WHERE date >= ? AND date <= ? AND amount = ? AND account = ?`,
+ [sevenDaysBefore, sevenDaysAfter, trans.amount || 0, acctId],
+ );
+ }
// Sort the matched transactions according to the distance from the original
// transactions date. i.e. if the original transaction is in 21-02-2024 and
@@ -620,6 +646,10 @@ export async function syncAccount(
);
const acctRow = await db.select('accounts', id);
+ // If syncing an account from sync source it must not use strictIdChecking. This allows
+ // the fuzzy search to match transactions where the import IDs are different. It is a known quirk
+ // that account sync sources can give two different transaction IDs even though it's the same transaction.
+ const useStrictIdChecking = !acctRow.account_sync_source;
if (latestTransaction) {
const startingTransaction = await db.first(
@@ -670,7 +700,12 @@ export async function syncAccount(
}));
return runMutator(async () => {
- const result = await reconcileTransactions(id, transactions, true);
+ const result = await reconcileTransactions(
+ id,
+ transactions,
+ true,
+ useStrictIdChecking,
+ );
await updateAccountBalance(id, accountBalance);
return result;
});
@@ -725,7 +760,12 @@ export async function syncAccount(
starting_balance_flag: true,
});
- const result = await reconcileTransactions(id, transactions, true);
+ const result = await reconcileTransactions(
+ id,
+ transactions,
+ true,
+ useStrictIdChecking,
+ );
return {
...result,
added: [initialId, ...result.added],
diff --git a/upcoming-release-notes/2991.md b/upcoming-release-notes/2991.md
new file mode 100644
index 000000000..3681a1e52
--- /dev/null
+++ b/upcoming-release-notes/2991.md
@@ -0,0 +1,6 @@
+---
+category: Bugfix
+authors: [ttlgeek, strazto, pmoon00]
+---
+
+Prevent transaction deduplication for imported transactions
--
GitLab