-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enable api getUsers for subadmins #18399
Conversation
} else { | ||
$subAdminOfGroups = \OC_SubAdmin::getSubAdminsGroups($user->getUID()); | ||
|
||
if($limit === null) { $limit = -1; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put the statement on a new line.
Beside my comments this looks good. |
cc @DeepDiver1975 Provisioning API + SubAdmin ❓ |
cc @tomneedham |
} | ||
$batch = []; | ||
|
||
$groupUsers = OC_Group::displayNamesInGroups($subAdminOfGroups, $search, $limit, $offset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No... I just spend some time to get rid of most of the OC_* calls in the provisioning api.
Just use a simple loop and use $this->groupManager->displayNamesInGroups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rullzer But if I use this in a loop than limit and offset are not used as expected. If the subadmin uses limit=2 and has two groups to manage he gets 4 results.
Please fix the test case |
Addition does make a lot of sense! Thanks @michag86. |
@rullzer The OC_Subadmin stuff isn't available in any proper implementation 😢 |
Well that is known. But the provisioning API is already using the calls currently. So I do not really see an issue with that. It is far from ideal, I admit that. |
4acbfb5
to
15b5107
Compare
Rebased and squashed. |
Issue for displayNamesInGroups in #18418 |
Now subadmins can get a list of users they are subadmins of.
* Test pass again * Code coverage getUsers is at 100% again
15b5107
to
ef3aa12
Compare
A new inspection was created. |
So oracle is fine now. Let the reviews begin! |
@@ -68,7 +69,31 @@ public function getUsers(){ | |||
$limit = !empty($_GET['limit']) ? $_GET['limit'] : null; | |||
$offset = !empty($_GET['offset']) ? $_GET['offset'] : null; | |||
|
|||
$users = $this->userManager->search($search, $limit, $offset); | |||
// Check if user is logged in | |||
$user = $this->userSession->getUser(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already done by the API::SUBADMIN_AUTH
?
Tested and works 👍 CURL command: |
We're already past feature freeze unfortunately. @MTRichards @cmonteroluque can we get this useful addition get scheduled for 9.0 ? |
Will link it for consideration |
CC @SergioBertolinSG for additional provisioning API API-level tests |
Tested, works 👍 |
@SergioBertolinSG please add an integration test to test this behavior - THX |
enable api getUsers for subadmins
Make it possible to use getUsers from provisioning api for subadmins.