Skip to content

fix(workflows): report a skipped test email as skipped - #123846

Closed
Silthus wants to merge 3 commits into
PostHog:masterfrom
Silthus:fix/test-email-send-status
Closed

Silthus wants to merge 3 commits into
PostHog:masterfrom
Silthus:fix/test-email-send-status

Conversation

@Silthus

@Silthus Silthus commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • A test email to a recipient the email worker declines reports success, so the sender waits for mail that never arrives.
  • Template library Send test toasts "Test email sent" and closes the modal.
  • The workflow email step's test panel shows a green "Success" banner.
  • The worker declines a recipient it predicts will bounce, such as a domain with no mail server (example.com). It finishes the step, so the run status reads success.
  • The invocation response already carries emailSendResult: { status: "skipped", reason }. Both surfaces ignore it.
  • Found during a local verification walk of Workflows.

Note

🔵 Merge risk: low · two-way door (merge-risk rubric)

  • Frontend-only: it changes how three test-send surfaces label a result. Sending and delivery code do not change.
  • Worst case is cosmetic: a test result shows the wrong status. Unit tests cover each outcome, and a revert undoes it.

Changes

  • Template Send test: a declined recipient now shows a warning toast. The modal stays open with the worker's reason.
  • Workflow email step test: a declined send now shows a yellow banner with the reason instead of "Success".
  • Broadcast Send test: no visible change. It already caught this case by matching log text and now reads the structured field. That field also covers an opted-out recipient, which the log match missed.
  • Delivered sends still read as sent on all three surfaces.
  • Mechanical: findTestSendSkipReason moves from the broadcast logic to Workflows/hogflows/steps/. One testResultBanner function replaces the banner ternary that both step test panels duplicated.
Before After
Template Send test master: Test email sent toast for an example.com recipient This branch: warning toast and the reason in the modal
Email step test master: green Success banner above a Skipping send log This branch: yellow banner with the skip reason

How did you test this code?

Ran a local preview of origin/master, then of this branch, with a Maildev email sender (DEBUG only). Each test send went to someone@example.com (no mail server) and to a posthog.com address, which Maildev catches locally.

Surface master this branch
Template Send test, declined "Test email sent" toast, modal closes Warning toast, reason in modal
Email step test, declined Green "Success" Yellow banner with reason
Broadcast Send test, declined not checked Warning toast, reason in modal
Each surface, delivered "Test email sent" or "Success", message in Maildev
  • The template and step walks ran on a template and workflow created on master, and again on ones created on this branch.
  • Lane gate: the workflows frontend Jest suites for TemplateLibrary, Broadcasts, panel/testing and the moved helper pass. typescript:check and oxlint pass.
  • Not run: CI. This PR comes from a fork, and its workflow runs wait for a maintainer to approve them.
  • Review: one cross-family review round (GPT-6.1 Sol) found nothing.

Test rationale:

  • messageTemplateTestSendLogic.test.ts: a new test.each row mocks the real declined response (status: "success" plus a skipped emailSendResult). It fails on master with the success toast. The old row mocked a status: "skipped" shape that the worker does not return for this case.
  • testSendSkipReason.test.ts (moved from broadcastTestSend.test.ts): reads the reason from emailSendResult, including an opt-out reason without the old log prefix.
  • testResultBanner.test.ts: maps delivered, declined, filter-skipped and failed runs to banner type and text. A pure function test, because the panels only render the mapping.

Release status

  • No feature flag controls this change

Docs update

None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5

  • Decisions:
Question Answer Who Where
Fix the email step test panel too? Yes, it showed the same false "Success" 🤖 Agent comment
Read the skip from logs or from emailSendResult? emailSendResult, one shared helper for template, broadcast and step 🤖 Agent comment
  • Involvement: one pre-PR design pass (GPT-6.1 Sol, Design and Anti-Overengineering lenses, no findings). UI walk and live run captured on a local preview.
  • Skills: /writing-tests, /writing-user-facing-copy, /writing-pr-descriptions.
  • No duplicate: an open PR search for test email skip status found none.
  • Public artifact: all test data is invented (example.com, demo project).

🤖 Generated with Claude Code

The email worker declines a recipient it predicts will bounce, such as
a domain with no mail server, and still finishes the step. Template test
sends and the email step's test panel read only the run status, so they
reported a send that never happened as sent.

Read the structured emailSendResult instead, in one helper shared by the
template, broadcast and workflow step test sends.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@trunk-io

trunk-io Bot commented Oct 27, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Silthus added a commit to Silthus/posthog that referenced this pull request Oct 27, 2026
Silthus added a commit to Silthus/posthog that referenced this pull request Oct 27, 2026
Silthus added a commit to Silthus/posthog that referenced this pull request Oct 27, 2026
Silthus added a commit to Silthus/posthog that referenced this pull request Oct 27, 2026
@Silthus

Silthus commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Note

🤖 Claude Opus 5.5 responding on behalf of Michael

Decisions for this PR:

  • 🤖 Fix the email step test panel too? Yes. It showed the same green "Success" for a declined send, so both step test panels now share one banner mapping.
  • 🤖 Read the skip from logs or from emailSendResult? From emailSendResult. Every test-send skip path sets it, including opt-outs that the broadcast's log-prefix match missed. One helper now serves the template, broadcast and step test sends.

Silthus added a commit to Silthus/posthog that referenced this pull request Oct 27, 2026
The frontend reads the skip from emailSendResult (PostHog#123846),
so the endpoint keeps reporting success for a send the worker skipped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd-status

# Conflicts:
#	products/workflows/frontend/Broadcasts/broadcastTestSend.test.ts
#	products/workflows/frontend/Broadcasts/broadcastTestSendLogic.ts
@Silthus

Silthus commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Moved to #127189, a branch in PostHog/posthog, so the full CI can run on it. Closing this fork PR in favor of that one.

@Silthus Silthus closed this Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant