Merge pull request #15322 from snipe/importer_model_fixes

Importer model fixes

app/Http/Traits/TwoColumnUniqueUndeletedTrait.php

@@ -11,15 +11,17 @@ trait TwoColumnUniqueUndeletedTrait
      * @param  string $field
      * @return string
      */
-    protected function prepareTwoColumnUniqueUndeletedRule($parameters, $field)
+    protected function prepareTwoColumnUniqueUndeletedRule($parameters)
     {
         $column = $parameters[0];
         $value = $this->{$parameters[0]};
 
+        // This is an existing model we're updating so ignore the current ID ($this->getKey())
         if ($this->exists) {
             return 'two_column_unique_undeleted:'.$this->table.','.$this->getKey().','.$column.','.$value;
         }
 
+        // This is a new record, so we can ignore the current ID
         return 'two_column_unique_undeleted:'.$this->table.',0,'.$column.','.$value;
     }
 }

app/Importer/AssetImporter.php

@@ -71,8 +71,10 @@ class AssetImporter extends ItemImporter
         $asset = Asset::where(['asset_tag'=> (string) $asset_tag])->first();
         if ($asset) {
             if (! $this->updating) {
-                $this->log('A matching Asset '.$asset_tag.' already exists');
-                return;
+                $exists_error = trans('general.import_asset_tag_exists', ['asset_tag' => $asset_tag]);
+                $this->log($exists_error);
+                $this->addErrorToBag($asset, 'asset_tag', $exists_error);
+                return $exists_error;
             }
 
             $this->log('Updating Asset');

app/Importer/Importer.php

@@ -281,6 +281,13 @@ abstract class Importer
         }
     }
 
+    protected function addErrorToBag($item, $field,  $error_message)
+    {
+        if ($this->errorCallback) {
+            call_user_func($this->errorCallback, $item, $field, [$field => [$error_message]]);
+        }
+    }
+
     /**
      * Finds the user matching given data, or creates a new one if there is no match.
      * This is NOT used by the User Import, only for Asset/Accessory/etc where

app/Importer/ItemImporter.php

@@ -196,39 +196,46 @@ class ItemImporter extends Importer
     {
         $condition = array();
         $asset_model_name = $this->findCsvMatch($row, 'asset_model');
+        $asset_model_category = $this->findCsvMatch($row, 'category');
         $asset_modelNumber = $this->findCsvMatch($row, 'model_number');
+
         // TODO: At the moment, this means  we can't update the model number if the model name stays the same.
         if (! $this->shouldUpdateField($asset_model_name)) {
             return;
         }
+
         if ((empty($asset_model_name)) && (! empty($asset_modelNumber))) {
             $asset_model_name = $asset_modelNumber;
         } elseif ((empty($asset_model_name)) && (empty($asset_modelNumber))) {
             $asset_model_name = 'Unknown';
         }
 
-        if ((!empty($asset_model_name)) && (empty($asset_modelNumber))) {
-            $condition[] = ['name', '=', $asset_model_name];
-        } elseif ((!empty($asset_model_name)) && (!empty($asset_modelNumber))) {
-            $condition[] = ['name', '=', $asset_model_name];
-            $condition[] = ['model_number', '=', $asset_modelNumber];
+        $asset_model = AssetModel::select('id');
+
+        if (!empty($asset_model_name)) {
+            $asset_model = $asset_model->where('name', '=', $asset_model_name);
+
+            if (!empty($asset_modelNumber)) {
+                $asset_model = $asset_model->where('model_number', '=', $asset_modelNumber);
+            }
         }
 
         $editingModel = $this->updating;
-        $asset_model = AssetModel::where($condition)->first();
+        $asset_model = $asset_model->first();
 
         if ($asset_model) {
+
             if (! $this->updating) {
                 $this->log('A matching model already exists, returning it.');
-
                 return $asset_model->id;
             }
+
             $this->log('Matching Model found, updating it.');
             $item = $this->sanitizeItemForStoring($asset_model, $editingModel);
             $item['name'] = $asset_model_name;
             $item['notes'] = $this->findCsvMatch($row, 'model_notes');
 
-            if(!empty($asset_modelNumber)){
+            if (!empty($asset_modelNumber)){
                 $item['model_number'] = $asset_modelNumber;
             }
 
@@ -237,23 +244,29 @@ class ItemImporter extends Importer
             $this->log('Asset Model Updated');
             
             return $asset_model->id;
-        }
-        $this->log('No Matching Model, Creating a new one');
 
+        }
+
+        $this->log('No Matching Model, Creating a new one');
         $asset_model = new AssetModel();
         $item = $this->sanitizeItemForStoring($asset_model, $editingModel);
         $item['name'] = $asset_model_name;
         $item['model_number'] = $asset_modelNumber;
         $item['notes'] = $this->findCsvMatch($row, 'model_notes');
+        $item['category_id'] = $this->createOrFetchCategory($asset_model_category);
 
         $asset_model->fill($item);
+        //$asset_model = AssetModel::firstOrNew($item);
         $item = null;
 
+
+
         if ($asset_model->save()) {
             $this->log('Asset Model '.$asset_model_name.' with model number '.$asset_modelNumber.' was created');
 
             return $asset_model->id;
         }
+        $this->log('Asset Model Errors: '.$asset_model->getErrors());
         $this->logError($asset_model, 'Asset Model "'.$asset_model_name.'"');
 
         return null;

app/Livewire/Importer.php

@@ -374,6 +374,12 @@ class Importer extends Component
                     'model name',
                     'model',
                 ],
+            'eol_date' =>
+                [
+                    'eol',
+                    'eol date',
+                    'asset eol date',
+                ],
             'gravatar' =>
                 [
                     'gravatar',

app/Models/AssetModel.php

@@ -10,6 +10,7 @@ use Illuminate\Support\Facades\Gate;
 use Illuminate\Support\Facades\Storage;
 use Watson\Validating\ValidatingTrait;
 use \App\Presenters\AssetModelPresenter;
+use App\Http\Traits\TwoColumnUniqueUndeletedTrait;
 
 /**
  * Model for Asset Models. Asset Models contain higher level
@@ -21,21 +22,8 @@ class AssetModel extends SnipeModel
 {
     use HasFactory;
     use SoftDeletes;
-    protected $presenter = AssetModelPresenter::class;
     use Loggable, Requestable, Presentable;
-
-    protected $table = 'models';
-    protected $hidden = ['user_id', 'deleted_at'];
-
-    // Declare the rules for the model validation
-    protected $rules = [
-        'name'              => 'string|required|min:1|max:255|unique:models,name',
-        'model_number'      => 'string|max:255|nullable',
-        'min_amt'           => 'integer|min:0|nullable',
-        'category_id'       => 'required|integer|exists:categories,id',
-        'manufacturer_id'   => 'integer|exists:manufacturers,id|nullable',
-        'eol'               => 'integer:min:0|max:240|nullable',
-    ];
+    use TwoColumnUniqueUndeletedTrait;
 
     /**
      * Whether the model should inject its identifier to the unique
@@ -44,8 +32,26 @@ class AssetModel extends SnipeModel
      *
      * @var bool
      */
