Merge pull request #15221 from marcusmoore/fixes/bulk-checkin-logging

Fixed accessories and consumables not being logged correctly during bulk check-in

app/Http/Controllers/Users/BulkUsersController.php

@@ -16,6 +16,7 @@ use App\Models\Consumable;
 use App\Models\User;
 use Carbon\Carbon;
 use Illuminate\Http\Request;
+use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Auth;
 use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Password;
@@ -219,20 +220,18 @@ class BulkUsersController extends Controller
 
         $users = User::whereIn('id', $user_raw_array)->get();
         $assets = Asset::whereIn('assigned_to', $user_raw_array)->where('assigned_type', User::class)->get();
-        $accessories = DB::table('accessories_checkout')->where('assigned_type', User::class)->whereIn('assigned_to', $user_raw_array)->get();
+        $accessoryUserRows = DB::table('accessories_checkout')->where('assigned_type', User::class)->whereIn('assigned_to', $user_raw_array)->get();
         $licenses = DB::table('license_seats')->whereIn('assigned_to', $user_raw_array)->get();
-        $consumables = DB::table('consumables_users')->whereIn('assigned_to', $user_raw_array)->get();
+        $consumableUserRows = DB::table('consumables_users')->whereIn('assigned_to', $user_raw_array)->get();
 
         if ((($assets->count() > 0) && ((!$request->filled('status_id')) || ($request->input('status_id') == '')))) {
             return redirect()->route('users.index')->with('error', 'No status selected');
         }
 
-
         $this->logItemCheckinAndDelete($assets, Asset::class);
-        $this->logItemCheckinAndDelete($accessories, Accessory::class);
+        $this->logAccessoriesCheckin($accessoryUserRows);
         $this->logItemCheckinAndDelete($licenses, License::class);
-        $this->logItemCheckinAndDelete($consumables, Consumable::class);
-
+        $this->logConsumablesCheckin($consumableUserRows);
 
         Asset::whereIn('id', $assets->pluck('id'))->update([
             'status_id'     => e(request('status_id')),
@@ -241,19 +240,14 @@ class BulkUsersController extends Controller
             'expected_checkin' => null,
         ]);
 
-
         LicenseSeat::whereIn('id', $licenses->pluck('id'))->update(['assigned_to' => null]);
-        ConsumableAssignment::whereIn('id', $consumables->pluck('id'))->delete();
-
+        ConsumableAssignment::whereIn('id', $consumableUserRows->pluck('id'))->delete();
 
         foreach ($users as $user) {
-
-            $user->consumables()->sync([]);
             $user->accessories()->sync([]);
             if ($request->input('delete_user')=='1') {
                 $user->delete();
             }
-
         }
 
         $msg = trans('general.bulk_checkin_success');
@@ -279,7 +273,7 @@ class BulkUsersController extends Controller
             if ($itemType == License::class){
                 $item_id = $item->license_id;
             }
-            
+
             $logAction->item_id = $item_id;
             // We can't rely on get_class here because the licenses/accessories fetched above are not eloquent models, but simply arrays.
             $logAction->item_type = $itemType;
@@ -291,6 +285,34 @@ class BulkUsersController extends Controller
         }
     }
 
