fix(budget): seed edit fields with two decimals and allow signed amounts for refunds (#2175, #2176)

This commit is contained in:
Maurice
2026-09-01 15:07:26 +02:00
committed by Maurice
parent 49b0ce28d5
commit d1aa573167
18 changed files with 717 additions and 78 deletions
+14 -9
View File
@@ -20,16 +20,20 @@ import { AddonsService } from '../addons/addons.service';
/** Legacy registrar gate: the whole budget surface rides the budget addon. */
const budgetAddonOn = addonGate(ADDON_IDS.BUDGET);
/** Reusable Zod shape for the per-payer amounts on a budget item. */
/**
* Reusable Zod shape for the per-payer amounts on a budget item. Amounts are
* signed like the REST contract's (#2176): a negative payer is the recipient
* of a refund recorded as a negative expense.
*/
const payersSchema = z.array(z.object({
user_id: z.number().int().positive(),
amount: z.number().nonnegative(),
amount: z.number(),
})).describe('Who actually paid, and how much each paid, in the expense currency. Ask the user; do not guess.');
/** Reusable Zod shape for an unequal split: what each participant owes. */
/** Reusable Zod shape for an unequal split: what each participant owes. Signed, like the REST contract (#2176). */
const splitMembersSchema = z.array(z.object({
user_id: z.number().int().positive(),
amount: z.number().nonnegative(),
amount: z.number(),
})).describe('Unequal split: what each participant owes, in the expense currency. The amounts must add up to the expense total. Ask the user; do not guess.');
function parseId(value: string | string[]): number | null {
@@ -116,9 +120,10 @@ export class BudgetMcp {
// Once payers are sent at all, the write path derives the total from them and
// an empty list therefore means zero, not "no opinion". Reading total_price
// here instead would certify a split against a figure the row never receives.
// Negative payers count with their sign (#2176) — the write path stores them.
if (payers !== undefined) {
const roster = this.db.rosterUserIds(tripId);
return sumCents(payers.filter(p => p.amount > 0 && roster.has(p.user_id)).map(p => p.amount));
return sumCents(payers.filter(p => p.amount !== 0 && roster.has(p.user_id)).map(p => p.amount));
}
if (total_price !== undefined) return toCents(total_price);
return fallbackCents;
@@ -151,7 +156,7 @@ export class BudgetMcp {
// certified here would not be the split stored. Refuse rather than let the
// difference surface on the balances screen.
if (payers) {
const payerStrangers = payers.filter(p => p.amount > 0 && !roster.has(p.user_id)).map(p => p.user_id);
const payerStrangers = payers.filter(p => p.amount !== 0 && !roster.has(p.user_id)).map(p => p.user_id);
if (payerStrangers.length > 0) {
return `payers contains user IDs that are not on this trip: ${payerStrangers.join(', ')}. Resolve them with list_trip_members.`;
}
@@ -177,7 +182,7 @@ export class BudgetMcp {
tripId: z.number().int().positive(),
name: z.string().min(1).max(200),
category: z.string().max(100).optional().describe('Budget category (e.g. Accommodation, Food, Transport)'),
total_price: z.number().nonnegative(),
total_price: z.number().describe('Signed: a negative total records a refund/partial reimbursement (#2176)'),
currency: z.string().max(10).nullable().optional().describe('ISO currency code (e.g. "EUR"); defaults to the trip currency'),
member_ids: z.array(z.number().int().positive()).optional().describe('Trip member user IDs splitting this expense equally. Omit to split across all trip members (owner + members); pass [] for no split.'),
members: splitMembersSchema.optional().describe('Uneven split: what each participant owes, in the expense currency. The amounts must add up to the expense total. Use this instead of member_ids, never alongside it.'),
@@ -250,7 +255,7 @@ export class BudgetMcp {
itemId: z.number().int().positive(),
name: z.string().min(1).max(200).optional(),
category: z.string().max(100).optional(),
total_price: z.number().nonnegative().optional(),
total_price: z.number().optional(),
currency: z.string().max(10).nullable().optional().describe('ISO currency code the expense is in (e.g. "USD"); null puts it back in the trip currency. Changing it re-freezes the FX rate at today\'s rate.'),
member_ids: z.array(z.number().int().positive()).optional().describe('Trip member user IDs splitting this expense equally; replaces the current split. Omit to leave unchanged, pass [] for no split.'),
members: splitMembersSchema.optional().describe('Uneven split: what each participant owes, in the expense currency; replaces the current split. The amounts must add up to the expense total. Use this instead of member_ids, never alongside it.'),
@@ -302,7 +307,7 @@ export class BudgetMcp {
tripId: z.number().int().positive(),
name: z.string().min(1).max(200),
category: z.string().max(100).optional().describe('Budget category (e.g. Accommodation, Food, Transport)'),
total_price: z.number().nonnegative(),
total_price: z.number().describe('Signed: a negative total records a refund/partial reimbursement (#2176)'),
note: z.string().max(500).optional(),
userIds: z.array(z.number().int().positive()).optional().describe('User IDs splitting this item; omit to split across all trip members, or pass an empty array for no split'),
place_id: z.number().int().positive().optional().describe('Place on this trip the expense belongs to (the museum ticket for that museum), linking it in the planner'),
+21 -8
View File
@@ -157,14 +157,18 @@ export class BudgetService {
return new Set([...unique].filter(id => roster.has(id)));
}
/** Replace the payer rows of an item and keep total_price = sum of payer amounts. */
/**
* Replace the payer rows of an item and keep total_price = sum of payer amounts.
* A negative amount is a real payer — the recipient of a refund (#2176) — and
* is stored as such; only a zero (or NaN) amount says nothing and is dropped.
*/
private writeItemPayers(itemId: number | string, tripId: string | number, payers: { user_id: number; amount: number }[]) {
this.db.run('DELETE FROM budget_item_payers WHERE budget_item_id = ?', itemId);
const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_payers (budget_item_id, user_id, amount) VALUES (?, ?, ?)');
const known = this.rosterMemberIds(tripId, payers.map(p => p.user_id));
const accepted: number[] = [];
for (const p of payers) {
if (!(p.amount > 0) || !known.has(p.user_id)) continue;
if (!p.amount || !known.has(p.user_id)) continue;
insert.run(itemId, p.user_id, p.amount);
accepted.push(p.amount);
}
@@ -365,8 +369,9 @@ export class BudgetService {
}
// total_price is derived from explicit payers when given; otherwise the caller
// value (planning entries, or a bill no one has paid yet).
const payerTotal = sumMoney((data.payers || []).filter(p => p.amount > 0).map(p => p.amount));
// value (planning entries, or a bill no one has paid yet). Negative payer
// amounts (a refund's recipient, #2176) count like any other.
const payerTotal = sumMoney((data.payers || []).filter(p => p.amount !== 0).map(p => p.amount));
const total = data.payers && data.payers.length > 0 ? payerTotal : (data.total_price || 0);
const knownMembers = data.members ? this.rosterMemberIds(tripId, data.members.map(m => m.user_id)) : null;
@@ -644,6 +649,12 @@ export class BudgetService {
* The remainder cent rotates with the item id rather than always landing on the
* first member, so across several expenses the rounding evens out instead of
* always favouring the same person.
*
* Floor-based (`totalCents - baseCents * n`, never `%`), so a negative total —
* a refund split across its beneficiaries (#2176) — still yields a remainder
* in [0, n) and shares that sum back to the total exactly. The client mirror
* (CostsPanel.helpers.splitEqualShares) must stay share-for-share identical;
* the parity fixture in budget.service.calc.test.ts pins both sides.
*/
private splitEqualShares(totalCents: number, members: { user_id: number }[], itemId: number): Record<number, number> {
const n = members.length;
@@ -755,10 +766,11 @@ export class BudgetService {
if (members.length === 0) continue; // planning-only entry → doesn't affect balances
// Payers are credited what they actually paid (converted to trip currency with
// the item's stored exchange rate)…
// 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.
let creditCents = 0;
for (const p of payers) {
const paid = toTripCents(p.amount > 0 ? p.amount : 0, item.currency, item.exchange_rate);
const paid = toTripCents(p.amount, item.currency, item.exchange_rate);
ensure(p.user_id, p).cents += paid;
creditCents += paid;
}
@@ -770,9 +782,10 @@ export class BudgetService {
// 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.
// 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.
const hasCustomSplit = members.some(m => m.amount !== null && m.amount !== undefined);
const splitCents = creditCents > 0
const splitCents = creditCents !== 0
? creditCents
: toTripCents(item.total_price, item.currency, item.exchange_rate);
const equalShares = !hasCustomSplit ? this.splitEqualShares(splitCents, members, item.id) : {};
+45 -1
View File
@@ -43,6 +43,7 @@ let checkPermission: MockInstance;
import { createTables } from '../../src/db/schema';
import { runMigrations } from '../../src/db/migrations';
import { BudgetModule } from '../../src/nest/budget/budget.module';
import { ExchangeRatesService } from '../../src/nest/budget/exchange-rates.service';
import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter';
import { ZodValidationPipe } from '../../src/nest/common/zod-validation.pipe';
@@ -52,7 +53,12 @@ describe('Budget e2e (real auth guard + temp SQLite, real budget SQL)', () => {
let tripId: number;
async function build() {
const moduleRef = await Test.createTestingModule({ imports: [DatabaseModule, RealtimeModule, BudgetModule] }).compile();
const moduleRef = await Test.createTestingModule({ imports: [DatabaseModule, RealtimeModule, BudgetModule] })
// The settlement read awaits live FX rates; the trip here is all-EUR, so a
// null result is the identity — and the test never touches the network.
.overrideProvider(ExchangeRatesService)
.useValue({ getRates: async () => null })
.compile();
const nest = moduleRef.createNestApplication();
nest.use(cookieParser());
nest.useGlobalFilters(new TrekExceptionFilter());
@@ -146,6 +152,44 @@ describe('Budget e2e (real auth guard + temp SQLite, real budget SQL)', () => {
expect(res.body.error.toLowerCase()).toContain('name');
});
it('201 on a negative expense (partial reimbursement) and the settlement nets it out (#2176)', async () => {
// The dinner: user 1 fronts 90, split with user 2 → user 2 owes 45.
const dinner = await request(server)
.post(`/api/trips/${tripId}/budget`)
.set('Cookie', sessionCookie(1))
.send({ name: 'Dinner', payers: [{ user_id: 1, amount: 90 }], member_ids: [1, 2] });
expect(dinner.status).toBe(201);
// The refund: -30 lands with user 1, shared with user 2 → user 2's debt
// drops by 15. The write path used to null this to a payer-less 0 € row.
const refund = await request(server)
.post(`/api/trips/${tripId}/budget`)
.set('Cookie', sessionCookie(1))
.send({ name: 'Hotel partial refund', total_price: -30, payers: [{ user_id: 1, amount: -30 }], member_ids: [1, 2] });
expect(refund.status).toBe(201);
expect(refund.body.item).toMatchObject({ total_price: -30 });
expect(refund.body.item.payers).toEqual([expect.objectContaining({ user_id: 1, amount: -30 })]);
const row = db.prepare('SELECT total_price FROM budget_items WHERE id = ?').get(refund.body.item.id) as { total_price: number };
expect(row.total_price).toBe(-30);
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;
// 45 owed from the dinner minus the 15 refund share = 30, and Σ balances = 0.
expect(balance(2)).toBe(-30);
expect(balance(1)).toBe(30);
expect(balances.reduce((a, b) => a + Math.round(b.balance * 100), 0)).toBe(0);
// Clean up so the ledger tests below start from an empty trip.
for (const id of [dinner.body.item.id, refund.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`)
@@ -412,6 +412,128 @@ describe('calculateSettlement', () => {
});
});
// ── Refunds: negative amounts are first-class expenses (#2176) ───────────────
describe('calculateSettlement — negative amounts (#2176)', () => {
it('a refund credits the members and debits its recipient', () => {
// A 30 € refund lands with Alice; all three had shared the original cost.
// Alice received 30 but only 10 of it was hers, so she owes 20.
setupDb(
[makeItem(1, -30)],
[makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob'), makeMember(1, 3, 'carol')],
[makePayer(1, 1, -30, 'alice')],
);
const result = budget.calculateSettlement(1);
const balance = (uid: number) => result.balances.find(b => b.user_id === uid)!.balance;
expect(balance(1)).toBe(-20);
expect(balance(2)).toBe(10);
expect(balance(3)).toBe(10);
expect(centSum(result.balances.map(b => b.balance))).toBe(0);
});
it('a refund reduces the debt its expense created', () => {
// 90 € dinner fronted by Alice, then a 30 € partial reimbursement she keeps —
// both split three ways. Bob's debt drops from 30 to 20.
setupDb(
[makeItem(1, 90), makeItem(2, -30)],
[
makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob'), makeMember(1, 3, 'carol'),
makeMember(2, 1, 'alice'), makeMember(2, 2, 'bob'), makeMember(2, 3, 'carol'),
],
[makePayer(1, 1, 90, 'alice'), makePayer(2, 1, -30, 'alice')],
);
const result = budget.calculateSettlement(1);
const balance = (uid: number) => result.balances.find(b => b.user_id === uid)!.balance;
expect(balance(1)).toBe(40);
expect(balance(2)).toBe(-20);
expect(balance(3)).toBe(-20);
expect(result.flows).toEqual(expect.arrayContaining([
expect.objectContaining({ amount: 20, from: expect.objectContaining({ user_id: 2 }), to: expect.objectContaining({ user_id: 1 }) }),
]));
});
it('an odd negative total still nets to exactly zero', () => {
setupDb(
[makeItem(1, -100.01)],
[makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob'), makeMember(1, 3, 'carol')],
[makePayer(1, 1, -100.01, 'alice')],
);
const result = budget.calculateSettlement(1);
expect(centSum(result.balances.map(b => b.balance))).toBe(0);
});
it('a negative custom split settles by the custom amounts', () => {
// 100 € refund to Alice; by agreement Bob is owed 70 of it, Carol 30.
setupDb(
[makeItem(1, -100)],
[
{ ...makeMember(1, 2, 'bob'), amount: -70 },
{ ...makeMember(1, 3, 'carol'), amount: -30 },
],
[makePayer(1, 1, -100, 'alice')],
);
const result = budget.calculateSettlement(1);
const balance = (uid: number) => result.balances.find(b => b.user_id === uid)!.balance;
expect(balance(1)).toBe(-100);
expect(balance(2)).toBe(70);
expect(balance(3)).toBe(30);
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.
setupDb(
[makeItem(1, -90)],
[makeMember(1, 1, 'alice'), makeMember(1, 2, 'bob'), makeMember(1, 3, 'carol')],
[],
);
const result = budget.calculateSettlement(1);
expect(result.balances.map(b => b.balance)).toEqual([30, 30, 30]);
});
});
// ── Client/server share parity (#2176) ───────────────────────────────────────
//
// splitEqualShares exists twice: here (netting the settlement in cents) and on
// the client (previewing the split in euros — CostsPanel.helpers.ts). This
// fixture is duplicated verbatim in
// client/src/components/Budget/CostsPanel.helpers.test.ts; if either
// implementation drifts — sign handling included — its copy of the table fails.
const SHARE_PARITY_FIXTURE: { totalCents: number; users: number[]; itemId: number; expected: Record<number, number> }[] = [
{ totalCents: 10000, users: [1, 2, 3], itemId: 0, expected: { 1: 3334, 2: 3333, 3: 3333 } },
{ totalCents: 10000, users: [1, 2, 3], itemId: 1, expected: { 1: 3333, 2: 3334, 3: 3333 } },
{ totalCents: -10000, users: [1, 2, 3], itemId: 0, expected: { 1: -3333, 2: -3333, 3: -3334 } },
{ totalCents: -10000, users: [1, 2, 3], itemId: 1, expected: { 1: -3334, 2: -3333, 3: -3333 } },
{ totalCents: -101, users: [1, 2], itemId: 0, expected: { 1: -50, 2: -51 } },
{ totalCents: -101, users: [1, 2], itemId: 1, expected: { 1: -51, 2: -50 } },
{ totalCents: -1, users: [1, 2, 3], itemId: 0, expected: { 1: 0, 2: 0, 3: -1 } },
];
describe('splitEqualShares — client parity (#2176)', () => {
// Private on purpose (only the settlement calls it); the parity pin reaches
// through so the fixture exercises the real implementation, not a re-model.
const split = (
budget as unknown as {
splitEqualShares(totalCents: number, members: { user_id: number }[], itemId: number): Record<number, number>;
}
).splitEqualShares.bind(budget);
it.each(SHARE_PARITY_FIXTURE)(
'splits $totalCents cents across $users.length members (item $itemId) exactly like the client',
({ totalCents, users, itemId, expected }) => {
const shares = split(totalCents, users.map(user_id => ({ user_id })), itemId);
expect(shares).toEqual(expected);
expect((Object.values(shares) as number[]).reduce((a, b) => a + b, 0)).toBe(totalCents);
},
);
});
// ── #1382: balances and flows have to tell the same story ────────────────────
// The ledger is netted in whole cents of the trip currency, so a balance is only
// ever a whole number of cents and every one of them can be settled. What used to
@@ -581,6 +581,102 @@ describe('settlement parties are confined to the trip', () => {
});
});
// ── Refunds persist as negative expenses (#2176) ─────────────────────────────
// The write path used to filter payers on amount > 0 and re-derive total_price
// from the survivors, silently turning a negative entry into a 0 € one with no
// payer row.
describe('negative amounts persist end-to-end (#2176)', () => {
it('BUDGET-SVC-DB-032: stores a negative payer row and the negative total', () => {
const { user: alice } = createUser(testDb, { username: 'alice' });
const trip = createTrip(testDb, alice.id);
const item = budget.createBudgetItem(trip.id, {
name: 'Hotel partial refund',
total_price: -100,
payers: [{ user_id: alice.id, amount: -100 }],
}) as { id: number; payers: { user_id: number; amount: number }[]; total_price: number };
expect(item.total_price).toBe(-100);
expect(item.payers).toEqual([expect.objectContaining({ user_id: alice.id, amount: -100 })]);
const row = testDb.prepare('SELECT total_price FROM budget_items WHERE id = ?').get(item.id) as { total_price: number };
expect(row.total_price).toBe(-100);
});
it('BUDGET-SVC-DB-033: an update keeps a negative payer instead of nulling the entry', () => {
const { user: alice } = createUser(testDb, { username: 'alice' });
const { user: bob } = createUser(testDb, { username: 'bob' });
const trip = createTrip(testDb, alice.id);
addTripMember(testDb, trip.id, bob.id);
const item = budget.createBudgetItem(trip.id, {
name: 'Refund', total_price: -60, payers: [{ user_id: alice.id, amount: -60 }],
}) as { id: number };
const updated = budget.updateBudgetItem(item.id, trip.id, {
payers: [{ user_id: alice.id, amount: -40 }, { user_id: bob.id, amount: -20 }],
}) as { payers: { user_id: number; amount: number }[]; total_price: number };
expect(updated.total_price).toBe(-60);
expect(updated.payers).toHaveLength(2);
expect(updated.payers.map(p => p.amount).sort((a, b) => a - b)).toEqual([-40, -20]);
});
it('BUDGET-SVC-DB-034: mixed-sign payers derive the netted total', () => {
// Alice fronted 100 but bob pocketed a 30 refund on the same receipt.
const { user: alice } = createUser(testDb, { username: 'alice' });
const { user: bob } = createUser(testDb, { username: 'bob' });
const trip = createTrip(testDb, alice.id);
addTripMember(testDb, trip.id, bob.id);
const item = budget.createBudgetItem(trip.id, {
name: 'Tickets', payers: [{ user_id: alice.id, amount: 100 }, { user_id: bob.id, amount: -30 }],
}) as { payers: { user_id: number }[]; total_price: number };
expect(item.payers).toHaveLength(2);
expect(item.total_price).toBe(70);
});
it('BUDGET-SVC-DB-035: a zero-amount payer is still dropped', () => {
const { user: alice } = createUser(testDb, { username: 'alice' });
const { user: bob } = createUser(testDb, { username: 'bob' });
const trip = createTrip(testDb, alice.id);
addTripMember(testDb, trip.id, bob.id);
const item = budget.createBudgetItem(trip.id, {
name: 'Dinner', payers: [{ user_id: alice.id, amount: 90 }, { user_id: bob.id, amount: 0 }],
}) as { payers: { user_id: number }[]; total_price: number };
expect(item.payers.map(p => p.user_id)).toEqual([alice.id]);
expect(item.total_price).toBe(90);
});
it('BUDGET-SVC-DB-036: a negative expense settles against real rows — Σ balances = 0', () => {
const { user: alice } = createUser(testDb, { username: 'alice' });
const { user: bob } = createUser(testDb, { username: 'bob' });
const trip = createTrip(testDb, alice.id);
addTripMember(testDb, trip.id, bob.id);
budget.createBudgetItem(trip.id, {
name: 'Dinner', payers: [{ user_id: alice.id, amount: 90 }],
member_ids: [alice.id, bob.id],
});
budget.createBudgetItem(trip.id, {
name: 'Refund', payers: [{ user_id: alice.id, amount: -30 }],
member_ids: [alice.id, bob.id],
});
const result = budget.calculateSettlement(trip.id);
const balance = (uid: number) => result.balances.find(b => b.user_id === uid)!.balance;
// 90 out, 30 back, both split evenly: bob nets 45 - 15 = 30 owed to alice.
expect(balance(alice.id)).toBe(30);
expect(balance(bob.id)).toBe(-30);
expect(result.balances.reduce((a, b) => a + Math.round(b.balance * 100), 0)).toBe(0);
expect(result.flows).toEqual([
expect.objectContaining({ amount: 30, from: expect.objectContaining({ user_id: bob.id }), to: expect.objectContaining({ user_id: alice.id }) }),
]);
});
});
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' });