Merge pull request #13959 from snipe/fixes/rebased_bulk_edit_fix

Fixed status and model in bulk edit

app/Http/Controllers/Assets/BulkAssetsController.php

@@ -7,6 +7,8 @@ use App\Helpers\Helper;
 use App\Http\Controllers\CheckInOutRequest;
 use App\Http\Controllers\Controller;
 use App\Models\Asset;
+use App\Models\AssetModel;
+use App\Models\Statuslabel;
 use App\Models\Setting;
 use App\View\Label;
 use Illuminate\Http\Request;
@@ -23,6 +25,13 @@ class BulkAssetsController extends Controller
     /**
      * Display the bulk edit page.
      *
+     * This method is super weird because it's kinda of like a controller within a controller.
+     * It's main function is to determine what the bulk action in, and then return a view with
+     * the information that view needs, be it bulk delete, bulk edit, restore, etc.
+     *
+     * This is something that made sense at the time, but sort of doesn't make sense now. A JS front-end to determine form
+     * action would make a lot more sense here and make things a lot more clear.
+     *
      * @author [A. Gianotto] [<snipe@snipe.net>]
      * @return View
      * @internal param int $assetId
@@ -33,6 +42,9 @@ class BulkAssetsController extends Controller
     {
         $this->authorize('view', Asset::class);
 
+        /**
+         * No asset IDs were passed
+         */
         if (! $request->filled('ids')) {
             return redirect()->back()->with('error', trans('admin/hardware/message.update.no_assets_selected'));
         }
@@ -41,40 +53,33 @@ class BulkAssetsController extends Controller
         $bulk_back_url = request()->headers->get('referer');
         session(['bulk_back_url' => $bulk_back_url]);
 
-        $asset_ids = array_values(array_unique($request->input('ids')));
 
-        //custom fields logic
+        $asset_ids = $request->input('ids');
-        $asset_custom_field = Asset::with(['model.fieldset.fields', 'model'])->whereIn('id', $asset_ids)->whereHas('model', function ($query) {
+        $assets = Asset::with('assignedTo', 'location', 'model')->find($asset_ids);
-            return $query->where('fieldset_id', '!=', null);
-        })->get();
 
