diff --git a/app/Exports/Controllers/ImportController.php b/app/Exports/Controllers/ImportController.php index 4d2c83090..d8dceed2f 100644 --- a/app/Exports/Controllers/ImportController.php +++ b/app/Exports/Controllers/ImportController.php @@ -4,7 +4,7 @@ declare(strict_types=1); namespace BookStack\Exports\Controllers; -use BookStack\Activity\ActivityType; +use BookStack\Exceptions\ZipImportException; use BookStack\Exceptions\ZipValidationException; use BookStack\Exports\ImportRepo; use BookStack\Http\Controller; @@ -48,12 +48,9 @@ class ImportController extends Controller try { $import = $this->imports->storeFromUpload($file); } catch (ZipValidationException $exception) { - session()->flash('validation_errors', $exception->errors); - return redirect('/import'); + return redirect('/import')->with('validation_errors', $exception->errors); } - $this->logActivity(ActivityType::IMPORT_CREATE, $import); - return redirect($import->getUrl()); } @@ -80,20 +77,20 @@ class ImportController extends Controller $parent = null; if ($import->type === 'page' || $import->type === 'chapter') { + session()->setPreviousUrl($import->getUrl()); $data = $this->validate($request, [ - 'parent' => ['required', 'string'] + 'parent' => ['required', 'string'], ]); $parent = $data['parent']; } - $entity = $this->imports->runImport($import, $parent); - if ($entity) { - $this->logActivity(ActivityType::IMPORT_RUN, $import); - return redirect($entity->getUrl()); + try { + $entity = $this->imports->runImport($import, $parent); + } catch (ZipImportException $exception) { + return redirect($import->getUrl())->with('import_errors', $exception->errors); } - // TODO - Redirect to result - // TODO - Or redirect back with errors - return 'failed'; + + return redirect($entity->getUrl()); } /** @@ -104,8 +101,6 @@ class ImportController extends Controller $import = $this->imports->findVisible($id); $this->imports->deleteImport($import); - $this->logActivity(ActivityType::IMPORT_DELETE, $import); - return redirect('/import'); } } diff --git a/app/Exports/ImportRepo.php b/app/Exports/ImportRepo.php index d169d4845..f72386c47 100644 --- a/app/Exports/ImportRepo.php +++ b/app/Exports/ImportRepo.php @@ -2,6 +2,7 @@ namespace BookStack\Exports; +use BookStack\Activity\ActivityType; use BookStack\Entities\Models\Entity; use BookStack\Entities\Queries\EntityQueries; use BookStack\Exceptions\FileUploadException; @@ -14,8 +15,10 @@ use BookStack\Exports\ZipExports\Models\ZipExportPage; use BookStack\Exports\ZipExports\ZipExportReader; use BookStack\Exports\ZipExports\ZipExportValidator; use BookStack\Exports\ZipExports\ZipImportRunner; +use BookStack\Facades\Activity; use BookStack\Uploads\FileStorage; use Illuminate\Database\Eloquent\Collection; +use Illuminate\Support\Facades\DB; use Symfony\Component\HttpFoundation\File\UploadedFile; class ImportRepo @@ -93,25 +96,42 @@ class ImportRepo $import->path = $path; $import->save(); + Activity::add(ActivityType::IMPORT_CREATE, $import); + return $import; } /** - * @throws ZipValidationException|ZipImportException + * @throws ZipImportException */ - public function runImport(Import $import, ?string $parent = null): ?Entity + public function runImport(Import $import, ?string $parent = null): Entity { $parentModel = null; if ($import->type === 'page' || $import->type === 'chapter') { $parentModel = $parent ? $this->entityQueries->findVisibleByStringIdentifier($parent) : null; } - return $this->importer->run($import, $parentModel); + DB::beginTransaction(); + try { + $model = $this->importer->run($import, $parentModel); + } catch (ZipImportException $e) { + DB::rollBack(); + $this->importer->revertStoredFiles(); + throw $e; + } + + DB::commit(); + $this->deleteImport($import); + Activity::add(ActivityType::IMPORT_RUN, $import); + + return $model; } public function deleteImport(Import $import): void { $this->storage->delete($import->path); $import->delete(); + + Activity::add(ActivityType::IMPORT_DELETE, $import); } } diff --git a/app/Exports/ZipExports/ZipImportReferences.php b/app/Exports/ZipExports/ZipImportReferences.php index 3bce16bbb..b23d5e72b 100644 --- a/app/Exports/ZipExports/ZipImportReferences.php +++ b/app/Exports/ZipExports/ZipImportReferences.php @@ -139,4 +139,21 @@ class ZipImportReferences ]); } } + + + /** + * @return Image[] + */ + public function images(): array + { + return $this->images; + } + + /** + * @return Attachment[] + */ + public function attachments(): array + { + return $this->attachments; + } } diff --git a/app/Exports/ZipExports/ZipImportRunner.php b/app/Exports/ZipExports/ZipImportRunner.php index 9f19f03e2..c5b9da319 100644 --- a/app/Exports/ZipExports/ZipImportRunner.php +++ b/app/Exports/ZipExports/ZipImportRunner.php @@ -47,14 +47,17 @@ class ZipImportRunner * Returns the top-level entity item which was imported. * @throws ZipImportException */ - public function run(Import $import, ?Entity $parent = null): ?Entity + public function run(Import $import, ?Entity $parent = null): Entity { $zipPath = $this->getZipPath($import); $reader = new ZipExportReader($zipPath); $errors = (new ZipExportValidator($reader))->validate(); if ($errors) { - throw new ZipImportException(["ZIP failed to validate"]); + throw new ZipImportException([ + trans('errors.import_validation_failed'), + ...$errors, + ]); } try { @@ -65,15 +68,14 @@ class ZipImportRunner // Validate parent type if ($exportModel instanceof ZipExportBook && ($parent !== null)) { - throw new ZipImportException(["Must not have a parent set for a Book import"]); - } else if ($exportModel instanceof ZipExportChapter && (!$parent instanceof Book)) { - throw new ZipImportException(["Parent book required for chapter import"]); + throw new ZipImportException(["Must not have a parent set for a Book import."]); + } else if ($exportModel instanceof ZipExportChapter && !($parent instanceof Book)) { + throw new ZipImportException(["Parent book required for chapter import."]); } else if ($exportModel instanceof ZipExportPage && !($parent instanceof Book || $parent instanceof Chapter)) { - throw new ZipImportException(["Parent book or chapter required for page import"]); + throw new ZipImportException(["Parent book or chapter required for page import."]); } - $this->ensurePermissionsPermitImport($exportModel); - $entity = null; + $this->ensurePermissionsPermitImport($exportModel, $parent); if ($exportModel instanceof ZipExportBook) { $entity = $this->importBook($exportModel, $reader); @@ -81,32 +83,46 @@ class ZipImportRunner $entity = $this->importChapter($exportModel, $parent, $reader); } else if ($exportModel instanceof ZipExportPage) { $entity = $this->importPage($exportModel, $parent, $reader); + } else { + throw new ZipImportException(['No importable data found in import data.']); } - // TODO - In transaction? - // TODO - Revert uploaded files if goes wrong - // TODO - Attachments - // TODO - Images - // (Both listed/stored in references) - $this->references->replaceReferences(); $reader->close(); $this->cleanup(); - dd('stop'); - - // TODO - Delete import/zip after import? - // Do this in parent repo? - return $entity; } - protected function cleanup() + /** + * Revert any files which have been stored during this import process. + * Considers files only, and avoids the database under the + * assumption that the database may already have been + * reverted as part of a transaction rollback. + */ + public function revertStoredFiles(): void + { + foreach ($this->references->images() as $image) { + $this->imageService->destroyFileAtPath($image->type, $image->path); + } + + foreach ($this->references->attachments() as $attachment) { + if (!$attachment->external) { + $this->attachmentService->deleteFileInStorage($attachment); + } + } + + $this->cleanup(); + } + + protected function cleanup(): void { foreach ($this->tempFilesToCleanup as $file) { unlink($file); } + + $this->tempFilesToCleanup = []; } protected function importBook(ZipExportBook $exportBook, ZipExportReader $reader): Book @@ -256,9 +272,6 @@ class ZipImportRunner { $errors = []; - // TODO - Extract messages to language files - // TODO - Ensure these are shown to users on failure - $chapters = []; $pages = []; $images = []; @@ -266,7 +279,7 @@ class ZipImportRunner if ($exportModel instanceof ZipExportBook) { if (!userCan('book-create-all')) { - $errors[] = 'You are lacking the required permission to create books.'; + $errors[] = trans('errors.import_perms_books'); } array_push($pages, ...$exportModel->pages); array_push($chapters, ...$exportModel->chapters); @@ -283,7 +296,7 @@ class ZipImportRunner if (count($chapters) > 0) { $permission = 'chapter-create' . ($parent ? '' : '-all'); if (!userCan($permission, $parent)) { - $errors[] = 'You are lacking the required permission to create chapters.'; + $errors[] = trans('errors.import_perms_chapters'); } } @@ -295,25 +308,25 @@ class ZipImportRunner if (count($pages) > 0) { if ($parent) { if (!userCan('page-create', $parent)) { - $errors[] = 'You are lacking the required permission to create pages.'; + $errors[] = trans('errors.import_perms_pages'); } } else { $hasPermission = userCan('page-create-all') || userCan('page-create-own'); if (!$hasPermission) { - $errors[] = 'You are lacking the required permission to create pages.'; + $errors[] = trans('errors.import_perms_pages'); } } } if (count($images) > 0) { if (!userCan('image-create-all')) { - $errors[] = 'You are lacking the required permissions to create images.'; + $errors[] = trans('errors.import_perms_images'); } } if (count($attachments) > 0) { if (!userCan('attachment-create-all')) { - $errors[] = 'You are lacking the required permissions to create attachments.'; + $errors[] = trans('errors.import_perms_attachments'); } } diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php index fa53c4ae4..033f23341 100644 --- a/app/Uploads/AttachmentService.php +++ b/app/Uploads/AttachmentService.php @@ -151,7 +151,7 @@ class AttachmentService * Delete a file from the filesystem it sits on. * Cleans any empty leftover folders. */ - protected function deleteFileInStorage(Attachment $attachment): void + public function deleteFileInStorage(Attachment $attachment): void { $this->storage->delete($attachment->path); } diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index e501cc7b1..5c455cf86 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -153,11 +153,19 @@ class ImageService */ public function destroy(Image $image): void { - $disk = $this->storage->getDisk($image->type); - $disk->destroyAllMatchingNameFromPath($image->path); + $this->destroyFileAtPath($image->type, $image->path); $image->delete(); } + /** + * Destroy the underlying image file at the given path. + */ + public function destroyFileAtPath(string $type, string $path): void + { + $disk = $this->storage->getDisk($type); + $disk->destroyAllMatchingNameFromPath($path); + } + /** * Delete gallery and drawings that are not within HTML content of pages or page revisions. * Checks based off of only the image name. diff --git a/lang/en/entities.php b/lang/en/entities.php index ae1c1e8d4..26a563a7e 100644 --- a/lang/en/entities.php +++ b/lang/en/entities.php @@ -52,6 +52,7 @@ return [ 'import_pending_none' => 'No imports have been started.', 'import_continue' => 'Continue Import', 'import_continue_desc' => 'Review the content due to be imported from the uploaded ZIP file. When ready, run the import to add its contents to this system. The uploaded ZIP import file will be automatically removed on successful import.', + 'import_details' => 'Import Details', 'import_run' => 'Run Import', 'import_size' => ':size Import ZIP Size', 'import_uploaded_at' => 'Uploaded :relativeTime', @@ -60,6 +61,8 @@ return [ 'import_location_desc' => 'Select a target location for your imported content. You\'ll need the relevant permissions to create within the location you choose.', 'import_delete_confirm' => 'Are you sure you want to delete this import?', 'import_delete_desc' => 'This will delete the uploaded import ZIP file, and cannot be undone.', + 'import_errors' => 'Import Errors', + 'import_errors_desc' => 'The follow errors occurred during the import attempt:', // Permissions and restrictions 'permissions' => 'Permissions', diff --git a/lang/en/errors.php b/lang/en/errors.php index 3f2f30331..ced80a32c 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -109,6 +109,12 @@ return [ 'import_zip_cant_read' => 'Could not read ZIP file.', 'import_zip_cant_decode_data' => 'Could not find and decode ZIP data.json content.', 'import_zip_no_data' => 'ZIP file data has no expected book, chapter or page content.', + 'import_validation_failed' => 'Import ZIP failed to validate with errors:', + 'import_perms_books' => 'You are lacking the required permissions to create books.', + 'import_perms_chapters' => 'You are lacking the required permissions to create chapters.', + 'import_perms_pages' => 'You are lacking the required permissions to create pages.', + 'import_perms_images' => 'You are lacking the required permissions to create images.', + 'import_perms_attachments' => 'You are lacking the required permission to create attachments.', // API errors 'api_no_authorization_found' => 'No authorization token found on the request', diff --git a/resources/views/exports/import-show.blade.php b/resources/views/exports/import-show.blade.php index 40867377f..e4f199aa2 100644 --- a/resources/views/exports/import-show.blade.php +++ b/resources/views/exports/import-show.blade.php @@ -7,8 +7,19 @@
{{ trans('entities.import_continue_desc') }}
+ @if(session()->has('import_errors')) +{{ trans('entities.import_errors_desc') }}
+ @foreach(session()->get('import_errors') ?? [] as $error) +{{ $error }}
+ @endforeach +{{ trans('entities.import_location_desc') }}
+{{ trans('entities.import_location_desc') }}
+ @if($errors->has('parent')) +