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

Added issue search via api #3612

Merged
merged 23 commits into from
Mar 7, 2018
Merged

Conversation

kolaente
Copy link
Member

@kolaente kolaente commented Mar 2, 2018

This PR adds the ability to search for issues via the api. The code is taken from the normal issue search in the GUI.

@codecov-io
Copy link

codecov-io commented Mar 2, 2018

Codecov Report

Merging #3612 into master will increase coverage by 0.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3612      +/-   ##
=========================================
+ Coverage   35.88%   35.9%   +0.01%     
=========================================
  Files         286     286              
  Lines       41294   41312      +18     
=========================================
+ Hits        14820   14832      +12     
- Misses      24294   24299       +5     
- Partials     2180    2181       +1
Impacted Files Coverage Δ
routers/api/v1/repo/issue.go 37.83% <75%> (+2.15%) ⬆️
models/repo_indexer.go 48.3% <0%> (ø) ⬆️

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 d71f510...34918c6. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2018
@@ -1819,6 +1819,12 @@
"in": "path",
"required": true
},
{
Copy link
Member

@sapk sapk Mar 2, 2018

Choose a reason for hiding this comment

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

This should not be edited manually but generated base on code comments at start of ListIssues func.
Once the comments are added. Use make generate-swagger to re-generate this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok, I didn't know that. I'll do ASAP

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -5,13 +5,15 @@
package repo

import (
"bytes"
Copy link
Member

@sapk sapk Mar 2, 2018

Choose a reason for hiding this comment

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

You allready done it.

}
var issueIDs []int64
var err error
if len(keyword) > 0 {
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 you can do something like this:

if len(keyword) > 0 {
	issueIDs, err = indexer.SearchIssuesByKeyword(ctx.Repo.Repository.ID, keyword)
}

if len(keyword) == 0 || len(issueIDs) > 0 {
	issues, err = models.Issues(&models.IssuesOptions{
		RepoIDs:  []int64{ctx.Repo.Repository.ID},
		Page:     ctx.QueryInt("page"),
		PageSize: setting.UI.IssuePagingNum,
		IsClosed: isClosed,
		IssueIDs: issueIDs,
	})
}

(simple search function call where you can immediately return empty array instead of doing "if" would be cleaner...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks good, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it

}
}

// Show the results if we either dont have a keyword or the issues found by said keyword are > 0
Copy link
Member

Choose a reason for hiding this comment

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

nit: This comment is not needed, its obvious from code

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah you're right, I removed it.

issueIDs, err = indexer.SearchIssuesByKeyword(ctx.Repo.Repository.ID, keyword)

// Didn't found anything
if len(issueIDs) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Not needed, issues is already empty

Copy link
Member Author

Choose a reason for hiding this comment

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

But if I don't define it there, I cannot access it in line 83 and 89

Copy link
Member

Choose a reason for hiding this comment

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

It is defined on line 65

Copy link
Member

Choose a reason for hiding this comment

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

I am talking about variable issues and if block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ups, I though you were talking about defining the issues variable in line 65 😁

PageSize: setting.UI.IssuePagingNum,
IsClosed: isClosed,
})
// Define issues var
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment pls

// Define issues var
var issues []*models.Issue

// Check for search
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment pls

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

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 its obvious from code

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah probably 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

@lunny lunny added modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Mar 3, 2018
@lunny lunny added this to the 1.5.0 milestone Mar 3, 2018
issueIDs, err = indexer.SearchIssuesByKeyword(ctx.Repo.Repository.ID, keyword)
}

if len(keyword) == 0 || len(issueIDs) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

It seems this line is unnecessary.

Copy link
Contributor

@thehowl thehowl Mar 3, 2018

Choose a reason for hiding this comment

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

No, it's necessary, but a comment should be added to explain it. At this point, three outcomes are possible:

  1. No keywords were given
  2. Keywords were given, returned no results
  3. Keywords were given, returned some results.

This condition guards against condition 2, because otherwise models.Issues would just be called and would return all the issues as if no keyword was passed.

So, add comment:

// We don't fetch any issue if we have keywords, but SearchIssuesByKeyword returned no issue IDs.
// (In other words, we don't fetch issues if no issue matches our keywords)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a comment.

Copy link
Member

@Morlinest Morlinest Mar 3, 2018

Choose a reason for hiding this comment

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

@thehowl you are right, but we should avoid using another function names (it can be changed and you can not track changes in comments) in comment, just explain "why" that code is needed. IMO your last part is enough:

// Don't fetch issues if no issue matches keyword

Copy link
Member Author

Choose a reason for hiding this comment

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

@Morlinest I've added a comment explaining it.

Copy link
Contributor

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Minor changes.

var issues []*models.Issue

