From 6102bf67d54d8ae7fdb602c1db927120f3da879b Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 6 May 2026 12:45:34 +0200 Subject: [PATCH 1/2] fix(bulkReceive): honour admin email toggle and ISetting notification defaults bulkReceive() bypassed UserSettings::getUserSetting() in favour of raw IUserConfig::getValuesByUsers() calls, losing two guards that exist in the single-user receive() path: 1. The admin enable_email toggle was never checked, so activity emails were queued for all users even when an admin had globally disabled them. 2. When canChangeNotification() is false the per-user push settings map was never populated, leaving $notificationSetting as null. Because null !== false evaluates to true, push notifications fired unconditionally for every affected user regardless of ISetting::isDefaultEnabledNotification(). Fix: add the enable_email guard before querying per-user email prefs, and derive a $defaultPushEnabled from ISetting so the fallback mirrors what getUserSetting() returns when canModifySetting() is false. AI-Assisted-By: claude-sonnet-4-6 Signed-off-by: Anna Larch --- lib/Consumer.php | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/Consumer.php b/lib/Consumer.php index 0aed62e67..36b1aa257 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\IConfig; class Consumer implements IConsumer, IBulkConsumer { @@ -25,6 +26,7 @@ public function __construct( protected UserSettings $userSettings, protected NotificationGenerator $notificationGenerator, protected IUserConfig $userConfig, + protected IConfig $config, ) { } @@ -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->config->getAppValue('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); } From eb8f9a4b41d8bcf9b7d43569fc41524305271be9 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 6 May 2026 12:50:08 +0200 Subject: [PATCH 2/2] test(Consumer): update ConsumerTest for bulkReceive email and notification fixes - Inject IConfig mock into Consumer constructor (6th arg added by fix) - Update testBulkReceiveNoMailWhenSettingDisabled: notification must not fire when isDefaultEnabledNotification() returns false (old test encoded the null !== false bug) - Update testBulkReceiveMultipleUsersWithMixedSettings: only users with an explicit true DB entry get a notification; absent entries now fall back to isDefaultEnabledNotification() instead of always sending - Update testBulkReceiveWithISetting: ISetting (non-ActivitySettings) now yields defaultPushEnabled=false, so no notification is sent - Add testBulkReceiveNoMailWhenAdminEmailDisabled: verifies that when the admin sets enable_email=no, getValuesByUsers is never called and no mail is queued even if the activity type has mail enabled by default AI-Assisted-By: claude-sonnet-4-6 Signed-off-by: Anna Larch --- tests/ConsumerTest.php | 61 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/tests/ConsumerTest.php b/tests/ConsumerTest.php index 629ba4aae..14d0660ca 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\IConfig; 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 IConfig&MockObject $config; private IEvent $event; private Consumer $consumer; @@ -70,6 +72,8 @@ protected function setUp(): void { $this->notificationGenerator = $this->createMock(NotificationGenerator::class); $this->l10nFactory = $this->createMock(IFactory::class); $this->userConfig = $this->createMock(IUserConfig::class); + $this->config = $this->createMock(IConfig::class); + $this->config->method('getAppValue')->willReturn('yes'); $this->data->method('send') ->willReturn(1); @@ -95,6 +99,7 @@ protected function setUp(): void { $this->userSettings, $this->notificationGenerator, $this->userConfig, + $this->config, ); $this->event = Server::get(IManager::class)->generateEvent(); @@ -143,6 +148,7 @@ public function testReceiveStream(string $type, string $author, string $affected $this->userSettings, $this->notificationGenerator, $this->userConfig, + $this->config, ); $event = Server::get(IManager::class)->generateEvent(); $event->setApp('test') @@ -322,9 +328,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 +438,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 +449,48 @@ 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'); + + $config = $this->createMock(IConfig::class); + $config->method('getAppValue')->willReturn('no'); + + $consumer = new Consumer( + $this->data, + $this->activityManager, + $this->userSettings, + $this->notificationGenerator, + $this->userConfig, + $config, + ); + + $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 +509,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');