From 38d8d3076df4d044a9e6fdd8d612d4f18ee6b6cf Mon Sep 17 00:00:00 2001 From: spencerrlongg Date: Wed, 4 Dec 2024 21:43:47 -0600 Subject: [PATCH] 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 | 4 +- app/Actions/Assets/StoreAssetAction.php | 79 ++++++++++++------- app/Actions/Assets/UpdateAssetAction.php | 62 +++++++++------ app/Exceptions/Handler.php | 1 - app/Http/Controllers/Api/AssetsController.php | 3 +- .../Controllers/Assets/AssetsController.php | 3 +- .../Assets/BulkAssetsController.php | 3 - .../Requests/Assets/UpdateAssetRequest.php | 1 - tests/Feature/Assets/Api/UpdateAssetTest.php | 2 +- tests/Feature/Assets/Ui/StoreAssetTest.php | 1 - 10 files changed, 91 insertions(+), 68 deletions(-) diff --git a/app/Actions/Assets/DestroyAssetAction.php b/app/Actions/Assets/DestroyAssetAction.php index 095f703e62..6ea96abede 100644 --- a/app/Actions/Assets/DestroyAssetAction.php +++ b/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); } } diff --git a/app/Actions/Assets/StoreAssetAction.php b/app/Actions/Assets/StoreAssetAction.php index 157d537f8e..b9a40546a0 100644 --- a/app/Actions/Assets/StoreAssetAction.php +++ b/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); + } } \ No newline at end of file diff --git a/app/Actions/Assets/UpdateAssetAction.php b/app/Actions/Assets/UpdateAssetAction.php index b82276469f..7cde179c54 100644 --- a/app/Actions/Assets/UpdateAssetAction.php +++ b/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); + } } \ No newline at end of file diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index 0a6f020879..2b8eaa3627 100644 --- a/app/Exceptions/Handler.php +++ b/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; diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php index c665e069bd..5b52da246f 100644 --- a/app/Http/Controllers/Api/AssetsController.php +++ b/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())); } } diff --git a/app/Http/Controllers/Assets/AssetsController.php b/app/Http/Controllers/Assets/AssetsController.php index bd23a2aa8e..26d62787b8 100755 --- a/app/Http/Controllers/Assets/AssetsController.php +++ b/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) { diff --git a/app/Http/Controllers/Assets/BulkAssetsController.php b/app/Http/Controllers/Assets/BulkAssetsController.php index 7bd9b7fd78..c4170a9a34 100644 --- a/app/Http/Controllers/Assets/BulkAssetsController.php +++ b/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); diff --git a/app/Http/Requests/Assets/UpdateAssetRequest.php b/app/Http/Requests/Assets/UpdateAssetRequest.php index 1732b75028..5c8d524bde 100644 --- a/app/Http/Requests/Assets/UpdateAssetRequest.php +++ b/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. diff --git a/tests/Feature/Assets/Api/UpdateAssetTest.php b/tests/Feature/Assets/Api/UpdateAssetTest.php index 1cb6879d1e..df4448a2db 100644 --- a/tests/Feature/Assets/Api/UpdateAssetTest.php +++ b/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); diff --git a/tests/Feature/Assets/Ui/StoreAssetTest.php b/tests/Feature/Assets/Ui/StoreAssetTest.php index 560dec4125..db1cd1eb09 100644 --- a/tests/Feature/Assets/Ui/StoreAssetTest.php +++ b/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 {