From 77db6d3dbd621453e368637d2146ed5ac96066cf Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 7 Jun 2016 13:51:16 +0200 Subject: [PATCH 1/2] Improve the UX for "not found" perma links --- apps/files/js/app.js | 3 + apps/files/lib/Controller/ViewController.php | 65 ++++++++++--------- apps/files/templates/index.php | 1 + .../tests/Controller/ViewControllerTest.php | 13 ++-- 4 files changed, 46 insertions(+), 36 deletions(-) diff --git a/apps/files/js/app.js b/apps/files/js/app.js index 7a3d78f966..fbfa510e07 100644 --- a/apps/files/js/app.js +++ b/apps/files/js/app.js @@ -53,6 +53,9 @@ this.$showHiddenFiles = $('input#showhiddenfilesToggle'); var showHidden = $('#showHiddenFiles').val() === "1"; this.$showHiddenFiles.prop('checked', showHidden); + if ($('#fileNotFound').val() === "1") { + OC.Notification.showTemporary(t('files', 'File could not be found')); + } this._filesConfig = new OC.Backbone.Model({ showhidden: showHidden diff --git a/apps/files/lib/Controller/ViewController.php b/apps/files/lib/Controller/ViewController.php index 1b0903b41d..18b6cf719c 100644 --- a/apps/files/lib/Controller/ViewController.php +++ b/apps/files/lib/Controller/ViewController.php @@ -27,9 +27,9 @@ namespace OCA\Files\Controller; use OC\AppFramework\Http\Request; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\ContentSecurityPolicy; -use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; +use OCP\Files\NotFoundException; use OCP\IConfig; use OCP\IL10N; use OCP\INavigationManager; @@ -37,7 +37,6 @@ use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUserSession; use Symfony\Component\EventDispatcher\EventDispatcherInterface; -use OCP\AppFramework\Http\NotFoundResponse; use OCP\Files\Folder; use OCP\App\IAppManager; @@ -142,11 +141,15 @@ class ViewController extends Controller { * @param string $view * @param string $fileid * @return TemplateResponse - * @throws \OCP\Files\NotFoundException */ public function index($dir = '', $view = '', $fileid = null) { + $fileNotFound = false; if ($fileid !== null) { - return $this->showFile($fileid); + try { + return $this->showFile($fileid); + } catch (NotFoundException $e) { + $fileNotFound = true; + } } $nav = new \OCP\Template('files', 'appnavigation', ''); @@ -245,6 +248,7 @@ class ViewController extends Controller { $params['defaultFileSortingDirection'] = $this->config->getUserValue($user, 'files', 'file_sorting_direction', 'asc'); $showHidden = (bool) $this->config->getUserValue($this->userSession->getUser()->getUID(), 'files', 'show_hidden', false); $params['showHiddenFiles'] = $showHidden ? 1 : 0; + $params['fileNotFound'] = $fileNotFound ? 1 : 0; $params['appNavigation'] = $nav; $params['appContents'] = $contentItems; $this->navigationManager->setActiveEntry('files_index'); @@ -265,40 +269,37 @@ class ViewController extends Controller { * Redirects to the file list and highlight the given file id * * @param string $fileId file id to show - * @return Response redirect response or not found response + * @return RedirectResponse redirect response or not found response + * @throws \OCP\Files\NotFoundException * * @NoCSRFRequired * @NoAdminRequired */ public function showFile($fileId) { - try { - $uid = $this->userSession->getUser()->getUID(); - $baseFolder = $this->rootFolder->get($uid . '/files/'); + $uid = $this->userSession->getUser()->getUID(); + $baseFolder = $this->rootFolder->get($uid . '/files/'); + $files = $baseFolder->getById($fileId); + $params = []; + + if (empty($files) && $this->appManager->isEnabledForUser('files_trashbin')) { + $baseFolder = $this->rootFolder->get($uid . '/files_trashbin/files/'); $files = $baseFolder->getById($fileId); - $params = []; - - if (empty($files) && $this->appManager->isEnabledForUser('files_trashbin')) { - $baseFolder = $this->rootFolder->get($uid . '/files_trashbin/files/'); - $files = $baseFolder->getById($fileId); - $params['view'] = 'trashbin'; - } - - if (!empty($files)) { - $file = current($files); - if ($file instanceof Folder) { - // set the full path to enter the folder - $params['dir'] = $baseFolder->getRelativePath($file->getPath()); - } else { - // set parent path as dir - $params['dir'] = $baseFolder->getRelativePath($file->getParent()->getPath()); - // and scroll to the entry - $params['scrollto'] = $file->getName(); - } - return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', $params)); - } - } catch (\OCP\Files\NotFoundException $e) { - return new NotFoundResponse(); + $params['view'] = 'trashbin'; } - return new NotFoundResponse(); + + if (!empty($files)) { + $file = current($files); + if ($file instanceof Folder) { + // set the full path to enter the folder + $params['dir'] = $baseFolder->getRelativePath($file->getPath()); + } else { + // set parent path as dir + $params['dir'] = $baseFolder->getRelativePath($file->getParent()->getPath()); + // and scroll to the entry + $params['scrollto'] = $file->getName(); + } + return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', $params)); + } + throw new \OCP\Files\NotFoundException(); } } diff --git a/apps/files/templates/index.php b/apps/files/templates/index.php index 7281edd3ae..42ce941a4a 100644 --- a/apps/files/templates/index.php +++ b/apps/files/templates/index.php @@ -14,6 +14,7 @@ + diff --git a/apps/files/tests/Controller/ViewControllerTest.php b/apps/files/tests/Controller/ViewControllerTest.php index 049f44fc0a..45495ac9d0 100644 --- a/apps/files/tests/Controller/ViewControllerTest.php +++ b/apps/files/tests/Controller/ViewControllerTest.php @@ -26,6 +26,7 @@ namespace OCA\Files\Tests\Controller; use OCA\Files\Controller\ViewController; use OCP\AppFramework\Http; +use OCP\Files\NotFoundException; use OCP\IUser; use OCP\Template; use Test\TestCase; @@ -259,7 +260,8 @@ class ViewControllerTest extends TestCase { 'isPublic' => false, 'defaultFileSorting' => 'name', 'defaultFileSortingDirection' => 'asc', - 'showHiddenFiles' => false, + 'showHiddenFiles' => 0, + 'fileNotFound' => 0, 'mailNotificationEnabled' => 'no', 'mailPublicNotificationEnabled' => 'no', 'allowShareWithLink' => 'yes', @@ -410,11 +412,14 @@ class ViewControllerTest extends TestCase { ->with(123) ->will($this->returnValue([])); - $expected = new Http\NotFoundResponse(); if ($useShowFile) { - $this->assertEquals($expected, $this->viewController->showFile(123)); + $this->expectException('OCP\Files\NotFoundException'); + $this->viewController->showFile(123); } else { - $this->assertEquals($expected, $this->viewController->index('/whatever', '', '123')); + $response = $this->viewController->index('MyDir', 'MyView', '123'); + $this->assertInstanceOf('OCP\AppFramework\Http\TemplateResponse', $response); + $params = $response->getParams(); + $this->assertEquals(1, $params['fileNotFound']); } } From 68a7aed27b697bdce07006df9b860d62a3b89c67 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 8 Jun 2016 09:24:00 +0200 Subject: [PATCH 2/2] Adjust test to work with phpunit < 5.2 --- apps/files/tests/Controller/ViewControllerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files/tests/Controller/ViewControllerTest.php b/apps/files/tests/Controller/ViewControllerTest.php index 45495ac9d0..34c40ecea5 100644 --- a/apps/files/tests/Controller/ViewControllerTest.php +++ b/apps/files/tests/Controller/ViewControllerTest.php @@ -413,7 +413,7 @@ class ViewControllerTest extends TestCase { ->will($this->returnValue([])); if ($useShowFile) { - $this->expectException('OCP\Files\NotFoundException'); + $this->setExpectedException('OCP\Files\NotFoundException'); $this->viewController->showFile(123); } else { $response = $this->viewController->index('MyDir', 'MyView', '123');