diff --git a/backend/src/adminSettings.ts b/backend/src/adminSettings.ts new file mode 100644 index 0000000..08dcf0a --- /dev/null +++ b/backend/src/adminSettings.ts @@ -0,0 +1,81 @@ +import { pool } from './db'; + +/** + * Every admin-configurable setting, in one table. + * + * `cart_expiry_hours` used to be read by an inline query in two places, each + * with its own `|| '24'`. With several settings and read sites scattered across + * routes and the cron job that stops being tenable: a default written twice is + * a default that will eventually disagree with itself. Adding a setting means + * adding a row here and nothing else. + * + * Values are stored as text, so each row declares how to read it back. Numbers + * were the only kind until the greeting format arrived; typing it per setting + * rather than assuming means the next string one costs nothing. + */ +const DEFINITIONS = [ + { key: 'cart_expiry_hours', name: 'cartExpiryHours', type: 'hours', fallback: 24 }, + { key: 'verify_token_hours', name: 'verifyTokenHours', type: 'hours', fallback: 24 }, + { key: 'password_reset_hours', name: 'passwordResetHours', type: 'hours', fallback: 1 }, + { key: 'greeting_format', name: 'greetingFormat', type: 'text', fallback: 'Hi {{firstName}},' }, + { key: 'greeting_fallback', name: 'greetingFallback', type: 'text', fallback: 'Hi,' } +] as const; + +type Definition = (typeof DEFINITIONS)[number]; + +export type SettingName = Definition['name']; + +export type HoursSettingName = Extract['name']; +export type TextSettingName = Extract['name']; + +export type AdminSettings = Record & Record; + +export const HOURS_SETTINGS: readonly HoursSettingName[] = DEFINITIONS.filter( + (d): d is Extract => d.type === 'hours' +).map(d => d.name); + +export const TEXT_SETTINGS: readonly TextSettingName[] = DEFINITIONS.filter( + (d): d is Extract => d.type === 'text' +).map(d => d.name); + +export async function getSettings(): Promise { + const { rows } = await pool.query(`SELECT key, value FROM admin_settings`); + const stored = new Map(rows.map(r => [r.key, r.value])); + + const settings = {} as Record; + for (const { key, name, type, fallback } of DEFINITIONS) { + const raw = stored.get(key); + if (type === 'text') { + // An empty format would render every greeting as nothing at all, which + // reads as a bug in the email rather than a setting someone cleared. + settings[name] = raw !== undefined && raw.trim() !== '' ? raw : fallback; + continue; + } + const parsed = parseFloat(raw ?? ''); + // A row that is present but unparseable falls back rather than yielding + // NaN, which would otherwise reach Date arithmetic and mint a token with an + // Invalid Date expiry that no query could ever match. + settings[name] = Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; + } + return settings as AdminSettings; +} + +/** + * Writes the supplied settings, ignoring names that were not sent. + * + * Partial rather than whole-object so a caller updating one field does not have + * to know the current value of the others to avoid clobbering them. + */ +export async function updateSettings( + values: Partial> +): Promise { + for (const { key, name } of DEFINITIONS) { + const value = values[name]; + if (value === undefined) continue; + await pool.query( + `INSERT INTO admin_settings (key, value, updated_at) VALUES ($1, $2, now()) + ON CONFLICT (key) DO UPDATE SET value = $2, updated_at = now()`, + [key, String(value)] + ); + } +} diff --git a/backend/src/emailTemplates.ts b/backend/src/emailTemplates.ts index 59e6945..404498f 100644 --- a/backend/src/emailTemplates.ts +++ b/backend/src/emailTemplates.ts @@ -49,30 +49,30 @@ export const TEMPLATES: Record = { verification: { label: 'Email verification', required: ['verifyUrl'], - available: ['greeting', 'verifyUrl'], + available: ['greeting', 'firstName', 'lastName', 'verifyUrl', 'expiresIn'], defaultSubject: 'Confirm your email address', defaultBody: '{{greeting}}\n\n' + 'Please confirm this address so we know we can reach you.\n\n' + '[Confirm my email]({{verifyUrl}})\n\n' + - 'This link expires in 24 hours.' + 'This link expires in {{expiresIn}}.' }, passwordReset: { label: 'Password reset', required: ['resetUrl'], - available: ['greeting', 'resetUrl'], + available: ['greeting', 'firstName', 'lastName', 'resetUrl', 'expiresIn'], defaultSubject: 'Reset your Redefined Designs password', defaultBody: 'Someone asked to reset the password for this account.\n\n' + - '[Choose a new password]({{resetUrl}}). This link expires in one hour.\n\n' + + '[Choose a new password]({{resetUrl}}). This link expires in {{expiresIn}}.\n\n' + "If this wasn't you, you can ignore this email — your password has not changed." }, favoriteSold: { label: 'Favorited item sold', required: ['itemName'], - available: ['itemName', 'siteUrl'], + available: ['greeting', 'firstName', 'lastName', 'itemName', 'siteUrl'], defaultSubject: '"{{itemName}}" has been sold', defaultBody: 'An item you favorited has been sold to another customer, so it is no longer available.\n\n' + @@ -85,7 +85,7 @@ export const TEMPLATES: Record = { favoriteWithdrawn: { label: 'Favorited item withdrawn', required: ['itemName'], - available: ['itemName', 'siteUrl'], + available: ['greeting', 'firstName', 'lastName', 'itemName', 'siteUrl'], defaultSubject: '"{{itemName}}" is no longer available', defaultBody: 'An item you favorited has been withdrawn and is no longer available.\n\n' + @@ -101,7 +101,7 @@ export const TEMPLATES: Record = { // whether it was them. This is the mail that catches an account // takeover, so it goes to the address being replaced. required: ['newEmail'], - available: ['greeting', 'newEmail'], + available: ['greeting', 'firstName', 'lastName', 'newEmail'], defaultSubject: 'Your Redefined Designs email address was changed', defaultBody: '{{greeting}}\n\n' + @@ -114,27 +114,57 @@ export const TEMPLATES: Record = { cartReminder: { label: 'Cart reminder', required: ['itemList', 'cartUrl'], - available: ['greeting', 'itemList', 'cartUrl'], + available: ['greeting', 'firstName', 'lastName', 'itemList', 'cartUrl', 'holdDuration'], defaultSubject: 'Items waiting in your cart', defaultBody: '{{greeting}}\n\n' + 'You still have items in your cart at Redefined Designs:\n\n' + '{{itemList}}\n\n' + + 'Items are held for {{holdDuration}} from when they were added.\n\n' + '[View your cart]({{cartUrl}}) before your reservation expires.' } }; /** - * The `{{greeting}}` value: "Hi Thom," when a first name is known, "Hi," - * otherwise. + * Renders a configured lifetime, in hours, as the words an email should use. + * + * All three duration placeholders go through this, so the reset email and the + * cart reminder say "one hour" the same way rather than in two authors' + * phrasing. A fractional hour drops to minutes: "0.5 hours" reads badly, and + * "1.5 hours" reads worse in a sentence a customer is meant to act on. + */ +export function formatDuration(hours: number): string { + if (!Number.isInteger(hours)) { + return `${Math.round(hours * 60)} minutes`; + } + return hours === 1 ? 'one hour' : `${hours} hours`; +} + +/** + * Builds the `{{greeting}}` value from the admin-configured format. * * One placeholder rather than a bare name, so a template author writes * `{{greeting}}` on its own line instead of `Hi {{firstName}},` — which reads * as "Hi ," for anyone who registered before first names were required (#106). + * `{{firstName}}` and `{{lastName}}` are still offered for a template that + * genuinely wants the name inline, but the greeting is the safe default. + * + * The fallback is a separate setting rather than the format with the name + * removed. Editing a name out of a sentence is the kind of thing that has to be + * right every time and cannot be, so an admin writes both and neither is + * guessed. */ -export function greeting(firstName: string | null | undefined): string { - const name = (firstName ?? '').trim(); - return name ? `Hi ${name},` : 'Hi,'; +export function greeting( + firstName: string | null | undefined, + format: string, + fallback: string, + lastName?: string | null +): string { + const first = (firstName ?? '').trim(); + if (!first) return fallback; + return format + .replace(/\{\{\s*firstName\s*\}\}/g, first) + .replace(/\{\{\s*lastName\s*\}\}/g, (lastName ?? '').trim()); } /** Matches `{{name}}`, tolerating whitespace inside the braces. */ @@ -175,13 +205,20 @@ function substitute(text: string, values: Record): string { */ export const SAMPLE_VALUES: Record = { greeting: 'Hi Ada,', + firstName: 'Ada', + lastName: 'Lovelace', verifyUrl: 'https://example.com/verify-email?token=sample-token', resetUrl: 'https://example.com/reset-password?token=sample-token', itemName: 'Walnut sideboard', siteUrl: 'https://example.com', newEmail: 'new.address@example.com', itemList: '- Walnut sideboard\n- Brass table lamp', - cartUrl: 'https://example.com/cart' + cartUrl: 'https://example.com/cart', + // Fallbacks only. The admin preview overrides both from the live settings, + // so the pane shows the duration that would actually be sent rather than a + // plausible-looking number that disagrees with it. + expiresIn: 'one hour', + holdDuration: '24 hours' }; export interface StoredTemplate { diff --git a/backend/src/favoriteAlerts.ts b/backend/src/favoriteAlerts.ts index b9f2c36..37e2cb4 100644 --- a/backend/src/favoriteAlerts.ts +++ b/backend/src/favoriteAlerts.ts @@ -1,6 +1,7 @@ import { pool } from './db'; import { sendMail } from './mailer'; -import { renderTemplate, TemplateKey } from './emailTemplates'; +import { renderTemplate, greeting, TemplateKey } from './emailTemplates'; +import { getSettings } from './adminSettings'; import { loadStoredTemplate } from './routes/adminEmailTemplates'; // Shown to the customer when they opt in, and stored verbatim against their @@ -11,6 +12,10 @@ export const FAVORITE_ALERTS_CONSENT_TEXT = export interface FavoriteRecipient { email: string; + // Nullable because customers who registered while names were optional have + // none — the same reason the greeting needs a fallback at all. + first_name: string | null; + last_name: string | null; item_name: string; } @@ -26,7 +31,7 @@ export async function collectFavoriteRecipients( if (!itemIds.length) return []; const { rows } = await pool.query( - `SELECT c.email, i.name AS item_name + `SELECT c.email, c.first_name, c.last_name, i.name AS item_name FROM favorites f JOIN customers c ON c.id = f.customer_id JOIN items i ON i.id = f.item_id @@ -51,9 +56,13 @@ async function send(recipients: FavoriteRecipient[], key: TemplateKey): Promise< // for everyone, only the item name differs. const stored = await loadStoredTemplate(key); const siteUrl = process.env.PUBLIC_URL ?? ''; + const { greetingFormat, greetingFallback } = await getSettings(); for (const recipient of recipients) { const { subject, html } = renderTemplate(key, stored, { + greeting: greeting(recipient.first_name, greetingFormat, greetingFallback, recipient.last_name), + firstName: recipient.first_name ?? '', + lastName: recipient.last_name ?? '', itemName: recipient.item_name, siteUrl }); diff --git a/backend/src/routes/adminEmailTemplates.ts b/backend/src/routes/adminEmailTemplates.ts index 4b06c3e..ecef93c 100644 --- a/backend/src/routes/adminEmailTemplates.ts +++ b/backend/src/routes/adminEmailTemplates.ts @@ -7,8 +7,11 @@ import { StoredTemplate, missingPlaceholders, renderTemplate, + formatDuration, + greeting, SAMPLE_VALUES } from '../emailTemplates'; +import { getSettings } from '../adminSettings'; const router = Router(); @@ -73,6 +76,35 @@ router.get('/', asyncRoute(async (_req: Request, res: Response) => { // // Deliberately does not enforce required placeholders. Saving refuses a body // that dropped one; previewing it is how an admin sees what they have done. +/** + * The sample values, with the three duration placeholders replaced by what the + * settings actually hold. + * + * The preview exists so an admin sees the email that will be sent. A duration + * drawn from a static sample would show "one hour" while the setting said two, + * which is the precise failure this placeholder was added to remove. + */ +async function previewValues(key: TemplateKey): Promise> { + const { + cartExpiryHours, + verifyTokenHours, + passwordResetHours, + greetingFormat, + greetingFallback + } = await getSettings(); + // `expiresIn` names one placeholder but two different lifetimes, so the value + // depends on which template is being previewed. The route knows the key. + const expiresIn = key === 'passwordReset' ? passwordResetHours : verifyTokenHours; + return { + ...SAMPLE_VALUES, + holdDuration: formatDuration(cartExpiryHours), + expiresIn: formatDuration(expiresIn), + // Built from the configured format for the same reason as the durations: + // the preview is meant to show the email that will be sent. + greeting: greeting(SAMPLE_VALUES.firstName, greetingFormat, greetingFallback, SAMPLE_VALUES.lastName) + }; +} + router.post('/:key/preview', asyncRoute(async (req: Request, res: Response) => { const key = req.params.key; if (!isTemplateKey(key)) { @@ -86,7 +118,7 @@ router.post('/:key/preview', asyncRoute(async (req: Request, res: Response) => { subject: typeof subject === 'string' ? subject : null, body: typeof body === 'string' ? body : null }, - SAMPLE_VALUES + await previewValues(key) ); res.json(rendered); diff --git a/backend/src/routes/adminSettings.ts b/backend/src/routes/adminSettings.ts index 02a8e9c..68c0890 100644 --- a/backend/src/routes/adminSettings.ts +++ b/backend/src/routes/adminSettings.ts @@ -1,30 +1,45 @@ import { Router, Request, Response } from 'express'; -import { pool } from '../db'; import { asyncRoute } from '../asyncRoute'; +import { + getSettings, + updateSettings, + HOURS_SETTINGS, + TEXT_SETTINGS, + SettingName +} from '../adminSettings'; const router = Router(); router.get('/', asyncRoute(async (_req: Request, res: Response) => { - const { rows } = await pool.query(`SELECT key, value FROM admin_settings`); - const map: Record = {}; - for (const r of rows) map[r.key] = r.value; - res.json({ - cartExpiryHours: parseFloat(map.cart_expiry_hours || '24') - }); + res.json(await getSettings()); })); router.put('/', asyncRoute(async (req: Request, res: Response) => { - const { cartExpiryHours } = req.body; - const hours = parseFloat(cartExpiryHours); - if (Number.isNaN(hours) || hours <= 0) { - return res.status(400).json({ error: 'cartExpiryHours must be a positive number' }); + const values: Partial> = {}; + + // Only what was sent is validated and written, so a caller updating one field + // does not have to echo the others back to avoid clobbering them. + for (const name of HOURS_SETTINGS) { + const raw = req.body[name]; + if (raw === undefined) continue; + const hours = parseFloat(raw); + if (Number.isNaN(hours) || hours <= 0) { + return res.status(400).json({ error: `${name} must be a positive number` }); + } + values[name] = hours; } - await pool.query( - `INSERT INTO admin_settings (key, value, updated_at) VALUES ('cart_expiry_hours', $1, now()) - ON CONFLICT (key) DO UPDATE SET value = $1, updated_at = now()`, - [String(hours)] - ); - res.json({ cartExpiryHours: hours }); + + for (const name of TEXT_SETTINGS) { + const raw = req.body[name]; + if (raw === undefined) continue; + if (typeof raw !== 'string' || raw.trim() === '') { + return res.status(400).json({ error: `${name} cannot be empty` }); + } + values[name] = raw; + } + + await updateSettings(values); + res.json(await getSettings()); })); export default router; diff --git a/backend/src/routes/cart.ts b/backend/src/routes/cart.ts index b40578a..cc5be3a 100644 --- a/backend/src/routes/cart.ts +++ b/backend/src/routes/cart.ts @@ -2,14 +2,10 @@ import { Router, Request, Response } from 'express'; import { pool } from '../db'; import { asyncRoute } from '../asyncRoute'; import { requireCustomer } from '../middleware/customerAuth'; +import { getSettings } from '../adminSettings'; const router = Router(); -async function getCartExpiryHours(): Promise { - const { rows } = await pool.query(`SELECT value FROM admin_settings WHERE key = 'cart_expiry_hours'`); - return rows.length ? parseFloat(rows[0].value) : 24; -} - const CART_ITEM_SELECT = ` SELECT ci.item_id, ci.added_at, ci.expires_at, @@ -60,8 +56,8 @@ router.post('/items/:itemId', requireCustomer, asyncRoute(async (req: Request, r cartId = newCart[0].id; } - const hours = await getCartExpiryHours(); - const expiresAt = new Date(Date.now() + hours * 60 * 60 * 1000); + const { cartExpiryHours } = await getSettings(); + const expiresAt = new Date(Date.now() + cartExpiryHours * 60 * 60 * 1000); await client.query( `INSERT INTO cart_items (cart_id, item_id, expires_at) VALUES ($1, $2, $3)`, [cartId, itemId, expiresAt] diff --git a/backend/src/routes/customers.ts b/backend/src/routes/customers.ts index 9364729..d7c33b0 100755 --- a/backend/src/routes/customers.ts +++ b/backend/src/routes/customers.ts @@ -4,7 +4,8 @@ import crypto from 'node:crypto'; import { pool } from '../db'; import { requireCustomer } from '../middleware/customerAuth'; import { sendMail } from '../mailer'; -import { renderTemplate, greeting } from '../emailTemplates'; +import { renderTemplate, greeting, formatDuration } from '../emailTemplates'; +import { getSettings } from '../adminSettings'; import { loadStoredTemplate } from './adminEmailTemplates'; import { MARKETING_CONSENT_TEXT, isValidEmail } from '../utils'; import { FAVORITE_ALERTS_CONSENT_TEXT } from '../favoriteAlerts'; @@ -15,8 +16,6 @@ const router = Router(); const SESSION_DAYS = 30; -const VERIFY_TOKEN_TTL_MS = 24 * 60 * 60 * 1000; - // Registration, changing an address, and resending all need the same three // steps: supersede any outstanding link, mint a new one, send it. Written out // three times they would drift, and the step most likely to be forgotten is the @@ -29,21 +28,26 @@ const VERIFY_TOKEN_TTL_MS = 24 * 60 * 60 * 1000; async function issueVerificationEmail( customerId: number, email: string, - firstName: string | null + firstName: string | null, + lastName: string | null = null ): Promise { await pool.query( `DELETE FROM customer_tokens WHERE customer_id = $1 AND kind = 'verify_email'`, [customerId] ); + const { verifyTokenHours, greetingFormat, greetingFallback } = await getSettings(); const token = crypto.randomBytes(24).toString('hex'); await pool.query( `INSERT INTO customer_tokens (token, customer_id, kind, expires_at) VALUES ($1, $2, 'verify_email', $3)`, - [token, customerId, new Date(Date.now() + VERIFY_TOKEN_TTL_MS)] + [token, customerId, new Date(Date.now() + verifyTokenHours * 60 * 60 * 1000)] ); const verifyUrl = `${process.env.PUBLIC_URL}/verify-email?token=${token}`; const template = renderTemplate('verification', await loadStoredTemplate('verification'), { - greeting: greeting(firstName), - verifyUrl + greeting: greeting(firstName, greetingFormat, greetingFallback, lastName), + firstName: firstName ?? '', + lastName: lastName ?? '', + verifyUrl, + expiresIn: formatDuration(verifyTokenHours) }); sendMail(email, template.subject, template.html) .catch(err => console.error('verify email send failed', err)); @@ -133,7 +137,7 @@ router.post('/register', asyncRoute(async (req: Request, res: Response) => { ); const customer = rows[0]; - await issueVerificationEmail(customer.id, customer.email, customer.first_name); + await issueVerificationEmail(customer.id, customer.email, customer.first_name, customer.last_name); const sessionToken = await createSession(customer.id); setSessionCookie(res, sessionToken); @@ -169,12 +173,11 @@ router.post( return res.status(400).json({ error: 'your email address is already verified' }); } - await issueVerificationEmail(customer.id, customer.email, customer.first_name); + await issueVerificationEmail(customer.id, customer.email, customer.first_name, customer.last_name); res.status(204).end(); }) ); -const RESET_TOKEN_TTL_MS = 60 * 60 * 1000; // Always answers 200, whether or not the address has an account. A response // that differed would let anyone test addresses for membership. @@ -195,16 +198,20 @@ router.post('/request-password-reset', passwordResetRequestLimiter, asyncRoute(a // from an older message in the customer's inbox. await pool.query(`DELETE FROM customer_tokens WHERE customer_id = $1 AND kind = 'password_reset'`, [customer.id]); + const { passwordResetHours, greetingFormat, greetingFallback } = await getSettings(); const token = crypto.randomBytes(32).toString('hex'); await pool.query( `INSERT INTO customer_tokens (token, customer_id, kind, expires_at) VALUES ($1, $2, 'password_reset', $3)`, - [token, customer.id, new Date(Date.now() + RESET_TOKEN_TTL_MS)] + [token, customer.id, new Date(Date.now() + passwordResetHours * 60 * 60 * 1000)] ); const resetUrl = `${process.env.PUBLIC_URL}/reset-password?token=${token}`; const resetTemplate = renderTemplate('passwordReset', await loadStoredTemplate('passwordReset'), { - greeting: greeting(customer.first_name), - resetUrl + greeting: greeting(customer.first_name, greetingFormat, greetingFallback, customer.last_name), + firstName: customer.first_name ?? '', + lastName: customer.last_name ?? '', + resetUrl, + expiresIn: formatDuration(passwordResetHours) }); sendMail(customer.email, resetTemplate.subject, resetTemplate.html) .catch(err => console.error('password reset email send failed', err)); @@ -437,10 +444,13 @@ router.put('/me/email', requireCustomer, asyncRoute(async (req: Request, res: Re // Both sends happen after the row is written, never before — the same rule // favoriteAlerts follows, so a change that failed cannot produce mail saying // it succeeded. - await issueVerificationEmail(req.customerId as number, normalized, customer.first_name); + await issueVerificationEmail(req.customerId as number, normalized, customer.first_name, customer.last_name); + const { greetingFormat, greetingFallback } = await getSettings(); const notice = renderTemplate('emailChanged', await loadStoredTemplate('emailChanged'), { - greeting: greeting(customer.first_name), + greeting: greeting(customer.first_name, greetingFormat, greetingFallback, customer.last_name), + firstName: customer.first_name ?? '', + lastName: customer.last_name ?? '', newEmail: normalized }); sendMail(previousEmail, notice.subject, notice.html) diff --git a/backend/src/server.ts b/backend/src/server.ts index 397207f..c88abc2 100755 --- a/backend/src/server.ts +++ b/backend/src/server.ts @@ -2,7 +2,8 @@ import cron from 'node-cron'; import app from './app'; import { pool } from './db'; import { sendMail } from './mailer'; -import { renderTemplate, greeting } from './emailTemplates'; +import { renderTemplate, greeting, formatDuration } from './emailTemplates'; +import { getSettings } from './adminSettings'; import { loadStoredTemplate } from './routes/adminEmailTemplates'; import { validateEnv } from './envValidation'; @@ -25,7 +26,7 @@ async function sweepExpiredCarts(): Promise { async function sendCartReminders(): Promise { try { const { rows } = await pool.query(` - SELECT c.email, c.first_name, i.name AS item_name, ci.expires_at, ci.id AS cart_item_id + SELECT c.email, c.first_name, c.last_name, i.name AS item_name, ci.expires_at, ci.id AS cart_item_id FROM cart_items ci JOIN carts ca ON ca.id = ci.cart_id JOIN customers c ON c.id = ca.customer_id @@ -35,15 +36,17 @@ async function sendCartReminders(): Promise { AND ci.expires_at > now() `); - const byEmail = new Map(); + const byEmail = new Map(); for (const row of rows) { - if (!byEmail.has(row.email)) byEmail.set(row.email, { firstName: row.first_name, items: [] }); + if (!byEmail.has(row.email)) byEmail.set(row.email, { firstName: row.first_name, lastName: row.last_name, items: [] }); byEmail.get(row.email)!.items.push({ name: row.item_name, expiresAt: row.expires_at, cartItemId: row.cart_item_id }); } // Loaded once rather than per recipient: the copy is shared, only the // greeting and the item list differ. const stored = await loadStoredTemplate('cartReminder'); + const { cartExpiryHours, greetingFormat, greetingFallback } = await getSettings(); + const holdDuration = formatDuration(cartExpiryHours); for (const [email, data] of byEmail) { // Markdown, not HTML. Values are substituted into the template source @@ -54,9 +57,12 @@ async function sendCartReminders(): Promise { .join('\n'); const { subject, html } = renderTemplate('cartReminder', stored, { - greeting: greeting(data.firstName), + greeting: greeting(data.firstName, greetingFormat, greetingFallback, data.lastName), + firstName: data.firstName ?? '', + lastName: data.lastName ?? '', itemList, - cartUrl: `${process.env.PUBLIC_URL}/cart` + cartUrl: `${process.env.PUBLIC_URL}/cart`, + holdDuration }); await sendMail(email, subject, html); const ids = data.items.map(i => i.cartItemId); diff --git a/backend/tests/integration/adminSettings.integration.test.ts b/backend/tests/integration/adminSettings.integration.test.ts new file mode 100644 index 0000000..69df181 --- /dev/null +++ b/backend/tests/integration/adminSettings.integration.test.ts @@ -0,0 +1,103 @@ +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; +import { getSettings } from '../../src/adminSettings'; + +// resetDb only clears the stored email templates out of admin_settings — it +// matches `email\_%`. The settings themselves survive it, so without this a +// value written by one test is the baseline the next one reads. Cleared after +// the file too, so nothing leaks into the suites that run behind it. +beforeEach(async () => { + await resetDb(); + await pool.query(`DELETE FROM admin_settings`); +}); + +afterAll(async () => { + await pool.query(`DELETE FROM admin_settings`); + await pool.end(); + await closeDb(); +}); + +describe('GET /api/admin/settings', () => { + it('answers with every setting, falling back for the ones never written', async () => { + const res = await request(app).get('/api/admin/settings'); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ + cartExpiryHours: 24, + verifyTokenHours: 24, + passwordResetHours: 1, + greetingFormat: 'Hi {{firstName}},', + greetingFallback: 'Hi,' + }); + }); +}); + +describe('PUT /api/admin/settings', () => { + it('stores the lifetimes and reads them back', async () => { + const res = await request(app) + .put('/api/admin/settings') + .send({ verifyTokenHours: 2, passwordResetHours: 0.5 }); + + expect(res.status).toBe(200); + expect(res.body.verifyTokenHours).toBe(2); + expect(res.body.passwordResetHours).toBe(0.5); + // Read back through the accessor the senders use, not just the response. + expect((await getSettings()).verifyTokenHours).toBe(2); + }); + + // Partial rather than whole-object: a caller updating one field should not + // have to echo the others back to avoid clobbering them. + it('leaves settings it was not sent alone', async () => { + await request(app).put('/api/admin/settings').send({ cartExpiryHours: 6 }); + await request(app).put('/api/admin/settings').send({ verifyTokenHours: 3 }); + + const { cartExpiryHours, verifyTokenHours } = await getSettings(); + expect(cartExpiryHours).toBe(6); + expect(verifyTokenHours).toBe(3); + }); + + it('stores the greeting format and its fallback', async () => { + const res = await request(app) + .put('/api/admin/settings') + .send({ greetingFormat: 'Dear {{firstName}} {{lastName}}:', greetingFallback: 'Hello there,' }); + + expect(res.status).toBe(200); + expect(res.body.greetingFormat).toBe('Dear {{firstName}} {{lastName}}:'); + expect(res.body.greetingFallback).toBe('Hello there,'); + }); + + it.each([ + ['cartExpiryHours', 0], + ['verifyTokenHours', -1], + ['passwordResetHours', 'soon'] + ])('refuses %s of %p, naming the field', async (name, value) => { + const res = await request(app).put('/api/admin/settings').send({ [name]: value }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain(name); + }); + + // An empty format would render every greeting as nothing at all, which reads + // as a broken email rather than a setting someone cleared. + it.each(['greetingFormat', 'greetingFallback'])('refuses an empty %s', async (name) => { + const res = await request(app).put('/api/admin/settings').send({ [name]: ' ' }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain(name); + }); + + // 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 () => { + await request(app).put('/api/admin/settings').send({ cartExpiryHours: 6 }); + + const res = await request(app) + .put('/api/admin/settings') + .send({ cartExpiryHours: 8, verifyTokenHours: -3 }); + + expect(res.status).toBe(400); + expect((await getSettings()).cartExpiryHours).toBe(6); + }); +}); diff --git a/backend/tests/unit/emailTemplates.test.ts b/backend/tests/unit/emailTemplates.test.ts index c108bf2..8311525 100644 --- a/backend/tests/unit/emailTemplates.test.ts +++ b/backend/tests/unit/emailTemplates.test.ts @@ -3,6 +3,8 @@ import { TemplateKey, missingPlaceholders, renderTemplate, + formatDuration, + greeting, SAMPLE_VALUES } from '../../src/emailTemplates'; @@ -142,7 +144,12 @@ describe('renderTemplate', () => { const { html, subject } = renderTemplate( 'cartReminder', {}, - { greeting: 'Hi Thom,', itemList: '- One thing', cartUrl: 'https://shop.test/cart' } + { + greeting: 'Hi Thom,', + itemList: '- One thing', + cartUrl: 'https://shop.test/cart', + holdDuration: '24 hours' + } ); expect(html).not.toMatch(/\{\{\s*\w+\s*\}\}/); expect(subject).not.toMatch(/\{\{\s*\w+\s*\}\}/); @@ -164,3 +171,106 @@ describe('SAMPLE_VALUES, which the admin preview renders with', () => { expect(subject).not.toMatch(/\{\{\s*\w+\s*\}\}/); }); }); + +describe('formatDuration, which puts a configured lifetime into email copy', () => { + // The three templates that mention a duration all render it through this, so + // "one hour" in the reset email and "one hour" in the cart reminder are the + // same string produced the same way rather than two authors' phrasing. + it('spells a single hour rather than printing a numeral', () => { + expect(formatDuration(1)).toBe('one hour'); + }); + + it('counts whole hours', () => { + expect(formatDuration(2)).toBe('2 hours'); + expect(formatDuration(24)).toBe('24 hours'); + }); + + // A fractional hour reads badly as "0.5 hours", and worse as "1.5 hours" in a + // sentence a customer is meant to act on. + it('drops to minutes for anything that is not a whole number of hours', () => { + expect(formatDuration(0.5)).toBe('30 minutes'); + expect(formatDuration(0.25)).toBe('15 minutes'); + expect(formatDuration(1.5)).toBe('90 minutes'); + }); +}); + +describe('the duration placeholders', () => { + it('offers expiresIn on the two templates that carry a link with a lifetime', () => { + expect(TEMPLATES.verification.available).toContain('expiresIn'); + expect(TEMPLATES.passwordReset.available).toContain('expiresIn'); + }); + + it('offers holdDuration on the cart reminder', () => { + expect(TEMPLATES.cartReminder.available).toContain('holdDuration'); + }); + + // Required would reject every template an admin saved before this existed, + // and the whole point is that their copy keeps sending. + it.each([ + ['verification', 'expiresIn'], + ['passwordReset', 'expiresIn'], + ['cartReminder', 'holdDuration'] + ] as const)('does not make %s require %s', (key, name) => { + expect(TEMPLATES[key].required).not.toContain(name); + }); + + it('renders a body saved before the placeholder existed, unchanged', () => { + const { html } = renderTemplate( + 'passwordReset', + { subject: 'Reset it', body: 'Go [here]({{resetUrl}}). This link expires in one hour.' }, + { resetUrl: 'https://shop.test/r', expiresIn: 'two hours' } + ); + expect(html).toContain('one hour'); + expect(html).not.toMatch(/\{\{\s*\w+\s*\}\}/); + }); + + it('substitutes the duration when a body does use the placeholder', () => { + const { html } = renderTemplate( + 'passwordReset', + { subject: 'Reset it', body: 'Go [here]({{resetUrl}}). Expires in {{expiresIn}}.' }, + { resetUrl: 'https://shop.test/r', expiresIn: 'two hours' } + ); + expect(html).toContain('two hours'); + }); +}); + +describe('greeting, built from the configured format', () => { + const FORMAT = 'Hi {{firstName}},'; + const FALLBACK = 'Hi,'; + + it('substitutes the first name into the format', () => { + expect(greeting('Ada', FORMAT, FALLBACK)).toBe('Hi Ada,'); + }); + + it('honours a format an admin has rewritten', () => { + expect(greeting('Ada', 'Dear {{firstName}} {{lastName}}:', FALLBACK, 'Lovelace')) + .toBe('Dear Ada Lovelace:'); + }); + + it('tolerates whitespace inside the braces, as the renderer does', () => { + expect(greeting('Ada', 'Hi {{ firstName }},', FALLBACK)).toBe('Hi Ada,'); + }); + + // The case #106 was about. Customers who registered while first names were + // optional genuinely have none, and substituting an empty string into the + // format would send them "Hi ,". + it.each([null, undefined, '', ' '])( + 'uses the fallback whole rather than a format with a hole in it (%p)', + (name) => { + expect(greeting(name, FORMAT, FALLBACK)).toBe('Hi,'); + } + ); + + it('does not leave a lastName placeholder behind when there is no last name', () => { + expect(greeting('Ada', 'Hi {{firstName}} {{lastName}},', FALLBACK, null)) + .not.toMatch(/\{\{/); + }); +}); + +describe('every template can address the customer', () => { + it.each(KEYS)('%s offers greeting, firstName and lastName', (key) => { + expect(TEMPLATES[key].available).toEqual( + expect.arrayContaining(['greeting', 'firstName', 'lastName']) + ); + }); +}); diff --git a/frontend/src/admin/Settings.tsx b/frontend/src/admin/Settings.tsx index 8b4c8b7..42ecfc2 100644 --- a/frontend/src/admin/Settings.tsx +++ b/frontend/src/admin/Settings.tsx @@ -5,6 +5,8 @@ import Button from 'antd/es/button'; import Typography from 'antd/es/typography'; import message from 'antd/es/message'; import Card from 'antd/es/card'; +import Space from 'antd/es/space'; +import Input from 'antd/es/input'; import { fetchAdminSettings, updateAdminSettings } from './adminSettingsApi'; const { Title, Text } = Typography; @@ -12,12 +14,11 @@ const { Title, Text } = Typography; export default function Settings() { const [form] = Form.useForm(); const [loading, setLoading] = useState(true); + const [saving, setSaving] = useState(false); useEffect(() => { fetchAdminSettings() - .then(s => { - form.setFieldsValue({ cartExpiryHours: s.cartExpiryHours }); - }) + .then(s => form.setFieldsValue(s)) // A rejection here used to leave the form spinning indefinitely. .catch(() => message.error('Could not load settings')) .finally(() => setLoading(false)); @@ -25,22 +26,97 @@ export default function Settings() { async function handleSave() { const values = await form.validateFields(); - await updateAdminSettings(values); - message.success('Settings saved'); + setSaving(true); + try { + await updateAdminSettings(values); + message.success('Settings saved'); + } catch (err) { + // Reported rather than swallowed: the previous version announced success + // whatever the server said. + message.error((err as Error).message); + } finally { + setSaving(false); + } } + // One form across both cards, so Save commits every field rather than each + // card needing a button of its own. return ( - - Cart Settings - - How long an item stays reserved in a customer's cart before it's automatically released back to available inventory. - -
- - - - -
- +
+ + + Cart + + How long an item stays reserved in a customer's cart before it's automatically released back to available inventory. + + + + + + + + Link lifetimes + {/* The emails state these durations from a placeholder, so changing a + value here changes what the customer is told. That is the point: + the wording used to be a second, hand-written copy of the number. */} + + How long the links in the verification and password reset emails stay valid. The emails state these + durations, so they follow whatever is set here. + + + + + + + + + + + Greeting + {/* Two fields rather than one. Editing the name out of a format for + customers who have none is guesswork that has to be right every + time, and getting it wrong ships "Hi ," — the exact failure #106 + was about. An admin writes both and neither is guessed. */} + + What {'{{greeting}}'} becomes in every email. Use {'{{firstName}}'} and{' '} + {'{{lastName}}'} in the format. The fallback is used whole for customers who registered + without a first name. + + + + + + + + + + + +
); } diff --git a/frontend/src/admin/adminSettingsApi.ts b/frontend/src/admin/adminSettingsApi.ts index fb5e528..c15f34c 100644 --- a/frontend/src/admin/adminSettingsApi.ts +++ b/frontend/src/admin/adminSettingsApi.ts @@ -1,9 +1,14 @@ export interface AdminSettings { cartExpiryHours: number; + verifyTokenHours: number; + passwordResetHours: number; + greetingFormat: string; + greetingFallback: string; } export async function fetchAdminSettings(): Promise { const res = await fetch('/api/admin/settings'); + if (!res.ok) throw new Error('Could not load settings'); return res.json(); } @@ -13,5 +18,10 @@ export async function updateAdminSettings(settings: AdminSettings): Promise { + test.afterEach(async ({ page }) => { + await page.request.put('/api/admin/settings', { data: DEFAULTS }); + }); + + test('offers the link lifetimes and the greeting alongside cart expiry', async ({ page }) => { + await openSettings(page); + + // antd renders a stepped InputNumber to the step's precision, so "24.0". + await expect(page.getByLabel('Cart expiry (hours)')).toHaveValue('24.0'); + await expect(page.getByLabel('Email verification link (hours)')).toHaveValue('24.0'); + await expect(page.getByLabel('Password reset link (hours)')).toHaveValue('1.0'); + await expect(page.getByLabel('Greeting format')).toHaveValue('Hi {{firstName}},'); + await expect(page.getByLabel('Fallback, when there is no first name')).toHaveValue('Hi,'); + }); + + test('saves a new lifetime, and the server keeps it', async ({ page }) => { + await openSettings(page); + + await page.getByLabel('Password reset link (hours)').fill('3'); + await page.getByRole('button', { name: 'Save' }).click(); + await expect(page.getByText('Settings saved')).toBeVisible(); + + const stored = await (await page.request.get('/api/admin/settings')).json(); + expect(stored.passwordResetHours).toBe(3); + }); + + // The whole reason the placeholder exists: the sentence in the email is + // rendered from the setting rather than written out beside it. + test('the password reset preview states the configured lifetime', async ({ page }) => { + await page.request.put('/api/admin/settings', { data: { passwordResetHours: 2 } }); + + await page.goto('/admin'); + await page.getByRole('tab', { name: 'Emails' }).click(); + await page.getByRole('tab', { name: /Password reset/ }).click(); + + const preview = page.frameLocator('iframe[title="Password reset preview"]'); + await expect(preview.getByText('2 hours')).toBeVisible(); + }); + + test('the preview greets through the configured format', async ({ page }) => { + await page.request.put('/api/admin/settings', { data: { greetingFormat: 'Salutations {{firstName}}!' } }); + + await page.goto('/admin'); + await page.getByRole('tab', { name: 'Emails' }).click(); + await page.getByRole('tab', { name: /Email verification/ }).click(); + + const preview = page.frameLocator('iframe[title="Email verification preview"]'); + await expect(preview.getByText('Salutations Ada!')).toBeVisible(); + }); + + test('refuses a lifetime of zero rather than reporting a save', async ({ page }) => { + await openSettings(page); + + await page.getByLabel('Password reset link (hours)').fill('0'); + await page.getByRole('button', { name: 'Save' }).click(); + + await expect(page.getByText('Settings saved')).toHaveCount(0); + }); +});