From 7afd7da2b44820dc7da4acd6b110299ce3b9368d Mon Sep 17 00:00:00 2001 From: spencerrlongg Date: Wed, 21 May 2025 11:44:54 -0500 Subject: [PATCH 1/6] initial work, more testing/tests needed --- .../Controllers/Api/DepartmentsController.php | 3 +- app/Http/Requests/StoreDepartmentRequest.php | 32 +++++++++++++++++++ .../Departments/Api/CreateDepartmentsTest.php | 12 +++++++ 3 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 app/Http/Requests/StoreDepartmentRequest.php diff --git a/app/Http/Controllers/Api/DepartmentsController.php b/app/Http/Controllers/Api/DepartmentsController.php index 167d5cbc2a..ec8a605caf 100644 --- a/app/Http/Controllers/Api/DepartmentsController.php +++ b/app/Http/Controllers/Api/DepartmentsController.php @@ -4,6 +4,7 @@ namespace App\Http\Controllers\Api; use App\Helpers\Helper; use App\Http\Controllers\Controller; +use App\Http\Requests\StoreDepartmentRequest; use App\Http\Transformers\DepartmentsTransformer; use App\Http\Transformers\SelectlistTransformer; use App\Models\Department; @@ -94,7 +95,7 @@ class DepartmentsController extends Controller * @since [v4.0] * @param \App\Http\Requests\ImageUploadRequest $request */ - public function store(ImageUploadRequest $request) : JsonResponse + public function store(StoreDepartmentRequest $request): JsonResponse { $this->authorize('create', Department::class); $department = new Department; diff --git a/app/Http/Requests/StoreDepartmentRequest.php b/app/Http/Requests/StoreDepartmentRequest.php new file mode 100644 index 0000000000..3b94e19326 --- /dev/null +++ b/app/Http/Requests/StoreDepartmentRequest.php @@ -0,0 +1,32 @@ +|string> + */ + public function rules(): array + { + $modelRules = (new Department)->getRules(); + + return array_merge( + $modelRules, + ); + } +} diff --git a/tests/Feature/Departments/Api/CreateDepartmentsTest.php b/tests/Feature/Departments/Api/CreateDepartmentsTest.php index e0f975dd7f..4b887bfc3d 100644 --- a/tests/Feature/Departments/Api/CreateDepartmentsTest.php +++ b/tests/Feature/Departments/Api/CreateDepartmentsTest.php @@ -3,6 +3,7 @@ namespace Tests\Feature\Departments\Api; use App\Models\AssetModel; +use App\Models\Company; use App\Models\Department; use App\Models\Category; use App\Models\User; @@ -39,4 +40,15 @@ class CreateDepartmentsTest extends TestCase $this->assertEquals('Test Note', $department->notes); } + public function testNoArraysAllowed() + { + $response = $this->actingAsForApi(User::factory()->superuser()->create()) + ->postJson(route('api.departments.store'), [ + 'name' => 'Test Department', + 'company_id' => [1, 2], + ]) + ->assertOk() + ->assertStatusMessageIs('error'); + } + } From 7571ff007f6a870543d0b554e8839c922333455c Mon Sep 17 00:00:00 2001 From: spencerrlongg Date: Wed, 21 May 2025 12:51:21 -0500 Subject: [PATCH 2/6] add fillable properties to rules, some tests, move authorization to request --- .../Controllers/Api/DepartmentsController.php | 3 +- app/Models/Department.php | 11 ++-- .../Departments/Api/CreateDepartmentsTest.php | 53 ++++++++++++++++--- 3 files changed, 55 insertions(+), 12 deletions(-) diff --git a/app/Http/Controllers/Api/DepartmentsController.php b/app/Http/Controllers/Api/DepartmentsController.php index ec8a605caf..adbe4bc35c 100644 --- a/app/Http/Controllers/Api/DepartmentsController.php +++ b/app/Http/Controllers/Api/DepartmentsController.php @@ -97,9 +97,8 @@ class DepartmentsController extends Controller */ public function store(StoreDepartmentRequest $request): JsonResponse { - $this->authorize('create', Department::class); $department = new Department; - $department->fill($request->all()); + $department->fill($request->validated()); $department = $request->handleImages($department); $department->created_by = auth()->id(); diff --git a/app/Models/Department.php b/app/Models/Department.php index 592fd840b1..4b9eb849f7 100644 --- a/app/Models/Department.php +++ b/app/Models/Department.php @@ -30,10 +30,13 @@ class Department extends SnipeModel ]; protected $rules = [ - 'name' => 'required|max:255|is_unique_department', - 'location_id' => 'numeric|nullable', - 'company_id' => 'numeric|nullable', - 'manager_id' => 'numeric|nullable', + 'name' => 'required|max:255|is_unique_department', + 'location_id' => 'numeric|nullable|exists:locations,id', + 'company_id' => 'numeric|nullable|exists:companies,id', + 'manager_id' => 'numeric|nullable|exists:users,id', + 'phone' => 'string|max:255|nullable', + 'fax' => 'string|max:255|nullable', + 'notes' => 'string|max:255|nullable', ]; /** diff --git a/tests/Feature/Departments/Api/CreateDepartmentsTest.php b/tests/Feature/Departments/Api/CreateDepartmentsTest.php index 4b887bfc3d..0ddb25319e 100644 --- a/tests/Feature/Departments/Api/CreateDepartmentsTest.php +++ b/tests/Feature/Departments/Api/CreateDepartmentsTest.php @@ -8,29 +8,37 @@ use App\Models\Department; use App\Models\Category; use App\Models\User; use Illuminate\Testing\Fluent\AssertableJson; +use phpDocumentor\Reflection\Location; use Tests\TestCase; class CreateDepartmentsTest extends TestCase { - public function testRequiresPermissionToCreateDepartment() + public function test_requires_permission_to_create_department() { $this->actingAsForApi(User::factory()->create()) ->postJson(route('api.departments.store')) ->assertForbidden(); } - public function testCanCreateDepartment() + public function test_can_create_department_with_all_fields() { + $company = Company::factory()->create(); + //$location = Location::factory()->create(); ??? + $manager = User::factory()->create(); $response = $this->actingAsForApi(User::factory()->superuser()->create()) ->postJson(route('api.departments.store'), [ - 'name' => 'Test Department', - 'notes' => 'Test Note', + 'name' => 'Test Department', + 'company_id' => $company->id, + //'location_id' => $location->id, + 'manager_id' => $manager->id, + 'notes' => 'Test Note', + 'phone' => '1234567890', + 'fax' => '1234567890', ]) ->assertOk() ->assertStatusMessageIs('success') - ->assertStatus(200) ->json(); $this->assertTrue(Department::where('name', 'Test Department')->exists()); @@ -40,7 +48,27 @@ class CreateDepartmentsTest extends TestCase $this->assertEquals('Test Note', $department->notes); } - public function testNoArraysAllowed() + public function test_name_required_for_department() + { + $response = $this->actingAsForApi(User::factory()->superuser()->create()) + ->postJson(route('api.departments.store'), [ + 'company_id' => Company::factory()->create()->id, + ]) + ->assertOk() + ->assertStatusMessageIs('error'); + } + + public function test_only_name_required_for_department() + { + $response = $this->actingAsForApi(User::factory()->superuser()->create()) + ->postJson(route('api.departments.store'), [ + 'name' => 'Test Department', + ]) + ->assertOk() + ->assertStatusMessageIs('success'); + } + + public function test_arrays_not_allowed_for_numeric_fields() { $response = $this->actingAsForApi(User::factory()->superuser()->create()) ->postJson(route('api.departments.store'), [ @@ -51,4 +79,17 @@ class CreateDepartmentsTest extends TestCase ->assertStatusMessageIs('error'); } + public function test_arrays_of_strings_are_not_allowed_for_numeric_fields() + { + $response = $this->actingAsForApi(User::factory()->superuser()->create()) + ->postJson(route('api.departments.store'), [ + 'name' => 'Test Department', + 'company_id' => ['1', '2'], + ]) + ->assertOk() + ->assertStatusMessageIs('error'); + } + + + } From 120316bae0a47a075860ef297d2e0d431649b87a Mon Sep 17 00:00:00 2001 From: spencerrlongg Date: Wed, 21 May 2025 13:36:31 -0500 Subject: [PATCH 3/6] wrong location import --- tests/Feature/Departments/Api/CreateDepartmentsTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Feature/Departments/Api/CreateDepartmentsTest.php b/tests/Feature/Departments/Api/CreateDepartmentsTest.php index 0ddb25319e..381665da5c 100644 --- a/tests/Feature/Departments/Api/CreateDepartmentsTest.php +++ b/tests/Feature/Departments/Api/CreateDepartmentsTest.php @@ -6,9 +6,9 @@ use App\Models\AssetModel; use App\Models\Company; use App\Models\Department; use App\Models\Category; +use App\Models\Location; use App\Models\User; use Illuminate\Testing\Fluent\AssertableJson; -use phpDocumentor\Reflection\Location; use Tests\TestCase; class CreateDepartmentsTest extends TestCase @@ -25,13 +25,13 @@ class CreateDepartmentsTest extends TestCase public function test_can_create_department_with_all_fields() { $company = Company::factory()->create(); - //$location = Location::factory()->create(); ??? + $location = Location::factory()->create(); $manager = User::factory()->create(); $response = $this->actingAsForApi(User::factory()->superuser()->create()) ->postJson(route('api.departments.store'), [ 'name' => 'Test Department', 'company_id' => $company->id, - //'location_id' => $location->id, + 'location_id' => $location->id, 'manager_id' => $manager->id, 'notes' => 'Test Note', 'phone' => '1234567890', From 1b397cd780e8cbb3a9703bfc8db1e5a68dcc6c71 Mon Sep 17 00:00:00 2001 From: spencerrlongg Date: Wed, 21 May 2025 18:17:45 -0500 Subject: [PATCH 4/6] assertDbHas --- .../Departments/Api/CreateDepartmentsTest.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/Feature/Departments/Api/CreateDepartmentsTest.php b/tests/Feature/Departments/Api/CreateDepartmentsTest.php index 381665da5c..4d8e968572 100644 --- a/tests/Feature/Departments/Api/CreateDepartmentsTest.php +++ b/tests/Feature/Departments/Api/CreateDepartmentsTest.php @@ -27,7 +27,8 @@ class CreateDepartmentsTest extends TestCase $company = Company::factory()->create(); $location = Location::factory()->create(); $manager = User::factory()->create(); - $response = $this->actingAsForApi(User::factory()->superuser()->create()) + $user = User::factory()->superuser()->create(); + $response = $this->actingAsForApi($user) ->postJson(route('api.departments.store'), [ 'name' => 'Test Department', 'company_id' => $company->id, @@ -46,6 +47,17 @@ class CreateDepartmentsTest extends TestCase $department = Department::find($response['payload']['id']); $this->assertEquals('Test Department', $department->name); $this->assertEquals('Test Note', $department->notes); + + $this->assertDatabaseHas('departments', [ + 'name' => 'Test Department', + 'company_id' => $company->id, + 'location_id' => $location->id, + 'manager_id' => $manager->id, + 'notes' => 'Test Note', + 'phone' => '1234567890', + 'fax' => '1234567890', + 'created_by' => $user->id, + ]); } public function test_name_required_for_department() From ddb031f091234bfa93b01fc4e342f152d2514c18 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 4 Nov 2025 15:56:23 +0000 Subject: [PATCH 5/6] Fixed #18148 and #17451 - return int for user_count, fixed validation --- .../Controllers/Api/DepartmentsController.php | 29 +++++----- .../Transformers/DepartmentsTransformer.php | 2 +- app/Models/Department.php | 2 +- app/Providers/ValidationServiceProvider.php | 53 +++++++++---------- resources/lang/en-US/validation.php | 2 +- 5 files changed, 42 insertions(+), 46 deletions(-) diff --git a/app/Http/Controllers/Api/DepartmentsController.php b/app/Http/Controllers/Api/DepartmentsController.php index adbe4bc35c..92008bddef 100644 --- a/app/Http/Controllers/Api/DepartmentsController.php +++ b/app/Http/Controllers/Api/DepartmentsController.php @@ -27,18 +27,19 @@ class DepartmentsController extends Controller $allowed_columns = ['id', 'name', 'image', 'users_count', 'notes']; $departments = Department::select( - 'departments.id', - 'departments.name', - 'departments.phone', - 'departments.fax', - 'departments.location_id', - 'departments.company_id', - 'departments.manager_id', - 'departments.created_at', - 'departments.updated_at', - 'departments.image', - 'departments.notes', - )->with('users')->with('location')->with('manager')->with('company')->withCount('users as users_count'); + [ + 'departments.id', + 'departments.name', + 'departments.phone', + 'departments.fax', + 'departments.location_id', + 'departments.company_id', + 'departments.manager_id', + 'departments.created_at', + 'departments.updated_at', + 'departments.image', + 'departments.notes' + ])->with('users')->with('location')->with('manager')->with('company')->withCount('users as users_count'); if ($request->filled('search')) { $departments = $departments->TextSearch($request->input('search')); @@ -105,7 +106,7 @@ class DepartmentsController extends Controller $department->manager_id = ($request->filled('manager_id') ? $request->input('manager_id') : null); if ($department->save()) { - return response()->json(Helper::formatStandardApiResponse('success', $department, trans('admin/departments/message.create.success'))); + return response()->json(Helper::formatStandardApiResponse('success', (new DepartmentsTransformer)->transformDepartment($department), trans('admin/departments/message.create.success'))); } return response()->json(Helper::formatStandardApiResponse('error', null, $department->getErrors())); @@ -121,7 +122,7 @@ class DepartmentsController extends Controller public function show($id) : array { $this->authorize('view', Department::class); - $department = Department::findOrFail($id); + $department = Department::withCount('users as users_count')->findOrFail($id); return (new DepartmentsTransformer)->transformDepartment($department); } diff --git a/app/Http/Transformers/DepartmentsTransformer.php b/app/Http/Transformers/DepartmentsTransformer.php index e072585a12..267413e104 100644 --- a/app/Http/Transformers/DepartmentsTransformer.php +++ b/app/Http/Transformers/DepartmentsTransformer.php @@ -43,7 +43,7 @@ class DepartmentsTransformer 'id' => (int) $department->location->id, 'name' => e($department->location->name), ] : null, - 'users_count' => e($department->users_count), + 'users_count' => (int) ($department->users_count), 'notes' => Helper::parseEscapedMarkedownInline($department->notes), 'created_at' => Helper::getFormattedDateObject($department->created_at, 'datetime'), 'updated_at' => Helper::getFormattedDateObject($department->updated_at, 'datetime'), diff --git a/app/Models/Department.php b/app/Models/Department.php index 4fc72e474f..fda461bdad 100644 --- a/app/Models/Department.php +++ b/app/Models/Department.php @@ -31,7 +31,7 @@ class Department extends SnipeModel ]; protected $rules = [ - 'name' => 'required|max:255|is_unique_department', + 'name' => 'required|max:255|is_unique_across_company_and_location:departments,name', 'location_id' => 'numeric|nullable|exists:locations,id', 'company_id' => 'numeric|nullable|exists:companies,id', 'manager_id' => 'numeric|nullable|exists:users,id', diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php index 13ee6c5729..26bc9ada07 100644 --- a/app/Providers/ValidationServiceProvider.php +++ b/app/Providers/ValidationServiceProvider.php @@ -283,41 +283,36 @@ class ValidationServiceProvider extends ServiceProvider } }); - Validator::extend('is_unique_department', function ($attribute, $value, $parameters, $validator) { + /** + * Check that the 'name' field is unique in the table while within both company_id and location_id + * This is only used by Departments right now, but could be used elsewhere in the future. + */ + Validator::extend('is_unique_across_company_and_location', function ($attribute, $value, $parameters, $validator) { $data = $validator->getData(); + $table = array_get($parameters, 0); - if ( - array_key_exists('location_id', $data) && $data['location_id'] !== null && - array_key_exists('company_id', $data) && $data['company_id'] !== null - ) { - //for updating existing departments - if(array_key_exists('id', $data) && $data['id'] !== null){ - $count = Department::where('name', $data['name']) - ->where('location_id', $data['location_id']) - ->where('company_id', $data['company_id']) - ->whereNotNull('company_id') - ->whereNotNull('location_id') - ->where('id', '!=', $data['id']) - ->count('name'); + $count = DB::table($table)->select($attribute) + ->where($attribute, $value) + ->whereNull('deleted_at'); - return $count < 1; - }else // for entering in new departments - { - $count = Department::where('name', $data['name']) - ->where('location_id', $data['location_id']) - ->where('company_id', $data['company_id']) - ->whereNotNull('company_id') - ->whereNotNull('location_id') - ->count('name'); - - return $count < 1; + if (array_key_exists('id', $data) && $data['id'] !== null) { + $count = $count->where('id', '!=', $data['id']); } - } - else { - return true; - } + + if (array_key_exists('location_id', $data) && $data['location_id'] !== null) { + $count = $count->where('location_id', $data['location_id']); + } + + if (array_key_exists('company_id', $data) && $data['company_id'] !== null) { + $count = $count->where('company_id', $data['company_id']); + } + + $count = $count->count('name'); + return $count < 1; + }); + Validator::extend('not_array', function ($attribute, $value, $parameters, $validator) { return !is_array($value); }); diff --git a/resources/lang/en-US/validation.php b/resources/lang/en-US/validation.php index f80dd70163..2d4af64fb4 100644 --- a/resources/lang/en-US/validation.php +++ b/resources/lang/en-US/validation.php @@ -174,7 +174,7 @@ return [ 'ulid' => 'The :attribute field must be a valid ULID.', 'uuid' => 'The :attribute field must be a valid UUID.', 'fmcs_location' => 'Full multiple company support and location scoping is enabled in the Admin Settings, and the selected location and selected company are not compatible.', - + 'is_unique_across_company_and_location' => 'The :attribute must be unique within the selected company and location.', /* |-------------------------------------------------------------------------- From 4a39d7c67a8a858564bdca6df9302bc3c14ec409 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 4 Nov 2025 16:06:15 +0000 Subject: [PATCH 6/6] Use transformer for dept update responses --- app/Http/Controllers/Api/DepartmentsController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Http/Controllers/Api/DepartmentsController.php b/app/Http/Controllers/Api/DepartmentsController.php index 92008bddef..076b5a4c4b 100644 --- a/app/Http/Controllers/Api/DepartmentsController.php +++ b/app/Http/Controllers/Api/DepartmentsController.php @@ -142,7 +142,7 @@ class DepartmentsController extends Controller $department = $request->handleImages($department); if ($department->save()) { - return response()->json(Helper::formatStandardApiResponse('success', $department, trans('admin/departments/message.update.success'))); + return response()->json(Helper::formatStandardApiResponse('success', (new DepartmentsTransformer)->transformDepartment($department), trans('admin/departments/message.update.success'))); } return response()->json(Helper::formatStandardApiResponse('error', null, $department->getErrors()));