Merge pull request #16954 from grokability/develop

Merge dev into master

Author: snipe

app/Http/Controllers/Users/UsersController.php

@@ -178,7 +178,7 @@ private function filterDisplayable($permissions)
      * @author [A. Gianotto] [<[email protected]>]
      * @since [v1.0]
      * @param $permissions
-     * @return \Illuminate\Contracts\View\View
+     * @return \Illuminate\Contracts\View\View|\Illuminate\Http\RedirectResponse
      * @internal param int $id
      * @throws \Illuminate\Auth\Access\AuthorizationException
      */
@@ -190,6 +190,10 @@ public function edit(User $user)
 
         if ($user) {
 
+            if ($user->trashed()) {
+                return redirect()->route('users.show', $user->id);
+            }
+
             $permissions = config('permissions');
             $groups = Group::pluck('name', 'id');
 

app/Listeners/CheckoutableListener.php

@@ -76,8 +76,9 @@ public function onCheckedOut($event)
              * 4. If the admin CC email is set, even if the item being checked out doesn't have an email address (location, etc)
              */
 
-            if ($event->checkoutable->requireAcceptance() || $event->checkoutable->getEula() ||
-                $this->checkoutableShouldSendEmail($event)) {
+            if ($event->checkoutable->requireAcceptance() /* does category require acceptance? */ ||
+                $event->checkoutable->getEula() /* is there *some* kind of EULA? */ ||
+                $this->checkoutableShouldSendEmail($event) /* does the category have 'checkin_email' [sic] set? */) {
 
 
                 // Send a checkout email to the admin CC addresses, even if the target has no email
@@ -171,30 +172,28 @@ public function onCheckedIn($event)
         $notifiable = $this->getNotifiableUsers($event);
 
         // Send email notifications
-        try {
-            /**
-             * Send an email if any of the following conditions are met:
-             * 1. The asset requires acceptance
-             * 2. The item has a EULA
-             * 3. The item should send an email at check-in/check-out
-             * 4. If the admin CC email is set, even if the item being checked in doesn't have an email address (location, etc)
-             */
+        if ($this->checkoutableShouldSendEmail($event)) {
+            try {
+                /**
+                 * Send a check-in n email *only* if the item should send an email at check-in/check-out
+                 */
 
-            // Send a checkout email to the admin's CC addresses, even if the target has no email
-            if (!empty($ccEmails)) {
-                Mail::to($ccEmails)->send($mailable);
-                Log::info('Checkin Mail sent to CC addresses');
-            }
+                // Send a checkout email to the admin's CC addresses, even if the target has no email
+                if (!empty($ccEmails)) {
+                    Mail::to($ccEmails)->send($mailable);
+                    Log::info('Checkin Mail sent to CC addresses');
+                }
 
-            // Send a checkout email to the target if it has an email
-            if (!empty($notifiable->email)) {
-                Mail::to($notifiable)->send($mailable);
-                Log::info('Checkin Mail sent to checkout target');
+                // Send a checkout email to the target if it has an email
+                if (!empty($notifiable->email)) {
+                    Mail::to($notifiable)->send($mailable);
+                    Log::info('Checkin Mail sent to checkout target');
+                }
+            } catch (ClientException $e) {
+                Log::debug("Exception caught during checkin email: " . $e->getMessage());
+            } catch (Exception $e) {
+                Log::debug("Exception caught during checkin email: " . $e->getMessage());
             }
-        } catch (ClientException $e) {
-            Log::debug("Exception caught during checkin email: " . $e->getMessage());
-        } catch (Exception $e) {
-            Log::debug("Exception caught during checkin email: " . $e->getMessage());
         }
 
         // Send Webhook notification

app/Rules/UserCannotSwitchCompaniesIfItemsAssigned.php

@@ -15,7 +15,7 @@ class UserCannotSwitchCompaniesIfItemsAssigned implements ValidationRule
      */
     public function validate(string $attribute, mixed $value, Closure $fail): void
     {
-        $user = User::find(request()->route('user')->id);
+        $user = request()->route('user');
 
         if (($value) && ($user->allAssignedCount() > 0) && (Setting::getSettings()->full_multiple_companies_support=='1')) {
 

tests/Feature/Users/Ui/UpdateUserTest.php

@@ -5,6 +5,7 @@
 use App\Models\Asset;
 use App\Models\Company;
 use App\Models\User;
+use Error;
 use Tests\TestCase;
 
 class UpdateUserTest extends TestCase
@@ -16,6 +17,15 @@ public function testPageRenders()
             ->assertOk();
     }
 
+    public function testCannotViewEditPageForSoftDeletedUser()
+    {
+        $user = User::factory()->trashed()->create();
+
+        $this->actingAs(User::factory()->superuser()->create())
+            ->get(route('users.edit', $user->id))
+            ->assertRedirectToRoute('users.show', $user->id);
+    }
+
     public function testUsersCanBeActivatedWithNumber()
     {
         $admin = User::factory()->superuser()->create();
@@ -161,4 +171,40 @@ public function testMultiCompanyUserCanBeUpdatedIfHasAssetInSameCompany()
 
         $this->followRedirects($response)->assertSee('success');
     }
+
+    /**
+     * This can occur if the user edit screen is open in one tab and
+     * the user is deleted in another before the edit form is submitted.
+     * @link https://app.shortcut.com/grokability/story/29166
+     */
+    public function testAttemptingToUpdateDeletedUserIsHandledGracefully()
+    {
+        [$companyA, $companyB] = Company::factory()->count(2)->create();
+        $user = User::factory()->for($companyA)->create();
+        Asset::factory()->assignedToUser($user)->create();
+
+        $id = $user->id;
+
+        $user->delete();
+
+        $response = $this->actingAs(User::factory()->editUsers()->create())
+            ->put(route('users.update', $id), [
+                'first_name' => 'test',
+                'username' => 'test',
+                'company_id' => $companyB->id,
+            ]);
+
+        $this->assertFalse($response->exceptions->contains(function ($exception) {
+            // Avoid hard 500
+            return $exception instanceof Error;
+        }));
+
+        // As of now, the user will be updated but not be restored
+        $this->assertDatabaseHas('users', [
+            'id' => $id,
+            'first_name' => 'test',
+            'username' => 'test',
+            'company_id' => $companyB->id,
+        ]);
+    }
 }