feat(forms): form definitions with per-field validation #497

Merged

longjacksonle merged 5 commits from feat/PMS-731-form-definitions into main 2026-08-06 20:32:19 +02:00

Conversation 0 Commits 5 Files changed 10 +2253 -1

longjacksonle commented 2026-08-06 20:25:27 +02:00

Owner

Copy link

Implements PMS-731: form definitions with per-field validation, the substrate the PMS-730 MACD request flow consumes. Built in mokosh rather than extracted from eForm, per the decision recorded on the issue: eForm's whole form-definition surface is a three-field struct with no required flag and no rules, stored as an opaque JSON blob in a column added by an error-swallowing ALTER, and its validation is one hardcoded "email is required". There was nothing to extract.

Scope is bounded by the MACD field list reviewed on the issue, so every type and rule here traces to a line in that list.

What is here

• migrations/100_form_definitions.sql: form_definitions, form_fields, form_submissions, each with its fail-closed tenant_isolation policy attached explicitly.
• A runtime validator (src/modules/forms/validation.rs) interpreting the stored rule set, reporting failures as AppError::Validation with a Vec<FieldError>.
• Service + routes under /api/v1/forms: definition CRUD, submit, list submissions.
• 13 unit tests on the validator, 6 integration tests over HTTP.

Field types: text, textarea, email, date, select, boolean. Rules: required, length, email pattern, date-not-in-past, option-set membership.

Decisions worth reviewing

Numeric range and file upload are deliberately absent. The issue's proposed approach listed numeric range; nothing in the MACD set has a number field at all, and no field wants an attachment. A rule with no caller is a liability rather than an affordance, and both are one migration away when a form needs them.

The validator crate could not carry the per-field rules, and the issue's assumption that it would is corrected here. validator 0.19 is derive-based, so #[derive(Validate)] expands rules known at compile time, while these rules are rows a tenant authored at runtime. What is reused is the error vocabulary: AppError::Validation carrying Vec<FieldError> is the exact wire shape derive-based request validation already produces, so a client renders a form error and a DTO error identically. Note also that validator::ValidationErrors::add takes &'static str and cannot carry a runtime field name without leaking, which is why FieldError is the right target rather than the crate's own error type.

Conditional requiredness is one form-level rule, not a condition engine. The MACD departure form needs forward_to only when mailbox_handling = forward. form_definitions.rules holds a rule list with exactly one kind, required_if, which buys that single behaviour. Conditional display stays out of scope.

Submissions are a JSONB payload on a tenant-scoped row, not a row per value. The issue asked for relational storage "so RLS covers them", but a policy attaches to the table via tenant_id and is indifferent to the payload's shape. Nothing in PMS-730's acceptance criteria queries across submissions by field value; normalise later if PMS-732 reporting actually needs it.

The change-type to article mapping is a column here, not a table. form_definitions.kb_article_id is PMS-730's "KB article selected by the requested change type". This row IS the request-type vocabulary, so the mapping is a column rather than a join, and a standalone table would have duplicated the vocabulary. A ticket created from a submission copies it into tickets.procedure_kb_article_id (migration 099, merged in #495).

Behaviour that is easy to get wrong, and is pinned by tests

• A string that is empty after trimming counts as ABSENT: a required field answered with whitespace reports "required" rather than storing a blank, and an optional one is omitted rather than persisting noise.
• A boolean answered false IS an answer. Treating it as absent would make a required checkbox impossible to say "no" to.
• Unknown payload keys are rejected, not dropped. A typo'd key would otherwise leave the request worked from incomplete data, which is the round-trip cost PMS-730 exists to remove.
• Every error is collected before returning, so a submission missing three required fields reports all three.
• A PATCH carrying fields replaces the set rather than merging: field identity is the payload key, and a merge cannot express a rename or deletion unambiguously. Submissions keep their own stored payload, so this does not rewrite history.
• A definition with submissions refuses deletion (409); is_active = false retires it, and a retired definition refuses new submissions rather than accepting them silently.
• Author errors are reported in the same per-field shape a client's errors use: duplicate field name, select with no options, inverted length bounds, a rule naming an unknown field, and a required_if whose equals is not an option of the field it reads.

RLS

All three tables attach ENABLE + FORCE + the NULLIF'd GUC comparison explicitly, because the DO-block loops in migrations 024 and 038 have already run and a table created now inherits no policy at all. tests/rls_coverage.rs verifies this generically and passes with the allowlist still empty. Every serving query goes through Database::begin_with_tenant; there is no pre-auth path in this PR, since the public magic-link submission arrives with PMS-730 and will resolve its tenant from the token before calling in.

