diff --git a/app/group.go b/app/group.go index b1b841dcb6..e84cd34d9e 100644 --- a/app/group.go +++ b/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) diff --git a/model/user.go b/model/user.go index 36764310ad..4d71c13587 100644 --- a/model/user.go +++ b/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"` diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index 981b597361..b2c36643c3 100644 --- a/store/sqlstore/group_store.go +++ b/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"). - Join("UserGroups ON UserGroups.Id = GroupMembers.GroupId"). + LeftJoin("GroupMembers ON GroupMembers.UserId = Users.Id"). + 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"). - Join("UserGroups ON UserGroups.Id = GroupMembers.GroupId"). + LeftJoin("GroupMembers ON GroupMembers.UserId = Users.Id"). + 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 { diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index 3fc631361c..c5aa02aaed 100644 --- a/store/storetest/group_store.go +++ b/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}, - expectedTotalCount: numberOfUsers, + expectedUserIDs: []string{users[0].Id, users[1].Id, users[2].Id, users[3].Id, user.Id}, + expectedTotalCount: numberOfUsers + 1, groupIDs: []string{}, page: 0, perPage: 100, }, "All members, page 1": { - expectedUserIDs: []string{users[0].Id, users[1].Id}, - expectedTotalCount: numberOfUsers, + expectedUserIDs: []string{users[0].Id, users[1].Id, users[2].Id}, + expectedTotalCount: numberOfUsers + 1, groupIDs: []string{}, page: 0, - perPage: 2, + perPage: 3, }, "All members, page 2": { - expectedUserIDs: []string{users[2].Id, users[3].Id}, - expectedTotalCount: numberOfUsers, + expectedUserIDs: []string{users[3].Id, users[4].Id}, + 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}, - expectedTotalCount: 2, + expectedUserIDs: []string{users[0].Id, users[2].Id, users[4].Id}, + 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}, - expectedTotalCount: 2, + expectedUserIDs: []string{users[1].Id, users[3].Id, users[4].Id}, + expectedTotalCount: 3, groupIDs: []string{groups[0].Id}, page: 0, perPage: 100, }, "All groups, no users would be removed": { - expectedUserIDs: []string{}, - expectedTotalCount: 0, + expectedUserIDs: []string{users[4].Id}, + 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}, - expectedTotalCount: numberOfUsers, + expectedUserIDs: []string{users[0].Id, users[1].Id, users[2].Id, users[3].Id, users[4].Id}, + expectedTotalCount: numberOfUsers + 1, groupIDs: []string{}, page: 0, perPage: 100, }, "All members, page 1": { - expectedUserIDs: []string{users[0].Id, users[1].Id}, - expectedTotalCount: numberOfUsers, + expectedUserIDs: []string{users[0].Id, users[1].Id, users[2].Id}, + expectedTotalCount: numberOfUsers + 1, groupIDs: []string{}, page: 0, - perPage: 2, + perPage: 3, }, "All members, page 2": { - expectedUserIDs: []string{users[2].Id, users[3].Id}, - expectedTotalCount: numberOfUsers, + expectedUserIDs: []string{users[3].Id, users[4].Id}, + 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}, - expectedTotalCount: 2, + expectedUserIDs: []string{users[0].Id, users[2].Id, users[4].Id}, + 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}, - expectedTotalCount: 2, + expectedUserIDs: []string{users[1].Id, users[3].Id, users[4].Id}, + expectedTotalCount: 3, groupIDs: []string{groups[0].Id}, page: 0, perPage: 100, }, "All groups, no users would be removed": { - expectedUserIDs: []string{}, - expectedTotalCount: 0, + expectedUserIDs: []string{users[4].Id}, + 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)