OpenVelo Agent
63 lines
7.6 KiB
Markdown
63 lines
7.6 KiB
Markdown
# 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.
|