Merge pull request #16035 from nextcloud/share-search-hide-on-match

dont show remote and email options if we have an exact match for local user email

lib/private/Collaboration/Collaborators/Search.php

@@ -85,6 +85,13 @@ class Search implements ISearch {
 			$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];
 	}
 

lib/private/Collaboration/Collaborators/UserPlugin.php

@@ -32,6 +32,7 @@ use OCP\Collaboration\Collaborators\ISearchPlugin;
 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;
@@ -75,11 +76,11 @@ class UserPlugin implements ISearchPlugin {
 		$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);
+				$usersInGroup = $userGroup->searchDisplayName($search, $limit, $offset);
-				foreach ($usersTmp as $uid => $userDisplayName) {
+				foreach ($usersInGroup as $user) {
-					$users[(string) $uid] = $userDisplayName;
+					$users[$user->getUID()] = $user;
 				}
 			}
 		} else {
@@ -88,7 +89,7 @@ class UserPlugin implements ISearchPlugin {
 			$currentUserGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser());
 			foreach ($usersTmp as $user) {
 				if ($user->isEnabled()) { // Don't keep deactivated users
-					$users[(string) $user->getUID()] = $user->getDisplayName();
+					$users[$user->getUID()] = $user;
 
 					$addToWideResults = false;
 					if ($this->shareeEnumeration && !$this->shareeEnumerationInGroupOnly) {
@@ -123,9 +124,15 @@ class UserPlugin implements ISearchPlugin {
 
 		$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;
 				}
@@ -156,7 +163,10 @@ class UserPlugin implements ISearchPlugin {
 
 				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);
 				}
 
@@ -179,6 +189,9 @@ class UserPlugin implements ISearchPlugin {
 
 		$type = new SearchResultType('users');
 		$searchResult->addResultSet($type, $result['wide'], $result['exact']);
+		if (count($result['exact'])) {
+			$searchResult->markExactIdMatch($type);
+		}
 
 		return $hasMoreResults;
 	}

tests/lib/Collaboration/Collaborators/UserPluginTest.php

@@ -27,6 +27,7 @@ use OC\Collaboration\Collaborators\SearchResult;
 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;
@@ -93,9 +94,8 @@ class UserPluginTest extends TestCase {
 		$this->config->expects($this->any())
 			->method('getAppValue')
 			->willReturnCallback(
-				function($appName, $key, $default)
+				function ($appName, $key, $default)
-				use ($shareWithGroupOnly, $shareeEnumeration, $shareeEnumerationLimitToGroup)
+				use ($shareWithGroupOnly, $shareeEnumeration, $shareeEnumerationLimitToGroup) {
-				{
 					if ($appName === 'core' && $key === 'shareapi_only_share_with_group_members') {
 						return $shareWithGroupOnly ? 'yes' : 'no';
 					} else if ($appName === 'core' && $key === 'shareapi_allow_share_dialog_user_enumeration') {
@@ -127,6 +127,16 @@ class UserPluginTest extends TestCase {
 		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],
@@ -137,33 +147,33 @@ class UserPluginTest extends TestCase {
 				'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',
@@ -267,7 +277,7 @@ class UserPluginTest extends TestCase {
 				true,
 				['abc', 'xyz'],
 				[
-					['abc', 'test', 2, 0, ['test1' => 'Test One']],
+					['abc', 'test', 2, 0, [$this->getUserMock('test1', 'Test One')]],
 					['xyz', 'test', 2, 0, []],
 				],
 				[],
@@ -283,7 +293,7 @@ class UserPluginTest extends TestCase {
 				false,
 				['abc', 'xyz'],
 				[
-					['abc', 'test', 2, 0, ['test1' => 'Test One']],
+					['abc', 'test', 2, 0, [$this->getUserMock('test1', 'Test One')]],
 					['xyz', 'test', 2, 0, []],
 				],
 				[],
@@ -298,12 +308,12 @@ class UserPluginTest extends TestCase {
 				['abc', 'xyz'],
 				[
 					['abc', 'test', 2, 0, [
-						'test1' => 'Test One',
+						$this->getUserMock('test1', 'Test One'),
-						'test2' => 'Test Two',
+						$this->getUserMock('test2', 'Test Two'),
 					]],
 					['xyz', 'test', 2, 0, [
-						'test1' => 'Test One',
+						$this->getUserMock('test1', 'Test One'),
-						'test2' => 'Test Two',
+						$this->getUserMock('test2', 'Test Two'),
 					]],
 				],
 				[],
@@ -321,12 +331,12 @@ class UserPluginTest extends TestCase {
 				['abc', 'xyz'],
 				[
 					['abc', 'test', 2, 0, [
-						'test1' => 'Test One',
+						$this->getUserMock('test1', 'Test One'),
-						'test2' => 'Test Two',
+						$this->getUserMock('test2', 'Test Two'),
 					]],
 					['xyz', 'test', 2, 0, [
-						'test1' => 'Test One',
+						$this->getUserMock('test1', 'Test One'),
-						'test2' => 'Test Two',
+						$this->getUserMock('test2', 'Test Two'),
 					]],
 				],
 				[],
@@ -341,10 +351,10 @@ class UserPluginTest extends TestCase {
 				['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'),
 					]],
 				],
 				[
@@ -363,10 +373,10 @@ class UserPluginTest extends TestCase {
 				['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'),
 					]],
 				],
 				[
@@ -410,31 +420,36 @@ class UserPluginTest extends TestCase {
 			->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))
+				$this->groupManager->method('getUserGroups')
-					->method('getUserGroupIds')
+					->with($this->user)
-					->withConsecutive(
+					->willReturn($groups);
-						[$this->user],
+
-						[$singleUser]
+				$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)))
+			foreach ($userResponse as $groupDefinition) {
-				->method('displayNamesInGroup')
+				[$gid, $search, $limit, $offset, $users] = $groupDefinition;
-				->with($this->anything(), $searchTerm, $this->limit, $this->offset)
+				$groups[$gid]->method('searchDisplayName')
-				->willReturnMap($userResponse);
+					->with($search, $limit, $offset)
+					->willReturn($users);
+			}
 		}
 
 		if ($singleUser !== false) {
@@ -457,24 +472,24 @@ class UserPluginTest extends TestCase {
 		$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,
-			]
+			],
 		];
 	}
 
@@ -489,8 +504,8 @@ class UserPluginTest extends TestCase {
 
 		$this->session->expects($this->once())
 			->method('getUser')
-			->willReturnCallback(function() use ($currentUserId) {
+			->willReturnCallback(function () use ($currentUserId) {
-				if($currentUserId !== null) {
+				if ($currentUserId !== null) {
 					return $this->getUserMock($currentUserId, $currentUserId);
 				}
 				return null;
@@ -506,55 +521,55 @@ class UserPluginTest extends TestCase {
 				'test',
 				['groupA'],
 				[
-					[ 'uid' => 'test1', 'groups' => ['groupA'] ],
+					['uid' => 'test1', 'groups' => ['groupA']],
-					[ 'uid' => 'test2', 'groups' => ['groupB'] ]
+					['uid' => 'test2', 'groups' => ['groupB']],
 				],
-				['test1']
+				['test1'],
 			],
 			[
 				'test',
 				['groupA'],
 				[
-					[ 'uid' => 'test1', 'groups' => ['groupA'] ],
+					['uid' => 'test1', 'groups' => ['groupA']],
-					[ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ]
+					['uid' => 'test2', 'groups' => ['groupB', 'groupA']],
 				],
-				['test1', 'test2']
+				['test1', 'test2'],
 			],
 			[
 				'test',
 				['groupA'],
 				[
-					[ 'uid' => 'test1', 'groups' => ['groupA', 'groupC'] ],
+					['uid' => 'test1', 'groups' => ['groupA', 'groupC']],
-					[ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ]
+					['uid' => 'test2', 'groups' => ['groupB', 'groupA']],
 				],
-				['test1', 'test2']
+				['test1', 'test2'],
 			],
 			[
 				'test',
 				['groupC', 'groupB'],
 				[
-					[ 'uid' => 'test1', 'groups' => ['groupA', 'groupC'] ],
+					['uid' => 'test1', 'groups' => ['groupA', 'groupC']],
-					[ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ]
+					['uid' => 'test2', 'groups' => ['groupB', 'groupA']],
 				],
-				['test1', 'test2']
+				['test1', 'test2'],
 			],
 			[
 				'test',
 				[],
 				[
-					[ 'uid' => 'test1', 'groups' => ['groupA'] ],
+					['uid' => 'test1', 'groups' => ['groupA']],
-					[ 'uid' => 'test2', 'groups' => ['groupB', 'groupA'] ]
+					['uid' => 'test2', 'groups' => ['groupB', 'groupA']],
 				],
-				[]
+				[],
 			],
 			[
 				'test',
 				['groupC', 'groupB'],
 				[
-					[ 'uid' => 'test1', 'groups' => [] ],
+					['uid' => 'test1', 'groups' => []],
-					[ 'uid' => 'test2', 'groups' => [] ]
+					['uid' => 'test2', 'groups' => []],
 				],
-				[]
+				[],
 			],
 		];
 	}
@@ -570,7 +585,7 @@ class UserPluginTest extends TestCase {
 		}, $matchingUsers);
 
 		$mappedResult = array_map(function ($user) {
-			return ['label' => $user, 'value' => [ 'shareType' => 0, 'shareWith' => $user ]];
+			return ['label' => $user, 'value' => ['shareType' => 0, 'shareWith' => $user]];
 		}, $result);
 
 		$this->userManager->expects($this->once())