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

API endpoint for searching teams. #8108

Merged
merged 23 commits into from
Oct 1, 2019

Conversation

davidsvantesson
Copy link
Contributor

@davidsvantesson davidsvantesson commented Sep 5, 2019

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.

Signed-off-by: dasv <david.svantesson@qrtech.se>
@codecov-io
Copy link

codecov-io commented Sep 5, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d3bc3dd). Click here to learn what that means.
The diff coverage is 79.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8108   +/-   ##
=========================================
  Coverage          ?   41.64%           
=========================================
  Files             ?      495           
  Lines             ?    65523           
  Branches          ?        0           
=========================================
  Hits              ?    27285           
  Misses            ?    34721           
  Partials          ?     3517
Impacted Files Coverage Δ
routers/api/v1/api.go 73% <100%> (ø)
routers/api/v1/org/team.go 38.18% <78.08%> (ø)
models/org_team.go 54.71% <78.57%> (ø)

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 d3bc3dd...5785acd. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 5, 2019
@lunny lunny added the modifies/api This PR adds API routes or modifies them label Sep 6, 2019
@lafriks
Copy link
Member

lafriks commented Sep 6, 2019

Why is global team search needed? Imho it should be under /orgs/<orgname>/teams/search route

@davidsvantesson
Copy link
Contributor Author

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.

@lafriks
Copy link
Member

lafriks commented Sep 6, 2019

Currently I do not see use case for this and I would want to reserve /teams route for global user groups/teams when we implement such

Signed-off-by: David Svantesson <davidsvantesson@gmail.com>
Signed-off-by: David Svantesson <davidsvantesson@gmail.com>
Signed-off-by: David Svantesson <davidsvantesson@gmail.com>
@davidsvantesson
Copy link
Contributor Author

Ok, changed to /orgs/{org}/teams/search.

Signed-off-by: David Svantesson <davidsvantesson@gmail.com>
routers/api/v1/org/team.go Outdated Show resolved Hide resolved
grammar

Co-Authored-By: Richard Mahn <richmahn@users.noreply.github.com>
@davidsvantesson
Copy link
Contributor Author

I have added a few test cases for this as well, so I consider it ready for review.

models/org_team.go Outdated Show resolved Hide resolved
models/org_team.go Outdated Show resolved Hide resolved
routers/api/v1/org/team.go Outdated Show resolved Hide resolved
routers/api/v1/org/team.go Outdated Show resolved Hide resolved
routers/api/v1/org/team.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot 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 Sep 13, 2019
@davidsvantesson
Copy link
Contributor Author

@guillep2k Maybe you could take some time to review this?
Although #8045 can add teams without this, the team search field does not work. I think I messed up a bit by separating these PRs, sorry.

integrations/api_team_test.go Outdated Show resolved Hide resolved
Signed-off-by: David Svantesson <davidsvantesson@gmail.com>
// SearchTeamOptions holds the search options
type SearchTeamOptions struct {
UserID int64
UserIsAdmin bool
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Keyword string
OrgID int64
IncludeDesc bool
Limit int
Copy link
Member

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?

Copy link
Contributor Author

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.

@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 Sep 24, 2019
@davidsvantesson
Copy link
Contributor Author

Can this be merged?


if len(opts.Keyword) > 0 {
lowerKeyword := strings.ToLower(opts.Keyword)
var keywordCond = builder.NewCond()
Copy link
Member

@lunny lunny Sep 27, 2019

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})
}

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

// "$ref": "#/definitions/Team"
opts := &models.SearchTeamOptions{
UserID: ctx.Data["SignedUserID"].(int64),
Keyword: strings.Trim(ctx.Query("q"), " "),
Copy link
Member

Choose a reason for hiding this comment

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

strings.TrimSpace

// items:
// "$ref": "#/definitions/Team"
opts := &models.SearchTeamOptions{
UserID: ctx.Data["SignedUserID"].(int64),
Copy link
Member

Choose a reason for hiding this comment

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

ctx.User.ID

UserID: ctx.Data["SignedUserID"].(int64),
Keyword: strings.Trim(ctx.Query("q"), " "),
OrgID: ctx.Org.Organization.ID,
IncludeDesc: (ctx.Query("inclDesc") == "" || ctx.QueryBool("inclDesc")),
Copy link
Member

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 <davidsvantesson@gmail.com>
@lunny lunny merged commit 36bcd4c into go-gitea:master Oct 1, 2019
@davidsvantesson davidsvantesson deleted the api-endpoint-teams-search branch October 17, 2019 09:03
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@6543 6543 added this to the 1.10.0 milestone May 19, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants