From e4b55da071bdae17dc5aff1d7bdb15013b7664c8 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 30 Nov 2016 20:56:10 +0100 Subject: [PATCH] Use group display name in sharing API + UI --- apps/files_sharing/lib/API/Share20OCS.php | 3 +- .../lib/Controller/ShareesController.php | 30 +++++++---- .../tests/API/Share20OCSTest.php | 53 +++++++++++++++++-- apps/files_sharing/tests/API/ShareesTest.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/API/Share20OCS.php b/apps/files_sharing/lib/API/Share20OCS.php index 9280124d99a6..64f09f4782d1 100644 --- 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 index 9eb978deccbd..03c9a92551cc 100644 --- a/apps/files_sharing/lib/Controller/ShareesController.php +++ b/apps/files_sharing/lib/Controller/ShareesController.php @@ -158,9 +158,10 @@ 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) === strtolower($search)) { + if (strtolower($uid) === $lowerSearch || strtolower($userDisplayName) === $lowerSearch) { + if (strtolower($uid) === $lowerSearch) { $foundUserById = true; } $this->result['exact']['users'][] = [ @@ -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,13 +230,19 @@ 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)) { + $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, @@ -243,7 +250,7 @@ protected function getGroups($search) { ]; } else { $this->result['groups'][] = [ - 'label' => $gid, + 'label' => $group->getDisplayName(), 'value' => [ 'shareType' => Share::SHARE_TYPE_GROUP, 'shareWith' => $gid, @@ -258,7 +265,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(), @@ -292,10 +299,11 @@ protected function getRemote($search) { 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) { $foundRemoteById = true; } $this->result['exact']['remotes'][] = [ diff --git a/apps/files_sharing/tests/API/Share20OCSTest.php b/apps/files_sharing/tests/API/Share20OCSTest.php index ed4718ab4b87..6aa8495a9c74 100644 --- 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 index 2aa2571fc89e..fa87af8d33a6 100644 --- 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 index 600e2ecbf56d..a3a4e97c2b67 100644 --- 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 index c0a285f8822d..3e9fb07fcd8b 100644 --- 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 index f0e027d9fb91..2762baacacc8 100644 --- 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 index 9d9001dc9e8f..88d0ac1b9df2 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 */