From 14256d631cf32d8774c7fcffe322d65f964fcd5d Mon Sep 17 00:00:00 2001
From: Vincent Petry <pvince81@owncloud.com>
Date: Wed, 30 Nov 2016 20:56:10 +0100
Subject: [PATCH] Use group display name in sharing API + UI
---
.../lib/Controller/ShareAPIController.php | 3 +-
.../lib/Controller/ShareesAPIController.php | 30 +++++++----
.../Controller/ShareAPIControllerTest.php | 53 +++++++++++++++++--
.../Controller/ShareesAPIControllerTest.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/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index 90274beba49..ccd0e3aa31d 100644
--- a/apps/files_sharing/lib/Controller/ShareAPIController.php
+++ b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -168,8 +168,9 @@ class ShareAPIController extends OCSController {
$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/ShareesAPIController.php b/apps/files_sharing/lib/Controller/ShareesAPIController.php
index 6b3208dc289..cd5139693cf 100644
--- a/apps/files_sharing/lib/Controller/ShareesAPIController.php
+++ b/apps/files_sharing/lib/Controller/ShareesAPIController.php
@@ -165,9 +165,10 @@ class ShareesAPIController extends OCSController {
}
$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'][] = [
@@ -225,7 +226,7 @@ class ShareesAPIController extends OCSController {
$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';
@@ -236,13 +237,19 @@ class ShareesAPIController extends OCSController {
// 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,
@@ -250,7 +257,7 @@ class ShareesAPIController extends OCSController {
];
} else {
$this->result['groups'][] = [
- 'label' => $gid,
+ 'label' => $group->getDisplayName(),
'value' => [
'shareType' => Share::SHARE_TYPE_GROUP,
'shareWith' => $gid,
@@ -265,7 +272,7 @@ class ShareesAPIController extends OCSController {
$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(),
@@ -299,10 +306,11 @@ class ShareesAPIController extends OCSController {
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) {
$result['exactIdMatch'] = true;
}
$result['exact'][] = [
diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
index ed4aa1dba9e..6f6529a2be7 100644
--- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
@@ -1882,9 +1882,10 @@ class ShareAPIControllerTest extends \Test\TestCase {
], $share, [], false
];
+ // with existing group
$share = \OC::$server->getShareManager()->newShare();
$share->setShareType(\OCP\Share::SHARE_TYPE_GROUP)
- ->setSharedWith('recipient')
+ ->setSharedWith('recipientGroup')
->setSharedBy('initiator')
->setShareOwner('owner')
->setPermissions(\OCP\Constants::PERMISSION_READ)
@@ -1914,8 +1915,47 @@ class ShareAPIControllerTest extends \Test\TestCase {
'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
@@ -2029,6 +2069,13 @@ class ShareAPIControllerTest extends \Test\TestCase {
*/
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/Controller/ShareesAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
index c570cb16980..c68e2304743 100644
--- a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
@@ -133,7 +133,7 @@ class ShareesAPIControllerTest extends TestCase {
* @param string $gid
* @return \OCP\IGroup|\PHPUnit_Framework_MockObject_MockObject
*/
- protected function getGroupMock($gid) {
+ protected function getGroupMock($gid, $displayName = null) {
$group = $this->getMockBuilder('OCP\IGroup')
->disableOriginalConstructor()
->getMock();
@@ -142,6 +142,15 @@ class ShareesAPIControllerTest extends TestCase {
->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;
}
@@ -467,6 +476,7 @@ class ShareesAPIControllerTest extends TestCase {
return [
['test', false, true, [], [], [], [], true, false],
['test', false, false, [], [], [], [], true, false],
+ // group without display name
[
'test', false, true,
[$this->getGroupMock('test1')],
@@ -476,6 +486,36 @@ class ShareesAPIControllerTest extends TestCase {
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 654eebf4997..9a9d95cfb60 100644
--- a/core/js/sharedialogresharerinfoview.js
+++ b/core/js/sharedialogresharerinfoview.js
@@ -80,7 +80,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 b01f0f790ac..9b10f067afc 100644
--- a/core/js/shareitemmodel.js
+++ b/core/js/shareitemmodel.js
@@ -344,6 +344,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 985610d51fb..cbb74714ff7 100644
--- a/core/js/tests/specs/sharedialogviewSpec.js
+++ b/core/js/tests/specs/sharedialogviewSpec.js
@@ -975,16 +975,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 1cddcb2acaa..3d3baf75d15 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 */
--
GitLab