From 9f1f5c45bf0d1fe9e7ac489a8df31f76f569569e Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 08:15:05 +0200 Subject: [PATCH] refactor(places): places, tags, ratings and place images through repositories --- .../AssignmentParticipants.repository.ts | 21 + .../repositories/DayAssignments.repository.ts | 74 ++- .../repositories/PlaceRatings.repository.ts | 33 ++ .../src/db/repositories/Places.repository.ts | 417 +++++++++++++++- server/src/db/repositories/Tags.repository.ts | 75 +++ .../nest/assignments/assignments.service.ts | 103 ++-- server/src/nest/places/place-image.ts | 17 + server/src/nest/places/places.mcp.ts | 30 +- server/src/nest/places/places.module.ts | 19 +- server/src/nest/places/places.service.ts | 459 ++++++++++-------- server/tests/e2e/places.e2e.test.ts | 139 ++---- .../DayAssignments.repository.test.ts | 50 +- .../PlaceRatings.repository.test.ts | 90 +++- .../db/repositories/Places.repository.test.ts | 304 +++++++++++- .../db/repositories/Tags.repository.test.ts | 62 ++- .../unit/nest/assignments.service.test.ts | 54 +++ .../tests/unit/nest/places.directions.test.ts | 7 +- server/tests/unit/nest/places.service.test.ts | 79 ++- .../tests/unit/nest/roadtrip-planning.test.ts | 2 +- .../unit/services/conflictUpdate.test.ts | 7 + 20 files changed, 1668 insertions(+), 374 deletions(-) diff --git a/server/src/db/repositories/AssignmentParticipants.repository.ts b/server/src/db/repositories/AssignmentParticipants.repository.ts index 3e29e0611..b48c9778f 100644 --- a/server/src/db/repositories/AssignmentParticipants.repository.ts +++ b/server/src/db/repositories/AssignmentParticipants.repository.ts @@ -99,6 +99,27 @@ export class AssignmentParticipantsRepository extends TrekRepository { if (user_ids.length === 0) return; diff --git a/server/src/db/repositories/DayAssignments.repository.ts b/server/src/db/repositories/DayAssignments.repository.ts index 93c971762..34431bc04 100644 --- a/server/src/db/repositories/DayAssignments.repository.ts +++ b/server/src/db/repositories/DayAssignments.repository.ts @@ -300,13 +300,20 @@ export class DayAssignmentsRepository extends TrekRepository { /** * AS9 (`assignmentExistsInDay`) — `SELECT da.id FROM day_assignments da * JOIN days d ON da.day_id = d.id WHERE da.id = ? AND da.day_id = ? AND - * d.trip_id = ?`. Raw-bind (`number | string`, D4's T5 escape hatch, - * `TripsRepository.findAccessible`'s precedent): the legacy guard binds - * the route's raw params straight into the statement with no `Number()`/ - * `toRowId` conversion of its own, so this must accept and bind exactly - * what it's handed, not coerce it first. + * d.trip_id = ?`. + * + * **`id: number`, not `number | string` (Task 3 review H1, absorbed in + * Task 4 — `task-3-review.md` §9.1):** this method's ONLY caller + * (`AssignmentsService.assignmentExistsInDay`) now runs `toRowId` on the + * route's raw `id` BEFORE calling this — a non-canonical id (`"3 "`, + * `"3.0"`) must resolve to "not found" in the SERVICE gate, not pass this + * statement's own SQLite text/integer affinity match and then disagree + * with `deleteAssignment`'s `toRowId(id)!` downstream (the H1 regression). + * `day_id`/`trip_id` keep their `number | string` raw-bind scoping — + * `deleteAssignment` never reads either back, so there is no gate-vs-write + * id to protect there. */ - async existsInDay(id: number | string, day_id: number | string, trip_id: number | string): Promise { + async existsInDay(id: number, day_id: number | string, trip_id: number | string): Promise { const row = await this.qb('da') .join('da.day', 'd') .select(['da.id']) @@ -318,13 +325,21 @@ export class DayAssignmentsRepository extends TrekRepository { /** * AS12 (`getAssignmentForTrip`) — `SELECT da.* FROM day_assignments da * JOIN days d ON da.day_id = d.id WHERE da.id = ? AND d.trip_id = ?`. - * Same raw-bind seam as `existsInDay` above — every real caller - * (`AssignmentOpsController`, `AssignmentsMcp`, `ItineraryRpc`) hands this - * the route/tool id verbatim, and `ItineraryRpc.unassign` reads - * `existing.day_id` off the result, so this is a genuine re-select, not - * only an existence guard. + * + * **`id: number`, not `number | string` (Task 3 review H1, absorbed in + * Task 4 — `task-3-review.md` §9.1):** this method's ONLY caller + * (`AssignmentsService.getAssignmentForTrip`) now runs `toRowId` on the + * route/tool's raw `id` BEFORE calling this, the same H1 fix + * `existsInDay` above applies — every mutation behind this gate + * (`updateTime`/`setEndDay`/`updateNotes`/`setLegTransportMode`/ + * `setIncomingLegTransportMode`/`setParticipants`/`moveAssignment`) + * converts `id` with its own `toRowId(id)!`, and `ItineraryRpc.unassign` + * reads `existing.day_id` off THIS result (a genuine re-select, not only + * an existence guard) — so the id this read resolves must be the same one + * every one of those writes uses. `trip_id` keeps its `number | string` + * raw-bind scoping, unchanged. */ - async findInTrip(id: number | string, trip_id: number | string): Promise { + async findInTrip(id: number, trip_id: number | string): Promise { const row = await this.qb('da') .join('da.day', 'd') .select(['da.*']) @@ -509,4 +524,39 @@ export class DayAssignmentsRepository extends TrekRepository { async setIncomingLegMode(id: number, mode: string | null): Promise { await this.nativeUpdate({ id }, { incoming_leg_transport_mode: mode }); } + + // --------------------------------------------------------------------------- + // Plan 3c Task 4 (`PlacesService.exportGpx`) — appended per the task-4 + // brief's "task-3-report.md" pointer ("DayAssignmentsRepository for the + // PL30 GPX itinerary read if it fits there"). Unlike AS16/AS18, this join + // (`day_assignments` -> `days`, `day_assignments` -> `places`) is fully + // expressible through the entity's own declared relations (`da.day`, + // `da.place`) — no `day_accommodations`-shaped gap — so the QueryBuilder is + // used directly rather than Kysely. + // --------------------------------------------------------------------------- + + /** + * PL30 (`places.service.ts::exportGpx`) — `SELECT d.day_number, d.date, + * d.title, p.name, p.lat, p.lng FROM days d JOIN day_assignments da ON + * da.day_id = d.id JOIN places p ON p.id = da.place_id WHERE d.trip_id = ? + * AND p.lat IS NOT NULL AND p.lng IS NOT NULL ORDER BY d.day_number, + * da.order_index` — rooted here on `day_assignments` instead of `days` + * (the legacy statement's own `FROM` root) because both joins the legacy + * statement needs (`day_assignments -> days`, `day_assignments -> places`) + * are single hops FROM this entity, while `days` has no direct relation to + * `places`. Same result set, same ordering; the root alias choice is not + * observable from the row shape. + */ + async listItineraryForGpx(trip_id: string | number): Promise<{ + day_number: number; date: string | null; title: string | null; + name: string; lat: number; lng: number; + }[]> { + return this.qb('da') + .join('da.day', 'd') + .join('da.place', 'p') + .select(['d.day_number', 'd.date', 'd.title', 'p.name', 'p.lat', 'p.lng']) + .where('d.trip_id = ? AND p.lat IS NOT NULL AND p.lng IS NOT NULL', [trip_id]) + .orderBy({ 'd.day_number': 'asc', 'da.order_index': 'asc' }) + .execute<{ day_number: number; date: string | null; title: string | null; name: string; lat: number; lng: number }[]>('all', false); + } } diff --git a/server/src/db/repositories/PlaceRatings.repository.ts b/server/src/db/repositories/PlaceRatings.repository.ts index 8f1337afb..dfd89d87b 100644 --- a/server/src/db/repositories/PlaceRatings.repository.ts +++ b/server/src/db/repositories/PlaceRatings.repository.ts @@ -40,4 +40,37 @@ export class PlaceRatingsRepository extends TrekRepository { .orderBy({ 'pr.created_at': 'asc' }) .execute('all', false); } + + /** + * PL51 (`places.service.ts::rate`) — `INSERT INTO place_ratings (place_id, + * user_id, rating) VALUES (?, ?, ?) ON CONFLICT(place_id, user_id) DO + * UPDATE SET rating = excluded.rating` — the only explicit `ON CONFLICT … + * DO UPDATE` in the Plan 3c cluster (#1435). `onConflictFields: ['place', + * 'user']` on the entity's own declared composite unique + * (`PlaceRatingsSchema`'s `uniques: [{ properties: ['place', 'user'] }]`, + * itself generated from the migration's `UNIQUE(place_id, user_id)` + * — verified against both, not assumed). `onConflictMergeFields: ['rating']` + * is load-bearing, not decorative: without it `em.upsert`'s default merge + * set is every non-key column, which on THIS entity is only `rating` + * anyway (`created_at` is the sole other column and is excluded because it + * is the primary key's sibling default, not a mergeable field) — named + * explicitly so a future column added to this entity does not silently + * join the merge set and start touching a column the legacy statement + * never wrote on conflict. The ruling this method exists to satisfy: a + * vote must never touch `places.updated_at` (a separate table this + * statement doesn't reference at all) so it can't 409 another member's + * `If-Match` — proven by a repository test that reads `places.updated_at` + * unchanged across a rating write. + */ + async upsertRating(place_id: number, user_id: number, rating: number): Promise { + await this.upsert( + { place: place_id, user: user_id, rating }, + { onConflictFields: ['place', 'user'], onConflictAction: 'merge', onConflictMergeFields: ['rating'] }, + ); + } + + /** PL50 — `DELETE FROM place_ratings WHERE place_id = ? AND user_id = ?` (a null rating clears the vote). */ + async deleteRating(place_id: number, user_id: number): Promise { + await this.nativeDelete({ place: place_id, user: user_id }); + } } diff --git a/server/src/db/repositories/Places.repository.ts b/server/src/db/repositories/Places.repository.ts index d4bdc30e6..eee29b869 100644 --- a/server/src/db/repositories/Places.repository.ts +++ b/server/src/db/repositories/Places.repository.ts @@ -1,6 +1,6 @@ import type { Places } from '../entities/Places.entity'; import type { Tags } from '../entities/Tags.entity'; -import { columnRef } from '../dialect/sql-functions'; +import { absDifference, columnRef, currentTimestamp, lowerTrim } from '../dialect/sql-functions'; import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; @@ -53,7 +53,8 @@ export interface TagRow { const _tagRowKeys: AssertRowKeys = true; -interface PlaceWithCategoryRow extends PlaceRow { +/** PL3 (`PlacesService.list`) — a `places` row joined with its category's flat columns. */ +export interface PlaceWithCategoryRow extends PlaceRow { category_name: string | null; category_color: string | null; category_icon: string | null; @@ -181,4 +182,416 @@ export class PlacesRepository extends TrekRepository { .execute<{ id: number } | undefined>('get', false); return !!row; } + + // --------------------------------------------------------------------------- + // Plan 3c Task 4 (`PlacesService` core) — every method below is additive; + // `findWithTagsAndRatings`/`existsByGoogleIdOrImageUrl` above are Task 0b's + // and Task 1's, unchanged. + // --------------------------------------------------------------------------- + + /** + * PL0/PL7/PL49/AS5 — `SELECT id FROM places WHERE id = ? AND trip_id = ?`. + * + * **`id: number`, not `number | string` (Task 3 review H1, absorbed + * here — `task-3-review.md` §9.1):** the legacy raw-bind seam other + * guard-shaped methods in this cluster preserve (`TripsRepository + * .findAccessible`'s precedent) is the WRONG shape for a method whose + * caller then writes with `toRowId(placeId)!`. A non-canonical id + * (`"3 "`, `"3.0"`, `"+3"`) matches this statement via SQLite's own + * loose text/integer affinity — `existsInTrip` would answer `true` — while + * `toRowId` (`/^\d+$/` + `Number.isSafeInteger`, program rule 15) rejects + * the same string and returns `null`, so the write downstream runs + * `toRowId(placeId)!` on a value the gate just vouched for and gets + * `null` — exactly Task 3's H1 (`AssignmentsService.dayExists`/ + * `placeExists`/`assignmentExistsInDay`/`getAssignmentForTrip`, all four + * kept on this seam, all four reproducing it). This method's only two + * callers (`PlacesService.get`/`applyUpdate`'s existence half — via + * `applyUpdate` calling `findInTrip` below — and `AssignmentsService + * .placeExists`, once Task 4 repoints it) now run `toRowId` FIRST and + * pass the validated `number` in, matching `DaysService.getDay`'s shape + * (the review's cited correct precedent) — so `id` here is narrowed to + * match what every caller actually has after its own gate, and a second + * `toRowId(...)!` downstream can never disagree with this one. + */ + async existsInTrip(id: number, trip_id: number): Promise { + const row = await this.qb('p') + .select(['p.id']) + .where('p.id = ? AND p.trip_id = ?', [id, trip_id]) + .execute<{ id: number } | undefined>('get', false); + return !!row; + } + + /** + * PL9/PL48 — `SELECT * FROM places WHERE id = ? AND trip_id = ?`, byte- + * identical statements at both legacy sites (`applyUpdate`'s pre-image + * read and `searchImage`'s existence-plus-name read) — one method for + * both rather than two identically-bodied ones. + * + * `id: number`, same H1 fix and reasoning as `existsInTrip` above: both + * callers write with the SAME already-`toRowId`'d id this read returns + * (`applyUpdate` feeds it straight into `updatePlace`/`tagsRepo`; + * `searchImage` only reads, but takes the same gate shape for + * consistency — one rule for every place-id gate in this service, not + * one per call site). + */ + async findInTrip(id: number, trip_id: number): Promise { + return this.qb('p') + .select(['p.*']) + .where('p.id = ? AND p.trip_id = ?', [id, trip_id]) + .execute('get', false); + } + + /** + * PL17/PL20 — `SELECT google_place_id, image_url FROM places WHERE id = ? + * AND trip_id = ?`, the reclaim-candidate projection `remove`/`removeMany` + * read BEFORE their delete transaction. `id: number`, same H1 fix: both + * callers immediately follow a `true` result with `deleteById(id)` on the + * SAME numeric id. + */ + async reclaimInputs(id: number, trip_id: number): Promise<{ google_place_id: string | null; image_url: string | null } | undefined> { + return this.qb('p') + .select(['p.google_place_id', 'p.image_url']) + .where('p.id = ? AND p.trip_id = ?', [id, trip_id]) + .execute<{ google_place_id: string | null; image_url: string | null } | undefined>('get', false); + } + + /** PL19/PL21 — `DELETE FROM places WHERE id = ?`, unscoped: every caller proved trip access first. */ + async deleteById(id: number): Promise { + await this.nativeDelete({ id }); + } + + /** + * PL23 — `` SELECT id FROM places WHERE trip_id = ? AND id IN (${…}) ``, + * narrowed to the caller's own INPUT ORDER (not SQL result order — SQLite + * makes no ordering promise for an `IN (...)` scan and the legacy code + * re-filters the original `ids` array through a `Set` built from the row + * set for exactly this reason). `$in` for the dynamic list (rule 17b, "the + * good kind"). + */ + async scopedIds(trip_id: string | number, ids: number[]): Promise { + if (ids.length === 0) return []; + const rows = await this.qb('p') + .select(['p.id']) + .where('p.trip_id = ?', [trip_id]) + .andWhere({ id: { $in: ids } }) + .execute<{ id: number }[]>('all', false); + const owned = new Set(rows.map((r) => r.id)); + return ids.filter((id) => owned.has(id)); + } + + /** + * PL4 — the 25-column `INSERT INTO places (...)` the service's `create` + * builds. Every value here is already fully coerced by the caller (the + * `?? null` vs `|| null` split PL4's own legacy comment documents — 0 is + * legitimate for `lat`/`lng`/`price`/`fill_percent`, and + * `duration_minutes` defaults to 60 — stays the service's decision, this + * writes exactly what it is handed, `DaysRepository.createDay`'s split). + * `em.insert()` returns the generated PK (R6's `lastInsertRowid` + * replacement). + */ + async insertPlace(input: { + trip_id: number; + name: string; + description: string | null; + lat: number | null; + lng: number | null; + address: string | null; + category_id: number | null; + price: number | null; + currency: string | null; + place_time: string | null; + end_time: string | null; + duration_minutes: number; + notes: string | null; + image_url: string | null; + google_place_id: string | null; + google_ftid: string | null; + osm_id: string | null; + amap_poi_id: string | null; + website: string | null; + phone: string | null; + transport_mode: string; + route_geometry: string | null; + route_color: string | null; + stop_type: string | null; + fill_percent: number | null; + }): Promise { + return await this.insert({ + trip: input.trip_id, + name: input.name, + description: input.description, + lat: input.lat, + lng: input.lng, + address: input.address, + category: input.category_id, + price: input.price, + currency: input.currency, + place_time: input.place_time, + end_time: input.end_time, + duration_minutes: input.duration_minutes, + notes: input.notes, + image_url: input.image_url, + google_place_id: input.google_place_id, + google_ftid: input.google_ftid, + osm_id: input.osm_id, + amap_poi_id: input.amap_poi_id, + website: input.website, + phone: input.phone, + transport_mode: input.transport_mode, + route_geometry: input.route_geometry, + route_color: input.route_color, + stop_type: input.stop_type, + fill_percent: input.fill_percent, + }); + } + + /** + * PL11 — `UPDATE places SET name = COALESCE(?, name), description = ?, + * lat = ?, lng = ?, address = ?, category_id = ?, price = ?, currency = + * COALESCE(?, currency), place_time = ?, end_time = ?, duration_minutes = + * ?, notes = ?, image_url = ?, google_place_id = ?, google_ftid = ?, + * osm_id = ?, amap_poi_id = ?, website = ?, phone = ?, transport_mode = + * COALESCE(?, transport_mode), route_color = ?, stop_type = ?, + * fill_percent = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?` — ONE + * `nativeUpdate` with a typed full partial (the ruling): the SQL + * `COALESCE(?, col)` keep-if-null semantics (`name`/`currency`/ + * `transport_mode`) and the twenty `!== undefined ? x : existing.x` + * pre-image fallbacks are both already resolved to their FINAL value by + * the caller (`PlacesService.applyUpdate`, which reads the pre-image via + * `findInTrip` first) — this method writes exactly what it is handed, + * plus the timestamp stamp every write gets. + */ + async updatePlace(id: number, write: { + name: string | null; + description: string | null; + lat: number | null; + lng: number | null; + address: string | null; + category_id: number | null; + price: number | null; + currency: string | null; + place_time: string | null; + end_time: string | null; + duration_minutes: number | null; + notes: string | null; + image_url: string | null; + google_place_id: string | null; + google_ftid: string | null; + osm_id: string | null; + amap_poi_id: string | null; + website: string | null; + phone: string | null; + transport_mode: string | null; + route_color: string | null; + stop_type: string | null; + fill_percent: number | null; + }): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { + name: write.name, + description: write.description, + lat: write.lat, + lng: write.lng, + address: write.address, + category: write.category_id, + price: write.price, + currency: write.currency, + place_time: write.place_time, + end_time: write.end_time, + duration_minutes: write.duration_minutes, + notes: write.notes, + image_url: write.image_url, + google_place_id: write.google_place_id, + google_ftid: write.google_ftid, + osm_id: write.osm_id, + amap_poi_id: write.amap_poi_id, + website: write.website, + phone: write.phone, + transport_mode: write.transport_mode, + route_color: write.route_color, + stop_type: write.stop_type, + fill_percent: write.fill_percent, + updated_at: currentTimestamp(platform), + }); + } + + /** + * PL25 (`findDuplicatePlace`'s `externalId` strategy) — `SELECT id, + * google_ftid FROM places WHERE trip_id = ? AND (google_place_id = ? OR + * google_ftid = ? OR osm_id = ? OR amap_poi_id = ?) ORDER BY id ASC LIMIT + * 1` — the same candidate id bound in all four positions, exactly as the + * legacy statement does. + */ + async findDuplicateByExternalId(trip_id: string, external_id: string): Promise<{ id: number; google_ftid: string | null } | undefined> { + return this.qb('p') + .select(['p.id', 'p.google_ftid']) + .where('p.trip_id = ?', [trip_id]) + .andWhere({ + $or: [ + { google_place_id: external_id }, + { google_ftid: external_id }, + { osm_id: external_id }, + { amap_poi_id: external_id }, + ], + }) + .orderBy({ 'p.id': 'asc' }) + .limit(1) + .execute<{ id: number; google_ftid: string | null } | undefined>('get', false); + } + + /** + * PL26 (`findDuplicatePlace`'s `name` strategy) — `SELECT id, google_ftid + * FROM places WHERE trip_id = ? AND lower(trim(name)) = ? ORDER BY id ASC + * LIMIT 1`. Rule 18's documented-disagreement exception clause: the legacy + * statement's OWN bound value is already JS-lowered+trimmed + * (`normalizePlaceName`, `@trek/shared/place/place-match.ts`, Unicode- + * aware), so `lowered_trimmed_name` here must be passed in ALREADY + * lowered — never re-lowered through `lowerParam` (that would apply + * SQLite's ASCII-only `LOWER()` a second time, on a value the caller + * already folded fully). `lowerTrim(platform, 'p.name')` renders + * `LOWER(TRIM(name))` (SQLite `LOWER()` is ASCII-only, program rule 18 — + * the documented divergence PL26's legacy comment (`places.service.ts + * :609-619`) already accepts and this repository reproduces literally, + * not fixes). + */ + async findDuplicateByName(trip_id: string, lowered_trimmed_name: string): Promise<{ id: number; google_ftid: string | null } | undefined> { + const platform = this.getEntityManager().getPlatform(); + return this.qb('p') + .select(['p.id', 'p.google_ftid']) + .where('p.trip_id = ?', [trip_id]) + .andWhere({ [lowerTrim(platform, 'p.name')]: lowered_trimmed_name }) + .orderBy({ 'p.id': 'asc' }) + .limit(1) + .execute<{ id: number; google_ftid: string | null } | undefined>('get', false); + } + + /** + * PL27 (`findDuplicatePlace`'s `coords` strategy) — `SELECT id, + * google_ftid FROM places WHERE trip_id = ? AND lat IS NOT NULL AND lng IS + * NOT NULL AND abs(lat - ?) <= ? AND abs(lng - ?) <= ? ORDER BY id ASC + * LIMIT 1`. `absDifference(platform, ref, value)` renders `ABS( - ?)` + * with `value` already bound inside the fragment (`sql-functions.ts`'s own + * docstring) — used as a filter KEY paired with `{ $lte: tolerance }`, so + * the emitted SQL and bind order match the legacy statement exactly: + * `ABS(lat - ?) <= ?` then `ABS(lng - ?) <= ?`, the SAME `tolerance` value + * on both. + */ + async findDuplicateByCoords(trip_id: string, lat: number, lng: number, tolerance: number): Promise<{ id: number; google_ftid: string | null } | undefined> { + const platform = this.getEntityManager().getPlatform(); + return this.qb('p') + .select(['p.id', 'p.google_ftid']) + .where('p.trip_id = ?', [trip_id]) + .andWhere({ + lat: { $ne: null }, + lng: { $ne: null }, + [absDifference(platform, 'p.lat', lat)]: { $lte: tolerance }, + [absDifference(platform, 'p.lng', lng)]: { $lte: tolerance }, + }) + .orderBy({ 'p.id': 'asc' }) + .limit(1) + .execute<{ id: number; google_ftid: string | null } | undefined>('get', false); + } + + /** + * PL29 (`exportGpx`'s waypoint read) — `SELECT p.name, p.description, + * p.address, p.lat, p.lng, p.route_geometry, c.name AS category FROM + * places p LEFT JOIN categories c ON c.id = p.category_id WHERE p.trip_id + * = ? ORDER BY p.id`. PL30 (the `days`/`day_assignments` itinerary half) + * is `DayAssignmentsRepository.listItineraryForGpx` — a different table + * root, kept there per the task-4 brief's own pointer. + */ + async listForGpx(trip_id: string): Promise<{ + name: string; description: string | null; address: string | null; + lat: number | null; lng: number | null; route_geometry: string | null; + category: string | null; + }[]> { + return this.qb('p') + .leftJoin('p.category', 'c') + .select(['p.name', 'p.description', 'p.address', 'p.lat', 'p.lng', 'p.route_geometry', 'c.name as category']) + .where('p.trip_id = ?', [trip_id]) + .orderBy({ 'p.id': 'asc' }) + .execute<{ name: string; description: string | null; address: string | null; lat: number | null; lng: number | null; route_geometry: string | null; category: string | null }[]>('all', false); + } + + /** + * PI1 (`place-image.ts:27`) — `SELECT 1 FROM places WHERE image_url = ? + * LIMIT 1`. Trip-agnostic on purpose (an uploaded image is ref-counted + * across every trip, not just the one it was uploaded from — a place + * copied or shared across trips can share the file). + */ + async existsByImageUrl(imageUrl: string): Promise { + const row = await this.qb('p') + .select(['p.id']) + .where({ image_url: imageUrl }) + .limit(1) + .execute<{ id: number } | undefined>('get', false); + return !!row; + } + + /** + * PL3 (`PlacesService.list`) — the trip's places, joined with their + * category's flat columns, filtered by up to four optional fragments + * built as conditional `andWhere`s rather than a hand-assembled SQL + * string (§18.3's surprise: the legacy statement's own text is not + * knowable from the source without tracing four `if`s — this makes each + * fragment a single, separately readable `andWhere` call instead). + * + * `filters.searchPattern` arrives PRE-BUILT (`%${escapeLikePattern(term)}%`, + * the service's job, matching every other "coercions stay in the service" + * split in this program) — this binds it verbatim into all three `LIKE … + * ESCAPE '\'` fragments, matching the legacy's one-param-three-times bind. + * The `ESCAPE '\'` clause is written into the raw fragment text because + * MikroORM's `$like` operator does not add one (§18.3). + * + * `filters.category`/`filters.tag` bind their raw string values with no + * `Number()` conversion (matching the legacy statement's own untyped + * bind); the two `assignment` fragments reference `place_tags`/ + * `day_assignments`/`days` by physical table name in a subquery (neither + * has, or needs, an entity-relation path for this shape) — the same + * raw-SQL-subquery shape `PlacesService.list`'s own legacy string used. + * + * `DISTINCT` kept via `.select([...], true)` even though a place has at + * most one category row (so it changes nothing about THIS join) — parity + * with the legacy statement's own `SELECT DISTINCT`, not a functional + * requirement. + */ + async listForTrip(trip_id: string, filters: { + searchPattern?: string; + category?: string; + tag?: string; + assignment?: 'all' | 'unassigned' | 'assigned'; + }): Promise { + const qb = this.qb('p') + .leftJoin('p.category', 'c') + .select(['p.*', 'c.name as category_name', 'c.color as category_color', 'c.icon as category_icon'], true) + .where('p.trip_id = ?', [trip_id]); + + if (filters.searchPattern) { + qb.andWhere( + "(p.name LIKE ? ESCAPE '\\' OR p.address LIKE ? ESCAPE '\\' OR p.description LIKE ? ESCAPE '\\')", + [filters.searchPattern, filters.searchPattern, filters.searchPattern], + ); + } + if (filters.category) { + qb.andWhere('p.category_id = ?', [filters.category]); + } + if (filters.tag) { + qb.andWhere('p.id IN (SELECT place_id FROM place_tags WHERE tag_id = ?)', [filters.tag]); + } + if (filters.assignment === 'unassigned') { + qb.andWhere( + 'p.id NOT IN (SELECT da.place_id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE d.trip_id = ?)', + [trip_id], + ); + } else if (filters.assignment === 'assigned') { + qb.andWhere( + 'p.id IN (SELECT da.place_id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE d.trip_id = ?)', + [trip_id], + ); + } + + qb.orderBy({ 'p.created_at': 'desc' }); + + return qb.execute('all', false); + } } diff --git a/server/src/db/repositories/Tags.repository.ts b/server/src/db/repositories/Tags.repository.ts index fa034635b..a7bfdcf5e 100644 --- a/server/src/db/repositories/Tags.repository.ts +++ b/server/src/db/repositories/Tags.repository.ts @@ -30,6 +30,22 @@ export interface TagForPlaceRow { created_at: string | null; } +/** + * `place_tags` (`place_id`, `tag_id`, `PRIMARY KEY (place_id, tag_id)`, + * `Migration20200101000000_baseline_schema.ts:195-199` — verified against + * the migration, not assumed) as `Places.entity.ts`'s own `place_tags` + * many-to-many relation declares it — a PLAIN pivot table + * (`p.manyToMany(Tags).pivotTable('place_tags')...`) with no owning entity of + * its own (unlike `PackingItems`' `packing_item_contributors`, which has a + * `pivotEntity`). Neither `em.upsert` nor `qb()` can target a table with no + * entity mapping — both are bound to exactly one mapped entity — so + * `TrekRepository.kysely()` (D3's sanctioned next escape hatch) is the only + * API that reaches it. + */ +interface PlaceTagsKyselyDB { + place_tags: { place_id: number; tag_id: number }; +} + export class TagsRepository extends TrekRepository { /** * `SELECT * FROM tags WHERE user_id = ? ORDER BY name ASC`, via the shared @@ -163,4 +179,63 @@ export class TagsRepository extends TrekRepository { .where({ 'p.id': { $in: placeIds } }) .execute('all', false); } + + // --------------------------------------------------------------------------- + // Plan 3c Task 4 (`PlacesService`) — additive methods. + // --------------------------------------------------------------------------- + + /** + * PL2 (`PlacesService.tagsOnTrip`) — `` SELECT id, user_id FROM tags WHERE + * id IN (${…}) ``, the ownership half of tag scoping: the caller + * (`tagsOnTrip`) intersects these rows' `user_id` against the trip's + * roster, and an id belonging to nobody on the roster drops silently. + * `$in` for the dynamic list (rule 17b). + */ + async findByIds(ids: number[]): Promise<{ id: number; user_id: number }[]> { + if (ids.length === 0) return []; + // `t.user` (unaliased, not separately joined) resolves to the physical + // `user_id` column and comes back keyed as `user_id` on the raw driver + // row — `TripsRepository.findAccessible`'s `.select(['t.id', 't.user', + // 't.currency'])` establishes the same shape. + return this.qb('t') + .select(['t.id', 't.user']) + .where({ id: { $in: ids } }) + .execute<{ id: number; user_id: number }[]>('all', false); + } + + // --------------------------------------------------------------------------- + // Plan 3c Task 4 (`PlacesService.create`/`applyUpdate`, PL5/PL12/PL13) — + // `place_tags` writes. `PlaceTagsKyselyDB` above documents why Kysely. + // --------------------------------------------------------------------------- + + /** + * PL5/PL13 — `INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES + * (?, ?)`, the legacy statement run once per tag id. One batched Kysely + * insert with `onConflict((oc) => oc.columns(['place_id', 'tag_id']) + * .doNothing())` on the pivot's real composite `PRIMARY KEY (place_id, + * tag_id)` — SQLite's `ON CONFLICT (...) DO NOTHING` (supported since + * SQLite 3.24, well within better-sqlite3's bundled version) is the + * upsert-API-less equivalent of `INSERT OR IGNORE` for a table with no + * entity mapping. Same end state as the legacy per-row loop (every + * genuinely new pair inserted, every duplicate silently skipped); the + * caller (`PlacesService.tagsOnTrip`) always hands an already + * roster-scoped, deduplicated tag id list — the empty-array guard here is + * defensive, matching every other batch-write method in this program. + */ + async insertIgnore(place_id: number, tag_ids: number[]): Promise { + if (tag_ids.length === 0) return; + await this.kysely() + .insertInto('place_tags') + .values(tag_ids.map((tag_id) => ({ place_id, tag_id }))) + .onConflict((oc) => oc.columns(['place_id', 'tag_id']).doNothing()) + .execute(); + } + + /** PL12 — `DELETE FROM place_tags WHERE place_id = ?` (the replace-all half of an update). */ + async deleteForPlace(place_id: number): Promise { + await this.kysely() + .deleteFrom('place_tags') + .where('place_id', '=', place_id) + .execute(); + } } diff --git a/server/src/nest/assignments/assignments.service.ts b/server/src/nest/assignments/assignments.service.ts index 005dc07fe..808034b14 100644 --- a/server/src/nest/assignments/assignments.service.ts +++ b/server/src/nest/assignments/assignments.service.ts @@ -65,21 +65,26 @@ function sortMinutes(time: string | null): number | null { * `AssignmentParticipantsRepository` (Plan 3c Task 3, consuming Task 2's * `findWithPlaceAndCategory`/`listForDay` projection unchanged for AS1/AS3); * AS20–AS23 (`roadtrip_vias`) stay raw on `DatabaseService`, `// ASn — Plan - * 3d` marked — that table belongs to Plan 3d. AS5 (`placeExists`) is the one - * exception to "SQL lives in a repository method": `Places.repository.ts` - * was another task's exclusive, actively-edited file this session (never - * touched here), so the equivalent one-line `qb()` raw-condition read is - * inlined below rather than added as a `PlacesRepository` method — flagged - * in the task report for a follow-up to move it once that file is free. + * 3d` marked — that table belongs to Plan 3d. AS5 (`placeExists`) called + * `PlacesRepository.existsInTrip` through Task 4's follow-up, once that file + * was free (it moved off an inlined `qb()` read this class carried for one + * session while `Places.repository.ts` was another task's exclusive file). * - * Assignment ids arriving from a route/tool are an **affinity seam** for the - * mutation methods (`toRowId(id)!`, program rule 15) — every real call site - * only ever reaches a mutation downstream of its own gate check in the same - * request (`assignmentExistsInDay`/`getAssignmentForTrip`, both of which - * stay on the legacy raw-bind seam themselves, matching `TripsRepository - * .findAccessible`'s documented `number | string` pass-through), so the - * non-null assertion is dead-code-safe, the same shape `DaysService` already - * uses for its own single `getDay` gate. + * **Assignment/day/place ids arriving from a route/tool are `toRowId`'d + * inside each existence gate, not passed through on the affinity seam** + * (Task 3 review H1, a live regression fixed by Task 4): `dayExists`/ + * `placeExists`/`assignmentExistsInDay`/`getAssignmentForTrip` used to bind + * the caller's raw id straight into their own read, which SQLite's + * text/integer affinity matches for a shape (`"3 "`, `"3.0"`, `"+3"`) + * `toRowId` rejects — so a gate could answer "found" for an id the mutation + * behind it, converting with its own `toRowId(id)!`, then treated as `null` + * (a 500 on `move`, a silent no-write "success" on `notes`/`delete`). Fixed + * the way `DaysService.getDay` (`days.service.ts:238-242`, the correct + * precedent — NOT the raw pass-through this docstring previously, + * incorrectly, cited it for) does it: `toRowId` runs INSIDE the gate, which + * returns not-found on `null`, so every mutation's own `toRowId(id)!` below + * can never disagree with the gate that authorized it — the non-null + * assertion is dead-code-safe for real, not just by convention. */ @Injectable() export class AssignmentsService { @@ -173,25 +178,39 @@ export class AssignmentsService { }); } - /** AS4 — `DaysRepository.existsInTrip`. Raw-bind pass-through, unchanged. */ + /** + * AS4 — `DaysRepository.existsInTrip`, `dayId` `toRowId`'d INSIDE this + * gate (Task 3 review H1 fix, absorbed in Task 4): `createAssignment` + * (the only write behind this gate) converts `dayId` with its own + * `toRowId(dayId)!` — a non-canonical id (`"3 "`, `"3.0"`) must answer + * "not found" HERE, not pass `DaysRepository.existsInTrip`'s own raw-bind + * affinity match and then hit `toRowId(dayId)!` downstream returning + * `null` (the live regression: a `day_id: null` INSERT, 500). `tripId` + * keeps the trip-scoping seam every gate in this cluster uses (D4's T5, + * `TripsRepository.findAccessible`'s precedent) — trip access is already + * verified upstream of every route this class serves, so `Number(tripId)` + * is the correct, unchanged shape (`DaysService.getDay`'s own precedent + * keeps its `tripId` unconverted too). + */ async dayExists(dayId: string | number, tripId: string | number) { - return await this.daysRepo.existsInTrip(dayId, tripId); + const id = toRowId(dayId); + if (id === null) return false; + return await this.daysRepo.existsInTrip(id, Number(tripId)); } /** - * AS5 — `SELECT id FROM places WHERE id = ? AND trip_id = ?`. `Places - * .repository.ts` was another task's exclusive, actively-edited file this - * session (see the class docstring) — inlined here as the one exception to - * "SQL lives in a repository method" rather than adding a method there. - * Same raw-bind seam as `dayExists`/`DayAssignmentsRepository.existsInDay`. + * AS5 — `PlacesRepository.existsInTrip`, same H1 fix and reasoning as + * `dayExists` above: `createAssignment`'s own `toRowId(placeId)!` must + * never disagree with this gate. `PlacesRepository.existsInTrip` itself + * narrowed its `id` parameter to `number` for this exact reason + * (`Places.repository.ts`'s own docstring) — Plan 3c Task 1 originally + * landed the method, Task 4 repointed this call site and applied the H1 + * fix in the same change. */ async placeExists(placeId: unknown, tripId: string | number) { - const row = await this.placesRepo - .qb('p') - .select(['p.id']) - .where('p.id = ? AND p.trip_id = ?', [placeId, tripId]) - .execute<{ id: number } | undefined>('get', false); - return !!row; + const id = toRowId(placeId); + if (id === null) return false; + return await this.placesRepo.existsInTrip(id, Number(tripId)); } /** @@ -233,9 +252,20 @@ export class AssignmentsService { return await this.getAssignmentWithPlace(insertedId); } - /** AS9 — `DayAssignmentsRepository.existsInDay`. Raw-bind pass-through, unchanged. */ + /** + * AS9 — `DayAssignmentsRepository.existsInDay`, `id` `toRowId`'d INSIDE + * this gate (Task 3 review H1 fix, absorbed in Task 4): `deleteAssignment` + * (the only write behind this gate, `AssignmentsController.remove`) converts + * `id` with its own `toRowId(id)!` — this must reject exactly what that + * rejects. `dayId`/`tripId` keep the legacy's own raw-bind scoping + * (`DayAssignmentsRepository.existsInDay`'s own `number | string` + * parameters, unchanged): `deleteAssignment` never reads either back, so + * there is no gate-vs-write id to disagree on for them. + */ async assignmentExistsInDay(id: string | number, dayId: string | number, tripId: string | number) { - return await this.dayAssignmentsRepo.existsInDay(id, dayId, tripId); + const rowId = toRowId(id); + if (rowId === null) return false; + return await this.dayAssignmentsRepo.existsInDay(rowId, dayId, tripId); } /** AS10 — `DayAssignmentsRepository.deleteById`. */ @@ -257,9 +287,20 @@ export class AssignmentsService { }); } - /** AS12 — `DayAssignmentsRepository.findInTrip`. Raw-bind pass-through, unchanged. */ + /** + * AS12 — `DayAssignmentsRepository.findInTrip`, `id` `toRowId`'d INSIDE + * this gate (Task 3 review H1 fix, absorbed in Task 4): every mutation + * behind this gate (`updateTime`, `setEndDay`, `updateNotes`, + * `setLegTransportMode`, `setIncomingLegTransportMode`, `setParticipants`, + * and `ItineraryRpc.unassign` via `moveAssignment`) converts `id` with its + * own `toRowId(id)!` — this must reject exactly what those reject, the + * same shape `assignmentExistsInDay` above uses for `deleteAssignment`. + * `tripId` keeps the legacy's own raw-bind scoping, unchanged. + */ async getAssignmentForTrip(id: string | number, tripId: string | number) { - return await this.dayAssignmentsRepo.findInTrip(id, tripId); + const rowId = toRowId(id); + if (rowId === null) return undefined; + return await this.dayAssignmentsRepo.findInTrip(rowId, tripId); } async moveAssignment(id: string | number, newDayId: unknown, orderIndex: number | null | undefined) { diff --git a/server/src/nest/places/place-image.ts b/server/src/nest/places/place-image.ts index d5b58bd90..1c46ff73a 100644 --- a/server/src/nest/places/place-image.ts +++ b/server/src/nest/places/place-image.ts @@ -20,6 +20,23 @@ export function isUploadedPlaceImage(url: string | null | undefined): url is str * 'places' category, mirroring tripService.deleteOldCover. Best-effort: never * throws (central key validation rejects a hostile stored value; the catch * swallows it exactly like the old unlink guard). + * + * Plan 3c Task 4: `PlacesService` no longer calls this free function — it has + * its own converted private `reclaimPlaceImage` method (`places.service.ts`), + * which does the same job through `PlacesRepository.existsByImageUrl` for + * PI1 and one raw `// Plan 3h` statement for PI2. This copy, and the raw `db` + * proxy import it still needs, stay here UNCHANGED because + * `collections.service.ts` (`src/nest/collections/`, Plan 3h's file — the + * `collection_places` table is Plan 3h's) also imports and calls this exact + * function on four of its own call sites, verified by grep + * (`grep -rn "from '.*place-image'" server/src`) — contradicting the + * inventory's stated "every caller is this service" premise + * (`plan3c-sql-inventory.md` §8d). Converting that caller means wiring a + * `PlacesRepository` injection into `CollectionsModule`, a change to an + * unowned domain's DI graph out of this task's file ownership and test + * budget — flagged for whoever next owns `src/nest/collections/**` (Plan 3h) + * to finish, the same `// Plan 3h` carve-out pattern PI2/PP6 already use in + * this cluster. */ export async function reclaimPlaceImage(storage: StorageService, url: string | null | undefined): Promise { if (!isUploadedPlaceImage(url)) return; diff --git a/server/src/nest/places/places.mcp.ts b/server/src/nest/places/places.mcp.ts index 35fcecf2f..78ef9f5b4 100644 --- a/server/src/nest/places/places.mcp.ts +++ b/server/src/nest/places/places.mcp.ts @@ -4,6 +4,7 @@ import { TOOL_ANNOTATIONS_DELETE, TOOL_ANNOTATIONS_NON_IDEMPOTENT, demoDenied, errorResult, ok, } from '../../nest-mcp'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service'; import { mapsSearchRequestSchema, @@ -20,11 +21,12 @@ import { AuthService } from '../auth/auth.service'; import { AssignmentsService } from '../assignments/assignments.service'; import { JourneyDomainService } from '../journey/journey-domain.service'; import { noAccess, permissionDenied } from '../../mcp/tools/_shared'; -import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; import { MapsService } from '../maps/maps.service'; import { PlacesService } from './places.service'; import { isDirectionsUrl } from './maps-dir.helpers'; +import { Trips } from '../../db/entities/Trips.entity'; +import type { TripsRepository } from '../../db/repositories/Trips.repository'; function parseId(value: string | string[]): number | null { const n = Number(Array.isArray(value) ? value[0] : value); @@ -53,7 +55,7 @@ export class PlacesMcp { constructor( private readonly places: PlacesService, private readonly maps: MapsService, - private readonly db: DatabaseService, + @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, private readonly auth: AuthService, private readonly journey: JourneyDomainService, private readonly assignments: AssignmentsService, @@ -97,7 +99,7 @@ export class PlacesMcp { ctx: McpContext, ) { if (await this.auth.isDemoUser(ctx.userId)) return demoDenied(); - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('place_edit', tripId, ctx.userId))) return permissionDenied(); const place = await this.places.create(String(tripId), { name, description, lat, lng, address, category_id, google_place_id, google_ftid, osm_id, amap_poi_id, notes, website, phone, image_url, price, currency, stop_type }); this.guards.safeBroadcast(tripId, 'place:created', { place }); @@ -142,7 +144,7 @@ export class PlacesMcp { ctx: McpContext, ) { if (await this.auth.isDemoUser(ctx.userId)) return demoDenied(); - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('place_edit', tripId, ctx.userId))) return permissionDenied(); if (!(await this.assignments.dayExists(dayId, tripId))) return { content: [{ type: 'text' as const, text: 'Day not found.' }], isError: true }; try { @@ -205,7 +207,7 @@ export class PlacesMcp { ctx: McpContext, ) { if (await this.auth.isDemoUser(ctx.userId)) return demoDenied(); - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('place_edit', tripId, ctx.userId))) return permissionDenied(); const place = await this.places.update(String(tripId), String(placeId), { name, description, lat, lng, address, category_id, price, currency, place_time, end_time, duration_minutes, notes, website, phone, image_url, transport_mode, osm_id, google_place_id, google_ftid, amap_poi_id, stop_type, fill_percent }); if (!place) return { content: [{ type: 'text' as const, text: 'Place not found.' }], isError: true }; @@ -230,7 +232,7 @@ export class PlacesMcp { ) { if (await this.auth.isDemoUser(ctx.userId)) return demoDenied(); // Rating is a personal vote — any trip member may cast one, place_edit not required. - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); const place = await this.places.rate(String(tripId), String(placeId), ctx.userId, rating ?? null); if (!place) return { content: [{ type: 'text' as const, text: 'Place not found.' }], isError: true }; this.guards.safeBroadcast(tripId, 'place:updated', { place }); @@ -249,7 +251,7 @@ export class PlacesMcp { }) async deletePlace({ tripId, placeId }: { tripId: number; placeId: number }, ctx: McpContext) { if (await this.auth.isDemoUser(ctx.userId)) return demoDenied(); - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('place_edit', tripId, ctx.userId))) return permissionDenied(); // Scope the id to the trip before the hook: onPlaceDeleted keys on the place // id alone, so a foreign id would detach that trip's journey entries even @@ -291,7 +293,7 @@ export class PlacesMcp { }, ctx: McpContext, ) { - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); const places = await this.places.list(String(tripId), { search, category, tag, assignment }); return ok({ places }); } @@ -343,7 +345,7 @@ export class PlacesMcp { ctx: McpContext, ) { if (await this.auth.isDemoUser(ctx.userId)) return demoDenied(); - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('place_edit', tripId, ctx.userId))) return permissionDenied(); // Same opts the REST route builds: the enrichment pass is keyed on the calling @@ -374,7 +376,7 @@ export class PlacesMcp { }) async importGpx(input: RoadtripGpxImport, ctx: McpContext) { if (await this.auth.isDemoUser(ctx.userId)) return demoDenied(); - if (!(await this.db.canAccessTrip(input.tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(input.tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('place_edit', input.tripId, ctx.userId))) return permissionDenied(); if (!input.importWaypoints && !input.importRoutes && !input.importTracks) return errorResult('No import types selected.'); try { @@ -406,7 +408,7 @@ export class PlacesMcp { ctx: McpContext, ) { // A read, like the REST route: seeing the trip is enough, no place_edit. - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!waypoints && !tracks && !dayRoutes) return errorResult('No export types selected.'); const result = await this.places.exportGpx(String(tripId), { waypoints, tracks, dayRoutes }); if (!result) return errorResult('Nothing to export.'); @@ -425,7 +427,7 @@ export class PlacesMcp { }) async bulkDeletePlaces({ tripId, placeIds }: { tripId: number; placeIds: number[] }, ctx: McpContext) { if (await this.auth.isDemoUser(ctx.userId)) return demoDenied(); - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('place_edit', tripId, ctx.userId))) return permissionDenied(); // Trip-scoped, and ahead of the DELETE: journey_entries.source_place_id is @@ -482,7 +484,7 @@ export class PlacesMcp { ctx: McpContext, ) { if (await this.auth.isDemoUser(ctx.userId)) return demoDenied(); - if (!(await this.db.canAccessTrip(tripId, ctx.userId))) return noAccess(); + if (!(await this.tripsRepo.findAccessible(tripId, ctx.userId))) return noAccess(); if (!(await this.guards.hasTripPermission('place_edit', tripId, ctx.userId))) return permissionDenied(); const fields = { category_id, price, currency, transport_mode, place_time, end_time, duration_minutes, notes, website, phone, image_url, description, stop_type, fill_percent }; @@ -504,7 +506,7 @@ export class PlacesMcp { }) async tripPlacesResource(uri: URL, { tripId }: { tripId: string | string[] }, ctx: McpContext) { const id = parseId(tripId); - if (id === null || !(await this.db.canAccessTrip(id, ctx.userId))) { + if (id === null || !(await this.tripsRepo.findAccessible(id, ctx.userId))) { return { contents: [{ uri: uri.href, diff --git a/server/src/nest/places/places.module.ts b/server/src/nest/places/places.module.ts index 85980dc6d..62762b821 100644 --- a/server/src/nest/places/places.module.ts +++ b/server/src/nest/places/places.module.ts @@ -1,4 +1,11 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; +import { Places } from '../../db/entities/Places.entity'; +import { Tags } from '../../db/entities/Tags.entity'; +import { PlaceRatings } from '../../db/entities/PlaceRatings.entity'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; +import { DayAssignments } from '../../db/entities/DayAssignments.entity'; +import { Trips } from '../../db/entities/Trips.entity'; import { JourneyDomainModule } from '../journey/journey-domain.module'; import { PlacesController } from './places.controller'; import { PlacesService } from './places.service'; @@ -45,7 +52,17 @@ import { MAX_PLACE_IMAGE_SIZE } from '../common/place-image-upload'; StorageModule, // AccommodationsDomainModule: deleting a place takes the nights booked at it with // it, and the cascade behind a stay belongs to the domain that owns it. - McpSharedModule, PermissionsModule, QueryHelpersModule, MapsModule, AuthModule, AppConfigModule, UnsplashModule, PlacePhotosModule, JourneyDomainModule, RealtimeModule, PluginGuardsModule, AssignmentsDomainModule, AccommodationsDomainModule], + McpSharedModule, PermissionsModule, QueryHelpersModule, MapsModule, AuthModule, AppConfigModule, UnsplashModule, PlacePhotosModule, JourneyDomainModule, RealtimeModule, PluginGuardsModule, AssignmentsDomainModule, AccommodationsDomainModule, + // Plan 3c Task 4: `PlacesService`'s and `PlacesMcp`'s own `@InjectRepository` + // constructor params (`PlacesRepository`, `TagsRepository`, + // `PlaceRatingsRepository`, `TripMembersRepository`, + // `DayAssignmentsRepository`, `TripsRepository`) — registered directly on + // THIS module (not a sibling domain module's `exports`), matching + // `AssignmentsDomainModule`'s own `MikroOrmModule.forFeature(...)` + // precedent: a `forFeature` registration only reaches providers declared + // in the SAME module. + MikroOrmModule.forFeature([Places, Tags, PlaceRatings, TripMembers, DayAssignments, Trips]), + ], controllers: [PlacesController], providers: [PlacesService, PlacesMcp, PlacesRpc], exports: [PlacesService], diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index e66315784..cbc2a5144 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -1,4 +1,6 @@ +import path from 'node:path'; import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { XMLValidator } from 'fast-xml-parser'; import { TRACK_COLORS, placeMatchStrategies, type PlaceMatchCandidate } from '@trek/shared'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; @@ -9,7 +11,8 @@ import type { PlaceWithTags } from '../database/database.service'; import { PermissionsService } from '../permissions/permissions.service'; import { MapsService, GOOGLE_SHORT_HOSTS, isGoogleMapsHost } from '../maps/maps.service'; import { isDirectionsUrl, parseDirectionsUrl } from './maps-dir.helpers'; -import type { Place, User } from '../../types'; +import { toRowId } from '../common/row-id'; +import type { User } from '../../types'; import { QueryHelpersService } from '../query-helpers/query-helpers.service'; import { ratingAggregate } from '../common/rowShape'; import { checkSsrf, safeFetchFollow, SsrfBlockedError } from '../../utils/ssrfGuard'; @@ -26,10 +29,20 @@ import type { GpxExportDay, GpxExportOptions, GpxExportPlace } from './gpx-expor import { UnsplashService } from '../unsplash/unsplash.service'; import { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service'; import { type UpdateConflict, isUpdateConflict } from '../common/conflictResult'; -import { reclaimPlaceImage } from './place-image'; +import { isUploadedPlaceImage } from './place-image'; import { JourneyDomainService } from '../journey/journey-domain.service'; import { StorageService } from '../storage/storage.service'; import { AccommodationsService } from '../accommodations/accommodations.service'; +import { Places } from '../../db/entities/Places.entity'; +import { Tags } from '../../db/entities/Tags.entity'; +import { PlaceRatings } from '../../db/entities/PlaceRatings.entity'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; +import { DayAssignments } from '../../db/entities/DayAssignments.entity'; +import type { PlacesRepository, PlaceWithCategoryRow } from '../../db/repositories/Places.repository'; +import type { TagsRepository } from '../../db/repositories/Tags.repository'; +import type { PlaceRatingsRepository } from '../../db/repositories/PlaceRatings.repository'; +import type { TripMembersRepository } from '../../db/repositories/TripMembers.repository'; +import type { DayAssignmentsRepository } from '../../db/repositories/DayAssignments.repository'; /** Rows a place delete took down with the nights booked there, for the caller to announce. */ export interface CancelledStays { @@ -64,7 +77,6 @@ import { type ListImportOptions, type ListImportResult, type PlaceImportResult, - type PlaceWithCategory, } from './places.helpers'; type Trip = TripAccess; @@ -135,6 +147,11 @@ export class PlacesService { private readonly storage: StorageService, private readonly accommodations: AccommodationsService, private readonly uow: UnitOfWork, + @InjectRepository(Places) private readonly placesRepo: PlacesRepository, + @InjectRepository(Tags) private readonly tagsRepo: TagsRepository, + @InjectRepository(PlaceRatings) private readonly placeRatingsRepo: PlaceRatingsRepository, + @InjectRepository(TripMembers) private readonly tripMembersRepo: TripMembersRepository, + @InjectRepository(DayAssignments) private readonly dayAssignmentsRepo: DayAssignmentsRepository, ) {} async verifyTripAccess(tripId: string, userId: number) { @@ -160,11 +177,13 @@ export class PlacesService { private async tagsOnTrip(tripId: string | number, tagIds: number[]): Promise { const unique = [...new Set(tagIds)]; if (unique.length === 0) return []; - const roster = await this.dbs.rosterUserIds(tripId); - const owned = this.dbs.all<{ id: number; user_id: number }>( - `SELECT id, user_id FROM tags WHERE id IN (${unique.map(() => '?').join(',')})`, - ...unique, - ); + // PL1 — `TripMembersRepository.rosterUserIds`, injected directly (not + // through `DatabaseService`), matching `AssignmentsService`'s AS28 + // precedent: each domain converts its own `DatabaseService`-delegated + // callers as it lands. + const roster = await this.tripMembersRepo.rosterUserIds(tripId); + // PL2 — `SELECT id, user_id FROM tags WHERE id IN (${…})`. + const owned = await this.tagsRepo.findByIds(unique); return owned.filter(t => roster.has(t.user_id)).map(t => t.id); } @@ -172,6 +191,37 @@ export class PlacesService { this.realtime.broadcast(tripId, event, payload, socketId); } + /** + * Delete a custom place-image object once nothing references it any more + * (moved in from `place-image.ts`'s free function `reclaimPlaceImage`, + * option (a) — see `place-image.ts`'s own docstring for why that free + * function stays, unconverted, for `collections.service.ts`'s own call + * sites). A trip place and a collection saved-place can share the same + * uploaded file — save-to-collection and copy-to-trip copy `image_url` by + * reference — so this ref-counts across both tables before deleting. + * + * PI1 (`SELECT 1 FROM places WHERE image_url = ? LIMIT 1`) through + * `PlacesRepository.existsByImageUrl`; PI2 (`SELECT 1 FROM collection_places + * WHERE image_url = ? LIMIT 1`, Plan 3h's table) stays one raw statement, + * evaluated only when PI1 is false — reproducing the legacy `UNION ALL … + * LIMIT 1`'s short-circuit by evaluation order, the same pattern + * `PlacePhotoCacheService.isReferenced` uses for the same two tables + * (Plan 3c Task 1's PP6 ruling). `path.basename()` keeps the storage name + * confined to the 'places' category. Best-effort: never throws (central + * key validation rejects a hostile stored value; the catch swallows it + * exactly like the legacy unlink guard). + */ + private async reclaimPlaceImage(url: string | null | undefined): Promise { + if (!isUploadedPlaceImage(url)) return; + if (await this.placesRepo.existsByImageUrl(url)) return; + // Plan 3h: collection_places is not yet repository-backed. + const referenced = this.dbs.get('SELECT 1 FROM collection_places WHERE image_url = ? LIMIT 1', url); + if (referenced) return; + await this.storage.delete('places', path.basename(url)).catch(() => { + /* best-effort */ + }); + } + // ------------------------------------------------------------------------- // List places // ------------------------------------------------------------------------- @@ -180,43 +230,17 @@ export class PlacesService { tripId: string, filters: { search?: string; category?: string; tag?: string; assignment?: 'all' | 'unassigned' | 'assigned' }, ) { - let query = ` - SELECT DISTINCT p.*, c.name as category_name, c.color as category_color, c.icon as category_icon - FROM places p - LEFT JOIN categories c ON p.category_id = c.id - WHERE p.trip_id = ? - `; - const params: (string | number)[] = [tripId]; - - if (filters.search) { - // ESCAPE so a `%` or `_` the user typed matches literally instead of - // acting as a LIKE wildcard (a bare '%' used to return the whole trip). - query += " AND (p.name LIKE ? ESCAPE '\\' OR p.address LIKE ? ESCAPE '\\' OR p.description LIKE ? ESCAPE '\\')"; - const searchParam = `%${escapeLikePattern(filters.search)}%`; - params.push(searchParam, searchParam, searchParam); - } - - if (filters.category) { - query += ' AND p.category_id = ?'; - params.push(filters.category); - } - - if (filters.tag) { - query += ' AND p.id IN (SELECT place_id FROM place_tags WHERE tag_id = ?)'; - params.push(filters.tag); - } - - if (filters.assignment === 'unassigned') { - query += ` AND p.id NOT IN (SELECT da.place_id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE d.trip_id = ?)`; - params.push(tripId); - } else if (filters.assignment === 'assigned') { - query += ` AND p.id IN (SELECT da.place_id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE d.trip_id = ?)`; - params.push(tripId); - } - - query += ' ORDER BY p.created_at DESC'; - - const places = this.dbs.prepare(query).all(...params) as PlaceWithCategory[]; + // ESCAPE so a `%` or `_` the user typed matches literally instead of + // acting as a LIKE wildcard (a bare '%' used to return the whole trip) — + // the coercion stays here (the service), the repository binds the + // already-built pattern verbatim (PL3). + const searchPattern = filters.search ? `%${escapeLikePattern(filters.search)}%` : undefined; + const places: PlaceWithCategoryRow[] = await this.placesRepo.listForTrip(tripId, { + searchPattern, + category: filters.category, + tag: filters.tag, + assignment: filters.assignment, + }); const placeIds = places.map(p => p.id); const tagsByPlaceId = await this.queryHelpers.loadTagsByPlaceIds(placeIds); @@ -248,42 +272,51 @@ export class PlacesService { transport_mode, route_geometry, route_color, stop_type, fill_percent, tags = [], } = body; - const result = this.dbs.run(` - INSERT INTO places (trip_id, name, description, lat, lng, address, category_id, price, currency, - place_time, end_time, - duration_minutes, notes, image_url, google_place_id, google_ftid, osm_id, amap_poi_id, website, phone, transport_mode, - route_geometry, route_color, stop_type, fill_percent) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) - `, - // lat/lng/price/duration_minutes use an explicit undefined check, not `||`: - // 0 is a legitimate value for all four (Null Island, a free entry, a - // drive-by stop) and the falsy coercion silently threw it away. - // - // The hour stays the default for a place created without one, which is what it - // has always been and what the column itself declares. The road trip rail reads - // the value as a stay, and a stop added from the corridor search brings its own - // figure — ten minutes for fuel, twenty for a rest area — so the kinds that would - // be misread as an hour never take the default in the first place. - tripId, name, description || null, lat ?? null, lng ?? null, address || null, - category_id || null, price ?? null, currency || null, - place_time || null, end_time || null, duration_minutes ?? 60, notes || null, image_url || null, - google_place_id || null, google_ftid || null, osm_id || null, amap_poi_id || null, website || null, phone || null, transport_mode || 'walking', - route_geometry || null, route_color || null, stop_type || null, - // `?? null` rather than `|| null`, the same reason lat/lng have it: the column is a - // percentage and the falsy check would be a silent floor. - fill_percent ?? null, - ); - - const placeId = result.lastInsertRowid; + // PL4 — the 25-column INSERT. lat/lng/price/duration_minutes/fill_percent + // use an explicit undefined check, not `||`: 0 is a legitimate value for + // all five (Null Island, a free entry, a drive-by stop, an empty tank) + // and the falsy coercion silently threw it away. + // + // The hour stays the default for a place created without one, which is what it + // has always been and what the column itself declares. The road trip rail reads + // the value as a stay, and a stop added from the corridor search brings its own + // figure — ten minutes for fuel, twenty for a rest area — so the kinds that would + // be misread as an hour never take the default in the first place. + const placeId = await this.placesRepo.insertPlace({ + trip_id: Number(tripId), + name, + description: description || null, + lat: lat ?? null, + lng: lng ?? null, + address: address || null, + category_id: category_id || null, + price: price ?? null, + currency: currency || null, + place_time: place_time || null, + end_time: end_time || null, + duration_minutes: duration_minutes ?? 60, + notes: notes || null, + image_url: image_url || null, + google_place_id: google_place_id || null, + google_ftid: google_ftid || null, + osm_id: osm_id || null, + amap_poi_id: amap_poi_id || null, + website: website || null, + phone: phone || null, + transport_mode: transport_mode || 'walking', + route_geometry: route_geometry || null, + route_color: route_color || null, + stop_type: stop_type || null, + fill_percent: fill_percent ?? null, + }); + // PL5 — `INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)`. if (tags && tags.length > 0) { - const insertTag = this.dbs.prepare('INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)'); - for (const tagId of await this.tagsOnTrip(tripId, tags)) { - insertTag.run(placeId, tagId); - } + await this.tagsRepo.insertIgnore(placeId, await this.tagsOnTrip(tripId, tags)); } - return (await this.dbs.getPlaceWithTags(Number(placeId)))!; + // PL6 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. + return (await this.placesRepo.findWithTagsAndRatings(placeId))!; } // ------------------------------------------------------------------------- @@ -291,9 +324,16 @@ export class PlacesService { // ------------------------------------------------------------------------- async get(tripId: string, placeId: string) { - const placeCheck = this.dbs.get('SELECT id FROM places WHERE id = ? AND trip_id = ?', placeId, tripId); - if (!placeCheck) return null; - return await this.dbs.getPlaceWithTags(placeId); + // PL7 — the existence gate. `toRowId` first (Task 3 review H1, absorbed + // here): a non-canonical id must resolve to "not found" here, the same + // answer `findWithTagsAndRatings` below would give it anyway — this + // just skips the trip-scoped existence read for an id that can never + // exist. + const id = toRowId(placeId); + if (id === null) return null; + if (!(await this.placesRepo.existsInTrip(id, Number(tripId)))) return null; + // PL8 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. + return await this.placesRepo.findWithTagsAndRatings(id); } // ------------------------------------------------------------------------- @@ -307,7 +347,7 @@ export class PlacesService { ifMatch?: string, ): Promise { const { result, reclaim } = await this.applyUpdate(tripId, placeId, body, ifMatch); - if (reclaim !== undefined) await reclaimPlaceImage(this.storage, reclaim); + if (reclaim !== undefined) await this.reclaimPlaceImage(reclaim); return result; } @@ -322,14 +362,24 @@ export class PlacesService { body: PlaceUpdateInput, ifMatch?: string, ): Promise<{ result: PlaceWithTags | UpdateConflict | null; reclaim?: string | null }> { - const existingPlace = this.dbs.get('SELECT * FROM places WHERE id = ? AND trip_id = ?', placeId, tripId); + // PL9 — the pre-image read every `!== undefined` fallback below needs. + // `toRowId` first (Task 3 review H1, absorbed here): every write in + // this method (`updatePlace`, `tagsRepo.deleteForPlace`/`insertIgnore`) + // uses this SAME validated `id` — a non-canonical `placeId` (`"3 "`, + // `"3.0"`) must resolve to "not found" HERE, not pass a loose SQLite + // affinity match and then hit a downstream write keyed on a `toRowId(x)!` + // that returns `null`. + const id = toRowId(placeId); + if (id === null) return { result: null }; + const existingPlace = await this.placesRepo.findInTrip(id, Number(tripId)); if (!existingPlace) return { result: null }; // Optimistic concurrency (#1135): when the caller sent the version it based its // edit on and the row has moved on since, reject instead of clobbering. Absent // token => unconditional update (back-compat — old clients keep last-write-wins). if (ifMatch !== undefined && existingPlace.updated_at != null && String(existingPlace.updated_at) !== ifMatch) { - return { result: { conflict: true, server: await this.dbs.getPlaceWithTags(placeId) } }; + // PL10 — the `{conflict: true, server}` body when `If-Match` loses (#1135). + return { result: { conflict: true, server: await this.placesRepo.findWithTagsAndRatings(id) } }; } const { @@ -339,75 +389,58 @@ export class PlacesService { transport_mode, route_color, stop_type, fill_percent, tags, } = body; - this.dbs.run(` - UPDATE places SET - name = COALESCE(?, name), - description = ?, - lat = ?, - lng = ?, - address = ?, - category_id = ?, - price = ?, - currency = COALESCE(?, currency), - place_time = ?, - end_time = ?, - duration_minutes = ?, - notes = ?, - image_url = ?, - google_place_id = ?, - google_ftid = ?, - osm_id = ?, - amap_poi_id = ?, - website = ?, - phone = ?, - transport_mode = COALESCE(?, transport_mode), - route_color = ?, - stop_type = ?, - fill_percent = ?, - updated_at = CURRENT_TIMESTAMP - WHERE id = ? - `, - name || null, - description !== undefined ? description : existingPlace.description, - lat !== undefined ? lat : existingPlace.lat, - lng !== undefined ? lng : existingPlace.lng, - address !== undefined ? address : existingPlace.address, - category_id !== undefined ? category_id : existingPlace.category_id, - price !== undefined ? price : existingPlace.price, - currency || null, - place_time !== undefined ? place_time : existingPlace.place_time, - end_time !== undefined ? end_time : existingPlace.end_time, + // PL11 — ONE `nativeUpdate` with a typed full partial (the ruling): the + // SQL `COALESCE(?, col)` keep-if-null semantics (name/currency/ + // transport_mode, all three bound `x || null`) and the twenty + // `!== undefined ? x : existing.x` pre-image fallbacks are resolved to + // their final value HERE, in the service — the repository writes + // exactly what it is handed. + await this.placesRepo.updatePlace(id, { + // COALESCE(?, name): the bound value is `name || null` (falsy clears + // to null), and `?? existingPlace.name` is the COALESCE half itself — + // a `nativeUpdate` writes a literal value, it does not evaluate SQL + // COALESCE, so the keep-if-null fold has to be resolved here. + name: (name || null) ?? existingPlace.name, + description: description !== undefined ? description : existingPlace.description, + lat: lat !== undefined ? lat : existingPlace.lat, + lng: lng !== undefined ? lng : existingPlace.lng, + address: address !== undefined ? address : existingPlace.address, + category_id: category_id !== undefined ? category_id : existingPlace.category_id, + price: price !== undefined ? price : existingPlace.price, + // COALESCE(?, currency) — same fold as `name` above. + currency: (currency || null) ?? existingPlace.currency, + place_time: place_time !== undefined ? place_time : existingPlace.place_time, + end_time: end_time !== undefined ? end_time : existingPlace.end_time, // Not COALESCE, like its neighbours: the contract says an explicit null // clears the planned stay length, and COALESCE made null and absent the // same thing, so the field advertised a reset it never performed. 0 keeps // working, which a `|| null` bind would have swallowed. - duration_minutes !== undefined ? duration_minutes : existingPlace.duration_minutes, - notes !== undefined ? notes : existingPlace.notes, - image_url !== undefined ? image_url : existingPlace.image_url, - google_place_id !== undefined ? google_place_id : existingPlace.google_place_id, - google_ftid !== undefined ? google_ftid : existingPlace.google_ftid, - osm_id !== undefined ? osm_id : existingPlace.osm_id, - amap_poi_id !== undefined ? amap_poi_id : existingPlace.amap_poi_id, - website !== undefined ? website : existingPlace.website, - phone !== undefined ? phone : existingPlace.phone, - transport_mode || null, + duration_minutes: duration_minutes !== undefined ? duration_minutes : existingPlace.duration_minutes, + notes: notes !== undefined ? notes : existingPlace.notes, + image_url: image_url !== undefined ? image_url : existingPlace.image_url, + google_place_id: google_place_id !== undefined ? google_place_id : existingPlace.google_place_id, + google_ftid: google_ftid !== undefined ? google_ftid : existingPlace.google_ftid, + osm_id: osm_id !== undefined ? osm_id : existingPlace.osm_id, + amap_poi_id: amap_poi_id !== undefined ? amap_poi_id : existingPlace.amap_poi_id, + website: website !== undefined ? website : existingPlace.website, + phone: phone !== undefined ? phone : existingPlace.phone, + // COALESCE(?, transport_mode) — same fold as `name`/`currency` above. + transport_mode: (transport_mode || null) ?? existingPlace.transport_mode, // Deliberately not COALESCE: an explicit null is how the picker resets a // track back to its category colour (#776). - route_color !== undefined ? route_color : existingPlace.route_color, + route_color: route_color !== undefined ? route_color : existingPlace.route_color, // Same shape: an explicit null is how a fuel stop becomes an ordinary place again. - stop_type !== undefined ? stop_type : existingPlace.stop_type, + stop_type: stop_type !== undefined ? stop_type : existingPlace.stop_type, // And how a stop that had its own fill amount goes back to following the setting. - fill_percent !== undefined ? fill_percent : existingPlace.fill_percent, - placeId, - ); + fill_percent: fill_percent !== undefined ? fill_percent : existingPlace.fill_percent, + }); if (tags !== undefined) { - this.dbs.run('DELETE FROM place_tags WHERE place_id = ?', placeId); + // PL12 — `DELETE FROM place_tags WHERE place_id = ?`. + await this.tagsRepo.deleteForPlace(id); if (tags.length > 0) { - const insertTag = this.dbs.prepare('INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)'); - for (const tagId of await this.tagsOnTrip(tripId, tags)) { - insertTag.run(placeId, tagId); - } + // PL13 — `INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)`. + await this.tagsRepo.insertIgnore(id, await this.tagsOnTrip(tripId, tags)); } } @@ -418,7 +451,8 @@ export class PlacesService { ? existingPlace.image_url : undefined; - return { result: await this.dbs.getPlaceWithTags(placeId), reclaim }; + // PL14 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. + return { result: await this.placesRepo.findWithTagsAndRatings(id), reclaim }; } // ------------------------------------------------------------------------- @@ -432,6 +466,7 @@ export class PlacesService { */ async linkedExpenseIds(tripId: string | number, placeIds: Array): Promise { if (placeIds.length === 0) return []; + // PL15 — `budget_items` is Plan 3e's table; stays raw. const rows = this.dbs.all<{ id: number }>( `SELECT id FROM budget_items WHERE trip_id = ? AND place_id IN (${placeIds.map(() => '?').join(',')})`, tripId, ...placeIds, @@ -452,6 +487,9 @@ 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, ); @@ -467,39 +505,47 @@ export class PlacesService { } async remove(tripId: string, placeId: string): Promise<{ deleted: boolean; cancelled: CancelledStays }> { - const place = this.dbs.get<{ google_place_id: string | null; image_url: string | null }>( - 'SELECT google_place_id, image_url FROM places WHERE id = ? AND trip_id = ?', placeId, tripId, - ); const cancelled = noCancelledStays(); + // `toRowId` first (Task 3 review H1, absorbed here): the write below + // (`deleteById`) must use the SAME id this gate's existence read used. + const id = toRowId(placeId); + if (id === null) return { deleted: false, cancelled }; + // PL17 — the reclaim-candidate projection, read before the delete. + const place = await this.placesRepo.reclaimInputs(id, Number(tripId)); if (!place) return { deleted: false, cancelled }; // The linked expense goes with the place, the same way a booking takes its // expense with it (#1298). One transaction, so a place can never survive // half-detached from its money. await this.uow.transactional(async () => { - await this.cancelStaysAt(tripId, placeId, cancelled); - this.dbs.run('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?', tripId, placeId); - this.dbs.run('DELETE FROM places WHERE id = ?', placeId); + await this.cancelStaysAt(tripId, id, cancelled); + // PL18 — `budget_items` is Plan 3e's table; stays raw. + this.dbs.run('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?', tripId, id); + // PL19 — `DELETE FROM places WHERE id = ?`. + await this.placesRepo.deleteById(id); }); await reclaimPhotoCache(this.photoCache, place.google_place_id, place.image_url); - await reclaimPlaceImage(this.storage, place.image_url); + await this.reclaimPlaceImage(place.image_url); return { deleted: true, cancelled }; } async removeMany(tripId: string, ids: number[]): Promise<{ deleted: number[]; cancelled: CancelledStays }> { const cancelled = noCancelledStays(); if (ids.length === 0) return { deleted: [], cancelled }; - const selectStmt = this.dbs.prepare('SELECT google_place_id, image_url FROM places WHERE id = ? AND trip_id = ?'); - const deleteStmt = this.dbs.prepare('DELETE FROM places WHERE id = ?'); - const deleteExpenseStmt = this.dbs.prepare('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?'); const deleted: number[] = []; const reclaimable: { google_place_id: string | null; image_url: string | null }[] = []; await this.uow.transactional(async () => { for (const id of ids) { - const row = selectStmt.get(id, tripId) as { google_place_id: string | null; image_url: string | null } | undefined; + // PL20 — the reclaim-candidate projection, read before the delete + // (same statement as PL17). `id` is already a genuine `number` + // here (a body-validated array, not a route string) — no `toRowId` + // gate needed, the H1 class of bug is a route-string problem. + const row = await this.placesRepo.reclaimInputs(id, Number(tripId)); if (!row) continue; await this.cancelStaysAt(tripId, id, cancelled); - deleteExpenseStmt.run(tripId, id); - deleteStmt.run(id); + // PL22 — `budget_items` is Plan 3e's table; stays raw. + this.dbs.run('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?', tripId, id); + // PL21 — `DELETE FROM places WHERE id = ?`. + await this.placesRepo.deleteById(id); deleted.push(id); reclaimable.push(row); } @@ -507,7 +553,7 @@ export class PlacesService { // Reclaim after the transaction commits so isReferenced() sees the final place set. for (const row of reclaimable) { await reclaimPhotoCache(this.photoCache, row.google_place_id, row.image_url); - await reclaimPlaceImage(this.storage, row.image_url); + await this.reclaimPlaceImage(row.image_url); } return { deleted, cancelled }; } @@ -519,13 +565,8 @@ export class PlacesService { * that trip's journey entries even though the delete itself refuses it. */ async scopedIds(tripId: string, ids: number[]): Promise { - if (ids.length === 0) return []; - const placeholders = ids.map(() => '?').join(','); - const rows = this.dbs.all<{ id: number }>( - `SELECT id FROM places WHERE trip_id = ? AND id IN (${placeholders})`, tripId, ...ids, - ); - const owned = new Set(rows.map((r) => r.id)); - return ids.filter((id) => owned.has(id)); + // PL23 — input-order preservation lives in the repository method now. + return this.placesRepo.scopedIds(tripId, ids); } // ------------------------------------------------------------------------- @@ -553,7 +594,7 @@ export class PlacesService { }); // Settle reclaims after the transaction commits, so the refcount sees the // final image_url state. - for (const reclaim of reclaims) await reclaimPlaceImage(this.storage, reclaim); + for (const reclaim of reclaims) await this.reclaimPlaceImage(reclaim); return updated; } @@ -630,29 +671,18 @@ export class PlacesService { for (const strategy of placeMatchStrategies(place)) { let hit: { id: number; google_ftid: string | null } | undefined; if (strategy.by === 'externalId') { - hit = this.dbs.get<{ id: number; google_ftid: string | null }>(` - SELECT id, google_ftid FROM places - WHERE trip_id = ? AND (google_place_id = ? OR google_ftid = ? OR osm_id = ? OR amap_poi_id = ?) - ORDER BY id ASC - LIMIT 1 - `, tripId, strategy.id, strategy.id, strategy.id, strategy.id); + // PL25 — the same candidate id bound in all four positions. + hit = await this.placesRepo.findDuplicateByExternalId(tripId, strategy.id); } else if (strategy.by === 'name') { - hit = this.dbs.get<{ id: number; google_ftid: string | null }>(` - SELECT id, google_ftid FROM places - WHERE trip_id = ? AND lower(trim(name)) = ? - ORDER BY id ASC - LIMIT 1 - `, tripId, strategy.name); + // PL26 — `strategy.name` is already `.trim().toLowerCase()`'d in JS + // (`normalizePlaceName`, `@trek/shared/place/place-match.ts`, + // Unicode-aware) — the documented legacy disagreement with SQLite's + // ASCII-only `lower()` (rule 18's exception clause: the legacy + // statement itself received a JS-lowered value). + hit = await this.placesRepo.findDuplicateByName(tripId, strategy.name); } else { - hit = this.dbs.get<{ id: number; google_ftid: string | null }>(` - SELECT id, google_ftid FROM places - WHERE trip_id = ? - AND lat IS NOT NULL AND lng IS NOT NULL - AND abs(lat - ?) <= ? - AND abs(lng - ?) <= ? - ORDER BY id ASC - LIMIT 1 - `, tripId, strategy.lat, strategy.tolerance, strategy.lng, strategy.tolerance); + // PL27 — `abs(lat - ?) <= ? AND abs(lng - ?) <= ?` through the Task 0b `absDifference` helper. + hit = await this.placesRepo.findDuplicateByCoords(tripId, strategy.lat, strategy.lng, strategy.tolerance); } if (hit) return hit; } @@ -679,29 +709,21 @@ export class PlacesService { * than handing over a file that imports as nothing on the other end. */ async exportGpx(tripId: string, opts: GpxExportOptions = {}): Promise<{ gpx: string; filename: string } | null> { + // PL28 — `SELECT title FROM trips WHERE id = ?`. `trips` is outside this + // task's owned repositories (Plan 3c Task 7's domain) and + // `Trips.repository.ts` has a live concurrent editor in this tree this + // session (Task 6, adding its own byte-identical `getTitle` for a + // different caller) — stays raw here rather than risk a duplicate-method + // collision; once Task 6 lands, this call can reuse its `getTitle`. const trip = this.dbs.get<{ title: string }>('SELECT title FROM trips WHERE id = ?', tripId); if (!trip) return null; - const places = this.dbs.all(` - SELECT p.name, p.description, p.address, p.lat, p.lng, p.route_geometry, c.name AS category - FROM places p - LEFT JOIN categories c ON c.id = p.category_id - WHERE p.trip_id = ? - ORDER BY p.id - `, tripId); + // PL29 — the waypoint projection. + const places = await this.placesRepo.listForGpx(tripId) as GpxExportPlace[]; - // One row per stop, ordered the way the day plan draws it, then folded into days. - const stops = this.dbs.all<{ - day_number: number; date: string | null; title: string | null; - name: string; lat: number; lng: number; - }>(` - SELECT d.day_number, d.date, d.title, p.name, p.lat, p.lng - FROM days d - JOIN day_assignments da ON da.day_id = d.id - JOIN places p ON p.id = da.place_id - WHERE d.trip_id = ? AND p.lat IS NOT NULL AND p.lng IS NOT NULL - ORDER BY d.day_number, da.order_index - `, tripId); + // One row per stop, ordered the way the day plan draws it, then folded + // into days. PL30 — `DayAssignmentsRepository.listItineraryForGpx`. + const stops = await this.dayAssignmentsRepo.listItineraryForGpx(tripId); const days = new Map(); for (const stop of stops) { @@ -1562,7 +1584,12 @@ export class PlacesService { // ------------------------------------------------------------------------- async searchImage(tripId: string, placeId: string, userId: number) { - const place = this.dbs.get('SELECT * FROM places WHERE id = ? AND trip_id = ?', placeId, tripId); + // `toRowId` first (Task 3 review H1, absorbed here) — same gate shape + // as every other place-id route in this service. + const id = toRowId(placeId); + if (id === null) return { error: 'Place not found', status: 404 }; + // PL48 — same statement as PL9 (`applyUpdate`'s pre-image read). + const place = await this.placesRepo.findInTrip(id, Number(tripId)); if (!place) return { error: 'Place not found', status: 404 }; return this.unsplash.searchUnsplashPhotos(place.name + (place.address ? ' ' + place.address : ''), 5, await this.unsplash.getUnsplashKey(userId)); @@ -1579,17 +1606,23 @@ export class PlacesService { * place (with the new aggregate) or null when the place isn't in the trip. */ async rate(tripId: string, placeId: string, userId: number, rating: number | null): Promise { - const place = this.dbs.get('SELECT id FROM places WHERE id = ? AND trip_id = ?', placeId, tripId); - if (!place) return null; + // `toRowId` first (Task 3 review H1, absorbed here): PL50/PL51 write + // with this SAME id — a non-canonical `placeId` must resolve to "not + // found" here rather than pass a loose affinity match and then write a + // rating against an id `place_ratings`'s own FK never actually named. + const id = toRowId(placeId); + if (id === null) return null; + // PL49 — the existence gate. + if (!(await this.placesRepo.existsInTrip(id, Number(tripId)))) return null; if (rating === null) { - this.dbs.run('DELETE FROM place_ratings WHERE place_id = ? AND user_id = ?', placeId, userId); + // PL50 — `DELETE FROM place_ratings WHERE place_id = ? AND user_id = ?`. + await this.placeRatingsRepo.deleteRating(id, userId); } else { - this.dbs.run(` - INSERT INTO place_ratings (place_id, user_id, rating) VALUES (?, ?, ?) - ON CONFLICT(place_id, user_id) DO UPDATE SET rating = excluded.rating - `, placeId, userId, rating); + // PL51 — the only explicit `ON CONFLICT … DO UPDATE` in the cluster; never touches `places.updated_at`. + await this.placeRatingsRepo.upsertRating(id, userId, rating); } - return await this.dbs.getPlaceWithTags(placeId); + // PL52 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. + return await this.placesRepo.findWithTagsAndRatings(id); } // Journey hooks — non-fatal, mirroring the route's try/catch wrappers. diff --git a/server/tests/e2e/places.e2e.test.ts b/server/tests/e2e/places.e2e.test.ts index 04a938413..be2cdf7fd 100644 --- a/server/tests/e2e/places.e2e.test.ts +++ b/server/tests/e2e/places.e2e.test.ts @@ -1,9 +1,18 @@ /** * Places module e2e — exercises the migrated /api/trips/:tripId/places endpoints - * through the real JwtAuthGuard against a temp SQLite db. PlacesService runs its - * real (DI-native) SQL — places/categories/tags/place_tags/place_ratings DDL - * below; journeyService, the permission check, canAccessTrip and the WebSocket - * broadcast are mocked. + * through the real JwtAuthGuard against a real migrated-and-seeded temp SQLite db + * (createSnapshotTestDb(), Plan 3c Task 4 — this used to hand-roll a dozen + * CREATE TABLEs, a second hand-maintained schema copy that omitted several + * `places` columns `PlacesRepository.insertPlace`'s own `em.insert()` + * RETURNING read-back names (`reservation_status`, `reservation_notes`, + * `reservation_datetime`) and stubbed `getPlaceWithTags`/`canAccessTrip` + * directly rather than routing through the real repositories this domain now + * uses — the same class of failure `days.e2e.test.ts` (Task 2) and + * `assignments.e2e.test.ts` (Task 3) already fixed for their own files. + * PlacesService runs its real SQL (DI-injected, no service mock); + * journeyService, the permission check and the WebSocket broadcast stay + * mocked. Every `it(...)` body below is unchanged from before this + * conversion — only the DB bootstrap changed. */ import { describe, it, expect, beforeAll, afterAll, beforeEach, vi, type MockInstance } from 'vitest'; import request from 'supertest'; @@ -12,74 +21,22 @@ import type { Server } from 'http'; import { DatabaseModule } from '../../src/nest/database/database.module'; import { RealtimeModule } from '../../src/nest/realtime/realtime.module'; import { Test } from '@nestjs/testing'; -import { seedUser, sessionCookie } from './harness'; +import { sessionCookie } from './harness'; -const { db } = vi.hoisted(() => { - // eslint-disable-next-line @typescript-eslint/no-require-imports - const Database = require('better-sqlite3'); - const tmp = new Database(':memory:'); - tmp.exec('PRAGMA journal_mode = WAL'); - tmp.exec(`CREATE TABLE users (id INTEGER PRIMARY KEY AUTOINCREMENT, username TEXT NOT NULL, - email TEXT NOT NULL UNIQUE, role TEXT NOT NULL DEFAULT 'user', password_version INTEGER NOT NULL DEFAULT 0, - avatar TEXT);`); - // PlacesService runs its real SQL since the place fold — the full place column - // set the INSERT/UPDATE/SELECT paths touch, plus the joined projections. - tmp.exec(`CREATE TABLE places (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, name TEXT, - description TEXT, lat REAL, lng REAL, address TEXT, category_id INTEGER, price REAL, currency TEXT, - place_time TEXT, end_time TEXT, duration_minutes INTEGER, notes TEXT, image_url TEXT, - google_place_id TEXT, google_ftid TEXT, osm_id TEXT, amap_poi_id TEXT, website TEXT, phone TEXT, transport_mode TEXT, - route_geometry TEXT, route_color TEXT, stop_type TEXT, fill_percent INTEGER, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP, updated_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec(`CREATE TABLE categories (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT, color TEXT, icon TEXT);`); - // Plan 3c Task 1: TagsRepository.listForPlaces (QH1) selects `user_id` - // explicitly (the legacy statement's `t.*` tolerated a narrower fixture - // schema; a named column list does not) — added here, matching the real - // `tags` table (`Migration20200101000000_baseline_schema.ts`). - tmp.exec(`CREATE TABLE tags (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER NOT NULL, name TEXT, color TEXT, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec(`CREATE TABLE place_tags (place_id INTEGER NOT NULL, tag_id INTEGER NOT NULL, - PRIMARY KEY (place_id, tag_id));`); - tmp.exec(`CREATE TABLE place_ratings (place_id INTEGER NOT NULL, user_id INTEGER NOT NULL, rating INTEGER, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP, UNIQUE(place_id, user_id));`); - // The assignment=unassigned/assigned filters join these. - tmp.exec('CREATE TABLE days (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, day_number INTEGER, date TEXT, title TEXT);'); - // The GPX export reads the trip title for and the filename. - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT, user_id INTEGER, currency TEXT);'); - // TripAccessGuard now reads TripsRepository.findAccessible directly - // (Plan 3c Task 0b), a real join against trip_members. - tmp.exec('CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);'); - tmp.exec(`CREATE TABLE day_assignments (id INTEGER PRIMARY KEY AUTOINCREMENT, day_id INTEGER NOT NULL, - place_id INTEGER NOT NULL, order_index INTEGER DEFAULT 0);`); - // reclaimPlaceImage ref-counts an uploaded thumbnail across both tables. - // Deleting a place cancels the nights booked at it (#2354), so the delete path - // reads this table even in a file that never books one. - tmp.exec(`CREATE TABLE day_accommodations (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER, - place_id INTEGER, start_day_id INTEGER, end_day_id INTEGER, check_in TEXT, check_in_end TEXT, - check_out TEXT, confirmation TEXT, notes TEXT, created_at TEXT DEFAULT (datetime('now')));`); - tmp.exec(`CREATE TABLE collection_places (id INTEGER PRIMARY KEY AUTOINCREMENT, image_url TEXT);`); - // reclaimPhotoCache's removeIfUnreferenced sweeps the Google photo cache. - tmp.exec(`CREATE TABLE google_place_photo_meta (place_id TEXT PRIMARY KEY, attribution TEXT, error_at DATETIME);`); - // Deleting a place takes its linked expense with it (#1298). - tmp.exec(`CREATE TABLE budget_items (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, - name TEXT, total_price REAL DEFAULT 0, place_id INTEGER, reservation_id INTEGER);`); - // StorageRegistryService (behind StorageModule, now in this module chain) reads - // this at onModuleInit. - tmp.exec('CREATE TABLE app_settings (key TEXT PRIMARY KEY, value TEXT);'); - return { db: tmp }; +vi.mock('../../src/db/database', async () => { + const { createSnapshotTestDb, buildDbMock } = await import('../helpers/db-mock'); + return buildDbMock(createSnapshotTestDb()); }); +const { broadcast } = vi.hoisted(() => ({ broadcast: vi.fn() })); +vi.mock('../../src/websocket', () => ({ broadcast })); -const { canAccessTrip } = vi.hoisted(() => ({ canAccessTrip: vi.fn() })); -const { getPlaceWithTags } = vi.hoisted(() => ({ - // The one db/database helper PlacesService still calls through (post-write - // re-selects). Real implementation over the temp db so the responses carry - // the joined category/tags/ratings the client expects. - getPlaceWithTags: vi.fn(), +import { db } from '../../src/db/database'; + +const { onPlaceCreated, onPlaceUpdated, onPlaceDeleted } = vi.hoisted(() => ({ + onPlaceCreated: vi.fn().mockResolvedValue(undefined), + onPlaceUpdated: vi.fn().mockResolvedValue(undefined), + onPlaceDeleted: vi.fn().mockResolvedValue(undefined), })); -vi.mock('../../src/db/database', () => ({ - db, canAccessTrip, isOwner: vi.fn(() => true), getPlaceWithTags, closeDb: () => {}, reinitialize: () => {}, -})); -vi.mock('../../src/websocket', () => ({ broadcast: vi.fn() })); -import { broadcast } from '../../src/websocket'; import { JourneyDomainService } from '../../src/nest/journey/journey-domain.service'; import { PermissionsService } from '../../src/nest/permissions/permissions.service'; @@ -102,11 +59,7 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { async function build() { const moduleRef = await Test.createTestingModule({ imports: [await TestUnitOfWorkModule.forRoot(db), await createTestMikroOrmModule(db), DatabaseModule, RealtimeModule, PlacesModule] }) .overrideProvider(JourneyDomainService) - .useValue({ - onPlaceCreated: vi.fn().mockResolvedValue(undefined), - onPlaceUpdated: vi.fn().mockResolvedValue(undefined), - onPlaceDeleted: vi.fn().mockResolvedValue(undefined), - }) + .useValue({ onPlaceCreated, onPlaceUpdated, onPlaceDeleted }) .compile(); const nest = moduleRef.createNestApplication(); nest.use(cookieParser()); @@ -119,28 +72,32 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { } beforeAll(async () => { - seedUser(db as never, { id: 1 }); - getPlaceWithTags.mockImplementation((placeId: number | string) => { - const place = db.prepare(`SELECT p.*, c.name AS category_name, c.color AS category_color, c.icon AS category_icon - FROM places p LEFT JOIN categories c ON p.category_id = c.id WHERE p.id = ?`).get(placeId) as Record | undefined; - if (!place) return null; - const tags = db.prepare('SELECT t.* FROM tags t JOIN place_tags pt ON t.id = pt.tag_id WHERE pt.place_id = ?').all(placeId); - return { ...place, category: place.category_id ? { id: place.category_id, name: place.category_name } : null, tags, ratings: [], rating_avg: null, rating_count: 0 }; - }); + // harness.ts's seedUser() omits password_hash, which the real migrated + // schema requires NOT NULL (days.e2e.test.ts's/assignments.e2e.test.ts's + // own precedent) — raw inserts here instead. `username: 'e2e-user'` + // (user 1) matches the harness default so assertion bodies that spell + // out the owner's username stay unchanged. User 2 owns the "foreign" + // trip (id 6) several tests below reference — `trips.user_id` and + // `places.trip_id` both carry a real FK now (`ON DELETE CASCADE`), so a + // dangling trip_id the old hand-rolled DDL tolerated would fail here. + db.prepare("INSERT INTO users (id, username, email, password_hash, role, password_version) VALUES (1, 'e2e-user', 'e2e@example.test', 'x', 'user', 0)").run(); + db.prepare("INSERT INTO users (id, username, email, password_hash, role, password_version) VALUES (2, 'peer', 'peer@example.test', 'x', 'user', 0)").run(); + db.prepare("INSERT INTO trips (id, title, user_id) VALUES (6, 'Theirs', 2)").run(); app = await build(); checkPermission = vi.spyOn(app.get(PermissionsService), 'checkPermission'); server = app.getHttpServer(); }); beforeEach(() => { - db.exec('DELETE FROM trips; DELETE FROM places; DELETE FROM place_tags; DELETE FROM place_ratings; DELETE FROM day_assignments; DELETE FROM days;'); - // Plan 3c Task 0b: TripAccessGuard reads TripsRepository.findAccessible - // directly now, a real query — `canAccessTrip.mockReturnValue(...)` no - // longer intercepts it, so trip 5's real row is (re-)seeded every test - // instead, owned by user 1, matching what the mock used to fake. GPX - // tests below re-seed their own data-bearing row with `INSERT OR - // REPLACE` over this default. + // Trip 5 (owned by user 1) is the suite's main trip, re-seeded fresh + // every test — deleting it (`ON DELETE CASCADE`) also clears every place/ + // day/assignment/rating/tag-link/budget-item this suite seeded under it, + // the real-FK equivalent of the old blanket `DELETE FROM places; …`. + // Trip 6 (the "foreign" trip, owned by user 2) is seeded once in + // beforeAll and left alone. + db.exec('DELETE FROM trips WHERE id = 5;'); db.prepare("INSERT INTO trips (id, title, user_id) VALUES (5, 'Trip', 1)").run(); + db.exec('DELETE FROM places WHERE trip_id = 6;'); checkPermission.mockReturnValue(true); }); @@ -184,7 +141,8 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { }); it('200 (not 201) bulk-delete, 400 on bad ids', async () => { - db.prepare("INSERT INTO places (id, trip_id, name) VALUES (1, 5, 'A'), (2, 5, 'B'), (3, 6, 'Foreign')").run(); + db.prepare("INSERT INTO places (id, trip_id, name) VALUES (1, 5, 'A'), (2, 5, 'B')").run(); + db.prepare("INSERT INTO places (id, trip_id, name) VALUES (3, 6, 'Foreign')").run(); const ok = await request(server).post('/api/trips/5/places/bulk-delete').set('Cookie', sessionCookie(1)).send({ ids: [1, 2, 3] }); expect(ok.status).toBe(200); expect(ok.body).toEqual({ deleted: [1, 2], count: 2 }); @@ -279,7 +237,8 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { }); it('DELETE :id removes the row, 404 for a foreign place', async () => { - db.prepare("INSERT INTO places (id, trip_id, name) VALUES (9, 5, 'Gone'), (10, 6, 'Foreign')").run(); + db.prepare("INSERT INTO places (id, trip_id, name) VALUES (9, 5, 'Gone')").run(); + db.prepare("INSERT INTO places (id, trip_id, name) VALUES (10, 6, 'Foreign')").run(); const ok = await request(server).delete('/api/trips/5/places/9').set('Cookie', sessionCookie(1)); expect(ok.status).toBe(200); diff --git a/server/tests/unit/db/repositories/DayAssignments.repository.test.ts b/server/tests/unit/db/repositories/DayAssignments.repository.test.ts index 1c4889281..d921706c9 100644 --- a/server/tests/unit/db/repositories/DayAssignments.repository.test.ts +++ b/server/tests/unit/db/repositories/DayAssignments.repository.test.ts @@ -291,13 +291,13 @@ describe('DayAssignmentsRepository — AS4/AS9/AS12 existence + trip-scoped read expect(await assignments.existsInDay(a.id, day.id, trip.id + 1)).toBe(false); }); - it('ASSIGNREPO-002 (AS9): raw-bind — a string id binds unconverted, same as a real number', async () => { + it('ASSIGNREPO-002 (AS9): raw-bind on day_id/trip_id — a string binds unconverted, same as a real number; `id` itself is `number`-only now (Task 3 review H1, absorbed in Task 4: the method\'s only caller `toRowId`s `id` before calling, so the type narrowed to match)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const day = createDay(testDb, trip.id); const place = createPlace(testDb, trip.id); const a = createDayAssignment(testDb, day.id, place.id); - expect(await assignments.existsInDay(String(a.id), String(day.id), String(trip.id))).toBe(true); + expect(await assignments.existsInDay(a.id, String(day.id), String(trip.id))).toBe(true); }); it('ASSIGNREPO-003 (AS12, findInTrip): the raw `da.*` row, scoped to the trip via a two-hop join; undefined cross-trip', async () => { @@ -595,3 +595,49 @@ it('ASSIGNREPO-023 (D-shape): a notes write after an unrelated identity-map read await withRequestContext(t.orm, () => assignments.setNotes(a.id, 'fresh note')); expect(await assignments.findInTrip(a.id, trip.id)).toMatchObject({ notes: 'fresh note' }); }); + +// --------------------------------------------------------------------------- +// Plan 3c Task 4 (`PlacesService.exportGpx`) — the PL30 itinerary read. +// --------------------------------------------------------------------------- + +describe('DayAssignmentsRepository.listItineraryForGpx (PL30)', () => { + it('ASSIGNREPO-024: joins day + place, only stops with coordinates, ordered by day_number then order_index', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day1 = createDay(testDb, trip.id, { day_number: 1, date: '2026-01-01', title: 'Day One' }); + const day2 = createDay(testDb, trip.id, { day_number: 2, date: '2026-01-02' }); + const geo = createPlace(testDb, trip.id, { name: 'Has Coords', lat: 1, lng: 2 }); + const noGeo = testDb.prepare('INSERT INTO places (trip_id, name, lat, lng) VALUES (?, ?, NULL, NULL)').run(trip.id, 'No Coords').lastInsertRowid as number; + const secondStop = createPlace(testDb, trip.id, { name: 'Second Stop', lat: 3, lng: 4 }); + // day2's stop first (order_index 0), day1's stop second — proves the + // ORDER BY is day_number then order_index, not insertion/id order. + createDayAssignment(testDb, day2.id, secondStop.id, { order_index: 0 }); + createDayAssignment(testDb, day1.id, geo.id, { order_index: 0 }); + createDayAssignment(testDb, day1.id, noGeo, { order_index: 1 }); + + const rows = await assignments.listItineraryForGpx(trip.id); + expect(rows).toEqual([ + { day_number: 1, date: '2026-01-01', title: 'Day One', name: 'Has Coords', lat: 1, lng: 2 }, + { day_number: 2, date: '2026-01-02', title: null, name: 'Second Stop', lat: 3, lng: 4 }, + ]); + }); + + it('ASSIGNREPO-025: a trip with no coordinate-bearing stops returns []', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const noGeo = testDb.prepare('INSERT INTO places (trip_id, name, lat, lng) VALUES (?, ?, NULL, NULL)').run(trip.id, 'No Coords').lastInsertRowid as number; + createDayAssignment(testDb, day.id, noGeo); + expect(await assignments.listItineraryForGpx(trip.id)).toEqual([]); + }); + + it('ASSIGNREPO-026: another trip\'s stops never leak in', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const other = createTrip(testDb, user.id); + const day = createDay(testDb, other.id); + const place = createPlace(testDb, other.id, { lat: 5, lng: 6 }); + createDayAssignment(testDb, day.id, place.id); + expect(await assignments.listItineraryForGpx(trip.id)).toEqual([]); + }); +}); diff --git a/server/tests/unit/db/repositories/PlaceRatings.repository.test.ts b/server/tests/unit/db/repositories/PlaceRatings.repository.test.ts index 8fd488b50..1ba6999fd 100644 --- a/server/tests/unit/db/repositories/PlaceRatings.repository.test.ts +++ b/server/tests/unit/db/repositories/PlaceRatings.repository.test.ts @@ -89,12 +89,96 @@ describe('PlaceRatingsRepository.listForPlaces', () => { // TAGREPO comments already use for a method that IS at risk. it('PLACERATINGSREPO-005 (D-shape): a rating written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { const { user: owner } = createUser(testDb); + const { user: seedVoter } = createUser(testDb, { username: 'seed-voter' }); const { user: voter } = createUser(testDb, { username: 'fresh-voter' }); const trip = createTrip(testDb, owner.id); const place = createPlace(testDb, trip.id); - await t.repo(PlaceRatings).find({}); // populate the identity map with an unrelated (empty) read - rate(place.id, voter.id, 3, '2026-01-01 00:00:00'); + rate(place.id, seedVoter.id, 1, '2026-01-01 00:00:00'); + // rule 20: the FIRST, wider setup read passes `disableIdentityMap: false` + // explicitly and carries the column the later write targets (`rating`) — + // `find({})` with the base default merged in populates nothing. + await t.repo(PlaceRatings).find({}, { disableIdentityMap: false }); + rate(place.id, voter.id, 3, '2026-01-02 00:00:00'); const rows = await placeRatings.listForPlaces([place.id]); - expect(rows).toEqual([{ place_id: place.id, user_id: voter.id, username: 'fresh-voter', avatar: null, rating: 3 }]); + expect(rows).toEqual([ + { place_id: place.id, user_id: seedVoter.id, username: 'seed-voter', avatar: null, rating: 1 }, + { place_id: place.id, user_id: voter.id, username: 'fresh-voter', avatar: null, rating: 3 }, + ]); + }); +}); + +// --------------------------------------------------------------------------- +// Plan 3c Task 4 — upsertRating (PL51) / deleteRating (PL50). +// --------------------------------------------------------------------------- + +describe('PlaceRatingsRepository.upsertRating (PL51)', () => { + it('UPSERTRATINGREPO-001: inserts a new vote, then a re-vote replaces the rating via ON CONFLICT DO UPDATE', async () => { + const { user: owner } = createUser(testDb); + const { user: voter } = createUser(testDb, { username: 'voter1' }); + const trip = createTrip(testDb, owner.id); + const place = createPlace(testDb, trip.id); + + await placeRatings.upsertRating(place.id, voter.id, 5); + let rows = testDb.prepare('SELECT rating FROM place_ratings WHERE place_id = ? AND user_id = ?').all(place.id, voter.id) as { rating: number }[]; + expect(rows).toEqual([{ rating: 5 }]); + + await placeRatings.upsertRating(place.id, voter.id, 2); + rows = testDb.prepare('SELECT rating FROM place_ratings WHERE place_id = ? AND user_id = ?').all(place.id, voter.id) as { rating: number }[]; + expect(rows).toEqual([{ rating: 2 }]); + // Exactly one row for the (place, user) pair — the composite unique key held, not a duplicate insert. + const count = testDb.prepare('SELECT COUNT(*) AS n FROM place_ratings WHERE place_id = ? AND user_id = ?').get(place.id, voter.id) as { n: number }; + expect(count.n).toBe(1); + }); + + it('UPSERTRATINGREPO-002 (PL51 ruling): a re-vote never touches places.updated_at — a vote must not 409 another member\'s If-Match', async () => { + const { user: owner } = createUser(testDb); + const { user: voter } = createUser(testDb, { username: 'voter2' }); + const trip = createTrip(testDb, owner.id); + const place = createPlace(testDb, trip.id); + testDb.prepare("UPDATE places SET updated_at = datetime('now', '-1 hour') WHERE id = ?").run(place.id); + const before = (testDb.prepare('SELECT updated_at FROM places WHERE id = ?').get(place.id) as { updated_at: string }).updated_at; + + await placeRatings.upsertRating(place.id, voter.id, 4); + await placeRatings.upsertRating(place.id, voter.id, 1); // the conflict path + + const after = (testDb.prepare('SELECT updated_at FROM places WHERE id = ?').get(place.id) as { updated_at: string }).updated_at; + expect(after).toBe(before); + }); + + it('UPSERTRATINGREPO-003: two different voters on the same place both keep their own rating', async () => { + const { user: owner } = createUser(testDb); + const { user: voterA } = createUser(testDb, { username: 'va' }); + const { user: voterB } = createUser(testDb, { username: 'vb' }); + const trip = createTrip(testDb, owner.id); + const place = createPlace(testDb, trip.id); + await placeRatings.upsertRating(place.id, voterA.id, 5); + await placeRatings.upsertRating(place.id, voterB.id, 1); + const rows = testDb.prepare('SELECT user_id, rating FROM place_ratings WHERE place_id = ? ORDER BY user_id').all(place.id); + expect(rows).toEqual([ + { user_id: Math.min(voterA.id, voterB.id), rating: voterA.id < voterB.id ? 5 : 1 }, + { user_id: Math.max(voterA.id, voterB.id), rating: voterA.id < voterB.id ? 1 : 5 }, + ]); + }); +}); + +describe('PlaceRatingsRepository.deleteRating (PL50)', () => { + it('DELETERATINGREPO-001: removes exactly the (place, user) row, leaving another voter\'s row alone', async () => { + const { user: owner } = createUser(testDb); + const { user: voterA } = createUser(testDb, { username: 'da' }); + const { user: voterB } = createUser(testDb, { username: 'db' }); + const trip = createTrip(testDb, owner.id); + const place = createPlace(testDb, trip.id); + await placeRatings.upsertRating(place.id, voterA.id, 5); + await placeRatings.upsertRating(place.id, voterB.id, 3); + await placeRatings.deleteRating(place.id, voterA.id); + expect(testDb.prepare('SELECT user_id FROM place_ratings WHERE place_id = ?').all(place.id)).toEqual([{ user_id: voterB.id }]); + }); + + it('DELETERATINGREPO-002: deleting a vote that does not exist is a no-op, not a throw', async () => { + const { user: owner } = createUser(testDb); + const { user: voter } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const place = createPlace(testDb, trip.id); + await expect(placeRatings.deleteRating(place.id, voter.id)).resolves.toBeUndefined(); }); }); diff --git a/server/tests/unit/db/repositories/Places.repository.test.ts b/server/tests/unit/db/repositories/Places.repository.test.ts index 5955a5a67..5fc56735b 100644 --- a/server/tests/unit/db/repositories/Places.repository.test.ts +++ b/server/tests/unit/db/repositories/Places.repository.test.ts @@ -9,7 +9,7 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; -import { createCategory, createPlace, createTag, createTrip, createUser } from '../../../helpers/factories'; +import { createCategory, createDay, createDayAssignment, createPlace, createTag, createTrip, createUser } from '../../../helpers/factories'; import { Places } from '../../../../src/db/entities/Places.entity'; import type { PlacesRepository } from '../../../../src/db/repositories/Places.repository'; @@ -229,3 +229,305 @@ describe('PlacesRepository.existsByGoogleIdOrImageUrl', () => { expect(await places.existsByGoogleIdOrImageUrl('never-seen', '/api/maps/place-photo/never-seen/bytes')).toBe(false); }); }); + +// --------------------------------------------------------------------------- +// Plan 3c Task 4 — the new PlacesRepository methods. +// --------------------------------------------------------------------------- + +describe('PlacesRepository.existsInTrip / findInTrip / reclaimInputs / deleteById (PL0/PL7/PL9/PL17/PL19/PL48/PL49/AS5)', () => { + it('PLACEREPO-012: existsInTrip is true only for the matching id+trip pair', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + expect(await places.existsInTrip(place.id, trip.id)).toBe(true); + expect(await places.existsInTrip(place.id, otherTrip.id)).toBe(false); + expect(await places.existsInTrip(999999, trip.id)).toBe(false); + }); + + it('PLACEREPO-013 (D-shape): a name write after an unrelated identity-map read is visible in findInTrip\'s FIRST wider read', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Before' }); + // Populate the identity map with an unrelated, WIDER read first — `{ disableIdentityMap: false }` + // explicitly (program rule 20: the base default leaves the identity map empty, making this vacuous). + await t.repo(Places).find({ trip: trip.id }, { disableIdentityMap: false }); + testDb.prepare('UPDATE places SET name = ? WHERE id = ?').run('After', place.id); + const row = await places.findInTrip(place.id, trip.id); + expect(row?.name).toBe('After'); + }); + + it('PLACEREPO-014: findInTrip returns every scalar column (PL9/PL48\'s SELECT *), undefined cross-trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Full Row' }); + const row = await places.findInTrip(place.id, trip.id); + expect(row).toMatchObject({ id: place.id, trip_id: trip.id, name: 'Full Row' }); + expect(await places.findInTrip(place.id, otherTrip.id)).toBeUndefined(); + }); + + it('PLACEREPO-015: reclaimInputs projects only google_place_id/image_url', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + testDb.prepare('UPDATE places SET google_place_id = ?, image_url = ? WHERE id = ?').run('gpid-1', '/uploads/places/x.jpg', place.id); + expect(await places.reclaimInputs(place.id, trip.id)).toEqual({ google_place_id: 'gpid-1', image_url: '/uploads/places/x.jpg' }); + expect(await places.reclaimInputs(999999, trip.id)).toBeUndefined(); + }); + + it('PLACEREPO-016: deleteById removes exactly that row', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const keep = createPlace(testDb, trip.id, { name: 'Keep' }); + const gone = createPlace(testDb, trip.id, { name: 'Gone' }); + await places.deleteById(gone.id); + expect(testDb.prepare('SELECT id FROM places WHERE id = ?').get(gone.id)).toBeUndefined(); + expect(testDb.prepare('SELECT id FROM places WHERE id = ?').get(keep.id)).toBeDefined(); + }); +}); + +describe('PlacesRepository.scopedIds (PL23) — input-order preservation', () => { + it('PLACEREPO-017: returns only the trip\'s own ids, in the CALLER\'s input order, not row/id order', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const other = createTrip(testDb, user.id); + const p1 = createPlace(testDb, trip.id); + const p2 = createPlace(testDb, trip.id); + const p3 = createPlace(testDb, trip.id); + const foreign = createPlace(testDb, other.id); + // Deliberately out of id order and interleaved with a foreign + missing id. + const result = await places.scopedIds(trip.id, [p3.id, 999999, p1.id, foreign.id, p2.id]); + expect(result).toEqual([p3.id, p1.id, p2.id]); + }); + + it('PLACEREPO-018: empty input short-circuits to []', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + expect(await places.scopedIds(trip.id, [])).toEqual([]); + }); +}); + +describe('PlacesRepository.insertPlace (PL4) / updatePlace (PL11)', () => { + it('PLACEREPO-019: insertPlace writes every one of the 25 columns and returns the generated id', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const id = await places.insertPlace({ + trip_id: trip.id, name: 'Full Insert', description: 'd', lat: 0, lng: 0, address: 'a', + category_id: null, price: 0, currency: 'USD', place_time: '09:00', end_time: '10:00', + duration_minutes: 60, notes: 'n', image_url: '/uploads/places/x.jpg', google_place_id: 'g', + google_ftid: 'f', osm_id: 'o', amap_poi_id: 'am', website: 'w', phone: 'p', + transport_mode: 'walking', route_geometry: '[[1,2]]', route_color: '#fff', stop_type: 'fuel', + fill_percent: 50, + }); + const row = await places.findInTrip(id, trip.id); + expect(row).toMatchObject({ + trip_id: trip.id, name: 'Full Insert', description: 'd', lat: 0, lng: 0, address: 'a', + category_id: null, price: 0, currency: 'USD', place_time: '09:00', end_time: '10:00', + duration_minutes: 60, notes: 'n', image_url: '/uploads/places/x.jpg', google_place_id: 'g', + google_ftid: 'f', osm_id: 'o', amap_poi_id: 'am', website: 'w', phone: 'p', + transport_mode: 'walking', route_geometry: '[[1,2]]', route_color: '#fff', stop_type: 'fuel', + fill_percent: 50, + }); + }); + + it('PLACEREPO-020 (PL11 matrix): a written null vs a written value are byte-distinct on the same column', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Original' }); + const base = { + name: 'Original', description: null, lat: null, lng: null, address: null, category_id: null, + price: null, currency: null, place_time: null, end_time: null, duration_minutes: null, + notes: null, image_url: null, google_place_id: null, google_ftid: null, osm_id: null, + amap_poi_id: null, website: null, phone: null, transport_mode: null, route_color: null, + stop_type: null, fill_percent: null, + }; + // A non-COALESCE column (description) written explicitly null clears it. + await places.updatePlace(place.id, { ...base, description: 'first' }); + expect((await places.findInTrip(place.id, trip.id))?.description).toBe('first'); + await places.updatePlace(place.id, { ...base, description: null }); + expect((await places.findInTrip(place.id, trip.id))?.description).toBeNull(); + // duration_minutes: 0 is a real value, distinct from null (the service's + // `!== undefined` fallback — this repository test proves the write itself + // stores 0 and null distinctly, not that either collapses to the other). + await places.updatePlace(place.id, { ...base, duration_minutes: 0 }); + expect((await places.findInTrip(place.id, trip.id))?.duration_minutes).toBe(0); + await places.updatePlace(place.id, { ...base, duration_minutes: null }); + expect((await places.findInTrip(place.id, trip.id))?.duration_minutes).toBeNull(); + }); + + it('PLACEREPO-021: updatePlace stamps updated_at with the current time', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Stampable' }); + // Force a stale, second-resolution-distinguishable `updated_at` — this + // write and the assertion below both happen within the same real + // second, so comparing against `createPlace`'s own `CURRENT_TIMESTAMP` + // default would be flaky (Object.is on two identical second-granularity + // strings), not a proof of anything. + testDb.prepare("UPDATE places SET updated_at = datetime('now', '-1 hour') WHERE id = ?").run(place.id); + const before = (await places.findInTrip(place.id, trip.id))?.updated_at; + await places.updatePlace(place.id, { + name: 'Stamped', description: null, lat: null, lng: null, address: null, category_id: null, + price: null, currency: null, place_time: null, end_time: null, duration_minutes: null, + notes: null, image_url: null, google_place_id: null, google_ftid: null, osm_id: null, + amap_poi_id: null, website: null, phone: null, transport_mode: null, route_color: null, + stop_type: null, fill_percent: null, + }); + const after = await places.findInTrip(place.id, trip.id); + expect(after?.name).toBe('Stamped'); + expect(after?.updated_at).not.toBe(before); + }); +}); + +describe('PlacesRepository.findDuplicateByExternalId / findDuplicateByName / findDuplicateByCoords (PL25/PL26/PL27)', () => { + it('PLACEREPO-022 (PL25): matches on any of the four provider-id columns, lowest id wins', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const first = createPlace(testDb, trip.id, { name: 'First' }); + testDb.prepare('UPDATE places SET osm_id = ? WHERE id = ?').run('node:123', first.id); + const second = createPlace(testDb, trip.id, { name: 'Second' }); + testDb.prepare('UPDATE places SET google_ftid = ? WHERE id = ?').run('node:123', second.id); + const hit = await places.findDuplicateByExternalId(String(trip.id), 'node:123'); + expect(hit?.id).toBe(first.id); + }); + + it('PLACEREPO-023 (PL26, rule 18 exception): lower(trim(name)) matches ASCII-folded names; a non-ASCII cased letter does NOT match — the documented legacy divergence, pinned not fixed', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: ' Café Central ' }); + // The service's own JS-lowered+trimmed value, matching `normalizePlaceName`'s + // Unicode-aware fold — this side is honest, so it would match the row exactly. + const jsLowered = ' café central '.trim().toLowerCase(); + // SQLite's lower()/trim() is ASCII-only: it lowercases "C" but never touches + // "É"/"é" — `Café Central` (stored) trims+lowers via SQL to `café central`. + const hit = await places.findDuplicateByName(String(trip.id), jsLowered); + expect(hit?.id).toBe(place.id); + // A row stored with an upper-case accented letter (`CAFÉ`) does NOT match a + // JS-lowered candidate (`café`) — SQLite's LOWER() never folds `É` to `é`, + // reproducing the documented disagreement (places.service.ts's own PL26 docstring). + const accented = createPlace(testDb, trip.id, { name: 'CAFÉ NORD' }); + const jsLoweredAccented = 'café nord'.toLowerCase(); + const miss = await places.findDuplicateByName(String(trip.id), jsLoweredAccented); + expect(miss?.id).not.toBe(accented.id); + }); + + it('PLACEREPO-024 (PL27): matches within the coordinate tolerance box, on both lat and lng', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { lat: 48.8566, lng: 2.3522 }); + const hit = await places.findDuplicateByCoords(String(trip.id), 48.8566 + 0.00005, 2.3522 - 0.00005, 0.0001); + expect(hit?.id).toBe(place.id); + const miss = await places.findDuplicateByCoords(String(trip.id), 48.9, 2.5, 0.0001); + expect(miss).toBeUndefined(); + }); +}); + +describe('PlacesRepository.listForGpx (PL29) and existsByImageUrl (PI1)', () => { + it('PLACEREPO-025: projects the waypoint columns plus the flat category name, ordered by id', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const category = createCategory(testDb, { name: 'Trails' }); + const p1 = createPlace(testDb, trip.id, { name: 'A', category_id: category.id }); + // `createPlace` always falls back to the first available category when + // no override is given (its own `?? defaultCat?.id ?? null` chain — an + // explicit `category_id: null` override is ALSO nullish, so it falls + // through the same way), and deleting the categories table would also + // break p1's own join. Insert place B directly instead, to genuinely + // pin `category_id = NULL` while `category` (p1's) still exists. + const p2Id = testDb.prepare('INSERT INTO places (trip_id, name, category_id) VALUES (?, ?, NULL)').run(trip.id, 'B').lastInsertRowid; + const p2 = { id: p2Id as number }; + const rows = await places.listForGpx(String(trip.id)); + expect(rows.map((r) => r.name)).toEqual(['A', 'B']); + expect(rows[0]).toMatchObject({ name: 'A', category: 'Trails' }); + expect(rows[1]).toMatchObject({ name: 'B', category: null }); + void p1; void p2; + }); + + it('PLACEREPO-026 (PI1): existsByImageUrl is trip-agnostic — true for any place carrying the url', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + testDb.prepare('UPDATE places SET image_url = ? WHERE id = ?').run('/uploads/places/shared.jpg', place.id); + expect(await places.existsByImageUrl('/uploads/places/shared.jpg')).toBe(true); + expect(await places.existsByImageUrl('/uploads/places/nope.jpg')).toBe(false); + }); +}); + +describe('PlacesRepository.listForTrip (PL3) — filter fragments', () => { + it('PLACEREPO-027: no filters — every trip place, DISTINCT, newest first', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const older = createPlace(testDb, trip.id, { name: 'Older' }); + testDb.prepare("UPDATE places SET created_at = datetime('now', '-1 hour') WHERE id = ?").run(older.id); + const newer = createPlace(testDb, trip.id, { name: 'Newer' }); + const rows = await places.listForTrip(String(trip.id), {}); + expect(rows.map((r) => r.id)).toEqual([newer.id, older.id]); + }); + + it('PLACEREPO-028: searchPattern matches name/address/description (already-escaped, wrapped by the caller)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const hit = createPlace(testDb, trip.id, { name: 'Eiffel Tower' }); + const miss = createPlace(testDb, trip.id, { name: 'Louvre' }); + const rows = await places.listForTrip(String(trip.id), { searchPattern: '%Eiffel%' }); + expect(rows.map((r) => r.id)).toEqual([hit.id]); + void miss; + }); + + it('PLACEREPO-029: category filters to that category_id only', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const cat = createCategory(testDb, { name: 'Museums' }); + const inCat = createPlace(testDb, trip.id, { category_id: cat.id }); + const outCat = createPlace(testDb, trip.id); + testDb.prepare('DELETE FROM categories WHERE id != ?').run(cat.id); + const rows = await places.listForTrip(String(trip.id), { category: String(cat.id) }); + expect(rows.map((r) => r.id)).toEqual([inCat.id]); + void outCat; + }); + + it('PLACEREPO-030: tag filters via the place_tags subquery', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const tag = createTag(testDb, user.id, { name: 'Must see' }); + const tagged = createPlace(testDb, trip.id, { name: 'Tagged' }); + const untagged = createPlace(testDb, trip.id, { name: 'Untagged' }); + testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(tagged.id, tag.id); + const rows = await places.listForTrip(String(trip.id), { tag: String(tag.id) }); + expect(rows.map((r) => r.id)).toEqual([tagged.id]); + void untagged; + }); + + it('PLACEREPO-031: assignment=unassigned / assigned split the trip\'s places by day_assignments membership', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const assigned = createPlace(testDb, trip.id, { name: 'On a day' }); + const unassigned = createPlace(testDb, trip.id, { name: 'In the pool' }); + createDayAssignment(testDb, day.id, assigned.id); + const assignedRows = await places.listForTrip(String(trip.id), { assignment: 'assigned' }); + expect(assignedRows.map((r) => r.id)).toEqual([assigned.id]); + const unassignedRows = await places.listForTrip(String(trip.id), { assignment: 'unassigned' }); + expect(unassignedRows.map((r) => r.id)).toEqual([unassigned.id]); + }); + + it('PLACEREPO-032: search + category + tag + assignment combine with AND, all four fragments at once', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const cat = createCategory(testDb, { name: 'Parks' }); + const tag = createTag(testDb, user.id, { name: 'Green' }); + const day = createDay(testDb, trip.id); + const winner = createPlace(testDb, trip.id, { name: 'Central Park', category_id: cat.id }); + testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(winner.id, tag.id); + createDayAssignment(testDb, day.id, winner.id); + // A near-miss on every axis, to prove the AND actually excludes it. + const other = createPlace(testDb, trip.id, { name: 'Central Park Annex', category_id: cat.id }); + testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(other.id, tag.id); + // `other` is left unassigned, so `assignment: 'assigned'` alone should exclude it. + const rows = await places.listForTrip(String(trip.id), { + searchPattern: '%Central Park%', category: String(cat.id), tag: String(tag.id), assignment: 'assigned', + }); + expect(rows.map((r) => r.id)).toEqual([winner.id]); + }); +}); diff --git a/server/tests/unit/db/repositories/Tags.repository.test.ts b/server/tests/unit/db/repositories/Tags.repository.test.ts index 78fefa7ad..3b4717775 100644 --- a/server/tests/unit/db/repositories/Tags.repository.test.ts +++ b/server/tests/unit/db/repositories/Tags.repository.test.ts @@ -318,8 +318,11 @@ describe('TagsRepository', () => { const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id); const tag = createTag(testDb, user.id, { name: 'Fresh' }); - // Populate the identity map with a narrower read first. - await tags.findByIdAndUser(tag.id, user.id); + // rule 20: the FIRST, wider setup read passes `disableIdentityMap: + // false` explicitly and carries the column the later write targets + // (`name`) — `find({})` with the base default merged in populates + // nothing. + await t.repo(Tags).find({}, { disableIdentityMap: false }); attach(tag.id, place.id); testDb.prepare('UPDATE tags SET name = ? WHERE id = ?').run('Renamed', tag.id); const rows = await tags.listForPlaces([place.id]); @@ -327,3 +330,58 @@ describe('TagsRepository', () => { }); }); }); + +// --------------------------------------------------------------------------- +// Plan 3c Task 4 — findByIds (PL2) and the place_tags pivot writes (PL5/PL12/PL13). +// --------------------------------------------------------------------------- + +describe('TagsRepository.findByIds (PL2)', () => { + it('FINDBYIDSREPO-001: returns id + user_id for the matching ids, empty input short-circuits', async () => { + const { user: ownerA } = createUser(testDb); + const { user: ownerB } = createUser(testDb, { username: 'owner_b' }); + const tagA = createTag(testDb, ownerA.id, { name: 'A' }); + const tagB = createTag(testDb, ownerB.id, { name: 'B' }); + const rows = await tags.findByIds([tagA.id, tagB.id, 999999]); + expect(rows.map((r) => ({ id: r.id, user_id: r.user_id })).sort((a, b) => a.id - b.id)).toEqual( + [{ id: tagA.id, user_id: ownerA.id }, { id: tagB.id, user_id: ownerB.id }].sort((a, b) => a.id - b.id), + ); + expect(await tags.findByIds([])).toEqual([]); + }); +}); + +describe('TagsRepository.insertIgnore / deleteForPlace (PL5/PL12/PL13 — place_tags pivot)', () => { + it('PLACETAGSREPO-001: insertIgnore attaches every listed tag, ignoring a duplicate pair', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + const tagA = createTag(testDb, user.id, { name: 'A' }); + const tagB = createTag(testDb, user.id, { name: 'B' }); + await tags.insertIgnore(place.id, [tagA.id, tagB.id]); + // Re-run with an overlapping id — the pre-existing pair must not error or duplicate. + await tags.insertIgnore(place.id, [tagA.id]); + const rows = testDb.prepare('SELECT tag_id FROM place_tags WHERE place_id = ? ORDER BY tag_id').all(place.id) as { tag_id: number }[]; + expect(rows.map((r) => r.tag_id).sort((a, b) => a - b)).toEqual([tagA.id, tagB.id].sort((a, b) => a - b)); + }); + + it('PLACETAGSREPO-002: insertIgnore with an empty array is a no-op', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + await tags.insertIgnore(place.id, []); + const rows = testDb.prepare('SELECT tag_id FROM place_tags WHERE place_id = ?').all(place.id); + expect(rows).toEqual([]); + }); + + it('PLACETAGSREPO-003: deleteForPlace removes every row for that place, leaving other places\' tags alone', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + const other = createPlace(testDb, trip.id); + const tag = createTag(testDb, user.id, { name: 'Shared' }); + await tags.insertIgnore(place.id, [tag.id]); + await tags.insertIgnore(other.id, [tag.id]); + await tags.deleteForPlace(place.id); + expect(testDb.prepare('SELECT * FROM place_tags WHERE place_id = ?').all(place.id)).toEqual([]); + expect(testDb.prepare('SELECT * FROM place_tags WHERE place_id = ?').all(other.id)).toHaveLength(1); + }); +}); diff --git a/server/tests/unit/nest/assignments.service.test.ts b/server/tests/unit/nest/assignments.service.test.ts index 2ecffb3ef..a4396191b 100644 --- a/server/tests/unit/nest/assignments.service.test.ts +++ b/server/tests/unit/nest/assignments.service.test.ts @@ -173,6 +173,60 @@ describe('dayExists / placeExists / assignmentExistsInDay', () => { expect(await svc.assignmentExistsInDay(a.id, otherDay.id, trip.id)).toBe(false); expect(await svc.assignmentExistsInDay(a.id, day.id, trip.id + 1)).toBe(false); }); + + // Task 3 review H1 (live regression, fixed and absorbed in Task 4): these + // four gates used to bind a non-canonical id (`" "`, `".0"`, + // `"+"`) straight into their own read, which SQLite's text/integer + // affinity matches — so the gate answered "found" for an id `toRowId` + // (the SAME conversion the write behind each gate applies) rejects as + // `null`. Each case below proves the gate itself now says "not found" for + // exactly the id shapes `toRowId` rejects, so the mutation behind it can + // never be reached with a mismatched id. + describe('H1 — non-canonical ids resolve to not-found in the gate itself', () => { + it('ASG-SVC-042: dayExists rejects " "/".0"/"+", not just non-numeric ids', async () => { + const { trip, day } = fixture(); + expect(await svc.dayExists(`${day.id} `, trip.id)).toBe(false); + expect(await svc.dayExists(`${day.id}.0`, trip.id)).toBe(false); + expect(await svc.dayExists(`+${day.id}`, trip.id)).toBe(false); + expect(await svc.dayExists('abc', trip.id)).toBe(false); + // The canonical shape still passes, proving the fix didn't just make everything false. + expect(await svc.dayExists(String(day.id), trip.id)).toBe(true); + }); + + it('ASG-SVC-043: placeExists rejects " "/".0"/"+"', async () => { + const { trip, place } = fixture(); + expect(await svc.placeExists(`${place.id} `, trip.id)).toBe(false); + expect(await svc.placeExists(`${place.id}.0`, trip.id)).toBe(false); + expect(await svc.placeExists(`+${place.id}`, trip.id)).toBe(false); + expect(await svc.placeExists(String(place.id), trip.id)).toBe(true); + }); + + it('ASG-SVC-044: assignmentExistsInDay rejects a non-canonical assignment id', async () => { + const { trip, day, place } = fixture(); + const a = createDayAssignment(testDb, day.id, place.id); + expect(await svc.assignmentExistsInDay(`${a.id} `, day.id, trip.id)).toBe(false); + expect(await svc.assignmentExistsInDay(`${a.id}.0`, day.id, trip.id)).toBe(false); + expect(await svc.assignmentExistsInDay(String(a.id), day.id, trip.id)).toBe(true); + }); + + it('ASG-SVC-045: getAssignmentForTrip rejects a non-canonical assignment id (undefined, not the row)', async () => { + const { trip, day, place } = fixture(); + const a = createDayAssignment(testDb, day.id, place.id); + expect(await svc.getAssignmentForTrip(`${a.id} `, trip.id)).toBeUndefined(); + expect(await svc.getAssignmentForTrip(`${a.id}.0`, trip.id)).toBeUndefined(); + expect(await svc.getAssignmentForTrip(String(a.id), trip.id)).toMatchObject({ id: a.id }); + }); + + it('ASG-SVC-046: deleteAssignment is never reached with a mismatched id — the controller-shape gate-then-write leaves the row intact for a non-canonical id', async () => { + const { trip, day, place } = fixture(); + const a = createDayAssignment(testDb, day.id, place.id); + // The controller's own shape: gate, then act only if the gate passed. + const gate = await svc.assignmentExistsInDay(`${a.id} `, day.id, trip.id); + expect(gate).toBe(false); + if (gate) await svc.deleteAssignment(`${a.id} `); + expect(await svc.getAssignmentForTrip(String(a.id), trip.id)).toMatchObject({ id: a.id }); + }); + }); }); // ── createAssignment ────────────────────────────────────────────────────────── diff --git a/server/tests/unit/nest/places.directions.test.ts b/server/tests/unit/nest/places.directions.test.ts index 2b4b52d60..a35876570 100644 --- a/server/tests/unit/nest/places.directions.test.ts +++ b/server/tests/unit/nest/places.directions.test.ts @@ -54,7 +54,7 @@ import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/plac import { JourneyDomainService } from '../../../src/nest/journey/journey-domain.service'; import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; import { makeStorageFixture } from '../../helpers/storage-fixture'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, sharedTestOrm } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestPlacesRepo, createTestTripMembersRepo, createTestDayAssignmentsRepo, sharedTestOrm } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const photoCacheStub = { removeIfUnreferenced: vi.fn() } as unknown as PlacePhotoCacheService; @@ -92,6 +92,11 @@ async function svc(searchNominatim: MapsService['searchNominatim']): Promise { const trip = createTrip(testDb, user.id); expect(await svc.get(String(trip.id), '99999')).toBeNull(); }); + + // Task 3 review H1's lesson, applied to every place-id gate this task + // converts: a non-canonical id shape (`" "`, `".0"`, `"+"`) + // must resolve to not-found HERE, in the gate — SQLite's own affinity + // would otherwise match it, and this service's writes downstream convert + // with `toRowId`, which rejects the same shapes. + it('PLACE-SVC-012b (H1) — non-canonical id shapes (" ", ".0", "+") also return null, not just non-numeric ids', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + expect(await svc.get(String(trip.id), `${place.id} `)).toBeNull(); + expect(await svc.get(String(trip.id), `${place.id}.0`)).toBeNull(); + expect(await svc.get(String(trip.id), `+${place.id}`)).toBeNull(); + // The canonical shape still finds it, proving the fix didn't just null everything. + expect(await svc.get(String(trip.id), String(place.id))).not.toBeNull(); + }); }); // ── update ──────────────────────────────────────────────────────────────────── @@ -316,6 +337,16 @@ describe('update', () => { expect(await svc.update(String(trip.id), '99999', { name: 'Ghost' })).toBeNull(); }); + it('PLACE-SVC-014b (H1) — a non-canonical id (" ") is null, not a write: the row is left untouched', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Untouched' }); + const result = await svc.update(String(trip.id), `${place.id} `, { name: 'Clobbered' }); + expect(result).toBeNull(); + const row = testDb.prepare('SELECT name FROM places WHERE id = ?').get(place.id) as { name: string }; + expect(row.name).toBe('Untouched'); + }); + it('PLACE-SVC-015 — updates tags (replaces old set)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -442,6 +473,15 @@ describe('remove', () => { expect((await svc.remove(String(trip.id), '99999')).deleted).toBe(false); }); + it('PLACE-SVC-018b (H1) — a non-canonical id (" ") is not deleted: deleted:false, row intact', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Survives' }); + const result = await svc.remove(String(trip.id), `${place.id} `); + expect(result.deleted).toBe(false); + expect(await svc.get(String(trip.id), String(place.id))).not.toBeNull(); + }); + it('PLACE-SVC-019 — deleting one place does not remove others', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -952,6 +992,15 @@ describe('searchImage', () => { expect(result.status).toBe(404); }); + it('PLACE-SVC-030b (H1) — a non-canonical id (" ") also 404s', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + const result = await svc.searchImage(String(trip.id), `${place.id} `, user.id) as any; + expect(result.error).toBeDefined(); + expect(result.status).toBe(404); + }); + it('PLACE-SVC-031 — searches Unsplash without a stored API key', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -1160,6 +1209,34 @@ describe('custom place image reclaim', () => { const count = testDb.prepare('SELECT COUNT(*) AS n FROM place_ratings WHERE place_id = ?').get(place.id) as { n: number }; expect(count.n).toBe(0); }); + + it('PLACE-SVC-051b (H1) — a non-canonical id (" ") is null and writes nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id) as { id: number }; + expect(await svc.rate(String(trip.id), `${place.id} `, user.id, 4)).toBeNull(); + const count = testDb.prepare('SELECT COUNT(*) AS n FROM place_ratings WHERE place_id = ?').get(place.id) as { n: number }; + expect(count.n).toBe(0); + }); + + // PL51's ruling: an ON CONFLICT DO UPDATE limited to `rating` must never + // touch `places.updated_at` — a vote must not 409 another member's + // offline edit (#1435). Proven directly against the column, not just + // against the response shape. + it('PLACE-SVC-051c (PL51) — rating a place never bumps places.updated_at', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Silent Vote' }) as { id: number }; + const before = (testDb.prepare('SELECT updated_at FROM places WHERE id = ?').get(place.id) as { updated_at: string | null }).updated_at; + + await svc.rate(String(trip.id), String(place.id), user.id, 5); + // Re-vote (the ON CONFLICT DO UPDATE path) — the one statement in the + // whole cluster that could plausibly touch `updated_at` via a careless + // merge-field list. + await svc.rate(String(trip.id), String(place.id), user.id, 2); + const after = (testDb.prepare('SELECT updated_at FROM places WHERE id = ?').get(place.id) as { updated_at: string | null }).updated_at; + expect(after).toBe(before); + }); }); // ── Automatic track colours (#776) ──────────────────────────────────────────── diff --git a/server/tests/unit/nest/roadtrip-planning.test.ts b/server/tests/unit/nest/roadtrip-planning.test.ts index 510bb60a7..216ae24a1 100644 --- a/server/tests/unit/nest/roadtrip-planning.test.ts +++ b/server/tests/unit/nest/roadtrip-planning.test.ts @@ -248,7 +248,7 @@ describe('Roadtrip MCP registration and search', () => { const mcp = new PlacesMcp( places as never, {} as never, - { canAccessTrip: () => true } as never, + { findAccessible: () => ({ id: 1, user_id: 1, currency: null }) } as never, auth as never, {} as never, {} as never, diff --git a/server/tests/unit/services/conflictUpdate.test.ts b/server/tests/unit/services/conflictUpdate.test.ts index 5e6db3487..70767ba14 100644 --- a/server/tests/unit/services/conflictUpdate.test.ts +++ b/server/tests/unit/services/conflictUpdate.test.ts @@ -63,6 +63,8 @@ import { createTestAssignmentParticipantsRepo, createTestGooglePlacePhotoMetaRepo, createTestPlacesRepo, + createTestTripMembersRepo, + createTestDayAssignmentsRepo, sharedTestOrm, } from '../../helpers/test-uow'; @@ -105,6 +107,11 @@ beforeAll(async () => { new JourneyDomainService(dbs, realtime, new TrekPhotosRepository(dbs), await createTestUnitOfWork(dbs.connection)), makeStorageFixture('').storage, await accommodationsOver(dbs), await createTestUnitOfWork(dbs.connection), + await createTestPlacesRepo(dbs.connection), + await createTestTagsRepo(dbs.connection), + await createTestPlaceRatingsRepo(dbs.connection), + await createTestTripMembersRepo(dbs.connection), + await createTestDayAssignmentsRepo(dbs.connection), ); });