Browse Source

Fix counts on issues dashboard (#2215)

* Fix counts on issues dashboard

* setupSess -> setupSession

* Unit test

* Load repo owners for issues
Ethan Koenig 3 years ago
parent
commit
7e0654bd9e

+ 45 - 12
models/issue.go

@@ -1057,6 +1057,7 @@ type IssuesOptions struct {
1057 1057
 	MilestoneID int64
1058 1058
 	RepoIDs     []int64
1059 1059
 	Page        int
1060
+	PageSize    int
1060 1061
 	IsClosed    util.OptionalBool
1061 1062
 	IsPull      util.OptionalBool
1062 1063
 	Labels      string
@@ -1085,21 +1086,16 @@ func sortIssuesSession(sess *xorm.Session, sortType string) {
1085 1086
 	}
1086 1087
 }
1087 1088
 
1088
-// Issues returns a list of issues by given conditions.
1089
-func Issues(opts *IssuesOptions) ([]*Issue, error) {
1090
-	var sess *xorm.Session
1091
-	if opts.Page >= 0 {
1089
+func (opts *IssuesOptions) setupSession(sess *xorm.Session) error {
1090
+	if opts.Page >= 0 && opts.PageSize > 0 {
1092 1091
 		var start int
1093 1092
 		if opts.Page == 0 {
1094 1093
 			start = 0
1095 1094
 		} else {
1096
-			start = (opts.Page - 1) * setting.UI.IssuePagingNum
1095
+			start = (opts.Page - 1) * opts.PageSize
1097 1096
 		}
1098
-		sess = x.Limit(setting.UI.IssuePagingNum, start)
1099
-	} else {
1100
-		sess = x.NewSession()
1097
+		sess.Limit(opts.PageSize, start)
1101 1098
 	}
1102
-	defer sess.Close()
1103 1099
 
1104 1100
 	if len(opts.IssueIDs) > 0 {
1105 1101
 		sess.In("issue.id", opts.IssueIDs)
@@ -1144,12 +1140,10 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) {
1144 1140
 		sess.And("issue.is_pull=?", false)
1145 1141
 	}
1146 1142
 
1147
-	sortIssuesSession(sess, opts.SortType)
1148
-
1149 1143
 	if len(opts.Labels) > 0 && opts.Labels != "0" {
1150 1144
 		labelIDs, err := base.StringsToInt64s(strings.Split(opts.Labels, ","))
1151 1145
 		if err != nil {
1152
-			return nil, err
1146
+			return err
1153 1147
 		}
1154 1148
 		if len(labelIDs) > 0 {
1155 1149
 			sess.
@@ -1157,6 +1151,45 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) {
1157 1151
 				In("issue_label.label_id", labelIDs)
1158 1152
 		}
1159 1153
 	}
1154
+	return nil
1155
+}
1156
+
1157
+// CountIssuesByRepo map from repoID to number of issues matching the options
1158
+func CountIssuesByRepo(opts *IssuesOptions) (map[int64]int64, error) {
1159
+	sess := x.NewSession()
1160
+	defer sess.Close()
1161
+
1162
+	if err := opts.setupSession(sess); err != nil {
1163
+		return nil, err
1164
+	}
1165
+
1166
+	countsSlice := make([]*struct {
1167
+		RepoID int64
1168
+		Count  int64
1169
+	}, 0, 10)
1170
+	if err := sess.GroupBy("issue.repo_id").
1171
+		Select("issue.repo_id AS repo_id, COUNT(*) AS count").
1172
+		Table("issue").
1173
+		Find(&countsSlice); err != nil {
1174
+		return nil, err
1175
+	}
1176
+
1177
+	countMap := make(map[int64]int64, len(countsSlice))
1178
+	for _, c := range countsSlice {
1179
+		countMap[c.RepoID] = c.Count
1180
+	}
1181
+	return countMap, nil
1182
+}
1183
+
1184
+// Issues returns a list of issues by given conditions.
1185
+func Issues(opts *IssuesOptions) ([]*Issue, error) {
1186
+	sess := x.NewSession()
1187
+	defer sess.Close()
1188
+
1189
+	if err := opts.setupSession(sess); err != nil {
1190
+		return nil, err
1191
+	}
1192
+	sortIssuesSession(sess, opts.SortType)
1160 1193
 
1161 1194
 	issues := make([]*Issue, 0, setting.UI.IssuePagingNum)
1162 1195
 	if err := sess.Find(&issues); err != nil {

+ 0 - 1
models/issue_indexer.go

@@ -133,7 +133,6 @@ func populateIssueIndexer() error {
133 133
 				RepoID:   repo.ID,
134 134
 				IsClosed: util.OptionalBoolNone,
135 135
 				IsPull:   util.OptionalBoolNone,
136
-				Page:     -1, // do not page
137 136
 			})
138 137
 			if err != nil {
139 138
 				return fmt.Errorf("Issues: %v", err)

+ 1 - 19
models/main_test.go

@@ -8,11 +8,8 @@ import (
8 8
 
9 9
 	"code.gitea.io/gitea/modules/setting"
10 10
 
11
-	"github.com/go-xorm/core"
12
-	"github.com/go-xorm/xorm"
13 11
 	_ "github.com/mattn/go-sqlite3" // for the test engine
14 12
 	"github.com/stretchr/testify/assert"
15
-	"gopkg.in/testfixtures.v2"
16 13
 )
17 14
 
18 15
 // TestFixturesAreConsistent assert that test fixtures are consistent
@@ -21,23 +18,8 @@ func TestFixturesAreConsistent(t *testing.T) {
21 18
 	CheckConsistencyForAll(t)
22 19
 }
23 20
 
24
-// CreateTestEngine create an xorm engine for testing
25
-func CreateTestEngine() error {
26
-	var err error
27
-	x, err = xorm.NewEngine("sqlite3", "file::memory:?cache=shared")
28
-	if err != nil {
29
-		return err
30
-	}
31
-	x.SetMapper(core.GonicMapper{})
32
-	if err = x.StoreEngine("InnoDB").Sync2(tables...); err != nil {
33
-		return err
34
-	}
35
-
36
-	return InitFixtures(&testfixtures.SQLite{}, "fixtures/")
37
-}
38
-
39 21
 func TestMain(m *testing.M) {
40
-	if err := CreateTestEngine(); err != nil {
22
+	if err := CreateTestEngine("fixtures/"); err != nil {
41 23
 		fmt.Printf("Error creating test engine: %v\n", err)
42 24
 		os.Exit(1)
43 25
 	}

+ 5 - 0
models/repo_list.go

@@ -15,6 +15,11 @@ import (
15 15
 // RepositoryList contains a list of repositories
16 16
 type RepositoryList []*Repository
17 17
 
18
+// RepositoryListOfMap make list from values of map
19
+func RepositoryListOfMap(repoMap map[int64]*Repository) RepositoryList {
20
+	return RepositoryList(valuesRepository(repoMap))
21
+}
22
+
18 23
 func (repos RepositoryList) loadAttributes(e Engine) error {
19 24
 	if len(repos) == 0 {
20 25
 		return nil

+ 18 - 0
models/unit_tests.go

@@ -7,13 +7,31 @@ package models
7 7
 import (
8 8
 	"testing"
9 9
 
10
+	"github.com/go-xorm/core"
10 11
 	"github.com/go-xorm/xorm"
11 12
 	"github.com/stretchr/testify/assert"
13
+	"gopkg.in/testfixtures.v2"
12 14
 )
13 15
 
14 16
 // NonexistentID an ID that will never exist
15 17
 const NonexistentID = 9223372036854775807
16 18
 
19
+// CreateTestEngine create in-memory sqlite database for unit tests
20
+// Any package that calls this must import github.com/mattn/go-sqlite3
21
+func CreateTestEngine(fixturesDir string) error {
22
+	var err error
23
+	x, err = xorm.NewEngine("sqlite3", "file::memory:?cache=shared")
24
+	if err != nil {
25
+		return err
26
+	}
27
+	x.SetMapper(core.GonicMapper{})
28
+	if err = x.StoreEngine("InnoDB").Sync2(tables...); err != nil {
29
+		return err
30
+	}
31
+
32
+	return InitFixtures(&testfixtures.SQLite{}, fixturesDir)
33
+}
34
+
17 35
 // PrepareTestDatabase load test fixtures into test database
18 36
 func PrepareTestDatabase() error {
19 37
 	return LoadFixtures()

+ 150 - 0
modules/test/context_tests.go

@@ -0,0 +1,150 @@
1
+// Copyright 2017 The Gitea Authors. All rights reserved.
2
+// Use of this source code is governed by a MIT-style
3
+// license that can be found in the LICENSE file.
4
+
5
+package test
6
+
7
+import (
8
+	"net/http"
9
+	"net/url"
10
+	"testing"
11
+
12
+	"code.gitea.io/gitea/modules/context"
13
+
14
+	"github.com/stretchr/testify/assert"
15
+	macaron "gopkg.in/macaron.v1"
16
+)
17
+
18
+// MockContext mock context for unit tests
19
+func MockContext(t *testing.T) *context.Context {
20
+	var macaronContext *macaron.Context
21
+	mac := macaron.New()
22
+	mac.Get("*/", func(ctx *macaron.Context) {
23
+		macaronContext = ctx
24
+	})
25
+	req, err := http.NewRequest("GET", "star", nil)
26
+	assert.NoError(t, err)
27
+	req.Form = url.Values{}
28
+	mac.ServeHTTP(&mockResponseWriter{}, req)
29
+	assert.NotNil(t, macaronContext)
30
+	assert.EqualValues(t, req, macaronContext.Req.Request)
31
+	macaronContext.Locale = &mockLocale{}
32
+	macaronContext.Resp = &mockResponseWriter{}
33
+	macaronContext.Render = &mockRender{ResponseWriter: macaronContext.Resp}
34
+	return &context.Context{
35
+		Context: macaronContext,
36
+	}
37
+}
38
+
39
+type mockLocale struct{}
40
+
41
+func (l mockLocale) Language() string {
42
+	return "en"
43
+}
44
+
45
+func (l mockLocale) Tr(s string, _ ...interface{}) string {
46
+	return "test translation"
47
+}
48
+
49
+type mockResponseWriter struct {
50
+	status int
51
+	size   int
52
+}
53
+
54
+func (rw *mockResponseWriter) Header() http.Header {
55
+	return map[string][]string{}
56
+}
57
+
58
+func (rw *mockResponseWriter) Write(b []byte) (int, error) {
59
+	rw.size += len(b)
60
+	return len(b), nil
61
+}
62
+
63
+func (rw *mockResponseWriter) WriteHeader(status int) {
64
+	rw.status = status
65
+}
66
+
67
+func (rw *mockResponseWriter) Flush() {
68
+}
69
+
70
+func (rw *mockResponseWriter) Status() int {
71
+	return rw.status
72
+}
73
+
74
+func (rw *mockResponseWriter) Written() bool {
75
+	return rw.status > 0
76
+}
77
+
78
+func (rw *mockResponseWriter) Size() int {
79
+	return rw.size
80
+}
81
+
82
+func (rw *mockResponseWriter) Before(b macaron.BeforeFunc) {
83
+	b(rw)
84
+}
85
+
86
+type mockRender struct {
87
+	http.ResponseWriter
88
+}
89
+
90
+func (tr *mockRender) SetResponseWriter(rw http.ResponseWriter) {
91
+	tr.ResponseWriter = rw
92
+}
93
+
94
+func (tr *mockRender) JSON(int, interface{}) {
95
+}
96
+
97
+func (tr *mockRender) JSONString(interface{}) (string, error) {
98
+	return "", nil
99
+}
100
+
101
+func (tr *mockRender) RawData(status int, _ []byte) {
102
+	tr.Status(status)
103
+}
104
+
105
+func (tr *mockRender) PlainText(status int, _ []byte) {
106
+	tr.Status(status)
107
+}
108
+
109
+func (tr *mockRender) HTML(status int, _ string, _ interface{}, _ ...macaron.HTMLOptions) {
110
+	tr.Status(status)
111
+}
112
+
113
+func (tr *mockRender) HTMLSet(status int, _ string, _ string, _ interface{}, _ ...macaron.HTMLOptions) {
114
+	tr.Status(status)
115
+}
116
+
117
+func (tr *mockRender) HTMLSetString(string, string, interface{}, ...macaron.HTMLOptions) (string, error) {
118
+	return "", nil
119
+}
120
+
121
+func (tr *mockRender) HTMLString(string, interface{}, ...macaron.HTMLOptions) (string, error) {
122
+	return "", nil
123
+}
124
+
125
+func (tr *mockRender) HTMLSetBytes(string, string, interface{}, ...macaron.HTMLOptions) ([]byte, error) {
126
+	return nil, nil
127
+}
128
+
129
+func (tr *mockRender) HTMLBytes(string, interface{}, ...macaron.HTMLOptions) ([]byte, error) {
130
+	return nil, nil
131
+}
132
+
133
+func (tr *mockRender) XML(status int, _ interface{}) {
134
+	tr.Status(status)
135
+}
136
+
137
+func (tr *mockRender) Error(status int, _ ...string) {
138
+	tr.Status(status)
139
+}
140
+
141
+func (tr *mockRender) Status(status int) {
142
+	tr.ResponseWriter.WriteHeader(status)
143
+}
144
+
145
+func (tr *mockRender) SetTemplatePath(string, string) {
146
+}
147
+
148
+func (tr *mockRender) HasTemplateSet(string) bool {
149
+	return true
150
+}

+ 1 - 0
routers/api/v1/repo/issue.go

@@ -31,6 +31,7 @@ func ListIssues(ctx *context.APIContext) {
31 31
 	issues, err := models.Issues(&models.IssuesOptions{
32 32
 		RepoID:   ctx.Repo.Repository.ID,
33 33
 		Page:     ctx.QueryInt("page"),
34
+		PageSize: setting.UI.IssuePagingNum,
34 35
 		IsClosed: isClosed,
35 36
 	})
36 37
 	if err != nil {

+ 1 - 0
routers/repo/issue.go

@@ -193,6 +193,7 @@ func Issues(ctx *context.Context) {
193 193
 			MentionedID: mentionedID,
194 194
 			MilestoneID: milestoneID,
195 195
 			Page:        pager.Current(),
196
+			PageSize:    setting.UI.IssuePagingNum,
196 197
 			IsClosed:    util.OptionalBoolOf(isShowClosed),
197 198
 			IsPull:      util.OptionalBoolOf(isPullList),
198 199
 			Labels:      selectLabels,

+ 41 - 57
routers/user/home.go

@@ -270,94 +270,77 @@ func Issues(ctx *context.Context) {
270 270
 		userRepoIDs = []int64{-1}
271 271
 	}
272 272
 
273
-	var issues []*models.Issue
273
+	opts := &models.IssuesOptions{
274
+		RepoID:   repoID,
275
+		IsClosed: util.OptionalBoolOf(isShowClosed),
276
+		IsPull:   util.OptionalBoolOf(isPullList),
277
+		SortType: sortType,
278
+	}
279
+
274 280
 	switch filterMode {
275 281
 	case models.FilterModeAll:
276
-		// Get all issues from repositories from this user.
277
-		issues, err = models.Issues(&models.IssuesOptions{
278
-			RepoIDs:  userRepoIDs,
279
-			RepoID:   repoID,
280
-			Page:     page,
281
-			IsClosed: util.OptionalBoolOf(isShowClosed),
282
-			IsPull:   util.OptionalBoolOf(isPullList),
283
-			SortType: sortType,
284
-		})
285
-
282
+		opts.RepoIDs = userRepoIDs
286 283
 	case models.FilterModeAssign:
287
-		// Get all issues assigned to this user.
288
-		issues, err = models.Issues(&models.IssuesOptions{
289
-			RepoID:     repoID,
290
-			AssigneeID: ctxUser.ID,
291
-			Page:       page,
292
-			IsClosed:   util.OptionalBoolOf(isShowClosed),
293
-			IsPull:     util.OptionalBoolOf(isPullList),
294
-			SortType:   sortType,
295
-		})
296
-
284
+		opts.AssigneeID = ctxUser.ID
297 285
 	case models.FilterModeCreate:
298
-		// Get all issues created by this user.
299
-		issues, err = models.Issues(&models.IssuesOptions{
300
-			RepoID:   repoID,
301
-			PosterID: ctxUser.ID,
302
-			Page:     page,
303
-			IsClosed: util.OptionalBoolOf(isShowClosed),
304
-			IsPull:   util.OptionalBoolOf(isPullList),
305
-			SortType: sortType,
306
-		})
286
+		opts.PosterID = ctxUser.ID
307 287
 	case models.FilterModeMention:
308
-		// Get all issues created by this user.
309
-		issues, err = models.Issues(&models.IssuesOptions{
310
-			RepoID:      repoID,
311
-			MentionedID: ctxUser.ID,
312
-			Page:        page,
313
-			IsClosed:    util.OptionalBoolOf(isShowClosed),
314
-			IsPull:      util.OptionalBoolOf(isPullList),
315
-			SortType:    sortType,
316
-		})
288
+		opts.MentionedID = ctxUser.ID
317 289
 	}
318 290
 
291
+	counts, err := models.CountIssuesByRepo(opts)
319 292
 	if err != nil {
320
-		ctx.Handle(500, "Issues", err)
293
+		ctx.Handle(500, "CountIssuesByRepo", err)
321 294
 		return
322 295
 	}
323 296
 
324
-	showRepos, err := models.IssueList(issues).LoadRepositories()
297
+	opts.Page = page
298
+	opts.PageSize = setting.UI.IssuePagingNum
299
+	issues, err := models.Issues(opts)
325 300
 	if err != nil {
326
-		ctx.Handle(500, "LoadRepositories", fmt.Errorf("%v", err))
301
+		ctx.Handle(500, "Issues", err)
327 302
 		return
328 303
 	}
329 304
 
330
-	if repoID > 0 {
331
-		var theRepo *models.Repository
332
-		for _, repo := range showRepos {
333
-			if repo.ID == repoID {
334
-				theRepo = repo
335
-				break
336
-			}
305
+	showReposMap := make(map[int64]*models.Repository, len(counts))
306
+	for repoID := range counts {
307
+		repo, err := models.GetRepositoryByID(repoID)
308
+		if err != nil {
309
+			ctx.Handle(500, "GetRepositoryByID", err)
310
+			return
337 311
 		}
312
+		showReposMap[repoID] = repo
313
+	}
338 314
 
339
-		if theRepo == nil {
340
-			theRepo, err = models.GetRepositoryByID(repoID)
315
+	if repoID > 0 {
316
+		if _, ok := showReposMap[repoID]; !ok {
317
+			repo, err := models.GetRepositoryByID(repoID)
341 318
 			if err != nil {
342
-				ctx.Handle(500, "GetRepositoryByID", fmt.Errorf("[#%d]%v", repoID, err))
319
+				ctx.Handle(500, "GetRepositoryByID", fmt.Errorf("[%d]%v", repoID, err))
343 320
 				return
344 321
 			}
345
-			showRepos = append(showRepos, theRepo)
322
+			showReposMap[repoID] = repo
346 323
 		}
347 324
 
325
+		repo := showReposMap[repoID]
326
+
348 327
 		// Check if user has access to given repository.
349
-		if !theRepo.IsOwnedBy(ctxUser.ID) && !theRepo.HasAccess(ctxUser) {
350
-			ctx.Handle(404, "Issues", fmt.Errorf("#%d", repoID))
328
+		if !repo.IsOwnedBy(ctxUser.ID) && !repo.HasAccess(ctxUser) {
329
+			ctx.Status(404)
351 330
 			return
352 331
 		}
353 332
 	}
354 333
 
355
-	err = models.RepositoryList(showRepos).LoadAttributes()
356
-	if err != nil {
334
+	showRepos := models.RepositoryListOfMap(showReposMap)
335
+	if err = showRepos.LoadAttributes(); err != nil {
357 336
 		ctx.Handle(500, "LoadAttributes", fmt.Errorf("%v", err))
358 337
 		return
359 338
 	}
360 339
 
340
+	for _, issue := range issues {
341
+		issue.Repo = showReposMap[issue.RepoID]
342
+	}
343
+
361 344
 	issueStats := models.GetUserIssueStats(repoID, ctxUser.ID, userRepoIDs, filterMode, isPullList)
362 345
 
363 346
 	var total int
@@ -369,6 +352,7 @@ func Issues(ctx *context.Context) {
369 352
 
370 353
 	ctx.Data["Issues"] = issues
371 354
 	ctx.Data["Repos"] = showRepos
355
+	ctx.Data["Counts"] = counts
372 356
 	ctx.Data["Page"] = paginater.New(total, setting.UI.IssuePagingNum, page, 5)
373 357
 	ctx.Data["IssueStats"] = issueStats
374 358
 	ctx.Data["ViewType"] = viewType

+ 33 - 0
routers/user/home_test.go

@@ -0,0 +1,33 @@
1
+// Copyright 2017 The Gitea Authors. All rights reserved.
2
+// Use of this source code is governed by a MIT-style
3
+// license that can be found in the LICENSE file.
4
+
5
+package user
6
+
7
+import (
8
+	"net/http"
9
+	"testing"
10
+
11
+	"code.gitea.io/gitea/models"
12
+	"code.gitea.io/gitea/modules/test"
13
+
14
+	"code.gitea.io/gitea/modules/setting"
15
+	"github.com/stretchr/testify/assert"
16
+)
17
+
18
+func TestIssues(t *testing.T) {
19
+	setting.UI.IssuePagingNum = 1
20
+	assert.NoError(t, models.LoadFixtures())
21
+
22
+	ctx := test.MockContext(t)
23
+	ctx.User = models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User)
24
+	ctx.SetParams(":type", "issues")
25
+	ctx.Req.Form.Set("state", "closed")
26
+	Issues(ctx)
27
+	assert.EqualValues(t, http.StatusOK, ctx.Resp.Status())
28
+
29
+	assert.EqualValues(t, map[int64]int64{1: 1, 2: 1}, ctx.Data["Counts"])
30
+	assert.EqualValues(t, true, ctx.Data["IsShowClosed"])
31
+	assert.Len(t, ctx.Data["Issues"], 1)
32
+	assert.Len(t, ctx.Data["Repos"], 2)
33
+}

+ 33 - 0
routers/user/main_test.go

@@ -0,0 +1,33 @@
1
+// Copyright 2017 The Gitea Authors. All rights reserved.
2
+// Use of this source code is governed by a MIT-style
3
+// license that can be found in the LICENSE file.
4
+
5
+package user
6
+
7
+import (
8
+	"fmt"
9
+	"os"
10
+	"path/filepath"
11
+	"testing"
12
+
13
+	"code.gitea.io/gitea/models"
14
+	"code.gitea.io/gitea/modules/setting"
15
+
16
+	_ "github.com/mattn/go-sqlite3" // for the test engine
17
+)
18
+
19
+func TestMain(m *testing.M) {
20
+	if err := models.CreateTestEngine("../../models/fixtures/"); err != nil {
21
+		fmt.Printf("Error creating test engine: %v\n", err)
22
+		os.Exit(1)
23
+	}
24
+
25
+	setting.AppURL = "https://try.gitea.io/"
26
+	setting.RunUser = "runuser"
27
+	setting.SSH.Port = 3000
28
+	setting.SSH.Domain = "try.gitea.io"
29
+	setting.RepoRootPath = filepath.Join(os.TempDir(), "repos")
30
+	setting.AppDataPath = filepath.Join(os.TempDir(), "appdata")
31
+
32
+	os.Exit(m.Run())
33
+}

+ 1 - 1
templates/user/dashboard/issues.tmpl

@@ -23,7 +23,7 @@
23 23
 					{{range .Repos}}
24 24
 						<a class="{{if eq $.RepoID .ID}}ui basic blue button{{end}} repo name item" href="{{$.Link}}?type={{$.ViewType}}{{if not (eq $.RepoID .ID)}}&repo={{.ID}}{{end}}&sort={{$.SortType}}&state={{$.State}}">
25 25
 							<span class="text truncate">{{.FullName}}</span>
26
-							<div class="floating ui {{if $.IsShowClosed}}red{{else}}green{{end}} label">{{if $.IsShowClosed}}{{if $.PageIsPulls}}{{.NumClosedPulls}}{{else}}{{.NumClosedIssues}}{{end}}{{else}}{{if $.PageIsPulls}}{{.NumOpenPulls}}{{else}}{{.NumOpenIssues}}{{end}}{{end}}</div>
26
+							<div class="floating ui {{if $.IsShowClosed}}red{{else}}green{{end}} label">{{index $.Counts .ID}}</div>
27 27
 						</a>
28 28
 					{{end}}
29 29
 				</div>