Merge remote-tracking branch 'origin/develop'

snipe · snipe · commit 029f3030a7de · 2025-11-04T16:06:35.000Z
diff --git a/app/Http/Controllers/Api/DepartmentsController.php b/app/Http/Controllers/Api/DepartmentsController.php
@@ -4,6 +4,7 @@
 
 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;
@@ -26,18 +27,19 @@ public function index(Request $request) : JsonResponse | array
         $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'));
@@ -94,18 +96,17 @@ public function index(Request $request) : JsonResponse | array
      * @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;
-        $department->fill($request->all());
+        $department->fill($request->validated());
         $department = $request->handleImages($department);
 
         $department->created_by = auth()->id();
         $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 @@ public function store(ImageUploadRequest $request) : JsonResponse
     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);
     }
 
@@ -141,7 +142,7 @@ public function update(ImageUploadRequest $request, $id) : JsonResponse
         $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()));
diff --git a/app/Http/Requests/StoreDepartmentRequest.php b/app/Http/Requests/StoreDepartmentRequest.php
@@ -0,0 +1,32 @@
+<?php
+
+namespace App\Http\Requests;
+
+use App\Models\Department;
+use Illuminate\Foundation\Http\FormRequest;
+use Illuminate\Support\Facades\Gate;
+
+class StoreDepartmentRequest extends ImageUploadRequest
+{
+    /**
+     * Determine if the user is authorized to make this request.
+     */
+    public function authorize(): bool
+    {
+        return Gate::allows('create', new Department);
+    }
+
+    /**
+     * Get the validation rules that apply to the request.
+     *
+     * @return array<string, \Illuminate\Contracts\Validation\ValidationRule|array<mixed>|string>
+     */
+    public function rules(): array
+    {
+        $modelRules = (new Department)->getRules();
+
+        return array_merge(
+            $modelRules,
+        );
+    }
+}
diff --git a/app/Http/Transformers/DepartmentsTransformer.php b/app/Http/Transformers/DepartmentsTransformer.php
@@ -43,7 +43,7 @@ public function transformDepartment(Department $department = null)
                     '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
@@ -31,10 +31,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_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',
+        'phone'       => 'string|max:255|nullable',
+        'fax'         => 'string|max:255|nullable',
+        'notes'       => 'string|max:255|nullable',
     ];
 
     /**
diff --git a/app/Providers/ValidationServiceProvider.php b/app/Providers/ValidationServiceProvider.php
@@ -283,41 +283,36 @@ public function boot()
             }
         });
 
-        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');
-
-                    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');
+            $count = DB::table($table)->select($attribute)
+                ->where($attribute, $value)
+                ->whereNull('deleted_at');
 
-                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
@@ -174,7 +174,7 @@
     '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.',
 
     /*
     |--------------------------------------------------------------------------
diff --git a/tests/Feature/Departments/Api/CreateDepartmentsTest.php b/tests/Feature/Departments/Api/CreateDepartmentsTest.php
@@ -3,8 +3,10 @@
 namespace Tests\Feature\Departments\Api;
 
 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 Tests\TestCase;
@@ -13,30 +15,93 @@ 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()
     {
-        $response = $this->actingAsForApi(User::factory()->superuser()->create())
+        $company = Company::factory()->create();
+        $location = Location::factory()->create();
+        $manager = User::factory()->create();
+        $user = User::factory()->superuser()->create();
+        $response = $this->actingAsForApi($user)
             ->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());
 
         $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()
+    {
+        $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'), [
+                'name'       => 'Test Department',
+                'company_id' => [1, 2],
+            ])
+            ->assertOk()
+            ->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');
+    }
+
+
+
 }