+    private function logAccessoriesCheckin(Collection $accessoryUserRows): void
+    {
+        foreach ($accessoryUserRows as $accessoryUserRow) {
+            $logAction = new Actionlog();
+            $logAction->item_id = $accessoryUserRow->accessory_id;
+            $logAction->item_type = Accessory::class;
+            $logAction->target_id = $accessoryUserRow->assigned_to;
+            $logAction->target_type = User::class;
+            $logAction->user_id = Auth::id();
+            $logAction->note = 'Bulk checkin items';
+            $logAction->logaction('checkin from');
+        }
+    }
+
+    private function logConsumablesCheckin(Collection $consumableUserRows): void
+    {
+        foreach ($consumableUserRows as $consumableUserRow) {
+            $logAction = new Actionlog();
+            $logAction->item_id = $consumableUserRow->consumable_id;
+            $logAction->item_type = Consumable::class;
+            $logAction->target_id = $consumableUserRow->assigned_to;
+            $logAction->target_type = User::class;
+            $logAction->user_id = Auth::id();
+            $logAction->note = 'Bulk checkin items';
+            $logAction->logaction('checkin from');
+        }
+    }
+
     /**
      * Save bulk-edited users
      *

tests/Feature/Users/Ui/BulkActions/BulkDeleteUsersTest.php

@@ -0,0 +1,260 @@
+<?php
+
+namespace Tests\Feature\Users\Ui\BulkActions;
+
+use App\Models\Accessory;
+use App\Models\Asset;
+use App\Models\Consumable;
+use App\Models\License;
+use App\Models\LicenseSeat;
+use App\Models\Statuslabel;
+use App\Models\User;
+use Illuminate\Database\Eloquent\Model;
+use Tests\TestCase;
+
+class BulkDeleteUsersTest extends TestCase
+{
+    public function test_requires_correct_permission()
+    {
+        $this->actingAs(User::factory()->create())
+            ->post(route('users/bulksave'), [
+                'ids' => [
+                    User::factory()->create()->id,
+                ],
+                'status_id' => Statuslabel::factory()->create()->id,
+            ])
+            ->assertForbidden();
+    }
+
+    public function test_validation()
+    {
+        $user = User::factory()->create();
+        Asset::factory()->assignedToUser($user)->create();
+
+        $actor = $this->actingAs(User::factory()->editUsers()->create());
+
+        // "ids" required
+        $actor->post(route('users/bulksave'), [
+            // 'ids' => [
+            //     $user->id,
+            // ],
+            'status_id' => Statuslabel::factory()->create()->id,
+        ])->assertSessionHas('error')->assertRedirect();
+
+        // "status_id" needed when provided users have assets associated
+        $actor->post(route('users/bulksave'), [
+            'ids' => [
+                $user->id,
+            ],
+            // 'status_id' => Statuslabel::factory()->create()->id,
+        ])->assertSessionHas('error')->assertRedirect();
+    }
+
+    public function test_cannot_perform_bulk_actions_on_self()
+    {
+        $actor = User::factory()->editUsers()->create();
+
+        $this->actingAs($actor)
+            ->post(route('users/bulksave'), [
+                'ids' => [
+                    $actor->id,
+                ],
+                'delete_user' => '1',
+            ])
+            ->assertRedirect(route('users.index'))
+            ->assertSessionHas('success', trans('general.bulk_checkin_delete_success'));
+
+        $this->assertNotSoftDeleted($actor);
+    }
+
+    public function test_accessories_can_be_bulk_checked_in()
+    {
+        [$accessoryA, $accessoryB] = Accessory::factory()->count(2)->create();
+        [$userA, $userB, $userC] = User::factory()->count(3)->create();
+
+        // Add checkouts for multiple accessories to multiple users to get different ids in the mix
+        $this->attachAccessoryToUsers($accessoryA, [$userA, $userB, $userC]);
+        $this->attachAccessoryToUsers($accessoryB, [$userA, $userB]);
+
+        $this->actingAs(User::factory()->editUsers()->create())
+            ->post(route('users/bulksave'), [
+                'ids' => [
+                    $userA->id,
+                    $userC->id,
+                ],
+                'status_id' => Statuslabel::factory()->create()->id,
+            ])
+            ->assertRedirect(route('users.index'));
+
+        $this->assertTrue($userA->fresh()->accessories->isEmpty());
+        $this->assertTrue($userB->fresh()->accessories->isNotEmpty());
+        $this->assertTrue($userC->fresh()->accessories->isEmpty());
+
+        // These assertions check against a bug where the wrong value from
+        // accessories_users was being populated in action_logs.item_id.
+        $this->assertActionLogCheckInEntryFor($userA, $accessoryA);
+        $this->assertActionLogCheckInEntryFor($userA, $accessoryB);
+        $this->assertActionLogCheckInEntryFor($userC, $accessoryA);
+    }
+
+    public function test_assets_can_be_bulk_checked_in()
+    {
+        [$userA, $userB, $userC] = User::factory()->count(3)->create();
+
+        $assetForUserA = $this->assignAssetToUser($userA);
+        $lonelyAsset = $this->assignAssetToUser($userB);
+        $assetForUserC = $this->assignAssetToUser($userC);
+
+        $this->actingAs(User::factory()->editUsers()->create())
+            ->post(route('users/bulksave'), [
+                'ids' => [
+                    $userA->id,
+                    $userC->id,
+                ],
+                'status_id' => Statuslabel::factory()->create()->id,
+            ])
+            ->assertRedirect(route('users.index'));
+
+        $this->assertTrue($userA->fresh()->assets->isEmpty());
+        $this->assertTrue($userB->fresh()->assets->isNotEmpty());
+        $this->assertTrue($userC->fresh()->assets->isEmpty());
+
+        $this->assertActionLogCheckInEntryFor($userA, $assetForUserA);
+        $this->assertActionLogCheckInEntryFor($userC, $assetForUserC);
+    }
+
+    public function test_consumables_can_be_bulk_checked_in()
+    {
+        [$consumableA, $consumableB] = Consumable::factory()->count(2)->create();
+        [$userA, $userB, $userC] = User::factory()->count(3)->create();
+
+        // Add checkouts for multiple consumables to multiple users to get different ids in the mix
+        $this->attachConsumableToUsers($consumableA, [$userA, $userB, $userC]);
+        $this->attachConsumableToUsers($consumableB, [$userA, $userB]);
+
+        $this->actingAs(User::factory()->editUsers()->create())
+            ->post(route('users/bulksave'), [
+                'ids' => [
+                    $userA->id,
+                    $userC->id,
+                ],
+                'status_id' => Statuslabel::factory()->create()->id,
+            ])
+            ->assertRedirect(route('users.index'));
+
+        $this->assertTrue($userA->fresh()->consumables->isEmpty());
+        $this->assertTrue($userB->fresh()->consumables->isNotEmpty());
+        $this->assertTrue($userC->fresh()->consumables->isEmpty());
+
+        // These assertions check against a bug where the wrong value from
+        // consumables_users was being populated in action_logs.item_id.
+        $this->assertActionLogCheckInEntryFor($userA, $consumableA);
+        $this->assertActionLogCheckInEntryFor($userA, $consumableB);
+        $this->assertActionLogCheckInEntryFor($userC, $consumableA);
+    }
+
+    public function test_license_seats_can_be_bulk_checked_in()
+    {
+        [$userA, $userB, $userC] = User::factory()->count(3)->create();
+
+        $licenseSeatForUserA = LicenseSeat::factory()->assignedToUser($userA)->create();
+        $lonelyLicenseSeat = LicenseSeat::factory()->assignedToUser($userB)->create();
+        $licenseSeatForUserC = LicenseSeat::factory()->assignedToUser($userC)->create();
+
+        $this->actingAs(User::factory()->editUsers()->create())
+            ->post(route('users/bulksave'), [
+                'ids' => [
+                    $userA->id,
+                    $userC->id,
+                ],
+            ])
+            ->assertRedirect(route('users.index'))
+            ->assertSessionHas('success', trans('general.bulk_checkin_success'));
+
+        $this->assertDatabaseMissing('license_seats', [
+            'license_id' => $licenseSeatForUserA->license->id,
+            'assigned_to' => $userA->id,
+        ]);
+
+        $this->assertDatabaseMissing('license_seats', [
+            'license_id' => $licenseSeatForUserC->license->id,
+            'assigned_to' => $userC->id,
+        ]);
+
+        // Slightly different from the other assertions since we use
+        // the license and not the license seat in this case.
+        $this->assertDatabaseHas('action_logs', [
+            'action_type' => 'checkin from',
+            'target_id' => $userA->id,
+            'target_type' => User::class,
+            'note' => 'Bulk checkin items',
+            'item_type' => License::class,
+            'item_id' => $licenseSeatForUserA->license->id,
+        ]);
+
+        $this->assertDatabaseHas('action_logs', [
+            'action_type' => 'checkin from',
+            'target_id' => $userC->id,
+            'target_type' => User::class,
+            'note' => 'Bulk checkin items',
+            'item_type' => License::class,
+            'item_id' => $licenseSeatForUserC->license->id,
+        ]);
+    }
+
+    public function test_users_can_be_deleted_in_bulk()
+    {
+        [$userA, $userB, $userC] = User::factory()->count(3)->create();
+
+        $this->actingAs(User::factory()->editUsers()->create())
+            ->post(route('users/bulksave'), [
+                'ids' => [
+                    $userA->id,
+                    $userC->id,
+                ],
+                'delete_user' => '1',
+            ])
+            ->assertRedirect(route('users.index'))
+            ->assertSessionHas('success', trans('general.bulk_checkin_delete_success'));
+
+        $this->assertSoftDeleted($userA);
+        $this->assertNotSoftDeleted($userB);
+        $this->assertSoftDeleted($userC);
+    }
+
+    private function assignAssetToUser(User $user): Asset
+    {
+        return Asset::factory()->assignedToUser($user)->create();
+    }
+
+    private function attachAccessoryToUsers(Accessory $accessory, array $users): void
+    {
+        foreach ($users as $user) {
+            $accessoryCheckout = $accessory->checkouts()->make();
+            $accessoryCheckout->assignedTo()->associate($user);
+            $accessoryCheckout->save();
+        }
+    }
+
+    private function attachConsumableToUsers(Consumable $consumable, array $users): void
+    {
+        foreach ($users as $user) {
+            $consumable->users()->attach($consumable->id, [
+                'consumable_id' => $consumable->id,
+                'assigned_to' => $user->id,
+            ]);
+        }
+    }
+
+    private function assertActionLogCheckInEntryFor(User $user, Model $model): void
+    {
+        $this->assertDatabaseHas('action_logs', [
+            'action_type' => 'checkin from',
+            'target_id' => $user->id,
+            'target_type' => User::class,
+            'note' => 'Bulk checkin items',
+            'item_type' => get_class($model),
+            'item_id' => $model->id,
+        ]);
+    }
+}