Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: Allow COMMENT reviews to not specify a body #16229

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions integrations/api_pull_review_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
Expand Down
22 changes: 16 additions & 6 deletions routers/api/v1/repo/pull_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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:
Expand All @@ -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
Expand All @@ -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
}

Expand Down