diff --git a/app/portal/[eventSlug]/actions.ts b/app/portal/[eventSlug]/actions.ts index b4b7b88..de4d099 100644 --- a/app/portal/[eventSlug]/actions.ts +++ b/app/portal/[eventSlug]/actions.ts @@ -274,16 +274,27 @@ export async function saveSubmissionAction(_prev: FormState, formData: FormData) const { ctx, me, eventSlug } = await actionSession(formData); const submissionId = text(formData, 'submissionId'); const formId = text(formData, 'formId'); - const fields = formId ? await submissionFields(formId) : []; const mine = await listMySubmissions(me.id); - if (!mine.some((entry) => entry.id === submissionId)) throw notFound('That session'); + const current = mine.find((entry) => entry.id === submissionId); + if (!current) throw notFound('That session'); + + /* + Only the questions this submission was asked are read back. Reading every question the form + holds records an empty answer to one that was never shown, which afterwards is + indistinguishable from a speaker who was asked and skipped it. + */ + const fields = formId ? await submissionFields(formId, current) : []; await recordRevision(ctx, 'session', submissionId, 'Edited the session content'); await updateMySubmission(ctx, me.id, submissionId, { title: text(formData, 'title'), descriptionMarkdown: text(formData, 'descriptionMarkdown'), level: text(formData, 'level'), + formatId: text(formData, 'formatId'), + trackId: text(formData, 'trackId'), + /* Checkboxes, so an empty list is "untag everything" and has to reach the service as one. */ + tagIds: formData.getAll('tagIds').map((entry) => String(entry)), answers: fields.length > 0 ? readAnswers(fields, formData) : undefined, }); diff --git a/app/portal/[eventSlug]/submissions/[submissionId]/SubmissionEditor.tsx b/app/portal/[eventSlug]/submissions/[submissionId]/SubmissionEditor.tsx index 8999384..ce1f6ae 100644 --- a/app/portal/[eventSlug]/submissions/[submissionId]/SubmissionEditor.tsx +++ b/app/portal/[eventSlug]/submissions/[submissionId]/SubmissionEditor.tsx @@ -1,10 +1,19 @@ 'use client'; import { useActionState, useState } from 'react'; -import { Card, CardBody, CardHeader, CardTitle, Input, Select, Textarea } from '@/components/ui'; +import { + Card, + CardBody, + CardHeader, + CardTitle, + Checkbox, + Input, + Select, + Textarea, +} from '@/components/ui'; import type { FormFieldSpec } from '@/lib/forms/contract'; import { renderMarkdown } from '@/lib/markdown'; -import type { PortalSubmission } from '@/lib/services/portal'; +import type { PortalSubmission, PortalTaxonomy } from '@/lib/services/portal'; import { IDLE_STATE } from '../../../form-state'; import styles from '../../../portal.module.css'; import { saveSubmissionAction, withdrawSubmissionAction } from '../../actions'; @@ -19,12 +28,15 @@ export function SubmissionEditor({ submission, fields, levelOptions, + taxonomy, }: { eventSlug: string; submission: PortalSubmission; fields: FormFieldSpec[]; /** What the form offers for Audience level, or `null` if it does not ask. */ levelOptions: string[] | null; + /** The event's formats, tracks and tags — each `null` where the form does not ask. */ + taxonomy: PortalTaxonomy; }) { const [state, action] = useActionState(saveSubmissionAction, IDLE_STATE); const [description, setDescription] = useState(submission.descriptionMarkdown ?? ''); @@ -118,6 +130,85 @@ export function SubmissionEditor({ )} + + {/* + Session format and Track. Both were asked on the way in and neither was editable + afterwards — Track appeared only as read-only header text — so a speaker who picked + the wrong one had no way to say so, on the two fields organizers filter and route on. + The options are the event's own lists, and the value is the id rather than the name, + because renaming a track must not detach the submissions that chose it. + */} + {taxonomy.formats && ( +
+ + + +
+ )} + + {taxonomy.tracks && ( +
+ + + +
+ )} + + {/* + Checkboxes rather than a multi-select, which is how the public form asks it. An event + with the tags question but no tags yet gets the note instead of an empty box, so the + blank is explained rather than looking broken. + */} + {taxonomy.tags && ( +
+ Tags + {taxonomy.tags.length === 0 ? ( + The organizers have not set up any tags yet. + ) : ( +
+ {taxonomy.tags.map((option) => ( + + ))} +
+ )} + +
+ )} diff --git a/app/portal/[eventSlug]/submissions/[submissionId]/page.tsx b/app/portal/[eventSlug]/submissions/[submissionId]/page.tsx index 221a11c..b5d7910 100644 --- a/app/portal/[eventSlug]/submissions/[submissionId]/page.tsx +++ b/app/portal/[eventSlug]/submissions/[submissionId]/page.tsx @@ -8,6 +8,7 @@ import { listGroupMembers, submissionFields, submissionLevelOptions, + submissionTaxonomy, type PortalSubmission, } from '@/lib/services/portal'; import { @@ -39,9 +40,10 @@ export default async function SubmissionDetailPage({ throw error; } - const [fields, levelOptions, members] = await Promise.all([ - submissionFields(submission.formId), + const [fields, levelOptions, taxonomy, members] = await Promise.all([ + submissionFields(submission.formId, submission), submissionLevelOptions(submission.formId), + submissionTaxonomy(submission.formId, event.id), listGroupMembers(submission.id, me.id), ]); @@ -142,6 +144,7 @@ export default async function SubmissionDetailPage({ submission={submission} fields={fields} levelOptions={levelOptions} + taxonomy={taxonomy} /> ) : ( diff --git a/lib/forms/contract.ts b/lib/forms/contract.ts index feb57b5..12ab441 100644 --- a/lib/forms/contract.ts +++ b/lib/forms/contract.ts @@ -318,6 +318,10 @@ export function evaluateCondition(condition: Condition, value: AnswerValue): boo * Whether a field is currently on screen. Because conditions are one hop, this is a direct lookup * against the referenced field's own answer — never a recursive walk, and never dependent on * whether the referenced field is itself visible. + * + * A condition whose question is not in `byId` shows the field. That is the right answer for a rule + * left dangling by a deleted question, and the wrong one for a caller that handed over a filtered + * list — pass the whole form, then drop what you are not rendering. */ export function isFieldVisible(field: FormFieldSpec, values: AnswerMap, byId: Map): boolean { if (!field.showIf) return true; @@ -326,8 +330,12 @@ export function isFieldVisible(field: FormFieldSpec, values: AnswerMap, byId: Ma return evaluateCondition(field.showIf, values[target.key] ?? null); } -export function visibleFields(fields: FormFieldSpec[], values: AnswerMap): FormFieldSpec[] { - const byId = new Map(fields.map((field) => [field.id, field])); +/** + * Generic in the field, so a caller holding `RuntimeField`s gets `RuntimeField`s back rather than + * having to cast its resolved options away to ask what is on screen. + */ +export function visibleFields(fields: T[], values: AnswerMap): T[] { + const byId = new Map(fields.map((field) => [field.id, field])); return fields.filter((field) => isFieldVisible(field, values, byId)); } diff --git a/lib/services/asked-questions.test.ts b/lib/services/asked-questions.test.ts new file mode 100644 index 0000000..417619c --- /dev/null +++ b/lib/services/asked-questions.test.ts @@ -0,0 +1,160 @@ +import { describe, expect, it } from 'vitest'; +import type { Condition, FormFieldSpec } from '../forms/contract'; +import { askedAnswers } from './review'; +import { askedQuestions, type AskedSource } from './submissions'; + +/* + `CFP-S2`. A form whose workshop question is gated on the built-in Session format, which is the + shape the evaluator built and the one the seeded demo happens not to have: its only condition + points at a custom field, so the bug never showed there. +*/ + +const FORMAT_ID = 'fmt-workshop'; +const TALK_ID = 'fmt-talk'; + +function field(overrides: Partial & { id: string; key: string }): FormFieldSpec { + return { + builtinKey: null, + type: 'short_text', + label: overrides.key, + position: 0, + step: 1, + required: false, + options: null, + showIf: null, + minLength: null, + maxLength: null, + charLimitGroup: null, + ...overrides, + }; +} + +const TITLE = field({ id: 'f-title', key: 'title', builtinKey: 'title', position: 0 }); +const FORMAT = field({ + id: 'f-format', + key: 'format', + builtinKey: 'format', + type: 'select', + options: [TALK_ID, FORMAT_ID], + position: 1, +}); +const PREREQS = field({ + id: 'f-prereqs', + key: 'prerequisites', + label: 'Workshop prerequisites', + type: 'long_text', + position: 2, + showIf: { fieldId: FORMAT.id, op: 'eq', value: FORMAT_ID } satisfies Condition, +}); +const NOTES = field({ id: 'f-notes', key: 'notes', label: 'Anything else', position: 3 }); + +const FORM = [TITLE, FORMAT, PREREQS, NOTES]; + +function row(overrides: Partial = {}): AskedSource { + return { + title: 'Taming 40-Minute CI', + descriptionMarkdown: 'A talk about builds.', + formatId: TALK_ID, + trackId: null, + level: null, + answers: {}, + tagIds: [], + ...overrides, + }; +} + +describe('askedQuestions', () => { + it('leaves out a question gated on a format the submission is not', () => { + const keys = askedQuestions(FORM, row()).map((entry) => entry.key); + expect(keys).toEqual(['title', 'format', 'notes']); + }); + + it('includes it once the submission is that format', () => { + const keys = askedQuestions(FORM, row({ formatId: FORMAT_ID })).map((entry) => entry.key); + expect(keys).toContain('prerequisites'); + }); + + /* The regression itself: the gating value lives in a column, not in `answers`. */ + it('reads the gating value from the column rather than from the answer map', () => { + const asked = askedQuestions(FORM, row({ formatId: FORMAT_ID, answers: {} })); + expect(asked.map((entry) => entry.key)).toContain('prerequisites'); + }); + + it('resolves a condition on the built-in track the same way', () => { + const gated = field({ + id: 'f-cfp', + key: 'cfp_notes', + position: 4, + showIf: { fieldId: 'f-track', op: 'eq', value: 'trk-infra' }, + }); + const track = field({ id: 'f-track', key: 'track', builtinKey: 'track', type: 'select' }); + const form = [...FORM, track, gated]; + + expect(askedQuestions(form, row({ trackId: 'trk-infra' })).map((e) => e.key)).toContain( + 'cfp_notes', + ); + expect(askedQuestions(form, row({ trackId: 'trk-web' })).map((e) => e.key)).not.toContain( + 'cfp_notes', + ); + }); + + it('answers questions gated on a custom answer from the answer map', () => { + const parent = field({ id: 'f-recorded', key: 'prior_recording', type: 'select' }); + const child = field({ + id: 'f-link', + key: 'recording_link', + position: 5, + showIf: { fieldId: parent.id, op: 'eq', value: 'yes' }, + }); + const form = [...FORM, parent, child]; + + expect( + askedQuestions(form, row({ answers: { prior_recording: 'yes' } })).map((e) => e.key), + ).toContain('recording_link'); + expect( + askedQuestions(form, row({ answers: { prior_recording: 'no' } })).map((e) => e.key), + ).not.toContain('recording_link'); + }); + + it('keeps the order the form is in', () => { + expect(askedQuestions(FORM, row({ formatId: FORMAT_ID })).map((entry) => entry.key)).toEqual([ + 'title', + 'format', + 'prerequisites', + 'notes', + ]); + }); +}); + +describe('askedAnswers', () => { + it('hides an answer stored against a question this submission was never asked', () => { + const answers = { prerequisites: '', notes: 'Please schedule me early.' }; + expect(askedAnswers(answers, FORM, row())).toEqual({ notes: 'Please schedule me early.' }); + }); + + it('shows it on a submission that was asked', () => { + const answers = { prerequisites: 'Bring a laptop.', notes: '' }; + expect(askedAnswers(answers, FORM, row({ formatId: FORMAT_ID }))).toEqual(answers); + }); + + /* + Already-stored empties are the damage the write path did before the fix, so filtering has to be + at render — a row saved by the old portal keeps the key. + */ + it('hides a non-empty answer too, because a value there is stale rather than asked', () => { + const answers = { prerequisites: 'Left over from when this was a workshop.' }; + expect(askedAnswers(answers, FORM, row())).toEqual({}); + }); + + /* An answer with no question left to consult. The panel falls back to the raw key for these. */ + it('keeps an answer whose question is no longer on the form', () => { + const answers = { retired_question: 'Answered before that question was deleted.' }; + expect(askedAnswers(answers, FORM, row())).toEqual(answers); + }); + + it('leaves an unconditional questionnaire untouched', () => { + const form = [TITLE, NOTES]; + const answers = { notes: 'No conditions anywhere on this form.' }; + expect(askedAnswers(answers, form, row())).toEqual(answers); + }); +}); diff --git a/lib/services/portal-taxonomy.test.ts b/lib/services/portal-taxonomy.test.ts new file mode 100644 index 0000000..15537e8 --- /dev/null +++ b/lib/services/portal-taxonomy.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, it } from 'vitest'; +import { choiceError, tagsError } from './portal'; + +/** + * `CFP-S2`. Session format, Track and Tags were asked on the public form and then not editable at + * all — Track was read-only header text — leaving no way for a speaker to correct the two fields + * organizers filter and route on. The portal now offers the event's own lists, and these are the + * rule behind the controls: the edit view posts to a server action, and a server action takes + * whatever it is given. + */ +const FORMATS = [ + { id: 'fmt-talk', name: 'Talk' }, + { id: 'fmt-workshop', name: 'Workshop' }, +]; + +const TAGS = [ + { id: 'tag-ai', name: 'AI' }, + { id: 'tag-ops', name: 'Ops' }, +]; + +describe('choiceError', () => { + it('accepts an id the event has', () => { + expect(choiceError('session format', 'fmt-workshop', FORMATS)).toBeNull(); + }); + + /** Optional, so clearing it is an answer rather than an invalid one. */ + it('accepts a blank', () => { + expect(choiceError('session format', '', FORMATS)).toBeNull(); + expect(choiceError('session format', undefined, FORMATS)).toBeNull(); + }); + + it('refuses an id the event does not have', () => { + expect(choiceError('session format', 'fmt-keynote', FORMATS)).toBe( + 'Choose one of the session formats offered', + ); + }); + + /** + * These are ids, not names. Posting the label a speaker read has to fail, or a renamed track + * would quietly detach every submission that chose it. + */ + it('refuses the visible name in place of the id', () => { + expect(choiceError('session format', 'Workshop', FORMATS)).toBe( + 'Choose one of the session formats offered', + ); + }); + + /** The form can leave the question out, in which case the editor renders no control at all. */ + it('refuses any value when the form does not ask', () => { + expect(choiceError('track', 'trk-platform', null)).toBe('This form does not ask for a track'); + }); + + it('accepts a form that does not ask being sent nothing', () => { + expect(choiceError('track', '', null)).toBeNull(); + }); + + it('says which question it is talking about', () => { + expect(choiceError('track', 'nope', [])).toBe('Choose one of the tracks offered'); + }); +}); + +describe('tagsError', () => { + it('accepts ids the event has', () => { + expect(tagsError(['tag-ai', 'tag-ops'], TAGS)).toBeNull(); + }); + + /** Untagging everything is a legitimate save, not a missing answer. */ + it('accepts an empty set', () => { + expect(tagsError([], TAGS)).toBeNull(); + expect(tagsError(undefined, TAGS)).toBeNull(); + }); + + /** + * All or nothing. Keeping the ids it recognized would leave a speaker looking at a set of tags + * they did not choose, with no sign that anything was dropped. + */ + it('refuses the whole set when one id is not the event’s', () => { + expect(tagsError(['tag-ai', 'tag-invented'], TAGS)).toBe('Choose from the tags offered'); + }); + + it('refuses any tag when the form does not ask', () => { + expect(tagsError(['tag-ai'], null)).toBe('This form does not ask for tags'); + }); + + it('accepts a form that does not ask being sent nothing', () => { + expect(tagsError([], null)).toBeNull(); + }); + + /** An event that has the question but no tags yet offers none, so none can be chosen. */ + it('refuses everything when the event has no tags', () => { + expect(tagsError(['tag-ai'], [])).toBe('Choose from the tags offered'); + expect(tagsError([], [])).toBeNull(); + }); +}); diff --git a/lib/services/portal.ts b/lib/services/portal.ts index c64ab6f..ece69a9 100644 --- a/lib/services/portal.ts +++ b/lib/services/portal.ts @@ -14,6 +14,8 @@ import { scheduledSession, sessionFormat, submission, + submissionTag, + tag, track, user, } from '../../db/schema'; @@ -22,7 +24,7 @@ import { can } from '../context'; import { appUrl } from '../env'; import { conflict, forbidden, invalid, notFound } from '../errors'; import type { AnswerMap, FormFieldSpec } from '../forms/contract'; -import { clearHiddenAnswers, validateAnswers } from '../forms/contract'; +import { clearHiddenAnswers, isBuiltinKey, validateAnswers } from '../forms/contract'; import { formatRef } from '../ids'; import { sendMail } from '../mail'; import { markdownToText, renderMarkdown, renderTrustedMarkdown } from '../markdown'; @@ -37,7 +39,7 @@ import { import { activeSmsTransportName } from '../sms'; import { mutateAgendaAtomically } from './agenda-guard'; import { assertParticipantLimits } from './forms'; -import { DEFAULT_LEVELS } from './submissions'; +import { DEFAULT_LEVELS, askedQuestions, type AskedSource, type NamedRow } from './submissions'; import { phoneVerificationIsCurrent } from './notification-preferences'; /** @@ -436,6 +438,10 @@ export type PortalSubmission = { level: string | null; formatName: string | null; trackName: string | null; + /* The ids behind those two names, and the tags, because the portal edits them and names are not keys. */ + formatId: string | null; + trackId: string | null; + tagIds: string[]; formId: string; formSlug: string; formName: string; @@ -491,16 +497,27 @@ export async function listMySubmissions(participantId: string): Promise row.submission.id), - ), - ); + const ids = rows.map((row) => row.submission.id); + + const [scheduled, tagRows] = await Promise.all([ + db + .select({ session: scheduledSession, roomName: room.name }) + .from(scheduledSession) + .leftJoin(room, eq(room.id, scheduledSession.roomId)) + .where(inArray(scheduledSession.submissionId, ids)), + db + .select({ submissionId: submissionTag.submissionId, tagId: submissionTag.tagId }) + .from(submissionTag) + .where(inArray(submissionTag.submissionId, ids)), + ]); + + /* Tags live in their own table, so one batched read rather than a query per submission. */ + const tagsBySubmission = new Map(); + for (const row of tagRows) { + const held = tagsBySubmission.get(row.submissionId); + if (held) held.push(row.tagId); + else tagsBySubmission.set(row.submissionId, [row.tagId]); + } const slotBySubmission = new Map( scheduled .filter((row) => row.session.submissionId) @@ -528,6 +545,9 @@ export async function listMySubmissions(participantId: string): Promise { +/** + * The organizer-authored questions this submission was asked and a speaker can still answer. + * + * Conditions are resolved over the whole form and only then are the built-ins dropped. Filtering + * them out first is what put "Workshop prerequisites" on a Talk: the rule pointed at the Session + * format question, which was no longer in the list, and an unresolvable rule shows its field. + */ +export async function submissionFields( + formId: string, + row: AskedSource, +): Promise { const rows = await getDb() .select() .from(formField) .where(eq(formField.formId, formId)) .orderBy(asc(formField.position)); - return rows - .filter((row) => !row.builtinKey && row.type !== 'file') - .map((row) => ({ - id: row.id, - key: row.key, - builtinKey: null, - type: row.type, - label: row.label, - position: row.position, - step: row.step, - required: row.required, - options: row.options ?? null, - showIf: row.showIf ?? null, - minLength: row.minLength, - maxLength: row.maxLength, - charLimitGroup: row.charLimitGroup, + const specs = rows + /* The abstract's own questions. The participant set is answered on a different screen. */ + .filter((entry) => entry.entity === 'abstract') + .map((entry) => ({ + id: entry.id, + key: entry.key, + /* The column is free text; only the keys the contract knows carry built-in behaviour. */ + builtinKey: isBuiltinKey(entry.builtinKey) ? entry.builtinKey : null, + type: entry.type, + label: entry.label, + position: entry.position, + step: entry.step, + required: entry.required, + options: entry.options ?? null, + showIf: entry.showIf ?? null, + minLength: entry.minLength, + maxLength: entry.maxLength, + charLimitGroup: entry.charLimitGroup, })); + + /* + A file question is left out of what the portal renders rather than out of what it resolves: it + is still an answer another question can be conditioned on, and the portal simply has nowhere to + re-upload from. + */ + return askedQuestions(specs, row).filter( + (field) => !field.builtinKey && field.type !== 'file', + ); } /** @@ -595,10 +635,75 @@ export async function submissionLevelOptions(formId: string): Promise { + const db = getDb(); + const asked = new Set( + ( + await db + .select({ builtinKey: formField.builtinKey }) + .from(formField) + .where( + and( + eq(formField.formId, formId), + inArray(formField.builtinKey, ['format', 'track', 'tags']), + ), + ) + ).map((row) => row.builtinKey), + ); + if (asked.size === 0) return { formats: null, tracks: null, tags: null }; + + /* Only the lists the form actually asks for: a CFP with no tags question should cost no query. */ + const [formats, tracks, tags] = await Promise.all([ + asked.has('format') + ? db + .select({ id: sessionFormat.id, name: sessionFormat.name }) + .from(sessionFormat) + .where(eq(sessionFormat.eventId, eventId)) + .orderBy(asc(sessionFormat.position)) + : null, + asked.has('track') + ? db + .select({ id: track.id, name: track.name }) + .from(track) + .where(eq(track.eventId, eventId)) + .orderBy(asc(track.position)) + : null, + asked.has('tags') + ? db + .select({ id: tag.id, name: tag.name }) + .from(tag) + .where(eq(tag.eventId, eventId)) + .orderBy(asc(tag.name)) + : null, + ]); + + return { formats, tracks, tags }; +} + export const submissionEditSchema = z.object({ title: z.string().trim().min(3, 'Give the session a title').max(255), descriptionMarkdown: z.string().max(5000, 'Description is limited to 5,000 characters').optional(), level: z.string().trim().max(60).optional(), + formatId: z.string().trim().optional(), + trackId: z.string().trim().optional(), + tagIds: z.array(z.string().trim()).optional(), }); /** @@ -622,6 +727,36 @@ export function levelError( return null; } +/** + * The id has to name a row the event actually has. Same reasoning as `levelError` and the same + * shape, minus the tolerance for a legacy value: `format_id` and `track_id` are foreign keys, so + * whatever is on the record was picked from this list to begin with. + * + * `question` is the wording a speaker sees, so it reads as the label above the control did. + */ +export function choiceError( + question: string, + value: string | undefined, + options: NamedRow[] | null, +): string | null { + if (!value) return null; + if (!options) return `This form does not ask for a ${question}`; + if (!options.some((row) => row.id === value)) return `Choose one of the ${question}s offered`; + return null; +} + +/** + * Every tag has to be one the event offers, and there is no partial save: an id the event does not + * have means the request did not come from the form, and taking the rest of it would leave the + * speaker looking at a set of tags they did not choose. + */ +export function tagsError(values: string[] | undefined, options: NamedRow[] | null): string | null { + if (!values || values.length === 0) return null; + if (!options) return 'This form does not ask for tags'; + const known = new Set(options.map((row) => row.id)); + return values.every((value) => known.has(value)) ? null : 'Choose from the tags offered'; +} + export type SubmissionEditInput = z.infer & { answers?: AnswerMap }; async function requireMyRole(participantId: string, submissionId: string) { @@ -658,29 +793,73 @@ export async function updateMySubmission( throw invalid('Some details need attention', details); } - const levelOptions = await submissionLevelOptions(current.formId); - const levelProblem = levelError(parsed.data.level, levelOptions, current.level); - if (levelProblem) throw invalid('Some details need attention', { level: levelProblem }); + const [levelOptions, taxonomy] = await Promise.all([ + submissionLevelOptions(current.formId), + submissionTaxonomy(current.formId, ctx.eventId), + ]); - const fields = await submissionFields(current.formId); + const details: Record = {}; + const note = (field: string, problem: string | null) => { + if (problem) details[field] = problem; + }; + note('level', levelError(parsed.data.level, levelOptions, current.level)); + note('formatId', choiceError('session format', parsed.data.formatId, taxonomy.formats)); + note('trackId', choiceError('track', parsed.data.trackId, taxonomy.tracks)); + note('tagIds', tagsError(parsed.data.tagIds, taxonomy.tags)); + if (Object.keys(details).length > 0) throw invalid('Some details need attention', details); + + /* + What the record will say once this save lands. + + Only the questions the form asks move: the editor renders no control for one it does not have, + so an empty value there means "nobody was shown this" rather than "cleared", and writing it + through would quietly drop a value on the next unrelated save. + + The conditions below are resolved against this rather than against `current`, because a speaker + switching a Talk to a Workshop is asked the workshop questions by that same save. + */ + const written = { + title: parsed.data.title, + descriptionMarkdown: blankToNull(parsed.data.descriptionMarkdown), + level: levelOptions ? blankToNull(parsed.data.level) : current.level, + formatId: taxonomy.formats ? blankToNull(parsed.data.formatId) : current.formatId, + trackId: taxonomy.tracks ? blankToNull(parsed.data.trackId) : current.trackId, + answers: input.answers ?? current.answers, + tagIds: taxonomy.tags && parsed.data.tagIds ? parsed.data.tagIds : current.tagIds, + }; + + const fields = await submissionFields(current.formId, written); const answers = input.answers ? clearHiddenAnswers(fields, input.answers) : current.answers; if (input.answers) validateAnswers(fields, answers); - await getDb() + const db = getDb(); + await db .update(submission) .set({ - title: parsed.data.title, - descriptionMarkdown: blankToNull(parsed.data.descriptionMarkdown), - /* - Only when the form asks. The editor renders no control for a question the form does not - have, so an empty `level` in that case means "nobody was shown this" rather than "cleared" — - writing it through would quietly drop a value on the next unrelated save. - */ - ...(levelOptions ? { level: blankToNull(parsed.data.level) } : {}), + title: written.title, + descriptionMarkdown: written.descriptionMarkdown, + ...(levelOptions ? { level: written.level } : {}), + ...(taxonomy.formats ? { formatId: written.formatId } : {}), + ...(taxonomy.tracks ? { trackId: written.trackId } : {}), answers, updatedAt: new Date(), }) .where(and(eq(submission.id, submissionId), eq(submission.eventId, ctx.eventId))); + + /* + Tags are rows rather than a column, so the set is replaced the same way a submit replaces it. + Only when the form asks — and `tagIds` being absent from the payload is left alone too, so a + caller that saves nothing but a title does not silently untag the session. + */ + if (taxonomy.tags && parsed.data.tagIds) { + await db.delete(submissionTag).where(eq(submissionTag.submissionId, submissionId)); + if (parsed.data.tagIds.length > 0) { + await db + .insert(submissionTag) + .values(parsed.data.tagIds.map((tagId) => ({ submissionId, tagId }))) + .onConflictDoNothing(); + } + } } /** diff --git a/lib/services/review.ts b/lib/services/review.ts index d52ac80..b02ee78 100644 --- a/lib/services/review.ts +++ b/lib/services/review.ts @@ -28,6 +28,8 @@ import { can, requireCapability } from '../context'; import { toCsv } from '../csv'; import { appUrl } from '../env'; import { DEFAULT_TIMEZONE, zonedDateKey } from '../event-dates'; +import type { AnswerMap, FormFieldSpec } from '../forms/contract'; +import { isBuiltinKey } from '../forms/contract'; import { conflict, forbidden, invalid, notFound } from '../errors'; import { formatRef, slugify } from '../ids'; import { sendMail } from '../mail'; @@ -36,7 +38,12 @@ import { parseSpeakerName } from '../speaker-name'; import { assertRoundDateOrder } from '../review-round-dates'; import { weightedScore } from '../review-scoring'; import { DECISION_TEMPLATES, loadCommsContext, sendDecisionNotice, wrapInBranding } from './comms'; -import { ensureParticipant, linkPrimarySpeaker } from './submissions'; +import { + askedQuestions, + ensureParticipant, + linkPrimarySpeaker, + type AskedSource, +} from './submissions'; import { emitWebhook } from '../webhooks'; /** @@ -2582,6 +2589,27 @@ export type SubmissionReview = { conflictedReviewerUserIds: string[]; }; +/** + * `CFP-S2`. The answers a reader should see: everything except answers to questions the form holds + * but this submission was never asked. "Workshop prerequisites —" on a talk reads as a question the + * speaker skipped, which for a reviewer scoring completeness is worse than not showing it at all. + * + * An answer whose question is no longer on the form is kept. There is no rule left to consult, and + * losing sight of a real answer is worse than an unlabelled one — the panel already falls back to + * the raw key for those. + */ +export function askedAnswers( + answers: Record, + fields: FormFieldSpec[], + row: AskedSource, +): Record { + const asked = new Set(askedQuestions(fields, row).map((field) => field.key)); + const onTheForm = new Set(fields.map((field) => field.key)); + return Object.fromEntries( + Object.entries(answers).filter(([key]) => asked.has(key) || !onTheForm.has(key)), + ); +} + export async function loadSubmissionReview( ctx: EventContext, submissionId: string, @@ -2656,10 +2684,20 @@ export async function loadSubmissionReview( }), db .select({ + id: formField.id, key: formField.key, label: formField.label, type: formField.type, builtinKey: formField.builtinKey, + entity: formField.entity, + position: formField.position, + step: formField.step, + required: formField.required, + options: formField.options, + showIf: formField.showIf, + minLength: formField.minLength, + maxLength: formField.maxLength, + charLimitGroup: formField.charLimitGroup, }) .from(formField) .where(eq(formField.formId, row.formId)), @@ -2730,7 +2768,18 @@ export async function loadSubmissionReview( trackName: trackRow?.name ?? null, formatName: formatRow?.name ?? null, tags: tagRows, - answers: row.answers as Record, + answers: askedAnswers( + row.answers as Record, + fieldRows + /* The abstract's own questions. The participant set is answered on a different screen. */ + .filter((field) => field.entity === 'abstract') + .map((field) => ({ + ...field, + /* The column is free text; only the keys the contract knows carry built-in behaviour. */ + builtinKey: isBuiltinKey(field.builtinKey) ? field.builtinKey : null, + })), + { ...row, answers: row.answers as AnswerMap, tagIds: tagRows.map((entry) => entry.id) }, + ), answerLabels: Object.fromEntries(fieldRows.map((field) => [field.key, field.label])), submittedAt: row.submittedAt, decidedAt: row.decidedAt, diff --git a/lib/services/submissions.ts b/lib/services/submissions.ts index 6de2a99..4294543 100644 --- a/lib/services/submissions.ts +++ b/lib/services/submissions.ts @@ -29,6 +29,7 @@ import { splitAnswers, validateAnswers, validateParticipantCounts, + visibleFields, type AnswerMap, type AnswerValue, type BuiltinKey, @@ -627,7 +628,7 @@ export type DraftValueSource = { export function rehydrateDraftValues( row: DraftValueSource, tagIds: string[], - fields: RuntimeField[], + fields: readonly FormFieldSpec[], ): AnswerMap { const values: AnswerMap = { ...row.answers }; for (const field of fields) { @@ -657,6 +658,24 @@ export function rehydrateDraftValues( return values; } +export type AskedSource = DraftValueSource & { tagIds: string[] }; + +/** + * `CFP-S2`. The questions a submission was actually asked, in form order. + * + * Every surface that shows a submission after the fact has to answer this, and none of them can + * answer it from `answers` alone: a question conditioned on "Session format is Workshop" is gated + * by a value that lives in a column. Rehydrating the built-ins first is what makes the condition + * resolvable at all — filter them out and `isFieldVisible` sees a rule pointing at nothing, treats + * it as no rule, and shows a workshop question on a talk. + * + * Pass the whole form. Callers drop the built-ins from what they render afterwards, which is a + * different question from what was asked. + */ +export function askedQuestions(fields: T[], row: AskedSource): T[] { + return visibleFields(fields, rehydrateDraftValues(row, row.tagIds, fields)); +} + /** Rehydrates a draft into the shape the runtime renders, built-in columns folded back in. */ export async function loadDraftValues( submissionId: string,