Merge pull request #14697 from snipe/bug/sc-25502/disable_delete_if_not_deletable_user

Fixed UI where delete button was not disabled even if the user couldn't be deleted

app/Http/Controllers/Api/UsersController.php

@@ -75,8 +75,8 @@ class UsersController extends Controller
             'users.autoassign_licenses',
             'users.website',
 
-        ])->with('manager', 'groups', 'userloc', 'company', 'department', 'assets', 'licenses', 'accessories', 'consumables', 'createdBy',)
+        ])->with('manager', 'groups', 'userloc', 'company', 'department', 'assets', 'licenses', 'accessories', 'consumables', 'createdBy')
-            ->withCount('assets as assets_count', 'licenses as licenses_count', 'accessories as accessories_count', 'consumables as consumables_count');
+            ->withCount('assets as assets_count', 'licenses as licenses_count', 'accessories as accessories_count', 'consumables as consumables_count', 'managesUsers as manages_users_count', 'managedLocations as manages_locations_count');
 
 
         if ($request->filled('activated')) {
@@ -187,6 +187,14 @@ class UsersController extends Controller
             $users->has('accessories', '=', $request->input('accessories_count'));
         }
 
+        if ($request->filled('manages_users_count')) {
+            $users->has('manages_users_count', '=', $request->input('manages_users_count'));
+        }
+
+        if ($request->filled('manages_locations_count')) {
+            $users->has('manages_locations_count', '=', $request->input('manages_locations_count'));
+        }
+
         if ($request->filled('autoassign_licenses')) {
             $users->where('autoassign_licenses', '=', $request->input('autoassign_licenses'));
         }
@@ -244,6 +252,8 @@ class UsersController extends Controller
                         'licenses_count',
                         'consumables_count',
                         'accessories_count',
+                        'manages_user_count',
+                        'manages_locations_count',
                         'phone',
                         'address',
                         'city',
@@ -405,11 +415,15 @@ class UsersController extends Controller
     {
         $this->authorize('view', User::class);
 
-        $user = User::withCount('assets as assets_count', 'licenses as licenses_count', 'accessories as accessories_count', 'consumables as consumables_count')->findOrFail($id);
+        $user = User::withCount('assets as assets_count', 'licenses as licenses_count', 'accessories as accessories_count', 'consumables as consumables_count', 'managesUsers as manages_users_count', 'managedLocations as manages_locations_count');
-        $user = Company::scopeCompanyables($user)->find($id);
+
-        $this->authorize('update', $user);
+        if ($user = Company::scopeCompanyables($user)->find($id)) {
+            $this->authorize('view', $user);
+            return (new UsersTransformer)->transformUser($user);
+        }
+        
+        return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id'))));
 
-        return (new UsersTransformer)->transformUser($user);
     }
 
 
@@ -470,7 +484,6 @@ class UsersController extends Controller
         }
 
 
-
         // Update the location of any assets checked out to this user
         Asset::where('assigned_type', User::class)
             ->where('assigned_to', $user->id)->update(['location_id' => $request->input('location_id', null)]);
