diff --git a/web/src/markdown.ts b/web/src/markdown.ts index 074d4a5588..0c4572a1b2 100644 --- a/web/src/markdown.ts +++ b/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; diff --git a/web/src/markdown_config.ts b/web/src/markdown_config.ts index 1b2b716709..d82b735ec4 100644 --- a/web/src/markdown_config.ts +++ b/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)), diff --git a/web/src/rendered_markdown.ts b/web/src/rendered_markdown.ts index 6bdd097f0d..5269e50dc5 100644 --- a/web/src/rendered_markdown.ts +++ b/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"); } diff --git a/web/tests/markdown_parse.test.cjs b/web/tests/markdown_parse.test.cjs index d4db3c65d1..83098abf30 100644 --- a/web/tests/markdown_parse.test.cjs +++ b/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; diff --git a/web/tests/rendered_markdown.test.cjs b/web/tests/rendered_markdown.test.cjs index 66b19adf13..ad4a302bf8 100644 --- a/web/tests/rendered_markdown.test.cjs +++ b/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)"); diff --git a/zerver/lib/mention.py b/zerver/lib/mention.py index 5795b0be37..26f2b25e11 100644 --- a/zerver/lib/mention.py +++ b/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 diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 209d6fe0a1..c699e16c72 100644 --- a/zerver/tests/test_message_edit.py +++ b/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") diff --git a/zerver/tests/test_message_send.py b/zerver/tests/test_message_send.py index b55ef2cf1c..378e861ac5 100644 --- a/zerver/tests/test_message_send.py +++ b/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")