api: do not batch email notifications

Closes https://gitlab.com/commento/commento/issues/234

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

api/cron_email_notification.go

@@ -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
-}

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"`
 }

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
 	}

api/email_notification.go

@@ -1,8 +1,6 @@
 package main
 
-import (
+import ()
-	"time"
-)
 
 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
-}

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)
+		e, err := emailGet(m.Email)
-		emailNotificationEnqueue(emailNotification{
+		if err != nil {
-			Email:         m.Email,
+			// No such email.
-			CommenterName: commenterName,
+			continue
-			Domain:        d.Domain,
+		}
-			Path:          path,
+
-			Title:         title,
+		if !e.SendModeratorNotifications {
-			CommentHex:    commentHex,
+			continue
-			Kind:          kind,
+		}
-		})
+
+		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
+	epc, err := emailGet(pc.Email)
-	// of sending.
+	if err != nil {
-	emailNotificationEnqueue(emailNotification{
+		// No such email.
-		Email:         pc.Email,
+		return
-		CommenterName: commenterName,
-		Domain:        d.Domain,
-		Path:          path,
-		Title:         title,
-		CommentHex:    commentHex,
-		Kind:          "reply",
-	})
 	}
 
-func emailNotificationNew(d domain, path string, commenterHex string, commentHex string, parentHex string, state string) {
+	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, 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)
+	emailNotificationModerator(d, path, p.Title, commenterHex, commentHex, html, state)
-	emailNotificationReply(d, path, p.Title, commenterHex, commentHex, parentHex, state)
+	emailNotificationReply(d, path, p.Title, commenterHex, commentHex, html, parentHex, state)
 }

api/email_notification_send.go

@@ -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
-	}
-}

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())

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
-func smtpEmailNotification(to string, toName string, unsubscribeSecretHex string, notifications []emailNotificationText, kind string) error {
+	CommenterName        string
-	var subject string
+	Title                string
-	if kind == "reply" {
+	Html                 ht.HTML
-		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)

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)

frontend/unsubscribe.html

@@ -35,7 +35,7 @@
           <input type="checkbox" id="reply" value="reply">
           <label for="reply">Email me when someone replies to my comment</label>
           <div class="pitch">
-            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.
           </div>
         </div>
         <div class="msg"></div>

templates/email-notification.txt

@@ -2,7 +2,6 @@
 <html>
   <head>
     <meta name="viewport" content="user-scalable=no,initial-scale=1">
-    <title>You have {{ .Subject }}</title>
     <style type="text/css">
       @media only screen and (max-width:600px) {
           .logo {
@@ -22,19 +21,23 @@
     </style>
   </head>
   <body class="content" style="font-size:14px;background:white;font-family:sans-serif;padding:0px;margin:0px;">
-    <div class="h1" style="font-weight:bold;text-align:center;margin-top:12px;padding:8px;font-size:24px;">You have {{ .Subject }}</div>
+    <div class="h1" style="font-weight:bold;text-align:center;margin-top:12px;padding:8px;font-size:18px;">
+    {{ if eq .Kind "reply" }}
+      Unread Reply: {{ .Title }}
+    {{ end }}
+    {{ if eq .Kind "pending-moderation" }}
+      Pending Moderation: {{ .Title }}
+    {{ end }}
+    </div>
     <div class="comments-container" style="display:flex;justify-content:center;">
       <div class="comments" style="max-width:600px;width:calc(100% - 20px);margin-top:16px;border-top:1px solid #eee;">
-        {{ with .Notifications }}
-          {{ range . }}
-
         <div class="comment" style="border-radius:2px;width:calc(100% - 32px);padding:16px;margin:8px 0px 8px 0px;border-bottom:1px solid #eee;">
           <div class="options" style="float:right;">
           {{ if eq .Kind "pending-moderation" }}
-                  <a href="{{ $.Origin }}/api/email/moderate?commentHex={{ .CommentHex }}&action=approve&unsubscribeSecretHex={{ $.UnsubscribeSecretHex }}" target="_black" class="option green" style="padding-right:5px;text-transform:uppercase;font-size:12px;font-weight:bold;text-decoration:none;color:#2f9e44;">Approve</a>
+            <a href="{{ .Origin }}/api/email/moderate?commentHex={{ .CommentHex }}&action=approve&unsubscribeSecretHex={{ .UnsubscribeSecretHex }}" target="_black" class="option green" style="padding-right:5px;text-transform:uppercase;font-size:12px;font-weight:bold;text-decoration:none;color:#2f9e44;">Approve</a>
           {{ end }}
           {{ if ne .Kind "reply" }}
-                  <a href="{{ $.Origin }}/api/email/moderate?commentHex={{ .CommentHex }}&action=delete&unsubscribeSecretHex={{ $.UnsubscribeSecretHex }}" target="_black" class="option red" style="padding-right:5px;text-transform:uppercase;font-size:12px;font-weight:bold;text-decoration:none;color:#f03e3e;">Delete</a>
+            <a href="{{ .Origin }}/api/email/moderate?commentHex={{ .CommentHex }}&action=delete&unsubscribeSecretHex={{ .UnsubscribeSecretHex }}" target="_black" class="option red" style="padding-right:5px;text-transform:uppercase;font-size:12px;font-weight:bold;text-decoration:none;color:#f03e3e;">Delete</a>
           {{ end }}
             <a href="http://{{ .Domain }}{{ .Path }}#commento-{{ .CommentHex }}" class="option gray" style="padding-right:5px;text-transform:uppercase;font-size:12px;font-weight:bold;text-decoration:none;color:#495057;">Context</a>
           </div>
@@ -47,8 +50,6 @@
             {{ .Html }}
           </div>
         </div>
-        {{ end }}
-        {{ end }}
 
         <div class="footer" style="width:100%;margin-top:16px;">
           <a href="https://commento.io" class="logo" style="float:right;font-weight:bold;color:#868e96;font-size:13px;text-decoration:none;">Powered by Commento</a>