mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(planner): road trip mode, the dawarich trail and the route overview follow the account to another browser
The three planner view switches lived in sessionStorage only, so a new tab, another browser or another device opened every trip on the plain view again. They are now kept per trip in one user setting, planner_trip_views, validated against a contract in @trek/shared. The server keeps only switches that are on and only trips the user can still open, so a deleted trip or one the user left drops out on the next write. The tab copy in sessionStorage stays and answers until the settings have loaded. Refs #2639
This commit is contained in:
@@ -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<string, unknown>) {
|
||||
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);
|
||||
|
||||
@@ -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],
|
||||
|
||||
@@ -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<PlannerTripViews | null> {
|
||||
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<unknown> {
|
||||
return key === PLANNER_TRIP_VIEWS_KEY ? await this.keepOpenableTripViews(userId, value) : value;
|
||||
}
|
||||
|
||||
async getAdminUserDefaults(): Promise<Record<string, unknown>> {
|
||||
const rows = await this.appSettings.findByKeyPrefix('default_user_setting_');
|
||||
const defaults: Record<string, unknown> = {};
|
||||
@@ -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<string, unknown>) {
|
||||
@@ -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++;
|
||||
}
|
||||
});
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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('');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user