From 351b5fb4ecbce8028612185cfb427facdfe513db Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 28 Jul 2026 00:04:50 +0200 Subject: [PATCH] fix(server): fix the quirks preserved by the budget DI migration --- server/src/nest/budget/budget.mcp.ts | 50 ++- server/src/nest/budget/budget.service.ts | 351 ++++++++++-------- .../unit/mcp/tools-budget-advanced.test.ts | 12 +- .../unit/nest/budget.service.calc.test.ts | 8 +- .../tests/unit/nest/budget.service.db.test.ts | 31 +- server/tests/unit/nest/budget.service.test.ts | 5 +- 6 files changed, 278 insertions(+), 179 deletions(-) diff --git a/server/src/nest/budget/budget.mcp.ts b/server/src/nest/budget/budget.mcp.ts index 8a33da04c..4da026a69 100644 --- a/server/src/nest/budget/budget.mcp.ts +++ b/server/src/nest/budget/budget.mcp.ts @@ -43,16 +43,24 @@ function resolveMemberIds(tripId: number, member_ids?: number[]): number[] | und } /** - * Budget MCP surface — ported 1:1 from the legacy registrars: the eleven tools + * Budget MCP surface — ported from the legacy registrars: the eleven tools * from src/mcp/tools/budget.ts and the trek://trips/{tripId}/budget, * …/budget/per-person and …/budget/settlement resources from - * src/mcp/resources.ts (identical names, descriptions, schemas, annotations, - * error/payload shapes and broadcasts — including the MCP-specific - * 'budget:members-updated' { item } and 'budget:member-paid-updated' - * { itemId, member } payloads, and the settlement writes that do not freeze an - * FX rate). The registration-time gates map to `when` (the whole-registrar - * budget-addon check) plus the declarative budget read/write access markers - * (the legacy `if (R)` / `if (W)` checks, resolved by trekMcpAccessPolicy). + * src/mcp/resources.ts (identical names, descriptions, schemas, annotations + * and error shapes). The registration-time gates map to `when` (the + * whole-registrar budget-addon check) plus the declarative budget read/write + * access markers (the legacy `if (R)` / `if (W)` checks, resolved by + * trekMcpAccessPolicy). + * + * Quirk fixes on top of the ported legacy behavior: broadcasts use the REST + * payload shapes ('budget:members-updated' { itemId, members, persons } and + * 'budget:member-paid-updated' { itemId, userId, paid } — the legacy + * MCP-specific { item } / { itemId, member } shapes were silent no-ops in the + * client's remoteEventHandler), the mutating settlement/item paths route + * through the freeze-then-write composites so REST and MCP can't diverge on + * the #1445 FX freeze, and the settlement resource resolves the trip currency + * and live rates like get_settlement_summary instead of silently defaulting + * to an unconverted EUR base. */ @McpController() export class BudgetMcp { @@ -154,7 +162,9 @@ export class BudgetMcp { if (isDemoUser(ctx.userId)) return demoDenied(); if (!this.budget.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!hasTripPermission('budget_edit', tripId, ctx.userId)) return permissionDenied(); - const item = this.budget.updateBudgetItem(itemId, tripId, { name, category, total_price, member_ids, payers, persons, days, note }); + // Freeze-then-write composite (no-op while the schema has no currency input, + // but keeps REST and MCP on one code path for the #1445 freeze). + const item = await this.budget.update(itemId, tripId, { name, category, total_price, member_ids, payers, persons, days, note }); if (!item) return errorResult('Budget item not found.'); safeBroadcast(tripId, 'budget:updated', { item }); return ok({ item }); @@ -194,7 +204,7 @@ export class BudgetMcp { return this.budget.getBudgetItem(created.id, tripId)!; }); safeBroadcast(tripId, 'budget:created', { item }); - if (members && members.length > 0) safeBroadcast(tripId, 'budget:members-updated', { item }); + if (members && members.length > 0) safeBroadcast(tripId, 'budget:members-updated', { itemId: item.id, members: item.members, persons: item.persons }); return ok({ item }); } catch { return errorResult('Failed to create budget item.'); @@ -220,7 +230,7 @@ export class BudgetMcp { const result = this.budget.updateMembers(itemId, tripId, userIds); if (!result) return errorResult('Budget item not found.'); const item = this.budget.getBudgetItem(itemId, tripId); - safeBroadcast(tripId, 'budget:members-updated', { item }); + safeBroadcast(tripId, 'budget:members-updated', { itemId, members: result.members, persons: result.item.persons }); return ok({ item }); } @@ -242,7 +252,7 @@ export class BudgetMcp { if (!this.budget.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!hasTripPermission('budget_edit', tripId, ctx.userId)) return permissionDenied(); const member = this.budget.toggleMemberPaid(itemId, tripId, memberId, paid); - safeBroadcast(tripId, 'budget:member-paid-updated', { itemId, member }); + safeBroadcast(tripId, 'budget:member-paid-updated', { itemId, userId: memberId, paid: paid ? 1 : 0 }); return ok({ member }); } @@ -304,7 +314,9 @@ export class BudgetMcp { if (isDemoUser(ctx.userId)) return demoDenied(); if (!this.budget.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!hasTripPermission('budget_edit', tripId, ctx.userId)) return permissionDenied(); - const settlement = this.budget.insertSettlement(tripId, { from_user_id, to_user_id, amount }, ctx.userId); + // Freeze-then-write composite, same as the REST path (no-op while the + // schema has no currency input — see the #1445 note in the class doc). + const settlement = await this.budget.createSettlement(tripId, { from_user_id, to_user_id, amount }, ctx.userId); safeBroadcast(tripId, 'budget:settlement-created', { settlement }); return ok({ settlement }); } @@ -330,7 +342,8 @@ export class BudgetMcp { if (isDemoUser(ctx.userId)) return demoDenied(); if (!this.budget.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!hasTripPermission('budget_edit', tripId, ctx.userId)) return permissionDenied(); - const settlement = this.budget.applySettlementUpdate(settlementId, tripId, { from_user_id, to_user_id, amount }); + // Freeze-then-write composite, same as the REST path. + const settlement = await this.budget.updateSettlement(settlementId, tripId, { from_user_id, to_user_id, amount }); if (!settlement) return errorResult('Settlement not found.'); safeBroadcast(tripId, 'budget:settlement-updated', { settlement }); return ok({ settlement }); @@ -436,7 +449,14 @@ export class BudgetMcp { }], }; } - const settlement = this.budget.calculateSettlement(id); + // Resolve the trip currency + live rates like get_settlement_summary — the + // legacy resource called calculateSettlement(id) bare, silently netting a + // non-EUR trip in EUR with no FX conversion. + const trip = this.db.get<{ currency?: string }>('SELECT currency FROM trips WHERE id = ?', id); + const tripCurrency = trip?.currency || 'EUR'; + const effectiveBase = tripCurrency.toUpperCase(); + const rates = await this.exchangeRates.getRates(effectiveBase); + const settlement = this.budget.calculateSettlement(id, { base: effectiveBase, rates, tripCurrency }); return { contents: [{ uri: uri.href, diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index a7dd4bb0e..96c7dea61 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -18,7 +18,7 @@ type SettlementRow = { }; /** - * Budget domain service — owns the budget SQL (moved 1:1 from the legacy + * Budget domain service — owns the budget SQL (moved from the legacy * services/budgetService.ts: identical statements, the `||` falsy-coercion * defaults, the COALESCE / CASE WHEN sentinel conventions on update and the * post-write re-selects). Trip access, the 'budget_edit' permission and the @@ -26,6 +26,14 @@ type SettlementRow = { * legacy tripService/userCleanupService and the legacy MCP trips/transports * registrars) go through budget.bridge.ts instead of importing this class * directly. + * + * Quirk fixes on top of the relocated legacy behavior: every multi-statement + * write now runs in db.transaction() ("transactions are not optional"), + * linkBudgetItemToReservation passes reservation_id through the insert instead + * of a redundant second UPDATE, settlement re-selects are targeted single-row + * queries instead of a full listSettlements() scan, settlement usernames use + * COALESCE(display_name, username) like every item query, and updateMembers no + * longer double-applies avatarUrl. */ @Injectable() export class BudgetService { @@ -271,59 +279,61 @@ export class BudgetService { reservation_id?: number | null; }, ) { - const maxOrder = this.db.get<{ max: number | null }>('SELECT MAX(sort_order) as max FROM budget_items WHERE trip_id = ?', tripId)!; - const sortOrder = (maxOrder.max !== null ? maxOrder.max : -1) + 1; + return this.db.transaction(() => { + const maxOrder = this.db.get<{ max: number | null }>('SELECT MAX(sort_order) as max FROM budget_items WHERE trip_id = ?', tripId)!; + const sortOrder = (maxOrder.max !== null ? maxOrder.max : -1) + 1; - const cat = data.category || 'other'; + const cat = data.category || 'other'; - // Ensure category has a sort_order entry - const catExists = this.db.get('SELECT 1 FROM budget_category_order WHERE trip_id = ? AND category = ?', tripId, cat); - if (!catExists) { - const maxCatOrder = this.db.get<{ max: number | null }>('SELECT MAX(sort_order) as max FROM budget_category_order WHERE trip_id = ?', tripId); - const catOrder = (maxCatOrder?.max !== null && maxCatOrder?.max !== undefined ? maxCatOrder.max : -1) + 1; - this.db.run('INSERT OR IGNORE INTO budget_category_order (trip_id, category, sort_order) VALUES (?, ?, ?)', tripId, cat, catOrder); - } + // Ensure category has a sort_order entry + const catExists = this.db.get('SELECT 1 FROM budget_category_order WHERE trip_id = ? AND category = ?', tripId, cat); + if (!catExists) { + const maxCatOrder = this.db.get<{ max: number | null }>('SELECT MAX(sort_order) as max FROM budget_category_order WHERE trip_id = ?', tripId); + const catOrder = (maxCatOrder?.max !== null && maxCatOrder?.max !== undefined ? maxCatOrder.max : -1) + 1; + this.db.run('INSERT OR IGNORE INTO budget_category_order (trip_id, category, sort_order) VALUES (?, ?, ?)', tripId, cat, catOrder); + } - // 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 = (data.payers || []).reduce((a, p) => a + (p.amount > 0 ? p.amount : 0), 0); - const total = data.payers && data.payers.length > 0 ? payerTotal : (data.total_price || 0); + // 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 = (data.payers || []).reduce((a, p) => a + (p.amount > 0 ? p.amount : 0), 0); + const total = data.payers && data.payers.length > 0 ? payerTotal : (data.total_price || 0); - const knownMembers = data.members ? this.knownUserIds(data.members.map(m => m.user_id)) : null; - const members = data.members && knownMembers ? data.members.filter(m => knownMembers.has(m.user_id)) : undefined; - const knownIds = data.member_ids ? this.knownUserIds(data.member_ids) : null; - const memberIds = data.member_ids && knownIds ? data.member_ids.filter(uid => knownIds.has(uid)) : undefined; + const knownMembers = data.members ? this.knownUserIds(data.members.map(m => m.user_id)) : null; + const members = data.members && knownMembers ? data.members.filter(m => knownMembers.has(m.user_id)) : undefined; + const knownIds = data.member_ids ? this.knownUserIds(data.member_ids) : null; + const memberIds = data.member_ids && knownIds ? data.member_ids.filter(uid => knownIds.has(uid)) : undefined; - const result = this.db.run( - 'INSERT INTO budget_items (trip_id, category, name, total_price, currency, exchange_rate, persons, days, note, sort_order, expense_date, reservation_id) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', - tripId, - cat, - data.name, - total, - data.currency || null, - data.exchange_rate != null ? data.exchange_rate : 1, - memberIds ? memberIds.length : (data.persons != null ? data.persons : null), - data.days !== undefined && data.days !== null ? data.days : null, - data.note || null, - sortOrder, - data.expense_date || null, - data.reservation_id != null ? data.reservation_id : null, - ); + const result = this.db.run( + 'INSERT INTO budget_items (trip_id, category, name, total_price, currency, exchange_rate, persons, days, note, sort_order, expense_date, reservation_id) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', + tripId, + cat, + data.name, + total, + data.currency || null, + data.exchange_rate != null ? data.exchange_rate : 1, + memberIds ? memberIds.length : (data.persons != null ? data.persons : null), + data.days !== undefined && data.days !== null ? data.days : null, + data.note || null, + sortOrder, + data.expense_date || null, + data.reservation_id != null ? data.reservation_id : null, + ); - const itemId = result.lastInsertRowid as number; - if (data.payers && data.payers.length > 0) this.writeItemPayers(itemId, data.payers); - if (members && members.length > 0) { - const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); - for (const m of members) insert.run(itemId, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); - } else if (memberIds && memberIds.length > 0) { - const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, NULL)'); - for (const uid of memberIds) insert.run(itemId, uid); - } + const itemId = result.lastInsertRowid as number; + if (data.payers && data.payers.length > 0) this.writeItemPayers(itemId, data.payers); + if (members && members.length > 0) { + const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); + for (const m of members) insert.run(itemId, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); + } else if (memberIds && memberIds.length > 0) { + const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, NULL)'); + for (const uid of memberIds) insert.run(itemId, uid); + } - const item = this.db.get('SELECT * FROM budget_items WHERE id = ?', itemId)!; - item.members = this.loadItemMembers(itemId); - item.payers = this.loadItemPayers(itemId); - return item; + const item = this.db.get('SELECT * FROM budget_items WHERE id = ?', itemId)!; + item.members = this.loadItemMembers(itemId); + item.payers = this.loadItemPayers(itemId); + return item; + }); } /** Fetch a single budget item hydrated with its members and payers, scoped to the trip. */ @@ -340,10 +350,9 @@ export class BudgetService { reservationId: number, data: { name: string; category?: string; total_price: number }, ) { - const item = this.createBudgetItem(tripId, data) as BudgetItem & { reservation_id?: number | null }; - this.db.run('UPDATE budget_items SET reservation_id = ? WHERE id = ?', reservationId, item.id); - item.reservation_id = reservationId; - return item; + // createBudgetItem accepts reservation_id directly — the legacy separate + // UPDATE after the insert was redundant (and non-atomic). + return this.createBudgetItem(tripId, { ...data, reservation_id: reservationId }); } updateBudgetItem( @@ -357,10 +366,11 @@ export class BudgetService { persons?: number | null; days?: number | null; note?: string | null; sort_order?: number; expense_date?: string | null; }, ) { - const item = this.db.get('SELECT * FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId); - if (!item) return null; + return this.db.transaction(() => { + const item = this.db.get('SELECT * FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId); + if (!item) return null; - this.db.run(` + this.db.run(` UPDATE budget_items SET category = COALESCE(?, category), name = COALESCE(?, name), @@ -374,59 +384,60 @@ export class BudgetService { expense_date = CASE WHEN ? THEN ? ELSE expense_date END WHERE id = ? `, - data.category || null, - data.name || null, - data.total_price !== undefined ? 1 : null, data.total_price !== undefined ? data.total_price : 0, - data.currency !== undefined ? 1 : 0, data.currency !== undefined ? (data.currency || null) : null, - data.exchange_rate !== undefined ? 1 : null, data.exchange_rate !== undefined ? data.exchange_rate : 1, - data.persons !== undefined ? 1 : null, data.persons !== undefined ? data.persons : null, - data.days !== undefined ? 1 : 0, data.days !== undefined ? data.days : null, - data.note !== undefined ? 1 : 0, data.note !== undefined ? data.note : null, - data.sort_order !== undefined ? 1 : null, data.sort_order !== undefined ? data.sort_order : 0, - data.expense_date !== undefined ? 1 : 0, data.expense_date !== undefined ? (data.expense_date || null) : null, - id, - ); + data.category || null, + data.name || null, + data.total_price !== undefined ? 1 : null, data.total_price !== undefined ? data.total_price : 0, + data.currency !== undefined ? 1 : 0, data.currency !== undefined ? (data.currency || null) : null, + data.exchange_rate !== undefined ? 1 : null, data.exchange_rate !== undefined ? data.exchange_rate : 1, + data.persons !== undefined ? 1 : null, data.persons !== undefined ? data.persons : null, + data.days !== undefined ? 1 : 0, data.days !== undefined ? data.days : null, + data.note !== undefined ? 1 : 0, data.note !== undefined ? data.note : null, + data.sort_order !== undefined ? 1 : null, data.sort_order !== undefined ? data.sort_order : 0, + data.expense_date !== undefined ? 1 : 0, data.expense_date !== undefined ? (data.expense_date || null) : null, + id, + ); - // Optional inline payer/member replacement (the edit modal saves all at once). - if (data.payers !== undefined) { - this.writeItemPayers(id, data.payers); - // writeItemPayers derives total_price from the payer sum (0 for no payers). - // A "recorded total, nobody assigned" expense clears payers but still carries - // an explicit total_price — re-apply it so it isn't clobbered to 0. - if (data.payers.length === 0 && data.total_price !== undefined) { - this.db.run('UPDATE budget_items SET total_price = ? WHERE id = ?', data.total_price, id); + // Optional inline payer/member replacement (the edit modal saves all at once). + if (data.payers !== undefined) { + this.writeItemPayers(id, data.payers); + // writeItemPayers derives total_price from the payer sum (0 for no payers). + // A "recorded total, nobody assigned" expense clears payers but still carries + // an explicit total_price — re-apply it so it isn't clobbered to 0. + if (data.payers.length === 0 && data.total_price !== undefined) { + this.db.run('UPDATE budget_items SET total_price = ? WHERE id = ?', data.total_price, id); + } } - } - if (data.members !== undefined) { - const known = this.knownUserIds(data.members.map(m => m.user_id)); - const members = data.members.filter(m => known.has(m.user_id)); - this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); - const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); - for (const m of members) insert.run(id, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); - this.db.run('UPDATE budget_items SET persons = ? WHERE id = ?', members.length || null, id); - } else if (data.member_ids !== undefined) { - const known = this.knownUserIds(data.member_ids); - const memberIds = data.member_ids.filter(uid => known.has(uid)); - this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); - const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, NULL)'); - for (const uid of memberIds) insert.run(id, uid); - this.db.run('UPDATE budget_items SET persons = ? WHERE id = ?', memberIds.length || null, id); - } - - // If category changed, update category order table - if (data.category) { - const catExists = this.db.get('SELECT 1 FROM budget_category_order WHERE trip_id = ? AND category = ?', tripId, data.category); - if (!catExists) { - const maxCatOrder = this.db.get<{ max: number | null }>('SELECT MAX(sort_order) as max FROM budget_category_order WHERE trip_id = ?', tripId); - const catOrder = (maxCatOrder?.max !== null && maxCatOrder?.max !== undefined ? maxCatOrder.max : -1) + 1; - this.db.run('INSERT OR IGNORE INTO budget_category_order (trip_id, category, sort_order) VALUES (?, ?, ?)', tripId, data.category, catOrder); + if (data.members !== undefined) { + const known = this.knownUserIds(data.members.map(m => m.user_id)); + const members = data.members.filter(m => known.has(m.user_id)); + this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); + const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); + for (const m of members) insert.run(id, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); + this.db.run('UPDATE budget_items SET persons = ? WHERE id = ?', members.length || null, id); + } else if (data.member_ids !== undefined) { + const known = this.knownUserIds(data.member_ids); + const memberIds = data.member_ids.filter(uid => known.has(uid)); + this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); + const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, NULL)'); + for (const uid of memberIds) insert.run(id, uid); + this.db.run('UPDATE budget_items SET persons = ? WHERE id = ?', memberIds.length || null, id); } - } - const updated = this.db.get('SELECT * FROM budget_items WHERE id = ?', id)!; - updated.members = this.loadItemMembers(id); - updated.payers = this.loadItemPayers(id); - return updated; + // If category changed, update category order table + if (data.category) { + const catExists = this.db.get('SELECT 1 FROM budget_category_order WHERE trip_id = ? AND category = ?', tripId, data.category); + if (!catExists) { + const maxCatOrder = this.db.get<{ max: number | null }>('SELECT MAX(sort_order) as max FROM budget_category_order WHERE trip_id = ?', tripId); + const catOrder = (maxCatOrder?.max !== null && maxCatOrder?.max !== undefined ? maxCatOrder.max : -1) + 1; + this.db.run('INSERT OR IGNORE INTO budget_category_order (trip_id, category, sort_order) VALUES (?, ?, ?)', tripId, data.category, catOrder); + } + } + + const updated = this.db.get('SELECT * FROM budget_items WHERE id = ?', id)!; + updated.members = this.loadItemMembers(id); + updated.payers = this.loadItemPayers(id); + return updated; + }); } // ------------------------------------------------------------------------- @@ -434,13 +445,15 @@ export class BudgetService { // ------------------------------------------------------------------------- setItemPayers(id: string | number, tripId: string | number, payers: { user_id: number; amount: number }[]) { - const item = this.db.get('SELECT id FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId); - if (!item) return null; - this.writeItemPayers(id, payers); - const updated = this.db.get('SELECT * FROM budget_items WHERE id = ?', id)!; - updated.members = this.loadItemMembers(id); - updated.payers = this.loadItemPayers(id); - return updated; + return this.db.transaction(() => { + const item = this.db.get('SELECT id FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId); + if (!item) return null; + this.writeItemPayers(id, payers); + const updated = this.db.get('SELECT * FROM budget_items WHERE id = ?', id)!; + updated.members = this.loadItemMembers(id); + updated.payers = this.loadItemPayers(id); + return updated; + }); } deleteBudgetItem(id: string | number, tripId: string | number): boolean { @@ -455,47 +468,52 @@ export class BudgetService { // ------------------------------------------------------------------------- updateMembers(id: string | number, tripId: string | number, userIds: number[]) { - const item = this.db.get('SELECT * FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId); - if (!item) return null; + return this.db.transaction(() => { + const item = this.db.get('SELECT * FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId); + if (!item) return null; - const existingPaid: Record = {}; - const existing = this.db.all<{ user_id: number; paid: number }>('SELECT user_id, paid FROM budget_item_members WHERE budget_item_id = ?', id); - for (const e of existing) existingPaid[e.user_id] = e.paid; + const existingPaid: Record = {}; + const existing = this.db.all<{ user_id: number; paid: number }>('SELECT user_id, paid FROM budget_item_members WHERE budget_item_id = ?', id); + for (const e of existing) existingPaid[e.user_id] = e.paid; - this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); + this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); - const known = this.knownUserIds(userIds); - const memberIds = userIds.filter(uid => known.has(uid)); - if (memberIds.length > 0) { - const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid) VALUES (?, ?, ?)'); - for (const userId of memberIds) insert.run(id, userId, existingPaid[userId] || 0); - this.db.run('UPDATE budget_items SET persons = ? WHERE id = ?', memberIds.length, id); - } else { - this.db.run('UPDATE budget_items SET persons = NULL WHERE id = ?', id); - } + const known = this.knownUserIds(userIds); + const memberIds = userIds.filter(uid => known.has(uid)); + if (memberIds.length > 0) { + const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid) VALUES (?, ?, ?)'); + for (const userId of memberIds) insert.run(id, userId, existingPaid[userId] || 0); + this.db.run('UPDATE budget_items SET persons = ? WHERE id = ?', memberIds.length, id); + } else { + this.db.run('UPDATE budget_items SET persons = NULL WHERE id = ?', id); + } - const members = this.loadItemMembers(id).map(m => ({ ...m, avatar_url: avatarUrl(m) })); - const updated = this.db.get('SELECT * FROM budget_items WHERE id = ?', id)!; - return { members, item: updated }; + // loadItemMembers already applies avatar_url — the legacy second .map was redundant. + const members = this.loadItemMembers(id); + const updated = this.db.get('SELECT * FROM budget_items WHERE id = ?', id)!; + return { members, item: updated }; + }); } removeUserFromBudgetItems(userId: number): void { - const itemIds = this.db.all<{ budget_item_id: number }>( - 'SELECT DISTINCT budget_item_id FROM budget_item_members WHERE user_id = ?', - userId, - ).map(r => r.budget_item_id); - if (itemIds.length === 0) { - return; - } + this.db.transaction(() => { + const itemIds = this.db.all<{ budget_item_id: number }>( + 'SELECT DISTINCT budget_item_id FROM budget_item_members WHERE user_id = ?', + userId, + ).map(r => r.budget_item_id); + if (itemIds.length === 0) { + return; + } - this.db.run('DELETE FROM budget_item_members WHERE user_id = ?', userId); + this.db.run('DELETE FROM budget_item_members WHERE user_id = ?', userId); - const remaining = this.db.prepare('SELECT COUNT(*) AS count FROM budget_item_members WHERE budget_item_id = ?'); - const setPersons = this.db.prepare('UPDATE budget_items SET persons = ? WHERE id = ?'); - for (const itemId of itemIds) { - const { count } = remaining.get(itemId) as { count: number }; - setPersons.run(count || null, itemId); - } + const remaining = this.db.prepare('SELECT COUNT(*) AS count FROM budget_item_members WHERE budget_item_id = ?'); + const setPersons = this.db.prepare('UPDATE budget_items SET persons = ? WHERE id = ?'); + for (const itemId of itemIds) { + const { count } = remaining.get(itemId) as { count: number }; + setPersons.run(count || null, itemId); + } + }); } toggleMemberPaid(id: string | number, tripId: string | number, userId: string | number, paid: boolean) { @@ -705,25 +723,44 @@ export class BudgetService { // Settlements (persisted settle-up transfers — history + undo) // ------------------------------------------------------------------------- - listSettlements(tripId: string | number) { - const rows = this.db.all(` + // Settlement usernames use COALESCE(display_name, username) like every item + // query (the legacy raw fu.username was the odd one out). + private static readonly SETTLEMENT_SELECT = ` SELECT s.id, s.trip_id, s.from_user_id, s.to_user_id, s.amount, s.currency, s.exchange_rate, s.created_at, s.created_by_user_id, - fu.username AS from_username, fu.avatar AS from_avatar, - tu.username AS to_username, tu.avatar AS to_avatar + COALESCE(fu.display_name, fu.username) AS from_username, fu.avatar AS from_avatar, + COALESCE(tu.display_name, tu.username) AS to_username, tu.avatar AS to_avatar FROM budget_settlements s JOIN users fu ON s.from_user_id = fu.id JOIN users tu ON s.to_user_id = tu.id - WHERE s.trip_id = ? - ORDER BY s.created_at DESC, s.id DESC - `, tripId); - return rows.map(r => ({ + `; + + private mapSettlementRow(r: SettlementRow) { + return { id: r.id, trip_id: r.trip_id, from_user_id: r.from_user_id, to_user_id: r.to_user_id, amount: r.amount, currency: r.currency ?? null, exchange_rate: r.exchange_rate ?? 1, created_at: r.created_at, created_by_user_id: r.created_by_user_id, from_username: r.from_username, from_avatar_url: avatarUrl({ avatar: r.from_avatar }), to_username: r.to_username, to_avatar_url: avatarUrl({ avatar: r.to_avatar }), - })); + }; + } + + listSettlements(tripId: string | number) { + const rows = this.db.all( + `${BudgetService.SETTLEMENT_SELECT} + WHERE s.trip_id = ? + ORDER BY s.created_at DESC, s.id DESC + `, tripId); + return rows.map(r => this.mapSettlementRow(r)); + } + + /** Targeted single-row read (the legacy re-select was a full listSettlements scan). */ + getSettlement(id: string | number, tripId: string | number) { + const row = this.db.get( + `${BudgetService.SETTLEMENT_SELECT} + WHERE s.trip_id = ? AND s.id = ? + `, tripId, id); + return row ? this.mapSettlementRow(row) : null; } /** Raw settlement insert (no FX freeze) — the REST path wraps it in createSettlement. */ @@ -739,7 +776,7 @@ export class BudgetService { data.exchange_rate != null ? data.exchange_rate : 1, createdByUserId ?? null, ); - return this.listSettlements(tripId).find(s => s.id === Number(result.lastInsertRowid)) || null; + return this.getSettlement(Number(result.lastInsertRowid), tripId); } /** Raw settlement update (no FX freeze) — the REST path wraps it in updateSettlement. */ @@ -762,7 +799,7 @@ export class BudgetService { data.exchange_rate !== undefined ? 1 : null, data.exchange_rate !== undefined ? data.exchange_rate : 1, id, ); - return this.listSettlements(tripId).find(s => s.id === Number(id)) || null; + return this.getSettlement(id, tripId); } deleteSettlement(id: string | number, tripId: string | number): boolean { @@ -795,7 +832,7 @@ export class BudgetService { return this.createBudgetItem(tripId, data); } - async update(id: string, tripId: string, data: Parameters[2]) { + async update(id: string | number, tripId: string | number, data: Parameters[2]) { await this.freezeForeignRate(tripId, data, id); return this.updateBudgetItem(id, tripId, data); } @@ -808,18 +845,18 @@ export class BudgetService { return this.setItemPayers(id, tripId, payers); } - async createSettlement(tripId: string, data: { from_user_id: number; to_user_id: number; amount: number; currency?: string | null }, userId: number) { + async createSettlement(tripId: string | number, data: { from_user_id: number; to_user_id: number; amount: number; currency?: string | null }, userId: number) { // Freeze the FX rate for the display currency the amount was entered in so the // transfer keeps cancelling its expense when live rates drift (#1445). await this.freezeForeignRate(tripId, data); return this.insertSettlement(tripId, data, userId); } - async updateSettlement(id: string, tripId: string, data: { from_user_id: number; to_user_id: number; amount: number; currency?: string | null }) { + async updateSettlement(id: string | number, tripId: string | number, data: { from_user_id: number; to_user_id: number; amount: number; currency?: string | null }) { // Pass the settlement's stored currency so an edit that doesn't change it keeps // the already-frozen rate (#1445) — otherwise a live-rate drift would re-open a // settled position on an unrelated edit. - const existing = this.listSettlements(tripId).find((s) => s.id === Number(id)); + const existing = this.getSettlement(id, tripId); await this.freezeForeignRate(tripId, data, undefined, existing?.currency ?? null); return this.applySettlementUpdate(id, tripId, data); } diff --git a/server/tests/unit/mcp/tools-budget-advanced.test.ts b/server/tests/unit/mcp/tools-budget-advanced.test.ts index d54cf2226..5266e7bd6 100644 --- a/server/tests/unit/mcp/tools-budget-advanced.test.ts +++ b/server/tests/unit/mcp/tools-budget-advanced.test.ts @@ -83,7 +83,13 @@ describe('Tool: set_budget_item_members', () => { // Regression: returns a hydrated item, not the raw row from updateMembers. expect(data.item.members.map((m: any) => m.user_id)).toEqual([user.id]); expect(Array.isArray(data.item.payers)).toBe(true); - expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'budget:members-updated', expect.any(Object)); + // Quirk fix: the broadcast carries the REST payload shape (the legacy + // MCP-specific { item } shape was a silent no-op in the client handler). + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'budget:members-updated', expect.objectContaining({ + itemId: item.id, + members: expect.arrayContaining([expect.objectContaining({ user_id: user.id })]), + persons: 1, + })); }); }); @@ -215,7 +221,9 @@ describe('Tool: toggle_budget_member_paid', () => { }); const data = parseToolResult(result) as any; expect(data.member).toBeDefined(); - expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'budget:member-paid-updated', expect.any(Object)); + // Quirk fix: the broadcast carries the REST payload shape (the legacy + // MCP-specific { itemId, member } shape was a silent no-op in the client). + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'budget:member-paid-updated', expect.objectContaining({ itemId: item.id, userId: user.id, paid: 1 })); }); }); diff --git a/server/tests/unit/nest/budget.service.calc.test.ts b/server/tests/unit/nest/budget.service.calc.test.ts index 8100f3659..cbfce335f 100644 --- a/server/tests/unit/nest/budget.service.calc.test.ts +++ b/server/tests/unit/nest/budget.service.calc.test.ts @@ -485,9 +485,11 @@ describe('applySettlementUpdate', () => { return { get: vi.fn(), all: vi.fn(), run }; } if (sql.includes('FROM budget_settlements')) { - return { get: vi.fn(), all: vi.fn(() => [ - { id: 7, trip_id: 1, from_user_id: 2, to_user_id: 1, amount: 10.13, from_username: 'bob', to_username: 'alice', from_avatar: null, to_avatar: null }, - ]), run: vi.fn() }; + // Quirk fix: the re-select is a targeted single-row get, not a full + // listSettlements scan. + return { get: vi.fn(() => ( + { id: 7, trip_id: 1, from_user_id: 2, to_user_id: 1, amount: 10.13, from_username: 'bob', to_username: 'alice', from_avatar: null, to_avatar: null } + )), all: vi.fn(() => []), run: vi.fn() }; } return { get: vi.fn(), all: vi.fn(() => []), run: vi.fn() }; }); diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index 5cf07d0e8..f8eed3698 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -3,7 +3,9 @@ * Uses a real in-memory SQLite DB so the SQL WHERE clauses are exercised. * BUDGET-SVC-DB-001 through 014 moved 1:1 from the legacy * tests/unit/services/budgetServiceDb.test.ts; 015–018 pin the budget.bridge - * delegation (the bridge instance runs over the same mocked db Proxy). + * delegation (the bridge instance runs over the same mocked db Proxy); + * 019–020 pin the post-fold quirk fixes (COALESCE(display_name) on + * settlements, transactional multi-statement writes). */ import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; @@ -421,3 +423,30 @@ describe('budget.bridge delegation', () => { expect(row.reservation_id).toBe(reservationId); }); }); + +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' }); + const { user: bob } = createUser(testDb, { username: 'bob' }); + testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Alice Displayed', alice.id); + const trip = createTrip(testDb, alice.id); + + const created = budget.insertSettlement(trip.id, { from_user_id: alice.id, to_user_id: bob.id, amount: 10 }, alice.id); + + expect(created!.from_username).toBe('Alice Displayed'); + expect(created!.to_username).toBe('bob'); + expect(budget.listSettlements(trip.id)[0].from_username).toBe('Alice Displayed'); + }); + + it('BUDGET-SVC-DB-020: a failing item insert rolls back the category-order side write (quirk fix)', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + // NOT NULL violation on name fires after the category-order upsert — the + // legacy non-transactional create leaked the budget_category_order row. + expect(() => budget.createBudgetItem(trip.id, { name: null as unknown as string, category: 'atomic-test' })).toThrow(); + + const cat = testDb.prepare("SELECT 1 FROM budget_category_order WHERE trip_id = ? AND category = 'atomic-test'").get(trip.id); + expect(cat).toBeUndefined(); + }); +}); diff --git a/server/tests/unit/nest/budget.service.test.ts b/server/tests/unit/nest/budget.service.test.ts index 3d6beebb7..dedcd1887 100644 --- a/server/tests/unit/nest/budget.service.test.ts +++ b/server/tests/unit/nest/budget.service.test.ts @@ -150,10 +150,13 @@ describe('BudgetService', () => { it('updateSettlement threads the stored currency through the freeze', async () => { const s = svc(); const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); - vi.spyOn(s, 'listSettlements').mockReturnValue([{ id: 7, currency: 'USD' }] as never); + // Quirk fix: the stored-currency lookup is a targeted getSettlement, not + // a full listSettlements scan. + const getSpy = vi.spyOn(s, 'getSettlement').mockReturnValue({ id: 7, currency: 'USD' } as never); const applySpy = vi.spyOn(s, 'applySettlementUpdate').mockReturnValue({ id: 7 } as never); await s.updateSettlement('7', '5', { from_user_id: 1, to_user_id: 2, amount: 12, currency: 'USD' }); // the settlement's stored currency is threaded through so an unchanged-currency edit keeps the frozen rate (#1445) + expect(getSpy).toHaveBeenCalledWith('7', '5'); expect(freezeSpy).toHaveBeenCalledWith('5', { from_user_id: 1, to_user_id: 2, amount: 12, currency: 'USD' }, undefined, 'USD'); expect(applySpy).toHaveBeenCalledWith('7', '5', { from_user_id: 1, to_user_id: 2, amount: 12, currency: 'USD' }); });