From bbdc5a08138dc29561ebcf9355f060c29f244dba Mon Sep 17 00:00:00 2001 From: Thomas Berger Date: Sun, 26 Jan 2020 17:47:13 +0100 Subject: [PATCH 01/26] API: Added pull review read only endpoints --- modules/structs/pull_review.go | 40 ++++ routers/api/v1/api.go | 10 +- routers/api/v1/repo/pull_review.go | 345 +++++++++++++++++++++++++++++ routers/api/v1/swagger/repo.go | 52 ++++- templates/swagger/v1_json.tmpl | 264 ++++++++++++++++++++++ 5 files changed, 698 insertions(+), 13 deletions(-) create mode 100644 modules/structs/pull_review.go create mode 100644 routers/api/v1/repo/pull_review.go diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go new file mode 100644 index 0000000000000..977a96f9599f9 --- /dev/null +++ b/modules/structs/pull_review.go @@ -0,0 +1,40 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package structs + +import ( + "time" +) + +// PullRequestReview represents a pull request review +type PullRequestReview struct { + ID int64 `json:"id"` + PRURL string `json:"pull_request_url"` + Reviewer *User `json:"user"` + Body string `json:"body"` + CommitID string `json:"commit_id"` + Type string `json:"type"` + + // swagger:strfmt date-time + Created time.Time `json:"created_at"` + + // TODO: is there a way to get a URL to the review itself? + // HTMLURL string `json:"html_url"` +} + +// PullRequestReviewComment represents a comment on a pull request +type PullRequestReviewComment struct { + ID int64 `json:"id"` + URL string `json:"url"` + PRURL string `json:"pull_request_url"` + ReviewID int64 `json:"pull_request_review_id"` + Path string `json:"path"` + CommitID string `json:"commit_id"` + DiffHunk string `json:"diff_hunk"` + LineNum uint64 `json:"position"` + OldLineNum uint64 `json:"original_position"` + Reviewer *User `json:"user"` + Body string `json:"body"` +} diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 4c9f9dd03e2e7..c830ad77aacc5 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -500,7 +500,7 @@ func RegisterRoutes(m *macaron.Macaron) { bind := binding.Bind if setting.API.EnableSwagger { - m.Get("/swagger", misc.Swagger) //Render V1 by default + m.Get("/swagger", misc.Swagger) // Render V1 by default } m.Group("/v1", func() { @@ -773,6 +773,14 @@ func RegisterRoutes(m *macaron.Macaron) { Patch(reqToken(), reqRepoWriter(models.UnitTypePullRequests), bind(api.EditPullRequestOption{}), repo.EditPullRequest) m.Combo("/merge").Get(repo.IsPullRequestMerged). Post(reqToken(), mustNotBeArchived, reqRepoWriter(models.UnitTypePullRequests), bind(auth.MergePullRequestForm{}), repo.MergePullRequest) + m.Group("/reviews", func() { + m.Combo("").Get(repo.ListPullReviews) + m.Group("/:id", func() { + m.Combo("").Get(repo.GetPullReview) + m.Combo("/comments").Get(repo.GetPullReviewComments) + }) + }) + }) }, mustAllowPulls, reqRepoReader(models.UnitTypeCode), context.ReferencesGitRepo(false)) m.Group("/statuses", func() { diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go new file mode 100644 index 0000000000000..6a1050b7d857b --- /dev/null +++ b/routers/api/v1/repo/pull_review.go @@ -0,0 +1,345 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package repo + +import ( + "net/http" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/structs" +) + +// ListPullReviews lists all reviews of a pull request +func ListPullReviews(ctx *context.APIContext) { + // swagger:operation GET /repos/{owner}/{repo}/pulls/{index}/reviews repository repoListPullReviews + // --- + // summary: List all reviews for a pull request. + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: index + // in: path + // description: index of the pull request + // type: integer + // format: int64 + // required: true + // responses: + // "200": + // "$ref": "#/responses/PullReviewList" + // "404": + // "$ref": "#/responses/notFound" + + pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) + if err != nil { + if models.IsErrPullRequestNotExist(err) { + ctx.NotFound("GetPullRequestByIndex", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) + } + return + } + + if err = pr.LoadIssue(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + return + } + + if err = pr.Issue.LoadRepo(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + return + } + + allReviews, err := models.FindReviews(models.FindReviewOptions{ + Type: models.ReviewTypeUnknown, + IssueID: pr.IssueID, + }) + + if err != nil { + ctx.Error(http.StatusInternalServerError, "FindReviews", err) + return + } + + var apiReviews []structs.PullRequestReview + for _, review := range allReviews { + // show pending reviews only for the user who created them + if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID { + continue + } + + if err = review.LoadReviewer(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadReviewer", err) + return + } + + var reviewType string + switch review.Type { + case models.ReviewTypeApprove: + reviewType = "APPROVE" + case models.ReviewTypeReject: + reviewType = "REJECT" + case models.ReviewTypeComment: + reviewType = "COMMENT" + case models.ReviewTypePending: + reviewType = "PENDING" + default: + reviewType = "UNKNOWN" + + } + + apiReviews = append(apiReviews, structs.PullRequestReview{ + ID: review.ID, + PRURL: pr.Issue.APIURL(), + Reviewer: review.Reviewer.APIFormat(), + Body: review.Content, + Created: review.CreatedUnix.AsTime(), + CommitID: review.CommitID, + Type: reviewType, + }) + } + + ctx.JSON(http.StatusOK, &apiReviews) +} + +// GetPullReview gets a specific review of a pull request +func GetPullReview(ctx *context.APIContext) { + // swagger:operation GET /repos/{owner}/{repo}/pulls/{index}/reviews/{id} repository repoGetPullReview + // --- + // summary: Get a specific review for a pull request. + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: index + // in: path + // description: index of the pull request + // type: integer + // format: int64 + // required: true + // - name: id + // in: path + // description: id of the review + // type: integer + // format: int64 + // required: true + // responses: + // "200": + // "$ref": "#/responses/PullReview" + // "404": + // "$ref": "#/responses/notFound" + + pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) + if err != nil { + if models.IsErrPullRequestNotExist(err) { + ctx.NotFound("GetPullRequestByIndex", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) + } + return + } + + review, err := models.GetReviewByID(ctx.ParamsInt64(":id")) + if err != nil { + if models.IsErrReviewNotExist(err) { + ctx.NotFound("GetReviewByID", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) + } + return + } + + // validate the the review is for the given PR + if review.IssueID != pr.IssueID { + ctx.NotFound("ReviewNotInPR", err) + return + } + + // make sure that the user has access to this review if it is pending + if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID { + ctx.NotFound("GetReviewByID", err) + return + } + + if err = pr.LoadIssue(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + return + } + + if err = pr.Issue.LoadRepo(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + return + } + + if err = review.LoadReviewer(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadReviewer", err) + return + } + + var reviewType string + switch review.Type { + case models.ReviewTypeApprove: + reviewType = "APPROVE" + case models.ReviewTypeReject: + reviewType = "REJECT" + case models.ReviewTypeComment: + reviewType = "COMMENT" + case models.ReviewTypePending: + reviewType = "PENDING" + default: + reviewType = "UNKNOWN" + + } + apiReview := structs.PullRequestReview{ + ID: review.ID, + PRURL: pr.Issue.APIURL(), + Reviewer: review.Reviewer.APIFormat(), + Body: review.Content, + Created: review.CreatedUnix.AsTime(), + CommitID: review.CommitID, + Type: reviewType, + } + + ctx.JSON(http.StatusOK, &apiReview) +} + +// GetPullReviewComments lists all comments of a pull request review +func GetPullReviewComments(ctx *context.APIContext) { + // swagger:operation GET /repos/{owner}/{repo}/pulls/{index}/reviews/{id}/comments repository repoGetPullReviewComments + // --- + // summary: Get a specific review for a pull request. + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: index + // in: path + // description: index of the pull request + // type: integer + // format: int64 + // required: true + // - name: id + // in: path + // description: id of the review + // type: integer + // format: int64 + // required: true + // responses: + // "200": + // "$ref": "#/responses/PullReviewCommentList" + // "404": + // "$ref": "#/responses/notFound" + + pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) + if err != nil { + if models.IsErrPullRequestNotExist(err) { + ctx.NotFound("GetPullRequestByIndex", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) + } + return + } + + review, err := models.GetReviewByID(ctx.ParamsInt64(":id")) + if err != nil { + if models.IsErrReviewNotExist(err) { + ctx.NotFound("GetReviewByID", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) + } + return + } + + // validate the the review is for the given PR + if review.IssueID != pr.IssueID { + ctx.NotFound("ReviewNotInPR", err) + return + } + + // make sure that the user has access to this review if it is pending + if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID { + ctx.NotFound("GetReviewByID", err) + return + } + + err = pr.LoadIssue() + if err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + return + } + + err = review.LoadAttributes() + if err != nil { + ctx.Error(http.StatusInternalServerError, "LoadAttributes", err) + return + } + + err = review.LoadCodeComments() + if err != nil { + ctx.Error(http.StatusInternalServerError, "LoadCodeComments", err) + return + } + + err = review.Issue.LoadRepo() + if err != nil { + ctx.Error(http.StatusInternalServerError, "LoadRepo", err) + return + } + + var apiComments []structs.PullRequestReviewComment + + for _, lines := range review.CodeComments { + for _, comments := range lines { + for _, comment := range comments { + apiComment := structs.PullRequestReviewComment{ + ID: comment.ID, + URL: comment.HTMLURL(), + PRURL: review.Issue.APIURL(), + ReviewID: review.ID, + Path: comment.TreePath, + CommitID: comment.CommitSHA, + DiffHunk: comment.Patch, + Reviewer: review.Reviewer.APIFormat(), + Body: comment.Content, + } + if comment.Line < 0 { + apiComment.OldLineNum = comment.UnsignedLine() + } else { + apiComment.LineNum = comment.UnsignedLine() + } + apiComments = append(apiComments, apiComment) + } + } + } + + ctx.JSON(http.StatusOK, &apiComments) +} diff --git a/routers/api/v1/swagger/repo.go b/routers/api/v1/swagger/repo.go index 4ac5c6d2d50b5..0b7acb6453f78 100644 --- a/routers/api/v1/swagger/repo.go +++ b/routers/api/v1/swagger/repo.go @@ -127,6 +127,34 @@ type swaggerResponsePullRequestList struct { Body []api.PullRequest `json:"body"` } +// PullReview +// swagger:response PullReview +type swaggerResponsePullReview struct { + // in:body + Body api.PullRequestReview `json:"body"` +} + +// PullReviewList +// swagger:response PullReviewList +type swaggerResponsePullReviewList struct { + // in:body + Body []api.PullRequestReview `json:"body"` +} + +// PullComment +// swagger:response PullReviewComment +type swaggerPullReviewComment struct { + // in:body + Body api.PullRequestReviewComment `json:"body"` +} + +// PullCommentList +// swagger:response PullReviewCommentList +type swaggerResponsePullReviewCommentList struct { + // in:body + Body []api.PullRequestReviewComment `json:"body"` +} + // Status // swagger:response Status type swaggerResponseStatus struct { @@ -158,35 +186,35 @@ type swaggerResponseSearchResults struct { // AttachmentList // swagger:response AttachmentList type swaggerResponseAttachmentList struct { - //in: body + // in: body Body []api.Attachment `json:"body"` } // Attachment // swagger:response Attachment type swaggerResponseAttachment struct { - //in: body + // in: body Body api.Attachment `json:"body"` } // GitTreeResponse // swagger:response GitTreeResponse type swaggerGitTreeResponse struct { - //in: body + // in: body Body api.GitTreeResponse `json:"body"` } // GitBlobResponse // swagger:response GitBlobResponse type swaggerGitBlobResponse struct { - //in: body + // in: body Body api.GitBlobResponse `json:"body"` } // Commit // swagger:response Commit type swaggerCommit struct { - //in: body + // in: body Body api.Commit `json:"body"` } @@ -208,28 +236,28 @@ type swaggerCommitList struct { // True if there is another page HasMore bool `json:"X-HasMore"` - //in: body + // in: body Body []api.Commit `json:"body"` } // EmptyRepository // swagger:response EmptyRepository type swaggerEmptyRepository struct { - //in: body + // in: body Body api.APIError `json:"body"` } // FileResponse // swagger:response FileResponse type swaggerFileResponse struct { - //in: body + // in: body Body api.FileResponse `json:"body"` } // ContentsResponse // swagger:response ContentsResponse type swaggerContentsResponse struct { - //in: body + // in: body Body api.ContentsResponse `json:"body"` } @@ -243,20 +271,20 @@ type swaggerContentsListResponse struct { // FileDeleteResponse // swagger:response FileDeleteResponse type swaggerFileDeleteResponse struct { - //in: body + // in: body Body api.FileDeleteResponse `json:"body"` } // TopicListResponse // swagger:response TopicListResponse type swaggerTopicListResponse struct { - //in: body + // in: body Body []api.TopicResponse `json:"body"` } // TopicNames // swagger:response TopicNames type swaggerTopicNames struct { - //in: body + // in: body Body api.TopicName `json:"body"` } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 11af8e035bc68..07042e99a97ea 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -6143,6 +6143,154 @@ } } }, + "/repos/{owner}/{repo}/pulls/{index}/reviews": { + "get": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "List all reviews for a pull request.", + "operationId": "repoListPullReviews", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "index of the pull request", + "name": "index", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "$ref": "#/responses/PullReviewList" + }, + "404": { + "$ref": "#/responses/notFound" + } + } + } + }, + "/repos/{owner}/{repo}/pulls/{index}/reviews/{id}": { + "get": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Get a specific review for a pull request.", + "operationId": "repoGetPullReview", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "index of the pull request", + "name": "index", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "id of the review", + "name": "id", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "$ref": "#/responses/PullReview" + }, + "404": { + "$ref": "#/responses/notFound" + } + } + } + }, + "/repos/{owner}/{repo}/pulls/{index}/reviews/{id}/comments": { + "get": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Get a specific review for a pull request.", + "operationId": "repoGetPullReviewComments", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "index of the pull request", + "name": "index", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "id of the review", + "name": "id", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "$ref": "#/responses/PullReviewCommentList" + }, + "404": { + "$ref": "#/responses/notFound" + } + } + } + }, "/repos/{owner}/{repo}/raw/{filepath}": { "get": { "produces": [ @@ -11968,6 +12116,92 @@ }, "x-go-package": "code.gitea.io/gitea/modules/structs" }, + "PullRequestReview": { + "description": "PullRequestReview represents a pull request review", + "type": "object", + "properties": { + "body": { + "type": "string", + "x-go-name": "Body" + }, + "commit_id": { + "type": "string", + "x-go-name": "CommitID" + }, + "created_at": { + "type": "string", + "format": "date-time", + "x-go-name": "Created" + }, + "id": { + "type": "integer", + "format": "int64", + "x-go-name": "ID" + }, + "pull_request_url": { + "type": "string", + "x-go-name": "PRURL" + }, + "type": { + "type": "string", + "x-go-name": "Type" + }, + "user": { + "$ref": "#/definitions/User" + } + }, + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, + "PullRequestReviewComment": { + "description": "PullRequestReviewComment represents a comment on a pull request", + "type": "object", + "properties": { + "body": { + "type": "string", + "x-go-name": "Body" + }, + "commit_id": { + "type": "string", + "x-go-name": "CommitID" + }, + "diff_hunk": { + "type": "string", + "x-go-name": "DiffHunk" + }, + "id": { + "type": "integer", + "format": "int64", + "x-go-name": "ID" + }, + "original_position": { + "type": "integer", + "format": "uint64", + "x-go-name": "OldLineNum" + }, + "path": { + "type": "string", + "x-go-name": "Path" + }, + "position": { + "type": "integer", + "format": "uint64", + "x-go-name": "LineNum" + }, + "pull_request_review_id": { + "type": "integer", + "format": "int64", + "x-go-name": "ReviewID" + }, + "pull_request_url": { + "type": "string", + "x-go-name": "PRURL" + }, + "user": { + "$ref": "#/definitions/User" + } + }, + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, "Reaction": { "description": "Reaction contain one reaction", "type": "object", @@ -13088,6 +13322,36 @@ } } }, + "PullReview": { + "description": "PullReview", + "schema": { + "$ref": "#/definitions/PullRequestReview" + } + }, + "PullReviewComment": { + "description": "PullComment", + "schema": { + "$ref": "#/definitions/PullRequestReviewComment" + } + }, + "PullReviewCommentList": { + "description": "PullCommentList", + "schema": { + "type": "array", + "items": { + "$ref": "#/definitions/PullRequestReviewComment" + } + } + }, + "PullReviewList": { + "description": "PullReviewList", + "schema": { + "type": "array", + "items": { + "$ref": "#/definitions/PullRequestReview" + } + } + }, "Reaction": { "description": "Reaction", "schema": { From 70dfe71f0e9d91bfa7e4e40a4f25489350953894 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 27 Apr 2020 02:21:21 +0200 Subject: [PATCH 02/26] Update Structs, move Conversion, Refactor --- models/review.go | 6 ++ modules/convert/pull_review.go | 91 ++++++++++++++++++++ modules/structs/pull_review.go | 68 ++++++++++----- routers/api/v1/repo/pull_review.go | 133 ++++------------------------- routers/api/v1/swagger/repo.go | 8 +- 5 files changed, 162 insertions(+), 144 deletions(-) create mode 100644 modules/convert/pull_review.go diff --git a/models/review.go b/models/review.go index bbd4983fc26a7..0d45ca03b107b 100644 --- a/models/review.go +++ b/models/review.go @@ -110,6 +110,12 @@ func (r *Review) loadAttributes(e Engine) (err error) { if err = r.loadIssue(e); err != nil { return } + if err = r.loadCodeComments(e); err != nil { + return + } + if err = r.loadCodeComments(e); err != nil { + return + } return } diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go new file mode 100644 index 0000000000000..b3e8cadccbae8 --- /dev/null +++ b/modules/convert/pull_review.go @@ -0,0 +1,91 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package convert + +import ( + "code.gitea.io/gitea/models" + api "code.gitea.io/gitea/modules/structs" +) + +func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) { + + if err := r.LoadAttributes(); err != nil { + return nil, err + } + + result := &api.PullReview{ + ID: r.ID, + Reviewer: ToUser(r.Reviewer, doer != nil, doer.IsAdmin || doer.ID == r.ReviewerID), + State: api.ReviewStateUnknown, + Body: r.Content, + CommitID: r.CommitID, + Stale: r.Stale, + Official: r.Official, + Submitted: r.CreatedUnix.AsTime(), + PRURL: r.Issue.HTMLURL(), + } + + switch r.Type { + case models.ReviewTypeApprove: + result.State = api.ReviewStateApproved + case models.ReviewTypeReject: + result.State = api.ReviewStateRequestChanges + case models.ReviewTypeComment: + result.State = api.ReviewStateComment + case models.ReviewTypePending: + result.State = api.ReviewStatePending + } + + return result, nil +} + +func ToPullReviewList(rl []*models.Review, doer *models.User) ([]*api.PullReview, error) { + result := make([]*api.PullReview, 0, len(rl)) + for i := range rl { + // show pending reviews only for the user who created them + if rl[i].Type == models.ReviewTypePending && !(doer.IsAdmin || doer.ID == rl[i].ReviewerID) { + continue + } + r, err := ToPullReview(rl[i], doer) + if err != nil { + return nil, err + } + result = append(result, r) + } + return result, nil +} + +func ToPullReviewCommentList(r *models.Review, doer *models.User) ([]*api.PullReviewComment, error) { + if err := r.LoadAttributes(); err != nil { + return nil, err + } + + apiComments := make([]*api.PullReviewComment, 0, len(r.CodeComments)) + + for _, lines := range r.CodeComments { + for _, comments := range lines { + for _, comment := range comments { + apiComment := api.PullReviewComment{ + ID: comment.ID, + HTMLURL: comment.HTMLURL(), + HTMLPullURL: r.Issue.APIURL(), + ReviewID: r.ID, + Path: comment.TreePath, + CommitID: comment.CommitSHA, + DiffHunk: comment.Patch, + Reviewer: ToUser(r.Reviewer, doer != nil, doer.IsAdmin || doer.ID == r.ReviewerID), + Body: comment.Content, + } + if comment.Line < 0 { + apiComment.OldLineNum = comment.UnsignedLine() + } else { + apiComment.LineNum = comment.UnsignedLine() + } + apiComments = append(apiComments, &apiComment) + } + } + } + return apiComments, nil +} diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index 977a96f9599f9..0d8eaa202aa69 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -8,33 +8,57 @@ import ( "time" ) -// PullRequestReview represents a pull request review -type PullRequestReview struct { +// ReviewStateType review state type +type ReviewStateType string + +const ( + // ReviewStateApproved pr is approved + ReviewStateApproved ReviewStateType = "APPROVED" + // ReviewStatePending pr state is pending + ReviewStatePending ReviewStateType = "PENDING" + // ReviewStateComment is a comment review + ReviewStateComment ReviewStateType = "COMMENT" + // ReviewStateRequestChanges changes for pr are requested + ReviewStateRequestChanges ReviewStateType = "REQUEST_CHANGES" + // ReviewStateUnknown state of pr is unknown + ReviewStateUnknown ReviewStateType = "UNKNOWN" +) + +// PullReview represents a pull request review +type PullReview struct { + ID int64 `json:"id"` + Reviewer *User `json:"user"` + State ReviewStateType `json:"state"` + Body string `json:"body"` + CommitID string `json:"commit_id"` + Stale bool `json:"stale"` + Official bool `json:"official"` + // swagger:strfmt date-time + Submitted time.Time `json:"submitted_at"` + + PRURL string `json:"pull_request_url"` +} + +// PullReviewComment represents a comment on a pull request +type PullReviewComment struct { ID int64 `json:"id"` - PRURL string `json:"pull_request_url"` - Reviewer *User `json:"user"` Body string `json:"body"` - CommitID string `json:"commit_id"` - Type string `json:"type"` + Reviewer *User `json:"user"` + ReviewID int64 `json:"pull_request_review_id"` + InReplayToID int64 `json:"in_reply_to_id"` // swagger:strfmt date-time Created time.Time `json:"created_at"` + // swagger:strfmt date-time + Updated time.Time `json:"updated_at"` - // TODO: is there a way to get a URL to the review itself? - // HTMLURL string `json:"html_url"` -} + Path string `json:"path"` + CommitID string `json:"commit_id"` + OrigCommitID string `json:"original_commit_id"` + DiffHunk string `json:"diff_hunk"` + LineNum uint64 `json:"position"` + OldLineNum uint64 `json:"original_position"` -// PullRequestReviewComment represents a comment on a pull request -type PullRequestReviewComment struct { - ID int64 `json:"id"` - URL string `json:"url"` - PRURL string `json:"pull_request_url"` - ReviewID int64 `json:"pull_request_review_id"` - Path string `json:"path"` - CommitID string `json:"commit_id"` - DiffHunk string `json:"diff_hunk"` - LineNum uint64 `json:"position"` - OldLineNum uint64 `json:"original_position"` - Reviewer *User `json:"user"` - Body string `json:"body"` + HTMLURL string `json:"html_url"` + HTMLPullURL string `json:"pull_request_url"` } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 6a1050b7d857b..2b2bc11045b1a 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -9,7 +9,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" - "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/convert" ) // ListPullReviews lists all reviews of a pull request @@ -72,42 +72,10 @@ func ListPullReviews(ctx *context.APIContext) { return } - var apiReviews []structs.PullRequestReview - for _, review := range allReviews { - // show pending reviews only for the user who created them - if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID { - continue - } - - if err = review.LoadReviewer(); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadReviewer", err) - return - } - - var reviewType string - switch review.Type { - case models.ReviewTypeApprove: - reviewType = "APPROVE" - case models.ReviewTypeReject: - reviewType = "REJECT" - case models.ReviewTypeComment: - reviewType = "COMMENT" - case models.ReviewTypePending: - reviewType = "PENDING" - default: - reviewType = "UNKNOWN" - - } - - apiReviews = append(apiReviews, structs.PullRequestReview{ - ID: review.ID, - PRURL: pr.Issue.APIURL(), - Reviewer: review.Reviewer.APIFormat(), - Body: review.Content, - Created: review.CreatedUnix.AsTime(), - CommitID: review.CommitID, - Type: reviewType, - }) + apiReviews, err := convert.ToPullReviewList(allReviews, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "convertToPullReviewList", err) + return } ctx.JSON(http.StatusOK, &apiReviews) @@ -176,51 +144,18 @@ func GetPullReview(ctx *context.APIContext) { } // make sure that the user has access to this review if it is pending - if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID { + if review.Type == models.ReviewTypePending && (ctx.User.IsAdmin || review.ReviewerID != ctx.User.ID) { ctx.NotFound("GetReviewByID", err) return } - if err = pr.LoadIssue(); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadIssue", err) - return - } - - if err = pr.Issue.LoadRepo(); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadIssue", err) - return - } - - if err = review.LoadReviewer(); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadReviewer", err) + apiReview, err := convert.ToPullReview(review, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "convertToPullReview", err) return } - var reviewType string - switch review.Type { - case models.ReviewTypeApprove: - reviewType = "APPROVE" - case models.ReviewTypeReject: - reviewType = "REJECT" - case models.ReviewTypeComment: - reviewType = "COMMENT" - case models.ReviewTypePending: - reviewType = "PENDING" - default: - reviewType = "UNKNOWN" - - } - apiReview := structs.PullRequestReview{ - ID: review.ID, - PRURL: pr.Issue.APIURL(), - Reviewer: review.Reviewer.APIFormat(), - Body: review.Content, - Created: review.CreatedUnix.AsTime(), - CommitID: review.CommitID, - Type: reviewType, - } - - ctx.JSON(http.StatusOK, &apiReview) + ctx.JSON(http.StatusOK, apiReview) } // GetPullReviewComments lists all comments of a pull request review @@ -291,55 +226,17 @@ func GetPullReviewComments(ctx *context.APIContext) { return } - err = pr.LoadIssue() - if err != nil { - ctx.Error(http.StatusInternalServerError, "LoadIssue", err) - return - } - - err = review.LoadAttributes() - if err != nil { - ctx.Error(http.StatusInternalServerError, "LoadAttributes", err) - return - } - - err = review.LoadCodeComments() - if err != nil { - ctx.Error(http.StatusInternalServerError, "LoadCodeComments", err) - return - } - err = review.Issue.LoadRepo() if err != nil { ctx.Error(http.StatusInternalServerError, "LoadRepo", err) return } - var apiComments []structs.PullRequestReviewComment - - for _, lines := range review.CodeComments { - for _, comments := range lines { - for _, comment := range comments { - apiComment := structs.PullRequestReviewComment{ - ID: comment.ID, - URL: comment.HTMLURL(), - PRURL: review.Issue.APIURL(), - ReviewID: review.ID, - Path: comment.TreePath, - CommitID: comment.CommitSHA, - DiffHunk: comment.Patch, - Reviewer: review.Reviewer.APIFormat(), - Body: comment.Content, - } - if comment.Line < 0 { - apiComment.OldLineNum = comment.UnsignedLine() - } else { - apiComment.LineNum = comment.UnsignedLine() - } - apiComments = append(apiComments, apiComment) - } - } + apiComments, err := convert.ToPullReviewCommentList(review, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "convertToPullReviewCommentList", err) + return } - ctx.JSON(http.StatusOK, &apiComments) + ctx.JSON(http.StatusOK, apiComments) } diff --git a/routers/api/v1/swagger/repo.go b/routers/api/v1/swagger/repo.go index cdda8b97e6e45..bcbc2b5fa98c1 100644 --- a/routers/api/v1/swagger/repo.go +++ b/routers/api/v1/swagger/repo.go @@ -145,28 +145,28 @@ type swaggerResponsePullRequestList struct { // swagger:response PullReview type swaggerResponsePullReview struct { // in:body - Body api.PullRequestReview `json:"body"` + Body api.PullReview `json:"body"` } // PullReviewList // swagger:response PullReviewList type swaggerResponsePullReviewList struct { // in:body - Body []api.PullRequestReview `json:"body"` + Body []api.PullReview `json:"body"` } // PullComment // swagger:response PullReviewComment type swaggerPullReviewComment struct { // in:body - Body api.PullRequestReviewComment `json:"body"` + Body api.PullReviewComment `json:"body"` } // PullCommentList // swagger:response PullReviewCommentList type swaggerResponsePullReviewCommentList struct { // in:body - Body []api.PullRequestReviewComment `json:"body"` + Body []api.PullReviewComment `json:"body"` } // Status From 644aa1c7525f0ee5327127eec68668615e262343 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 27 Apr 2020 02:46:00 +0200 Subject: [PATCH 03/26] refactor --- modules/convert/pull_review.go | 34 +++++++++++++++++++--------------- modules/structs/pull_review.go | 1 - 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go index b3e8cadccbae8..70ac57afdb3a8 100644 --- a/modules/convert/pull_review.go +++ b/modules/convert/pull_review.go @@ -57,33 +57,37 @@ func ToPullReviewList(rl []*models.Review, doer *models.User) ([]*api.PullReview return result, nil } -func ToPullReviewCommentList(r *models.Review, doer *models.User) ([]*api.PullReviewComment, error) { - if err := r.LoadAttributes(); err != nil { +func ToPullReviewCommentList(review *models.Review, doer *models.User) ([]*api.PullReviewComment, error) { + if err := review.LoadAttributes(); err != nil { return nil, err } - apiComments := make([]*api.PullReviewComment, 0, len(r.CodeComments)) + apiComments := make([]*api.PullReviewComment, 0, len(review.CodeComments)) - for _, lines := range r.CodeComments { + for _, lines := range review.CodeComments { for _, comments := range lines { for _, comment := range comments { - apiComment := api.PullReviewComment{ - ID: comment.ID, - HTMLURL: comment.HTMLURL(), - HTMLPullURL: r.Issue.APIURL(), - ReviewID: r.ID, - Path: comment.TreePath, - CommitID: comment.CommitSHA, - DiffHunk: comment.Patch, - Reviewer: ToUser(r.Reviewer, doer != nil, doer.IsAdmin || doer.ID == r.ReviewerID), - Body: comment.Content, + apiComment := &api.PullReviewComment{ + ID: comment.ID, + Body: comment.Content, + Reviewer: ToUser(review.Reviewer, doer != nil, doer.IsAdmin || doer.ID == review.ReviewerID), + ReviewID: review.ID, + Created: comment.CreatedUnix.AsTime(), + Updated: comment.UpdatedUnix.AsTime(), + Path: comment.TreePath, + CommitID: comment.CommitSHA, + OrigCommitID: comment.OldRef, + DiffHunk: comment.Patch, + HTMLURL: comment.HTMLURL(), + HTMLPullURL: review.Issue.APIURL(), } + if comment.Line < 0 { apiComment.OldLineNum = comment.UnsignedLine() } else { apiComment.LineNum = comment.UnsignedLine() } - apiComments = append(apiComments, &apiComment) + apiComments = append(apiComments, apiComment) } } } diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index 0d8eaa202aa69..6eaa9e292e68c 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -46,7 +46,6 @@ type PullReviewComment struct { Reviewer *User `json:"user"` ReviewID int64 `json:"pull_request_review_id"` - InReplayToID int64 `json:"in_reply_to_id"` // swagger:strfmt date-time Created time.Time `json:"created_at"` // swagger:strfmt date-time From 79eca0b13fbc54665a7cc13261f6d8ca3a3063ae Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 27 Apr 2020 02:53:17 +0200 Subject: [PATCH 04/26] lint & co --- modules/convert/pull_review.go | 3 ++ templates/swagger/v1_json.tmpl | 62 +++++++++++++++++++++++++--------- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go index 70ac57afdb3a8..df6c687b16ad8 100644 --- a/modules/convert/pull_review.go +++ b/modules/convert/pull_review.go @@ -9,6 +9,7 @@ import ( api "code.gitea.io/gitea/modules/structs" ) +// ToPullReview convert a review to api format func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) { if err := r.LoadAttributes(); err != nil { @@ -41,6 +42,7 @@ func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) return result, nil } +// ToPullReviewList convert a list of review to it's api format func ToPullReviewList(rl []*models.Review, doer *models.User) ([]*api.PullReview, error) { result := make([]*api.PullReview, 0, len(rl)) for i := range rl { @@ -57,6 +59,7 @@ func ToPullReviewList(rl []*models.Review, doer *models.User) ([]*api.PullReview return result, nil } +// ToPullReviewCommentList convert the CodeComments of an review to it's api format func ToPullReviewCommentList(review *models.Review, doer *models.User) ([]*api.PullReviewComment, error) { if err := review.LoadAttributes(); err != nil { return nil, err diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 10ffdcc469e5d..9774ced389093 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -13279,8 +13279,8 @@ }, "x-go-package": "code.gitea.io/gitea/modules/structs" }, - "PullRequestReview": { - "description": "PullRequestReview represents a pull request review", + "PullReview": { + "description": "PullReview represents a pull request review", "type": "object", "properties": { "body": { @@ -13291,23 +13291,30 @@ "type": "string", "x-go-name": "CommitID" }, - "created_at": { - "type": "string", - "format": "date-time", - "x-go-name": "Created" - }, "id": { "type": "integer", "format": "int64", "x-go-name": "ID" }, + "official": { + "type": "boolean", + "x-go-name": "Official" + }, "pull_request_url": { "type": "string", "x-go-name": "PRURL" }, - "type": { + "stale": { + "type": "boolean", + "x-go-name": "Stale" + }, + "state": { + "$ref": "#/definitions/ReviewStateType" + }, + "submitted_at": { "type": "string", - "x-go-name": "Type" + "format": "date-time", + "x-go-name": "Submitted" }, "user": { "$ref": "#/definitions/User" @@ -13315,8 +13322,8 @@ }, "x-go-package": "code.gitea.io/gitea/modules/structs" }, - "PullRequestReviewComment": { - "description": "PullRequestReviewComment represents a comment on a pull request", + "PullReviewComment": { + "description": "PullReviewComment represents a comment on a pull request", "type": "object", "properties": { "body": { @@ -13327,15 +13334,28 @@ "type": "string", "x-go-name": "CommitID" }, + "created_at": { + "type": "string", + "format": "date-time", + "x-go-name": "Created" + }, "diff_hunk": { "type": "string", "x-go-name": "DiffHunk" }, + "html_url": { + "type": "string", + "x-go-name": "HTMLURL" + }, "id": { "type": "integer", "format": "int64", "x-go-name": "ID" }, + "original_commit_id": { + "type": "string", + "x-go-name": "OrigCommitID" + }, "original_position": { "type": "integer", "format": "uint64", @@ -13357,7 +13377,12 @@ }, "pull_request_url": { "type": "string", - "x-go-name": "PRURL" + "x-go-name": "HTMLPullURL" + }, + "updated_at": { + "type": "string", + "format": "date-time", + "x-go-name": "Updated" }, "user": { "$ref": "#/definitions/User" @@ -13708,6 +13733,11 @@ }, "x-go-package": "code.gitea.io/gitea/modules/structs" }, + "ReviewStateType": { + "description": "ReviewStateType review state type", + "type": "string", + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, "SearchResults": { "description": "SearchResults results of a successful search", "type": "object", @@ -14549,13 +14579,13 @@ "PullReview": { "description": "PullReview", "schema": { - "$ref": "#/definitions/PullRequestReview" + "$ref": "#/definitions/PullReview" } }, "PullReviewComment": { "description": "PullComment", "schema": { - "$ref": "#/definitions/PullRequestReviewComment" + "$ref": "#/definitions/PullReviewComment" } }, "PullReviewCommentList": { @@ -14563,7 +14593,7 @@ "schema": { "type": "array", "items": { - "$ref": "#/definitions/PullRequestReviewComment" + "$ref": "#/definitions/PullReviewComment" } } }, @@ -14572,7 +14602,7 @@ "schema": { "type": "array", "items": { - "$ref": "#/definitions/PullRequestReview" + "$ref": "#/definitions/PullReview" } } }, From 0ce765ccb791a3fcb7dcd80705ce311dbac62183 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 27 Apr 2020 03:16:35 +0200 Subject: [PATCH 05/26] fix lint + refactor --- models/review.go | 3 --- modules/convert/pull_review.go | 30 ++++++++++++++++++++---------- modules/structs/pull_review.go | 3 ++- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/models/review.go b/models/review.go index 0d45ca03b107b..0e811b174c8de 100644 --- a/models/review.go +++ b/models/review.go @@ -113,9 +113,6 @@ func (r *Review) loadAttributes(e Engine) (err error) { if err = r.loadCodeComments(e); err != nil { return } - if err = r.loadCodeComments(e); err != nil { - return - } return } diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go index df6c687b16ad8..7b4a281affd4d 100644 --- a/modules/convert/pull_review.go +++ b/modules/convert/pull_review.go @@ -16,16 +16,21 @@ func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) return nil, err } + auth := false + if doer != nil { + auth = doer.IsAdmin || doer.ID == r.ReviewerID + } + result := &api.PullReview{ - ID: r.ID, - Reviewer: ToUser(r.Reviewer, doer != nil, doer.IsAdmin || doer.ID == r.ReviewerID), - State: api.ReviewStateUnknown, - Body: r.Content, - CommitID: r.CommitID, - Stale: r.Stale, - Official: r.Official, - Submitted: r.CreatedUnix.AsTime(), - PRURL: r.Issue.HTMLURL(), + ID: r.ID, + Reviewer: ToUser(r.Reviewer, doer != nil, auth), + State: api.ReviewStateUnknown, + Body: r.Content, + CommitID: r.CommitID, + Stale: r.Stale, + Official: r.Official, + Submitted: r.CreatedUnix.AsTime(), + HTMLPullURL: r.Issue.HTMLURL(), } switch r.Type { @@ -67,13 +72,18 @@ func ToPullReviewCommentList(review *models.Review, doer *models.User) ([]*api.P apiComments := make([]*api.PullReviewComment, 0, len(review.CodeComments)) + auth := false + if doer != nil { + auth = doer.IsAdmin || doer.ID == review.ReviewerID + } + for _, lines := range review.CodeComments { for _, comments := range lines { for _, comment := range comments { apiComment := &api.PullReviewComment{ ID: comment.ID, Body: comment.Content, - Reviewer: ToUser(review.Reviewer, doer != nil, doer.IsAdmin || doer.ID == review.ReviewerID), + Reviewer: ToUser(review.Reviewer, doer != nil, auth), ReviewID: review.ID, Created: comment.CreatedUnix.AsTime(), Updated: comment.UpdatedUnix.AsTime(), diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index 6eaa9e292e68c..3ea7b36f0072d 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -36,7 +36,8 @@ type PullReview struct { // swagger:strfmt date-time Submitted time.Time `json:"submitted_at"` - PRURL string `json:"pull_request_url"` + // HTMLURL string `json:"html_url"` // blocked by https://github.com/go-gitea/gitea/issues/11226 + HTMLPullURL string `json:"pull_request_url"` } // PullReviewComment represents a comment on a pull request From d860be8a4ae005b997b3834e0f666c91327fec0e Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 27 Apr 2020 07:55:42 +0200 Subject: [PATCH 06/26] add new Review state, rm unessesary, refacotr loadAttributes, convert patch to diff --- models/review.go | 16 ++++++++++++---- modules/convert/pull_review.go | 14 +++++++++++++- modules/structs/pull_review.go | 2 ++ routers/api/v1/repo/pull_review.go | 6 ------ 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/models/review.go b/models/review.go index 0e811b174c8de..6ac34736932e8 100644 --- a/models/review.go +++ b/models/review.go @@ -86,15 +86,23 @@ func (r *Review) LoadCodeComments() error { } func (r *Review) loadIssue(e Engine) (err error) { - r.Issue, err = getIssueByID(e, r.IssueID) + if r.Issue == nil { + r.Issue, err = getIssueByID(e, r.IssueID) + } return } func (r *Review) loadReviewer(e Engine) (err error) { - if r.ReviewerID == 0 { - return nil + if r.Reviewer == nil { + if r.ReviewerID == 0 { + return nil + } + r.Reviewer, err = getUserByID(e, r.ReviewerID) + if err != nil && IsErrUserNotExist(err) { + r.Reviewer = NewGhostUser() + err = nil + } } - r.Reviewer, err = getUserByID(e, r.ReviewerID) return } diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go index 7b4a281affd4d..817059ecbfd4c 100644 --- a/modules/convert/pull_review.go +++ b/modules/convert/pull_review.go @@ -5,6 +5,8 @@ package convert import ( + "strings" + "code.gitea.io/gitea/models" api "code.gitea.io/gitea/modules/structs" ) @@ -42,6 +44,8 @@ func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) result.State = api.ReviewStateComment case models.ReviewTypePending: result.State = api.ReviewStatePending + case models.ReviewTypeRequest: + result.State = api.ReviewStateRequestReview } return result, nil @@ -90,7 +94,7 @@ func ToPullReviewCommentList(review *models.Review, doer *models.User) ([]*api.P Path: comment.TreePath, CommitID: comment.CommitSHA, OrigCommitID: comment.OldRef, - DiffHunk: comment.Patch, + DiffHunk: patch2diff(comment.Patch), HTMLURL: comment.HTMLURL(), HTMLPullURL: review.Issue.APIURL(), } @@ -106,3 +110,11 @@ func ToPullReviewCommentList(review *models.Review, doer *models.User) ([]*api.P } return apiComments, nil } + +func patch2diff(patch string) string { + split := strings.Split(patch, "\n@@") + if len(split) == 2 { + return "@@" + split[1] + } + return "" +} diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index 3ea7b36f0072d..8c2a08e3a3e5c 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -21,6 +21,8 @@ const ( // ReviewStateRequestChanges changes for pr are requested ReviewStateRequestChanges ReviewStateType = "REQUEST_CHANGES" // ReviewStateUnknown state of pr is unknown + ReviewStateRequestReview ReviewStateType = "REQUEST_REVIEW" + // ReviewStateUnknown state of pr is unknown ReviewStateUnknown ReviewStateType = "UNKNOWN" ) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 2b2bc11045b1a..f41989a49e1ed 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -226,12 +226,6 @@ func GetPullReviewComments(ctx *context.APIContext) { return } - err = review.Issue.LoadRepo() - if err != nil { - ctx.Error(http.StatusInternalServerError, "LoadRepo", err) - return - } - apiComments, err := convert.ToPullReviewCommentList(review, ctx.User) if err != nil { ctx.Error(http.StatusInternalServerError, "convertToPullReviewCommentList", err) From a4b998cf7ad0bf8a5add69acce5debba838bed50 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 27 Apr 2020 08:42:07 +0200 Subject: [PATCH 07/26] add DeletePullReview --- models/review.go | 33 +++++++++++ modules/structs/pull_review.go | 3 +- routers/api/v1/api.go | 4 +- routers/api/v1/repo/pull_review.go | 92 ++++++++++++++++++++++++++---- templates/swagger/v1_json.tmpl | 55 +++++++++++++++++- 5 files changed, 173 insertions(+), 14 deletions(-) diff --git a/models/review.go b/models/review.go index 6ac34736932e8..64d2a001df4cc 100644 --- a/models/review.go +++ b/models/review.go @@ -74,6 +74,9 @@ type Review struct { } func (r *Review) loadCodeComments(e Engine) (err error) { + if err = r.loadIssue(e); err != nil { + return err + } if r.CodeComments == nil { r.CodeComments, err = fetchCodeCommentsByReview(e, r.Issue, nil, r) } @@ -667,3 +670,33 @@ func CanMarkConversation(issue *Issue, doer *User) (permResult bool, err error) return true, nil } + +// DeleteReview delete a review and it's code comments +func DeleteReview(r *Review) error { + sess := x.NewSession() + defer sess.Close() + + if err := sess.Begin(); err != nil { + return err + } + + if err := r.loadCodeComments(sess); err != nil { + return err + } + + opts := FindCommentsOptions{ + Type: CommentTypeCode, + IssueID: r.IssueID, + ReviewID: r.ID, + } + + if _, err := sess.Where(opts.toConds()).Delete(new(Comment)); err != nil { + return err + } + + if _, err := sess.ID(r.ID).Delete(new(Review)); err != nil { + return err + } + + return sess.Commit() +} diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index 8c2a08e3a3e5c..0cca957ed2e00 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -38,7 +38,8 @@ type PullReview struct { // swagger:strfmt date-time Submitted time.Time `json:"submitted_at"` - // HTMLURL string `json:"html_url"` // blocked by https://github.com/go-gitea/gitea/issues/11226 + // HTMLURL string `json:"html_url"` // ToDo: blocked by https://github.com/go-gitea/gitea/issues/11226 + HTMLPullURL string `json:"pull_request_url"` } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 48ad33b0cb3c8..9a1516990d746 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -797,7 +797,9 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/reviews", func() { m.Combo("").Get(repo.ListPullReviews) m.Group("/:id", func() { - m.Combo("").Get(repo.GetPullReview) + m.Combo(""). + Get(repo.GetPullReview). + Delete(repo.DeletePullReview) m.Combo("/comments").Get(repo.GetPullReviewComments) }) }) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index f41989a49e1ed..f25af6f7b8caf 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -5,6 +5,7 @@ package repo import ( + "fmt" "net/http" "code.gitea.io/gitea/models" @@ -194,6 +195,81 @@ func GetPullReviewComments(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" + review, statusSet := prepareSingleReview(ctx) + if statusSet { + return + } + + apiComments, err := convert.ToPullReviewCommentList(review, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "convertToPullReviewCommentList", err) + return + } + + ctx.JSON(http.StatusOK, apiComments) +} + +// DeletePullReview delete a specific review from a pull request +func DeletePullReview(ctx *context.APIContext) { + // swagger:operation DELETE /repos/{owner}/{repo}/pulls/{index}/reviews/{id} repository repoDeletePullReview + // --- + // summary: Delete a specific review from a pull request + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: index + // in: path + // description: index of the pull request + // type: integer + // format: int64 + // required: true + // - name: id + // in: path + // description: id of the review + // type: integer + // format: int64 + // required: true + // responses: + // "204": + // "$ref": "#/responses/empty" + // "403": + // "$ref": "#/responses/forbidden" + // "404": + // "$ref": "#/responses/notFound" + + review, statusSet := prepareSingleReview(ctx) + if statusSet { + return + } + + if ctx.User == nil { + ctx.NotFound() + return + } + if !ctx.User.IsAdmin && ctx.User.ID != review.ReviewerID { + ctx.Error(http.StatusForbidden, "only admin and user itself can delete a review", nil) + return + } + + if err := models.DeleteReview(review); err != nil { + ctx.Error(http.StatusInternalServerError, "DeleteReview", fmt.Errorf("can not delete ReviewID: %d", review.ID)) + return + } + + ctx.Status(http.StatusNoContent) +} + +func prepareSingleReview(ctx *context.APIContext) (r *models.Review, statusSet bool) { pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrPullRequestNotExist(err) { @@ -201,7 +277,7 @@ func GetPullReviewComments(ctx *context.APIContext) { } else { ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) } - return + return nil, true } review, err := models.GetReviewByID(ctx.ParamsInt64(":id")) @@ -211,26 +287,20 @@ func GetPullReviewComments(ctx *context.APIContext) { } else { ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) } - return + return nil, true } // validate the the review is for the given PR if review.IssueID != pr.IssueID { ctx.NotFound("ReviewNotInPR", err) - return + return nil, true } // make sure that the user has access to this review if it is pending if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID { ctx.NotFound("GetReviewByID", err) - return - } - - apiComments, err := convert.ToPullReviewCommentList(review, ctx.User) - if err != nil { - ctx.Error(http.StatusInternalServerError, "convertToPullReviewCommentList", err) - return + return nil, true } - ctx.JSON(http.StatusOK, apiComments) + return review, false } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 9774ced389093..c647b341791d9 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -6796,6 +6796,59 @@ "$ref": "#/responses/notFound" } } + }, + "delete": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Delete a specific review from a pull request", + "operationId": "repoDeletePullReview", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "index of the pull request", + "name": "index", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "id of the review", + "name": "id", + "in": "path", + "required": true + } + ], + "responses": { + "204": { + "$ref": "#/responses/empty" + }, + "403": { + "$ref": "#/responses/forbidden" + }, + "404": { + "$ref": "#/responses/notFound" + } + } } }, "/repos/{owner}/{repo}/pulls/{index}/reviews/{id}/comments": { @@ -13302,7 +13355,7 @@ }, "pull_request_url": { "type": "string", - "x-go-name": "PRURL" + "x-go-name": "HTMLPullURL" }, "stale": { "type": "boolean", From e1c13b04a0de69c9c1cbdb8467c4cf34355d59d0 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 27 Apr 2020 09:04:08 +0200 Subject: [PATCH 08/26] add paggination --- models/review.go | 4 ++++ routers/api/v1/repo/pull_review.go | 14 ++++++++++++-- templates/swagger/v1_json.tmpl | 12 ++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/models/review.go b/models/review.go index 64d2a001df4cc..930481e21075c 100644 --- a/models/review.go +++ b/models/review.go @@ -150,6 +150,7 @@ func GetReviewByID(id int64) (*Review, error) { // FindReviewOptions represent possible filters to find reviews type FindReviewOptions struct { + ListOptions Type ReviewType IssueID int64 ReviewerID int64 @@ -176,6 +177,9 @@ func (opts *FindReviewOptions) toCond() builder.Cond { func findReviews(e Engine, opts FindReviewOptions) ([]*Review, error) { reviews := make([]*Review, 0, 10) sess := e.Where(opts.toCond()) + if opts.Page > 0 { + sess = opts.ListOptions.setSessionPagination(sess) + } return reviews, sess. Asc("created_unix"). Asc("id"). diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index f25af6f7b8caf..7e69135757a5a 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/convert" + "code.gitea.io/gitea/routers/api/v1/utils" ) // ListPullReviews lists all reviews of a pull request @@ -37,6 +38,14 @@ func ListPullReviews(ctx *context.APIContext) { // type: integer // format: int64 // required: true + // - name: page + // in: query + // description: page number of results to return (1-based) + // type: integer + // - name: limit + // in: query + // description: page size of results, maximum page size is 50 + // type: integer // responses: // "200": // "$ref": "#/responses/PullReviewList" @@ -64,8 +73,9 @@ func ListPullReviews(ctx *context.APIContext) { } allReviews, err := models.FindReviews(models.FindReviewOptions{ - Type: models.ReviewTypeUnknown, - IssueID: pr.IssueID, + ListOptions: utils.GetListOptions(ctx), + Type: models.ReviewTypeUnknown, + IssueID: pr.IssueID, }) if err != nil { diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index c647b341791d9..a015194cc2cec 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -6734,6 +6734,18 @@ "name": "index", "in": "path", "required": true + }, + { + "type": "integer", + "description": "page number of results to return (1-based)", + "name": "page", + "in": "query" + }, + { + "type": "integer", + "description": "page size of results, maximum page size is 50", + "name": "limit", + "in": "query" } ], "responses": { From 4b60f40d1c673c2bfd9f0e88843662d74b82b24c Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 27 Apr 2020 22:04:29 +0200 Subject: [PATCH 09/26] draft1: Create & submit review --- modules/structs/pull_review.go | 29 +++- routers/api/v1/api.go | 10 +- routers/api/v1/repo/pull_review.go | 264 +++++++++++++++++++++++++---- routers/api/v1/swagger/options.go | 9 + templates/swagger/v1_json.tmpl | 191 ++++++++++++++++++++- 5 files changed, 461 insertions(+), 42 deletions(-) diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index 0cca957ed2e00..5b409c5154496 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -23,7 +23,7 @@ const ( // ReviewStateUnknown state of pr is unknown ReviewStateRequestReview ReviewStateType = "REQUEST_REVIEW" // ReviewStateUnknown state of pr is unknown - ReviewStateUnknown ReviewStateType = "UNKNOWN" + ReviewStateUnknown ReviewStateType = "" ) // PullReview represents a pull request review @@ -43,7 +43,7 @@ type PullReview struct { HTMLPullURL string `json:"pull_request_url"` } -// PullReviewComment represents a comment on a pull request +// PullReviewComment represents a comment on a pull request review type PullReviewComment struct { ID int64 `json:"id"` Body string `json:"body"` @@ -65,3 +65,28 @@ type PullReviewComment struct { HTMLURL string `json:"html_url"` HTMLPullURL string `json:"pull_request_url"` } + +// CreatePullReviewOptions are options to create a pull review +type CreatePullReviewOptions struct { + Event ReviewStateType `json:"event"` + Body string `json:"body"` + CommitID string `json:"commit_id"` + Comments []CreatePullReviewComment `json:"comments"` +} + +// CreatePullReviewComment represent a review comment for creation api +type CreatePullReviewComment struct { + // the tree path + Path string `json:"path"` + Body string `json:"body"` + // if comment to old file line or 0 + OldLineNum int64 `json:"old_position"` + // if comment to new file line or 0 + NewLineNum int64 `json:"new_position"` +} + +// SubmitPullReviewOptions are options to submit a pending pull review +type SubmitPullReviewOptions struct { + Event ReviewStateType `json:"event"` + Body string `json:"body"` +} diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 9a1516990d746..333f47c385ea3 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -795,12 +795,16 @@ func RegisterRoutes(m *macaron.Macaron) { m.Combo("/merge").Get(repo.IsPullRequestMerged). Post(reqToken(), mustNotBeArchived, bind(auth.MergePullRequestForm{}), repo.MergePullRequest) m.Group("/reviews", func() { - m.Combo("").Get(repo.ListPullReviews) + m.Combo(""). + Get(repo.ListPullReviews). + Post(reqToken(), bind(api.CreatePullRequestOption{}), repo.CreatePullReview) m.Group("/:id", func() { m.Combo(""). Get(repo.GetPullReview). - Delete(repo.DeletePullReview) - m.Combo("/comments").Get(repo.GetPullReviewComments) + Delete(reqToken(), repo.DeletePullReview). + Post(reqToken(), bind(api.SubmitPullReviewOptions{}), repo.SubmitPullReview) + m.Combo("/comments"). + Get(repo.GetPullReviewComments) }) }) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 7e69135757a5a..7a59be50efa72 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -7,18 +7,21 @@ package repo import ( "fmt" "net/http" + "strings" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/convert" + api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/routers/api/v1/utils" + pull_service "code.gitea.io/gitea/services/pull" ) // ListPullReviews lists all reviews of a pull request func ListPullReviews(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/pulls/{index}/reviews repository repoListPullReviews // --- - // summary: List all reviews for a pull request. + // summary: List all reviews for a pull request // produces: // - application/json // parameters: @@ -96,7 +99,7 @@ func ListPullReviews(ctx *context.APIContext) { func GetPullReview(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/pulls/{index}/reviews/{id} repository repoGetPullReview // --- - // summary: Get a specific review for a pull request. + // summary: Get a specific review for a pull request // produces: // - application/json // parameters: @@ -128,35 +131,8 @@ func GetPullReview(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) - if err != nil { - if models.IsErrPullRequestNotExist(err) { - ctx.NotFound("GetPullRequestByIndex", err) - } else { - ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) - } - return - } - - review, err := models.GetReviewByID(ctx.ParamsInt64(":id")) - if err != nil { - if models.IsErrReviewNotExist(err) { - ctx.NotFound("GetReviewByID", err) - } else { - ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) - } - return - } - - // validate the the review is for the given PR - if review.IssueID != pr.IssueID { - ctx.NotFound("ReviewNotInPR", err) - return - } - - // make sure that the user has access to this review if it is pending - if review.Type == models.ReviewTypePending && (ctx.User.IsAdmin || review.ReviewerID != ctx.User.ID) { - ctx.NotFound("GetReviewByID", err) + review, statusSet := prepareSingleReview(ctx) + if statusSet { return } @@ -173,7 +149,7 @@ func GetPullReview(ctx *context.APIContext) { func GetPullReviewComments(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/pulls/{index}/reviews/{id}/comments repository repoGetPullReviewComments // --- - // summary: Get a specific review for a pull request. + // summary: Get a specific review for a pull request // produces: // - application/json // parameters: @@ -314,3 +290,227 @@ func prepareSingleReview(ctx *context.APIContext) (r *models.Review, statusSet b return review, false } + +// CreatePullReview create a review to an pull request +func CreatePullReview(ctx *context.APIContext, opts api.CreatePullReviewOptions) { + // swagger:operation POST /repos/{owner}/{repo}/pulls/{index}/reviews repository repoCreatePullReview + // --- + // summary: Create a review to an pull request + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: index + // in: path + // description: index of the pull request + // type: integer + // format: int64 + // required: true + // - name: body + // in: body + // required: true + // schema: + // "$ref": "#/definitions/CreatePullRequestOption" + // responses: + // "200": + // "$ref": "#/responses/PullReview" + // "404": + // "$ref": "#/responses/notFound" + // "422": + // "$ref": "#/responses/validationError" + + pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) + if err != nil { + if models.IsErrPullRequestNotExist(err) { + ctx.NotFound("GetPullRequestByIndex", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) + } + return + } + + // determine review type + reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body) + if isWrong { + return + } + + // create review comments + for _, c := range opts.Comments { + line := c.NewLineNum + if c.OldLineNum > 0 { + line = c.OldLineNum * -1 + } + + if _, err := pull_service.CreateCodeComment( + ctx.User, + ctx.Repo.GitRepo, + pr.Issue, + line, + c.Body, + c.Path, + true, // is review + 0, // no reply + opts.CommitID, + ); err != nil { + ctx.ServerError("CreateCodeComment", err) + return + } + } + + // create review and associate all pending review comments + review, _, err := pull_service.SubmitReview(ctx.User, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, opts.CommitID) + if err != nil { + ctx.Error(http.StatusInternalServerError, "SubmitReview", err) + return + } + + // convert response + apiReview, err := convert.ToPullReview(review, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "convertToPullReview", err) + return + } + ctx.JSON(http.StatusOK, apiReview) + + return +} + +// SubmitPullReview submit a pending review to an pull request +func SubmitPullReview(ctx *context.APIContext, opts api.SubmitPullReviewOptions) { + // swagger:operation POST /repos/{owner}/{repo}/pulls/{index}/reviews/{id} repository repoSubmitPullReview + // --- + // summary: Submit a pending review to an pull request + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: index + // in: path + // description: index of the pull request + // type: integer + // format: int64 + // required: true + // - name: id + // in: path + // description: id of the review + // type: integer + // format: int64 + // required: true + // - name: body + // in: body + // required: true + // schema: + // "$ref": "#/definitions/SubmitPullReviewOptions" + // responses: + // "200": + // "$ref": "#/responses/PullReview" + // "404": + // "$ref": "#/responses/notFound" + // "422": + // "$ref": "#/responses/validationError" + + review, isWrong := prepareSingleReview(ctx) + if isWrong { + return + } + + if review.Type != models.ReviewTypePending { + ctx.JSON(http.StatusUnprocessableEntity, fmt.Errorf("only a pending review can be submitted")) + return + } + + pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) + if err != nil { + if models.IsErrPullRequestNotExist(err) { + ctx.NotFound("GetPullRequestByIndex", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) + } + return + } + + // determine review type + reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body) + if isWrong { + return + } + + headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(pr.GetGitRefName()) + if err != nil { + ctx.InternalServerError(err) + return + } + + // create review and associate all pending review comments + review, _, err = pull_service.SubmitReview(ctx.User, ctx.Repo.GitRepo, pr.Issue, reviewType, opts.Body, headCommitID) + if err != nil { + ctx.Error(http.StatusInternalServerError, "SubmitReview", err) + return + } + + // convert response + apiReview, err := convert.ToPullReview(review, ctx.User) + if err != nil { + ctx.Error(http.StatusInternalServerError, "convertToPullReview", err) + return + } + ctx.JSON(http.StatusOK, apiReview) + return +} + +func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, event api.ReviewStateType, body string) (reviewType models.ReviewType, isWrong bool) { + if err := pr.LoadIssue(); err != nil { + ctx.InternalServerError(err) + return -1, true + } + + switch event { + case api.ReviewStateApproved: + // can not approve your own PR + if pr.Issue.IsPoster(ctx.User.ID) { + ctx.JSON(http.StatusUnprocessableEntity, fmt.Errorf("approve your own pull is not allowed")) + return -1, true + } + reviewType = models.ReviewTypeApprove + + case api.ReviewStateRequestChanges: + // can not reject your own PR + if pr.Issue.IsPoster(ctx.User.ID) { + ctx.JSON(http.StatusUnprocessableEntity, fmt.Errorf("reject your own pull is not allowed")) + return -1, true + } + reviewType = models.ReviewTypeReject + + case api.ReviewStateComment: + reviewType = models.ReviewTypeComment + default: + reviewType = models.ReviewTypePending + } + + // reject reviews with empty body if not approve type + if reviewType != models.ReviewTypeApprove && len(strings.TrimSpace(body)) == 0 { + ctx.JSON(http.StatusUnprocessableEntity, fmt.Errorf("review event %s need body", event)) + return -1, true + } + + return reviewType, false +} diff --git a/routers/api/v1/swagger/options.go b/routers/api/v1/swagger/options.go index 4bb649616a01a..f13dc63864366 100644 --- a/routers/api/v1/swagger/options.go +++ b/routers/api/v1/swagger/options.go @@ -137,4 +137,13 @@ type swaggerParameterBodies struct { // in:body CreateOAuth2ApplicationOptions api.CreateOAuth2ApplicationOptions + + // in:body + CreatePullReviewOptions api.CreatePullReviewOptions + + // in:body + CreatePullReviewComment api.CreatePullReviewComment + + // in:body + SubmitPullReviewOptions api.SubmitPullReviewOptions } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index a015194cc2cec..297cd612a23ff 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -6710,7 +6710,7 @@ "tags": [ "repository" ], - "summary": "List all reviews for a pull request.", + "summary": "List all reviews for a pull request", "operationId": "repoListPullReviews", "parameters": [ { @@ -6756,6 +6756,59 @@ "$ref": "#/responses/notFound" } } + }, + "post": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Create a review to an pull request", + "operationId": "repoCreatePullReview", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "index of the pull request", + "name": "index", + "in": "path", + "required": true + }, + { + "name": "body", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/CreatePullRequestOption" + } + } + ], + "responses": { + "200": { + "$ref": "#/responses/PullReview" + }, + "404": { + "$ref": "#/responses/notFound" + }, + "422": { + "$ref": "#/responses/validationError" + } + } } }, "/repos/{owner}/{repo}/pulls/{index}/reviews/{id}": { @@ -6766,7 +6819,7 @@ "tags": [ "repository" ], - "summary": "Get a specific review for a pull request.", + "summary": "Get a specific review for a pull request", "operationId": "repoGetPullReview", "parameters": [ { @@ -6809,6 +6862,67 @@ } } }, + "post": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Submit a pending review to an pull request", + "operationId": "repoSubmitPullReview", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "index of the pull request", + "name": "index", + "in": "path", + "required": true + }, + { + "type": "integer", + "format": "int64", + "description": "id of the review", + "name": "id", + "in": "path", + "required": true + }, + { + "name": "body", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/SubmitPullReviewOptions" + } + } + ], + "responses": { + "200": { + "$ref": "#/responses/PullReview" + }, + "404": { + "$ref": "#/responses/notFound" + }, + "422": { + "$ref": "#/responses/validationError" + } + } + }, "delete": { "produces": [ "application/json" @@ -6871,7 +6985,7 @@ "tags": [ "repository" ], - "summary": "Get a specific review for a pull request.", + "summary": "Get a specific review for a pull request", "operationId": "repoGetPullReviewComments", "parameters": [ { @@ -11176,6 +11290,59 @@ }, "x-go-package": "code.gitea.io/gitea/modules/structs" }, + "CreatePullReviewComment": { + "description": "CreatePullReviewComment represent a review comment for creation api", + "type": "object", + "properties": { + "body": { + "type": "string", + "x-go-name": "Body" + }, + "new_position": { + "description": "if comment to new file line or 0", + "type": "integer", + "format": "int64", + "x-go-name": "NewLineNum" + }, + "old_position": { + "description": "if comment to old file line or 0", + "type": "integer", + "format": "int64", + "x-go-name": "OldLineNum" + }, + "path": { + "description": "the tree path", + "type": "string", + "x-go-name": "Path" + } + }, + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, + "CreatePullReviewOptions": { + "description": "CreatePullReviewOptions are options to create a pull review", + "type": "object", + "properties": { + "body": { + "type": "string", + "x-go-name": "Body" + }, + "comments": { + "type": "array", + "items": { + "$ref": "#/definitions/CreatePullReviewComment" + }, + "x-go-name": "Comments" + }, + "commit_id": { + "type": "string", + "x-go-name": "CommitID" + }, + "event": { + "$ref": "#/definitions/ReviewStateType" + } + }, + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, "CreateReleaseOption": { "description": "CreateReleaseOption options when creating a release", "type": "object", @@ -13388,7 +13555,7 @@ "x-go-package": "code.gitea.io/gitea/modules/structs" }, "PullReviewComment": { - "description": "PullReviewComment represents a comment on a pull request", + "description": "PullReviewComment represents a comment on a pull request review", "type": "object", "properties": { "body": { @@ -13903,6 +14070,20 @@ }, "x-go-package": "code.gitea.io/gitea/modules/structs" }, + "SubmitPullReviewOptions": { + "description": "SubmitPullReviewOptions are options to submit a pending pull review", + "type": "object", + "properties": { + "body": { + "type": "string", + "x-go-name": "Body" + }, + "event": { + "$ref": "#/definitions/ReviewStateType" + } + }, + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, "Tag": { "description": "Tag represents a repository tag", "type": "object", @@ -14908,7 +15089,7 @@ "parameterBodies": { "description": "parameterBodies", "schema": { - "$ref": "#/definitions/CreateOAuth2ApplicationOptions" + "$ref": "#/definitions/SubmitPullReviewOptions" } }, "redirect": { From 2f2c400138635788014f3e241a098fd6e9480fe9 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 27 Apr 2020 22:12:06 +0200 Subject: [PATCH 10/26] fix lint --- routers/api/v1/repo/pull_review.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 7a59be50efa72..d89fefe4537d3 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -381,8 +381,6 @@ func CreatePullReview(ctx *context.APIContext, opts api.CreatePullReviewOptions) return } ctx.JSON(http.StatusOK, apiReview) - - return } // SubmitPullReview submit a pending review to an pull request @@ -474,7 +472,6 @@ func SubmitPullReview(ctx *context.APIContext, opts api.SubmitPullReviewOptions) return } ctx.JSON(http.StatusOK, apiReview) - return } func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, event api.ReviewStateType, body string) (reviewType models.ReviewType, isWrong bool) { From aeda695061f0be2598a0141162872c940127cabb Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 28 Apr 2020 00:50:48 +0200 Subject: [PATCH 11/26] fix lint --- modules/structs/pull_review.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index 5b409c5154496..a60b0dc051857 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -20,7 +20,7 @@ const ( ReviewStateComment ReviewStateType = "COMMENT" // ReviewStateRequestChanges changes for pr are requested ReviewStateRequestChanges ReviewStateType = "REQUEST_CHANGES" - // ReviewStateUnknown state of pr is unknown + // ReviewStateRequestReview review is requested from user ReviewStateRequestReview ReviewStateType = "REQUEST_REVIEW" // ReviewStateUnknown state of pr is unknown ReviewStateUnknown ReviewStateType = "" From ef2047d4641c781059cc6e277f25a5c9167cbf79 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 28 Apr 2020 18:52:09 +0200 Subject: [PATCH 12/26] impruve test --- models/review_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/models/review_test.go b/models/review_test.go index 45ddb3181d2b9..fb3a5488e5397 100644 --- a/models/review_test.go +++ b/models/review_test.go @@ -131,9 +131,11 @@ func TestGetReviewersByIssueID(t *testing.T) { allReviews, err := GetReviewersByIssueID(issue.ID) assert.NoError(t, err) - for i, review := range allReviews { - assert.Equal(t, expectedReviews[i].Reviewer, review.Reviewer) - assert.Equal(t, expectedReviews[i].Type, review.Type) - assert.Equal(t, expectedReviews[i].UpdatedUnix, review.UpdatedUnix) + if assert.Len(t, allReviews, 3) { + for i, review := range allReviews { + assert.Equal(t, expectedReviews[i].Reviewer, review.Reviewer) + assert.Equal(t, expectedReviews[i].Type, review.Type) + assert.Equal(t, expectedReviews[i].UpdatedUnix, review.UpdatedUnix) + } } } From 42a6b8916dae1e40d828d6043c53e87d6794e572 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 28 Apr 2020 18:55:10 +0200 Subject: [PATCH 13/26] DONT use GhostUser for loadReviewer --- models/review.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/models/review.go b/models/review.go index 930481e21075c..a35abc37abf83 100644 --- a/models/review.go +++ b/models/review.go @@ -101,10 +101,6 @@ func (r *Review) loadReviewer(e Engine) (err error) { return nil } r.Reviewer, err = getUserByID(e, r.ReviewerID) - if err != nil && IsErrUserNotExist(err) { - r.Reviewer = NewGhostUser() - err = nil - } } return } From 5a6f383db230bd097ce08ac2d977aaf3ddaccbb7 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 28 Apr 2020 19:23:32 +0200 Subject: [PATCH 14/26] expose comments_count of a PullReview --- modules/convert/pull_review.go | 19 ++++++++++--------- modules/structs/pull_review.go | 15 ++++++++------- templates/swagger/v1_json.tmpl | 5 +++++ 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go index 817059ecbfd4c..33a701ce08ce1 100644 --- a/modules/convert/pull_review.go +++ b/modules/convert/pull_review.go @@ -24,15 +24,16 @@ func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) } result := &api.PullReview{ - ID: r.ID, - Reviewer: ToUser(r.Reviewer, doer != nil, auth), - State: api.ReviewStateUnknown, - Body: r.Content, - CommitID: r.CommitID, - Stale: r.Stale, - Official: r.Official, - Submitted: r.CreatedUnix.AsTime(), - HTMLPullURL: r.Issue.HTMLURL(), + ID: r.ID, + Reviewer: ToUser(r.Reviewer, doer != nil, auth), + State: api.ReviewStateUnknown, + Body: r.Content, + CommitID: r.CommitID, + Stale: r.Stale, + Official: r.Official, + CodeCommentsCount: len(r.CodeComments), + Submitted: r.CreatedUnix.AsTime(), + HTMLPullURL: r.Issue.HTMLURL(), } switch r.Type { diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index a60b0dc051857..355a267d71dd2 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -28,13 +28,14 @@ const ( // PullReview represents a pull request review type PullReview struct { - ID int64 `json:"id"` - Reviewer *User `json:"user"` - State ReviewStateType `json:"state"` - Body string `json:"body"` - CommitID string `json:"commit_id"` - Stale bool `json:"stale"` - Official bool `json:"official"` + ID int64 `json:"id"` + Reviewer *User `json:"user"` + State ReviewStateType `json:"state"` + Body string `json:"body"` + CommitID string `json:"commit_id"` + Stale bool `json:"stale"` + Official bool `json:"official"` + CodeCommentsCount int `json:"comments_count"` // swagger:strfmt date-time Submitted time.Time `json:"submitted_at"` diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 297cd612a23ff..b640834c0f9bb 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -13519,6 +13519,11 @@ "type": "string", "x-go-name": "Body" }, + "comments_count": { + "type": "integer", + "format": "int64", + "x-go-name": "CodeCommentsCount" + }, "commit_id": { "type": "string", "x-go-name": "CommitID" From eedd1f235c01ae2f301ba0912bdcb6adc9114d91 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 28 Apr 2020 19:44:29 +0200 Subject: [PATCH 15/26] infent GetCodeCommentsCount() --- models/review.go | 20 ++++++++++++++++++++ modules/convert/pull_review.go | 2 +- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/models/review.go b/models/review.go index a35abc37abf83..4fc2c6cca81c5 100644 --- a/models/review.go +++ b/models/review.go @@ -83,6 +83,26 @@ func (r *Review) loadCodeComments(e Engine) (err error) { return } +// GetCodeCommentsCount return count of CodeComments a Review has +func (r *Review) GetCodeCommentsCount() int { + //Find comments + opts := FindCommentsOptions{ + Type: CommentTypeCode, + IssueID: r.IssueID, + ReviewID: r.ID, + } + conds := opts.toConds() + if r.ID == 0 { + conds = conds.And(builder.Eq{"invalidated": false}) + } + + count, err := x.Where(conds).Count(new(Comment)) + if err != nil { + return 0 + } + return int(count) +} + // LoadCodeComments loads CodeComments func (r *Review) LoadCodeComments() error { return r.loadCodeComments(x) diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go index 33a701ce08ce1..6c09f7dd34003 100644 --- a/modules/convert/pull_review.go +++ b/modules/convert/pull_review.go @@ -31,7 +31,7 @@ func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) CommitID: r.CommitID, Stale: r.Stale, Official: r.Official, - CodeCommentsCount: len(r.CodeComments), + CodeCommentsCount: r.GetCodeCommentsCount(), Submitted: r.CreatedUnix.AsTime(), HTMLPullURL: r.Issue.HTMLURL(), } From ac86f43943d520b404be4302e4fd1e33d8a09f75 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 28 Apr 2020 21:24:38 +0200 Subject: [PATCH 16/26] fixes --- models/review.go | 14 +++++- routers/api/v1/api.go | 2 +- routers/api/v1/repo/pull_review.go | 74 +++++++++++++++--------------- templates/swagger/v1_json.tmpl | 2 +- 4 files changed, 51 insertions(+), 41 deletions(-) diff --git a/models/review.go b/models/review.go index 4fc2c6cca81c5..f115791d860b7 100644 --- a/models/review.go +++ b/models/review.go @@ -700,8 +700,8 @@ func DeleteReview(r *Review) error { return err } - if err := r.loadCodeComments(sess); err != nil { - return err + if r.ID == 0 { + return fmt.Errorf("review is not allowed to be 0") } opts := FindCommentsOptions{ @@ -714,6 +714,16 @@ func DeleteReview(r *Review) error { return err } + opts = FindCommentsOptions{ + Type: CommentTypeReview, + IssueID: r.IssueID, + ReviewID: r.ID, + } + + if _, err := sess.Where(opts.toConds()).Delete(new(Comment)); err != nil { + return err + } + if _, err := sess.ID(r.ID).Delete(new(Review)); err != nil { return err } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 333f47c385ea3..754e146fc1eae 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -797,7 +797,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/reviews", func() { m.Combo(""). Get(repo.ListPullReviews). - Post(reqToken(), bind(api.CreatePullRequestOption{}), repo.CreatePullReview) + Post(reqToken(), bind(api.CreatePullReviewOptions{}), repo.CreatePullReview) m.Group("/:id", func() { m.Combo(""). Get(repo.GetPullReview). diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index d89fefe4537d3..dc214f6fb489c 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -255,42 +255,6 @@ func DeletePullReview(ctx *context.APIContext) { ctx.Status(http.StatusNoContent) } -func prepareSingleReview(ctx *context.APIContext) (r *models.Review, statusSet bool) { - pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) - if err != nil { - if models.IsErrPullRequestNotExist(err) { - ctx.NotFound("GetPullRequestByIndex", err) - } else { - ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) - } - return nil, true - } - - review, err := models.GetReviewByID(ctx.ParamsInt64(":id")) - if err != nil { - if models.IsErrReviewNotExist(err) { - ctx.NotFound("GetReviewByID", err) - } else { - ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) - } - return nil, true - } - - // validate the the review is for the given PR - if review.IssueID != pr.IssueID { - ctx.NotFound("ReviewNotInPR", err) - return nil, true - } - - // make sure that the user has access to this review if it is pending - if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID { - ctx.NotFound("GetReviewByID", err) - return nil, true - } - - return review, false -} - // CreatePullReview create a review to an pull request func CreatePullReview(ctx *context.APIContext, opts api.CreatePullReviewOptions) { // swagger:operation POST /repos/{owner}/{repo}/pulls/{index}/reviews repository repoCreatePullReview @@ -319,7 +283,7 @@ func CreatePullReview(ctx *context.APIContext, opts api.CreatePullReviewOptions) // in: body // required: true // schema: - // "$ref": "#/definitions/CreatePullRequestOption" + // "$ref": "#/definitions/CreatePullReviewOptions" // responses: // "200": // "$ref": "#/responses/PullReview" @@ -511,3 +475,39 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even return reviewType, false } + +func prepareSingleReview(ctx *context.APIContext) (r *models.Review, statusSet bool) { + pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) + if err != nil { + if models.IsErrPullRequestNotExist(err) { + ctx.NotFound("GetPullRequestByIndex", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) + } + return nil, true + } + + review, err := models.GetReviewByID(ctx.ParamsInt64(":id")) + if err != nil { + if models.IsErrReviewNotExist(err) { + ctx.NotFound("GetReviewByID", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) + } + return nil, true + } + + // validate the the review is for the given PR + if review.IssueID != pr.IssueID { + ctx.NotFound("ReviewNotInPR", err) + return nil, true + } + + // make sure that the user has access to this review if it is pending + if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID { + ctx.NotFound("GetReviewByID", err) + return nil, true + } + + return review, false +} diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index b640834c0f9bb..63cb395ba71ee 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -6794,7 +6794,7 @@ "in": "body", "required": true, "schema": { - "$ref": "#/definitions/CreatePullRequestOption" + "$ref": "#/definitions/CreatePullReviewOptions" } } ], From 75f6c1bfff9abe588a7d6003a83b8ab5651be0be Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 28 Apr 2020 21:54:30 +0200 Subject: [PATCH 17/26] fix+impruve --- routers/api/v1/repo/pull_review.go | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index dc214f6fb489c..dd90f9e2aecd7 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -308,6 +308,11 @@ func CreatePullReview(ctx *context.APIContext, opts api.CreatePullReviewOptions) return } + if err := pr.Issue.LoadRepo(); err != nil { + ctx.Error(http.StatusInternalServerError, "pr.Issue.LoadRepo", err) + return + } + // create review comments for _, c := range opts.Comments { line := c.NewLineNum @@ -396,7 +401,7 @@ func SubmitPullReview(ctx *context.APIContext, opts api.SubmitPullReviewOptions) } if review.Type != models.ReviewTypePending { - ctx.JSON(http.StatusUnprocessableEntity, fmt.Errorf("only a pending review can be submitted")) + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("only a pending review can be submitted")) return } @@ -416,9 +421,15 @@ func SubmitPullReview(ctx *context.APIContext, opts api.SubmitPullReviewOptions) return } + // if review stay pending return + if reviewType == models.ReviewTypePending { + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("review stay pending")) + return + } + headCommitID, err := ctx.Repo.GitRepo.GetRefCommitID(pr.GetGitRefName()) if err != nil { - ctx.InternalServerError(err) + ctx.Error(http.StatusInternalServerError, "GitRepo: GetRefCommitID", err) return } @@ -440,7 +451,7 @@ func SubmitPullReview(ctx *context.APIContext, opts api.SubmitPullReviewOptions) func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, event api.ReviewStateType, body string) (reviewType models.ReviewType, isWrong bool) { if err := pr.LoadIssue(); err != nil { - ctx.InternalServerError(err) + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) return -1, true } @@ -448,7 +459,7 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even case api.ReviewStateApproved: // can not approve your own PR if pr.Issue.IsPoster(ctx.User.ID) { - ctx.JSON(http.StatusUnprocessableEntity, fmt.Errorf("approve your own pull is not allowed")) + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("approve your own pull is not allowed")) return -1, true } reviewType = models.ReviewTypeApprove @@ -456,7 +467,7 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even case api.ReviewStateRequestChanges: // can not reject your own PR if pr.Issue.IsPoster(ctx.User.ID) { - ctx.JSON(http.StatusUnprocessableEntity, fmt.Errorf("reject your own pull is not allowed")) + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("reject your own pull is not allowed")) return -1, true } reviewType = models.ReviewTypeReject @@ -469,7 +480,7 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even // reject reviews with empty body if not approve type if reviewType != models.ReviewTypeApprove && len(strings.TrimSpace(body)) == 0 { - ctx.JSON(http.StatusUnprocessableEntity, fmt.Errorf("review event %s need body", event)) + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("review event %s need body", event)) return -1, true } From 627b9d65f632f571c0cc2148a21986cc817ca6f2 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 29 Apr 2020 16:49:05 +0200 Subject: [PATCH 18/26] some nits --- models/review.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/models/review.go b/models/review.go index f115791d860b7..1a8a7940033db 100644 --- a/models/review.go +++ b/models/review.go @@ -85,7 +85,6 @@ func (r *Review) loadCodeComments(e Engine) (err error) { // GetCodeCommentsCount return count of CodeComments a Review has func (r *Review) GetCodeCommentsCount() int { - //Find comments opts := FindCommentsOptions{ Type: CommentTypeCode, IssueID: r.IssueID, @@ -116,12 +115,10 @@ func (r *Review) loadIssue(e Engine) (err error) { } func (r *Review) loadReviewer(e Engine) (err error) { - if r.Reviewer == nil { - if r.ReviewerID == 0 { - return nil - } - r.Reviewer, err = getUserByID(e, r.ReviewerID) + if r.Reviewer != nil || r.ReviewerID == 0 { + return nil } + r.Reviewer, err = getUserByID(e, r.ReviewerID) return } From 0cf99dca6ea075fbb371474b36f5ef621f91e07f Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 29 Apr 2020 18:55:04 +0200 Subject: [PATCH 19/26] Handle Ghosts :ghost: --- models/fixtures/review.yml | 5 +++- models/review.go | 6 ++--- modules/convert/pull_review.go | 11 +++++--- routers/api/v1/repo/pull_review.go | 43 ++++++++++++++---------------- 4 files changed, 35 insertions(+), 30 deletions(-) diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index 87621012fff07..35d3dee2e6b06 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -60,6 +60,8 @@ reviewer_id: 4 issue_id: 3 content: "New review 5" + commit_id: 8091a55037cd59e47293aca02981b5a67076b364 + stale: true updated_unix: 946684813 created_unix: 946684813 - @@ -77,5 +79,6 @@ reviewer_id: 100 issue_id: 3 content: "a deleted user's review" + official: true updated_unix: 946684815 - created_unix: 946684815 \ No newline at end of file + created_unix: 946684815 diff --git a/models/review.go b/models/review.go index 1a8a7940033db..35eb5d4b60c8c 100644 --- a/models/review.go +++ b/models/review.go @@ -128,15 +128,15 @@ func (r *Review) LoadReviewer() error { } func (r *Review) loadAttributes(e Engine) (err error) { - if err = r.loadReviewer(e); err != nil { - return - } if err = r.loadIssue(e); err != nil { return } if err = r.loadCodeComments(e); err != nil { return } + if err = r.loadReviewer(e); err != nil { + return + } return } diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go index 6c09f7dd34003..d27d68f541a1d 100644 --- a/modules/convert/pull_review.go +++ b/modules/convert/pull_review.go @@ -13,9 +13,11 @@ import ( // ToPullReview convert a review to api format func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) { - if err := r.LoadAttributes(); err != nil { - return nil, err + if !models.IsErrUserNotExist(err) { + return nil, err + } + r.Reviewer = models.NewGhostUser() } auth := false @@ -72,7 +74,10 @@ func ToPullReviewList(rl []*models.Review, doer *models.User) ([]*api.PullReview // ToPullReviewCommentList convert the CodeComments of an review to it's api format func ToPullReviewCommentList(review *models.Review, doer *models.User) ([]*api.PullReviewComment, error) { if err := review.LoadAttributes(); err != nil { - return nil, err + if !models.IsErrUserNotExist(err) { + return nil, err + } + review.Reviewer = models.NewGhostUser() } apiComments := make([]*api.PullReviewComment, 0, len(review.CodeComments)) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index dd90f9e2aecd7..50dd772d3ea4a 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -131,7 +131,7 @@ func GetPullReview(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - review, statusSet := prepareSingleReview(ctx) + review, _, statusSet := prepareSingleReview(ctx) if statusSet { return } @@ -181,7 +181,7 @@ func GetPullReviewComments(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - review, statusSet := prepareSingleReview(ctx) + review, _, statusSet := prepareSingleReview(ctx) if statusSet { return } @@ -233,7 +233,7 @@ func DeletePullReview(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - review, statusSet := prepareSingleReview(ctx) + review, _, statusSet := prepareSingleReview(ctx) if statusSet { return } @@ -395,7 +395,7 @@ func SubmitPullReview(ctx *context.APIContext, opts api.SubmitPullReviewOptions) // "422": // "$ref": "#/responses/validationError" - review, isWrong := prepareSingleReview(ctx) + review, pr, isWrong := prepareSingleReview(ctx) if isWrong { return } @@ -405,16 +405,6 @@ func SubmitPullReview(ctx *context.APIContext, opts api.SubmitPullReviewOptions) return } - pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) - if err != nil { - if models.IsErrPullRequestNotExist(err) { - ctx.NotFound("GetPullRequestByIndex", err) - } else { - ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) - } - return - } - // determine review type reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body) if isWrong { @@ -487,7 +477,7 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even return reviewType, false } -func prepareSingleReview(ctx *context.APIContext) (r *models.Review, statusSet bool) { +func prepareSingleReview(ctx *context.APIContext) (r *models.Review, pr *models.PullRequest, statusSet bool) { pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrPullRequestNotExist(err) { @@ -495,7 +485,7 @@ func prepareSingleReview(ctx *context.APIContext) (r *models.Review, statusSet b } else { ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) } - return nil, true + return nil, nil, true } review, err := models.GetReviewByID(ctx.ParamsInt64(":id")) @@ -505,20 +495,27 @@ func prepareSingleReview(ctx *context.APIContext) (r *models.Review, statusSet b } else { ctx.Error(http.StatusInternalServerError, "GetReviewByID", err) } - return nil, true + return nil, nil, true } // validate the the review is for the given PR if review.IssueID != pr.IssueID { - ctx.NotFound("ReviewNotInPR", err) - return nil, true + ctx.NotFound("ReviewNotInPR") + return nil, nil, true } // make sure that the user has access to this review if it is pending - if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID { - ctx.NotFound("GetReviewByID", err) - return nil, true + if review.Type == models.ReviewTypePending && review.ReviewerID != ctx.User.ID && !ctx.User.IsAdmin { + ctx.NotFound("GetReviewByID") + return nil, nil, true + } + + if err := review.LoadAttributes(); err != nil { + if !models.IsErrUserNotExist(err) { + ctx.Error(http.StatusInternalServerError, "ReviewLoadAttributes", err) + return nil, nil, true + } } - return review, false + return review, pr, false } From d9e1480f0ef2095826d65ad729e2c98c1d22538d Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 29 Apr 2020 18:55:32 +0200 Subject: [PATCH 20/26] add TEST for GET apis --- integrations/api_pull_review_test.go | 79 ++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 integrations/api_pull_review_test.go diff --git a/integrations/api_pull_review_test.go b/integrations/api_pull_review_test.go new file mode 100644 index 0000000000000..02ec71b5d2553 --- /dev/null +++ b/integrations/api_pull_review_test.go @@ -0,0 +1,79 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package integrations + +import ( + "net/http" + "testing" + + "code.gitea.io/gitea/models" + api "code.gitea.io/gitea/modules/structs" + "github.com/stretchr/testify/assert" +) + +func TestAPIPullReview(t *testing.T) { + defer prepareTestEnv(t)() + pullIssue := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 3}).(*models.Issue) + assert.NoError(t, pullIssue.LoadAttributes()) + repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: pullIssue.RepoID}).(*models.Repository) + + // test ListPullReviews + session := loginUser(t, "user2") + token := getTokenForLoggedInUser(t, session) + req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/%d/reviews?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token) + resp := session.MakeRequest(t, req, http.StatusOK) + + var reviews []*api.PullReview + DecodeJSON(t, resp, &reviews) + if !assert.Len(t, reviews, 6) { + return + } + for _, r := range reviews { + assert.EqualValues(t, pullIssue.HTMLURL(), r.HTMLPullURL) + } + assert.EqualValues(t, 8, reviews[3].ID) + assert.EqualValues(t, "APPROVED", reviews[3].State) + assert.EqualValues(t, 0, reviews[3].CodeCommentsCount) + assert.EqualValues(t, true, reviews[3].Stale) + assert.EqualValues(t, false, reviews[3].Official) + + assert.EqualValues(t, 10, reviews[5].ID) + assert.EqualValues(t, "REQUEST_CHANGES", reviews[5].State) + assert.EqualValues(t, 1, reviews[5].CodeCommentsCount) + assert.EqualValues(t, 0, reviews[5].Reviewer.ID) // ghost user + assert.EqualValues(t, false, reviews[5].Stale) + assert.EqualValues(t, true, reviews[5].Official) + + // test GetPullReview + req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/%d/reviews/%d?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, reviews[3].ID, token) + resp = session.MakeRequest(t, req, http.StatusOK) + var review api.PullReview + DecodeJSON(t, resp, &review) + assert.EqualValues(t, *reviews[3], review) + + req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/%d/reviews/%d?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, reviews[5].ID, token) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &review) + assert.EqualValues(t, *reviews[5], review) + + // test GetPullReviewComments + comment := models.AssertExistsAndLoadBean(t, &models.Comment{ID: 7}).(*models.Comment) + req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/%d/reviews/%d/comments?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, 10, token) + resp = session.MakeRequest(t, req, http.StatusOK) + var reviewComments []*api.PullReviewComment + DecodeJSON(t, resp, &reviewComments) + assert.Len(t, reviewComments, 1) + assert.EqualValues(t, "Ghost", reviewComments[0].Reviewer.UserName) + assert.EqualValues(t, "a review from a deleted user", reviewComments[0].Body) + assert.EqualValues(t, comment.ID, reviewComments[0].ID) + assert.EqualValues(t, comment.UpdatedUnix, reviewComments[0].Updated.Unix()) + assert.EqualValues(t, comment.HTMLURL(), reviewComments[0].HTMLURL) + + // test CreatePullReview + + // test SubmitPullReview + + // test DeletePullReview +} From cbca510258c3390acae89ecf5dd62bda03fc1e2a Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 29 Apr 2020 20:22:08 +0200 Subject: [PATCH 21/26] complete TESTS --- integrations/api_pull_review_test.go | 47 ++++++++++++++++++++++++++-- models/fixtures/pull_request.yml | 4 +-- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/integrations/api_pull_review_test.go b/integrations/api_pull_review_test.go index 02ec71b5d2553..c90a5c11cd129 100644 --- a/integrations/api_pull_review_test.go +++ b/integrations/api_pull_review_test.go @@ -5,6 +5,7 @@ package integrations import ( + "fmt" "net/http" "testing" @@ -22,7 +23,7 @@ func TestAPIPullReview(t *testing.T) { // test ListPullReviews session := loginUser(t, "user2") token := getTokenForLoggedInUser(t, session) - req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/%d/reviews?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token) + req := NewRequestf(t, http.MethodGet, "/api/v1/repos/%s/%s/pulls/%d/reviews?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token) resp := session.MakeRequest(t, req, http.StatusOK) var reviews []*api.PullReview @@ -47,7 +48,7 @@ func TestAPIPullReview(t *testing.T) { assert.EqualValues(t, true, reviews[5].Official) // test GetPullReview - req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/%d/reviews/%d?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, reviews[3].ID, token) + req = NewRequestf(t, http.MethodGet, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, reviews[3].ID, token) resp = session.MakeRequest(t, req, http.StatusOK) var review api.PullReview DecodeJSON(t, resp, &review) @@ -60,7 +61,7 @@ func TestAPIPullReview(t *testing.T) { // test GetPullReviewComments comment := models.AssertExistsAndLoadBean(t, &models.Comment{ID: 7}).(*models.Comment) - req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/pulls/%d/reviews/%d/comments?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, 10, token) + req = NewRequestf(t, http.MethodGet, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d/comments?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, 10, token) resp = session.MakeRequest(t, req, http.StatusOK) var reviewComments []*api.PullReviewComment DecodeJSON(t, resp, &reviewComments) @@ -72,8 +73,48 @@ func TestAPIPullReview(t *testing.T) { assert.EqualValues(t, comment.HTMLURL(), reviewComments[0].HTMLURL) // test CreatePullReview + 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: "body1", + // Event: "" # will result in PENDING + 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, + }, + }, + }) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &review) + assert.EqualValues(t, 6, review.ID) + assert.EqualValues(t, "PENDING", review.State) + assert.EqualValues(t, 2, review.CodeCommentsCount) // test SubmitPullReview + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/reviews/%d?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, review.ID, token), &api.SubmitPullReviewOptions{ + Event: "APPROVED", + Body: "just two nits", + }) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &review) + assert.EqualValues(t, 6, review.ID) + assert.EqualValues(t, "APPROVED", review.State) + assert.EqualValues(t, 2, review.CodeCommentsCount) // test DeletePullReview + 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: "just a comment", + Event: "COMMENT", + }) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &review) + assert.EqualValues(t, "COMMENT", review.State) + assert.EqualValues(t, 0, review.CodeCommentsCount) + 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) } diff --git a/models/fixtures/pull_request.yml b/models/fixtures/pull_request.yml index da9566bc481eb..b555da83ef762 100644 --- a/models/fixtures/pull_request.yml +++ b/models/fixtures/pull_request.yml @@ -8,7 +8,7 @@ base_repo_id: 1 head_branch: branch1 base_branch: master - merge_base: 1234567890abcdef + merge_base: 4a357436d925b5c974181ff12a994538ddc5a269 has_merged: true merger_id: 2 @@ -22,7 +22,7 @@ base_repo_id: 1 head_branch: branch2 base_branch: master - merge_base: fedcba9876543210 + merge_base: 4a357436d925b5c974181ff12a994538ddc5a269 has_merged: false - From 9b0bbc1ffa23dd20950767134feb928f95ace85d Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 30 Apr 2020 01:03:14 +0200 Subject: [PATCH 22/26] add HTMLURL to PullReview responce --- models/review.go | 53 ++++++++++++++++++++++------------ modules/convert/pull_review.go | 1 + modules/structs/pull_review.go | 3 +- 3 files changed, 36 insertions(+), 21 deletions(-) diff --git a/models/review.go b/models/review.go index 35eb5d4b60c8c..7229d0e2973fe 100644 --- a/models/review.go +++ b/models/review.go @@ -83,25 +83,6 @@ func (r *Review) loadCodeComments(e Engine) (err error) { return } -// GetCodeCommentsCount return count of CodeComments a Review has -func (r *Review) GetCodeCommentsCount() int { - opts := FindCommentsOptions{ - Type: CommentTypeCode, - IssueID: r.IssueID, - ReviewID: r.ID, - } - conds := opts.toConds() - if r.ID == 0 { - conds = conds.And(builder.Eq{"invalidated": false}) - } - - count, err := x.Where(conds).Count(new(Comment)) - if err != nil { - return 0 - } - return int(count) -} - // LoadCodeComments loads CodeComments func (r *Review) LoadCodeComments() error { return r.loadCodeComments(x) @@ -727,3 +708,37 @@ func DeleteReview(r *Review) error { return sess.Commit() } + +// GetCodeCommentsCount return count of CodeComments a Review has +func (r *Review) GetCodeCommentsCount() int { + opts := FindCommentsOptions{ + Type: CommentTypeCode, + IssueID: r.IssueID, + ReviewID: r.ID, + } + conds := opts.toConds() + if r.ID == 0 { + conds = conds.And(builder.Eq{"invalidated": false}) + } + + count, err := x.Where(conds).Count(new(Comment)) + if err != nil { + return 0 + } + return int(count) +} + +// HTMLURL formats a URL-string to the related review issue-comment +func (r *Review) HTMLURL() string { + opts := FindCommentsOptions{ + Type: CommentTypeReview, + IssueID: r.IssueID, + ReviewID: r.ID, + } + comment := new(Comment) + has, err := x.Where(opts.toConds()).Get(comment) + if err != nil || !has { + return "" + } + return comment.HTMLURL() +} diff --git a/modules/convert/pull_review.go b/modules/convert/pull_review.go index d27d68f541a1d..619f9f070e0a2 100644 --- a/modules/convert/pull_review.go +++ b/modules/convert/pull_review.go @@ -35,6 +35,7 @@ func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error) Official: r.Official, CodeCommentsCount: r.GetCodeCommentsCount(), Submitted: r.CreatedUnix.AsTime(), + HTMLURL: r.HTMLURL(), HTMLPullURL: r.Issue.HTMLURL(), } diff --git a/modules/structs/pull_review.go b/modules/structs/pull_review.go index 355a267d71dd2..bf9eafc243640 100644 --- a/modules/structs/pull_review.go +++ b/modules/structs/pull_review.go @@ -39,8 +39,7 @@ type PullReview struct { // swagger:strfmt date-time Submitted time.Time `json:"submitted_at"` - // HTMLURL string `json:"html_url"` // ToDo: blocked by https://github.com/go-gitea/gitea/issues/11226 - + HTMLURL string `json:"html_url"` HTMLPullURL string `json:"pull_request_url"` } From ac9a31fa0858076824531a06c02a5cb10cb6c0d2 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 30 Apr 2020 01:06:38 +0200 Subject: [PATCH 23/26] code format as per @lafriks --- models/review.go | 14 ++++++++------ routers/api/v1/repo/pull_review.go | 13 ++++++------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/models/review.go b/models/review.go index 7229d0e2973fe..ebc172e829980 100644 --- a/models/review.go +++ b/models/review.go @@ -74,12 +74,13 @@ type Review struct { } func (r *Review) loadCodeComments(e Engine) (err error) { - if err = r.loadIssue(e); err != nil { - return err + if r.CodeComments != nil { + return } - if r.CodeComments == nil { - r.CodeComments, err = fetchCodeCommentsByReview(e, r.Issue, nil, r) + if err = r.loadIssue(e); err != nil { + return } + r.CodeComments, err = fetchCodeCommentsByReview(e, r.Issue, nil, r) return } @@ -89,9 +90,10 @@ func (r *Review) LoadCodeComments() error { } func (r *Review) loadIssue(e Engine) (err error) { - if r.Issue == nil { - r.Issue, err = getIssueByID(e, r.IssueID) + if r.Issue != nil { + return } + r.Issue, err = getIssueByID(e, r.IssueID) return } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 50dd772d3ea4a..5c57aafeb3cd7 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -439,12 +439,13 @@ func SubmitPullReview(ctx *context.APIContext, opts api.SubmitPullReviewOptions) ctx.JSON(http.StatusOK, apiReview) } -func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, event api.ReviewStateType, body string) (reviewType models.ReviewType, isWrong bool) { +func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, event api.ReviewStateType, body string) (models.ReviewType, bool) { if err := pr.LoadIssue(); err != nil { ctx.Error(http.StatusInternalServerError, "LoadIssue", err) return -1, true } + var reviewType models.ReviewType switch event { case api.ReviewStateApproved: // can not approve your own PR @@ -477,7 +478,7 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even return reviewType, false } -func prepareSingleReview(ctx *context.APIContext) (r *models.Review, pr *models.PullRequest, statusSet bool) { +func prepareSingleReview(ctx *context.APIContext) (*models.Review, *models.PullRequest, bool) { pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrPullRequestNotExist(err) { @@ -510,11 +511,9 @@ func prepareSingleReview(ctx *context.APIContext) (r *models.Review, pr *models. return nil, nil, true } - if err := review.LoadAttributes(); err != nil { - if !models.IsErrUserNotExist(err) { - ctx.Error(http.StatusInternalServerError, "ReviewLoadAttributes", err) - return nil, nil, true - } + if err := review.LoadAttributes(); err != nil && !models.IsErrUserNotExist(err) { + ctx.Error(http.StatusInternalServerError, "ReviewLoadAttributes", err) + return nil, nil, true } return review, pr, false From 28d4f34a326acaae2f3b2fbb09b676df5fa68724 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 30 Apr 2020 01:22:10 +0200 Subject: [PATCH 24/26] update swagger definition --- templates/swagger/v1_json.tmpl | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 63cb395ba71ee..7c6bb21652db6 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -13528,6 +13528,10 @@ "type": "string", "x-go-name": "CommitID" }, + "html_url": { + "type": "string", + "x-go-name": "HTMLURL" + }, "id": { "type": "integer", "format": "int64", From d1ae269d796243c97becde4f1b1227fabe30e492 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 1 May 2020 16:33:49 +0200 Subject: [PATCH 25/26] Update routers/api/v1/repo/pull_review.go Co-authored-by: David Svantesson --- routers/api/v1/repo/pull_review.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 5c57aafeb3cd7..0f25bcfc68e77 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -71,7 +71,7 @@ func ListPullReviews(ctx *context.APIContext) { } if err = pr.Issue.LoadRepo(); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + ctx.Error(http.StatusInternalServerError, "LoadRepo", err) return } From e66b1398c3358ae701ec7014167a160947de416b Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 1 May 2020 16:43:28 +0200 Subject: [PATCH 26/26] add comments --- routers/api/v1/repo/pull_review.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 0f25bcfc68e77..b3772b00a9894 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -439,6 +439,7 @@ func SubmitPullReview(ctx *context.APIContext, opts api.SubmitPullReviewOptions) ctx.JSON(http.StatusOK, apiReview) } +// 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) { if err := pr.LoadIssue(); err != nil { ctx.Error(http.StatusInternalServerError, "LoadIssue", err) @@ -478,6 +479,7 @@ func preparePullReviewType(ctx *context.APIContext, pr *models.PullRequest, even return reviewType, false } +// prepareSingleReview return review, related pull and false or nil, nil and true if an error happen func prepareSingleReview(ctx *context.APIContext) (*models.Review, *models.PullRequest, bool) { pr, err := models.GetPullRequestByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil {