(V2) Improve validation & error handling (#3414)

* wip - migration transfer

* (WIP) Response refactor:
- Catching exceptions at top level
- Tests refactor

* wip

* Wrappign migration validator:
- Migration dropped to queue
- New validator messages
- New exception messages

* Fixes for tests

app/Exceptions/MigrationValidatorFailed.php

@@ -14,6 +14,6 @@ class MigrationValidatorFailed extends Exception
 
     public function report()
     {
-        // Send, an e-mail & notify users.
+        return $this->message;
     }
 }

app/Exceptions/NonExistingMigrationFile.php

@@ -0,0 +1,19 @@
+<?php
+
+namespace App\Exceptions;
+
+use Exception;
+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.';
+    }
+}

app/Http/Controllers/MigrationController.php

@@ -11,10 +11,15 @@
 
 namespace App\Http\Controllers;
 
+use App\Exceptions\MigrationValidatorFailed;
+use App\Exceptions\NonExistingMigrationFile;
+use App\Exceptions\ProcessingMigrationArchiveFailed;
+use App\Exceptions\ResourceNotAvailableForMigration;
 use App\Http\Requests\Account\CreateAccountRequest;
 use App\Http\Requests\Migration\UploadMigrationFileRequest;
 use App\Jobs\Account\CreateAccount;
 use App\Jobs\Util\StartMigration;
+use App\Mail\MigrationFailed;
 use App\Models\Account;
 use App\Models\Company;
 use App\Models\CompanyUser;
@@ -23,6 +28,11 @@ use App\Transformers\CompanyUserTransformer;
 use Illuminate\Foundation\Bus\DispatchesJobs;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Log;
+use Illuminate\Support\Facades\Mail;
+use Illuminate\Support\Str;
+use function GuzzleHttp\Promise\queue;
+use App\Models\User;
+use Illuminate\Support\Facades\Artisan;
 
 class MigrationController extends BaseController
 {
@@ -69,7 +79,6 @@ class MigrationController extends BaseController
      *          response=422,
      *          description="Validation error",
      *          @OA\JsonContent(ref="#/components/schemas/ValidationError"),
-
      *       ),
      *       @OA\Response(
      *           response="default",
@@ -82,11 +91,10 @@ class MigrationController extends BaseController
     {
         $company->delete();
 
-        return response()->json(['message'=>'Company purged'], 200);
+        return response()->json(['message' => 'Company purged'], 200);
     }
 
 
-
     /**
      *
      * Purge Company but save settings
@@ -122,7 +130,6 @@ class MigrationController extends BaseController
      *          response=422,
      *          description="Validation error",
      *          @OA\JsonContent(ref="#/components/schemas/ValidationError"),
-
      *       ),
      *       @OA\Response(
      *           response="default",
@@ -136,7 +143,7 @@ class MigrationController extends BaseController
         $company->client->delete();
         $company->save();
 
-        return response()->json(['message'=>'Settings preserved'], 200);
+        return response()->json(['message' => 'Settings preserved'], 200);
     }
 
     /**
@@ -152,7 +159,7 @@ class MigrationController extends BaseController
      *      @OA\Parameter(ref="#/components/parameters/X-Api-Secret"),
      *      @OA\Parameter(ref="#/components/parameters/X-Api-Token"),
      *      @OA\Parameter(ref="#/components/parameters/X-Requested-With"),
-     *      @OA\Parameter(ref="#/components/parameters/X-Api-Password"), 
+     *      @OA\Parameter(ref="#/components/parameters/X-Api-Password"),
      *      @OA\Parameter(
      *          name="migration",
      *          in="path",
@@ -175,7 +182,6 @@ class MigrationController extends BaseController
      *          response=422,
      *          description="Validation error",
      *          @OA\JsonContent(ref="#/components/schemas/ValidationError"),
-
      *       ),
      *       @OA\Response(
      *           response="default",
@@ -186,13 +192,23 @@ class MigrationController extends BaseController
      */
     public function startMigration(Request $request, Company $company)
     {
-        if($request->has('force'))
+        if ($request->has('force'))
             $this->purgeCompany($company);
 
-        if(app()->environment() !== 'testing') {
+        $migration_file = $request->file('migration')
-            StartMigration::dispatchNow($request->file('migration'), auth()->user(), $company);
+            ->storeAs('migrations', $request->file('migration')->getClientOriginalName());
-        }
 
-        return response()->json([], 200);
+        if (app()->environment() == 'testing') return;
+
+        $user = auth()->user();
+        $company = $company;
+
+        StartMigration::dispatch($migration_file, $user, $company);
+
+        return response()->json([
+            '_id' => Str::uuid(),   
+            'method' => config('queue.default'),
+            'started_at' => now(),
+        ], 200);
     }
 }

app/Jobs/Util/Import.php

