fix(api): allow pending-inline-comment-only reviews (#39433)

Prior to this change, the API rejected reviews without a summary
comment, even if it had pending inline comments. This differs from the
web UI, which accepts such reviews. The affected endpoints are:

1. Submitting via POST /repos/{owner}/{repo}/pulls/{index}/reviews/{id}.
2. Creating via POST /repos/{owner}/{repo}/pulls/{index}/reviews, both
when finishing an existing pending review and when creating a new
pending review.

The endpoints ran their own emptiness check, which ignored comments
already in a pending review. The fix drops it for comment and pending
reviews and relies on the model's check, which counts them, as the web
UI does.

A review with neither a body nor inline comments remains invalid.

---------

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: bircni <bircni@icloud.com>

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

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

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