Skip to content

Commit aa2dbab

Browse files
fix(plugin-email): Send test with "None (log only)" picked but not saved no longer delivers over the live SMTP (#25519)
<!-- claude-projects-attribution: {"github_login":"os-sales"} --> _Requested by **Sales** · [project thread](https://claude.ai/code/project/chan_01D5bWFY3rFzgJcytD5gD4Uu?thread=cmsg_01D5bWFY3rFzgJcytD5gD4UuACdPWFJTLDcTrByFCHLzqb)_ Fixes #25509 Clause-②: no Metadata-protocol: none ## What was wrong Setup → Email's "Send test email" (the `mail/test` handler in `packages/plugins/plugin-email/src/email-plugin.ts`) tests the form's merged values, saved or not: `smtp` builds a one-shot `SmtpTransport`, Resend / Postmark build a one-shot service. `provider === 'log'` matched neither branch, so `target` stayed the LIVE service. With SMTP live and "None (log only — no real delivery)" picked on the form but not saved, the press delivered a real mail and answered `ok: true` "Sent test email to …". ## The change - New `provider === 'log'` branch: when the live transport is not already a `LogTransport`, the test goes through a one-shot `EmailService` over a new `LogTransport`, with the same `persistence` option as the smtp branch (the press is still recorded in `sys_email`). When the live transport already IS a `LogTransport` (a saved `log`, or the out-of-the-box namespace), the target stays the live service and the answer is the existing saved-`log` warning, unchanged. - The answer for the unsaved-`log` press uses the saved-`log` answer's severity and words (`ok: false`, `severity: 'warning'`, "only logged and recorded in sys_email"), the banner's phrase for a log-only transport ("no mail left this machine"), and names the live transport it did not use, so the two doors agree: `Nothing was delivered — the provider on the form is "None (log only)", so the test message was only logged and recorded in sys_email; no mail left this machine. The live transport was not used (Mail is delivered through SMTP (smtp.saved.test:587)); save the form to stop delivery.` - The other providers' branch is the old `provider !== 'log'` body, untouched (it became the `else`). - Landing: the file the claim names. `packages/spec` is not touched. ## Tests `packages/plugins/plugin-email/src/mail-log-provider.pin.test.ts` gains a describe block (its harness's `sendTest` now takes an optional unsaved form state, posted as `payload.values`, as the page does): - SMTP saved on the page and live, form provider `log`: `nodemailer.sendMail` is never called; the answer is `ok: false` / `warning`, starts "Nothing was delivered", says "no mail left this machine" and names `SMTP (smtp.saved.test:587)`; the live transport is still the `SmtpTransport` afterwards. - SMTP built at boot from the boot options (`OS_EMAIL_SMTP_*` as `providerOptions`), form provider `log`: no `sendMail`, same answer shape. - CONTROL: form provider `smtp` with a host still sends (`sendMail` once, `ok: true`, "Sent test email to admin@example.test via smtp (smtp.form.test:587)"). Fixture re-judged: two pins in `email-plugin.queue-delivery.test.ts` ("mail/test is never queued") pressed the button with form provider `log` to reach the harness's host transport, which is exactly the behaviour this PR removes. They now press with an SMTP host on the form, faking only nodemailer's wire (`sendMail` resolves; `sendMail` rejects with `535 5.7.8`), and keep every assertion they had (inline answer from the transport, no `sys_job_queue` job, audit row finalized `sent`). A third leg keeps the queue invariant on the LIVE service: a live `LogTransport` with queue delivery on answers the warning inline, with no job. Runs (head `79a4642ce8`, after merging `origin/main`; the merge brought no plugin-email path): - `pnpm --filter @objectstack/plugin-email test`: Test Files 38 passed (38), Tests 689 passed (689). - `pnpm --filter @objectstack/plugin-email typecheck`: exit 0 (`tsc --noEmit`, then `check:test-typecheck: OK`). **Ablation** (head `2485802f70`, through `scripts/ablation-replace.mjs`): the anchor `if (!(svc.options.transport instanceof LogTransport)) {` was replaced by `if (false /* ABLATION-25509 */) {`. The tool reported "ok mutation landed: anchor 1 -> 0, blob a4f67d1 -> 6d31a251e7e3". The pin went RED: Tests 2 failed | 6 passed (8), both unsaved-`log` legs failing on "expected vi.fn() to not be called at all, but actually been called 1 times" (the live SMTP sent), with the CONTROL and the saved-`log` pins holding. The restore reported "ok restored: blob == HEAD (a4f67d1) and `git diff HEAD` is empty". The test imports `./email-plugin.js` from `src`, so no `dist` leg applies. ## Gates `node scripts/pm/dispatch-gates.mjs --commands` at `79a4642ce8` derives 67 commands; all 67 ran locally with their exit codes recorded. `--ran` says: "67 derived famil(ies) accounted for — 66 run, 1 NOT-MEASURED (1 DERIVED from a recorded exit 3)". - NOT MEASURED: `check:dual-build-cjs-loads`, reason: PREREQUISITE NOT MET (it reads built output of every package; this worktree built only plugin-email's closure). CI's Build Core / Lint lanes carry it. - The CI-only families (Test Core, Build Core, the type-check lanes, artifact rosters) are read from this PR's check-runs. ## Acceptance notes - The handler's own doc comment already says a one-shot temporary service is built when the form differs from the live one; the `log` branch now honours it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016g7nYP9wdNx1GDa5XDr7Uy --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7a70be4 commit aa2dbab

4 files changed

Lines changed: 146 additions & 9 deletions

File tree

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
'@objectstack/plugin-email': patch
3+
---
4+
5+
fix(plugin-email): "Send test email" with "None (log only)" picked but not saved no longer delivers over the live transport
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
Setup → Email's "Send test email" tests what the form says, saved or not: an SMTP host on the form gets a one-shot SMTP connection, a Resend / Postmark key a one-shot client. With "None (log only — no real delivery)" picked and not saved, the test used to go through the live service instead, so with SMTP live a real message was delivered and the answer was "Sent test email to …". The test now goes through a one-shot `LogTransport` (recorded in `sys_email` like any other test send) and answers with a warning that nothing was delivered and no mail left the machine, naming the live transport it did not use. Once `log` is saved the answer is the existing warning that no delivery transport is active. The other providers are unchanged.

‎packages/plugins/plugin-email/src/email-plugin.queue-delivery.test.ts‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,16 @@ import { createMemoryQueue } from '@objectstack/core';
1515
import { assertEngineDeleteDispatch } from '@objectstack/objectql';
1616
import { DbQueueAdapter } from '@objectstack/service-queue';
1717
import { EmailServicePlugin, resolveDurableQueue } from './email-plugin.js';
18-
import { EmailService, EMAIL_SEND_QUEUE } from './email-service.js';
18+
import { EmailService, EMAIL_SEND_QUEUE, LogTransport } from './email-service.js';
19+
20+
// "Send test email" tests the FORM's provider through a one-shot transport, so
21+
// a press with an SMTP host on the form reaches nodemailer, never the harness
22+
// transport. Only the SMTP wire is faked; everything above it is real.
23+
const nm = vi.hoisted(() => ({ createTransport: vi.fn(), sendMail: vi.fn() }));
24+
vi.mock('nodemailer', () => ({
25+
default: { createTransport: nm.createTransport },
26+
createTransport: nm.createTransport,
27+
}));
1928

2029
// ── harness ────────────────────────────────────────────────────────────────
2130

@@ -511,15 +520,28 @@ describe('settings gate — a save degrades, it never breaks the boot or the mai
511520
// ── mail/test ──────────────────────────────────────────────────────────────
512521

513522
describe('mail/test is never queued', () => {
523+
// The form names an SMTP host, so the press builds a one-shot SMTP service
524+
// and its answer is that connection's own. (A form provider of `log` used to
525+
// reach the harness transport here: the press tested the LIVE service while
526+
// the form said "None (log only)". It now tests a one-shot LogTransport.)
527+
const SMTP_FORM = { provider: 'smtp', smtp_host: 'smtp.form.test', smtp_port: 587, from_email: 'no-reply@example.test' };
528+
529+
beforeEach(() => {
530+
nm.createTransport.mockReset();
531+
nm.sendMail.mockReset();
532+
nm.createTransport.mockImplementation(() => ({ sendMail: nm.sendMail, close: vi.fn() }));
533+
});
534+
514535
it('sends inline and reports the transport result, with queue mode on', async () => {
536+
nm.sendMail.mockResolvedValue({ messageId: '<wire-1@smtp>', response: '250 2.0.0 Ok' });
515537
const h = await boot({
516538
plugin: { queueDelivery: true },
517539
mail: { queue_delivery: { value: true, source: 'global' } },
518540
});
519541
await h.ready();
520542

521543
const result = await h.settings.action('test')!({
522-
values: { provider: 'log', from_email: 'no-reply@example.test' },
544+
values: SMTP_FORM,
523545
payload: { to: 'ops@example.test' },
524546
});
525547

@@ -528,29 +550,52 @@ describe('mail/test is never queued', () => {
528550
// removed from this button.
529551
expect(result.ok).toBe(true);
530552
expect(String(result.message)).toMatch(/Sent test email to ops@example\.test/);
531-
expect(h.transport.send).toHaveBeenCalledTimes(1);
553+
expect(nm.sendMail).toHaveBeenCalledTimes(1);
554+
expect(h.transport.send).not.toHaveBeenCalled();
532555
expect(h.jobs()).toHaveLength(0);
533556
// The audit row is finalized, not left at `queued` for a worker.
534557
expect(last(h.sysEmail())).toMatchObject({ status: 'sent' });
535558
});
536559

537560
it('reports a real SMTP failure instead of enqueueing it', async () => {
561+
nm.sendMail.mockRejectedValue(new Error('535 5.7.8 authentication failed'));
538562
const h = await boot({
539-
transport: { send: vi.fn(async () => { throw new Error('535 5.7.8 authentication failed'); }) },
540563
plugin: { queueDelivery: true },
541564
mail: { queue_delivery: { value: true, source: 'global' } },
542565
});
543566
await h.ready();
544567

545568
const result = await h.settings.action('test')!({
546-
values: { provider: 'log', from_email: 'no-reply@example.test' },
569+
values: SMTP_FORM,
547570
payload: { to: 'ops@example.test' },
548571
});
549572

550573
expect(result.ok).toBe(false);
551574
expect(String(result.message)).toMatch(/535 5\.7\.8/);
552575
expect(h.jobs()).toHaveLength(0);
553576
});
577+
578+
it('a press that tests the LIVE service (a live LogTransport, provider log) is inline too', async () => {
579+
const h = await boot({
580+
plugin: { queueDelivery: true },
581+
mail: {
582+
provider: { value: 'log', source: 'global' },
583+
queue_delivery: { value: true, source: 'global' },
584+
},
585+
});
586+
await h.ready();
587+
expect(h.service().options.transport).toBeInstanceOf(LogTransport);
588+
589+
const result = await h.settings.action('test')!({
590+
values: { provider: 'log', from_email: 'no-reply@example.test' },
591+
payload: { to: 'ops@example.test' },
592+
});
593+
594+
expect(result).toMatchObject({ ok: false, severity: 'warning' });
595+
expect(h.transport.send).not.toHaveBeenCalled();
596+
expect(h.jobs()).toHaveLength(0);
597+
expect(last(h.sysEmail())).toMatchObject({ status: 'sent' });
598+
});
554599
});
555600

556601
beforeEach(() => { vi.clearAllMocks(); });

‎packages/plugins/plugin-email/src/email-plugin.ts‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -718,7 +718,28 @@ export class EmailServicePlugin implements Plugin {
718718
} catch (err: any) {
719719
return { ok: false, severity: 'error', message: `Failed to build SMTP transport: ${err?.message ?? String(err)}` };
720720
}
721-
} else if (provider !== 'log') {
721+
} else if (provider === 'log') {
722+
// "None (log only — no real delivery)" on the form tests the
723+
// form's choice, as every other provider does: a one-shot
724+
// LogTransport, never the live service. Testing `svc` here
725+
// delivered a real mail over a live SMTP while the form said
726+
// nothing would be. A live LogTransport already IS the form's
727+
// choice, so that case keeps the saved-`log` answer below.
728+
if (!(svc.options.transport instanceof LogTransport)) {
729+
target = new EmailService({
730+
transport: new LogTransport(ctx.logger),
731+
defaultFrom: merged.from_email
732+
? {
733+
address: String(merged.from_email),
734+
name: merged.from_name ? String(merged.from_name) : undefined,
735+
}
736+
: undefined,
737+
// Same sys_email audit trail as the smtp test above.
738+
...(svc.options.persistence ? { persistence: svc.options.persistence } : {}),
739+
logger: ctx.logger,
740+
});
741+
}
742+
} else {
722743
// A provider with no transport behind it — a value stored while
723744
// the settings page still offered SendGrid / Amazon SES (#5094).
724745
// Refuse before asking for an API key: nothing here can use one.
@@ -790,6 +811,18 @@ export class EmailServicePlugin implements Plugin {
790811
+ 'Configure an SMTP host (or an API provider) and save before testing.',
791812
};
792813
}
814+
// The form's unsaved "None (log only)": the same severity and
815+
// the same words for what happened, plus the live transport the
816+
// test did not use and what saving the form will do.
817+
if (target !== svc && target.options.transport instanceof LogTransport) {
818+
return {
819+
ok: false,
820+
severity: 'warning',
821+
message: 'Nothing was delivered — the provider on the form is "None (log only)", so the test message '
822+
+ 'was only logged and recorded in sys_email; no mail left this machine. The live transport was '
823+
+ `not used (${describeLiveMailTransport(svc).label}); save the form to stop delivery.`,
824+
};
825+
}
793826
return {
794827
ok: true,
795828
severity: 'info',

‎packages/plugins/plugin-email/src/mail-log-provider.pin.test.ts‎

Lines changed: 52 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,13 @@ function fakeSettings(values: Record<string, Resolved>) {
4444
},
4545
/** What the page states, as the settings GET route asks for it (waits for a running re-apply). */
4646
statement: () => liveStatus!(),
47-
/** The "Send test email" answer for the stored values, with nothing edited on the form. */
48-
sendTest: () => actions.get('mail/test')!({
47+
/**
48+
* The "Send test email" answer for the stored values — with nothing edited
49+
* on the form, or with `form` as the unsaved form state the page posts.
50+
*/
51+
sendTest: (form?: Record<string, unknown>) => actions.get('mail/test')!({
4952
values: Object.fromEntries(Object.entries(values).map(([k, v]) => [k, v.value])),
50-
payload: { to: 'admin@example.test' },
53+
payload: form ? { to: 'admin@example.test', values: form } : { to: 'admin@example.test' },
5154
}),
5255
};
5356
}
@@ -177,3 +180,49 @@ describe('CONTROL — saving provider "log" after a boot with no SMTP', () => {
177180
expect(nm.sendMail).not.toHaveBeenCalled();
178181
});
179182
});
183+
184+
// "Send test email" tests the FORM's choice, saved or not. With "None (log
185+
// only)" picked and not saved, the test used to go through the live service —
186+
// a real mail over a live SMTP, answered `ok: true`.
187+
describe('"Send test email" with provider "log" on the form, not saved, while SMTP is live', () => {
188+
const SMTP_SAVED = {
189+
provider: { value: 'smtp', source: 'global' },
190+
smtp_host: { value: 'smtp.saved.test', source: 'global' },
191+
};
192+
193+
it('sends nothing, and the answer says nothing was delivered', async () => {
194+
const h = await boot(SMTP_SAVED);
195+
expect(h.service.options.transport).toBeInstanceOf(SmtpTransport);
196+
nm.sendMail.mockClear();
197+
198+
const answer = await h.settings.sendTest({ provider: 'log' });
199+
expect(nm.sendMail).not.toHaveBeenCalled();
200+
expect(answer).toMatchObject({ ok: false, severity: 'warning' });
201+
expect(answer.message).toMatch(/^Nothing was delivered/);
202+
expect(answer.message).toMatch(/no mail left this machine/);
203+
expect(answer.message).toMatch(/Mail is delivered through SMTP \(smtp\.saved\.test:587\)/);
204+
// A test changes nothing: the live transport is still the saved SMTP.
205+
expect(h.service.options.transport).toBeInstanceOf(SmtpTransport);
206+
});
207+
208+
it('SMTP built at boot from the boot options: sends nothing either', async () => {
209+
const h = await boot({}, { provider: 'smtp', providerOptions: { host: 'smtp.env.test', port: 2525 } });
210+
expect(h.service.options.transport).toBeInstanceOf(SmtpTransport);
211+
nm.sendMail.mockClear();
212+
213+
const answer = await h.settings.sendTest({ provider: 'log' });
214+
expect(nm.sendMail).not.toHaveBeenCalled();
215+
expect(answer).toMatchObject({ ok: false, severity: 'warning' });
216+
expect(answer.message).toMatch(/^Nothing was delivered/);
217+
});
218+
219+
it('CONTROL — provider "smtp" with a host on the form still sends', async () => {
220+
const h = await boot(SMTP_SAVED);
221+
nm.sendMail.mockClear();
222+
223+
const answer = await h.settings.sendTest({ provider: 'smtp', smtp_host: 'smtp.form.test', smtp_port: 587 });
224+
expect(nm.sendMail).toHaveBeenCalledTimes(1);
225+
expect(answer).toMatchObject({ ok: true, severity: 'info' });
226+
expect(answer.message).toMatch(/^Sent test email to admin@example\.test via smtp \(smtp\.form\.test:587\)/);
227+
});
228+
});

0 commit comments

Comments
 (0)