-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
issue search on my related repositories #9758
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
Conversation
…l-issue-search
… also trying to fix issue of searching the right repos based on the filter
…pagination; using shown issue status for open/close count; removed some debugging
Codecov Report
@@ Coverage Diff @@
## master #9758 +/- ##
==========================================
+ Coverage 43.71% 43.78% +0.07%
==========================================
Files 586 586
Lines 81652 81764 +112
==========================================
+ Hits 35691 35802 +111
+ Misses 41535 41527 -8
- Partials 4426 4435 +9
Continue to review full report at Codecov.
|
I think this should be issue search on my related repositories but not a generic global issue search on all the public repositories. |
… so that the list of repo ids to serach for the keyword is not limited, we need to get all the issue ids for the shown issue stats
This PR is ready to review, thanks! |
@bhalbright you need to add |
@6543 I actually left those off on purpose but I can see it is debatable. My reasoning is that the numbers next to those links (for example "In your repositories") is not affected by the search, so I thought clicking on it should clear the search and show you the number of issues next to the link. |
|
@lunny are you referring to this design that was proposed by @scullhead? I started down that path but I didn't see an easy way to make it work in the organization view, there isn't quite enough room available depending on resolution with the existing links there (well, not enough room if aligning vertically with the list of issues): |
routers/user/home.go
Outdated
var forceEmpty bool | ||
var issueIDsFromSearch []int64 | ||
var keyword string | ||
if !isPullList { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not pull requests too? Are they not indexed? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is just because the scope of the change is for issues #2434
But I could see that this functionality would be useful for pull requests. I'm not sure the typical way things are done here...I guess some projects are strict about not expanding scope whereas others don't mind a bit. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and I don't know if PRs are indexed or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just going to mark this resolved. I limited the scope to issues based on my understanding of #2434
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the database, pull requests are issues as well (only with is_pull = true
), they are stored in the issue
table and they are indexed indeed (the conversations, not the code itself). This page is used for issues and PRs equally, so I think there's no reason to skip this block when isPullList
is true
. The distinction is already taken care of when the IsPull
flag is set a few lines above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I updated and it now shows on the pulls page and seems to work just fine
…repo ids to search for issues, this is an added protection to ensure we don't search repos the user does not have access to
… issues ids that goes over 1000
…l-issue-search # Conflicts: # routers/user/home.go
…l-issue-search # Conflicts: # templates/user/dashboard/issues.tmpl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so sorry it took me so long to review your PR. Here's some comments.
models/issue.go
Outdated
@@ -76,6 +76,7 @@ var ( | |||
const issueTasksRegexpStr = `(^\s*[-*]\s\[[\sx]\]\s.)|(\n\s*[-*]\s\[[\sx]\]\s.)` | |||
const issueTasksDoneRegexpStr = `(^\s*[-*]\s\[[x]\]\s.)|(\n\s*[-*]\s\[[x]\]\s.)` | |||
const issueMaxDupIndexAttempts = 3 | |||
const maxIssueIDs = 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the actual limit on SQLite might be a little lower (997 or something), but you need to account for other parameters as well. For example, if the query targets repositories with a specific topic, the topic ID will be a parameter. I'd reduce this limit to say... 950? to be on the safe side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I updated to 950
routers/user/home.go
Outdated
var forceEmpty bool | ||
var issueIDsFromSearch []int64 | ||
var keyword string | ||
if !isPullList { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the database, pull requests are issues as well (only with is_pull = true
), they are stored in the issue
table and they are indexed indeed (the conversations, not the code itself). This page is used for issues and PRs equally, so I think there's no reason to skip this block when isPullList
is true
. The distinction is already taken care of when the IsPull
flag is set a few lines above.
routers/user/home.go
Outdated
var keyword string | ||
if !isPullList { | ||
keyword = strings.Trim(ctx.Query("q"), " ") | ||
if bytes.Contains([]byte(keyword), []byte{0x00}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go supports the null character inside strings, so I don't see how this could be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I removed the check. I had copied and pasted this from the issue search code inside a specific repository
templates/user/dashboard/issues.tmpl
Outdated
</div> | ||
</div> | ||
<div class="column center aligned"> | ||
{{if .PageIsIssues}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRs could be supported too just by removing this if.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks
… GetRepoIDsForIssuesOptions, showing search on pulls dashboard page too (not just issues)
Added search box on dashboard issues page; searches across user's repositories as appropriate.
Ref issue: #2434