From c4ce15aa4a3dbda8d623a9f66ee9689f1540f359 Mon Sep 17 00:00:00 2001 From: Benjamin Gaussorgues Date: Thu, 3 Oct 2024 15:03:11 +0200 Subject: [PATCH] fix(users): improve recently active search - Remove DISTINCT clause to fix PgSQL - Join user table only if necessary - Don't show people who never connected in active list - Add test Signed-off-by: Benjamin Gaussorgues --- lib/private/User/Manager.php | 43 +++++++++++++------------- tests/lib/User/ManagerTest.php | 56 ++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 21 deletions(-) diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 3ee748a277577..a2f622dcbe839 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -750,30 +750,31 @@ public function validateUserId(string $uid, bool $checkDataDirectory = false): v public function getLastLoggedInUsers(?int $limit = null, int $offset = 0, string $search = ''): array { $connection = \OC::$server->getDatabaseConnection(); $queryBuilder = $connection->getQueryBuilder(); - $queryBuilder->selectDistinct('uid') - ->from('users', 'u') - ->leftJoin('u', 'preferences', 'p', $queryBuilder->expr()->andX( - $queryBuilder->expr()->eq('p.userid', 'uid'), - $queryBuilder->expr()->eq('p.appid', $queryBuilder->expr()->literal('login')), - $queryBuilder->expr()->eq('p.configkey', $queryBuilder->expr()->literal('lastLogin'))) - ); - if ($search !== '') { - $queryBuilder->leftJoin('u', 'preferences', 'p1', $queryBuilder->expr()->andX( - $queryBuilder->expr()->eq('p1.userid', 'uid'), - $queryBuilder->expr()->eq('p1.appid', $queryBuilder->expr()->literal('settings')), - $queryBuilder->expr()->eq('p1.configkey', $queryBuilder->expr()->literal('email'))) - ) - // sqlite doesn't like re-using a single named parameter here - ->where($queryBuilder->expr()->iLike('uid', $queryBuilder->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%'))) - ->orWhere($queryBuilder->expr()->iLike('displayname', $queryBuilder->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%'))) - ->orWhere($queryBuilder->expr()->iLike('p1.configvalue', $queryBuilder->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%')) - ); - } - $queryBuilder->orderBy($queryBuilder->func()->lower('p.configvalue'), 'DESC') - ->addOrderBy('uid_lower', 'ASC') + $queryBuilder->select('login.userid') + ->from('preferences', 'login') + ->where($queryBuilder->expr()->eq('login.appid', $queryBuilder->expr()->literal('login'))) + ->andWhere($queryBuilder->expr()->eq('login.configkey', $queryBuilder->expr()->literal('lastLogin'))) + ->orderBy('login.configvalue', 'DESC') ->setFirstResult($offset) ->setMaxResults($limit); + if ($search === '') { + $queryBuilder->addOrderBy($queryBuilder->func()->lower('userid'), 'ASC'); + } else { + $queryBuilder->leftJoin('login', 'preferences', 'email', $queryBuilder->expr()->andX( + $queryBuilder->expr()->eq('login.userid', 'email.userid'), + $queryBuilder->expr()->eq('email.appid', $queryBuilder->expr()->literal('settings')), + $queryBuilder->expr()->eq('email.configkey', $queryBuilder->expr()->literal('email')), + )); + $queryBuilder->join('login', 'users', 'user', $queryBuilder->expr()->eq('login.userid', 'user.uid')); + $queryBuilder->andWhere($queryBuilder->expr()->orX( + $queryBuilder->expr()->iLike('uid', $queryBuilder->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%')), + $queryBuilder->expr()->iLike('displayname', $queryBuilder->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%')), + $queryBuilder->expr()->iLike('email.configvalue', $queryBuilder->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%')), + )); + $queryBuilder->addOrderBy('uid_lower', 'ASC'); + } + $result = $queryBuilder->executeQuery(); /** @var list $uids */ $uids = $result->fetchAll(\PDO::FETCH_COLUMN); diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php index 0adfa049e2cce..cb4d19279193a 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -663,6 +663,62 @@ public function testCallForSeenUsers(): void { $user4->delete(); } + public function testRecentlyActive(): void { + $manager = \OC::$server->getUserManager(); + $config = \OC::$server->get(IConfig::class); + + // Create some users + $now = (string)time(); + $user1 = $manager->createUser('test_active_1', 'test_active_1'); + $config->setUserValue('test_active_1', 'login', 'lastLogin', $now); + $user1->setDisplayName('test active 1'); + $user1->setSystemEMailAddress('roger@active.com'); + + $user2 = $manager->createUser('TEST_ACTIVE_2_FRED', 'TEST_ACTIVE_2'); + $config->setUserValue('TEST_ACTIVE_2_FRED', 'login', 'lastLogin', $now); + $user2->setDisplayName('TEST ACTIVE 2 UPPER'); + $user2->setSystemEMailAddress('Fred@Active.Com'); + + $user3 = $manager->createUser('test_active_3', 'test_active_3'); + $config->setUserValue('test_active_3', 'login', 'lastLogin', $now + 1); + $user3->setDisplayName('test active 3'); + + $user4 = $manager->createUser('test_active_4', 'test_active_4'); + $config->setUserValue('test_active_4', 'login', 'lastLogin', $now); + $user4->setDisplayName('Test Active 4'); + + $user5 = $manager->createUser('test_inactive_1', 'test_inactive_1'); + $user5->setDisplayName('Test Inactive 1'); + $user2->setSystemEMailAddress('jeanne@Active.Com'); + + // Search recently active + // - No search, case-insensitive order + $users = $manager->getLastLoggedInUsers(4); + $this->assertEquals($users, ['test_active_3', 'test_active_1', 'TEST_ACTIVE_2_FRED', 'test_active_4']); + // - Search, case-insensitive order + $users = $manager->getLastLoggedInUsers(search: 'act'); + $this->assertEquals($users, ['test_active_3', 'test_active_1', 'TEST_ACTIVE_2_FRED', 'test_active_4']); + // - No search with offset + $users = $manager->getLastLoggedInUsers(2, 2); + $this->assertEquals($users, ['TEST_ACTIVE_2_FRED', 'test_active_4']); + // - Case insensitive search (email) + $users = $manager->getLastLoggedInUsers(search: 'active.com'); + $this->assertEquals($users, ['test_active_1', 'TEST_ACTIVE_2_FRED']); + // - Case insensitive search (display name) + $users = $manager->getLastLoggedInUsers(search: 'upper'); + $this->assertEquals($users, ['TEST_ACTIVE_2_FRED']); + // - Case insensitive search (uid) + $users = $manager->getLastLoggedInUsers(search: 'fred'); + $this->assertEquals($users, ['TEST_ACTIVE_2_FRED']); + + // Delete users and config keys + $user1->delete(); + $user2->delete(); + $user3->delete(); + $user4->delete(); + $user5->delete(); + } + public function testDeleteUser(): void { $config = $this->getMockBuilder(AllConfig::class) ->disableOriginalConstructor()