diff --git a/models/organization/team_repo.go b/models/organization/team_repo.go
index 1184e39263..c90dfdeda0 100644
--- a/models/organization/team_repo.go
+++ b/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)
+}
diff --git a/models/organization/team_repo_test.go b/models/organization/team_repo_test.go
new file mode 100644
index 0000000000..c0d6750df9
--- /dev/null
+++ b/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)
+ }
+}
diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go
index ecc9216950..a9b1360df1 100644
--- a/models/repo/user_repo.go
+++ b/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) {
diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go
index d2bf6dc912..f2abc2ffa0 100644
--- a/models/repo/user_repo_test.go
+++ b/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)
-}
diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go
index ea9d8b0f37..0bbf5a1ea4 100644
--- a/routers/api/v1/repo/collaborators.go
+++ b/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
diff --git a/routers/web/repo/issue_page_meta.go b/routers/web/repo/issue_page_meta.go
index ac0b1c6425..e04d76b287 100644
--- a/routers/web/repo/issue_page_meta.go
+++ b/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
diff --git a/services/issue/assignee.go b/services/issue/assignee.go
index 52ee9f2b22..c7e2495568 100644
--- a/services/issue/assignee.go
+++ b/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
}
diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go
new file mode 100644
index 0000000000..bf0d8cb298
--- /dev/null
+++ b/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)
+}
diff --git a/services/pull/reviewer_test.go b/services/pull/reviewer_test.go
new file mode 100644
index 0000000000..1ff373bafb
--- /dev/null
+++ b/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)
+}
diff --git a/services/repository/review.go b/services/repository/review.go
deleted file mode 100644
index 40513e6bc6..0000000000
--- a/services/repository/review.go
+++ /dev/null
@@ -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)
-}
diff --git a/services/repository/review_test.go b/services/repository/review_test.go
deleted file mode 100644
index 2db56d4e8a..0000000000
--- a/services/repository/review_test.go
+++ /dev/null
@@ -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)
-}
diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go
index 93c9ca0920..122afbfa08 100644
--- a/tests/integration/api_repo_test.go
+++ b/tests/integration/api_repo_test.go
@@ -718,8 +718,8 @@ func TestAPIRepoGetReviewers(t *testing.T) {
resp := MakeRequest(t, req, http.StatusOK)
var reviewers []*api.User
DecodeJSON(t, resp, &reviewers)
- if assert.Len(t, reviewers, 3) {
- assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID})
+ if assert.Len(t, reviewers, 1) {
+ assert.ElementsMatch(t, []int64{2}, []int64{reviewers[0].ID})
}
}