diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php index d08880759c..7482c03f17 100644 --- a/app/Http/Controllers/Api/AssetsController.php +++ b/app/Http/Controllers/Api/AssetsController.php @@ -4,6 +4,10 @@ namespace App\Http\Controllers\Api; use App\Events\CheckoutableCheckedIn; use App\Http\Requests\StoreAssetRequest; +use App\Http\Traits\MigratesLegacyAssetLocations; +use App\Models\CheckoutAcceptance; +use App\Models\LicenseSeat; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\JsonResponse; use Illuminate\Support\Facades\Crypt; use Illuminate\Support\Facades\Gate; @@ -45,6 +49,8 @@ use Route; */ class AssetsController extends Controller { + use MigratesLegacyAssetLocations; + /** * Returns JSON listing of all assets * @@ -864,11 +870,9 @@ class AssetsController extends Controller */ public function checkin(Request $request, $asset_id) { - $this->authorize('checkin', Asset::class); $asset = Asset::with('model')->findOrFail($asset_id); $this->authorize('checkin', $asset); - $target = $asset->assignedTo; if (is_null($target)) { return response()->json(Helper::formatStandardApiResponse('error', [ @@ -881,7 +885,6 @@ class AssetsController extends Controller $asset->expected_checkin = null; //$asset->last_checkout = null; $asset->last_checkin = now(); - $asset->assigned_to = null; $asset->assignedTo()->disassociate($asset); $asset->accepted = null; @@ -889,10 +892,16 @@ class AssetsController extends Controller $asset->name = $request->input('name'); } + $this->migrateLegacyLocations($asset); + $asset->location_id = $asset->rtd_location_id; if ($request->filled('location_id')) { $asset->location_id = $request->input('location_id'); + + if ($request->input('update_default_location')){ + $asset->rtd_location_id = $request->input('location_id'); + } } if ($request->has('status_id')) { @@ -906,12 +915,22 @@ class AssetsController extends Controller $originalValues['action_date'] = $checkin_at; } - if(!empty($asset->licenseseats->all())){ - foreach ($asset->licenseseats as $seat){ - $seat->assigned_to = null; - $seat->save(); - } - } + $asset->licenseseats->each(function (LicenseSeat $seat) { + $seat->update(['assigned_to' => null]); + }); + + // Get all pending Acceptances for this asset and delete them + CheckoutAcceptance::pending() + ->whereHasMorph( + 'checkoutable', + [Asset::class], + function (Builder $query) use ($asset) { + $query->where('id', $asset->id); + }) + ->get() + ->map(function ($acceptance) { + $acceptance->delete(); + }); if ($asset->save()) { event(new CheckoutableCheckedIn($asset, $target, Auth::user(), $request->input('note'), $checkin_at, $originalValues)); diff --git a/app/Http/Controllers/Assets/AssetCheckinController.php b/app/Http/Controllers/Assets/AssetCheckinController.php index a8ed32deb1..82cb98abe9 100644 --- a/app/Http/Controllers/Assets/AssetCheckinController.php +++ b/app/Http/Controllers/Assets/AssetCheckinController.php @@ -6,8 +6,10 @@ use App\Events\CheckoutableCheckedIn; use App\Helpers\Helper; use App\Http\Controllers\Controller; use App\Http\Requests\AssetCheckinRequest; +use App\Http\Traits\MigratesLegacyAssetLocations; use App\Models\Asset; use App\Models\CheckoutAcceptance; +use App\Models\LicenseSeat; use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Redirect; @@ -15,6 +17,8 @@ use Illuminate\Support\Facades\View; class AssetCheckinController extends Controller { + use MigratesLegacyAssetLocations; + /** * Returns a view that presents a form to check an asset back into inventory. * @@ -69,9 +73,7 @@ class AssetCheckinController extends Controller $asset->expected_checkin = null; //$asset->last_checkout = null; $asset->last_checkin = now(); - $asset->assigned_to = null; $asset->assignedTo()->disassociate($asset); - $asset->assigned_type = null; $asset->accepted = null; $asset->name = $request->get('name'); @@ -79,24 +81,7 @@ class AssetCheckinController extends Controller $asset->status_id = e($request->get('status_id')); } - // This is just meant to correct legacy issues where some user data would have 0 - // as a location ID, which isn't valid. Later versions of Snipe-IT have stricter validation - // rules, so it's necessary to fix this for long-time users. It's kinda gross, but will help - // people (and their data) in the long run - - if ($asset->rtd_location_id == '0') { - \Log::debug('Manually override the RTD location IDs'); - \Log::debug('Original RTD Location ID: '.$asset->rtd_location_id); - $asset->rtd_location_id = ''; - \Log::debug('New RTD Location ID: '.$asset->rtd_location_id); - } - - if ($asset->location_id == '0') { - \Log::debug('Manually override the location IDs'); - \Log::debug('Original Location ID: '.$asset->location_id); - $asset->location_id = ''; - \Log::debug('New Location ID: '.$asset->location_id); - } + $this->migrateLegacyLocations($asset); $asset->location_id = $asset->rtd_location_id; @@ -117,12 +102,9 @@ class AssetCheckinController extends Controller $checkin_at = $request->get('checkin_at'); } - if(!empty($asset->licenseseats->all())){ - foreach ($asset->licenseseats as $seat){ - $seat->assigned_to = null; - $seat->save(); - } - } + $asset->licenseseats->each(function (LicenseSeat $seat) { + $seat->update(['assigned_to' => null]); + }); // Get all pending Acceptances for this asset and delete them $acceptances = CheckoutAcceptance::pending()->whereHasMorph('checkoutable', diff --git a/app/Http/Traits/MigratesLegacyAssetLocations.php b/app/Http/Traits/MigratesLegacyAssetLocations.php new file mode 100644 index 0000000000..13b464d0ca --- /dev/null +++ b/app/Http/Traits/MigratesLegacyAssetLocations.php @@ -0,0 +1,33 @@ +rtd_location_id == '0') { + \Log::debug('Manually override the RTD location IDs'); + \Log::debug('Original RTD Location ID: ' . $asset->rtd_location_id); + $asset->rtd_location_id = ''; + \Log::debug('New RTD Location ID: ' . $asset->rtd_location_id); + } + + if ($asset->location_id == '0') { + \Log::debug('Manually override the location IDs'); + \Log::debug('Original Location ID: ' . $asset->location_id); + $asset->location_id = ''; + \Log::debug('New Location ID: ' . $asset->location_id); + } + } +} diff --git a/database/factories/AssetFactory.php b/database/factories/AssetFactory.php index 432495bcc7..5461303c88 100644 --- a/database/factories/AssetFactory.php +++ b/database/factories/AssetFactory.php @@ -8,7 +8,6 @@ use App\Models\Location; use App\Models\Statuslabel; use App\Models\Supplier; use App\Models\User; -use Carbon\Carbon; use Carbon\CarbonImmutable; use Illuminate\Database\Eloquent\Factories\Factory; @@ -289,12 +288,13 @@ class AssetFactory extends Factory }); } - public function assignedToUser() + public function assignedToUser(User $user = null) { - return $this->state(function () { + return $this->state(function () use ($user) { return [ - 'assigned_to' => User::factory(), + 'assigned_to' => $user->id ?? User::factory(), 'assigned_type' => User::class, + 'last_checkout' => now()->subDay(), ]; }); } @@ -352,4 +352,19 @@ class AssetFactory extends Factory { return $this->state(['requestable' => false]); } + + /** + * This allows bypassing model level validation if you want to purposefully + * create an asset in an invalid state. Validation is turned back on + * after the model is created via the factory. + * @return AssetFactory + */ + public function canBeInvalidUponCreation() + { + return $this->afterMaking(function (Asset $asset) { + $asset->setValidating(false); + })->afterCreating(function (Asset $asset) { + $asset->setValidating(true); + }); + } } diff --git a/database/factories/LicenseSeatFactory.php b/database/factories/LicenseSeatFactory.php index 3c6cc4246b..cd9acfee1b 100644 --- a/database/factories/LicenseSeatFactory.php +++ b/database/factories/LicenseSeatFactory.php @@ -3,6 +3,7 @@ namespace Database\Factories; use App\Models\License; +use App\Models\User; use Illuminate\Database\Eloquent\Factories\Factory; class LicenseSeatFactory extends Factory @@ -13,4 +14,13 @@ class LicenseSeatFactory extends Factory 'license_id' => License::factory(), ]; } + + public function assignedToUser(User $user = null) + { + return $this->state(function () use ($user) { + return [ + 'assigned_to' => $user->id ?? User::factory(), + ]; + }); + } } diff --git a/tests/Feature/Api/Assets/AssetCheckinTest.php b/tests/Feature/Api/Assets/AssetCheckinTest.php index f71191d80c..27d5e6de80 100644 --- a/tests/Feature/Api/Assets/AssetCheckinTest.php +++ b/tests/Feature/Api/Assets/AssetCheckinTest.php @@ -2,8 +2,15 @@ namespace Tests\Feature\Api\Assets; +use App\Events\CheckoutableCheckedIn; use App\Models\Asset; +use App\Models\CheckoutAcceptance; +use App\Models\LicenseSeat; +use App\Models\Location; +use App\Models\Statuslabel; use App\Models\User; +use Illuminate\Support\Carbon; +use Illuminate\Support\Facades\Event; use Tests\Support\InteractsWithSettings; use Tests\TestCase; @@ -11,20 +18,150 @@ class AssetCheckinTest extends TestCase { use InteractsWithSettings; - public function testLastCheckInFieldIsSetOnCheckin() + public function testCheckingInAssetRequiresCorrectPermission() { - $admin = User::factory()->superuser()->create(); - $asset = Asset::factory()->create(['last_checkin' => null]); + $this->actingAsForApi(User::factory()->create()) + ->postJson(route('api.asset.checkin', Asset::factory()->assignedToUser()->create())) + ->assertForbidden(); + } - $asset->checkOut(User::factory()->create(), $admin, now()); + public function testCannotCheckInNonExistentAsset() + { + $this->actingAsForApi(User::factory()->checkinAssets()->create()) + ->postJson(route('api.asset.checkin', ['id' => 'does-not-exist'])) + ->assertStatusMessageIs('error'); + } - $this->actingAsForApi($admin) - ->postJson(route('api.asset.checkin', $asset)) + public function testCannotCheckInAssetThatIsNotCheckedOut() + { + $this->actingAsForApi(User::factory()->checkinAssets()->create()) + ->postJson(route('api.asset.checkin', Asset::factory()->create()->id)) + ->assertStatusMessageIs('error'); + } + + public function testAssetCanBeCheckedIn() + { + Event::fake([CheckoutableCheckedIn::class]); + + $user = User::factory()->create(); + $location = Location::factory()->create(); + $status = Statuslabel::factory()->create(); + $asset = Asset::factory()->assignedToUser($user)->create([ + 'expected_checkin' => now()->addDay(), + 'last_checkin' => null, + 'accepted' => 'accepted', + ]); + + $this->assertTrue($asset->assignedTo->is($user)); + + $currentTimestamp = now(); + + $this->actingAsForApi(User::factory()->checkinAssets()->create()) + ->postJson(route('api.asset.checkin', $asset), [ + 'name' => 'Changed Name', + 'status_id' => $status->id, + 'location_id' => $location->id, + ]) ->assertOk(); - $this->assertNotNull( - $asset->fresh()->last_checkin, - 'last_checkin field should be set on checkin' - ); + $this->assertNull($asset->refresh()->assignedTo); + $this->assertNull($asset->expected_checkin); + $this->assertNull($asset->last_checkout); + $this->assertNotNull($asset->last_checkin); + $this->assertNull($asset->assignedTo); + $this->assertNull($asset->assigned_type); + $this->assertNull($asset->accepted); + $this->assertEquals('Changed Name', $asset->name); + $this->assertEquals($status->id, $asset->status_id); + $this->assertTrue($asset->location()->is($location)); + + Event::assertDispatched(function (CheckoutableCheckedIn $event) use ($currentTimestamp) { + // this could be better mocked but is ok for now. + return Carbon::parse($event->action_date)->diffInSeconds($currentTimestamp) < 2; + }, 1); + } + + public function testLocationIsSetToRTDLocationByDefaultUponCheckin() + { + $rtdLocation = Location::factory()->create(); + $asset = Asset::factory()->assignedToUser()->create([ + 'location_id' => Location::factory()->create()->id, + 'rtd_location_id' => $rtdLocation->id, + ]); + + $this->actingAsForApi(User::factory()->checkinAssets()->create()) + ->postJson(route('api.asset.checkin', $asset->id)); + + $this->assertTrue($asset->refresh()->location()->is($rtdLocation)); + } + + public function testDefaultLocationCanBeUpdatedUponCheckin() + { + $location = Location::factory()->create(); + $asset = Asset::factory()->assignedToUser()->create(); + + $this->actingAsForApi(User::factory()->checkinAssets()->create()) + ->postJson(route('api.asset.checkin', $asset), [ + 'location_id' => $location->id, + 'update_default_location' => true, + ]); + + $this->assertTrue($asset->refresh()->defaultLoc()->is($location)); + } + + public function testAssetsLicenseSeatsAreClearedUponCheckin() + { + $asset = Asset::factory()->assignedToUser()->create(); + LicenseSeat::factory()->assignedToUser()->for($asset)->create(); + + $this->assertNotNull($asset->licenseseats->first()->assigned_to); + + $this->actingAsForApi(User::factory()->checkinAssets()->create()) + ->postJson(route('api.asset.checkin', $asset)); + + $this->assertNull($asset->refresh()->licenseseats->first()->assigned_to); + } + + public function testLegacyLocationValuesSetToZeroAreUpdated() + { + $asset = Asset::factory()->canBeInvalidUponCreation()->assignedToUser()->create([ + 'rtd_location_id' => 0, + 'location_id' => 0, + ]); + + $this->actingAsForApi(User::factory()->checkinAssets()->create()) + ->postJson(route('api.asset.checkin', $asset)); + + $this->assertNull($asset->refresh()->rtd_location_id); + $this->assertEquals($asset->location_id, $asset->rtd_location_id); + } + + public function testPendingCheckoutAcceptancesAreClearedUponCheckin() + { + $asset = Asset::factory()->assignedToUser()->create(); + + $acceptance = CheckoutAcceptance::factory()->for($asset, 'checkoutable')->pending()->create(); + + $this->actingAsForApi(User::factory()->checkinAssets()->create()) + ->postJson(route('api.asset.checkin', $asset)); + + $this->assertFalse($acceptance->exists(), 'Acceptance was not deleted'); + } + + public function testCheckinTimeAndActionLogNoteCanBeSet() + { + Event::fake(); + + $this->actingAsForApi(User::factory()->checkinAssets()->create()) + ->postJson(route('api.asset.checkin', Asset::factory()->assignedToUser()->create()), [ + // time is appended to the provided date in controller + 'checkin_at' => '2023-01-02', + 'note' => 'hi there', + ]); + + Event::assertDispatched(function (CheckoutableCheckedIn $event) { + return Carbon::parse('2023-01-02')->isSameDay(Carbon::parse($event->action_date)) + && $event->note === 'hi there'; + }, 1); } } diff --git a/tests/Feature/Assets/AssetCheckinTest.php b/tests/Feature/Assets/AssetCheckinTest.php deleted file mode 100644 index 059fb1294f..0000000000 --- a/tests/Feature/Assets/AssetCheckinTest.php +++ /dev/null @@ -1,32 +0,0 @@ -superuser()->create(); - $asset = Asset::factory()->create(['last_checkin' => null]); - - $asset->checkOut(User::factory()->create(), $admin, now()); - - $this->actingAs($admin) - ->post(route('hardware.checkin.store', [ - 'assetId' => $asset->id, - ])) - ->assertRedirect(); - - $this->assertNotNull( - $asset->fresh()->last_checkin, - 'last_checkin field should be set on checkin' - ); - } -} diff --git a/tests/Feature/Checkins/AssetCheckinTest.php b/tests/Feature/Checkins/AssetCheckinTest.php new file mode 100644 index 0000000000..a8e26312be --- /dev/null +++ b/tests/Feature/Checkins/AssetCheckinTest.php @@ -0,0 +1,167 @@ +actingAs(User::factory()->create()) + ->post(route('hardware.checkin.store', [ + 'assetId' => Asset::factory()->assignedToUser()->create()->id, + ])) + ->assertForbidden(); + } + + public function testCannotCheckInAssetThatIsNotCheckedOut() + { + $this->actingAs(User::factory()->checkinAssets()->create()) + ->post(route('hardware.checkin.store', ['assetId' => Asset::factory()->create()->id])) + ->assertSessionHas('error') + ->assertRedirect(route('hardware.index')); + } + + public function testAssetCanBeCheckedIn() + { + Event::fake([CheckoutableCheckedIn::class]); + + $user = User::factory()->create(); + $location = Location::factory()->create(); + $status = Statuslabel::first() ?? Statuslabel::factory()->create(); + $asset = Asset::factory()->assignedToUser($user)->create([ + 'expected_checkin' => now()->addDay(), + 'last_checkin' => null, + 'accepted' => 'accepted', + ]); + + $this->assertTrue($asset->assignedTo->is($user)); + + $currentTimestamp = now(); + + $this->actingAs(User::factory()->checkinAssets()->create()) + ->post( + route('hardware.checkin.store', ['assetId' => $asset->id, 'backto' => 'user']), + [ + 'name' => 'Changed Name', + 'status_id' => $status->id, + 'location_id' => $location->id, + ], + ) + ->assertRedirect(route('users.show', $user)); + + $this->assertNull($asset->refresh()->assignedTo); + $this->assertNull($asset->expected_checkin); + $this->assertNull($asset->last_checkout); + $this->assertNotNull($asset->last_checkin); + $this->assertNull($asset->assignedTo); + $this->assertNull($asset->assigned_type); + $this->assertNull($asset->accepted); + $this->assertEquals('Changed Name', $asset->name); + $this->assertEquals($status->id, $asset->status_id); + $this->assertTrue($asset->location()->is($location)); + + Event::assertDispatched(function (CheckoutableCheckedIn $event) use ($currentTimestamp) { + // this could be better mocked but is ok for now. + return Carbon::parse($event->action_date)->diffInSeconds($currentTimestamp) < 2; + }, 1); + } + + public function testLocationIsSetToRTDLocationByDefaultUponCheckin() + { + $rtdLocation = Location::factory()->create(); + $asset = Asset::factory()->assignedToUser()->create([ + 'location_id' => Location::factory()->create()->id, + 'rtd_location_id' => $rtdLocation->id, + ]); + + $this->actingAs(User::factory()->checkinAssets()->create()) + ->post(route('hardware.checkin.store', ['assetId' => $asset->id])); + + $this->assertTrue($asset->refresh()->location()->is($rtdLocation)); + } + + public function testDefaultLocationCanBeUpdatedUponCheckin() + { + $location = Location::factory()->create(); + $asset = Asset::factory()->assignedToUser()->create(); + + $this->actingAs(User::factory()->checkinAssets()->create()) + ->post(route('hardware.checkin.store', ['assetId' => $asset->id]), [ + 'location_id' => $location->id, + 'update_default_location' => 0 + ]); + + $this->assertTrue($asset->refresh()->defaultLoc()->is($location)); + } + + public function testAssetsLicenseSeatsAreClearedUponCheckin() + { + $asset = Asset::factory()->assignedToUser()->create(); + LicenseSeat::factory()->assignedToUser()->for($asset)->create(); + + $this->assertNotNull($asset->licenseseats->first()->assigned_to); + + $this->actingAs(User::factory()->checkinAssets()->create()) + ->post(route('hardware.checkin.store', ['assetId' => $asset->id])); + + $this->assertNull($asset->refresh()->licenseseats->first()->assigned_to); + } + + public function testLegacyLocationValuesSetToZeroAreUpdated() + { + $asset = Asset::factory()->canBeInvalidUponCreation()->assignedToUser()->create([ + 'rtd_location_id' => 0, + 'location_id' => 0, + ]); + + $this->actingAs(User::factory()->checkinAssets()->create()) + ->post(route('hardware.checkin.store', ['assetId' => $asset->id])); + + $this->assertNull($asset->refresh()->rtd_location_id); + $this->assertEquals($asset->location_id, $asset->rtd_location_id); + } + + public function testPendingCheckoutAcceptancesAreClearedUponCheckin() + { + $asset = Asset::factory()->assignedToUser()->create(); + + $acceptance = CheckoutAcceptance::factory()->for($asset, 'checkoutable')->pending()->create(); + + $this->actingAs(User::factory()->checkinAssets()->create()) + ->post(route('hardware.checkin.store', ['assetId' => $asset->id])); + + $this->assertFalse($acceptance->exists(), 'Acceptance was not deleted'); + } + + public function testCheckinTimeAndActionLogNoteCanBeSet() + { + Event::fake([CheckoutableCheckedIn::class]); + + $this->actingAs(User::factory()->checkinAssets()->create()) + ->post(route( + 'hardware.checkin.store', + ['assetId' => Asset::factory()->assignedToUser()->create()->id] + ), [ + 'checkin_at' => '2023-01-02', + 'note' => 'hello' + ]); + + Event::assertDispatched(function (CheckoutableCheckedIn $event) { + return $event->action_date === '2023-01-02' && $event->note === 'hello'; + }, 1); + } +} diff --git a/tests/Feature/Notifications/AssetWebhookTest.php b/tests/Feature/Notifications/AssetWebhookTest.php index 158bced664..95218f98e7 100644 --- a/tests/Feature/Notifications/AssetWebhookTest.php +++ b/tests/Feature/Notifications/AssetWebhookTest.php @@ -108,6 +108,54 @@ class AssetWebhookTest extends TestCase Notification::assertNotSentTo(new AnonymousNotifiable, CheckinAssetNotification::class); } + public function testCheckInEmailSentToUserIfSettingEnabled() + { + Notification::fake(); + + $user = User::factory()->create(); + $asset = Asset::factory()->assignedToUser($user)->create(); + + $asset->model->category->update(['checkin_email' => true]); + + event(new CheckoutableCheckedIn( + $asset, + $user, + User::factory()->checkinAssets()->create(), + '' + )); + + Notification::assertSentTo( + [$user], + function (CheckinAssetNotification $notification, $channels) { + return in_array('mail', $channels); + }, + ); + } + + public function testCheckInEmailNotSentToUserIfSettingDisabled() + { + Notification::fake(); + + $user = User::factory()->create(); + $asset = Asset::factory()->assignedToUser($user)->create(); + + $asset->model->category->update(['checkin_email' => false]); + + event(new CheckoutableCheckedIn( + $asset, + $user, + User::factory()->checkinAssets()->create(), + '' + )); + + Notification::assertNotSentTo( + [$user], + function (CheckinAssetNotification $notification, $channels) { + return in_array('mail', $channels); + } + ); + } + private function createAsset() { return Asset::factory()->laptopMbp()->create();