Merge pull request #16889 from owncloud/group-sorting-fixes

Only sort by group name when LDAP is involved

7 files changed, 183 insertions, 11 deletions

diff --git a/lib/private/group/manager.php b/lib/private/group/manager.php
index 12136a1bd25..70263fc2afa 100644
--- a/lib/private/group/manager.php
+++ b/lib/private/group/manager.php
@@ -98,6 +98,24 @@ class Manager extends PublicEmitter implements IGroupManager {
 	}
 
 	/**
+	 * Checks whether a given backend is used
+	 *
+	 * @param string $backendClass Full classname including complete namespace
+	 * @return bool
+	 */
+	public function isBackendUsed($backendClass) {
+		$backendClass = strtolower(ltrim($backendClass, '\\'));
+
+		foreach ($this->backends as $backend) {
+			if (strtolower(get_class($backend)) === $backendClass) {
+				return true;
+			}
+		}
+
+		return false;
+	}
+
+	/**
 	 * @param \OC_Group_Backend $backend
 	 */
 	public function addBackend($backend) {
diff --git a/lib/public/igroupmanager.php b/lib/public/igroupmanager.php
index ffd459b09e1..ec4b03c5d18 100644
--- a/lib/public/igroupmanager.php
+++ b/lib/public/igroupmanager.php
@@ -41,6 +41,15 @@ namespace OCP;
  */
 interface IGroupManager {
 	/**
+	 * Checks whether a given backend is used
+	 *
+	 * @param string $backendClass Full classname including complete namespace
+	 * @return bool
+	 * @since 8.1.0
+	 */
+	public function isBackendUsed($backendClass);
+
+	/**
 	 * @param \OCP\UserInterface $backend
 	 * @since 8.0.0
 	 */
diff --git a/settings/controller/groupscontroller.php b/settings/controller/groupscontroller.php
index c3c0ea5ff20..6cb0cd3e008 100644
--- a/settings/controller/groupscontroller.php
+++ b/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(
diff --git a/settings/js/users/groups.js b/settings/js/users/groups.js
index 0fcca39843f..d205e915508 100644
--- a/settings/js/users/groups.js
+++ b/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 = '';
@@ -63,6 +69,33 @@ GroupList = {
 		var lis = $userGroupList.find('.isgroup').get();
 
 		lis.sort(function (a, b) {
+			// "Everyone" always at the top
+			if ($(a).data('gid') === '_everyone') {
+				return -1;
+			} else if ($(b).data('gid') === '_everyone') {
+				return 1;
+			}
+
+			// "admin" always as second
+			if ($(a).data('gid') === 'admin') {
+				return -1;
+			} else if ($(b).data('gid') === 'admin') {
+				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()
@@ -113,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) {
 
@@ -271,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();
diff --git a/settings/templates/users/part.grouplist.php b/settings/templates/users/part.grouplist.php
index 5b516bc0e16..51638c7bcce 100644
--- a/settings/templates/users/part.grouplist.php
+++ b/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="#">
diff --git a/settings/users.php b/settings/users.php
index 44e2548be72..c438e5fbfd0 100644
--- a/settings/users.php
+++ b/settings/users.php
@@ -37,12 +37,23 @@ 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')) {
+	$isLDAPUsed = $groupManager->isBackendUsed('\OCA\user_ldap\GROUP_LDAP');
+	if ($isLDAPUsed) {
+		// LDAP user count can be slow, so we sort by group 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 +86,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);
diff --git a/tests/settings/controller/groupscontrollertest.php b/tests/settings/controller/groupscontrollertest.php
index 3c15754846c..82b4c7d3c05 100644
--- a/tests/settings/controller/groupscontrollertest.php
+++ b/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);
 	}