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.

Sign up for GitHub

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

Jump to bottom

Added issue search via api #3612

Merged
merged 23 commits into from Mar 7, 2018
Merged

Added issue search via api #3612

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7a54205
Started implementing issue api search
kolaente Mar 1, 2018
5bc4f3f
Implemented issue search via api
kolaente Mar 2, 2018
f3870bf
Added search to swagger.json
kolaente Mar 2, 2018
bfd61f9
Removed todo
kolaente Mar 2, 2018
752d5b5
fmt
kolaente Mar 2, 2018
b01f5a7
Merge branch 'master' into feature/api-issue-search
kolaente Mar 2, 2018
c2542eb
Added comment to generate swagger json via "generate-swagger"
kolaente Mar 2, 2018
a1211c0
Merge remote-tracking branch 'upstream/master' into feature/api-issue…
kolaente Mar 2, 2018
3755d09
Merge branch 'feature/api-issue-search' of https://github.com/kolaent…
kolaente Mar 2, 2018
7818adc
Simplified search
kolaente Mar 2, 2018
1bfcb04
fmt
kolaente Mar 2, 2018
38cbfd9
Removed unessecary comment
kolaente Mar 2, 2018
7b08c92
Removed unessecary declaration of the issues-variable
kolaente Mar 2, 2018
533e1aa
Removed unessecary comment
kolaente Mar 2, 2018
b598149
Removed unessecary comment
kolaente Mar 2, 2018
be9ddba
Merge branch 'master' into feature/api-issue-search
kolaente Mar 3, 2018
c5d9b4c
Merge remote-tracking branch 'upstream/master' into feature/api-issue…
kolaente Mar 3, 2018
5df8c13
Merge branch 'feature/api-issue-search' of https://github.com/kolaent…
kolaente Mar 3, 2018
5255550
Added explanation keyword
kolaente Mar 3, 2018
5f2405d
Simplified check for empty keyword
kolaente Mar 3, 2018
1db0418
corrected check if keyword is empty
kolaente Mar 3, 2018
1f209aa
Merge branch 'master' into feature/api-issue-search
kolaente Mar 5, 2018
34918c6
Merge branch 'master' into feature/api-issue-search
lafriks Mar 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 42 additions & 0 deletions public/swagger.v1.json
View file
Open in desktop
Expand Up @@ -1830,6 +1830,12 @@
"description": "page number of requested issues",
"name": "page",
"in": "query"
},
{
"type": "string",
"description": "search string",
"name": "q",
"in": "query"
}
],
"responses": {
Expand Down Expand Up @@ -3225,6 +3231,37 @@
},
"/repos/{owner}/{repo}/releases": {
"get": {
"produces": [
"application/json"
],
"tags": [
"repository"
],
"summary": "List a repo's releases",
"operationId": "repoListReleases",
"parameters": [
{
"type": "string",
"description": "owner of the repo",
"name": "owner",
"in": "path",
"required": true
},
{
"type": "string",
"description": "name of the repo",
"name": "repo",
"in": "path",
"required": true
}
],
"responses": {
"200": {
"$ref": "#/responses/ReleaseList"
}
}
},
"post": {
"consumes": [
"application/json"
],
Expand Down Expand Up @@ -5202,6 +5239,11 @@
"uniqueItems": true,
"x-go-name": "Key"
},
"read_only": {
"description": "Describe if the key has only read access or read/write",
"type": "boolean",
"x-go-name": "ReadOnly"
},
"title": {
"description": "Title of the key to add",
"type": "string",
Expand Down
42 changes: 36 additions & 6 deletions routers/api/v1/repo/issue.go
View file
Open in desktop
Expand Up @@ -5,13 +5,15 @@
package repo

import (
"bytes"
Copy link
Member

@sapk sapk Mar 2, 2018 •
edited

Choose a reason for hiding this comment

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

You allready done it.

"fmt"
"strings"

api "code.gitea.io/sdk/gitea"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/indexer"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
)
Expand Down Expand Up @@ -42,6 +44,10 @@ func ListIssues(ctx *context.APIContext) {
// in: query
// description: page number of requested issues
// type: integer
// - name: q
// in: query
// description: search string
// type: string
// responses:
// "200":
// "$ref": "#/responses/IssueList"
Expand All @@ -55,12 +61,36 @@ func ListIssues(ctx *context.APIContext) {
isClosed = util.OptionalBoolFalse
}

issues, err := models.Issues(&models.IssuesOptions{
RepoIDs: []int64{ctx.Repo.Repository.ID},
Page: ctx.QueryInt("page"),
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

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

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)

keyword = ""
}
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

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 😁

issues = []*models.Issue{}
}
}

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

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 •
edited

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 •
edited

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.

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

if err != nil {
ctx.Error(500, "Issues", err)
return
Expand Down