Fix review request API

models/issues/review.go

@@ -46,6 +46,7 @@ func (err ErrReviewNotExist) Unwrap() error {
 type ErrNotValidReviewRequest struct {
 	Reason string
 	UserID int64
+	TeamID int64
 	RepoID int64
 }
 
@@ -56,9 +57,10 @@ func IsErrNotValidReviewRequest(err error) bool {
 }
 
 func (err ErrNotValidReviewRequest) Error() string {
-	return fmt.Sprintf("%s [user_id: %d, repo_id: %d]",
+	return fmt.Sprintf("%s [user_id: %d, team_id: %d, repo_id: %d]",
 		err.Reason,
 		err.UserID,
+		err.TeamID,
 		err.RepoID)
 }
 
@@ -740,7 +742,7 @@ func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user
 		return nil, nil
 	}
 
-	if _, err = db.DeleteByBean(ctx, review); err != nil {
+	if _, err = db.DeleteByID[Review](ctx, review.ID); err != nil {
 		return nil, err
 	}
 

routers/api/v1/repo/collaborators.go

@@ -17,7 +17,6 @@ import (
 	"code.gitea.io/gitea/routers/api/v1/utils"
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/convert"
-	issue_service "code.gitea.io/gitea/services/issue"
 	pull_service "code.gitea.io/gitea/services/pull"
 	repo_service "code.gitea.io/gitea/services/repository"
 )
@@ -322,7 +321,7 @@ func GetReviewers(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
-	canChooseReviewer := issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0)
+	canChooseReviewer := pull_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0)
 	if !canChooseReviewer {
 		ctx.APIError(http.StatusForbidden, errors.New("doer has no permission to get reviewers"))
 		return

routers/api/v1/repo/pull.go

@@ -15,6 +15,7 @@ import (
 	activities_model "code.gitea.io/gitea/models/activities"
 	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/organization"
 	access_model "code.gitea.io/gitea/models/perm/access"
 	pull_model "code.gitea.io/gitea/models/pull"
 	repo_model "code.gitea.io/gitea/models/repo"
@@ -536,8 +537,16 @@ func CreatePullRequest(ctx *context.APIContext) {
 		PullRequest: pr,
 		AssigneeIDs: assigneeIDs,
 	}
-	prOpts.Reviewers, prOpts.TeamReviewers = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers)
-	if ctx.Written() {
+	prOpts.Reviewers, prOpts.TeamReviewers, err = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers)
+	switch {
+	case user_model.IsErrUserNotExist(err):
+		ctx.APIErrorNotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name))
+		return
+	case organization.IsErrTeamNotExist(err):
+		ctx.APIErrorNotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name))
+		return
+	case err != nil:
+		ctx.APIError(http.StatusInternalServerError, err)
 		return
 	}
 

routers/api/v1/repo/pull_review.go

@@ -19,7 +19,6 @@ import (
 	"code.gitea.io/gitea/routers/api/v1/utils"
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/convert"
-	issue_service "code.gitea.io/gitea/services/issue"
 	pull_service "code.gitea.io/gitea/services/pull"
 )
 
