School controller refactoring and testing.

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']
             .'<br>Name: '.$input['first_name'].' '.$input['last_name']
             .'<br>Judging Pref: '.$input['judging_preference']

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'],
+        $userCreator = app(CreateNewUser::class);
-            'last_name' => ['required'],
-            'email' => ['required', 'email', 'unique:users'],
-            'cell_phone' => ['required'],
-            'judging_preference' => ['required'],
-        ]);
-        // Generate a random password
         $randomPassword = Str::random(12);
-        $newUser = User::create([
+        $data = request()->all();
-            'first_name' => $validData['first_name'],
+        $data['password'] = $randomPassword;
-            'last_name' => $validData['last_name'],
+        $data['password_confirmation'] = $randomPassword;
-            'email' => $validData['email'],
+        $newDirector = $userCreator->create($data);
-            'cell_phone' => $validData['cell_phone'],
+        $newDirector->update([
-            'judging_preference' => $validData['judging_preference'],
+            'school_id' => $school->id,
-            'password' => Hash::make($randomPassword),
-            'school_id' => auth()->user()->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]];
+        Mail::to($newDirector->email)->send(new NewUserPassword($newDirector, $randomPassword));
-        auditionLog($logMessage, $logAffected);
-        Mail::to($newUser->email)->send(new NewUserPassword($newUser, $randomPassword));
 
         return redirect()->back()->with('success', 'Director added');
     }
 
-    public function setHeadDirector(
+    public function setHeadDirector(School $school, User $user, SetHeadDirector $headSetter)
-        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(
+    public function addDomain(School $school)
-        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 {
+        app(AddSchoolEmailDomain::class)->addDomain($school, $verifiedData['domain']);
-            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);
 
-        return redirect()->back()->with('success', 'Domain added');
+        return redirect()->route('schools.show', $school)->with('success', 'Domain added');
     }
 
-    public function deleteDomain(
+    public function deleteDomain(SchoolEmailDomain $domain)
-        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');
     }
 }

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');
         }

tests/Feature/app/Http/Controllers/SchoolControllerTest.php

@@ -1,6 +1,7 @@
 <?php
 
 use App\Models\School;
+use App\Models\SchoolEmailDomain;
 use App\Models\User;
 use Illuminate\Foundation\Testing\RefreshDatabase;
 
@@ -186,5 +187,199 @@ describe('update method', function () {
             ->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();
+
+    });
 });