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 <[email protected]>
nextcloud · Oct 16, 2024 · 157fd45 · 157fd45
1 parent f17c422
commit 157fd45
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 31 deletions.
diff --git 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,44 +743,51 @@ public function validateUserId(string $uid, bool $checkDataDirectory = false): v
 	/**
 	 * 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')
-			->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')))
 			->setFirstResult($offset)
-			->setMaxResults($limit);
+			->setMaxResults($limit)
+		;
 
-		$result = $queryBuilder->executeQuery();
-		/** @var list<string> $uids */
-		$uids = $result->fetchAll(\PDO::FETCH_COLUMN);
-		$result->closeCursor();
+		if ($connection->getDatabasePlatform() instanceof OraclePlatform) {
+			// Oracle don't want to run ORDER BY on CLOB column
+			$queryBuilder->orderBy($queryBuilder->expr()->castColumn('login.configvalue', IQueryBuilder::PARAM_INT), 'DESC');
+		} else {
+			$queryBuilder->orderBy('login.configvalue', 'DESC');
+		}
+
+		if ($search !== '') {
+			$displayNameMatches = $this->searchDisplayName($search);
+			$matchedUids = array_map(static fn (IUser $u): string => $u->getUID(), $displayNameMatches);
+
+			$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')),
+				))
+				->andWhere($queryBuilder->expr()->orX(
+					$queryBuilder->expr()->in('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
@@ -16,6 +16,7 @@
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IUser;
+use OCP\IUserManager;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
@@ -579,7 +580,7 @@ public function testCountUsersTwoBackends(): void {
 	}
 
 	public function testCountUsersOnlyDisabled(): void {
-		$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 @@ public function testCountUsersOnlyDisabled(): void {
 	}
 
 	public function testCountUsersOnlySeen(): void {
-		$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 @@ public function testCountUsersOnlySeen(): void {
 	}
 
 	public function testCallForSeenUsers(): void {
-		$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 @@ public function testCallForSeenUsers(): void {
 		$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('[email protected]');
+
+		$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('[email protected]');
+
+		$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('[email protected]');
+
+		// 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(): void {
 		$config = $this->getMockBuilder(AllConfig::class)
 			->disableOriginalConstructor()