From 97dbaa26c4d8a6f77794fd0f04348654fe529063 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 24 Jan 2018 13:07:47 +0100 Subject: [PATCH 1/5] Show the displayname in the users group list Signed-off-by: Joas Schilling --- lib/private/Settings/Personal/PersonalInfo.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Settings/Personal/PersonalInfo.php b/lib/private/Settings/Personal/PersonalInfo.php index 9e2efa6c04095..05175d20a2b02 100644 --- a/lib/private/Settings/Personal/PersonalInfo.php +++ b/lib/private/Settings/Personal/PersonalInfo.php @@ -174,7 +174,7 @@ public function getPriority() { private function getGroups(IUser $user) { $groups = array_map( function(IGroup $group) { - return $group->getGID(); + return $group->getDisplayName(); }, $this->groupManager->getUserGroups($user) ); From 63089821a4b1b0f5bb16be62f70fcaf90c3ed3f9 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 24 Jan 2018 13:39:17 +0100 Subject: [PATCH 2/5] Correctly return the group name Signed-off-by: Joas Schilling --- lib/private/Group/MetaData.php | 2 +- settings/Controller/GroupsController.php | 15 +++++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/lib/private/Group/MetaData.php b/lib/private/Group/MetaData.php index d5c8b581f8bcd..99594301990d6 100644 --- a/lib/private/Group/MetaData.php +++ b/lib/private/Group/MetaData.php @@ -160,7 +160,7 @@ private function addEntry(&$entries, &$sortKeys, &$sortIndex, $data) { 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/settings/Controller/GroupsController.php b/settings/Controller/GroupsController.php index 8985a76ec956b..19b7c53f8b9ed 100644 --- a/settings/Controller/GroupsController.php +++ b/settings/Controller/GroupsController.php @@ -28,6 +28,7 @@ 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 @@ public function create($id) { 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 @@ public function destroy($id) { return new DataResponse( array( 'status' => 'success', - 'data' => array( - 'groupname' => $id - ) + 'data' => ['groupname' => $group->getDisplayName()] ), Http::STATUS_NO_CONTENT ); From 2609b30df0694f7d03e886c1782df54d4c9b01ac Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 24 Jan 2018 13:54:49 +0100 Subject: [PATCH 3/5] Fix group displaynames in activity Signed-off-by: Joas Schilling --- .../dav/lib/CalDAV/Activity/Provider/Base.php | 67 +++++++++++++------ .../lib/CalDAV/Activity/Provider/Calendar.php | 6 +- .../lib/CalDAV/Activity/Provider/Event.php | 6 +- .../lib/Activity/Providers/Groups.php | 64 +++++++++++++++--- 4 files changed, 108 insertions(+), 35 deletions(-) diff --git a/apps/dav/lib/CalDAV/Activity/Provider/Base.php b/apps/dav/lib/CalDAV/Activity/Provider/Base.php index b6d8a5be7369c..99ad903f2479d 100644 --- a/apps/dav/lib/CalDAV/Activity/Provider/Base.php +++ b/apps/dav/lib/CalDAV/Activity/Provider/Base.php @@ -26,6 +26,8 @@ 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; } /** @@ -112,31 +122,19 @@ protected function generateLegacyCalendarParameter($id, $name) { ]; } - /** - * @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 @@ protected function generateUserParameter($uid) { * @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 ff12914428538..db79b0f66568e 100644 --- a/apps/dav/lib/CalDAV/Activity/Provider/Calendar.php +++ b/apps/dav/lib/CalDAV/Activity/Provider/Calendar.php @@ -26,6 +26,7 @@ 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 eabd2e517c07c..f13cb0c266b1a 100644 --- a/apps/dav/lib/CalDAV/Activity/Provider/Event.php +++ b/apps/dav/lib/CalDAV/Activity/Provider/Event.php @@ -26,6 +26,7 @@ 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/files_sharing/lib/Activity/Providers/Groups.php b/apps/files_sharing/lib/Activity/Providers/Groups.php index 53262e19311b6..9a8f7164c55d7 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 @@ protected function getParsedParameters(IEvent $event) { 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; + } } From ae369e870dea914c1d2d4cfac6d29dc4f51c4709 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6=20=28skjnldsv=29?= Date: Mon, 5 Mar 2018 10:10:05 +0100 Subject: [PATCH 4/5] Fixed caldav tests and metadata 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ (skjnldsv) --- apps/dav/tests/unit/CalDAV/Activity/Provider/BaseTest.php | 8 +++++++- tests/lib/Group/MetaDataTest.php | 6 +++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/apps/dav/tests/unit/CalDAV/Activity/Provider/BaseTest.php b/apps/dav/tests/unit/CalDAV/Activity/Provider/BaseTest.php index affc1909e3f6d..37a56f88042da 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\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/tests/lib/Group/MetaDataTest.php b/tests/lib/Group/MetaDataTest.php index 04d2ff807b479..02c881b2e2521 100644 --- a/tests/lib/Group/MetaDataTest.php +++ b/tests/lib/Group/MetaDataTest.php @@ -56,9 +56,9 @@ private function getGroupMock($countCallCount = 0) { $group->expects($this->exactly(9)) ->method('getGID') ->will($this->onConsecutiveCalls( - 'admin', 'admin', 'admin', - 'g2', 'g2', 'g2', - 'g3', 'g3', 'g3')); + 'admin', 'admin', + 'g2', 'g2', + 'g3', 'g3')); $group->expects($this->exactly($countCallCount)) ->method('count') From 5e15c76d214d6d1425bf05ceaa937371e71eaa38 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 12 Mar 2018 12:25:55 +0100 Subject: [PATCH 5/5] Fix unit tests Signed-off-by: Joas Schilling --- .../Controller/GroupsControllerTest.php | 56 ++++++++++++++----- tests/lib/Group/MetaDataTest.php | 13 ++++- 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/tests/Settings/Controller/GroupsControllerTest.php b/tests/Settings/Controller/GroupsControllerTest.php index ecbfa9ea05e0a..43853d81fcf69 100644 --- a/tests/Settings/Controller/GroupsControllerTest.php +++ b/tests/Settings/Controller/GroupsControllerTest.php @@ -16,6 +16,7 @@ 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; @@ -66,6 +67,9 @@ public function testIndexSortByName() { $firstGroup ->method('getGID') ->will($this->returnValue('firstGroup')); + $firstGroup + ->method('getDisplayName') + ->will($this->returnValue('1st group')); $firstGroup ->method('count') ->will($this->returnValue(12)); @@ -74,6 +78,9 @@ public function testIndexSortByName() { $secondGroup ->method('getGID') ->will($this->returnValue('secondGroup')); + $secondGroup + ->method('getDisplayName') + ->will($this->returnValue('2nd group')); $secondGroup ->method('count') ->will($this->returnValue(25)); @@ -82,6 +89,9 @@ public function testIndexSortByName() { $thirdGroup ->method('getGID') ->will($this->returnValue('thirdGroup')); + $thirdGroup + ->method('getDisplayName') + ->will($this->returnValue('3rd group')); $thirdGroup ->method('count') ->will($this->returnValue(14)); @@ -90,6 +100,9 @@ public function testIndexSortByName() { $fourthGroup ->method('getGID') ->will($this->returnValue('admin')); + $fourthGroup + ->method('getDisplayName') + ->will($this->returnValue('Administrators')); $fourthGroup ->method('count') ->will($this->returnValue(18)); @@ -119,7 +132,7 @@ public function testIndexSortByName() { 'adminGroups' => array( 0 => array( 'id' => 'admin', - 'name' => 'admin', + 'name' => 'Administrators', 'usercount' => 0,//User count disabled 18, ) ), @@ -127,17 +140,17 @@ public function testIndexSortByName() { 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, ), ) @@ -158,6 +171,9 @@ public function testIndexSortbyCount() { $firstGroup ->method('getGID') ->will($this->returnValue('firstGroup')); + $firstGroup + ->method('getDisplayName') + ->will($this->returnValue('1st group')); $firstGroup ->method('count') ->will($this->returnValue(12)); @@ -166,6 +182,9 @@ public function testIndexSortbyCount() { $secondGroup ->method('getGID') ->will($this->returnValue('secondGroup')); + $secondGroup + ->method('getDisplayName') + ->will($this->returnValue('2nd group')); $secondGroup ->method('count') ->will($this->returnValue(25)); @@ -174,6 +193,9 @@ public function testIndexSortbyCount() { $thirdGroup ->method('getGID') ->will($this->returnValue('thirdGroup')); + $thirdGroup + ->method('getDisplayName') + ->will($this->returnValue('3rd group')); $thirdGroup ->method('count') ->will($this->returnValue(14)); @@ -182,6 +204,9 @@ public function testIndexSortbyCount() { $fourthGroup ->method('getGID') ->will($this->returnValue('admin')); + $fourthGroup + ->method('getDisplayName') + ->will($this->returnValue('Administrators')); $fourthGroup ->method('count') ->will($this->returnValue(18)); @@ -212,7 +237,7 @@ public function testIndexSortbyCount() { 'adminGroups' => array( 0 => array( 'id' => 'admin', - 'name' => 'admin', + 'name' => 'Administrators', 'usercount' => 18, ) ), @@ -220,17 +245,17 @@ public function testIndexSortbyCount() { 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 @@ public function testCreateSuccessful() { ->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 @@ public function testCreateUnsuccessful() { } 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 @@ public function testDestroySuccessful() { $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 02c881b2e2521..4e6389aad6b18 100644 --- a/tests/lib/Group/MetaDataTest.php +++ b/tests/lib/Group/MetaDataTest.php @@ -53,13 +53,20 @@ private function getGroupMock($countCallCount = 0) { ->disableOriginalConstructor() ->getMock(); - $group->expects($this->exactly(9)) + $group->expects($this->exactly(6)) ->method('getGID') ->will($this->onConsecutiveCalls( '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') ->with('') @@ -83,7 +90,7 @@ public function testGet() { $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 @@ public function testGetWithSorting() { $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']); }