From 361f008c705009eee8d7435f46095760ac706456 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 12 Sep 2016 17:09:46 +0200 Subject: [PATCH] Make it possible to filter by tags with REPORT method Enhanced the REPORT method on the Webdav endpoint and added a "oc:favorite" filter rule. When set, it will return a flat list of results filtered with only favorite files. The web UI was also adjusted to use this REPORT method instead of the private API endpoint. Signed-off-by: Roeland Jago Douma --- .../lib/Connector/Sabre/FilesReportPlugin.php | 42 +++++ .../dav/lib/Connector/Sabre/ServerFactory.php | 1 + .../dav/lib/Connector/Sabre/ShareTypeList.php | 6 +- apps/dav/lib/Connector/Sabre/TagList.php | 6 +- .../Connector/Sabre/FilesReportPluginTest.php | 39 +++- apps/files/appinfo/routes.php | 6 - apps/files/js/favoritesfilelist.js | 27 +-- apps/files/lib/Controller/ApiController.php | 17 -- apps/files/lib/Service/TagService.php | 21 --- .../tests/Controller/ApiControllerTest.php | 176 ------------------ apps/files/tests/js/favoritesfilelistspec.js | 53 +++--- core/js/files/client.js | 6 +- core/js/tests/specs/files/clientSpec.js | 4 +- 13 files changed, 124 insertions(+), 280 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 5b96f4f0f6..4df5f8b80e 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -39,6 +39,7 @@ use OCP\Files\Folder; use OCP\IGroupManager; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\TagNotFoundException; +use OCP\ITagManager; class FilesReportPlugin extends ServerPlugin { @@ -74,6 +75,13 @@ class FilesReportPlugin extends ServerPlugin { */ private $tagMapper; + /** + * Manager for private tags + * + * @var ITagManager + */ + private $fileTagger; + /** * @var IUserSession */ @@ -92,11 +100,18 @@ class FilesReportPlugin extends ServerPlugin { /** * @param Tree $tree * @param View $view + * @param ISystemTagManager $tagManager + * @param ISystemTagObjectMapper $tagMapper + * @param ITagManager $fileTagger manager for private tags + * @param IUserSession $userSession + * @param IGroupManager $groupManager + * @param Folder $userfolder */ public function __construct(Tree $tree, View $view, ISystemTagManager $tagManager, ISystemTagObjectMapper $tagMapper, + ITagManager $fileTagger, IUserSession $userSession, IGroupManager $groupManager, Folder $userFolder @@ -105,6 +120,7 @@ class FilesReportPlugin extends ServerPlugin { $this->fileView = $view; $this->tagManager = $tagManager; $this->tagMapper = $tagMapper; + $this->fileTagger = $fileTagger; $this->userSession = $userSession; $this->groupManager = $groupManager; $this->userFolder = $userFolder; @@ -215,12 +231,38 @@ class FilesReportPlugin extends ServerPlugin { $ns = '{' . $this::NS_OWNCLOUD . '}'; $resultFileIds = null; $systemTagIds = []; + $favoriteFilter = null; foreach ($filterRules as $filterRule) { if ($filterRule['name'] === $ns . 'systemtag') { $systemTagIds[] = $filterRule['value']; } + if ($filterRule['name'] === $ns . 'favorite') { + $favoriteFilter = true; + } } + if ($favoriteFilter !== null) { + $resultFileIds = $this->fileTagger->load('files')->getFavorites(); + if (empty($resultFileIds)) { + return []; + } + } + + if (!empty($systemTagIds)) { + $fileIds = $this->getSystemTagFileIds($systemTagIds); + if (empty($resultFileIds)) { + $resultFileIds = $fileIds; + } else { + $resultFileIds = array_intersect($fileIds, $resultFileIds); + } + } + + return $resultFileIds; + } + + private function getSystemTagFileIds($systemTagIds) { + $resultFileIds = null; + // check user permissions, if applicable if (!$this->isAdmin()) { // check visibility/permission diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index 644f0f28f5..6d9f9b1bc8 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -177,6 +177,7 @@ class ServerFactory { $view, \OC::$server->getSystemTagManager(), \OC::$server->getSystemTagObjectMapper(), + \OC::$server->getTagManager(), $this->userSession, \OC::$server->getGroupManager(), $userFolder diff --git a/apps/dav/lib/Connector/Sabre/ShareTypeList.php b/apps/dav/lib/Connector/Sabre/ShareTypeList.php index fd9ba3f726..4d9179b665 100644 --- a/apps/dav/lib/Connector/Sabre/ShareTypeList.php +++ b/apps/dav/lib/Connector/Sabre/ShareTypeList.php @@ -66,7 +66,11 @@ class ShareTypeList implements Element { static function xmlDeserialize(Reader $reader) { $shareTypes = []; - foreach ($reader->parseInnerTree() as $elem) { + $tree = $reader->parseInnerTree(); + if ($tree === null) { + return null; + } + foreach ($tree as $elem) { if ($elem['name'] === '{' . self::NS_OWNCLOUD . '}share-type') { $shareTypes[] = (int)$elem['value']; } diff --git a/apps/dav/lib/Connector/Sabre/TagList.php b/apps/dav/lib/Connector/Sabre/TagList.php index 2357617126..2f514112a4 100644 --- a/apps/dav/lib/Connector/Sabre/TagList.php +++ b/apps/dav/lib/Connector/Sabre/TagList.php @@ -85,7 +85,11 @@ class TagList implements Element { static function xmlDeserialize(Reader $reader) { $tags = []; - foreach ($reader->parseInnerTree() as $elem) { + $tree = $reader->parseInnerTree(); + if ($tree === null) { + return null; + } + foreach ($tree as $elem) { if ($elem['name'] === '{' . self::NS_OWNCLOUD . '}tag') { $tags[] = $elem['value']; } diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index 336a33058b..2097777c8f 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -26,12 +26,15 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre; use OCA\DAV\Connector\Sabre\FilesReportPlugin as FilesReportPluginImplementation; use OCP\IPreview; +use OCP\ITagManager; +use OCP\IUserSession; use Sabre\DAV\Exception\NotFound; use OCP\SystemTag\ISystemTagObjectMapper; use OC\Files\View; use OCP\Files\Folder; use OCP\IGroupManager; use OCP\SystemTag\ISystemTagManager; +use OCP\ITags; class FilesReportPluginTest extends \Test\TestCase { /** @var \Sabre\DAV\Server|\PHPUnit_Framework_MockObject_MockObject */ @@ -46,6 +49,9 @@ class FilesReportPluginTest extends \Test\TestCase { /** @var ISystemTagManager|\PHPUnit_Framework_MockObject_MockObject */ private $tagManager; + /** @var ITags|\PHPUnit_Framework_MockObject_MockObject */ + private $privateTags; + /** @var \OCP\IUserSession */ private $userSession; @@ -87,20 +93,20 @@ class FilesReportPluginTest extends \Test\TestCase { ->disableOriginalConstructor() ->getMock(); - $this->tagManager = $this->getMockBuilder('\OCP\SystemTag\ISystemTagManager') - ->disableOriginalConstructor() - ->getMock(); - $this->tagMapper = $this->getMockBuilder('\OCP\SystemTag\ISystemTagObjectMapper') - ->disableOriginalConstructor() - ->getMock(); - $this->userSession = $this->getMockBuilder('\OCP\IUserSession') - ->disableOriginalConstructor() - ->getMock(); - $this->previewManager = $this->getMockBuilder('\OCP\IPreview') ->disableOriginalConstructor() ->getMock(); + $this->tagManager = $this->createMock(ISystemTagManager::class); + $this->tagMapper = $this->createMock(ISystemTagObjectMapper::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->privateTags = $this->createMock(ITags::class); + $privateTagManager = $this->createMock(ITagManager::class); + $privateTagManager->expects($this->any()) + ->method('load') + ->with('files') + ->will($this->returnValue($this->privateTags)); + $user = $this->getMockBuilder('\OCP\IUser') ->disableOriginalConstructor() ->getMock(); @@ -116,6 +122,7 @@ class FilesReportPluginTest extends \Test\TestCase { $this->view, $this->tagManager, $this->tagMapper, + $privateTagManager, $this->userSession, $this->groupManager, $this->userFolder @@ -652,4 +659,16 @@ class FilesReportPluginTest extends \Test\TestCase { $this->assertEquals(['222'], array_values($this->invokePrivate($this->plugin, 'processFilterRules', [$rules]))); } + + public function testProcessFavoriteFilter() { + $rules = [ + ['name' => '{http://owncloud.org/ns}favorite', 'value' => '1'], + ]; + + $this->privateTags->expects($this->once()) + ->method('getFavorites') + ->will($this->returnValue(['456', '789'])); + + $this->assertEquals(['456', '789'], array_values($this->invokePrivate($this->plugin, 'processFilterRules', [$rules]))); + } } diff --git a/apps/files/appinfo/routes.php b/apps/files/appinfo/routes.php index 6237e8413e..49dbe55343 100644 --- a/apps/files/appinfo/routes.php +++ b/apps/files/appinfo/routes.php @@ -44,12 +44,6 @@ $application->registerRoutes( 'verb' => 'POST', 'requirements' => array('path' => '.+'), ), - array( - 'name' => 'API#getFilesByTag', - 'url' => '/api/v1/tags/{tagName}/files', - 'verb' => 'GET', - 'requirements' => array('tagName' => '.+'), - ), array( 'name' => 'API#getRecentFiles', 'url' => '/api/v1/recent/', diff --git a/apps/files/js/favoritesfilelist.js b/apps/files/js/favoritesfilelist.js index e6532ab188..380689be10 100644 --- a/apps/files/js/favoritesfilelist.js +++ b/apps/files/js/favoritesfilelist.js @@ -75,24 +75,25 @@ $(document).ready(function() { // there is only root this._setCurrentDir('/', false); - this._reloadCall = $.ajax({ - url: OC.generateUrl('/apps/files/api/v1/tags/{tagName}/files', {tagName: tagName}), - type: 'GET', - dataType: 'json' - }); + this._reloadCall = this.filesClient.getFilteredFiles( + { + favorite: true + }, + { + properties: this._getWebdavProperties() + } + ); var callBack = this.reloadCallback.bind(this); return this._reloadCall.then(callBack, callBack); }, - reloadCallback: function(result) { - delete this._reloadCall; - this.hideMask(); - - if (result.files) { - this.setFiles(result.files.sort(this._sortComparator)); - return true; + reloadCallback: function(status, result) { + if (result) { + // prepend empty dir info because original handler + result.unshift({}); } - return false; + + return OCA.Files.FileList.prototype.reloadCallback.call(this, status, result); } }); diff --git a/apps/files/lib/Controller/ApiController.php b/apps/files/lib/Controller/ApiController.php index 7ce83bfca1..d6f88581b9 100644 --- a/apps/files/lib/Controller/ApiController.php +++ b/apps/files/lib/Controller/ApiController.php @@ -173,23 +173,6 @@ class ApiController extends Controller { }, $nodes)); } - /** - * Returns a list of all files tagged with the given tag. - * - * @NoAdminRequired - * - * @param string $tagName tag name to filter by - * @return DataResponse - */ - public function getFilesByTag($tagName) { - $nodes = $this->tagService->getFilesByTag($tagName); - $files = $this->formatNodes($nodes); - foreach ($files as &$file) { - $file['tags'] = [$tagName]; - } - return new DataResponse(['files' => $files]); - } - /** * Returns a list of recently modifed files. * diff --git a/apps/files/lib/Service/TagService.php b/apps/files/lib/Service/TagService.php index 04e962b5e1..4482fb4537 100644 --- a/apps/files/lib/Service/TagService.php +++ b/apps/files/lib/Service/TagService.php @@ -90,26 +90,5 @@ class TagService { // list is up to date, in case of concurrent changes ? return $tags; } - - /** - * Get all files for the given tag - * - * @param string $tagName tag name to filter by - * @return Node[] list of matching files - * @throws \Exception if the tag does not exist - */ - public function getFilesByTag($tagName) { - try { - $fileIds = $this->tagger->getIdsForTag($tagName); - } catch (\Exception $e) { - return []; - } - - $allNodes = []; - foreach ($fileIds as $fileId) { - $allNodes = array_merge($allNodes, $this->homeFolder->getById((int) $fileId)); - } - return $allNodes; - } } diff --git a/apps/files/tests/Controller/ApiControllerTest.php b/apps/files/tests/Controller/ApiControllerTest.php index 9bfc6d6f5e..4b7bec065a 100644 --- a/apps/files/tests/Controller/ApiControllerTest.php +++ b/apps/files/tests/Controller/ApiControllerTest.php @@ -103,182 +103,6 @@ class ApiControllerTest extends TestCase { ); } - public function testGetFilesByTagEmpty() { - $tagName = 'MyTagName'; - $this->tagService->expects($this->once()) - ->method('getFilesByTag') - ->with($this->equalTo([$tagName])) - ->will($this->returnValue([])); - - $expected = new DataResponse(['files' => []]); - $this->assertEquals($expected, $this->apiController->getFilesByTag([$tagName])); - } - - public function testGetFilesByTagSingle() { - $tagName = 'MyTagName'; - $fileInfo = new FileInfo( - '/root.txt', - $this->getMockBuilder('\OC\Files\Storage\Storage') - ->disableOriginalConstructor() - ->getMock(), - '/var/www/root.txt', - [ - 'mtime' => 55, - 'mimetype' => 'application/pdf', - 'permissions' => 31, - 'size' => 1234, - 'etag' => 'MyEtag', - ], - $this->getMockBuilder('\OCP\Files\Mount\IMountPoint') - ->disableOriginalConstructor() - ->getMock() - ); - $node = $this->getMockBuilder('\OC\Files\Node\File') - ->disableOriginalConstructor() - ->getMock(); - $node->expects($this->once()) - ->method('getFileInfo') - ->will($this->returnValue($fileInfo)); - $this->tagService->expects($this->once()) - ->method('getFilesByTag') - ->with($this->equalTo([$tagName])) - ->will($this->returnValue([$node])); - - $this->shareManager->expects($this->any()) - ->method('getSharesBy') - ->with( - $this->equalTo('user1'), - $this->anything(), - $node, - $this->equalTo(false), - $this->equalTo(1) - ) - ->will($this->returnCallback(function($userId, $shareType) { - if ($shareType === \OCP\Share::SHARE_TYPE_USER || $shareType === \OCP\Share::SHARE_TYPE_LINK) { - return ['dummy_share']; - } - return []; - })); - - $expected = new DataResponse([ - 'files' => [ - [ - 'id' => null, - 'parentId' => null, - 'mtime' => 55000, - 'name' => 'root.txt', - 'permissions' => 31, - 'mimetype' => 'application/pdf', - 'size' => 1234, - 'type' => 'file', - 'etag' => 'MyEtag', - 'path' => '/', - 'tags' => [ - [ - 'MyTagName' - ] - ], - 'shareTypes' => [\OCP\Share::SHARE_TYPE_USER, \OCP\Share::SHARE_TYPE_LINK] - ], - ], - ]); - $this->assertEquals($expected, $this->apiController->getFilesByTag([$tagName])); - } - - public function testGetFilesByTagMultiple() { - $tagName = 'MyTagName'; - $fileInfo1 = new FileInfo( - '/root.txt', - $this->getMockBuilder('\OC\Files\Storage\Storage') - ->disableOriginalConstructor() - ->getMock(), - '/var/www/root.txt', - [ - 'mtime' => 55, - 'mimetype' => 'application/pdf', - 'permissions' => 31, - 'size' => 1234, - 'etag' => 'MyEtag', - ], - $this->getMockBuilder('\OCP\Files\Mount\IMountPoint') - ->disableOriginalConstructor() - ->getMock() - ); - $fileInfo2 = new FileInfo( - '/root.txt', - $this->getMockBuilder('\OC\Files\Storage\Storage') - ->disableOriginalConstructor() - ->getMock(), - '/var/www/some/sub.txt', - [ - 'mtime' => 999, - 'mimetype' => 'application/binary', - 'permissions' => 31, - 'size' => 9876, - 'etag' => 'SubEtag', - ], - $this->getMockBuilder('\OCP\Files\Mount\IMountPoint') - ->disableOriginalConstructor() - ->getMock() - ); - $node1 = $this->getMockBuilder('\OC\Files\Node\File') - ->disableOriginalConstructor() - ->getMock(); - $node1->expects($this->once()) - ->method('getFileInfo') - ->will($this->returnValue($fileInfo1)); - $node2 = $this->getMockBuilder('\OC\Files\Node\File') - ->disableOriginalConstructor() - ->getMock(); - $node2->expects($this->once()) - ->method('getFileInfo') - ->will($this->returnValue($fileInfo2)); - $this->tagService->expects($this->once()) - ->method('getFilesByTag') - ->with($this->equalTo([$tagName])) - ->will($this->returnValue([$node1, $node2])); - - $expected = new DataResponse([ - 'files' => [ - [ - 'id' => null, - 'parentId' => null, - 'mtime' => 55000, - 'name' => 'root.txt', - 'permissions' => 31, - 'mimetype' => 'application/pdf', - 'size' => 1234, - 'type' => 'file', - 'etag' => 'MyEtag', - 'path' => '/', - 'tags' => [ - [ - 'MyTagName' - ] - ], - ], - [ - 'id' => null, - 'parentId' => null, - 'mtime' => 999000, - 'name' => 'root.txt', - 'permissions' => 31, - 'mimetype' => 'application/binary', - 'size' => 9876, - 'type' => 'file', - 'etag' => 'SubEtag', - 'path' => '/', - 'tags' => [ - [ - 'MyTagName' - ] - ], - ] - ], - ]); - $this->assertEquals($expected, $this->apiController->getFilesByTag([$tagName])); - } - public function testUpdateFileTagsEmpty() { $expected = new DataResponse([]); $this->assertEquals($expected, $this->apiController->updateFileTags('/path.txt')); diff --git a/apps/files/tests/js/favoritesfilelistspec.js b/apps/files/tests/js/favoritesfilelistspec.js index 1c833d334e..db890927ed 100644 --- a/apps/files/tests/js/favoritesfilelistspec.js +++ b/apps/files/tests/js/favoritesfilelistspec.js @@ -41,13 +41,9 @@ describe('OCA.Files.FavoritesFileList tests', function() { '' ); }); - afterEach(function() { - fileList.destroy(); - fileList = undefined; - }); describe('loading file list', function() { - var response; + var fetchStub; beforeEach(function() { fileList = new OCA.Files.FavoritesFileList( @@ -55,36 +51,31 @@ describe('OCA.Files.FavoritesFileList tests', function() { ); OCA.Files.FavoritesPlugin.attach(fileList); - fileList.reload(); - - /* jshint camelcase: false */ - response = { - files: [{ - id: 7, - name: 'test.txt', - path: '/somedir', - size: 123, - mtime: 11111000, - tags: [OC.TAG_FAVORITE], - permissions: OC.PERMISSION_ALL, - mimetype: 'text/plain' - }] - }; + fetchStub = sinon.stub(fileList.filesClient, 'getFilteredFiles'); + }); + afterEach(function() { + fetchStub.restore(); + fileList.destroy(); + fileList = undefined; }); it('render files', function() { - var request; + var deferred = $.Deferred(); + fetchStub.returns(deferred.promise()); - expect(fakeServer.requests.length).toEqual(1); - request = fakeServer.requests[0]; - expect(request.url).toEqual( - OC.generateUrl('apps/files/api/v1/tags/{tagName}/files', {tagName: OC.TAG_FAVORITE}) - ); + fileList.reload(); - fakeServer.requests[0].respond( - 200, - { 'Content-Type': 'application/json' }, - JSON.stringify(response) - ); + expect(fetchStub.calledOnce).toEqual(true); + + deferred.resolve(207, [{ + id: 7, + name: 'test.txt', + path: '/somedir', + size: 123, + mtime: 11111000, + tags: [OC.TAG_FAVORITE], + permissions: OC.PERMISSION_ALL, + mimetype: 'text/plain' + }]); var $rows = fileList.$el.find('tbody tr'); var $tr = $rows.eq(0); diff --git a/core/js/files/client.js b/core/js/files/client.js index a195258afb..572f7879e1 100644 --- a/core/js/files/client.js +++ b/core/js/files/client.js @@ -437,6 +437,7 @@ * * @param {Object} filter filter criteria * @param {Object} [filter.systemTagIds] list of system tag ids to filter by + * @param {bool} [filter.favorite] set it to filter by favorites * @param {Object} [options] options * @param {Array} [options.properties] list of Webdav properties to retrieve * @@ -454,7 +455,7 @@ properties = options.properties; } - if (!filter || !filter.systemTagIds || !filter.systemTagIds.length) { + if (!filter || (!filter.systemTagIds && _.isUndefined(filter.favorite))) { throw 'Missing filter argument'; } @@ -480,6 +481,9 @@ _.each(filter.systemTagIds, function(systemTagIds) { body += ' ' + escapeHTML(systemTagIds) + '\n'; }); + if (filter.favorite) { + body += ' ' + (filter.favorite ? '1': '0') + '\n'; + } body += ' \n'; // end of root diff --git a/core/js/tests/specs/files/clientSpec.js b/core/js/tests/specs/files/clientSpec.js index 7673ec6e0f..f75998029a 100644 --- a/core/js/tests/specs/files/clientSpec.js +++ b/core/js/tests/specs/files/clientSpec.js @@ -461,9 +461,7 @@ describe('OC.Files.Client tests', function() { it('throws exception if arguments are missing', function() { var thrown = null; try { - client.getFilteredFiles({ - systemTagIds: [] - }); + client.getFilteredFiles({}); } catch (e) { thrown = true; }