@@ -611,7 +610,94 @@ func CreateReviewRequests(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 
 	opts := web.GetForm(ctx).(*api.PullReviewRequestOptions)
-	apiReviewRequest(ctx, *opts, true)
+
+	// this will load issue
+	pr, err := issues_model.GetPullRequestByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index"))
+	if err != nil {
+		if issues_model.IsErrPullRequestNotExist(err) {
+			ctx.APIErrorNotFound("GetPullRequestByIndex", err)
+		} else {
+			ctx.APIError(http.StatusInternalServerError, err)
+		}
+		return
+	}
+
+	pr.Issue.Repo = ctx.Repo.Repository
+
+	allowedUsers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, pr.Issue.PosterID)
+	if err != nil {
+		ctx.APIError(http.StatusInternalServerError, err)
+		return
+	}
+	filteredUsers := make([]*user_model.User, 0, len(opts.Reviewers))
+	for _, reviewer := range opts.Reviewers {
+		found := false
+		for _, allowedUser := range allowedUsers {
+			if allowedUser.Name == reviewer || allowedUser.Email == reviewer {
+				filteredUsers = append(filteredUsers, allowedUser)
+				found = true
+				break
+			}
+		}
+		if !found {
+			ctx.APIError(http.StatusUnprocessableEntity, "")
+			return
+		}
+	}
+
+	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.APIError(http.StatusInternalServerError, err)
+			return
+		}
+		for _, teamReviewer := range opts.TeamReviewers {
+			found := false
+			for _, allowedTeam := range allowedTeams {
+				if allowedTeam.Name == teamReviewer {
+					filteredTeams = append(filteredTeams, allowedTeam)
+					found = true
+					break
+				}
+			}
+			if !found {
+				ctx.APIError(http.StatusUnprocessableEntity, "")
+				return
+			}
+		}
+	}
+	comments, err := pull_service.ReviewRequests(ctx, pr, ctx.Doer, filteredUsers, filteredTeams)
+	if err != nil {
+		if issues_model.IsErrReviewRequestOnClosedPR(err) {
+			ctx.APIError(http.StatusForbidden, err)
+			return
+		}
+		if issues_model.IsErrNotValidReviewRequest(err) {
+			ctx.APIError(http.StatusUnprocessableEntity, err)
+			return
+		}
+		ctx.APIError(http.StatusInternalServerError, 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.APIError(http.StatusInternalServerError, err)
+				return
+			}
+			reviews = append(reviews, comment.Review)
+		}
+	}
+
+	apiReviews, err := convert.ToPullReviewList(ctx, reviews, ctx.Doer)
+	if err != nil {
+		ctx.APIError(http.StatusInternalServerError, err)
+		return
+	}
+	ctx.JSON(http.StatusCreated, apiReviews)
 }
 
 // DeleteReviewRequests delete review requests to an pull request
@@ -653,51 +739,38 @@ func DeleteReviewRequests(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 	opts := web.GetForm(ctx).(*api.PullReviewRequestOptions)
-	apiReviewRequest(ctx, *opts, false)
+	deleteReviewRequests(ctx, *opts)
 }
 
-func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerNames []string) (reviewers []*user_model.User, teamReviewers []*organization.Team) {
-	var err error
+func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerNames []string) (reviewers []*user_model.User, teamReviewers []*organization.Team, err error) {
 	for _, r := range reviewerNames {
 		var reviewer *user_model.User
 		if strings.Contains(r, "@") {
 			reviewer, err = user_model.GetUserByEmail(ctx, r)
 		} else {
 			reviewer, err = user_model.GetUserByName(ctx, r)
 		}
-
 		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
-				ctx.APIErrorNotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", r))
-				return nil, nil
-			}
-			ctx.APIErrorInternal(err)
-			return nil, nil
+			return nil, nil, err
 		}
 
 		reviewers = append(reviewers, reviewer)
 	}
 
 	if ctx.Repo.Repository.Owner.IsOrganization() && len(teamReviewerNames) > 0 {
 		for _, t := range teamReviewerNames {
-			var teamReviewer *organization.Team
-			teamReviewer, err = organization.GetTeam(ctx, ctx.Repo.Owner.ID, t)
+			teamReviewer, err := organization.GetTeam(ctx, ctx.Repo.Owner.ID, t)
 			if err != nil {
-				if organization.IsErrTeamNotExist(err) {
-					ctx.APIErrorNotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", t))
-					return nil, nil
-				}
-				ctx.APIErrorInternal(err)
-				return nil, nil
+				return nil, nil, err
 			}
 
 			teamReviewers = append(teamReviewers, teamReviewer)
 		}
 	}
-	return reviewers, teamReviewers
+	return reviewers, teamReviewers, nil
 }
 
-func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions, isAdd bool) {
+func deleteReviewRequests(ctx *context.APIContext, opts api.PullReviewRequestOptions) {
 	pr, err := issues_model.GetPullRequestByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index"))
 	if err != nil {
 		if issues_model.IsErrPullRequestNotExist(err) {
@@ -719,18 +792,21 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions
 		return
 	}
 
-	reviewers, teamReviewers := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers)
-	if ctx.Written() {
+	reviewers, teamReviewers, err := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers)
+	switch {
+	case user_model.IsErrUserNotExist(err):
+		ctx.APIErrorNotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name))
+		return
+	case organization.IsErrTeamNotExist(err):
+		ctx.APIErrorNotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name))
+		return
+	case err != nil:
+		ctx.APIError(http.StatusInternalServerError, err)
 		return
