Rewrite delivery of issue and comment mails (#9009)

* Mail issue subscribers, rework the function

* Simplify a little more

* Fix unused variable

* Refactor mail delivery to avoid heavy load on server

* Avoid splitting into too many goroutines

* Fix comments and optimize GetMaileableUsersByIDs()

* Fix return on errors

Author: guillep2k

models/issue.go

@@ -1219,6 +1219,19 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) {
 	return issues, nil
 }
 
+// GetParticipantsIDsByIssueID returns the IDs of all users who participated in comments of an issue,
+// but skips joining with `user` for performance reasons.
+// User permissions must be verified elsewhere if required.
+func GetParticipantsIDsByIssueID(issueID int64) ([]int64, error) {
+	userIDs := make([]int64, 0, 5)
+	return userIDs, x.Table("comment").
+		Cols("poster_id").
+		Where("issue_id = ?", issueID).
+		And("type in (?,?,?)", CommentTypeComment, CommentTypeCode, CommentTypeReview).
+		Distinct("poster_id").
+		Find(&userIDs)
+}
+
 // GetParticipantsByIssueID returns all users who are participated in comments of an issue.
 func GetParticipantsByIssueID(issueID int64) ([]*User, error) {
 	return getParticipantsByIssueID(x, issueID)

models/issue_assignees.go

@@ -41,6 +41,18 @@ func (issue *Issue) loadAssignees(e Engine) (err error) {
 	return
 }
 
+// GetAssigneeIDsByIssue returns the IDs of users assigned to an issue
+// but skips joining with `user` for performance reasons.
+// User permissions must be verified elsewhere if required.
+func GetAssigneeIDsByIssue(issueID int64) ([]int64, error) {
+	userIDs := make([]int64, 0, 5)
+	return userIDs, x.Table("issue_assignees").
+		Cols("assignee_id").
+		Where("issue_id = ?", issueID).
+		Distinct("assignee_id").
+		Find(&userIDs)
+}
+
 // GetAssigneesByIssue returns everyone assigned to that issue
 func GetAssigneesByIssue(issue *Issue) (assignees []*User, err error) {
 	return getAssigneesByIssue(x, issue)

models/issue_watch.go

@@ -60,6 +60,18 @@ func getIssueWatch(e Engine, userID, issueID int64) (iw *IssueWatch, exists bool
 	return
 }
 
+// GetIssueWatchersIDs returns IDs of subscribers to a given issue id
+// but avoids joining with `user` for performance reasons
+// User permissions must be verified elsewhere if required
+func GetIssueWatchersIDs(issueID int64) ([]int64, error) {
+	ids := make([]int64, 0, 64)
+	return ids, x.Table("issue_watch").
+		Where("issue_id=?", issueID).
+		And("is_watching = ?", true).
+		Select("user_id").
+		Find(&ids)
+}
+
 // GetIssueWatchers returns watchers/unwatchers of a given issue
 func GetIssueWatchers(issueID int64) (IssueWatchList, error) {
 	return getIssueWatchers(x, issueID)

models/repo_watch.go

@@ -140,6 +140,18 @@ func GetWatchers(repoID int64) ([]*Watch, error) {
 	return getWatchers(x, repoID)
 }
 
+// GetRepoWatchersIDs returns IDs of watchers for a given repo ID
+// but avoids joining with `user` for performance reasons
+// User permissions must be verified elsewhere if required
+func GetRepoWatchersIDs(repoID int64) ([]int64, error) {
+	ids := make([]int64, 0, 64)
+	return ids, x.Table("watch").
+		Where("watch.repo_id=?", repoID).
+		And("watch.mode<>?", RepoWatchModeDont).
+		Select("user_id").
+		Find(&ids)
+}
+
 // GetWatchers returns range of users watching given repository.
 func (repo *Repository) GetWatchers(page int) ([]*User, error) {
 	users := make([]*User, 0, ItemsPerPage)

models/user.go

@@ -1307,6 +1307,20 @@ func getUserEmailsByNames(e Engine, names []string) []string {
 	return mails
 }
 
+// GetMaileableUsersByIDs gets users from ids, but only if they can receive mails
+func GetMaileableUsersByIDs(ids []int64) ([]*User, error) {
+	if len(ids) == 0 {
+		return nil, nil
+	}
+	ous := make([]*User, 0, len(ids))
+	return ous, x.In("id", ids).
+		Where("`type` = ?", UserTypeIndividual).
+		And("`prohibit_login` = ?", false).
+		And("`is_active` = ?", true).
+		And("`email_notifications_preference` = ?", EmailNotificationsEnabled).
+		Find(&ous)
+}
+
 // GetUsersByIDs returns all resolved users from a list of Ids.
 func GetUsersByIDs(ids []int64) ([]*User, error) {
 	ous := make([]*User, 0, len(ids))

services/mailer/mail.go

@@ -164,13 +164,7 @@ func SendCollaboratorMail(u, doer *models.User, repo *models.Repository) {
 	SendAsync(msg)
 }
 
-func composeIssueCommentMessage(issue *models.Issue, doer *models.User, actionType models.ActionType, fromMention bool,
-	content string, comment *models.Comment, tos []string, info string) *Message {
-
-	if err := issue.LoadPullRequest(); err != nil {
-		log.Error("LoadPullRequest: %v", err)
-		return nil
-	}
+func composeIssueCommentMessages(ctx *mailCommentContext, tos []string, fromMention bool, info string) []*Message {
 
 	var (
 		subject string
@@ -182,29 +176,29 @@ func composeIssueCommentMessage(issue *models.Issue, doer *models.User, actionTy
 	)
 
 	commentType := models.CommentTypeComment
-	if comment != nil {
+	if ctx.Comment != nil {
 		prefix = "Re: "
-		commentType = comment.Type
-		link = issue.HTMLURL() + "#" + comment.HashTag()
+		commentType = ctx.Comment.Type
+		link = ctx.Issue.HTMLURL() + "#" + ctx.Comment.HashTag()
 	} else {
-		link = issue.HTMLURL()
+		link = ctx.Issue.HTMLURL()
 	}
 
 	reviewType := models.ReviewTypeComment
-	if comment != nil && comment.Review != nil {
-		reviewType = comment.Review.Type
+	if ctx.Comment != nil && ctx.Comment.Review != nil {
+		reviewType = ctx.Comment.Review.Type
 	}
 
-	fallback = prefix + fallbackMailSubject(issue)
+	fallback = prefix + fallbackMailSubject(ctx.Issue)
 
 	// This is the body of the new issue or comment, not the mail body
-	body := string(markup.RenderByType(markdown.MarkupName, []byte(content), issue.Repo.HTMLURL(), issue.Repo.ComposeMetas()))
+	body := string(markup.RenderByType(markdown.MarkupName, []byte(ctx.Content), ctx.Issue.Repo.HTMLURL(), ctx.Issue.Repo.ComposeMetas()))
 
-	actType, actName, tplName := actionToTemplate(issue, actionType, commentType, reviewType)
+	actType, actName, tplName := actionToTemplate(ctx.Issue, ctx.ActionType, commentType, reviewType)
 
-	if comment != nil && comment.Review != nil {
+	if ctx.Comment != nil && ctx.Comment.Review != nil {
 		reviewComments = make([]*models.Comment, 0, 10)
-		for _, lines := range comment.Review.CodeComments {
+		for _, lines := range ctx.Comment.Review.CodeComments {
 			for _, comments := range lines {
 				reviewComments = append(reviewComments, comments...)
 			}
@@ -215,12 +209,12 @@ func composeIssueCommentMessage(issue *models.Issue, doer *models.User, actionTy
 		"FallbackSubject": fallback,
 		"Body":            body,
 		"Link":            link,
-		"Issue":           issue,
-		"Comment":         comment,
-		"IsPull":          issue.IsPull,
-		"User":            issue.Repo.MustOwner(),
-		"Repo":            issue.Repo.FullName(),
-		"Doer":            doer,
+		"Issue":           ctx.Issue,
+		"Comment":         ctx.Comment,
+		"IsPull":          ctx.Issue.IsPull,
+		"User":            ctx.Issue.Repo.MustOwner(),
+		"Repo":            ctx.Issue.Repo.FullName(),
+		"Doer":            ctx.Doer,
 		"IsMention":       fromMention,
 		"SubjectPrefix":   prefix,
 		"ActionType":      actType,
@@ -246,18 +240,23 @@ func composeIssueCommentMessage(issue *models.Issue, doer *models.User, actionTy
 		log.Error("ExecuteTemplate [%s]: %v", string(tplName)+"/body", err)
 	}
 
-	msg := NewMessageFrom(tos, doer.DisplayName(), setting.MailService.FromEmail, subject, mailBody.String())
-	msg.Info = fmt.Sprintf("Subject: %s, %s", subject, info)
-
-	// Set Message-ID on first message so replies know what to reference
-	if comment == nil {
-		msg.SetHeader("Message-ID", "<"+issue.ReplyReference()+">")
-	} else {
-		msg.SetHeader("In-Reply-To", "<"+issue.ReplyReference()+">")
-		msg.SetHeader("References", "<"+issue.ReplyReference()+">")
+	// Make sure to compose independent messages to avoid leaking user emails
+	msgs := make([]*Message, 0, len(tos))
+	for _, to := range tos {
+		msg := NewMessageFrom([]string{to}, ctx.Doer.DisplayName(), setting.MailService.FromEmail, subject, mailBody.String())
+		msg.Info = fmt.Sprintf("Subject: %s, %s", subject, info)
+
+		// Set Message-ID on first message so replies know what to reference
+		if ctx.Comment == nil {
+			msg.SetHeader("Message-ID", "<"+ctx.Issue.ReplyReference()+">")
+		} else {
+			msg.SetHeader("In-Reply-To", "<"+ctx.Issue.ReplyReference()+">")
+			msg.SetHeader("References", "<"+ctx.Issue.ReplyReference()+">")
+		}
+		msgs = append(msgs, msg)
 	}
 
-	return msg
+	return msgs
 }
 
 func sanitizeSubject(subject string) string {
@@ -269,21 +268,15 @@ func sanitizeSubject(subject string) string {
 	return mime.QEncoding.Encode("utf-8", string(runes))
 }
 
-// SendIssueCommentMail composes and sends issue comment emails to target receivers.
-func SendIssueCommentMail(issue *models.Issue, doer *models.User, actionType models.ActionType, content string, comment *models.Comment, tos []string) {
-	if len(tos) == 0 {
-		return
-	}
-
-	SendAsync(composeIssueCommentMessage(issue, doer, actionType, false, content, comment, tos, "issue comment"))
-}
-
-// SendIssueMentionMail composes and sends issue mention emails to target receivers.
-func SendIssueMentionMail(issue *models.Issue, doer *models.User, actionType models.ActionType, content string, comment *models.Comment, tos []string) {
-	if len(tos) == 0 {
-		return
-	}
-	SendAsync(composeIssueCommentMessage(issue, doer, actionType, true, content, comment, tos, "issue mention"))
+// SendIssueAssignedMail composes and sends issue assigned email
+func SendIssueAssignedMail(issue *models.Issue, doer *models.User, content string, comment *models.Comment, tos []string) {
+	SendAsyncs(composeIssueCommentMessages(&mailCommentContext{
+		Issue:      issue,
+		Doer:       doer,
+		ActionType: models.ActionType(0),
+		Content:    content,
+		Comment:    comment,
+	}, tos, false, "issue assigned"))
 }
 
 // actionToTemplate returns the type and name of the action facing the user
@@ -341,8 +334,3 @@ func actionToTemplate(issue *models.Issue, actionType models.ActionType,
 	}
 	return
 }
-
-// SendIssueAssignedMail composes and sends issue assigned email
-func SendIssueAssignedMail(issue *models.Issue, doer *models.User, content string, comment *models.Comment, tos []string) {
-	SendAsync(composeIssueCommentMessage(issue, doer, models.ActionType(0), false, content, comment, tos, "issue assigned"))
-}

services/mailer/mail_comment.go

@@ -27,11 +27,18 @@ func mailParticipantsComment(ctx models.DBContext, c *models.Comment, opType mod
 	if err = models.UpdateIssueMentions(ctx, c.IssueID, userMentions); err != nil {
 		return fmt.Errorf("UpdateIssueMentions [%d]: %v", c.IssueID, err)
 	}
-	mentions := make([]string, len(userMentions))
+	mentions := make([]int64, len(userMentions))
 	for i, u := range userMentions {
-		mentions[i] = u.LowerName
+		mentions[i] = u.ID
 	}
-	if err = mailIssueCommentToParticipants(issue, c.Poster, opType, c.Content, c, mentions); err != nil {
+	if err = mailIssueCommentToParticipants(
+		&mailCommentContext{
+			Issue:      issue,
+			Doer:       c.Poster,
+			ActionType: opType,
+			Content:    c.Content,
+			Comment:    c,
+		}, mentions); err != nil {
 		log.Error("mailIssueCommentToParticipants: %v", err)
 	}
 	return nil