From 44ca212f25c04b46e938a8220a23c570a501e33e Mon Sep 17 00:00:00 2001 From: Sebastian Sauer Date: Tue, 22 Jun 2021 19:59:47 +0200 Subject: [PATCH 1/2] 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. Signed-off-by: Sebastian Sauer --- routers/api/v1/repo/pull_review.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 35414e0a80c53..dbc39266df0e2 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -469,6 +469,7 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even return -1, true } + var needsBody = true var reviewType models.ReviewType switch event { case api.ReviewStateApproved: @@ -478,6 +479,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 +491,14 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even case api.ReviewStateComment: reviewType = models.ReviewTypeComment + needsBody = false 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 && len(strings.TrimSpace(body)) == 0 { + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("review event %s requires a body", event)) return -1, true } From 2f0dd67a8f0748d4e4347c3417f3cc91d5262c75 Mon Sep 17 00:00:00 2001 From: Sebastian Sauer Date: Wed, 23 Jun 2021 19:36:49 +0200 Subject: [PATCH 2/2] Ensure comments or Body is provided and add some integration tests for reviewtype COMMENT. --- integrations/api_pull_review_test.go | 54 ++++++++++++++++++++++++++++ routers/api/v1/repo/pull_review.go | 17 ++++++--- 2 files changed, 66 insertions(+), 5 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 dbc39266df0e2..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,13 +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 } - var needsBody = true + needsBody := true + hasBody := len(strings.TrimSpace(body)) > 0 + var reviewType models.ReviewType switch event { case api.ReviewStateApproved: @@ -492,12 +494,17 @@ 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 a body is required for this call - if needsBody && len(strings.TrimSpace(body)) == 0 { + if needsBody && !hasBody { ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("review event %s requires a body", event)) return -1, true }