From 6a555996dca6ba71c65818e14ab0eeafa1af6dc2 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 18 Jan 2024 11:09:05 +0000 Subject: [PATCH] [GITEA] POST /repos/{owner}/{repo}/pulls/{index}/reviews/{id}/comments (squash) do not implicitly create a review If a comment already exists in a review, the comment is added. If it is the first comment added to a review, it will implicitly create a new review instead of adding to the existing one. The pull_service.CreateCodeComment function is responsibe for this behavior and it will defer to createCodeComment once the review is determined, either because it was found or because it was created. Rename createCodeComment into CreateCodeCommentKnownReviewID to expose it and change the API endpoint to use it instead. Since the review is provided by the user and verified to exist already, there is no need for the logic implemented by CreateCodeComment. The tests are modified to remove the initial comment from the fixture because it was creating the false positive. I was verified to fail without this fix. --- routers/api/v1/repo/pull_review.go | 14 +++----------- services/pull/review.go | 6 +++--- tests/integration/api_pull_review_test.go | 21 +-------------------- 3 files changed, 7 insertions(+), 34 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index b3ad22a956..3959449560 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -331,22 +331,14 @@ func CreatePullReviewComment(ctx *context.APIContext) { line = opts.OldLineNum * -1 } - comment, err := pull_service.CreateCodeComment(ctx, + comment, err := pull_service.CreateCodeCommentKnownReviewID(ctx, ctx.Doer, - ctx.Repo.GitRepo, + pr.Issue.Repo, pr.Issue, - line, opts.Body, opts.Path, - // as of e522e774cae2240279fc48c349fc513c9d3353ee - // isPending is not needed because review.ID is always available - // and does not need to be discovered implicitly - false, + line, review.ID, - // as of e522e774cae2240279fc48c349fc513c9d3353ee - // latestCommitID is not needed because it is only used to - // create a new review in case it does not already exist - "", ) if err != nil { ctx.InternalServerError(err) diff --git a/services/pull/review.go b/services/pull/review.go index e48f380154..33bbec54ca 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -95,7 +95,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. return nil, err } - comment, err := createCodeComment(ctx, + comment, err := CreateCodeCommentKnownReviewID(ctx, doer, issue.Repo, issue, @@ -135,7 +135,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. } } - comment, err := createCodeComment(ctx, + comment, err := CreateCodeCommentKnownReviewID(ctx, doer, issue.Repo, issue, @@ -161,7 +161,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. } // createCodeComment creates a plain code comment at the specified line / path -func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64) (*issues_model.Comment, error) { +func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64) (*issues_model.Comment, error) { var commitID, patch string if err := issue.LoadPullRequest(ctx); err != nil { return nil, fmt.Errorf("LoadPullRequest: %w", err) diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 7341b14297..c66c7d752d 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -45,7 +45,6 @@ func TestAPIPullReviewCreateDeleteComment(t *testing.T) { t.Run("Event_"+string(event), func(t *testing.T) { path := "README.md" var review api.PullReview - existingCommentBody := "existing comment body" var reviewLine int64 = 1 // cleanup @@ -76,18 +75,11 @@ func TestAPIPullReviewCreateDeleteComment(t *testing.T) { req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/pulls/%d/reviews", repo.FullName(), pullIssue.Index), &api.CreatePullReviewOptions{ Body: "body1", Event: event, - Comments: []api.CreatePullReviewComment{ - { - Path: path, - Body: existingCommentBody, - OldLineNum: reviewLine, - }, - }, }).AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &review) require.EqualValues(t, string(event), review.State) - require.EqualValues(t, 1, review.CodeCommentsCount) + require.EqualValues(t, 0, review.CodeCommentsCount) } { @@ -100,17 +92,6 @@ func TestAPIPullReviewCreateDeleteComment(t *testing.T) { } requireReviewCount(1) - { - req := NewRequestf(t, http.MethodGet, "/api/v1/repos/%s/pulls/%d/reviews/%d/comments", repo.FullName(), pullIssue.Index, review.ID). - AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusOK) - var reviewComments []*api.PullReviewComment - DecodeJSON(t, resp, &reviewComments) - require.Len(t, reviewComments, 1) - assert.EqualValues(t, username, reviewComments[0].Poster.UserName) - assert.EqualValues(t, existingCommentBody, reviewComments[0].Body) - } - newCommentBody := "first new line" var reviewComment api.PullReviewComment