diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index 83858b4d5..13b272f93 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -546,10 +546,42 @@ export class BudgetService { } deleteBudgetItem(id: string | number, tripId: string | number): boolean { - const item = this.db.get('SELECT id FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId); + const item = this.db.get<{ id: number; reservation_id: number | null }>( + 'SELECT id, reservation_id FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId, + ); if (!item) return false; - this.db.run('DELETE FROM budget_items WHERE id = ?', id); - return true; + return this.db.transaction(() => { + this.db.run('DELETE FROM budget_items WHERE id = ?', id); + // The booking keeps a copy of this expense's total in its metadata, and + // the reservation update path preserves that copy across edits. With the + // expense gone there is nothing left to mirror, so drop it here rather + // than leave a price on the card that no longer has anything behind it. + if (item.reservation_id) this.clearReservationPrice(tripId, item.reservation_id); + return true; + }); + } + + /** + * The counterpart to syncReservationPrice: take the mirrored total back off + * the booking. Non-fatal, exactly like the writer. + */ + private clearReservationPrice(tripId: string | number, reservationId: number): void { + try { + const reservation = this.db.get<{ id: number; metadata: string | null }>( + 'SELECT id, metadata FROM reservations WHERE id = ? AND trip_id = ?', + reservationId, tripId, + ); + if (!reservation?.metadata) return; + const meta = JSON.parse(reservation.metadata); + if (!meta || typeof meta !== 'object' || meta.price === undefined) return; + delete meta.price; + delete meta.priceCurrency; + this.db.run('UPDATE reservations SET metadata = ? WHERE id = ?', JSON.stringify(meta), reservation.id); + const updatedRes = this.db.get('SELECT * FROM reservations WHERE id = ?', reservation.id); + this.realtime.broadcast(String(tripId), 'reservation:updated', { reservation: updatedRes }, undefined); + } catch (err) { + console.error('[budget] Failed to clear the mirrored price from the reservation:', err); + } } // ------------------------------------------------------------------------- diff --git a/server/src/nest/reservations/reservation-metadata.ts b/server/src/nest/reservations/reservation-metadata.ts new file mode 100644 index 000000000..bcfc3cfd0 --- /dev/null +++ b/server/src/nest/reservations/reservation-metadata.ts @@ -0,0 +1,55 @@ +/** + * Metadata keys on a reservation that the booking forms do not own. + * + * `price` and `priceCurrency` are written by the expense side + * (BudgetService.syncReservationPrice) and by the booking importer, never by a + * booking form. The forms rebuild metadata from their own fields on every save + * and carry only `transit` and `airtrail_ids` across, so an ordinary edit used + * to drop the price and the card lost it until the expense was saved again + * (#2233). The same happened unattended on the AirTrail poll, which updates a + * reservation with a mapped metadata object that never contains a price. + * + * Kept out of the reservations service so it can be exercised on its own. + */ + +/** Read a stored metadata column into an object, or null if it is not one. */ +function parseStored(stored: string | null | undefined): Record | null { + if (!stored) return null; + try { + const parsed: unknown = JSON.parse(stored); + return parsed && typeof parsed === 'object' && !Array.isArray(parsed) + ? (parsed as Record) + : null; + } catch { + return null; + } +} + +/** + * Carry `price` / `priceCurrency` from the stored metadata into an incoming + * object that does not name them. + * + * Everything else passes through untouched, which is what keeps the caller in + * control: `undefined` (no metadata on the payload) leaves the row alone, `null` + * clears the column, and naming the key — including setting it to null — is + * taken at face value, so a price can still be removed deliberately. + * + * There is deliberately no "only while an expense is linked" condition. The + * importer stamps a price onto the booking whether or not the Costs addon is + * enabled, and an MCP booking created with a price of 0 gets no linked item + * either, so that condition would drop a real price on the first edit. What + * stops a stale price from outliving its expense is the other end: + * BudgetService.deleteBudgetItem clears the mirror when the expense goes away. + */ +export function keepMirroredPrice(incoming: unknown, stored: string | null | undefined): unknown { + if (!incoming || typeof incoming !== 'object' || Array.isArray(incoming)) return incoming; + const next = incoming as Record; + if ('price' in next) return incoming; + + const prev = parseStored(stored); + if (!prev || prev.price === undefined) return incoming; + + const kept: Record = { ...next, price: prev.price }; + if (prev.priceCurrency !== undefined && !('priceCurrency' in next)) kept.priceCurrency = prev.priceCurrency; + return kept; +} diff --git a/server/src/nest/reservations/reservations.mcp.ts b/server/src/nest/reservations/reservations.mcp.ts index 9671187c0..fba5bb671 100644 --- a/server/src/nest/reservations/reservations.mcp.ts +++ b/server/src/nest/reservations/reservations.mcp.ts @@ -785,7 +785,7 @@ export class ReservationsMcp { confirmation_number: z.string().max(100).optional(), url: urlField, notes: z.string().max(1000).optional(), - metadata: z.record(z.string(), z.string()).optional().describe('Type-specific metadata: flights → { airline, flight_number, departure_airport, arrival_airport }; trains → { train_number, platform, seat }. Replaces the stored metadata wholesale. Values are plain strings, so per-segment detail belongs in legs[], not here.'), + metadata: z.record(z.string(), z.string()).optional().describe('Type-specific metadata: flights → { airline, flight_number, departure_airport, arrival_airport }; trains → { train_number, platform, seat }. Replaces the stored metadata wholesale, except that a price mirrored from a linked expense survives unless you name the price key yourself. Values are plain strings, so per-segment detail belongs in legs[], not here.'), endpoints: endpointSchema, legs: legsSchema, needs_review: z.boolean().optional(), diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 62dd02a28..28dce5c35 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -4,6 +4,7 @@ import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import { ReservationsReadRepository, toTraveler } from './reservations-read.repository'; +import { keepMirroredPrice } from './reservation-metadata'; import type { Reservation, User } from '../../types'; import { BudgetService } from '../budget/budget.service'; import { typeToCostCategory } from '@trek/shared'; @@ -720,14 +721,12 @@ export class ReservationsService { } } - // metadata.price / priceCurrency are owned by the linked expense - // (BudgetService.syncReservationPrice writes them), not by the booking form: - // the client rebuilds metadata from the form on every save and only carries - // transit / airtrail_ids over, so a plain edit used to wipe the price off the - // card until the expense was re-saved. Keep them while a linked item exists; - // a payload that sets them itself, or a booking without a linked expense, - // is left untouched. - const nextMetadata = this.keepLinkedExpensePrice(id, tripId, metadata, current.metadata); + // metadata.price / priceCurrency are written by the expense side and by the + // booking importer, never by the booking form: the client rebuilds metadata + // from its fields on every save and carries only transit / airtrail_ids + // over, so a plain edit used to wipe the price off the card until the + // expense was re-saved. A payload that does not name the keys keeps them. + const nextMetadata = keepMirroredPrice(metadata, current.metadata); const resolvedType = (type ?? current.type) || 'other'; const nextReservationTime = resolvedType === 'hotel' @@ -833,32 +832,6 @@ export class ReservationsService { return { reservation, accommodationChanged }; } - /** - * Carry `price` / `priceCurrency` from the stored metadata into an incoming - * metadata object that doesn't set them, as long as a budget item is still - * linked to the booking. Returns the incoming value untouched otherwise — - * `undefined` (field absent) and `null` (explicit clear) pass straight through. - */ - private keepLinkedExpensePrice(id: string | number, tripId: string | number, incoming: unknown, stored: string | null | undefined): unknown { - if (!incoming || typeof incoming !== 'object' || Array.isArray(incoming) || !stored) return incoming; - const next = incoming as Record; - if (next.price !== undefined) return incoming; - let prev: Record; - try { - const parsed: unknown = JSON.parse(stored); - if (!parsed || typeof parsed !== 'object') return incoming; - prev = parsed as Record; - } catch { - return incoming; - } - if (prev.price === undefined) return incoming; - const linked = this.db.get('SELECT id FROM budget_items WHERE trip_id = ? AND reservation_id = ?', tripId, id); - if (!linked) return incoming; - const kept: Record = { ...next, price: prev.price }; - if (prev.priceCurrency !== undefined && next.priceCurrency === undefined) kept.priceCurrency = prev.priceCurrency; - return kept; - } - /** The accommodation + budget-item + reservation deletes are one logical * cascade — all-or-nothing. */ remove(id: string | number, tripId: string | number): { deleted: { id: number; title: string; type: string; accommodation_id: number | null } | undefined; accommodationDeleted: boolean; deletedBudgetItemId: number | null } { diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index c81542abd..35b2ed133 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -680,6 +680,37 @@ describe('negative amounts persist end-to-end (#2176)', () => { }); }); +describe('deleting an expense takes its price off the booking (#2233)', () => { + it('BUDGET-SVC-DB-040: the mirrored price and currency are cleared, the rest of the metadata stays', () => { + // The reservation update path keeps metadata.price across booking edits, so + // the expense side has to remove it when the expense itself goes away. + // Without this the card would show a price with nothing behind it, for good. + const { user } = createUser(testDb, { username: 'owner' }); + const trip = createTrip(testDb, user.id, { title: 'Trip' }); + const res = testDb.prepare( + "INSERT INTO reservations (trip_id, title, type, metadata) VALUES (?, 'Flight', 'flight', ?)", + ).run(trip.id, JSON.stringify({ airline: 'CZ', seat: '12A', price: '2040', priceCurrency: 'CNY' })); + const reservationId = Number(res.lastInsertRowid); + + const item = budget.createBudgetItem(trip.id, { name: 'Flight', total_price: 2040 }); + testDb.prepare('UPDATE budget_items SET reservation_id = ? WHERE id = ?').run(reservationId, item.id); + + expect(budget.deleteBudgetItem(item.id, trip.id)).toBe(true); + + const after = testDb.prepare('SELECT metadata FROM reservations WHERE id = ?').get(reservationId) as { metadata: string }; + expect(JSON.parse(after.metadata)).toEqual({ airline: 'CZ', seat: '12A' }); + }); + + it('BUDGET-SVC-DB-041: an expense that was never linked to a booking deletes as before', () => { + const { user } = createUser(testDb, { username: 'owner' }); + const trip = createTrip(testDb, user.id, { title: 'Trip' }); + const item = budget.createBudgetItem(trip.id, { name: 'Coffee', total_price: 4 }); + + expect(budget.deleteBudgetItem(item.id, trip.id)).toBe(true); + expect(testDb.prepare('SELECT id FROM budget_items WHERE id = ?').get(item.id)).toBeUndefined(); + }); +}); + 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' }); diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index fcd3dd19c..ae30396df 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -213,21 +213,37 @@ describe('ReservationsService (DI-native, real SQL)', () => { expect(JSON.parse(reservation.metadata as string)).toEqual({ airline: 'CZ', seat: '12A', price: '2040', priceCurrency: 'CNY' }); }); - it('RESV-SVC-034: a payload that sets `price` itself, or a booking with no linked expense, is stored as sent', () => { + it('RESV-SVC-034: a payload that names `price` is taken at face value, set or cleared', () => { const { trip } = ownerTrip(); - const linkedRes = createReservation(testDb, trip.id, { title: 'Linked', type: 'flight' }); + const res = createReservation(testDb, trip.id, { title: 'Linked', type: 'flight' }); const item = createBudgetItem(testDb, trip.id, { name: 'Linked', total_price: 100 }); - testDb.prepare('UPDATE budget_items SET reservation_id = ? WHERE id = ?').run(linkedRes.id, item.id); - testDb.prepare('UPDATE reservations SET metadata = ? WHERE id = ?').run(JSON.stringify({ price: '100' }), linkedRes.id); - let current = svc.getReservation(String(linkedRes.id), String(trip.id))!; - let { reservation } = svc.update(String(linkedRes.id), String(trip.id), { metadata: { price: '150' } }, current); + testDb.prepare('UPDATE budget_items SET reservation_id = ? WHERE id = ?').run(res.id, item.id); + testDb.prepare('UPDATE reservations SET metadata = ? WHERE id = ?').run(JSON.stringify({ price: '100' }), res.id); + + let current = svc.getReservation(String(res.id), String(trip.id))!; + let { reservation } = svc.update(String(res.id), String(trip.id), { metadata: { price: '150' } }, current); expect(JSON.parse(reservation.metadata as string)).toEqual({ price: '150' }); - const orphanRes = createReservation(testDb, trip.id, { title: 'Orphan', type: 'flight' }); - testDb.prepare('UPDATE reservations SET metadata = ? WHERE id = ?').run(JSON.stringify({ price: '999' }), orphanRes.id); - current = svc.getReservation(String(orphanRes.id), String(trip.id))!; - ({ reservation } = svc.update(String(orphanRes.id), String(trip.id), { metadata: { seat: '1A' } }, current)); - expect(JSON.parse(reservation.metadata as string)).toEqual({ seat: '1A' }); + // Naming the key with a null is how a caller removes the price on purpose; + // it must not be read as "the form did not mention it". + current = svc.getReservation(String(res.id), String(trip.id))!; + ({ reservation } = svc.update(String(res.id), String(trip.id), { metadata: { price: null, seat: '1A' } }, current)); + expect(JSON.parse(reservation.metadata as string)).toEqual({ price: null, seat: '1A' }); + }); + + it('RESV-SVC-036: a price with no linked expense is kept too, because the importer stamps one either way', () => { + // booking-import writes metadata.price unconditionally but only creates the + // linked cost when the Costs addon is on and the price is above zero, so an + // instance with that addon off has bookings carrying a price and no budget + // row at all. Keying the carry-over on a linked item would drop those. + const { trip } = ownerTrip(); + const res = createReservation(testDb, trip.id, { title: 'Imported', type: 'flight' }); + testDb.prepare('UPDATE reservations SET metadata = ? WHERE id = ?') + .run(JSON.stringify({ price: '999', priceCurrency: 'EUR' }), res.id); + const current = svc.getReservation(String(res.id), String(trip.id))!; + + const { reservation } = svc.update(String(res.id), String(trip.id), { metadata: { seat: '1A' } }, current); + expect(JSON.parse(reservation.metadata as string)).toEqual({ seat: '1A', price: '999', priceCurrency: 'EUR' }); }); it('RESV-SVC-035: metadata null still clears the stored metadata, linked expense or not', () => {