upstream/nextcloud
1
0
Fork 0
mirror of https://github.com/nextcloud/server.git synced 2026-02-20 00:12:30 -05:00
Code Issues Projects Releases Packages Wiki Activity Actions 38

fix(users): improve recently active search

Browse source 
- 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>
This commit is contained in:
Benjamin Gaussorgues 2024-10-03 15:03:11 +02:00
parent 34ade098bd
commit 79db082fe6
2 changed files with 101 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

65
lib/private/User/Manager.php
View file

@ -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,52 @@ 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')
->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('pref_login.userid')
->from('preferences', 'pref_login')
->where($queryBuilder->expr()->eq('pref_login.appid', $queryBuilder->expr()->literal('login')))
->andWhere($queryBuilder->expr()->eq('pref_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();
// 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');
return $uids;
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 $list;
}
private function verifyUid(string $uid, bool $checkDataDirectory = false): bool {

67
tests/lib/User/ManagerTest.php
View file

@ -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(): 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 @@ class ManagerTest extends TestCase {
}
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 @@ class ManagerTest extends TestCase {
}
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 @@ 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(): void {
$config = $this->getMockBuilder(AllConfig::class)
->disableOriginalConstructor()