Verification

just test-integration green end to end (23 test binaries), plus cargo fmt --all --check, cargo clippy --all-targets -- -D warnings, and all six guard scripts (check-migration-prefixes, check-migration-immutability, check-pool-safety, check-no-duplicate-mail-copy, check-runner-labels, check-oci-build-cache).

Open questions from the field-list review

Still unanswered on the issue and NOT blocking this PR, because the four MACD forms are data rather than code: whether moves are actually requested this way, whether department needs a per-client option set, and whether a request ever covers more than one person. The last one is the only one that would reshape the model, since a bulk onboarding is a repeat group.

Note on CI

The Playwright job fails on staging for a reason unrelated to this branch: the bunyip hub rejects the E2E account's TOTP at /login/2fa, which reproduces on main and on every open PR. Diagnosis and suggested checks are on #496.

Implements PMS-731: form definitions with per-field validation, the substrate the PMS-730 MACD request flow consumes. Built in mokosh rather than extracted from eForm, per the decision recorded on the issue: eForm's whole form-definition surface is a three-field struct with no required flag and no rules, stored as an opaque JSON blob in a column added by an error-swallowing ALTER, and its validation is one hardcoded "email is required". There was nothing to extract. Scope is bounded by the MACD field list reviewed on the issue, so every type and rule here traces to a line in that list. ## What is here - `migrations/100_form_definitions.sql`: `form_definitions`, `form_fields`, `form_submissions`, each with its fail-closed `tenant_isolation` policy attached explicitly. - A runtime validator (`src/modules/forms/validation.rs`) interpreting the stored rule set, reporting failures as `AppError::Validation` with a `Vec<FieldError>`. - Service + routes under `/api/v1/forms`: definition CRUD, submit, list submissions. - 13 unit tests on the validator, 6 integration tests over HTTP. Field types: text, textarea, email, date, select, boolean. Rules: required, length, email pattern, date-not-in-past, option-set membership. ## Decisions worth reviewing **Numeric range and file upload are deliberately absent.** The issue's proposed approach listed numeric range; nothing in the MACD set has a number field at all, and no field wants an attachment. A rule with no caller is a liability rather than an affordance, and both are one migration away when a form needs them. **The `validator` crate could not carry the per-field rules, and the issue's assumption that it would is corrected here.** `validator` 0.19 is derive-based, so `#[derive(Validate)]` expands rules known at compile time, while these rules are rows a tenant authored at runtime. What is reused is the error vocabulary: `AppError::Validation` carrying `Vec<FieldError>` is the exact wire shape derive-based request validation already produces, so a client renders a form error and a DTO error identically. Note also that `validator::ValidationErrors::add` takes `&'static str` and cannot carry a runtime field name without leaking, which is why `FieldError` is the right target rather than the crate's own error type. **Conditional requiredness is one form-level rule, not a condition engine.** The MACD departure form needs `forward_to` only when `mailbox_handling = forward`. `form_definitions.rules` holds a rule list with exactly one kind, `required_if`, which buys that single behaviour. Conditional display stays out of scope. **Submissions are a JSONB payload on a tenant-scoped row, not a row per value.** The issue asked for relational storage "so RLS covers them", but a policy attaches to the table via `tenant_id` and is indifferent to the payload's shape. Nothing in PMS-730's acceptance criteria queries across submissions by field value; normalise later if PMS-732 reporting actually needs it. **The change-type to article mapping is a column here, not a table.** `form_definitions.kb_article_id` is PMS-730's "KB article selected by the requested change type". This row IS the request-type vocabulary, so the mapping is a column rather than a join, and a standalone table would have duplicated the vocabulary. A ticket created from a submission copies it into `tickets.procedure_kb_article_id` (migration 099, merged in #495). ## Behaviour that is easy to get wrong, and is pinned by tests - A string that is empty after trimming counts as ABSENT: a required field answered with whitespace reports "required" rather than storing a blank, and an optional one is omitted rather than persisting noise. - A boolean answered `false` IS an answer. Treating it as absent would make a required checkbox impossible to say "no" to. - Unknown payload keys are rejected, not dropped. A typo'd key would otherwise leave the request worked from incomplete data, which is the round-trip cost PMS-730 exists to remove. - Every error is collected before returning, so a submission missing three required fields reports all three. - A PATCH carrying `fields` replaces the set rather than merging: field identity is the payload key, and a merge cannot express a rename or deletion unambiguously. Submissions keep their own stored payload, so this does not rewrite history. - A definition with submissions refuses deletion (409); `is_active = false` retires it, and a retired definition refuses new submissions rather than accepting them silently. - Author errors are reported in the same per-field shape a client's errors use: duplicate field name, select with no options, inverted length bounds, a rule naming an unknown field, and a `required_if` whose `equals` is not an option of the field it reads. ## RLS All three tables attach `ENABLE` + `FORCE` + the `NULLIF`'d GUC comparison explicitly, because the DO-block loops in migrations 024 and 038 have already run and a table created now inherits no policy at all. `tests/rls_coverage.rs` verifies this generically and passes with the allowlist still empty. Every serving query goes through `Database::begin_with_tenant`; there is no pre-auth path in this PR, since the public magic-link submission arrives with PMS-730 and will resolve its tenant from the token before calling in. ## Verification `just test-integration` green end to end (23 test binaries), plus `cargo fmt --all --check`, `cargo clippy --all-targets -- -D warnings`, and all six guard scripts (`check-migration-prefixes`, `check-migration-immutability`, `check-pool-safety`, `check-no-duplicate-mail-copy`, `check-runner-labels`, `check-oci-build-cache`). ## Open questions from the field-list review Still unanswered on the issue and NOT blocking this PR, because the four MACD forms are data rather than code: whether moves are actually requested this way, whether `department` needs a per-client option set, and whether a request ever covers more than one person. The last one is the only one that would reshape the model, since a bulk onboarding is a repeat group. ## Note on CI The Playwright job fails on staging for a reason unrelated to this branch: the bunyip hub rejects the E2E account's TOTP at `/login/2fa`, which reproduces on `main` and on every open PR. Diagnosis and suggested checks are on #496.

longjacksonle added 5 commits 2026-08-06 20:25:27 +02:00

feat(forms): add form definition, field and submission tables ... e961c651e8

PMS-731: the substrate the PMS-730 MACD request flow consumes. A form definition owns an ordered field set, each field carrying its type, label, required flag and validation bounds; submissions are stored tenant-scoped against the definition they answered.

Scope is bounded by the MACD field list reviewed on the issue. The field types (text, textarea, email, date, select, boolean) and the validation rules (required, length, email pattern, date-not-in-past, option-set membership) are exactly those that list needs. Numeric range and file upload are deliberately absent: nothing in the MACD set uses either, and a rule with no caller is a liability rather than an affordance.

`form_definitions.rules` carries cross-field rules, with one kind in v1: `required_if`. The MACD departure form needs `forward_to` only when `mailbox_handling = forward`, which is conditional REQUIREDNESS. One form-level rule list buys that single behaviour without a per-field condition engine, and conditional display stays out of scope.

`form_definitions.kb_article_id` is PMS-730's "KB article selected by the requested change type". The mapping is a column here rather than a separate change-type table because this row IS the request-type vocabulary: one definition per request type.

Submissions hold `ON DELETE RESTRICT` on their definition, so a definition that has ever been submitted cannot be deleted; `is_active = false` retires it instead. A submission is a record of something a client asked for and must outlive the form's retirement.

RLS is attached explicitly on all three tables. The DO-block loops in migrations 024 and 038 have already run, so a table created now inherits no policy at all; the shape mirrors 042_portal_setup_tokens.sql (ENABLE + FORCE + a NULLIF'd GUC comparison that fail-closes when the GUC is unset).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011myibMMwyb6za3GVWJGkiX

feat(forms): add the runtime submission validator ... fdba879f60

The part that does not exist in eForm, whose entire validation surface is one hardcoded "email is required", and the part the `validator` crate cannot supply either: `#[derive(Validate)]` expands rules known at COMPILE time, while these rules are rows a tenant authored at RUNTIME. So the rule set is interpreted.

