From 847527fd6dd8e53eea918e6e41da6ebc64be1388 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 17 Feb 2017 08:58:19 +0800 Subject: [PATCH] Fix all the bugs in issues and pulls on dashboard (#943) * fix all the bugs in issues and pulls on dashboard * small fix and refactor * add method getRepoIDs for IssueList --- models/issue.go | 59 +++++++++++++++++ models/user.go | 28 ++++++++ routers/user/home.go | 98 +++++++++------------------- templates/user/dashboard/issues.tmpl | 2 +- 4 files changed, 120 insertions(+), 67 deletions(-) diff --git a/models/issue.go b/models/issue.go index 3ae34009bf..b160004fd5 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1399,3 +1399,62 @@ func updateIssue(e Engine, issue *Issue) error { func UpdateIssue(issue *Issue) error { return updateIssue(x, issue) } + +// IssueList defines a list of issues +type IssueList []*Issue + +func (issues IssueList) getRepoIDs() []int64 { + repoIDs := make([]int64, 0, len(issues)) + for _, issue := range issues { + var has bool + for _, repoID := range repoIDs { + if repoID == issue.RepoID { + has = true + break + } + } + if !has { + repoIDs = append(repoIDs, issue.RepoID) + } + } + return repoIDs +} + +func (issues IssueList) loadRepositories(e Engine) ([]*Repository, error) { + if len(issues) == 0 { + return nil, nil + } + + repoIDs := issues.getRepoIDs() + rows, err := e. + Where("id > 0"). + In("id", repoIDs). + Rows(new(Repository)) + if err != nil { + return nil, fmt.Errorf("find repository: %v", err) + } + defer rows.Close() + + repositories := make([]*Repository, 0, len(repoIDs)) + repoMaps := make(map[int64]*Repository, len(repoIDs)) + for rows.Next() { + var repo Repository + err = rows.Scan(&repo) + if err != nil { + return nil, fmt.Errorf("find repository: %v", err) + } + + repositories = append(repositories, &repo) + repoMaps[repo.ID] = &repo + } + + for _, issue := range issues { + issue.Repo = repoMaps[issue.RepoID] + } + return repositories, nil +} + +// LoadRepositories loads issues' all repositories +func (issues IssueList) LoadRepositories() ([]*Repository, error) { + return issues.loadRepositories(x) +} diff --git a/models/user.go b/models/user.go index 5f0816cd48..c9ddbe7c8b 100644 --- a/models/user.go +++ b/models/user.go @@ -500,6 +500,34 @@ func (u *User) GetRepositories(page, pageSize int) (err error) { return err } +// GetRepositoryIDs returns repositories IDs where user owned +func (u *User) GetRepositoryIDs() ([]int64, error) { + var ids []int64 + return ids, x.Table("repository").Cols("id").Where("owner_id = ?", u.ID).Find(&ids) +} + +// GetOrgRepositoryIDs returns repositories IDs where user's team owned +func (u *User) GetOrgRepositoryIDs() ([]int64, error) { + var ids []int64 + return ids, x.Table("repository"). + Cols("repository.id"). + Join("INNER", "team_user", "repository.owner_id = team_user.org_id AND team_user.uid = ?", u.ID). + GroupBy("repository.id").Find(&ids) +} + +// GetAccessRepoIDs returns all repsitories IDs where user's or user is a team member orgnizations +func (u *User) GetAccessRepoIDs() ([]int64, error) { + ids, err := u.GetRepositoryIDs() + if err != nil { + return nil, err + } + ids2, err := u.GetOrgRepositoryIDs() + if err != nil { + return nil, err + } + return append(ids, ids2...), nil +} + // GetMirrorRepositories returns mirror repositories that user owns, including private repositories. func (u *User) GetMirrorRepositories() ([]*Repository, error) { return GetUserMirrorRepositories(u.ID) diff --git a/routers/user/home.go b/routers/user/home.go index 4aa233b477..868f984b5a 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -214,48 +214,28 @@ func Issues(ctx *context.Context) { // Get repositories. var err error - var repos []*models.Repository - userRepoIDs := make([]int64, 0, len(repos)) + var userRepoIDs []int64 if ctxUser.IsOrganization() { env, err := ctxUser.AccessibleReposEnv(ctx.User.ID) if err != nil { ctx.Handle(500, "AccessibleReposEnv", err) return } - repos, err = env.Repos(1, ctxUser.NumRepos) + userRepoIDs, err = env.RepoIDs(1, ctxUser.NumRepos) if err != nil { - ctx.Handle(500, "GetRepositories", err) + ctx.Handle(500, "env.RepoIDs", err) return } - - for _, repo := range repos { - if (isPullList && repo.NumPulls == 0) || - (!isPullList && - (!repo.EnableUnit(models.UnitTypeIssues) || repo.NumIssues == 0)) { - continue - } - - userRepoIDs = append(userRepoIDs, repo.ID) - } - - if len(userRepoIDs) <= 0 { - userRepoIDs = []int64{-1} - } - } else { - if err := ctxUser.GetRepositories(1, ctx.User.NumRepos); err != nil { - ctx.Handle(500, "GetRepositories", err) + userRepoIDs, err = ctxUser.GetAccessRepoIDs() + if err != nil { + ctx.Handle(500, "ctxUser.GetAccessRepoIDs", err) return } - repos = ctxUser.Repos + } - for _, repo := range repos { - if (isPullList && repo.NumPulls == 0) || - (!isPullList && - (!repo.EnableUnit(models.UnitTypeIssues) || repo.NumIssues == 0)) { - continue - } - } + if len(userRepoIDs) <= 0 { + userRepoIDs = []int64{-1} } var issues []*models.Issue @@ -309,55 +289,41 @@ func Issues(ctx *context.Context) { return } - showRepos := make([]*models.Repository, 0, len(issues)) - showReposSet := make(map[int64]bool) + showRepos, err := models.IssueList(issues).LoadRepositories() + if err != nil { + ctx.Handle(500, "LoadRepositories", fmt.Errorf("%v", err)) + return + } if repoID > 0 { - repo, err := models.GetRepositoryByID(repoID) - if err != nil { - ctx.Handle(500, "GetRepositoryByID", fmt.Errorf("[#%d]%v", repoID, err)) - return + var theRepo *models.Repository + for _, repo := range showRepos { + if repo.ID == repoID { + theRepo = repo + break + } } - if err = repo.GetOwner(); err != nil { - ctx.Handle(500, "GetOwner", fmt.Errorf("[#%d]%v", repoID, err)) - return + if theRepo == nil { + theRepo, err = models.GetRepositoryByID(repoID) + if err != nil { + ctx.Handle(500, "GetRepositoryByID", fmt.Errorf("[#%d]%v", repoID, err)) + return + } + showRepos = append(showRepos, theRepo) } // Check if user has access to given repository. - if !repo.IsOwnedBy(ctxUser.ID) && !repo.HasAccess(ctxUser) { + if !theRepo.IsOwnedBy(ctxUser.ID) && !theRepo.HasAccess(ctxUser) { ctx.Handle(404, "Issues", fmt.Errorf("#%d", repoID)) return } - - showReposSet[repoID] = true - showRepos = append(showRepos, repo) } - for _, issue := range issues { - // Get Repository data. - issue.Repo, err = models.GetRepositoryByID(issue.RepoID) - if err != nil { - ctx.Handle(500, "GetRepositoryByID", fmt.Errorf("[#%d]%v", issue.RepoID, err)) - return - } - - // Get Owner data. - if err = issue.Repo.GetOwner(); err != nil { - ctx.Handle(500, "GetOwner", fmt.Errorf("[#%d]%v", issue.RepoID, err)) - return - } - - // Append repo to list of shown repos - if filterMode == models.FilterModeAll { - // Use a map to make sure we don't add the same Repository twice. - _, ok := showReposSet[issue.RepoID] - if !ok { - showReposSet[issue.RepoID] = true - // Append to list of shown Repositories. - showRepos = append(showRepos, issue.Repo) - } - } + err = models.RepositoryList(showRepos).LoadAttributes() + if err != nil { + ctx.Handle(500, "LoadAttributes", fmt.Errorf("%v", err)) + return } issueStats := models.GetUserIssueStats(repoID, ctxUser.ID, userRepoIDs, filterMode, isPullList) diff --git a/templates/user/dashboard/issues.tmpl b/templates/user/dashboard/issues.tmpl index daf9b26dd1..a146250c18 100644 --- a/templates/user/dashboard/issues.tmpl +++ b/templates/user/dashboard/issues.tmpl @@ -23,7 +23,7 @@ {{range .Repos}} {{.FullName}} -
{{if $.IsShowClosed}}{{.NumClosedIssues}}{{else}}{{.NumOpenIssues}}{{end}}
+
{{if $.IsShowClosed}}{{if $.PageIsPulls}}{{.NumClosedPulls}}{{else}}{{.NumClosedIssues}}{{end}}{{else}}{{if $.PageIsPulls}}{{.NumOpenPulls}}{{else}}{{.NumOpenIssues}}{{end}}{{end}}
 {{end}}