From 2aeecd98fa70e26d8f895e68da04d800dbb8b55d Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 15:57:51 +0200 Subject: [PATCH] refactor(accommodations): stays through repositories inside the existing transactions --- .../DayAccommodations.repository.ts | 272 ++++++++++++++++ .../repositories/DayAssignments.repository.ts | 153 +++++++++ .../src/db/repositories/Places.repository.ts | 27 ++ .../repositories/Reservations.repository.ts | 133 +++++++- .../accommodations-domain.module.ts | 24 +- .../nest/accommodations/accommodations.mcp.ts | 16 +- .../accommodations/accommodations.service.ts | 304 ++++++++++++------ server/src/nest/days/days.module.ts | 17 +- server/src/nest/days/days.service.ts | 58 ++-- server/src/nest/places/places.service.ts | 21 +- .../tests/helpers/accommodations-service.ts | 13 +- server/tests/helpers/plugin-host.ts | 11 +- server/tests/integration/dayReorder.test.ts | 2 + server/tests/unit/nest/days.service.test.ts | 3 + server/tests/unit/nest/places.service.test.ts | 29 ++ .../unit/nest/trip-read-model.service.test.ts | 1 + server/tests/unit/nest/trips.service.test.ts | 1 + 17 files changed, 921 insertions(+), 164 deletions(-) diff --git a/server/src/db/repositories/DayAccommodations.repository.ts b/server/src/db/repositories/DayAccommodations.repository.ts index 224fce9fd..f381305c8 100644 --- a/server/src/db/repositories/DayAccommodations.repository.ts +++ b/server/src/db/repositories/DayAccommodations.repository.ts @@ -1,7 +1,85 @@ import type { DayAccommodations } from '../entities/DayAccommodations.entity'; import { coalesceParam, columnRef } from '../dialect/sql-functions'; +import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; +/** + * A `day_accommodations` row exactly as AC34 (`AccommodationsService + * .getAccommodation`) re-selects it — every scalar column of the entity + * (`a.*`), the trip-scoping guard every write path (REST update/delete, MCP, + * RPC) re-reads through. + */ +export interface DayAccommodationRow { + id: number; + trip_id: number; + place_id: number | null; + start_day_id: number; + end_day_id: number; + check_in: string | null; + check_in_end: string | null; + check_out: string | null; + confirmation: string | null; + notes: string | null; + created_at: string | null; +} + +const _dayAccommodationRowKeys: AssertRowKeys = true; + +/** AC2's projection: {@link DayAccommodationRow} plus the linked place's display fields. */ +export interface DayAccommodationWithPlaceRow extends DayAccommodationRow { + place_name: string | null; + place_address: string | null; + place_image: string | null; + place_lat: number | null; + place_lng: number | null; +} + +/** AC3's projection: {@link DayAccommodationWithPlaceRow} plus the linked booking's title — one row per linked `reservations` row (the LEFT JOIN fans out; parity keeps the duplicates, AC3's own ruling). */ +export interface DayAccommodationListRow extends DayAccommodationWithPlaceRow { + reservation_title: string | null; +} + +/** + * Kysely typing for AC3 (`listForTripWithPlaceAndBooking`) — `reservations + * .accommodation_id` is a bare `p.text().nullable()` column with no FK to + * `day_accommodations` (§18.1 of the inventory, the same gap + * `ReservationsRepository.joinedQuery` documents from the other side), so + * the QueryBuilder's relation-path `.join()` cannot express `LEFT JOIN + * reservations r ON r.accommodation_id = a.id` at all. + */ +interface DayAccommodationListKyselyDB { + day_accommodations: { + id: number; + // `number | string` — `ReservationsRepository.listForTrip`'s own + // `trip_id` widening: the raw-bind flexibility a `.where(col, '=', + // value)` call keeps regardless of how loosely its value is typed + // (program rule 23 bans a raw SQL-text condition, not a loosely-typed + // bound value). + trip_id: number | string; + place_id: number | null; + start_day_id: number; + end_day_id: number; + check_in: string | null; + check_in_end: string | null; + check_out: string | null; + confirmation: string | null; + notes: string | null; + created_at: string | null; + }; + places: { + id: number; + name: string; + address: string | null; + image_url: string | null; + lat: number | null; + lng: number | null; + }; + reservations: { + accommodation_id: string | null; + title: string; + }; +} + /** * Plan 3d Task 2 (`ReservationsService`'s `day_accommodations` writes — RS27, * RS29, RS30, RS37–RS40, RS42, RS43, RS46, RS47 — plus the RS21 dispatch @@ -146,4 +224,198 @@ export class DayAccommodationsRepository extends TrekRepository('get', false); return row?.trip_id; } + + // --------------------------------------------------------------------------- + // Plan 3d Task 3 (`AccommodationsService`, `days.service.ts` DY19/DY20/DY22, + // `places.service.ts` PL16) — appended after Task 2's own methods above, + // per this task's file-ownership rule ("the class is shared... Task 3 + // appends rather than replaces"). + // --------------------------------------------------------------------------- + + /** + * AC2 (`AccommodationsService.getAccommodationWithPlace`) — `SELECT a.*, + * p.name as place_name, p.address as place_address, p.image_url as + * place_image, p.lat as place_lat, p.lng as place_lng FROM + * day_accommodations a LEFT JOIN places p ON a.place_id = p.id WHERE a.id + * = ?`. The re-select every stay write answers with, post-commit. + */ + async findWithPlace(id: number): Promise { + // `{ 'a.id': id }`, not a bare `{ id }` (`DayAssignmentsRepository + // .findWithPlaceAndCategory`'s precedent): the LEFT JOIN to `places` + // means an unqualified `id` key is ambiguous between `a.id` and `p.id`. + return await this.qb('a') + .leftJoin('a.place', 'p') + .select(['a.*', 'p.name as place_name', 'p.address as place_address', 'p.image_url as place_image', 'p.lat as place_lat', 'p.lng as place_lng']) + .where({ 'a.id': id }) + .execute('get', false); + } + + /** + * AC3 (`AccommodationsService.listAccommodations`) — AC2's projection plus + * `r.title as reservation_title` via `LEFT JOIN reservations r ON + * r.accommodation_id = a.id`, `WHERE a.trip_id = ? ORDER BY a.created_at + * ASC`. Kysely (the reservations join, see {@link DayAccommodationListKyselyDB}). + * Fans out one row per linked booking — the legacy statement's own shape + * (the delete path documents more than one booking per stay is legal) — + * parity keeps the duplicates, no `DISTINCT`. + */ + async listForTripWithPlaceAndBooking(trip_id: number | string): Promise { + const rows = await this.kysely() + .selectFrom('day_accommodations as a') + .leftJoin('places as p', 'p.id', 'a.place_id') + .leftJoin('reservations as r', 'r.accommodation_id', 'a.id') + .selectAll('a') + .select([ + 'p.name as place_name', + 'p.address as place_address', + 'p.image_url as place_image', + 'p.lat as place_lat', + 'p.lng as place_lng', + 'r.title as reservation_title', + ]) + .where('a.trip_id', '=', trip_id) + .orderBy('a.created_at', 'asc') + .execute(); + return rows as DayAccommodationListRow[]; + } + + /** + * AC34 (`AccommodationsService.getAccommodation`) — `SELECT * FROM + * day_accommodations WHERE id = ? AND trip_id = ?`, the trip-scoping + * guard every write path (REST update/delete, MCP, RPC) re-reads through. + * `id`/`trip_id: number` (rule 21 — the SERVICE parses both ONCE via + * `toRowId` before calling this). + */ + async findInTrip(id: number, trip_id: number): Promise { + return await this.qb('a') + .select(['a.*']) + .where({ id, trip: trip_id }) + .execute('get', false); + } + + /** + * AC30 (`AccommodationsService.createAccommodation`) — `INSERT INTO + * day_accommodations (trip_id, place_id, start_day_id, end_day_id, + * check_in, check_in_end, check_out, confirmation, notes) VALUES (?×9)`. + * Nine columns — a DIFFERENT method from Task 2's own `insertBookingStay` + * (seven columns, no `check_in_end`/`notes`, D4). The caller passes + * already-decided values (the `|| null` coercions stay in the service); + * `em.insert()` returns the generated PK (R6's `lastInsertRowid` + * replacement). + */ + async insertStay(input: { + trip_id: number | string; + place_id: number | null; + start_day_id: number; + end_day_id: number; + check_in: string | null; + check_in_end: string | null; + check_out: string | null; + confirmation: string | null; + notes: string | null; + }): Promise { + return await this.insert({ + trip: input.trip_id, + place: input.place_id, + startDay: input.start_day_id, + endDay: input.end_day_id, + check_in: input.check_in, + check_in_end: input.check_in_end, + check_out: input.check_out, + confirmation: input.confirmation, + notes: input.notes, + }); + } + + /** + * AC36 (`AccommodationsService.updateAccommodation`) — `UPDATE + * day_accommodations SET place_id = ?, start_day_id = ?, end_day_id = ?, + * check_in = ?, check_in_end = ?, check_out = ?, confirmation = ?, notes = + * ? WHERE id = ?`. Eight columns, written verbatim — the caller's + * `fields.x !== undefined ? fields.x : existing.x` presence sentinel + * (note: NO `|| null` here, unlike AC30 — the legacy statement itself has + * none for these columns, so `''` is stored as `''`). + */ + async updateStay(id: number, write: { + place_id: number | null; + start_day_id: number; + end_day_id: number; + check_in: string | null; + check_in_end: string | null; + check_out: string | null; + confirmation: string | null; + notes: string | null; + }): Promise { + await this.nativeUpdate({ id }, { + place: write.place_id, + startDay: write.start_day_id, + endDay: write.end_day_id, + check_in: write.check_in, + check_in_end: write.check_in_end, + check_out: write.check_out, + confirmation: write.confirmation, + notes: write.notes, + }); + } + + /** + * AC44 (`AccommodationsService.deleteAccommodation`) — `DELETE FROM + * day_accommodations WHERE id = ?`. No trip scoping (the legacy statement + * has none either — the caller already scoped `id` via AC34's guard, + * rule 21). + */ + async deleteById(id: number): Promise { + await this.nativeDelete({ id }); + } + + /** + * DY19 (`DaysService.assertNoInvertedAccommodation`) — `SELECT a.id, + * s.day_number AS start_no, e.day_number AS end_no FROM day_accommodations + * a JOIN days s ON a.start_day_id = s.id JOIN days e ON a.end_day_id = + * e.id WHERE a.trip_id = ?`. Both `days` joins are declared relations + * (`startDay`/`endDay`), so the QueryBuilder expresses this directly. + */ + async listStartEndDayNumbers(trip_id: number): Promise<{ id: number; start_no: number; end_no: number }[]> { + return await this.qb('a') + .join('a.startDay', 's') + .join('a.endDay', 'e') + .select(['a.id', 's.day_number as start_no', 'e.day_number as end_no']) + .where({ trip: trip_id }) + .execute<{ id: number; start_no: number; end_no: number }[]>('all', false); + } + + /** + * DY20 (`DaysService.resyncAccommodationDays`) — `SELECT id, + * start_day_id, end_day_id FROM day_accommodations WHERE trip_id = ?`. + * `start_day_id`/`end_day_id` are `persist(false)` mirrors of the + * `startDay`/`endDay` relations — a bare select would drop them (the + * `RoadtripDayBoundariesRepository` trap, Task 1's report), so both go + * through `columnRef`. + */ + async listForResync(trip_id: number): Promise<{ id: number; start_day_id: number; end_day_id: number }[]> { + const platform = this.getEntityManager().getPlatform(); + return await this.qb('a') + .select(['a.id', columnRef(platform, 'a.start_day_id').as('start_day_id'), columnRef(platform, 'a.end_day_id').as('end_day_id')]) + .where({ trip: trip_id }) + .execute<{ id: number; start_day_id: number; end_day_id: number }[]>('all', false); + } + + /** DY22 (`DaysService.resyncAccommodationDays`) — `UPDATE day_accommodations SET start_day_id = ?, end_day_id = ? WHERE id = ?`. */ + async setDayRange(id: number, start_day_id: number, end_day_id: number): Promise { + await this.nativeUpdate({ id }, { startDay: start_day_id, endDay: end_day_id }); + } + + /** + * PL16 (`PlacesService.cancelStaysAt`) — `SELECT id FROM day_accommodations + * WHERE trip_id = ? AND place_id = ?`. Reached from `PlacesService` through + * `AccommodationsService` (already injected there for the cascade itself), + * not a new repository dependency of `PlacesService`'s own (R4 — keeps the + * trip scoping AC44's own unscoped delete relies on, inventory §14.3). + */ + async listForPlace(trip_id: number, place_id: number): Promise<{ id: number }[]> { + return await this.qb('a') + .select(['a.id']) + .where({ trip: trip_id, place: place_id }) + .execute<{ id: number }[]>('all', false); + } } diff --git a/server/src/db/repositories/DayAssignments.repository.ts b/server/src/db/repositories/DayAssignments.repository.ts index 06a7e852b..2fa346bb0 100644 --- a/server/src/db/repositories/DayAssignments.repository.ts +++ b/server/src/db/repositories/DayAssignments.repository.ts @@ -780,4 +780,157 @@ export class DayAssignmentsRepository extends TrekRepository { .execute(); return rows; } + + // --------------------------------------------------------------------------- + // Plan 3d Task 3 (`AccommodationsService`) — additive, per this task's own + // file-ownership rule ("additive methods on Days/Places/DayAssignments/ + // Reservations repositories where a read belongs there"). + // --------------------------------------------------------------------------- + + /** AC1 (`AccommodationsService.announceMirror`) — `SELECT id FROM day_assignments WHERE day_id = ? ORDER BY order_index`, once per touched day. */ + async listIdsForDay(day_id: number): Promise { + const rows = await this.qb('da') + .select(['da.id']) + .where({ day: day_id }) + .orderBy({ order_index: 'asc' }) + .execute<{ id: number }[]>('all', false); + return rows.map((r) => r.id); + } + + /** + * AC7 (`AccommodationsService.positionForCheckIn`) — `SELECT + * da.order_index, COALESCE(da.assignment_time, p.place_time, other.check_in) + * AS at FROM day_assignments da JOIN places p ON p.id = da.place_id LEFT + * JOIN day_accommodations other ON other.id = da.accommodation_id WHERE + * da.day_id = ? AND da.id != ? ORDER BY da.order_index`. Same Kysely + * escape hatch as {@link effectiveStart}/{@link listForTimeSort} (no ORM + * relation to `day_accommodations`). + */ + async listSeatTimes(day_id: number, exclude_id: number): Promise<{ order_index: number | null; at: string | null }[]> { + const rows = await this.kysely() + .selectFrom('day_assignments as da') + .innerJoin('places as p', 'p.id', 'da.place_id') + .leftJoin('day_accommodations as other', 'other.id', 'da.accommodation_id') + .select((eb) => ['da.order_index as order_index', eb.fn.coalesce('da.assignment_time', 'p.place_time', 'other.check_in').as('at')]) + .where('da.day_id', '=', day_id) + .where('da.id', '!=', exclude_id) + .orderBy('da.order_index', 'asc') + .execute(); + return rows as { order_index: number | null; at: string | null }[]; + } + + /** + * AC8 (`AccommodationsService.seatedByCheckIn`) — {@link listSeatTimes}'s + * same joins, projected to `da.id` instead of `order_index`, no exclusion: + * `SELECT da.id, COALESCE(da.assignment_time, p.place_time, other.check_in) + * AS at FROM day_assignments da JOIN places p ON p.id = da.place_id LEFT + * JOIN day_accommodations other ON other.id = da.accommodation_id WHERE + * da.day_id = ? ORDER BY da.order_index`. + */ + async listSeatTimesWithIds(day_id: number): Promise<{ id: number; at: string | null }[]> { + const rows = await this.kysely() + .selectFrom('day_assignments as da') + .innerJoin('places as p', 'p.id', 'da.place_id') + .leftJoin('day_accommodations as other', 'other.id', 'da.accommodation_id') + .select((eb) => ['da.id as id', eb.fn.coalesce('da.assignment_time', 'p.place_time', 'other.check_in').as('at')]) + .where('da.day_id', '=', day_id) + .orderBy('da.order_index', 'asc') + .execute(); + return rows as { id: number; at: string | null }[]; + } + + /** + * AC12 (`AccommodationsService.locatedStopIds`) — `SELECT da.id FROM + * day_assignments da JOIN places p ON p.id = da.place_id WHERE da.day_id = + * ? AND p.lat IS NOT NULL AND p.lng IS NOT NULL ORDER BY da.order_index + * ASC, da.created_at ASC, da.id ASC`. `$ne: null` (a typed operator, rule + * 23) for the `IS NOT NULL` pair. + */ + async listLocatedIds(day_id: number): Promise { + const rows = await this.qb('da') + .join('da.place', 'p') + .select(['da.id']) + .where({ day: day_id, 'p.lat': { $ne: null }, 'p.lng': { $ne: null } }) + .orderBy({ 'da.order_index': 'asc', 'da.created_at': 'asc', 'da.id': 'asc' }) + .execute<{ id: number }[]>('all', false); + return rows.map((r) => r.id); + } + + /** + * AC18/AC24 (`AccommodationsService.relocateOwnStop`/`mirrorStay`) — one + * method, two call shapes (D4): `SELECT id FROM day_assignments WHERE + * day_id = ? AND place_id = ? AND id != ?` (AC18, with `exclude_id`) and + * `SELECT id FROM day_assignments WHERE day_id = ? AND place_id = ?` + * (AC24, without). + */ + async existsForDayAndPlace(day_id: number, place_id: number, exclude_id?: number): Promise { + const query = this.qb('da').select(['da.id']).where({ day: day_id, place: place_id }); + if (exclude_id !== undefined) query.andWhere({ id: { $ne: exclude_id } }); + const row = await query.execute<{ id: number } | undefined>('get', false); + return !!row; + } + + /** + * AC19 (`AccommodationsService.relocateOwnStop`) — `UPDATE day_assignments + * SET order_index = order_index - 1 WHERE day_id = ? AND order_index > ?`, + * a column-from-column update (`columnIncrementedBy`, {@link shiftOrderFrom}'s + * precedent). + */ + async closeGap(day_id: number, from_index: number): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.qb() + .update({ order_index: columnIncrementedBy(platform, 'order_index', -1) }) + .where({ day: day_id, order_index: { $gt: from_index } }) + .execute('run'); + } + + /** + * AC20 (`AccommodationsService.relocateOwnStop`) — `SELECT MAX(order_index) + * AS max FROM day_assignments WHERE day_id = ? AND id != ?`, read the same + * way {@link maxOrderIndex} is (an ordered `limit(1)`, null NOT pre-folded — + * the caller's own `max.max !== null ? max.max : -1` check needs to tell + * "no rows" from a stored `0`). + */ + async maxOrderIndexExcluding(day_id: number, exclude_id: number): Promise { + const row = await this.qb('da') + .select('da.order_index') + .where({ day: day_id, id: { $ne: exclude_id } }) + .orderBy({ order_index: 'desc' }) + .limit(1) + .execute<{ order_index: number | null } | undefined>('get', false); + return row?.order_index ?? null; + } + + /** AC21 (`AccommodationsService.relocateOwnStop`) — `UPDATE day_assignments SET day_id = ?, place_id = ?, order_index = ? WHERE id = ?`. */ + async relocate(id: number, day_id: number, place_id: number, order_index: number): Promise { + await this.nativeUpdate({ id }, { day: day_id, place: place_id, order_index }); + } + + /** + * AC22 (`AccommodationsService.relocateOwnStop`) — `UPDATE day_assignments + * SET order_index = order_index + 1 WHERE day_id = ? AND order_index >= ? + * AND id != ?`, the same column-from-column shape as {@link shiftOrderFrom} + * with an extra exclusion. + */ + async shiftFromExcluding(day_id: number, from_index: number, exclude_id: number): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.qb() + .update({ order_index: columnIncrementedBy(platform, 'order_index', 1) }) + .where({ day: day_id, order_index: { $gte: from_index }, id: { $ne: exclude_id } }) + .execute('run'); + } + + /** AC25 (`AccommodationsService.ownStops`) — `SELECT id, day_id, place_id, order_index FROM day_assignments WHERE accommodation_id = ?`. `day_id`/`place_id` are `persist(false)` relation mirrors — `columnRef`, the same trap {@link getDayId} documents. */ + async listOwnedByStay(accommodation_id: number): Promise<{ id: number; day_id: number; place_id: number; order_index: number | null }[]> { + const platform = this.getEntityManager().getPlatform(); + return await this.qb('da') + .select(['da.id', columnRef(platform, 'da.day_id').as('day_id'), columnRef(platform, 'da.place_id').as('place_id'), 'da.order_index']) + .where({ accommodation_id }) + .execute<{ id: number; day_id: number; place_id: number; order_index: number | null }[]>('all', false); + } + + /** AC26 (`AccommodationsService.releaseStops`, `keepStop` branch) — `UPDATE day_assignments SET accommodation_id = NULL WHERE id = ?`. */ + async clearStay(id: number): Promise { + await this.nativeUpdate({ id }, { accommodation_id: null }); + } } diff --git a/server/src/db/repositories/Places.repository.ts b/server/src/db/repositories/Places.repository.ts index f480d4b00..e36ee90da 100644 --- a/server/src/db/repositories/Places.repository.ts +++ b/server/src/db/repositories/Places.repository.ts @@ -865,4 +865,31 @@ export class PlacesRepository extends TrekRepository { .execute<{ name: string; lat: number | null; lng: number | null; stop_type: string | null } | undefined>('get', false); return row ?? undefined; } + + // --------------------------------------------------------------------------- + // Plan 3d Task 3 (`AccommodationsService`) — additive, per this task's own + // file-ownership rule ("additive methods on Days/Places/DayAssignments/ + // Reservations repositories where a read belongs there"). + // --------------------------------------------------------------------------- + + /** AC9 (`AccommodationsService.stampLodging`) — `SELECT stop_type FROM places WHERE id = ?`. */ + async getStopType(id: number): Promise<{ stop_type: string | null } | undefined> { + return await this.qb('p') + .select(['p.stop_type']) + .where({ id }) + .execute<{ stop_type: string | null } | undefined>('get', false); + } + + /** AC10 (`AccommodationsService.stampLodging`) — `UPDATE places SET stop_type = 'hotel' WHERE id = ?`. Does NOT stamp `places.updated_at` (matches the legacy statement, R7 class — flagged, not fixed). */ + async stampHotel(id: number): Promise { + await this.nativeUpdate({ id }, { stop_type: 'hotel' }); + } + + /** AC31 (`AccommodationsService.createAccommodation`) — `SELECT name FROM places WHERE id = ?`, the auto-created partner booking's title source. */ + async getName(id: number): Promise<{ name: string } | undefined> { + return await this.qb('p') + .select(['p.name']) + .where({ id }) + .execute<{ name: string } | undefined>('get', false); + } } diff --git a/server/src/db/repositories/Reservations.repository.ts b/server/src/db/repositories/Reservations.repository.ts index 65117e372..638943999 100644 --- a/server/src/db/repositories/Reservations.repository.ts +++ b/server/src/db/repositories/Reservations.repository.ts @@ -1,7 +1,7 @@ import type { Reservations } from '../entities/Reservations.entity'; import { DayAssignments } from '../entities/DayAssignments.entity'; import { Days } from '../entities/Days.entity'; -import { columnRef, concatKysely, dayDistance, substringKysely } from '../dialect/sql-functions'; +import { castIntegerKysely, coalesceParam, columnRef, concatKysely, dayDistance, substringKysely } from '../dialect/sql-functions'; import { TrekRepository } from './_shared/trek-repository'; /** @@ -119,6 +119,42 @@ interface ReservationRestampKyselyDB { }; } +/** + * Kysely typing for AC37/AC40 (`listIdMetadataByStay`/`listIdsByStay`). + * + * A bound-parameter `WHERE accommodation_id = ?` on this TEXT column is NOT + * the same comparison the legacy statement's own `Number(id)` bind produced + * (Task 3 finding, verified empirically against the live driver, not + * assumed from the inventory's §18.1 wording): `better-sqlite3` binds every + * plain JS number as SQLite REAL, integer-valued or not (`typeof(?)` on a + * bound `14` reads back `'real'`) — including through MikroORM's Kysely + * dialect — and SQLite's TEXT-affinity conversion of a REAL for a + * comparison renders its decimal form (`14` → `'14.0'`), not the plain + * integer text `'14'`. A row this repository itself writes through + * `em.insert()`/`nativeUpdate()` stores the OTHER shape (`'14'`, no `.0` — + * MikroORM inlines the value as a literal in the generated SQL, rule 22, + * which SQLite's parser treats as a genuine INTEGER token, not a + * REAL-bound parameter) — so a plain `WHERE accommodation_id = ?` bound + * with a number would consistently MISS every row this cluster's own + * `em.insert()` calls write, not just the documented `"14.0"` edge case. + * {@link castIntegerKysely} (SQLF-057, `sql-functions.test.ts`) sidesteps + * the whole shape question: `CAST(accommodation_id AS INTEGER) = ?` matches + * `'14'` AND `'14.0'` alike (SQLite compares the CAST result against the + * bound REAL/INTEGER numerically, not as text), which is what "every + * linked booking" (AC37/AC40's own contract — more than one reservation, + * written by different call sites with different shapes, can point at one + * stay) actually needs, and is R2's own "tested with both `'14'` and + * `'14.0'` rows" requirement satisfied structurally rather than by picking + * a side. + */ +interface ReservationsByAccommodationKyselyDB { + reservations: { + id: number; + accommodation_id: string | null; + metadata: string | null; + }; +} + export class ReservationsRepository extends TrekRepository { private joinedQuery() { return this.kysely() @@ -424,6 +460,101 @@ export class ReservationsRepository extends TrekRepository { .execute<{ id: number } | undefined>('get', false); return row?.id; } + + // --------------------------------------------------------------------------- + // Plan 3d Task 3 (`AccommodationsService`) — additive, per this task's own + // file-ownership rule ("additive methods on Days/Places/DayAssignments/ + // Reservations repositories where a read belongs there"). + // --------------------------------------------------------------------------- + + /** + * AC33 (`AccommodationsService.createAccommodation`) — `INSERT INTO + * reservations (trip_id, day_id, title, reservation_time, location, + * confirmation_number, notes, status, type, accommodation_id, metadata) + * VALUES (?, ?, ?, ?, ?, ?, ?, 'confirmed', 'hotel', ?, ?)` — the + * auto-created partner hotel booking. `status`/`type` literals, `location` + * always `null` (matching the legacy statement exactly — it never binds a + * value there). `accommodation_id: string` — R2: the caller stringifies + * the new stay's numeric id (`String(newId)`) the same way `insertReservation`'s + * own `accommodation_id` caller already does, so SQLite's TEXT-affinity + * storage is identical to the legacy raw bind of the bare number. + */ + async insertHotelPartner(input: { + trip_id: number | string; + day_id: number; + title: string; + reservation_time: string | null; + confirmation_number: string | null; + notes: string | null; + accommodation_id: string; + metadata: string | null; + }): Promise { + return await this.insert({ + trip: input.trip_id, + day: input.day_id, + title: input.title, + reservation_time: input.reservation_time, + location: null, + confirmation_number: input.confirmation_number, + notes: input.notes, + status: 'confirmed', + type: 'hotel', + accommodation_id: input.accommodation_id, + metadata: input.metadata, + }); + } + + /** + * AC37 (`AccommodationsService.updateAccommodation`) — `SELECT id, + * metadata FROM reservations WHERE accommodation_id = ?`, every linked + * booking (no unique constraint on the column — more than one booking can + * point at the same stay, and different write paths can leave it in + * either TEXT shape). `castIntegerKysely` (see + * {@link ReservationsByAccommodationKyselyDB}'s docstring for why). + */ + async listIdMetadataByStay(accommodation_id: number): Promise<{ id: number; metadata: string | null }[]> { + const platform = this.getEntityManager().getPlatform(); + const rows = await this.kysely() + .selectFrom('reservations') + .select(['id', 'metadata']) + .where((eb) => eb(castIntegerKysely(platform, eb, 'accommodation_id'), '=', accommodation_id)) + .execute(); + return rows as { id: number; metadata: string | null }[]; + } + + /** + * AC38 (`AccommodationsService.updateAccommodation`) — `UPDATE + * reservations SET metadata = ?, confirmation_number = COALESCE(?, + * confirmation_number) WHERE id = ?`. Runs AFTER `updateAccommodation`'s + * own transaction commits (§18.6 — R5 class, flagged not fixed). + * `coalesceParam` for the value-side COALESCE (`DayAccommodationsRepository + * .patchConfirmation`'s precedent). + */ + async setMetadataAndConfirmation(id: number, metadata: string, confirmation: string | null): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.qb() + .update({ + metadata, + confirmation_number: coalesceParam(platform, 'confirmation_number', confirmation), + }) + .where({ id }) + .execute('run'); + } + + /** + * AC40 (`AccommodationsService.deleteAccommodation`) — `SELECT id FROM + * reservations WHERE accommodation_id = ?`, ALL linked bookings. Same + * `castIntegerKysely` reasoning as {@link listIdMetadataByStay}. + */ + async listIdsByStay(accommodation_id: number): Promise<{ id: number }[]> { + const platform = this.getEntityManager().getPlatform(); + const rows = await this.kysely() + .selectFrom('reservations') + .select(['id']) + .where((eb) => eb(castIntegerKysely(platform, eb, 'accommodation_id'), '=', accommodation_id)) + .execute(); + return rows as { id: number }[]; + } } /** `SELECT *` — every scalar column of `Reservations`, RS35's shape. */ diff --git a/server/src/nest/accommodations/accommodations-domain.module.ts b/server/src/nest/accommodations/accommodations-domain.module.ts index e7e98a7a7..becc324db 100644 --- a/server/src/nest/accommodations/accommodations-domain.module.ts +++ b/server/src/nest/accommodations/accommodations-domain.module.ts @@ -1,8 +1,15 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { AccommodationsService } from './accommodations.service'; import { AssignmentsDomainModule } from '../assignments/assignments-domain.module'; import { PermissionsModule } from '../permissions/permissions.module'; import { RealtimeModule } from '../realtime/realtime.module'; +import { DayAccommodations } from '../../db/entities/DayAccommodations.entity'; +import { DayAssignments } from '../../db/entities/DayAssignments.entity'; +import { Places } from '../../db/entities/Places.entity'; +import { Days } from '../../db/entities/Days.entity'; +import { RoadtripVias } from '../../db/entities/RoadtripVias.entity'; +import { Reservations } from '../../db/entities/Reservations.entity'; /** * The accommodations SERVICE, split from the controller/MCP/RPC surfaces (the @@ -15,9 +22,24 @@ import { RealtimeModule } from '../realtime/realtime.module'; * nights booked at it with it, and the cascade behind a stay (its partner * booking, that booking's expense, the day stop it wrote) belongs in one place * rather than copied into the delete path. + * + * `MikroOrmModule.forFeature([DayAccommodations, DayAssignments, Places, + * Days, RoadtripVias, Reservations])` registers `DayAccommodationsRepository` + * (this task's own) plus `DayAssignmentsRepository`/`PlacesRepository`/ + * `DaysRepository`/`RoadtripViasRepository`/`ReservationsRepository` (owned + * elsewhere, reached the same way `ReservationsModule` reaches `Days`/ + * `Places`/`DayAssignments` — the entity classes only, never the owning + * module) for `AccommodationsService`'s `@InjectRepository` constructor + * params (Plan 3d Task 3). `PlacesService.cancelStaysAt` (PL16) reaches + * `DayAccommodationsRepository` through `AccommodationsService` itself + * (already injected there) instead of a repository dependency of its own — + * no entity added here for that. */ @Module({ - imports: [PermissionsModule, RealtimeModule, AssignmentsDomainModule], + imports: [ + PermissionsModule, RealtimeModule, AssignmentsDomainModule, + MikroOrmModule.forFeature([DayAccommodations, DayAssignments, Places, Days, RoadtripVias, Reservations]), + ], providers: [AccommodationsService], exports: [AccommodationsService], }) diff --git a/server/src/nest/accommodations/accommodations.mcp.ts b/server/src/nest/accommodations/accommodations.mcp.ts index e626685ed..f7d068b96 100644 --- a/server/src/nest/accommodations/accommodations.mcp.ts +++ b/server/src/nest/accommodations/accommodations.mcp.ts @@ -8,7 +8,6 @@ import { z } from 'zod'; import { AuthService } from '../auth/auth.service'; import { PlacesService } from '../places/places.service'; import { noAccess, permissionDenied } from '../../mcp/tools/_shared'; -import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; import { AccommodationsService, type MirrorSender } from './accommodations.service'; @@ -23,15 +22,22 @@ function parseId(value: string | string[]): number | null { * DaysMcp when accommodations became their own domain; names, descriptions, * schemas, annotations, error shapes and broadcasts are unchanged. * - * It keeps DatabaseService and PlacesService because create_place_accommodation - * creates the place and the stay in one transaction. That is also what let - * DaysMcp drop both of them, and DaysModule its PlacesModule import. + * It keeps PlacesService because create_place_accommodation creates the place + * and the stay in one transaction. That is also what let DaysMcp drop it, and + * DaysModule its PlacesModule import. + * + * R12 (Plan 3d Task 3): the `DatabaseService` this class used to inject was + * unused (§18.15 of the inventory) — dropped. The shared positional + * test-helper (`tests/helpers/mcp-test-controllers.ts:303`) that builds this + * class by constructor position is a concurrent implementer's file (Task 5) + * and was not touched here (the task brief's own hard rule); it needs a + * one-line follow-up dropping the `dbService` argument from its + * `new AccommodationsMcp(...)` call. */ @McpController() export class AccommodationsMcp { constructor( private readonly accommodations: AccommodationsService, - private readonly db: DatabaseService, private readonly places: PlacesService, private readonly auth: AuthService, private readonly guards: McpToolGuardsService, diff --git a/server/src/nest/accommodations/accommodations.service.ts b/server/src/nest/accommodations/accommodations.service.ts index bc6481a72..171554347 100644 --- a/server/src/nest/accommodations/accommodations.service.ts +++ b/server/src/nest/accommodations/accommodations.service.ts @@ -1,10 +1,24 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import type { RoadtripVia, TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { DatabaseService, type PlaceWithTags, type TripAccess } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; import { PermissionsService } from '../permissions/permissions.service'; import { AssignmentsService } from '../assignments/assignments.service'; +import { toRowId } from '../common/row-id'; +import { DayAccommodations } from '../../db/entities/DayAccommodations.entity'; +import type { DayAccommodationsRepository, DayAccommodationRow } from '../../db/repositories/DayAccommodations.repository'; +import { DayAssignments } from '../../db/entities/DayAssignments.entity'; +import type { DayAssignmentsRepository } from '../../db/repositories/DayAssignments.repository'; +import { Places } from '../../db/entities/Places.entity'; +import type { PlacesRepository } from '../../db/repositories/Places.repository'; +import { Days } from '../../db/entities/Days.entity'; +import type { DaysRepository } from '../../db/repositories/Days.repository'; +import { RoadtripVias } from '../../db/entities/RoadtripVias.entity'; +import type { RoadtripViasRepository } from '../../db/repositories/RoadtripVias.repository'; +import { Reservations } from '../../db/entities/Reservations.entity'; +import type { ReservationsRepository } from '../../db/repositories/Reservations.repository'; import type { User } from '../../types'; type Trip = TripAccess; @@ -40,6 +54,32 @@ export interface AccommodationMirror { /** A write that left the day plan alone. Exported for the surfaces that write a * stay row themselves and have to answer with a mirror either way. */ export const noStayMirror = (): AccommodationMirror => ({ created: null, moved: null, updated: [], removed: [], stamped: null }); + +/** + * AC33's `accommodation_id` value, in the exact TEXT shape the TRUE legacy + * (pre-ORM) statement stored (Task 3 finding, verified empirically against + * the live `better-sqlite3` driver, not assumed): the legacy `INSERT INTO + * reservations (…, accommodation_id, …) VALUES (…, ?, …)` bound the new + * stay's id — a plain JS `number` from `result.lastInsertRowid` — as a + * `?` parameter, and `better-sqlite3` binds every plain JS number as SQLite + * REAL regardless of integer-ness (confirmed via `typeof(?)`); SQLite's own + * "numeric data inserted into a TEXT column is converted to text first" + * rule then renders a REAL's decimal form, `14` → `'14.0'`, not the plain + * integer text `'14'`. `accommodations.service.test.ts`'s DAY-SVC-020/023/ + * 025 etc. read this column back with their OWN plain-number-bound raw SQL + * (`testDb.prepare('… WHERE accommodation_id = ?').get(accom.id)`, matching + * the same REAL-binding path), so this write must reproduce that exact + * shape for those UNCHANGED "proof" tests to keep passing — `String(id)` + * (`'14'`, what `MikroORM.em.insert()` would store for either a number or a + * string, since it inlines the value as a SQL-text literal, rule 22, a + * DIFFERENT shape) does not. `ReservationsRepository`'s own reads of this + * column (`listIdMetadataByStay`/`listIdsByStay`) go through + * `castIntegerKysely` instead, which matches this shape AND `'14'` alike — + * see that file's docstring. + */ +function legacyBoundIntegerText(id: number): string { + return `${id}.0`; +} const noMirror = noStayMirror; export interface DayAccommodation { @@ -97,6 +137,12 @@ export class AccommodationsService { private readonly realtime: RealtimeService, private readonly assignments: AssignmentsService, private readonly uow: UnitOfWork, + @InjectRepository(DayAccommodations) private readonly dayAccommodationsRepo: DayAccommodationsRepository, + @InjectRepository(DayAssignments) private readonly dayAssignmentsRepo: DayAssignmentsRepository, + @InjectRepository(Places) private readonly placesRepo: PlacesRepository, + @InjectRepository(Days) private readonly daysRepo: DaysRepository, + @InjectRepository(RoadtripVias) private readonly roadtripViasRepo: RoadtripViasRepository, + @InjectRepository(Reservations) private readonly reservationsRepo: ReservationsRepository, ) {} private get db() { @@ -183,6 +229,19 @@ export class AccommodationsService { return this.releaseStops(accommodationId, {}); } + /** + * PL16 (`PlacesService.cancelStaysAt`) — the stays booked at a place, by + * id. `PlacesService` reaches this repository read through the + * `AccommodationsService` it already injects (for the cascade itself) + * rather than a `DayAccommodationsRepository` dependency of its own — + * avoids widening `PlacesService`'s constructor (and the shared positional + * test-helper wiring several other suites depend on) for one read. + */ + async listStayIdsForPlace(tripId: number, placeId: number): Promise { + const stays = await this.dayAccommodationsRepo.listForPlace(tripId, placeId); + return stays.map(s => s.id); + } + /** * Send what a stay write did to the day plan, and let the journey skeletons * catch up the way an assignment route does. @@ -210,8 +269,8 @@ export class AccommodationsService { // created/moved event alone puts the row at the end of the day on every other // screen, so the day that changed sends its order along. for (const dayId of await this.touchedDays(mirror)) { - const orderedIds = this.db.all<{ id: number }>( - 'SELECT id FROM day_assignments WHERE day_id = ? ORDER BY order_index', dayId).map(row => row.id); + // AC1 + const orderedIds = await this.dayAssignmentsRepo.listIdsForDay(dayId); send('assignment:reordered', { dayId, orderedIds }); } // After the order, the way the time sort sends them: the planner routes the @@ -235,39 +294,31 @@ export class AccommodationsService { // ------------------------------------------------------------------------- - private async getAccommodationWithPlace(id: number | bigint) { - return this.db.get(` - SELECT a.*, p.name as place_name, p.address as place_address, p.image_url as place_image, p.lat as place_lat, p.lng as place_lng - FROM day_accommodations a - LEFT JOIN places p ON a.place_id = p.id - WHERE a.id = ? - `, id); + private async getAccommodationWithPlace(id: number) { + // AC2 + return await this.dayAccommodationsRepo.findWithPlace(id); } async listAccommodations(tripId: string | number) { - return this.db.all(` - SELECT a.*, p.name as place_name, p.address as place_address, p.image_url as place_image, p.lat as place_lat, p.lng as place_lng, - r.title as reservation_title - FROM day_accommodations a - LEFT JOIN places p ON a.place_id = p.id - LEFT JOIN reservations r ON r.accommodation_id = a.id - WHERE a.trip_id = ? - ORDER BY a.created_at ASC - `, tripId); + // AC3 + return await this.dayAccommodationsRepo.listForTripWithPlaceAndBooking(tripId); } async validateAccommodationRefs(tripId: string | number, placeId?: number, startDayId?: number, endDayId?: number) { const errors: { field: string; message: string }[] = []; if (placeId !== undefined) { - const place = this.db.get('SELECT id FROM places WHERE id = ? AND trip_id = ?', placeId, tripId); + // AC4 + const place = await this.placesRepo.existsInTrip(placeId, Number(tripId)); if (!place) errors.push({ field: 'place_id', message: 'Place not found' }); } if (startDayId !== undefined) { - const startDay = this.db.get('SELECT id FROM days WHERE id = ? AND trip_id = ?', startDayId, tripId); + // AC5 + const startDay = await this.daysRepo.existsInTrip(startDayId, tripId); if (!startDay) errors.push({ field: 'start_day_id', message: 'Start day not found' }); } if (endDayId !== undefined) { - const endDay = this.db.get('SELECT id FROM days WHERE id = ? AND trip_id = ?', endDayId, tripId); + // AC6 + const endDay = await this.daysRepo.existsInTrip(endDayId, tripId); if (!endDay) errors.push({ field: 'end_day_id', message: 'End day not found' }); } return errors; @@ -300,14 +351,15 @@ export class AccommodationsService { // against. Without it a night parked at the end of the day can find itself. // Another booked night on the day counts by its check-in, so two bookings on // one day settle by the clock. - const rows = this.db.all<{ order_index: number; at: string | null }>(` - SELECT da.order_index, COALESCE(da.assignment_time, p.place_time, other.check_in) AS at - FROM day_assignments da JOIN places p ON p.id = da.place_id - LEFT JOIN day_accommodations other ON other.id = da.accommodation_id - WHERE da.day_id = ? AND da.id != ? ORDER BY da.order_index - `, dayId, excludeId ?? -1); + // AC7 + const rows = await this.dayAssignmentsRepo.listSeatTimes(dayId, excludeId ?? -1); let seat = 0; - for (const row of rows) if (row.at !== null && row.at <= checkIn) seat = row.order_index + 1; + // `Number(row.order_index) + 1`, not `row.order_index + 1`: the legacy row + // type claimed `order_index: number` (non-null) although the column is + // nullable — a stored NULL there added to 1 in JS coerces to 1 + // (`null + 1 === 1`), and `Number(null)` reproduces that same coercion + // for the repository's honestly-nullable projection. + for (const row of rows) if (row.at !== null && row.at <= checkIn) seat = Number(row.order_index) + 1; return seat; } @@ -323,12 +375,8 @@ export class AccommodationsService { */ private async seatedByCheckIn(dayId: number, ownId: number, checkIn: string | null | undefined): Promise { if (!checkIn) return true; - const rows = this.db.all<{ id: number; at: string | null }>(` - SELECT da.id, COALESCE(da.assignment_time, p.place_time, other.check_in) AS at - FROM day_assignments da JOIN places p ON p.id = da.place_id - LEFT JOIN day_accommodations other ON other.id = da.accommodation_id - WHERE da.day_id = ? ORDER BY da.order_index - `, dayId); + // AC8 + const rows = await this.dayAssignmentsRepo.listSeatTimesWithIds(dayId); const own = rows.findIndex(row => row.id === ownId); if (own < 0) return true; const laterAhead = rows.slice(0, own).some(row => row.at !== null && row.at > checkIn); @@ -346,9 +394,12 @@ export class AccommodationsService { * (a campsite, say) is theirs. */ private async stampLodging(placeId: number): Promise { - const place = this.db.get<{ stop_type: string | null }>('SELECT stop_type FROM places WHERE id = ?', placeId); + // AC9 + const place = await this.placesRepo.getStopType(placeId); if (!place || place.stop_type) return null; - this.db.run("UPDATE places SET stop_type = 'hotel' WHERE id = ?", placeId); + // AC10 + await this.placesRepo.stampHotel(placeId); + // AC11 — primitive, already-converted (`DatabaseService.getPlaceWithTags` → `PlacesRepository.findWithTagsAndRatings`). return await this.db.getPlaceWithTags(placeId); } @@ -368,11 +419,8 @@ export class AccommodationsService { } private async locatedStopIds(dayId: number): Promise { - return this.db.all<{ id: number }>(` - SELECT da.id FROM day_assignments da JOIN places p ON p.id = da.place_id - WHERE da.day_id = ? AND p.lat IS NOT NULL AND p.lng IS NOT NULL - ORDER BY da.order_index ASC, da.created_at ASC, da.id ASC - `, dayId).map(row => row.id); + // AC12 + return await this.dayAssignmentsRepo.listLocatedIds(dayId); } /** @@ -393,7 +441,8 @@ export class AccommodationsService { for (const [dayId, previousIds] of before) { const nextIds = await this.locatedStopIds(dayId); if (previousIds.length === nextIds.length && previousIds.every((id, i) => id === nextIds[i])) continue; - const vias = this.db.all('SELECT id, after_order_index, sequence FROM roadtrip_vias WHERE day_id = ?', dayId); + // AC13 + const vias: PinnedVia[] = await this.roadtripViasRepo.listAnchors(dayId); if (!vias.length) continue; // A via behind the day's last stop bends the drive into the next day. It stays @@ -413,14 +462,13 @@ export class AccommodationsService { } if (!remove.length && !moved.length) continue; - for (const viaId of remove) this.db.run('DELETE FROM roadtrip_vias WHERE id = ? AND day_id = ?', viaId, dayId); - for (const via of moved) this.db.run('UPDATE roadtrip_vias SET after_order_index = ? WHERE id = ? AND day_id = ?', via.after_order_index, via.id, dayId); + // AC14 + for (const viaId of remove) await this.roadtripViasRepo.deleteInDay(viaId, dayId); + // AC15 + for (const via of moved) await this.roadtripViasRepo.setAnchor(via.id, dayId, via.after_order_index); await this.renumberMergedLegs(dayId, vias.filter(via => !remove.includes(via.id)), moved); - this.noteVias(mirror, { - dayId, - vias: this.db.all( - 'SELECT id, day_id, after_order_index, sequence, lat, lng, created_at FROM roadtrip_vias WHERE day_id = ? ORDER BY after_order_index, sequence, id', dayId), - }); + // AC16 + this.noteVias(mirror, { dayId, vias: await this.roadtripViasRepo.listForDay(dayId) }); } } @@ -459,9 +507,11 @@ export class AccommodationsService { for (const onLeg of byLeg.values()) { if (!onLeg.some(via => landed.has(via.id))) continue; onLeg.sort((a, b) => a.after_order_index - b.after_order_index || a.sequence - b.sequence || a.id - b.id); - onLeg.forEach((via, index) => { - if (via.sequence !== index) this.db.run('UPDATE roadtrip_vias SET sequence = ? WHERE id = ? AND day_id = ?', index, via.id, dayId); - }); + // AC17 — only rows whose sequence changed (the caller's own guard; the + // statement itself is `setSequence`, RoadtripViasRepository's RT22). + for (const [index, via] of onLeg.entries()) { + if (via.sequence !== index) await this.roadtripViasRepo.setSequence(via.id, dayId, index); + } } } @@ -486,37 +536,38 @@ export class AccommodationsService { * Runs inside the caller's transaction. */ private async relocateOwnStop( - stop: { id: number; day_id: number; order_index: number }, + stop: { id: number; day_id: number; order_index: number | null }, placeId: number, dayId: number, checkIn?: string | null, ): Promise { - if (this.db.get('SELECT id FROM day_assignments WHERE day_id = ? AND place_id = ? AND id != ?', dayId, placeId, stop.id)) { + // AC18 + if (await this.dayAssignmentsRepo.existsForDayAndPlace(dayId, placeId, stop.id)) { return null; } // Close the gap the row leaves behind. A no-op when it is not changing days, // because it is put back into that same numbering two statements down. - this.db.run( - 'UPDATE day_assignments SET order_index = order_index - 1 WHERE day_id = ? AND order_index > ?', - stop.day_id, stop.order_index, - ); + // `Number(stop.order_index)` — see `positionForCheckIn`'s docstring for + // why this reproduces the legacy row type's implicit non-null coercion. + // AC19 + await this.dayAssignmentsRepo.closeGap(stop.day_id, Number(stop.order_index)); // Park it at the end of the target day first, then seat it by the check-in the // same way a fresh insert would. Two steps, because the index it should get is // read off the chain it is not part of yet. - const max = this.db.get<{ max: number | null }>( - 'SELECT MAX(order_index) AS max FROM day_assignments WHERE day_id = ? AND id != ?', dayId, stop.id)!; - const end = (max.max !== null ? max.max : -1) + 1; - this.db.run('UPDATE day_assignments SET day_id = ?, place_id = ?, order_index = ? WHERE id = ?', dayId, placeId, end, stop.id); + // AC20 + const max = await this.dayAssignmentsRepo.maxOrderIndexExcluding(dayId, stop.id); + const end = (max !== null ? max : -1) + 1; + // AC21 + await this.dayAssignmentsRepo.relocate(stop.id, dayId, placeId, end); const seat = await this.positionForCheckIn(dayId, checkIn, stop.id); if (seat < end) { - this.db.run( - 'UPDATE day_assignments SET order_index = order_index + 1 WHERE day_id = ? AND order_index >= ? AND id != ?', - dayId, seat, stop.id, - ); - this.db.run('UPDATE day_assignments SET order_index = ? WHERE id = ?', seat, stop.id); + // AC22 + await this.dayAssignmentsRepo.shiftFromExcluding(dayId, seat, stop.id); + // AC23 — `setOrderIndex` (exists, AS11/AS19's shared method). + await this.dayAssignmentsRepo.setOrderIndex(stop.id, undefined, seat); } return this.assignments.getAssignmentWithPlace(stop.id); @@ -534,7 +585,8 @@ export class AccommodationsService { // Claiming that row would make cancelling the booking delete a stop the traveller // placed, so the booking rides along with it and marks nothing as its own. Same // answer for a place already planned for that day by hand. - if (this.db.get('SELECT id FROM day_assignments WHERE day_id = ? AND place_id = ?', dayId, placeId)) return mirror; + // AC24 + if (await this.dayAssignmentsRepo.existsForDayAndPlace(dayId, placeId)) return mirror; const before = await this.stopOrders([dayId]); // Through AssignmentsService, seated where the check-in says (see @@ -555,9 +607,8 @@ export class AccommodationsService { /** The day stops this booking, and only this booking, put on the plan. */ private async ownStops(accommodationId: number) { - return this.db.all<{ id: number; day_id: number; place_id: number; order_index: number }>( - 'SELECT id, day_id, place_id, order_index FROM day_assignments WHERE accommodation_id = ?', accommodationId - ); + // AC25 + return await this.dayAssignmentsRepo.listOwnedByStay(accommodationId); } /** @@ -580,7 +631,8 @@ export class AccommodationsService { const before = await this.stopOrders(own.map(stop => stop.day_id)); for (const stop of own) { if (opts.keepStop) { - this.db.run('UPDATE day_assignments SET accommodation_id = NULL WHERE id = ?', stop.id); + // AC26 + await this.dayAssignmentsRepo.clearStay(stop.id); // The stop stays, but it is the traveller's now. Days hides a stop whose // accommodation_id is set, so a client left holding the old row keeps the // place invisible on a day it is standing on. @@ -588,7 +640,8 @@ export class AccommodationsService { if (released) mirror.updated.push(released); continue; } - this.db.run('DELETE FROM day_assignments WHERE id = ?', stop.id); + // AC27 + await this.dayAssignmentsRepo.deleteById(stop.id); mirror.removed.push({ id: stop.id, dayId: stop.day_id }); } await this.reanchorVias(mirror, before); @@ -646,7 +699,8 @@ export class AccommodationsService { const beforeRebuild = await this.stopOrders(own.map(stop => stop.day_id)); for (const stop of own) { - this.db.run('DELETE FROM day_assignments WHERE id = ?', stop.id); + // AC28 + await this.dayAssignmentsRepo.deleteById(stop.id); mirror.removed.push({ id: stop.id, dayId: stop.day_id }); } await this.reanchorVias(mirror, beforeRebuild); @@ -663,37 +717,54 @@ export class AccommodationsService { // The stay, its partner hotel reservation and the day stop it implies are one // logical write, and an atomic one, so a failed insert halfway can't leave an orphan. const written = await this.uow.transactional(async () => { - const result = this.db.run( - 'INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', - tripId, place_id, start_day_id, end_day_id, check_in || null, check_in_end || null, check_out || null, confirmation || null, notes || null - ); - - const newId = result.lastInsertRowid; + // AC30 + const newId = await this.dayAccommodationsRepo.insertStay({ + trip_id: tripId, + place_id: place_id ?? null, + start_day_id, + end_day_id, + check_in: check_in || null, + check_in_end: check_in_end || null, + check_out: check_out || null, + confirmation: confirmation || null, + notes: notes || null, + }); // Auto-create linked reservation for this accommodation - const placeName = this.db.get<{ name: string }>('SELECT name FROM places WHERE id = ?', place_id)?.name || 'Hotel'; - const startDayDate = this.db.get<{ date: string }>('SELECT date FROM days WHERE id = ?', start_day_id)?.date || null; + // AC31 + const placeName = (await this.placesRepo.getName(place_id))?.name || 'Hotel'; + // AC32 + const startDayDate = (await this.daysRepo.findById(start_day_id))?.date || null; const meta: Record = {}; if (check_in) meta.check_in_time = check_in; if (check_in_end) meta.check_in_end_time = check_in_end; if (check_out) meta.check_out_time = check_out; - this.db.run(` - INSERT INTO reservations (trip_id, day_id, title, reservation_time, location, confirmation_number, notes, status, type, accommodation_id, metadata) - VALUES (?, ?, ?, ?, ?, ?, ?, 'confirmed', 'hotel', ?, ?) - `, - tripId, start_day_id, placeName, startDayDate || null, null, - confirmation || null, notes || null, newId, - Object.keys(meta).length > 0 ? JSON.stringify(meta) : null - ); + // AC33 + await this.reservationsRepo.insertHotelPartner({ + trip_id: tripId, + day_id: start_day_id, + title: placeName, + reservation_time: startDayDate || null, + confirmation_number: confirmation || null, + notes: notes || null, + accommodation_id: legacyBoundIntegerText(newId), + metadata: Object.keys(meta).length > 0 ? JSON.stringify(meta) : null, + }); - return { accommodationId: newId, mirror: await this.mirrorStay(Number(newId), place_id ?? null, start_day_id, check_in) }; + return { accommodationId: newId, mirror: await this.mirrorStay(newId, place_id ?? null, start_day_id, check_in) }; }); return { accommodation: await this.getAccommodationWithPlace(written.accommodationId), mirror: written.mirror }; } - async getAccommodation(id: string | number, tripId: string | number) { - return this.db.get('SELECT * FROM day_accommodations WHERE id = ? AND trip_id = ?', id, tripId); + async getAccommodation(id: string | number, tripId: string | number): Promise { + // AC34 — the trip-scoping guard every write path (REST update/delete, + // MCP, RPC) re-reads through. Both ids parsed ONCE here (rule 21); a + // miss reads as `undefined`, matching the legacy raw-bind miss. + const idNum = toRowId(id); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return undefined; + return await this.dayAccommodationsRepo.findInTrip(idNum, tripIdNum); } async updateAccommodation(id: string | number, existing: DayAccommodation, fields: { @@ -712,11 +783,21 @@ export class AccommodationsService { // The stay row and the day stop that mirrors it describe the same booking, so a // move that wrote only one of the two must not survive. const mirror = await this.uow.transactional(async () => { - this.db.run( - 'UPDATE day_accommodations SET place_id = ?, start_day_id = ?, end_day_id = ?, check_in = ?, check_in_end = ?, check_out = ?, confirmation = ?, notes = ? WHERE id = ?', - newPlaceId, newStartDayId, newEndDayId, newCheckIn, newCheckInEnd, newCheckOut, newConfirmation, newNotes, id - ); - return await this.remirrorStay(Number(id), newPlaceId, newStartDayId, newCheckIn, { + // AC36 — `existing.id`, the SAME id AC34's gate (`getAccommodation`) + // already resolved (rule 21): the caller always reads `existing` from + // that gate first, so this reuses its value rather than re-parsing the + // raw `id` parameter. + await this.dayAccommodationsRepo.updateStay(existing.id, { + place_id: newPlaceId, + start_day_id: newStartDayId, + end_day_id: newEndDayId, + check_in: newCheckIn, + check_in_end: newCheckInEnd, + check_out: newCheckOut, + confirmation: newConfirmation, + notes: newNotes, + }); + return await this.remirrorStay(existing.id, newPlaceId, newStartDayId, newCheckIn, { checkInChanged: fields.check_in !== undefined && (fields.check_in || null) !== (existing.check_in || null), }); }); @@ -725,17 +806,18 @@ export class AccommodationsService { // lets more than one hotel booking point at the same block and there is no // unique constraint on reservations.accommodation_id, so a single .get() would // silently leave the others on the old times. - const linkedRes = this.db.all<{ id: number; metadata: string | null }>('SELECT id, metadata FROM reservations WHERE accommodation_id = ?', Number(id)); + // AC37 — runs AFTER the transaction above commits (§18.6, R5 class: pre-existing, flagged not fixed). + const linkedRes = await this.reservationsRepo.listIdMetadataByStay(existing.id); for (const res of linkedRes) { const meta = res.metadata ? JSON.parse(res.metadata) : {}; if (newCheckIn) meta.check_in_time = newCheckIn; if (newCheckInEnd) meta.check_in_end_time = newCheckInEnd; if (newCheckOut) meta.check_out_time = newCheckOut; - this.db.run('UPDATE reservations SET metadata = ?, confirmation_number = COALESCE(?, confirmation_number) WHERE id = ?', - JSON.stringify(meta), newConfirmation || null, res.id); + // AC38 + await this.reservationsRepo.setMetadataAndConfirmation(res.id, JSON.stringify(meta), newConfirmation || null); } - return { accommodation: await this.getAccommodationWithPlace(Number(id)), mirror }; + return { accommodation: await this.getAccommodationWithPlace(existing.id), mirror }; } /** @@ -755,21 +837,31 @@ export class AccommodationsService { deletedBudgetItemIds: number[]; mirror: AccommodationMirror; }> { + // AC39 — `toRowId(id)!`: every caller (REST `.remove()`, MCP, RPC) already + // called `getAccommodation`/AC34 as its own gate before reaching here, + // with the SAME raw `id` (rule 21) — the `AssignmentsService` precedent + // for a write downstream of a separately-gated existence check. + const idNum = toRowId(id)!; return await this.uow.transactional(async () => { - const linkedRes = this.db.all<{ id: number }>('SELECT id FROM reservations WHERE accommodation_id = ?', Number(id)); + // AC40 + const linkedRes = await this.reservationsRepo.listIdsByStay(idNum); const deletedBudgetItemIds: number[] = []; for (const res of linkedRes) { + // AC41/AC42 — `budget_items` is Plan 3e's table; stays raw. const linkedBudget = this.db.get<{ id: number }>('SELECT id FROM budget_items WHERE reservation_id = ?', res.id); if (linkedBudget) { this.db.run('DELETE FROM budget_items WHERE id = ?', linkedBudget.id); deletedBudgetItemIds.push(linkedBudget.id); } - this.db.run('DELETE FROM reservations WHERE id = ?', res.id); + // AC43 + await this.reservationsRepo.deleteById(res.id); } - const mirror = await this.releaseStops(Number(id), opts); + const mirror = await this.releaseStops(idNum, opts); - this.db.run('DELETE FROM day_accommodations WHERE id = ?', id); + // AC44 — no trip scoping (the legacy statement has none either; the + // caller already scoped `id` via AC34's guard). + await this.dayAccommodationsRepo.deleteById(idNum); const linkedReservationIds = linkedRes.map(r => r.id); return { linkedReservationId: linkedReservationIds[0] ?? null, diff --git a/server/src/nest/days/days.module.ts b/server/src/nest/days/days.module.ts index 761d964b6..feffa392b 100644 --- a/server/src/nest/days/days.module.ts +++ b/server/src/nest/days/days.module.ts @@ -17,6 +17,7 @@ import { DayNotes } from '../../db/entities/DayNotes.entity'; import { Trips } from '../../db/entities/Trips.entity'; import { Reservations } from '../../db/entities/Reservations.entity'; import { ReservationEndpoints } from '../../db/entities/ReservationEndpoints.entity'; +import { DayAccommodations } from '../../db/entities/DayAccommodations.entity'; /** * Days (S6 — Phase 2 trip sub-domain), mounted at /api/trips/:tripId/days. @@ -28,12 +29,14 @@ import { ReservationEndpoints } from '../../db/entities/ReservationEndpoints.ent * own domain now (day-notes/). * * `MikroOrmModule.forFeature([Days, DayAssignments, DayNotes, Trips, - * Reservations, ReservationEndpoints])` registers `DaysRepository`/ - * `DayAssignmentsRepository`/`DayNotesRepository`/`TripsRepository`/ - * `ReservationsRepository`/`ReservationEndpointsRepository` for - * `DaysService`'s `@InjectRepository` constructor params (Plan 3c Task 2; - * the last two added by Plan 3d Task 2 for DY14–DY18/DY23) — the same - * forFeature + `@InjectRepository` wiring every converted domain copies + * Reservations, ReservationEndpoints, DayAccommodations])` registers + * `DaysRepository`/`DayAssignmentsRepository`/`DayNotesRepository`/ + * `TripsRepository`/`ReservationsRepository`/`ReservationEndpointsRepository`/ + * `DayAccommodationsRepository` for `DaysService`'s `@InjectRepository` + * constructor params (Plan 3c Task 2; `Reservations`/`ReservationEndpoints` + * added by Plan 3d Task 2 for DY14–DY18/DY23; `DayAccommodations` added by + * Plan 3d Task 3 for DY19/DY20/DY22) — the same forFeature + + * `@InjectRepository` wiring every converted domain copies * (`trip-membership.module.ts`'s precedent for pulling in a repository this * module doesn't otherwise own). */ @@ -46,7 +49,7 @@ import { ReservationEndpoints } from '../../db/entities/ReservationEndpoints.ent AuthModule, RealtimeModule, PluginGuardsModule, - MikroOrmModule.forFeature([Days, DayAssignments, DayNotes, Trips, Reservations, ReservationEndpoints]), + MikroOrmModule.forFeature([Days, DayAssignments, DayNotes, Trips, Reservations, ReservationEndpoints, DayAccommodations]), ], controllers: [DaysController], providers: [DaysService, DaysMcp, DaysRpc], diff --git a/server/src/nest/days/days.service.ts b/server/src/nest/days/days.service.ts index 74d6cef5d..977f526f5 100644 --- a/server/src/nest/days/days.service.ts +++ b/server/src/nest/days/days.service.ts @@ -21,6 +21,8 @@ import { Reservations } from '../../db/entities/Reservations.entity'; import type { ReservationsRepository } from '../../db/repositories/Reservations.repository'; import { ReservationEndpoints } from '../../db/entities/ReservationEndpoints.entity'; import type { ReservationEndpointsRepository } from '../../db/repositories/ReservationEndpoints.repository'; +import { DayAccommodations } from '../../db/entities/DayAccommodations.entity'; +import type { DayAccommodationsRepository } from '../../db/repositories/DayAccommodations.repository'; type Trip = TripAccess; @@ -60,11 +62,12 @@ export class DayReorderError extends Error {} /** * Day domain service — the day + day-assignment-projection SQL now lives in * `DaysRepository`/`DayAssignmentsRepository`/`DayNotesRepository` (Plan 3c - * Task 2); `restampReservationDates`/`assertNoInvertedAccommodation` and the - * `day_accommodations`/`reservations`/`reservation_endpoints` halves of - * `resyncAccommodationDays` stay on the raw `DatabaseService` connection — - * those tables belong to Plan 3d (`// DYn — Plan 3d` marks each site). Trip - * access still rides `DatabaseService.canAccessTrip` (Task 0b's 110 + * Task 2); `restampReservationDates`/`assertNoInvertedAccommodation` and + * `resyncAccommodationDays`'s `reservations`/`reservation_endpoints`/ + * `day_accommodations` statements now go through `ReservationsRepository`/ + * `ReservationEndpointsRepository` (Plan 3d Task 2) and + * `DayAccommodationsRepository` (Plan 3d Task 3) — no raw SQL left in this + * file. Trip access still rides `DatabaseService.canAccessTrip` (Task 0b's 110 * unconverted callers, this among them); mutations use the 'day_edit' * permission; the WebSocket broadcast keeps its legacy call path. * @@ -124,6 +127,10 @@ export class DaysService { // this task adds (`day_accommodations` stays raw here, Task 3's own). @InjectRepository(Reservations) private readonly reservationsRepo: ReservationsRepository, @InjectRepository(ReservationEndpoints) private readonly reservationEndpointsRepo: ReservationEndpointsRepository, + // Plan 3d Task 3 (DY19/DY20/DY22): `assertNoInvertedAccommodation`'s + // read and `resyncAccommodationDays`'s `day_accommodations` statements + // convert onto this — Task 2 deliberately left it unadded ("Task 3's own"). + @InjectRepository(DayAccommodations) private readonly dayAccommodationsRepo: DayAccommodationsRepository, ) {} async verifyTripAccess(tripId: string | number, userId: number) { @@ -399,27 +406,19 @@ export class DaysService { /** * A stay must not end before it begins after a reorder/insert. * - * DY19 — Plan 3d: the statement's root table is `day_accommodations` - * (joined to `days` only to read `day_number`), so it stays raw on - * `DatabaseService` per the inventory's ruling, even though `days` itself - * is owned here. + * DY19 — Plan 3d Task 3: the statement's root table is `day_accommodations` + * (joined to `days` only to read `day_number`), so it converts on + * `DayAccommodationsRepository.listStartEndDayNumbers` (the shared class + * Task 2 stubbed and this task appends to), not a new method on + * `DaysRepository` — `days` here is only the JOIN target. * * `tripId: number` (Task 9 fix wave, H2): the same parsed-once value every * other survivor in this class now takes — see `restampReservationDates`'s * docstring for why binding the raw route string here was the live bug. */ private async assertNoInvertedAccommodation(tripId: number): Promise { - // DY19 — Plan 3d (marker normalised to the `// — Plan 3d` shape - // every other survivor in this file uses, per the Task 0 review carry - // item — this task does not own `day_accommodations`, so the statement - // itself is untouched) - const spans = this.db.all<{ id: number; start_no: number; end_no: number }>(` - SELECT a.id, s.day_number AS start_no, e.day_number AS end_no - FROM day_accommodations a - JOIN days s ON a.start_day_id = s.id - JOIN days e ON a.end_day_id = e.id - WHERE a.trip_id = ? - `, tripId); + // DY19 — Plan 3d Task 3: `DayAccommodationsRepository.listStartEndDayNumbers`. + const spans = await this.dayAccommodationsRepo.listStartEndDayNumbers(tripId); for (const span of spans) { if (span.start_no > span.end_no) { throw new DayReorderError('This move would make an accommodation end before it starts.'); @@ -437,8 +436,8 @@ export class DaysService { * whole trip still shifts everything together. The linked hotel reservation follows * its accommodation's start day in both branches. * - * DY20/DY22 — Plan 3d (`day_accommodations`) stay raw, Task 3's own table; - * DY21 (`DaysRepository.findByTripAndDate`) and DY25 + * DY20/DY22 — Plan 3d Task 3: `DayAccommodationsRepository.listForResync`/ + * `setDayRange`. DY21 (`DaysRepository.findByTripAndDate`) and DY25 * (`DaysRepository.findById`) convert — both root on `days`, which this * plan owns. DY24 (the `day_assignments` stop that follows its booking) * converts via `DayAssignmentsRepository.reanchorToDay`, a Kysely @@ -447,7 +446,7 @@ export class DaysService { * `ReservationsRepository.restampLinkedReservation` — Plan 3d Task 2. * * `tripId: number` (Task 9 fix wave, H2): callers now pass their own - * `toRowId`-parsed value; DY20's raw bind uses it too (previously the raw + * `toRowId`-parsed value; DY20's read uses it too (previously the raw * route string — the same affinity-seam mismatch documented on * `restampReservationDates`). */ @@ -455,16 +454,10 @@ export class DaysService { tripId: number, prevDateByDayId: Map, ): Promise { - // DY20 — Plan 3d - const stays = this.db.all<{ id: number; start_day_id: number; end_day_id: number }>( - 'SELECT id, start_day_id, end_day_id FROM day_accommodations WHERE trip_id = ?', - tripId - ); + // DY20 + const stays = await this.dayAccommodationsRepo.listForResync(tripId); if (stays.length === 0) return; - // DY22 — Plan 3d - const updateStay = this.db.prepare('UPDATE day_accommodations SET start_day_id = ?, end_day_id = ? WHERE id = ?'); - for (const stay of stays) { const oldStartDate = prevDateByDayId.get(stay.start_day_id); const oldEndDate = prevDateByDayId.get(stay.end_day_id); @@ -474,7 +467,8 @@ export class DaysService { const newEnd = await this.daysRepo.findByTripAndDate(tripId, oldEndDate); if (newStart && newEnd && newStart.day_number <= newEnd.day_number && (newStart.id !== stay.start_day_id || newEnd.id !== stay.end_day_id)) { - updateStay.run(newStart.id, newEnd.id, stay.id); + // DY22 + await this.dayAccommodationsRepo.setDayRange(stay.id, newStart.id, newEnd.id); // The day stop a booking wrote moves with it, the way its linked booking does. // Left behind it would sit on a day the traveller no longer sleeps there, with // nothing on screen to say why. Re-indexed to the end of the target day, because diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index be6dfb8a6..937bd07c2 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -552,15 +552,18 @@ export class PlacesService { * * Runs inside the caller's transaction. */ - private async cancelStaysAt(tripId: string | number, placeId: string | number, into: CancelledStays): Promise { - // PL16 — `day_accommodations` is Plan 3d's table; stays raw. Runs inside - // the caller's own transaction (R4 — Plan 3d must not re-open or - // re-scope this transaction when it converts `accommodations`). - const stays = this.dbs.all<{ id: number }>( - 'SELECT id FROM day_accommodations WHERE trip_id = ? AND place_id = ?', tripId, placeId, - ); - for (const stay of stays) { - const gone = await this.accommodations.deleteAccommodation(stay.id); + private async cancelStaysAt(tripId: number, placeId: number, into: CancelledStays): Promise { + // PL16 (Plan 3d Task 3) — `day_accommodations` is that task's table; + // reached through `AccommodationsService.listStayIdsForPlace` (already + // injected here) rather than a new `DayAccommodationsRepository` + // dependency of this service's own (keeps this file's constructor, and + // the shared positional test-helper wiring, untouched). Runs inside the + // caller's own transaction (R4 — Plan 3d must not re-open or re-scope + // this transaction), and keeps `id`/`tripId` as the SAME `toRowId`-parsed + // numbers `remove`/`removeMany` already resolved (rule 21). + const stayIds = await this.accommodations.listStayIdsForPlace(tripId, placeId); + for (const stayId of stayIds) { + const gone = await this.accommodations.deleteAccommodation(stayId); // What went down with the night is what the caller has to announce. The // partner booking and its expense are rows the Bookings list and the Costs // total are still holding; place:deleted says nothing about either, and a diff --git a/server/tests/helpers/accommodations-service.ts b/server/tests/helpers/accommodations-service.ts index ee6f6d079..de0f46c10 100644 --- a/server/tests/helpers/accommodations-service.ts +++ b/server/tests/helpers/accommodations-service.ts @@ -10,7 +10,8 @@ import { TrekPhotosRepository } from '../../src/nest/photos/trek-photos.reposito import { createTestUnitOfWork, createTestAppSettingsRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestDayAssignmentsRepo, createTestDaysRepo, createTestPlacesRepo, - createTestTripMembersRepo, createTestRoadtripViasRepo, sharedTestOrm, + createTestTripMembersRepo, createTestRoadtripViasRepo, createTestDayAccommodationsRepo, createTestReservationsRepo, + sharedTestOrm, } from './test-uow'; /** @@ -52,5 +53,13 @@ export async function accommodationsOver(dbs: DatabaseService): Promise { await createTestTripsRepo(testDb), await createTestReservationsRepo(testDb), await createTestReservationEndpointsRepo(testDb), + await createTestDayAccommodationsRepo(testDb), ); }); const reorderDays = async (tripId: number, orderedIds: number[]) => await svc.reorder(tripId, orderedIds); diff --git a/server/tests/unit/nest/days.service.test.ts b/server/tests/unit/nest/days.service.test.ts index 9547aee68..2b81c3b68 100644 --- a/server/tests/unit/nest/days.service.test.ts +++ b/server/tests/unit/nest/days.service.test.ts @@ -70,6 +70,7 @@ import { createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestReservationsRepo, createTestReservationEndpointsRepo, + createTestDayAccommodationsRepo, } from '../../helpers/test-uow'; let svc: DaysService; @@ -87,6 +88,7 @@ beforeAll(async () => { await createTestTripsRepo(testDb), await createTestReservationsRepo(testDb), await createTestReservationEndpointsRepo(testDb), + await createTestDayAccommodationsRepo(testDb), ); }); let accommodations: Awaited>; @@ -845,6 +847,7 @@ describe('DaysService.canEdit', () => { await createTestTripsRepo(testDb), await createTestReservationsRepo(testDb), await createTestReservationEndpointsRepo(testDb), + await createTestDayAccommodationsRepo(testDb), ); const trip = { id: 1, user_id: 1 } as never; diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index deb853e50..1eef641dd 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -705,6 +705,35 @@ describe('remove', () => { expect(removeIfUnreferencedSpy).toHaveBeenCalledWith('ChIJgid'); }); + + it('PLACE-SVC-019c (R5, MikroORM 7.2.1 nested transactional on SQLite) — a stay\'s own transaction nests as a SAVEPOINT under this transaction: a later failure in the OUTER write rolls the already-released inner savepoint back too', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const hotel = createPlace(testDb, trip.id, { name: 'Hotel Adlon' }); + const { accommodation } = (await accommodations.createAccommodation(trip.id, { + place_id: hotel.id, start_day_id: day.id, end_day_id: day.id, + })) as { accommodation: { id: number } }; + const reservation = testDb.prepare('SELECT id FROM reservations WHERE accommodation_id = ?').get(accommodation.id) as { id: number }; + + // cancelStaysAt (PL16) calls AccommodationsService.deleteAccommodation + // FIRST inside this transaction — its own `uow.transactional` call nests + // as a SAVEPOINT (UnitOfWork's NESTED propagation) and releases + // successfully, deleting the stay and its reservation. Only THEN does + // the place row's own DELETE fail. If the nested call were an + // independent, already-committed transaction instead of a true + // savepoint, the stay and its reservation would stay deleted regardless + // of what happens to the place afterward — they do not. + testDb.exec("CREATE TRIGGER boom BEFORE DELETE ON places BEGIN SELECT RAISE(ABORT, 'boom'); END"); + try { + await expect(svc.remove(String(trip.id), String(hotel.id))).rejects.toThrow(); + expect(testDb.prepare('SELECT id FROM day_accommodations WHERE id = ?').get(accommodation.id)).toBeDefined(); + expect(testDb.prepare('SELECT id FROM reservations WHERE id = ?').get(reservation.id)).toBeDefined(); + expect(testDb.prepare('SELECT id FROM places WHERE id = ?').get(hotel.id)).toBeDefined(); + } finally { + testDb.exec('DROP TRIGGER boom'); + } + }); }); // ── removeMany ──────────────────────────────────────────────────────────────── diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index 7f88d8643..9b342140a 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -130,6 +130,7 @@ beforeAll(async () => { await createTestTripsRepo(dbs().connection), await createTestReservationsRepo(dbs().connection), await createTestReservationEndpointsRepo(dbs().connection), + await createTestDayAccommodationsRepo(dbs().connection), ); placesSvc = new PlacesService( dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 7facb611d..085a3b609 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -163,6 +163,7 @@ beforeAll(async () => { await createTestTripsRepo(dbs().connection), await createTestReservationsRepo(dbs().connection), await createTestReservationEndpointsRepo(dbs().connection), + await createTestDayAccommodationsRepo(dbs().connection), ); placesSvc = new PlacesService( dbs(),