What is reused is the error vocabulary. Failures are reported as `AppError::Validation` carrying a `Vec<FieldError>`, the exact wire shape the derive-based request validation already produces, so a client renders a form error and a DTO error identically. Note that `validator::ValidationErrors::add` takes `&'static str` and therefore cannot carry a runtime field name without leaking, which is why `FieldError` is the right target rather than the crate's own error type.

Every error is collected before returning, so a submission missing three required fields reports all three rather than the first.

Two behaviours worth calling out because they are easy to get wrong. A string that is empty after trimming is treated as ABSENT, so a required field answered with whitespace reports "required" instead of storing a blank, and an optional one is omitted rather than persisting noise. A boolean answered `false` IS an answer, because treating it as absent would make a required checkbox impossible to say "no" to.

Unknown payload keys are rejected rather than silently dropped: a typo'd key would otherwise leave the request worked from incomplete data, which is the exact round-trip cost PMS-730 exists to remove.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011myibMMwyb6za3GVWJGkiX

feat(forms): serve form definition CRUD and validated submissions ... c06f370f76

Mounts the module under /api/v1: list, get, create, update and delete a definition, plus submit to one and list its submissions.

PMS-683 posture throughout: every query runs inside `Database::begin_with_tenant`, which sets the `app.current_tenant` GUC transaction-locally, so the three tables are safe under their fail-closed policies. There is no pre-auth path here; the public magic-link submission arrives with PMS-730 and will resolve its tenant from the token before calling in. `TenantTransaction` is exported from the db module so the field helpers can take the tenant-bound transaction rather than a raw one.

