From 320081ccd6666322aea2462daa7a02947e2bc885 Mon Sep 17 00:00:00 2001
From: Sahil Batra <sahil@zulip.com>
Date: Wed, 27 Mar 2024 20:02:25 +0530
Subject: [PATCH] mention: Do not include deactivated users in group mention
 data.

There is no behavioral changes to deactivated users as we do
not create UserMessage rows or call the notification code path
for deactivated users in a user group mention. But it is better
to not include the deactivated users in fields like
"mention_user_ids", so this commit updates the code to not
include deactivated users in the computed mention data.
---
 zerver/lib/mention.py                  | 14 ++++++++++++--
 zerver/tests/test_markdown.py          |  9 +++++++++
 zerver/tests/test_notification_data.py | 17 +++++++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/zerver/lib/mention.py b/zerver/lib/mention.py
index bc7c535d4f..5795b0be37 100644
--- a/zerver/lib/mention.py
+++ b/zerver/lib/mention.py
@@ -4,7 +4,7 @@ from dataclasses import dataclass
 from re import Match
 
 from django.conf import settings
-from django.db.models import Q
+from django.db.models import Prefetch, Q
 
 from zerver.lib.users import get_inaccessible_user_ids
 from zerver.models import NamedUserGroup, UserProfile
@@ -269,9 +269,19 @@ class MentionData:
         self.user_group_members: dict[int, list[int]] = {}
         user_group_names = possible_user_group_mentions(content)
         if user_group_names:
+            # We are not directly doing 'prefetch_related("direct_members")'
+            # because then we would have to filter out deactivated users
+            # using 'group.direct_members.filter(is_active=True)', and that
+            # would take away the advantage of using 'prefetch_related'
+            # because of not using group.direct_members.all().
             for group in NamedUserGroup.objects.filter(
                 realm_id=realm_id, name__in=user_group_names, is_system_group=False
-            ).prefetch_related("direct_members"):
+            ).prefetch_related(
+                Prefetch(
+                    "direct_members",
+                    queryset=UserProfile.objects.filter(realm_id=realm_id, is_active=True),
+                )
+            ):
                 self.user_group_name_info[group.name.lower()] = group
                 self.user_group_members[group.id] = [m.id for m in group.direct_members.all()]
 
diff --git a/zerver/tests/test_markdown.py b/zerver/tests/test_markdown.py
index 00490fdd68..3eee89c099 100644
--- a/zerver/tests/test_markdown.py
+++ b/zerver/tests/test_markdown.py
@@ -293,6 +293,15 @@ class MarkdownMiscTest(ZulipTestCase):
         mention_data = MentionData(mention_backend, content, message_sender=None)
         self.assertTrue(mention_data.message_has_topic_wildcards())
 
+        content = "@*hamletcharacters*"
+        group = NamedUserGroup.objects.get(realm=realm, name="hamletcharacters")
+        mention_data = MentionData(mention_backend, content, message_sender=None)
+        self.assertCountEqual(mention_data.get_group_members(group.id), [hamlet.id, cordelia.id])
+
+        change_user_is_active(cordelia, False)
+        mention_data = MentionData(mention_backend, content, message_sender=None)
+        self.assertEqual(mention_data.get_group_members(group.id), [hamlet.id])
+
     def test_invalid_katex_path(self) -> None:
         with self.settings(DEPLOY_ROOT="/nonexistent"):
             with self.assertLogs(level="ERROR") as m:
diff --git a/zerver/tests/test_notification_data.py b/zerver/tests/test_notification_data.py
index 0c49e2deb6..a36f72bb27 100644
--- a/zerver/tests/test_notification_data.py
+++ b/zerver/tests/test_notification_data.py
@@ -1,4 +1,5 @@
 from zerver.actions.user_groups import check_add_user_group
+from zerver.actions.users import do_deactivate_user
 from zerver.lib.mention import MentionBackend, MentionData
 from zerver.lib.notification_data import UserMessageNotificationsData, get_user_group_mentions_data
 from zerver.lib.test_classes import ZulipTestCase
@@ -457,3 +458,19 @@ class TestNotificationData(ZulipTestCase):
                 cordelia.id: hamlet_and_cordelia.id,
             },
         )
+
+        # Deactivated users are excluded.
+        do_deactivate_user(hamlet, acting_user=None)
+        result = get_user_group_mentions_data(
+            mentioned_user_ids=set(),
+            mentioned_user_group_ids=[hamlet_and_cordelia.id],
+            mention_data=MentionData(
+                mention_backend, "hey @*hamlet_and_cordelia*!", message_sender=None
+            ),
+        )
+        self.assertDictEqual(
+            result,
+            {
+                cordelia.id: hamlet_and_cordelia.id,
+            },
+        )