Add repository type option to /api/repo/search #2326

Morlinest · 2017-08-17T16:07:40Z

First attempt to solve #2321. I want to check, if this way is OK.

lafriks · 2017-08-18T00:13:24Z

models/repo_list.go

 }

+type RepoType int


Missing comments for type and constants below

Comments added

Morlinest · 2017-08-18T08:19:35Z

Added some comments and rename RepoTypeAll to RepoTypeAny. BTW it's working solution :).

lafriks · 2017-08-19T17:27:43Z

Can you add integration tests for this?

Morlinest · 2017-08-19T18:33:29Z

@lafriks I can try. I have to learn what's in test db first and how to do it right.

Morlinest · 2017-08-19T20:02:31Z

@lafriks I can't even run tests in gitea, so no...

Morlinest · 2017-08-20T13:05:33Z

Rebased on master

codecov-io · 2017-09-16T14:59:14Z

Codecov Report

Merging #2326 into master will increase coverage by 0.28%.
The diff coverage is 63.01%.

@@            Coverage Diff             @@
##           master    #2326      +/-   ##
==========================================
+ Coverage   27.32%   27.61%   +0.28%     
==========================================
  Files          86       86              
  Lines       17135    17112      -23     
==========================================
+ Hits         4682     4725      +43     
+ Misses      11775    11709      -66     
  Partials      678      678

Impacted Files	Coverage Δ
routers/user/profile.go	`0% <0%> (ø)`	⬆️
models/repo_list.go	`51.59% <73.01%> (+27.7%)`	⬆️
models/user.go	`18.67% <0%> (+0.21%)`	⬆️
models/consistency.go	`96.55% <0%> (+2.58%)`	⬆️

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 1eedd98...19c15bb. Read the comment docs.

Morlinest · 2017-09-16T16:14:15Z

Rebased on top of #2371. Only last commit is new. I'll create new tests for added feature soon.

daviian

Added my thoughts ;-)
And is the huge amount of new test data really necessary for your tests?

daviian · 2017-09-18T20:15:13Z

models/repo_list.go

