Fix get reviewers' bug (#32415)

This PR rewrites `GetReviewer` function and move it to service layer.

Reviewers should not be watchers, so that this PR removed all watchers
from reviewers. When the repository is under an organization, the pull
request unit read permission will be checked to resolve the bug of
#32394

Fix #32394

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)
+}

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)
+}

services/repository/review.go

@@ -1,24 +0,0 @@
-// Copyright 2022 The Gitea Authors. All rights reserved.
-// SPDX-License-Identifier: MIT
-
-package repository
-
-import (
-	"context"
-
-	"code.gitea.io/gitea/models/organization"
-	"code.gitea.io/gitea/models/perm"
-	repo_model "code.gitea.io/gitea/models/repo"
-)
-
-// 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/repository/review_test.go

@@ -1,28 +0,0 @@
-// Copyright 2022 The Gitea Authors. All rights reserved.
-// SPDX-License-Identifier: MIT
-
-package repository
-
-import (
-	"testing"
-
-	"code.gitea.io/gitea/models/db"
-	repo_model "code.gitea.io/gitea/models/repo"
-	"code.gitea.io/gitea/models/unittest"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func TestRepoGetReviewerTeams(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
-	repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2})
-	teams, err := GetReviewerTeams(db.DefaultContext, repo2)
-	assert.NoError(t, err)
-	assert.Empty(t, teams)
-
-	repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3})
-	teams, err = GetReviewerTeams(db.DefaultContext, repo3)
-	assert.NoError(t, err)
-	assert.Len(t, teams, 2)
-}