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") + } +}