diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 97c5dcee78..e478f93655 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -337,6 +337,7 @@ func CreatePullReviewComment(ctx *context.APIContext) { pr.Issue, opts.Body, opts.Path, + review.CommitID, line, review.ID, nil, diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 941e428039..c2e30770d3 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -44,12 +44,15 @@ func RenderNewCodeCommentForm(ctx *context.Context) { ctx.Data["PageIsPullFiles"] = true ctx.Data["Issue"] = issue ctx.Data["CurrentReview"] = currentReview - pullHeadCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(issue.PullRequest.GetGitRefName()) - if err != nil { - ctx.ServerError("GetRefCommitID", err) - return + afterCommitID := ctx.FormString("after_commit_id") + if afterCommitID == "" { + afterCommitID, err = ctx.Repo.GitRepo.GetRefCommitID(issue.PullRequest.GetGitRefName()) + if err != nil { + ctx.ServerError("GetRefCommitID", err) + return + } } - ctx.Data["AfterCommitID"] = pullHeadCommitID + ctx.Data["AfterCommitID"] = afterCommitID ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled upload.AddUploadContext(ctx, "comment") ctx.HTML(http.StatusOK, tplNewComment) diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index 14e6714a63..dd0fa7df29 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -11,6 +11,7 @@ import ( "forgejo.org/models/db" issues_model "forgejo.org/models/issues" "forgejo.org/models/unittest" + "forgejo.org/modules/git" "forgejo.org/modules/templates" "forgejo.org/services/context" "forgejo.org/services/contexttest" @@ -28,6 +29,13 @@ func TestRenderConversation(t *testing.T) { _ = pr.Issue.LoadPoster(db.DefaultContext) _ = pr.Issue.LoadRepo(db.DefaultContext) + require.NoError(t, pr.LoadHeadRepo(t.Context())) + repo, err := git.OpenRepository(t.Context(), pr.HeadRepo.RepoPath()) + defer repo.Close() + require.NoError(t, err) + prHeadCommitID, err := repo.GetBranchCommitID(pr.HeadBranch) + require.NoError(t, err) + run := func(name string, cb func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder)) { t.Run(name, func(t *testing.T) { ctx, resp := contexttest.MockContext(t, "/") @@ -42,7 +50,9 @@ func TestRenderConversation(t *testing.T) { var preparedComment *issues_model.Comment run("prepare", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { - comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, 1, "content", "", false, 0, pr.HeadCommitID, nil) + comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, + 1, "content", "", false, 0, + prHeadCommitID, nil) require.NoError(t, err) comment.Invalidated = true diff --git a/services/pull/review.go b/services/pull/review.go index b8d4a30be8..5500ce4582 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -137,6 +137,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. issue, content, treePath, + latestCommitID, line, replyReviewID, attachments, @@ -178,6 +179,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. issue, content, treePath, + latestCommitID, line, review.ID, attachments, @@ -199,7 +201,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. } // CreateCodeCommentKnownReviewID creates a plain code comment at the specified line / path -func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) { +func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath, afterCommitID string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) { var commitID, blamedCommitID, patch string blamedLine := line if err := issue.LoadPullRequest(ctx); err != nil { @@ -249,7 +251,7 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, // FIXME validate treePath // Get latest commit referencing the commented line // No need for get commit for base branch changes - commit, lineres, err := gitRepo.LineBlame(head, treePath, uint64(line)) + commit, lineres, err := gitRepo.LineBlame(afterCommitID, treePath, uint64(line)) if err == nil { blamedCommitID = commit.ID.String() blamedLine = int64(lineres) diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index f3c0c0989d..5bd9a50b3b 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -201,7 +201,7 @@ {{end}} {{else}} - +
{{if $.IsSplitStyle}} {{template "repo/diff/section_split" dict "file" . "root" $}} {{else}} diff --git a/tests/integration/comment_roles_test.go b/tests/integration/comment_roles_test.go index df1c5b1188..f9eebf34ff 100644 --- a/tests/integration/comment_roles_test.go +++ b/tests/integration/comment_roles_test.go @@ -4,6 +4,7 @@ package integration import ( + "fmt" "net/http" "net/url" "path" @@ -188,16 +189,23 @@ func testEasyLeavePRComment(t *testing.T, session *TestSession, user, repo, id, // testEasyLeavePRReviewComment is used to add review comments to specific lines of changed files in the diff of the PR. func testEasyLeavePRReviewComment(t *testing.T, session *TestSession, user, repo, id, file, line, message, replyID string) { t.Helper() + req := NewRequestf(t, "GET", "/%s/%s/pulls/%s/files/reviews/new_comment", user, repo, id) + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) values := map[string]string{ - "origin": "diff", - "side": "proposed", - "line": line, - "path": file, - "content": message, - "single_review": "true", + "origin": doc.GetInputValueByName("origin"), + "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), + "side": "proposed", + "line": line, + "path": file, + "diff_start_cid": doc.GetInputValueByName("diff_start_cid"), + "diff_end_cid": doc.GetInputValueByName("diff_end_cid"), + "diff_base_cid": doc.GetInputValueByName("diff_base_cid"), + "content": message, + "single_review": "true", } if len(replyID) > 0 { values["reply"] = replyID } - session.MakeRequest(t, NewRequestWithValues(t, "POST", path.Join(user, repo, "pulls", id, "files/reviews/comments"), values), http.StatusOK) + session.MakeRequest(t, NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/pulls/%s/files/reviews/comments", user, repo, id), values), http.StatusOK) } diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 3c0ea62a9c..262733f72b 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -214,27 +214,27 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { defer tests.PrintCurrentTest(t)() req := NewRequest(t, "GET", "/user2/repo1/pulls/3/files/reviews/new_comment") resp := session.MakeRequest(t, req, http.StatusOK) - doc := NewHTMLParser(t, resp.Body) + newCommentForm := NewHTMLParser(t, resp.Body) var firstReviewID int64 { // first (outdated) review req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/comments", map[string]string{ - "origin": doc.GetInputValueByName("origin"), - "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), + "origin": newCommentForm.GetInputValueByName("origin"), + "latest_commit_id": newCommentForm.GetInputValueByName("latest_commit_id"), "side": "proposed", "line": "2", "path": "iso-8859-1.txt", - "diff_start_cid": doc.GetInputValueByName("diff_start_cid"), - "diff_end_cid": doc.GetInputValueByName("diff_end_cid"), - "diff_base_cid": doc.GetInputValueByName("diff_base_cid"), + "diff_start_cid": newCommentForm.GetInputValueByName("diff_start_cid"), + "diff_end_cid": newCommentForm.GetInputValueByName("diff_end_cid"), + "diff_base_cid": newCommentForm.GetInputValueByName("diff_base_cid"), "content": "nitpicking comment", "pending_review": "", }) session.MakeRequest(t, req, http.StatusOK) req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/submit", map[string]string{ - "commit_id": doc.GetInputValueByName("latest_commit_id"), + "commit_id": newCommentForm.GetInputValueByName("latest_commit_id"), "content": "looks good", "type": "comment", }) @@ -243,7 +243,7 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { // retrieve comment_id by reloading the comment page req = NewRequest(t, "GET", "/user2/repo1/pulls/3") resp = session.MakeRequest(t, req, http.StatusOK) - doc = NewHTMLParser(t, resp.Body) + doc := NewHTMLParser(t, resp.Body) commentID, ok := doc.Find(`[data-action="Resolve"]`).Attr("data-comment-id") assert.True(t, ok) @@ -266,21 +266,21 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { // second (up-to-date) review on the same line // make a second review req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/comments", map[string]string{ - "origin": doc.GetInputValueByName("origin"), - "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), + "origin": newCommentForm.GetInputValueByName("origin"), + "latest_commit_id": newCommentForm.GetInputValueByName("latest_commit_id"), "side": "proposed", "line": "2", "path": "iso-8859-1.txt", - "diff_start_cid": doc.GetInputValueByName("diff_start_cid"), - "diff_end_cid": doc.GetInputValueByName("diff_end_cid"), - "diff_base_cid": doc.GetInputValueByName("diff_base_cid"), + "diff_start_cid": newCommentForm.GetInputValueByName("diff_start_cid"), + "diff_end_cid": newCommentForm.GetInputValueByName("diff_end_cid"), + "diff_base_cid": newCommentForm.GetInputValueByName("diff_base_cid"), "content": "nitpicking comment", "pending_review": "", }) session.MakeRequest(t, req, http.StatusOK) req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/submit", map[string]string{ - "commit_id": doc.GetInputValueByName("latest_commit_id"), + "commit_id": newCommentForm.GetInputValueByName("latest_commit_id"), "content": "looks better", "type": "comment", }) @@ -289,7 +289,7 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { // retrieve comment_id by reloading the comment page req = NewRequest(t, "GET", "/user2/repo1/pulls/3") resp = session.MakeRequest(t, req, http.StatusOK) - doc = NewHTMLParser(t, resp.Body) + doc := NewHTMLParser(t, resp.Body) commentIDs := doc.Find(`[data-action="Resolve"]`).Map(func(i int, elt *goquery.Selection) string { v, _ := elt.Attr("data-comment-id") @@ -318,7 +318,7 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { // even on template error, the page returns HTTP 200 // count the comments to ensure success. - doc = NewHTMLParser(t, resp.Body) + doc := NewHTMLParser(t, resp.Body) comments := doc.Find(`.comment-code-cloud > .comment`) assert.Len(t, comments.Nodes, 1) // the outdated comment belongs to another review and should not be shown }) @@ -674,13 +674,17 @@ func TestPullRequestReplyMail(t *testing.T) { review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 1002}, "type = 0") - req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ - "origin": "diff", - "content": "Just a comment!", - "side": "proposed", - "line": "4", - "path": "README.md", - "reply": strconv.FormatInt(review.ID, 10), + req := NewRequest(t, "GET", "/user2/repo1/pulls/2/files/reviews/new_comment") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ + "origin": "diff", + "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), + "content": "Just a comment!", + "side": "proposed", + "line": "4", + "path": "README.md", + "reply": strconv.FormatInt(review.ID, 10), }) session.MakeRequest(t, req, http.StatusOK) @@ -696,12 +700,16 @@ func TestPullRequestReplyMail(t *testing.T) { called = true })() - req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ - "origin": "diff", - "content": "Notification time 2!", - "side": "proposed", - "line": "2", - "path": "README.md", + req := NewRequest(t, "GET", "/user2/repo1/pulls/2/files/reviews/new_comment") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ + "origin": "diff", + "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), + "content": "Notification time 2!", + "side": "proposed", + "line": "2", + "path": "README.md", }) session.MakeRequest(t, req, http.StatusOK) @@ -725,13 +733,17 @@ func TestPullRequestReplyMail(t *testing.T) { review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 1001, Type: issues_model.ReviewTypeComment}) - req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ - "origin": "diff", - "content": "Notification time!", - "side": "proposed", - "line": "3", - "path": "README.md", - "reply": strconv.FormatInt(review.ID, 10), + req := NewRequest(t, "GET", "/user2/repo1/pulls/2/files/reviews/new_comment") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ + "origin": "diff", + "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), + "content": "Notification time!", + "side": "proposed", + "line": "3", + "path": "README.md", + "reply": strconv.FormatInt(review.ID, 10), }) session.MakeRequest(t, req, http.StatusOK) @@ -751,13 +763,17 @@ func TestPullRequestReplyMail(t *testing.T) { called = true })() - req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ - "origin": "diff", - "content": "Notification time 2!", - "side": "proposed", - "line": "5", - "path": "README.md", - "single_review": "true", + req := NewRequest(t, "GET", "/user2/repo1/pulls/2/files/reviews/new_comment") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/2/files/reviews/comments", map[string]string{ + "origin": "diff", + "latest_commit_id": doc.GetInputValueByName("latest_commit_id"), + "content": "Notification time 2!", + "side": "proposed", + "line": "5", + "path": "README.md", + "single_review": "true", }) session.MakeRequest(t, req, http.StatusOK) @@ -1409,6 +1425,52 @@ func TestPullRequestCommentPlacement(t *testing.T) { tester.assertFilesChangedDiff(diff) tester.assertCommitDiff(commitSHA, diff) }) + + t.Run("comment on specific commit adjusts correctly to later changes in the PR", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + tester := newPullRequestCommentPlacementTester(t) + + // Modify an earlier part of the file in one commit, and then change line numbers in a second commit by + // removing some content from the file earlier than the first commit + content := tester.fileContent + content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1) + commit1 := tester.changeFile("file1.md", content) + content = strings.Replace(content, "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n", "", 1) + commit2 := tester.changeFile("file1.md", content) + tester.createPR() + + // Create a comment on commit1's "Line 50" change, from the commit-specific view: + comment := tester.commentFromSpecificCommit(commit1, "file1.md", 50) + assert.Equal(t, `diff --git a/file1.md b/file1.md +--- a/file1.md ++++ b/file1.md +@@ -48,3 +48,3 @@ + Line 48 + Line 49 +-Line 50 ++Line 50--modified`, comment.PatchQuoted) + assert.Equal(t, "proposed", comment.DiffSide()) + assert.EqualValues(t, 50, comment.Line) + assert.Equal(t, commit1, comment.CommitSHA) + + diff50 := []diffTableRow{ + {rowType: RowHasCode, code: "Line 49"}, + {rowType: RowDelCode, code: "Line 50"}, + {rowType: RowAddCode, code: "Line 50--modified"}, + {rowType: RowComment, commentID: comment.ID}, + {rowType: RowHasCode, code: "Line 51"}, + } + tester.assertFilesChangedDiff(diff50) + tester.assertCommitDiff(commit1, diff50) + + diff10 := []diffTableRow{ + {rowType: RowDelCode, code: "Line 9"}, + {rowType: RowDelCode, code: "Line 10"}, + {rowType: RowHasCode, code: "Line 11"}, + } + tester.assertFilesChangedDiff(diff10) + tester.assertCommitDiff(commit2, diff10) + }) }) } @@ -1526,14 +1588,24 @@ func (tester *PullRequestCommentPlacementTester) createPR() { } func (tester *PullRequestCommentPlacementTester) commentFromFilesChanged(filename string, line int) *issues_model.Comment { - commentContent := uuid.New().String() - req := NewRequest(tester.t, "GET", + // omit after_commit_id -- new_comment form defaults to fetching the PR head fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index)) resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) + return tester.commentFromNewCommentForm(resp, filename, line) +} +func (tester *PullRequestCommentPlacementTester) commentFromSpecificCommit(commitID, filename string, line int) *issues_model.Comment { + req := NewRequest(tester.t, "GET", + fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment?after_commit_id=%s", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, commitID)) + resp := tester.session.MakeRequest(tester.t, req, http.StatusOK) + return tester.commentFromNewCommentForm(resp, filename, line) +} + +func (tester *PullRequestCommentPlacementTester) commentFromNewCommentForm(resp *httptest.ResponseRecorder, filename string, line int) *issues_model.Comment { + commentContent := uuid.New().String() doc := NewHTMLParser(tester.t, resp.Body) - req = NewRequestWithValues(tester.t, "POST", + req := NewRequestWithValues(tester.t, "POST", fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/comments", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index), map[string]string{ "origin": doc.GetInputValueByName("origin"),