Skip to content

Commit 085f717

Browse files
kolaentelafrikszeripathlunny
authored
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 <[email protected]> Co-authored-by: Lauris BH <[email protected]> Co-authored-by: Andrew Thornton <[email protected]> Co-authored-by: Lunny Xiao <[email protected]>
1 parent f17edfa commit 085f717

File tree

18 files changed

+87
-27
lines changed

18 files changed

+87
-27
lines changed

‎models/activities/action.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ const (
6464
ActionPublishRelease // 24
6565
ActionPullReviewDismissed // 25
6666
ActionPullRequestReadyForReview // 26
67+
ActionAutoMergePullRequest // 27
6768
)
6869

6970
// Action represents user operation type and other information to
@@ -550,7 +551,7 @@ func notifyWatchers(ctx context.Context, actions ...*Action) error {
550551
if !permIssue[i] {
551552
continue
552553
}
553-
case ActionCreatePullRequest, ActionCommentPull, ActionMergePullRequest, ActionClosePullRequest, ActionReopenPullRequest:
554+
case ActionCreatePullRequest, ActionCommentPull, ActionMergePullRequest, ActionClosePullRequest, ActionReopenPullRequest, ActionAutoMergePullRequest:
554555
if !permPR[i] {
555556
continue
556557
}

‎modules/notification/action/action.go

+14
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,20 @@ func (*actionNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer
283283
}
284284
}
285285

286+
func (*actionNotifier) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
287+
if err := activities_model.NotifyWatchers(&activities_model.Action{
288+
ActUserID: doer.ID,
289+
ActUser: doer,
290+
OpType: activities_model.ActionAutoMergePullRequest,
291+
Content: fmt.Sprintf("%d|%s", pr.Issue.Index, pr.Issue.Title),
292+
RepoID: pr.Issue.Repo.ID,
293+
Repo: pr.Issue.Repo,
294+
IsPrivate: pr.Issue.Repo.IsPrivate,
295+
}); err != nil {
296+
log.Error("NotifyWatchers [%d]: %v", pr.ID, err)
297+
}
298+
}
299+
286300
func (*actionNotifier) NotifyPullRevieweDismiss(doer *user_model.User, review *issues_model.Review, comment *issues_model.Comment) {
287301
reviewerName := review.Reviewer.Name
288302
if len(review.OriginalAuthor) > 0 {

‎modules/notification/base/notifier.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ type Notifier interface {
3434
NotifyIssueChangeLabels(doer *user_model.User, issue *issues_model.Issue,
3535
addedLabels, removedLabels []*issues_model.Label)
3636
NotifyNewPullRequest(pr *issues_model.PullRequest, mentions []*user_model.User)
37-
NotifyMergePullRequest(*issues_model.PullRequest, *user_model.User)
37+
NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User)
38+
NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User)
3839
NotifyPullRequestSynchronized(doer *user_model.User, pr *issues_model.PullRequest)
3940
NotifyPullRequestReview(pr *issues_model.PullRequest, review *issues_model.Review, comment *issues_model.Comment, mentions []*user_model.User)
4041
NotifyPullRequestCodeComment(pr *issues_model.PullRequest, comment *issues_model.Comment, mentions []*user_model.User)

‎modules/notification/base/null.go

+4
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,10 @@ func (*NullNotifier) NotifyPullRequestCodeComment(pr *issues_model.PullRequest,
5454
func (*NullNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
5555
}
5656

57+
// NotifyAutoMergePullRequest places a place holder function
58+
func (*NullNotifier) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
59+
}
60+
5761
// NotifyPullRequestSynchronized places a place holder function
5862
func (*NullNotifier) NotifyPullRequestSynchronized(doer *user_model.User, pr *issues_model.PullRequest) {
5963
}

‎modules/notification/mail/mail.go

+10
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,16 @@ func (m *mailNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer
153153
}
154154
}
155155

156+
func (m *mailNotifier) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
157+
if err := pr.LoadIssue(); err != nil {
158+
log.Error("pr.LoadIssue: %v", err)
159+
return
160+
}
161+
if err := mailer.MailParticipants(pr.Issue, doer, activities_model.ActionAutoMergePullRequest, nil); err != nil {
162+
log.Error("MailParticipants: %v", err)
163+
}
164+
}
165+
156166
func (m *mailNotifier) NotifyPullRequestPushCommits(doer *user_model.User, pr *issues_model.PullRequest, comment *issues_model.Comment) {
157167
ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("mailNotifier.NotifyPullRequestPushCommits Pull[%d] #%d in [%d]", pr.ID, pr.Index, pr.BaseRepoID))
158168
defer finished()