keyword := strings.Trim(ctx.Query("q"), " ")
if bytes.Contains([]byte(keyword), []byte{0x00}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

strings.IndexByte(keyword, 0)

issueIDs, err = indexer.SearchIssuesByKeyword(ctx.Repo.Repository.ID, keyword)
}

if len(keyword) == 0 || len(issueIDs) > 0 {
Copy link
Contributor

@thehowl thehowl Mar 3, 2018

Choose a reason for hiding this comment

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

No, it's necessary, but a comment should be added to explain it. At this point, three outcomes are possible:

  1. No keywords were given
  2. Keywords were given, returned no results
  3. Keywords were given, returned some results.

This condition guards against condition 2, because otherwise models.Issues would just be called and would return all the issues as if no keyword was passed.

So, add comment:

// We don't fetch any issue if we have keywords, but SearchIssuesByKeyword returned no issue IDs.
// (In other words, we don't fetch issues if no issue matches our keywords)

var issues []*models.Issue

keyword := strings.Trim(ctx.Query("q"), " ")
if strings.IndexByte(keyword, 0) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to >= or != -1 (Doesn't guard against \x00 being the first byte)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok thanks, I didn't know about that.

Copy link
Contributor

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

LGTM

@tboerger tboerger 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 Mar 3, 2018
@tboerger tboerger 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 Mar 4, 2018
@lunny
Copy link
Member

lunny commented Mar 4, 2018

need @Morlinest and @sapk 's approvals.

Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

@sapk
Copy link
Member

sapk commented Mar 7, 2018

@lafriks CI fail is because Drone fail to pull the repo.

@lunny
Copy link
Member

lunny commented Mar 7, 2018

@sapk restarted.

@lafriks lafriks merged commit 1a83581 into go-gitea:master Mar 7, 2018
@kolaente
Copy link
Member Author

kolaente commented Mar 7, 2018

Thanks!

@kolaente kolaente deleted the feature/api-issue-search branch March 7, 2018 21:30
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Apr 11, 2018
aswild added a commit to aswild/gitea that referenced this pull request Jul 6, 2018
* SECURITY
  * Limit uploaded avatar image-size to 4096x3072 by default (go-gitea#4353)
  * Do not allow to reuse TOTP passcode (go-gitea#3878)
* FEATURE
  * Add cli commands to regen hooks & keys (go-gitea#3979)
  * Add support for FIDO U2F (go-gitea#3971)
  * Added user language setting (go-gitea#3875)
  * LDAP Public SSH Keys synchronization (go-gitea#1844)
  * Add topic support (go-gitea#3711)
  * Multiple assignees (go-gitea#3705)
  * Add protected branch whitelists for merging (go-gitea#3689)
  * Global code search support (go-gitea#3664)
  * Add label descriptions (go-gitea#3662)
  * Add issue search via API (go-gitea#3612)
  * Add repository setting to enable/disable health checks (go-gitea#3607)
  * Emoji Autocomplete (go-gitea#3433)
  * Implements generator cli for secrets (go-gitea#3531)
* ENHANCEMENT
  * Add more webhooks support and refactor webhook templates directory (go-gitea#3929)
  * Add new option to allow only OAuth2/OpenID user registration (go-gitea#3910)
  * Add option to use paged LDAP search when synchronizing users (go-gitea#3895)
  * Symlink icons (go-gitea#1416)
  * Improve release page UI (go-gitea#3693)
  * Add admin dashboard option to run health checks (go-gitea#3606)
  * Add branch link in branch list (go-gitea#3576)
  * Reduce sql query times in retrieveFeeds (go-gitea#3547)
  * Option to enable or disable swagger endpoints (go-gitea#3502)
  * Add missing licenses (go-gitea#3497)
  * Reduce repo indexer disk usage (go-gitea#3452)
  * Enable caching on assets and avatars (go-gitea#3376)
  * Add repository search ordered by stars/forks. Forks column in admin repo list (go-gitea#3969)
  * Add Environment Variables to Docker template (go-gitea#4012)
  * LFS: make HTTP auth period configurable (go-gitea#4035)
  * Add config path as an optionial flag when changing pass via CLI (go-gitea#4184)
  * Refactor User Settings sections (go-gitea#3900)
  * Allow square brackets in external issue patterns (go-gitea#3408)
  * Add Attachment API (go-gitea#3478)
  * Add EnableTimetracking option to app settings (go-gitea#3719)
  * Add config option to enable or disable log executed SQL (go-gitea#3726)
  * Shows total tracked time in issue and milestone list (go-gitea#3341)
* TRANSLATION
  * Improve English grammar and consistency (go-gitea#3614)
* DEPLOYMENT
  * Allow Gitea to run as different USER in Docker (go-gitea#3961)
  * Provide compressed release binaries (go-gitea#3991)
  * Sign release binaries (go-gitea#4188)
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
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. modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants