diff --git a/app/Http/Traits/TwoColumnUniqueUndeletedTrait.php b/app/Http/Traits/TwoColumnUniqueUndeletedTrait.php index 4aae02bfbd..0cf0edb455 100644 --- a/app/Http/Traits/TwoColumnUniqueUndeletedTrait.php +++ b/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; } } diff --git a/app/Importer/AssetImporter.php b/app/Importer/AssetImporter.php index 4bb887bcd8..781a6311fe 100644 --- a/app/Importer/AssetImporter.php +++ b/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'); diff --git a/app/Importer/Importer.php b/app/Importer/Importer.php index 9738723509..c2214ef37d 100644 --- a/app/Importer/Importer.php +++ b/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 diff --git a/app/Importer/ItemImporter.php b/app/Importer/ItemImporter.php index ee680413d8..29197ca5dc 100644 --- a/app/Importer/ItemImporter.php +++ b/app/Importer/ItemImporter.php @@ -196,64 +196,77 @@ 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; } $asset_model->update($item); $asset_model->save(); $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; diff --git a/app/Livewire/Importer.php b/app/Livewire/Importer.php index e164af36d6..32dd7912fb 100644 --- a/app/Livewire/Importer.php +++ b/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', diff --git a/app/Models/AssetModel.php b/app/Models/AssetModel.php index 7a66620916..3c023507db 100755 --- a/app/Models/AssetModel.php +++ b/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 * diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php index 041aaad98e..1f3abca8a6 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/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; } diff --git a/resources/lang/en-US/general.php b/resources/lang/en-US/general.php index 4144fd2f6a..7634387906 100644 --- a/resources/lang/en-US/general.php +++ b/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.', ]; diff --git a/resources/views/livewire/importer.blade.php b/resources/views/livewire/importer.blade.php index ab4646041b..92dfd128ad 100644 --- a/resources/views/livewire/importer.blade.php +++ b/resources/views/livewire/importer.blade.php @@ -33,6 +33,7 @@ + @@ -41,8 +42,8 @@ @foreach($error_bag as $field => $error_list) + diff --git a/tests/Feature/AssetModels/Api/CreateAssetModelsTest.php b/tests/Feature/AssetModels/Api/CreateAssetModelsTest.php index a0b1c27b73..2928d926f6 100644 --- a/tests/Feature/AssetModels/Api/CreateAssetModelsTest.php +++ b/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(); + + } + } diff --git a/tests/Feature/AssetModels/Ui/CreateAssetModelsTest.php b/tests/Feature/AssetModels/Ui/CreateAssetModelsTest.php index ef35aa5f6c..b0c6f65a59 100644 --- a/tests/Feature/AssetModels/Ui/CreateAssetModelsTest.php +++ b/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')); + + } + }
{{ trans('general.item') }}Field {{ trans('general.error') }}
{{ $key }}{{ $field }} - {{ $field }}: {{ implode(", ",$error_list) }}