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 <art27@cantab.net>

* as per @lunny

Signed-off-by: Andrew Thornton <art27@cantab.net>

* attempt to fix mssql

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Update models/user.go

* Explicit columns

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Add test and fix 0 counted orgs

Signed-off-by: Andrew Thornton <art27@cantab.net>

* remove orgname from api

Signed-off-by: Andrew Thornton <art27@cantab.net>

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)
+	}
+}

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)
+		}
+	}
+}

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
 	}
+	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]
 
-	u.Orgs = make([]*User, len(ous))
+	sess.Select("`user`.*, count(repo_id) as org_count").
-	for i, ou := range ous {
+		Table("user").
-		u.Orgs[i], err = GetUserByID(ou.OrgID)
+		Join("INNER", "org_user", "`org_user`.org_id=`user`.id").
-		if err != nil {
+		Join("LEFT", builder.
-			return err
+			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
 }
 

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)
 			})