From e075a521bb5905a9e489e7856b4fc880f8e90798 Mon Sep 17 00:00:00 2001 From: Kevin Brown Date: Wed, 16 Sep 2026 21:05:19 -0600 Subject: [PATCH] Close every reply with an invitation to contact support A sender whose report did not come out had no way to know where to turn, and one whose report did had no easy way to suggest an improvement. Every reply now ends with: Questions, problems, or suggestions? Your feedback is welcome at
. The address is the new support_address key in email.yaml, not the code, since it is deployment-specific. Empty or absent leaves the line off, so a reply is unchanged until it is configured; an address with no "@" fails at load, as alert_recipient does. The line is added once, in deliver, after the attach-or-link branch, so it closes attachment replies and link replies alike, successes and failures, and comes after the links. Operator alerts go out through SendAlert and do not get it. It is set off by a blank line rather than the "-- " signature delimiter, which many mail clients dim or fold. Co-Authored-By: Claude Opus 5 (1M context) --- config/email.yaml.example | 5 ++ internal/mailgun/config.go | 17 ++++ internal/mailgun/outbound.go | 16 ++++ internal/mailgun/service.go | 1 + internal/mailgun/support_test.go | 128 +++++++++++++++++++++++++++++++ 5 files changed, 167 insertions(+) create mode 100644 internal/mailgun/support_test.go diff --git a/config/email.yaml.example b/config/email.yaml.example index 55599df..28bbf28 100644 --- a/config/email.yaml.example +++ b/config/email.yaml.example @@ -41,3 +41,8 @@ alert_recipient: "" # e.g. support@example.com # alert_cooldown_minutes: 15 # alert_max_per_hour: 10 # alert_max_per_day: 20 + +# Closes every reply to a sender, success or failure, with: +# Questions, problems, or suggestions? Your feedback is welcome at
. +# Empty or absent leaves the line off. +support_address: "" # e.g. support@example.com diff --git a/internal/mailgun/config.go b/internal/mailgun/config.go index 2e6bf48..1f0f588 100644 --- a/internal/mailgun/config.go +++ b/internal/mailgun/config.go @@ -39,6 +39,9 @@ type fileConfig struct { AlertCooldownMinutes int `yaml:"alert_cooldown_minutes"` AlertMaxPerHour int `yaml:"alert_max_per_hour"` AlertMaxPerDay int `yaml:"alert_max_per_day"` + + // SupportAddress is named at the bottom of every reply; see supportAddress. + SupportAddress string `yaml:"support_address"` } // credentials holds the Google OAuth secrets the sheets-link delivery mode @@ -103,6 +106,9 @@ func Load(root string, engine *app.App, logger *log.Logger) (*Service, error) { if s.alertTo, s.alertCfg, err = alertSettings(cfg); err != nil { return nil, err } + if s.support, err = supportAddress(cfg); err != nil { + return nil, err + } switch { case s.apiKey == "" && s.signKey == "" && s.domain == "" && s.from == "": @@ -145,6 +151,17 @@ func alertSettings(cfg fileConfig) (string, alert.Config, error) { }, nil } +// supportAddress reads the address a reply invites the sender to write to. It +// lives in email.yaml rather than the code because it is deployment-specific. +// Empty turns the footer off. +func supportAddress(cfg fileConfig) (string, error) { + address := strings.TrimSpace(cfg.SupportAddress) + if address != "" && !strings.Contains(address, "@") { + return "", fmt.Errorf("support_address %q is not an email address", address) + } + return address, nil +} + // parseDelivery normalizes the address -> delivery mode map and rejects any // value that is not a known mode. An unrecognized mode is a typo, and a typo // that silently left the route on attachment delivery would be invisible. diff --git a/internal/mailgun/outbound.go b/internal/mailgun/outbound.go index 637fbed..7c0ae7b 100644 --- a/internal/mailgun/outbound.go +++ b/internal/mailgun/outbound.go @@ -155,6 +155,8 @@ func (s *Service) deliver(ctx context.Context, sub store.EmailSubmission) error } } + text = withSupportFooter(text, s.support) + if err := s.send(ctx, sub.Sender, replySubject(sub.Subject), threadingID(sub.MessageID), text, attachments); err != nil { return err } @@ -388,6 +390,20 @@ func withLinks(text string, links, labels []string) string { return b.String() } +// withSupportFooter closes a reply with an invitation to write to the support +// address, whether the jobs succeeded or failed -- a sender whose report did not +// come out is the one who most needs to know where to turn. +// +// It is set off by a blank line, not the "-- " signature delimiter: many mail +// clients dim or fold what follows that delimiter, and this line is meant to be +// read. The address is left bare; clients make it a link on their own. +func withSupportFooter(text, address string) string { + if address == "" { + return text + } + return fmt.Sprintf("%s\n\nQuestions, problems, or suggestions? Your feedback is welcome at %s.", text, address) +} + // SendAlert sends one plain-text operator alert from REPLY_FROM, with its // subject as given: no "Re:", no threading, no attachments. It satisfies // alert.Mailer. It never reports its own failure: the alert channel is the diff --git a/internal/mailgun/service.go b/internal/mailgun/service.go index 48bbae5..4c09359 100644 --- a/internal/mailgun/service.go +++ b/internal/mailgun/service.go @@ -94,6 +94,7 @@ type Service struct { delivery map[string]string // recipient address -> delivery mode (absent = modeEmail) allowed map[string]bool // envelope senders permitted to submit (empty = all) maxBytes int64 // per-attachment size limit + support string // support_address from email.yaml, closing every reply; empty omits it sendBase string // Mailgun Send API base URL; overridable in tests client *http.Client // outbound HTTP client (carries the send timeout) diff --git a/internal/mailgun/support_test.go b/internal/mailgun/support_test.go new file mode 100644 index 0000000..50a22c8 --- /dev/null +++ b/internal/mailgun/support_test.go @@ -0,0 +1,128 @@ +package mailgun + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "filemill/internal/store" +) + +const testSupportLine = "Questions, problems, or suggestions? Your feedback is welcome at support@example.com." + +// supportFixture is a delivery fixture with a support address configured. +func supportFixture(t *testing.T) *deliveryFixture { + t.Helper() + f := newDeliveryFixture(t) + f.service.support = "support@example.com" + return f +} + +// An attachment reply ends with the support line, set off by a blank line. +func TestDeliverEndsAttachmentReplyWithSupportLine(t *testing.T) { + f := supportFixture(t) + f.addSubmission(t, 1, "excel@mill.test", "schedule.xlsx") + + text := deliverOne(t, f) + + if !strings.HasSuffix(text, "\n\n"+testSupportLine) { + t.Errorf("reply must end with the support line after a blank line; got %q", text) + } +} + +// In a link reply the support line comes after the links, so it closes the +// message rather than splitting the report from its link. +func TestDeliverPutsSupportLineAfterTheLinks(t *testing.T) { + f := supportFixture(t) + f.addSubmission(t, 1, "iwk@mill.test", "schedule.xlsx") + + text := deliverOne(t, f) + + link := strings.Index(text, "https://docs.google.com/spreadsheets/d/drive-file-1/edit") + support := strings.Index(text, testSupportLine) + if link < 0 || support < 0 || support < link { + t.Errorf("support line must follow the link; got %q", text) + } + if !strings.HasSuffix(text, testSupportLine) { + t.Errorf("reply must end with the support line; got %q", text) + } +} + +// A failed job is where a sender most needs to know where to turn. +func TestDeliverIncludesSupportLineWhenTheJobFailed(t *testing.T) { + f := supportFixture(t) + f.addSubmission(t, 1, "excel@mill.test") + f.engine.pending[0].Jobs[0].Job.Status = store.StatusFailed + f.engine.pending[0].Jobs[0].Job.Message = "no worker table found in the PDF" + + text := deliverOne(t, f) + + if !strings.Contains(text, "no worker table found in the PDF") { + t.Errorf("reply must still carry the failure message; got %q", text) + } + if !strings.HasSuffix(text, testSupportLine) { + t.Errorf("a failure reply must end with the support line; got %q", text) + } +} + +// With no support address the reply is exactly what it was before the footer +// existed. +func TestDeliverOmitsSupportLineWhenUnconfigured(t *testing.T) { + f := newDeliveryFixture(t) + f.addSubmission(t, 1, "excel@mill.test", "schedule.xlsx") + + text := deliverOne(t, f) + + if text != "schedule.pdf (workerlist_sheets): ok" { + t.Errorf("reply = %q, want the unadorned job line", text) + } +} + +// Alerts go to the operator, who is the support address; inviting them to +// write to themselves would be noise. +func TestSendAlertCarriesNoSupportLine(t *testing.T) { + var text string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := r.ParseMultipartForm(1 << 20); err != nil { + t.Errorf("parse form: %v", err) + return + } + text = r.FormValue("text") + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + s := sendTestService(server.URL, &http.Client{Timeout: time.Second}) + s.support = "support@example.com" + + if err := s.SendAlert(context.Background(), "ops@example.com", "[FileMill] test", "the detail"); err != nil { + t.Fatalf("SendAlert: %v", err) + } + + if text != "the detail" { + t.Errorf("alert text = %q, want it unchanged", text) + } +} + +func TestWithSupportFooter(t *testing.T) { + if got := withSupportFooter("body", ""); got != "body" { + t.Errorf("no address: got %q, want the body unchanged", got) + } + if got, want := withSupportFooter("body", "support@example.com"), "body\n\n"+testSupportLine; got != want { + t.Errorf("got %q, want %q", got, want) + } +} + +func TestSupportAddress(t *testing.T) { + if got, err := supportAddress(fileConfig{}); err != nil || got != "" { + t.Errorf("no key = %q, %v; want the footer off", got, err) + } + if got, err := supportAddress(fileConfig{SupportAddress: " support@example.com "}); err != nil || got != "support@example.com" { + t.Errorf("padded key = %q, %v; want it trimmed", got, err) + } + if _, err := supportAddress(fileConfig{SupportAddress: "support"}); err == nil { + t.Error("a support_address that is not an address must fail at load") + } +}