Merge pull request #8779 from nextcloud/backport/8255/show-group-display-names

[stable13] Show group display names

10 files changed, 178 insertions, 67 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/lib/private/Group/MetaData.php b/lib/private/Group/MetaData.php
index d5c8b581f8b..99594301990 100644
--- a/lib/private/Group/MetaData.php
+++ b/lib/private/Group/MetaData.php
@@ -160,7 +160,7 @@ class MetaData {
 	private function generateGroupMetaData(\OCP\IGroup $group, $userSearch) {
 		return array(
 				'id' => $group->getGID(),
-				'name' => $group->getGID(),
+				'name' => $group->getDisplayName(),
 				'usercount' => $this->sorting === self::SORT_USERCOUNT ? $group->count($userSearch) : 0,
 			);
 	}
diff --git a/lib/private/Settings/Personal/PersonalInfo.php b/lib/private/Settings/Personal/PersonalInfo.php
index 9e2efa6c040..05175d20a2b 100644
--- a/lib/private/Settings/Personal/PersonalInfo.php
+++ b/lib/private/Settings/Personal/PersonalInfo.php
@@ -174,7 +174,7 @@ class PersonalInfo implements ISettings {
 	private function getGroups(IUser $user) {
 		$groups = array_map(
 			function(IGroup $group) {
-				return $group->getGID();
+				return $group->getDisplayName();
 			},
 			$this->groupManager->getUserGroups($user)
 		);
diff --git a/settings/Controller/GroupsController.php b/settings/Controller/GroupsController.php
index 8985a76ec95..19b7c53f8b9 100644
--- a/settings/Controller/GroupsController.php
+++ b/settings/Controller/GroupsController.php
@@ -28,6 +28,7 @@ use OC\AppFramework\Http;
 use OC\Group\MetaData;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\DataResponse;
+use OCP\IGroup;
 use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IRequest;
@@ -108,13 +109,9 @@ class GroupsController extends Controller {
 				Http::STATUS_CONFLICT
 			);
 		}
-		if($this->groupManager->createGroup($id)) {
-			return new DataResponse(
-				array(
-					'groupname' => $id
-				),
-				Http::STATUS_CREATED
-			);
+		$group = $this->groupManager->createGroup($id);
+		if($group instanceof IGroup) {
+			return new DataResponse(['groupname' => $group->getDisplayName()], Http::STATUS_CREATED);
 		}
 
 		return new DataResponse(
@@ -140,9 +137,7 @@ class GroupsController extends Controller {
 				return new DataResponse(
 					array(
 						'status' => 'success',
-						'data' => array(
-							'groupname' => $id
-						)
+						'data' => ['groupname' => $group->getDisplayName()]
 					),
 					Http::STATUS_NO_CONTENT
 				);
diff --git a/tests/Settings/Controller/GroupsControllerTest.php b/tests/Settings/Controller/GroupsControllerTest.php
index ecbfa9ea05e..43853d81fcf 100644
--- a/tests/Settings/Controller/GroupsControllerTest.php
+++ b/tests/Settings/Controller/GroupsControllerTest.php
@@ -16,6 +16,7 @@ use OC\Settings\Controller\GroupsController;
 use OC\User\User;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
+use OCP\IGroup;
 use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\IRequest;
@@ -67,6 +68,9 @@ class GroupsControllerTest extends \Test\TestCase {
 			->method('getGID')
 			->will($this->returnValue('firstGroup'));
 		$firstGroup
+			->method('getDisplayName')
+			->will($this->returnValue('1st group'));
+		$firstGroup
 			->method('count')
 			->will($this->returnValue(12));
 		$secondGroup = $this->getMockBuilder(Group::class)
@@ -75,6 +79,9 @@ class GroupsControllerTest extends \Test\TestCase {
 			->method('getGID')
 			->will($this->returnValue('secondGroup'));
 		$secondGroup
+			->method('getDisplayName')
+			->will($this->returnValue('2nd group'));
+		$secondGroup
 			->method('count')
 			->will($this->returnValue(25));
 		$thirdGroup = $this->getMockBuilder(Group::class)
@@ -83,6 +90,9 @@ class GroupsControllerTest extends \Test\TestCase {
 			->method('getGID')
 			->will($this->returnValue('thirdGroup'));
 		$thirdGroup
+			->method('getDisplayName')
+			->will($this->returnValue('3rd group'));
+		$thirdGroup
 			->method('count')
 			->will($this->returnValue(14));
 		$fourthGroup = $this->getMockBuilder(Group::class)
@@ -91,6 +101,9 @@ class GroupsControllerTest extends \Test\TestCase {
 			->method('getGID')
 			->will($this->returnValue('admin'));
 		$fourthGroup
+			->method('getDisplayName')
+			->will($this->returnValue('Administrators'));
+		$fourthGroup
 			->method('count')
 			->will($this->returnValue(18));
 		/** @var \OC\Group\Group[] $groups */
@@ -119,7 +132,7 @@ class GroupsControllerTest extends \Test\TestCase {
 				'adminGroups' => array(
 					0 => array(
 						'id' => 'admin',
-						'name' => 'admin',
+						'name' => 'Administrators',
 						'usercount' => 0,//User count disabled 18,
 					)
 				),
@@ -127,17 +140,17 @@ class GroupsControllerTest extends \Test\TestCase {
 					array(
 						0 => array(
 							'id' => 'firstGroup',
-							'name' => 'firstGroup',
+							'name' => '1st group',
 							'usercount' => 0,//User count disabled 12,
 						),
 						1 => array(
 							'id' => 'secondGroup',
-							'name' => 'secondGroup',
+							'name' => '2nd group',
 							'usercount' => 0,//User count disabled 25,
 						),
 						2 => array(
 							'id' => 'thirdGroup',
-							'name' => 'thirdGroup',
+							'name' => '3rd group',
 							'usercount' => 0,//User count disabled 14,
 						),
 					)
@@ -159,6 +172,9 @@ class GroupsControllerTest extends \Test\TestCase {
 			->method('getGID')
 			->will($this->returnValue('firstGroup'));
 		$firstGroup
+			->method('getDisplayName')
+			->will($this->returnValue('1st group'));
+		$firstGroup
 			->method('count')
 			->will($this->returnValue(12));
 		$secondGroup = $this->getMockBuilder(Group::class)
@@ -167,6 +183,9 @@ class GroupsControllerTest extends \Test\TestCase {
 			->method('getGID')
 			->will($this->returnValue('secondGroup'));
 		$secondGroup
+			->method('getDisplayName')
+			->will($this->returnValue('2nd group'));
+		$secondGroup
 			->method('count')
 			->will($this->returnValue(25));
 		$thirdGroup = $this->getMockBuilder(Group::class)
@@ -175,6 +194,9 @@ class GroupsControllerTest extends \Test\TestCase {
 			->method('getGID')
 			->will($this->returnValue('thirdGroup'));
 		$thirdGroup
+			->method('getDisplayName')
+			->will($this->returnValue('3rd group'));
+		$thirdGroup
 			->method('count')
 			->will($this->returnValue(14));
 		$fourthGroup = $this->getMockBuilder(Group::class)
@@ -183,6 +205,9 @@ class GroupsControllerTest extends \Test\TestCase {
 			->method('getGID')
 			->will($this->returnValue('admin'));
 		$fourthGroup
+			->method('getDisplayName')
+			->will($this->returnValue('Administrators'));
+		$fourthGroup
 			->method('count')
 			->will($this->returnValue(18));
 		/** @var \OC\Group\Group[] $groups */
@@ -212,7 +237,7 @@ class GroupsControllerTest extends \Test\TestCase {
 				'adminGroups' => array(
 					0 => array(
 						'id' => 'admin',
-						'name' => 'admin',
+						'name' => 'Administrators',
 						'usercount' => 18,
 					)
 				),
@@ -220,17 +245,17 @@ class GroupsControllerTest extends \Test\TestCase {
 					array(
 						0 => array(
 							'id' => 'secondGroup',
-							'name' => 'secondGroup',
+							'name' => '2nd group',
 							'usercount' => 25,
 						),
 						1 => array(
 							'id' => 'thirdGroup',
-							'name' => 'thirdGroup',
+							'name' => '3rd group',
 							'usercount' => 14,
 						),
 						2 => array(
 							'id' => 'firstGroup',
-							'name' => 'firstGroup',
+							'name' => '1st group',
 							'usercount' => 12,
 						),
 					)
@@ -264,15 +289,19 @@ class GroupsControllerTest extends \Test\TestCase {
 			->method('groupExists')
 			->with('NewGroup')
 			->will($this->returnValue(false));
+
+		$group = $this->createMock(IGroup::class);
+		$group->method('getDisplayName')
+			->willReturn('New group');
 		$this->groupManager
 			->expects($this->once())
 			->method('createGroup')
 			->with('NewGroup')
-			->will($this->returnValue(true));
+			->willReturn($group);
 
 		$expectedResponse = new DataResponse(
 			array(
-				'groupname' => 'NewGroup'
+				'groupname' => 'New group'
 			),
 			Http::STATUS_CREATED
 		);
@@ -304,13 +333,14 @@ class GroupsControllerTest extends \Test\TestCase {
 	}
 
 	public function testDestroySuccessful() {
-		$group = $this->getMockBuilder(Group::class)
-			->disableOriginalConstructor()->getMock();
+		$group = $this->createMock(IGroup::class);
 		$this->groupManager
 			->expects($this->once())
 			->method('get')
 			->with('ExistingGroup')
 			->will($this->returnValue($group));
+		$group->method('getDisplayName')
+			->willReturn('Existing group');
 		$group
 			->expects($this->once())
 			->method('delete')
@@ -319,7 +349,7 @@ class GroupsControllerTest extends \Test\TestCase {
 		$expectedResponse = new DataResponse(
 			array(
 				'status' => 'success',
-				'data' => array('groupname' => 'ExistingGroup')
+				'data' => array('groupname' => 'Existing group')
 			),
 			Http::STATUS_NO_CONTENT
 		);
diff --git a/tests/lib/Group/MetaDataTest.php b/tests/lib/Group/MetaDataTest.php
index 04d2ff807b4..4e6389aad6b 100644
--- a/tests/lib/Group/MetaDataTest.php
+++ b/tests/lib/Group/MetaDataTest.php
@@ -53,12 +53,19 @@ class MetaDataTest extends \Test\TestCase {
 			->disableOriginalConstructor()
 			->getMock();
 
-		$group->expects($this->exactly(9))
+		$group->expects($this->exactly(6))
 			->method('getGID')
 			->will($this->onConsecutiveCalls(
-				'admin', 'admin', 'admin',
-				'g2', 'g2', 'g2',
-				'g3', 'g3', 'g3'));
+				'admin', 'admin',
+				'g2', 'g2',
+				'g3', 'g3'));
+
+		$group->expects($this->exactly(3))
+			->method('getDisplayName')
+			->will($this->onConsecutiveCalls(
+				'display admin',
+				'display g2',
+				'display g3'));
 
 		$group->expects($this->exactly($countCallCount))
 			->method('count')
@@ -83,7 +90,7 @@ class MetaDataTest extends \Test\TestCase {
 		$this->assertSame(1, count($adminGroups));
 		$this->assertSame(2, count($ordinaryGroups));
 
-		$this->assertSame('g2', $ordinaryGroups[0]['name']);
+		$this->assertSame('display g2', $ordinaryGroups[0]['name']);
 		// user count is not loaded
 		$this->assertSame(0, $ordinaryGroups[0]['usercount']);
 	}
@@ -103,7 +110,7 @@ class MetaDataTest extends \Test\TestCase {
 		$this->assertSame(1, count($adminGroups));
 		$this->assertSame(2, count($ordinaryGroups));
 
-		$this->assertSame('g3', $ordinaryGroups[0]['name']);
+		$this->assertSame('display g3', $ordinaryGroups[0]['name']);
 		$this->assertSame(5, $ordinaryGroups[0]['usercount']);
 	}