From 54285319f68459f28e8b923ecb865fd7af71a6e4 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Tue, 23 Apr 2024 00:03:26 +0200 Subject: [PATCH] fix(incoming): allow replies to comments - allow attachments to code comments - incoming mails from issue comments are now identified as comments Fixes: https://codeberg.org/forgejo/forgejo/issues/3374 --- services/mailer/incoming/incoming_handler.go | 67 +++++++++++--------- tests/integration/incoming_email_test.go | 52 +++++++-------- 2 files changed, 61 insertions(+), 58 deletions(-) diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index dc0b539822..325b94ae09 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/services/mailer/incoming/incoming_handler.go @@ -82,31 +82,34 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u return nil } + log.Trace("incoming mail related to %T", ref) + + 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 = append(attachmentIDs, a.UUID) + } + } + + if content.Content == "" && len(attachmentIDs) == 0 { + log.Trace("incoming mail has no content and no attachement", ref) + 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 = append(attachmentIDs, a.UUID) - } - } - - if content.Content == "" && len(attachmentIDs) == 0 { - return nil - } - _, err = issue_service.CreateIssueComment(ctx, doer, issue.Repo, issue, content.Content, attachmentIDs) if err != nil { return fmt.Errorf("CreateIssueComment failed: %w", err) @@ -114,11 +117,13 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u case *issues_model.Comment: comment := r - if content.Content == "" { - return nil - } - - if comment.Type == issues_model.CommentTypeCode { + switch comment.Type { + case issues_model.CommentTypeComment, issues_model.CommentTypeReview: + _, 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.CommentTypeCode: _, err := pull_service.CreateCodeComment( ctx, doer, @@ -130,12 +135,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: + log.Trace("incoming mail related to comment of type %v is ignored", comment.Type) } + default: + log.Trace("incoming mail related to %T is ignored", ref) } return nil } diff --git a/tests/integration/incoming_email_test.go b/tests/integration/incoming_email_test.go index 1284833864..543e620dbf 100644 --- a/tests/integration/incoming_email_test.go +++ b/tests/integration/incoming_email_test.go @@ -76,14 +76,11 @@ func TestIncomingEmail(t *testing.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)() + checkReply := func(t *testing.T, payload []byte, issue *issues_model.Issue, commentType issues_model.CommentType) { + t.Helper() handler := &incoming.ReplyHandler{} - 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)) @@ -101,7 +98,7 @@ func TestIncomingEmail(t *testing.T) { comments, err := issues_model.FindComments(db.DefaultContext, &issues_model.FindCommentsOptions{ IssueID: issue.ID, - Type: issues_model.CommentTypeComment, + Type: commentType, }) assert.NoError(t, err) assert.NotEmpty(t, comments) @@ -113,6 +110,14 @@ func TestIncomingEmail(t *testing.T) { attachment := comment.Attachments[0] assert.Equal(t, content.Attachments[0].Name, attachment.Name) assert.EqualValues(t, 4, attachment.Size) + } + t.Run("Issue", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + payload, err := incoming_payload.CreateReferencePayload(issue) + assert.NoError(t, err) + + checkReply(t, payload, issue, issues_model.CommentTypeComment) }) t.Run("CodeComment", func(t *testing.T) { @@ -121,33 +126,22 @@ func TestIncomingEmail(t *testing.T) { comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 6}) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) - 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) + + checkReply(t, payload, issue, issues_model.CommentTypeCode) + }) + + t.Run("Comment", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 2}) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) 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.Empty(t, comment.Attachments) + checkReply(t, payload, issue, issues_model.CommentTypeComment) }) })