From 286a85e65cd0136ac4239ac64e61a69778931d0c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Nov 2024 13:10:41 -0800 Subject: [PATCH] Fix review request API --- routers/api/v1/repo/pull_review.go | 78 ++++++++++++++++++++++++++++-- routers/web/repo/pull_review.go | 8 ++- services/issue/assignee.go | 41 ++++++++++++---- services/pull/pull.go | 4 +- 4 files changed, 114 insertions(+), 17 deletions(-) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index def860eee8f..0dbb5827d3e 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -611,7 +611,79 @@ func CreateReviewRequests(ctx *context.APIContext) { // "$ref": "#/responses/notFound" opts := web.GetForm(ctx).(*api.PullReviewRequestOptions) - apiReviewRequest(ctx, *opts, true) + + pr, err := issues_model.GetPullRequestByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64(":index")) + if err != nil { + if issues_model.IsErrPullRequestNotExist(err) { + ctx.NotFound("GetPullRequestByIndex", err) + } else { + ctx.Error(http.StatusInternalServerError, "GetPullRequestByIndex", err) + } + return + } + + allowedUsers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, pr.Issue.PosterID) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetReviewers", err) + return + } + filteredUsers := make([]*user_model.User, 0, len(opts.Reviewers)) + for _, reviewer := range opts.Reviewers { + for _, allowedUser := range allowedUsers { + if allowedUser.Name == reviewer || allowedUser.Email == reviewer { + filteredUsers = append(filteredUsers, allowedUser) + break + } + } + } + + filteredTeams := make([]*organization.Team, 0, len(opts.TeamReviewers)) + if ctx.Repo.Repository.Owner.IsOrganization() && len(opts.TeamReviewers) > 0 { + allowedTeams, err := pull_service.GetReviewerTeams(ctx, ctx.Repo.Repository) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetReviewers", err) + return + } + for _, teamReviewer := range opts.TeamReviewers { + for _, allowedTeam := range allowedTeams { + if allowedTeam.Name == teamReviewer { + filteredTeams = append(filteredTeams, allowedTeam) + break + } + } + } + } + comments, err := issue_service.ReviewRequests(ctx, pr, ctx.Doer, filteredUsers, filteredTeams) + if err != nil { + if issues_model.IsErrReviewRequestOnClosedPR(err) { + ctx.Error(http.StatusForbidden, "", err) + return + } + if issues_model.IsErrNotValidReviewRequest(err) { + ctx.Error(http.StatusUnprocessableEntity, "", err) + return + } + ctx.Error(http.StatusInternalServerError, "ReviewRequests", err) + return + } + + reviews := make([]*issues_model.Review, 0, len(filteredUsers)) + for _, comment := range comments { + if comment != nil { + if err = comment.LoadReview(ctx); err != nil { + ctx.ServerError("ReviewRequest", err) + return + } + reviews = append(reviews, comment.Review) + } + } + + apiReviews, err := convert.ToPullReviewList(ctx, reviews, ctx.Doer) + if err != nil { + ctx.Error(http.StatusInternalServerError, "convertToPullReviewList", err) + return + } + ctx.JSON(http.StatusCreated, apiReviews) } // DeleteReviewRequests delete review requests to an pull request @@ -730,7 +802,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions } for _, reviewer := range reviewers { - comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, &permDoer, reviewer, isAdd) + comment, err := issue_service.ReviewRequest(ctx, pr, ctx.Doer, &permDoer, reviewer, isAdd) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) @@ -755,7 +827,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions if ctx.Repo.Repository.Owner.IsOrganization() && len(opts.TeamReviewers) > 0 { for _, teamReviewer := range teamReviewers { - comment, err := issue_service.TeamReviewRequest(ctx, pr.Issue, ctx.Doer, teamReviewer, isAdd) + comment, err := issue_service.TeamReviewRequest(ctx, pr, ctx.Doer, teamReviewer, isAdd) if err != nil { if issues_model.IsErrReviewRequestOnClosedPR(err) { ctx.Error(http.StatusForbidden, "", err) diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index aa2e689e423..dcef506e190 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -365,6 +365,10 @@ func UpdatePullReviewRequest(ctx *context.Context) { ctx.Status(http.StatusForbidden) return } + if err := issue.LoadPullRequest(ctx); err != nil { + ctx.ServerError("issue.LoadPullRequest", err) + return + } if reviewID < 0 { // negative reviewIDs represent team requests if err := issue.Repo.LoadOwner(ctx); err != nil { @@ -395,7 +399,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach") + _, err = issue_service.TeamReviewRequest(ctx, issue.PullRequest, ctx.Doer, team, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( @@ -427,7 +431,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - _, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach") + _, err = issue_service.ReviewRequest(ctx, issue.PullRequest, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach") if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( diff --git a/services/issue/assignee.go b/services/issue/assignee.go index c7e24955687..f408cde92f7 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -8,6 +8,7 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/organization" + org_model "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" @@ -61,16 +62,16 @@ func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, do } // ReviewRequest add or remove a review request from a user for this PR, and make comment for it. -func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, permDoer *access_model.Permission, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { - err = isValidReviewRequest(ctx, reviewer, doer, isAdd, issue, permDoer) +func ReviewRequest(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, permDoer *access_model.Permission, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { + err = isValidReviewRequest(ctx, reviewer, doer, isAdd, pr.Issue, permDoer) if err != nil { return nil, err } if isAdd { - comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer) + comment, err = issues_model.AddReviewRequest(ctx, pr.Issue, reviewer, doer) } else { - comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer) + comment, err = issues_model.RemoveReviewRequest(ctx, pr.Issue, reviewer, doer) } if err != nil { @@ -78,12 +79,32 @@ func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_mo } if comment != nil { - notify_service.PullRequestReviewRequest(ctx, doer, issue, reviewer, isAdd, comment) + notify_service.PullRequestReviewRequest(ctx, doer, pr.Issue, reviewer, isAdd, comment) } return comment, err } +func ReviewRequests(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, reviewers []*user_model.User, reviewTeams []*org_model.Team) (comments []*issues_model.Comment, err error) { + for _, reviewer := range reviewers { + comment, err := ReviewRequest(ctx, pr, doer, nil, reviewer, true) + if err != nil { + return nil, err + } + comments = append(comments, comment) + } + + for _, reviewTeam := range reviewTeams { + comment, err := TeamReviewRequest(ctx, pr, doer, reviewTeam, true) + if err != nil { + return nil, err + } + comments = append(comments, comment) + } + + return comments, nil +} + // isValidReviewRequest Check permission for ReviewRequest func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue, permDoer *access_model.Permission) error { if reviewer.IsOrganization() { @@ -216,15 +237,15 @@ func isValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, } // TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it. -func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool) (comment *issues_model.Comment, err error) { - err = isValidTeamReviewRequest(ctx, reviewer, doer, isAdd, issue) +func TeamReviewRequest(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, reviewer *organization.Team, isAdd bool) (comment *issues_model.Comment, err error) { + err = isValidTeamReviewRequest(ctx, reviewer, doer, isAdd, pr.Issue) if err != nil { return nil, err } if isAdd { - comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer) + comment, err = issues_model.AddTeamReviewRequest(ctx, pr.Issue, reviewer, doer) } else { - comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer) + comment, err = issues_model.RemoveTeamReviewRequest(ctx, pr.Issue, reviewer, doer) } if err != nil { @@ -235,7 +256,7 @@ func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *use return nil, nil } - return comment, teamReviewRequestNotify(ctx, issue, doer, reviewer, isAdd, comment) + return comment, teamReviewRequestNotify(ctx, pr.Issue, doer, reviewer, isAdd, comment) } func ReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewNotifiers []*ReviewRequestNotifier) { diff --git a/services/pull/pull.go b/services/pull/pull.go index 3362cb97ff7..1180586ad06 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -212,12 +212,12 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster) for _, reviewer := range opts.Reviewers { - if _, err = issue_service.ReviewRequest(ctx, pr.Issue, issue.Poster, &permDoer, reviewer, true); err != nil { + if _, err = issue_service.ReviewRequest(ctx, pr, issue.Poster, &permDoer, reviewer, true); err != nil { return err } } for _, teamReviewer := range opts.TeamReviewers { - if _, err = issue_service.TeamReviewRequest(ctx, pr.Issue, issue.Poster, teamReviewer, true); err != nil { + if _, err = issue_service.TeamReviewRequest(ctx, pr, issue.Poster, teamReviewer, true); err != nil { return err } }