[MM-21946] [MM-21945] Ensure deleted groups are not returned from the groups API (#13747)

* MM-21946 Ensure deleted groups are not returned from the groups API

* MM-21946 Only append to query at the end

* MM-21946 Add DeleteAt check to the top of the GetGroups function and clean up the formatting a bit

* MM-21946 Move the From statement into the next block

store/sqlstore/group_store.go

@@ -905,7 +905,7 @@ func (s *SqlGroupStore) groupsBySyncableBaseQuery(st model.GroupSyncableType, t
 			From("UserGroups ug").
 			LeftJoin("(SELECT GroupMembers.GroupId, COUNT(*) AS MemberCount FROM GroupMembers LEFT JOIN Users ON Users.Id = GroupMembers.UserId WHERE GroupMembers.DeleteAt = 0 AND Users.DeleteAt = 0 GROUP BY GroupId) AS Members ON Members.GroupId = ug.Id").
 			LeftJoin(fmt.Sprintf("%[1]s ON %[1]s.GroupId = ug.Id", table)).
-			Where(fmt.Sprintf("%[1]s.DeleteAt = 0 AND %[1]s.%[2]s = ?", table, idCol), syncableID).
+			Where(fmt.Sprintf("ug.DeleteAt = 0 AND %[1]s.DeleteAt = 0 AND %[1]s.%[2]s = ?", table, idCol), syncableID).
 			OrderBy("ug.DisplayName")
 	}
 
@@ -963,18 +963,21 @@ func (s *SqlGroupStore) GetGroupsByTeam(teamId string, opts model.GroupSearchOpt
 func (s *SqlGroupStore) GetGroups(page, perPage int, opts model.GroupSearchOpts) ([]*model.Group, *model.AppError) {
 	var groups []*model.Group
 
-	groupsQuery := s.getQueryBuilder().Select("g.*").From("UserGroups g").Limit(uint64(perPage)).Offset(uint64(page * perPage)).OrderBy("g.DisplayName")
+	groupsQuery := s.getQueryBuilder().Select("g.*")
 
 	if opts.IncludeMemberCount {
 		groupsQuery = s.getQueryBuilder().
 			Select("g.*, coalesce(Members.MemberCount, 0) AS MemberCount").
-			From("UserGroups g").
+			LeftJoin("(SELECT GroupMembers.GroupId, COUNT(*) AS MemberCount FROM GroupMembers LEFT JOIN Users ON Users.Id = GroupMembers.UserId WHERE GroupMembers.DeleteAt = 0 AND Users.DeleteAt = 0 GROUP BY GroupId) AS Members ON Members.GroupId = g.Id")
-			LeftJoin("(SELECT GroupMembers.GroupId, COUNT(*) AS MemberCount FROM GroupMembers LEFT JOIN Users ON Users.Id = GroupMembers.UserId WHERE GroupMembers.DeleteAt = 0 AND Users.DeleteAt = 0 GROUP BY GroupId) AS Members ON Members.GroupId = g.Id").
-			Limit(uint64(perPage)).
-			Offset(uint64(page * perPage)).
-			OrderBy("g.DisplayName")
 	}
 
+	groupsQuery = groupsQuery.
+		From("UserGroups g").
+		Where("g.DeleteAt = 0").
+		Limit(uint64(perPage)).
+		Offset(uint64(page * perPage)).
+		OrderBy("g.DisplayName")
+
 	if len(opts.Q) > 0 {
 		pattern := fmt.Sprintf("%%%s%%", sanitizeSearchTerm(opts.Q, "\\"))
 		operatorKeyword := "ILIKE"

store/storetest/group_store.go

@@ -2046,7 +2046,7 @@ func testGetGroupsByChannel(t *testing.T, ss store.Store) {
 	channel1, err := ss.Channel().Save(channel1, 9999)
 	require.Nil(t, err)
 
-	// Create Groups 1 and 2
+	// Create Groups 1, 2 and a deleted group
 	group1, err := ss.Group().Create(&model.Group{
 		Name:        model.NewId(),
 		DisplayName: "group-1",
@@ -2063,8 +2063,17 @@ func testGetGroupsByChannel(t *testing.T, ss store.Store) {
 	})
 	require.Nil(t, err)
 
+	deletedGroup, err := ss.Group().Create(&model.Group{
+		Name:        model.NewId(),
+		DisplayName: "group-deleted",
+		RemoteId:    model.NewId(),
+		Source:      model.GroupSourceLdap,
+		DeleteAt:    1,
+	})
+	require.Nil(t, err)
+
 	// And associate them with Channel1
-	for _, g := range []*model.Group{group1, group2} {
+	for _, g := range []*model.Group{group1, group2, deletedGroup} {
 		_, err = ss.Group().CreateGroupSyncable(&model.GroupSyncable{
 			AutoAdd:    true,
 			SyncableId: channel1.Id,
@@ -2263,7 +2272,7 @@ func testGetGroupsByTeam(t *testing.T, ss store.Store) {
 	team1, err := ss.Team().Save(team1)
 	require.Nil(t, err)
 
-	// Create Groups 1 and 2
+	// Create Groups 1, 2 and a deleted group
 	group1, err := ss.Group().Create(&model.Group{
 		Name:        model.NewId(),
 		DisplayName: "group-1",
@@ -2280,8 +2289,17 @@ func testGetGroupsByTeam(t *testing.T, ss store.Store) {
 	})
 	require.Nil(t, err)
 
+	deletedGroup, err := ss.Group().Create(&model.Group{
+		Name:        model.NewId(),
+		DisplayName: "group-deleted",
+		RemoteId:    model.NewId(),
+		Source:      model.GroupSourceLdap,
+		DeleteAt:    1,
+	})
+	require.Nil(t, err)
+
 	// And associate them with Team1
-	for _, g := range []*model.Group{group1, group2} {
+	for _, g := range []*model.Group{group1, group2, deletedGroup} {
 		_, err = ss.Group().CreateGroupSyncable(&model.GroupSyncable{
 			AutoAdd:    true,
 			SyncableId: team1.Id,
@@ -2348,6 +2366,9 @@ func testGetGroupsByTeam(t *testing.T, ss store.Store) {
 	_, err = ss.User().Update(user2, true)
 	require.Nil(t, err)
 
+	_, err = ss.Group().UpsertMember(deletedGroup.Id, user1.Id)
+	require.Nil(t, err)
+
 	group1WithMemberCount := *group1
 	group1WithMemberCount.MemberCount = model.NewInt(1)
 
@@ -2512,8 +2533,17 @@ func testGetGroups(t *testing.T, ss store.Store) {
 	})
 	require.Nil(t, err)
 
+	deletedGroup, err := ss.Group().Create(&model.Group{
+		Name:        model.NewId() + "-group-deleted",
+		DisplayName: "group-deleted",
+		RemoteId:    model.NewId(),
+		Source:      model.GroupSourceLdap,
+		DeleteAt:    1,
+	})
+	require.Nil(t, err)
+
 	// And associate them with Team1
-	for _, g := range []*model.Group{group1, group2} {
+	for _, g := range []*model.Group{group1, group2, deletedGroup} {
 		_, err = ss.Group().CreateGroupSyncable(&model.GroupSyncable{
 			AutoAdd:    true,
 			SyncableId: team1.Id,
@@ -2606,6 +2636,9 @@ func testGetGroups(t *testing.T, ss store.Store) {
 	_, err = ss.Group().UpsertMember(group1.Id, user2.Id)
 	require.Nil(t, err)
 
+	_, err = ss.Group().UpsertMember(deletedGroup.Id, user1.Id)
+	require.Nil(t, err)
+
 	user2.DeleteAt = 1
 	ss.User().Update(user2, true)
 
@@ -2701,6 +2734,9 @@ func testGetGroups(t *testing.T, ss store.Store) {
 					if g.Id == group1.Id && *g.MemberCount != 1 {
 						return false
 					}
+					if g.DeleteAt != 0 {
+						return false
+					}
 				}
 				return true
 			},
@@ -2718,6 +2754,9 @@ func testGetGroups(t *testing.T, ss store.Store) {
 					if g.Id == group3.Id {
 						return false
 					}
+					if g.DeleteAt != 0 {
+						return false
+					}
 				}
 				return true
 			},
@@ -2735,6 +2774,9 @@ func testGetGroups(t *testing.T, ss store.Store) {
 					if g.Id == group1.Id || g.Id == group2.Id {
 						return false
 					}
+					if g.DeleteAt != 0 {
+						return false
+					}
 				}
 				return true
 			},