diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 613524d5c3..b18a7b6fff 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -178,7 +178,7 @@ class UsersController extends Controller * @author [A. Gianotto] [] * @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 @@ class UsersController extends Controller if ($user) { + if ($user->trashed()) { + return redirect()->route('users.show', $user->id); + } + $permissions = config('permissions'); $groups = Group::pluck('name', 'id'); diff --git a/app/Listeners/CheckoutableListener.php b/app/Listeners/CheckoutableListener.php index 4c204a9c7b..227f6cd278 100644 --- a/app/Listeners/CheckoutableListener.php +++ b/app/Listeners/CheckoutableListener.php @@ -76,8 +76,9 @@ class CheckoutableListener * 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 @@ class CheckoutableListener $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 diff --git a/app/Rules/UserCannotSwitchCompaniesIfItemsAssigned.php b/app/Rules/UserCannotSwitchCompaniesIfItemsAssigned.php index a433ee9a28..b31bc91d1b 100644 --- a/app/Rules/UserCannotSwitchCompaniesIfItemsAssigned.php +++ b/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')) { diff --git a/tests/Feature/Users/Ui/UpdateUserTest.php b/tests/Feature/Users/Ui/UpdateUserTest.php index abaec1b70e..4728531be6 100644 --- a/tests/Feature/Users/Ui/UpdateUserTest.php +++ b/tests/Feature/Users/Ui/UpdateUserTest.php @@ -5,6 +5,7 @@ namespace Tests\Feature\Users\Ui; 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 @@ class UpdateUserTest extends TestCase ->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 @@ class UpdateUserTest extends TestCase $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, + ]); + } }