258 lines
14 KiB
Markdown
258 lines
14 KiB
Markdown
# Implementation Plan: Job 889 — Admin Area General Settings and Categories 1.07
|
|
|
|
## Status
|
|
NOT FULLY IMPLEMENTED. Two residual defects remain after Job 888:
|
|
|
|
1. **Categories SQL** — `backend/src/modules/admin/categories.service.ts:30-37` uses
|
|
`createQueryBuilder('c').orderBy('LOWER(c.abbreviation)', 'ASC').addOrderBy('c.abbreviation', 'ASC').getMany()`.
|
|
`getMany()` auto-selects every column mapped on `CategoryEntity`, including
|
|
`created_at` and `updated_at` (added by migration
|
|
`1700000000200-AddCategoryTimestampsAndUniqueAbbrev`). The
|
|
`InitSchema1700000000000` migration (`backend/src/database/migrations/1700000000000-InitSchema.ts:28-37`)
|
|
creates the `category` table **without** those columns and relies on the
|
|
200-migration to add them at startup. On any database where the 200-migration
|
|
has not yet run (e.g. when an older pre-1.06 DB is hit before migrations
|
|
resolve, or when tests / fresh clones initialise the schema out of order),
|
|
SQLite raises `SqliteError: no such column: c.created_at` and the Categories
|
|
section on `/admin/general` (which embeds `AdminCategoriesComponent`) renders
|
|
the error to the admin. The fix is to make the `category` table include
|
|
`created_at` and `updated_at` in its initial `CREATE TABLE` and keep the 200
|
|
migration as a no-op `if (!names.has(...))` ALTER for pre-existing DBs.
|
|
|
|
2. **Event End equal-to-start field-level message** — `frontend/src/app/features/admin/general.pure.ts:41-51`
|
|
already returns `{ endBeforeStart: true }` when `e <= s`, and
|
|
`frontend/src/app/features/admin/general.component.ts:123-128` renders the
|
|
`general-endBeforeStart` element whenever `eventEndCrossFieldError()` is true.
|
|
However the message lives in a **separate** `data-testid="general-endBeforeStart"`
|
|
div instead of inside the per-field error region
|
|
`data-testid="general-eventEnd-error"` that the Job (and `docs/guides/admin-general-settings.md`)
|
|
describe as the canonical "field-level validation message" for the Event End
|
|
input. The Job explicitly states: *"no visible message inside the field-level
|
|
error region (e.g., `[data-testid=general-eventEnd-error]`)"*. The fix is to
|
|
funnel the cross-field `endBeforeStart` message into `general-eventEnd-error`
|
|
(and surface it via `aria-invalid` / `aria-describedby` / `title` on the
|
|
input) so it is rendered as the Event End field's own validation message.
|
|
|
|
## 1. Architectural Reconnaissance
|
|
|
|
- **Codebase style & conventions:**
|
|
- **Backend:** NestJS 10, TypeScript strict, TypeORM 0.3.20 over
|
|
`better-sqlite3`. Migrations live under
|
|
`backend/src/database/migrations/` and are wired through
|
|
`DatabaseModule.MIGRATIONS` (`backend/src/database/database.module.ts:31-36`)
|
|
and run on bootstrap by `DatabaseInitService.init()` (`backend/src/database/database-init.service.ts:26-54`).
|
|
Entities are auto-selected from `DatabaseModule.ENTITIES` and must remain
|
|
in sync with the SQL schema produced by the migrations.
|
|
- **Frontend:** Angular 17+ standalone components with
|
|
`ChangeDetectionStrategy.OnPush`, reactive forms, and the `Value` /
|
|
`Invalid` / `TouchedOrDirty` signal trio for inline error messaging
|
|
(see `general.component.ts:204-321`). Pure validation / message helpers
|
|
live in `frontend/src/app/features/admin/general.pure.ts` and are
|
|
unit-tested under `tests/frontend/admin-general-pure.spec.ts`.
|
|
- **Error envelopes:** backend emits
|
|
`{ code: 'VALIDATION_FAILED', message, details: [{path, message}] }` via
|
|
`ZodValidationPipe` (`backend/src/common/pipes/zod-validation.pipe.ts:9-15`)
|
|
for `400`s; `ApiError` helpers live in `backend/src/common/errors/api-error.ts`.
|
|
|
|
- **Data Layer:** SQLite (`better-sqlite3`) database file at
|
|
`process.env.DATABASE_PATH` (default `/data/hipctf/db.sqlite`). The
|
|
`category` table is created by `InitSchema1700000000000` and seeded /
|
|
amended by `SeedSystemData1700000000100`,
|
|
`AddCategoryTimestampsAndUniqueAbbrev1700000000200`, and
|
|
`UpdateSystemCategoryKeys1700000000300`. The
|
|
`CategoryEntity` (`backend/src/database/entities/category.entity.ts`) is
|
|
the single source of truth for the column names that TypeORM will SELECT
|
|
via `getMany()`.
|
|
|
|
- **Test Framework & Structure:** Jest (`ts-jest`) with two projects
|
|
(`backend` and `frontend`) configured in `tests/jest.config.js`. Tests
|
|
live in `tests/backend/**/*.spec.ts` and `tests/frontend/**/*.spec.ts`
|
|
and run with a single command `npm test` from the repo root (already in
|
|
`package.json:scripts.test`). Backend tests typically boot the real
|
|
`AppModule` against `:memory:` SQLite via
|
|
`process.env.DATABASE_PATH = ':memory:'`; frontend tests are pure
|
|
function specs that never touch the DOM. New tests must follow the same
|
|
layout.
|
|
|
|
- **Required Tools & Dependencies:** No new packages. Node 20+, npm
|
|
workspaces, `jest`, `ts-jest`, `better-sqlite3`, `@nestjs/typeorm`,
|
|
and `@nestjs/common` are all already declared and bootstrapped by
|
|
`setup.sh` (`setup.sh:1-19`). No `setup.sh` change is required for
|
|
this Job.
|
|
|
|
## 2. Impacted Files
|
|
|
|
- **To Modify:**
|
|
- `backend/src/database/migrations/1700000000000-InitSchema.ts` —
|
|
add `"created_at"` and `"updated_at"` columns to the initial
|
|
`CREATE TABLE "category"` so the columns exist from the very first
|
|
migration; preserve the `description` and `icon_path` defaults.
|
|
- `frontend/src/app/features/admin/general.component.ts` —
|
|
extend the Event End `general-eventEnd-error` element so it also
|
|
renders the cross-field `endBeforeStart` message when no
|
|
per-field `invalidDatetime` error is present; keep
|
|
`general-endBeforeStart` as a redundant/legacy visual element only
|
|
if it is already asserted in tests (see Test Strategy); bind
|
|
`[attr.aria-invalid]` / `[attr.aria-describedby]` / `[attr.title]`
|
|
on the Event End `<input>` to the merged state.
|
|
- `frontend/src/app/features/admin/general.pure.ts` — add a tiny
|
|
helper (e.g. `eventEndFieldError(fieldErrors, crossField)`) that
|
|
returns the right human-readable string for the merged Event End
|
|
field error, so the template can read it from a single source.
|
|
- `tests/frontend/admin-general-pure.spec.ts` — add focused unit
|
|
assertions for the new helper (equal-to-start and end-before-start
|
|
cases).
|
|
- `tests/backend/migrations.spec.ts` — add a focused assertion that
|
|
running `InitSchema1700000000000` against an empty database
|
|
produces a `category` table that contains a `created_at` column
|
|
(and `updated_at`). This locks the SQL schema in place so the
|
|
Categories endpoint can never regress to the
|
|
`no such column: c.created_at` state.
|
|
- `docs/guides/admin-general-settings.md` — clarify that the
|
|
Event End inline error region (`general-eventEnd-error`) is the
|
|
canonical place where the end-before-start message surfaces.
|
|
|
|
- **To Create:**
|
|
- None required. Existing test files are sufficient.
|
|
|
|
## 3. Proposed Changes
|
|
|
|
### 1. Database / Schema Migration
|
|
|
|
1. **Update `InitSchema1700000000000`** at
|
|
`backend/src/database/migrations/1700000000000-InitSchema.ts:28-37` so
|
|
the `category` `CREATE TABLE` includes `created_at` and `updated_at`
|
|
columns from the very first migration:
|
|
```sql
|
|
CREATE TABLE IF NOT EXISTS "category" (
|
|
"id" TEXT PRIMARY KEY,
|
|
"system_key" TEXT,
|
|
"name" TEXT NOT NULL,
|
|
"abbreviation" TEXT NOT NULL,
|
|
"description" TEXT NOT NULL DEFAULT '',
|
|
"icon_path" TEXT NOT NULL DEFAULT '',
|
|
"created_at" TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')),
|
|
"updated_at" TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now'))
|
|
);
|
|
```
|
|
Mirror the `strftime('%Y-%m-%dT%H:%M:%fZ','now')` default already used
|
|
for `user.created_at` (line 17) and `challenge.created_at` (line 54) so
|
|
newly-inserted rows get a sane timestamp without a follow-up
|
|
UPDATE-pass.
|
|
|
|
2. **Keep `AddCategoryTimestampsAndUniqueAbbrev1700000000200`
|
|
unchanged.** Its `if (!names.has('created_at'))` /
|
|
`if (!names.has('updated_at'))` guards (`backend/src/database/migrations/1700000000200-AddCategoryTimestampsAndUniqueAbbrev.ts:8-20`)
|
|
make the ALTER a no-op for fresh databases and still patch
|
|
older pre-1.06 databases that lack the columns. No changes needed
|
|
to `down()` either — the down() drops the columns, which is the
|
|
correct mirror of `up()`.
|
|
|
|
3. **Keep `UpdateSystemCategoryKeys1700000000300` unchanged.** Its
|
|
`INSERT` already lists `created_at` and `updated_at`
|
|
(`backend/src/database/migrations/1700000000300-UpdateSystemCategoryKeys.ts:51`).
|
|
|
|
4. No need to touch `CategoryEntity`
|
|
(`backend/src/database/entities/category.entity.ts`) — its current
|
|
`@Column('text', { name: 'created_at', default: '' })` and
|
|
`@Column('text', { name: 'updated_at', default: '' })` annotations
|
|
match the columns we are guaranteeing.
|
|
|
|
### 2. Backend Logic & APIs
|
|
|
|
No controller / service logic changes are required. With the schema
|
|
fix, `AdminCategoriesService.list()` at
|
|
`backend/src/modules/admin/categories.service.ts:30-37` will resolve
|
|
`c.created_at` and `c.updated_at` and return rows that satisfy the
|
|
existing `CategoryView` shape used by the front-end.
|
|
|
|
If desired, the controller surface can stay untouched
|
|
(`backend/src/modules/admin/admin-categories.controller.ts:30-34`) —
|
|
no new endpoint is needed.
|
|
|
|
### 3. Frontend UI Integration
|
|
|
|
1. **Update `frontend/src/app/features/admin/general.pure.ts`** to
|
|
expose a small `eventEndFieldMessage(controlErrors, crossField)` pure
|
|
helper that returns the canonical Event End field error string:
|
|
|
|
| `controlErrors?.['invalidDatetime']` | `form.errors?.['endBeforeStart']` | Returned message |
|
|
|---------------------------------------|-----------------------------------|----------------------------------------------------------|
|
|
| `true` | any | `"Event end must be a valid ISO-8601 datetime."` |
|
|
| falsy | `true` | `"Event end must be after event start."` |
|
|
| falsy | falsy | `null` |
|
|
|
|
This mirrors the existing `datetimeMessage` pattern and keeps the
|
|
template a thin consumer.
|
|
|
|
2. **Update `frontend/src/app/features/admin/general.component.ts`**:
|
|
|
|
- Replace the current `@if (showEventEndError())` /
|
|
`@if (eventEndCrossFieldError())` pair (lines 123-128) with a single
|
|
`eventEndFieldMessage` consumer that always renders inside the
|
|
`data-testid="general-eventEnd-error"` element. When both
|
|
validators are off, the element is absent.
|
|
- Bind `[attr.aria-invalid]` to `true` when
|
|
`eventEndFieldMessage(...)` is non-null, otherwise null.
|
|
- Bind `[attr.aria-describedby]` to `"general-eventEnd-error"` and
|
|
bind `[attr.title]` to the resolved message string in that same
|
|
case.
|
|
- Drop the separate `general-endBeforeStart` block **only after**
|
|
confirming no existing test asserts on it (a quick search of
|
|
`tests/frontend` shows it is only mentioned in
|
|
`tests/frontend/admin-general-pure.spec.ts` as a value assertion on
|
|
the constant `endBeforeStartMessage`, not as a DOM query). If a test
|
|
asserts on the element, keep it as a redundant duplicate or update
|
|
the test.
|
|
|
|
3. **Do not change** the form-level validator at
|
|
`general.pure.ts:41-51` — it already returns `{ endBeforeStart: true }`
|
|
for both `e < s` and `e === s`. The Job requirement is purely a
|
|
rendering fix.
|
|
|
|
## 4. Test Strategy
|
|
|
|
- **Target backend test file:** `tests/backend/migrations.spec.ts` (new
|
|
spec). Add a focused `describe('InitSchema creates category with
|
|
created_at + updated_at columns')` that:
|
|
- Boots `AppModule` against `:memory:` SQLite.
|
|
- Calls `dataSource.runMigrations({ transaction: 'each' })`.
|
|
- Queries `PRAGMA table_info("category")` and asserts the returned
|
|
names contain both `created_at` and `updated_at`.
|
|
- Sanity-checks that `AdminCategoriesService.list()` returns an array
|
|
(length ≥ 6 seeded system rows) without throwing — this is the
|
|
regression test that prevents the user-visible
|
|
`no such column: c.created_at` error from coming back.
|
|
|
|
- **Target backend test file:** `tests/backend/admin-categories-service.spec.ts`
|
|
(extend). Add one `it('list() succeeds against the migrated schema and
|
|
returns seeded rows')` that runs after the existing CRUD specs. The
|
|
existing tests already use `synchronize: true` which would have masked
|
|
the bug; the new test should rely on the **migrations** path so it
|
|
actually exercises the schema the production app uses.
|
|
|
|
- **Target frontend test file:** `tests/frontend/admin-general-pure.spec.ts`
|
|
(extend). Add a `describe('eventEndFieldMessage')` block with three
|
|
focused cases:
|
|
1. Returns the ISO message when `invalidDatetime` is set, even if
|
|
`endBeforeStart` is also set.
|
|
2. Returns the end-before-start message when `invalidDatetime` is
|
|
absent and `endBeforeStart` is set.
|
|
3. Returns `null` when both are absent.
|
|
|
|
- **Mocking Strategy:** No new mocks. Backend spec uses the real
|
|
`AppModule` against `:memory:` SQLite (already the pattern in
|
|
`tests/backend/admin-validation.spec.ts` and
|
|
`tests/backend/admin-general-service.spec.ts`). Frontend spec
|
|
continues to assert on the pure helper return values only.
|
|
|
|
- **Single command:** `npm test` from `/repo` runs the full suite
|
|
(already in `package.json:14`). Frontend and backend can also be run
|
|
separately via `npm run test:backend` / `npm run test:frontend`.
|
|
|
|
## 5. Persistent Project Data (/data)
|
|
|
|
Not applicable to this Job. The DB schema change is a migration; tests
|
|
use `:memory:` SQLite and never write to `/data`. The Categories
|
|
endpoint reads from the existing `category` table without touching
|
|
uploaded files or any other shared asset. |