From 1ab44cb01d43eb1c0b606ae842e701f5a8ad2b36 Mon Sep 17 00:00:00 2001
From: zeripath <art27@cantab.net>
Date: Thu, 3 Feb 2022 23:01:16 +0000
Subject: [PATCH] Prevent merge messages from being sorted to the top of email
chains (#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 #18560
Signed-off-by: Andrew Thornton <art27@cantab.net>
* add test
Signed-off-by: Andrew Thornton <art27@cantab.net>
---
services/mailer/mail.go | 20 +++++-
services/mailer/mail_test.go | 115 ++++++++++++++++++++++++++++++++++-
2 files changed, 131 insertions(+), 4 deletions(-)
diff --git a/services/mailer/mail.go b/services/mailer/mail.go
index e5aa250083..d5f3f2ac03 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)
diff --git a/services/mailer/mail_test.go b/services/mailer/mail_test.go
index 07690063cd..6c800e457b 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)
+ }
+ })
+ }
+}