MM-60415 ignore deactivated bots (mattermost#28184)

* ignore bot accounts when counting deactivated users * moved query to squirrel Co-authored-by: Ben Schumacher <[email protected]> * set is null right * Run a different query depending on driver due to performance reasons. --------- Co-authored-by: Ben Schumacher <[email protected]> Co-authored-by: Mattermost Build <[email protected]>
Aryakoste · Sep 20, 2024 · dfa1d10 · dfa1d10
1 parent 9b368b9
commit dfa1d10
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 3 deletions.
diff --git a/server/channels/store/sqlstore/user_store.go b/server/channels/store/sqlstore/user_store.go
@@ -1708,7 +1708,27 @@ func (us SqlUserStore) performSearch(query sq.SelectBuilder, term string, option
 
 func (us SqlUserStore) AnalyticsGetInactiveUsersCount() (int64, error) {
 	var count int64
-	err := us.GetReplicaX().Get(&count, "SELECT COUNT(Id) FROM Users WHERE DeleteAt > 0")
+	query := us.getQueryBuilder().
+		Select("COUNT(Id)").
+		From("Users")
+	if us.DriverName() == model.DatabaseDriverPostgres {
+		query = query.LeftJoin("Bots ON Users.ID = Bots.UserId").
+			Where(sq.And{
+				sq.Gt{"Users.DeleteAt": 0},
+				sq.Eq{"Bots.UserId": nil},
+			})
+	} else {
+		query = query.Where(sq.And{
+			sq.Expr("Users.Id NOT IN (SELECT UserId FROM Bots)"),
+			sq.Gt{"Users.DeleteAt": 0},
+		})
+	}
+	queryStr, args, err := query.ToSql()
+	if err != nil {
+		return int64(0), errors.Wrap(err, "failed to create a SQL query to count inactive users")
+	}
+	err = us.GetReplicaX().Get(&count, queryStr, args...)
+
 	if err != nil {
 		return int64(0), errors.Wrap(err, "failed to count inactive Users")
 	}

diff --git a/server/channels/store/storetest/user_store.go b/server/channels/store/storetest/user_store.go
@@ -46,6 +46,7 @@ func TestUserStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) {
 	t.Run("AnalyticsActiveCount", func(t *testing.T) { testUserStoreAnalyticsActiveCount(t, rctx, ss, s) })
 	t.Run("AnalyticsActiveCountForPeriod", func(t *testing.T) { testUserStoreAnalyticsActiveCountForPeriod(t, rctx, ss, s) })
 	t.Run("AnalyticsGetInactiveUsersCount", func(t *testing.T) { testUserStoreAnalyticsGetInactiveUsersCount(t, rctx, ss) })
+	t.Run("AnalyticsGetInactiveUsersCountIgnoreBots", func(t *testing.T) { testUserStoreAnalyticsGetInactiveUsersCountIgnoreBots(t, rctx, ss) })
 	t.Run("AnalyticsGetSystemAdminCount", func(t *testing.T) { testUserStoreAnalyticsGetSystemAdminCount(t, rctx, ss) })
 	t.Run("AnalyticsGetGuestCount", func(t *testing.T) { testUserStoreAnalyticsGetGuestCount(t, rctx, ss) })
 	t.Run("AnalyticsGetExternalUsers", func(t *testing.T) { testUserStoreAnalyticsGetExternalUsers(t, rctx, ss) })
@@ -4472,23 +4473,57 @@ func testUserStoreAnalyticsGetInactiveUsersCount(t *testing.T, rctx request.CTX,
 	u1.Email = MakeEmail()
 	_, err := ss.User().Save(rctx, u1)
 	require.NoError(t, err)
-	defer func() { require.NoError(t, ss.User().PermanentDelete(rctx, u1.Id)) }()
+	t.Cleanup(func() { require.NoError(t, ss.User().PermanentDelete(rctx, u1.Id)) })
 
 	count, err := ss.User().AnalyticsGetInactiveUsersCount()
 	require.NoError(t, err)
+	require.Equal(t, count, int64(0), "No users should have been inactive yet")
 
 	u2 := &model.User{}
 	u2.Email = MakeEmail()
 	u2.DeleteAt = model.GetMillis()
 	_, err = ss.User().Save(rctx, u2)
 	require.NoError(t, err)
-	defer func() { require.NoError(t, ss.User().PermanentDelete(rctx, u2.Id)) }()
+	t.Cleanup(func() { require.NoError(t, ss.User().PermanentDelete(rctx, u2.Id)) })
 
 	newCount, err := ss.User().AnalyticsGetInactiveUsersCount()
 	require.NoError(t, err)
 	require.Equal(t, count, newCount-1, "Expected 1 more inactive users but found otherwise.")
 }
 
+func testUserStoreAnalyticsGetInactiveUsersCountIgnoreBots(t *testing.T, rctx request.CTX, ss store.Store) {
+	u1 := &model.User{}
+	u1.Email = MakeEmail()
+	_, err := ss.User().Save(rctx, u1)
+	require.NoError(t, err)
+	t.Cleanup(func() { require.NoError(t, ss.User().PermanentDelete(rctx, u1.Id)) })
+
+	count, err := ss.User().AnalyticsGetInactiveUsersCount()
+	require.NoError(t, err)
+
+	u2 := &model.User{}
+	u2.Email = MakeEmail()
+	u2.DeleteAt = model.GetMillis()
+	_, err = ss.User().Save(rctx, u2)
+	require.NoError(t, err)
+
+	t.Cleanup(func() { require.NoError(t, ss.User().PermanentDelete(rctx, u2.Id)) })
+
+	_, nErr := ss.Bot().Save(&model.Bot{
+		UserId:   u2.Id,
+		Username: u2.Username,
+		OwnerId:  u2.Id,
+	})
+	require.NoError(t, nErr)
+	u2.IsBot = true
+
+	t.Cleanup(func() { require.NoError(t, ss.Bot().PermanentDelete(u2.Id)) })
+
+	newCount, err := ss.User().AnalyticsGetInactiveUsersCount()
+	require.NoError(t, err)
+	require.Equal(t, count, newCount, "Expected same inactive users but found otherwise.")
+}
+
 func testUserStoreAnalyticsGetSystemAdminCount(t *testing.T, rctx request.CTX, ss store.Store) {
 	countBefore, err := ss.User().AnalyticsGetSystemAdminCount()
 	require.NoError(t, err)