From 23bbec44591836171e2b8ad3dd2577016fd47e72 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Fri, 17 May 2024 16:45:41 +0200 Subject: [PATCH 1/3] tests(services/mailer): refactor mail_admin_new_user_test * use MockVariableValue where appropriate * split the tests in two with t.Run for clarity --- services/mailer/mail_admin_new_user.go | 5 +- services/mailer/mail_admin_new_user_test.go | 73 ++++++++------------- services/mailer/main_test.go | 32 +++++++++ 3 files changed, 61 insertions(+), 49 deletions(-) diff --git a/services/mailer/mail_admin_new_user.go b/services/mailer/mail_admin_new_user.go index aa0571e57c..54287b1b7e 100644 --- a/services/mailer/mail_admin_new_user.go +++ b/services/mailer/mail_admin_new_user.go @@ -19,14 +19,11 @@ const ( tplNewUserMail base.TplName = "notify/admin_new_user" ) -var sa = SendAsync - // MailNewUser sends notification emails on new user registrations to all admins func MailNewUser(ctx context.Context, u *user_model.User) { if !setting.Admin.SendNotificationEmailOnNewUser { return } - if setting.MailService == nil { // No mail service configured return @@ -77,5 +74,5 @@ func mailNewUser(ctx context.Context, u *user_model.User, lang string, tos []str msg.Info = subject msgs = append(msgs, msg) } - sa(msgs...) + SendAsync(msgs...) } diff --git a/services/mailer/mail_admin_new_user_test.go b/services/mailer/mail_admin_new_user_test.go index b89d888ee1..603a8b95c9 100644 --- a/services/mailer/mail_admin_new_user_test.go +++ b/services/mailer/mail_admin_new_user_test.go @@ -11,10 +11,12 @@ import ( "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/translation" + "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + _ "github.com/mattn/go-sqlite3" ) func getTestUsers(t *testing.T) []*user_model.User { @@ -45,54 +47,35 @@ func cleanUpUsers(ctx context.Context, users []*user_model.User) { } func TestAdminNotificationMail_test(t *testing.T) { - translation.InitLocales(context.Background()) - locale := translation.NewLocale("") - key := "mail.admin.new_user.user_info" - translatedKey := locale.Tr(key) - require.NotEqualValues(t, key, translatedKey) - - mailService := setting.Mailer{ - From: "test@example.com", - Protocol: "dummy", - } - - setting.MailService = &mailService - - // test with SEND_NOTIFICATION_EMAIL_ON_NEW_USER enabled - setting.Admin.SendNotificationEmailOnNewUser = true - ctx := context.Background() - NewContext(ctx) users := getTestUsers(t) - oldSendAsync := sa - defer func() { - sa = oldSendAsync - cleanUpUsers(ctx, users) - }() - called := false - sa = func(msgs ...*Message) { - assert.Equal(t, len(msgs), 1, "Test provides only one admin user, so only one email must be sent") - assert.Equal(t, msgs[0].To, users[0].Email, "checks if the recipient is the admin of the instance") - manageUserURL := setting.AppURL + "admin/users/" + strconv.FormatInt(users[1].ID, 10) - assert.Contains(t, msgs[0].Body, manageUserURL) - assert.Contains(t, msgs[0].Body, users[1].HTMLURL()) - assert.Contains(t, msgs[0].Body, translatedKey, "the .Locale translates to nothing") - assert.Contains(t, msgs[0].Body, users[1].Name, "user name of the newly created user") - for _, untranslated := range []string{"mail.admin", "admin.users"} { - assert.NotContains(t, msgs[0].Body, untranslated, "this is an untranslated placeholder prefix") - } - called = true - } - MailNewUser(ctx, users[1]) - assert.True(t, called) + t.Run("SendNotificationEmailOnNewUser_true", func(t *testing.T) { + defer test.MockVariableValue(&setting.Admin.SendNotificationEmailOnNewUser, true)() - // test with SEND_NOTIFICATION_EMAIL_ON_NEW_USER disabled; emails shouldn't be sent - setting.Admin.SendNotificationEmailOnNewUser = false - sa = func(msgs ...*Message) { - assert.Equal(t, 1, 0, "this shouldn't execute. MailNewUser must exit early since SEND_NOTIFICATION_EMAIL_ON_NEW_USER is disabled") - } + called := false + defer mockMailSettings(func(msgs ...*Message) { + assert.Equal(t, len(msgs), 1, "Test provides only one admin user, so only one email must be sent") + assert.Equal(t, msgs[0].To, users[0].Email, "checks if the recipient is the admin of the instance") + manageUserURL := setting.AppURL + "admin/users/" + strconv.FormatInt(users[1].ID, 10) + assert.Contains(t, msgs[0].Body, manageUserURL) + assert.Contains(t, msgs[0].Body, users[1].HTMLURL()) + assert.Contains(t, msgs[0].Body, users[1].Name, "user name of the newly created user") + assertTranslatedLocale(t, msgs[0].Body, "mail.admin", "admin.users") + called = true + })() + MailNewUser(ctx, users[1]) + assert.True(t, called) + }) - MailNewUser(ctx, users[1]) + t.Run("SendNotificationEmailOnNewUser_false", func(t *testing.T) { + defer test.MockVariableValue(&setting.Admin.SendNotificationEmailOnNewUser, false)() + defer mockMailSettings(func(msgs ...*Message) { + assert.Equal(t, 1, 0, "this shouldn't execute. MailNewUser must exit early since SEND_NOTIFICATION_EMAIL_ON_NEW_USER is disabled") + })() + MailNewUser(ctx, users[1]) + }) + + cleanUpUsers(ctx, users) } diff --git a/services/mailer/main_test.go b/services/mailer/main_test.go index f803c736ca..399d05ac7b 100644 --- a/services/mailer/main_test.go +++ b/services/mailer/main_test.go @@ -4,13 +4,45 @@ package mailer import ( + "context" "testing" "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/templates" + "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/modules/translation" _ "code.gitea.io/gitea/models/actions" + + "github.com/stretchr/testify/assert" ) func TestMain(m *testing.M) { unittest.MainTest(m) } + +func assertTranslatedLocale(t *testing.T, message string, prefixes ...string) { + t.Helper() + for _, prefix := range prefixes { + assert.NotContains(t, message, prefix, "there is an untranslated locale prefix") + } +} + +func mockMailSettings(send func(msgs ...*Message)) func() { + translation.InitLocales(context.Background()) + subjectTemplates, bodyTemplates = templates.Mailer(context.Background()) + mailService := setting.Mailer{ + From: "test@gitea.com", + } + cleanups := []func(){ + test.MockVariableValue(&setting.MailService, &mailService), + test.MockVariableValue(&setting.Domain, "localhost"), + test.MockVariableValue(&SendAsync, send), + } + return func() { + for _, cleanup := range cleanups { + cleanup() + } + } +} From 55c850a8a8fe9e2d1fc57619ec5f04bd404e8dfd Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Fri, 17 May 2024 16:47:35 +0200 Subject: [PATCH 2/3] tests(services/mailer): coverage for the issue/default.tmpl logic * the tests fail when issue/default.tmpl is removed * coverage for: * activities_model.ActionCreateIssue * activities_model.ActionCommentIssue * activities_model.ActionCloseIssue * activities_model.ActionReopenIssue * activities_model.ActionCommentPull * activities_model.ActionMergePullRequest * activities_model.ActionApprovePullRequest * activities_model.ActionRejectPullRequest * replace mocks with calls to mockMailSettings --- services/mailer/mail_test.go | 116 +++++++++++++++++++++++++++++++---- services/mailer/mailer.go | 2 - 2 files changed, 104 insertions(+), 14 deletions(-) diff --git a/services/mailer/mail_test.go b/services/mailer/mail_test.go index d87c57ffe7..c026a52e7a 100644 --- a/services/mailer/mail_test.go +++ b/services/mailer/mail_test.go @@ -23,6 +23,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/markup" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" ) @@ -52,12 +53,6 @@ const bodyTpl = ` func prepareMailerTest(t *testing.T) (doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, comment *issues_model.Comment) { assert.NoError(t, unittest.PrepareTestDatabase()) - mailService := setting.Mailer{ - From: "test@gitea.com", - } - - setting.MailService = &mailService - setting.Domain = "localhost" doer = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1, Owner: doer}) @@ -68,6 +63,7 @@ func prepareMailerTest(t *testing.T) (doer *user_model.User, repo *repo_model.Re } func TestComposeIssueCommentMessage(t *testing.T) { + defer mockMailSettings(nil)() doer, _, issue, comment := prepareMailerTest(t) markup.Init(&markup.ProcessorHelper{ @@ -76,8 +72,7 @@ func TestComposeIssueCommentMessage(t *testing.T) { }, }) - setting.IncomingEmail.Enabled = true - defer func() { setting.IncomingEmail.Enabled = false }() + defer test.MockVariableValue(&setting.IncomingEmail.Enabled, true)() subjectTemplates = texttmpl.Must(texttmpl.New("issue/comment").Parse(subjectTpl)) bodyTemplates = template.Must(template.New("issue/comment").Parse(bodyTpl)) @@ -123,11 +118,9 @@ func TestComposeIssueCommentMessage(t *testing.T) { } func TestComposeIssueMessage(t *testing.T) { + defer mockMailSettings(nil)() doer, _, issue, _ := prepareMailerTest(t) - subjectTemplates = texttmpl.Must(texttmpl.New("issue/new").Parse(subjectTpl)) - bodyTemplates = template.Must(template.New("issue/new").Parse(bodyTpl)) - recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}, {Name: "Test2", Email: "test2@gitea.com"}} msgs, err := composeIssueCommentMessages(&mailCommentContext{ Context: context.TODO(), // TODO: use a correct context @@ -145,7 +138,7 @@ func TestComposeIssueMessage(t *testing.T) { references := gomailMsg.GetHeader("References") assert.Len(t, mailto, 1, "exactly one recipient is expected in the To field") - assert.Equal(t, "[user2/repo1] @user2 #1 - issue1", subject[0]) + assert.Equal(t, "[user2/repo1] issue1 (#1)", subject[0]) assert.Equal(t, "", inReplyTo[0], "In-Reply-To header doesn't match") assert.Equal(t, "", references[0], "References header doesn't match") assert.Equal(t, "", messageID[0], "Message-ID header doesn't match") @@ -153,7 +146,103 @@ func TestComposeIssueMessage(t *testing.T) { assert.Len(t, gomailMsg.GetHeader("List-Unsubscribe"), 1) // url without mailto } +func TestMailerIssueTemplate(t *testing.T) { + defer mockMailSettings(nil)() + assert.NoError(t, unittest.PrepareTestDatabase()) + + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + expect := func(t *testing.T, msg *Message, issue *issues_model.Issue, expected ...string) { + subject := msg.ToMessage().GetHeader("Subject") + msgbuf := new(bytes.Buffer) + _, _ = msg.ToMessage().WriteTo(msgbuf) + wholemsg := msgbuf.String() + assert.Contains(t, subject[0], fallbackMailSubject(issue)) + for _, s := range expected { + assert.Contains(t, wholemsg, s) + } + assertTranslatedLocale(t, wholemsg, "mail.issue") + } + + testCompose := func(t *testing.T, ctx *mailCommentContext) *Message { + t.Helper() + recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}} + + ctx.Context = context.Background() + fromMention := false + msgs, err := composeIssueCommentMessages(ctx, "en-US", recipients, fromMention, "TestMailerIssueTemplate") + assert.NoError(t, err) + assert.Len(t, msgs, 1) + return msgs[0] + } + + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1}) + assert.NoError(t, issue.LoadRepo(db.DefaultContext)) + + msg := testCompose(t, &mailCommentContext{ + Issue: issue, Doer: doer, ActionType: activities_model.ActionCreateIssue, + Content: issue.Content, + }) + expect(t, msg, issue, issue.Content) + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 2, Issue: issue}) + + msg = testCompose(t, &mailCommentContext{ + Issue: issue, Doer: doer, ActionType: activities_model.ActionCommentIssue, + Content: comment.Content, Comment: comment, + }) + expect(t, msg, issue, comment.Content) + + msg = testCompose(t, &mailCommentContext{ + Issue: issue, Doer: doer, ActionType: activities_model.ActionCloseIssue, + Content: comment.Content, Comment: comment, + }) + expect(t, msg, issue, comment.Content) + + msg = testCompose(t, &mailCommentContext{ + Issue: issue, Doer: doer, ActionType: activities_model.ActionReopenIssue, + Content: comment.Content, Comment: comment, + }) + expect(t, msg, issue, comment.Content) + + pull := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) + assert.NoError(t, pull.LoadAttributes(db.DefaultContext)) + pullComment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 4, Issue: pull}) + + msg = testCompose(t, &mailCommentContext{ + Issue: pull, Doer: doer, ActionType: activities_model.ActionCommentPull, + Content: pullComment.Content, Comment: pullComment, + }) + expect(t, msg, pull, pullComment.Content) + + msg = testCompose(t, &mailCommentContext{ + Issue: pull, Doer: doer, ActionType: activities_model.ActionMergePullRequest, + Content: pullComment.Content, Comment: pullComment, + }) + expect(t, msg, pull, pullComment.Content, pull.PullRequest.BaseBranch) + + reviewComment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 9}) + assert.NoError(t, reviewComment.LoadReview(db.DefaultContext)) + + approveComment := reviewComment + approveComment.Review.Type = issues_model.ReviewTypeApprove + msg = testCompose(t, &mailCommentContext{ + Issue: pull, Doer: doer, ActionType: activities_model.ActionApprovePullRequest, + Content: approveComment.Content, Comment: approveComment, + }) + expect(t, msg, pull, approveComment.Content) + + rejectComment := reviewComment + rejectComment.Review.Type = issues_model.ReviewTypeReject + msg = testCompose(t, &mailCommentContext{ + Issue: pull, Doer: doer, ActionType: activities_model.ActionRejectPullRequest, + Content: rejectComment.Content, Comment: rejectComment, + }) + expect(t, msg, pull, rejectComment.Content) +} + func TestTemplateSelection(t *testing.T) { + defer mockMailSettings(nil)() doer, repo, issue, comment := prepareMailerTest(t) recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}} @@ -208,6 +297,7 @@ func TestTemplateSelection(t *testing.T) { } func TestTemplateServices(t *testing.T) { + defer mockMailSettings(nil)() doer, _, issue, comment := prepareMailerTest(t) assert.NoError(t, issue.LoadRepo(db.DefaultContext)) @@ -260,6 +350,7 @@ func testComposeIssueCommentMessage(t *testing.T, ctx *mailCommentContext, recip } func TestGenerateAdditionalHeaders(t *testing.T) { + defer mockMailSettings(nil)() doer, _, issue, _ := prepareMailerTest(t) ctx := &mailCommentContext{Context: context.TODO() /* TODO: use a correct context */, Issue: issue, Doer: doer} @@ -289,6 +380,7 @@ func TestGenerateAdditionalHeaders(t *testing.T) { } func Test_createReference(t *testing.T) { + defer mockMailSettings(nil)() _, _, issue, comment := prepareMailerTest(t) _, _, pullIssue, _ := prepareMailerTest(t) pullIssue.IsPull = true diff --git a/services/mailer/mailer.go b/services/mailer/mailer.go index 5e8e3dbb38..a9316cca76 100644 --- a/services/mailer/mailer.go +++ b/services/mailer/mailer.go @@ -365,10 +365,8 @@ func (s *sendmailSender) Send(from string, to []string, msg io.WriterTo) error { return waitError } -// Sender sendmail mail sender type dummySender struct{} -// Send send email func (s *dummySender) Send(from string, to []string, msg io.WriterTo) error { buf := bytes.Buffer{} if _, err := msg.WriteTo(&buf); err != nil { From fc45a0d9bab15132e71bf09b624c5be63e439edc Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 18 May 2024 18:42:54 +0200 Subject: [PATCH 3/3] cleanup(services/mailer): mark deadcode for removal There is no activities_model.Action* when sending a review comment, this is deadcode and should be removed. Or a new event should be added to differentiate it from a regular comment when evaluating templates. --- services/mailer/mail.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/mailer/mail.go b/services/mailer/mail.go index 86bd40ff29..b04925881d 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -517,7 +517,7 @@ func actionToTemplate(issue *issues_model.Issue, actionType activities_model.Act case issues_model.ReviewTypeReject: name = "reject" default: - name = "review" + name = "review" // TODO: there is no activities_model.Action* when sending a review comment, this is deadcode and should be removed } case issues_model.CommentTypeCode: name = "code"