Send mail to issue/pr assignee/reviewer also when OnMention is set (#18707) (#18765)

Backport #18707

Addresses #17892, where emails notifications are not sent to assignees (issue and PR) and reviewers (PR) when they have the email setting Only email on mention enabled.

From the user experience perspective, when a user gets a issue/PR assigned or a PR review request, he/she would expect to be implicitly mentioned since the assignment or request is personal and targeting a single person only. Thus I see #17892 as a bug. Could we therefore mark this ticket as such?

The changed code just explicitly checks for the EmailNotificationsOnMention setting beside the existing EmailNotificationsEnabled check. Too rude?

@lunny mentioned a mock mail server for tests, is there something ready. How could I make use of it?

See #12774 (comment)

Fix #17892

Co-authored-by: flozzone <flozzone@gmail.com>

diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go
index 6ad0ca0d85cf026e919f0196b38be6979e1029b4..4609e513a5947e5c10da2e0f401f42835b05a4bf 100644 (file)
--- a/modules/notification/mail/mail.go
+++ b/modules/notification/mail/mail.go
@@ -115,7 +115,7 @@ func (m *mailNotifier) NotifyPullRequestCodeComment(pr *models.PullRequest, comm
 
 func (m *mailNotifier) NotifyIssueChangeAssignee(doer *user_model.User, issue *models.Issue, assignee *user_model.User, removed bool, comment *models.Comment) {
        // mail only sent to added assignees and not self-assignee
-       if !removed && doer.ID != assignee.ID && assignee.EmailNotifications() == user_model.EmailNotificationsEnabled {
+       if !removed && doer.ID != assignee.ID && (assignee.EmailNotifications() == user_model.EmailNotificationsEnabled || assignee.EmailNotifications() == user_model.EmailNotificationsOnMention) {
                ct := fmt.Sprintf("Assigned #%d.", issue.Index)
                if err := mailer.SendIssueAssignedMail(issue, doer, ct, comment, []*user_model.User{assignee}); err != nil {
                        log.Error("Error in SendIssueAssignedMail for issue[%d] to assignee[%d]: %v", issue.ID, assignee.ID, err)
@@ -124,7 +124,7 @@ func (m *mailNotifier) NotifyIssueChangeAssignee(doer *user_model.User, issue *m
 }
 
 func (m *mailNotifier) NotifyPullReviewRequest(doer *user_model.User, issue *models.Issue, reviewer *user_model.User, isRequest bool, comment *models.Comment) {
-       if isRequest && doer.ID != reviewer.ID && reviewer.EmailNotifications() == user_model.EmailNotificationsEnabled {
+       if isRequest && doer.ID != reviewer.ID && (reviewer.EmailNotifications() == user_model.EmailNotificationsEnabled || reviewer.EmailNotifications() == user_model.EmailNotificationsOnMention) {
                ct := fmt.Sprintf("Requested to review %s.", issue.HTMLURL())
                if err := mailer.SendIssueAssignedMail(issue, doer, ct, comment, []*user_model.User{reviewer}); err != nil {
                        log.Error("Error in SendIssueAssignedMail for issue[%d] to reviewer[%d]: %v", issue.ID, reviewer.ID, err)