From 280271718ba2b16b4dfd1266ec66f537a85f61df Mon Sep 17 00:00:00 2001 From: David Bomba Date: Fri, 6 Mar 2020 07:30:32 +1100 Subject: [PATCH] Migration improvements: (#3428) - Refactored exceptions - Changed failed.blade.php - Removed report() method from exceptions - Added new force flag for MigrationController.php --- app/Exceptions/MigrationValidatorFailed.php | 10 +---- app/Exceptions/NonExistingMigrationFile.php | 10 +---- .../ProcessingMigrationArchiveFailed.php | 19 +-------- app/Exceptions/ResourceDependencyMissing.php | 15 +------ .../ResourceNotAvailableForMigration.php | 16 +------- app/Http/Controllers/MigrationController.php | 6 +-- app/Jobs/Util/Import.php | 39 +++++++++++-------- app/Jobs/Util/StartMigration.php | 13 +++---- app/Mail/MigrationFailed.php | 7 ++-- .../2020_03_05_123315_create_jobs_table.php | 36 +++++++++++++++++ .../views/email/migration/failed.blade.php | 4 +- tests/Unit/Migration/ImportTest.php | 17 ++++++++ 12 files changed, 94 insertions(+), 98 deletions(-) create mode 100644 database/migrations/2020_03_05_123315_create_jobs_table.php diff --git a/app/Exceptions/MigrationValidatorFailed.php b/app/Exceptions/MigrationValidatorFailed.php index 2359e815a228..80ef8c5ac482 100644 --- a/app/Exceptions/MigrationValidatorFailed.php +++ b/app/Exceptions/MigrationValidatorFailed.php @@ -7,13 +7,5 @@ use Throwable; class MigrationValidatorFailed extends Exception { - public function __construct($message = "", $code = 0, Throwable $previous = null) - { - parent::__construct($message, $code, $previous); - } - - public function report() - { - return $this->message; - } + // .. } diff --git a/app/Exceptions/NonExistingMigrationFile.php b/app/Exceptions/NonExistingMigrationFile.php index ad4e60698a18..ad7adbb58f92 100644 --- a/app/Exceptions/NonExistingMigrationFile.php +++ b/app/Exceptions/NonExistingMigrationFile.php @@ -7,13 +7,5 @@ use Throwable; class NonExistingMigrationFile extends Exception { - public function __construct($message = "", $code = 0, Throwable $previous = null) - { - parent::__construct($message, $code, $previous); - } - - public function report() - { - return 'Migration file doesn\'t exist or it is corrupted.'; - } + // .. } diff --git a/app/Exceptions/ProcessingMigrationArchiveFailed.php b/app/Exceptions/ProcessingMigrationArchiveFailed.php index 1fd03a077fa1..da2488954ffa 100644 --- a/app/Exceptions/ProcessingMigrationArchiveFailed.php +++ b/app/Exceptions/ProcessingMigrationArchiveFailed.php @@ -7,22 +7,5 @@ use Throwable; class ProcessingMigrationArchiveFailed extends Exception { - /** - * @var Throwable - */ - private $previous; - - public function __construct($message = "", $code = 0, Throwable $previous = null) - { - parent::__construct($message, $code, $previous); - $this->message = $message; - $this->code = $code; - $this->previous = $previous; - } - - public function report() - { - return 'Unable to open migration archive.'; - } - + // .. } diff --git a/app/Exceptions/ResourceDependencyMissing.php b/app/Exceptions/ResourceDependencyMissing.php index b775abe044c0..a265dc048cb0 100644 --- a/app/Exceptions/ResourceDependencyMissing.php +++ b/app/Exceptions/ResourceDependencyMissing.php @@ -7,18 +7,5 @@ use Throwable; class ResourceDependencyMissing extends Exception { - private $resource; - private $dependency; - - public function __construct($resource, $dependency) - { - parent::__construct(); - $this->resource = $resource; - $this->dependency = $dependency; - } - - public function report() - { - return "Resource '{$this->resource}' depends on '{$this->dependency}'."; - } + // .. } diff --git a/app/Exceptions/ResourceNotAvailableForMigration.php b/app/Exceptions/ResourceNotAvailableForMigration.php index 8b8f3b306292..0e53111d547b 100644 --- a/app/Exceptions/ResourceNotAvailableForMigration.php +++ b/app/Exceptions/ResourceNotAvailableForMigration.php @@ -7,19 +7,5 @@ use Throwable; class ResourceNotAvailableForMigration extends Exception { - private $resource; - - public function __construct($resource) - { - parent::__construct(); - - $this->resource = $resource; - } - - public function report() - { - // TODO: Handle this nicely, throw response, etc. - return "Resource {$this->resource} is not available for the migration."; - } - + // .. } diff --git a/app/Http/Controllers/MigrationController.php b/app/Http/Controllers/MigrationController.php index 4bf3ddefdd12..bb677b0ce87d 100644 --- a/app/Http/Controllers/MigrationController.php +++ b/app/Http/Controllers/MigrationController.php @@ -15,6 +15,7 @@ use App\Exceptions\MigrationValidatorFailed; use App\Exceptions\NonExistingMigrationFile; use App\Exceptions\ProcessingMigrationArchiveFailed; use App\Exceptions\ResourceNotAvailableForMigration; +use App\Factory\CompanyFactory; use App\Http\Requests\Account\CreateAccountRequest; use App\Http\Requests\Migration\UploadMigrationFileRequest; use App\Jobs\Account\CreateAccount; @@ -192,7 +193,7 @@ class MigrationController extends BaseController */ public function startMigration(Request $request, Company $company) { - if ($request->has('force')) + if ($request->has('force') && !empty($request->force)) $this->purgeCompany($company); $migration_file = $request->file('migration') @@ -201,12 +202,11 @@ class MigrationController extends BaseController if (app()->environment() == 'testing') return; $user = auth()->user(); - $company = $company; StartMigration::dispatch($migration_file, $user, $company); return response()->json([ - '_id' => Str::uuid(), + '_id' => Str::uuid(), 'method' => config('queue.default'), 'started_at' => now(), ], 200); diff --git a/app/Jobs/Util/Import.php b/app/Jobs/Util/Import.php index 8f6e0c4e9a3b..2e01f4d81256 100644 --- a/app/Jobs/Util/Import.php +++ b/app/Jobs/Util/Import.php @@ -131,14 +131,19 @@ class Import implements ShouldQueue foreach ($this->data as $key => $resource) { if (!in_array($key, $this->available_imports)) { - throw new ResourceNotAvailableForMigration($key); + throw new ResourceNotAvailableForMigration("Resource {$key} is not available for migration."); } -\Log::error($key); + + \Log::error($key); $method = sprintf("process%s", Str::ucfirst(Str::camel($key))); $this->{$method}($resource); + + info("$key done!!"); } + + info('CompletedπŸš€πŸš€πŸš€πŸš€πŸš€ at ' . now()); } /** @@ -155,7 +160,7 @@ class Import implements ShouldQueue if ($validator->fails()) { // \Log::error($validator->errors()); - throw new MigrationValidatorFailed($validator->errors()); + throw new MigrationValidatorFailed(json_encode($validator->errors())); } if (isset($data['account_id'])) @@ -185,7 +190,7 @@ class Import implements ShouldQueue $validator = Validator::make($data, $rules); if ($validator->fails()) { - throw new MigrationValidatorFailed($validator->errors()); + throw new MigrationValidatorFailed(json_encode($validator->errors())); } foreach ($data as $resource) { @@ -231,7 +236,7 @@ class Import implements ShouldQueue $validator = Validator::make($data, $rules); if ($validator->fails()) { - throw new MigrationValidatorFailed($validator->errors()); + throw new MigrationValidatorFailed(json_encode($validator->errors())); } $user_repository = new UserRepository(); @@ -324,7 +329,7 @@ class Import implements ShouldQueue $validator = Validator::make($data, $rules); if ($validator->fails()) { - throw new MigrationValidatorFailed($validator->errors()); + throw new MigrationValidatorFailed(json_encode($validator->errors())); } $product_repository = new ProductRepository(); @@ -358,7 +363,7 @@ class Import implements ShouldQueue $validator = Validator::make($data, $rules); if ($validator->fails()) { - throw new MigrationValidatorFailed($validator->errors()); + throw new MigrationValidatorFailed(json_encode($validator->errors())); } $invoice_repository = new InvoiceRepository(); @@ -368,7 +373,7 @@ class Import implements ShouldQueue $modified = $resource; if (array_key_exists('client_id', $resource) && !array_key_exists('clients', $this->ids)) { - throw new ResourceDependencyMissing(array_key_first($data), 'clients'); + throw new ResourceDependencyMissing('Processing invoices failed, because of missing dependency - clients.'); } $modified['client_id'] = $this->transformId('clients', $resource['client_id']); @@ -405,7 +410,7 @@ class Import implements ShouldQueue $validator = Validator::make($data, $rules); if ($validator->fails()) { - throw new MigrationValidatorFailed($validator->errors()); + throw new MigrationValidatorFailed(json_encode($validator->errors())); } $credit_repository = new CreditRepository(); @@ -415,7 +420,7 @@ class Import implements ShouldQueue $modified = $resource; if (array_key_exists('client_id', $resource) && !array_key_exists('clients', $this->ids)) { - throw new ResourceDependencyMissing(array_key_first($data), 'clients'); + throw new ResourceDependencyMissing('Processing credits failed, because of missing dependency - clients.'); } $modified['client_id'] = $this->transformId('clients', $resource['client_id']); @@ -451,7 +456,7 @@ class Import implements ShouldQueue $validator = Validator::make($data, $rules); if ($validator->fails()) { - throw new MigrationValidatorFailed($validator->errors()); + throw new MigrationValidatorFailed(json_encode($validator->errors())); } $quote_repository = new QuoteRepository(); @@ -461,7 +466,7 @@ class Import implements ShouldQueue $modified = $resource; if (array_key_exists('client_id', $resource) && !array_key_exists('clients', $this->ids)) { - throw new ResourceDependencyMissing(array_key_first($data), 'clients'); + throw new ResourceDependencyMissing('Processing quotes failed, because of missing dependency - clients.'); } $modified['client_id'] = $this->transformId('clients', $resource['client_id']); @@ -502,7 +507,7 @@ class Import implements ShouldQueue $validator = Validator::make($data, $rules); if ($validator->fails()) { - throw new MigrationValidatorFailed($validator->errors()); + throw new MigrationValidatorFailed(json_encode($validator->errors())); } $payment_repository = new PaymentRepository(new CreditRepository()); @@ -512,7 +517,7 @@ class Import implements ShouldQueue $modified = $resource; if (array_key_exists('client_id', $resource) && !array_key_exists('clients', $this->ids)) { - throw new ResourceDependencyMissing(array_key_first($data), 'clients'); + throw new ResourceDependencyMissing('Processing payments failed, because of missing dependency - clients.'); } $modified['client_id'] = $this->transformId('clients', $resource['client_id']); @@ -557,11 +562,11 @@ class Import implements ShouldQueue $modified = $resource; if (array_key_exists('invoice_id', $resource) && $resource['invoice_id'] && !array_key_exists('invoices', $this->ids)) { - throw new ResourceDependencyMissing(array_key_first($data), 'invoices'); + throw new ResourceDependencyMissing('Processing documents failed, because of missing dependency - invoices.'); } if (array_key_exists('expense_id', $resource) && $resource['expense_id'] && !array_key_exists('expenses', $this->ids)) { - throw new ResourceDependencyMissing(array_key_first($data), 'expenses'); + throw new ResourceDependencyMissing('Processing documents failed, because of missing dependency - expenses.'); } /** Remove because of polymorphic joins. */ @@ -611,7 +616,7 @@ class Import implements ShouldQueue $validator = Validator::make($data, $rules); if ($validator->fails()) { - throw new MigrationValidatorFailed($validator->errors()); + throw new MigrationValidatorFailed(json_encode($validator->errors())); } foreach ($data as $resource) { diff --git a/app/Jobs/Util/StartMigration.php b/app/Jobs/Util/StartMigration.php index 50a5c14797ef..4e7e83fcd2a3 100644 --- a/app/Jobs/Util/StartMigration.php +++ b/app/Jobs/Util/StartMigration.php @@ -63,7 +63,7 @@ class StartMigration implements ShouldQueue auth()->login($this->user, false); auth()->user()->setCompany($this->company); - + $zip = new \ZipArchive(); $archive = $zip->open($this->filepath); @@ -71,7 +71,7 @@ class StartMigration implements ShouldQueue try { if (!$archive) - throw new ProcessingMigrationArchiveFailed(); + throw new ProcessingMigrationArchiveFailed('Processing migration archive failed. Migration file is possibly corrupted.'); $zip->extractTo(storage_path("migrations/{$filename}")); $zip->close(); @@ -81,10 +81,9 @@ class StartMigration implements ShouldQueue $this->start($filename); } catch (NonExistingMigrationFile | ProcessingMigrationArchiveFailed | ResourceNotAvailableForMigration | MigrationValidatorFailed | ResourceDependencyMissing $e) { - Mail::to($this->user) - ->send(new MigrationFailed($e, $e->getMessage())); - - if(app()->environment() !== 'production') info($e->getMessage()); + Mail::to($this->user)->send(new MigrationFailed($e, $e->getMessage())); + + if (app()->environment() !== 'production') info($e->getMessage()); } } @@ -98,7 +97,7 @@ class StartMigration implements ShouldQueue $file = storage_path("migrations/$filename/migration.json"); if (!file_exists($file)) - throw new NonExistingMigrationFile(); + throw new NonExistingMigrationFile('Migration file does not exist, or it is corrupted.'); $handle = fopen($file, "r"); $file = fread($handle, filesize($file)); diff --git a/app/Mail/MigrationFailed.php b/app/Mail/MigrationFailed.php index d9843c5a9a25..71bb53cfac9e 100644 --- a/app/Mail/MigrationFailed.php +++ b/app/Mail/MigrationFailed.php @@ -11,18 +11,17 @@ class MigrationFailed extends Mailable use Queueable, SerializesModels; public $exception; - public $message; + public $content; /** * Create a new message instance. * - * @param $message + * @param $content * @param $exception */ - public function __construct($exception, $message = null) + public function __construct($exception, $content = null) { $this->exception = $exception; - $this->message = 'Oops, looks like something went wrong with your migration. Please try again, later.'; } /** diff --git a/database/migrations/2020_03_05_123315_create_jobs_table.php b/database/migrations/2020_03_05_123315_create_jobs_table.php new file mode 100644 index 000000000000..1be9e8a80eb1 --- /dev/null +++ b/database/migrations/2020_03_05_123315_create_jobs_table.php @@ -0,0 +1,36 @@ +bigIncrements('id'); + $table->string('queue')->index(); + $table->longText('payload'); + $table->unsignedTinyInteger('attempts'); + $table->unsignedInteger('reserved_at')->nullable(); + $table->unsignedInteger('available_at'); + $table->unsignedInteger('created_at'); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::dropIfExists('jobs'); + } +} diff --git a/resources/views/email/migration/failed.blade.php b/resources/views/email/migration/failed.blade.php index a00259cb11ae..780f2051fa02 100644 --- a/resources/views/email/migration/failed.blade.php +++ b/resources/views/email/migration/failed.blade.php @@ -2,6 +2,6 @@ Whoops! Looks like your migration failed. 
-    {!! $exception->report() !!}
-    {!! $message !!}
+    {!! $exception->getMessage() !!}
+    {!! $content !!}
diff --git a/tests/Unit/Migration/ImportTest.php b/tests/Unit/Migration/ImportTest.php index 8f319862893c..6b56b96cd4b4 100644 --- a/tests/Unit/Migration/ImportTest.php +++ b/tests/Unit/Migration/ImportTest.php @@ -504,4 +504,21 @@ class ImportTest extends TestCase $this->assertTrue(true, 'Documents importing not completed yet. Missing expenses.'); } + + public function testExceptionMailSending() + { + Mail::fake(); + + $data['panda_bears'] = [ + 'name' => 'Awesome Panda Bear', + ]; + + try { + Import::dispatchNow($data, $this->company, $this->user); + } + catch (ResourceNotAvailableForMigration $e) { + Mail::to($this->user)->send(new MigrationFailed($e, $e->getMessage())); + $this->assertTrue(true); + } + } }