Skip to content

Commit 19ee1c9

Browse files
committed
Notifications: Logged errors and prevented them blocking user
Failed notification sends could block the user action, whereas it's probably more important that the user action takes places uninteruupted than showing an error screen for the user to debug. Logs notification errors so issues can still be debugged by admins. Closes #5315
1 parent fcf0bf7 commit 19ee1c9

File tree

2 files changed

+31
-1
lines changed

2 files changed

+31
-1
lines changed

app/Activity/Notifications/Handlers/BaseNotificationHandler.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use BookStack\Entities\Models\Entity;
88
use BookStack\Permissions\PermissionApplicator;
99
use BookStack\Users\Models\User;
10+
use Illuminate\Support\Facades\Log;
1011

1112
abstract class BaseNotificationHandler implements NotificationHandler
1213
{
@@ -36,7 +37,11 @@ protected function sendNotificationToUserIds(string $notification, array $userId
3637
}
3738

3839
// Send the notification
39-
$user->notify(new $notification($detail, $initiator));
40+
try {
41+
$user->notify(new $notification($detail, $initiator));
42+
} catch (\Exception $exception) {
43+
Log::error("Failed to send email notification to user [id:{$user->id}] with error: {$exception->getMessage()}");
44+
}
4045
}
4146
}
4247
}

tests/Activity/WatchTest.php

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
use BookStack\Activity\WatchLevels;
1414
use BookStack\Entities\Models\Entity;
1515
use BookStack\Settings\UserNotificationPreferences;
16+
use Illuminate\Contracts\Notifications\Dispatcher;
17+
use Illuminate\Support\Facades\Mail;
1618
use Illuminate\Support\Facades\Notification;
1719
use Tests\TestCase;
1820

@@ -365,6 +367,29 @@ public function test_notifications_sent_in_right_language()
365367
}
366368
}
367369

370+
public function test_failed_notifications_dont_block_and_log_errors()
371+
{
372+
$logger = $this->withTestLogger();
373+
$editor = $this->users->editor();
374+
$admin = $this->users->admin();
375+
$page = $this->entities->page();
376+
$book = $page->book;
377+
$activityLogger = app()->make(ActivityLogger::class);
378+
379+
$watches = new UserEntityWatchOptions($editor, $book);
380+
$watches->updateLevelByValue(WatchLevels::UPDATES);
381+
382+
$mockDispatcher = $this->mock(Dispatcher::class);
383+
$mockDispatcher->shouldReceive('send')->once()
384+
->andThrow(\Exception::class, 'Failed to connect to mail server');
385+
386+
$this->actingAs($admin);
387+
388+
$activityLogger->add(ActivityType::PAGE_UPDATE, $page);
389+
390+
$this->assertTrue($logger->hasErrorThatContains("Failed to send email notification to user [id:{$editor->id}] with error: Failed to connect to mail server"));
391+
}
392+
368393
public function test_notifications_not_sent_if_lacking_view_permission_for_related_item()
369394
{
370395
$notifications = Notification::fake();

0 commit comments

Comments
 (0)