From c42c31a111756c931292a404feabf24750d886e0 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 11 May 2020 23:04:08 +0100 Subject: [PATCH] Correctly set the organization num repos (#11339) * Correctly set the organization num repos Correctly set the organization num repos to the number of accessible repos for the user Fix #11194 Signed-off-by: Andrew Thornton * as per @lunny Signed-off-by: Andrew Thornton * attempt to fix mssql Signed-off-by: Andrew Thornton * Update models/user.go * Explicit columns Signed-off-by: Andrew Thornton * Add test and fix 0 counted orgs Signed-off-by: Andrew Thornton * remove orgname from api Signed-off-by: Andrew Thornton --- .../api_helper_for_declarative_test.go | 83 +++++++++++ integrations/org_count_test.go | 140 ++++++++++++++++++ models/user.go | 50 ++++++- routers/api/v1/api.go | 6 +- 4 files changed, 268 insertions(+), 11 deletions(-) create mode 100644 integrations/org_count_test.go diff --git a/integrations/api_helper_for_declarative_test.go b/integrations/api_helper_for_declarative_test.go index cae7691c4..ec7f1d749 100644 --- a/integrations/api_helper_for_declarative_test.go +++ b/integrations/api_helper_for_declarative_test.go @@ -266,3 +266,86 @@ func doAPICreateFile(ctx APITestContext, treepath string, options *api.CreateFil } } } + +func doAPICreateOrganization(ctx APITestContext, options *api.CreateOrgOption, callback ...func(*testing.T, api.Organization)) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/orgs?token=%s", ctx.Token) + + req := NewRequestWithJSON(t, "POST", url, &options) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + resp := ctx.Session.MakeRequest(t, req, http.StatusCreated) + + var contents api.Organization + DecodeJSON(t, resp, &contents) + if len(callback) > 0 { + callback[0](t, contents) + } + } +} + +func doAPICreateOrganizationRepository(ctx APITestContext, orgName string, options *api.CreateRepoOption, callback ...func(*testing.T, api.Repository)) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/orgs/%s/repos?token=%s", orgName, ctx.Token) + + req := NewRequestWithJSON(t, "POST", url, &options) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + resp := ctx.Session.MakeRequest(t, req, http.StatusCreated) + + var contents api.Repository + DecodeJSON(t, resp, &contents) + if len(callback) > 0 { + callback[0](t, contents) + } + } +} + +func doAPICreateOrganizationTeam(ctx APITestContext, orgName string, options *api.CreateTeamOption, callback ...func(*testing.T, api.Team)) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/orgs/%s/teams?token=%s", orgName, ctx.Token) + + req := NewRequestWithJSON(t, "POST", url, &options) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + resp := ctx.Session.MakeRequest(t, req, http.StatusCreated) + + var contents api.Team + DecodeJSON(t, resp, &contents) + if len(callback) > 0 { + callback[0](t, contents) + } + } +} + +func doAPIAddUserToOrganizationTeam(ctx APITestContext, teamID int64, username string) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/teams/%d/members/%s?token=%s", teamID, username, ctx.Token) + + req := NewRequest(t, "PUT", url) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + ctx.Session.MakeRequest(t, req, http.StatusNoContent) + } +} + +func doAPIAddRepoToOrganizationTeam(ctx APITestContext, teamID int64, orgName, repoName string) func(t *testing.T) { + return func(t *testing.T) { + url := fmt.Sprintf("/api/v1/teams/%d/repos/%s/%s?token=%s", teamID, orgName, repoName, ctx.Token) + + req := NewRequest(t, "PUT", url) + if ctx.ExpectedCode != 0 { + ctx.Session.MakeRequest(t, req, ctx.ExpectedCode) + return + } + ctx.Session.MakeRequest(t, req, http.StatusNoContent) + } +} diff --git a/integrations/org_count_test.go b/integrations/org_count_test.go new file mode 100644 index 000000000..755ee3cee --- /dev/null +++ b/integrations/org_count_test.go @@ -0,0 +1,140 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package integrations + +import ( + "net/url" + "strings" + "testing" + + "code.gitea.io/gitea/models" + api "code.gitea.io/gitea/modules/structs" + "github.com/stretchr/testify/assert" +) + +func TestOrgCounts(t *testing.T) { + onGiteaRun(t, testOrgCounts) +} + +func testOrgCounts(t *testing.T, u *url.URL) { + orgOwner := "user2" + orgName := "testOrg" + orgCollaborator := "user4" + ctx := NewAPITestContext(t, orgOwner, "repo1") + + var ownerCountRepos map[string]int + var collabCountRepos map[string]int + + t.Run("GetTheOwnersNumRepos", doCheckOrgCounts(orgOwner, map[string]int{}, + false, + func(_ *testing.T, calcOrgCounts map[string]int) { + ownerCountRepos = calcOrgCounts + }, + )) + t.Run("GetTheCollaboratorsNumRepos", doCheckOrgCounts(orgCollaborator, map[string]int{}, + false, + func(_ *testing.T, calcOrgCounts map[string]int) { + collabCountRepos = calcOrgCounts + }, + )) + + t.Run("CreatePublicTestOrganization", doAPICreateOrganization(ctx, &api.CreateOrgOption{ + UserName: orgName, + Visibility: "public", + })) + + // Following the creation of the organization, the orgName must appear in the counts with 0 repos + ownerCountRepos[orgName] = 0 + + t.Run("AssertNumRepos0ForTestOrg", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + + // the collaborator is not a collaborator yet + t.Run("AssertNoTestOrgReposForCollaborator", doCheckOrgCounts(orgCollaborator, collabCountRepos, true)) + + t.Run("CreateOrganizationPrivateRepo", doAPICreateOrganizationRepository(ctx, orgName, &api.CreateRepoOption{ + Name: "privateTestRepo", + AutoInit: true, + Private: true, + })) + + ownerCountRepos[orgName] = 1 + t.Run("AssertNumRepos1ForTestOrg", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + + t.Run("AssertNoTestOrgReposForCollaborator", doCheckOrgCounts(orgCollaborator, collabCountRepos, true)) + + var testTeam api.Team + + t.Run("CreateTeamForPublicTestOrganization", doAPICreateOrganizationTeam(ctx, orgName, &api.CreateTeamOption{ + Name: "test", + Permission: "read", + Units: []string{"repo.code", "repo.issues", "repo.wiki", "repo.pulls", "repo.releases"}, + CanCreateOrgRepo: true, + }, func(_ *testing.T, team api.Team) { + testTeam = team + })) + + t.Run("AssertNoTestOrgReposForCollaborator", doCheckOrgCounts(orgCollaborator, collabCountRepos, true)) + + t.Run("AddCollboratorToTeam", doAPIAddUserToOrganizationTeam(ctx, testTeam.ID, orgCollaborator)) + + collabCountRepos[orgName] = 0 + t.Run("AssertNumRepos0ForTestOrgForCollaborator", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + + // Now create a Public Repo + t.Run("CreateOrganizationPublicRepo", doAPICreateOrganizationRepository(ctx, orgName, &api.CreateRepoOption{ + Name: "publicTestRepo", + AutoInit: true, + })) + + ownerCountRepos[orgName] = 2 + t.Run("AssertNumRepos2ForTestOrg", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + collabCountRepos[orgName] = 1 + t.Run("AssertNumRepos1ForTestOrgForCollaborator", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + + // Now add the testTeam to the privateRepo + t.Run("AddTestTeamToPrivateRepo", doAPIAddRepoToOrganizationTeam(ctx, testTeam.ID, orgName, "privateTestRepo")) + + t.Run("AssertNumRepos2ForTestOrg", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) + collabCountRepos[orgName] = 2 + t.Run("AssertNumRepos2ForTestOrgForCollaborator", doCheckOrgCounts(orgOwner, ownerCountRepos, true)) +} + +func doCheckOrgCounts(username string, orgCounts map[string]int, strict bool, callback ...func(*testing.T, map[string]int)) func(t *testing.T) { + canonicalCounts := make(map[string]int, len(orgCounts)) + + for key, value := range orgCounts { + newKey := strings.TrimSpace(strings.ToLower(key)) + canonicalCounts[newKey] = value + } + + return func(t *testing.T) { + user := models.AssertExistsAndLoadBean(t, &models.User{ + Name: username, + }).(*models.User) + + user.GetOrganizations(&models.SearchOrganizationsOptions{All: true}) + + calcOrgCounts := map[string]int{} + + for _, org := range user.Orgs { + calcOrgCounts[org.LowerName] = org.NumRepos + count, ok := canonicalCounts[org.LowerName] + if ok { + assert.True(t, count == org.NumRepos, "Number of Repos in %s is %d when we expected %d", org.Name, org.NumRepos, count) + } else { + assert.False(t, strict, "Did not expect to see %s with count %d", org.Name, org.NumRepos) + } + } + + for key, value := range orgCounts { + _, seen := calcOrgCounts[strings.TrimSpace(strings.ToLower(key))] + assert.True(t, seen, "Expected to see %s with %d but did not", key, value) + } + + if len(callback) > 0 { + callback[0](t, calcOrgCounts) + } + } +} diff --git a/models/user.go b/models/user.go index a89eb144c..01cce2506 100644 --- a/models/user.go +++ b/models/user.go @@ -712,18 +712,52 @@ func (u *User) GetOwnedOrganizations() (err error) { // GetOrganizations returns paginated organizations that user belongs to. func (u *User) GetOrganizations(opts *SearchOrganizationsOptions) error { - ous, err := GetOrgUsersByUserID(u.ID, opts) + sess := x.NewSession() + defer sess.Close() + + schema, err := x.TableInfo(new(User)) if err != nil { return err } - - u.Orgs = make([]*User, len(ous)) - for i, ou := range ous { - u.Orgs[i], err = GetUserByID(ou.OrgID) - if err != nil { - return err - } + groupByCols := &strings.Builder{} + for _, col := range schema.Columns() { + fmt.Fprintf(groupByCols, "`%s`.%s,", schema.Name, col.Name) } + groupByStr := groupByCols.String() + groupByStr = groupByStr[0 : len(groupByStr)-1] + + sess.Select("`user`.*, count(repo_id) as org_count"). + Table("user"). + Join("INNER", "org_user", "`org_user`.org_id=`user`.id"). + Join("LEFT", builder. + Select("id as repo_id, owner_id as repo_owner_id"). + From("repository"). + Where(accessibleRepositoryCondition(u)), "`repository`.repo_owner_id = `org_user`.org_id"). + And("`org_user`.uid=?", u.ID). + GroupBy(groupByStr) + if opts.PageSize != 0 { + sess = opts.setSessionPagination(sess) + } + type OrgCount struct { + User `xorm:"extends"` + OrgCount int + } + orgCounts := make([]*OrgCount, 0, 10) + + if err := sess. + Asc("`user`.name"). + Find(&orgCounts); err != nil { + return err + } + + orgs := make([]*User, len(orgCounts)) + for i, orgCount := range orgCounts { + orgCount.User.NumRepos = orgCount.OrgCount + orgs[i] = &orgCount.User + } + + u.Orgs = orgs + return nil } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 6973e1df5..0d62b751c 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -383,7 +383,7 @@ func orgAssignment(args ...bool) macaron.Handler { var err error if assignOrg { - ctx.Org.Organization, err = models.GetOrgByName(ctx.Params(":orgname")) + ctx.Org.Organization, err = models.GetOrgByName(ctx.Params(":org")) if err != nil { if models.IsErrOrgNotExist(err) { ctx.NotFound() @@ -857,7 +857,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/users/:username/orgs", org.ListUserOrgs) m.Post("/orgs", reqToken(), bind(api.CreateOrgOption{}), org.Create) m.Get("/orgs", org.GetAll) - m.Group("/orgs/:orgname", func() { + m.Group("/orgs/:org", func() { m.Combo("").Get(org.Get). Patch(reqToken(), reqOrgOwnership(), bind(api.EditOrgOption{}), org.Edit). Delete(reqToken(), reqOrgOwnership(), org.Delete) @@ -907,7 +907,7 @@ func RegisterRoutes(m *macaron.Macaron) { }) m.Group("/repos", func() { m.Get("", org.GetTeamRepos) - m.Combo("/:orgname/:reponame"). + m.Combo("/:org/:reponame"). Put(org.AddTeamRepository). Delete(org.RemoveTeamRepository) })