diff --git a/models/issues/review.go b/models/issues/review.go index f2f73daec8c..bb1541df2ed 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -456,7 +456,7 @@ func ReviewExists(ctx context.Context, issue *Issue, treePath string, line int64 type ContentEmptyErr struct{} func (ContentEmptyErr) Error() string { - return "Review content is empty" + return "review requires a body or a comment" } // IsContentEmptyErr returns true if err is a ContentEmptyErr diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index a9dcde7f6d6..874c80cc0a6 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -507,7 +507,7 @@ func CreatePullReview(ctx *context.APIContext) { } // determine review type - reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body, len(opts.Comments) > 0) + reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body) if isWrong { return } @@ -562,7 +562,7 @@ func CreatePullReview(ctx *context.APIContext) { // create review and associate all pending review comments review, _, err := pull_service.SubmitReview(ctx, ctx.Doer, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, opts.CommitID, nil) if err != nil { - if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) { + if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) || issues_model.IsContentEmptyErr(err) { ctx.APIError(http.StatusUnprocessableEntity, err.Error()) } else { ctx.APIErrorInternal(err) @@ -634,7 +634,7 @@ func SubmitPullReview(ctx *context.APIContext) { } // determine review type - reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body, len(review.Comments) > 0) + reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body) if isWrong { return } @@ -654,7 +654,7 @@ func SubmitPullReview(ctx *context.APIContext) { // create review and associate all pending review comments review, _, err = pull_service.SubmitReview(ctx, ctx.Doer, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, headCommitID, nil) if err != nil { - if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) { + if errors.Is(err, pull_service.ErrSubmitReviewOnClosedPR) || issues_model.IsContentEmptyErr(err) { ctx.APIError(http.StatusUnprocessableEntity, err.Error()) } else { ctx.APIErrorInternal(err) @@ -672,16 +672,12 @@ func SubmitPullReview(ctx *context.APIContext) { } // preparePullReviewType return ReviewType and false or nil and true if an error happen -func preparePullReviewType(ctx *context.APIContext, pr *issues_model.PullRequest, event api.ReviewStateType, body string, hasComments bool) (issues_model.ReviewType, bool) { +func preparePullReviewType(ctx *context.APIContext, pr *issues_model.PullRequest, event api.ReviewStateType, body string) (issues_model.ReviewType, bool) { if err := pr.LoadIssue(ctx); err != nil { ctx.APIErrorInternal(err) return -1, true } - needsBody := true - hasBody := len(strings.TrimSpace(body)) > 0 - - var reviewType issues_model.ReviewType switch event { case api.ReviewStateApproved: // can not approve your own PR @@ -689,8 +685,7 @@ func preparePullReviewType(ctx *context.APIContext, pr *issues_model.PullRequest ctx.APIError(http.StatusUnprocessableEntity, "approve your own pull is not allowed") return -1, true } - reviewType = issues_model.ReviewTypeApprove - needsBody = false + return issues_model.ReviewTypeApprove, false case api.ReviewStateRequestChanges: // can not reject your own PR @@ -698,27 +693,17 @@ func preparePullReviewType(ctx *context.APIContext, pr *issues_model.PullRequest ctx.APIError(http.StatusUnprocessableEntity, "reject your own pull is not allowed") return -1, true } - reviewType = issues_model.ReviewTypeReject - - case api.ReviewStateComment: - reviewType = issues_model.ReviewTypeComment - needsBody = false - // if there is no body we need to ensure that there are comments - if !hasBody && !hasComments { - ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("review event %s requires a body or a comment", event)) + if strings.TrimSpace(body) == "" { + ctx.APIError(http.StatusUnprocessableEntity, "review event REQUEST_CHANGES requires a body") return -1, true } + return issues_model.ReviewTypeReject, false + + case api.ReviewStateComment: + return issues_model.ReviewTypeComment, false default: - reviewType = issues_model.ReviewTypePending + return issues_model.ReviewTypePending, false } - - // reject reviews with empty body if a body is required for this call - if needsBody && !hasBody { - ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("review event %s requires a body", event)) - return -1, true - } - - return reviewType, false } // prepareSingleReview return review, related pull and false or nil, nil and true if an error happen diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index e16cb5cea49..d5c8a4b41b3 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -160,6 +160,26 @@ func testAPIPullReviewGeneral(t *testing.T) { AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) + reviewsURL := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index) + pendingComment := []api.CreatePullReviewComment{{Path: "README.md", Body: "pending comment", NewLineNum: 1}} + req = NewRequestWithJSON(t, http.MethodPost, reviewsURL, &api.CreatePullReviewOptions{Body: "draft"}).AddTokenAuth(token) + pendingReviewURL := fmt.Sprintf("%s/%d", reviewsURL, DecodeJSON(t, MakeRequest(t, req, http.StatusOK), &api.PullReview{}).ID) + req = NewRequestWithJSON(t, http.MethodPost, pendingReviewURL, &api.SubmitPullReviewOptions{Event: "COMMENT"}).AddTokenAuth(token) + MakeRequest(t, req, http.StatusUnprocessableEntity) + req = NewRequestWithJSON(t, http.MethodPost, reviewsURL, &api.CreatePullReviewOptions{Comments: pendingComment}).AddTokenAuth(token) + MakeRequest(t, req, http.StatusOK) + req = NewRequestWithJSON(t, http.MethodPost, pendingReviewURL, &api.SubmitPullReviewOptions{Event: "COMMENT"}).AddTokenAuth(token) + review = DecodeJSON(t, MakeRequest(t, req, http.StatusOK), &api.PullReview{}) + assert.EqualValues(t, "COMMENT", review.State) + assert.Equal(t, 1, review.CodeCommentsCount) + + req = NewRequestWithJSON(t, http.MethodPost, reviewsURL, &api.CreatePullReviewOptions{Comments: pendingComment}).AddTokenAuth(token) + MakeRequest(t, req, http.StatusOK) + req = NewRequestWithJSON(t, http.MethodPost, reviewsURL, &api.CreatePullReviewOptions{Event: "COMMENT"}).AddTokenAuth(token) + review = DecodeJSON(t, MakeRequest(t, req, http.StatusOK), &api.PullReview{}) + assert.EqualValues(t, "COMMENT", review.State) + assert.Equal(t, 1, review.CodeCommentsCount) + // test CreatePullReview Comment without body but with comments req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews", repo.OwnerName, repo.Name, pullIssue.Index), &api.CreatePullReviewOptions{ // Body: "", @@ -210,7 +230,7 @@ func testAPIPullReviewGeneral(t *testing.T) { resp = MakeRequest(t, req, http.StatusUnprocessableEntity) errMap := make(map[string]any) json.Unmarshal(resp.Body.Bytes(), &errMap) - assert.Equal(t, "review event COMMENT requires a body or a comment", errMap["message"]) + assert.Equal(t, "review requires a body or a comment", errMap["message"]) // test get review requests // to make it simple, use same api with get review