From 162b11bd7a2ddb42354a645e1584c483062bd807 Mon Sep 17 00:00:00 2001 From: Adhityaa Chandrasekar Date: Fri, 25 Oct 2019 01:04:27 -0700 Subject: [PATCH] api: do not batch email notifications Closes https://gitlab.com/commento/commento/issues/234 --- api/comment_new.go | 2 +- api/cron_email_notification.go | 66 ----------------------------- api/email.go | 1 - api/email_get.go | 8 ++-- api/email_notification.go | 70 +------------------------------ api/email_notification_new.go | 71 +++++++++++++++++++------------- api/email_notification_send.go | 63 ---------------------------- api/main.go | 2 - api/smtp_email_notification.go | 49 +++++++--------------- api/utils_misc.go | 10 ----- frontend/unsubscribe.html | 2 +- templates/email-notification.txt | 31 +++++++------- 12 files changed, 80 insertions(+), 295 deletions(-) delete mode 100644 api/cron_email_notification.go delete mode 100644 api/email_notification_send.go diff --git a/api/comment_new.go b/api/comment_new.go index c31695f..751f322 100644 --- a/api/comment_new.go +++ b/api/comment_new.go @@ -149,6 +149,6 @@ func commentNewHandler(w http.ResponseWriter, r *http.Request) { bodyMarshal(w, response{"success": true, "commentHex": commentHex, "state": state, "html": html}) if smtpConfigured { - go emailNotificationNew(d, path, commenterHex, commentHex, *x.ParentHex, state) + go emailNotificationNew(d, path, commenterHex, commentHex, html, *x.ParentHex, state) } } diff --git a/api/cron_email_notification.go b/api/cron_email_notification.go deleted file mode 100644 index 48d86ad..0000000 --- a/api/cron_email_notification.go +++ /dev/null @@ -1,66 +0,0 @@ -package main - -import ( - "time" -) - -func emailNotificationBegin() error { - go func() { - for { - statement := ` - SELECT email, sendModeratorNotifications, sendReplyNotifications - FROM emails - WHERE pendingEmails > 0 AND lastEmailNotificationDate < $1; - ` - rows, err := db.Query(statement, time.Now().UTC().Add(time.Duration(-10)*time.Minute)) - if err != nil { - logger.Errorf("cannot query domains: %v", err) - return - } - defer rows.Close() - - for rows.Next() { - var email string - var sendModeratorNotifications bool - var sendReplyNotifications bool - if err = rows.Scan(&email, &sendModeratorNotifications, &sendReplyNotifications); err != nil { - logger.Errorf("cannot scan email in cron job to send notifications: %v", err) - continue - } - - if _, ok := emailQueue[email]; !ok { - if err = emailNotificationPendingReset(email); err != nil { - logger.Errorf("error resetting pendingEmails: %v", err) - continue - } - } - - cont := true - kindListMap := map[string][]emailNotification{} - for cont { - select { - case e := <-emailQueue[email]: - if _, ok := kindListMap[e.Kind]; !ok { - kindListMap[e.Kind] = []emailNotification{} - } - - if (e.Kind == "reply" && sendReplyNotifications) || sendModeratorNotifications { - kindListMap[e.Kind] = append(kindListMap[e.Kind], e) - } - default: - cont = false - break - } - } - - for kind, list := range kindListMap { - go emailNotificationSend(email, kind, list) - } - } - - time.Sleep(10 * time.Minute) - } - }() - - return nil -} diff --git a/api/email.go b/api/email.go index f01debf..1fce250 100644 --- a/api/email.go +++ b/api/email.go @@ -8,7 +8,6 @@ type email struct { Email string `json:"email"` UnsubscribeSecretHex string `json:"unsubscribeSecretHex"` LastEmailNotificationDate time.Time `json:"lastEmailNotificationDate"` - PendingEmails int `json:"-"` SendReplyNotifications bool `json:"sendReplyNotifications"` SendModeratorNotifications bool `json:"sendModeratorNotifications"` } diff --git a/api/email_get.go b/api/email_get.go index ae4ac2f..c626a33 100644 --- a/api/email_get.go +++ b/api/email_get.go @@ -6,14 +6,14 @@ import ( func emailGet(em string) (email, error) { statement := ` - SELECT email, unsubscribeSecretHex, lastEmailNotificationDate, pendingEmails, sendReplyNotifications, sendModeratorNotifications + SELECT email, unsubscribeSecretHex, lastEmailNotificationDate, sendReplyNotifications, sendModeratorNotifications FROM emails WHERE email = $1; ` row := db.QueryRow(statement, em) e := email{} - if err := row.Scan(&e.Email, &e.UnsubscribeSecretHex, &e.LastEmailNotificationDate, &e.PendingEmails, &e.SendReplyNotifications, &e.SendModeratorNotifications); err != nil { + if err := row.Scan(&e.Email, &e.UnsubscribeSecretHex, &e.LastEmailNotificationDate, &e.SendReplyNotifications, &e.SendModeratorNotifications); err != nil { // TODO: is this the only error? return e, errorNoSuchEmail } @@ -23,14 +23,14 @@ func emailGet(em string) (email, error) { func emailGetByUnsubscribeSecretHex(unsubscribeSecretHex string) (email, error) { statement := ` - SELECT email, unsubscribeSecretHex, lastEmailNotificationDate, pendingEmails, sendReplyNotifications, sendModeratorNotifications + SELECT email, unsubscribeSecretHex, lastEmailNotificationDate, sendReplyNotifications, sendModeratorNotifications FROM emails WHERE unsubscribeSecretHex = $1; ` row := db.QueryRow(statement, unsubscribeSecretHex) e := email{} - if err := row.Scan(&e.Email, &e.UnsubscribeSecretHex, &e.LastEmailNotificationDate, &e.PendingEmails, &e.SendReplyNotifications, &e.SendModeratorNotifications); err != nil { + if err := row.Scan(&e.Email, &e.UnsubscribeSecretHex, &e.LastEmailNotificationDate, &e.SendReplyNotifications, &e.SendModeratorNotifications); err != nil { // TODO: is this the only error? return e, errorNoSuchUnsubscribeSecretHex } diff --git a/api/email_notification.go b/api/email_notification.go index 88892f0..451c3b1 100644 --- a/api/email_notification.go +++ b/api/email_notification.go @@ -1,8 +1,6 @@ package main -import ( - "time" -) +import () type emailNotification struct { Email string @@ -13,69 +11,3 @@ type emailNotification struct { CommentHex string Kind string } - -var emailQueue map[string](chan emailNotification) = map[string](chan emailNotification){} - -func emailNotificationPendingResetAll() error { - statement := ` - UPDATE emails - SET pendingEmails = 0; - ` - _, err := db.Exec(statement) - if err != nil { - logger.Errorf("cannot reset pendingEmails: %v", err) - return err - } - - return nil -} - -func emailNotificationPendingIncrement(email string) error { - statement := ` - UPDATE emails - SET pendingEmails = pendingEmails + 1 - WHERE email = $1; - ` - _, err := db.Exec(statement, email) - if err != nil { - logger.Errorf("cannot increment pendingEmails: %v", err) - return err - } - - return nil -} - -func emailNotificationPendingReset(email string) error { - statement := ` - UPDATE emails - SET pendingEmails = 0, lastEmailNotificationDate = $2 - WHERE email = $1; - ` - _, err := db.Exec(statement, email, time.Now().UTC()) - if err != nil { - logger.Errorf("cannot decrement pendingEmails: %v", err) - return err - } - - return nil -} - -func emailNotificationEnqueue(e emailNotification) error { - if err := emailNotificationPendingIncrement(e.Email); err != nil { - logger.Errorf("cannot increment pendingEmails when enqueueing: %v", err) - return err - } - - if _, ok := emailQueue[e.Email]; !ok { - // don't enqueue more than 10 emails as we won't send more than 10 comments - // in one email anyway - emailQueue[e.Email] = make(chan emailNotification, 10) - } - - select { - case emailQueue[e.Email] <- e: - default: - } - - return nil -} diff --git a/api/email_notification_new.go b/api/email_notification_new.go index fa0ffed..32f6020 100644 --- a/api/email_notification_new.go +++ b/api/email_notification_new.go @@ -2,13 +2,11 @@ package main import () -func emailNotificationModerator(d domain, path string, title string, commenterHex string, commentHex string, state string) { +func emailNotificationModerator(d domain, path string, title string, commenterHex string, commentHex string, html string, state string) { if d.EmailNotificationPolicy == "none" { return } - // We'll need to check again when we're sending in case the comment was - // approved midway anyway. if d.EmailNotificationPolicy == "pending-moderation" && state == "approved" { return } @@ -39,20 +37,37 @@ func emailNotificationModerator(d domain, path string, title string, commenterHe continue } - emailNotificationPendingIncrement(m.Email) - emailNotificationEnqueue(emailNotification{ - Email: m.Email, - CommenterName: commenterName, - Domain: d.Domain, - Path: path, - Title: title, - CommentHex: commentHex, - Kind: kind, - }) + e, err := emailGet(m.Email) + if err != nil { + // No such email. + continue + } + + if !e.SendModeratorNotifications { + continue + } + + statement := ` + SELECT name + FROM commenters + WHERE email = $1; + ` + row := db.QueryRow(statement, m.Email) + var name string + if err := row.Scan(&name); err != nil { + // The moderator has probably not created a commenter account. + // We should only send emails to people who signed up, so skip. + continue + } + + if err := smtpEmailNotification(m.Email, name, kind, d.Domain, path, commentHex, commenterName, title, html, e.UnsubscribeSecretHex); err != nil { + logger.Errorf("error sending email to %s: %v", m.Email) + continue + } } } -func emailNotificationReply(d domain, path string, title string, commenterHex string, commentHex string, parentHex string, state string) { +func emailNotificationReply(d domain, path string, title string, commenterHex string, commentHex string, html string, parentHex string, state string) { // No reply notifications for root comments. if parentHex == "root" { return @@ -105,20 +120,20 @@ func emailNotificationReply(d domain, path string, title string, commenterHex st commenterName = c.Name } - // We'll check if they want to receive reply notifications later at the time - // of sending. - emailNotificationEnqueue(emailNotification{ - Email: pc.Email, - CommenterName: commenterName, - Domain: d.Domain, - Path: path, - Title: title, - CommentHex: commentHex, - Kind: "reply", - }) + epc, err := emailGet(pc.Email) + if err != nil { + // No such email. + return + } + + if !epc.SendReplyNotifications { + return + } + + smtpEmailNotification(pc.Email, pc.Name, "reply", d.Domain, path, commentHex, commenterName, title, html, epc.UnsubscribeSecretHex) } -func emailNotificationNew(d domain, path string, commenterHex string, commentHex string, parentHex string, state string) { +func emailNotificationNew(d domain, path string, commenterHex string, commentHex string, html string, parentHex string, state string) { p, err := pageGet(d.Domain, path) if err != nil { logger.Errorf("cannot get page to send email notification: %v", err) @@ -134,6 +149,6 @@ func emailNotificationNew(d domain, path string, commenterHex string, commentHex } } - emailNotificationModerator(d, path, p.Title, commenterHex, commentHex, state) - emailNotificationReply(d, path, p.Title, commenterHex, commentHex, parentHex, state) + emailNotificationModerator(d, path, p.Title, commenterHex, commentHex, html, state) + emailNotificationReply(d, path, p.Title, commenterHex, commentHex, html, parentHex, state) } diff --git a/api/email_notification_send.go b/api/email_notification_send.go deleted file mode 100644 index 8f81891..0000000 --- a/api/email_notification_send.go +++ /dev/null @@ -1,63 +0,0 @@ -package main - -import ( - "html/template" -) - -func emailNotificationSend(em string, kind string, notifications []emailNotification) { - if len(notifications) == 0 { - return - } - - e, err := emailGet(em) - if err != nil { - logger.Errorf("cannot get email: %v", err) - return - } - - messages := []emailNotificationText{} - for _, notification := range notifications { - statement := ` - SELECT html - FROM comments - WHERE commentHex = $1; - ` - row := db.QueryRow(statement, notification.CommentHex) - - var html string - if err = row.Scan(&html); err != nil { - // the comment was deleted? - // TODO: is this the only error? - return - } - - messages = append(messages, emailNotificationText{ - emailNotification: notification, - Html: template.HTML(html), - }) - } - - statement := ` - SELECT name - FROM commenters - WHERE email = $1; - ` - row := db.QueryRow(statement, em) - - var name string - if err := row.Scan(&name); err != nil { - // The moderator has probably not created a commenter account. Let's just - // use their email as name. - name = nameFromEmail(em) - } - - if err := emailNotificationPendingReset(em); err != nil { - logger.Errorf("cannot reset after email notification: %v", err) - return - } - - if err := smtpEmailNotification(em, name, e.UnsubscribeSecretHex, messages, kind); err != nil { - logger.Errorf("cannot send email notification: %v", err) - return - } -} diff --git a/api/main.go b/api/main.go index faa0324..6fccb47 100644 --- a/api/main.go +++ b/api/main.go @@ -10,8 +10,6 @@ func main() { exitIfError(smtpTemplatesLoad()) exitIfError(oauthConfigure()) exitIfError(markdownRendererCreate()) - exitIfError(emailNotificationPendingResetAll()) - exitIfError(emailNotificationBegin()) exitIfError(sigintCleanupSetup()) exitIfError(versionCheckStart()) exitIfError(domainExportCleanupBegin()) diff --git a/api/smtp_email_notification.go b/api/smtp_email_notification.go index eb43b77..700d290 100644 --- a/api/smtp_email_notification.go +++ b/api/smtp_email_notification.go @@ -9,43 +9,19 @@ import ( tt "text/template" ) -type emailNotificationText struct { - emailNotification - Html ht.HTML -} - type emailNotificationPlugs struct { Origin string Kind string - Subject string UnsubscribeSecretHex string - Notifications []emailNotificationText + Domain string + Path string + CommentHex string + CommenterName string + Title string + Html ht.HTML } -func smtpEmailNotification(to string, toName string, unsubscribeSecretHex string, notifications []emailNotificationText, kind string) error { - var subject string - if kind == "reply" { - var verb string - if len(notifications) > 1 { - verb = "replies" - } else { - verb = "reply" - } - subject = fmt.Sprintf("%d new comment %s", len(notifications), verb) - } else { - var verb string - if len(notifications) > 1 { - verb = "comments" - } else { - verb = "comment" - } - if kind == "pending-moderation" { - subject = fmt.Sprintf("%d new %s pending moderation", len(notifications), verb) - } else { - subject = fmt.Sprintf("%d new %s on your website", len(notifications), verb) - } - } - +func smtpEmailNotification(to string, toName string, kind string, domain string, path string, commentHex string, commenterName string, title string, html string, unsubscribeSecretHex string) error { h, err := tt.New("header").Parse(`MIME-Version: 1.0 From: Commento <{{.FromAddress}}> To: {{.ToName}} <{{.ToAddress}}> @@ -53,9 +29,8 @@ Content-Type: text/html; charset=UTF-8 Subject: {{.Subject}} `) - var header bytes.Buffer - h.Execute(&header, &headerPlugs{FromAddress: os.Getenv("SMTP_FROM_ADDRESS"), ToAddress: to, ToName: toName, Subject: "[Commento] " + subject}) + h.Execute(&header, &headerPlugs{FromAddress: os.Getenv("SMTP_FROM_ADDRESS"), ToAddress: to, ToName: toName, Subject: "[Commento] " + title}) t, err := ht.ParseFiles(fmt.Sprintf("%s/templates/email-notification.txt", os.Getenv("STATIC"))) if err != nil { @@ -67,9 +42,13 @@ Subject: {{.Subject}} err = t.Execute(&body, &emailNotificationPlugs{ Origin: os.Getenv("ORIGIN"), Kind: kind, - Subject: subject, + Domain: domain, + Path: path, + CommentHex: commentHex, + CommenterName: commenterName, + Title: title, + Html: ht.HTML(html), UnsubscribeSecretHex: unsubscribeSecretHex, - Notifications: notifications, }) if err != nil { logger.Errorf("error generating templated HTML for email notification: %v", err) diff --git a/api/utils_misc.go b/api/utils_misc.go index 19415f5..e40f46d 100644 --- a/api/utils_misc.go +++ b/api/utils_misc.go @@ -10,16 +10,6 @@ func concat(a bytes.Buffer, b bytes.Buffer) []byte { return append(a.Bytes(), b.Bytes()...) } -func nameFromEmail(email string) string { - for i, c := range email { - if c == '@' { - return email[:i] - } - } - - return email -} - func exitIfError(err error) { if err != nil { fmt.Printf("fatal error: %v\n", err) diff --git a/frontend/unsubscribe.html b/frontend/unsubscribe.html index 537057c..7536f39 100644 --- a/frontend/unsubscribe.html +++ b/frontend/unsubscribe.html @@ -35,7 +35,7 @@
- When someone replies to your comment, you can choose to receive a notification. These emails will be batched and delayed (by 10 minutes) so that your inbox won't get overwhelmed. + When someone replies to your comment, you can choose to receive a email notification.
diff --git a/templates/email-notification.txt b/templates/email-notification.txt index 9f39957..58a83c6 100644 --- a/templates/email-notification.txt +++ b/templates/email-notification.txt @@ -2,7 +2,6 @@ - You have {{ .Subject }} -
You have {{ .Subject }}
+
+ {{ if eq .Kind "reply" }} + Unread Reply: {{ .Title }} + {{ end }} + {{ if eq .Kind "pending-moderation" }} + Pending Moderation: {{ .Title }} + {{ end }} +
- {{ with .Notifications }} - {{ range . }} -
- {{ if eq .Kind "pending-moderation" }} - Approve - {{ end }} - {{ if ne .Kind "reply" }} - Delete - {{ end }} - Context + {{ if eq .Kind "pending-moderation" }} + Approve + {{ end }} + {{ if ne .Kind "reply" }} + Delete + {{ end }} + Context
{{ .CommenterName }}
@@ -45,10 +48,8 @@
{{ .Html }} -
+
- {{ end }} - {{ end }}