Skip to content

Commit ae15487

Browse files
mejo-grnd-alt
mejo-
authored and
grnd-alt
committed
fix(notifciations): only notify users with access to board on mentions
Signed-off-by: Jonas <jonas@freesources.org>
1 parent a2f0de8 commit ae15487

2 files changed

Lines changed: 176 additions & 5 deletions

File tree

‎lib/Notification/NotificationHelper.php‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -200,9 +200,28 @@ public function sendBoardShared(int $boardId, Acl $acl, bool $markAsRead = false
200200
}
201201

202202
public function sendMention(IComment $comment): void {
203-
foreach ($comment->getMentions() as $mention) {
204-
$card = $this->cardMapper->find($comment->getObjectId());
205-
$boardId = $this->cardMapper->findBoardId($card->getId());
203+
$mentions = $comment->getMentions();
204+
if (empty($mentions)) {
205+
return;
206+
}
207+
208+
$card = $this->cardMapper->find($comment->getObjectId());
209+
$boardId = $this->cardMapper->findBoardId($card->getId());
210+
$boardUsers = $this->permissionService->findUsers($boardId);
211+
212+
foreach ($mentions as $mention) {
213+
if (($mention['type'] ?? 'users') !== 'users') {
214+
// skip non-user mentions
215+
continue;
216+
}
217+
$mentionedUserId = (string)$mention['id'];
218+
if ($mentionedUserId === $this->currentUser) {
219+
continue;
220+
}
221+
if (!array_key_exists($mentionedUserId, $boardUsers)) {
222+
// skip users that don't have access to the board
223+
continue;
224+
}
206225
$notification = $this->notificationManager->createNotification();
207226
$notification
208227
->setApp('deck')

‎tests/unit/Notification/NotificationHelperTest.php‎

Lines changed: 154 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -489,8 +489,8 @@ public function testSendMention() {
489489
$comment->expects($this->once())
490490
->method('getMentions')
491491
->willReturn([
492-
['id' => 'user1'],
493-
['id' => 'user2']
492+
['id' => 'user1', 'type' => 'users'],
493+
['id' => 'user2', 'type' => 'users'],
494494
]);
495495
$card = new Card();
496496
$card->setId(123);
@@ -503,6 +503,13 @@ public function testSendMention() {
503503
->method('findBoardId')
504504
->with(123)
505505
->willReturn(1);
506+
$this->permissionService->expects($this->once())
507+
->method('findUsers')
508+
->with(1)
509+
->willReturn([
510+
'user1' => $this->createUserMock('user1'),
511+
'user2' => $this->createUserMock('user2'),
512+
]);
506513

507514
$notification1 = $this->createMock(INotification::class);
508515
$notification1->expects($this->once())->method('setApp')->with('deck')->willReturn($notification1);
@@ -527,4 +534,149 @@ public function testSendMention() {
527534

528535
$this->notificationHelper->sendMention($comment);
529536
}
537+
538+
/**
539+
* When a comment mentions the current user ('admin'), that mention must
540+
* be silently skipped while other mentioned users still get notified.
541+
*/
542+
public function testSendMentionSkipsSelf() {
543+
$comment = $this->createMock(IComment::class);
544+
$comment->expects($this->any())
545+
->method('getObjectId')
546+
->willReturn(123);
547+
$comment->expects($this->any())
548+
->method('getMessage')
549+
->willReturn('@admin @user1 This is a message.');
550+
$comment->expects($this->once())
551+
->method('getMentions')
552+
->willReturn([
553+
['id' => 'admin', 'type' => 'users'],
554+
['id' => 'user1', 'type' => 'users'],
555+
]);
556+
$card = new Card();
557+
$card->setId(123);
558+
$card->setTitle('MyCard');
559+
$this->cardMapper->expects($this->any())
560+
->method('find')
561+
->with(123)
562+
->willReturn($card);
563+
$this->cardMapper->expects($this->any())
564+
->method('findBoardId')
565+
->with(123)
566+
->willReturn(1);
567+
$this->permissionService->expects($this->once())
568+
->method('findUsers')
569+
->with(1)
570+
->willReturn([
571+
'admin' => $this->createUserMock('admin'),
572+
'user1' => $this->createUserMock('user1'),
573+
]);
574+
575+
// Only one notification should be created — for user1, not for admin
576+
$notification = $this->createMock(INotification::class);
577+
$notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification);
578+
$notification->expects($this->once())->method('setUser')->with('user1')->willReturn($notification);
579+
$notification->expects($this->once())->method('setObject')->with('card', 123)->willReturn($notification);
580+
$notification->expects($this->once())->method('setSubject')->with('card-comment-mentioned', ['MyCard', 1, 'admin'])->willReturn($notification);
581+
$notification->expects($this->once())->method('setDateTime')->willReturn($notification);
582+
583+
$this->notificationManager->expects($this->once())
584+
->method('createNotification')
585+
->willReturn($notification);
586+
$this->notificationManager->expects($this->once())
587+
->method('notify')
588+
->with($notification);
589+
590+
$this->notificationHelper->sendMention($comment);
591+
}
592+
593+
/**
594+
* Unsharing a board (markAsRead=true) must still clean up stale
595+
* notifications via markProcessed, even when the participant is the
596+
* acting user. This ensures pre-existing self-notifications get removed.
597+
*/
598+
public function testSendBoardSharedSelfMarkAsReadStillCleansUp() {
599+
$board = new Board();
600+
$board->setId(123);
601+
$board->setTitle('MyBoardTitle');
602+
$acl = new Acl();
603+
$acl->setParticipant('admin');
604+
$acl->setType(Acl::PERMISSION_TYPE_USER);
605+
$this->boardMapper->expects($this->once())
606+
->method('find')
607+
->with(123)
608+
->willReturn($board);
609+
610+
$notification = $this->createMock(INotification::class);
611+
$notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification);
612+
$notification->expects($this->once())->method('setUser')->with('admin')->willReturn($notification);
613+
$notification->expects($this->once())->method('setObject')->with('board', 123)->willReturn($notification);
614+
$notification->expects($this->once())->method('setSubject')->with('board-shared', ['MyBoardTitle', 'admin'])->willReturn($notification);
615+
616+
$this->notificationManager->expects($this->once())
617+
->method('createNotification')
618+
->willReturn($notification);
619+
// markProcessed must be called to clean up any stale notification
620+
$this->notificationManager->expects($this->once())
621+
->method('markProcessed')
622+
->with($notification);
623+
$this->notificationManager->expects($this->never())
624+
->method('notify');
625+
626+
$this->notificationHelper->sendBoardShared(123, $acl, true);
627+
}
628+
629+
/**
630+
* A mention of a user who is not a board member must not trigger a notification.
631+
*/
632+
public function testSendMentionSkipsNonBoardMembers(): void {
633+
$comment = $this->createMock(IComment::class);
634+
$comment->expects($this->any())
635+
->method('getObjectId')
636+
->willReturn(123);
637+
$comment->expects($this->any())
638+
->method('getMessage')
639+
->willReturn('@user1 @outsider This is a message.');
640+
$comment->expects($this->once())
641+
->method('getMentions')
642+
->willReturn([
643+
['id' => 'user1', 'type' => 'users'],
644+
['id' => 'outsider', 'type' => 'users'],
645+
]);
646+
647+
$card = new Card();
648+
$card->setId(123);
649+
$card->setTitle('MyCard');
650+
$this->cardMapper->expects($this->once())
651+
->method('find')
652+
->with(123)
653+
->willReturn($card);
654+
$this->cardMapper->expects($this->once())
655+
->method('findBoardId')
656+
->with(123)
657+
->willReturn(1);
658+
// Only user1 is a board member; outsider is not
659+
$this->permissionService->expects($this->once())
660+
->method('findUsers')
661+
->with(1)
662+
->willReturn([
663+
'user1' => $this->createUserMock('user1'),
664+
]);
665+
666+
// Exactly one notification must be created and sent — for user1 only
667+
$notification = $this->createMock(INotification::class);
668+
$notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification);
669+
$notification->expects($this->once())->method('setUser')->with('user1')->willReturn($notification);
670+
$notification->expects($this->once())->method('setObject')->with('card', 123)->willReturn($notification);
671+
$notification->expects($this->once())->method('setSubject')->with('card-comment-mentioned', ['MyCard', 1, 'admin'])->willReturn($notification);
672+
$notification->expects($this->once())->method('setDateTime')->willReturn($notification);
673+
674+
$this->notificationManager->expects($this->once())
675+
->method('createNotification')
676+
->willReturn($notification);
677+
$this->notificationManager->expects($this->once())
678+
->method('notify')
679+
->with($notification);
680+
$this->notificationHelper->sendMention($comment);
681+
}
530682
}

0 commit comments

Comments
 (0)