Skip to content

Commit 157fd45

Browse files
AltahrimAltahrim
committed
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]>
1 parent f17c422 commit 157fd45

File tree

2 files changed

+100
-31
lines changed

2 files changed

+100
-31
lines changed

‎lib/private/User/Manager.php‎

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
*/
88
namespace OC\User;
99

10+
use Doctrine\DBAL\Platforms\OraclePlatform;
1011
use OC\Hooks\PublicEmitter;
1112
use OC\Memcache\WithLocalCache;
1213
use OCP\DB\QueryBuilder\IQueryBuilder;
@@ -742,44 +743,51 @@ public function validateUserId(string $uid, bool $checkDataDirectory = false): v
742743
/**
743744
* Gets the list of user ids sorted by lastLogin, from most recent to least recent
744745
*
745-
* @param int|null $limit how many users to fetch
746+
* @param int|null $limit how many users to fetch (default: 25, max: 100)
746747
* @param int $offset from which offset to fetch
747748
* @param string $search search users based on search params
748749
* @return list<string> list of user IDs
749750
*/
750751
public function getLastLoggedInUsers(?int $limit = null, int $offset = 0, string $search = ''): array {
752+
// We can't load all users who already logged in
753+
$limit = min(100, $limit ?: 25);
754+
751755
$connection = \OC::$server->getDatabaseConnection();
752756
$queryBuilder = $connection->getQueryBuilder();
753-
$queryBuilder->selectDistinct('uid')
754-
->from('users', 'u')
755-
->leftJoin('u', 'preferences', 'p', $queryBuilder->expr()->andX(
756-
$queryBuilder->expr()->eq('p.userid', 'uid'),
757-
$queryBuilder->expr()->eq('p.appid', $queryBuilder->expr()->literal('login')),
758-
$queryBuilder->expr()->eq('p.configkey', $queryBuilder->expr()->literal('lastLogin')))
759-
);
760-
if ($search !== '') {
761-
$queryBuilder->leftJoin('u', 'preferences', 'p1', $queryBuilder->expr()->andX(
762-
$queryBuilder->expr()->eq('p1.userid', 'uid'),
763-
$queryBuilder->expr()->eq('p1.appid', $queryBuilder->expr()->literal('settings')),
764-
$queryBuilder->expr()->eq('p1.configkey', $queryBuilder->expr()->literal('email')))
765-
)
766-
// sqlite doesn't like re-using a single named parameter here
767-
->where($queryBuilder->expr()->iLike('uid', $queryBuilder->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%')))
768-
->orWhere($queryBuilder->expr()->iLike('displayname', $queryBuilder->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%')))
769-
->orWhere($queryBuilder->expr()->iLike('p1.configvalue', $queryBuilder->createPositionalParameter('%' . $connection->escapeLikeParameter($search) . '%'))
770-
);
771-
}
772-
$queryBuilder->orderBy($queryBuilder->func()->lower('p.configvalue'), 'DESC')
773-
->addOrderBy('uid_lower', 'ASC')
757+
$queryBuilder->select('login.userid')
758+
->from('preferences', 'login')
759+
->where($queryBuilder->expr()->eq('login.appid', $queryBuilder->expr()->literal('login')))
760+
->andWhere($queryBuilder->expr()->eq('login.configkey', $queryBuilder->expr()->literal('lastLogin')))
774761
->setFirstResult($offset)
775-
->setMaxResults($limit);
762+
->setMaxResults($limit)
763+
;
776764

777-
$result = $queryBuilder->executeQuery();
778-
/** @var list<string> $uids */
779-
$uids = $result->fetchAll(\PDO::FETCH_COLUMN);
780-
$result->closeCursor();
765+
if ($connection->getDatabasePlatform() instanceof OraclePlatform) {
766+
// Oracle don't want to run ORDER BY on CLOB column
767+
$queryBuilder->orderBy($queryBuilder->expr()->castColumn('login.configvalue', IQueryBuilder::PARAM_INT), 'DESC');
768+
} else {
769+
$queryBuilder->orderBy('login.configvalue', 'DESC');
770+
}
771+
772+
if ($search !== '') {
773+
$displayNameMatches = $this->searchDisplayName($search);
774+
$matchedUids = array_map(static fn (IUser $u): string => $u->getUID(), $displayNameMatches);
775+
776+
$queryBuilder
777+
->leftJoin('login', 'preferences', 'email', $queryBuilder->expr()->andX(
778+
$queryBuilder->expr()->eq('login.userid', 'email.userid'),
779+
$queryBuilder->expr()->eq('email.appid', $queryBuilder->expr()->literal('settings')),
780+
$queryBuilder->expr()->eq('email.configkey', $queryBuilder->expr()->literal('email')),
781+
))
782+
->andWhere($queryBuilder->expr()->orX(
783+
$queryBuilder->expr()->in('login.userid', $queryBuilder->createNamedParameter($matchedUids, IQueryBuilder::PARAM_STR_ARRAY)),
784+
));
785+
}
786+
787+
/** @var list<string> */
788+
$list = $queryBuilder->executeQuery()->fetchAll(\PDO::FETCH_COLUMN);
781789

782-
return $uids;
790+
return $list;
783791
}
784792

785793
private function verifyUid(string $uid, bool $checkDataDirectory = false): bool {

‎tests/lib/User/ManagerTest.php‎

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use OCP\ICacheFactory;
1717
use OCP\IConfig;
1818
use OCP\IUser;
19+
use OCP\IUserManager;
1920
use Psr\Log\LoggerInterface;
2021
use Test\TestCase;
2122

@@ -579,7 +580,7 @@ public function testCountUsersTwoBackends(): void {
579580
}
580581

581582
public function testCountUsersOnlyDisabled(): void {
582-
$manager = \OC::$server->getUserManager();
583+
$manager = \OCP\Server::get(IUserManager::class);
583584
// count other users in the db before adding our own
584585
$countBefore = $manager->countDisabledUsers();
585586

@@ -604,7 +605,7 @@ public function testCountUsersOnlyDisabled(): void {
604605
}
605606

606607
public function testCountUsersOnlySeen(): void {
607-
$manager = \OC::$server->getUserManager();
608+
$manager = \OCP\Server::get(IUserManager::class);
608609
// count other users in the db before adding our own
609610
$countBefore = $manager->countSeenUsers();
610611

@@ -630,7 +631,7 @@ public function testCountUsersOnlySeen(): void {
630631
}
631632

632633
public function testCallForSeenUsers(): void {
633-
$manager = \OC::$server->getUserManager();
634+
$manager = \OCP\Server::get(IUserManager::class);
634635
// count other users in the db before adding our own
635636
$count = 0;
636637
$function = function (IUser $user) use (&$count) {
@@ -663,6 +664,66 @@ public function testCallForSeenUsers(): void {
663664
$user4->delete();
664665
}
665666

667+
/**
668+
* @runInSeparateProcess
669+
* @preserveGlobalState disabled
670+
*/
671+
public function testRecentlyActive(): void {
672+
$config = \OCP\Server::get(IConfig::class);
673+
$manager = \OCP\Server::get(IUserManager::class);
674+
675+
// Create some users
676+
$now = (string)time();
677+
$user1 = $manager->createUser('test_active_1', 'test_active_1');
678+
$config->setUserValue('test_active_1', 'login', 'lastLogin', $now);
679+
$user1->setDisplayName('test active 1');
680+
$user1->setSystemEMailAddress('[email protected]');
681+
682+
$user2 = $manager->createUser('TEST_ACTIVE_2_FRED', 'TEST_ACTIVE_2');
683+
$config->setUserValue('TEST_ACTIVE_2_FRED', 'login', 'lastLogin', $now);
684+
$user2->setDisplayName('TEST ACTIVE 2 UPPER');
685+
$user2->setSystemEMailAddress('[email protected]');
686+
687+
$user3 = $manager->createUser('test_active_3', 'test_active_3');
688+
$config->setUserValue('test_active_3', 'login', 'lastLogin', $now + 1);
689+
$user3->setDisplayName('test active 3');
690+
691+
$user4 = $manager->createUser('test_active_4', 'test_active_4');
692+
$config->setUserValue('test_active_4', 'login', 'lastLogin', $now);
693+
$user4->setDisplayName('Test Active 4');
694+
695+
$user5 = $manager->createUser('test_inactive_1', 'test_inactive_1');
696+
$user5->setDisplayName('Test Inactive 1');
697+
$user2->setSystemEMailAddress('[email protected]');
698+
699+
// Search recently active
700+
// - No search, case-insensitive order
701+
$users = $manager->getLastLoggedInUsers(4);
702+
$this->assertEquals(['test_active_3', 'test_active_1', 'TEST_ACTIVE_2_FRED', 'test_active_4'], $users);
703+
// - Search, case-insensitive order
704+
$users = $manager->getLastLoggedInUsers(search: 'act');
705+
$this->assertEquals(['test_active_3', 'test_active_1', 'TEST_ACTIVE_2_FRED', 'test_active_4'], $users);
706+
// - No search with offset
707+
$users = $manager->getLastLoggedInUsers(2, 2);
708+
$this->assertEquals(['TEST_ACTIVE_2_FRED', 'test_active_4'], $users);
709+
// - Case insensitive search (email)
710+
$users = $manager->getLastLoggedInUsers(search: 'active.com');
711+
$this->assertEquals(['test_active_1', 'TEST_ACTIVE_2_FRED'], $users);
712+
// - Case insensitive search (display name)
713+
$users = $manager->getLastLoggedInUsers(search: 'upper');
714+
$this->assertEquals(['TEST_ACTIVE_2_FRED'], $users);
715+
// - Case insensitive search (uid)
716+
$users = $manager->getLastLoggedInUsers(search: 'fred');
717+
$this->assertEquals(['TEST_ACTIVE_2_FRED'], $users);
718+
719+
// Delete users and config keys
720+
$user1->delete();
721+
$user2->delete();
722+
$user3->delete();
723+
$user4->delete();
724+
$user5->delete();
725+
}
726+
666727
public function testDeleteUser(): void {
667728
$config = $this->getMockBuilder(AllConfig::class)
668729
->disableOriginalConstructor()

0 commit comments

Comments
 (0)