+	}
+
+	// Set again after reset by Count()
+	if includeStarred {
 		sess.Join("INNER", "star", "star.repo_id = repository.id")


IMO includeStarred is misleading as it doesn't include starred, it restricts to starred repositories.
Suggest changing to something like includeOnlyStarred !?

I think this part is only about adding/joining "starred" table to SQL. It doesn't restrict anything. Maybe renaming it to includeStarredTable or joinStarredTable would be better.

Inner join actually does restrict only to records that are in started table

@lafriks True, I was thinking about left join in the future update... My mistake.

daviian · 2017-09-18T20:39:04Z

models/repo_list.go

@@ -217,7 +279,7 @@ func Repositories(opts *SearchRepoOptions) (_ RepositoryList, count int64, err e

 	if err = x.
 		Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
-		OrderBy(opts.OrderBy).
+		OrderBy(opts.OrderBy.String()).


Doesn't this lead to an error if len(opts.OrderBy) == 0 evaluates to true as "id ASC".String() does not exist?

I think no. opts.OrderBy is custom type SearchOrderBy, you can't pass just string and it can't be of zero length here, because there is check for that above:

if len(opts.OrderBy) == 0 { opts.OrderBy = SearchOrderByAlphabetically }

But in https://github.com/Morlinest/gitea/blob/0baae9f69e2e54eb4d9fb3329b188f5f9a88bcd9/models/repo_list.go#L275
you it gets set to "id ASC" before calling .String()

You are right, I was looking at other place... Answer: It will not fail :). You can put any string there.

daviian · 2017-09-18T20:59:04Z

routers/home.go

@@ -61,7 +61,6 @@ func Swagger(ctx *context.Context) {
 // RepoSearchOptions when calling search repositories
 type RepoSearchOptions struct {
 	Ranger   func(*models.SearchRepoOptions) (models.RepositoryList, int64, error)


AFAIK Ranger is not needed anymore and can be generally be removed from RepoSearchOptions and thus at

gitea/routers/admin/repos.go

Line 27 in 566e8ec

Ranger: models.Repositories,

too.

Yes, but I'll remove more redudant functions in another PR (I mentioned it in #2371). 1 removed function is enough right now (as it belongs to #2371). This PR (it's not based on master) is about adding feature.

daviian · 2017-09-18T21:22:00Z

integrations/api_repo_test.go

+	for _, repo := range body.Data {
+		assert.Contains(t, repo.Name, keyword)
+		assert.False(t, repo.Private)
+	}


This test should also be added to your testCases list

I am thinking (last few days) about extracting this change to new PR because actually it's fix for non working test.

daviian · 2017-09-18T21:28:53Z

models/repo_list_test.go

 	assert.NoError(t, err)
 	assert.Equal(t, int64(3), count)
+	assert.Len(t, repos, 3)


Add existing tests to testCases.

I don't want to do it in this (#2371) PR because:

I want to show my PR will pass existing tests without changes

For moving existing tests to my added test cases it would be needed fixes at least in "repository.yml" and "access.yml".

lunny · 2017-09-22T07:49:27Z

@Morlinest why close this PR?

Morlinest · 2017-09-29T22:46:43Z

@lunny I'll reopen this PR once needed changes will be made and merged (#2321, #2371).

lafriks requested changes Aug 18, 2017

View reviewed changes

Morlinest force-pushed the feature-api-repo-search-options branch from c0a0ada to 9a50260 Compare August 18, 2017 08:17

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

Morlinest force-pushed the feature-api-repo-search-options branch from 9a50260 to 90e1e5b Compare August 18, 2017 08:36

Morlinest mentioned this pull request Aug 18, 2017

Add number of repos for every filter type #2312

Closed

Morlinest force-pushed the feature-api-repo-search-options branch from 90e1e5b to 94f563b Compare August 18, 2017 12:54

lunny added this to the 1.3.0 milestone Aug 20, 2017

Morlinest force-pushed the feature-api-repo-search-options branch from 8cc9ea4 to 4ad515f Compare August 20, 2017 13:04

Morlinest mentioned this pull request Aug 20, 2017

Use custom search for each filter type in dashboard #2343

Merged

Morlinest force-pushed the feature-api-repo-search-options branch from 4ad515f to e07d6e2 Compare August 21, 2017 21:08

Morlinest mentioned this pull request Aug 25, 2017

Refactor searchRepositoryByName function and remove another redundant function #2371

Closed

Morlinest force-pushed the feature-api-repo-search-options branch from e07d6e2 to 43b77fe Compare September 16, 2017 12:46

Morlinest changed the title ~~Add repo type search option to repo/search api~~ Add repository type option to /api/repo/search Sep 16, 2017

Morlinest force-pushed the feature-api-repo-search-options branch 2 times, most recently from f4cc1bb to 144c507 Compare September 16, 2017 14:40

lafriks approved these changes Sep 16, 2017

View reviewed changes

daviian reviewed Sep 18, 2017

View reviewed changes

Morlinest force-pushed the feature-api-repo-search-options branch 2 times, most recently from 0baae9f to 2c8263c Compare September 20, 2017 14:37

Morlinest added 4 commits September 21, 2017 11:28

Add more repo search tests

a50f7a8

Add and fix /api/repo/search integration tests

50867df

Add owner to access table for organization repositories

2d8b046

Fix repo search tests

27f037d

Morlinest added 17 commits September 21, 2017 11:28

Always test returned repos length

4fcbded

Add test with lower pagesize limit (test more pages)

8cb7ce2

Fix/rewrite searchRepositoryByName function

dae998e

Remove authorization decisions making

7c9323d

Fix search of starred repositories

9da9e96

Simplify code

14f73a3

Remove "Searcher" option from SearchRepoOptions

3982925

Fix visibility of organizations repositories

41d4838

Simplify searching

886033f

Fix /api/repo/search checks

c091ee3

Remove redudant condition

4af3945

Fix access condition expression

11ef70a

Simplify TestSearchRepositoryByName code

76e3d48

Simplify and unify TestAPISearchRepo code

ce0fe4e

Add specific tests to improve test coverage

ab5d7cb

Add repo type option to /api/repo/search

62076a3

Add tests and fix result of collaborative filter in specific condition

19c15bb

Morlinest force-pushed the feature-api-repo-search-options branch from 2c8263c to 19c15bb Compare September 21, 2017 09:48

Morlinest closed this Sep 22, 2017

Morlinest mentioned this pull request Oct 21, 2017

Add search mode option to /api/repo/search #2756

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add repository type option to /api/repo/search #2326

Add repository type option to /api/repo/search #2326

Morlinest commented Aug 17, 2017

lafriks Aug 18, 2017

Morlinest Aug 19, 2017

Morlinest commented Aug 18, 2017

lafriks commented Aug 19, 2017

Morlinest commented Aug 19, 2017

Morlinest commented Aug 19, 2017

Morlinest commented Aug 20, 2017

codecov-io commented Sep 16, 2017 •

edited

Loading

Morlinest commented Sep 16, 2017

daviian left a comment •

edited

Loading

daviian Sep 18, 2017

Morlinest Sep 19, 2017

lafriks Sep 19, 2017

Morlinest Sep 19, 2017

daviian Sep 18, 2017

Morlinest Sep 19, 2017 •

edited

Loading

daviian Sep 19, 2017

Morlinest Sep 19, 2017

daviian Sep 18, 2017

Morlinest Sep 19, 2017

daviian Sep 18, 2017

Morlinest Sep 19, 2017

daviian Sep 18, 2017

Morlinest Sep 19, 2017

lunny commented Sep 22, 2017

Morlinest commented Sep 29, 2017

Add repository type option to /api/repo/search #2326

Add repository type option to /api/repo/search #2326

Conversation

Morlinest commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morlinest commented Aug 18, 2017

lafriks commented Aug 19, 2017

Morlinest commented Aug 19, 2017

Morlinest commented Aug 19, 2017

Morlinest commented Aug 20, 2017

codecov-io commented Sep 16, 2017 • edited Loading

Codecov Report

Morlinest commented Sep 16, 2017

daviian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morlinest Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Sep 22, 2017

Morlinest commented Sep 29, 2017

codecov-io commented Sep 16, 2017 •

edited

Loading

daviian left a comment •

edited

Loading

Morlinest Sep 19, 2017 •

edited

Loading