From 56cf4b4ee75376c01c9680d1a38d47f402661669 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Tue, 21 Jun 2022 15:12:13 +0530 Subject: [PATCH 01/27] Clean license, and guest user check for top threads --- api4/insights.go | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/api4/insights.go b/api4/insights.go index 39493a97f3..817dec6ce0 100644 --- a/api4/insights.go +++ b/api4/insights.go @@ -21,8 +21,8 @@ func (api *API) InitInsights() { api.BaseRoutes.InsightsForUser.Handle("/channels", api.APISessionRequired(minimumProfessionalLicense(rejectGuests(getTopChannelsForUserSince)))).Methods("GET") // Threads - api.BaseRoutes.InsightsForTeam.Handle("/threads", api.APISessionRequired(requireLicense(getTopThreadsForTeamSince))).Methods("GET") - api.BaseRoutes.InsightsForUser.Handle("/threads", api.APISessionRequired(requireLicense(getTopThreadsForUserSince))).Methods("GET") + api.BaseRoutes.InsightsForTeam.Handle("/threads", api.APISessionRequired(minimumProfessionalLicense(rejectGuests(getTopThreadsForTeamSince)))).Methods("GET") + api.BaseRoutes.InsightsForUser.Handle("/threads", api.APISessionRequired(minimumProfessionalLicense(rejectGuests(getTopThreadsForUserSince)))).Methods("GET") } // Top Reactions @@ -243,21 +243,14 @@ func getTopThreadsForTeamSince(c *Context, w http.ResponseWriter, r *http.Reques return } - // license check - lic := c.App.Srv().License() - if lic.SkuShortName != model.LicenseShortSkuProfessional && lic.SkuShortName != model.LicenseShortSkuEnterprise { - c.Err = model.NewAppError("", "api.insights.license_error", nil, "", http.StatusNotImplemented) - return - } - - // restrict guests and users with no access to team + // restrict users with no access to team user, err := c.App.GetUser(c.AppContext.Session().UserId) if err != nil { c.Err = err return } - if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), team.Id, model.PermissionViewTeam) || user.IsGuest() { + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), team.Id, model.PermissionViewTeam) { c.SetPermissionError(model.PermissionViewTeam) return } @@ -286,7 +279,7 @@ func getTopThreadsForTeamSince(c *Context, w http.ResponseWriter, r *http.Reques func getTopThreadsForUserSince(c *Context, w http.ResponseWriter, r *http.Request) { c.Params.TeamId = r.URL.Query().Get("team_id") - // restrict guests and users with no access to team + // restrict users with no access to team user, err := c.App.GetUser(c.AppContext.Session().UserId) if err != nil { c.Err = err @@ -305,14 +298,7 @@ func getTopThreadsForUserSince(c *Context, w http.ResponseWriter, r *http.Reques return } - // license check - lic := c.App.Srv().License() - if lic.SkuShortName != model.LicenseShortSkuProfessional && lic.SkuShortName != model.LicenseShortSkuEnterprise { - c.Err = model.NewAppError("", "api.insights.license_error", nil, "", http.StatusNotImplemented) - return - } - - if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), team.Id, model.PermissionViewTeam) || user.IsGuest() { + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), team.Id, model.PermissionViewTeam) { c.SetPermissionError(model.PermissionViewTeam) return } From 0ab21899414cb3959901208905169b8803319f72 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Wed, 22 Jun 2022 16:11:40 +0530 Subject: [PATCH 02/27] Add top DMs route and handlers --- api4/insights.go | 33 ++++++++++++++ app/app_iface.go | 1 + app/opentracing/opentracing_layer.go | 22 +++++++++ app/post.go | 11 +++++ model/insights.go | 26 +++++++++++ store/opentracinglayer/opentracinglayer.go | 18 ++++++++ store/retrylayer/retrylayer.go | 21 +++++++++ store/sqlstore/post_store.go | 52 ++++++++++++++++++++++ store/store.go | 3 ++ store/storetest/mocks/PostStore.go | 23 ++++++++++ store/timerlayer/timerlayer.go | 16 +++++++ 11 files changed, 226 insertions(+) diff --git a/api4/insights.go b/api4/insights.go index 817dec6ce0..db2bda4e36 100644 --- a/api4/insights.go +++ b/api4/insights.go @@ -23,6 +23,9 @@ func (api *API) InitInsights() { // Threads api.BaseRoutes.InsightsForTeam.Handle("/threads", api.APISessionRequired(minimumProfessionalLicense(rejectGuests(getTopThreadsForTeamSince)))).Methods("GET") api.BaseRoutes.InsightsForUser.Handle("/threads", api.APISessionRequired(minimumProfessionalLicense(rejectGuests(getTopThreadsForUserSince)))).Methods("GET") + + // user DMs + api.BaseRoutes.InsightsForUser.Handle("/dms", api.APISessionRequired(minimumProfessionalLicense(rejectGuests(getTopDMsForUserSince)))).Methods("GET") } // Top Reactions @@ -326,6 +329,36 @@ func getTopThreadsForUserSince(c *Context, w http.ResponseWriter, r *http.Reques w.Write(js) } +// Top DMs +func getTopDMsForUserSince(c *Context, w http.ResponseWriter, r *http.Request) { + user, err := c.App.GetUser(c.AppContext.Session().UserId) + if err != nil { + c.Err = err + return + } + + startTime := model.StartOfDayForTimeRange(c.Params.TimeRange, user.GetTimezoneLocation()) + + topDMs, err := c.App.GetTopDMsForUserSince(c.AppContext.Session().UserId, &model.InsightsOpts{ + StartUnixMilli: startTime.UnixMilli(), + Page: c.Params.Page, + PerPage: c.Params.PerPage, + }) + + if err != nil { + c.Err = err + return + } + + js, jsonErr := json.Marshal(topDMs) + if jsonErr != nil { + c.Err = model.NewAppError("getTopDMsForUserSince", "api.marshal_error", nil, jsonErr.Error(), http.StatusInternalServerError) + return + } + + w.Write(js) +} + // postCountByDurationViewModel expects a list of channels that are pre-authorized for the given user to view. func postCountByDurationViewModel(c *Context, topChannelList *model.TopChannelList, startTime *time.Time, timeRange string, userID *string, location *time.Location) (model.ChannelPostCountByDuration, *model.AppError) { if len(topChannelList.Items) == 0 { diff --git a/app/app_iface.go b/app/app_iface.go index e1e0aeb057..031ba54c36 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -791,6 +791,7 @@ type AppIface interface { GetTokenById(token string) (*model.Token, *model.AppError) GetTopChannelsForTeamSince(c request.CTX, teamID, userID string, opts *model.InsightsOpts) (*model.TopChannelList, *model.AppError) GetTopChannelsForUserSince(c request.CTX, userID, teamID string, opts *model.InsightsOpts) (*model.TopChannelList, *model.AppError) + GetTopDMsForUserSince(userID string, opts *model.InsightsOpts) (*model.TopDMList, *model.AppError) GetTopReactionsForTeamSince(teamID string, userID string, opts *model.InsightsOpts) (*model.TopReactionList, *model.AppError) GetTopReactionsForUserSince(userID string, teamID string, opts *model.InsightsOpts) (*model.TopReactionList, *model.AppError) GetTopThreadsForTeamSince(c request.CTX, teamID, userID string, opts *model.InsightsOpts) (*model.TopThreadList, *model.AppError) diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index d31592efe5..c004dde2e2 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -9904,6 +9904,28 @@ func (a *OpenTracingAppLayer) GetTopChannelsForUserSince(c request.CTX, userID s return resultVar0, resultVar1 } +func (a *OpenTracingAppLayer) GetTopDMsForUserSince(userID string, opts *model.InsightsOpts) (*model.TopDMList, *model.AppError) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetTopDMsForUserSince") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0, resultVar1 := a.app.GetTopDMsForUserSince(userID, opts) + + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + func (a *OpenTracingAppLayer) GetTopReactionsForTeamSince(teamID string, userID string, opts *model.InsightsOpts) (*model.TopReactionList, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetTopReactionsForTeamSince") diff --git a/app/post.go b/app/post.go index 2379ee300f..1905ad95aa 100644 --- a/app/post.go +++ b/app/post.go @@ -1932,6 +1932,17 @@ func (a *App) GetTopThreadsForUserSince(c request.CTX, teamID, userID string, op return topThreadsWithEmbedAndImage, nil } +func (a *App) GetTopDMsForUserSince(userID string, opts *model.InsightsOpts) (*model.TopDMList, *model.AppError) { + if !a.Config().FeatureFlags.InsightsEnabled { + return nil, model.NewAppError("GetTopDMsForUserSince", "app.insights.feature_disabled", nil, "", http.StatusNotImplemented) + } + topDMs, err := a.Srv().Store.Post().GetTopDMsForUserSince(userID, opts.StartUnixMilli, opts.Page*opts.PerPage, opts.PerPage) + if err != nil { + return nil, model.NewAppError("GetTopDMsForUserSince", "app.post.get_top_dms_for_user_since.app_error", nil, err.Error(), http.StatusInternalServerError) + } + return topDMs, nil +} + func includeEmbedsAndImages(a *App, c request.CTX, topThreadList *model.TopThreadList, userID string) (*model.TopThreadList, error) { for _, topThread := range topThreadList.Items { topThread.Post = a.PreparePostForClientWithEmbedsAndImages(c, topThread.Post, false, false) diff --git a/model/insights.go b/model/insights.go index dbd1720bc7..76667e8916 100644 --- a/model/insights.go +++ b/model/insights.go @@ -97,6 +97,18 @@ type DurationPostCount struct { PostCount int `db:"postcount"` } +// Top DMs +type TopDM struct { + MessageCount int64 `json:"post_count"` + Participants string `json:"-"` + SecondParticipant string `json:"second_participant"` +} + +type TopDMList struct { + InsightsListData + Items []*TopDM `json:"items"` +} + func TimeRangeToNumberDays(timeRange string) int { var n int switch timeRange { @@ -243,3 +255,17 @@ func GetTopThreadListWithPagination(threads []*TopThread, limit int) *TopThreadL return &TopThreadList{InsightsListData: InsightsListData{HasNext: hasNext}, Items: threads} } + +// GetTopDMListWithPagination adds a rank to each item in the given list of TopDM and checks if there is +// another page that can be fetched based on the given limit and offset. The given list of TopDM is assumed to be +// sorted by MessageCount(score). Returns a TopDMList. +func GetTopDMListWithPagination(dms []*TopDM, limit int) *TopDMList { + // Add pagination support + var hasNext bool + if (limit != 0) && (len(dms) == limit+1) { + hasNext = true + dms = dms[:len(dms)-1] + } + + return &TopDMList{InsightsListData: InsightsListData{HasNext: hasNext}, Items: dms} +} diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 85c4829f58..bb04863f55 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -6007,6 +6007,24 @@ func (s *OpenTracingLayerPostStore) GetSingle(id string, inclDeleted bool) (*mod return result, err } +func (s *OpenTracingLayerPostStore) GetTopDMsForUserSince(userID string, since int64, offset int, limit int) (*model.TopDMList, error) { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PostStore.GetTopDMsForUserSince") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + result, err := s.PostStore.GetTopDMsForUserSince(userID, since, offset, limit) + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return result, err +} + func (s *OpenTracingLayerPostStore) HasAutoResponsePostByUserSince(options model.GetPostsSinceOptions, userId string) (bool, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PostStore.HasAutoResponsePostByUserSince") diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index 5094c41fcb..56c7345078 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -6806,6 +6806,27 @@ func (s *RetryLayerPostStore) GetSingle(id string, inclDeleted bool) (*model.Pos } +func (s *RetryLayerPostStore) GetTopDMsForUserSince(userID string, since int64, offset int, limit int) (*model.TopDMList, error) { + + tries := 0 + for { + result, err := s.PostStore.GetTopDMsForUserSince(userID, since, offset, limit) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerPostStore) HasAutoResponsePostByUserSince(options model.GetPostsSinceOptions, userId string) (bool, error) { tries := 0 diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index af7b11ca01..f65986c1d5 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -2963,3 +2963,55 @@ func (s *SqlPostStore) updateThreadsFromPosts(transaction *sqlxTxWrapper, posts } return nil } + +func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset int, limit int) (*model.TopDMList, error) { + channelSelector := s.getQueryBuilder().Select("Id", "TotalMsgCount").From("Channels").Join("ChannelMembers as cm on cm.ChannelId = Channels.Id"). + Where(sq.And{ + sq.Expr("Channels.Type = 'D'"), + sq.Eq{"cm.UserId": userID}, + }) + var aggregator string + + if s.DriverName() == model.DatabaseDriverMysql { + aggregator = "group_concat(distinct cm.UserId) as Participants" + } else { + aggregator = "string_agg(distinct cm.UserId, ',') as Participants" + } + + topDMsBuilder := s.getQueryBuilder().Select("count(p.id) as MessageCount", aggregator).FromSelect(channelSelector, "vch"). + Join("ChannelMembers as cm on cm.ChannelId = vch.Id"). + Join("Posts as p on p.ChannelId = vch.Id"). + Where(sq.Gt{ + "p.UpdateAt": since, + }).GroupBy("vch.id"). + Limit(uint64(limit)). + Offset(uint64(offset)) + + topDMsBuilder = topDMsBuilder.OrderBy("MessageCount DESC").Limit(uint64(limit)).Offset(uint64(offset)) + + topDMs := make([]*model.TopDM, 0) + sql, args, err := topDMsBuilder.ToSql() + if err != nil { + return nil, errors.Wrap(err, "GetTopDMsForUserSince_ToSql") + } + err = s.GetReplicaX().Select(&topDMs, sql, args...) + if err != nil { + return nil, errors.Wrapf(err, "failed to find top DMs for user-id: %s", userID) + } + + // fill SecondParticipant column + topDMs = postProcessTopDMs(userID, topDMs) + return model.GetTopDMListWithPagination(topDMs, limit), nil +} + +func postProcessTopDMs(userID string, topDMs []*model.TopDM) []*model.TopDM { + for _, topDM := range topDMs { + participants := strings.Split(topDM.Participants, ",") + if participants[0] == userID { + topDM.SecondParticipant = participants[1] + } else { + topDM.SecondParticipant = participants[0] + } + } + return topDMs +} diff --git a/store/store.go b/store/store.go index 370dad74dc..fbd2ed3f1a 100644 --- a/store/store.go +++ b/store/store.go @@ -388,6 +388,9 @@ type PostStore interface { GetPostsSinceForSync(options model.GetPostsSinceForSyncOptions, cursor model.GetPostsSinceForSyncCursor, limit int) ([]*model.Post, model.GetPostsSinceForSyncCursor, error) // GetNthRecentPostTime returns the CreateAt time of the nth most recent post. GetNthRecentPostTime(n int64) (int64, error) + + // Insights - top DMs + GetTopDMsForUserSince(userID string, since int64, offset int, limit int) (*model.TopDMList, error) } type UserStore interface { diff --git a/store/storetest/mocks/PostStore.go b/store/storetest/mocks/PostStore.go index 0f24d48db0..bc430ad0f0 100644 --- a/store/storetest/mocks/PostStore.go +++ b/store/storetest/mocks/PostStore.go @@ -700,6 +700,29 @@ func (_m *PostStore) GetSingle(id string, inclDeleted bool) (*model.Post, error) return r0, r1 } +// GetTopDMsForUserSince provides a mock function with given fields: userID, since, offset, limit +func (_m *PostStore) GetTopDMsForUserSince(userID string, since int64, offset int, limit int) (*model.TopDMList, error) { + ret := _m.Called(userID, since, offset, limit) + + var r0 *model.TopDMList + if rf, ok := ret.Get(0).(func(string, int64, int, int) *model.TopDMList); ok { + r0 = rf(userID, since, offset, limit) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.TopDMList) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, int64, int, int) error); ok { + r1 = rf(userID, since, offset, limit) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // HasAutoResponsePostByUserSince provides a mock function with given fields: options, userId func (_m *PostStore) HasAutoResponsePostByUserSince(options model.GetPostsSinceOptions, userId string) (bool, error) { ret := _m.Called(options, userId) diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index eb920da051..16d0dbf0d1 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -5434,6 +5434,22 @@ func (s *TimerLayerPostStore) GetSingle(id string, inclDeleted bool) (*model.Pos return result, err } +func (s *TimerLayerPostStore) GetTopDMsForUserSince(userID string, since int64, offset int, limit int) (*model.TopDMList, error) { + start := time.Now() + + result, err := s.PostStore.GetTopDMsForUserSince(userID, since, offset, limit) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("PostStore.GetTopDMsForUserSince", success, elapsed) + } + return result, err +} + func (s *TimerLayerPostStore) HasAutoResponsePostByUserSince(options model.GetPostsSinceOptions, userId string) (bool, error) { start := time.Now() From a8fa09f946b037b588e96260dd6922a38e7a3c1d Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 27 Jun 2022 14:03:25 +0530 Subject: [PATCH 03/27] Fix counting of posts, remove redundant limit offset for queries --- store/sqlstore/post_store.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index f65986c1d5..16e5394147 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -2983,9 +2983,7 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset Join("Posts as p on p.ChannelId = vch.Id"). Where(sq.Gt{ "p.UpdateAt": since, - }).GroupBy("vch.id"). - Limit(uint64(limit)). - Offset(uint64(offset)) + }).GroupBy("vch.id") topDMsBuilder = topDMsBuilder.OrderBy("MessageCount DESC").Limit(uint64(limit)).Offset(uint64(offset)) @@ -3006,7 +3004,14 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset func postProcessTopDMs(userID string, topDMs []*model.TopDM) []*model.TopDM { for _, topDM := range topDMs { + // divide message count by 2, because it's counted twice due to channel memberships being 2 for dms. + topDM.MessageCount = topDM.MessageCount / 2 participants := strings.Split(topDM.Participants, ",") + if len(participants) == 1 { + // chatting to self + topDM.SecondParticipant = userID + continue + } if participants[0] == userID { topDM.SecondParticipant = participants[1] } else { From 2b66652f694cf5ee58e3a0e4b7c8fcfaedaa2016 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Tue, 28 Jun 2022 12:22:25 +0530 Subject: [PATCH 04/27] Fix top dms query, add storetests --- store/sqlstore/post_store.go | 14 +++-- store/storetest/post_store.go | 113 ++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 4 deletions(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 16e5394147..ab96f31fd1 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -2981,8 +2981,13 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset topDMsBuilder := s.getQueryBuilder().Select("count(p.id) as MessageCount", aggregator).FromSelect(channelSelector, "vch"). Join("ChannelMembers as cm on cm.ChannelId = vch.Id"). Join("Posts as p on p.ChannelId = vch.Id"). - Where(sq.Gt{ - "p.UpdateAt": since, + Where(sq.And{ + sq.Gt{ + "p.UpdateAt": since, + }, + sq.Eq{ + "p.DeleteAt": 0, + }, }).GroupBy("vch.id") topDMsBuilder = topDMsBuilder.OrderBy("MessageCount DESC").Limit(uint64(limit)).Offset(uint64(offset)) @@ -3004,13 +3009,14 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset func postProcessTopDMs(userID string, topDMs []*model.TopDM) []*model.TopDM { for _, topDM := range topDMs { - // divide message count by 2, because it's counted twice due to channel memberships being 2 for dms. - topDM.MessageCount = topDM.MessageCount / 2 participants := strings.Split(topDM.Participants, ",") if len(participants) == 1 { // chatting to self topDM.SecondParticipant = userID continue + } else { + // divide message count by 2, because it's counted twice due to channel memberships being 2 for dms. + topDM.MessageCount = topDM.MessageCount / 2 } if participants[0] == userID { topDM.SecondParticipant = participants[1] diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 15147cbfb6..33984b58a4 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -58,6 +58,7 @@ func TestPostStore(t *testing.T, ss store.Store, s SqlStore) { t.Run("HasAutoResponsePostByUserSince", func(t *testing.T) { testHasAutoResponsePostByUserSince(t, ss) }) t.Run("GetPostsSinceForSync", func(t *testing.T) { testGetPostsSinceForSync(t, ss, s) }) t.Run("GetNthRecentPostTime", func(t *testing.T) { testGetNthRecentPostTime(t, ss) }) + t.Run("GetTopDMsForUserSince", func(t *testing.T) { testGetTopDMsForUserSince(t, ss, s) }) } func testPostStoreSave(t *testing.T, ss store.Store) { @@ -3861,3 +3862,115 @@ func testGetNthRecentPostTime(t *testing.T, ss store.Store) { assert.Error(t, err) assert.IsType(t, &store.ErrNotFound{}, err) } + +func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { + // users + user := model.User{Id: model.NewId()} + u1 := model.User{Id: model.NewId()} + u2 := model.User{Id: model.NewId()} + u3 := model.User{Id: model.NewId()} + u4 := model.User{Id: model.NewId()} + // user direct messages + chUser1, nErr := ss.Channel().CreateDirectChannel(&u1, &user) + require.NoError(t, nErr) + chUser2, nErr := ss.Channel().CreateDirectChannel(&u2, &user) + require.NoError(t, nErr) + chUser3, nErr := ss.Channel().CreateDirectChannel(&u3, &user) + require.NoError(t, nErr) + // other user direct message + chUser3User4, nErr := ss.Channel().CreateDirectChannel(&u3, &u4) + require.NoError(t, nErr) + + // sample post data + // for u1 + _, err := ss.Post().Save(&model.Post{ + ChannelId: chUser1.Id, + UserId: u1.Id, + }) + require.NoError(t, err) + _, err = ss.Post().Save(&model.Post{ + ChannelId: chUser1.Id, + UserId: user.Id, + }) + require.NoError(t, err) + // for u2: 1 post + postToDelete, err := ss.Post().Save(&model.Post{ + ChannelId: chUser2.Id, + UserId: u2.Id, + }) + require.NoError(t, err) + // for user-u3: 3 posts + for i := 0; i < 3; i++ { + _, err = ss.Post().Save(&model.Post{ + ChannelId: chUser3.Id, + UserId: user.Id, + }) + require.NoError(t, err) + } + // for u4-u3: 4 posts + _, err = ss.Post().Save(&model.Post{ + ChannelId: chUser3User4.Id, + UserId: u3.Id, + }) + require.NoError(t, err) + _, err = ss.Post().Save(&model.Post{ + ChannelId: chUser3User4.Id, + UserId: u4.Id, + }) + require.NoError(t, err) + _, err = ss.Post().Save(&model.Post{ + ChannelId: chUser3User4.Id, + UserId: u3.Id, + }) + require.NoError(t, err) + + _, err = ss.Post().Save(&model.Post{ + ChannelId: chUser3User4.Id, + UserId: u4.Id, + }) + require.NoError(t, err) + t.Run("should return topDMs when userid is specified ", func(t *testing.T) { + topDMs, err := ss.Post().GetTopDMsForUserSince(user.Id, 100, 0, 100) + require.NoError(t, err) + // len of topDMs.Items should be 3 + require.Len(t, topDMs.Items, 3) + // check order, magnitude of items + // fmt.Println(topDMs.Items[0].MessageCount, topDMs.Items[1].MessageCount, topDMs.Items[2].MessageCount) + require.Equal(t, topDMs.Items[0].SecondParticipant, u3.Id) + require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) + require.Equal(t, topDMs.Items[1].SecondParticipant, u1.Id) + require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) + require.Equal(t, topDMs.Items[2].SecondParticipant, u2.Id) + require.Equal(t, topDMs.Items[2].MessageCount, int64(1)) + // this also ensures that u3-u4 conversation doesn't show up in others' top DMs. + }) + t.Run("topDMs should only consider user's DM channels ", func(t *testing.T) { + // u4 only takes part in one conversation + topDMs, err := ss.Post().GetTopDMsForUserSince(u4.Id, 100, 0, 100) + require.NoError(t, err) + // len of topDMs.Items should be 3 + require.Len(t, topDMs.Items, 1) + // check order, magnitude of items + require.Equal(t, topDMs.Items[0].SecondParticipant, u3.Id) + require.Equal(t, topDMs.Items[0].MessageCount, int64(4)) + }) + t.Run("topDMs will consider self dms", func(t *testing.T) { + chUser, nErr := ss.Channel().CreateDirectChannel(&user, &user) + require.NoError(t, nErr) + _, err = ss.Post().Save(&model.Post{ + ChannelId: chUser.Id, + UserId: user.Id, + }) + // delete u2 post + err := ss.Post().Delete(postToDelete.Id, 200, user.Id) + require.NoError(t, err) + // u4 only takes part in one conversation + topDMs, err := ss.Post().GetTopDMsForUserSince(user.Id, 100, 0, 100) + require.NoError(t, err) + // len of topDMs.Items should be 3 + require.Len(t, topDMs.Items, 3) + // check order, magnitude of items + require.Equal(t, topDMs.Items[2].SecondParticipant, user.Id) + require.Equal(t, topDMs.Items[2].MessageCount, int64(1)) + }) +} From 553b436d40c1f9321f25f2961eec310ca001ee13 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Tue, 28 Jun 2022 12:53:19 +0530 Subject: [PATCH 05/27] Test pagination of top DMs --- model/insights_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/model/insights_test.go b/model/insights_test.go index 671e0443fb..c56fb0c980 100644 --- a/model/insights_test.go +++ b/model/insights_test.go @@ -120,3 +120,41 @@ func TestGetTopThreadListWithPagination(t *testing.T) { }) } } + +func TestGetTopDMsListWithPagination(t *testing.T) { + dms := []*TopDM{ + {SecondParticipant: NewId(), MessageCount: 100}, + {SecondParticipant: NewId(), MessageCount: 80}, + {SecondParticipant: NewId(), MessageCount: 90}, + {SecondParticipant: NewId(), MessageCount: 76}, + {SecondParticipant: NewId(), MessageCount: 43}, + {SecondParticipant: NewId(), MessageCount: 2}, + {SecondParticipant: NewId(), MessageCount: 1}, + } + hasNextTT := []struct { + Description string + Limit int + Offset int + Expected *TopDMList + }{ + { + Description: "has one page", + Limit: len(dms), + Offset: 0, + Expected: &TopDMList{InsightsListData: InsightsListData{HasNext: false}, Items: dms}, + }, + { + Description: "has more than one page", + Limit: len(dms) - 1, + Offset: 0, + Expected: &TopDMList{InsightsListData: InsightsListData{HasNext: true}, Items: dms}, + }, + } + + for _, test := range hasNextTT { + t.Run(test.Description, func(t *testing.T) { + actual := GetTopDMListWithPagination(dms, test.Limit) + assert.Equal(t, test.Expected.HasNext, actual.HasNext) + }) + } +} From c7dc0eee2219240fc353892ad0c1e420fb2cd289 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Tue, 28 Jun 2022 13:50:40 +0530 Subject: [PATCH 06/27] Add app tests for top DMs --- app/post_test.go | 102 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/app/post_test.go b/app/post_test.go index bc38d528aa..81f7dcdd11 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -3062,3 +3062,105 @@ func TestGetTopThreadsForUserSince(t *testing.T) { require.Nil(t, appErr) require.Len(t, topUser2ThreadsAfterPrivateReplyDelete.Items, 0) } + +func TestGetTopDMsForUserSince(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.Server.configStore.SetReadOnlyFF(false) + defer th.Server.configStore.SetReadOnlyFF(true) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.FeatureFlags.InsightsEnabled = true }) + + // users + user := th.CreateUser() + u1 := th.CreateUser() + u2 := th.CreateUser() + u3 := th.CreateUser() + u4 := th.CreateUser() + // user direct messages + chUser1, nErr := th.App.createDirectChannel(u1.Id, user.Id) + fmt.Println(chUser1, nErr) + require.Nil(t, nErr) + chUser2, nErr := th.App.createDirectChannel(u2.Id, user.Id) + require.Nil(t, nErr) + chUser3, nErr := th.App.createDirectChannel(u3.Id, user.Id) + require.Nil(t, nErr) + // other user direct message + chUser3User4, nErr := th.App.createDirectChannel(u3.Id, u4.Id) + require.Nil(t, nErr) + + // sample post data + // for u1 + _, err := th.App.CreatePostAsUser(th.Context, &model.Post{ + ChannelId: chUser1.Id, + UserId: u1.Id, + }, "", false) + require.Nil(t, err) + _, err = th.App.CreatePostAsUser(th.Context, &model.Post{ + ChannelId: chUser1.Id, + UserId: user.Id, + }, "", false) + require.Nil(t, err) + // for u2: 1 post + _, err = th.App.CreatePostAsUser(th.Context, &model.Post{ + ChannelId: chUser2.Id, + UserId: u2.Id, + }, "", false) + require.Nil(t, err) + // for user-u3: 3 posts + for i := 0; i < 3; i++ { + _, err = th.App.CreatePostAsUser(th.Context, &model.Post{ + ChannelId: chUser3.Id, + UserId: user.Id, + }, "", false) + require.Nil(t, err) + } + // for u4-u3: 4 posts + _, err = th.App.CreatePostAsUser(th.Context, &model.Post{ + ChannelId: chUser3User4.Id, + UserId: u3.Id, + }, "", false) + require.Nil(t, err) + _, err = th.App.CreatePostAsUser(th.Context, &model.Post{ + ChannelId: chUser3User4.Id, + UserId: u4.Id, + }, "", false) + require.Nil(t, err) + _, err = th.App.CreatePostAsUser(th.Context, &model.Post{ + ChannelId: chUser3User4.Id, + UserId: u3.Id, + }, "", false) + require.Nil(t, err) + + _, err = th.App.CreatePostAsUser(th.Context, &model.Post{ + ChannelId: chUser3User4.Id, + UserId: u4.Id, + }, "", false) + require.Nil(t, err) + + t.Run("should return topDMs when userid is specified ", func(t *testing.T) { + topDMs, err := th.App.GetTopDMsForUserSince(user.Id, &model.InsightsOpts{StartUnixMilli: 100, Page: 0, PerPage: 100}) + require.Nil(t, err) + // len of topDMs.Items should be 3 + require.Len(t, topDMs.Items, 3) + // check order, magnitude of items + // fmt.Println(topDMs.Items[0].MessageCount, topDMs.Items[1].MessageCount, topDMs.Items[2].MessageCount) + require.Equal(t, topDMs.Items[0].SecondParticipant, u3.Id) + require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) + require.Equal(t, topDMs.Items[1].SecondParticipant, u1.Id) + require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) + require.Equal(t, topDMs.Items[2].SecondParticipant, u2.Id) + require.Equal(t, topDMs.Items[2].MessageCount, int64(1)) + // this also ensures that u3-u4 conversation doesn't show up in others' top DMs. + }) + t.Run("topDMs should only consider user's DM channels ", func(t *testing.T) { + // u4 only takes part in one conversation + topDMs, err := th.App.GetTopDMsForUserSince(u4.Id, &model.InsightsOpts{StartUnixMilli: 100, Page: 0, PerPage: 100}) + require.Nil(t, err) + // len of topDMs.Items should be 3 + require.Len(t, topDMs.Items, 1) + // check order, magnitude of items + require.Equal(t, topDMs.Items[0].SecondParticipant, u3.Id) + require.Equal(t, topDMs.Items[0].MessageCount, int64(4)) + }) +} From 0936c891ff99531fd56bf1fa342a22531678788b Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Tue, 28 Jun 2022 17:05:52 +0530 Subject: [PATCH 07/27] Add client, integration tests for top DMs --- api4/insights_test.go | 84 +++++++++++++++++++++++++++++++++++++++++++ model/client4.go | 15 ++++++++ 2 files changed, 99 insertions(+) diff --git a/api4/insights_test.go b/api4/insights_test.go index 38f23367d7..6ea51b00f5 100644 --- a/api4/insights_test.go +++ b/api4/insights_test.go @@ -816,3 +816,87 @@ func TestGetTopThreadsForUserSince(t *testing.T) { require.Nil(t, appErr) require.Len(t, topUser2ThreadsAfterPrivateReplyDelete.Items, 0) } + +func TestGetTopDMsForUserSince(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.ConfigStore.SetReadOnlyFF(false) + defer th.ConfigStore.SetReadOnlyFF(true) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.FeatureFlags.InsightsEnabled = true }) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + // basicuser1 - bu1, basicuser - bu + // create dm channels for bu-bu, bu1-bu1, bu-bu1 + basicUser := th.BasicUser + basicUser1 := th.BasicUser2 + + th.LoginBasic2() + client := th.Client + channelBu1, _, err := client.CreateDirectChannel(basicUser1.Id, basicUser1.Id) + require.Nil(t, err) + + th.LoginBasic() + client = th.Client + channelBu, _, err := client.CreateDirectChannel(basicUser.Id, basicUser.Id) + require.Nil(t, err) + channelBu12, _, err := client.CreateDirectChannel(basicUser.Id, basicUser1.Id) + require.Nil(t, err) + + // create 2 posts in channelBu, 1 in channelBu1, 3 in channelBu12 + postsGenConfig := []map[string]interface{}{ + { + "chId": channelBu.Id, + "postCount": 2, + }, + { + "chId": channelBu1.Id, + "postCount": 1, + }, + { + "chId": channelBu12.Id, + "postCount": 3, + }, + } + + for _, postGen := range postsGenConfig { + postCount := postGen["postCount"].(int) + for i := 0; i < postCount; i++ { + if postGen["chId"] == channelBu1.Id { + th.LoginBasic2() + client = th.Client + userId := basicUser1.Id + post := &model.Post{UserId: userId, ChannelId: postGen["chId"].(string), Message: "zz" + model.NewId() + "a"} + _, _, err = client.CreatePost(post) + require.Nil(t, err) + } else { + th.LoginBasic() + client = th.Client + userId := basicUser.Id + post := &model.Post{UserId: userId, ChannelId: postGen["chId"].(string), Message: "zz" + model.NewId() + "a"} + _, _, err = client.CreatePost(post) + require.Nil(t, err) + } + } + } + + // get top dms for bu + t.Run("get top dms for basic user 1", func(t *testing.T) { + th.LoginBasic() + client = th.Client + topDMs, _, err := client.GetTopDMsForUserSince("today", 0, 100) + require.Nil(t, err) + require.Len(t, topDMs.Items, 2) + require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) + require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) + }) + + // get top dms for bu1 + t.Run("get top dms for basic user 2", func(t *testing.T) { + th.LoginBasic2() + client = th.Client + topDMs, _, err := client.GetTopDMsForUserSince("today", 0, 100) + require.Nil(t, err) + require.Len(t, topDMs.Items, 2) + require.Equal(t, topDMs.Items[1].MessageCount, int64(1)) + }) +} diff --git a/model/client4.go b/model/client4.go index 76555fd9da..63c652caa0 100644 --- a/model/client4.go +++ b/model/client4.go @@ -6657,6 +6657,21 @@ func (c *Client4) GetTopReactionsForUserSince(teamId string, timeRange string, p return topReactions, BuildResponse(r), nil } +func (c *Client4) GetTopDMsForUserSince(timeRange string, page int, perPage int) (*TopDMList, *Response, error) { + query := fmt.Sprintf("?time_range=%v&page=%v&per_page=%v", timeRange, page, perPage) + + r, err := c.DoAPIGet(c.usersRoute()+"/me/top/dms"+query, "") + if err != nil { + return nil, BuildResponse(r), err + } + defer closeBody(r) + var topDMs *TopDMList + if jsonErr := json.NewDecoder(r.Body).Decode(&topDMs); jsonErr != nil { + return nil, nil, NewAppError("GetTopReactionsForUserSince", "api.unmarshal_error", nil, jsonErr.Error(), http.StatusInternalServerError) + } + return topDMs, BuildResponse(r), nil +} + // Timezone Section // GetSupportedTimezone returns a page of supported timezones on the system. From 8e6f294fbde06a3b3689d3c613d51aaca631efdd Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Thu, 21 Jul 2022 18:49:46 +0530 Subject: [PATCH 08/27] Lint, test fixes --- app/post_test.go | 8 ++++---- store/storetest/post_store.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/post_test.go b/app/post_test.go index 81f7dcdd11..c18bf2a9dc 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -3078,15 +3078,15 @@ func TestGetTopDMsForUserSince(t *testing.T) { u3 := th.CreateUser() u4 := th.CreateUser() // user direct messages - chUser1, nErr := th.App.createDirectChannel(u1.Id, user.Id) + chUser1, nErr := th.App.createDirectChannel(th.Context, u1.Id, user.Id) fmt.Println(chUser1, nErr) require.Nil(t, nErr) - chUser2, nErr := th.App.createDirectChannel(u2.Id, user.Id) + chUser2, nErr := th.App.createDirectChannel(th.Context, u2.Id, user.Id) require.Nil(t, nErr) - chUser3, nErr := th.App.createDirectChannel(u3.Id, user.Id) + chUser3, nErr := th.App.createDirectChannel(th.Context, u3.Id, user.Id) require.Nil(t, nErr) // other user direct message - chUser3User4, nErr := th.App.createDirectChannel(u3.Id, u4.Id) + chUser3User4, nErr := th.App.createDirectChannel(th.Context, u3.Id, u4.Id) require.Nil(t, nErr) // sample post data diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 33984b58a4..84707d599e 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -3930,8 +3930,8 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { }) require.NoError(t, err) t.Run("should return topDMs when userid is specified ", func(t *testing.T) { - topDMs, err := ss.Post().GetTopDMsForUserSince(user.Id, 100, 0, 100) - require.NoError(t, err) + topDMs, storeErr := ss.Post().GetTopDMsForUserSince(user.Id, 100, 0, 100) + require.NoError(t, storeErr) // len of topDMs.Items should be 3 require.Len(t, topDMs.Items, 3) // check order, magnitude of items @@ -3946,8 +3946,8 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { }) t.Run("topDMs should only consider user's DM channels ", func(t *testing.T) { // u4 only takes part in one conversation - topDMs, err := ss.Post().GetTopDMsForUserSince(u4.Id, 100, 0, 100) - require.NoError(t, err) + topDMs, storeErr := ss.Post().GetTopDMsForUserSince(u4.Id, 100, 0, 100) + require.NoError(t, storeErr) // len of topDMs.Items should be 3 require.Len(t, topDMs.Items, 1) // check order, magnitude of items From ec463c45fc391a5e8d27477e71a76ef75cde3e88 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Thu, 21 Jul 2022 19:11:09 +0530 Subject: [PATCH 09/27] go vet fixes --- api4/insights_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/api4/insights_test.go b/api4/insights_test.go index 6ea51b00f5..f5245e5a88 100644 --- a/api4/insights_test.go +++ b/api4/insights_test.go @@ -833,14 +833,14 @@ func TestGetTopDMsForUserSince(t *testing.T) { th.LoginBasic2() client := th.Client channelBu1, _, err := client.CreateDirectChannel(basicUser1.Id, basicUser1.Id) - require.Nil(t, err) + require.NoError(t, err) th.LoginBasic() client = th.Client channelBu, _, err := client.CreateDirectChannel(basicUser.Id, basicUser.Id) - require.Nil(t, err) + require.NoError(t, err) channelBu12, _, err := client.CreateDirectChannel(basicUser.Id, basicUser1.Id) - require.Nil(t, err) + require.NoError(t, err) // create 2 posts in channelBu, 1 in channelBu1, 3 in channelBu12 postsGenConfig := []map[string]interface{}{ @@ -867,14 +867,14 @@ func TestGetTopDMsForUserSince(t *testing.T) { userId := basicUser1.Id post := &model.Post{UserId: userId, ChannelId: postGen["chId"].(string), Message: "zz" + model.NewId() + "a"} _, _, err = client.CreatePost(post) - require.Nil(t, err) + require.NoError(t, err) } else { th.LoginBasic() client = th.Client userId := basicUser.Id post := &model.Post{UserId: userId, ChannelId: postGen["chId"].(string), Message: "zz" + model.NewId() + "a"} _, _, err = client.CreatePost(post) - require.Nil(t, err) + require.NoError(t, err) } } } @@ -884,7 +884,7 @@ func TestGetTopDMsForUserSince(t *testing.T) { th.LoginBasic() client = th.Client topDMs, _, err := client.GetTopDMsForUserSince("today", 0, 100) - require.Nil(t, err) + require.NoError(t, err) require.Len(t, topDMs.Items, 2) require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) @@ -895,7 +895,7 @@ func TestGetTopDMsForUserSince(t *testing.T) { th.LoginBasic2() client = th.Client topDMs, _, err := client.GetTopDMsForUserSince("today", 0, 100) - require.Nil(t, err) + require.NoError(t, err) require.Len(t, topDMs.Items, 2) require.Equal(t, topDMs.Items[1].MessageCount, int64(1)) }) From 7b3755f3d5b0560c12e893f133774d98d75546f9 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Fri, 22 Jul 2022 12:43:41 +0530 Subject: [PATCH 10/27] Add translation --- i18n/en.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/i18n/en.json b/i18n/en.json index a86209588a..5b068e0b14 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -5871,6 +5871,10 @@ "id": "app.post.get_root_posts.app_error", "translation": "Unable to get the posts for the channel." }, + { + "id": "app.post.get_top_dms_for_user_since.app_error", + "translation": "Unable to get top DMs for user." + }, { "id": "app.post.get_top_threads_for_team_since.app_error", "translation": "Unable to get top threads for team." From 27131f30512b919e15a46ca89e79ab4f86151d38 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Fri, 22 Jul 2022 12:52:47 +0530 Subject: [PATCH 11/27] Remove redundant translation --- i18n/en.json | 4 ---- 1 file changed, 4 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index 5b068e0b14..62dc855108 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1917,10 +1917,6 @@ "id": "api.insights.feature_disabled", "translation": "Insights is behind a feature flag which is not enabled." }, - { - "id": "api.insights.license_error", - "translation": "Your license doesn't support Insights feature." - }, { "id": "api.invalid_channel", "translation": "Channel listed in the request doesn't belong to the user" From 5f56d43d80c83aec92f62393f2a2445564da615e Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 25 Jul 2022 14:07:08 +0530 Subject: [PATCH 12/27] Make the following changes - Use db userId instead of referring to session - Filter out bot DM channels, and add relevant test --- api4/insights.go | 2 +- api4/insights_test.go | 38 ++++++++++++++++++++++++++++-------- store/sqlstore/post_store.go | 33 ++++++++++++++++++++++--------- 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/api4/insights.go b/api4/insights.go index db2bda4e36..e8302040d3 100644 --- a/api4/insights.go +++ b/api4/insights.go @@ -339,7 +339,7 @@ func getTopDMsForUserSince(c *Context, w http.ResponseWriter, r *http.Request) { startTime := model.StartOfDayForTimeRange(c.Params.TimeRange, user.GetTimezoneLocation()) - topDMs, err := c.App.GetTopDMsForUserSince(c.AppContext.Session().UserId, &model.InsightsOpts{ + topDMs, err := c.App.GetTopDMsForUserSince(user.Id, &model.InsightsOpts{ StartUnixMilli: startTime.UnixMilli(), Page: c.Params.Page, PerPage: c.Params.PerPage, diff --git a/api4/insights_test.go b/api4/insights_test.go index f5245e5a88..1f03aaa680 100644 --- a/api4/insights_test.go +++ b/api4/insights_test.go @@ -824,36 +824,58 @@ func TestGetTopDMsForUserSince(t *testing.T) { th.ConfigStore.SetReadOnlyFF(false) defer th.ConfigStore.SetReadOnlyFF(true) th.App.UpdateConfig(func(cfg *model.Config) { cfg.FeatureFlags.InsightsEnabled = true }) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableBotAccountCreation = true }) th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + // basicuser1 - bu1, basicuser - bu - // create dm channels for bu-bu, bu1-bu1, bu-bu1 + // create dm channels for bu-bu, bu1-bu1, bu-bu1, bot-bu basicUser := th.BasicUser basicUser1 := th.BasicUser2 th.LoginBasic2() client := th.Client - channelBu1, _, err := client.CreateDirectChannel(basicUser1.Id, basicUser1.Id) + channelBu1Bu1, _, err := client.CreateDirectChannel(basicUser1.Id, basicUser1.Id) require.NoError(t, err) th.LoginBasic() client = th.Client - channelBu, _, err := client.CreateDirectChannel(basicUser.Id, basicUser.Id) + channelBuBu, _, err := client.CreateDirectChannel(basicUser.Id, basicUser.Id) require.NoError(t, err) - channelBu12, _, err := client.CreateDirectChannel(basicUser.Id, basicUser1.Id) + channelBuBu1, _, err := client.CreateDirectChannel(basicUser.Id, basicUser1.Id) + require.NoError(t, err) + + // bot creation with permission + th.AddPermissionToRole(model.PermissionCreateBot.Id, model.TeamUserRoleId) + th.App.UpdateUserRoles(th.BasicUser.Id, model.TeamUserRoleId+" "+model.SystemUserRoleId, false) + bot := &model.Bot{ + Username: GenerateTestUsername(), + DisplayName: "a bot", + Description: "bot", + } + + createdBot, resp, err := th.Client.CreateBot(bot) + require.NoError(t, err) + CheckCreatedStatus(t, resp) + defer th.App.PermanentDeleteBot(createdBot.UserId) + channelBuBot, _, err := client.CreateDirectChannel(basicUser.Id, createdBot.UserId) require.NoError(t, err) // create 2 posts in channelBu, 1 in channelBu1, 3 in channelBu12 postsGenConfig := []map[string]interface{}{ { - "chId": channelBu.Id, + "chId": channelBuBu.Id, "postCount": 2, }, { - "chId": channelBu1.Id, + "chId": channelBu1Bu1.Id, "postCount": 1, }, { - "chId": channelBu12.Id, + "chId": channelBuBu1.Id, + "postCount": 3, + }, + { + "chId": channelBuBot.Id, "postCount": 3, }, } @@ -861,7 +883,7 @@ func TestGetTopDMsForUserSince(t *testing.T) { for _, postGen := range postsGenConfig { postCount := postGen["postCount"].(int) for i := 0; i < postCount; i++ { - if postGen["chId"] == channelBu1.Id { + if postGen["chId"] == channelBu1Bu1.Id { th.LoginBasic2() client = th.Client userId := basicUser1.Id diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index ab96f31fd1..7fe22ee1fc 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -3003,26 +3003,41 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset } // fill SecondParticipant column - topDMs = postProcessTopDMs(userID, topDMs) + topDMs, err = postProcessTopDMs(s, userID, topDMs) + if err != nil { + return nil, err + } return model.GetTopDMListWithPagination(topDMs, limit), nil } -func postProcessTopDMs(userID string, topDMs []*model.TopDM) []*model.TopDM { +func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([]*model.TopDM, error) { + var topDMsFiltered = []*model.TopDM{} for _, topDM := range topDMs { participants := strings.Split(topDM.Participants, ",") if len(participants) == 1 { // chatting to self topDM.SecondParticipant = userID - continue } else { // divide message count by 2, because it's counted twice due to channel memberships being 2 for dms. topDM.MessageCount = topDM.MessageCount / 2 + + if participants[0] == userID { + topDM.SecondParticipant = participants[1] + } else { + topDM.SecondParticipant = participants[0] + } + + // filter topDM out if second user is bot + users, err := s.User().GetProfileByIds(context.Background(), []string{topDM.SecondParticipant}, &store.UserGetByIdsOpts{}, true) + if err != nil { + return nil, errors.Wrapf(err, "failed to get second participant information for user-id: %s", topDM.SecondParticipant) + } + if users[0].IsBot { + continue + } } - if participants[0] == userID { - topDM.SecondParticipant = participants[1] - } else { - topDM.SecondParticipant = participants[0] - } + + topDMsFiltered = append(topDMsFiltered, topDM) } - return topDMs + return topDMsFiltered, nil } From e2e9e2cd149f671410214f06a3ece2fd7f803fac Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 25 Jul 2022 14:24:46 +0530 Subject: [PATCH 13/27] Add check for deleted users' DMs --- api4/insights_test.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/api4/insights_test.go b/api4/insights_test.go index 1f03aaa680..f95e59299a 100644 --- a/api4/insights_test.go +++ b/api4/insights_test.go @@ -823,7 +823,9 @@ func TestGetTopDMsForUserSince(t *testing.T) { th.ConfigStore.SetReadOnlyFF(false) defer th.ConfigStore.SetReadOnlyFF(true) - th.App.UpdateConfig(func(cfg *model.Config) { cfg.FeatureFlags.InsightsEnabled = true }) + th.App.UpdateConfig(func(c *model.Config) { + *c.TeamSettings.EnableUserDeactivation = true + }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableBotAccountCreation = true }) th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) @@ -921,4 +923,17 @@ func TestGetTopDMsForUserSince(t *testing.T) { require.Len(t, topDMs.Items, 2) require.Equal(t, topDMs.Items[1].MessageCount, int64(1)) }) + // deactivate basicuser1 + _, err = th.Client.DeleteUser(basicUser1.Id) + require.NoError(t, err) + // deactivated users DMs should show in topDMs + t.Run("get top dms for basic user 1", func(t *testing.T) { + th.LoginBasic() + client = th.Client + topDMs, _, err := client.GetTopDMsForUserSince("today", 0, 100) + require.NoError(t, err) + require.Len(t, topDMs.Items, 2) + require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) + require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) + }) } From 365def37cb3082cd64b87fc4bfb7541e6cfdb54b Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 25 Jul 2022 14:36:23 +0530 Subject: [PATCH 14/27] Lint fixes --- api4/insights_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/api4/insights_test.go b/api4/insights_test.go index f95e59299a..235fad247b 100644 --- a/api4/insights_test.go +++ b/api4/insights_test.go @@ -907,8 +907,8 @@ func TestGetTopDMsForUserSince(t *testing.T) { t.Run("get top dms for basic user 1", func(t *testing.T) { th.LoginBasic() client = th.Client - topDMs, _, err := client.GetTopDMsForUserSince("today", 0, 100) - require.NoError(t, err) + topDMs, _, topDmsErr := client.GetTopDMsForUserSince("today", 0, 100) + require.NoError(t, topDmsErr) require.Len(t, topDMs.Items, 2) require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) @@ -918,8 +918,8 @@ func TestGetTopDMsForUserSince(t *testing.T) { t.Run("get top dms for basic user 2", func(t *testing.T) { th.LoginBasic2() client = th.Client - topDMs, _, err := client.GetTopDMsForUserSince("today", 0, 100) - require.NoError(t, err) + topDMs, _, topDmsErr := client.GetTopDMsForUserSince("today", 0, 100) + require.NoError(t, topDmsErr) require.Len(t, topDMs.Items, 2) require.Equal(t, topDMs.Items[1].MessageCount, int64(1)) }) @@ -930,8 +930,8 @@ func TestGetTopDMsForUserSince(t *testing.T) { t.Run("get top dms for basic user 1", func(t *testing.T) { th.LoginBasic() client = th.Client - topDMs, _, err := client.GetTopDMsForUserSince("today", 0, 100) - require.NoError(t, err) + topDMs, _, topDmsErr := client.GetTopDMsForUserSince("today", 0, 100) + require.NoError(t, topDmsErr) require.Len(t, topDMs.Items, 2) require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) From b3330b1eb0175409b6c5833f9984ff238606768b Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 25 Jul 2022 16:09:44 +0530 Subject: [PATCH 15/27] Fix boolean check of user.IsBot --- store/sqlstore/post_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 7fe22ee1fc..cb0faeb0de 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -3032,7 +3032,7 @@ func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([ if err != nil { return nil, errors.Wrapf(err, "failed to get second participant information for user-id: %s", topDM.SecondParticipant) } - if users[0].IsBot { + if users[0].IsBot == true { continue } } From 870d86ae1d035ab8c25145fc2053b2777bd5bff4 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 25 Jul 2022 16:38:46 +0530 Subject: [PATCH 16/27] Save users to db in storetests, lint fix --- store/sqlstore/post_store.go | 2 +- store/storetest/post_store.go | 19 ++++++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index cb0faeb0de..7fe22ee1fc 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -3032,7 +3032,7 @@ func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([ if err != nil { return nil, errors.Wrapf(err, "failed to get second participant information for user-id: %s", topDM.SecondParticipant) } - if users[0].IsBot == true { + if users[0].IsBot { continue } } diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 84707d599e..b3df516ad7 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -3865,11 +3865,17 @@ func testGetNthRecentPostTime(t *testing.T, ss store.Store) { func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { // users - user := model.User{Id: model.NewId()} - u1 := model.User{Id: model.NewId()} - u2 := model.User{Id: model.NewId()} - u3 := model.User{Id: model.NewId()} - u4 := model.User{Id: model.NewId()} + user := model.User{Email: MakeEmail(), Username: model.NewId()} + u1 := model.User{Email: MakeEmail(), Username: model.NewId()} + u2 := model.User{Email: MakeEmail(), Username: model.NewId()} + u3 := model.User{Email: MakeEmail(), Username: model.NewId()} + u4 := model.User{Email: MakeEmail(), Username: model.NewId()} + _, err := ss.User().Save(&user) + _, err = ss.User().Save(&u1) + _, err = ss.User().Save(&u2) + _, err = ss.User().Save(&u3) + _, err = ss.User().Save(&u4) + require.NoError(t, err) // user direct messages chUser1, nErr := ss.Channel().CreateDirectChannel(&u1, &user) require.NoError(t, nErr) @@ -3883,7 +3889,7 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { // sample post data // for u1 - _, err := ss.Post().Save(&model.Post{ + _, err = ss.Post().Save(&model.Post{ ChannelId: chUser1.Id, UserId: u1.Id, }) @@ -3935,7 +3941,6 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { // len of topDMs.Items should be 3 require.Len(t, topDMs.Items, 3) // check order, magnitude of items - // fmt.Println(topDMs.Items[0].MessageCount, topDMs.Items[1].MessageCount, topDMs.Items[2].MessageCount) require.Equal(t, topDMs.Items[0].SecondParticipant, u3.Id) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) require.Equal(t, topDMs.Items[1].SecondParticipant, u1.Id) From db192aff1b4017975554791bc0078a048a2923e4 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 25 Jul 2022 17:00:06 +0530 Subject: [PATCH 17/27] Lint fixes --- store/storetest/post_store.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index b3df516ad7..968792078e 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -3871,9 +3871,13 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { u3 := model.User{Email: MakeEmail(), Username: model.NewId()} u4 := model.User{Email: MakeEmail(), Username: model.NewId()} _, err := ss.User().Save(&user) + require.NoError(t, err) _, err = ss.User().Save(&u1) + require.NoError(t, err) _, err = ss.User().Save(&u2) + require.NoError(t, err) _, err = ss.User().Save(&u3) + require.NoError(t, err) _, err = ss.User().Save(&u4) require.NoError(t, err) // user direct messages From e94532c8628d788c496d2aca03f3b17901ff1f07 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Wed, 27 Jul 2022 18:41:18 +0530 Subject: [PATCH 18/27] SecondParticipant now has extended user object --- api4/insights_test.go | 1 + app/post_test.go | 8 ++++---- model/insights.go | 11 ++++++++--- model/insights_test.go | 14 +++++++------- store/sqlstore/post_store.go | 21 +++++++++++++++++---- store/storetest/post_store.go | 10 +++++----- 6 files changed, 42 insertions(+), 23 deletions(-) diff --git a/api4/insights_test.go b/api4/insights_test.go index 235fad247b..8cbe7a40e1 100644 --- a/api4/insights_test.go +++ b/api4/insights_test.go @@ -912,6 +912,7 @@ func TestGetTopDMsForUserSince(t *testing.T) { require.Len(t, topDMs.Items, 2) require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) + require.Equal(t, topDMs.Items[0].SecondParticipant.Id, basicUser1.Id) }) // get top dms for bu1 diff --git a/app/post_test.go b/app/post_test.go index 1f611f8287..f8b12ed746 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -3145,11 +3145,11 @@ func TestGetTopDMsForUserSince(t *testing.T) { require.Len(t, topDMs.Items, 3) // check order, magnitude of items // fmt.Println(topDMs.Items[0].MessageCount, topDMs.Items[1].MessageCount, topDMs.Items[2].MessageCount) - require.Equal(t, topDMs.Items[0].SecondParticipant, u3.Id) + require.Equal(t, topDMs.Items[0].SecondParticipant.Id, u3.Id) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) - require.Equal(t, topDMs.Items[1].SecondParticipant, u1.Id) + require.Equal(t, topDMs.Items[1].SecondParticipant.Id, u1.Id) require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) - require.Equal(t, topDMs.Items[2].SecondParticipant, u2.Id) + require.Equal(t, topDMs.Items[2].SecondParticipant.Id, u2.Id) require.Equal(t, topDMs.Items[2].MessageCount, int64(1)) // this also ensures that u3-u4 conversation doesn't show up in others' top DMs. }) @@ -3160,7 +3160,7 @@ func TestGetTopDMsForUserSince(t *testing.T) { // len of topDMs.Items should be 3 require.Len(t, topDMs.Items, 1) // check order, magnitude of items - require.Equal(t, topDMs.Items[0].SecondParticipant, u3.Id) + require.Equal(t, topDMs.Items[0].SecondParticipant.Id, u3.Id) require.Equal(t, topDMs.Items[0].MessageCount, int64(4)) }) } diff --git a/model/insights.go b/model/insights.go index 76667e8916..85a8f29f31 100644 --- a/model/insights.go +++ b/model/insights.go @@ -90,6 +90,11 @@ type InsightUserInformation struct { Username string `json:"username"` } +type TopDMInsightUserInformation struct { + InsightUserInformation + Position string `json:"position"` +} + type DurationPostCount struct { ChannelID string `db:"channelid"` // Duration is an ISO8601 date string representing either a day or a day and hour (ex. "2022-05-26" or "2022-05-26T14"). @@ -99,9 +104,9 @@ type DurationPostCount struct { // Top DMs type TopDM struct { - MessageCount int64 `json:"post_count"` - Participants string `json:"-"` - SecondParticipant string `json:"second_participant"` + MessageCount int64 `json:"post_count"` + Participants string `json:"-"` + SecondParticipant *TopDMInsightUserInformation `json:"second_participant"` } type TopDMList struct { diff --git a/model/insights_test.go b/model/insights_test.go index c56fb0c980..5505cc1bc8 100644 --- a/model/insights_test.go +++ b/model/insights_test.go @@ -123,13 +123,13 @@ func TestGetTopThreadListWithPagination(t *testing.T) { func TestGetTopDMsListWithPagination(t *testing.T) { dms := []*TopDM{ - {SecondParticipant: NewId(), MessageCount: 100}, - {SecondParticipant: NewId(), MessageCount: 80}, - {SecondParticipant: NewId(), MessageCount: 90}, - {SecondParticipant: NewId(), MessageCount: 76}, - {SecondParticipant: NewId(), MessageCount: 43}, - {SecondParticipant: NewId(), MessageCount: 2}, - {SecondParticipant: NewId(), MessageCount: 1}, + {SecondParticipant: &TopDMInsightUserInformation{InsightUserInformation: InsightUserInformation{Id: NewId()}}, MessageCount: 100}, + {SecondParticipant: &TopDMInsightUserInformation{InsightUserInformation: InsightUserInformation{Id: NewId()}}, MessageCount: 80}, + {SecondParticipant: &TopDMInsightUserInformation{InsightUserInformation: InsightUserInformation{Id: NewId()}}, MessageCount: 90}, + {SecondParticipant: &TopDMInsightUserInformation{InsightUserInformation: InsightUserInformation{Id: NewId()}}, MessageCount: 76}, + {SecondParticipant: &TopDMInsightUserInformation{InsightUserInformation: InsightUserInformation{Id: NewId()}}, MessageCount: 43}, + {SecondParticipant: &TopDMInsightUserInformation{InsightUserInformation: InsightUserInformation{Id: NewId()}}, MessageCount: 2}, + {SecondParticipant: &TopDMInsightUserInformation{InsightUserInformation: InsightUserInformation{Id: NewId()}}, MessageCount: 1}, } hasNextTT := []struct { Description string diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index d98a940896..5d423efc5d 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -3014,27 +3014,40 @@ func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([ var topDMsFiltered = []*model.TopDM{} for _, topDM := range topDMs { participants := strings.Split(topDM.Participants, ",") + var secondParticipantId string if len(participants) == 1 { // chatting to self - topDM.SecondParticipant = userID + secondParticipantId = userID } else { // divide message count by 2, because it's counted twice due to channel memberships being 2 for dms. topDM.MessageCount = topDM.MessageCount / 2 if participants[0] == userID { - topDM.SecondParticipant = participants[1] + secondParticipantId = participants[1] } else { - topDM.SecondParticipant = participants[0] + secondParticipantId = participants[0] } // filter topDM out if second user is bot - users, err := s.User().GetProfileByIds(context.Background(), []string{topDM.SecondParticipant}, &store.UserGetByIdsOpts{}, true) + users, err := s.User().GetProfileByIds(context.Background(), []string{secondParticipantId}, &store.UserGetByIdsOpts{}, true) if err != nil { return nil, errors.Wrapf(err, "failed to get second participant information for user-id: %s", topDM.SecondParticipant) } if users[0].IsBot { continue } + user := users[0] + topDM.SecondParticipant = &model.TopDMInsightUserInformation{ + InsightUserInformation: model.InsightUserInformation{ + Id: user.Id, + LastPictureUpdate: user.LastPictureUpdate, + FirstName: user.FirstName, + LastName: user.LastName, + Username: user.Username, + NickName: user.Nickname, + }, + Position: user.Position, + } } topDMsFiltered = append(topDMsFiltered, topDM) diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 9b00b05994..7308272804 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -4073,11 +4073,11 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { // len of topDMs.Items should be 3 require.Len(t, topDMs.Items, 3) // check order, magnitude of items - require.Equal(t, topDMs.Items[0].SecondParticipant, u3.Id) + require.Equal(t, topDMs.Items[0].SecondParticipant.Id, u3.Id) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) - require.Equal(t, topDMs.Items[1].SecondParticipant, u1.Id) + require.Equal(t, topDMs.Items[1].SecondParticipant.Id, u1.Id) require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) - require.Equal(t, topDMs.Items[2].SecondParticipant, u2.Id) + require.Equal(t, topDMs.Items[2].SecondParticipant.Id, u2.Id) require.Equal(t, topDMs.Items[2].MessageCount, int64(1)) // this also ensures that u3-u4 conversation doesn't show up in others' top DMs. }) @@ -4088,7 +4088,7 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { // len of topDMs.Items should be 3 require.Len(t, topDMs.Items, 1) // check order, magnitude of items - require.Equal(t, topDMs.Items[0].SecondParticipant, u3.Id) + require.Equal(t, topDMs.Items[0].SecondParticipant.Id, u3.Id) require.Equal(t, topDMs.Items[0].MessageCount, int64(4)) }) t.Run("topDMs will consider self dms", func(t *testing.T) { @@ -4107,7 +4107,7 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { // len of topDMs.Items should be 3 require.Len(t, topDMs.Items, 3) // check order, magnitude of items - require.Equal(t, topDMs.Items[2].SecondParticipant, user.Id) + require.Equal(t, topDMs.Items[2].SecondParticipant.Id, user.Id) require.Equal(t, topDMs.Items[2].MessageCount, int64(1)) }) } From 55bf7458d7f2bf36c67522f6b1d47be8ffa93800 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Thu, 28 Jul 2022 12:35:17 +0530 Subject: [PATCH 19/27] Temporarily add second participant information for self dms --- store/sqlstore/post_store.go | 42 +++++++++++++++++------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 5d423efc5d..5a746ae558 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -3027,29 +3027,27 @@ func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([ } else { secondParticipantId = participants[0] } - - // filter topDM out if second user is bot - users, err := s.User().GetProfileByIds(context.Background(), []string{secondParticipantId}, &store.UserGetByIdsOpts{}, true) - if err != nil { - return nil, errors.Wrapf(err, "failed to get second participant information for user-id: %s", topDM.SecondParticipant) - } - if users[0].IsBot { - continue - } - user := users[0] - topDM.SecondParticipant = &model.TopDMInsightUserInformation{ - InsightUserInformation: model.InsightUserInformation{ - Id: user.Id, - LastPictureUpdate: user.LastPictureUpdate, - FirstName: user.FirstName, - LastName: user.LastName, - Username: user.Username, - NickName: user.Nickname, - }, - Position: user.Position, - } } - + // filter topDM out if second user is bot + users, err := s.User().GetProfileByIds(context.Background(), []string{secondParticipantId}, &store.UserGetByIdsOpts{}, true) + if err != nil { + return nil, errors.Wrapf(err, "failed to get second participant information for user-id: %s", topDM.SecondParticipant.Id) + } + if users[0].IsBot { + continue + } + user := users[0] + topDM.SecondParticipant = &model.TopDMInsightUserInformation{ + InsightUserInformation: model.InsightUserInformation{ + Id: user.Id, + LastPictureUpdate: user.LastPictureUpdate, + FirstName: user.FirstName, + LastName: user.LastName, + Username: user.Username, + NickName: user.Nickname, + }, + Position: user.Position, + } topDMsFiltered = append(topDMsFiltered, topDM) } return topDMsFiltered, nil From f007d941555348988baea9c52a66c82a0919db96 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Thu, 28 Jul 2022 12:46:18 +0530 Subject: [PATCH 20/27] Ignore self DMs --- api4/insights_test.go | 10 ++++------ store/sqlstore/post_store.go | 2 +- store/storetest/post_store.go | 7 ++----- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/api4/insights_test.go b/api4/insights_test.go index 8cbe7a40e1..5830762368 100644 --- a/api4/insights_test.go +++ b/api4/insights_test.go @@ -909,8 +909,7 @@ func TestGetTopDMsForUserSince(t *testing.T) { client = th.Client topDMs, _, topDmsErr := client.GetTopDMsForUserSince("today", 0, 100) require.NoError(t, topDmsErr) - require.Len(t, topDMs.Items, 2) - require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) + require.Len(t, topDMs.Items, 1) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) require.Equal(t, topDMs.Items[0].SecondParticipant.Id, basicUser1.Id) }) @@ -921,8 +920,8 @@ func TestGetTopDMsForUserSince(t *testing.T) { client = th.Client topDMs, _, topDmsErr := client.GetTopDMsForUserSince("today", 0, 100) require.NoError(t, topDmsErr) - require.Len(t, topDMs.Items, 2) - require.Equal(t, topDMs.Items[1].MessageCount, int64(1)) + require.Len(t, topDMs.Items, 1) + require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) }) // deactivate basicuser1 _, err = th.Client.DeleteUser(basicUser1.Id) @@ -933,8 +932,7 @@ func TestGetTopDMsForUserSince(t *testing.T) { client = th.Client topDMs, _, topDmsErr := client.GetTopDMsForUserSince("today", 0, 100) require.NoError(t, topDmsErr) - require.Len(t, topDMs.Items, 2) - require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) + require.Len(t, topDMs.Items, 1) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) }) } diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 5a746ae558..b1fb82aeb2 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -3017,7 +3017,7 @@ func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([ var secondParticipantId string if len(participants) == 1 { // chatting to self - secondParticipantId = userID + continue } else { // divide message count by 2, because it's counted twice due to channel memberships being 2 for dms. topDM.MessageCount = topDM.MessageCount / 2 diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 7308272804..aca7a8f76e 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -4091,7 +4091,7 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { require.Equal(t, topDMs.Items[0].SecondParticipant.Id, u3.Id) require.Equal(t, topDMs.Items[0].MessageCount, int64(4)) }) - t.Run("topDMs will consider self dms", func(t *testing.T) { + t.Run("topDMs will not consider self dms", func(t *testing.T) { chUser, nErr := ss.Channel().CreateDirectChannel(&user, &user) require.NoError(t, nErr) _, err = ss.Post().Save(&model.Post{ @@ -4105,9 +4105,6 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { topDMs, err := ss.Post().GetTopDMsForUserSince(user.Id, 100, 0, 100) require.NoError(t, err) // len of topDMs.Items should be 3 - require.Len(t, topDMs.Items, 3) - // check order, magnitude of items - require.Equal(t, topDMs.Items[2].SecondParticipant.Id, user.Id) - require.Equal(t, topDMs.Items[2].MessageCount, int64(1)) + require.Len(t, topDMs.Items, 2) }) } From 57e25b5d8fe5cba4de970525148cc62002ab9764 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Thu, 28 Jul 2022 16:36:37 +0530 Subject: [PATCH 21/27] Add information on individual message count --- model/insights.go | 13 ++++++-- store/sqlstore/post_store.go | 65 ++++++++++++++++++++++++++++++------ 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/model/insights.go b/model/insights.go index 85a8f29f31..2593d3e619 100644 --- a/model/insights.go +++ b/model/insights.go @@ -104,9 +104,16 @@ type DurationPostCount struct { // Top DMs type TopDM struct { - MessageCount int64 `json:"post_count"` - Participants string `json:"-"` - SecondParticipant *TopDMInsightUserInformation `json:"second_participant"` + MessageCount int64 `json:"post_count"` + OutgoingMessageCount int64 `json:"outgoing_message_count"` + Participants string `json:"-"` + ChannelId string `json:"-"` + SecondParticipant *TopDMInsightUserInformation `json:"second_participant"` +} + +type OutgoingMessageQueryResult struct { + ChannelId string + MessageCount int } type TopDMList struct { diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index b1fb82aeb2..ea5e176e91 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -2978,7 +2978,7 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset aggregator = "string_agg(distinct cm.UserId, ',') as Participants" } - topDMsBuilder := s.getQueryBuilder().Select("count(p.id) as MessageCount", aggregator).FromSelect(channelSelector, "vch"). + topDMsBuilder := s.getQueryBuilder().Select("count(p.id) as MessageCount", aggregator, "vch.Id as ChannelId").FromSelect(channelSelector, "vch"). Join("ChannelMembers as cm on cm.ChannelId = vch.Id"). Join("Posts as p on p.ChannelId = vch.Id"). Where(sq.And{ @@ -3012,12 +3012,16 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([]*model.TopDM, error) { var topDMsFiltered = []*model.TopDM{} + var secondParticipantIds []string + var channelIds []string + + // identify second participant in a list of participants for _, topDM := range topDMs { participants := strings.Split(topDM.Participants, ",") var secondParticipantId string if len(participants) == 1 { - // chatting to self - continue + // channel with self + secondParticipantId = "-1" } else { // divide message count by 2, because it's counted twice due to channel memberships being 2 for dms. topDM.MessageCount = topDM.MessageCount / 2 @@ -3028,15 +3032,53 @@ func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([ secondParticipantId = participants[0] } } - // filter topDM out if second user is bot - users, err := s.User().GetProfileByIds(context.Background(), []string{secondParticipantId}, &store.UserGetByIdsOpts{}, true) - if err != nil { - return nil, errors.Wrapf(err, "failed to get second participant information for user-id: %s", topDM.SecondParticipant.Id) - } - if users[0].IsBot { + secondParticipantIds = append(secondParticipantIds, secondParticipantId) + channelIds = append(channelIds, topDM.ChannelId) + } + + // get user profiles + users, err := s.User().GetProfileByIds(context.Background(), secondParticipantIds, &store.UserGetByIdsOpts{}, true) + if err != nil { + return nil, errors.Wrapf(err, "failed to get second participants' information") + } + + // get outgoing message count for userId + outgoingMessagesQuery := s.getQueryBuilder().Select("ch.Id as ChannelId, count(p.Id) as MessageCount").From("Channels as ch"). + Join("Posts as p on p.ChannelId=ch.Id").Where(sq.Eq{ + "ch.Id": channelIds, + "p.UserId": userID, + }).GroupBy("ch.Id") + + outgoingMessages := make([]*model.OutgoingMessageQueryResult, 0) + sql, args, err := outgoingMessagesQuery.ToSql() + if err != nil { + return nil, errors.Wrap(err, "GetTopDMsForUserSince_outgoingMessagesQuery_ToSql") + } + err = s.GetReplicaX().Select(&outgoingMessages, sql, args...) + if err != nil { + return nil, errors.Wrapf(err, "failed to find top DMs for user-id: %s", userID) + } + + // create map of channelId -> MessageCount + outgoingMessagesMap := make(map[string]int) + for _, outgoingMessage := range outgoingMessages { + outgoingMessagesMap[outgoingMessage.ChannelId] = outgoingMessage.MessageCount + } + + // create map of userId -> User + usersMap := make(map[string]*model.User) + for _, user := range users { + usersMap[user.Id] = user + } + + for index, topDM := range topDMs { + if secondParticipantIds[index] == "-1" { + continue + } + user := usersMap[secondParticipantIds[index]] + if user.IsBot { continue } - user := users[0] topDM.SecondParticipant = &model.TopDMInsightUserInformation{ InsightUserInformation: model.InsightUserInformation{ Id: user.Id, @@ -3048,8 +3090,11 @@ func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([ }, Position: user.Position, } + + topDM.OutgoingMessageCount = int64(outgoingMessagesMap[topDM.ChannelId]) topDMsFiltered = append(topDMsFiltered, topDM) } + return topDMsFiltered, nil } From 9f468cf01185e0d52504278ed5e36ed6b3c379c3 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Thu, 28 Jul 2022 17:22:14 +0530 Subject: [PATCH 22/27] Select total message count from Channel instead of counting posts --- store/sqlstore/post_store.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index ea5e176e91..1a71553fee 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -2978,7 +2978,7 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset aggregator = "string_agg(distinct cm.UserId, ',') as Participants" } - topDMsBuilder := s.getQueryBuilder().Select("count(p.id) as MessageCount", aggregator, "vch.Id as ChannelId").FromSelect(channelSelector, "vch"). + topDMsBuilder := s.getQueryBuilder().Select("vch.TotalMsgCount as MessageCount", aggregator, "vch.Id as ChannelId").FromSelect(channelSelector, "vch"). Join("ChannelMembers as cm on cm.ChannelId = vch.Id"). Join("Posts as p on p.ChannelId = vch.Id"). Where(sq.And{ @@ -3023,9 +3023,6 @@ func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([ // channel with self secondParticipantId = "-1" } else { - // divide message count by 2, because it's counted twice due to channel memberships being 2 for dms. - topDM.MessageCount = topDM.MessageCount / 2 - if participants[0] == userID { secondParticipantId = participants[1] } else { From f84a6ae7ac903022488c000f065b32f5c32dc7e6 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Fri, 29 Jul 2022 13:05:05 +0530 Subject: [PATCH 23/27] Add TotalMsgCount to GROUP BY fields --- store/sqlstore/post_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 1a71553fee..501d40a3dd 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -2988,7 +2988,7 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset sq.Eq{ "p.DeleteAt": 0, }, - }).GroupBy("vch.id") + }).GroupBy("vch.id", "vch.TotalMsgCount") topDMsBuilder = topDMsBuilder.OrderBy("MessageCount DESC").Limit(uint64(limit)).Offset(uint64(offset)) From 61a716a98c5680897133f870320cfe9b445c8c28 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 1 Aug 2022 13:58:50 +0530 Subject: [PATCH 24/27] Fix Posts query while populating top DMs, add OutgoingMessageCount check to tests --- store/sqlstore/post_store.go | 23 +++++++++++++++++------ store/storetest/post_store.go | 3 +++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 501d40a3dd..6e80105a3d 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -3003,14 +3003,14 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset } // fill SecondParticipant column - topDMs, err = postProcessTopDMs(s, userID, topDMs) + topDMs, err = postProcessTopDMs(s, userID, topDMs, since) if err != nil { return nil, err } return model.GetTopDMListWithPagination(topDMs, limit), nil } -func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([]*model.TopDM, error) { +func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM, since int64) ([]*model.TopDM, error) { var topDMsFiltered = []*model.TopDM{} var secondParticipantIds []string var channelIds []string @@ -3041,10 +3041,21 @@ func postProcessTopDMs(s *SqlPostStore, userID string, topDMs []*model.TopDM) ([ // get outgoing message count for userId outgoingMessagesQuery := s.getQueryBuilder().Select("ch.Id as ChannelId, count(p.Id) as MessageCount").From("Channels as ch"). - Join("Posts as p on p.ChannelId=ch.Id").Where(sq.Eq{ - "ch.Id": channelIds, - "p.UserId": userID, - }).GroupBy("ch.Id") + Join("Posts as p on p.ChannelId=ch.Id").Where( + sq.And{ + sq.Gt{ + "p.UpdateAt": since, + }, + sq.Eq{ + "p.DeleteAt": 0, + }, + sq.Eq{ + "ch.Id": channelIds, + }, + sq.Eq{ + "p.UserId": userID, + }, + }).GroupBy("ch.Id") outgoingMessages := make([]*model.OutgoingMessageQueryResult, 0) sql, args, err := outgoingMessagesQuery.ToSql() diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index aca7a8f76e..f077182a01 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -4075,10 +4075,13 @@ func testGetTopDMsForUserSince(t *testing.T, ss store.Store, s SqlStore) { // check order, magnitude of items require.Equal(t, topDMs.Items[0].SecondParticipant.Id, u3.Id) require.Equal(t, topDMs.Items[0].MessageCount, int64(3)) + require.Equal(t, topDMs.Items[0].OutgoingMessageCount, int64(3)) require.Equal(t, topDMs.Items[1].SecondParticipant.Id, u1.Id) require.Equal(t, topDMs.Items[1].MessageCount, int64(2)) + require.Equal(t, topDMs.Items[1].OutgoingMessageCount, int64(1)) require.Equal(t, topDMs.Items[2].SecondParticipant.Id, u2.Id) require.Equal(t, topDMs.Items[2].MessageCount, int64(1)) + require.Equal(t, topDMs.Items[2].OutgoingMessageCount, int64(0)) // this also ensures that u3-u4 conversation doesn't show up in others' top DMs. }) t.Run("topDMs should only consider user's DM channels ", func(t *testing.T) { From d5f5d18fa9ce46cf4d77be3624d4c252b4d26f98 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 1 Aug 2022 14:38:32 +0530 Subject: [PATCH 25/27] Lint fix --- api4/insights_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api4/insights_test.go b/api4/insights_test.go index 5830762368..73b9c87496 100644 --- a/api4/insights_test.go +++ b/api4/insights_test.go @@ -848,7 +848,7 @@ func TestGetTopDMsForUserSince(t *testing.T) { // bot creation with permission th.AddPermissionToRole(model.PermissionCreateBot.Id, model.TeamUserRoleId) - th.App.UpdateUserRoles(th.BasicUser.Id, model.TeamUserRoleId+" "+model.SystemUserRoleId, false) + th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.TeamUserRoleId+" "+model.SystemUserRoleId, false) bot := &model.Bot{ Username: GenerateTestUsername(), DisplayName: "a bot", From 45e434cc26bffadb1102fd89f2cd174cc5de2927 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Wed, 3 Aug 2022 13:28:03 +0530 Subject: [PATCH 26/27] Fix issue with pagination where has_next is false for every case --- store/sqlstore/post_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 6e80105a3d..be309e4e3c 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -2990,7 +2990,7 @@ func (s *SqlPostStore) GetTopDMsForUserSince(userID string, since int64, offset }, }).GroupBy("vch.id", "vch.TotalMsgCount") - topDMsBuilder = topDMsBuilder.OrderBy("MessageCount DESC").Limit(uint64(limit)).Offset(uint64(offset)) + topDMsBuilder = topDMsBuilder.OrderBy("MessageCount DESC").Limit(uint64(limit + 1)).Offset(uint64(offset)) topDMs := make([]*model.TopDM, 0) sql, args, err := topDMsBuilder.ToSql() From e5c1b17d772fe3904c6418269967e8395f7bb950 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Thu, 11 Aug 2022 12:19:25 +0530 Subject: [PATCH 27/27] Lint fixes --- api4/insights.go | 8 ++++---- app/post_test.go | 2 -- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/api4/insights.go b/api4/insights.go index 71a368e96a..0f84e69eec 100644 --- a/api4/insights.go +++ b/api4/insights.go @@ -225,7 +225,7 @@ func getTopChannelsForUserSince(c *Context, w http.ResponseWriter, r *http.Reque js, jsonErr := json.Marshal(topChannels) if jsonErr != nil { - c.Err = model.NewAppError("getTopChannelsForUserSince", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) + c.Err = model.NewAppError("getTopChannelsForUserSince", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(jsonErr) return } @@ -318,9 +318,9 @@ func getTopThreadsForUserSince(c *Context, w http.ResponseWriter, r *http.Reques return } - js, err := json.Marshal(topThreads) - if err != nil { - c.Err = model.NewAppError("getTopThreadsForUserSince", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err) + js, jsonErr := json.Marshal(topThreads) + if jsonErr != nil { + c.Err = model.NewAppError("getTopThreadsForUserSince", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(jsonErr) return } diff --git a/app/post_test.go b/app/post_test.go index f144b894ca..2ca888c004 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -3067,8 +3067,6 @@ func TestGetTopDMsForUserSince(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() - th.Server.configStore.SetReadOnlyFF(false) - defer th.Server.configStore.SetReadOnlyFF(true) th.App.UpdateConfig(func(cfg *model.Config) { cfg.FeatureFlags.InsightsEnabled = true }) // users