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.
This commit is contained in:
Sahil Batra 2024-03-27 20:02:25 +05:30 committed by Tim Abbott
parent 0b58820294
commit 320081ccd6
3 changed files with 38 additions and 2 deletions

View File

@ -4,7 +4,7 @@ from dataclasses import dataclass
from re import Match from re import Match
from django.conf import settings 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.lib.users import get_inaccessible_user_ids
from zerver.models import NamedUserGroup, UserProfile from zerver.models import NamedUserGroup, UserProfile
@ -269,9 +269,19 @@ class MentionData:
self.user_group_members: dict[int, list[int]] = {} self.user_group_members: dict[int, list[int]] = {}
user_group_names = possible_user_group_mentions(content) user_group_names = possible_user_group_mentions(content)
if user_group_names: 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( for group in NamedUserGroup.objects.filter(
realm_id=realm_id, name__in=user_group_names, is_system_group=False 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_name_info[group.name.lower()] = group
self.user_group_members[group.id] = [m.id for m in group.direct_members.all()] self.user_group_members[group.id] = [m.id for m in group.direct_members.all()]

View File

@ -293,6 +293,15 @@ class MarkdownMiscTest(ZulipTestCase):
mention_data = MentionData(mention_backend, content, message_sender=None) mention_data = MentionData(mention_backend, content, message_sender=None)
self.assertTrue(mention_data.message_has_topic_wildcards()) 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: def test_invalid_katex_path(self) -> None:
with self.settings(DEPLOY_ROOT="/nonexistent"): with self.settings(DEPLOY_ROOT="/nonexistent"):
with self.assertLogs(level="ERROR") as m: with self.assertLogs(level="ERROR") as m:

View File

@ -1,4 +1,5 @@
from zerver.actions.user_groups import check_add_user_group 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.mention import MentionBackend, MentionData
from zerver.lib.notification_data import UserMessageNotificationsData, get_user_group_mentions_data from zerver.lib.notification_data import UserMessageNotificationsData, get_user_group_mentions_data
from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_classes import ZulipTestCase
@ -457,3 +458,19 @@ class TestNotificationData(ZulipTestCase):
cordelia.id: hamlet_and_cordelia.id, 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,
},
)