+
     protected $injectUniqueIdentifier = true;
     use ValidatingTrait;
+    protected $table = 'models';
+    protected $hidden = ['user_id', 'deleted_at'];
+    protected $presenter = AssetModelPresenter::class;
+
+    // Declare the rules for the model validation
+
+
+    protected $rules = [
+        'name'              => 'string|required|min:1|max:255|two_column_unique_undeleted:model_number',
+        'model_number'      => 'string|max:255|nullable|two_column_unique_undeleted:name',
+        'min_amt'           => 'integer|min:0|nullable',
+        'category_id'       => 'required|integer|exists:categories,id',
+        'manufacturer_id'   => 'integer|exists:manufacturers,id|nullable',
+        'eol'               => 'integer:min:0|max:240|nullable',
+    ];
+
+
 
     /**
      * The attributes that are mass assignable.
@@ -86,6 +92,9 @@ class AssetModel extends SnipeModel
         'manufacturer' => ['name'],
     ];
 
+
+
+
     /**
      * Establishes the model -> assets relationship
      *

app/Providers/ValidationServiceProvider.php

@@ -6,10 +6,7 @@ use App\Models\CustomField;
 use App\Models\Department;
 use App\Models\Setting;
 use Illuminate\Support\Facades\DB;
-use Illuminate\Support\Facades\Crypt;
-use Illuminate\Support\Facades\Log;
 use Illuminate\Support\ServiceProvider;
-use Illuminate\Validation\Rule;
 use Illuminate\Support\Facades\Validator;
 
 /**
@@ -91,18 +88,26 @@ class ValidationServiceProvider extends ServiceProvider
          *
          * $parameters[0] - the name of the first table we're looking at
          * $parameters[1] - the ID (this will be 0 on new creations)
-         * $parameters[2] - the name of the second table we're looking at
+         * $parameters[2] - the name of the second field we're looking at
          * $parameters[3] - the value that the request is passing for the second table we're
          *                  checking for uniqueness across
          *
          */
         Validator::extend('two_column_unique_undeleted', function ($attribute, $value, $parameters, $validator) {
+
             if (count($parameters)) {
+                
                 $count = DB::table($parameters[0])
-                         ->select('id')->where($attribute, '=', $value)
-                         ->whereNull('deleted_at')
-                         ->where('id', '!=', $parameters[1])
-                         ->where($parameters[2], $parameters[3])->count();
+                    ->select('id')
+                    ->where($attribute, '=', $value)
+                    ->where('id', '!=', $parameters[1]);
+
+                if ($parameters[3]!='') {
+                    $count = $count->where($parameters[2], $parameters[3]);
+                }
+
+                $count = $count->whereNull('deleted_at')
+                    ->count();
 
                 return $count < 1;
             }

resources/lang/en-US/general.php

@@ -561,5 +561,6 @@ return [
     'remaining_var' => ':count Remaining',
     'assets_in_var' => 'Assets in :name :type',
     'label' => 'Label',
+    'import_asset_tag_exists' => 'An asset with the asset tag :asset_tag already exists and an update was not requested. No change was made.',
 
 ];

resources/views/livewire/importer.blade.php

@@ -33,6 +33,7 @@
                         <table class="table table-striped table-bordered" id="errors-table">
                             <thead>
                             <th>{{ trans('general.item') }}</th>
+                            <th>Field</th>
                             <th>{{ trans('general.error') }}</th>
                             </thead>
                             <tbody>
@@ -41,8 +42,8 @@
                                     @foreach($error_bag as $field => $error_list)
                                         <tr>
                                             <td><b>{{ $key }}</b></td>
+                                            <td><b>{{ $field }}</b></td>
                                             <td>
-                                                <b>{{ $field }}:</b>
                                                 <span>{{ implode(", ",$error_list) }}</span>
                                                 <br />
                                             </td>

tests/Feature/AssetModels/Api/CreateAssetModelsTest.php

@@ -24,7 +24,7 @@ class CreateAssetModelsTest extends TestCase
         $response = $this->actingAsForApi(User::factory()->superuser()->create())
             ->postJson(route('api.models.store'), [
                 'name' => 'Test AssetModel',
-                'category_id' => Category::factory()->create()->id
+                'category_id' => Category::factory()->assetLaptopCategory()->create()->id
             ])
             ->assertOk()
             ->assertStatusMessageIs('success')
@@ -53,9 +53,52 @@ class CreateAssetModelsTest extends TestCase
             ])
             ->json();
 
