From 1b78f9debc63082b9a684cd0fa67a14e9fe3069d Mon Sep 17 00:00:00 2001 From: Martin Kraft Date: Wed, 15 May 2019 12:03:47 -0400 Subject: [PATCH] MM-14897: Changes to be able to add and remove groups from channels. (#10794) * MM-15162: Changes for LDAP groups removals phase. * MM-14897: Changes to be able to add and remove groups from channels. * Update model/client4.go * MM-14897: PR-requested change to string interpolation. --- api4/group.go | 54 ++++++- api4/group_test.go | 24 ++- app/group.go | 16 +- app/group_test.go | 11 +- cmd/mattermost/commands/group.go | 4 +- model/client4.go | 28 +++- model/group.go | 9 +- store/layered_store.go | 10 +- store/layered_store_supplier.go | 5 +- store/local_cache_supplier_groups.go | 8 +- store/redis_supplier_groups.go | 9 +- store/sqlstore/group_supplier.go | 103 ++++++++---- store/store.go | 5 +- store/storetest/group_supplier.go | 150 ++++++++++++++++-- store/storetest/mocks/GroupStore.go | 26 ++- .../mocks/LayeredStoreDatabaseLayer.go | 33 +++- store/storetest/mocks/LayeredStoreSupplier.go | 33 +++- web/params.go | 90 ++++++----- 18 files changed, 475 insertions(+), 143 deletions(-) diff --git a/api4/group.go b/api4/group.go index a7a4ff02e2..24c7869551 100644 --- a/api4/group.go +++ b/api4/group.go @@ -483,18 +483,44 @@ func getGroupsByChannel(c *Context, w http.ResponseWriter, r *http.Request) { return } - if !c.App.SessionHasPermissionTo(c.App.Session, model.PERMISSION_MANAGE_SYSTEM) { - c.SetPermissionError(model.PERMISSION_MANAGE_SYSTEM) + var permission *model.Permission + channel, err := c.App.GetChannel(c.Params.ChannelId) + if err != nil { + c.Err = err + return + } + if channel.Type == model.CHANNEL_PRIVATE { + permission = model.PERMISSION_MANAGE_PRIVATE_CHANNEL_MEMBERS + } else { + permission = model.PERMISSION_MANAGE_PUBLIC_CHANNEL_MEMBERS + } + if !c.App.SessionHasPermissionToChannel(c.App.Session, c.Params.ChannelId, permission) { + c.SetPermissionError(permission) return } - groups, err := c.App.GetGroupsByChannel(c.Params.ChannelId, c.Params.Page, c.Params.PerPage) + opts := model.GroupSearchOpts{ + Q: c.Params.Q, + IncludeMemberCount: c.Params.IncludeMemberCount, + } + if c.Params.Paginate == nil || *c.Params.Paginate { + opts.PageOpts = &model.PageOpts{Page: c.Params.Page, PerPage: c.Params.PerPage} + } + + groups, totalCount, err := c.App.GetGroupsByChannel(c.Params.ChannelId, opts) if err != nil { c.Err = err return } - b, marshalErr := json.Marshal(groups) + b, marshalErr := json.Marshal(struct { + Groups []*model.Group `json:"groups"` + Count int `json:"total_group_count"` + }{ + Groups: groups, + Count: totalCount, + }) + if marshalErr != nil { c.Err = model.NewAppError("Api4.getGroupsByChannel", "api.marshal_error", nil, marshalErr.Error(), http.StatusInternalServerError) return @@ -569,6 +595,26 @@ func getGroups(c *Context, w http.ResponseWriter, r *http.Request) { opts.NotAssociatedToTeam = teamID } + channelID := c.Params.NotAssociatedToChannel + if len(channelID) == 26 { + channel, err := c.App.GetChannel(channelID) + if err != nil { + c.Err = err + return + } + var permission *model.Permission + if channel.Type == model.CHANNEL_PRIVATE { + permission = model.PERMISSION_MANAGE_PRIVATE_CHANNEL_MEMBERS + } else { + permission = model.PERMISSION_MANAGE_PUBLIC_CHANNEL_MEMBERS + } + if !c.App.SessionHasPermissionToChannel(c.App.Session, channelID, permission) { + c.SetPermissionError(permission) + return + } + opts.NotAssociatedToChannel = channelID + } + groups, err := c.App.GetGroups(c.Params.Page, c.Params.PerPage, opts) if err != nil { c.Err = err diff --git a/api4/group_test.go b/api4/group_test.go index e168acf36d..a94066efdc 100644 --- a/api4/group_test.go +++ b/api4/group_test.go @@ -8,9 +8,10 @@ import ( "net/http" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" + "github.com/mattermost/mattermost-server/model" ) @@ -654,25 +655,34 @@ func TestGetGroupsByChannel(t *testing.T) { }) assert.Nil(t, err) - _, response := th.SystemAdminClient.GetGroupsByChannel("asdfasdf", 0, 60) + opts := model.GroupSearchOpts{ + PageOpts: &model.PageOpts{ + Page: 0, + PerPage: 60, + }, + } + + _, _, response := th.SystemAdminClient.GetGroupsByChannel("asdfasdf", opts) CheckBadRequestStatus(t, response) th.App.SetLicense(nil) - _, response = th.SystemAdminClient.GetGroupsByChannel(th.BasicChannel.Id, 0, 60) + _, _, response = th.SystemAdminClient.GetGroupsByChannel(th.BasicChannel.Id, opts) CheckNotImplementedStatus(t, response) th.App.SetLicense(model.NewTestLicense("ldap")) - _, response = th.Client.GetGroupsByChannel(th.BasicChannel.Id, 0, 60) + privateChannel := th.CreateChannelWithClient(th.SystemAdminClient, model.CHANNEL_PRIVATE) + + _, _, response = th.Client.GetGroupsByChannel(privateChannel.Id, opts) CheckForbiddenStatus(t, response) - groups, response := th.SystemAdminClient.GetGroupsByChannel(th.BasicChannel.Id, 0, 60) + groups, _, response := th.SystemAdminClient.GetGroupsByChannel(th.BasicChannel.Id, opts) assert.Nil(t, response.Error) assert.ElementsMatch(t, []*model.Group{group}, groups) - groups, response = th.SystemAdminClient.GetGroupsByChannel(model.NewId(), 0, 60) - assert.Nil(t, response.Error) + groups, _, response = th.SystemAdminClient.GetGroupsByChannel(model.NewId(), opts) + assert.Equal(t, "store.sql_channel.get.existing.app_error", response.Error.Id) assert.Empty(t, groups) } diff --git a/app/group.go b/app/group.go index 4e9a5c71b7..de4de36e0d 100644 --- a/app/group.go +++ b/app/group.go @@ -165,12 +165,20 @@ func (a *App) ChannelMembersToRemove() ([]*model.ChannelMember, *model.AppError) return result.Data.([]*model.ChannelMember), nil } -func (a *App) GetGroupsByChannel(channelId string, page, perPage int) ([]*model.Group, *model.AppError) { - result := <-a.Srv.Store.Group().GetGroupsByChannel(channelId, page, perPage) +func (a *App) GetGroupsByChannel(channelId string, opts model.GroupSearchOpts) ([]*model.Group, int, *model.AppError) { + result := <-a.Srv.Store.Group().GetGroupsByChannel(channelId, opts) if result.Err != nil { - return nil, result.Err + return nil, 0, result.Err } - return result.Data.([]*model.Group), nil + groups := result.Data.([]*model.Group) + + result = <-a.Srv.Store.Group().CountGroupsByChannel(channelId, opts) + if result.Err != nil { + return nil, 0, result.Err + } + count := result.Data.(int64) + + return groups, int(count), nil } func (a *App) GetGroupsByTeam(teamId string, opts model.GroupSearchOpts) ([]*model.Group, int, *model.AppError) { diff --git a/app/group_test.go b/app/group_test.go index b7f5a1dcf6..f4192a5206 100644 --- a/app/group_test.go +++ b/app/group_test.go @@ -215,11 +215,18 @@ func TestGetGroupsByChannel(t *testing.T) { require.Nil(t, err) require.NotNil(t, gs) - groups, err := th.App.GetGroupsByChannel(th.BasicChannel.Id, 0, 60) + opts := model.GroupSearchOpts{ + PageOpts: &model.PageOpts{ + Page: 0, + PerPage: 60, + }, + } + + groups, _, err := th.App.GetGroupsByChannel(th.BasicChannel.Id, opts) require.Nil(t, err) require.ElementsMatch(t, []*model.Group{group}, groups) - groups, err = th.App.GetGroupsByChannel(model.NewId(), 0, 60) + groups, _, err = th.App.GetGroupsByChannel(model.NewId(), opts) require.Nil(t, err) require.Empty(t, groups) } diff --git a/cmd/mattermost/commands/group.go b/cmd/mattermost/commands/group.go index 1468610ef0..4b2df633eb 100644 --- a/cmd/mattermost/commands/group.go +++ b/cmd/mattermost/commands/group.go @@ -128,7 +128,7 @@ func channelGroupEnableCmdF(command *cobra.Command, args []string) error { return errors.New("Unable to find channel '" + args[0] + "'") } - groups, appErr := a.GetGroupsByChannel(channel.Id, 0, 9999) + groups, _, appErr := a.GetGroupsByChannel(channel.Id, model.GroupSearchOpts{}) if appErr != nil { return appErr } @@ -198,7 +198,7 @@ func channelGroupListCmdF(command *cobra.Command, args []string) error { return errors.New("Unable to find channel '" + args[0] + "'") } - groups, appErr := a.GetGroupsByChannel(channel.Id, 0, 9999) + groups, _, appErr := a.GetGroupsByChannel(channel.Id, model.GroupSearchOpts{}) if appErr != nil { return appErr } diff --git a/model/client4.go b/model/client4.go index a6fe4ab5ce..d90ec78638 100644 --- a/model/client4.go +++ b/model/client4.go @@ -3311,15 +3311,27 @@ func (c *Client4) UnlinkLdapGroup(dn string) (*Group, *Response) { } // GetGroupsByChannel retrieves the Mattermost Groups associated with a given channel -func (c *Client4) GetGroupsByChannel(channelId string, page, perPage int) ([]*Group, *Response) { - path := fmt.Sprintf("%s/groups?page=%v&per_page=%v", c.GetChannelRoute(channelId), page, perPage) +func (c *Client4) GetGroupsByChannel(channelId string, opts GroupSearchOpts) ([]*Group, int, *Response) { + path := fmt.Sprintf("%s/groups?q=%v&include_member_count=%v", c.GetChannelRoute(channelId), opts.Q, opts.IncludeMemberCount) + if opts.PageOpts != nil { + path = fmt.Sprintf("%s&page=%v&per_page=%v", path, opts.PageOpts.Page, opts.PageOpts.PerPage) + } r, appErr := c.DoApiGet(path, "") if appErr != nil { - return nil, BuildErrorResponse(r, appErr) + return nil, 0, BuildErrorResponse(r, appErr) } defer closeBody(r) - return GroupsFromJson(r.Body), BuildResponse(r) + responseData := struct { + Groups []*Group `json:"groups"` + Count int `json:"total_group_count"` + }{} + if err := json.NewDecoder(r.Body).Decode(&responseData); err != nil { + appErr := NewAppError("Api4.GetGroupsByChannel", "api.marshal_error", nil, err.Error(), http.StatusInternalServerError) + return nil, 0, BuildErrorResponse(r, appErr) + } + + return responseData.Groups, responseData.Count, BuildResponse(r) } // GetGroupsByTeam retrieves the Mattermost Groups associated with a given team @@ -3349,8 +3361,12 @@ func (c *Client4) GetGroupsByTeam(teamId string, opts GroupSearchOpts) ([]*Group // GetGroups retrieves Mattermost Groups func (c *Client4) GetGroups(opts GroupSearchOpts) ([]*Group, *Response) { path := fmt.Sprintf( - "%s?include_member_count=%v¬_associated_to_team=%v&q=%v", - c.GetGroupsRoute(), opts.IncludeMemberCount, opts.NotAssociatedToTeam, opts.Q, + "%s?include_member_count=%v¬_associated_to_team=%v¬_associated_to_channel=%v&q=%v", + c.GetGroupsRoute(), + opts.IncludeMemberCount, + opts.NotAssociatedToTeam, + opts.NotAssociatedToChannel, + opts.Q, ) if opts.PageOpts != nil { path = fmt.Sprintf("%s&page=%v&per_page=%v", path, opts.PageOpts.Page, opts.PageOpts.PerPage) diff --git a/model/group.go b/model/group.go index 627676250b..535fbdf8ca 100644 --- a/model/group.go +++ b/model/group.go @@ -56,10 +56,11 @@ type LdapGroupSearchOpts struct { } type GroupSearchOpts struct { - Q string - NotAssociatedToTeam string - IncludeMemberCount bool - PageOpts *PageOpts + Q string + NotAssociatedToTeam string + NotAssociatedToChannel string + IncludeMemberCount bool + PageOpts *PageOpts } type PageOpts struct { diff --git a/store/layered_store.go b/store/layered_store.go index be50ba5894..64484c1aa0 100644 --- a/store/layered_store.go +++ b/store/layered_store.go @@ -466,9 +466,15 @@ func (s *LayeredGroupStore) ChannelMembersToRemove() StoreChannel { }) } -func (s *LayeredGroupStore) GetGroupsByChannel(channelId string, page, perPage int) StoreChannel { +func (s *LayeredGroupStore) GetGroupsByChannel(channelId string, opts model.GroupSearchOpts) StoreChannel { return s.RunQuery(func(supplier LayeredStoreSupplier) *LayeredStoreSupplierResult { - return supplier.GetGroupsByChannel(s.TmpContext, channelId, page, perPage) + return supplier.GetGroupsByChannel(s.TmpContext, channelId, opts) + }) +} + +func (s *LayeredGroupStore) CountGroupsByChannel(channelId string, opts model.GroupSearchOpts) StoreChannel { + return s.RunQuery(func(supplier LayeredStoreSupplier) *LayeredStoreSupplierResult { + return supplier.CountGroupsByChannel(s.TmpContext, channelId, opts) }) } diff --git a/store/layered_store_supplier.go b/store/layered_store_supplier.go index 3cca15714c..6ad24e5e69 100644 --- a/store/layered_store_supplier.go +++ b/store/layered_store_supplier.go @@ -74,8 +74,11 @@ type LayeredStoreSupplier interface { TeamMembersToRemove(ctx context.Context, hints ...LayeredStoreHint) *LayeredStoreSupplierResult ChannelMembersToRemove(ctx context.Context, hints ...LayeredStoreHint) *LayeredStoreSupplierResult - GetGroupsByChannel(ctx context.Context, channelId string, page, perPage int, hints ...LayeredStoreHint) *LayeredStoreSupplierResult + GetGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult + CountGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult + GetGroupsByTeam(ctx context.Context, teamId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult CountGroupsByTeam(ctx context.Context, teamId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult + GetGroups(ctx context.Context, page, perPage int, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult } diff --git a/store/local_cache_supplier_groups.go b/store/local_cache_supplier_groups.go index 962404456f..f2ee6f2da5 100644 --- a/store/local_cache_supplier_groups.go +++ b/store/local_cache_supplier_groups.go @@ -109,8 +109,12 @@ func (s *LocalCacheSupplier) ChannelMembersToRemove(ctx context.Context, hints . return s.Next().ChannelMembersToRemove(ctx, hints...) } -func (s *LocalCacheSupplier) GetGroupsByChannel(ctx context.Context, channelId string, page, perPage int, hints ...LayeredStoreHint) *LayeredStoreSupplierResult { - return s.Next().GetGroupsByChannel(ctx, channelId, page, perPage, hints...) +func (s *LocalCacheSupplier) GetGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult { + return s.Next().GetGroupsByChannel(ctx, channelId, opts, hints...) +} + +func (s *LocalCacheSupplier) CountGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult { + return s.Next().CountGroupsByChannel(ctx, channelId, opts, hints...) } func (s *LocalCacheSupplier) GetGroupsByTeam(ctx context.Context, teamId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult { diff --git a/store/redis_supplier_groups.go b/store/redis_supplier_groups.go index 6d570d3312..986f9adee2 100644 --- a/store/redis_supplier_groups.go +++ b/store/redis_supplier_groups.go @@ -109,9 +109,14 @@ func (s *RedisSupplier) ChannelMembersToRemove(ctx context.Context, hints ...Lay return s.Next().ChannelMembersToRemove(ctx, hints...) } -func (s *RedisSupplier) GetGroupsByChannel(ctx context.Context, channelId string, page, perPage int, hints ...LayeredStoreHint) *LayeredStoreSupplierResult { +func (s *RedisSupplier) GetGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult { // TODO: Redis caching. - return s.Next().GetGroupsByChannel(ctx, channelId, page, perPage, hints...) + return s.Next().GetGroupsByChannel(ctx, channelId, opts, hints...) +} + +func (s *RedisSupplier) CountGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult { + // TODO: Redis caching. + return s.Next().CountGroupsByChannel(ctx, channelId, opts, hints...) } func (s *RedisSupplier) GetGroupsByTeam(ctx context.Context, teamId string, opts model.GroupSearchOpts, hints ...LayeredStoreHint) *LayeredStoreSupplierResult { diff --git a/store/sqlstore/group_supplier.go b/store/sqlstore/group_supplier.go index 3ab87d6607..9addedff70 100644 --- a/store/sqlstore/group_supplier.go +++ b/store/sqlstore/group_supplier.go @@ -827,32 +827,47 @@ func (s *SqlSupplier) TeamMembersToRemove(ctx context.Context, hints ...store.La return result } -func (s *SqlSupplier) GetGroupsByChannel(ctx context.Context, channelId string, page, perPage int, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { +func (s *SqlSupplier) CountGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { result := store.NewSupplierResult() - var groups []*model.Group - offset := page * perPage - _, err := s.GetReplica().Select(&groups, ` - SELECT - ug.* - FROM - GroupChannels gc - LEFT JOIN - UserGroups ug - ON - gc.GroupId = ug.Id - WHERE - gc.DeleteAt = 0 - AND - ug.DeleteAt = 0 - AND - gc.ChannelId = :ChannelId - ORDER BY - ug.DisplayName - LIMIT :Limit - OFFSET :Offset`, - map[string]interface{}{"ChannelId": channelId, "Limit": perPage, "Offset": offset}) + countQuery := s.groupsBySyncableBaseQuery(model.GroupSyncableTypeChannel, selectCountGroups, channelId, opts) + countQueryString, args, err := countQuery.ToSql() + if err != nil { + result.Err = model.NewAppError("SqlGroupStore.CountGroupsByChannel", "store.sql_group.app_error", nil, err.Error(), http.StatusInternalServerError) + return result + } + + count, err := s.GetReplica().SelectInt(countQueryString, args...) + if err != nil { + result.Err = model.NewAppError("SqlGroupStore.CountGroupsByChannel", "store.select_error", nil, err.Error(), http.StatusInternalServerError) + return result + } + + result.Data = count + + return result +} + +func (s *SqlSupplier) GetGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { + result := store.NewSupplierResult() + + query := s.groupsBySyncableBaseQuery(model.GroupSyncableTypeChannel, selectGroups, channelId, opts) + + if opts.PageOpts != nil { + offset := uint64(opts.PageOpts.Page * opts.PageOpts.PerPage) + query = query.OrderBy("ug.DisplayName").Limit(uint64(opts.PageOpts.PerPage)).Offset(offset) + } + + queryString, args, err := query.ToSql() + if err != nil { + result.Err = model.NewAppError("SqlGroupStore.GetGroupsByChannel", "store.sql_group.app_error", nil, err.Error(), http.StatusInternalServerError) + return result + } + + var groups []*model.Group + + _, err = s.GetReplica().Select(&groups, queryString, args...) if err != nil { result.Err = model.NewAppError("SqlGroupStore.GetGroupsByChannel", "store.select_error", nil, err.Error(), http.StatusInternalServerError) return result @@ -919,25 +934,35 @@ func (s *SqlSupplier) ChannelMembersToRemove(ctx context.Context, hints ...store return result } -func (s *SqlSupplier) groupsByTeamBaseQuery(t selectType, teamID string, opts model.GroupSearchOpts) squirrel.SelectBuilder { +func (s *SqlSupplier) groupsBySyncableBaseQuery(st model.GroupSyncableType, t selectType, syncableID string, opts model.GroupSearchOpts) squirrel.SelectBuilder { selectStrs := map[selectType]string{ selectGroups: "ug.*", selectCountGroups: "COUNT(*)", } + var table string + var idCol string + if st == model.GroupSyncableTypeTeam { + table = "GroupTeams" + idCol = "TeamId" + } else { + table = "GroupChannels" + idCol = "ChannelId" + } + query := s.getQueryBuilder(). Select(selectStrs[t]). - From("GroupTeams gt"). - LeftJoin("UserGroups ug ON gt.GroupId = ug.Id"). - Where("ug.DeleteAt = 0 AND gt.TeamId = ? AND gt.DeleteAt = 0", teamID) + From(fmt.Sprintf("%s gs", table)). + LeftJoin("UserGroups ug ON gs.GroupId = ug.Id"). + Where(fmt.Sprintf("ug.DeleteAt = 0 AND gs.%s = ? AND gs.DeleteAt = 0", idCol), syncableID) if opts.IncludeMemberCount && t == selectGroups { query = s.getQueryBuilder(). Select("ug.*, coalesce(Members.MemberCount, 0) AS MemberCount"). From("UserGroups ug"). LeftJoin("(SELECT GroupMembers.GroupId, COUNT(*) AS MemberCount FROM GroupMembers WHERE GroupMembers.DeleteAt = 0 GROUP BY GroupId) AS Members ON Members.GroupId = ug.Id"). - LeftJoin("GroupTeams ON GroupTeams.GroupId = ug.Id"). - Where("GroupTeams.DeleteAt = 0 AND GroupTeams.TeamId = ?", teamID). + 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). OrderBy("ug.DisplayName") } @@ -956,7 +981,7 @@ func (s *SqlSupplier) groupsByTeamBaseQuery(t selectType, teamID string, opts mo func (s *SqlSupplier) CountGroupsByTeam(ctx context.Context, teamId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { result := store.NewSupplierResult() - countQuery := s.groupsByTeamBaseQuery(selectCountGroups, teamId, opts) + countQuery := s.groupsBySyncableBaseQuery(model.GroupSyncableTypeTeam, selectCountGroups, teamId, opts) countQueryString, args, err := countQuery.ToSql() if err != nil { @@ -978,7 +1003,7 @@ func (s *SqlSupplier) CountGroupsByTeam(ctx context.Context, teamId string, opts func (s *SqlSupplier) GetGroupsByTeam(ctx context.Context, teamId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { result := store.NewSupplierResult() - query := s.groupsByTeamBaseQuery(selectGroups, teamId, opts) + query := s.groupsBySyncableBaseQuery(model.GroupSyncableTypeTeam, selectGroups, teamId, opts) if opts.PageOpts != nil { offset := uint64(opts.PageOpts.Page * opts.PageOpts.PerPage) @@ -1045,6 +1070,22 @@ func (s *SqlSupplier) GetGroups(ctx context.Context, page, perPage int, opts mod `, opts.NotAssociatedToTeam) } + if len(opts.NotAssociatedToChannel) == 26 { + groupsQuery = groupsQuery.Where(` + g.Id NOT IN ( + SELECT + Id + FROM + UserGroups + JOIN GroupChannels ON GroupChannels.GroupId = UserGroups.Id + WHERE + GroupChannels.DeleteAt = 0 + AND UserGroups.DeleteAt = 0 + AND GroupChannels.ChannelId = ? + ) + `, opts.NotAssociatedToChannel) + } + queryString, args, err := groupsQuery.ToSql() if err != nil { result.Err = model.NewAppError("SqlGroupStore.GetGroups", "store.sql_group.app_error", nil, err.Error(), http.StatusInternalServerError) diff --git a/store/store.go b/store/store.go index bc18f9d1f7..aafbc15d99 100644 --- a/store/store.go +++ b/store/store.go @@ -594,9 +594,12 @@ type GroupStore interface { TeamMembersToRemove() StoreChannel ChannelMembersToRemove() StoreChannel - GetGroupsByChannel(channelId string, page, perPage int) StoreChannel + GetGroupsByChannel(channelId string, opts model.GroupSearchOpts) StoreChannel + CountGroupsByChannel(channelId string, opts model.GroupSearchOpts) StoreChannel + GetGroupsByTeam(teamId string, opts model.GroupSearchOpts) StoreChannel CountGroupsByTeam(teamId string, opts model.GroupSearchOpts) StoreChannel + GetGroups(page, perPage int, opts model.GroupSearchOpts) StoreChannel } diff --git a/store/storetest/group_supplier.go b/store/storetest/group_supplier.go index 537c444b10..3951b6c2db 100644 --- a/store/storetest/group_supplier.go +++ b/store/storetest/group_supplier.go @@ -1633,23 +1633,44 @@ func testGetGroupsByChannel(t *testing.T, ss store.Store) { }) require.Nil(t, res.Err) + // add members + u1 := &model.User{ + Email: MakeEmail(), + Username: model.NewId(), + } + res = <-ss.User().Save(u1) + require.Nil(t, res.Err) + user1 := res.Data.(*model.User) + <-ss.Group().CreateOrRestoreMember(group1.Id, user1.Id) + + group1WithMemberCount := model.Group(*group1) + group1WithMemberCount.MemberCount = model.NewInt(1) + + group2WithMemberCount := model.Group(*group2) + group2WithMemberCount.MemberCount = model.NewInt(0) + testCases := []struct { - Name string - ChannelId string - Page int - PerPage int - Result []*model.Group + Name string + ChannelId string + Page int + PerPage int + Result []*model.Group + Opts model.GroupSearchOpts + TotalCount *int64 }{ { - Name: "Get the two Groups for Channel1", - ChannelId: channel1.Id, - Page: 0, - PerPage: 60, - Result: []*model.Group{group1, group2}, + Name: "Get the two Groups for Channel1", + ChannelId: channel1.Id, + Opts: model.GroupSearchOpts{}, + Page: 0, + PerPage: 60, + Result: []*model.Group{group1, group2}, + TotalCount: model.NewInt64(2), }, { Name: "Get first Group for Channel1 with page 0 with 1 element", ChannelId: channel1.Id, + Opts: model.GroupSearchOpts{}, Page: 0, PerPage: 1, Result: []*model.Group{group1}, @@ -1657,6 +1678,7 @@ func testGetGroupsByChannel(t *testing.T, ss store.Store) { { Name: "Get second Group for Channel1 with page 1 with 1 element", ChannelId: channel1.Id, + Opts: model.GroupSearchOpts{}, Page: 1, PerPage: 1, Result: []*model.Group{group2}, @@ -1664,24 +1686,72 @@ func testGetGroupsByChannel(t *testing.T, ss store.Store) { { Name: "Get third Group for Channel2", ChannelId: channel2.Id, + Opts: model.GroupSearchOpts{}, Page: 0, PerPage: 60, Result: []*model.Group{group3}, }, { - Name: "Get empty Groups for a fake id", - ChannelId: model.NewId(), + Name: "Get empty Groups for a fake id", + ChannelId: model.NewId(), + Opts: model.GroupSearchOpts{}, + Page: 0, + PerPage: 60, + Result: []*model.Group{}, + TotalCount: model.NewInt64(0), + }, + { + Name: "Get group matching name", + ChannelId: channel1.Id, + Opts: model.GroupSearchOpts{Q: string([]rune(group1.Name)[2:10])}, // very low change of a name collision + Page: 0, + PerPage: 100, + Result: []*model.Group{group1}, + TotalCount: model.NewInt64(1), + }, + { + Name: "Get group matching display name", + ChannelId: channel1.Id, + Opts: model.GroupSearchOpts{Q: "rouP-1"}, + Page: 0, + PerPage: 100, + Result: []*model.Group{group1}, + TotalCount: model.NewInt64(1), + }, + { + Name: "Get group matching multiple display names", + ChannelId: channel1.Id, + Opts: model.GroupSearchOpts{Q: "roUp-"}, + Page: 0, + PerPage: 100, + Result: []*model.Group{group1, group2}, + TotalCount: model.NewInt64(2), + }, + { + Name: "Include member counts", + ChannelId: channel1.Id, + Opts: model.GroupSearchOpts{IncludeMemberCount: true}, Page: 0, - PerPage: 60, - Result: []*model.Group{}, + PerPage: 2, + Result: []*model.Group{&group1WithMemberCount, &group2WithMemberCount}, }, } for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { - res := <-ss.Group().GetGroupsByChannel(tc.ChannelId, tc.Page, tc.PerPage) + if tc.Opts.PageOpts == nil { + tc.Opts.PageOpts = &model.PageOpts{} + } + tc.Opts.PageOpts.Page = tc.Page + tc.Opts.PageOpts.PerPage = tc.PerPage + res := <-ss.Group().GetGroupsByChannel(tc.ChannelId, tc.Opts) require.Nil(t, res.Err) require.ElementsMatch(t, tc.Result, res.Data.([]*model.Group)) + if tc.TotalCount != nil { + res = <-ss.Group().CountGroupsByChannel(tc.ChannelId, tc.Opts) + count := res.Data.(int64) + require.Equal(t, *tc.TotalCount, count) + } }) } } @@ -1904,8 +1974,19 @@ func testGetGroups(t *testing.T, ss store.Store) { team1, err := ss.Team().Save(team1) require.Nil(t, err) + // Create Channel1 + channel1 := &model.Channel{ + TeamId: model.NewId(), + DisplayName: "Channel1", + Name: model.NewId(), + Type: model.CHANNEL_PRIVATE, + } + res := <-ss.Channel().Save(channel1, 9999) + require.Nil(t, res.Err) + channel1 = res.Data.(*model.Channel) + // Create Groups 1 and 2 - res := <-ss.Group().Create(&model.Group{ + res = <-ss.Group().Create(&model.Group{ Name: model.NewId(), DisplayName: "group-1", RemoteId: model.NewId(), @@ -1948,6 +2029,17 @@ func testGetGroups(t *testing.T, ss store.Store) { team2, err = ss.Team().Save(team2) require.Nil(t, err) + // Create Channel2 + channel2 := &model.Channel{ + TeamId: model.NewId(), + DisplayName: "Channel2", + Name: model.NewId(), + Type: model.CHANNEL_PRIVATE, + } + res = <-ss.Channel().Save(channel2, 9999) + require.Nil(t, res.Err) + channel2 = res.Data.(*model.Channel) + // Create Group3 res = <-ss.Group().Create(&model.Group{ Name: model.NewId(), @@ -1967,6 +2059,26 @@ func testGetGroups(t *testing.T, ss store.Store) { }) require.Nil(t, res.Err) + // And associate Group1 to Channel2 + res = <-ss.Group().CreateGroupSyncable(&model.GroupSyncable{ + AutoAdd: true, + SyncableId: channel2.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: group1.Id, + }) + require.Nil(t, res.Err) + + // And associate Group2 and Group3 to Channel1 + for _, g := range []*model.Group{group2, group3} { + res = <-ss.Group().CreateGroupSyncable(&model.GroupSyncable{ + AutoAdd: true, + SyncableId: channel1.Id, + Type: model.GroupSyncableTypeChannel, + GroupId: g.Id, + }) + require.Nil(t, res.Err) + } + // add members u1 := &model.User{ Email: MakeEmail(), @@ -2082,6 +2194,9 @@ func testGetGroups(t *testing.T, ss store.Store) { Page: 0, PerPage: 100, Resultf: func(groups []*model.Group) bool { + if len(groups) == 0 { + return false + } for _, g := range groups { if g.Id == group3.Id { return false @@ -2096,6 +2211,9 @@ func testGetGroups(t *testing.T, ss store.Store) { Page: 0, PerPage: 100, Resultf: func(groups []*model.Group) bool { + if len(groups) == 0 { + return false + } for _, g := range groups { if g.Id == group1.Id || g.Id == group2.Id { return false diff --git a/store/storetest/mocks/GroupStore.go b/store/storetest/mocks/GroupStore.go index 73e7bd0926..e04a48cc1a 100644 --- a/store/storetest/mocks/GroupStore.go +++ b/store/storetest/mocks/GroupStore.go @@ -45,6 +45,22 @@ func (_m *GroupStore) ChannelMembersToRemove() store.StoreChannel { return r0 } +// CountGroupsByChannel provides a mock function with given fields: channelId, opts +func (_m *GroupStore) CountGroupsByChannel(channelId string, opts model.GroupSearchOpts) store.StoreChannel { + ret := _m.Called(channelId, opts) + + var r0 store.StoreChannel + if rf, ok := ret.Get(0).(func(string, model.GroupSearchOpts) store.StoreChannel); ok { + r0 = rf(channelId, opts) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(store.StoreChannel) + } + } + + return r0 +} + // CountGroupsByTeam provides a mock function with given fields: teamId, opts func (_m *GroupStore) CountGroupsByTeam(teamId string, opts model.GroupSearchOpts) store.StoreChannel { ret := _m.Called(teamId, opts) @@ -253,13 +269,13 @@ func (_m *GroupStore) GetGroups(page int, perPage int, opts model.GroupSearchOpt return r0 } -// GetGroupsByChannel provides a mock function with given fields: channelId, page, perPage -func (_m *GroupStore) GetGroupsByChannel(channelId string, page int, perPage int) store.StoreChannel { - ret := _m.Called(channelId, page, perPage) +// GetGroupsByChannel provides a mock function with given fields: channelId, opts +func (_m *GroupStore) GetGroupsByChannel(channelId string, opts model.GroupSearchOpts) store.StoreChannel { + ret := _m.Called(channelId, opts) var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, int, int) store.StoreChannel); ok { - r0 = rf(channelId, page, perPage) + if rf, ok := ret.Get(0).(func(string, model.GroupSearchOpts) store.StoreChannel); ok { + r0 = rf(channelId, opts) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(store.StoreChannel) diff --git a/store/storetest/mocks/LayeredStoreDatabaseLayer.go b/store/storetest/mocks/LayeredStoreDatabaseLayer.go index 236802106e..90dde216af 100644 --- a/store/storetest/mocks/LayeredStoreDatabaseLayer.go +++ b/store/storetest/mocks/LayeredStoreDatabaseLayer.go @@ -193,6 +193,29 @@ func (_m *LayeredStoreDatabaseLayer) Compliance() store.ComplianceStore { return r0 } +// CountGroupsByChannel provides a mock function with given fields: ctx, channelId, opts, hints +func (_m *LayeredStoreDatabaseLayer) CountGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { + _va := make([]interface{}, len(hints)) + for _i := range hints { + _va[_i] = hints[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, channelId, opts) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 *store.LayeredStoreSupplierResult + if rf, ok := ret.Get(0).(func(context.Context, string, model.GroupSearchOpts, ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult); ok { + r0 = rf(ctx, channelId, opts, hints...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*store.LayeredStoreSupplierResult) + } + } + + return r0 +} + // CountGroupsByTeam provides a mock function with given fields: ctx, teamId, opts, hints func (_m *LayeredStoreDatabaseLayer) CountGroupsByTeam(ctx context.Context, teamId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { _va := make([]interface{}, len(hints)) @@ -276,20 +299,20 @@ func (_m *LayeredStoreDatabaseLayer) GetGroups(ctx context.Context, page int, pe return r0 } -// GetGroupsByChannel provides a mock function with given fields: ctx, channelId, page, perPage, hints -func (_m *LayeredStoreDatabaseLayer) GetGroupsByChannel(ctx context.Context, channelId string, page int, perPage int, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { +// GetGroupsByChannel provides a mock function with given fields: ctx, channelId, opts, hints +func (_m *LayeredStoreDatabaseLayer) GetGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { _va := make([]interface{}, len(hints)) for _i := range hints { _va[_i] = hints[_i] } var _ca []interface{} - _ca = append(_ca, ctx, channelId, page, perPage) + _ca = append(_ca, ctx, channelId, opts) _ca = append(_ca, _va...) ret := _m.Called(_ca...) var r0 *store.LayeredStoreSupplierResult - if rf, ok := ret.Get(0).(func(context.Context, string, int, int, ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult); ok { - r0 = rf(ctx, channelId, page, perPage, hints...) + if rf, ok := ret.Get(0).(func(context.Context, string, model.GroupSearchOpts, ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult); ok { + r0 = rf(ctx, channelId, opts, hints...) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*store.LayeredStoreSupplierResult) diff --git a/store/storetest/mocks/LayeredStoreSupplier.go b/store/storetest/mocks/LayeredStoreSupplier.go index 61e598373f..755ceda6a8 100644 --- a/store/storetest/mocks/LayeredStoreSupplier.go +++ b/store/storetest/mocks/LayeredStoreSupplier.go @@ -60,6 +60,29 @@ func (_m *LayeredStoreSupplier) ChannelMembersToRemove(ctx context.Context, hint return r0 } +// CountGroupsByChannel provides a mock function with given fields: ctx, channelId, opts, hints +func (_m *LayeredStoreSupplier) CountGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { + _va := make([]interface{}, len(hints)) + for _i := range hints { + _va[_i] = hints[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, channelId, opts) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 *store.LayeredStoreSupplierResult + if rf, ok := ret.Get(0).(func(context.Context, string, model.GroupSearchOpts, ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult); ok { + r0 = rf(ctx, channelId, opts, hints...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*store.LayeredStoreSupplierResult) + } + } + + return r0 +} + // CountGroupsByTeam provides a mock function with given fields: ctx, teamId, opts, hints func (_m *LayeredStoreSupplier) CountGroupsByTeam(ctx context.Context, teamId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { _va := make([]interface{}, len(hints)) @@ -106,20 +129,20 @@ func (_m *LayeredStoreSupplier) GetGroups(ctx context.Context, page int, perPage return r0 } -// GetGroupsByChannel provides a mock function with given fields: ctx, channelId, page, perPage, hints -func (_m *LayeredStoreSupplier) GetGroupsByChannel(ctx context.Context, channelId string, page int, perPage int, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { +// GetGroupsByChannel provides a mock function with given fields: ctx, channelId, opts, hints +func (_m *LayeredStoreSupplier) GetGroupsByChannel(ctx context.Context, channelId string, opts model.GroupSearchOpts, hints ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult { _va := make([]interface{}, len(hints)) for _i := range hints { _va[_i] = hints[_i] } var _ca []interface{} - _ca = append(_ca, ctx, channelId, page, perPage) + _ca = append(_ca, ctx, channelId, opts) _ca = append(_ca, _va...) ret := _m.Called(_ca...) var r0 *store.LayeredStoreSupplierResult - if rf, ok := ret.Get(0).(func(context.Context, string, int, int, ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult); ok { - r0 = rf(ctx, channelId, page, perPage, hints...) + if rf, ok := ret.Get(0).(func(context.Context, string, model.GroupSearchOpts, ...store.LayeredStoreHint) *store.LayeredStoreSupplierResult); ok { + r0 = rf(ctx, channelId, opts, hints...) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*store.LayeredStoreSupplierResult) diff --git a/web/params.go b/web/params.go index 0e21c1e553..b457800dcd 100644 --- a/web/params.go +++ b/web/params.go @@ -21,50 +21,51 @@ const ( ) type Params struct { - UserId string - TeamId string - InviteId string - TokenId string - ChannelId string - PostId string - FileId string - Filename string - PluginId string - CommandId string - HookId string - ReportId string - EmojiId string - AppId string - Email string - Username string - TeamName string - ChannelName string - PreferenceName string - EmojiName string - Category string - Service string - JobId string - JobType string - ActionId string - RoleId string - RoleName string - SchemeId string - Scope string - GroupId string - Page int - PerPage int - LogsPerPage int - Permanent bool - RemoteId string - SyncableId string - SyncableType model.GroupSyncableType - BotUserId string - Q string - IsLinked *bool - IsConfigured *bool - NotAssociatedToTeam string - Paginate *bool - IncludeMemberCount bool + UserId string + TeamId string + InviteId string + TokenId string + ChannelId string + PostId string + FileId string + Filename string + PluginId string + CommandId string + HookId string + ReportId string + EmojiId string + AppId string + Email string + Username string + TeamName string + ChannelName string + PreferenceName string + EmojiName string + Category string + Service string + JobId string + JobType string + ActionId string + RoleId string + RoleName string + SchemeId string + Scope string + GroupId string + Page int + PerPage int + LogsPerPage int + Permanent bool + RemoteId string + SyncableId string + SyncableType model.GroupSyncableType + BotUserId string + Q string + IsLinked *bool + IsConfigured *bool + NotAssociatedToTeam string + NotAssociatedToChannel string + Paginate *bool + IncludeMemberCount bool } func ParamsFromRequest(r *http.Request) *Params { @@ -249,6 +250,7 @@ func ParamsFromRequest(r *http.Request) *Params { } params.NotAssociatedToTeam = query.Get("not_associated_to_team") + params.NotAssociatedToChannel = query.Get("not_associated_to_channel") if val, err := strconv.ParseBool(query.Get("paginate")); err == nil { params.Paginate = &val