Fixes nested location selectlist (#7483)

* Rename child locations method

* Use Ajax dropdown for locations selectlist for edit/create

* Removed locations database call on edit/create blades for faster loading

* Updated locations controller to use the new iterator

* Increase pagination on locations controller to 500

We’re already loading all of that data up beforehand anyway, so no point in keeping the query smaller.

* Fixed the else to make codacy happy

* Improve the design and performance of the nested location selectlist (#7484)

* Improve the design and performance of the nested location selectlist

* Fixed parse errors

* Removed debugging code/comments

app/Http/Controllers/Api/LocationsController.php

@@ -8,6 +8,8 @@ use App\Helpers\Helper;
 use App\Models\Location;
 use App\Http\Transformers\LocationsTransformer;
 use App\Http\Transformers\SelectlistTransformer;
+use Illuminate\Pagination\LengthAwarePaginator;
+use Illuminate\Support\Collection;
 
 class LocationsController extends Controller
 {
@@ -26,7 +28,7 @@ class LocationsController extends Controller
                 'updated_at','manager_id','image',
                 'assigned_assets_count','users_count','assets_count','currency'];
 
-        $locations = Location::with('parent', 'manager', 'childLocations')->select([
+        $locations = Location::with('parent', 'manager', 'children')->select([
             'locations.id',
             'locations.name',
             'locations.address',
@@ -109,7 +111,7 @@ class LocationsController extends Controller
     public function show($id)
     {
         $this->authorize('view', Location::class);
-        $location = Location::with('parent', 'manager', 'childLocations')
+        $location = Location::with('parent', 'manager', 'children')
             ->select([
                 'locations.id',
                 'locations.name',
@@ -181,6 +183,27 @@ class LocationsController extends Controller
     /**
      * Gets a paginated collection for the select2 menus
      *
+     * This is handled slightly differently as of ~4.7.8-pre, as
+     * we have to do some recursive magic to get the hierarchy to display
+     * properly when looking at the parent/child relationship in the
+     * rich menus.
+     *
+     * This means we can't use the normal pagination that we use elsewhere
+     * in our selectlists, since we have to get the full set before we can
+     * determine which location is parent/child/grandchild, etc.
+     *
+     * This also means that hierarchy display gets a little funky when people
+     * use the Select2 search functionality, but there's not much we can do about
+     * that right now.
+     *
+     * As a result, instead of paginating as part of the query, we have to grab
+     * the entire data set, and then invoke a paginator manually and pass that
+     * through to the SelectListTransformer.
+     *
+     * Many thanks to @uberbrady for the help getting this working better.
+     * Recursion still sucks, but I guess he doesn't have to get in the
+     * sea... this time.
+     *
      * @author [A. Gianotto] [<snipe@snipe.net>]
      * @since [v4.0.16]
      * @see \App\Http\Transformers\SelectlistTransformer
@@ -192,25 +215,39 @@ class LocationsController extends Controller
         $locations = Location::select([
             'locations.id',
             'locations.name',
+            'locations.parent_id',
             'locations.image',
         ]);
 
+        $page = 1;
+        if ($request->filled('page')) {
+            $page = $request->input('page');
+        }
+
         if ($request->filled('search')) {
-            $locations = $locations->where('locations.name', 'LIKE', '%'.$request->get('search').'%');
+            \Log::debug('Searching... ');
+            $locations = $locations->where('locations.name', 'LIKE', '%'.$request->input('search').'%');
         }
 
-        $locations = $locations->orderBy('name', 'ASC')->paginate(50);
+        $locations = $locations->orderBy('name', 'ASC')->get();
 
-        // Loop through and set some custom properties for the transformer to use.
-        // This lets us have more flexibility in special cases like assets, where
-        // they may not have a ->name value but we want to display something anyway
+        $locations_with_children = [];
         foreach ($locations as $location) {
-            $location->use_text = $location->name;
-            $location->use_image = ($location->image) ? url('/').'/uploads/locations/'.$location->image : null;
+            if(!array_key_exists($location->parent_id, $locations_with_children)) {
+                $locations_with_children[$location->parent_id] = [];
+            }
+            $locations_with_children[$location->parent_id][] = $location;
         }
 
-        return (new SelectlistTransformer)->transformSelectlist($locations);
+        $location_options = Location::indenter($locations_with_children);
+
+        $locations_formatted = new Collection($location_options);
+        $paginated_results =  new LengthAwarePaginator($locations_formatted->forPage($page, 500), $locations_formatted->count(), 500, $page, []);
+
+        //return [];
+        return (new SelectlistTransformer)->transformSelectlist($paginated_results);
 
     }
 
+
 }

app/Http/Controllers/LocationsController.php

@@ -57,14 +57,7 @@ class LocationsController extends Controller
     public function create()
     {
         $this->authorize('create', Location::class);
-        $locations = Location::orderBy('name', 'ASC')->get();
-
-        $location_options_array = Location::getLocationHierarchy($locations);
-        $location_options = Location::flattenLocationsArray($location_options_array);
-        $location_options = array('' => 'Top Level') + $location_options;
-
         return view('locations/edit')
-            ->with('location_options', $location_options)
             ->with('item', new Location);
     }
 
@@ -130,14 +123,8 @@ class LocationsController extends Controller
             return redirect()->route('locations.index')->with('error', trans('admin/locations/message.does_not_exist'));
         }
 
-        // Show the page
-        $locations = Location::orderBy('name', 'ASC')->get();
-        $location_options_array = Location::getLocationHierarchy($locations);
-        $location_options = Location::flattenLocationsArray($location_options_array);
-        $location_options = array('' => 'Top Level') + $location_options;
 
-        return view('locations/edit', compact('item'))
-            ->with('location_options', $location_options);
+        return view('locations/edit', compact('item'));
     }
 
 
@@ -227,7 +214,7 @@ class LocationsController extends Controller
         if ($location->users->count() > 0) {
             return redirect()->to(route('locations.index'))->with('error', trans('admin/locations/message.assoc_users'));
 
-        } elseif ($location->childLocations->count() > 0) {
+        } elseif ($location->children->count() > 0) {
             return redirect()->to(route('locations.index'))->with('error', trans('admin/locations/message.assoc_child_loc'));
 
         } elseif ($location->assets->count() > 0) {

app/Http/Transformers/LocationsTransformer.php

@@ -23,7 +23,7 @@ class LocationsTransformer
         if ($location) {
 
             $children_arr = [];
-            foreach($location->childLocations as $child) {
+            foreach($location->children as $child) {
                 $children_arr[] = [
                     'id' => (int) $child->id,
                     'name' => $child->name

app/Models/Location.php

@@ -113,7 +113,8 @@ class Location extends SnipeModel
 
     public function parent()
     {
-        return $this->belongsTo('\App\Models\Location', 'parent_id','id');
+        return $this->belongsTo('\App\Models\Location', 'parent_id','id')
+            ->with('parent');
     }
 
     public function manager()
@@ -121,9 +122,9 @@ class Location extends SnipeModel
         return $this->belongsTo('\App\Models\User', 'manager_id');
     }
 
-    public function childLocations()
-    {
-        return $this->hasMany('\App\Models\Location', 'parent_id');
+    public function children() {
+        return $this->hasMany('\App\Models\Location','parent_id')
+            ->with('children');
     }
 
     // I don't think we need this anymore since we de-normed location_id in assets?
@@ -137,59 +138,37 @@ class Location extends SnipeModel
         return $this->attributes['ldap_ou'] = empty($ldap_ou) ? null : $ldap_ou;
     }
 
-    public static function getLocationHierarchy($locations, $parent_id = null)
-    {
 
+    /**
+     * Query builder scope to order on parent
+     *
+     * @param  Illuminate\Database\Query\Builder  $query  Query builder instance
+     * @param  text                              $order       Order
+     *
+     * @return Illuminate\Database\Query\Builder          Modified query builder
+     */
 
-        $op = array();
-
-        foreach ($locations as $location) {
-
-            if ($location['parent_id'] == $parent_id) {
-                $op[$location['id']] =
-                    array(
-                        'name' => $location['name'],
-                        'parent_id' => $location['parent_id']
-                    );
-
-                // Using recursion
-                $children =  Location::getLocationHierarchy($locations, $location['id']);
-                if ($children) {
-                    $op[$location['id']]['children'] = $children;
-                }
-
-            }
-
+    public static function indenter($locations_with_children, $parent_id = null, $prefix = '') {
+        $results = Array();
+        
+        if (!array_key_exists($parent_id, $locations_with_children)) {
+            return [];
         }
-        return $op;
+
+        foreach ($locations_with_children[$parent_id] as $location) {
+            $location->use_text = $prefix.' '.$location->name;
+            $location->use_image = ($location->image) ? url('/').'/uploads/locations/'.$location->image : null;
+            $results[] = $location;
+            //now append the children. (if we have any)
+            if (array_key_exists($location->id, $locations_with_children)) {
+                $results = array_merge($results, Location::indenter($locations_with_children, $location->id,$prefix.'--'));
+            }
+        }
+        return $results;
     }
 
 
-    public static function flattenLocationsArray($location_options_array = null)
-    {
-        $location_options = array();
-        foreach ($location_options_array as $id => $value) {
 
-            // get the top level key value
-            $location_options[$id] = $value['name'];
-
-                // If there is a key named children, it has child locations and we have to walk it
-            if (array_key_exists('children', $value)) {
-
-                foreach ($value['children'] as $child_id => $child_location_array) {
-                    $child_location_options = Location::flattenLocationsArray($value['children']);
-
-                    foreach ($child_location_options as $child_id => $child_name) {
-                        $location_options[$child_id] = '--'.$child_name;
-                    }
-                }
-
-            }
-
-        }
-
-        return $location_options;
-    }
 
     /**
     * Query builder scope to order on parent

resources/views/locations/edit.blade.php

@@ -10,16 +10,8 @@
 @section('inputFields')
 @include ('partials.forms.edit.name', ['translated_name' => trans('admin/locations/table.name')])
 
-<!-- Parent-->
-<div class="form-group {{ $errors->has('parent_id') ? ' has-error' : '' }}">
-    <label for="parent_id" class="col-md-3 control-label">
-        {{ trans('admin/locations/table.parent') }}
-    </label>
-    <div class="col-md-9{{  (\App\Helpers\Helper::checkIfRequired($item, 'parent_id')) ? ' required' : '' }}">
-        {!! Form::select('parent_id', $location_options , Input::old('parent_id', $item->parent_id), array('class'=>'select2 parent', 'style'=>'width:350px')) !!}
-        {!! $errors->first('parent_id', '<span class="alert-msg"><i class="fa fa-times"></i> :message</span>') !!}
-    </div>
-</div>
+<!-- parent -->
+@include ('partials.forms.edit.location-select', ['translated_name' => trans('admin/locations/table.parent'), 'fieldname' => 'parent_id'])
 
 <!-- Manager-->
 @include ('partials.forms.edit.user-select', ['translated_name' => trans('admin/users/table.manager'), 'fieldname' => 'manager_id'])