Skip to content

Commit b592258

Browse files
wolfogrefuxiaohei
authored andcommitted
Include public repos in doer's dashboard for issue search (go-gitea#28304)
It will fix go-gitea#28268 . <img width="1313" alt="image" src="https://github.com/go-gitea/gitea/assets/9418365/cb1e07d5-7a12-4691-a054-8278ba255bfc"> <img width="1318" alt="image" src="https://github.com/go-gitea/gitea/assets/9418365/4fd60820-97f1-4c2c-a233-d3671a5039e9"> ## ⚠️ BREAKING ⚠️ But need to give up some features: <img width="1312" alt="image" src="https://github.com/go-gitea/gitea/assets/9418365/281c0d51-0e7d-473f-bbed-216e2f645610"> However, such abandonment may fix go-gitea#28055 . ## Backgroud When the user switches the dashboard context to an org, it means they want to search issues in the repos that belong to the org. However, when they switch to themselves, it means all repos they can access because they may have created an issue in a public repo that they don't own. <img width="286" alt="image" src="https://github.com/go-gitea/gitea/assets/9418365/182dcd5b-1c20-4725-93af-96e8dfae5b97"> It's a confusing design. Think about this: What does "In your repositories" mean when the user switches to an org? Repos belong to the user or the org? Whatever, it has been broken by go-gitea#26012 and its following PRs. After the PR, it searches for issues in repos that the dashboard context user owns or has been explicitly granted access to, so it causes go-gitea#28268. ## How to fix it It's not really difficult to fix it. Just extend the repo scope to search issues when the dashboard context user is the doer. Since the user may create issues or be mentioned in any public repo, we can just set `AllPublic` to true, which is already supported by indexers. The DB condition will also support it in this PR. But the real difficulty is how to count the search results grouped by repos. It's something like "search issues with this keyword and those filters, and return the total number and the top results. **Then, group all of them by repo and return the counts of each group.**" <img width="314" alt="image" src="https://github.com/go-gitea/gitea/assets/9418365/5206eb20-f8f5-49b9-b45a-1be2fcf679f4"> Before go-gitea#26012, it was being done in the DB, but it caused the results to be incomplete (see the description of go-gitea#26012). And to keep this, go-gitea#26012 implement it in an inefficient way, just count the issues by repo one by one, so it cannot work when `AllPublic` is true because it's almost impossible to do this for all public repos. https://github.com/go-gitea/gitea/blob/1bfcdeef4cca0f5509476358e5931c13d37ed1ca/modules/indexer/issues/indexer.go#L318-L338 ## Give up unnecessary features We may can resovle `TODO: use "group by" of the indexer engines to implement it`, I'm sure it can be done with Elasticsearch, but IIRC, Bleve and Meilisearch don't support "group by". And the real question is, does it worth it? Why should we need to know the counts grouped by repos? Let me show you my search dashboard on gitea.com. <img width="1304" alt="image" src="https://github.com/go-gitea/gitea/assets/9418365/2bca2d46-6c71-4de1-94cb-0c9af27c62ff"> I never think the long repo list helps anything. And if we agree to abandon it, things will be much easier. That is this PR. ## TODO I know it's important to filter by repos when searching issues. However, it shouldn't be the way we have it now. It could be implemented like this. <img width="1316" alt="image" src="https://github.com/go-gitea/gitea/assets/9418365/99ee5f21-cbb5-4dfe-914d-cb796cb79fbe"> The indexers support it well now, but it requires some frontend work, which I'm not good at. So, I think someone could help do that in another PR and merge this one to fix the bug first. Or please block this PR and help to complete it. Finally, "Switch dashboard context" is also a design that needs improvement. In my opinion, it can be accomplished by adding filtering conditions instead of "switching".
1 parent c5bbb73 commit b592258

File tree

7 files changed

+46
-221
lines changed

7 files changed

+46
-221
lines changed

