From 7e0654bd9e4f90fc156884afd88cb82ad8df86a8 Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Wed, 2 Aug 2017 22:09:16 -0700 Subject: [PATCH] Fix counts on issues dashboard (#2215) * Fix counts on issues dashboard * setupSess -> setupSession * Unit test * Load repo owners for issues --- models/issue.go | 57 +++++++--- models/issue_indexer.go | 1 - models/main_test.go | 20 +--- models/repo_list.go | 5 + models/unit_tests.go | 18 ++++ modules/test/context_tests.go | 150 +++++++++++++++++++++++++++ routers/api/v1/repo/issue.go | 1 + routers/repo/issue.go | 1 + routers/user/home.go | 112 +++++++++----------- routers/user/home_test.go | 33 ++++++ routers/user/main_test.go | 33 ++++++ templates/user/dashboard/issues.tmpl | 2 +- 12 files changed, 336 insertions(+), 97 deletions(-) create mode 100644 modules/test/context_tests.go create mode 100644 routers/user/home_test.go create mode 100644 routers/user/main_test.go diff --git a/models/issue.go b/models/issue.go index d40b81eb32..709ebc35f9 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1057,6 +1057,7 @@ type IssuesOptions struct { MilestoneID int64 RepoIDs []int64 Page int + PageSize int IsClosed util.OptionalBool IsPull util.OptionalBool Labels string @@ -1085,21 +1086,16 @@ func sortIssuesSession(sess *xorm.Session, sortType string) { } } -// Issues returns a list of issues by given conditions. -func Issues(opts *IssuesOptions) ([]*Issue, error) { - var sess *xorm.Session - if opts.Page >= 0 { +func (opts *IssuesOptions) setupSession(sess *xorm.Session) error { + if opts.Page >= 0 && opts.PageSize > 0 { var start int if opts.Page == 0 { start = 0 } else { - start = (opts.Page - 1) * setting.UI.IssuePagingNum + start = (opts.Page - 1) * opts.PageSize } - sess = x.Limit(setting.UI.IssuePagingNum, start) - } else { - sess = x.NewSession() + sess.Limit(opts.PageSize, start) } - defer sess.Close() if len(opts.IssueIDs) > 0 { sess.In("issue.id", opts.IssueIDs) @@ -1144,12 +1140,10 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) { sess.And("issue.is_pull=?", false) } - sortIssuesSession(sess, opts.SortType) - if len(opts.Labels) > 0 && opts.Labels != "0" { labelIDs, err := base.StringsToInt64s(strings.Split(opts.Labels, ",")) if err != nil { - return nil, err + return err } if len(labelIDs) > 0 { sess. @@ -1157,6 +1151,45 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) { In("issue_label.label_id", labelIDs) } } + return nil +} + +// CountIssuesByRepo map from repoID to number of issues matching the options +func CountIssuesByRepo(opts *IssuesOptions) (map[int64]int64, error) { + sess := x.NewSession() + defer sess.Close() + + if err := opts.setupSession(sess); err != nil { + return nil, err + } + + countsSlice := make([]*struct { + RepoID int64 + Count int64 + }, 0, 10) + if err := sess.GroupBy("issue.repo_id"). + Select("issue.repo_id AS repo_id, COUNT(*) AS count"). + Table("issue"). + Find(&countsSlice); err != nil { + return nil, err + } + + countMap := make(map[int64]int64, len(countsSlice)) + for _, c := range countsSlice { + countMap[c.RepoID] = c.Count + } + return countMap, nil +} + +// Issues returns a list of issues by given conditions. +func Issues(opts *IssuesOptions) ([]*Issue, error) { + sess := x.NewSession() + defer sess.Close() + + if err := opts.setupSession(sess); err != nil { + return nil, err + } + sortIssuesSession(sess, opts.SortType) issues := make([]*Issue, 0, setting.UI.IssuePagingNum) if err := sess.Find(&issues); err != nil { diff --git a/models/issue_indexer.go b/models/issue_indexer.go index c2cb8b429e..05b324f535 100644 --- a/models/issue_indexer.go +++ b/models/issue_indexer.go @@ -133,7 +133,6 @@ func populateIssueIndexer() error { RepoID: repo.ID, IsClosed: util.OptionalBoolNone, IsPull: util.OptionalBoolNone, - Page: -1, // do not page }) if err != nil { return fmt.Errorf("Issues: %v", err) diff --git a/models/main_test.go b/models/main_test.go index 57e72a57fc..451b5e4b21 100644 --- a/models/main_test.go +++ b/models/main_test.go @@ -8,11 +8,8 @@ import ( "code.gitea.io/gitea/modules/setting" - "github.com/go-xorm/core" - "github.com/go-xorm/xorm" _ "github.com/mattn/go-sqlite3" // for the test engine "github.com/stretchr/testify/assert" - "gopkg.in/testfixtures.v2" ) // TestFixturesAreConsistent assert that test fixtures are consistent @@ -21,23 +18,8 @@ func TestFixturesAreConsistent(t *testing.T) { CheckConsistencyForAll(t) } -// CreateTestEngine create an xorm engine for testing -func CreateTestEngine() error { - var err error - x, err = xorm.NewEngine("sqlite3", "file::memory:?cache=shared") - if err != nil { - return err - } - x.SetMapper(core.GonicMapper{}) - if err = x.StoreEngine("InnoDB").Sync2(tables...); err != nil { - return err - } - - return InitFixtures(&testfixtures.SQLite{}, "fixtures/") -} - func TestMain(m *testing.M) { - if err := CreateTestEngine(); err != nil { + if err := CreateTestEngine("fixtures/"); err != nil { fmt.Printf("Error creating test engine: %v\n", err) os.Exit(1) } diff --git a/models/repo_list.go b/models/repo_list.go index a2dae85c84..2158cfe676 100644 --- a/models/repo_list.go +++ b/models/repo_list.go @@ -15,6 +15,11 @@ import ( // RepositoryList contains a list of repositories type RepositoryList []*Repository +// RepositoryListOfMap make list from values of map +func RepositoryListOfMap(repoMap map[int64]*Repository) RepositoryList { + return RepositoryList(valuesRepository(repoMap)) +} + func (repos RepositoryList) loadAttributes(e Engine) error { if len(repos) == 0 { return nil diff --git a/models/unit_tests.go b/models/unit_tests.go index b4b36ba6b7..315627d8e0 100644 --- a/models/unit_tests.go +++ b/models/unit_tests.go @@ -7,13 +7,31 @@ package models import ( "testing" + "github.com/go-xorm/core" "github.com/go-xorm/xorm" "github.com/stretchr/testify/assert" + "gopkg.in/testfixtures.v2" ) // NonexistentID an ID that will never exist const NonexistentID = 9223372036854775807 +// CreateTestEngine create in-memory sqlite database for unit tests +// Any package that calls this must import github.com/mattn/go-sqlite3 +func CreateTestEngine(fixturesDir string) error { + var err error + x, err = xorm.NewEngine("sqlite3", "file::memory:?cache=shared") + if err != nil { + return err + } + x.SetMapper(core.GonicMapper{}) + if err = x.StoreEngine("InnoDB").Sync2(tables...); err != nil { + return err + } + + return InitFixtures(&testfixtures.SQLite{}, fixturesDir) +} + // PrepareTestDatabase load test fixtures into test database func PrepareTestDatabase() error { return LoadFixtures() diff --git a/modules/test/context_tests.go b/modules/test/context_tests.go new file mode 100644 index 0000000000..6bb7ffe987 --- /dev/null +++ b/modules/test/context_tests.go @@ -0,0 +1,150 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package test + +import ( + "net/http" + "net/url" + "testing" + + "code.gitea.io/gitea/modules/context" + + "github.com/stretchr/testify/assert" + macaron "gopkg.in/macaron.v1" +) + +// MockContext mock context for unit tests +func MockContext(t *testing.T) *context.Context { + var macaronContext *macaron.Context + mac := macaron.New() + mac.Get("*/", func(ctx *macaron.Context) { + macaronContext = ctx + }) + req, err := http.NewRequest("GET", "star", nil) + assert.NoError(t, err) + req.Form = url.Values{} + mac.ServeHTTP(&mockResponseWriter{}, req) + assert.NotNil(t, macaronContext) + assert.EqualValues(t, req, macaronContext.Req.Request) + macaronContext.Locale = &mockLocale{} + macaronContext.Resp = &mockResponseWriter{} + macaronContext.Render = &mockRender{ResponseWriter: macaronContext.Resp} + return &context.Context{ + Context: macaronContext, + } +} + +type mockLocale struct{} + +func (l mockLocale) Language() string { + return "en" +} + +func (l mockLocale) Tr(s string, _ ...interface{}) string { + return "test translation" +} + +type mockResponseWriter struct { + status int + size int +} + +func (rw *mockResponseWriter) Header() http.Header { + return map[string][]string{} +} + +func (rw *mockResponseWriter) Write(b []byte) (int, error) { + rw.size += len(b) + return len(b), nil +} + +func (rw *mockResponseWriter) WriteHeader(status int) { + rw.status = status +} + +func (rw *mockResponseWriter) Flush() { +} + +func (rw *mockResponseWriter) Status() int { + return rw.status +} + +func (rw *mockResponseWriter) Written() bool { + return rw.status > 0 +} + +func (rw *mockResponseWriter) Size() int { + return rw.size +} + +func (rw *mockResponseWriter) Before(b macaron.BeforeFunc) { + b(rw) +} + +type mockRender struct { + http.ResponseWriter +} + +func (tr *mockRender) SetResponseWriter(rw http.ResponseWriter) { + tr.ResponseWriter = rw +} + +func (tr *mockRender) JSON(int, interface{}) { +} + +func (tr *mockRender) JSONString(interface{}) (string, error) { + return "", nil +} + +func (tr *mockRender) RawData(status int, _ []byte) { + tr.Status(status) +} + +func (tr *mockRender) PlainText(status int, _ []byte) { + tr.Status(status) +} + +func (tr *mockRender) HTML(status int, _ string, _ interface{}, _ ...macaron.HTMLOptions) { + tr.Status(status) +} + +func (tr *mockRender) HTMLSet(status int, _ string, _ string, _ interface{}, _ ...macaron.HTMLOptions) { + tr.Status(status) +} + +func (tr *mockRender) HTMLSetString(string, string, interface{}, ...macaron.HTMLOptions) (string, error) { + return "", nil +} + +func (tr *mockRender) HTMLString(string, interface{}, ...macaron.HTMLOptions) (string, error) { + return "", nil +} + +func (tr *mockRender) HTMLSetBytes(string, string, interface{}, ...macaron.HTMLOptions) ([]byte, error) { + return nil, nil +} + +func (tr *mockRender) HTMLBytes(string, interface{}, ...macaron.HTMLOptions) ([]byte, error) { + return nil, nil +} + +func (tr *mockRender) XML(status int, _ interface{}) { + tr.Status(status) +} + +func (tr *mockRender) Error(status int, _ ...string) { + tr.Status(status) +} + +func (tr *mockRender) Status(status int) { + tr.ResponseWriter.WriteHeader(status) +} + +func (tr *mockRender) SetTemplatePath(string, string) { +} + +func (tr *mockRender) HasTemplateSet(string) bool { + return true +} diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 253bdb43d2..9f51022f35 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -31,6 +31,7 @@ func ListIssues(ctx *context.APIContext) { issues, err := models.Issues(&models.IssuesOptions{ RepoID: ctx.Repo.Repository.ID, Page: ctx.QueryInt("page"), + PageSize: setting.UI.IssuePagingNum, IsClosed: isClosed, }) if err != nil { diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 473e829713..e4ed10d980 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -193,6 +193,7 @@ func Issues(ctx *context.Context) { MentionedID: mentionedID, MilestoneID: milestoneID, Page: pager.Current(), + PageSize: setting.UI.IssuePagingNum, IsClosed: util.OptionalBoolOf(isShowClosed), IsPull: util.OptionalBoolOf(isPullList), Labels: selectLabels, diff --git a/routers/user/home.go b/routers/user/home.go index 7128890690..b2096bc2ce 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -270,94 +270,77 @@ func Issues(ctx *context.Context) { userRepoIDs = []int64{-1} } - var issues []*models.Issue - switch filterMode { - case models.FilterModeAll: - // Get all issues from repositories from this user. - issues, err = models.Issues(&models.IssuesOptions{ - RepoIDs: userRepoIDs, - RepoID: repoID, - Page: page, - IsClosed: util.OptionalBoolOf(isShowClosed), - IsPull: util.OptionalBoolOf(isPullList), - SortType: sortType, - }) - - case models.FilterModeAssign: - // Get all issues assigned to this user. - issues, err = models.Issues(&models.IssuesOptions{ - RepoID: repoID, - AssigneeID: ctxUser.ID, - Page: page, - IsClosed: util.OptionalBoolOf(isShowClosed), - IsPull: util.OptionalBoolOf(isPullList), - SortType: sortType, - }) - - case models.FilterModeCreate: - // Get all issues created by this user. - issues, err = models.Issues(&models.IssuesOptions{ - RepoID: repoID, - PosterID: ctxUser.ID, - Page: page, - IsClosed: util.OptionalBoolOf(isShowClosed), - IsPull: util.OptionalBoolOf(isPullList), - SortType: sortType, - }) - case models.FilterModeMention: - // Get all issues created by this user. - issues, err = models.Issues(&models.IssuesOptions{ - RepoID: repoID, - MentionedID: ctxUser.ID, - Page: page, - IsClosed: util.OptionalBoolOf(isShowClosed), - IsPull: util.OptionalBoolOf(isPullList), - SortType: sortType, - }) + opts := &models.IssuesOptions{ + RepoID: repoID, + IsClosed: util.OptionalBoolOf(isShowClosed), + IsPull: util.OptionalBoolOf(isPullList), + SortType: sortType, } + switch filterMode { + case models.FilterModeAll: + opts.RepoIDs = userRepoIDs + case models.FilterModeAssign: + opts.AssigneeID = ctxUser.ID + case models.FilterModeCreate: + opts.PosterID = ctxUser.ID + case models.FilterModeMention: + opts.MentionedID = ctxUser.ID + } + + counts, err := models.CountIssuesByRepo(opts) + if err != nil { + ctx.Handle(500, "CountIssuesByRepo", err) + return + } + + opts.Page = page + opts.PageSize = setting.UI.IssuePagingNum + issues, err := models.Issues(opts) if err != nil { ctx.Handle(500, "Issues", err) return } - showRepos, err := models.IssueList(issues).LoadRepositories() - if err != nil { - ctx.Handle(500, "LoadRepositories", fmt.Errorf("%v", err)) - return + showReposMap := make(map[int64]*models.Repository, len(counts)) + for repoID := range counts { + repo, err := models.GetRepositoryByID(repoID) + if err != nil { + ctx.Handle(500, "GetRepositoryByID", err) + return + } + showReposMap[repoID] = repo } if repoID > 0 { - var theRepo *models.Repository - for _, repo := range showRepos { - if repo.ID == repoID { - theRepo = repo - break - } - } - - if theRepo == nil { - theRepo, err = models.GetRepositoryByID(repoID) + if _, ok := showReposMap[repoID]; !ok { + repo, err := models.GetRepositoryByID(repoID) if err != nil { - ctx.Handle(500, "GetRepositoryByID", fmt.Errorf("[#%d]%v", repoID, err)) + ctx.Handle(500, "GetRepositoryByID", fmt.Errorf("[%d]%v", repoID, err)) return } - showRepos = append(showRepos, theRepo) + showReposMap[repoID] = repo } + repo := showReposMap[repoID] + // Check if user has access to given repository. - if !theRepo.IsOwnedBy(ctxUser.ID) && !theRepo.HasAccess(ctxUser) { - ctx.Handle(404, "Issues", fmt.Errorf("#%d", repoID)) + if !repo.IsOwnedBy(ctxUser.ID) && !repo.HasAccess(ctxUser) { + ctx.Status(404) return } } - err = models.RepositoryList(showRepos).LoadAttributes() - if err != nil { + showRepos := models.RepositoryListOfMap(showReposMap) + if err = showRepos.LoadAttributes(); err != nil { ctx.Handle(500, "LoadAttributes", fmt.Errorf("%v", err)) return } + for _, issue := range issues { + issue.Repo = showReposMap[issue.RepoID] + } + issueStats := models.GetUserIssueStats(repoID, ctxUser.ID, userRepoIDs, filterMode, isPullList) var total int @@ -369,6 +352,7 @@ func Issues(ctx *context.Context) { ctx.Data["Issues"] = issues ctx.Data["Repos"] = showRepos + ctx.Data["Counts"] = counts ctx.Data["Page"] = paginater.New(total, setting.UI.IssuePagingNum, page, 5) ctx.Data["IssueStats"] = issueStats ctx.Data["ViewType"] = viewType diff --git a/routers/user/home_test.go b/routers/user/home_test.go new file mode 100644 index 0000000000..beca936174 --- /dev/null +++ b/routers/user/home_test.go @@ -0,0 +1,33 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package user + +import ( + "net/http" + "testing" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/test" + + "code.gitea.io/gitea/modules/setting" + "github.com/stretchr/testify/assert" +) + +func TestIssues(t *testing.T) { + setting.UI.IssuePagingNum = 1 + assert.NoError(t, models.LoadFixtures()) + + ctx := test.MockContext(t) + ctx.User = models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) + ctx.SetParams(":type", "issues") + ctx.Req.Form.Set("state", "closed") + Issues(ctx) + assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) + + assert.EqualValues(t, map[int64]int64{1: 1, 2: 1}, ctx.Data["Counts"]) + assert.EqualValues(t, true, ctx.Data["IsShowClosed"]) + assert.Len(t, ctx.Data["Issues"], 1) + assert.Len(t, ctx.Data["Repos"], 2) +} diff --git a/routers/user/main_test.go b/routers/user/main_test.go new file mode 100644 index 0000000000..83c0c65474 --- /dev/null +++ b/routers/user/main_test.go @@ -0,0 +1,33 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package user + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/setting" + + _ "github.com/mattn/go-sqlite3" // for the test engine +) + +func TestMain(m *testing.M) { + if err := models.CreateTestEngine("../../models/fixtures/"); err != nil { + fmt.Printf("Error creating test engine: %v\n", err) + os.Exit(1) + } + + setting.AppURL = "https://try.gitea.io/" + setting.RunUser = "runuser" + setting.SSH.Port = 3000 + setting.SSH.Domain = "try.gitea.io" + setting.RepoRootPath = filepath.Join(os.TempDir(), "repos") + setting.AppDataPath = filepath.Join(os.TempDir(), "appdata") + + os.Exit(m.Run()) +} diff --git a/templates/user/dashboard/issues.tmpl b/templates/user/dashboard/issues.tmpl index a146250c18..954b868214 100644 --- a/templates/user/dashboard/issues.tmpl +++ b/templates/user/dashboard/issues.tmpl @@ -23,7 +23,7 @@ {{range .Repos}} {{.FullName}} -
{{if $.IsShowClosed}}{{if $.PageIsPulls}}{{.NumClosedPulls}}{{else}}{{.NumClosedIssues}}{{end}}{{else}}{{if $.PageIsPulls}}{{.NumOpenPulls}}{{else}}{{.NumOpenIssues}}{{end}}{{end}}
+
{{index $.Counts .ID}}
 {{end}}