148 lines
13 KiB
Markdown
148 lines
13 KiB
Markdown
# Implementation Plan: Job 845 — First-Start Create Admin Modal 1.04 (surface field validation)
|
||
|
||
## 1. Architectural Reconnaissance
|
||
|
||
- **Codebase style & conventions:**
|
||
- NestJS API + Angular 17 SPA monorepo (workspaces in `package.json`).
|
||
- Frontend: **Standalone components**, `ChangeDetectionStrategy.OnPush`, Angular **signals** (`signal`/`computed`), no NgModules. Style guide mandates `OnPush` and signals for state.
|
||
- Reactive Forms via `ReactiveFormsModule` with `fb.nonNullable.group(...)` (non-nullable typed controls).
|
||
- All services are `providedIn: 'root'`. Two HTTP interceptors wired in `frontend/src/main.ts` (`csrfInterceptor`, then `authInterceptor`).
|
||
- Frontend tests are **pure-logic / no TestBed**: `tests/frontend/**/*.spec.ts` import pure functions (validators, session-storage helpers, shell decision helpers) and rely on `jest` (`testEnvironment: 'jsdom'`) runnable with the root command `npm test` (which uses `jest --config tests/jest.config.js --selectProjects frontend`).
|
||
- Strictly-typed reactive forms per `angular-forms` skill (`FormGroup<T>`, `fb.nonNullable`).
|
||
- Data attributes / `data-testid` selectors are the stable DOM hooks (see `docs/guides/bootstrap.md`).
|
||
- **Data Layer:** N/A for this Job — purely a frontend UI/validation correctness fix. The backend already returns a structured `400 VALIDATION_FAILED` envelope; the bug is solely that the SPA short-circuits on `form.invalid` without ever surfacing per-field error messages.
|
||
- **Test Framework & Structure:**
|
||
- Jest with `ts-jest`, jsdom environment for frontend.
|
||
- Tests live **only** in `tests/frontend/` (never co-located with source).
|
||
- Root command: `npm test` (runs all projects via `jest --config tests/jest.config.js`).
|
||
- Existing precedent at `tests/frontend/setup-create-admin.spec.ts` is a pure-logic suite (mirrors component logic in standalone helper functions). **We will follow that pattern** — no TestBed, no DOM rendering required, no visual confirmation.
|
||
- **Required Tools & Dependencies:** No new dependencies, system tools, or `setup.sh` changes needed. The fix is a localized source-code change in the standalone component + its template + a pure-helper module that mirrors the per-field error messages (for parity testing). The npm workspace already has `@angular/forms ^17.3` and `jest`.
|
||
|
||
## 2. Impacted Files
|
||
|
||
### To Modify
|
||
- **`frontend/src/app/features/setup/setup-create-admin.component.ts`** — fix the reactivity of the per-field error calculations and expose `passwordErrors()`. Convert the `computed(...)`s whose bodies read `FormControl` properties (which are NOT signals in Angular 17) into plain **methods** so the template re-evaluates them on every change-detection cycle (cheap, OnPush-friendly, no extra subscriptions). Alternatively keep them as computed but reflect `touched` into a local `signal()` driven by `statusChanges`/`valueChanges` + an effect; **plain methods are the minimal, idiomatic fix** and are used elsewhere in the same component style. Keep all existing public surface (`form`, `submitting`, `serverError`, `serverErrorCode`, `canRetry`, `policyDescription`, `submit`, `swallowBackdrop`, `swallowEscape`) intact.
|
||
- **`frontend/src/app/features/setup/setup-create-admin.component.html`** — render the new Password field-error block, mirror `aria-invalid` on the Password and Confirm Password inputs (Username already has it), and keep the `data-testid` hooks the bootstrap guide documents (`setup-username-error`, add `setup-password-error`, keep `setup-confirm-error`).
|
||
- **`frontend/src/app/features/setup/setup-create-admin.component.css`** — no functional change required; `.field-error` styling already exists.
|
||
- **`tests/frontend/setup-create-admin.spec.ts`** — extend the existing pure-logic suite to cover the new password-error helper and to assert that, given an all-empty (or partially-filled) post-submit state, the per-field error strings the template renders are correct.
|
||
|
||
### To Create
|
||
- **`frontend/src/app/features/setup/setup-create-admin.field-errors.ts`** — pure helper functions that return the per-field error string (or `null`) for a given `(value, touched)` snapshot. Keeping them as pure functions (no Angular DI, no signals) is deliberate: it lets the component call them from methods **and** lets the pure-logic test suite assert them without spinning up Angular. Mirrors the pattern already used by `setup-create-admin.validators.ts` and tested in `setup-create-admin.spec.ts`.
|
||
|
||
## 3. Proposed Changes
|
||
|
||
### Root cause
|
||
Angular 17 `FormControl.touched`, `.valid`, and `.errors` are plain class properties, **not** signals. The existing `computed(() => … usernameErrors …)` (component file lines 70–78 and 80–87) reads those plain properties and so registers **zero signal dependencies**. Even after `submit()` calls `form.markAllAsTouched()`, the computed is not re-evaluated — its cached `null` is returned and the `@if (usernameErrors()) { … }` block stays hidden. The same applies to `confirmErrors()` for the `passwordConfirm` control. There is also no `passwordErrors()` at all and no `aria-invalid` binding on the Password/Confirm inputs, which matches the user's observation.
|
||
|
||
### 3.1 New pure helper module
|
||
|
||
`frontend/src/app/features/setup/setup-create-admin.field-errors.ts`:
|
||
|
||
```ts
|
||
export type FieldErrorKey =
|
||
| 'required'
|
||
| 'minlength'
|
||
| 'maxlength'
|
||
| 'pattern'
|
||
| 'passwordMismatch';
|
||
|
||
export interface FieldErrorContext {
|
||
value: string;
|
||
touched: boolean;
|
||
errors: Partial<Record<FieldErrorKey, true>> | null;
|
||
groupMismatch?: boolean;
|
||
}
|
||
|
||
export function usernameError(ctx: FieldErrorContext): string | null { … }
|
||
export function passwordError(ctx: FieldErrorContext): string | null { … } // currently required-only; password POLICY is server-enforced
|
||
export function confirmError(ctx: FieldErrorContext): string | null { … }
|
||
```
|
||
|
||
Rules encoded (single source of truth, identical to current component intent):
|
||
|
||
- **Username:**
|
||
- not touched or no errors → `null`
|
||
- `required` → `'Username is required'`
|
||
- `minlength` → `'Username must be at least 3 characters'`
|
||
- `maxlength` → `'Username must be at most 32 characters'`
|
||
- `pattern` → `'Username may only contain letters, digits, dot, dash and underscore'`
|
||
- else → `'Invalid username'`
|
||
- **Password:**
|
||
- not touched or no errors → `null`
|
||
- `required` → `'Password is required'`
|
||
- else → `null` (policy is intentionally server-side only; this component never surfaces a "weak password" field error pre-submit)
|
||
- **Confirm:**
|
||
- not touched and no `passwordMismatch` group error → `null`
|
||
- `required` → `'Please confirm your password'`
|
||
- `passwordMismatch` (group-level) → `'Passwords do not match'`
|
||
- else → `'Invalid value'`
|
||
|
||
These signatures accept the small slice of state they need so the component can pass `c.value`, `c.touched`, and `c.errors` (cast through `Partial<Record<…, true>>`) and so the test suite can call them directly without touching Angular.
|
||
|
||
### 3.2 Component changes
|
||
|
||
`setup-create-admin.component.ts`:
|
||
|
||
1. **Replace the two `computed(...)`s with plain methods** so the template re-renders them on every CD pass:
|
||
- `usernameError(): string | null { return usernameError({ value: this.form.controls.username.value, touched: this.form.controls.username.touched, errors: this.form.controls.username.errors }); }`
|
||
- `passwordError(): string | null { return passwordError({ value: this.form.controls.password.value, touched: this.form.controls.password.touched, errors: this.form.controls.password.errors }); }` ← **new**
|
||
- `confirmError(): string | null { return confirmError({ value: this.form.controls.passwordConfirm.value, touched: this.form.controls.passwordConfirm.touched, errors: this.form.controls.passwordConfirm.errors, groupMismatch: !!this.form.errors?.['passwordMismatch'] }); }`
|
||
2. **Delete the orphaned `usernameErrors` and `confirmErrors` signal properties** (template rewires to methods).
|
||
3. Leave `submit()` exactly as-is: `markAllAsTouched()` then early-return on invalid. The early return is now correct and observable (because `…Error()` is re-evaluated by the next CD cycle that runs after `markAllAsTouched` synchronously mutates `touched` on every control). No new `effect()` needed; this is the minimal fix.
|
||
4. Keep OnPush (`ChangeDetectionStrategy.OnPush`), keep all existing imports/decorators/HostListener for `escape` and `swallowBackdrop`. The host binding keyboard handler stays so the modal remains non-dismissible.
|
||
|
||
> Optional, NOT recommended for this Job (kept out of scope to honor "minimal changes"): a reactive alternative would be to subscribe to `statusChanges` / `valueChanges` via `toSignal()` in the constructor and gate rendering on those signals. The plain-method approach above is the minimum-surface fix that fully resolves the bug and matches the component's current style.
|
||
|
||
### 3.3 Template changes
|
||
|
||
`setup-create-admin.component.html`:
|
||
|
||
1. Username `<input>` — keep existing `[attr.aria-invalid]="!!usernameError()"` and existing `<small class="field-error">` block, but rebind to the new method name.
|
||
2. Password `<label class="field">` — add `aria-invalid` and add the missing error block below the policy hint:
|
||
```html
|
||
<input … formControlName="password"
|
||
[attr.aria-invalid]="!!passwordError()"
|
||
data-testid="setup-password" />
|
||
<small class="hint">{{ policyDescription() }}</small>
|
||
@if (passwordError()) {
|
||
<small class="field-error" data-testid="setup-password-error">{{ passwordError() }}</small>
|
||
}
|
||
```
|
||
3. Confirm `<input>` — add `aria-invalid`:
|
||
```html
|
||
<input … formControlName="passwordConfirm"
|
||
[attr.aria-invalid]="!!confirmError()"
|
||
data-testid="setup-password-confirm" />
|
||
@if (confirmError()) {
|
||
<small class="field-error" data-testid="setup-confirm-error">{{ confirmError() }}</small>
|
||
}
|
||
```
|
||
4. Submit button — **unchanged**. The Job accepts either "visible per-field error messages" **or** "disabled submit button until valid" — we keep the click-to-trigger-feedback behavior (already used by `markAllAsTouched`) and now surface real errors, which is the original design intent. We deliberately do **not** disable the button on invalid (preserves parity with the existing guide which says the button is "disabled only while submitting").
|
||
|
||
### 3.4 CSS
|
||
|
||
No changes. `.field-error` color/size is already defined.
|
||
|
||
### 3.5 No backend changes
|
||
|
||
The backend's `SetupController.createAdmin` already responds with `400 VALIDATION_FAILED` and a `details` array; that contract is unchanged. The client never reached it before, and after the fix the client still won't for an empty form (which is correct — the per-field client-side errors are now visible; the server is reachable when the client actually submits valid input).
|
||
|
||
## 4. Test Strategy
|
||
|
||
- **Target Unit Test File:** `tests/frontend/setup-create-admin.spec.ts` (extend the existing pure-logic file; do **not** create a co-located spec under `frontend/`).
|
||
- The existing file already exports nothing new — it lives entirely in `tests/frontend/` per project convention and runs with the root command `npm test` (Jest, jsdom).
|
||
- **Mocking Strategy:**
|
||
- **No TestBed, no HTTP, no DOM rendering.** The new helpers (`usernameError`, `passwordError`, `confirmError`) are pure functions in `setup-create-admin.field-errors.ts` — they are imported and called directly with hand-built `FieldErrorContext` literals, mirroring the existing `usernameErrors`/`isFormReadyForSubmit` helpers in the same file.
|
||
- The existing `passwordMatchValidator` tests and username rules act as the de-facto model; we extend the "Composed submit gate" suite to also assert the rendered error strings.
|
||
- **New test cases (≤ 8, focused on the bug paths):**
|
||
1. `usernameError({ value: '', touched: true, errors: { required: true } })` returns `'Username is required'`.
|
||
2. `usernameError({ value: 'ab', touched: true, errors: { minlength: true } })` returns the minlength message.
|
||
3. `passwordError({ value: '', touched: true, errors: { required: true } })` returns `'Password is required'`.
|
||
4. `passwordError({ value: 'x', touched: true, errors: null })` returns `null` (policy is server-side only; no pre-submit hint).
|
||
5. `confirmError({ value: '', touched: true, errors: { required: true } })` returns `'Please confirm your password'`.
|
||
6. `confirmError({ value: 'a', touched: true, errors: null, groupMismatch: true })` returns `'Passwords do not match'`.
|
||
7. `confirmError({ value: 'a', touched: false, errors: null })` returns `null` (no spurious visibility pre-touch).
|
||
8. `usernameError({ value: '', touched: false, errors: { required: true } })` returns `null` (no spurious visibility pre-touch).
|
||
- **How to run:** `npm test` (root). Selectors: `npm run test:frontend` for fast feedback. No browser, no screenshot, no Visual regression.
|
||
- **Out of scope (explicitly):** TestBed rendering, HTTP mock for `SetupCreateAdminService.create`, fixture-based aria assertion, and end-to-end visual confirmation — all forbidden by the test rules.
|