From 085f717529008c31b147f76ea7eeaf06ca8801bd Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 3 Nov 2022 16:49:00 +0100 Subject: [PATCH] feat: notify doers of a merge when automerging (#21553) I found myself wondering whether a PR I scheduled for automerge was actually merged. It was, but I didn't receive a mail notification for it - that makes sense considering I am the doer and usually don't want to receive such notifications. But ideally I want to receive a notification when a PR was merged because I scheduled it for automerge. This PR implements exactly that. The implementation works, but I wonder if there's a way to avoid passing the "This PR was automerged" state down so much. I tried solving this via the database (checking if there's an automerge scheduled for this PR when sending the notification) but that did not work reliably, probably because sending the notification happens async and the entry might have already been deleted. My implementation might be the most straightforward but maybe not the most elegant. Signed-off-by: Andrew Thornton Co-authored-by: Lauris BH Co-authored-by: Andrew Thornton Co-authored-by: Lunny Xiao --- models/activities/action.go | 3 ++- modules/notification/action/action.go | 14 ++++++++++++ modules/notification/base/notifier.go | 3 ++- modules/notification/base/null.go | 4 ++++ modules/notification/mail/mail.go | 10 +++++++++ modules/notification/notification.go | 7 ++++++ modules/notification/ui/ui.go | 4 ++++ modules/notification/webhook/webhook.go | 5 +++++ modules/templates/helper.go | 2 +- options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/pull.go | 2 +- routers/web/feed/convert.go | 8 ++++++- routers/web/repo/pull.go | 2 +- services/automerge/automerge.go | 2 +- services/mailer/mail.go | 4 ++-- services/mailer/mail_issue.go | 29 ++++++++++++++----------- services/pull/merge.go | 8 +++++-- tests/integration/pull_merge_test.go | 6 ++--- 18 files changed, 87 insertions(+), 27 deletions(-) diff --git a/models/activities/action.go b/models/activities/action.go index 147511edec6f..cad3263c2d79 100644 --- a/models/activities/action.go +++ b/models/activities/action.go @@ -64,6 +64,7 @@ const ( ActionPublishRelease // 24 ActionPullReviewDismissed // 25 ActionPullRequestReadyForReview // 26 + ActionAutoMergePullRequest // 27 ) // Action represents user operation type and other information to @@ -550,7 +551,7 @@ func notifyWatchers(ctx context.Context, actions ...*Action) error { if !permIssue[i] { continue } - case ActionCreatePullRequest, ActionCommentPull, ActionMergePullRequest, ActionClosePullRequest, ActionReopenPullRequest: + case ActionCreatePullRequest, ActionCommentPull, ActionMergePullRequest, ActionClosePullRequest, ActionReopenPullRequest, ActionAutoMergePullRequest: if !permPR[i] { continue } diff --git a/modules/notification/action/action.go b/modules/notification/action/action.go index d3ff8b156e1c..44d115f3d72a 100644 --- a/modules/notification/action/action.go +++ b/modules/notification/action/action.go @@ -283,6 +283,20 @@ func (*actionNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer } } +func (*actionNotifier) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) { + if err := activities_model.NotifyWatchers(&activities_model.Action{ + ActUserID: doer.ID, + ActUser: doer, + OpType: activities_model.ActionAutoMergePullRequest, + Content: fmt.Sprintf("%d|%s", pr.Issue.Index, pr.Issue.Title), + RepoID: pr.Issue.Repo.ID, + Repo: pr.Issue.Repo, + IsPrivate: pr.Issue.Repo.IsPrivate, + }); err != nil { + log.Error("NotifyWatchers [%d]: %v", pr.ID, err) + } +} + func (*actionNotifier) NotifyPullRevieweDismiss(doer *user_model.User, review *issues_model.Review, comment *issues_model.Comment) { reviewerName := review.Reviewer.Name if len(review.OriginalAuthor) > 0 { diff --git a/modules/notification/base/notifier.go b/modules/notification/base/notifier.go index d6cec92e19ba..9edab8213fab 100644 --- a/modules/notification/base/notifier.go +++ b/modules/notification/base/notifier.go @@ -34,7 +34,8 @@ type Notifier interface { NotifyIssueChangeLabels(doer *user_model.User, issue *issues_model.Issue, addedLabels, removedLabels []*issues_model.Label) NotifyNewPullRequest(pr *issues_model.PullRequest, mentions []*user_model.User) - NotifyMergePullRequest(*issues_model.PullRequest, *user_model.User) + NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) + NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) NotifyPullRequestSynchronized(doer *user_model.User, pr *issues_model.PullRequest) NotifyPullRequestReview(pr *issues_model.PullRequest, review *issues_model.Review, comment *issues_model.Comment, mentions []*user_model.User) NotifyPullRequestCodeComment(pr *issues_model.PullRequest, comment *issues_model.Comment, mentions []*user_model.User) diff --git a/modules/notification/base/null.go b/modules/notification/base/null.go index b113ae79e1e5..f051fbc26f46 100644 --- a/modules/notification/base/null.go +++ b/modules/notification/base/null.go @@ -54,6 +54,10 @@ func (*NullNotifier) NotifyPullRequestCodeComment(pr *issues_model.PullRequest, func (*NullNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) { } +// NotifyAutoMergePullRequest places a place holder function +func (*NullNotifier) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) { +} + // NotifyPullRequestSynchronized places a place holder function func (*NullNotifier) NotifyPullRequestSynchronized(doer *user_model.User, pr *issues_model.PullRequest) { } diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index 100b4eb36f86..54f561839d0f 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -153,6 +153,16 @@ func (m *mailNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer } } +func (m *mailNotifier) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) { + if err := pr.LoadIssue(); err != nil { + log.Error("pr.LoadIssue: %v", err) + return + } + if err := mailer.MailParticipants(pr.Issue, doer, activities_model.ActionAutoMergePullRequest, nil); err != nil { + log.Error("MailParticipants: %v", err) + } +} + func (m *mailNotifier) NotifyPullRequestPushCommits(doer *user_model.User, pr *issues_model.PullRequest, comment *issues_model.Comment) { ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("mailNotifier.NotifyPullRequestPushCommits Pull[%d] #%d in [%d]", pr.ID, pr.Index, pr.BaseRepoID)) defer finished() diff --git a/modules/notification/notification.go b/modules/notification/notification.go index 693c7f5c22f8..7bdc0a04c496 100644 --- a/modules/notification/notification.go +++ b/modules/notification/notification.go @@ -98,6 +98,13 @@ func NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) } } +// NotifyAutoMergePullRequest notifies merge pull request to notifiers +func NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) { + for _, notifier := range notifiers { + notifier.NotifyAutoMergePullRequest(pr, doer) + } +} + // NotifyNewPullRequest notifies new pull request to notifiers func NotifyNewPullRequest(pr *issues_model.PullRequest, mentions []*user_model.User) { for _, notifier := range notifiers { diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go index 4d96a6b0edd4..0e2b3e67c7f6 100644 --- a/modules/notification/ui/ui.go +++ b/modules/notification/ui/ui.go @@ -119,6 +119,10 @@ func (ns *notificationService) NotifyMergePullRequest(pr *issues_model.PullReque }) } +func (ns *notificationService) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) { + ns.NotifyMergePullRequest(pr, doer) +} + func (ns *notificationService) NotifyNewPullRequest(pr *issues_model.PullRequest, mentions []*user_model.User) { if err := pr.LoadIssue(); err != nil { log.Error("Unable to load issue: %d for pr: %d: Error: %v", pr.IssueID, pr.ID, err) diff --git a/modules/notification/webhook/webhook.go b/modules/notification/webhook/webhook.go index 630b56598464..c591e1e34dbc 100644 --- a/modules/notification/webhook/webhook.go +++ b/modules/notification/webhook/webhook.go @@ -632,6 +632,11 @@ func (m *webhookNotifier) NotifyPushCommits(pusher *user_model.User, repo *repo_ } } +func (m *webhookNotifier) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) { + // just redirect to the NotifyMergePullRequest + m.NotifyMergePullRequest(pr, doer) +} + func (*webhookNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) { ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("webhook.NotifyMergePullRequest Pull[%d] #%d in [%d]", pr.ID, pr.Index, pr.BaseRepoID)) defer finished() diff --git a/modules/templates/helper.go b/modules/templates/helper.go index a7232914400a..a127b98dc2ed 100644 --- a/modules/templates/helper.go +++ b/modules/templates/helper.go @@ -905,7 +905,7 @@ func ActionIcon(opType activities_model.ActionType) string { return "git-pull-request" case activities_model.ActionCommentIssue, activities_model.ActionCommentPull: return "comment-discussion" - case activities_model.ActionMergePullRequest: + case activities_model.ActionMergePullRequest, activities_model.ActionAutoMergePullRequest: return "git-merge" case activities_model.ActionCloseIssue, activities_model.ActionClosePullRequest: return "issue-closed" diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index ac2b2ecc99d8..8ffb7a5b2082 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -3003,6 +3003,7 @@ reopen_pull_request = `reopened pull request %[3]s#%[2]s` comment_issue = `commented on issue %[3]s#%[2]s` comment_pull = `commented on pull request %[3]s#%[2]s` merge_pull_request = `merged pull request %[3]s#%[2]s` +auto_merge_pull_request = `automatically merged pull request %[3]s#%[2]s` transfer_repo = transferred repository %s to %s push_tag = pushed tag %[3]s to %[4]s delete_tag = deleted tag %[2]s from %[3]s diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index ebb9c0f261f0..f7e82dab3724 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -839,7 +839,7 @@ func MergePullRequest(ctx *context.APIContext) { } } - if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil { + if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { if models.IsErrInvalidMergeStyle(err) { ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do))) } else if models.IsErrMergeConflicts(err) { diff --git a/routers/web/feed/convert.go b/routers/web/feed/convert.go index 306ecf7d6a3f..c6fc352b647f 100644 --- a/routers/web/feed/convert.go +++ b/routers/web/feed/convert.go @@ -115,6 +115,12 @@ func feedActionsToFeedItems(ctx *context.Context, actions activities_model.Actio link.Href = pullLink } title += ctx.TrHTMLEscapeArgs("action.merge_pull_request", pullLink, act.GetIssueInfos()[0], act.ShortRepoPath()) + case activities_model.ActionAutoMergePullRequest: + pullLink := toPullLink(act) + if link.Href == "#" { + link.Href = pullLink + } + title += ctx.TrHTMLEscapeArgs("action.auto_merge_pull_request", pullLink, act.GetIssueInfos()[0], act.ShortRepoPath()) case activities_model.ActionCloseIssue: issueLink := toIssueLink(act) if link.Href == "#" { @@ -221,7 +227,7 @@ func feedActionsToFeedItems(ctx *context.Context, actions activities_model.Actio if len(comment) != 0 { desc += "\n\n" + renderMarkdown(ctx, act, comment) } - case activities_model.ActionMergePullRequest: + case activities_model.ActionMergePullRequest, activities_model.ActionAutoMergePullRequest: desc = act.GetIssueInfos()[1] case activities_model.ActionCloseIssue, activities_model.ActionReopenIssue, activities_model.ActionClosePullRequest, activities_model.ActionReopenPullRequest: desc = act.GetIssueTitle() diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index fc95bbf240f0..41eac7cc3923 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1002,7 +1002,7 @@ func MergePullRequest(ctx *context.Context) { } } - if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil { + if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { if models.IsErrInvalidMergeStyle(err) { ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option")) ctx.Redirect(issue.Link()) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index ca008ebfe6c8..3ee8af2344d3 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -257,7 +257,7 @@ func handlePull(pullID int64, sha string) { defer baseGitRepo.Close() } - if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message); err != nil { + if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil { log.Error("pull_service.Merge: %v", err) return } diff --git a/services/mailer/mail.go b/services/mailer/mail.go index a5bfa496f9ea..85a7d107e527 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -340,7 +340,7 @@ func createReference(issue *issues_model.Issue, comment *issues_model.Comment, a extra = fmt.Sprintf("/close/%d", time.Now().UnixNano()/1e6) case activities_model.ActionReopenIssue, activities_model.ActionReopenPullRequest: extra = fmt.Sprintf("/reopen/%d", time.Now().UnixNano()/1e6) - case activities_model.ActionMergePullRequest: + case activities_model.ActionMergePullRequest, activities_model.ActionAutoMergePullRequest: extra = fmt.Sprintf("/merge/%d", time.Now().UnixNano()/1e6) case activities_model.ActionPullRequestReadyForReview: extra = fmt.Sprintf("/ready/%d", time.Now().UnixNano()/1e6) @@ -451,7 +451,7 @@ func actionToTemplate(issue *issues_model.Issue, actionType activities_model.Act name = "close" case activities_model.ActionReopenIssue, activities_model.ActionReopenPullRequest: name = "reopen" - case activities_model.ActionMergePullRequest: + case activities_model.ActionMergePullRequest, activities_model.ActionAutoMergePullRequest: name = "merge" case activities_model.ActionPullReviewDismissed: name = "review_dismissed" diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index 33a20694e836..61e276805dbf 100644 --- a/services/mailer/mail_issue.go +++ b/services/mailer/mail_issue.go @@ -25,11 +25,12 @@ func fallbackMailSubject(issue *issues_model.Issue) string { type mailCommentContext struct { context.Context - Issue *issues_model.Issue - Doer *user_model.User - ActionType activities_model.ActionType - Content string - Comment *issues_model.Comment + Issue *issues_model.Issue + Doer *user_model.User + ActionType activities_model.ActionType + Content string + Comment *issues_model.Comment + ForceDoerNotification bool } const ( @@ -93,7 +94,7 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*user_mo visited := make(container.Set[int64], len(unfiltered)+len(mentions)+1) // Avoid mailing the doer - if ctx.Doer.EmailNotificationsPreference != user_model.EmailNotificationsAndYourOwn { + if ctx.Doer.EmailNotificationsPreference != user_model.EmailNotificationsAndYourOwn && !ctx.ForceDoerNotification { visited.Add(ctx.Doer.ID) } @@ -181,17 +182,19 @@ func MailParticipants(issue *issues_model.Issue, doer *user_model.User, opType a content := issue.Content if opType == activities_model.ActionCloseIssue || opType == activities_model.ActionClosePullRequest || opType == activities_model.ActionReopenIssue || opType == activities_model.ActionReopenPullRequest || - opType == activities_model.ActionMergePullRequest { + opType == activities_model.ActionMergePullRequest || opType == activities_model.ActionAutoMergePullRequest { content = "" } + forceDoerNotification := opType == activities_model.ActionAutoMergePullRequest if err := mailIssueCommentToParticipants( &mailCommentContext{ - Context: context.TODO(), // TODO: use a correct context - Issue: issue, - Doer: doer, - ActionType: opType, - Content: content, - Comment: nil, + Context: context.TODO(), // TODO: use a correct context + Issue: issue, + Doer: doer, + ActionType: opType, + Content: content, + Comment: nil, + ForceDoerNotification: forceDoerNotification, }, mentions); err != nil { log.Error("mailIssueCommentToParticipants: %v", err) } diff --git a/services/pull/merge.go b/services/pull/merge.go index 0ca373018331..56ee9c9a734e 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -133,7 +133,7 @@ func GetDefaultMergeMessage(baseGitRepo *git.Repository, pr *issues_model.PullRe // Merge merges pull request to base repository. // Caller should check PR is ready to be merged (review and status checks) -func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) error { +func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, wasAutoMerged bool) error { if err := pr.LoadHeadRepo(); err != nil { log.Error("LoadHeadRepo: %v", err) return fmt.Errorf("LoadHeadRepo: %w", err) @@ -193,7 +193,11 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U log.Error("GetOwner for issue repo [%d]: %v", pr.ID, err) } - notification.NotifyMergePullRequest(pr, doer) + if wasAutoMerged { + notification.NotifyAutoMergePullRequest(pr, doer) + } else { + notification.NotifyMergePullRequest(pr, doer) + } // Reset cached commit count cache.Remove(pr.Issue.Repo.GetCommitsCountCacheKey(pr.BaseBranch, true)) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 9bd430084dcc..bec85e8a8d0f 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -245,11 +245,11 @@ func TestCantMergeConflict(t *testing.T) { gitRepo, err := git.OpenRepository(git.DefaultContext, repo_model.RepoPath(user1.Name, repo1.Name)) assert.NoError(t, err) - err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "CONFLICT") + err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "CONFLICT", false) assert.Error(t, err, "Merge should return an error due to conflict") assert.True(t, models.IsErrMergeConflicts(err), "Merge error is not a conflict error") - err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleRebase, "", "CONFLICT") + err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleRebase, "", "CONFLICT", false) assert.Error(t, err, "Merge should return an error due to conflict") assert.True(t, models.IsErrRebaseConflicts(err), "Merge error is not a conflict error") gitRepo.Close() @@ -344,7 +344,7 @@ func TestCantMergeUnrelated(t *testing.T) { BaseBranch: "base", }) - err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "UNRELATED") + err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "UNRELATED", false) assert.Error(t, err, "Merge should return an error due to unrelated") assert.True(t, models.IsErrMergeUnrelatedHistories(err), "Merge error is not a unrelated histories error") gitRepo.Close()