Merge pull request #505 from nextcloud/bugfix/noid/upload-errors

Display attachment errors properly and fix uploading big files

css/style.scss

@@ -911,14 +911,11 @@ input.input-inline {
 	}
 }
 
-#card-attachments {
-	ul {
-		margin: 5px;
-	}
-
-	.details {
-		font-size: 8pt;
-		padding-left: 15px;
+.card-attachments {
+	.error {
+		padding-left: 38px;
+		margin-bottom: 5px;
+		background-position: 10px;
 	}
 }
 

js/service/FileService.js

@@ -30,13 +30,17 @@ export default class FileService {
 		this.cardservice = CardService;
 		this.uploader.onAfterAddingFile = this.onAfterAddingFile.bind(this);
 		this.uploader.onSuccessItem = this.onSuccessItem.bind(this);
+		this.uploader.onErrorItem = this.onErrorItem.bind(this);
+
+		this.status = null;
 	}
 
 
 	runUpload (fileItem, attachmentId) {
-		fileItem.url = OC.generateUrl('/apps/deck/cards/' + fileItem.cardId + '/attachment');
+		this.status = null;
+		fileItem.url = OC.generateUrl('/apps/deck/cards/' + fileItem.cardId + '/attachment?type=deck_file');
 		if (typeof attachmentId !== 'undefined') {
-			fileItem.url = OC.generateUrl('/apps/deck/cards/' + fileItem.cardId + '/attachment/' + attachmentId);
+			fileItem.url = OC.generateUrl('/apps/deck/cards/' + fileItem.cardId + '/attachment/' + attachmentId + '?type=deck_file');
 		} else {
 			fileItem.formData = [
 				{
@@ -95,6 +99,13 @@ export default class FileService {
 		this.cardservice.get(item.cardId).attachments.push(response);
 	}
 
+	onErrorItem (item, response) {
+		this.status = {
+			error: t('deck', `Failed to upload:`) + ' ' + item.file.name,
+			message: response.message
+		};
+	}
+
 }
 
 app.service('FileService', FileService);

lib/Service/AttachmentService.php

@@ -32,9 +32,11 @@ use OCA\Deck\Db\CardMapper;
 use OCA\Deck\InvalidAttachmentType;
 use OCA\Deck\NoPermissionException;
 use OCA\Deck\NotFoundException;
+use OCA\Deck\StatusException;
 use OCP\AppFramework\Http\Response;
 use OCP\ICache;
 use OCP\ICacheFactory;
+use OCP\IL10N;
 
 class AttachmentService {
 
@@ -48,6 +50,8 @@ class AttachmentService {
 	private $application;
 	/** @var ICache */
 	private $cache;
+	/** @var IL10N */
+	private $l10n;
 
 	/**
 	 * AttachmentService constructor.
@@ -58,16 +62,17 @@ class AttachmentService {
 	 * @param Application $application
 	 * @param ICacheFactory $cacheFactory
 	 * @param $userId
+	 * @param IL10N $l10n
 	 * @throws \OCP\AppFramework\QueryException
 	 */
-	public function __construct(AttachmentMapper $attachmentMapper, CardMapper $cardMapper, PermissionService $permissionService, Application $application, ICacheFactory $cacheFactory, $userId) {
+	public function __construct(AttachmentMapper $attachmentMapper, CardMapper $cardMapper, PermissionService $permissionService, Application $application, ICacheFactory $cacheFactory, $userId, IL10N $l10n) {
 		$this->attachmentMapper = $attachmentMapper;
 		$this->cardMapper = $cardMapper;
 		$this->permissionService = $permissionService;
 		$this->userId = $userId;
 		$this->application = $application;
 		$this->cache = $cacheFactory->createDistributed('deck-card-attachments-');
-
+		$this->l10n = $l10n;
 
 		// Register shipped attachment services
 		// TODO: move this to a plugin based approach once we have different types of attachments
@@ -145,6 +150,9 @@ class AttachmentService {
 		} catch (InvalidAttachmentType $e) {
 			// just store the data
 		}
+		if ($attachment->getData() === null) {
+			throw new StatusException($this->l10n->t('No data was provided to create an attachment.'));
+		}
 		$attachment = $this->attachmentMapper->insert($attachment);
 
 		// extend data so the frontend can use it properly after creating

lib/Service/FileService.php

@@ -25,9 +25,11 @@ namespace OCA\Deck\Service;
 
 use OC\Security\CSP\ContentSecurityPolicyManager;
 use OCA\Deck\Db\Attachment;
+use OCA\Deck\StatusException;
 use OCP\AppFramework\Http\ContentSecurityPolicy;
 use OCP\AppFramework\Http\EmptyContentSecurityPolicy;
 use OCP\AppFramework\Http\FileDisplayResponse;
+use OCP\Files\Cache\IScanner;
 use OCP\Files\IAppData;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
@@ -100,6 +102,10 @@ class FileService implements IAttachmentService {
 		return $attachment;
 	}
 
+	/**
+	 * @return array
+	 * @throws StatusException
+	 */
 	private function getUploadedFile () {
 		$file = $this->request->getUploadedFile('file');
 		$error = null;
@@ -115,13 +121,13 @@ class FileService implements IAttachmentService {
 		];
 
 		if (empty($file)) {
-			$error = $this->l10n->t('No file uploaded');
+			$error = $this->l10n->t('No file uploaded or file size exceeds maximum of %s', [\OCP\Util::humanFileSize(\OCP\Util::uploadLimit())]);
 		}
 		if (!empty($file) && array_key_exists('error', $file) && $file['error'] !== UPLOAD_ERR_OK) {
 			$error = $phpFileUploadErrors[$file['error']];
 		}
 		if ($error !== null) {
-			throw new \Exception($error);
+			throw new StatusException($error);
 		}
 		return $file;
 	}
@@ -131,16 +137,21 @@ class FileService implements IAttachmentService {
 		$folder = $this->getFolder($attachment);
 		$fileName = $file['name'];
 		if ($folder->fileExists($fileName)) {
-			throw new \Exception('File already exists.');
+			throw new StatusException('File already exists.');
 		}
+
 		$target = $folder->newFile($fileName);
-		$target->putContent(file_get_contents($file['tmp_name'], 'r'));
+		$content = fopen($file['tmp_name'], 'rb');
+		$target->putContent($content);
+		fclose($content);
 
 		$attachment->setData($fileName);
 	}
 
 	/**
 	 * This method requires to be used with POST so we can properly get the form data
+	 *
+	 * @throws \Exception
 	 */
 	public function update(Attachment $attachment) {
 		$file = $this->getUploadedFile();
@@ -148,7 +159,9 @@ class FileService implements IAttachmentService {
 		$attachment->setData($fileName);
 
 		$target = $this->getFileForAttachment($attachment);
-		$target->putContent(file_get_contents($file['tmp_name'], 'r'));
+		$content = fopen($file['tmp_name'], 'rb');
+		$target->putContent($content);
+		fclose($content);
 
 		$attachment->setLastModified(time());
 	}

templates/part.card.php

@@ -104,6 +104,7 @@
 		</div>
 	</div>
 	<div class="section-content card-attachments">
+		<div class="error icon-error" ng-if="fileservice.status"><strong>{{ fileservice.status.error }}</strong><br />{{ fileservice.status.message }}</div>
 		<attachment-list-component ng-if="params.tab === 1 && cardservice.getCurrent() && isArray(cardservice.getCurrent().attachments)" attachments="cardservice.getCurrent().attachments"></attachment-list-component>
 	</div>
 

tests/unit/Service/AttachmentServiceTest.php

@@ -34,6 +34,7 @@ use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\IAppContainer;
 use OCP\ICache;
 use OCP\ICacheFactory;
+use OCP\IL10N;
 use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
@@ -67,6 +68,8 @@ class AttachmentServiceTest extends TestCase {
 	private $appContainer;
 	/** ICache */
 	private $cache;
+	/** @var IL10N */
+	private $l10n;
 
 	/**
 	 * @throws \OCP\AppFramework\QueryException
@@ -91,7 +94,9 @@ class AttachmentServiceTest extends TestCase {
 			->method('getContainer')
 			->willReturn($this->appContainer);
 
-        $this->attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->permissionService, $this->application, $this->cacheFactory, $this->userId);
+		$this->l10n = $this->createMock(IL10N::class);
+
+        $this->attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->permissionService, $this->application, $this->cacheFactory, $this->userId, $this->l10n);
     }
 
     public function testRegisterAttachmentService() {
@@ -103,7 +108,7 @@ class AttachmentServiceTest extends TestCase {
 		$application->expects($this->any())
 			->method('getContainer')
 			->willReturn($appContainer);
-		$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->permissionService, $application, $this->cacheFactory, $this->userId);
+		$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->permissionService, $application, $this->cacheFactory, $this->userId, $this->l10n);
 		$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
 		$this->assertEquals($fileServiceMock, $attachmentService->getService('deck_file'));
 		$this->assertEquals(MyAttachmentService::class, get_class($attachmentService->getService('custom')));
@@ -121,7 +126,7 @@ class AttachmentServiceTest extends TestCase {
 		$application->expects($this->any())
 			->method('getContainer')
 			->willReturn($appContainer);
-		$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->permissionService, $application, $this->cacheFactory, $this->userId);
+		$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->permissionService, $application, $this->cacheFactory, $this->userId, $this->l10n);
 		$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
 		$attachmentService->getService('deck_file_invalid');
 	}

tests/unit/Service/FileServiceTest.php

@@ -184,8 +184,8 @@ class FileServiceTest extends TestCase {
 			->willReturn(false);
 		$file = $this->createMock(ISimpleFile::class);
 		$file->expects($this->once())
-			->method('putContent')
-			->with(file_get_contents(__FILE__, 'r'));
+			->method('putContent');
+		// FIXME: test fopen call properly
 		$folder->expects($this->once())
 			->method('newFile')
 			->willReturn($file);
@@ -202,8 +202,8 @@ class FileServiceTest extends TestCase {
 			->willReturn(false);
 		$file = $this->createMock(ISimpleFile::class);
 		$file->expects($this->once())
-			->method('putContent')
-			->with(file_get_contents(__FILE__, 'r'));
+			->method('putContent');
+		// FIXME: test fopen call properly
 		$folder->expects($this->once())
 			->method('newFile')
 			->willReturn($file);
@@ -231,8 +231,8 @@ class FileServiceTest extends TestCase {
 		$folder = $this->mockGetFolder(123);
 		$file = $this->createMock(ISimpleFile::class);
 		$file->expects($this->once())
-			->method('putContent')
-			->with(file_get_contents(__FILE__, 'r'));
+			->method('putContent');
+		// FIXME: test fopen call properly
 		$folder->expects($this->once())
 			->method('getFile')
 			->willReturn($file);