models/issues/issue_search.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
type IssuesOptions struct { //nolint
2424
db.Paginator
2525
RepoIDs []int64 // overwrites RepoCond if the length is not 0
26+
AllPublic bool // include also all public repositories
2627
RepoCond builder.Cond
2728
AssigneeID int64
2829
PosterID int64
@@ -197,6 +198,12 @@ func applyRepoConditions(sess *xorm.Session, opts *IssuesOptions) *xorm.Session
197198
} else if len(opts.RepoIDs) > 1 {
198199
opts.RepoCond = builder.In("issue.repo_id", opts.RepoIDs)
199200
}
201+
if opts.AllPublic {
202+
if opts.RepoCond == nil {
203+
opts.RepoCond = builder.NewCond()
204+
}
205+
opts.RepoCond = opts.RepoCond.Or(builder.In("issue.repo_id", builder.Select("id").From("repository").Where(builder.Eq{"is_private": false})))
206+
}
200207
if opts.RepoCond != nil {
201208
sess.And(opts.RepoCond)
202209
}

modules/indexer/issues/db/options.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ func ToDBOptions(ctx context.Context, options *internal.SearchOptions) (*issue_m
5555
opts := &issue_model.IssuesOptions{
5656
Paginator: options.Paginator,
5757
RepoIDs: options.RepoIDs,
58+
AllPublic: options.AllPublic,
5859
RepoCond: nil,
5960
AssigneeID: convertID(options.AssigneeID),
6061
PosterID: convertID(options.PosterID),

modules/indexer/issues/dboptions.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ func ToSearchOptions(keyword string, opts *issues_model.IssuesOptions) *SearchOp
1212
searchOpt := &SearchOptions{
1313
Keyword: keyword,
1414
RepoIDs: opts.RepoIDs,
15-
AllPublic: false,
15+
AllPublic: opts.AllPublic,
1616
IsPull: opts.IsPull,
1717
IsClosed: opts.IsClosed,
1818
}

modules/indexer/issues/indexer.go

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313

1414
db_model "code.gitea.io/gitea/models/db"
1515
repo_model "code.gitea.io/gitea/models/repo"
16-
"code.gitea.io/gitea/modules/container"
1716
"code.gitea.io/gitea/modules/graceful"
1817
"code.gitea.io/gitea/modules/indexer/issues/bleve"
1918
"code.gitea.io/gitea/modules/indexer/issues/db"
@@ -314,30 +313,3 @@ func CountIssues(ctx context.Context, opts *SearchOptions) (int64, error) {
314313
_, total, err := SearchIssues(ctx, opts)
315314
return total, err
316315
}
317-
318-
// CountIssuesByRepo counts issues by options and group by repo id.
319-
// It's not a complete implementation, since it requires the caller should provide the repo ids.
320-
// That means opts.RepoIDs must be specified, and opts.AllPublic must be false.
321-
// It's good enough for the current usage, and it can be improved if needed.
322-
// TODO: use "group by" of the indexer engines to implement it.
323-
func CountIssuesByRepo(ctx context.Context, opts *SearchOptions) (map[int64]int64, error) {
324-
if len(opts.RepoIDs) == 0 {
325-
return nil, fmt.Errorf("opts.RepoIDs must be specified")
326-
}
327-
if opts.AllPublic {
328-
return nil, fmt.Errorf("opts.AllPublic must be false")
329-
}
330-
331-
repoIDs := container.SetOf(opts.RepoIDs...).Values()
332-
ret := make(map[int64]int64, len(repoIDs))
333-
// TODO: it could be faster if do it in parallel for some indexer engines. Improve it if users report it's slow.
334-
for _, repoID := range repoIDs {
335-
count, err := CountIssues(ctx, opts.Copy(func(o *internal.SearchOptions) { o.RepoIDs = []int64{repoID} }))
336-
if err != nil {
337-
return nil, err
338-
}
339-
ret[repoID] = count
340-
}
341-
342-
return ret, nil
343-
}

routers/web/user/home.go

Lines changed: 21 additions & 138 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"code.gitea.io/gitea/modules/container"
2727
"code.gitea.io/gitea/modules/context"
2828
issue_indexer "code.gitea.io/gitea/modules/indexer/issues"
29-
"code.gitea.io/gitea/modules/json"
3029
"code.gitea.io/gitea/modules/log"
3130
"code.gitea.io/gitea/modules/markup"
3231
"code.gitea.io/gitea/modules/markup/markdown"
@@ -335,7 +334,6 @@ func Pulls(ctx *context.Context) {
335334

336335
ctx.Data["Title"] = ctx.Tr("pull_requests")
337336
ctx.Data["PageIsPulls"] = true
338-
ctx.Data["SingleRepoAction"] = "pull"
339337
buildIssueOverview(ctx, unit.TypePullRequests)
340338
}
341339

@@ -349,7 +347,6 @@ func Issues(ctx *context.Context) {
349347

350348
ctx.Data["Title"] = ctx.Tr("issues")
351349
ctx.Data["PageIsIssues"] = true
352-
ctx.Data["SingleRepoAction"] = "issue"
353350
buildIssueOverview(ctx, unit.TypeIssues)
354351
}
355352

@@ -475,6 +472,13 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
475472
opts.RepoIDs = []int64{0}
476473
}
477474
}
475+
if ctx.Doer.ID == ctxUser.ID && filterMode != issues_model.FilterModeYourRepositories {
476+
// If the doer is the same as the context user, which means the doer is viewing his own dashboard,
477+
// it's not enough to show the repos that the doer owns or has been explicitly granted access to,
478+
// because the doer may create issues or be mentioned in any public repo.
479+
// So we need search issues in all public repos.
480+
opts.AllPublic = true
481+
}
478482