@@ -480,12 +493,6 @@ class UsersController extends Controller
           
         if ($user->save()) {
 
-            // Sync group memberships:
-            // This was changed in Snipe-IT v4.6.x to 4.7, since we upgraded to Laravel 5.5
-            // which changes the behavior of has vs filled.
-            // The $request->has method will now return true even if the input value is an empty string or null.
-            // A new $request->filled method has was added that provides the previous behavior of the has method.
-
             // Check if the request has groups passed and has a value
             if ($request->filled('groups')) {
 

app/Http/Transformers/UsersTransformer.php

@@ -21,6 +21,7 @@ class UsersTransformer
 
     public function transformUser(User $user)
     {
+
         $array = [
                 'id' => (int) $user->id,
                 'avatar' => e($user->present()->gravatar),
@@ -64,6 +65,8 @@ class UsersTransformer
                 'licenses_count' => (int) $user->licenses_count,
                 'accessories_count' => (int) $user->accessories_count,
                 'consumables_count' => (int) $user->consumables_count,
+                'manages_users_count' => (int) $user->manages_users_count,
+                'manages_locations_count' => (int) $user->manages_locations_count,
                 'company' => ($user->company) ? ['id' => (int) $user->company->id, 'name'=> e($user->company->name)] : null,
                 'created_by' => ($user->createdBy) ? [
                     'id' => (int) $user->createdBy->id,

app/Models/User.php

@@ -214,10 +214,12 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo
     public function isDeletable()
     {
         return Gate::allows('delete', $this)
-            && ($this->assets()->count() === 0)
+            && ($this->assets->count() === 0)
-            && ($this->licenses()->count() === 0)
+            && ($this->licenses->count() === 0)
-            && ($this->consumables()->count() === 0)
+            && ($this->consumables->count() === 0)
-            && ($this->accessories()->count() === 0)
+            && ($this->accessories->count() === 0)
+            && ($this->managedLocations->count() === 0)
+            && ($this->managesUsers->count() === 0)
             && ($this->deleted_at == '');
     }
 
@@ -410,6 +412,19 @@ class User extends SnipeModel implements AuthenticatableContract, AuthorizableCo
         return $this->belongsTo(self::class, 'manager_id')->withTrashed();
     }
 
+    /**
+     * Establishes the user -> managed users relationship
+     *
+     * @author A. Gianotto <snipe@snipe.net>
+     * @since [v6.4.1]
+     * @return \Illuminate\Database\Eloquent\Relations\Relation
+     */
+    public function managesUsers()
+    {
+        return $this->hasMany(\App\Models\User::class, 'manager_id');
+    }
+
+
     /**
      * Establishes the user -> managed locations relationship
      *

app/Presenters/UserPresenter.php

@@ -221,7 +221,7 @@ class UserPresenter extends Presenter
                 'switchable' => true,
                 'escape' => true,
                 'class' => 'css-barcode',
-                'title' => 'Assets',
+                'title' => trans('general.assets'),
                 'visible' => true,
             ],
             [
@@ -230,7 +230,7 @@ class UserPresenter extends Presenter
                 'sortable' => true,
                 'switchable' => true,
                 'class' => 'css-license',
-                'title' => 'License',
+                'title' => trans('general.licenses'),
                 'visible' => true,
             ],
             [
@@ -239,7 +239,7 @@ class UserPresenter extends Presenter
                 'sortable' => true,
                 'switchable' => true,
                 'class' => 'css-consumable',
-                'title' => 'Consumables',
+                'title' => trans('general.consumables'),
                 'visible' => true,
             ],
             [
@@ -248,7 +248,25 @@ class UserPresenter extends Presenter
                 'sortable' => true,
                 'switchable' => true,
                 'class' => 'css-accessory',
-                'title' => 'Accessories',
+                'title' => trans('general.accessories'),
+                'visible' => true,
+            ],
+            [
+                'field' => 'manages_users_count',
+                'searchable' => false,
+                'sortable' => true,
+                'switchable' => true,
+                'class' => 'css-users',
+                'title' => trans('admin/users/table.managed_users'),
+                'visible' => true,
+            ],
+            [
+                'field' => 'manages_locations_count',
+                'searchable' => false,
+                'sortable' => true,
+                'switchable' => true,
+                'class' => 'css-location',
+                'title' => trans('admin/users/table.managed_locations'),
                 'visible' => true,
             ],
             [

public/mix-manifest.json

@@ -1,8 +1,8 @@
 {
     "/js/build/app.js": "/js/build/app.js?id=ea5f3edebafdb29b616d23fa89106080",
     "/css/dist/skins/skin-blue.css": "/css/dist/skins/skin-blue.css?id=f677207c6cf9678eb539abecb408c374",
-    "/css/build/overrides.css": "/css/build/overrides.css?id=be3c0a217bc6c0e0744f75faed784887",
+    "/css/build/overrides.css": "/css/build/overrides.css?id=3d1aa807fc9395794b76f4cdab99c984",
-    "/css/build/app.css": "/css/build/app.css?id=a168b0a799aa800ee926bffa1b1a434a",
+    "/css/build/app.css": "/css/build/app.css?id=40e80d931c21cde71b27be4c8eaaea62",
     "/css/build/AdminLTE.css": "/css/build/AdminLTE.css?id=dc383f8560a8d4adb51d44fb4043e03b",
     "/css/dist/skins/skin-orange.css": "/css/dist/skins/skin-orange.css?id=6f0563e726c2fe4fab4026daaa5bfdf2",
     "/css/dist/skins/skin-orange-dark.css": "/css/dist/skins/skin-orange-dark.css?id=620b684d9dd9d3bb5fdda00a3a2467c3",
@@ -18,7 +18,7 @@
     "/css/dist/skins/skin-green.css": "/css/dist/skins/skin-green.css?id=0a82a6ae6bb4e58fe62d162c4fb50397",
     "/css/dist/skins/skin-contrast.css": "/css/dist/skins/skin-contrast.css?id=da6c7997d9de2f8329142399f0ce50da",
     "/css/dist/skins/skin-red.css": "/css/dist/skins/skin-red.css?id=44bf834f2110504a793dadec132a5898",
-    "/css/dist/all.css": "/css/dist/all.css?id=6e9aa08b535262053b95aee495caa7df",
+    "/css/dist/all.css": "/css/dist/all.css?id=b51ba67a606c04c8f12fa30adcf33fd0",
     "/css/dist/signature-pad.css": "/css/dist/signature-pad.css?id=6a89d3cd901305e66ced1cf5f13147f7",
     "/css/dist/signature-pad.min.css": "/css/dist/signature-pad.min.css?id=6a89d3cd901305e66ced1cf5f13147f7",
     "/css/webfonts/fa-brands-400.ttf": "/css/webfonts/fa-brands-400.ttf?id=0141634c24336be626e05c8b77d1fa27",

resources/assets/less/overrides.less

@@ -586,6 +586,8 @@ th.css-barcode > .th-inner,
 th.css-license > .th-inner,
 th.css-consumable > .th-inner,
 th.css-envelope > .th-inner,
+th.css-users > .th-inner,
+th.css-location > .th-inner,
 th.css-accessory > .th-inner
 {
   font-size: 0px;
@@ -602,6 +604,8 @@ th.css-barcode > .th-inner::before,
 th.css-license > .th-inner::before,
 th.css-consumable > .th-inner::before,
 th.css-envelope > .th-inner::before,
+th.css-users > .th-inner::before,
+th.css-location > .th-inner::before,
 th.css-accessory > .th-inner::before
 
 {
@@ -621,6 +625,7 @@ th.css-padlock > .th-inner::before
 }
 
 /**
+BEGIN ICON TABLE HEADERS
 Set the font-weight css property as 900 (For Solid), 400 (Regular or Brands), 300 (Light for pro icons).
 **/
 th.css-barcode > .th-inner::before
@@ -643,12 +648,20 @@ th.css-envelope > .th-inner::before
   content: "\f0e0"; font-family: "Font Awesome 5 Free"; font-weight: 400;
 }
 
-
 th.css-accessory > .th-inner::before
 {
   content: "\f11c"; font-family: "Font Awesome 5 Free"; font-weight: 400;
 }
 
+th.css-users > .th-inner::before {
+  content: "\f0c0"; font-family: "Font Awesome 5 Free"; font-size: 15px;
+}
+
+th.css-location > .th-inner::before {
+  content: "\f3c5"; font-family: "Font Awesome 5 Free"; font-size: 19px; margin-bottom: 0px;
+}
+
+
 .small-box .inner {
   padding-left: 15px;
   padding-right: 15px;

resources/lang/en-US/admin/users/table.php

@@ -20,6 +20,7 @@ return array(
     'lock_passwords'		=> 'Login details cannot be changed on this installation.',
     'manager' 				=> 'Manager',
     'managed_locations'     => 'Managed Locations',
+    'managed_users'         => 'Managed Users',
     'name' 					=> 'Name',
     'nogroup'               => 'No groups have been created yet. To add one, visit: ',
     'notes'                 => 'Notes',

resources/views/users/view.blade.php

@@ -89,9 +89,9 @@
           </a>
         </li>
 
-        @if ($user->managedLocations()->count() >= 0 )
+        @if ($user->managedLocations->count() >= 0 )
         <li>
-          <a href="#managed" data-toggle="tab">
+          <a href="#managed-locations" data-toggle="tab">
             <span class="hidden-lg hidden-md">
               <i class="fas fa-map-marker-alt fa-2x"></i></span>
             <span class="hidden-xs hidden-sm">{{ trans('admin/users/table.managed_locations') }}
@@ -100,7 +100,19 @@
         </li>
         @endif
 
-        @can('update', $user)
+          @if ($user->managesUsers->count() >= 0 )
+              <li>
+                  <a href="#managed-users" data-toggle="tab">
+            <span class="hidden-lg hidden-md">
+              <i class="fa-solid fa-users fa-2x"></i></span>
+                      <span class="hidden-xs hidden-sm">{{ trans('admin/users/table.managed_users') }}
+                      {!! ($user->managesUsers->count() > 0 ) ? '<badge class="badge badge-secondary">'.number_format($user->managesUsers->count()).'</badge>' : '' !!}
+                  </a>
+              </li>
+          @endif
+
+
+      @can('update', $user)
           <li class="dropdown pull-right">
             <a class="dropdown-toggle" data-toggle="dropdown" href="#">
               <span class="hidden-xs"><i class="fas fa-cog" aria-hidden="true"></i></span>
@@ -114,7 +126,7 @@
             <ul class="dropdown-menu">
               <li><a href="{{ route('users.edit', $user->id) }}">{{ trans('admin/users/general.edit') }}</a></li>
               <li><a href="{{ route('users.clone.show', $user->id) }}">{{ trans('admin/users/general.clone') }}</a></li>
-              @if ((Auth::user()->id !== $user->id) && (!config('app.lock_passwords')) && ($user->deleted_at==''))
+              @if ((Auth::user()->id !== $user->id) && (!config('app.lock_passwords')) && ($user->deleted_at=='') && ($user->isDeletable()))
                 <li><a href="{{ route('users.destroy', $user->id) }}">{{ trans('button.delete') }}</a></li>
               @endif
             </ul>
@@ -221,11 +233,15 @@
                 @can('delete', $user)
                   @if ($user->deleted_at=='')
                     <div class="col-md-12" style="padding-top: 30px;">
-                      <form action="{{route('users.destroy',$user->id)}}" method="POST">
+                        @if ($user->isDeletable())
-                        {{csrf_field()}}
+                          <form action="{{route('users.destroy',$user->id)}}" method="POST">
-                        {{ method_field("DELETE")}}
+                            {{csrf_field()}}
-                        <button style="width: 100%;" class="btn btn-sm btn-warning hidden-print">{{ trans('button.delete')}}</button>
+                            {{ method_field("DELETE")}}
-                      </form>
+                            <button style="width: 100%;" class="btn btn-sm btn-warning hidden-print">{{ trans('button.delete')}}</button>
+                          </form>
+                            @else
+                            <button style="width: 100%;" class="btn btn-sm btn-warning hidden-print disabled">{{ trans('button.delete')}}</button>
+                        @endif
                     </div>
                     <div class="col-md-12" style="padding-top: 5px;">
                       <form action="{{ route('users/bulkedit') }}" method="POST">
@@ -1014,7 +1030,7 @@
           </div>
         </div><!-- /.tab-pane -->
 
-        <div class="tab-pane" id="managed">
+        <div class="tab-pane" id="managed-locations">
 
             @include('partials.locations-bulk-actions')
 
@@ -1046,6 +1062,39 @@
             </table>
 
           </div>
+
+          <div class="tab-pane" id="managed-users">
+
+              @include('partials.locations-bulk-actions')
+
+
+              <table
+                      data-columns="{{ \App\Presenters\UserPresenter::dataTableLayout() }}"
+                      data-cookie-id-table="managedUsersTable"
+                      data-click-to-select="true"
+                      data-pagination="true"
+                      data-id-table="managedUsersTable"
+                      data-toolbar="#usersBulkEditToolbar"
+                      data-bulk-button-id="#bulkUsersEditButton"
+                      data-bulk-form-id="#usersBulkForm"
+                      data-search="true"
+                      data-show-footer="true"
+                      data-side-pagination="server"
+                      data-show-columns="true"
+                      data-show-fullscreen="true"
+                      data-show-export="true"
+                      data-show-refresh="true"
+                      data-sort-order="asc"
+                      id="managedUsersTable"
+                      class="table table-striped snipe-table"
+                      data-url="{{ route('api.users.index', ['manager_id' => $user->id]) }}"
+                      data-export-options='{
+              "fileName": "export-users-{{ date('Y-m-d') }}",
+              "ignoreColumn": ["actions","image","change","checkbox","checkincheckout","icon"]
+              }'>
+              </table>
+
+          </div>
         </div><!-- /consumables-tab -->
       </div><!-- /.tab-content -->
     </div><!-- nav-tabs-custom -->

tests/Feature/Api/Users/UsersDeleteTest.php

@@ -0,0 +1,42 @@
+<?php
+
+namespace Tests\Feature\Api\Users;
+
+use App\Models\Location;
+use App\Models\User;
+use Laravel\Passport\Passport;
+use Tests\TestCase;
+
+class UsersDeleteTest extends TestCase
+{
+
+
+    public function testDisallowUserDeletionIfStillManagingPeople()
+    {
+        $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']);
+        User::factory()->create(['first_name' => 'Lowly', 'last_name' => 'Worker', 'manager_id' => $manager->id]);
+        $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable());
+    }
+
+    public function testDisallowUserDeletionIfStillManagingLocations()
+    {
+        $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']);
+        Location::factory()->create(['manager_id' => $manager->id]);
+        $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable());
+    }
+
+    public function testAllowUserDeletionIfNotManagingLocations()
+    {
+        $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']);
+        $this->actingAs(User::factory()->deleteUsers()->create())->assertTrue($manager->isDeletable());
+    }
+
+    public function testDisallowUserDeletionIfNoDeletePermissions()
+    {
+        $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']);
+        Location::factory()->create(['manager_id' => $manager->id]);
+        $this->actingAs(User::factory()->editUsers()->create())->assertFalse($manager->isDeletable());
+    }
+
+
+}