Fix get reviewers' bug

models/organization/team_repo.go

@@ -9,6 +9,7 @@ import (
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/perm"
 	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unit"
 
 	"xorm.io/builder"
 )
@@ -83,3 +84,16 @@ func GetTeamsWithAccessToRepo(ctx context.Context, orgID, repoID int64, mode per
 		OrderBy("name").
 		Find(&teams)
 }
+
+// GetTeamsWithAccessToRepoUnit returns all teams in an organization that have given access level to the repository special unit.
+func GetTeamsWithAccessToRepoUnit(ctx context.Context, orgID, repoID int64, mode perm.AccessMode, unitType unit.Type) ([]*Team, error) {
+	teams := make([]*Team, 0, 5)
+	return teams, db.GetEngine(ctx).Where("team_unit.access_mode >= ?", mode).
+		Join("INNER", "team_repo", "team_repo.team_id = team.id").
+		Join("INNER", "team_unit", "team_unit.team_id = team.id").
+		And("team_repo.org_id = ?", orgID).
+		And("team_repo.repo_id = ?", repoID).
+		And("team_unit.type = ?", unitType).
+		OrderBy("name").
+		Find(&teams)
+}

models/organization/team_repo_test.go

@@ -0,0 +1,31 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package organization_test
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/models/organization"
+	"code.gitea.io/gitea/models/perm"
+	"code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unit"
+	"code.gitea.io/gitea/models/unittest"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetTeamsWithAccessToRepoUnit(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	org41 := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 41})
+	repo61 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 61})
+
+	teams, err := organization.GetTeamsWithAccessToRepoUnit(db.DefaultContext, org41.ID, repo61.ID, perm.AccessModeRead, unit.TypePullRequests)
+	assert.NoError(t, err)
+	if assert.Len(t, teams, 2) {
+		assert.EqualValues(t, 21, teams[0].ID)
+		assert.EqualValues(t, 22, teams[1].ID)
+	}
+}

models/repo/user_repo.go

@@ -11,7 +11,6 @@ import (
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/container"
-	api "code.gitea.io/gitea/modules/structs"
 
 	"xorm.io/builder"
 )
@@ -146,57 +145,6 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us
 	return users, nil
 }
 
-// GetReviewers get all users can be requested to review:
-// * for private repositories this returns all users that have read access or higher to the repository.
-// * for public repositories this returns all users that have read access or higher to the repository,
-// all repo watchers and all organization members.
-// TODO: may be we should have a busy choice for users to block review request to them.
-func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) ([]*user_model.User, error) {
-	// Get the owner of the repository - this often already pre-cached and if so saves complexity for the following queries
-	if err := repo.LoadOwner(ctx); err != nil {
-		return nil, err
-	}
-
-	cond := builder.And(builder.Neq{"`user`.id": posterID}).
-		And(builder.Eq{"`user`.is_active": true})
-
-	if repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate {
-		// This a private repository:
-		// Anyone who can read the repository is a requestable reviewer
-
-		cond = cond.And(builder.In("`user`.id",
-			builder.Select("user_id").From("access").Where(
-				builder.Eq{"repo_id": repo.ID}.
-					And(builder.Gte{"mode": perm.AccessModeRead}),
-			),
-		))
-
-		if repo.Owner.Type == user_model.UserTypeIndividual && repo.Owner.ID != posterID {
-			// as private *user* repos don't generate an entry in the `access` table,
-			// the owner of a private repo needs to be explicitly added.
-			cond = cond.Or(builder.Eq{"`user`.id": repo.Owner.ID})
-		}
-	} else {
-		// This is a "public" repository:
-		// Any user that has read access, is a watcher or organization member can be requested to review
-		cond = cond.And(builder.And(builder.In("`user`.id",
-			builder.Select("user_id").From("access").
-				Where(builder.Eq{"repo_id": repo.ID}.
-					And(builder.Gte{"mode": perm.AccessModeRead})),
-		).Or(builder.In("`user`.id",
-			builder.Select("user_id").From("watch").
-				Where(builder.Eq{"repo_id": repo.ID}.
-					And(builder.In("mode", WatchModeNormal, WatchModeAuto))),
-		).Or(builder.In("`user`.id",
-			builder.Select("uid").From("org_user").
-				Where(builder.Eq{"org_id": repo.OwnerID}),
-		)))))
-	}
-
-	users := make([]*user_model.User, 0, 8)
-	return users, db.GetEngine(ctx).Where(cond).OrderBy(user_model.GetOrderByName()).Find(&users)
-}
-
 // GetIssuePostersWithSearch returns users with limit of 30 whose username started with prefix that have authored an issue/pull request for the given repository
 // If isShowFullName is set to true, also include full name prefix search
 func GetIssuePostersWithSearch(ctx context.Context, repo *Repository, isPull bool, search string, isShowFullName bool) ([]*user_model.User, error) {

models/repo/user_repo_test.go

@@ -38,46 +38,3 @@ func TestRepoAssignees(t *testing.T) {
 		assert.NotContains(t, []int64{users[0].ID, users[1].ID, users[2].ID}, 15)
 	}
 }
