Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve notifications for WIP draft PR's #14663

Merged
merged 14 commits into from
Jun 23, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 26 additions & 25 deletions models/action.go
Original file line number Diff line number Diff line change
@@ -26,31 +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
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
15 changes: 8 additions & 7 deletions models/notification.go
Original file line number Diff line number Diff line change
@@ -207,13 +207,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 {
6 changes: 5 additions & 1 deletion models/pull.go
Original file line number Diff line number Diff line change
@@ -595,9 +595,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
}
}
12 changes: 12 additions & 0 deletions modules/notification/mail/mail.go
Original file line number Diff line number Diff line change
@@ -73,6 +73,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.ActionPullRequestReadyForReview, 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)
40 changes: 35 additions & 5 deletions modules/notification/ui/ui.go
Original file line number Diff line number Diff line change
@@ -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,
@@ -106,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,
})
}
}
2 changes: 2 additions & 0 deletions services/mailer/mail.go
Original file line number Diff line number Diff line change
@@ -377,6 +377,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:
12 changes: 7 additions & 5 deletions services/mailer/mail_issue.go
Original file line number Diff line number Diff line change
@@ -30,7 +30,7 @@ const (

// 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 []*models.User) error {

@@ -74,11 +74,13 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*models.

// =========== 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() && ctx.ActionType != models.ActionCreatePullRequest) {
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)

2 changes: 2 additions & 0 deletions templates/mail/issue/default.tmpl
Original file line number Diff line number Diff line change
@@ -51,6 +51,8 @@
<b>@{{.Doer.Name}}</b> commented on this pull request.
{{else if eq .ActionName "review_dismissed"}}
<b>@{{.Doer.Name}}</b> dismissed last review from {{.Comment.Review.Reviewer.Name}} for this pull request.
{{else if eq .ActionName "ready_for_review"}}
<b>@{{.Doer.Name}}</b> marked this pull request ready for review.
{{end}}

{{- if eq .Body ""}}