aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorBenjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>2024-10-03 15:03:11 +0200
committerBenjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>2024-10-30 10:17:03 +0100
commit9a7ee321faf4052d09657c172264470b5bb7daec (patch)
tree3ab1565597132386a69837695eb776ae0eabb7b1
parent51d8dc76c5582ef0f0129696616eb775ef9eaa7f (diff)
downloadnextcloud-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.php40
-rw-r--r--tests/lib/User/ManagerTest.php67
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()