From fdde7c82877341c8d86213c549e1a92ccd35ca6e Mon Sep 17 00:00:00 2001 From: jfrerich Date: Fri, 19 Jul 2019 13:30:59 -0500 Subject: [PATCH] =?UTF-8?q?[MM-16871]=20Remove=20bots=20from=20"Daily=20Ac?= =?UTF-8?q?tive=20Users"=20and=20"Monthly=E2=80=A6=20(#11658)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add model.UserCountOptions to analyticsActiveCount method Add new test for analyticsActiveCount method * Add ability to delete entries from status store, by calling SQl directly. There is no method available to delete entries from the status store interace * Instead of cleaning up after a test by passing userIds, just delete all records in the status table and start fresh * Remove Comment --- app/analytics.go | 4 +- app/diagnostics.go | 4 +- store/sqlstore/user_store.go | 10 +++- store/sqlstore/user_store_test.go | 2 +- store/store.go | 2 +- store/storetest/mocks/UserStore.go | 14 ++--- store/storetest/user_store.go | 82 ++++++++++++++++++++++++++++-- 7 files changed, 98 insertions(+), 20 deletions(-) diff --git a/app/analytics.go b/app/analytics.go index ca4d713aea..a7aa0b43dd 100644 --- a/app/analytics.go +++ b/app/analytics.go @@ -93,14 +93,14 @@ func (a *App) GetAnalytics(name string, teamId string) (model.AnalyticsRows, *mo dailyActiveChan := make(chan store.StoreResult, 1) go func() { - dailyActive, err := a.Srv.Store.User().AnalyticsActiveCount(DAY_MILLISECONDS) + dailyActive, err := a.Srv.Store.User().AnalyticsActiveCount(DAY_MILLISECONDS, model.UserCountOptions{IncludeBotAccounts: false}) dailyActiveChan <- store.StoreResult{Data: dailyActive, Err: err} close(dailyActiveChan) }() monthlyActiveChan := make(chan store.StoreResult, 1) go func() { - monthlyActive, err := a.Srv.Store.User().AnalyticsActiveCount(MONTH_MILLISECONDS) + monthlyActive, err := a.Srv.Store.User().AnalyticsActiveCount(MONTH_MILLISECONDS, model.UserCountOptions{IncludeBotAccounts: false}) monthlyActiveChan <- store.StoreResult{Data: monthlyActive, Err: err} close(monthlyActiveChan) }() diff --git a/app/diagnostics.go b/app/diagnostics.go index 58a5b5b25a..b9aa140bd4 100644 --- a/app/diagnostics.go +++ b/app/diagnostics.go @@ -123,14 +123,14 @@ func (a *App) trackActivity() { activeUsersDailyCountChan := make(chan store.StoreResult, 1) go func() { - count, err := a.Srv.Store.User().AnalyticsActiveCount(DAY_MILLISECONDS) + count, err := a.Srv.Store.User().AnalyticsActiveCount(DAY_MILLISECONDS, model.UserCountOptions{IncludeBotAccounts: false}) activeUsersDailyCountChan <- store.StoreResult{Data: count, Err: err} close(activeUsersDailyCountChan) }() activeUsersMonthlyCountChan := make(chan store.StoreResult, 1) go func() { - count, err := a.Srv.Store.User().AnalyticsActiveCount(MONTH_MILLISECONDS) + count, err := a.Srv.Store.User().AnalyticsActiveCount(MONTH_MILLISECONDS, model.UserCountOptions{IncludeBotAccounts: false}) activeUsersMonthlyCountChan <- store.StoreResult{Data: count, Err: err} close(activeUsersMonthlyCountChan) }() diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 800fb59286..96b96f6326 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -1105,11 +1105,17 @@ func (us SqlUserStore) Count(options model.UserCountOptions) (int64, *model.AppE return count, nil } -func (us SqlUserStore) AnalyticsActiveCount(timePeriod int64) (int64, *model.AppError) { +func (us SqlUserStore) AnalyticsActiveCount(timePeriod int64, options model.UserCountOptions) (int64, *model.AppError) { time := model.GetMillis() - timePeriod - query := "SELECT COUNT(*) FROM Status WHERE LastActivityAt > :Time" + query := "SELECT COUNT(*) FROM Status s" + + if options.IncludeBotAccounts { + query += " WHERE LastActivityAt > :Time" + } else { + query += " LEFT JOIN Bots ON s.UserId = Bots.UserId WHERE Bots.UserId IS NULL AND LastActivityAt > :Time" + } v, err := us.GetReplica().SelectInt(query, map[string]interface{}{"Time": time}) if err != nil { diff --git a/store/sqlstore/user_store_test.go b/store/sqlstore/user_store_test.go index 37d169c90f..fe2c748544 100644 --- a/store/sqlstore/user_store_test.go +++ b/store/sqlstore/user_store_test.go @@ -10,5 +10,5 @@ import ( ) func TestUserStore(t *testing.T) { - StoreTest(t, storetest.TestUserStore) + StoreTestWithSqlSupplier(t, storetest.TestUserStore) } diff --git a/store/store.go b/store/store.go index 78d7777eef..22543aaf37 100644 --- a/store/store.go +++ b/store/store.go @@ -285,7 +285,7 @@ type UserStore interface { UpdateFailedPasswordAttempts(userId string, attempts int) *model.AppError GetSystemAdminProfiles() (map[string]*model.User, *model.AppError) PermanentDelete(userId string) *model.AppError - AnalyticsActiveCount(time int64) (int64, *model.AppError) + AnalyticsActiveCount(time int64, options model.UserCountOptions) (int64, *model.AppError) GetUnreadCount(userId string) (int64, error) GetUnreadCountForChannel(userId string, channelId string) (int64, *model.AppError) GetAnyUnreadPostCountForChannel(userId string, channelId string) (int64, *model.AppError) diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index d00b74b8b9..9ae070dc0e 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -13,20 +13,20 @@ type UserStore struct { mock.Mock } -// AnalyticsActiveCount provides a mock function with given fields: time -func (_m *UserStore) AnalyticsActiveCount(time int64) (int64, *model.AppError) { - ret := _m.Called(time) +// AnalyticsActiveCount provides a mock function with given fields: time, options +func (_m *UserStore) AnalyticsActiveCount(time int64, options model.UserCountOptions) (int64, *model.AppError) { + ret := _m.Called(time, options) var r0 int64 - if rf, ok := ret.Get(0).(func(int64) int64); ok { - r0 = rf(time) + if rf, ok := ret.Get(0).(func(int64, model.UserCountOptions) int64); ok { + r0 = rf(time, options) } else { r0 = ret.Get(0).(int64) } var r1 *model.AppError - if rf, ok := ret.Get(1).(func(int64) *model.AppError); ok { - r1 = rf(time) + if rf, ok := ret.Get(1).(func(int64, model.UserCountOptions) *model.AppError); ok { + r1 = rf(time, options) } else { if ret.Get(1) != nil { r1 = ret.Get(1).(*model.AppError) diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 0928f4e46c..f289f39a43 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -15,7 +15,17 @@ import ( "github.com/mattermost/mattermost-server/store" ) -func TestUserStore(t *testing.T, ss store.Store) { +const ( + DAY_MILLISECONDS = 24 * 60 * 60 * 1000 + MONTH_MILLISECONDS = 31 * DAY_MILLISECONDS +) + +func cleanupStatusStore(t *testing.T, s SqlSupplier) { + _, execerr := s.GetMaster().ExecNoTimeout(` DELETE FROM Status `) + require.Nil(t, execerr) +} + +func TestUserStore(t *testing.T, ss store.Store, s SqlSupplier) { users, err := ss.User().GetAll() require.Nil(t, err, "failed cleaning up test users") @@ -25,6 +35,7 @@ func TestUserStore(t *testing.T, ss store.Store) { } t.Run("Count", func(t *testing.T) { testCount(t, ss) }) + t.Run("AnalyticsActiveCount", func(t *testing.T) { testUserStoreAnalyticsActiveCount(t, ss, s) }) t.Run("AnalyticsGetInactiveUsersCount", func(t *testing.T) { testUserStoreAnalyticsGetInactiveUsersCount(t, ss) }) t.Run("AnalyticsGetSystemAdminCount", func(t *testing.T) { testUserStoreAnalyticsGetSystemAdminCount(t, ss) }) t.Run("Save", func(t *testing.T) { testUserStoreSave(t, ss) }) @@ -36,7 +47,7 @@ func TestUserStore(t *testing.T, ss store.Store) { t.Run("GetAllProfiles", func(t *testing.T) { testUserStoreGetAllProfiles(t, ss) }) t.Run("GetProfiles", func(t *testing.T) { testUserStoreGetProfiles(t, ss) }) t.Run("GetProfilesInChannel", func(t *testing.T) { testUserStoreGetProfilesInChannel(t, ss) }) - t.Run("GetProfilesInChannelByStatus", func(t *testing.T) { testUserStoreGetProfilesInChannelByStatus(t, ss) }) + t.Run("GetProfilesInChannelByStatus", func(t *testing.T) { testUserStoreGetProfilesInChannelByStatus(t, ss, s) }) t.Run("GetProfilesWithoutTeam", func(t *testing.T) { testUserStoreGetProfilesWithoutTeam(t, ss) }) t.Run("GetAllProfilesInChannel", func(t *testing.T) { testUserStoreGetAllProfilesInChannel(t, ss) }) t.Run("GetProfilesNotInChannel", func(t *testing.T) { testUserStoreGetProfilesNotInChannel(t, ss) }) @@ -54,7 +65,7 @@ func TestUserStore(t *testing.T, ss store.Store) { t.Run("UserUnreadCount", func(t *testing.T) { testUserUnreadCount(t, ss) }) t.Run("UpdateMfaSecret", func(t *testing.T) { testUserStoreUpdateMfaSecret(t, ss) }) t.Run("UpdateMfaActive", func(t *testing.T) { testUserStoreUpdateMfaActive(t, ss) }) - t.Run("GetRecentlyActiveUsersForTeam", func(t *testing.T) { testUserStoreGetRecentlyActiveUsersForTeam(t, ss) }) + t.Run("GetRecentlyActiveUsersForTeam", func(t *testing.T) { testUserStoreGetRecentlyActiveUsersForTeam(t, ss, s) }) t.Run("GetNewUsersForTeam", func(t *testing.T) { testUserStoreGetNewUsersForTeam(t, ss) }) t.Run("Search", func(t *testing.T) { testUserStoreSearch(t, ss) }) t.Run("SearchNotInChannel", func(t *testing.T) { testUserStoreSearchNotInChannel(t, ss) }) @@ -752,7 +763,10 @@ func testUserStoreGetProfilesInChannel(t *testing.T, ss store.Store) { }) } -func testUserStoreGetProfilesInChannelByStatus(t *testing.T, ss store.Store) { +func testUserStoreGetProfilesInChannelByStatus(t *testing.T, ss store.Store, s SqlSupplier) { + + cleanupStatusStore(t, s) + teamId := model.NewId() u1, err := ss.User().Save(&model.User{ @@ -2022,7 +2036,10 @@ func testUserStoreUpdateMfaActive(t *testing.T, ss store.Store) { } } -func testUserStoreGetRecentlyActiveUsersForTeam(t *testing.T, ss store.Store) { +func testUserStoreGetRecentlyActiveUsersForTeam(t *testing.T, ss store.Store, s SqlSupplier) { + + cleanupStatusStore(t, s) + teamId := model.NewId() u1, err := ss.User().Save(&model.User{ @@ -3334,6 +3351,61 @@ func testCount(t *testing.T, ss store.Store) { require.Equal(t, int64(0), count) } +func testUserStoreAnalyticsActiveCount(t *testing.T, ss store.Store, s SqlSupplier) { + + cleanupStatusStore(t, s) + + // Create 5 users statuses u0, u1, u2, u3, u4. + // u4 is also a bot + u0Id := model.NewId() + u1Id := model.NewId() + u2Id := model.NewId() + u3Id := model.NewId() + + u4, err := ss.User().Save(&model.User{ + Email: MakeEmail(), + Username: "u4" + model.NewId(), + }) + require.Nil(t, err) + defer func() { require.Nil(t, ss.User().PermanentDelete(u4.Id)) }() + + _, err = ss.Bot().Save(&model.Bot{ + UserId: u4.Id, + Username: u4.Username, + OwnerId: u1Id, + }) + require.Nil(t, err) + + millis := model.GetMillis() + millisTwoDaysAgo := model.GetMillis() - (2 * DAY_MILLISECONDS) + millisTwoMonthsAgo := model.GetMillis() - (2 * MONTH_MILLISECONDS) + + // u0 last activity status is two months ago. + // u1 last activity status is two days ago. + // u2, u3, u4 last activity is within last day + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{UserId: u0Id, Status: model.STATUS_OFFLINE, LastActivityAt: millisTwoMonthsAgo})) + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{UserId: u1Id, Status: model.STATUS_OFFLINE, LastActivityAt: millisTwoDaysAgo})) + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{UserId: u2Id, Status: model.STATUS_OFFLINE, LastActivityAt: millis})) + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{UserId: u3Id, Status: model.STATUS_OFFLINE, LastActivityAt: millis})) + require.Nil(t, ss.Status().SaveOrUpdate(&model.Status{UserId: u4.Id, Status: model.STATUS_OFFLINE, LastActivityAt: millis})) + + // Daily counts (without bots) + count, err := ss.User().AnalyticsActiveCount(DAY_MILLISECONDS, model.UserCountOptions{IncludeBotAccounts: false}) + assert.Equal(t, int64(2), count) + + // Daily counts (with bots) + count, err = ss.User().AnalyticsActiveCount(DAY_MILLISECONDS, model.UserCountOptions{IncludeBotAccounts: true}) + assert.Equal(t, int64(3), count) + + // Monthly counts (without bots) + count, err = ss.User().AnalyticsActiveCount(MONTH_MILLISECONDS, model.UserCountOptions{IncludeBotAccounts: false}) + assert.Equal(t, int64(3), count) + + // Monthly counts - (with bots) + count, err = ss.User().AnalyticsActiveCount(MONTH_MILLISECONDS, model.UserCountOptions{IncludeBotAccounts: true}) + assert.Equal(t, int64(4), count) +} + func testUserStoreAnalyticsGetInactiveUsersCount(t *testing.T, ss store.Store) { u1 := &model.User{} u1.Email = MakeEmail()