fix(transactions): supersede rows imported twice instead of deleting them
ci / lint-test (push) Successful in 48s
ci / lint-test (push) Successful in 48s
Statements 107, 142 and 143 bill overlapping periods on one ANZ account, so 31 transactions -- $42,040.68 -- are in the ledger twice. They are marked superseded, not deleted. Every child of transactions is ON DELETE CASCADE (splits, tags, overrides, expense_metadata, order_reviews), so deleting "the duplicate" destroys whatever curation sits on it, and which member of a pair holds that curation is an accident of import order: here 1 pair carries splits and 6 carry overrides, all on the surviving side, but nothing guarantees that. Superseding keeps the row, keeps its children, and makes a mistake one UPDATE to undo rather than a restore from backup. reconciled_with_id could not be reused. Its predicate is scoped to statement_id IS NULL on purpose -- a statement line pointing at something else is the survivor, not the duplicate -- and here both rows are statement lines. The exclusion goes into EXCLUDE_RECONCILED_SOURCE rather than into a new fragment, so every query already asking "count each purchase once" gets it without being edited. The trip cost queries did not use that fragment at all and now do; verified a no-op on current data (0 trip-tagged rows are either reconciled sources or duplicates), but they were one import away from double-counting. Most of the $42k is transfers and investments, which spend already excludes. The damage was elsewhere: duplicated rows in the list, and rules re-splitting a duplicate -- txn 3807 is one of these 31 and was a candidate for splitting earlier today. Balances are unchanged: no duplicate carried a split.
This commit is contained in:
@@ -0,0 +1,37 @@
|
|||||||
|
-- A transaction imported twice cannot simply be deleted.
|
||||||
|
--
|
||||||
|
-- Every child of `transactions` is ON DELETE CASCADE — splits, tags, overrides,
|
||||||
|
-- expense_metadata, order_reviews. Deleting a row said to be "the duplicate"
|
||||||
|
-- therefore destroys whatever curation happens to sit on it, silently and
|
||||||
|
-- unrecoverably. The curation is not reliably on the surviving side either: of
|
||||||
|
-- the 31 known duplicate pairs, one carries splits and six carry overrides, and
|
||||||
|
-- which member holds them is an accident of import order.
|
||||||
|
--
|
||||||
|
-- So a duplicate is superseded, never removed. The row stays, keeps its
|
||||||
|
-- children, and points at the row that replaces it. Reversing a mistake is then
|
||||||
|
-- one UPDATE rather than a restore from backup.
|
||||||
|
--
|
||||||
|
-- This is the statement-vs-statement case. `reconciled_with_id` already covers
|
||||||
|
-- manual-vs-statement, and deliberately cannot be reused: the predicate that
|
||||||
|
-- hides a reconciled row is scoped to `statement_id IS NULL`, because a
|
||||||
|
-- statement line pointing at something else is the survivor, not the duplicate.
|
||||||
|
-- Both of these rows are statement lines.
|
||||||
|
|
||||||
|
ALTER TABLE transactions
|
||||||
|
ADD COLUMN IF NOT EXISTS superseded_by_id integer
|
||||||
|
REFERENCES transactions(id) ON DELETE SET NULL;
|
||||||
|
|
||||||
|
COMMENT ON COLUMN transactions.superseded_by_id IS
|
||||||
|
'This row was imported twice; the named row is the one that counts. Excluded from every figure, kept for its children and its audit trail. NULL = live.';
|
||||||
|
|
||||||
|
CREATE INDEX IF NOT EXISTS idx_transactions_superseded
|
||||||
|
ON transactions (superseded_by_id)
|
||||||
|
WHERE superseded_by_id IS NOT NULL;
|
||||||
|
|
||||||
|
-- A row cannot supersede itself, and a survivor cannot itself be superseded
|
||||||
|
-- (that would hide both members of the pair and lose the amount entirely).
|
||||||
|
ALTER TABLE transactions
|
||||||
|
DROP CONSTRAINT IF EXISTS transactions_no_self_supersede;
|
||||||
|
ALTER TABLE transactions
|
||||||
|
ADD CONSTRAINT transactions_no_self_supersede
|
||||||
|
CHECK (superseded_by_id IS NULL OR superseded_by_id <> id);
|
||||||
@@ -183,11 +183,14 @@ model transactions {
|
|||||||
payment_method String? // card | cash | bank_transfer | other; NULL = unknown (migration 0016)
|
payment_method String? // card | cash | bank_transfer | other; NULL = unknown (migration 0016)
|
||||||
owner_id Int?
|
owner_id Int?
|
||||||
reconciled_with_id Int?
|
reconciled_with_id Int?
|
||||||
|
superseded_by_id Int?
|
||||||
principal_amount Decimal? @db.Decimal(12, 2)
|
principal_amount Decimal? @db.Decimal(12, 2)
|
||||||
interest_amount Decimal? @db.Decimal(12, 2)
|
interest_amount Decimal? @db.Decimal(12, 2)
|
||||||
statement statements? @relation(fields: [statement_id], references: [id], onDelete: Cascade)
|
statement statements? @relation(fields: [statement_id], references: [id], onDelete: Cascade)
|
||||||
reconciled_with transactions? @relation("reconciled", fields: [reconciled_with_id], references: [id], onDelete: SetNull)
|
reconciled_with transactions? @relation("reconciled", fields: [reconciled_with_id], references: [id], onDelete: SetNull)
|
||||||
reconciled_by transactions[] @relation("reconciled")
|
reconciled_by transactions[] @relation("reconciled")
|
||||||
|
superseded_by transactions? @relation("superseded", fields: [superseded_by_id], references: [id], onDelete: SetNull)
|
||||||
|
supersedes transactions[] @relation("superseded")
|
||||||
expense_metadata expense_metadata?
|
expense_metadata expense_metadata?
|
||||||
order_review order_reviews?
|
order_review order_reviews?
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -608,3 +608,61 @@ describe("getStatements — overlapping billing periods", () => {
|
|||||||
expect(rows.every((r) => r.overlaps.length === 0)).toBe(true);
|
expect(rows.every((r) => r.overlaps.length === 0)).toBe(true);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// A statement imported twice puts every transaction in the overlap in the
|
||||||
|
// ledger twice. The duplicate is superseded rather than deleted, because every
|
||||||
|
// child of `transactions` cascades on delete.
|
||||||
|
describe("superseded duplicates are excluded but kept", () => {
|
||||||
|
it("hides a superseded row from the transaction list", async () => {
|
||||||
|
const { ownerId } = await seedParticipants(pool);
|
||||||
|
const keep = await insertTransaction(pool, ownerId, { description: "RAIZ INVESTMENT", amount: 1500 });
|
||||||
|
const dup = await insertTransaction(pool, ownerId, { description: "RAIZ INVESTMENT", amount: 1500 });
|
||||||
|
await pool.query(`UPDATE transactions SET superseded_by_id = $1 WHERE id = $2`, [keep, dup]);
|
||||||
|
|
||||||
|
const { data, total } = await getTransactions(ownerId, { limit: 50, offset: 0 });
|
||||||
|
expect(total).toBe(1);
|
||||||
|
expect(data.map((t) => t.id)).toEqual([keep]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("keeps the superseded row and its children in the database", async () => {
|
||||||
|
const { ownerId, otherId } = await seedParticipants(pool);
|
||||||
|
const keep = await insertTransaction(pool, ownerId, { amount: 100 });
|
||||||
|
const dup = await insertTransaction(pool, ownerId, { amount: 100 });
|
||||||
|
await pool.query(
|
||||||
|
`INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) VALUES ($1, $2, 50)`,
|
||||||
|
[dup, otherId]
|
||||||
|
);
|
||||||
|
await pool.query(`UPDATE transactions SET superseded_by_id = $1 WHERE id = $2`, [keep, dup]);
|
||||||
|
|
||||||
|
const rows = await pool.query(`SELECT superseded_by_id FROM transactions WHERE id = $1`, [dup]);
|
||||||
|
expect(rows.rows[0].superseded_by_id).toBe(keep);
|
||||||
|
const kids = await pool.query(`SELECT count(*)::int AS n FROM transaction_splits WHERE transaction_id = $1`, [dup]);
|
||||||
|
expect(kids.rows[0].n).toBe(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The point of excluding it: a split on a duplicate must not be owed twice.
|
||||||
|
it("does not count a superseded row towards what someone owes", async () => {
|
||||||
|
const { ownerId, otherId } = await seedParticipants(pool);
|
||||||
|
const keep = await insertTransaction(pool, ownerId, { amount: 100 });
|
||||||
|
const dup = await insertTransaction(pool, ownerId, { amount: 100 });
|
||||||
|
for (const id of [keep, dup]) {
|
||||||
|
await pool.query(
|
||||||
|
`INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) VALUES ($1, $2, 50)`,
|
||||||
|
[id, otherId]
|
||||||
|
);
|
||||||
|
}
|
||||||
|
await pool.query(`UPDATE transactions SET superseded_by_id = $1 WHERE id = $2`, [keep, dup]);
|
||||||
|
|
||||||
|
const balances = await getParticipantBalances(ownerId);
|
||||||
|
const bob = balances.find((b) => b.id === otherId);
|
||||||
|
expect(Number(bob!.total_owed)).toBeCloseTo(50);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("refuses to let a row supersede itself", async () => {
|
||||||
|
const { ownerId } = await seedParticipants(pool);
|
||||||
|
const id = await insertTransaction(pool, ownerId);
|
||||||
|
await expect(
|
||||||
|
pool.query(`UPDATE transactions SET superseded_by_id = $1 WHERE id = $1`, [id])
|
||||||
|
).rejects.toThrow();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -36,7 +36,9 @@ export const OWNER_SCOPE = `COALESCE(t.owner_id, s.owner_id)`;
|
|||||||
export const STATEMENTS_JOIN = `LEFT JOIN statements s ON s.id = t.statement_id`;
|
export const STATEMENTS_JOIN = `LEFT JOIN statements s ON s.id = t.statement_id`;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Drops the manual/CSV row that a statement line has superseded.
|
* Drops rows that a different row has replaced. Two distinct cases:
|
||||||
|
*
|
||||||
|
* **1. The manual/CSV row a statement line superseded** (`reconciled_with_id`).
|
||||||
*
|
*
|
||||||
* Reconciliation keeps both rows: the manual one the user entered and the
|
* Reconciliation keeps both rows: the manual one the user entered and the
|
||||||
* statement line it turned out to be. Only the statement line should count, or
|
* statement line it turned out to be. Only the statement line should count, or
|
||||||
@@ -52,8 +54,26 @@ export const STATEMENTS_JOIN = `LEFT JOIN statements s ON s.id = t.statement_id`
|
|||||||
* inserted with `reconciled_with_id` NULL and are held out of the reconcile
|
* inserted with `reconciled_with_id` NULL and are held out of the reconcile
|
||||||
* queue by `needsCardMatch()`, so nothing ever sets it. If one is reconciled by
|
* queue by `needsCardMatch()`, so nothing ever sets it. If one is reconciled by
|
||||||
* hand against a card line, this is what stops it double-counting.
|
* hand against a card line, this is what stops it double-counting.
|
||||||
|
*
|
||||||
|
* **2. The statement row imported twice** (`superseded_by_id`, migration 0023).
|
||||||
|
*
|
||||||
|
* When two statements for one account bill overlapping periods, every
|
||||||
|
* transaction in the overlap arrives twice — 31 pairs on ANZ `4085-56264`, from
|
||||||
|
* statements 107/142/143. Case 1 cannot express this: its predicate is scoped
|
||||||
|
* to `statement_id IS NULL` on purpose, and here BOTH rows are statement lines.
|
||||||
|
*
|
||||||
|
* The superseded row is excluded rather than deleted because every child of
|
||||||
|
* `transactions` cascades on delete, and the curation is not reliably on the
|
||||||
|
* surviving side.
|
||||||
|
*
|
||||||
|
* Adding it to this fragment rather than making a new one is deliberate: every
|
||||||
|
* query that already asks "count each purchase once" now excludes both kinds
|
||||||
|
* without being edited. Anything summing transactions without this fragment
|
||||||
|
* still double-counts — that is the same gap that let reconciled rows into the
|
||||||
|
* analytics routes in the first place.
|
||||||
*/
|
*/
|
||||||
export const EXCLUDE_RECONCILED_SOURCE = `NOT (t.statement_id IS NULL AND t.reconciled_with_id IS NOT NULL)`;
|
export const EXCLUDE_RECONCILED_SOURCE = `NOT (t.statement_id IS NULL AND t.reconciled_with_id IS NOT NULL)
|
||||||
|
AND t.superseded_by_id IS NULL`;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The currency `t.amount` is actually denominated in.
|
* The currency `t.amount` is actually denominated in.
|
||||||
|
|||||||
@@ -897,6 +897,7 @@ export interface TripAnalytics {
|
|||||||
// alias the shared fragments assume.
|
// alias the shared fragments assume.
|
||||||
const TRIP_TOTAL_SPEND = `COALESCE(SUM(
|
const TRIP_TOTAL_SPEND = `COALESCE(SUM(
|
||||||
CASE WHEN ${NET_SPEND_ROWS}
|
CASE WHEN ${NET_SPEND_ROWS}
|
||||||
|
AND ${EXCLUDE_RECONCILED_SOURCE}
|
||||||
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
||||||
THEN ${SPEND_SIGNED} ELSE 0 END
|
THEN ${SPEND_SIGNED} ELSE 0 END
|
||||||
), 0)::float AS total_spend`;
|
), 0)::float AS total_spend`;
|
||||||
@@ -964,6 +965,7 @@ export async function getTripAnalytics(tripId: number, ownerId: number): Promise
|
|||||||
JOIN transactions t ON t.id = o.transaction_id
|
JOIN transactions t ON t.id = o.transaction_id
|
||||||
WHERE o.trip_id = $1
|
WHERE o.trip_id = $1
|
||||||
AND ${NET_SPEND_ROWS}
|
AND ${NET_SPEND_ROWS}
|
||||||
|
AND ${EXCLUDE_RECONCILED_SOURCE}
|
||||||
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
||||||
GROUP BY 1
|
GROUP BY 1
|
||||||
ORDER BY 2 DESC
|
ORDER BY 2 DESC
|
||||||
@@ -977,6 +979,7 @@ export async function getTripAnalytics(tripId: number, ownerId: number): Promise
|
|||||||
JOIN transactions t ON t.id = o.transaction_id
|
JOIN transactions t ON t.id = o.transaction_id
|
||||||
WHERE o.trip_id = $1
|
WHERE o.trip_id = $1
|
||||||
AND ${NET_SPEND_ROWS}
|
AND ${NET_SPEND_ROWS}
|
||||||
|
AND ${EXCLUDE_RECONCILED_SOURCE}
|
||||||
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
||||||
GROUP BY 1
|
GROUP BY 1
|
||||||
ORDER BY 1
|
ORDER BY 1
|
||||||
@@ -991,6 +994,7 @@ export async function getTripAnalytics(tripId: number, ownerId: number): Promise
|
|||||||
JOIN transactions t ON t.id = o.transaction_id
|
JOIN transactions t ON t.id = o.transaction_id
|
||||||
WHERE o.trip_id = $1
|
WHERE o.trip_id = $1
|
||||||
AND ${NET_SPEND_ROWS}
|
AND ${NET_SPEND_ROWS}
|
||||||
|
AND ${EXCLUDE_RECONCILED_SOURCE}
|
||||||
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
||||||
GROUP BY 1
|
GROUP BY 1
|
||||||
ORDER BY 2 DESC
|
ORDER BY 2 DESC
|
||||||
@@ -1008,6 +1012,7 @@ export async function getTripAnalytics(tripId: number, ownerId: number): Promise
|
|||||||
JOIN tags tg ON tg.id = tt.tag_id
|
JOIN tags tg ON tg.id = tt.tag_id
|
||||||
WHERE o.trip_id = $1
|
WHERE o.trip_id = $1
|
||||||
AND ${NET_SPEND_ROWS}
|
AND ${NET_SPEND_ROWS}
|
||||||
|
AND ${EXCLUDE_RECONCILED_SOURCE}
|
||||||
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
AND COALESCE(o.category_override, t.category, 'other') NOT IN ('transfers', 'investment')
|
||||||
GROUP BY tg.id
|
GROUP BY tg.id
|
||||||
ORDER BY 4 DESC
|
ORDER BY 4 DESC
|
||||||
|
|||||||
Reference in New Issue
Block a user