Remove unnecessary loads in org_team (#1035)

Author: ethantkoenig

models/org.go

@@ -517,7 +517,7 @@ func RemoveOrgUser(orgID, userID int64) error {
 		return err
 	}
 	for _, t := range teams {
-		if err = removeTeamMember(sess, org.ID, t.ID, userID); err != nil {
+		if err = removeTeamMember(sess, t, userID); err != nil {
 			return err
 		}
 	}

models/org_team.go

@@ -59,12 +59,12 @@ func (t *Team) GetMembers() (err error) {
 // AddMember adds new membership of the team to the organization,
 // the user will have membership to the organization automatically when needed.
 func (t *Team) AddMember(userID int64) error {
-	return AddTeamMember(t.OrgID, t.ID, userID)
+	return AddTeamMember(t, userID)
 }
 
 // RemoveMember removes member from team of organization.
 func (t *Team) RemoveMember(userID int64) error {
-	return RemoveTeamMember(t.OrgID, t.ID, userID)
+	return RemoveTeamMember(t, userID)
 }
 
 func (t *Team) hasRepository(e Engine, repoID int64) bool {
@@ -443,113 +443,101 @@ func GetUserTeams(orgID, userID int64) ([]*Team, error) {
 
 // AddTeamMember adds new membership of given team to given organization,
 // the user will have membership to given organization automatically when needed.
-func AddTeamMember(orgID, teamID, userID int64) error {
-	if IsTeamMember(orgID, teamID, userID) {
+func AddTeamMember(team *Team, userID int64) error {
+	if IsTeamMember(team.OrgID, team.ID, userID) {
 		return nil
 	}
 
-	if err := AddOrgUser(orgID, userID); err != nil {
+	if err := AddOrgUser(team.OrgID, userID); err != nil {
 		return err
 	}
 
 	// Get team and its repositories.
-	t, err := GetTeamByID(teamID)
-	if err != nil {
-		return err
-	}
-	t.NumMembers++
+	team.NumMembers++
 
-	if err = t.GetRepositories(); err != nil {
+	if err := team.GetRepositories(); err != nil {
 		return err
 	}
 
 	sess := x.NewSession()
 	defer sessionRelease(sess)
-	if err = sess.Begin(); err != nil {
+	if err := sess.Begin(); err != nil {
 		return err
 	}
 
-	tu := &TeamUser{
+	if _, err := sess.Insert(&TeamUser{
 		UID:    userID,
-		OrgID:  orgID,
-		TeamID: teamID,
-	}
-	if _, err = sess.Insert(tu); err != nil {
+		OrgID:  team.OrgID,
+		TeamID: team.ID,
+	}); err != nil {
 		return err
-	} else if _, err = sess.Id(t.ID).Update(t); err != nil {
+	} else if _, err := sess.Id(team.ID).Update(team); err != nil {
 		return err
 	}
 
 	// Give access to team repositories.
-	for _, repo := range t.Repos {
-		if err = repo.recalculateTeamAccesses(sess, 0); err != nil {
+	for _, repo := range team.Repos {
+		if err := repo.recalculateTeamAccesses(sess, 0); err != nil {
 			return err
 		}
 	}
 
 	// We make sure it exists before.
 	ou := new(OrgUser)
-	if _, err = sess.
+	if _, err := sess.
 		Where("uid = ?", userID).
-		And("org_id = ?", orgID).
+		And("org_id = ?", team.OrgID).
 		Get(ou); err != nil {
 		return err
 	}
 	ou.NumTeams++
-	if t.IsOwnerTeam() {
+	if team.IsOwnerTeam() {
 		ou.IsOwner = true
 	}
-	if _, err = sess.Id(ou.ID).AllCols().Update(ou); err != nil {
+	if _, err := sess.Id(ou.ID).AllCols().Update(ou); err != nil {
 		return err
 	}
 
 	return sess.Commit()
 }
 
-func removeTeamMember(e Engine, orgID, teamID, userID int64) error {
-	if !isTeamMember(e, orgID, teamID, userID) {
+func removeTeamMember(e Engine, team *Team, userID int64) error {
+	if !isTeamMember(e, team.OrgID, team.ID, userID) {
 		return nil
 	}
 
-	// Get team and its repositories.
-	t, err := getTeamByID(e, teamID)
-	if err != nil {
-		return err
-	}
-
 	// Check if the user to delete is the last member in owner team.
-	if t.IsOwnerTeam() && t.NumMembers == 1 {
+	if team.IsOwnerTeam() && team.NumMembers == 1 {
 		return ErrLastOrgOwner{UID: userID}
 	}
 
-	t.NumMembers--
+	team.NumMembers--
 
-	if err = t.getRepositories(e); err != nil {
+	if err := team.getRepositories(e); err != nil {
 		return err
 	}
 
 	// Get organization.
-	org, err := getUserByID(e, orgID)
+	org, err := getUserByID(e, team.OrgID)
 	if err != nil {
 		return err
 	}
 
-	tu := &TeamUser{
+	if _, err := e.Delete(&TeamUser{
 		UID:    userID,
-		OrgID:  orgID,
-		TeamID: teamID,
-	}
-	if _, err := e.Delete(tu); err != nil {
+		OrgID:  team.OrgID,
+		TeamID: team.ID,
+	}); err != nil {
 		return err
 	} else if _, err = e.
-		Id(t.ID).
+		Id(team.ID).
 		AllCols().
-		Update(t); err != nil {
+		Update(team); err != nil {
 		return err
 	}
 
 	// Delete access to team repositories.
-	for _, repo := range t.Repos {
+	for _, repo := range team.Repos {
 		if err = repo.recalculateTeamAccesses(e, 0); err != nil {
 			return err
 		}
@@ -565,7 +553,7 @@ func removeTeamMember(e Engine, orgID, teamID, userID int64) error {
 		return err
 	}
 	ou.NumTeams--
-	if t.IsOwnerTeam() {
+	if team.IsOwnerTeam() {
 		ou.IsOwner = false
 	}
 	if _, err = e.
@@ -578,13 +566,13 @@ func removeTeamMember(e Engine, orgID, teamID, userID int64) error {
 }
 
 // RemoveTeamMember removes member from given team of given organization.
-func RemoveTeamMember(orgID, teamID, userID int64) error {
+func RemoveTeamMember(team *Team, userID int64) error {
 	sess := x.NewSession()
 	defer sessionRelease(sess)
 	if err := sess.Begin(); err != nil {
 		return err
 	}
-	if err := removeTeamMember(sess, orgID, teamID, userID); err != nil {
+	if err := removeTeamMember(sess, team, userID); err != nil {
 		return err
 	}
 	return sess.Commit()

models/org_team_test.go

@@ -298,7 +298,7 @@ func TestAddTeamMember(t *testing.T) {
 
 	test := func(teamID, userID int64) {
 		team := AssertExistsAndLoadBean(t, &Team{ID: teamID}).(*Team)
-		assert.NoError(t, AddTeamMember(team.OrgID, team.ID, userID))
+		assert.NoError(t, AddTeamMember(team, userID))
 		AssertExistsAndLoadBean(t, &TeamUser{UID: userID, TeamID: teamID})
 		CheckConsistencyFor(t, &Team{ID: teamID}, &User{ID: team.OrgID})
 	}
@@ -312,7 +312,7 @@ func TestRemoveTeamMember(t *testing.T) {
 
 	testSuccess := func(teamID, userID int64) {
 		team := AssertExistsAndLoadBean(t, &Team{ID: teamID}).(*Team)
-		assert.NoError(t, RemoveTeamMember(team.OrgID, team.ID, userID))
+		assert.NoError(t, RemoveTeamMember(team, userID))
 		AssertNotExistsBean(t, &TeamUser{UID: userID, TeamID: teamID})
 		CheckConsistencyFor(t, &Team{ID: teamID})
 	}
@@ -322,7 +322,7 @@ func TestRemoveTeamMember(t *testing.T) {
 	testSuccess(3, NonexistentID)
 
 	team := AssertExistsAndLoadBean(t, &Team{ID: 1}).(*Team)
-	err := RemoveTeamMember(team.OrgID, team.ID, 2)
+	err := RemoveTeamMember(team, 2)
 	assert.True(t, IsErrLastOrgOwner(err))
 }