Merge pull request 'feat: tabs and a rendered preview for the email templates (#119)' (#128) from feature/119-email-template-tabs into main

Reviewed-on: #128

backend/src/emailTemplates.ts

@@ -160,6 +160,30 @@ function substitute(text: string, values: Record<string, string>): string {
   );
 }
 
+/**
+ * Representative values for every placeholder any template accepts, used to
+ * render a preview in the admin.
+ *
+ * Kept here beside the definitions rather than in the route, so that adding a
+ * placeholder to a template puts the missing sample right next to the change
+ * that needs it. A unit test asserts every `available` name has an entry, since
+ * a missing one would render the preview with a literal {{placeholder}} in it
+ * and quietly teach the admin that their copy is broken when it is not.
+ *
+ * itemList is markdown because values are substituted into the markdown source
+ * before rendering, which is the same reason the real caller supplies markdown.
+ */
+export const SAMPLE_VALUES: Record<string, string> = {
+  greeting: 'Hi Ada,',
+  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'
+};
+
 export interface StoredTemplate {
   subject?: string | null;
   body?: string | null;

backend/src/routes/adminEmailTemplates.ts

@@ -1,7 +1,14 @@
 import { Router, Request, Response } from 'express';
 import { pool } from '../db';
 import { asyncRoute } from '../asyncRoute';
-import { TEMPLATES, TemplateKey, StoredTemplate, missingPlaceholders } from '../emailTemplates';
+import {
+  TEMPLATES,
+  TemplateKey,
+  StoredTemplate,
+  missingPlaceholders,
+  renderTemplate,
+  SAMPLE_VALUES
+} from '../emailTemplates';
 
 const router = Router();
 
@@ -53,6 +60,38 @@ router.get('/', asyncRoute(async (_req: Request, res: Response) => {
   );
 }));
 
