From a5e2283cf166358f01e9ee850fd71abb38ad4bee Mon Sep 17 00:00:00 2001 From: sebastian-sauer Date: Fri, 25 Jun 2021 00:05:51 +0200 Subject: [PATCH] API: Allow COMMENT reviews to not specify a body (#16229) * Allow COMMENT reviews to not specify a body when using web ui there is no need to specify a body. so we don't need to specify a body if adding a COMMENT-review via our api. * Ensure comments or Body is provided and add some integration tests for reviewtype COMMENT. Signed-off-by: Sebastian Sauer --- integrations/api_pull_review_test.go | 54 ++++++++++++++++++++++++++++ routers/api/v1/repo/pull_review.go | 22 ++++++++---- 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/integrations/api_pull_review_test.go b/integrations/api_pull_review_test.go index ebe8539a826d3..bcc0cbffcb381 100644 --- a/integrations/api_pull_review_test.go +++ b/integrations/api_pull_review_test.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/models" api "code.gitea.io/gitea/modules/structs" + jsoniter "github.com/json-iterator/go" "github.com/stretchr/testify/assert" ) @@ -139,6 +140,59 @@ func TestAPIPullReview(t *testing.T) { req = NewRequestf(t, http.MethodDelete, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, review.ID, token) resp = session.MakeRequest(t, req, http.StatusNoContent) + // test CreatePullReview Comment without body but with comments + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token), &api.CreatePullReviewOptions{ + // Body: "", + Event: "COMMENT", + Comments: []api.CreatePullReviewComment{{ + Path: "README.md", + Body: "first new line", + OldLineNum: 0, + NewLineNum: 1, + }, { + Path: "README.md", + Body: "first old line", + OldLineNum: 1, + NewLineNum: 0, + }, + }, + }) + var commentReview api.PullReview + + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &commentReview) + assert.EqualValues(t, "COMMENT", commentReview.State) + assert.EqualValues(t, 2, commentReview.CodeCommentsCount) + assert.EqualValues(t, "", commentReview.Body) + assert.EqualValues(t, false, commentReview.Dismissed) + + // test CreatePullReview Comment with body but without comments + commentBody := "This is a body of the comment." + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token), &api.CreatePullReviewOptions{ + Body: commentBody, + Event: "COMMENT", + Comments: []api.CreatePullReviewComment{}, + }) + + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &commentReview) + assert.EqualValues(t, "COMMENT", commentReview.State) + assert.EqualValues(t, 0, commentReview.CodeCommentsCount) + assert.EqualValues(t, commentBody, commentReview.Body) + assert.EqualValues(t, false, commentReview.Dismissed) + + // test CreatePullReview Comment without body and no comments + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token), &api.CreatePullReviewOptions{ + Body: "", + Event: "COMMENT", + Comments: []api.CreatePullReviewComment{}, + }) + resp = session.MakeRequest(t, req, http.StatusUnprocessableEntity) + errMap := make(map[string]interface{}) + json := jsoniter.ConfigCompatibleWithStandardLibrary + json.Unmarshal(resp.Body.Bytes(), &errMap) + assert.EqualValues(t, "review event COMMENT requires a body or a comment", errMap["message"].(string)) + // test get review requests // to make it simple, use same api with get review pullIssue12 := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 12}).(*models.Issue) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 35414e0a80c53..323904f45c0e3 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -307,7 +307,7 @@ func CreatePullReview(ctx *context.APIContext) { } // determine review type - reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body) + reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body, len(opts.Comments) > 0) if isWrong { return } @@ -429,7 +429,7 @@ func SubmitPullReview(ctx *context.APIContext) { } // determine review type - reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body) + reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body, len(review.Comments) > 0) if isWrong { return } @@ -463,12 +463,15 @@ func SubmitPullReview(ctx *context.APIContext) { } // preparePullReviewType return ReviewType and false or nil and true if an error happen -func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, event api.ReviewStateType, body string) (models.ReviewType, bool) { +func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, event api.ReviewStateType, body string, hasComments bool) (models.ReviewType, bool) { if err := pr.LoadIssue(); err != nil { ctx.Error(http.StatusInternalServerError, "LoadIssue", err) return -1, true } + needsBody := true + hasBody := len(strings.TrimSpace(body)) > 0 + var reviewType models.ReviewType switch event { case api.ReviewStateApproved: @@ -478,6 +481,7 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even return -1, true } reviewType = models.ReviewTypeApprove + needsBody = false case api.ReviewStateRequestChanges: // can not reject your own PR @@ -489,13 +493,19 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even case api.ReviewStateComment: reviewType = models.ReviewTypeComment + needsBody = false + // if there is no body we need to ensure that there are comments + if !hasBody && !hasComments { + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("review event %s requires a body or a comment", event)) + return -1, true + } default: reviewType = models.ReviewTypePending } - // reject reviews with empty body if not approve type - if reviewType != models.ReviewTypeApprove && len(strings.TrimSpace(body)) == 0 { - ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("review event %s need body", event)) + // reject reviews with empty body if a body is required for this call + if needsBody && !hasBody { + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("review event %s requires a body", event)) return -1, true }