Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include public repos in doer's dashboard for issue search #28304

Merged
merged 8 commits into from
Dec 7, 2023
Merged

Include public repos in doer's dashboard for issue search #28304

merged 8 commits into from
Dec 7, 2023

Conversation

wolfogre
Copy link
Member

@wolfogre wolfogre commented Nov 30, 2023

It will fix #28268 .

image image

⚠️ BREAKING ⚠️

But need to give up some features:

image

However, such abandonment may fix #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.

image

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 #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 #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."

image

Before #26012, it was being done in the DB, but it caused the results to be incomplete (see the description of #26012).

And to keep this, #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.

// 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
}

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.

image

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.

image

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".

@wolfogre wolfogre added type/bug pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! backport/v1.20 This PR should be backported to Gitea 1.20 backport/v1.21 This PR should be backported to Gitea 1.21 labels Nov 30, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 30, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 30, 2023
@lunny lunny added this to the 1.22.0 milestone Nov 30, 2023
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not need the left "repo list", so I have no objection.

But it doesn't seem good to backport such a breaking change.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 5, 2023
@lunny lunny removed the backport/v1.20 This PR should be backported to Gitea 1.20 label Dec 5, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 5, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 7, 2023
@lunny lunny merged commit beb71f5 into go-gitea:main Dec 7, 2023
25 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 7, 2023
@GiteaBot
Copy link
Contributor

GiteaBot commented Dec 7, 2023

I was unable to create a backport for 1.21. @wolfogre, please send one manually. 🍵

go run ./contrib/backport 28304
...  // fix git conflicts if any
go run ./contrib/backport --continue

@GiteaBot GiteaBot added the backport/manual No power to the bots! Create your backport yourself! label Dec 7, 2023
brechtvl pushed a commit to blender/gitea that referenced this pull request Dec 7, 2023
…8304)

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">

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 .

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.

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

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.

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".
@wolfogre wolfogre added the backport/done All backports for this PR have been created label Dec 7, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 8, 2023
* giteaofficial/main:
  Improve text in Security settings (go-gitea#28393)
  Fix Docker meta action for releases (go-gitea#28232)
  Make gogit Repository.GetBranchNames consistent (go-gitea#28348)
  Remove GetByBean method because sometimes it's danger when query condition parameter is zero and also introduce new generic methods (go-gitea#28220)
  Include public repos in doer's dashboard for issue search (go-gitea#28304)
  Issue fixes for RSS feed improvements (go-gitea#28380)
  Fix margin in server signed signature verification view (go-gitea#28379)
  [skip ci] Updated translations via Crowdin
  Fix incorrect run order of action jobs (go-gitea#28367)
  Improve RSS feed icons (go-gitea#28368)
  Use `filepath` instead of `path` to create SQLite3 database file (go-gitea#28374)
  Fix incorrect default value of `[attachment].MAX_SIZE` (go-gitea#28373)
  Fix object does not exist error when checking citation file (go-gitea#28314)
@wolfogre wolfogre removed backport/done All backports for this PR have been created backport/manual No power to the bots! Create your backport yourself! backport/v1.21 This PR should be backported to Gitea 1.21 labels Dec 11, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 14, 2023
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
…8304)

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".
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 23, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 23, 2024
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
…8304)

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".
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 6, 2024
@6543
Copy link
Member

6543 commented Jul 5, 2024

if you use it less like an "issue search" and more like an "issue dashboard" the repo filter IS quite usefull ...

some could argue that the UX was bad ... yes. I'll open a new issue to bring "repo filters" back .. -> #31578

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User Issues page no longer shows issues in all repositories pulls/issue page very slow with >19000 repos
5 participants