Refactor asset handling and streamline code

This refactoring introduces helper methods for handling image uploads, custom fields, and checkouts, improving code organization and readability. Unnecessary imports, comments, and unused variables were removed for cleaner code. Error handling was also improved with additional exception reporting in the asset update flow.

app/Actions/Assets/DestroyAssetAction.php

@@ -7,6 +7,8 @@ use App\Models\Asset;
 use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Facades\Storage;
+use Exception;
+
 
 class DestroyAssetAction
 {
@@ -27,7 +29,7 @@ class DestroyAssetAction
         if ($asset->image) {
             try {
                 Storage::disk('public')->delete('assets'.'/'.$asset->image);
-            } catch (\Exception $e) {
+            } catch (Exception $e) {
                 Log::debug($e);
             }
         }

app/Actions/Assets/StoreAssetAction.php

@@ -9,12 +9,11 @@ use App\Models\AssetModel;
 use App\Models\Company;
 use App\Models\Location;
 use App\Models\Setting;
+use App\Models\SnipeModel;
 use App\Models\User;
 use Carbon\Carbon;
 use Illuminate\Support\Facades\Crypt;
 use Illuminate\Support\Facades\Gate;
-use Illuminate\Support\Facades\Log;
-use Illuminate\Support\MessageBag;
 
 class StoreAssetAction
 {
@@ -31,7 +30,6 @@ class StoreAssetAction
         $asset_tag = null,
         $order_number = null,
         $notes = null,
-        $user_id = null,
         $warranty_months = null,
         $purchase_cost = null,
         $asset_eol_date = null,
@@ -40,8 +38,7 @@ class StoreAssetAction
         $supplier_id = null,
         $requestable = null,
         $rtd_location_id = null,
-        $location_id = null, //do something with this
-        $files = null,
+        $location_id = null,
         $byod = 0,
         $assigned_user = null,
         $assigned_asset = null,
@@ -87,17 +84,43 @@ class StoreAssetAction
             $asset->location_id = $rtd_location_id;
         }
 
-        //api only
-        if ($request->has('image_source')) {
-            $request->offsetSet('image', $request->offsetGet('image_source'));
-        }
-
-        if ($request->has('image')) {
-            $asset = $request->handleImages($asset);
-        }
+        $asset = self::handleImages($request, $asset);
 
         $model = AssetModel::find($model_id);
 
+        self::handleCustomFields($model, $request, $asset);
+
+        $asset->save();
+        if (request('assigned_user')) {
+            $target = User::find(request('assigned_user'));
+            // the api doesn't have these location-y bits - good reason?
+            $location = $target->location_id;
+        } elseif (request('assigned_asset')) {
+            $target = Asset::find(request('assigned_asset'));
+            $location = $target->location_id;
+        } elseif (request('assigned_location')) {
+            $target = Location::find(request('assigned_location'));
+            $location = $target->id;
+        }
+
+        if (isset($target)) {
+            self::handleCheckout($target, $asset, $request, $location);
+        }
+        //this was in api and not gui
+        if ($asset->image) {
+            $asset->image = $asset->getImageUrl();
+        }
+        return $asset;
+    }
+
+    /**
+     * @param  $model
+     * @param  ImageUploadRequest  $request
+     * @param  Asset|\App\Models\SnipeModel  $asset
+     * @return void
+     */
+    private static function handleCustomFields($model, ImageUploadRequest $request, $asset): void
+    {
         if (($model) && ($model instanceof AssetModel) && ($model->fieldset)) {
             foreach ($model->fieldset->fields as $field) {
                 if ($field->field_encrypted == '1') {
@@ -117,27 +140,23 @@ class StoreAssetAction
                 }
             }
         }
+    }
 