-        $models = $asset_custom_field->unique('model_id');  
+        $models = $assets->unique('model_id');
         $modelNames = [];
         foreach($models as $model) {
             $modelNames[] = $model->model->name;
         }
 
         if ($request->filled('bulk_actions')) {
+
+
             switch ($request->input('bulk_actions')) {
                 case 'labels':
                     $this->authorize('view', Asset::class);
-                    $assets_found = Asset::find($asset_ids);
-                    
-                    if ($assets_found->isEmpty()){
-                        return redirect()->back();
-                    }
 
                     return (new Label)
-                        ->with('assets', $assets_found)
+                        ->with('assets', $assets)
                         ->with('settings', Setting::getSettings())
                         ->with('bulkedit', true)
                         ->with('count', 0);
 
                 case 'delete':
                     $this->authorize('delete', Asset::class);
-                    $assets = Asset::with('assignedTo', 'location')->find($asset_ids);
+                    $assets->each(function ($assets) {
-                    $assets->each(function ($asset) {
+                        $this->authorize('delete', $assets);
-                        $this->authorize('delete', $asset);
                     });
 
                     return view('hardware/bulk-delete')->with('assets', $assets);
@@ -85,11 +90,11 @@ class BulkAssetsController extends Controller
                     $assets->each(function ($asset) {
                         $this->authorize('delete', $asset);
                     });
-
                     return view('hardware/bulk-restore')->with('assets', $assets);
 
                 case 'edit':
                     $this->authorize('update', Asset::class);
+
                     return view('hardware/bulk')
                         ->with('assets', $asset_ids)
                         ->with('statuslabel_list', Helper::statusLabelList())
@@ -117,25 +122,31 @@ class BulkAssetsController extends Controller
 
         // Get the back url from the session and then destroy the session
         $bulk_back_url = route('hardware.index');
+
         if ($request->session()->has('bulk_back_url')) {
             $bulk_back_url = $request->session()->pull('bulk_back_url');
         }
 
        $custom_field_columns = CustomField::all()->pluck('db_column')->toArray();
 
-        if (Session::exists('ids')) {
+     
-            $assets = Session::get('ids'); 
+        if (! $request->filled('ids') || count($request->input('ids')) == 0) {
-        } elseif (! $request->filled('ids') || count($request->input('ids')) <= 0) {
             return redirect($bulk_back_url)->with('error', trans('admin/hardware/message.update.no_assets_selected'));
         }
 
-        $assets = array_keys($request->input('ids'));
 
-       if ($request->anyFilled($custom_field_columns)) {
+        $assets = Asset::whereIn('id', array_keys($request->input('ids')))->get();
-           $custom_fields_present = true;
+
-         } else {
+
-           $custom_fields_present = false;
+
-         }
+        /**
+         * If ANY of these are filled, prepare to update the values on the assets.
+         *
+         * Additional checks will be needed for some of them to make sure the values
+         * make sense (for example, changing the status ID to something incompatible with
+         * its checkout status.
+         */
+
         if (($request->filled('purchase_date'))
             || ($request->filled('expected_checkin'))
             || ($request->filled('purchase_cost'))
@@ -154,15 +165,22 @@ class BulkAssetsController extends Controller
             || ($request->anyFilled($custom_field_columns))
 
         ) {
-            foreach ($assets as $assetId) {
+            // Let's loop through those assets and build an update array
+            foreach ($assets as $asset) {
 
                 $this->update_array = [];
 
+                /**
+                 * Leave out model_id and status here because we do math on that later. We have to do some extra
+                 * validation and checks on those two.
+                 *
+                 * It's tempting to make these match the request check above, but some of these values require
+                 * extra work to make sure the data makes sense.
+                 */
                 $this->conditionallyAddItem('purchase_date')
                     ->conditionallyAddItem('expected_checkin')
                     ->conditionallyAddItem('order_number')
                     ->conditionallyAddItem('requestable')
-                    ->conditionallyAddItem('status_id')
                     ->conditionallyAddItem('supplier_id')
                     ->conditionallyAddItem('warranty_months')
                     ->conditionallyAddItem('next_audit_date');
@@ -170,6 +188,9 @@ class BulkAssetsController extends Controller
                         $this->conditionallyAddItem($custom_field_column); 
                    }
 
+                /**
+                 * Blank out fields that were requested to be blanked out via checkbox
+                 */
                 if ($request->input('null_purchase_date')=='1') {
                     $this->update_array['purchase_date'] = null;
                 }
@@ -186,7 +207,6 @@ class BulkAssetsController extends Controller
                     $this->update_array['purchase_cost'] =  $request->input('purchase_cost');
                 }
 
-
                 if ($request->filled('company_id')) {
                     $this->update_array['company_id'] = $request->input('company_id');
                     if ($request->input('company_id') == 'clear') {
@@ -194,48 +214,92 @@ class BulkAssetsController extends Controller
                     }
                 }
 
+                /**
+                 * We're trying to change the model ID - we need to do some extra checks here to make sure
+                 * the custom field values work for the custom fieldset rules around this asset. Uniqueness
+                 * and requiredness across the fieldset is particularly important, since those are
+                 * fieldset-specific attributes.
+                 */
+                if ($request->filled('model_id')) {
+                    $this->update_array['model_id'] = AssetModel::find($request->input('model_id'))->id;
+                }
+
+                /**
+                 * We're trying to change the status ID - we need to do some extra checks here to
+                 * make sure the status label type is one that makes sense for the state of the asset,
+                 * for example, we shouldn't be able to make an asset archived if it's currently assigned
+                 * to someone/something.
+                 */
+                if ($request->filled('status_id')) {
+                    $updated_status = Statuslabel::find($request->input('status_id'));
+
+                    // We cannot assign a non-deployable status type if the asset is already assigned.
+                    // This could probably be added to a form request.
+                    // If the asset isn't assigned, we don't care what the status is.
+                    // Otherwise we need to make sure the status type is still a deployable one.
+                    if (
+                        ($asset->assigned_to == '')
+                        || ($updated_status->deployable == '1') && ($asset->assetstatus->deployable == '1')
+                    ) {
+                        $this->update_array['status_id'] = $updated_status->id;
+                    }
+
+                }
+
+                /**
+                 * We're changing the location ID - figure out which location we should apply
+                 * this change to:
+                 *
+                 * 0 - RTD location only
+                 * 1 - location ID and RTD location ID
+                 * 2 - location ID only
+                 *
+                 * Note: this is kinda dumb and we should just use human-readable values IMHO. - snipe
+                 */
                 if ($request->filled('rtd_location_id')) {
+
                     if (($request->filled('update_real_loc')) && (($request->input('update_real_loc')) == '0')) {
                         $this->update_array['rtd_location_id'] = $request->input('rtd_location_id');
                     }
+
                     if (($request->filled('update_real_loc')) && (($request->input('update_real_loc')) == '1')) {
                         $this->update_array['location_id'] = $request->input('rtd_location_id');
                         $this->update_array['rtd_location_id'] = $request->input('rtd_location_id');
                     }
+
                     if (($request->filled('update_real_loc')) && (($request->input('update_real_loc')) == '2')) {
                         $this->update_array['location_id'] = $request->input('rtd_location_id');
                     }
+
                 }
 
+
+                /**
+                 * ------------------------------------------------------------------------------
+                 * ANYTHING that happens past this foreach
+                 * WILL NOT BE logged in the edit log_meta data
+                 *  ------------------------------------------------------------------------------
+                 */
                 $changed = [];
-                $asset = Asset::find($assetId);
 
                 foreach ($this->update_array as $key => $value) {
+
                     if ($this->update_array[$key] != $asset->{$key}) {
                         $changed[$key]['old'] = $asset->{$key};
                         $changed[$key]['new'] = $this->update_array[$key];
                     }
+
                 }
 
-                if ($custom_fields_present) {
+                /**
+                 * Start all the custom fields shenanigans
+                 */
 
-                    $model = $asset->model()->first();
+                // Does the model have a fieldset?
+                if ($asset->model->fieldset) {
+                    foreach ($asset->model->fieldset->fields as $field) {
 
-                    // Use the rules of the new model fieldsets if the model changed
+                        if ((array_key_exists($field->db_column, $this->update_array)) && ($field->field_encrypted == '1')) {
-                    if ($request->filled('model_id')) {
-                        $this->update_array['model_id'] =  $request->input('model_id');
-                        $model = \App\Models\AssetModel::find($request->input('model_id'));
-                    }
-
-
-                    // Make sure this model is valid
-                    $assetCustomFields = ($model) ? $model->fieldset : null;
-
-                    if ($assetCustomFields && $assetCustomFields->fields) {
-
-                            foreach ($assetCustomFields->fields as $field) {
-
-                                if ((array_key_exists($field->db_column, $this->update_array)) && ($field->field_encrypted=='1')) {
                             $decrypted_old = Helper::gracefulDecrypt($field, $asset->{$field->db_column});
 
                             /*
@@ -260,7 +324,6 @@ class BulkAssetsController extends Controller
                              */
                         } else {
 
-
                             if ((array_key_exists($field->db_column, $this->update_array)) && ($asset->{$field->db_column} != $this->update_array[$field->db_column])) {
 
                                 // Check if this is an array, and if so, flatten it
@@ -273,9 +336,7 @@ class BulkAssetsController extends Controller
                         }
 
                     } // endforeach
-                    } // end custom field check
+                }
-                } // end custom fields handler
-
 
 
                 // Check if it passes validation, and then try to save

app/Models/Asset.php

@@ -266,7 +266,7 @@ class Asset extends Depreciable
 
     /**
      * Determines if an asset is available for checkout.
-     * This checks to see if the it's checked out to an invalid (deleted) user
+     * This checks to see if it's checked out to an invalid (deleted) user
      * OR if the assigned_to and deleted_at fields on the asset are empty AND
      * that the status is deployable
      *
@@ -753,7 +753,7 @@ class Asset extends Depreciable
     }
 
     /**
-     * Establishes the asset -> status relationship
+     * Establishes the asset -> license seats relationship
      *
      * @author [A. Gianotto] [<snipe@snipe.net>]
      * @since [v4.0]

resources/lang/en/general.php

@@ -491,5 +491,6 @@ return [
     'upload_error' => 'Error uploading file. Please check that there are no empty rows and that no column names are duplicated.',
     'copy_to_clipboard' => 'Copy to Clipboard',
     'copied' => 'Copied!',
+    'status_compatibility' => 'If assets are already assigned, they cannot be changed to a non-deployable status type and this value change will be skipped.',
 
 ];

resources/views/hardware/bulk-restore.blade.php

@@ -30,7 +30,7 @@
               <tr>
                 <td></td>
                 <td>{{ trans('admin/hardware/table.id') }}</td>
-                <td>{{ trans('admin/hardware/table.name') }}</td>
+                <td>{{ trans('admin/hardware/form.name') }}</td>
                 <td>{{ trans('admin/hardware/table.location')}}</td>
               </tr>
             </thead>

resources/views/hardware/bulk.blade.php

@@ -19,18 +19,22 @@
 
     <p>{{ trans('admin/hardware/form.bulk_update_help') }}</p>
 
-    <div class="callout callout-warning">
+
-      <i class="fas fa-exclamation-triangle"></i> {{ trans_choice('admin/hardware/form.bulk_update_warn', count($assets), ['asset_count' => count($assets)]) }}
-        @if (count($models) > 0)
-            {{ trans_choice('admin/hardware/form.bulk_update_with_custom_field', count($models), ['asset_model_count' => count($models)]) }} 
-        @endif 
-    </div>
 
     <form class="form-horizontal" method="post" action="{{ route('hardware/bulksave') }}" autocomplete="off" role="form">
       {{ csrf_field() }}
 
       <div class="box box-default">
         <div class="box-body">
+
+          <div class="callout callout-warning">
+            <i class="fas fa-exclamation-triangle"></i> {{ trans_choice('admin/hardware/form.bulk_update_warn', count($assets), ['asset_count' => count($assets)]) }}
+
+            @if (count($models) > 0)
+              {{ trans_choice('admin/hardware/form.bulk_update_with_custom_field', count($models), ['asset_model_count' => count($models)]) }}
+            @endif
+          </div>
+
           <!-- Purchase Date -->
           <div class="form-group {{ $errors->has('purchase_date') ? ' has-error' : '' }}">
             <label for="purchase_date" class="col-md-3 control-label">{{ trans('admin/hardware/form.date') }}</label>
@@ -76,6 +80,7 @@
             </label>
             <div class="col-md-7">
               {{ Form::select('status_id', $statuslabel_list , old('status_id'), array('class'=>'select2', 'style'=>'width:100%', 'aria-label'=>'status_id')) }}
+              <p class="help-block">{{ trans('general.status_compatibility') }}</p>
               {!! $errors->first('status_id', '<span class="alert-msg" aria-hidden="true"><i class="fas fa-times" aria-hidden="true"></i> :message</span>') !!}
             </div>
           </div>

resources/views/hardware/view.blade.php

@@ -1206,15 +1206,15 @@
                 <thead>
                 <tr>
                   <th data-visible="true" data-field="icon" style="width: 40px;" class="hidden-xs" data-formatter="iconFormatter">{{ trans('admin/hardware/table.icon') }}</th>
-                  <th class="col-sm-2" data-visible="true" data-field="action_date" data-formatter="dateDisplayFormatter">{{ trans('general.date') }}</th>
+                  <th data-visible="true" data-field="action_date" data-formatter="dateDisplayFormatter">{{ trans('general.date') }}</th>
-                  <th class="col-sm-1" data-visible="true" data-field="admin" data-formatter="usersLinkObjFormatter">{{ trans('general.admin') }}</th>
+                  <th data-visible="true" data-field="admin" data-formatter="usersLinkObjFormatter">{{ trans('general.admin') }}</th>
-                  <th class="col-sm-1" data-visible="true" data-field="action_type">{{ trans('general.action') }}</th>
+                  <th data-visible="true" data-field="action_type">{{ trans('general.action') }}</th>
-                  <th class="col-sm-2" data-visible="true" data-field="item" data-formatter="polymorphicItemFormatter">{{ trans('general.item') }}</th>
+                  <th data-visible="true" data-field="item" data-formatter="polymorphicItemFormatter">{{ trans('general.item') }}</th>
-                  <th class="col-sm-2" data-visible="true" data-field="target" data-formatter="polymorphicItemFormatter">{{ trans('general.target') }}</th>
+                  <th data-visible="true" data-field="target" data-formatter="polymorphicItemFormatter">{{ trans('general.target') }}</th>
-                  <th class="col-sm-2" data-field="note">{{ trans('general.notes') }}</th>
+                  <th data-field="note">{{ trans('general.notes') }}</th>
-                  <th class="col-md-3" data-field="signature_file" data-visible="false"  data-formatter="imageFormatter">{{ trans('general.signature') }}</th>
+                  <th data-field="signature_file" data-visible="false"  data-formatter="imageFormatter">{{ trans('general.signature') }}</th>
-                  <th class="col-md-3" data-visible="false" data-field="file" data-visible="false"  data-formatter="fileUploadFormatter">{{ trans('general.download') }}</th>
+                  <th data-visible="false" data-field="file" data-visible="false"  data-formatter="fileUploadFormatter">{{ trans('general.download') }}</th>
-                  <th class="col-sm-2" data-field="log_meta" data-visible="true" data-formatter="changeLogFormatter">{{ trans('admin/hardware/table.changed')}}</th>
+                  <th data-field="log_meta" data-visible="true" data-formatter="changeLogFormatter">{{ trans('admin/hardware/table.changed')}}</th>
                 </tr>
                 </thead>
               </table>