From 999605f832bfa25b9f92f323209bdc25b8e8c3f4 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 30 Aug 2023 17:10:50 -0700 Subject: [PATCH 1/9] Add failing test --- tests/Feature/Api/Users/UsersUpdateTest.php | 30 +++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 tests/Feature/Api/Users/UsersUpdateTest.php diff --git a/tests/Feature/Api/Users/UsersUpdateTest.php b/tests/Feature/Api/Users/UsersUpdateTest.php new file mode 100644 index 000000000000..db1ebfe7255d --- /dev/null +++ b/tests/Feature/Api/Users/UsersUpdateTest.php @@ -0,0 +1,30 @@ +withoutExceptionHandling(); + $admin = User::factory()->superuser()->create(); + $user = User::factory()->forDepartment(['name' => 'Department A'])->create(); + $department = Department::factory()->create(); + + $this->actingAsForApi($admin)->patch(route('api.users.update', $user), [ + 'department_id' => ['id' => $department->id] + ])->assertOk(); + + $this->assertTrue( + $user->fresh()->department()->is($department), + 'User is not associated with expected department' + ); + } +} From a7996596107407ac3bed6cd8d988e3388fcab058 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 30 Aug 2023 17:33:23 -0700 Subject: [PATCH 2/9] Scaffold tests and add context --- tests/Feature/Api/Users/UsersUpdateTest.php | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/tests/Feature/Api/Users/UsersUpdateTest.php b/tests/Feature/Api/Users/UsersUpdateTest.php index db1ebfe7255d..a8091ba5011f 100644 --- a/tests/Feature/Api/Users/UsersUpdateTest.php +++ b/tests/Feature/Api/Users/UsersUpdateTest.php @@ -11,15 +11,28 @@ class UsersUpdateTest extends TestCase { use InteractsWithSettings; - public function testSomething() + + public function testCanUpdateUserViaPatch() + { + $this->markTestIncomplete(); + } + + public function testCanUpdateUserViaPut() + { + $this->markTestIncomplete(); + } + + public function testDepartmentPatching() { - $this->withoutExceptionHandling(); $admin = User::factory()->superuser()->create(); $user = User::factory()->forDepartment(['name' => 'Department A'])->create(); $department = Department::factory()->create(); $this->actingAsForApi($admin)->patch(route('api.users.update', $user), [ - 'department_id' => ['id' => $department->id] + // This isn't valid but doesn't return an error + 'department_id' => ['id' => $department->id], + // This is the correct syntax + // 'department_id' => $department->id, ])->assertOk(); $this->assertTrue( From 1c3c36f2a044d08a46f65d398dee24c87c739cdf Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 6 Sep 2023 16:14:14 -0700 Subject: [PATCH 3/9] Begin to implement patch test --- tests/Feature/Api/Users/UsersUpdateTest.php | 85 +++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/tests/Feature/Api/Users/UsersUpdateTest.php b/tests/Feature/Api/Users/UsersUpdateTest.php index a8091ba5011f..c81301235471 100644 --- a/tests/Feature/Api/Users/UsersUpdateTest.php +++ b/tests/Feature/Api/Users/UsersUpdateTest.php @@ -2,8 +2,12 @@ namespace Tests\Feature\Api\Users; +use App\Models\Company; use App\Models\Department; +use App\Models\Group; +use App\Models\Location; use App\Models\User; +use Illuminate\Support\Facades\Hash; use Tests\Support\InteractsWithSettings; use Tests\TestCase; @@ -11,8 +15,89 @@ class UsersUpdateTest extends TestCase { use InteractsWithSettings; + public function testValidationForUpdatingUserViaPatch() + { + $this->markTestIncomplete(); + } public function testCanUpdateUserViaPatch() + { + $admin = User::factory()->superuser()->create(); + $manager = User::factory()->create(); + $company = Company::factory()->create(); + $department = Department::factory()->create(); + $location = Location::factory()->create(); + [$groupA, $groupB] = Group::factory()->count(2)->create(); + + $user = User::factory()->create([ + 'activated' => false, + 'two_factor_enrolled' => false, + 'two_factor_optin' => false, + 'remote' => false, + 'vip' => false, + ]); + + $this->actingAsForApi($admin) + ->patch(route('api.users.update', $user), [ + 'first_name' => 'Mabel', + 'last_name' => 'Mora', + 'username' => 'mabel', + 'password' => 'super-secret', + 'email' => 'mabel@onlymurderspod.com', + // @todo: + // 'permissions' => '', + 'activated' => true, + 'phone' => '619-555-5555', + 'jobtitle' => 'Host', + 'manager_id' => $manager->id, + 'employee_num' => '1111', + 'notes' => 'Pretty good artist', + 'company_id' => $company->id, + 'two_factor_enrolled' => true, + 'two_factor_optin' => true, + 'department_id' => $department->id, + 'location_id' => $location->id, + 'remote' => true, + 'groups' => $groupA->id, + 'vip' => true, + 'start_date' => '2021-08-01', + 'end_date' => '2025-12-31', + ]) + ->assertOk(); + + $user->refresh(); + $this->assertEquals('Mabel', $user->first_name); + $this->assertEquals('Mora', $user->last_name); + $this->assertEquals('mabel', $user->username); + $this->assertTrue(Hash::check('super-secret', $user->password)); + $this->assertEquals('mabel@onlymurderspod.com', $user->email); + $this->assertTrue($user->activated); + $this->assertEquals('619-555-5555', $user->phone); + $this->assertEquals('Host', $user->jobtitle); + $this->assertTrue($user->manager->is($manager)); + $this->assertEquals('1111', $user->employee_num); + $this->assertEquals('Pretty good artist', $user->notes); + $this->assertTrue($user->company->is($company)); + // @todo: + // $this->assertEquals(1, $user->two_factor_enrolled); + // $this->assertEquals(1, $user->two_factor_optin); + $this->assertTrue($user->department->is($department)); + $this->assertTrue($user->location->is($location)); + $this->assertEquals(1, $user->remote); + $this->assertTrue($user->groups->contains($groupA)); + $this->assertTrue($user->vip); + $this->assertEquals('2021-08-01', $user->start_date); + $this->assertEquals('2025-12-31', $user->end_date); + + // `groups` can be an id or array or ids + $this->patch(route('api.users.update', $user), ['groups' => [$groupA->id, $groupB->id]]); + + $user->refresh(); + $this->assertTrue($user->groups->contains($groupA)); + $this->assertTrue($user->groups->contains($groupB)); + } + + public function testValidationForUpdatingUserViaPut() { $this->markTestIncomplete(); } From 899c2eb19b48c1ecf8d741ab592f375b6143b2ca Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 7 Sep 2023 12:34:50 -0700 Subject: [PATCH 4/9] Implement test case --- app/Http/Requests/SaveUserRequest.php | 1 + tests/Feature/Api/Users/UsersUpdateTest.php | 29 +++++---------------- 2 files changed, 8 insertions(+), 22 deletions(-) diff --git a/app/Http/Requests/SaveUserRequest.php b/app/Http/Requests/SaveUserRequest.php index 98e561549efa..6527fca56f05 100644 --- a/app/Http/Requests/SaveUserRequest.php +++ b/app/Http/Requests/SaveUserRequest.php @@ -32,6 +32,7 @@ public function response(array $errors) public function rules() { $rules = [ + 'department_id' => 'nullable|integer|exists:departments,id', 'manager_id' => 'nullable|exists:users,id', ]; diff --git a/tests/Feature/Api/Users/UsersUpdateTest.php b/tests/Feature/Api/Users/UsersUpdateTest.php index c81301235471..14c9dcdbcbc1 100644 --- a/tests/Feature/Api/Users/UsersUpdateTest.php +++ b/tests/Feature/Api/Users/UsersUpdateTest.php @@ -15,11 +15,6 @@ class UsersUpdateTest extends TestCase { use InteractsWithSettings; - public function testValidationForUpdatingUserViaPatch() - { - $this->markTestIncomplete(); - } - public function testCanUpdateUserViaPatch() { $admin = User::factory()->superuser()->create(); @@ -38,7 +33,7 @@ public function testCanUpdateUserViaPatch() ]); $this->actingAsForApi($admin) - ->patch(route('api.users.update', $user), [ + ->patchJson(route('api.users.update', $user), [ 'first_name' => 'Mabel', 'last_name' => 'Mora', 'username' => 'mabel', @@ -107,22 +102,12 @@ public function testCanUpdateUserViaPut() $this->markTestIncomplete(); } - public function testDepartmentPatching() + public function testDepartmentValidation() { - $admin = User::factory()->superuser()->create(); - $user = User::factory()->forDepartment(['name' => 'Department A'])->create(); - $department = Department::factory()->create(); - - $this->actingAsForApi($admin)->patch(route('api.users.update', $user), [ - // This isn't valid but doesn't return an error - 'department_id' => ['id' => $department->id], - // This is the correct syntax - // 'department_id' => $department->id, - ])->assertOk(); - - $this->assertTrue( - $user->fresh()->department()->is($department), - 'User is not associated with expected department' - ); + $this->actingAsForApi(User::factory()->superuser()->create()) + ->patchJson(route('api.users.update', User::factory()->create()), [ + // This isn't valid but was not returning an error + 'department_id' => ['id' => 1], + ])->assertJsonValidationErrorFor('department_id'); } } From 56e6205667a7cafee37d4144383891ef65c34cfc Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 7 Sep 2023 12:45:03 -0700 Subject: [PATCH 5/9] Formatting --- tests/Feature/Api/Users/UsersUpdateTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Feature/Api/Users/UsersUpdateTest.php b/tests/Feature/Api/Users/UsersUpdateTest.php index 14c9dcdbcbc1..7c070c04cd6d 100644 --- a/tests/Feature/Api/Users/UsersUpdateTest.php +++ b/tests/Feature/Api/Users/UsersUpdateTest.php @@ -106,8 +106,8 @@ public function testDepartmentValidation() { $this->actingAsForApi(User::factory()->superuser()->create()) ->patchJson(route('api.users.update', User::factory()->create()), [ - // This isn't valid but was not returning an error - 'department_id' => ['id' => 1], - ])->assertJsonValidationErrorFor('department_id'); + // This isn't valid but was not returning an error + 'department_id' => ['id' => 1], + ])->assertJsonValidationErrorFor('department_id'); } } From 39ff575ac15cc76baac82917e7c53066142e0729 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 7 Sep 2023 13:16:08 -0700 Subject: [PATCH 6/9] Remove unused test cases --- tests/Feature/Api/Users/UsersUpdateTest.php | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/tests/Feature/Api/Users/UsersUpdateTest.php b/tests/Feature/Api/Users/UsersUpdateTest.php index 7c070c04cd6d..00e77357c9ea 100644 --- a/tests/Feature/Api/Users/UsersUpdateTest.php +++ b/tests/Feature/Api/Users/UsersUpdateTest.php @@ -91,23 +91,4 @@ public function testCanUpdateUserViaPatch() $this->assertTrue($user->groups->contains($groupA)); $this->assertTrue($user->groups->contains($groupB)); } - - public function testValidationForUpdatingUserViaPut() - { - $this->markTestIncomplete(); - } - - public function testCanUpdateUserViaPut() - { - $this->markTestIncomplete(); - } - - public function testDepartmentValidation() - { - $this->actingAsForApi(User::factory()->superuser()->create()) - ->patchJson(route('api.users.update', User::factory()->create()), [ - // This isn't valid but was not returning an error - 'department_id' => ['id' => 1], - ])->assertJsonValidationErrorFor('department_id'); - } } From a7a70f6981496c6ccacc6c64be816de6d5e1f332 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 7 Sep 2023 13:21:27 -0700 Subject: [PATCH 7/9] Test permissions update --- tests/Feature/Api/Users/UsersUpdateTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Feature/Api/Users/UsersUpdateTest.php b/tests/Feature/Api/Users/UsersUpdateTest.php index 00e77357c9ea..e05bfa81af60 100644 --- a/tests/Feature/Api/Users/UsersUpdateTest.php +++ b/tests/Feature/Api/Users/UsersUpdateTest.php @@ -39,8 +39,7 @@ public function testCanUpdateUserViaPatch() 'username' => 'mabel', 'password' => 'super-secret', 'email' => 'mabel@onlymurderspod.com', - // @todo: - // 'permissions' => '', + 'permissions' => '{"a.new.permission":"1"}', 'activated' => true, 'phone' => '619-555-5555', 'jobtitle' => 'Host', @@ -66,6 +65,7 @@ public function testCanUpdateUserViaPatch() $this->assertEquals('mabel', $user->username); $this->assertTrue(Hash::check('super-secret', $user->password)); $this->assertEquals('mabel@onlymurderspod.com', $user->email); + $this->assertArrayHasKey('a.new.permission', $user->decodePermissions()); $this->assertTrue($user->activated); $this->assertEquals('619-555-5555', $user->phone); $this->assertEquals('Host', $user->jobtitle); From 4caa501996a64ad8f5cc3f30397e491362ec49b9 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 7 Sep 2023 13:28:32 -0700 Subject: [PATCH 8/9] Relax property type check --- app/Http/Requests/SaveUserRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Http/Requests/SaveUserRequest.php b/app/Http/Requests/SaveUserRequest.php index 6527fca56f05..b38193c15ac8 100644 --- a/app/Http/Requests/SaveUserRequest.php +++ b/app/Http/Requests/SaveUserRequest.php @@ -32,7 +32,7 @@ public function response(array $errors) public function rules() { $rules = [ - 'department_id' => 'nullable|integer|exists:departments,id', + 'department_id' => 'nullable|exists:departments,id', 'manager_id' => 'nullable|exists:users,id', ]; From c6c1c64c1e5f1be72dccda49c31ca017cff24a04 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 7 Sep 2023 13:30:05 -0700 Subject: [PATCH 9/9] Remove todo --- tests/Feature/Api/Users/UsersUpdateTest.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/Feature/Api/Users/UsersUpdateTest.php b/tests/Feature/Api/Users/UsersUpdateTest.php index e05bfa81af60..9337080c42e2 100644 --- a/tests/Feature/Api/Users/UsersUpdateTest.php +++ b/tests/Feature/Api/Users/UsersUpdateTest.php @@ -26,8 +26,6 @@ public function testCanUpdateUserViaPatch() $user = User::factory()->create([ 'activated' => false, - 'two_factor_enrolled' => false, - 'two_factor_optin' => false, 'remote' => false, 'vip' => false, ]); @@ -47,8 +45,6 @@ public function testCanUpdateUserViaPatch() 'employee_num' => '1111', 'notes' => 'Pretty good artist', 'company_id' => $company->id, - 'two_factor_enrolled' => true, - 'two_factor_optin' => true, 'department_id' => $department->id, 'location_id' => $location->id, 'remote' => true, @@ -73,9 +69,6 @@ public function testCanUpdateUserViaPatch() $this->assertEquals('1111', $user->employee_num); $this->assertEquals('Pretty good artist', $user->notes); $this->assertTrue($user->company->is($company)); - // @todo: - // $this->assertEquals(1, $user->two_factor_enrolled); - // $this->assertEquals(1, $user->two_factor_optin); $this->assertTrue($user->department->is($department)); $this->assertTrue($user->location->is($location)); $this->assertEquals(1, $user->remote);