MM-17383: Update query to include users who are not members of any gr… (#11730)

* MM-17383: Update query to include users who are not members of any groups.

* MM-17383: Fixes govet complaint.

* MM-17383: Sorts by username.

* MM-17383: Removes accidental staging.

app/group.go

@@ -4,8 +4,6 @@
 package app
 
 import (
-	"strings"
-
 	"github.com/mattermost/mattermost-server/model"
 )
 
@@ -140,7 +138,7 @@ func (a *App) TeamMembersMinusGroupMembers(teamID string, groupIDs []string, pag
 	// parse all group ids of all users
 	allUsersGroupIDMap := map[string]bool{}
 	for _, user := range users {
-		for _, groupID := range strings.Split(user.GroupIDs, ",") {
+		for _, groupID := range user.GetGroupIDs() {
 			allUsersGroupIDMap[groupID] = true
 		}
 	}
@@ -166,7 +164,7 @@ func (a *App) TeamMembersMinusGroupMembers(teamID string, groupIDs []string, pag
 	// populate each instance's groups field
 	for _, user := range users {
 		user.Groups = []*model.Group{}
-		for _, groupID := range strings.Split(user.GroupIDs, ",") {
+		for _, groupID := range user.GetGroupIDs() {
 			group, ok := groupMap[groupID]
 			if ok {
 				user.Groups = append(user.Groups, group)
@@ -199,7 +197,7 @@ func (a *App) ChannelMembersMinusGroupMembers(channelID string, groupIDs []strin
 	// parse all group ids of all users
 	allUsersGroupIDMap := map[string]bool{}
 	for _, user := range users {
-		for _, groupID := range strings.Split(user.GroupIDs, ",") {
+		for _, groupID := range user.GetGroupIDs() {
 			allUsersGroupIDMap[groupID] = true
 		}
 	}
@@ -225,7 +223,7 @@ func (a *App) ChannelMembersMinusGroupMembers(channelID string, groupIDs []strin
 	// populate each instance's groups field
 	for _, user := range users {
 		user.Groups = []*model.Group{}
-		for _, groupID := range strings.Split(user.GroupIDs, ",") {
+		for _, groupID := range user.GetGroupIDs() {
 			group, ok := groupMap[groupID]
 			if ok {
 				user.Groups = append(user.Groups, group)

model/user.go

@@ -783,13 +783,24 @@ func IsValidLocale(locale string) bool {
 
 type UserWithGroups struct {
 	User
-	GroupIDs    string   `json:"-"`
+	GroupIDs    *string  `json:"-"`
 	Groups      []*Group `json:"groups"`
 	SchemeGuest bool     `json:"scheme_guest"`
 	SchemeUser  bool     `json:"scheme_user"`
 	SchemeAdmin bool     `json:"scheme_admin"`
 }
 
+func (u *UserWithGroups) GetGroupIDs() []string {
+	if u.GroupIDs == nil {
+		return nil
+	}
+	trimmed := strings.TrimSpace(*u.GroupIDs)
+	if len(trimmed) == 0 {
+		return nil
+	}
+	return strings.Split(trimmed, ",")
+}
+
 type UsersWithGroupsAndCount struct {
 	Users []*UserWithGroups `json:"users"`
 	Count int64             `json:"total_count"`

store/sqlstore/group_store.go

@@ -992,8 +992,8 @@ func (s *SqlGroupStore) teamMembersMinusGroupMembersQuery(teamID string, groupID
 		Join("Teams ON Teams.Id = TeamMembers.TeamId").
 		Join("Users ON Users.Id = TeamMembers.UserId").
 		LeftJoin("Bots ON Bots.UserId = TeamMembers.UserId").
-		Join("GroupMembers ON GroupMembers.UserId = Users.Id").
+		LeftJoin("GroupMembers ON GroupMembers.UserId = Users.Id").
-		Join("UserGroups ON UserGroups.Id = GroupMembers.GroupId").
+		LeftJoin("UserGroups ON UserGroups.Id = GroupMembers.GroupId").
 		Where("TeamMembers.DeleteAt = 0").
 		Where("Teams.DeleteAt = 0").
 		Where("Users.DeleteAt = 0").
@@ -1012,7 +1012,7 @@ func (s *SqlGroupStore) teamMembersMinusGroupMembersQuery(teamID string, groupID
 // groups.
 func (s *SqlGroupStore) TeamMembersMinusGroupMembers(teamID string, groupIDs []string, page, perPage int) ([]*model.UserWithGroups, *model.AppError) {
 	query := s.teamMembersMinusGroupMembersQuery(teamID, groupIDs, false)
-	query = query.OrderBy("Users.Id").Limit(uint64(perPage)).Offset(uint64(page * perPage))
+	query = query.OrderBy("Users.Username ASC").Limit(uint64(perPage)).Offset(uint64(page * perPage))
 
 	queryString, args, err := query.ToSql()
 	if err != nil {
@@ -1070,8 +1070,8 @@ func (s *SqlGroupStore) channelMembersMinusGroupMembersQuery(channelID string, g
 		Join("Channels ON Channels.Id = ChannelMembers.ChannelId").
 		Join("Users ON Users.Id = ChannelMembers.UserId").
 		LeftJoin("Bots ON Bots.UserId = ChannelMembers.UserId").
-		Join("GroupMembers ON GroupMembers.UserId = Users.Id").
+		LeftJoin("GroupMembers ON GroupMembers.UserId = Users.Id").
-		Join("UserGroups ON UserGroups.Id = GroupMembers.GroupId").
+		LeftJoin("UserGroups ON UserGroups.Id = GroupMembers.GroupId").
 		Where("Channels.DeleteAt = 0").
 		Where("Users.DeleteAt = 0").
 		Where("Bots.UserId IS NULL").
@@ -1089,7 +1089,7 @@ func (s *SqlGroupStore) channelMembersMinusGroupMembersQuery(channelID string, g
 // groups.
 func (s *SqlGroupStore) ChannelMembersMinusGroupMembers(channelID string, groupIDs []string, page, perPage int) ([]*model.UserWithGroups, *model.AppError) {
 	query := s.channelMembersMinusGroupMembersQuery(channelID, groupIDs, false)
-	query = query.OrderBy("Users.Id").Limit(uint64(perPage)).Offset(uint64(page * perPage))
+	query = query.OrderBy("Users.Username ASC").Limit(uint64(perPage)).Offset(uint64(page * perPage))
 
 	queryString, args, err := query.ToSql()
 	if err != nil {

store/storetest/group_store.go

@@ -2245,7 +2245,7 @@ func testTeamMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 	for i := 0; i < numberOfUsers; i++ {
 		user := &model.User{
 			Email:    MakeEmail(),
-			Username: model.NewId(),
+			Username: fmt.Sprintf("%d_%s", i, model.NewId()),
 		}
 		user, err = ss.User().Save(user)
 		require.Nil(t, err)
@@ -2256,6 +2256,17 @@ func testTeamMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 		require.Nil(t, err)
 	}
 
+	// Extra user outside of the group member users.
+	user := &model.User{
+		Email:    MakeEmail(),
+		Username: "99_" + model.NewId(),
+	}
+	user, err = ss.User().Save(user)
+	require.Nil(t, err)
+	users = append(users, user)
+	_, err = ss.Team().SaveMember(&model.TeamMember{TeamId: team.Id, UserId: user.Id, SchemeUser: true, SchemeAdmin: false}, 999)
+	require.Nil(t, err)
+
 	for i := 0; i < numberOfGroups; i++ {
 		group := &model.Group{
 			Name:        fmt.Sprintf("n_%d_%s", i, model.NewId()),
@@ -2270,7 +2281,7 @@ func testTeamMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 	}
 
 	sort.Slice(users, func(i, j int) bool {
-		return users[i].Id < users[j].Id
+		return users[i].Username < users[j].Username
 	})
 
 	// Add even users to even group, and the inverse
@@ -2294,43 +2305,43 @@ func testTeamMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 		teardown           func()
 	}{
 		"No group IDs, all members": {
-			expectedUserIDs:    []string{users[0].Id, users[1].Id, users[2].Id, users[3].Id},
+			expectedUserIDs:    []string{users[0].Id, users[1].Id, users[2].Id, users[3].Id, user.Id},
-			expectedTotalCount: numberOfUsers,
+			expectedTotalCount: numberOfUsers + 1,
 			groupIDs:           []string{},
 			page:               0,
 			perPage:            100,
 		},
 		"All members, page 1": {
-			expectedUserIDs:    []string{users[0].Id, users[1].Id},
+			expectedUserIDs:    []string{users[0].Id, users[1].Id, users[2].Id},
-			expectedTotalCount: numberOfUsers,
+			expectedTotalCount: numberOfUsers + 1,
 			groupIDs:           []string{},
 			page:               0,
-			perPage:            2,
+			perPage:            3,
 		},
 		"All members, page 2": {
-			expectedUserIDs:    []string{users[2].Id, users[3].Id},
+			expectedUserIDs:    []string{users[3].Id, users[4].Id},
-			expectedTotalCount: numberOfUsers,
+			expectedTotalCount: numberOfUsers + 1,
 			groupIDs:           []string{},
 			page:               1,
-			perPage:            2,
+			perPage:            3,
 		},
 		"Group 1, even users would be removed": {
-			expectedUserIDs:    []string{users[0].Id, users[2].Id},
+			expectedUserIDs:    []string{users[0].Id, users[2].Id, users[4].Id},
-			expectedTotalCount: 2,
+			expectedTotalCount: 3,
 			groupIDs:           []string{groups[1].Id},
 			page:               0,
 			perPage:            100,
 		},
 		"Group 0, odd users would be removed": {
-			expectedUserIDs:    []string{users[1].Id, users[3].Id},
+			expectedUserIDs:    []string{users[1].Id, users[3].Id, users[4].Id},
-			expectedTotalCount: 2,
+			expectedTotalCount: 3,
 			groupIDs:           []string{groups[0].Id},
 			page:               0,
 			perPage:            100,
 		},
 		"All groups, no users would be removed": {
-			expectedUserIDs:    []string{},
+			expectedUserIDs:    []string{users[4].Id},
-			expectedTotalCount: 0,
+			expectedTotalCount: 1,
 			groupIDs:           []string{groups[0].Id, groups[1].Id},
 			page:               0,
 			perPage:            100,
@@ -2359,11 +2370,6 @@ func testTeamMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 			require.Nil(t, err)
 			require.ElementsMatch(t, tc.expectedUserIDs, mapUserIDs(actual))
 
-			for _, user := range actual {
-				require.NotNil(t, user.GroupIDs)
-				require.True(t, (user.SchemeAdmin || user.SchemeUser))
-			}
-
 			actualCount, err := ss.Group().CountTeamMembersMinusGroupMembers(team.Id, tc.groupIDs)
 			require.Nil(t, err)
 			require.Equal(t, tc.expectedTotalCount, actualCount)
@@ -2391,14 +2397,14 @@ func testChannelMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 	for i := 0; i < numberOfUsers; i++ {
 		user := &model.User{
 			Email:    MakeEmail(),
-			Username: model.NewId(),
+			Username: fmt.Sprintf("%d_%s", i, model.NewId()),
 		}
 		user, err = ss.User().Save(user)
 		require.Nil(t, err)
 		users = append(users, user)
 
 		trueOrFalse := int(math.Mod(float64(i), 2)) == 0
-		_, err := ss.Channel().SaveMember(&model.ChannelMember{
+		_, err = ss.Channel().SaveMember(&model.ChannelMember{
 			ChannelId:   channel.Id,
 			UserId:      user.Id,
 			SchemeUser:  trueOrFalse,
@@ -2408,6 +2414,22 @@ func testChannelMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 		require.Nil(t, err)
 	}
 
+	// Extra user outside of the group member users.
+	user, err := ss.User().Save(&model.User{
+		Email:    MakeEmail(),
+		Username: "99_" + model.NewId(),
+	})
+	require.Nil(t, err)
+	users = append(users, user)
+	_, err = ss.Channel().SaveMember(&model.ChannelMember{
+		ChannelId:   channel.Id,
+		UserId:      user.Id,
+		SchemeUser:  true,
+		SchemeAdmin: false,
+		NotifyProps: model.GetDefaultChannelNotifyProps(),
+	})
+	require.Nil(t, err)
+
 	for i := 0; i < numberOfGroups; i++ {
 		group := &model.Group{
 			Name:        fmt.Sprintf("n_%d_%s", i, model.NewId()),
@@ -2422,7 +2444,7 @@ func testChannelMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 	}
 
 	sort.Slice(users, func(i, j int) bool {
-		return users[i].Id < users[j].Id
+		return users[i].Username < users[j].Username
 	})
 
 	// Add even users to even group, and the inverse
@@ -2446,43 +2468,43 @@ func testChannelMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 		teardown           func()
 	}{
 		"No group IDs, all members": {
-			expectedUserIDs:    []string{users[0].Id, users[1].Id, users[2].Id, users[3].Id},
+			expectedUserIDs:    []string{users[0].Id, users[1].Id, users[2].Id, users[3].Id, users[4].Id},
-			expectedTotalCount: numberOfUsers,
+			expectedTotalCount: numberOfUsers + 1,
 			groupIDs:           []string{},
 			page:               0,
 			perPage:            100,
 		},
 		"All members, page 1": {
-			expectedUserIDs:    []string{users[0].Id, users[1].Id},
+			expectedUserIDs:    []string{users[0].Id, users[1].Id, users[2].Id},
-			expectedTotalCount: numberOfUsers,
+			expectedTotalCount: numberOfUsers + 1,
 			groupIDs:           []string{},
 			page:               0,
-			perPage:            2,
+			perPage:            3,
 		},
 		"All members, page 2": {
-			expectedUserIDs:    []string{users[2].Id, users[3].Id},
+			expectedUserIDs:    []string{users[3].Id, users[4].Id},
-			expectedTotalCount: numberOfUsers,
+			expectedTotalCount: numberOfUsers + 1,
 			groupIDs:           []string{},
 			page:               1,
-			perPage:            2,
+			perPage:            3,
 		},
 		"Group 1, even users would be removed": {
-			expectedUserIDs:    []string{users[0].Id, users[2].Id},
+			expectedUserIDs:    []string{users[0].Id, users[2].Id, users[4].Id},
-			expectedTotalCount: 2,
+			expectedTotalCount: 3,
 			groupIDs:           []string{groups[1].Id},
 			page:               0,
 			perPage:            100,
 		},
 		"Group 0, odd users would be removed": {
-			expectedUserIDs:    []string{users[1].Id, users[3].Id},
+			expectedUserIDs:    []string{users[1].Id, users[3].Id, users[4].Id},
-			expectedTotalCount: 2,
+			expectedTotalCount: 3,
 			groupIDs:           []string{groups[0].Id},
 			page:               0,
 			perPage:            100,
 		},
 		"All groups, no users would be removed": {
-			expectedUserIDs:    []string{},
+			expectedUserIDs:    []string{users[4].Id},
-			expectedTotalCount: 0,
+			expectedTotalCount: 1,
 			groupIDs:           []string{groups[0].Id, groups[1].Id},
 			page:               0,
 			perPage:            100,
@@ -2511,11 +2533,6 @@ func testChannelMembersMinusGroupMembers(t *testing.T, ss store.Store) {
 			require.Nil(t, err)
 			require.ElementsMatch(t, tc.expectedUserIDs, mapUserIDs(actual))
 
-			for _, user := range actual {
-				require.NotNil(t, user.GroupIDs)
-				require.True(t, (user.SchemeAdmin || user.SchemeUser))
-			}
-
 			actualCount, err := ss.Group().CountChannelMembersMinusGroupMembers(channel.Id, tc.groupIDs)
 			require.Nil(t, err)
 			require.Equal(t, tc.expectedTotalCount, actualCount)