From 249b18865499d43bf5c6154c8d26c9ddbc477dbe Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 13:30:01 -0800 Subject: [PATCH 01/10] Refactor category API and transformer for query optimization Signed-off-by: snipe --- .../Transformers/CategoriesTransformer.php | 20 ++++++++++- app/Models/Category.php | 34 ++++--------------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/app/Http/Transformers/CategoriesTransformer.php b/app/Http/Transformers/CategoriesTransformer.php index 427cfaa299..b5d6a9cba9 100644 --- a/app/Http/Transformers/CategoriesTransformer.php +++ b/app/Http/Transformers/CategoriesTransformer.php @@ -22,6 +22,24 @@ class CategoriesTransformer public function transformCategory(Category $category = null) { + + switch ($category->category_type) { + case 'asset': + $category->item_count = $category->assets_count; + break; + case 'accessory': + $category->item_count = $category->accessories_count; + break; + case 'consumable': + $category->item_count = $category->consumables_count; + break; + case 'component': + $category->item_count = $category->components_count; + break; + default: + $category->item_count = 0; + } + if ($category) { $array = [ 'id' => (int) $category->id, @@ -33,7 +51,7 @@ class CategoriesTransformer 'eula' => ($category->getEula()), 'checkin_email' => ($category->checkin_email == '1'), 'require_acceptance' => ($category->require_acceptance == '1'), - 'item_count' => (int) $category->itemCount(), + 'item_count' => (int) $category->item_count, 'assets_count' => (int) $category->assets_count, 'accessories_count' => (int) $category->accessories_count, 'consumables_count' => (int) $category->consumables_count, diff --git a/app/Models/Category.php b/app/Models/Category.php index f339debfea..23ad8223db 100755 --- a/app/Models/Category.php +++ b/app/Models/Category.php @@ -10,6 +10,7 @@ use Illuminate\Database\Eloquent\SoftDeletes; use Illuminate\Support\Facades\Gate; use Watson\Validating\ValidatingTrait; use App\Helpers\Helper; +use Illuminate\Support\Str; /** * Model for Categories. Categories are a higher-level group @@ -97,8 +98,11 @@ class Category extends SnipeModel */ public function isDeletable() { - return Gate::allows('delete', $this) - && ($this->itemCount() == 0); + if (Gate::allows('delete', $this)) { + $category_type_var = Str::plural($this->category_type).'_count'; + return $this->{$category_type_var}; + } + } /** @@ -148,31 +152,7 @@ class Category extends SnipeModel { return $this->hasMany(\App\Models\Component::class); } - - /** - * Get the number of items in the category - * - * @author [A. Gianotto] [] - * @since [v2.0] - * @return int - */ - public function itemCount() - { - switch ($this->category_type) { - case 'asset': - return $this->assets()->count(); - case 'accessory': - return $this->accessories()->count(); - case 'component': - return $this->components()->count(); - case 'consumable': - return $this->consumables()->count(); - case 'license': - return $this->licenses()->count(); - } - - return '0'; - } + /** * Establishes the category -> assets relationship From bef4224e14ba44bcf1de0878b93ca981656a35fd Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 13:50:24 -0800 Subject: [PATCH 02/10] Added notes to itemCount() Signed-off-by: snipe --- app/Models/Category.php | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/app/Models/Category.php b/app/Models/Category.php index 23ad8223db..3474ade215 100755 --- a/app/Models/Category.php +++ b/app/Models/Category.php @@ -98,11 +98,8 @@ class Category extends SnipeModel */ public function isDeletable() { - if (Gate::allows('delete', $this)) { - $category_type_var = Str::plural($this->category_type).'_count'; - return $this->{$category_type_var}; - } - + return Gate::allows('delete', $this) + && ($this->{Str::plural($this->category_type).'_count'} == 0); } /** @@ -152,7 +149,34 @@ class Category extends SnipeModel { return $this->hasMany(\App\Models\Component::class); } - + + /** + * Get the number of items in the category. This should NEVER be used in + * a collection of cartegories, as you'll end up with an n+1 query problem. + * + * It should only be used in a single categoiry context. + * + * @author [A. Gianotto] [] + * @since [v2.0] + * @return int + */ + public function itemCount() + { + switch ($this->category_type) { + case 'asset': + return $this->assets()->count(); + case 'accessory': + return $this->accessories()->count(); + case 'component': + return $this->components()->count(); + case 'consumable': + return $this->consumables()->count(); + case 'license': + return $this->licenses()->count(); + } + + return '0'; + } /** * Establishes the category -> assets relationship From f50b622eb819a81b79a4e7445a0dee723a1f82b1 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 16:15:40 -0800 Subject: [PATCH 03/10] Disallow editing of category type once created, added help text Signed-off-by: snipe --- resources/views/categories/edit.blade.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/resources/views/categories/edit.blade.php b/resources/views/categories/edit.blade.php index 1b68d46b9b..9d4d25c814 100755 --- a/resources/views/categories/edit.blade.php +++ b/resources/views/categories/edit.blade.php @@ -15,11 +15,16 @@
- {{ Form::select('category_type', $category_types , old('category_type', $item->category_type), array('class'=>'select2', 'style'=>'min-width:350px', 'aria-label'=>'category_type', $item->itemCount() > 0 ? 'disabled' : '')) }} + {{ Form::select('category_type', $category_types , old('category_type', $item->category_type), array('class'=>'select2', 'style'=>'min-width:350px', 'aria-label'=>'category_type', ($item->category_type!='') || ($item->itemCount() > 0) ? 'disabled' : '')) }} {!! $errors->first('category_type', '') !!}
+
+

{!! trans('admin/categories/message.update.cannot_change_category_type') !!}

+
+ +
From d508374c579f60143fb207b389863ef6f7d256ea Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 16:15:51 -0800 Subject: [PATCH 04/10] Disallowed category type change string Signed-off-by: snipe --- resources/lang/en/admin/categories/message.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/resources/lang/en/admin/categories/message.php b/resources/lang/en/admin/categories/message.php index 48cf5478e1..4e493f68b6 100644 --- a/resources/lang/en/admin/categories/message.php +++ b/resources/lang/en/admin/categories/message.php @@ -13,7 +13,8 @@ return array( 'update' => array( 'error' => 'Category was not updated, please try again', - 'success' => 'Category updated successfully.' + 'success' => 'Category updated successfully.', + 'cannot_change_category_type' => 'You cannot change the category type once it has been created', ), 'delete' => array( From b66cd313b94e44e9c1767f253660772f611e8fb5 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 16:16:25 -0800 Subject: [PATCH 05/10] Refactored itemCount() to use existing blah_count fields if they exist Signed-off-by: snipe --- app/Models/Category.php | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/app/Models/Category.php b/app/Models/Category.php index 3474ade215..75e7551ff4 100755 --- a/app/Models/Category.php +++ b/app/Models/Category.php @@ -98,8 +98,9 @@ class Category extends SnipeModel */ public function isDeletable() { + return Gate::allows('delete', $this) - && ($this->{Str::plural($this->category_type).'_count'} == 0); + && ($this->itemCount() == 0); } /** @@ -152,7 +153,7 @@ class Category extends SnipeModel /** * Get the number of items in the category. This should NEVER be used in - * a collection of cartegories, as you'll end up with an n+1 query problem. + * a collection of categories, as you'll end up with an n+1 query problem. * * It should only be used in a single categoiry context. * @@ -162,6 +163,11 @@ class Category extends SnipeModel */ public function itemCount() { + + if (isset($this->{Str::plural($this->category_type).'_count'})) { + return $this->{Str::plural($this->category_type).'_count'}; + } + switch ($this->category_type) { case 'asset': return $this->assets()->count(); @@ -173,9 +179,9 @@ class Category extends SnipeModel return $this->consumables()->count(); case 'license': return $this->licenses()->count(); + default: 0; } - return '0'; } /** From 825df2cf75df65d5a8877d76e217fd21dd12421b Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 16:17:03 -0800 Subject: [PATCH 06/10] Load up the item counts with the individual API methods for accurate numbers without n+1 Signed-off-by: snipe --- app/Http/Controllers/Api/CategoriesController.php | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/Api/CategoriesController.php b/app/Http/Controllers/Api/CategoriesController.php index 164928aaba..59e6d27430 100644 --- a/app/Http/Controllers/Api/CategoriesController.php +++ b/app/Http/Controllers/Api/CategoriesController.php @@ -10,6 +10,7 @@ use App\Models\Category; use Illuminate\Http\Request; use App\Http\Requests\ImageUploadRequest; use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Facades\Validator; class CategoriesController extends Controller { @@ -107,7 +108,7 @@ class CategoriesController extends Controller public function show($id) { $this->authorize('view', Category::class); - $category = Category::findOrFail($id); + $category = Category::withCount('assets as assets_count', 'accessories as accessories_count', 'consumables as consumables_count', 'components as components_count', 'licenses as licenses_count')->findOrFail($id); return (new CategoriesTransformer)->transformCategory($category); } @@ -126,10 +127,18 @@ class CategoriesController extends Controller { $this->authorize('update', Category::class); $category = Category::findOrFail($id); + + if ($category->category_type !== $request->input('category_type')) { + return response()->json( + Helper::formatStandardApiResponse('error', null, trans('admin/categories/message.update.cannot_change_category_type')) + ); + } $category->fill($request->all()); - $category->category_type = strtolower($request->input('category_type')); + $category = $request->handleImages($category); + + if ($category->save()) { return response()->json(Helper::formatStandardApiResponse('success', $category, trans('admin/categories/message.update.success'))); } @@ -148,7 +157,7 @@ class CategoriesController extends Controller public function destroy($id) { $this->authorize('delete', Category::class); - $category = Category::findOrFail($id); + $category = Category::withCount('assets as assets_count', 'accessories as accessories_count', 'consumables as consumables_count', 'components as components_count', 'licenses as licenses_count')->findOrFail($id); if (! $category->isDeletable()) { return response()->json( From 151719a91c3579eddce56efb59494242b22f7061 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 16:24:46 -0800 Subject: [PATCH 07/10] Added comments Signed-off-by: snipe --- app/Http/Controllers/Api/CategoriesController.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/Http/Controllers/Api/CategoriesController.php b/app/Http/Controllers/Api/CategoriesController.php index 59e6d27430..c518813405 100644 --- a/app/Http/Controllers/Api/CategoriesController.php +++ b/app/Http/Controllers/Api/CategoriesController.php @@ -128,17 +128,15 @@ class CategoriesController extends Controller $this->authorize('update', Category::class); $category = Category::findOrFail($id); - if ($category->category_type !== $request->input('category_type')) { + // Don't allow the user to change the category_type once it's been created + if (($request->filled('category_type')) && ($category->category_type != $request->input('category_type'))) { return response()->json( Helper::formatStandardApiResponse('error', null, trans('admin/categories/message.update.cannot_change_category_type')) ); } $category->fill($request->all()); - $category = $request->handleImages($category); - - if ($category->save()) { return response()->json(Helper::formatStandardApiResponse('success', $category, trans('admin/categories/message.update.success'))); } From 8f3a237ea0d363808697b18773c9cd87a0190704 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 16:29:51 -0800 Subject: [PATCH 08/10] More comments Signed-off-by: snipe --- app/Http/Transformers/CategoriesTransformer.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/Http/Transformers/CategoriesTransformer.php b/app/Http/Transformers/CategoriesTransformer.php index b5d6a9cba9..5ea8ee3e01 100644 --- a/app/Http/Transformers/CategoriesTransformer.php +++ b/app/Http/Transformers/CategoriesTransformer.php @@ -23,6 +23,8 @@ class CategoriesTransformer public function transformCategory(Category $category = null) { + // We only ever use item_count for categories in this transformer, so it makes sense to keep it + // simple and do this switch here. switch ($category->category_type) { case 'asset': $category->item_count = $category->assets_count; From 0b0fdd8aa5d74daec9834136d96a9c78bfee6634 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 16:32:18 -0800 Subject: [PATCH 09/10] Small formatting fixes, fixed typo in comment Signed-off-by: snipe --- app/Models/Category.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Models/Category.php b/app/Models/Category.php index 75e7551ff4..836f28e8e4 100755 --- a/app/Models/Category.php +++ b/app/Models/Category.php @@ -100,7 +100,7 @@ class Category extends SnipeModel { return Gate::allows('delete', $this) - && ($this->itemCount() == 0); + && ($this->itemCount() == 0); } /** @@ -155,7 +155,7 @@ class Category extends SnipeModel * Get the number of items in the category. This should NEVER be used in * a collection of categories, as you'll end up with an n+1 query problem. * - * It should only be used in a single categoiry context. + * It should only be used in a single category context. * * @author [A. Gianotto] [] * @since [v2.0] From 459c95064e3611ff3732bcfcc74264df06479f2d Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Jan 2023 17:54:04 -0800 Subject: [PATCH 10/10] Fixed default Signed-off-by: snipe --- app/Models/Category.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/Models/Category.php b/app/Models/Category.php index 836f28e8e4..b00e5a7bf9 100755 --- a/app/Models/Category.php +++ b/app/Models/Category.php @@ -179,7 +179,8 @@ class Category extends SnipeModel return $this->consumables()->count(); case 'license': return $this->licenses()->count(); - default: 0; + default: + return 0; } }