From 5db21ce7e10ba78ede8841bea9db7a63adbececb Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 23 Aug 2023 10:29:17 +0800 Subject: [PATCH] Fix counting and filtering on the dashboard page for issues (#26657) This PR has multiple parts, and I didn't split them because it's not easy to test them separately since they are all about the dashboard page for issues. 1. Support counting issues via indexer to fix #26361 2. Fix repo selection so it also fixes #26653 3. Keep keywords in filter links. The first two are regressions of #26012. After: https://github.com/go-gitea/gitea/assets/9418365/71dfea7e-d9e2-42b6-851a-cc081435c946 Thanks to @CaiCandong for helping with some tests. --- models/repo/repo_list.go | 36 +++-- modules/indexer/issues/indexer.go | 41 ++++- modules/indexer/issues/internal/model.go | 13 ++ routers/web/user/home.go | 195 +++++++++++++---------- templates/user/dashboard/issues.tmpl | 12 +- 5 files changed, 187 insertions(+), 110 deletions(-) diff --git a/models/repo/repo_list.go b/models/repo/repo_list.go index b8427bec4e..a0485ed8d4 100644 --- a/models/repo/repo_list.go +++ b/models/repo/repo_list.go @@ -130,6 +130,10 @@ type SearchRepoOptions struct { // True -> include just collaborative // False -> include just non-collaborative Collaborate util.OptionalBool + // What type of unit the user can be collaborative in, + // it is ignored if Collaborate is False. + // TypeInvalid means any unit type. + UnitType unit.Type // None -> include forks AND non-forks // True -> include just forks // False -> include just non-forks @@ -382,19 +386,25 @@ func SearchRepositoryCondition(opts *SearchRepoOptions) builder.Cond { if opts.Collaborate != util.OptionalBoolFalse { // A Collaboration is: - collaborateCond := builder.And( - // 1. Repository we don't own - builder.Neq{"owner_id": opts.OwnerID}, - // 2. But we can see because of: - builder.Or( - // A. We have unit independent access - UserAccessRepoCond("`repository`.id", opts.OwnerID), - // B. We are in a team for - UserOrgTeamRepoCond("`repository`.id", opts.OwnerID), - // C. Public repositories in organizations that we are member of - userOrgPublicRepoCondPrivate(opts.OwnerID), - ), - ) + + collaborateCond := builder.NewCond() + // 1. Repository we don't own + collaborateCond = collaborateCond.And(builder.Neq{"owner_id": opts.OwnerID}) + // 2. But we can see because of: + { + userAccessCond := builder.NewCond() + // A. We have unit independent access + userAccessCond = userAccessCond.Or(UserAccessRepoCond("`repository`.id", opts.OwnerID)) + // B. We are in a team for + if opts.UnitType == unit.TypeInvalid { + userAccessCond = userAccessCond.Or(UserOrgTeamRepoCond("`repository`.id", opts.OwnerID)) + } else { + userAccessCond = userAccessCond.Or(userOrgTeamUnitRepoCond("`repository`.id", opts.OwnerID, opts.UnitType)) + } + // C. Public repositories in organizations that we are member of + userAccessCond = userAccessCond.Or(userOrgPublicRepoCondPrivate(opts.OwnerID)) + collaborateCond = collaborateCond.And(userAccessCond) + } if !opts.Private { collaborateCond = collaborateCond.And(builder.Expr("owner_id NOT IN (SELECT org_id FROM org_user WHERE org_user.uid = ? AND org_user.is_public = ?)", opts.OwnerID, false)) } diff --git a/modules/indexer/issues/indexer.go b/modules/indexer/issues/indexer.go index 6619949104..020659c82b 100644 --- a/modules/indexer/issues/indexer.go +++ b/modules/indexer/issues/indexer.go @@ -13,6 +13,7 @@ import ( db_model "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/indexer/issues/bleve" "code.gitea.io/gitea/modules/indexer/issues/db" @@ -277,7 +278,7 @@ func IsAvailable(ctx context.Context) bool { } // SearchOptions indicates the options for searching issues -type SearchOptions internal.SearchOptions +type SearchOptions = internal.SearchOptions const ( SortByCreatedDesc = internal.SortByCreatedDesc @@ -291,7 +292,6 @@ const ( ) // SearchIssues search issues by options. -// It returns issue ids and a bool value indicates if the result is imprecise. func SearchIssues(ctx context.Context, opts *SearchOptions) ([]int64, int64, error) { indexer := *globalIndexer.Load() @@ -305,7 +305,7 @@ func SearchIssues(ctx context.Context, opts *SearchOptions) ([]int64, int64, err indexer = db.NewIndexer() } - result, err := indexer.Search(ctx, (*internal.SearchOptions)(opts)) + result, err := indexer.Search(ctx, opts) if err != nil { return nil, 0, err } @@ -317,3 +317,38 @@ func SearchIssues(ctx context.Context, opts *SearchOptions) ([]int64, int64, err return ret, result.Total, nil } + +// CountIssues counts issues by options. It is a shortcut of SearchIssues(ctx, opts) but only returns the total count. +func CountIssues(ctx context.Context, opts *SearchOptions) (int64, error) { + opts = opts.Copy(func(options *SearchOptions) { opts.Paginator = &db_model.ListOptions{PageSize: 0} }) + + _, total, err := SearchIssues(ctx, opts) + return total, err +} + +// CountIssuesByRepo counts issues by options and group by repo id. +// It's not a complete implementation, since it requires the caller should provide the repo ids. +// That means opts.RepoIDs must be specified, and opts.AllPublic must be false. +// It's good enough for the current usage, and it can be improved if needed. +// TODO: use "group by" of the indexer engines to implement it. +func CountIssuesByRepo(ctx context.Context, opts *SearchOptions) (map[int64]int64, error) { + if len(opts.RepoIDs) == 0 { + return nil, fmt.Errorf("opts.RepoIDs must be specified") + } + if opts.AllPublic { + return nil, fmt.Errorf("opts.AllPublic must be false") + } + + repoIDs := container.SetOf(opts.RepoIDs...).Values() + ret := make(map[int64]int64, len(repoIDs)) + // TODO: it could be faster if do it in parallel for some indexer engines. Improve it if users report it's slow. + for _, repoID := range repoIDs { + count, err := CountIssues(ctx, opts.Copy(func(o *internal.SearchOptions) { o.RepoIDs = []int64{repoID} })) + if err != nil { + return nil, err + } + ret[repoID] = count + } + + return ret, nil +} diff --git a/modules/indexer/issues/internal/model.go b/modules/indexer/issues/internal/model.go index 2de1e0e2bf..031745dd2f 100644 --- a/modules/indexer/issues/internal/model.go +++ b/modules/indexer/issues/internal/model.go @@ -109,6 +109,19 @@ type SearchOptions struct { SortBy SortBy // sort by field } +// Copy returns a copy of the options. +// Be careful, it's not a deep copy, so `SearchOptions.RepoIDs = {...}` is OK while `SearchOptions.RepoIDs[0] = ...` is not. +func (o *SearchOptions) Copy(edit ...func(options *SearchOptions)) *SearchOptions { + if o == nil { + return nil + } + v := *o + for _, e := range edit { + e(&v) + } + return &v +} + type SortBy string const ( diff --git a/routers/web/user/home.go b/routers/web/user/home.go index 8c1447f707..d1a4877e6d 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -448,21 +448,26 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { // - Team org's owns the repository. // - Team has read permission to repository. repoOpts := &repo_model.SearchRepoOptions{ - Actor: ctx.Doer, - OwnerID: ctx.Doer.ID, - Private: true, - AllPublic: false, - AllLimited: false, + Actor: ctx.Doer, + OwnerID: ctx.Doer.ID, + Private: true, + AllPublic: false, + AllLimited: false, + Collaborate: util.OptionalBoolNone, + UnitType: unitType, + Archived: util.OptionalBoolFalse, } if team != nil { repoOpts.TeamID = team.ID } + accessibleRepos := container.Set[int64]{} { ids, _, err := repo_model.SearchRepositoryIDs(repoOpts) if err != nil { ctx.ServerError("SearchRepositoryIDs", err) return } + accessibleRepos.AddMultiple(ids...) opts.RepoIDs = ids if len(opts.RepoIDs) == 0 { // no repos found, don't let the indexer return all repos @@ -489,40 +494,16 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { keyword := strings.Trim(ctx.FormString("q"), " ") ctx.Data["Keyword"] = keyword - accessibleRepos := container.Set[int64]{} - { - ids, err := issues_model.GetRepoIDsForIssuesOptions(opts, ctxUser) - if err != nil { - ctx.ServerError("GetRepoIDsForIssuesOptions", err) - return - } - for _, id := range ids { - accessibleRepos.Add(id) - } - } - // Educated guess: Do or don't show closed issues. isShowClosed := ctx.FormString("state") == "closed" opts.IsClosed = util.OptionalBoolOf(isShowClosed) // Filter repos and count issues in them. Count will be used later. // USING NON-FINAL STATE OF opts FOR A QUERY. - var issueCountByRepo map[int64]int64 - { - issueIDs, err := issueIDsFromSearch(ctx, keyword, opts) - if err != nil { - ctx.ServerError("issueIDsFromSearch", err) - return - } - if len(issueIDs) > 0 { // else, no issues found, just leave issueCountByRepo empty - opts.IssueIDs = issueIDs - issueCountByRepo, err = issues_model.CountIssuesByRepo(ctx, opts) - if err != nil { - ctx.ServerError("CountIssuesByRepo", err) - return - } - opts.IssueIDs = nil // reset, the opts will be used later - } + issueCountByRepo, err := issue_indexer.CountIssuesByRepo(ctx, issue_indexer.ToSearchOptions(keyword, opts)) + if err != nil { + ctx.ServerError("CountIssuesByRepo", err) + return } // Make sure page number is at least 1. Will be posted to ctx.Data. @@ -551,13 +532,13 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { // Parse ctx.FormString("repos") and remember matched repo IDs for later. // Gets set when clicking filters on the issues overview page. - repoIDs := getRepoIDs(ctx.FormString("repos")) - if len(repoIDs) > 0 { - // Remove repo IDs that are not accessible to the user. - repoIDs = util.SliceRemoveAllFunc(repoIDs, func(v int64) bool { - return !accessibleRepos.Contains(v) - }) - opts.RepoIDs = repoIDs + selectedRepoIDs := getRepoIDs(ctx.FormString("repos")) + // Remove repo IDs that are not accessible to the user. + selectedRepoIDs = util.SliceRemoveAllFunc(selectedRepoIDs, func(v int64) bool { + return !accessibleRepos.Contains(v) + }) + if len(selectedRepoIDs) > 0 { + opts.RepoIDs = selectedRepoIDs } // ------------------------------ @@ -568,7 +549,7 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { // USING FINAL STATE OF opts FOR A QUERY. var issues issues_model.IssueList { - issueIDs, err := issueIDsFromSearch(ctx, keyword, opts) + issueIDs, _, err := issue_indexer.SearchIssues(ctx, issue_indexer.ToSearchOptions(keyword, opts)) if err != nil { ctx.ServerError("issueIDsFromSearch", err) return @@ -584,6 +565,18 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { // Add repository pointers to Issues. // ---------------------------------- + // Remove repositories that should not be shown, + // which are repositories that have no issues and are not selected by the user. + selectedReposMap := make(map[int64]struct{}, len(selectedRepoIDs)) + for _, repoID := range selectedRepoIDs { + selectedReposMap[repoID] = struct{}{} + } + for k, v := range issueCountByRepo { + if _, ok := selectedReposMap[k]; !ok && v == 0 { + delete(issueCountByRepo, k) + } + } + // showReposMap maps repository IDs to their Repository pointers. showReposMap, err := loadRepoByIDs(ctxUser, issueCountByRepo, unitType) if err != nil { @@ -615,44 +608,10 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { // ------------------------------- // Fill stats to post to ctx.Data. // ------------------------------- - var issueStats *issues_model.IssueStats - { - statsOpts := issues_model.IssuesOptions{ - RepoIDs: repoIDs, - User: ctx.Doer, - IsPull: util.OptionalBoolOf(isPullList), - IsClosed: util.OptionalBoolOf(isShowClosed), - IssueIDs: nil, - IsArchived: util.OptionalBoolFalse, - LabelIDs: opts.LabelIDs, - Org: org, - Team: team, - RepoCond: opts.RepoCond, - } - - if keyword != "" { - statsOpts.RepoIDs = opts.RepoIDs - allIssueIDs, err := issueIDsFromSearch(ctx, keyword, &statsOpts) - if err != nil { - ctx.ServerError("issueIDsFromSearch", err) - return - } - statsOpts.IssueIDs = allIssueIDs - } - - if keyword != "" && len(statsOpts.IssueIDs) == 0 { - // So it did search with the keyword, but no issue found. - // Just set issueStats to empty. - issueStats = &issues_model.IssueStats{} - } else { - // So it did search with the keyword, and found some issues. It needs to get issueStats of these issues. - // Or the keyword is empty, so it doesn't need issueIDs as filter, just get issueStats with statsOpts. - issueStats, err = issues_model.GetUserIssueStats(filterMode, statsOpts) - if err != nil { - ctx.ServerError("GetUserIssueStats", err) - return - } - } + issueStats, err := getUserIssueStats(ctx, filterMode, issue_indexer.ToSearchOptions(keyword, opts), ctx.Doer.ID) + if err != nil { + ctx.ServerError("getUserIssueStats", err) + return } // Will be posted to ctx.Data. @@ -722,7 +681,7 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { ctx.Data["IssueStats"] = issueStats ctx.Data["ViewType"] = viewType ctx.Data["SortType"] = sortType - ctx.Data["RepoIDs"] = opts.RepoIDs + ctx.Data["RepoIDs"] = selectedRepoIDs ctx.Data["IsShowClosed"] = isShowClosed ctx.Data["SelectLabels"] = selectedLabels @@ -777,14 +736,6 @@ func getRepoIDs(reposQuery string) []int64 { return repoIDs } -func issueIDsFromSearch(ctx *context.Context, keyword string, opts *issues_model.IssuesOptions) ([]int64, error) { - ids, _, err := issue_indexer.SearchIssues(ctx, issue_indexer.ToSearchOptions(keyword, opts)) - if err != nil { - return nil, fmt.Errorf("SearchIssues: %w", err) - } - return ids, nil -} - func loadRepoByIDs(ctxUser *user_model.User, issueCountByRepo map[int64]int64, unitType unit.Type) (map[int64]*repo_model.Repository, error) { totalRes := make(map[int64]*repo_model.Repository, len(issueCountByRepo)) repoIDs := make([]int64, 0, 500) @@ -913,3 +864,71 @@ func UsernameSubRoute(ctx *context.Context) { } } } + +func getUserIssueStats(ctx *context.Context, filterMode int, opts *issue_indexer.SearchOptions, doerID int64) (*issues_model.IssueStats, error) { + opts = opts.Copy(func(o *issue_indexer.SearchOptions) { + o.AssigneeID = nil + o.PosterID = nil + o.MentionID = nil + o.ReviewRequestedID = nil + o.ReviewedID = nil + }) + + var ( + err error + ret = &issues_model.IssueStats{} + ) + + { + openClosedOpts := opts.Copy() + switch filterMode { + case issues_model.FilterModeAll, issues_model.FilterModeYourRepositories: + case issues_model.FilterModeAssign: + openClosedOpts.AssigneeID = &doerID + case issues_model.FilterModeCreate: + openClosedOpts.PosterID = &doerID + case issues_model.FilterModeMention: + openClosedOpts.MentionID = &doerID + case issues_model.FilterModeReviewRequested: + openClosedOpts.ReviewRequestedID = &doerID + case issues_model.FilterModeReviewed: + openClosedOpts.ReviewedID = &doerID + } + openClosedOpts.IsClosed = util.OptionalBoolFalse + ret.OpenCount, err = issue_indexer.CountIssues(ctx, openClosedOpts) + if err != nil { + return nil, err + } + openClosedOpts.IsClosed = util.OptionalBoolTrue + ret.ClosedCount, err = issue_indexer.CountIssues(ctx, openClosedOpts) + if err != nil { + return nil, err + } + } + + ret.YourRepositoriesCount, err = issue_indexer.CountIssues(ctx, opts) + if err != nil { + return nil, err + } + ret.AssignCount, err = issue_indexer.CountIssues(ctx, opts.Copy(func(o *issue_indexer.SearchOptions) { o.AssigneeID = &doerID })) + if err != nil { + return nil, err + } + ret.CreateCount, err = issue_indexer.CountIssues(ctx, opts.Copy(func(o *issue_indexer.SearchOptions) { o.PosterID = &doerID })) + if err != nil { + return nil, err + } + ret.MentionCount, err = issue_indexer.CountIssues(ctx, opts.Copy(func(o *issue_indexer.SearchOptions) { o.MentionID = &doerID })) + if err != nil { + return nil, err + } + ret.ReviewRequestedCount, err = issue_indexer.CountIssues(ctx, opts.Copy(func(o *issue_indexer.SearchOptions) { o.ReviewRequestedID = &doerID })) + if err != nil { + return nil, err + } + ret.ReviewedCount, err = issue_indexer.CountIssues(ctx, opts.Copy(func(o *issue_indexer.SearchOptions) { o.ReviewedID = &doerID })) + if err != nil { + return nil, err + } + return ret, nil +} diff --git a/templates/user/dashboard/issues.tmpl b/templates/user/dashboard/issues.tmpl index 8d6cc67afe..a89098c6ab 100644 --- a/templates/user/dashboard/issues.tmpl +++ b/templates/user/dashboard/issues.tmpl @@ -5,29 +5,29 @@
- + {{.locale.Tr "home.issues.in_your_repos"}} {{CountFmt .IssueStats.YourRepositoriesCount}} - + {{.locale.Tr "repo.issues.filter_type.assigned_to_you"}} {{CountFmt .IssueStats.AssignCount}} - + {{.locale.Tr "repo.issues.filter_type.created_by_you"}} {{CountFmt .IssueStats.CreateCount}} {{if .PageIsPulls}} - + {{.locale.Tr "repo.issues.filter_type.review_requested"}} {{CountFmt .IssueStats.ReviewRequestedCount}} - + {{.locale.Tr "repo.issues.filter_type.reviewed_by_you"}} {{CountFmt .IssueStats.ReviewedCount}} {{end}} - + {{.locale.Tr "repo.issues.filter_type.mentioning_you"}} {{CountFmt .IssueStats.MentionCount}}