From 55f1fcf0ad20d69fcd62c764f6da74ae56bd5f73 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Tue, 17 Sep 2024 22:56:26 +0200 Subject: [PATCH] Add missing comment reply handling (#32050) Fixes #31937 - Add missing comment reply handling - Use `onGiteaRun` in the test because the fixtures are not present otherwise (did this behaviour change?) Compare without whitespaces. --- services/mailer/incoming/incoming_handler.go | 56 ++-- tests/integration/incoming_email_test.go | 282 ++++++++++--------- 2 files changed, 172 insertions(+), 166 deletions(-) diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index dc0b53982216d..38a234eac1fd2 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/services/mailer/incoming/incoming_handler.go @@ -82,43 +82,40 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u return nil } - switch r := ref.(type) { - case *issues_model.Issue: - attachmentIDs := make([]string, 0, len(content.Attachments)) - if setting.Attachment.Enabled { - for _, attachment := range content.Attachments { - a, err := attachment_service.UploadAttachment(ctx, bytes.NewReader(attachment.Content), setting.Attachment.AllowedTypes, int64(len(attachment.Content)), &repo_model.Attachment{ - Name: attachment.Name, - UploaderID: doer.ID, - RepoID: issue.Repo.ID, - }) - if err != nil { - if upload.IsErrFileTypeForbidden(err) { - log.Info("Skipping disallowed attachment type: %s", attachment.Name) - continue - } - return err + attachmentIDs := make([]string, 0, len(content.Attachments)) + if setting.Attachment.Enabled { + for _, attachment := range content.Attachments { + a, err := attachment_service.UploadAttachment(ctx, bytes.NewReader(attachment.Content), setting.Attachment.AllowedTypes, int64(len(attachment.Content)), &repo_model.Attachment{ + Name: attachment.Name, + UploaderID: doer.ID, + RepoID: issue.Repo.ID, + }) + if err != nil { + if upload.IsErrFileTypeForbidden(err) { + log.Info("Skipping disallowed attachment type: %s", attachment.Name) + continue } - attachmentIDs = append(attachmentIDs, a.UUID) + return err } + attachmentIDs = append(attachmentIDs, a.UUID) } + } - if content.Content == "" && len(attachmentIDs) == 0 { - return nil - } + if content.Content == "" && len(attachmentIDs) == 0 { + return nil + } - _, err = issue_service.CreateIssueComment(ctx, doer, issue.Repo, issue, content.Content, attachmentIDs) + switch r := ref.(type) { + case *issues_model.Issue: + _, err := issue_service.CreateIssueComment(ctx, doer, issue.Repo, issue, content.Content, attachmentIDs) if err != nil { return fmt.Errorf("CreateIssueComment failed: %w", err) } case *issues_model.Comment: comment := r - if content.Content == "" { - return nil - } - - if comment.Type == issues_model.CommentTypeCode { + switch comment.Type { + case issues_model.CommentTypeCode: _, err := pull_service.CreateCodeComment( ctx, doer, @@ -130,11 +127,16 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u false, // not pending review but a single review comment.ReviewID, "", - nil, + attachmentIDs, ) if err != nil { return fmt.Errorf("CreateCodeComment failed: %w", err) } + default: + _, err := issue_service.CreateIssueComment(ctx, doer, issue.Repo, issue, content.Content, attachmentIDs) + if err != nil { + return fmt.Errorf("CreateIssueComment failed: %w", err) + } } } return nil diff --git a/tests/integration/incoming_email_test.go b/tests/integration/incoming_email_test.go index 12848338648af..88571303ac72b 100644 --- a/tests/integration/incoming_email_test.go +++ b/tests/integration/incoming_email_test.go @@ -7,6 +7,7 @@ import ( "io" "net" "net/smtp" + "net/url" "strings" "testing" "time" @@ -26,187 +27,190 @@ import ( ) func TestIncomingEmail(t *testing.T) { - defer tests.PrepareTestEnv(t)() + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1}) - user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1}) + t.Run("Payload", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() - t.Run("Payload", func(t *testing.T) { - defer tests.PrintCurrentTest(t)() + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 1}) - comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 1}) + _, err := incoming_payload.CreateReferencePayload(user) + assert.Error(t, err) - _, err := incoming_payload.CreateReferencePayload(user) - assert.Error(t, err) + issuePayload, err := incoming_payload.CreateReferencePayload(issue) + assert.NoError(t, err) + commentPayload, err := incoming_payload.CreateReferencePayload(comment) + assert.NoError(t, err) - issuePayload, err := incoming_payload.CreateReferencePayload(issue) - assert.NoError(t, err) - commentPayload, err := incoming_payload.CreateReferencePayload(comment) - assert.NoError(t, err) + _, err = incoming_payload.GetReferenceFromPayload(db.DefaultContext, []byte{1, 2, 3}) + assert.Error(t, err) - _, err = incoming_payload.GetReferenceFromPayload(db.DefaultContext, []byte{1, 2, 3}) - assert.Error(t, err) + ref, err := incoming_payload.GetReferenceFromPayload(db.DefaultContext, issuePayload) + assert.NoError(t, err) + assert.IsType(t, ref, new(issues_model.Issue)) + assert.EqualValues(t, issue.ID, ref.(*issues_model.Issue).ID) - ref, err := incoming_payload.GetReferenceFromPayload(db.DefaultContext, issuePayload) - assert.NoError(t, err) - assert.IsType(t, ref, new(issues_model.Issue)) - assert.EqualValues(t, issue.ID, ref.(*issues_model.Issue).ID) + ref, err = incoming_payload.GetReferenceFromPayload(db.DefaultContext, commentPayload) + assert.NoError(t, err) + assert.IsType(t, ref, new(issues_model.Comment)) + assert.EqualValues(t, comment.ID, ref.(*issues_model.Comment).ID) + }) - ref, err = incoming_payload.GetReferenceFromPayload(db.DefaultContext, commentPayload) - assert.NoError(t, err) - assert.IsType(t, ref, new(issues_model.Comment)) - assert.EqualValues(t, comment.ID, ref.(*issues_model.Comment).ID) - }) + t.Run("Token", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() - t.Run("Token", func(t *testing.T) { - defer tests.PrintCurrentTest(t)() + payload := []byte{1, 2, 3, 4, 5} - payload := []byte{1, 2, 3, 4, 5} + token, err := token_service.CreateToken(token_service.ReplyHandlerType, user, payload) + assert.NoError(t, err) + assert.NotEmpty(t, token) - token, err := token_service.CreateToken(token_service.ReplyHandlerType, user, payload) - assert.NoError(t, err) - assert.NotEmpty(t, token) + ht, u, p, err := token_service.ExtractToken(db.DefaultContext, token) + assert.NoError(t, err) + assert.Equal(t, token_service.ReplyHandlerType, ht) + assert.Equal(t, user.ID, u.ID) + assert.Equal(t, payload, p) + }) - ht, u, p, err := token_service.ExtractToken(db.DefaultContext, token) - assert.NoError(t, err) - assert.Equal(t, token_service.ReplyHandlerType, ht) - assert.Equal(t, user.ID, u.ID) - assert.Equal(t, payload, p) - }) + t.Run("Handler", func(t *testing.T) { + t.Run("Reply", func(t *testing.T) { + t.Run("Comment", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() - t.Run("Handler", func(t *testing.T) { - t.Run("Reply", func(t *testing.T) { - t.Run("Comment", func(t *testing.T) { - defer tests.PrintCurrentTest(t)() + handler := &incoming.ReplyHandler{} - handler := &incoming.ReplyHandler{} + payload, err := incoming_payload.CreateReferencePayload(issue) + assert.NoError(t, err) - payload, err := incoming_payload.CreateReferencePayload(issue) - assert.NoError(t, err) - - assert.Error(t, handler.Handle(db.DefaultContext, &incoming.MailContent{}, nil, payload)) - assert.NoError(t, handler.Handle(db.DefaultContext, &incoming.MailContent{}, user, payload)) + assert.Error(t, handler.Handle(db.DefaultContext, &incoming.MailContent{}, nil, payload)) + assert.NoError(t, handler.Handle(db.DefaultContext, &incoming.MailContent{}, user, payload)) - content := &incoming.MailContent{ - Content: "reply by mail", - Attachments: []*incoming.Attachment{ - { - Name: "attachment.txt", - Content: []byte("test"), + content := &incoming.MailContent{ + Content: "reply by mail", + Attachments: []*incoming.Attachment{ + { + Name: "attachment.txt", + Content: []byte("test"), + }, }, - }, - } + } + + assert.NoError(t, handler.Handle(db.DefaultContext, content, user, payload)) + + comments, err := issues_model.FindComments(db.DefaultContext, &issues_model.FindCommentsOptions{ + IssueID: issue.ID, + Type: issues_model.CommentTypeComment, + }) + assert.NoError(t, err) + assert.NotEmpty(t, comments) + comment := comments[len(comments)-1] + assert.Equal(t, user.ID, comment.PosterID) + assert.Equal(t, content.Content, comment.Content) + assert.NoError(t, comment.LoadAttachments(db.DefaultContext)) + assert.Len(t, comment.Attachments, 1) + attachment := comment.Attachments[0] + assert.Equal(t, content.Attachments[0].Name, attachment.Name) + assert.EqualValues(t, 4, attachment.Size) + }) - assert.NoError(t, handler.Handle(db.DefaultContext, content, user, payload)) + t.Run("CodeComment", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 6}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) - comments, err := issues_model.FindComments(db.DefaultContext, &issues_model.FindCommentsOptions{ - IssueID: issue.ID, - Type: issues_model.CommentTypeComment, + handler := &incoming.ReplyHandler{} + content := &incoming.MailContent{ + Content: "code reply by mail", + Attachments: []*incoming.Attachment{ + { + Name: "attachment.txt", + Content: []byte("test"), + }, + }, + } + + payload, err := incoming_payload.CreateReferencePayload(comment) + assert.NoError(t, err) + + assert.NoError(t, handler.Handle(db.DefaultContext, content, user, payload)) + + comments, err := issues_model.FindComments(db.DefaultContext, &issues_model.FindCommentsOptions{ + IssueID: issue.ID, + Type: issues_model.CommentTypeCode, + }) + assert.NoError(t, err) + assert.NotEmpty(t, comments) + comment = comments[len(comments)-1] + assert.Equal(t, user.ID, comment.PosterID) + assert.Equal(t, content.Content, comment.Content) + assert.NoError(t, comment.LoadAttachments(db.DefaultContext)) + assert.Len(t, comment.Attachments, 1) + attachment := comment.Attachments[0] + assert.Equal(t, content.Attachments[0].Name, attachment.Name) + assert.EqualValues(t, 4, attachment.Size) }) - assert.NoError(t, err) - assert.NotEmpty(t, comments) - comment := comments[len(comments)-1] - assert.Equal(t, user.ID, comment.PosterID) - assert.Equal(t, content.Content, comment.Content) - assert.NoError(t, comment.LoadAttachments(db.DefaultContext)) - assert.Len(t, comment.Attachments, 1) - attachment := comment.Attachments[0] - assert.Equal(t, content.Attachments[0].Name, attachment.Name) - assert.EqualValues(t, 4, attachment.Size) }) - t.Run("CodeComment", func(t *testing.T) { + t.Run("Unsubscribe", func(t *testing.T) { defer tests.PrintCurrentTest(t)() - comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 6}) - issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) + watching, err := issues_model.CheckIssueWatch(db.DefaultContext, user, issue) + assert.NoError(t, err) + assert.True(t, watching) + + handler := &incoming.UnsubscribeHandler{} - handler := &incoming.ReplyHandler{} content := &incoming.MailContent{ - Content: "code reply by mail", - Attachments: []*incoming.Attachment{ - { - Name: "attachment.txt", - Content: []byte("test"), - }, - }, + Content: "unsub me", } - payload, err := incoming_payload.CreateReferencePayload(comment) + payload, err := incoming_payload.CreateReferencePayload(issue) assert.NoError(t, err) assert.NoError(t, handler.Handle(db.DefaultContext, content, user, payload)) - comments, err := issues_model.FindComments(db.DefaultContext, &issues_model.FindCommentsOptions{ - IssueID: issue.ID, - Type: issues_model.CommentTypeCode, - }) + watching, err = issues_model.CheckIssueWatch(db.DefaultContext, user, issue) assert.NoError(t, err) - assert.NotEmpty(t, comments) - comment = comments[len(comments)-1] - assert.Equal(t, user.ID, comment.PosterID) - assert.Equal(t, content.Content, comment.Content) - assert.NoError(t, comment.LoadAttachments(db.DefaultContext)) - assert.Empty(t, comment.Attachments) + assert.False(t, watching) }) }) - t.Run("Unsubscribe", func(t *testing.T) { - defer tests.PrintCurrentTest(t)() - - watching, err := issues_model.CheckIssueWatch(db.DefaultContext, user, issue) - assert.NoError(t, err) - assert.True(t, watching) + if setting.IncomingEmail.Enabled { + // This test connects to the configured email server and is currently only enabled for MySql integration tests. + // It sends a reply to create a comment. If the comment is not detected after 10 seconds the test fails. + t.Run("IMAP", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() - handler := &incoming.UnsubscribeHandler{} + payload, err := incoming_payload.CreateReferencePayload(issue) + assert.NoError(t, err) + token, err := token_service.CreateToken(token_service.ReplyHandlerType, user, payload) + assert.NoError(t, err) - content := &incoming.MailContent{ - Content: "unsub me", - } + msg := gomail.NewMessage() + msg.SetHeader("To", strings.Replace(setting.IncomingEmail.ReplyToAddress, setting.IncomingEmail.TokenPlaceholder, token, 1)) + msg.SetHeader("From", user.Email) + msg.SetBody("text/plain", token) + err = gomail.Send(&smtpTestSender{}, msg) + assert.NoError(t, err) - payload, err := incoming_payload.CreateReferencePayload(issue) - assert.NoError(t, err) + assert.Eventually(t, func() bool { + comments, err := issues_model.FindComments(db.DefaultContext, &issues_model.FindCommentsOptions{ + IssueID: issue.ID, + Type: issues_model.CommentTypeComment, + }) + assert.NoError(t, err) + assert.NotEmpty(t, comments) - assert.NoError(t, handler.Handle(db.DefaultContext, content, user, payload)) + comment := comments[len(comments)-1] - watching, err = issues_model.CheckIssueWatch(db.DefaultContext, user, issue) - assert.NoError(t, err) - assert.False(t, watching) - }) + return comment.PosterID == user.ID && comment.Content == token + }, 10*time.Second, 1*time.Second) + }) + } }) - - if setting.IncomingEmail.Enabled { - // This test connects to the configured email server and is currently only enabled for MySql integration tests. - // It sends a reply to create a comment. If the comment is not detected after 10 seconds the test fails. - t.Run("IMAP", func(t *testing.T) { - defer tests.PrintCurrentTest(t)() - - payload, err := incoming_payload.CreateReferencePayload(issue) - assert.NoError(t, err) - token, err := token_service.CreateToken(token_service.ReplyHandlerType, user, payload) - assert.NoError(t, err) - - msg := gomail.NewMessage() - msg.SetHeader("To", strings.Replace(setting.IncomingEmail.ReplyToAddress, setting.IncomingEmail.TokenPlaceholder, token, 1)) - msg.SetHeader("From", user.Email) - msg.SetBody("text/plain", token) - err = gomail.Send(&smtpTestSender{}, msg) - assert.NoError(t, err) - - assert.Eventually(t, func() bool { - comments, err := issues_model.FindComments(db.DefaultContext, &issues_model.FindCommentsOptions{ - IssueID: issue.ID, - Type: issues_model.CommentTypeComment, - }) - assert.NoError(t, err) - assert.NotEmpty(t, comments) - - comment := comments[len(comments)-1] - - return comment.PosterID == user.ID && comment.Content == token - }, 10*time.Second, 1*time.Second) - }) - } } // A simple SMTP mail sender used for integration tests.