Merge pull request #16946 from marcusmoore/bug/sc-29166

Gracefully handle error when editing of soft deleted users

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/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,
+        ]);
+    }
 }