feat: add ability to sort by last login #45249
Conversation
…e of these needs to be passed as orderBy and sort can be ASC or DESC Signed-off-by: yemkareems <yemkareems@gmail.com>
… desc. Also last_login sort logic changed Signed-off-by: yemkareems <yemkareems@gmail.com>
…rder is lastLogin DESC Signed-off-by: yemkareems <yemkareems@gmail.com>
Signed-off-by: yemkareems <yemkareems@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -189,7 +191,7 @@ | |||
|
|
|||
| $users = []; | |||
| foreach ($subAdminOfGroups as $group) { | |||
| $users[] = array_keys($this->groupManager->displayNamesInGroup($group, $search, $limit, $offset)); | |||
| $users[] = array_keys($this->groupManager->displayNamesInGroup($group, $search, $limit, $offset, $orderBy, $sort)); | |||
Check notice
Code scanning / Psalm
PossiblyInvalidArgument Note
| @@ -189,7 +191,7 @@ | |||
|
|
|||
| $users = []; | |||
| foreach ($subAdminOfGroups as $group) { | |||
| $users[] = array_keys($this->groupManager->displayNamesInGroup($group, $search, $limit, $offset)); | |||
| $users[] = array_keys($this->groupManager->displayNamesInGroup($group, $search, $limit, $offset, $orderBy, $sort)); | |||
Check notice
Code scanning / Psalm
PossiblyNullArgument Note
| @@ -130,15 +130,15 @@ public function __construct( | |||
| * | |||
| * 200: Users returned | |||
| */ | |||
| public function getUsers(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { | |||
| public function getUsers(string $search = '', ?int $limit = null, int $offset = 0, string $orderBy = 'lastLogin', string $sort = 'DESC'): DataResponse { | |||
There was a problem hiding this comment.
| public function getUsers(string $search = '', ?int $limit = null, int $offset = 0, string $orderBy = 'lastLogin', string $sort = 'DESC'): DataResponse { | |
| public function getUsers(string $search = '', ?int $limit = null, int $offset = 0, string $orderBy = 'uid', string $sort = 'ASC'): DataResponse { |
We should not break the existing sort order
| @@ -167,19 +167,21 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = | |||
| * @param string $search Text to search for | |||
| * @param int|null $limit Limit the amount of groups returned | |||
| * @param int $offset Offset for searching for groups | |||
| * @param string $orderBy Field to order the results with | |||
| * @param string $sort ASC or DESC | |||
There was a problem hiding this comment.
| * @param string $sort ASC or DESC | |
| * @param string $sort asc or desc |
Would use lowercase like our other API endpoints
| * @return DataResponse<Http::STATUS_OK, array{users: array<string, Provisioning_APIUserDetails|array{id: string}>}, array{}> | ||
| * | ||
| * 200: Users details returned | ||
| */ | ||
| public function getUsersDetails(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { | ||
| public function getUsersDetails(string $search = '', ?int $limit = null, int $offset = 0, string $orderBy = 'lastLogin', string $sort = 'DESC'): DataResponse { |
There was a problem hiding this comment.
| public function getUsersDetails(string $search = '', ?int $limit = null, int $offset = 0, string $orderBy = 'lastLogin', string $sort = 'DESC'): DataResponse { | |
| public function getUsersDetails(string $search = '', ?int $limit = null, int $offset = 0, string $orderBy = 'uid', string $sort = 'asc'): DataResponse { |
Same to not break the sort order + lowercase sort param
kesselb
added
2. developing
| @@ -126,7 +126,7 @@ public function checkPassword($loginName, $password); | |||
| * @return \OCP\IUser[] | |||
| * @since 8.0.0 | |||
| */ | |||
| public function search($pattern, $limit = null, $offset = null); | |||
| public function search($pattern, $limit = null, $offset = null, $orderBy = 'uid', $sort = 'ASC'); | |||
There was a problem hiding this comment.
Everything in lib/public also known as OCP is our public api. We try to keep changes as low as possible to keep our api stable for our app developers (internal and external).
There are a couple of apps implementing IUserBackend (e.g. ldap, user_saml, guests, just to name a few). What we usually do if we need to add a new functionality is to add a new interface. Then every user backend can signal that the given functionality is available. Example: https://github.com/nextcloud/server/pull/34443/files#diff-06d89144465282e8807eac44163df0436a6beca7eeda0d988988f3688ae1c9c6
Please discuss this with Stephan and Côme. I guess if adding a new search method is not feasible, we have to adjust the signature here, but that should be planned so that all our apps are ready for 30.
Changing interfaces is tricky, especially when apps are not updated properly during an upgrade. We had such issues in the past. A class not implementing an interface properly is causing a PHP Fatal error.
Switching to this branch with user_ldap enabled:
PHP Fatal error: Declaration of OCA\User_LDAP\User_Proxy::getDisplayNames($search = '', $limit = null, $offset = null) must be compatible with OCP\UserInterface::getDisplayNames($search = '', $limit = null, $offset = null, string $orderBy = 'lastLogin', string $sort = 'DESC')
| $limit = $this->fixLimit($limit); | ||
|
|
||
| $this->fixDI(); | ||
|
|
||
| $query = $this->dbConn->getQueryBuilder(); | ||
|
|
||
| $appId = 'settings'; | ||
| $configKey = 'email'; | ||
| if($orderBy == 'lastLogin') { |
There was a problem hiding this comment.
| if($orderBy == 'lastLogin') { | |
| if($orderBy === 'lastLogin') { |
| $query->select('uid', 'displayname') | ||
| ->from($this->table, 'u') | ||
| ->leftJoin('u', 'preferences', 'p', $query->expr()->andX( | ||
| $query->expr()->eq('userid', 'uid'), | ||
| $query->expr()->eq('appid', $query->expr()->literal('settings')), | ||
| $query->expr()->eq('configkey', $query->expr()->literal('email'))) | ||
| $query->expr()->eq('appid', $query->expr()->literal($appId)), |
There was a problem hiding this comment.
This change breaks searching by email.
- Open user management
- Click on the search icon on the upper right next to your profile picture
- Enter an email address of a user
=> No result on this branch
The left join is there to look up a user by the email address.
A subselect for lastLogin (similar to the preferences join) should work.
There was a problem hiding this comment.
Index: lib/private/User/Database.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/private/User/Database.php b/lib/private/User/Database.php
--- a/lib/private/User/Database.php (revision 1b86b6c7f67d1fbc2936a76289e93df2a6e63445)
+++ b/lib/private/User/Database.php (date 1715447859568)
@@ -275,25 +275,32 @@
$query = $this->dbConn->getQueryBuilder();
- $appId = 'settings';
- $configKey = 'email';
- if($orderBy == 'lastLogin') {
- $appId = 'login';
- $configKey = 'lastLogin';
+ if ($orderBy === 'lastLogin') {
+ $lastLoginSubSelect = $this->dbConn->getQueryBuilder();
+ $lastLoginSubSelect->select('configvalue')
+ ->from('preferences', 'p2')
+ ->where($lastLoginSubSelect->expr()->andX(
+ $lastLoginSubSelect->expr()->eq('p2.userid', 'uid'),
+ $lastLoginSubSelect->expr()->eq('p2.appid', $lastLoginSubSelect->expr()->literal('login')),
+ $lastLoginSubSelect->expr()->eq('p2.configkey', $lastLoginSubSelect->expr()->literal('lastLogin')),
+ ));
+ $orderByExpression = $query->createFunction('(' . $lastLoginSubSelect->getSQL() .')');
+ } else {
+ $orderByExpression = $query->func()->lower('displayname');
}
- $query->select('uid', 'displayname')
+ $query->select('uid', 'displayname', $orderByExpression)
->from($this->table, 'u')
->leftJoin('u', 'preferences', 'p', $query->expr()->andX(
$query->expr()->eq('userid', 'uid'),
- $query->expr()->eq('appid', $query->expr()->literal($appId)),
- $query->expr()->eq('configkey', $query->expr()->literal($configKey)))
+ $query->expr()->eq('appid', $query->expr()->literal('settings')),
+ $query->expr()->eq('configkey', $query->expr()->literal('email')))
)
// sqlite doesn't like re-using a single named parameter here
->where($query->expr()->iLike('uid', $query->createPositionalParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')))
->orWhere($query->expr()->iLike('displayname', $query->createPositionalParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')))
->orWhere($query->expr()->iLike('configvalue', $query->createPositionalParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')))
- ->orderBy($query->func()->lower('configvalue'), $sort)
+ ->orderBy($orderByExpression, $sort)
->addOrderBy('uid_lower', 'ASC')
->setMaxResults($limit)
->setFirstResult($offset);
There was a problem hiding this comment.
If getDisplayNames is called from getUsers (the route the user management xhr request is using) the list is sorted again in:
server/lib/private/User/Database.php
Line 428 in 4560ddd
| if (is_array($backendUsers)) { | ||
| foreach ($backendUsers as $uid) { | ||
| $users[$uid] = new LazyUser($uid, $this, null, $backend); | ||
| } | ||
| } | ||
| } | ||
| switch ($orderBy.' '.$sort) { |
There was a problem hiding this comment.
Sorting here only works with $limit and $offset null, for paginated requests the result will be wrong.
| usort($users, function (IUser $a, IUser $b) { | ||
| return strcasecmp($a->getDisplayName(), $b->getDisplayName()); | ||
| }); | ||
| switch ($orderBy.' '.$sort) { |
There was a problem hiding this comment.
Sorting here only works with $limit and $offset null, for paginated requests the result will be wrong.
Summary
TODO
Checklist