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.
2024-11-18 20:09:26 +00:00 · 2023-08-23 10:29:17 +08:00 · 2023-08-23 10:29:17 +08:00 · 5db21ce7e1
parent 3b91b2d6b1
commit 5db21ce7e1
5 changed files with 187 additions and 110 deletions
--- 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))
 			}
--- 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
+}
--- 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 (
--- 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
+}
--- a/templates/user/dashboard/issues.tmpl
+++ b/templates/user/dashboard/issues.tmpl
@ -5,29 +5,29 @@
 		<div class="ui stackable grid">
 			<div class="four wide column">
 				<div class="ui secondary vertical filter menu gt-bg-transparent">
-					<a class="{{if eq .ViewType "your_repositories"}}active{{end}} item" href="{{.Link}}?type=your_repositories&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}">
+					<a class="{{if eq .ViewType "your_repositories"}}active{{end}} item" href="{{.Link}}?type=your_repositories&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}&q={{$.Keyword}}">
 						{{.locale.Tr "home.issues.in_your_repos"}}
 						<strong class="ui right">{{CountFmt .IssueStats.YourRepositoriesCount}}</strong>
 					</a>
-					<a class="{{if eq .ViewType "assigned"}}active{{end}} item" href="{{.Link}}?type=assigned&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}">
+					<a class="{{if eq .ViewType "assigned"}}active{{end}} item" href="{{.Link}}?type=assigned&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}&q={{$.Keyword}}">
 						{{.locale.Tr "repo.issues.filter_type.assigned_to_you"}}
 						<strong class="ui right">{{CountFmt .IssueStats.AssignCount}}</strong>
 					</a>
-					<a class="{{if eq .ViewType "created_by"}}active{{end}} item" href="{{.Link}}?type=created_by&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}">
+					<a class="{{if eq .ViewType "created_by"}}active{{end}} item" href="{{.Link}}?type=created_by&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}&q={{$.Keyword}}">
 						{{.locale.Tr "repo.issues.filter_type.created_by_you"}}
 						<strong class="ui right">{{CountFmt .IssueStats.CreateCount}}</strong>
 					</a>
 					{{if .PageIsPulls}}
-						<a class="{{if eq .ViewType "review_requested"}}active{{end}} item" href="{{.Link}}?type=review_requested&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}">
+						<a class="{{if eq .ViewType "review_requested"}}active{{end}} item" href="{{.Link}}?type=review_requested&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}&q={{$.Keyword}}">
 							{{.locale.Tr "repo.issues.filter_type.review_requested"}}
 							<strong class="ui right">{{CountFmt .IssueStats.ReviewRequestedCount}}</strong>
 						</a>
-						<a class="{{if eq .ViewType "reviewed_by"}}active{{end}} item" href="{{.Link}}?type=reviewed_by&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}">
+						<a class="{{if eq .ViewType "reviewed_by"}}active{{end}} item" href="{{.Link}}?type=reviewed_by&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}&q={{$.Keyword}}">
 							{{.locale.Tr "repo.issues.filter_type.reviewed_by_you"}}
 							<strong class="ui right">{{CountFmt .IssueStats.ReviewedCount}}</strong>
 						</a>
 					{{end}}
-					<a class="{{if eq .ViewType "mentioned"}}active{{end}} item" href="{{.Link}}?type=mentioned&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}">
+					<a class="{{if eq .ViewType "mentioned"}}active{{end}} item" href="{{.Link}}?type=mentioned&repos=[{{range $.RepoIDs}}{{.}}%2C{{end}}]&sort={{$.SortType}}&state={{.State}}&q={{$.Keyword}}">
 						{{.locale.Tr "repo.issues.filter_type.mentioning_you"}}
 						<strong class="ui right">{{CountFmt .IssueStats.MentionCount}}</strong>
 					</a>