From e0a6afb0f70759ad9c2f42ff44cbd41676cbbd87 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 2 Feb 2022 23:47:51 +0000 Subject: [PATCH 1/2] 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 #18560 Signed-off-by: Andrew Thornton --- services/mailer/mail.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/services/mailer/mail.go b/services/mailer/mail.go index e5aa2500837b6..d5f3f2ac03798 100644 --- 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) From a9e9fad251d9361c4da54d7a3f0ee3141938feb6 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 3 Feb 2022 15:53:55 +0000 Subject: [PATCH 2/2] add test Signed-off-by: Andrew Thornton --- services/mailer/mail_test.go | 115 ++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-) diff --git a/services/mailer/mail_test.go b/services/mailer/mail_test.go index 07690063cd297..6c800e457b0c2 100644 --- 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) + } + }) + } +}