parent 2220828

author Brady Wetherington <bwetherington@grokability.com> 1728320853 +0100 committer Brady Wetherington <bwetherington@grokability.com> 1733158021 +0000 Prevent setting assigned_to without setting assigned_type Fixed tests to include assigned_type when setting assigned_to Add new tests for assigned_to without assigned_type Added tighter validation to assigned_to and assigned_type, new tests Fixed wrong comment Fixed tests to include assigned_type when setting assigned_to Add new tests for assigned_to without assigned_type Fixed wrong comment
snipe · Dec 2, 2024 · eccdcc3 · eccdcc3
1 parent 2220828
commit eccdcc3
Show file tree

Hide file tree

Showing 8 changed files with 167 additions and 9 deletions.
diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php
@@ -679,7 +679,9 @@ public function store(StoreAssetRequest $request): JsonResponse
 
             return response()->json(Helper::formatStandardApiResponse('success', $asset, trans('admin/hardware/message.create.success')));
 
-            return response()->json(Helper::formatStandardApiResponse('success', (new AssetsTransformer)->transformAsset($asset), trans('admin/hardware/message.create.success')));
+            // below is what we want the _eventual_ return to look like - in a more standardized format.
+            // return response()->json(Helper::formatStandardApiResponse('success', (new AssetsTransformer)->transformAsset($asset), trans('admin/hardware/message.create.success')));
+
         }
 
         return response()->json(Helper::formatStandardApiResponse('error', null, $asset->getErrors()), 200);

diff --git a/app/Http/Requests/StoreAssetRequest.php b/app/Http/Requests/StoreAssetRequest.php
@@ -39,7 +39,6 @@ public function prepareForValidation(): void
         $this->merge([
             'asset_tag' => $this->asset_tag ?? Asset::autoincrement_asset(),
             'company_id' => $idForCurrentUser,
-            'assigned_to' => $assigned_to ?? null,
         ]);
     }
 

diff --git a/app/Models/Asset.php b/app/Models/Asset.php
@@ -119,7 +119,8 @@ public function declinedCheckout(User $declinedBy, $signature)
         'byod'              => ['nullable', 'boolean'],
         'order_number'      => ['nullable', 'string', 'max:191'],
         'notes'             => ['nullable', 'string', 'max:65535'],
-        'assigned_to'       => ['nullable', 'integer'],
+        'assigned_to'   => ['nullable', 'integer', 'required_with:assigned_type'],
+        'assigned_type' => ['nullable', 'required_with:assigned_to', 'in:'.User::class.",".Location::class.",".Asset::class],
         'requestable'       => ['nullable', 'boolean'],
         'assigned_user'     => ['nullable', 'exists:users,id,deleted_at,NULL'],
         'assigned_location' => ['nullable', 'exists:locations,id,deleted_at,NULL'],

diff --git a/app/Observers/AssetObserver.php b/app/Observers/AssetObserver.php
@@ -40,8 +40,9 @@ public function updating(Asset $asset)
 
         // If the asset isn't being checked out or audited, log the update.
         // (Those other actions already create log entries.)
