Skip to content

Commit 0358a67

Browse files
lunnylunny
committed
Some improvements
1 parent 43aeacb commit 0358a67

File tree

5 files changed

+53
-24
lines changed

5 files changed

+53
-24
lines changed

‎models/issues/review.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -738,7 +738,7 @@ func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user
738738
return nil, nil
739739
}
740740

741-
if _, err = db.DeleteByBean(ctx, review); err != nil {
741+
if _, err = db.DeleteByID[Review](ctx, review.ID); err != nil {
742742
return nil, err
743743
}
744744

‎routers/api/v1/repo/pull.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
activities_model "code.gitea.io/gitea/models/activities"
1717
git_model "code.gitea.io/gitea/models/git"
1818
issues_model "code.gitea.io/gitea/models/issues"
19+
"code.gitea.io/gitea/models/organization"
1920
access_model "code.gitea.io/gitea/models/perm/access"
2021
pull_model "code.gitea.io/gitea/models/pull"
2122
repo_model "code.gitea.io/gitea/models/repo"
@@ -561,8 +562,16 @@ func CreatePullRequest(ctx *context.APIContext) {
561562
PullRequest: pr,
562563
AssigneeIDs: assigneeIDs,
563564
}
564-
prOpts.Reviewers, prOpts.TeamReviewers = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers)
565-
if ctx.Written() {
565+
prOpts.Reviewers, prOpts.TeamReviewers, err = parseReviewersByNames(ctx, form.Reviewers, form.TeamReviewers)
566+
switch {
567+
case user_model.IsErrUserNotExist(err):
568+
ctx.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name))
569+
return
570+
case organization.IsErrTeamNotExist(err):
571+
ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name))
572+
return
573+
case err != nil:
574+
ctx.Error(http.StatusInternalServerError, "GetUser", err)
566575
return
567576
}
568577

‎routers/api/v1/repo/pull_review.go

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -628,12 +628,18 @@ func CreateReviewRequests(ctx *context.APIContext) {
628628
}
629629
filteredUsers := make([]*user_model.User, 0, len(opts.Reviewers))
630630
for _, reviewer := range opts.Reviewers {
631+
found := false
631632
for _, allowedUser := range allowedUsers {
632633
if allowedUser.Name == reviewer || allowedUser.Email == reviewer {
633634
filteredUsers = append(filteredUsers, allowedUser)
635+
found = true
634636
break
635637
}
636638
}
639+
if !found {
640+
ctx.Error(http.StatusUnprocessableEntity, "", "")
641+
return
642+
}
637643
}
638644

639645
filteredTeams := make([]*organization.Team, 0, len(opts.TeamReviewers))
@@ -644,12 +650,18 @@ func CreateReviewRequests(ctx *context.APIContext) {
644650
return
645651
}
646652
for _, teamReviewer := range opts.TeamReviewers {
653+
found := false
647654
for _, allowedTeam := range allowedTeams {
648655
if allowedTeam.Name == teamReviewer {
649656
filteredTeams = append(filteredTeams, allowedTeam)
657+
found = true
650658
break
651659
}
652660
}
661+
if !found {
662+
ctx.Error(http.StatusUnprocessableEntity, "", "")
663+
return
664+
}
653665
}
654666
}
655667
comments, err := pull_service.ReviewRequests(ctx, pr, ctx.Doer, filteredUsers, filteredTeams)
@@ -727,45 +739,32 @@ func DeleteReviewRequests(ctx *context.APIContext) {
727739
deleteReviewRequests(ctx, *opts)
728740
}
729741

