100 lines
14 KiB
Markdown
100 lines
14 KiB
Markdown
# Phase 8 Follow-Up — Business Name as Email Sender: Summary
|
|
|
|
A focused fix on top of Phase 8: every outgoing email's visible sender **name** now comes from the installed business's own name, not `EMAIL_FROM_NAME` or the internal product name. The sender **email address** stays exactly as it was — environment-configured, since it has to match whatever address/domain is actually verified with the SMTP relay.
|
|
|
|
---
|
|
|
|
## Where the business name is resolved
|
|
|
|
In `src/services/notifications/render-email.ts`'s `renderEmailForNotification()` — the one function that turns a stored `Notification` row into an actual email, already responsible for loading the business (for the existing name/logo/address/policy fields every template uses). It now also does:
|
|
|
|
```ts
|
|
const business = await prisma.business.findUnique({ where: { id: notification.businessId }, include: {...} });
|
|
const sender: EmailSender = { name: resolveSenderName(business.name) };
|
|
```
|
|
|
|
`notification.businessId` is a required, non-nullable column (`prisma/schema.prisma`) — every `Notification` in this schema is already business-scoped, so this is a `findUnique` on the notification's own explicit relationship, never a global `findFirst()`. Verified directly with an integration test that creates two businesses and confirms a notification scoped to business A never resolves business B's name, even though B exists in the same database (`sender-identity.integration.test.ts`, "never lets one business's notification resolve another business's name").
|
|
|
|
This lookup happens **fresh, every time** — `renderEmailForNotification` is called by the background processor right before a `PENDING` delivery is actually sent (`reminder-worker.ts`), never at notification-creation time. That was already true before this change (see that file's own header, written in Phase 7); this change just rides the same guarantee for the sender name. No caching beyond the implicit request-scoped Prisma round trip — nothing is cached across requests or worker runs, so a business rename takes effect on the very next email sent, with no restart and no environment-variable change. Verified directly: `sender-identity.integration.test.ts`'s "uses the updated business name for the very next email after a rename" test sends one email, renames the business via a plain `prisma.business.update()` (the same call `BusinessService.updateBusinessProfile` makes from Owner Settings), sends a second email in the same process, and asserts the second one uses the new name.
|
|
|
|
## How it reaches the Brevo provider
|
|
|
|
`renderEmailForNotification()` now returns `RenderedEmailWithSender` (`{ subject, html, text, sender }`) instead of the old `RenderedEmail`. `reminder-worker.ts`'s `processEmailDelivery()` — the **only** call site of `EmailProvider.sendEmail()` in the entire codebase (confirmed by grep before making this change) — passes `sender: email.sender` straight through:
|
|
|
|
```ts
|
|
const email = await renderEmailForNotification(prisma, delivery.notification);
|
|
const sendResult = await emailProvider.sendEmail({
|
|
to: { email: recipientEmail },
|
|
sender: email.sender,
|
|
subject: email.subject,
|
|
html: email.html,
|
|
text: email.text,
|
|
idempotencyKey: delivery.id,
|
|
});
|
|
```
|
|
|
|
`SendEmailParams` (`src/services/notifications/providers/email-provider.ts`) gained a required `sender: EmailSender` field (`{ name: string; email?: string }`). `BrevoEmailProvider.sendEmail()` uses `params.sender.name` for the SMTP `from` display name, and **always** its own `this.fromAddress` (from `BrevoEmailProviderConfig`, env-configured) for the address — `params.sender.email` is accepted in the shape for forward-compatibility with a future provider that might need it, but Brevo's own implementation ignores it, documented inline and covered by a test (`brevo-email-provider.test.ts`, "sends with the per-message sender name and the provider's own configured address, ignoring any sender.email"). `BrevoEmailProviderConfig` itself dropped `fromName` entirely — it now only carries `host`/`port`/`username`/`password`/`fromAddress`, i.e. only static, non-business-derived provider settings, per the architecture this was explicitly asked to follow. `loadBrevoEmailProviderConfigFromEnv()` no longer reads `EMAIL_FROM_NAME` at all — that env var is read exactly once, inside `resolveSenderName()` (see below), not at provider-construction time.
|
|
|
|
`ConsoleEmailProvider` (the dev default) also prints the resolved sender name (`From: {name}`) so local development shows exactly what would be sent — this is what the manual verification below used.
|
|
|
|
## The sender-name fallback order
|
|
|
|
`src/services/notifications/sender-identity.ts` — a small, pure, DB-free function:
|
|
|
|
```ts
|
|
export function resolveSenderName(businessName: string | null | undefined): string {
|
|
const trimmedBusinessName = businessName?.trim();
|
|
if (trimmedBusinessName) return trimmedBusinessName;
|
|
|
|
const envFallback = process.env.EMAIL_FROM_NAME?.trim();
|
|
if (envFallback) return envFallback;
|
|
|
|
return "Booking Notifications";
|
|
}
|
|
```
|
|
|
|
1. **The installed business's own name** — the normal, always-taken path for a real installed application (every `Notification` is business-scoped, and `Business.name` is a required, non-empty column populated by the installer/seed).
|
|
2. **`EMAIL_FROM_NAME`**, only reached if tier 1 somehow resolves to nothing (a blank business name, or, in principle, a future notification type with no business association at all — no such type exists in this codebase today). Never required to be kept in sync with the business name.
|
|
3. **`"Booking Notifications"`** — a neutral, non-product name, reached only if neither of the above is set. `NextChair` never appears anywhere in this function.
|
|
|
|
Unit-tested directly (`sender-identity.test.ts`): business name wins even when `EMAIL_FROM_NAME` is also set; `EMAIL_FROM_NAME` is used only when the business name is null/undefined/empty/whitespace; the neutral fallback is used when neither is available; and a regression guard asserting the function never returns `"NextChair"` at any tier.
|
|
|
|
## Why the sender email stays environment-configured
|
|
|
|
Brevo (and SMTP relays generally) require the sending address to belong to a domain/address actually verified with the account — an arbitrary per-message address would either be silently rewritten by the relay or rejected outright. The business name has no such constraint (any string is a valid display name), so it was safe to make per-message and business-derived; the address was not, and stays exactly where it was before this change — a single environment variable (`EMAIL_FROM_ADDRESS`) loaded once into `BrevoEmailProviderConfig`. `.env.example` and `.env.local.example` were updated to make this explicit for `EMAIL_FROM_NAME` specifically (now documented as "optional... only used when no business name can be resolved... you do not need to set or keep this in sync with Business Settings"); `EMAIL_FROM_ADDRESS`'s own existing documentation was untouched, since its behaviour didn't change.
|
|
|
|
## Delivery-record snapshotting
|
|
|
|
**Not snapshotted.** `NotificationDelivery` gained no new column for this. This follows the existing, explicit design philosophy stated in `render-email.ts`'s own header (written in Phase 7, unchanged by this follow-up): never store rendered content on the row, always re-derive it from current data at send time. Adding a `senderNameUsed` column would cut against that stated design for a "where useful" (the brief's own wording — optional) requirement, and — because `Business` itself has no history/versioning table — a snapshot column would only be reliable going forward, not retroactively reconstructable for anything sent before it existed. `providerMessageId`/`providerName` already exist on `NotificationDelivery` for send-forensics; reconstructing "what name did we actually show" for a historical delivery would require resolving what the business's name was at `sentAt`, which this app cannot do today (documented here as a known limitation, not fixed, since it wasn't asked for and would need a real audit/versioning mechanism to do properly).
|
|
|
|
## Background notification processing
|
|
|
|
No change was needed to `processDueNotificationDeliveries`'s own structure — it already calls `renderEmailForNotification()` fresh for every delivery it processes, so the "resolve current business identity when rendering/sending, don't assume creation-time branding is still current" requirement was already satisfied by the existing Phase 7 architecture. This follow-up only added the sender-name resolution to that same, already-correct call. Verified directly: `sender-identity.integration.test.ts`'s "passes the resolved business name all the way through the background processor to the email provider" test goes through the real `processDueNotificationDeliveries()` entry point (not just `renderEmailForNotification()` directly) and asserts the `MockEmailProvider` captured the correct sender name.
|
|
|
|
## Email types covered
|
|
|
|
Every email type is covered by construction, not type-by-type special-casing: `renderEmailForNotification()` is the **only** path to an outgoing email (confirmed: `EmailProvider.sendEmail()` has exactly one call site, `reminder-worker.ts`), and it resolves and attaches the sender identically regardless of which `NotificationType` it's rendering or which template (a specific booking/loyalty template, or the generic fallback template) ends up producing the subject/html/text. This includes `STAFF_INVITATION_RECEIVED` (currently rendered via the generic fallback template — the specialized `renderStaffInvitationEmail`/`renderStaffScheduleChangedEmail` functions in `templates/staff-emails.ts` are not wired into `renderEmailForNotification`'s dispatch switch, a pre-existing gap from Phase 7 left untouched here since it's out of this follow-up's scope), which is why a dedicated integration test exists for it specifically (`sender-identity.integration.test.ts`, "resolves the business name for a staff-invitation notification").
|
|
|
|
## Tests added
|
|
|
|
**Unit** (`src/services/notifications/__tests__/sender-identity.test.ts`, 6 tests): business name used when present; business name wins over `EMAIL_FROM_NAME`; `EMAIL_FROM_NAME` used only when no business name; neutral fallback used when neither is set (including whitespace-only business names); `NextChair` never returned at any tier.
|
|
|
|
**Unit, updated** (`brevo-email-provider.test.ts`): existing tests updated for the new `sender` field; one new test asserts the provider uses the per-message sender name and its own configured address, explicitly ignoring `sender.email`.
|
|
|
|
**Integration, real MySQL** (`src/services/notifications/__tests__/sender-identity.integration.test.ts`, 5 tests): a booking/loyalty-style notification resolves the installed business's name; a staff-invitation notification resolves it too (via the generic template); the full background-processor path (`processDueNotificationDeliveries`) passes the resolved name through to the email provider; renaming the business mid-process changes the very next email's sender with no restart; one business's notification never resolves another business's name even when both exist in the same database.
|
|
|
|
Full regression run after these changes: 236 unit tests (was 230), 163 integration tests (was 158) — all passing, zero new failures.
|
|
|
|
## Manual verification
|
|
|
|
Ran directly against the real seeded local MySQL database (not a scripted mock), using the actual `ConsoleEmailProvider` the app selects when `EMAIL_PROVIDER=console` (this environment's local `.env` setting) — deliberately with `.env`'s `EMAIL_FROM_NAME` left at its old, stale, misconfigured value (`"NextChair"`) specifically to prove that value is no longer used as the primary source:
|
|
|
|
- Every real, seeded notification (loyalty stamp, staff new-appointment, owner new-booking) rendered and "sent" through the actual `processDueNotificationDeliveries` → `renderEmailForNotification` → `ConsoleEmailProvider` pipeline showed `From: Blake Heritage Barbershop` — never `From: NextChair`, despite the misconfigured env var.
|
|
- Renamed the business directly via `prisma.business.update()` (the same underlying call Owner Settings' "Business Details" form makes) to `"Verify Rename Barbershop"`, in the same running process, no restart.
|
|
- The very next notification sent immediately afterward showed `From: Verify Rename Barbershop` — confirming no caching, no restart required, no environment-variable change required.
|
|
- Reverted the business name back to its original value afterward, leaving the seeded database as found.
|
|
|
|
Not independently re-verified this specific change through a full Docker Compose install cycle: `docker-compose.local.yaml`/`Dockerfile` were not touched by this follow-up, the email-sending code path is identical in a container (same source, same `ConsoleEmailProvider`/`BrevoEmailProvider` classes), and Phase 8's own summary already recorded a full, real Docker Compose lifecycle verification (build → empty volume → install → seed → restart-persists → teardown). Re-running that entire cycle here would not exercise anything Docker-specific about this particular change; the sender-name behaviour itself was verified against the real database and real (non-mocked, for the "would send" log) code path instead, which is the part that's actually new.
|
|
|
|
Not verified (unchanged from Phase 8's own "Unverified external-provider behaviour" section, still true here): a real Brevo SMTP send with the new sender-name behaviour — no live Brevo credentials were available in this environment. `BrevoEmailProvider`'s exact `nodemailer.sendMail({ from: { address, name } })` call is unit-tested (mocked transporter, asserting the correct `from` object is constructed), which is the same level of provider-boundary confidence this whole project has used for Brevo throughout every prior phase.
|