Merge pull request 'fix(admin): let a setting whose default is empty actually be cleared (#280)' (#290) from fix/280-clearable-text-settings into main

Reviewed-on: #290

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<Definition, { type: 'hours' }> => 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<Definition, { type: 'text' }> => d.type === 'text'
 ).map(d => d.name);

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 };
 }
 

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 () => {