diff --git a/app/Services/AdminNotificationService.php b/app/Services/AdminNotificationService.php index 505ce1f..8a8885b 100644 --- a/app/Services/AdminNotificationService.php +++ b/app/Services/AdminNotificationService.php @@ -23,7 +23,7 @@ class AdminNotificationService } $errorKey = $this->getErrorKey($exception); - $throttleMinutes = config('libra.notifications.throttle_minutes', 15); + $throttleMinutes = (int) config('libra.notifications.throttle_minutes', 15); if (Cache::has($errorKey)) { return; diff --git a/docs/tasks/23.md b/docs/tasks/23.md new file mode 100644 index 0000000..a23e490 --- /dev/null +++ b/docs/tasks/23.md @@ -0,0 +1,93 @@ +--- +id: 23 +title: 23 Fix AdminNotificationService Carbon TypeError Masking Exceptions +status: Done +date: 2026-09-28 +parent: null +--- + +## Description + +Investigating a client report of a missing workshop-registration email led to discovering a real +bug: `App\Services\AdminNotificationService::notifyError()` throws its own `TypeError` whenever +it runs, which **masks the original exception** it was trying to report and prevents the admin +notification from ever being sent. Root cause: `config('libra.notifications.throttle_minutes', 15)` +resolves to `env('ERROR_NOTIFICATION_THROTTLE_MINUTES', 15)` (see `config/libra.php` line 14). +Production's `.env` has `ERROR_NOTIFICATION_THROTTLE_MINUTES=15` set — Laravel's `env()` helper +does not cast a numeric-looking string to an int, so this returns the **string** `"15"`, not the +int `15`. That string is then passed to `now()->addMinutes($throttleMinutes)` in +`AdminNotificationService::notifyError()` (line 32), and Carbon 3.11 (this project's installed +version, per `composer.lock`) requires `addMinutes()`'s argument to be `int|float` — passing a +string throws `TypeError: Carbon\Carbon::rawAddUnit(): Argument #3 ($value) must be of type +int|float, string given`. + +This is a live production bug, confirmed via `storage/logs/laravel.log` on h1 +(`/home/libra/public_html/storage/logs/laravel.log`, entries timestamped `2026-09-27 21:35:26` +and `21:40:31`) — every exception that reaches `Handler->report()` and is judged +notify-worthy by `AdminNotificationService::shouldNotifyAdmin()` (e.g. any +`Symfony\Component\Mailer\Exception\TransportExceptionInterface`, or a critical +`QueryException`) currently crashes this method instead of sending the notification, and the +crash happens **before** `Cache::put()` on line 32 ever executes, so the throttle-dedup cache key +is never written — meaning this crash also can't self-heal or throttle, it fires on every single +qualifying exception, indefinitely, until fixed. + +**File to modify:** `app/Services/AdminNotificationService.php` + +Current (lines 25-32): +```php + $errorKey = $this->getErrorKey($exception); + $throttleMinutes = config('libra.notifications.throttle_minutes', 15); + + if (Cache::has($errorKey)) { + return; + } + + Cache::put($errorKey, true, now()->addMinutes($throttleMinutes)); +``` + +Change to cast the config value to an int before use: +```php + $errorKey = $this->getErrorKey($exception); + $throttleMinutes = (int) config('libra.notifications.throttle_minutes', 15); + + if (Cache::has($errorKey)) { + return; + } + + Cache::put($errorKey, true, now()->addMinutes($throttleMinutes)); +``` + +This is the only line that changes. No other method, no other file. + +## Acceptance Criteria + +- [x] `app/Services/AdminNotificationService.php`'s `$throttleMinutes` assignment casts the config + value with `(int)` +- [x] A test constructs `AdminNotificationService` and calls `notifyError()` with a throwable that + makes `shouldNotifyAdmin()` return true (e.g. a mock/stub implementing + `Symfony\Component\Mailer\Exception\TransportExceptionInterface`, or simply an instance of a + class implementing `App\Contracts\ShouldNotifyAdmin`), with + `config(['libra.notifications.throttle_minutes' => '15'])` explicitly set as a **string** in + the test (reproducing production's actual condition — `env()` never returns a string this + way in tests, so it must be set explicitly to catch this class of bug) — the call completes + without throwing, and a mail notification is sent (assert via `Notification::fake()` / + `Mail::fake()` as appropriate for however `SystemErrorNotification` delivers) +- [x] The same test, run against the **old** code (temporarily, to confirm the test actually + catches the bug) throws a `TypeError` — state in the completion report that this was + confirmed, per this task's RED step + +## QA Verification (2026-09-28) + +All 3 acceptance criteria verified directly: +1. Confirmed `(int)` cast present at `app/Services/AdminNotificationService.php:26`. +2. Ran `tests/Unit/Services/AdminNotificationServiceTest.php`'s new test + (`does not throw and still sends notification when throttle_minutes config is a string`) — + passed. +3. Temporarily removed the `(int)` cast, re-ran the same test — reproduced the exact TypeError + (`Carbon\Carbon::rawAddUnit(): Argument #3 ($value) must be of type int|float, string given`), + confirming RED. Restored the `(int)` cast. Full suite re-run: 17/17 passed. `git diff` confirms + only the single intended line differs from the pre-fix version. + +## Sub-tasks + +None. diff --git a/docs/tasks/statuses.md b/docs/tasks/statuses.md index a7d2f2a..3e24cf0 100644 --- a/docs/tasks/statuses.md +++ b/docs/tasks/statuses.md @@ -134,3 +134,4 @@ | 20 | 20 Nablus Workshop Form - Match Site Background and Card Styling | Done | | 21 | 21 Nablus Workshop Form - Flyer Size Adjustment | Done | | 22 | 22 Fix Footer Address - Add Missing Street Name | Done | +| 23 | 23 Fix AdminNotificationService Carbon TypeError Masking Exceptions | Done | diff --git a/tests/Unit/Services/AdminNotificationServiceTest.php b/tests/Unit/Services/AdminNotificationServiceTest.php index 209f4ee..44c2a0e 100644 --- a/tests/Unit/Services/AdminNotificationServiceTest.php +++ b/tests/Unit/Services/AdminNotificationServiceTest.php @@ -184,4 +184,27 @@ describe('notifyError', function () { Notification::assertSentOnDemandTimes(SystemErrorNotification::class, 2); }); + + it('does not throw and still sends notification when throttle_minutes config is a string', function () { + // Reproduces a live production bug: env('ERROR_NOTIFICATION_THROTTLE_MINUTES', 15) returns + // the string "15" (env() never casts numeric-looking strings to int), which used to be + // passed straight into now()->addMinutes(), and Carbon 3.11 throws a TypeError for a + // string argument there. That TypeError masked the original exception being reported and + // notifyError() never actually sent the admin notification. + Notification::fake(); + Cache::flush(); + config(['libra.notifications.throttle_minutes' => '15']); + + $service = new AdminNotificationService; + $exception = new TransportException('SMTP error'); + + $service->notifyError($exception); + + Notification::assertSentOnDemand( + SystemErrorNotification::class, + function ($notification, $channels, $notifiable) { + return $notifiable->routes['mail'] === 'admin@test.com'; + } + ); + }); });