From f04c1517b56833911fbceb380e1f95760bd71835 Mon Sep 17 00:00:00 2001 From: OpenVelo Agent Date: Wed, 22 Jul 2026 14:27:40 +0000 Subject: [PATCH 1/2] docs: update documentation to OKF v0.1 format --- docs/api/admin.md | 16 ++++---- docs/architecture/key-files.md | 13 ++++--- docs/guides/admin-general-settings.md | 53 +++++++++++++-------------- docs/index.md | 7 ++-- 4 files changed, 46 insertions(+), 43 deletions(-) diff --git a/docs/api/admin.md b/docs/api/admin.md index 93d388c..f4422b6 100644 --- a/docs/api/admin.md +++ b/docs/api/admin.md @@ -1,9 +1,9 @@ --- type: api title: Admin Endpoints -description: Admin-only endpoints for user management, general settings, categories, and supporting uploads. +description: Admin-only endpoints for user management, required general settings, categories, and supporting uploads. tags: [api, admin, users, general, categories] -timestamp: 2026-07-22T12:00:00Z +timestamp: 2026-07-22T14:24:08Z --- # Endpoints @@ -46,11 +46,13 @@ removes the row. * `logo` up to 2048 chars (a public URL — uploaded separately) * `welcomeMarkdown` up to 64 000 chars * `themeKey` is one of `THEME_IDS` -* `eventStartUtc` / `eventEndUtc` are validated as ISO-8601 datetimes - (zod `string().datetime(...)`) — empty strings are explicitly allowed - to keep the event window unconfigured. `superRefine` then enforces - `eventEndUtc > eventStartUtc` and reports the issue on `eventEndUtc` - when both fields are non-empty. +* `eventStartUtc` / `eventEndUtc` must each be non-empty ISO-8601 + datetimes. Empty and malformed strings produce a field-specific + `400 VALIDATION_FAILED` issue (`eventStartUtc must be a valid ISO-8601 + datetime` or `eventEndUtc must be a valid ISO-8601 datetime`). + `superRefine` additionally requires `eventEndUtc > eventStartUtc` and + reports `eventEndUtc must be strictly after eventStartUtc` on the end + field when both values parse but are out of order. * `defaultChallengeIp` 1–255 chars * `registrationsEnabled` boolean diff --git a/docs/architecture/key-files.md b/docs/architecture/key-files.md index 8f7484f..2625452 100644 --- a/docs/architecture/key-files.md +++ b/docs/architecture/key-files.md @@ -1,9 +1,9 @@ --- type: architecture title: Key Files Index -description: One-line responsibility for important source files, including bootstrap payloads, runtime theme application, authenticated event streaming, and validated site-logo uploads. -tags: [architecture, index, key-files] -timestamp: 2026-07-22T13:40:00Z +description: One-line responsibility for important source and contract-test files, including strict event-window validation. +tags: [architecture, key-files, event-window, validation] +timestamp: 2026-07-22T14:24:08Z --- # Backend @@ -25,7 +25,9 @@ timestamp: 2026-07-22T13:40:00Z | `backend/src/modules/auth/auth.controller.ts` | Registers authentication and account endpoints. | | `backend/src/modules/auth/auth.service.ts` | Handles sessions, authentication, registration, and password changes. | | `backend/src/modules/uploads/uploads.controller.ts` | Registers admin-only multipart uploads, including Sharp-backed site-logo format validation. | -| `backend/src/modules/admin/dto/general.dto.ts` | Zod schema for `PUT /api/v1/admin/general/settings` — string-length rules, theme-key enum, ISO-8601 datetime check on the event-window fields (empty string allowed), and the `endAfterStart` super-refine. | +| `backend/src/modules/admin/dto/general.dto.ts` | Zod contract for `PUT /api/v1/admin/general/settings`; both event timestamps are required ISO-8601 values and end must be strictly after start. | +| `tests/backend/admin-general-event-window.spec.ts` | Focused contract tests for valid, empty, malformed, equal, and reversed event-window timestamps. | +| `tests/backend/admin-general-service.spec.ts` | General-settings schema and service tests, including per-field empty datetime failures and settings-event emission. | # Frontend @@ -47,7 +49,8 @@ timestamp: 2026-07-22T13:40:00Z | `frontend/src/app/features/shell/tabs/quick-tabs.component.ts` | Main shell navigation tabs. | | `frontend/src/app/features/shell/change-password/change-password-modal.component.ts` | Change-password form modal. | | `frontend/src/app/features/admin/general.component.ts` | `AdminGeneralComponent` reactive form for `/admin/general` — per-field inline error rendering (page-title + event-start + event-end), logo upload wiring, welcome Markdown preview, event-state derivation, and SSE `general` event handling. | -| `frontend/src/app/features/admin/general.pure.ts` | Pure helpers used by `AdminGeneralComponent` — `deriveEventState`, `endAfterStartValidator`, `normalizePageTitle`, `toIsoUtc`, `toDatetimeLocal`, `pageTitleError` / `pageTitleMessage`, `isoDatetimeValidator`, and `datetimeMessage`. | +| `frontend/src/app/features/admin/general.pure.ts` | Pure General Settings helpers, including required datetime validation, field messages, UTC conversion, and end-after-start validation. | +| `tests/frontend/admin-general-pure.spec.ts` | Pure client-contract tests for required event timestamps, datetime messaging, UTC conversion, and event-window ordering. | | `tests/frontend/authenticated-event-source.spec.ts` | Tests SSE authorization, frame transport behavior, and the `401`/`403` unauthorized path. | | `tests/frontend/auth-session-events.spec.ts` | Pure tests for cross-tab invalidation message encoding, payload validation, and `storage`-event filtering. | diff --git a/docs/guides/admin-general-settings.md b/docs/guides/admin-general-settings.md index 39cdc30..02f081d 100644 --- a/docs/guides/admin-general-settings.md +++ b/docs/guides/admin-general-settings.md @@ -1,9 +1,9 @@ --- type: guide title: Admin — General Settings -description: How an admin edits global platform settings (page title, logo, theme, event window, default challenge IP, registrations, welcome Markdown) from the /admin/general page. +description: How an admin edits required platform-wide settings, including the validated event window, from the /admin/general page. tags: [guide, admin, settings, general, tester, datetime, validation] -timestamp: 2026-07-22T14:07:58Z +timestamp: 2026-07-22T14:24:08Z --- # When this view is available @@ -42,8 +42,8 @@ The page is a single reactive form with these controls (every | Logo (file picker) | `general-logo-file` | `logo` (public URL) | Uploads via `POST /api/v1/uploads/logo`; the returned `publicUrl` is bound to a hidden input `general-logo`. | | Logo upload status | `general-logo-uploading` / `general-logo-error` / `general-logo-current` | — | Inline status text under the file picker. | | Global theme | `general-themeKey` | `themeKey` | `` bindings to surface `aria-invalid`, `aria-describedby`, `title`, and `validity.validationMessage` whenever the form is in the `endBeforeStart` invalid state; ensure the inline `general-endBeforeStart` and `general-eventEnd-error` blocks render as soon as the cross-field validator fires (not only after `touched`); mirror the same logic symmetrically on the Event start side for completeness. + - `frontend/src/app/features/admin/general.pure.ts` — make the inline-error messages for `endBeforeStart` and `invalidDatetime` available as exports if they are not already (they are inline strings today; centralise to keep the template clean). Optionally tighten `isoDatetimeValidator` to keep treating empty as invalid client-side so the UI can also block Save when Start/End are blank after the fix — see "Open Question" below. + - `tests/backend/admin-validation.spec.ts` — add focused negative-case tests for the general-settings endpoint: empty `eventEndUtc` rejected with HTTP 400, malformed string rejected with HTTP 400, end ≤ start rejected with HTTP 400; plus a positive test that confirms valid ISO-8601 values persist unchanged. + - `tests/frontend/admin-general-pure.spec.ts` — add a small `invalidDatetime` regression asserting that `isoDatetimeValidator({ value: '' })` returns `{ invalidDatetime: true }` (if we choose to flip client validation to match backend) — see Open Question. + +- **To Create:** + - `tests/backend/admin-general-event-window.spec.ts` — dedicated spec for the new negative cases; uses the same Nest-app fixture pattern as `tests/backend/admin-general-service.spec.ts` but focuses on PUT validation only. + +## 3. Proposed Changes + +### Backend + +1. **Tighten the zod schema** in `backend/src/modules/admin/dto/general.dto.ts`: + - Replace each `eventStartUtc` / `eventEndUtc` field from `z.union([z.literal(''), z.string().datetime({ message: '...' })])` to `z.string().datetime({ message: '... must be a valid ISO-8601 datetime' })` directly. The empty-string alternative is what allowed `eventEndUtc=''` to overwrite a valid schedule (Job negative case 1). + - Keep the `superRefine` rule that emits `eventEndUtc must be strictly after eventStartUtc` on `path: ['eventEndUtc']` when both values are parseable and `end <= start`. + - The error envelope remains `{ code: 'VALIDATION_FAILED', message: 'Request validation failed', details: [{path, message}] }` (HTTP 400), emitted by `ZodValidationPipe` (`backend/src/common/pipes/zod-validation.pipe.ts:9-15`) — no pipe changes needed. + +2. **Persist only on success:** nothing to change in `AdminGeneralService.updateSettings` itself; because `ZodValidationPipe` throws before the handler is called, the prior schedule is preserved automatically on rejected payloads (the SettingsService is never touched). + +3. **Docs reconciliation (non-blocking):** update `docs/api/admin.md:48-53` and `docs/guides/admin-general-settings.md` sentences that say "empty strings are allowed" so the docs reflect that Start/End are now required (it is OK if the docs lag the change; the Job is authoritative). + +### Frontend + +1. **Update `general.pure.ts`** to keep the validator surface aligned with the new backend: + - If we choose to also reject empty strings client-side (recommended — keeps UI feedback consistent with backend), change `isoDatetimeValidator` so `''` returns `{ invalidDatetime: true }` instead of `null`. If we keep empty-allowed client-side, then `datetimeMessage` must still show a helpful message on `''` so the field stays consistent with the disabled Save button. + - Expose a `endBeforeStartMessage` helper that returns `'Event end must be after event start.'` so the template renders it from one source. + +2. **Update `general.component.ts`** for the Event End input and surrounding @if blocks (lines 109-125): + - Bind `[attr.aria-invalid]` and `[attr.aria-describedby]` on the End `` to a new `showEventEndCrossFieldError` computed that is true whenever the form has `errors.endBeforeStart`. Also bind `[attr.title]` to the message string and `[attr.validity.validationMessage]` is not directly supported by Angular — instead, surface a `data-error-message` attribute or render a hidden `` so screen readers can announce it. + - Render the `general-endBeforeStart` block whenever `endBeforeStart` is present (drop the `touched` gate so the user sees the error the moment they leave End ≤ Start). Keep `general-eventEnd-error` rendering only for `invalidDatetime` cases. + - Symmetrically, for Event start, render the `general-eventStart-error` whenever `invalidDatetime` is on the control (today it is correctly shown when the input becomes dirty AND invalid). + +3. **Save gating remains:** `[disabled]="submitting() || form.invalid"` keeps Save disabled while any of (a) the inline per-field validators, (b) the cross-field `endBeforeStart`, or (c) the page-title required/blank validator are in error. No change needed. + +### Open Question (please confirm before implementation) + +The current docs intentionally allow empty Start/End so the event window can be cleared (`general.pure.ts:69-72`, `docs/api/admin.md:48-53`). The Job description says empty strings should be rejected (negative case 1). I am interpreting the Job as the source of truth and proposing we **reject empty strings** in both backend and client. If instead the intent is to keep `''` valid but reject "malformed-but-not-empty" strings, the backend fix becomes a no-op (it already does that) and only the UI changes are needed. Please confirm before implementation begins. + +## 4. Test Strategy + +- **Target backend test file:** new `tests/backend/admin-general-event-window.spec.ts`. Reuses `tests/backend/csrf-client.ts`, follows the exact pattern from `tests/backend/admin-validation.spec.ts:16-46` (boots `AppModule`, registers first admin, primes CSRF, then exercises `PUT /api/v1/admin/general/settings` via `request.agent`). +- **Target frontend test file:** `tests/frontend/admin-general-pure.spec.ts`. Add only the helper-level assertions (e.g., `isoDatetimeValidator({ value: '' })` returns `{ invalidDatetime: true }`) — no DOM rendering tests. The existing pattern uses plain `describe` / `it` with assertions on function returns. +- **Mocking strategy:** No new mocks. The backend spec uses the real `AppModule` against `:memory:` SQLite (set via `process.env.DATABASE_PATH = ':memory:'` at the top of the file, mirroring `admin-validation.spec.ts:1-3`). Auth uses `register-first-admin` + login + CSRF — already idempotent because the database is in-memory and the test creates exactly one admin in `beforeAll`. +- **Cases to cover (minimal & focused):** + 1. **Backend negative — empty Event End** ⇒ `PUT /settings` with `eventEndUtc: ''` returns HTTP 400 and the per-field message, and a follow-up `GET /settings` shows `eventEndUtc` unchanged. + 2. **Backend negative — empty Event Start** ⇒ symmetric to (1). + 3. **Backend negative — malformed datetime** ⇒ `PUT /settings` with `eventEndUtc: 'not-a-date'` returns HTTP 400 and `'eventEndUtc must be a valid ISO-8601 datetime'`. + 4. **Backend negative — end ≤ start** ⇒ valid Start + `End === Start` returns HTTP 400 with `'eventEndUtc must be strictly after eventStartUtc'`. + 5. **Backend positive — valid window** ⇒ well-formed `2026-08-15T10:30:00.000Z` / `2026-08-20T18:45:00.000Z` persists and is round-tripped unchanged. + 6. **Frontend pure** — assert `isoDatetimeValidator('')` returns the invalid sentinel (if we choose to flip empty to invalid client-side). + 7. **Frontend pure** — assert the new `endBeforeStartMessage` helper returns the canonical string. +- **Single command:** `npm test` from `/repo` runs the full suite. Frontend and backend can also be run separately via `npm run test:backend` / `npm run test:frontend` (already in `package.json`). + +## 5. Persistent Project Data (/data) + +Not applicable to this Job. No mock data, seed files, or shared assets are produced or consumed by the bug fix. Tests boot against `:memory:` SQLite, which is sufficient. diff --git a/backend/src/modules/admin/dto/general.dto.ts b/backend/src/modules/admin/dto/general.dto.ts index 22e1bf0..52496bf 100644 --- a/backend/src/modules/admin/dto/general.dto.ts +++ b/backend/src/modules/admin/dto/general.dto.ts @@ -7,8 +7,8 @@ export const GeneralSettingsSchema = z logo: z.string().max(2048), welcomeMarkdown: z.string().max(64_000), themeKey: z.enum(THEME_IDS as unknown as [string, ...string[]]), - eventStartUtc: z.union([z.literal(''), z.string().datetime({ message: 'eventStartUtc must be a valid ISO-8601 datetime' })]), - eventEndUtc: z.union([z.literal(''), z.string().datetime({ message: 'eventEndUtc must be a valid ISO-8601 datetime' })]), + eventStartUtc: z.string().datetime({ message: 'eventStartUtc must be a valid ISO-8601 datetime' }), + eventEndUtc: z.string().datetime({ message: 'eventEndUtc must be a valid ISO-8601 datetime' }), defaultChallengeIp: z.string().min(1).max(255), registrationsEnabled: z.boolean(), }) diff --git a/frontend/src/app/features/admin/general.component.ts b/frontend/src/app/features/admin/general.component.ts index aba8f26..048d617 100644 --- a/frontend/src/app/features/admin/general.component.ts +++ b/frontend/src/app/features/admin/general.component.ts @@ -10,6 +10,7 @@ import { datetimeMessage, deriveEventState, endAfterStartValidator, + endBeforeStartMessage, isoDatetimeValidator, normalizePageTitle, pageTitleMessage, @@ -113,14 +114,17 @@ import { type="datetime-local" formControlName="eventEndUtc" data-testid="general-eventEnd" - [attr.aria-invalid]="showEventEndError() || (form.controls.eventEndUtc.touched && !!form.error?.['endBeforeStart']) ? 'true' : null" - [attr.aria-describedby]="(showEventEndError() || (form.controls.eventEndUtc.touched && !!form.error?.['endBeforeStart'])) ? 'general-eventEnd-error general-endBeforeStart' : null" + [attr.aria-invalid]="showEventEndError() || eventEndCrossFieldError() ? 'true' : null" + [attr.aria-describedby]="(showEventEndError() || eventEndCrossFieldError()) + ? 'general-eventEnd-error general-endBeforeStart' + : null" + [attr.title]="(showEventEndError() ? eventEndMessage() : eventEndCrossFieldError() ? endBeforeStartMessage : null)" /> @if (showEventEndError()) {
{{ eventEndMessage() }}
} - @if (form.controls.eventEndUtc.touched && form.error?.['endBeforeStart']) { -
Event end must be after event start.
+ @if (eventEndCrossFieldError()) { +
{{ endBeforeStartMessage }}
} @@ -256,6 +260,10 @@ export class AdminGeneralComponent implements OnInit { datetimeMessage('Event end', this.eventEndValue(), this.form.controls.eventEndUtc.errors), ); + readonly eventEndCrossFieldError = computed( + () => !!this.form.errors?.['endBeforeStart'], + ); + constructor() { this.form.controls.welcomeMarkdown.valueChanges .pipe(takeUntilDestroyed(this.destroyRef)) diff --git a/frontend/src/app/features/admin/general.pure.ts b/frontend/src/app/features/admin/general.pure.ts index 0db8f5d..781bb32 100644 --- a/frontend/src/app/features/admin/general.pure.ts +++ b/frontend/src/app/features/admin/general.pure.ts @@ -67,7 +67,6 @@ export function toIsoUtc(local: string): string { export function isoDatetimeValidator(ctrl: { value: string | null | undefined }): { invalidDatetime: true } | null { const v = ctrl?.value ?? ''; - if (v === '') return null; const t = Date.parse(v); return Number.isFinite(t) ? null : { invalidDatetime: true }; } @@ -81,8 +80,10 @@ export function datetimeMessage( return `${fieldLabel} must be a valid ISO-8601 datetime.`; } const v = value ?? ''; - if (v !== '' && !Number.isFinite(Date.parse(v))) { + if (!Number.isFinite(Date.parse(v))) { return `${fieldLabel} must be a valid ISO-8601 datetime.`; } return null; } + +export const endBeforeStartMessage = 'Event end must be after event start.'; diff --git a/tests/backend/admin-general-event-window.spec.ts b/tests/backend/admin-general-event-window.spec.ts new file mode 100644 index 0000000..5c5fe02 --- /dev/null +++ b/tests/backend/admin-general-event-window.spec.ts @@ -0,0 +1,68 @@ +process.env.NODE_ENV = 'test'; + +import { GeneralSettingsSchema } from '../../backend/src/modules/admin/dto/general.dto'; + +const baseValid = { + pageTitle: 'OpenVelo', + logo: '', + welcomeMarkdown: '', + themeKey: 'classic', + defaultChallengeIp: '127.0.0.1', + registrationsEnabled: false, +}; + +function withEvent(start: string, end: string) { + return { ...baseValid, eventStartUtc: start, eventEndUtc: end }; +} + +describe('Admin general settings - event window validation (Job 888)', () => { + it('accepts a valid event window', () => { + const r = GeneralSettingsSchema.safeParse( + withEvent('2026-08-15T10:30:00.000Z', '2026-08-20T18:45:00.000Z'), + ); + expect(r.success).toBe(true); + }); + + it('rejects an empty eventEndUtc with a per-field message', () => { + const r = GeneralSettingsSchema.safeParse(withEvent('2026-08-15T10:30:00.000Z', '')); + expect(r.success).toBe(false); + if (r.success) return; + const endIssue = r.error.issues.find((i) => i.path.join('.') === 'eventEndUtc'); + expect(endIssue?.message).toBe('eventEndUtc must be a valid ISO-8601 datetime'); + }); + + it('rejects an empty eventStartUtc with a per-field message', () => { + const r = GeneralSettingsSchema.safeParse(withEvent('', '2026-08-20T18:45:00.000Z')); + expect(r.success).toBe(false); + if (r.success) return; + const startIssue = r.error.issues.find((i) => i.path.join('.') === 'eventStartUtc'); + expect(startIssue?.message).toBe('eventStartUtc must be a valid ISO-8601 datetime'); + }); + + it('rejects a non-empty malformed eventEndUtc', () => { + const r = GeneralSettingsSchema.safeParse(withEvent('2026-08-15T10:30:00.000Z', 'not-a-date')); + expect(r.success).toBe(false); + if (r.success) return; + const endIssue = r.error.issues.find((i) => i.path.join('.') === 'eventEndUtc'); + expect(endIssue?.message).toBe('eventEndUtc must be a valid ISO-8601 datetime'); + }); + + it('rejects end equal to start', () => { + const ts = '2026-08-15T10:30:00.000Z'; + const r = GeneralSettingsSchema.safeParse(withEvent(ts, ts)); + expect(r.success).toBe(false); + if (r.success) return; + const endIssue = r.error.issues.find((i) => i.path.join('.') === 'eventEndUtc'); + expect(endIssue?.message).toBe('eventEndUtc must be strictly after eventStartUtc'); + }); + + it('rejects end before start', () => { + const r = GeneralSettingsSchema.safeParse( + withEvent('2026-08-20T18:45:00.000Z', '2026-08-15T10:30:00.000Z'), + ); + expect(r.success).toBe(false); + if (r.success) return; + const endIssue = r.error.issues.find((i) => i.path.join('.') === 'eventEndUtc'); + expect(endIssue?.message).toBe('eventEndUtc must be strictly after eventStartUtc'); + }); +}); diff --git a/tests/backend/admin-general-service.spec.ts b/tests/backend/admin-general-service.spec.ts index a54b40c..f7c3416 100644 --- a/tests/backend/admin-general-service.spec.ts +++ b/tests/backend/admin-general-service.spec.ts @@ -141,7 +141,7 @@ describe('GeneralSettingsSchema - validation rules', () => { expect(r.success).toBe(false); }); - it('accepts empty strings (unconfigured event window) for both datetime fields', () => { + it('rejects empty strings for event datetime fields with per-field messages (Job 888)', () => { const r = GeneralSettingsSchema.safeParse({ pageTitle: 'T', logo: '', @@ -152,7 +152,12 @@ describe('GeneralSettingsSchema - validation rules', () => { defaultChallengeIp: '127.0.0.1', registrationsEnabled: false, }); - expect(r.success).toBe(true); + expect(r.success).toBe(false); + if (r.success) return; + const startIssue = r.error.issues.find((i) => i.path.join('.') === 'eventStartUtc'); + const endIssue = r.error.issues.find((i) => i.path.join('.') === 'eventEndUtc'); + expect(startIssue?.message).toBe('eventStartUtc must be a valid ISO-8601 datetime'); + expect(endIssue?.message).toBe('eventEndUtc must be a valid ISO-8601 datetime'); }); }); diff --git a/tests/frontend/admin-general-pure.spec.ts b/tests/frontend/admin-general-pure.spec.ts index 60b9c27..3d6018d 100644 --- a/tests/frontend/admin-general-pure.spec.ts +++ b/tests/frontend/admin-general-pure.spec.ts @@ -165,10 +165,10 @@ describe('pageTitleMessage', () => { }); describe('isoDatetimeValidator', () => { - it('returns null for an empty value (unconfigured is allowed)', () => { - expect(isoDatetimeValidator({ value: '' })).toBeNull(); - expect(isoDatetimeValidator({ value: null })).toBeNull(); - expect(isoDatetimeValidator({ value: undefined })).toBeNull(); + it('returns { invalidDatetime: true } for an empty value (empty string no longer accepted)', () => { + expect(isoDatetimeValidator({ value: '' })).toEqual({ invalidDatetime: true }); + expect(isoDatetimeValidator({ value: null })).toEqual({ invalidDatetime: true }); + expect(isoDatetimeValidator({ value: undefined })).toEqual({ invalidDatetime: true }); }); it('returns null for a valid ISO-8601 datetime', () => { @@ -194,9 +194,9 @@ describe('datetimeMessage', () => { .toBe('Event start must be a valid ISO-8601 datetime.'); }); - it('returns null for an empty value', () => { - expect(datetimeMessage('Event start', '', null)).toBeNull(); - expect(datetimeMessage('Event start', '', { invalidDatetime: false as any })).toBeNull(); + it('returns the per-field error for an empty value (empty is now invalid)', () => { + expect(datetimeMessage('Event start', '', null)).toBe('Event start must be a valid ISO-8601 datetime.'); + expect(datetimeMessage('Event end', '', { invalidDatetime: true })).toBe('Event end must be a valid ISO-8601 datetime.'); }); it('returns null for a valid ISO-8601 datetime', () => { -- 2.52.0