Refactor and fix incorrect comment (#1247)

9 files changed, 56 insertions, 59 deletions

diff --git a/models/access.go b/models/access.go
index 49a8838ea6..98ead19a0b 100644
--- a/models/access.go
+++ b/models/access.go
@@ -59,21 +59,21 @@ type Access struct {
 	Mode   AccessMode
 }
 
-func accessLevel(e Engine, user *User, repo *Repository) (AccessMode, error) {
+func accessLevel(e Engine, userID int64, repo *Repository) (AccessMode, error) {
 	mode := AccessModeNone
 	if !repo.IsPrivate {
 		mode = AccessModeRead
 	}
 
-	if user == nil {
+	if userID == 0 {
 		return mode, nil
 	}
 
-	if user.ID == repo.OwnerID {
+	if userID == repo.OwnerID {
 		return AccessModeOwner, nil
 	}
 
-	a := &Access{UserID: user.ID, RepoID: repo.ID}
+	a := &Access{UserID: userID, RepoID: repo.ID}
 	if has, err := e.Get(a); !has || err != nil {
 		return mode, err
 	}
@@ -81,19 +81,19 @@ func accessLevel(e Engine, user *User, repo *Repository) (AccessMode, error) {
 }
 
 // AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the
-// user does not have access. User can be nil!
-func AccessLevel(user *User, repo *Repository) (AccessMode, error) {
-	return accessLevel(x, user, repo)
+// user does not have access.
+func AccessLevel(userID int64, repo *Repository) (AccessMode, error) {
+	return accessLevel(x, userID, repo)
 }
 
-func hasAccess(e Engine, user *User, repo *Repository, testMode AccessMode) (bool, error) {
-	mode, err := accessLevel(e, user, repo)
+func hasAccess(e Engine, userID int64, repo *Repository, testMode AccessMode) (bool, error) {
+	mode, err := accessLevel(e, userID, repo)
 	return testMode <= mode, err
 }
 
-// HasAccess returns true if someone has the request access level. User can be nil!
-func HasAccess(user *User, repo *Repository, testMode AccessMode) (bool, error) {
-	return hasAccess(x, user, repo, testMode)
+// HasAccess returns true if user has access to repo
+func HasAccess(userID int64, repo *Repository, testMode AccessMode) (bool, error) {
+	return hasAccess(x, userID, repo, testMode)
 }
 
 type repoAccess struct {
diff --git a/models/access_test.go b/models/access_test.go
index 6b3cce502c..29d40d9586 100644
--- a/models/access_test.go
+++ b/models/access_test.go
@@ -25,19 +25,19 @@ func TestAccessLevel(t *testing.T) {
 	repo1 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 2, IsPrivate: false}).(*Repository)
 	repo2 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 3, IsPrivate: true}).(*Repository)
 
-	level, err := AccessLevel(user1, repo1)
+	level, err := AccessLevel(user1.ID, repo1)
 	assert.NoError(t, err)
 	assert.Equal(t, AccessModeOwner, level)
 
-	level, err = AccessLevel(user1, repo2)
+	level, err = AccessLevel(user1.ID, repo2)
 	assert.NoError(t, err)
 	assert.Equal(t, AccessModeWrite, level)
 
-	level, err = AccessLevel(user2, repo1)
+	level, err = AccessLevel(user2.ID, repo1)
 	assert.NoError(t, err)
 	assert.Equal(t, AccessModeRead, level)
 
-	level, err = AccessLevel(user2, repo2)
+	level, err = AccessLevel(user2.ID, repo2)
 	assert.NoError(t, err)
 	assert.Equal(t, AccessModeNone, level)
 }
@@ -51,19 +51,19 @@ func TestHasAccess(t *testing.T) {
 	repo2 := AssertExistsAndLoadBean(t, &Repository{OwnerID: 3, IsPrivate: true}).(*Repository)
 
 	for _, accessMode := range accessModes {
-		has, err := HasAccess(user1, repo1, accessMode)
+		has, err := HasAccess(user1.ID, repo1, accessMode)
 		assert.NoError(t, err)
 		assert.True(t, has)
 
-		has, err = HasAccess(user1, repo2, accessMode)
+		has, err = HasAccess(user1.ID, repo2, accessMode)
 		assert.NoError(t, err)
 		assert.Equal(t, accessMode <= AccessModeWrite, has)
 
-		has, err = HasAccess(user2, repo1, accessMode)
+		has, err = HasAccess(user2.ID, repo1, accessMode)
 		assert.NoError(t, err)
 		assert.Equal(t, accessMode <= AccessModeRead, has)
 
-		has, err = HasAccess(user2, repo2, accessMode)
+		has, err = HasAccess(user2.ID, repo2, accessMode)
 		assert.NoError(t, err)
 		assert.Equal(t, accessMode <= AccessModeNone, has)
 	}
diff --git a/models/issue.go b/models/issue.go
index c740d8fec4..d5e20eb20a 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -374,7 +374,7 @@ func (issue *Issue) RemoveLabel(doer *User, label *Label) error {
 		return err
 	}
 
-	if has, err := HasAccess(doer, issue.Repo, AccessModeWrite); err != nil {
+	if has, err := HasAccess(doer.ID, issue.Repo, AccessModeWrite); err != nil {
 		return err
 	} else if !has {
 		return ErrLabelNotExist{}
@@ -415,7 +415,7 @@ func (issue *Issue) ClearLabels(doer *User) (err error) {
 		return err
 	}
 
-	if has, err := hasAccess(sess, doer, issue.Repo, AccessModeWrite); err != nil {
+	if has, err := hasAccess(sess, doer.ID, issue.Repo, AccessModeWrite); err != nil {
 		return err
 	} else if !has {
 		return ErrLabelNotExist{}
@@ -809,23 +809,14 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 		}
 	}
 
-	if opts.Issue.AssigneeID > 0 {
-		assignee, err := getUserByID(e, opts.Issue.AssigneeID)
-		if err != nil && !IsErrUserNotExist(err) {
-			return fmt.Errorf("getUserByID: %v", err)
+	if assigneeID := opts.Issue.AssigneeID; assigneeID > 0 {
+		valid, err := hasAccess(e, assigneeID, opts.Repo, AccessModeWrite)
+		if err != nil {
+			return fmt.Errorf("hasAccess [user_id: %d, repo_id: %d]: %v", assigneeID, opts.Repo.ID, err)
 		}
-
-		// Assume assignee is invalid and drop silently.
-		opts.Issue.AssigneeID = 0
-		if assignee != nil {
-			valid, err := hasAccess(e, assignee, opts.Repo, AccessModeWrite)
-			if err != nil {
-				return fmt.Errorf("hasAccess [user_id: %d, repo_id: %d]: %v", assignee.ID, opts.Repo.ID, err)
-			}
-			if valid {
-				opts.Issue.AssigneeID = assignee.ID
-				opts.Issue.Assignee = assignee
-			}
+		if !valid {
+			opts.Issue.AssigneeID = 0
+			opts.Issue.Assignee = nil
 		}
 	}
 
diff --git a/models/org_team.go b/models/org_team.go
index 84282da83c..db25e61547 100644
--- a/models/org_team.go
+++ b/models/org_team.go
@@ -139,18 +139,19 @@ func (t *Team) removeRepository(e Engine, repo *Repository, recalculate bool) (e
 		}
 	}
 
-	if err = t.getMembers(e); err != nil {
-		return fmt.Errorf("get team members: %v", err)
+	teamUsers, err := getTeamUsersByTeamID(e, t.ID)
+	if err != nil {
+		return fmt.Errorf("getTeamUsersByTeamID: %v", err)
 	}
-	for _, u := range t.Members {
-		has, err := hasAccess(e, u, repo, AccessModeRead)
+	for _, teamUser:= range teamUsers {
+		has, err := hasAccess(e, teamUser.UID, repo, AccessModeRead)
 		if err != nil {
 			return err
 		} else if has {
 			continue
 		}
 
-		if err = watchRepo(e, u.ID, repo.ID, false); err != nil {
+		if err = watchRepo(e, teamUser.UID, repo.ID, false); err != nil {
 			return err
 		}
 	}
@@ -399,20 +400,25 @@ func IsTeamMember(orgID, teamID, userID int64) bool {
 	return isTeamMember(x, orgID, teamID, userID)
 }
 
-func getTeamMembers(e Engine, teamID int64) (_ []*User, err error) {
+func getTeamUsersByTeamID(e Engine, teamID int64) ([]*TeamUser, error) {
 	teamUsers := make([]*TeamUser, 0, 10)
-	if err = e.
+	return teamUsers, e.
 		Where("team_id=?", teamID).
-		Find(&teamUsers); err != nil {
+		Find(&teamUsers)
+}
+
+func getTeamMembers(e Engine, teamID int64) (_ []*User, err error) {
+	teamUsers, err := getTeamUsersByTeamID(e, teamID)
+	if err != nil {
 		return nil, fmt.Errorf("get team-users: %v", err)
 	}
-	members := make([]*User, 0, len(teamUsers))
-	for i := range teamUsers {
-		member := new(User)
-		if _, err = e.Id(teamUsers[i].UID).Get(member); err != nil {
-			return nil, fmt.Errorf("get user '%d': %v", teamUsers[i].UID, err)
+	members := make([]*User, len(teamUsers))
+	for i, teamUser := range teamUsers {
+		member, err := getUserByID(e, teamUser.UID)
+		if err != nil {
+			return nil, fmt.Errorf("get user '%d': %v", teamUser.UID, err)
 		}
-		members = append(members, member)
+		members[i] = member
 	}
 	return members, nil
 }
diff --git a/models/org_team_test.go b/models/org_team_test.go
index db0a814684..506de6e6bd 100644
--- a/models/org_team_test.go
+++ b/models/org_team_test.go
@@ -243,7 +243,7 @@ func TestDeleteTeam(t *testing.T) {
 	// check that team members don't have "leftover" access to repos
 	user := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User)
 	repo := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
-	accessMode, err := AccessLevel(user, repo)
+	accessMode, err := AccessLevel(user.ID, repo)
 	assert.NoError(t, err)
 	assert.True(t, accessMode < AccessModeWrite)
 }
diff --git a/models/release.go b/models/release.go
index 3e3db98ab4..ae3200d39d 100644
--- a/models/release.go
+++ b/models/release.go
@@ -365,7 +365,7 @@ func DeleteReleaseByID(id int64, u *User, delTag bool) error {
 		return fmt.Errorf("GetRepositoryByID: %v", err)
 	}
 
-	has, err := HasAccess(u, repo, AccessModeWrite)
+	has, err := HasAccess(u.ID, repo, AccessModeWrite)
 	if err != nil {
 		return fmt.Errorf("HasAccess: %v", err)
 	} else if !has {
diff --git a/models/repo.go b/models/repo.go
index d44f4ba489..eac0f015f0 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -531,7 +531,7 @@ func (repo *Repository) ComposeCompareURL(oldCommitID, newCommitID string) strin
 
 // HasAccess returns true when user has access to this repository
 func (repo *Repository) HasAccess(u *User) bool {
-	has, _ := HasAccess(u, repo, AccessModeRead)
+	has, _ := HasAccess(u.ID, repo, AccessModeRead)
 	return has
 }
 
diff --git a/models/ssh_key.go b/models/ssh_key.go
index 802333f48c..ba7007028f 100644
--- a/models/ssh_key.go
+++ b/models/ssh_key.go
@@ -794,7 +794,7 @@ func DeleteDeployKey(doer *User, id int64) error {
 		if err != nil {
 			return fmt.Errorf("GetRepositoryByID: %v", err)
 		}
-		yes, err := HasAccess(doer, repo, AccessModeAdmin)
+		yes, err := HasAccess(doer.ID, repo, AccessModeAdmin)
 		if err != nil {
 			return fmt.Errorf("HasAccess: %v", err)
 		} else if !yes {
diff --git a/models/user.go b/models/user.go
index 7cdff1a46f..cfc01936f0 100644
--- a/models/user.go
+++ b/models/user.go
@@ -478,7 +478,7 @@ func (u *User) DeleteAvatar() error {
 
 // IsAdminOfRepo returns true if user has admin or higher access of repository.
 func (u *User) IsAdminOfRepo(repo *Repository) bool {
-	has, err := HasAccess(u, repo, AccessModeAdmin)
+	has, err := HasAccess(u.ID, repo, AccessModeAdmin)
 	if err != nil {
 		log.Error(3, "HasAccess: %v", err)
 	}
@@ -487,7 +487,7 @@ func (u *User) IsAdminOfRepo(repo *Repository) bool {
 
 // IsWriterOfRepo returns true if user has write access to given repository.
 func (u *User) IsWriterOfRepo(repo *Repository) bool {
-	has, err := HasAccess(u, repo, AccessModeWrite)
+	has, err := HasAccess(u.ID, repo, AccessModeWrite)
 	if err != nil {
 		log.Error(3, "HasAccess: %v", err)
 	}
@@ -1103,7 +1103,7 @@ func GetUserByID(id int64) (*User, error) {
 
 // GetAssigneeByID returns the user with write access of repository by given ID.
 func GetAssigneeByID(repo *Repository, userID int64) (*User, error) {
-	has, err := HasAccess(&User{ID: userID}, repo, AccessModeWrite)
+	has, err := HasAccess(userID, repo, AccessModeWrite)
 	if err != nil {
 		return nil, err
 	} else if !has {