From eccdcc373eee3e8c55b9311fd4cdfb2897821b25 Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Mon, 7 Oct 2024 18:07:33 +0100 Subject: [PATCH] parent 2220828b00d5e5e579a13329498e999685ccee60 author Brady Wetherington 1728320853 +0100 committer Brady Wetherington 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 --- app/Http/Controllers/Api/AssetsController.php | 4 +- app/Http/Requests/StoreAssetRequest.php | 1 - app/Models/Asset.php | 3 +- app/Observers/AssetObserver.php | 5 +- tests/Feature/Assets/Api/AssetIndexTest.php | 8 +- tests/Feature/Assets/Api/StoreAssetTest.php | 65 +++++++++++++++ tests/Feature/Assets/Api/UpdateAssetTest.php | 80 +++++++++++++++++++ tests/Unit/AssetTest.php | 10 +++ 8 files changed, 167 insertions(+), 9 deletions(-) diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php index 7a6566d34e81..6e60cbada836 100644 --- 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 index fb7469ac88f9..66179ac739c4 100644 --- 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 index ce8b870eb2e0..862b99436252 100644 --- 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 index 0d01428ea8e9..a8f5f09ae8de 100644 --- 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 index c4a362d28b71..8554edb3d459 100644 --- 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 index ea5cfb617820..7136b546a398 100644 --- 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 index df4448a2db5c..e04f3f681144 100644 --- 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 index d0f3af623373..aea894f9fb94 100644 --- 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); + } }