-	    if (($attributes['assigned_to'] == $attributesOriginal['assigned_to'])
-	    && ($same_checkout_counter) && ($same_checkin_counter)
+        if (array_key_exists('assigned_to', $attributes) && array_key_exists('assigned_to', $attributesOriginal)
+            && ($attributes['assigned_to'] == $attributesOriginal['assigned_to'])
+            && ($same_checkout_counter) && ($same_checkin_counter)
             && ((isset( $attributes['next_audit_date']) ? $attributes['next_audit_date'] : null) == (isset($attributesOriginal['next_audit_date']) ? $attributesOriginal['next_audit_date']: null))
             && ($attributes['last_checkout'] == $attributesOriginal['last_checkout']) && (!$restoring_or_deleting))
         {

diff --git a/tests/Feature/Assets/Api/AssetIndexTest.php b/tests/Feature/Assets/Api/AssetIndexTest.php
@@ -83,7 +83,7 @@ public function testAssetApiIndexReturnsDueOrOverdueForAudit()
 
     public function testAssetApiIndexReturnsDueForExpectedCheckin()
     {
-        Asset::factory()->count(3)->create(['assigned_to' => '1', 'expected_checkin' => Carbon::now()->format('Y-m-d')]);
+        Asset::factory()->count(3)->create(['assigned_to' => '1', 'assigned_type' => User::class, 'expected_checkin' => Carbon::now()->format('Y-m-d')]);
 
         $this->actingAsForApi(User::factory()->superuser()->create())
             ->getJson(
@@ -99,7 +99,7 @@ public function testAssetApiIndexReturnsDueForExpectedCheckin()
 
     public function testAssetApiIndexReturnsOverdueForExpectedCheckin()
     {
-        Asset::factory()->count(3)->create(['assigned_to' => '1', 'expected_checkin' => Carbon::now()->subDays(1)->format('Y-m-d')]);
+        Asset::factory()->count(3)->create(['assigned_to' => '1', 'assigned_type' => User::class, 'expected_checkin' => Carbon::now()->subDays(1)->format('Y-m-d')]);
 
         $this->actingAsForApi(User::factory()->superuser()->create())
             ->getJson(route('api.assets.list-upcoming', ['action' => 'checkins', 'upcoming_status' => 'overdue']))
@@ -113,8 +113,8 @@ public function testAssetApiIndexReturnsOverdueForExpectedCheckin()
 
     public function testAssetApiIndexReturnsDueOrOverdueForExpectedCheckin()
     {
-        Asset::factory()->count(3)->create(['assigned_to' => '1', 'expected_checkin' => Carbon::now()->subDays(1)->format('Y-m-d')]);
-        Asset::factory()->count(2)->create(['assigned_to' => '1', 'expected_checkin' => Carbon::now()->format('Y-m-d')]);
+        Asset::factory()->count(3)->create(['assigned_to' => '1', 'assigned_type' => User::class, 'expected_checkin' => Carbon::now()->subDays(1)->format('Y-m-d')]);
+        Asset::factory()->count(2)->create(['assigned_to' => '1', 'assigned_type' => User::class, 'expected_checkin' => Carbon::now()->format('Y-m-d')]);
 
         $this->actingAsForApi(User::factory()->superuser()->create())
             ->getJson(route('api.assets.list-upcoming', ['action' => 'checkins', 'upcoming_status' => 'due-or-overdue']))

diff --git a/tests/Feature/Assets/Api/StoreAssetTest.php b/tests/Feature/Assets/Api/StoreAssetTest.php
@@ -162,6 +162,71 @@ public function testSaveWithPendingStatusAndUserReturnsValidationError()
         $this->assertNotNull($response->json('messages.status_id'));
     }
 
+    public function testSaveWithAssignedToChecksOut()
+    {
+        $user = User::factory()->create();
+        $response = $this->actingAsForApi(User::factory()->superuser()->create())
+            ->postJson(route('api.assets.store'), [
+                'asset_tag' => '1235',
+                'assigned_to' => $user->id,
+                'assigned_type' => User::class,
+                'model_id' => AssetModel::factory()->create()->id,
+                'status_id' => Statuslabel::factory()->readyToDeploy()->create()->id,
+            ])
+            ->assertOk()
+            ->assertStatusMessageIs('success');
+
+        $asset = Asset::find($response->json()['payload']['id']);
+        $this->assertEquals($user->id, $asset->assigned_to);
+        $this->assertEquals('Asset created successfully. :)', $response->json('messages'));
+    }
+
+
+    public function testSaveWithNoAssignedTypeReturnsValidationError()
+    {
+        $response = $this->actingAsForApi(User::factory()->superuser()->create())
+            ->postJson(route('api.assets.store'), [
+                'asset_tag' => '1235',
+                'assigned_to' => '1',
+//                'assigned_type' => User::class, //deliberately omit assigned_type
+                'model_id' => AssetModel::factory()->create()->id,
+                'status_id' => Statuslabel::factory()->readyToDeploy()->create()->id,
+            ])
+            ->assertOk()
+            ->assertStatusMessageIs('error');
+        $this->assertNotNull($response->json('messages.assigned_type'));
+    }
+
+    public function testSaveWithBadAssignedTypeReturnsValidationError()
+    {
+        $response = $this->actingAsForApi(User::factory()->superuser()->create())
+            ->postJson(route('api.assets.store'), [
+                'asset_tag'     => '1235',
+                'assigned_to'   => '1',
+                'assigned_type' => 'nonsense_string', //deliberately bad assigned_type
+                'model_id'      => AssetModel::factory()->create()->id,
+                'status_id'     => Statuslabel::factory()->readyToDeploy()->create()->id,
+            ])
+            ->assertOk()
+            ->assertStatusMessageIs('error');
+        $this->assertNotNull($response->json('messages.assigned_type'));
+    }
+
+    public function testSaveWithAssignedTypeAndNoAssignedToReturnsValidationError()
+    {
+        $response = $this->actingAsForApi(User::factory()->superuser()->create())
+            ->postJson(route('api.assets.store'), [
+                'asset_tag'     => '1235',
+                //'assigned_to'   => '1', //deliberately omit assigned_to
+                'assigned_type' => User::class,
+                'model_id'      => AssetModel::factory()->create()->id,
+                'status_id'     => Statuslabel::factory()->readyToDeploy()->create()->id,
+            ])
+            ->assertOk()
+            ->assertStatusMessageIs('error');
+        $this->assertNotNull($response->json('messages.assigned_to'));
+    }
+
     public function testSaveWithPendingStatusWithoutUserIsSuccessful()
     {
         $response = $this->actingAsForApi(User::factory()->superuser()->create())

diff --git a/tests/Feature/Assets/Api/UpdateAssetTest.php b/tests/Feature/Assets/Api/UpdateAssetTest.php
@@ -423,6 +423,86 @@ public function testCheckoutToUserOnAssetUpdate()
         $this->assertEquals($asset->assigned_type, 'App\Models\User');
     }
 
+    public function testCheckoutToUserWithAssignedToAndAssignedType()
+    {
+        $asset = Asset::factory()->create();
+        $user = User::factory()->editAssets()->create();
+        $assigned_user = User::factory()->create();
+
+        $response = $this->actingAsForApi($user)
+            ->patchJson(route('api.assets.update', $asset->id), [
+                'assigned_to' => $assigned_user->id,
+                'assigned_type' => User::class
+            ])
+            ->assertOk()
+            ->assertStatusMessageIs('success')
+            ->json();
+
+        $asset->refresh();
+        $this->assertEquals($assigned_user->id, $asset->assigned_to);
+        $this->assertEquals($asset->assigned_type, 'App\Models\User');
+    }
+
+    public function testCheckoutToUserWithAssignedToWithoutAssignedType()
+    {
+        $asset = Asset::factory()->create();
+        $user = User::factory()->editAssets()->create();
+        $assigned_user = User::factory()->create();
+
+        $response = $this->actingAsForApi($user)
+            ->patchJson(route('api.assets.update', $asset->id), [
+                'assigned_to' => $assigned_user->id,
+//                'assigned_type' => User::class //deliberately omit assigned_type
+            ])
+            ->assertOk()
+            ->assertStatusMessageIs('error');
+
+        $asset->refresh();
+        $this->assertNotEquals($assigned_user->id, $asset->assigned_to);
+        $this->assertNotEquals($asset->assigned_type, 'App\Models\User');
+        $this->assertNotNull($response->json('messages.assigned_type'));
+    }
+
+    public function testCheckoutToUserWithAssignedToWithBadAssignedType()
+    {
+        $asset = Asset::factory()->create();
+        $user = User::factory()->editAssets()->create();
+        $assigned_user = User::factory()->create();
+
+        $response = $this->actingAsForApi($user)
+            ->patchJson(route('api.assets.update', $asset->id), [
+                'assigned_to'   => $assigned_user->id,
+                'assigned_type' => 'more_deliberate_nonsense' //deliberately bad assigned_type
+            ])
+            ->assertOk()
+            ->assertStatusMessageIs('error');
+
+        $asset->refresh();
+        $this->assertNotEquals($assigned_user->id, $asset->assigned_to);
+        $this->assertNotEquals($asset->assigned_type, 'App\Models\User');
+        $this->assertNotNull($response->json('messages.assigned_type'));
+    }
+
+    public function testCheckoutToUserWithoutAssignedToWithAssignedType()
+    {
+        $asset = Asset::factory()->create();
+        $user = User::factory()->editAssets()->create();
+        $assigned_user = User::factory()->create();
+
+        $response = $this->actingAsForApi($user)
+            ->patchJson(route('api.assets.update', $asset->id), [
+                //'assigned_to'   => $assigned_user->id, // deliberately omit assigned_to
+                'assigned_type' => User::class
+            ])
+            ->assertOk()
+            ->assertStatusMessageIs('error');
+
+        $asset->refresh();
+        $this->assertNotEquals($assigned_user->id, $asset->assigned_to);
+        $this->assertNotEquals($asset->assigned_type, 'App\Models\User');
+        $this->assertNotNull($response->json('messages.assigned_to'));
+    }
+
     public function testCheckoutToDeletedUserFailsOnAssetUpdate()
     {
         $asset = Asset::factory()->create();

diff --git a/tests/Unit/AssetTest.php b/tests/Unit/AssetTest.php
@@ -4,6 +4,7 @@
 use App\Models\Asset;
 use App\Models\AssetModel;
 use App\Models\Category;
+use App\Models\User;
 use Carbon\Carbon;
 use Tests\TestCase;
 use App\Models\Setting;
@@ -189,4 +190,13 @@ public function testWarrantyExpiresAttribute()
         $this->assertEquals(Carbon::createFromDate(2019, 1, 1)->format('Y-m-d'), $asset->warranty_expires->format('Y-m-d'));
 
     }
+
+    public function testAssignedTypeWithoutAssignTo()
+    {
+        $user = User::factory()->create();
+        $asset = Asset::factory()->create([
+            'assigned_to' => $user->id
+        ]);
+        $this->assertModelMissing($asset);
+    }
 }