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

feat: notify doers of a merge when automerging #21553

Merged
merged 7 commits into from
Nov 3, 2022
2 changes: 1 addition & 1 deletion modules/notification/action/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ func (a *actionNotifier) NotifyPullRequestReview(pr *issues_model.PullRequest, r
}
}

func (*actionNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
func (*actionNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User, wasAutoMerged bool) {
if err := activities_model.NotifyWatchers(&activities_model.Action{
ActUserID: doer.ID,
ActUser: doer,
Expand Down
2 changes: 1 addition & 1 deletion modules/notification/base/notifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ 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, wasAutoMerged bool)
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
2 changes: 1 addition & 1 deletion modules/notification/base/null.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (*NullNotifier) NotifyPullRequestCodeComment(pr *issues_model.PullRequest,
}

// NotifyMergePullRequest places a place holder function
func (*NullNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
func (*NullNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User, b bool) {
}

// NotifyPullRequestSynchronized places a place holder function
Expand Down
12 changes: 6 additions & 6 deletions modules/notification/mail/mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func (m *mailNotifier) NotifyCreateIssueComment(doer *user_model.User, repo *rep
}

func (m *mailNotifier) NotifyNewIssue(issue *issues_model.Issue, mentions []*user_model.User) {
if err := mailer.MailParticipants(issue, issue.Poster, activities_model.ActionCreateIssue, mentions); err != nil {
if err := mailer.MailParticipants(issue, issue.Poster, activities_model.ActionCreateIssue, mentions, false); err != nil {
log.Error("MailParticipants: %v", err)
}
}
Expand All @@ -75,7 +75,7 @@ func (m *mailNotifier) NotifyIssueChangeStatus(doer *user_model.User, issue *iss
}
}

if err := mailer.MailParticipants(issue, doer, actionType, nil); err != nil {
if err := mailer.MailParticipants(issue, doer, actionType, nil, false); err != nil {
log.Error("MailParticipants: %v", err)
}
}
Expand All @@ -86,14 +86,14 @@ func (m *mailNotifier) NotifyIssueChangeTitle(doer *user_model.User, issue *issu
return
}
if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() {
if err := mailer.MailParticipants(issue, doer, activities_model.ActionPullRequestReadyForReview, nil); err != nil {
if err := mailer.MailParticipants(issue, doer, activities_model.ActionPullRequestReadyForReview, nil, false); err != nil {
log.Error("MailParticipants: %v", err)
}
}
}

func (m *mailNotifier) NotifyNewPullRequest(pr *issues_model.PullRequest, mentions []*user_model.User) {
if err := mailer.MailParticipants(pr.Issue, pr.Issue.Poster, activities_model.ActionCreatePullRequest, mentions); err != nil {
if err := mailer.MailParticipants(pr.Issue, pr.Issue.Poster, activities_model.ActionCreatePullRequest, mentions, false); err != nil {
log.Error("MailParticipants: %v", err)
}
}
Expand Down Expand Up @@ -143,12 +143,12 @@ func (m *mailNotifier) NotifyPullReviewRequest(doer *user_model.User, issue *iss
}
}

func (m *mailNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
func (m *mailNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User, wasAutoMerged bool) {
if err := pr.LoadIssue(); err != nil {
log.Error("pr.LoadIssue: %v", err)
return
}
if err := mailer.MailParticipants(pr.Issue, doer, activities_model.ActionMergePullRequest, nil); err != nil {
if err := mailer.MailParticipants(pr.Issue, doer, activities_model.ActionMergePullRequest, nil, wasAutoMerged); err != nil {
log.Error("MailParticipants: %v", err)
}
}
Expand Down
4 changes: 2 additions & 2 deletions modules/notification/notification.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ func NotifyDeleteIssue(doer *user_model.User, issue *issues_model.Issue) {
}

// NotifyMergePullRequest notifies merge pull request to notifiers
func NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
func NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User, wasAutoMerged bool) {
for _, notifier := range notifiers {
notifier.NotifyMergePullRequest(pr, doer)
notifier.NotifyMergePullRequest(pr, doer, wasAutoMerged)
}
}

Expand Down
2 changes: 1 addition & 1 deletion modules/notification/ui/ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func (ns *notificationService) NotifyIssueChangeTitle(doer *user_model.User, iss
}
}

func (ns *notificationService) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
func (ns *notificationService) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User, wasAutoMerged bool) {
_ = ns.issueQueue.Push(issueNotificationOpts{
IssueID: pr.Issue.ID,
NotificationAuthorID: doer.ID,
Expand Down
2 changes: 1 addition & 1 deletion modules/notification/webhook/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ func (m *webhookNotifier) NotifyPushCommits(pusher *user_model.User, repo *repo_
}
}

func (*webhookNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User) {
func (*webhookNotifier) NotifyMergePullRequest(pr *issues_model.PullRequest, doer *user_model.User, wasAutoMerged bool) {
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 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
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
28 changes: 15 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 @@ -172,7 +173,7 @@ func mailIssueCommentBatch(ctx *mailCommentContext, users []*user_model.User, vi

// MailParticipants sends new issue thread created emails to repository watchers
// and mentioned people.
func MailParticipants(issue *issues_model.Issue, doer *user_model.User, opType activities_model.ActionType, mentions []*user_model.User) error {
func MailParticipants(issue *issues_model.Issue, doer *user_model.User, opType activities_model.ActionType, mentions []*user_model.User, forceDoerNotification bool) error {
if setting.MailService == nil {
// No mail service configured
return nil
Expand All @@ -186,12 +187,13 @@ func MailParticipants(issue *issues_model.Issue, doer *user_model.User, opType a
}
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
2 changes: 1 addition & 1 deletion services/pull/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
return false
}

notification.NotifyMergePullRequest(pr, merger)
notification.NotifyMergePullRequest(pr, merger, false)

log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String())
return true
Expand Down
6 changes: 3 additions & 3 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: %v", err)
Expand Down Expand Up @@ -193,7 +193,7 @@ 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)
notification.NotifyMergePullRequest(pr, doer, wasAutoMerged)

// Reset cached commit count
cache.Remove(pr.Issue.Repo.GetCommitsCountCacheKey(pr.BaseBranch, true))
Expand Down Expand Up @@ -871,7 +871,7 @@ func MergedManually(pr *issues_model.PullRequest, doer *user_model.User, baseGit
return err
}

notification.NotifyMergePullRequest(pr, doer)
notification.NotifyMergePullRequest(pr, doer, false)
log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commitID)
return nil
}
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