479483
switch filterMode {
480484
case issues_model.FilterModeAll:
@@ -499,14 +503,6 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
499503
isShowClosed := ctx.FormString("state") == "closed"
500504
opts.IsClosed = util.OptionalBoolOf(isShowClosed)
501505

502-
// Filter repos and count issues in them. Count will be used later.
503-
// USING NON-FINAL STATE OF opts FOR A QUERY.
504-
issueCountByRepo, err := issue_indexer.CountIssuesByRepo(ctx, issue_indexer.ToSearchOptions(keyword, opts))
505-
if err != nil {
506-
ctx.ServerError("CountIssuesByRepo", err)
507-
return
508-
}
509-
510506
// Make sure page number is at least 1. Will be posted to ctx.Data.
511507
page := ctx.FormInt("page")
512508
if page <= 1 {
@@ -531,17 +527,6 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
531527
}
532528
opts.LabelIDs = labelIDs
533529

534-
// Parse ctx.FormString("repos") and remember matched repo IDs for later.
535-
// Gets set when clicking filters on the issues overview page.
536-
selectedRepoIDs := getRepoIDs(ctx.FormString("repos"))
537-
// Remove repo IDs that are not accessible to the user.
538-
selectedRepoIDs = slices.DeleteFunc(selectedRepoIDs, func(v int64) bool {
539-
return !accessibleRepos.Contains(v)
540-
})
541-
if len(selectedRepoIDs) > 0 {
542-
opts.RepoIDs = selectedRepoIDs
543-
}
544-
545530
// ------------------------------
546531
// Get issues as defined by opts.
547532
// ------------------------------
@@ -562,41 +547,6 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
562547
}
563548
}
564549

