Fix get reviewers' bug

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

@@ -18,6 +18,7 @@ import (
 	"code.gitea.io/gitea/routers/api/v1/utils"
 	"code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/services/convert"
+	pull_service "code.gitea.io/gitea/services/pull"
 	repo_service "code.gitea.io/gitea/services/repository"
 )
 
@@ -323,7 +324,7 @@ func GetReviewers(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
-	reviewers, err := repo_model.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0)
+	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.go

@@ -56,7 +56,6 @@ import (
 	"code.gitea.io/gitea/services/forms"
 	issue_service "code.gitea.io/gitea/services/issue"
 	pull_service "code.gitea.io/gitea/services/pull"
-	repo_service "code.gitea.io/gitea/services/repository"
 	user_service "code.gitea.io/gitea/services/user"
 )
 
@@ -698,13 +697,13 @@ func RetrieveRepoReviewers(ctx *context.Context, repo *repo_model.Repository, is
 			posterID = 0
 		}
 
-		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/pull/reviewer.go

@@ -0,0 +1,108 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package pull
+
+import (
+	"context"
+	"fmt"
+
+	"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...)
+
+		if repo.Owner.Visibility.IsLimited() && doerID == 0 {
+			return nil, fmt.Errorf("permission denied")
+		}
+
+		if (repo.IsPrivate || repo.Owner.Visibility.IsPrivate()) && !uniqueUserIDs.Contains(doerID) {
+			return nil, fmt.Errorf("permission denied")
+		}
+	} else {
+		userIDs := make([]int64, 0, 10)
+		if err := e.Table("access").
+			Where("repo_id = ? AND mode >= ?", repo.ID, perm.AccessModeRead).
+			Select("user_id").
+			Find(&userIDs); err != nil {
+			return nil, err
+		}
+		uniqueUserIDs.AddMultiple(userIDs...)
+		if repo.IsPrivate && !uniqueUserIDs.Contains(doerID) && doerID != repo.OwnerID {
+			return nil, fmt.Errorf("permission denied")
+		}
+	}
+
+	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
+		}
+	}
+	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.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead)
+}

services/pull/reviewer_test.go

@@ -0,0 +1,72 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package pull_test
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unittest"
+	pull_service "code.gitea.io/gitea/services/pull"
+
+	"github.com/stretchr/testify/assert"
+)
+
+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 := pull_service.GetReviewers(ctx, repo1, 2, 0)
+	assert.NoError(t, err)
+	if assert.Len(t, reviewers, 1) {
+		assert.ElementsMatch(t, []int64{2}, []int64{reviewers[0].ID})
+	}
+
+	// should not include doer and remove the poster
+	reviewers, err = pull_service.GetReviewers(ctx, repo1, 11, 2)
+	assert.NoError(t, err)
+	assert.Len(t, reviewers, 0)
+
+	// should not include PR poster, if PR poster would be otherwise eligible
+	reviewers, err = pull_service.GetReviewers(ctx, repo1, 11, 4)
+	assert.NoError(t, err)
+	assert.Len(t, reviewers, 1)
+
+	// test private user repo
+	repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
+
+	reviewers, err = pull_service.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 = pull_service.GetReviewers(ctx, repo3, 2, 1)
+	assert.NoError(t, err)
+	assert.Len(t, reviewers, 2)
+
+	reviewers, err = pull_service.GetReviewers(ctx, repo3, 2, 2)
+	assert.NoError(t, err)
+	assert.Len(t, reviewers, 1)
+}
+
+func TestRepoGetReviewerTeams(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
+	teams, err := pull_service.GetReviewerTeams(db.DefaultContext, repo2)
+	assert.NoError(t, err)
+	assert.Empty(t, teams)
+
+	repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
+	teams, err = pull_service.GetReviewerTeams(db.DefaultContext, repo3)
+	assert.NoError(t, err)
+	assert.Len(t, teams, 2)
+}