From 9ffc77a9b9ccf1a9cb014b24a4f6e5c6ea60e53c Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 30 Nov 2016 20:21:44 +0100 Subject: [PATCH 1/3] Adding group display name support --- lib/private/Group/Backend.php | 11 +- lib/private/Group/Group.php | 11 +- lib/private/Group/Manager.php | 16 +- lib/public/GroupInterface.php | 12 ++ lib/public/IGroup.php | 9 + tests/lib/Group/ManagerTest.php | 361 ++++++++++++++++++++------------ 6 files changed, 270 insertions(+), 150 deletions(-) diff --git a/lib/private/Group/Backend.php b/lib/private/Group/Backend.php index 38ff54e36468..ad1d697ef925 100644 --- a/lib/private/Group/Backend.php +++ b/lib/private/Group/Backend.php @@ -30,22 +30,13 @@ abstract class Backend implements \OCP\GroupInterface { */ const NOT_IMPLEMENTED = -501; - /** - * actions that user backends can define - */ - const CREATE_GROUP = 0x00000001; - const DELETE_GROUP = 0x00000010; - const ADD_TO_GROUP = 0x00000100; - const REMOVE_FROM_GOUP = 0x00001000; - //OBSOLETE const GET_DISPLAYNAME = 0x00010000; - const COUNT_USERS = 0x00100000; - protected $possibleActions = [ self::CREATE_GROUP => 'createGroup', self::DELETE_GROUP => 'deleteGroup', self::ADD_TO_GROUP => 'addToGroup', self::REMOVE_FROM_GOUP => 'removeFromGroup', self::COUNT_USERS => 'countUsersInGroup', + self::GROUP_DETAILS => 'getGroupDetails', ]; /** diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index 355c24730c06..2d96be1c195c 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -65,18 +65,27 @@ class Group implements IGroup { * @param \OC\Group\Backend[] $backends * @param \OC\User\Manager $userManager * @param \OC\Hooks\PublicEmitter $emitter + * @param string $displayName */ - public function __construct($gid, $backends, $userManager, $emitter = null) { + public function __construct($gid, $backends, $userManager, $emitter = null, $displayName = null) { $this->gid = $gid; $this->backends = $backends; $this->userManager = $userManager; $this->emitter = $emitter; + $this->displayName = $displayName; } public function getGID() { return $this->gid; } + public function getDisplayName() { + if (is_null($this->displayName)) { + return $this->gid; + } + return $this->displayName; + } + /** * get all users in the group * diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index fcc88b5e0ee1..e84bde07d7ef 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -154,19 +154,29 @@ public function get($gid) { /** * @param string $gid + * @param string $displayName * @return \OCP\IGroup */ - protected function getGroupObject($gid) { + protected function getGroupObject($gid, $displayName = null) { $backends = []; foreach ($this->backends as $backend) { - if ($backend->groupExists($gid)) { + if ($backend->implementsActions(\OC\Group\Backend::GROUP_DETAILS)) { + $groupData = $backend->getGroupDetails($gid); + if (is_array($groupData)) { + // take the display name from the first backend that has a non-null one + if (is_null($displayName) && isset($groupData['displayName'])) { + $displayName = $groupData['displayName']; + } + $backends[] = $backend; + } + } else if ($backend->groupExists($gid)) { $backends[] = $backend; } } if (count($backends) === 0) { return null; } - $this->cachedGroups[$gid] = new Group($gid, $backends, $this->userManager, $this); + $this->cachedGroups[$gid] = new Group($gid, $backends, $this->userManager, $this, $displayName); return $this->cachedGroups[$gid]; } diff --git a/lib/public/GroupInterface.php b/lib/public/GroupInterface.php index 71f3fe9cdca8..b39285cf250e 100644 --- a/lib/public/GroupInterface.php +++ b/lib/public/GroupInterface.php @@ -39,6 +39,18 @@ */ interface GroupInterface { + /** + * actions that user backends can define + */ + const CREATE_GROUP = 0x00000001; + const DELETE_GROUP = 0x00000010; + const ADD_TO_GROUP = 0x00000100; + const REMOVE_FROM_GOUP = 0x00001000; // oops + const REMOVE_FROM_GROUP = 0x00001000; + //OBSOLETE const GET_DISPLAYNAME = 0x00010000; + const COUNT_USERS = 0x00100000; + const GROUP_DETAILS = 0x01000000; + /** * Check if backend implements actions * @param int $actions bitwise-or'ed actions diff --git a/lib/public/IGroup.php b/lib/public/IGroup.php index f4c4beabca15..a8273adb1708 100644 --- a/lib/public/IGroup.php +++ b/lib/public/IGroup.php @@ -2,6 +2,7 @@ /** * @author Morris Jobke * @author Robin Appelman + * @author Vincent Petry * * @copyright Copyright (c) 2016, ownCloud GmbH. * @license AGPL-3.0 @@ -35,6 +36,14 @@ interface IGroup { */ public function getGID(); + /** + * Returns the group display name + * + * @return string + * @since 9.2 + */ + public function getDisplayName(); + /** * get all users in the group * diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 68b17861f922..7554533ab4e0 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -1,22 +1,81 @@ - * This file is licensed under the Affero General Public License version 3 or - * later. - * See the COPYING-README file. + * @author Robin Appelman + * @author Vincent Petry + * + * @copyright Copyright (c) 2016, ownCloud GmbH. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * */ - namespace Test\Group; -use OC\User\User; +use OCP\IUser; +use OCP\GroupInterface; class ManagerTest extends \Test\TestCase { + private function getTestUser($userId) { + $mockUser = $this->createMock(IUser::class); + $mockUser->expects($this->any()) + ->method('getUID') + ->will($this->returnValue($userId)); + $mockUser->expects($this->any()) + ->method('getDisplayName') + ->will($this->returnValue($userId)); + return $mockUser; + } + + private function getTestBackend($implementedActions = null) { + if (is_null($implementedActions)) { + $implementedActions = + GroupInterface::ADD_TO_GROUP | + GroupInterface::REMOVE_FROM_GOUP | + GroupInterface::COUNT_USERS | + GroupInterface::CREATE_GROUP | + GroupInterface::DELETE_GROUP; + } + // need to declare it this way due to optional methods + // thanks to the implementsActions logic + $backend = $this->getMockBuilder(\OCP\GroupInterface::class) + ->disableOriginalConstructor() + ->setMethods([ + 'getGroupDetails', + 'implementsActions', + 'getUserGroups', + 'inGroup', + 'getGroups', + 'groupExists', + 'usersInGroup', + 'createGroup', + 'addToGroup', + 'removeFromGroup', + ]) + ->getMock(); + $backend->expects($this->any()) + ->method('implementsActions') + ->will($this->returnCallback(function($actions) use ($implementedActions) { + return (bool)($actions & $implementedActions); + })); + return $backend; + } + public function testGet() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('groupExists') ->with('group1') @@ -48,7 +107,7 @@ public function testGetNotExists() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('groupExists') ->with('group1') @@ -84,7 +143,7 @@ public function testGetMultipleBackends() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 */ - $backend1 = $this->createMock('\OC\Group\Database'); + $backend1 = $this->getTestBackend(); $backend1->expects($this->any()) ->method('groupExists') ->with('group1') @@ -93,7 +152,7 @@ public function testGetMultipleBackends() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend2 */ - $backend2 = $this->createMock('\OC\Group\Database'); + $backend2 = $this->getTestBackend(); $backend2->expects($this->any()) ->method('groupExists') ->with('group1') @@ -117,16 +176,13 @@ public function testCreate() { * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ $backendGroupCreated = false; - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('groupExists') ->with('group1') ->will($this->returnCallback(function () use (&$backendGroupCreated) { return $backendGroupCreated; })); - $backend->expects($this->once()) - ->method('implementsActions') - ->will($this->returnValue(true)); $backend->expects($this->once()) ->method('createGroup') ->will($this->returnCallback(function () use (&$backendGroupCreated) { @@ -148,7 +204,7 @@ public function testCreateExists() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('groupExists') ->with('group1') @@ -171,7 +227,7 @@ public function testSearch() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getGroups') ->with('1') @@ -198,7 +254,7 @@ public function testSearchMultipleBackends() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 */ - $backend1 = $this->createMock('\OC\Group\Database'); + $backend1 = $this->getTestBackend(); $backend1->expects($this->once()) ->method('getGroups') ->with('1') @@ -210,7 +266,7 @@ public function testSearchMultipleBackends() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend2 */ - $backend2 = $this->createMock('\OC\Group\Database'); + $backend2 = $this->getTestBackend(); $backend2->expects($this->once()) ->method('getGroups') ->with('1') @@ -239,7 +295,7 @@ public function testSearchMultipleBackendsLimitAndOffset() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 */ - $backend1 = $this->createMock('\OC\Group\Database'); + $backend1 = $this->getTestBackend(); $backend1->expects($this->once()) ->method('getGroups') ->with('1', 2, 1) @@ -251,7 +307,7 @@ public function testSearchMultipleBackendsLimitAndOffset() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend2 */ - $backend2 = $this->createMock('\OC\Group\Database'); + $backend2 = $this->getTestBackend(); $backend2->expects($this->once()) ->method('getGroups') ->with('1', 2, 1) @@ -280,7 +336,7 @@ public function testGetUserGroups() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -298,7 +354,7 @@ public function testGetUserGroups() { $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); - $groups = $manager->getUserGroups(new User('user1', $userBackend)); + $groups = $manager->getUserGroups($this->getTestUser('user1')); $this->assertEquals(1, count($groups)); $group1 = reset($groups); $this->assertEquals('group1', $group1->getGID()); @@ -334,7 +390,7 @@ public function testInGroup() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -358,7 +414,7 @@ public function testIsAdmin() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -382,7 +438,7 @@ public function testNotAdmin() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -406,7 +462,7 @@ public function testGetUserGroupsMultipleBackends() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 */ - $backend1 = $this->createMock('\OC\Group\Database'); + $backend1 = $this->getTestBackend(); $backend1->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -418,7 +474,7 @@ public function testGetUserGroupsMultipleBackends() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend2 */ - $backend2 = $this->createMock('\OC\Group\Database'); + $backend2 = $this->getTestBackend(); $backend2->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -436,7 +492,7 @@ public function testGetUserGroupsMultipleBackends() { $manager->addBackend($backend1); $manager->addBackend($backend2); - $groups = $manager->getUserGroups(new User('user1', $userBackend)); + $groups = $manager->getUserGroups($this->getTestUser('user1')); $this->assertEquals(2, count($groups)); $group1 = reset($groups); $group2 = next($groups); @@ -444,28 +500,28 @@ public function testGetUserGroupsMultipleBackends() { $this->assertEquals('group2', $group2->getGID()); } - public function testDisplayNamesInGroupWithOneUserBackend() { + public function testDisplayNamesInGroupWithOneUserBackend() { /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') ->with('testgroup') ->will($this->returnValue(true)); - $backend->expects($this->any()) - ->method('InGroup') + $backend->expects($this->any()) + ->method('inGroup') ->will($this->returnCallback(function($uid, $gid) { - switch($uid) { - case 'user1' : return false; - case 'user2' : return true; - case 'user3' : return false; - case 'user33': return true; - default: - return null; - } - })); + switch($uid) { + case 'user1' : return false; + case 'user2' : return true; + case 'user3' : return false; + case 'user33': return true; + default: + return null; + } + })); /** * @var \OC\User\Manager $userManager @@ -477,21 +533,21 @@ public function testDisplayNamesInGroupWithOneUserBackend() { ->method('searchDisplayName') ->with('user3') ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { - switch($offset) { - case 0 : return ['user3' => new User('user3', $userBackend), - 'user33' => new User('user33', $userBackend)]; - case 2 : return []; - } - })); + switch($offset) { + case 0 : return ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')]; + case 2 : return []; + } + })); $userManager->expects($this->any()) ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return new User('user1', $userBackend); - case 'user2' : return new User('user2', $userBackend); - case 'user3' : return new User('user3', $userBackend); - case 'user33': return new User('user33', $userBackend); + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); default: return null; } @@ -508,29 +564,29 @@ public function testDisplayNamesInGroupWithOneUserBackend() { $this->assertTrue(isset($users['user33'])); } - public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified() { + public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified() { /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') ->with('testgroup') ->will($this->returnValue(true)); - $backend->expects($this->any()) - ->method('InGroup') + $backend->expects($this->any()) + ->method('inGroup') ->will($this->returnCallback(function($uid, $gid) { - switch($uid) { - case 'user1' : return false; - case 'user2' : return true; - case 'user3' : return false; - case 'user33': return true; - case 'user333': return true; - default: - return null; - } - })); + switch($uid) { + case 'user1' : return false; + case 'user2' : return true; + case 'user3' : return false; + case 'user33': return true; + case 'user333': return true; + default: + return null; + } + })); /** * @var \OC\User\Manager $userManager @@ -542,22 +598,22 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified() { ->method('searchDisplayName') ->with('user3') ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { - switch($offset) { - case 0 : return ['user3' => new User('user3', $userBackend), - 'user33' => new User('user33', $userBackend)]; - case 2 : return ['user333' => new User('user333', $userBackend)]; - } - })); + switch($offset) { + case 0 : return ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')]; + case 2 : return ['user333' => $this->getTestUser('user333')]; + } + })); $userManager->expects($this->any()) ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return new User('user1', $userBackend); - case 'user2' : return new User('user2', $userBackend); - case 'user3' : return new User('user3', $userBackend); - case 'user33': return new User('user33', $userBackend); - case 'user333': return new User('user333', $userBackend); + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + case 'user333': return $this->getTestUser('user333'); default: return null; } @@ -577,27 +633,27 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified() { public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpecified() { /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') ->with('testgroup') ->will($this->returnValue(true)); - $backend->expects($this->any()) + $backend->expects($this->any()) ->method('inGroup') ->will($this->returnCallback(function($uid) { - switch($uid) { - case 'user1' : return false; - case 'user2' : return true; - case 'user3' : return false; - case 'user33': return true; - case 'user333': return true; - default: - return null; - } - })); + switch($uid) { + case 'user1' : return false; + case 'user2' : return true; + case 'user3' : return false; + case 'user33': return true; + case 'user333': return true; + default: + return null; + } + })); /** * @var \OC\User\Manager $userManager @@ -609,25 +665,25 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpeci ->method('searchDisplayName') ->with('user3') ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { - switch($offset) { - case 0 : - return [ - 'user3' => new User('user3', $userBackend), - 'user33' => new User('user33', $userBackend), - 'user333' => new User('user333', $userBackend) - ]; - } - })); + switch($offset) { + case 0 : + return [ + 'user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33'), + 'user333' => $this->getTestUser('user333') + ]; + } + })); $userManager->expects($this->any()) ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return new User('user1', $userBackend); - case 'user2' : return new User('user2', $userBackend); - case 'user3' : return new User('user3', $userBackend); - case 'user33': return new User('user33', $userBackend); - case 'user333': return new User('user333', $userBackend); + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + case 'user333': return $this->getTestUser('user333'); default: return null; } @@ -647,15 +703,15 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpeci public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') ->with('testgroup') ->will($this->returnValue(true)); - $backend->expects($this->once()) + $backend->expects($this->once()) ->method('usersInGroup') ->with('testgroup', '', -1, 0) ->will($this->returnValue(['user2', 'user33'])); @@ -670,10 +726,10 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return new User('user1', $userBackend); - case 'user2' : return new User('user2', $userBackend); - case 'user3' : return new User('user3', $userBackend); - case 'user33': return new User('user33', $userBackend); + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); default: return null; } @@ -692,9 +748,9 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitSpecified() { /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') ->with('testgroup') @@ -714,10 +770,10 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitS ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return new User('user1', $userBackend); - case 'user2' : return new User('user2', $userBackend); - case 'user3' : return new User('user3', $userBackend); - case 'user33': return new User('user33', $userBackend); + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); default: return null; } @@ -734,11 +790,11 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitS $this->assertFalse(isset($users['user33'])); } - public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitAndOffsetSpecified() { + public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitAndOffsetSpecified() { /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') ->with('testgroup') @@ -759,10 +815,10 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitA ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return new User('user1', $userBackend); - case 'user2' : return new User('user2', $userBackend); - case 'user3' : return new User('user3', $userBackend); - case 'user33': return new User('user33', $userBackend); + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); default: return null; } @@ -783,7 +839,7 @@ public function testGetUserGroupsWithAddUser() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $expectedGroups = []; $backend->expects($this->any()) ->method('getUserGroups') @@ -795,9 +851,6 @@ public function testGetUserGroupsWithAddUser() { ->method('groupExists') ->with('group1') ->will($this->returnValue(true)); - $backend->expects($this->once()) - ->method('implementsActions') - ->will($this->returnValue(true)); /** * @var \OC\User\Manager $userManager @@ -807,7 +860,7 @@ public function testGetUserGroupsWithAddUser() { $manager->addBackend($backend); // prime cache - $user1 = new User('user1', null); + $user1 = $this->getTestUser('user1'); $groups = $manager->getUserGroups($user1); $this->assertEquals([], $groups); @@ -827,7 +880,7 @@ public function testGetUserGroupsWithRemoveUser() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $expectedGroups = ['group1']; $backend->expects($this->any()) ->method('getUserGroups') @@ -839,9 +892,6 @@ public function testGetUserGroupsWithRemoveUser() { ->method('groupExists') ->with('group1') ->will($this->returnValue(true)); - $backend->expects($this->once()) - ->method('implementsActions') - ->will($this->returnValue(true)); $backend->expects($this->once()) ->method('inGroup') ->will($this->returnValue(true)); @@ -857,7 +907,7 @@ public function testGetUserGroupsWithRemoveUser() { $manager->addBackend($backend); // prime cache - $user1 = new User('user1', null); + $user1 = $this->getTestUser('user1'); $groups = $manager->getUserGroups($user1); $this->assertEquals(1, count($groups)); $group1 = reset($groups); @@ -877,7 +927,7 @@ public function testGetUserIdGroups() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('getUserGroups') ->with('user1') @@ -894,4 +944,43 @@ public function testGetUserIdGroups() { $this->assertEquals([], $groups); } + public function testGroupDisplayName() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend( + GroupInterface::ADD_TO_GROUP | + GroupInterface::REMOVE_FROM_GOUP | + GroupInterface::COUNT_USERS | + GroupInterface::CREATE_GROUP | + GroupInterface::DELETE_GROUP | + GroupInterface::GROUP_DETAILS + ); + $backend->expects($this->any()) + ->method('getGroupDetails') + ->will($this->returnValueMap([ + ['group1', ['gid' => 'group1', 'displayName' => 'Group One']], + ['group2', ['gid' => 'group2']], + ])); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->createMock('\OC\User\Manager'); + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + // group with display name + $group = $manager->get('group1'); + $this->assertNotNull($group); + $this->assertEquals('group1', $group->getGID()); + $this->assertEquals('Group One', $group->getDisplayName()); + + // group without display name + $group = $manager->get('group2'); + $this->assertNotNull($group); + $this->assertEquals('group2', $group->getGID()); + $this->assertEquals('group2', $group->getDisplayName()); + } + } From e4b55da071bdae17dc5aff1d7bdb15013b7664c8 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 30 Nov 2016 20:56:10 +0100 Subject: [PATCH 2/3] Use group display name in sharing API + UI --- apps/files_sharing/lib/API/Share20OCS.php | 3 +- .../lib/Controller/ShareesController.php | 30 +++++++---- .../tests/API/Share20OCSTest.php | 53 +++++++++++++++++-- apps/files_sharing/tests/API/ShareesTest.php | 42 ++++++++++++++- core/js/sharedialogresharerinfoview.js | 2 +- core/js/shareitemmodel.js | 8 +++ core/js/tests/specs/sharedialogviewSpec.js | 23 +++++++- core/js/tests/specs/shareitemmodelSpec.js | 5 ++ 8 files changed, 147 insertions(+), 19 deletions(-) diff --git a/apps/files_sharing/lib/API/Share20OCS.php b/apps/files_sharing/lib/API/Share20OCS.php index 9280124d99a6..64f09f4782d1 100644 --- a/apps/files_sharing/lib/API/Share20OCS.php +++ b/apps/files_sharing/lib/API/Share20OCS.php @@ -144,8 +144,9 @@ protected function formatShare(\OCP\Share\IShare $share) { $result['share_with'] = $share->getSharedWith(); $result['share_with_displayname'] = $sharedWith !== null ? $sharedWith->getDisplayName() : $share->getSharedWith(); } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) { + $group = $this->groupManager->get($share->getSharedWith()); $result['share_with'] = $share->getSharedWith(); - $result['share_with_displayname'] = $share->getSharedWith(); + $result['share_with_displayname'] = $group !== null ? $group->getDisplayName() : $share->getSharedWith(); } else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK) { $result['share_with'] = $share->getPassword(); diff --git a/apps/files_sharing/lib/Controller/ShareesController.php b/apps/files_sharing/lib/Controller/ShareesController.php index 9eb978deccbd..03c9a92551cc 100644 --- a/apps/files_sharing/lib/Controller/ShareesController.php +++ b/apps/files_sharing/lib/Controller/ShareesController.php @@ -158,9 +158,10 @@ protected function getUsers($search) { } $foundUserById = false; + $lowerSearch = strtolower($search); foreach ($users as $uid => $userDisplayName) { - if (strtolower($uid) === strtolower($search) || strtolower($userDisplayName) === strtolower($search)) { - if (strtolower($uid) === strtolower($search)) { + if (strtolower($uid) === $lowerSearch || strtolower($userDisplayName) === $lowerSearch) { + if (strtolower($uid) === $lowerSearch) { $foundUserById = true; } $this->result['exact']['users'][] = [ @@ -218,7 +219,7 @@ protected function getGroups($search) { $this->result['groups'] = $this->result['exact']['groups'] = []; $groups = $this->groupManager->search($search, $this->limit, $this->offset); - $groups = array_map(function (IGroup $group) { return $group->getGID(); }, $groups); + $groupIds = array_map(function (IGroup $group) { return $group->getGID(); }, $groups); if (!$this->shareeEnumeration || sizeof($groups) < $this->limit) { $this->reachedEndFor[] = 'groups'; @@ -229,13 +230,19 @@ protected function getGroups($search) { // Intersect all the groups that match with the groups this user is a member of $userGroups = $this->groupManager->getUserGroups($this->userSession->getUser()); $userGroups = array_map(function (IGroup $group) { return $group->getGID(); }, $userGroups); - $groups = array_intersect($groups, $userGroups); + $groupIds = array_intersect($groupIds, $userGroups); } - foreach ($groups as $gid) { - if (strtolower($gid) === strtolower($search)) { + $lowerSearch = strtolower($search); + foreach ($groups as $group) { + // FIXME: use a more efficient approach + $gid = $group->getGID(); + if (!in_array($gid, $groupIds)) { + continue; + } + if (strtolower($gid) === $lowerSearch || strtolower($group->getDisplayName()) === $lowerSearch) { $this->result['exact']['groups'][] = [ - 'label' => $gid, + 'label' => $group->getDisplayName(), 'value' => [ 'shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => $gid, @@ -243,7 +250,7 @@ protected function getGroups($search) { ]; } else { $this->result['groups'][] = [ - 'label' => $gid, + 'label' => $group->getDisplayName(), 'value' => [ 'shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => $gid, @@ -258,7 +265,7 @@ protected function getGroups($search) { $group = $this->groupManager->get($search); if ($group instanceof IGroup && (!$this->shareWithGroupOnly || in_array($group->getGID(), $userGroups))) { array_push($this->result['exact']['groups'], [ - 'label' => $group->getGID(), + 'label' => $group->getDisplayName(), 'value' => [ 'shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => $group->getGID(), @@ -292,10 +299,11 @@ protected function getRemote($search) { if (!is_array($cloudIds)) { $cloudIds = [$cloudIds]; } + $lowerSearch = strtolower($search); foreach ($cloudIds as $cloudId) { list(, $serverUrl) = $this->splitUserRemote($cloudId); - if (strtolower($contact['FN']) === strtolower($search) || strtolower($cloudId) === strtolower($search)) { - if (strtolower($cloudId) === strtolower($search)) { + if (strtolower($contact['FN']) === $lowerSearch || strtolower($cloudId) === $lowerSearch) { + if (strtolower($cloudId) === $lowerSearch) { $foundRemoteById = true; } $this->result['exact']['remotes'][] = [ diff --git a/apps/files_sharing/tests/API/Share20OCSTest.php b/apps/files_sharing/tests/API/Share20OCSTest.php index ed4718ab4b87..6aa8495a9c74 100644 --- a/apps/files_sharing/tests/API/Share20OCSTest.php +++ b/apps/files_sharing/tests/API/Share20OCSTest.php @@ -2080,9 +2080,10 @@ public function dataFormatShare() { ], $share, [], false ]; + // with existing group $share = \OC::$server->getShareManager()->newShare(); $share->setShareType(Share::SHARE_TYPE_GROUP) - ->setSharedWith('recipient') + ->setSharedWith('recipientGroup') ->setSharedBy('initiator') ->setShareOwner('owner') ->setPermissions(\OCP\Constants::PERMISSION_READ) @@ -2112,8 +2113,47 @@ public function dataFormatShare() { 'file_source' => 3, 'file_parent' => 1, 'file_target' => 'myTarget', - 'share_with' => 'recipient', - 'share_with_displayname' => 'recipient', + 'share_with' => 'recipientGroup', + 'share_with_displayname' => 'recipientGroupDisplayName', + 'mail_send' => 0, + 'mimetype' => 'myMimeType', + ], $share, [], false + ]; + + // with unknown group / no group backend + $share = \OC::$server->getShareManager()->newShare(); + $share->setShareType(Share::SHARE_TYPE_GROUP) + ->setSharedWith('recipientGroup2') + ->setSharedBy('initiator') + ->setShareOwner('owner') + ->setPermissions(\OCP\Constants::PERMISSION_READ) + ->setNode($file) + ->setShareTime(new \DateTime('2000-01-01T00:01:02')) + ->setTarget('myTarget') + ->setId(42); + $result[] = [ + [ + 'id' => 42, + 'share_type' => Share::SHARE_TYPE_GROUP, + 'uid_owner' => 'initiator', + 'displayname_owner' => 'initiator', + 'permissions' => 1, + 'stime' => 946684862, + 'parent' => null, + 'expiration' => null, + 'token' => null, + 'uid_file_owner' => 'owner', + 'displayname_file_owner' => 'owner', + 'path' => 'file', + 'item_type' => 'file', + 'storage_id' => 'storageId', + 'storage' => 100, + 'item_source' => 3, + 'file_source' => 3, + 'file_parent' => 1, + 'file_target' => 'myTarget', + 'share_with' => 'recipientGroup2', + 'share_with_displayname' => 'recipientGroup2', 'mail_send' => 0, 'mimetype' => 'myMimeType', ], $share, [], false @@ -2229,6 +2269,13 @@ public function dataFormatShare() { */ public function testFormatShare(array $expects, \OCP\Share\IShare $share, array $users, $exception) { $this->userManager->method('get')->will($this->returnValueMap($users)); + + $recipientGroup = $this->createMock('\OCP\IGroup'); + $recipientGroup->method('getDisplayName')->willReturn('recipientGroupDisplayName'); + $this->groupManager->method('get')->will($this->returnValueMap([ + ['recipientGroup', $recipientGroup], + ])); + $this->urlGenerator->method('linkToRouteAbsolute') ->with('files_sharing.sharecontroller.showShare', ['token' => 'myToken']) ->willReturn('myLink'); diff --git a/apps/files_sharing/tests/API/ShareesTest.php b/apps/files_sharing/tests/API/ShareesTest.php index 2aa2571fc89e..fa87af8d33a6 100644 --- a/apps/files_sharing/tests/API/ShareesTest.php +++ b/apps/files_sharing/tests/API/ShareesTest.php @@ -134,7 +134,7 @@ protected function getUserMock($uid, $displayName) { * @param string $gid * @return \OCP\IGroup|\PHPUnit_Framework_MockObject_MockObject */ - protected function getGroupMock($gid) { + protected function getGroupMock($gid, $displayName = null) { $group = $this->getMockBuilder(IGroup::class) ->disableOriginalConstructor() ->getMock(); @@ -143,6 +143,15 @@ protected function getGroupMock($gid) { ->method('getGID') ->willReturn($gid); + if (is_null($displayName)) { + // note: this is how the Group class behaves + $displayName = $gid; + } + + $group->expects($this->any()) + ->method('getDisplayName') + ->willReturn($displayName); + return $group; } @@ -468,6 +477,7 @@ public function dataGetGroups() { return [ ['test', false, true, [], [], [], [], true, false], ['test', false, false, [], [], [], [], true, false], + // group without display name [ 'test', false, true, [$this->getGroupMock('test1')], @@ -477,6 +487,36 @@ public function dataGetGroups() { true, false, ], + // group with display name, search by id + [ + 'test', false, true, + [$this->getGroupMock('test1', 'Test One')], + [], + [], + [['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + true, + false, + ], + // group with display name, search by display name + [ + 'one', false, true, + [$this->getGroupMock('test1', 'Test One')], + [], + [], + [['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + true, + false, + ], + // group with display name, search by display name, exact expected + [ + 'Test One', false, true, + [$this->getGroupMock('test1', 'Test One')], + [], + [['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]], + [], + true, + false, + ], [ 'test', false, false, [$this->getGroupMock('test1')], diff --git a/core/js/sharedialogresharerinfoview.js b/core/js/sharedialogresharerinfoview.js index 600e2ecbf56d..a3a4e97c2b67 100644 --- a/core/js/sharedialogresharerinfoview.js +++ b/core/js/sharedialogresharerinfoview.js @@ -78,7 +78,7 @@ 'core', 'Shared with you and the group {group} by {owner}', { - group: this.model.getReshareWith(), + group: this.model.getReshareWithDisplayName(), owner: ownerDisplayName } ); diff --git a/core/js/shareitemmodel.js b/core/js/shareitemmodel.js index c0a285f8822d..3e9fb07fcd8b 100644 --- a/core/js/shareitemmodel.js +++ b/core/js/shareitemmodel.js @@ -337,6 +337,14 @@ return this.get('reshare').share_with; }, + /** + * @returns {string} + */ + getReshareWithDisplayName: function() { + var reshare = this.get('reshare'); + return reshare.share_with_displayname || reshare.share_with; + }, + /** * @returns {number} */ diff --git a/core/js/tests/specs/sharedialogviewSpec.js b/core/js/tests/specs/sharedialogviewSpec.js index f0e027d9fb91..2762baacacc8 100644 --- a/core/js/tests/specs/sharedialogviewSpec.js +++ b/core/js/tests/specs/sharedialogviewSpec.js @@ -1038,16 +1038,35 @@ describe('OC.Share.ShareDialogView', function() { dialog.render(); expect(dialog.$el.find('.shareWithField').prop('disabled')).toEqual(true); }); - it('shows reshare owner', function() { + it('shows reshare owner for single user share', function() { shareModel.set({ reshare: { - uid_owner: 'user1' + uid_owner: 'user1', + displayname_owner: 'User One', + share_type: OC.Share.SHARE_TYPE_USER }, shares: [], permissions: OC.PERMISSION_READ }); dialog.render(); expect(dialog.$el.find('.resharerInfoView .reshare').length).toEqual(1); + expect(dialog.$el.find('.resharerInfoView .reshare').text().trim()).toEqual('Shared with you by User One'); + }); + it('shows reshare owner for single user share', function() { + shareModel.set({ + reshare: { + uid_owner: 'user1', + displayname_owner: 'User One', + share_with: 'group2', + share_with_displayname: 'Group Two', + share_type: OC.Share.SHARE_TYPE_GROUP + }, + shares: [], + permissions: OC.PERMISSION_READ + }); + dialog.render(); + expect(dialog.$el.find('.resharerInfoView .reshare').length).toEqual(1); + expect(dialog.$el.find('.resharerInfoView .reshare').text().trim()).toEqual('Shared with you and the group Group Two by User One'); }); it('does not show reshare owner if owner is current user', function() { shareModel.set({ diff --git a/core/js/tests/specs/shareitemmodelSpec.js b/core/js/tests/specs/shareitemmodelSpec.js index 9d9001dc9e8f..88d0ac1b9df2 100644 --- a/core/js/tests/specs/shareitemmodelSpec.js +++ b/core/js/tests/specs/shareitemmodelSpec.js @@ -190,6 +190,7 @@ describe('OC.Share.ShareItemModel', function() { uid_owner: 'owner', displayname_owner: 'Owner', share_with: 'root', + share_with_displayname: 'Wurzel', permissions: 1 }, { @@ -221,7 +222,11 @@ describe('OC.Share.ShareItemModel', function() { // user share has higher priority expect(reshare.share_type).toEqual(OC.Share.SHARE_TYPE_USER); expect(reshare.share_with).toEqual('root'); + expect(reshare.share_with_displayname).toEqual('Wurzel'); expect(reshare.id).toEqual('1'); + + expect(model.getReshareWith()).toEqual('root'); + expect(model.getReshareWithDisplayName()).toEqual('Wurzel'); }); it('does not parse link share when for a different file', function() { /* jshint camelcase: false */ From 77accad0f6142c18a8f58937a0b760ac849713df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Tue, 6 Dec 2016 13:51:57 +0100 Subject: [PATCH 3/3] Fix reporting of risky tests --- tests/lib/TestCase.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/lib/TestCase.php b/tests/lib/TestCase.php index b0c75cc82d0b..7b841305e7e5 100644 --- a/tests/lib/TestCase.php +++ b/tests/lib/TestCase.php @@ -145,7 +145,9 @@ protected function tearDown() { // fail hard if xml errors have not been cleaned up $errors = libxml_get_errors(); libxml_clear_errors(); - $this->assertEquals([], $errors); + if (!empty($errors)) { + self::assertEquals([], $errors, "There have been xml parsing errors"); + } // tearDown the traits $traits = $this->getTestTraits();