feat: address the CodeRabbit review on the one-time email preset: rate-limit POST /campaigns-estimate as a read, count a mailbox whose sent-today counter fails as having nothing left today instead of untouched, lock the campaign row FOR UPDATE before counting email steps so concurrent inserts cannot give a one-time campaign two messages, keep a plain-text test email free of the HTML signature, and make the wizard's estimate panel say an audience beyond the two-year horizon cannot be projected instead of reading the null as one sending day

This commit is contained in:
Matthew Meszaros
2026-09-03 01:42:00 -07:00
parent 24b7e26fbb
commit 6b1ddd8bb8
5 changed files with 31 additions and 12 deletions
+1 -1
View File
@@ -462,7 +462,7 @@ func Run(
// Audience-versus-pool projection for the campaign wizard (no
// campaign id yet). Read-level: it writes nothing.
protected.POST("/campaigns-estimate", m.RequireOrganization(), m.RequireAccess(models.PermViewCampaigns, models.APIPermReadCampaigns), h.EstimateCampaign)
protected.POST("/campaigns-estimate", m.RateLimitMiddleware(models.RateLimitRead), m.RequireOrganization(), m.RequireAccess(models.PermViewCampaigns, models.APIPermReadCampaigns), h.EstimateCampaign)
campaigns := protected.Group("/campaigns")
campaigns.Use(m.RateLimitMiddleware(models.RateLimitWrite))
+4 -3
View File
@@ -932,9 +932,10 @@ func (s *campaignService) Estimate(ctx context.Context, orgID uuid.UUID, in *mod
out.DailyCapacity += lim
sent, err := s.taskRepo.CountCampaignEmailsSentToday(ctx, acct.ID)
if err != nil {
// A counter blip must not blank the whole estimate; today then
// reads as untouched, which is the optimistic side.
sent = 0
// A counter blip must not blank the whole estimate, but it must
// not flatter it either: a mailbox whose sends today are unknown
// contributes nothing to today and only counts from tomorrow.
continue
}
out.RemainingToday += max(0, lim-sent)
}
+3
View File
@@ -129,9 +129,12 @@ func (r *sequenceRepository) Create(ctx context.Context, userID string, campaign
}
defer tx.Rollback(ctx)
// FOR UPDATE serialises step creation per campaign, so two concurrent
// inserts on a one-time campaign cannot both see zero email steps.
query := `
SELECT user_id, organization_id, kind
FROM campaigns WHERE id = $1
FOR UPDATE
`
params := []any{
+5 -1
View File
@@ -54,8 +54,12 @@ func (s *tasksService) SendTestEmail(ctx context.Context, userID string, account
subject = "[TEST] " + subject
// Add signature if enabled
// Same guard as the campaign send path: a plain-text test must not grow
// an HTML part back through the signature.
if account.SignatureSync {
bodyHTML = AddSignature(bodyHTML, account.SignatureHTML, true)
if bodyHTML != "" {
bodyHTML = AddSignature(bodyHTML, account.SignatureHTML, true)
}
bodyPlain = AddSignature(bodyPlain, account.SignaturePlain, false)
}
@@ -1377,16 +1377,27 @@ function EstimatePanel({
</>
);
tone = "warn";
} else if (sendingDays === null || finish === null) {
// The backend leaves both null when the audience is not covered
// inside its two-year horizon, which must not read as "one day".
headline = "Longer than this estimate can project";
detail = (
<>
{recipients.toLocaleString()} recipients at up to {dailyCapacity.toLocaleString()} a day would take
years.{" "}
<button type="button" onClick={onEditPool} className="underline underline-offset-2 hover:text-slate-900">
Add mailboxes or raise the daily limit
</button>
, or narrow the audience.
</>
);
tone = "warn";
} else {
const days = sendingDays ?? 0;
const days = sendingDays;
headline =
days <= 1
? finish
? `Finishes ${startsAt ? "on" : "today,"} ${fmtDate(finish)}`
: "Finishes in one sending day"
: finish
? `About ${days} sending days, finishing around ${fmtDate(finish)}`
: `More than ${days} sending days`;
? `Finishes ${startsAt ? "on" : "today,"} ${fmtDate(finish)}`
: `About ${days} sending days, finishing around ${fmtDate(finish)}`;
detail = (
<>
{recipients.toLocaleString()} recipient{recipients === 1 ? "" : "s"} across {mailboxes} mailbox