diff --git a/client/src/pages/tripPlanner/usePlannerMapView.test.ts b/client/src/pages/tripPlanner/usePlannerMapView.test.ts index a5599856f..34fee3c83 100644 --- a/client/src/pages/tripPlanner/usePlannerMapView.test.ts +++ b/client/src/pages/tripPlanner/usePlannerMapView.test.ts @@ -1,11 +1,12 @@ -// FE-TP-MAPVIEW-001 to FE-TP-MAPVIEW-012 +// FE-TP-MAPVIEW-001 to FE-TP-MAPVIEW-013 // // The planner's map view switches and its first fit, driven straight through // usePlannerMapView. Every toggle is called here, together with the storage each one // remembers its state in: the planner tests only ever read these values. import { act, renderHook } from '@testing-library/react' -import { resetAllStores } from '../../../tests/helpers/store' -import { buildPlace, buildTrip } from '../../../tests/helpers/factories' +import { resetAllStores, seedStore } from '../../../tests/helpers/store' +import { buildPlace, buildSettings, buildTrip } from '../../../tests/helpers/factories' +import { useSettingsStore } from '../../store/settingsStore' import type { Place, Trip } from '../../types' import { usePlannerMapView } from './usePlannerMapView' @@ -103,18 +104,21 @@ describe('usePlannerMapView', () => { expect(result.current.routeProfile).toBe('plugin:bikes/gravel') }) - it('FE-TP-MAPVIEW-007: the whole-trip overview flips and is kept per trip for the session', () => { + it('FE-TP-MAPVIEW-007: the whole-trip overview flips and is kept per trip, in the tab and on the account', () => { + seedStore(useSettingsStore, { settings: buildSettings(), isLoaded: true }) const { result } = renderMapView() act(() => { result.current.toggleOverview() }) expect(result.current.overviewShown).toBe(true) expect(sessionStorage.getItem('trip-overview-7')).toBe('1') + expect(useSettingsStore.getState().settings.planner_trip_views).toEqual({ '7': { overview: true } }) expect(renderMapView().result.current.overviewShown).toBe(true) expect(renderMapView({ tripId: 8 }).result.current.overviewShown).toBe(false) act(() => { result.current.toggleOverview() }) expect(result.current.overviewShown).toBe(false) expect(sessionStorage.getItem('trip-overview-7')).toBe('0') + expect(useSettingsStore.getState().settings.planner_trip_views).toEqual({}) }) it('FE-TP-MAPVIEW-008: the map lock flips, is read through its ref and is remembered by the browser', () => { @@ -160,12 +164,14 @@ describe('usePlannerMapView', () => { expect(result.current.isMobileRef.current).toBe(true) }) - it('FE-TP-MAPVIEW-011: the recorded trail flips and is kept per trip for the session', () => { + it('FE-TP-MAPVIEW-011: the recorded trail flips and is kept per trip, in the tab and on the account', () => { + seedStore(useSettingsStore, { settings: buildSettings(), isLoaded: true }) const { result } = renderMapView() act(() => { result.current.toggleDawarichTrail() }) expect(result.current.dawarichTrailShown).toBe(true) expect(sessionStorage.getItem('trip-dawarich-7')).toBe('1') + expect(useSettingsStore.getState().settings.planner_trip_views).toEqual({ '7': { dawarich: true } }) // The addon is off in this store, so switching the layer on asks nothing of the network. expect(result.current.dawarichTrail.status).toBe('idle') expect(renderMapView().result.current.dawarichTrailShown).toBe(true) @@ -175,6 +181,19 @@ describe('usePlannerMapView', () => { expect(sessionStorage.getItem('trip-dawarich-7')).toBe('0') }) + it('FE-TP-MAPVIEW-013: the overview and the trail switched on in another browser are on here too (#2639)', () => { + // A fresh tab: nothing in sessionStorage, only the account's settings. + seedStore(useSettingsStore, { + settings: buildSettings({ planner_trip_views: { '7': { overview: true, dawarich: true } } }), + isLoaded: true, + }) + + const { result } = renderMapView() + expect(result.current.overviewShown).toBe(true) + expect(result.current.dawarichTrailShown).toBe(true) + expect(renderMapView({ tripId: 8 }).result.current.overviewShown).toBe(false) + }) + it('FE-TP-MAPVIEW-012: each trip gets one automatic fit, once it has a place to fit to', () => { const tripA = buildTrip({ id: 1 }) const tripB = buildTrip({ id: 2 }) diff --git a/client/src/pages/tripPlanner/usePlannerMapView.ts b/client/src/pages/tripPlanner/usePlannerMapView.ts index f43c3dc77..dbd2ab107 100644 --- a/client/src/pages/tripPlanner/usePlannerMapView.ts +++ b/client/src/pages/tripPlanner/usePlannerMapView.ts @@ -1,5 +1,6 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { useDawarichTrail } from '../../components/Map/useDawarichTrail' +import { usePlannerTripView } from './usePlannerTripView' import type { TripStoreState } from '../../store/tripStore' import type { Place } from '../../types' import type { PlannerBase } from './plannerTypes' @@ -52,16 +53,10 @@ export function usePlannerMapView(options: PlannerMapViewOptions) { const transitRoutesShown = routeShown && selectedDayId != null const [routeProfile, setRouteProfile] = useState('driving') // Whole-trip route overview (#1736): every day's route at once, each in its own - // colour. Per trip and per session like road trip mode: it answers "what does the - // whole thing look like", which is a question you ask of one trip, not a preference. - const [overviewShown, setOverviewShown] = useState(() => sessionStorage.getItem(`trip-overview-${tripId}`) === '1') - const toggleOverview = useCallback(() => { - setOverviewShown(prev => { - const next = !prev - sessionStorage.setItem(`trip-overview-${tripId}`, next ? '1' : '0') - return next - }) - }, [tripId]) + // colour. Per trip like road trip mode, and remembered on the account the same + // way (#2639): it answers "what does the whole thing look like", which is a + // question you ask of one trip, not a preference. + const [overviewShown, toggleOverview] = usePlannerTripView(tripId, 'overview') // A locked map stays where the traveller put it (#2010): picking a day or a place no // longer zooms or pans it. Remembered per browser, like the other view toggles; the // first frame of a trip still fits, or the map would open on nothing. @@ -79,19 +74,11 @@ export function usePlannerMapView(options: PlannerMapViewOptions) { return next }) }, []) - // The recorded route from Dawarich (#2279), per trip and per session for the - // same reason as the overview above: it answers "what actually happened on - // this trip", which is a question about one trip rather than a preference. - const [dawarichTrailShown, setDawarichTrailShown] = useState( - () => sessionStorage.getItem(`trip-dawarich-${tripId}`) === '1', - ) - const toggleDawarichTrail = useCallback(() => { - setDawarichTrailShown(prev => { - const next = !prev - sessionStorage.setItem(`trip-dawarich-${tripId}`, next ? '1' : '0') - return next - }) - }, [tripId]) + // The recorded route from Dawarich (#2279), per trip and remembered on the + // account for the same reason as the overview above: it answers "what + // actually happened on this trip", a question about one trip rather than a + // preference. + const [dawarichTrailShown, toggleDawarichTrail] = usePlannerTripView(tripId, 'dawarich') // Fetched here rather than in MapViewAuto so the desktop and the phone share // one request, and so the pill that toggles it can show why there is no line. const dawarichTrail = useDawarichTrail(tripId, dawarichTrailShown) diff --git a/client/src/pages/tripPlanner/usePlannerTripView.test.ts b/client/src/pages/tripPlanner/usePlannerTripView.test.ts new file mode 100644 index 000000000..3d984b010 --- /dev/null +++ b/client/src/pages/tripPlanner/usePlannerTripView.test.ts @@ -0,0 +1,113 @@ +// FE-TP-TRIPVIEW-001 to FE-TP-TRIPVIEW-006 +// +// The planner's view switches the account remembers per trip (#2639): road trip +// mode, the Dawarich trail and the route overview. The tab keeps a copy that +// answers before the settings arrive. +import { act, renderHook, waitFor } from '@testing-library/react' +import { http, HttpResponse } from 'msw' +import { server } from '../../../tests/helpers/msw/server' +import { resetAllStores, seedStore } from '../../../tests/helpers/store' +import { buildSettings } from '../../../tests/helpers/factories' +import { useSettingsStore } from '../../store/settingsStore' +import { usePlannerTripView } from './usePlannerTripView' + +function captureSettingWrites() { + const writes: Array<{ key: string; value: unknown }> = [] + server.use(http.put('/api/settings', async ({ request }) => { + const body = await request.json() as { key: string; value: unknown } + writes.push(body) + return HttpResponse.json({ success: true, key: body.key, value: body.value }) + })) + return writes +} + +beforeEach(() => { + resetAllStores() +}) + +describe('usePlannerTripView', () => { + it('FE-TP-TRIPVIEW-001: a fresh browser opens the trip with the switches the account has on', () => { + seedStore(useSettingsStore, { + settings: buildSettings({ planner_trip_views: { '42': { roadtrip: true } } }), + isLoaded: true, + }) + expect(renderHook(() => usePlannerTripView(42, 'roadtrip')).result.current[0]).toBe(true) + expect(renderHook(() => usePlannerTripView(42, 'dawarich')).result.current[0]).toBe(false) + expect(renderHook(() => usePlannerTripView(43, 'roadtrip')).result.current[0]).toBe(false) + }) + + it('FE-TP-TRIPVIEW-002: the tab copy answers until the settings are here, then the account decides', () => { + sessionStorage.setItem('trip-roadtrip-42', '1') + const { result } = renderHook(() => usePlannerTripView(42, 'roadtrip')) + expect(result.current[0]).toBe(true) + + // Switched off on another device since this tab last looked. + act(() => { seedStore(useSettingsStore, { settings: buildSettings(), isLoaded: true }) }) + expect(result.current[0]).toBe(false) + }) + + it('FE-TP-TRIPVIEW-003: a switch is saved on the account next to the other trips, and only what is on', async () => { + const writes = captureSettingWrites() + seedStore(useSettingsStore, { + settings: buildSettings({ planner_trip_views: { '7': { overview: true }, '42': { dawarich: true } } }), + isLoaded: true, + }) + const { result } = renderHook(() => usePlannerTripView(42, 'roadtrip')) + + act(() => { result.current[1]() }) + expect(result.current[0]).toBe(true) + expect(sessionStorage.getItem('trip-roadtrip-42')).toBe('1') + await waitFor(() => expect(writes).toHaveLength(1)) + expect(writes[0]).toEqual({ + key: 'planner_trip_views', + value: { '7': { overview: true }, '42': { dawarich: true, roadtrip: true } }, + }) + + act(() => { result.current[1]() }) + expect(result.current[0]).toBe(false) + await waitFor(() => expect(writes).toHaveLength(2)) + expect(writes[1].value).toEqual({ '7': { overview: true }, '42': { dawarich: true } }) + }) + + it('FE-TP-TRIPVIEW-004: a switch made before the settings arrived waits for them instead of erasing other trips', async () => { + const writes = captureSettingWrites() + server.use(http.get('/api/settings', () => HttpResponse.json({ + settings: buildSettings({ planner_trip_views: { '7': { overview: true } } }), + }))) + const { result } = renderHook(() => usePlannerTripView(42, 'dawarich')) + + act(() => { result.current[1]() }) + expect(result.current[0]).toBe(true) + await waitFor(() => expect(writes).toHaveLength(1)) + expect(writes[0].value).toEqual({ '7': { overview: true }, '42': { dawarich: true } }) + expect(result.current[0]).toBe(true) + }) + + it('FE-TP-TRIPVIEW-005: a save that fails keeps the switch for the session', async () => { + let tried = false + server.use(http.put('/api/settings', () => { + tried = true + return HttpResponse.json({ error: 'nope' }, { status: 500 }) + })) + seedStore(useSettingsStore, { settings: buildSettings(), isLoaded: true }) + const { result } = renderHook(() => usePlannerTripView(42, 'overview')) + + act(() => { result.current[1]() }) + await waitFor(() => expect(tried).toBe(true)) + await act(async () => { await Promise.resolve() }) + expect(result.current[0]).toBe(true) + expect(sessionStorage.getItem('trip-overview-42')).toBe('1') + }) + + it('FE-TP-TRIPVIEW-006: settings that cannot be loaded leave the tab copy in charge and write nothing', async () => { + const writes = captureSettingWrites() + server.use(http.get('/api/settings', () => HttpResponse.json({ error: 'offline' }, { status: 503 }))) + vi.spyOn(console, 'error').mockImplementation(() => {}) + const { result } = renderHook(() => usePlannerTripView(42, 'roadtrip')) + + act(() => { result.current[1]() }) + await act(async () => { await new Promise(r => setTimeout(r, 20)) }) + expect(result.current[0]).toBe(true) + expect(writes).toHaveLength(0) + }) +}) diff --git a/client/src/pages/tripPlanner/usePlannerTripView.ts b/client/src/pages/tripPlanner/usePlannerTripView.ts new file mode 100644 index 000000000..57e2aaa04 --- /dev/null +++ b/client/src/pages/tripPlanner/usePlannerTripView.ts @@ -0,0 +1,70 @@ +import { useCallback, useState } from 'react' +import { PLANNER_TRIP_VIEWS_KEY, compactPlannerTripViews, type PlannerTripViewFlag } from '@trek/shared' +import { useSettingsStore } from '../../store/settingsStore' + +function readSession(key: string): boolean | null { + try { + const raw = sessionStorage.getItem(key) + return raw === '1' ? true : raw === '0' ? false : null + } catch { + return null + } +} + +function writeSession(key: string, on: boolean): void { + try { sessionStorage.setItem(key, on ? '1' : '0') } catch { /* the account row still has it */ } +} + +interface TabCopy { + key: string + on: boolean | null + /** A switch made here that the account row has not taken yet. */ + pending: boolean +} + +/** + * One of the planner's remembered view switches: road trip mode, the Dawarich + * trail or the route overview (#2639). The account keeps them per trip, so they + * are back in another browser or on another device. The tab keeps a copy of + * its own that answers until the account's settings have arrived, so a reload + * does not open on the plain view and then jump. Once they are here they + * decide, because a switch flipped on another device is newer than this tab. + */ +export function usePlannerTripView(tripId: number, flag: PlannerTripViewFlag): [boolean, () => void] { + const sessionKey = `trip-${flag}-${tripId}` + const saved = useSettingsStore(s => s.settings.planner_trip_views?.[String(tripId)]?.[flag] === true) + const loaded = useSettingsStore(s => s.isLoaded) + const [local, setLocal] = useState(() => ({ key: sessionKey, on: readSession(sessionKey), pending: false })) + // A different trip in the same planner reads its own copy instead of keeping this one. + const copy = local.key === sessionKey ? local : { key: sessionKey, on: readSession(sessionKey), pending: false } + const shown = loaded && !copy.pending ? saved : (copy.on ?? saved) + + const toggle = useCallback(() => { + const next = !shown + setLocal({ key: sessionKey, on: next, pending: true }) + writeSession(sessionKey, next) + void remember(tripId, flag, next).finally(() => { + setLocal(prev => (prev.key === sessionKey && prev.on === next ? { ...prev, pending: false } : prev)) + }) + }, [shown, sessionKey, tripId, flag]) + + return [shown, toggle] +} + +async function remember(tripId: number, flag: PlannerTripViewFlag, on: boolean): Promise { + const store = useSettingsStore.getState() + // The row holds every trip, so it is only written over once the account's + // own copy is here; otherwise one switch would erase all the others. The + // store takes the new value before the request goes out, so an offline save + // still shows for the rest of the session. + if (!store.isLoaded) await store.loadSettings() + const { settings, isLoaded, updateSetting } = useSettingsStore.getState() + if (!isLoaded) return + const views = { ...settings.planner_trip_views } + views[String(tripId)] = { ...views[String(tripId)], [flag]: on } + try { + await updateSetting(PLANNER_TRIP_VIEWS_KEY, compactPlannerTripViews(views)) + } catch { + // Offline or refused: the store and the tab copy keep the switch for this session. + } +} diff --git a/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx index 7b343ea96..dac548894 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx @@ -344,8 +344,11 @@ beforeEach(() => { }) as unknown as typeof window.__addToast seedStore(useAuthStore, { user: buildUser({ id: 1 }), isAuthenticated: true, placesPhotosEnabled: false }) - // The two switches this whole file depends on. + // The two switches this whole file depends on. The mode sits on the account as well + // as in the tab (#2639), with the settings already loaded, so a toggle saves straight + // away instead of leaving a settings fetch behind that lands in the next test. sessionStorage.setItem('trip-roadtrip-42', '1') + useSettingsStore.setState(s => ({ isLoaded: true, settings: { ...s.settings, planner_trip_views: { '42': { roadtrip: true } } } })) vi.spyOn(addonsApi, 'enabled').mockResolvedValue({ addons: [{ id: 'roadtrip' }] } as never) vi.spyOn(authApi, 'getAppConfig').mockResolvedValue({}) diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index 1433a6132..b2a6c4445 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -54,6 +54,7 @@ import { usePlaceEdits } from './usePlaceEdits' import { usePlaceFormOpeners } from './usePlaceFormOpeners' import { useDayPlanEdits } from './useDayPlanEdits' import { useBookingEdits } from './useBookingEdits' +import { usePlannerTripView } from './usePlannerTripView' import { useIsPhone } from '../../mobile/useIsPhone' import { useRoadtripVias } from '../../components/Roadtrip/useRoadtripVias' @@ -116,10 +117,10 @@ export function useTripPlanner() { // asked to open ('collab' in particular, guessed off) survives the gap. const [addonsLoaded, setAddonsLoaded] = useState(false) // Road trip mode swaps the plan view's left rail (and later its map layer) for the - // drive-first reading of the same trip. Per trip and per session, like the tab choice: - // someone planning a road trip stays in it across reloads without it leaking into - // their next, non-driving trip. - const [storedRoadtripMode, setRoadtripMode] = useState(() => sessionStorage.getItem(`trip-roadtrip-${tripId}`) === '1') + // drive-first reading of the same trip. Remembered per trip on the account (#2639): + // someone planning a road trip finds it again in another browser without it + // leaking into their next, non-driving trip. + const [storedRoadtripMode, toggleRoadtripMode] = usePlannerTripView(tripId, 'roadtrip') // Declared here rather than with the other layout state further down, because // road-trip mode is decided on it and the assignment and place lists below are // decided on that. One subscriber for the whole hook. @@ -151,13 +152,6 @@ export function useTripPlanner() { Object.entries(storedAssignments).map(([dayId, visits]) => [dayId, visits.filter(v => !hidden(v))]), ) }, [roadtripMode, roadtripSettings.roadtrip_service_stops_in_days, storedAssignments]) - const toggleRoadtripMode = useCallback(() => { - setRoadtripMode(prev => { - const next = !prev - sessionStorage.setItem(`trip-roadtrip-${tripId}`, next ? '1' : '0') - return next - }) - }, [tripId]) const [collabFeatures, setCollabFeatures] = useState<{ chat: boolean; notes: boolean; links: boolean; polls: boolean; whatsnext: boolean }>({ chat: true, notes: true, links: true, polls: true, whatsnext: true }) const [tripAccommodations, setTripAccommodations] = useState([]) const places = useMemo( diff --git a/client/src/types.ts b/client/src/types.ts index bb2355834..2c07c4bd0 100644 --- a/client/src/types.ts +++ b/client/src/types.ts @@ -6,6 +6,7 @@ // each documented against the producing service). Re-exported here so the rest // of the client keeps importing from '../types' unchanged. import type { + PlannerTripViews, TrekWsEventName, TrekWsPluginEventName, Trip, @@ -237,6 +238,8 @@ export interface Settings { start_page?: 'dashboard' | 'active_trip' /** Which planner tab 'active_trip' opens on — a TripTabId (constants/tripTabs). */ start_trip_tab?: string + /** Road trip mode, the Dawarich trail and the route overview per trip, only those that are on (#2639). */ + planner_trip_views?: PlannerTripViews // AI booking-import fallback (per-user config; used when the admin has not set // instance-wide config on the llm_parsing addon). llm_api_key is masked on read. llm_provider?: 'local' | 'openai' | 'anthropic' diff --git a/server/src/nest/settings/settings.controller.ts b/server/src/nest/settings/settings.controller.ts index 2bff79b52..d2977717a 100644 --- a/server/src/nest/settings/settings.controller.ts +++ b/server/src/nest/settings/settings.controller.ts @@ -12,7 +12,9 @@ import { SettingsService, isAdminOnlyEndpointSetting } from './settings.service' import { Body, Controller, Get, HttpCode, HttpException, Post, Put, Req, UseGuards } from '@nestjs/common'; import { MASKED_SETTING_VALUE, + PLANNER_TRIP_VIEWS_KEY, adminDefaultUserSettingsResponseSchema, + plannerTripViewsSchema, settingUpsertResponseSchema, settingsBulkResponseSchema, settingsListResponseSchema, @@ -61,6 +63,22 @@ export class SettingsController { } } + /** + * The one value with a shape the route checks (#2639): the planner's view + * switches per trip. Clearing it stays open, like every other key. + */ + private assertValidValues(settings: Record) { + const value = settings[PLANNER_TRIP_VIEWS_KEY]; + if (value === undefined || value === null) return; + const parsed = plannerTripViewsSchema.safeParse(value); + if (!parsed.success) { + throw new HttpException( + { error: `Invalid value for ${PLANNER_TRIP_VIEWS_KEY}: ${parsed.error.issues[0]?.message ?? 'not accepted'}` }, + 400, + ); + } + } + @Get() @ResponseContract(settingsListResponseSchema) async list(@CurrentUser() user: User) { @@ -71,6 +89,7 @@ export class SettingsController { @ResponseContract(settingUpsertResponseSchema) async upsert(@CurrentUser() user: User, @Body() body: SettingUpsertDto) { this.assertMayWriteInstanceEndpoint({ [body.key]: body.value }); + this.assertValidValues({ [body.key]: body.value }); // assertMayWriteInstanceEndpoint only covers llm_base_url and provider 'local'. // llm_api_key and llm_model are writable by every user, and on a managed // install both cost the operator money, so the key list decides here. @@ -90,6 +109,7 @@ export class SettingsController { @ResponseContract(settingsBulkResponseSchema) async bulk(@CurrentUser() user: User, @Body() body: SettingsBulkDto) { this.assertMayWriteInstanceEndpoint(body.settings); + this.assertValidValues(body.settings); const { allowed, blocked } = splitManagedKeys(body.settings, this.env.isManaged()); try { const updated = await this.settings.bulkUpsertSettings(user.id, allowed); diff --git a/server/src/nest/settings/settings.module.ts b/server/src/nest/settings/settings.module.ts index fcb9989dd..5ae5c5d3f 100644 --- a/server/src/nest/settings/settings.module.ts +++ b/server/src/nest/settings/settings.module.ts @@ -1,5 +1,6 @@ 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 { AdminDefaultUserSettingsController, SettingsController } from './settings.controller'; @@ -19,8 +20,9 @@ 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)`. - imports: [AppConfigModule, AuditModule, MikroOrmModule.forFeature([AppSettings, Settings])], + // `@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])], 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 41ccd36f0..8af4c956f 100644 --- a/server/src/nest/settings/settings.service.ts +++ b/server/src/nest/settings/settings.service.ts @@ -1,13 +1,23 @@ 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 { InjectRepository } from '@mikro-orm/nestjs'; import { Injectable } from '@nestjs/common'; -import { MASKED_SETTING_VALUE, WEEK_START_VALUES, normalizeAppearance } from '@trek/shared'; +import { + MASKED_SETTING_VALUE, + PLANNER_TRIP_VIEWS_KEY, + WEEK_START_VALUES, + compactPlannerTripViews, + normalizeAppearance, + plannerTripViewsSchema, + type PlannerTripViews, +} from '@trek/shared'; /** * Exported so a caller that hands settings to somebody else can assert its own @@ -170,8 +180,30 @@ 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, ) {} + /** + * The planner's remembered view switches (#2639), reduced to trips this user + * can still open and to switches that are on. The client only ever adds to + * the row, so this is where a deleted trip, or one the user was removed + * from, leaves it again. Anything the contract does not accept is stored as + * an empty row; the controller has already answered 400 for a request. + */ + private async keepOpenableTripViews(userId: number, value: unknown): Promise { + if (value === null || value === undefined) return null; + const parsed = plannerTripViewsSchema.safeParse(value); + 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)); + return Object.fromEntries(Object.entries(views).filter(([tripId]) => openable.has(tripId))); + } + + private async prepareValue(userId: number, key: string, value: unknown): Promise { + return key === PLANNER_TRIP_VIEWS_KEY ? await this.keepOpenableTripViews(userId, value) : value; + } + async getAdminUserDefaults(): Promise> { const rows = await this.appSettings.findByKeyPrefix('default_user_setting_'); const defaults: Record = {}; @@ -297,7 +329,8 @@ export class SettingsService { } async upsertSetting(userId: number, key: string, value: unknown) { - await this.settings.upsertForUser(userId, key, serializeValue(key, value)); + const prepared = await this.prepareValue(userId, key, value); + await this.settings.upsertForUser(userId, key, serializeValue(key, prepared)); } async bulkUpsertSettings(userId: number, settings: Record) { @@ -308,7 +341,8 @@ export class SettingsService { // bulk save can never overwrite a stored secret with the mask (the // single-upsert route has the same no-op in the controller). if (value === MASKED_SETTING_VALUE) continue; - await this.settings.upsertForUser(userId, key, serializeValue(key, value)); + const prepared = await this.prepareValue(userId, key, value); + await this.settings.upsertForUser(userId, key, serializeValue(key, prepared)); written++; } }); diff --git a/server/tests/e2e/settings.e2e.test.ts b/server/tests/e2e/settings.e2e.test.ts index 7150018ce..6b1f607b6 100644 --- a/server/tests/e2e/settings.e2e.test.ts +++ b/server/tests/e2e/settings.e2e.test.ts @@ -32,6 +32,12 @@ const { db } = vi.hoisted(() => { tmp.exec(`CREATE TABLE settings (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER NOT NULL, 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. + 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);', + ); return { db: tmp }; }); @@ -327,4 +333,57 @@ describe('Settings e2e (real auth guard + temp SQLite)', () => { expect(await findRow(orm, AppSettings, { key: 'default_user_setting_week_start' })).toBeNull(); }); }); + + 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); + }); + + it('PUT keeps the switches of trips the user can open and GET hands them back', async () => { + const put = await request(server) + .put('/api/settings') + .set('Cookie', sessionCookie(1)) + .send({ + key: 'planner_trip_views', + value: { + '41': { roadtrip: true }, + '42': { roadtrip: true }, + '43': { dawarich: true, overview: false }, + '99': { overview: true }, + }, + }); + expect(put.status).toBe(200); + const res = await request(server).get('/api/settings').set('Cookie', sessionCookie(1)); + expect(res.body.settings.planner_trip_views).toEqual({ '41': { roadtrip: true }, '43': { dawarich: true } }); + }); + + it('bulk prunes the same way', async () => { + const res = await request(server) + .post('/api/settings/bulk') + .set('Cookie', sessionCookie(1)) + .send({ settings: { planner_trip_views: { '41': { overview: true }, '42': { overview: true } } } }); + expect(res.status).toBe(200); + expect(JSON.parse(String(await userSetting(1, 'planner_trip_views')))).toEqual({ '41': { overview: true } }); + }); + + it('400 for a value outside the contract, on both routes, and nothing is stored', async () => { + const put = await request(server) + .put('/api/settings') + .set('Cookie', sessionCookie(1)) + .send({ key: 'planner_trip_views', value: { '41': { roadtrip: 'on' } } }); + expect(put.status).toBe(400); + expect(put.body.error).toMatch(/planner_trip_views/); + const bulk = await request(server) + .post('/api/settings/bulk') + .set('Cookie', sessionCookie(1)) + .send({ settings: { theme: 'dark', planner_trip_views: ['41'] } }); + expect(bulk.status).toBe(400); + expect(await userSetting(1, 'planner_trip_views')).toBeNull(); + expect(await userSetting(1, 'theme')).toBeNull(); + }); + }); }); diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index faafc6450..ea235693a 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -413,7 +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)), + new SettingsService(await createTestUnitOfWork(db), appSettings, await createTestSettingsRepo(db), await createTestTripsRepo(db)), (await sharedTestOrm(db)).repo(Addons), addons, ); @@ -507,7 +507,7 @@ export async function createPluginRpcHostParts( generalStorage, await createTestUnitOfWork(db), (await sharedTestOrm(db)).em, - new SettingsService(await createTestUnitOfWork(db), appSettings, await createTestSettingsRepo(db)), + new SettingsService(await createTestUnitOfWork(db), appSettings, await createTestSettingsRepo(db), await createTestTripsRepo(db)), ); // 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 362560cd8..add6d4a4b 100644 --- a/server/tests/unit/nest/journey-share.service.test.ts +++ b/server/tests/unit/nest/journey-share.service.test.ts @@ -95,7 +95,12 @@ beforeAll(async () => { await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), ), - new SettingsService(uow, await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb)), + new SettingsService( + uow, + await createTestAppSettingsRepo(testDb), + await createTestSettingsRepo(testDb), + await createTestTripsRepo(testDb), + ), // Plan 3g Task 3: JourneyShareTokensRepository (JS1-JS15) + the // already-built JourneysRepository (JS8/JS12), same instance the domain // service above uses. diff --git a/server/tests/unit/nest/settings.service.test.ts b/server/tests/unit/nest/settings.service.test.ts index 22845a309..51b52e3db 100644 --- a/server/tests/unit/nest/settings.service.test.ts +++ b/server/tests/unit/nest/settings.service.test.ts @@ -10,7 +10,7 @@ import { db as testDb } from '../../../src/db/database'; import { Settings } from '../../../src/db/entities/Settings.entity'; import { SettingsService } from '../../../src/nest/settings/settings.service'; -import { createUser } from '../../helpers/factories'; +import { addTripMember, createTrip, createUser } from '../../helpers/factories'; import { findRows } from '../../helpers/factories/rows'; import { readUserSetting, setAppSetting, setUserSetting } from '../../helpers/factories/settings'; import { resetTestDb } from '../../helpers/test-db'; @@ -20,6 +20,7 @@ import { createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo, + createTestTripsRepo, } from '../../helpers/test-uow'; import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; @@ -61,6 +62,7 @@ beforeAll(async () => { await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb), + await createTestTripsRepo(testDb), ); }); @@ -406,3 +408,61 @@ describe('legacy quirk fixes', () => { expect(await svc.bulkUpsertSettings(user.id, { a: '1', b: '••••••••' })).toBe(1); }); }); + +// #2639: road trip mode, the Dawarich trail and the route overview follow the +// user to another browser through one settings row, and that row must not keep +// trips the user can no longer open. +describe('planner_trip_views', () => { + it('SET-SVC-037 — keeps the switches of trips the user owns or is a member of', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + const own = createTrip(testDb, user.id); + const shared = createTrip(testDb, other.id); + addTripMember(testDb, shared.id, user.id); + await svc.upsertSetting(user.id, 'planner_trip_views', { + [own.id]: { roadtrip: true }, + [shared.id]: { dawarich: true, overview: true }, + }); + expect((await svc.getUserSettings(user.id)).planner_trip_views).toEqual({ + [own.id]: { roadtrip: true }, + [shared.id]: { dawarich: true, overview: true }, + }); + }); + + it('SET-SVC-038 — drops trips the user cannot open, deleted ones included, on every write', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + 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 svc.upsertSetting(user.id, 'planner_trip_views', { + [own.id]: { roadtrip: true }, + [foreign.id]: { roadtrip: true }, + [gone.id]: { overview: true }, + }); + expect(JSON.parse(String(await readUserSetting(t, user.id, 'planner_trip_views')))).toEqual({ + [own.id]: { roadtrip: true }, + }); + }); + + it('SET-SVC-039 — stores only switches that are on, and the bulk path does the same', async () => { + const { user } = createUser(testDb); + const a = createTrip(testDb, user.id); + const b = createTrip(testDb, user.id); + await svc.bulkUpsertSettings(user.id, { + planner_trip_views: { [a.id]: { roadtrip: false, overview: true }, [b.id]: { dawarich: false } }, + }); + expect(JSON.parse(String(await readUserSetting(t, user.id, 'planner_trip_views')))).toEqual({ + [a.id]: { overview: true }, + }); + }); + + it('SET-SVC-040 — a value outside the contract is stored as an empty row, null clears it', async () => { + const { user } = createUser(testDb); + await svc.upsertSetting(user.id, 'planner_trip_views', { abc: { roadtrip: 'yes' } }); + expect(await readUserSetting(t, user.id, 'planner_trip_views')).toBe('{}'); + await svc.upsertSetting(user.id, 'planner_trip_views', null); + expect(await readUserSetting(t, user.id, 'planner_trip_views')).toBe(''); + }); +}); diff --git a/server/tests/unit/nest/share.service.test.ts b/server/tests/unit/nest/share.service.test.ts index c955776ba..fa3a248bf 100644 --- a/server/tests/unit/nest/share.service.test.ts +++ b/server/tests/unit/nest/share.service.test.ts @@ -45,6 +45,7 @@ import { createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo, + createTestTripsRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, @@ -82,6 +83,7 @@ beforeAll(async () => { await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb), + await createTestTripsRepo(testDb), ), permissionsStub, new QueryHelpersService( diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index e06345246..6cc6bb59b 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -296,7 +296,7 @@ beforeAll(async () => { coversFx.storage, await createTestUnitOfWork(testDb), (await sharedTestOrm(testDb)).em, - new SettingsService(await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb)), + new SettingsService(await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb), await createTestTripsRepo(testDb)), ); 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( diff --git a/shared/src/settings/settings.schema.spec.ts b/shared/src/settings/settings.schema.spec.ts index e3dc6dd9b..4dc4ff6d8 100644 --- a/shared/src/settings/settings.schema.spec.ts +++ b/shared/src/settings/settings.schema.spec.ts @@ -1,4 +1,11 @@ -import { settingUpsertRequestSchema, settingsBulkRequestSchema, MASKED_SETTING_VALUE } from './settings.schema'; +import { + settingUpsertRequestSchema, + settingsBulkRequestSchema, + MASKED_SETTING_VALUE, + PLANNER_TRIP_VIEWS_MAX, + compactPlannerTripViews, + plannerTripViewsSchema, +} from './settings.schema'; import { describe, it, expect } from 'vitest'; @@ -23,3 +30,36 @@ describe('MASKED_SETTING_VALUE', () => { expect(MASKED_SETTING_VALUE).toBe('••••••••'); }); }); + +describe('plannerTripViewsSchema (#2639)', () => { + it('takes the three switches per trip id', () => { + expect( + plannerTripViewsSchema.safeParse({ '12': { roadtrip: true, dawarich: false, overview: true } }).success, + ).toBe(true); + expect(plannerTripViewsSchema.safeParse({}).success).toBe(true); + }); + + it('refuses a key that is not a trip id and a switch that is not a boolean', () => { + expect(plannerTripViewsSchema.safeParse({ abc: { roadtrip: true } }).success).toBe(false); + expect(plannerTripViewsSchema.safeParse({ '0': { roadtrip: true } }).success).toBe(false); + expect(plannerTripViewsSchema.safeParse({ '12': { roadtrip: 'yes' } }).success).toBe(false); + expect(plannerTripViewsSchema.safeParse(['12']).success).toBe(false); + }); + + it('refuses more trips than the row is meant to hold', () => { + const many = Object.fromEntries( + Array.from({ length: PLANNER_TRIP_VIEWS_MAX + 1 }, (_, i) => [String(i + 1), { roadtrip: true }]), + ); + expect(plannerTripViewsSchema.safeParse(many).success).toBe(false); + }); +}); + +describe('compactPlannerTripViews', () => { + it('keeps only the switches that are on and drops trips with none', () => { + expect( + compactPlannerTripViews({ '1': { roadtrip: true, dawarich: false }, '2': { overview: false }, '3': {} }), + ).toEqual({ + '1': { roadtrip: true }, + }); + }); +}); diff --git a/shared/src/settings/settings.schema.ts b/shared/src/settings/settings.schema.ts index 0bd96b1e6..c3676c61e 100644 --- a/shared/src/settings/settings.schema.ts +++ b/shared/src/settings/settings.schema.ts @@ -66,3 +66,43 @@ export type SettingsBulkResponse = z.infer; /** GET/PUT /api/admin/default-user-settings: the stored defaults a new account starts with, by key. */ export const adminDefaultUserSettingsResponseSchema = z.record(z.string(), z.unknown()); export type AdminDefaultUserSettingsResponse = z.infer; + +/** + * The planner's view switches a traveller expects to find again on any device + * (#2639): road trip mode, the recorded Dawarich trail and the whole-trip route + * overview. One settings row per user, keyed by trip id, holding only the + * switches that are on; a trip without any is left out instead of stored as a + * row of falses. The server drops trips the user can no longer open on every + * write, so a deleted trip does not linger in the row. + */ +export const PLANNER_TRIP_VIEWS_KEY = 'planner_trip_views'; +/** More trips than anyone keeps open at once; it bounds the row, not a person. */ +export const PLANNER_TRIP_VIEWS_MAX = 500; + +export const plannerTripViewSchema = z.object({ + roadtrip: z.boolean().optional(), + dawarich: z.boolean().optional(), + overview: z.boolean().optional(), +}); +export type PlannerTripView = z.infer; +export type PlannerTripViewFlag = keyof PlannerTripView; + +export const plannerTripViewsSchema = z + .record(z.string().regex(/^[1-9]\d{0,9}$/), plannerTripViewSchema) + .refine((views) => Object.keys(views).length <= PLANNER_TRIP_VIEWS_MAX, { + message: `At most ${PLANNER_TRIP_VIEWS_MAX} trips`, + }); +export type PlannerTripViews = z.infer; + +/** The views with every trip that has no switch on left out. */ +export function compactPlannerTripViews(views: PlannerTripViews): PlannerTripViews { + const kept: PlannerTripViews = {}; + for (const [tripId, view] of Object.entries(views)) { + const on: PlannerTripView = {}; + if (view.roadtrip) on.roadtrip = true; + if (view.dawarich) on.dawarich = true; + if (view.overview) on.overview = true; + if (Object.keys(on).length > 0) kept[tripId] = on; + } + return kept; +}