diff --git a/e2e/tests/pipeline-approve.spec.js b/e2e/tests/pipeline-approve.spec.js index dee4131..2642215 100644 --- a/e2e/tests/pipeline-approve.spec.js +++ b/e2e/tests/pipeline-approve.spec.js @@ -12,16 +12,16 @@ * → enqueues email.send on the in-process queue * the email.send worker (server/email-jobs.js) finds no mail provider * (RESEND_API_KEY / SMTP_* / Gmail OAuth all blank, no mailbox_accounts row) - * and takes its documented dev fallback at email-jobs.js:154-160: + * and takes its dev fallback: it skips the provider call and nothing else. * → contacts.status: 'pending' → 'sent', sent_at set, send_attempts = 1 - * → returns { dryRun: true } *before* the provider-success block, so - * pipeline_jobs.stage stays 'send' (never reaches 'monitor'), - * email_sent_at / smtp_message_id stay NULL, and no email_replies or - * email_events rows are written. + * → pipeline_jobs.stage: 'send' → 'monitor', email_sent_at set + * → an outbound email_replies row and a 'sent' email_event (labelled + * dryRun) are written; provider ids stay NULL because nothing was sent. * - * That last point is a real quirk of the no-provider path, so it is asserted - * explicitly rather than papered over — if the dry-run branch ever learns to - * sync the pipeline row, this spec fails loudly and should be updated. + * This spec previously asserted the opposite and said it should be updated if + * the dry-run branch ever learned to sync the pipeline row. It has: the early + * return used to strand every approved job at 'send' on any provider-less + * deployment — which is what CI is, and what a fresh self-host is. */ const { test, expect } = require('@playwright/test'); @@ -107,14 +107,14 @@ test('approving a review-stage job moves it out of review and marks the contact expect(job.email_approved).toBe(1); expect(job.email_to).toBe(fixture.emailTo); expect(job.error).toBeNull(); - // Canary for the dry-run early-return described in the file header: with no - // mail provider the worker never reaches the pipeline_jobs sync, so the job - // stops at 'send' instead of advancing to 'monitor'. - expect(job.stage).toBe('send'); - expect(job.email_sent_at).toBeNull(); + // The no-provider path runs the same bookkeeping as a real send, minus the + // send: the job advances out of 'send', and the thread row and event exist + // so the UI isn't blank. Provider ids stay NULL because nothing was sent. + expect(job.stage).toBe('monitor'); + expect(job.email_sent_at).toBeTruthy(); expect(job.smtp_message_id).toBeNull(); - expect(countRows('email_replies', 'contact_id = ?', [fixture.contactId])).toBe(0); - expect(countRows('email_events', 'contact_id = ?', [fixture.contactId])).toBe(0); + expect(countRows('email_replies', 'contact_id = ?', [fixture.contactId])).toBe(1); + expect(countRows('email_events', 'contact_id = ?', [fixture.contactId])).toBe(1); }); test('approve is stage-gated: the action disappears and a second approve is rejected', async ({ page }) => { @@ -137,5 +137,8 @@ test('approve is stage-gated: the action disappears and a second approve is reje const second = await approveOnce(); expect(second.status()).toBe(400); - expect((await second.json()).error).toBe('Job is in stage "send", not "review"'); + // Match the gate, not the stage the job happens to have moved on to — the + // worker races this assertion ('send' while queued, 'monitor' once the + // handler finishes), and which side wins is timing, not behavior. + expect((await second.json()).error).toMatch(/^Job is in stage "(send|monitor)", not "review"$/); }); diff --git a/server/__tests__/pipeline-send-integration.test.js b/server/__tests__/pipeline-send-integration.test.js index 5f09fd1..0a77cf7 100644 --- a/server/__tests__/pipeline-send-integration.test.js +++ b/server/__tests__/pipeline-send-integration.test.js @@ -336,10 +336,13 @@ test('approve → send: contact reaches sent and the pipeline job advances to mo assert.equal(queue.getStats().failed, 0); }); -test('no mail provider configured: the dry-run branch marks the contact sent but leaves the job at send', async () => { - // Pins the behavior the Playwright suite observes in CI, where no - // RESEND_API_KEY exists: email-jobs.js:154-160 returns { dryRun: true } - // *before* the pipeline_jobs sync, so the job never reaches 'monitor'. +test('no mail provider configured: dry-run completes the whole flow without calling out', async () => { + // This test used to pin the opposite: the dry-run branch returned early, + // *before* the pipeline_jobs sync, so a provider-less deployment (which is + // what CI is, and what a fresh self-host is) left every approved job + // stranded at stage='send' — the contact said "sent" while the Pipeline + // page showed it stuck mid-flight forever. The branch now skips only the + // provider call; all bookkeeping below it runs. const f = await driveToReview('dryrun'); const mailAgent = makeMailAgent({ configured: false }); const queue = makeQueue(mailAgent); @@ -350,16 +353,29 @@ test('no mail provider configured: the dry-run branch marks the contact sent but const contact = await getContact(contactId); assert.equal(contact.status, 'sent'); assert.ok(contact.sent_at); - assert.equal(contact.provider_message_id, null); + assert.equal(contact.provider_message_id, null, 'nothing was sent, so there is no provider id'); const job = await getJob(f.pipelineJobId); - assert.equal(job.stage, 'send'); - assert.equal(job.email_sent_at, null); + assert.equal(job.stage, 'monitor', 'the job must not be stranded at send'); + assert.ok(job.email_sent_at, 'pipeline row records when the flow completed'); assert.equal(job.smtp_message_id, null); assert.equal(mailAgent.sent.length, 0, 'no provider call may happen in dry-run mode'); - const events = await query('SELECT event_type FROM email_events WHERE contact_id = ?', [contactId]); - assert.equal(events.rows.length, 0); + + // The thread row and the event exist so the UI isn't blank, and the event + // is labelled so nobody mistakes a dry run for a real delivery. + const thread = await query( + "SELECT direction, to_email FROM email_replies WHERE contact_id = ?", [contactId] + ); + assert.equal(thread.rows.length, 1); + assert.equal(thread.rows[0].direction, 'outbound'); + + const events = await query( + 'SELECT event_type, payload FROM email_events WHERE contact_id = ?', [contactId] + ); + assert.equal(events.rows.length, 1); + assert.equal(events.rows[0].event_type, 'sent'); + assert.equal(JSON.parse(events.rows[0].payload || '{}').dryRun, true); }); test('terminal provider failure: contact fails and the pipeline job bounces back to review', async () => { diff --git a/server/email-jobs.js b/server/email-jobs.js index 52d2d0f..e413b58 100644 --- a/server/email-jobs.js +++ b/server/email-jobs.js @@ -150,13 +150,22 @@ function register({ jobQueue, query, queryOne, exec, mailAgent }) { const mailbox = await resolveMailbox(contact); - // Dev fallback: if no provider config, mark sent (keeps existing behavior). - if (!mailAgent.isConfigured() && !mailbox) { - await exec( - `UPDATE contacts SET status='sent', sent_at=CURRENT_TIMESTAMP, send_error=NULL, last_send_attempt_at=CURRENT_TIMESTAMP, send_attempts=COALESCE(send_attempts,0)+1 WHERE id=?`, - [contactId] + // Dev fallback: with no provider configured we don't call out to anyone, + // but everything downstream still runs. This used to `return` early, which + // skipped the pipeline_jobs sync below — so on every provider-less + // deployment an approved job reached contacts.status='sent' while + // pipeline_jobs.stage stayed at 'send' forever, and the Pipeline page + // showed it stuck mid-flight with no thread row and no event. + const dryRun = !mailAgent.isConfigured() && !mailbox; + if (dryRun) { + // Everything below records a fully successful send, so this line is the + // only operator-facing signal that nothing actually went out. Nothing + // fails fast at boot without a mail provider (email.js isConfigured() is + // just a boolean), so this path is reachable in production — where it + // means outreach is silently going nowhere. Warn, don't swallow. + log.warn( + `[email-jobs] dry-run: no mail provider configured (RESEND_API_KEY / SMTP_* / mailbox account all absent) — contact ${contactId} will be marked sent without sending` ); - return { dryRun: true }; } await exec( @@ -164,24 +173,26 @@ function register({ jobQueue, query, queryOne, exec, mailAgent }) { [contactId] ); - const result = await mailAgent.sendEmail({ - to: emailTo, - subject, - body, - mailboxAccount: mailbox, - // Persist refreshed Gmail tokens so the next send uses the new access_token - onCredsRefreshed: async (fresh) => { - if (!mailbox?.id) return; - try { - await exec( - 'UPDATE mailbox_accounts SET credentials_encrypted = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', - [secrets.encrypt(fresh), mailbox.id] - ); - } catch (e) { - log.warn('[email-jobs] failed to persist refreshed creds:', e.message); - } - }, - }); + const result = dryRun + ? { success: true, messageId: null, provider: 'dry-run' } + : await mailAgent.sendEmail({ + to: emailTo, + subject, + body, + mailboxAccount: mailbox, + // Persist refreshed Gmail tokens so the next send uses the new access_token + onCredsRefreshed: async (fresh) => { + if (!mailbox?.id) return; + try { + await exec( + 'UPDATE mailbox_accounts SET credentials_encrypted = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', + [secrets.encrypt(fresh), mailbox.id] + ); + } catch (e) { + log.warn('[email-jobs] failed to persist refreshed creds:', e.message); + } + }, + }); if (!result.success) { // Transient vs terminal distinction: network-ish errors -> throw to retry. @@ -255,10 +266,10 @@ function register({ jobQueue, query, queryOne, exec, mailAgent }) { contactId, providerMessageId: result.messageId, eventType: 'sent', - payload: { provider: result.provider, to: emailTo }, + payload: { provider: result.provider, to: emailTo, dryRun: dryRun || undefined }, }); - return { success: true, messageId: result.messageId }; + return { success: true, messageId: result.messageId, dryRun: dryRun || undefined }; }); // ---- email.batch_send ----