Merge pull request '[v7.0/forgejo] fix(incoming): allow replies to comments' (#3382) from bp-v7.0/forgejo-5428531 into v7.0/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3382

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 == "" {
+		switch comment.Type {
-			return nil
+		case issues_model.CommentTypeComment, issues_model.CommentTypeReview:
-		}
+			_, err = issue_service.CreateIssueComment(ctx, doer, issue.Repo, issue, content.Content, attachmentIDs)
-
+			if err != nil {
-		if comment.Type == issues_model.CommentTypeCode {
+				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
 }

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) {
+			checkReply := func(t *testing.T, payload []byte, issue *issues_model.Issue, commentType issues_model.CommentType) {
-				defer tests.PrintCurrentTest(t)()
+				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{}
+				payload, err := incoming_payload.CreateReferencePayload(comment)
-				content := &incoming.MailContent{
+				assert.NoError(t, err)
-					Content: "code reply by mail",
+
-					Attachments: []*incoming.Attachment{
+				checkReply(t, payload, issue, issues_model.CommentTypeCode)
-						{
+			})
-							Name:    "attachment.txt",
+
-							Content: []byte("test"),
+			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))
+				checkReply(t, payload, issue, issues_model.CommentTypeComment)
-
-				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)
 			})
 		})