-
-func TestRepoGetReviewers(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
-	// test public repo
-	repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-
-	ctx := db.DefaultContext
-	reviewers, err := repo_model.GetReviewers(ctx, repo1, 2, 2)
-	assert.NoError(t, err)
-	if assert.Len(t, reviewers, 3) {
-		assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID})
-	}
-
-	// should include doer if doer is not PR poster.
-	reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 2)
-	assert.NoError(t, err)
-	assert.Len(t, reviewers, 3)
-
-	// should not include PR poster, if PR poster would be otherwise eligible
-	reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 4)
-	assert.NoError(t, err)
-	assert.Len(t, reviewers, 2)
-
-	// test private user repo
-	repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
-
-	reviewers, err = repo_model.GetReviewers(ctx, repo2, 2, 4)
-	assert.NoError(t, err)
-	assert.Len(t, reviewers, 1)
-	assert.EqualValues(t, reviewers[0].ID, 2)
-
-	// test private org repo
-	repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
-
-	reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 1)
-	assert.NoError(t, err)
-	assert.Len(t, reviewers, 2)
-
-	reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 2)
-	assert.NoError(t, err)
-	assert.Len(t, reviewers, 1)
-}

routers/api/v1/repo/collaborators.go

@@ -17,6 +17,8 @@ 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"
 )
 
@@ -320,7 +322,13 @@ func GetReviewers(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
-	reviewers, err := repo_model.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0)
+	canChooseReviewer := issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0)
+	if !canChooseReviewer {
+		ctx.Error(http.StatusForbidden, "GetReviewers", errors.New("doer has no permission to get reviewers"))
+		return
+	}
+
+	reviewers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "ListCollaborators", err)
 		return

routers/web/repo/issue_page_meta.go

@@ -19,7 +19,7 @@ import (
 	shared_user "code.gitea.io/gitea/routers/web/shared/user"
 	"code.gitea.io/gitea/services/context"
 	issue_service "code.gitea.io/gitea/services/issue"
-	repo_service "code.gitea.io/gitea/services/repository"
+	pull_service "code.gitea.io/gitea/services/pull"
 )
 
 type issueSidebarMilestoneData struct {
@@ -186,7 +186,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)
+			data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID)
 		}
 	}
 
@@ -231,13 +231,13 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) {
 
 	if data.CanChooseReviewer {
 		var err error
-		reviewers, err = repo_model.GetReviewers(ctx, repo, ctx.Doer.ID, posterID)
+		reviewers, err = pull_service.GetReviewers(ctx, repo, ctx.Doer.ID, posterID)
 		if err != nil {
 			ctx.ServerError("GetReviewers", err)
 			return
 		}
 
-		teamReviewers, err = repo_service.GetReviewerTeams(ctx, repo)
+		teamReviewers, err = pull_service.GetReviewerTeams(ctx, repo)
 		if err != nil {
 			ctx.ServerError("GetReviewerTeams", err)
 			return

services/issue/assignee.go

@@ -119,7 +119,7 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
 		return err
 	}
 
-	canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue)
+	canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID)
 
 	if isAdd {
 		if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) {
@@ -178,7 +178,7 @@ func isValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team,
 		}
 	}
 