-        $asset->save();
-        if (request('assigned_user')) {
-            $target = User::find(request('assigned_user'));
-            // the api doesn't have these location-y bits - good reason?
-            $location = $target->location_id;
-        } elseif (request('assigned_asset')) {
-            $target = Asset::find(request('assigned_asset'));
-            $location = $target->location_id;
-        } elseif (request('assigned_location')) {
-            $target = Location::find(request('assigned_location'));
-            $location = $target->id;
+    private static function handleImages($request, $asset)
+    {
+        //api
+        if ($request->has('image_source')) {
+            $request->offsetSet('image', $request->offsetGet('image_source'));
         }
 
-        if (isset($target)) {
-            $asset->checkOut($target, auth()->user(), date('Y-m-d H:i:s'), $request->input('expected_checkin', null), 'Checked out on asset creation', $request->get('name'), $location);
-        }
-        //this was in api and not gui
-        if ($asset->image) {
-            $asset->image = $asset->getImageUrl();
+        if ($request->has('image')) {
+            $asset = $request->handleImages($asset);
         }
         return $asset;
     }
+
+    private static function handleCheckout($target, $asset, $request, $location): void
+    {
+            $asset->checkOut($target, auth()->user(), date('Y-m-d H:i:s'), $request->input('expected_checkin', null), 'Checked out on asset creation', $request->get('name'), $location);
+    }
 }

app/Actions/Assets/UpdateAssetAction.php

@@ -10,6 +10,7 @@ use App\Models\Asset;
 use App\Models\AssetModel;
 use App\Models\Company;
 use App\Models\Location;
+use App\Models\SnipeModel;
 use App\Models\Statuslabel;
 use App\Models\User;
 use Carbon\Carbon;
@@ -53,7 +54,8 @@ class UpdateAssetAction
         $asset_tag = null,
         $notes = null,
         $isBulk = false,
-    ): \App\Models\SnipeModel {
+    ): SnipeModel
+    {
 
         $asset->status_id = $status_id ?? $asset->status_id;
         $asset->warranty_months = $warranty_months ?? $asset->warranty_months;
@@ -165,30 +167,7 @@ class UpdateAssetAction
 
         $asset = $request->handleImages($asset);
 
-        $model = $asset->model;
-        if (($model) && (isset($model->fieldset))) {
-            foreach ($model->fieldset->fields as $field) {
-                $field_val = $request->input($field->db_column, null);
-
-                if ($request->has($field->db_column)) {
-                    if ($field->element == 'checkbox') {
-                        if (is_array($field_val)) {
-                            $field_val = implode(',', $field_val);
-                        }
-                    }
-                    if ($field->field_encrypted == '1') {
-                        if (Gate::allows('assets.view.encrypted_custom_fields')) {
-                            $field_val = Crypt::encrypt($field_val);
-                        } else {
-                            throw new CustomFieldPermissionException();
-                        }
-                    }
-                    $asset->{$field->db_column} = $field_val;
-                }
-            }
-        }
-
-        session()->put(['redirect_option' => $request->get('redirect_option'), 'checkout_to_type' => $request->get('checkout_to_type')]);
+        self::handleCustomFields($request, $asset);
 
         if ($isBulk) {
             self::bulkLocationUpdate($asset, $request);
@@ -208,7 +187,7 @@ class UpdateAssetAction
         }
 
         if (isset($target)) {
-            $asset->checkOut($target, auth()->user(), date('Y-m-d H:i:s'), '', 'Checked out on asset update', e($request->get('name')), $location);
+            self::handleCheckout($asset, $target, $request, $location);
         }
 
         if ($asset->image) {
@@ -246,4 +225,35 @@ class UpdateAssetAction
         }
 
     }
+
+    private static function handleCustomFields($request, $asset): void
+    {
+        $model = $asset->model;
+        if (($model) && (isset($model->fieldset))) {
+            foreach ($model->fieldset->fields as $field) {
+                $field_val = $request->input($field->db_column, null);
+
+                if ($request->has($field->db_column)) {
+                    if ($field->element == 'checkbox') {
+                        if (is_array($field_val)) {
+                            $field_val = implode(',', $field_val);
+                        }
+                    }
+                    if ($field->field_encrypted == '1') {
+                        if (Gate::allows('assets.view.encrypted_custom_fields')) {
+                            $field_val = Crypt::encrypt($field_val);
+                        } else {
+                            throw new CustomFieldPermissionException();
+                        }
+                    }
+                    $asset->{$field->db_column} = $field_val;
+                }
+            }
+        }
+    }
+
+    private static function handleCheckout($asset, $target, $request, $location): void
+    {
+        $asset->checkOut($target, auth()->user(), date('Y-m-d H:i:s'), '', 'Checked out on asset update', e($request->get('name')), $location);
+    }
 }

app/Exceptions/Handler.php

@@ -2,7 +2,6 @@
 
 namespace App\Exceptions;
 
-use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
 use App\Helpers\Helper;
 use Illuminate\Validation\ValidationException;

app/Http/Controllers/Api/AssetsController.php

@@ -614,7 +614,6 @@ class AssetsController extends Controller
                 asset_tag: $request->validated('asset_tag'),
                 order_number: $request->validated('order_number'),
                 notes: $request->validated('notes'),
-                user_id: $request->validated('user_id'),
                 warranty_months: $request->validated('warranty_months'),
                 purchase_cost: $request->validated('purchase_cost'),
                 asset_eol_date: $request->validated('asset_eol_date'),
