From 646e530eea13e0a3d5b57ab86b76f3d7baf6eeae Mon Sep 17 00:00:00 2001 From: "Reichenbach, Michael" <755327+Silthus@users.noreply.github.com> Date: Tue, 27 Oct 2026 08:49:06 +0000 Subject: [PATCH] fix(workflows): report a skipped test email as skipped 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 --- .../Broadcasts/broadcastTestSend.test.ts | 28 ------------- .../Broadcasts/broadcastTestSendLogic.ts | 16 +------- .../TemplateLibrary/SendTestEmailModal.tsx | 8 ++-- .../messageTemplateTestSendLogic.test.ts | 29 +++++++++---- .../messageTemplateTestSendLogic.ts | 18 ++++++-- .../HogFlowEditorNotificationPanelTest.tsx | 17 +------- .../panel/testing/HogFlowEditorPanelTest.tsx | 17 +------- .../panel/testing/testResultBanner.test.ts | 41 +++++++++++++++++++ .../panel/testing/testResultBanner.ts | 21 ++++++++++ .../hogflows/steps/testSendSkipReason.test.ts | 24 +++++++++++ .../hogflows/steps/testSendSkipReason.ts | 10 +++++ 11 files changed, 141 insertions(+), 88 deletions(-) delete mode 100644 products/workflows/frontend/Broadcasts/broadcastTestSend.test.ts create mode 100644 products/workflows/frontend/Workflows/hogflows/panel/testing/testResultBanner.test.ts create mode 100644 products/workflows/frontend/Workflows/hogflows/panel/testing/testResultBanner.ts create mode 100644 products/workflows/frontend/Workflows/hogflows/steps/testSendSkipReason.test.ts create mode 100644 products/workflows/frontend/Workflows/hogflows/steps/testSendSkipReason.ts diff --git a/products/workflows/frontend/Broadcasts/broadcastTestSend.test.ts b/products/workflows/frontend/Broadcasts/broadcastTestSend.test.ts deleted file mode 100644 index 953830604fab..000000000000 --- a/products/workflows/frontend/Broadcasts/broadcastTestSend.test.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { HogflowTestResult } from '../Workflows/hogflows/steps/types' -import { findTestSendSkipReason } from './broadcastTestSendLogic' - -const result = (partial: Partial): HogflowTestResult => - ({ status: 'success', nextActionId: null, ...partial }) as HogflowTestResult - -const log = (message: string): any => ({ level: 'info', timestamp: '2026-01-01T00:00:00Z', message }) - -describe('findTestSendSkipReason', () => { - it('finds the reason a declined send reports, even though the run says success', () => { - // The worker declines the send but finishes the step, so status alone would read as sent. - const reason = - 'Skipping send: the domain "example.com" has no reachable mail servers, so this message would hard bounce.' - expect( - findTestSendSkipReason( - result({ status: 'success', logs: [log('Executing action [Action:send_test_email]'), log(reason)] }) - ) - ).toBe(reason) - }) - - it.each([ - ['a delivered send', result({ status: 'success', logs: [log('Executing action [Action:send_test_email]')] })], - ['a run with no logs', result({ status: 'success' })], - ['no result at all', null], - ])('reports no reason for %s', (_name, input) => { - expect(findTestSendSkipReason(input)).toBeUndefined() - }) -}) diff --git a/products/workflows/frontend/Broadcasts/broadcastTestSendLogic.ts b/products/workflows/frontend/Broadcasts/broadcastTestSendLogic.ts index d16e80655dbc..dcb26179306a 100644 --- a/products/workflows/frontend/Broadcasts/broadcastTestSendLogic.ts +++ b/products/workflows/frontend/Broadcasts/broadcastTestSendLogic.ts @@ -11,6 +11,7 @@ import { userLogic } from 'scenes/userLogic' import { UserType } from '~/types' import type { HogFlowApi } from '../generated/api.schemas' +import { findTestSendSkipReason } from '../Workflows/hogflows/steps/testSendSkipReason' import { HogflowTestResult } from '../Workflows/hogflows/steps/types' import { createExampleEvent } from '../Workflows/hogflows/testEventFactory' import type { HogFlow, HogFlowAction, HogFlowEdge } from '../Workflows/hogflows/types' @@ -27,21 +28,6 @@ import { // and it goes over as `current_action_id` so the worker runs just this node. const TEST_EMAIL_ACTION_ID = 'send_test_email' -// Every reason the email worker declines a send is logged behind this prefix -// (nodejs/src/cdp/services/messaging/email-validation.service.ts). -const SKIP_LOG_PREFIX = 'Skipping send:' - -/** - * The reason the worker declined to send, if it did. - * - * A declined send still finishes the step cleanly, so the invocation comes back `success` and the - * reason appears only in the logs. Reading the status alone reports a test as delivered when - * nothing was sent, and the sender waits for mail that never arrives. - */ -export function findTestSendSkipReason(result: HogflowTestResult | null): string | undefined { - return result?.logs?.find((log) => log.message?.includes(SKIP_LOG_PREFIX))?.message -} - // Generated by kea-typegen. Update if you're an agent, ignore if you're human. export interface broadcastTestSendLogicValues { previewPerson: BroadcastPreviewPerson | null // broadcastPreviewLogic diff --git a/products/workflows/frontend/TemplateLibrary/SendTestEmailModal.tsx b/products/workflows/frontend/TemplateLibrary/SendTestEmailModal.tsx index 22671911dbed..398cc2ca64aa 100644 --- a/products/workflows/frontend/TemplateLibrary/SendTestEmailModal.tsx +++ b/products/workflows/frontend/TemplateLibrary/SendTestEmailModal.tsx @@ -20,6 +20,7 @@ export function SendTestEmailModal(props: MessageTemplateLogicProps & { isOpen: sendDisabledReason, testSendResult, testSendResultLoading, + testSendSkipReason, } = useValues(logic) const { setModalOpen, setRecipientEmail, setSenderIntegrationId, sendTestEmail } = useActions(logic) @@ -86,11 +87,8 @@ export function SendTestEmailModal(props: MessageTemplateLogicProps & { isOpen: )} - {testSendResult && testSendResult.status === 'skipped' ? ( - - {testSendResult.logs?.map((log) => log.message).join(' ') || - 'The send was skipped for this recipient.'} - + {testSendSkipReason ? ( + {testSendSkipReason} ) : testSendResult && testSendResult.status === 'error' ? ( {testSendResult.errors?.join(', ') || 'Failed to send test email'} diff --git a/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.test.ts b/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.test.ts index 81a4245849aa..6c9b439febd1 100644 --- a/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.test.ts +++ b/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.test.ts @@ -124,27 +124,42 @@ describe('messageTemplateTestSendLogic', () => { expect(logic.values.isModalOpen).toBe(false) }) - it('treats a skipped send as a distinct outcome from a failure', async () => { + it.each([ + [ + 'the worker declines the recipient', + { + status: 'success', + emailSendResult: { + status: 'skipped', + reason: 'Skipping send: the domain "example.com" has no reachable mail servers, so this message would hard bounce.', + }, + }, + 'Skipping send: the domain "example.com" has no reachable mail servers, so this message would hard bounce.', + ], + [ + 'the workflow skips the step', + { status: 'skipped', logs: [{ message: 'Recipient has opted out.' }] }, + 'Recipient has opted out.', + ], + ])('reports a skipped send as skipped when %s', async (_name, response, expectedReason) => { useMocks({ post: { - '/api/environments/:team_id/hog_flows/:id/invocations/': () => [ - 200, - { status: 'skipped', logs: [{ message: 'Recipient has opted out.' }] }, - ], + '/api/environments/:team_id/hog_flows/:id/invocations/': () => [200, response], }, }) logic.actions.setModalOpen(true) - logic.actions.setRecipientEmail('recipient@example.com') + logic.actions.setRecipientEmail('someone@example.com') await expectLogic(logic, () => { logic.actions.sendTestEmail() }).toDispatchActions(['sendTestEmailSuccess']) expect(mockToast.warning).toHaveBeenCalledWith('Test email skipped, see details below') + expect(mockToast.success).not.toHaveBeenCalled() expect(mockToast.error).not.toHaveBeenCalled() expect(logic.values.isModalOpen).toBe(true) - expect(logic.values.testSendResult?.status).toBe('skipped') + expect(logic.values.testSendSkipReason).toBe(expectedReason) }) it('keeps the modal open and surfaces the error when the send fails', async () => { diff --git a/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.ts b/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.ts index dc1d9315665d..ed4c4bd2824d 100644 --- a/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.ts +++ b/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.ts @@ -14,6 +14,7 @@ import { senderFrom, senderIntegrationFor, } from '../../../../frontend/src/scenes/hog-functions/email-templater/emailSenderFields' +import { findTestSendSkipReason } from '../Workflows/hogflows/steps/testSendSkipReason' import { HogflowTestResult } from '../Workflows/hogflows/steps/types' import { createExampleEvent } from '../Workflows/hogflows/testEventFactory' import type { HogFlow, HogFlowAction, HogFlowEdge } from '../Workflows/hogflows/types' @@ -40,6 +41,7 @@ export interface messageTemplateTestSendLogicValues { senderIntegrationIdOverride: number | null testSendResult: HogflowTestResult | null testSendResultLoading: boolean + testSendSkipReason: string | undefined } // Generated by kea-typegen. Update if you're an agent, ignore if you're human. @@ -106,6 +108,7 @@ export interface messageTemplateTestSendLogicMeta { recipientEmail: string, senderIntegrationId: number | null ) => string | undefined + testSendSkipReason: (testSendResult: HogflowTestResult | null) => string | undefined } } @@ -226,6 +229,15 @@ export const messageTemplateTestSendLogic = kea [s.testSendResult], + (testSendResult: HogflowTestResult | null): string | undefined => + findTestSendSkipReason(testSendResult) ?? + (testSendResult?.status === 'skipped' + ? testSendResult.logs?.map((log) => log.message).join(' ') || + 'The send was skipped for this recipient.' + : undefined), + ], sendDisabledReason: [ (s) => [s.template, s.recipientEmail, s.senderIntegrationId], ( @@ -260,12 +272,12 @@ export const messageTemplateTestSendLogic = kea { - if (testSendResult.status === 'success') { + if (values.testSendSkipReason) { + lemonToast.warning('Test email skipped, see details below') + } else if (testSendResult.status === 'success') { lemonToast.success(`Test email sent to ${values.recipientEmail}`) actions.setModalOpen(false) actions.clearTestSendResult() - } else if (testSendResult.status === 'skipped') { - lemonToast.warning('Test email skipped, see details below') } else { lemonToast.error('Failed to send test email') } diff --git a/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorNotificationPanelTest.tsx b/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorNotificationPanelTest.tsx index 6c5c34ceda8e..ac458aa1b584 100644 --- a/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorNotificationPanelTest.tsx +++ b/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorNotificationPanelTest.tsx @@ -31,6 +31,7 @@ import { TRIGGER_NODE_ID, workflowLogic } from '../../../workflowLogic' import { hogFlowEditorLogic } from '../../hogFlowEditorLogic' import { hogFlowEditorNotificationTestLogic } from './hogFlowEditorNotificationTestLogic' import { reorderGlobalsForEmailAction } from './hogFlowEditorNotificationTestLogic' +import { testResultBanner } from './testResultBanner' export function EmailActionTestContent(): JSX.Element | null { const { workflow, selectedNode } = useValues(hogFlowEditorLogic) @@ -359,21 +360,7 @@ export function EmailActionTestContent(): JSX.Element | null {
No tests run yet
) : ( <> - - {testResult.status === 'success' - ? 'Success' - : testResult.status === 'skipped' - ? 'Workflow was skipped because the event did not match the filter criteria' - : 'Error: ' + testResult.errors?.join(', ')} - +
Logs diff --git a/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorPanelTest.tsx b/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorPanelTest.tsx index 923aeaf0cd93..4d7081afc40b 100644 --- a/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorPanelTest.tsx +++ b/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorPanelTest.tsx @@ -13,6 +13,7 @@ import { TRIGGER_NODE_ID, workflowLogic } from '../../../workflowLogic' import { hogFlowEditorLogic } from '../../hogFlowEditorLogic' import { HogFlowEditorTestEvent } from './HogFlowEditorTestEvent' import { hogFlowEditorTestLogic } from './hogFlowEditorTestLogic' +import { testResultBanner } from './testResultBanner' export function HogFlowTestPanelNonSelected(): JSX.Element { return ( @@ -148,21 +149,7 @@ export function HogFlowEditorPanelTest(): JSX.Element | null {
No tests run yet
) : ( <> - - {testResult.status === 'success' - ? 'Success' - : testResult.status === 'skipped' - ? 'Workflow was skipped because the event did not match the filter criteria' - : 'Error: ' + testResult.errors?.join(', ')} - +
Logs diff --git a/products/workflows/frontend/Workflows/hogflows/panel/testing/testResultBanner.test.ts b/products/workflows/frontend/Workflows/hogflows/panel/testing/testResultBanner.test.ts new file mode 100644 index 000000000000..cd3a6c4e21f6 --- /dev/null +++ b/products/workflows/frontend/Workflows/hogflows/panel/testing/testResultBanner.test.ts @@ -0,0 +1,41 @@ +import { HogflowTestResult } from '../../steps/types' +import { testResultBanner } from './testResultBanner' + +const result = (partial: Partial): HogflowTestResult => + ({ status: 'success', nextActionId: null, ...partial }) as HogflowTestResult + +describe('testResultBanner', () => { + it.each([ + [ + 'a delivered email', + result({ emailSendResult: { status: 'accepted' } }), + { type: 'success', children: 'Success' }, + ], + [ + 'a declined email', + result({ + emailSendResult: { + status: 'skipped', + reason: 'Skipping send: the domain "example.com" has no reachable mail servers, so this message would hard bounce.', + }, + }), + { + type: 'warning', + children: + 'Skipping send: the domain "example.com" has no reachable mail servers, so this message would hard bounce.', + }, + ], + [ + 'a run the filters skipped', + result({ status: 'skipped' }), + { type: 'warning', children: 'Workflow was skipped because the event did not match the filter criteria' }, + ], + [ + 'a failed run', + result({ status: 'error', errors: ['Template not found', 'Missing input'] }), + { type: 'error', children: 'Error: Template not found, Missing input' }, + ], + ])('describes %s', (_name, input, expected) => { + expect(testResultBanner(input)).toEqual(expected) + }) +}) diff --git a/products/workflows/frontend/Workflows/hogflows/panel/testing/testResultBanner.ts b/products/workflows/frontend/Workflows/hogflows/panel/testing/testResultBanner.ts new file mode 100644 index 000000000000..193d8887b23e --- /dev/null +++ b/products/workflows/frontend/Workflows/hogflows/panel/testing/testResultBanner.ts @@ -0,0 +1,21 @@ +import { findTestSendSkipReason } from '../../steps/testSendSkipReason' +import { HogflowTestResult } from '../../steps/types' + +export interface TestResultBannerProps { + type: 'success' | 'warning' | 'error' + children: string +} + +export function testResultBanner(testResult: HogflowTestResult): TestResultBannerProps { + const skipReason = findTestSendSkipReason(testResult) + if (skipReason) { + return { type: 'warning', children: skipReason } + } + if (testResult.status === 'skipped') { + return { type: 'warning', children: 'Workflow was skipped because the event did not match the filter criteria' } + } + if (testResult.status === 'error') { + return { type: 'error', children: 'Error: ' + testResult.errors?.join(', ') } + } + return { type: 'success', children: 'Success' } +} diff --git a/products/workflows/frontend/Workflows/hogflows/steps/testSendSkipReason.test.ts b/products/workflows/frontend/Workflows/hogflows/steps/testSendSkipReason.test.ts new file mode 100644 index 000000000000..8629846bf860 --- /dev/null +++ b/products/workflows/frontend/Workflows/hogflows/steps/testSendSkipReason.test.ts @@ -0,0 +1,24 @@ +import { findTestSendSkipReason } from './testSendSkipReason' +import { HogflowTestResult } from './types' + +const result = (partial: Partial): HogflowTestResult => + ({ status: 'success', nextActionId: null, ...partial }) as HogflowTestResult + +describe('findTestSendSkipReason', () => { + it.each([ + 'Skipping send: the domain "example.com" has no reachable mail servers, so this message would hard bounce.', + 'Recipient has opted out, skipping message delivery.', + ])('finds the reason a declined send reports, even though the run says success: %s', (reason) => { + expect( + findTestSendSkipReason(result({ status: 'success', emailSendResult: { status: 'skipped', reason } })) + ).toBe(reason) + }) + + it.each([ + ['a delivered send', result({ status: 'success', emailSendResult: { status: 'accepted' } })], + ['a run without an email send', result({ status: 'success' })], + ['no result at all', null], + ])('reports no reason for %s', (_name, input) => { + expect(findTestSendSkipReason(input)).toBeUndefined() + }) +}) diff --git a/products/workflows/frontend/Workflows/hogflows/steps/testSendSkipReason.ts b/products/workflows/frontend/Workflows/hogflows/steps/testSendSkipReason.ts new file mode 100644 index 000000000000..4e91161d8dca --- /dev/null +++ b/products/workflows/frontend/Workflows/hogflows/steps/testSendSkipReason.ts @@ -0,0 +1,10 @@ +import { HogflowTestResult } from './types' + +// A declined send still finishes the step, so the run status reads `success` when nothing was sent. +export function findTestSendSkipReason(result: HogflowTestResult | null): string | undefined { + const emailSendResult = result?.emailSendResult + if (emailSendResult?.status !== 'skipped') { + return undefined + } + return emailSendResult.reason || 'Nothing was sent to this recipient.' +}