" + body + "
", + BodyPlain: body, + } +} + +// A wait or action node has no subject and no body. Scoring it as copy made +// every campaign that used one fail preflight with "scores 55/100 for spam +// signals" about a step that was never an email. +func TestWorstStepContentScoreSkipsNonEmailSteps(t *testing.T) { + good := strings.Repeat("A real sentence about the recipient's work. ", 5) + seqs := []models.Sequence{ + emailStep(0, "Quick question about hiring", good), + {Kind: "wait", Position: 1}, + {Kind: "action", Position: 2}, + emailStep(3, "Following up on my note", good), + } + + worst, _, _, scored := worstStepContentScore(seqs, 0) + if scored != 2 { + t.Errorf("scored %d steps, want the 2 email steps", scored) + } + if worst != 100 { + t.Errorf("clean campaign scored %d, want 100", worst) + } +} + +// A campaign of nothing but control nodes has no copy to judge, which must read +// as "nothing to score" rather than as a perfect or a failing score. +func TestWorstStepContentScoreReportsNothingToScore(t *testing.T) { + _, _, _, scored := worstStepContentScore([]models.Sequence{ + {Kind: "wait", Position: 0}, + {Kind: "action", Position: 1}, + }, 0) + if scored != 0 { + t.Errorf("scored %d steps, want 0", scored) + } +} + +// The reported step number must be the step's position, so preflight and the +// per-send warning name the same step. +func TestWorstStepContentScoreReportsThePositionOfTheWorstStep(t *testing.T) { + good := strings.Repeat("A real sentence about the recipient's work. ", 5) + seqs := []models.Sequence{ + emailStep(0, "Quick question about hiring", good), + {Kind: "wait", Position: 1}, + emailStep(2, "FREE CASH PRIZE GUARANTEED!!!", "Act now, click here, 100% free, risk free."), + } + + worst, step, issue, scored := worstStepContentScore(seqs, 0) + if scored != 2 { + t.Fatalf("scored %d steps, want 2", scored) + } + if step != 3 { + t.Errorf("worst step reported as %d, want 3 (position 2)", step) + } + if worst >= 60 { + t.Errorf("obviously spammy copy scored %d, want it below the default floor", worst) + } + if issue == "" { + t.Error("no leading issue reported for the worst step") + } +} + +// An empty step list falls out with nothing scored: the caller reports that +// rather than treating it as passing content. +func TestWorstStepContentScoreOnEmptyCampaign(t *testing.T) { + if _, _, _, scored := worstStepContentScore(nil, 0); scored != 0 { + t.Errorf("scored %d steps on an empty campaign, want 0", scored) + } +} + +// Attachments are campaign-wide, so preflight weighs them the way the send path +// does instead of reporting a score the activity feed later contradicts. +func TestWorstStepContentScoreCountsAttachments(t *testing.T) { + good := strings.Repeat("A real sentence about the recipient's work. ", 5) + seqs := []models.Sequence{emailStep(0, "Quick question about hiring", good)} + + clean, _, _, _ := worstStepContentScore(seqs, 0) + withAtt, _, _, _ := worstStepContentScore(seqs, 2) + if withAtt >= clean { + t.Errorf("attachment score %d not below the clean %d", withAtt, clean) + } +} diff --git a/internal/app/advanced/service.go b/internal/app/advanced/service.go index 15fef5b4..d1dbc965 100644 --- a/internal/app/advanced/service.go +++ b/internal/app/advanced/service.go @@ -165,7 +165,10 @@ type service struct { dispatcher EventDispatcher // audienceRepo measures a campaign's list for the preflight report. // Optional/nil-safe: without it the list check is simply absent. - audienceRepo repository.CampaignAudienceRepository + audienceRepo repository.CampaignAudienceRepository + // attachmentRepo lets preflight weigh attachments as the send path does. + // Optional/nil-safe: without it the content check scores none. + attachmentRepo repository.AttachmentRepository notifier Notifier realtime ReplyRealtimePublisher automationRunner AutomationRunner @@ -222,6 +225,7 @@ func (s *service) UpdateOrganizationSettings(ctx context.Context, organizationID if settings == nil { return errx.New(errx.BadRequest, "settings are required") } + settings.Normalize() if err := s.repo.UpsertOutreachSettings(ctx, organizationID, updatedBy, settings); err != nil { return toErrx(err) } @@ -247,6 +251,7 @@ func (s *service) UpdateCampaignSettings(ctx context.Context, campaignID uuid.UU if settings == nil { return errx.New(errx.BadRequest, "settings are required") } + settings.Normalize() if err := s.repo.UpsertCampaignAdvancedSettings(ctx, campaignID, settings); err != nil { return toErrx(err) } @@ -1760,11 +1765,40 @@ func (s *service) ProcessRetryableDeadLetters(ctx context.Context) (int, *errx.E return retried, nil } +// worstStepContentScore returns the lowest-scoring email step's score, number, +// leading issue, and how many steps were scored. Only email steps carry copy: a +// wait or action node would otherwise score as the campaign's worst content. +func worstStepContentScore(seqs []models.Sequence, attachments int) (worst, worstStep int, issue string, scored int) { + worst = 101 + for _, seq := range seqs { + if seq.Kind != "" && seq.Kind != "email" { + continue + } + scored++ + r := warmlint.ScoreWithAttachments(seq.Subject, seq.BodyHTML, seq.BodyPlain, attachments) + if r.Score >= worst { + continue + } + worst, worstStep, issue = r.Score, seq.Position+1, "" + for _, is := range r.Issues { + if is.Severity == "high" { + issue = is.Message + break + } + } + if issue == "" && len(r.Issues) > 0 { + issue = r.Issues[0].Message + } + } + return worst, worstStep, issue, scored +} + // contentScoreCheck scores every step's copy and reports the worst. A step list // it could not read reports as FAILED, not passed: a check that did not run // must never look like one that succeeded. func (s *service) contentScoreCheck(ctx context.Context, campaignID uuid.UUID, floor int, recommendations *[]string) models.PreflightCheckResult { - if floor <= 0 { + // Out of range means a row written before the floor was clamped. + if floor <= 0 || floor > 100 { floor = 60 } seqs, err := s.campaignRepo.GetSequencesByCampaignID(ctx, campaignID) @@ -1787,21 +1821,32 @@ func (s *service) contentScoreCheck(ctx context.Context, campaignID uuid.UUID, f } } - worst, worstStep, issue := 101, 0, "" - for i, seq := range seqs { - r := warmlint.Score(seq.Subject, seq.BodyHTML, seq.BodyPlain) - if r.Score >= worst { - continue - } - worst, worstStep, issue = r.Score, i+1, "" - for _, is := range r.Issues { - if is.Severity == "high" { - issue = is.Message - break + // Attachments are campaign-wide and the send path scores them, so preflight + // weighs them too rather than reporting a score the feed later contradicts. + attachments := 0 + if s.attachmentRepo != nil { + atts, aerr := s.attachmentRepo.ListByCampaign(ctx, campaignID) + if aerr != nil { + // Scoring as none would pass copy the send path then warns about. + *recommendations = append(*recommendations, "Re-run preflight; the campaign's attachments could not be read.") + return models.PreflightCheckResult{ + Key: "content_score", + Passed: false, + Severity: "warning", + Message: "Could not read the campaign's attachments to score its copy.", + Remediation: "Re-run preflight.", } } - if issue == "" && len(r.Issues) > 0 { - issue = r.Issues[0].Message + attachments = len(atts) + } + + worst, worstStep, issue, scored := worstStepContentScore(seqs, attachments) + if scored == 0 { + return models.PreflightCheckResult{ + Key: "content_score", + Passed: true, + Severity: "info", + Message: "No email steps to score yet.", } } @@ -1862,3 +1907,17 @@ func (s *service) WireAudience(r repository.CampaignAudienceRepository) { type AudienceAware interface { WireAudience(r repository.CampaignAudienceRepository) } + +// WireAttachments attaches the campaign attachment counter the content check +// scores with, so preflight and the send path weigh attachments the same way. +func (s *service) WireAttachments(r repository.AttachmentRepository) { + s.attachmentRepo = r +} + +// AttachmentAware is the optional capability the caller uses to attach it. +type AttachmentAware interface { + WireAttachments(r repository.AttachmentRepository) +} + +// main attaches this by type assertion, which fails silently, so pin it here. +var _ AttachmentAware = (*service)(nil) diff --git a/internal/models/advanced_outreach.go b/internal/models/advanced_outreach.go index 614eea26..4cb38d51 100644 --- a/internal/models/advanced_outreach.go +++ b/internal/models/advanced_outreach.go @@ -61,10 +61,24 @@ type PreflightValidationSettings struct { // and again per send against the rendered text. Advisory: it warns, it // never blocks a send. CheckContentScore bool `json:"check_content_score"` - // MinContentScore is the 0-100 floor below which copy is flagged. + // MinContentScore is the 1-100 floor below which copy is flagged. MinContentScore int `json:"min_content_score"` } +// Normalize clamps the settings an API caller can put out of range, so a stored +// value can never make every campaign fail the check or none of them. +func (s *AdvancedOutreachSettings) Normalize() { + if s == nil { + return + } + if s.Preflight.MinContentScore > 100 { + s.Preflight.MinContentScore = 100 + } + if s.Preflight.MinContentScore < 1 { + s.Preflight.MinContentScore = 1 + } +} + type DeliverabilityDashboardSettings struct { Enabled bool `json:"enabled"` ShowSuppressionLog bool `json:"show_suppression_log"` diff --git a/internal/models/advanced_outreach_test.go b/internal/models/advanced_outreach_test.go new file mode 100644 index 00000000..db20d61d --- /dev/null +++ b/internal/models/advanced_outreach_test.go @@ -0,0 +1,33 @@ +package models + +import "testing" + +// The content-score floor reaches the API as a plain integer. Left unclamped, a +// floor above 100 flags every campaign forever and a floor at or below 0 is a +// control that does nothing, since the readers fall back to the default. +func TestNormalizeClampsTheContentScoreFloor(t *testing.T) { + for _, tc := range []struct{ in, want int }{ + {-40, 1}, + {0, 1}, + {1, 1}, + {60, 60}, + {100, 100}, + {5000, 100}, + } { + s := DefaultAdvancedOutreachSettings() + s.Preflight.MinContentScore = tc.in + s.Normalize() + if s.Preflight.MinContentScore != tc.want { + t.Errorf("floor %d normalized to %d, want %d", tc.in, s.Preflight.MinContentScore, tc.want) + } + } +} + +func TestNormalizeLeavesTheDefaultsAlone(t *testing.T) { + s := DefaultAdvancedOutreachSettings() + before := s + s.Normalize() + if s.Preflight != before.Preflight { + t.Errorf("defaults changed under Normalize: %+v -> %+v", before.Preflight, s.Preflight) + } +} diff --git a/internal/pkg/warmlint/lint.go b/internal/pkg/warmlint/lint.go index 44c7ec46..2931bef8 100644 --- a/internal/pkg/warmlint/lint.go +++ b/internal/pkg/warmlint/lint.go @@ -13,7 +13,8 @@ import ( var ( stackedPunct = regexp.MustCompile(`[!?]{2,}`) wordToken = regexp.MustCompile(`[a-z0-9%]+`) - linkPattern = regexp.MustCompile(`https?://`) + linkPattern = regexp.MustCompile(`https?://[^\s"'<>)\]]*`) + hrefPattern = regexp.MustCompile(`(?i)href\s*=\s*["']?\s*(https?://[^\s"'<>]*)`) htmlTag = regexp.MustCompile(`(?i)<[a-z!/][^>]*>`) imgTag = regexp.MustCompile(`(?i)" + body + "
" + + strings.Repeat(`see this `, 6) + "
" + + res := Score("Quick question", html, "") + if !hasIssue(res, "too_many_links") { + t.Errorf("six anchors in an HTML body were not counted: %+v", res.Issues) + } + + // The editor stores a plain-text body derived from the HTML, which also + // drops the hrefs. The score must not depend on which one is present. + res = Score("Quick question", html, body) + if !hasIssue(res, "too_many_links") { + t.Errorf("six anchors alongside a link-free plain body were not counted: %+v", res.Issues) + } +} + +func TestScoreDoesNotDoubleCountSelfLinkingAnchors(t *testing.T) { + // A URL used as its own anchor text appears in both the href and the text. + // Counting both would flag three links as six. + body := strings.Repeat("A real sentence about the recipient's work. ", 10) + html := "" + body + "
" + + `https://a.com/1 ` + + `https://b.com/2 ` + + `https://c.com/3` + "
" + plain := body + " https://a.com/1 https://b.com/2 https://c.com/3" + + res := Score("Quick question", html, plain) + if hasIssue(res, "too_many_links") { + t.Errorf("three self-linking anchors were counted as more: %+v", res.Issues) + } +} + +func TestScoreStillCountsPlainTextURLs(t *testing.T) { + body := strings.Repeat("A real sentence about the recipient's work. ", 10) + plain := body + " https://a.com/1 https://b.com/2 https://c.com/3 https://d.com/4 https://e.com/5" + + res := Score("Quick question", "", plain) + if !hasIssue(res, "too_many_links") { + t.Errorf("five bare URLs in a plain body were not counted: %+v", res.Issues) + } +} + +func TestScoreCountsAnchorsAndBareURLsTogether(t *testing.T) { + // Labeled anchors and bare URLs are different destinations. Counting only + // the larger of the two sets let six distinct links score a clean 100. + body := strings.Repeat("A real sentence about the recipient's work. ", 10) + html := "" + body + "
" + + `one two three ` + + "https://d.com/4 https://e.com/5 https://f.com/6
" + plain := body + " one two three https://d.com/4 https://e.com/5 https://f.com/6" + + res := Score("Quick question", html, plain) + if !hasIssue(res, "too_many_links") { + t.Errorf("three anchors plus three bare URLs were not counted as six: %+v", res.Issues) + } +} + +func TestScoreIgnoresTrailingPunctuationWhenMatchingAnchors(t *testing.T) { + // A URL that ends a sentence in the plain text is the same link as the + // anchor's destination, so it must not count a second time. + body := strings.Repeat("A real sentence about the recipient's work. ", 10) + html := "" + body + "
" + + `https://a.com/1, https://b.com/2.` + "
" + plain := body + " https://a.com/1, https://b.com/2." + + res := Score("Quick question", html, plain) + if hasIssue(res, "too_many_links") { + t.Errorf("two self-linking anchors counted as more than two: %+v", res.Issues) + } +} diff --git a/internal/repository/pg_campaign.go b/internal/repository/pg_campaign.go index 49e00f2f..5cfe42f9 100644 --- a/internal/repository/pg_campaign.go +++ b/internal/repository/pg_campaign.go @@ -1265,7 +1265,7 @@ func (r *campaignRepository) GetSequenceByID(ctx context.Context, sequenceID uui // GetSequencesByCampaignID retrieves all sequences for a campaign ordered by position func (r *campaignRepository) GetSequencesByCampaignID(ctx context.Context, campaignID uuid.UUID) ([]models.Sequence, error) { query := ` - SELECT id, name, subject, body_plain, body_html, body_sync, body_code, wait_after, position, updated_at, created_at + SELECT id, name, subject, body_plain, body_html, body_sync, body_code, wait_after, position, kind, updated_at, created_at FROM sequences WHERE campaign_id = $1 ORDER BY position ASC, created_at ASC @@ -1283,7 +1283,7 @@ func (r *campaignRepository) GetSequencesByCampaignID(ctx context.Context, campa var seq models.Sequence err := rows.Scan( &seq.ID, &seq.Name, &seq.Subject, &seq.BodyPlain, &seq.BodyHTML, - &seq.BodySync, &seq.BodyCode, &seq.WaitAfter, &seq.Position, &seq.UpdatedAt, &seq.CreatedAt, + &seq.BodySync, &seq.BodyCode, &seq.WaitAfter, &seq.Position, &seq.Kind, &seq.UpdatedAt, &seq.CreatedAt, ) if err != nil { db.CaptureError(err, "", nil, "scan") diff --git a/internal/repository/sequence_kind_live_test.go b/internal/repository/sequence_kind_live_test.go new file mode 100644 index 00000000..4beed58f --- /dev/null +++ b/internal/repository/sequence_kind_live_test.go @@ -0,0 +1,55 @@ +package repository + +import ( + "context" + "testing" + + "github.com/google/uuid" +) + +// GetSequencesByCampaignID did not select `kind`, so every step came back +// looking like an email. Preflight's content check then scored wait and action +// nodes as copy and reported their empty subject and body as the campaign's +// worst content. +// +// WARMBLY_TEST_DB=postgres://warmbly:warmbly@localhost:15432/warmbly_dev?sslmode=disable \ +// go test ./internal/repository/ -run LiveSequenceKind -v +func TestLiveSequenceKindSurvivesTheRoundTrip(t *testing.T) { + handle, pool := liveContactDB(t) + f := newSharedOrgFixture(t, pool) + repo := NewCampaignRepostory(handle) + ctx := context.Background() + + t.Cleanup(func() { + if _, err := pool.Exec(context.Background(), + `DELETE FROM sequences WHERE campaign_id = $1`, f.campaign); err != nil { + t.Errorf("cleanup sequences: %v", err) + } + }) + + for _, step := range []struct { + pos int + kind string + name string + }{{0, "email", "Intro"}, {1, "wait", "Hold"}, {2, "action", "Tag"}} { + if _, err := pool.Exec(ctx, + `INSERT INTO sequences (id, campaign_id, organization_id, name, subject, body_plain, body_html, wait_after, position, kind) + VALUES ($1, $2, $3, $4, '', '', '', 0, $5, $6)`, + uuid.New(), f.campaign, f.org, step.name, step.pos, step.kind); err != nil { + t.Fatalf("insert %s step: %v", step.kind, err) + } + } + + seqs, err := repo.GetSequencesByCampaignID(ctx, f.campaign) + if err != nil { + t.Fatalf("get sequences: %v", err) + } + if len(seqs) != 3 { + t.Fatalf("got %d steps, want 3", len(seqs)) + } + for i, want := range []string{"email", "wait", "action"} { + if seqs[i].Kind != want { + t.Errorf("step %d kind = %q, want %q", i, seqs[i].Kind, want) + } + } +} diff --git a/internal/tasks/content_gate.go b/internal/tasks/content_gate.go index 20959763..f5215679 100644 --- a/internal/tasks/content_gate.go +++ b/internal/tasks/content_gate.go @@ -23,6 +23,12 @@ func (s *tasksService) warnOnWeakContent(ctx context.Context, orgID, campaignID, if s.advanced == nil || s.campaignLogRepo == nil { return } + // A clean 100 clears every floor, so the common case never reads settings. + res := warmlint.ScoreWithAttachments(subject, bodyHTML, bodyPlain, attachments) + if res.Score >= 100 { + return + } + // Campaign-effective, not org-only: a campaign that turned the check off or // moved its floor must be honored here as it is at preflight. settings, xerr := s.advanced.EffectiveSettings(ctx, orgID, campaignID) @@ -30,11 +36,10 @@ func (s *tasksService) warnOnWeakContent(ctx context.Context, orgID, campaignID, return } floor := settings.Preflight.MinContentScore - if floor <= 0 { + // Out of range means a row written before the floor was clamped. + if floor <= 0 || floor > 100 { floor = 60 } - - res := warmlint.ScoreWithAttachments(subject, bodyHTML, bodyPlain, attachments) if res.Score >= floor { return } @@ -62,7 +67,8 @@ func (s *tasksService) warnOnWeakContent(ctx context.Context, orgID, campaignID, Message: fmt.Sprintf("Step %d's copy scores %d/100 for spam signals as sent (floor %d).%s", step, res.Score, floor, detail), Metadata: map[string]interface{}{ - "level": "warning", + // "warn" is the dashboard's amber tier; anything else reads as info. + "level": "warn", "sequence_id": seq, "score": res.Score, "floor": floor, diff --git a/web/src/app/app/settings/sending/page.tsx b/web/src/app/app/settings/sending/page.tsx index af9bf270..e51e1f37 100644 --- a/web/src/app/app/settings/sending/page.tsx +++ b/web/src/app/app/settings/sending/page.tsx @@ -222,12 +222,12 @@ function SendingSettings() { description="Copy scoring below this out of 100 is flagged. Higher is stricter." >Advisory deliverability score — never blocks sending.
+Advisory deliverability score. It never blocks sending.