From 7a3f68ec8b03f7be688fd30976910725ae9ac7a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 24 Aug 2017 17:28:15 +0200 Subject: [PATCH 1/6] Provide user search besides just display name search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is simply a generalization of the current "displayNamesInGroup" method; it will be needed in a following commit to get extended data for a user in a method in which "displayNamesInGroup" is currently used. Signed-off-by: Daniel Calviño Sánchez --- lib/private/Group/Manager.php | 24 ++- lib/public/IGroupManager.php | 12 ++ tests/lib/Group/ManagerTest.php | 300 ++++++++++++++++++++++++++++++++ 3 files changed, 332 insertions(+), 4 deletions(-) diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 6d4f5a091c696..4cdad707975a5 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -313,14 +313,14 @@ public function getUserGroupIds(IUser $user) { } /** - * get a list of all display names in a group + * get a list of all users in a group * @param string $gid * @param string $search * @param int $limit * @param int $offset - * @return array an array of display names (value) and user ids (key) + * @return array an array of users (value) and user ids (key) */ - public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0) { + public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { $group = $this->get($gid); if(is_null($group)) { return array(); @@ -358,7 +358,23 @@ public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0 $matchingUsers = array(); foreach($groupUsers as $groupUser) { - $matchingUsers[$groupUser->getUID()] = $groupUser->getDisplayName(); + $matchingUsers[$groupUser->getUID()] = $groupUser; + } + return $matchingUsers; + } + + /** + * get a list of all display names in a group + * @param string $gid + * @param string $search + * @param int $limit + * @param int $offset + * @return array an array of display names (value) and user ids (key) + */ + public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0) { + $matchingUsers = array(); + foreach($this->usersInGroup($gid, $search, $limit, $offset) as $uid => $user) { + $matchingUsers[$uid] = $user->getDisplayName(); } return $matchingUsers; } diff --git a/lib/public/IGroupManager.php b/lib/public/IGroupManager.php index be322b6432546..139f1b7a851b5 100644 --- a/lib/public/IGroupManager.php +++ b/lib/public/IGroupManager.php @@ -109,6 +109,18 @@ public function getUserGroups(IUser $user = null); */ public function getUserGroupIds(IUser $user); + /** + * get a list of all users in a group + * + * @param string $gid + * @param string $search + * @param int $limit + * @param int $offset + * @return array an array of users (value) and user ids (key) + * @since 13.0.0 + */ + public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0); + /** * get a list of all display names in a group * diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 23a35024e0a96..e67e1fa0ba6e3 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -498,6 +498,306 @@ public function testGetUserGroupsMultipleBackends() { $this->assertEquals('group2', $group2->getGID()); } + public function testUsersInGroupWithOneUserBackend() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->any()) + ->method('inGroup') + ->will($this->returnCallback(function($uid, $gid) { + switch($uid) { + case 'user1' : return false; + case 'user2' : return true; + case 'user3' : return false; + case 'user33': return true; + default: + return null; + } + })); + + $this->userManager->expects($this->any()) + ->method('searchDisplayName') + ->with('user3') + ->will($this->returnCallback(function($search, $limit, $offset) { + switch($offset) { + case 0 : return ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')]; + case 2 : return []; + } + return null; + })); + $this->userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($this->userManager, $this->logger); + $manager->addBackend($backend); + + $users = $manager->usersInGroup('testgroup', 'user3'); + $this->assertCount(1, $users); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + } + + public function testUsersInGroupWithOneUserBackendWithLimitSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->any()) + ->method('inGroup') + ->will($this->returnCallback(function($uid, $gid) { + switch($uid) { + case 'user1' : return false; + case 'user2' : return true; + case 'user3' : return false; + case 'user33': return true; + case 'user333': return true; + default: + return null; + } + })); + + $this->userManager->expects($this->any()) + ->method('searchDisplayName') + ->with('user3') + ->will($this->returnCallback(function($search, $limit, $offset) { + switch($offset) { + case 0 : return ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')]; + case 2 : return ['user333' => $this->getTestUser('user333')]; + } + return null; + })); + $this->userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + case 'user333': return $this->getTestUser('user333'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($this->userManager, $this->logger); + $manager->addBackend($backend); + + $users = $manager->usersInGroup('testgroup', 'user3', 1); + $this->assertCount(1, $users); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + $this->assertFalse(isset($users['user333'])); + } + + public function testUsersInGroupWithOneUserBackendWithLimitAndOffsetSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->any()) + ->method('inGroup') + ->will($this->returnCallback(function($uid) { + switch($uid) { + case 'user1' : return false; + case 'user2' : return true; + case 'user3' : return false; + case 'user33': return true; + case 'user333': return true; + default: + return null; + } + })); + + $this->userManager->expects($this->any()) + ->method('searchDisplayName') + ->with('user3') + ->will($this->returnCallback(function($search, $limit, $offset) { + switch($offset) { + case 0 : + return [ + 'user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33'), + 'user333' => $this->getTestUser('user333') + ]; + } + return null; + })); + $this->userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + case 'user333': return $this->getTestUser('user333'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($this->userManager, $this->logger); + $manager->addBackend($backend); + + $users = $manager->usersInGroup('testgroup', 'user3', 1, 1); + $this->assertCount(1, $users); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertFalse(isset($users['user33'])); + $this->assertTrue(isset($users['user333'])); + } + + public function testUsersInGroupWithOneUserBackendAndSearchEmpty() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('usersInGroup') + ->with('testgroup', '', -1, 0) + ->will($this->returnValue(array('user2', 'user33'))); + + $this->userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($this->userManager, $this->logger); + $manager->addBackend($backend); + + $users = $manager->usersInGroup('testgroup', ''); + $this->assertCount(2, $users); + $this->assertFalse(isset($users['user1'])); + $this->assertTrue(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + } + + public function testUsersInGroupWithOneUserBackendAndSearchEmptyAndLimitSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('usersInGroup') + ->with('testgroup', '', 1, 0) + ->will($this->returnValue(array('user2'))); + + $this->userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($this->userManager, $this->logger); + $manager->addBackend($backend); + + $users = $manager->displayNamesInGroup('testgroup', '', 1); + $this->assertCount(1, $users); + $this->assertFalse(isset($users['user1'])); + $this->assertTrue(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertFalse(isset($users['user33'])); + } + + public function testUsersInGroupWithOneUserBackendAndSearchEmptyAndLimitAndOffsetSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('usersInGroup') + ->with('testgroup', '', 1, 1) + ->will($this->returnValue(array('user33'))); + + $this->userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($this->userManager, $this->logger); + $manager->addBackend($backend); + + $users = $manager->usersInGroup('testgroup', '', 1, 1); + $this->assertCount(1, $users); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + } + public function testDisplayNamesInGroupWithOneUserBackend() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend From 6eeb15d5b66e215b2213537f09bbdaa00a4097d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 24 Aug 2017 17:44:38 +0200 Subject: [PATCH 2/6] Refactor how user data is appended to the result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will make possible to conditionally add other fields like the e-mail address in a following commit. Signed-off-by: Daniel Calviño Sánchez --- .../lib/Controller/ShareesAPIController.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareesAPIController.php b/apps/files_sharing/lib/Controller/ShareesAPIController.php index 94a4854dbde87..3c0a69f538281 100644 --- a/apps/files_sharing/lib/Controller/ShareesAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareesAPIController.php @@ -181,21 +181,23 @@ protected function getUsers($search) { if (strtolower($uid) === $lowerSearch) { $foundUserById = true; } - $this->result['exact']['users'][] = [ + $userData = [ 'label' => $userDisplayName, 'value' => [ 'shareType' => Share::SHARE_TYPE_USER, 'shareWith' => $uid, ], ]; + $this->result['exact']['users'][] = $userData; } else { - $this->result['users'][] = [ + $userData = [ 'label' => $userDisplayName, 'value' => [ 'shareType' => Share::SHARE_TYPE_USER, 'shareWith' => $uid, ], ]; + $this->result['users'][] = $userData; } } @@ -213,13 +215,14 @@ protected function getUsers($search) { } if ($addUser) { - array_push($this->result['exact']['users'], [ + $userData = [ 'label' => $user->getDisplayName(), 'value' => [ 'shareType' => Share::SHARE_TYPE_USER, 'shareWith' => $user->getUID(), ], - ]); + ]; + $this->result['exact']['users'][] = $userData; } } } From 0587979326406bd20ed62e5e0b096974893aa82b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 24 Aug 2017 18:02:18 +0200 Subject: [PATCH 3/6] Refactor to search for users instead of just for display names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will be needed in a following commit to return extended user data in the result. Signed-off-by: Daniel Calviño Sánchez --- .../lib/Controller/ShareesAPIController.php | 16 +++++----- .../Controller/ShareesAPIControllerTest.php | 30 +++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareesAPIController.php b/apps/files_sharing/lib/Controller/ShareesAPIController.php index 3c0a69f538281..9646ed852a3ab 100644 --- a/apps/files_sharing/lib/Controller/ShareesAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareesAPIController.php @@ -156,9 +156,9 @@ protected function getUsers($search) { // Search in all the groups this user is part of $userGroups = $this->groupManager->getUserGroupIds($this->userSession->getUser()); foreach ($userGroups as $userGroup) { - $usersTmp = $this->groupManager->displayNamesInGroup($userGroup, $search, $this->limit, $this->offset); - foreach ($usersTmp as $uid => $userDisplayName) { - $users[$uid] = $userDisplayName; + $usersTmp = $this->groupManager->usersInGroup($userGroup, $search, $this->limit, $this->offset); + foreach ($usersTmp as $uid => $user) { + $users[$uid] = $user; } } } else { @@ -166,7 +166,7 @@ protected function getUsers($search) { $usersTmp = $this->userManager->searchDisplayName($search, $this->limit, $this->offset); foreach ($usersTmp as $user) { - $users[$user->getUID()] = $user->getDisplayName(); + $users[$user->getUID()] = $user; } } @@ -176,13 +176,13 @@ protected function getUsers($search) { $foundUserById = false; $lowerSearch = strtolower($search); - foreach ($users as $uid => $userDisplayName) { - if (strtolower($uid) === $lowerSearch || strtolower($userDisplayName) === $lowerSearch) { + foreach ($users as $uid => $user) { + if (strtolower($uid) === $lowerSearch || strtolower($user->getDisplayName()) === $lowerSearch) { if (strtolower($uid) === $lowerSearch) { $foundUserById = true; } $userData = [ - 'label' => $userDisplayName, + 'label' => $user->getDisplayName(), 'value' => [ 'shareType' => Share::SHARE_TYPE_USER, 'shareWith' => $uid, @@ -191,7 +191,7 @@ protected function getUsers($search) { $this->result['exact']['users'][] = $userData; } else { $userData = [ - 'label' => $userDisplayName, + 'label' => $user->getDisplayName(), 'value' => [ 'shareType' => Share::SHARE_TYPE_USER, 'shareWith' => $uid, diff --git a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php index e027d0751cb75..7ba17f410dcdf 100644 --- a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php @@ -302,7 +302,7 @@ public function dataGetUsers() { true, ['abc', 'xyz'], [ - ['abc', 'test', 2, 0, ['test1' => 'Test One']], + ['abc', 'test', 2, 0, ['test1' => $this->getUserMock('test1', 'Test One')]], ['xyz', 'test', 2, 0, []], ], [], @@ -318,7 +318,7 @@ public function dataGetUsers() { false, ['abc', 'xyz'], [ - ['abc', 'test', 2, 0, ['test1' => 'Test One']], + ['abc', 'test', 2, 0, ['test1' => $this->getUserMock('test1', 'Test One')]], ['xyz', 'test', 2, 0, []], ], [], @@ -333,12 +333,12 @@ public function dataGetUsers() { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + 'test1' => $this->getUserMock('test1', 'Test One'), + 'test2' => $this->getUserMock('test2', 'Test Two'), ]], ['xyz', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + 'test1' => $this->getUserMock('test1', 'Test One'), + 'test2' => $this->getUserMock('test2', 'Test Two'), ]], ], [], @@ -356,12 +356,12 @@ public function dataGetUsers() { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + 'test1' => $this->getUserMock('test1', 'Test One'), + 'test2' => $this->getUserMock('test2', 'Test Two'), ]], ['xyz', 'test', 2, 0, [ - 'test1' => 'Test One', - 'test2' => 'Test Two', + 'test1' => $this->getUserMock('test1', 'Test One'), + 'test2' => $this->getUserMock('test2', 'Test Two'), ]], ], [], @@ -376,10 +376,10 @@ public function dataGetUsers() { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test' => 'Test One', + 'test' => $this->getUserMock('test', 'Test One'), ]], ['xyz', 'test', 2, 0, [ - 'test2' => 'Test Two', + 'test2' => $this->getUserMock('test2', 'Test Two'), ]], ], [ @@ -398,10 +398,10 @@ public function dataGetUsers() { ['abc', 'xyz'], [ ['abc', 'test', 2, 0, [ - 'test' => 'Test One', + 'test' => $this->getUserMock('test', 'Test One'), ]], ['xyz', 'test', 2, 0, [ - 'test2' => 'Test Two', + 'test2' => $this->getUserMock('test2', 'Test Two'), ]], ], [ @@ -460,7 +460,7 @@ public function testGetUsers($searchTerm, $shareWithGroupOnly, $shareeEnumeratio } $this->groupManager->expects($this->exactly(sizeof($groupResponse))) - ->method('displayNamesInGroup') + ->method('usersInGroup') ->with($this->anything(), $searchTerm, $this->invokePrivate($this->sharees, 'limit'), $this->invokePrivate($this->sharees, 'offset')) ->willReturnMap($userResponse); } From fbb931b2dba2904eb4e1398a952bf0ab25316f53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 24 Aug 2017 19:38:42 +0200 Subject: [PATCH 4/6] Extend getUsers to return the e-mail addresses if available MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When searching the sharees only the name of the sharee and its type was returned. Now, if the sharee is a user, its e-mail address is also returned (if it is set for that user); this will be used by the front-end to provide more actions without needing to query the server again for each matched user. Signed-off-by: Daniel Calviño Sánchez --- .../lib/Controller/ShareesAPIController.php | 9 + .../Controller/ShareesAPIControllerTest.php | 181 +++++++++++++++++- 2 files changed, 189 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/lib/Controller/ShareesAPIController.php b/apps/files_sharing/lib/Controller/ShareesAPIController.php index 9646ed852a3ab..351be04d6b4a4 100644 --- a/apps/files_sharing/lib/Controller/ShareesAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareesAPIController.php @@ -188,6 +188,9 @@ protected function getUsers($search) { 'shareWith' => $uid, ], ]; + if ($user->getEMailAddress()) { + $userData['value']['emailAddress'] = $user->getEMailAddress(); + } $this->result['exact']['users'][] = $userData; } else { $userData = [ @@ -197,6 +200,9 @@ protected function getUsers($search) { 'shareWith' => $uid, ], ]; + if ($user->getEMailAddress()) { + $userData['value']['emailAddress'] = $user->getEMailAddress(); + } $this->result['users'][] = $userData; } } @@ -222,6 +228,9 @@ protected function getUsers($search) { 'shareWith' => $user->getUID(), ], ]; + if ($user->getEMailAddress()) { + $userData['value']['emailAddress'] = $user->getEMailAddress(); + } $this->result['exact']['users'][] = $userData; } } diff --git a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php index 7ba17f410dcdf..0fa31547aa2fe 100644 --- a/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php @@ -119,9 +119,10 @@ protected function setUp() { /** * @param string $uid * @param string $displayName + * @param string $emailAddress * @return \OCP\IUser|\PHPUnit_Framework_MockObject_MockObject */ - protected function getUserMock($uid, $displayName) { + protected function getUserMock($uid, $displayName, $emailAddress = '') { $user = $this->getMockBuilder('OCP\IUser') ->disableOriginalConstructor() ->getMock(); @@ -134,6 +135,10 @@ protected function getUserMock($uid, $displayName) { ->method('getDisplayName') ->willReturn($displayName); + $user->expects($this->any()) + ->method('getEMailAddress') + ->willReturn($emailAddress); + return $user; } @@ -174,12 +179,24 @@ public function dataGetUsers() { ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], ], [], true, $this->getUserMock('test', 'Test') ], + [ + 'test', false, true, [], [], + [ + ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test', 'emailAddress' => 'test@server.com']], + ], [], true, $this->getUserMock('test', 'Test', 'test@server.com') + ], [ 'test', false, false, [], [], [ ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test']], ], [], true, $this->getUserMock('test', 'Test') ], + [ + 'test', false, false, [], [], + [ + ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test', 'emailAddress' => 'test@server.com']], + ], [], true, $this->getUserMock('test', 'Test', 'test@server.com') + ], [ 'test', true, true, [], [], [], [], true, $this->getUserMock('test', 'Test') @@ -194,12 +211,24 @@ public function dataGetUsers() { ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => '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', 'emailAddress' => 'test@server.com']], + ], [], true, $this->getUserMock('test', 'Test', 'test@server.com') + ], [ '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') ], + [ + 'test', true, false, ['test-group'], [['test-group', 'test', 2, 0, []]], + [ + ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test', 'emailAddress' => 'test@server.com']], + ], [], true, $this->getUserMock('test', 'Test', 'test@server.com') + ], [ 'test', false, @@ -215,6 +244,21 @@ public function dataGetUsers() { true, false, ], + [ + 'test', + false, + true, + [], + [ + $this->getUserMock('test1', 'Test One', 'test1@server.com'), + ], + [], + [ + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1', 'emailAddress' => 'test1@server.com']], + ], + true, + false, + ], [ 'test', false, @@ -245,6 +289,23 @@ public function dataGetUsers() { false, false, ], + [ + 'test', + false, + true, + [], + [ + $this->getUserMock('test1', 'Test One', 'test1@server.com'), + $this->getUserMock('test2', 'Test Two', 'test2@server.com'), + ], + [], + [ + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1', 'emailAddress' => 'test1@server.com']], + ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2', 'emailAddress' => 'test2@server.com']], + ], + false, + false, + ], [ 'test', false, @@ -279,6 +340,26 @@ public function dataGetUsers() { false, false, ], + [ + 'test', + false, + true, + [], + [ + $this->getUserMock('test0', 'Test', 'test0@server.com'), + $this->getUserMock('test1', 'Test One', 'test1@server.com'), + $this->getUserMock('test2', 'Test Two', 'test2@server.com'), + ], + [ + ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test0', 'emailAddress' => 'test0@server.com']], + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1', 'emailAddress' => 'test1@server.com']], + ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2', 'emailAddress' => 'test2@server.com']], + ], + false, + false, + ], [ 'test', false, @@ -296,6 +377,23 @@ public function dataGetUsers() { true, false, ], + [ + 'test', + false, + false, + [], + [ + $this->getUserMock('test0', 'Test', 'test0@server.com'), + $this->getUserMock('test1', 'Test One', 'test1@server.com'), + $this->getUserMock('test2', 'Test Two', 'test2@server.com'), + ], + [ + ['label' => 'Test', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test0', 'emailAddress' => 'test0@server.com']], + ], + [], + true, + false, + ], [ 'test', true, @@ -312,6 +410,22 @@ public function dataGetUsers() { true, false, ], + [ + 'test', + true, + true, + ['abc', 'xyz'], + [ + ['abc', 'test', 2, 0, ['test1' => $this->getUserMock('test1', 'Test One', 'test1@server.com')]], + ['xyz', 'test', 2, 0, []], + ], + [], + [ + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1', 'emailAddress' => 'test1@server.com']], + ], + true, + false, + ], [ 'test', true, @@ -349,6 +463,29 @@ public function dataGetUsers() { false, false, ], + [ + 'test', + true, + true, + ['abc', 'xyz'], + [ + ['abc', 'test', 2, 0, [ + 'test1' => $this->getUserMock('test1', 'Test One', 'test1@server.com'), + 'test2' => $this->getUserMock('test2', 'Test Two', 'test2@server.com'), + ]], + ['xyz', 'test', 2, 0, [ + 'test1' => $this->getUserMock('test1', 'Test One', 'test1@server.com'), + 'test2' => $this->getUserMock('test2', 'Test Two', 'test2@server.com'), + ]], + ], + [], + [ + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test1', 'emailAddress' => 'test1@server.com']], + ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2', 'emailAddress' => 'test2@server.com']], + ], + false, + false, + ], [ 'test', true, @@ -391,6 +528,28 @@ public function dataGetUsers() { false, false, ], + [ + 'test', + true, + true, + ['abc', 'xyz'], + [ + ['abc', 'test', 2, 0, [ + 'test' => $this->getUserMock('test', 'Test One', 'test@server.com'), + ]], + ['xyz', 'test', 2, 0, [ + 'test2' => $this->getUserMock('test2', 'Test Two', 'test2@server.com'), + ]], + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test', 'emailAddress' => 'test@server.com']], + ], + [ + ['label' => 'Test Two', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test2', 'emailAddress' => 'test2@server.com']], + ], + false, + false, + ], [ 'test', true, @@ -411,6 +570,26 @@ public function dataGetUsers() { true, false, ], + [ + 'test', + true, + false, + ['abc', 'xyz'], + [ + ['abc', 'test', 2, 0, [ + 'test' => $this->getUserMock('test', 'Test One', 'test@server.com'), + ]], + ['xyz', 'test', 2, 0, [ + 'test2' => $this->getUserMock('test2', 'Test Two', 'test2@server.com'), + ]], + ], + [ + ['label' => 'Test One', 'value' => ['shareType' => Share::SHARE_TYPE_USER, 'shareWith' => 'test', 'emailAddress' => 'test@server.com']], + ], + [], + true, + false, + ], ]; } From 0122aad2a49992a8ff6b1e086f7b200009215360 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 25 Aug 2017 17:18:32 +0200 Subject: [PATCH 5/6] Add method to send again a mail notification for a share MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mail notifications can be resent only to shares with users that have a known e-mail address; e-mail shares are not taken into account, as they have a different workflow. Signed-off-by: Daniel Calviño Sánchez --- lib/private/Share20/Manager.php | 33 +++++++ lib/public/Share/IManager.php | 12 +++ tests/lib/Share20/ManagerTest.php | 157 ++++++++++++++++++++++++++++++ 3 files changed, 202 insertions(+) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 46e3bd75c5cc8..304b83e5a4406 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -677,6 +677,39 @@ public function createShare(\OCP\Share\IShare $share) { return $share; } + /** + * Sends again the e-mail notification for a share + * + * @param \OCP\Share\IShare $share + * @return \OCP\Share\IShare the share object + * @throws \InvalidArgumentException if share type does not support mail + * notifications or the sharee has no known e-mail address + * @throws \Exception if mail could not be sent + */ + public function resendMailNotification(\OCP\Share\IShare $share) { + $this->canShare($share); + + if ($share->getShareType() !== \OCP\Share::SHARE_TYPE_USER) { + throw new \InvalidArgumentException("Share mail notification can be sent only for user shares"); + } + + $user = $this->userManager->get($share->getSharedWith()); + $emailAddress = $user->getEMailAddress(); + + if ($emailAddress === null || $emailAddress === '') { + throw new \InvalidArgumentException("Share mail notification can not be sent to a user without a mail"); + } + + $this->sendMailNotification( + $share->getNode()->getName(), + $this->urlGenerator->linkToRouteAbsolute('files.viewcontroller.showFile', [ 'fileid' => $share->getNode()->getId() ]), + $share->getSharedBy(), + $emailAddress + ); + + return $share; + } + /** * @param string $filename file/folder name * @param string $link link to the file/folder diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index 31f04803066cc..5f726aa248807 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -44,6 +44,18 @@ interface IManager { */ public function createShare(IShare $share); + /** + * Sends again the e-mail notification for a share + * + * @param IShare $share + * @return IShare The share object + * @throws \InvalidArgumentException if share type does not support mail + * notifications or the sharee has no known e-mail address + * @throws \Exception if mail could not be sent + * @since 13.0.0 + */ + public function resendMailNotification(IShare $share); + /** * Update a share. * The target of the share can't be changed this way: use moveShare diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 174aaffd0b668..0fdf0552e21ad 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -2004,6 +2004,163 @@ public function testCreateShareOfIncommingFederatedShare() { $manager->createShare($share); } + public function testResendMailNotificationUserType() { + $manager = $this->createManagerMock() + ->setMethods(['canShare', 'sendMailNotification']) + ->getMock(); + + $file = $this->createMock(File::class); + $file->method('getId')->willReturn('fileId'); + $file->method('getName')->willReturn('fileName'); + + $share = $this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $file, 'user0', 'sharer', null, null); + + $user = $this->createMock(IUser::class); + $user->method('getEMailAddress')->willReturn('user0@server.com'); + + $this->userManager->method('get')->with('user0')->willReturn($user); + + $manager->expects($this->once()) + ->method('canShare') + ->with($share) + ->willReturn(true); + + $this->urlGenerator + ->method('linkToRouteAbsolute') + ->with('files.viewcontroller.showFile', + ['fileid' => 'fileId']) + ->willReturn('absolute/route/to/file'); + + $manager->expects($this->once()) + ->method('sendMailNotification') + ->with('fileName', + 'absolute/route/to/file', + 'sharer', + 'user0@server.com'); + + $manager->resendMailNotification($share); + } + + public function testResendMailNotificationUserTypeWithoutMail() { + $manager = $this->createManagerMock() + ->setMethods(['canShare', 'sendMailNotification']) + ->getMock(); + + $share = $this->createShare(null, \OCP\Share::SHARE_TYPE_USER, null, 'user0', null, null, null); + + $user = $this->createMock(IUser::class); + $user->method('getEMailAddress')->willReturn(''); + + $this->userManager->method('get')->with('user0')->willReturn($user); + + $manager->expects($this->once()) + ->method('canShare') + ->with($share) + ->willReturn(true); + + $manager->expects($this->never()) + ->method('sendMailNotification'); + + try { + $manager->resendMailNotification($share); + + $this->fail('InvalidArgumentException was not thrown'); + } catch (\InvalidArgumentException $e) { + } + } + + public function testResendMailNotificationGroupType() { + $manager = $this->createManagerMock() + ->setMethods(['canShare', 'sendMailNotification']) + ->getMock(); + + $share = $this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, null, null, null, null, null); + + $manager->expects($this->once()) + ->method('canShare') + ->with($share) + ->willReturn(true); + + $manager->expects($this->never()) + ->method('sendMailNotification'); + + try { + $manager->resendMailNotification($share); + + $this->fail('InvalidArgumentException was not thrown'); + } catch (\InvalidArgumentException $e) { + } + } + + public function testResendMailNotificationLinkType() { + $manager = $this->createManagerMock() + ->setMethods(['canShare', 'sendMailNotification']) + ->getMock(); + + $share = $this->createShare(null, \OCP\Share::SHARE_TYPE_LINK, null, null, null, null, null); + + $manager->expects($this->once()) + ->method('canShare') + ->with($share) + ->willReturn(true); + + $manager->expects($this->never()) + ->method('sendMailNotification'); + + try { + $manager->resendMailNotification($share); + + $this->fail('InvalidArgumentException was not thrown'); + } catch (\InvalidArgumentException $e) { + } + } + + public function testResendMailNotificationRemoteType() { + $manager = $this->createManagerMock() + ->setMethods(['canShare', 'sendMailNotification']) + ->getMock(); + + $share = $this->createShare(null, \OCP\Share::SHARE_TYPE_REMOTE, null, null, null, null, null); + + $manager->expects($this->once()) + ->method('canShare') + ->with($share) + ->willReturn(true); + + $manager->expects($this->never()) + ->method('sendMailNotification'); + + try { + $manager->resendMailNotification($share); + + $this->fail('InvalidArgumentException was not thrown'); + } catch (\InvalidArgumentException $e) { + } + } + + public function testResendMailNotificationEmailType() { + $manager = $this->createManagerMock() + ->setMethods(['canShare', 'sendMailNotification']) + ->getMock(); + + $share = $this->createShare(null, \OCP\Share::SHARE_TYPE_EMAIL, null, null, null, null, null); + + $manager->expects($this->once()) + ->method('canShare') + ->with($share) + ->willReturn(true); + + $manager->expects($this->never()) + ->method('sendMailNotification'); + + try { + $manager->resendMailNotification($share); + + $this->fail('InvalidArgumentException was not thrown'); + } catch (\InvalidArgumentException $e) { + } + } + public function testGetSharesBy() { $share = $this->manager->newShare(); From 04e78c178d76fe5290d3f1d6ea624caaf79dadd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 25 Aug 2017 18:43:08 +0200 Subject: [PATCH 6/6] Add API endpoint to resend an e-mail notification for a share MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes possible to trigger the resend from the front-end. Signed-off-by: Daniel Calviño Sánchez --- apps/files_sharing/appinfo/routes.php | 5 + .../lib/Controller/ShareAPIController.php | 25 ++++ .../Controller/ShareAPIControllerTest.php | 112 ++++++++++++++++++ 3 files changed, 142 insertions(+) diff --git a/apps/files_sharing/appinfo/routes.php b/apps/files_sharing/appinfo/routes.php index 310b1c46eb6c9..f8370154fda11 100644 --- a/apps/files_sharing/appinfo/routes.php +++ b/apps/files_sharing/appinfo/routes.php @@ -68,6 +68,11 @@ 'url' => '/api/v1/shares', 'verb' => 'POST', ], + [ + 'name' => 'ShareAPI#resendMailNotification', + 'url' => '/api/v1/shares/{id}/resendMailNotification', + 'verb' => 'POST', + ], [ 'name' => 'ShareAPI#getShare', 'url' => '/api/v1/shares/{id}', diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index c57a738457e2a..1b379b1286311 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -485,6 +485,31 @@ public function createShare( return new DataResponse($output); } + /** + * Sends again the e-mail notification for a share + * + * @NoAdminRequired + * + * @param string $id + * @return DataResponse + * @throws OCSNotFoundException + */ + public function resendMailNotification($id) { + try { + $share = $this->getShareById($id); + } catch (ShareNotFound $e) { + throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist', $id)); + } + + if (!$this->canAccessShare($share)) { + throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist', $id)); + } + + $this->shareManager->resendMailNotification($share); + + return new DataResponse(); + } + /** * @param \OCP\Files\File|\OCP\Files\Folder $node * @param boolean $includeTags diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index f5df9b62b1947..87abf640b79cb 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -1148,6 +1148,118 @@ public function testCreateReshareOfFederatedMountNoDeletePermissions() { $ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_USER, 'validUser'); } + public function testResendMailNotification() { + $share = $this->createShare(42, \OCP\Share::SHARE_TYPE_USER, null, null, null, null, null, null, null, null, null, null); + + /** @var \OCA\Files_Sharing\Controller\ShareAPIController $ocs */ + $ocs = $this->getMockBuilder(ShareAPIController::class) + ->setConstructorArgs([ + $this->appName, + $this->request, + $this->shareManager, + $this->groupManager, + $this->userManager, + $this->rootFolder, + $this->urlGenerator, + $this->currentUser, + $this->l, + ])->setMethods(['canAccessShare']) + ->getMock(); + + $this->shareManager + ->expects($this->once()) + ->method('getShareById') + ->with('ocinternal:42') + ->willReturn($share); + + $ocs->expects($this->once()) + ->method('canAccessShare') + ->with($share) + ->willReturn(true); + + $this->shareManager + ->expects($this->once()) + ->method('resendMailNotification') + ->with($share); + + $ocs->resendMailNotification(42); + } + + /** + * @expectedException \OCP\AppFramework\OCS\OCSNotFoundException + * @expectedExceptionMessage Wrong share ID, share doesn't exist + */ + public function testResendMailNotificationShareNotFound() { + $share = $this->createShare(42, \OCP\Share::SHARE_TYPE_USER, null, null, null, null, null, null, null, null, null, null); + + /** @var \OCA\Files_Sharing\Controller\ShareAPIController $ocs */ + $ocs = $this->getMockBuilder(ShareAPIController::class) + ->setConstructorArgs([ + $this->appName, + $this->request, + $this->shareManager, + $this->groupManager, + $this->userManager, + $this->rootFolder, + $this->urlGenerator, + $this->currentUser, + $this->l, + ])->setMethods(['canAccessShare']) + ->getMock(); + + $this->shareManager + ->expects($this->once()) + ->method('getShareById') + ->with('ocinternal:42') + ->will($this->throwException(new \OCP\Share\Exceptions\ShareNotFound())); + + $this->shareManager + ->expects($this->never()) + ->method('resendMailNotification'); + + $ocs->resendMailNotification(42); + } + + /** + * @expectedException \OCP\AppFramework\OCS\OCSNotFoundException + * @expectedExceptionMessage Wrong share ID, share doesn't exist + */ + public function testResendMailNotificationCanNotAccess() { + $share = $this->createShare(42, \OCP\Share::SHARE_TYPE_USER, null, null, null, null, null, null, null, null, null, null); + + /** @var \OCA\Files_Sharing\Controller\ShareAPIController $ocs */ + $ocs = $this->getMockBuilder(ShareAPIController::class) + ->setConstructorArgs([ + $this->appName, + $this->request, + $this->shareManager, + $this->groupManager, + $this->userManager, + $this->rootFolder, + $this->urlGenerator, + $this->currentUser, + $this->l, + ])->setMethods(['canAccessShare']) + ->getMock(); + + $this->shareManager + ->expects($this->once()) + ->method('getShareById') + ->with('ocinternal:42') + ->willReturn($share); + + $ocs->expects($this->once()) + ->method('canAccessShare') + ->with($share) + ->willReturn(false); + + $this->shareManager + ->expects($this->never()) + ->method('resendMailNotification'); + + $ocs->resendMailNotification(42); + } + /** * @expectedException \OCP\AppFramework\OCS\OCSNotFoundException * @expectedExceptionMessage Wrong share ID, share doesn't exist