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