From 22e4a1ead0601e096fd939bf6a0f29f9f147628b Mon Sep 17 00:00:00 2001 From: siddharthd Date: Wed, 29 Jul 2026 10:23:06 +1000 Subject: [PATCH] fix(splits): make every split account for 100% MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 50/50 arrangement was stored as a single row saying "Sonu 50%". The arithmetic was never wrong — `myShare` resolves the payer's share as `100 - SUM(everyone else)`, so balances and per-user spend were correct throughout. It was still a bug, because a ledger is read as well as computed: on screen that row is a 50% share against a blank, which looks like half the money is unallocated and is indistinguishable from a split somebody abandoned half-finished. It also leaked. `getSharedTransactions` filters by participant with an EXISTS on an explicit split row, so filtering the Shared view by the payer silently dropped every transaction where their share was only ever implied. Four write paths could produce it, three of them unguarded: - the Slack nudge's share button, which inserted one row - `POST /api/transactions`, where the add form shows an amber total under 100 but saves anyway — this is how Lawn Mowing and Hedge Pruning were stored - `applyRuleActions`, where ten of the fourteen live split rules name only the other person `completeSplit` is now the single place that writes the remainder, and every one of those paths ends in it. The remainder goes to the transaction's *owner*, never to "me": the owner's row on their own transaction is excluded from both halves of the balance query, so it cannot create, enlarge or discharge a debt, whereas a row for me on someone else's transaction is a real obligation. That distinction is what makes this safe to apply to existing data. Also fixes the order panel's "Shared 50/50" toggle, which was inert in both directions: it posted a lone 50% row to share (rejected — must total 100%) and an empty array to un-share (rejected — array required), because no way to clear a split existed. DELETE on the splits route is that way. Backfill: 7 rows, verified against a row-level dump diff — 2549 -> 2556 rows, none removed, none modified — and participant balances byte identical before and after (Molina 19556.07, Sonu 20913.35). Every split in the database now totals 100%. Not done: a database-level constraint. Enforcing the sum needs a deferred constraint trigger, and the rule path commits its DELETE and INSERT as separate statements, so the trigger would reject the intermediate state. Making it work means wrapping every write path in a transaction, which is a larger change than the defect warrants. --- src/__tests__/integration/splits.test.ts | 304 ++++++++++++++++++ src/app/api/rules/apply/[id]/revert/route.ts | 6 + src/app/api/slack/interactive/route.ts | 6 + src/app/api/transactions/[id]/splits/route.ts | 24 ++ src/app/api/transactions/route.ts | 6 + src/components/order-details.tsx | 22 +- src/lib/hooks.ts | 30 ++ src/lib/rule-actions.ts | 5 + src/lib/splits.ts | 94 ++++++ 9 files changed, 490 insertions(+), 7 deletions(-) create mode 100644 src/__tests__/integration/splits.test.ts create mode 100644 src/lib/splits.ts diff --git a/src/__tests__/integration/splits.test.ts b/src/__tests__/integration/splits.test.ts new file mode 100644 index 0000000..1aec6f5 --- /dev/null +++ b/src/__tests__/integration/splits.test.ts @@ -0,0 +1,304 @@ +import { describe, it, expect, beforeAll, afterAll, beforeEach } from "vitest"; +import type { Pool } from "pg"; +import { + createPool, + mockDbWithPool, + resetDB, + seedParticipants, + insertTransaction, +} from "./helpers"; + +/** + * Every split adds up to 100%. + * + * The failure this guards against is not an arithmetic one — `myShare` has + * always treated the payer's share as the remainder, so the numbers were right. + * It is that the remainder was never written down, so a 50/50 arrangement was + * stored as a single row reading "Sonu 50%" and displayed as half a split. + */ +describe("completeSplit", () => { + let pool: Pool; + let completeSplit: (id: number) => Promise; + + beforeAll(async () => { + pool = createPool(); + mockDbWithPool(pool); + ({ completeSplit } = await import("@/lib/splits")); + }); + + afterAll(async () => { + await pool.end(); + }); + + beforeEach(async () => { + await resetDB(pool); + }); + + const sharesOf = async (txId: number) => { + const r = await pool.query( + `SELECT participant_id, share_percent::float FROM transaction_splits + WHERE transaction_id = $1 ORDER BY participant_id`, + [txId] + ); + return r.rows as { participant_id: number; share_percent: number }[]; + }; + + it("writes the payer's half of a 50/50 recorded as one row", async () => { + const { ownerId, otherId } = await seedParticipants(pool); + const txId = await insertTransaction(pool, ownerId); + await pool.query( + `INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) + VALUES ($1, $2, 50)`, + [txId, otherId] + ); + + await completeSplit(txId); + + expect(await sharesOf(txId)).toEqual([ + { participant_id: ownerId, share_percent: 50 }, + { participant_id: otherId, share_percent: 50 }, + ]); + }); + + it("leaves an unsplit transaction unsplit", async () => { + // A transaction nobody shares is not a 100% split of itself. Writing one + // would put every row in the Shared view. + const { ownerId } = await seedParticipants(pool); + const txId = await insertTransaction(pool, ownerId); + + await completeSplit(txId); + + expect(await sharesOf(txId)).toEqual([]); + }); + + it("adds nothing when the other party owes all of it", async () => { + const { ownerId, otherId } = await seedParticipants(pool); + const txId = await insertTransaction(pool, ownerId); + await pool.query( + `INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) + VALUES ($1, $2, 100)`, + [txId, otherId] + ); + + await completeSplit(txId); + + expect(await sharesOf(txId)).toEqual([ + { participant_id: otherId, share_percent: 100 }, + ]); + }); + + it("removes the owner's row when the others grow to cover the whole amount", async () => { + // A 50/50 revised to "they owe all of it". The owner's share becomes zero, + // and a 0% row cannot be stored anyway — `share_percent > 0` is a CHECK + // constraint — so the row has to go rather than be zeroed. + const { ownerId, otherId } = await seedParticipants(pool); + const txId = await insertTransaction(pool, ownerId); + await pool.query( + `INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) + VALUES ($1, $2, 100), ($1, $3, 50)`, + [txId, otherId, ownerId] + ); + + await completeSplit(txId); + + expect(await sharesOf(txId)).toEqual([ + { participant_id: otherId, share_percent: 100 }, + ]); + }); + + it("fills the remainder for a three-way split, not a half", async () => { + const { ownerId, otherId } = await seedParticipants(pool); + const third = ( + await pool.query(`INSERT INTO participants (name) VALUES ('Carol') RETURNING id`) + ).rows[0].id as number; + const txId = await insertTransaction(pool, ownerId); + await pool.query( + `INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) + VALUES ($1, $2, 50), ($1, $3, 25)`, + [txId, otherId, third] + ); + + await completeSplit(txId); + + expect(await sharesOf(txId)).toEqual([ + { participant_id: ownerId, share_percent: 25 }, + { participant_id: otherId, share_percent: 50 }, + { participant_id: third, share_percent: 25 }, + ]); + }); + + it("leaves an over-allocated split alone instead of trimming someone's share", async () => { + // >100% is a caller's mistake. Silently deleting a share to force the total + // down would destroy the evidence of it. + // No single row may exceed 100 (CHECK constraint), but two can add up past + // it — 60 + 60 is how an over-allocated split actually arrives. + const { ownerId, otherId } = await seedParticipants(pool); + const third = ( + await pool.query(`INSERT INTO participants (name) VALUES ('Dave') RETURNING id`) + ).rows[0].id as number; + const txId = await insertTransaction(pool, ownerId); + await pool.query( + `INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) + VALUES ($1, $2, 60), ($1, $3, 60)`, + [txId, otherId, third] + ); + + await completeSplit(txId); + + expect(await sharesOf(txId)).toEqual([ + { participant_id: otherId, share_percent: 60 }, + { participant_id: third, share_percent: 60 }, + ]); + }); + + it("gives the remainder to the statement's owner when the row has none", async () => { + // Statement rows carry no owner_id of their own; it comes from the + // statement. Transaction 3828 was one of these. + const { ownerId, otherId } = await seedParticipants(pool); + const stmt = await pool.query( + `INSERT INTO statements (owner_id, filename, bank_name, account_number, billing_start_date, billing_end_date) + VALUES ($1, 'test.pdf', 'Test Bank', '0001', '2026-06-01', '2026-06-30') RETURNING id`, + [ownerId] + ); + const tx = await pool.query( + `INSERT INTO transactions (owner_id, statement_id, transaction_date, description, amount, transaction_type, row_index) + VALUES (NULL, $1, '2026-06-15', 'Statement row', 29.17, 'debit', 0) RETURNING id`, + [stmt.rows[0].id] + ); + const txId = tx.rows[0].id as number; + await pool.query( + `INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) + VALUES ($1, $2, 50)`, + [txId, otherId] + ); + + await completeSplit(txId); + + expect(await sharesOf(txId)).toEqual([ + { participant_id: ownerId, share_percent: 50 }, + { participant_id: otherId, share_percent: 50 }, + ]); + }); + + it("never puts my share on someone else's transaction", async () => { + // The remainder goes to the transaction's owner, never to "me". A row for + // me on a transaction I do not own is a debt I owe, and this helper must + // not invent one. + const { ownerId, otherId } = await seedParticipants(pool); + const txId = await insertTransaction(pool, otherId); // they paid + await pool.query( + `INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) + VALUES ($1, $2, 50)`, + [txId, ownerId] + ); + + await completeSplit(txId); + + const shares = await sharesOf(txId); + expect(shares).toEqual([ + { participant_id: ownerId, share_percent: 50 }, + { participant_id: otherId, share_percent: 50 }, + ]); + // My share is unchanged — the new row belongs to the payer. + expect(shares.find((s) => s.participant_id === ownerId)?.share_percent).toBe(50); + }); + + it("does not disturb a settled split", async () => { + // Adding the payer's row must not touch anyone else's `settled` flag — + // that is how $37k of discharged debt gets resurrected. + const { ownerId, otherId } = await seedParticipants(pool); + const txId = await insertTransaction(pool, ownerId); + await pool.query( + `INSERT INTO transaction_splits (transaction_id, participant_id, share_percent, settled) + VALUES ($1, $2, 50, true)`, + [txId, otherId] + ); + + await completeSplit(txId); + + const r = await pool.query( + `SELECT settled FROM transaction_splits + WHERE transaction_id = $1 AND participant_id = $2`, + [txId, otherId] + ); + expect(r.rows[0].settled).toBe(true); + }); + + it("is idempotent", async () => { + const { ownerId, otherId } = await seedParticipants(pool); + const txId = await insertTransaction(pool, ownerId); + await pool.query( + `INSERT INTO transaction_splits (transaction_id, participant_id, share_percent) + VALUES ($1, $2, 50)`, + [txId, otherId] + ); + + await completeSplit(txId); + const once = await sharesOf(txId); + await completeSplit(txId); + expect(await sharesOf(txId)).toEqual(once); + }); +}); + +/** + * The rule path, which is where ten of the live rules write a single share. + */ +describe("applyRuleActions completes the split", () => { + let pool: Pool; + let applyRuleActions: ( + id: number, + actions: { apply_split?: { participant_id: number; share_percent: number }[] } + ) => Promise; + + beforeAll(async () => { + pool = createPool(); + mockDbWithPool(pool); + ({ applyRuleActions } = await import("@/lib/rule-actions")); + }); + + afterAll(async () => { + await pool.end(); + }); + + beforeEach(async () => { + await resetDB(pool); + }); + + it("writes the payer's half for a rule that names only the other person", async () => { + const { ownerId, otherId } = await seedParticipants(pool); + const txId = await insertTransaction(pool, ownerId, { description: "Woolworths" }); + + await applyRuleActions(txId, { + apply_split: [{ participant_id: otherId, share_percent: 50 }], + }); + + const r = await pool.query( + `SELECT participant_id, share_percent::float FROM transaction_splits + WHERE transaction_id = $1 ORDER BY participant_id`, + [txId] + ); + expect(r.rows).toEqual([ + { participant_id: ownerId, share_percent: 50 }, + { participant_id: otherId, share_percent: 50 }, + ]); + }); + + it("leaves a rule that already names both alone", async () => { + const { ownerId, otherId } = await seedParticipants(pool); + const txId = await insertTransaction(pool, ownerId); + + await applyRuleActions(txId, { + apply_split: [ + { participant_id: ownerId, share_percent: 50 }, + { participant_id: otherId, share_percent: 50 }, + ], + }); + + const r = await pool.query( + `SELECT sum(share_percent)::float AS total, count(*)::int AS n + FROM transaction_splits WHERE transaction_id = $1`, + [txId] + ); + expect(r.rows[0]).toEqual({ total: 100, n: 2 }); + }); +}); diff --git a/src/app/api/rules/apply/[id]/revert/route.ts b/src/app/api/rules/apply/[id]/revert/route.ts index c600585..9f739f3 100644 --- a/src/app/api/rules/apply/[id]/revert/route.ts +++ b/src/app/api/rules/apply/[id]/revert/route.ts @@ -1,6 +1,7 @@ import { NextRequest, NextResponse } from "next/server"; import { getCurrentUser } from "@/lib/auth"; import { queryRaw } from "@/lib/db"; +import { completeSplit } from "@/lib/splits"; interface SnapshotEntry { transaction_id: number; @@ -91,6 +92,11 @@ export async function POST(req: NextRequest, { params }: { params: Promise<{ id: [txId, s.participant_id, s.share_percent, s.settled] ); } + // A snapshot taken before splits were required to total 100% holds the old + // partial shape, and restoring it verbatim would reintroduce exactly what + // this run is being undone from. The owner's share is balance-neutral, so + // completing it cannot change what the revert owes anyone. + await completeSplit(txId); } await queryRaw( diff --git a/src/app/api/slack/interactive/route.ts b/src/app/api/slack/interactive/route.ts index a8a34fd..a622ccd 100644 --- a/src/app/api/slack/interactive/route.ts +++ b/src/app/api/slack/interactive/route.ts @@ -6,6 +6,7 @@ import { slackUserForParticipant, } from "@/lib/slack-verify"; import { nudgeBlocks, detailsModal, partnerNudgeBlocks } from "@/lib/slack-blocks"; +import { completeSplit } from "@/lib/splits"; import { RATINGS, OWNER_PARTICIPANT_ID, @@ -330,6 +331,11 @@ async function toggleShare(transactionId: number): Promise { DO UPDATE SET share_percent = 50`, [transactionId, SECOND_CONSUMER_ID] ); + // Both halves, not just theirs. The button means "50/50", and a lone row for + // the other person renders as a 50% share against a blank. The payer's half + // comes from completeSplit rather than a second hardcoded insert, so it lands + // on whoever actually owns the row instead of assuming that is me. + await completeSplit(transactionId); return null; } diff --git a/src/app/api/transactions/[id]/splits/route.ts b/src/app/api/transactions/[id]/splits/route.ts index 28587b5..d8a35d9 100644 --- a/src/app/api/transactions/[id]/splits/route.ts +++ b/src/app/api/transactions/[id]/splits/route.ts @@ -41,6 +41,30 @@ export async function GET( return NextResponse.json(splits); } +/** + * Remove every split — un-share the transaction. + * + * POST cannot express this: it requires shares totalling 100%, and an empty + * array is not that. Without this the order panel's "Shared 50/50" toggle had + * no way back, and pressing it to un-share failed with "splits array required". + */ +export async function DELETE( + req: NextRequest, + { params }: { params: Promise<{ id: string }> } +) { + const user = await getCurrentUser(req); + if (!user) return NextResponse.json({ error: "Unauthorized" }, { status: 403 }); + const { id } = await params; + const transactionId = Number(id); + if (!(await canAccessTransactions(user.id, [transactionId]))) { + return NextResponse.json({ error: "Not found" }, { status: 404 }); + } + const removed = await prisma.transaction_splits.deleteMany({ + where: { transaction_id: transactionId }, + }); + return NextResponse.json({ removed: removed.count }); +} + export async function POST( req: NextRequest, { params }: { params: Promise<{ id: string }> } diff --git a/src/app/api/transactions/route.ts b/src/app/api/transactions/route.ts index 3b48df6..2e7e7b6 100644 --- a/src/app/api/transactions/route.ts +++ b/src/app/api/transactions/route.ts @@ -2,6 +2,7 @@ import { NextRequest, NextResponse } from "next/server"; import { getCurrentUser } from "@/lib/auth"; import { getTransactions } from "@/lib/queries"; import { queryRaw } from "@/lib/db"; +import { completeSplit } from "@/lib/splits"; export async function GET(req: NextRequest) { const user = await getCurrentUser(req); @@ -79,6 +80,11 @@ export async function POST(req: NextRequest) { [transactionId, s.participant_id, s.share_percent] ); } + // The form lets you name just the other person and shows the total in amber + // when it is under 100 — which is how "Lawn Mowing, Sonu 50%" was saved with + // the other half nowhere. Fill in the payer's share rather than refusing: + // naming only the other person is a reasonable thing to mean. + await completeSplit(transactionId); } return NextResponse.json({ id: transactionId }, { status: 201 }); diff --git a/src/components/order-details.tsx b/src/components/order-details.tsx index d76bf54..fee2b54 100644 --- a/src/components/order-details.tsx +++ b/src/components/order-details.tsx @@ -7,6 +7,7 @@ import { useParticipants, useSetOrderReview, useSetSplits, + useClearSplits, type ItemOpinion, type ItemVerdict, type OrderReceipt, @@ -249,20 +250,27 @@ function SharedToggle({ otherName: string; }) { const setSplits = useSetSplits(); + const clearSplits = useClearSplits(); const shared = splits.some((s) => s.participant_id === SECOND_CONSUMER_ID); return (