Add userdetails to ocs api#8847

skjnldsv · 2018-03-16T08:40:01Z

To replace our current entry point used by the users list settings, I added some required data such as:

storageLocation
lastLogin
backend
subadmins groups list
Tests

Required by #8824

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

MorrisJobke · 2018-03-16T09:10:22Z

apps/provisioning_api/lib/Controller/UsersController.php

+	 *
+	 * @param string $search
+	 * @param int $offset
+	 * @return DataResponse


There should be no need to define what is already present in code 😉

You want me to remove the comment? :)

Only the @param and @return statements as they are covered by the code already.

MorrisJobke · 2018-03-16T09:12:20Z

apps/provisioning_api/lib/Controller/UsersController.php

 	 * @throws OCSException
 	 */
-	protected function getUserData(string $userId): array {
+	protected function getUserData(string $userId, bool $throw = true): array {


You should catch the exception in getUsersDetails instead of adding variables that cause different behavior.

nickvergessen · 2018-03-16T09:35:41Z

apps/provisioning_api/lib/Controller/UsersController.php

+	 */
+	public function getUsersDetails(string $search = '', $offset = null): DataResponse {
+		// Limit set to 25 for performance issues 
+		$limit = 25;


Since this is an API, I'd vote to add it as a paramter to the function.
If you know you deal with an instance which has only 30 users, there is no need to page the request

Okay for me then :)

nickvergessen · 2018-03-16T09:36:25Z

apps/provisioning_api/lib/Controller/UsersController.php

+
+			$users = [];
+			foreach ($subAdminOfGroups as $group) {
+				$users = array_merge($users, $this->groupManager->displayNamesInGroup($group, $search));


use limit and offset here?

Seems legit, the slice is already done in displayNamesInGroup...
I don't know why it wasn't like that already :)

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

codecov · 2018-03-16T10:44:38Z

Codecov Report

Merging #8847 into master will decrease coverage by <.01%.
The diff coverage is 51.21%.

@@             Coverage Diff              @@
##             master    #8847      +/-   ##
============================================
- Coverage      51.9%   51.89%   -0.01%     
- Complexity    25269    25276       +7     
============================================
  Files          1604     1604              
  Lines         94765    94792      +27     
  Branches       1377     1377              
============================================
+ Hits          49190    49197       +7     
- Misses        45575    45595      +20

Impacted Files	Coverage Δ	Complexity Δ
apps/provisioning_api/appinfo/routes.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...rovisioning_api/lib/Controller/UsersController.php	`77.2% <52.5%> (-3.98%)`	`133 <16> (+7)`
apps/files_trashbin/lib/Trashbin.php	`72.46% <0%> (-0.25%)`	`136% <0%> (ø)`
lib/private/Files/ObjectStore/SwiftFactory.php	`55% <0%> (+4%)`	`37% <0%> (ø)`	⬇️

skjnldsv · 2018-03-16T10:45:39Z

Okay, fixed and comment addressed! Please review :)

skjnldsv · 2018-03-16T11:02:15Z

Failures unrelated it seems!

rullzer

good stuff

MorrisJobke · 2018-03-16T13:46:08Z

apps/provisioning_api/lib/Controller/UsersController.php

 			throw new OCSException('Unknown error occurred', 102);
 		} else {
-			return new DataResponse($groups);
+			return new DataResponse($groups);;


Please fix ;)

MorrisJobke · 2018-03-16T13:48:24Z

apps/provisioning_api/lib/Controller/UsersController.php

+	public function getUserSubAdminGroups(string $userId): DataResponse {
+		$groups = $this->getUserSubAdminGroupsData($userId);
+
 		if(!$groups) {


Empty list would result in Unknown error occurred:

php > echo (![]) ? 'true' : 'false'; true

SHouldn't the error message a bit more specific? Or is this a known behavior?

I have no idea, it was already there. Should we check for empty()?

Hum in the end, No. We should just return an empty data. No errors.

I guess it's fine to stay with the current way. 🙈

With my last edits?

MorrisJobke · 2018-03-16T13:53:39Z

beside that it looks good 👍

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

MorrisJobke

Works 👍

skjnldsv added 2 commits March 16, 2018 09:19

Add userdetails to ocs api

ce625bb

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

Fixed throw error on unreachable users

aa1e871

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

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

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

skjnldsv self-assigned this Mar 16, 2018

skjnldsv requested review from MorrisJobke, nickvergessen and rullzer March 16, 2018 08:40

skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 16, 2018

MorrisJobke reviewed Mar 16, 2018

View reviewed changes

nickvergessen reviewed Mar 16, 2018

View reviewed changes

Fixed tests and improved code

f5137fe

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

skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 16, 2018

skjnldsv requested review from MorrisJobke and nickvergessen March 16, 2018 11:07

rullzer approved these changes Mar 16, 2018

View reviewed changes

MorrisJobke reviewed Mar 16, 2018

View reviewed changes

Check if groups is array, return empty do not throw

2493e94

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

skjnldsv requested a review from MorrisJobke March 16, 2018 14:36

skjnldsv force-pushed the ocs-api-userdetails branch from 86aeef4 to a9bf964 Compare March 16, 2018 15:07

skjnldsv force-pushed the ocs-api-userdetails branch from a9bf964 to 16326ef Compare March 16, 2018 15:09

skjnldsv mentioned this pull request Mar 17, 2018

Vue migration: settings #8824

Merged

34 tasks

fixup! Check if groups is array, return empty do not throw

2465934

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

skjnldsv force-pushed the ocs-api-userdetails branch from 16326ef to 2465934 Compare March 19, 2018 14:13

skjnldsv requested a review from blizzz March 20, 2018 08:47

MorrisJobke approved these changes Mar 20, 2018

View reviewed changes

MorrisJobke merged commit 29c551c into master Mar 20, 2018

MorrisJobke deleted the ocs-api-userdetails branch March 20, 2018 17:12

MorrisJobke mentioned this pull request Mar 20, 2018

Critical changes for developers & admins for Nextcloud 14 #7827

Closed

acanrpk mentioned this pull request Jan 8, 2019

NextCloud 13+? Bu1k/ProvisioningAPI#1

Open

Uh oh!

Conversation

skjnldsv commented Mar 16, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

skjnldsv commented Mar 16, 2018

Uh oh!

skjnldsv commented Mar 16, 2018

Uh oh!

rullzer left a comment

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!

MorrisJobke commented Mar 16, 2018

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skjnldsv commented Mar 16, 2018 •

edited

Loading

codecov bot commented Mar 16, 2018 •

edited

Loading