mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(reservations): keep a mirrored booking price without keying it on the expense
The carry-over was conditional on a budget_items row still being linked to the booking, which drops a real price in two shipped cases. The booking importer 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; an MCP booking created with a price of 0 is in the same position. Both lost the price on the first edit. The condition is gone: a payload that does not name the key keeps what is stored, and naming it, null included, is taken at face value so a price can still be removed on purpose. What stops a price from outliving its expense is now the other end. deleteBudgetItem is the single chokepoint for all four delete paths, and it clears the mirror there, inside the same transaction as the delete. That also removes an ordering hazard the linked-row check had: the controller runs update() before it syncs the budget, so on an explicit clear the row still existed and the price would have come back a moment before it went. The predicate moves into reservation-metadata.ts so it can be read and tested on its own, and the update_transport tool description no longer promises that metadata is replaced wholesale.
This commit is contained in:
Maurice
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
@@ -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<string, unknown> | null {
|
||||
if (!stored) return null;
|
||||
try {
|
||||
const parsed: unknown = JSON.parse(stored);
|
||||
return parsed && typeof parsed === 'object' && !Array.isArray(parsed)
|
||||
? (parsed as Record<string, unknown>)
|
||||
: 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<string, unknown>;
|
||||
if ('price' in next) return incoming;
|
||||
|
||||
const prev = parseStored(stored);
|
||||
if (!prev || prev.price === undefined) return incoming;
|
||||
|
||||
const kept: Record<string, unknown> = { ...next, price: prev.price };
|
||||
if (prev.priceCurrency !== undefined && !('priceCurrency' in next)) kept.priceCurrency = prev.priceCurrency;
|
||||
return kept;
|
||||
}
|
||||
@@ -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(),
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
if (next.price !== undefined) return incoming;
|
||||
let prev: Record<string, unknown>;
|
||||
try {
|
||||
const parsed: unknown = JSON.parse(stored);
|
||||
if (!parsed || typeof parsed !== 'object') return incoming;
|
||||
prev = parsed as Record<string, unknown>;
|
||||
} 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<string, unknown> = { ...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 } {
|
||||
|
||||
@@ -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' });
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
Reference in New Issue
Block a user