Merge pull request #9558 from nextcloud/fix/9489/trashbin_previews

Previews on for all trashbin files

apps/files_trashbin/appinfo/routes.php

@@ -29,7 +29,7 @@ $application->registerRoutes($this, [
 	'routes' => [
 		[
 			'name' => 'Preview#getPreview',
-			'url' => '/ajax/preview.php',
+			'url' => '/preview',
 			'verb' => 'GET',
 		],
 	],

apps/files_trashbin/js/filelist.js

@@ -260,7 +260,7 @@
 		},
 
 		generatePreviewUrl: function(urlSpec) {
-			return OC.generateUrl('/apps/files_trashbin/ajax/preview.php?') + $.param(urlSpec);
+			return OC.generateUrl('/apps/files_trashbin/preview?') + $.param(urlSpec);
 		},
 
 		getDownloadUrl: function() {

apps/files_trashbin/lib/Controller/PreviewController.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, Roeland Jago Douma <roeland@famdouma.nl>
  *
@@ -26,6 +27,7 @@ namespace OCA\Files_Trashbin\Controller;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\File;
 use OCP\Files\Folder;
 use OCP\Files\IMimeTypeDetector;
@@ -48,45 +50,36 @@ class PreviewController extends Controller {
 	/** @var IPreview */
 	private $previewManager;
 
-	/**
-	 * @param string $appName
-	 * @param IRequest $request
-	 * @param IRootFolder $rootFolder
-	 * @param $userId
-	 * @param IMimeTypeDetector $mimeTypeDetector
-	 * @param IPreview $previewManager
-	 */
-	public function __construct($appName,
+	/** @var ITimeFactory */
+	private $time;
+
+	public function __construct(string $appName,
 								IRequest $request,
 								IRootFolder $rootFolder,
-								$userId,
+								string $userId,
 								IMimeTypeDetector $mimeTypeDetector,
-								IPreview $previewManager) {
+								IPreview $previewManager,
+								ITimeFactory $time) {
 		parent::__construct($appName, $request);
 
 		$this->rootFolder = $rootFolder;
 		$this->userId = $userId;
 		$this->mimeTypeDetector = $mimeTypeDetector;
 		$this->previewManager = $previewManager;
+		$this->time = $time;
 	}
 
 	/**
 	 * @NoAdminRequired
 	 * @NoCSRFRequired
 	 *
-	 * @param string $file
-	 * @param int $x
-	 * @param int $y
 	 * @return DataResponse|Http\FileDisplayResponse
 	 */
 	public function getPreview(
-		$file = '',
-		$x = 44,
-		$y = 44
+		int $fileId,
+		int $x = 44,
+		int $y = 44
 	) {
-		if ($file === '') {
-			return new DataResponse([], Http::STATUS_BAD_REQUEST);
-		}
 
 		if ($x === 0 || $y === 0) {
 			return new DataResponse([], Http::STATUS_BAD_REQUEST);
@@ -96,23 +89,41 @@ class PreviewController extends Controller {
 			$userFolder = $this->rootFolder->getUserFolder($this->userId);
 			/** @var Folder $trash */
 			$trash = $userFolder->getParent()->get('files_trashbin/files');
-			$trashFile = $trash->get($file);
+			$trashFiles = $trash->getById($fileId);
+
+			if (empty($trashFiles)) {
+				throw new NotFoundException();
+			}
+
+			$trashFile = array_pop($trashFiles);
 
 			if ($trashFile instanceof Folder) {
 				return new DataResponse([], Http::STATUS_BAD_REQUEST);
 			}
 
-			/** @var File $trashFile */
-			$fileName = $trashFile->getName();
-			$i = strrpos($fileName, '.');
-			if ($i !== false) {
-				$fileName = substr($fileName, 0, $i);
-			}
+			/*
+			 * Files in the root of the trashbin are timetamped.
+			 * So we have to strip that in order to properly detect the mimetype of the file.
+			 */
+			if ($trashFile->getParent()->getPath() === $trash->getPath()) {
+				/** @var File $trashFile */
+				$fileName = $trashFile->getName();
+				$i = strrpos($fileName, '.');
+				if ($i !== false) {
+					$fileName = substr($fileName, 0, $i);
+				}
 
-			$mimeType = $this->mimeTypeDetector->detectPath($fileName);
+				$mimeType = $this->mimeTypeDetector->detectPath($fileName);
+			} else {
+				$mimeType = $this->mimeTypeDetector->detectPath($trashFile->getName());
+			}
 
 			$f = $this->previewManager->getPreview($trashFile, $x, $y, true, IPreview::MODE_FILL, $mimeType);
-			return new Http\FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]);
+			$response = new Http\FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]);
+
+			// Cache previews for 24H
+			$response->cacheFor(3600 * 24);
+			return $response;
 		} catch (NotFoundException $e) {
 			return new DataResponse([], Http::STATUS_NOT_FOUND);
 		} catch (\InvalidArgumentException $e) {

apps/files_trashbin/lib/Helper.php

@@ -116,10 +116,9 @@ class Helper {
 	 */
 	public static function formatFileInfos($fileInfos) {
 		$files = array();
-		$id = 0;
 		foreach ($fileInfos as $i) {
 			$entry = \OCA\Files\Helper::formatFileInfo($i);
-			$entry['id'] = $id++;
+			$entry['id'] = $i->getId();
 			$entry['etag'] = $entry['mtime']; // add fake etag, it is only needed to identify the preview image
 			$entry['permissions'] = \OCP\Constants::PERMISSION_READ;
 			$files[] = $entry;

apps/files_trashbin/tests/Controller/PreviewControllerTest.php

@@ -26,6 +26,7 @@ use OCA\Files_Trashbin\Controller\PreviewController;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\FileDisplayResponse;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\File;
 use OCP\Files\Folder;
 use OCP\Files\IMimeTypeDetector;
@@ -50,6 +51,9 @@ class PreviewControllerTest extends TestCase {
 	/** @var IPreview|\PHPUnit_Framework_MockObject_MockObject */
 	private $previewManager;
 
+	/** @var ITimeFactory|\PHPUnit_Framework_MockObject_MockObject */
+	private $time;
+
 	/** @var PreviewController */
 	private $controller;
 
@@ -60,6 +64,7 @@ class PreviewControllerTest extends TestCase {
 		$this->userId = 'user';
 		$this->mimeTypeDetector = $this->createMock(IMimeTypeDetector::class);
 		$this->previewManager = $this->createMock(IPreview::class);
+		$this->time = $this->createMock(ITimeFactory::class);
 
 		$this->controller = new PreviewController(
 			'files_versions',
@@ -67,26 +72,20 @@ class PreviewControllerTest extends TestCase {
 			$this->rootFolder,
 			$this->userId,
 			$this->mimeTypeDetector,
-			$this->previewManager
+			$this->previewManager,
+			$this->time
 		);
 	}
 
-	public function testInvalidFile() {
-		$res = $this->controller->getPreview('');
-		$expected = new DataResponse([], Http::STATUS_BAD_REQUEST);
-
-		$this->assertEquals($expected, $res);
-	}
-
 	public function testInvalidWidth() {
-		$res = $this->controller->getPreview('file', 0);
+		$res = $this->controller->getPreview(42, 0);
 		$expected = new DataResponse([], Http::STATUS_BAD_REQUEST);
 
 		$this->assertEquals($expected, $res);
 	}
 
 	public function testInvalidHeight() {
-		$res = $this->controller->getPreview('file', 10, 0);
+		$res = $this->controller->getPreview(42, 10, 0);
 		$expected = new DataResponse([], Http::STATUS_BAD_REQUEST);
 
 		$this->assertEquals($expected, $res);
@@ -111,12 +110,15 @@ class PreviewControllerTest extends TestCase {
 			->willReturn('myMime');
 
 		$file = $this->createMock(File::class);
-		$trash->method('get')
-			->with($this->equalTo('file.1234'))
-			->willReturn($file);
+		$trash->method('getById')
+			->with($this->equalTo(42))
+			->willReturn([$file]);
 		$file->method('getName')
 			->willReturn('file.1234');
 
+		$file->method('getParent')
+			->willReturn($trash);
+
 		$preview = $this->createMock(ISimpleFile::class);
 		$this->previewManager->method('getPreview')
 			->with($this->equalTo($file), 10, 10, true, IPreview::MODE_FILL, 'myMime')
@@ -124,8 +126,14 @@ class PreviewControllerTest extends TestCase {
 		$preview->method('getMimeType')
 			->willReturn('previewMime');
 
-		$res = $this->controller->getPreview('file.1234', 10, 10);
+		$this->time->method('getTime')
+			->willReturn(1337);
+
+		$this->overwriteService(ITimeFactory::class, $this->time);
+
+		$res = $this->controller->getPreview(42, 10, 10);
 		$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'previewMime']);
+		$expected->cacheFor(3600 * 24);
 
 		$this->assertEquals($expected, $res);
 	}
@@ -144,11 +152,11 @@ class PreviewControllerTest extends TestCase {
 			->with('files_trashbin/files')
 			->willReturn($trash);
 
-		$trash->method('get')
-			->with($this->equalTo('file.1234'))
-			->willThrowException(new NotFoundException());
+		$trash->method('getById')
+			->with($this->equalTo(42))
+			->willReturn([]);
 
-		$res = $this->controller->getPreview('file.1234', 10, 10);
+		$res = $this->controller->getPreview(42, 10, 10);
 		$expected = new DataResponse([], Http::STATUS_NOT_FOUND);
 
 		$this->assertEquals($expected, $res);
@@ -169,11 +177,11 @@ class PreviewControllerTest extends TestCase {
 			->willReturn($trash);
 
 		$folder = $this->createMock(Folder::class);
-		$trash->method('get')
-			->with($this->equalTo('folder'))
-			->willReturn($folder);
+		$trash->method('getById')
+			->with($this->equalTo(43))
+			->willReturn([$folder]);
 
-		$res = $this->controller->getPreview('folder', 10, 10);
+		$res = $this->controller->getPreview(43, 10, 10);
 		$expected = new DataResponse([], Http::STATUS_BAD_REQUEST);
 
 		$this->assertEquals($expected, $res);