Remove redudant functions and code #2652

Morlinest · 2017-10-04T13:27:13Z

Remove useless functions/code. Fixes multiple bugs and remove duplicated code, like:

Corrects ordering of repositories on admin page
Corrects results when searching (explore page) - now you can get 2 different results when searching with/without keyword because two different functions are used and implementation is not same for both. But results with non empty keyword should always contains subset of full search (empty keyword) results.
models.Repositories() ignores privacy settings in search but uses it for count (even it's always true as it is used only in 1 place with fixed privacy settings). This function can be replaced by models.SearchRepositoryByName() as it does exactly same thing.
models.GetRecentUpdatedRepositories() doesn't make sense, as it is used only if query parameter q is empty and can return results with different ordering (not "recent updated"). models.SearchRepositoryByName() should do same thing.

Bonus: less code to maintain, less space for bugs :)

Extracted from #2371 and further improved.

codecov-io · 2017-10-04T13:39:16Z

Codecov Report

Merging #2652 into master will increase coverage by 0.1%.
The diff coverage is 70%.

@@            Coverage Diff            @@
##           master    #2652     +/-   ##
=========================================
+ Coverage   27.28%   27.38%   +0.1%     
=========================================
  Files          86       86             
  Lines       17062    17010     -52     
=========================================
+ Hits         4655     4658      +3     
+ Misses      11728    11673     -55     
  Partials      679      679

Impacted Files	Coverage Δ
models/issue_indexer.go	`8.21% <0%> (-0.24%)`	⬇️
models/repo_list.go	`53.12% <100%> (+17.41%)`	⬆️

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 c2346e4...d3e349b. Read the comment docs.

ethantkoenig

It seems that the RepoSearchOptions.Searcher field is never used. I think we still need to use it, but if not then we should just remove the field.

ethantkoenig · 2017-10-04T16:43:13Z

routers/home.go

 		PageSize: setting.UI.ExplorePagingNum,
-		Searcher: ctx.User,


Why is this line removed?

Not needed anymore. ~~But now I see I have to make some changes as models.GetRecentUpdatedRepositories() need only Searcher and not OwnerID.~~

@ethantkoenig I looked at it again and I don't need to change anything. Reason is to unify behaviour when you search with/without keyword. I'll post images.

ethantkoenig · 2017-10-04T16:46:02Z

routers/home.go

+		PageSize: opts.PageSize,
+		OrderBy:  orderBy,
+		Private:  opts.Private,
+		Keyword:  keyword,


Should we add Searcher: opts.Searcher,?

Searcher is useless without OwnerID. In models.SearchRepositoryByName is used only if OwnerID > 0.

lafriks · 2017-10-04T17:22:49Z

models/repo_list.go

@@ -129,6 +129,8 @@ const (
 	SearchOrderByNewest                              = "created_unix DESC"
 	SearchOrderBySize                                = "size ASC"
 	SearchOrderBySizeReverse                         = "size DESC"
+	SearchOrderByID                                  = "ID ASC"


Please write column names lowercase

Morlinest · 2017-10-04T19:06:47Z

@ethantkoenig This is how it looks now:

This PR should fix that. Explore page should be only for showing public repositories (as it is if you use any keyword). For private there is dashboard, user page, user settings, ...

daviian · 2017-10-07T22:48:10Z

@Morlinest Tested and seems good to me except one point: I miss owned private repositories in explore page (for none-admins).

daviian · 2017-10-07T23:46:21Z

@Morlinest Please rebase :)

Morlinest · 2017-10-08T08:49:06Z

@daviian Yes, it's expected change. The reason is explained in second and last point in PR description. Try to open explore page on current master build and type any keyword, you will be missing private repositories (example screenshots: #2652 (comment)).

daviian · 2017-10-08T08:57:03Z

@Morlinest But instead of removing it for all results, it should be included IMO. But I think I am ok if we fix that in another PR.

Morlinest · 2017-10-08T09:25:10Z

I am not against it, but it would be few more changes to code. If also @lafriks or @ethantkoenig (or any other member :)) agree with it, I can change it to show private repositories (owned + collaborative) of signed user too.

muellert · 2017-10-08T14:14:38Z

I am very new to Gitea and have no Gogs experience, so my thinking may not be in line with the philosophy of the software at all, but I think item visibility should only depend on the role of the user performing a search and on the ACL for that item, not on which page the user started the search on.

lafriks · 2017-10-08T14:21:31Z

I also think that user should see on explore page all repositories he has access. For example in my company all repositories are private and in such case that would totally make that page useless

Morlinest · 2017-10-08T14:23:01Z

OK, I'll change current behaviour.

