Add missing comment reply handling (#32050) (#32065)

Backport #32050 by @KN4CK3R

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.

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

services/mailer/incoming/incoming_handler.go

@@ -82,43 +82,40 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u
 		return nil
 	}
 
+	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
+	}
+
 	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)
+		_, 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

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) {
+					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))
+
+					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)
+				})
+
+				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})
+
+					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)
+				})
+			})
+
+			t.Run("Unsubscribe", func(t *testing.T) {
 				defer tests.PrintCurrentTest(t)()
 
-				handler := &incoming.ReplyHandler{}
+				watching, err := issues_model.CheckIssueWatch(db.DefaultContext, user, issue)
+				assert.NoError(t, err)
+				assert.True(t, watching)
+
+				handler := &incoming.UnsubscribeHandler{}
+
+				content := &incoming.MailContent{
+					Content: "unsub me",
+				}
 
 				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))
-
-				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,
-				})
+				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.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.False(t, watching)
 			})
+		})
 
-			t.Run("CodeComment", func(t *testing.T) {
+		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)()
 
-				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)
+				payload, err := incoming_payload.CreateReferencePayload(issue)
+				assert.NoError(t, err)
+				token, err := token_service.CreateToken(token_service.ReplyHandlerType, user, payload)
 				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,
-				})
+				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.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.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)
 			})
-		})
-
-		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)
-
-			handler := &incoming.UnsubscribeHandler{}
-
-			content := &incoming.MailContent{
-				Content: "unsub me",
-			}
-
-			payload, err := incoming_payload.CreateReferencePayload(issue)
-			assert.NoError(t, err)
-
-			assert.NoError(t, handler.Handle(db.DefaultContext, content, user, payload))
-
-			watching, err = issues_model.CheckIssueWatch(db.DefaultContext, user, issue)
-			assert.NoError(t, err)
-			assert.False(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)()
-
-			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.