From b95d12700ccd5987e580ee30efc50c76019de97a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 25 Mar 2015 15:18:11 +0100 Subject: [PATCH 1/8] Add logic for getting the user from the rss token to the Manager --- lib/private/activitymanager.php | 65 ++++++++++++++++++++++++++++++++ lib/private/server.php | 8 +++- lib/public/activity/imanager.php | 10 +++++ 3 files changed, 81 insertions(+), 2 deletions(-) diff --git a/lib/private/activitymanager.php b/lib/private/activitymanager.php index c6cd5a1fe8..26db0c78df 100644 --- a/lib/private/activitymanager.php +++ b/lib/private/activitymanager.php @@ -28,8 +28,34 @@ namespace OC; use OCP\Activity\IConsumer; use OCP\Activity\IExtension; use OCP\Activity\IManager; +use OCP\IConfig; +use OCP\IRequest; +use OCP\IUserSession; class ActivityManager implements IManager { + /** @var IRequest */ + protected $request; + + /** @var IUserSession */ + protected $session; + + /** @var IConfig */ + protected $config; + + /** + * constructor of the controller + * + * @param IRequest $request + * @param IUserSession $session + * @param IConfig $config + */ + public function __construct(IRequest $request, + IUserSession $session, + IConfig $config) { + $this->request = $request; + $this->session = $session; + $this->config = $config; + } /** * @var \Closure[] @@ -348,4 +374,43 @@ class ActivityManager implements IManager { return array(' and ((' . implode(') or (', $conditions) . '))', $parameters); } + + /** + * Get the user we need to use + * + * Either the user is logged in, or we try to get it from the token + * + * @return string + * @throws \UnexpectedValueException If the token is invalid, does not exist or is not unique + */ + public function getCurrentUserId() { + if (!$this->session->isLoggedIn()) { + return $this->getUserFromToken(); + } else { + return $this->session->getUser()->getUID(); + } + } + + /** + * Get the user for the token + * + * @return string + * @throws \UnexpectedValueException If the token is invalid, does not exist or is not unique + */ + protected function getUserFromToken() { + $token = (string) $this->request->getParam('token', ''); + if (strlen($token) !== 30) { + throw new \UnexpectedValueException('The token is invalid'); + } + + $users = $this->config->getUsersForUserValue('activity', 'rsstoken', $token); + + if (sizeof($users) !== 1) { + // No unique user found + throw new \UnexpectedValueException('The token is invalid'); + } + + // Token found login as that user + return array_shift($users); + } } diff --git a/lib/private/server.php b/lib/private/server.php index 592f8d9a04..6b212ee94a 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -223,8 +223,12 @@ class Server extends SimpleContainer implements IServerContainer { new ArrayCache() ); }); - $this->registerService('ActivityManager', function ($c) { - return new ActivityManager(); + $this->registerService('ActivityManager', function (Server $c) { + return new ActivityManager( + $c->getRequest(), + $c->getUserSession(), + $c->getConfig() + ); }); $this->registerService('AvatarManager', function ($c) { return new AvatarManager(); diff --git a/lib/public/activity/imanager.php b/lib/public/activity/imanager.php index f7885860c4..2e55c8b45b 100644 --- a/lib/public/activity/imanager.php +++ b/lib/public/activity/imanager.php @@ -136,4 +136,14 @@ interface IManager { * @return array */ function getQueryForFilter($filter); + + /** + * Get the user we need to use + * + * Either the user is logged in, or we try to get it from the token + * + * @return string + * @throws \UnexpectedValueException If the token is invalid, does not exist or is not unique + */ + public function getCurrentUserId(); } From 17f882c3cf2d99a786daff56df5da8280746affb Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 25 Mar 2015 15:19:25 +0100 Subject: [PATCH 2/8] Add a filter for favorites and allow limiting the all-list to favorites WARNING: do not use this, when you have a lot of favorites, ~50 should be the limit --- apps/files/lib/activity.php | 71 +++++++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 2 deletions(-) diff --git a/apps/files/lib/activity.php b/apps/files/lib/activity.php index c376dc7b14..c613a07472 100644 --- a/apps/files/lib/activity.php +++ b/apps/files/lib/activity.php @@ -29,11 +29,13 @@ use OCP\IURLGenerator; class Activity implements IExtension { const FILTER_FILES = 'files'; + const FILTER_FAVORITES = 'files_favorites'; const TYPE_SHARE_CREATED = 'file_created'; const TYPE_SHARE_CHANGED = 'file_changed'; const TYPE_SHARE_DELETED = 'file_deleted'; const TYPE_SHARE_RESTORED = 'file_restored'; + const TYPE_FAVORITES = 'files_favorites'; /** @var IL10N */ protected $l; @@ -74,6 +76,7 @@ class Activity implements IExtension { return [ self::TYPE_SHARE_CREATED => (string) $l->t('A new file or folder has been created'), self::TYPE_SHARE_CHANGED => (string) $l->t('A file or folder has been changed'), + self::TYPE_FAVORITES => (string) $l->t('Limit notifications about creation and changes to your favorite files'), self::TYPE_SHARE_DELETED => (string) $l->t('A file or folder has been deleted'), self::TYPE_SHARE_RESTORED => (string) $l->t('A file or folder has been restored'), ]; @@ -230,6 +233,11 @@ class Activity implements IExtension { public function getNavigation() { return [ 'apps' => [ + self::FILTER_FAVORITES => [ + 'id' => self::FILTER_FAVORITES, + 'name' => (string) $this->l->t('Favorites'), + 'url' => $this->URLGenerator->linkToRoute('activity.Activities.showList', ['filter' => self::FILTER_FAVORITES]), + ], self::FILTER_FILES => [ 'id' => self::FILTER_FILES, 'name' => (string) $this->l->t('Files'), @@ -247,7 +255,7 @@ class Activity implements IExtension { * @return boolean */ public function isFilterValid($filterValue) { - return $filterValue === self::FILTER_FILES; + return $filterValue === self::FILTER_FILES || $filterValue === self::FILTER_FAVORITES; } /** @@ -259,7 +267,7 @@ class Activity implements IExtension { * @return array|false */ public function filterNotificationTypes($types, $filter) { - if ($filter === self::FILTER_FILES) { + if ($filter === self::FILTER_FILES || $filter === self::FILTER_FAVORITES) { return array_intersect([ self::TYPE_SHARE_CREATED, self::TYPE_SHARE_CHANGED, @@ -280,9 +288,68 @@ class Activity implements IExtension { * @return array|false */ public function getQueryForFilter($filter) { + $user = \OC::$server->getActivityManager()->getCurrentUserId(); + // Display actions from all files if ($filter === self::FILTER_FILES) { return ['`app` = ?', ['files']]; } + + if (!$user) { + // Remaining filters only work with a user/token + return false; + } + + // Display actions from favorites only + if ($filter === self::FILTER_FAVORITES || $filter === 'all' && $this->userSettingFavoritesOnly($user)) { + $tagManager = \OC::$server->getTagManager(); + + $tags = $tagManager->load('files', [], false, $user); + $favorites = $tags->getFavorites(); + + if (isset($favorites[50])) { + // Too many favorites, can not put them into one query anymore... + return ['`app` = ?', ['files']]; + } + + $rootFolder = \OC::$server->getUserFolder($user); + $parameters = $fileQueryList = []; + foreach ($favorites as $favorite) { + $nodes = $rootFolder->getById($favorite); + if ($nodes) { + /** @var \OCP\Files\Node $node */ + $node = array_shift($nodes); + + $fileQueryList[] = '`file` = ?'; + $parameters[] = substr($node->getPath(), strlen($user . '/files/')); + + if ($node instanceof \OCP\Files\Folder) { + // Also look for subfolders and files + $fileQueryList[] = '`file` LIKE ?'; + $parameters[] = substr($node->getPath(), strlen($user . '/files/')) . '/%'; + } + } + } + + if (empty($fileQueryList)) { + // No favorites... + return ['`app` = ?', ['files']]; + } + + return [ + ' CASE WHEN `app` = \'files\' THEN (' . implode(' OR ', $fileQueryList) . ') ELSE `app` <> \'files\' END ', + $parameters, + ]; + } return false; } + + /** + * Is the file actions favorite limitation enabled? + * + * @param string $user + * @return bool + */ + protected function userSettingFavoritesOnly($user) { + return (bool) \OC::$server->getConfig()->getUserValue($user, 'activity', 'notify_stream_' . self::TYPE_FAVORITES, false); + } } From dd535968e822c21084ecb4f118b3dc4f572bf330 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 25 Mar 2015 17:46:19 +0100 Subject: [PATCH 3/8] Add tests from getCurrentUserId() method and fix the constructor --- tests/lib/activitymanager.php | 110 +++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-) diff --git a/tests/lib/activitymanager.php b/tests/lib/activitymanager.php index d227c05d82..51ab4a7a94 100644 --- a/tests/lib/activitymanager.php +++ b/tests/lib/activitymanager.php @@ -13,10 +13,34 @@ class Test_ActivityManager extends \Test\TestCase { /** @var \OC\ActivityManager */ private $activityManager; + /** @var \PHPUnit_Framework_MockObject_MockObject */ + protected $request; + + /** @var \PHPUnit_Framework_MockObject_MockObject */ + protected $session; + + /** @var \PHPUnit_Framework_MockObject_MockObject */ + protected $config; + protected function setUp() { parent::setUp(); - $this->activityManager = new \OC\ActivityManager(); + $this->request = $this->getMockBuilder('OCP\IRequest') + ->disableOriginalConstructor() + ->getMock(); + $this->session = $this->getMockBuilder('OCP\IUserSession') + ->disableOriginalConstructor() + ->getMock(); + $this->config = $this->getMockBuilder('OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + + $this->activityManager = new \OC\ActivityManager( + $this->request, + $this->session, + $this->config + ); + $this->activityManager->registerExtension(function() { return new NoOpExtension(); }); @@ -111,6 +135,90 @@ class Test_ActivityManager extends \Test\TestCase { $result = $this->activityManager->getQueryForFilter('InvalidFilter'); $this->assertEquals(array(null, null), $result); } + + public function getUserFromTokenThrowInvalidTokenData() { + return [ + [null, []], + ['', []], + ['12345678901234567890123456789', []], + ['1234567890123456789012345678901', []], + ['123456789012345678901234567890', []], + ['123456789012345678901234567890', ['user1', 'user2']], + ]; + } + + /** + * @expectedException \UnexpectedValueException + * @dataProvider getUserFromTokenThrowInvalidTokenData + * + * @param string $token + * @param array $users + */ + public function testGetUserFromTokenThrowInvalidToken($token, $users) { + if ($token !== null) { + $this->request->expects($this->any()) + ->method('getParam') + ->with('token', '') + ->willReturn($token); + } + $this->config->expects($this->any()) + ->method('getUsersForUserValue') + ->with('activity', 'rsstoken', $token) + ->willReturn($users); + + \Test_Helper::invokePrivate($this->activityManager, 'getUserFromToken'); + } + + public function getUserFromTokenData() { + return [ + [null, '123456789012345678901234567890', 'user1'], + ['user2', null, 'user2'], + ['user2', '123456789012345678901234567890', 'user2'], + ]; + } + + /** + * @dataProvider getUserFromTokenData + * + * @param string $userLoggedIn + * @param string $token + * @param string $expected + */ + public function testGetUserFromToken($userLoggedIn, $token, $expected) { + if ($userLoggedIn !== null) { + $this->mockUserSession($userLoggedIn); + } + + if ($token !== null) { + $this->request->expects($this->any()) + ->method('getParam') + ->with('token', '') + ->willReturn($token); + } + + $this->config->expects($this->any()) + ->method('getUsersForUserValue') + ->with('activity', 'rsstoken', '123456789012345678901234567890') + ->willReturn(['user1']); + + $this->assertEquals($expected, $this->activityManager->getCurrentUserId()); + } + + protected function mockUserSession($user) { + $mockUser = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + $mockUser->expects($this->any()) + ->method('getUID') + ->willReturn($user); + + $this->session->expects($this->any()) + ->method('isLoggedIn') + ->willReturn(true); + $this->session->expects($this->any()) + ->method('getUser') + ->willReturn($mockUser); + } } class SimpleExtension implements \OCP\Activity\IExtension { From e365ea7ec55ca50cf133cc6bf64adb475a6303bf Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 25 Mar 2015 18:21:30 +0100 Subject: [PATCH 4/8] Use DI for the objects where possible --- apps/files/appinfo/app.php | 5 ++++- apps/files/lib/activity.php | 29 +++++++++++++++++++++++------ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/apps/files/appinfo/app.php b/apps/files/appinfo/app.php index e4cac7c7c1..578c1a52e8 100644 --- a/apps/files/appinfo/app.php +++ b/apps/files/appinfo/app.php @@ -59,6 +59,9 @@ $templateManager->registerTemplate('application/vnd.oasis.opendocument.spreadshe \OC::$server->getActivityManager()->registerExtension(function() { return new \OCA\Files\Activity( \OC::$server->query('L10NFactory'), - \OC::$server->getURLGenerator() + \OC::$server->getURLGenerator(), + \OC::$server->getActivityManager(), + \OC::$server->getTagManager(), + \OC::$server->getConfig() ); }); diff --git a/apps/files/lib/activity.php b/apps/files/lib/activity.php index c613a07472..559342d979 100644 --- a/apps/files/lib/activity.php +++ b/apps/files/lib/activity.php @@ -24,7 +24,10 @@ namespace OCA\Files; use OC\L10N\Factory; use OCP\Activity\IExtension; +use OCP\Activity\IManager; +use OCP\IConfig; use OCP\IL10N; +use OCP\ITagManager; use OCP\IURLGenerator; class Activity implements IExtension { @@ -46,14 +49,29 @@ class Activity implements IExtension { /** @var IURLGenerator */ protected $URLGenerator; + /** @var \OCP\Activity\IManager */ + protected $activityManager; + + /** @var \OCP\IConfig */ + protected $config; + + /** @var \OCP\ITagManager */ + protected $tagManager; + /** * @param Factory $languageFactory * @param IURLGenerator $URLGenerator + * @param IManager $activityManager + * @param ITagManager $tagManager + * @param IConfig $config */ - public function __construct(Factory $languageFactory, IURLGenerator $URLGenerator) { + public function __construct(Factory $languageFactory, IURLGenerator $URLGenerator, IManager $activityManager, ITagManager $tagManager, IConfig $config) { $this->languageFactory = $languageFactory; $this->URLGenerator = $URLGenerator; $this->l = $this->getL10N(); + $this->activityManager = $activityManager; + $this->tagManager = $tagManager; + $this->config = $config; } /** @@ -288,7 +306,7 @@ class Activity implements IExtension { * @return array|false */ public function getQueryForFilter($filter) { - $user = \OC::$server->getActivityManager()->getCurrentUserId(); + $user = $this->activityManager->getCurrentUserId(); // Display actions from all files if ($filter === self::FILTER_FILES) { return ['`app` = ?', ['files']]; @@ -301,9 +319,7 @@ class Activity implements IExtension { // Display actions from favorites only if ($filter === self::FILTER_FAVORITES || $filter === 'all' && $this->userSettingFavoritesOnly($user)) { - $tagManager = \OC::$server->getTagManager(); - - $tags = $tagManager->load('files', [], false, $user); + $tags = $this->tagManager->load('files', [], false, $user); $favorites = $tags->getFavorites(); if (isset($favorites[50])) { @@ -311,6 +327,7 @@ class Activity implements IExtension { return ['`app` = ?', ['files']]; } + // Can not DI because the user is not known on instantiation $rootFolder = \OC::$server->getUserFolder($user); $parameters = $fileQueryList = []; foreach ($favorites as $favorite) { @@ -350,6 +367,6 @@ class Activity implements IExtension { * @return bool */ protected function userSettingFavoritesOnly($user) { - return (bool) \OC::$server->getConfig()->getUserValue($user, 'activity', 'notify_stream_' . self::TYPE_FAVORITES, false); + return (bool) $this->config->getUserValue($user, 'activity', 'notify_stream_' . self::TYPE_FAVORITES, false); } } From 9233d32834d07f8bb55d8efb3436d70e66014cb5 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 30 Mar 2015 17:21:06 +0200 Subject: [PATCH 5/8] Move tag related code into a helper so we can test the query without a view --- apps/files/appinfo/app.php | 4 +- apps/files/lib/activity.php | 63 +++---- apps/files/lib/activityhelper.php | 84 +++++++++ apps/files/tests/activitytest.php | 297 ++++++++++++++++++++++++++++++ 4 files changed, 414 insertions(+), 34 deletions(-) create mode 100644 apps/files/lib/activityhelper.php create mode 100644 apps/files/tests/activitytest.php diff --git a/apps/files/appinfo/app.php b/apps/files/appinfo/app.php index 578c1a52e8..c483ad31ec 100644 --- a/apps/files/appinfo/app.php +++ b/apps/files/appinfo/app.php @@ -61,7 +61,9 @@ $templateManager->registerTemplate('application/vnd.oasis.opendocument.spreadshe \OC::$server->query('L10NFactory'), \OC::$server->getURLGenerator(), \OC::$server->getActivityManager(), - \OC::$server->getTagManager(), + new \OCA\Files\ActivityHelper( + \OC::$server->getTagManager() + ), \OC::$server->getConfig() ); }); diff --git a/apps/files/lib/activity.php b/apps/files/lib/activity.php index 559342d979..7302bfb33b 100644 --- a/apps/files/lib/activity.php +++ b/apps/files/lib/activity.php @@ -27,7 +27,6 @@ use OCP\Activity\IExtension; use OCP\Activity\IManager; use OCP\IConfig; use OCP\IL10N; -use OCP\ITagManager; use OCP\IURLGenerator; class Activity implements IExtension { @@ -55,22 +54,22 @@ class Activity implements IExtension { /** @var \OCP\IConfig */ protected $config; - /** @var \OCP\ITagManager */ - protected $tagManager; + /** @var \OCA\Files\ActivityHelper */ + protected $helper; /** * @param Factory $languageFactory * @param IURLGenerator $URLGenerator * @param IManager $activityManager - * @param ITagManager $tagManager + * @param ActivityHelper $helper * @param IConfig $config */ - public function __construct(Factory $languageFactory, IURLGenerator $URLGenerator, IManager $activityManager, ITagManager $tagManager, IConfig $config) { + public function __construct(Factory $languageFactory, IURLGenerator $URLGenerator, IManager $activityManager, ActivityHelper $helper, IConfig $config) { $this->languageFactory = $languageFactory; $this->URLGenerator = $URLGenerator; $this->l = $this->getL10N(); $this->activityManager = $activityManager; - $this->tagManager = $tagManager; + $this->helper = $helper; $this->config = $config; } @@ -94,7 +93,7 @@ class Activity implements IExtension { return [ self::TYPE_SHARE_CREATED => (string) $l->t('A new file or folder has been created'), self::TYPE_SHARE_CHANGED => (string) $l->t('A file or folder has been changed'), - self::TYPE_FAVORITES => (string) $l->t('Limit notifications about creation and changes to your favorite files'), + self::TYPE_FAVORITES => (string) $l->t('Limit notifications about creation and changes to your favorite files (Stream only)'), self::TYPE_SHARE_DELETED => (string) $l->t('A file or folder has been deleted'), self::TYPE_SHARE_RESTORED => (string) $l->t('A file or folder has been restored'), ]; @@ -250,19 +249,20 @@ class Activity implements IExtension { */ public function getNavigation() { return [ - 'apps' => [ + 'top' => [ self::FILTER_FAVORITES => [ 'id' => self::FILTER_FAVORITES, 'name' => (string) $this->l->t('Favorites'), 'url' => $this->URLGenerator->linkToRoute('activity.Activities.showList', ['filter' => self::FILTER_FAVORITES]), ], + ], + 'apps' => [ self::FILTER_FILES => [ 'id' => self::FILTER_FILES, 'name' => (string) $this->l->t('Files'), 'url' => $this->URLGenerator->linkToRoute('activity.Activities.showList', ['filter' => self::FILTER_FILES]), ], ], - 'top' => [], ]; } @@ -319,41 +319,38 @@ class Activity implements IExtension { // Display actions from favorites only if ($filter === self::FILTER_FAVORITES || $filter === 'all' && $this->userSettingFavoritesOnly($user)) { - $tags = $this->tagManager->load('files', [], false, $user); - $favorites = $tags->getFavorites(); - - if (isset($favorites[50])) { + try { + $favorites = $this->helper->getFavoriteFilePaths($user); + } catch (\RuntimeException $e) { // Too many favorites, can not put them into one query anymore... return ['`app` = ?', ['files']]; } - // Can not DI because the user is not known on instantiation - $rootFolder = \OC::$server->getUserFolder($user); + /* + * Display activities only, when they are not `type` create/change + * or `file` is a favorite or in a favorite folder + */ $parameters = $fileQueryList = []; - foreach ($favorites as $favorite) { - $nodes = $rootFolder->getById($favorite); - if ($nodes) { - /** @var \OCP\Files\Node $node */ - $node = array_shift($nodes); + $parameters[] = 'files'; - $fileQueryList[] = '`file` = ?'; - $parameters[] = substr($node->getPath(), strlen($user . '/files/')); + $fileQueryList[] = '`type` <> ?'; + $parameters[] = self::TYPE_SHARE_CREATED; + $fileQueryList[] = '`type` <> ?'; + $parameters[] = self::TYPE_SHARE_CHANGED; - if ($node instanceof \OCP\Files\Folder) { - // Also look for subfolders and files - $fileQueryList[] = '`file` LIKE ?'; - $parameters[] = substr($node->getPath(), strlen($user . '/files/')) . '/%'; - } - } + foreach ($favorites['items'] as $favorite) { + $fileQueryList[] = '`file` = ?'; + $parameters[] = $favorite; + } + foreach ($favorites['folders'] as $favorite) { + $fileQueryList[] = '`file` LIKE ?'; + $parameters[] = $favorite . '/%'; } - if (empty($fileQueryList)) { - // No favorites... - return ['`app` = ?', ['files']]; - } + $parameters[] = 'files'; return [ - ' CASE WHEN `app` = \'files\' THEN (' . implode(' OR ', $fileQueryList) . ') ELSE `app` <> \'files\' END ', + ' CASE WHEN `app` = ? THEN (' . implode(' OR ', $fileQueryList) . ') ELSE `app` <> ? END ', $parameters, ]; } diff --git a/apps/files/lib/activityhelper.php b/apps/files/lib/activityhelper.php new file mode 100644 index 0000000000..e05ae6c983 --- /dev/null +++ b/apps/files/lib/activityhelper.php @@ -0,0 +1,84 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OCA\Files; + +use OCP\Files\Folder; +use OCP\ITagManager; + +class ActivityHelper { + /** If a user has a lot of favorites the query might get too slow and long */ + const FAVORITE_LIMIT = 50; + + /** @var \OCP\ITagManager */ + protected $tagManager; + + /** + * @param ITagManager $tagManager + */ + public function __construct(ITagManager $tagManager) { + $this->tagManager = $tagManager; + } + + /** + * Returns an array with the favorites + * + * @param string $user + * @return array + * @throws \RuntimeException when too many or no favorites where found + */ + public function getFavoriteFilePaths($user) { + $tags = $this->tagManager->load('files', [], false, $user); + $favorites = $tags->getFavorites(); + + if (empty($favorites)) { + throw new \RuntimeException('No favorites', 1); + } else if (isset($favorites[self::FAVORITE_LIMIT])) { + throw new \RuntimeException('Too many favorites', 2); + } + + // Can not DI because the user is not known on instantiation + $rootFolder = \OC::$server->getUserFolder($user); + $folders = $items = []; + foreach ($favorites as $favorite) { + $nodes = $rootFolder->getById($favorite); + if ($nodes) { + /** @var \OCP\Files\Node $node */ + $node = array_shift($nodes); + $path = substr($node->getPath(), strlen($user . '/files/')); + + $items[] = $path; + if ($node instanceof Folder) { + $folders[] = $path; + } + } + } + + if (empty($items)) { + throw new \RuntimeException('No favorites', 1); + } + + return [ + 'items' => $items, + 'folders' => $folders, + ]; + } +} diff --git a/apps/files/tests/activitytest.php b/apps/files/tests/activitytest.php new file mode 100644 index 0000000000..24240a0377 --- /dev/null +++ b/apps/files/tests/activitytest.php @@ -0,0 +1,297 @@ + + * This file is licensed under the Affero General Public License version 3 or + * later. + * See the COPYING-README file. + * +*/ + +namespace OCA\Files\Tests; + +use OCA\Files\Activity; +use Test\TestCase; + +class ActivityTest extends TestCase { + + /** @var \OC\ActivityManager */ + private $activityManager; + + /** @var \PHPUnit_Framework_MockObject_MockObject */ + protected $request; + + /** @var \PHPUnit_Framework_MockObject_MockObject */ + protected $session; + + /** @var \PHPUnit_Framework_MockObject_MockObject */ + protected $config; + + /** @var \PHPUnit_Framework_MockObject_MockObject */ + protected $activityHelper; + + /** @var \OCA\Files\Activity */ + protected $activityExtension; + + protected function setUp() { + parent::setUp(); + + $this->request = $this->getMockBuilder('OCP\IRequest') + ->disableOriginalConstructor() + ->getMock(); + $this->session = $this->getMockBuilder('OCP\IUserSession') + ->disableOriginalConstructor() + ->getMock(); + $this->config = $this->getMockBuilder('OCP\IConfig') + ->disableOriginalConstructor() + ->getMock(); + $this->activityHelper = $this->getMockBuilder('OCA\Files\ActivityHelper') + ->disableOriginalConstructor() + ->getMock(); + + $this->activityManager = new \OC\ActivityManager( + $this->request, + $this->session, + $this->config + ); + + $this->activityExtension = $activityExtension = new Activity( + new \OC\L10N\Factory(), + $this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(), + $this->activityManager, + $this->activityHelper, + $this->config + ); + + $this->activityManager->registerExtension(function() use ($activityExtension) { + return $activityExtension; + }); + } + + public function testNotificationTypes() { + $result = $this->activityExtension->getNotificationTypes('en'); + $this->assertTrue(is_array($result), 'Asserting getNotificationTypes() returns an array'); + $this->assertCount(5, $result); + $this->assertArrayHasKey(Activity::TYPE_SHARE_CREATED, $result); + $this->assertArrayHasKey(Activity::TYPE_SHARE_CHANGED, $result); + $this->assertArrayHasKey(Activity::TYPE_FAVORITES, $result); + $this->assertArrayHasKey(Activity::TYPE_SHARE_DELETED, $result); + $this->assertArrayHasKey(Activity::TYPE_SHARE_RESTORED, $result); + } + + public function testDefaultTypes() { + $result = $this->activityExtension->getDefaultTypes('stream'); + $this->assertTrue(is_array($result), 'Asserting getDefaultTypes(stream) returns an array'); + $this->assertCount(4, $result); + $result = array_flip($result); + $this->assertArrayHasKey(Activity::TYPE_SHARE_CREATED, $result); + $this->assertArrayHasKey(Activity::TYPE_SHARE_CHANGED, $result); + $this->assertArrayNotHasKey(Activity::TYPE_FAVORITES, $result); + $this->assertArrayHasKey(Activity::TYPE_SHARE_DELETED, $result); + $this->assertArrayHasKey(Activity::TYPE_SHARE_RESTORED, $result); + + $result = $this->activityExtension->getDefaultTypes('email'); + $this->assertFalse($result, 'Asserting getDefaultTypes(email) returns false'); + } + + public function testTranslate() { + $this->assertFalse( + $this->activityExtension->translate('files_sharing', '', '', array(), false, false, 'en'), + 'Asserting that no translations are set for files_sharing' + ); + } + + public function testGetSpecialParameterList() { + $this->assertFalse( + $this->activityExtension->getSpecialParameterList('files_sharing', ''), + 'Asserting that no special parameters are set for files_sharing' + ); + } + + public function typeIconData() { + return [ + [Activity::TYPE_SHARE_CHANGED, 'icon-change'], + [Activity::TYPE_SHARE_CREATED, 'icon-add-color'], + [Activity::TYPE_SHARE_DELETED, 'icon-delete-color'], + [Activity::TYPE_SHARE_RESTORED, false], + [Activity::TYPE_FAVORITES, false], + ['unknown type', false], + ]; + } + + /** + * @dataProvider typeIconData + * + * @param string $type + * @param mixed $expected + */ + public function testTypeIcon($type, $expected) { + $this->assertSame($expected, $this->activityExtension->getTypeIcon($type)); + } + + public function testGroupParameter() { + $this->assertFalse( + $this->activityExtension->getGroupParameter(['app' => 'files_sharing']), + 'Asserting that no group parameters are set for files_sharing' + ); + } + + public function testNavigation() { + $result = $this->activityExtension->getNavigation(); + $this->assertCount(1, $result['top']); + $this->assertArrayHasKey(Activity::FILTER_FAVORITES, $result['top']); + + $this->assertCount(1, $result['apps']); + $this->assertArrayHasKey(Activity::FILTER_FILES, $result['apps']); + } + + public function testIsFilterValid() { + $this->assertTrue($this->activityExtension->isFilterValid(Activity::FILTER_FAVORITES)); + $this->assertTrue($this->activityExtension->isFilterValid(Activity::FILTER_FILES)); + $this->assertFalse($this->activityExtension->isFilterValid('unknown filter')); + } + + public function filterNotificationTypesData() { + return [ + [ + Activity::FILTER_FILES, + [ + 'NT0', + Activity::TYPE_SHARE_CREATED, + Activity::TYPE_SHARE_CHANGED, + Activity::TYPE_SHARE_DELETED, + Activity::TYPE_SHARE_RESTORED, + Activity::TYPE_FAVORITES, + ], [ + Activity::TYPE_SHARE_CREATED, + Activity::TYPE_SHARE_CHANGED, + Activity::TYPE_SHARE_DELETED, + Activity::TYPE_SHARE_RESTORED, + ], + ], + [ + Activity::FILTER_FILES, + [ + 'NT0', + Activity::TYPE_SHARE_CREATED, + Activity::TYPE_FAVORITES, + ], + [ + Activity::TYPE_SHARE_CREATED, + ], + ], + [ + Activity::FILTER_FAVORITES, + [ + 'NT0', + Activity::TYPE_SHARE_CREATED, + Activity::TYPE_SHARE_CHANGED, + Activity::TYPE_SHARE_DELETED, + Activity::TYPE_SHARE_RESTORED, + Activity::TYPE_FAVORITES, + ], [ + Activity::TYPE_SHARE_CREATED, + Activity::TYPE_SHARE_CHANGED, + Activity::TYPE_SHARE_DELETED, + Activity::TYPE_SHARE_RESTORED, + ], + ], + [ + 'unknown filter', + [ + 'NT0', + Activity::TYPE_SHARE_CREATED, + Activity::TYPE_SHARE_CHANGED, + Activity::TYPE_SHARE_DELETED, + Activity::TYPE_SHARE_RESTORED, + Activity::TYPE_FAVORITES, + ], + false, + ], + ]; + } + + /** + * @dataProvider filterNotificationTypesData + * + * @param string $filter + * @param array $types + * @param mixed $expected + */ + public function testFilterNotificationTypes($filter, $types, $expected) { + $result = $this->activityExtension->filterNotificationTypes($types, $filter); + $this->assertEquals($expected, $result); + } + + public function queryForFilterData() { + return [ + [ + new \RuntimeException(), + '`app` = ?', + ['files'] + ], + [ + [ + 'items' => [], + 'folders' => [], + ], + ' CASE WHEN `app` = ? THEN (`type` <> ? OR `type` <> ?) ELSE `app` <> ? END ', + ['files', Activity::TYPE_SHARE_CREATED, Activity::TYPE_SHARE_CHANGED, 'files'] + ], + [ + [ + 'items' => ['file.txt', 'folder'], + 'folders' => ['folder'], + ], + ' CASE WHEN `app` = ? THEN (`type` <> ? OR `type` <> ? OR `file` = ? OR `file` = ? OR `file` LIKE ?) ELSE `app` <> ? END ', + ['files', Activity::TYPE_SHARE_CREATED, Activity::TYPE_SHARE_CHANGED, 'file.txt', 'folder', 'folder/%', 'files'] + ], + ]; + } + + /** + * @dataProvider queryForFilterData + * + * @param mixed $will + * @param string $query + * @param array $parameters + */ + public function testQueryForFilter($will, $query, $parameters) { + $this->mockUserSession('test'); + + $this->config->expects($this->any()) + ->method('getUserValue') + ->willReturnMap([ + ['test', 'activity', 'notify_stream_' . Activity::TYPE_FAVORITES, false, true], + ]); + if (is_array($will)) { + $this->activityHelper->expects($this->any()) + ->method('getFavoriteFilePaths') + ->with('test') + ->willReturn($will); + } else { + $this->activityHelper->expects($this->any()) + ->method('getFavoriteFilePaths') + ->with('test') + ->willThrowException($will); + } + + $result = $this->activityExtension->getQueryForFilter('all'); + $this->assertEquals([$query, $parameters], $result); + } + + protected function mockUserSession($user) { + $mockUser = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor() + ->getMock(); + $mockUser->expects($this->any()) + ->method('getUID') + ->willReturn($user); + + $this->session->expects($this->any()) + ->method('isLoggedIn') + ->willReturn(true); + $this->session->expects($this->any()) + ->method('getUser') + ->willReturn($mockUser); + } +} From 116b257b4d1e9b986fc2d9a438b5c2cc92e3717f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 31 Mar 2015 17:07:44 +0200 Subject: [PATCH 6/8] DeMorgan applied the wrong transformation --- apps/files/lib/activity.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/files/lib/activity.php b/apps/files/lib/activity.php index 7302bfb33b..007f9a1dda 100644 --- a/apps/files/lib/activity.php +++ b/apps/files/lib/activity.php @@ -333,9 +333,8 @@ class Activity implements IExtension { $parameters = $fileQueryList = []; $parameters[] = 'files'; - $fileQueryList[] = '`type` <> ?'; + $fileQueryList[] = '(`type` <> ? AND `type` <> ?)'; $parameters[] = self::TYPE_SHARE_CREATED; - $fileQueryList[] = '`type` <> ?'; $parameters[] = self::TYPE_SHARE_CHANGED; foreach ($favorites['items'] as $favorite) { From efcc2e87ab860c50f7904ac8e98914589a9c097a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 31 Mar 2015 17:35:04 +0200 Subject: [PATCH 7/8] Adjust by/self filter aswell and fix tests --- apps/files/lib/activity.php | 2 +- apps/files/tests/activitytest.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/files/lib/activity.php b/apps/files/lib/activity.php index 007f9a1dda..fff49ea4ea 100644 --- a/apps/files/lib/activity.php +++ b/apps/files/lib/activity.php @@ -318,7 +318,7 @@ class Activity implements IExtension { } // Display actions from favorites only - if ($filter === self::FILTER_FAVORITES || $filter === 'all' && $this->userSettingFavoritesOnly($user)) { + if ($filter === self::FILTER_FAVORITES || in_array($filter, ['all', 'by', 'self']) && $this->userSettingFavoritesOnly($user)) { try { $favorites = $this->helper->getFavoriteFilePaths($user); } catch (\RuntimeException $e) { diff --git a/apps/files/tests/activitytest.php b/apps/files/tests/activitytest.php index 24240a0377..b8766f9522 100644 --- a/apps/files/tests/activitytest.php +++ b/apps/files/tests/activitytest.php @@ -234,7 +234,7 @@ class ActivityTest extends TestCase { 'items' => [], 'folders' => [], ], - ' CASE WHEN `app` = ? THEN (`type` <> ? OR `type` <> ?) ELSE `app` <> ? END ', + ' CASE WHEN `app` = ? THEN ((`type` <> ? AND `type` <> ?)) ELSE `app` <> ? END ', ['files', Activity::TYPE_SHARE_CREATED, Activity::TYPE_SHARE_CHANGED, 'files'] ], [ @@ -242,7 +242,7 @@ class ActivityTest extends TestCase { 'items' => ['file.txt', 'folder'], 'folders' => ['folder'], ], - ' CASE WHEN `app` = ? THEN (`type` <> ? OR `type` <> ? OR `file` = ? OR `file` = ? OR `file` LIKE ?) ELSE `app` <> ? END ', + ' CASE WHEN `app` = ? THEN ((`type` <> ? AND `type` <> ?) OR `file` = ? OR `file` = ? OR `file` LIKE ?) ELSE `app` <> ? END ', ['files', Activity::TYPE_SHARE_CREATED, Activity::TYPE_SHARE_CHANGED, 'file.txt', 'folder', 'folder/%', 'files'] ], ]; From 730efe25a4c386b2d2e4c1d1b0d16a71be7b9f28 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 1 Apr 2015 12:13:49 +0200 Subject: [PATCH 8/8] Make scrutinizer happy --- apps/files/lib/activityhelper.php | 2 +- apps/files/tests/activitytest.php | 2 +- lib/private/allconfig.php | 2 +- lib/private/server.php | 2 +- lib/public/iconfig.php | 2 +- lib/public/iservercontainer.php | 2 +- tests/lib/activitymanager.php | 27 ++++++++++----------------- 7 files changed, 16 insertions(+), 23 deletions(-) diff --git a/apps/files/lib/activityhelper.php b/apps/files/lib/activityhelper.php index e05ae6c983..f9ff722b1c 100644 --- a/apps/files/lib/activityhelper.php +++ b/apps/files/lib/activityhelper.php @@ -60,7 +60,7 @@ class ActivityHelper { $folders = $items = []; foreach ($favorites as $favorite) { $nodes = $rootFolder->getById($favorite); - if ($nodes) { + if (!empty($nodes)) { /** @var \OCP\Files\Node $node */ $node = array_shift($nodes); $path = substr($node->getPath(), strlen($user . '/files/')); diff --git a/apps/files/tests/activitytest.php b/apps/files/tests/activitytest.php index b8766f9522..1f8d6330e5 100644 --- a/apps/files/tests/activitytest.php +++ b/apps/files/tests/activitytest.php @@ -95,7 +95,7 @@ class ActivityTest extends TestCase { public function testTranslate() { $this->assertFalse( - $this->activityExtension->translate('files_sharing', '', '', array(), false, false, 'en'), + $this->activityExtension->translate('files_sharing', '', [], false, false, 'en'), 'Asserting that no translations are set for files_sharing' ); } diff --git a/lib/private/allconfig.php b/lib/private/allconfig.php index df75a332a1..63cc92601b 100644 --- a/lib/private/allconfig.php +++ b/lib/private/allconfig.php @@ -253,7 +253,7 @@ class AllConfig implements \OCP\IConfig { * @param string $userId the userId of the user that we want to store the value under * @param string $appName the appName that we stored the value under * @param string $key the key under which the value is being stored - * @param string $default the default value to be returned if the value isn't set + * @param mixed $default the default value to be returned if the value isn't set * @return string */ public function getUserValue($userId, $appName, $key, $default = '') { diff --git a/lib/private/server.php b/lib/private/server.php index 6b212ee94a..02d649585d 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -425,7 +425,7 @@ class Server extends SimpleContainer implements IServerContainer { * currently being processed is returned from this method. * In case the current execution was not initiated by a web request null is returned * - * @return \OCP\IRequest|null + * @return \OCP\IRequest */ function getRequest() { return $this->query('Request'); diff --git a/lib/public/iconfig.php b/lib/public/iconfig.php index c63ba1a90a..f28a114a2b 100644 --- a/lib/public/iconfig.php +++ b/lib/public/iconfig.php @@ -133,7 +133,7 @@ interface IConfig { * @param string $userId the userId of the user that we want to store the value under * @param string $appName the appName that we stored the value under * @param string $key the key under which the value is being stored - * @param string $default the default value to be returned if the value isn't set + * @param mixed $default the default value to be returned if the value isn't set * @return string */ public function getUserValue($userId, $appName, $key, $default = ''); diff --git a/lib/public/iservercontainer.php b/lib/public/iservercontainer.php index d7df884adf..f11a5c0133 100644 --- a/lib/public/iservercontainer.php +++ b/lib/public/iservercontainer.php @@ -59,7 +59,7 @@ interface IServerContainer { * is returned from this method. * In case the current execution was not initiated by a web request null is returned * - * @return \OCP\IRequest|null + * @return \OCP\IRequest */ function getRequest(); diff --git a/tests/lib/activitymanager.php b/tests/lib/activitymanager.php index 51ab4a7a94..d3263fa2ed 100644 --- a/tests/lib/activitymanager.php +++ b/tests/lib/activitymanager.php @@ -155,17 +155,7 @@ class Test_ActivityManager extends \Test\TestCase { * @param array $users */ public function testGetUserFromTokenThrowInvalidToken($token, $users) { - if ($token !== null) { - $this->request->expects($this->any()) - ->method('getParam') - ->with('token', '') - ->willReturn($token); - } - $this->config->expects($this->any()) - ->method('getUsersForUserValue') - ->with('activity', 'rsstoken', $token) - ->willReturn($users); - + $this->mockRSSToken($token, $token, $users); \Test_Helper::invokePrivate($this->activityManager, 'getUserFromToken'); } @@ -188,20 +178,23 @@ class Test_ActivityManager extends \Test\TestCase { if ($userLoggedIn !== null) { $this->mockUserSession($userLoggedIn); } + $this->mockRSSToken($token, '123456789012345678901234567890', ['user1']); - if ($token !== null) { + $this->assertEquals($expected, $this->activityManager->getCurrentUserId()); + } + + protected function mockRSSToken($requestToken, $userToken, $users) { + if ($requestToken !== null) { $this->request->expects($this->any()) ->method('getParam') ->with('token', '') - ->willReturn($token); + ->willReturn($requestToken); } $this->config->expects($this->any()) ->method('getUsersForUserValue') - ->with('activity', 'rsstoken', '123456789012345678901234567890') - ->willReturn(['user1']); - - $this->assertEquals($expected, $this->activityManager->getCurrentUserId()); + ->with('activity', 'rsstoken', $userToken) + ->willReturn($users); } protected function mockUserSession($user) {