-        // dd($response);
         $this->assertFalse(AssetModel::where('name', 'Test AssetModel')->exists());
 
     }
 
+    public function testUniquenessAcrossModelNameAndModelNumber()
+    {
+        AssetModel::factory()->create(['name' => 'Test Model', 'model_number'=>'1234']);
+
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->postJson(route('api.models.store'), [
+                'name' => 'Test Model',
+                'model_number' => '1234',
+                'category_id' => Category::factory()->assetLaptopCategory()->create()->id
+            ])
+            ->assertStatus(200)
+            ->assertOk()
+            ->assertStatusMessageIs('error')
+            ->assertJson([
+                'messages' => [
+                    'name'    => ['The name must be unique across models and model number. '],
+                    'model_number'    => ['The model number must be unique across models and name. '],
+                ],
+            ])
+            ->json();
+
+    }
+
+    public function testUniquenessAcrossModelNameAndModelNumberWithBlankModelNumber()
+    {
+        AssetModel::factory()->create(['name' => 'Test Model']);
+
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->postJson(route('api.models.store'), [
+                'name' => 'Test Model',
+                'category_id' => Category::factory()->assetLaptopCategory()->create()->id
+            ])
+            ->assertStatus(200)
+            ->assertOk()
+            ->assertStatusMessageIs('error')
+            ->assertJson([
+                'messages' => [
+                    'name'    => ['The name must be unique across models and model number. '],
+                ],
+            ])
+            ->json();
+
+    }
+
 }

tests/Feature/AssetModels/Ui/CreateAssetModelsTest.php

@@ -24,6 +24,7 @@ class CreateAssetModelsTest extends TestCase
         $this->assertFalse(AssetModel::where('name', 'Test Model')->exists());
 
         $this->actingAs(User::factory()->superuser()->create())
+            ->from(route('models.create'))
             ->post(route('models.store'), [
                 'name' => 'Test Model',
                 'category_id' => Category::factory()->create()->id
@@ -51,4 +52,46 @@ class CreateAssetModelsTest extends TestCase
 
     }
 
+    public function testUniquenessAcrossModelNameAndModelNumber()
+    {
+
+        AssetModel::factory()->create(['name' => 'Test Model', 'model_number'=>'1234']);
+
+        $response = $this->actingAs(User::factory()->superuser()->create())
+            ->from(route('models.create'))
+            ->post(route('models.store'), [
+                'name' => 'Test Model',
+                'model_number' => '1234',
+                'category_id' => Category::factory()->create()->id
+            ])
+            ->assertStatus(302)
+            ->assertSessionHasErrors(['name','model_number'])
+            ->assertRedirect(route('models.create'))
+            ->assertInvalid(['name','model_number']);
+
+        $this->followRedirects($response)->assertSee(trans('general.error'));
+
+    }
+
+    public function testUniquenessAcrossModelNameAndModelNumberWithoutModelNumber()
+    {
+
+        AssetModel::factory()->create(['name' => 'Test Model', 'model_number'=> null]);
+
+        $response = $this->actingAs(User::factory()->superuser()->create())
+            ->from(route('models.create'))
+            ->post(route('models.store'), [
+                'name' => 'Test Model',
+                'model_number' => null,
+                'category_id' => Category::factory()->create()->id
+            ])
+            ->assertStatus(302)
+            ->assertSessionHasErrors(['name'])
+            ->assertRedirect(route('models.create'))
+            ->assertInvalid(['name']);
+
+        $this->followRedirects($response)->assertSee(trans('general.error'));
+
+    }
+
 }