From 453f3beffaf629efa5c715f7ca88b5c0a1034af8 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 30 Nov 2016 20:21:44 +0100 Subject: [PATCH 1/6] Adding group display name support --- lib/private/Group/Backend.php | 15 +- 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 | 435 ++++++++++++++++---------------- 6 files changed, 269 insertions(+), 229 deletions(-) diff --git a/lib/private/Group/Backend.php b/lib/private/Group/Backend.php index 14c36d9342..1e8d62f5e4 100644 --- a/lib/private/Group/Backend.php +++ b/lib/private/Group/Backend.php @@ -31,23 +31,14 @@ 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 = array( + 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', + ]; /** * Get all supported actions diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index 9379d604c4..bb85445f54 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -66,18 +66,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 3191113898..944598a829 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -155,19 +155,29 @@ class Manager extends PublicEmitter implements IGroupManager { /** * @param string $gid + * @param string $displayName * @return \OCP\IGroup */ - protected function getGroupObject($gid) { + protected function getGroupObject($gid, $displayName = null) { $backends = array(); 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 6f456f51fd..97837e50b1 100644 --- a/lib/public/GroupInterface.php +++ b/lib/public/GroupInterface.php @@ -40,6 +40,18 @@ namespace OCP; */ 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 d5fcbcc5cd..0cc62e9a8e 100644 --- a/lib/public/IGroup.php +++ b/lib/public/IGroup.php @@ -4,6 +4,7 @@ * * @author Morris Jobke * @author Robin Appelman + * @author Vincent Petry * * @license AGPL-3.0 * @@ -36,6 +37,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 ece99eb569..93fe67ad9b 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -1,52 +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\Manager; -use OC\User\User; +use OCP\IUser; +use OCP\GroupInterface; class ManagerTest extends \Test\TestCase { - /** @var Manager */ - private $userManager; - - public function setUp() { - parent::setUp(); - - $this->userManager = $this->getMockBuilder('\OC\User\Manager') - ->disableOriginalConstructor() - ->getMock(); + 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; } - /** - * @param string $uid - * @param \OC\User\Backend $backend - * @return User - */ - private function newUser($uid, $backend) { - $config = $this->getMockBuilder('\OCP\IConfig') + 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(); - $urlgenerator = $this->getMockBuilder('\OCP\IURLGenerator') - ->disableOriginalConstructor() - ->getMock(); - - return new User($uid, $backend, null, $config, $urlgenerator); + $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->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('groupExists') ->with('group1') @@ -70,9 +99,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('groupExists') ->with('group1') @@ -100,9 +127,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 */ - $backend1 = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend1 = $this->getTestBackend(); $backend1->expects($this->any()) ->method('groupExists') ->with('group1') @@ -111,9 +136,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend2 */ - $backend2 = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend2 = $this->getTestBackend(); $backend2->expects($this->any()) ->method('groupExists') ->with('group1') @@ -133,18 +156,13 @@ class ManagerTest extends \Test\TestCase { * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ $backendGroupCreated = false; - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $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) { @@ -162,9 +180,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('groupExists') ->with('group1') @@ -183,9 +199,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getGroups') ->with('1') @@ -208,9 +222,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 */ - $backend1 = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend1 = $this->getTestBackend(); $backend1->expects($this->once()) ->method('getGroups') ->with('1') @@ -222,9 +234,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend2 */ - $backend2 = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend2 = $this->getTestBackend(); $backend2->expects($this->once()) ->method('getGroups') ->with('1') @@ -249,9 +259,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 */ - $backend1 = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend1 = $this->getTestBackend(); $backend1->expects($this->once()) ->method('getGroups') ->with('1', 2, 1) @@ -263,9 +271,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend2 */ - $backend2 = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend2 = $this->getTestBackend(); $backend2->expects($this->once()) ->method('getGroups') ->with('1', 2, 1) @@ -290,9 +296,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -308,7 +312,7 @@ class ManagerTest extends \Test\TestCase { $manager = new \OC\Group\Manager($this->userManager); $manager->addBackend($backend); - $groups = $manager->getUserGroups($this->newUser('user1', $userBackend)); + $groups = $manager->getUserGroups($this->getTestUser('user1')); $this->assertEquals(1, count($groups)); $group1 = reset($groups); $this->assertEquals('group1', $group1->getGID()); @@ -344,9 +348,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -365,9 +367,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -386,9 +386,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -407,9 +405,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 */ - $backend1 = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend1 = $this->getTestBackend(); $backend1->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -421,9 +417,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend2 */ - $backend2 = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend2 = $this->getTestBackend(); $backend2->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -439,7 +433,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend1); $manager->addBackend($backend2); - $groups = $manager->getUserGroups($this->newUser('user1', $userBackend)); + $groups = $manager->getUserGroups($this->getTestUser('user1')); $this->assertEquals(2, count($groups)); $group1 = reset($groups); $group2 = next($groups); @@ -447,30 +441,28 @@ class ManagerTest extends \Test\TestCase { $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->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $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; + } + })); $userBackend = $this->getMockBuilder('\OC\User\Backend') ->disableOriginalConstructor() @@ -480,21 +472,21 @@ class ManagerTest extends \Test\TestCase { ->method('searchDisplayName') ->with('user3') ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { - switch($offset) { - case 0 : return array('user3' => $this->newUser('user3', $userBackend), - 'user33' => $this->newUser('user33', $userBackend)); - case 2 : return array(); - } - })); + switch($offset) { + case 0 : return ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')]; + case 2 : return []; + } + })); $this->userManager->expects($this->any()) ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return $this->newUser('user1', $userBackend); - case 'user2' : return $this->newUser('user2', $userBackend); - case 'user3' : return $this->newUser('user3', $userBackend); - case 'user33': return $this->newUser('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; } @@ -511,31 +503,29 @@ class ManagerTest extends \Test\TestCase { $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->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $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; + } + })); $userBackend = $this->getMockBuilder('\OC\User\Backend') ->disableOriginalConstructor() @@ -545,22 +535,22 @@ class ManagerTest extends \Test\TestCase { ->method('searchDisplayName') ->with('user3') ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { - switch($offset) { - case 0 : return array('user3' => $this->newUser('user3', $userBackend), - 'user33' => $this->newUser('user33', $userBackend)); - case 2 : return array('user333' => $this->newUser('user333', $userBackend)); - } - })); + switch($offset) { + case 0 : return ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')]; + case 2 : return ['user333' => $this->getTestUser('user333')]; + } + })); $this->userManager->expects($this->any()) ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return $this->newUser('user1', $userBackend); - case 'user2' : return $this->newUser('user2', $userBackend); - case 'user3' : return $this->newUser('user3', $userBackend); - case 'user33': return $this->newUser('user33', $userBackend); - case 'user333': return $this->newUser('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; } @@ -580,29 +570,27 @@ class ManagerTest extends \Test\TestCase { public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpecified() { /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $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; + } + })); $userBackend = $this->getMockBuilder('\OC\User\Backend') ->disableOriginalConstructor() @@ -612,25 +600,25 @@ class ManagerTest extends \Test\TestCase { ->method('searchDisplayName') ->with('user3') ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { - switch($offset) { - case 0 : - return array( - 'user3' => $this->newUser('user3', $userBackend), - 'user33' => $this->newUser('user33', $userBackend), - 'user333' => $this->newUser('user333', $userBackend) - ); - } - })); + switch($offset) { + case 0 : + return [ + 'user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33'), + 'user333' => $this->getTestUser('user333') + ]; + } + })); $this->userManager->expects($this->any()) ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return $this->newUser('user1', $userBackend); - case 'user2' : return $this->newUser('user2', $userBackend); - case 'user3' : return $this->newUser('user3', $userBackend); - case 'user33': return $this->newUser('user33', $userBackend); - case 'user333': return $this->newUser('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; } @@ -650,17 +638,15 @@ class ManagerTest extends \Test\TestCase { public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $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(array('user2', 'user33'))); @@ -673,10 +659,10 @@ class ManagerTest extends \Test\TestCase { ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return $this->newUser('user1', $userBackend); - case 'user2' : return $this->newUser('user2', $userBackend); - case 'user3' : return $this->newUser('user3', $userBackend); - case 'user33': return $this->newUser('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; } @@ -695,11 +681,9 @@ class ManagerTest extends \Test\TestCase { public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitSpecified() { /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend1 + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') ->with('testgroup') @@ -718,10 +702,10 @@ class ManagerTest extends \Test\TestCase { ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return $this->newUser('user1', $userBackend); - case 'user2' : return $this->newUser('user2', $userBackend); - case 'user3' : return $this->newUser('user3', $userBackend); - case 'user33': return $this->newUser('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; } @@ -738,13 +722,11 @@ class ManagerTest extends \Test\TestCase { $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->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) ->method('groupExists') ->with('testgroup') @@ -763,10 +745,10 @@ class ManagerTest extends \Test\TestCase { ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { switch($uid) { - case 'user1' : return $this->newUser('user1', $userBackend); - case 'user2' : return $this->newUser('user2', $userBackend); - case 'user3' : return $this->newUser('user3', $userBackend); - case 'user33': return $this->newUser('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; } @@ -787,10 +769,8 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); - $expectedGroups = array(); + $backend = $this->getTestBackend(); + $expectedGroups = []; $backend->expects($this->any()) ->method('getUserGroups') ->with('user1') @@ -801,15 +781,12 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->with('group1') ->will($this->returnValue(true)); - $backend->expects($this->once()) - ->method('implementsActions') - ->will($this->returnValue(true)); $manager = new \OC\Group\Manager($this->userManager); $manager->addBackend($backend); // prime cache - $user1 = $this->newUser('user1', null); + $user1 = $this->getTestUser('user1'); $groups = $manager->getUserGroups($user1); $this->assertEquals(array(), $groups); @@ -829,10 +806,8 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); - $expectedGroups = array('group1'); + $backend = $this->getTestBackend(); + $expectedGroups = ['group1']; $backend->expects($this->any()) ->method('getUserGroups') ->with('user1') @@ -843,9 +818,6 @@ class ManagerTest extends \Test\TestCase { ->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 +829,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend); // prime cache - $user1 = $this->newUser('user1', null); + $user1 = $this->getTestUser('user1'); $groups = $manager->getUserGroups($user1); $this->assertEquals(1, count($groups)); $group1 = reset($groups); @@ -877,9 +849,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); + $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('getUserGroups') ->with('user1') @@ -892,4 +862,43 @@ class ManagerTest extends \Test\TestCase { $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 14256d631cf32d8774c7fcffe322d65f964fcd5d Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 30 Nov 2016 20:56:10 +0100 Subject: [PATCH 2/6] Use group display name in sharing API + UI --- .../lib/Controller/ShareAPIController.php | 3 +- .../lib/Controller/ShareesAPIController.php | 30 +++++++---- .../Controller/ShareAPIControllerTest.php | 53 +++++++++++++++++-- .../Controller/ShareesAPIControllerTest.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/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 90274beba4..ccd0e3aa31 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -168,8 +168,9 @@ class ShareAPIController extends OCSController { $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/ShareesAPIController.php b/apps/files_sharing/lib/Controller/ShareesAPIController.php index 6b3208dc28..cd5139693c 100644 --- a/apps/files_sharing/lib/Controller/ShareesAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareesAPIController.php @@ -165,9 +165,10 @@ class ShareesAPIController extends OCSController { } $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'][] = [ @@ -225,7 +226,7 @@ class ShareesAPIController extends OCSController { $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'; @@ -236,13 +237,19 @@ class ShareesAPIController extends OCSController { // 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, @@ -250,7 +257,7 @@ class ShareesAPIController extends OCSController { ]; } else { $this->result['groups'][] = [ - 'label' => $gid, + 'label' => $group->getDisplayName(), 'value' => [ 'shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => $gid, @@ -265,7 +272,7 @@ class ShareesAPIController extends OCSController { $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(), @@ -299,10 +306,11 @@ class ShareesAPIController extends OCSController { 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) { $result['exactIdMatch'] = true; } $result['exact'][] = [ diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index ed4aa1dba9..6f6529a2be 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -1882,9 +1882,10 @@ class ShareAPIControllerTest extends \Test\TestCase { ], $share, [], false ]; + // with existing group $share = \OC::$server->getShareManager()->newShare(); $share->setShareType(\OCP\Share::SHARE_TYPE_GROUP) - ->setSharedWith('recipient') + ->setSharedWith('recipientGroup') ->setSharedBy('initiator') ->setShareOwner('owner') ->setPermissions(\OCP\Constants::PERMISSION_READ) @@ -1914,8 +1915,47 @@ class ShareAPIControllerTest extends \Test\TestCase { '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 @@ -2029,6 +2069,13 @@ class ShareAPIControllerTest extends \Test\TestCase { */ 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/Controller/ShareesAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php index c570cb1698..c68e230474 100644 --- a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php @@ -133,7 +133,7 @@ class ShareesAPIControllerTest extends TestCase { * @param string $gid * @return \OCP\IGroup|\PHPUnit_Framework_MockObject_MockObject */ - protected function getGroupMock($gid) { + protected function getGroupMock($gid, $displayName = null) { $group = $this->getMockBuilder('OCP\IGroup') ->disableOriginalConstructor() ->getMock(); @@ -142,6 +142,15 @@ class ShareesAPIControllerTest extends TestCase { ->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; } @@ -467,6 +476,7 @@ class ShareesAPIControllerTest extends TestCase { return [ ['test', false, true, [], [], [], [], true, false], ['test', false, false, [], [], [], [], true, false], + // group without display name [ 'test', false, true, [$this->getGroupMock('test1')], @@ -476,6 +486,36 @@ class ShareesAPIControllerTest extends TestCase { 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 654eebf499..9a9d95cfb6 100644 --- a/core/js/sharedialogresharerinfoview.js +++ b/core/js/sharedialogresharerinfoview.js @@ -80,7 +80,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 b01f0f790a..9b10f067af 100644 --- a/core/js/shareitemmodel.js +++ b/core/js/shareitemmodel.js @@ -344,6 +344,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 985610d51f..cbb74714ff 100644 --- a/core/js/tests/specs/sharedialogviewSpec.js +++ b/core/js/tests/specs/sharedialogviewSpec.js @@ -975,16 +975,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 1cddcb2aca..3d3baf75d1 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 b62b82c2ded5a27effb7ef2f40f266f12db8e590 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/6] Fix reporting of risky tests Signed-off-by: Arthur Schiwon --- 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 f115c11938..afed281791 100644 --- a/tests/lib/TestCase.php +++ b/tests/lib/TestCase.php @@ -184,7 +184,9 @@ abstract class TestCase extends TestCasePhpUnitCompatibility { // 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"); + } \OC\Files\Cache\Storage::getGlobalCache()->clearCache(); From 11faa6da21149870f9002af323a1be7506b89527 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 22 Dec 2016 18:53:29 +0100 Subject: [PATCH 4/6] declare field Signed-off-by: Arthur Schiwon --- lib/private/Group/Group.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index bb85445f54..69dce21569 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -31,6 +31,9 @@ namespace OC\Group; use OCP\IGroup; class Group implements IGroup { + /** @var null|string */ + protected $displayName; + /** * @var string $id */ From b4d779977a9b9a15b7268ad68a897735f9607dc5 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 22 Dec 2016 19:33:09 +0100 Subject: [PATCH 5/6] fix tests Signed-off-by: Arthur Schiwon --- apps/files_sharing/tests/Controller/ShareAPIControllerTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 6f6529a2be..97774081b6 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -23,10 +23,8 @@ */ namespace OCA\Files_Sharing\Tests\Controller; -use OC\ContactsManager; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSNotFoundException; -use OCP\Contacts; use OCP\Files\Folder; use OCP\IL10N; use OCA\Files_Sharing\Controller\ShareAPIController; @@ -39,6 +37,7 @@ use OCP\IUser; use OCP\Files\IRootFolder; use OCP\Lock\LockedException; use OCP\Share\IManager; +use OCP\Share; /** * Class ShareAPIControllerTest From 91a1e5fd9d939c69ccacacd44001d52453957093 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 22 Dec 2016 20:53:52 +0100 Subject: [PATCH 6/6] fix more tests Signed-off-by: Arthur Schiwon --- tests/lib/Group/ManagerTest.php | 46 ++++++++++++++++----------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 93fe67ad9b..1a7ced5f1b 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -22,10 +22,20 @@ */ namespace Test\Group; +use OC\User\Manager; use OCP\IUser; use OCP\GroupInterface; class ManagerTest extends \Test\TestCase { + /** @var Manager|\PHPUnit_Framework_MockObject_MockObject $userManager */ + protected $userManager; + + protected function setUp() { + parent::setUp(); + + $this->userManager = $this->createMock(Manager::class); + } + private function getTestUser($userId) { $mockUser = $this->createMock(IUser::class); $mockUser->expects($this->any()) @@ -306,9 +316,6 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $userBackend = $this->getMockBuilder('\OC\Group\Database') - ->disableOriginalConstructor() - ->getMock(); $manager = new \OC\Group\Manager($this->userManager); $manager->addBackend($backend); @@ -426,9 +433,6 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $userBackend = $this->getMockBuilder('\OC\User\Backend') - ->disableOriginalConstructor() - ->getMock(); $manager = new \OC\Group\Manager($this->userManager); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -477,8 +481,8 @@ class ManagerTest extends \Test\TestCase { 'user33' => $this->getTestUser('user33')]; case 2 : return []; } + return null; })); - $this->userManager->expects($this->any()) ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { @@ -540,8 +544,8 @@ class ManagerTest extends \Test\TestCase { 'user33' => $this->getTestUser('user33')]; case 2 : return ['user333' => $this->getTestUser('user333')]; } + return null; })); - $this->userManager->expects($this->any()) ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { @@ -600,16 +604,16 @@ class ManagerTest extends \Test\TestCase { ->method('searchDisplayName') ->with('user3') ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { - switch($offset) { - case 0 : - return [ - 'user3' => $this->getTestUser('user3'), - 'user33' => $this->getTestUser('user33'), - 'user333' => $this->getTestUser('user333') - ]; - } + switch($offset) { + case 0 : + return [ + 'user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33'), + 'user333' => $this->getTestUser('user333') + ]; + } + return null; })); - $this->userManager->expects($this->any()) ->method('get') ->will($this->returnCallback(function($uid) use ($userBackend) { @@ -793,7 +797,7 @@ class ManagerTest extends \Test\TestCase { // add user $group = $manager->get('group1'); $group->addUser($user1); - $expectedGroups = array('group1'); + $expectedGroups = ['group1']; // check result $groups = $manager->getUserGroups($user1); @@ -881,11 +885,7 @@ class ManagerTest extends \Test\TestCase { ['group2', ['gid' => 'group2']], ])); - /** - * @var \OC\User\Manager $userManager - */ - $userManager = $this->createMock('\OC\User\Manager'); - $manager = new \OC\Group\Manager($userManager); + $manager = new \OC\Group\Manager($this->userManager); $manager->addBackend($backend); // group with display name