-	canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue)
+	canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID)
 
 	if isAdd {
 		if issue.Repo.IsPrivate {
@@ -276,12 +276,12 @@ func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doe
 }
 
 // CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR
-func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue) bool {
+func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, posterID int64) bool {
 	if repo.IsArchived {
 		return false
 	}
 	// The poster of the PR can change the reviewers
-	if doer.ID == issue.PosterID {
+	if doer.ID == posterID {
 		return true
 	}
 

services/pull/reviewer.go

@@ -0,0 +1,89 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package pull
+
+import (
+	"context"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/models/organization"
+	"code.gitea.io/gitea/models/perm"
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unit"
+	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/container"
+
+	"xorm.io/builder"
+)
+
+// GetReviewers get all users can be requested to review:
+// - Poster should not be listed
+// - For collaborator, all users that have read access or higher to the repository.
+// - For repository under organization, users under the teams which have read permission or higher of pull request unit
+// - Owner will be listed if it's not an organization, not the poster and not in the list of reviewers
+func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, posterID int64) ([]*user_model.User, error) {
+	if err := repo.LoadOwner(ctx); err != nil {
+		return nil, err
+	}
+
+	e := db.GetEngine(ctx)
+	uniqueUserIDs := make(container.Set[int64])
+
+	collaboratorIDs := make([]int64, 0, 10)
+	if err := e.Table("collaboration").Where("repo_id=?", repo.ID).
+		And("mode >= ?", perm.AccessModeRead).
+		Select("user_id").
+		Find(&collaboratorIDs); err != nil {
+		return nil, err
+	}
+	uniqueUserIDs.AddMultiple(collaboratorIDs...)
+
+	if repo.Owner.IsOrganization() {
+		additionalUserIDs := make([]int64, 0, 10)
+		if err := e.Table("team_user").
+			Join("INNER", "team_repo", "`team_repo`.team_id = `team_user`.team_id").
+			Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id").
+			Where("`team_repo`.repo_id = ? AND (`team_unit`.access_mode >= ? AND `team_unit`.`type` = ?)",
+				repo.ID, perm.AccessModeRead, unit.TypePullRequests).
+			Distinct("`team_user`.uid").
+			Select("`team_user`.uid").
+			Find(&additionalUserIDs); err != nil {
+			return nil, err
+		}
+		uniqueUserIDs.AddMultiple(additionalUserIDs...)
+	}
+
+	uniqueUserIDs.Remove(posterID) // posterID should not be in the list of reviewers
+
+	// Leave a seat for owner itself to append later, but if owner is an organization
+	// and just waste 1 unit is cheaper than re-allocate memory once.
+	users := make([]*user_model.User, 0, len(uniqueUserIDs)+1)
+	if len(uniqueUserIDs) > 0 {
+		if err := e.In("id", uniqueUserIDs.Values()).
+			Where(builder.Eq{"`user`.is_active": true}).
+			OrderBy(user_model.GetOrderByName()).
+			Find(&users); err != nil {
+			return nil, err
+		}
+	}
+
+	// add owner after all users are loaded because we can avoid load owner twice
+	if repo.OwnerID != posterID && !repo.Owner.IsOrganization() && !uniqueUserIDs.Contains(repo.OwnerID) {
+		users = append(users, repo.Owner)
+	}
+
+	return users, nil
+}
+
+// GetReviewerTeams get all teams can be requested to review
+func GetReviewerTeams(ctx context.Context, repo *repo_model.Repository) ([]*organization.Team, error) {
+	if err := repo.LoadOwner(ctx); err != nil {
+		return nil, err
+	}
+	if !repo.Owner.IsOrganization() {
+		return nil, nil
+	}
+
+	return organization.GetTeamsWithAccessToRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests)
+}