From 5d30ed9ad1bfe6f6e43cd779b3decb73010d4499 Mon Sep 17 00:00:00 2001 From: voxsim Date: Thu, 26 Jun 2014 12:38:33 +0200 Subject: [PATCH 1/4] fix in displayNamesInGroup: when specified limit N, we did complex search only in the first N users --- lib/private/group/manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/group/manager.php b/lib/private/group/manager.php index 3613c7547b..942bdbb91c 100644 --- a/lib/private/group/manager.php +++ b/lib/private/group/manager.php @@ -209,7 +209,7 @@ class Manager extends PublicEmitter { return array(); } // only user backends have the capability to do a complex search for users - $groupUsers = $group->searchUsers('', $limit, $offset); + $groupUsers = $group->searchUsers(''); $search = trim($search); if(!empty($search)) { //TODO: for OC 7 earliest: user backend should get a method to check selected users against a pattern From a49610e18a07d04dd3ed861e15b64a94b8a45e19 Mon Sep 17 00:00:00 2001 From: voxsim Date: Thu, 28 Aug 2014 13:51:48 +0200 Subject: [PATCH 2/4] change logic in displayNamesInGroup and add some unit tests --- lib/private/group/manager.php | 27 ++-- tests/lib/group/manager.php | 292 ++++++++++++++++++++++++++++++++-- 2 files changed, 296 insertions(+), 23 deletions(-) diff --git a/lib/private/group/manager.php b/lib/private/group/manager.php index 942bdbb91c..982a3ceec4 100644 --- a/lib/private/group/manager.php +++ b/lib/private/group/manager.php @@ -208,23 +208,30 @@ class Manager extends PublicEmitter { if(is_null($group)) { return array(); } - // only user backends have the capability to do a complex search for users - $groupUsers = $group->searchUsers(''); + $search = trim($search); + $groupUsers = array(); + if(!empty($search)) { - //TODO: for OC 7 earliest: user backend should get a method to check selected users against a pattern + // only user backends have the capability to do a complex search for users $filteredUsers = $this->userManager->search($search); - $testUsers = true; + foreach($filteredUsers as $filteredUser) { + if($group->inGroup($filteredUser)) { + $groupUsers []= $filteredUser; + } + } + if($limit === -1) { + $groupUsers = array_slice($groupUsers, $offset); + } else { + $groupUsers = array_slice($groupUsers, $offset, $limit); + } } else { - $filteredUsers = array(); - $testUsers = false; + $groupUsers = $group->searchUsers('', $limit, $offset); } $matchingUsers = array(); - foreach($groupUsers as $user) { - if(!$testUsers || isset($filteredUsers[$user->getUID()])) { - $matchingUsers[$user->getUID()] = $user->getDisplayName(); - } + foreach($groupUsers as $groupUser) { + $matchingUsers[$groupUser->getUID()] = $groupUser->getDisplayName(); } return $matchingUsers; } diff --git a/tests/lib/group/manager.php b/tests/lib/group/manager.php index 6799a598d4..d4a1a73783 100644 --- a/tests/lib/group/manager.php +++ b/tests/lib/group/manager.php @@ -346,14 +346,7 @@ class Manager extends \PHPUnit_Framework_TestCase { $this->assertEquals('group2', $group2->getGID()); } - public function testDisplayNamesInGroupMultipleUserBackends() { - $userBackend = $this->getMock('\OC_User_Backend'); - - $user1 = new User('user1', $userBackend); - $user2 = new User('user2', $userBackend); - $user3 = new User('user3', $userBackend); - $user4 = new User('user33', $userBackend); - + public function testDisplayNamesInGroupWithOneUserBackend() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC_Group_Backend $backend1 */ @@ -363,20 +356,30 @@ class Manager extends \PHPUnit_Framework_TestCase { ->with('testgroup') ->will($this->returnValue(true)); - $backend->expects($this->once()) - ->method('usersInGroup') - ->with('testgroup', '', -1, 0) - ->will($this->returnValue(array('user2', 'user33'))); + $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; + } + })); /** * @var \OC\User\Manager $userManager */ $userManager = $this->getMock('\OC\User\Manager'); $userBackend = $this->getMock('\OC_User_Backend'); + $userManager->expects($this->once()) ->method('search') ->with('user3') - ->will($this->returnValue(array('user3' => $user3, 'user33' => $user4))); + ->will($this->returnValue(array('user3' => new User('user3', $userBackend), + 'user33' => new User('user33', $userBackend)))); $userManager->expects($this->any()) ->method('get') @@ -396,6 +399,269 @@ class Manager extends \PHPUnit_Framework_TestCase { $users = $manager->displayNamesInGroup('testgroup', 'user3'); $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + } + + public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC_Group_Backend $backend1 + */ + $backend = $this->getMock('\OC_Group_Database'); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $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; + } + })); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->getMock('\OC\User\Manager'); + $userBackend = $this->getMock('\OC_User_Backend'); + + $userManager->expects($this->once()) + ->method('search') + ->with('user3') + ->will($this->returnValue(array('user3' => new User('user3', $userBackend), + 'user33' => new User('user33', $userBackend), + 'user333' => new User('user333', $userBackend)))); + + $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); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->displayNamesInGroup('testgroup', 'user3', 1); + $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + $this->assertFalse(isset($users['user333'])); + } + + public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC_Group_Backend $backend1 + */ + $backend = $this->getMock('\OC_Group_Database'); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $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; + } + })); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->getMock('\OC\User\Manager'); + $userBackend = $this->getMock('\OC_User_Backend'); + + $userManager->expects($this->once()) + ->method('search') + ->with('user3') + ->will($this->returnValue(array('user3' => new User('user3', $userBackend), + 'user33' => new User('user33', $userBackend), + 'user333' => new User('user333', $userBackend)))); + + $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); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->displayNamesInGroup('testgroup', 'user3', 1, 1); + $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertFalse(isset($users['user33'])); + $this->assertTrue(isset($users['user333'])); + } + + public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC_Group_Backend $backend1 + */ + $backend = $this->getMock('\OC_Group_Database'); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('usersInGroup') + ->with('testgroup', '', -1, 0) + ->will($this->returnValue(array('user2', 'user33'))); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->getMock('\OC\User\Manager'); + $userBackend = $this->getMock('\OC_User_Backend'); + + $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); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->displayNamesInGroup('testgroup', ''); + $this->assertEquals(2, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertTrue(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + } + + public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC_Group_Backend $backend1 + */ + $backend = $this->getMock('\OC_Group_Database'); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('usersInGroup') + ->with('testgroup', '', 1, 0) + ->will($this->returnValue(array('user2'))); + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->getMock('\OC\User\Manager'); + $userBackend = $this->getMock('\OC_User_Backend'); + + $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); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->displayNamesInGroup('testgroup', '', 1); + $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertTrue(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertFalse(isset($users['user33'])); + } + + public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitAndOffsetSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC_Group_Backend $backend1 + */ + $backend = $this->getMock('\OC_Group_Database'); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('usersInGroup') + ->with('testgroup', '', 1, 1) + ->will($this->returnValue(array('user33'))); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->getMock('\OC\User\Manager'); + $userBackend = $this->getMock('\OC_User_Backend'); + + $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); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->displayNamesInGroup('testgroup', '', 1, 1); + $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); $this->assertTrue(isset($users['user33'])); } From 1366133d2bba33391b25a57ad0182b317fe3ecb6 Mon Sep 17 00:00:00 2001 From: voxsim Date: Mon, 15 Sep 2014 18:37:54 +0200 Subject: [PATCH 3/4] add more logic in displayNamesInGroup for big user bases --- lib/private/group/manager.php | 21 +++++++++++++---- tests/lib/group/manager.php | 44 ++++++++++++++++++++++++++--------- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/lib/private/group/manager.php b/lib/private/group/manager.php index 982a3ceec4..e3744ab769 100644 --- a/lib/private/group/manager.php +++ b/lib/private/group/manager.php @@ -214,12 +214,23 @@ class Manager extends PublicEmitter { if(!empty($search)) { // only user backends have the capability to do a complex search for users - $filteredUsers = $this->userManager->search($search); - foreach($filteredUsers as $filteredUser) { - if($group->inGroup($filteredUser)) { - $groupUsers []= $filteredUser; - } + $searchOffset = 0; + if($limit === -1) { + $searchLimit = $group->count(''); + } else { + $searchLimit = $limit * 2; } + + do { + $filteredUsers = $this->userManager->search($search, $searchLimit, $searchOffset); + foreach($filteredUsers as $filteredUser) { + if($group->inGroup($filteredUser)) { + $groupUsers []= $filteredUser; + } + } + $searchOffset += $searchLimit; + } while(sizeof($groupUsers) < $searchLimit+$offset && sizeof($filteredUsers) > 0 && sizeof($filteredUsers) === $searchLimit); + if($limit === -1) { $groupUsers = array_slice($groupUsers, $offset); } else { diff --git a/tests/lib/group/manager.php b/tests/lib/group/manager.php index d4a1a73783..70d9783946 100644 --- a/tests/lib/group/manager.php +++ b/tests/lib/group/manager.php @@ -369,17 +369,31 @@ class Manager extends \PHPUnit_Framework_TestCase { } })); + $backend->expects($this->once()) + ->method('implementsActions') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('countUsersInGroup') + ->with('testgroup', '') + ->will($this->returnValue(2)); + /** * @var \OC\User\Manager $userManager */ $userManager = $this->getMock('\OC\User\Manager'); $userBackend = $this->getMock('\OC_User_Backend'); - $userManager->expects($this->once()) + $userManager->expects($this->any()) ->method('search') ->with('user3') - ->will($this->returnValue(array('user3' => new User('user3', $userBackend), - 'user33' => new User('user33', $userBackend)))); + ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { + switch($offset) { + case 0 : return array('user3' => new User('user3', $userBackend), + 'user33' => new User('user33', $userBackend)); + case 2 : return array(); + } + })); $userManager->expects($this->any()) ->method('get') @@ -435,12 +449,16 @@ class Manager extends \PHPUnit_Framework_TestCase { $userManager = $this->getMock('\OC\User\Manager'); $userBackend = $this->getMock('\OC_User_Backend'); - $userManager->expects($this->once()) + $userManager->expects($this->any()) ->method('search') ->with('user3') - ->will($this->returnValue(array('user3' => new User('user3', $userBackend), - 'user33' => new User('user33', $userBackend), - 'user333' => new User('user333', $userBackend)))); + ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { + switch($offset) { + case 0 : return array('user3' => new User('user3', $userBackend), + 'user33' => new User('user33', $userBackend)); + case 2 : return array('user333' => new User('user333', $userBackend)); + } + })); $userManager->expects($this->any()) ->method('get') @@ -498,12 +516,16 @@ class Manager extends \PHPUnit_Framework_TestCase { $userManager = $this->getMock('\OC\User\Manager'); $userBackend = $this->getMock('\OC_User_Backend'); - $userManager->expects($this->once()) + $userManager->expects($this->any()) ->method('search') ->with('user3') - ->will($this->returnValue(array('user3' => new User('user3', $userBackend), - 'user33' => new User('user33', $userBackend), - 'user333' => new User('user333', $userBackend)))); + ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { + switch($offset) { + case 0 : return array('user3' => new User('user3', $userBackend), + 'user33' => new User('user33', $userBackend)); + case 2 : return array('user333' => new User('user333', $userBackend)); + } + })); $userManager->expects($this->any()) ->method('get') From 7a14f94ae5cc8d02d46294ca5079c929877c2fe4 Mon Sep 17 00:00:00 2001 From: voxsim Date: Thu, 18 Sep 2014 17:50:19 +0200 Subject: [PATCH 4/4] 1. remove sizeof($filteredUsers) > 0 as condition 2. use count instead of sizeof. Latter is an alias to first one, practically we stick to count everywhere. Having it consistent helps with readability. 3. move whitespace so we have $groupUsers[] = $filteredUser; instead of $groupUsers []= $filteredUser; --- lib/private/group/manager.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/group/manager.php b/lib/private/group/manager.php index e3744ab769..da598f0c33 100644 --- a/lib/private/group/manager.php +++ b/lib/private/group/manager.php @@ -225,11 +225,11 @@ class Manager extends PublicEmitter { $filteredUsers = $this->userManager->search($search, $searchLimit, $searchOffset); foreach($filteredUsers as $filteredUser) { if($group->inGroup($filteredUser)) { - $groupUsers []= $filteredUser; + $groupUsers[]= $filteredUser; } } $searchOffset += $searchLimit; - } while(sizeof($groupUsers) < $searchLimit+$offset && sizeof($filteredUsers) > 0 && sizeof($filteredUsers) === $searchLimit); + } while(count($groupUsers) < $searchLimit+$offset && count($filteredUsers) === $searchLimit); if($limit === -1) { $groupUsers = array_slice($groupUsers, $offset);