-	}
-
-	var reviews []*issues_model.Review
-	if isAdd {
-		reviews = make([]*issues_model.Review, 0, len(reviewers))
 	}
 
 	for _, reviewer := range reviewers {
-		comment, err := issue_service.ReviewRequest(ctx, pr.Issue, ctx.Doer, &permDoer, reviewer, isAdd)
+		_, err := pull_service.ReviewRequest(ctx, pr, ctx.Doer, &permDoer, reviewer, false)
 		if err != nil {
 			if issues_model.IsErrReviewRequestOnClosedPR(err) {
 				ctx.APIError(http.StatusForbidden, err)
@@ -743,19 +819,11 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions
 			ctx.APIErrorInternal(err)
 			return
 		}
-
-		if comment != nil && isAdd {
-			if err = comment.LoadReview(ctx); err != nil {
-				ctx.APIErrorInternal(err)
-				return
-			}
-			reviews = append(reviews, comment.Review)
-		}
 	}
 
 	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)
+			_, err := pull_service.TeamReviewRequest(ctx, pr, ctx.Doer, teamReviewer, false)
 			if err != nil {
 				if issues_model.IsErrReviewRequestOnClosedPR(err) {
 					ctx.APIError(http.StatusForbidden, err)
@@ -768,28 +836,10 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions
 				ctx.APIErrorInternal(err)
 				return
 			}
-
-			if comment != nil && isAdd {
-				if err = comment.LoadReview(ctx); err != nil {
-					ctx.APIErrorInternal(err)
-					return
-				}
-				reviews = append(reviews, comment.Review)
-			}
 		}
 	}
 
-	if isAdd {
-		apiReviews, err := convert.ToPullReviewList(ctx, reviews, ctx.Doer)
-		if err != nil {
-			ctx.APIErrorInternal(err)
-			return
-		}
-		ctx.JSON(http.StatusCreated, apiReviews)
-	} else {
-		ctx.Status(http.StatusNoContent)
-		return
-	}
+	ctx.Status(http.StatusNoContent)
 }
 
 // DismissPullReview dismiss a review for a pull request

routers/web/repo/issue_page_meta.go

@@ -18,7 +18,6 @@ import (
 	"code.gitea.io/gitea/modules/optional"
 	shared_user "code.gitea.io/gitea/routers/web/shared/user"
 	"code.gitea.io/gitea/services/context"
-	issue_service "code.gitea.io/gitea/services/issue"
 	pull_service "code.gitea.io/gitea/services/pull"
 )
 
@@ -194,7 +193,7 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) {
 		if d.Issue == nil {
 			data.CanChooseReviewer = true
 		} else {
-			data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID)
+			data.CanChooseReviewer = pull_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID)
 		}
 	}
 

routers/web/repo/pull_review.go

@@ -20,7 +20,6 @@ import (
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/context/upload"
 	"code.gitea.io/gitea/services/forms"
-	issue_service "code.gitea.io/gitea/services/issue"
 	pull_service "code.gitea.io/gitea/services/pull"
 	user_service "code.gitea.io/gitea/services/user"
 )
@@ -365,6 +364,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 +398,8 @@ func UpdatePullReviewRequest(ctx *context.Context) {
 				return
 			}
 
-			_, err = issue_service.TeamReviewRequest(ctx, issue, ctx.Doer, team, action == "attach")
+			// TODO: Team review request should check if the team has permission to review the PR
+			_, err = pull_service.TeamReviewRequest(ctx, issue.PullRequest, ctx.Doer, team, action == "attach")
 			if err != nil {
 				if issues_model.IsErrNotValidReviewRequest(err) {
 					log.Warn(
@@ -427,7 +431,8 @@ func UpdatePullReviewRequest(ctx *context.Context) {
 			return
 		}
 
-		_, err = issue_service.ReviewRequest(ctx, issue, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach")
+		// TODO: Reviewer review request should check if the user has permission to review the PR
+		_, err = pull_service.ReviewRequest(ctx, issue.PullRequest, ctx.Doer, &ctx.Repo.Permission, reviewer, action == "attach")
 		if err != nil {
 			if issues_model.IsErrNotValidReviewRequest(err) {
 				log.Warn(