From eeeef6717e6063e381060e34aa5f3484680ae2b2 Mon Sep 17 00:00:00 2001 From: Chen Asraf Date: Sat, 11 Apr 2026 23:27:28 +0300 Subject: [PATCH] feat: update notifications accumulate logic --- lib/Notification/Notifier.php | 232 ++++++++------- lib/Service/NotificationService.php | 276 ++++++++++++++---- .../unit/Service/NotificationServiceTest.php | 7 + 3 files changed, 352 insertions(+), 163 deletions(-) diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index f5ecc98..6ae4c42 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -34,125 +34,159 @@ class Notifier implements INotifier { $l = $this->l10nFactory->get(Application::APP_ID, $languageCode); $params = $notification->getSubjectParameters(); + $count = (int)($params['count'] ?? 1); + $userParam = [ + 'type' => 'user', + 'id' => $params['userId'] ?? '', + 'name' => $params['userDisplayName'] ?? '', + ]; + $houseParam = [ + 'type' => 'highlight', + 'id' => (string)($params['houseId'] ?? ''), + 'name' => $params['houseName'] ?? '', + ]; + switch ($notification->getSubject()) { case 'photo_uploaded': - $notification->setRichSubject( - $l->t('{user} uploaded a photo in {house}'), - [ - 'user' => [ - 'type' => 'user', - 'id' => $params['userId'] ?? '', - 'name' => $params['userDisplayName'] ?? '', - ], - 'house' => [ - 'type' => 'highlight', - 'id' => (string)($params['houseId'] ?? ''), - 'name' => $params['houseName'] ?? '', - ], - ] - ); + if ($count <= 1) { + $notification->setRichSubject( + $l->t('{user} uploaded a photo in {house}'), + ['user' => $userParam, 'house' => $houseParam], + ); + } else { + $notification->setRichSubject( + $l->n( + '{user} uploaded %n photo in {house}', + '{user} uploaded %n photos in {house}', + $count, + ), + ['user' => $userParam, 'house' => $houseParam], + ); + } break; case 'note_created': - $notification->setRichSubject( - $l->t('{user} added a note "{title}" in {house}'), - [ - 'user' => [ - 'type' => 'user', - 'id' => $params['userId'] ?? '', - 'name' => $params['userDisplayName'] ?? '', + if ($count <= 1) { + $notification->setRichSubject( + $l->t('{user} added a note "{title}" in {house}'), + [ + 'user' => $userParam, + 'title' => [ + 'type' => 'highlight', + 'id' => (string)($params['noteId'] ?? ''), + 'name' => $params['noteTitle'] ?? '', + ], + 'house' => $houseParam, ], - 'title' => [ - 'type' => 'highlight', - 'id' => (string)($params['noteId'] ?? ''), - 'name' => $params['noteTitle'] ?? '', - ], - 'house' => [ - 'type' => 'highlight', - 'id' => (string)($params['houseId'] ?? ''), - 'name' => $params['houseName'] ?? '', - ], - ] - ); + ); + } else { + $notification->setRichSubject( + $l->n( + '{user} added %n note in {house}', + '{user} added %n notes in {house}', + $count, + ), + ['user' => $userParam, 'house' => $houseParam], + ); + } break; case 'note_edited': - $notification->setRichSubject( - $l->t('{user} edited the note "{title}" in {house}'), - [ - 'user' => [ - 'type' => 'user', - 'id' => $params['userId'] ?? '', - 'name' => $params['userDisplayName'] ?? '', + if ($count <= 1) { + $notification->setRichSubject( + $l->t('{user} edited the note "{title}" in {house}'), + [ + 'user' => $userParam, + 'title' => [ + 'type' => 'highlight', + 'id' => (string)($params['noteId'] ?? ''), + 'name' => $params['noteTitle'] ?? '', + ], + 'house' => $houseParam, ], - 'title' => [ - 'type' => 'highlight', - 'id' => (string)($params['noteId'] ?? ''), - 'name' => $params['noteTitle'] ?? '', - ], - 'house' => [ - 'type' => 'highlight', - 'id' => (string)($params['houseId'] ?? ''), - 'name' => $params['houseName'] ?? '', - ], - ] - ); + ); + } else { + $notification->setRichSubject( + $l->n( + '{user} edited %n note in {house}', + '{user} edited %n notes in {house}', + $count, + ), + ['user' => $userParam, 'house' => $houseParam], + ); + } break; case 'item_added': - $notification->setRichSubject( - $l->t('{user} added "{item}" to {list} in {house}'), - [ - 'user' => [ - 'type' => 'user', - 'id' => $params['userId'] ?? '', - 'name' => $params['userDisplayName'] ?? '', + $listParam = [ + 'type' => 'highlight', + 'id' => 'list', + 'name' => $params['listName'] ?? '', + ]; + if ($count <= 1) { + $notification->setRichSubject( + $l->t('{user} added "{item}" to {list} in {house}'), + [ + 'user' => $userParam, + 'item' => [ + 'type' => 'highlight', + 'id' => 'item', + 'name' => $params['itemName'] ?? '', + ], + 'list' => $listParam, + 'house' => $houseParam, ], - 'item' => [ - 'type' => 'highlight', - 'id' => 'item', - 'name' => $params['itemName'] ?? '', + ); + } else { + $notification->setRichSubject( + $l->n( + '{user} added %n item to {list} in {house}', + '{user} added %n items to {list} in {house}', + $count, + ), + [ + 'user' => $userParam, + 'list' => $listParam, + 'house' => $houseParam, ], - 'list' => [ - 'type' => 'highlight', - 'id' => 'list', - 'name' => $params['listName'] ?? '', - ], - 'house' => [ - 'type' => 'highlight', - 'id' => (string)($params['houseId'] ?? ''), - 'name' => $params['houseName'] ?? '', - ], - ] - ); + ); + } break; case 'item_done': - $notification->setRichSubject( - $l->t('{user} completed "{item}" on {list} in {house}'), - [ - 'user' => [ - 'type' => 'user', - 'id' => $params['userId'] ?? '', - 'name' => $params['userDisplayName'] ?? '', + $listParam = [ + 'type' => 'highlight', + 'id' => 'list', + 'name' => $params['listName'] ?? '', + ]; + if ($count <= 1) { + $notification->setRichSubject( + $l->t('{user} completed "{item}" on {list} in {house}'), + [ + 'user' => $userParam, + 'item' => [ + 'type' => 'highlight', + 'id' => 'item', + 'name' => $params['itemName'] ?? '', + ], + 'list' => $listParam, + 'house' => $houseParam, ], - 'item' => [ - 'type' => 'highlight', - 'id' => 'item', - 'name' => $params['itemName'] ?? '', + ); + } else { + $notification->setRichSubject( + $l->n( + '{user} completed %n item on {list} in {house}', + '{user} completed %n items on {list} in {house}', + $count, + ), + [ + 'user' => $userParam, + 'list' => $listParam, + 'house' => $houseParam, ], - 'list' => [ - 'type' => 'highlight', - 'id' => 'list', - 'name' => $params['listName'] ?? '', - ], - 'house' => [ - 'type' => 'highlight', - 'id' => (string)($params['houseId'] ?? ''), - 'name' => $params['houseName'] ?? '', - ], - ] - ); + ); + } break; case 'item_reminder': diff --git a/lib/Service/NotificationService.php b/lib/Service/NotificationService.php index 6198021..c0264e4 100644 --- a/lib/Service/NotificationService.php +++ b/lib/Service/NotificationService.php @@ -9,6 +9,7 @@ namespace OCA\Pantry\Service; use OCA\Pantry\AppInfo\Application; use OCA\Pantry\Db\HouseMemberMapper; +use OCP\IConfig; use OCP\IURLGenerator; use OCP\IUserManager; use OCP\Notification\IManager as INotificationManager; @@ -22,6 +23,16 @@ class NotificationService { public const PREF_NOTIFY_ITEM_RECUR = 'notify_item_recur'; public const PREF_NOTIFY_ITEM_DONE = 'notify_item_done'; + /** + * If two events of the same kind happen within this window, they are + * grouped into a single notification with an accumulated count and a + * short list of samples. After this window the counter resets. + */ + private const GROUP_WINDOW_SECONDS = 30 * 60; + + /** Maximum number of sample names stored per group (used for rendering). */ + private const MAX_SAMPLES = 3; + public function __construct( private INotificationManager $notificationManager, private HouseMemberMapper $memberMapper, @@ -29,81 +40,127 @@ class NotificationService { private PrefsService $prefs, private IURLGenerator $urlGenerator, private IUserManager $userManager, + private IConfig $config, private LoggerInterface $logger, ) { } public function notifyPhotoUploaded(int $houseId, string $authorUid): void { - $this->sendToHouseMembers($houseId, $authorUid, 'photo_uploaded', 'photo', self::PREF_NOTIFY_PHOTO, function () use ($houseId, $authorUid) { - $house = $this->houseService->get($houseId); - $author = $this->userManager->get($authorUid); - return [ - 'userId' => $authorUid, - 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, - 'houseId' => $houseId, - 'houseName' => $house->getName(), - ]; - }); + $this->sendAggregated( + $houseId, + $authorUid, + 'photo_uploaded', + 'photo', + 'photo:' . $authorUid . ':' . $houseId, + self::PREF_NOTIFY_PHOTO, + null, + function () use ($houseId, $authorUid) { + $house = $this->houseService->get($houseId); + $author = $this->userManager->get($authorUid); + return [ + 'userId' => $authorUid, + 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, + 'houseId' => $houseId, + 'houseName' => $house->getName(), + ]; + }, + ); } public function notifyNoteCreated(int $houseId, string $authorUid, int $noteId, string $noteTitle): void { - $this->sendToHouseMembers($houseId, $authorUid, 'note_created', 'note', self::PREF_NOTIFY_NOTE_CREATE, function () use ($houseId, $authorUid, $noteId, $noteTitle) { - $house = $this->houseService->get($houseId); - $author = $this->userManager->get($authorUid); - return [ - 'userId' => $authorUid, - 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, - 'houseId' => $houseId, - 'houseName' => $house->getName(), - 'noteId' => $noteId, - 'noteTitle' => $noteTitle, - ]; - }); + $this->sendAggregated( + $houseId, + $authorUid, + 'note_created', + 'note', + 'note_create:' . $authorUid . ':' . $houseId, + self::PREF_NOTIFY_NOTE_CREATE, + $noteTitle, + function () use ($houseId, $authorUid, $noteId, $noteTitle) { + $house = $this->houseService->get($houseId); + $author = $this->userManager->get($authorUid); + return [ + 'userId' => $authorUid, + 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, + 'houseId' => $houseId, + 'houseName' => $house->getName(), + 'noteId' => $noteId, + 'noteTitle' => $noteTitle, + ]; + }, + ); } public function notifyNoteEdited(int $houseId, string $authorUid, int $noteId, string $noteTitle): void { - $this->sendToHouseMembers($houseId, $authorUid, 'note_edited', 'note', self::PREF_NOTIFY_NOTE_EDIT, function () use ($houseId, $authorUid, $noteId, $noteTitle) { - $house = $this->houseService->get($houseId); - $author = $this->userManager->get($authorUid); - return [ - 'userId' => $authorUid, - 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, - 'houseId' => $houseId, - 'houseName' => $house->getName(), - 'noteId' => $noteId, - 'noteTitle' => $noteTitle, - ]; - }); + $this->sendAggregated( + $houseId, + $authorUid, + 'note_edited', + 'note', + 'note_edit:' . $authorUid . ':' . $houseId, + self::PREF_NOTIFY_NOTE_EDIT, + $noteTitle, + function () use ($houseId, $authorUid, $noteId, $noteTitle) { + $house = $this->houseService->get($houseId); + $author = $this->userManager->get($authorUid); + return [ + 'userId' => $authorUid, + 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, + 'houseId' => $houseId, + 'houseName' => $house->getName(), + 'noteId' => $noteId, + 'noteTitle' => $noteTitle, + ]; + }, + ); } public function notifyItemAdded(int $houseId, string $authorUid, string $itemName, string $listName): void { - $this->sendToHouseMembers($houseId, $authorUid, 'item_added', 'item', self::PREF_NOTIFY_ITEM_ADD, function () use ($houseId, $authorUid, $itemName, $listName) { - $house = $this->houseService->get($houseId); - $author = $this->userManager->get($authorUid); - return [ - 'userId' => $authorUid, - 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, - 'houseId' => $houseId, - 'houseName' => $house->getName(), - 'itemName' => $itemName, - 'listName' => $listName, - ]; - }); + $this->sendAggregated( + $houseId, + $authorUid, + 'item_added', + 'item', + 'item_add:' . $authorUid . ':' . $houseId . ':' . $listName, + self::PREF_NOTIFY_ITEM_ADD, + $itemName, + function () use ($houseId, $authorUid, $itemName, $listName) { + $house = $this->houseService->get($houseId); + $author = $this->userManager->get($authorUid); + return [ + 'userId' => $authorUid, + 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, + 'houseId' => $houseId, + 'houseName' => $house->getName(), + 'itemName' => $itemName, + 'listName' => $listName, + ]; + }, + ); } public function notifyItemDone(int $houseId, string $authorUid, string $itemName, string $listName): void { - $this->sendToHouseMembers($houseId, $authorUid, 'item_done', 'item', self::PREF_NOTIFY_ITEM_DONE, function () use ($houseId, $authorUid, $itemName, $listName) { - $house = $this->houseService->get($houseId); - $author = $this->userManager->get($authorUid); - return [ - 'userId' => $authorUid, - 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, - 'houseId' => $houseId, - 'houseName' => $house->getName(), - 'itemName' => $itemName, - 'listName' => $listName, - ]; - }); + $this->sendAggregated( + $houseId, + $authorUid, + 'item_done', + 'item', + 'item_done:' . $authorUid . ':' . $houseId . ':' . $listName, + self::PREF_NOTIFY_ITEM_DONE, + $itemName, + function () use ($houseId, $authorUid, $itemName, $listName) { + $house = $this->houseService->get($houseId); + $author = $this->userManager->get($authorUid); + return [ + 'userId' => $authorUid, + 'userDisplayName' => $author ? $author->getDisplayName() : $authorUid, + 'houseId' => $houseId, + 'houseName' => $house->getName(), + 'itemName' => $itemName, + 'listName' => $listName, + ]; + }, + ); } /** @@ -148,9 +205,38 @@ class NotificationService { } /** - * @param callable():array $paramsFn Lazy parameter builder (only called if at least one member needs notification) + * Send a notification that is automatically aggregated with other events + * in the same scope within GROUP_WINDOW_SECONDS. + * + * Each recipient has an independent accumulator stored in their user + * config (`pantry:notif_state_{hash}`). When a new event arrives we: + * 1. load the accumulator + * 2. if stale (> window), reset count/samples + * 3. increment count, append the sample name + * 4. save the accumulator + * 5. dismiss the old notification for the same object + * 6. create a new notification with `count` and `samples` in params + * + * The Notifier uses those params to render a singular or plural subject. + * + * @param string $scope A stable identifier for the event group (must + * include author + house + any other context like + * list name that should NOT cross-contaminate). + * @param string|null $sample Human-friendly label for the individual + * event (item/note title). null for photos. + * @param callable():array $paramsFn Lazy parameter builder (only called + * if at least one recipient needs it). */ - private function sendToHouseMembers(int $houseId, string $authorUid, string $subject, string $objectType, string $prefKey, callable $paramsFn): void { + private function sendAggregated( + int $houseId, + string $authorUid, + string $subject, + string $objectType, + string $scope, + string $prefKey, + ?string $sample, + callable $paramsFn, + ): void { $members = $this->memberMapper->findByHouse($houseId); $recipients = []; @@ -169,18 +255,51 @@ class NotificationService { return; } - $params = $paramsFn(); + $baseParams = $paramsFn(); $link = $this->urlGenerator->linkToRouteAbsolute('pantry.page.index'); $iconUrl = $this->urlGenerator->getAbsoluteURL( $this->urlGenerator->imagePath(Application::APP_ID, 'app-dark.svg') ); - $objectId = (string)($params['noteId'] ?? $params['houseId']); + // Use a hashed object id so markProcessed matches the same group. + $objectId = substr(md5($scope), 0, 32); + $stateKey = 'notif_state_' . substr(md5($scope), 0, 32); + $now = time(); foreach ($recipients as $uid) { try { - // Dismiss any previous notification for the same object so edits - // don't pile up — only the latest notification is shown. + // Load accumulator state for this recipient. + $raw = $this->config->getUserValue($uid, Application::APP_ID, $stateKey, ''); + $state = $this->decodeState($raw, $now); + + // Increment count and update samples (keeping latest MAX_SAMPLES). + $state['count'] = ($state['count'] ?? 0) + 1; + $state['lastTs'] = $now; + $samples = $state['samples'] ?? []; + if ($sample !== null && $sample !== '') { + // Keep unique, most-recent first. + $samples = array_values(array_filter($samples, fn ($s) => $s !== $sample)); + array_unshift($samples, $sample); + if (count($samples) > self::MAX_SAMPLES) { + $samples = array_slice($samples, 0, self::MAX_SAMPLES); + } + } + $state['samples'] = $samples; + + $this->config->setUserValue( + $uid, + Application::APP_ID, + $stateKey, + json_encode($state), + ); + + // Build final subject params: merge base params with aggregation data. + $params = array_merge($baseParams, [ + 'count' => (int)$state['count'], + 'samples' => $state['samples'], + ]); + + // Dismiss any previous notification for the same object group. $stale = $this->notificationManager->createNotification(); $stale->setApp(Application::APP_ID) ->setUser($uid) @@ -208,6 +327,35 @@ class NotificationService { } } + /** + * Decode an accumulator state, resetting it if it is older than the + * grouping window (so a long pause between events starts a new group). + * + * @return array{count: int, lastTs: int, samples: list} + */ + private function decodeState(string $raw, int $now): array { + $empty = ['count' => 0, 'lastTs' => 0, 'samples' => []]; + if ($raw === '') { + return $empty; + } + $decoded = json_decode($raw, true); + if (!is_array($decoded)) { + return $empty; + } + $lastTs = (int)($decoded['lastTs'] ?? 0); + if ($lastTs === 0 || ($now - $lastTs) > self::GROUP_WINDOW_SECONDS) { + return $empty; + } + return [ + 'count' => (int)($decoded['count'] ?? 0), + 'lastTs' => $lastTs, + 'samples' => array_values(array_filter( + (array)($decoded['samples'] ?? []), + fn ($v) => is_string($v), + )), + ]; + } + public function isNotificationEnabled(string $uid, int $houseId, string $prefKey): bool { return $this->prefs->getNotificationPref($uid, $houseId, $prefKey); } diff --git a/tests/unit/Service/NotificationServiceTest.php b/tests/unit/Service/NotificationServiceTest.php index d2a55f3..a60dba3 100644 --- a/tests/unit/Service/NotificationServiceTest.php +++ b/tests/unit/Service/NotificationServiceTest.php @@ -13,6 +13,7 @@ use OCA\Pantry\Db\HouseMemberMapper; use OCA\Pantry\Service\HouseService; use OCA\Pantry\Service\NotificationService; use OCA\Pantry\Service\PrefsService; +use OCP\IConfig; use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; @@ -35,6 +36,8 @@ class NotificationServiceTest extends TestCase { private IURLGenerator $urlGenerator; /** @var IUserManager&MockObject */ private IUserManager $userManager; + /** @var IConfig&MockObject */ + private IConfig $config; private NotificationService $svc; protected function setUp(): void { @@ -44,6 +47,9 @@ class NotificationServiceTest extends TestCase { $this->prefs = $this->createMock(PrefsService::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->userManager = $this->createMock(IUserManager::class); + $this->config = $this->createMock(IConfig::class); + // Default: no stored accumulator state (fresh group each call). + $this->config->method('getUserValue')->willReturn(''); $this->svc = new NotificationService( $this->notifManager, @@ -52,6 +58,7 @@ class NotificationServiceTest extends TestCase { $this->prefs, $this->urlGenerator, $this->userManager, + $this->config, $this->createMock(LoggerInterface::class), );