Api: return users details by groups by skjnldsv · Pull Request #8904 · nextcloud/server

skjnldsv · 2018-03-20T18:08:21Z

This allows to get the users detailed data directly by requesting the groupid

$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details').then((response)=>console.log(response))$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details?limit=10').then((response)=>console.log(response))$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details?limit=2&offset=2').then((response)=>console.log(response))

nickvergessen · 2018-03-22T09:50:15Z

+ // Check subadmin has access to this group
+ if($this->groupManager->isAdmin($user->getUID())
+ || $isSubadminOfGroup) {
+ $users = $this->groupManager->get($groupId)->getUsers();


you already got the group in 210,
also here the null check is missing.

nickvergessen · 2018-03-22T09:51:13Z

+ $user = $this->userSession->getUser();
+
+ // Check the group exists
+ if(!$this->groupManager->groupExists($groupId)) {


I suggest to get the group here and use the check for null to error out.
If you got a group don't check it any further

codecov · 2018-03-22T14:40:47Z

Codecov Report

Merging #8904 into master will decrease coverage by 0.09%.
The diff coverage is 58.4%.

@@ Coverage Diff @@## master #8904 +/- ##
===========================================
- Coverage 51.97% 51.87% -0.1% + Complexity 25287 25285 -2 
===========================================
Files 1601 1602 +1 Lines 94984 94996 +12 Branches 1388 1388 ===========================================
- Hits 49366 49283 -83 - Misses 45618 45713 +95

Impacted Files	Coverage Δ	Complexity Δ
...isioning_api/composer/composer/autoload_static.php	`0% <ø> (ø)`	`1 <0> (ø)`	⬇️
apps/provisioning_api/appinfo/routes.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ioning_api/composer/composer/autoload_classmap.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...rovisioning_api/lib/Controller/UsersController.php	`75% <100%> (+1.63%)`	`127 <0> (-13)`	⬇️
...ovisioning_api/lib/Controller/GroupsController.php	`68.88% <38.63%> (-23.97%)`	`27 <14> (+2)`
apps/provisioning_api/lib/Controller/AUserData.php	`68.11% <68.11%> (ø)`	`12 <12> (?)`
lib/private/Files/ObjectStore/Swift.php	`0% <0%> (-72.42%)`	`9% <0%> (ø)`
lib/private/Files/ObjectStore/SwiftFactory.php	`0% <0%> (-51%)`	`37% <0%> (ø)`
apps/files/lib/Controller/ViewController.php	`77.19% <0%> (-1.15%)`	`18% <0%> (ø)`
lib/private/Files/Mount/Manager.php	`100% <0%> (ø)`	`23% <0%> (-2%)`	⬇️
... and 6 more

skjnldsv · 2018-03-22T17:23:29Z

Ready for review again.
@nickvergessen

juliusknorr · 2018-03-22T20:12:58Z

 // Users
 ['root' => '/cloud', 'name' => 'Users#getUsers', 'url' => '/users', 'verb' => 'GET'],
 ['root' => '/cloud', 'name' => 'Users#getUsersDetails', 'url' => '/users/details', 'verb' => 'GET'],
+ ['root' => '/cloud', 'name' => 'Users#getUsersGroupDetails', 'url' => '/users/{groupId}/details', 'verb' => 'GET'],


The route doesn't really fit the schema of the others. Maybe something like /groups/{groupId}/users would make more sense.

I asked myself the same question, and was rather confuse, do we extract tehe users by groups or do we get the users in group :)
Therefore my current choice, especially since the getUsersData function is in the UsersController :)

And since /groups/{groupId}/ actually return the users, shouldn't we go for something like /groups/{groupId}/details? It really lokks like groups-informations related data. I would prefer something like /groups/{groupId}/users/details but /groups/{groupId}/users doesn't exists :/

/groups/{groupId}/details sounds good to me.

Such url is supposed to return the details of the group, like we have in /groups/details :)

I had teh same weird feeling on the url,
it doesn't feel right to me, maybe a ?group=<filter> on the current endpoint is the better option?

juliusknorr · 2018-03-22T20:21:12Z

+ $isSubadminOfGroup = false;
+ $group = $this->groupManager->get($groupId);
+ if ($group !== null) {
+ $isSubadminOfGroup = $this->groupManager->getSubAdmin()->isSubAdminofGroup($user, $group);


isSubAdminofGroup -> isSubAdminOfGroup

juliusknorr · 2018-03-22T20:31:53Z

+ if ($group !== null) {
+ $isSubadminOfGroup = $this->groupManager->getSubAdmin()->isSubAdminofGroup($user, $group);
+ } else {
+ throw new OCSException('The requested group could not be found', \OCP\API::RESPOND_NOT_FOUND);


\OCP\API is deprecated, see OCSNotFoundException

juliusknorr · 2018-03-22T20:32:48Z

+ }, $users);
+ $users = array_slice(array_values($users), $offset, $limit);
+ } else {
+ throw new OCSException('User does not have access to specified group', \OCP\API::RESPOND_UNAUTHORISED);


\OCP\API is deprecated, see OCSForbiddenException

skjnldsv · 2018-03-23T17:24:33Z

Ready for reviews again! :)

$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details').then((response)=>console.log(response))$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details?limit=10').then((response)=>console.log(response))$.get(OC.linkToOCS('cloud/groups/admin/users',2)+'details?limit=2&offset=2').then((response)=>console.log(response))

Fixed

skjnldsv · 2018-03-23T17:26:10Z

 ['root' => '/cloud', 'name' => 'Groups#getGroups', 'url' => '/groups', 'verb' => 'GET'],
 ['root' => '/cloud', 'name' => 'Groups#getGroupsDetails', 'url' => '/groups/details', 'verb' => 'GET'],
 ['root' => '/cloud', 'name' => 'Groups#getGroup', 'url' => '/groups/{groupId}', 'verb' => 'GET'],
