-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
API endpoint for searching teams. #8108
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
API endpoint for searching teams. #8108
Conversation
Signed-off-by: dasv <[email protected]>
bcddfd5
to
7cfa3d5
Compare
Codecov Report
@@ Coverage Diff @@
## master #8108 +/- ##
=========================================
Coverage ? 41.64%
=========================================
Files ? 495
Lines ? 65523
Branches ? 0
=========================================
Hits ? 27285
Misses ? 34721
Partials ? 3517
Continue to review full report at Codecov.
|
Why is global team search needed? Imho it should be under |
Maybe possibility to search all teams you belong to or admin want to search all teams in installation? I considered if it should be under the organization but didn't want to restrict future possibilities too much. Anyhow I only need the search within an organization so it doesn't matter to me. |
Currently I do not see use case for this and I would want to reserve |
Signed-off-by: David Svantesson <[email protected]>
49baf42
to
ebecd5a
Compare
Signed-off-by: David Svantesson <[email protected]>
Signed-off-by: David Svantesson <[email protected]>
Ok, changed to |
Signed-off-by: David Svantesson <[email protected]>
grammar Co-Authored-By: Richard Mahn <[email protected]>
3d52c44
to
ff4520b
Compare
I have added a few test cases for this as well, so I consider it ready for review. |
Signed-off-by: David Svantesson <[email protected]>
…his PR. Signed-off-by: David Svantesson <[email protected]>
@guillep2k Maybe you could take some time to review this? |
Signed-off-by: David Svantesson <[email protected]>
models/org_team.go
Outdated
// SearchTeamOptions holds the search options | ||
type SearchTeamOptions struct { | ||
UserID int64 | ||
UserIsAdmin bool |
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.
UserIsAdmin
is not used? I guess the router takes care of the permissions, but why is it here?
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.
Removed
models/org_team.go
Outdated
Keyword string | ||
OrgID int64 | ||
IncludeDesc bool | ||
Limit int |
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 didn't check other API endpoints, but isn't it a bit strange to limit the query while not supporting pagination?
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.
Pagination added to the API.
Can this be merged? |
models/org_team.go
Outdated
|
||
if len(opts.Keyword) > 0 { | ||
lowerKeyword := strings.ToLower(opts.Keyword) | ||
var keywordCond = builder.NewCond() |
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.
var keywordCond = builder.Like{"lower_name"}
if opts.IncludeDesc {
keywordCond = keywordCond.Or(builder.Like{"LOWER(description)", lowerKeyword})
}
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.
:(
var keywordCond builder.Cond = builder.Like{"lower_name"}
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.
Sorry, ran out of laptop battery before I could finish properly. Will fix later.
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.
Now I think I got it right.
routers/api/v1/org/team.go
Outdated
// "$ref": "#/definitions/Team" | ||
opts := &models.SearchTeamOptions{ | ||
UserID: ctx.Data["SignedUserID"].(int64), | ||
Keyword: strings.Trim(ctx.Query("q"), " "), |
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.
strings.TrimSpace
routers/api/v1/org/team.go
Outdated
// items: | ||
// "$ref": "#/definitions/Team" | ||
opts := &models.SearchTeamOptions{ | ||
UserID: ctx.Data["SignedUserID"].(int64), |
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.
ctx.User.ID
routers/api/v1/org/team.go
Outdated
UserID: ctx.Data["SignedUserID"].(int64), | ||
Keyword: strings.Trim(ctx.Query("q"), " "), | ||
OrgID: ctx.Org.Organization.ID, | ||
IncludeDesc: (ctx.Query("inclDesc") == "" || ctx.QueryBool("inclDesc")), |
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.
Maybe include_desc
is better.
Signed-off-by: David Svantesson <[email protected]>
Add API endpoint
GET /orgs/{org}/teams/search
for searching teams.This is needed for #8045, but as it is working code by itself I thought it is better to make a smaller PR first.