Some improvements

models/issues/review.go

@@ -738,7 +738,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/pull.go

@@ -16,6 +16,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"
@@ -561,8 +562,16 @@ func CreatePullRequest(ctx *context.APIContext) {
 		PullRequest: pr,
 		AssigneeIDs: assigneeIDs,
 	}
-	prOpts.Reviewers, prOpts.TeamReviewers = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers)
+	prOpts.Reviewers, prOpts.TeamReviewers, err = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers)
-	if ctx.Written() {
+	switch {
+	case user_model.IsErrUserNotExist(err):
+		ctx.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name))
+		return
+	case organization.IsErrTeamNotExist(err):
+		ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name))
+		return
+	case err != nil:
+		ctx.Error(http.StatusInternalServerError, "GetUser", err)
 		return
 	}
 

routers/api/v1/repo/pull_review.go

@@ -628,12 +628,18 @@ func CreateReviewRequests(ctx *context.APIContext) {
 	}
 	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.Error(http.StatusUnprocessableEntity, "", "")
+			return
+		}
 	}
 
 	filteredTeams := make([]*organization.Team, 0, len(opts.TeamReviewers))
@@ -644,12 +650,18 @@ func CreateReviewRequests(ctx *context.APIContext) {
 			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.Error(http.StatusUnprocessableEntity, "", "")
+				return
+			}
 		}
 	}
 	comments, err := pull_service.ReviewRequests(ctx, pr, ctx.Doer, filteredUsers, filteredTeams)
@@ -727,8 +739,7 @@ func DeleteReviewRequests(ctx *context.APIContext) {
 	deleteReviewRequests(ctx, *opts)
 }
 
-func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerNames []string) (reviewers []*user_model.User, teamReviewers []*organization.Team) {
+func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerNames []string) (reviewers []*user_model.User, teamReviewers []*organization.Team, err error) {
-	var err error
 	for _, r := range reviewerNames {
 		var reviewer *user_model.User
 		if strings.Contains(r, "@") {
@@ -736,14 +747,8 @@ func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerN
 		} else {
 			reviewer, err = user_model.GetUserByName(ctx, r)
 		}
-
 		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
+			return nil, nil, err
-				ctx.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", r))
-				return nil, nil
-			}
-			ctx.Error(http.StatusInternalServerError, "GetUser", err)
-			return nil, nil
 		}
 
 		reviewers = append(reviewers, reviewer)
@@ -751,21 +756,15 @@ func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerN
 
 	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) {
+				return nil, nil, err
-					ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", t))
-					return nil, nil
-				}
-				ctx.Error(http.StatusInternalServerError, "ReviewRequest", err)
-				return nil, nil
 			}
 
 			teamReviewers = append(teamReviewers, teamReviewer)
 		}
 	}
-	return reviewers, teamReviewers
+	return reviewers, teamReviewers, nil
 }
 
 func deleteReviewRequests(ctx *context.APIContext, opts api.PullReviewRequestOptions) {
@@ -790,8 +789,16 @@ func deleteReviewRequests(ctx *context.APIContext, opts api.PullReviewRequestOpt
 		return
 	}
 
-	reviewers, teamReviewers := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers)
+	reviewers, teamReviewers, err := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers)
-	if ctx.Written() {
+	switch {
+	case user_model.IsErrUserNotExist(err):
+		ctx.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name))
+		return
+	case organization.IsErrTeamNotExist(err):
+		ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name))
+		return
+	case err != nil:
+		ctx.Error(http.StatusInternalServerError, "GetUser", err)
 		return
 	}
 

services/pull/review_request.go

@@ -212,6 +212,10 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
 		}
 	}
 
+	if err := issue.LoadRepo(ctx); err != nil {
+		return err
+	}
+
 	permReviewer, err := access_model.GetUserRepoPermission(ctx, issue.Repo, reviewer)
 	if err != nil {
 		return err

tests/integration/api_pull_review_test.go

@@ -11,6 +11,7 @@ import (
 	auth_model "code.gitea.io/gitea/models/auth"
 	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/perm"
 	access_model "code.gitea.io/gitea/models/perm/access"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
@@ -18,6 +19,7 @@ import (
 	"code.gitea.io/gitea/modules/json"
 	api "code.gitea.io/gitea/modules/structs"
 	pull_service "code.gitea.io/gitea/services/pull"
+	repo_service "code.gitea.io/gitea/services/repository"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
@@ -246,6 +248,13 @@ func TestAPIPullReviewRequest(t *testing.T) {
 	req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
 		Reviewers: []string{"user4@example.com", "user8"},
 	}).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusUnprocessableEntity)
+
+	user8 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 8})
+	repo_service.AddOrUpdateCollaborator(db.DefaultContext, repo, user8, perm.AccessModeRead)
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
+		Reviewers: []string{"user8"},
+	}).AddTokenAuth(token)
 	MakeRequest(t, req, http.StatusCreated)
 
 	// poster of pr can't be reviewer
@@ -258,7 +267,7 @@ func TestAPIPullReviewRequest(t *testing.T) {
 	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
 		Reviewers: []string{"testOther"},
 	}).AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusNotFound)
+	MakeRequest(t, req, http.StatusUnprocessableEntity)
 
 	// Test Remove Review Request
 	session2 := loginUser(t, "user4")