fix(#2562): Prevent transaction deduplication for imported transactio…

…ns (#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>

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);
+    },
+  );
 });

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],

upcoming-release-notes/2991.md

@@ -0,0 +1,6 @@
+---
+category: Bugfix
+authors: [ttlgeek, strazto, pmoon00]
+---
+
+Prevent transaction deduplication for imported transactions