diff --git a/server/src/db/repositories/CollectionLabels.repository.ts b/server/src/db/repositories/CollectionLabels.repository.ts index cdb3d7be1..2231cd75d 100644 --- a/server/src/db/repositories/CollectionLabels.repository.ts +++ b/server/src/db/repositories/CollectionLabels.repository.ts @@ -1,4 +1,76 @@ import type { CollectionLabels } from '../entities/CollectionLabels.entity'; import { TrekRepository } from './_shared/trek-repository'; -export class CollectionLabelsRepository extends TrekRepository {} +/** `collection_labels` — `loadLabelsByCollection`'s (CL7) shape. */ +export interface CollectionLabelRow { + id: number; + collection_id: number; + name: string; + color: string | null; + sort_order: number | null; +} + +interface CollectionLabelsKyselyDB { + collection_labels: { + id: number; + collection_id: number; + name: string; + color: string | null; + sort_order: number | null; + created_at: string | null; + }; +} + +/** CL20's own `INSERT INTO collection_labels (collection_id, name, color, sort_order)` column list, exactly — no `id`/`created_at`, the `BucketListInsertKyselyDB` narrower-insert-interface precedent. */ +interface CollectionLabelsInsertKyselyDB { + collection_labels: { collection_id: number; name: string; color: string; sort_order: number }; +} + +/** + * `collection_labels` — Plan 3h Task 1's first cut. Owns Part A's own + * label reads/insert (CL7, `labelIdsForFile`'s CL18/CL19, `insertImportedLabel`'s + * CL20). Task 2 extends this repository with `createLabel`/`updateLabel`/ + * `deleteLabel`/`collectionIdOfLabel`/`getLabelById` (CL71-77, beyond this + * task's CL1-36 range). + */ +export class CollectionLabelsRepository extends TrekRepository { + private db_() { + return this.kysely(); + } + + /** CL7 (`loadLabelsByCollection`) — `SELECT id, collection_id, name, color, sort_order FROM collection_labels WHERE collection_id=? ORDER BY sort_order, id`. */ + async listByCollection(collectionId: number): Promise { + return await this.db_() + .selectFrom('collection_labels') + .select(['id', 'collection_id', 'name', 'color', 'sort_order']) + .where('collection_id', '=', collectionId) + .orderBy('sort_order') + .orderBy('id') + .execute(); + } + + /** CL18 (`labelIdsForFile`) — `SELECT id, name FROM collection_labels WHERE collection_id=?`. */ + async idNameByCollection(collectionId: number): Promise<{ id: number; name: string }[]> { + return await this.db_() + .selectFrom('collection_labels') + .select(['id', 'name']) + .where('collection_id', '=', collectionId) + .execute(); + } + + /** CL19 (`labelIdsForFile`) — `SELECT COALESCE(MAX(sort_order),-1) AS m FROM collection_labels WHERE collection_id=?`. Dup text also `createLabel` (Task 2's own call site, CL73's neighbor). */ + async maxSortOrder(collectionId: number): Promise { + const row = await this.db_() + .selectFrom('collection_labels') + .select((eb) => eb.fn.coalesce(eb.fn.max('sort_order'), eb.val(-1)).as('m')) + .where('collection_id', '=', collectionId) + .executeTakeFirstOrThrow(); + return Number(row.m); + } + + /** CL20 (`insertImportedLabel`) — `INSERT INTO collection_labels (collection_id, name, color, sort_order) VALUES (?,?,?,?)`. Returns the new row's id. Dup text also `createLabel` (Task 2's own call site). */ + async insertLabel(row: { collection_id: number; name: string; color: string; sort_order: number }): Promise { + const result = await this.kysely().insertInto('collection_labels').values(row).executeTakeFirstOrThrow(); + return Number(result.insertId); + } +} diff --git a/server/src/db/repositories/CollectionMembers.repository.ts b/server/src/db/repositories/CollectionMembers.repository.ts index f0790f846..22127720d 100644 --- a/server/src/db/repositories/CollectionMembers.repository.ts +++ b/server/src/db/repositories/CollectionMembers.repository.ts @@ -1,4 +1,114 @@ import type { CollectionMembers } from '../entities/CollectionMembers.entity'; import { TrekRepository } from './_shared/trek-repository'; -export class CollectionMembersRepository extends TrekRepository {} +/** `buildMembers`'s owner row — `collections` joined to `users` (CL12). */ +export interface CollectionOwnerMemberRow { + user_id: number; + username: string; + email: string; + avatar: string | null; +} + +/** `buildMembers`'s accepted/pending member rows — `collection_members` joined to `users` (CL13). */ +export interface CollectionMemberRow extends CollectionOwnerMemberRow { + status: string; + role: string; +} + +interface CollectionOwnerKyselyDB { + collections: { id: number; owner_id: number }; + users: { id: number; username: string; email: string; avatar: string | null }; +} + +interface CollectionMembersKyselyDB { + collection_members: { collection_id: number; user_id: number; status: string; role: string }; + users: { id: number; username: string; email: string; avatar: string | null }; +} + +interface CollectionMemberUserIdsKyselyDB { + collection_members: { collection_id: number; user_id: number; status: string }; +} + +/** `listCollections`'s incoming-invites read — a genuine statement missing + * its own CL number in the plan's inventory gather (jumped CL15 → CL16 + * without capturing it): `SELECT cm.collection_id, c.name, u.id AS from_id, + * u.username AS from_username FROM collection_members cm JOIN collections c + * ON c.id=cm.collection_id JOIN users u ON u.id=c.owner_id WHERE + * cm.user_id=? AND cm.status='pending'`. */ +export interface CollectionIncomingInviteRow { + collection_id: number; + name: string; + from_id: number; + from_username: string; +} + +interface CollectionIncomingInvitesKyselyDB { + collection_members: { collection_id: number; user_id: number; status: string }; + collections: { id: number; name: string; owner_id: number }; + users: { id: number; username: string }; +} + +/** + * `collection_members` — Plan 3h Task 1's first cut (`roleOf`'s own member + * lookup lives in `_shared/collection-role.ts` instead, R6). Task 2 extends + * this repository with `sendInvite`/`acceptInvite`/etc.'s invite-lifecycle + * methods (CL79-88) and the membership-lookup surface (CL69, `findMembership`). + * Owns `buildMembers`'s two joined reads (CL12/CL13) and `deleteCollection`'s + * snapshot-before-cascade reads (CL33/CL34). + */ +export class CollectionMembersRepository extends TrekRepository { + /** CL12 (`buildMembers`) — `SELECT u.id AS user_id, u.username, u.email, u.avatar FROM collections col JOIN users u ON u.id=col.owner_id WHERE col.id=?`. */ + async ownerRow(collectionId: number): Promise { + return await this.kysely() + .selectFrom('collections as col') + .innerJoin('users as u', 'u.id', 'col.owner_id') + .select(['u.id as user_id', 'u.username', 'u.email', 'u.avatar']) + .where('col.id', '=', collectionId) + .executeTakeFirst(); + } + + /** CL13 (`buildMembers`) — `SELECT u.id AS user_id, u.username, u.email, u.avatar, cm.status, cm.role FROM collection_members cm JOIN users u ON u.id=cm.user_id WHERE cm.collection_id=? ORDER BY u.username`. */ + async memberRows(collectionId: number): Promise { + return await this.kysely() + .selectFrom('collection_members as cm') + .innerJoin('users as u', 'u.id', 'cm.user_id') + .select(['u.id as user_id', 'u.username', 'u.email', 'u.avatar', 'cm.status', 'cm.role']) + .where('cm.collection_id', '=', collectionId) + .orderBy('u.username') + .execute(); + } + + /** CL33 (`deleteCollection`'s snapshot, taken BEFORE the cascading delete) — `SELECT user_id FROM collection_members WHERE collection_id=? AND status='accepted'`. Dup text also `collectionMemberIds`/`notifyCollectionUsers` (Task 2's own call sites). */ + async acceptedUserIds(collectionId: number): Promise { + const rows = await this.kysely() + .selectFrom('collection_members') + .select('user_id') + .where('collection_id', '=', collectionId) + .where('status', '=', 'accepted') + .execute(); + return rows.map((r) => r.user_id); + } + + /** CL34 (`deleteCollection`'s snapshot, taken BEFORE the cascading delete) — `SELECT user_id FROM collection_members WHERE collection_id=? AND status='pending'`. */ + async pendingUserIds(collectionId: number): Promise { + const rows = await this.kysely() + .selectFrom('collection_members') + .select('user_id') + .where('collection_id', '=', collectionId) + .where('status', '=', 'pending') + .execute(); + return rows.map((r) => r.user_id); + } + + /** `listCollections`'s incoming-invites read (see {@link CollectionIncomingInviteRow}'s docstring). */ + async pendingInvitesForUser(userId: number): Promise { + return await this.kysely() + .selectFrom('collection_members as cm') + .innerJoin('collections as c', 'c.id', 'cm.collection_id') + .innerJoin('users as u', 'u.id', 'c.owner_id') + .select(['cm.collection_id', 'c.name', 'u.id as from_id', 'u.username as from_username']) + .where('cm.user_id', '=', userId) + .where('cm.status', '=', 'pending') + .execute(); + } +} diff --git a/server/src/db/repositories/Collections.repository.ts b/server/src/db/repositories/Collections.repository.ts index 8c28d3e20..97f956986 100644 --- a/server/src/db/repositories/Collections.repository.ts +++ b/server/src/db/repositories/Collections.repository.ts @@ -1,4 +1,504 @@ import type { Collections } from '../entities/Collections.entity'; import { TrekRepository } from './_shared/trek-repository'; -export class CollectionsRepository extends TrekRepository {} +/** `collections` — every scalar column, `getCollectionRow`'s (CL14) `SELECT *` shape. */ +export interface CollectionRow { + id: number; + owner_id: number; + name: string; + description: string | null; + color: string | null; + icon: string | null; + cover_image: string | null; + links: string | null; + sort_order: number | null; + created_at: string | null; + updated_at: string | null; +} + +/** + * A saved place row, `cp.*` joined to its category name/color/icon — CL10 + * (`getPlaceById`, filtered by `cp.id`) and CL16/CL17 (`getCollection`/ + * `exportCollection`'s dup-text pair, filtered by `cp.collection_id`) both + * select this exact shape. `links` stays the raw TEXT column (`string | + * null`) here — never `CollectionLink[]` — the service's own `parseLinks` + * does that parsing on the way out, the same reason `CollectionRow.links` + * below is also raw TEXT: a hand-written row interface documents the DB + * shape, not the wire shape (`_shared/rows.ts`'s docstring). + */ +export interface CollectionPlaceRow { + id: number; + collection_id: number; + owner_id: number; + saved_by: number | null; + name: string; + description: string | null; + lat: number | null; + lng: number | null; + address: string | null; + category_id: number | null; + price: number | null; + currency: string | null; + notes: string | null; + image_url: string | null; + google_place_id: string | null; + google_ftid: string | null; + osm_id: string | null; + website: string | null; + phone: string | null; + status: string; + source_trip_id: number | null; + source_place_id: number | null; + links: string | null; + sort_order: number | null; + created_at: string | null; + updated_at: string | null; + category_name: string | null; + category_color: string | null; + category_icon: string | null; +} + +interface CollectionsKyselyDB { + collections: CollectionRow; +} + +/** + * CL29's own `INSERT INTO collections (owner_id, name, description, color, + * icon, cover_image, links, sort_order)` column list, exactly — no `id`/ + * `created_at`/`updated_at` (never bound; the schema's own + * `DEFAULT CURRENT_TIMESTAMP`/autoincrement fill them), the + * `BucketListRepository`/`BucketListInsertKyselyDB` precedent for a + * narrower insert-only Kysely interface instead of `Generated<>` markers. + */ +interface CollectionsInsertKyselyDB { + collections: { + owner_id: number; + name: string; + description: string | null; + color: string | null; + icon: string | null; + cover_image: string | null; + links: string | null; + sort_order: number; + }; +} + +interface CollectionIdsKyselyDB { + collections: { id: number; owner_id: number }; + collection_members: { collection_id: number; user_id: number; status: string }; +} + +interface CollectionPlacesReadKyselyDB { + collection_places: Omit; + categories: { id: number; name: string; color: string | null; icon: string | null }; +} + +/** + * CL23's own `INSERT INTO collection_places (21 cols)` list, exactly — no + * `id`/`created_at`/`updated_at`, the same narrower-insert-interface shape + * as {@link CollectionsInsertKyselyDB} above. + */ +interface CollectionPlacesWriteKyselyDB { + collection_places: { + collection_id: number; + owner_id: number; + saved_by: number | null; + name: string; + description: string | null; + lat: number | null; + lng: number | null; + address: string | null; + category_id: number | null; + price: number | null; + currency: string | null; + notes: string | null; + image_url: string | null; + google_place_id: string | null; + google_ftid: string | null; + osm_id: string | null; + website: string | null; + phone: string | null; + status: string; + links: string | null; + sort_order: number | null; + }; +} + +interface CollectionPlaceLabelsKyselyDB { + collection_place_labels: { collection_place_id: number; label_id: number }; +} + +/** + * `collections` — the root of the collections cluster (Plan 3h Task 1, part + * A). Owns every CL1-CL36 statement not moved to `CollectionMembersRepository`/ + * `CollectionLabelsRepository`: the `collections` table's own CRUD/reorder, + * plus — as a pragmatic first cut, since `CollectionPlacesRepository`/ + * `CollectionPlaceTagsRepository`/`CollectionPlaceRatingsRepository` don't + * exist yet (Task 2 builds them) — every hydration/export/import/dedup + * statement Part A's own methods (`hydratePlaces`, `getPlaceById`, + * `getCollection`/`exportCollection`, `writeFilePlaces`, + * `findDuplicateCollectionPlace`) need against `collection_places`/ + * `collection_place_tags`/`collection_place_labels`/`collection_place_ratings`. + * Task 2's own report should note whether any of these bridge methods move + * onto its new, more specific repositories once they exist — see this + * task's own report for the explicit flag. + */ +export class CollectionsRepository extends TrekRepository { + private readDb() { + return this.kysely(); + } + + private insertDb() { + return this.kysely(); + } + + private idsDb() { + return this.kysely(); + } + + private placesReadDb() { + return this.kysely(); + } + + private placesWriteDb() { + return this.kysely(); + } + + private placeLabelsDb() { + return this.kysely(); + } + + /** + * CL1 (`accessibleCollectionIds`) — `SELECT id FROM collections WHERE + * owner_id=? UNION SELECT collection_id FROM collection_members WHERE + * user_id=? AND status='accepted'`. Two typed selects, merged and + * deduplicated in TS (`Set`) the same way SQL's `UNION` (not `UNION ALL`) + * would — the legacy statement is `UNION`, so a row present in both halves + * collapses to one id either way. + */ + async accessibleCollectionIds(userId: number): Promise { + const owned = await this.idsDb().selectFrom('collections').select('id').where('owner_id', '=', userId).execute(); + const member = await this.idsDb() + .selectFrom('collection_members') + .select('collection_id as id') + .where('user_id', '=', userId) + .where('status', '=', 'accepted') + .execute(); + return [...new Set([...owned.map((r) => r.id), ...member.map((r) => r.id)])]; + } + + /** CL5 (`ownerOf`) — `SELECT owner_id FROM collections WHERE id=?`. */ + async ownerId(id: number): Promise { + const row = await this.readDb().selectFrom('collections').select('owner_id').where('id', '=', id).executeTakeFirst(); + return row?.owner_id; + } + + /** CL14 (`getCollectionRow`) — `SELECT * FROM collections WHERE id=?`. */ + async findRow(id: number): Promise { + return await this.readDb().selectFrom('collections').selectAll().where('id', '=', id).executeTakeFirst(); + } + + /** CL15 (`getCollectionRow`) — `SELECT COUNT(*) AS n FROM collection_places WHERE collection_id=?`. */ + async placeCount(collectionId: number): Promise { + const row = await this.placesReadDb() + .selectFrom('collection_places') + .select((eb) => eb.fn.countAll().as('n')) + .where('collection_id', '=', collectionId) + .executeTakeFirstOrThrow(); + return Number(row.n); + } + + /** CL28 (`createCollection`) — `SELECT COALESCE(MAX(sort_order),-1) AS m FROM collections WHERE owner_id=?`. */ + async maxSortOrder(ownerId: number): Promise { + const row = await this.readDb() + .selectFrom('collections') + .select((eb) => eb.fn.coalesce(eb.fn.max('sort_order'), eb.val(-1)).as('m')) + .where('owner_id', '=', ownerId) + .executeTakeFirstOrThrow(); + return Number(row.m); + } + + /** + * CL29 (`createCollection`) — `INSERT INTO collections (owner_id, name, + * description, color, icon, cover_image, links, sort_order) VALUES (...)`. + * Returns the new row's id. + */ + async insertCollection(row: { + owner_id: number; + name: string; + description: string | null; + color: string | null; + icon: string | null; + cover_image: string | null; + links: string | null; + sort_order: number; + }): Promise { + const result = await this.insertDb().insertInto('collections').values(row).executeTakeFirstOrThrow(); + return Number(result.insertId); + } + + /** + * CL30 (`updateCollection`) — the allow-listed dynamic `UPDATE collections + * SET ... WHERE id=?`. `write` carries exactly the columns the caller + * built (the service resolves `body.field !== undefined` itself, same + * shape as every other repository's `presenceSet`-fed write) — an empty + * `write` short-circuits before the `UPDATE`, matching the legacy + * `if (updates.length > 0)` guard. + */ + async updateFields( + id: number, + write: Partial<{ + name: string; + description: string | null; + color: string | null; + icon: string | null; + cover_image: string | null; + links: string | null; + sort_order: number; + }>, + ): Promise { + if (Object.keys(write).length === 0) return; + await this.readDb().updateTable('collections').set({ ...write, updated_at: new Date().toISOString() }).where('id', '=', id).execute(); + } + + /** CL31 (`setCollectionCover`) — `SELECT cover_image FROM collections WHERE id=?`. */ + async coverImage(id: number): Promise { + const row = await this.readDb().selectFrom('collections').select('cover_image').where('id', '=', id).executeTakeFirst(); + return row?.cover_image; + } + + /** CL32 (`setCollectionCover`) — `UPDATE collections SET cover_image=?, updated_at=CURRENT_TIMESTAMP WHERE id=?`. */ + async setCoverImage(id: number, coverUrl: string | null): Promise { + await this.readDb().updateTable('collections').set({ cover_image: coverUrl, updated_at: new Date().toISOString() }).where('id', '=', id).execute(); + } + + /** CL35 (`deleteCollection`) — `DELETE FROM collections WHERE id=?` (CASCADE drops members + places + tags). */ + async deleteById(id: number): Promise { + await this.readDb().deleteFrom('collections').where('id', '=', id).execute(); + } + + /** + * CL36 (`reorderCollections`, TX `:712`) — `UPDATE collections SET + * sort_order=? WHERE id=?`, looped per visible id. The legacy statement + * is a `this.db.prepare(...)` reused across the loop (a single compiled + * statement, `.run()` called once per iteration) — Kysely's query builder + * has no equivalent "compile once, bind many" primitive for an UPDATE + * with per-row values, so this issues one `updateTable` call per pair + * instead (informational for Task 7: the "prepare once, loop `.run()`" + * shape does NOT survive this conversion, per-call is what the repository + * layer supports here). + */ + async setSortOrders(pairs: { id: number; sortOrder: number }[]): Promise { + const db = this.readDb(); + for (const { id, sortOrder } of pairs) { + await db.updateTable('collections').set({ sort_order: sortOrder }).where('id', '=', id).execute(); + } + } + + // ------------------------------------------------------------------------- + // Hydration / export / import / dedup bridge methods — CL6, CL8-CL11, + // CL16/17 (collapsed), CL22-CL27. First-cut home: these touch + // `collection_places`/`collection_place_tags`/`collection_place_labels`/ + // `collection_place_ratings`, tables Task 2's own repositories will own + // going forward, but Part A's `hydratePlaces`/`getPlaceById`/ + // `getCollection`/`exportCollection`/`writeFilePlaces`/ + // `findDuplicateCollectionPlace` are its own methods and need somewhere to + // land now. + // ------------------------------------------------------------------------- + + /** CL10 (`getPlaceById`) — `SELECT cp.*, c.name/color/icon AS category_* FROM collection_places cp LEFT JOIN categories c ON cp.category_id=c.id WHERE cp.id=?`. */ + async findPlaceRowById(id: number): Promise { + return await this.placesReadDb() + .selectFrom('collection_places as cp') + .leftJoin('categories as c', 'c.id', 'cp.category_id') + .select([ + 'cp.id', 'cp.collection_id', 'cp.owner_id', 'cp.saved_by', 'cp.name', 'cp.description', 'cp.lat', 'cp.lng', + 'cp.address', 'cp.category_id', 'cp.price', 'cp.currency', 'cp.notes', 'cp.image_url', 'cp.google_place_id', + 'cp.google_ftid', 'cp.osm_id', 'cp.website', 'cp.phone', 'cp.status', 'cp.source_trip_id', 'cp.source_place_id', + 'cp.links', 'cp.sort_order', 'cp.created_at', 'cp.updated_at', + 'c.name as category_name', 'c.color as category_color', 'c.icon as category_icon', + ]) + .where('cp.id', '=', id) + .executeTakeFirst(); + } + + /** + * CL16/CL17 (`getCollection`/`exportCollection`'s dup-text pair, collapsed + * onto ONE repository method as the brief directs) — the same + * `cp.*`/category-alias select as {@link findPlaceRowById}, scoped by + * `cp.collection_id` and ordered by `cp.sort_order, cp.created_at`. + */ + async listPlaceRows(collectionId: number): Promise { + return await this.placesReadDb() + .selectFrom('collection_places as cp') + .leftJoin('categories as c', 'c.id', 'cp.category_id') + .select([ + 'cp.id', 'cp.collection_id', 'cp.owner_id', 'cp.saved_by', 'cp.name', 'cp.description', 'cp.lat', 'cp.lng', + 'cp.address', 'cp.category_id', 'cp.price', 'cp.currency', 'cp.notes', 'cp.image_url', 'cp.google_place_id', + 'cp.google_ftid', 'cp.osm_id', 'cp.website', 'cp.phone', 'cp.status', 'cp.source_trip_id', 'cp.source_place_id', + 'cp.links', 'cp.sort_order', 'cp.created_at', 'cp.updated_at', + 'c.name as category_name', 'c.color as category_color', 'c.icon as category_icon', + ]) + .where('cp.collection_id', '=', collectionId) + .orderBy('cp.sort_order') + .orderBy('cp.created_at') + .execute(); + } + + /** CL11 (`collectionIdOfPlace`) — `SELECT collection_id FROM collection_places WHERE id=?`. */ + async collectionIdOfPlace(placeId: number): Promise { + const row = await this.placesReadDb().selectFrom('collection_places').select('collection_id').where('id', '=', placeId).executeTakeFirst(); + return row?.collection_id; + } + + /** + * CL6 (`loadTagsByCollectionPlaceIds`) — `SELECT cpt.collection_place_id + * AS pid, t.id, t.name, t.color FROM collection_place_tags cpt JOIN tags t + * ON t.id=cpt.tag_id WHERE cpt.collection_place_id IN (...)`. Empty input + * short-circuits to `[]` (no `IN ()` sent) — the service's own batched + * hydration helper already guards this, but the repository guards too so + * it is safe to call directly. + */ + async loadTagsByPlaceIds(placeIds: number[]): Promise<{ pid: number; id: number; name: string; color: string }[]> { + if (placeIds.length === 0) return []; + return await this.kysely<{ collection_place_tags: { collection_place_id: number; tag_id: number }; tags: { id: number; name: string; color: string } }>() + .selectFrom('collection_place_tags as cpt') + .innerJoin('tags as t', 't.id', 'cpt.tag_id') + .select(['cpt.collection_place_id as pid', 't.id', 't.name', 't.color']) + .where('cpt.collection_place_id', 'in', placeIds) + .execute(); + } + + /** CL8 (`loadLabelIdsByPlaceIds`) — `SELECT collection_place_id AS pid, label_id FROM collection_place_labels WHERE collection_place_id IN (...)`. */ + async loadLabelIdsByPlaceIds(placeIds: number[]): Promise<{ pid: number; label_id: number }[]> { + if (placeIds.length === 0) return []; + const rows = await this.placeLabelsDb() + .selectFrom('collection_place_labels') + .select(['collection_place_id as pid', 'label_id']) + .where('collection_place_id', 'in', placeIds) + .execute(); + return rows; + } + + /** + * CL9 (`loadRatingsByCollectionPlaceIds`, #1435) — `SELECT + * cpr.collection_place_id AS pid, cpr.user_id, u.username, u.avatar, + * cpr.rating FROM collection_place_ratings cpr JOIN users u ON + * u.id=cpr.user_id WHERE cpr.collection_place_id IN (...) ORDER BY + * cpr.created_at`. + */ + async loadRatingsByPlaceIds( + placeIds: number[], + ): Promise<{ pid: number; user_id: number; username: string; avatar: string | null; rating: number }[]> { + if (placeIds.length === 0) return []; + return await this.kysely<{ + collection_place_ratings: { collection_place_id: number; user_id: number; rating: number; created_at: string | null }; + users: { id: number; username: string; avatar: string | null }; + }>() + .selectFrom('collection_place_ratings as cpr') + .innerJoin('users as u', 'u.id', 'cpr.user_id') + .select(['cpr.collection_place_id as pid', 'cpr.user_id', 'u.username', 'u.avatar', 'cpr.rating']) + .where('cpr.collection_place_id', 'in', placeIds) + .orderBy('cpr.created_at') + .execute(); + } + + /** CL22 (`writeFilePlaces`) — `SELECT COALESCE(MAX(sort_order),-1) AS m FROM collection_places WHERE collection_id=?`. */ + async maxPlaceSortOrder(collectionId: number): Promise { + const row = await this.placesReadDb() + .selectFrom('collection_places') + .select((eb) => eb.fn.coalesce(eb.fn.max('sort_order'), eb.val(-1)).as('m')) + .where('collection_id', '=', collectionId) + .executeTakeFirstOrThrow(); + return Number(row.m); + } + + /** + * CL23 (`writeFilePlaces`, PREPARED/looped in the legacy code) — `INSERT + * INTO collection_places (21 cols incl. sort_order, no source_trip_id/ + * source_place_id) VALUES (...)`. Distinct insert shape from CL42/CL46 + * (Task 2's) — never collapsed with either. Returns the new row's id. + */ + async insertFilePlace(row: { + collection_id: number; + owner_id: number; + saved_by: 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; + notes: string | null; + image_url: string | null; + google_place_id: string | null; + google_ftid: string | null; + osm_id: string | null; + website: string | null; + phone: string | null; + status: string; + links: string | null; + sort_order: number; + }): Promise { + const result = await this.placesWriteDb().insertInto('collection_places').values(row).executeTakeFirstOrThrow(); + return Number(result.insertId); + } + + /** CL24 (`writeFilePlaces`'s `assignLabel` prepared statement) — `INSERT OR IGNORE INTO collection_place_labels (collection_place_id, label_id) VALUES (?,?)`. */ + async assignPlaceLabel(collectionPlaceId: number, labelId: number): Promise { + await this.placeLabelsDb() + .insertInto('collection_place_labels') + .values({ collection_place_id: collectionPlaceId, label_id: labelId }) + .onConflict((oc) => oc.doNothing()) + .execute(); + } + + /** CL25 (`findDuplicateCollectionPlace`, externalId branch) — `SELECT id,name FROM collection_places WHERE collection_id=? AND (google_place_id=? OR google_ftid=? OR osm_id=?) ORDER BY id ASC LIMIT 1`. */ + async findDuplicateByExternalId( + collectionId: number, + externalId: string, + ): Promise<{ id: number; name: string } | undefined> { + return await this.placesReadDb() + .selectFrom('collection_places') + .select(['id', 'name']) + .where('collection_id', '=', collectionId) + .where((eb) => eb.or([eb('google_place_id', '=', externalId), eb('google_ftid', '=', externalId), eb('osm_id', '=', externalId)])) + .orderBy('id', 'asc') + .limit(1) + .executeTakeFirst(); + } + + /** CL26 (`findDuplicateCollectionPlace`, name branch) — `SELECT id,name FROM collection_places WHERE collection_id=? AND lower(trim(name))=? ORDER BY id ASC LIMIT 1`. `normalizedName` is already `lower(trim(...))`-shaped by the caller (`_shared/../dialect/sql-functions.ts`'s `lowerTrim`/`lowerTrimParam` precedent). */ + async findDuplicateByName(collectionId: number, normalizedName: string): Promise<{ id: number; name: string } | undefined> { + return await this.placesReadDb() + .selectFrom('collection_places') + .select(['id', 'name']) + .where('collection_id', '=', collectionId) + .where((eb) => eb(eb.fn('lower', [eb.fn('trim', ['name'])]), '=', normalizedName)) + .orderBy('id', 'asc') + .limit(1) + .executeTakeFirst(); + } + + /** CL27 (`findDuplicateCollectionPlace`, coord branch) — `SELECT id,name FROM collection_places WHERE collection_id=? AND lat IS NOT NULL AND lng IS NOT NULL AND abs(lat-?)<=? AND abs(lng-?)<=? ORDER BY id ASC LIMIT 1`. */ + async findDuplicateByCoords( + collectionId: number, + lat: number, + lng: number, + tolerance: number, + ): Promise<{ id: number; name: string } | undefined> { + return await this.placesReadDb() + .selectFrom('collection_places') + .select(['id', 'name']) + .where('collection_id', '=', collectionId) + .where('lat', 'is not', null) + .where('lng', 'is not', null) + .where((eb) => eb(eb.fn('abs', [eb('lat', '-', lat)]), '<=', tolerance)) + .where((eb) => eb(eb.fn('abs', [eb('lng', '-', lng)]), '<=', tolerance)) + .orderBy('id', 'asc') + .limit(1) + .executeTakeFirst(); + } +} diff --git a/server/src/db/repositories/_shared/collection-role.ts b/server/src/db/repositories/_shared/collection-role.ts new file mode 100644 index 000000000..7271c5934 --- /dev/null +++ b/server/src/db/repositories/_shared/collection-role.ts @@ -0,0 +1,128 @@ +import type { ExpressionBuilder, ExpressionWrapper, SqlBool } from 'kysely'; +import type { CollectionMembers } from '../../entities/CollectionMembers.entity'; +import type { Collections } from '../../entities/Collections.entity'; +import type { TrekRepository } from './trek-repository'; + +/** + * Plan 3h Task 1, R6 — the collections cluster's own membership/role model, + * as ONE shared typed predicate instead of three ad hoc re-implementations. + * + * Legacy shape (`collections.service.ts` at HEAD, pre-conversion): + * - `isVisible` (CL2) — `SELECT 1 FROM collections WHERE id=? AND + * owner_id=? UNION SELECT 1 FROM collection_members WHERE + * collection_id=? AND user_id=? AND status='accepted' LIMIT 1`. + * - `isOwner` (CL3) — `SELECT 1 FROM collections WHERE id=? AND owner_id=?`. + * - `roleOf` (CL4) — calls `isOwner` first (`'owner'` short-circuit), else + * `SELECT role FROM collection_members WHERE collection_id=? AND + * user_id=? AND status='accepted'`, normalizing an unrecognized/legacy + * role value to `'editor'` (only `'admin'`/`'viewer'` pass through as-is). + * + * `roleOf`'s own legacy body ALREADY resolved `isVisible`/`isOwner` as its + * own first branch — the three functions were never independent, only + * independently re-queried. {@link resolveCollectionRole} is the SAME + * two-step resolution (owner match, else an accepted member row's role), + * built once here: `isVisible` becomes `role !== null`, `isOwner` becomes + * `role === 'owner'`, `roleOf` returns the role directly. + * `assertAccess`/`assertCanEdit`/`assertCanDelete` stay service-level + * orchestration (CollectionsService) — they call this predicate and branch + * on their own three distinct refusal shapes (404 no-access, 403-viewer + * edit-refused, 403-non-admin delete-refused); this file has no knowledge + * of HTTP status codes. + * + * **QB-callable form** ({@link resolveCollectionRole}): takes the caller's + * own `CollectionsRepository`/`CollectionMembersRepository` (both extend + * `TrekRepository`, typed generically here so this file stays independent + * of either repository class — the same reason `_shared/owned-lookup.ts` + * types its helpers against `TrekRepository` rather than a concrete + * repository) and resolves the role via two ordinary `findOne` calls — + * MikroORM's own QueryBuilder-backed API, the "repository methods that need + * it inline" form R6 asks for. This is what `CollectionsService`'s + * `isVisible`/`isOwner`/`roleOf` call directly; Task 2's own + * `assertCanEdit`/`assertCanDelete` call sites throughout the saved-places/ + * copy-to-trip/labels/invites surface go through the SAME service methods, + * never re-deriving the predicate. + * + * **Kysely-callable form** ({@link collectionVisibleExpr}): an inline + * boolean `EXISTS`/`OR` predicate for a caller already building a raw, + * multi-join statement through `this.kysely()` (the shape Task 2's dynamic + * OR-condition methods — `matchingCollectionPlaces`/`findMembership`, + * CL58/CL69 — are the most likely future consumers of), modelled on + * `packing-visibility.ts`'s `packingVisibleToActorExpr`: an owner-id equality + * plus a correlated `EXISTS` against `collection_members`, mirroring CL2's + * own UNION shape as a boolean OR instead of a value-returning UNION SELECT. + * This form answers "is this collection visible to this user", not "what is + * their role" — a caller needing the actual role (for an edit/delete gate) + * always uses {@link resolveCollectionRole}, never this boolean form. + * + * Proven against five actor cases (owner, admin member, editor member, + * viewer member, an invited-but-still-pending user, a complete stranger) by + * `tests/unit/db/repositories/_shared/collection-role.parity.test.ts`, + * mutation-proved (flip one branch, confirm the matching case goes red) — + * Task 2 imports that harness rather than re-deriving it. + */ +export type CollectionRole = 'owner' | 'admin' | 'editor' | 'viewer' | null; + +/** + * The two-step role resolution CL2/CL3/CL4 all reduce to: an owner match + * short-circuits to `'owner'`; otherwise an accepted member row's own + * `role` column is read and normalized (`'admin'`/`'viewer'` pass through, + * anything else — including a future/unknown value — becomes `'editor'`, + * the legacy `roleOf`'s own fallback); no row of either kind resolves to + * `null` (no access). + * + * `fields: ['id']`/`fields: ['role']` — a narrow projection, like every + * other "rows out, existence/value only" read in this program + * (`findOwnedByUser`'s own docstring). + */ +export async function resolveCollectionRole( + collections: TrekRepository, + members: TrekRepository, + collectionId: number, + userId: number, +): Promise { + const owner = await collections.findOne({ id: collectionId, owner: userId }, { fields: ['id'] }); + if (owner) return 'owner'; + const member = await members.findOne( + { collection: collectionId, user: userId, status: 'accepted' }, + { fields: ['role'] }, + ); + if (!member) return null; + return member.role === 'admin' || member.role === 'viewer' ? (member.role as 'admin' | 'viewer') : 'editor'; +} + +/** + * `collections`/`collection_members`'s REAL table names, the shape + * {@link collectionVisibleExpr} needs. A consumer's own local Kysely `DB` + * interface satisfies this by structural extension, the + * `ReservationVisibilityKyselyDB`/`PackingVisibilityKyselyDB` precedent. + */ +export interface CollectionVisibilityKyselyDB { + collections: { id: number; owner_id: number }; + collection_members: { collection_id: number; user_id: number; status: string }; +} + +/** + * The Kysely-expression twin of CL2's UNION, as a boolean OR: owner-id + * equality, or a correlated `EXISTS` against an accepted `collection_members` + * row. `collections` is a genuine `p.manyToOne(Users)` FK + * (`Collections.entity.ts`), but this form takes the raw `owner_id` column + * directly rather than joining through the relation — matching CL2's own + * `owner_id = ?` text, and avoiding a second join the caller's own outer + * query may not want. + */ +export function collectionVisibleExpr( + eb: ExpressionBuilder, + userId: number, +): ExpressionWrapper { + return eb.or([ + eb('collections.owner_id', '=', userId), + eb.exists( + eb + .selectFrom('collection_members as cm') + .select('cm.collection_id') + .whereRef('cm.collection_id', '=', 'collections.id') + .where('cm.user_id', '=', userId) + .where('cm.status', '=', 'accepted'), + ), + ]); +} diff --git a/server/src/nest/collections/collections.mcp.ts b/server/src/nest/collections/collections.mcp.ts index af89dec5d..17f6045e7 100644 --- a/server/src/nest/collections/collections.mcp.ts +++ b/server/src/nest/collections/collections.mcp.ts @@ -5,9 +5,11 @@ import { demoDenied, ok, } from '../../nest-mcp'; import { z } from 'zod'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { AuthService } from '../auth/auth.service'; import { ADDON_IDS } from '../../addons'; -import { DatabaseService } from '../database/database.service'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; import { CollectionsService } from './collections.service'; import { collectionCreateRequestSchema, collectionUpdateRequestSchema, @@ -57,7 +59,7 @@ const collectionsAddonOn = addonGate(ADDON_IDS.COLLECTIONS); export class CollectionsMcp { constructor( private readonly collections: CollectionsService, - private readonly db: DatabaseService, + @InjectRepository(Users) private readonly users: UsersRepository, private readonly auth: AuthService, readonly addons: AddonsService, ) {} @@ -379,7 +381,7 @@ export class CollectionsMcp { // try/catch added with the post-fold quirk pass — the legacy handler was one // of two where an unexpected throw escaped to the SDK instead of isError. try { - const me = this.db.get<{ username: string; email: string }>('SELECT username, email FROM users WHERE id = ?', ctx.userId); + const me = await this.users.findUsernameEmail(ctx.userId); const res = await this.collections.sendInvite(collection_id, ctx.userId, me?.username ?? '', me?.email ?? '', user_id, role); if (res.error) return { content: [{ type: 'text' as const, text: res.error }], isError: true }; return ok({ success: true }); diff --git a/server/src/nest/collections/collections.module.ts b/server/src/nest/collections/collections.module.ts index 4f0468c55..4df126345 100644 --- a/server/src/nest/collections/collections.module.ts +++ b/server/src/nest/collections/collections.module.ts @@ -1,4 +1,10 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; +import { Collections } from '../../db/entities/Collections.entity'; +import { CollectionMembers } from '../../db/entities/CollectionMembers.entity'; +import { CollectionLabels } from '../../db/entities/CollectionLabels.entity'; +import { Categories } from '../../db/entities/Categories.entity'; +import { Users } from '../../db/entities/Users.entity'; import { CollectionsController } from './collections.controller'; import { CollectionsService } from './collections.service'; import { CollectionsRpc } from './collections.rpc'; @@ -35,7 +41,8 @@ import { MAX_COVER_SIZE } from './collections.controller'; }), }), StorageModule, - NotificationsModule, AddonsModule, PermissionsModule, AuthModule, AppConfigModule, PluginGuardsModule], + NotificationsModule, AddonsModule, PermissionsModule, AuthModule, AppConfigModule, PluginGuardsModule, + MikroOrmModule.forFeature([Collections, CollectionMembers, CollectionLabels, Categories, Users])], controllers: [CollectionsController], providers: [CollectionsService, CollectionsMcp, CollectionsRpc], exports: [CollectionsService], diff --git a/server/src/nest/collections/collections.service.ts b/server/src/nest/collections/collections.service.ts index 3fb22488f..1f9549f8d 100644 --- a/server/src/nest/collections/collections.service.ts +++ b/server/src/nest/collections/collections.service.ts @@ -1,10 +1,20 @@ import path from 'path'; import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { DatabaseService } from '../database/database.service'; import { PermissionsService } from '../permissions/permissions.service'; import { RealtimeService } from '../realtime/realtime.service'; import { reclaimPlaceImage } from '../places/place-image'; import { StorageService } from '../storage/storage.service'; +import { Collections } from '../../db/entities/Collections.entity'; +import { CollectionsRepository, type CollectionPlaceRow } from '../../db/repositories/Collections.repository'; +import { CollectionMembers } from '../../db/entities/CollectionMembers.entity'; +import { CollectionMembersRepository } from '../../db/repositories/CollectionMembers.repository'; +import { CollectionLabels } from '../../db/entities/CollectionLabels.entity'; +import { CollectionLabelsRepository } from '../../db/repositories/CollectionLabels.repository'; +import { Categories } from '../../db/entities/Categories.entity'; +import type { CategoriesRepository } from '../../db/repositories/Categories.repository'; +import { resolveCollectionRole, type CollectionRole } from '../../db/repositories/_shared/collection-role'; import { COORD_DEDUP_TOLERANCE, externalIdsOf, @@ -78,13 +88,12 @@ function httpError(status: number, message: string): never { throw err; } -export type EffectiveRole = 'owner' | 'admin' | 'editor' | 'viewer' | null; +/** Re-exported under this service's historical name — the underlying type + * now lives in `_shared/collection-role.ts` (R6), the ONE shared predicate + * `isVisible`/`isOwner`/`roleOf` all resolve through. */ +export type EffectiveRole = CollectionRole; -interface PlaceRow extends CollectionPlace { - category_name?: string | null; - category_color?: string | null; - category_icon?: string | null; -} +type PlaceRow = CollectionPlaceRow; const MAX_LABELS_PER_COLLECTION = 50; @@ -106,6 +115,10 @@ export class CollectionsService { private readonly notifications: NotificationsService, private readonly storage: StorageService, private readonly uow: UnitOfWork, + @InjectRepository(Collections) private readonly collectionsRepo: CollectionsRepository, + @InjectRepository(CollectionMembers) private readonly members: CollectionMembersRepository, + @InjectRepository(CollectionLabels) private readonly labels: CollectionLabelsRepository, + @InjectRepository(Categories) private readonly categories: CategoriesRepository, ) {} /** @@ -129,22 +142,12 @@ export class CollectionsService { // ------------------------------------------------------------------------- async accessibleCollectionIds(userId: number): Promise { - const rows = this.db.all<{ id: number }>(` - SELECT id FROM collections WHERE owner_id = ? - UNION - SELECT collection_id FROM collection_members WHERE user_id = ? AND status = 'accepted' - `, userId, userId); - return rows.map(r => r.id); + return this.collectionsRepo.accessibleCollectionIds(userId); } + /** R6 — CL2/CL3/CL4 collapse onto the ONE shared predicate in `_shared/collection-role.ts`. */ private async isVisible(userId: number, collectionId: number): Promise { - const row = this.db.get(` - SELECT 1 FROM collections WHERE id = ? AND owner_id = ? - UNION - SELECT 1 FROM collection_members WHERE collection_id = ? AND user_id = ? AND status = 'accepted' - LIMIT 1 - `, collectionId, userId, collectionId, userId); - return !!row; + return (await resolveCollectionRole(this.collectionsRepo, this.members, collectionId, userId)) !== null; } async assertAccess(userId: number, collectionId: number): Promise { @@ -152,20 +155,13 @@ export class CollectionsService { } async isOwner(userId: number, collectionId: number): Promise { - const row = this.db.get('SELECT 1 FROM collections WHERE id = ? AND owner_id = ?', collectionId, userId); - return !!row; + return (await resolveCollectionRole(this.collectionsRepo, this.members, collectionId, userId)) === 'owner'; } /** The viewer's effective permission on a list: owner (full), or their accepted * member role, or null when they have no access. */ async roleOf(userId: number, collectionId: number): Promise { - if (await this.isOwner(userId, collectionId)) return 'owner'; - const row = this.db.get<{ role: string }>( - "SELECT role FROM collection_members WHERE collection_id = ? AND user_id = ? AND status = 'accepted'", - collectionId, userId, - ); - if (!row) return null; - return row.role === 'admin' || row.role === 'viewer' ? row.role : 'editor'; + return resolveCollectionRole(this.collectionsRepo, this.members, collectionId, userId); } /** Add/edit a place — owner, admin or editor. 404 hides lists you can't see, @@ -184,9 +180,9 @@ export class CollectionsService { } private async ownerOf(collectionId: number): Promise { - const row = this.db.get<{ owner_id: number }>('SELECT owner_id FROM collections WHERE id = ?', collectionId); - if (!row) httpError(404, 'Collection not found'); - return row.owner_id; + const ownerId = await this.collectionsRepo.ownerId(collectionId); + if (ownerId === undefined) httpError(404, 'Collection not found'); + return ownerId; } // ------------------------------------------------------------------------- @@ -195,14 +191,7 @@ export class CollectionsService { private async loadTagsByCollectionPlaceIds(placeIds: number[]): Promise> { const out: Record = {}; - if (placeIds.length === 0) return out; - const placeholders = placeIds.map(() => '?').join(','); - const rows = this.db.all<{ pid: number; id: number; name: string; color: string }>(` - SELECT cpt.collection_place_id AS pid, t.id, t.name, t.color - FROM collection_place_tags cpt - JOIN tags t ON t.id = cpt.tag_id - WHERE cpt.collection_place_id IN (${placeholders}) - `, ...placeIds); + const rows = await this.collectionsRepo.loadTagsByPlaceIds(placeIds); for (const r of rows) { if (!out[r.pid]) out[r.pid] = []; out[r.pid].push({ id: r.id, name: r.name, color: r.color }); @@ -212,21 +201,13 @@ export class CollectionsService { /** A list's own label definitions, in display order. */ private async loadLabelsByCollection(collectionId: number): Promise { - return this.db.all( - 'SELECT id, collection_id, name, color, sort_order FROM collection_labels WHERE collection_id = ? ORDER BY sort_order, id', - collectionId, - ); + return this.labels.listByCollection(collectionId); } /** Assigned label ids per place, batched (mirrors loadTagsByCollectionPlaceIds). */ private async loadLabelIdsByPlaceIds(placeIds: number[]): Promise> { const out: Record = {}; - if (placeIds.length === 0) return out; - const placeholders = placeIds.map(() => '?').join(','); - const rows = this.db.all<{ pid: number; label_id: number }>( - `SELECT collection_place_id AS pid, label_id FROM collection_place_labels WHERE collection_place_id IN (${placeholders})`, - ...placeIds, - ); + const rows = await this.collectionsRepo.loadLabelIdsByPlaceIds(placeIds); for (const r of rows) { if (!out[r.pid]) out[r.pid] = []; out[r.pid].push(r.label_id); @@ -237,14 +218,7 @@ export class CollectionsService { /** Per-voter rating rows (#1435), batched (mirrors loadTagsByCollectionPlaceIds). */ private async loadRatingsByCollectionPlaceIds(placeIds: number[]): Promise> { const out: Record = {}; - if (placeIds.length === 0) return out; - const rows = this.db.all<{ pid: number; user_id: number; username: string; avatar: string | null; rating: number }>(` - SELECT cpr.collection_place_id AS pid, cpr.user_id, u.username, u.avatar, cpr.rating - FROM collection_place_ratings cpr - JOIN users u ON u.id = cpr.user_id - WHERE cpr.collection_place_id IN (${placeIds.map(() => '?').join(',')}) - ORDER BY cpr.created_at - `, ...placeIds); + const rows = await this.collectionsRepo.loadRatingsByPlaceIds(placeIds); for (const { pid, ...rest } of rows) { if (!out[pid]) out[pid] = []; out[pid].push(rest); @@ -276,45 +250,31 @@ export class CollectionsService { } private async getPlaceById(placeId: number): Promise { - const row = this.db.get(` - SELECT cp.*, c.name AS category_name, c.color AS category_color, c.icon AS category_icon - FROM collection_places cp - LEFT JOIN categories c ON cp.category_id = c.id - WHERE cp.id = ? - `, placeId); + const row = await this.collectionsRepo.findPlaceRowById(placeId); if (!row) httpError(404, 'Place not found'); return (await this.hydratePlaces([row]))[0]; } private async collectionIdOfPlace(placeId: number): Promise { - const row = this.db.get<{ collection_id: number }>('SELECT collection_id FROM collection_places WHERE id = ?', placeId); - if (!row) httpError(404, 'Place not found'); - return row.collection_id; + const collectionId = await this.collectionsRepo.collectionIdOfPlace(placeId); + if (collectionId === undefined) httpError(404, 'Place not found'); + return collectionId; } private async buildMembers(collectionId: number): Promise { - const owner = this.db.get>(` - SELECT u.id AS user_id, u.username, u.email, u.avatar - FROM collections col JOIN users u ON u.id = col.owner_id - WHERE col.id = ? - `, collectionId); - const members = this.db.all>(` - SELECT u.id AS user_id, u.username, u.email, u.avatar, cm.status, cm.role - FROM collection_members cm JOIN users u ON u.id = cm.user_id - WHERE cm.collection_id = ? - ORDER BY u.username - `, collectionId); + const owner = await this.members.ownerRow(collectionId); + const memberRows = await this.members.memberRows(collectionId); const result: CollectionMember[] = []; if (owner) result.push({ ...owner, status: 'accepted', role: 'admin', is_owner: true }); - for (const m of members) result.push({ ...m, is_owner: false }); + for (const m of memberRows) result.push({ ...m, is_owner: false } as CollectionMember); return result; } private async getCollectionRow(id: number): Promise { - const col = this.db.get('SELECT * FROM collections WHERE id = ?', id); + const col = await this.collectionsRepo.findRow(id); if (!col) httpError(404, 'Collection not found'); - const placeCount = this.db.get<{ n: number }>('SELECT COUNT(*) AS n FROM collection_places WHERE collection_id = ?', id)!.n; - return { ...col, links: parseLinks(col.links), place_count: placeCount, members: await this.buildMembers(id) }; + const placeCount = await this.collectionsRepo.placeCount(id); + return { ...col, links: parseLinks(col.links), place_count: placeCount, members: await this.buildMembers(id) } as Collection; } // ------------------------------------------------------------------------- @@ -330,13 +290,7 @@ export class CollectionsService { }))) .sort((a, b) => (a.sort_order ?? 0) - (b.sort_order ?? 0) || a.id - b.id); - const incomingInvites = this.db.all<{ collection_id: number; name: string; from_id: number; from_username: string }>(` - SELECT cm.collection_id, c.name, u.id AS from_id, u.username AS from_username - FROM collection_members cm - JOIN collections c ON c.id = cm.collection_id - JOIN users u ON u.id = c.owner_id - WHERE cm.user_id = ? AND cm.status = 'pending' - `, userId) + const incomingInvites = (await this.members.pendingInvitesForUser(userId)) .map(r => ({ collection_id: r.collection_id, name: r.name, from: { id: r.from_id, username: r.from_username } })); return { collections, incomingInvites }; @@ -345,13 +299,7 @@ export class CollectionsService { async getCollection(userId: number, id: number): Promise { await this.assertAccess(userId, id); const collection = await this.getCollectionRow(id); - const rows = this.db.all(` - SELECT cp.*, c.name AS category_name, c.color AS category_color, c.icon AS category_icon - FROM collection_places cp - LEFT JOIN categories c ON cp.category_id = c.id - WHERE cp.collection_id = ? - ORDER BY cp.sort_order, cp.created_at - `, id); + const rows = await this.collectionsRepo.listPlaceRows(id); return { collection: { ...collection, is_owner: collection.owner_id === userId, labels: await this.loadLabelsByCollection(id) }, places: await this.hydratePlaces(rows), @@ -381,13 +329,7 @@ export class CollectionsService { const labels = await this.loadLabelsByCollection(id); const labelNameById = new Map(labels.map(l => [l.id, l.name])); - const rows = this.db.all(` - SELECT cp.*, c.name AS category_name, c.color AS category_color, c.icon AS category_icon - FROM collection_places cp - LEFT JOIN categories c ON cp.category_id = c.id - WHERE cp.collection_id = ? - ORDER BY cp.sort_order, cp.created_at - `, id); + const rows = await this.collectionsRepo.listPlaceRows(id); const labelIdsByPlace = await this.loadLabelIdsByPlaceIds(rows.map(r => r.id)); const places: CollectionFilePlace[] = rows.map(row => ({ @@ -407,7 +349,7 @@ export class CollectionsService { google_place_id: row.google_place_id ?? null, google_ftid: row.google_ftid ?? null, osm_id: row.osm_id ?? null, - status: row.status, + status: row.status as CollectionStatus, links: parseLinks((row as { links?: unknown }).links), category: row.category_name ?? null, labels: (labelIdsByPlace[row.id] || []).map(lid => labelNameById.get(lid)).filter((n): n is string => !!n), @@ -530,12 +472,10 @@ export class CollectionsService { collectionId: number, labels: CollectionFileLabel[] | undefined, ): Promise<{ byName: Map; created: number }> { const byName = new Map(); - for (const row of this.db.all<{ id: number; name: string }>('SELECT id, name FROM collection_labels WHERE collection_id = ?', collectionId)) { + for (const row of await this.labels.idNameByCollection(collectionId)) { byName.set(row.name.trim().toLowerCase(), row.id); } - let sortOrder = this.db.get<{ m: number }>( - 'SELECT COALESCE(MAX(sort_order), -1) AS m FROM collection_labels WHERE collection_id = ?', collectionId, - )!.m + 1; + let sortOrder = (await this.labels.maxSortOrder(collectionId)) + 1; let created = 0; for (const label of (labels ?? []).slice(0, MAX_COLLECTION_FILE_LABELS)) { const key = label.name.trim().toLowerCase(); @@ -570,22 +510,11 @@ export class CollectionsService { // The palette is instance-wide and read-only here: a file names a category, // it does not get to create one. const categoryIdByName = new Map(); - for (const c of this.db.all<{ id: number; name: string }>('SELECT id, name FROM categories')) { + for (const c of await this.categories.listIdName()) { categoryIdByName.set(c.name.trim().toLowerCase(), c.id); } const ownerId = await this.ownerOf(collectionId); - const firstOrder = this.db.get<{ m: number }>( - 'SELECT COALESCE(MAX(sort_order), -1) AS m FROM collection_places WHERE collection_id = ?', collectionId, - )!.m + 1; - - const insertPlace = this.db.prepare(` - INSERT INTO collection_places ( - collection_id, owner_id, saved_by, name, description, lat, lng, address, - category_id, price, currency, notes, image_url, google_place_id, google_ftid, - osm_id, website, phone, status, links, sort_order - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) - `); - const assignLabel = this.db.prepare('INSERT OR IGNORE INTO collection_place_labels (collection_place_id, label_id) VALUES (?, ?)'); + const firstOrder = (await this.collectionsRepo.maxPlaceSortOrder(collectionId)) + 1; let imported = 0; let skipped = 0; @@ -607,19 +536,18 @@ export class CollectionsService { duplicates += 1; continue; } - const res = insertPlace.run( - collectionId, ownerId, savedBy, - place.name, place.description ?? null, place.lat ?? null, place.lng ?? null, place.address ?? null, - place.category ? (categoryIdByName.get(place.category.trim().toLowerCase()) ?? null) : null, - place.price ?? null, place.currency ?? null, place.notes ?? null, - place.image_url ?? null, place.google_place_id ?? null, place.google_ftid ?? null, - place.osm_id ?? null, place.website ?? null, place.phone ?? null, - place.status ?? 'idea', serializeLinks(place.links), firstOrder + imported, - ); - const placeId = Number(res.lastInsertRowid); + const placeId = await this.collectionsRepo.insertFilePlace({ + collection_id: collectionId, owner_id: ownerId, saved_by: savedBy, + name: place.name, description: place.description ?? null, lat: place.lat ?? null, lng: place.lng ?? null, address: place.address ?? null, + category_id: place.category ? (categoryIdByName.get(place.category.trim().toLowerCase()) ?? null) : null, + price: place.price ?? null, currency: place.currency ?? null, notes: place.notes ?? null, + image_url: place.image_url ?? null, google_place_id: place.google_place_id ?? null, google_ftid: place.google_ftid ?? null, + osm_id: place.osm_id ?? null, website: place.website ?? null, phone: place.phone ?? null, + status: place.status ?? 'idea', links: serializeLinks(place.links), sort_order: firstOrder + imported, + }); for (const labelName of place.labels ?? []) { const labelId = labelIdByName.get(labelName.trim().toLowerCase()); - if (labelId) assignLabel.run(placeId, labelId); + if (labelId) await this.collectionsRepo.assignPlaceLabel(placeId, labelId); } imported += 1; } @@ -633,48 +561,38 @@ export class CollectionsService { * import is sent by the caller rather than one per label. */ private async insertImportedLabel(collectionId: number, label: CollectionFileLabel, sortOrder: number): Promise { - const res = this.db.run( - 'INSERT INTO collection_labels (collection_id, name, color, sort_order) VALUES (?, ?, ?, ?)', - collectionId, label.name.trim(), label.color ?? '#6366f1', sortOrder, - ); - return Number(res.lastInsertRowid); + return this.labels.insertLabel({ + collection_id: collectionId, name: label.name.trim(), color: label.color ?? '#6366f1', sort_order: sortOrder, + }); } async createCollection(userId: number, body: CollectionCreateRequest): Promise { - const max = this.db.get<{ m: number }>('SELECT COALESCE(MAX(sort_order), -1) AS m FROM collections WHERE owner_id = ?', userId)!.m; - const result = this.db.run(` - INSERT INTO collections (owner_id, name, description, color, icon, cover_image, links, sort_order) - VALUES (?, ?, ?, ?, ?, ?, ?, ?) - `, - userId, - body.name, - body.description ?? null, - body.color ?? '#6366f1', - body.icon ?? 'Bookmark', - body.cover_image ?? null, - serializeLinks(body.links), - max + 1, - ); - const col = await this.getCollectionRow(Number(result.lastInsertRowid)); + const max = await this.collectionsRepo.maxSortOrder(userId); + const id = await this.collectionsRepo.insertCollection({ + owner_id: userId, + name: body.name, + description: body.description ?? null, + color: body.color ?? '#6366f1', + icon: body.icon ?? 'Bookmark', + cover_image: body.cover_image ?? null, + links: serializeLinks(body.links), + sort_order: max + 1, + }); + const col = await this.getCollectionRow(id); return { ...col, is_owner: true }; } async updateCollection(userId: number, id: number, body: CollectionUpdateRequest, socketId?: string): Promise { await this.assertCanEdit(userId, id); - const updates: string[] = []; - const params: (string | number | null)[] = []; - if (body.name !== undefined) { updates.push('name = ?'); params.push(body.name); } - if (body.description !== undefined) { updates.push('description = ?'); params.push(body.description ?? null); } - if (body.color !== undefined) { updates.push('color = ?'); params.push(body.color ?? null); } - if (body.icon !== undefined) { updates.push('icon = ?'); params.push(body.icon ?? null); } - if (body.cover_image !== undefined) { updates.push('cover_image = ?'); params.push(body.cover_image ?? null); } - if (body.links !== undefined) { updates.push('links = ?'); params.push(serializeLinks(body.links)); } - if (body.sort_order !== undefined) { updates.push('sort_order = ?'); params.push(body.sort_order); } - if (updates.length > 0) { - updates.push("updated_at = CURRENT_TIMESTAMP"); - params.push(id); - this.db.run(`UPDATE collections SET ${updates.join(', ')} WHERE id = ?`, ...params); - } + await this.collectionsRepo.updateFields(id, { + ...(body.name !== undefined ? { name: body.name } : {}), + ...(body.description !== undefined ? { description: body.description ?? null } : {}), + ...(body.color !== undefined ? { color: body.color ?? null } : {}), + ...(body.icon !== undefined ? { icon: body.icon ?? null } : {}), + ...(body.cover_image !== undefined ? { cover_image: body.cover_image ?? null } : {}), + ...(body.links !== undefined ? { links: serializeLinks(body.links) } : {}), + ...(body.sort_order !== undefined ? { sort_order: body.sort_order } : {}), + }); await this.notifyCollectionUsers(id, socketId, 'collections:updated'); const col = await this.getCollectionRow(id); return { ...col, is_owner: col.owner_id === userId }; @@ -683,8 +601,8 @@ export class CollectionsService { /** Set (or clear) a list's cover image, reclaiming the previous file. */ async setCollectionCover(userId: number, id: number, coverUrl: string | null, socketId?: string): Promise { await this.assertCanEdit(userId, id); - const prev = this.db.get<{ cover_image: string | null }>('SELECT cover_image FROM collections WHERE id = ?', id)?.cover_image ?? null; - this.db.run('UPDATE collections SET cover_image = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', coverUrl, id); + const prev = (await this.collectionsRepo.coverImage(id)) ?? null; + await this.collectionsRepo.setCoverImage(id, coverUrl); if (prev && prev !== coverUrl) await this.deleteOldCollectionCover(prev); await this.notifyCollectionUsers(id, socketId, 'collections:updated'); const col = await this.getCollectionRow(id); @@ -696,10 +614,10 @@ export class CollectionsService { if (!(await this.isOwner(userId, id))) httpError(403, 'Only the owner can delete this list'); // Snapshot recipients BEFORE the cascade wipes collection_members. - const accepted = this.db.all<{ user_id: number }>("SELECT user_id FROM collection_members WHERE collection_id = ? AND status = 'accepted'", id).map(r => r.user_id); - const pending = this.db.all<{ user_id: number }>("SELECT user_id FROM collection_members WHERE collection_id = ? AND status = 'pending'", id).map(r => r.user_id); + const accepted = await this.members.acceptedUserIds(id); + const pending = await this.members.pendingUserIds(id); - this.db.run('DELETE FROM collections WHERE id = ?', id); // CASCADE drops members + places + tags + await this.collectionsRepo.deleteById(id); // CASCADE drops members + places + tags [...new Set([...accepted, ...pending])] .filter(uid => uid !== userId) @@ -708,11 +626,12 @@ export class CollectionsService { async reorderCollections(userId: number, orderedIds: number[]): Promise { const visible = new Set(await this.accessibleCollectionIds(userId)); - const stmt = this.db.prepare('UPDATE collections SET sort_order = ? WHERE id = ?'); await this.uow.transactional(async () => { - orderedIds.forEach((cid, index) => { - if (visible.has(cid)) stmt.run(index, cid); - }); + await this.collectionsRepo.setSortOrders( + orderedIds + .map((cid, index) => ({ id: cid, sortOrder: index })) + .filter(({ id }) => visible.has(id)), + ); }); } @@ -742,24 +661,11 @@ export class CollectionsService { for (const strategy of placeMatchStrategies(candidate)) { let hit: { id: number; name: string } | undefined; if (strategy.by === 'externalId') { - hit = this.db.get<{ id: number; name: string }>(` - SELECT id, name FROM collection_places - WHERE collection_id = ? AND (google_place_id = ? OR google_ftid = ? OR osm_id = ?) - ORDER BY id ASC LIMIT 1 - `, collectionId, strategy.id, strategy.id, strategy.id); + hit = await this.collectionsRepo.findDuplicateByExternalId(collectionId, strategy.id); } else if (strategy.by === 'name') { - hit = this.db.get<{ id: number; name: string }>(` - SELECT id, name FROM collection_places - WHERE collection_id = ? AND lower(trim(name)) = ? - ORDER BY id ASC LIMIT 1 - `, collectionId, strategy.name); + hit = await this.collectionsRepo.findDuplicateByName(collectionId, strategy.name); } else { - hit = this.db.get<{ id: number; name: string }>(` - SELECT id, name FROM collection_places - WHERE collection_id = ? AND lat IS NOT NULL AND lng IS NOT NULL - AND abs(lat - ?) <= ? AND abs(lng - ?) <= ? - ORDER BY id ASC LIMIT 1 - `, collectionId, strategy.lat, strategy.tolerance, strategy.lng, strategy.tolerance); + hit = await this.collectionsRepo.findDuplicateByCoords(collectionId, strategy.lat, strategy.lng, strategy.tolerance); } if (hit) return hit; } diff --git a/server/tests/helpers/test-uow.ts b/server/tests/helpers/test-uow.ts index 5b552aa02..086f4a0f0 100644 --- a/server/tests/helpers/test-uow.ts +++ b/server/tests/helpers/test-uow.ts @@ -65,6 +65,12 @@ import { RoadtripPreferences } from '../../src/db/entities/RoadtripPreferences.e import type { RoadtripPreferencesRepository } from '../../src/db/repositories/RoadtripPreferences.repository'; import { RoadtripDayBoundaries } from '../../src/db/entities/RoadtripDayBoundaries.entity'; import type { RoadtripDayBoundariesRepository } from '../../src/db/repositories/RoadtripDayBoundaries.repository'; +import { Collections } from '../../src/db/entities/Collections.entity'; +import type { CollectionsRepository } from '../../src/db/repositories/Collections.repository'; +import { CollectionMembers } from '../../src/db/entities/CollectionMembers.entity'; +import type { CollectionMembersRepository } from '../../src/db/repositories/CollectionMembers.repository'; +import { CollectionLabels } from '../../src/db/entities/CollectionLabels.entity'; +import type { CollectionLabelsRepository } from '../../src/db/repositories/CollectionLabels.repository'; const perHandle = new WeakMap>(); const appSettingsPerHandle = new WeakMap>(); @@ -586,3 +592,41 @@ export function createTestRoadtripDayBoundariesRepo(db: Database.Database): Prom roadtripDayBoundariesRepoPerHandle.set(db, pending); return pending; } + +// --------------------------------------------------------------------------- +// Plan 3h Task 1 (collections part A) — `CollectionsRepository`/ +// `CollectionMembersRepository`/`CollectionLabelsRepository` a hand-constructed +// `CollectionsService` needs. Same memoisation-per-handle pattern as every +// helper above. +// --------------------------------------------------------------------------- + +const collectionsRepoPerHandle = new WeakMap>(); +const collectionMembersRepoPerHandle = new WeakMap>(); +const collectionLabelsRepoPerHandle = new WeakMap>(); + +/** The `CollectionsRepository` a hand-constructed `CollectionsService` needs. */ +export function createTestCollectionsRepo(db: Database.Database): Promise { + const existing = collectionsRepoPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(Collections)); + collectionsRepoPerHandle.set(db, pending); + return pending; +} + +/** The `CollectionMembersRepository` a hand-constructed `CollectionsService` needs. */ +export function createTestCollectionMembersRepo(db: Database.Database): Promise { + const existing = collectionMembersRepoPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(CollectionMembers)); + collectionMembersRepoPerHandle.set(db, pending); + return pending; +} + +/** The `CollectionLabelsRepository` a hand-constructed `CollectionsService` needs. */ +export function createTestCollectionLabelsRepo(db: Database.Database): Promise { + const existing = collectionLabelsRepoPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(CollectionLabels)); + collectionLabelsRepoPerHandle.set(db, pending); + return pending; +} diff --git a/server/tests/unit/db/repositories/_shared/collection-role.parity.test.ts b/server/tests/unit/db/repositories/_shared/collection-role.parity.test.ts new file mode 100644 index 000000000..29e1d7cd0 --- /dev/null +++ b/server/tests/unit/db/repositories/_shared/collection-role.parity.test.ts @@ -0,0 +1,131 @@ +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 { createUser } from '../../../../helpers/factories'; +import { Collections } from '../../../../../src/db/entities/Collections.entity'; +import { CollectionMembers } from '../../../../../src/db/entities/CollectionMembers.entity'; +import { resolveCollectionRole, type CollectionRole } from '../../../../../src/db/repositories/_shared/collection-role'; + +/** + * Plan 3h Task 1, R6 — the collections cluster's shared collection-role + * predicate's own parity harness, built BEFORE any consumer (Task 2's + * `assertCanEdit`/`assertCanDelete` call sites throughout saved-places/ + * copy-to-trip/labels/invites) converts. `CollectionsService`'s own + * `isVisible`/`isOwner`/`roleOf`/`assertAccess`/`assertCanEdit`/ + * `assertCanDelete` all resolve through {@link resolveCollectionRole} — + * this file proves the predicate itself; `collections.service.test.ts`'s + * own describe blocks exercise it end-to-end through the real service. + * + * Six actor cases (the R6 ruling text names five roles plus a stranger, + * six distinct actors in total): owner, an accepted admin member, an + * accepted editor member, an accepted viewer member, an invited-but-still- + * `pending` user, and a complete stranger (no row at all). Each case is + * checked against all three refusal shapes R6 calls out — 404 (no access), + * 403-viewer (edit refused), 403-non-admin (delete refused) — via a local + * re-implementation of `CollectionsService`'s own branch logic + * (`assertCanEdit`/`assertCanDelete`'s exact `if` chain, copied here as the + * test's own oracle, independent of the production service — the same + * "hold this side fixed" reasoning `reservation-visibility.test.ts`'s + * docstring gives, adapted from a SQL string to a decision table since this + * predicate is value-returning, not boolean). + * + * **Mutation-proved**: flipping {@link resolveCollectionRole}'s `r === + * 'viewer'`-adjacent branch (temporarily changing the member-row + * normalization so `'viewer'` fell through to `'editor'` instead of staying + * `'viewer'`) made the VIEWER case's `assertCanEdit-refused` assertion go + * red (a viewer would then wrongly pass the edit gate) while every other + * case stayed green — confirmed by hand during this task, then reverted; + * not left in the tree. + */ + +const testDb = createSnapshotTestDb(); +let t: TestOrm; + +beforeAll(async () => { t = await createTestOrm(testDb); }); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +type Actor = 'owner' | 'admin' | 'editor' | 'viewer' | 'pending' | 'stranger'; + +function seedFixture() { + const { user: owner } = createUser(testDb); + const { user: admin } = createUser(testDb); + const { user: editor } = createUser(testDb); + const { user: viewer } = createUser(testDb); + const { user: pending } = createUser(testDb); + const { user: stranger } = createUser(testDb); + + const collectionId = testDb + .prepare("INSERT INTO collections (owner_id, name) VALUES (?, 'Fixture')") + .run(owner.id).lastInsertRowid as number; + + const addMember = (userId: number, role: string, status: string) => + testDb.prepare('INSERT INTO collection_members (collection_id, user_id, status, role) VALUES (?, ?, ?, ?)').run(collectionId, userId, status, role); + addMember(admin.id, 'admin', 'accepted'); + addMember(editor.id, 'editor', 'accepted'); + addMember(viewer.id, 'viewer', 'accepted'); + addMember(pending.id, 'editor', 'pending'); + + return { + collectionId: collectionId as number, + userIds: { owner: owner.id, admin: admin.id, editor: editor.id, viewer: viewer.id, pending: pending.id, stranger: stranger.id } as Record, + }; +} + +/** `assertAccess`'s own branch: 404 iff the predicate returns null. */ +function assertAccessLike(role: CollectionRole): 'ok' | 404 { + return role === null ? 404 : 'ok'; +} + +/** `assertCanEdit`'s own branch: 404 no-access, 403 viewer-refused, else ok. */ +function assertCanEditLike(role: CollectionRole): 'ok' | 404 | 403 { + if (role === null) return 404; + if (role === 'viewer') return 403; + return 'ok'; +} + +/** `assertCanDelete`'s own branch: 404 no-access, 403 non-admin-refused, else ok. */ +function assertCanDeleteLike(role: CollectionRole): 'ok' | 404 | 403 { + if (role === null) return 404; + if (role !== 'owner' && role !== 'admin') return 403; + return 'ok'; +} + +const EXPECTED: Record = { + owner: { role: 'owner', access: 'ok', edit: 'ok', del: 'ok' }, + admin: { role: 'admin', access: 'ok', edit: 'ok', del: 'ok' }, + editor: { role: 'editor', access: 'ok', edit: 'ok', del: 403 }, + viewer: { role: 'viewer', access: 'ok', edit: 403, del: 403 }, + pending: { role: null, access: 404, edit: 404, del: 404 }, + stranger: { role: null, access: 404, edit: 404, del: 404 }, +}; + +describe('_shared/collection-role — R6 parity harness (owner/admin/editor/viewer/pending/stranger)', () => { + (Object.keys(EXPECTED) as Actor[]).forEach((actor) => { + it(`resolves the ${actor} case to role=${String(EXPECTED[actor].role)} and the matching refusal shapes`, async () => { + const { collectionId, userIds } = seedFixture(); + const collections = t.repo(Collections); + const members = t.repo(CollectionMembers); + + const role = await resolveCollectionRole(collections, members, collectionId, userIds[actor]); + const expected = EXPECTED[actor]; + + expect(role).toBe(expected.role); + expect(assertAccessLike(role)).toBe(expected.access); + expect(assertCanEditLike(role)).toBe(expected.edit); + expect(assertCanDeleteLike(role)).toBe(expected.del); + }); + }); + + it('a stranger and a pending invite are indistinguishable to the predicate (both resolve to null, no access)', async () => { + const { collectionId, userIds } = seedFixture(); + const collections = t.repo(Collections); + const members = t.repo(CollectionMembers); + + const pendingRole = await resolveCollectionRole(collections, members, collectionId, userIds.pending); + const strangerRole = await resolveCollectionRole(collections, members, collectionId, userIds.stranger); + expect(pendingRole).toBeNull(); + expect(strangerRole).toBeNull(); + }); +}); diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index ce71dc5dc..bea32254c 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/server/tests/unit/nest/collections.service.test.ts @@ -55,7 +55,10 @@ import { CollectionsService } from '../../../src/nest/collections/collections.se import { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; import { makeStorageFixture } from '../../helpers/storage-fixture'; import { notificationsStub } from '../../helpers/notifications'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestDatabaseService, createTestGooglePlacePhotoMetaRepo, createTestPlacesRepo } from '../../helpers/test-uow'; +import { + createTestUnitOfWork, createTestAppSettingsRepo, createTestDatabaseService, createTestGooglePlacePhotoMetaRepo, createTestPlacesRepo, + createTestCollectionsRepo, createTestCollectionMembersRepo, createTestCollectionLabelsRepo, createTestCategoriesRepo, +} from '../../helpers/test-uow'; const storageFx = makeStorageFixture(''); let svc: CollectionsService; @@ -65,8 +68,24 @@ let svc: CollectionsService; // constructor needs two repositories, resolved through `createTestOrm`. let photoCache: PlacePhotoCacheService; const removeIfUnreferenced = (id: string) => photoCache.removeIfUnreferenced(id); +// Plan 3h Task 1 (part A, R6's construction pattern): the SAME direct-construction +// shape every converted service in this program uses for its own hand-built unit +// test (`atlas.service.test.ts`/`journey-domain.service.test.ts` precedent) — real +// repositories resolved off `sharedTestOrm(testDb)` (via `createTestXRepo` helpers, +// `allowGlobalContext: true` by default, `test-orm.ts`'s own docstring), no +// `withRequestContext` wrapper needed or added anywhere in this file. `db` (the raw +// `DatabaseService`) stays a constructor param and is still exercised directly: +// Part B's methods (saved places CRUD, copy-to-trip, labels-assignment, invites — +// Task 2's own, unconverted by this task) still issue raw `this.db.prepare(...)` +// calls the describe blocks below covering them exercise unchanged. beforeAll(async () => { - svc = new CollectionsService(await createTestDatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService(), notificationsStub(notifSend), storageFx.storage, await createTestUnitOfWork(testDb)); + svc = new CollectionsService( + await createTestDatabaseService(testDb), + new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), + new RealtimeService(), notificationsStub(notifSend), storageFx.storage, await createTestUnitOfWork(testDb), + await createTestCollectionsRepo(testDb), await createTestCollectionMembersRepo(testDb), + await createTestCollectionLabelsRepo(testDb), await createTestCategoriesRepo(testDb), + ); photoCache = new PlacePhotoCacheService( new DatabaseService(testDb), makeStorageFixture('photos/google/').storage, @@ -1280,8 +1299,12 @@ describe('exportCollection / importCollection (#2198)', () => { const owner = createUser(testDb).user; const before = (await svc.listCollections(owner.id)).collections.length; const insert = testDb.prepare.bind(testDb); + // Plan 3h Task 1: `writeFilePlaces`'s INSERT now goes through Kysely + // (`CollectionsRepository.insertFilePlace`), which compiles to lowercase, + // double-quoted SQL (`insert into "collection_places" (...)`) — matched + // case-insensitively here instead of the legacy literal uppercase text. const spy = vi.spyOn(testDb, 'prepare').mockImplementation((sql: string) => { - if (sql.includes('INSERT INTO collection_places')) throw new Error('disk is full'); + if (/insert\s+into\s+"?collection_places"?/i.test(sql)) throw new Error('disk is full'); return insert(sql); }); @@ -1429,8 +1452,10 @@ describe('importIntoCollection', () => { const col = await svc.createCollection(owner.id, { name: 'Lisbon' }); await svc.savePlace(owner.id, { collection_id: col.id, name: 'Time Out Market' }); const real = testDb.prepare.bind(testDb); + // Plan 3h Task 1: see COLLECTIONS-SVC-111's own comment — the Kysely-compiled + // INSERT is lowercase/double-quoted now, matched case-insensitively. const spy = vi.spyOn(testDb, 'prepare').mockImplementation((sql: string) => { - if (sql.includes('INSERT INTO collection_places')) throw new Error('disk is full'); + if (/insert\s+into\s+"?collection_places"?/i.test(sql)) throw new Error('disk is full'); return real(sql); }); @@ -1525,3 +1550,107 @@ describe('exportCollectionGpx / readCollectionGpx (#2301)', () => { expect((await svc.listCollections(owner.id)).collections).toHaveLength(0); }); }); + +// ── Plan 3h Task 1 — repository-conversion parity (CL1-CL36) ──────────────── +// +// The describe blocks above already exercise every converted method through +// the service's own public behaviour (createCollection/listCollections/ +// getCollection/updateCollection/reorderCollections/exportCollection/ +// importCollection/deleteCollection), asserting against raw `testDb.prepare` +// reads the same way a hand-rolled "legacy" comparison would. This block adds +// the two things the brief calls out explicitly: a full-key `toEqual` parity +// test on a fully-seeded `getCollectionRow`/`getCollection` read (owner + +// admin + editor + viewer + a pending invite; places with labels and ratings +// from more than one voter), and a dedicated ordering proof for +// `deleteCollection`'s CL33/34 snapshot-before-CL35-cascade shape (R6's own +// risk note: a reversed order still passes an ordinary status-code test). +describe('Plan 3h Task 1 — repository conversion parity', () => { + it('COLLECTIONS-SVC-200: getCollection is full-key identical to the legacy read model on a fully seeded list', async () => { + const owner = createUser(testDb).user; + const admin = createUser(testDb).user; + const editor = createUser(testDb).user; + const viewer = createUser(testDb).user; + const pending = createUser(testDb).user; + const col = await svc.createCollection(owner.id, { name: 'Full house', description: 'Every role', color: '#111111' }); + addMember(col.id, admin.id, 'admin'); + addMember(col.id, editor.id, 'editor'); + addMember(col.id, viewer.id, 'viewer'); + await svc.sendInvite(col.id, owner.id, owner.username, owner.email, pending.id); + + const tag = createTag(testDb, owner.id, { name: 'Foodie' }); + const place = (await svc.savePlace(owner.id, { + collection_id: col.id, name: 'Cafe', lat: 1, lng: 2, tag_ids: [tag.id], + })).place!; + await svc.setRating(owner.id, place.id, 5); + await svc.setRating(admin.id, place.id, 3); + + const result = await svc.getCollection(owner.id, col.id); + + // The legacy read model, recomputed independently from raw SQL against + // the same seeded rows — the oracle this test proves the repository + // conversion against, kept structurally separate from the production + // code path it verifies. + const colRow = testDb.prepare('SELECT * FROM collections WHERE id = ?').get(col.id) as Record; + const placeCount = (testDb.prepare('SELECT COUNT(*) AS n FROM collection_places WHERE collection_id = ?').get(col.id) as { n: number }).n; + const legacyMembers = [ + { user_id: owner.id, username: owner.username, email: owner.email, avatar: null, status: 'accepted', role: 'admin', is_owner: true }, + // CL13's own `SELECT ... FROM collection_members cm JOIN users u ...` + // has NO status filter — both the accepted admin/editor/viewer AND the + // still-`pending` invite come back, ordered by `u.username`. + ...[ + { u: admin, role: 'admin', status: 'accepted' }, + { u: editor, role: 'editor', status: 'accepted' }, + { u: viewer, role: 'viewer', status: 'accepted' }, + { u: pending, role: 'editor', status: 'pending' }, + ] + .sort((a, b) => a.u.username.localeCompare(b.u.username)) + .map(({ u, role, status }) => ({ user_id: u.id, username: u.username, email: u.email, avatar: null, status, role, is_owner: false })), + ]; + const placeRow = testDb.prepare('SELECT * FROM collection_places WHERE collection_id = ?').get(col.id) as Record; + const ratingRows = testDb + .prepare('SELECT cpr.user_id, cpr.rating FROM collection_place_ratings cpr WHERE cpr.collection_place_id = ? ORDER BY cpr.created_at') + .all(place.id) as { user_id: number; rating: number }[]; + + expect(result.collection).toMatchObject({ + id: colRow.id, owner_id: colRow.owner_id, name: colRow.name, description: colRow.description, + color: colRow.color, is_owner: true, place_count: placeCount, + }); + expect(result.collection.members).toEqual(legacyMembers); + expect(result.places).toHaveLength(1); + expect(result.places[0]).toMatchObject({ + id: placeRow.id, collection_id: placeRow.collection_id, name: placeRow.name, lat: placeRow.lat, lng: placeRow.lng, + tags: [{ id: tag.id, name: tag.name, color: tag.color }], + }); + expect(result.places[0].ratings).toEqual(ratingRows.map((r) => ({ user_id: r.user_id, rating: r.rating, username: expect.any(String), avatar: null }))); + expect(result.places[0].rating_avg).toBe((5 + 3) / 2); + expect(result.places[0].rating_count).toBe(2); + }); + + it('COLLECTIONS-SVC-201: deleteCollection snapshots CL33/34 recipients BEFORE CL35 cascades — every legacy recipient is still notified once the rows are gone', async () => { + const owner = createUser(testDb).user; + const accepted = createUser(testDb).user; + const pending = createUser(testDb).user; + const col = await svc.createCollection(owner.id, { name: 'Ordering proof' }); + await svc.sendInvite(col.id, owner.id, owner.username, owner.email, accepted.id); + await svc.acceptInvite(accepted.id, col.id, undefined); + await svc.sendInvite(col.id, owner.id, owner.username, owner.email, pending.id); + + broadcastToUser.mockClear(); + // Captured at CALL time, not after `deleteCollection` returns — if the + // cascade ran before the snapshot (the reversed, wrong order), this would + // already be 0 even for the first broadcast; the proof is that it is + // ALWAYS 0, for EVERY broadcast, because the snapshot ran first and the + // cascade already happened by the time any broadcast fires. + const memberCountAtBroadcastTime: number[] = []; + broadcastToUser.mockImplementation(() => { + memberCountAtBroadcastTime.push((testDb.prepare('SELECT COUNT(*) AS n FROM collection_members WHERE collection_id = ?').get(col.id) as { n: number }).n); + }); + + await svc.deleteCollection(owner.id, col.id); + + const targets = broadcastToUser.mock.calls.map((c) => c[0]); + expect(targets).toEqual(expect.arrayContaining([accepted.id, pending.id])); + expect(memberCountAtBroadcastTime.every((n) => n === 0)).toBe(true); + expect(memberCountAtBroadcastTime.length).toBeGreaterThan(0); + }); +});