From 2ce63db651ad4ec27b8e7b0edd03c89b1c317f97 Mon Sep 17 00:00:00 2001 From: ramteid Date: Thu, 19 Feb 2026 21:58:36 +0100 Subject: [PATCH] fix(notifications): don't notify users about their own actions Users were receiving notifications for actions they performed themselves, such as commenting on cards or sharing boards. Two root causes: 1. NotificationHelper: sendMention() and sendBoardShared() did not filter out the acting user from notification recipients. - sendMention: skip mentions where the mentioned user is the comment author (currentUser) - sendBoardShared (user type): only call notify() when participant differs from currentUser; markProcessed still runs unconditionally to clean up any pre-existing stale notifications on unshare 2. DeckProvider: $ownActivity was computed from $event->getAuthor() before the real author was resolved. For comments, the author is overridden to DECK_NOAUTHOR_COMMENT_SYSTEM_ENFORCED in ActivityManager, so $ownActivity was always false. Moved the check after the author fallback resolution. Note: sendCardAssigned already had the self-check in AssignmentService. Signed-off-by: ramteid --- lib/Activity/DeckProvider.php | 2 +- lib/Notification/NotificationHelper.php | 5 +- .../Notification/NotificationHelperTest.php | 116 ++++++++++++++++++ 3 files changed, 121 insertions(+), 2 deletions(-) diff --git a/lib/Activity/DeckProvider.php b/lib/Activity/DeckProvider.php index 8dbff70e1..e81c6a2e1 100644 --- a/lib/Activity/DeckProvider.php +++ b/lib/Activity/DeckProvider.php @@ -73,7 +73,6 @@ public function parse($language, IEvent $event, ?IEvent $previousEvent = null): $subjectIdentifier = $event->getSubject(); $subjectParams = $event->getSubjectParameters(); - $ownActivity = ($event->getAuthor() === $this->userId); /** * Map stored parameter objects to rich string types @@ -85,6 +84,7 @@ public function parse($language, IEvent $event, ?IEvent $previousEvent = null): $author = $subjectParams['author']; unset($subjectParams['author']); } + $ownActivity = ($author === $this->userId); $user = $this->userManager->get($author); $params = []; if ($user !== null) { diff --git a/lib/Notification/NotificationHelper.php b/lib/Notification/NotificationHelper.php index 10b2197b3..d11b2201e 100644 --- a/lib/Notification/NotificationHelper.php +++ b/lib/Notification/NotificationHelper.php @@ -174,7 +174,7 @@ public function sendBoardShared(int $boardId, Acl $acl, bool $markAsRead = false $notification = $this->generateBoardShared($board, $acl->getParticipant()); if ($markAsRead) { $this->notificationManager->markProcessed($notification); - } else { + } elseif ($acl->getParticipant() !== $this->currentUser) { $notification->setDateTime(new DateTime()); $this->notificationManager->notify($notification); } @@ -201,6 +201,9 @@ public function sendBoardShared(int $boardId, Acl $acl, bool $markAsRead = false public function sendMention(IComment $comment): void { foreach ($comment->getMentions() as $mention) { + if ((string)$mention['id'] === $this->currentUser) { + continue; + } $card = $this->cardMapper->find($comment->getObjectId()); $boardId = $this->cardMapper->findBoardId($card->getId()); $notification = $this->notificationManager->createNotification(); diff --git a/tests/unit/Notification/NotificationHelperTest.php b/tests/unit/Notification/NotificationHelperTest.php index 3ac7d3c78..36cb9bb55 100644 --- a/tests/unit/Notification/NotificationHelperTest.php +++ b/tests/unit/Notification/NotificationHelperTest.php @@ -527,4 +527,120 @@ public function testSendMention() { $this->notificationHelper->sendMention($comment); } + + /** + * When a comment mentions the current user ('admin'), that mention must + * be silently skipped while other mentioned users still get notified. + */ + public function testSendMentionSkipsSelf() { + $comment = $this->createMock(IComment::class); + $comment->expects($this->any()) + ->method('getObjectId') + ->willReturn(123); + $comment->expects($this->any()) + ->method('getMessage') + ->willReturn('@admin @user1 This is a message.'); + $comment->expects($this->once()) + ->method('getMentions') + ->willReturn([ + ['id' => 'admin'], + ['id' => 'user1'] + ]); + $card = new Card(); + $card->setId(123); + $card->setTitle('MyCard'); + $this->cardMapper->expects($this->any()) + ->method('find') + ->with(123) + ->willReturn($card); + $this->cardMapper->expects($this->any()) + ->method('findBoardId') + ->with(123) + ->willReturn(1); + + // Only one notification should be created — for user1, not for admin + $notification = $this->createMock(INotification::class); + $notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification); + $notification->expects($this->once())->method('setUser')->with('user1')->willReturn($notification); + $notification->expects($this->once())->method('setObject')->with('card', 123)->willReturn($notification); + $notification->expects($this->once())->method('setSubject')->with('card-comment-mentioned', ['MyCard', 1, 'admin'])->willReturn($notification); + $notification->expects($this->once())->method('setDateTime')->willReturn($notification); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + $this->notificationManager->expects($this->once()) + ->method('notify') + ->with($notification); + + $this->notificationHelper->sendMention($comment); + } + + /** + * Sharing a board with yourself must not trigger a notification. + */ + public function testSendBoardSharedUserSkipsSelf() { + $board = new Board(); + $board->setId(123); + $board->setTitle('MyBoardTitle'); + $acl = new Acl(); + $acl->setParticipant('admin'); + $acl->setType(Acl::PERMISSION_TYPE_USER); + $this->boardMapper->expects($this->once()) + ->method('find') + ->with(123) + ->willReturn($board); + + // generateBoardShared still creates the notification object, + // but notify() must never be called for the acting user + $notification = $this->createMock(INotification::class); + $notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification); + $notification->expects($this->once())->method('setUser')->with('admin')->willReturn($notification); + $notification->expects($this->once())->method('setObject')->with('board', 123)->willReturn($notification); + $notification->expects($this->once())->method('setSubject')->with('board-shared', ['MyBoardTitle', 'admin'])->willReturn($notification); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + $this->notificationManager->expects($this->never()) + ->method('notify'); + + $this->notificationHelper->sendBoardShared(123, $acl); + } + + /** + * Unsharing a board (markAsRead=true) must still clean up stale + * notifications via markProcessed, even when the participant is the + * acting user. This ensures pre-existing self-notifications get removed. + */ + public function testSendBoardSharedSelfMarkAsReadStillCleansUp() { + $board = new Board(); + $board->setId(123); + $board->setTitle('MyBoardTitle'); + $acl = new Acl(); + $acl->setParticipant('admin'); + $acl->setType(Acl::PERMISSION_TYPE_USER); + $this->boardMapper->expects($this->once()) + ->method('find') + ->with(123) + ->willReturn($board); + + $notification = $this->createMock(INotification::class); + $notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification); + $notification->expects($this->once())->method('setUser')->with('admin')->willReturn($notification); + $notification->expects($this->once())->method('setObject')->with('board', 123)->willReturn($notification); + $notification->expects($this->once())->method('setSubject')->with('board-shared', ['MyBoardTitle', 'admin'])->willReturn($notification); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + // markProcessed must be called to clean up any stale notification + $this->notificationManager->expects($this->once()) + ->method('markProcessed') + ->with($notification); + $this->notificationManager->expects($this->never()) + ->method('notify'); + + $this->notificationHelper->sendBoardShared(123, $acl, true); + } }