84 lines
12 KiB
Markdown
84 lines
12 KiB
Markdown
# Implementation Plan: Job 888 — Admin Area General Settings (event window validation)
|
|
|
|
## Status
|
|
NOT YET IMPLEMENTED. Two negative-case defects remain in the admin General settings form:
|
|
|
|
1. **Backend (`backend/src/modules/admin/dto/general.dto.ts:10-11`)** — the `eventStartUtc` / `eventEndUtc` fields are typed `z.union([z.literal(''), z.string().datetime({...})])`. Empty strings bypass validation and overwrite a previously valid event schedule with `''`. The docs at `docs/api/admin.md:48-53` and `docs/guides/admin-general-settings.md` describe this empty-allowed behaviour, but the Job explicitly requires the empty case to be rejected so the prior schedule is preserved.
|
|
2. **Frontend (`frontend/src/app/features/admin/general.component.ts:109-125`)** — the cross-field `endBeforeStart` error and the per-field `general-eventEnd-error` are both gated on `form.controls.eventEndUtc.touched`. When the user simply types a value the field-level @if may not render visibly, and the End input has no `aria-invalid`, `aria-describedby`, `title`, or `validity.validationMessage` binding, so a disabled Save button leaves the user with no visible cue.
|
|
|
|
The plan below fixes both defects and re-enables existing positive paths. No schema migration is required.
|
|
|
|
## 1. Architectural Reconnaissance
|
|
|
|
- **Codebase style & conventions:**
|
|
- **Backend:** NestJS 10, TypeScript strict, zod schemas under `backend/src/modules/<module>/dto/<module>.dto.ts` consumed via `ZodValidationPipe` (`backend/src/common/pipes/zod-validation.pipe.ts`). Errors are raised through `ApiError.validation(...)` returning `{ code: 'VALIDATION_FAILED', message, details: [{path, message}] }` with HTTP 400 (`backend/src/common/errors/api-error.ts`).
|
|
- **Frontend:** Angular 17+ standalone components with `ChangeDetectionStrategy.OnPush`, reactive forms (`ReactiveFormsModule`), and explicit signal wrappers (`Value`, `Invalid`, `TouchedOrDirty`) for inline error messaging (see `general.component.ts:200-257`). Templates use `@if` blocks with `data-testid` hooks tested in `tests/frontend/admin-general-pure.spec.ts`.
|
|
- **Pure helpers:** `frontend/src/app/features/admin/general.pure.ts` exports all validation/message helpers and is the only place to mutate when changing field-level rules; the component imports them.
|
|
|
|
- **Data Layer:** SQLite (`better-sqlite3`) with a `setting` key/value table (`docs/database/auth-settings.md`). `AdminGeneralService.updateSettings` (`backend/src/modules/admin/general.service.ts:61-74`) writes each `eventStartUtc` / `eventEndUtc` value through `SettingsService.set`. No schema migration is needed for this Job.
|
|
|
|
- **Test Framework & Structure:** Jest (`ts-jest`) with two projects (backend / frontend) configured in `tests/jest.config.js`. Tests live under `tests/backend/**` and `tests/frontend/**`. The repo-root `npm test` (alias of `jest --config tests/jest.config.js`) runs everything. Backend tests boot the full Nest app via `Test.createTestingModule({ imports: [AppModule] })` and use `request.agent` + CSRF (`tests/backend/csrf-client.ts`); frontend tests are pure-function specs under `tests/frontend/admin-general-pure.spec.ts` — no DOM, no UI. New tests must follow the same single-command layout.
|
|
|
|
- **Required Tools & Dependencies:** No new packages. The required tools (Node 20+, npm workspaces, `jest`, `ts-jest`, `better-sqlite3`) are already declared in `package.json` and bootstrapped by `setup.sh`. The implementer does not need to install anything new.
|
|
|
|
## 2. Impacted Files
|
|
|
|
- **To Modify:**
|
|
- `backend/src/modules/admin/dto/general.dto.ts` — tighten the `eventStartUtc` / `eventEndUtc` zod union so empty strings are rejected with the per-field message; leave `superRefine` end-after-start check in place.
|
|
- `frontend/src/app/features/admin/general.component.ts` — extend the Event end `<input>` 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 `<input>` 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 `<span class="sr-only">` 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.
|