[Refactor] convert team(s) to apiTeam(s) (#13745)

* Refactor: teams to api convert

* make org load optional

* more info in tests

integrations/api_team_test.go

@@ -69,7 +69,7 @@ func TestAPITeam(t *testing.T) {
 	resp = session.MakeRequest(t, req, http.StatusCreated)
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
-	checkTeamResponse(t, &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
+	checkTeamResponse(t, "CreateTeam1", &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
 		teamToCreate.Permission, teamToCreate.Units, nil)
 	checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
 		teamToCreate.Permission, teamToCreate.Units, nil)
@@ -90,7 +90,7 @@ func TestAPITeam(t *testing.T) {
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
-	checkTeamResponse(t, &apiTeam, teamToEdit.Name, *teamToEdit.Description, *teamToEdit.IncludesAllRepositories,
+	checkTeamResponse(t, "EditTeam1", &apiTeam, teamToEdit.Name, *teamToEdit.Description, *teamToEdit.IncludesAllRepositories,
 		teamToEdit.Permission, unit.AllUnitKeyNames(), nil)
 	checkTeamBean(t, apiTeam.ID, teamToEdit.Name, *teamToEdit.Description, *teamToEdit.IncludesAllRepositories,
 		teamToEdit.Permission, unit.AllUnitKeyNames(), nil)
@@ -102,7 +102,7 @@ func TestAPITeam(t *testing.T) {
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
-	checkTeamResponse(t, &apiTeam, teamToEdit.Name, *teamToEditDesc.Description, *teamToEdit.IncludesAllRepositories,
+	checkTeamResponse(t, "EditTeam1_DescOnly", &apiTeam, teamToEdit.Name, *teamToEditDesc.Description, *teamToEdit.IncludesAllRepositories,
 		teamToEdit.Permission, unit.AllUnitKeyNames(), nil)
 	checkTeamBean(t, apiTeam.ID, teamToEdit.Name, *teamToEditDesc.Description, *teamToEdit.IncludesAllRepositories,
 		teamToEdit.Permission, unit.AllUnitKeyNames(), nil)
@@ -114,7 +114,7 @@ func TestAPITeam(t *testing.T) {
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
-	checkTeamResponse(t, &apiTeam, teamRead.Name, *teamToEditDesc.Description, teamRead.IncludesAllRepositories,
+	checkTeamResponse(t, "ReadTeam1", &apiTeam, teamRead.Name, *teamToEditDesc.Description, teamRead.IncludesAllRepositories,
 		teamRead.AccessMode.String(), teamRead.GetUnitNames(), teamRead.GetUnitsMap())
 
 	// Delete team.
@@ -135,7 +135,7 @@ func TestAPITeam(t *testing.T) {
 	resp = session.MakeRequest(t, req, http.StatusCreated)
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
-	checkTeamResponse(t, &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
+	checkTeamResponse(t, "CreateTeam2", &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
 		"read", nil, teamToCreate.UnitsMap)
 	checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
 		"read", nil, teamToCreate.UnitsMap)
@@ -156,7 +156,7 @@ func TestAPITeam(t *testing.T) {
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
-	checkTeamResponse(t, &apiTeam, teamToEdit.Name, *teamToEdit.Description, *teamToEdit.IncludesAllRepositories,
+	checkTeamResponse(t, "EditTeam2", &apiTeam, teamToEdit.Name, *teamToEdit.Description, *teamToEdit.IncludesAllRepositories,
 		"read", nil, teamToEdit.UnitsMap)
 	checkTeamBean(t, apiTeam.ID, teamToEdit.Name, *teamToEdit.Description, *teamToEdit.IncludesAllRepositories,
 		"read", nil, teamToEdit.UnitsMap)
@@ -168,7 +168,7 @@ func TestAPITeam(t *testing.T) {
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
-	checkTeamResponse(t, &apiTeam, teamToEdit.Name, *teamToEditDesc.Description, *teamToEdit.IncludesAllRepositories,
+	checkTeamResponse(t, "EditTeam2_DescOnly", &apiTeam, teamToEdit.Name, *teamToEditDesc.Description, *teamToEdit.IncludesAllRepositories,
 		"read", nil, teamToEdit.UnitsMap)
 	checkTeamBean(t, apiTeam.ID, teamToEdit.Name, *teamToEditDesc.Description, *teamToEdit.IncludesAllRepositories,
 		"read", nil, teamToEdit.UnitsMap)
@@ -180,7 +180,7 @@ func TestAPITeam(t *testing.T) {
 	apiTeam = api.Team{}
 	DecodeJSON(t, resp, &apiTeam)
 	assert.NoError(t, teamRead.GetUnits())
-	checkTeamResponse(t, &apiTeam, teamRead.Name, *teamToEditDesc.Description, teamRead.IncludesAllRepositories,
+	checkTeamResponse(t, "ReadTeam2", &apiTeam, teamRead.Name, *teamToEditDesc.Description, teamRead.IncludesAllRepositories,
 		teamRead.AccessMode.String(), teamRead.GetUnitNames(), teamRead.GetUnitsMap())
 
 	// Delete team.
@@ -189,8 +189,8 @@ func TestAPITeam(t *testing.T) {
 	unittest.AssertNotExistsBean(t, &organization.Team{ID: teamID})
 }
 
-func checkTeamResponse(t *testing.T, apiTeam *api.Team, name, description string, includesAllRepositories bool, permission string, units []string, unitsMap map[string]string) {
+func checkTeamResponse(t *testing.T, testName string, apiTeam *api.Team, name, description string, includesAllRepositories bool, permission string, units []string, unitsMap map[string]string) {
-	t.Run(name+description, func(t *testing.T) {
+	t.Run(testName, func(t *testing.T) {
 		assert.Equal(t, name, apiTeam.Name, "name")
 		assert.Equal(t, description, apiTeam.Description, "description")
 		assert.Equal(t, includesAllRepositories, apiTeam.IncludesAllRepositories, "includesAllRepositories")
@@ -209,7 +209,9 @@ func checkTeamResponse(t *testing.T, apiTeam *api.Team, name, description string
 func checkTeamBean(t *testing.T, id int64, name, description string, includesAllRepositories bool, permission string, units []string, unitsMap map[string]string) {
 	team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: id}).(*organization.Team)
 	assert.NoError(t, team.GetUnits(), "GetUnits")
-	checkTeamResponse(t, convert.ToTeam(team), name, description, includesAllRepositories, permission, units, unitsMap)
+	apiTeam, err := convert.ToTeam(team)
+	assert.NoError(t, err)
+	checkTeamResponse(t, fmt.Sprintf("checkTeamBean/%s_%s", name, description), apiTeam, name, description, includesAllRepositories, permission, units, unitsMap)
 }
 
 type TeamSearchResults struct {

integrations/org_test.go

@@ -157,7 +157,7 @@ func TestOrgRestrictedUser(t *testing.T) {
 
 	resp := adminSession.MakeRequest(t, req, http.StatusCreated)
 	DecodeJSON(t, resp, &apiTeam)
-	checkTeamResponse(t, &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
+	checkTeamResponse(t, "CreateTeam_codereader", &apiTeam, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
 		teamToCreate.Permission, teamToCreate.Units, nil)
 	checkTeamBean(t, apiTeam.ID, teamToCreate.Name, teamToCreate.Description, teamToCreate.IncludesAllRepositories,
 		teamToCreate.Permission, teamToCreate.Units, nil)

modules/convert/convert.go

@@ -304,22 +304,53 @@ func ToOrganization(org *organization.Organization) *api.Organization {
 	}
 }
 
-// ToTeam convert organization.Team to api.Team
+// ToTeam convert models.Team to api.Team
-func ToTeam(team *organization.Team) *api.Team {
+func ToTeam(team *organization.Team, loadOrg ...bool) (*api.Team, error) {
-	if team == nil {
+	teams, err := ToTeams([]*organization.Team{team}, len(loadOrg) != 0 && loadOrg[0])
-		return nil
+	if err != nil || len(teams) == 0 {
+		return nil, err
+	}
+	return teams[0], nil
+}
+
+// ToTeams convert models.Team list to api.Team list
+func ToTeams(teams []*organization.Team, loadOrgs bool) ([]*api.Team, error) {
+	if len(teams) == 0 || teams[0] == nil {
+		return nil, nil
+	}
+
+	cache := make(map[int64]*api.Organization)
+	apiTeams := make([]*api.Team, len(teams))
+	for i := range teams {
+		if err := teams[i].GetUnits(); err != nil {
+			return nil, err
 		}
 
-	return &api.Team{
+		apiTeams[i] = &api.Team{
-		ID:                      team.ID,
+			ID:                      teams[i].ID,
-		Name:                    team.Name,
+			Name:                    teams[i].Name,
-		Description:             team.Description,
+			Description:             teams[i].Description,
-		IncludesAllRepositories: team.IncludesAllRepositories,
+			IncludesAllRepositories: teams[i].IncludesAllRepositories,
-		CanCreateOrgRepo:        team.CanCreateOrgRepo,
+			CanCreateOrgRepo:        teams[i].CanCreateOrgRepo,
-		Permission:              team.AccessMode.String(),
+			Permission:              teams[i].AccessMode.String(),
-		Units:                   team.GetUnitNames(),
+			Units:                   teams[i].GetUnitNames(),
-		UnitsMap:                team.GetUnitsMap(),
+			UnitsMap:                teams[i].GetUnitsMap(),
+		}
+
+		if loadOrgs {
+			apiOrg, ok := cache[teams[i].OrgID]
+			if !ok {
+				org, err := organization.GetOrgByID(teams[i].OrgID)
+				if err != nil {
+					return nil, err
+				}
+				apiOrg = ToOrganization(org)
+				cache[teams[i].OrgID] = apiOrg
+			}
+			apiTeams[i].Organization = apiOrg
+		}
 	}
+	return apiTeams, nil
 }
 
 // ToAnnotatedTag convert git.Tag to api.AnnotatedTag

modules/convert/issue_comment.go

@@ -152,7 +152,7 @@ func ToTimelineComment(c *models.Comment, doer *user_model.User) *api.TimelineCo
 		comment.Assignee = ToUser(c.Assignee, nil)
 	}
 	if c.AssigneeTeam != nil {
-		comment.AssigneeTeam = ToTeam(c.AssigneeTeam)
+		comment.AssigneeTeam, _ = ToTeam(c.AssigneeTeam)
 	}
 
 	if c.ResolveDoer != nil {

modules/convert/pull_review.go

@@ -22,10 +22,15 @@ func ToPullReview(ctx context.Context, r *models.Review, doer *user_model.User)
 		r.Reviewer = user_model.NewGhostUser()
 	}
 
+	apiTeam, err := ToTeam(r.ReviewerTeam)
+	if err != nil {
+		return nil, err
+	}
+
 	result := &api.PullReview{
 		ID:                r.ID,
 		Reviewer:          ToUser(r.Reviewer, doer),
-		ReviewerTeam:      ToTeam(r.ReviewerTeam),
+		ReviewerTeam:      apiTeam,
 		State:             api.ReviewStateUnknown,
 		Body:              r.Content,
 		CommitID:          r.CommitID,

modules/convert/repository.go

@@ -186,10 +186,7 @@ func innerToRepo(repo *repo_model.Repository, mode perm.AccessMode, isParent boo
 
 // ToRepoTransfer convert a models.RepoTransfer to a structs.RepeTransfer
 func ToRepoTransfer(t *models.RepoTransfer) *api.RepoTransfer {
-	var teams []*api.Team
+	teams, _ := ToTeams(t.Teams, false)
-	for _, v := range t.Teams {
-		teams = append(teams, ToTeam(v))
-	}
 
 	return &api.RepoTransfer{
 		Doer:      ToUser(t.Doer, nil),

routers/api/v1/org/team.go

@@ -58,16 +58,12 @@ func ListTeams(ctx *context.APIContext) {
 		return
 	}
 
-	apiTeams := make([]*api.Team, len(teams))
+	apiTeams, err := convert.ToTeams(teams, false)
-	for i := range teams {
+	if err != nil {
-		if err := teams[i].GetUnits(); err != nil {
+		ctx.Error(http.StatusInternalServerError, "ConvertToTeams", err)
-			ctx.Error(http.StatusInternalServerError, "GetUnits", err)
 		return
 	}
 
-		apiTeams[i] = convert.ToTeam(teams[i])
-	}
-
 	ctx.SetTotalCountHeader(count)
 	ctx.JSON(http.StatusOK, apiTeams)
 }
@@ -101,26 +97,11 @@ func ListUserTeams(ctx *context.APIContext) {
 		return
 	}
 
-	cache := make(map[int64]*api.Organization)
+	apiTeams, err := convert.ToTeams(teams, true)
-	apiTeams := make([]*api.Team, len(teams))
-	for i := range teams {
-		apiOrg, ok := cache[teams[i].OrgID]
-		if !ok {
-			org, err := organization.GetOrgByID(teams[i].OrgID)
 	if err != nil {
-				ctx.Error(http.StatusInternalServerError, "GetUserByID", err)
+		ctx.Error(http.StatusInternalServerError, "ConvertToTeams", err)
-				return
-			}
-			apiOrg = convert.ToOrganization(org)
-			cache[teams[i].OrgID] = apiOrg
-		}
-		if err := teams[i].GetUnits(); err != nil {
-			ctx.Error(http.StatusInternalServerError, "teams[i].GetUnits()", err)
 		return
 	}
-		apiTeams[i] = convert.ToTeam(teams[i])
-		apiTeams[i].Organization = apiOrg
-	}
 
 	ctx.SetTotalCountHeader(count)
 	ctx.JSON(http.StatusOK, apiTeams)
@@ -144,12 +125,13 @@ func GetTeam(ctx *context.APIContext) {
 	//   "200":
 	//     "$ref": "#/responses/Team"
 
-	if err := ctx.Org.Team.GetUnits(); err != nil {
+	apiTeam, err := convert.ToTeam(ctx.Org.Team)
-		ctx.Error(http.StatusInternalServerError, "team.GetUnits", err)
+	if err != nil {
+		ctx.InternalServerError(err)
 		return
 	}
 
-	ctx.JSON(http.StatusOK, convert.ToTeam(ctx.Org.Team))
+	ctx.JSON(http.StatusOK, apiTeam)
 }
 
 func attachTeamUnits(team *organization.Team, units []string) {
@@ -241,7 +223,12 @@ func CreateTeam(ctx *context.APIContext) {
 		return
 	}
 
-	ctx.JSON(http.StatusCreated, convert.ToTeam(team))
+	apiTeam, err := convert.ToTeam(team)
+	if err != nil {
+		ctx.InternalServerError(err)
+		return
+	}
+	ctx.JSON(http.StatusCreated, apiTeam)
 }
 
 // EditTeam api for edit a team
@@ -318,7 +305,13 @@ func EditTeam(ctx *context.APIContext) {
 		ctx.Error(http.StatusInternalServerError, "EditTeam", err)
 		return
 	}
-	ctx.JSON(http.StatusOK, convert.ToTeam(team))
+
+	apiTeam, err := convert.ToTeam(team)
+	if err != nil {
+		ctx.InternalServerError(err)
+		return
+	}
+	ctx.JSON(http.StatusOK, apiTeam)
 }
 
 // DeleteTeam api for delete a team
@@ -782,18 +775,11 @@ func SearchTeam(ctx *context.APIContext) {
 		return
 	}
 
-	apiTeams := make([]*api.Team, len(teams))
+	apiTeams, err := convert.ToTeams(teams, false)
-	for i := range teams {
+	if err != nil {
-		if err := teams[i].GetUnits(); err != nil {
+		ctx.InternalServerError(err)
-			log.Error("Team GetUnits failed: %v", err)
-			ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
-				"ok":    false,
-				"error": "SearchTeam failed to get units",
-			})
 		return
 	}
-		apiTeams[i] = convert.ToTeam(teams[i])
-	}
 
 	ctx.SetLinkHeader(int(maxResults), listOptions.PageSize)
 	ctx.SetTotalCountHeader(maxResults)

routers/api/v1/repo/teams.go

@@ -12,7 +12,6 @@ import (
 	"code.gitea.io/gitea/models/organization"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/convert"
-	api "code.gitea.io/gitea/modules/structs"
 )
 
 // ListTeams list a repository's teams
@@ -48,16 +47,12 @@ func ListTeams(ctx *context.APIContext) {
 		return
 	}
 
-	apiTeams := make([]*api.Team, len(teams))
+	apiTeams, err := convert.ToTeams(teams, false)
-	for i := range teams {
+	if err != nil {
-		if err := teams[i].GetUnits(); err != nil {
+		ctx.InternalServerError(err)
-			ctx.Error(http.StatusInternalServerError, "GetUnits", err)
 		return
 	}
 
-		apiTeams[i] = convert.ToTeam(teams[i])
-	}
-
 	ctx.JSON(http.StatusOK, apiTeams)
 }
 
@@ -103,11 +98,11 @@ func IsTeam(ctx *context.APIContext) {
 	}
 
 	if models.HasRepository(team, ctx.Repo.Repository.ID) {
-		if err := team.GetUnits(); err != nil {
+		apiTeam, err := convert.ToTeam(team)
-			ctx.Error(http.StatusInternalServerError, "GetUnits", err)
+		if err != nil {
+			ctx.InternalServerError(err)
 			return
 		}
-		apiTeam := convert.ToTeam(team)
 		ctx.JSON(http.StatusOK, apiTeam)
 		return
 	}

routers/web/org/teams.go

@@ -23,7 +23,6 @@ import (
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/convert"
 	"code.gitea.io/gitea/modules/log"
-	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/utils"
 	"code.gitea.io/gitea/services/forms"
@@ -357,18 +356,15 @@ func SearchTeam(ctx *context.Context) {
 		return
 	}
 
-	apiTeams := make([]*api.Team, len(teams))
+	apiTeams, err := convert.ToTeams(teams, false)
-	for i := range teams {
+	if err != nil {
-		if err := teams[i].GetUnits(); err != nil {
+		log.Error("convert ToTeams failed: %v", err)
-			log.Error("Team GetUnits failed: %v", err)
 		ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
 			"ok":    false,
 			"error": "SearchTeam failed to get units",
 		})
 		return
 	}
-		apiTeams[i] = convert.ToTeam(teams[i])
-	}
 
 	ctx.SetTotalCountHeader(maxResults)
 	ctx.JSON(http.StatusOK, map[string]interface{}{