fix(admin): let a setting whose default is empty actually be cleared (#280) #290
@@ -40,7 +40,7 @@ const DEFINITIONS = [
|
|||||||
// changed by whoever runs the shop, not by whoever deploys it, and a redeploy
|
// 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
|
// to change an address would be absurd. Empty means do not notify, which is
|
||||||
// the default and a working configuration.
|
// 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).
|
// 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
|
// Per-link caps bound each link, but links accumulate — twenty links at the
|
||||||
// default 25 is five hundred submissions nobody decided to accept.
|
// 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
|
// 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
|
// reset cannot delete anything — it moves the window's start instead, which
|
||||||
// makes it an auditable fact rather than a deletion.
|
// 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;
|
] as const;
|
||||||
|
|
||||||
type Definition = (typeof DEFINITIONS)[number];
|
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'
|
(d): d is Extract<Definition, { type: 'hours' }> => d.type === 'hours'
|
||||||
).map(d => d.name);
|
).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(
|
export const TEXT_SETTINGS: readonly TextSettingName[] = DEFINITIONS.filter(
|
||||||
(d): d is Extract<Definition, { type: 'text' }> => d.type === 'text'
|
(d): d is Extract<Definition, { type: 'text' }> => d.type === 'text'
|
||||||
).map(d => d.name);
|
).map(d => d.name);
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ import {
|
|||||||
CHOICE_SETTINGS,
|
CHOICE_SETTINGS,
|
||||||
CHOICE_OPTIONS,
|
CHOICE_OPTIONS,
|
||||||
isValidChoice,
|
isValidChoice,
|
||||||
|
mayBeEmpty,
|
||||||
SettingName
|
SettingName
|
||||||
} from '../adminSettings';
|
} from '../adminSettings';
|
||||||
|
|
||||||
@@ -44,13 +45,24 @@ function readHours(name: SettingName, raw: unknown): Reading {
|
|||||||
|
|
||||||
function readText(name: SettingName, raw: unknown): Reading {
|
function readText(name: SettingName, raw: unknown): Reading {
|
||||||
if (raw === undefined) return SKIP;
|
if (raw === undefined) return SKIP;
|
||||||
if (typeof raw !== 'string' || raw.trim() === '') {
|
if (typeof raw !== 'string') {
|
||||||
// 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.
|
|
||||||
return { ok: false, error: `${name} cannot be empty` };
|
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 };
|
return { ok: true, value: raw };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -95,6 +95,51 @@ describe('PUT /api/admin/settings', () => {
|
|||||||
expect(res.body.error).toContain(name);
|
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
|
// Every field is validated before any is written, so a request that is part
|
||||||
// nonsense does not half-apply.
|
// nonsense does not half-apply.
|
||||||
it('does not write anything when one field in the request is invalid', async () => {
|
it('does not write anything when one field in the request is invalid', async () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user