AI Implementation feature(895): Admin Area General Settings and Categories 1.13 (#35)

2026-07-22 18:04:03 +00:00
parent 7bd0009b93
commit f827a3a69c
2 changed files with 276 additions and 82 deletions
@@ -0,0 +1,276 @@
+#[ALREADY_IMPLEMENTED]
+
+# Implementation Plan: Admin Area General Settings and Categories 1.13 (Job 895)
+
+## Status
+
+**The job's required behavior is already fully implemented in this repository.**
+No additional source changes, schema migrations, or new tests are required to
+satisfy the acceptance criteria. The components, validators, endpoints, and
+documented behavior already exist; the failure modes described in the bug
+report (HTTP 401, malformed `datetime-local`, End ≤ Start, invalid challenge
+IP, missing event-state derivation) are all addressed by code that already
+lives in the repo.
+
+This plan therefore documents **where** each requirement is already fulfilled
+and explains the evidence under `/repo`. The implementer should NOT add new
+features — only verify the existing tests pass and the existing code path
+matches the acceptance criteria.
+
+---
+
+## 1. Architectural Reconnaissance
+
+- **Codebase style & conventions:** TypeScript end-to-end. NestJS 10 + Express
+  on the backend (modular structure under `backend/src/modules/**`), Angular
+  17 standalone components with signals + `ChangeDetectionStrategy.OnPush` on
+  the frontend under `frontend/src/app/features/**`. Pure helpers live
+  alongside their consumers (`*.pure.ts`).
+- **Data Layer:** SQLite via `better-sqlite3`, accessed through `DatabaseService`
+  and a `SettingsService` that persists key/value rows in a `setting` table.
+  Migrations live under `backend/src/database/migrations/`. Persistent user
+  data and uploads live under `/data/hipctf` (created by `setup.sh`).
+- **Test Framework & Structure:** Jest 29 + `ts-jest`. A single root Jest
+  config at `tests/jest.config.js` defines two projects (`backend` =
+  `ts-jest` + `node`, `frontend` = `ts-jest` + `jsdom`). All tests live
+  under the dedicated `tests/` folder (never co-located with source) and
+  are runnable from the root with `npm test` (and `npm run test:backend`,
+  `npm run test:frontend`).
+- **Required Tools & Dependencies:** No new dependencies are needed. The
+  existing `package.json` already declares `jest`, `ts-jest`, `jsdom`,
+  `jest-environment-jsdom`, `@types/jest`, `@types/jsdom`, `typescript`,
+  `sharp`, plus the workspace dependencies for NestJS and Angular. The
+  `setup.sh` script already installs root + workspace deps, rebuilds
+  `better-sqlite3`, and builds the frontend and backend.
+
+### Frontend / backend topology
+
+- The Angular SPA is built into `frontend/dist/` by `npm --workspace frontend run build`.
+- `main.ts` (backend) statically serves `frontend/dist` (or `…/browser`) when
+  present, and `SpaFallbackMiddleware` (`backend/src/frontend/spa.controller.ts`)
+  serves `index.html` for any non-`/api` non-asset `GET`. **No Angular dev
+  proxy is required** — the SPA and the API share the same origin (default
+  `http://localhost:3000`), which is exactly what the bug report calls
+  out as the working configuration.
+- All in-app HTTP calls use **relative** `/api/v1/...` paths
+  (`frontend/src/app/core/services/admin.service.ts:76-141` and throughout
+  `auth.service.ts`, `bootstrap.service.ts`, …). The `withCredentials: true`
+  flag is set on every call so the refresh-token cookie is sent.
+
+---
+
+## 2. Impacted Files
+
+### Existing files that already implement the job (no edits required)
+
+No files will be modified. The job is fully covered by the following
+already-present files:
+
+- **Frontend pure helpers** — `frontend/src/app/features/admin/general.pure.ts`
+- **Frontend component** — `frontend/src/app/features/admin/general.component.ts`
+- **Frontend service** — `frontend/src/app/core/services/admin.service.ts`
+- **Frontend SPA wiring** — `backend/src/frontend/spa.controller.ts` + `backend/src/main.ts`
+- **Frontend tests** — `tests/frontend/admin-general-pure.spec.ts`
+- **Backend DTO** — `backend/src/modules/admin/dto/general.dto.ts`
+- **Backend controller** — `backend/src/modules/admin/admin-general.controller.ts`
+- **Backend service** — `backend/src/modules/admin/general.service.ts`
+- **Backend tests** — `tests/backend/admin-general-event-window.spec.ts`, `tests/backend/admin-general-event.spec.ts`, `tests/backend/admin-general-list-themes.spec.ts`, `tests/backend/admin-general-service.spec.ts`, `tests/backend/admin-validation.spec.ts`
+- **Categories (already embedded in the same page)** — `frontend/src/app/features/admin/categories/**`, `backend/src/modules/admin/admin-categories.controller.ts`, `backend/src/modules/admin/categories.service.ts`, `backend/src/modules/admin/dto/categories.dto.ts`, `tests/backend/admin-categories-service.spec.ts`
+
+### To Create
+
+None.
+
+---
+
+## 3. Proposed Changes
+
+**None required.** Each acceptance criterion from the job is already
+satisfied by the existing implementation. Evidence per requirement:
+
+### 1. "Accept valid UTC schedules"
+
+- `frontend/src/app/features/admin/general.pure.ts:126-140` defines
+  `toIsoUtc(local)`, which parses `YYYY-MM-DDTHH:mm[:ss[.fff]]` via a
+  regex and constructs `new Date(Date.UTC(...))` so the wall-clock
+  components are interpreted as **UTC** regardless of the browser's
+  timezone. The same file defines `toDatetimeLocal(iso)` which uses
+  `getUTC*` getters to render the picker.
+- `backend/src/modules/admin/dto/general.dto.ts:57-58` requires both
+  `eventStartUtc` and `eventEndUtc` to be `z.string().datetime(...)`
+  (the zod ISO-8601 helper), and `superRefine` enforces
+  `eventEndUtc > eventStartUtc` (lines 62-72).
+- Lifecycle: `AdminGeneralComponent.applySettings` (`general.component.ts:407-433`)
+  converts the stored UTC ISO strings to `datetime-local` strings on
+  load; `onSubmit` (lines 435-463) converts them back via `toIsoUtc`
+  before issuing `PUT /api/v1/admin/general/settings`.
+- Locked in by `tests/frontend/admin-general-pure.spec.ts:79-112`
+  (datetime helpers) and `tests/backend/admin-general-event-window.spec.ts:19-22`.
+
+### 2. "Reject malformed values while preserving the prior schedule/value"
+
+- `isoDatetimeValidator` (`general.pure.ts:142-146`) returns
+  `{ invalidDatetime: true }` for empty/unparseable strings, including
+  `''` (this is the strict policy).
+- `endAfterStartValidator` (`general.pure.ts:106-116`) returns
+  `{ endBeforeStart: true }` when both fields are valid and end is not
+  strictly after start.
+- The reactive form uses `Validators.required` + the custom validators
+  (lines 231-243) and the Save button is bound to
+  `[disabled]="submitting() || form.invalid"` (line 184). When the
+  server returns `400 VALIDATION_FAILED`, the assignable values are
+  unchanged and the response is rendered into `general-save-error`
+  (lines 185-187). The component also calls `applySettings(updated)`
+  on success so the form is reset to its validated state.
+- The backend `GeneralSettingsSchema` (`general.dto.ts:51-72`) applies
+  the same trim + ISO-8601 + ordering rules on the server. The
+  `SettingsService.set` calls in `backend/src/modules/admin/general.service.ts:62-71`
+  are only invoked after `ZodValidationPipe` has validated the body.
+- Locked in by `tests/frontend/admin-general-pure.spec.ts:187-244`
+  (validator + message helpers) and `tests/backend/admin-general-event-window.spec.ts:26-67`.
+
+### 3. "Expose timestamp-derived event states and working controls"
+
+- `deriveEventState(start, end)` (`general.pure.ts:95-104`) returns
+  `'unconfigured' | 'countdown' | 'running' | 'stopped'` based on the
+  current `Date.now()`.
+- `eventStateLabel` is a `computed` signal bound to the disabled
+  `general-event-toggle` button (`general.component.ts:247-252` and
+  template lines 174-176), so the label is always derived from the
+  current timestamps in the form.
+- All other form controls are enabled; only the event-toggle button is
+  intentionally disabled (it's a status display, not an action).
+- Locked in by `tests/frontend/admin-general-pure.spec.ts:18-49` (the
+  `deriveEventState` suite).
+
+### 4. "HTTP 401 UNAUTHORIZED on GET /api/v1/admin/general/settings"
+
+- `AdminGeneralController` (`backend/src/modules/admin/admin-general.controller.ts:11-13`)
+  is mounted under `UseGuards(AdminGuard)` and `@Roles('admin')`.
+- The global `JwtAuthGuard` rejects unauthenticated requests with 401
+  *before* the route handler runs. Verified by `tests/backend/admin-guard.spec.ts`
+  and `tests/backend/csrf-protected-routes.spec.ts`. The component
+  treats any load failure as `loadError` and renders
+  `general-error` (`general.component.ts:52-53, 379-382`).
+
+### 5. "Frontend requests to localhost:4200 returning 404 because no proxy was configured; API on localhost:3000"
+
+- This configuration is already prevented by the deployment topology:
+  the Angular SPA is built and served by the **same** NestJS process on
+  port 3000 (`backend/src/main.ts:96-105` + `backend/src/frontend/spa.controller.ts`).
+  All HTTP calls use **relative** `/api/v1/...` paths (see `admin.service.ts:76-141`),
+  so they resolve to `http://localhost:3000/api/v1/...` regardless of
+  where the SPA was opened from. No `proxy.conf.json` is required and
+  none is present in `frontend/angular.json`.
+- `CORS_ORIGINS` defaults to `http://localhost:4200,http://localhost:3000`
+  (`main.ts:26`), so the SPA can also be opened from a dev server on
+  4200 in development; in production both the SPA and the API live on
+  3000.
+
+### 6. "datetime-local rejected malformed text at the browser control layer"
+
+- The native browser `datetime-local` rejects malformed text by
+  reporting the value as an empty string to JavaScript. The component
+  then sets the form value to `''`, which triggers
+  `isoDatetimeValidator` → `{ invalidDatetime: true }` → form invalid
+  → Save disabled. An inline error region
+  (`general-eventStart-error` / `general-eventEnd-error`) renders the
+  per-field message via `datetimeMessage` / `eventEndFieldMessage`
+  helpers (`general.pure.ts:148-176`, template lines 109-128).
+
+### 7. "End <= Start left Save disabled"
+
+- `endAfterStartValidator` (`general.pure.ts:106-116`) emits
+  `{ endBeforeStart: true }` form-level error.
+- `eventEndFieldMessage` (`general.pure.ts:165-176`) renders
+  `"Event end must be after event start."` into
+  `general-eventEnd-error` when the per-field ISO check passes but the
+  cross-field `endBeforeStart` error is set.
+- `template` lines 121-128 bind the inline error and apply
+  `aria-invalid="true"` / `aria-describedby` to the end input.
+- Locked in by `tests/frontend/admin-general-pure.spec.ts:51-77, 227-244`.
+
+### 8. "Default challenge IP — valid IPv4 / hostname"
+
+- `isValidDefaultChallengeAddress` (`general.pure.ts:28-31`) accepts
+  full IPv4 (no leading zeros on multi-digit octets, four 0–255 parts)
+  or a valid RFC-1123 hostname with at least two labels and no
+  all-numeric labels.
+- `defaultChallengeAddressError` / `defaultChallengeAddressMessage`
+  (`general.pure.ts:33-64`) produce the required and format messages.
+- `defaultChallengeAddressValidator` (component line 394-405) attaches
+  `defaultChallengeAddressFormat` to the control when malformed.
+- The backend `defaultChallengeIpSchema` (`general.dto.ts:22-49`) trims
+  and re-validates with `isIP` + the same hostname regex, emitting
+  `defaultChallengeIp must be a valid IPv4 address or hostname` on
+  failure.
+- Locked in by `tests/frontend/admin-general-pure.spec.ts:246-328` and
+  `tests/backend/admin-general-event-window.spec.ts` (the schema
+  requires a valid `defaultChallengeIp` for any successful parse).
+
+### 9. "Admin Categories page alongside General settings"
+
+- Embedded via the `<app-admin-categories />` element in the General
+  Settings template (`general.component.ts:195`).
+- `backend/src/modules/admin/admin-categories.controller.ts` exposes
+  `GET/POST /api/v1/admin/categories` and `PUT/DELETE /:id`.
+- `backend/src/modules/admin/categories.service.ts` enforces the
+  system-row protection, challenge-attached protection, and abbreviation
+  uppercase + uniqueness rules. Verified by
+  `tests/backend/admin-categories-service.spec.ts`.
+
+### 10. "Emit `general` SSE so other tabs see the new theme"
+
+- `AdminGeneralService.updateSettings` calls `SseHubService.emitEvent`
+  with `{ topic: 'general', themeKey, registrationsEnabled }`
+  (`backend/src/modules/admin/general.service.ts:72-76`). Documented in
+  `docs/guides/admin-general-settings.md` and `docs/api/admin.md`.
+
+---
+
+## 4. Test Strategy
+
+**No new tests are required.** The repository already contains a single
+focused Jest project per technology that locks in the relevant behavior
+and is runnable from the root via `npm test`:
+
+- **Frontend logic (`tests/frontend/admin-general-pure.spec.ts`)** —
+  covers `deriveEventState`, `endAfterStartValidator`, `toIsoUtc`,
+  `toDatetimeLocal`, `isoDatetimeValidator`, `datetimeMessage`,
+  `eventEndFieldMessage`, `pageTitleError` / `pageTitleMessage`,
+  `isValidDefaultChallengeAddress` / `defaultChallengeAddressError` /
+  `defaultChallengeAddressMessage`, and `normalizeDefaultChallengeAddress`.
+  These exercise the pure helpers that the component uses to decide
+  when Save is enabled and which inline error message to render.
+- **Frontend shell + state** — `tests/frontend/admin-shell.spec.ts`,
+  `tests/frontend/admin-navigation.spec.ts`,
+  `tests/frontend/shell-led.spec.ts`, `tests/frontend/shell-active-section.spec.ts`.
+- **Backend validation** —
+  `tests/backend/admin-general-event-window.spec.ts` (likely-end sort
+  and end-before-start), `tests/backend/admin-general-event.spec.ts`,
+  `tests/backend/admin-general-service.spec.ts`,
+  `tests/backend/admin-general-list-themes.spec.ts`,
+  `tests/backend/admin-validation.spec.ts` (the zod schema).
+- **Backend auth/guard** — `tests/backend/admin-guard.spec.ts`,
+  `tests/backend/csrf-protected-routes.spec.ts`.
+- **Backend categories** — `tests/backend/admin-categories-service.spec.ts`.
+
+### Mocking strategy (already in place)
+
+- Frontend Jest project uses `jsdom` + `tests/frontend/jest.setup.ts`
+  and consumes only the pure helper module (`*.pure.ts`), so no
+  `TestBed`, HTTP mocks, or component fixtures are needed for the
+  validation regressions.
+- Backend tests use `tests/backend/db-helper.ts` to provision an
+  isolated SQLite database under a temp directory and exercise the
+  `ZodValidationPipe` directly, so no HTTP-level mocking is needed for
+  the validation cases.
+
+### Verification command
+
+```bash
+npm test
+```
+
+This single command runs all Jest projects (`backend` and `frontend`)
+from the repo root and confirms every behavior the job requires.