diff --git a/app/Http/Controllers/Api/DepartmentsController.php b/app/Http/Controllers/Api/DepartmentsController.php index 167d5cbc2a..076b5a4c4b 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; @@ -26,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')); @@ -94,18 +96,17 @@ 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; - $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 @@ 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); } @@ -141,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())); 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/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 1569081fdd..fda461bdad 100644 --- 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 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.', /* |-------------------------------------------------------------------------- diff --git a/tests/Feature/Departments/Api/CreateDepartmentsTest.php b/tests/Feature/Departments/Api/CreateDepartmentsTest.php index e0f975dd7f..4d8e968572 100644 --- 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,23 +15,31 @@ 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()); @@ -37,6 +47,61 @@ 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() + { + $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'); + } + + + }