Fixed disabled users query #9261
Conversation
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Codecov Report
@@ Coverage Diff @@
## master #9261 +/- ##
============================================
+ Coverage 51.95% 58.07% +6.12%
============================================
Files 1607 67 -1540
Lines 95393 7943 -87450
Branches 1394 1394
============================================
- Hits 49559 4613 -44946
+ Misses 45834 3330 -42504 |
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
skjnldsv
force-pushed
the
disabled-users-by-group-fix
branch
from
f57d65f
to
c7aabd2
Compare
skjnldsv
added
bug
3. to review
|
Tests ok, ready to review :) |
lib/private/User/Manager.php
Outdated
| * @return int | ||
| * @since 14.0.0 | ||
| */ | ||
| public function countDisabledUsersOfGroups(array $groups) { |
There was a problem hiding this comment.
Also give a hint for the return type here? : int ;)
lib/public/IUserManager.php
Outdated
| * @return int | ||
| * @since 14.0.0 | ||
| */ | ||
| public function countDisabledUsersOfGroups(array $groups); |
There was a problem hiding this comment.
Same here - type hint for the return type would be nice.
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
skjnldsv
force-pushed
the
disabled-users-by-group-fix
branch
from
c7aabd2
to
d81ac13
Compare
lib/private/User/Manager.php
Outdated
| $queryBuilder = \OC::$server->getDatabaseConnection()->getQueryBuilder(); | ||
| $queryBuilder->select($queryBuilder->createFunction('COUNT(Distinct uid)')) | ||
| ->from('preferences', 'p') | ||
| ->innerJoin('p', 'group_user', 'g', 'p.userid = g.uid') |
There was a problem hiding this comment.
This hard codes the user manager to our group backend ❌
lib/private/User/Manager.php
Outdated
| ->where($queryBuilder->expr()->eq('appid', $queryBuilder->createNamedParameter('core'))) | ||
| ->andWhere($queryBuilder->expr()->eq('configkey', $queryBuilder->createNamedParameter('enabled'))) | ||
| ->andWhere($queryBuilder->expr()->eq('configvalue', $queryBuilder->createNamedParameter('false'), IQueryBuilder::PARAM_STR)) | ||
| ->andWhere($queryBuilder->expr()->in('gid', $queryBuilder->createNamedParameter(implode(',', $groups)))); |
There was a problem hiding this comment.
no need to implode, $queryBuilder->createNamedParameter($groups, IQueryBuilder::PARAM_STR_ARRAY) has you covered
lib/public/IUserManager.php
Outdated
| */ | ||
| public function countDisabledUsers(); | ||
| public function countDisabledUsersOfGroups(array $groups):int; |
There was a problem hiding this comment.
Could you add a space before the int? That's how we use it in all other places in our code.
| * | ||
| * @return int | ||
| * @since 11.0.0 | ||
| * @since 12.0.0 |
There was a problem hiding this comment.
it was an error, implemented in 12 in the private fct but in 11 in the public interface :)
I guess a copy/paste error
skjnldsv
added
2. developing
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
| @@ -39,6 +39,7 @@ | |||
| self::ADD_TO_GROUP => 'addToGroup', | |||
| self::REMOVE_FROM_GOUP => 'removeFromGroup', | |||
| self::COUNT_USERS => 'countUsersInGroup', | |||
| self::COUNT_DISABLED => 'countDisabledInGroup', | |||
There was a problem hiding this comment.
Yeah, we discussed it with @rullzer and @nickvergessen.
Unfortunately
server/lib/public/GroupInterface.php
Lines 48 to 59 in da6c2c9
There was a problem hiding this comment.
We should go the interface way 😉 see #8935
There was a problem hiding this comment.
that was also the conclusion in IRC ;)
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
|
Let's close this in favor of a new one |
Fix #8009
Fix #8008
I feel like it's safer to return a different entry instead of directly subtracting the disabled users to the total user count. People might want to get the full data anyway.