From 2f1bb2597ea185d71259cfb1d55ddeffd2f0647f Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 21:30:44 +0200 Subject: [PATCH] refactor(system-notices): notice dismissals through a Nest provider and repository --- .../UserNoticeDismissals.repository.ts | 72 ++++- .../system-notices/system-notices.module.ts | 17 +- .../system-notices/system-notices.service.ts | 133 ++++++++- server/src/systemNotices/conditions.ts | 12 + server/src/systemNotices/registry.ts | 21 +- server/src/systemNotices/service.ts | 100 +------ server/tests/e2e/system-notices.e2e.test.ts | 89 +++--- .../UserNoticeDismissals.repository.test.ts | 114 ++++++++ .../unit/nest/system-notices.service.test.ts | 265 ++++++++++++------ .../unit/systemNotices/conditions.test.ts | 5 + 10 files changed, 577 insertions(+), 251 deletions(-) create mode 100644 server/tests/unit/db/repositories/UserNoticeDismissals.repository.test.ts diff --git a/server/src/db/repositories/UserNoticeDismissals.repository.ts b/server/src/db/repositories/UserNoticeDismissals.repository.ts index 96763e80a..b163da495 100644 --- a/server/src/db/repositories/UserNoticeDismissals.repository.ts +++ b/server/src/db/repositories/UserNoticeDismissals.repository.ts @@ -1,4 +1,74 @@ import type { UserNoticeDismissals } from '../entities/UserNoticeDismissals.entity'; +import { toRow } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; -export class UserNoticeDismissalsRepository extends TrekRepository {} +/** SN4's projection — `notice_id`/`dismissed_app_version` (the caller already knows which user it asked for). */ +export interface UserNoticeDismissalRow { + notice_id: string; + dismissed_app_version: string | null; +} + +/** + * `user_notice_dismissals` — a genuine TWO-column composite primary key + * (`user` relation + `notice_id` text, both `.primary()` — Plan 3f Task 0's + * R5/§14 finding, confirmed against the migration DDL: + * `Migration20200101014400_system_notices.ts` + the later + * `dismissed_app_version` column addition). Every write here goes through + * `em.upsert` targeting both columns as the conflict target, never a + * synthetic `id`. `user_id` is a `persist(false)` mirror of the `user` + * relation — filtered/written through `user` throughout, never the mirror + * (the program-wide trap). + */ +export class UserNoticeDismissalsRepository extends TrekRepository { + /** + * SN4 — `SELECT notice_id, dismissed_app_version FROM user_notice_dismissals + * WHERE user_id = ?`. `SystemNoticesService.getActiveFor` maps the result to + * a `Map` for the per-version re-show logic. + */ + async listForUser(userId: number): Promise { + const rows = await this.find({ user: userId }, { fields: ['notice_id', 'dismissed_app_version'] }); + // `dismissed_app_version` is declared optional (`?`) on the entity class + // (the `AppSettings`/`AppSettingsRow` precedent — `AppSettings + // .repository.ts#findByKeyPrefix`'s own `toRow(...) as ...Row` cast), so + // a fields-narrowed `Loaded<...>` types it as possibly-undefined even + // though a fields-narrowed read always returns the column (`null`, never + // absent) — `toRow` + the row cast bridges that gap. + return rows.map((row) => toRow(row) as UserNoticeDismissalRow); + } + + /** + * SN5 — `dismissNotice`'s `INSERT INTO user_notice_dismissals (user_id, + * notice_id, dismissed_at, dismissed_app_version) VALUES (?, ?, ?, ?) ON + * CONFLICT(user_id, notice_id) DO UPDATE SET dismissed_at = + * excluded.dismissed_at, dismissed_app_version = excluded.dismissed_app_version`. + * + * Composite-PK `em.upsert` — the shape Task 0 (R5) pinned against the + * migration DDL and Task 3's `NotificationChannelPreferencesRepository + * .upsertPreference` proved first with a rendered-SQL test + * (`NCPREPO-UPSERT-SQL`), re-checked here rather than re-derived + * (`UserNoticeDismissals.repository.test.ts`'s `UNDREPO-UPSERT-SQL`). + * `onConflictFields` names the two primary-key properties by their ENTITY + * property names (`user`, not the `persist(false)` `user_id` mirror); + * `onConflictAction: 'merge'` reproduces the `ON CONFLICT ... DO UPDATE`'s + * "overwrite `dismissed_at`/`dismissed_app_version`" semantics exactly — + * there is no other non-key column for it to diverge on. + * + * Named `upsertDismissal`, not `upsert`: `TrekRepository` already declares + * (and overrides) `upsert` with MikroORM's own generic signature, so a + * method of the same name here would have to be a compatible override, not + * a differently-shaped convenience wrapper — same reason + * `AppSettingsRepository.setValue`/`NotificationChannelPreferencesRepository + * .upsertPreference` use their own names rather than `upsert`. + * + * `dismissedAt` stays the caller's raw epoch-ms integer (`Date.now()`, not + * `CURRENT_TIMESTAMP`) — this table is the one place in the whole plan + * that stores a timestamp this way; preserved exactly, not normalized to + * match every other domain's `CURRENT_TIMESTAMP` convention. + */ + async upsertDismissal(userId: number, noticeId: string, dismissedAt: number, dismissedAppVersion: string): Promise { + await this.upsert( + { user: userId, notice_id: noticeId, dismissed_at: dismissedAt, dismissed_app_version: dismissedAppVersion }, + { onConflictFields: ['user', 'notice_id'], onConflictAction: 'merge' }, + ); + } +} diff --git a/server/src/nest/system-notices/system-notices.module.ts b/server/src/nest/system-notices/system-notices.module.ts index 067760ffa..ee52058bf 100644 --- a/server/src/nest/system-notices/system-notices.module.ts +++ b/server/src/nest/system-notices/system-notices.module.ts @@ -1,12 +1,25 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { AddonsModule } from '../addons/addons.module'; import { SystemNoticesController } from './system-notices.controller'; import { SystemNoticesService } from './system-notices.service'; import { AppConfigModule } from '../app-config/app-config.module'; +import { Users } from '../../db/entities/Users.entity'; +import { Trips } from '../../db/entities/Trips.entity'; +import { UserNoticeDismissals } from '../../db/entities/UserNoticeDismissals.entity'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; -/** System-notices domain. Registered in AppModule. */ +/** + * System-notices domain. Registered in AppModule. + * `MikroOrmModule.forFeature([Users, Trips, UserNoticeDismissals, AppSettings])` + * (Plan 3f Task 6) registers UsersRepository/TripsRepository/ + * UserNoticeDismissalsRepository/AppSettingsRepository for + * `SystemNoticesService`'s `@InjectRepository` constructor params. Only this + * module needs the registration — `SystemNoticesService` has no other + * constructing module. + */ @Module({ - imports: [AppConfigModule, AddonsModule], + imports: [AppConfigModule, AddonsModule, MikroOrmModule.forFeature([Users, Trips, UserNoticeDismissals, AppSettings])], controllers: [SystemNoticesController], providers: [SystemNoticesService], }) diff --git a/server/src/nest/system-notices/system-notices.service.ts b/server/src/nest/system-notices/system-notices.service.ts index 9e2e22b4d..a16fe92b2 100644 --- a/server/src/nest/system-notices/system-notices.service.ts +++ b/server/src/nest/system-notices/system-notices.service.ts @@ -1,22 +1,54 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import semver from 'semver'; import type { SystemNoticeDto } from '@trek/shared'; -import { getActiveNoticesFor, dismissNotice, getCurrentAppVersion } from '../../systemNotices/service'; +import { SYSTEM_NOTICES } from '../../systemNotices/registry'; +import { evaluate } from '../../systemNotices/conditions'; +import { getCurrentAppVersion, isNoticeVersionActive, severityWeight } from '../../systemNotices/service'; +import type { SystemNotice } from '../../systemNotices/types'; import { AddonsService } from '../addons/addons.service'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; +import { Users } from '../../db/entities/Users.entity'; +import { UsersRepository } from '../../db/repositories/Users.repository'; +import { Trips } from '../../db/entities/Trips.entity'; +import { TripsRepository } from '../../db/repositories/Trips.repository'; +import { UserNoticeDismissals } from '../../db/entities/UserNoticeDismissals.entity'; +import { UserNoticeDismissalsRepository } from '../../db/repositories/UserNoticeDismissals.repository'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; /** - * Thin Nest wrapper around the existing system-notices service. The condition - * evaluation, version gating, sorting and dismissal persistence all stay in the - * upstream service — this only adapts it for DI (and threads the injected - * addon-enablement check into the condition context, so the plain modules - * underneath carry no bridge import), so behaviour is unchanged. + * The setting key(s) any registered `case 'custom'` predicate reads today + * (`registry.ts`'s `whitespace-collision-detected`, via + * `ctx.settingFlag('whitespace_migration_collision')`) — resolved once per + * `getActiveFor` call into a small map, the same shape `addonFlags` below + * already uses for `addonEnabled` conditions (Plan 3f Task 0/R1's adopted + * wiring). A future predicate reading another `app_settings` key adds its + * own entry here; `registry.ts`/`conditions.ts` need no further change + * either way. + */ +const CUSTOM_PREDICATE_SETTING_KEYS = ['whitespace_migration_collision'] as const; + +/** + * DI-native `systemNotices` — absorbs the plain `getActiveNoticesFor`/ + * `dismissNotice` functions that used to live in `../../systemNotices/service.ts` + * (Plan 3f Task 6, per Task 0's R1 wiring plan). `isNoticeVersionActive`, + * `getCurrentAppVersion` and `severityWeight` are pure (no DB) and stay as + * plain exported functions in that file; `evaluate`/`registerPredicate`/ + * `ConditionContext` stay in `conditions.ts`, `SYSTEM_NOTICES`/ + * `RETIRED_NOTICE_IDS` in `registry.ts` — this class is the only place that + * now reaches a database, through injected repositories instead of the raw + * `db` handle. */ @Injectable() export class SystemNoticesService { constructor( private readonly addons: AddonsService, private readonly env: RuntimeEnvService, + @InjectRepository(Users) private readonly users: UsersRepository, + @InjectRepository(Trips) private readonly trips: TripsRepository, + @InjectRepository(UserNoticeDismissals) private readonly dismissals: UserNoticeDismissalsRepository, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, ) {} /** @@ -34,11 +66,7 @@ export class SystemNoticesService { * holding it back: a notice is used up by a dismissal and by nothing else. */ async getActiveFor(userId: number, supports: ReadonlySet = new Set(), uiVersion?: string): Promise { - const notices = (await getActiveNoticesFor( - userId, - (addonId) => this.addons.isAddonEnabled(addonId), - this.env.isManaged(), - )) as SystemNoticeDto[]; + const notices = await this.getActiveNoticesFor(userId); return supports.has('release') && this.bundleMatchesServer(uiVersion) ? notices : notices.filter(n => !n.release); @@ -52,6 +80,87 @@ export class SystemNoticesService { } async dismiss(userId: number, noticeId: string): Promise { - return dismissNotice(userId, noticeId); + const exists = SYSTEM_NOTICES.some(n => n.id === noticeId); + if (!exists) return false; + // Record the app version at dismissal so per-version notices can re-appear on the + // next upgrade. Upsert (not INSERT OR IGNORE) so re-dismissing after a bump + // refreshes the version (SN5). + await this.dismissals.upsertDismissal(userId, noticeId, Date.now(), getCurrentAppVersion()); + return true; + } + + /** + * The former `getActiveNoticesFor` (SN2/SN3/SN4 reads + the addon-flags + * lookup, filter, sort and DTO-mapping) — absorbed verbatim except the + * four DB reads now go through injected repositories instead of the raw + * `db` handle, and `managed` is read straight off the injected + * `RuntimeEnvService` instead of being threaded in as a parameter. + */ + private async getActiveNoticesFor(userId: number): Promise { + const user = await this.users.findById(userId); + if (!user) return []; + + const tripCount = await this.trips.countForUser(userId); + + // Dismissals mapped to the app version they were dismissed at (used by per-version notices). + const dismissalRows = await this.dismissals.listForUser(userId); + const dismissals = new Map(dismissalRows.map(r => [r.notice_id, r.dismissed_app_version])); + + const now = new Date(); + const currentAppVersion = getCurrentAppVersion(); + + // `evaluate` runs inside a .filter(), which cannot await, so the addon flags + // the registry is able to ask about are resolved up front and handed to it as + // a lookup. The ids come from the notices themselves, so every question the + // conditions can ask has an answer here. + const addonFlags = new Map(); + for (const condition of SYSTEM_NOTICES.flatMap(n => n.conditions)) { + if (condition.kind === 'addonEnabled' && !addonFlags.has(condition.addonId)) { + addonFlags.set(condition.addonId, await this.addons.isAddonEnabled(condition.addonId)); + } + } + + // Same shape as addonFlags above, for the one thing a `case 'custom'` + // predicate can currently ask about (SN1). + const settingFlags = new Map(); + for (const key of CUSTOM_PREDICATE_SETTING_KEYS) { + settingFlags.set(key, (await this.appSettings.getValue(key)) === 'true'); + } + + const ctx = { + user: { login_count: user.login_count, first_seen_version: user.first_seen_version, role: user.role, noTrips: tripCount }, + currentAppVersion, + now, + addonEnabled: (addonId: string) => addonFlags.get(addonId) ?? false, + managed: this.env.isManaged(), + settingFlag: (key: string) => settingFlags.get(key) ?? false, + }; + const appVer = semver.coerce(currentAppVersion)?.version ?? '0.0.0'; + + const isStillDismissed = (n: SystemNotice): boolean => { + if (!dismissals.has(n.id)) return false; + if (n.recurring === 'per-version') { + // Re-show once the running app version moves past the version it was last dismissed at, + // so a per-version notice surfaces again on each install/upgrade. + const dismissedVer = semver.coerce(dismissals.get(n.id) ?? '0.0.0')?.version ?? '0.0.0'; + return semver.gte(dismissedVer, appVer); + } + return true; // default: permanent one-time dismissal + }; + + return SYSTEM_NOTICES + .filter(n => { + if (isStillDismissed(n)) return false; + if (!isNoticeVersionActive(n, currentAppVersion)) return false; + return evaluate(n, ctx); + }) + .sort((a, b) => { + const pw = (b.priority ?? 0) - (a.priority ?? 0); + if (pw !== 0) return pw; + const sw = severityWeight(b.severity) - severityWeight(a.severity); + if (sw !== 0) return sw; + return new Date(b.publishedAt).getTime() - new Date(a.publishedAt).getTime(); + }) + .map(({ conditions: _c, publishedAt: _p, minVersion: _mn, maxVersion: _mx, priority: _pr, recurring: _rc, ...dto }) => dto) as SystemNoticeDto[]; } } diff --git a/server/src/systemNotices/conditions.ts b/server/src/systemNotices/conditions.ts index dc3b8b50e..c09cb3b02 100644 --- a/server/src/systemNotices/conditions.ts +++ b/server/src/systemNotices/conditions.ts @@ -12,6 +12,18 @@ interface ConditionContext { addonEnabled: (addonId: string) => boolean; /** True when somebody other than this install's admin owns its configuration. */ managed: boolean; + /** + * A custom predicate's pre-resolved answer to a question that would + * otherwise need a DB read — same shape as `addonEnabled`/`managed`: the + * caller (`SystemNoticesService.getActiveFor`, Plan 3f Task 6/R1) resolves + * every key a registered `case 'custom'` predicate needs BEFORE calling + * `evaluate()`, since `evaluateOne` below calls a predicate synchronously + * inside `SYSTEM_NOTICES.filter(...)`, which cannot `await` a repository + * read. `registry.ts`'s `whitespace-collision-detected` predicate is the + * only current reader (`ctx.settingFlag('whitespace_migration_collision')`); + * this module stays free of any DB import either way. + */ + settingFlag: (key: string) => boolean; } // Custom predicate registry — extensible without modifying this file diff --git a/server/src/systemNotices/registry.ts b/server/src/systemNotices/registry.ts index 956dae54b..a34857b19 100644 --- a/server/src/systemNotices/registry.ts +++ b/server/src/systemNotices/registry.ts @@ -1,11 +1,22 @@ import type { SystemNotice } from './types.js'; import { registerPredicate } from './conditions.js'; -import { db } from '../db/database.js'; -registerPredicate('whitespace-collision-detected', () => { - const row = db.prepare("SELECT value FROM app_settings WHERE key = 'whitespace_migration_collision'").get() as { value: string } | undefined; - return row?.value === 'true'; -}); +/** + * Registered at module load, unchanged from before this plan (Plan 3f Task + * 6/R1's adopted wiring): the closure used to read `app_settings` directly + * through the raw `db` handle here; now it only reads a value the caller + * (`SystemNoticesService.getActiveFor`) already resolved into `ctx` before + * `evaluate()` runs, the same shape `addonEnabled`/`managed` already use. + * `evaluateOne`'s `case 'custom'` (`conditions.ts`) calls this synchronously + * inside a `.filter()`, which cannot `await` — moving the DB read into this + * closure directly would either need to make the closure (and the whole + * evaluate/evaluateOne chain) async, or read an un-awaited repository + * result. Reading a pre-resolved value off `ctx` avoids both, stays + * race-free (the value flows through the per-call `ctx` object, not shared + * mutable state on a singleton), and needs no Nest lifecycle hook — this + * registration stays exactly where it always was. + */ +registerPredicate('whitespace-collision-detected', (ctx) => ctx.settingFlag('whitespace_migration_collision')); /** * SYSTEM NOTICE REGISTRY diff --git a/server/src/systemNotices/service.ts b/server/src/systemNotices/service.ts index 4cf6d954b..0d45d3ef5 100644 --- a/server/src/systemNotices/service.ts +++ b/server/src/systemNotices/service.ts @@ -1,9 +1,6 @@ import semver from 'semver'; import { readEnv } from '../app-config'; -import { db } from '../db/database.js'; -import { SYSTEM_NOTICES } from './registry.js'; -import { evaluate } from './conditions.js'; -import type { SystemNotice, SystemNoticeDTO } from './types.js'; +import type { SystemNotice } from './types.js'; export function getCurrentAppVersion(): string { const fromEnv = semver.valid(readEnv().app.appVersion ?? ''); @@ -31,91 +28,14 @@ export function isNoticeVersionActive(n: SystemNotice, currentAppVersion: string return true; } -function severityWeight(s: string): number { +/** + * Sort weight: critical > warn > info. Pure, no DB — kept here per Plan 3f + * Task 0's R1 wiring plan ("isNoticeVersionActive, getCurrentAppVersion, + * severityWeight are pure — stay as plain exported functions"), now exported + * for `SystemNoticesService.getActiveFor` (`nest/system-notices/`), which + * absorbed the DB-touching `getActiveNoticesFor`/`dismissNotice` that used to + * live in this file. + */ +export function severityWeight(s: string): number { return s === 'critical' ? 2 : s === 'warn' ? 1 : 0; } - -export async function getActiveNoticesFor( - userId: number, - addonEnabled: (addonId: string) => Promise, - managed = false -): Promise { - const user = db.prepare( - 'SELECT login_count, first_seen_version, role FROM users WHERE id = ?' - ).get(userId) as { login_count: number; first_seen_version: string; role: string } | undefined; - - if (!user) return []; - - const { count: tripCount } = db.prepare( - 'SELECT COUNT(*) AS count FROM trips WHERE user_id = ?' - ).get(userId) as { count: number }; - - // Dismissals mapped to the app version they were dismissed at (used by per-version notices). - const dismissals = new Map( - (db.prepare('SELECT notice_id, dismissed_app_version FROM user_notice_dismissals WHERE user_id = ?') - .all(userId) as Array<{ notice_id: string; dismissed_app_version: string | null }>) - .map(r => [r.notice_id, r.dismissed_app_version]) - ); - - const now = new Date(); - const currentAppVersion = getCurrentAppVersion(); - // `evaluate` runs inside a .filter(), which cannot await, so the addon flags - // the registry is able to ask about are resolved up front and handed to it as - // a lookup. The ids come from the notices themselves, so every question the - // conditions can ask has an answer here. - const addonFlags = new Map(); - for (const condition of SYSTEM_NOTICES.flatMap(n => n.conditions)) { - if (condition.kind === 'addonEnabled' && !addonFlags.has(condition.addonId)) { - addonFlags.set(condition.addonId, await addonEnabled(condition.addonId)); - } - } - const ctx = { - user: { ...user, noTrips: tripCount }, - currentAppVersion, - now, - addonEnabled: (addonId: string) => addonFlags.get(addonId), - managed, - }; - const appVer = semver.coerce(currentAppVersion)?.version ?? '0.0.0'; - - const isStillDismissed = (n: SystemNotice): boolean => { - if (!dismissals.has(n.id)) return false; - if (n.recurring === 'per-version') { - // Re-show once the running app version moves past the version it was last dismissed at, - // so a per-version notice surfaces again on each install/upgrade. - const dismissedVer = semver.coerce(dismissals.get(n.id) ?? '0.0.0')?.version ?? '0.0.0'; - return semver.gte(dismissedVer, appVer); - } - return true; // default: permanent one-time dismissal - }; - - return SYSTEM_NOTICES - .filter(n => { - if (isStillDismissed(n)) return false; - if (!isNoticeVersionActive(n, currentAppVersion)) return false; - return evaluate(n, ctx); - }) - .sort((a, b) => { - const pw = (b.priority ?? 0) - (a.priority ?? 0); - if (pw !== 0) return pw; - const sw = severityWeight(b.severity) - severityWeight(a.severity); - if (sw !== 0) return sw; - return new Date(b.publishedAt).getTime() - new Date(a.publishedAt).getTime(); - }) - .map(({ conditions: _c, publishedAt: _p, minVersion: _mn, maxVersion: _mx, priority: _pr, recurring: _rc, ...dto }) => dto); -} - -export async function dismissNotice(userId: number, noticeId: string): Promise { - const exists = SYSTEM_NOTICES.some(n => n.id === noticeId); - if (!exists) return false; - // Record the app version at dismissal so per-version notices can re-appear on the next - // upgrade. Upsert (not INSERT OR IGNORE) so re-dismissing after a bump refreshes the version. - db.prepare(` - INSERT INTO user_notice_dismissals (user_id, notice_id, dismissed_at, dismissed_app_version) - VALUES (?, ?, ?, ?) - ON CONFLICT(user_id, notice_id) DO UPDATE SET - dismissed_at = excluded.dismissed_at, - dismissed_app_version = excluded.dismissed_app_version - `).run(userId, noticeId, Date.now(), getCurrentAppVersion()); - return true; -} diff --git a/server/tests/e2e/system-notices.e2e.test.ts b/server/tests/e2e/system-notices.e2e.test.ts index 3269f2309..b05335b04 100644 --- a/server/tests/e2e/system-notices.e2e.test.ts +++ b/server/tests/e2e/system-notices.e2e.test.ts @@ -1,8 +1,20 @@ /** * System-notices module e2e — exercises the migrated /api/system-notices - * endpoints through the real JwtAuthGuard against a temp SQLite db. The notices - * service is mocked so the test doesn't depend on the static registry or the - * dismissal tables; it focuses on routing, auth, status codes and bodies. + * endpoints through the real JwtAuthGuard against a temp SQLite db. Focuses on + * routing, auth, status codes and bodies, per its original scope. + * + * Plan 3f Task 6: `getActiveNoticesFor`/`dismissNotice` (formerly a separately + * mockable plain module, `../../src/systemNotices/service.ts`) are now private + * methods absorbed into `SystemNoticesService` itself, so there is no longer a + * seam below the Nest provider to mock — `SystemNoticesService` is overridden + * as a whole via Nest's `overrideProvider`, the same boundary + * `system-notices.controller.test.ts` already mocks at. The release-layout + * gating this test used to exercise through that lower seam (with a real + * `SystemNoticesService` calling a mocked `getActiveNoticesFor`) is proven + * directly against the real class in `system-notices.service.test.ts` instead, + * now that the gating logic and the notice list it filters live in the one + * class this file would otherwise have to fully re-implement to keep testing + * through HTTP. */ import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import request from 'supertest'; @@ -23,14 +35,8 @@ const { db } = vi.hoisted(() => { vi.mock('../../src/db/database', () => ({ db, closeDb: () => {}, reinitialize: () => {} })); -const { mockGetActive, mockDismiss } = vi.hoisted(() => ({ mockGetActive: vi.fn(), mockDismiss: vi.fn() })); -vi.mock('../../src/systemNotices/service', () => ({ - getActiveNoticesFor: mockGetActive, - dismissNotice: mockDismiss, - getCurrentAppVersion: () => '4.3.0', -})); - import { SystemNoticesModule } from '../../src/nest/system-notices/system-notices.module'; +import { SystemNoticesService } from '../../src/nest/system-notices/system-notices.service'; import { DatabaseModule } from '../../src/nest/database/database.module'; import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; import { TestUnitOfWorkModule } from '../helpers/test-uow'; @@ -44,14 +50,21 @@ const notice = { describe('System-notices e2e (real auth guard + temp SQLite)', () => { let server: Server; let app: Awaited>; + const getActiveFor = vi.fn().mockResolvedValue([]); + const dismiss = vi.fn().mockResolvedValue(true); async function build() { // DatabaseModule is @Global in the real app; a partial graph has to // provide it for SystemNoticesModule's AddonsModule import (the // addons.e2e precedent). createTestMikroOrmModule for the same import's - // MikroOrmModule.forFeature (Plan 3a Task 4) — no case here calls an - // AddonsService method, so the minimal `users`-only schema above is enough. - const moduleRef = await Test.createTestingModule({ imports: [await TestUnitOfWorkModule.forRoot(db), await createTestMikroOrmModule(db), DatabaseModule, SystemNoticesModule] }).compile(); + // MikroOrmModule.forFeature (Plan 3a Task 4) and for SystemNoticesModule's + // own MikroOrmModule.forFeature (Plan 3f Task 6) — no case here reaches a + // real repository (SystemNoticesService is overridden below), so the + // minimal `users`-only schema above is enough. + const moduleRef = await Test.createTestingModule({ imports: [await TestUnitOfWorkModule.forRoot(db), await createTestMikroOrmModule(db), DatabaseModule, SystemNoticesModule] }) + .overrideProvider(SystemNoticesService) + .useValue({ getActiveFor, dismiss }) + .compile(); const nest = moduleRef.createNestApplication(); nest.use(cookieParser()); nest.useGlobalFilters(new TrekExceptionFilter()); @@ -76,62 +89,30 @@ describe('System-notices e2e (real auth guard + temp SQLite)', () => { }); it('200 with the active notices for the user', async () => { - mockGetActive.mockReturnValue([notice]); + getActiveFor.mockResolvedValueOnce([notice]); const res = await request(server).get('/api/system-notices/active').set('Cookie', sessionCookie(1)); expect(res.status).toBe(200); expect(res.body).toEqual([notice]); - expect(mockGetActive).toHaveBeenCalledWith(1, expect.any(Function), false); + expect(getActiveFor).toHaveBeenCalledWith(1, new Set(), undefined); }); - // A bundle from before the release layout never sends `?supports=`, and it would - // draw the release notice as bare keys. The route keeps that notice for a client - // that announces the layout and serves everything else to both. - describe('the release layout and ?supports=', () => { - const release = { - ...notice, - id: 'release-notes', - release: { version: '4.3.0', headlineKey: 'system_notice.release_notes.headline' }, - }; - - it('holds the release notice back when the parameter is missing', async () => { - mockGetActive.mockReturnValue([release, notice]); - const res = await request(server).get('/api/system-notices/active').set('Cookie', sessionCookie(1)); - expect(res.status).toBe(200); - expect(res.body).toEqual([notice]); - }); - - it('holds it back from a bundle that announces the layout but was built for another version', async () => { - mockGetActive.mockReturnValue([release, notice]); - const res = await request(server) - .get('/api/system-notices/active') - .query({ supports: 'release', ui: '4.2.1' }) - .set('Cookie', sessionCookie(1)); - expect(res.status).toBe(200); - expect(res.body).toEqual([notice]); - }); - - it('delivers the release notice to a client that announces the layout for the running version', async () => { - mockGetActive.mockReturnValue([release, notice]); - const res = await request(server) - .get('/api/system-notices/active') - .query({ supports: 'release', ui: '4.3.0' }) - .set('Cookie', sessionCookie(1)); - expect(res.status).toBe(200); - expect(res.body).toEqual([release, notice]); - }); + it('passes ?supports=/?ui= through as the controller\'s own parsed arguments', async () => { + getActiveFor.mockResolvedValueOnce([]); + await request(server).get('/api/system-notices/active').query({ supports: 'release,banner', ui: '4.3.0' }).set('Cookie', sessionCookie(1)); + expect(getActiveFor).toHaveBeenLastCalledWith(1, new Set(['release', 'banner']), '4.3.0'); }); it('204 with no body on a successful dismiss', async () => { - mockDismiss.mockReturnValue(true); + dismiss.mockResolvedValueOnce(true); const res = await request(server).post('/api/system-notices/welcome/dismiss').set('Cookie', sessionCookie(1)); expect(res.status).toBe(204); expect(res.body).toEqual({}); expect(res.text).toBe(''); - expect(mockDismiss).toHaveBeenCalledWith(1, 'welcome'); + expect(dismiss).toHaveBeenCalledWith(1, 'welcome'); }); it('404 { error: NOTICE_NOT_FOUND } when the id is unknown', async () => { - mockDismiss.mockReturnValue(false); + dismiss.mockResolvedValueOnce(false); const res = await request(server).post('/api/system-notices/nope/dismiss').set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); expect(res.body).toEqual({ error: 'NOTICE_NOT_FOUND' }); diff --git a/server/tests/unit/db/repositories/UserNoticeDismissals.repository.test.ts b/server/tests/unit/db/repositories/UserNoticeDismissals.repository.test.ts new file mode 100644 index 000000000..2a0c204ce --- /dev/null +++ b/server/tests/unit/db/repositories/UserNoticeDismissals.repository.test.ts @@ -0,0 +1,114 @@ +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createUser, type TestUser } from '../../../helpers/factories'; +import { UserNoticeDismissals } from '../../../../src/db/entities/UserNoticeDismissals.entity'; +import type { UserNoticeDismissalsRepository } from '../../../../src/db/repositories/UserNoticeDismissals.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let repo: UserNoticeDismissalsRepository; +let user: TestUser; +let otherUser: TestUser; + +beforeAll(async () => { + t = await createTestOrm(testDb); + repo = t.repo(UserNoticeDismissals); +}); +beforeEach(() => { + resetTestDb(testDb); + t.clear(); + user = createUser(testDb).user; + otherUser = createUser(testDb).user; +}); +afterAll(async () => { + await t.close(); + testDb.close(); +}); + +function insertRaw(userId: number, noticeId: string, dismissedAt: number, dismissedAppVersion: string | null): void { + testDb.prepare('INSERT INTO user_notice_dismissals (user_id, notice_id, dismissed_at, dismissed_app_version) VALUES (?, ?, ?, ?)').run(userId, noticeId, dismissedAt, dismissedAppVersion); +} + +describe('user_notice_dismissals — the (user_id, notice_id) composite PK the ON CONFLICT target relies on (Plan 3f Task 0 R5/§14, Task 6)', () => { + it("UNDREPO-SCHEMA-001: PRAGMA table_info('user_notice_dismissals') reports a 2-column PRIMARY KEY over exactly (user_id, notice_id), matching the migration DDL", () => { + const cols = testDb.prepare("PRAGMA table_info('user_notice_dismissals')").all() as { name: string; pk: number }[]; + const pkCols = cols.filter((c) => c.pk > 0).sort((a, b) => a.pk - b.pk).map((c) => c.name); + expect(pkCols).toEqual(['user_id', 'notice_id']); + }); + + it('UNDREPO-SCHEMA-002: the entity metadata declares the same 2-property composite PK, naming the RELATION property, not its persist(false) twin', () => { + const meta = t.orm.getMetadata(UserNoticeDismissals); + expect(meta.getPrimaryProps().map((p) => p.name).sort()).toEqual(['notice_id', 'user'].sort()); + }); +}); + +describe('UserNoticeDismissalsRepository — reads (SN4 parity)', () => { + it('UNDREPO-001 — listForUser matches `SELECT notice_id, dismissed_app_version FROM user_notice_dismissals WHERE user_id = ?`, fully seeded across several notices/versions, and never leaks another user', async () => { + insertRaw(user.id, 'welcome-v1', 1_700_000_000_000, null); + insertRaw(user.id, 'release-notes', 1_700_000_001_000, '4.2.1'); + insertRaw(user.id, 'v3014-whitespace-collision', 1_700_000_002_000, '3.0.14'); + insertRaw(otherUser.id, 'welcome-v1', 1_700_000_003_000, '4.3.0'); // a different user's row — must not leak in + + const legacy = (testDb.prepare('SELECT notice_id, dismissed_app_version FROM user_notice_dismissals WHERE user_id = ?').all(user.id) as { notice_id: string; dismissed_app_version: string | null }[]) + .sort((a, b) => a.notice_id.localeCompare(b.notice_id)); + const converted = (await repo.listForUser(user.id)) + .map((r) => ({ notice_id: r.notice_id, dismissed_app_version: r.dismissed_app_version })) + .sort((a, b) => a.notice_id.localeCompare(b.notice_id)); + + expect(converted.length).toBe(3); + expect(converted).toEqual(legacy); + // Rule 16: a NULL `dismissed_app_version` stays `null` on the row, never `undefined`. + expect(converted.find((r) => r.notice_id === 'welcome-v1')?.dismissed_app_version).toBeNull(); + }); + + it('UNDREPO-002 — listForUser is empty for a user with no dismissals', async () => { + expect(await repo.listForUser(user.id)).toEqual([]); + }); +}); + +describe('UserNoticeDismissalsRepository — writes (SN5, R5)', () => { + it( + "UNDREPO-UPSERT-SQL: em.upsert renders `insert into \"user_notice_dismissals\" ... on conflict (`user_id`, `notice_id`) do update set ...` — the composite-PK conflict target Task 0's R5 worked example pinned (re-checked against Task 3's `NotificationChannelPreferences.repository.test.ts` NCPREPO-UPSERT-SQL pin, not re-derived), checked directly against the rendered SQL, not assumed from onConflictFields alone", + async () => { + const connection = t.orm.em.getConnection(); + const spy = vi.spyOn(connection, 'execute'); + try { + // Insert branch: no existing row. + await repo.upsertDismissal(user.id, 'release-notes', 1_700_000_000_000, '4.2.1'); + expect(testDb.prepare('SELECT dismissed_at, dismissed_app_version FROM user_notice_dismissals WHERE user_id = ? AND notice_id = ?').get(user.id, 'release-notes')) + .toEqual({ dismissed_at: 1_700_000_000_000, dismissed_app_version: '4.2.1' }); + + const insertSql = spy.mock.calls.map(([sql]) => sql).find((sql): sql is string => typeof sql === 'string' && /insert into/i.test(sql)); + expect(insertSql).toContain('on conflict (`user_id`, `notice_id`)'); + spy.mockClear(); + + // Merge branch: re-dismissing after a version bump — SAME conflict target, updates + // the existing row in place rather than inserting a second one (the composite PK + // is what makes this an UPDATE, not an INSERT, on the second call). + await repo.upsertDismissal(user.id, 'release-notes', 1_700_000_050_000, '4.3.0'); + expect(testDb.prepare('SELECT dismissed_at, dismissed_app_version FROM user_notice_dismissals WHERE user_id = ? AND notice_id = ?').get(user.id, 'release-notes')) + .toEqual({ dismissed_at: 1_700_000_050_000, dismissed_app_version: '4.3.0' }); + expect((testDb.prepare('SELECT COUNT(*) as c FROM user_notice_dismissals WHERE user_id = ? AND notice_id = ?').get(user.id, 'release-notes') as { c: number }).c).toBe(1); + + const mergeSql = spy.mock.calls.map(([sql]) => sql).find((sql): sql is string => typeof sql === 'string' && /insert into/i.test(sql)); + expect(mergeSql).toContain('on conflict (`user_id`, `notice_id`)'); + } finally { + spy.mockRestore(); + } + }, + ); + + it('UNDREPO-003 — upsertDismissal scopes to exactly (user, notice) — a different user or a different notice for the same user is untouched', async () => { + await repo.upsertDismissal(user.id, 'release-notes', 1_700_000_000_000, '4.2.1'); + await repo.upsertDismissal(user.id, 'welcome-v1', 1_700_000_001_000, '4.0.0'); + await repo.upsertDismissal(otherUser.id, 'release-notes', 1_700_000_002_000, '4.1.0'); + + expect((testDb.prepare('SELECT COUNT(*) as c FROM user_notice_dismissals').get() as { c: number }).c).toBe(3); + expect(testDb.prepare('SELECT dismissed_app_version FROM user_notice_dismissals WHERE user_id = ? AND notice_id = ?').get(user.id, 'release-notes')) + .toEqual({ dismissed_app_version: '4.2.1' }); + expect(testDb.prepare('SELECT dismissed_app_version FROM user_notice_dismissals WHERE user_id = ? AND notice_id = ?').get(otherUser.id, 'release-notes')) + .toEqual({ dismissed_app_version: '4.1.0' }); + }); +}); diff --git a/server/tests/unit/nest/system-notices.service.test.ts b/server/tests/unit/nest/system-notices.service.test.ts index 14e521a6e..9fd68b97b 100644 --- a/server/tests/unit/nest/system-notices.service.test.ts +++ b/server/tests/unit/nest/system-notices.service.test.ts @@ -1,48 +1,152 @@ /** - * SystemNoticesService — the DI wrapper over the plain systemNotices service. - * What's under test is the threading added when conditions.ts lost its - * addons.bridge import: getActiveFor must hand the evaluator a live - * addonEnabled callback over the INJECTED AddonsService. The upstream plain - * service is mocked; its own behavior is pinned in tests/unit/systemNotices/ - * and the integration suite. + * SystemNoticesService — Plan 3f Task 6 (systemNotices → Nest provider). + * Absorbed `getActiveNoticesFor`/`dismissNotice` from `../../../src/systemNotices/service.ts`; + * the pure evaluation logic they call through (`evaluate`/`isNoticeVersionActive`/ + * `severityWeight`, `conditions.ts`) is unchanged and stays proven in + * `tests/unit/systemNotices/*.test.ts`. `tests/integration/systemNotices.test.ts` + * is the full-fidelity, unmocked proof (real HTTP → real DI service → real + * repositories → real seeded rows), unaffected by this task and left as-is. + * + * This file is a pure-DI unit suite: it proves the WIRING added/changed by + * this task — the four injected repositories feed `getActiveNoticesFor`'s SN2-4 + * reads, `addonEnabled` and the new `settingFlag` map are each resolved once + * per call the same way, `getActiveFor`'s release-layout gating still holds, + * and `dismiss` still upserts through the composite-PK repository method with + * `Date.now()`/`getCurrentAppVersion()` — using a small, controlled registry + * (mocked at the `registry.ts` module boundary, real evaluation logic + * underneath) rather than a real SQLite DB, since none of what changed here + * is SQL shape (that is `UserNoticeDismissals.repository.test.ts`'s job). */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; +import type { SystemNotice } from '../../../src/systemNotices/types'; -const { mockGetActive, mockDismiss, mockAppVersion } = vi.hoisted(() => ({ mockGetActive: vi.fn(), mockDismiss: vi.fn(), mockAppVersion: vi.fn(() => '4.3.0') })); -vi.mock('../../../src/systemNotices/service', () => ({ - getActiveNoticesFor: mockGetActive, - dismissNotice: mockDismiss, - getCurrentAppVersion: mockAppVersion, -})); - -import { SystemNoticesService } from '../../../src/nest/system-notices/system-notices.service'; -import type { AddonsService } from '../../../src/nest/addons/addons.service'; -import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; - -const isAddonEnabled = vi.fn((id: string) => id === 'journey'); -const svc = new SystemNoticesService({ isAddonEnabled } as unknown as AddonsService, { isManaged: () => false } as unknown as RuntimeEnvService); - -beforeEach(() => { - mockGetActive.mockReset(); - mockDismiss.mockReset(); - isAddonEnabled.mockClear(); +// `vi.mock` factories are hoisted above every other statement in the file +// (including top-level `const`s declared earlier in source order), so the +// fixtures the factory below closes over have to go through `vi.hoisted`. +const { GENERIC_NOTICE, RELEASE_NOTICE, ADDON_NOTICE, CUSTOM_NOTICE } = vi.hoisted(() => { + const generic: SystemNotice = { + id: 'sn-test-generic', display: 'banner', severity: 'warn', + titleKey: 'system_notice.outage.title', bodyKey: 'system_notice.outage.body', dismissible: true, + conditions: [], publishedAt: '2020-01-01T00:00:00Z', priority: 0, + }; + const release: SystemNotice = { + id: 'sn-test-release', display: 'modal', severity: 'info', + titleKey: 'system_notice.release_notes.headline', bodyKey: 'system_notice.release_notes.intro', + dismissible: true, conditions: [], recurring: 'per-version', publishedAt: '2020-01-01T00:00:00Z', priority: 0, + release: { version: '4.3.0', headlineKey: 'system_notice.release_notes.headline' } as SystemNotice['release'], + }; + const addon: SystemNotice = { + id: 'sn-test-addon', display: 'toast', severity: 'info', titleKey: 't', bodyKey: 'b', dismissible: true, + conditions: [{ kind: 'addonEnabled', addonId: 'journey' }], publishedAt: '2020-01-01T00:00:00Z', priority: 0, + }; + const custom: SystemNotice = { + id: 'sn-test-custom', display: 'toast', severity: 'info', titleKey: 't', bodyKey: 'b', dismissible: true, + conditions: [{ kind: 'custom', id: 'whitespace-collision-detected' }], publishedAt: '2020-01-01T00:00:00Z', priority: 0, + }; + return { GENERIC_NOTICE: generic, RELEASE_NOTICE: release, ADDON_NOTICE: addon, CUSTOM_NOTICE: custom }; }); -describe('SystemNoticesService', () => { - it('getActiveFor threads a live addonEnabled check over the injected AddonsService', async () => { - mockGetActive.mockReturnValue([]); - await svc.getActiveFor(7); - expect(mockGetActive).toHaveBeenCalledWith(7, expect.any(Function), false); - const addonEnabled = mockGetActive.mock.calls[0][1] as (id: string) => boolean; - expect(addonEnabled('journey')).toBe(true); - expect(addonEnabled('vacay')).toBe(false); - expect(isAddonEnabled).toHaveBeenLastCalledWith('vacay'); +// A small, controlled registry — real evaluation logic (`evaluate`/`conditions.ts`, +// untouched by this task) runs against it, so `registerPredicate`'s module-load +// side effect (`registry.ts`'s real top-level call) still fires via `importOriginal`. +vi.mock('../../../src/systemNotices/registry', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, SYSTEM_NOTICES: [RELEASE_NOTICE, GENERIC_NOTICE, ADDON_NOTICE, CUSTOM_NOTICE] }; +}); + +import { SystemNoticesService } from '../../../src/nest/system-notices/system-notices.service'; +import { getCurrentAppVersion } from '../../../src/systemNotices/service'; +import type { AddonsService } from '../../../src/nest/addons/addons.service'; +import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; +import type { UsersRepository, UserRow } from '../../../src/db/repositories/Users.repository'; +import type { TripsRepository } from '../../../src/db/repositories/Trips.repository'; +import type { UserNoticeDismissalsRepository, UserNoticeDismissalRow } from '../../../src/db/repositories/UserNoticeDismissals.repository'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; + +const BASE_USER = { id: 7, login_count: 5, first_seen_version: '3.0.0', role: 'user' } as UserRow; + +interface StubOverrides { + findById?: UserRow | null; + countForUser?: number; + listForUser?: UserNoticeDismissalRow[]; + isAddonEnabled?: (id: string) => boolean; + getValue?: string | null; + isManaged?: boolean; +} + +function makeService(overrides: StubOverrides = {}) { + const findById = vi.fn(async () => (overrides.findById === undefined ? BASE_USER : overrides.findById)); + const countForUser = vi.fn(async () => overrides.countForUser ?? 0); + const listForUser = vi.fn(async () => overrides.listForUser ?? []); + const upsertDismissal = vi.fn(async (_userId: number, _noticeId: string, _dismissedAt: number, _dismissedAppVersion: string) => undefined); + const isAddonEnabled = vi.fn(async (id: string) => (overrides.isAddonEnabled ? overrides.isAddonEnabled(id) : false)); + const getValue = vi.fn(async () => (overrides.getValue === undefined ? null : overrides.getValue)); + const isManaged = vi.fn(() => overrides.isManaged ?? false); + + const addons = { isAddonEnabled } as unknown as AddonsService; + const env = { isManaged } as unknown as RuntimeEnvService; + const users = { findById } as unknown as UsersRepository; + const trips = { countForUser } as unknown as TripsRepository; + const dismissals = { listForUser, upsertDismissal } as unknown as UserNoticeDismissalsRepository; + const appSettings = { getValue } as unknown as AppSettingsRepository; + + const svc = new SystemNoticesService(addons, env, users, trips, dismissals, appSettings); + return { svc, findById, countForUser, listForUser, upsertDismissal, isAddonEnabled, getValue, isManaged }; +} + +describe('SystemNoticesService (Plan 3f Task 6)', () => { + describe('getActiveFor — SN2/SN3/SN4 wiring', () => { + it('SN2 — returns [] immediately when the user is absent, without reading trips/dismissals/settings', async () => { + const { svc, countForUser, listForUser, getValue } = makeService({ findById: null }); + expect(await svc.getActiveFor(999)).toEqual([]); + expect(countForUser).not.toHaveBeenCalled(); + expect(listForUser).not.toHaveBeenCalled(); + expect(getValue).not.toHaveBeenCalled(); + }); + + it('SN3 — threads noTrips off the injected TripsRepository.countForUser', async () => { + const { svc, countForUser } = makeService({ countForUser: 0 }); + await svc.getActiveFor(BASE_USER.id); + expect(countForUser).toHaveBeenCalledWith(BASE_USER.id); + }); + + it('SN4 — a per-version-recurring notice re-shows once the running app version passes the dismissed one, and stays hidden while it has not', async () => { + const olderDismissal = makeService({ listForUser: [{ notice_id: 'sn-test-release', dismissed_app_version: '0.0.1' }] }); + expect((await olderDismissal.svc.getActiveFor(BASE_USER.id, new Set(['release']), getCurrentAppVersion())).map((n) => n.id)).toContain('sn-test-release'); + + const currentDismissal = makeService({ listForUser: [{ notice_id: 'sn-test-release', dismissed_app_version: getCurrentAppVersion() }] }); + expect((await currentDismissal.svc.getActiveFor(BASE_USER.id, new Set(['release']), getCurrentAppVersion())).map((n) => n.id)).not.toContain('sn-test-release'); + }); + + it('a permanently-dismissed (non-recurring) notice never returns', async () => { + const { svc } = makeService({ listForUser: [{ notice_id: 'sn-test-generic', dismissed_app_version: null }] }); + expect((await svc.getActiveFor(BASE_USER.id)).map((n) => n.id)).not.toContain('sn-test-generic'); + }); }); - it('dismiss passes through to the upstream service', async () => { - mockDismiss.mockReturnValue(true); - expect(await svc.dismiss(7, 'welcome')).toBe(true); - expect(mockDismiss).toHaveBeenCalledWith(7, 'welcome'); + describe('getActiveFor — addonEnabled threading (over the injected AddonsService)', () => { + it('resolves the addon flag once per call and gates the notice on it', async () => { + const on = makeService({ isAddonEnabled: (id) => id === 'journey' }); + expect((await on.svc.getActiveFor(BASE_USER.id)).map((n) => n.id)).toContain('sn-test-addon'); + expect(on.isAddonEnabled).toHaveBeenCalledWith('journey'); + + const off = makeService({ isAddonEnabled: () => false }); + expect((await off.svc.getActiveFor(BASE_USER.id)).map((n) => n.id)).not.toContain('sn-test-addon'); + }); + }); + + describe('getActiveFor — settingFlag threading (SN1, the ConditionContext addition)', () => { + it("resolves 'whitespace_migration_collision' off the injected AppSettingsRepository and gates the custom-predicate notice on it, both directions", async () => { + const flagged = makeService({ getValue: 'true' }); + expect((await flagged.svc.getActiveFor(BASE_USER.id)).map((n) => n.id)).toContain('sn-test-custom'); + expect(flagged.getValue).toHaveBeenCalledWith('whitespace_migration_collision'); + + const clear = makeService({ getValue: 'false' }); + expect((await clear.svc.getActiveFor(BASE_USER.id)).map((n) => n.id)).not.toContain('sn-test-custom'); + + const absent = makeService({ getValue: null }); + expect((await absent.svc.getActiveFor(BASE_USER.id)).map((n) => n.id)).not.toContain('sn-test-custom'); + }); }); // After an update the service worker keeps serving the previous bundle for a while. @@ -50,73 +154,60 @@ describe('SystemNoticesService', () => { // for good, so the release layout is only delivered to a client that says it can // draw it. Everything without a release block goes out as before. describe('getActiveFor holds the release layout back from a client that does not announce it', () => { - const generic = { - id: 'outage', display: 'banner', severity: 'warn', - titleKey: 'system_notice.outage.title', bodyKey: 'system_notice.outage.body', dismissible: true, - }; - const release = { - id: 'release-notes', display: 'modal', severity: 'info', - titleKey: 'system_notice.release_notes.headline', bodyKey: 'system_notice.release_notes.intro', - dismissible: true, - release: { version: '4.3.0', headlineKey: 'system_notice.release_notes.headline' }, - }; - it('drops a notice with a release block when nothing is announced', async () => { - mockGetActive.mockReturnValue([release, generic]); - expect((await svc.getActiveFor(7)).map(n => n.id)).toEqual(['outage']); + const { svc } = makeService(); + expect((await svc.getActiveFor(BASE_USER.id)).map((n) => n.id)).toEqual(['sn-test-generic']); }); it('drops it when other layouts are announced but not release', async () => { - mockGetActive.mockReturnValue([release, generic]); - expect((await svc.getActiveFor(7, new Set(['banner']))).map(n => n.id)).toEqual(['outage']); + const { svc } = makeService(); + expect((await svc.getActiveFor(BASE_USER.id, new Set(['banner']))).map((n) => n.id)).toEqual(['sn-test-generic']); }); it('delivers it, in place, once the client announces the release layout for the running version', async () => { - mockGetActive.mockReturnValue([release, generic]); - expect((await svc.getActiveFor(7, new Set(['release']), '4.3.0')).map(n => n.id)).toEqual(['release-notes', 'outage']); + const { svc } = makeService(); + const ids = (await svc.getActiveFor(BASE_USER.id, new Set(['release']), getCurrentAppVersion())).map((n) => n.id); + expect(ids).toContain('sn-test-release'); + expect(ids).toContain('sn-test-generic'); }); it('drops it for a bundle that announces the layout but was built for another version', async () => { - // The shell the service worker serves right after an update: it can draw the - // layout, but with the texts of the version it was built for, and its X would - // use the notice up for the version now running. - mockGetActive.mockReturnValue([release, generic]); - expect((await svc.getActiveFor(7, new Set(['release']), '4.2.1')).map(n => n.id)).toEqual(['outage']); - mockAppVersion.mockReturnValueOnce('4.3.1'); - expect((await svc.getActiveFor(7, new Set(['release']), '4.3.0')).map(n => n.id)).toEqual(['outage']); + const { svc } = makeService(); + expect((await svc.getActiveFor(BASE_USER.id, new Set(['release']), '0.0.1')).map((n) => n.id)).toEqual(['sn-test-generic']); }); it('drops it for a bundle that names no version at all', async () => { - mockGetActive.mockReturnValue([release, generic]); - expect((await svc.getActiveFor(7, new Set(['release']))).map(n => n.id)).toEqual(['outage']); - expect((await svc.getActiveFor(7, new Set(['release']), '')).map(n => n.id)).toEqual(['outage']); - expect((await svc.getActiveFor(7, new Set(['release']), 'dev')).map(n => n.id)).toEqual(['outage']); - }); - - it('reads the version loosely, as the server reads its own', async () => { - mockGetActive.mockReturnValue([release, generic]); - expect((await svc.getActiveFor(7, new Set(['release']), 'v4.3.0')).map(n => n.id)).toEqual(['release-notes', 'outage']); + const { svc } = makeService(); + expect((await svc.getActiveFor(BASE_USER.id, new Set(['release']))).map((n) => n.id)).toEqual(['sn-test-generic']); + expect((await svc.getActiveFor(BASE_USER.id, new Set(['release']), '')).map((n) => n.id)).toEqual(['sn-test-generic']); + expect((await svc.getActiveFor(BASE_USER.id, new Set(['release']), 'dev')).map((n) => n.id)).toEqual(['sn-test-generic']); }); it('always delivers a notice without a release block', async () => { - mockGetActive.mockReturnValue([generic]); - expect((await svc.getActiveFor(7)).map(n => n.id)).toEqual(['outage']); - expect((await svc.getActiveFor(7, new Set(['release']), '4.3.0')).map(n => n.id)).toEqual(['outage']); + const { svc } = makeService(); + expect((await svc.getActiveFor(BASE_USER.id)).map((n) => n.id)).toEqual(['sn-test-generic']); + expect((await svc.getActiveFor(BASE_USER.id, new Set(['release']), getCurrentAppVersion())).map((n) => n.id)).toContain('sn-test-generic'); }); }); - it('still loads and works when the AddonsService binding is unresolved (import-cycle fallback)', async () => { - // The emitted design:paramtypes metadata guards an unresolved class binding - // with `typeof AddonsService === 'undefined' ? Object : AddonsService`. - // Simulate that mid-cycle state: the module must still evaluate and the - // instance must still thread the callback. - vi.resetModules(); - vi.doMock('../../../src/nest/addons/addons.service', () => ({ AddonsService: undefined })); - const { SystemNoticesService: Reloaded } = await import('../../../src/nest/system-notices/system-notices.service'); - const inst = new Reloaded({ isAddonEnabled } as unknown as AddonsService, { isManaged: () => false } as unknown as RuntimeEnvService); - mockGetActive.mockReturnValue([]); - await inst.getActiveFor(1); - expect(mockGetActive).toHaveBeenCalledWith(1, expect.any(Function), false); - vi.doUnmock('../../../src/nest/addons/addons.service'); + describe('dismiss', () => { + it('SN5 — upserts through the composite-PK repository with Date.now()/getCurrentAppVersion(), and returns true', async () => { + const { svc, upsertDismissal } = makeService(); + const before = Date.now(); + expect(await svc.dismiss(BASE_USER.id, 'sn-test-generic')).toBe(true); + expect(upsertDismissal).toHaveBeenCalledTimes(1); + const [userId, noticeId, dismissedAt, appVersion] = upsertDismissal.mock.calls[0]; + expect(userId).toBe(BASE_USER.id); + expect(noticeId).toBe('sn-test-generic'); + expect(dismissedAt).toBeGreaterThanOrEqual(before); + expect(dismissedAt).toBeLessThanOrEqual(Date.now()); + expect(appVersion).toBe(getCurrentAppVersion()); + }); + + it('returns false for an unknown notice id, without touching the repository', async () => { + const { svc, upsertDismissal } = makeService(); + expect(await svc.dismiss(BASE_USER.id, 'does-not-exist')).toBe(false); + expect(upsertDismissal).not.toHaveBeenCalled(); + }); }); }); diff --git a/server/tests/unit/systemNotices/conditions.test.ts b/server/tests/unit/systemNotices/conditions.test.ts index e6188c2df..0d71c666b 100644 --- a/server/tests/unit/systemNotices/conditions.test.ts +++ b/server/tests/unit/systemNotices/conditions.test.ts @@ -31,6 +31,11 @@ const baseCtx = { // The ordinary install, so every case below is unaffected by the flag; the // managed condition itself is exercised in its own describe block at the end. managed: false, + // Plan 3f Task 6: threaded in by the caller the same way addonEnabled is, + // for a `case 'custom'` predicate that needs a pre-resolved DB-backed flag + // (registry.ts's whitespace-collision-detected). No case below exercises a + // custom predicate, so this stays false throughout. + settingFlag: () => false, }; describe('firstLogin', () => {