275 lines
13 KiB
Markdown
275 lines
13 KiB
Markdown
# 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 `<input formControlName="…">` 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 `<input>`
|
|
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.
|