From 76057105ca73b9354bec01be3e7b5039de80c1e0 Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Fri, 24 Feb 2017 01:25:09 -0500 Subject: [PATCH] Remove unnecessary loads in org_team (#1035) --- models/org.go | 2 +- models/org_team.go | 82 ++++++++++++++++++----------------------- models/org_team_test.go | 6 +-- 3 files changed, 39 insertions(+), 51 deletions(-) diff --git a/models/org.go b/models/org.go index 9fd81c1c0..49dcfcaa6 100644 --- a/models/org.go +++ b/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 } } diff --git a/models/org_team.go b/models/org_team.go index 55a539643..9296815fb 100644 --- a/models/org_team.go +++ b/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() diff --git a/models/org_team_test.go b/models/org_team_test.go index 39a37489e..db0a81468 100644 --- a/models/org_team_test.go +++ b/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)) }