+// Renders what an email would look like, from the subject and body in the
+// editor rather than from what is stored — so an admin sees the effect of an
+// edit before committing to it.
+//
+// Rendered here rather than in the browser, deliberately. renderTemplate is the
+// only thing that turns this markdown into HTML, and markdown-it is configured
+// with html: false, which is what stops an admin putting script into a
+// customer's inbox. A second renderer in the frontend would be a second place
+// for that setting to be wrong, and a preview that differs from the mailer is
+// worse than no preview.
+//
+// Deliberately does not enforce required placeholders. Saving refuses a body
+// that dropped one; previewing it is how an admin sees what they have done.
+router.post('/:key/preview', asyncRoute(async (req: Request, res: Response) => {
+  const key = req.params.key;
+  if (!isTemplateKey(key)) {
+    return res.status(404).json({ error: 'unknown template' });
+  }
+
+  const { subject, body } = req.body ?? {};
+  const rendered = renderTemplate(
+    key,
+    {
+      subject: typeof subject === 'string' ? subject : null,
+      body: typeof body === 'string' ? body : null
+    },
+    SAMPLE_VALUES
+  );
+
+  res.json(rendered);
+}));
+
 router.put('/:key', asyncRoute(async (req: Request, res: Response) => {
   const key = req.params.key;
   if (!isTemplateKey(key)) {

backend/tests/unit/emailTemplates.test.ts

@@ -2,7 +2,8 @@ import {
   TEMPLATES,
   TemplateKey,
   missingPlaceholders,
-  renderTemplate
+  renderTemplate,
+  SAMPLE_VALUES
 } from '../../src/emailTemplates';
 
 const KEYS: TemplateKey[] = [
@@ -147,3 +148,19 @@ describe('renderTemplate', () => {
     expect(subject).not.toMatch(/\{\{\s*\w+\s*\}\}/);
   });
 });
+
+describe('SAMPLE_VALUES, which the admin preview renders with', () => {
+  // A missing sample renders the preview with a literal {{placeholder}} in it,
+  // which teaches the admin their copy is broken when it is not. Adding a
+  // placeholder to a template must mean adding a sample for it.
+  it.each(KEYS)('%s has a sample for every placeholder it accepts', (key) => {
+    const missing = TEMPLATES[key].available.filter((name) => !(name in SAMPLE_VALUES));
+    expect(missing).toEqual([]);
+  });
+
+  it.each(KEYS)('%s previews with no placeholder left unsubstituted', (key) => {
+    const { html, subject } = renderTemplate(key, {}, SAMPLE_VALUES);
+    expect(html).not.toMatch(/\{\{\s*\w+\s*\}\}/);
+    expect(subject).not.toMatch(/\{\{\s*\w+\s*\}\}/);
+  });
+});

frontend/src/admin/EmailTemplateCard.tsx

@@ -1,115 +0,0 @@
-import { useState } from 'react';
-import Card from 'antd/es/card';
-import Input from 'antd/es/input';
-import Button from 'antd/es/button';
-import Space from 'antd/es/space';
-import Tag from 'antd/es/tag';
-import Typography from 'antd/es/typography';
-import message from 'antd/es/message';
-import { EmailTemplate, saveEmailTemplate, resetEmailTemplate } from './emailTemplatesApi';
-
-const { Text, Paragraph } = Typography;
-
-type Props = Readonly<{
-  template: EmailTemplate;
-  onChanged: (updated: EmailTemplate) => void;
-}>;
-
-export default function EmailTemplateCard({ template, onChanged }: Props) {
-  // Falls back to the default so the editor starts from the real copy rather
-  // than an empty box. `subject`/`body` being null means "never customised",
-  // which is why the badge below can say so.
-  const [subject, setSubject] = useState(template.subject ?? template.defaultSubject);
-  const [body, setBody] = useState(template.body ?? template.defaultBody);
-  const [saving, setSaving] = useState(false);
-
-  const customised = template.subject !== null || template.body !== null;
-
-  async function handleSave() {
-    setSaving(true);
-    try {
-      const updated = await saveEmailTemplate(template.key, subject, body);
-      onChanged(updated);
-      message.success(`${template.label} saved`);
-    } catch (err) {
-      // The server's refusal names the placeholder that is missing, which is
-      // the only useful thing to say here — so it is shown rather than replaced
-      // with something generic.
-      message.error((err as Error).message);
-    } finally {
-      setSaving(false);
-    }
-  }
-
-  async function handleReset() {
-    setSaving(true);
-    try {
-      const restored = await resetEmailTemplate(template.key);
-      setSubject(template.defaultSubject);
-      setBody(template.defaultBody);
-      onChanged(restored);
-      message.success(`${template.label} restored to the default`);
-    } catch (err) {
-      message.error((err as Error).message);
-    } finally {
-      setSaving(false);
-    }
-  }
-
-  return (
-    <Card
-      style={{ marginBottom: 16 }}
-      title={
-        <Space>
-          {template.label}
-          {customised ? <Tag color="blue">Customised</Tag> : <Tag>Default</Tag>}
-        </Space>
-      }
-    >
-      <Paragraph type="secondary" style={{ marginBottom: 8 }}>
-        Markdown. Placeholders are replaced when the email is sent
-        {template.required.length > 0 && (
-          <>
-            {' '}— <Text strong>{template.required.map((n) => `{{${n}}}`).join(' and ')}</Text>{' '}
-            {template.required.length === 1 ? 'is' : 'are'} required and cannot be removed
-          </>
-        )}
-        .
-      </Paragraph>
-
-      <Space wrap size={[4, 4]} style={{ marginBottom: 12 }}>
-        {template.available.map((name) => (
-          <Tag key={name} style={{ fontFamily: 'monospace' }}>{`{{${name}}}`}</Tag>
-        ))}
-      </Space>
-
-      <Input
-        aria-label={`${template.label} subject`}
-        value={subject}
-        onChange={(e) => setSubject(e.target.value)}
-        placeholder="Subject"
-        style={{ marginBottom: 8 }}
-      />
-      <Input.TextArea
-        aria-label={`${template.label} body`}
-        value={body}
-        onChange={(e) => setBody(e.target.value)}
-        autoSize={{ minRows: 6, maxRows: 16 }}
-        style={{ fontFamily: 'monospace', marginBottom: 12 }}
-      />
-
-      <Space>
-        <Button type="primary" loading={saving} onClick={handleSave}>
-          Save
-        </Button>
-        {/* Only offered when there is something to restore, so the button is
-            not a no-op that looks like it did something. */}
-        {customised && (
-          <Button loading={saving} onClick={handleReset}>
-            Restore default
-          </Button>
-        )}
-      </Space>
-    </Card>
-  );
-}

frontend/src/admin/EmailTemplateEditor.tsx

@@ -0,0 +1,170 @@
+import { useEffect, useState } from 'react';
+import Input from 'antd/es/input';
+import Button from 'antd/es/button';
+import Space from 'antd/es/space';
+import Tag from 'antd/es/tag';
+import Row from 'antd/es/row';
+import Col from 'antd/es/col';
+import Alert from 'antd/es/alert';
+import Typography from 'antd/es/typography';
+import message from 'antd/es/message';
+import {
+  EmailTemplate,
+  saveEmailTemplate,
+  resetEmailTemplate,
+  previewEmailTemplate
+} from './emailTemplatesApi';
+
+const { Text, Paragraph } = Typography;
+
+type Props = Readonly<{
+  template: EmailTemplate;
+  onChanged: (updated: EmailTemplate) => void;
+}>;
+
+// Long enough that the preview is not re-rendered on every keystroke, short
+// enough that it feels like it is following what you type.
+const PREVIEW_DEBOUNCE_MS = 400;
+
+export default function EmailTemplateEditor({ template, onChanged }: Props) {
+  // Falls back to the default so the editor starts from the real copy rather
+  // than an empty box. `subject`/`body` being null means "never customised",
+  // which is why the badge on the tab can say so.
+  const [subject, setSubject] = useState(template.subject ?? template.defaultSubject);
+  const [body, setBody] = useState(template.body ?? template.defaultBody);
+  const [saving, setSaving] = useState(false);
+  const [preview, setPreview] = useState<{ subject: string; html: string } | null>(null);
+  const [previewError, setPreviewError] = useState<string | null>(null);
+
+  const customised = template.subject !== null || template.body !== null;
+
+  // Previews the draft in the editor, not what is stored, so the effect of an
+  // edit is visible before committing to it. Debounced, and the timer is
+  // cleared on change so an abandoned keystroke never issues a request.
+  useEffect(() => {
+    const timer = setTimeout(() => {
+      previewEmailTemplate(template.key, subject, body)
+        .then((rendered) => {
+          setPreview(rendered);
+          setPreviewError(null);
+        })
+        .catch((err: Error) => setPreviewError(err.message));
+    }, PREVIEW_DEBOUNCE_MS);
+    return () => clearTimeout(timer);
+  }, [template.key, subject, body]);
+
+  async function handleSave() {
+    setSaving(true);
+    try {
+      const updated = await saveEmailTemplate(template.key, subject, body);
+      onChanged(updated);
+      message.success(`${template.label} saved`);
+    } catch (err) {
+      // The server's refusal names the placeholder that is missing, which is
+      // the only useful thing to say here — so it is shown rather than replaced
+      // with something generic.
+      message.error((err as Error).message);
+    } finally {
+      setSaving(false);
+    }
+  }
+
+  async function handleReset() {
+    setSaving(true);
+    try {
+      const restored = await resetEmailTemplate(template.key);
+      setSubject(template.defaultSubject);
+      setBody(template.defaultBody);
+      onChanged(restored);
+      message.success(`${template.label} restored to the default`);
+    } catch (err) {
+      message.error((err as Error).message);
+    } finally {
+      setSaving(false);
+    }
+  }
+
+  return (
+    <Row gutter={24}>
+      <Col xs={24} lg={13}>
+        <Paragraph type="secondary" style={{ marginBottom: 8 }}>
+          Markdown. Placeholders are replaced when the email is sent
+          {template.required.length > 0 && (
+            <>
+              {' '}— <Text strong>{template.required.map((n) => `{{${n}}}`).join(' and ')}</Text>{' '}
+              {template.required.length === 1 ? 'is' : 'are'} required and cannot be removed
+            </>
+          )}
+          .
+        </Paragraph>
+
+        <Space wrap size={[4, 4]} style={{ marginBottom: 12 }}>
+          {template.available.map((name) => (
+            <Tag key={name} style={{ fontFamily: 'monospace' }}>{`{{${name}}}`}</Tag>
+          ))}
+        </Space>
+
+        <Input
+          aria-label={`${template.label} subject`}
+          value={subject}
+          onChange={(e) => setSubject(e.target.value)}
+          placeholder="Subject"
+          style={{ marginBottom: 8 }}
+        />
+        <Input.TextArea
+          aria-label={`${template.label} body`}
+          value={body}
+          onChange={(e) => setBody(e.target.value)}
+          autoSize={{ minRows: 10, maxRows: 24 }}
+          style={{ fontFamily: 'monospace', marginBottom: 12 }}
+        />
+
+        <Space>
+          <Button type="primary" loading={saving} onClick={handleSave}>
+            Save
+          </Button>
+          {/* Only offered when there is something to restore, so the button is
+              not a no-op that looks like it did something. */}
+          {customised && (
+            <Button loading={saving} onClick={handleReset}>
+              Restore default
+            </Button>
+          )}
+        </Space>
+      </Col>
+
+      <Col xs={24} lg={11}>
+        <Paragraph type="secondary" style={{ marginBottom: 8 }}>
+          Preview, with sample values in place of the placeholders.
+        </Paragraph>
+        {previewError && (
+          <Alert type="error" showIcon message={previewError} style={{ marginBottom: 8 }} />
+        )}
+        <div
+          style={{
+            border: '1px solid rgba(128,128,128,0.3)',
+            borderRadius: 8,
+            overflow: 'hidden'
+          }}
+        >
+          <div style={{ padding: '8px 12px', borderBottom: '1px solid rgba(128,128,128,0.3)' }}>
+            <Text type="secondary">Subject: </Text>
+            <Text strong data-testid="preview-subject">{preview?.subject ?? ''}</Text>
+          </div>
+          {/* An iframe rather than dangerouslySetInnerHTML. The markup is safe
+              by construction — the server renders it with markdown-it's raw HTML
+              disabled — but an email is its own styling context, and rendering
+              it inline would let the admin theme's CSS change how it looks and
+              so make the preview lie. sandbox with no allow-* keeps script
+              inert even if that guarantee ever slipped. */}
+          <iframe
+            title={`${template.label} preview`}
+            sandbox=""
+            srcDoc={preview?.html ?? ''}
+            style={{ width: '100%', height: 420, border: 0, background: '#fff' }}
+          />
+        </div>
+      </Col>
+    </Row>
+  );
+}

frontend/src/admin/Settings.tsx

@@ -5,9 +5,12 @@ 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 Tabs from 'antd/es/tabs';
+import Tag from 'antd/es/tag';
+import Space from 'antd/es/space';
 import { fetchAdminSettings, updateAdminSettings } from './adminSettingsApi';
 import { EmailTemplate, fetchEmailTemplates } from './emailTemplatesApi';
-import EmailTemplateCard from './EmailTemplateCard';
+import EmailTemplateEditor from './EmailTemplateEditor';
 
 const { Title, Text } = Typography;
 
@@ -67,11 +70,28 @@ export default function Settings() {
     <Text type="secondary">
       The wording customers receive. Leave one alone and it sends the built-in copy.
     </Text>
-    <div style={{ marginTop: 16 }}>
+    {/* Tabs rather than a stacked column. With six templates the cart reminder
-      {templates.map(template => (
+        sat below five editors, so which one you were editing was knowable only
-        <EmailTemplateCard key={template.key} template={template} onChanged={handleTemplateChanged} />
+        from a card title you had already scrolled past. The Default/Customised
-      ))}
+        tag moves onto the tab label, so which templates have been changed is
-    </div>
+        visible without opening each one. */}
+    <Tabs
+      style={{ marginTop: 16 }}
+      items={templates.map(template => ({
+        key: template.key,
+        label: (
+          <Space size={4}>
+            {template.label}
+            {template.subject !== null || template.body !== null
+              ? <Tag color="blue">Customised</Tag>
+              : <Tag>Default</Tag>}
+          </Space>
+        ),
+        children: (
+          <EmailTemplateEditor template={template} onChanged={handleTemplateChanged} />
+        )
+      }))}
+    />
     </div>
   );
 }

frontend/src/admin/emailTemplatesApi.ts

@@ -54,3 +54,28 @@ export async function resetEmailTemplate(key: string): Promise<EmailTemplate> {
   );
   return json<EmailTemplate>(res);
 }
+
+export interface RenderedEmail {
+  subject: string;
+  html: string;
+}
+
+// Rendered by the server, which owns the only markdown renderer in the system
+// and the html: false setting that keeps script out of a customer's inbox. A
+// preview rendered in the browser would be a second implementation of both, and
+// one that disagreed with the mailer would be worse than none.
+export async function previewEmailTemplate(
+  key: string,
+  subject: string,
+  body: string
+): Promise<RenderedEmail> {
+  const res = await expectOk(
+    await fetch(`/api/admin/email-templates/${key}/preview`, {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify({ subject, body })
+    }),
+    'could not render the preview'
+  );
+  return json<RenderedEmail>(res);
+}

frontend/tests/e2e/email-templates.spec.ts

@@ -14,6 +14,17 @@ async function openSettings(page: import('@playwright/test').Page) {
   await expect(page.getByRole('heading', { name: 'Customer emails' })).toBeVisible();
 }
 
+// Opens one template's tab. Only the active tab's editor is mounted, which is
+// what makes the labels below unambiguous — the previous stacked layout had
+// every editor on screen at once and a locator for "Save" matched all six.
+async function openTemplate(page: import('@playwright/test').Page, label: string) {
+  await page.getByRole('tab', { name: new RegExp(label) }).click();
+  await expect(page.getByLabel(`${label} subject`)).toBeVisible();
+}
+
+const previewFrame = (page: import('@playwright/test').Page, label: string) =>
+  page.frameLocator(`iframe[title="${label} preview"]`);
+
 // Serial: these edit one shared stored template, and the suite runs fully
 // parallel by default — so run concurrently they would race, one asserting a
 // template is unset while another has just saved it.
@@ -24,7 +35,7 @@ test.describe('Editing the customer emails', () => {
     await restore(page, 'passwordReset');
   });
 
-  test('shows every template, marked default until it is edited', async ({ page }) => {
+  test('offers every template as a tab, marked default until it is edited', async ({ page }) => {
     await openSettings(page);
 
     for (const label of [
@@ -32,23 +43,27 @@ test.describe('Editing the customer emails', () => {
       'Password reset',
       'Favorited item sold',
       'Favorited item withdrawn',
-      'Cart reminder'
+      'Cart reminder',
+      'Email address changed'
     ]) {
-      await expect(page.getByText(label, { exact: true })).toBeVisible();
+      await expect(page.getByRole('tab', { name: new RegExp(label) })).toBeVisible();
     }
+
+    // The badge lives on the tab now, so which templates have been changed is
+    // visible without opening each one.
+    await expect(page.getByRole('tab', { name: /Password reset.*Default/ })).toBeVisible();
   });
 
   test('saves a replacement subject and body', async ({ page }) => {
     const subject = `Reset ${suffix()}`;
     await openSettings(page);
+    await openTemplate(page, 'Password reset');
 
     await page.getByLabel('Password reset subject').fill(subject);
     await page
       .getByLabel('Password reset body')
       .fill('Fresh wording. [Choose a new password]({{resetUrl}}).');
-
+    await page.getByRole('button', { name: 'Save', exact: true }).click();
-    const card = page.locator('.ant-card').filter({ hasText: 'Password reset' });
-    await card.getByRole('button', { name: 'Save', exact: true }).click();
 
     await expect(page.getByText('Password reset saved')).toBeVisible();
 
@@ -63,11 +78,10 @@ test.describe('Editing the customer emails', () => {
   // about — and the admin has to be told which placeholder is missing.
   test('refuses a body that drops the required placeholder, and says which', async ({ page }) => {
     await openSettings(page);
+    await openTemplate(page, 'Password reset');
 
     await page.getByLabel('Password reset body').fill('Just click the thing in your email.');
-
+    await page.getByRole('button', { name: 'Save', exact: true }).click();
-    const card = page.locator('.ant-card').filter({ hasText: 'Password reset' });
-    await card.getByRole('button', { name: 'Save', exact: true }).click();
 
     await expect(page.getByText('the body must keep {{resetUrl}}')).toBeVisible();
 
@@ -83,8 +97,8 @@ test.describe('Editing the customer emails', () => {
     });
 
     await openSettings(page);
-    const card = page.locator('.ant-card').filter({ hasText: 'Password reset' });
+    await openTemplate(page, 'Password reset');
-    await card.getByRole('button', { name: 'Restore default' }).click();
+    await page.getByRole('button', { name: 'Restore default' }).click();
 
     await expect(page.getByText('Password reset restored to the default')).toBeVisible();
 
@@ -94,3 +108,61 @@ test.describe('Editing the customer emails', () => {
     expect(reset.body).toBeNull();
   });
 });
+
+test.describe('Previewing the customer emails', () => {
+  test.afterEach(async ({ page }) => {
+    await restore(page, 'passwordReset');
+  });
+
+  test('shows the draft being edited, not the stored copy', async ({ page }) => {
+    const wording = `Wording ${suffix()}`;
+    await openSettings(page);
+    await openTemplate(page, 'Password reset');
+
+    await page
+      .getByLabel('Password reset body')
+      .fill(`${wording}. [Choose a new password]({{resetUrl}}).`);
+
+    // Nothing has been saved. The preview still reflects it, which is the whole
+    // point: an admin sees the effect before committing to it.
+    await expect(previewFrame(page, 'Password reset').getByText(wording)).toBeVisible();
+
+    const stored = await (await page.request.get('/api/admin/email-templates')).json();
+    expect(stored.find((t: { key: string }) => t.key === 'passwordReset').body).toBeNull();
+  });
+
+  test('substitutes sample values rather than showing raw placeholders', async ({ page }) => {
+    await openSettings(page);
+    await openTemplate(page, 'Password reset');
+
+    const frame = previewFrame(page, 'Password reset');
+    await expect(frame.getByRole('link')).toHaveAttribute('href', /reset-password\?token=/);
+    await expect(frame.locator('body')).not.toContainText('{{resetUrl}}');
+  });
+
+  // The control that stops an admin putting script into a customer's inbox is
+  // markdown-it's html: false on the server. The preview has to show the same
+  // thing the mailer emits, or it would be reassuring about the wrong output.
+  test('escapes raw HTML exactly as the mailer does', async ({ page }) => {
+    await openSettings(page);
+    await openTemplate(page, 'Password reset');
+
+    await page
+      .getByLabel('Password reset body')
+      .fill('<script>alert(1)</script> [link]({{resetUrl}})');
+
+    await expect(previewFrame(page, 'Password reset').getByText('<script>alert(1)</script>')).toBeVisible();
+  });
+
+  // Appended by the server and not editable, so it has to appear in the preview
+  // of the two templates it belongs to and nowhere else.
+  test('includes the consent footer on a favorite template, and not on others', async ({ page }) => {
+    await openSettings(page);
+
+    await openTemplate(page, 'Favorited item sold');
+    await expect(previewFrame(page, 'Favorited item sold').getByText(/account page/)).toBeVisible();
+
+    await openTemplate(page, 'Password reset');
+    await expect(previewFrame(page, 'Password reset').locator('body')).not.toContainText('account page');
+  });
+});