Merge pull request #8255 from nextcloud/bugfix/noid/group-display-name

Full implement group display names

8 files changed, 159 insertions, 78 deletions

diff --git a/apps/dav/lib/CalDAV/Activity/Provider/Base.php b/apps/dav/lib/CalDAV/Activity/Provider/Base.php
index b6d8a5be736..99ad903f247 100644
--- a/apps/dav/lib/CalDAV/Activity/Provider/Base.php
+++ b/apps/dav/lib/CalDAV/Activity/Provider/Base.php
@@ -26,6 +26,8 @@ namespace OCA\DAV\CalDAV\Activity\Provider;
 use OCA\DAV\CalDAV\CalDavBackend;
 use OCP\Activity\IEvent;
 use OCP\Activity\IProvider;
+use OCP\IGroup;
+use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IUser;
 use OCP\IUserManager;
@@ -35,14 +37,22 @@ abstract class Base implements IProvider {
 	/** @var IUserManager */
 	protected $userManager;
 
-	/** @var string[] cached displayNames - key is the UID and value the displayname */
-	protected $displayNames = [];
+	/** @var string[]  */
+	protected $userDisplayNames = [];
+
+	/** @var IGroupManager */
+	protected $groupManager;
+
+	/** @var string[] */
+	protected $groupDisplayNames = [];
 
 	/**
 	 * @param IUserManager $userManager
+	 * @param IGroupManager $groupManager
 	 */
-	public function __construct(IUserManager $userManager) {
+	public function __construct(IUserManager $userManager, IGroupManager $groupManager) {
 		$this->userManager = $userManager;
+		$this->groupManager = $groupManager;
 	}
 
 	/**
@@ -113,30 +123,18 @@ abstract class Base implements IProvider {
 	}
 
 	/**
-	 * @param string $id
-	 * @return array
-	 */
-	protected function generateGroupParameter($id) {
-		return [
-			'type' => 'group',
-			'id' => $id,
-			'name' => $id,
-		];
-	}
-
-	/**
 	 * @param string $uid
 	 * @return array
 	 */
 	protected function generateUserParameter($uid) {
-		if (!isset($this->displayNames[$uid])) {
-			$this->displayNames[$uid] = $this->getDisplayName($uid);
+		if (!isset($this->userDisplayNames[$uid])) {
+			$this->userDisplayNames[$uid] = $this->getUserDisplayName($uid);
 		}
 
 		return [
 			'type' => 'user',
 			'id' => $uid,
-			'name' => $this->displayNames[$uid],
+			'name' => $this->userDisplayNames[$uid],
 		];
 	}
 
@@ -144,12 +142,39 @@ abstract class Base implements IProvider {
 	 * @param string $uid
 	 * @return string
 	 */
-	protected function getDisplayName($uid) {
+	protected function getUserDisplayName($uid) {
 		$user = $this->userManager->get($uid);
 		if ($user instanceof IUser) {
 			return $user->getDisplayName();
-		} else {
-			return $uid;
 		}
+		return $uid;
+	}
+
+	/**
+	 * @param string $gid
+	 * @return array
+	 */
+	protected function generateGroupParameter($gid) {
+		if (!isset($this->groupDisplayNames[$gid])) {
+			$this->groupDisplayNames[$gid] = $this->getGroupDisplayName($gid);
+		}
+
+		return [
+			'type' => 'group',
+			'id' => $gid,
+			'name' => $this->groupDisplayNames[$gid],
+		];
+	}
+
+	/**
+	 * @param string $gid
+	 * @return string
+	 */
+	protected function getGroupDisplayName($gid) {
+		$group = $this->groupManager->get($gid);
+		if ($group instanceof IGroup) {
+			return $group->getDisplayName();
+		}
+		return $gid;
 	}
 }
diff --git a/apps/dav/lib/CalDAV/Activity/Provider/Calendar.php b/apps/dav/lib/CalDAV/Activity/Provider/Calendar.php
index ff129144285..db79b0f6656 100644
--- a/apps/dav/lib/CalDAV/Activity/Provider/Calendar.php
+++ b/apps/dav/lib/CalDAV/Activity/Provider/Calendar.php
@@ -26,6 +26,7 @@ namespace OCA\DAV\CalDAV\Activity\Provider;
 use OCP\Activity\IEvent;
 use OCP\Activity\IEventMerger;
 use OCP\Activity\IManager;
+use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IURLGenerator;
 use OCP\IUserManager;
@@ -63,10 +64,11 @@ class Calendar extends Base {
 	 * @param IURLGenerator $url
 	 * @param IManager $activityManager
 	 * @param IUserManager $userManager
+	 * @param IGroupManager $groupManager
 	 * @param IEventMerger $eventMerger
 	 */
-	public function __construct(IFactory $languageFactory, IURLGenerator $url, IManager $activityManager, IUserManager $userManager, IEventMerger $eventMerger) {
-		parent::__construct($userManager);
+	public function __construct(IFactory $languageFactory, IURLGenerator $url, IManager $activityManager, IUserManager $userManager, IGroupManager $groupManager, IEventMerger $eventMerger) {
+		parent::__construct($userManager, $groupManager);
 		$this->languageFactory = $languageFactory;
 		$this->url = $url;
 		$this->activityManager = $activityManager;
diff --git a/apps/dav/lib/CalDAV/Activity/Provider/Event.php b/apps/dav/lib/CalDAV/Activity/Provider/Event.php
index eabd2e517c0..f13cb0c266b 100644
--- a/apps/dav/lib/CalDAV/Activity/Provider/Event.php
+++ b/apps/dav/lib/CalDAV/Activity/Provider/Event.php
@@ -26,6 +26,7 @@ namespace OCA\DAV\CalDAV\Activity\Provider;
 use OCP\Activity\IEvent;
 use OCP\Activity\IEventMerger;
 use OCP\Activity\IManager;
+use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IURLGenerator;
 use OCP\IUserManager;
@@ -57,10 +58,11 @@ class Event extends Base {
 	 * @param IURLGenerator $url
 	 * @param IManager $activityManager
 	 * @param IUserManager $userManager
+	 * @param IGroupManager $groupManager
 	 * @param IEventMerger $eventMerger
 	 */
-	public function __construct(IFactory $languageFactory, IURLGenerator $url, IManager $activityManager, IUserManager $userManager, IEventMerger $eventMerger) {
-		parent::__construct($userManager);
+	public function __construct(IFactory $languageFactory, IURLGenerator $url, IManager $activityManager, IUserManager $userManager, IGroupManager $groupManager, IEventMerger $eventMerger) {
+		parent::__construct($userManager, $groupManager);
 		$this->languageFactory = $languageFactory;
 		$this->url = $url;
 		$this->activityManager = $activityManager;
diff --git a/apps/dav/tests/unit/CalDAV/Activity/Provider/BaseTest.php b/apps/dav/tests/unit/CalDAV/Activity/Provider/BaseTest.php
index affc1909e3f..37a56f88042 100644
--- a/apps/dav/tests/unit/CalDAV/Activity/Provider/BaseTest.php
+++ b/apps/dav/tests/unit/CalDAV/Activity/Provider/BaseTest.php
@@ -29,6 +29,7 @@ use OCP\Activity\IProvider;
 use OCP\IL10N;
 use OCP\IUser;
 use OCP\IUserManager;
+use OCP\IGroupManager;
 use Test\TestCase;
 
 class BaseTest extends TestCase {
@@ -36,15 +37,20 @@ class BaseTest extends TestCase {
 	/** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */
 	protected $userManager;
 
+	/** @var IGroupManager|\PHPUnit_Framework_MockObject_MockObject */
+	protected $groupManager;
+
 	/** @var IProvider|Base|\PHPUnit_Framework_MockObject_MockObject */
 	protected $provider;
 
 	protected function setUp() {
 		parent::setUp();
 		$this->userManager = $this->createMock(IUserManager::class);
+		$this->groupManager = $this->createMock(IGroupManager::class);
 		$this->provider = $this->getMockBuilder(Base::class)
 			->setConstructorArgs([
-				$this->userManager
+				$this->userManager,
+				$this->groupManager
 			])
 			->setMethods(['parse'])
 			->getMock();
diff --git a/apps/files_sharing/lib/Activity/Providers/Groups.php b/apps/files_sharing/lib/Activity/Providers/Groups.php
index 53262e19311..9a8f7164c55 100644
--- a/apps/files_sharing/lib/Activity/Providers/Groups.php
+++ b/apps/files_sharing/lib/Activity/Providers/Groups.php
@@ -24,6 +24,12 @@
 namespace OCA\Files_Sharing\Activity\Providers;
 
 use OCP\Activity\IEvent;
+use OCP\Activity\IManager;
+use OCP\IGroup;
+use OCP\IGroupManager;
+use OCP\IURLGenerator;
+use OCP\IUserManager;
+use OCP\L10N\IFactory;
 
 class Groups extends Base {
 
@@ -32,6 +38,24 @@ class Groups extends Base {
 	const SUBJECT_UNSHARED_GROUP_SELF = 'unshared_group_self';
 	const SUBJECT_UNSHARED_GROUP_BY = 'unshared_group_by';
 
+	/** @var IGroupManager */
+	protected $groupManager;
+
+	/** @var string[] */
+	protected $groupDisplayNames = [];
+
+	/**
+	 * @param IFactory $languageFactory
+	 * @param IURLGenerator $url
+	 * @param IManager $activityManager
+	 * @param IUserManager $userManager
+	 * @param IGroupManager $groupManager
+	 */
+	public function __construct(IFactory $languageFactory, IURLGenerator $url, IManager $activityManager, IUserManager $userManager, IGroupManager $groupManager) {
+		parent::__construct($languageFactory, $url, $activityManager, $userManager);
+		$this->groupManager = $groupManager;
+	}
+
 	/**
 	 * @param IEvent $event
 	 * @return IEvent
@@ -103,24 +127,44 @@ class Groups extends Base {
 			case self::SUBJECT_UNSHARED_GROUP_BY:
 				return [
 					'file' => $this->getFile($parameters[0], $event),
-					'group' => [
-						'type' => 'group',
-						'id' => $parameters[2],
-						'name' => $parameters[2],
-					],
+					'group' => $this->generateGroupParameter($parameters[2]),
 					'actor' => $this->getUser($parameters[1]),
 				];
 			case self::SUBJECT_SHARED_GROUP_SELF:
 			case self::SUBJECT_UNSHARED_GROUP_SELF:
 				return [
 					'file' => $this->getFile($parameters[0], $event),
-					'group' => [
-						'type' => 'group',
-						'id' => $parameters[1],
-						'name' => $parameters[1],
-					],
+					'group' => $this->generateGroupParameter($parameters[1]),
 				];
 		}
 		return [];
 	}
+
+	/**
+	 * @param string $gid
+	 * @return array
+	 */
+	protected function generateGroupParameter($gid) {
+		if (!isset($this->groupDisplayNames[$gid])) {
+			$this->groupDisplayNames[$gid] = $this->getGroupDisplayName($gid);
+		}
+
+		return [
+			'type' => 'group',
+			'id' => $gid,
+			'name' => $this->groupDisplayNames[$gid],
+		];
+	}
+
+	/**
+	 * @param string $gid
+	 * @return string
+	 */
+	protected function getGroupDisplayName($gid) {
+		$group = $this->groupManager->get($gid);
+		if ($group instanceof IGroup) {
+			return $group->getDisplayName();
+		}
+		return $gid;
+	}
 }
diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php
index 60b8393e1e4..087e65a1bca 100644
--- a/apps/provisioning_api/lib/Controller/UsersController.php
+++ b/apps/provisioning_api/lib/Controller/UsersController.php
@@ -790,9 +790,10 @@ class UsersController extends OCSController {
 		}
 
 		// Get the subadmin groups
-		$groups = $this->groupManager->getSubAdmin()->getSubAdminsGroups($user);
-		foreach ($groups as $key => $group) {
-			$groups[$key] = $group->getGID();
+		$subAdminGroups = $this->groupManager->getSubAdmin()->getSubAdminsGroups($user);
+		$groups = [];
+		foreach ($subAdminGroups as $key => $group) {
+			$groups[] = $group->getGID();
 		}
 
 		if(!$groups) {
diff --git a/apps/workflowengine/js/admin.js b/apps/workflowengine/js/admin.js
index ab122a8cd65..891e50c4189 100644
--- a/apps/workflowengine/js/admin.js
+++ b/apps/workflowengine/js/admin.js
@@ -149,6 +149,7 @@
 			message: '',
 			errorMessage: '',
 			saving: false,
+			groups: [],
 			initialize: function() {
 				// this creates a new copy of the object to definitely have a new reference and being able to reset the model
 				this.originalModel = JSON.parse(JSON.stringify(this.model));
@@ -161,6 +162,25 @@
 				if (this.model.get('id') === undefined) {
 					this.hasChanged = true;
 				}
+				var self = this;
+				$.ajax({
+					url: OC.generateUrl('settings/users/groups'),
+					dataType: 'json',
+					quietMillis: 100,
+				}).success(function(response) {
+					// add admin groups
+					$.each(response.data.adminGroups, function(id, group) {
+						self.groups.push({ id: group.id, displayname: group.name });
+					});
+					// add groups
+					$.each(response.data.groups, function(id, group) {
+						self.groups.push({ id: group.id, displayname: group.name });
+					});
+					self.render();
+				}).error(function(data) {
+					OC.Notification.error(t('workflowengine', 'Unable to retrieve the group list'), {type: 'error'});
+					console.log(data);
+				});
 			},
 			delete: function() {
 				if (OC.PasswordConfirmation.requiresPasswordConfirmation()) {
@@ -304,10 +324,11 @@
 						id = $element.data('id'),
 						check = checks[id],
 						valueElement = $element.find('.check-value').first();
+						var self = this;
 
 					_.each(OCA.WorkflowEngine.availablePlugins, function(plugin) {
 						if (_.isFunction(plugin.render)) {
-							plugin.render(valueElement, check);
+							plugin.render(valueElement, check, self.groups);
 						}
 					});
 				}, this);
diff --git a/apps/workflowengine/js/usergroupmembershipplugin.js b/apps/workflowengine/js/usergroupmembershipplugin.js
index 1c09e7d5ccd..53f35fedf2d 100644
--- a/apps/workflowengine/js/usergroupmembershipplugin.js
+++ b/apps/workflowengine/js/usergroupmembershipplugin.js
@@ -34,7 +34,7 @@
 				]
 			};
 		},
-		render: function(element, check) {
+		render: function(element, check, groups) {
 			if (check['class'] !== 'OCA\\WorkflowEngine\\Check\\UserGroupMembership') {
 				return;
 			}
@@ -42,50 +42,30 @@
 			$(element).css('width', '400px');
 
 			$(element).select2({
-				ajax: {
-					url: OC.generateUrl('settings/users/groups'),
-					dataType: 'json',
-					quietMillis: 100,
-					data: function (term) {
-						return {
-							pattern: term, //search term
-							filterGroups: true,
-							sortGroups: 2 // by groupname
-						};
-					},
-					results: function (response) {
-						// TODO improve error case
-						if (response.data === undefined) {
-							console.error('Failure happened', response);
-							return;
-						}
-
-						var results = [];
-
-						// add admin groups
-						$.each(response.data.adminGroups, function(id, group) {
-							results.push({ id: group.id });
+				data: { results: groups, text: 'displayname' },
+				initSelection: function (element, callback) {
+					var groupId = element.val();
+					if (groupId && groups.length > 0) {
+						callback({
+							id: groupId,
+							displayname: groups.find(function (group) {
+								return group.id === groupId;
+							}).displayname
 						});
-						// add groups
-						$.each(response.data.groups, function(id, group) {
-							results.push({ id: group.id });
+					} else if (groupId) {
+						callback({
+							id: groupId,
+							displayname: groupId
 						});
-
-						// TODO once limit and offset is implemented for groups we should paginate the search results
-						return {
-							results: results,
-							more: false
-						};
+					} else {
+						callback();
 					}
 				},
-				initSelection: function (element, callback) {
-					callback({id: element.val()});
-				},
 				formatResult: function (element) {
-					return '<span>' + escapeHTML(element.id) + '</span>';
+					return '<span>' + escapeHTML(element.displayname) + '</span>';
 				},
 				formatSelection: function (element) {
-					return '<span title="'+escapeHTML(element.id)+'">'+escapeHTML(element.id)+'</span>';
+					return '<span title="'+escapeHTML(element.id)+'">'+escapeHTML(element.displayname)+'</span>';
 				}
 			});
 		}