From 5fe4b417bfe6c4fc879b97541fa79c4538bf1c15 Mon Sep 17 00:00:00 2001 From: mAi Date: Thu, 7 May 2026 20:26:49 +0200 Subject: [PATCH] =?UTF-8?q?refactor:=20server-side=20wiring=20through=20th?= =?UTF-8?q?e=20questions=20registry=20=E2=80=94=20closes=20the=20gap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flips three callers from inline per-type strips to registry dispatch. THE main payoff: the date_ranked_choice required-validation gap that the audit doc flagged is now closed by construction. submit/+server.ts: - Replace the inline empty-check loop with `getQuestion(q.type).isAnswerEmpty(q, body.answers[q.id])`. - The legacy rule matched only string/array empties; date_ranked_choice's `{}` answer (object exists but no options rated) passed even when the question was required. ONE source of truth, two callers (client + server), no drift possible. results.ts: - aggregateResults walks the form definition and routes each question through `getQuestion(q.type).{emptyStats, ingest, finalise}`. - All seven per-type emptyStats / ingest / finalise functions deleted (~150 lines). - publicResults delegates to each module's sanitizeForPublic. The hardcoded text-strip rule moves into short_text/long_text modules. - ScaleStatsWip / DateRankedOptionStatsWip private types deleted — each module now owns its own accumulator shape. export/+server.ts: - Replace the per-type CSV column expansion (the date_ranked_choice one-column-per-option special case + the otherwise-one-column-per-question fallthrough) with a uniform `getQuestion(q.type).csvColumns(q)` walk. - Replace the per-cell answer extraction with `getQuestion(q.type).csvCellFor(q, answer, col)`. schemas.ts: - Per-question-type schemas deleted (~40 lines). FeedbackQuestionSchema imports each module's schema directly and assembles the discriminated union. Adding a new type means: create lib/questions/.ts + register in registry.ts + add one import + one tuple entry here. (Kept the explicit tuple because TypeScript can't infer a properly- narrowed discriminated union from a runtime-built array — the inferred FeedbackQuestion type would lose specificity at every call site.) 123 server tests pass — all existing assertions still hold against the registry-routed aggregator. svelte-check + bun run build clean. Client-side wiring (FormBuilder, participant page, Results.svelte, admin detail submissions table) lands in the next commit. --- src/lib/schemas.ts | 83 +++---- src/lib/server/results.ts | 216 +++--------------- .../api/admin/feedback/[id]/export/+server.ts | 27 +-- .../public/feedback/[slug]/submit/+server.ts | 16 +- 4 files changed, 83 insertions(+), 259 deletions(-) diff --git a/src/lib/schemas.ts b/src/lib/schemas.ts index d1001f9..133dcf5 100644 --- a/src/lib/schemas.ts +++ b/src/lib/schemas.ts @@ -1,66 +1,37 @@ /** * Zod schemas for fdbck request body validation. + * + * The per-question-type schemas now live in `lib/questions/.ts`. This + * file imports them directly and assembles the discriminated union — adding + * a new question type means creating one file under `lib/questions/`, + * registering it in `lib/questions/registry.ts`, AND adding one line here + * (TypeScript's discriminated-union inference can't pick up a runtime-built + * array, so the tuple stays explicit to keep `FeedbackQuestion` properly + * narrowed at the call sites). */ import { z } from 'zod'; +import { ShortTextQuestionSchema } from './questions/short_text'; +import { LongTextQuestionSchema } from './questions/long_text'; +import { SingleChoiceQuestionSchema } from './questions/single_choice'; +import { MultiChoiceQuestionSchema } from './questions/multi_choice'; +import { ScaleQuestionSchema } from './questions/scale'; +import { BooleanQuestionSchema } from './questions/boolean'; +import { + DateRankedChoiceQuestionSchema, + DateRankedOptionSchema as PerTypeDateRankedOptionSchema, +} from './questions/date_ranked_choice'; -const FeedbackQuestionBaseSchema = z.object({ - id: z.string().min(1).max(64), - label: z.string().min(1).max(200), - required: z.boolean().optional(), - help: z.string().max(500).optional(), -}); - -/** One date/time option in a `date_ranked_choice` question. Times are stored as UTC ISO 8601 strings. */ -export const DateRankedOptionSchema = z.object({ - id: z.string().min(1).max(64).regex(/^[a-zA-Z0-9_-]+$/, { - message: 'option id may only contain letters, digits, "-" and "_"', - }), - start: z.string().datetime({ offset: true }), - end: z.string().datetime({ offset: true }).optional(), - label: z.string().max(200).optional(), -}); +/** Re-exported for callers that need the option shape standalone. */ +export const DateRankedOptionSchema = PerTypeDateRankedOptionSchema; export const FeedbackQuestionSchema = z.discriminatedUnion('type', [ - FeedbackQuestionBaseSchema.extend({ - type: z.literal('short_text'), - placeholder: z.string().max(100).optional(), - }), - FeedbackQuestionBaseSchema.extend({ - type: z.literal('long_text'), - placeholder: z.string().max(100).optional(), - }), - FeedbackQuestionBaseSchema.extend({ - type: z.literal('single_choice'), - options: z.array(z.string().min(1).max(200)).min(2).max(20), - }), - FeedbackQuestionBaseSchema.extend({ - type: z.literal('multi_choice'), - options: z.array(z.string().min(1).max(200)).min(2).max(20), - }), - FeedbackQuestionBaseSchema.extend({ - type: z.literal('scale'), - min: z.number().int().min(0).max(100), - max: z.number().int().min(1).max(100), - min_label: z.string().max(50).optional(), - max_label: z.string().max(50).optional(), - }), - FeedbackQuestionBaseSchema.extend({ - type: z.literal('boolean'), - }), - FeedbackQuestionBaseSchema.extend({ - type: z.literal('date_ranked_choice'), - options: z.array(DateRankedOptionSchema).min(2).max(50) - .refine( - (opts) => new Set(opts.map((o) => o.id)).size === opts.length, - { message: 'date_ranked_choice option ids must be unique' }, - ), - // Scale is locked at 1-5 (5-point Likert) per design — only the labels are author-configurable. - scale: z.object({ - min_label: z.string().max(50).optional(), - max_label: z.string().max(50).optional(), - }).optional(), - allow_partial: z.boolean().optional(), - }), + ShortTextQuestionSchema, + LongTextQuestionSchema, + SingleChoiceQuestionSchema, + MultiChoiceQuestionSchema, + ScaleQuestionSchema, + BooleanQuestionSchema, + DateRankedChoiceQuestionSchema, ]); /** Version stamp like `0.260505` (YYMMDD) or `0.260505.b` for same-day re-edits. */ diff --git a/src/lib/server/results.ts b/src/lib/server/results.ts index 7ee6bc9..78715a5 100644 --- a/src/lib/server/results.ts +++ b/src/lib/server/results.ts @@ -3,8 +3,15 @@ * * Aggregations are computed from the form_snapshot stored on each submission, * so historical results stay correct even after the form is later edited. + * + * The per-type aggregation logic (emptyStats / ingest / finalise / sanitize) + * lives in `$lib/questions/.ts`. This file is now a thin dispatcher: + * `aggregateResults` walks the questions and routes each one through the + * registry. Adding a new question type means writing one module file — no + * edit here. */ import type { FeedbackFormDefinition, FeedbackQuestion } from '../schemas'; +import { getQuestion } from '../questions/registry'; export interface ScaleStats { type: 'scale'; @@ -81,200 +88,49 @@ export function aggregateResults( current: FeedbackFormDefinition, subs: SubmissionRow[], ): AggregatedResults { - const questions: QuestionResult[] = current.questions.map((q) => ({ - id: q.id, - label: q.label, - type: q.type, - stats: emptyStats(q), - })); - const byId = new Map(questions.map((q) => [q.id, q])); + const questions: QuestionResult[] = current.questions.map((q) => { + const mod = getQuestion(q.type); + return { + id: q.id, + label: q.label, + type: q.type, + // Each module's emptyStats is typed by its own question variant; the + // top-level FeedbackQuestion[] has been narrowed by getQuestion's + // dispatch on `q.type` so the cast is sound. + stats: mod.emptyStats(q as never) as QuestionStats, + }; + }); + const qById = new Map(current.questions.map((q) => [q.id, q])); + const resultById = new Map(questions.map((q) => [q.id, q])); for (const sub of subs) { - for (const q of questions) { + for (const q of current.questions) { const v = sub.answers?.[q.id]; if (v === undefined || v === null) continue; - ingest(byId.get(q.id)!, current.questions.find((cq) => cq.id === q.id)!, v, sub.created_at); + const result = resultById.get(q.id)!; + getQuestion(q.type).ingest(result.stats as never, q as never, v, sub.created_at); } } - for (const q of questions) finalise(q.stats); + for (const q of questions) { + const def = qById.get(q.id); + if (!def) continue; + getQuestion(def.type).finalise(q.stats as never); + } return { total_submissions: subs.length, questions }; } -// Aggregation-time accumulator types. The `_sum` field is a real, typed -// member during ingest; finalise reads it and drops it before returning the -// public stats shape. Cleaner than the previous (s as X & { _sum?: number }) -// inline-cast-and-coalesce pattern. -type ScaleStatsWip = ScaleStats & { _sum: number }; -type DateRankedOptionStatsWip = DateRankedOptionStats & { _sum: number }; - -function emptyStats(q: FeedbackQuestion): QuestionStats { - switch (q.type) { - case 'scale': { - const wip: ScaleStatsWip = { - type: 'scale', - count: 0, - min: q.min, - max: q.max, - mean: null, - histogram: Array.from({ length: q.max - q.min + 1 }, (_, i) => ({ - value: q.min + i, - count: 0, - })), - _sum: 0, - }; - return wip; - } - case 'single_choice': - case 'multi_choice': - return { - type: q.type, - count: 0, - options: q.options.map((option) => ({ option, count: 0 })), - other_count: 0, - }; - case 'boolean': - return { type: 'boolean', count: 0, yes: 0, no: 0 }; - case 'short_text': - case 'long_text': - return { type: q.type, count: 0, answers: [] }; - case 'date_ranked_choice': - return { - type: 'date_ranked_choice', - count: 0, - options: q.options.map((opt) => { - const wip: DateRankedOptionStatsWip = { - id: opt.id, - start: opt.start, - end: opt.end ?? null, - label: opt.label ?? null, - count: 0, - mean: null, - histogram: [1, 2, 3, 4, 5].map((value) => ({ value, count: 0 })), - _sum: 0, - }; - return wip; - }), - }; - } -} - -function ingest( - out: QuestionResult, - q: FeedbackQuestion, - v: unknown, - created_at: string, -): void { - const s = out.stats; - switch (s.type) { - case 'scale': { - if (typeof v !== 'number' || !Number.isFinite(v)) return; - const acc = s as ScaleStatsWip; - acc.count++; - const bucket = acc.histogram.find((b) => b.value === v); - if (bucket) bucket.count++; - acc._sum += v; - return; - } - case 'single_choice': { - if (typeof v !== 'string') return; - s.count++; - const hit = s.options.find((o) => o.option === v); - if (hit) hit.count++; - else s.other_count++; - return; - } - case 'multi_choice': { - if (!Array.isArray(v)) return; - if (v.length === 0) return; - s.count++; - for (const choice of v) { - if (typeof choice !== 'string') continue; - const hit = s.options.find((o) => o.option === choice); - if (hit) hit.count++; - else s.other_count++; - } - return; - } - case 'boolean': { - if (typeof v !== 'boolean') return; - s.count++; - if (v) s.yes++; - else s.no++; - return; - } - case 'short_text': - case 'long_text': { - if (typeof v !== 'string' || v.trim() === '') return; - s.count++; - s.answers.push({ value: v, created_at }); - return; - } - case 'date_ranked_choice': { - if (!v || typeof v !== 'object' || Array.isArray(v)) return; - const ratings = v as Record; - let touched = false; - for (const opt of s.options) { - const raw = ratings[opt.id]; - if (raw === undefined || raw === null) continue; - if (typeof raw !== 'number' || !Number.isInteger(raw) || raw < 1 || raw > 5) continue; - const acc = opt as DateRankedOptionStatsWip; - acc.count++; - const bucket = acc.histogram.find((b) => b.value === raw); - if (bucket) bucket.count++; - acc._sum += raw; - touched = true; - } - if (touched) s.count++; - return; - } - } - void q; // q is the schema definition; ingest dispatches on s.type which - // is derived from it via emptyStats. Kept in the signature so a - // future per-type module can take the question for richer logic. -} - -function finalise(s: QuestionStats): void { - if (s.type === 'scale') { - const acc = s as ScaleStatsWip; - s.mean = s.count > 0 ? acc._sum / s.count : null; - delete (s as Partial)._sum; - return; - } - if (s.type === 'date_ranked_choice') { - for (const opt of s.options) { - const acc = opt as DateRankedOptionStatsWip; - opt.mean = opt.count > 0 ? acc._sum / opt.count : null; - delete (opt as Partial)._sum; - } - // Sort by mean desc with tiebreaks: count of "5"s, then "4"s, then count desc, then id. - s.options.sort((a, b) => { - const am = a.mean ?? -Infinity; - const bm = b.mean ?? -Infinity; - if (am !== bm) return bm - am; - const a5 = a.histogram.find((h) => h.value === 5)?.count ?? 0; - const b5 = b.histogram.find((h) => h.value === 5)?.count ?? 0; - if (a5 !== b5) return b5 - a5; - const a4 = a.histogram.find((h) => h.value === 4)?.count ?? 0; - const b4 = b.histogram.find((h) => h.value === 4)?.count ?? 0; - if (a4 !== b4) return b4 - a4; - if (a.count !== b.count) return b.count - a.count; - return a.id.localeCompare(b.id); - }); - } -} - -/** Public-safe results: drop free-text answers (PII / open data). */ +/** Public-safe results: drop free-text answers (PII / open data). + * Each module's sanitizeForPublic decides what survives — text types strip + * the answers list and keep only the count; everything else passes through. */ export function publicResults(r: AggregatedResults): AggregatedResults { return { total_submissions: r.total_submissions, - questions: r.questions.map((q) => { - if (q.stats.type === 'short_text' || q.stats.type === 'long_text') { - return { ...q, stats: { type: q.stats.type, count: q.stats.count, answers: [] } }; - } - return q; - }), + questions: r.questions.map((q) => ({ + ...q, + stats: getQuestion(q.type).sanitizeForPublic(q.stats as never) as QuestionStats, + })), }; } diff --git a/src/routes/api/admin/feedback/[id]/export/+server.ts b/src/routes/api/admin/feedback/[id]/export/+server.ts index 4c1d60d..421ab50 100644 --- a/src/routes/api/admin/feedback/[id]/export/+server.ts +++ b/src/routes/api/admin/feedback/[id]/export/+server.ts @@ -5,6 +5,8 @@ import { badRequest } from '$lib/server/errors'; import { fdb } from '$lib/server/fdb'; import { withOwnedInstance } from '$lib/server/admin-route'; import type { FeedbackFormDefinition } from '$lib/schemas'; +import { getQuestion } from '$lib/questions/registry'; +import type { CsvColumn } from '$lib/questions/types'; interface SubmissionRow { id: string; @@ -92,16 +94,14 @@ export const GET = withOwnedInstance(async ({ inst, event }) => { const formDef = inst.form_definition as FeedbackFormDefinition | null; const questions = formDef?.questions ?? []; - // Expand `date_ranked_choice` questions into one column per option (e.g. `kickoff_when[opt1]`). - // Other types stay as a single column keyed by question id. - const colSpecs: { qid: string; optId?: string; header: string }[] = []; + // Per-question column expansion. date_ranked_choice yields one column + // per option; everything else is a single column keyed by question id. + // The shape is owned by each module's csvColumns / csvCellFor. + const colSpecs: (CsvColumn & { question: typeof questions[number] })[] = []; for (const q of questions) { - if (q.type === 'date_ranked_choice') { - for (const opt of q.options) { - colSpecs.push({ qid: q.id, optId: opt.id, header: `${q.id}[${opt.id}]` }); - } - } else { - colSpecs.push({ qid: q.id, header: q.id }); + const mod = getQuestion(q.type); + for (const col of mod.csvColumns(q)) { + colSpecs.push({ ...col, question: q }); } } @@ -109,13 +109,8 @@ export const GET = withOwnedInstance(async ({ inst, event }) => { const subRows = submissions.map((row) => [ row.id, row.created_at, row.display_name, row.client_session_id, ...colSpecs.map((c) => { - const v = row.answers?.[c.qid]; - if (c.optId) { - if (!v || typeof v !== 'object' || Array.isArray(v)) return ''; - const r = (v as Record)[c.optId]; - return r === null || r === undefined ? '' : r; - } - return v ?? ''; + const answer = row.answers?.[c.qid]; + return getQuestion(c.question.type).csvCellFor(c.question, answer, c); }), ]); const submissionsCsv = rowsToCsv(subHeaders, subRows); diff --git a/src/routes/api/public/feedback/[slug]/submit/+server.ts b/src/routes/api/public/feedback/[slug]/submit/+server.ts index 2367793..0cae701 100644 --- a/src/routes/api/public/feedback/[slug]/submit/+server.ts +++ b/src/routes/api/public/feedback/[slug]/submit/+server.ts @@ -12,6 +12,7 @@ import { findExistingSubmission, isHoneypotTrap, } from '$lib/server/feedback'; +import { getQuestion } from '$lib/questions/registry'; import { checkRate } from '$lib/server/rate-limit'; import { fdb } from '$lib/server/fdb'; @@ -40,15 +41,16 @@ export const POST: RequestHandler = async ({ params, request, getClientAddress } for (const id of Object.keys(body.answers)) { if (!knownIds.has(id)) return badRequest(`Unknown question id: ${id}`); } + // Required-question gate. Per-type isAnswerEmpty is THE source of truth — + // the legacy inline empty-check missed `date_ranked_choice` answers of + // shape `{}` (object exists but no options rated). The registry rule + // now closes that gap by construction. for (const q of formDef.questions) { if (q.required) { - const v = body.answers[q.id]; - const empty = - v === undefined || - v === null || - (typeof v === 'string' && v.trim() === '') || - (Array.isArray(v) && v.length === 0); - if (empty) return badRequest(`Missing answer for required question: ${q.id}`); + const mod = getQuestion(q.type); + if (mod.isAnswerEmpty(q, body.answers[q.id])) { + return badRequest(`Missing answer for required question: ${q.id}`); + } } }