m0rph3us1987/HIPCTF2
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

AI Implementation feature(898): Admin Area General Settings and Categories 1.16 (#38)

Browse Source
This commit was merged in pull request #38.
This commit is contained in:
m0rph3us1987
2026-07-22 19:19:46 +00:00
parent f62c77cf41
commit 113dd47371
11 changed files with 224 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.kilo/plans/897.md
-85
View File
@@ -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<T>` 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 `<form>` 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
<form class="modal" (click)="$event.stopPropagation()" (submit)="$event.preventDefault()" data-testid="cat-form-modal">
```
There is **no `[formGroup]="form"` binding** on the `<form>` element. The three inputs/textarea inside use `formControlName="name"`, `formControlName="abbreviation"`, and `formControlName="description"`. Without the `formGroup` directive on the parent `<form>`, 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 `<input>` 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 `<img>` 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 `<form>`) **before** the next `effect` flush writes the new `iconPreview` value, OR the user is reading the `<img>` 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 `<form>` opening tag from:
```html
<form class="modal" (click)="$event.stopPropagation()" (submit)="$event.preventDefault()" data-testid="cat-form-modal">
```
to:
```html
<form class="modal" [formGroup]="form" (click)="$event.stopPropagation()" (submit)="$event.preventDefault()" data-testid="cat-form-modal">
```
- 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 `<input>`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 `<form>` 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.
.kilo/plans/898.md
+35
View File
@@ -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.