OpenVelo Agent
174 lines
8.1 KiB
Markdown
174 lines
8.1 KiB
Markdown
# Implementation Plan: Admin General Settings — Event Start/End Validation (Job 887)
|
|
|
|
## Status
|
|
|
|
NOT already implemented. Two distinct gaps are described in the Job:
|
|
|
|
1. **Backend (`PUT /api/v1/admin/general/settings`)** accepts any string for
|
|
`eventStartUtc` / `eventEndUtc` and persists it. Only the cross-field
|
|
`end > start` rule is enforced. Garbage payloads like
|
|
`eventStartUtc='not-a-date'` return HTTP 200 and overwrite the stored
|
|
schedule.
|
|
2. **Frontend (`/admin/general`)** has no per-field validation feedback for the
|
|
two `datetime-local` inputs. The cross-field rule fires correctly
|
|
(`form.invalid`, Save disabled), but the inputs render no `title`,
|
|
`aria-invalid`, `aria-describedby`, `validationMessage`, or `[data-testid=...-error]`
|
|
element when either field is empty/invalid individually.
|
|
|
|
## 1. Architectural Reconnaissance
|
|
|
|
- **Codebase style & conventions:**
|
|
- Backend: NestJS controllers, Zod schemas in `*.dto.ts`, validated by the
|
|
shared `ZodValidationPipe` (`backend/src/common/pipes/zod-validation.pipe.ts`)
|
|
which throws `ApiError.validation('Request validation failed', details)`
|
|
mapped to HTTP 400 `VALIDATION_FAILED`.
|
|
- Frontend: Angular 17+ standalone components, signal-based state, `OnPush`,
|
|
`ReactiveFormsModule`, pure helper module at
|
|
`frontend/src/app/features/admin/general.pure.ts`. Inline error display
|
|
pattern is already used for the `pageTitle` field
|
|
(`general-pageTitle-error` + `pageTitleMessage(...)` computed signal +
|
|
`showPageTitleError()` computed) — replicate this exact pattern for the
|
|
two date inputs.
|
|
- **Data Layer:** SQLite via `better-sqlite3`. `SettingsService` stores every
|
|
general-settings key as a single string row in the `setting` table. No
|
|
schema migration is required for this job — we are tightening input
|
|
validation, not changing storage.
|
|
- **Test Framework & Structure:**
|
|
- Jest 29 with `ts-jest`, root config at `tests/jest.config.js`. Two
|
|
projects: `backend` (`tests/backend/`) and `frontend`
|
|
(`tests/frontend/`). Tests live ONLY in `/repo/tests/`.
|
|
- Single command `npm test` runs everything.
|
|
- **Required Tools & Dependencies:** None. We use the already-installed
|
|
`zod` (datetime check is built into zod ≥ 3.20) and the existing
|
|
Angular reactive-forms stack. No `setup.sh` changes are required.
|
|
|
|
## 2. Impacted Files
|
|
|
|
- **To Modify:**
|
|
- `backend/src/modules/admin/dto/general.dto.ts` — tighten
|
|
`eventStartUtc` / `eventEndUtc` to require real ISO-8601 datetimes.
|
|
- `frontend/src/app/features/admin/general.component.ts` — add per-field
|
|
datetime error rendering and inline error elements under both inputs.
|
|
- `frontend/src/app/features/admin/general.pure.ts` — add pure helpers
|
|
(`isoDatetimeValidator`, `datetimeMessage`) so the same logic is
|
|
unit-testable in isolation.
|
|
- `tests/backend/admin-general-service.spec.ts` — add negative cases for
|
|
non-datetime payloads.
|
|
- `tests/frontend/admin-general-pure.spec.ts` — add tests for the new
|
|
validators + message helpers.
|
|
- **To Create:** None. All changes fit inside existing files.
|
|
|
|
## 3. Proposed Changes
|
|
|
|
### 3.1 Backend — strict ISO-8601 datetime validation
|
|
|
|
In `backend/src/modules/admin/dto/general.dto.ts`:
|
|
|
|
- Replace the two loose `z.string()` declarations with zod's built-in
|
|
`.datetime({ offset: false })` check (or
|
|
`z.string().datetime({ message: '...' })` with explicit messages), so that
|
|
`GeneralSettingsSchema` rejects non-ISO-8601 strings with a clear
|
|
`INVALID_DATETIME` message before the request reaches
|
|
`SettingsService.set(...)`. This guarantees:
|
|
- The previously valid schedule is unchanged when validation fails (the
|
|
service is never called).
|
|
- The error is surfaced via the standard
|
|
`ApiError.validation('Request validation failed', details)` envelope
|
|
produced by `ZodValidationPipe` (HTTP 400 `VALIDATION_FAILED`).
|
|
- Keep the existing `superRefine` end > start rule; it now runs only on
|
|
values that already passed the per-field datetime check.
|
|
- Empty string handling: the current UI sends `''` when the user clears an
|
|
input, and the current code accepts `''`. To avoid regressing the
|
|
"unconfigured event" state, allow `z.union([z.string().datetime(...), z.literal('')])`
|
|
(an empty string is a valid "no event scheduled" value). Anything that
|
|
is neither empty nor a valid ISO datetime is rejected.
|
|
|
|
### 3.2 Frontend — per-field datetime error rendering
|
|
|
|
In `frontend/src/app/features/admin/general.component.ts`:
|
|
|
|
- Apply `isoDatetimeValidator` (new, see 3.3) as a per-control validator
|
|
on both `eventStartUtc` and `eventEndUtc` (alongside the existing
|
|
group-level `endAfterStartValidator`).
|
|
- For each input, add the `aria-describedby` / `aria-invalid` attributes
|
|
and an inline `<div class="field-error" data-testid="general-eventStart-error">…</div>`
|
|
(and the matching `general-eventEnd-error`) that renders when the
|
|
control is touched/dirty AND invalid, driven by a `computed` signal —
|
|
exactly mirroring the existing `showPageTitleError` /
|
|
`general-pageTitle-error` pattern in this same file.
|
|
- The message text comes from the new `datetimeMessage(value, errors)`
|
|
pure helper (see 3.3). Messages:
|
|
- non-empty value that fails `Date.parse` →
|
|
`"Event start must be a valid ISO-8601 datetime."` (or "Event end").
|
|
- empty value → no message (empty is the intentional "unconfigured"
|
|
state; the `endBeforeStart` group validator already stays quiet when
|
|
one side is empty).
|
|
- The existing `general-endBeforeStart` element stays put; it now
|
|
coexists with the two new per-field error nodes.
|
|
|
|
### 3.3 Frontend pure helpers
|
|
|
|
In `frontend/src/app/features/admin/general.pure.ts`, add:
|
|
|
|
```ts
|
|
export function isoDatetimeValidator(ctrl: { value: string | null }) {
|
|
const v = ctrl?.value ?? '';
|
|
if (v === '') return null; // unconfigured is allowed
|
|
const t = Date.parse(v);
|
|
return Number.isFinite(t) ? null : { invalidDatetime: true };
|
|
}
|
|
|
|
export function datetimeMessage(
|
|
fieldLabel: string,
|
|
value: string | null | undefined,
|
|
controlErrors: { [k: string]: unknown } | null | undefined,
|
|
): string | null {
|
|
if (controlErrors?.['invalidDatetime']) {
|
|
return `${fieldLabel} must be a valid ISO-8601 datetime.`;
|
|
}
|
|
// also catch the case where the user typed something unparseable but
|
|
// the validator did not run yet (defensive)
|
|
const v = value ?? '';
|
|
if (v !== '' && !Number.isFinite(Date.parse(v))) {
|
|
return `${fieldLabel} must be a valid ISO-8601 datetime.`;
|
|
}
|
|
return null;
|
|
}
|
|
```
|
|
|
|
These are exported from the existing pure file so the component can call
|
|
them directly and the spec file can exercise them in isolation.
|
|
|
|
### 3.4 No persistence side-effects on rejection
|
|
|
|
This is already enforced by the existing `ZodValidationPipe` (the
|
|
service is never invoked when validation fails), so no further change is
|
|
needed there.
|
|
|
|
## 4. Test Strategy
|
|
|
|
- **Target Unit Test Files:**
|
|
- `tests/backend/admin-general-service.spec.ts` — extend the existing
|
|
`GeneralSettingsSchema - validation rules` describe block with two
|
|
cases:
|
|
- `eventStartUtc = 'not-a-date'` → schema returns `success: false`.
|
|
- `eventStartUtc = '2026-08-15T10:30:00.000Z'` and
|
|
`eventEndUtc = 'also-not-a-date'` → schema returns
|
|
`success: false`, and the issue message includes `'eventEndUtc'`
|
|
/ contains the word `datetime` (to assert the new error is the
|
|
source of the rejection, not the cross-field rule).
|
|
- `tests/frontend/admin-general-pure.spec.ts` — extend with:
|
|
- `isoDatetimeValidator` returns `null` for empty input.
|
|
- `isoDatetimeValidator` returns `null` for a valid ISO string.
|
|
- `isoDatetimeValidator` returns `{ invalidDatetime: true }` for
|
|
`'not-a-date'`.
|
|
- `datetimeMessage` returns the correct per-field message for
|
|
`'Event start'` / `'Event end'` and returns `null` for empty /
|
|
valid input.
|
|
- **Mocking Strategy:** Pure-function tests need no mocks. The existing
|
|
backend spec already constructs the schema in-process; we just feed
|
|
it bad payloads and inspect `safeParse` results. No DB, no HTTP
|
|
server, no Angular `TestBed` are required.
|
|
- **Command:** `npm test` (or the narrower
|
|
`npm run test:backend` / `npm run test:frontend`).
|