@@ -69,15 +69,15 @@ class Import implements ShouldQueue
      * @var array
      */
     private $available_imports = [
-        'company', 
+        'company',
-        'users', 
+        'users',
-        'tax_rates', 
+        'tax_rates',
-        'clients', 
+        'clients',
-        'products', 
+        'products',
-        'invoices', 
+        'invoices',
-        'quotes', 
+        'quotes',
-        'payments', 
+        'payments',
-        'credits', 
+        'credits',
         'company_gateways',
         'documents',
         'client_gateway_tokens',
@@ -126,24 +126,15 @@ class Import implements ShouldQueue
      */
     public function handle()
     {
-    
+        foreach ($this->data as $key => $resource) {
-        try {
-            foreach ($this->data as $key => $resource) {
 
-                if (!in_array($key, $this->available_imports)) {
+            if (!in_array($key, $this->available_imports)) {
-                    throw new ResourceNotAvailableForMigration($key);
+                throw new ResourceNotAvailableForMigration($key);
-                }
-
-                $method = sprintf("process%s", Str::ucfirst(Str::camel($key)));
-
-                $this->{$method}($resource);
             }
-        } catch (ResourceNotAvailableForMigration $e) {
+
-            Mail::to($this->user)->send(new MigrationFailed($e));
+            $method = sprintf("process%s", Str::ucfirst(Str::camel($key)));
-        } catch (MigrationValidatorFailed $e) {
+
-            Mail::to($this->user)->send(new MigrationFailed($e));
+            $this->{$method}($resource);
-        } catch (ResourceDependencyMissing $e) {
-            Mail::to($this->user)->send(new MigrationFailed($e));
         }
     }
 
@@ -297,7 +288,7 @@ class Import implements ShouldQueue
                     $modified_contacts[$key]['company_id'] = $this->company->id;
                     $modified_contacts[$key]['user_id'] = $this->processUserId($resource);
                     $modified_contacts[$key]['client_id'] = $client->id;
-                    $modified_contacts[$key]['password'] = 'mysuperpassword'; // @todo, and clean up the code.. 
+                    $modified_contacts[$key]['password'] = 'mysuperpassword'; // @todo, and clean up the code..
                     unset($modified_contacts[$key]['id']);
                 }
 
@@ -482,7 +473,7 @@ class Import implements ShouldQueue
             );
 
             $old_user_key = array_key_exists('user_id', $resource) ?? $this->user->id;
-            
+
             $key = "invoices_{$resource['id']}";
 
             $this->ids['quotes'][$key] = [
@@ -568,14 +559,14 @@ class Import implements ShouldQueue
             if (array_key_exists('expense_id', $resource) && $resource['expense_id'] && !array_key_exists('expenses', $this->ids)) {
                 throw new ResourceDependencyMissing(array_key_first($data), 'expenses');
             }
-            
+
             /** Remove because of polymorphic joins. */
             unset($modified['invoice_id']);
             unset($modified['expense_id']);
 
             if(array_key_exists('invoice_id', $resource) && $resource['invoice_id'] && array_key_exists('invoices', $this->ids)) {
-                $modified['documentable_id'] = $this->transformId('invoices', $resource['invoice_id']);  
+                $modified['documentable_id'] = $this->transformId('invoices', $resource['invoice_id']);
-                $modified['documentable_type'] = 'App\\Models\\Invoice'; 
+                $modified['documentable_type'] = 'App\\Models\\Invoice';
             }
 
             if(array_key_exists('expense_id', $resource) && $resource['expense_id'] && array_key_exists('expenses', $this->ids)) {
@@ -608,7 +599,7 @@ class Import implements ShouldQueue
             '*.gateway_key' => 'required',
             '*.fees_and_limits' => new ValidCompanyGatewayFeesAndLimitsRule(),
         ];
-        
+
         $validator = Validator::make($data, $rules);
 
         if ($validator->fails()) {
@@ -653,7 +644,7 @@ class Import implements ShouldQueue
         ClientGatewayToken::unguard();
 
         foreach ($data as $resource) {
-            
+
             $modified = $resource;
 
             unset($modified['id']);

app/Jobs/Util/StartMigration.php

@@ -2,6 +2,10 @@
 
 namespace App\Jobs\Util;
 
+use App\Exceptions\MigrationValidatorFailed;
+use App\Exceptions\NonExistingMigrationFile;
+use App\Exceptions\ResourceDependencyMissing;
+use App\Mail\MigrationFailed;
 use App\Models\User;
 use App\Models\Company;
 use App\Libraries\MultiDB;
@@ -11,6 +15,8 @@ use Illuminate\Queue\InteractsWithQueue;
 use Illuminate\Contracts\Queue\ShouldQueue;
 use Illuminate\Foundation\Bus\Dispatchable;
 use App\Exceptions\ProcessingMigrationArchiveFailed;
+use App\Exceptions\ResourceNotAvailableForMigration;
+use Illuminate\Support\Facades\Mail;
 
 class StartMigration implements ShouldQueue
 {
@@ -38,7 +44,7 @@ class StartMigration implements ShouldQueue
      */
     public function __construct($filepath, User $user, Company $company)
     {
-        $this->filepath = $filepath;
+        $this->filepath = base_path("public/storage/$filepath");
         $this->user = $user;
         $this->company = $company;
     }
@@ -47,6 +53,8 @@ class StartMigration implements ShouldQueue
      * Execute the job.
      *
      * @return void
+     * @throws ProcessingMigrationArchiveFailed
+     * @throws NonExistingMigrationFile
      */
     public function handle()
     {
@@ -58,43 +66,39 @@ class StartMigration implements ShouldQueue
         $filename = pathinfo($this->filepath, PATHINFO_FILENAME);
 
         try {
-            if ($archive) {
+            if (!$archive)
-                $zip->extractTo(storage_path("migrations/{$filename}"));
-                $zip->close();
-
-                if (app()->environment() !== 'testing') {
-                    $this->start($filename);
-                }
-            } else {
                 throw new ProcessingMigrationArchiveFailed();
-            }
-        } catch (ProcessingMigrationArchiveFailed $e) {
-            // TODO: Break the code, stop the migration.. send an e-mail.
-        }
 
-        // Rest of the migration..
+            $zip->extractTo(storage_path("migrations/{$filename}"));
+            $zip->close();
+
+            if (app()->environment() == 'testing')
+                return;
+
+            $this->start($filename);
+        } catch (NonExistingMigrationFile | ProcessingMigrationArchiveFailed | ResourceNotAvailableForMigration | MigrationValidatorFailed | ResourceDependencyMissing $e) {
+            Mail::to(auth()->user())->send(new MigrationFailed($e->getMessage()));
+            if(app()->environment() !== 'production') info($e->getMessage());
+        }
     }
 
 
     /**
      * Main method to start the migration.
+     * @throws NonExistingMigrationFile
      */
-    protected function start(string $filename): void
+    public function start(string $filename): void
     {
         $file = storage_path("migrations/$filename/migration.json");
 
         if (!file_exists($file))
-            return;
+            throw new NonExistingMigrationFile();
 
-        try {
+        $handle = fopen($file, "r");
-            $handle = fopen($file, "r");
+        $file = fread($handle, filesize($file));
-            $file = fread($handle, filesize($file));
+        fclose($handle);
-            fclose($handle);
 
-            $data = json_decode($file, 1);
+        $data = json_decode($file, 1);
-            Import::dispatchNow($data, $this->company, $this->user);
+        Import::dispatchNow($data, $this->company, $this->user);
-        } catch (\Exception $e) {
-            info('Migration failed. Handle this.'); // TODO: Handle the failed job.
-        }
     }
 }

app/Mail/MigrationFailed.php

@@ -3,7 +3,6 @@
 namespace App\Mail;
 
 use Illuminate\Bus\Queueable;
-use Illuminate\Contracts\Queue\ShouldQueue;
 use Illuminate\Mail\Mailable;
 use Illuminate\Queue\SerializesModels;
 
@@ -12,15 +11,18 @@ class MigrationFailed extends Mailable
     use Queueable, SerializesModels;
 
     public $exception;
+    public $message;
 
     /**
      * Create a new message instance.
      *
+     * @param $message
      * @param $exception
      */
-    public function __construct($exception)
+    public function __construct($exception, $message = null)
     {
         $this->exception = $exception;
+        $this->message = 'Oops, looks like something went wrong with your migration. Please try again, later.';
     }
 
     /**

composer.json

@@ -49,7 +49,8 @@
         "webpatser/laravel-countries": "dev-master#75992ad",
         "wildbit/postmark-php": "^2.6",
         "yajra/laravel-datatables-html": "^4.0",
-        "yajra/laravel-datatables-oracle": "~9.0"
+        "yajra/laravel-datatables-oracle": "~9.0",
+        "ext-json": "*"
     },
     "require-dev": {
         "ext-json": "*",

database/migrations/2020_03_03_113807_create_jobs_table.php

@@ -0,0 +1,36 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+class CreateJobsTable extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        Schema::create('jobs', function (Blueprint $table) {
+            $table->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');
+    }
+}

resources/views/email/migration/failed.blade.php

@@ -3,4 +3,5 @@ Looks like your migration failed.
 
 <pre>
     {!! $exception->report() !!}
+    {!! $message !!}
 </pre>

tests/Unit/Migration/ImportTest.php

@@ -61,15 +61,16 @@ class ImportTest extends TestCase
 
     public function testExceptionOnUnavailableResource()
     {
-        Mail::fake();
-
         $data['panda_bears'] = [
             'name' => 'Awesome Panda Bear',
         ];
 
-        Import::dispatchNow($data, $this->company, $this->user);
+        try {
-
+            Import::dispatchNow($data, $this->company, $this->user);
-        Mail::assertSent(MigrationFailed::class);
+        }
+        catch (ResourceNotAvailableForMigration $e) {
+            $this->assertTrue(true);
+        }
     }
 
     public function testCompanyUpdating()
@@ -87,8 +88,6 @@ class ImportTest extends TestCase
 
     public function testInvoicesFailsWithoutClient()
     {
-        Mail::fake();
-
         $data['invoices'] = [
             0 => [
                 'client_id' => 1,
@@ -96,14 +95,16 @@ class ImportTest extends TestCase
             ]
         ];
 
-        Import::dispatchNow($data, $this->company, $this->user);
+        try {
-
+            Import::dispatchNow($data, $this->company, $this->user);
-        Mail::assertSent(MigrationFailed::class);
+        } catch(ResourceDependencyMissing $e) {
+            $this->assertTrue(true);
+        }
     }
 
     public function testInvoicesImporting()
     {
-        //$this->makeTestData();
+        $this->makeTestData();
 
         $this->invoice->forceDelete();
         $this->quote->forceDelete();
@@ -117,8 +118,6 @@ class ImportTest extends TestCase
 
     public function testQuotesFailsWithoutClient()
     {
-        Mail::fake();
-
         $data['quotes'] = [
             0 => [
                 'client_id' => 1,
@@ -126,9 +125,11 @@ class ImportTest extends TestCase
             ]
         ];
 
-        Import::dispatchNow($data, $this->company, $this->user);
+        try {
-
+            Import::dispatchNow($data, $this->company, $this->user);
-        Mail::assertSent(MigrationFailed::class);
+        } catch(ResourceDependencyMissing $e) {
+            $this->assertTrue(true);
+        }
     }
 
     public function testImportFileExists()
@@ -143,9 +144,9 @@ class ImportTest extends TestCase
 
     }
 
+
     public function testAllImport()
     {
-
         //$this->makeTestData();
 
         $this->invoice->forceDelete();
@@ -185,22 +186,6 @@ class ImportTest extends TestCase
         $this->assertGreaterThanOrEqual(0, $client->balance);
     }
 
-    public function testInvoiceImporting()
-    {
-        $original_number = Invoice::count();
-
-        $this->invoice->forceDelete();
-        $this->quote->forceDelete();
-        // $migration_file = base_path() . '/tests/Unit/Migration/migration.json';
-
-        // $this->migration_array = json_decode(file_get_contents($migration_file), 1);
-
-        Import::dispatchNow($this->migration_array, $this->company, $this->user);
-
-        $this->assertGreaterThan($original_number, Invoice::count());
-
-    }
-
     // public function testInvoiceAttributes()
     // {
     //     $original_number = Invoice::count();
@@ -276,8 +261,6 @@ class ImportTest extends TestCase
 
     public function testPaymentDependsOnClient()
     {
-        Mail::fake();
-
         $data['payments'] = [
             0 => [
                 'client_id' => 1,
@@ -285,9 +268,11 @@ class ImportTest extends TestCase
             ]
         ];
 
-        Import::dispatchNow($data, $this->company, $this->user);
+        try {
-
+            Import::dispatchNow($data, $this->company, $this->user);
-        Mail::assertSent(MigrationFailed::class);
+        } catch(ResourceDependencyMissing $e) {
+            $this->assertTrue(true);
+        }
     }
 
     public function testQuotesImport()
@@ -460,7 +445,7 @@ class ImportTest extends TestCase
 
         //         if (!$record) {
         //             $differences['documents']['missing'][] = $document['id'];
-        //         }   
+        //         }
         //     }
         // }
 
@@ -481,7 +466,7 @@ class ImportTest extends TestCase
         // $this->migration_array = json_decode(file_get_contents($migration_file), 1);
 
         Import::dispatchNow($this->migration_array, $this->company, $this->user);
-        
+
         $this->assertGreaterThan($original, ClientContact::count());
     }
 
@@ -493,9 +478,9 @@ class ImportTest extends TestCase
         $original = ClientGatewayToken::count();
 
         Import::dispatchNow($this->migration_array, $this->company, $this->user);
-        
+
        // $this->assertGreaterThan($original, ClientGatewayToken::count());
-       // 
+       //
                 $this->assertTrue(true, 'ClientGatewayTokens importing not completed yet.');
 
     }
@@ -503,7 +488,7 @@ class ImportTest extends TestCase
 
     public function testDocumentsImport()
     {
-        $this->invoice->forceDelete(); 
+        $this->invoice->forceDelete();
         $this->quote->forceDelete();
 
         $original = Document::count();