fix test issues

2026-07-06 02:39:13 -04:00
parent 90f9857b06
commit 304fa6bfd0
14 changed files with 1018 additions and 17 deletions
@@ -0,0 +1,836 @@
+# PHPUnit Failure Fix Summary
+
+## Overview
+
+The PHPUnit run failed with a large number of cascading failures, but the issues are concentrated in a few root causes.
+
+Result summary:
+
+- **679 tests passed**
+- **352 tests failed**
+- **2 tests risky**
+- Most failures are caused by authorization changes, missing routes, schema drift, stale test setup, and inconsistent API contracts.
+
+Do **not** fix the 352 failures one by one. Fix the root causes below in order.
+
+---
+
+## 1. Fix Authorization and Permission Test Setup
+
+### Problem
+
+Many tests now fail with:
+
+- `403 Forbidden`
+- `401 Unauthorized`
+
+Most protected routes are correctly blocking users because the tests authenticate users without assigning required roles or permissions.
+
+Example bad pattern:
+
+```php
+$user = User::factory()->create();
+Sanctum::actingAs($user);
+```
+
+This creates an authenticated user, but not an authorized admin.
+
+### Required Fix
+
+Update test helpers to create proper users with roles and permissions.
+
+Recommended helpers:
+
+```php
+$this->actingAsApiAdministrator();
+$this->actingAsApiUserWithPermissions(['permission.name']);
+$this->actingAsParent();
+$this->actingAsTeacher();
+```
+
+Then update affected feature tests to use the correct helper.
+
+### Priority
+
+**Critical**
+
+This is the biggest failure cluster and must be fixed before chasing controller-level test failures.
+
+---
+
+## 2. Protect School Year Admin Routes Correctly
+
+### Problem
+
+The test expects non-admin users to be rejected from:
+
+```http
+GET /api/v1/school-years
+```
+
+But the route currently returns `200` for a non-admin user.
+
+### Required Fix
+
+Separate public/current school-year routes from admin-only routes.
+
+Recommended access model:
+
+```text
+GET /api/v1/school-years              -> admin only
+GET /api/v1/school-years/current      -> authenticated users
+GET /api/v1/school-years/options      -> authenticated users if frontend needs it
+```
+
+Use a stricter permission for the index route, such as:
+
+```text
+school_year.manage
+school_year.admin.view
+```
+
+Do not let parent users list all school years unless that is intentional.
+
+### Priority
+
+**Critical**
+
+---
+
+## 3. Add Missing `SchoolYearController@archive`
+
+### Problem
+
+The route exists:
+
+```http
+POST /api/v1/school-years/{schoolYear}/archive
+```
+
+But the controller method does not exist.
+
+### Required Fix
+
+Add the method to:
+
+```text
+app/Http/Controllers/Api/SchoolYears/SchoolYearController.php
+```
+
+Required method:
+
+```php
+public function archive(int $schoolYear): JsonResponse
+```
+
+Then either:
+
+- implement real archive behavior, or
+- remove the route if archiving is no longer supported.
+
+### Priority
+
+**Critical**
+
+---
+
+## 4. Update API Route Coverage Matrix
+
+### Problem
+
+The route coverage tests report missing mapping for:
+
+```http
+GET /api/v1/admin/progress
+GET /api/v1/admin/progress/meta
+```
+
+### Required Fix
+
+Add both routes to the API use-case coverage matrix.
+
+Assign them to the correct domain, likely:
+
+```text
+Academic Progress
+Administrator Reporting
+```
+
+Also update E2E workflow coverage if required by the test suite.
+
+### Priority
+
+**High**
+
+---
+
+## 5. Fix Database Schema Drift
+
+### Problem
+
+Several tests fail because models/services expect columns that migrations do not provide, or migrations require fields that services/tests do not send.
+
+### Required Fixes
+
+#### `email_templates`
+
+Error:
+
+```text
+table email_templates has no column named created_at
+```
+
+Fix:
+
+```php
+$table->timestamps();
+```
+
+Or set `$timestamps = false` on the model if timestamps are not intended.
+
+Preferred fix: **add timestamps**.
+
+---
+
+#### `class_prep_adjustments`
+
+Error:
+
+```text
+table class_prep_adjustments has no column named updated_at
+```
+
+Fix:
+
+```php
+$table->timestamps();
+```
+
+---
+
+#### `late_slip_logs`
+
+Error:
+
+```text
+NOT NULL constraint failed: late_slip_logs.printed_at
+```
+
+Fix one:
+
+```php
+$table->timestamp('printed_at')->nullable();
+```
+
+or ensure every insert provides `printed_at`.
+
+Preferred fix: **make nullable**.
+
+---
+
+#### `additional_charges`
+
+Error:
+
+```text
+NOT NULL constraint failed: additional_charges.title
+```
+
+Fix one:
+
+- provide `title` in every creation path
+- or make `title` nullable/defaulted
+
+Preferred fix: **update service/test payloads to always provide title** unless legacy data requires nullable.
+
+---
+
+#### `payments`
+
+Error:
+
+```text
+NOT NULL constraint failed: payments.number_of_installments
+```
+
+Fix:
+
+```php
+$table->unsignedInteger('number_of_installments')->default(1);
+```
+
+Also ensure payment creation services pass a value when appropriate.
+
+---
+
+#### `ip_attempts`
+
+Error:
+
+```text
+NOT NULL constraint failed: ip_attempts.last_attempt_at
+```
+
+Fix:
+
+```php
+$table->timestamp('last_attempt_at')->useCurrent();
+```
+
+or update all insert paths.
+
+---
+
+#### `student_class`
+
+Error:
+
+```text
+table student_class has no column named school_id
+```
+
+Fix one:
+
+- add `school_id` to the pivot table
+- or remove references to `school_id`
+
+Preferred fix: **add `school_id`** if school isolation is required.
+
+---
+
+#### `semester_scores`
+
+Error:
+
+```text
+NOT NULL constraint failed: semester_scores.school_id
+```
+
+Fix:
+
+- provide `school_id` in all score creation/update paths
+- or allow nullable for legacy records
+
+Preferred fix: **provide `school_id` consistently**.
+
+### Priority
+
+**High**
+
+---
+
+## 6. Stabilize SQLite Test Database
+
+### Problem
+
+The log contains repeated:
+
+```text
+SQLSTATE[HY000]: General error: 5 database is locked
+```
+
+### Required Fix
+
+Do not share one SQLite file across many concurrent or transaction-heavy tests.
+
+Recommended CI setup:
+
+```env
+DB_CONNECTION=sqlite
+DB_DATABASE=/tmp/alrahma_testing.sqlite
+```
+
+Before tests:
+
+```bash
+rm -f /tmp/alrahma_testing.sqlite
+touch /tmp/alrahma_testing.sqlite
+php artisan migrate:fresh --env=testing
+```
+
+Also check:
+
+- no parallel tests share the same SQLite file
+- long transactions are not left open
+- `RefreshDatabase` is used consistently
+
+### Priority
+
+**High**
+
+---
+
+## 7. Update Unit Tests for Service Constructor Changes
+
+### Problem
+
+Many unit tests manually instantiate services with outdated constructor arguments.
+
+Examples:
+
+```text
+AdministratorAbsenceService expects 6 arguments, test passes 5
+TeacherSubmissionNotificationService expects 3 arguments, test passes 2
+ClassProgressMutationService expects 4 arguments, test passes 2
+DiscountInvoiceService expects 1 argument, test passes 0
+```
+
+### Required Fix
+
+Replace manual construction:
+
+```php
+$service = new SomeService($oldArg1, $oldArg2);
+```
+
+with container resolution:
+
+```php
+$service = app(SomeService::class);
+```
+
+If mocks are needed, bind them into the container before resolving the service.
+
+### Priority
+
+**High**
+
+---
+
+## 8. Fix Final Class Mocking
+
+### Problem
+
+This class cannot be mocked:
+
+```text
+App\Services\ApplicationUrlService
+```
+
+because it is declared `final`.
+
+### Required Fix
+
+Choose one:
+
+1. Remove `final`
+2. Extract an interface, for example:
+
+```php
+interface ApplicationUrlProvider
+```
+
+3. Use a real instance in tests instead of mocking it
+
+Preferred fix: **extract an interface** if the service should remain final.
+
+### Priority
+
+**Medium**
+
+---
+
+## 9. Standardize API Response Shape
+
+### Problem
+
+Some tests expect one response shape while controllers return another.
+
+Examples of inconsistent shapes:
+
+```json
+{
+  "ok": true
+}
+```
+
+```json
+{
+  "status": true,
+  "message": "...",
+  "data": {}
+}
+```
+
+Raw arrays are also returned in some places.
+
+### Required Fix
+
+Standardize API v1 responses.
+
+Recommended shape:
+
+```json
+{
+  "status": true,
+  "message": "Success",
+  "data": {}
+}
+```
+
+Then update:
+
+- controllers
+- API resources
+- frontend API clients
+- tests
+
+### Priority
+
+**Medium**
+
+---
+
+## 10. Fix Validation Contract Drift
+
+### Problem
+
+Several controllers now require fields that old tests/frontend callers do not send.
+
+### Required Fixes
+
+#### Attendance comment templates
+
+Current required fields:
+
+```text
+min_score
+max_score
+template_text
+```
+
+Old callers send:
+
+```text
+comment
+type
+is_active
+```
+
+Fix:
+
+- support legacy aliases, or
+- update callers/tests to the new payload
+
+Preferred fix: **support aliases temporarily and migrate frontend/tests**.
+
+---
+
+#### Score comments
+
+Current required field:
+
+```text
+comments
+```
+
+Old callers send:
+
+```text
+comment
+```
+
+Fix:
+
+```php
+$comments = $request->input('comments', $request->input('comment'));
+```
+
+---
+
+#### Below-60 grading
+
+Current required field:
+
+```text
+semester
+```
+
+Some callers only send:
+
+```text
+school_year
+```
+
+Fix one:
+
+- make `semester` optional and infer current semester
+- or update callers/tests to always send `semester`
+
+Preferred fix: **frontend/tests should send semester explicitly**.
+
+---
+
+#### Print request status
+
+Current validation rejects:
+
+```text
+processing
+```
+
+Fix one:
+
+- add `processing` to allowed statuses
+- or update callers/tests to use the valid status value
+
+Preferred fix: **allow `processing`** if it exists in the real workflow.
+
+---
+
+#### Role permissions
+
+Current required field:
+
+```text
+permissions
+```
+
+Old callers send:
+
+```text
+permission_ids
+```
+
+Fix:
+
+- accept `permission_ids` as alias
+- or update frontend/tests
+
+Preferred fix: **accept both**.
+
+### Priority
+
+**Medium**
+
+---
+
+## 11. Add Missing Legacy Route Aliases
+
+### Problem
+
+Some tests/frontend paths call legacy endpoints that now return `404`.
+
+Examples:
+
+```http
+GET /api/v1/configuration-admin
+POST /api/v1/configuration-admin
+GET /api/v1/attendance-tracking/compose
+```
+
+### Required Fix
+
+Either:
+
+- add compatibility aliases to the current controllers, or
+- update frontend/tests to the new endpoints
+
+Recommended short-term fix:
+
+```php
+Route::get('/configuration-admin', ...);
+Route::post('/configuration-admin', ...);
+Route::get('/attendance-tracking/compose', ...);
+```
+
+Long-term fix:
+
+- remove legacy aliases only after frontend and tests fully migrate.
+
+### Priority
+
+**Medium**
+
+---
+
+## 12. Normalize Validation Status Codes
+
+### Problem
+
+Some validation failures return:
+
+```http
+400 Bad Request
+```
+
+when tests expect:
+
+```http
+422 Unprocessable Entity
+```
+
+### Required Fix
+
+Use Laravel validation responses consistently.
+
+Expected status rules:
+
+```text
+400 -> malformed request
+401 -> unauthenticated
+403 -> authenticated but forbidden
+404 -> missing resource
+422 -> validation error
+```
+
+### Priority
+
+**Medium**
+
+---
+
+## 13. Review Enrollment Bulk Status Endpoint
+
+### Problem
+
+Endpoint returns:
+
+```http
+207 Multi-Status
+```
+
+where tests expect:
+
+```http
+200 OK
+```
+
+Affected endpoint:
+
+```http
+/api/v1/administrator/enrollment-withdrawal/update-statuses
+```
+
+### Required Fix
+
+Decide the correct contract:
+
+- return `207` only for partial success/failure
+- return `200` if the operation is all-or-nothing
+
+Then update either the controller or the tests.
+
+### Priority
+
+**Low to Medium**
+
+---
+
+## 14. Fix Bulk User Creation Count
+
+### Problem
+
+Bulk user creation expects:
+
+```text
+3000 users
+```
+
+but creates:
+
+```text
+3001 users
+```
+
+### Required Fix
+
+Find the extra user:
+
+```php
+User::where('email', 'like', 'bulk_api_%')
+    ->pluck('email')
+    ->countBy();
+```
+
+Check whether the extra user comes from:
+
+- test setup
+- authenticated admin fixture
+- duplicate creation retry
+- linked/secondary user creation
+- email prefix overlap
+
+### Priority
+
+**Low**
+
+---
+
+## 15. Fix Public Token / Time-Off Route Tests
+
+### Problem
+
+Invalid token route returns a `400` HTML response.
+
+### Required Fix
+
+Review the public time-off token contract:
+
+- expected token format
+- expiry behavior
+- invalid token response
+- whether response should be HTML, JSON, redirect, `400`, or `404`
+
+Then update controller/tests to match the intended behavior.
+
+### Priority
+
+**Low**
+
+---
+
+## 16. Remove Insecure TLS Setting from CI
+
+### Problem
+
+The log warns:
+
+```text
+NODE_TLS_REJECT_UNAUTHORIZED=0 makes TLS connections insecure
+```
+
+### Required Fix
+
+Remove this setting from CI.
+
+If the runner needs to trust an internal/self-signed certificate, install the internal CA certificate properly instead.
+
+### Priority
+
+**Low but important for security**
+
+---
+
+# Recommended Fix Order
+
+Fix in this order:
+
+1. Authorization test helpers and permission contracts
+2. School-year admin route protection
+3. Missing `SchoolYearController@archive`
+4. Route coverage matrix for admin progress routes
+5. Database schema drift
+6. SQLite test database locking
+7. Service constructor test updates
+8. Final class mocking issue
+9. API response shape consistency
+10. Validation payload aliases and status codes
+11. Missing legacy route aliases
+12. Remaining workflow-specific failures
+
+---
+
+# Main Root Causes
+
+The test suite is failing because of these root causes:
+
+1. **Authorization hardening changed route behavior**
+2. **Tests still create users without proper roles/permissions**
+3. **Backend routes and frontend/test expectations drifted apart**
+4. **Migrations and models are no longer aligned**
+5. **Service constructors changed but unit tests were not updated**
+6. **API response and validation contracts are inconsistent**
+7. **SQLite test database setup is unstable**
+
+Fixing those root causes should collapse most of the 352 failures without manually touching every failing test.