Student controller refactoring and testing.

app/Actions/Students/CreateStudent.php

@@ -4,6 +4,7 @@ namespace App\Actions\Students;
 
 use App\Exceptions\AuditionAdminException;
 use App\Models\Student;
+use Arr;
 
 class CreateStudent
 {
@@ -11,29 +12,33 @@ class CreateStudent
     {
     }
 
-    public function __invoke(
-        string $firstName,
-        string $lastName,
-        int $grade,
-        array $optionalData = [],
-        int|string $school_id = 'user'
-    ): Student {
-        if ($school_id === 'user') {
-            $school_id = auth()->user()->school_id;
+    /**
+     * @throws AuditionAdminException
+     */
+    public function __invoke(array $newData): Student
+    {
+
+        // $newData[] must include keys first_name, last_name, grade - throw an exception if it does not
+        foreach (['first_name', 'last_name', 'grade'] as $key) {
+            if (! Arr::has($newData, $key)) {
+                throw new AuditionAdminException('Missing required data');
             }
-        if (Student::where('first_name', $firstName)->where('last_name', $lastName)
-            ->where('school_id', $school_id)->exists()) {
+        }
+
+        if (! Arr::has($newData, 'school_id')) {
+            $newData['school_id'] = auth()->user()->school_id;
+        }
+        if (Student::where('first_name', $newData['first_name'])->where('last_name', $newData['last_name'])
+            ->where('school_id', $newData['school_id'])->exists()) {
             throw new AuditionAdminException('Student already exists');
         }
 
-        $newStudent = Student::create([
-            'first_name' => $firstName,
-            'last_name' => $lastName,
-            'grade' => $grade,
-            'school_id' => $school_id,
-            'optional_data' => $optionalData,
+        return Student::create([
+            'first_name' => $newData['first_name'],
+            'last_name' => $newData['last_name'],
+            'grade' => $newData['grade'],
+            'school_id' => $newData['school_id'],
+            'optional_data' => $newData['optional_data'] ?? null,
         ]);
-
-        return $newStudent;
     }
 }

app/Actions/Students/UpdateStudent.php

@@ -0,0 +1,48 @@
+<?php
+
+namespace App\Actions\Students;
+
+use App\Exceptions\AuditionAdminException;
+use App\Models\Student;
+use Arr;
+
+class UpdateStudent
+{
+    public function __construct()
+    {
+    }
+
+    /**
+     * @throws AuditionAdminException
+     */
+    public function __invoke(Student $student, array $newData): bool
+    {
+
+        // $newData[] must include keys first_name, last_name, grade - throw an exception if it does not
+        foreach (['first_name', 'last_name', 'grade'] as $key) {
+            if (! Arr::has($newData, $key)) {
+                throw new AuditionAdminException('Missing required data');
+            }
+        }
+
+        if (! Arr::has($newData, 'school_id')) {
+            $newData['school_id'] = auth()->user()->school_id;
+        }
+
+        if (Student::where('first_name', $newData['first_name'])
+            ->where('last_name', $newData['last_name'])
+            ->where('school_id', $newData['school_id'])
+            ->where('id', '!=', $student->id)
+            ->exists()) {
+            throw new AuditionAdminException('Student already exists');
+        }
+
+        return $student->update([
+            'first_name' => $newData['first_name'],
+            'last_name' => $newData['last_name'],
+            'grade' => $newData['grade'],
+            'school_id' => $newData['school_id'],
+            'optional_data' => $newData['optional_data'] ?? null,
+        ]);
+    }
+}

app/Http/Controllers/StudentController.php

@@ -2,9 +2,9 @@
 
 namespace App\Http\Controllers;
 
+use App\Actions\Students\CreateStudent;
 use App\Http\Requests\StudentStoreRequest;
 use App\Models\Audition;
-use App\Models\AuditLogEntry;
 use App\Models\Student;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Auth;
@@ -36,31 +36,19 @@ class StudentController extends Controller
      */
     public function store(StudentStoreRequest $request)
     {
-        if ($request->user()->cannot('create', Student::class)) {
-            abort(403);
-        }
-
-        $student = Student::create([
+        $creator = app(CreateStudent::class);
+        /** @noinspection PhpUnhandledExceptionInspection */
+        $creator([
             'first_name' => $request['first_name'],
             'last_name' => $request['last_name'],
             'grade' => $request['grade'],
-            'school_id' => Auth::user()->school_id,
+            'optional_data' => $request->optional_data,
         ]);
-        if ($request['shirt_size'] !== 'none') {
-            $student->update(['optional_data->shirt_size' => $request['shirt_size']]);
-        }
 
+        /** @codeCoverageIgnoreEnd */
         return redirect('/students')->with('success', 'Student Created');
     }
 
-    /**
-     * Display the specified resource.
-     */
-    public function show(Request $request, Student $student)
-    {
-        //
-    }
-
     /**
      * Show the form for editing the specified resource.
      */
@@ -78,52 +66,17 @@ class StudentController extends Controller
     /**
      * Update the specified resource in storage.
      */
-    public function update(Request $request, Student $student)
+    public function update(StudentStoreRequest $request, Student $student)
     {
-
         if ($request->user()->cannot('update', $student)) {
             abort(403);
         }
-        request()->validate([
-            'first_name' => ['required'],
-            'last_name' => ['required'],
-            'grade' => ['required', 'integer'],
-            'shirt_size' => [
-                'nullable',
-                function ($attribute, $value, $fail) {
-                    if (! array_key_exists($value, Student::$shirtSizes)) {
-                        $fail("The selected $attribute is invalid.");
-                    }
-                },
-            ],
-        ]);
-
-        if (Student::where('first_name', request('first_name'))
-            ->where('last_name', request('last_name'))
-            ->where('school_id', Auth::user()->school_id)
-            ->where('id', '!=', $student->id)
-            ->exists()) {
-            return redirect()->route('students.edit', $student)->with('error',
-                'A student with that name already exists at your school.');
-        }
 
         $student->update([
-            'first_name' => request('first_name'),
-            'last_name' => request('last_name'),
-            'grade' => request('grade'),
-        ]);
-
-        $student->update(['optional_data->shirt_size' => $request['shirt_size']]);
-
-        $message = 'Updated student #'.$student->id.'<br>Name: '.$student->full_name().'<br>Grade: '.$student->grade.'<br>School: '.$student->school->name;
-        AuditLogEntry::create([
-            'user' => auth()->user()->email,
-            'ip_address' => request()->ip(),
-            'message' => $message,
-            'affected' => [
-                'students' => [$student->id],
-                'schools' => [$student->school_id],
-            ],
+            'first_name' => $request['first_name'],
+            'last_name' => $request['last_name'],
+            'grade' => $request['grade'],
+            'optional_data' => $request->optional_data,
         ]);
 
         return redirect('/students')->with('success', 'Student updated successfully.');
@@ -137,16 +90,7 @@ class StudentController extends Controller
         if ($request->user()->cannot('delete', $student)) {
             abort(403);
         }
-        $message = 'Deleted student #'.$student->id.'<br>Name: '.$student->full_name().'<br>Grade: '.$student->grade.'<br>School: '.$student->school->name;
-        AuditLogEntry::create([
-            'user' => auth()->user()->email,
-            'ip_address' => request()->ip(),
-            'message' => $message,
-            'affected' => [
-                'students' => [$student->id],
-                'schools' => [$student->school_id],
-            ],
-        ]);
+
         $student->delete();
 
         return redirect(route('students.index'));

app/Http/Requests/StudentStoreRequest.php

@@ -32,6 +32,14 @@ class StudentStoreRequest extends FormRequest
         ];
     }
 
+    // Helper method to get optional data (optional)
+    public function getOptionalData(): array
+    {
+        return ($this->shirt_size !== 'none')
+            ? ['shirt_size' => $this->shirt_size]
+            : [];
+    }
+
     public function authorize(): bool
     {
         return true;

app/Observers/StudentObserver.php

@@ -5,6 +5,9 @@ namespace App\Observers;
 use App\Models\AuditLogEntry;
 use App\Models\Student;
 
+use function auth;
+use function request;
+
 class StudentObserver
 {
     /**
@@ -29,7 +32,16 @@ class StudentObserver
      */
     public function updated(Student $student): void
     {
-        //
+        $message = 'Updated student #'.$student->id.' - '.$student->full_name().'<br>Grade: '.$student->grade.'<br>School: '.$student->school->name;
+        AuditLogEntry::create([
+            'user' => auth()->user()->email ?? 'none',
+            'ip_address' => request()->ip(),
+            'message' => $message,
+            'affected' => [
+                'students' => [$student->id],
+                'schools' => [$student->school_id],
+            ],
+        ]);
     }
 
     /**
@@ -37,22 +49,15 @@ class StudentObserver
      */
     public function deleted(Student $student): void
     {
-        //
-    }
-
-    /**
-     * Handle the Student "restored" event.
-     */
-    public function restored(Student $student): void
-    {
-        //
-    }
-
-    /**
-     * Handle the Student "force deleted" event.
-     */
-    public function forceDeleted(Student $student): void
-    {
-        //
+        $message = 'Deleted student #'.$student->id.' - '.$student->full_name().'<br>Grade: '.$student->grade.'<br>School: '.$student->school->name;
+        AuditLogEntry::create([
+            'user' => auth()->user()->email ?? 'none',
+            'ip_address' => request()->ip(),
+            'message' => $message,
+            'affected' => [
+                'students' => [$student->id],
+                'schools' => [$student->school_id],
+            ],
+        ]);
     }
 }

routes/user.php

@@ -7,7 +7,6 @@ use App\Http\Controllers\EntryController;
 use App\Http\Controllers\PdfInvoiceController;
 use App\Http\Controllers\SchoolController;
 use App\Http\Controllers\StudentController;
-use App\Http\Controllers\UserController;
 use Illuminate\Support\Facades\Route;
 
 Route::middleware(['auth', 'verified'])->group(function () {
@@ -40,7 +39,7 @@ Route::middleware([
     'auth', 'verified',
 ])->controller(StudentController::class)->group(function () {
     Route::get('/students', 'index')->name('students.index');
-    Route::post('students', 'store')->name('students.store');
+    Route::post('students', 'store')->middleware(['can:create,App\Models\Student'])->name('students.store');
     Route::get('/students/{student}/edit', 'edit')->name('students.edit');
     Route::patch('/students/{student}', 'update')->name('students.update');
     Route::delete('/students/{student}', 'destroy')->name('students.destroy');

tests/Feature/app/Actions/Students/CreateStudentTest.php

@@ -16,24 +16,23 @@ beforeEach(function () {
 });
 
 it('can create a student with basic data', function () {
-    ($this->creator)(
-        'John',
-        'Doe',
-        8,
-        [],
-        School::factory()->create()->id
-    );
+    ($this->creator)([
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 8,
+        'school_id' => School::factory()->create()->id,
+    ]);
     expect(Student::first())->toBeInstanceOf(Student::class);
 });
 
 it('can include optional data', function () {
-    ($this->creator)(
-        'John',
-        'Doe',
-        8,
-        ['shirt_size' => 'M'],
-        School::factory()->create()->id
-    );
+    ($this->creator)([
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 8,
+        'optional_data' => ['shirt_size' => 'M'],
+        'school_id' => School::factory()->create()->id,
+    ]);
     $student = Student::first();
     expect($student->optional_data['shirt_size'])->toEqual('M');
 });
@@ -44,30 +43,40 @@ it('uses the current users school if none is specified', function () {
     $user->school_id = $school->id;
     $user->save();
     $this->actingAs($user);
-    ($this->creator)(
-        'John',
-        'Doe',
-        8,
-        ['shirt_size' => 'M'],
-    );
+    ($this->creator)([
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 8,
+        'optional_data' => ['shirt_size' => 'M'],
+    ]);
     $student = Student::first();
     expect($student->school_id)->toEqual($school->id);
 });
 
 it('throws an error if we try to create a duplicate student (same name and school)', function () {
     $school = School::factory()->create();
-    ($this->creator)(
-        'John',
-        'Doe',
-        8,
-        ['shirt_size' => 'M'],
-        $school->id
-    );
-    ($this->creator)(
-        'John',
-        'Doe',
-        11,
-        ['shirt_size' => 'XL'],
-        $school->id
-    );
+    ($this->creator)([
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 8,
+        'optional_data' => ['shirt_size' => 'M'],
+        'school_id' => $school->id,
+    ]);
+    ($this->creator)([
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 11,
+        'optional_data' => ['shirt_size' => 'XL'],
+        'school_id' => $school->id,
+    ]);
 })->throws(AuditionAdminException::class, 'Student already exists');
+
+it('throws an error if data is missing', function () {
+    $school = School::factory()->create();
+    ($this->creator)([
+        'last_name' => 'Doe',
+        'grade' => 8,
+        'optional_data' => ['shirt_size' => 'M'],
+        'school_id' => $school->id,
+    ]);
+})->throws(AuditionAdminException::class, 'Missing required data');

tests/Feature/app/Actions/Students/UpdateStudentTest.php

@@ -0,0 +1,98 @@
+<?php
+
+/** @noinspection PhpUnhandledExceptionInspection */
+
+use App\Actions\Students\CreateStudent;
+use App\Actions\Students\UpdateStudent;
+use App\Exceptions\AuditionAdminException;
+use App\Models\School;
+use App\Models\Student;
+use App\Models\User;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+
+uses(RefreshDatabase::class);
+
+beforeEach(function () {
+    $this->creator = app(CreateStudent::class);
+    $this->editor = app(UpdateStudent::class);
+    $this->editedStudent = ($this->creator)([
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 8,
+        'school_id' => School::factory()->create()->id,
+    ]);
+});
+
+it('can update a student with basic data', function () {
+    ($this->editor)($this->editedStudent, [
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 14,
+        'school_id' => School::factory()->create()->id,
+    ]);
+    expect(Student::first()->grade)->toBe(14);
+});
+
+it('can include optional data', function () {
+    ($this->editor)($this->editedStudent, [
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 2,
+        'optional_data' => ['shirt_size' => 'M'],
+        'school_id' => School::factory()->create()->id,
+    ]);
+    $student = Student::first();
+    expect($student->optional_data['shirt_size'])->toEqual('M')
+        ->and($student->grade)->toBe(2);
+});
+
+it('uses the current users school if none is specified', function () {
+    $user = User::factory()->create();
+    $school = School::factory()->create();
+    $user->school_id = $school->id;
+    $user->save();
+    $this->actingAs($user);
+    ($this->editor)($this->editedStudent, [
+        'first_name' => 'Jane',
+        'last_name' => 'Doe',
+        'grade' => 8,
+        'optional_data' => ['shirt_size' => 'M'],
+    ]);
+    $student = Student::first();
+    expect($student->school_id)->toEqual($school->id)
+        ->and($student->first_name)->toEqual('Jane');
+});
+
+it('throws an error if we try to create a duplicate student (same name and school)', function () {
+    $school = School::factory()->create();
+    ($this->creator)([
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 8,
+        'optional_data' => ['shirt_size' => 'M'],
+        'school_id' => $school->id,
+    ]);
+    $student2 = ($this->creator)([
+        'first_name' => 'Jane',
+        'last_name' => 'Doe',
+        'grade' => 11,
+        'optional_data' => ['shirt_size' => 'XL'],
+        'school_id' => $school->id,
+    ]);
+    ($this->editor)($student2, [
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 11,
+        'school_id' => $school->id,
+    ]);
+})->throws(AuditionAdminException::class, 'Student already exists');
+
+it('throws an error if data is missing', function () {
+    $school = School::factory()->create();
+    ($this->editor)($this->editedStudent, [
+        'last_name' => 'Doe',
+        'grade' => 8,
+        'optional_data' => ['shirt_size' => 'M'],
+        'school_id' => $school->id,
+    ]);
+})->throws(AuditionAdminException::class, 'Missing required data');

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

@@ -8,6 +8,20 @@ use Illuminate\Foundation\Testing\RefreshDatabase;
 
 uses(RefreshDatabase::class);
 
+beforeEach(function () {
+    $this->school = School::factory()->create();
+    $this->submitData = [
+        'first_name' => 'John',
+        'last_name' => 'Doe',
+        'grade' => 8,
+    ];
+    $this->updateData = [
+        'first_name' => 'George',
+        'last_name' => 'Washington',
+        'grade' => 53,
+    ];
+});
+
 describe('Test the index method of the student controller', function () {
     it('redirects to the dashboard if the user does not have a school', function () {
         $user = User::factory()->create();
@@ -48,5 +62,146 @@ describe('Test the index method of the student controller', function () {
 });
 
 describe('Test the store method of the student controller', function () {
+    it('redirects to the dashboard if the user does not have a school', function () {
+        $user = User::factory()->create();
+        //$user->update(['school_id' => School::factory()->create()->id]);;
+        $this->actingAs($user);
+        $response = $this->post(route('students.store'), $this->submitData);
+        $response->assertStatus(403);
+    });
+
+    it('creates a student with only basic information', function () {
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $this->actingAs($user);
+        $response = $this->post(route('students.store'), $this->submitData);
+        $response->assertRedirect(route('students.index'));
+        expect(Student::first())->toBeInstanceOf(Student::class)
+            ->and(Student::first()->first_name)->toEqual('John')
+            ->and(Student::first()->last_name)->toEqual('Doe')
+            ->and(Student::first()->grade)->toEqual(8)
+            ->and(Student::first()->school_id)->toEqual($this->school->id);
+    });
+
+    it('creates a student with optional data', function () {
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $this->actingAs($user);
+        $localData = $this->submitData;
+        $localData['optional_data'] = ['shirt_size' => 'M'];
+        $response = $this->post(route('students.store'), $localData);
+        $response->assertRedirect(route('students.index'));
+        expect(Student::first()->optional_data['shirt_size'])->toEqual('M');
 
     });
+
+    it('will not allow a user to create a student for another school', function () {
+        $otherSchool = School::factory()->create();
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $this->actingAs($user);
+        $localData = $this->submitData;
+        $localData['school_id'] = $otherSchool->id;
+        $response = $this->post(route('students.store'), $localData);
+        expect(Student::first()->school_id)->toBe($this->school->id);
+    });
+});
+
+describe('Test the edit method of the student controller', function () {
+    it('will not allow a user to edit a student from another school', function () {
+        $otherStudent = Student::factory()->create();
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $this->actingAs($user);
+        $response = $this->get(route('students.edit', $otherStudent));
+        $response->assertStatus(403);
+    });
+
+    it('produces an edit for for a student', function () {
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $student = Student::factory()->forSchool($this->school)->create();
+        $this->actingAs($user);
+        $response = $this->get(route('students.edit', $student));
+        $response->assertViewIs('students.edit')
+            ->assertViewHas('student', $student)
+            ->assertSee(route('students.update', $student));
+    });
+});
+
+describe('Test the update method of the student controller', function () {
+    it('will not allow a user to edit a student from another school', function () {
+        $otherStudent = Student::factory()->create();
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $this->actingAs($user);
+        expect($otherStudent->school_id)->not->toEqual($this->school->id);
+        $response = $this->patch(route('students.update', $otherStudent), $this->submitData);
+        $response->assertStatus(403);
+    });
+
+    it('modifies a student with only basic information', function () {
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $this->actingAs($user);
+        $this->post(route('students.store'), $this->submitData);
+        $response = $this->patch(route('students.update', Student::first()), $this->updateData);
+
+        $response->assertRedirect(route('students.index'));
+        expect(Student::first())->toBeInstanceOf(Student::class)
+            ->and(Student::first()->first_name)->toEqual('George')
+            ->and(Student::first()->last_name)->toEqual('Washington')
+            ->and(Student::first()->grade)->toEqual(53)
+            ->and(Student::first()->school_id)->toEqual($this->school->id);
+    });
+
+    it('modifies a student with optional data', function () {
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $this->actingAs($user);
+        $localData = $this->submitData;
+        $localData['optional_data'] = ['shirt_size' => 'M'];
+        $this->post(route('students.store'), $localData);
+
+        $localUpdateData = $this->updateData;
+        $localUpdateData['optional_data'] = ['shirt_size' => 'XL'];
+
+        $response = $this->patch(route('students.update', Student::first()), $localUpdateData);
+
+        $response->assertRedirect(route('students.index'));
+
+        expect(Student::first()->optional_data['shirt_size'])->toEqual('XL');
+    });
+});
+
+describe('Test the destroy method of the student controller', function () {
+    it('will not allow a user to delete a student from another school', function () {
+        $otherStudent = Student::factory()->create();
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $this->actingAs($user);
+        expect($otherStudent->school_id)->not->toEqual($this->school->id);
+        $response = $this->delete(route('students.destroy', $otherStudent));
+        $response->assertStatus(403);
+    });
+
+    it('allows a user to delete their own students', function () {
+        $student = Student::factory()->forSchool($this->school)->create();
+        $user = User::factory()->create();
+        $user->school_id = $this->school->id;
+        $user->save();
+        $this->actingAs($user);
+        $response = $this->delete(route('students.destroy', $student));
+        $response->assertRedirect(route('students.index'));
+        expect(Student::first())->toBeNull();
+    });
+});