diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index 37d8d592c..83858b4d5 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -765,6 +765,24 @@ export class BudgetService { const payers = allPayers.filter(p => p.budget_item_id === item.id); if (members.length === 0) continue; // planning-only entry → doesn't affect balances + // An expense nobody has paid stays out of the ledger (#2225), which is what + // both cost panels already promise by flagging the row "Unfinished" (the + // hint beside it reads: total only, not settled yet) and counting it into + // the Outstanding card instead (CostsPanel/costsModel.isUnfinished). The + // panels additionally require a non-zero total to paint that label; the + // ledger deliberately does not, or PUT /budget/:id/payers with an empty + // array (which zeroes total_price and does not re-apply it) would leave a + // custom split debiting its members against no credit at all. + // Charging its split members regardless, as #1382/#2176/#1543 left in + // place below, debits a share with no credit behind it: Σ(balances) drifts + // off zero by the unpaid total, and the greedy simplifier at the end of this + // method has no memory of which expense made which debt, so it pairs the + // inflated debtor with a creditor from some unrelated expense and offers a + // payment between two people who never shared a bill. Booking every flow it + // offered then left someone short: "everyone square" beside a balance that + // is not zero. + if (!payers.some(p => p.amount !== 0)) continue; + // Payers are credited what they actually paid (converted to trip currency with // the item's stored exchange rate). A negative payer — the recipient of a // refund (#2176) — is debited by the same arithmetic: their credit is negative. @@ -781,14 +799,12 @@ export class BudgetService { // total_price: the two are the same figure (the write path derives the total // from the payer sum), but converting the total separately would round to a // different cent on a foreign-currency expense and leave the item off by one. - // With nobody down as a payer there is nothing to divide, so the recorded total - // stands in and an unpaid bill keeps reading as money owed. Negative credits - // (a refund, #2176) divide the same way — only exactly zero falls back. + // Negative credits (a refund, #2176) divide the same way. Nothing falls back + // to total_price any more: an item with no payer behind it never reaches + // here since #2225. An item whose payers net to exactly zero still does, and + // divides zero, which is what it is worth. const hasCustomSplit = members.some(m => m.amount !== null && m.amount !== undefined); - const splitCents = creditCents !== 0 - ? creditCents - : toTripCents(item.total_price, item.currency, item.exchange_rate); - const equalShares = !hasCustomSplit ? this.splitEqualShares(splitCents, members, item.id) : {}; + const equalShares = !hasCustomSplit ? this.splitEqualShares(creditCents, members, item.id) : {}; for (const m of members) { const memberShare = hasCustomSplit && m.amount !== null && m.amount !== undefined ? toTripCents(m.amount, item.currency, item.exchange_rate) diff --git a/server/tests/e2e/budget.e2e.test.ts b/server/tests/e2e/budget.e2e.test.ts index ba86fd587..23c7c5958 100644 --- a/server/tests/e2e/budget.e2e.test.ts +++ b/server/tests/e2e/budget.e2e.test.ts @@ -190,6 +190,44 @@ describe('Budget e2e (real auth guard + temp SQLite, real budget SQL)', () => { } }); + it('an expense with no payer creates no debt and no flow (#2225)', async () => { + // The shape from the report: one bill user 1 fronted for both, plus a row + // saved with "No one paid yet" that only user 2 is on. The taxi used to be + // debited with no credit behind it and its whole 60 offered to user 1. + const dinner = await request(server) + .post(`/api/trips/${tripId}/budget`) + .set('Cookie', sessionCookie(1)) + .send({ name: 'Dinner', payers: [{ user_id: 1, amount: 100 }], member_ids: [1, 2] }); + expect(dinner.status).toBe(201); + + const unpaid = await request(server) + .post(`/api/trips/${tripId}/budget`) + .set('Cookie', sessionCookie(1)) + .send({ name: 'Taxi', total_price: 60, payers: [], member_ids: [2] }); + expect(unpaid.status).toBe(201); + expect(unpaid.body.item).toMatchObject({ total_price: 60, payers: [] }); + + const settlement = await request(server) + .get(`/api/trips/${tripId}/budget/settlement`) + .set('Cookie', sessionCookie(1)); + expect(settlement.status).toBe(200); + const balances = settlement.body.balances as { user_id: number; balance: number }[]; + const balance = (uid: number) => balances.find(b => b.user_id === uid)!.balance; + // Only the dinner settles: user 2 owes half of it and not a cent of the taxi. + expect(balance(1)).toBe(50); + expect(balance(2)).toBe(-50); + expect(balances.reduce((a, b) => a + Math.round(b.balance * 100), 0)).toBe(0); + expect(settlement.body.flows).toEqual([ + expect.objectContaining({ amount: 50, from: expect.objectContaining({ user_id: 2 }), to: expect.objectContaining({ user_id: 1 }) }), + ]); + + // Clean up so the ledger tests below start from an empty trip. + for (const id of [dinner.body.item.id, unpaid.body.item.id]) { + const del = await request(server).delete(`/api/trips/${tripId}/budget/${id}`).set('Cookie', sessionCookie(1)); + expect(del.status).toBe(200); + } + }); + it('200 on settlement update with permission, persisting the new amount', async () => { const created = await request(server) .post(`/api/trips/${tripId}/budget/settlements`) diff --git a/server/tests/unit/mcp/tools-budget-advanced.test.ts b/server/tests/unit/mcp/tools-budget-advanced.test.ts index cfc2ef4d8..4138d840b 100644 --- a/server/tests/unit/mcp/tools-budget-advanced.test.ts +++ b/server/tests/unit/mcp/tools-budget-advanced.test.ts @@ -39,6 +39,14 @@ import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createTrip, createBudgetItem, addTripMember } from '../../helpers/factories'; import { createMcpHarness, parseToolResult, parseResourceResult, type McpHarness } from '../../helpers/mcp-harness'; +import { BudgetController } from '../../../src/nest/budget/budget.controller'; +import { BudgetService } from '../../../src/nest/budget/budget.service'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.service'; +import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; +import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; +import type { TripAccess } from '../../../src/nest/database/database.service'; +import type { User } from '../../../src/types'; beforeAll(() => { createTables(testDb); @@ -332,6 +340,40 @@ describe('Settlement tools', () => { }); }); + it('get_settlement_summary agrees with GET /budget/settlement, unpaid expense included (#2225)', async () => { + // The two surfaces resolve the trip currency and the rates separately before + // calling calculateSettlement, so the parity that matters is on the payload. + // The fixture carries the row the fix is about: a total nobody has paid. + const { user, other, trip } = tripWithTwo(); + const paid = createBudgetItem(testDb, trip.id, { total_price: 100 }); + testDb.prepare('INSERT INTO budget_item_members (budget_item_id, user_id, paid) VALUES (?, ?, 0), (?, ?, 0)') + .run(paid.id, user.id, paid.id, other.id); + testDb.prepare('INSERT INTO budget_item_payers (budget_item_id, user_id, amount) VALUES (?, ?, ?)') + .run(paid.id, user.id, 100); + const unpaid = createBudgetItem(testDb, trip.id, { total_price: 40 }); + testDb.prepare('INSERT INTO budget_item_members (budget_item_id, user_id, paid) VALUES (?, ?, 0)') + .run(unpaid.id, other.id); + + const dbService = new DatabaseService(testDb); + const controller = new BudgetController( + new BudgetService(dbService, new PermissionsService(dbService), new ExchangeRatesService(), new RealtimeService()), + ); + const rest = await controller.settlement( + { id: user.id } as User, + { id: trip.id, user_id: user.id } as TripAccess, + String(trip.id), + ); + + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'get_settlement_summary', arguments: { tripId: trip.id } }); + const data = parseToolResult(result) as { summary: typeof rest }; + expect(data.summary.balances).toEqual(rest.balances); + expect(data.summary.flows).toEqual(rest.flows); + // Only the paid 100 settles; the 40 nobody paid is outstanding, not owed. + expect(rest.balances.map(b => b.balance)).toEqual([50, -50]); + }); + }); + it('get_settlement_summary returns balances and flows', async () => { try { const { user, other, trip } = tripWithTwo(); diff --git a/server/tests/unit/nest/budget.service.calc.test.ts b/server/tests/unit/nest/budget.service.calc.test.ts index 967833dc7..8c57c804d 100644 --- a/server/tests/unit/nest/budget.service.calc.test.ts +++ b/server/tests/unit/nest/budget.service.calc.test.ts @@ -155,6 +155,9 @@ describe('calculateSettlement', () => { ); const result = budget.calculateSettlement(1); expect(result.flows).toEqual([]); + // "No flows" on its own said nothing about the balances behind them: they + // were -50/-50 here until #2225, an offer of nothing next to money owed. + expect(centSum(result.balances.map(b => b.balance))).toBe(0); }); it('2 members, 1 payer: payer is owed half, non-payer owes half', () => { @@ -484,9 +487,10 @@ describe('calculateSettlement — negative amounts (#2176)', () => { expect(centSum(result.balances.map(b => b.balance))).toBe(0); }); - it('a payer-less negative total still reads as money owed back (fallback path)', () => { - // Nobody is recorded as the refund's recipient yet — the recorded total - // stands in, mirroring the positive unpaid-bill fallback. + it('a payer-less refund owes nobody anything until its recipient is named (#2225)', () => { + // Nobody is recorded as having received the refund, so there is no credit to + // hand back: the row is outstanding, not a debt the trip owes its members. + // It used to credit all three 30 € out of thin air. setupDb( [makeItem(1, -90)], [makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob'), makeMember(1, 3, 'carol')], @@ -494,7 +498,8 @@ describe('calculateSettlement — negative amounts (#2176)', () => { ); const result = budget.calculateSettlement(1); - expect(result.balances.map(b => b.balance)).toEqual([30, 30, 30]); + expect(result.balances).toEqual([]); + expect(result.flows).toEqual([]); }); }); @@ -621,10 +626,10 @@ describe('calculateSettlement — cent-exact settle-up (#1382)', () => { } }); - it('#1382 an unpaid bill still reads as money owed', () => { - // Nobody is down as a payer, so there is nothing to divide between the payers — - // the recorded total stands in, and the expense keeps showing up as outstanding - // rather than quietly netting to zero. + it('#2225 an unpaid bill owes nobody anything', () => { + // Nobody is down as a payer, so nobody is out of pocket and there is nothing + // to pay back. It used to debit all three 30 € against no credit at all, + // leaving Σ(balances) at -90 with no flow able to clear it. setupDb( [makeItem(1, 90)], [makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob'), makeMember(1, 3, 'carol')], @@ -632,7 +637,94 @@ describe('calculateSettlement — cent-exact settle-up (#1382)', () => { ); const result = budget.calculateSettlement(1); - expect(result.balances.map(b => b.balance)).toEqual([-30, -30, -30]); + expect(result.balances).toEqual([]); + expect(result.flows).toEqual([]); + }); +}); + +// ── Unpaid expenses stay out of the ledger (#2225) ──────────────────────── + +describe('calculateSettlement: unpaid expenses (#2225)', () => { + it('an unpaid expense does not turn its members into debtors of an unrelated payer', () => { + // The shape from the issue: a 300 € bill Alice fronted for all four, plus a + // 60 € row nobody has paid that only Bob and Carol are on. The 60 € used to + // be debited with no credit behind it, and the simplifier (which knows + // nothing about which expense made which debt) handed the extra 60 € to + // Alice, who was never owed it. + setupDb( + [makeItem(1, 300), makeItem(2, 60)], + [ + makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob'), makeMember(1, 3, 'carol'), makeMember(1, 4, 'dave'), + makeMember(2, 2, 'bob'), makeMember(2, 3, 'carol'), + ], + [makePayer(1, 1, 300, 'alice')], + ); + const result = budget.calculateSettlement(1); + const balance = (uid: number) => result.balances.find(b => b.user_id === uid)!.balance; + + expect(centSum(result.balances.map(b => b.balance))).toBe(0); + // Being on the unpaid row costs Bob and Carol nothing over Dave, who is not. + expect(balance(2)).toBe(balance(4)); + expect(balance(3)).toBe(balance(4)); + expect(balance(1)).toBe(225); + expect(centSum(result.flows.map(f => f.amount))).toBe(Math.round(balance(1) * 100)); + for (const f of result.flows) expect(f.to.user_id).toBe(1); + }); + + it('an unpaid expense with a custom split is skipped too', () => { + // The custom branch had the same hole and no coverage: 100 € split 70/30 by + // agreement, with nobody recorded as having paid it. + setupDb( + [makeItem(1, 90), makeItem(2, 100)], + [ + makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob'), makeMember(1, 3, 'carol'), + { ...makeMember(2, 2, 'bob'), amount: 70 }, + { ...makeMember(2, 3, 'carol'), amount: 30 }, + ], + [makePayer(1, 1, 90, 'alice')], + ); + const result = budget.calculateSettlement(1); + const balance = (uid: number) => result.balances.find(b => b.user_id === uid)!.balance; + + expect(balance(1)).toBe(60); + expect(balance(2)).toBe(-30); + expect(balance(3)).toBe(-30); + expect(centSum(result.balances.map(b => b.balance))).toBe(0); + }); + + it('a zero-total item with no payer contributes nothing and no rows', () => { + // Payer-less is payer-less regardless of the total: it contributes 0 either + // way, and both panels synthesise a missing member's 0.00 row from the trip + // roster (CostsPanel.tsx:853, MCostsTab.tsx:273), so dropping the row costs + // the UI nothing. + setupDb( + [makeItem(1, 0)], + [makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob')], + [], + ); + const result = budget.calculateSettlement(1); + + expect(result.balances).toEqual([]); + expect(result.flows).toEqual([]); + }); + + it('a custom split left with no payer and a zeroed total creates no debt', () => { + // PUT /budget/:id/payers with an empty array zeroes total_price via + // writeItemPayers and, unlike updateBudgetItem, never re-applies it, so the + // row survives as custom per-member amounts with no credit behind them. A + // guard that also required a non-zero total would let exactly this shape + // through and rebuild the #2225 phantom debt from a first-party endpoint. + setupDb( + [makeItem(1, 0)], + [ + { ...makeMember(1, 2, 'bob'), amount: 40 }, + { ...makeMember(1, 3, 'cara'), amount: 60 }, + ], + [], + ); + const result = budget.calculateSettlement(1); + + expect(result.balances).toEqual([]); expect(result.flows).toEqual([]); }); }); diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index 6232f68cb..c81542abd 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -314,8 +314,9 @@ function seedIssue1543Trip(tripCurrency: string) { testDb.prepare('UPDATE trips SET currency = ? WHERE id = ?').run(tripCurrency, trip.id); const members = [{ user_id: me.id }, { user_id: danil.id }, { user_id: serega.id }]; - // 9 000 ₽ nobody has paid yet, 9 000 ₽ paid by me, and $100 paid by me. The USD row - // carries the rate frozen at entry time: units of USD per 1 RUB. + // 9 000 ₽ nobody has paid yet (outstanding, settled by nobody, #2225), 9 000 ₽ paid + // by me, and $100 paid by me. The USD row carries the rate frozen at entry time: + // units of USD per 1 RUB. budget.createBudgetItem(trip.id, { name: 'Проезд обратно', total_price: 9000, currency: 'RUB', members }); budget.createBudgetItem(trip.id, { name: 'Проезд туда', currency: 'RUB', payers: [{ user_id: me.id, amount: 9000 }], members }); budget.createBudgetItem(trip.id, { @@ -332,18 +333,20 @@ describe('calculateSettlement with a foreign-currency expense (#1543)', () => { const result = budget.calculateSettlement(trip.id, { base: 'RUB', tripCurrency: 'RUB', rates: RATES.RUB }); const balanceOf = (id: number) => result.balances.find(b => b.user_id === id)!.balance; - // Total spend is 9 000 + 9 000 + $100 (≈7 668 ₽), so each of the three owes a third - // of it and I am owed back everything I fronted beyond my own share. The bug divided - // the RUB shares by the USD rate and reported +451 092 / −230 080 / −230 012 instead. - // Tolerance is a rouble: the cent-rotation in splitEqualShares moves the odd cent of - // the $100 between members, which the USD rate magnifies ~77x. - const totalSpend = 18000 + 100 / RATES.RUB.USD; - const share = totalSpend / 3; - expect(balanceOf(me.id)).toBeCloseTo(9000 + 100 / RATES.RUB.USD - share, -1); + // What actually settles is 9 000 ₽ + $100 (≈7 668 ₽): the third expense is the + // 9 000 ₽ nobody has paid, which is outstanding rather than owed (#2225). Each of + // the three owes a third of that, and I am owed back everything I fronted beyond my + // own share. The bug divided the RUB shares by the USD rate and reported +451 092 / + // −230 080 / −230 012 instead. Tolerance is a rouble: the cent-rotation in + // splitEqualShares moves the odd cent of the $100 between members, which the USD + // rate magnifies ~77x. + const settledSpend = 9000 + 100 / RATES.RUB.USD; + const share = settledSpend / 3; + expect(balanceOf(me.id)).toBeCloseTo(settledSpend - share, -1); expect(balanceOf(danil.id)).toBeCloseTo(-share, -1); expect(balanceOf(serega.id)).toBeCloseTo(-share, -1); - // The 9 000 ₽ expense nobody paid is the only imbalance in the trip. - expect(result.balances.reduce((a, b) => a + b.balance, 0)).toBeCloseTo(-9000, 1); + // Nothing is left over: every rouble owed is a rouble somebody is owed. + expect(result.balances.reduce((a, b) => a + Math.round(b.balance * 100), 0)).toBe(0); }); it('BUDGET-SVC-DB-005: reports the same balances when the display currency differs from the trip currency', () => { @@ -353,7 +356,7 @@ describe('calculateSettlement with a foreign-currency expense (#1543)', () => { const inEur = budget.calculateSettlement(trip.id, { base: 'EUR', tripCurrency: 'RUB', rates: RATES.EUR }); const danilEur = inEur.balances.find(b => b.user_id === danil.id)!.balance; - const shareRub = (18000 + 100 / RATES.RUB.USD) / 3; + const shareRub = (9000 + 100 / RATES.RUB.USD) / 3; expect(danilEur).toBeCloseTo(-shareRub / RATES.EUR.RUB, 0); }); }); @@ -677,6 +680,50 @@ describe('negative amounts persist end-to-end (#2176)', () => { }); }); +describe('an expense nobody paid stays out of the ledger (#2225)', () => { + it('BUDGET-SVC-DB-037: an unpaid expense moves neither the balances nor the offered flows', () => { + const { user: alice } = createUser(testDb, { username: 'alice' }); + const { user: bob } = createUser(testDb, { username: 'bob' }); + const { user: carol } = createUser(testDb, { username: 'carol' }); + const trip = createTrip(testDb, alice.id, { title: 'Trip' }); + addTripMember(testDb, trip.id, bob.id); + addTripMember(testDb, trip.id, carol.id); + + budget.createBudgetItem(trip.id, { + name: 'Dinner', payers: [{ user_id: alice.id, amount: 120 }], + member_ids: [alice.id, bob.id, carol.id], + }); + const before = budget.calculateSettlement(trip.id); + + // The row from the issue: a recorded total left on "No one paid yet", split + // between the two people who were actually there. + const unpaid = budget.createBudgetItem(trip.id, { + name: 'Taxi', total_price: 161.57, payers: [], member_ids: [bob.id, carol.id], + }) as { id: number; payers: unknown[]; total_price: number }; + expect(unpaid.payers).toEqual([]); + expect(unpaid.total_price).toBe(161.57); + + // Re-saving it from the edit modal (payers cleared, total re-sent) must not + // let writeItemPayers derive the total back down to 0. + const resaved = budget.updateBudgetItem(unpaid.id, trip.id, { + total_price: 161.57, payers: [], member_ids: [bob.id, carol.id], + }) as { payers: unknown[]; total_price: number }; + expect(resaved.payers).toEqual([]); + expect(resaved.total_price).toBe(161.57); + const payerRows = testDb + .prepare('SELECT count(*) AS cnt FROM budget_item_payers WHERE budget_item_id = ?') + .get(unpaid.id) as { cnt: number }; + expect(payerRows.cnt).toBe(0); + + // 161.57 with no credit behind it used to push bob and carol 80.79/80.78 further + // into the red and hand the whole of it to alice, who never fronted a rouble of it. + const after = budget.calculateSettlement(trip.id); + expect(after.balances).toEqual(before.balances); + expect(after.flows).toEqual(before.flows); + expect(after.balances.reduce((a, b) => a + Math.round(b.balance * 100), 0)).toBe(0); + }); +}); + describe('post-fold quirk fixes', () => { it('BUDGET-SVC-DB-019: settlements prefer display_name over username (quirk fix)', () => { const { user: alice } = createUser(testDb, { username: 'alice' }); diff --git a/wiki/Budget-Tracking.md b/wiki/Budget-Tracking.md index e092b86d5..e334b70c8 100644 --- a/wiki/Budget-Tracking.md +++ b/wiki/Budget-Tracking.md @@ -67,9 +67,9 @@ Deleting the booking or the place deletes its linked expense with it. Removing t - **One person paid** — pick a member from the dropdown. This is the default. - **Multiple people paid** — switch with the link beside the label, include each payer and type what each of them put in. The amounts have to add up to the total; until they do, the dialog reads *Payer amounts must add up to …* and refuses to save. -- **No one paid yet** — the first entry in the single-payer dropdown, for an expense you are only planning. The amount still counts toward **Total trip spend**, and everyone on its split is still charged their share in **Balances** — what is missing is the credit side, so nobody is recorded as having covered it. +- **No one paid yet**: the first entry in the single-payer dropdown, for an expense you are only planning. The amount still counts toward **Total trip spend**, but it creates no debt: with nobody out of pocket for it there is nothing to pay anyone back, so it stays out of **Balances** and out of the settle-up suggestions until you fill in who paid. -An expense with no payer is flagged **Unfinished** on its row and counted into the **Outstanding amount** card, which is where to look when the balances show money owed that nobody is down as having paid. +An expense with no payer is flagged **Unfinished** on its row and counted into the **Outstanding amount** card, which is where to look for the spending that is recorded but not yet settled between anyone. ## Splitting costs