565-
// ----------------------------------
566-
// Add repository pointers to Issues.
567-
// ----------------------------------
568-
569-
// Remove repositories that should not be shown,
570-
// which are repositories that have no issues and are not selected by the user.
571-
selectedRepos := container.SetOf(selectedRepoIDs...)
572-
for k, v := range issueCountByRepo {
573-
if v == 0 && !selectedRepos.Contains(k) {
574-
delete(issueCountByRepo, k)
575-
}
576-
}
577-
578-
// showReposMap maps repository IDs to their Repository pointers.
579-
showReposMap, err := loadRepoByIDs(ctx, ctxUser, issueCountByRepo, unitType)
580-
if err != nil {
581-
if repo_model.IsErrRepoNotExist(err) {
582-
ctx.NotFound("GetRepositoryByID", err)
583-
return
584-
}
585-
ctx.ServerError("loadRepoByIDs", err)
586-
return
587-
}
588-
589-
// a RepositoryList
590-
showRepos := repo_model.RepositoryListOfMap(showReposMap)
591-
sort.Sort(showRepos)
592-
593-
// maps pull request IDs to their CommitStatus. Will be posted to ctx.Data.
594-
for _, issue := range issues {
595-
if issue.Repo == nil {
596-
issue.Repo = showReposMap[issue.RepoID]
597-
}
598-
}
599-
600550
commitStatuses, lastStatus, err := pull_service.GetIssuesAllCommitStatus(ctx, issues)
601551
if err != nil {
602552
ctx.ServerError("GetIssuesLastCommitStatus", err)
@@ -606,7 +556,7 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
606556
// -------------------------------
607557
// Fill stats to post to ctx.Data.
608558
// -------------------------------
609-
issueStats, err := getUserIssueStats(ctx, filterMode, issue_indexer.ToSearchOptions(keyword, opts), ctx.Doer.ID)
559+
issueStats, err := getUserIssueStats(ctx, ctxUser, filterMode, issue_indexer.ToSearchOptions(keyword, opts))
610560
if err != nil {
611561
ctx.ServerError("getUserIssueStats", err)
612562
return
@@ -619,25 +569,6 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
619569
} else {
620570
shownIssues = int(issueStats.ClosedCount)
621571
}
622-
if len(opts.RepoIDs) != 0 {
623-
shownIssues = 0
624-
for _, repoID := range opts.RepoIDs {
625-
shownIssues += int(issueCountByRepo[repoID])
626-
}
627-
}
628-
629-
var allIssueCount int64
630-
for _, issueCount := range issueCountByRepo {
631-
allIssueCount += issueCount
632-
}
633-
ctx.Data["TotalIssueCount"] = allIssueCount
634-
635-
if len(opts.RepoIDs) == 1 {
636-
repo := showReposMap[opts.RepoIDs[0]]
637-
if repo != nil {
638-
ctx.Data["SingleRepoLink"] = repo.Link()
639-
}
640-
}
641572

642573
ctx.Data["IsShowClosed"] = isShowClosed
643574

@@ -674,12 +605,9 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
674605
}
675606
ctx.Data["CommitLastStatus"] = lastStatus
676607
ctx.Data["CommitStatuses"] = commitStatuses
677-
ctx.Data["Repos"] = showRepos
678-
ctx.Data["Counts"] = issueCountByRepo
679608
ctx.Data["IssueStats"] = issueStats
680609
ctx.Data["ViewType"] = viewType
681610
ctx.Data["SortType"] = sortType
682-
ctx.Data["RepoIDs"] = selectedRepoIDs
683611
ctx.Data["IsShowClosed"] = isShowClosed
684612
ctx.Data["SelectLabels"] = selectedLabels
685613

@@ -689,15 +617,9 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
689617
ctx.Data["State"] = "open"
690618
}
691619

692-
// Convert []int64 to string
693-
reposParam, _ := json.Marshal(opts.RepoIDs)
694-
695-
ctx.Data["ReposParam"] = string(reposParam)
696-
697620
pager := context.NewPagination(shownIssues, setting.UI.IssuePagingNum, page, 5)
698621
pager.AddParam(ctx, "q", "Keyword")
699622
pager.AddParam(ctx, "type", "ViewType")
700-
pager.AddParam(ctx, "repos", "ReposParam")
701623
pager.AddParam(ctx, "sort", "SortType")
702624
pager.AddParam(ctx, "state", "State")
703625
pager.AddParam(ctx, "labels", "SelectLabels")
@@ -708,55 +630,6 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
708630
ctx.HTML(http.StatusOK, tplIssues)
709631
}
710632

