diff --git a/css/style.scss b/css/style.scss index 757efa6bc..30e518921 100644 --- a/css/style.scss +++ b/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; } } diff --git a/js/service/FileService.js b/js/service/FileService.js index f1b6eb629..1db2617ba 100644 --- a/js/service/FileService.js +++ b/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); \ No newline at end of file diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 1836249e1..a2bbcbc0a 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -32,9 +32,11 @@ 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 @@ public function create($cardId, $type, $data) { } 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 diff --git a/lib/Service/FileService.php b/lib/Service/FileService.php index bf2553ac4..50b71f6fd 100644 --- a/lib/Service/FileService.php +++ b/lib/Service/FileService.php @@ -25,9 +25,11 @@ 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 @@ public function extendData(Attachment $attachment) { return $attachment; } + /** + * @return array + * @throws StatusException + */ private function getUploadedFile () { $file = $this->request->getUploadedFile('file'); $error = null; @@ -115,13 +121,13 @@ private function getUploadedFile () { ]; 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 @@ public function create(Attachment $attachment) { $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 @@ public function update(Attachment $attachment) { $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()); } diff --git a/templates/part.card.php b/templates/part.card.php index 30165cd2a..1b8a4920a 100644 --- a/templates/part.card.php +++ b/templates/part.card.php @@ -104,6 +104,7 @@
+
{{ fileservice.status.error }}
{{ fileservice.status.message }}
diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index 5346d7543..fc387de21 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -34,6 +34,7 @@ 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 @@ public function setUp() { ->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 @@ public function testRegisterAttachmentService() { $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 @@ public function testRegisterAttachmentServiceNotExisting() { $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'); } diff --git a/tests/unit/Service/FileServiceTest.php b/tests/unit/Service/FileServiceTest.php index 3885fbc06..39dc9fcec 100644 --- a/tests/unit/Service/FileServiceTest.php +++ b/tests/unit/Service/FileServiceTest.php @@ -184,8 +184,8 @@ public function testCreate() { ->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 @@ public function testCreateNoFolder() { ->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 @@ public function testUpdate() { $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);