fix: allow child transactions to have different transfer payees (#4255)

* fix: allow child transactions to have different transfer payees

* Add release notes

packages/loot-core/src/server/accounts/__snapshots__/transfer.test.ts.snap

@@ -1,5 +1,140 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`Transfer split transfers are retained on child transactions 1`] = `
+Array [
+  Object {
+    "account": "one",
+    "amount": 5000,
+    "category": null,
+    "cleared": 1,
+    "date": 20170101,
+    "error": null,
+    "id": "id5",
+    "imported_id": null,
+    "imported_payee": null,
+    "is_child": 0,
+    "is_parent": 0,
+    "notes": null,
+    "parent_id": null,
+    "payee": "id3",
+    "payee_name": "",
+    "reconciled": 0,
+    "schedule": null,
+    "sort_order": 123456789,
+    "starting_balance_flag": 0,
+    "tombstone": 0,
+    "transfer_id": "id6",
+  },
+  Object {
+    "account": "two",
+    "amount": -5000,
+    "category": null,
+    "cleared": 0,
+    "date": 20170101,
+    "error": null,
+    "id": "id6",
+    "imported_id": null,
+    "imported_payee": null,
+    "is_child": 0,
+    "is_parent": 0,
+    "notes": null,
+    "parent_id": null,
+    "payee": "id2",
+    "payee_name": "",
+    "reconciled": 0,
+    "schedule": null,
+    "sort_order": 123456789,
+    "starting_balance_flag": 0,
+    "tombstone": 0,
+    "transfer_id": "id5",
+  },
+]
+`;
+
+exports[`Transfer split transfers are retained on child transactions 2`] = `
+"Snapshot Diff:
+- First value
++ Second value
+
+@@ -8,11 +8,11 @@
+      \\"error\\": null,
+      \\"id\\": \\"id5\\",
+      \\"imported_id\\": null,
+      \\"imported_payee\\": null,
+      \\"is_child\\": 0,
+-     \\"is_parent\\": 0,
++     \\"is_parent\\": 1,
+      \\"notes\\": null,
+      \\"parent_id\\": null,
+      \\"payee\\": \\"id3\\",
+      \\"payee_name\\": \\"\\",
+      \\"reconciled\\": 0,"
+`;
+
+exports[`Transfer split transfers are retained on child transactions 3`] = `
+"Snapshot Diff:
+- First value
++ Second value
+
+@@ -21,10 +21,56 @@
+      \\"starting_balance_flag\\": 0,
+      \\"tombstone\\": 0,
+      \\"transfer_id\\": \\"id6\\",
+    },
+    Object {
++     \\"account\\": \\"one\\",
++     \\"amount\\": 2000,
++     \\"category\\": null,
++     \\"cleared\\": 1,
++     \\"date\\": 20170101,
++     \\"error\\": null,
++     \\"id\\": \\"id7\\",
++     \\"imported_id\\": null,
++     \\"imported_payee\\": null,
++     \\"is_child\\": 1,
++     \\"is_parent\\": 0,
++     \\"notes\\": null,
++     \\"parent_id\\": \\"id5\\",
++     \\"payee\\": \\"id2\\",
++     \\"payee_name\\": \\"\\",
++     \\"reconciled\\": 0,
++     \\"schedule\\": null,
++     \\"sort_order\\": 123456789,
++     \\"starting_balance_flag\\": 0,
++     \\"tombstone\\": 0,
++     \\"transfer_id\\": \\"id8\\",
++   },
++   Object {
++     \\"account\\": \\"one\\",
++     \\"amount\\": -2000,
++     \\"category\\": null,
++     \\"cleared\\": 0,
++     \\"date\\": 20170101,
++     \\"error\\": null,
++     \\"id\\": \\"id8\\",
++     \\"imported_id\\": null,
++     \\"imported_payee\\": null,
++     \\"is_child\\": 0,
++     \\"is_parent\\": 0,
++     \\"notes\\": null,
++     \\"parent_id\\": null,
++     \\"payee\\": \\"id2\\",
++     \\"payee_name\\": \\"\\",
++     \\"reconciled\\": 0,
++     \\"schedule\\": null,
++     \\"sort_order\\": 123456789,
++     \\"starting_balance_flag\\": 0,
++     \\"tombstone\\": 0,
++     \\"transfer_id\\": \\"id7\\",
++   },
++   Object {
+      \\"account\\": \\"two\\",
+      \\"amount\\": -5000,
+      \\"category\\": null,
+      \\"cleared\\": 0,
+      \\"date\\": 20170101,"
+`;
+
 exports[`Transfer transfers are properly de-categorized 1`] = `
 Array [
   Object {

packages/loot-core/src/server/accounts/rules.test.ts

@@ -563,6 +563,31 @@ describe('Rule', () => {
   });
 
   describe('split actions', () => {
+    test('splits can change the payee', () => {
+      const rule = new Rule({
+        conditionsOp: 'and',
+        conditions: [{ op: 'is', field: 'payee', value: '123' }],
+        actions: [
+          {
+            op: 'set-split-amount',
+            field: 'amount',
+            value: 100,
+            options: { splitIndex: 1, method: 'fixed-amount' },
+          },
+          {
+            op: 'set',
+            field: 'payee',
+            value: '456',
+            options: { splitIndex: 1 },
+          },
+        ],
+      });
+
+      expect(rule.exec({ payee: '123' })).toMatchObject({
+        subtransactions: [{ payee: '456' }],
+      });
+    });
+
     const fixedAmountRule = new Rule({
       conditionsOp: 'and',
       conditions: [{ op: 'is', field: 'imported_payee', value: 'James' }],

packages/loot-core/src/server/accounts/transfer.test.ts

@@ -44,6 +44,9 @@ type Transaction = {
   notes?: string;
   payee: string;
   transfer_id?: string;
+  is_parent?: boolean;
+  is_child?: boolean;
+  parent_id?: string;
 };
 
 describe('Transfer', () => {
@@ -167,4 +170,52 @@ describe('Transfer', () => {
     await transfer.onUpdate(transaction);
     differ.expectToMatchDiff(await getAllTransactions());
   });
+
+  test('split transfers are retained on child transactions', async () => {
+    // test: first add a txn having a transfer acct payee
+    // then mark it as `is_parent` and add a child txn
+    // the child txn should have a different transfer acct payee
+    // and `is_child` set to true
+    await prepareDatabase();
+
+    const [transferOne, transferTwo] = await Promise.all([
+      db.first("SELECT * FROM payees WHERE transfer_acct = 'one'"),
+      db.first("SELECT * FROM payees WHERE transfer_acct = 'two'"),
+    ]);
+
+    let parent: Transaction = {
+      account: 'one',
+      amount: 5000,
+      payee: transferTwo.id,
+      date: '2017-01-01',
+    };
+    parent.id = await db.insertTransaction(parent);
+    await transfer.onInsert(parent);
+    parent = await db.getTransaction(parent.id);
+
+    const differ = expectSnapshotWithDiffer(await getAllTransactions());
+
+    // mark the txn as parent
+    await db.updateTransaction({ id: parent.id, is_parent: true });
+    await transfer.onUpdate(parent);
+    differ.expectToMatchDiff(await getAllTransactions());
+
+    // add a child txn
+    let child: Transaction = {
+      account: 'one',
+      amount: 2000,
+      payee: transferOne.id,
+      date: '2017-01-01',
+      is_child: true,
+      parent_id: parent.id,
+    };
+    child.id = await db.insertTransaction(child);
+    await transfer.onInsert(child);
+    differ.expectToMatchDiff(await getAllTransactions());
+
+    // ensure that the child txn has the correct transfer acct payee
+    child = await db.getTransaction(child.id);
+    expect(child.transfer_id).not.toBe(parent.transfer_id);
+    expect(child.payee).toBe(transferOne.id);
+  });
 });

packages/loot-core/src/server/accounts/transfer.ts

@@ -55,29 +55,6 @@ export async function addTransfer(transaction, transferredAccount) {
     [transaction.account],
   );
 
-  // We need to enforce certain constraints with child transaction transfers
-  if (transaction.parent_id) {
-    const row = await db.first(
-      `
-        SELECT p.id, p.transfer_acct FROM v_transactions t
-        LEFT JOIN payees p ON p.id = t.payee
-        WHERE t.id = ?
-      `,
-      [transaction.parent_id],
-    );
-
-    if (row.transfer_acct) {
-      if (row.id !== transaction.payee) {
-        // This child transaction is trying to use a transfer payee,
-        // but the parent is already using a different transfer payee.
-        // This is not allowed, so not only do we do nothing, we clear
-        // the payee of the child transaction to make it clear
-        await db.updateTransaction({ id: transaction.id, payee: null });
-        return { id: transaction.id, payee: null };
-      }
-    }
-  }
-
   const id = await db.insertTransaction({
     account: transferredAccount,
     amount: -transaction.amount,

upcoming-release-notes/4255.md

@@ -0,0 +1,6 @@
+---
+category: Bugfix
+authors: [jfdoming]
+---
+
+Allow child transactions to have different transfer payees