fix: Admin can see all private repositories on Explore page. #1026
Conversation
models/repo.go
Outdated
There was a problem hiding this comment.
It's not a good idea to copy Session. I think you can use cond?
models/repo.go
Outdated
There was a problem hiding this comment.
count, err := x.Where(cond).Cound(new(Repository))
|
LGTM |
tboerger
added
the
lgtm/need 1
|
conflicted |
|
fixed conflicts. |
| } | ||
|
|
||
| return repos, nil | ||
| count = countRepositories(-1, opts.Private) |
There was a problem hiding this comment.
Here, we check opts.Private to see if we should include private repos, but when populating the repos slice, we always include private repos regardless of opts.Private
There was a problem hiding this comment.
Repositories always returns all repositories. maybe we need to change countRepositories(-1, true)?
There was a problem hiding this comment.
If Repositories(..) always returns all repositories, I think it is misleading for it to take a SearchRepoOptions as a parameter, since it ignores most of the fields of opts.
There was a problem hiding this comment.
I can create another PR to refactor the codes according to your suggestion.
There was a problem hiding this comment.
Ok, I suppose I could do it too. Either way, it's outside the scope of this PR.
|
|
||
| // Repositories returns all repositories | ||
| func Repositories(opts *SearchRepoOptions) (_ RepositoryList, err error) { | ||
| func Repositories(opts *SearchRepoOptions) (_ RepositoryList, count int64, err error) { |
There was a problem hiding this comment.
EDITED: Why does this function not examine the contents of opts.Searcher, and exclude repos accordingly (like GetRecentUpdateRepositories(..)~)?
There was a problem hiding this comment.
Why need to examine the contents of opts.Searcher?
https://github.com/go-gitea/gitea/blob/master/models/issue_indexer.go#L119
https://github.com/go-gitea/gitea/blob/master/routers/admin/repos.go#L28
There was a problem hiding this comment.
I meant like GetRecentUpdateRepsitories(..); sorry if that caused any confusion.
Even if every codepath that calls Repositories(..) has opts.Searcher set to nil, someone might in the future add a changes that calls Repositories(..), have opts.Searcher not be nil, and expect Repositories to filter accordingly.
There was a problem hiding this comment.
Since Repositories(..) only looks at opts.PageSize, opts.Page, and opts.OrderBy, would it be better to have it take those 3 values as separate arguments?
There was a problem hiding this comment.
NO. I think using SearchRepoOptions would be better than take 3 arguments if we want to extend the function.
|
LGTM |
|
let L-G-T-M work @ethantkoenig Thanks for review this PR. |
|
let L-G-T-M work |
tboerger
added
lgtm/done
fix #1022
RepoSearchOptions