From ab7cb9509deaa062033fb7f232be6d4975384c40 Mon Sep 17 00:00:00 2001
From: Lunny Xiao <xiaolunwen@gmail.com>
Date: Sun, 3 Nov 2024 22:50:10 -0800
Subject: [PATCH] Fix get reviewers' bug

---
 models/repo/user_repo.go | 82 +++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 39 deletions(-)

diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go
index c305603e02..c74023511d 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"
 )
@@ -145,54 +144,59 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us
 }
 
 // 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.
+// - Poster should not be listed
+// - For collabrators, all users that have read access or higher to the repository.
+// - For repository under orgnization, 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 *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})
+	e := db.GetEngine(ctx)
+	uniqueUserIDs := make(container.Set[int64])
 
-	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})
+	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...)
 	} 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}),
-		)))))
+		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...)
+	}
+	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)
 	}
 
-	users := make([]*user_model.User, 0, 8)
-	return users, db.GetEngine(ctx).Where(cond).OrderBy(user_model.GetOrderByName()).Find(&users)
+	return users, nil
 }
 
 // GetIssuePostersWithSearch returns users with limit of 30 whose username started with prefix that have authored an issue/pull request for the given repository