AI Implementation feature(901): Admin Area Challenges: List, Import/Export and Add/Edit Modal 1.01 (#42)
This commit was merged in pull request #42.
This commit is contained in:
@@ -1,62 +0,0 @@
|
||||
# Implementation Plan: Fix Admin Challenge Create Validation (All Fields "Required")
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
- **Codebase style & conventions:** TypeScript end-to-end, NestJS 10 REST API with `@Controller` / `@UseGuards` / `@Body` parameter pipes, Angular 17+ standalone components using signals + `ReactiveFormsModule`. The backend uses Zod (`zod`) for DTO validation via a custom `ZodValidationPipe`. The frontend uses Angular `FormBuilder` for state, a pure helper (`challenge-form.pure.ts`) for payload shaping, and an Angular `HttpClient`-based `AdminService`.
|
||||
- **Data Layer:** TypeORM + `better-sqlite3` (`./data/` SQLite file). Entities under `backend/src/database/entities/`. Schema is migration-driven.
|
||||
- **Test Framework & Structure:** Jest multi-project (`tests/jest.config.js`) with two projects — `backend` (Node env, `tests/backend/**/*.spec.ts`) and `frontend` (jsdom env, `tests/frontend/**/*.spec.ts`). Run with `npm test` from the repo root. Frontend tests already cover the pure helpers in `challenge-form.pure.ts` (`tests/frontend/admin-challenges-form.spec.ts`); backend tests cover service-level flows (`tests/backend/admin-challenges-service.spec.ts`) and API guards + structured validation (`tests/backend/admin-challenges-api.spec.ts`).
|
||||
- **Required Tools & Dependencies:** No new dependencies. The fix is contained in the frontend. Backend, schemas, and entity are unchanged.
|
||||
|
||||
## 2. Impacted Files
|
||||
- **To Modify:**
|
||||
- `frontend/src/app/features/admin/challenges/challenges.component.ts` — change `onFormSubmit` to unwrap the `{ body }` envelope before calling `AdminService.createChallenge` / `updateChallenge`. The modal already emits `{ body }` (see `challenge-form-modal.component.ts:528`), so the parent must extract `body` first. This is the root cause of the bug.
|
||||
- `tests/frontend/admin-challenges-form.spec.ts` — add a tiny regression test that pins the payload shape produced by the form-submit wiring (one assertion that the emitted body is the raw `CreateChallengeBody`, not a `{ body: ... }` wrapper).
|
||||
- **To Create:**
|
||||
- None. The backend DTOs (`backend/src/modules/admin/dto/challenges.dto.ts`), service (`backend/src/modules/admin/challenges.service.ts`), and controller (`backend/src/modules/admin/admin-challenges.controller.ts`) are already correct and need no changes.
|
||||
|
||||
## 3. Proposed Changes
|
||||
1. **Root cause (no schema change required):**
|
||||
The frontend form modal already builds the correct `CreateChallengeBody` via `toCreateBody(...)` (`challenge-form.pure.ts:158`) and emits it wrapped: `this.submit.emit({ body })` (`challenge-form-modal.component.ts:528`).
|
||||
The smart container at `challenges.component.ts:337-369` (`onFormSubmit`) forwards the entire emitted object into `AdminService.createChallenge(payload)` / `updateChallenge(id, payload)` (`admin.service.ts:268` and `:276`). As a result, the HTTP request body becomes `{ body: { name, descriptionMd, categoryId, ... } }` — every field is one level too deep.
|
||||
The backend `ZodValidationPipe` (`backend/src/common/pipes/zod-validation.pipe.ts`) runs `CreateChallengeSchema.safeParse(value)` where `value` is that wrapped object. All required top-level fields (`name`, `descriptionMd`, `categoryId`, `difficulty`, `initialPoints`, `minimumPoints`, `decaySolves`, `flag`, `protocol`) are `undefined`, so Zod emits `Required` for each one and the controller responds `400 VALIDATION_FAILED` with the `details[]` array — exactly what the Job describes.
|
||||
|
||||
2. **Frontend wiring fix (the only production change):**
|
||||
In `frontend/src/app/features/admin/challenges/challenges.component.ts`, change the body of `onFormSubmit(payload)` so the wrapped envelope is unwrapped before hitting the HTTP layer:
|
||||
```ts
|
||||
async onFormSubmit(payload: { body: CreateChallengeBody | UpdateChallengeBody }): Promise<void> {
|
||||
this.saving.set(true);
|
||||
this.formError.set(null);
|
||||
this.fieldErrors.set(null);
|
||||
const body = payload.body;
|
||||
try {
|
||||
if (this.formMode() === 'create') {
|
||||
await this.admin.createChallenge(body as CreateChallengeBody);
|
||||
} else {
|
||||
const detail = this.formDetail();
|
||||
if (!detail) throw new Error('Missing challenge detail');
|
||||
await this.admin.updateChallenge(detail.id, body as UpdateChallengeBody);
|
||||
}
|
||||
// ...rest unchanged
|
||||
} catch (e) { /* unchanged */ } finally { /* unchanged */ }
|
||||
}
|
||||
```
|
||||
Add `CreateChallengeBody` / `UpdateChallengeBody` to the existing import from `../../../core/services/admin.service` at the top of the file (they are already exported there).
|
||||
No other files need editing — the modal, the pure helpers, the service, the DTO schemas, the controller, and the persistence layer are all already correct.
|
||||
|
||||
3. **Optional (NOT required for this Job):** The `ChallengeFormSubmitPayload` envelope (`{ body }`) is now redundant. Leaving it intact is the smallest blast-radius fix and keeps the contract explicit for future fields. **No change here.**
|
||||
|
||||
4. **Smoke-testable end-to-end flow after the fix (manual verification, no UI tests):**
|
||||
- Add a WEB challenge with a unique name; expect `201` and a row in the list.
|
||||
- Add a second challenge with a name that comes before alphabetically to verify sort behaviour.
|
||||
- Type in the search box to verify `q=` debounce filters case-insensitively.
|
||||
- Toggle the Name sort header to verify asc/desc.
|
||||
|
||||
## 4. Test Strategy
|
||||
- **Target Test File:** `tests/frontend/admin-challenges-form.spec.ts` (extend the existing `pure: challenge form helpers` describe with a regression test on the payload shape).
|
||||
- **Mocking Strategy:** No new mocking needed. The new test is a pure unit test that calls `toCreateBody` with a representative defaults object and asserts the returned object's keys are top-level (`name`, `descriptionMd`, `categoryId`, `difficulty`, `initialPoints`, `minimumPoints`, `decaySolves`, `flag`, `protocol`, `port`, `ipAddress`, `enabled`, `files`) — i.e. there is no nested `body` property. This pins the contract between the pure helper and the HTTP layer so a future regression that wraps the payload again will be caught instantly.
|
||||
- **What is intentionally NOT covered:** No new live HTTP / supertest spec for `POST /api/v1/admin/challenges`. The existing `tests/backend/admin-challenges-api.spec.ts` already asserts the route's guard + validation error envelope; the existing `tests/backend/admin-challenges-service.spec.ts` already exercises `AdminChallengesService.create` end-to-end with a fully-formed payload and proves the backend works when given a valid body. Adding another e2e spec would require seeding a category + admin + CSRF cookie chain and would only re-prove what the service spec already proves — out of scope per the "minimal, focused tests" rule.
|
||||
- **Run:** `npm test` (or `npm run test:frontend` for the targeted project).
|
||||
|
||||
## Already-implemented evidence
|
||||
- Backend validation pipeline (`ZodValidationPipe` + `CreateChallengeSchema`) and persistence (`AdminChallengesService.create`) are correct and exercised by `tests/backend/admin-challenges-service.spec.ts` (the `creates a WEB challenge with a single staged file and exposes it in the list` case at `tests/backend/admin-challenges-service.spec.ts:88`).
|
||||
- Frontend pure helpers (`toCreateBody`, `validateChallengeForm`, `prefillChallengeFormDefaults`, `buildInitialFiles`) are correct and covered by `tests/frontend/admin-challenges-form.spec.ts`.
|
||||
- The defect is exclusively the missing unwrap in `challenges.component.ts onFormSubmit`, which wraps the raw `CreateChallengeBody` in an extra `{ body }` envelope before sending it to `AdminService.createChallenge`. The fix is one site, one new regression assertion, no schema or API change.
|
||||
@@ -0,0 +1,151 @@
|
||||
# Implementation Plan: Job 901 — Admin Area Challenges: List, Import/Export and Add/Edit Modal 1.01
|
||||
|
||||
## 0. Status: Already-Implemented Audit (per Job spec)
|
||||
|
||||
The Job description explicitly identifies **which cases already pass** and which are bugs to fix. Per the README/architecture docs (`docs/guides/admin-challenges.md`), the **list, search, sort, import, export, delete, file staging, and discard-on-cancel** flows are fully implemented and verified by the existing test suites (`tests/frontend/admin-challenges-list.spec.ts`, `tests/frontend/admin-challenges-form.spec.ts`, `tests/backend/admin-challenges-*.spec.ts`). The cases the spec marks PASS (1, 2, 5, 7) are therefore considered already implemented.
|
||||
|
||||
**Cases that FAIL and need code changes:**
|
||||
- **Case 3** — Connection NC with blank/non-numeric Port: submit is silently rejected; `validateChallengeForm`'s `'Port is required for NC'` message is computed in `onOk()` but never surfaced as a `cf-error-port` (or general) inline error. Also `<input type=number>` strips non-digits before they reach the validator.
|
||||
- **Case 4** — Blank IP with NC: the form auto-fills `port` to `1337` and accepts the submission, but the requirement is that blank IP with NC should not silently succeed. (Re-read Job text: "no rejection — IP is optional and the form auto-fills port to 1337, so a blank IP with NC accepted submission with port 1337." — i.e. blank IP itself is OK; the issue is that auto-fill of `port` to `1337` masks the real "Port is required" error. The Job marks this as a separate failing case to be discussed.)
|
||||
- **Case 6** — Drag-and-drop oversized upload: the modal only wires `<input type=file>`. No `drop`/`dragover` handler exists.
|
||||
|
||||
The plan therefore covers only the **delta** required to fix cases 3, 4, 6, plus regression tests for those fixes.
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
|
||||
- **Codebase style & conventions:**
|
||||
- TypeScript monorepo (`backend/` NestJS + `frontend/` Angular). `strict` TS across both.
|
||||
- Frontend uses **standalone components**, **OnPush**, Angular **signals** for component state, **Reactive Forms** with `FormBuilder.nonNullable.group(...)`.
|
||||
- Errors are surfaced via per-field signals `fieldErrors` (Record<field,string>) and a top-level `formError` signal; inline elements use `data-testid="cf-error-<field>"`.
|
||||
- Backend: NestJS modules; `admin-challenges.controller.ts` + `challenges.service.ts` + `challenge-files.service.ts`. Zod-validated DTOs. Already enforces size limit (`UPLOAD_SIZE_LIMIT` via `parseUploadSizeLimit`) and per-upload oversize rejection — verified by `tests/backend/uploads.spec.ts`.
|
||||
- Styling is component-scoped via `styles: [...]` literals (no global CSS edits required).
|
||||
- **Data Layer:** SQLite via better-sqlite3, file-based under `./data/` (persistent named volume `/data` per project policy).
|
||||
- **Test Framework & Structure:**
|
||||
- Jest with two projects (`tests/jest.config.js`): `backend` (node) and `frontend` (jsdom).
|
||||
- Single command from root: `npm test`.
|
||||
- Tests live in `tests/backend/*.spec.ts` and `tests/frontend/*.spec.ts` — **never** alongside source.
|
||||
- **Required Tools & Dependencies:** No new packages required. All tooling (Angular Forms, Reactive Forms, Jest jsdom, ts-jest) already present.
|
||||
|
||||
## 2. Impacted Files
|
||||
|
||||
- **To Modify:**
|
||||
- `frontend/src/app/features/admin/challenges/challenge-form-modal.component.ts`
|
||||
- Plumb `clientErrors` from `validateChallengeForm` into `fieldErrors` input (or a new local `clientFieldErrors` signal) so `cf-error-port` renders.
|
||||
- Replace the silent "abort" path in `onOk()` with: when `clientErrors` is non-empty, mark only the relevant controls touched/dirty (so untouched ones don't pop up), emit/show inline errors, switch to the tab that contains the first error, and short-circuit the submit. Clear these client errors when the user edits a field.
|
||||
- Change `<input id="cf-port" type="number" ...>` to `type="text" inputmode="numeric"` so non-numeric characters are not silently stripped; add `Validators.pattern(/^\d+$/)` and an integer validator (`Validators.min(1)`, `Validators.max(65535)`) on the `port` control so `<input type=number>` quirks don't hide the value. Validate numeric-ness in `validateChallengeForm` (it already checks `Number.isInteger`).
|
||||
- Stop the **protocol valueChanges side-effect** from auto-filling `port` to `1337` when switching to NC — instead leave it `null` so the pure validator can fire "Port is required for NC". (This addresses Case 4: blank IP should not cause silent success driven by auto-fill port; with NC, blank port must be rejected visibly.)
|
||||
- Add a `(drop)` / `(dragover)` / `(dragleave)` handler to the Files tab to support drag-and-drop uploads, reusing the existing `onFileChange` staging pipeline. Reject oversize files using the same `globalFileLimit` check.
|
||||
- Add `preventDefault` on `dragover` so the browser does not navigate to the file when dropped outside the input.
|
||||
- `frontend/src/app/features/admin/challenges/challenge-form-modal.component.ts` (template portion only):
|
||||
- Add a drag-and-drop zone (`<div class="drop-zone" (dragover)="onDragOver($event)" (drop)="onDrop($event)">`) wrapping the file input. Show visual state on dragover.
|
||||
- `frontend/src/app/features/admin/challenges/challenge-form.pure.ts`
|
||||
- Add a small helper `firstErrorTab(errors)` that maps a `ChallengeFormErrors` map to `'general' | 'connection' | 'files'` so the modal can auto-switch the active tab when a field is rejected.
|
||||
- Tighten `validateChallengeForm` for port: reject non-integer or out-of-range values for NC **even if** the user typed characters — already covered by `Number.isInteger`/`min`/`max`. (No logic change needed; just rely on it.)
|
||||
- `tests/frontend/admin-challenges-form.spec.ts`
|
||||
- Add regression specs for cases 3 and 4: validate that submitting NC with blank port surfaces `cf-error-port` text "Port is required for NC"; validate that the auto-fill-to-1337 behavior is removed so a blank NC port triggers the inline error.
|
||||
- Add a regression spec for case 6: simulate a `drop` event with a `DataTransfer` containing a small `File` and verify `AdminService.stageChallengeFile` is called and a new staged entry appears; also verify oversize drop is rejected via `cf-upload-error` without hitting the network.
|
||||
|
||||
- **To Create:**
|
||||
- **None.** All edits go into existing files. (No new files are required; we are extending existing modules per the Job "Add/Edit Modal 1.01" patch.)
|
||||
|
||||
## 3. Proposed Changes
|
||||
|
||||
### 3.1 Backend — no changes required
|
||||
- The server already returns `400 VALIDATION_FAILED` with `details[].path = "port"` for blank/non-numeric NC ports (`backend/src/modules/admin/dto/challenges.dto.ts` Zod schema enforces `port: z.number().int().min(1).max(65535).optional()` with conditional refinement for `NC`). The frontend simply must render the error, which is the responsibility of `validateChallengeForm` (client-side, before any network call).
|
||||
- The file-size limit is already enforced by `challenge-files.service.ts` (the staged upload endpoint returns 400 with a structured error envelope that the modal already maps to `cf-upload-error` in `onFileChange`).
|
||||
|
||||
### 3.2 Frontend — `challenge-form-modal.component.ts`
|
||||
|
||||
1. **Stop auto-filling `port` on protocol change.**
|
||||
- In the `protocol.valueChanges` subscriber (currently lines 373–381), remove the `else if (p === 'NC' && ...)` branch that sets `port` to `1337`. With this removed, switching to NC leaves `port === null`, so the next submit fires the `validateChallengeForm` pure-error `'Port is required for NC'` path, which we then surface (step 3).
|
||||
|
||||
2. **Surface `validateChallengeForm` errors inline.**
|
||||
- Add a private `clientFieldErrors = signal<ChallengeFormErrors | null>(null)`.
|
||||
- In `onOk()`:
|
||||
```ts
|
||||
const clientErrors = validateChallengeForm(values);
|
||||
if (Object.keys(clientErrors).length > 0) {
|
||||
this.clientFieldErrors.set(clientErrors);
|
||||
// mark only those controls touched so the existing `showError()` template renders
|
||||
for (const f of Object.keys(clientErrors) as (keyof ChallengeFormErrors)[]) {
|
||||
const ctrl = this.form.controls[f as keyof ChallengeFormControls];
|
||||
if (ctrl) { ctrl.markAsTouched(); ctrl.markAsDirty(); }
|
||||
}
|
||||
// auto-switch to the tab containing the first error
|
||||
const tab = firstErrorTab(clientErrors);
|
||||
if (tab) this.tab.set(tab);
|
||||
return;
|
||||
}
|
||||
this.clientFieldErrors.set(null);
|
||||
```
|
||||
- Extend `showError(field)` (currently lines 401–419) to **prefer** `clientFieldErrors()[field]` over server errors and over Angular built-in validator errors. The pure-helper message is the most specific; the existing fall-through to required/min/max remains for untouched-after-clearing flows.
|
||||
- Clear the per-field `clientFieldErrors` entry when the user edits the corresponding control: subscribe to `this.form.valueChanges` and on each change drop any key whose value now passes that field's pure check (cheap: re-run `validateChallengeForm` and diff). This keeps the error visible until the user actually fixes the input.
|
||||
|
||||
3. **Replace `<input type="number">` for port with `<input type="text" inputmode="numeric">`.**
|
||||
- Rationale: native `type=number` silently strips non-digits before Angular's FormControl sees them, which prevents `validateChallengeForm` from ever receiving a non-numeric value to reject. Using `type="text" inputmode="numeric"` keeps the on-screen numeric keypad on mobile but lets the validator surface a clear error.
|
||||
- Add to the `port` control: `Validators.pattern(/^\d+$/)` (in `buildChallengeFormGroup`) so the Angular built-in path also catches it, giving `showError('port')` an additional message via the existing `pattern` branch. (Add a `pattern` branch in `showError` returning `${humanLabel(field)} has an invalid value`.)
|
||||
|
||||
4. **Add drag-and-drop support to the Files tab.**
|
||||
- Template change (inside the `tab() === 'files'` block):
|
||||
```html
|
||||
<div class="drop-zone"
|
||||
[class.dragover]="dragOver()"
|
||||
(dragover)="onDragOver($event)"
|
||||
(dragleave)="onDragLeave($event)"
|
||||
(drop)="onDrop($event)"
|
||||
data-testid="cf-drop-zone">
|
||||
<p>Drop files here or use the picker below</p>
|
||||
<input id="cf-upload" type="file" multiple (change)="onFileChange($event)" data-testid="cf-upload" />
|
||||
</div>
|
||||
```
|
||||
- Component:
|
||||
```ts
|
||||
readonly dragOver = signal(false);
|
||||
onDragOver(ev: DragEvent): void { ev.preventDefault(); this.dragOver.set(true); }
|
||||
onDragLeave(ev: DragEvent): void { this.dragOver.set(false); }
|
||||
onDrop(ev: DragEvent): void {
|
||||
ev.preventDefault();
|
||||
this.dragOver.set(false);
|
||||
const files = ev.dataTransfer?.files ? Array.from(ev.dataTransfer.files) : [];
|
||||
if (files.length === 0) return;
|
||||
// Reuse the same staging pipeline as the picker:
|
||||
void this.stageFiles(files);
|
||||
}
|
||||
```
|
||||
Extract the existing `for (const file of list)` loop in `onFileChange` into a private `stageFiles(files: File[])` method; both `onFileChange` and `onDrop` call it. Oversize check, error message, and placeholder staging remain identical, so case 5 behavior is preserved and case 6 is now covered.
|
||||
|
||||
### 3.3 Frontend — `challenge-form.pure.ts`
|
||||
- Add `firstErrorTab(errors)` mapping `name|descriptionMd|categoryId|difficulty|initialPoints|minimumPoints|decaySolves|flag → 'general'`, `protocol|port|ipAddress → 'connection'`, empty otherwise.
|
||||
|
||||
### 3.4 Tests (regression only, in dedicated folder)
|
||||
Add cases to `tests/frontend/admin-challenges-form.spec.ts`:
|
||||
1. **Case 3 — blank NC port:**
|
||||
- Build the form, set values to a valid challenge but `protocol: 'NC'` and `port: null`.
|
||||
- Call the modal's `onOk()` indirectly: assert that after invoking submit, `clientFieldErrors` contains `port === 'Port is required for NC'` and the `cf-error-port` element renders that text (mount the component via `TestBed.createComponent`).
|
||||
- Assert that the port `<input>` no longer has `type="number"` (regression for non-numeric stripping).
|
||||
2. **Case 3 — non-numeric NC port:**
|
||||
- Simulate user typing `"12a3"` into the port field. Assert the input retains the raw string and `validateChallengeForm` returns `port: 'Port is required for NC'` (or new "must be an integer" message).
|
||||
3. **Case 4 — blank IP with NC, blank port:**
|
||||
- Verify the modal **does not** auto-fill port to 1337 when switching to NC, and that submitting with `ipAddress=''` and `port=null` shows the inline port error.
|
||||
4. **Case 6 — drag-and-drop:**
|
||||
- Stub `AdminService.stageChallengeFile` via TestBed providers; dispatch a synthetic `drop` event with a `File` in `dataTransfer.files`; assert the staged entry appears and `stageChallengeFile` was called.
|
||||
- Drop a file larger than `globalFileLimit`; assert `cf-upload-error` text appears and no network call is made.
|
||||
|
||||
(For new tests the existing `tests/frontend/admin-challenges-form.spec.ts` already imports from the component; we extend it in place rather than creating new files.)
|
||||
|
||||
## 4. Test Strategy
|
||||
|
||||
- **Target Unit Test File:**
|
||||
- `tests/frontend/admin-challenges-form.spec.ts` (existing; extend with the four cases above).
|
||||
- **Mocking Strategy:**
|
||||
- All external boundaries (the network) are mocked via Angular's `provideHttpClientTesting` plus a stub of `AdminService` for `stageChallengeFile` / `discardChallengeFile` / `createChallenge` / `updateChallenge`.
|
||||
- The drag-and-drop test stubs `AdminService.stageChallengeFile` with a Jasmine spy that resolves `{ token: 't-1', publicUrl: '/uploads/challenges/x', originalFilename, mimeType, sizeBytes }`.
|
||||
- For oversize, assert the spy is **not** called and `fileUploadError` is set, using a synthetic `File` built from a `Uint8Array` of the desired length.
|
||||
- Backend tests are not required because the server already rejects blank/non-numeric ports and oversize uploads (covered by `tests/backend/admin-challenges-api.spec.ts` and `tests/backend/uploads.spec.ts`).
|
||||
|
||||
## 5. Out of Scope / Non-changes
|
||||
|
||||
- No backend changes (server already returns the right error envelopes).
|
||||
- No new dependencies, no `setup.sh` edits, no migration scripts, no DB schema changes.
|
||||
- No changes to import/export, list, search, sort, delete flows (already passing per Job spec).
|
||||
- No drag-and-drop file picker behavior for import — that uses a `<input type=file>` chooser inside `AdminChallengesComponent.pickImportFile`, which the Job does not flag.
|
||||
Reference in New Issue
Block a user