fix(timeline): rehydrate event-form pickers after a validation failure
Decision 6 / REQ-010 promised the pickers survive a fail(400) "including pre-selected persons/documents", but EventForm only seeded them from event/initialPersons — never from the fail payload — and the payload carried only bare ids, which can't rebuild a chip (chips need displayName/title). On the use:enhance path the in-memory selection survived; on a no-JS full reload the chips were silently dropped. Now the save action re-fetches the selected persons/documents by id (lookupSelections, non-ok swallowed like the prefill path) and returns full chip data; EventForm seeds the pickers from form.persons/documents ahead of the seeded event. Extract preservedFormFields() to DRY the four fail payloads; validateEventForm now returns the error pair and the route owns the fail(). Component test pins the rehydration; the server spec now asserts the fail payload carries labelled chips, not just ids. Addresses PR #832 review (Developer + Requirements Engineer concern, REQ-010). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -30,6 +30,10 @@ interface FormResult {
|
|||||||
type?: string;
|
type?: string;
|
||||||
personIds?: string[];
|
personIds?: string[];
|
||||||
documentIds?: string[];
|
documentIds?: string[];
|
||||||
|
// Rehydrated chip data (id + label) so the pickers re-render after a fail(400)
|
||||||
|
// even on a no-JS full reload — bare ids alone can't rebuild a chip (REQ-010).
|
||||||
|
persons?: PersonOption[];
|
||||||
|
documents?: DocumentOption[];
|
||||||
}
|
}
|
||||||
|
|
||||||
let {
|
let {
|
||||||
@@ -46,8 +50,10 @@ let {
|
|||||||
form?: FormResult | null;
|
form?: FormResult | null;
|
||||||
} = $props();
|
} = $props();
|
||||||
|
|
||||||
// Initial-state snapshot from incoming props (event > preserved fail payload).
|
// Initial-state snapshot from incoming props, preferring a preserved fail payload
|
||||||
// The form owns these after mount; re-mount with a different `event` to reset.
|
// over the seeded `event`. This component is intentionally single-shot: props are
|
||||||
|
// snapshotted into $state once, so a parent re-render with a different `event`
|
||||||
|
// won't update the form — the two dedicated routes always remount, which is fine.
|
||||||
let title = $state(form?.title ?? event?.title ?? '');
|
let title = $state(form?.title ?? event?.title ?? '');
|
||||||
let description = $state(form?.description ?? event?.description ?? '');
|
let description = $state(form?.description ?? event?.description ?? '');
|
||||||
let type = $state<string>(form?.type ?? event?.type ?? 'PERSONAL');
|
let type = $state<string>(form?.type ?? event?.type ?? 'PERSONAL');
|
||||||
@@ -55,19 +61,23 @@ let dateIso = $state(event?.eventDate ?? '');
|
|||||||
let precision = $state<DatePrecision>((event?.precision as DatePrecision) ?? 'DAY');
|
let precision = $state<DatePrecision>((event?.precision as DatePrecision) ?? 'DAY');
|
||||||
let endDateIso = $state(event?.eventDateEnd ?? '');
|
let endDateIso = $state(event?.eventDateEnd ?? '');
|
||||||
|
|
||||||
|
// On a fail(400) the server returns rehydrated chip data (form.persons/documents)
|
||||||
|
// so the pickers survive the round-trip — even without JS — ahead of the seeded
|
||||||
|
// `event` or the prefill initials (REQ-010 / Decision 6).
|
||||||
let selectedPersons = $state<PersonOption[]>(
|
let selectedPersons = $state<PersonOption[]>(
|
||||||
event?.persons ? event.persons.map(toPersonOption) : initialPersons
|
form?.persons ?? (event?.persons ? event.persons.map(toPersonOption) : initialPersons)
|
||||||
);
|
);
|
||||||
let selectedDocuments = $state<DocumentOption[]>(
|
let selectedDocuments = $state<DocumentOption[]>(
|
||||||
event?.documents
|
form?.documents ??
|
||||||
? event.documents.map((d) => ({
|
(event?.documents
|
||||||
// Graceful degradation: DocumentRef has no precision fields. formatDocumentOption
|
? event.documents.map((d) => ({
|
||||||
// defaults a missing precision to DAY, so the chip shows the full documentDate.
|
// Graceful degradation: DocumentRef has no precision fields. formatDocumentOption
|
||||||
id: d.id,
|
// defaults a missing precision to DAY, so the chip shows the full documentDate.
|
||||||
title: d.title,
|
id: d.id,
|
||||||
documentDate: d.documentDate
|
title: d.title,
|
||||||
}))
|
documentDate: d.documentDate
|
||||||
: initialDocuments
|
}))
|
||||||
|
: initialDocuments)
|
||||||
);
|
);
|
||||||
|
|
||||||
const isEdit = $derived(event !== undefined);
|
const isEdit = $derived(event !== undefined);
|
||||||
|
|||||||
@@ -58,6 +58,19 @@ describe('EventForm — required-field error (REQ-010)', () => {
|
|||||||
await page.getByRole('button', { name: 'Speichern' }).click();
|
await page.getByRole('button', { name: 'Speichern' }).click();
|
||||||
await expect.element(page.getByText('Bitte einen Titel eingeben.')).toBeInTheDocument();
|
await expect.element(page.getByText('Bitte einen Titel eingeben.')).toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('rehydrates the pickers from the fail payload (Decision 6)', async () => {
|
||||||
|
render(EventForm, {
|
||||||
|
form: {
|
||||||
|
titleError: 'Bitte einen Titel eingeben.',
|
||||||
|
title: '',
|
||||||
|
persons: [{ id: 'p1', displayName: 'Anna Müller' }],
|
||||||
|
documents: [{ id: 'd1', title: 'Brief A', documentDate: '1925-04-01' }]
|
||||||
|
}
|
||||||
|
});
|
||||||
|
await expect.element(page.getByText('Anna Müller')).toBeInTheDocument();
|
||||||
|
await expect.element(page.getByText(/Brief A/)).toBeInTheDocument();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('EventForm — submitting state (named AC)', () => {
|
describe('EventForm — submitting state (named AC)', () => {
|
||||||
|
|||||||
@@ -1,9 +1,12 @@
|
|||||||
import { fail } from '@sveltejs/kit';
|
|
||||||
import { m } from '$lib/paraglide/messages.js';
|
import { m } from '$lib/paraglide/messages.js';
|
||||||
|
import { createApiClient } from '$lib/shared/api.server';
|
||||||
import type { components } from '$lib/generated/api';
|
import type { components } from '$lib/generated/api';
|
||||||
import type { DatePrecision } from '$lib/shared/utils/documentDate';
|
import type { DatePrecision } from '$lib/shared/utils/documentDate';
|
||||||
|
import type { PersonOption } from '$lib/person/personOption';
|
||||||
|
import type { DocumentOption } from '$lib/document/documentTypeahead';
|
||||||
|
|
||||||
type TimelineEventRequest = components['schemas']['TimelineEventRequest'];
|
type TimelineEventRequest = components['schemas']['TimelineEventRequest'];
|
||||||
|
type ApiClient = ReturnType<typeof createApiClient>;
|
||||||
|
|
||||||
// Prevents open redirect: validate before constructing /persons/{id}. See OWASP CWE-601.
|
// Prevents open redirect: validate before constructing /persons/{id}. See OWASP CWE-601.
|
||||||
const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
|
const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
|
||||||
@@ -68,24 +71,60 @@ export function parseEventForm(formData: FormData): ParsedEventForm {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Surfaces all failing required-field errors simultaneously (title + date) via a
|
* Returns both failing required-field errors (title + date) simultaneously, or
|
||||||
* fail(400) payload that also preserves every entered value — including the
|
* null when the form is valid. The route owns the `fail(400)` so it can enrich
|
||||||
* picker arrays — so the form re-renders without losing state. Returns null when
|
* the payload with the preserved field values and rehydrated picker selections.
|
||||||
* the form is valid.
|
|
||||||
*/
|
*/
|
||||||
export function validateEventForm(parsed: ParsedEventForm) {
|
export function validateEventForm(
|
||||||
|
parsed: ParsedEventForm
|
||||||
|
): { titleError: string; dateError: string } | null {
|
||||||
const titleError = parsed.title.length === 0 ? m.event_editor_title_required() : '';
|
const titleError = parsed.title.length === 0 ? m.event_editor_title_required() : '';
|
||||||
const dateError = parsed.eventDate.length === 0 ? m.event_editor_date_required() : '';
|
const dateError = parsed.eventDate.length === 0 ? m.event_editor_date_required() : '';
|
||||||
if (!titleError && !dateError) return null;
|
if (!titleError && !dateError) return null;
|
||||||
return fail(400, {
|
return { titleError, dateError };
|
||||||
titleError,
|
}
|
||||||
dateError,
|
|
||||||
|
/** The entered field values echoed back in every `fail(...)` so the form re-renders without loss. */
|
||||||
|
export function preservedFormFields(parsed: ParsedEventForm) {
|
||||||
|
return {
|
||||||
title: parsed.title,
|
title: parsed.title,
|
||||||
description: parsed.description,
|
description: parsed.description,
|
||||||
type: parsed.type,
|
type: parsed.type,
|
||||||
personIds: parsed.personIds,
|
personIds: parsed.personIds,
|
||||||
documentIds: parsed.documentIds
|
documentIds: parsed.documentIds
|
||||||
});
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Re-fetches the selected persons/documents by id so a `fail(400)` can re-render
|
||||||
|
* the pickers with full chip labels — the form only resubmits bare ids, which
|
||||||
|
* cannot rebuild a chip on their own (Decision 6 / REQ-010). Non-ok lookups are
|
||||||
|
* swallowed: a since-deleted id silently drops from the picker rather than
|
||||||
|
* leaking existence, mirroring the prefill path in the new-route load.
|
||||||
|
*/
|
||||||
|
export async function lookupSelections(
|
||||||
|
api: ApiClient,
|
||||||
|
personIds: string[],
|
||||||
|
documentIds: string[]
|
||||||
|
): Promise<{ persons: PersonOption[]; documents: DocumentOption[] }> {
|
||||||
|
const [personResults, documentResults] = await Promise.all([
|
||||||
|
Promise.all(personIds.map((id) => api.GET('/api/persons/{id}', { params: { path: { id } } }))),
|
||||||
|
Promise.all(
|
||||||
|
documentIds.map((id) => api.GET('/api/documents/{id}', { params: { path: { id } } }))
|
||||||
|
)
|
||||||
|
]);
|
||||||
|
return {
|
||||||
|
persons: personResults.filter((r) => r.response.ok && r.data).map((r) => r.data!),
|
||||||
|
documents: documentResults
|
||||||
|
.filter((r) => r.response.ok && r.data)
|
||||||
|
.map((r) => ({
|
||||||
|
id: r.data!.id,
|
||||||
|
title: r.data!.title,
|
||||||
|
documentDate: r.data!.documentDate,
|
||||||
|
metaDatePrecision: r.data!.metaDatePrecision,
|
||||||
|
metaDateEnd: r.data!.metaDateEnd
|
||||||
|
}))
|
||||||
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Builds the TimelineEventRequest write body from parsed form fields. */
|
/** Builds the TimelineEventRequest write body from parsed form fields. */
|
||||||
|
|||||||
@@ -5,6 +5,8 @@ import { requireWriteAll } from '$lib/shared/server/permissions';
|
|||||||
import {
|
import {
|
||||||
parseEventForm,
|
parseEventForm,
|
||||||
validateEventForm,
|
validateEventForm,
|
||||||
|
preservedFormFields,
|
||||||
|
lookupSelections,
|
||||||
toEventRequest,
|
toEventRequest,
|
||||||
resolveNavTarget
|
resolveNavTarget
|
||||||
} from '$lib/timeline/eventFormServer';
|
} from '$lib/timeline/eventFormServer';
|
||||||
@@ -47,14 +49,21 @@ export const actions = {
|
|||||||
}) => {
|
}) => {
|
||||||
const formData = await request.formData();
|
const formData = await request.formData();
|
||||||
const parsed = parseEventForm(formData);
|
const parsed = parseEventForm(formData);
|
||||||
|
const api = createApiClient(fetch);
|
||||||
|
|
||||||
const invalid = validateEventForm(parsed);
|
const errors = validateEventForm(parsed);
|
||||||
if (invalid) return invalid;
|
if (errors) {
|
||||||
|
const { persons, documents } = await lookupSelections(
|
||||||
|
api,
|
||||||
|
parsed.personIds,
|
||||||
|
parsed.documentIds
|
||||||
|
);
|
||||||
|
return fail(400, { ...errors, ...preservedFormFields(parsed), persons, documents });
|
||||||
|
}
|
||||||
|
|
||||||
const versionRaw = formData.get('version')?.toString();
|
const versionRaw = formData.get('version')?.toString();
|
||||||
const version = versionRaw ? Number(versionRaw) : undefined;
|
const version = versionRaw ? Number(versionRaw) : undefined;
|
||||||
|
|
||||||
const api = createApiClient(fetch);
|
|
||||||
const result = await api.PUT('/api/timeline/events/{id}', {
|
const result = await api.PUT('/api/timeline/events/{id}', {
|
||||||
params: { path: { id: params.id } },
|
params: { path: { id: params.id } },
|
||||||
body: toEventRequest(parsed, version)
|
body: toEventRequest(parsed, version)
|
||||||
@@ -63,11 +72,7 @@ export const actions = {
|
|||||||
if (!result.response.ok) {
|
if (!result.response.ok) {
|
||||||
return fail(result.response.status, {
|
return fail(result.response.status, {
|
||||||
error: getErrorMessage(extractErrorCode(result.error)),
|
error: getErrorMessage(extractErrorCode(result.error)),
|
||||||
title: parsed.title,
|
...preservedFormFields(parsed)
|
||||||
description: parsed.description,
|
|
||||||
type: parsed.type,
|
|
||||||
personIds: parsed.personIds,
|
|
||||||
documentIds: parsed.documentIds
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -5,6 +5,8 @@ import { requireWriteAll } from '$lib/shared/server/permissions';
|
|||||||
import {
|
import {
|
||||||
parseEventForm,
|
parseEventForm,
|
||||||
validateEventForm,
|
validateEventForm,
|
||||||
|
preservedFormFields,
|
||||||
|
lookupSelections,
|
||||||
toEventRequest,
|
toEventRequest,
|
||||||
resolveNavTarget
|
resolveNavTarget
|
||||||
} from '$lib/timeline/eventFormServer';
|
} from '$lib/timeline/eventFormServer';
|
||||||
@@ -53,21 +55,24 @@ export async function load({
|
|||||||
export const actions = {
|
export const actions = {
|
||||||
save: async ({ request, fetch }: { request: Request; fetch: typeof globalThis.fetch }) => {
|
save: async ({ request, fetch }: { request: Request; fetch: typeof globalThis.fetch }) => {
|
||||||
const parsed = parseEventForm(await request.formData());
|
const parsed = parseEventForm(await request.formData());
|
||||||
|
|
||||||
const invalid = validateEventForm(parsed);
|
|
||||||
if (invalid) return invalid;
|
|
||||||
|
|
||||||
const api = createApiClient(fetch);
|
const api = createApiClient(fetch);
|
||||||
|
|
||||||
|
const errors = validateEventForm(parsed);
|
||||||
|
if (errors) {
|
||||||
|
const { persons, documents } = await lookupSelections(
|
||||||
|
api,
|
||||||
|
parsed.personIds,
|
||||||
|
parsed.documentIds
|
||||||
|
);
|
||||||
|
return fail(400, { ...errors, ...preservedFormFields(parsed), persons, documents });
|
||||||
|
}
|
||||||
|
|
||||||
const result = await api.POST('/api/timeline/events', { body: toEventRequest(parsed) });
|
const result = await api.POST('/api/timeline/events', { body: toEventRequest(parsed) });
|
||||||
|
|
||||||
if (!result.response.ok) {
|
if (!result.response.ok) {
|
||||||
return fail(result.response.status, {
|
return fail(result.response.status, {
|
||||||
error: getErrorMessage(extractErrorCode(result.error)),
|
error: getErrorMessage(extractErrorCode(result.error)),
|
||||||
title: parsed.title,
|
...preservedFormFields(parsed)
|
||||||
description: parsed.description,
|
|
||||||
type: parsed.type,
|
|
||||||
personIds: parsed.personIds,
|
|
||||||
documentIds: parsed.documentIds
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -150,9 +150,23 @@ describe('zeitstrahl/events/new save action (REQ-004/009/010/015)', () => {
|
|||||||
expect(post.mock.calls[0][1].body.precision).toBe('DAY');
|
expect(post.mock.calls[0][1].body.precision).toBe('DAY');
|
||||||
});
|
});
|
||||||
|
|
||||||
it('returns fail(400) with preserved picker arrays on blank title', async () => {
|
it('returns fail(400) with preserved + rehydrated pickers on blank title', async () => {
|
||||||
const post = vi.fn();
|
const post = vi.fn();
|
||||||
vi.mocked(createApiClient).mockReturnValue({ POST: post } as never);
|
// On validation failure the action re-fetches the selected persons/documents
|
||||||
|
// by id so the fail payload can rebuild full chips (Decision 6 / REQ-010).
|
||||||
|
const get = vi.fn((path: string, opts: { params: { path: { id: string } } }) => {
|
||||||
|
const id = opts.params.path.id;
|
||||||
|
if (path === '/api/persons/{id}')
|
||||||
|
return Promise.resolve({
|
||||||
|
response: { ok: true },
|
||||||
|
data: { id, displayName: `Person ${id}` }
|
||||||
|
});
|
||||||
|
return Promise.resolve({
|
||||||
|
response: { ok: true },
|
||||||
|
data: { id, title: `Doc ${id}`, documentDate: '1925-04-01' }
|
||||||
|
});
|
||||||
|
});
|
||||||
|
vi.mocked(createApiClient).mockReturnValue({ POST: post, GET: get } as never);
|
||||||
|
|
||||||
const result = await actions.save(
|
const result = await actions.save(
|
||||||
saveEvent({
|
saveEvent({
|
||||||
@@ -169,6 +183,12 @@ describe('zeitstrahl/events/new save action (REQ-004/009/010/015)', () => {
|
|||||||
expect(failData(result).personIds).toEqual(['p1', 'p2']);
|
expect(failData(result).personIds).toEqual(['p1', 'p2']);
|
||||||
expect(failData(result).documentIds).toEqual(['d1']);
|
expect(failData(result).documentIds).toEqual(['d1']);
|
||||||
expect(failData(result).titleError).toBeTruthy();
|
expect(failData(result).titleError).toBeTruthy();
|
||||||
|
// Rehydrated chips carry labels, not just ids.
|
||||||
|
expect(failData(result).persons).toEqual([
|
||||||
|
{ id: 'p1', displayName: 'Person p1' },
|
||||||
|
{ id: 'p2', displayName: 'Person p2' }
|
||||||
|
]);
|
||||||
|
expect((failData(result).documents as { id: string }[])[0].id).toBe('d1');
|
||||||
});
|
});
|
||||||
|
|
||||||
it('surfaces both title and date errors when both blank (REQ-011)', async () => {
|
it('surfaces both title and date errors when both blank (REQ-011)', async () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user