From 036f9384b4db2678c1e24894d9b63e0503c0942c Mon Sep 17 00:00:00 2001 From: Farhan Munshi <3207297+fmunshi@users.noreply.github.com> Date: Fri, 24 Apr 2020 17:12:54 -0400 Subject: [PATCH] [MM-23264] Get channel member counts by group (#14068) * MM-23264 Add api endpoint for get groups with members in channel Add store tests Add tests for api func Gofmt Apply changes from code review * MM-23264 Make store layers * MM-23264 Check read permission on channel member counts * Trigger CI --- api4/channel.go | 34 +++++ api4/channel_test.go | 92 ++++++++++++ i18n/en.json | 4 + model/channel.go | 12 ++ model/client4.go | 9 ++ store/opentracing_layer.go | 18 +++ store/sqlstore/channel_store.go | 32 ++++ store/store.go | 1 + store/storetest/channel_store.go | 203 ++++++++++++++++++++++++++ store/storetest/mocks/ChannelStore.go | 25 ++++ store/timer_layer.go | 16 ++ 11 files changed, 446 insertions(+) diff --git a/api4/channel.go b/api4/channel.go index fe5a4f368b..882f2f0f4a 100644 --- a/api4/channel.go +++ b/api4/channel.go @@ -44,6 +44,7 @@ func (api *API) InitChannel() { api.BaseRoutes.Channel.Handle("/pinned", api.ApiSessionRequired(getPinnedPosts)).Methods("GET") api.BaseRoutes.Channel.Handle("/timezones", api.ApiSessionRequired(getChannelMembersTimezones)).Methods("GET") api.BaseRoutes.Channel.Handle("/members_minus_group_members", api.ApiSessionRequired(channelMembersMinusGroupMembers)).Methods("GET") + api.BaseRoutes.Channel.Handle("/member_counts_by_group", api.ApiSessionRequired(channelMemberCountsByGroup)).Methods("GET") api.BaseRoutes.ChannelForUser.Handle("/unread", api.ApiSessionRequired(getChannelUnread)).Methods("GET") @@ -1639,6 +1640,39 @@ func channelMembersMinusGroupMembers(c *Context, w http.ResponseWriter, r *http. w.Write(b) } +func channelMemberCountsByGroup(c *Context, w http.ResponseWriter, r *http.Request) { + if c.App.License() == nil { + c.Err = model.NewAppError("Api4.channelMemberCountsByGroup", "api.channel.channel_member_counts_by_group.license.error", nil, "", http.StatusNotImplemented) + return + } + + c.RequireChannelId() + if c.Err != nil { + return + } + + if !c.App.SessionHasPermissionToChannel(*c.App.Session(), c.Params.ChannelId, model.PERMISSION_READ_CHANNEL) { + c.SetPermissionError(model.PERMISSION_READ_CHANNEL) + return + } + + includeTimezones := r.URL.Query().Get("include_timezones") == "true" + + channelMemberCounts, err := c.App.Srv().Store.Channel().GetMemberCountsByGroup(c.Params.ChannelId, includeTimezones) + if err != nil { + c.Err = err + return + } + + b, marshalErr := json.Marshal(channelMemberCounts) + if marshalErr != nil { + c.Err = model.NewAppError("Api4.channelMemberCountsByGroup", "api.marshal_error", nil, marshalErr.Error(), http.StatusInternalServerError) + return + } + + w.Write(b) +} + func getChannelModerations(c *Context, w http.ResponseWriter, r *http.Request) { if c.App.License() == nil { c.Err = model.NewAppError("Api4.GetChannelModerations", "api.channel.get_channel_moderations.license.error", nil, "", http.StatusNotImplemented) diff --git a/api4/channel_test.go b/api4/channel_test.go index 5dfc6459c3..245826c1e9 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -3542,3 +3542,95 @@ func TestPatchChannelModerations(t *testing.T) { }) } + +func TestGetChannelMemberCountsByGroup(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + channel := th.BasicChannel + t.Run("Errors without a license", func(t *testing.T) { + _, res := th.SystemAdminClient.GetChannelMemberCountsByGroup(channel.Id, false, "") + require.Equal(t, "api.channel.channel_member_counts_by_group.license.error", res.Error.Id) + }) + + th.App.SetLicense(model.NewTestLicense()) + + t.Run("Errors without read permission to the channel", func(t *testing.T) { + _, res := th.Client.GetChannelMemberCountsByGroup(model.NewId(), false, "") + require.Equal(t, "api.context.permissions.app_error", res.Error.Id) + }) + + t.Run("Returns empty for a channel with no members or groups", func(t *testing.T) { + memberCounts, _ := th.SystemAdminClient.GetChannelMemberCountsByGroup(channel.Id, false, "") + require.Equal(t, []*model.ChannelMemberCountByGroup{}, memberCounts) + }) + + user := th.BasicUser + user.Timezone["useAutomaticTimezone"] = "false" + user.Timezone["manualTimezone"] = "XOXO/BLABLA" + _, err := th.App.UpsertGroupMember(th.Group.Id, user.Id) + require.Nil(t, err) + _, resp := th.SystemAdminClient.UpdateUser(user) + CheckNoError(t, resp) + + user2 := th.BasicUser2 + user2.Timezone["automaticTimezone"] = "NoWhere/Island" + _, err = th.App.UpsertGroupMember(th.Group.Id, user2.Id) + require.Nil(t, err) + _, resp = th.SystemAdminClient.UpdateUser(user2) + CheckNoError(t, resp) + + t.Run("Returns users in group without timezones", func(t *testing.T) { + memberCounts, _ := th.SystemAdminClient.GetChannelMemberCountsByGroup(channel.Id, false, "") + expectedMemberCounts := []*model.ChannelMemberCountByGroup{ + { + GroupId: th.Group.Id, + ChannelMemberCount: 2, + ChannelMemberTimezonesCount: 0, + }, + } + require.Equal(t, expectedMemberCounts, memberCounts) + }) + + t.Run("Returns users in group with timezones", func(t *testing.T) { + memberCounts, _ := th.SystemAdminClient.GetChannelMemberCountsByGroup(channel.Id, true, "") + expectedMemberCounts := []*model.ChannelMemberCountByGroup{ + { + GroupId: th.Group.Id, + ChannelMemberCount: 2, + ChannelMemberTimezonesCount: 2, + }, + } + require.Equal(t, expectedMemberCounts, memberCounts) + }) + + id := model.NewId() + group := &model.Group{ + DisplayName: "dn_" + id, + Name: "name" + id, + Source: model.GroupSourceLdap, + RemoteId: model.NewId(), + } + + _, err = th.App.CreateGroup(group) + require.Nil(t, err) + _, err = th.App.UpsertGroupMember(group.Id, user.Id) + require.Nil(t, err) + + t.Run("Returns multiple groups with users in group with timezones", func(t *testing.T) { + memberCounts, _ := th.SystemAdminClient.GetChannelMemberCountsByGroup(channel.Id, true, "") + expectedMemberCounts := []*model.ChannelMemberCountByGroup{ + { + GroupId: group.Id, + ChannelMemberCount: 1, + ChannelMemberTimezonesCount: 1, + }, + { + GroupId: th.Group.Id, + ChannelMemberCount: 2, + ChannelMemberTimezonesCount: 2, + }, + } + require.ElementsMatch(t, expectedMemberCounts, memberCounts) + }) +} diff --git a/i18n/en.json b/i18n/en.json index 3c730c2a57..d3e7372448 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -243,6 +243,10 @@ "id": "api.channel.change_channel_privacy.public_to_private", "translation": "This channel has been converted to a Private Channel." }, + { + "id": "api.channel.channel_member_counts_by_group.license.error", + "translation": "Your license does not support groups" + }, { "id": "api.channel.convert_channel_to_private.default_channel_error", "translation": "This default channel cannot be converted into a private channel." diff --git a/model/channel.go b/model/channel.go index 5dd5e5a0e3..c67e730ee8 100644 --- a/model/channel.go +++ b/model/channel.go @@ -128,6 +128,12 @@ type ChannelSearchOpts struct { PerPage *int } +type ChannelMemberCountByGroup struct { + GroupId string `db:"-" json:"group_id"` + ChannelMemberCount int64 `db:"-" json:"channel_member_count"` + ChannelMemberTimezonesCount int64 `db:"-" json:"channel_member_timezones_count"` +} + func (o *Channel) DeepCopy() *Channel { copy := *o if copy.SchemeId != nil { @@ -181,6 +187,12 @@ func ChannelModerationsPatchFromJson(data io.Reader) []*ChannelModerationPatch { return o } +func ChannelMemberCountsByGroupFromJson(data io.Reader) []*ChannelMemberCountByGroup { + var o []*ChannelMemberCountByGroup + json.NewDecoder(data).Decode(&o) + return o +} + func (o *Channel) Etag() string { return Etag(o.Id, o.UpdateAt) } diff --git a/model/client4.go b/model/client4.go index b4ad6079d6..3fbae16568 100644 --- a/model/client4.go +++ b/model/client4.go @@ -5004,3 +5004,12 @@ func (c *Client4) PatchChannelModerations(channelID string, patch []*ChannelMode defer closeBody(r) return ChannelModerationsFromJson(r.Body), BuildResponse(r) } + +func (c *Client4) GetChannelMemberCountsByGroup(channelID string, includeTimezones bool, etag string) ([]*ChannelMemberCountByGroup, *Response) { + r, err := c.DoApiGet(c.GetChannelRoute(channelID)+"/member_counts_by_group?include_timezones="+strconv.FormatBool(includeTimezones), etag) + if err != nil { + return nil, BuildErrorResponse(r, err) + } + defer closeBody(r) + return ChannelMemberCountsByGroupFromJson(r.Body), BuildResponse(r) +} diff --git a/store/opentracing_layer.go b/store/opentracing_layer.go index 38f6fcf14c..5f292e06d4 100644 --- a/store/opentracing_layer.go +++ b/store/opentracing_layer.go @@ -1111,6 +1111,24 @@ func (s *OpenTracingLayerChannelStore) GetMemberCountFromCache(channelId string) return resultVar0 } +func (s *OpenTracingLayerChannelStore) GetMemberCountsByGroup(channelID string, includeTimezones bool) ([]*model.ChannelMemberCountByGroup, *model.AppError) { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.GetMemberCountsByGroup") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + resultVar0, resultVar1 := s.ChannelStore.GetMemberCountsByGroup(channelID, includeTimezones) + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + func (s *OpenTracingLayerChannelStore) GetMemberForPost(postId string, userId string) (*model.ChannelMember, *model.AppError) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.GetMemberForPost") diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index 47a1a18065..849066c0f3 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1779,6 +1779,38 @@ func (s SqlChannelStore) GetMemberCount(channelId string, allowFromCache bool) ( return count, nil } +// GetMemberCountsByGroup returns a slice of ChannelMemberCountByGroup for a given channel +// which contains the number of channel members for each group and optionally the number of unique timezones present for each group in the channel +func (s SqlChannelStore) GetMemberCountsByGroup(channelID string, includeTimezones bool) ([]*model.ChannelMemberCountByGroup, *model.AppError) { + selectStr := "GroupMembers.GroupId, COUNT(ChannelMembers.UserId) AS ChannelMemberCount" + + if includeTimezones { + selectStr = "GroupMembers.GroupId, COUNT(ChannelMembers.UserId) AS ChannelMemberCount, COUNT( DISTINCT Users.Timezone ) AS ChannelMemberTimezonesCount" + } + + query := s.getQueryBuilder(). + Select(selectStr). + From("ChannelMembers"). + Join("GroupMembers ON GroupMembers.UserId = ChannelMembers.UserId") + + if includeTimezones { + query = query.Join("Users ON Users.Id = GroupMembers.UserId") + } + + query = query.Where(sq.Eq{"ChannelMembers.ChannelId": channelID}).GroupBy("GroupMembers.GroupId") + + queryString, args, err := query.ToSql() + if err != nil { + return nil, model.NewAppError("SqlChannelStore.GetMemberCountsByGroup", "store.sql.build_query.app_error", nil, err.Error(), http.StatusInternalServerError) + } + var data []*model.ChannelMemberCountByGroup + if _, err = s.GetReplica().Select(&data, queryString, args...); err != nil { + return nil, model.NewAppError("SqlChannelStore.GetMemberCountsByGroup", "store.sql_channel.get_member_count.app_error", nil, "channel_id="+channelID+", "+err.Error(), http.StatusInternalServerError) + } + + return data, nil +} + func (s SqlChannelStore) InvalidatePinnedPostCount(channelId string) { } diff --git a/store/store.go b/store/store.go index fb71f3df88..111513e34a 100644 --- a/store/store.go +++ b/store/store.go @@ -170,6 +170,7 @@ type ChannelStore interface { InvalidateMemberCount(channelId string) GetMemberCountFromCache(channelId string) int64 GetMemberCount(channelId string, allowFromCache bool) (int64, *model.AppError) + GetMemberCountsByGroup(channelID string, includeTimezones bool) ([]*model.ChannelMemberCountByGroup, *model.AppError) InvalidatePinnedPostCount(channelId string) GetPinnedPostCount(channelId string, allowFromCache bool) (int64, *model.AppError) InvalidateGuestCount(channelId string) diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index 0e6a6efcda..2db7997181 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -15,6 +15,7 @@ import ( "github.com/mattermost/gorp" "github.com/mattermost/mattermost-server/v5/model" + "github.com/mattermost/mattermost-server/v5/services/timezones" "github.com/mattermost/mattermost-server/v5/store" ) @@ -70,6 +71,7 @@ func TestChannelStore(t *testing.T, ss store.Store, s SqlSupplier) { t.Run("GetMember", func(t *testing.T) { testGetMember(t, ss) }) t.Run("GetMemberForPost", func(t *testing.T) { testChannelStoreGetMemberForPost(t, ss) }) t.Run("GetMemberCount", func(t *testing.T) { testGetMemberCount(t, ss) }) + t.Run("GetMemberCountsByGroup", func(t *testing.T) { testGetMemberCountsByGroup(t, ss) }) t.Run("GetGuestCount", func(t *testing.T) { testGetGuestCount(t, ss) }) t.Run("SearchMore", func(t *testing.T) { testChannelStoreSearchMore(t, ss) }) t.Run("SearchInTeam", func(t *testing.T) { testChannelStoreSearchInTeam(t, ss) }) @@ -4267,6 +4269,207 @@ func testGetMemberCount(t *testing.T, ss store.Store) { require.EqualValuesf(t, 2, count, "got incorrect member count %v", count) } +func testGetMemberCountsByGroup(t *testing.T, ss store.Store) { + var memberCounts []*model.ChannelMemberCountByGroup + teamId := model.NewId() + g1 := &model.Group{ + Name: model.NewId(), + DisplayName: model.NewId(), + Source: model.GroupSourceLdap, + RemoteId: model.NewId(), + } + _, err := ss.Group().Create(g1) + require.Nil(t, err) + + c1 := model.Channel{ + TeamId: teamId, + DisplayName: "Channel1", + Name: "zz" + model.NewId() + "b", + Type: model.CHANNEL_OPEN, + } + _, err = ss.Channel().Save(&c1, -1) + require.Nil(t, err) + + u1 := &model.User{ + Timezone: timezones.DefaultUserTimezone(), + Email: MakeEmail(), + DeleteAt: 0, + } + _, err = ss.User().Save(u1) + require.Nil(t, err) + _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u1.Id}, -1) + require.Nil(t, err) + + m1 := model.ChannelMember{ + ChannelId: c1.Id, + UserId: u1.Id, + NotifyProps: model.GetDefaultChannelNotifyProps(), + } + _, err = ss.Channel().SaveMember(&m1) + require.Nil(t, err) + + t.Run("empty slice for channel with no groups", func(t *testing.T) { + memberCounts, err = ss.Channel().GetMemberCountsByGroup(c1.Id, false) + expectedMemberCounts := []*model.ChannelMemberCountByGroup{} + require.Nil(t, err) + require.Equal(t, expectedMemberCounts, memberCounts) + }) + + _, err = ss.Group().UpsertMember(g1.Id, u1.Id) + require.Nil(t, err) + + t.Run("returns memberCountsByGroup without timezones", func(t *testing.T) { + memberCounts, err = ss.Channel().GetMemberCountsByGroup(c1.Id, false) + expectedMemberCounts := []*model.ChannelMemberCountByGroup{ + { + GroupId: g1.Id, + ChannelMemberCount: 1, + ChannelMemberTimezonesCount: 0, + }, + } + require.Nil(t, err) + require.Equal(t, expectedMemberCounts, memberCounts) + }) + + t.Run("returns memberCountsByGroup with timezones", func(t *testing.T) { + memberCounts, err = ss.Channel().GetMemberCountsByGroup(c1.Id, true) + expectedMemberCounts := []*model.ChannelMemberCountByGroup{ + { + GroupId: g1.Id, + ChannelMemberCount: 1, + ChannelMemberTimezonesCount: 1, + }, + } + require.Nil(t, err) + require.Equal(t, expectedMemberCounts, memberCounts) + }) + + g2 := &model.Group{ + Name: model.NewId(), + DisplayName: model.NewId(), + Source: model.GroupSourceLdap, + RemoteId: model.NewId(), + } + _, err = ss.Group().Create(g2) + require.Nil(t, err) + + // create 5 different users with 2 different timezones for group 2 + for i := 1; i <= 5; i++ { + timeZone := timezones.DefaultUserTimezone() + if i == 1 { + timeZone["manualTimezone"] = "EDT" + } + + u := &model.User{ + Timezone: timeZone, + Email: MakeEmail(), + DeleteAt: 0, + } + _, err = ss.User().Save(u) + require.Nil(t, err) + _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u.Id}, -1) + require.Nil(t, err) + + m := model.ChannelMember{ + ChannelId: c1.Id, + UserId: u.Id, + NotifyProps: model.GetDefaultChannelNotifyProps(), + } + _, err = ss.Channel().SaveMember(&m) + require.Nil(t, err) + + _, err = ss.Group().UpsertMember(g2.Id, u.Id) + require.Nil(t, err) + } + + g3 := &model.Group{ + Name: model.NewId(), + DisplayName: model.NewId(), + Source: model.GroupSourceLdap, + RemoteId: model.NewId(), + } + + _, err = ss.Group().Create(g3) + require.Nil(t, err) + + // create 10 different users with 3 different timezones for group 3 + for i := 1; i <= 10; i++ { + timeZone := timezones.DefaultUserTimezone() + if i == 1 { + timeZone["manualTimezone"] = "EDT" + } else if i == 2 { + timeZone["manualTimezone"] = "PST" + } + + u := &model.User{ + Timezone: timeZone, + Email: MakeEmail(), + DeleteAt: 0, + } + _, err = ss.User().Save(u) + require.Nil(t, err) + _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u.Id}, -1) + require.Nil(t, err) + + m := model.ChannelMember{ + ChannelId: c1.Id, + UserId: u.Id, + NotifyProps: model.GetDefaultChannelNotifyProps(), + } + _, err = ss.Channel().SaveMember(&m) + require.Nil(t, err) + + _, err = ss.Group().UpsertMember(g3.Id, u.Id) + require.Nil(t, err) + } + + t.Run("returns memberCountsByGroup for multiple groups with lots of users without timezones", func(t *testing.T) { + memberCounts, err = ss.Channel().GetMemberCountsByGroup(c1.Id, false) + expectedMemberCounts := []*model.ChannelMemberCountByGroup{ + { + GroupId: g1.Id, + ChannelMemberCount: 1, + ChannelMemberTimezonesCount: 0, + }, + { + GroupId: g2.Id, + ChannelMemberCount: 5, + ChannelMemberTimezonesCount: 0, + }, + { + GroupId: g3.Id, + ChannelMemberCount: 10, + ChannelMemberTimezonesCount: 0, + }, + } + require.Nil(t, err) + require.ElementsMatch(t, expectedMemberCounts, memberCounts) + }) + + t.Run("returns memberCountsByGroup for multiple groups with lots of users with timezones", func(t *testing.T) { + memberCounts, err = ss.Channel().GetMemberCountsByGroup(c1.Id, true) + expectedMemberCounts := []*model.ChannelMemberCountByGroup{ + { + GroupId: g1.Id, + ChannelMemberCount: 1, + ChannelMemberTimezonesCount: 1, + }, + { + GroupId: g2.Id, + ChannelMemberCount: 5, + ChannelMemberTimezonesCount: 2, + }, + { + GroupId: g3.Id, + ChannelMemberCount: 10, + ChannelMemberTimezonesCount: 3, + }, + } + require.Nil(t, err) + require.ElementsMatch(t, expectedMemberCounts, memberCounts) + }) +} + func testGetGuestCount(t *testing.T, ss store.Store) { teamId := model.NewId() diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index af6995b6d7..ae596400c5 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -854,6 +854,31 @@ func (_m *ChannelStore) GetMemberCountFromCache(channelId string) int64 { return r0 } +// GetMemberCountsByGroup provides a mock function with given fields: channelID, includeTimezones +func (_m *ChannelStore) GetMemberCountsByGroup(channelID string, includeTimezones bool) ([]*model.ChannelMemberCountByGroup, *model.AppError) { + ret := _m.Called(channelID, includeTimezones) + + var r0 []*model.ChannelMemberCountByGroup + if rf, ok := ret.Get(0).(func(string, bool) []*model.ChannelMemberCountByGroup); ok { + r0 = rf(channelID, includeTimezones) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*model.ChannelMemberCountByGroup) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, bool) *model.AppError); ok { + r1 = rf(channelID, includeTimezones) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // GetMemberForPost provides a mock function with given fields: postId, userId func (_m *ChannelStore) GetMemberForPost(postId string, userId string) (*model.ChannelMember, *model.AppError) { ret := _m.Called(postId, userId) diff --git a/store/timer_layer.go b/store/timer_layer.go index 7682f0f610..47548e6680 100644 --- a/store/timer_layer.go +++ b/store/timer_layer.go @@ -1032,6 +1032,22 @@ func (s *TimerLayerChannelStore) GetMemberCountFromCache(channelId string) int64 return resultVar0 } +func (s *TimerLayerChannelStore) GetMemberCountsByGroup(channelID string, includeTimezones bool) ([]*model.ChannelMemberCountByGroup, *model.AppError) { + start := timemodule.Now() + + resultVar0, resultVar1 := s.ChannelStore.GetMemberCountsByGroup(channelID, includeTimezones) + + elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) + if s.Root.Metrics != nil { + success := "false" + if resultVar1 == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("ChannelStore.GetMemberCountsByGroup", success, elapsed) + } + return resultVar0, resultVar1 +} + func (s *TimerLayerChannelStore) GetMemberForPost(postId string, userId string) (*model.ChannelMember, *model.AppError) { start := timemodule.Now()