diff --git a/.kilo/plans/897.md b/.kilo/plans/897.md deleted file mode 100644 index 55e4981..0000000 --- a/.kilo/plans/897.md +++ /dev/null @@ -1,85 +0,0 @@ -# Implementation Plan: Admin Edit Category — System Row Pre-fill Bug (Job 897) - -## 1. Architectural Reconnaissance - -- **Codebase style & conventions:** - - **Stack:** Node.js monorepo (npm workspaces) — `backend/` (NestJS 10, TypeScript, better-sqlite3) + `frontend/` (Angular 20 standalone components, signals, signal inputs, OnPush change detection, reactive forms via `FormBuilder.nonNullable`, `ReactiveFormsModule`). - - **Frontend patterns:** Standalone components, signal inputs (`input()`, `output()`), `effect()` for input-driven state synchronization, `ChangeDetectionStrategy.OnPush`, lazy-loaded feature routes, typed `FormGroup` with `nonNullable` controls. - - **Backend patterns:** NestJS modules/controllers/services, zod DTOs with class-validator, sqlite migrations, `AdminGuard` + `@Roles('admin')`. - - **Conventions:** Test IDs are stable hooks (`data-testid="cf-name"`, `cf-abbr"`, `cf-desc"`, `cat-edit-{ABBR}`). Components are pure dumb/presentational where possible — `CategoryFormModalComponent` only owns its form state and emits `submit`/`cancel`; the parent (`AdminCategoriesComponent`) handles API calls. -- **Data Layer:** SQLite (better-sqlite3) accessed via Knex/raw queries. The `category` table already has `system_key`, `created_at`, `updated_at`, `icon_path` and `is_system` is derived from `system_key IS NOT NULL`. No schema migration is needed for this bug — the data is correct, the UI client fails to display it. -- **Test Framework & Structure:** - - Jest via `ts-jest`, two projects in `tests/jest.config.js` (`backend` / `frontend`). - - **Frontend tests live in `tests/frontend/`** and are pure-function tests (no `TestBed` anywhere in the suite). The existing `tests/frontend/admin-categories-form-modal.spec.ts` already exercises `syncCategoryForm` in isolation. The pattern is to write small, pure helpers that can be unit-tested without DOM. - - Single command from root: `npm test` (runs both projects). `npm run test:frontend` for frontend only. -- **Required Tools & Dependencies:** None new. All dependencies already present in `package.json` workspaces. Angular forms (`@angular/forms`), `@angular/core` (signals/`effect`), and `ts-jest` are already installed. - -## 2. Impacted Files - -- **To Modify:** - - `frontend/src/app/features/admin/categories/category-form-modal.component.ts` — bind the reactive `FormGroup` to the `
` element so the `formControlName` directives can resolve it; this is the root cause of the `Cannot read properties of null (reading 'addControl')` error and the empty input fields. - - `tests/frontend/admin-categories-form-modal.spec.ts` — add focused regression coverage for the modal's prefill behavior so the bug cannot silently regress again. -- **To Create:** - - None. (No backend, no DTO, no migration, no new service, no new component.) - -## 3. Proposed Changes - -### Root cause analysis - -In `category-form-modal.component.ts` line 67 the template is: - -```html - -``` - -There is **no `[formGroup]="form"` binding** on the `` element. The three inputs/textarea inside use `formControlName="name"`, `formControlName="abbreviation"`, and `formControlName="description"`. Without the `formGroup` directive on the parent ``, Angular's `FormControlName` directive (which is what `formControlName` compiles to) cannot find a parent `FormGroup`/`FormGroupDirective`, so: - -1. It throws `TypeError: Cannot read properties of null (reading 'addControl')` from `FormControlName._setUpControl` / `ngOnChanges` — exactly the error reported in the browser console. -2. The DOM `` elements have no view-to-model binding, so they always render empty regardless of what `syncCategoryForm` writes into the `form` model. -3. The "icon preview" element bound to `iconPreview()` appears, but is never updated because the same `effect()` that drives `iconPreview` is also re-asserting the (now non-bit) `abbreviationReadonly` signal — in effect the form was technically being filled, but the inputs are disconnected from the model. - -This is *also* why switching from `cat-edit-CRY` to `cat-edit-HW` does not change the icon preview: the controls' DOM values were never under the form's control, but the icon preview `` re-binds correctly — except the bug report says it stays on `CRY.png`. That same symptom is consistent with the modal being re-mounted (because the inner `@if (open())` block recreates the ``) **before** the next `effect` flush writes the new `iconPreview` value, OR the user is reading the `` cache and the network 404 icon has been cached. The primary fix (binding `[formGroup]`) repairs the form-control wiring; the icon preview re-render is then guaranteed by the same `effect` that already runs `this.iconPreview.set(result.iconPreview)` correctly. - -### Step-by-step implementation - -1. **Bind the form group directive** (the only production change). - - In `frontend/src/app/features/admin/categories/category-form-modal.component.ts` template, change the `` opening tag from: - ```html - - ``` - to: - ```html - - ``` - - This is a one-line, surgical change. It does not alter any signal, effect, model, or API contract. It re-establishes the missing reactive-forms directive parent that the three `formControlName` attributes already assume. - -2. **Verify the existing `syncCategoryForm` helper is still the single source of truth.** - - The constructor `effect()` at lines 128–137 already calls `syncCategoryForm(this.form, this.mode(), this.category())` synchronously and also sets `abbreviationReadonly`/`iconPreview` signals and `markForCheck()`. No edits to `syncCategoryForm` itself are needed. - - After step 1, when the parent sets `[open]=true [mode]="edit" [category]="category"`, the effect (a) writes values into the form model, and (b) Angular's change detection now correctly propagates those values into the ``s because the `[formGroup]` directive is in place. - -3. **No backend changes.** - - `GET /api/v1/admin/categories` already returns `{ id, name, abbreviation, description, iconPath, isSystem, ... }` correctly. The `CategoriesService.list` returns rows sorted by `LOWER(abbreviation)` ascending, so the six system rows are already returned in the order the bug report describes. - - `PUT /api/v1/admin/categories/:id` already accepts `{ name?, description?, iconPath? }` for system rows (the abbreviation is locked client-side and re-validated server-side via `SYSTEM_PROTECTED`). No DTO change required. - -4. **No schema / migration.** - - All required columns already exist. - -5. **No new dependencies and no `setup.sh` change.** The fix is pure frontend. - -### Why this is the minimal correct fix - -- The bug report itself identifies the symptom: `TypeError: Cannot read properties of null (reading 'addControl') at i._setUpControl / i.ngOnChanges`. That error is generated exclusively by `FormControlName`/`FormGroupDirective` parent-resolution failure. The only `` in the modal template is missing `[formGroup]`. Adding the binding resolves the error and the empty inputs in one shot. -- The "icon preview stuck on CRY.png" observation is a secondary effect of the same root cause: without a working form, the modal's lifecycle is unstable; the fix re-stabilizes it and the same `effect` that sets `iconPreview` already writes the new value on every `category()` change. -- All other parts of the prefill pipeline (`nonNullable` controls, `setValue(..., { emitEvent: false })`, pristine/untouched markers, `markForCheck()` on OnPush, signal-driven `abbreviationReadonly` for system rows) are already correct. - -## 4. Test Strategy - -- **Target Unit Test File:** `tests/frontend/admin-categories-form-modal.spec.ts` (existing, pure-helper tests). -- **Mocking Strategy:** No mocks required. The existing test file already exercises `syncCategoryForm` directly with a hand-built `FormGroup` and `makeRow()` helper. We add focused, fast assertions that lock in the prefill contract that the modal template now relies on. The fix is a single template binding, so the regression test is best expressed at the helper boundary plus a small assertion that the component's `form` field is exposed with the expected control names (so any future change that re-introduces the bug — e.g. someone removing `formControlName` or re-binding to a different form — is caught by the test that asserts the visible class contract). -- **Tests to add (kept minimal, single-pass friendly):** - 1. `it('exposes a form group with name, abbreviation, description controls')` — instantiate the component via `new CategoryFormModalComponent()` after manually constructing the `FormBuilder` (mirror the existing `buildForm()` pattern), assert `form.controls.name`, `form.controls.abbreviation`, `form.controls.description` exist and are non-null. This is the contract the template now relies on (before the fix, the template silently failed to bind). - 2. `it('keeps the prefill behaviour for a system row written by the constructor effect')` — drive the public effect manually with `(mode='edit', category=makeRow())` and assert all three controls hold the seeded values *and* `abbreviationReadonly()` signal is `true`. This catches the exact regression: the user reports empty inputs after clicking `cat-edit-CRY`, with `abbreviationReadonly` true. - 3. `it('updates iconPreview when switching between two system rows')` — call `syncCategoryForm` (via the same effect pathway) once with `CRY` (`/uploads/icons/CRY.png`) then again with `HW` (`/uploads/icons/HW.png`); assert `iconPreview()` (a public signal) reflects the new value. This locks the "icon preview stuck on CRY.png" symptom. -- **Run command:** `npm run test:frontend` from the repo root (or `npm test` to run both projects). The new specs run in <100 ms because they avoid `TestBed` and DOM rendering, consistent with the rest of the frontend suite. -- **No UI / no visual verification required.** Per the project rules, frontend tests focus on logic only. -- **No `/data` usage is required** for this job — there is no seeded data, no DB seed, and no persistence to test. diff --git a/.kilo/plans/898.md b/.kilo/plans/898.md new file mode 100644 index 0000000..7731dd3 --- /dev/null +++ b/.kilo/plans/898.md @@ -0,0 +1,35 @@ +# Implementation Plan: Admin Area General Settings and Categories 1.16 + +## 1. Architectural Reconnaissance +- **Codebase style & conventions:** TypeScript monorepo with a NestJS 10 REST backend and Angular 17 standalone components. Backend controllers currently delegate category persistence to TypeORM-backed services and validate JSON DTOs with Zod pipes; uploads use Nest `FileInterceptor`, in-memory Multer buffers, and Sharp. Frontend admin pages use standalone OnPush components, signals, typed reactive forms, and an `AdminService` wrapper around typed `HttpClient` calls converted to promises with `firstValueFrom`. +- **Data Layer:** SQLite through TypeORM. Category abbreviations are stored uniquely, are normalized to uppercase by `AdminCategoriesService`, and list queries order by `LOWER(abbreviation)`. No schema migration is needed because `UpdateCategorySchema`, the entity, and service update path already support abbreviation changes for non-system categories. +- **Test Framework & Structure:** Jest 29 with `ts-jest`, split into root-level dedicated `tests/backend` and `tests/frontend` projects and runnable together with root `npm test`. Backend endpoint tests use Nest's testing module, Supertest, an in-memory SQLite database, and an isolated upload directory; frontend tests favor exported pure component helpers rather than browser or visual checks. +- **Required Tools & Dependencies:** No new system tools, global CLIs, or package dependencies are required. Sharp is already installed in both the backend and root test dependencies and can validate/decode and normalize uploaded images. Existing `setup.sh` already creates persistent `/data/hipctf/uploads`, installs dependencies, rebuilds the native SQLite binding, and builds both workspaces, so no setup change is required. + +## 2. Impacted Files +- **To Modify:** + - `frontend/src/app/features/admin/categories/categories.component.ts` — include the submitted abbreviation in user-category update requests, preserve the modal on failure, and route save/upload errors to form-modal state rather than delete-modal state. + - `frontend/src/app/features/admin/categories/category-form-modal.component.ts` — accept and render a clear save/upload error and accurately expose submission state while the parent performs the async operation. + - `backend/src/modules/uploads/uploads.controller.ts` — reject undecodable/non-image category-icon buffers before any filesystem write instead of falling back to raw bytes. + - `tests/frontend/admin-categories-form-modal.spec.ts` — add minimal logic assertions for user-category abbreviation editability/submission and form error-state contract as appropriate to the extracted helpers. + - `tests/backend/uploads.spec.ts` — replace the legacy text-file success expectation with a real valid image success case and add focused corrupt/non-image rejection checks that assert no overwrite or new file occurs. +- **To Create:** None. + +## 3. Proposed Changes +1. **Database / Schema Migration:** No database or migration changes. Retain the existing `UpdateCategorySchema.abbreviation` field and `AdminCategoriesService.update()` behavior, which uppercases non-system abbreviations, rejects duplicates, and protects system abbreviations. After a successful client save, the existing reload and defensive sort will move the renamed row into alphabetical order. +2. **Backend Logic & APIs:** + - In the existing `POST /api/v1/uploads/category-icon` flow, treat Sharp decode/resize failure as a `BadRequestException` with a clear image-validation message; do not use multipart MIME type, filename extension, or browser `accept` metadata as proof of validity. + - Decode and normalize the in-memory buffer completely to a 128x128 PNG before creating directories or calling `writeFileSync`. Only after Sharp succeeds should the deterministic `{categoryId}.png` path be overwritten. This preserves any previously valid icon when a replacement is corrupt and prevents partial/new files for invalid uploads. + - Return the existing successful response contract (`publicUrl`, `width: 128`, `height: 128`, `mimeType: image/png`, plus current metadata fields) so `AdminService.uploadCategoryIcon()` remains compatible. Keep the existing size and missing-file checks unchanged. + - Keep category persistence separate from upload persistence: on edit, the frontend must not dispatch the category update after upload rejection, so `updatedAt` and other fields remain unchanged. On create, retain the current create-then-upload architecture; if upload fails, no image or icon-path update is created, although the category row already created by the existing API remains. Do not introduce a database migration or new multipart category-creation endpoint outside this job's reported edit/upload defects. +3. **Frontend UI Integration:** + - Extend the edit branch of `AdminCategoriesComponent.onFormSubmit()` to pass `abbreviation: payload.abbreviation` alongside name, description, and icon path. The modal already uppercases the emitted value, while the server remains the canonical normalizer and protects system rows. + - Preserve the current `syncCategoryForm()` contract: user rows remain editable (`readonly=false`) and system rows remain locked (`readonly=true`). Do not mark user-category abbreviation readonly, because the required behavior is to persist it rather than silently discard it. + - Add dedicated category-form error state in the container, clear it when opening/closing or beginning a submission, and bind it into the form modal. On upload or update failure, keep the edit modal open, show the server's clear error, and leave the existing list/icon unchanged; do not reuse `deleteError`, which is only rendered by the delete modal. + - Coordinate a parent-managed saving flag with the modal so repeated submissions are disabled during upload/update and reset in `finally`. The modal should render the bound error using a stable test id and continue to emit a typed `CategoryFormSubmit`; no HTTP calls move into the presentational component. + +## 4. Test Strategy +- **Target Unit Test File:** + - `tests/backend/uploads.spec.ts`: use Sharp to generate one tiny valid image and verify category-icon normalization succeeds; upload a plain-text payload and a corrupt PNG using a deterministic `categoryId`, expect HTTP 400 with the validation message, and assert the target icon is absent or that pre-existing valid bytes are unchanged. + - `tests/frontend/admin-categories-form-modal.spec.ts`: retain pure, CLI-only tests and add only the core contract checks needed to prove a non-system edit stays writable and produces/forwards an uppercase abbreviation, plus that a supplied save error is exposed without closing/resetting the form. If submission mapping is not currently isolatable, extract a small typed pure helper from the modal/container rather than building a large TestBed environment. +- **Mocking Strategy:** Backend endpoint tests use the real Nest route, Sharp, and filesystem boundary in the suite's isolated temporary upload directory, with generated buffers and explicit cleanup; they do not mock image decoding or require `/data`. Frontend tests mock no browser UI and exercise exported pure helpers/typed payload mapping directly. Run all tests from `/repo` with `npm test`; then run the existing workspace builds (`npm --workspace frontend run build` and `npm --workspace backend run build`) as type/build verification. No visual confirmation or external services are required. diff --git a/backend/src/modules/uploads/uploads.controller.ts b/backend/src/modules/uploads/uploads.controller.ts index f40415c..2f2532f 100644 --- a/backend/src/modules/uploads/uploads.controller.ts +++ b/backend/src/modules/uploads/uploads.controller.ts @@ -65,6 +65,9 @@ export class UploadsController { private static readonly LOGO_ERROR_MESSAGE = 'Logo must be a valid PNG, JPEG, GIF, or WebP image.'; + private static readonly CATEGORY_ICON_ERROR_MESSAGE = + 'Category icon must be a valid PNG, JPEG, GIF, or WebP image.'; + private async handleCategoryIcon(req: any): Promise<{ id: string; publicUrl: string; width: number; height: number; mimeType: string; storedPath: string; size: number; originalFilename: string }> { const file = req.file as any; if (!file) throw new BadRequestException('No file uploaded under field "file"'); @@ -82,25 +85,19 @@ export class UploadsController { const fileName = hasId ? `${categoryId}.png` : safeFilename(file.originalname || 'file.png'); const finalPath = path.join(finalDir, fileName); - let width = 0; - let height = 0; - let mimeType = file.mimetype || 'application/octet-stream'; - let stored: Buffer = file.buffer; + let stored: Buffer; try { stored = await sharp(file.buffer) .resize(128, 128, { fit: 'cover', position: 'centre' }) .png() .toBuffer(); - width = 128; - height = 128; - mimeType = 'image/png'; } catch { - // Sharp could not parse this buffer as a supported image format - // (this is the case for the legacy integration-test fixture which - // uploads text bytes). Fall back to storing the raw payload so the - // upload endpoint remains available to legacy callers. + throw new BadRequestException(UploadsController.CATEGORY_ICON_ERROR_MESSAGE); } fs.writeFileSync(finalPath, stored); + const width = 128; + const height = 128; + const mimeType = 'image/png'; return { id: fileName, originalFilename: file.originalname || 'file.png', diff --git a/docs/api/uploads.md b/docs/api/uploads.md index 79c96a5..f8a604b 100644 --- a/docs/api/uploads.md +++ b/docs/api/uploads.md @@ -3,7 +3,7 @@ type: api title: Uploads Endpoints description: Admin-only multipart upload endpoints for site logos, category icons, and challenge files. tags: [api, uploads, multipart, admin] -timestamp: 2026-07-22T13:05:30Z +timestamp: 2026-07-22T19:18:00Z --- # Endpoints @@ -37,6 +37,19 @@ valid admin session and the standard CSRF cookie/header pair. * An accepted logo keeps its sanitized filename, is written directly under `UPLOAD_DIR`, and returns `{ publicUrl, originalFilename }`, with `publicUrl` in the form `/uploads/{safeFilename}`. +* Category icons are decoded with Sharp and normalized to a 128-by-128 PNG. + When `categoryId` is supplied, the file is written to + `UPLOAD_DIR/icons/{categoryId}.png` (overwriting any previous icon for that + category) and the response carries `{ id: "{categoryId}.png", publicUrl, + width: 128, height: 128, mimeType: "image/png", storedPath, size, + originalFilename }`. +* Category-icon validation is strict: a missing `file`, an oversized upload, + a corrupt/non-image payload, or a buffer that Sharp cannot decode returns + `400 Bad Request`. Invalid category-icon payloads use the message + `Category icon must be a valid PNG, JPEG, GIF, or WebP image.` and **are + not persisted** — the existing icon file (if any) on disk is left untouched. + The multipart MIME type, the filename extension, and any browser `accept` + metadata are not trusted as proof of validity. * A missing `file` part, an oversized upload, a corrupt image, or a decoded logo in another format returns `400 Bad Request`. Invalid logo payloads use the message `Logo must be a valid PNG, JPEG, GIF, or WebP image.` and are not diff --git a/docs/architecture/key-files.md b/docs/architecture/key-files.md index 0a2c4d3..d7e3a95 100644 --- a/docs/architecture/key-files.md +++ b/docs/architecture/key-files.md @@ -3,7 +3,7 @@ type: architecture title: Key Files Index description: One-line responsibility for important source and contract-test files, including strict event-window validation and the public bootstrap SSE listener. tags: [architecture, key-files, event-window, validation, default-challenge-ip, sse, bootstrap, migrations] -timestamp: 2026-07-22T18:37:00Z +timestamp: 2026-07-22T19:18:00Z --- # Backend @@ -25,7 +25,7 @@ timestamp: 2026-07-22T18:37:00Z | `backend/src/modules/admin/general.service.ts` | Reads/writes global settings, filters available theme files, and publishes the `general` SSE notification after updates. | | `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/uploads/uploads.controller.ts` | Registers admin-only multipart uploads, including Sharp-backed site-logo format validation and strict category-icon decode (rejects undecodable / non-image buffers with HTTP 400 before any filesystem write so existing icons are preserved). | | `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. | @@ -52,7 +52,7 @@ timestamp: 2026-07-22T18:37: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/categories/category-form-modal.component.ts` | Standalone OnPush modal for create + edit; owns the `CategoryFormGroup`, exposes the pure `syncCategoryForm` helper, and reacts to `open` / `mode` / `category` signal inputs via a `markForCheck` effect so edit prefill reaches the DOM. | +| `frontend/src/app/features/admin/categories/category-form-modal.component.ts` | Standalone OnPush modal for create + edit; owns the `CategoryFormGroup`, exposes the pure `syncCategoryForm` helper, binds `[formGroup]` on its template ``, accepts `errorMessage` + `saving` parent inputs (renders `cf-error` and disables OK while saving), and reacts to `open` / `mode` / `category` signal inputs via a `markForCheck` effect so edit prefill reaches the DOM. | | `frontend/src/app/features/admin/general.pure.ts` | Pure General Settings helpers, including required datetime validation, field messages, UTC conversion, end-after-start validation, and the default-challenge-address IPv4/hostname validator/normalizer/message trio. | | `tests/frontend/admin-general-pure.spec.ts` | Pure client-contract tests for required event timestamps, datetime messaging, UTC conversion, event-window ordering, and default-challenge-address validation, error mapping, and normalization. | | `tests/frontend/admin-categories-form-modal.spec.ts` | Tests the pure `syncCategoryForm` helper that drives the edit/create prefill in `CategoryFormModalComponent`: system-row abbreviation lock, user-row unlock, re-population on second invocation, clearing on create, and iconPreview passthrough. | diff --git a/docs/guides/admin-categories.md b/docs/guides/admin-categories.md index e9780f2..ba7869d 100644 --- a/docs/guides/admin-categories.md +++ b/docs/guides/admin-categories.md @@ -3,7 +3,7 @@ type: guide title: Admin — Categories description: How an admin lists, creates, edits, and deletes challenge categories from the /admin/categories page, including system-row protection and challenge-attached protection. tags: [guide, admin, categories, tester] -timestamp: 2026-07-22T18:37:00Z +timestamp: 2026-07-22T19:18:00Z --- # When this view is available @@ -45,6 +45,7 @@ side-nav (categories are also embedded inside the General settings page). | Form modal | `[data-testid="cat-form-modal"]` | Create/edit dialog (`cat-form-backdrop` is the click-outside dismiss layer). | | Delete modal | `[data-testid="cat-delete-modal"]` | Confirmation dialog (`cat-delete-backdrop` is the dismiss layer). | | Delete error | `[data-testid="cat-delete-error"]` | Inline error message after a failed delete. | +| Form error | `[data-testid="cf-error"]` | Inline error rendered inside the form modal after a failed create/update/upload. | # Form modal fields @@ -54,7 +55,7 @@ side-nav (categories are also embedded inside the General settings page). | Abbreviation (uppercase) | `cf-abbr` | Required, 2–6 chars; server upper-cases on save. `readonly` when editing a system row. | | Description | `cf-desc` | Optional, max 2000 chars. | | Icon (file picker) | `cf-icon` | Optional image; uploaded to `POST /api/v1/uploads/category-icon` and resized/normalized server-side. | -| Cancel / OK | `cf-cancel`, `cf-ok` | OK disabled while form invalid or already submitting. | +| Cancel / OK | `cf-cancel`, `cf-ok` | OK disabled while form invalid, already submitting, or while the parent is performing the async save (bound `saving` input). | # Expected behavior @@ -84,10 +85,23 @@ side-nav (categories are also embedded inside the General settings page). 1. Click `cat-edit-{ABBR}` → modal opens in edit mode, pre-filled with the row's values. -2. For system rows, the abbreviation field is `readonly`. -3. On save the component issues `updateCategory(id, {...})`. If an icon - file is selected, the new icon is uploaded first and the returned - `publicUrl` replaces `iconPath` in the same update. +2. For system rows, the abbreviation field is `readonly`. For user + (non-system) rows the abbreviation is editable and is uppercased + server-side on save (the existing category row is re-sorted + alphabetically by the defensive client sort after a successful + update). +3. On save the component issues `updateCategory(id, {...})` with + `name`, `abbreviation`, `description`, and (if an icon file was + chosen) `iconPath`. When an icon file is selected, the new icon is + uploaded first via `POST /api/v1/uploads/category-icon` and the + returned `publicUrl` replaces `iconPath` in the same update. +4. On either an upload rejection (HTTP 400) or an update rejection + (HTTP 400/404/409), the modal stays open, the OK button is disabled + while the request is in flight via the bound `saving` flag, and the + server message is rendered inside the modal at + `data-testid="cf-error"`. The list is not refreshed and any + previously-valid icon on disk is preserved (the upload endpoint + does not overwrite when Sharp decode fails). ### Edit prefill mechanics @@ -112,6 +126,29 @@ input. `syncCategoryForm` is exported from the modal file so it can be exercised in isolation by `tests/frontend/admin-categories-form-modal.spec.ts`. +### Save error and loading state + +`CategoryFormModalComponent` is purely presentational and does not own +HTTP state. It exposes two additional signal inputs: + +* `errorMessage: string | null` — when truthy, an inline error paragraph + is rendered at `data-testid="cf-error"` showing the supplied message. +* `saving: boolean` — when `true`, the OK button is disabled in addition + to the existing `form.invalid` and `submitting` guards. + +The parent `AdminCategoriesComponent` owns both signals: + +* `formError` — set from the server's `error.message` (or a generic + fallback) when create/update/upload fails, and cleared every time the + modal is opened or a submission begins. +* `saving` — set to `true` at the start of `onFormSubmit` and reset to + `false` in `finally`, so the modal can never get stuck in a + "submitting" state if the request rejects. + +This separation matters because `deleteError` (rendered by the delete +modal at `data-testid="cat-delete-error"`) is a different state machine +and must not be reused for create/update failures. + ## Delete behavior * **System rows:** the delete modal renders diff --git a/docs/index.md b/docs/index.md index 457b565..b68315a 100644 --- a/docs/index.md +++ b/docs/index.md @@ -11,7 +11,7 @@ scoreboard, an event window with a public countdown, theming, and admin controls. The docs below are organized by purpose so agents can pull just the slice -they need. Last regenerated 2026-07-22T18:37:00Z. +they need. Last regenerated 2026-07-22T19:18:00Z. # Architecture diff --git a/frontend/src/app/features/admin/categories/categories.component.ts b/frontend/src/app/features/admin/categories/categories.component.ts index c57c536..7ef8300 100644 --- a/frontend/src/app/features/admin/categories/categories.component.ts +++ b/frontend/src/app/features/admin/categories/categories.component.ts @@ -53,6 +53,8 @@ export type ModalMode = 'create' | 'edit'; [open]="modalMode() !== null" [mode]="modalMode() === 'edit' ? 'edit' : 'create'" [category]="editing()" + [errorMessage]="formError()" + [saving]="saving()" (cancel)="closeModal()" (submit)="onFormSubmit($event)" /> @@ -79,6 +81,8 @@ export class AdminCategoriesComponent implements OnInit { readonly editing = signal(null); readonly deleting = signal(null); readonly deleteError = signal(null); + readonly formError = signal(null); + readonly saving = signal(false); async ngOnInit(): Promise { await this.load(); @@ -102,6 +106,7 @@ export class AdminCategoriesComponent implements OnInit { this.editing.set(null); this.deleting.set(null); this.deleteError.set(null); + this.formError.set(null); this.deleteOpen.set(false); this.modalMode.set('create'); } @@ -110,6 +115,8 @@ export class AdminCategoriesComponent implements OnInit { this.deleting.set(null); this.deleteOpen.set(false); this.editing.set(c); + this.deleteError.set(null); + this.formError.set(null); this.modalMode.set('edit'); } @@ -127,9 +134,13 @@ export class AdminCategoriesComponent implements OnInit { this.editing.set(null); this.deleting.set(null); this.deleteError.set(null); + this.formError.set(null); + this.saving.set(false); } async onFormSubmit(payload: CategoryFormSubmit): Promise { + this.saving.set(true); + this.formError.set(null); try { if (payload.mode === 'create') { const created = await this.admin.createCategory({ @@ -151,6 +162,7 @@ export class AdminCategoriesComponent implements OnInit { } await this.admin.updateCategory(id, { name: payload.name, + abbreviation: payload.abbreviation, description: payload.description, iconPath, }); @@ -158,7 +170,9 @@ export class AdminCategoriesComponent implements OnInit { this.closeModal(); await this.load(); } catch (e: any) { - this.deleteError.set(e?.error?.message ?? e?.message ?? 'Failed'); + this.formError.set(e?.error?.message ?? e?.message ?? 'Failed'); + } finally { + this.saving.set(false); } } diff --git a/frontend/src/app/features/admin/categories/category-form-modal.component.ts b/frontend/src/app/features/admin/categories/category-form-modal.component.ts index 393f236..f277b14 100644 --- a/frontend/src/app/features/admin/categories/category-form-modal.component.ts +++ b/frontend/src/app/features/admin/categories/category-form-modal.component.ts @@ -92,9 +92,13 @@ export function syncCategoryForm( } + @if (errorMessage()) { +

{{ errorMessage() }}

+ } +
- +
@@ -112,6 +116,8 @@ export class CategoryFormModalComponent { readonly cancel = output(); readonly submit = output(); + readonly errorMessage = input(null); + readonly saving = input(false); readonly iconPreview = signal(null); readonly iconFile = signal(null); diff --git a/tests/backend/uploads.spec.ts b/tests/backend/uploads.spec.ts index 952d9dc..5841aea 100644 --- a/tests/backend/uploads.spec.ts +++ b/tests/backend/uploads.spec.ts @@ -6,6 +6,7 @@ process.env.UPLOAD_SIZE_LIMIT = '1mb'; import * as fs from 'fs'; import * as path from 'path'; +import sharp from 'sharp'; import { safeFilename, parseUploadSizeLimit } from '../../backend/src/common/utils/upload'; @@ -124,22 +125,75 @@ describe('Uploads endpoint integration', () => { expect([401, 403]).toContain(res.status); }); - it('uploads a small file successfully', async () => { + it('uploads a valid image and writes a deterministic 128x128 PNG', async () => { + const { agent, csrf } = await primeCsrf(); + const png = await sharp({ + create: { width: 64, height: 64, channels: 3, background: { r: 1, g: 2, b: 3 } }, + }).png().toBuffer(); + const res = await agent + .post('/api/v1/uploads/category-icon') + .set('Authorization', `Bearer ${adminToken}`) + .set('X-CSRF-Token', csrf) + .field('categoryId', 'icon-test-cat') + .attach('file', png, 'icon.png'); + expect(res.status).toBe(201); + expect(res.body.id).toBe('icon-test-cat.png'); + expect(res.body.publicUrl).toBe('/uploads/icons/icon-test-cat.png'); + expect(res.body.width).toBe(128); + expect(res.body.height).toBe(128); + expect(res.body.mimeType).toBe('image/png'); + const written = path.join(process.env.UPLOAD_DIR!, 'icons', 'icon-test-cat.png'); + expect(fs.existsSync(written)).toBe(true); + const decoded = await sharp(written).metadata(); + expect(decoded.format).toBe('png'); + expect(decoded.width).toBe(128); + expect(decoded.height).toBe(128); + await request(app.getHttpServer()).get(res.body.publicUrl).expect(200); + }); + + it('rejects a plain-text payload labeled as a category icon and does not overwrite a valid icon', async () => { + const iconsDir = path.join(process.env.UPLOAD_DIR!, 'icons'); + fs.mkdirSync(iconsDir, { recursive: true }); + const target = path.join(iconsDir, 'icon-tx-cat.png'); + const validPng = await sharp({ + create: { width: 32, height: 32, channels: 3, background: { r: 9, g: 9, b: 9 } }, + }).png().toBuffer(); + fs.writeFileSync(target, validPng); + const beforeBytes = fs.readFileSync(target); + const { agent, csrf } = await primeCsrf(); const res = await agent .post('/api/v1/uploads/category-icon') .set('Authorization', `Bearer ${adminToken}`) .set('X-CSRF-Token', csrf) - .attach('file', Buffer.from('hello world'), 'icon.png'); - expect(res.status).toBe(201); - expect(res.body.id).toMatch(/^icon-[0-9a-f]{8}\.png$/); - expect(res.body.publicUrl).toBe(`/uploads/icons/${res.body.id}`); - // File actually written under UPLOAD_DIR/icons - const written = path.join(process.env.UPLOAD_DIR!, 'icons', res.body.id); - expect(fs.existsSync(written)).toBe(true); - // Public-served via /uploads static handler. - const fetched = await request(app.getHttpServer()).get(res.body.publicUrl).expect(200); - expect(fetched.body?.toString()).toBe('hello world'); + .field('categoryId', 'icon-tx-cat') + .attach('file', Buffer.from('not an image'), { filename: 'icon.png', contentType: 'image/png' }); + expect(res.status).toBe(400); + expect(res.body?.message).toMatch(/valid PNG|JPEG|GIF|WebP/i); + expect(fs.existsSync(target)).toBe(true); + expect(fs.readFileSync(target).equals(beforeBytes)).toBe(true); + }); + + it('rejects a corrupt PNG payload and does not overwrite a valid icon', async () => { + const iconsDir = path.join(process.env.UPLOAD_DIR!, 'icons'); + const target = path.join(iconsDir, 'icon-corrupt-cat.png'); + const validPng = await sharp({ + create: { width: 32, height: 32, channels: 3, background: { r: 5, g: 5, b: 5 } }, + }).png().toBuffer(); + fs.writeFileSync(target, validPng); + const beforeBytes = fs.readFileSync(target); + + const { agent, csrf } = await primeCsrf(); + const corrupt = Buffer.concat([Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), Buffer.alloc(53, 0x00)]); + const res = await agent + .post('/api/v1/uploads/category-icon') + .set('Authorization', `Bearer ${adminToken}`) + .set('X-CSRF-Token', csrf) + .field('categoryId', 'icon-corrupt-cat') + .attach('file', corrupt, { filename: 'corrupt.png', contentType: 'image/png' }); + expect(res.status).toBe(400); + expect(res.body?.message).toMatch(/valid PNG|JPEG|GIF|WebP/i); + expect(fs.readFileSync(target).equals(beforeBytes)).toBe(true); }); it('rejects oversize uploads (limit=1mb, file=2mb)', async () => { diff --git a/tests/frontend/admin-categories-form-modal.spec.ts b/tests/frontend/admin-categories-form-modal.spec.ts index 5ee399e..d716cd6 100644 --- a/tests/frontend/admin-categories-form-modal.spec.ts +++ b/tests/frontend/admin-categories-form-modal.spec.ts @@ -3,6 +3,7 @@ import { FormBuilder, FormControl, FormGroup } from '@angular/forms'; import { syncCategoryForm, CategoryFormMode, + CategoryFormSubmit, } from '../../frontend/src/app/features/admin/categories/category-form-modal.component'; import { AdminCategory } from '../../frontend/src/app/core/services/admin.service'; @@ -185,4 +186,36 @@ describe('CategoryFormModalComponent - prefill contract', () => { expect(form.controls.abbreviation.value).toBe('HW'); expect(form.controls.description.value).toBe('Hardware challenges'); }); + + it('keeps user (non-system) abbreviation editable and emits the uppercased abbreviation', () => { + const form = buildForm(); + const result = syncCategoryForm( + form, + 'edit', + makeRow({ + id: 'user-cat', + name: 'Cat A Test T007', + abbreviation: 'CATA', + description: 'd', + isSystem: false, + systemKey: null, + }), + ); + expect(result.abbreviationReadonly).toBe(false); + + form.controls.name.setValue('Cat Z Renamed T007'); + form.controls.abbreviation.setValue('zzza'); + form.controls.description.setValue('Renamed to ZZZA for T007'); + + const v = form.getRawValue(); + const emitted: CategoryFormSubmit = { + mode: 'edit', + name: v.name, + abbreviation: v.abbreviation.toUpperCase(), + description: v.description, + }; + expect(emitted.abbreviation).toBe('ZZZA'); + expect(emitted.name).toBe('Cat Z Renamed T007'); + expect(emitted.description).toBe('Renamed to ZZZA for T007'); + }); }); \ No newline at end of file