Skip to content

Add repository search unit and integration tests #2575

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

Merged
merged 13 commits into from
Oct 10, 2017

Conversation

Morlinest
Copy link
Member

Extracted from #2371. Adds only tests, no feature/fix. Please read FIXME comments (4x).

@lafriks lafriks added this to the 1.3.0 milestone Sep 22, 2017
@codecov-io
Copy link

codecov-io commented Sep 22, 2017

Codecov Report

Merging #2575 into master will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2575      +/-   ##
==========================================
+ Coverage   27.13%   27.28%   +0.15%     
==========================================
  Files          86       86              
  Lines       17062    17062              
==========================================
+ Hits         4629     4655      +26     
+ Misses      11755    11728      -27     
- Partials      678      679       +1
Impacted Files Coverage Δ
models/user.go 18.89% <0%> (+0.65%) ⬆️
models/repo_list.go 35.71% <0%> (+10.98%) ⬆️

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 0cef8ce...d4bd8d7. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 22, 2017
user2: {count: 4, repoName: "big_test_", privacy: privacyTypePublic}},
},
{name: "RepositoriesAccessibleAndRelatedToUser", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user.ID), expectedResults: expectedResults{
// FIXME: Should return 4 (all public repositories related to "another" user = owned + collaborative), now returns only public repositories directly owned by user
Copy link
Member

Choose a reason for hiding this comment

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

but here is no user login, what's collaborative?

Copy link
Member Author

Choose a reason for hiding this comment

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

Repositories related to user.ID. In my opinion logged user is here only to check rights. That means logged user and anonymous user has same rights if searching for repositories related to another user.

Copy link
Member

@daviian daviian Sep 24, 2017

Choose a reason for hiding this comment

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

But collaborative also includes repositories the user has contributed to. Don't think thats right.
IMO the question is: transitive owning!? (user owns org, org owns repo => user owns repo?) @bkcsoft and me have discussed that already in discord.
His and my opinion was to include org-repos as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@daviian It includes repositories, for which user has some rights through teams. In dashboard, collaborative is defined as ownerID != userID and Source as ownerID == userID. You don't know that user is owner of organization from api/repo/search result.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you own organization, you are automaticaly in Owners team.

Copy link
Member

@daviian daviian Sep 25, 2017

Choose a reason for hiding this comment

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

I think there should be a vote on the expected behaviour of this API call.
@go-gitea/owners

Copy link
Member

@daviian daviian Oct 7, 2017

Choose a reason for hiding this comment

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

@lunny @Morlinest I compared the result to githubs implementation. Github doesn't include any repositories from organizations (even owned ones) when searching for repositories of a user. So I think 2 is correct.

Please add check for repoOwnerID

Copy link
Member

Choose a reason for hiding this comment

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

Since we need collaborative for our dashboard we should include it in "public" search as well, but limited to public orgs and membership and public repositories.

count: 8},
{name: "PublicRepositoriesOfUser",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15},
count: 3}, // FIXME: Should return 2 (only directly owned repositories), now includes 1 public repository from owned organization
Copy link
Member

Choose a reason for hiding this comment

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

Same comment from above. Transitive owning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Organization and other repositories related to user should be included only if collaborate option is supplied.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Should be 2 here.

@Morlinest Morlinest force-pushed the feature/add-repo-search-tests branch 2 times, most recently from 8e0c288 to 40efd7c Compare October 2, 2017 21:04
user2: {count: 4, repoName: "big_test_", privacy: privacyTypePublic}},
},
{name: "RepositoriesAccessibleAndRelatedToUser", requestURL: fmt.Sprintf("/api/v1/repos/search?uid=%d", user.ID), expectedResults: expectedResults{
// FIXME: Should return 4 (all public repositories related to "another" user = owned + collaborative), now returns only public repositories directly owned by user
Copy link
Member

@daviian daviian Oct 7, 2017

Choose a reason for hiding this comment

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

@lunny @Morlinest I compared the result to githubs implementation. Github doesn't include any repositories from organizations (even owned ones) when searching for repositories of a user. So I think 2 is correct.

Please add check for repoOwnerID

count: 8},
{name: "PublicRepositoriesOfUser",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15},
count: 3}, // FIXME: Should return 2 (only directly owned repositories), now includes 1 public repository from owned organization
Copy link
Member

Choose a reason for hiding this comment

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

You are right. Should be 2 here.

count: 3}, // FIXME: Should return 2 (only directly owned repositories), now includes 1 public repository from owned organization
{name: "PublicAndPrivateRepositoriesOfUser",
opts: &SearchRepoOptions{Page: 1, PageSize: 10, OwnerID: 15, Private: true},
count: 6}, // FIXME: Should return 4 (only directly owned repositories), now includes 2 repositories from owned organization
Copy link
Member

Choose a reason for hiding this comment

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

You are right. Should be 4 here.

nil: {count: 1, privacy: privacyTypePublic},
user: {count: 1, privacy: privacyTypePublic},
user2: {count: 2}},
},
Copy link
Member

@daviian daviian Oct 7, 2017

Choose a reason for hiding this comment

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

Please add check for repoOwnerID

Copy link
Member

Choose a reason for hiding this comment

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

Not possible, since includes collaborative too


// test search private repository on explore page
repos, count, err = SearchRepositoryByName(&SearchRepoOptions{
Keyword: "repo_13",
Page: 1,
PageSize: 10,
Private: true,
Searcher: &User{ID: 14},
Copy link
Member

Choose a reason for hiding this comment

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

IMO you should leave it as it is.
Although the implementation doesn't need it because no OwnerID i set, but it could be a test that it has no impact.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@@ -60,10 +55,66 @@ func TestSearchRepositoryByName(t *testing.T) {
Page: 1,
PageSize: 10,
Private: true,
Searcher: &User{ID: 14},
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

count int
}{
{name: "PublicRepositoriesByName",
opts: &SearchRepoOptions{Keyword: "big_test_", PageSize: 10},
Copy link
Member

Choose a reason for hiding this comment

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

You should add similar checks as in api_repo_tests for all test cases. E.g. repo name, private/public repositories

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@daviian
Copy link
Member

daviian commented Oct 7, 2017

@Morlinest Please rebase :)

@Morlinest Morlinest force-pushed the feature/add-repo-search-tests branch from 40efd7c to 198c2f9 Compare October 8, 2017 09:40
@daviian
Copy link
Member

daviian commented Oct 9, 2017

LGTM

@tboerger 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 9, 2017
@Morlinest Morlinest force-pushed the feature/add-repo-search-tests branch from 0bae86d to d4bd8d7 Compare October 9, 2017 14:26
@lunny
Copy link
Member

lunny commented Oct 10, 2017

LGTM

@tboerger 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
@lunny lunny merged commit c2346e4 into go-gitea:master Oct 10, 2017
@Morlinest Morlinest deleted the feature/add-repo-search-tests branch October 10, 2017 01:36
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
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. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants