diff --git a/docs/content/docs/api/reference/deliverability-ops.mdx b/docs/content/docs/api/reference/deliverability-ops.mdx
index 08a42eb0a..2ec34e0e7 100644
--- a/docs/content/docs/api/reference/deliverability-ops.mdx
+++ b/docs/content/docs/api/reference/deliverability-ops.mdx
@@ -106,11 +106,15 @@ The `unsubscribe` block is the workspace default for the opt-out appended after
`send_time_optimization.enabled` defaults to `false`. Set it to `true` and campaign scheduling holds each send until the recipient's local clock reaches one of `preferred_hours`, resolving the recipient's timezone from the contact's `timezone` custom field, then the country-code suffix of its email domain, then `default_contact_timezone`. It can only delay a send: the campaign window, the mailbox's sending profile, its daily cap, and the campaign end date all still bind. See [Sending behavior](/guides/sending-behavior/).
+
+`inbox_tagging.action_required_in_inbox` defaults to `true`. With automatic inbox tagging on, each automated notification is also asked whether it needs someone to act (a failed payment, a suspended account, a suspicious sign-in, a sending limit, a service about to expire), and the ones that do stay in the inbox labelled Action required instead of moving to the Automated view. A question in `inbox_tagging.questions` with `automated: true` is also asked of notifications. See [Mail that needs your action](/guides/inbox-tagging/#mail-that-needs-your-action).
+
+
## Update outreach settings
`PATCH /outreach/settings`
-Replaces the organization's advanced outreach settings with the supplied object. Send the full settings block (the value is upserted, not deep-merged). Returns no body on success.
+Replaces the organization's advanced outreach settings with the supplied object. Send the full settings block (the value is upserted, not deep-merged). A field the object omits takes its default, not its previous value. Returns no body on success.
Auth: **Scope** `WRITE_CAMPAIGNS` · **Org permission** `manage_settings`
diff --git a/docs/content/docs/guides/inbox-tagging.mdx b/docs/content/docs/guides/inbox-tagging.mdx
index 99bb02c84..e371e5b11 100644
--- a/docs/content/docs/guides/inbox-tagging.mdx
+++ b/docs/content/docs/guides/inbox-tagging.mdx
@@ -59,11 +59,11 @@ So every notification is also asked one question: does it tell the recipient abo
- the conversation **stays in the inbox**, and in Unread and the unread badge, instead of moving to Automated
- it is labelled **Action required** (next to **Notification**), and the **Action required** view in the rail lists every such conversation with its unread count
- it sorts with the replies due today when the inbox is sorted by relevance
-- members who can both manage mailboxes and use the inbox get a **Mail that needs action** [notification](/guides/notifications/), by email too unless they turned it off. It is tied to the message, so reading the message reads the notification
+- members who can both manage mailboxes and use the inbox get a **Mail that needs action** [notification](/guides/notifications/), by email too unless they turned it off. It names the mailbox and links to the conversation but never quotes the message, since mail that claims an account is suspended is also what phishing looks like. It is tied to the message, so reading the message reads the notification
Routine notices are not affected: a sign-in code, a receipt, a newsletter or a "your settings were changed" confirmation still goes to Automated. Archive the conversation once the problem is dealt with. The label is an ordinary label: removing it takes the conversation out of the **Action required** view but leaves it in the inbox.
-A notification recognised offline by its sender alone costs one small call for this question and nothing else; a notification the model judged has it in the same call as every other question. If the call fails, the message is left untagged and stays in the inbox rather than being filed away unread.
+A notification recognised offline by its sender alone costs one small call for this question and nothing else; a notification the model judged has it in the same call as every other question. If that call fails, the notification is filed under Automated as it was before the check existed, and `--recheck-notifications` (below) asks it later.
It is on by default for every workspace. To turn it off, clear **Keep mail that needs action in the inbox** under **Settings > Sending > Automated mail**; notifications then go to Automated as before and no call is made for them. To file other automated mail your own way, ask one of [your own questions](#your-own-questions) of notifications too.
@@ -222,7 +222,7 @@ Notifications classified before the action check existed are sitting in Automate
warmblyctl inbox-tag backfill --org you@example.com --days 30 --recheck-notifications
```
-Only messages stored as a notification by a verdict that never asked the action check are covered, and each is asked again, so the ones that need action return to the inbox labelled **Action required**. The workspace has to ask notifications something: the command refuses when **Keep mail that needs action in the inbox** is off and none of its questions is asked of notifications. A message whose call fails is left untagged, and in the inbox, for the next plain backfill. Like any backfill, it labels, moves and never notifies.
+Only messages stored as a notification by a verdict that never asked the action check are covered, and each is asked again, so the ones that need action return to the inbox labelled **Action required**. The command refuses when **Keep mail that needs action in the inbox** is off. A notification with no text to read is skipped. A message whose call fails is offered again by the next run, or by a plain backfill. Like any backfill, it labels, moves and never notifies.
## What it costs
diff --git a/docs/public/openapi.json b/docs/public/openapi.json
index 5fcbbe55b..a2f67acef 100644
--- a/docs/public/openapi.json
+++ b/docs/public/openapi.json
@@ -18812,7 +18812,7 @@
"patch": {
"operationId": "deliverability-ops_update_settings",
"summary": "Update outreach settings",
- "description": "Replaces the organization's advanced outreach settings with the supplied object (upserted, not deep-merged). Returns no body.",
+ "description": "Replaces the organization's advanced outreach settings with the supplied object (upserted, not deep-merged). A field the object omits takes its default, not its previous value. Returns no body.",
"tags": [
"deliverability-ops"
],
diff --git a/internal/api/handler/advanced_outreach.go b/internal/api/handler/advanced_outreach.go
index 48252c632..3ad3eff0a 100644
--- a/internal/api/handler/advanced_outreach.go
+++ b/internal/api/handler/advanced_outreach.go
@@ -38,7 +38,9 @@ func (h *Handler) UpdateOutreachSettings(c *gin.Context) {
errx.JSON(c, errx.New(errx.BadRequest, "invalid user id"))
return
}
- var req models.UpsertOutreachSettingsRequest
+ // A field the request omits takes its default rather than its zero value,
+ // so a client written before a default-on setting cannot switch it off.
+ req := models.UpsertOutreachSettingsRequest{Settings: models.DefaultAdvancedOutreachSettings()}
if err := c.ShouldBindJSON(&req); err != nil {
errx.JSON(c, errx.InvalidBody(err))
return
diff --git a/internal/app/advanced/events.go b/internal/app/advanced/events.go
index 42caac544..c24e50205 100644
--- a/internal/app/advanced/events.go
+++ b/internal/app/advanced/events.go
@@ -185,8 +185,8 @@ func (s *service) notifyAboutMessage(userID uuid.UUID, orgID *uuid.UUID, uniboxE
}()
}
-// uniboxThreadLink opens the conversation itself rather than the inbox.
-func uniboxThreadLink(threadID string) string {
+// UniboxThreadLink opens the conversation itself rather than the inbox.
+func UniboxThreadLink(threadID string) string {
if threadID == "" {
return "/app/unibox"
}
diff --git a/internal/app/advanced/service.go b/internal/app/advanced/service.go
index 69b343547..d5049f527 100644
--- a/internal/app/advanced/service.go
+++ b/internal/app/advanced/service.go
@@ -290,7 +290,11 @@ func (s *service) UpdateOrganizationSettings(ctx context.Context, organizationID
if err := settings.Validate(); err != nil {
return errx.NewWithIdentifier(errx.BadRequest, "invalid_setting", err.Error())
}
- if err := inboxtag.ValidateQuestions(settings.InboxTagging.Questions); err != nil {
+ var saved []models.InboxTagQuestion
+ if current, err := s.repo.GetOutreachSettings(ctx, organizationID); err == nil && current != nil {
+ saved = current.InboxTagging.Questions
+ }
+ if err := inboxtag.ValidateQuestions(settings.InboxTagging.Questions, saved); err != nil {
return errx.NewWithIdentifier(errx.BadRequest, "invalid_setting", err.Error())
}
if err := s.repo.UpsertOutreachSettings(ctx, organizationID, updatedBy, settings); err != nil {
@@ -1567,7 +1571,7 @@ func (s *service) ProcessIncomingReply(ctx context.Context, emailAccountID uuid.
body = "Held until " + held.Format("2 Jan") + " · " + msg.Subject
}
}
- s.notifyAboutMessage(uid, account.OrganizationID, msg.ID, cat, title, body, uniboxThreadLink(msg.ThreadID), map[string]any{
+ s.notifyAboutMessage(uid, account.OrganizationID, msg.ID, cat, title, body, UniboxThreadLink(msg.ThreadID), map[string]any{
"intent": string(intent),
"email_account_id": emailAccountID.String(),
"thread_id": msg.ThreadID,
diff --git a/internal/app/consumer/action_required_notify_test.go b/internal/app/consumer/action_required_notify_test.go
index 86862e62b..7d7e1989d 100644
--- a/internal/app/consumer/action_required_notify_test.go
+++ b/internal/app/consumer/action_required_notify_test.go
@@ -2,7 +2,9 @@ package jobs
import (
"context"
+ "strings"
"testing"
+ "time"
"github.com/google/uuid"
@@ -20,46 +22,51 @@ type capturedOrgNotice struct {
groupKey string
}
-type captureOrgNotifier struct{ got []capturedOrgNotice }
+type captureOrgNotifier struct{ got chan capturedOrgNotice }
func (c *captureOrgNotifier) NotifyOrg(context.Context, uuid.UUID, models.OrganizationPermission, uuid.UUID, models.NotificationCategory, string, string, string, map[string]any, string) {
}
func (c *captureOrgNotifier) NotifyOrgAboutMessage(_ context.Context, orgID uuid.UUID, perm models.OrganizationPermission, message uuid.UUID, category models.NotificationCategory, title, body, link string, _ map[string]any, groupKey string) {
- c.got = append(c.got, capturedOrgNotice{orgID, perm, message, category, title, body, link, groupKey})
+ c.got <- capturedOrgNotice{orgID, perm, message, category, title, body, link, groupKey}
}
// The notice goes to members who both keep mailboxes running and can open the
-// message, and is tied to the message so reading it reads the notification.
+// message, is tied to the message, and never carries the sender's subject.
func TestNotifyActionRequiredReachesMailboxManagers(t *testing.T) {
- n := &captureOrgNotifier{}
+ n := &captureOrgNotifier{got: make(chan capturedOrgNotice, 1)}
s := &JobsService{Notifier: n}
orgID := uuid.New()
msg := &models.EmailMessageStoreData{
ID: uuid.New(),
EmailID: uuid.New(),
ThreadID: "thread/1",
- Subject: " Payment declined ",
+ Subject: "Your account is suspended, verify at evil.example",
}
- s.notifyActionRequired(context.Background(), orgID, msg)
+ s.notifyActionRequired(orgID, "sales@acme.test", msg)
- if len(n.got) != 1 {
- t.Fatalf("raised %d notifications, want 1", len(n.got))
+ var got capturedOrgNotice
+ select {
+ case got = <-n.got:
+ case <-time.After(2 * time.Second):
+ t.Fatal("no notification raised")
}
- got := n.got[0]
if got.orgID != orgID || got.message != msg.ID || got.category != models.NotifInboxActionRequired {
t.Fatalf("notice = %+v", got)
}
if got.perm != models.PermManageEmails|models.PermAccessUnibox {
t.Fatalf("perm = %b, want manage mailboxes and use the inbox", got.perm)
}
- if got.body != "Payment declined" || got.link != "/app/unibox/all/thread%2F1" || got.groupKey == "" {
+ if got.title != "Action required in sales@acme.test" || got.link != "/app/unibox/all/thread%2F1" || got.groupKey == "" {
t.Fatalf("notice = %+v", got)
}
+ if strings.Contains(got.title+got.body, "evil.example") {
+ t.Fatalf("the sender's subject reached the notification: %+v", got)
+ }
}
func TestNotifyActionRequiredWithoutNotifierIsQuiet(t *testing.T) {
s := &JobsService{}
- s.notifyActionRequired(context.Background(), uuid.New(), &models.EmailMessageStoreData{ID: uuid.New()})
+ s.notifyActionRequired(uuid.New(), "", &models.EmailMessageStoreData{ID: uuid.New()})
}
diff --git a/internal/app/consumer/event_new_email.go b/internal/app/consumer/event_new_email.go
index 97a9c951a..df46505b7 100644
--- a/internal/app/consumer/event_new_email.go
+++ b/internal/app/consumer/event_new_email.go
@@ -5,7 +5,6 @@ import (
"encoding/json"
"errors"
"fmt"
- "net/url"
"slices"
"strings"
"time"
@@ -703,10 +702,11 @@ func containsSpamFlag(flags []string) bool {
// because they are facts and a question about a fact is a question that can be
// answered confidently and wrongly.
func (s *JobsService) tagInboundMessage(ctx context.Context, e *models.JobEventNewEmail) {
- orgID, err := s.orgForMailbox(ctx, e.Message.EmailID)
- if err != nil || orgID == uuid.Nil {
+ account := s.recipientAccount(ctx, e.Message.EmailID)
+ if account == nil || account.OrganizationID == nil || *account.OrganizationID == uuid.Nil {
return
}
+ orgID := *account.OrganizationID
// Our previous message in the thread, read from the database rather than
// asked. A reply is an answer, and the question it answers is not in it:
@@ -727,7 +727,7 @@ func (s *JobsService) tagInboundMessage(ctx context.Context, e *models.JobEventN
// Live arrivals only; the backfill labels history and never acts on it.
s.actOnInboxTag(ctx, orgID, msg, d)
if d.ActionRequired {
- s.notifyActionRequired(ctx, orgID, e.Message)
+ s.notifyActionRequired(orgID, account.Email, e.Message)
}
// Tell the dashboard the message changed.
@@ -745,28 +745,31 @@ func (s *JobsService) tagInboundMessage(ctx context.Context, e *models.JobEventN
}
// notifyActionRequired tells the members who keep mailboxes running that one
-// received mail needing action, since nobody may be reading that mailbox.
-func (s *JobsService) notifyActionRequired(ctx context.Context, orgID uuid.UUID, m *models.EmailMessageStoreData) {
+// received mail needing action, since nobody may be reading that mailbox. The
+// sender's subject stays out: this mail is phishing-shaped by selection.
+func (s *JobsService) notifyActionRequired(orgID uuid.UUID, mailbox string, m *models.EmailMessageStoreData) {
if s.Notifier == nil {
return
}
title := "Action required in a mailbox"
- if acc := s.recipientAccount(ctx, m.EmailID); acc != nil && acc.Email != "" {
- title = "Action required in " + acc.Email
+ if mailbox != "" {
+ title = "Action required in " + mailbox
}
- body := strings.TrimSpace(m.Subject)
- if body == "" {
- body = "(no subject)"
+ notify := func(ctx context.Context) {
+ s.Notifier.NotifyOrgAboutMessage(ctx, orgID, models.PermManageEmails|models.PermAccessUnibox, m.ID,
+ models.NotifInboxActionRequired, title,
+ "An automated message in this mailbox says something needs someone to act. Open it in the inbox.",
+ advanced.UniboxThreadLink(m.ThreadID), map[string]any{
+ "email_account_id": m.EmailID.String(),
+ "thread_id": m.ThreadID,
+ }, "inbox_action_required:"+m.ID.String())
}
- link := "/app/unibox"
- if m.ThreadID != "" {
- link = "/app/unibox/all/" + url.PathEscape(m.ThreadID)
- }
- s.Notifier.NotifyOrgAboutMessage(ctx, orgID, models.PermManageEmails|models.PermAccessUnibox, m.ID,
- models.NotifInboxActionRequired, title, body, link, map[string]any{
- "email_account_id": m.EmailID.String(),
- "thread_id": m.ThreadID,
- }, "inbox_action_required:"+m.ID.String())
+ // Off the ingest path: the fan-out is a round trip per member.
+ go func() {
+ ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+ defer cancel()
+ notify(ctx)
+ }()
}
// actOnInboxTag executes the actions the tagging policy allows for one
@@ -803,17 +806,3 @@ func (s *JobsService) actOnInboxTag(ctx context.Context, orgID uuid.UUID, msg in
}
log.Info().Str("message_id", msg.MessageID).Strs("actions", done).Msg("inbox tagging acted on a reply")
}
-
-// orgForMailbox resolves the workspace that owns a mailbox. Tagging is scoped
-// per workspace (labels, storage, idempotency), so a mailbox with no org is not
-// taggable rather than taggable into nowhere.
-func (s *JobsService) orgForMailbox(ctx context.Context, accountID uuid.UUID) (uuid.UUID, error) {
- if s.EmailRepository == nil {
- return uuid.Nil, nil
- }
- account, xerr := s.EmailRepository.GetByID(ctx, accountID)
- if xerr != nil || account == nil || account.OrganizationID == nil {
- return uuid.Nil, nil
- }
- return *account.OrganizationID, nil
-}
diff --git a/internal/app/inboxtag/action_required_test.go b/internal/app/inboxtag/action_required_test.go
index 17bc9eae6..25f7a3236 100644
--- a/internal/app/inboxtag/action_required_test.go
+++ b/internal/app/inboxtag/action_required_test.go
@@ -133,19 +133,23 @@ func TestOfflineNotificationWithTheCheckOffMakesNoCall(t *testing.T) {
}
}
-// A failed check never hides the notice: it stays untagged, and in the inbox,
-// for the next backfill.
-func TestFailedActionCheckLeavesTheNoticeUntagged(t *testing.T) {
+// A failed check files the notice as it was filed before the check existed,
+// unasked, so an outage does not fill every inbox with newsletters and the
+// notification re-check asks it later.
+func TestFailedActionCheckFilesTheNoticeUnasked(t *testing.T) {
asker := &failingAsker{}
repo := &fakeRepo{}
svc := newService(t, asker, repo)
- m := billingNotice()
- if _, err := svc.Classify(context.Background(), m); err == nil {
- t.Fatal("a failed call was not reported")
+ d, err := svc.Classify(context.Background(), billingNotice())
+ if err != nil {
+ t.Fatalf("classify: %v", err)
}
- if asker.calls != 1 || len(repo.saved) != 0 || repo.tagged[m.MessageID] {
- t.Fatalf("calls %d, saved %v, claim kept %v", asker.calls, repo.saved, repo.tagged[m.MessageID])
+ if asker.calls != 1 || !d.Automated() || len(repo.saved) != 1 || !repo.saved[0].Automated {
+ t.Fatalf("calls %d, decision %+v, saved %+v", asker.calls, d, repo.saved)
+ }
+ if string(repo.saved[0].Answers) != "{}" {
+ t.Fatalf("answers = %s, want none so the re-check offers it again", repo.saved[0].Answers)
}
}
@@ -237,9 +241,26 @@ func TestRecheckNotificationsReclassifiesStoredNotices(t *testing.T) {
}
}
+// Nothing to read is skipped rather than reopened, or it would come back
+// unasked and be offered on every run.
+func TestRecheckNotificationsSkipsEmptyNotices(t *testing.T) {
+ asker := &countingAsker{}
+ repo := &fakeRepo{notices: []repository.BackfillCandidate{{MessageID: "", FromAddr: "no-reply@vendor.test"}}}
+ svc := newService(t, asker, repo)
+
+ p, err := svc.Backfill(context.Background(), uuid.New(), BackfillOptions{RecheckNotifications: true})
+ if err != nil {
+ t.Fatalf("recheck: %v", err)
+ }
+ if p.Skipped != 1 || asker.calls != 0 || len(repo.reopened) != 0 {
+ t.Fatalf("progress %+v, calls %d, reopened %v", p, asker.calls, repo.reopened)
+ }
+}
+
func TestRecheckNotificationsRefusesWhenNothingIsAsked(t *testing.T) {
svc := newService(t, &countingAsker{}, &fakeRepo{})
- svc.WireSettings(noticeSettings(false))
+ // A question asked of notifications alone does not write the check's answer.
+ svc.WireSettings(noticeSettings(false, invoiceQuestion(true)))
if _, err := svc.Backfill(context.Background(), uuid.New(), BackfillOptions{RecheckNotifications: true}); !errors.Is(err, ErrNothingToAsk) {
t.Fatalf("err = %v, want ErrNothingToAsk", err)
}
diff --git a/internal/app/inboxtag/custom.go b/internal/app/inboxtag/custom.go
index 6e45bf515..b792095be 100644
--- a/internal/app/inboxtag/custom.go
+++ b/internal/app/inboxtag/custom.go
@@ -167,10 +167,16 @@ func ReservedLabel(label string) bool {
}
// ValidateQuestions refuses a workspace question whose label the built-in
-// taxonomy owns. Shape is checked by the settings model.
-func ValidateQuestions(qs []models.InboxTagQuestion) error {
+// taxonomy owns. A label the workspace had already saved stays valid, so a
+// built-in label added later does not block every settings save that follows.
+// Shape is checked by the settings model.
+func ValidateQuestions(qs, saved []models.InboxTagQuestion) error {
+ kept := map[string]bool{}
+ for _, l := range CustomLabels(saved) {
+ kept[strings.ToLower(l)] = true
+ }
check := func(label string) error {
- if ReservedLabel(label) {
+ if ReservedLabel(label) && !kept[strings.ToLower(label)] {
return fmt.Errorf("label %q is a built-in tagging label; pick another name", label)
}
return nil
diff --git a/internal/app/inboxtag/custom_test.go b/internal/app/inboxtag/custom_test.go
index d441ec8a2..3a8f3e485 100644
--- a/internal/app/inboxtag/custom_test.go
+++ b/internal/app/inboxtag/custom_test.go
@@ -196,16 +196,16 @@ func TestValidateQuestionsRefusesBuiltInLabels(t *testing.T) {
for _, label := range []string{LabelNeedsReview, "needs review", "SALES PITCH", LabelGoneQuiet} {
q := laterMaybe(models.InboxTagQuestionAction{})
q.Label = label
- if err := ValidateQuestions([]models.InboxTagQuestion{q}); err == nil {
+ if err := ValidateQuestions([]models.InboxTagQuestion{q}, nil); err == nil {
t.Errorf("%q accepted", label)
}
}
c := roleQuestion()
c.Choices[0].Label = "interested"
- if err := ValidateQuestions([]models.InboxTagQuestion{c}); err == nil {
+ if err := ValidateQuestions([]models.InboxTagQuestion{c}, nil); err == nil {
t.Error("a built-in label accepted as a choice")
}
- if err := ValidateQuestions([]models.InboxTagQuestion{laterMaybe(models.InboxTagQuestionAction{}), roleQuestion()}); err != nil {
+ if err := ValidateQuestions([]models.InboxTagQuestion{laterMaybe(models.InboxTagQuestionAction{}), roleQuestion()}, nil); err != nil {
t.Fatalf("valid questions refused: %v", err)
}
}
@@ -386,3 +386,16 @@ func TestRecheckKeepsLabelsWhenNothingWasStored(t *testing.T) {
t.Fatalf("labels removed with no verdict stored: %v", cats.removed)
}
}
+
+// A label saved before the built-in taxonomy took its name keeps working; a
+// new question cannot take it.
+func TestValidateQuestionsKeepsASavedLabelThatBecameBuiltIn(t *testing.T) {
+ q := laterMaybe(models.InboxTagQuestionAction{})
+ q.Label = "Action required"
+ if err := ValidateQuestions([]models.InboxTagQuestion{q}, []models.InboxTagQuestion{q}); err != nil {
+ t.Fatalf("saved label refused: %v", err)
+ }
+ if err := ValidateQuestions([]models.InboxTagQuestion{q}, []models.InboxTagQuestion{laterMaybe(models.InboxTagQuestionAction{})}); err == nil {
+ t.Fatal("a new question took a built-in label")
+ }
+}
diff --git a/internal/app/inboxtag/service.go b/internal/app/inboxtag/service.go
index d44fb76da..8bf65a235 100644
--- a/internal/app/inboxtag/service.go
+++ b/internal/app/inboxtag/service.go
@@ -207,14 +207,14 @@ func (s *Service) Classify(ctx context.Context, m Message) (Decision, error) {
case facts.DeterministicKind == KindNotification && HasContent(state):
// A notice decided offline is still asked whether it needs acting on,
// so a failed payment is not filed away with the receipts. A failed
- // call leaves it untagged and in the inbox, never hidden unread.
+ // call files it as before, unasked, for --recheck-notifications.
if qs := NotificationQuestions(ws.questions, ws.actionRequired); len(qs) > 0 {
custom = ws.questions
state.Language = LanguageHint(ws.languages)
resp, err = s.asker.Ask(ctx, state, qs)
if err != nil {
- release()
- return Decision{}, err
+ log.Warn().Err(err).Str("message_id", m.MessageID).Msg("inbox tagging: action check failed; notification filed unasked")
+ resp = nil
}
}
}
@@ -453,7 +453,7 @@ type BackfillOptions struct {
// ErrNothingToAsk refuses a notification re-check for a workspace that asks
// automated mail nothing.
-var ErrNothingToAsk = errors.New("this workspace keeps no automated mail in the inbox: turn on \"Keep mail that needs action in the inbox\" or ask one of its questions of notifications first")
+var ErrNothingToAsk = errors.New("this workspace does not ask notifications whether they need action: turn on \"Keep mail that needs action in the inbox\" under Settings > Sending first")
// Backfill classifies historical inbound mail that has never been tagged.
//
@@ -485,8 +485,7 @@ func (s *Service) Backfill(ctx context.Context, orgID uuid.UUID, opts BackfillOp
case opts.RecheckColdInbound:
candidates, err = s.repo.ListColdInboundInCampaignThreads(ctx, orgID, opts.Since, opts.Limit)
case opts.RecheckNotifications:
- ws := s.workspace(ctx, orgID)
- if len(NotificationQuestions(ws.questions, ws.actionRequired)) == 0 {
+ if !s.workspace(ctx, orgID).actionRequired {
return p, ErrNothingToAsk
}
candidates, err = s.repo.ListUncheckedNotifications(ctx, orgID, opts.Since, opts.Limit)
@@ -544,6 +543,15 @@ func (s *Service) Backfill(ctx context.Context, orgID uuid.UUID, opts BackfillOp
}
}
if opts.RecheckNotifications {
+ // Nothing to read means nothing is asked, so the verdict would
+ // come back unasked and be offered again on every run.
+ if !HasContent(BuildState(c.Subject, c.BodyText, "", "")) {
+ p.Skipped++
+ if opts.OnProgress != nil {
+ opts.OnProgress(p, c.Subject)
+ }
+ continue
+ }
stale, err = s.repo.Reopen(ctx, orgID, c.MessageID, KindNotification)
if err != nil {
p.Failed++
diff --git a/internal/repository/inbox_tag_campaign_live_test.go b/internal/repository/inbox_tag_campaign_live_test.go
index 25d679690..1c3e01783 100644
--- a/internal/repository/inbox_tag_campaign_live_test.go
+++ b/internal/repository/inbox_tag_campaign_live_test.go
@@ -213,4 +213,14 @@ func TestLiveInboxTagUncheckedNotifications(t *testing.T) {
if !slices.Equal(ids, []string{""}) {
t.Fatalf("offered %v, want only the notice never asked", ids)
}
+
+ // Reopening it brings the message back to the inbox until it is judged again.
+ f.exec(`UPDATE unibox_emails SET automated = true WHERE message_id = '' AND email_id = $1`, f.mailbox)
+ if _, err := NewInboxTagRepository(f.pool).Reopen(ctx, f.org, "", "notification"); err != nil {
+ t.Fatalf("reopen: %v", err)
+ }
+ var automated bool
+ if err := f.pool.QueryRow(ctx, `SELECT automated FROM unibox_emails WHERE message_id = '' AND email_id = $1`, f.mailbox).Scan(&automated); err != nil || automated {
+ t.Fatalf("automated = %v (%v), want the reopened message back in the inbox", automated, err)
+ }
}
diff --git a/internal/repository/pg_inbox_tag.go b/internal/repository/pg_inbox_tag.go
index 2543aeeb4..2d81daa76 100644
--- a/internal/repository/pg_inbox_tag.go
+++ b/internal/repository/pg_inbox_tag.go
@@ -392,13 +392,24 @@ func (r *inboxTagRepository) listCandidates(ctx context.Context, filter string,
// Reopen drops one stored verdict of the given kind so the message can be
// classified again, and returns the labels it had written. Only a complete
-// verdict of that kind is touched.
+// verdict of that kind is touched. The message returns to the inbox with it,
+// so a re-classification that fails leaves it untagged and visible.
func (r *inboxTagRepository) Reopen(ctx context.Context, orgID uuid.UUID, messageID, kind string) ([]string, error) {
var labels []string
err := r.db.QueryRow(ctx, `
- DELETE FROM inbox_tag_results
- WHERE organization_id = $1 AND message_id = $2 AND status = 'complete' AND kind = $3
- RETURNING labels
+ WITH dropped AS (
+ DELETE FROM inbox_tag_results
+ WHERE organization_id = $1 AND message_id = $2 AND status = 'complete' AND kind = $3
+ RETURNING email_account_id, message_id, labels
+ ), shown AS (
+ UPDATE unibox_emails ue
+ SET automated = false
+ FROM dropped
+ WHERE ue.email_id = dropped.email_account_id
+ AND ue.message_id = dropped.message_id
+ AND ue.automated
+ )
+ SELECT labels FROM dropped
`, orgID, messageID, kind).Scan(&labels)
if errors.Is(err, pgx.ErrNoRows) {
return nil, nil
diff --git a/web/src/app/app/settings/sending/TaggingQuestions.tsx b/web/src/app/app/settings/sending/TaggingQuestions.tsx
index cf8e98a57..9ac89ad04 100644
--- a/web/src/app/app/settings/sending/TaggingQuestions.tsx
+++ b/web/src/app/app/settings/sending/TaggingQuestions.tsx
@@ -74,8 +74,9 @@ function actionSummary(a: InboxTagQuestionAction): string {
}
// problems mirrors the server's checks, so Save explains a refusal instead of
-// the autosave failing after the fact.
-function problems(q: InboxTagQuestion, taken: Set): string[] {
+// the autosave failing after the fact. kept holds the labels the question was
+// saved with, which stay valid even if a built-in label took the name since.
+function problems(q: InboxTagQuestion, taken: Set, kept: Set): string[] {
const out: string[] = [];
if (!q.question.trim()) out.push("Write the question.");
const seen = new Set();
@@ -87,7 +88,7 @@ function problems(q: InboxTagQuestion, taken: Set): string[] {
}
const key = name.toLowerCase();
if ([...name].length > INBOX_TAG_LABEL_MAX_LEN) out.push(`"${name}" is longer than ${INBOX_TAG_LABEL_MAX_LEN} characters.`);
- if (isAutomaticTag(name)) out.push(`"${name}" is a built-in label. Pick another name.`);
+ if (isAutomaticTag(name) && !kept.has(key)) out.push(`"${name}" is a built-in label. Pick another name.`);
else if (taken.has(key) || seen.has(key)) out.push(`"${name}" is already used by another question or option.`);
seen.add(key);
};
@@ -294,7 +295,8 @@ function QuestionForm({
const [q, setQ] = React.useState(initial);
const [tried, setTried] = React.useState(false);
const dirty = JSON.stringify(q) !== JSON.stringify(initial);
- const issues = problems(q, taken);
+ const kept = React.useMemo(() => new Set(labelsOf(initial).map((l) => l.toLowerCase())), [initial]);
+ const issues = problems(q, taken, kept);
const patch = (next: Partial) => setQ((prev) => ({ ...prev, ...next }));
const patchChoice = (i: number, next: Partial) =>