test(accommodations): cover the paths the day stop added, and the ratchet fell on

Six lines and four functions the mirror brought in were never entered by a test,
which is what took the domain under its coverage floor.

The keepStop branch of a delete, the plugin surface's own update, and the
transaction that has to roll a place and its stay back together. Plus every
sender a surface hands announceMirror: REST and the three plugin methods build
one each, and a fixture that swallowed the call let all four go unexercised --
the one link between a stay write and the sockets everyone else is watching.
This commit is contained in:
Maurice
2026-09-13 12:12:38 +02:00
parent edf0538260
commit eff4605ef3
4 changed files with 149 additions and 9 deletions
@@ -370,6 +370,28 @@ describe('Tool: create_place_accommodation', () => {
expect(testDb.prepare('SELECT COUNT(*) as n FROM places WHERE trip_id = ?').get(trip.id)).toMatchObject({ n: 0 });
});
});
it('a write that fails inside the transaction rolls both halves back and says so', async () => {
// The place, the stay and the day stop the stay implies go in together or not at
// all. A category that is not there is the cheapest way to make the first insert
// fail on a foreign key; what matters is that the tool answers with an error
// instead of a half-written trip.
const { user } = createUser(testDb);
const trip = createTrip(testDb, user.id);
const day = createDay(testDb, trip.id);
await withHarness(user.id, async (h) => {
const result = await h.client.callTool({
name: 'create_place_accommodation',
arguments: { tripId: trip.id, name: 'Hotel Nirgendwo', category_id: 99999, start_day_id: day.id, end_day_id: day.id },
});
expect(result.isError).toBe(true);
const { content } = result as { content: { type: string; text?: string }[] };
expect(content[0].text).toContain('Failed to create place and accommodation');
expect(testDb.prepare('SELECT COUNT(*) as n FROM places WHERE trip_id = ?').get(trip.id)).toMatchObject({ n: 0 });
expect(testDb.prepare('SELECT COUNT(*) as n FROM day_accommodations WHERE trip_id = ?').get(trip.id)).toMatchObject({ n: 0 });
expect(testDb.prepare('SELECT COUNT(*) as n FROM day_assignments WHERE day_id = ?').get(day.id)).toMatchObject({ n: 0 });
});
});
});
// ---------------------------------------------------------------------------
@@ -96,6 +96,35 @@ describe('AccommodationsController (parity with the legacy accommodations sub-ro
.toEqual({ accommodation: { id: 9 }, assignment: mirror.created, removedAssignments: mirror.removed });
expect(announceMirror).toHaveBeenCalledWith('5', mirror, expect.any(Function), 'sock');
});
it('ACC-CTL-003 404s a place or day that is not on this trip, before the write', () => {
const get = vi.fn().mockReturnValue({ id: 9 });
const update = vi.fn();
const validateRefs = vi.fn().mockReturnValue([{ message: 'Place not found' }]);
const svc = makeService({ get, update, validateRefs } as Partial<AccommodationsService>);
expect(thrown(() => new AccommodationsController(svc).update(user, '5', '9', refs)))
.toEqual({ status: 404, body: { error: 'Place not found' } });
expect(update).not.toHaveBeenCalled();
});
it('ACC-CTL-004 the mirror sender is the same broadcast, socket id and all', () => {
// announceMirror decides which events a stay write implies; the controller only
// hands it the door out. That door has to carry the sender's socket id, or the
// session that just moved the booking is told about its own stop twice.
const get = vi.fn().mockReturnValue({ id: 9 });
const mirror = { created: { id: 78, day_id: 11 }, removed: [{ id: 77, dayId: 10 }], stamped: null };
const update = vi.fn().mockReturnValue({ accommodation: { id: 9 }, mirror });
const broadcast = vi.fn();
type Send = (event: string, payload: unknown) => void;
const announceMirror = vi.fn((_tripId: string, m: typeof mirror, send: Send) => {
for (const stop of m.removed) send('assignment:deleted', { assignmentId: stop.id, dayId: stop.dayId });
if (m.created) send('assignment:created', { assignment: m.created });
});
const svc = makeService({ get, update, broadcast, announceMirror } as Partial<AccommodationsService>);
new AccommodationsController(svc).update(user, '5', '9', refs, 'sock');
expect(broadcast).toHaveBeenCalledWith('5', 'assignment:deleted', { assignmentId: 77, dayId: 10 }, 'sock');
expect(broadcast).toHaveBeenCalledWith('5', 'assignment:created', { assignment: mirror.created }, 'sock');
});
});
describe('DELETE /:id', () => {
@@ -130,5 +159,24 @@ describe('AccommodationsController (parity with the legacy accommodations sub-ro
expect(broadcast).toHaveBeenCalledWith('5', 'reservation:deleted', { reservationId: 4 }, 'sock');
expect(broadcast).toHaveBeenCalledWith('5', 'reservation:deleted', { reservationId: 5 }, 'sock');
});
it('ACC-CTL-005 ?keepStop=true is what turns a night back into an ordinary pause', () => {
// The road trip popup switching the night off: the booking goes, the stop it
// brought stays where it is in the drive. Nothing else may pass that flag, so
// only the literal string counts.
const get = vi.fn().mockReturnValue({ id: 9 });
const remove = vi.fn().mockReturnValue({
linkedReservationId: null, deletedBudgetItemId: null,
linkedReservationIds: [], deletedBudgetItemIds: [], mirror: noMirror,
});
const svc = makeService({ get, remove } as Partial<AccommodationsService>);
const controller = new AccommodationsController(svc);
controller.remove(user, '5', '9', 'sock', 'true');
expect(remove).toHaveBeenLastCalledWith('9', { keepStop: true });
controller.remove(user, '5', '9', 'sock');
expect(remove).toHaveBeenLastCalledWith('9', { keepStop: false });
});
});
});
@@ -779,6 +779,38 @@ describe('the day stop a booking implies', () => {
expect(mirror.removed).toEqual([]);
});
it('ACC-032b keepStop hands the mirrored stop to the traveller instead of taking it away', () => {
// The road trip popup switching a night back to an ordinary pause: the booking is
// what was cancelled, not the place, and the stop sits mid-drive where re-adding it
// would land it at the end of the day instead.
const { user } = createUser(testDb);
const trip = createTrip(testDb, user.id);
const day = createDay(testDb, trip.id);
const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' });
const { accommodation } = book(trip.id, place.id, day.id, day.id);
const mirrored = stopsOn(day.id)[0];
const { mirror } = svc.deleteAccommodation(accommodation.id, { keepStop: true });
// Still there, and now unmarked: nothing may move or delete it on the next edit.
expect(stopsOn(day.id)).toEqual([expect.objectContaining({ id: mirrored.id, place_id: place.id, accommodation_id: null })]);
// Nothing to announce either — no session has to take a stop off its day plan.
expect(mirror.removed).toEqual([]);
expect(testDb.prepare('SELECT id FROM day_accommodations WHERE id = ?').get(accommodation.id)).toBeUndefined();
});
it('ACC-032c remove() forwards keepStop, which is the only door the REST route uses', () => {
const { user } = createUser(testDb);
const trip = createTrip(testDb, user.id);
const day = createDay(testDb, trip.id);
const place = createPlace(testDb, trip.id, { name: 'Hotel Adlon' });
const { accommodation } = book(trip.id, place.id, day.id, day.id);
svc.remove(accommodation.id, { keepStop: true });
expect(stopsOn(day.id)).toEqual([expect.objectContaining({ place_id: place.id, accommodation_id: null })]);
});
it('ACC-033 announceMirror sends the removal before the arrival', () => {
// Order matters on a move: the day plan would briefly hold the place twice if the
// arrival went first.
@@ -30,7 +30,7 @@ const req = (method: string, params: Record<string, unknown> = {}): RpcRequest =
const events = (r: { broadcast: ReturnType<typeof vi.fn> }) => r.broadcast.mock.calls.map((c) => c[1]);
/** Trip 1 belongs to user 42; reservation 5 and accommodation 11 sit on it. */
function build(opts: { canEdit?: boolean; cascade?: boolean; seenActions?: string[] } = {}) {
function build(opts: { canEdit?: boolean; cascade?: boolean; stop?: boolean; seenActions?: string[] } = {}) {
const realtime = { broadcast: vi.fn() } as unknown as RealtimeService & { broadcast: ReturnType<typeof vi.fn> };
const reservations = {
create: vi.fn(() => ({ reservation: { id: 40 }, accommodationCreated: !!opts.cascade })),
@@ -45,25 +45,35 @@ function build(opts: { canEdit?: boolean; cascade?: boolean; seenActions?: strin
syncBudgetOnUpdate: vi.fn(),
notifyBookingChange: vi.fn(),
} as unknown as ReservationsService & Record<string, ReturnType<typeof vi.fn>>;
/** A stay write that left the day plan alone. */
const noMirror = () => ({ created: null, removed: [], stamped: null });
/** What a stay write did to the day plan, on top of writing the stay itself. */
type Mirror = { created: { id: number; day_id: number } | null; removed: { id: number; dayId: number }[]; stamped: null };
/** A stay write that left the day plan alone, which is what most of these cases are. */
const noMirror = (): Mirror => ({ created: null, removed: [], stamped: null });
/** With `stop`, the write moved its day stop from day 3 to day 4. */
const mirror = (): Mirror => (opts.stop ? { created: { id: 78, day_id: 4 }, removed: [{ id: 77, dayId: 3 }], stamped: null } : noMirror());
// The lodging blocks live in AccommodationsService; AccommodationsRpc still calls its
// injected copy `days`, which is why the fixture keeps that name.
const days = {
validateAccommodationRefs: vi.fn((_t: number, placeId?: number) => (placeId === 404 ? [{ message: 'place 404 is not on this trip' }] : [])),
createAccommodation: vi.fn(() => ({ accommodation: { id: 60 }, mirror: noMirror() })),
createAccommodation: vi.fn(() => ({ accommodation: { id: 60 }, mirror: mirror() })),
getAccommodation: vi.fn((id: number) => (id === 11 ? { id: 11 } : undefined)),
updateAccommodation: vi.fn(() => ({ accommodation: { id: 11 }, mirror: noMirror() })),
updateAccommodation: vi.fn(() => ({ accommodation: { id: 11 }, mirror: mirror() })),
deleteAccommodation: vi.fn(() => ({
linkedReservationId: opts.cascade ? 40 : null,
deletedBudgetItemId: opts.cascade ? 7 : null,
linkedReservationIds: opts.cascade ? [40] : [],
deletedBudgetItemIds: opts.cascade ? [7] : [],
mirror: noMirror(),
mirror: mirror(),
})),
// The real one broadcasts the day stop a booking writes; these cases are about
// the booking events, so the fixture keeps it inert and BOOK-RPC-016 covers it.
announceMirror: vi.fn(),
// Stands in for the real fan-out rather than swallowing it, so the sender each
// handler builds is exercised: that sender is the plugin surface's only link
// between a stay write and the sockets, and a mock that never calls it would
// let a broken one through. Inert whenever the mirror is empty, which is every
// case not built with `stop`.
announceMirror: vi.fn((_tripId: number, m: Mirror, send: (event: string, payload: unknown) => void) => {
for (const stop of m.removed) send('assignment:deleted', { assignmentId: stop.id, dayId: stop.dayId });
if (m.created) send('assignment:created', { assignment: m.created });
}),
} as unknown as AccommodationsService & Record<string, ReturnType<typeof vi.fn>>;
const guards = new PluginGuards(
{
@@ -257,6 +267,34 @@ describe('AccommodationsRpc', () => {
expect(f.days.announceMirror).toHaveBeenCalledWith(1, { created: null, removed: [], stamped: null }, expect.any(Function));
});
it('BOOK-RPC-017b editing a block writes it, announces it and answers with the block', async () => {
const f = build();
const res = await f.host().dispatch(req('accommodations.update', { tripId: 1, accommodationId: 11, input: { check_in: '15:00' } }), 42);
expect(f.days.updateAccommodation).toHaveBeenCalledWith(11, { id: 11 }, expect.objectContaining({ check_in: '15:00' }));
expect(events(f.realtime)).toEqual(['accommodation:updated']);
expect(f.days.announceMirror).toHaveBeenCalledWith(1, { created: null, removed: [], stamped: null }, expect.any(Function));
// The block itself, not the { accommodation, mirror } pair the service returns:
// the mirror is a broadcast concern and plugins never asked for it.
expect(res).toMatchObject({ k: 'res', result: { id: 11 } });
});
it('BOOK-RPC-017c what announceMirror sends goes out over the trip socket, on all three writes', async () => {
// The sender each handler hands announceMirror is the plugin surface's only link
// between a stay write and the day plan every other session is looking at. What
// belongs in the mirror is the service's call; that it reaches a socket at all is
// this one's, which is why the fixture's mirror is the same on all three.
for (const request of [
req('accommodations.create', { tripId: 1, input: { place_id: 7, start_day_id: 3, end_day_id: 4 } }),
req('accommodations.update', { tripId: 1, accommodationId: 11, input: { start_day_id: 4 } }),
req('accommodations.delete', { tripId: 1, accommodationId: 11 }),
]) {
const f = build({ stop: true });
await f.host().dispatch(request, 42);
expect(f.realtime.broadcast).toHaveBeenCalledWith(1, 'assignment:deleted', { assignmentId: 77, dayId: 3 });
expect(f.realtime.broadcast).toHaveBeenCalledWith(1, 'assignment:created', { assignment: { id: 78, day_id: 4 } });
}
});
it('BOOK-RPC-015d deleting a block without a partner cascades only its own event', async () => {
const f = build();
await f.host().dispatch(req('accommodations.delete', { tripId: 1, accommodationId: 11 }), 42);