diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index 7d6b2c6361..1317f5aad6 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/services/mailer/incoming/incoming_handler.go @@ -148,7 +148,7 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u nil, issue, comment.Line, - 0, // extraLinesCount: a reply to a comment inherit single line + comment.ExtraLinesCount, // inherit the parent's range so the reply threads under a multi-line comment content.Content, comment.TreePath, false, // not pending review but a single review diff --git a/templates/repo/diff/comment_form_datahandler.tmpl b/templates/repo/diff/comment_form_datahandler.tmpl index d0e493488d..5b72aa5b18 100644 --- a/templates/repo/diff/comment_form_datahandler.tmpl +++ b/templates/repo/diff/comment_form_datahandler.tmpl @@ -1,5 +1,5 @@ {{if $.comment}} - {{template "repo/diff/comment_form" dict "root" $.root "hidden" $.hidden "reply" $.reply "Line" $.comment.UnsignedLine "File" $.comment.TreePath "Side" $.comment.DiffSide "HasComments" true}} + {{template "repo/diff/comment_form" dict "root" $.root "hidden" $.hidden "reply" $.reply "Line" $.comment.UnsignedLine "ExtraLinesCount" $.comment.ExtraLinesCount "File" $.comment.TreePath "Side" $.comment.DiffSide "HasComments" true}} {{else if $.root}} {{template "repo/diff/comment_form" $}} {{else}} diff --git a/tests/integration/incoming_email_test.go b/tests/integration/incoming_email_test.go index c9876a63dd..95f8867a36 100644 --- a/tests/integration/incoming_email_test.go +++ b/tests/integration/incoming_email_test.go @@ -206,6 +206,44 @@ func TestIncomingEmail(t *testing.T) { checkReply(t, payload, issue, issues_model.CommentTypeComment) }) + + t.Run("MultiLineCodeComment", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // A reply to a multi-line code comment must inherit the parent's extra_lines_count + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) + require.NoError(t, issue.LoadRepo(db.DefaultContext)) + review, err := issues_model.CreateReview(db.DefaultContext, issues_model.CreateReviewOptions{ + Type: issues_model.ReviewTypeComment, + Issue: issue, + Reviewer: user, + }) + require.NoError(t, err) + // Anchor the parent with a stored patch so the reply reuses it (no git resolution needed). + parent, err := issues_model.CreateComment(db.DefaultContext, &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypeCode, + Doer: user, + Repo: issue.Repo, + Issue: issue, + Content: "multi-line parent", + LineNum: 4, + ExtraLinesCount: 2, + TreePath: "README.md", + CommitSHA: "0000000000000000000000000000000000000000", + ReviewID: review.ID, + Patch: "@@ -4,3 +4,3 @@", + }) + require.NoError(t, err) + + payload, err := incoming_payload.CreateReferencePayload(parent) + require.NoError(t, err) + + handler := &incoming.ReplyHandler{} + require.NoError(t, handler.Handle(db.DefaultContext, &incoming.MailContent{Content: "multi-line reply by mail"}, user, payload)) + + reply := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{Content: "multi-line reply by mail", IssueID: issue.ID, Type: issues_model.CommentTypeCode}) + assert.EqualValues(t, 2, reply.ExtraLinesCount) + }) }) t.Run("Unsubscribe", func(t *testing.T) { diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index b072c445bf..306e8fa8d9 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -1838,6 +1838,53 @@ func TestPullRequestCommentPlacement(t *testing.T) { assert.False(t, comment.Invalidated) }) + t.Run("reply to a multi-line comment threads under it", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + content := tester.fileContent + content = strings.Replace(content, "Line 48\n", "Line 48--modified\n", 1) + content = strings.Replace(content, "Line 49\n", "Line 49--modified\n", 1) + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + tester.changeFile("file1.md", content) + tester.createPR() + + comment := tester.multiLineCommentFromFilesChanged("file1.md", 48, 2) + assert.EqualValues(t, 2, comment.ExtraLinesCount) + assert.EqualValues(t, 50, comment.DisplayLine()) + + // Simulate a reply like the reply button does + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d/files", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index)) + resp := tester.session.MakeRequest(t, req, http.StatusOK) + form := NewHTMLParser(t, resp.Body).Find(`.conversation-holder[data-extra-lines-count="2"] form`) + field := func(name string) string { + return form.Find(fmt.Sprintf(`input[name=%q]`, name)).AttrOr("value", "") + } + require.Equal(t, "2", field("extra_lines_count"), "reply form must carry the parent's extra_lines_count") + + const replyContent = "reply through the multi-line comment reply form" + req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/comments", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index), map[string]string{ + "origin": field("origin"), + "before_commit_id": field("before_commit_id"), + "latest_commit_id": field("latest_commit_id"), + "side": field("side"), + "line": field("line"), + "extra_lines_count": field("extra_lines_count"), + "path": field("path"), + "reply": field("reply"), + "content": replyContent, + }) + tester.session.MakeRequest(t, req, http.StatusOK) + + // The reply threads under the parent: it shares the conversation grouping key + // (ReviewID + TreePath + DisplayLine, the end of the range). + reply := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{Content: replyContent}) + assert.EqualValues(t, 2, reply.ExtraLinesCount) + assert.Equal(t, comment.ReviewID, reply.ReviewID) + assert.Equal(t, comment.TreePath, reply.TreePath) + assert.Equal(t, comment.DisplayLine(), reply.DisplayLine()) + }) + t.Run("multi-line proposed comment over unmodified-then-modified lines stays visible", func(t *testing.T) { defer tests.PrintCurrentTest(t)() tester := newPullRequestCommentPlacementTester(t)