Merge pull request #16402 from owncloud/issue-15956-slow-group-usercount

Sort user groups by group name and hide the user count

lib/private/group/metadata.php

@@ -27,7 +27,8 @@ namespace OC\Group;
 
 class MetaData {
 	const SORT_NONE = 0;
-	const SORT_USERCOUNT = 1;
+	const SORT_USERCOUNT = 1; // May have performance issues on LDAP backends
+	const SORT_GROUPNAME = 2;
 
 	/**
 	 * @var string $user
@@ -121,15 +122,19 @@ class MetaData {
 	}
 
 	/**
-	 * sets the sort mode, currently 0 (none) and 1 (user entries,
-	 * descending) are supported
-	 * @param int $sortMode (SORT_NONE, SORT_USERCOUNT)
+	 * sets the sort mode, see SORT_* constants for supported modes
+	 *
+	 * @param int $sortMode
 	 */
 	public function setSorting($sortMode) {
-		if($sortMode >= 0 && $sortMode <= 1) {
-			$this->sorting = $sortMode;
-		} else {
-			$this->sorting = 0;
+		switch ($sortMode) {
+			case self::SORT_USERCOUNT:
+			case self::SORT_GROUPNAME:
+				$this->sorting = $sortMode;
+				break;
+
+			default:
+				$this->sorting = self::SORT_NONE;
 		}
 	}
 
@@ -139,27 +144,29 @@ class MetaData {
 	 * @param array $sortKeys the sort key array, by reference
 	 * @param int $sortIndex the sort key index, by reference
 	 * @param array $data the group's meta data as returned by generateGroupMetaData()
-	 * @return null
 	 */
 	private function addEntry(&$entries, &$sortKeys, &$sortIndex, $data) {
 		$entries[] = $data;
-		if($this->sorting === 1) {
+		if ($this->sorting === self::SORT_USERCOUNT) {
 			$sortKeys[$sortIndex] = $data['usercount'];
 			$sortIndex++;
+		} else if ($this->sorting === self::SORT_GROUPNAME) {
+			$sortKeys[$sortIndex] = $data['name'];
+			$sortIndex++;
 		}
 	}
 
 	/**
 	 * creates an array containing the group meta data
-	 * @param \OC\Group\Group $group
+	 * @param \OCP\IGroup $group
 	 * @param string $userSearch
 	 * @return array with the keys 'id', 'name' and 'usercount'
 	 */
-	private function generateGroupMetaData(\OC\Group\Group $group, $userSearch) {
+	private function generateGroupMetaData(\OCP\IGroup $group, $userSearch) {
 		return array(
 				'id' => $group->getGID(),
 				'name' => $group->getGID(),
-				'usercount' => $group->count($userSearch)
+				'usercount' => $this->sorting === self::SORT_USERCOUNT ? $group->count($userSearch) : 0,
 			);
 	}
 
@@ -170,15 +177,17 @@ class MetaData {
 	 * @param return null
 	 */
 	private function sort(&$entries, $sortKeys) {
-		if($this->sorting > 0) {
+		if ($this->sorting === self::SORT_USERCOUNT) {
 			array_multisort($sortKeys, SORT_DESC, $entries);
+		} else if ($this->sorting === self::SORT_GROUPNAME) {
+			array_multisort($sortKeys, SORT_ASC, $entries);
 		}
 	}
 
 	/**
 	 * returns the available groups
 	 * @param string $search a search string
-	 * @return \OC\Group\Group[]
+	 * @return \OCP\IGroup[]
 	 */
 	private function getGroups($search = '') {
 		if($this->isAdmin) {

settings/controller/groupscontroller.php

@@ -76,7 +76,7 @@ class GroupsController extends Controller {
 
 		$groupsInfo = new \OC\Group\MetaData($this->userSession->getUser()->getUID(),
 			$this->isAdmin, $this->groupManager);
-		$groupsInfo->setSorting($groupsInfo::SORT_USERCOUNT);
+		$groupsInfo->setSorting($groupsInfo::SORT_GROUPNAME);
 		list($adminGroups, $groups) = $groupsInfo->get($groupPattern, $pattern);
 
 		return new DataResponse(

settings/js/users/groups.js

@@ -271,7 +271,8 @@ GroupList = {
 $(document).ready( function () {
 	$userGroupList = $('#usergrouplist');
 	GroupList.initDeleteHandling();
-	GroupList.getEveryoneCount();
+	// TODO: disabled due to performance issues
+	// GroupList.getEveryoneCount();
 
 	// Display or hide of Create Group List Element
 	$('#newgroup-form').hide();

settings/users.php

@@ -42,7 +42,7 @@ $config = \OC::$server->getConfig();
 $isAdmin = OC_User::isAdminUser(OC_User::getUser());
 
 $groupsInfo = new \OC\Group\MetaData(OC_User::getUser(), $isAdmin, $groupManager);
-$groupsInfo->setSorting($groupsInfo::SORT_USERCOUNT);
+$groupsInfo->setSorting($groupsInfo::SORT_GROUPNAME);
 list($adminGroup, $groups) = $groupsInfo->get();
 
 $recoveryAdminEnabled = OC_App::isEnabled('encryption') &&

tests/lib/group/metadata.php

@@ -16,7 +16,7 @@ class Test_MetaData extends \Test\TestCase {
 			->getMock();
 	}
 
-	private function getGroupMock() {
+	private function getGroupMock($countCallCount = 0) {
 		$group = $this->getMockBuilder('\OC\Group\Group')
 			->disableOriginalConstructor()
 			->getMock();
@@ -28,7 +28,7 @@ class Test_MetaData extends \Test\TestCase {
 				'g2', 'g2', 'g2',
 				'g3', 'g3', 'g3'));
 
-		$group->expects($this->exactly(3))
+		$group->expects($this->exactly($countCallCount))
 			->method('count')
 			->with('')
 			->will($this->onConsecutiveCalls(2, 3, 5));
@@ -54,14 +54,15 @@ class Test_MetaData extends \Test\TestCase {
 		$this->assertSame(2, count($ordinaryGroups));
 
 		$this->assertSame('g2', $ordinaryGroups[0]['name']);
-		$this->assertSame(3, $ordinaryGroups[0]['usercount']);
+		// user count is not loaded
+		$this->assertSame(0, $ordinaryGroups[0]['usercount']);
 	}
 
 	public function testGetWithSorting() {
 		$groupManager = $this->getGroupManagerMock();
 		$groupMetaData = new \OC\Group\MetaData('foo', true, $groupManager);
 		$groupMetaData->setSorting($groupMetaData::SORT_USERCOUNT);
-		$group = $this->getGroupMock();
+		$group = $this->getGroupMock(3);
 		$groups = array_fill(0, 3, $group);
 
 		$groupManager->expects($this->once())

tests/settings/controller/groupscontrollertest.php

@@ -111,26 +111,26 @@ class GroupsControllerTest extends \Test\TestCase {
 					0 => array(
 						'id' => 'admin',
 						'name' => 'admin',
-						'usercount' => 18
+						'usercount' => 0,//User count disabled 18,
 					)
 				),
 				'groups' =>
 					array(
 						0 => array(
-							'id' => 'secondGroup',
-							'name' => 'secondGroup',
-							'usercount' => 25
-						),
-						1 => array(
-							'id' => 'thirdGroup',
-							'name' => 'thirdGroup',
-							'usercount' => 14
-						),
-						2 => array(
 							'id' => 'firstGroup',
 							'name' => 'firstGroup',
-							'usercount' => 12
-						)
+							'usercount' => 0,//User count disabled 12,
+						),
+						1 => array(
+							'id' => 'secondGroup',
+							'name' => 'secondGroup',
+							'usercount' => 0,//User count disabled 25,
+						),
+						2 => array(
+							'id' => 'thirdGroup',
+							'name' => 'thirdGroup',
+							'usercount' => 0,//User count disabled 14,
+						),
 					)
 				)
 			)