move email notification logic into Email struct

backend/app/notify/email.go

@@ -14,6 +14,7 @@ import (
 
 	log "github.com/go-pkgz/lgr"
 	"github.com/go-pkgz/repeater"
+	"github.com/hashicorp/go-multierror"
 	"github.com/pkg/errors"
 
 	"github.com/umputun/remark42/backend/app/templates"
@@ -164,35 +165,55 @@ func (e *Email) setTemplates() error {
 	return nil
 }
 
-// Send email about comment reply to Request.Email if it's set,
-// also sends email to site administrator if appropriate option is set.
+// Send email about comment reply to Request.Emails and Request.AdminEmails
+// if they're set.
 // Thread safe
 func (e *Email) Send(ctx context.Context, req Request) error {
-	if req.Email == "" {
-		// this means we can't send this request via Email
-		return nil
-	}
 	select {
 	case <-ctx.Done():
-		return errors.Errorf("sending message to %q aborted due to canceled context", req.Email)
+		return errors.Errorf("sending email messages about comment %q aborted due to canceled context", req.Comment.ID)
 	default:
 	}
 
-	if req.parent.User.ID == req.Comment.User.ID && !req.ForAdmin {
-		// don't send anything if if user replied to their own comment
-		return nil
-	}
-	log.Printf("[DEBUG] send notification via %s, comment id %s", e, req.Comment.ID)
-	msg, err := e.buildMessageFromRequest(req, req.ForAdmin)
-	if err != nil {
-		return err
+	result := new(multierror.Error)
+
+	// send user notifications
+	for _, email := range req.Emails {
+		email := email
+		log.Printf("[DEBUG] send user notification via %s, comment id %s", e, req.Comment.ID)
+		msg, err := e.buildMessageFromRequest(req, email, false)
+		if err != nil {
+			return err
+		}
+
+		err = repeater.NewDefault(5, time.Millisecond*250).Do(
+			ctx,
+			func() error {
+				return e.sendMessage(emailMessage{from: e.From, to: email, message: msg})
+			})
+
+		result = multierror.Append(errors.Wrapf(err, "problem sending user email notification to %q", email))
 	}
 
-	return repeater.NewDefault(5, time.Millisecond*250).Do(
-		ctx,
-		func() error {
-			return e.sendMessage(emailMessage{from: e.From, to: req.Email, message: msg})
-		})
+	// send admin notifications
+	for _, email := range req.AdminEmails {
+		email := email
+		log.Printf("[DEBUG] send admin notification via %s, comment id %s", e, req.Comment.ID)
+		msg, err := e.buildMessageFromRequest(req, email, true)
+		if err != nil {
+			return err
+		}
+
+		err = repeater.NewDefault(5, time.Millisecond*250).Do(
+			ctx,
+			func() error {
+				return e.sendMessage(emailMessage{from: e.From, to: email, message: msg})
+			})
+
+		result = multierror.Append(errors.Wrapf(err, "problem sending admin email notification to %q", email))
+	}
+
+	return result.ErrorOrNil()
 }
 
 // SendVerification email verification VerificationRequest.Email if it's set.
@@ -204,7 +225,7 @@ func (e *Email) SendVerification(ctx context.Context, req VerificationRequest) e
 	}
 	select {
 	case <-ctx.Done():
-		return errors.Errorf("sending message to %q aborted due to canceled context", req.Email)
+		return errors.Errorf("sending message to %q aborted due to canceled context", req.User)
 	default:
 	}
 
@@ -239,16 +260,16 @@ func (e *Email) buildVerificationMessage(user, email, token, site string) (strin
 }
 
 // buildMessageFromRequest generates email message based on Request using e.MsgTemplate
-func (e *Email) buildMessageFromRequest(req Request, forAdmin bool) (string, error) {
+func (e *Email) buildMessageFromRequest(req Request, email string, forAdmin bool) (string, error) {
 	subject := "New reply to your comment"
 	if forAdmin {
 		subject = "New comment to your site"
 	}
 	if req.Comment.PostTitle != "" {
-		subject += fmt.Sprintf(" for \"%s\"", req.Comment.PostTitle)
+		subject += fmt.Sprintf(" for %q", req.Comment.PostTitle)
 	}
 
-	token, err := e.TokenGenFn(req.parent.User.ID, req.Email, req.Comment.Locator.SiteID)
+	token, err := e.TokenGenFn(req.parent.User.ID, email, req.Comment.Locator.SiteID)
 	if err != nil {
 		return "", errors.Wrapf(err, "error creating token for unsubscribe link")
 	}
@@ -266,7 +287,7 @@ func (e *Email) buildMessageFromRequest(req Request, forAdmin bool) (string, err
 		CommentLink:     commentURLPrefix + req.Comment.ID,
 		CommentDate:     req.Comment.Timestamp,
 		PostTitle:       req.Comment.PostTitle,
-		Email:           req.Email,
+		Email:           email,
 		UnsubscribeLink: unsubscribeLink,
 		ForAdmin:        forAdmin,
 	}
@@ -282,7 +303,7 @@ func (e *Email) buildMessageFromRequest(req Request, forAdmin bool) (string, err
 	if err != nil {
 		return "", errors.Wrapf(err, "error executing template to build comment reply message")
 	}
-	return e.buildMessage(subject, msg.String(), req.Email, "text/html", unsubscribeLink)
+	return e.buildMessage(subject, msg.String(), email, "text/html", unsubscribeLink)
 }
 
 // buildMessage generates email message to send using net/smtp.Data()

backend/app/notify/email_test.go

@@ -113,16 +113,16 @@ func TestEmailSendErrors(t *testing.T) {
 
 	e.msgTmpl, err = template.New("test").Parse("{{.Test}}")
 	assert.NoError(t, err)
-	assert.EqualError(t, e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "test"}}, Email: "bad@example.org"}),
+	assert.EqualError(t, e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "test"}}, Emails: []string{"bad@example.org"}}),
 		"error executing template to build comment reply message: template: test:1:2: executing \"test\" at <.Test>: can't evaluate field Test in type notify.msgTmplData")
 
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel()
-	assert.EqualError(t, e.Send(ctx, Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "test"}}, Email: "bad@example.org"}),
-		"sending message to \"bad@example.org\" aborted due to canceled context")
+	assert.EqualError(t, e.Send(ctx, Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "test"}}, Emails: []string{"bad@example.org"}}),
+		"sending email messages about comment \"999\" aborted due to canceled context")
 
 	e.smtp = &fakeTestSMTP{}