‎modules/notification/notification.go

+7
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,13 @@ func NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User)
9898
}
9999
}
100100

101+
// NotifyAutoMergePullRequest notifies merge pull request to notifiers
102+
func NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
103+
for _, notifier := range notifiers {
104+
notifier.NotifyAutoMergePullRequest(pr, doer)
105+
}
106+
}
107+
101108
// NotifyNewPullRequest notifies new pull request to notifiers
102109
func NotifyNewPullRequest(pr *issues_model.PullRequest, mentions []*user_model.User) {
103110
for _, notifier := range notifiers {

‎modules/notification/ui/ui.go

+4
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,10 @@ func (ns *notificationService) NotifyMergePullRequest(pr *issues_model.PullReque
119119
})
120120
}
121121

122+
func (ns *notificationService) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
123+
ns.NotifyMergePullRequest(pr, doer)
124+
}
125+
122126
func (ns *notificationService) NotifyNewPullRequest(pr *issues_model.PullRequest, mentions []*user_model.User) {
123127
if err := pr.LoadIssue(); err != nil {
124128
log.Error("Unable to load issue: %d for pr: %d: Error: %v", pr.IssueID, pr.ID, err)

‎modules/notification/webhook/webhook.go

+5
Original file line numberDiff line numberDiff line change
@@ -632,6 +632,11 @@ func (m *webhookNotifier) NotifyPushCommits(pusher *user_model.User, repo *repo_
632632
}
633633
}
634634

635+
func (m *webhookNotifier) NotifyAutoMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
636+
// just redirect to the NotifyMergePullRequest
637+
m.NotifyMergePullRequest(pr, doer)
638+
}
639+
635640
func (*webhookNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
636641
ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("webhook.NotifyMergePullRequest Pull[%d] #%d in [%d]", pr.ID, pr.Index, pr.BaseRepoID))
637642
defer finished()

‎modules/templates/helper.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -905,7 +905,7 @@ func ActionIcon(opType activities_model.ActionType) string {
905905
return "git-pull-request"
906906
case activities_model.ActionCommentIssue, activities_model.ActionCommentPull:
907907
return "comment-discussion"
908-
case activities_model.ActionMergePullRequest:
908+
case activities_model.ActionMergePullRequest, activities_model.ActionAutoMergePullRequest:
909909
return "git-merge"
910910
case activities_model.ActionCloseIssue, activities_model.ActionClosePullRequest:
911911
return "issue-closed"

‎options/locale/locale_en-US.ini

+1
Original file line numberDiff line numberDiff line change
@@ -3003,6 +3003,7 @@ reopen_pull_request = `reopened pull request <a href="%[1]s">%[3]s#%[2]s</a>`
30033003
comment_issue = `commented on issue <a href="%[1]s">%[3]s#%[2]s</a>`
30043004
comment_pull = `commented on pull request <a href="%[1]s">%[3]s#%[2]s</a>`
30053005
merge_pull_request = `merged pull request <a href="%[1]s">%[3]s#%[2]s</a>`
3006+
auto_merge_pull_request = `automatically merged pull request <a href="%[1]s">%[3]s#%[2]s</a>`
30063007
transfer_repo = transferred repository <code>%s</code> to <a href="%s">%s</a>
30073008
push_tag = pushed tag <a href="%[2]s">%[3]s</a> to <a href="%[1]s">%[4]s</a>
30083009
delete_tag = deleted tag %[2]s from <a href="%[1]s">%[3]s</a>

‎routers/api/v1/repo/pull.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -839,7 +839,7 @@ func MergePullRequest(ctx *context.APIContext) {
839839
}
840840
}
841841