Authoring a definition is admin-gated, matching ticket templates and workflow rules, because it is tenant-wide configuration. Reading a form and submitting to it are open to any authenticated agent, since filling a request on a client's behalf is a normal path.

A PATCH that includes `fields` REPLACES the field set rather than merging it. Field identity is the payload key, and a merge cannot express a rename or a deletion unambiguously. Submissions already taken keep their own stored payload, so rewriting a field set does not rewrite history.

Definition-time checks that the database cannot express as a single constraint are reported in the same per-field shape a bad submission gets, so an author sees the same errors a client would: a duplicate field name, a select with no options, inverted length bounds, a rule naming a field the form does not have, and a `required_if` whose `equals` is not an option of the field it reads. Each of those would otherwise produce a rule that can never fire, or a form that can never be satisfied.

Deleting a definition with submissions is a 409 rather than a raw FK error, and a retired definition refuses new submissions rather than accepting them silently.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011myibMMwyb6za3GVWJGkiX

test(forms): cover the definition round trip and per-field rejection ... 34d6c297c1

Exercises the three acceptance criteria over HTTP using the MACD departure form from the field-list review: a definition round-trips with its ordered field set, option set and cross-field rule intact; a submission is rejected with per-field errors; and a valid one is stored.

The rejection test asserts the whole error set at once rather than the first failure, including the conditional case where `forward_to` is required only because `mailbox_handling` is `forward`, and confirms nothing was persisted by any rejected attempt.

Also pinned: answers are trimmed before storage and a blank optional is omitted rather than stored empty; a form with submissions refuses deletion; a retired form refuses new submissions, or the flag would be decorative; and authoring stays admin-only while submitting does not.

Per-rule behaviour of the validator itself is unit-tested next to the interpreter, so this file covers the wiring, the persisted result, and the surfaces unit tests cannot reach. RLS coverage for the three new tables is asserted generically by tests/rls_coverage.rs, which fails any tenant-scoped table lacking a policy, so it is not duplicated here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011myibMMwyb6za3GVWJGkiX

fix(forms): reject an update that strands a rule on a removed field ... f7331926c9

Self-review catch. The update path only checked rules when the request carried `rules`, so a PATCH that replaced the field set while leaving the rules alone could drop a field a `required_if` targets. The rule survived in the column pointing at a name no field had, and the interpreter treats such a rule as inert, so the conditional requiredness would have stopped applying with nothing reported to the author.

The check now runs whenever EITHER half changes, evaluating whichever rule set will be in force against whichever field set will be in force. Dropping a field and the rule that targets it in the same request stays legitimate and is covered by the test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011myibMMwyb6za3GVWJGkiX

longjacksonle scheduled this pull request to auto merge when all checks succeed 2026-08-06 20:26:36 +02:00

longjacksonle merged commit a91713a7a2 into main 2026-08-06 20:32:19 +02:00

longjacksonle deleted branch feat/PMS-731-form-definitions 2026-08-06 20:32:19 +02:00

longjacksonle referenced this pull request from a commit 2026-08-06 20:32:20 +02:00

Merge pull request 'feat(forms): form definitions with per-field validation' (#497) from feat/PMS-731-form-definitions into main

longjacksonle referenced this pull request 2026-08-06 20:51:17 +02:00

feat(forms): client request forms (MACD) by magic link #498

longjacksonle referenced this pull request 2026-08-06 21:03:38 +02:00

feat(reports): measured duration per client-request type #499

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

psa-systems/mokosh-server!497

No description provided.