From 16740b41e426af45413da863a11eca9a83618fce Mon Sep 17 00:00:00 2001 From: spencerrlongg Date: Tue, 3 Dec 2024 13:50:30 -0600 Subject: [PATCH] Simplify asset handling logic for creation and update Refactor asset creation and update processes by removing redundant save checks and organizing error handling logic. Enhance success and failure feedback in the asset creation flow to improve clarity and user experience. Add support for detailed multi-success and partial failure messages to alert users of outcomes. --- app/Actions/Assets/StoreAssetAction.php | 42 ++++++++-------- app/Actions/Assets/UpdateAssetAction.php | 33 ++++++------ .../Controllers/Assets/AssetsController.php | 50 ++++++++++++------- .../lang/en-US/admin/hardware/message.php | 4 +- 4 files changed, 72 insertions(+), 57 deletions(-) diff --git a/app/Actions/Assets/StoreAssetAction.php b/app/Actions/Assets/StoreAssetAction.php index 04f9b3fa01..157d537f8e 100644 --- a/app/Actions/Assets/StoreAssetAction.php +++ b/app/Actions/Assets/StoreAssetAction.php @@ -118,28 +118,26 @@ class StoreAssetAction } } - if ($asset->isValid() && $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)) { - $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(); - } - return $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; } - return false; + + 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(); + } + return $asset; } } \ No newline at end of file diff --git a/app/Actions/Assets/UpdateAssetAction.php b/app/Actions/Assets/UpdateAssetAction.php index 5bfb57e17f..b82276469f 100644 --- a/app/Actions/Assets/UpdateAssetAction.php +++ b/app/Actions/Assets/UpdateAssetAction.php @@ -194,26 +194,25 @@ class UpdateAssetAction self::bulkLocationUpdate($asset, $request); } - if ($asset->save()) { + $asset->save(); // check out stuff - $location = Location::find($asset->location_id); - if (!is_null($assigned_user) && ($target = User::find($assigned_user))) { - $location = $target->location_id; - } elseif (!is_null($assigned_asset) && ($target = Asset::find($assigned_asset))) { - $location = $target->location_id; - Asset::where('assigned_type', \App\Models\Asset::class)->where('assigned_to', $asset->id) - ->update(['location_id' => $target->location_id]); - } elseif (!is_null($assigned_location) && ($target = Location::find($assigned_location))) { - $location = $target->id; - } + $location = Location::find($asset->location_id); + if (!is_null($assigned_user) && ($target = User::find($assigned_user))) { + $location = $target->location_id; + } elseif (!is_null($assigned_asset) && ($target = Asset::find($assigned_asset))) { + $location = $target->location_id; + Asset::where('assigned_type', \App\Models\Asset::class)->where('assigned_to', $asset->id) + ->update(['location_id' => $target->location_id]); + } elseif (!is_null($assigned_location) && ($target = Location::find($assigned_location))) { + $location = $target->id; + } - 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); - } + 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); + } - if ($asset->image) { - $asset->image = $asset->getImageUrl(); - } + if ($asset->image) { + $asset->image = $asset->getImageUrl(); } return $asset; diff --git a/app/Http/Controllers/Assets/AssetsController.php b/app/Http/Controllers/Assets/AssetsController.php index e0f4a5116a..6cb725656d 100755 --- a/app/Http/Controllers/Assets/AssetsController.php +++ b/app/Http/Controllers/Assets/AssetsController.php @@ -109,12 +109,12 @@ class AssetsController extends Controller public function store(StoreAssetRequest $request): RedirectResponse { $successes = []; + $failures = []; $errors = []; - try { - $asset_tags = $request->input('asset_tags'); - $serials = $request->input('serials'); - //DB::transaction(function () use ($request, $asset_tags, $serials, $custom_fields) { - foreach ($asset_tags as $key => $asset_tag) { + $asset_tags = $request->input('asset_tags'); + $serials = $request->input('serials'); + foreach ($asset_tags as $key => $asset_tag) { + try { $asset = StoreAssetAction::run( model_id: $request->validated('model_id'), status_id: $request->validated('status_id'), @@ -144,23 +144,39 @@ class AssetsController extends Controller next_audit_date: $request->validated('next_audit_date'), ); $successes[] = " $asset->id])."' style='color: white;'>".e($asset->asset_tag).""; - if (!$asset) { - $failures[] = join(",", $asset->getErrors()->all()); + } catch (ValidationException|CheckoutNotAllowed $e) { + $errors[] = $e->getMessage(); + } catch (Exception $e) { + report($e); + } + } + $failures[] = join(",", $errors); + session()->put(['redirect_option' => $request->get('redirect_option'), 'checkout_to_type' => $request->get('checkout_to_type')]); + + if ($successes) { + if ($failures) { + //some succeeded, some failed + return redirect()->to(Helper::getRedirectOption($request, $asset->id, 'Assets')) //FIXME - not tested + ->with('success-unescaped', trans_choice('admin/hardware/message.create.multi_success_linked', $successes, ['links' => join(", ", $successes)])) + ->with('warning', trans_choice('admin/hardware/message.create.partial_failure', $failures, ['failures' => join("; ", $failures)])); + } else { + if (count($successes) == 1) { + //the most common case, keeping it so we don't have to make every use of that translation string be trans_choice'ed + //and re-translated + return redirect()->to(Helper::getRedirectOption($request, $asset->id, 'Assets')) + ->with('success-unescaped', trans('admin/hardware/message.create.success_linked', ['link' => route('hardware.show', ['hardware' => $asset->id]), 'id', 'tag' => e($asset->asset_tag)])); + } else { + //multi-success + return redirect()->to(Helper::getRedirectOption($request, $asset->id, 'Assets')) + ->with('success-unescaped', trans_choice('admin/hardware/message.create.multi_success_linked', $successes, ['links' => join(", ", $successes)])); } } - //}); - session()->put(['redirect_option' => $request->get('redirect_option'), 'checkout_to_type' => $request->get('checkout_to_type')]); - return redirect()->to(Helper::getRedirectOption($request, $asset->id, 'Assets')) - ->with('success-unescaped', trans('admin/hardware/message.create.success_linked', ['link' => route('hardware.show', ['hardware' => $asset->id]), 'id', 'tag' => e($asset->asset_tag)])); - } catch (CheckoutNotAllowed $e) { - return redirect()->route('hardware.index')->with('error', trans('admin/hardware/message.create.error')); - } catch (Exception $e) { - report($e); - return redirect()->back()->with('error', trans('general.something_went_wrong')); } + // this shouldn't happen, but php complains if there's no final return + return redirect()->to(Helper::getRedirectOption($request, $asset->id, 'Assets')) + ->with('success-unescaped', trans('admin/hardware/message.create.success_linked', ['link' => route('hardware.show', ['hardware' => $asset->id]), 'id', 'tag' => e($asset->asset_tag)])); } - /** * Returns a view that presents a form to edit an existing asset. * diff --git a/resources/lang/en-US/admin/hardware/message.php b/resources/lang/en-US/admin/hardware/message.php index 27877e02f9..10dfbff6fd 100644 --- a/resources/lang/en-US/admin/hardware/message.php +++ b/resources/lang/en-US/admin/hardware/message.php @@ -13,7 +13,9 @@ return [ 'create' => [ 'error' => 'Asset was not created, please try again. :(', 'success' => 'Asset created successfully. :)', - 'success_linked' => 'Asset with tag :tag was created successfully. Click here to view.', + 'success_linked' => 'Asset with tag :tag was created successfully. Click here to view.', + 'multi_success_linked' => 'Asset with tag :links was created successfully.|:count assets were created succesfully. :links.', + 'partial_failure' => 'An asset was unable to be created. Reason: :failures|:count assets were unable to be created. Reasons: :failures', ], 'update' => [