From bab067b0e2ceadf8b6547d64576d89fa6ed8bcd0 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Fri, 12 Feb 2021 22:06:33 +0100 Subject: [PATCH 1/6] #14559 Reduce amount of email notifications for WIP draft PR's don't notify repo watchers of WIP draft PR's --- models/notification.go | 15 ++++++++------- models/pull.go | 6 +++++- services/mailer/mail_issue.go | 12 +++++++----- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/models/notification.go b/models/notification.go index 362b490994071..c01ca0c15d957 100644 --- a/models/notification.go +++ b/models/notification.go @@ -162,13 +162,14 @@ func createOrUpdateIssueNotifications(e Engine, issueID, commentID, notification for _, id := range issueWatches { toNotify[id] = struct{}{} } - - repoWatches, err := getRepoWatchersIDs(e, issue.RepoID) - if err != nil { - return err - } - for _, id := range repoWatches { - toNotify[id] = struct{}{} + if !(issue.IsPull && HasWorkInProgressPrefix(issue.Title)) { + repoWatches, err := getRepoWatchersIDs(e, issue.RepoID) + if err != nil { + return err + } + for _, id := range repoWatches { + toNotify[id] = struct{}{} + } } issueParticipants, err := issue.getParticipantIDsByIssue(e) if err != nil { diff --git a/models/pull.go b/models/pull.go index 0d4691aac94b6..19b54ed9e9f9f 100644 --- a/models/pull.go +++ b/models/pull.go @@ -589,9 +589,13 @@ func (pr *PullRequest) IsWorkInProgress() bool { log.Error("LoadIssue: %v", err) return false } + return HasWorkInProgressPrefix(pr.Issue.Title) +} +// HasWorkInProgressPrefix determines if the given PR title has a Work In Progress prefix +func HasWorkInProgressPrefix(title string) bool { for _, prefix := range setting.Repository.PullRequest.WorkInProgressPrefixes { - if strings.HasPrefix(strings.ToUpper(pr.Issue.Title), prefix) { + if strings.HasPrefix(strings.ToUpper(title), prefix) { return true } } diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index b600060a67f5e..ece052a58635d 100644 --- a/services/mailer/mail_issue.go +++ b/services/mailer/mail_issue.go @@ -25,7 +25,7 @@ type mailCommentContext struct { // mailIssueCommentToParticipants can be used for both new issue creation and comment. // This function sends two list of emails: -// 1. Repository watchers and users who are participated in comments. +// 1. Repository watchers (except for WIP pull requests) and users who are participated in comments. // 2. Users who are not in 1. but get mentioned in current issue/comment. func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []int64) error { @@ -69,11 +69,13 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []int64) e // =========== Repo watchers =========== // Make repo watchers last, since it's likely the list with the most users - ids, err = models.GetRepoWatchersIDs(ctx.Issue.RepoID) - if err != nil { - return fmt.Errorf("GetRepoWatchersIDs(%d): %v", ctx.Issue.RepoID, err) + if !(ctx.Issue.IsPull && ctx.Issue.PullRequest.IsWorkInProgress()) { + ids, err = models.GetRepoWatchersIDs(ctx.Issue.RepoID) + if err != nil { + return fmt.Errorf("GetRepoWatchersIDs(%d): %v", ctx.Issue.RepoID, err) + } + unfiltered = append(ids, unfiltered...) } - unfiltered = append(ids, unfiltered...) visited := make(map[int64]bool, len(unfiltered)+len(mentions)+1) From c87dd556b773d95a890df7ba4a72b6c1ae52f595 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Fri, 12 Feb 2021 22:07:09 +0100 Subject: [PATCH 2/6] #13190 Notification when WIP Pull Request is ready for review --- modules/notification/mail/mail.go | 12 ++++++++++++ modules/notification/ui/ui.go | 13 +++++++++++++ 2 files changed, 25 insertions(+) diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index f984ea7661ae6..b4e8d8252145e 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -74,6 +74,18 @@ func (m *mailNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models. } } +func (m *mailNotifier) NotifyIssueChangeTitle(doer *models.User, issue *models.Issue, oldTitle string) { + if err := issue.LoadPullRequest(); err != nil { + log.Error("issue.LoadPullRequest: %v", err) + return + } + if issue.IsPull && models.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() { + if err := mailer.MailParticipants(issue, doer, models.ActionCreatePullRequest, nil); err != nil { + log.Error("MailParticipants: %v", err) + } + } +} + func (m *mailNotifier) NotifyNewPullRequest(pr *models.PullRequest, mentions []*models.User) { if err := mailer.MailParticipants(pr.Issue, pr.Issue.Poster, models.ActionCreatePullRequest, mentions); err != nil { log.Error("MailParticipants: %v", err) diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go index 25ea4d91c643f..7120f60dfe65e 100644 --- a/modules/notification/ui/ui.go +++ b/modules/notification/ui/ui.go @@ -94,6 +94,19 @@ func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue }) } +func (ns *notificationService) NotifyIssueChangeTitle(doer *models.User, issue *models.Issue, oldTitle string) { + if err := issue.LoadPullRequest(); err != nil { + log.Error("issue.LoadPullRequest: %v", err) + return + } + if issue.IsPull && models.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() { + _ = ns.issueQueue.Push(issueNotificationOpts{ + IssueID: issue.ID, + NotificationAuthorID: doer.ID, + }) + } +} + func (ns *notificationService) NotifyMergePullRequest(pr *models.PullRequest, doer *models.User) { _ = ns.issueQueue.Push(issueNotificationOpts{ IssueID: pr.Issue.ID, From 1ae2a39346ddf4c0add043122fd559d4e3f057f6 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Fri, 12 Feb 2021 23:31:26 +0100 Subject: [PATCH 3/6] Send email notification to repo watchers when WIP PR is created --- services/mailer/mail_issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index ece052a58635d..ac559ecb8088d 100644 --- a/services/mailer/mail_issue.go +++ b/services/mailer/mail_issue.go @@ -69,7 +69,7 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []int64) e // =========== Repo watchers =========== // Make repo watchers last, since it's likely the list with the most users - if !(ctx.Issue.IsPull && ctx.Issue.PullRequest.IsWorkInProgress()) { + if !(ctx.Issue.IsPull && ctx.Issue.PullRequest.IsWorkInProgress() && ctx.ActionType != models.ActionCreatePullRequest) { ids, err = models.GetRepoWatchersIDs(ctx.Issue.RepoID) if err != nil { return fmt.Errorf("GetRepoWatchersIDs(%d): %v", ctx.Issue.RepoID, err) From 5e646c8b26a8fba42281529d92f42125bb3ad077 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sat, 13 Feb 2021 20:45:08 +0100 Subject: [PATCH 4/6] Send ui notification to repo watchers when WIP PR is created --- modules/notification/ui/ui.go | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go index 7120f60dfe65e..9b18b311e9b7c 100644 --- a/modules/notification/ui/ui.go +++ b/modules/notification/ui/ui.go @@ -119,15 +119,32 @@ func (ns *notificationService) NotifyNewPullRequest(pr *models.PullRequest, ment log.Error("Unable to load issue: %d for pr: %d: Error: %v", pr.IssueID, pr.ID, err) return } - _ = ns.issueQueue.Push(issueNotificationOpts{ - IssueID: pr.Issue.ID, - NotificationAuthorID: pr.Issue.PosterID, - }) + toNotify := make(map[int64]struct{}, 32) + repoWatchers, err := models.GetRepoWatchersIDs(pr.Issue.RepoID) + if err != nil { + log.Error("GetRepoWatchersIDs: %v", err) + return + } + for _, id := range repoWatchers { + toNotify[id] = struct{}{} + } + issueParticipants, err := models.GetParticipantsIDsByIssueID(pr.IssueID) + if err != nil { + log.Error("GetParticipantsIDsByIssueID: %v", err) + return + } + for _, id := range issueParticipants { + toNotify[id] = struct{}{} + } + delete(toNotify, pr.Issue.PosterID) for _, mention := range mentions { + toNotify[mention.ID] = struct{}{} + } + for receiverID := range toNotify { _ = ns.issueQueue.Push(issueNotificationOpts{ IssueID: pr.Issue.ID, NotificationAuthorID: pr.Issue.PosterID, - ReceiverID: mention.ID, + ReceiverID: receiverID, }) } } From d16de308c52f3df6cf9b7d63eb847c105af49723 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sun, 14 Feb 2021 16:08:34 +0100 Subject: [PATCH 5/6] send specific email notification when PR is marked ready for review instead of reusing the CreatePullRequest action --- models/action.go | 1 + modules/notification/mail/mail.go | 2 +- services/mailer/mail.go | 2 ++ templates/mail/issue/default.tmpl | 2 ++ 4 files changed, 6 insertions(+), 1 deletion(-) diff --git a/models/action.go b/models/action.go index e8a1336566e15..d3e31ff55f2b9 100644 --- a/models/action.go +++ b/models/action.go @@ -51,6 +51,7 @@ const ( ActionCommentPull // 23 ActionPublishRelease // 24 ActionPullReviewDismissed // 25 + ActionPullRequestReadyForReview // 26 ) // Action represents user operation type and other information to diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index b4e8d8252145e..d6d3f05a944bb 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -80,7 +80,7 @@ func (m *mailNotifier) NotifyIssueChangeTitle(doer *models.User, issue *models.I return } if issue.IsPull && models.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() { - if err := mailer.MailParticipants(issue, doer, models.ActionCreatePullRequest, nil); err != nil { + if err := mailer.MailParticipants(issue, doer, models.ActionPullRequestReadyForReview, nil); err != nil { log.Error("MailParticipants: %v", err) } } diff --git a/services/mailer/mail.go b/services/mailer/mail.go index e87d34ab29521..c461da4c57014 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -306,6 +306,8 @@ func actionToTemplate(issue *models.Issue, actionType models.ActionType, name = "merge" case models.ActionPullReviewDismissed: name = "review_dismissed" + case models.ActionPullRequestReadyForReview: + name = "ready_for_review" default: switch commentType { case models.CommentTypeReview: diff --git a/templates/mail/issue/default.tmpl b/templates/mail/issue/default.tmpl index b7d576bef4adf..5e128baad6f09 100644 --- a/templates/mail/issue/default.tmpl +++ b/templates/mail/issue/default.tmpl @@ -51,6 +51,8 @@ @{{.Doer.Name}} commented on this pull request. {{else if eq .ActionName "review_dismissed"}} @{{.Doer.Name}} dismissed last review from {{.Comment.Review.Reviewer.Name}} for this pull request. + {{else if eq .ActionName "ready_for_review"}} + @{{.Doer.Name}} marked this pull request ready for review. {{end}} {{- if eq .Body ""}} From 4caea81eeb552d0d323b1c49992905f30f4143aa Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Mon, 15 Feb 2021 19:46:08 +0100 Subject: [PATCH 6/6] Fix lint error --- models/action.go | 52 ++++++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/models/action.go b/models/action.go index d3e31ff55f2b9..aba9037617c0b 100644 --- a/models/action.go +++ b/models/action.go @@ -26,32 +26,32 @@ type ActionType int // Possible action types. const ( - ActionCreateRepo ActionType = iota + 1 // 1 - ActionRenameRepo // 2 - ActionStarRepo // 3 - ActionWatchRepo // 4 - ActionCommitRepo // 5 - ActionCreateIssue // 6 - ActionCreatePullRequest // 7 - ActionTransferRepo // 8 - ActionPushTag // 9 - ActionCommentIssue // 10 - ActionMergePullRequest // 11 - ActionCloseIssue // 12 - ActionReopenIssue // 13 - ActionClosePullRequest // 14 - ActionReopenPullRequest // 15 - ActionDeleteTag // 16 - ActionDeleteBranch // 17 - ActionMirrorSyncPush // 18 - ActionMirrorSyncCreate // 19 - ActionMirrorSyncDelete // 20 - ActionApprovePullRequest // 21 - ActionRejectPullRequest // 22 - ActionCommentPull // 23 - ActionPublishRelease // 24 - ActionPullReviewDismissed // 25 - ActionPullRequestReadyForReview // 26 + ActionCreateRepo ActionType = iota + 1 // 1 + ActionRenameRepo // 2 + ActionStarRepo // 3 + ActionWatchRepo // 4 + ActionCommitRepo // 5 + ActionCreateIssue // 6 + ActionCreatePullRequest // 7 + ActionTransferRepo // 8 + ActionPushTag // 9 + ActionCommentIssue // 10 + ActionMergePullRequest // 11 + ActionCloseIssue // 12 + ActionReopenIssue // 13 + ActionClosePullRequest // 14 + ActionReopenPullRequest // 15 + ActionDeleteTag // 16 + ActionDeleteBranch // 17 + ActionMirrorSyncPush // 18 + ActionMirrorSyncCreate // 19 + ActionMirrorSyncDelete // 20 + ActionApprovePullRequest // 21 + ActionRejectPullRequest // 22 + ActionCommentPull // 23 + ActionPublishRelease // 24 + ActionPullReviewDismissed // 25 + ActionPullRequestReadyForReview // 26 ) // Action represents user operation type and other information to