@@ -624,7 +623,6 @@ class AssetsController extends Controller
                 requestable: $request->validated('requestable'),
                 rtd_location_id: $request->validated('rtd_location_id'),
                 location_id: $request->validated('location_id'),
-                files: $request->validated('files'),
                 byod: $request->validated('byod'),
                 assigned_user: $request->validated('assigned_user'),
                 assigned_asset: $request->validated('assigned_asset'),
@@ -638,6 +636,7 @@ class AssetsController extends Controller
         } catch (CheckoutNotAllowed $e) {
             return response()->json(Helper::formatStandardApiResponse('error', null, $e->getMessage()), 200);
         } catch (Exception $e) {
+            report($e);
             return response()->json(Helper::formatStandardApiResponse('error', null, $e->getMessage()));
         }
     }

app/Http/Controllers/Assets/AssetsController.php

@@ -115,7 +115,6 @@ class AssetsController extends Controller
                     asset_tag: $asset_tag,
                     order_number: $request->validated('order_number'),
                     notes: $request->validated('notes'),
-                    user_id: $request->validated('user_id'),
                     warranty_months: $request->validated('warranty_months'),
                     purchase_cost: $request->validated('purchase_cost'),
                     asset_eol_date: $request->validated('asset_eol_date'),
@@ -125,7 +124,6 @@ class AssetsController extends Controller
                     requestable: $request->validated('requestable'),
                     rtd_location_id: $request->validated('rtd_location_id'),
                     location_id: $request->validated('location_id'),
-                    files: $request->validated('files'),
                     byod: $request->validated('byod'),
                     assigned_user: $request->validated('assigned_user'),
                     assigned_asset: $request->validated('assigned_asset'),
@@ -274,6 +272,7 @@ class AssetsController extends Controller
                 asset_tag: $asset_tag, // same as serials
                 notes: $request->validated('notes'),
             );
+            session()->put(['redirect_option' => $request->get('redirect_option'), 'checkout_to_type' => $request->get('checkout_to_type')]);
             return redirect()->to(Helper::getRedirectOption($request, $updatedAsset->id, 'Assets'))
                 ->with('success', trans('admin/hardware/message.update.success'));
         } catch (ValidationException $e) {

app/Http/Controllers/Assets/BulkAssetsController.php

@@ -2,7 +2,6 @@
 
 namespace App\Http\Controllers\Assets;
 
-use App\Actions\Assets\StoreAssetAction;
 use App\Actions\Assets\UpdateAssetAction;
 use App\Exceptions\CustomFieldPermissionException;
 use App\Helpers\Helper;
@@ -241,11 +240,9 @@ class BulkAssetsController extends Controller
                     order_number: $request->input('order_number'),
                     isBulk: true,
                 );
-                // catch exceptions
             } catch (ValidationException $e) {
                 $errors = $e->validator->errors()->toArray();
             } catch (CustomFieldPermissionException $e) {
-                //$errors[$key] = $e->getMessage();
                 $custom_field_problem = true;
             } catch (\Exception $e) {
                 report($e);

app/Http/Requests/Assets/UpdateAssetRequest.php

@@ -30,7 +30,6 @@ class UpdateAssetRequest extends ImageUploadRequest
     public function rules()
     {
         $modelRules = (new Asset)->getRules();
-        // TODO: make sure this actually works
         if ((Setting::getSettings()->digit_separator === '1.234,56' || '1,234.56') && is_string($this->input('purchase_cost'))) {
             // If purchase_cost was submitted as a string with a comma separator
             // then we need to ignore the normal numeric rules.

tests/Feature/Assets/Api/UpdateAssetTest.php

@@ -86,7 +86,7 @@ class UpdateAssetTest extends TestCase
         $this->assertEquals('random_string', $updatedAsset->asset_tag);
         $this->assertEquals($userAssigned->id, $updatedAsset->assigned_to);
         $this->assertTrue($updatedAsset->company->is($company));
-        $this->assertTrue($updatedAsset->location->is($location)); //fix all location setting
+        $this->assertTrue($updatedAsset->location->is($location));
         $this->assertTrue($updatedAsset->model->is($model));
         $this->assertEquals('A New Asset', $updatedAsset->name);
         $this->assertEquals('Some notes', $updatedAsset->notes);

tests/Feature/Assets/Ui/StoreAssetTest.php

@@ -12,7 +12,6 @@ use Carbon\Carbon;
 use Illuminate\Http\UploadedFile;
 use Illuminate\Support\Facades\Storage;
 use Tests\TestCase;
-use function Amp\Promise\wait;
 
 class StoreAssetTest extends TestCase
 {