mention: Mentions via subgroup should be highlighted.

This commit does not add highlighting to any pre-existing group mentions
for which a user was part of the mentioned group via a subgroup. This
only fixes it for mentions moving forward.
Fixes https://chat.zulip.org/#narrow/channel/9-issues/topic/group.20mention.20not.20highlighted/near/2008541

web/src/markdown.ts

@@ -58,7 +58,7 @@ export type MarkdownHelpers = {
 
     // user groups
     get_user_group_from_name: (name: string) => {id: number; name: string} | undefined;
-    is_member_of_user_group: (user_id: number, user_group_id: number) => boolean;
+    is_member_of_user_group: (user_group_id: number, user_id: number) => boolean;
 
     // stream hashes
     get_stream_by_name: (stream_name: string) => {stream_id: number; name: string} | undefined;
@@ -318,7 +318,7 @@ function parse_with_options(
                     display_text = "@" + group.name;
                     classes = "user-group-mention";
                     if (
-                        helper_config.is_member_of_user_group(helper_config.my_user_id(), group.id)
+                        helper_config.is_member_of_user_group(group.id, helper_config.my_user_id())
                     ) {
                         // Mentioned the current user's group.
                         mentioned_group = true;

web/src/markdown_config.ts

@@ -72,7 +72,7 @@ export const get_helpers = (): MarkdownHelpers => ({
 
     // user groups
     get_user_group_from_name: (name) => user_group(user_groups.get_user_group_from_name(name)),
-    is_member_of_user_group: user_groups.is_direct_member_of,
+    is_member_of_user_group: user_groups.is_user_in_group,
 
     // stream hashes
     get_stream_by_name: (name) => stream(stream_data.get_sub(name)),

web/src/rendered_markdown.ts

@@ -205,7 +205,7 @@ export const update_elements = ($content: JQuery): void => {
 
         const my_user_id = people.my_current_user_id();
         // Mark user group you're a member of.
-        if (user_groups.is_direct_member_of(my_user_id, user_group_id)) {
+        if (user_groups.is_user_in_group(user_group_id, my_user_id)) {
             $(this).addClass("user-mention-me");
         }
 

web/tests/markdown_parse.test.cjs

@@ -55,7 +55,7 @@ function get_user_group_from_name(name) {
     return user_group_map.get(name);
 }
 
-function is_member_of_user_group(user_id, user_group_id) {
+function is_member_of_user_group(user_group_id, user_id) {
     assert.equal(user_group_id, staff_group.id);
     assert.equal(user_id, my_id);
     return true;

web/tests/rendered_markdown.test.cjs

@@ -76,8 +76,14 @@ const group_other = {
     id: 2,
     members: [cordelia.user_id],
 };
+const group_me_via_subgroup = {
+    name: "I am part of this group via a subgroup",
+    id: 3,
+    members: [],
+    direct_subgroup_ids: [group_me.id],
+};
 user_groups.initialize({
-    realm_user_groups: [group_me, group_other],
+    realm_user_groups: [group_me, group_other, group_me_via_subgroup],
 });
 
 const stream = {
@@ -363,6 +369,33 @@ run_test("user-group-mention", () => {
     assert.equal($group_other.text(), `@${group_other.name}`);
 });
 
+run_test("user-group-mention", () => {
+    // Setup
+    const $content = get_content_element();
+    const $group_me_via_subgroup = $.create("user-group-mention(me_via_subgroup)");
+    $group_me_via_subgroup.set_find_results(".highlight", false);
+    $group_me_via_subgroup.attr("data-user-group-id", group_me_via_subgroup.id);
+    const $group_other = $.create("user-group-mention(other)");
+    $group_other.set_find_results(".highlight", false);
+    $group_other.attr("data-user-group-id", group_other.id);
+    $content.set_find_results(
+        ".user-group-mention",
+        $array([$group_me_via_subgroup, $group_other]),
+    );
+
+    // Initial asserts
+    assert.ok(!$group_me_via_subgroup.hasClass("user-mention-me"));
+    assert.equal($group_me_via_subgroup.text(), "never-been-set");
+    assert.equal($group_other.text(), "never-been-set");
+
+    rm.update_elements($content);
+
+    // Final asserts
+    assert.ok($group_me_via_subgroup.hasClass("user-mention-me"));
+    assert.equal($group_me_via_subgroup.text(), `@${group_me_via_subgroup.name}`);
+    assert.equal($group_other.text(), `@${group_other.name}`);
+});
+
 run_test("user-group-mention (error)", () => {
     const $content = get_content_element();
     const $group = $.create("user-group-mention(bogus)");

zerver/lib/mention.py

@@ -4,8 +4,9 @@ from dataclasses import dataclass
 from re import Match
 
 from django.conf import settings
-from django.db.models import Prefetch, Q
+from django.db.models import Q
 
+from zerver.lib.user_groups import get_recursive_group_members
 from zerver.lib.users import get_inaccessible_user_ids
 from zerver.models import NamedUserGroup, UserProfile
 from zerver.models.streams import get_linkable_streams
@@ -269,21 +270,13 @@ 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(
-                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()]
+                self.user_group_members[group.id] = [
+                    m.id for m in get_recursive_group_members(group)
+                ]
 
     def get_user_by_name(self, name: str) -> FullNameInfo | None:
         # warning: get_user_by_name is not dependable if two

zerver/tests/test_message_edit.py

@@ -17,11 +17,12 @@ from zerver.actions.user_topics import do_set_user_topic_visibility_policy
 from zerver.lib.message import messages_for_ids
 from zerver.lib.message_cache import MessageDict
 from zerver.lib.test_classes import ZulipTestCase
-from zerver.lib.test_helpers import queries_captured
+from zerver.lib.test_helpers import most_recent_message, queries_captured
 from zerver.lib.topic import TOPIC_NAME
 from zerver.lib.utils import assert_is_not_none
 from zerver.models import Attachment, Message, NamedUserGroup, Realm, UserProfile, UserTopic
 from zerver.models.groups import SystemGroups
+from zerver.models.messages import UserMessage
 from zerver.models.realms import WildcardMentionPolicyEnum, get_realm
 from zerver.models.streams import get_stream
 
@@ -1557,6 +1558,42 @@ class EditMessageTest(ZulipTestCase):
             )
         self.assert_json_success(result)
 
+    def test_user_group_mentions_via_subgroup_when_editing(self) -> None:
+        user_profile = self.example_user("iago")
+        self.login("hamlet")
+        self.subscribe(user_profile, "Denmark")
+        my_group = check_add_user_group(
+            user_profile.realm, "my_group", [user_profile], acting_user=user_profile
+        )
+        my_group_via_subgroup = check_add_user_group(
+            user_profile.realm, "my_group_via_subgroup", [], acting_user=user_profile
+        )
+        add_subgroups_to_user_group(my_group_via_subgroup, [my_group], acting_user=None)
+
+        self.send_stream_message(
+            self.example_user("hamlet"), "Denmark", content="there is no mention"
+        )
+
+        message = most_recent_message(user_profile)
+        assert (
+            UserMessage.objects.get(
+                user_profile=user_profile, message=message
+            ).flags.mentioned.is_set
+            is False
+        )
+
+        result = self.client_patch(
+            "/json/messages/" + str(message.id),
+            {
+                "content": "test @*my_group_via_subgroup* mention",
+            },
+        )
+        self.assert_json_success(result)
+
+        assert UserMessage.objects.get(
+            user_profile=user_profile, message=message
+        ).flags.mentioned.is_set
+
     def test_user_group_mention_restrictions_while_editing(self) -> None:
         iago = self.example_user("iago")
         shiva = self.example_user("shiva")

zerver/tests/test_message_send.py

@@ -2153,6 +2153,26 @@ class StreamMessagesTest(ZulipTestCase):
         ):
             self.send_stream_message(cordelia, "test_stream", content)
 
+    def test_user_group_mentions_via_subgroup(self) -> None:
+        user_profile = self.example_user("iago")
+        self.subscribe(user_profile, "Denmark")
+        my_group = check_add_user_group(
+            user_profile.realm, "my_group", [user_profile], acting_user=user_profile
+        )
+        my_group_via_subgroup = check_add_user_group(
+            user_profile.realm, "my_group_via_subgroup", [], acting_user=user_profile
+        )
+        add_subgroups_to_user_group(my_group_via_subgroup, [my_group], acting_user=None)
+
+        self.send_stream_message(
+            self.example_user("hamlet"), "Denmark", content="test @*my_group_via_subgroup* mention"
+        )
+
+        message = most_recent_message(user_profile)
+        assert UserMessage.objects.get(
+            user_profile=user_profile, message=message
+        ).flags.mentioned.is_set
+
     def test_user_group_mention_restrictions(self) -> None:
         iago = self.example_user("iago")
         shiva = self.example_user("shiva")