From b59bf3e7dc803b24159744c8bf2819a95a7df8ab Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 21 Aug 2024 16:49:29 -0700 Subject: [PATCH 1/5] Add failing test --- .../AssetModels/Ui/UpdateAssetModelsTest.php | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php b/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php index 423eaad57..9a58eb5c5 100644 --- a/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php +++ b/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php @@ -4,6 +4,8 @@ namespace Tests\Feature\AssetModels\Ui; use App\Models\AssetModel; use App\Models\Category; +use App\Models\CustomField; +use App\Models\CustomFieldset; use App\Models\User; use Tests\TestCase; @@ -19,7 +21,7 @@ class UpdateAssetModelsTest extends TestCase ->assertStatus(403) ->assertForbidden(); } - + public function testUserCanEditAssetModels() { $category = Category::factory()->forAssets()->create(); @@ -62,4 +64,41 @@ class UpdateAssetModelsTest extends TestCase } + public function test_default_values_remain_unchanged_after_validation_error_occurs() + { + $this->markIncompleteIfMySQL('Custom Field Tests do not work in MySQL'); + + $assetModel = AssetModel::factory()->create(); + + $customFieldset = CustomFieldset::factory()->create(); + + [$customFieldOne, $customFieldTwo] = CustomField::factory()->count(2)->create(); + + $customFieldset->fields()->attach($customFieldOne, ['order' => 1, 'required' => false]); + $customFieldset->fields()->attach($customFieldTwo, ['order' => 2, 'required' => false]); + + $assetModel->fieldset()->associate($customFieldset); + + $assetModel->defaultValues()->attach($customFieldOne, ['default_value' => 'first default value']); + $assetModel->defaultValues()->attach($customFieldTwo, ['default_value' => 'second default value']); + + $this->actingAs(User::factory()->superuser()->create()) + ->put(route('models.update', ['model' => $assetModel]), [ + // should trigger validation error without name, etc, and NOT remove or change default values + 'add_default_values' => '1', + 'fieldset_id' => $customFieldset->id, + 'default_values' => [ + $customFieldOne->id => 'changed value', + $customFieldTwo->id => 'changed value', + ], + ]); + + $this->assertEquals( + 2, + $assetModel->fresh()->defaultValues->filter(function (CustomField $field) { + return in_array($field->pivot->default_value, ['first default value', 'second default value']); + })->count(), + 'Default field values were changed unexpectedly.' + ); + } } From bcace9d019b08048a954a9b89df9e09f8cbfa67e Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 21 Aug 2024 16:54:16 -0700 Subject: [PATCH 2/5] Point test to correct endpoint --- tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php b/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php index 9a58eb5c5..3ca91e982 100644 --- a/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php +++ b/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php @@ -14,11 +14,10 @@ class UpdateAssetModelsTest extends TestCase public function testPermissionRequiredToStoreAssetModel() { $this->actingAs(User::factory()->create()) - ->post(route('models.store'), [ - 'name' => 'Test Model', - 'category_id' => Category::factory()->create()->id + ->put(route('models.update', ['model' => AssetModel::factory()->create()]), [ + 'name' => 'Changed Name', + 'category_id' => Category::factory()->create()->id, ]) - ->assertStatus(403) ->assertForbidden(); } From 663b2fd844aa6461b5336f10f66d7ea49cf3ecdf Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 21 Aug 2024 16:59:38 -0700 Subject: [PATCH 3/5] Add test case --- .../AssetModels/Ui/UpdateAssetModelsTest.php | 42 +++++++++++++++++-- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php b/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php index 3ca91e982..83ffce7c9 100644 --- a/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php +++ b/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php @@ -68,9 +68,7 @@ class UpdateAssetModelsTest extends TestCase $this->markIncompleteIfMySQL('Custom Field Tests do not work in MySQL'); $assetModel = AssetModel::factory()->create(); - $customFieldset = CustomFieldset::factory()->create(); - [$customFieldOne, $customFieldTwo] = CustomField::factory()->count(2)->create(); $customFieldset->fields()->attach($customFieldOne, ['order' => 1, 'required' => false]); @@ -87,8 +85,8 @@ class UpdateAssetModelsTest extends TestCase 'add_default_values' => '1', 'fieldset_id' => $customFieldset->id, 'default_values' => [ - $customFieldOne->id => 'changed value', - $customFieldTwo->id => 'changed value', + $customFieldOne->id => 'first changed value', + $customFieldTwo->id => 'second changed value', ], ]); @@ -100,4 +98,40 @@ class UpdateAssetModelsTest extends TestCase 'Default field values were changed unexpectedly.' ); } + + public function test_default_values_can_be_updated() + { + $this->markIncompleteIfMySQL('Custom Field Tests do not work in MySQL'); + + $assetModel = AssetModel::factory()->create(); + $customFieldset = CustomFieldset::factory()->create(); + [$customFieldOne, $customFieldTwo] = CustomField::factory()->count(2)->create(); + + $customFieldset->fields()->attach($customFieldOne, ['order' => 1, 'required' => false]); + $customFieldset->fields()->attach($customFieldTwo, ['order' => 2, 'required' => false]); + + $assetModel->fieldset()->associate($customFieldset); + + $assetModel->defaultValues()->attach($customFieldOne, ['default_value' => 'first default value']); + $assetModel->defaultValues()->attach($customFieldTwo, ['default_value' => 'second default value']); + + $this->actingAs(User::factory()->superuser()->create()) + ->put(route('models.update', ['model' => $assetModel]), [ + // should trigger validation error without name, etc, and NOT remove or change default values + 'name' => 'Test Model Edited', + 'category_id' => $assetModel->category_id, + 'add_default_values' => '1', + 'fieldset_id' => $customFieldset->id, + 'default_values' => [ + $customFieldOne->id => 'first changed value', + $customFieldTwo->id => 'second changed value', + ], + ]); + + $potentiallyChangedDefaultValues = $assetModel->defaultValues->pluck('pivot.default_value'); + + $this->assertCount(2, $potentiallyChangedDefaultValues); + $this->assertContains('first changed value', $potentiallyChangedDefaultValues); + $this->assertContains('second changed value', $potentiallyChangedDefaultValues); + } } From d67975cb62f1f5ddb5bc3fa7c5297cb71e3388ea Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 21 Aug 2024 16:59:44 -0700 Subject: [PATCH 4/5] Implement fix --- app/Http/Controllers/AssetModelsController.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/Http/Controllers/AssetModelsController.php b/app/Http/Controllers/AssetModelsController.php index b6bb34738..60b1a3980 100755 --- a/app/Http/Controllers/AssetModelsController.php +++ b/app/Http/Controllers/AssetModelsController.php @@ -151,17 +151,17 @@ class AssetModelsController extends Controller $model->notes = $request->input('notes'); $model->requestable = $request->input('requestable', '0'); - $this->removeCustomFieldsDefaultValues($model); - $model->fieldset_id = $request->input('fieldset_id'); - if ($this->shouldAddDefaultValues($request->input())) { - if (!$this->assignCustomFieldsDefaultValues($model, $request->input('default_values'))){ - return redirect()->back()->withInput()->with('error', trans('admin/custom_fields/message.fieldset_default_value.error')); - } - } - if ($model->save()) { + $this->removeCustomFieldsDefaultValues($model); + + if ($this->shouldAddDefaultValues($request->input())) { + if (!$this->assignCustomFieldsDefaultValues($model, $request->input('default_values'))) { + return redirect()->back()->withInput()->with('error', trans('admin/custom_fields/message.fieldset_default_value.error')); + } + } + if ($model->wasChanged('eol')) { if ($model->eol > 0) { $newEol = $model->eol; From af0a95be12fbe3b12ec710e95935fa1dcb54bc74 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 21 Aug 2024 17:00:32 -0700 Subject: [PATCH 5/5] Simplify assertions --- .../Feature/AssetModels/Ui/UpdateAssetModelsTest.php | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php b/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php index 83ffce7c9..95eb592b6 100644 --- a/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php +++ b/tests/Feature/AssetModels/Ui/UpdateAssetModelsTest.php @@ -90,13 +90,10 @@ class UpdateAssetModelsTest extends TestCase ], ]); - $this->assertEquals( - 2, - $assetModel->fresh()->defaultValues->filter(function (CustomField $field) { - return in_array($field->pivot->default_value, ['first default value', 'second default value']); - })->count(), - 'Default field values were changed unexpectedly.' - ); + $potentiallyChangedDefaultValues = $assetModel->defaultValues->pluck('pivot.default_value'); + $this->assertCount(2, $potentiallyChangedDefaultValues); + $this->assertContains('first default value', $potentiallyChangedDefaultValues); + $this->assertContains('second default value', $potentiallyChangedDefaultValues); } public function test_default_values_can_be_updated() @@ -129,7 +126,6 @@ class UpdateAssetModelsTest extends TestCase ]); $potentiallyChangedDefaultValues = $assetModel->defaultValues->pluck('pivot.default_value'); - $this->assertCount(2, $potentiallyChangedDefaultValues); $this->assertContains('first changed value', $potentiallyChangedDefaultValues); $this->assertContains('second changed value', $potentiallyChangedDefaultValues);