From 47defafa09da53f6fbdada94fa5a39c96bd8bfaf Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Sat, 12 Sep 2026 03:13:38 -0700 Subject: [PATCH] feat: fix the six self-host defects reported in issue #439 (#456) * feat: fix the six defects reported in issue #439 by mapping the IMAP UNAVAILABLE, INUSE and NONEXISTENT response codes to retry-level errors instead of a critical reconnect prompt, synthesising a stable no-msgid key so one message with no Message-ID header can no longer 400 the internal map endpoint and wedge every later sync pass with its cursors held, adding mailhtml.FromText and HasContent so an API or agent-created step with a plain body stops shipping the composer's empty div placeholder as its text/html part (derived on create and plain-only update, exposed as body_html on update_campaign_step, dropped at send and preview time, and refused at campaign start with empty_step_body), honouring sender_strategy='explicit' in ResolveCampaignSenderPool and ValidateCampaignReady so an emptied explicit pool parks the campaign instead of widening it to every mailbox in the workspace, making the paused_no_accounts auto-pause loud with an error log line, an error-level activity-feed entry and an org-scoped CAMPAIGN_PAUSED realtime pulse, gating the admin sign-in's Turnstile widget on GET /v1/auth/config so a self-host with CAPTCHA_PROVIDER=none is not locked out, and parsing NATS_URL down to its host:port so a credentialed bus URL no longer reports NATS down * feat: act on the self-review of the issue #439 fixes by dropping the campaign wizard's own escapeHtml body_html builder, which entity-escaped the quotes in a conditional and made the template fail to parse at send time, and letting the backend's FromText render that part instead so wizard-written steps also get their bare URLs linked for click tracking, correcting the docs and openapi description that claimed an explicit sender pool never falls back when it still unions its tags as migration 000013 designed, extracting the duplicated blank-HTML-part guard into dropBlankHTMLPart shared by the send path and the preview, and recording why the no-msgid key keeps the folder name despite a RENAME changing it * feat: address the CodeRabbit review on the issue #439 fixes by holding the admin sign-in's Turnstile widget unmounted until /v1/auth/config resolves so an instance with no route to Cloudflare cannot raise a widget error on a screen nobody submitted, failing StartCampaign closed when the sequence read errors rather than skipping both the malformed-template and empty-body refusals, giving TCPCheck the default port its protocol assumes so a portless NATS_URL is no longer reported down, leaving a URL that carries a merge field unanchored because the send path renders bodies with text/template and a quoted contact value would break out of the href, and correcting the sequences guide and the Campaign and CampaignUpdate openapi descriptions that named the wrong tag field --- admin/src/app/auth/LoginPage.tsx | 33 ++++- .../src/components/captcha/TurnstileModal.tsx | 34 ++++- admin/src/lib/api/client/auth/index.ts | 12 ++ admin/src/lib/api/models/auth.ts | 11 ++ cmd/backend/main.go | 4 +- docs/content/docs/api/error-codes.mdx | 1 + docs/content/docs/api/reference/campaigns.mdx | 8 +- docs/content/docs/guides/campaigns.mdx | 4 + docs/content/docs/guides/sequences.mdx | 4 + docs/public/openapi.json | 3 + internal/app/aitools/tools_sequences.go | 5 +- internal/app/campaign/handlers.go | 41 ++++-- internal/app/sequence/handler.go | 37 +++++ internal/app/sysstatus/sysstatus.go | 54 ++++++- internal/app/sysstatus/sysstatus_test.go | 55 ++++++++ internal/app/worker/wmail/sync_imap.go | 43 +++++- .../app/worker/wmail/sync_imap_key_test.go | 64 +++++++++ internal/client/smtpimap/imap/err.go | 11 ++ internal/client/smtpimap/imap/err_test.go | 33 +++++ internal/pkg/mailhtml/fromtext.go | 132 ++++++++++++++++++ internal/pkg/mailhtml/fromtext_test.go | 108 ++++++++++++++ internal/repository/campaign_senders.go | 38 ++++- internal/repository/campaign_senders_test.go | 112 +++++++++++++++ internal/repository/pg_campaign.go | 40 ++++-- internal/repository/pg_sequence.go | 8 +- internal/tasks/campaign_task.go | 58 +++++++- internal/tasks/preview.go | 23 +++ internal/tasks/preview_test.go | 35 +++++ .../app/campaigns/NewCampaignDialog.tsx | 15 +- .../client/app/campaigns/createCampaign.ts | 5 +- 30 files changed, 973 insertions(+), 58 deletions(-) create mode 100644 internal/app/sysstatus/sysstatus_test.go create mode 100644 internal/app/worker/wmail/sync_imap_key_test.go create mode 100644 internal/pkg/mailhtml/fromtext.go create mode 100644 internal/pkg/mailhtml/fromtext_test.go create mode 100644 internal/repository/campaign_senders_test.go diff --git a/admin/src/app/auth/LoginPage.tsx b/admin/src/app/auth/LoginPage.tsx index b5512a8e..1faadb82 100644 --- a/admin/src/app/auth/LoginPage.tsx +++ b/admin/src/app/auth/LoginPage.tsx @@ -21,7 +21,7 @@ import { Label } from "@/components/ui/label"; import { InputOTP, InputOTPGroup, InputOTPSlot } from "@/components/ui/input-otp"; import { Logo } from "@/components/Logo"; import { TurnstileModal } from "@/components/captcha/TurnstileModal"; -import { login, loginConfirm, verifyTwoFA } from "@/lib/api/client/auth"; +import { getAuthConfig, login, loginConfirm, verifyTwoFA } from "@/lib/api/client/auth"; import type { LoginResponse } from "@/lib/api/models/auth"; import { setToken } from "@/lib/auth/storage"; import { APIError } from "@/lib/api/client"; @@ -51,6 +51,12 @@ export default function LoginPage() { const [session, setSession] = useState(""); const [code, setCode] = useState(""); const [captcha, setCaptcha] = useState(false); + // Whether this deployment verifies a captcha token at all. null until the + // answer arrives: nothing is mounted before then, so an instance with no + // route to Cloudflare does not raise a widget error on a screen nobody has + // submitted. A failed fetch resolves to true rather than leaving it + // pending, so the check is never skipped on an instance that enforces it. + const [captchaRequired, setCaptchaRequired] = useState(null); const [submitting, setSubmitting] = useState(false); const [error, setError] = useState(null); const [resendIn, setResendIn] = useState(0); @@ -59,6 +65,24 @@ export default function LoginPage() { const busy = submitting || captcha; + // What this deployment can do, read before anything mounts a widget. A + // self-host with CAPTCHA_PROVIDER=none cannot reach Cloudflare, so an + // invisible Turnstile there can only time out and lock the operator out. + useEffect(() => { + let cancelled = false; + getAuthConfig() + .then((cfg) => { + if (!cancelled) setCaptchaRequired(cfg.captcha); + }) + .catch(() => { + // Fail safe: assume the check is enforced. + if (!cancelled) setCaptchaRequired(true); + }); + return () => { + cancelled = true; + }; + }, []); + // Resend cooldown tick. useEffect(() => { if (resendIn <= 0) return; @@ -311,7 +335,12 @@ export default function LoginPage() { )} - + ) : ( diff --git a/admin/src/components/captcha/TurnstileModal.tsx b/admin/src/components/captcha/TurnstileModal.tsx index 5c63e1f6..9d1d195f 100644 --- a/admin/src/components/captcha/TurnstileModal.tsx +++ b/admin/src/components/captcha/TurnstileModal.tsx @@ -5,6 +5,20 @@ // the backend's TURNSTILE_BYPASS_TOKEN accepts; in prod it renders the // invisible Turnstile widget and delivers a real token. Either way the parent // gets a token via onToken and sends it as `turnstile` on login. +// +// `required` is the deployment's own answer, read from /v1/auth/config. With +// CAPTCHA_PROVIDER=none the backend verifies no token, and mounting the widget +// anyway meant an air-gapped or self-hosted instance could only sit on +// challenges.cloudflare.com until it timed out: the operator was locked out of +// their own admin panel. +// +// null means the answer has not arrived. Nothing is mounted and no token is +// delivered while it is pending, because mounting optimistically loads the +// Cloudflare script on an instance that may have no route to it, and its load +// failure raises an error on a screen the operator has not even submitted yet. +// A submit made in that window is held by the effect and resolves as soon as +// the config lands. A fetch that fails resolves to true, so the check is never +// skipped on an instance that does enforce it. import { useCallback, useEffect, useRef, type ComponentProps } from "react"; import Turnstile, { type BoundTurnstileObject } from "react-turnstile"; @@ -12,15 +26,22 @@ import { TURNSTILE_KEY } from "@/lib/env"; interface Props { visible: boolean; + required: boolean | null; onToken: (token: string) => void; onError?: (message?: string) => void; } -export function TurnstileModal({ visible, onToken, onError }: Props) { +export function TurnstileModal({ visible, required, onToken, onError }: Props) { const defaultDevBypassToken = "warmbly-local-turnstile-bypass"; - const bypassToken = import.meta.env.DEV + const devBypassToken = import.meta.env.DEV ? import.meta.env.VITE_TURNSTILE_BYPASS_TOKEN?.trim() || defaultDevBypassToken : ""; + // The dev bypass is its own answer and needs no deployment config. + const pending = required === null && devBypassToken === ""; + // No widget, and the token the parent gets is whatever the backend will + // accept: the dev bypass string, or "" when nothing is verified at all. + const skipWidget = required === false || devBypassToken !== ""; + const bypassToken = devBypassToken; const tokenRef = useRef(""); const waitingRef = useRef(false); @@ -77,7 +98,10 @@ export function TurnstileModal({ visible, onToken, onError }: Props) { }, [fail]); useEffect(() => { - if (visible && bypassToken) { + // Hold a submit made before the deployment answered; this effect runs + // again the moment it does, with visible still true. + if (pending) return; + if (visible && skipWidget) { onTokenRef.current(bypassToken); return; } @@ -96,9 +120,9 @@ export function TurnstileModal({ visible, onToken, onError }: Props) { } waitingRef.current = false; } - }, [visible, bypassToken, deliver, execute]); + }, [visible, pending, skipWidget, bypassToken, deliver, execute]); - if (bypassToken) return null; + if (pending || skipWidget) return null; const turnstileProps = { ref: turnstileRef, diff --git a/admin/src/lib/api/client/auth/index.ts b/admin/src/lib/api/client/auth/index.ts index 3a2262c4..0e79befc 100644 --- a/admin/src/lib/api/client/auth/index.ts +++ b/admin/src/lib/api/client/auth/index.ts @@ -4,6 +4,7 @@ import { Request } from "@/lib/api/client"; import type { + DeploymentAuthConfig, LoginRequest, LoginStartResponse, LoginConfirmRequest, @@ -13,6 +14,17 @@ import type { AdminProfile, } from "@/lib/api/models/auth"; +// What this deployment's auth can do. Public and unauthenticated: it is the +// first request the sign-in screen makes, and the answer decides whether a +// Turnstile widget is mounted at all. +export function getAuthConfig(): Promise { + return Request({ + method: "GET", + url: "/v1/auth/config", + timeout: 10_000, + }); +} + // Step 1: verify password + captcha. Emails a one-time code, returns a session. export function login(input: LoginRequest): Promise { return Request({ diff --git a/admin/src/lib/api/models/auth.ts b/admin/src/lib/api/models/auth.ts index 9d7c0151..ec6fc24d 100644 --- a/admin/src/lib/api/models/auth.ts +++ b/admin/src/lib/api/models/auth.ts @@ -2,6 +2,17 @@ import type { AdminToken } from "@/lib/auth/storage"; +// The slice of GET /v1/auth/config the admin sign-in needs. +// +// Only `captcha` today, and it exists because the login screen used to guess. +// A self-host with CAPTCHA_PROVIDER=none verifies no token, but the screen +// mounted the invisible Turnstile widget anyway and waited on a challenge it +// could not reach, so the operator was locked out of their own instance with +// "Verification timed out". +export interface DeploymentAuthConfig { + captcha: boolean; +} + export interface LoginRequest { email: string; password: string; diff --git a/cmd/backend/main.go b/cmd/backend/main.go index 32068d7c..92a5dcb3 100644 --- a/cmd/backend/main.go +++ b/cmd/backend/main.go @@ -1810,9 +1810,9 @@ func main() { systemChecker.Add("redis", func(ctx context.Context) error { return cache.Ping(ctx).Err() }) switch bus.Name() { case "kafka": - systemChecker.Add("kafka", sysstatus.TCPCheck(kafkaBootstrapServers)) + systemChecker.Add("kafka", sysstatus.TCPCheck(kafkaBootstrapServers, "9092")) case "nats": - systemChecker.Add("nats", sysstatus.TCPCheck(strings.TrimPrefix(getenvDefault("NATS_URL", "nats://localhost:4222"), "nats://"))) + systemChecker.Add("nats", sysstatus.TCPCheck(getenvDefault("NATS_URL", "nats://localhost:4222"), "4222")) } if sr := os.Getenv("SCHEMA_REGISTRY_URL"); sr != "" { systemChecker.Add("schema-registry", sysstatus.HTTPCheck(strings.TrimRight(sr, "/")+"/subjects")) diff --git a/docs/content/docs/api/error-codes.mdx b/docs/content/docs/api/error-codes.mdx index 7159b92f..4075eeb2 100644 --- a/docs/content/docs/api/error-codes.mdx +++ b/docs/content/docs/api/error-codes.mdx @@ -84,6 +84,7 @@ Returned when the request cannot be processed due to invalid syntax. | `no_contacts` | `POST /contacts/verification` selected no contacts: neither `contacts` nor a `campaign_id` with refused leads | | `list_bounce_risk` | `POST /campaigns/:id/start` refused the launch on the list's projected bounce rate. Clean or verify the list, or repeat the request with `acknowledge_list_risk: true` | | `leads_undeliverable` | `POST /campaigns/:id/start` found nothing to send because address verification refused every remaining lead; the campaign is parked at `paused_undeliverable` until they are re-verified or marked deliverable | +| `empty_step_body` | `POST /campaigns/:id/start` found an email step with nothing in either body, so it would send a blank message to every lead it reached. Write the step's body and start again | | `no_leads` | `POST /campaigns/:id/start` on a campaign that has never had a lead, with `continuous` off. Add contacts, or set `continuous` so it starts empty and waits for them. A campaign whose leads have all finished is a different case: it starts and waits | | `no_remaining_leads` | A platform-initiated restart of a campaign with nothing left to send and `continuous` off found nothing to do; the campaign is `completed` again. A start you request never answers this: it turns `continuous` on and waits | | `no_organization` | The request needs a workspace and the caller has none selected. Every entitlement, limit and suppression rule is scoped to a workspace, so a write that would run unscoped is refused rather than run without those checks. API keys always carry their workspace; a dashboard session picks one at sign-in, so this normally means the session predates the workspace being chosen. Select a workspace and retry | diff --git a/docs/content/docs/api/reference/campaigns.mdx b/docs/content/docs/api/reference/campaigns.mdx index 4b640d45..d8e2b412 100644 --- a/docs/content/docs/api/reference/campaigns.mdx +++ b/docs/content/docs/api/reference/campaigns.mdx @@ -177,7 +177,7 @@ Create a campaign. Only `name` is required, every other field is optional and ap | `schedule_windows` | array | no | Per-day sending windows, 7 arrays indexed by weekday (Sunday = 0) of `{start, end}` minute-of-day intervals. When non-empty it supersedes `days`, `start_time` and `end_time`. | | `email_tag_ids` | string[] | no | Mailbox tag ids that resolve the sender pool (tags strategy). | | `folder_ids` | string[] | no | Folder ids to file the campaign under. | -| `sender_strategy` | string | no | `tags` (default) or `explicit`. | +| `sender_strategy` | string | no | `tags` (default) or `explicit`. `tags` resolves the pool from `email_tag_ids`, and falls back to every active mailbox in the workspace when no tag and no explicit sender is set. `explicit` sends from the mailboxes in `senders`, plus any `email_tag_ids` set alongside them. It never falls back to every active mailbox, so a pool that empties out parks the campaign at `paused_no_accounts` instead of widening to the whole workspace. | | `rotation_mode` | string | no | How volume spreads across the chosen mailboxes. | | `senders` | object[] | no | Explicit-strategy mailbox pool (see sender input below). | | `ramp_enabled` | boolean | no | Enable per-campaign daily ramp-up. | @@ -732,6 +732,8 @@ Start (activate) the campaign so it begins sending real mail. Works from `draft` A start refused because every remaining lead was refused by address verification answers `leads_undeliverable` and parks the campaign at `paused_undeliverable`; re-verify the leads or mark them deliverable with [`POST /contacts/verification`](/api/reference/contacts/#verify-or-override-contacts), which resumes it. +A campaign whose email step has nothing in either body is refused with `empty_step_body` rather than started, because it would send a blank message to every lead it reached. + ### Response ```json @@ -832,7 +834,7 @@ Atomically replace the campaign's explicit sender pool with the supplied list. * | Field | Type | Required | Description | | --- | --- | --- | --- | -| `senders` | object[] | yes | The full new sender pool. Each item: `email_account_id` (uuid, required), `weight` (integer, optional), `enabled` (boolean, optional). | +| `senders` | object[] | yes | The full new sender pool. Each item: `email_account_id` (uuid, required), `weight` (integer, optional), `enabled` (boolean, optional). An empty array clears the pool. What that means depends on the campaign's `sender_strategy`: a `tags` campaign falls back to its tags or to every active mailbox, while an `explicit` one falls back to its tags only, and with none it is left with no mailbox to send from and parks itself at `paused_no_accounts`. | ```json { @@ -1020,7 +1022,7 @@ All fields optional. | `name` | string | no | Step label. | | `subject` | string | no | Subject template. | | `body_plain` | string | no | Plain-text body template. Leave it empty and the send path renders one from `body_html`, keeping list bullets, table rows and link destinations, and leaving out the stylesheet. | -| `body_html` | string | no | HTML body template. Sent as written, including a whole document with its own ``. Any `", false}, + {"real copy", "
Hi Ana
", true}, + {"image only", `
`, true}, + {"image with no source or alt", "
", false}, + {"rule", "
", true}, + } + for _, c := range cases { + if got := HasContent(c.body); got != c.want { + t.Errorf("%s: HasContent(%q) = %v, want %v", c.name, c.body, got, c.want) + } + } +} + +// The round trip the derivation relies on: whatever FromText writes for a +// non-empty body must read back as content, or the send-time guard would drop +// the part it just derived. +func TestFromTextOutputHasContent(t *testing.T) { + for _, in := range []string{"Hello", "a\n\nb", "https://example.com"} { + if !HasContent(FromText(in)) { + t.Errorf("FromText(%q) produced a body HasContent calls empty", in) + } + } +} + +// The send path renders the body with text/template, which escapes nothing by +// design, so an anchor built around a templated URL would let a contact value +// containing a quote break out of the href. Such a URL stays plain text. +func TestFromTextDoesNotAnchorATemplatedURL(t *testing.T) { + got := FromText("Book here: https://cal.example.com/?ref={{.Company}}") + if strings.Contains(got, " 0 || tagCount > 0 { return nil } + // A campaign that named its mailboxes by hand does NOT fall back to every + // active mailbox (see ExplicitSenderPool), so the check must not either: + // a check that passes a pool the scheduler would find empty is how a + // campaign starts and then parks itself on its first tick (issue #340 is + // the same mistake in the other direction). + var strategy string + if err := r.DB.QueryRow(ctx, `SELECT sender_strategy FROM campaigns WHERE id = $1`, campaignID).Scan(&strategy); err != nil { + return err + } + if strategy == CampaignSenderStrategyExplicit { + return errx.New(errx.BadRequest, + "this campaign sends from mailboxes picked by hand and none are left; pick its sending accounts again, or switch it back to selecting by tag") + } var activeMailboxes int if err := r.DB.QueryRow(ctx, ` SELECT COUNT(*) FROM email_accounts @@ -1979,15 +2001,15 @@ func (r *campaignRepository) GetCampaignSenders(ctx context.Context, campaignID return senders, nil } -// ReplaceCampaignSenders atomically swaps the explicit sender pool. An empty -// list is rejected — clearing senders should be done by switching the campaign -// back to sender_strategy='tags'. +// ReplaceCampaignSenders atomically swaps the explicit sender pool. +// +// An empty list is allowed and clears the pool: it is how the dashboard's +// sending-accounts picker deselects everything. What that then means depends +// on the campaign's sender_strategy, which is the safety boundary — a 'tags' +// campaign falls back to its tags or to every active mailbox, and an +// 'explicit' one resolves to no mailboxes at all rather than widening to the +// whole workspace (see ExplicitSenderPool). func (r *campaignRepository) ReplaceCampaignSenders(ctx context.Context, campaignID uuid.UUID, in []models.CampaignSenderInput) ([]models.CampaignSender, *errx.Error) { - // An empty list is allowed: it clears the explicit sender pool, so the - // campaign falls back to its email tags or, with neither, to every active - // mailbox of the owner. syncCampaignSendersTx handles the empty set safely - // (it deletes all current rows and inserts none). - // Resolve the campaign owner + organization so we can validate mailbox // ownership against the org (the senders route is org-scoped). var userID string diff --git a/internal/repository/pg_sequence.go b/internal/repository/pg_sequence.go index 01b2de71..82534690 100644 --- a/internal/repository/pg_sequence.go +++ b/internal/repository/pg_sequence.go @@ -16,6 +16,12 @@ import ( "github.com/warmbly/warmbly/internal/pkg/encrypt" ) +// emptyBodyHTML is what a step with no body carries: the dashboard composer's +// own empty document, so opening a blank step in the editor shows an empty +// canvas rather than nothing at all. It is NOT an empty string, which is why +// every "does this step have a body" check goes through mailhtml.HasContent. +const emptyBodyHTML = "
" + type SequenceRepository interface { Create(ctx context.Context, userID, campaignID string) (*models.Sequence, *errx.Error) Get(ctx context.Context, userID, campaignID string) ([]models.Sequence, *errx.Error) @@ -194,7 +200,7 @@ func (r *sequenceRepository) Create(ctx context.Context, userID string, campaign config.SequenceDefaultName, "", "", - "
", + emptyBodyHTML, nextPos, } diff --git a/internal/tasks/campaign_task.go b/internal/tasks/campaign_task.go index 345fbbcf..16e85b27 100644 --- a/internal/tasks/campaign_task.go +++ b/internal/tasks/campaign_task.go @@ -567,6 +567,10 @@ func (s *tasksService) HandleCampaignTask(task *proto.ProcessTask) *errx.Error { bodyHTML = "" } + // A blank HTML alternative never ships as the part the client prefers. + // Shared with the preview and the test send (see dropBlankHTMLPart). + bodyHTML = dropBlankHTMLPart(bodyHTML, bodyPlain) + // STEP 10.7: A hand-placed {{.UnsubscribeLink}} resolved to the bare signed // URL; give it an anchor so the recipient reads "Unsubscribe" and not the // API address (issue #341). After the plain part was derived, so plain text @@ -1066,16 +1070,56 @@ func (s *tasksService) clearIdle(ctx context.Context, campaign *models.Campaign) // reason is carried through to the activity log because "paused_no_accounts" // covers several very different fixes (connect a mailbox, widen a sending // window, repair DNS) and the status alone cannot tell them apart. +// +// It is deliberately LOUD. This runs unattended, hours after anyone touched +// the campaign, and a pause nobody is told about is a campaign that quietly +// stops sending: an error-level line in the operator's log, an error-level +// entry in the activity feed so the dashboard tints it red, and an org-scoped +// realtime pulse so every teammate's campaign list moves to "paused — no +// accounts" without a refresh. func (s *tasksService) autoPauseCampaign(ctx context.Context, campaignID, taskID uuid.UUID, reason string) { s.campaignRepo.UpdateStatusWithLock(ctx, campaignID, "paused_no_accounts") - s.taskRepo.UpdateTaskStatus(ctx, taskID, "completed") - if s.campaignLogRepo != nil { - s.campaignLogRepo.CreateLog(ctx, &repository.CampaignLogEntry{ - CampaignID: campaignID, - EventType: "auto_paused", - Message: reason, - }) + if taskID != uuid.Nil { + s.taskRepo.UpdateTaskStatus(ctx, taskID, "completed") } + + log.Error().Str("campaign_id", campaignID.String()).Str("reason", reason). + Msg("campaign auto-paused: no mailbox can send for it") + + // CreateLogOnce, not CreateLog: the reconciler re-checks paused campaigns + // too, and a repeat of the SAME reason must not fill the feed or re-pulse + // the dashboard every pass. Keyed on the reason rather than the code, so a + // pause whose cause changed (DNS, then no mailbox at all) still says so. + // The write also reports whether this pause is news, which is what gates + // the announcement below. + if s.campaignLogRepo == nil { + return + } + entry := &repository.CampaignLogEntry{ + CampaignID: campaignID, + EventType: "auto_paused", + Message: reason, + Metadata: map[string]interface{}{ + "level": "error", + "code": "no_accounts", + "reason": reason, + }, + } + written, err := s.campaignLogRepo.CreateLogOnce(ctx, entry, "reason", reason, time.Now().Add(-time.Hour)) + if err != nil || !written || s.streamingPublisher == nil { + return + } + campaign, gerr := s.campaignRepo.GetByID(ctx, campaignID) + if gerr != nil || campaign == nil { + return + } + s.streamingPublisher.PublishCampaignEvent(ctx, &pubsub.CampaignEvent{ + BaseEvent: pubsub.BaseEvent{EventType: pubsub.EventCampaignPaused, UserID: campaign.UserID}, + OrgID: campaignOrgID(campaign), + CampaignID: campaignID.String(), + Name: campaign.Name, + Status: "paused_no_accounts", + }) } // haltOrglessCampaign stops a campaign that reached the send path with no diff --git a/internal/tasks/preview.go b/internal/tasks/preview.go index 00bea8a8..7dce1df7 100644 --- a/internal/tasks/preview.go +++ b/internal/tasks/preview.go @@ -92,6 +92,26 @@ func (s *tasksService) PreviewEmail(ctx context.Context, orgID uuid.UUID, in Ema return out } +// dropBlankHTMLPart removes an HTML alternative that would render nothing. +// +// An empty HTML part is worse than no HTML part: every modern client prefers +// text/html, so the recipient reads a blank message while the real copy sits +// unread in the text alternative. It catches whatever produced the row, which +// is the composer's
placeholder on an API-created step, but also a +// variant whose HTML-only spintax resolved away. +// +// With no plain part either there is nothing to fall back to, so the body is +// left alone; a campaign in that state is refused at start instead. +func dropBlankHTMLPart(bodyHTML, bodyPlain string) string { + if bodyHTML == "" || strings.TrimSpace(bodyPlain) == "" { + return bodyHTML + } + if mailhtml.HasContent(bodyHTML) { + return bodyHTML + } + return "" +} + // finishBody applies what the send path adds after rendering, in its order: // derive the plain part, drop HTML for a plain-text campaign, turn a // hand-placed unsubscribe link into an anchor, add the mailbox signature, @@ -104,6 +124,9 @@ func finishBody(bodyHTML, bodyPlain string, textOnly bool, account *models.Email if textOnly { bodyHTML = "" } + // Shared with the send path, so the preview and the test send show the + // same message a recipient gets. + bodyHTML = dropBlankHTMLPart(bodyHTML, bodyPlain) // After the plain part is derived, so plain text keeps the URL it needs. linkText := "" if optOut != nil { diff --git a/internal/tasks/preview_test.go b/internal/tasks/preview_test.go index ced6bc70..d8bfe909 100644 --- a/internal/tasks/preview_test.go +++ b/internal/tasks/preview_test.go @@ -53,3 +53,38 @@ func TestPreviewTemplatesWithUsesTheGivenLink(t *testing.T) { t.Fatalf("default preview link changed: %q", q.BodyHTML) } } + +// A step created through the API or an agent tool carries the composer's empty +//
under body_html. It is not an empty string, so it shipped as the +// text/html alternative and the recipient opened a blank message with the real +// copy only in the fallback part. +func TestFinishBodyDropsATextlessHTMLPart(t *testing.T) { + h, p := finishBody("
", "Hi Ana, quick question.", false, nil, nil, "") + if h != "" { + t.Errorf("HTML part = %q, want it dropped so the plain part is what ships", h) + } + if p != "Hi Ana, quick question." { + t.Errorf("plain part = %q, want the step's own text", p) + } +} + +// The guard must not touch a body that renders something, including one whose +// only content is an image. +func TestFinishBodyKeepsABodyThatRenders(t *testing.T) { + if h, _ := finishBody("
Hi
", "Hi", false, nil, nil, ""); h != "
Hi
" { + t.Errorf("HTML part = %q, want it kept", h) + } + img := `
` + if h, _ := finishBody(img, "see the image", false, nil, nil, ""); h == "" { + t.Error("an image-only body was dropped; it renders for the recipient") + } +} + +// With no plain part either there is nothing to fall back to, so dropping the +// HTML would send an empty message. The start-time guard is what stops that +// campaign existing; here the body is left alone. +func TestFinishBodyKeepsAnEmptyHTMLPartWhenThereIsNoPlainText(t *testing.T) { + if h, _ := finishBody("
", "", false, nil, nil, ""); h == "" { + t.Error("dropped the only part the message had") + } +} diff --git a/web/src/components/app/campaigns/NewCampaignDialog.tsx b/web/src/components/app/campaigns/NewCampaignDialog.tsx index 120bf6f9..3db003fa 100644 --- a/web/src/components/app/campaigns/NewCampaignDialog.tsx +++ b/web/src/components/app/campaigns/NewCampaignDialog.tsx @@ -354,6 +354,12 @@ export function NewCampaignDialog({ open, onClose }: Props) { return () => document.removeEventListener("keydown", onKey); }, [open, requestClose]); + // The wizard writes plain text, so it sends plain text. The backend renders + // the HTML part from it: it used to be built here with an escapeHtml that + // turned the quotes in a conditional ({{if eq .Company "Acme"}}) into + // entities, which makes the template fail to parse at send time and ships + // the literal {{if}} to the recipient. The server's version also links bare + // URLs, so a wizard-written step gets click tracking like any other. function buildSteps() { return draft.sequences .filter((s) => s.subject.trim().length > 0 || s.body_plain.trim().length > 0) @@ -361,7 +367,6 @@ export function NewCampaignDialog({ open, onClose }: Props) { name: draft.kind === "one_time" ? "Email" : `Step ${i + 1}`, subject: s.subject.trim(), body_plain: s.body_plain, - body_html: `
${escapeHtml(s.body_plain).replace(/\n/g, "
")}
`, wait_after: i === 0 ? 0 : Math.max(0, s.wait_after), })); } @@ -1476,11 +1481,3 @@ function EstimatePanel({ ); } -function escapeHtml(s: string): string { - return s - .replace(/&/g, "&") - .replace(//g, ">") - .replace(/"/g, """) - .replace(/'/g, "'"); -} diff --git a/web/src/lib/api/client/app/campaigns/createCampaign.ts b/web/src/lib/api/client/app/campaigns/createCampaign.ts index cf6ad8da..a2c28daf 100644 --- a/web/src/lib/api/client/app/campaigns/createCampaign.ts +++ b/web/src/lib/api/client/app/campaigns/createCampaign.ts @@ -59,7 +59,10 @@ export interface CreateCampaignInput { name: string; subject: string; body_plain: string; - body_html: string; + // Optional: leave it off for a plain-text step and the backend renders + // the HTML part from body_plain, keeping the line structure and turning + // bare URLs into links so click tracking has hrefs to wrap. + body_html?: string; body_sync?: boolean; body_code?: boolean; wait_after?: number;