Use group display name in sharing API + UI

owncloud · Dec 5, 2016 · 8d5e6a1 · 8d5e6a1
1 parent 5c84c53
commit 8d5e6a1
Show file tree

Hide file tree

Showing 8 changed files with 142 additions and 16 deletions.
diff --git a/apps/files_sharing/lib/API/Share20OCS.php b/apps/files_sharing/lib/API/Share20OCS.php
@@ -144,8 +144,9 @@ protected function formatShare(\OCP\Share\IShare $share) {
 			$result['share_with'] = $share->getSharedWith();
 			$result['share_with_displayname'] = $sharedWith !== null ? $sharedWith->getDisplayName() : $share->getSharedWith();
 		} else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) {
+			$group = $this->groupManager->get($share->getSharedWith());
 			$result['share_with'] = $share->getSharedWith();
-			$result['share_with_displayname'] = $share->getSharedWith();
+			$result['share_with_displayname'] = $group !== null ? $group->getDisplayName() : $share->getSharedWith();
 		} else if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK) {
 
 			$result['share_with'] = $share->getPassword();

diff --git a/apps/files_sharing/lib/Controller/ShareesController.php b/apps/files_sharing/lib/Controller/ShareesController.php
@@ -158,8 +158,9 @@ protected function getUsers($search) {
 		}
 
 		$foundUserById = false;
+		$lowerSearch = strtolower($search);
 		foreach ($users as $uid => $userDisplayName) {
-			if (strtolower($uid) === strtolower($search) || strtolower($userDisplayName) === strtolower($search)) {
+			if (strtolower($uid) === $lowerSearch || strtolower($userDisplayName) === $lowerSearch) {
 				if (strtolower($uid) === strtolower($search)) {
 					$foundUserById = true;
 				}
@@ -218,7 +219,7 @@ protected function getGroups($search) {
 		$this->result['groups'] = $this->result['exact']['groups'] = [];
 
 		$groups = $this->groupManager->search($search, $this->limit, $this->offset);
-		$groups = array_map(function (IGroup $group) { return $group->getGID(); }, $groups);
+		$groupIds = array_map(function (IGroup $group) { return $group->getGID(); }, $groups);
 
 		if (!$this->shareeEnumeration || sizeof($groups) < $this->limit) {
 			$this->reachedEndFor[] = 'groups';
@@ -229,21 +230,26 @@ protected function getGroups($search) {
 			// Intersect all the groups that match with the groups this user is a member of
 			$userGroups = $this->groupManager->getUserGroups($this->userSession->getUser());
 			$userGroups = array_map(function (IGroup $group) { return $group->getGID(); }, $userGroups);
-			$groups = array_intersect($groups, $userGroups);
+			$groupIds = array_intersect($groupIds, $userGroups);
 		}
 
-		foreach ($groups as $gid) {
-			if (strtolower($gid) === strtolower($search)) {
+		foreach ($groups as $group) {
+			// FIXME: use a more efficient approach
+			$gid = $group->getGID();
+			if (!in_array($gid, $groupIds)) {
+				continue;
+			}
+			if (strtolower($gid) === strtolower($search) || strtolower($group->getDisplayName()) === strtolower($search)) {
 				$this->result['exact']['groups'][] = [
-					'label' => $gid,
+					'label' => $group->getDisplayName(),
 					'value' => [
 						'shareType' => Share::SHARE_TYPE_GROUP,
 						'shareWith' => $gid,
 					],
 				];
 			} else {
 				$this->result['groups'][] = [
-					'label' => $gid,
+					'label' => $group->getDisplayName(),
 					'value' => [
 						'shareType' => Share::SHARE_TYPE_GROUP,
 						'shareWith' => $gid,
@@ -258,7 +264,7 @@ protected function getGroups($search) {
 			$group = $this->groupManager->get($search);
 			if ($group instanceof IGroup && (!$this->shareWithGroupOnly || in_array($group->getGID(), $userGroups))) {
 				array_push($this->result['exact']['groups'], [
-					'label' => $group->getGID(),
+					'label' => $group->getDisplayName(),
 					'value' => [
 						'shareType' => Share::SHARE_TYPE_GROUP,
 						'shareWith' => $group->getGID(),

diff --git a/apps/files_sharing/tests/API/Share20OCSTest.php b/apps/files_sharing/tests/API/Share20OCSTest.php
@@ -2080,9 +2080,10 @@ public function dataFormatShare() {
 			], $share, [], false
 		];
 
+		// with existing group
 		$share = \OC::$server->getShareManager()->newShare();
 		$share->setShareType(Share::SHARE_TYPE_GROUP)
-			->setSharedWith('recipient')
+			->setSharedWith('recipientGroup')
 			->setSharedBy('initiator')
 			->setShareOwner('owner')
 			->setPermissions(\OCP\Constants::PERMISSION_READ)
@@ -2112,8 +2113,47 @@ public function dataFormatShare() {
 				'file_source' => 3,
 				'file_parent' => 1,
 				'file_target' => 'myTarget',
-				'share_with' => 'recipient',
-				'share_with_displayname' => 'recipient',
+				'share_with' => 'recipientGroup',
+				'share_with_displayname' => 'recipientGroupDisplayName',
+				'mail_send' => 0,
+				'mimetype' => 'myMimeType',
+			], $share, [], false
+		];
+
+		// with unknown group / no group backend
+		$share = \OC::$server->getShareManager()->newShare();
+		$share->setShareType(Share::SHARE_TYPE_GROUP)
+			->setSharedWith('recipientGroup2')
+			->setSharedBy('initiator')
+			->setShareOwner('owner')
+			->setPermissions(\OCP\Constants::PERMISSION_READ)
+			->setNode($file)
+			->setShareTime(new \DateTime('2000-01-01T00:01:02'))
+			->setTarget('myTarget')
+			->setId(42);
+		$result[] = [
+			[
+				'id' => 42,
+				'share_type' => Share::SHARE_TYPE_GROUP,
+				'uid_owner' => 'initiator',
+				'displayname_owner' => 'initiator',
+				'permissions' => 1,
+				'stime' => 946684862,
+				'parent' => null,
+				'expiration' => null,
+				'token' => null,
+				'uid_file_owner' => 'owner',
+				'displayname_file_owner' => 'owner',
+				'path' => 'file',
+				'item_type' => 'file',
+				'storage_id' => 'storageId',
+				'storage' => 100,
+				'item_source' => 3,
+				'file_source' => 3,
+				'file_parent' => 1,
+				'file_target' => 'myTarget',
+				'share_with' => 'recipientGroup2',
+				'share_with_displayname' => 'recipientGroup2',
 				'mail_send' => 0,
 				'mimetype' => 'myMimeType',
 			], $share, [], false
@@ -2229,6 +2269,13 @@ public function dataFormatShare() {
 	 */
 	public function testFormatShare(array $expects, \OCP\Share\IShare $share, array $users, $exception) {
 		$this->userManager->method('get')->will($this->returnValueMap($users));
+
+		$recipientGroup = $this->createMock('\OCP\IGroup');
+		$recipientGroup->method('getDisplayName')->willReturn('recipientGroupDisplayName');
+		$this->groupManager->method('get')->will($this->returnValueMap([
+			 ['recipientGroup', $recipientGroup],
+		]));
+
 		$this->urlGenerator->method('linkToRouteAbsolute')
 			->with('files_sharing.sharecontroller.showShare', ['token' => 'myToken'])
 			->willReturn('myLink');

diff --git a/apps/files_sharing/tests/API/ShareesTest.php b/apps/files_sharing/tests/API/ShareesTest.php
@@ -134,7 +134,7 @@ protected function getUserMock($uid, $displayName) {
 	 * @param string $gid
 	 * @return \OCP\IGroup|\PHPUnit_Framework_MockObject_MockObject
 	 */
-	protected function getGroupMock($gid) {
+	protected function getGroupMock($gid, $displayName = null) {
 		$group = $this->getMockBuilder(IGroup::class)
 			->disableOriginalConstructor()
 			->getMock();
@@ -143,6 +143,15 @@ protected function getGroupMock($gid) {
 			->method('getGID')
 			->willReturn($gid);
 
+		if (is_null($displayName)) {
+			// note: this is how the Group class behaves
+			$displayName = $gid;
+		}
+
+		$group->expects($this->any())
+			->method('getDisplayName')
+			->willReturn($displayName);
+
 		return $group;
 	}
 
@@ -468,6 +477,7 @@ public function dataGetGroups() {
 		return [
 			['test', false, true, [], [], [], [], true, false],
 			['test', false, false, [], [], [], [], true, false],
+			// group without display name
 			[
 				'test', false, true,
 				[$this->getGroupMock('test1')],
@@ -477,6 +487,36 @@ public function dataGetGroups() {
 				true,
 				false,
 			],
+			// group with display name, search by id
+			[
+				'test', false, true,
+				[$this->getGroupMock('test1', 'Test One')],
+				[],
+				[],
+				[['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]],
+				true,
+				false,
+			],
+			// group with display name, search by display name
+			[
+				'one', false, true,
+				[$this->getGroupMock('test1', 'Test One')],
+				[],
+				[],
+				[['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]],
+				true,
+				false,
+			],
+			// group with display name, search by display name, exact expected
+			[
+				'Test One', false, true,
+				[$this->getGroupMock('test1', 'Test One')],
+				[],
+				[['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => 'test1']]],
+				[],
+				true,
+				false,
+			],
 			[
 				'test', false, false,
 				[$this->getGroupMock('test1')],

diff --git a/core/js/sharedialogresharerinfoview.js b/core/js/sharedialogresharerinfoview.js
@@ -78,7 +78,7 @@
 					'core',
 					'Shared with you and the group {group} by {owner}',
 					{
-						group: this.model.getReshareWith(),
+						group: this.model.getReshareWithDisplayName(),
 						owner: ownerDisplayName
 					}
 				);

diff --git a/core/js/shareitemmodel.js b/core/js/shareitemmodel.js
@@ -337,6 +337,14 @@
 			return this.get('reshare').share_with;
 		},
 
+		/**
+		 * @returns {string}
+		 */
+		getReshareWithDisplayName: function() {
+			var reshare = this.get('reshare');
+			return reshare.share_with_displayname || reshare.share_with;
+		},
+
 		/**
 		 * @returns {number}
 		 */

diff --git a/core/js/tests/specs/sharedialogviewSpec.js b/core/js/tests/specs/sharedialogviewSpec.js
@@ -1038,16 +1038,35 @@ describe('OC.Share.ShareDialogView', function() {
 			dialog.render();
 			expect(dialog.$el.find('.shareWithField').prop('disabled')).toEqual(true);
 		});
-		it('shows reshare owner', function() {
+		it('shows reshare owner for single user share', function() {
 			shareModel.set({
 				reshare: {
-					uid_owner: 'user1'
+					uid_owner: 'user1',
+					displayname_owner: 'User One',
+					share_type: OC.Share.SHARE_TYPE_USER
 				},
 				shares: [],
 				permissions: OC.PERMISSION_READ
 			});
 			dialog.render();
 			expect(dialog.$el.find('.resharerInfoView .reshare').length).toEqual(1);
+			expect(dialog.$el.find('.resharerInfoView .reshare').text().trim()).toEqual('Shared with you by User One');
+		});
+		it('shows reshare owner for single user share', function() {
+			shareModel.set({
+				reshare: {
+					uid_owner: 'user1',
+					displayname_owner: 'User One',
+					share_with: 'group2',
+					share_with_displayname: 'Group Two',
+					share_type: OC.Share.SHARE_TYPE_GROUP
+				},
+				shares: [],
+				permissions: OC.PERMISSION_READ
+			});
+			dialog.render();
+			expect(dialog.$el.find('.resharerInfoView .reshare').length).toEqual(1);
+			expect(dialog.$el.find('.resharerInfoView .reshare').text().trim()).toEqual('Shared with you and the group Group Two by User One');
 		});
 		it('does not show reshare owner if owner is current user', function() {
 			shareModel.set({

diff --git a/core/js/tests/specs/shareitemmodelSpec.js b/core/js/tests/specs/shareitemmodelSpec.js
@@ -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 */