730-
func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerNames []string) (reviewers []*user_model.User, teamReviewers []*organization.Team) {
731-
var err error
742+
func parseReviewersByNames(ctx *context.APIContext, reviewerNames, teamReviewerNames []string) (reviewers []*user_model.User, teamReviewers []*organization.Team, err error) {
732743
for _, r := range reviewerNames {
733744
var reviewer *user_model.User
734745
if strings.Contains(r, "@") {
735746
reviewer, err = user_model.GetUserByEmail(ctx, r)
736747
} else {
737748
reviewer, err = user_model.GetUserByName(ctx, r)
738749
}
739-
740750
if err != nil {
741-
if user_model.IsErrUserNotExist(err) {
742-
ctx.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", r))
743-
return nil, nil
744-
}
745-
ctx.Error(http.StatusInternalServerError, "GetUser", err)
746-
return nil, nil
751+
return nil, nil, err
747752
}
748753

749754
reviewers = append(reviewers, reviewer)
750755
}
751756

752757
if ctx.Repo.Repository.Owner.IsOrganization() && len(teamReviewerNames) > 0 {
753758
for _, t := range teamReviewerNames {
754-
var teamReviewer *organization.Team
755-
teamReviewer, err = organization.GetTeam(ctx, ctx.Repo.Owner.ID, t)
759+
teamReviewer, err := organization.GetTeam(ctx, ctx.Repo.Owner.ID, t)
756760
if err != nil {
757-
if organization.IsErrTeamNotExist(err) {
758-
ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", t))
759-
return nil, nil
760-
}
761-
ctx.Error(http.StatusInternalServerError, "ReviewRequest", err)
762-
return nil, nil
761+
return nil, nil, err
763762
}
764763

765764
teamReviewers = append(teamReviewers, teamReviewer)
766765
}
767766
}
768-
return reviewers, teamReviewers
767+
return reviewers, teamReviewers, nil
769768
}
770769

771770
func deleteReviewRequests(ctx *context.APIContext, opts api.PullReviewRequestOptions) {
@@ -790,8 +789,16 @@ func deleteReviewRequests(ctx *context.APIContext, opts api.PullReviewRequestOpt
790789
return
791790
}
792791

793-
reviewers, teamReviewers := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers)
794-
if ctx.Written() {
792+
reviewers, teamReviewers, err := parseReviewersByNames(ctx, opts.Reviewers, opts.TeamReviewers)
793+
switch {
794+
case user_model.IsErrUserNotExist(err):
795+
ctx.NotFound("UserNotExist", fmt.Sprintf("User '%s' not exist", err.(user_model.ErrUserNotExist).Name))
796+
return
797+
case organization.IsErrTeamNotExist(err):
798+
ctx.NotFound("TeamNotExist", fmt.Sprintf("Team '%s' not exist", err.(organization.ErrTeamNotExist).Name))
799+
return
800+
case err != nil:
801+
ctx.Error(http.StatusInternalServerError, "GetUser", err)
795802
return
796803
}
797804

‎services/pull/review_request.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,10 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
212212
}
213213
}
214214

215+
if err := issue.LoadRepo(ctx); err != nil {
216+
return err
217+
}
218+
215219
permReviewer, err := access_model.GetUserRepoPermission(ctx, issue.Repo, reviewer)
216220
if err != nil {
217221
return err

‎tests/integration/api_pull_review_test.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,15 @@ import (
1111
auth_model "code.gitea.io/gitea/models/auth"
1212
"code.gitea.io/gitea/models/db"
1313
issues_model "code.gitea.io/gitea/models/issues"
14+
"code.gitea.io/gitea/models/perm"
1415
access_model "code.gitea.io/gitea/models/perm/access"
1516
repo_model "code.gitea.io/gitea/models/repo"
1617
"code.gitea.io/gitea/models/unittest"
1718
user_model "code.gitea.io/gitea/models/user"
1819
"code.gitea.io/gitea/modules/json"
1920
api "code.gitea.io/gitea/modules/structs"
2021
pull_service "code.gitea.io/gitea/services/pull"
22+
repo_service "code.gitea.io/gitea/services/repository"
2123
"code.gitea.io/gitea/tests"
2224

2325
"github.com/stretchr/testify/assert"
@@ -246,6 +248,13 @@ func TestAPIPullReviewRequest(t *testing.T) {
246248
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
247249
Reviewers: []string{"user4@example.com", "user8"},
248250
}).AddTokenAuth(token)
251+
MakeRequest(t, req, http.StatusUnprocessableEntity)
252+
253+
user8 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 8})
254+
repo_service.AddOrUpdateCollaborator(db.DefaultContext, repo, user8, perm.AccessModeRead)
255+
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
256+
Reviewers: []string{"user8"},
257+
}).AddTokenAuth(token)
249258
MakeRequest(t, req, http.StatusCreated)
250259

251260
// poster of pr can't be reviewer
@@ -258,7 +267,7 @@ func TestAPIPullReviewRequest(t *testing.T) {
258267
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers", repo.OwnerName, repo.Name, pullIssue.Index), &api.PullReviewRequestOptions{
259268
Reviewers: []string{"testOther"},
260269
}).AddTokenAuth(token)
261-
MakeRequest(t, req, http.StatusNotFound)
270+
MakeRequest(t, req, http.StatusUnprocessableEntity)
262271

263272
// Test Remove Review Request
264273
session2 := loginUser(t, "user4")

0 commit comments

Comments
 (0)