From 1cf1bc9c1ddc9a6625a1d373898e3ac2a6220ec2 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 21 Jun 2019 15:38:19 +0200 Subject: [PATCH] dont show remote and email options if we have an exact match for local user email Signed-off-by: Robin Appelman --- .../Collaboration/Collaborators/Search.php | 7 ++ .../Collaborators/UserPlugin.php | 27 ++++-- .../Collaborators/UserPluginTest.php | 96 +++++++++++-------- 3 files changed, 83 insertions(+), 47 deletions(-) diff --git a/lib/private/Collaboration/Collaborators/Search.php b/lib/private/Collaboration/Collaborators/Search.php index 7f5c5a1a81140..9b9decfecbed8 100644 --- a/lib/private/Collaboration/Collaborators/Search.php +++ b/lib/private/Collaboration/Collaborators/Search.php @@ -85,6 +85,13 @@ public function search($search, array $shareTypes, $lookup, $limit, $offset) { $searchResult->unsetResult($emailType); } + // if we have an exact local user match, there is no need to show the remote and email matches + $userType = new SearchResultType('users'); + if($searchResult->hasExactIdMatch($userType)) { + $searchResult->unsetResult($remoteType); + $searchResult->unsetResult($emailType); + } + return [$searchResult->asArray(), (bool)$hasMoreResults]; } diff --git a/lib/private/Collaboration/Collaborators/UserPlugin.php b/lib/private/Collaboration/Collaborators/UserPlugin.php index c40aaff4229fa..a54b9b4181178 100644 --- a/lib/private/Collaboration/Collaborators/UserPlugin.php +++ b/lib/private/Collaboration/Collaborators/UserPlugin.php @@ -31,6 +31,7 @@ use OCP\Collaboration\Collaborators\ISearchResult; use OCP\Collaboration\Collaborators\SearchResultType; use OCP\IConfig; +use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; use OCP\IUserManager; @@ -70,11 +71,11 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) { $userGroups = []; if ($this->shareWithGroupOnly) { // Search in all the groups this user is part of - $userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); + $userGroups = $this->groupManager->getUserGroups($this->userSession->getUser()); foreach ($userGroups as $userGroup) { - $usersTmp = $this->groupManager->displayNamesInGroup($userGroup, $search, $limit, $offset); - foreach ($usersTmp as $uid => $userDisplayName) { - $users[(string) $uid] = $userDisplayName; + $usersInGroup = $userGroup->searchDisplayName($search, $limit, $offset); + foreach ($usersInGroup as $user) { + $users[$user->getUID()] = $user; } } } else { @@ -96,9 +97,15 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) { $foundUserById = false; $lowerSearch = strtolower($search); - foreach ($users as $uid => $userDisplayName) { + foreach ($users as $uid => $user) { + $userDisplayName = $user->getDisplayName(); + $userEmail = $user->getEMailAddress(); $uid = (string) $uid; - if (strtolower($uid) === $lowerSearch || strtolower($userDisplayName) === $lowerSearch) { + if ( + strtolower($uid) === $lowerSearch || + strtolower($userDisplayName) === $lowerSearch || + strtolower($userEmail) === $lowerSearch + ) { if (strtolower($uid) === $lowerSearch) { $foundUserById = true; } @@ -129,7 +136,10 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) { if ($this->shareWithGroupOnly) { // Only add, if we have a common group - $commonGroups = array_intersect($userGroups, $this->groupManager->getUserGroupIds($user)); + $userGroupIds = array_map(function(IGroup $group) { + return $group->getGID(); + }, $userGroups); + $commonGroups = array_intersect($userGroupIds, $this->groupManager->getUserGroupIds($user)); $addUser = !empty($commonGroups); } @@ -151,6 +161,9 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) { $type = new SearchResultType('users'); $searchResult->addResultSet($type, $result['wide'], $result['exact']); + if (count($result['exact'])) { + $searchResult->markExactIdMatch($type); + } return $hasMoreResults; } diff --git a/tests/lib/Collaboration/Collaborators/UserPluginTest.php b/tests/lib/Collaboration/Collaborators/UserPluginTest.php index 3aeeaa3eecb12..94ed068469913 100644 --- a/tests/lib/Collaboration/Collaborators/UserPluginTest.php +++ b/tests/lib/Collaboration/Collaborators/UserPluginTest.php @@ -27,6 +27,7 @@ use OC\Collaboration\Collaborators\UserPlugin; use OCP\Collaboration\Collaborators\ISearchResult; use OCP\IConfig; +use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; use OCP\IUserManager; @@ -107,6 +108,16 @@ public function getUserMock($uid, $displayName, $enabled = true) { return $user; } + public function getGroupMock($gid) { + $group = $this->createMock(IGroup::class); + + $group->expects($this->any()) + ->method('getGID') + ->willReturn($gid); + + return $group; + } + public function dataGetUsers() { return [ ['test', false, true, [], [], [], [], true, false], @@ -117,33 +128,33 @@ public function dataGetUsers() { 'test', false, true, [], [], [ ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], - ], [], true, $this->getUserMock('test', 'Test') + ], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', false, false, [], [], [ ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], - ], [], true, $this->getUserMock('test', 'Test') + ], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', true, true, [], [], - [], [], true, $this->getUserMock('test', 'Test') + [], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', true, false, [], [], - [], [], true, $this->getUserMock('test', 'Test') + [], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', true, true, ['test-group'], [['test-group', 'test', 2, 0, []]], [ ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], - ], [], true, $this->getUserMock('test', 'Test') + ], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', true, false, ['test-group'], [['test-group', 'test', 2, 0, []]], [ ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], - ], [], true, $this->getUserMock('test', 'Test') + ], [], true, $this->getUserMock('test', 'Test'), ], [ 'test', @@ -247,7 +258,7 @@ public function dataGetUsers() { true, ['abc', 'xyz'], [ - ['abc', 'test', 2, 0, ['test1' => 'Test One']], + ['abc', 'test', 2, 0, [$this->getUserMock('test1', 'Test One')]], ['xyz', 'test', 2, 0, []], ], [], @@ -263,7 +274,7 @@ public function dataGetUsers() { false, ['abc', 'xyz'], [ - ['abc', 'test', 2, 0, ['test1' => 'Test One']], + ['abc', 'test', 2, 0, [$this->getUserMock('test1', 'Test One')]], ['xyz', 'test', 2, 0, []], ], [], @@ -278,12 +289,12 @@ public function dataGetUsers() { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), ]], ['xyz', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), ]], ], [], @@ -301,12 +312,12 @@ public function dataGetUsers() { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), ]], ['xyz', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + $this->getUserMock('test1', 'Test One'), + $this->getUserMock('test2', 'Test Two'), ]], ], [], @@ -321,10 +332,10 @@ public function dataGetUsers() { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test' => 'Test One', + $this->getUserMock('test', 'Test One'), ]], ['xyz', 'test', 2, 0, [ - 'test2' => 'Test Two', + $this->getUserMock('test2', 'Test Two'), ]], ], [ @@ -343,10 +354,10 @@ public function dataGetUsers() { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test' => 'Test One', + $this->getUserMock('test', 'Test One'), ]], ['xyz', 'test', 2, 0, [ - 'test2' => 'Test Two', + $this->getUserMock('test2', 'Test Two'), ]], ], [ @@ -404,31 +415,36 @@ function($appName, $key, $default) ->method('getUser') ->willReturn($this->user); - if(!$shareWithGroupOnly) { + if (!$shareWithGroupOnly) { $this->userManager->expects($this->once()) ->method('searchDisplayName') ->with($searchTerm, $this->limit, $this->offset) ->willReturn($userResponse); } else { + $groups = array_combine($groupResponse, array_map(function ($gid) { + return $this->getGroupMock($gid); + }, $groupResponse)); if ($singleUser !== false) { - $this->groupManager->expects($this->exactly(2)) - ->method('getUserGroupIds') - ->withConsecutive( - [$this->user], - [$singleUser] - ) + $this->groupManager->method('getUserGroups') + ->with($this->user) + ->willReturn($groups); + + $this->groupManager->method('getUserGroupIds') + ->with($singleUser) ->willReturn($groupResponse); } else { $this->groupManager->expects($this->once()) - ->method('getUserGroupIds') + ->method('getUserGroups') ->with($this->user) - ->willReturn($groupResponse); + ->willReturn($groups); } - $this->groupManager->expects($this->exactly(sizeof($groupResponse))) - ->method('displayNamesInGroup') - ->with($this->anything(), $searchTerm, $this->limit, $this->offset) - ->willReturnMap($userResponse); + foreach ($userResponse as $groupDefinition) { + [$gid, $search, $limit, $offset, $users] = $groupDefinition; + $groups[$gid]->method('searchDisplayName') + ->with($search, $limit, $offset) + ->willReturn($users); + } } if ($singleUser !== false) { @@ -451,24 +467,24 @@ public function takeOutCurrentUserProvider() { $inputUsers = [ 'alice' => 'Alice', 'bob' => 'Bob', - 'carol' => 'Carol' + 'carol' => 'Carol', ]; return [ [ $inputUsers, ['alice', 'carol'], - 'bob' + 'bob', ], [ $inputUsers, ['alice', 'bob', 'carol'], - 'dave' + 'dave', ], [ $inputUsers, ['alice', 'bob', 'carol'], - null - ] + null, + ], ]; } @@ -483,8 +499,8 @@ public function testTakeOutCurrentUser(array $users, array $expectedUIDs, $curre $this->session->expects($this->once()) ->method('getUser') - ->willReturnCallback(function() use ($currentUserId) { - if($currentUserId !== null) { + ->willReturnCallback(function () use ($currentUserId) { + if ($currentUserId !== null) { return $this->getUserMock($currentUserId, $currentUserId); } return null;