From be6800181d66592e68f6003fa128a25f239be201 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 16:51:32 -0500 Subject: [PATCH] fix(admin): let a setting whose default is empty actually be cleared (#280) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit intakeNotifyEmail and intakeCeilingResetAt both document empty as their default and as a working configuration — no notification address, and no ceiling reset recorded. The validator refused every empty text value, so either could be set and then never removed through the admin at all; the only way back was a DELETE against admin_settings. An admin who turned intake notifications on could not turn them off. Whether empty is a mistake is a fact about the setting rather than about its type, so it is now declared on the setting, in the DEFINITIONS row that already carries its type and fallback. A new setting states it once, in the place someone adding one is already editing, and nothing else has to know. That is what makes this different from special-casing two names in the validator, which would have left the next such setting to rediscover the same bug. The blanket refusal stays the default, because for a setting with a non-empty fallback an empty value really is a mistake: an empty greeting format renders every greeting as nothing at all, which reads as a broken email rather than as something a person cleared. Both those cases keep their tests. Whitespace is normalised to empty rather than stored. Somebody clearing a field they cannot see the end of leaves spaces behind, and they meant cleared. The tests check that the clearing survives the request rather than only being echoed back — the last one sets a value, clears it, and then reads it again through GET, which is the assertion that would have caught this had it existed. Closes #280 Co-Authored-By: Claude Opus 5 --- backend/src/adminSettings.ts | 18 +++++++- backend/src/routes/adminSettings.ts | 22 ++++++--- .../adminSettings.integration.test.ts | 45 +++++++++++++++++++ 3 files changed, 78 insertions(+), 7 deletions(-) diff --git a/backend/src/adminSettings.ts b/backend/src/adminSettings.ts index fec8e07..ce48ff4 100644 --- a/backend/src/adminSettings.ts +++ b/backend/src/adminSettings.ts @@ -40,7 +40,7 @@ const DEFINITIONS = [ // changed by whoever runs the shop, not by whoever deploys it, and a redeploy // to change an address would be absurd. Empty means do not notify, which is // the default and a working configuration. - { key: 'intake_notify_email', name: 'intakeNotifyEmail', type: 'text', fallback: '' }, + { key: 'intake_notify_email', name: 'intakeNotifyEmail', type: 'text', fallback: '', mayBeEmpty: true }, // The whole intake surface over a rolling 24 hours, across every link (#227). // Per-link caps bound each link, but links accumulate — twenty links at the // default 25 is five hundred submissions nobody decided to accept. @@ -57,7 +57,7 @@ const DEFINITIONS = [ // An ISO timestamp, or empty. The count is derived from rows that exist, so a // reset cannot delete anything — it moves the window's start instead, which // makes it an auditable fact rather than a deletion. - { key: 'intake_ceiling_reset_at', name: 'intakeCeilingResetAt', type: 'text', fallback: '' } + { key: 'intake_ceiling_reset_at', name: 'intakeCeilingResetAt', type: 'text', fallback: '', mayBeEmpty: true } ] as const; type Definition = (typeof DEFINITIONS)[number]; @@ -78,6 +78,20 @@ export const HOURS_SETTINGS: readonly HoursSettingName[] = DEFINITIONS.filter( (d): d is Extract => d.type === 'hours' ).map(d => d.name); +/** + * The text settings for which empty is a value rather than a mistake. + * + * Declared on the setting, beside its type and fallback, rather than in the + * validator — whether a setting may be cleared is a fact about that setting, + * and a new one should state it once in the row it already has. The blanket + * refusal stays the default, because for a setting with a non-empty fallback + * an empty value really is a mistake: an empty greeting format renders every + * greeting as nothing, which reads as a broken email. See #280. + */ +export function mayBeEmpty(name: SettingName): boolean { + return DEFINITIONS.some((d) => d.name === name && 'mayBeEmpty' in d && d.mayBeEmpty); +} + export const TEXT_SETTINGS: readonly TextSettingName[] = DEFINITIONS.filter( (d): d is Extract => d.type === 'text' ).map(d => d.name); diff --git a/backend/src/routes/adminSettings.ts b/backend/src/routes/adminSettings.ts index 1d715d1..aec9356 100644 --- a/backend/src/routes/adminSettings.ts +++ b/backend/src/routes/adminSettings.ts @@ -8,6 +8,7 @@ import { CHOICE_SETTINGS, CHOICE_OPTIONS, isValidChoice, + mayBeEmpty, SettingName } from '../adminSettings'; @@ -44,13 +45,24 @@ function readHours(name: SettingName, raw: unknown): Reading { function readText(name: SettingName, raw: unknown): Reading { if (raw === undefined) return SKIP; - if (typeof raw !== 'string' || raw.trim() === '') { - // Wrong for the two settings whose documented default is empty — - // intakeNotifyEmail and intakeCeilingResetAt cannot currently be cleared. - // Left as it was here deliberately: this change is the complexity refactor, - // and folding a behaviour fix into it would hide the fix. See #280. + if (typeof raw !== 'string') { return { ok: false, error: `${name} cannot be empty` }; } + if (raw.trim() === '') { + // Whether empty is a mistake is a fact about the setting, not about the + // type, so it is asked of the setting (#280). intakeNotifyEmail and + // intakeCeilingResetAt both document empty as their default and as a + // working configuration — meaning "do not notify" and "no reset recorded" — + // and the blanket rule meant an address could be set and never removed + // except by a DELETE against the table. + if (!mayBeEmpty(name)) { + return { ok: false, error: `${name} cannot be empty` }; + } + // Normalised, so whitespace is stored as cleared rather than as spaces. + // Someone clearing a field they cannot see the end of leaves whitespace, + // and they meant empty. + return { ok: true, value: '' }; + } return { ok: true, value: raw }; } diff --git a/backend/tests/integration/adminSettings.integration.test.ts b/backend/tests/integration/adminSettings.integration.test.ts index a76f720..8e3a971 100644 --- a/backend/tests/integration/adminSettings.integration.test.ts +++ b/backend/tests/integration/adminSettings.integration.test.ts @@ -95,6 +95,51 @@ describe('PUT /api/admin/settings', () => { expect(res.body.error).toContain(name); }); + /** + * The other half of the same question (#280). + * + * These two settings document empty as their default and as a working + * configuration — no notification address, and no ceiling reset recorded. The + * blanket "text cannot be empty" rule meant an address could be set and then + * never removed through the admin at all, only by a DELETE against the table. + */ + it.each(['intakeNotifyEmail', 'intakeCeilingResetAt'])( + 'lets %s be cleared, because empty is its documented default', + async (name) => { + const set = await request(app) + .put('/api/admin/settings') + .send({ [name]: name === 'intakeNotifyEmail' ? 'alerts@example.com' : '2026-09-01T00:00:00Z' }); + expect(set.status).toBe(200); + expect(set.body[name]).not.toBe(''); + + const cleared = await request(app).put('/api/admin/settings').send({ [name]: '' }); + + expect(cleared.status).toBe(200); + expect(cleared.body[name]).toBe(''); + } + ); + + // Whitespace is how a person clears a field they cannot see the end of, so it + // means cleared rather than being stored as spaces. + it('treats whitespace as cleared rather than storing it', async () => { + await request(app).put('/api/admin/settings').send({ intakeNotifyEmail: 'alerts@example.com' }); + + const res = await request(app).put('/api/admin/settings').send({ intakeNotifyEmail: ' ' }); + + expect(res.status).toBe(200); + expect(res.body.intakeNotifyEmail).toBe(''); + }); + + // The clearing is real, not just echoed back in the response. + it('reads a cleared setting back as empty on a later request', async () => { + await request(app).put('/api/admin/settings').send({ intakeNotifyEmail: 'alerts@example.com' }); + await request(app).put('/api/admin/settings').send({ intakeNotifyEmail: '' }); + + const res = await request(app).get('/api/admin/settings'); + + expect(res.body.intakeNotifyEmail).toBe(''); + }); + // Every field is validated before any is written, so a request that is part // nonsense does not half-apply. it('does not write anything when one field in the request is invalid', async () => {