fix(files): trashbin redirect and default fileid Sidebar open

Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>

apps/files/appinfo/routes.php

@@ -139,16 +139,14 @@ $application->registerRoutes(
 				'verb' => 'GET'
 			],
 			[
-				'name' => 'view#index',
+				'name' => 'view#indexView',
 				'url' => '/{view}',
 				'verb' => 'GET',
-				'postfix' => 'view',
 			],
 			[
-				'name' => 'view#index',
+				'name' => 'view#indexViewFileid',
 				'url' => '/{view}/{fileid}',
 				'verb' => 'GET',
-				'postfix' => 'fileid',
 			],
 		],
 		'ocs' => [

apps/files/lib/Controller/ViewController.php

@@ -153,17 +153,36 @@ class ViewController extends Controller {
 	 *
 	 * @param string $fileid
 	 * @return TemplateResponse|RedirectResponse
-	 * @throws NotFoundException
 	 */
-	public function showFile(string $fileid = null, int $openfile = 1): Response {
+	public function showFile(string $fileid = null): Response {
+		if (!$fileid) {
+			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index'));
+		}
+
 		// This is the entry point from the `/f/{fileid}` URL which is hardcoded in the server.
 		try {
-			return $this->redirectToFile($fileid, $openfile !== 0);
+			return $this->redirectToFile((int) $fileid);
 		} catch (NotFoundException $e) {
 			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', ['fileNotFound' => true]));
 		}
 	}
 
+
+	/**
+	 * @NoCSRFRequired
+	 * @NoAdminRequired
+	 * @UseSession
+	 * 
+	 * @param string $dir
+	 * @param string $view
+	 * @param string $fileid
+	 * @param bool $fileNotFound
+	 * @return TemplateResponse|RedirectResponse
+	 */
+	public function indexView($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
+		return $this->index($dir, $view, $fileid, $fileNotFound);
+	}
+
 	/**
 	 * @NoCSRFRequired
 	 * @NoAdminRequired
@@ -173,11 +192,30 @@ class ViewController extends Controller {
 	 * @param string $view
 	 * @param string $fileid
 	 * @param bool $fileNotFound
-	 * @param string $openfile - the openfile URL parameter if it was present in the initial request
 	 * @return TemplateResponse|RedirectResponse
-	 * @throws NotFoundException
 	 */
-	public function index($dir = '', $view = '', $fileid = null, $fileNotFound = false, $openfile = null) {
+	public function indexViewFileid($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
+		return $this->index($dir, $view, $fileid, $fileNotFound);
+	}
+
+	/**
+	 * @NoCSRFRequired
+	 * @NoAdminRequired
+	 * @UseSession
+	 *
+	 * @param string $dir
+	 * @param string $view
+	 * @param string $fileid
+	 * @param bool $fileNotFound
+	 * @return TemplateResponse|RedirectResponse
+	 */
+	public function index($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
+		if ($fileid !== null && $view !== 'trashbin') {
+			try {
+				return $this->redirectToFileIfInTrashbin((int) $fileid);
+			} catch (NotFoundException $e) {}
+		}
+
 		// Load the files we need
 		\OCP\Util::addStyle('files', 'merged');
 		\OCP\Util::addScript('files', 'merged-index', 'files');
@@ -192,8 +230,6 @@ class ViewController extends Controller {
 			$favElements['folders'] = [];
 		}
 
-		$contentItems = [];
-
 		try {
 			// If view is files, we use the directory, otherwise we use the root storage
 			$storageInfo =  $this->getStorageInfo(($view === 'files' && $dir) ? $dir : '/');
@@ -237,19 +273,19 @@ class ViewController extends Controller {
 		$policy->addAllowedWorkerSrcDomain('\'self\'');
 		$response->setContentSecurityPolicy($policy);
 
-		$this->provideInitialState($dir, $openfile);
+		$this->provideInitialState($dir, $fileid);
 
 		return $response;
 	}
 
 	/**
-	 * Add openFileInfo in initialState if $openfile is set.
+	 * Add openFileInfo in initialState.
 	 * @param string $dir - the ?dir= URL param
-	 * @param string $openfile - the ?openfile= URL param
+	 * @param string $fileid - the fileid URL param
 	 * @return void
 	 */
-	private function provideInitialState(string $dir, ?string $openfile): void {
-		if ($openfile === null) {
+	private function provideInitialState(string $dir, ?string $fileid): void {
+		if ($fileid === null) {
 			return;
 		}
 
@@ -261,7 +297,7 @@ class ViewController extends Controller {
 
 		$uid = $user->getUID();
 		$userFolder = $this->rootFolder->getUserFolder($uid);
-		$nodes = $userFolder->getById((int) $openfile);
+		$nodes = $userFolder->getById((int) $fileid);
 		$node = array_shift($nodes);
 
 		if ($node === null) {
@@ -293,44 +329,70 @@ class ViewController extends Controller {
 	}
 
 	/**
-	 * Redirects to the file list and highlight the given file id
+	 * Redirects to the trashbin file list and highlight the given file id
 	 *
-	 * @param string $fileId file id to show
-	 * @param bool $setOpenfile - whether or not to set the openfile URL parameter
+	 * @param int $fileId file id to show
 	 * @return RedirectResponse redirect response or not found response
-	 * @throws \OCP\Files\NotFoundException
+	 * @throws NotFoundException
 	 */
-	private function redirectToFile($fileId, bool $setOpenfile = false) {
+	private function redirectToFileIfInTrashbin($fileId): RedirectResponse {
 		$uid = $this->userSession->getUser()->getUID();
 		$baseFolder = $this->rootFolder->getUserFolder($uid);
-		$files = $baseFolder->getById($fileId);
+		$nodes = $baseFolder->getById($fileId);
 		$params = [];
 
-		if (empty($files) && $this->appManager->isEnabledForUser('files_trashbin')) {
+		if (empty($nodes) && $this->appManager->isEnabledForUser('files_trashbin')) {
+			/** @var Folder */
 			$baseFolder = $this->rootFolder->get($uid . '/files_trashbin/files/');
-			$files = $baseFolder->getById($fileId);
+			$nodes = $baseFolder->getById($fileId);
 			$params['view'] = 'trashbin';
+
+			if (!empty($nodes)) {
+				$node = current($nodes);
+				$params['fileid'] = $fileId;
+				if ($node instanceof Folder) {
+					// set the full path to enter the folder
+					$params['dir'] = $baseFolder->getRelativePath($node->getPath());
+				} else {
+					// set parent path as dir
+					$params['dir'] = $baseFolder->getRelativePath($node->getParent()->getPath());
+				}
+				return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.indexViewFileid', $params));
+			}
 		}
+		throw new NotFoundException();
+	}
 
-		if (!empty($files)) {
-			$file = current($files);
-			if ($file instanceof Folder) {
+	/**
+	 * Redirects to the file list and highlight the given file id
+	 *
+	 * @param int $fileId file id to show
+	 * @return RedirectResponse redirect response or not found response
+	 * @throws NotFoundException
+	 */
+	private function redirectToFile(int $fileId) {
+		$uid = $this->userSession->getUser()->getUID();
+		$baseFolder = $this->rootFolder->getUserFolder($uid);
+		$nodes = $baseFolder->getById($fileId);
+		$params = [];
+
+		try {
+			$this->redirectToFileIfInTrashbin($fileId);
+		} catch (NotFoundException $e) {}
+
+		if (!empty($nodes)) {
+			$node = current($nodes);
+			$params['fileid'] = $fileId;
+			if ($node instanceof Folder) {
 				// set the full path to enter the folder
-				$params['dir'] = $baseFolder->getRelativePath($file->getPath());
+				$params['dir'] = $baseFolder->getRelativePath($node->getPath());
 			} else {
 				// set parent path as dir
-				$params['dir'] = $baseFolder->getRelativePath($file->getParent()->getPath());
-				// and scroll to the entry
-				$params['scrollto'] = $file->getName();
-
-				if ($setOpenfile) {
-					// forward the openfile URL parameter.
-					$params['openfile'] = $fileId;
-				}
+				$params['dir'] = $baseFolder->getRelativePath($node->getParent()->getPath());
 			}
-
-			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', $params));
+			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.indexViewFileid', $params));
 		}
-		throw new \OCP\Files\NotFoundException();
+	
+		throw new NotFoundException();
 	}
 }

apps/files/src/actions/openInFilesAction.spec.ts

@@ -78,7 +78,7 @@ describe('Open in files action execute tests', () => {
 		// Silent action
 		expect(exec).toBe(null)
 		expect(goToRouteMock).toBeCalledTimes(1)
-		expect(goToRouteMock).toBeCalledWith(null, { fileid: 1, view: 'files' }, { fileid: 1, dir: '/Foo', openfile: true })
+		expect(goToRouteMock).toBeCalledWith(null, { fileid: 1, view: 'files' }, { fileid: 1, dir: '/Foo' })
 	})
 
 	test('Open in files with folder', async () => {
@@ -98,6 +98,6 @@ describe('Open in files action execute tests', () => {
 		// Silent action
 		expect(exec).toBe(null)
 		expect(goToRouteMock).toBeCalledTimes(1)
-		expect(goToRouteMock).toBeCalledWith(null, { fileid: 1, view: 'files' }, { fileid: 1, dir: '/Foo/Bar', openfile: true })
+		expect(goToRouteMock).toBeCalledWith(null, { fileid: 1, view: 'files' }, { fileid: 1, dir: '/Foo/Bar' })
 	})
 })

apps/files/src/actions/openInFilesAction.ts

@@ -44,7 +44,7 @@ export const action = new FileAction({
 		window.OCP.Files.Router.goToRoute(
 			null, // use default route
 			{ view: 'files', fileid: node.fileid },
-			{ dir, fileid: node.fileid, openfile: true },
+			{ dir, fileid: node.fileid },
 		)
 		return null
 	},

apps/files/src/actions/sidebarAction.ts

@@ -42,6 +42,10 @@ export const action = new FileAction({
 			return false
 		}
 
+		if (!nodes[0]) {
+			return false
+		}
+
 		// Only work if the sidebar is available
 		if (!window?.OCA?.Files?.Sidebar) {
 			return false
@@ -53,7 +57,7 @@ export const action = new FileAction({
 	async exec(node: Node, view: Navigation) {
 		try {
 			// TODO: migrate Sidebar to use a Node instead
-			window?.OCA?.Files?.Sidebar?.open?.(node.path)
+			await window.OCA.Files.Sidebar.open(node.path)
 
 			// Silently update current fileid
 			window.OCP.Files.Router.goToRoute(

apps/files/src/components/FilesListVirtual.vue

@@ -69,15 +69,17 @@
 <script lang="ts">
 import { translate, translatePlural } from '@nextcloud/l10n'
 import { getFileListHeaders, type Node } from '@nextcloud/files'
+import { showError } from '@nextcloud/dialogs'
 import Vue from 'vue'
 import VirtualList from './VirtualList.vue'
 
+import { action as sidebarAction } from '../actions/sidebarAction.ts'
 import FileEntry from './FileEntry.vue'
 import FilesListHeader from './FilesListHeader.vue'
 import FilesListTableFooter from './FilesListTableFooter.vue'
 import FilesListTableHeader from './FilesListTableHeader.vue'
 import filesListWidthMixin from '../mixins/filesListWidth.ts'
-import { showError } from '@nextcloud/dialogs'
+import logger from '../logger.js'
 
 export default Vue.extend({
 	name: 'FilesListVirtual',
@@ -174,10 +176,10 @@ export default Vue.extend({
 		if (document.documentElement.clientWidth > 1024) {
 			// Open the sidebar on the file if it's in the url and
 			// we're just loaded the app for the first time.
-			const Sidebar = window?.OCA?.Files?.Sidebar
-			const node = this.nodes.find(node => node.fileid === this.fileId) as Node
-			if (Sidebar && node) {
-				Sidebar.open(node.path)
+			const node = this.nodes.find(n => n.fileid === this.fileId) as Node
+			if (node && sidebarAction?.enabled?.([node], this.currentView)) {
+				logger.debug('Opening sidebar on file ' + node.path, { node })
+				sidebarAction.exec(node, this.currentView, this.currentFolder)
 			}
 		}
 	},

apps/files/src/views/Sidebar.vue

@@ -451,39 +451,41 @@ export default {
 		 * @throws {Error} loading failure
 		 */
 		async open(path) {
+			if (!path || path.trim() === '') {
+				throw new Error(`Invalid path '${path}'`)
+			}
+
 			// update current opened file
 			this.Sidebar.file = path
 
-			if (path && path.trim() !== '') {
-				// reset data, keep old fileInfo to not reload all tabs and just hide them
-				this.error = null
-				this.loading = true
+			// reset data, keep old fileInfo to not reload all tabs and just hide them
+			this.error = null
+			this.loading = true
 
-				try {
-					this.fileInfo = await FileInfo(this.davPath)
-					// adding this as fallback because other apps expect it
-					this.fileInfo.dir = this.file.split('/').slice(0, -1).join('/')
-
-					// DEPRECATED legacy views
-					// TODO: remove
-					this.views.forEach(view => {
-						view.setFileInfo(this.fileInfo)
-					})
-
-					this.$nextTick(() => {
-						if (this.$refs.tabs) {
-							this.$refs.tabs.updateTabs()
-						}
-						this.setActiveTab(this.Sidebar.activeTab || this.tabs[0].id)
-					})
-				} catch (error) {
-					this.error = t('files', 'Error while loading the file data')
-					console.error('Error while loading the file data', error)
+			try {
+				this.fileInfo = await FileInfo(this.davPath)
+				// adding this as fallback because other apps expect it
+				this.fileInfo.dir = this.file.split('/').slice(0, -1).join('/')
+
+				// DEPRECATED legacy views
+				// TODO: remove
+				this.views.forEach(view => {
+					view.setFileInfo(this.fileInfo)
+				})
 
-					throw new Error(error)
-				} finally {
-					this.loading = false
-				}
+				this.$nextTick(() => {
+					if (this.$refs.tabs) {
+						this.$refs.tabs.updateTabs()
+					}
+					this.setActiveTab(this.Sidebar.activeTab || this.tabs[0].id)
+				})
+			} catch (error) {
+				this.error = t('files', 'Error while loading the file data')
+				console.error('Error while loading the file data', error)
+
+				throw new Error(error)
+			} finally {
+				this.loading = false
 			}
 		},
 

apps/files/tests/Controller/ViewControllerTest.php

@@ -163,74 +163,28 @@ class ViewControllerTest extends TestCase {
 				[$this->user->getUID(), 'files', 'crop_image_previews', true, true],
 				[$this->user->getUID(), 'files', 'show_grid', true],
 			]);
+		
+		$baseFolderFiles = $this->getMockBuilder(Folder::class)->getMock();
+
+		$this->rootFolder->expects($this->any())
+			->method('getUserFolder')
+			->with('testuser1')
+			->willReturn($baseFolderFiles);
 
 		$this->config
 			->expects($this->any())
 			->method('getAppValue')
 			->willReturnArgument(2);
-		$this->shareManager->method('shareApiAllowLinks')
-			->willReturn(true);
-
-		$nav = new Template('files', 'appnavigation');
-		$nav->assign('navigationItems', [
-			'files' => [
-				'id' => 'files',
-				'appname' => 'files',
-				'script' => 'list.php',
-				'order' => 0,
-				'name' => \OC::$server->getL10N('files')->t('All files'),
-				'active' => false,
-				'icon' => '',
-				'type' => 'link',
-				'classes' => '',
-				'expanded' => false,
-				'unread' => 0,
-			],
-			'systemtagsfilter' => [
-				'id' => 'systemtagsfilter',
-				'appname' => 'systemtags',
-				'script' => 'list.php',
-				'order' => 25,
-				'name' => \OC::$server->getL10N('systemtags')->t('Tags'),
-				'active' => false,
-				'icon' => '',
-				'type' => 'link',
-				'classes' => '',
-				'expanded' => false,
-				'unread' => 0,
-			],
-		]);
 
 		$expected = new Http\TemplateResponse(
 			'files',
 			'index',
 			[
-				'usedSpacePercent' => 123,
-				'owner' => 'MyName',
-				'ownerDisplayName' => 'MyDisplayName',
-				'isPublic' => false,
-				'defaultFileSorting' => 'basename',
-				'defaultFileSortingDirection' => 'asc',
-				'showHiddenFiles' => 0,
-				'cropImagePreviews' => 1,
 				'fileNotFound' => 0,
-				'allowShareWithLink' => 'yes',
-				'appNavigation' => $nav,
-				'appContents' => [
-					'files' => [
-						'id' => 'files',
-						'content' => null,
-					],
-					'systemtagsfilter' => [
-						'id' => 'systemtagsfilter',
-						'content' => null,
-					],
-				],
-				'hiddenFields' => [],
-				'showgridview' => null
 			]
 		);
 		$policy = new Http\ContentSecurityPolicy();
+		$policy->addAllowedWorkerSrcDomain('\'self\'');
 		$policy->addAllowedFrameDomain('\'self\'');
 		$expected->setContentSecurityPolicy($policy);
 
@@ -249,100 +203,6 @@ class ViewControllerTest extends TestCase {
 		$this->assertEquals($expected, $this->viewController->index('MyDir', 'MyView'));
 	}
 
-	public function testShowFileRouteWithFolder() {
-		$node = $this->getMockBuilder(Folder::class)->getMock();
-		$node->expects($this->once())
-			->method('getPath')
-			->willReturn('/testuser1/files/test/sub');
-
-		$baseFolder = $this->getMockBuilder(Folder::class)->getMock();
-
-		$this->rootFolder->expects($this->once())
-			->method('getUserFolder')
-			->with('testuser1')
-			->willReturn($baseFolder);
-
-		$baseFolder->expects($this->once())
-			->method('getById')
-			->with(123)
-			->willReturn([$node]);
-		$baseFolder->expects($this->once())
-			->method('getRelativePath')
-			->with('/testuser1/files/test/sub')
-			->willReturn('/test/sub');
-
-		$this->urlGenerator
-			->expects($this->once())
-			->method('linkToRoute')
-			->with('files.view.index', ['dir' => '/test/sub'])
-			->willReturn('/apps/files/?dir=/test/sub');
-
-		$expected = new Http\RedirectResponse('/apps/files/?dir=/test/sub');
-		$this->assertEquals($expected, $this->viewController->index('', '', '123'));
-	}
-
-	public function testShowFileRouteWithFile() {
-		$parentNode = $this->getMockBuilder(Folder::class)->getMock();
-		$parentNode->expects($this->once())
-			->method('getPath')
-			->willReturn('testuser1/files/test');
-
-		$baseFolder = $this->getMockBuilder(Folder::class)->getMock();
-
-		$this->rootFolder->expects($this->once())
-			->method('getUserFolder')
-			->with('testuser1')
-			->willReturn($baseFolder);
-
-		$node = $this->getMockBuilder(File::class)->getMock();
-		$node->expects($this->once())
-			->method('getParent')
-			->willReturn($parentNode);
-		$node->expects($this->once())
-			->method('getName')
-			->willReturn('somefile.txt');
-
-		$baseFolder->expects($this->once())
-			->method('getById')
-			->with(123)
-			->willReturn([$node]);
-		$baseFolder->expects($this->once())
-			->method('getRelativePath')
-			->with('testuser1/files/test')
-			->willReturn('/test');
-
-		$this->urlGenerator
-			->expects($this->once())
-			->method('linkToRoute')
-			->with('files.view.index', ['dir' => '/test', 'scrollto' => 'somefile.txt'])
-			->willReturn('/apps/files/?dir=/test/sub&scrollto=somefile.txt');
-
-		$expected = new Http\RedirectResponse('/apps/files/?dir=/test/sub&scrollto=somefile.txt');
-		$this->assertEquals($expected, $this->viewController->index('', '', '123'));
-	}
-
-	public function testShowFileRouteWithInvalidFileId() {
-		$baseFolder = $this->getMockBuilder(Folder::class)->getMock();
-		$this->rootFolder->expects($this->once())
-			->method('getUserFolder')
-			->with('testuser1')
-			->willReturn($baseFolder);
-
-		$baseFolder->expects($this->once())
-			->method('getById')
-			->with(123)
-			->willReturn([]);
-
-		$this->urlGenerator->expects($this->once())
-			->method('linkToRoute')
-			->with('files.view.index', ['fileNotFound' => true])
-			->willReturn('redirect.url');
-
-		$response = $this->viewController->index('', 'MyView', '123');
-		$this->assertInstanceOf('OCP\AppFramework\Http\RedirectResponse', $response);
-		$this->assertEquals('redirect.url', $response->getRedirectURL());
-	}
-
 	public function testShowFileRouteWithTrashedFile() {
 		$this->appManager->expects($this->once())
 			->method('isEnabledForUser')
@@ -357,7 +217,7 @@ class ViewControllerTest extends TestCase {
 		$baseFolderFiles = $this->getMockBuilder(Folder::class)->getMock();
 		$baseFolderTrash = $this->getMockBuilder(Folder::class)->getMock();
 
-		$this->rootFolder->expects($this->once())
+		$this->rootFolder->expects($this->any())
 			->method('getUserFolder')
 			->with('testuser1')
 			->willReturn($baseFolderFiles);
@@ -366,7 +226,7 @@ class ViewControllerTest extends TestCase {
 			->with('testuser1/files_trashbin/files/')
 			->willReturn($baseFolderTrash);
 
-		$baseFolderFiles->expects($this->once())
+		$baseFolderFiles->expects($this->any())
 			->method('getById')
 			->with(123)
 			->willReturn([]);
@@ -375,9 +235,6 @@ class ViewControllerTest extends TestCase {
 		$node->expects($this->once())
 			->method('getParent')
 			->willReturn($parentNode);
-		$node->expects($this->once())
-			->method('getName')
-			->willReturn('somefile.txt');
 
 		$baseFolderTrash->expects($this->once())
 			->method('getById')
@@ -391,10 +248,10 @@ class ViewControllerTest extends TestCase {
 		$this->urlGenerator
 			->expects($this->once())
 			->method('linkToRoute')
-			->with('files.view.index', ['view' => 'trashbin', 'dir' => '/test.d1462861890/sub', 'scrollto' => 'somefile.txt'])
-			->willReturn('/apps/files/?view=trashbin&dir=/test.d1462861890/sub&scrollto=somefile.txt');
+			->with('files.view.indexViewFileid', ['view' => 'trashbin', 'dir' => '/test.d1462861890/sub', 'fileid' => '123'])
+			->willReturn('/apps/files/trashbin/123?dir=/test.d1462861890/sub');
 
-		$expected = new Http\RedirectResponse('/apps/files/?view=trashbin&dir=/test.d1462861890/sub&scrollto=somefile.txt');
+		$expected = new Http\RedirectResponse('/apps/files/trashbin/123?dir=/test.d1462861890/sub');
 		$this->assertEquals($expected, $this->viewController->index('', '', '123'));
 	}
 }