From 4e3129f4822a8e548d112855215abf63cf8f9b9f Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 19 May 2026 19:01:03 +0200 Subject: [PATCH] fix(bulkReceive): honour admin email toggle and ISetting notification defaults The admin Enable notification emails toggle was not checked in bulkReceive(), so emails could be queued even when the toggle was disabled. Also, ISetting-based settings that do not implement ActivitySettings had canChangeNotification() called on them, which caused a fatal error; the default push-enabled state was also ignored. Injects IAppConfig and uses getValueString() to check the toggle, consistent with the approach used elsewhere. ISetting instances that are not ActivitySettings now fall back to the setting default via isDefaultEnabledNotification() without calling canChangeNotification(). Signed-off-by: Anna Larch --- lib/Consumer.php | 20 +++++++++---- tests/ConsumerTest.php | 66 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/lib/Consumer.php b/lib/Consumer.php index 0aed62e67..0125049dc 100644 --- a/lib/Consumer.php +++ b/lib/Consumer.php @@ -16,6 +16,7 @@ use OCP\Config\IUserConfig; use OCP\Config\ValueType; use OCP\DB\Exception; +use OCP\IAppConfig; class Consumer implements IConsumer, IBulkConsumer { @@ -25,6 +26,7 @@ public function __construct( protected UserSettings $userSettings, protected NotificationGenerator $notificationGenerator, protected IUserConfig $userConfig, + protected IAppConfig $appConfig, ) { } @@ -78,13 +80,16 @@ public function bulkReceive(IEvent $event, array $affectedUserIds, ISetting $set $canChangeMail = $setting->canChangeMail(); $canChangePush = $setting instanceof ActivitySettings && $setting->canChangeNotification() === true; + $defaultPushEnabled = $setting instanceof ActivitySettings && $setting->isDefaultEnabledNotification(); - $userPushSettings = $userEmailSettings = $batchTimeSettings = null; + $userPushSettings = null; if ($canChangePush === true) { $userPushSettings = $this->userConfig->getValuesByUsers('activity', 'notify_notification_' . $event->getType(), ValueType::BOOL, $affectedUserIds); } - if ($canChangeMail === true || $setting->isDefaultEnabledMail() === true) { + $userEmailSettings = $batchTimeSettings = null; + $emailEnabled = $this->appConfig->getValueString('activity', 'enable_email', 'yes') !== 'no'; + if ($emailEnabled && ($canChangeMail === true || $setting->isDefaultEnabledMail() === true)) { $userEmailSettings = $this->userConfig->getValuesByUsers('activity', 'notify_email_' . $event->getType(), ValueType::BOOL, $affectedUserIds); $batchTimeSettings = $this->userConfig->getValuesByUsers('activity', 'notify_setting_batchtime', ValueType::INT, $affectedUserIds); } @@ -95,14 +100,17 @@ public function bulkReceive(IEvent $event, array $affectedUserIds, ISetting $set continue; } $event->setAffectedUser($affectedUser); - $notificationSetting = $userPushSettings[$affectedUser] ?? null; - if ($notificationSetting !== null) { - $notificationSetting = (bool)$notificationSetting; + + if ($canChangePush === true) { + $notificationSetting = isset($userPushSettings[$affectedUser]) ? (bool)$userPushSettings[$affectedUser] : $defaultPushEnabled; + } else { + $notificationSetting = $defaultPushEnabled; } + $emailSetting = $userEmailSettings[$affectedUser] ?? false; $emailSetting = ($emailSetting) ? ($batchTimeSettings[$affectedUser] ?? false) : false; - if ($notificationSetting !== false) { + if ($notificationSetting === true) { $this->notificationGenerator->sendNotificationForEvent($event, $activityId, $notificationSetting); } diff --git a/tests/ConsumerTest.php b/tests/ConsumerTest.php index 629ba4aae..569d5bdb7 100644 --- a/tests/ConsumerTest.php +++ b/tests/ConsumerTest.php @@ -33,6 +33,7 @@ use OCP\Activity\IManager; use OCP\Activity\ISetting; use OCP\Config\IUserConfig; +use OCP\IAppConfig; use OCP\IDBConnection; use OCP\IL10N; use OCP\L10N\IFactory; @@ -53,6 +54,7 @@ class ConsumerTest extends TestCase { protected NotificationGenerator&MockObject $notificationGenerator; protected UserSettings $userSettings; private IUserConfig&MockObject $userConfig; + private IAppConfig&MockObject $appConfig; private IEvent $event; private Consumer $consumer; @@ -70,7 +72,10 @@ protected function setUp(): void { $this->notificationGenerator = $this->createMock(NotificationGenerator::class); $this->l10nFactory = $this->createMock(IFactory::class); $this->userConfig = $this->createMock(IUserConfig::class); - + $this->appConfig = $this->createMock(IAppConfig::class); + $this->appConfig->method('getValueString') + ->with('activity', 'enable_email', 'yes') + ->willReturn('yes'); $this->data->method('send') ->willReturn(1); $this->l10nFactory @@ -95,6 +100,7 @@ protected function setUp(): void { $this->userSettings, $this->notificationGenerator, $this->userConfig, + $this->appConfig, ); $this->event = Server::get(IManager::class)->generateEvent(); @@ -143,6 +149,7 @@ public function testReceiveStream(string $type, string $author, string $affected $this->userSettings, $this->notificationGenerator, $this->userConfig, + $this->appConfig, ); $event = Server::get(IManager::class)->generateEvent(); $event->setApp('test') @@ -322,9 +329,7 @@ public function testBulkReceiveNoMailWhenSettingDisabled(): void { $this->data->expects($this->never()) ->method('storeMail'); - // Notification is still sent because $notificationSetting defaults to null - // and null !== false, so the default is to send notifications - $this->notificationGenerator->expects($this->once()) + $this->notificationGenerator->expects($this->never()) ->method('sendNotificationForEvent'); $this->consumer->bulkReceive($this->event, ['affectedUser'], $settings); @@ -434,8 +439,8 @@ public function testBulkReceiveMultipleUsersWithMixedSettings(): void { return []; }); - // user1 and user2 get notifications (user2 has null setting which defaults to send), author is skipped - $this->notificationGenerator->expects($this->exactly(2)) + // only user1 has an explicit notification=true in DB; user2 has no entry so falls back to isDefaultEnabledNotification()=false + $this->notificationGenerator->expects($this->once()) ->method('sendNotificationForEvent'); // user2 gets email, author is skipped $this->data->expects($this->once()) @@ -445,6 +450,50 @@ public function testBulkReceiveMultipleUsersWithMixedSettings(): void { $this->consumer->bulkReceive($this->event, ['user1', 'user2', 'author'], $settings); } + public function testBulkReceiveNoMailWhenAdminEmailDisabled(): void { + $time = time(); + $this->event->setApp('activity') + ->setType('type') + ->setAuthor('author') + ->setTimestamp($time) + ->setSubject('subject', ['subjectParam1']) + ->setMessage('message', ['messageParam1']) + ->setObject('', 0, 'file') + ->setLink('link'); + + $appConfig = $this->createMock(IAppConfig::class); + $appConfig->method('getValueString') + ->with('activity', 'enable_email', 'yes') + ->willReturn('no'); + + $consumer = new Consumer( + $this->data, + $this->activityManager, + $this->userSettings, + $this->notificationGenerator, + $this->userConfig, + $appConfig, + ); + + $settings = $this->createMock(ActivitySettings::class); + $settings->method('canChangeMail')->willReturn(true); + $settings->method('isDefaultEnabledMail')->willReturn(true); + $settings->method('canChangeNotification')->willReturn(false); + $settings->method('isDefaultEnabledNotification')->willReturn(false); + + $this->data->expects($this->once()) + ->method('bulkSend') + ->willReturn([1 => 'affectedUser']); + + $this->userConfig->expects($this->never()) + ->method('getValuesByUsers'); + + $this->data->expects($this->never()) + ->method('storeMail'); + + $consumer->bulkReceive($this->event, ['affectedUser'], $settings); + } + public function testBulkReceiveWithISetting(): void { $this->event->setApp('activity') ->setType('type') @@ -463,9 +512,8 @@ public function testBulkReceiveWithISetting(): void { ->method('bulkSend') ->willReturn([1 => 'affectedUser']); - // Notification is still sent because $notificationSetting defaults to null (not false) - // when ISetting is used (canChangeNotification not available), and null !== false - $this->notificationGenerator->expects($this->once()) + // ISetting is not ActivitySettings so $defaultPushEnabled is false — no notification sent + $this->notificationGenerator->expects($this->never()) ->method('sendNotificationForEvent'); $this->data->expects($this->never()) ->method('storeMail');