Fix FIXME and remove superfluous queries in models/org (#749)

models/action.go

@@ -658,17 +658,14 @@ func GetFeeds(ctxUser *User, actorID, offset int64, isProfile bool) ([]*Action,
 			And("is_private = ?", false).
 			And("act_user_id = ?", ctxUser.ID)
 	} else if actorID != -1 && ctxUser.IsOrganization() {
-		// FIXME: only need to get IDs here, not all fields of repository.
+		env, err := ctxUser.AccessibleReposEnv(actorID)
-		repos, _, err := ctxUser.GetUserRepositories(actorID, 1, ctxUser.NumRepos)
 		if err != nil {
-			return nil, fmt.Errorf("GetUserRepositories: %v", err)
+			return nil, fmt.Errorf("AccessibleReposEnv: %v", err)
 		}
-
+		repoIDs, err := env.RepoIDs(1, ctxUser.NumRepos)
-		var repoIDs []int64
+		if err != nil {
-		for _, repo := range repos {
+			return nil, fmt.Errorf("GetUserRepositories: %v", err)
-			repoIDs = append(repoIDs, repo.ID)
 		}
-
 		if len(repoIDs) > 0 {
 			sess.In("repo_id", repoIDs)
 		}

models/org.go

@@ -471,12 +471,6 @@ func RemoveOrgUser(orgID, userID int64) error {
 		return fmt.Errorf("GetUserByID [%d]: %v", orgID, err)
 	}
 
-	// FIXME: only need to get IDs here, not all fields of repository.
-	repos, _, err := org.GetUserRepositories(user.ID, 1, org.NumRepos)
-	if err != nil {
-		return fmt.Errorf("GetUserRepositories [%d]: %v", user.ID, err)
-	}
-
 	// Check if the user to delete is the last member in owner team.
 	if IsOrganizationOwner(orgID, userID) {
 		t, err := org.GetOwnerTeam()
@@ -501,10 +495,16 @@ func RemoveOrgUser(orgID, userID int64) error {
 	}
 
 	// Delete all repository accesses and unwatch them.
-	repoIDs := make([]int64, len(repos))
+	env, err := org.AccessibleReposEnv(user.ID)
-	for i := range repos {
+	if err != nil {
-		repoIDs = append(repoIDs, repos[i].ID)
+		return fmt.Errorf("AccessibleReposEnv: %v", err)
-		if err = watchRepo(sess, user.ID, repos[i].ID, false); err != nil {
+	}
+	repoIDs, err := env.RepoIDs(1, org.NumRepos)
+	if err != nil {
+		return fmt.Errorf("GetUserRepositories [%d]: %v", user.ID, err)
+	}
+	for _, repoID := range repoIDs {
+		if err = watchRepo(sess, user.ID, repoID, false); err != nil {
 			return err
 		}
 	}
@@ -577,82 +577,90 @@ func (org *User) GetUserTeams(userID int64) ([]*Team, error) {
 	return org.getUserTeams(x, userID)
 }
 
-// GetUserRepositories returns a range of repositories in organization
+// AccessibleReposEnvironment operations involving the repositories that are
-// that the user with the given userID has access to,
+// accessible to a particular user
-// and total number of records based on given condition.
+type AccessibleReposEnvironment interface {
-func (org *User) GetUserRepositories(userID int64, page, pageSize int) ([]*Repository, int64, error) {
+	CountRepos() (int64, error)
-	var cond builder.Cond = builder.Eq{
+	RepoIDs(page, pageSize int) ([]int64, error)
-		"`repository`.owner_id":   org.ID,
+	Repos(page, pageSize int) ([]*Repository, error)
-		"`repository`.is_private": false,
+	MirrorRepos() ([]*Repository, error)
-	}
+}
 
+type accessibleReposEnv struct {
+	org     *User
+	userID  int64
+	teamIDs []int64
+}
+
+// AccessibleReposEnv an AccessibleReposEnvironment for the repositories in `org`
+// that are accessible to the specified user.
+func (org *User) AccessibleReposEnv(userID int64) (AccessibleReposEnvironment, error) {
 	teamIDs, err := org.GetUserTeamIDs(userID)
 	if err != nil {
-		return nil, 0, fmt.Errorf("GetUserTeamIDs: %v", err)
+		return nil, err
 	}
+	return &accessibleReposEnv{org: org, userID: userID, teamIDs: teamIDs}, nil
+}
 
-	if len(teamIDs) > 0 {
+func (env *accessibleReposEnv) cond() builder.Cond {
-		cond = cond.Or(builder.In("team_repo.team_id", teamIDs))
+	var cond builder.Cond = builder.Eq{
+		"`repository`.owner_id":   env.org.ID,
+		"`repository`.is_private": false,
 	}
+	if len(env.teamIDs) > 0 {
+		cond = cond.Or(builder.In("team_repo.team_id", env.teamIDs))
+	}
+	return cond
+}
 
+func (env *accessibleReposEnv) CountRepos() (int64, error) {
+	repoCount, err := x.
+		Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id").
+		Where(env.cond()).
+		GroupBy("`repository`.id").
+		Count(&Repository{})
+	if err != nil {
+		return 0, fmt.Errorf("count user repositories in organization: %v", err)
+	}
+	return repoCount, nil
+}
+
+func (env *accessibleReposEnv) RepoIDs(page, pageSize int) ([]int64, error) {
 	if page <= 0 {
 		page = 1
 	}
 
-	repos := make([]*Repository, 0, pageSize)
+	repoIDs := make([]int64, 0, pageSize)
-
+	return repoIDs, x.
-	if err := x.
+		Table("repository").
-		Select("`repository`.id").
 		Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id").
-		Where(cond).
+		Where(env.cond()).
 		GroupBy("`repository`.id,`repository`.updated_unix").
 		OrderBy("updated_unix DESC").
 		Limit(pageSize, (page-1)*pageSize).
-		Find(&repos); err != nil {
+		Cols("`repository`.id").
-		return nil, 0, fmt.Errorf("get repository ids: %v", err)
+		Find(&repoIDs)
-	}
+}
-
-	repoIDs := make([]int64,pageSize)
-	for i := range repos {
-		repoIDs[i] = repos[i].ID
-	}
-
-	if err := x.
-		Select("`repository`.*").
-		Where(builder.In("`repository`.id",repoIDs)).
-		Find(&repos); err!=nil {
-		return nil, 0, fmt.Errorf("get repositories: %v", err)
-	}
 
-	repoCount, err := x.
+func (env *accessibleReposEnv) Repos(page, pageSize int) ([]*Repository, error) {
-		Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id").
+	repoIDs, err := env.RepoIDs(page, pageSize)
-		Where(cond).
-		GroupBy("`repository`.id").
-		Count(&Repository{})
 	if err != nil {
-		return nil, 0, fmt.Errorf("count user repositories in organization: %v", err)
+		return nil, fmt.Errorf("GetUserRepositoryIDs: %v", err)
 	}
 
-	return repos, repoCount, nil
+	repos := make([]*Repository, 0, len(repoIDs))
+	return repos, x.
+		Select("`repository`.*").
+		Where(builder.In("`repository`.id", repoIDs)).
+		Find(&repos)
 }
 
-// GetUserMirrorRepositories returns mirror repositories of the user
+func (env *accessibleReposEnv) MirrorRepos() ([]*Repository, error) {
-// that the user with the given userID has access to.
-func (org *User) GetUserMirrorRepositories(userID int64) ([]*Repository, error) {
-	teamIDs, err := org.GetUserTeamIDs(userID)
-	if err != nil {
-		return nil, fmt.Errorf("GetUserTeamIDs: %v", err)
-	}
-	if len(teamIDs) == 0 {
-		teamIDs = []int64{-1}
-	}
-
 	repos := make([]*Repository, 0, 10)
 	return repos, x.
 		Select("`repository`.*").
 		Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id AND `repository`.is_mirror=?", true).
-		Where("(`repository`.owner_id=? AND `repository`.is_private=?)", org.ID, false).
+		Where(env.cond()).
-		Or(builder.In("team_repo.team_id", teamIDs)).
 		GroupBy("`repository`.id").
 		OrderBy("updated_unix DESC").
 		Find(&repos)

routers/user/home.go

@@ -114,15 +114,20 @@ func Dashboard(ctx *context.Context) {
 	var err error
 	var repos, mirrors []*models.Repository
 	if ctxUser.IsOrganization() {
-		repos, _, err = ctxUser.GetUserRepositories(ctx.User.ID, 1, setting.UI.User.RepoPagingNum)
+		env, err := ctxUser.AccessibleReposEnv(ctx.User.ID)
 		if err != nil {
-			ctx.Handle(500, "GetUserRepositories", err)
+			ctx.Handle(500, "AccessibleReposEnv", err)
+			return
+		}
+		repos, err = env.Repos(1, setting.UI.User.RepoPagingNum)
+		if err != nil {
+			ctx.Handle(500, "env.Repos", err)
 			return
 		}
 
-		mirrors, err = ctxUser.GetUserMirrorRepositories(ctx.User.ID)
+		mirrors, err = env.MirrorRepos()
 		if err != nil {
-			ctx.Handle(500, "GetUserMirrorRepositories", err)
+			ctx.Handle(500, "env.MirrorRepos", err)
 			return
 		}
 	} else {
@@ -205,7 +210,12 @@ func Issues(ctx *context.Context) {
 	var err error
 	var repos []*models.Repository
 	if ctxUser.IsOrganization() {
-		repos, _, err = ctxUser.GetUserRepositories(ctx.User.ID, 1, ctxUser.NumRepos)
+		env, err := ctxUser.AccessibleReposEnv(ctx.User.ID)
+		if err != nil {
+			ctx.Handle(500, "AccessibleReposEnv", err)
+			return
+		}
+		repos, err = env.Repos(1, ctxUser.NumRepos)
 		if err != nil {
 			ctx.Handle(500, "GetRepositories", err)
 			return
@@ -353,9 +363,19 @@ func showOrgProfile(ctx *context.Context) {
 		err   error
 	)
 	if ctx.IsSigned && !ctx.User.IsAdmin {
-		repos, count, err = org.GetUserRepositories(ctx.User.ID, page, setting.UI.User.RepoPagingNum)
+		env, err := org.AccessibleReposEnv(ctx.User.ID)
+		if err != nil {
+			ctx.Handle(500, "AccessibleReposEnv", err)
+			return
+		}
+		repos, err = env.Repos(page, setting.UI.User.RepoPagingNum)
+		if err != nil {
+			ctx.Handle(500, "env.Repos", err)
+			return
+		}
+		count, err = env.CountRepos()
 		if err != nil {
-			ctx.Handle(500, "GetUserRepositories", err)
+			ctx.Handle(500, "env.CountRepos", err)
 			return
 		}
 		ctx.Data["Repos"] = repos