711-
func getRepoIDs(reposQuery string) []int64 {
712-
if len(reposQuery) == 0 || reposQuery == "[]" {
713-
return []int64{}
714-
}
715-
if !issueReposQueryPattern.MatchString(reposQuery) {
716-
log.Warn("issueReposQueryPattern does not match query: %q", reposQuery)
717-
return []int64{}
718-
}
719-
720-
var repoIDs []int64
721-
// remove "[" and "]" from string
722-
reposQuery = reposQuery[1 : len(reposQuery)-1]
723-
// for each ID (delimiter ",") add to int to repoIDs
724-
for _, rID := range strings.Split(reposQuery, ",") {
725-
// Ensure nonempty string entries
726-
if rID != "" && rID != "0" {
727-
rIDint64, err := strconv.ParseInt(rID, 10, 64)
728-
if err == nil {
729-
repoIDs = append(repoIDs, rIDint64)
730-
}
731-
}
732-
}
733-
734-
return repoIDs
735-
}
736-
737-
func loadRepoByIDs(ctx *context.Context, ctxUser *user_model.User, issueCountByRepo map[int64]int64, unitType unit.Type) (map[int64]*repo_model.Repository, error) {
738-
totalRes := make(map[int64]*repo_model.Repository, len(issueCountByRepo))
739-
repoIDs := make([]int64, 0, 500)
740-
for id := range issueCountByRepo {
741-
if id <= 0 {
742-
continue
743-
}
744-
repoIDs = append(repoIDs, id)
745-
if len(repoIDs) == 500 {
746-
if err := repo_model.FindReposMapByIDs(ctx, repoIDs, totalRes); err != nil {
747-
return nil, err
748-
}
749-
repoIDs = repoIDs[:0]
750-
}
751-
}
752-
if len(repoIDs) > 0 {
753-
if err := repo_model.FindReposMapByIDs(ctx, repoIDs, totalRes); err != nil {
754-
return nil, err
755-
}
756-
}
757-
return totalRes, nil
758-
}
759-
760633
// ShowSSHKeys output all the ssh keys of user by uid
761634
func ShowSSHKeys(ctx *context.Context) {
762635
keys, err := db.Find[asymkey_model.PublicKey](ctx, asymkey_model.FindPublicKeyOptions{
@@ -870,8 +743,15 @@ func UsernameSubRoute(ctx *context.Context) {
870743
}
871744
}
872745

873-
func getUserIssueStats(ctx *context.Context, filterMode int, opts *issue_indexer.SearchOptions, doerID int64) (*issues_model.IssueStats, error) {
746+
func getUserIssueStats(ctx *context.Context, ctxUser *user_model.User, filterMode int, opts *issue_indexer.SearchOptions) (*issues_model.IssueStats, error) {
747+
doerID := ctx.Doer.ID
748+
874749
opts = opts.Copy(func(o *issue_indexer.SearchOptions) {
750+
// If the doer is the same as the context user, which means the doer is viewing his own dashboard,
751+
// it's not enough to show the repos that the doer owns or has been explicitly granted access to,
752+
// because the doer may create issues or be mentioned in any public repo.
753+
// So we need search issues in all public repos.
754+
o.AllPublic = doerID == ctxUser.ID
875755
o.AssigneeID = nil
876756
o.PosterID = nil
877757
o.MentionID = nil
@@ -887,7 +767,10 @@ func getUserIssueStats(ctx *context.Context, filterMode int, opts *issue_indexer
887767
{
888768
openClosedOpts := opts.Copy()
889769
switch filterMode {
890-
case issues_model.FilterModeAll, issues_model.FilterModeYourRepositories:
770+
case issues_model.FilterModeAll:
771+
// no-op
772+
case issues_model.FilterModeYourRepositories:
773+
openClosedOpts.AllPublic = false
891774
case issues_model.FilterModeAssign:
892775
openClosedOpts.AssigneeID = &doerID
893776
case issues_model.FilterModeCreate:
@@ -911,7 +794,7 @@ func getUserIssueStats(ctx *context.Context, filterMode int, opts *issue_indexer
911794
}
912795
}
913796

914-
ret.YourRepositoriesCount, err = issue_indexer.CountIssues(ctx, opts)
797+
ret.YourRepositoriesCount, err = issue_indexer.CountIssues(ctx, opts.Copy(func(o *issue_indexer.SearchOptions) { o.AllPublic = false }))
915798
if err != nil {
916799
return nil, err
917800
}

routers/web/user/home_test.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,7 @@ func TestArchivedIssues(t *testing.T) {
4545
// Assert: One Issue (ID 30) from one Repo (ID 50) is retrieved, while nothing from archived Repo 51 is retrieved
4646
assert.EqualValues(t, http.StatusOK, ctx.Resp.Status())
4747

48-
assert.EqualValues(t, map[int64]int64{50: 1}, ctx.Data["Counts"])
4948
assert.Len(t, ctx.Data["Issues"], 1)
50-
assert.Len(t, ctx.Data["Repos"], 1)
5149
}
5250

5351
func TestIssues(t *testing.T) {
@@ -60,10 +58,8 @@ func TestIssues(t *testing.T) {
6058
Issues(ctx)
6159
assert.EqualValues(t, http.StatusOK, ctx.Resp.Status())
6260

63-
assert.EqualValues(t, map[int64]int64{1: 1, 2: 1}, ctx.Data["Counts"])
6461
assert.EqualValues(t, true, ctx.Data["IsShowClosed"])
6562
assert.Len(t, ctx.Data["Issues"], 1)
66-
assert.Len(t, ctx.Data["Repos"], 2)
6763
}
6864

6965
func TestPulls(t *testing.T) {

0 commit comments

Comments
 (0)