Feature/260 email the upload link #292

2026-09-03T18:50:05-05:00

bermudalamb commented

2026-09-03 18:50:05 -05:00

No description provided.

bermudalamb added 13 commits 2026-09-03 18:50:06 -05:00

docs(intake): design emailing an upload link to its recipient (#260 ) 9e1e650786

The issue asked for the security question to be settled before any code, and it is: the mail carries the working link.

The issue framed that as a loosening comparable to a password reset, and on inspection 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 reaches the storefront. It reads nothing, it is revocable, max_submissions caps it, and #227 caps the whole intake surface regardless of any single link. 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, and this project already makes the much larger bet with reset links.

The address is required for new links while the column stays nullable, which is not a contradiction: links already exist in QA and a migration cannot invent addresses for them, so they are grandfathered, and the requirement belongs in the route where new links are actually made. It lives on the link rather than on a contributor entity, because a link already carries a label naming who it is for and nothing yet suggests the same people submit repeatedly.

A failed send 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. The link is created, the send is attempted, and the response says which happened — which matters concretely because QA's MAIL_ALLOWLIST silently skips any address outside it and returns as though it sent. Without an explicit outcome, testing this in QA against a contributor's real address looks exactly like success, which is the afternoon the issue warned would otherwise be wasted.

Writing it turned up one thing the design had assumed and the code does not support. sendMail returns Promise<void> and returns early both when SMTP is unconfigured and when the recipient is not allowlisted, so a caller cannot tell either from success. It gains a MailOutcome return value instead. No existing caller changes — there are seven and every one ignores the result — and the alternative would have duplicated isAllowedRecipient and the SMTP check at a second site, which is the drift the guard-in-one-place comment in mailer.ts exists to prevent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

docs(intake): plan emailing the upload link (#260 ) a9e865b4bc

Four tasks over the approved design: sendMail gains an outcome, the column and template land together, the route requires an address and reports what the send did, and the admin screen asks for it.

sendMail goes first deliberately. Everything else depends on being able to tell a skipped send from a real one, and it is the only change touching a file seven other things already use — so if it is going to break anything, it should break before three tasks are stacked on top of it.

The plan is explicit that no existing caller changes. Ignoring a returned value is legal, which is what makes widening the return type additive rather than breaking, and re-deriving "would this address be blocked?" in the route would have duplicated isAllowedRecipient and the SMTP check at a second site.

Two specs in unrelated features create links with only a label, and the route will refuse that. They are fixed in the same task as the form rather than left for the suite to find, because the alternative is two unrelated features going red on somebody else's branch. That cost is named in the plan rather than discovered.

One inaccuracy is left in deliberately and said out loud: an SMTP rejection is reported as skipped-unconfigured rather than a fourth outcome. The distinction is real, nothing consumes it, and the admin's next action is identical either way — copy the link and send it by hand.

Self-review caught the failure mode from #281, where tasks referred to helpers that did not exist. Task 4 originally said the created-link state "may not be called created". Reading the component showed it is `issued` and holds a bare URL string with nowhere to put a delivery outcome, so the plan now adds a separate `mailed` state beside it rather than widening the one-time token display. Every name in that task is now one that exists.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

feat(mail): have sendMail say what it actually did (#260 ) 8a35c213fc

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>

test(mail): stop the allowlist test from reaching real Gmail (#260 ) 823796b92a

Review of the Task 1 commit approved the implementation but flagged the third test in mailOutcome.test.ts as a defect carried over from the brief: it set SMTP_USER, SMTP_PASSWORD, and an allowlist the recipient satisfied, so sendMail fell through both early-return guards and reached the real transporter, opening a live TLS connection to smtp.gmail.com:465. The .catch(() => 'threw') wrapper hid a fast auth rejection, a slow timeout, or an accidental real send equally, and on a restricted CI runner it would hang to the Jest timeout rather than fail fast.

The fix deletes that test rather than replacing it. What it was trying to prove — that an allowlisted recipient is not blocked — is already covered hermetically by backend/tests/unit/mailAllowlist.test.ts, which exercises isAllowedRecipient directly across exact matches, plus-suffixes, domains, and refusals. The other two tests in mailOutcome.test.ts are untouched; they cover the two paths that return early, which is the entire point of the change, and neither one reaches the transporter.

The file's top doc comment is updated to match: it now says only the two skip paths are covered here, names mailAllowlist.test.ts as where the allowlist's own behaviour is tested, and spells out why a third test that reaches the transporter does not belong in this file, so nobody adds one back.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

feat(intake): record where an upload link was sent, and how to say it (#260 ) eaecc43379

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>

feat(intake): require an address for an upload link and send the link to it (#260 ) b18b3e3a3d

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.

Also updates the other integration tests that created a link with only a label, since an address is now required, and adds the uploadLink template key that GET /api/admin/email-templates was missing from its list — an omission left by the template's addition in the prior commit on this branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

feat(admin): ask for the contributor's address when creating a link (#260 ) 9c5935e8e0

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>

test(integration): mock the mailer everywhere an upload link is issued (#260 ) 5a9022d8d1

intake.integration.test.ts, intakeCeiling.integration.test.ts and uploadLinks.integration.test.ts all create upload links, and since #260 that now sends real mail. env.setup.ts never clears SMTP_USER, SMTP_PASSWORD or MAIL_ALLOWLIST, so with those inherited from a developer's shell these three files opened live TLS connections to smtp.gmail.com:465 and, with no allowlist set, actually delivered to sarah@example.com. This is the exact hazard the "Do not add one back" comment in tests/unit/mailOutcome.test.ts already warns about, reintroduced at the integration layer.

All three now mock ../../src/mailer the same way accountDetails.integration.test.ts, favorites.integration.test.ts and resendVerification.integration.test.ts already do. uploadLinks.integration.test.ts is the one place that needs to see specific MailOutcome values come back through the route, so its two outcome tests were restructured to drive the mock's return value directly (sentMail.mockResolvedValueOnce(...)) instead of threading SMTP_USER/MAIL_ALLOWLIST through the real sendMail. That is a cleaner test anyway: it isolates the route's job (reporting whatever outcome sendMail returns) from sendMail's own skip logic, which is already covered hermetically by mailOutcome.test.ts and mailAllowlist.test.ts.

Also addresses the related minor finding that nothing asserted the mail actually carried the working link: required: ['submitUrl'] on the template only guards that the placeholder is present in the body, not that the route supplied a correct value for it. A new test in uploadLinks.integration.test.ts inspects the mock's captured call and asserts the html contains the created token's /submit/ URL, and covers the three submissionsAllowed phrasings (a numeric cap, a cap of exactly one, and uncapped).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

fix(mail): bound the transporter's timeouts, and fix a now-stale comment (#260 ) 0dd4ac36ff

adminUploadLinks.ts awaits sendMail on the admin's request path, the first awaited send on a user-facing request in this codebase, but the transporter in mailer.ts set no connectionTimeout, greetingTimeout or socketTimeout. nodemailer's defaults then apply: two minutes to connect, ten minutes on the socket. If the SMTP host is unreachable in a way that drops packets rather than refusing, the link row and its token are already committed by the time sendMail is called, the response hangs for up to two minutes, the browser or reverse proxy gives up first, and the token — shown exactly once and unrecoverable — is never rendered. That is the link being lost in exactly the way this feature's central invariant forbids.

All three timeouts are now set to 5000ms, with a comment explaining why a send on a request path has to fail fast rather than inherit nodemailer's fire-and-forget defaults. Five seconds is generous for a reachable host and short enough that a dead one fails while the admin is still willing to wait, leaving them the "not emailed" warning and a link they can still copy instead of a stuck spinner and a token nobody ever saw.

Also corrects the comment directly above the skipped-blocked return, which said "returning as though it sent" — true before #260, and precisely backwards now that the outcome is reported through MailOutcome rather than swallowed. Reworded to describe what the code actually does today, keeping the explanation of why it skips rather than throws.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

fix(mail-templates): add sample values for the upload-link placeholders (#260 ) ba26ee99f0

submitUrl, label and submissionsAllowed were added to the uploadLink template but SAMPLE_VALUES had no entries for any of them, so an admin opening Email templates, Upload link for a contributor, Preview saw literal {{submitUrl}} in the body — the preview being the only way to check an edit before saving.

The unit test meant to catch exactly this, in emailTemplates.test.ts, iterated a hardcoded KEYS array that predated intakeDraft and uploadLink, so it never checked either template's samples. KEYS is now Object.keys(TEMPLATES) as TemplateKey[], so the guard covers every template automatically and cannot go stale the same way again. intakeDraft already had samples for all its placeholders and passes once included, as expected.

One other test in the same file, "every template can address the customer", asserts that available contains greeting/firstName/lastName — a real invariant of the six customer-facing templates, but not of intakeDraft or uploadLink, which notify the shop and a contributor rather than a customer with a name on file. Switching that test to the new all-templates KEYS would have made it fail for both, so it now uses its own explicit CUSTOMER_FACING_KEYS list instead. That is a deliberate, commented exception: a hardcoded list is correct there because the claim itself does not extend to every template, whereas the SAMPLE_VALUES guard's claim does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

fix(admin): stop the upload links table claiming delivery it never recorded (#260 ) c12d40dc02

The table headed its address column "Sent to", but contact_email records only the intent an admin typed in, never whether delivery happened — that outcome is shown once, at creation, and is not persisted. In QA, where every send is blocked by design, every row read "Sent to ..." for links that were never emailed, and the honest warning that appears at creation is guarded on `issued`, so it vanishes on refresh, leaving the false heading as the only surviving statement. Renamed to "Email", which is true of what the column actually stores.

Separately, create() called setMailed(null) before every request, including one that would go on to 400. If an admin creates link A while mail is down (warning shown, token A still on screen) and then mistypes an address on a second attempt, the 400 path returned early — but the reset had already run, so the warning for link A disappeared while token A was still displayed on the same screen. setMailed is now only called after a successful create, alongside setIssued, so a request that never produces a new link can no longer clear a warning that belongs to the one still shown.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

fix(admin): return contact_email from the revoke query (#260 ) 4e7f255782

The revoke route's RETURNING clause omitted contact_email while its result was typed as UploadLinkRow, which declares the field as present. No user-visible effect since the response was never checked for it, but the type asserted something the query did not actually return. Added the column so the two agree.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

test(e2e): assert a created link's address shows in the table (#260 )

SonarQube Analysis / sonarqube (pull_request) Successful in 38m0s

Details

Linting / lint (pull_request) Successful in 4m43s

Details

046937cdce

The spec's E2E section requires that a created link shows its address in the table, but the test that creates one filled a throwaway address and never asserted the cell. The address is now kept in a variable and asserted on the row after creation, using it to locate the row's own cell rather than any other row's.

Not run: the local stack is down and Playwright was explicitly out of bounds for this pass, per the task instructions. This is written and verified by inspection and lint only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

bermudalamb merged commit b8ea33f45d into main

2026-09-03 18:50:37 -05:00

bermudalamb deleted branch feature/260-email-the-upload-link

2026-09-03 18:50:37 -05:00

bermudalamb referenced this pull request

2026-09-04 09:06:02 -05:00

Upload links should require an email address and send the link to it on creation #260

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#292