-	assert.EqualError(t, e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "error"}}, Email: "bad@example.org"}),
+	assert.EqualError(t, e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "error"}}, Emails: []string{"bad@example.org"}}),
 		"error creating token for unsubscribe link: token generation error")
 }
 
@@ -136,10 +136,7 @@ func TestEmailSend_ExitConditions(t *testing.T) {
 	// prevent triggering e.autoFlush creation
 	emptyRequest := Request{Comment: store.Comment{ID: "999"}}
 	assert.NoError(t, email.Send(context.Background(), emptyRequest),
-		"Message without parent comment User.Email is not sent and returns nil")
-	requestWithEqualUsersWithEmails := Request{Comment: store.Comment{ID: "999"}, Email: "good_example@example.org"}
-	assert.NoError(t, email.Send(context.Background(), requestWithEqualUsersWithEmails),
-		"Message with parent comment User equals comment User is not sent and returns nil")
+		"Message without Emails and AdminEmails is not sent and returns nil")
 }
 
 func TestEmailSendClientError(t *testing.T) {
@@ -184,6 +181,15 @@ func TestEmailSendClientError(t *testing.T) {
 		"e.send called without smtpClient set returns error")
 }
 
+func TestEmail_DefaultTemplates(t *testing.T) {
+	email, err := NewEmail(EmailParams{}, SMTPParams{})
+	assert.Error(t, err)
+	assert.Nil(t, email)
+	email, err = NewEmail(EmailParams{VerificationTemplatePath: "testdata/verification.html.tmpl"}, SMTPParams{})
+	assert.Error(t, err)
+	assert.Nil(t, email)
+}
+
 func TestEmail_Send(t *testing.T) {
 	email, err := NewEmail(EmailParams{
 		From:                     "from@example.org",
@@ -199,14 +205,14 @@ func TestEmail_Send(t *testing.T) {
 	req := Request{
 		Comment: store.Comment{ID: "999", User: store.User{ID: "1", Name: "test_user"}, ParentID: "1", PostTitle: "test_title"},
 		parent:  store.Comment{ID: "1", User: store.User{ID: "999", Name: "parent_user"}},
-		Email:   "test@example.org",
+		Emails:  []string{"test@example.org"},
 	}
 	assert.NoError(t, email.Send(context.TODO(), req))
 	assert.Equal(t, "from@example.org", fakeSMTP.readMail())
 	assert.Equal(t, 1, fakeSMTP.readQuitCount())
 	assert.Equal(t, "test@example.org", fakeSMTP.readRcpt())
 	// test buildMessageFromRequest separately for message text
-	res, err := email.buildMessageFromRequest(req, req.ForAdmin)
+	res, err := email.buildMessageFromRequest(req, req.Emails[0], false)
 	assert.NoError(t, err)
 	assert.Contains(t, res, `From: from@example.org
 To: test@example.org
@@ -218,14 +224,17 @@ List-Unsubscribe-Post: List-Unsubscribe=One-Click
 List-Unsubscribe: <https://remark42.com/api/v1/email/unsubscribe?site=&tkn=token>
 Date: `)
 
-	// send email to admin without parent set
+	// send email to both user and admin, without parent set
 	req = Request{
-		Comment:  store.Comment{ID: "999", User: store.User{ID: "1", Name: "test_user"}, PostTitle: "test_title"},
-		Email:    "admin@example.org",
-		ForAdmin: true,
+		Comment:     store.Comment{ID: "999", User: store.User{ID: "1", Name: "test_user"}, PostTitle: "test_title"},
+		Emails:      []string{"test@example.org"},
+		AdminEmails: []string{"admin@example.org"},
 	}
 	assert.NoError(t, email.Send(context.TODO(), req))
-	res, err = email.buildMessageFromRequest(req, req.ForAdmin)
+	assert.Equal(t, "from@example.org", fakeSMTP.readMail())
+	assert.Equal(t, 3, fakeSMTP.readQuitCount(), "plus two emails: one for user and one for admin")
+	assert.Equal(t, "admin@example.org", fakeSMTP.readRcpt())
+	res, err = email.buildMessageFromRequest(req, req.AdminEmails[0], true)
 	assert.NoError(t, err)
 	assert.Contains(t, res, `From: from@example.org
 To: admin@example.org
@@ -268,7 +277,7 @@ func TestEmail_SendVerification(t *testing.T) {
 	// VerificationRequest with canceled context
 	ctx, cancel := context.WithCancel(context.TODO())
 	cancel()
-	assert.EqualError(t, email.SendVerification(ctx, req), "sending message to \"test@example.org\" aborted due to canceled context")
+	assert.EqualError(t, email.SendVerification(ctx, req), "sending message to \"test_username\" aborted due to canceled context")
 
 	// test buildVerificationMessage separately for message text
 	res, err := email.buildVerificationMessage(req.User, req.Email, req.Token, req.SiteID)

backend/app/notify/notify.go

@@ -37,12 +37,12 @@ type Store interface {
 	GetUserEmail(siteID string, userID string) (string, error)
 }
 
-// Request notification either about comment or about particular user verification
+// Request notification either about comment
 type Request struct {
-	Comment  store.Comment // if set sent notifications about new comment
-	parent   store.Comment // fetched only in case Comment is set
-	Email    string        // if set (also) send email
-	ForAdmin bool          // if set, message supposed to be sent to administrator
+	Comment     store.Comment
+	parent      store.Comment
+	Emails      []string
+	AdminEmails []string
 }
 
 // VerificationRequest notification for user
@@ -82,18 +82,10 @@ func (s *Service) Submit(req Request) {
 	if len(s.destinations) == 0 || atomic.LoadUint32(&s.closed) != 0 {
 		return
 	}
-	// parent comment is fetched only if comment is present in the Request
 	if s.dataService != nil && req.Comment.ParentID != "" {
 		if p, err := s.dataService.Get(req.Comment.Locator, req.Comment.ParentID, store.User{}); err == nil {
 			req.parent = p
-			// user notification, should fetch email for it.
-			// administrator notification comes with pre-set email
-			if req.Email == "" {
-				req.Email, err = s.dataService.GetUserEmail(req.Comment.Locator.SiteID, p.User.ID)
-				if err != nil {
-					log.Printf("[WARN] can't read email for %s, %v", p.User.ID, err)
-				}
-			}
+			req.Emails = s.getNotificationEmails(req, p)
 		}
 	}
 	select {
@@ -103,6 +95,22 @@ func (s *Service) Submit(req Request) {
 	}
 }
 
+// getNotificationEmails returns list of emails for notifications for provided comment.
+// Emails is not added to the returned list in case original message is from the same user as the notification receiver.
+func (s *Service) getNotificationEmails(req Request, notifyComment store.Comment) (result []string) {
+	// add current user email only if the user is not the one who wrote the original comment
+	if notifyComment.User.ID != req.Comment.User.ID {
+		email, err := s.dataService.GetUserEmail(req.Comment.Locator.SiteID, notifyComment.User.ID)
+		if err != nil {
+			log.Printf("[WARN] can't read email for %s, %v", notifyComment.User.ID, err)
+		}
+		if email != "" {
+			result = append(result, email)
+		}
+	}
+	return result
+}
+
 // SubmitVerification to internal channel if not busy, drop if can't send
 func (s *Service) SubmitVerification(req VerificationRequest) {
 	if len(s.destinations) == 0 || atomic.LoadUint32(&s.closed) != 0 {

backend/app/notify/notify_test.go

@@ -139,6 +139,65 @@ func TestService_WithParent(t *testing.T) {
 	assert.Equal(t, "", destRes[1].parent.ID)
 }
 
+func TestService_EmailRetrieval(t *testing.T) {
+	dest := &MockDest{id: 1}
+	dataStore := &mockStore{data: map[string]store.Comment{}, emailData: map[string]string{}}
+
+	dataStore.data["p1"] = store.Comment{ID: "p1", User: store.User{ID: "u1"}}
+	dataStore.data["p2"] = store.Comment{ID: "p2", ParentID: "p1", User: store.User{ID: "u1"}}
+	dataStore.data["p3"] = store.Comment{ID: "p3", ParentID: "p1", User: store.User{ID: "u2"}}
+	dataStore.data["p4"] = store.Comment{ID: "p4", ParentID: "p3", User: store.User{ID: "u1"}}
+	dataStore.emailData["u1"] = "u1@example.com"
+
+	s := NewService(dataStore, 1, dest)
+	assert.NotNil(t, s)
+
+	// one comment, one notification
+	s.Submit(Request{Comment: dataStore.data["p1"]})
+	time.Sleep(time.Millisecond * 110)
+
+	destRes := dest.Get()
+	require.Equal(t, 1, len(destRes), "one comment notified")
+	assert.Equal(t, "p1", destRes[0].Comment.ID)
+	assert.Empty(t, destRes[0].parent)
+	assert.Empty(t, destRes[0].Emails)
+
+	// reply to the first comment, same comment as one in original comment
+	s.Submit(Request{Comment: dataStore.data["p2"]})
+	time.Sleep(time.Millisecond * 110)
+
+	destRes = dest.Get()
+	require.Equal(t, 2, len(destRes), "two comment notified")
+	assert.Equal(t, "p2", destRes[1].Comment.ID)
+	assert.Equal(t, "p1", destRes[1].parent.ID)
+	assert.Equal(t, "u1", destRes[1].parent.User.ID)
+	assert.Empty(t, destRes[1].Emails, "u1 is not notified they are the one who left the comment")
+
+	// another reply to the first comment, another user
+	s.Submit(Request{Comment: dataStore.data["p3"]})
+	time.Sleep(time.Millisecond * 110)
+
+	destRes = dest.Get()
+	require.Equal(t, 3, len(destRes), "three comment notified")
+	assert.Equal(t, "p3", destRes[2].Comment.ID)
+	assert.Equal(t, "p1", destRes[2].parent.ID)
+	assert.Equal(t, "u1", destRes[2].parent.User.ID)
+	assert.Equal(t, []string{"u1@example.com"}, destRes[2].Emails)
+
+	// reply to the last comment by another user, should trigger email retrieval error
+	s.Submit(Request{Comment: dataStore.data["p4"]})
+	time.Sleep(time.Millisecond * 110)
+
+	destRes = dest.Get()
+	require.Equal(t, 4, len(destRes), "four comment notified")
+	assert.Equal(t, "p4", destRes[3].Comment.ID)
+	assert.Equal(t, "p3", destRes[3].parent.ID)
+	assert.Equal(t, "u2", destRes[3].parent.User.ID)
+	assert.Empty(t, destRes[3].Emails, "no email can be retrieved for u2")
+
+	s.Close()
+}
+
 func TestService_Nop(t *testing.T) {
 	s := NopService
 	s.Submit(Request{Comment: store.Comment{}})
@@ -146,7 +205,10 @@ func TestService_Nop(t *testing.T) {
 	assert.Equal(t, uint32(1), atomic.LoadUint32(&s.closed))
 }
 
-type mockStore struct{ data map[string]store.Comment }
+type mockStore struct {
+	data      map[string]store.Comment
+	emailData map[string]string
+}
 
 func (m mockStore) Get(_ store.Locator, id string, _ store.User) (store.Comment, error) {
 	res, ok := m.data[id]
@@ -156,6 +218,10 @@ func (m mockStore) Get(_ store.Locator, id string, _ store.User) (store.Comment,
 	return res, nil
 }
 
-func (m mockStore) GetUserEmail(_, _ string) (string, error) {
-	return "", errors.New("no such user")
+func (m mockStore) GetUserEmail(_, userID string) (string, error) {
+	email, ok := m.emailData[userID]
+	if !ok {
+		return "", errors.New("no such user")
+	}
+	return email, nil
 }

backend/app/notify/telegram.go

@@ -85,11 +85,6 @@ func NewTelegram(token, channelID string, timeout time.Duration, api string) (*T
 
 // Send to telegram channel
 func (t *Telegram) Send(ctx context.Context, req Request) error {
-	if req.ForAdmin {
-		// request for administrator received, do nothing with it
-		// as we already sent message on request without this flag set
-		return nil
-	}
 	client := http.Client{Timeout: telegramTimeOut}
 	log.Printf("[DEBUG] send telegram notification to %s, comment id %s", t.channelID, req.Comment.ID)
 

backend/app/rest/api/rest_private.go

@@ -120,12 +120,12 @@ func (s *private) createCommentCtrl(w http.ResponseWriter, r *http.Request) {
 		Scopes(comment.Locator.URL, lastCommentsScope, comment.User.ID, comment.Locator.SiteID))
 
 	if s.notifyService != nil {
-		// user notification
-		s.notifyService.Submit(notify.Request{Comment: finalComment})
-		// admin notification
-		for _, adminEmail := range s.adminEmail {
-			s.notifyService.Submit(notify.Request{Comment: finalComment, Email: adminEmail, ForAdmin: true})
-		}
+		s.notifyService.Submit(
+			notify.Request{
+				Comment:     finalComment,
+				AdminEmails: s.adminEmail,
+			},
+		)
 	}
 
 	log.Printf("[DEBUG] created commend %+v", finalComment)

backend/app/rest/api/rest_private_test.go

@@ -618,9 +618,9 @@ func TestRest_EmailNotification(t *testing.T) {
 	require.NoError(t, render.DecodeJSON(strings.NewReader(string(body)), &parentComment))
 	// wait for mock notification Submit to kick off
 	time.Sleep(time.Millisecond * 30)
-	require.Equal(t, 2, len(mockDestination.Get()))
-	assert.Empty(t, mockDestination.Get()[0].Email)
-	assert.Equal(t, "admin@example.org", mockDestination.Get()[1].Email)
+	require.Equal(t, 1, len(mockDestination.Get()))
+	assert.Empty(t, mockDestination.Get()[0].Emails)
+	assert.Equal(t, []string{"admin@example.org"}, mockDestination.Get()[0].AdminEmails)
 
 	// create child comment from another user, email notification only to admin expected
 	req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader(fmt.Sprintf(
@@ -630,7 +630,7 @@ func TestRest_EmailNotification(t *testing.T) {
 	"locator":{"url": "https://radio-t.com/blah1",
 	"site": "remark42"}}`, parentComment.ID)))
 	assert.NoError(t, err)
-	req.Header.Add("X-JWT", devToken)
+	req.Header.Add("X-JWT", anonToken)
 	resp, err = client.Do(req)
 	assert.NoError(t, err)
 	body, err = ioutil.ReadAll(resp.Body)
@@ -639,9 +639,9 @@ func TestRest_EmailNotification(t *testing.T) {
 	require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
 	// wait for mock notification Submit to kick off
 	time.Sleep(time.Millisecond * 30)
-	require.Equal(t, 4, len(mockDestination.Get()))
-	assert.Empty(t, mockDestination.Get()[2].Email)
-	assert.Equal(t, "admin@example.org", mockDestination.Get()[3].Email)
+	require.Equal(t, 2, len(mockDestination.Get()))
+	assert.Empty(t, mockDestination.Get()[1].Emails)
+	assert.Equal(t, []string{"admin@example.org"}, mockDestination.Get()[1].AdminEmails)
 
 	// send confirmation token for email
 	req, err = http.NewRequest(http.MethodPost, ts.URL+"/api/v1/email/subscribe?site=remark42&address=good@example.com", nil)
@@ -694,7 +694,7 @@ func TestRest_EmailNotification(t *testing.T) {
 	"locator":{"url": "https://radio-t.com/blah1",
 	"site": "remark42"}}`, parentComment.ID)))
 	assert.NoError(t, err)
-	req.Header.Add("X-JWT", devToken)
+	req.Header.Add("X-JWT", anonToken)
 	resp, err = client.Do(req)
 	assert.NoError(t, err)
 	body, err = ioutil.ReadAll(resp.Body)
@@ -703,9 +703,9 @@ func TestRest_EmailNotification(t *testing.T) {
 	require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
 	// wait for mock notification Submit to kick off
 	time.Sleep(time.Millisecond * 30)
-	require.Equal(t, 6, len(mockDestination.Get()))
-	assert.Equal(t, "good@example.com", mockDestination.Get()[4].Email)
-	assert.Equal(t, "admin@example.org", mockDestination.Get()[5].Email)
+	require.Equal(t, 3, len(mockDestination.Get()))
+	assert.Equal(t, []string{"good@example.com"}, mockDestination.Get()[2].Emails)
+	assert.Equal(t, []string{"admin@example.org"}, mockDestination.Get()[2].AdminEmails)
 
 	// delete user's email
 	req, err = http.NewRequest(http.MethodDelete, ts.URL+"/api/v1/email?site=remark42", nil)
@@ -734,8 +734,9 @@ func TestRest_EmailNotification(t *testing.T) {
 	require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
 	// wait for mock notification Submit to kick off
 	time.Sleep(time.Millisecond * 30)
-	require.Equal(t, 8, len(mockDestination.Get()))
-	assert.Empty(t, mockDestination.Get()[6].Email)
+	require.Equal(t, 4, len(mockDestination.Get()))
+	assert.Empty(t, mockDestination.Get()[3].Emails)
+	assert.Equal(t, []string{"admin@example.org"}, mockDestination.Get()[3].AdminEmails)
 }
 
 func TestRest_UserAllData(t *testing.T) {