Skip to content

Commit df1b3e0

Browse files
ramteidramteid
committed
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 <ramteid@users.noreply.github.com>
1 parent d1d4fbf commit df1b3e0

3 files changed

Lines changed: 121 additions & 2 deletions

File tree

‎lib/Activity/DeckProvider.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,6 @@ public function parse($language, IEvent $event, ?IEvent $previousEvent = null):
7373

7474
$subjectIdentifier = $event->getSubject();
7575
$subjectParams = $event->getSubjectParameters();
76-
$ownActivity = ($event->getAuthor() === $this->userId);
7776

7877
/**
7978
* Map stored parameter objects to rich string types
@@ -85,6 +84,7 @@ public function parse($language, IEvent $event, ?IEvent $previousEvent = null):
8584
$author = $subjectParams['author'];
8685
unset($subjectParams['author']);
8786
}
87+
$ownActivity = ($author === $this->userId);
8888
$user = $this->userManager->get($author);
8989
$params = [];
9090
if ($user !== null) {

‎lib/Notification/NotificationHelper.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ public function sendBoardShared(int $boardId, Acl $acl, bool $markAsRead = false
174174
$notification = $this->generateBoardShared($board, $acl->getParticipant());
175175
if ($markAsRead) {
176176
$this->notificationManager->markProcessed($notification);
177-
} else {
177+
} elseif ($acl->getParticipant() !== $this->currentUser) {
178178
$notification->setDateTime(new DateTime());
179179
$this->notificationManager->notify($notification);
180180
}
@@ -201,6 +201,9 @@ public function sendBoardShared(int $boardId, Acl $acl, bool $markAsRead = false
201201

202202
public function sendMention(IComment $comment): void {
203203
foreach ($comment->getMentions() as $mention) {
204+
if ((string)$mention['id'] === $this->currentUser) {
205+
continue;
206+
}
204207
$card = $this->cardMapper->find($comment->getObjectId());
205208
$boardId = $this->cardMapper->findBoardId($card->getId());
206209
$notification = $this->notificationManager->createNotification();

‎tests/unit/Notification/NotificationHelperTest.php‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,4 +527,120 @@ public function testSendMention() {
527527

528528
$this->notificationHelper->sendMention($comment);
529529
}
530+
531+
/**
532+
* When a comment mentions the current user ('admin'), that mention must
533+
* be silently skipped while other mentioned users still get notified.
534+
*/
535+
public function testSendMentionSkipsSelf() {
536+
$comment = $this->createMock(IComment::class);
537+
$comment->expects($this->any())
538+
->method('getObjectId')
539+
->willReturn(123);
540+
$comment->expects($this->any())
541+
->method('getMessage')
542+
->willReturn('@admin @user1 This is a message.');
543+
$comment->expects($this->once())
544+
->method('getMentions')
545+
->willReturn([
546+
['id' => 'admin'],
547+
['id' => 'user1']
548+
]);
549+
$card = new Card();
550+
$card->setId(123);
551+
$card->setTitle('MyCard');
552+
$this->cardMapper->expects($this->any())
553+
->method('find')
554+
->with(123)
555+
->willReturn($card);
556+
$this->cardMapper->expects($this->any())
557+
->method('findBoardId')
558+
->with(123)
559+
->willReturn(1);
560+
561+
// Only one notification should be created — for user1, not for admin
562+
$notification = $this->createMock(INotification::class);
563+
$notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification);
564+
$notification->expects($this->once())->method('setUser')->with('user1')->willReturn($notification);
565+
$notification->expects($this->once())->method('setObject')->with('card', 123)->willReturn($notification);
566+
$notification->expects($this->once())->method('setSubject')->with('card-comment-mentioned', ['MyCard', 1, 'admin'])->willReturn($notification);
567+
$notification->expects($this->once())->method('setDateTime')->willReturn($notification);
568+
569+
$this->notificationManager->expects($this->once())
570+
->method('createNotification')
571+
->willReturn($notification);
572+
$this->notificationManager->expects($this->once())
573+
->method('notify')
574+
->with($notification);
575+
576+
$this->notificationHelper->sendMention($comment);
577+
}
578+
579+
/**
580+
* Sharing a board with yourself must not trigger a notification.
581+
*/
582+
public function testSendBoardSharedUserSkipsSelf() {
583+
$board = new Board();
584+
$board->setId(123);
585+
$board->setTitle('MyBoardTitle');
586+
$acl = new Acl();
587+
$acl->setParticipant('admin');
588+
$acl->setType(Acl::PERMISSION_TYPE_USER);
589+
$this->boardMapper->expects($this->once())
590+
->method('find')
591+
->with(123)
592+
->willReturn($board);
593+
594+
// generateBoardShared still creates the notification object,
595+
// but notify() must never be called for the acting user
596+
$notification = $this->createMock(INotification::class);
597+
$notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification);
598+
$notification->expects($this->once())->method('setUser')->with('admin')->willReturn($notification);
599+
$notification->expects($this->once())->method('setObject')->with('board', 123)->willReturn($notification);
600+
$notification->expects($this->once())->method('setSubject')->with('board-shared', ['MyBoardTitle', 'admin'])->willReturn($notification);
601+
602+
$this->notificationManager->expects($this->once())
603+
->method('createNotification')
604+
->willReturn($notification);
605+
$this->notificationManager->expects($this->never())
606+
->method('notify');
607+
608+
$this->notificationHelper->sendBoardShared(123, $acl);
609+
}
610+
611+
/**
612+
* Unsharing a board (markAsRead=true) must still clean up stale
613+
* notifications via markProcessed, even when the participant is the
614+
* acting user. This ensures pre-existing self-notifications get removed.
615+
*/
616+
public function testSendBoardSharedSelfMarkAsReadStillCleansUp() {
617+
$board = new Board();
618+
$board->setId(123);
619+
$board->setTitle('MyBoardTitle');
620+
$acl = new Acl();
621+
$acl->setParticipant('admin');
622+
$acl->setType(Acl::PERMISSION_TYPE_USER);
623+
$this->boardMapper->expects($this->once())
624+
->method('find')
625+
->with(123)
626+
->willReturn($board);
627+
628+
$notification = $this->createMock(INotification::class);
629+
$notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification);
630+
$notification->expects($this->once())->method('setUser')->with('admin')->willReturn($notification);
631+
$notification->expects($this->once())->method('setObject')->with('board', 123)->willReturn($notification);
632+
$notification->expects($this->once())->method('setSubject')->with('board-shared', ['MyBoardTitle', 'admin'])->willReturn($notification);
633+
634+
$this->notificationManager->expects($this->once())
635+
->method('createNotification')
636+
->willReturn($notification);
637+
// markProcessed must be called to clean up any stale notification
638+
$this->notificationManager->expects($this->once())
639+
->method('markProcessed')
640+
->with($notification);
641+
$this->notificationManager->expects($this->never())
642+
->method('notify');
643+
644+
$this->notificationHelper->sendBoardShared(123, $acl, true);
645+
}
530646
}

0 commit comments

Comments
 (0)