Api: return users details by groups#8904

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

apps/provisioning_api/lib/Controller/UsersController.php

+		// 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

apps/provisioning_api/lib/Controller/UsersController.php

+		$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

apps/provisioning_api/appinfo/routes.php

 		// 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

apps/provisioning_api/lib/Controller/UsersController.php

+		$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

apps/provisioning_api/lib/Controller/UsersController.php

+		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

apps/provisioning_api/lib/Controller/UsersController.php

+			}, $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

apps/provisioning_api/appinfo/routes.php

 		['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

apps/provisioning_api/lib/Controller/GroupsController.php

 			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

apps/provisioning_api/lib/Controller/GroupsController.php

 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

apps/provisioning_api/lib/Controller/GroupsController.php

-			$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

apps/provisioning_api/lib/Controller/UserDataTrait.php

+	 * @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

apps/provisioning_api/lib/Controller/GroupsController.php

+	 * @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) <[email protected]>

Signed-off-by: Joas Schilling <[email protected]>

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

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 0db5c79 Compare March 22, 2018 14:40

skjnldsv force-pushed the ocs-api-get-users-details-per-groups branch from 0db5c79 to e92f640 Compare 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 d473653 Compare 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 9b081c8 Compare 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 5d13464 Compare 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

skjnldsv and others added 8 commits April 5, 2018 17:08

Api, return users details by groups

68ae439

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Limit and offset

f4e84e1

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Cast types directly

4011ea9

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Fixed tests

b023bfe

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Trait bump

c103aaa

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Fixed tests

762002e

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Use abstract subclass so members can be defined

348171f

Signed-off-by: Joas Schilling <[email protected]>

Rebase to master

eb4d7fb

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

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

rullzer approved these changes Apr 6, 2018

View reviewed changes

rullzer merged commit 4a6e31c into master Apr 6, 2018

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

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!

codecov bot 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!

skjnldsv Mar 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

Milestone

skjnldsv commented Mar 20, 2018 •

edited

Loading

codecov bot commented Mar 22, 2018 •

edited

Loading

skjnldsv Mar 23, 2018 •

edited

Loading

skjnldsv commented Mar 23, 2018 •

edited

Loading

skjnldsv commented Apr 5, 2018 •

edited

Loading