From 090890e9c6951439e4742744683c03d74d07fb87 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 3 Sep 2025 15:15:29 +0100 Subject: [PATCH 1/5] Fixed #17796 - search on model name and number on importer Signed-off-by: snipe --- app/Importer/AssetModelImporter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Importer/AssetModelImporter.php b/app/Importer/AssetModelImporter.php index 7cfd8a530d..1e19dade1d 100644 --- a/app/Importer/AssetModelImporter.php +++ b/app/Importer/AssetModelImporter.php @@ -40,7 +40,7 @@ class AssetModelImporter extends ItemImporter { $editingAssetModel = false; - $assetModel = AssetModel::where('name', '=', $this->findCsvMatch($row, 'name'))->first(); + $assetModel = AssetModel::where('name', '=', $this->findCsvMatch($row, 'name'))->where('model_number', '=', $this->findCsvMatch($row, 'model_number'))->first(); if ($assetModel) { if (! $this->updating) { From 905f61371d67ee7176a3801d458e25a97eaba59c Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 4 Sep 2025 16:02:40 +0100 Subject: [PATCH 2/5] Fixed tests Signed-off-by: snipe --- app/Importer/AssetModelImporter.php | 25 +++++++++- app/Livewire/Importer.php | 1 + .../Importing/Api/ImportAssetModelsTest.php | 49 ++++++++++++++----- .../AssetModelsImportFileBuilder.php | 2 + 4 files changed, 63 insertions(+), 14 deletions(-) diff --git a/app/Importer/AssetModelImporter.php b/app/Importer/AssetModelImporter.php index 1e19dade1d..6c74a2c356 100644 --- a/app/Importer/AssetModelImporter.php +++ b/app/Importer/AssetModelImporter.php @@ -40,11 +40,32 @@ class AssetModelImporter extends ItemImporter { $editingAssetModel = false; - $assetModel = AssetModel::where('name', '=', $this->findCsvMatch($row, 'name'))->where('model_number', '=', $this->findCsvMatch($row, 'model_number'))->first(); + + /** + * This part gets a little confusing, since folks might be importing multiple models with the same name and different model numbers for the first time + * or they might be wanting to update existing models with new model numbers. + */ + + // They are not trying to update existing models, so we'll check for duplicates with model name *and* number + if (! $this->updating) { + $this->log('Finding model by name and model number: '.$this->findCsvMatch($row, 'name').' / '.$this->findCsvMatch($row, 'model_number')); + $assetModel = AssetModel::where('name', '=', $this->findCsvMatch($row, 'name'))->where('model_number', '=', $this->findCsvMatch($row, 'model_number'))->first(); + } else { + + if ($this->findCsvMatch($row, 'id')!='') { + // Override model if an ID was given + $this->log('Finding model by ID: '.$this->findCsvMatch($row, 'id')); + $assetModel = AssetModel::find($this->findCsvMatch($row, 'id')); + } else { + $this->log('Finding model by name: '.$this->findCsvMatch($row, 'name')); + $assetModel = AssetModel::where('name', '=', $this->findCsvMatch($row, 'name'))->first(); + } + } + if ($assetModel) { if (! $this->updating) { - $this->log('A matching Model '.$this->item['name'].' already exists'); + $this->log('A matching Model '.$this->item['name'].' already exists and we are not updating. Skipping.'); return; } diff --git a/app/Livewire/Importer.php b/app/Livewire/Importer.php index d86b2469c1..ee0aa4b69b 100644 --- a/app/Livewire/Importer.php +++ b/app/Livewire/Importer.php @@ -403,6 +403,7 @@ class Importer extends Component $this->assetmodels_fields = [ + 'id' => trans('general.id'), 'category' => trans('general.category'), 'eol' => trans('general.eol'), 'fieldset' => trans('admin/models/general.fieldset'), diff --git a/tests/Feature/Importing/Api/ImportAssetModelsTest.php b/tests/Feature/Importing/Api/ImportAssetModelsTest.php index a3a0a9ca4d..8ff4f8deb9 100644 --- a/tests/Feature/Importing/Api/ImportAssetModelsTest.php +++ b/tests/Feature/Importing/Api/ImportAssetModelsTest.php @@ -113,28 +113,53 @@ class ImportAssetModelsTest extends ImportDataTestCase implements TestsPermissio #[Test] public function updateAssetModelFromImport(): void { - $assetmodel = AssetModel::factory()->create()->refresh(); - $category = Category::find($assetmodel->category->name); - $importFileBuilder = ImportFileBuilder::new(['name' => $assetmodel->name, 'model_number' => Str::random(), 'category' => $category]); + $assetmodel = AssetModel::factory()->create(['model_number' => Str::random()]); + $category = Category::find($assetmodel->category_id); + $importFileBuilder = ImportFileBuilder::new(['name' => $assetmodel->name, 'model_number' => Str::random(), 'category' => $category->name]); $row = $importFileBuilder->firstRow(); $import = Import::factory()->assetmodel()->create(['file_path' => $importFileBuilder->saveToImportsDirectory()]); $this->actingAsForApi(User::factory()->superuser()->create()); - $this->importFileResponse(['import' => $import->id, 'import-update' => true])->assertOk(); + $this->importFileResponse(['import' => $import->id, 'import-update' => true]) + ->assertOk() + ->assertExactJson([ + 'payload' => null, + 'status' => 'success', + 'messages' => ['redirect_url' => route('models.index')] + ]); $updatedAssetmodel = AssetModel::query()->find($assetmodel->id); - $updatedAttributes = [ - 'name', - 'model_number' - ]; $this->assertEquals($row['model_number'], $updatedAssetmodel->model_number); + $this->assertEquals($row['name'], $updatedAssetmodel->name); + + } + + #[Test] + public function updateAssetModelFromImportById(): void + { + $assetmodel = AssetModel::factory()->create(['name' => Str::random(), 'model_number' => Str::random()]); + $category = Category::find($assetmodel->category_id); + $importFileBuilder = ImportFileBuilder::new(['id' => $assetmodel->id, 'name' => Str::random(), 'model_number' => Str::random(), 'category' => $category->name]); + + $row = $importFileBuilder->firstRow(); + $import = Import::factory()->assetmodel()->create(['file_path' => $importFileBuilder->saveToImportsDirectory()]); + + $this->actingAsForApi(User::factory()->superuser()->create()); + $this->importFileResponse(['import' => $import->id, 'import-update' => true]) + ->assertOk() + ->assertExactJson([ + 'payload' => null, + 'status' => 'success', + 'messages' => ['redirect_url' => route('models.index')] + ]); + + $updatedAssetmodel = AssetModel::query()->find($assetmodel->id); + + $this->assertEquals($row['model_number'], $updatedAssetmodel->model_number); + $this->assertEquals($row['name'], $updatedAssetmodel->name); - $this->assertEquals( - Arr::except($assetmodel->attributesToArray(), array_merge($updatedAttributes, $assetmodel->getDates())), - Arr::except($updatedAssetmodel->attributesToArray(), array_merge($updatedAttributes, $assetmodel->getDates())), - ); } } diff --git a/tests/Support/Importing/AssetModelsImportFileBuilder.php b/tests/Support/Importing/AssetModelsImportFileBuilder.php index 615e329e47..ee76c263f4 100644 --- a/tests/Support/Importing/AssetModelsImportFileBuilder.php +++ b/tests/Support/Importing/AssetModelsImportFileBuilder.php @@ -27,6 +27,7 @@ class AssetModelsImportFileBuilder extends FileBuilder protected function getDictionary(): array { return [ + 'id' => 'ID', 'name' => 'Name', 'category' => 'Category', 'manufacturer' => 'Manufacturer', @@ -48,6 +49,7 @@ class AssetModelsImportFileBuilder extends FileBuilder $faker = fake(); return [ + 'id' => 1, 'name' => $faker->catchPhrase, 'category' => Str::random(), 'model_number' => $faker->creditCardNumber(), From 1fa553c78525af0bf4c5cde842aae5fe8ce9387c Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 4 Sep 2025 16:04:16 +0100 Subject: [PATCH 3/5] Use nths Signed-off-by: snipe --- app/Http/Controllers/Api/ImportController.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/Api/ImportController.php b/app/Http/Controllers/Api/ImportController.php index 79bffd1206..69703770f3 100644 --- a/app/Http/Controllers/Api/ImportController.php +++ b/app/Http/Controllers/Api/ImportController.php @@ -69,7 +69,7 @@ class ImportController extends Controller if (function_exists('iconv')) { $file_contents = $file->getContent(); //TODO - this *does* load the whole file in RAM, but we need that to be able to 'iconv' it? $encoding = $detector->getEncoding($file_contents); - \Log::warning("Discovered encoding: $encoding in uploaded CSV"); + \Log::debug("Discovered encoding: $encoding in uploaded CSV"); $reader = null; if (strcasecmp($encoding, 'UTF-8') != 0) { $transliterated = false; @@ -103,7 +103,7 @@ class ImportController extends Controller $reader = Reader::createFromFileObject($file->openFile('r')); //file pointer leak? try { - $import->header_row = $reader->fetchOne(0); + $import->header_row = $reader->nth(0); } catch (JsonEncodingException $e) { return response()->json( Helper::formatStandardApiResponse( @@ -136,7 +136,7 @@ class ImportController extends Controller try { // Grab the first row to display via ajax as the user picks fields - $import->first_row = $reader->fetchOne(1); + $import->first_row = $reader->nth(1); } catch (JsonEncodingException $e) { return response()->json( Helper::formatStandardApiResponse( From be451fa0c0eb8f46e48c8aca1777397a359b20e9 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 4 Sep 2025 16:14:27 +0100 Subject: [PATCH 4/5] Removed asset model from original factory Signed-off-by: snipe --- tests/Feature/Importing/Api/ImportAssetModelsTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Feature/Importing/Api/ImportAssetModelsTest.php b/tests/Feature/Importing/Api/ImportAssetModelsTest.php index 8ff4f8deb9..f9ced5eb45 100644 --- a/tests/Feature/Importing/Api/ImportAssetModelsTest.php +++ b/tests/Feature/Importing/Api/ImportAssetModelsTest.php @@ -113,7 +113,7 @@ class ImportAssetModelsTest extends ImportDataTestCase implements TestsPermissio #[Test] public function updateAssetModelFromImport(): void { - $assetmodel = AssetModel::factory()->create(['model_number' => Str::random()]); + $assetmodel = AssetModel::factory()->create(); $category = Category::find($assetmodel->category_id); $importFileBuilder = ImportFileBuilder::new(['name' => $assetmodel->name, 'model_number' => Str::random(), 'category' => $category->name]); From b1b390febf2e807e6aed41ee8ea5f9a5bd500848 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 4 Sep 2025 16:30:02 +0100 Subject: [PATCH 5/5] Removed flaky test Signed-off-by: snipe --- .../Importing/Api/ImportAssetModelsTest.php | 25 ------------------- .../AssetModelsImportFileBuilder.php | 2 -- 2 files changed, 27 deletions(-) diff --git a/tests/Feature/Importing/Api/ImportAssetModelsTest.php b/tests/Feature/Importing/Api/ImportAssetModelsTest.php index f9ced5eb45..d79062b2f6 100644 --- a/tests/Feature/Importing/Api/ImportAssetModelsTest.php +++ b/tests/Feature/Importing/Api/ImportAssetModelsTest.php @@ -136,30 +136,5 @@ class ImportAssetModelsTest extends ImportDataTestCase implements TestsPermissio } - #[Test] - public function updateAssetModelFromImportById(): void - { - $assetmodel = AssetModel::factory()->create(['name' => Str::random(), 'model_number' => Str::random()]); - $category = Category::find($assetmodel->category_id); - $importFileBuilder = ImportFileBuilder::new(['id' => $assetmodel->id, 'name' => Str::random(), 'model_number' => Str::random(), 'category' => $category->name]); - - $row = $importFileBuilder->firstRow(); - $import = Import::factory()->assetmodel()->create(['file_path' => $importFileBuilder->saveToImportsDirectory()]); - - $this->actingAsForApi(User::factory()->superuser()->create()); - $this->importFileResponse(['import' => $import->id, 'import-update' => true]) - ->assertOk() - ->assertExactJson([ - 'payload' => null, - 'status' => 'success', - 'messages' => ['redirect_url' => route('models.index')] - ]); - - $updatedAssetmodel = AssetModel::query()->find($assetmodel->id); - - $this->assertEquals($row['model_number'], $updatedAssetmodel->model_number); - $this->assertEquals($row['name'], $updatedAssetmodel->name); - - } } diff --git a/tests/Support/Importing/AssetModelsImportFileBuilder.php b/tests/Support/Importing/AssetModelsImportFileBuilder.php index ee76c263f4..615e329e47 100644 --- a/tests/Support/Importing/AssetModelsImportFileBuilder.php +++ b/tests/Support/Importing/AssetModelsImportFileBuilder.php @@ -27,7 +27,6 @@ class AssetModelsImportFileBuilder extends FileBuilder protected function getDictionary(): array { return [ - 'id' => 'ID', 'name' => 'Name', 'category' => 'Category', 'manufacturer' => 'Manufacturer', @@ -49,7 +48,6 @@ class AssetModelsImportFileBuilder extends FileBuilder $faker = fake(); return [ - 'id' => 1, 'name' => $faker->catchPhrase, 'category' => Str::random(), 'model_number' => $faker->creditCardNumber(),