From 15c95032952249cce71c1228e28821052453a221 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 12 Oct 2026 21:13:57 +0000 Subject: [PATCH] refactor(settings): the planner views ask trip membership which trips a user can open, and their tests seed through the factories --- server/src/nest/settings/settings.module.ts | 9 ++++--- server/src/nest/settings/settings.service.ts | 7 +++--- server/tests/e2e/settings.e2e.test.ts | 25 +++++++++++-------- server/tests/helpers/plugin-host.ts | 14 ++--------- .../unit/nest/journey-share.service.test.ts | 10 +++++++- .../tests/unit/nest/settings.service.test.ts | 17 ++++++++++--- server/tests/unit/nest/share.service.test.ts | 11 ++++++-- server/tests/unit/nest/trips.service.test.ts | 12 +++++++-- 8 files changed, 66 insertions(+), 39 deletions(-) diff --git a/server/src/nest/settings/settings.module.ts b/server/src/nest/settings/settings.module.ts index 5ae5c5d3f..58b8b6d20 100644 --- a/server/src/nest/settings/settings.module.ts +++ b/server/src/nest/settings/settings.module.ts @@ -1,8 +1,8 @@ import { AppSettings } from '../../db/entities/AppSettings.entity'; import { Settings } from '../../db/entities/Settings.entity'; -import { Trips } from '../../db/entities/Trips.entity'; import { AppConfigModule } from '../app-config/app-config.module'; import { AuditModule } from '../audit/audit.module'; +import { TripMembershipModule } from '../trip-membership/trip-membership.module'; import { AdminDefaultUserSettingsController, SettingsController } from './settings.controller'; import { SettingsMcp } from './settings.mcp'; import { SettingsService } from './settings.service'; @@ -20,9 +20,10 @@ import { Module } from '@nestjs/common'; // (nothing resolves it off an ambient MikroORM request context — an // earlier design tried that and broke ~185 hand-built unit tests, see // task-5-review.md §4), so nothing in this module ever needs - // `@InjectRepository(Users)`. Trips is here for the planner view row (#2639), - // which keeps only trips the user can still open. - imports: [AppConfigModule, AuditModule, MikroOrmModule.forFeature([AppSettings, Settings, Trips])], + // `@InjectRepository(Users)`. TripMembershipModule is here for the planner + // view row (#2639), which keeps only trips the user can still open: the + // trips table belongs to another domain, so the ids come from its service. + imports: [AppConfigModule, AuditModule, TripMembershipModule, MikroOrmModule.forFeature([AppSettings, Settings])], controllers: [SettingsController, AdminDefaultUserSettingsController], providers: [SettingsService, SettingsMcp], exports: [SettingsService], diff --git a/server/src/nest/settings/settings.service.ts b/server/src/nest/settings/settings.service.ts index 80212915c..5209ae9fc 100644 --- a/server/src/nest/settings/settings.service.ts +++ b/server/src/nest/settings/settings.service.ts @@ -1,12 +1,11 @@ import { readEnv } from '../../app-config'; import { AppSettings } from '../../db/entities/AppSettings.entity'; import { Settings } from '../../db/entities/Settings.entity'; -import { Trips } from '../../db/entities/Trips.entity'; import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; import type { SettingsRepository } from '../../db/repositories/Settings.repository'; -import type { TripsRepository } from '../../db/repositories/Trips.repository'; import { decrypt_api_key, maybe_encrypt_api_key } from '../common/crypto/apiKeyCrypto'; import { UnitOfWork } from '../database/unit-of-work'; +import { TripMembershipService } from '../trip-membership/trip-membership.service'; import { InjectRepository } from '@mikro-orm/nestjs'; import { Injectable } from '@nestjs/common'; import { @@ -180,7 +179,7 @@ export class SettingsService { private readonly uow: UnitOfWork, @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, @InjectRepository(Settings) private readonly settings: SettingsRepository, - @InjectRepository(Trips) private readonly trips: TripsRepository, + private readonly membership: TripMembershipService, ) {} /** @@ -196,7 +195,7 @@ export class SettingsService { if (!parsed.success) return {}; const views = compactPlannerTripViews(parsed.data); if (Object.keys(views).length === 0) return views; - const openable = new Set((await this.trips.listAccessibleIds(userId)).map(String)); + const openable = new Set((await this.membership.listAccessibleTripIds(userId)).map(String)); return Object.fromEntries(Object.entries(views).filter(([tripId]) => openable.has(tripId))); } diff --git a/server/tests/e2e/settings.e2e.test.ts b/server/tests/e2e/settings.e2e.test.ts index 6b1f607b6..26faef0d7 100644 --- a/server/tests/e2e/settings.e2e.test.ts +++ b/server/tests/e2e/settings.e2e.test.ts @@ -7,6 +7,8 @@ */ import { AppSettings } from '../../src/db/entities/AppSettings.entity'; import { Settings } from '../../src/db/entities/Settings.entity'; +import { TripMembers } from '../../src/db/entities/TripMembers.entity'; +import { Trips } from '../../src/db/entities/Trips.entity'; import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; import { ZodValidationPipe } from '../../src/nest/common/zod-validation.pipe'; import { SettingsModule } from '../../src/nest/settings/settings.module'; @@ -33,11 +35,13 @@ const { db } = vi.hoisted(() => { key TEXT NOT NULL, value TEXT, UNIQUE(user_id, key));`); tmp.exec('CREATE TABLE app_settings (key TEXT PRIMARY KEY, value TEXT);'); // Only what the planner view row (#2639) asks: which trips the user can open. + // An ORM insert names every column the entity gives a default (currency, + // is_archived, reminder_days, the timestamps and added_at), so those are here too. tmp.exec(`CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER NOT NULL, - title TEXT NOT NULL DEFAULT 'Trip', created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec( - 'CREATE TABLE trip_members (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);', - ); + title TEXT NOT NULL DEFAULT 'Trip', currency TEXT, is_archived INTEGER DEFAULT 0, reminder_days INTEGER DEFAULT 3, + created_at DATETIME DEFAULT CURRENT_TIMESTAMP, updated_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); + tmp.exec(`CREATE TABLE trip_members (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, + user_id INTEGER NOT NULL, added_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); return { db: tmp }; }); @@ -335,12 +339,13 @@ describe('Settings e2e (real auth guard + temp SQLite)', () => { }); describe('planner_trip_views (#2639)', () => { - beforeEach(() => { - db.exec('DELETE FROM trip_members; DELETE FROM trips;'); - db.prepare('INSERT INTO trips (id, user_id) VALUES (?, ?)').run(41, 1); - db.prepare('INSERT INTO trips (id, user_id) VALUES (?, ?)').run(42, 2); - db.prepare('INSERT INTO trips (id, user_id) VALUES (?, ?)').run(43, 2); - db.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, ?)').run(43, 1); + beforeEach(async () => { + await deleteRows(orm, TripMembers); + await deleteRows(orm, Trips); + await insertRow(orm, Trips, { id: 41, user: 1, title: 'Trip' }); + await insertRow(orm, Trips, { id: 42, user: 2, title: 'Trip' }); + await insertRow(orm, Trips, { id: 43, user: 2, title: 'Trip' }); + await insertRow(orm, TripMembers, { trip: 43, user: 1 }); }); it('PUT keeps the switches of trips the user can open and GET hands them back', async () => { diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 9f81ab965..86f7136fc 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -413,12 +413,7 @@ export async function createPluginRpcHostParts( // Plan 4 Task 1 constructor-ripple: LlmConfigResolver's addon-row read // moved off DatabaseService onto AddonsRepository. const llmConfig = new LlmConfigResolver( - new SettingsService( - await createTestUnitOfWork(db), - appSettings, - await createTestSettingsRepo(db), - await createTestTripsRepo(db), - ), + new SettingsService(await createTestUnitOfWork(db), appSettings, await createTestSettingsRepo(db), membership), (await sharedTestOrm(db)).repo(Addons), addons, ); @@ -512,12 +507,7 @@ export async function createPluginRpcHostParts( generalStorage, await createTestUnitOfWork(db), (await sharedTestOrm(db)).em, - new SettingsService( - await createTestUnitOfWork(db), - appSettings, - await createTestSettingsRepo(db), - await createTestTripsRepo(db), - ), + new SettingsService(await createTestUnitOfWork(db), appSettings, await createTestSettingsRepo(db), membership), ); // Plan 4 Task 4: UserCleanupService's UC1 now goes through a directly-injected // EntityManager (MaintenanceRepository.deletePluginUserData), not DatabaseService. diff --git a/server/tests/unit/nest/journey-share.service.test.ts b/server/tests/unit/nest/journey-share.service.test.ts index add6d4a4b..d8e4a2b8b 100644 --- a/server/tests/unit/nest/journey-share.service.test.ts +++ b/server/tests/unit/nest/journey-share.service.test.ts @@ -22,6 +22,8 @@ import { JourneyShareService } from '../../../src/nest/journey/journey-share.ser import { TrekPhotoRegistrationService } from '../../../src/nest/photos/trek-photo-registration.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { SettingsService } from '../../../src/nest/settings/settings.service'; +import { TripMembershipModule } from '../../../src/nest/trip-membership/trip-membership.module'; +import { TripMembershipService } from '../../../src/nest/trip-membership/trip-membership.service'; import { createUser, createJourney, createJourneyEntry, addJourneyContributor } from '../../helpers/factories'; import { countRows, findRow, insertRow, insertRowIgnoringConflict, updateRows } from '../../helpers/factories/rows'; import { setAppSetting, setUserSetting } from '../../helpers/factories/settings'; @@ -36,6 +38,7 @@ import { import { createTestJourneyShareTokensRepo } from '../../helpers/journey-share-repos'; import { GALLERY_CHRONOLOGICAL_ORDER } from '../../helpers/legacy-gallery-order'; import { resetTestDb } from '../../helpers/test-db'; +import { createTestModule, type TestModule } from '../../helpers/test-module'; import type { TestOrm } from '../../helpers/test-orm'; import { sharedTestOrm, @@ -66,6 +69,7 @@ vi.mock('../../../src/db/database', async () => { let svc: JourneyShareService; let t: TestOrm; +let membership: TestModule; // `t`, `uow` and the settings repositories all derive from the SAME // `sharedTestOrm(testDb)` (task-2-review.md I2) — see settings.service.test.ts's @@ -76,6 +80,9 @@ beforeAll(async () => { const uow = await createTestUnitOfWork(testDb); t = await sharedTestOrm(testDb); const journeysRepo = await createTestJourneysRepo(testDb); + // SettingsService reads the trips a planner view may keep (#2639) through + // TripMembershipService, taken from its own module. + membership = await createTestModule({ db: testDb, imports: [TripMembershipModule] }); svc = new JourneyShareService( new JourneyDomainService( new RealtimeService(), @@ -99,7 +106,7 @@ beforeAll(async () => { uow, await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb), - await createTestTripsRepo(testDb), + membership.get(TripMembershipService), ), // Plan 3g Task 3: JourneyShareTokensRepository (JS1-JS15) + the // already-built JourneysRepository (JS8/JS12), same instance the domain @@ -124,6 +131,7 @@ beforeEach(() => { }); afterAll(async () => { + await membership.close(); await t.close(); testDb.close(); }); diff --git a/server/tests/unit/nest/settings.service.test.ts b/server/tests/unit/nest/settings.service.test.ts index 51b52e3db..02b45059c 100644 --- a/server/tests/unit/nest/settings.service.test.ts +++ b/server/tests/unit/nest/settings.service.test.ts @@ -9,18 +9,21 @@ */ import { db as testDb } from '../../../src/db/database'; import { Settings } from '../../../src/db/entities/Settings.entity'; +import { Trips } from '../../../src/db/entities/Trips.entity'; import { SettingsService } from '../../../src/nest/settings/settings.service'; +import { TripMembershipModule } from '../../../src/nest/trip-membership/trip-membership.module'; +import { TripMembershipService } from '../../../src/nest/trip-membership/trip-membership.service'; import { addTripMember, createTrip, createUser } from '../../helpers/factories'; -import { findRows } from '../../helpers/factories/rows'; +import { deleteRows, findRows } from '../../helpers/factories/rows'; import { readUserSetting, setAppSetting, setUserSetting } from '../../helpers/factories/settings'; import { resetTestDb } from '../../helpers/test-db'; +import { createTestModule, type TestModule } from '../../helpers/test-module'; import type { TestOrm } from '../../helpers/test-orm'; import { sharedTestOrm, createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo, - createTestTripsRepo, } from '../../helpers/test-uow'; import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; @@ -49,6 +52,7 @@ vi.mock('../../../src/nest/common/crypto/apiKeyCrypto', () => ({ let svc: SettingsService; let t: TestOrm; +let membership: TestModule; // `t`, `uow` and the two repositories all derive from the SAME `sharedTestOrm(testDb)` // (task-2-review.md I2): a second, independent `createTestOrm(testDb)` here would give @@ -56,13 +60,17 @@ let t: TestOrm; // opens its transaction on — a repository write inside setAdminUserDefaults/ // bulkUpsertSettings's transaction would then run outside it (or contend on a second // mutex over the same connection), exactly the trap the shared-ORM helpers exist to avoid. +// TripMembershipService, which lists the trips the planner view row (#2639) may keep, comes +// out of its own module instead of a constructor call here. Its container brings an ORM of +// its own, which is safe for it: that service only reads. beforeAll(async () => { t = await sharedTestOrm(testDb); + membership = await createTestModule({ db: testDb, imports: [TripMembershipModule] }); svc = new SettingsService( await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb), - await createTestTripsRepo(testDb), + membership.get(TripMembershipService), ); }); @@ -72,6 +80,7 @@ beforeEach(() => { }); afterAll(async () => { + await membership.close(); await t.close(); testDb.close(); }); @@ -435,7 +444,7 @@ describe('planner_trip_views', () => { const own = createTrip(testDb, user.id); const foreign = createTrip(testDb, other.id); const gone = createTrip(testDb, user.id); - testDb.prepare('DELETE FROM trips WHERE id = ?').run(gone.id); + await deleteRows(t, Trips, { id: gone.id }); await svc.upsertSetting(user.id, 'planner_trip_views', { [own.id]: { roadtrip: true }, [foreign.id]: { roadtrip: true }, diff --git a/server/tests/unit/nest/share.service.test.ts b/server/tests/unit/nest/share.service.test.ts index fa3a248bf..ea375c887 100644 --- a/server/tests/unit/nest/share.service.test.ts +++ b/server/tests/unit/nest/share.service.test.ts @@ -23,6 +23,8 @@ import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/plac import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; import { SettingsService } from '../../../src/nest/settings/settings.service'; import { ShareService, publicReservationMetadata } from '../../../src/nest/share/share.service'; +import { TripMembershipModule } from '../../../src/nest/trip-membership/trip-membership.module'; +import { TripMembershipService } from '../../../src/nest/trip-membership/trip-membership.service'; import type { User } from '../../../src/types'; import { createUser, @@ -39,13 +41,13 @@ import { deleteRows, findRow, insertRow, updateRows } from '../../helpers/factor import { setAppSetting, setUserSetting } from '../../helpers/factories/settings'; import { shareServiceRepoArgs } from '../../helpers/share-repos'; import { resetTestDb } from '../../helpers/test-db'; +import { createTestModule, type TestModule } from '../../helpers/test-module'; import type { TestOrm } from '../../helpers/test-orm'; import { sharedTestOrm, createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo, - createTestTripsRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, @@ -72,18 +74,22 @@ const photoCacheStub = { serveKey } as unknown as PlacePhotoCacheService; let svc: ShareService; let t: TestOrm; +let membership: TestModule; // `t`, `uow` and the settings repositories all derive from the SAME // `sharedTestOrm(testDb)` (task-2-review.md I2) — see settings.service.test.ts's // own comment on this pattern. beforeAll(async () => { t = await sharedTestOrm(testDb); + // SettingsService reads the trips a planner view may keep (#2639) through + // TripMembershipService, taken from its own module. + membership = await createTestModule({ db: testDb, imports: [TripMembershipModule] }); svc = new ShareService( new SettingsService( await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb), - await createTestTripsRepo(testDb), + membership.get(TripMembershipService), ), permissionsStub, new QueryHelpersService( @@ -105,6 +111,7 @@ beforeEach(() => { }); afterAll(async () => { + await membership.close(); await t.close(); testDb.close(); }); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index aecb6d3b9..d011e054d 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -146,6 +146,9 @@ import { Trips } from '../../../src/db/entities/Trips.entity'; import { Users } from '../../../src/db/entities/Users.entity'; import { legacyBoundIntegerText } from '../../../src/nest/common/row-id'; import { TripAccessService } from '../../../src/nest/trip-membership/trip-access.service'; +import { TripMembershipModule } from '../../../src/nest/trip-membership/trip-membership.module'; +import { TripMembershipService } from '../../../src/nest/trip-membership/trip-membership.service'; +import { createTestModule, type TestModule } from '../../helpers/test-module'; import { FakeRealtimeService } from '../../helpers/fake-realtime'; const realtime = new FakeRealtimeService(); @@ -226,6 +229,7 @@ let placesSvc: PlacesService; let svc: TripsService; let membersSvc: TripMembersService; let readModelSvc: TripReadModelService; +let membership: TestModule; beforeAll(async () => { dbsEm = (await sharedTestOrm(testDb)).em; photoCache = new PlacePhotoCacheService( @@ -277,6 +281,9 @@ beforeAll(async () => { await createTestCollectionPlacesRepo(testDb), buildPlaceImportService(), ); + // SettingsService reads the trips a planner view may keep (#2639) through TripMembershipService, + // which comes out of its own module instead of a constructor call here. + membership = await createTestModule({ db: testDb, imports: [TripMembershipModule] }); svc = new TripsService( new ReservationsService(new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), budgetSvc, realtime, notificationsStub(), new ReservationsReadService(await createTestReservationsRepo(testDb), await createTestReservationEndpointsRepo(testDb), await createTestReservationTravelersRepo(testDb), await createTestReservationDayPositionsRepo(testDb)), accommodationsSvc, await createTestUnitOfWork(testDb), await createTestReservationsRepo(testDb), await createTestReservationEndpointsRepo(testDb), await createTestReservationTravelersRepo(testDb), await createTestReservationDayPositionsRepo(testDb), await createTestDayAccommodationsRepo(testDb), await createTestDaysRepo(testDb), await createTestPlacesRepo(testDb), await createTestDayAssignmentsRepo(testDb), await createTestTripMembersRepo(testDb), await createTestUsersRepo(testDb), await createTestTripsRepo(testDb), await createTestBudgetItemsRepo(testDb)), daysSvc, @@ -296,7 +303,7 @@ beforeAll(async () => { coversFx.storage, await createTestUnitOfWork(testDb), (await sharedTestOrm(testDb)).em, - new SettingsService(await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb), await createTestTripsRepo(testDb)), + new SettingsService(await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb), membership.get(TripMembershipService)), ); membersSvc = new TripMembersService(budgetSvc, new UserCleanupService(new MaintenanceRepository(dbsEm!), budgetSvc, await createTestUnitOfWork(testDb), await createTestUsersRepo(testDb), await createTestTripMembersRepo(testDb), await createTestBudgetItemsRepo(testDb), await createTestBudgetSettlementsRepo(testDb), await createTestJourneyShareTokensRepo(testDb), await createTestJourneysRepo(testDb), await createTestJourneyEntriesRepo(testDb), await createTestJourneyContributorsRepo(testDb), await createTestShareTokensRepo(testDb), await createTestPluginsRepo(testDb), await createTestPluginUserErasureQueueRepo(testDb)), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), realtime, notificationsStub(), await createTestUnitOfWork(testDb), await createTestTripsRepo(testDb), await createTestTripMembersRepo(testDb), await createTestUsersRepo(testDb)); readModelSvc = new TripReadModelService( @@ -347,7 +354,8 @@ beforeEach(() => { resetTestDb(testDb); }); -afterAll(() => { +afterAll(async () => { + await membership.close(); testDb.close(); });