teams: adds some validation to the API

pkg/api/team_members.go

@@ -29,9 +29,14 @@ func AddTeamMember(c *middleware.Context, cmd m.AddTeamMemberCommand) Response {
 	cmd.OrgId = c.OrgId
 
 	if err := bus.Dispatch(&cmd); err != nil {
+		if err == m.ErrTeamNotFound {
+			return ApiError(404, "Team not found", nil)
+		}
+
 		if err == m.ErrTeamMemberAlreadyAdded {
-			return ApiError(400, "User is already added to this team", err)
+			return ApiError(400, "User is already added to this team", nil)
 		}
+
 		return ApiError(500, "Failed to add Member to Team", err)
 	}
 
@@ -43,6 +48,14 @@ func AddTeamMember(c *middleware.Context, cmd m.AddTeamMemberCommand) Response {
 // DELETE /api/teams/:teamId/members/:userId
 func RemoveTeamMember(c *middleware.Context) Response {
 	if err := bus.Dispatch(&m.RemoveTeamMemberCommand{OrgId: c.OrgId, TeamId: c.ParamsInt64(":teamId"), UserId: c.ParamsInt64(":userId")}); err != nil {
+		if err == m.ErrTeamNotFound {
+			return ApiError(404, "Team not found", nil)
+		}
+
+		if err == m.ErrTeamMemberNotFound {
+			return ApiError(404, "Team member not found", nil)
+		}
+
 		return ApiError(500, "Failed to remove Member from Team", err)
 	}
 	return ApiSuccess("Team Member removed")

pkg/models/team.go

@@ -7,8 +7,9 @@ import (
 
 // Typed errors
 var (
-	ErrTeamNotFound  = errors.New("Team not found")
-	ErrTeamNameTaken = errors.New("Team name is taken")
+	ErrTeamNotFound       = errors.New("Team not found")
+	ErrTeamNameTaken      = errors.New("Team name is taken")
+	ErrTeamMemberNotFound = errors.New("Team member not found")
 )
 
 // Team model

pkg/services/sqlstore/team.go

@@ -78,11 +78,12 @@ func UpdateTeam(cmd *m.UpdateTeamCommand) error {
 	})
 }
 
+// DeleteTeam will delete a team, its member and any permissions connected to the team
 func DeleteTeam(cmd *m.DeleteTeamCommand) error {
 	return inTransaction(func(sess *DBSession) error {
-		if res, err := sess.Query("SELECT 1 from team WHERE org_id=? and id=?", cmd.OrgId, cmd.Id); err != nil {
+		if teamExists, err := teamExists(cmd.OrgId, cmd.Id, sess); err != nil {
 			return err
-		} else if len(res) != 1 {
+		} else if !teamExists {
 			return m.ErrTeamNotFound
 		}
 
@@ -102,6 +103,16 @@ func DeleteTeam(cmd *m.DeleteTeamCommand) error {
 	})
 }
 
+func teamExists(orgId int64, teamId int64, sess *DBSession) (bool, error) {
+	if res, err := sess.Query("SELECT 1 from team WHERE org_id=? and id=?", orgId, teamId); err != nil {
+		return false, err
+	} else if len(res) != 1 {
+		return false, nil
+	}
+
+	return true, nil
+}
+
 func isTeamNameTaken(orgId int64, name string, existingId int64, sess *DBSession) (bool, error) {
 	var team m.Team
 	exists, err := sess.Where("org_id=? and name=?", orgId, name).Get(&team)
@@ -190,6 +201,7 @@ func GetTeamById(query *m.GetTeamByIdQuery) error {
 	return nil
 }
 
+// GetTeamsByUser is used by the Guardian when checking a users' permissions
 func GetTeamsByUser(query *m.GetTeamsByUserQuery) error {
 	query.Result = make([]*m.Team, 0)
 
@@ -205,6 +217,7 @@ func GetTeamsByUser(query *m.GetTeamsByUserQuery) error {
 	return nil
 }
 
+// AddTeamMember adds a user to a team
 func AddTeamMember(cmd *m.AddTeamMemberCommand) error {
 	return inTransaction(func(sess *DBSession) error {
 		if res, err := sess.Query("SELECT 1 from team_member WHERE org_id=? and team_id=? and user_id=?", cmd.OrgId, cmd.TeamId, cmd.UserId); err != nil {
@@ -213,9 +226,9 @@ func AddTeamMember(cmd *m.AddTeamMemberCommand) error {
 			return m.ErrTeamMemberAlreadyAdded
 		}
 
-		if res, err := sess.Query("SELECT 1 from team WHERE org_id=? and id=?", cmd.OrgId, cmd.TeamId); err != nil {
+		if teamExists, err := teamExists(cmd.OrgId, cmd.TeamId, sess); err != nil {
 			return err
-		} else if len(res) != 1 {
+		} else if !teamExists {
 			return m.ErrTeamNotFound
 		}
 
@@ -232,18 +245,30 @@ func AddTeamMember(cmd *m.AddTeamMemberCommand) error {
 	})
 }
 
+// RemoveTeamMember removes a member from a team
 func RemoveTeamMember(cmd *m.RemoveTeamMemberCommand) error {
 	return inTransaction(func(sess *DBSession) error {
+		if teamExists, err := teamExists(cmd.OrgId, cmd.TeamId, sess); err != nil {
+			return err
+		} else if !teamExists {
+			return m.ErrTeamNotFound
+		}
+
 		var rawSql = "DELETE FROM team_member WHERE org_id=? and team_id=? and user_id=?"
-		_, err := sess.Exec(rawSql, cmd.OrgId, cmd.TeamId, cmd.UserId)
+		res, err := sess.Exec(rawSql, cmd.OrgId, cmd.TeamId, cmd.UserId)
 		if err != nil {
 			return err
 		}
+		rows, err := res.RowsAffected()
+		if rows == 0 {
+			return m.ErrTeamMemberNotFound
+		}
 
 		return err
 	})
 }
 
+// GetTeamMembers return a list of members for the specified team
 func GetTeamMembers(query *m.GetTeamMembersQuery) error {
 	query.Result = make([]*m.TeamMemberDTO, 0)
 	sess := x.Table("team_member")

pkg/services/sqlstore/team_test.go

@@ -84,13 +84,16 @@ func TestTeamCommandsAndQueries(t *testing.T) {
 			})
 
 			Convey("Should be able to remove users from a group", func() {
+				err = AddTeamMember(&m.AddTeamMemberCommand{OrgId: testOrgId, TeamId: group1.Result.Id, UserId: userIds[0]})
+				So(err, ShouldBeNil)
+
 				err = RemoveTeamMember(&m.RemoveTeamMemberCommand{OrgId: testOrgId, TeamId: group1.Result.Id, UserId: userIds[0]})
 				So(err, ShouldBeNil)
 
-				q1 := &m.GetTeamMembersQuery{TeamId: group1.Result.Id}
-				err = GetTeamMembers(q1)
+				q2 := &m.GetTeamMembersQuery{OrgId: testOrgId, TeamId: group1.Result.Id}
+				err = GetTeamMembers(q2)
 				So(err, ShouldBeNil)
-				So(len(q1.Result), ShouldEqual, 0)
+				So(len(q2.Result), ShouldEqual, 0)
 			})
 
 			Convey("Should be able to remove a group with users and permissions", func() {