+ ['root' => '/cloud', 'name' => 'Groups#getGroupUsers', 'url' => '/groups/{groupId}/users', 'verb' => 'GET'],


Fallback to proper url design, above one is deprecated!

Could you add a note about this to #7827? But I guess it's not easily possible to drop this as it is part of the OCS standard 😉

nickvergessen · 2018-03-26T11:21:55Z

 IUserSession $userSession,
- ILogger $logger) {
+ ILogger $logger,
+ UsersController $userController) {


Instead extract the shared logic in a subclass/trait?

@nickvergessen what would you suggest to have a clean logic?

nickvergessen · 2018-03-26T11:22:08Z

 use OCP\IRequest;
 use OCP\IUserSession;
 use OCP\IUser;
+use OCA\Provisioning_API\Controller\UsersController;


this should not be needed

nickvergessen · 2018-03-26T11:22:43Z

- $offset = (int)$offset;
- }
-
+	public function getGroupsDetails(string $search = '', int $limit = null, int $offset = 0): DataResponse {


limit is not checked for null anymore?

Since we use slice to cut our arrays, null will be ignored. :)
array array_slice ( array $array , int $offset [, int $length = NULL [, bool $preserve_keys = FALSE ]] )
https://secure.php.net/manual/en/function.array-slice.php

nickvergessen · 2018-03-27T08:30:20Z

+ * @throws OCSException
+ */
+	public function getUserData(string $userId): array {
+ $currentLoggedInUser = $this->userSession->getUser();


Hmm php storm doesn't know userSession. I guess you need to declare it in this trait too.

skjnldsv · 2018-04-03T12:20:07Z

Bump, failure unrelated (swift) :)

MorrisJobke · 2018-04-04T13:15:57Z

+ * @return DataResponse
+ * @throws OCSException	
+ *
+ * @deprecated 14 Use getGroupUsers


Could you add a note about this to #7827? But I guess it's not easily possible to drop this as it is part of the OCS standard 😉

Yeah, I thought it was the better way. It's not that bad tough :)

Added to the changelog in #7827

MorrisJobke

Tested and works 👍

rullzer · 2018-04-04T20:25:43Z

CONFLICTS!!! ⚠️ 😱 🙀

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Signed-off-by: Joas Schilling <coding@schilljs.com>

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv · 2018-04-05T15:41:08Z

Failure unrelated: swift :)
@rullzer

rullzer

Lets do this!

skjnldsv added 3. to review Waiting for reviews feature: users and groups labels Mar 20, 2018

skjnldsv added this to the Nextcloud 14 milestone Mar 20, 2018

skjnldsv self-assigned this Mar 20, 2018

skjnldsv requested review from MorrisJobke, blizzz, juliusknorr, nickvergessen and rullzer March 20, 2018 18:08

skjnldsv mentioned this pull request Mar 21, 2018
Vue migration: settings #8824
Merged
34 tasks

nickvergessen previously requested changes Mar 22, 2018
View reviewed changes

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch from cac9609 to 0db5c79Compare March 22, 2018 14:40

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch from 0db5c79 to e92f640Compare March 22, 2018 14:52

skjnldsv requested a review from nickvergessen March 22, 2018 15:06

juliusknorr previously requested changes Mar 22, 2018
View reviewed changes

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch 8 times, most recently from f428a0b to d473653Compare March 23, 2018 17:02

skjnldsv requested a review from juliusknorr March 23, 2018 17:25

skjnldsv commented Mar 23, 2018
View reviewed changes

nickvergessen reviewed Mar 26, 2018
View reviewed changes

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch 2 times, most recently from 4b4f4dc to 9b081c8Compare March 27, 2018 07:48

nickvergessen reviewed Mar 27, 2018
View reviewed changes

nickvergessen force-pushed the ocs-api-get-users-details-per-groups branch from 1663ec5 to 5d13464Compare March 27, 2018 09:07

MorrisJobke reviewed Apr 4, 2018
View reviewed changes

MorrisJobke approved these changes Apr 4, 2018
View reviewed changes

MorrisJobke mentioned this pull request Apr 4, 2018
Critical changes for developers & admins for Nextcloud 14 #7827
Closed

skjnldsvand others added 8 commits April 5, 2018 17:08

Api, return users details by groups
68ae439
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Limit and offset
f4e84e1
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Cast types directly
4011ea9
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fixed tests
b023bfe
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Trait bump
c103aaa
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fixed tests
762002e
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Use abstract subclass so members can be defined
348171f
Signed-off-by: Joas Schilling <coding@schilljs.com>

Rebase to master
eb4d7fb
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch from 5d13464 to eb4d7fbCompare April 5, 2018 15:13

rullzer approved these changes Apr 6, 2018
View reviewed changes

rullzer merged commit 4a6e31c into masterApr 6, 2018

rullzer deleted the ocs-api-get-users-details-per-groups branch April 6, 2018 08:52

Uh oh!

Uh oh!

Conversation

skjnldsv commented Mar 20, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecovBot commented Mar 22, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skjnldsv commented Mar 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsvMar 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Mar 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer commented Apr 4, 2018

Uh oh!

skjnldsv commented Apr 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

skjnldsv commented Mar 20, 2018•
edited
Loading

codecovBot commented Mar 22, 2018•
edited
Loading

skjnldsvMar 23, 2018•
edited
Loading

skjnldsv commented Mar 23, 2018•
edited
Loading

skjnldsv commented Apr 5, 2018•
edited
Loading