diff options
author | Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com> | 2024-10-03 15:03:11 +0200 |
---|---|---|
committer | Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com> | 2024-10-30 10:17:03 +0100 |
commit | 9a7ee321faf4052d09657c172264470b5bb7daec (patch) | |
tree | 3ab1565597132386a69837695eb776ae0eabb7b1 | |
parent | 51d8dc76c5582ef0f0129696616eb775ef9eaa7f (diff) | |
download | nextcloud-server-9a7ee321faf4052d09657c172264470b5bb7daec.tar.gz nextcloud-server-9a7ee321faf4052d09657c172264470b5bb7daec.zip |
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 <benjamin.gaussorgues@nextcloud.com>
-rw-r--r-- | lib/private/User/Manager.php | 40 | ||||
-rw-r--r-- | tests/lib/User/ManagerTest.php | 67 |
2 files changed, 97 insertions, 10 deletions
diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index dbfa2f8351c..5e99107e504 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -7,6 +7,7 @@ */ namespace OC\User; +use Doctrine\DBAL\Platforms\OraclePlatform; use OC\Hooks\PublicEmitter; use OC\Memcache\WithLocalCache; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -742,12 +743,15 @@ class Manager extends PublicEmitter implements IUserManager { /** * Gets the list of user ids sorted by lastLogin, from most recent to least recent * - * @param int|null $limit how many users to fetch + * @param int|null $limit how many users to fetch (default: 25, max: 100) * @param int $offset from which offset to fetch * @param string $search search users based on search params * @return list<string> list of user IDs */ public function getLastLoggedInUsers(?int $limit = null, int $offset = 0, string $search = ''): array { + // We can't load all users who already logged in + $limit = min(100, $limit ?: 25); + $connection = \OC::$server->getDatabaseConnection(); $queryBuilder = $connection->getQueryBuilder(); $queryBuilder->selectDistinct('uid') @@ -772,14 +776,36 @@ class Manager extends PublicEmitter implements IUserManager { $queryBuilder->orderBy($queryBuilder->func()->lower('p.configvalue'), 'DESC') ->addOrderBy('uid_lower', 'ASC') ->setFirstResult($offset) - ->setMaxResults($limit); + ->setMaxResults($limit) + ; - $result = $queryBuilder->executeQuery(); - /** @var list<string> $uids */ - $uids = $result->fetchAll(\PDO::FETCH_COLUMN); - $result->closeCursor(); + // Oracle don't want to run ORDER BY on CLOB column + $loginOrder = $connection->getDatabasePlatform() instanceof OraclePlatform + ? $queryBuilder->expr()->castColumn('pref_login.configvalue', IQueryBuilder::PARAM_INT) + : 'pref_login.configvalue'; + $queryBuilder + ->orderBy($loginOrder, 'DESC') + ->addOrderBy($queryBuilder->func()->lower('pref_login.userid'), 'ASC'); + + if ($search !== '') { + $displayNameMatches = $this->searchDisplayName($search); + $matchedUids = array_map(static fn (IUser $u): string => $u->getUID(), $displayNameMatches); + + $queryBuilder + ->leftJoin('pref_login', 'preferences', 'pref_email', $queryBuilder->expr()->andX( + $queryBuilder->expr()->eq('pref_login.userid', 'pref_email.userid'), + $queryBuilder->expr()->eq('pref_email.appid', $queryBuilder->expr()->literal('settings')), + $queryBuilder->expr()->eq('pref_email.configkey', $queryBuilder->expr()->literal('email')), + )) + ->andWhere($queryBuilder->expr()->orX( + $queryBuilder->expr()->in('pref_login.userid', $queryBuilder->createNamedParameter($matchedUids, IQueryBuilder::PARAM_STR_ARRAY)), + )); + } + + /** @var list<string> */ + $list = $queryBuilder->executeQuery()->fetchAll(\PDO::FETCH_COLUMN); - return $uids; + return $list; } private function verifyUid(string $uid, bool $checkDataDirectory = false): bool { diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php index 01592851706..5f8ca6e37f7 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -16,6 +16,7 @@ use OCP\ICache; use OCP\ICacheFactory; use OCP\IConfig; use OCP\IUser; +use OCP\IUserManager; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -579,7 +580,7 @@ class ManagerTest extends TestCase { } public function testCountUsersOnlyDisabled() { - $manager = \OC::$server->getUserManager(); + $manager = \OCP\Server::get(IUserManager::class); // count other users in the db before adding our own $countBefore = $manager->countDisabledUsers(); @@ -604,7 +605,7 @@ class ManagerTest extends TestCase { } public function testCountUsersOnlySeen() { - $manager = \OC::$server->getUserManager(); + $manager = \OCP\Server::get(IUserManager::class); // count other users in the db before adding our own $countBefore = $manager->countSeenUsers(); @@ -630,7 +631,7 @@ class ManagerTest extends TestCase { } public function testCallForSeenUsers() { - $manager = \OC::$server->getUserManager(); + $manager = \OCP\Server::get(IUserManager::class); // count other users in the db before adding our own $count = 0; $function = function (IUser $user) use (&$count) { @@ -663,6 +664,66 @@ class ManagerTest extends TestCase { $user4->delete(); } + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testRecentlyActive(): void { + $config = \OCP\Server::get(IConfig::class); + $manager = \OCP\Server::get(IUserManager::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(['test_active_3', 'test_active_1', 'TEST_ACTIVE_2_FRED', 'test_active_4'], $users); + // - Search, case-insensitive order + $users = $manager->getLastLoggedInUsers(search: 'act'); + $this->assertEquals(['test_active_3', 'test_active_1', 'TEST_ACTIVE_2_FRED', 'test_active_4'], $users); + // - No search with offset + $users = $manager->getLastLoggedInUsers(2, 2); + $this->assertEquals(['TEST_ACTIVE_2_FRED', 'test_active_4'], $users); + // - Case insensitive search (email) + $users = $manager->getLastLoggedInUsers(search: 'active.com'); + $this->assertEquals(['test_active_1', 'TEST_ACTIVE_2_FRED'], $users); + // - Case insensitive search (display name) + $users = $manager->getLastLoggedInUsers(search: 'upper'); + $this->assertEquals(['TEST_ACTIVE_2_FRED'], $users); + // - Case insensitive search (uid) + $users = $manager->getLastLoggedInUsers(search: 'fred'); + $this->assertEquals(['TEST_ACTIVE_2_FRED'], $users); + + // Delete users and config keys + $user1->delete(); + $user2->delete(); + $user3->delete(); + $user4->delete(); + $user5->delete(); + } + public function testDeleteUser() { $config = $this->getMockBuilder(AllConfig::class) ->disableOriginalConstructor() |