From 2ed7d27235194522ba7dbe5f1a894c2629b05e46 Mon Sep 17 00:00:00 2001 From: OpenVelo Agent Date: Wed, 22 Jul 2026 18:53:11 +0000 Subject: [PATCH] feat: Admin Area General Settings and Categories 1.15 --- .kilo/plans/896.md | 274 ------------------ .kilo/plans/897.md | 85 ++++++ .../category-form-modal.component.ts | 2 +- .../admin-categories-form-modal.spec.ts | 63 ++++ 4 files changed, 149 insertions(+), 275 deletions(-) delete mode 100644 .kilo/plans/896.md create mode 100644 .kilo/plans/897.md diff --git a/.kilo/plans/896.md b/.kilo/plans/896.md deleted file mode 100644 index 41da5e4..0000000 --- a/.kilo/plans/896.md +++ /dev/null @@ -1,274 +0,0 @@ -# Implementation Plan: Admin Area General Settings and Categories 1.14 (Job 896) - -## 0. Status — NOT yet implemented - -The required behaviour for this Job is **NOT** fully implemented in this -repository. The category edit modal opens but the `Name`, `Abbreviation`, -and `Description` reactive-form controls are not populated with the -selected category's existing values when the user clicks the edit button -(e.g. `cat-edit-CRY`), and the abbreviation input is therefore not marked -`readonly` for system rows. The bug is reproduced every time an existing -category is opened for edit, regardless of whether the user previously -opened it in create mode or not. - -Evidence (read-only inspection): -* `frontend/src/app/features/admin/categories/category-form-modal.component.ts:91-110` - declares a constructor `effect()` that *intends* to call - `this.form.patchValue({ name, abbreviation, description })` and - `this.abbreviationReadonly.set(c.isSystem)` when `mode() === 'edit' && - category()` is truthy. The rest of the source tree (parent component, - service, backend controller/service/DTO, schemas, migrations, and the - existing backend Jest suite) already implements everything the Guide - describes; only the frontend modal's edit-prefill step is broken. - -Because the fix lives entirely in one frontend component (and a new -Jest test), no DB migration, backend code change, dependency addition, -or `setup.sh` change is required. - ---- - -## 1. Architectural Reconnaissance - -- **Codebase style & conventions:** - - Frontend: Angular 17.3 (`/repo/frontend/package.json`), standalone - components, `ChangeDetectionStrategy.OnPush`, signal inputs / - outputs, reactive forms via `fb.nonNullable.group`. - - Backend: NestJS 10 + Express, modular layout under - `backend/src/modules/admin/**`, TypeORM + `better-sqlite3`. - - Tests: Jest 29 with two projects (`backend` = `ts-jest`+`node`, - `frontend` = `ts-jest`+`jsdom`) configured in - `/repo/tests/jest.config.js`. A single `npm test` from the repo - root runs every spec. Frontend specs live under `tests/frontend/` - alongside existing admin specs. -- **Data Layer:** - - SQLite via TypeORM; `category` table has - `id`, `system_key`, `name`, `abbreviation`, `description`, - `icon_path`, `created_at`, `updated_at` columns - (`backend/src/database/entities/category.entity.ts`). - - Categories are CRUD-ed via `AdminCategoriesService` which already - uppercases the abbreviation, protects system rows from - abbreviation edits, and rejects duplicate abbreviations - (`backend/src/modules/admin/categories.service.ts:39-78`). - - User-supplied persistent state is stored under `/data/hipctf` - (created by `setup.sh`); this job reads no project-shared data. -- **Test Framework & Structure:** - - Jest 29 (`ts-jest`). Two projects configured in - `tests/jest.config.js`. Frontend specs in `tests/frontend/`, - backend specs in `tests/backend/`. Single `npm test` (also wired - into `package.json` at the repo root) executes everything. - - No DOM harness library is configured; existing frontend specs - query the rendered DOM through `fixture.nativeElement.querySelector` - on the rendered template (see e.g. - `tests/frontend/admin-general-pure.spec.ts`). -- **Required Tools & Dependencies:** No new dependencies are needed. - All tooling required to fix and verify this job is already - installed by `setup.sh` and the root `package.json` / - `tests/package.json`. The implementer does **not** need to touch - `setup.sh`. - -### Component wiring already present in the repo - -| Layer | File | Status | -|-------|------|--------| -| Edit modal | `frontend/src/app/features/admin/categories/category-form-modal.component.ts` | **buggy** — edit pre-fill does not reach the DOM | -| Categories list + edit dispatcher | `frontend/src/app/features/admin/categories/categories.component.ts:109-114` | correct (`openEdit(c)` sets `editing` and `modalMode`) | -| General page (embeds categories) | `frontend/src/app/features/admin/general.component.ts:195` | already imports `AdminCategoriesComponent` | -| Admin service | `frontend/src/app/core/services/admin.service.ts:96-122` | correct (`listCategories`, `updateCategory`) | -| Backend controller | `backend/src/modules/admin/admin-categories.controller.ts` | correct | -| Backend service + DTO | `backend/src/modules/admin/categories.service.ts`, `backend/src/modules/admin/dto/categories.dto.ts` | correct | -| Backend tests | `tests/backend/admin-categories-service.spec.ts` | correct (system-row, dup, etc.) | -| Guide | `docs/guides/admin-categories.md` | correct (states the modal must pre-fill) | - ---- - -## 2. Impacted Files - -### To Modify (1 file) - -- `frontend/src/app/features/admin/categories/category-form-modal.component.ts` - — replace the broken constructor `effect()` with a robust Angular 17 - reactive-form prefill that always populates `name`, - `abbreviation`, `description`, sets `abbreviationReadonly` from - `c.isSystem`, and resets `iconPreview`/`iconFile` whenever the - parent changes the `category` or `mode` input. Must keep the - existing `[data-testid]` selectors intact. - -### To Create (1 file) - -- `tests/frontend/admin-categories-form-modal.spec.ts` - — Jest spec that mounts `CategoryFormModalComponent` with - `TestBed`, simulates the parent passing `open=true`, `mode='edit'`, - and a fully-populated `AdminCategory` (including `isSystem: true`), - then asserts that: - 1. `input[data-testid="cf-name"]` value equals the row's `name`, - 2. `input[data-testid="cf-abbr"]` value equals the row's - `abbreviation` *and* has the `readonly` attribute when - `isSystem === true`, - 3. `textarea[data-testid="cf-desc"]` value equals the row's - `description`, - 4. switching the inputs to a different category (a second - `setInput('category', ...)` / `setInput('mode', 'edit')` call) - re-populates the form to the new row's values, - 5. switching back to `mode='create'` clears the form. - ---- - -## 3. Proposed Changes - -### 1. Fix the edit-prefill in the modal (`category-form-modal.component.ts`) - -The bug: the existing constructor `effect()` reads -`this.category()` and `this.mode()` and calls -`this.form.patchValue(...)` inside an OnPush component. In Angular 17 -the value-update flow works, but the *display* of the patched value -inside the `` element is not reaching the -DOM because the change originates from a constructor -`effect()` while change-detection is sitting idle, and the -`formControlName` directive's write-back into the native `` -relies on the OnPush view being marked dirty. The same code path also -fails to mark `abbreviationReadonly`, which is exactly what the bug -report observes (abbreviation is not `readonly` and the three fields -read `''`). - -Replace the constructor body so that: - -1. We keep reading both inputs inside the effect: - `const c = this.category(); const m = this.mode();`. -2. We synchronise the form using **individual - `FormControl.setValue` calls** (not `patchValue` on the group), so - the directive chain inside each `FormControlName` runs the - `DefaultValueAccessor.writeValue` path that reaches the native - input. -3. After every prefill (edit or create-reset), we explicitly call - `inject(ChangeDetectorRef).markForCheck()` so the OnPush view - repaints. -4. We still set `abbreviationReadonly.set(c.isSystem)` and - `iconPreview.set(c.iconPath || null)` inside the `edit` branch, and - `abbreviationReadonly.set(false)` / - `iconPreview.set(null)` in the `else` branch. -5. We also clear `iconFile` (already done in current code) and reset - the `description` control in the create branch. -6. We add `allowSignalWrites: true` to the `effect()` options so that - the signal writes inside it are tolerated. - -Pseudocode for the replacement constructor: - -```ts -private readonly cdr = inject(ChangeDetectorRef); - -constructor() { - effect( - () => { - const c = this.category(); - const m = this.mode(); - if (m === 'edit' && c) { - this.form.controls.name.setValue(c.name, { emitEvent: false }); - this.form.controls.abbreviation.setValue(c.abbreviation, { emitEvent: false }); - this.form.controls.description.setValue(c.description ?? '', { emitEvent: false }); - this.abbreviationReadonly.set(!!c.isSystem); - this.iconPreview.set(c.iconPath || null); - } else { - this.form.controls.name.setValue('', { emitEvent: false }); - this.form.controls.abbreviation.setValue('', { emitEvent: false }); - this.form.controls.description.setValue('', { emitEvent: false }); - this.abbreviationReadonly.set(false); - this.iconPreview.set(null); - } - this.iconFile.set(null); - this.form.controls.name.markAsUntouched(); - this.form.controls.name.markAsPristine(); - this.form.controls.abbreviation.markAsUntouched(); - this.form.controls.abbreviation.markAsPristine(); - this.form.controls.description.markAsUntouched(); - this.form.controls.description.markAsPristine(); - this.cdr.markForCheck(); - }, - { allowSignalWrites: true }, - ); -} -``` - -All other parts of the modal (template selectors `cf-name`, -`cf-abbr`, `cf-desc`, `cf-icon`, `cf-cancel`, `cf-ok`, -`cat-form-modal`, `cat-form-backdrop`; the `CategoryFormSubmit` -output; the icon FileReader; the readonly binding on the abbreviation -input) stay exactly the same, so the parent -`AdminCategoriesComponent` and its `submit` handler do not need to -change. - -### 2. Add a frontend Jest spec (`admin-categories-form-modal.spec.ts`) - -The spec file lives under `tests/frontend/`. It runs with the existing -`tests/jest.config.js` `frontend` project (jest + jsdom), is picked up -by `npm test` from the repo root, requires no UI, and asserts the -behaviour the bug report says is broken. - -Mocking strategy: -* No HTTP mocks are required — the modal is rendered in isolation with - its three inputs (`open`, `mode`, `category`) bound directly via - `fixture.componentRef.setInput(...)`. `AdminService` is unused by - the modal. -* Construct `CategoryFormModalComponent` through `TestBed` and target - by querying the existing `data-testid` attributes - (`cf-name`, `cf-abbr`, `cf-desc`). No CSS selectors are introduced. -* Provide `FormBuilder` and `DestroyRef` via DI (already provided - via `inject(FormBuilder)` and `inject(DestroyRef)` because - `TestBed.createComponent` resolves Angular's DI tree). - -Spec assertions (single `describe('edit prefill')`, four `it`s): - -1. `populates name, abbreviation, description for a system row` — - set `open=true`, `mode='edit'`, - `category = { id:'x', name:'Cryptography', abbreviation:'CRY', - description:'Cryptographic challenges', iconPath:'', - isSystem:true }`, call `fixture.detectChanges()`, - read each input's `.value`, expect the row's values, and expect - `cf-abbr` to carry the `readonly` attribute. -2. `populates name, abbreviation, description for a user row` — same - shape but `isSystem: false`; expect `cf-abbr` to NOT be `readonly`. -3. `re-populates when the parent switches to another row` — - `setInput('mode','edit')` with row A, `detectChanges()`, assert - values, then `setInput('category', rowB)`, `detectChanges()`, - assert new values. -4. `clears the form when the parent switches back to create` — - after the edit-prefill assertions, `setInput('mode','create')`, - `setInput('category', null)`, `detectChanges()`, assert empty - values. - -The spec is intentionally minimal (5 assertions per case, no async -timers, no DOM harnesses) so it compiles and runs instantly. - ---- - -## 4. Test Strategy - -- **Target spec file:** - `tests/frontend/admin-categories-form-modal.spec.ts` (new). -- **Mocking strategy:** no HTTP mocks, no service mocks. The modal is - component-only; `fixture.componentRef.setInput(...)` simulates the - parent setting the `open`, `mode`, `category` signals, and - `fixture.detectChanges()` flushes the patched values to the DOM - via the OnPush view marker we add. -- **Coverage scope:** the four bullets above plus a smoke check that - `submit` and `cancel` outputs are not emitted by the modal alone - (the parent owns the network call). -- **Run command:** `npm test` from the repository root - (`tests/jest.config.js` is the active config). The new spec runs - inside the `frontend` project and finishes in well under a second - with no network or DB activity. -- **Existing tests:** re-run `tests/backend/admin-categories-service.spec.ts` - to confirm the system-row / dup / CRUD rules still pass; the - frontend modal change has no backend impact. - ---- - -## 5. Files Touched — Summary - -| Path | Action | -|------|--------| -| `frontend/src/app/features/admin/categories/category-form-modal.component.ts` | edit — replace constructor `effect()` body with the explicit-`setValue` + `markForCheck` version above; add `inject(ChangeDetectorRef)` and `{ allowSignalWrites: true }` option | -| `tests/frontend/admin-categories-form-modal.spec.ts` | create — the four `it()`s described in §3.2 | - -No backend, schema, DTO, controller, service, route, migration, or -dependency change is required, and `setup.sh` does not need to be -modified. diff --git a/.kilo/plans/897.md b/.kilo/plans/897.md new file mode 100644 index 0000000..55e4981 --- /dev/null +++ b/.kilo/plans/897.md @@ -0,0 +1,85 @@ +# 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/frontend/src/app/features/admin/categories/category-form-modal.component.ts b/frontend/src/app/features/admin/categories/category-form-modal.component.ts index 1326dad..393f236 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 @@ -64,7 +64,7 @@ export function syncCategoryForm( template: ` @if (open()) {