From 9afd0ef864d2f3c3728e2260d2acc89e8ff841a0 Mon Sep 17 00:00:00 2001 From: "Reichenbach, Michael" <755327+Silthus@users.noreply.github.com> Date: Tue, 27 Oct 2026 05:13:58 +0000 Subject: [PATCH] fix(workflows): say why a test send was skipped A test send that the email worker declined, for example to a suppressed recipient or from a paused workflow, came back as a success. The template modal said the email was sent and the editor panes showed "Success". The test invocation endpoint now reports `skipped` when the email step was skipped, and keeps the next step for step-through. The modal and both editor panes show the worker's skip reason. A recipient without an email address now records its reason like the other skips. Refs Silthus/personal#204 Co-Authored-By: Claude Opus 5.5 --- nodejs/src/cdp/cdp-api.serial.test.ts | 8 ++--- nodejs/src/cdp/cdp-api.ts | 3 +- .../services/messaging/email.service.test.ts | 4 +++ .../cdp/services/messaging/email.service.ts | 6 ++-- .../TemplateLibrary/SendTestEmailModal.tsx | 8 ++--- .../messageTemplateTestSendLogic.test.ts | 20 +++++++++-- .../messageTemplateTestSendLogic.ts | 10 ++++++ .../HogFlowEditorNotificationPanelTest.tsx | 7 ++-- .../panel/testing/HogFlowEditorPanelTest.tsx | 7 ++-- .../getHogFlowTestResultMessage.test.ts | 33 +++++++++++++++++++ .../testing/getHogFlowTestResultMessage.ts | 20 +++++++++++ 11 files changed, 101 insertions(+), 25 deletions(-) create mode 100644 products/workflows/frontend/Workflows/hogflows/panel/testing/getHogFlowTestResultMessage.test.ts create mode 100644 products/workflows/frontend/Workflows/hogflows/panel/testing/getHogFlowTestResultMessage.ts diff --git a/nodejs/src/cdp/cdp-api.serial.test.ts b/nodejs/src/cdp/cdp-api.serial.test.ts index 64baeb284314..b9f24aa082ea 100644 --- a/nodejs/src/cdp/cdp-api.serial.test.ts +++ b/nodejs/src/cdp/cdp-api.serial.test.ts @@ -2788,9 +2788,9 @@ describe('CDP API', () => { }) it.each([ - { status: 'accepted' }, - { status: 'skipped', reason: 'The recipient has no reachable mail servers.' }, - ])('returns the inline email outcome %j without routing to the email queue', async (emailSendResult) => { + [{ status: 'accepted' }, 'success'], + [{ status: 'skipped', reason: 'The recipient has no reachable mail servers.' }, 'skipped'], + ])('returns inline email outcome %j as %s, not via the email queue', async (emailSendResult, testStatus) => { emailSpy.mockImplementation((invocation: any) => Promise.resolve({ invocation, @@ -2811,7 +2811,7 @@ describe('CDP API', () => { }) expect(res.status).toBe(200) - expect(res.body.status).toBe('success') + expect(res.body.status).toBe(testStatus) expect(res.body.emailSendResult).toEqual(emailSendResult) expect(res.body.errors).toEqual([]) // EmailService was called inline — proving the test endpoint forced inline delivery diff --git a/nodejs/src/cdp/cdp-api.ts b/nodejs/src/cdp/cdp-api.ts index 649511b14f71..543a37137dfa 100644 --- a/nodejs/src/cdp/cdp-api.ts +++ b/nodejs/src/cdp/cdp-api.ts @@ -934,10 +934,11 @@ export class CdpApi { ) options.isTest = true const result = await this.hogFlowExecutor.executeCurrentAction(invocation, { hogExecutorOptions: options }) + const emailWasSkipped = result.emailSendResult?.status === 'skipped' res.json({ nextActionId: result.skipped ? null : result.invocation.state.currentAction?.id, - status: result.error ? 'error' : result.skipped ? 'skipped' : 'success', + status: result.error ? 'error' : result.skipped || emailWasSkipped ? 'skipped' : 'success', errors: result.error ? [result.error] : [], logs: [...result.logs, ...logs], variables: result.invocation.state.variables ?? {}, diff --git a/nodejs/src/cdp/services/messaging/email.service.test.ts b/nodejs/src/cdp/services/messaging/email.service.test.ts index 7c3cd05338a0..456ffcc500cb 100644 --- a/nodejs/src/cdp/services/messaging/email.service.test.ts +++ b/nodejs/src/cdp/services/messaging/email.service.test.ts @@ -1140,6 +1140,10 @@ describe('EmailService', () => { expect(result.error).toBeUndefined() expect(result.metrics.map((m) => m.metric_name)).toEqual(['email_missing_recipient']) expect(result.skipped).toBe(true) + expect(result.emailSendResult).toEqual({ + status: 'skipped', + reason: expect.stringContaining('the recipient has no email address'), + }) expect(invocation.state.vmState?.stack).toEqual([{ success: false }]) }) diff --git a/nodejs/src/cdp/services/messaging/email.service.ts b/nodejs/src/cdp/services/messaging/email.service.ts index ff83d8746de4..0f6f513a0572 100644 --- a/nodejs/src/cdp/services/messaging/email.service.ts +++ b/nodejs/src/cdp/services/messaging/email.service.ts @@ -492,10 +492,10 @@ export class EmailService { // so record a skip with its own metric instead of an `email_failed`. Otherwise one project // with bad data inflates the platform send failure rate. if (!params.to?.email?.trim()) { - addLog( - 'warn', + const skipReason = 'Skipping send: the recipient has no email address. Add a filter so only people with an email address reach this step.' - ) + addLog('warn', skipReason) + result.emailSendResult = { status: 'skipped', reason: skipReason } if (!isTest) { result.metrics.push({ team_id: invocation.teamId, diff --git a/products/workflows/frontend/TemplateLibrary/SendTestEmailModal.tsx b/products/workflows/frontend/TemplateLibrary/SendTestEmailModal.tsx index 22671911dbed..3bacf279c627 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, + testSendSkipMessage, } = 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.'} - + {testSendSkipMessage ? ( + {testSendSkipMessage} ) : 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..5fa5263083d5 100644 --- a/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.test.ts +++ b/products/workflows/frontend/TemplateLibrary/messageTemplateTestSendLogic.test.ts @@ -129,7 +129,21 @@ describe('messageTemplateTestSendLogic', () => { post: { '/api/environments/:team_id/hog_flows/:id/invocations/': () => [ 200, - { status: 'skipped', logs: [{ message: 'Recipient has opted out.' }] }, + { + status: 'skipped', + nextActionId: 'exit', + emailSendResult: { + status: 'skipped', + reason: 'Skipping send: recipient(s) on the suppression list — recipient@example.com', + }, + logs: [ + { message: '[Action:send_test_email] Executing action' }, + { + message: + '[Action:send_test_email] Skipping send: recipient(s) on the suppression list — recipient@example.com', + }, + ], + }, ], }, }) @@ -144,7 +158,9 @@ describe('messageTemplateTestSendLogic', () => { expect(mockToast.warning).toHaveBeenCalledWith('Test email skipped, see details below') expect(mockToast.error).not.toHaveBeenCalled() expect(logic.values.isModalOpen).toBe(true) - expect(logic.values.testSendResult?.status).toBe('skipped') + expect(logic.values.testSendSkipMessage).toBe( + 'Skipping send: recipient(s) on the suppression list — recipient@example.com' + ) }) 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..0f2086413008 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 { emailSkipReason } from '../Workflows/hogflows/panel/testing/getHogFlowTestResultMessage' 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 + testSendSkipMessage: 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 + testSendSkipMessage: (testSendResult: HogflowTestResult | null) => string | undefined } } @@ -248,6 +251,13 @@ export const messageTemplateTestSendLogic = kea [s.testSendResult], + (testSendResult: HogflowTestResult | null): string | undefined => + testSendResult?.status === 'skipped' + ? (emailSkipReason(testSendResult) ?? 'The send was skipped for this recipient.') + : undefined, + ], }), listeners(({ actions, values }) => ({ setModalOpen: ({ open }) => { diff --git a/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorNotificationPanelTest.tsx b/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorNotificationPanelTest.tsx index 6c5c34ceda8e..c9c13dca1109 100644 --- a/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorNotificationPanelTest.tsx +++ b/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorNotificationPanelTest.tsx @@ -29,6 +29,7 @@ import { asDisplay } from 'products/persons/frontend/person-utils' import { renderWorkflowLogMessage } from '../../../logs/log-utils' import { TRIGGER_NODE_ID, workflowLogic } from '../../../workflowLogic' import { hogFlowEditorLogic } from '../../hogFlowEditorLogic' +import { getHogFlowTestResultMessage } from './getHogFlowTestResultMessage' import { hogFlowEditorNotificationTestLogic } from './hogFlowEditorNotificationTestLogic' import { reorderGlobalsForEmailAction } from './hogFlowEditorNotificationTestLogic' @@ -368,11 +369,7 @@ export function EmailActionTestContent(): JSX.Element | null { : 'error' } > - {testResult.status === 'success' - ? 'Success' - : testResult.status === 'skipped' - ? 'Workflow was skipped because the event did not match the filter criteria' - : 'Error: ' + testResult.errors?.join(', ')} + {getHogFlowTestResultMessage(testResult)}
diff --git a/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorPanelTest.tsx b/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorPanelTest.tsx index 923aeaf0cd93..f53bedb1dd5b 100644 --- a/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorPanelTest.tsx +++ b/products/workflows/frontend/Workflows/hogflows/panel/testing/HogFlowEditorPanelTest.tsx @@ -11,6 +11,7 @@ import { LogsViewerTable } from 'scenes/hog-functions/logs/LogsViewer' import { renderWorkflowLogMessage } from '../../../logs/log-utils' import { TRIGGER_NODE_ID, workflowLogic } from '../../../workflowLogic' import { hogFlowEditorLogic } from '../../hogFlowEditorLogic' +import { getHogFlowTestResultMessage } from './getHogFlowTestResultMessage' import { HogFlowEditorTestEvent } from './HogFlowEditorTestEvent' import { hogFlowEditorTestLogic } from './hogFlowEditorTestLogic' @@ -157,11 +158,7 @@ export function HogFlowEditorPanelTest(): JSX.Element | null { : 'error' } > - {testResult.status === 'success' - ? 'Success' - : testResult.status === 'skipped' - ? 'Workflow was skipped because the event did not match the filter criteria' - : 'Error: ' + testResult.errors?.join(', ')} + {getHogFlowTestResultMessage(testResult)}
diff --git a/products/workflows/frontend/Workflows/hogflows/panel/testing/getHogFlowTestResultMessage.test.ts b/products/workflows/frontend/Workflows/hogflows/panel/testing/getHogFlowTestResultMessage.test.ts new file mode 100644 index 000000000000..4af34636ed1d --- /dev/null +++ b/products/workflows/frontend/Workflows/hogflows/panel/testing/getHogFlowTestResultMessage.test.ts @@ -0,0 +1,33 @@ +import { HogflowTestResult } from '../../steps/types' +import { getHogFlowTestResultMessage } from './getHogFlowTestResultMessage' + +describe('getHogFlowTestResultMessage', () => { + it.each<[string, HogflowTestResult, string]>([ + ['a success', { status: 'success', nextActionId: 'exit' }, 'Success'], + ['an error', { status: 'error', nextActionId: null, errors: ['Invalid input'] }, 'Error: Invalid input'], + [ + 'an email the worker declined to send', + { + status: 'skipped', + nextActionId: 'exit', + emailSendResult: { + status: 'skipped', + reason: 'Skipping send: recipient(s) on the suppression list — person@example.com', + }, + }, + 'Skipping send: recipient(s) on the suppression list — person@example.com', + ], + [ + 'a skipped step without a reason', + { status: 'skipped', nextActionId: 'exit' }, + 'This step was skipped. See the logs for details.', + ], + [ + 'an event that did not match the trigger', + { status: 'skipped', nextActionId: null }, + 'Workflow was skipped because the event did not match the filter criteria', + ], + ])('describes %s', (_label, result, expected) => { + expect(getHogFlowTestResultMessage(result)).toBe(expected) + }) +}) diff --git a/products/workflows/frontend/Workflows/hogflows/panel/testing/getHogFlowTestResultMessage.ts b/products/workflows/frontend/Workflows/hogflows/panel/testing/getHogFlowTestResultMessage.ts new file mode 100644 index 000000000000..a278f33ccb13 --- /dev/null +++ b/products/workflows/frontend/Workflows/hogflows/panel/testing/getHogFlowTestResultMessage.ts @@ -0,0 +1,20 @@ +import { HogflowTestResult } from '../../steps/types' + +export function emailSkipReason(result: HogflowTestResult | null): string | undefined { + return result?.emailSendResult?.status === 'skipped' ? result.emailSendResult.reason : undefined +} + +export function getHogFlowTestResultMessage(result: HogflowTestResult): string { + if (result.status === 'success') { + return 'Success' + } + if (result.status === 'error') { + return 'Error: ' + result.errors?.join(', ') + } + return ( + emailSkipReason(result) ?? + (result.nextActionId + ? 'This step was skipped. See the logs for details.' + : 'Workflow was skipped because the event did not match the filter criteria') + ) +}