From 4c9341f689e840df30db3e227498a30fdb5b88ef Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Mon, 25 Dec 2017 18:25:16 -0500 Subject: [PATCH] Fix bugs in issue dashboard stats (#3073) --- models/fixtures/issue.yml | 7 +- models/issue.go | 122 ++++++++++++++++++++++------------- models/issue_indexer.go | 2 +- models/issue_test.go | 111 +++++++++++++++++++++++++++++++ routers/api/v1/repo/issue.go | 2 +- routers/repo/issue.go | 2 +- routers/user/home.go | 33 ++++++++-- 7 files changed, 224 insertions(+), 55 deletions(-) diff --git a/models/fixtures/issue.yml b/models/fixtures/issue.yml index 2592ad2de0..ff514e706c 100644 --- a/models/fixtures/issue.yml +++ b/models/fixtures/issue.yml @@ -47,6 +47,8 @@ content: content for the fourth issue is_closed: true is_pull: false + created_unix: 946684830 + updated_unix: 978307200 - id: 5 @@ -57,6 +59,9 @@ content: content for the fifth issue is_closed: true is_pull: false + created_unix: 946684840 + updated_unix: 978307200 + - id: 6 repo_id: 3 @@ -68,5 +73,5 @@ is_closed: false is_pull: false num_comments: 0 - created_unix: 946684800 + created_unix: 946684850 updated_unix: 978307200 diff --git a/models/issue.go b/models/issue.go index 984e6e31cf..13973b7d1c 100644 --- a/models/issue.go +++ b/models/issue.go @@ -10,14 +10,15 @@ import ( "sort" "strings" - api "code.gitea.io/sdk/gitea" - "github.com/Unknwon/com" - "github.com/go-xorm/xorm" - "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" + api "code.gitea.io/sdk/gitea" + + "github.com/Unknwon/com" + "github.com/go-xorm/builder" + "github.com/go-xorm/xorm" ) // Issue represents an issue or pull request of repository. @@ -1022,12 +1023,11 @@ func GetIssuesByIDs(issueIDs []int64) ([]*Issue, error) { // IssuesOptions represents options of an issue. type IssuesOptions struct { - RepoID int64 + RepoIDs []int64 // include all repos if empty AssigneeID int64 PosterID int64 MentionedID int64 MilestoneID int64 - RepoIDs []int64 Page int PageSize int IsClosed util.OptionalBool @@ -1073,9 +1073,7 @@ func (opts *IssuesOptions) setupSession(sess *xorm.Session) error { sess.In("issue.id", opts.IssueIDs) } - if opts.RepoID > 0 { - sess.And("issue.repo_id=?", opts.RepoID) - } else if len(opts.RepoIDs) > 0 { + if len(opts.RepoIDs) > 0 { // In case repository IDs are provided but actually no repository has issue. sess.In("issue.repo_id", opts.RepoIDs) } @@ -1339,58 +1337,92 @@ func GetIssueStats(opts *IssueStatsOptions) (*IssueStats, error) { return stats, err } +// UserIssueStatsOptions contains parameters accepted by GetUserIssueStats. +type UserIssueStatsOptions struct { + UserID int64 + RepoID int64 + UserRepoIDs []int64 + FilterMode int + IsPull bool + IsClosed bool +} + // GetUserIssueStats returns issue statistic information for dashboard by given conditions. -func GetUserIssueStats(repoID, uid int64, repoIDs []int64, filterMode int, isPull bool) *IssueStats { +func GetUserIssueStats(opts UserIssueStatsOptions) (*IssueStats, error) { + var err error stats := &IssueStats{} - countSession := func(isClosed, isPull bool, repoID int64, repoIDs []int64) *xorm.Session { - sess := x. - Where("issue.is_closed = ?", isClosed). - And("issue.is_pull = ?", isPull) - - if repoID > 0 { - sess.And("repo_id = ?", repoID) - } else if len(repoIDs) > 0 { - sess.In("repo_id", repoIDs) - } - - return sess + cond := builder.NewCond() + cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull}) + if opts.RepoID > 0 { + cond = cond.And(builder.Eq{"issue.repo_id": opts.RepoID}) } - stats.AssignCount, _ = countSession(false, isPull, repoID, nil). - And("assignee_id = ?", uid). - Count(new(Issue)) - - stats.CreateCount, _ = countSession(false, isPull, repoID, nil). - And("poster_id = ?", uid). - Count(new(Issue)) - - stats.YourRepositoriesCount, _ = countSession(false, isPull, repoID, repoIDs). - Count(new(Issue)) - - switch filterMode { + switch opts.FilterMode { case FilterModeAll: - stats.OpenCount, _ = countSession(false, isPull, repoID, repoIDs). + stats.OpenCount, err = x.Where(cond).And("is_closed = ?", false). + And(builder.In("issue.repo_id", opts.UserRepoIDs)). Count(new(Issue)) - stats.ClosedCount, _ = countSession(true, isPull, repoID, repoIDs). + if err != nil { + return nil, err + } + stats.ClosedCount, err = x.Where(cond).And("is_closed = ?", true). + And(builder.In("issue.repo_id", opts.UserRepoIDs)). Count(new(Issue)) + if err != nil { + return nil, err + } case FilterModeAssign: - stats.OpenCount, _ = countSession(false, isPull, repoID, nil). - And("assignee_id = ?", uid). + stats.OpenCount, err = x.Where(cond).And("is_closed = ?", false). + And("assignee_id = ?", opts.UserID). Count(new(Issue)) - stats.ClosedCount, _ = countSession(true, isPull, repoID, nil). - And("assignee_id = ?", uid). + if err != nil { + return nil, err + } + stats.ClosedCount, err = x.Where(cond).And("is_closed = ?", true). + And("assignee_id = ?", opts.UserID). Count(new(Issue)) + if err != nil { + return nil, err + } case FilterModeCreate: - stats.OpenCount, _ = countSession(false, isPull, repoID, nil). - And("poster_id = ?", uid). + stats.OpenCount, err = x.Where(cond).And("is_closed = ?", false). + And("poster_id = ?", opts.UserID). Count(new(Issue)) - stats.ClosedCount, _ = countSession(true, isPull, repoID, nil). - And("poster_id = ?", uid). + if err != nil { + return nil, err + } + stats.ClosedCount, err = x.Where(cond).And("is_closed = ?", true). + And("poster_id = ?", opts.UserID). Count(new(Issue)) + if err != nil { + return nil, err + } } - return stats + cond = cond.And(builder.Eq{"issue.is_closed": opts.IsClosed}) + stats.AssignCount, err = x.Where(cond). + And("assignee_id = ?", opts.UserID). + Count(new(Issue)) + if err != nil { + return nil, err + } + + stats.CreateCount, err = x.Where(cond). + And("poster_id = ?", opts.UserID). + Count(new(Issue)) + if err != nil { + return nil, err + } + + stats.YourRepositoriesCount, err = x.Where(cond). + And(builder.In("issue.repo_id", opts.UserRepoIDs)). + Count(new(Issue)) + if err != nil { + return nil, err + } + + return stats, nil } // GetRepoIssueStats returns number of open and closed repository issues by given filter mode. diff --git a/models/issue_indexer.go b/models/issue_indexer.go index 3a2ad157c3..26d053a5d7 100644 --- a/models/issue_indexer.go +++ b/models/issue_indexer.go @@ -42,7 +42,7 @@ func populateIssueIndexer() error { } for _, repo := range repos { issues, err := Issues(&IssuesOptions{ - RepoID: repo.ID, + RepoIDs: []int64{repo.ID}, IsClosed: util.OptionalBoolNone, IsPull: util.OptionalBoolNone, }) diff --git a/models/issue_test.go b/models/issue_test.go index 47bb4feeaa..851fe684fb 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -168,3 +168,114 @@ func TestUpdateIssueCols(t *testing.T) { assert.EqualValues(t, prevContent, updatedIssue.Content) AssertInt64InRange(t, now, then, int64(updatedIssue.UpdatedUnix)) } + +func TestIssues(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + for _, test := range []struct { + Opts IssuesOptions + ExpectedIssueIDs []int64 + }{ + { + IssuesOptions{ + AssigneeID: 1, + SortType: "oldest", + }, + []int64{1, 6}, + }, + { + IssuesOptions{ + RepoIDs: []int64{1, 3}, + SortType: "oldest", + Page: 1, + PageSize: 4, + }, + []int64{1, 2, 3, 5}, + }, + { + IssuesOptions{ + Labels: "1,2", + Page: 1, + PageSize: 4, + }, + []int64{5, 2, 1}, + }, + } { + issues, err := Issues(&test.Opts) + assert.NoError(t, err) + if assert.Len(t, issues, len(test.ExpectedIssueIDs)) { + for i, issue := range issues { + assert.EqualValues(t, test.ExpectedIssueIDs[i], issue.ID) + } + } + } +} + +func TestGetUserIssueStats(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + for _, test := range []struct { + Opts UserIssueStatsOptions + ExpectedIssueStats IssueStats + }{ + { + UserIssueStatsOptions{ + UserID: 1, + RepoID: 1, + FilterMode: FilterModeAll, + }, + IssueStats{ + YourRepositoriesCount: 0, + AssignCount: 1, + CreateCount: 1, + OpenCount: 0, + ClosedCount: 0, + }, + }, + { + UserIssueStatsOptions{ + UserID: 1, + FilterMode: FilterModeAssign, + }, + IssueStats{ + YourRepositoriesCount: 0, + AssignCount: 2, + CreateCount: 2, + OpenCount: 2, + ClosedCount: 0, + }, + }, + { + UserIssueStatsOptions{ + UserID: 1, + FilterMode: FilterModeCreate, + }, + IssueStats{ + YourRepositoriesCount: 0, + AssignCount: 2, + CreateCount: 2, + OpenCount: 2, + ClosedCount: 0, + }, + }, + { + UserIssueStatsOptions{ + UserID: 2, + UserRepoIDs: []int64{1, 2}, + FilterMode: FilterModeAll, + IsClosed: true, + }, + IssueStats{ + YourRepositoriesCount: 2, + AssignCount: 0, + CreateCount: 2, + OpenCount: 1, + ClosedCount: 2, + }, + }, + } { + stats, err := GetUserIssueStats(test.Opts) + if !assert.NoError(t, err) { + continue + } + assert.Equal(t, test.ExpectedIssueStats, *stats) + } +} diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 08815ee074..c2d4819f05 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -56,7 +56,7 @@ func ListIssues(ctx *context.APIContext) { } issues, err := models.Issues(&models.IssuesOptions{ - RepoID: ctx.Repo.Repository.ID, + RepoIDs: []int64{ctx.Repo.Repository.ID}, Page: ctx.QueryInt("page"), PageSize: setting.UI.IssuePagingNum, IsClosed: isClosed, diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 4e12d62f30..530586a143 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -190,8 +190,8 @@ func Issues(ctx *context.Context) { issues = []*models.Issue{} } else { issues, err = models.Issues(&models.IssuesOptions{ + RepoIDs: []int64{repo.ID}, AssigneeID: assigneeID, - RepoID: repo.ID, PosterID: posterID, MentionedID: mentionedID, MilestoneID: milestoneID, diff --git a/routers/user/home.go b/routers/user/home.go index 756da4f57f..01a1068134 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -9,13 +9,14 @@ import ( "fmt" "sort" - "github.com/Unknwon/paginater" - "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" + + "github.com/Unknwon/com" + "github.com/Unknwon/paginater" ) const ( @@ -231,21 +232,30 @@ func Issues(ctx *context.Context) { return } } - if len(userRepoIDs) <= 0 { userRepoIDs = []int64{-1} } opts := &models.IssuesOptions{ - RepoID: repoID, IsClosed: util.OptionalBoolOf(isShowClosed), IsPull: util.OptionalBoolOf(isPullList), SortType: sortType, } + if repoID > 0 { + opts.RepoIDs = []int64{repoID} + } + switch filterMode { case models.FilterModeAll: - opts.RepoIDs = userRepoIDs + if repoID > 0 { + if !com.IsSliceContainsInt64(userRepoIDs, repoID) { + // force an empty result + opts.RepoIDs = []int64{-1} + } + } else { + opts.RepoIDs = userRepoIDs + } case models.FilterModeAssign: opts.AssigneeID = ctxUser.ID case models.FilterModeCreate: @@ -308,7 +318,18 @@ func Issues(ctx *context.Context) { issue.Repo = showReposMap[issue.RepoID] } - issueStats := models.GetUserIssueStats(repoID, ctxUser.ID, userRepoIDs, filterMode, isPullList) + issueStats, err := models.GetUserIssueStats(models.UserIssueStatsOptions{ + UserID: ctxUser.ID, + RepoID: repoID, + UserRepoIDs: userRepoIDs, + FilterMode: filterMode, + IsPull: isPullList, + IsClosed: isShowClosed, + }) + if err != nil { + ctx.Handle(500, "GetUserIssueStats", err) + return + } var total int if !isShowClosed {