From 2962854541ae467d9f72f093757e670fccc202b6 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Fri, 4 Jul 2025 23:52:25 -0500 Subject: [PATCH] Student controller refactoring and testing. --- app/Actions/Students/CreateStudent.php | 43 ++--- app/Actions/Students/UpdateStudent.php | 48 ++++++ app/Http/Controllers/StudentController.php | 80 ++------- app/Http/Requests/StudentStoreRequest.php | 8 + app/Observers/StudentObserver.php | 41 +++-- routes/user.php | 3 +- .../Actions/Students/CreateStudentTest.php | 77 +++++---- .../Actions/Students/UpdateStudentTest.php | 98 +++++++++++ .../Controllers/StudentControllerTest.php | 155 ++++++++++++++++++ 9 files changed, 412 insertions(+), 141 deletions(-) create mode 100644 app/Actions/Students/UpdateStudent.php create mode 100644 tests/Feature/app/Actions/Students/UpdateStudentTest.php diff --git a/app/Actions/Students/CreateStudent.php b/app/Actions/Students/CreateStudent.php index 0233900..c5e49d8 100644 --- a/app/Actions/Students/CreateStudent.php +++ b/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; } } diff --git a/app/Actions/Students/UpdateStudent.php b/app/Actions/Students/UpdateStudent.php new file mode 100644 index 0000000..b3fef27 --- /dev/null +++ b/app/Actions/Students/UpdateStudent.php @@ -0,0 +1,48 @@ +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, + ]); + } +} diff --git a/app/Http/Controllers/StudentController.php b/app/Http/Controllers/StudentController.php index 25b4a37..9278354 100644 --- a/app/Http/Controllers/StudentController.php +++ b/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.'
Name: '.$student->full_name().'
Grade: '.$student->grade.'
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.'
Name: '.$student->full_name().'
Grade: '.$student->grade.'
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')); diff --git a/app/Http/Requests/StudentStoreRequest.php b/app/Http/Requests/StudentStoreRequest.php index ab6dfff..874e86c 100644 --- a/app/Http/Requests/StudentStoreRequest.php +++ b/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; diff --git a/app/Observers/StudentObserver.php b/app/Observers/StudentObserver.php index a5714c3..b924547 100644 --- a/app/Observers/StudentObserver.php +++ b/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().'
Grade: '.$student->grade.'
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().'
Grade: '.$student->grade.'
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], + ], + ]); } } diff --git a/routes/user.php b/routes/user.php index cdfcfcd..37db0b8 100644 --- a/routes/user.php +++ b/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'); diff --git a/tests/Feature/app/Actions/Students/CreateStudentTest.php b/tests/Feature/app/Actions/Students/CreateStudentTest.php index 58ee283..6d4601c 100644 --- a/tests/Feature/app/Actions/Students/CreateStudentTest.php +++ b/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'); diff --git a/tests/Feature/app/Actions/Students/UpdateStudentTest.php b/tests/Feature/app/Actions/Students/UpdateStudentTest.php new file mode 100644 index 0000000..2c2f3e8 --- /dev/null +++ b/tests/Feature/app/Actions/Students/UpdateStudentTest.php @@ -0,0 +1,98 @@ +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'); diff --git a/tests/Feature/app/Http/Controllers/StudentControllerTest.php b/tests/Feature/app/Http/Controllers/StudentControllerTest.php index a925eb9..69bcb56 100644 --- a/tests/Feature/app/Http/Controllers/StudentControllerTest.php +++ b/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(); + }); });