842-
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
842+
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil {
843843
if models.IsErrInvalidMergeStyle(err) {
844844
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)))
845845
} else if models.IsErrMergeConflicts(err) {

‎routers/web/feed/convert.go

+7-1
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,12 @@ func feedActionsToFeedItems(ctx *context.Context, actions activities_model.Actio
115115
link.Href = pullLink
116116
}
117117
title += ctx.TrHTMLEscapeArgs("action.merge_pull_request", pullLink, act.GetIssueInfos()[0], act.ShortRepoPath())
118+
case activities_model.ActionAutoMergePullRequest:
119+
pullLink := toPullLink(act)
120+
if link.Href == "#" {
121+
link.Href = pullLink
122+
}
123+
title += ctx.TrHTMLEscapeArgs("action.auto_merge_pull_request", pullLink, act.GetIssueInfos()[0], act.ShortRepoPath())
118124
case activities_model.ActionCloseIssue:
119125
issueLink := toIssueLink(act)
120126
if link.Href == "#" {
@@ -221,7 +227,7 @@ func feedActionsToFeedItems(ctx *context.Context, actions activities_model.Actio
221227
if len(comment) != 0 {
222228
desc += "\n\n" + renderMarkdown(ctx, act, comment)
223229
}
224-
case activities_model.ActionMergePullRequest:
230+
case activities_model.ActionMergePullRequest, activities_model.ActionAutoMergePullRequest:
225231
desc = act.GetIssueInfos()[1]
226232
case activities_model.ActionCloseIssue, activities_model.ActionReopenIssue, activities_model.ActionClosePullRequest, activities_model.ActionReopenPullRequest:
227233
desc = act.GetIssueTitle()

‎routers/web/repo/pull.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -1002,7 +1002,7 @@ func MergePullRequest(ctx *context.Context) {
10021002
}
10031003
}
10041004

1005-
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
1005+
if err := pull_service.Merge(ctx, pr, ctx.Doer, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil {
10061006
if models.IsErrInvalidMergeStyle(err) {
10071007
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
10081008
ctx.Redirect(issue.Link())

‎services/automerge/automerge.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ func handlePull(pullID int64, sha string) {
257257
defer baseGitRepo.Close()
258258
}
259259

260-
if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message); err != nil {
260+
if err := pull_service.Merge(ctx, pr, doer, baseGitRepo, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil {
261261
log.Error("pull_service.Merge: %v", err)
262262
return
263263
}

‎services/mailer/mail.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ func createReference(issue *issues_model.Issue, comment *issues_model.Comment, a
340340
extra = fmt.Sprintf("/close/%d", time.Now().UnixNano()/1e6)
341341
case activities_model.ActionReopenIssue, activities_model.ActionReopenPullRequest:
342342
extra = fmt.Sprintf("/reopen/%d", time.Now().UnixNano()/1e6)
343-
case activities_model.ActionMergePullRequest:
343+
case activities_model.ActionMergePullRequest, activities_model.ActionAutoMergePullRequest:
344344
extra = fmt.Sprintf("/merge/%d", time.Now().UnixNano()/1e6)
345345
case activities_model.ActionPullRequestReadyForReview:
346346
extra = fmt.Sprintf("/ready/%d", time.Now().UnixNano()/1e6)
@@ -451,7 +451,7 @@ func actionToTemplate(issue *issues_model.Issue, actionType activities_model.Act
451451
name = "close"
452452
case activities_model.ActionReopenIssue, activities_model.ActionReopenPullRequest:
453453
name = "reopen"
454-
case activities_model.ActionMergePullRequest:
454+
case activities_model.ActionMergePullRequest, activities_model.ActionAutoMergePullRequest:
455455
name = "merge"
456456
case activities_model.ActionPullReviewDismissed:
457457
name = "review_dismissed"

‎services/mailer/mail_issue.go

+16-13
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,12 @@ func fallbackMailSubject(issue *issues_model.Issue) string {
2525

2626
type mailCommentContext struct {
2727
context.Context
28-
Issue *issues_model.Issue
29-
Doer *user_model.User
30-
ActionType activities_model.ActionType
31-
Content string
32-
Comment *issues_model.Comment
28+
Issue *issues_model.Issue
29+
Doer *user_model.User
30+
ActionType activities_model.ActionType
31+
Content string
32+
Comment *issues_model.Comment
33+
ForceDoerNotification bool
3334
}
3435

3536
const (
@@ -93,7 +94,7 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*user_mo
9394
visited := make(container.Set[int64], len(unfiltered)+len(mentions)+1)
9495

9596
// Avoid mailing the doer
96-
if ctx.Doer.EmailNotificationsPreference != user_model.EmailNotificationsAndYourOwn {
97+
if ctx.Doer.EmailNotificationsPreference != user_model.EmailNotificationsAndYourOwn && !ctx.ForceDoerNotification {
9798
visited.Add(ctx.Doer.ID)
9899
}
99100

@@ -181,17 +182,19 @@ func MailParticipants(issue *issues_model.Issue, doer *user_model.User, opType a
181182
content := issue.Content
182183
if opType == activities_model.ActionCloseIssue || opType == activities_model.ActionClosePullRequest ||
183184
opType == activities_model.ActionReopenIssue || opType == activities_model.ActionReopenPullRequest ||
184-
opType == activities_model.ActionMergePullRequest {
185+
opType == activities_model.ActionMergePullRequest || opType == activities_model.ActionAutoMergePullRequest {
185186
content = ""
186187
}
188+
forceDoerNotification := opType == activities_model.ActionAutoMergePullRequest
187189
if err := mailIssueCommentToParticipants(
188190
&mailCommentContext{
189-
Context: context.TODO(), // TODO: use a correct context
190-
Issue: issue,
191-
Doer: doer,
192-
ActionType: opType,
193-
Content: content,
194-
Comment: nil,
191+
Context: context.TODO(), // TODO: use a correct context
192+
Issue: issue,
193+
Doer: doer,
194+
ActionType: opType,
195+
Content: content,
196+
Comment: nil,
197+
ForceDoerNotification: forceDoerNotification,
195198
}, mentions); err != nil {
196199
log.Error("mailIssueCommentToParticipants: %v", err)
197200
}

‎services/pull/merge.go

+6-2
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ func GetDefaultMergeMessage(baseGitRepo *git.Repository, pr *issues_model.PullRe
133133

134134
// Merge merges pull request to base repository.
135135
// Caller should check PR is ready to be merged (review and status checks)
136-
func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) error {
136+
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 {
137137
if err := pr.LoadHeadRepo(); err != nil {
138138
log.Error("LoadHeadRepo: %v", err)
139139
return fmt.Errorf("LoadHeadRepo: %w", err)
@@ -193,7 +193,11 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
193193
log.Error("GetOwner for issue repo [%d]: %v", pr.ID, err)
194194
}
195195

196-
notification.NotifyMergePullRequest(pr, doer)
196+
if wasAutoMerged {
197+
notification.NotifyAutoMergePullRequest(pr, doer)
198+
} else {
199+
notification.NotifyMergePullRequest(pr, doer)
200+
}
197201

198202
// Reset cached commit count
199203
cache.Remove(pr.Issue.Repo.GetCommitsCountCacheKey(pr.BaseBranch, true))

‎tests/integration/pull_merge_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -245,11 +245,11 @@ func TestCantMergeConflict(t *testing.T) {
245245
gitRepo, err := git.OpenRepository(git.DefaultContext, repo_model.RepoPath(user1.Name, repo1.Name))
246246
assert.NoError(t, err)
247247

248-
err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "CONFLICT")
248+
err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "CONFLICT", false)
249249
assert.Error(t, err, "Merge should return an error due to conflict")
250250
assert.True(t, models.IsErrMergeConflicts(err), "Merge error is not a conflict error")
251251

252-
err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleRebase, "", "CONFLICT")
252+
err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleRebase, "", "CONFLICT", false)
253253
assert.Error(t, err, "Merge should return an error due to conflict")
254254
assert.True(t, models.IsErrRebaseConflicts(err), "Merge error is not a conflict error")
255255
gitRepo.Close()
@@ -344,7 +344,7 @@ func TestCantMergeUnrelated(t *testing.T) {
344344
BaseBranch: "base",
345345
})
346346

347-
err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "UNRELATED")
347+
err = pull.Merge(context.Background(), pr, user1, gitRepo, repo_model.MergeStyleMerge, "", "UNRELATED", false)
348348
assert.Error(t, err, "Merge should return an error due to unrelated")
349349
assert.True(t, models.IsErrMergeUnrelatedHistories(err), "Merge error is not a unrelated histories error")
350350
gitRepo.Close()

0 commit comments

Comments
 (0)