muellert · 2017-10-08T14:29:00Z

I wrote "items" because I don't know if repos are the only type of things affected. But I'd like to be able to find other things like issues or wiki pages as well, if I generally have access to them.

Where would I look in the code to figure all of this out?

Morlinest · 2017-10-09T09:12:25Z

@daviian @lafriks Behaviour changed. Explore page now show always all public repositories and private repositories available to logged user. I had to add new option to models.SearchRepoOptions.

ethantkoenig · 2017-10-10T02:40:58Z

models/repo_list.go

@@ -182,6 +185,10 @@ func SearchRepositoryByName(opts *SearchRepoOptions) (repos RepositoryList, coun
 		cond = cond.And(builder.Eq{"is_private": false})
 	}

+	if opts.OwnerID > 0 && opts.AllPublic {
+		cond = cond.Or(builder.Eq{"is_private": false})


I think this is a bug. If opts.OwnerID > 0 && opts.AllPublic is true, then we will search for all public repos, even those that do not match the keyword.

ethantkoenig · 2017-10-10T05:53:57Z

LGTM

Morlinest · 2017-10-10T14:24:08Z

Added unit tests.

daviian · 2017-10-10T20:36:00Z

LGTM

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 4, 2017

Morlinest force-pushed the fix/use-same-function-for-repo-search branch from 3576ce4 to 195d7f1 Compare October 4, 2017 13:49

ethantkoenig suggested changes Oct 4, 2017

View reviewed changes

lafriks reviewed Oct 4, 2017

View reviewed changes

Morlinest force-pushed the fix/use-same-function-for-repo-search branch 3 times, most recently from 75100dd to 782d9ab Compare October 4, 2017 18:12

lafriks added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Oct 5, 2017

lafriks added this to the 1.3.0 milestone Oct 5, 2017

Morlinest mentioned this pull request Oct 8, 2017

Can't search for repos unless I'm admin #2673

Closed

7 tasks

Morlinest force-pushed the fix/use-same-function-for-repo-search branch from 054f9ba to 8d999c8 Compare October 8, 2017 09:38

ethantkoenig approved these changes Oct 8, 2017

View reviewed changes

Morlinest force-pushed the fix/use-same-function-for-repo-search branch from 4cb10e8 to 2c85a19 Compare October 9, 2017 08:53

Morlinest force-pushed the fix/use-same-function-for-repo-search branch from 2c85a19 to 6d47b19 Compare October 9, 2017 14:24

Morlinest added 3 commits October 10, 2017 03:47

Remove redudant functions and code

7570ff4

Lowercase column names

69c23f2

Remove Searcher field

82b6465

Morlinest force-pushed the fix/use-same-function-for-repo-search branch from 6d47b19 to 4a32cbe Compare October 10, 2017 01:47

ethantkoenig suggested changes Oct 10, 2017

View reviewed changes

Include all public repositories in search result

ceb8174

Morlinest force-pushed the fix/use-same-function-for-repo-search branch from 4a32cbe to ceb8174 Compare October 10, 2017 04:14

ethantkoenig approved these changes Oct 10, 2017

View reviewed changes

tboerger 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 Oct 10, 2017

Add unit tests

d3e349b

tboerger 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 Oct 10, 2017

bkcsoft merged commit dff26e2 into go-gitea:master Oct 10, 2017

Morlinest deleted the fix/use-same-function-for-repo-search branch October 10, 2017 20:42

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Uh oh!

Remove redudant functions and code #2652

Remove redudant functions and code #2652

Uh oh!

Conversation

Morlinest commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ethantkoenig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Morlinest Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Morlinest commented Oct 4, 2017

Uh oh!

daviian commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daviian commented Oct 7, 2017

Uh oh!

Morlinest commented Oct 8, 2017

Uh oh!

daviian commented Oct 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Morlinest commented Oct 8, 2017

Uh oh!

muellert commented Oct 8, 2017

Uh oh!

lafriks commented Oct 8, 2017

Uh oh!

Morlinest commented Oct 8, 2017

Uh oh!

muellert commented Oct 8, 2017

Uh oh!

Morlinest commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethantkoenig commented Oct 10, 2017

Uh oh!

Morlinest commented Oct 10, 2017

Uh oh!

daviian commented Oct 10, 2017

Uh oh!

Uh oh!

Morlinest commented Oct 4, 2017 •

edited

Loading

codecov-io commented Oct 4, 2017 •

edited

Loading

Morlinest Oct 4, 2017 •

edited

Loading

daviian commented Oct 7, 2017 •

edited

Loading

daviian commented Oct 8, 2017 •

edited

Loading