From 27983efeeac40a2c6ddc75e852b2cced7190aead Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 26 Nov 2024 17:16:35 -0800 Subject: [PATCH] Some improvements --- models/issues/review.go | 2 +- routers/api/v1/repo/pull.go | 13 ++++++- routers/api/v1/repo/pull_review.go | 47 +++++++++++++---------- services/pull/review_request.go | 4 ++ tests/integration/api_pull_review_test.go | 11 +++++- 5 files changed, 53 insertions(+), 24 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index 69241ff8856..0a038d9a590 100644 --- a/models/issues/review.go +++ b/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 } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 1116a4e9b1d..d317e2b8b3e 100644 --- a/routers/api/v1/repo/pull.go +++ b/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) - if ctx.Written() { + prOpts.Reviewers, prOpts.TeamReviewers, err = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers) + 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 } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 46da57dff1a..b7927c2d5d6 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/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) { - 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, "@") { @@ -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) { - ctx.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", r)) - return nil, nil - } - ctx.Error(http.StatusInternalServerError, "GetUser", err) - return nil, nil + return nil, nil, err } 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) { - ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", t)) - return nil, nil - } - ctx.Error(http.StatusInternalServerError, "ReviewRequest", err) - return nil, nil + return nil, nil, err } 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) - if ctx.Written() { + reviewers, teamReviewers, err := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers) + 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 } diff --git a/services/pull/review_request.go b/services/pull/review_request.go index 8ca74aad2c2..49d262f847b 100644 --- a/services/pull/review_request.go +++ b/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 diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 102c4381be5..b1308bb70f5 100644 --- a/tests/integration/api_pull_review_test.go +++ b/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")