Prevent merge messages from being sorted to the top of email chains (g…

…o-gitea#18566) * Prevent merge messages from being sorted to the top of email chains Gitea will currrently resend the same message-id for the closed/merged/reopened messages for issues. This will cause the merged message to leap to the top of an email chain and become out of sync. This PR adds specific suffices for these actions. Fix go-gitea#18560 Signed-off-by: Andrew Thornton <art27@cantab.net> * add test Signed-off-by: Andrew Thornton <art27@cantab.net>
zjjhot · Feb 3, 2022 · 1ab44cb · 1ab44cb
1 parent 9f9ca0a
commit 1ab44cb
Show file tree

Hide file tree

Showing 2 changed files with 131 additions and 4 deletions.
diff --git a/services/mailer/mail.go b/services/mailer/mail.go
@@ -14,6 +14,7 @@ import (
 	"strconv"
 	"strings"
 	texttmpl "text/template"
+	"time"
 
 	"code.gitea.io/gitea/models"
 	repo_model "code.gitea.io/gitea/models/repo"
@@ -298,13 +299,15 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient
 	}
 
 	// Make sure to compose independent messages to avoid leaking user emails
+	msgID := createReference(ctx.Issue, ctx.Comment, ctx.ActionType)
+	reference := createReference(ctx.Issue, nil, models.ActionType(0))
+
 	msgs := make([]*Message, 0, len(recipients))
 	for _, recipient := range recipients {
 		msg := NewMessageFrom([]string{recipient.Email}, ctx.Doer.DisplayName(), setting.MailService.FromEmail, subject, mailBody.String())
 		msg.Info = fmt.Sprintf("Subject: %s, %s", subject, info)
 
-		msg.SetHeader("Message-ID", "<"+createReference(ctx.Issue, ctx.Comment)+">")
-		reference := createReference(ctx.Issue, nil)
+		msg.SetHeader("Message-ID", "<"+msgID+">")
 		msg.SetHeader("In-Reply-To", "<"+reference+">")
 		msg.SetHeader("References", "<"+reference+">")
 
@@ -318,7 +321,7 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient
 	return msgs, nil
 }
 
-func createReference(issue *models.Issue, comment *models.Comment) string {
+func createReference(issue *models.Issue, comment *models.Comment, actionType models.ActionType) string {
 	var path string
 	if issue.IsPull {
 		path = "pulls"
@@ -329,6 +332,17 @@ func createReference(issue *models.Issue, comment *models.Comment) string {
 	var extra string
 	if comment != nil {
 		extra = fmt.Sprintf("/comment/%d", comment.ID)
+	} else {
+		switch actionType {
+		case models.ActionCloseIssue, models.ActionClosePullRequest:
+			extra = fmt.Sprintf("/close/%d", time.Now().UnixNano()/1e6)
+		case models.ActionReopenIssue, models.ActionReopenPullRequest:
+			extra = fmt.Sprintf("/reopen/%d", time.Now().UnixNano()/1e6)
+		case models.ActionMergePullRequest:
+			extra = fmt.Sprintf("/merge/%d", time.Now().UnixNano()/1e6)
+		case models.ActionPullRequestReadyForReview:
+			extra = fmt.Sprintf("/ready/%d", time.Now().UnixNano()/1e6)
+		}
 	}
 
 	return fmt.Sprintf("%s/%s/%d%s@%s", issue.Repo.FullName(), path, issue.Index, extra, setting.Domain)

diff --git a/services/mailer/mail_test.go b/services/mailer/mail_test.go
@@ -6,7 +6,9 @@ package mailer
 
 import (
 	"bytes"
+	"fmt"
 	"html/template"
+	"strings"
 	"testing"
 	texttmpl "text/template"
 
@@ -15,7 +17,6 @@ import (
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
-
 	"github.com/stretchr/testify/assert"
 )
 
@@ -250,3 +251,115 @@ func TestGenerateAdditionalHeaders(t *testing.T) {
 		}
 	}
 }
+
+func Test_createReference(t *testing.T) {
+	_, _, issue, comment := prepareMailerTest(t)
+	_, _, pullIssue, _ := prepareMailerTest(t)
+	pullIssue.IsPull = true
+
+	type args struct {
+		issue      *models.Issue
+		comment    *models.Comment
+		actionType models.ActionType
+	}
+	tests := []struct {
+		name   string
+		args   args
+		prefix string
+		suffix string
+	}{
+		{
+			name: "Open Issue",
+			args: args{
+				issue:      issue,
+				actionType: models.ActionCreateIssue,
+			},
+			prefix: fmt.Sprintf("%s/issues/%d@%s", issue.Repo.FullName(), issue.Index, setting.Domain),
+		},
+		{
+			name: "Open Pull",
+			args: args{
+				issue:      pullIssue,
+				actionType: models.ActionCreatePullRequest,
+			},
+			prefix: fmt.Sprintf("%s/pulls/%d@%s", issue.Repo.FullName(), issue.Index, setting.Domain),
+		},
+		{
+			name: "Comment Issue",
+			args: args{
+				issue:      issue,
+				comment:    comment,
+				actionType: models.ActionCommentIssue,
+			},
+			prefix: fmt.Sprintf("%s/issues/%d/comment/%d@%s", issue.Repo.FullName(), issue.Index, comment.ID, setting.Domain),
+		},
+		{
+			name: "Comment Pull",
+			args: args{
+				issue:      pullIssue,
+				comment:    comment,
+				actionType: models.ActionCommentPull,
+			},
+			prefix: fmt.Sprintf("%s/pulls/%d/comment/%d@%s", issue.Repo.FullName(), issue.Index, comment.ID, setting.Domain),
+		},
+		{
+			name: "Close Issue",
+			args: args{
+				issue:      issue,
+				actionType: models.ActionCloseIssue,
+			},
+			prefix: fmt.Sprintf("%s/issues/%d/close/", issue.Repo.FullName(), issue.Index),
+		},
+		{
+			name: "Close Pull",
+			args: args{
+				issue:      pullIssue,
+				actionType: models.ActionClosePullRequest,
+			},
+			prefix: fmt.Sprintf("%s/pulls/%d/close/", issue.Repo.FullName(), issue.Index),
+		},
+		{
+			name: "Reopen Issue",
+			args: args{
+				issue:      issue,
+				actionType: models.ActionReopenIssue,
+			},
+			prefix: fmt.Sprintf("%s/issues/%d/reopen/", issue.Repo.FullName(), issue.Index),
+		},
+		{
+			name: "Reopen Pull",
+			args: args{
+				issue:      pullIssue,
+				actionType: models.ActionReopenPullRequest,
+			},
+			prefix: fmt.Sprintf("%s/pulls/%d/reopen/", issue.Repo.FullName(), issue.Index),
+		},
+		{
+			name: "Merge Pull",
+			args: args{
+				issue:      pullIssue,
+				actionType: models.ActionMergePullRequest,
+			},
+			prefix: fmt.Sprintf("%s/pulls/%d/merge/", issue.Repo.FullName(), issue.Index),
+		},
+		{
+			name: "Ready Pull",
+			args: args{
+				issue:      pullIssue,
+				actionType: models.ActionPullRequestReadyForReview,
+			},
+			prefix: fmt.Sprintf("%s/pulls/%d/ready/", issue.Repo.FullName(), issue.Index),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := createReference(tt.args.issue, tt.args.comment, tt.args.actionType)
+			if !strings.HasPrefix(got, tt.prefix) {
+				t.Errorf("createReference() = %v, want %v", got, tt.prefix)
+			}
+			if !strings.HasSuffix(got, tt.suffix) {
+				t.Errorf("createReference() = %v, want %v", got, tt.prefix)
+			}
+		})
+	}
+}