Skip to content

Commit

Permalink
feat: notify doers of a merge when automerging (#21553)
Browse files Browse the repository at this point in the history
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 <art27@cantab.net>
Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
  • Loading branch information
4 people authored Nov 3, 2022
1 parent f17edfa commit 085f717
Show file tree
Hide file tree
Showing 18 changed files with 87 additions and 27 deletions.
3 changes: 2 additions & 1 deletion models/activities/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ const (
ActionPublishRelease // 24
ActionPullReviewDismissed // 25
ActionPullRequestReadyForReview // 26
ActionAutoMergePullRequest // 27
)

// Action represents user operation type and other information to
Expand Down Expand Up @@ -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
}
Expand Down
14 changes: 14 additions & 0 deletions modules/notification/action/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 2 additions & 1 deletion modules/notification/base/notifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions modules/notification/base/null.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
}
Expand Down
10 changes: 10 additions & 0 deletions modules/notification/mail/mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
7 changes: 7 additions & 0 deletions modules/notification/notification.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions modules/notification/ui/ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions modules/notification/webhook/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion modules/templates/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -3003,6 +3003,7 @@ reopen_pull_request = `reopened pull request <a href="%[1]s">%[3]s#%[2]s</a>`
comment_issue = `commented on issue <a href="%[1]s">%[3]s#%[2]s</a>`
comment_pull = `commented on pull request <a href="%[1]s">%[3]s#%[2]s</a>`
merge_pull_request = `merged pull request <a href="%[1]s">%[3]s#%[2]s</a>`
auto_merge_pull_request = `automatically merged pull request <a href="%[1]s">%[3]s#%[2]s</a>`
transfer_repo = transferred repository <code>%s</code> to <a href="%s">%s</a>
push_tag = pushed tag <a href="%[2]s">%[3]s</a> to <a href="%[1]s">%[4]s</a>
delete_tag = deleted tag %[2]s from <a href="%[1]s">%[3]s</a>
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
8 changes: 7 additions & 1 deletion routers/web/feed/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "#" {
Expand Down Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
2 changes: 1 addition & 1 deletion services/automerge/automerge.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions services/mailer/mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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"
Expand Down
29 changes: 16 additions & 13 deletions services/mailer/mail_issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}
Expand Down
8 changes: 6 additions & 2 deletions services/pull/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand Down
6 changes: 3 additions & 3 deletions tests/integration/pull_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit 085f717

Please sign in to comment.