Skip to content

Commit

Permalink
fix(users): improve recently active search
Browse files Browse the repository at this point in the history 
- 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>
  • Loading branch information
@Altahrim
Altahrim committed Oct 24, 2024
1 parent c588d46 commit 5cce50a
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 31 deletions.
65 changes: 37 additions & 28 deletions lib/private/User/Manager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -742,44 +743,52 @@ 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('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');

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 {
Expand Down
67 changes: 64 additions & 3 deletions tests/lib/User/ManagerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use OCP\ICacheFactory;
use OCP\IConfig;
use OCP\IUser;
use OCP\IUserManager;
use Psr\Log\LoggerInterface;
use Test\TestCase;

Expand Down Expand Up @@ -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();

Expand All @@ -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();

Expand All @@ -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) {
Expand Down Expand Up @@ -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('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()
Expand Down

0 comments on commit 5cce50a

Please sign in to comment.