Only sort by group name when LDAP is involved

lib/private/group/manager.php

@@ -97,6 +97,13 @@ class Manager extends PublicEmitter implements IGroupManager {
 		});
 	}
 
+	/**
+	 * @return \OC_Group_Backend[] Get registered backends
+	 */
+	public function getBackends() {
+		return $this->backends;
+	}
+
 	/**
 	 * @param \OC_Group_Backend $backend
 	 */

lib/public/igroupmanager.php

@@ -40,6 +40,12 @@ namespace OCP;
  * @since 8.0.0
  */
 interface IGroupManager {
+	/**
+	 * @return \OC_Group_Backend[] Get registered backends
+	 * @since 8.1.0
+	 */
+	public function getBackends();
+
 	/**
 	 * @param \OCP\UserInterface $backend
 	 * @since 8.0.0

settings/controller/groupscontroller.php

@@ -23,7 +23,8 @@
 namespace OC\Settings\Controller;
 
 use OC\AppFramework\Http;
-use \OCP\AppFramework\Controller;
+use OC\Group\MetaData;
+use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\IGroupManager;
 use OCP\IL10N;
@@ -69,14 +70,15 @@ class GroupsController extends Controller {
 	 *
 	 * @param string $pattern
 	 * @param bool $filterGroups
+	 * @param int $sortGroups
 	 * @return DataResponse
 	 */
-	public function index($pattern = '', $filterGroups = false) {
+	public function index($pattern = '', $filterGroups = false, $sortGroups = MetaData::SORT_USERCOUNT) {
 		$groupPattern = $filterGroups ? $pattern : '';
 
-		$groupsInfo = new \OC\Group\MetaData($this->userSession->getUser()->getUID(),
+		$groupsInfo = new MetaData($this->userSession->getUser()->getUID(),
 			$this->isAdmin, $this->groupManager);
-		$groupsInfo->setSorting($groupsInfo::SORT_GROUPNAME);
+		$groupsInfo->setSorting($sortGroups);
 		list($adminGroups, $groups) = $groupsInfo->get($groupPattern, $pattern);
 
 		return new DataResponse(

settings/js/users/groups.js

@@ -5,7 +5,8 @@
  * See the COPYING-README file.
  */
 
-var $userGroupList;
+var $userGroupList,
+	$sortGroupBy;
 
 var GroupList;
 GroupList = {
@@ -27,6 +28,11 @@ GroupList = {
 	},
 
 	setUserCount: function (groupLiElement, usercount) {
+		if ($sortGroupBy !== 1) {
+			// If we don't sort by group count we dont display them either
+			return;
+		}
+
 		var $groupLiElement = $(groupLiElement);
 		if (usercount === undefined || usercount === 0 || usercount < 0) {
 			usercount = '';
@@ -77,6 +83,19 @@ GroupList = {
 				return 1;
 			}
 
+			if ($sortGroupBy === 1) {
+				// Sort by user count first
+				var $usersGroupA = $(a).data('usercount'),
+					$usersGroupB = $(b).data('usercount');
+				if ($usersGroupA > 0 && $usersGroupA > $usersGroupB) {
+					return -1;
+				}
+				if ($usersGroupB > 0 && $usersGroupB > $usersGroupA) {
+					return 1;
+				}
+			}
+
+			// Fallback or sort by group name
 			return UserList.alphanum(
 				$(a).find('a span').text(),
 				$(b).find('a span').text()
@@ -127,7 +146,8 @@ GroupList = {
 			OC.generateUrl('/settings/users/groups'),
 			{
 				pattern: filter.getPattern(),
-				filterGroups: filter.filterGroups ? 1 : 0
+				filterGroups: filter.filterGroups ? 1 : 0,
+				sortGroups: $sortGroupBy
 			},
 			function (result) {
 
@@ -285,8 +305,11 @@ GroupList = {
 $(document).ready( function () {
 	$userGroupList = $('#usergrouplist');
 	GroupList.initDeleteHandling();
-	// TODO: disabled due to performance issues
-	// GroupList.getEveryoneCount();
+	$sortGroupBy = $userGroupList.data('sort-groups');
+	if ($sortGroupBy === 1) {
+		// Disabled due to performance issues, when we don't need it for sorting
+		GroupList.getEveryoneCount();
+	}
 
 	// Display or hide of Create Group List Element
 	$('#newgroup-form').hide();

settings/templates/users/part.grouplist.php

@@ -1,4 +1,4 @@
-<ul id="usergrouplist">
+<ul id="usergrouplist" data-sort-groups="<?php p($_['sortGroups']); ?>">
 	<!-- Add new group -->
 	<li id="newgroup-init">
 		<a href="#">

settings/users.php

@@ -37,12 +37,25 @@ OC_App::setActiveNavigationEntry( 'core_users' );
 $userManager = \OC_User::getManager();
 $groupManager = \OC_Group::getManager();
 
+// Set the sort option: SORT_USERCOUNT or SORT_GROUPNAME
+$sortGroupsBy = \OC\Group\MetaData::SORT_USERCOUNT;
+
+if (class_exists('\OCA\user_ldap\GROUP_LDAP')) {
+	$backends = $groupManager->getBackends();
+	foreach ($backends as $backend) {
+		if ($backend instanceof \OCA\user_ldap\GROUP_LDAP) {
+			// LDAP user count can be slow, so we sort by gorup name here
+			$sortGroupsBy = \OC\Group\MetaData::SORT_GROUPNAME;
+		}
+	}
+}
+
 $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_GROUPNAME);
+$groupsInfo->setSorting($sortGroupsBy);
 list($adminGroup, $groups) = $groupsInfo->get();
 
 $recoveryAdminEnabled = OC_App::isEnabled('encryption') &&
@@ -75,6 +88,7 @@ $defaultQuotaIsUserDefined=array_search($defaultQuota, $quotaPreset)===false
 
 $tmpl = new OC_Template("settings", "users/main", "user");
 $tmpl->assign('groups', $groups);
+$tmpl->assign('sortGroups', $sortGroupsBy);
 $tmpl->assign('adminGroup', $adminGroup);
 $tmpl->assign('isAdmin', (int)$isAdmin);
 $tmpl->assign('subadmins', $subadmins);

tests/settings/controller/groupscontrollertest.php

@@ -10,6 +10,7 @@
 namespace OC\Settings\Controller;
 
 use OC\Group\Group;
+use OC\Group\MetaData;
 use \OC\Settings\Application;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
@@ -50,7 +51,7 @@ class GroupsControllerTest extends \Test\TestCase {
 	 * TODO: Since GroupManager uses the static OC_Subadmin class it can't be mocked
 	 * to test for subadmins. Thus the test always assumes you have admin permissions...
 	 */
-	public function testIndex() {
+	public function testIndexSortByName() {
 		$firstGroup = $this->getMockBuilder('\OC\Group\Group')
 			->disableOriginalConstructor()->getMock();
 		$firstGroup
@@ -135,6 +136,99 @@ class GroupsControllerTest extends \Test\TestCase {
 				)
 			)
 		);
+		$response = $this->groupsController->index('', false, MetaData::SORT_GROUPNAME);
+		$this->assertEquals($expectedResponse, $response);
+	}
+
+	/**
+	 * TODO: Since GroupManager uses the static OC_Subadmin class it can't be mocked
+	 * to test for subadmins. Thus the test always assumes you have admin permissions...
+	 */
+	public function testIndexSortbyCount() {
+		$firstGroup = $this->getMockBuilder('\OC\Group\Group')
+			->disableOriginalConstructor()->getMock();
+		$firstGroup
+			->method('getGID')
+			->will($this->returnValue('firstGroup'));
+		$firstGroup
+			->method('count')
+			->will($this->returnValue(12));
+		$secondGroup = $this->getMockBuilder('\OC\Group\Group')
+			->disableOriginalConstructor()->getMock();
+		$secondGroup
+			->method('getGID')
+			->will($this->returnValue('secondGroup'));
+		$secondGroup
+			->method('count')
+			->will($this->returnValue(25));
+		$thirdGroup = $this->getMockBuilder('\OC\Group\Group')
+			->disableOriginalConstructor()->getMock();
+		$thirdGroup
+			->method('getGID')
+			->will($this->returnValue('thirdGroup'));
+		$thirdGroup
+			->method('count')
+			->will($this->returnValue(14));
+		$fourthGroup = $this->getMockBuilder('\OC\Group\Group')
+			->disableOriginalConstructor()->getMock();
+		$fourthGroup
+			->method('getGID')
+			->will($this->returnValue('admin'));
+		$fourthGroup
+			->method('count')
+			->will($this->returnValue(18));
+		/** @var \OC\Group\Group[] $groups */
+		$groups = array();
+		$groups[] = $firstGroup;
+		$groups[] = $secondGroup;
+		$groups[] = $thirdGroup;
+		$groups[] = $fourthGroup;
+
+		$user = $this->getMockBuilder('\OC\User\User')
+			->disableOriginalConstructor()->getMock();
+		$this->container['UserSession']
+			->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+		$user
+			->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('MyAdminUser'));
+		$this->container['GroupManager']
+			->method('search')
+			->will($this->returnValue($groups));
+
+		$expectedResponse = new DataResponse(
+			array(
+				'data' => array(
+				'adminGroups' => array(
+					0 => array(
+						'id' => 'admin',
+						'name' => 'admin',
+						'usercount' => 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,
+						),
+					)
+				)
+			)
+		);
 		$response = $this->groupsController->index();
 		$this->assertEquals($expectedResponse, $response);
 	}