Uh oh!
There was an error while loading. Please reload this page.
Add userdetails to ocs api - #8847
Conversation
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
| * | ||
| * @param string $search | ||
| * @param int $offset | ||
| * @return DataResponse |
There was a problem hiding this comment.
There should be no need to define what is already present in code 😉
There was a problem hiding this comment.
You want me to remove the comment? :)
There was a problem hiding this comment.
Only the @param and @return statements as they are covered by the code already.
| * @throws OCSException | ||
| */ | ||
| protected function getUserData(string $userId): array { | ||
| protected function getUserData(string $userId, bool $throw = true): array { |
There was a problem hiding this comment.
You should catch the exception in getUsersDetails instead of adding variables that cause different behavior.
| */ | ||
| public function getUsersDetails(string $search = '', $offset = null): DataResponse { | ||
| // Limit set to 25 for performance issues | ||
| $limit = 25; |
There was a problem hiding this comment.
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
| $users = []; | ||
| foreach ($subAdminOfGroups as $group) { | ||
| $users = array_merge($users, $this->groupManager->displayNamesInGroup($group, $search)); |
There was a problem hiding this comment.
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) <skjnldsv@protonmail.com>
Codecov Report
@@ 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
|
skjnldsv
commented
Mar 16, 2018
Okay, fixed and comment addressed! Please review :) |
skjnldsv
commented
Mar 16, 2018
Failures unrelated it seems! |
| throw new OCSException('Unknown error occurred', 102); | ||
| } else { | ||
| return new DataResponse($groups); | ||
| return new DataResponse($groups);; |
| public function getUserSubAdminGroups(string $userId): DataResponse { | ||
| $groups = $this->getUserSubAdminGroupsData($userId); | ||
| if(!$groups) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I have no idea, it was already there. Should we check for empty()?
There was a problem hiding this comment.
Hum in the end, No. We should just return an empty data. No errors.
There was a problem hiding this comment.
I guess it's fine to stay with the current way. 🙈
MorrisJobke
commented
Mar 16, 2018
beside that it looks good 👍 |
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
86aeef4 to
a9bf964Comparea9bf964 to
16326efCompareSigned-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
16326ef to
2465934Compare
To replace our current entry point used by the users list settings, I added some required data such as:
Required by #8824