Fix repository search function #2689
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2689 +/- ##
==========================================
- Coverage 26.96% 26.91% -0.06%
==========================================
Files 87 87
Lines 17297 17285 -12
==========================================
- Hits 4664 4652 -12
- Misses 11953 11954 +1
+ Partials 680 679 -1
Continue to review full report at Codecov.
|
tboerger
added
the
lgtm/need 2
|
One thing I found when testing is that team with read access to public repository is not listed because members are not added in |
Morlinest
force-pushed
the
fix/missing-collaborative-repositories
branch
from
b127b46
to
d26bdeb
Compare
models/repo_list.go
Outdated
| } | ||
|
|
||
| if !opts.Private { | ||
| cond = cond.And(builder.Eq{"is_private": false}) | ||
| if opts.Keyword != "" { |
There was a problem hiding this comment.
This is a duplicate of lines 183-186
There was a problem hiding this comment.
Fixed (rebase doesn't work always as expected :D)
Morlinest
force-pushed
the
fix/missing-collaborative-repositories
branch
from
d26bdeb
to
4b584d0
Compare
|
@ethantkoenig I wanted to completely remove |
|
@Morlinest why? |
|
Was not point of this PR, but as this is last function that was using it and it's only few lines to remove I can do it here. |
|
@ethantkoenig |
|
@ethantkoenig Is it OK now? Can you aproove changes? |
Morlinest
force-pushed
the
fix/missing-collaborative-repositories
branch
from
38a6d76
to
46a5037
Compare
|
LGTM |
tboerger
added
lgtm/need 1
|
@ethantkoenig your approval needed |
|
@Morlinest @lafriks I will take a look tonight |
| } else { | ||
| u, err := models.GetUserByID(opts.OwnerID) | ||
| var err error | ||
| repoOwner, err = models.GetUserByID(opts.OwnerID) | ||
| if err != nil { | ||
| ctx.JSON(500, api.SearchError{ |
There was a problem hiding this comment.
Should it be 404 if user doesn't exist?
There was a problem hiding this comment.
@lafriks IMO, it doesn't make sense to return a 500 if the client provides a bad user ID
There was a problem hiding this comment.
@ethantkoenig yes but not 404. It should be some other error or just empty repo list
There was a problem hiding this comment.
@lafriks @ethantkoenig Sounds like a usual 400 Bad Request for me.
There was a problem hiding this comment.
I didn't change it as it is current behaviour. I'll change it to "bad request".
There was a problem hiding this comment.
@ethantkoenig @daviian I see problem, this response is defined in api (swagger):
// Responses:
// 200: SearchResults
// 500: SearchError
There was a problem hiding this comment.
I would keep it as it is now and make changes in v2 api. I can fix only leaking internal information from response. Do you agree?
There was a problem hiding this comment.
@Morlinest @ethantkoenig as this is documented so let's not introduce breaking changes in API
There was a problem hiding this comment.
IMO, calling this a breaking change seems like a stretch, but I don't want to block this PR so I'll accept
models/fixtures/team.yml
Outdated
| unit_types: '[1,2,3,4,5,6,7]' | ||
|
|
There was a problem hiding this comment.
nit: remove trailing space
Morlinest
force-pushed
the
fix/missing-collaborative-repositories
branch
from
46a5037
to
7686465
Compare
|
LGTM |
tboerger
added
lgtm/done
Rewrite of
models.SearchRepositoryByName:whereconditions in search (that was causing showing private repositories not available toOwnerID)opts.Collaborateis set (true)Searcheras this option is confusing (you can't know, if found repository is related toOwnerIDorSearcher) and not used ifOwnerIDis not setFIXMEparts mentioned in unit and integration tests (integrations/api_repo_test.goandmodels/repo_list_test.go)Important files are
routers/api/v1/repo/repo.goandmodels/repo_list.go, other files are for tests.Fixes #2399, #2593.
Main/final part of #2371.