Merge pull request 'Feature/260 email the upload link' (#292) from feature/260-email-the-upload-link into main
Reviewed-on: #292
This commit was merged in pull request #292.
This commit is contained in:
@@ -0,0 +1,17 @@
|
|||||||
|
exports.up = (pgm) => {
|
||||||
|
pgm.sql(`
|
||||||
|
-- Where the link was sent (#260). Nullable, and deliberately so: links
|
||||||
|
-- already exist in QA and a migration cannot invent an address for them, so
|
||||||
|
-- they are grandfathered rather than backfilled with something untrue.
|
||||||
|
--
|
||||||
|
-- The requirement lives in the create route instead, which is where new
|
||||||
|
-- links are actually made. A NOT NULL column would have forced a choice
|
||||||
|
-- between inventing data and refusing to migrate.
|
||||||
|
ALTER TABLE upload_links
|
||||||
|
ADD COLUMN IF NOT EXISTS contact_email TEXT;
|
||||||
|
`);
|
||||||
|
};
|
||||||
|
|
||||||
|
exports.down = (pgm) => {
|
||||||
|
pgm.sql(`ALTER TABLE upload_links DROP COLUMN IF EXISTS contact_email;`);
|
||||||
|
};
|
||||||
@@ -280,6 +280,7 @@ export const uploadLinks = pgTable("upload_links", {
|
|||||||
maxSubmissions: integer("max_submissions"),
|
maxSubmissions: integer("max_submissions"),
|
||||||
lastUsedAt: timestamp("last_used_at", { withTimezone: true, mode: 'string' }),
|
lastUsedAt: timestamp("last_used_at", { withTimezone: true, mode: 'string' }),
|
||||||
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
|
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
|
||||||
|
contactEmail: text("contact_email"),
|
||||||
}, (table) => [
|
}, (table) => [
|
||||||
unique("upload_links_token_hash_key").on(table.tokenHash),
|
unique("upload_links_token_hash_key").on(table.tokenHash),
|
||||||
]);
|
]);
|
||||||
|
|||||||
@@ -18,7 +18,8 @@ export type TemplateKey =
|
|||||||
| 'favoriteWithdrawn'
|
| 'favoriteWithdrawn'
|
||||||
| 'cartReminder'
|
| 'cartReminder'
|
||||||
| 'emailChanged'
|
| 'emailChanged'
|
||||||
| 'intakeDraft';
|
| 'intakeDraft'
|
||||||
|
| 'uploadLink';
|
||||||
|
|
||||||
export interface TemplateDefinition {
|
export interface TemplateDefinition {
|
||||||
/** Shown in the admin so a card is identifiable without reading its body. */
|
/** Shown in the admin so a card is identifiable without reading its body. */
|
||||||
@@ -153,6 +154,21 @@ export const TEMPLATES: Record<TemplateKey, TemplateDefinition> = {
|
|||||||
'Nothing is listed until you publish it from that screen, and the price ' +
|
'Nothing is listed until you publish it from that screen, and the price ' +
|
||||||
'above is a suggestion rather than a decision.\n\n' +
|
'above is a suggestion rather than a decision.\n\n' +
|
||||||
'[Ask for another draft]({{regenerateUrl}}) - [Discard it]({{discardUrl}})'
|
'[Ask for another draft]({{regenerateUrl}}) - [Discard it]({{discardUrl}})'
|
||||||
|
},
|
||||||
|
|
||||||
|
uploadLink: {
|
||||||
|
label: 'Upload link for a contributor',
|
||||||
|
// The link itself, for the same reason verification requires verifyUrl: an
|
||||||
|
// email inviting somebody to send in photos, with no way to do it, sends
|
||||||
|
// perfectly happily and wastes everyone's time.
|
||||||
|
required: ['submitUrl'],
|
||||||
|
available: ['submitUrl', 'label', 'submissionsAllowed'],
|
||||||
|
defaultSubject: 'Send us your items',
|
||||||
|
defaultBody:
|
||||||
|
'You can send us photos of items you would like us to sell.\n\n' +
|
||||||
|
'[Send in an item]({{submitUrl}})\n\n' +
|
||||||
|
'You can send {{submissionsAllowed}}. Photograph one item at a time, and tell us anything you know about it — where it came from, what it is made of, any damage. A photo cannot show any of that.\n\n' +
|
||||||
|
'Keep this link to yourself: anyone who has it can send us items in your name.'
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -265,7 +281,10 @@ export const SAMPLE_VALUES: Record<string, string> = {
|
|||||||
regenerateUrl: 'https://example.com/api/intake-actions/1/regenerate?expires=0&sig=sample',
|
regenerateUrl: 'https://example.com/api/intake-actions/1/regenerate?expires=0&sig=sample',
|
||||||
discardUrl: 'https://example.com/api/intake-actions/1/discard?expires=0&sig=sample',
|
discardUrl: 'https://example.com/api/intake-actions/1/discard?expires=0&sig=sample',
|
||||||
expiresIn: 'one hour',
|
expiresIn: 'one hour',
|
||||||
holdDuration: '24 hours'
|
holdDuration: '24 hours',
|
||||||
|
submitUrl: 'https://example.com/submit/sample-token',
|
||||||
|
label: 'Autumn drop-off',
|
||||||
|
submissionsAllowed: '25 items'
|
||||||
};
|
};
|
||||||
|
|
||||||
export interface StoredTemplate {
|
export interface StoredTemplate {
|
||||||
|
|||||||
+35
-9
@@ -4,6 +4,16 @@ import nodemailer from 'nodemailer';
|
|||||||
// Brevo — has to set host, port and SMTP_SECURE explicitly rather than
|
// Brevo — has to set host, port and SMTP_SECURE explicitly rather than
|
||||||
// inheriting these, and getting that wrong fails at send time rather than at
|
// inheriting these, and getting that wrong fails at send time rather than at
|
||||||
// boot. See #64 on validating this at startup instead.
|
// boot. See #64 on validating this at startup instead.
|
||||||
|
// #260 put the first awaited send on a user-facing request path (the admin
|
||||||
|
// creating an upload link). nodemailer's defaults are two minutes to connect
|
||||||
|
// and ten minutes on the socket, which is fine for a fire-and-forget send but
|
||||||
|
// is not a bound anyone waiting on a response can live with: the link row and
|
||||||
|
// its token are already committed by the time sendMail is called, the token
|
||||||
|
// is shown exactly once, and a request that hangs long enough for the browser
|
||||||
|
// or reverse proxy to give up first loses it for good. Five seconds each is
|
||||||
|
// long enough for a reachable host and short enough that a dead one fails
|
||||||
|
// fast, leaving the admin with the "not emailed" warning and a link they can
|
||||||
|
// still copy, instead of a stuck spinner and a token nobody ever saw.
|
||||||
const transporter = nodemailer.createTransport({
|
const transporter = nodemailer.createTransport({
|
||||||
host: process.env.SMTP_HOST || 'smtp.gmail.com',
|
host: process.env.SMTP_HOST || 'smtp.gmail.com',
|
||||||
port: parseInt(process.env.SMTP_PORT || '465', 10),
|
port: parseInt(process.env.SMTP_PORT || '465', 10),
|
||||||
@@ -11,7 +21,10 @@ const transporter = nodemailer.createTransport({
|
|||||||
auth: {
|
auth: {
|
||||||
user: process.env.SMTP_USER,
|
user: process.env.SMTP_USER,
|
||||||
pass: process.env.SMTP_PASSWORD
|
pass: process.env.SMTP_PASSWORD
|
||||||
}
|
},
|
||||||
|
connectionTimeout: 5000,
|
||||||
|
greetingTimeout: 5000,
|
||||||
|
socketTimeout: 5000
|
||||||
});
|
});
|
||||||
|
|
||||||
interface ParsedAddress {
|
interface ParsedAddress {
|
||||||
@@ -85,23 +98,35 @@ export function isAllowedRecipient(to: string, allowlist: string | undefined): b
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function sendMail(to: string, subject: string, html: string): Promise<void> {
|
/**
|
||||||
|
* What a send attempt actually did.
|
||||||
|
*
|
||||||
|
* `sendMail` returns early in two cases that used to be indistinguishable from
|
||||||
|
* success — no SMTP credentials, and a recipient outside MAIL_ALLOWLIST — which
|
||||||
|
* meant a caller could report "emailed" for a message nobody would ever
|
||||||
|
* receive. QA restricts delivery by design, so that was not a hypothetical: it
|
||||||
|
* is the normal case there. See #260.
|
||||||
|
*/
|
||||||
|
export type MailOutcome = 'sent' | 'skipped-unconfigured' | 'skipped-blocked';
|
||||||
|
|
||||||
|
export async function sendMail(to: string, subject: string, html: string): Promise<MailOutcome> {
|
||||||
if (!process.env.SMTP_USER || !process.env.SMTP_PASSWORD) {
|
if (!process.env.SMTP_USER || !process.env.SMTP_PASSWORD) {
|
||||||
console.warn(`SMTP not configured — skipping email to ${to}: "${subject}"`);
|
console.warn(`SMTP not configured — skipping email to ${to}: "${subject}"`);
|
||||||
return;
|
return 'skipped-unconfigured';
|
||||||
}
|
}
|
||||||
|
|
||||||
// Guarded here rather than at the four call sites, so every sender is covered
|
// Guarded here rather than at the four call sites, so every sender is covered
|
||||||
// by construction and a fifth added later cannot bypass it by forgetting.
|
// by construction and a fifth added later cannot bypass it by forgetting.
|
||||||
//
|
//
|
||||||
// Skipping rather than throwing, and returning as though it sent: three of
|
// Skipping rather than throwing, and reporting the skip through MailOutcome
|
||||||
// the callers already swallow send failures into a log, so throwing would
|
// rather than pretending nothing happened: three of the callers already
|
||||||
// mostly be caught and logged anyway while risking a 500 on the signup path.
|
// swallow send failures into a log, so throwing would mostly be caught and
|
||||||
// The flow under test finishes, and the log says why no mail arrived — which
|
// logged anyway while risking a 500 on the signup path. The flow under test
|
||||||
// is the part that was missing when QA was simply muted.
|
// finishes, and both the log and the returned outcome say why no mail
|
||||||
|
// arrived — which is the part that was missing when QA was simply muted.
|
||||||
if (!isAllowedRecipient(to, process.env.MAIL_ALLOWLIST)) {
|
if (!isAllowedRecipient(to, process.env.MAIL_ALLOWLIST)) {
|
||||||
console.warn(`[mail-blocked] ${to} is not on MAIL_ALLOWLIST — skipping "${subject}"`);
|
console.warn(`[mail-blocked] ${to} is not on MAIL_ALLOWLIST — skipping "${subject}"`);
|
||||||
return;
|
return 'skipped-blocked';
|
||||||
}
|
}
|
||||||
|
|
||||||
await transporter.sendMail({
|
await transporter.sendMail({
|
||||||
@@ -110,4 +135,5 @@ export async function sendMail(to: string, subject: string, html: string): Promi
|
|||||||
subject,
|
subject,
|
||||||
html
|
html
|
||||||
});
|
});
|
||||||
|
return 'sent';
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2,7 +2,10 @@ import { Router, Request, Response } from 'express';
|
|||||||
import { pool, requireRow } from '../db';
|
import { pool, requireRow } from '../db';
|
||||||
import { asyncRoute } from '../asyncRoute';
|
import { asyncRoute } from '../asyncRoute';
|
||||||
import { generateToken, hashToken } from '../uploadLinks';
|
import { generateToken, hashToken } from '../uploadLinks';
|
||||||
import { trimTrailingSlashes } from '../utils';
|
import { trimTrailingSlashes, isValidEmail } from '../utils';
|
||||||
|
import { sendMail, MailOutcome } from '../mailer';
|
||||||
|
import { renderTemplate } from '../emailTemplates';
|
||||||
|
import { loadStoredTemplate } from './adminEmailTemplates';
|
||||||
|
|
||||||
const router = Router();
|
const router = Router();
|
||||||
|
|
||||||
@@ -27,7 +30,7 @@ const router = Router();
|
|||||||
* `token_hash` into every listing the moment somebody added a convenience.
|
* `token_hash` into every listing the moment somebody added a convenience.
|
||||||
*/
|
*/
|
||||||
const LINK_SELECT = `
|
const LINK_SELECT = `
|
||||||
SELECT id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at
|
SELECT id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at
|
||||||
FROM upload_links
|
FROM upload_links
|
||||||
`;
|
`;
|
||||||
|
|
||||||
@@ -44,6 +47,7 @@ const DEFAULT_MAX_SUBMISSIONS = 25;
|
|||||||
interface UploadLinkRow {
|
interface UploadLinkRow {
|
||||||
id: number;
|
id: number;
|
||||||
label: string;
|
label: string;
|
||||||
|
contact_email: string | null;
|
||||||
revoked_at: string | null;
|
revoked_at: string | null;
|
||||||
submission_count: number;
|
submission_count: number;
|
||||||
max_submissions: number | null;
|
max_submissions: number | null;
|
||||||
@@ -51,6 +55,19 @@ interface UploadLinkRow {
|
|||||||
created_at: string;
|
created_at: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What the mail tells the recipient about how much they may send.
|
||||||
|
*
|
||||||
|
* Words rather than a bare number for an uncapped link, so the sentence reads
|
||||||
|
* as a sentence instead of showing an empty space where a figure should be.
|
||||||
|
* An uncapped link is a deliberate choice the admin already had to make, so it
|
||||||
|
* is emailable like any other.
|
||||||
|
*/
|
||||||
|
function submissionsAllowed(maxSubmissions: number | null): string {
|
||||||
|
if (maxSubmissions === null) return 'as many items as you like';
|
||||||
|
return maxSubmissions === 1 ? '1 item' : `${maxSubmissions} items`;
|
||||||
|
}
|
||||||
|
|
||||||
router.get('/', asyncRoute(async (_req: Request, res: Response) => {
|
router.get('/', asyncRoute(async (_req: Request, res: Response) => {
|
||||||
const { rows } = await pool.query<UploadLinkRow>(`${LINK_SELECT} ORDER BY created_at DESC`);
|
const { rows } = await pool.query<UploadLinkRow>(`${LINK_SELECT} ORDER BY created_at DESC`);
|
||||||
res.json(rows);
|
res.json(rows);
|
||||||
@@ -62,6 +79,11 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => {
|
|||||||
return res.status(400).json({ error: 'a label is required' });
|
return res.status(400).json({ error: 'a label is required' });
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const email = typeof req.body?.email === 'string' ? req.body.email.trim() : '';
|
||||||
|
if (email === '' || !isValidEmail(email)) {
|
||||||
|
return res.status(400).json({ error: 'a valid email address is required' });
|
||||||
|
}
|
||||||
|
|
||||||
// Three cases, deliberately distinct. Absent means nobody decided, which
|
// Three cases, deliberately distinct. Absent means nobody decided, which
|
||||||
// gets the bounded default. An explicit null means unlimited — a decision
|
// gets the bounded default. An explicit null means unlimited — a decision
|
||||||
// someone made, visible in the request. A number is itself. Reading absent
|
// someone made, visible in the request. A number is itself. Reading absent
|
||||||
@@ -80,10 +102,10 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => {
|
|||||||
|
|
||||||
const token = generateToken();
|
const token = generateToken();
|
||||||
const { rows } = await pool.query<UploadLinkRow>(
|
const { rows } = await pool.query<UploadLinkRow>(
|
||||||
`INSERT INTO upload_links (label, token_hash, max_submissions)
|
`INSERT INTO upload_links (label, token_hash, max_submissions, contact_email)
|
||||||
VALUES ($1, $2, $3)
|
VALUES ($1, $2, $3, $4)
|
||||||
RETURNING id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
|
RETURNING id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
|
||||||
[label, hashToken(token), maxSubmissions]
|
[label, hashToken(token), maxSubmissions, email]
|
||||||
);
|
);
|
||||||
const link = requireRow(rows, 'the upload_links INSERT');
|
const link = requireRow(rows, 'the upload_links INSERT');
|
||||||
|
|
||||||
@@ -91,7 +113,43 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => {
|
|||||||
// outbound link is built from. Absent in local development, which yields a
|
// outbound link is built from. Absent in local development, which yields a
|
||||||
// relative URL the admin screen can still show and copy usefully.
|
// relative URL the admin screen can still show and copy usefully.
|
||||||
const base = trimTrailingSlashes(process.env.PUBLIC_URL ?? '');
|
const base = trimTrailingSlashes(process.env.PUBLIC_URL ?? '');
|
||||||
res.status(201).json({ ...link, token, url: `${base}/submit/${token}` });
|
const url = `${base}/submit/${token}`;
|
||||||
|
|
||||||
|
// Awaited, and its outcome reported rather than swallowed. Every other sender
|
||||||
|
// in this codebase fires and forgets because nobody is waiting on the answer;
|
||||||
|
// here somebody is — the admin is looking at the screen, and whether they
|
||||||
|
// now have to send the link by hand is the thing they need to know.
|
||||||
|
//
|
||||||
|
// A failure does not roll the link back. The token is shown exactly once, so
|
||||||
|
// a rollback would leave the admin retrying and holding a different link,
|
||||||
|
// discarding work that succeeded for the sake of tidiness.
|
||||||
|
let outcome: MailOutcome = 'skipped-unconfigured';
|
||||||
|
try {
|
||||||
|
const template = renderTemplate('uploadLink', await loadStoredTemplate('uploadLink'), {
|
||||||
|
submitUrl: url,
|
||||||
|
label: link.label,
|
||||||
|
submissionsAllowed: submissionsAllowed(link.max_submissions)
|
||||||
|
});
|
||||||
|
outcome = await sendMail(email, template.subject, template.html);
|
||||||
|
} catch (err) {
|
||||||
|
// Reported, not thrown. The link exists and is usable; the admin needs to
|
||||||
|
// be told the mail did not go, not handed a 500 for a link that was made.
|
||||||
|
//
|
||||||
|
// An SMTP rejection also lands here and is reported the same way as an
|
||||||
|
// unconfigured environment, which is not strictly accurate — a fourth
|
||||||
|
// outcome would be a real distinction, but nothing consumes it and the
|
||||||
|
// admin's next action is identical either way: copy the link and send it
|
||||||
|
// by hand.
|
||||||
|
console.error(`[upload-links] could not email ${email}:`, err);
|
||||||
|
outcome = 'skipped-unconfigured';
|
||||||
|
}
|
||||||
|
|
||||||
|
res.status(201).json({
|
||||||
|
...link,
|
||||||
|
token,
|
||||||
|
url,
|
||||||
|
mail: { sent: outcome === 'sent', outcome }
|
||||||
|
});
|
||||||
}));
|
}));
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -124,7 +182,7 @@ router.post('/:id/revoke', asyncRoute(async (req: Request, res: Response) => {
|
|||||||
const { rows } = await pool.query<UploadLinkRow>(
|
const { rows } = await pool.query<UploadLinkRow>(
|
||||||
`UPDATE upload_links SET revoked_at = COALESCE(revoked_at, now())
|
`UPDATE upload_links SET revoked_at = COALESCE(revoked_at, now())
|
||||||
WHERE id = $1
|
WHERE id = $1
|
||||||
RETURNING id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
|
RETURNING id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
|
||||||
[req.params.id]
|
[req.params.id]
|
||||||
);
|
);
|
||||||
|
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ describe('GET /api/admin/email-templates', () => {
|
|||||||
'favoriteWithdrawn',
|
'favoriteWithdrawn',
|
||||||
'intakeDraft',
|
'intakeDraft',
|
||||||
'passwordReset',
|
'passwordReset',
|
||||||
|
'uploadLink',
|
||||||
'verification'
|
'verification'
|
||||||
]);
|
]);
|
||||||
|
|
||||||
|
|||||||
@@ -4,6 +4,14 @@ import app from '../../src/app';
|
|||||||
import { pool } from '../../src/db';
|
import { pool } from '../../src/db';
|
||||||
import { resetDb, closeDb } from './setup/testDb';
|
import { resetDb, closeDb } from './setup/testDb';
|
||||||
|
|
||||||
|
// Every submission-cap test here issues a link, which now emails it. Mocked
|
||||||
|
// so the suite never opens a real connection to smtp.gmail.com — see the
|
||||||
|
// "Do not add one back" warning in tests/unit/mailOutcome.test.ts, which this
|
||||||
|
// mirrors at the integration layer.
|
||||||
|
jest.mock('../../src/mailer', () => ({
|
||||||
|
sendMail: jest.fn().mockResolvedValue('sent')
|
||||||
|
}));
|
||||||
|
|
||||||
const UPLOADS_DIR = process.env.UPLOADS_DIR as string;
|
const UPLOADS_DIR = process.env.UPLOADS_DIR as string;
|
||||||
|
|
||||||
// The same 1x1 PNG the upload validation suite uses, so the accepted case
|
// The same 1x1 PNG the upload validation suite uses, so the accepted case
|
||||||
@@ -34,7 +42,7 @@ async function storedFiles(): Promise<string[]> {
|
|||||||
async function issueLink(label = 'Sarah', maxSubmissions?: number | null): Promise<string> {
|
async function issueLink(label = 'Sarah', maxSubmissions?: number | null): Promise<string> {
|
||||||
const res = await request(app)
|
const res = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label, ...(maxSubmissions === undefined ? {} : { maxSubmissions }) });
|
.send({ label, email: 'sarah@example.com', ...(maxSubmissions === undefined ? {} : { maxSubmissions }) });
|
||||||
expect(res.status).toBe(201);
|
expect(res.status).toBe(201);
|
||||||
return res.body.token as string;
|
return res.body.token as string;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -4,6 +4,14 @@ import { pool } from '../../src/db';
|
|||||||
import { resetDb, closeDb } from './setup/testDb';
|
import { resetDb, closeDb } from './setup/testDb';
|
||||||
import { resetAlertThrottleForTests } from '../../src/intake/abuseAlert';
|
import { resetAlertThrottleForTests } from '../../src/intake/abuseAlert';
|
||||||
|
|
||||||
|
// makeLink() issues a link on every test, which now emails it. Mocked so the
|
||||||
|
// suite never opens a real connection to smtp.gmail.com — see the "Do not add
|
||||||
|
// one back" warning in tests/unit/mailOutcome.test.ts, which this mirrors at
|
||||||
|
// the integration layer.
|
||||||
|
jest.mock('../../src/mailer', () => ({
|
||||||
|
sendMail: jest.fn().mockResolvedValue('sent')
|
||||||
|
}));
|
||||||
|
|
||||||
const PNG = Buffer.from(
|
const PNG = Buffer.from(
|
||||||
'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
|
'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==',
|
||||||
'base64'
|
'base64'
|
||||||
@@ -20,7 +28,9 @@ afterAll(async () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
async function makeLink(): Promise<string> {
|
async function makeLink(): Promise<string> {
|
||||||
const res = await request(app).post('/api/admin/upload-links').send({ label: 'ceiling spec' });
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'ceiling spec', email: 'sarah@example.com' });
|
||||||
expect(res.status).toBe(201);
|
expect(res.status).toBe(201);
|
||||||
return res.body.token;
|
return res.body.token;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -3,8 +3,33 @@ import app from '../../src/app';
|
|||||||
import { pool } from '../../src/db';
|
import { pool } from '../../src/db';
|
||||||
import { resetDb, closeDb } from './setup/testDb';
|
import { resetDb, closeDb } from './setup/testDb';
|
||||||
|
|
||||||
|
// Every test in this file issues an upload link, which now emails it.
|
||||||
|
// Mocked exactly like the other integration suites that touch mail (see
|
||||||
|
// accountDetails.integration.test.ts, favorites.integration.test.ts,
|
||||||
|
// resendVerification.integration.test.ts) so the suite never opens a real
|
||||||
|
// connection to smtp.gmail.com — see the "Do not add one back" warning in
|
||||||
|
// tests/unit/mailOutcome.test.ts, which this mirrors at the integration
|
||||||
|
// layer.
|
||||||
|
//
|
||||||
|
// Unlike those three, the "emailing the link to its recipient" describe
|
||||||
|
// block below needs to see specific MailOutcome values come back through the
|
||||||
|
// route rather than a single fixed one. Threading real SMTP_USER /
|
||||||
|
// MAIL_ALLOWLIST env vars through the real sendMail was the previous
|
||||||
|
// approach and is exactly the hazard being removed here, so those tests are
|
||||||
|
// restructured to drive the mock's return value directly instead — that also
|
||||||
|
// makes them a cleaner test of the route's outcome-reporting (its job),
|
||||||
|
// separate from sendMail's own skip logic (already covered hermetically by
|
||||||
|
// mailOutcome.test.ts and mailAllowlist.test.ts).
|
||||||
|
jest.mock('../../src/mailer', () => ({
|
||||||
|
sendMail: jest.fn().mockResolvedValue('sent')
|
||||||
|
}));
|
||||||
|
import { sendMail, MailOutcome } from '../../src/mailer';
|
||||||
|
const sentMail = sendMail as jest.MockedFunction<typeof sendMail>;
|
||||||
|
|
||||||
beforeEach(async () => {
|
beforeEach(async () => {
|
||||||
await resetDb();
|
await resetDb();
|
||||||
|
sentMail.mockReset();
|
||||||
|
sentMail.mockResolvedValue('sent');
|
||||||
});
|
});
|
||||||
|
|
||||||
afterAll(async () => {
|
afterAll(async () => {
|
||||||
@@ -16,7 +41,7 @@ describe('issuing an upload link', () => {
|
|||||||
it('returns the token exactly once, at creation', async () => {
|
it('returns the token exactly once, at creation', async () => {
|
||||||
const created = await request(app)
|
const created = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label: 'Sarah' });
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
expect(created.status).toBe(201);
|
expect(created.status).toBe(201);
|
||||||
expect(created.body.label).toBe('Sarah');
|
expect(created.body.label).toBe('Sarah');
|
||||||
@@ -34,7 +59,7 @@ describe('issuing an upload link', () => {
|
|||||||
it('stores the digest rather than the token', async () => {
|
it('stores the digest rather than the token', async () => {
|
||||||
const created = await request(app)
|
const created = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label: 'Estate sale box 3' });
|
.send({ label: 'Estate sale box 3', email: 'sarah@example.com' });
|
||||||
|
|
||||||
const { rows } = await pool.query<{ token_hash: string }>(
|
const { rows } = await pool.query<{ token_hash: string }>(
|
||||||
`SELECT token_hash FROM upload_links`
|
`SELECT token_hash FROM upload_links`
|
||||||
@@ -51,7 +76,7 @@ describe('issuing an upload link', () => {
|
|||||||
it('refuses a non-positive submission cap', async () => {
|
it('refuses a non-positive submission cap', async () => {
|
||||||
const res = await request(app)
|
const res = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label: 'Bad cap', maxSubmissions: 0 });
|
.send({ label: 'Bad cap', email: 'sarah@example.com', maxSubmissions: 0 });
|
||||||
expect(res.status).toBe(400);
|
expect(res.status).toBe(400);
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -59,7 +84,9 @@ describe('issuing an upload link', () => {
|
|||||||
// safe. An unbounded link should be something asked for, not something that
|
// safe. An unbounded link should be something asked for, not something that
|
||||||
// happens when nobody thought about it.
|
// happens when nobody thought about it.
|
||||||
it('bounds a link that was created without a cap', async () => {
|
it('bounds a link that was created without a cap', async () => {
|
||||||
const res = await request(app).post('/api/admin/upload-links').send({ label: 'Sarah' });
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
expect(res.status).toBe(201);
|
expect(res.status).toBe(201);
|
||||||
expect(res.body.max_submissions).toBe(25);
|
expect(res.body.max_submissions).toBe(25);
|
||||||
@@ -68,7 +95,7 @@ describe('issuing an upload link', () => {
|
|||||||
it('allows unlimited when it is asked for explicitly', async () => {
|
it('allows unlimited when it is asked for explicitly', async () => {
|
||||||
const res = await request(app)
|
const res = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label: 'Always on', maxSubmissions: null });
|
.send({ label: 'Always on', email: 'sarah@example.com', maxSubmissions: null });
|
||||||
|
|
||||||
expect(res.status).toBe(201);
|
expect(res.status).toBe(201);
|
||||||
expect(res.body.max_submissions).toBeNull();
|
expect(res.body.max_submissions).toBeNull();
|
||||||
@@ -79,7 +106,7 @@ describe('revoking an upload link', () => {
|
|||||||
it('stamps revoked_at and reports it in the listing', async () => {
|
it('stamps revoked_at and reports it in the listing', async () => {
|
||||||
const created = await request(app)
|
const created = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label: 'Temporary' });
|
.send({ label: 'Temporary', email: 'sarah@example.com' });
|
||||||
|
|
||||||
const revoked = await request(app)
|
const revoked = await request(app)
|
||||||
.post(`/api/admin/upload-links/${created.body.id}/revoke`);
|
.post(`/api/admin/upload-links/${created.body.id}/revoke`);
|
||||||
@@ -94,7 +121,7 @@ describe('revoking an upload link', () => {
|
|||||||
it('is idempotent, keeping the original timestamp', async () => {
|
it('is idempotent, keeping the original timestamp', async () => {
|
||||||
const created = await request(app)
|
const created = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label: 'Temporary' });
|
.send({ label: 'Temporary', email: 'sarah@example.com' });
|
||||||
|
|
||||||
const first = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`);
|
const first = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`);
|
||||||
const second = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`);
|
const second = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`);
|
||||||
@@ -108,3 +135,121 @@ describe('revoking an upload link', () => {
|
|||||||
expect(res.status).toBe(404);
|
expect(res.status).toBe(404);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('emailing the link to its recipient', () => {
|
||||||
|
it('refuses to create a link with no address', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
expect(res.body.error).toMatch(/email/i);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses an address that is not one', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'not-an-address' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
expect(res.body.error).toMatch(/email/i);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('stores the address on the link', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(201);
|
||||||
|
|
||||||
|
const { rows } = await pool.query<{ contact_email: string }>(
|
||||||
|
`SELECT contact_email FROM upload_links WHERE id = $1`,
|
||||||
|
[res.body.id]
|
||||||
|
);
|
||||||
|
expect(rows[0]?.contact_email).toBe('sarah@example.com');
|
||||||
|
});
|
||||||
|
|
||||||
|
// The point of the whole change: QA blocks delivery by design, so a link that
|
||||||
|
// was not actually emailed must not be reported as though it was. Driven
|
||||||
|
// through the mock's return value rather than the SMTP_USER /
|
||||||
|
// MAIL_ALLOWLIST env vars sendMail itself would branch on — see the file
|
||||||
|
// header comment for why.
|
||||||
|
it.each<MailOutcome>(['skipped-unconfigured', 'skipped-blocked'])(
|
||||||
|
'reports a %s outcome from sendMail rather than as though it sent',
|
||||||
|
async (outcome) => {
|
||||||
|
sentMail.mockResolvedValueOnce(outcome);
|
||||||
|
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(201);
|
||||||
|
expect(res.body.mail).toEqual({ sent: false, outcome });
|
||||||
|
}
|
||||||
|
);
|
||||||
|
|
||||||
|
// A send that could not happen must never cost the admin the link, because
|
||||||
|
// the token is shown exactly once and a rollback would hand them a different
|
||||||
|
// one on the retry.
|
||||||
|
it('still returns a usable link when the mail did not go', async () => {
|
||||||
|
sentMail.mockResolvedValueOnce('skipped-unconfigured');
|
||||||
|
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
|
expect(res.body.token).toBeTruthy();
|
||||||
|
expect(res.body.url).toContain(res.body.token);
|
||||||
|
expect(res.body.mail.sent).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('lists the address, and tolerates a link that has none', async () => {
|
||||||
|
await pool.query(`INSERT INTO upload_links (label, token_hash) VALUES ('Older link', 'digest')`);
|
||||||
|
await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Newer link', email: 'sarah@example.com' });
|
||||||
|
|
||||||
|
const res = await request(app).get('/api/admin/upload-links');
|
||||||
|
|
||||||
|
const older = res.body.find((row: { label: string }) => row.label === 'Older link');
|
||||||
|
const newer = res.body.find((row: { label: string }) => row.label === 'Newer link');
|
||||||
|
expect(older.contact_email).toBeNull();
|
||||||
|
expect(newer.contact_email).toBe('sarah@example.com');
|
||||||
|
});
|
||||||
|
|
||||||
|
// required: ['submitUrl'] on the template is a guard that the placeholder is
|
||||||
|
// present in the body, not that the route supplied a working value for it.
|
||||||
|
// A route that passed the bare base URL, or dropped the token, would leave
|
||||||
|
// every other test here green — so this asserts the actual captured html,
|
||||||
|
// and covers the submissionsAllowed wording for all three cases (a numeric
|
||||||
|
// cap, a cap of exactly one, and uncapped) at the same time, since all three
|
||||||
|
// are the same kind of claim: what the mail says versus what was created.
|
||||||
|
it('emails a link that actually contains the created token, and states how many items may be sent', async () => {
|
||||||
|
const capped = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com', maxSubmissions: 25 });
|
||||||
|
expect(capped.status).toBe(201);
|
||||||
|
expect(sentMail).toHaveBeenCalledTimes(1);
|
||||||
|
const [cappedTo, , cappedHtml] = sentMail.mock.calls[0]!;
|
||||||
|
expect(cappedTo).toBe('sarah@example.com');
|
||||||
|
expect(cappedHtml).toContain(`/submit/${capped.body.token}`);
|
||||||
|
expect(cappedHtml).toContain('25 items');
|
||||||
|
|
||||||
|
const single = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com', maxSubmissions: 1 });
|
||||||
|
expect(single.status).toBe(201);
|
||||||
|
const [, , singleHtml] = sentMail.mock.calls[1]!;
|
||||||
|
expect(singleHtml).toContain(`/submit/${single.body.token}`);
|
||||||
|
expect(singleHtml).toContain('1 item');
|
||||||
|
expect(singleHtml).not.toContain('1 items');
|
||||||
|
|
||||||
|
const uncapped = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com', maxSubmissions: null });
|
||||||
|
expect(uncapped.status).toBe(201);
|
||||||
|
const [, , uncappedHtml] = sentMail.mock.calls[2]!;
|
||||||
|
expect(uncappedHtml).toContain(`/submit/${uncapped.body.token}`);
|
||||||
|
expect(uncappedHtml).toContain('as many items as you like');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -8,14 +8,11 @@ import {
|
|||||||
SAMPLE_VALUES
|
SAMPLE_VALUES
|
||||||
} from '../../src/emailTemplates';
|
} from '../../src/emailTemplates';
|
||||||
|
|
||||||
const KEYS: TemplateKey[] = [
|
// Derived from TEMPLATES rather than hardcoded, so a new template is covered
|
||||||
'verification',
|
// by every it.each below the moment it is added. A hardcoded list silently
|
||||||
'passwordReset',
|
// stops covering anything added after it was written — which is exactly how
|
||||||
'favoriteSold',
|
// intakeDraft and uploadLink went untested by the SAMPLE_VALUES guard below.
|
||||||
'favoriteWithdrawn',
|
const KEYS = Object.keys(TEMPLATES) as TemplateKey[];
|
||||||
'cartReminder',
|
|
||||||
'emailChanged'
|
|
||||||
];
|
|
||||||
|
|
||||||
describe('the built-in templates', () => {
|
describe('the built-in templates', () => {
|
||||||
it.each(KEYS)('%s has a default subject and body', (key) => {
|
it.each(KEYS)('%s has a default subject and body', (key) => {
|
||||||
@@ -268,7 +265,22 @@ describe('greeting, built from the configured format', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
describe('every template can address the customer', () => {
|
describe('every template can address the customer', () => {
|
||||||
it.each(KEYS)('%s offers greeting, firstName and lastName', (key) => {
|
// Not KEYS: this is an invariant of the six customer-facing templates only.
|
||||||
|
// intakeDraft and uploadLink notify the shop and a contributor respectively,
|
||||||
|
// not a customer with a name on file, so they are deliberately not held to
|
||||||
|
// it — a hardcoded list is correct here rather than a staleness risk,
|
||||||
|
// because the set of templates this claim applies to does not grow just
|
||||||
|
// because TEMPLATES does.
|
||||||
|
const CUSTOMER_FACING_KEYS: TemplateKey[] = [
|
||||||
|
'verification',
|
||||||
|
'passwordReset',
|
||||||
|
'favoriteSold',
|
||||||
|
'favoriteWithdrawn',
|
||||||
|
'cartReminder',
|
||||||
|
'emailChanged'
|
||||||
|
];
|
||||||
|
|
||||||
|
it.each(CUSTOMER_FACING_KEYS)('%s offers greeting, firstName and lastName', (key) => {
|
||||||
expect(TEMPLATES[key].available).toEqual(
|
expect(TEMPLATES[key].available).toEqual(
|
||||||
expect.arrayContaining(['greeting', 'firstName', 'lastName'])
|
expect.arrayContaining(['greeting', 'firstName', 'lastName'])
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -0,0 +1,50 @@
|
|||||||
|
import { sendMail } from '../../src/mailer';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What sendMail says it did.
|
||||||
|
*
|
||||||
|
* It returns early in two cases that are indistinguishable from success at the
|
||||||
|
* call site — SMTP unconfigured, and the recipient not on MAIL_ALLOWLIST — and
|
||||||
|
* #260 needs to tell them apart so the admin is not told a link was emailed
|
||||||
|
* when it was not.
|
||||||
|
*
|
||||||
|
* Only the two skip paths are covered here. A real send needs an SMTP server,
|
||||||
|
* which a unit test has no business starting; the integration test in Task 4
|
||||||
|
* covers the route's behaviour instead.
|
||||||
|
*
|
||||||
|
* The allowlist's own behaviour — exact matches, plus-suffixes, domains,
|
||||||
|
* refusals — is covered directly and hermetically in
|
||||||
|
* backend/tests/unit/mailAllowlist.test.ts, against isAllowedRecipient itself.
|
||||||
|
* There is deliberately no third test here that sets SMTP_USER/SMTP_PASSWORD
|
||||||
|
* and an allowed recipient: that combination falls through both guards and
|
||||||
|
* reaches the real transporter, which opens a live TLS connection to
|
||||||
|
* smtp.gmail.com:465. Do not add one back.
|
||||||
|
*/
|
||||||
|
describe('what sendMail reports', () => {
|
||||||
|
const original = { ...process.env };
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
process.env = { ...original };
|
||||||
|
});
|
||||||
|
|
||||||
|
it('says so when SMTP is not configured', async () => {
|
||||||
|
delete process.env.SMTP_USER;
|
||||||
|
delete process.env.SMTP_PASSWORD;
|
||||||
|
|
||||||
|
await expect(sendMail('someone@example.com', 'subject', '<p>body</p>')).resolves.toBe(
|
||||||
|
'skipped-unconfigured'
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The case that matters most: QA restricts delivery, and a blocked address
|
||||||
|
// previously returned exactly as though it had sent.
|
||||||
|
it('says so when the recipient is not on the allowlist', async () => {
|
||||||
|
process.env.SMTP_USER = 'user';
|
||||||
|
process.env.SMTP_PASSWORD = 'password';
|
||||||
|
process.env.MAIL_ALLOWLIST = 'allowed@example.com';
|
||||||
|
|
||||||
|
await expect(sendMail('someone-else@example.com', 'subject', '<p>body</p>')).resolves.toBe(
|
||||||
|
'skipped-blocked'
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -0,0 +1,48 @@
|
|||||||
|
import { TEMPLATES, missingPlaceholders, renderTemplate } from '../../src/emailTemplates';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The template that carries an upload link (#260).
|
||||||
|
*
|
||||||
|
* The one failure worth making impossible is a link email with no link in it:
|
||||||
|
* it sends, it looks fine in the log, and it is useless to the person who gets
|
||||||
|
* it. That is the same guard `verification` has on `verifyUrl`.
|
||||||
|
*/
|
||||||
|
describe('the upload link template', () => {
|
||||||
|
it('is offered in the admin like the others', () => {
|
||||||
|
expect(TEMPLATES.uploadLink.label).toBeTruthy();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('requires the submit link', () => {
|
||||||
|
expect(TEMPLATES.uploadLink.required).toContain('submitUrl');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses a body that has no link in it', () => {
|
||||||
|
expect(missingPlaceholders('uploadLink', 'Hello, a link is on its way.')).toEqual(['submitUrl']);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('accepts a body that has one', () => {
|
||||||
|
expect(missingPlaceholders('uploadLink', 'Send your items: {{submitUrl}}')).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('offers the label and the allowance as placeholders', () => {
|
||||||
|
expect(TEMPLATES.uploadLink.available).toEqual(
|
||||||
|
expect.arrayContaining(['submitUrl', 'label', 'submissionsAllowed'])
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The default body has to satisfy the guard it declares, or the feature ships
|
||||||
|
// unable to send its own default.
|
||||||
|
it('has a default body that satisfies its own requirement', () => {
|
||||||
|
expect(missingPlaceholders('uploadLink', TEMPLATES.uploadLink.defaultBody)).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('renders the link into the body', () => {
|
||||||
|
const rendered = renderTemplate(
|
||||||
|
'uploadLink',
|
||||||
|
{ subject: 'Send us your items', body: 'Here: {{submitUrl}}' },
|
||||||
|
{ submitUrl: 'https://example.com/submit/abc', label: 'Sarah', submissionsAllowed: '25 items' }
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(rendered.html).toContain('https://example.com/submit/abc');
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -0,0 +1,809 @@
|
|||||||
|
# Upload Link Email Implementation Plan
|
||||||
|
|
||||||
|
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||||
|
|
||||||
|
**Goal:** Make an email address part of creating an upload link, and send the link to it.
|
||||||
|
|
||||||
|
**Architecture:** One new nullable column, one new editable mail template, and a return value on `sendMail` so the route can tell the admin whether the mail actually went. The link is created whether or not the send succeeds, and the response says which.
|
||||||
|
|
||||||
|
**Tech Stack:** Express 4 + TypeScript, `pg`, `node-pg-migrate`, `nodemailer`, Jest + supertest, React + antd (`antd/es/...` deep imports), Playwright.
|
||||||
|
|
||||||
|
**Spec:** `docs/superpowers/specs/2026-09-03-upload-link-email-design.md`
|
||||||
|
**Issue:** #260
|
||||||
|
|
||||||
|
## Global Constraints
|
||||||
|
|
||||||
|
- **A failed send never loses the link and never becomes a 500.** The link is created, the send attempted, and the outcome reported. The token is shown exactly once, so rolling back on a send failure would discard work that succeeded.
|
||||||
|
- **`mail.outcome` is always present**, on success as well as failure, and is always one of `'sent' | 'skipped-unconfigured' | 'skipped-blocked'`. A field that appears only on failure is one every consumer must remember to check for.
|
||||||
|
- **No existing `sendMail` caller changes.** There are seven and every one ignores the return value.
|
||||||
|
- **`upload_links.contact_email` is nullable.** Links already exist in QA; the requirement lives in the route, not the column.
|
||||||
|
- **antd imports are deep and from `es`**: `import Input from 'antd/es/input';`. Never `import { Input } from 'antd'`.
|
||||||
|
- **Verify the frontend with `npm run build`, never a bare `npx tsc --noEmit`** — the app tsconfig excludes `tests/`, and a green bare `tsc` once broke a deploy here.
|
||||||
|
- **Branch:** `feature/260-email-the-upload-link`, already created off `main`, already carrying the spec commit. Commit there. Subjects end `(#260)`. Commit bodies are **not hard-wrapped** — one long line per paragraph, blank lines between. Write each message to a temporary file and use `git commit -F <file>`, then delete it. Do not push; the user pushes.
|
||||||
|
- End every commit message with:
|
||||||
|
`Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`
|
||||||
|
- Integration tests need the test database: `cd backend && npm run db:test:up`. Port 55432 is Hyper-V-reserved on this machine; if it will not bind, set `TEST_PGPORT` rather than editing the compose file.
|
||||||
|
- **Do NOT run `scripts/start-local.ps1`, `scripts/run-tests.ps1`, or any nvm script.** They prompt for UAC elevation and have previously stripped Node from this machine. Playwright needs the stack running, which only the user can start.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 1: `sendMail` reports what it did
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `backend/src/mailer.ts:88-113`
|
||||||
|
- Test: `backend/tests/unit/mailOutcome.test.ts` (create)
|
||||||
|
|
||||||
|
**Interfaces:**
|
||||||
|
- Consumes: nothing.
|
||||||
|
- Produces:
|
||||||
|
- `export type MailOutcome = 'sent' | 'skipped-unconfigured' | 'skipped-blocked';`
|
||||||
|
- `sendMail(to: string, subject: string, html: string): Promise<MailOutcome>`
|
||||||
|
|
||||||
|
**Why first.** Every later task depends on being able to tell a skipped send from a real one, and this is the only change that touches a file seven other things already use.
|
||||||
|
|
||||||
|
- [ ] **Step 1: Write the failing test**
|
||||||
|
|
||||||
|
Create `backend/tests/unit/mailOutcome.test.ts`:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
import { sendMail } from '../../src/mailer';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What sendMail says it did.
|
||||||
|
*
|
||||||
|
* It returns early in two cases that are indistinguishable from success at the
|
||||||
|
* call site — SMTP unconfigured, and the recipient not on MAIL_ALLOWLIST — and
|
||||||
|
* #260 needs to tell them apart so the admin is not told a link was emailed
|
||||||
|
* when it was not.
|
||||||
|
*
|
||||||
|
* Only the two skip paths are covered. A real send needs an SMTP server, which
|
||||||
|
* a unit test has no business starting; the integration test in Task 4 covers
|
||||||
|
* the route's behaviour instead.
|
||||||
|
*/
|
||||||
|
describe('what sendMail reports', () => {
|
||||||
|
const original = { ...process.env };
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
process.env = { ...original };
|
||||||
|
});
|
||||||
|
|
||||||
|
it('says so when SMTP is not configured', async () => {
|
||||||
|
delete process.env.SMTP_USER;
|
||||||
|
delete process.env.SMTP_PASSWORD;
|
||||||
|
|
||||||
|
await expect(sendMail('someone@example.com', 'subject', '<p>body</p>')).resolves.toBe(
|
||||||
|
'skipped-unconfigured'
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The case that matters most: QA restricts delivery, and a blocked address
|
||||||
|
// previously returned exactly as though it had sent.
|
||||||
|
it('says so when the recipient is not on the allowlist', async () => {
|
||||||
|
process.env.SMTP_USER = 'user';
|
||||||
|
process.env.SMTP_PASSWORD = 'password';
|
||||||
|
process.env.MAIL_ALLOWLIST = 'allowed@example.com';
|
||||||
|
|
||||||
|
await expect(sendMail('someone-else@example.com', 'subject', '<p>body</p>')).resolves.toBe(
|
||||||
|
'skipped-blocked'
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not report blocked for an address that is on the allowlist', async () => {
|
||||||
|
process.env.SMTP_USER = 'user';
|
||||||
|
process.env.SMTP_PASSWORD = 'password';
|
||||||
|
process.env.MAIL_ALLOWLIST = 'allowed@example.com';
|
||||||
|
|
||||||
|
// Not asserting 'sent': that would need a live SMTP server. Asserting only
|
||||||
|
// that the allowlist did not refuse it, which is this test's subject.
|
||||||
|
await expect(
|
||||||
|
sendMail('allowed@example.com', 'subject', '<p>body</p>').catch(() => 'threw')
|
||||||
|
).resolves.not.toBe('skipped-blocked');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Run it to verify it fails**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npx jest -c jest.unit.config.js tests/unit/mailOutcome.test.ts
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: FAIL — `sendMail` resolves to `undefined`, not `'skipped-unconfigured'`.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Write the implementation**
|
||||||
|
|
||||||
|
In `backend/src/mailer.ts`, add the type above `sendMail`:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
/**
|
||||||
|
* What a send attempt actually did.
|
||||||
|
*
|
||||||
|
* `sendMail` returns early in two cases that used to be indistinguishable from
|
||||||
|
* success — no SMTP credentials, and a recipient outside MAIL_ALLOWLIST — which
|
||||||
|
* meant a caller could report "emailed" for a message nobody would ever
|
||||||
|
* receive. QA restricts delivery by design, so that was not a hypothetical: it
|
||||||
|
* is the normal case there. See #260.
|
||||||
|
*/
|
||||||
|
export type MailOutcome = 'sent' | 'skipped-unconfigured' | 'skipped-blocked';
|
||||||
|
```
|
||||||
|
|
||||||
|
Then change the signature and the three exits, leaving every existing comment in place:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
export async function sendMail(to: string, subject: string, html: string): Promise<MailOutcome> {
|
||||||
|
if (!process.env.SMTP_USER || !process.env.SMTP_PASSWORD) {
|
||||||
|
console.warn(`SMTP not configured — skipping email to ${to}: "${subject}"`);
|
||||||
|
return 'skipped-unconfigured';
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
if (!isAllowedRecipient(to, process.env.MAIL_ALLOWLIST)) {
|
||||||
|
console.warn(`[mail-blocked] ${to} is not on MAIL_ALLOWLIST — skipping "${subject}"`);
|
||||||
|
return 'skipped-blocked';
|
||||||
|
}
|
||||||
|
|
||||||
|
await transporter.sendMail({
|
||||||
|
from: process.env.SMTP_FROM || process.env.SMTP_USER,
|
||||||
|
to,
|
||||||
|
subject,
|
||||||
|
html
|
||||||
|
});
|
||||||
|
return 'sent';
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**Change nothing else in this file, and no call site.** All seven callers ignore the return value, which is legal — that is what makes this additive rather than breaking.
|
||||||
|
|
||||||
|
- [ ] **Step 4: Run the test and the whole unit suite**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npx jest -c jest.unit.config.js tests/unit/mailOutcome.test.ts && npm run test:unit && npm run build && npm run lint
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: PASS, 3 tests; the full unit suite still green; clean build and lint. The build is what proves no caller broke.
|
||||||
|
|
||||||
|
- [ ] **Step 5: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add backend/src/mailer.ts backend/tests/unit/mailOutcome.test.ts
|
||||||
|
git commit -F- <<'EOF'
|
||||||
|
feat(mail): have sendMail say what it actually did (#260)
|
||||||
|
|
||||||
|
It returned Promise<void> and returned early in two cases that were indistinguishable from success at the call site: no SMTP credentials, and a recipient outside MAIL_ALLOWLIST. A caller could therefore report that it had emailed somebody a message nobody would ever receive, and in QA — which restricts delivery deliberately, as its entire safety property — that is the normal case rather than an edge one.
|
||||||
|
|
||||||
|
It now returns a MailOutcome saying which of the three happened. No existing caller changes: there are seven and every one ignores the result, so this is additive. Re-deriving the answer at a second site would have duplicated isAllowedRecipient and the SMTP check, which is exactly the drift the guard-in-one-place comment above them exists to prevent.
|
||||||
|
|
||||||
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||||||
|
EOF
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 2: The column and the template
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Create: `backend/migrations/1787700000000_add-upload-link-contact-email.js`
|
||||||
|
- Modify: `backend/src/db-drizzle/schema.ts` (the `uploadLinks` block)
|
||||||
|
- Modify: `backend/src/emailTemplates.ts` (the `TemplateKey` union and `TEMPLATES`)
|
||||||
|
- Test: `backend/tests/unit/uploadLinkTemplate.test.ts` (create)
|
||||||
|
|
||||||
|
**Interfaces:**
|
||||||
|
- Consumes: nothing from Task 1.
|
||||||
|
- Produces: `upload_links.contact_email TEXT` (nullable); the `'uploadLink'` `TemplateKey` with `required: ['submitUrl']` and `available: ['submitUrl', 'label', 'submissionsAllowed']`.
|
||||||
|
|
||||||
|
- [ ] **Step 1: Write the failing test**
|
||||||
|
|
||||||
|
Create `backend/tests/unit/uploadLinkTemplate.test.ts`:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
import { TEMPLATES, missingPlaceholders, renderTemplate } from '../../src/emailTemplates';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The template that carries an upload link (#260).
|
||||||
|
*
|
||||||
|
* The one failure worth making impossible is a link email with no link in it:
|
||||||
|
* it sends, it looks fine in the log, and it is useless to the person who gets
|
||||||
|
* it. That is the same guard `verification` has on `verifyUrl`.
|
||||||
|
*/
|
||||||
|
describe('the upload link template', () => {
|
||||||
|
it('is offered in the admin like the others', () => {
|
||||||
|
expect(TEMPLATES.uploadLink.label).toBeTruthy();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('requires the submit link', () => {
|
||||||
|
expect(TEMPLATES.uploadLink.required).toContain('submitUrl');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses a body that has no link in it', () => {
|
||||||
|
expect(missingPlaceholders('uploadLink', 'Hello, a link is on its way.')).toEqual(['submitUrl']);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('accepts a body that has one', () => {
|
||||||
|
expect(missingPlaceholders('uploadLink', 'Send your items: {{submitUrl}}')).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('offers the label and the allowance as placeholders', () => {
|
||||||
|
expect(TEMPLATES.uploadLink.available).toEqual(
|
||||||
|
expect.arrayContaining(['submitUrl', 'label', 'submissionsAllowed'])
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// The default body has to satisfy the guard it declares, or the feature ships
|
||||||
|
// unable to send its own default.
|
||||||
|
it('has a default body that satisfies its own requirement', () => {
|
||||||
|
expect(missingPlaceholders('uploadLink', TEMPLATES.uploadLink.defaultBody)).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('renders the link into the body', () => {
|
||||||
|
const rendered = renderTemplate(
|
||||||
|
'uploadLink',
|
||||||
|
{ subject: 'Send us your items', body: 'Here: {{submitUrl}}' },
|
||||||
|
{ submitUrl: 'https://example.com/submit/abc', label: 'Sarah', submissionsAllowed: '25 items' }
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(rendered.html).toContain('https://example.com/submit/abc');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
```
|
||||||
|
|
||||||
|
Read `renderTemplate`'s real signature in `backend/src/emailTemplates.ts` before writing this — the third argument is the placeholder values, and the second is the stored subject/body. Adjust the call above to match it exactly rather than guessing.
|
||||||
|
|
||||||
|
- [ ] **Step 2: Run it to verify it fails**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npx jest -c jest.unit.config.js tests/unit/uploadLinkTemplate.test.ts
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: FAIL — `TEMPLATES.uploadLink` is undefined.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Add the template**
|
||||||
|
|
||||||
|
In `backend/src/emailTemplates.ts`, add to the `TemplateKey` union:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
| 'intakeDraft'
|
||||||
|
| 'uploadLink';
|
||||||
|
```
|
||||||
|
|
||||||
|
And add to `TEMPLATES`, following the shape of `intakeDraft`:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
uploadLink: {
|
||||||
|
label: 'Upload link for a contributor',
|
||||||
|
// The link itself, for the same reason verification requires verifyUrl: an
|
||||||
|
// email inviting somebody to send in photos, with no way to do it, sends
|
||||||
|
// perfectly happily and wastes everyone's time.
|
||||||
|
required: ['submitUrl'],
|
||||||
|
available: ['submitUrl', 'label', 'submissionsAllowed'],
|
||||||
|
defaultSubject: 'Send us your items',
|
||||||
|
defaultBody:
|
||||||
|
'You can send us photos of items you would like us to sell.\n\n' +
|
||||||
|
'[Send in an item]({{submitUrl}})\n\n' +
|
||||||
|
'You can send {{submissionsAllowed}}. Photograph one item at a time, and tell us anything you know about it — where it came from, what it is made of, any damage. A photo cannot show any of that.\n\n' +
|
||||||
|
'Keep this link to yourself: anyone who has it can send us items in your name.'
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 4: Write the migration**
|
||||||
|
|
||||||
|
Create `backend/migrations/1787700000000_add-upload-link-contact-email.js`:
|
||||||
|
|
||||||
|
```javascript
|
||||||
|
exports.up = (pgm) => {
|
||||||
|
pgm.sql(`
|
||||||
|
-- Where the link was sent (#260). Nullable, and deliberately so: links
|
||||||
|
-- already exist in QA and a migration cannot invent an address for them, so
|
||||||
|
-- they are grandfathered rather than backfilled with something untrue.
|
||||||
|
--
|
||||||
|
-- The requirement lives in the create route instead, which is where new
|
||||||
|
-- links are actually made. A NOT NULL column would have forced a choice
|
||||||
|
-- between inventing data and refusing to migrate.
|
||||||
|
ALTER TABLE upload_links
|
||||||
|
ADD COLUMN IF NOT EXISTS contact_email TEXT;
|
||||||
|
`);
|
||||||
|
};
|
||||||
|
|
||||||
|
exports.down = (pgm) => {
|
||||||
|
pgm.sql(`ALTER TABLE upload_links DROP COLUMN IF EXISTS contact_email;`);
|
||||||
|
};
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 5: Update the Drizzle mirror**
|
||||||
|
|
||||||
|
`backend/src/db-drizzle/schema.ts` is generated by `drizzle-kit pull`, and `drizzleSchema.integration.test.ts` asserts it declares every column of every table. The local dev database is probably not running, so add the line by hand in the `uploadLinks` block, matching the file's tab indentation and generated style:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
contactEmail: text("contact_email"),
|
||||||
|
```
|
||||||
|
|
||||||
|
`text` is already imported — do not add an import.
|
||||||
|
|
||||||
|
- [ ] **Step 6: Run the tests**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npm run db:test:up && npx jest -c jest.unit.config.js tests/unit/uploadLinkTemplate.test.ts && npm run test:integration -- drizzleSchema && npm run build && npm run lint
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: PASS. The Drizzle guard is what proves the mirror is not stale.
|
||||||
|
|
||||||
|
- [ ] **Step 7: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add backend/migrations/1787700000000_add-upload-link-contact-email.js backend/src/db-drizzle/schema.ts backend/src/emailTemplates.ts backend/tests/unit/uploadLinkTemplate.test.ts
|
||||||
|
git commit -F- <<'EOF'
|
||||||
|
feat(intake): record where an upload link was sent, and how to say it (#260)
|
||||||
|
|
||||||
|
Adds upload_links.contact_email and the uploadLink mail template.
|
||||||
|
|
||||||
|
The column is nullable on purpose. Links already exist in QA and a migration cannot invent addresses for them, so they are grandfathered rather than backfilled with something untrue; the requirement belongs in the create route, which is where new links are actually made.
|
||||||
|
|
||||||
|
The template requires submitUrl, the same guard verification has on verifyUrl. An email inviting somebody to send in photos, with no way for them to do it, sends perfectly happily and looks fine in the log — it is the one failure here worth making impossible, and a test asserts the default body satisfies the guard it declares.
|
||||||
|
|
||||||
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||||||
|
EOF
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 3: The route requires an address and sends the mail
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `backend/src/routes/adminUploadLinks.ts` (`LINK_SELECT` at :29, the create route at :59)
|
||||||
|
- Test: `backend/tests/integration/uploadLinks.integration.test.ts`
|
||||||
|
|
||||||
|
**Interfaces:**
|
||||||
|
- Consumes: `MailOutcome` and `sendMail` from Task 1; the column and template from Task 2; `isValidEmail` from `../utils`; `renderTemplate` from `../emailTemplates`; `loadStoredTemplate` from `./adminEmailTemplates`.
|
||||||
|
- Produces: `POST /api/admin/upload-links` requires `email`; responds `201 { ...link, token, url, mail: { sent: boolean, outcome: MailOutcome } }`. `GET` returns `contact_email` on every row.
|
||||||
|
|
||||||
|
- [ ] **Step 1: Write the failing test**
|
||||||
|
|
||||||
|
Append to `backend/tests/integration/uploadLinks.integration.test.ts`, following the arrangement its existing cases use. Read them first — they use `request(app)` directly.
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
describe('emailing the link to its recipient', () => {
|
||||||
|
const original = { ...process.env };
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
process.env = { ...original };
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses to create a link with no address', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
expect(res.body.error).toMatch(/email/i);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses an address that is not one', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'not-an-address' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
expect(res.body.error).toMatch(/email/i);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('stores the address on the link', async () => {
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(201);
|
||||||
|
|
||||||
|
const { rows } = await pool.query<{ contact_email: string }>(
|
||||||
|
`SELECT contact_email FROM upload_links WHERE id = $1`,
|
||||||
|
[res.body.id]
|
||||||
|
);
|
||||||
|
expect(rows[0]?.contact_email).toBe('sarah@example.com');
|
||||||
|
});
|
||||||
|
|
||||||
|
// The point of the whole change: QA blocks delivery by design, so a link that
|
||||||
|
// was not actually emailed must not be reported as though it was.
|
||||||
|
it('says the mail was not sent when SMTP is not configured', async () => {
|
||||||
|
delete process.env.SMTP_USER;
|
||||||
|
delete process.env.SMTP_PASSWORD;
|
||||||
|
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(201);
|
||||||
|
expect(res.body.mail).toEqual({ sent: false, outcome: 'skipped-unconfigured' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('says the mail was not sent when the address is not allowlisted', async () => {
|
||||||
|
process.env.SMTP_USER = 'user';
|
||||||
|
process.env.SMTP_PASSWORD = 'password';
|
||||||
|
process.env.MAIL_ALLOWLIST = 'someone@example.com';
|
||||||
|
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
|
expect(res.status).toBe(201);
|
||||||
|
expect(res.body.mail).toEqual({ sent: false, outcome: 'skipped-blocked' });
|
||||||
|
});
|
||||||
|
|
||||||
|
// A send that could not happen must never cost the admin the link, because
|
||||||
|
// the token is shown exactly once and a rollback would hand them a different
|
||||||
|
// one on the retry.
|
||||||
|
it('still returns a usable link when the mail did not go', async () => {
|
||||||
|
delete process.env.SMTP_USER;
|
||||||
|
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
|
expect(res.body.token).toBeTruthy();
|
||||||
|
expect(res.body.url).toContain(res.body.token);
|
||||||
|
expect(res.body.mail.sent).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('lists the address, and tolerates a link that has none', async () => {
|
||||||
|
await pool.query(`INSERT INTO upload_links (label, token_hash) VALUES ('Older link', 'digest')`);
|
||||||
|
await request(app)
|
||||||
|
.post('/api/admin/upload-links')
|
||||||
|
.send({ label: 'Newer link', email: 'sarah@example.com' });
|
||||||
|
|
||||||
|
const res = await request(app).get('/api/admin/upload-links');
|
||||||
|
|
||||||
|
const older = res.body.find((row: { label: string }) => row.label === 'Older link');
|
||||||
|
const newer = res.body.find((row: { label: string }) => row.label === 'Newer link');
|
||||||
|
expect(older.contact_email).toBeNull();
|
||||||
|
expect(newer.contact_email).toBe('sarah@example.com');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Run it to verify it fails**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npm run test:integration -- uploadLinks
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: FAIL — links are created without an address and there is no `mail` in the response.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Write the implementation**
|
||||||
|
|
||||||
|
In `backend/src/routes/adminUploadLinks.ts`, add the imports:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
import { sendMail } from '../mailer';
|
||||||
|
import { renderTemplate } from '../emailTemplates';
|
||||||
|
import { loadStoredTemplate } from './adminEmailTemplates';
|
||||||
|
import { isValidEmail } from '../utils';
|
||||||
|
```
|
||||||
|
|
||||||
|
Add `contact_email` to `LINK_SELECT` and to the `UploadLinkRow` interface:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
const LINK_SELECT = `
|
||||||
|
SELECT id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at
|
||||||
|
FROM upload_links
|
||||||
|
`;
|
||||||
|
```
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
interface UploadLinkRow {
|
||||||
|
id: number;
|
||||||
|
label: string;
|
||||||
|
contact_email: string | null;
|
||||||
|
revoked_at: string | null;
|
||||||
|
submission_count: number;
|
||||||
|
max_submissions: number | null;
|
||||||
|
last_used_at: string | null;
|
||||||
|
created_at: string;
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
Add a helper above the create route:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
/**
|
||||||
|
* What the mail tells the recipient about how much they may send.
|
||||||
|
*
|
||||||
|
* Words rather than a bare number for an uncapped link, so the sentence reads
|
||||||
|
* as a sentence instead of showing an empty space where a figure should be.
|
||||||
|
* An uncapped link is a deliberate choice the admin already had to make, so it
|
||||||
|
* is emailable like any other.
|
||||||
|
*/
|
||||||
|
function submissionsAllowed(maxSubmissions: number | null): string {
|
||||||
|
if (maxSubmissions === null) return 'as many items as you like';
|
||||||
|
return maxSubmissions === 1 ? '1 item' : `${maxSubmissions} items`;
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
In the create route, validate the address immediately after the label check:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
const email = typeof req.body?.email === 'string' ? req.body.email.trim() : '';
|
||||||
|
if (email === '' || !isValidEmail(email)) {
|
||||||
|
return res.status(400).json({ error: 'a valid email address is required' });
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
Store it in the INSERT:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
const { rows } = await pool.query<UploadLinkRow>(
|
||||||
|
`INSERT INTO upload_links (label, token_hash, max_submissions, contact_email)
|
||||||
|
VALUES ($1, $2, $3, $4)
|
||||||
|
RETURNING id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
|
||||||
|
[label, hashToken(token), maxSubmissions, email]
|
||||||
|
);
|
||||||
|
```
|
||||||
|
|
||||||
|
And replace the final response with the send and the report:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
const url = `${base}/submit/${token}`;
|
||||||
|
|
||||||
|
// Awaited, and its outcome reported rather than swallowed. Every other sender
|
||||||
|
// in this codebase fires and forgets because nobody is waiting on the answer;
|
||||||
|
// here somebody is — the admin is looking at the screen, and whether they
|
||||||
|
// now have to send the link by hand is the thing they need to know.
|
||||||
|
//
|
||||||
|
// A failure does not roll the link back. The token is shown exactly once, so
|
||||||
|
// a rollback would leave the admin retrying and holding a different link,
|
||||||
|
// discarding work that succeeded for the sake of tidiness.
|
||||||
|
let outcome: MailOutcome = 'skipped-unconfigured';
|
||||||
|
try {
|
||||||
|
const template = renderTemplate('uploadLink', await loadStoredTemplate('uploadLink'), {
|
||||||
|
submitUrl: url,
|
||||||
|
label: link.label,
|
||||||
|
submissionsAllowed: submissionsAllowed(link.max_submissions)
|
||||||
|
});
|
||||||
|
outcome = await sendMail(email, template.subject, template.html);
|
||||||
|
} catch (err) {
|
||||||
|
// Reported, not thrown. The link exists and is usable; the admin needs to
|
||||||
|
// be told the mail did not go, not handed a 500 for a link that was made.
|
||||||
|
console.error(`[upload-links] could not email ${email}:`, err);
|
||||||
|
outcome = 'skipped-unconfigured';
|
||||||
|
}
|
||||||
|
|
||||||
|
res.status(201).json({
|
||||||
|
...link,
|
||||||
|
token,
|
||||||
|
url,
|
||||||
|
mail: { sent: outcome === 'sent', outcome }
|
||||||
|
});
|
||||||
|
```
|
||||||
|
|
||||||
|
Import `MailOutcome` as a type alongside `sendMail`. Check `renderTemplate`'s real signature and adjust the call to match it exactly.
|
||||||
|
|
||||||
|
**A note on the catch:** an SMTP rejection lands here and is reported as `skipped-unconfigured`, which is not strictly accurate. Leave it. Adding a fourth outcome for "the server refused it" would be a real distinction, but nothing consumes it and the admin's action is identical either way — copy the link and send it by hand. Say so in the commit rather than inventing the case.
|
||||||
|
|
||||||
|
- [ ] **Step 4: Run the tests**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npm run test:integration -- uploadLinks && npx jest -c jest.unit.config.js tests/unit/routesAreWrapped.test.ts && npm run build && npm run lint
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: PASS. The wrapper guard confirms no handler was added unwrapped.
|
||||||
|
|
||||||
|
- [ ] **Step 5: Run the whole backend suite**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd backend && npm run test:unit && npm run test:integration
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: everything green. This is what catches any other test that created a link with only a label.
|
||||||
|
|
||||||
|
- [ ] **Step 6: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add backend/src/routes/adminUploadLinks.ts backend/tests/integration/uploadLinks.integration.test.ts
|
||||||
|
git commit -F- <<'EOF'
|
||||||
|
feat(intake): require an address for an upload link and send the link to it (#260)
|
||||||
|
|
||||||
|
Creating a link now requires a valid email address and mails the link to it, which is the whole point: getting a link to a contributor was previously a copy-and-paste into whatever the admin happened to use.
|
||||||
|
|
||||||
|
The send is awaited and its outcome reported, unlike every other sender in this codebase, which fires and forgets because nobody is waiting on the answer. Here somebody is. The admin is looking at the screen, and whether they now have to send the link by hand is exactly the thing they need to know — and QA blocks delivery to any address outside MAIL_ALLOWLIST by design, so a link that was never emailed would otherwise look precisely like one that was.
|
||||||
|
|
||||||
|
A send that could not happen does not roll the link back. The token is displayed exactly once, so a rollback would leave the admin retrying and holding a different link, discarding work that had succeeded. They end up with a usable link and an honest statement about delivery instead.
|
||||||
|
|
||||||
|
One inaccuracy left deliberately: an SMTP rejection is reported as skipped-unconfigured rather than a fourth outcome of its own. The distinction is real but nothing consumes it, and the admin's next action is identical either way.
|
||||||
|
|
||||||
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||||||
|
EOF
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 4: The admin screen, and the callers this breaks
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `frontend/src/admin/UploadLinks.tsx`
|
||||||
|
- Modify: `frontend/tests/e2e/admin-upload-links.spec.ts`
|
||||||
|
- Modify: `frontend/tests/e2e/intake-submit.spec.ts:18`, `frontend/tests/e2e/admin-draft-queue.spec.ts:16`
|
||||||
|
|
||||||
|
**Interfaces:**
|
||||||
|
- Consumes: the route from Task 3.
|
||||||
|
- Produces: nothing later tasks depend on.
|
||||||
|
|
||||||
|
**Why the last three files are here.** Both specs create a link with `api.post('/api/admin/upload-links', { data: { label } })`, which the route now refuses. They are not incidental damage — they are the cost of the requirement, and leaving them for the suite to find later would mean two unrelated features going red.
|
||||||
|
|
||||||
|
- [ ] **Step 1: Add the field to the form**
|
||||||
|
|
||||||
|
In `frontend/src/admin/UploadLinks.tsx`, add state beside `label` (around line 38):
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
const [email, setEmail] = useState('');
|
||||||
|
```
|
||||||
|
|
||||||
|
Add the input after the label input (around line 105), matching the surrounding style and using a deep antd import if `Input` is not already imported:
|
||||||
|
|
||||||
|
```tsx
|
||||||
|
<Input
|
||||||
|
aria-label="Contributor email"
|
||||||
|
placeholder="Where should the link be sent?"
|
||||||
|
value={email}
|
||||||
|
onChange={(e) => setEmail(e.target.value)}
|
||||||
|
/>
|
||||||
|
```
|
||||||
|
|
||||||
|
Send it (around line 68):
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
label,
|
||||||
|
email,
|
||||||
|
maxSubmissions: unlimited ? null : Number(cap)
|
||||||
|
```
|
||||||
|
|
||||||
|
Disable the button until both are filled (around line 120):
|
||||||
|
|
||||||
|
```tsx
|
||||||
|
disabled={label.trim() === '' || email.trim() === ''}
|
||||||
|
```
|
||||||
|
|
||||||
|
Reset it in the create handler, beside the existing `setLabel('')`:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
setIssued(created.url);
|
||||||
|
setLabel('');
|
||||||
|
setEmail('');
|
||||||
|
setCap(DEFAULT_CAP);
|
||||||
|
```
|
||||||
|
|
||||||
|
Add the column to the table (around line 153):
|
||||||
|
|
||||||
|
```tsx
|
||||||
|
{ title: 'Label', dataIndex: 'label' },
|
||||||
|
{ title: 'Sent to', dataIndex: 'contact_email' },
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Tell the admin when the mail did not go**
|
||||||
|
|
||||||
|
The component holds the created link as `const [issued, setIssued] = useState<string | null>(null)` and sets it with `setIssued(created.url)` — a bare URL string, with nowhere to put a delivery outcome. Add a second piece of state beside it rather than widening `issued`, so the existing one-time-token display is untouched:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
// Whether the link that is currently on screen was actually emailed. Separate
|
||||||
|
// from `issued` so the one-time display of the token keeps working exactly as
|
||||||
|
// it did; this only adds a note beside it.
|
||||||
|
const [mailed, setMailed] = useState<boolean | null>(null);
|
||||||
|
```
|
||||||
|
|
||||||
|
Extend `UploadLink` with the new column, so the table can show it:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
interface UploadLink {
|
||||||
|
id: number;
|
||||||
|
label: string;
|
||||||
|
contact_email: string | null;
|
||||||
|
revoked_at: string | null;
|
||||||
|
```
|
||||||
|
|
||||||
|
In the create handler, set it from the response alongside `setIssued`:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
const created = await res.json();
|
||||||
|
setIssued(created.url);
|
||||||
|
setMailed(created.mail.sent);
|
||||||
|
```
|
||||||
|
|
||||||
|
And reset it wherever a new create begins, beside `setError(null)`, so a previous link's outcome cannot be read as this one's:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
setMailed(null);
|
||||||
|
```
|
||||||
|
|
||||||
|
Then render the warning next to wherever `issued` is displayed. The link stays on screen regardless — that is the point of not rolling back:
|
||||||
|
|
||||||
|
```tsx
|
||||||
|
{issued && mailed === false && (
|
||||||
|
<Alert
|
||||||
|
type="warning"
|
||||||
|
showIcon
|
||||||
|
message="The link was not emailed"
|
||||||
|
description="Copy it below and send it yourself. This is normal where no mail is configured, and in QA, where delivery is restricted to a fixed list of addresses."
|
||||||
|
/>
|
||||||
|
)}
|
||||||
|
```
|
||||||
|
|
||||||
|
`Alert` is imported with a deep import if it is not already: `import Alert from 'antd/es/alert';`. Check the file's existing imports first — several antd components are already imported there.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Fix the two specs that create links without an address**
|
||||||
|
|
||||||
|
In `frontend/tests/e2e/intake-submit.spec.ts` (line 18) and `frontend/tests/e2e/admin-draft-queue.spec.ts` (line 16), add an address to the `data` object:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
data: { label: `Intake spec ${RUN}`, email: `intake-${RUN}@example.com` }
|
||||||
|
```
|
||||||
|
|
||||||
|
Use the label each file already builds and an address carrying the same run id, so a failure names the run that caused it. `@example.com` is reserved for exactly this and cannot reach a real person.
|
||||||
|
|
||||||
|
- [ ] **Step 4: Update the upload-links spec**
|
||||||
|
|
||||||
|
In `frontend/tests/e2e/admin-upload-links.spec.ts`, every test that fills `Link label` must now also fill `Contributor email` before the create button is enabled. Add to each of the three:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
await page.getByLabel('Contributor email').fill(`${uniqueSuffix()}@example.com`);
|
||||||
|
```
|
||||||
|
|
||||||
|
And add one case asserting the requirement, inside the existing describe:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
// The address is the point of the change: a link nobody can be sent is the
|
||||||
|
// thing this replaced.
|
||||||
|
test('will not create a link without an address', async ({ page, admin }) => {
|
||||||
|
await admin.open('Upload Links');
|
||||||
|
await page.getByLabel('Link label').fill(`Nameless ${uniqueSuffix()}`);
|
||||||
|
|
||||||
|
await expect(page.getByRole('button', { name: 'Create link' })).toBeDisabled();
|
||||||
|
});
|
||||||
|
```
|
||||||
|
|
||||||
|
Read the file first: the create button's accessible name may not be `Create link`, and `admin.open`'s argument must match the real tab name. Use what is there.
|
||||||
|
|
||||||
|
- [ ] **Step 5: Verify the frontend**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd frontend && npm run build && npm run lint && npm run test:unit
|
||||||
|
```
|
||||||
|
|
||||||
|
`npm run build`, not a bare `npx tsc --noEmit` — the app tsconfig excludes `tests/`.
|
||||||
|
|
||||||
|
Expected: clean build, clean lint, unit tests pass.
|
||||||
|
|
||||||
|
- [ ] **Step 6: Run the e2e suite**
|
||||||
|
|
||||||
|
The local stack must be running. **Do not start it yourself** — ask the user.
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd frontend && npx playwright test admin-upload-links intake-submit admin-draft-queue --project=chromium
|
||||||
|
```
|
||||||
|
|
||||||
|
Then the whole suite, which is what finds anything else that created a link with only a label:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd frontend && npx playwright test --project=chromium
|
||||||
|
```
|
||||||
|
|
||||||
|
Expected: all green.
|
||||||
|
|
||||||
|
- [ ] **Step 7: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add frontend/src/admin/UploadLinks.tsx frontend/tests/e2e/admin-upload-links.spec.ts frontend/tests/e2e/intake-submit.spec.ts frontend/tests/e2e/admin-draft-queue.spec.ts
|
||||||
|
git commit -F- <<'EOF'
|
||||||
|
feat(admin): ask for the contributor's address when creating a link (#260)
|
||||||
|
|
||||||
|
The address is now a required field beside the label, the links table shows where each link was sent, and the admin is told plainly when the mail did not go — with the link still on screen to copy, which is the case that matters in QA and in local development where there is no mail at all.
|
||||||
|
|
||||||
|
Two specs in unrelated features created links with only a label and the route now refuses that, so they are updated here rather than left to go red on somebody else's branch. That is the cost of making the address required, and it is a small one: the compiler and the suite find every call site.
|
||||||
|
|
||||||
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||||||
|
EOF
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## After the plan
|
||||||
|
|
||||||
|
- The branch is `feature/260-email-the-upload-link`. **Do not push** — the user pushes and merges.
|
||||||
|
- The PR closes #260 and should say plainly that the mail carries the working token, and why that trade was judged acceptable: an upload link grants only "submit into a queue a person must approve", bounded by `max_submissions` and #227's ceiling.
|
||||||
|
- Per standing practice, follow with a separate SonarQube cleanup issue and PR — never folded into this branch.
|
||||||
|
- **Testing this in QA needs care.** `MAIL_ALLOWLIST` is hardcoded to a single address in `docker-compose.qa.yml` and is deliberately not a stack variable, because it is the entire safety property stopping a QA run emailing real people. Use that address or a `+suffix` variant of it. Any other address will report `skipped-blocked`, which is the feature working correctly.
|
||||||
@@ -0,0 +1,91 @@
|
|||||||
|
# Emailing an upload link to the person who will use it
|
||||||
|
|
||||||
|
**Issue:** #260. Builds on #222 (upload links) and #224 (the intake notification, which is the closest existing mail).
|
||||||
|
|
||||||
|
Today an admin creates an upload link, the token is shown once in the console, and getting it to a contributor is a copy-and-paste into whatever the admin happens to use. This makes the address part of creating the link, and sends the link to it.
|
||||||
|
|
||||||
|
## Decisions, and what each one rests on
|
||||||
|
|
||||||
|
**The mail carries the working link.** The issue framed this as a loosening comparable to a password reset, and that framing overstates it. A reset token takes over an account; an upload token grants exactly one capability — submit photos into a queue where a person must approve them before anything is published. It reads nothing, it is revocable, `max_submissions` caps it, and #227 caps the whole intake surface regardless. The worst outcome of a leaked upload link is junk in the review queue, which is bounded and reversible. That is a reasonable thing to put in an inbox; a reset link is a much larger bet and this project already makes that one.
|
||||||
|
|
||||||
|
**The address is required for new links, and the column is nullable.** Those are not in tension: links already exist in QA and a migration cannot invent addresses for them, so they are grandfathered at null, and the requirement lives in the route where new links are made. Making the column `NOT NULL` would mean either inventing data or a backfill that lies.
|
||||||
|
|
||||||
|
**The address lives on the link, not on a contributor.** A link already carries a label naming who it is for. A contributor entity would be the better model only if the same people submit repeatedly through different links, which nothing yet suggests, and it is materially more work.
|
||||||
|
|
||||||
|
**A failed send does not lose the link.** The token is displayed exactly once, so rolling the creation back on a send failure would leave the admin retrying and getting a different link — harmless but confusing, and it throws away work that succeeded. Instead the link is created, the send is attempted, and the response says which happened.
|
||||||
|
|
||||||
|
That last point is not defensive programming for its own sake. **QA sets `MAIL_ALLOWLIST` and silently skips any address outside it**, logging `[mail-blocked]` and returning as though it sent. Without an explicit outcome in the response, testing this in QA against a contributor's real address looks exactly like success. The issue itself warns this would otherwise waste an afternoon.
|
||||||
|
|
||||||
|
## Architecture
|
||||||
|
|
||||||
|
```
|
||||||
|
Admin | Upload Links
|
||||||
|
label + email + cap
|
||||||
|
│
|
||||||
|
▼
|
||||||
|
POST /api/admin/upload-links
|
||||||
|
├─ isValidEmail(email) 400 naming the field
|
||||||
|
├─ INSERT upload_links contact_email alongside label and cap
|
||||||
|
├─ renderTemplate('uploadLink') submitUrl required, as verification requires verifyUrl
|
||||||
|
└─ sendMail(...) → outcome 'sent' | 'skipped-unconfigured' | 'skipped-blocked'
|
||||||
|
│
|
||||||
|
▼
|
||||||
|
201 { ...link, token, url, mail: { sent: boolean, outcome: MailOutcome } }
|
||||||
|
```
|
||||||
|
|
||||||
|
### `sendMail` gains a return value
|
||||||
|
|
||||||
|
`sendMail` currently returns `Promise<void>` and returns early in two cases — SMTP unconfigured, and the recipient not on `MAIL_ALLOWLIST` — both indistinguishable from success at the call site. It will return a small result instead:
|
||||||
|
|
||||||
|
```ts
|
||||||
|
export type MailOutcome = 'sent' | 'skipped-unconfigured' | 'skipped-blocked';
|
||||||
|
```
|
||||||
|
|
||||||
|
**No existing caller changes.** There are seven, and every one ignores the return value; ignoring a returned value is legal, so this is additive rather than a breaking change. The alternative — re-deriving "would this address be blocked?" in the route — would duplicate `isAllowedRecipient` and the SMTP check at a second site, which is exactly the drift the guard-in-one-place comment in `mailer.ts` exists to prevent.
|
||||||
|
|
||||||
|
### Data
|
||||||
|
|
||||||
|
`upload_links.contact_email TEXT` — nullable, no default. Null means a link made before this existed, or by a caller that predates the requirement. Nothing reads it except the admin list and the mail at creation.
|
||||||
|
|
||||||
|
### The template
|
||||||
|
|
||||||
|
A new `uploadLink` key in `TEMPLATES`, editable in the admin like the others.
|
||||||
|
|
||||||
|
- `required: ['submitUrl']` — the same guard that stops a verification mail shipping without its link. A link email with no link is the one failure worth making impossible.
|
||||||
|
- `available: ['submitUrl', 'label', 'submissionsAllowed']`
|
||||||
|
- `submissionsAllowed` renders as a number for a capped link and as words for an uncapped one, so an unlimited link reads as a sentence rather than as a missing value. Pinned exactly, so it is not a coin-flip at implementation time: a capped link renders `25 items`, or `1 item` at a cap of one; an uncapped link renders `as many items as you like`. The default body uses it as "You can send {{submissionsAllowed}}."
|
||||||
|
|
||||||
|
### The admin screen
|
||||||
|
|
||||||
|
Email becomes a required field in the create form, beside the label. The links table gains the address, so it is visible who a link was sent to. When the response reports the mail was not sent, the admin is told plainly and the link is still shown to copy.
|
||||||
|
|
||||||
|
## Failure handling
|
||||||
|
|
||||||
|
| What happens | Result |
|
||||||
|
|---|---|
|
||||||
|
| Address missing or malformed | 400 naming the field. No link created. |
|
||||||
|
| SMTP not configured (local development) | Link created. `mail.sent` false, `mail.outcome` `'skipped-unconfigured'`. |
|
||||||
|
| Address not on `MAIL_ALLOWLIST` (QA) | Link created. `mail.sent` false, `mail.outcome` `'skipped-blocked'`. |
|
||||||
|
| SMTP rejects the message | Link created. `mail.sent` false. The admin copies the link by hand. |
|
||||||
|
| Everything works | Link created, `mail.sent` true, `mail.outcome` `'sent'`. |
|
||||||
|
|
||||||
|
`mail.outcome` is always present and always one of the three values, including on success — a field that appears only on failure is one every consumer has to remember to check for.
|
||||||
|
|
||||||
|
A send failure never rolls back the link, and never turns into a 500. The admin always ends up holding a usable link and an honest statement of whether it was delivered.
|
||||||
|
|
||||||
|
## Testing
|
||||||
|
|
||||||
|
- **Integration:** the address is required; a malformed one is refused naming the field; a valid one is stored; the mail is attempted with the rendered template; a send failure still returns 201 with a usable token and `mail.sent` false. Existing links with a null address still list and still work.
|
||||||
|
- **Unit:** the `uploadLink` template refuses a body with no `submitUrl`, through the existing `missingPlaceholders` guard.
|
||||||
|
- **E2E:** the create form refuses to submit without an address, and a created link shows its address in the table.
|
||||||
|
- **Existing callers:** the e2e fixtures and `admin-upload-links.spec.ts` create links with only a label and must be updated. That is the cost of the requirement, and the compiler and the suite find every one.
|
||||||
|
|
||||||
|
## Out of scope
|
||||||
|
|
||||||
|
**Re-sending a lost link.** Only the digest is stored, so the original token cannot be recovered. "Re-send" would mean minting a new link and revoking the old one — a different feature with its own decisions about what the recipient is told.
|
||||||
|
|
||||||
|
**Notifying the address on revoke.** Not asked for, and it is a separate judgement about whether a contributor should learn their link was withdrawn.
|
||||||
|
|
||||||
|
**A contributor entity.** See the decisions above.
|
||||||
|
|
||||||
|
**Any change to the seven existing `sendMail` call sites.** They keep ignoring the outcome. Making each of them report delivery is a larger piece of work with no demand behind it.
|
||||||
@@ -14,6 +14,7 @@ const { Paragraph, Text } = Typography;
|
|||||||
interface UploadLink {
|
interface UploadLink {
|
||||||
id: number;
|
id: number;
|
||||||
label: string;
|
label: string;
|
||||||
|
contact_email: string | null;
|
||||||
revoked_at: string | null;
|
revoked_at: string | null;
|
||||||
submission_count: number;
|
submission_count: number;
|
||||||
max_submissions: number | null;
|
max_submissions: number | null;
|
||||||
@@ -36,11 +37,16 @@ const DEFAULT_CAP = '25';
|
|||||||
export default function UploadLinks() {
|
export default function UploadLinks() {
|
||||||
const [links, setLinks] = useState<UploadLink[]>([]);
|
const [links, setLinks] = useState<UploadLink[]>([]);
|
||||||
const [label, setLabel] = useState('');
|
const [label, setLabel] = useState('');
|
||||||
|
const [email, setEmail] = useState('');
|
||||||
const [cap, setCap] = useState(DEFAULT_CAP);
|
const [cap, setCap] = useState(DEFAULT_CAP);
|
||||||
const [unlimited, setUnlimited] = useState(false);
|
const [unlimited, setUnlimited] = useState(false);
|
||||||
// Held only in component state and shown once. A refresh loses it, which is
|
// Held only in component state and shown once. A refresh loses it, which is
|
||||||
// the honest behaviour: the server genuinely cannot produce it again.
|
// the honest behaviour: the server genuinely cannot produce it again.
|
||||||
const [issued, setIssued] = useState<string | null>(null);
|
const [issued, setIssued] = useState<string | null>(null);
|
||||||
|
// Whether the link that is currently on screen was actually emailed. Separate
|
||||||
|
// from `issued` so the one-time display of the token keeps working exactly as
|
||||||
|
// it did; this only adds a note beside it.
|
||||||
|
const [mailed, setMailed] = useState<boolean | null>(null);
|
||||||
const [error, setError] = useState<string | null>(null);
|
const [error, setError] = useState<string | null>(null);
|
||||||
const [creating, setCreating] = useState(false);
|
const [creating, setCreating] = useState(false);
|
||||||
|
|
||||||
@@ -66,6 +72,7 @@ export default function UploadLinks() {
|
|||||||
// that are not this screen.
|
// that are not this screen.
|
||||||
body: JSON.stringify({
|
body: JSON.stringify({
|
||||||
label,
|
label,
|
||||||
|
email,
|
||||||
maxSubmissions: unlimited ? null : Number(cap)
|
maxSubmissions: unlimited ? null : Number(cap)
|
||||||
})
|
})
|
||||||
});
|
});
|
||||||
@@ -79,8 +86,14 @@ export default function UploadLinks() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const created = await res.json();
|
const created = await res.json();
|
||||||
|
// Reset here, not before the request: a 400 returns before this point, so
|
||||||
|
// rejecting a second link (say, a mistyped address) can no longer clear
|
||||||
|
// the warning that belongs to a still-displayed token from an earlier,
|
||||||
|
// successful create.
|
||||||
|
setMailed(created.mail.sent);
|
||||||
setIssued(created.url);
|
setIssued(created.url);
|
||||||
setLabel('');
|
setLabel('');
|
||||||
|
setEmail('');
|
||||||
setCap(DEFAULT_CAP);
|
setCap(DEFAULT_CAP);
|
||||||
setUnlimited(false);
|
setUnlimited(false);
|
||||||
await load();
|
await load();
|
||||||
@@ -106,6 +119,13 @@ export default function UploadLinks() {
|
|||||||
onChange={(e) => setLabel(e.target.value)}
|
onChange={(e) => setLabel(e.target.value)}
|
||||||
style={{ width: 260 }}
|
style={{ width: 260 }}
|
||||||
/>
|
/>
|
||||||
|
<Input
|
||||||
|
aria-label="Contributor email"
|
||||||
|
placeholder="Where should the link be sent?"
|
||||||
|
value={email}
|
||||||
|
onChange={(e) => setEmail(e.target.value)}
|
||||||
|
style={{ width: 260 }}
|
||||||
|
/>
|
||||||
<Input
|
<Input
|
||||||
placeholder="Max uses"
|
placeholder="Max uses"
|
||||||
aria-label="Maximum uses"
|
aria-label="Maximum uses"
|
||||||
@@ -117,13 +137,27 @@ export default function UploadLinks() {
|
|||||||
<Checkbox checked={unlimited} onChange={(e) => setUnlimited(e.target.checked)}>
|
<Checkbox checked={unlimited} onChange={(e) => setUnlimited(e.target.checked)}>
|
||||||
No limit
|
No limit
|
||||||
</Checkbox>
|
</Checkbox>
|
||||||
<Button type="primary" onClick={create} loading={creating} disabled={label.trim() === ''}>
|
<Button
|
||||||
|
type="primary"
|
||||||
|
onClick={create}
|
||||||
|
loading={creating}
|
||||||
|
disabled={label.trim() === '' || email.trim() === ''}
|
||||||
|
>
|
||||||
Create link
|
Create link
|
||||||
</Button>
|
</Button>
|
||||||
</Space>
|
</Space>
|
||||||
|
|
||||||
{error && <Alert type="error" message={error} showIcon />}
|
{error && <Alert type="error" message={error} showIcon />}
|
||||||
|
|
||||||
|
{issued && mailed === false && (
|
||||||
|
<Alert
|
||||||
|
type="warning"
|
||||||
|
showIcon
|
||||||
|
message="The link was not emailed"
|
||||||
|
description="Copy it below and send it yourself. This is normal where no mail is configured, and in QA, where delivery is restricted to a fixed list of addresses."
|
||||||
|
/>
|
||||||
|
)}
|
||||||
|
|
||||||
{issued && (
|
{issued && (
|
||||||
<Alert
|
<Alert
|
||||||
type="success"
|
type="success"
|
||||||
@@ -151,6 +185,12 @@ export default function UploadLinks() {
|
|||||||
pagination={false}
|
pagination={false}
|
||||||
columns={[
|
columns={[
|
||||||
{ title: 'Label', dataIndex: 'label' },
|
{ title: 'Label', dataIndex: 'label' },
|
||||||
|
// Not "Sent to": contact_email records only the address the admin
|
||||||
|
// gave when the link was created, never whether delivery actually
|
||||||
|
// happened — that outcome is shown once, at creation, and is not
|
||||||
|
// persisted. In QA, where every send is blocked by design, "Sent
|
||||||
|
// to" would be false for every row on the page.
|
||||||
|
{ title: 'Email', dataIndex: 'contact_email' },
|
||||||
{
|
{
|
||||||
title: 'Used',
|
title: 'Used',
|
||||||
render: (_, row) =>
|
render: (_, row) =>
|
||||||
|
|||||||
@@ -14,7 +14,7 @@ let token: string;
|
|||||||
test.beforeAll(async ({ playwright }) => {
|
test.beforeAll(async ({ playwright }) => {
|
||||||
const api = await createAdminContext(playwright);
|
const api = await createAdminContext(playwright);
|
||||||
const res = await api.post('/api/admin/upload-links', {
|
const res = await api.post('/api/admin/upload-links', {
|
||||||
data: { label: `Review queue spec ${RUN}` }
|
data: { label: `Review queue spec ${RUN}`, email: `draft-queue-${RUN}@example.com` }
|
||||||
});
|
});
|
||||||
expect(res.status(), 'creating the upload link').toBe(201);
|
expect(res.status(), 'creating the upload link').toBe(201);
|
||||||
token = (await res.json()).token;
|
token = (await res.json()).token;
|
||||||
|
|||||||
@@ -12,11 +12,13 @@ import { test, expect, uniqueSuffix } from './fixtures';
|
|||||||
test.describe('Managing upload links', () => {
|
test.describe('Managing upload links', () => {
|
||||||
test('issues a link, shows its token once, and lists it bounded', async ({ page, admin }) => {
|
test('issues a link, shows its token once, and lists it bounded', async ({ page, admin }) => {
|
||||||
const label = `Sarah ${uniqueSuffix()}`;
|
const label = `Sarah ${uniqueSuffix()}`;
|
||||||
|
const email = `${uniqueSuffix()}@example.com`;
|
||||||
|
|
||||||
await admin.goto();
|
await admin.goto();
|
||||||
await page.getByRole('tab', { name: 'Upload links' }).click();
|
await page.getByRole('tab', { name: 'Upload links' }).click();
|
||||||
|
|
||||||
await page.getByLabel('Link label').fill(label);
|
await page.getByLabel('Link label').fill(label);
|
||||||
|
await page.getByLabel('Contributor email').fill(email);
|
||||||
await page.getByRole('button', { name: 'Create link' }).click();
|
await page.getByRole('button', { name: 'Create link' }).click();
|
||||||
|
|
||||||
// Shown exactly once. The server keeps only a digest, so there is no
|
// Shown exactly once. The server keeps only a digest, so there is no
|
||||||
@@ -27,6 +29,8 @@ test.describe('Managing upload links', () => {
|
|||||||
|
|
||||||
const row = page.getByRole('row', { name: new RegExp(label) });
|
const row = page.getByRole('row', { name: new RegExp(label) });
|
||||||
await expect(row).toBeVisible();
|
await expect(row).toBeVisible();
|
||||||
|
// A created link shows its address in the table.
|
||||||
|
await expect(row.getByText(email)).toBeVisible();
|
||||||
// The default cap, not unlimited. An unbounded link should be asked for.
|
// The default cap, not unlimited. An unbounded link should be asked for.
|
||||||
await expect(row.getByText('0 of 25')).toBeVisible();
|
await expect(row.getByText('0 of 25')).toBeVisible();
|
||||||
await expect(row.getByText('Active')).toBeVisible();
|
await expect(row.getByText('Active')).toBeVisible();
|
||||||
@@ -38,6 +42,7 @@ test.describe('Managing upload links', () => {
|
|||||||
await admin.goto();
|
await admin.goto();
|
||||||
await page.getByRole('tab', { name: 'Upload links' }).click();
|
await page.getByRole('tab', { name: 'Upload links' }).click();
|
||||||
await page.getByLabel('Link label').fill(label);
|
await page.getByLabel('Link label').fill(label);
|
||||||
|
await page.getByLabel('Contributor email').fill(`${uniqueSuffix()}@example.com`);
|
||||||
await page.getByRole('button', { name: 'Create link' }).click();
|
await page.getByRole('button', { name: 'Create link' }).click();
|
||||||
|
|
||||||
const row = page.getByRole('row', { name: new RegExp(label) });
|
const row = page.getByRole('row', { name: new RegExp(label) });
|
||||||
@@ -61,6 +66,7 @@ test.describe('Managing upload links', () => {
|
|||||||
await admin.goto();
|
await admin.goto();
|
||||||
await page.getByRole('tab', { name: 'Upload links' }).click();
|
await page.getByRole('tab', { name: 'Upload links' }).click();
|
||||||
await page.getByLabel('Link label').fill(label);
|
await page.getByLabel('Link label').fill(label);
|
||||||
|
await page.getByLabel('Contributor email').fill(`${uniqueSuffix()}@example.com`);
|
||||||
await page.getByText('No limit').click();
|
await page.getByText('No limit').click();
|
||||||
await page.getByRole('button', { name: 'Create link' }).click();
|
await page.getByRole('button', { name: 'Create link' }).click();
|
||||||
|
|
||||||
@@ -69,4 +75,14 @@ test.describe('Managing upload links', () => {
|
|||||||
// A bare count rather than "0 of N".
|
// A bare count rather than "0 of N".
|
||||||
await expect(row.getByText('0 of', { exact: false })).toHaveCount(0);
|
await expect(row.getByText('0 of', { exact: false })).toHaveCount(0);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// The address is the point of the change: a link nobody can be sent is the
|
||||||
|
// thing this replaced.
|
||||||
|
test('will not create a link without an address', async ({ page, admin }) => {
|
||||||
|
await admin.goto();
|
||||||
|
await page.getByRole('tab', { name: 'Upload links' }).click();
|
||||||
|
await page.getByLabel('Link label').fill(`Nameless ${uniqueSuffix()}`);
|
||||||
|
|
||||||
|
await expect(page.getByRole('button', { name: 'Create link' })).toBeDisabled();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -16,7 +16,7 @@ test.beforeAll(async ({ playwright }) => {
|
|||||||
const api = await createAdminContext(playwright);
|
const api = await createAdminContext(playwright);
|
||||||
|
|
||||||
const res = await api.post('/api/admin/upload-links', {
|
const res = await api.post('/api/admin/upload-links', {
|
||||||
data: { label: `Intake spec ${RUN}` }
|
data: { label: `Intake spec ${RUN}`, email: `intake-${RUN}@example.com` }
|
||||||
});
|
});
|
||||||
expect(res.status(), 'creating the upload link').toBe(201);
|
expect(res.status(), 'creating the upload link').toBe(201);
|
||||||
token = (await res.json()).token;
|
token = (await res.json()).token;
|
||||||
|
|||||||
Reference in New Issue
Block a user