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

issue search on my related repositories #9758

Merged
merged 33 commits into from
Feb 29, 2020

Conversation

bhalbright
Copy link
Contributor

Added search box on dashboard issues page; searches across user's repositories as appropriate.

Ref issue: #2434

@codecov-io
Copy link

codecov-io commented Jan 14, 2020

Codecov Report

Merging #9758 into master will increase coverage by 0.07%.
The diff coverage is 80.35%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
modules/structs/user_app.go 0% <ø> (ø) ⬆️
routers/api/v1/api.go 76.13% <100%> (+0.77%) ⬆️
modules/convert/convert.go 75.88% <100%> (+0.71%) ⬆️
models/oauth2_application.go 61.01% <53.84%> (+2.86%) ⬆️
routers/api/v1/user/app.go 68.55% <80.95%> (+9.46%) ⬆️
services/pull/temp_repo.go 31.62% <0%> (-2.57%) ⬇️
modules/git/repo.go 47.7% <0%> (+0.91%) ⬆️
modules/process/manager.go 78.31% <0%> (+3.61%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4667efc...b2998a9. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 14, 2020
@lunny
Copy link
Member

lunny commented Jan 14, 2020

I think this should be issue search on my related repositories but not a generic global issue search on all the public repositories.

@bhalbright bhalbright changed the title WIP Global issue search WIP issue search on my related repositories Jan 14, 2020
@bhalbright
Copy link
Contributor Author

Here's an animated gif showing the basics of the change

search

@bhalbright bhalbright changed the title WIP issue search on my related repositories issue search on my related repositories Jan 16, 2020
@bhalbright bhalbright marked this pull request as ready for review January 16, 2020 04:09
@bhalbright
Copy link
Contributor Author

This PR is ready to review, thanks!

@6543
Copy link
Member

6543 commented Jan 16, 2020

@bhalbright you need to add &q={{$.Keyword}} to Line: 8, 13, 17, 21 too

@bhalbright
Copy link
Contributor Author

@bhalbright you need to add &q={{$.Keyword}} to Line: 8, 13, 17, 21 too

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

@GiteaBot GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 16, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 16, 2020
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Jan 16, 2020
@lafriks lafriks added this to the 1.12.0 milestone Jan 16, 2020
@lunny
Copy link
Member

lunny commented Jan 17, 2020

@bhalbright I like your previous design to put the search box on the second line and align right side. And I think that's also easier to optimize for mobile UI?

@bhalbright
Copy link
Contributor Author

bhalbright commented Jan 17, 2020

@bhalbright I like your previous design to put the search box on the second line and align right side. And I think that's also easier to optimize for mobile UI?

@lunny are you referring to this design that was proposed by @scullhead?
image

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):
image

models/issue.go Show resolved Hide resolved
models/issue.go Show resolved Hide resolved
var forceEmpty bool
var issueIDsFromSearch []int64
var keyword string
if !isPullList {
Copy link
Member

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? 🤔

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

@guillep2k guillep2k left a 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
Copy link
Member

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.

Copy link
Contributor Author

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

models/issue.go Show resolved Hide resolved
routers/user/home.go Show resolved Hide resolved
var forceEmpty bool
var issueIDsFromSearch []int64
var keyword string
if !isPullList {
Copy link
Member

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.

var keyword string
if !isPullList {
keyword = strings.Trim(ctx.Query("q"), " ")
if bytes.Contains([]byte(keyword), []byte{0x00}) {
Copy link
Member

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.

Copy link
Contributor Author

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

</div>
</div>
<div class="column center aligned">
{{if .PageIsIssues}}
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks

@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 Feb 29, 2020
@guillep2k guillep2k merged commit 82be59e into go-gitea:master Feb 29, 2020
@bhalbright bhalbright deleted the global-issue-search branch May 30, 2020 02:29
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants