From d9688fd3b04846d73901b1415199142b11f457c5 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Sat, 5 Jul 2025 02:54:27 -0500 Subject: [PATCH] School controller refactoring and testing. --- app/Actions/Fortify/CreateNewUser.php | 1 + app/Http/Controllers/SchoolController.php | 95 +++------ app/Rules/ValidRegistrationCode.php | 4 + .../Http/Controllers/SchoolControllerTest.php | 197 +++++++++++++++++- 4 files changed, 235 insertions(+), 62 deletions(-) diff --git a/app/Actions/Fortify/CreateNewUser.php b/app/Actions/Fortify/CreateNewUser.php index c60dadb..89fe61e 100644 --- a/app/Actions/Fortify/CreateNewUser.php +++ b/app/Actions/Fortify/CreateNewUser.php @@ -53,6 +53,7 @@ class CreateNewUser implements CreatesNewUsers 'password' => Hash::make($input['password']), ]); + // TODO: Move logging to observer $message = 'New User Registered - '.$input['email'] .'
Name: '.$input['first_name'].' '.$input['last_name'] .'
Judging Pref: '.$input['judging_preference'] diff --git a/app/Http/Controllers/SchoolController.php b/app/Http/Controllers/SchoolController.php index 070dada..83524de 100644 --- a/app/Http/Controllers/SchoolController.php +++ b/app/Http/Controllers/SchoolController.php @@ -2,6 +2,8 @@ namespace App\Http\Controllers; +use App\Actions\Fortify\CreateNewUser; +use App\Actions\Schools\AddSchoolEmailDomain; use App\Actions\Schools\AssignUserToSchool; use App\Actions\Schools\CreateSchool; use App\Actions\Schools\SetHeadDirector; @@ -12,15 +14,12 @@ use App\Models\AuditLogEntry; use App\Models\School; use App\Models\SchoolEmailDomain; use App\Models\User; -use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; -use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Mail; use Illuminate\Support\Str; use function abort; -use function auditionLog; use function redirect; use function request; @@ -97,52 +96,38 @@ class SchoolController extends Controller public function addDirector(School $school) { - if (auth()->user()->school_id !== $school->id) { - return redirect()->back()->with('error', 'No adding directors to another school'); + abort(403); } if (! auth()->user()->hasFlag('head_director')) { - return redirect()->back()->with('error', 'Only the head director can add directors to a school'); + abort(403); } - $validData = request()->validate([ - 'first_name' => ['required'], - 'last_name' => ['required'], - 'email' => ['required', 'email', 'unique:users'], - 'cell_phone' => ['required'], - 'judging_preference' => ['required'], - ]); - // Generate a random password + + $userCreator = app(CreateNewUser::class); $randomPassword = Str::random(12); - $newUser = User::create([ - 'first_name' => $validData['first_name'], - 'last_name' => $validData['last_name'], - 'email' => $validData['email'], - 'cell_phone' => $validData['cell_phone'], - 'judging_preference' => $validData['judging_preference'], - 'password' => Hash::make($randomPassword), - 'school_id' => auth()->user()->school_id, + $data = request()->all(); + $data['password'] = $randomPassword; + $data['password_confirmation'] = $randomPassword; + $newDirector = $userCreator->create($data); + $newDirector->update([ + 'school_id' => $school->id, ]); - $logMessage = 'Created user '.$newUser->full_name().' - '.$newUser->email.' as a director at '.$newUser->school->name; - $logAffected = ['users' => [$newUser->id], 'schools' => [$newUser->school_id]]; - auditionLog($logMessage, $logAffected); - Mail::to($newUser->email)->send(new NewUserPassword($newUser, $randomPassword)); + + Mail::to($newDirector->email)->send(new NewUserPassword($newDirector, $randomPassword)); return redirect()->back()->with('success', 'Director added'); } - public function setHeadDirector( - School $school, - User $user, - SetHeadDirector $headSetter - ) { + public function setHeadDirector(School $school, User $user, SetHeadDirector $headSetter) + { if (auth()->user()->school_id !== $school->id) { - return redirect()->back()->with('error', 'No setting the head director for another school'); + abort(403); } if (! auth()->user()->hasFlag('head_director')) { - return redirect()->back()->with('error', 'Only the head director can name a new head director'); + abort(403); } if ($school->id !== $user->school_id) { - return redirect()->back()->with('error', 'The proposed head director must be at your school'); + abort(403); } try { $headSetter->setHeadDirector($user); @@ -150,50 +135,38 @@ class SchoolController extends Controller return redirect()->back()->with('error', $e->getMessage()); } - return redirect()->back()->with('success', 'New head director set'); + return redirect()->route('schools.show', $school)->with('success', 'New head director set'); } - public function addDomain( - School $school - ) { + public function addDomain(School $school) + { if (auth()->user()->school_id !== $school->id) { - return redirect()->back()->with('error', 'No adding domains for another school'); + abort(403); } if (! auth()->user()->hasFlag('head_director')) { - return redirect()->back()->with('error', 'Only the head director can add domains'); + abort(403); } $verifiedData = request()->validate([ 'domain' => ['required'], ]); - try { - SchoolEmailDomain::create([ - 'school_id' => $school->id, - 'domain' => $verifiedData['domain'], - ]); - } catch (UniqueConstraintViolationException $e) { - return redirect()->back()->with('error', 'That domain is already associated with your school'); - } - $logMessage = 'Added domain '.$verifiedData['domain'].' to school '.$school->name; - $logAffected = ['schools' => [$school->id]]; - auditionLog($logMessage, $logAffected); + app(AddSchoolEmailDomain::class)->addDomain($school, $verifiedData['domain']); - return redirect()->back()->with('success', 'Domain added'); + return redirect()->route('schools.show', $school)->with('success', 'Domain added'); } - public function deleteDomain( - SchoolEmailDomain $domain - ) { + public function deleteDomain(SchoolEmailDomain $domain) + { if (auth()->user()->school_id !== $domain->school_id) { - return redirect()->back()->with('error', 'No deleting domains for another school'); + abort(403); } if (! auth()->user()->hasFlag('head_director')) { - return redirect()->back()->with('error', 'Only the head director can delete domains'); + abort(403); } - $logMessage = 'Deleted domain '.$domain->domain.' from school '.$domain->school->name; - $logAffected = ['schools' => [$domain->school_id]]; - auditionLog($logMessage, $logAffected); + $domain->delete(); - return redirect()->back()->with('success', 'Domain deleted'); + return redirect() + ->route('schools.show', auth()->user()->school) + ->with('success', 'Domain deleted'); } } diff --git a/app/Rules/ValidRegistrationCode.php b/app/Rules/ValidRegistrationCode.php index 647a3b4..9676924 100644 --- a/app/Rules/ValidRegistrationCode.php +++ b/app/Rules/ValidRegistrationCode.php @@ -18,6 +18,10 @@ class ValidRegistrationCode implements ValidationRule */ public function validate(string $attribute, mixed $value, Closure $fail): void { + if (auth()->check()) { + return; // Skip validation for authenticated users + } + if ($value !== Settings::get('registrationCode')) { $fail('Incorrect registration code provided'); } diff --git a/tests/Feature/app/Http/Controllers/SchoolControllerTest.php b/tests/Feature/app/Http/Controllers/SchoolControllerTest.php index e9beba0..b533386 100644 --- a/tests/Feature/app/Http/Controllers/SchoolControllerTest.php +++ b/tests/Feature/app/Http/Controllers/SchoolControllerTest.php @@ -1,6 +1,7 @@ assertRedirect(route('schools.show', $school)); expect($school->name === 'Eastman')->toBeFalse(); }); - +}); + +describe('addDirector method', function () { + it('will not all the user to add users to another school', function () { + $user = User::factory()->create(); + $school = School::factory()->create(); + $user->school_id = $school->id; + $user->save(); + actingAs($user); + $otherSchool = School::factory()->create(); + $response = $this->post(route('schools.add_director', $otherSchool)); + $response->assertStatus(403); + }); + + it('will only add users for the head director role', function () { + $user = User::factory()->create(); + $school = School::factory()->create(); + $user->school_id = $school->id; + $user->save(); + actingAs($user); + $response = $this->post(route('schools.add_director', $school)); + $response->assertStatus(403); + }); + + it('will add a user to the school for the head director', function () { + Mail::fake(); + $user = User::factory()->create(); + $school = School::factory()->create(); + $user->school_id = $school->id; + $user->save(); + $user->addFlag('head_director'); + actingAs($user); + expect(User::count())->toEqual(1); + $response = $this->post(route('schools.add_director', $school), [ + 'first_name' => 'Jean Luc', + 'last_name' => 'Picard', + 'email' => 'j.picard@starfleet.com', + 'cell_phone' => '1701', + 'judging_preference' => 'Shut up Wesley', + ]); + expect(User::count())->toEqual(2); + Mail::assertSentCount(1); + }); +}); + +describe('setHeadDirector method', function () { + it('will not allow a director to alter another school', function () { + $school = School::factory()->create(); + $user = User::factory()->create(); + $otherUser = User::factory()->create(); + $user->school_id = $school->id; + $user->save(); + $otherUser->school_id = $school->id; + $otherUser->save(); + $user->addFlag('head_director'); + actingAs($user); + $otherSchool = School::factory()->create(); + $response = $this->get(route('schools.set_head_director', [$otherSchool, $otherUser])); + $response->assertStatus(403); + }); + + it('only a head director can replace themselves', function () { + $school = School::factory()->create(); + $user = User::factory()->create(); + $otherUser = User::factory()->create(); + $user->school_id = $school->id; + $user->save(); + $otherUser->school_id = $school->id; + $otherUser->save(); + actingAs($user); + $response = $this->get(route('schools.set_head_director', [$school, $otherUser])); + $response->assertStatus(403); + }); + + it('you can only promote users at your school', function () { + $school = School::factory()->create(); + $user = User::factory()->create(); + $otherUser = User::factory()->create(); + $user->school_id = $school->id; + $user->save(); + $user->addFlag('head_director'); + actingAs($user); + $response = $this->get(route('schools.set_head_director', [$school, $otherUser])); + $response->assertStatus(403); + }); + + it('promotes another director', function () { + $school = School::factory()->create(); + $user = User::factory()->create(); + $otherUser = User::factory()->create(); + $user->school_id = $school->id; + $user->save(); + $otherUser->school_id = $school->id; + $otherUser->save(); + $user->addFlag('head_director'); + actingAs($user); + $response = $this->get(route('schools.set_head_director', [$school, $otherUser])); + $response->assertRedirect(route('schools.show', $school)) + ->assertSessionHas('success'); + $user->refresh(); + $otherUser->refresh(); + expect($user->hasFlag('head_director'))->toBeFalse(); + expect($otherUser->hasFlag('head_director'))->toBeTrue(); + // TODO: Mock the promoter action so we can test the exception handling + }); +}); + +describe('addDomain method', function () { + it('will not allow a user to add a domain to a school they are not a member of', function () { + $user = User::factory()->create(); + $school = School::factory()->create(); + $user->school_id = $school->id; + $user->save(); + actingAs($user); + $otherSchool = School::factory()->create(); + $response = $this->post(route('schools.add_domain', $otherSchool)); + $response->assertStatus(403); + }); + it('will only allow a head director to add a domain', function () { + $user = User::factory()->create(); + $school = School::factory()->create(); + $user->school_id = $school->id; + $user->save(); + actingAs($user); + $response = $this->post(route('schools.add_domain', $school)); + $response->assertStatus(403); + }); + it('will add a domain to the school', function () { + $user = User::factory()->create(); + $school = School::factory()->create(); + $user->school_id = $school->id; + $user->save(); + $user->addFlag('head_director'); + $user->refresh(); + actingAs($user); + $response = $this->post(route('schools.add_domain', $school), [ + 'domain' => 'test.com', + ]); + $response->assertRedirect(route('schools.show', $school)) + ->assertSessionHas('success'); + $school->refresh(); + expect(SchoolEmailDomain::where('domain', 'test.com') + ->where('school_id', $school->id) + ->exists())->toBeTrue(); + }); +}); + +describe('removeDomain method', function () { + it('will not allow a user to remove a domain from a school they are not a member of', function () { + $user = User::factory()->create(); + $school = School::factory()->create(); + $user->school_id = $school->id; + $user->save(); + actingAs($user); + $otherSchool = School::factory()->create(); + $domain = SchoolEmailDomain::create([ + 'school_id' => $otherSchool->id, + 'domain' => 'test.com', + ]); + $response = $this->get(route('schools.delete_domain', $domain)); + $response->assertStatus(403); + }); + it('will only allow a head director to remove a domain', function () { + $user = User::factory()->create(); + $school = School::factory()->create(); + $user->school_id = $school->id; + $user->save(); + actingAs($user); + $domain = SchoolEmailDomain::create([ + 'school_id' => $school->id, + 'domain' => 'test.com', + ]); + $response = $this->get(route('schools.delete_domain', $domain)); + $response->assertStatus(403); + }); + it('will remove a domain from the school', function () { + $user = User::factory()->create(); + $school = School::factory()->create(); + $user->school_id = $school->id; + $user->save(); + $user->addFlag('head_director'); + $domain = SchoolEmailDomain::create([ + 'school_id' => $school->id, + 'domain' => 'test.com', + ]); + actingAs($user); + $response = $this->get(route('schools.delete_domain', $domain)); + $response->assertRedirect(route('schools.show', $school)) + ->assertSessionHas('success'); + $school->refresh(); + expect(SchoolEmailDomain::where('domain', 'test.com') + ->where('school_id', $school->id) + ->exists())->toBeFalse(); + + }); });