From 410ae119d42d987e649116e5d795b506497b6784 Mon Sep 17 00:00:00 2001 From: roanster007 Date: Mon, 27 May 2024 15:11:19 +0530 Subject: [PATCH] markdown: Convert topic links generated by "#-mentions" to permalinks. This commit converts the links generated by the markdown of the "#-mention" of topics to permalinks -- the links containing the "with" narrow operator, the operand being the last message of the channel and topic of the mention. Fixes part of #21505 --- api_docs/changelog.md | 6 ++ api_docs/message-formatting.md | 38 +++++++- version.py | 2 +- zerver/lib/markdown/__init__.py | 30 +++++- zerver/lib/mention.py | 86 +++++++++++++++- zerver/lib/topic.py | 33 +++++++ zerver/tests/test_markdown.py | 119 ++++++++++++++++++++++- zerver/tests/test_message_move_stream.py | 2 +- 8 files changed, 300 insertions(+), 16 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index c027038a22..768a0776f4 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 347** + +* [Markdown message formatting](/api/message-formatting#links-to-channels-topics-and-messages): + Links to topic without a specified message now use the `with` + operator to follow moves of topics. + **Feature level 346** * [Markdown message formatting](/api/message-formatting#links-to-channels-topics-and-messages): diff --git a/api_docs/message-formatting.md b/api_docs/message-formatting.md index 226de60a4c..eedd5d08be 100644 --- a/api_docs/message-formatting.md +++ b/api_docs/message-formatting.md @@ -38,7 +38,7 @@ Sample HTML formats are as follows: + href="/#narrow/channel/9-announce/topic/Zulip.20updates/with/214"> #announce > Zulip updates @@ -47,9 +47,27 @@ Sample HTML formats are as follows: href="/#narrow/channel/9-announce/topic/Zulip.20updates/near/214"> #announce > Zulip updates @ 💬 + + + + #announce > Zulip updates + ``` -The older stream/topic elements include a `data-stream-id`, which +The `near` and `with` operators are documented in more detail in the +[search and URL documentation](/api/construct-narrow). When rendering +topic links with the `with` operator, the code doing the rendering may +pick the ID arbitrarily among messages accessible to the client and/or +acting user at the time of rendering. Currently, the server chooses +the message ID to use for `with` operators as the oldest message ID in +the topic accessible to the user who wrote the message. In channels +with protected history, this means the same Markdown syntax may be +rendered differently for users who joined at different times. + +The older stream/topic link elements include a `data-stream-id`, which historically was used in order to display the current channel name if the channel had been renamed. That field is **deprecated**, because displaying an updated value for the most common forms of this syntax @@ -64,7 +82,7 @@ are as follows: ```html + href="/#narrow/channel/9-announce/topic/with/214"> #announce > general chat @@ -73,9 +91,21 @@ are as follows: href="/#narrow/channel/9-announce/topic//near/214"> #announce > general chat @ 💬 + + + + #announce > general chat + ``` -**Changes**: Before Zulip 10.0 (feature level 346), empty string +**Changes**: Before Zulip 10.0 (feature level 347), the `with` field +was never used in topic link URLs generated by the server; the markup +currently used only for empty topics was used for all topic links. + +Before Zulip 10.0 (feature level 346), empty string was not a valid topic name in syntaxes for linking to topics and messages. diff --git a/version.py b/version.py index 4f738b312a..2dda916b0d 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 346 # Last bumped for topic="" support in link to topics/messages. +API_FEATURE_LEVEL = 347 # Last bumped for /with/ in topic links. # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/markdown/__init__.py b/zerver/lib/markdown/__init__.py index 7ff6d85dc4..a7a8f3c089 100644 --- a/zerver/lib/markdown/__init__.py +++ b/zerver/lib/markdown/__init__.py @@ -48,6 +48,7 @@ from zerver.lib.markdown import fenced_code from zerver.lib.markdown.fenced_code import FENCE_RE from zerver.lib.mention import ( BEFORE_MENTION_ALLOWED_REGEX, + ChannelTopicInfo, FullNameInfo, MentionBackend, MentionData, @@ -130,6 +131,7 @@ class DbData: active_realm_emoji: dict[str, EmojiInfo] sent_by_bot: bool stream_names: dict[str, int] + topic_info: dict[ChannelTopicInfo, int | None] translate_emoticons: bool user_upload_previews: dict[str, MarkdownImageMetadata] @@ -2049,6 +2051,13 @@ class StreamPattern(StreamTopicMessageProcessor): class StreamTopicPattern(StreamTopicMessageProcessor): + def get_with_operand(self, channel_topic: ChannelTopicInfo) -> int | None: + db_data: DbData | None = self.zmd.zulip_db_data + if db_data is None: + return None + with_operand = db_data.topic_info.get(channel_topic) + return with_operand + @override def handleMatch( # type: ignore[override] # https://github.com/python/mypy/issues/10197 self, m: Match[str], data: str @@ -2064,7 +2073,13 @@ class StreamTopicPattern(StreamTopicMessageProcessor): el.set("data-stream-id", str(stream_id)) stream_url = encode_stream(stream_id, stream_name) topic_url = hash_util_encode(topic_name) - link = f"/#narrow/channel/{stream_url}/topic/{topic_url}" + channel_topic_object = ChannelTopicInfo(stream_name, topic_name) + with_operand = self.get_with_operand(channel_topic_object) + if with_operand is not None: + link = f"/#narrow/channel/{stream_url}/topic/{topic_url}/with/{with_operand}" + else: + link = f"/#narrow/channel/{stream_url}/topic/{topic_url}" + el.set("href", link) if topic_name == "": @@ -2131,6 +2146,15 @@ def possible_linked_stream_names(content: str) -> set[str]: } +def possible_linked_topics(content: str) -> set[ChannelTopicInfo]: + # Here, we do not consider STREAM_TOPIC_MESSAGE_LINK_REGEX, since + # our callers only want to process links without a message ID. + return { + ChannelTopicInfo(match.group("stream_name"), match.group("topic_name")) + for match in re.finditer(STREAM_TOPIC_LINK_REGEX, content, re.VERBOSE) + } + + class AlertWordNotificationProcessor(markdown.preprocessors.Preprocessor): allowed_before_punctuation = {" ", "\n", "(", '"', ".", ",", "'", ";", "[", "*", "`", ">"} allowed_after_punctuation = { @@ -2753,6 +2777,9 @@ def do_convert( stream_names, acting_user=message_sender ) + linked_stream_topic_data = possible_linked_topics(content) + topic_info = mention_data.get_topic_info_map(linked_stream_topic_data) + if content_has_emoji_syntax(content): active_realm_emoji = get_name_keyed_dict_for_active_realm_emoji(message_realm.id) else: @@ -2766,6 +2793,7 @@ def do_convert( realm_url=message_realm.url, sent_by_bot=sent_by_bot, stream_names=stream_name_info, + topic_info=topic_info, translate_emoticons=translate_emoticons, user_upload_previews=user_upload_previews, ) diff --git a/zerver/lib/mention.py b/zerver/lib/mention.py index 1c77930deb..fceaf778e8 100644 --- a/zerver/lib/mention.py +++ b/zerver/lib/mention.py @@ -9,6 +9,7 @@ from django.db.models import Q from django_stubs_ext import StrPromise from zerver.lib.streams import filter_stream_authorization +from zerver.lib.topic import get_first_message_for_user_in_topic from zerver.lib.user_groups import get_root_id_annotated_recursive_subgroups_for_groups from zerver.lib.users import get_inaccessible_user_ids from zerver.models import NamedUserGroup, UserProfile @@ -67,6 +68,23 @@ class PossibleMentions: message_has_stream_wildcards: bool +@dataclass(frozen=True) +class ChannelTopicInfo: + channel_name: str + topic_name: str + + +@dataclass +class ChannelInfo: + channel_id: int + recipient_id: int + history_public_to_subscribers: bool + # TODO: Track whether the current user has only metadata access or + # content access, so that we can allow mentioning channels with + # only metadata access, while still enforcing content access to + # mention topics or messages within channels. + + class MentionBackend: # Be careful about reuse: MentionBackend contains caches which are # designed to only have the lifespan of a sender user (typically a @@ -78,7 +96,8 @@ class MentionBackend: def __init__(self, realm_id: int) -> None: self.realm_id = realm_id self.user_cache: dict[tuple[int, str], FullNameInfo] = {} - self.stream_cache: dict[str, int] = {} + self.stream_cache: dict[str, ChannelInfo] = {} + self.topic_cache: dict[ChannelTopicInfo, int | None] = {} def get_full_name_info_list( self, user_filters: list[UserFilter], message_sender: UserProfile | None @@ -152,7 +171,7 @@ class MentionBackend: for stream_name in stream_names: if stream_name in self.stream_cache: - result[stream_name] = self.stream_cache[stream_name] + result[stream_name] = self.stream_cache[stream_name].channel_id else: unseen_stream_names.append(stream_name) @@ -171,10 +190,14 @@ class MentionBackend: .values( "id", "name", + "recipient_id", + "history_public_to_subscribers", ) ) for row in rows: - self.stream_cache[row["name"]] = row["id"] + self.stream_cache[row["name"]] = ChannelInfo( + row["id"], row["recipient_id"], row["history_public_to_subscribers"] + ) result[row["name"]] = row["id"] else: authorization = filter_stream_authorization( @@ -189,11 +212,54 @@ class MentionBackend: is_subscribing_other_users=False, ) for stream in authorization.authorized_streams: - self.stream_cache[stream.name] = stream.id + assert stream.recipient_id is not None + self.stream_cache[stream.name] = ChannelInfo( + stream.id, stream.recipient_id, stream.history_public_to_subscribers + ) result[stream.name] = stream.id return result + def get_topic_info_map( + self, channel_topics: set[ChannelTopicInfo], acting_user: UserProfile | None + ) -> dict[ChannelTopicInfo, int | None]: + if not channel_topics: + return {} + + result: dict[ChannelTopicInfo, int | None] = {} + unseen_channel_topic: list[ChannelTopicInfo] = [] + + for channel_topic in channel_topics: + if channel_topic in self.topic_cache: + result[channel_topic] = self.topic_cache[channel_topic] + else: + unseen_channel_topic.append(channel_topic) + + for channel_topic in unseen_channel_topic: + channel_info = self.stream_cache.get(channel_topic.channel_name) + + if channel_info is None: + # The acting user does not have access to content in this channel. + continue + + recipient_id = channel_info.recipient_id + topic_name = channel_topic.topic_name + history_public_to_subscribers = channel_info.history_public_to_subscribers + + topic_latest_message = get_first_message_for_user_in_topic( + self.realm_id, + acting_user, + recipient_id, + topic_name, + history_public_to_subscribers, + acting_user_has_channel_content_access=True, + ) + + self.topic_cache[channel_topic] = topic_latest_message + result[channel_topic] = topic_latest_message + + return result + def user_mention_matches_topic_wildcard(mention: str) -> bool: return mention in topic_wildcards @@ -271,6 +337,7 @@ class MentionData: ) -> None: self.mention_backend = mention_backend realm_id = mention_backend.realm_id + self.message_sender = message_sender mentions = possible_mentions(content) possible_mentions_info = get_possible_mentions_info( mention_backend, mentions.mention_texts, message_sender @@ -356,7 +423,16 @@ class MentionData: def get_stream_name_map( self, stream_names: set[str], acting_user: UserProfile | None ) -> dict[str, int]: - return self.mention_backend.get_stream_name_map(stream_names, acting_user=acting_user) + return self.mention_backend.get_stream_name_map( + stream_names, acting_user=self.message_sender + ) + + def get_topic_info_map( + self, channel_topics: set[ChannelTopicInfo] + ) -> dict[ChannelTopicInfo, int | None]: + return self.mention_backend.get_topic_info_map( + channel_topics, acting_user=self.message_sender + ) def silent_mention_syntax_for_user(user_profile: UserProfile) -> str: diff --git a/zerver/lib/topic.py b/zerver/lib/topic.py index 8902b0c918..796f131bd1 100644 --- a/zerver/lib/topic.py +++ b/zerver/lib/topic.py @@ -78,6 +78,39 @@ def messages_for_topic( ) +def get_first_message_for_user_in_topic( + realm_id: int, + user_profile: UserProfile | None, + recipient_id: int, + topic_name: str, + history_public_to_subscribers: bool, + acting_user_has_channel_content_access: bool = False, +) -> int | None: + # Guard against incorrectly calling this function without + # first checking for channel access. + assert acting_user_has_channel_content_access + + if history_public_to_subscribers: + return ( + messages_for_topic(realm_id, recipient_id, topic_name) + .values_list("id", flat=True) + .first() + ) + + elif user_profile is not None: + return ( + UserMessage.objects.filter( + user_profile=user_profile, + message__recipient_id=recipient_id, + message__subject__iexact=topic_name, + ) + .values_list("message_id", flat=True) + .first() + ) + + return None + + def save_message_for_edit_use_case(message: Message) -> None: message.save( update_fields=[ diff --git a/zerver/tests/test_markdown.py b/zerver/tests/test_markdown.py index 0daccc9beb..c4289f113a 100644 --- a/zerver/tests/test_markdown.py +++ b/zerver/tests/test_markdown.py @@ -3007,7 +3007,7 @@ class MarkdownMentionTest(ZulipTestCase): assert_silent_mention("```quote\n@_*backend*\n```") -class MarkdownStreamMentionTests(ZulipTestCase): +class MarkdownStreamTopicMentionTests(ZulipTestCase): def test_stream_single(self) -> None: denmark = get_stream("Denmark", get_realm("zulip")) sender_user_profile = self.example_user("othello") @@ -3084,7 +3084,7 @@ class MarkdownStreamMentionTests(ZulipTestCase): "

#casesens

", ) - def test_topic_single(self) -> None: + def test_topic_single_containing_no_message(self) -> None: denmark = get_stream("Denmark", get_realm("zulip")) sender_user_profile = self.example_user("othello") msg = Message( @@ -3104,6 +3104,23 @@ class MarkdownStreamMentionTests(ZulipTestCase): f'

#{denmark.name} > {Message.EMPTY_TOPIC_FALLBACK_NAME}

', ) + def test_topic_single_containing_message(self) -> None: + denmark = get_stream("Denmark", get_realm("zulip")) + sender_user_profile = self.example_user("othello") + first_message_id = self.send_stream_message( + sender_user_profile, "Denmark", topic_name="some topic", content="test" + ) + msg = Message( + sender=sender_user_profile, + sending_client=get_client("test"), + realm=sender_user_profile.realm, + ) + content = "#**Denmark>some topic**" + self.assertEqual( + render_message_markdown(msg, content).rendered_content, + f'

#{denmark.name} > some topic

', + ) + def test_topic_atomic_string(self) -> None: realm = get_realm("zulip") # Create a linkifier. @@ -3119,17 +3136,23 @@ class MarkdownStreamMentionTests(ZulipTestCase): ) # Create a topic link that potentially interferes with the pattern. denmark = get_stream("Denmark", realm) + first_message_id = self.send_stream_message( + sender_user_profile, "Denmark", topic_name="#1234", content="test" + ) msg = Message(sender=sender_user_profile, sending_client=get_client("test"), realm=realm) content = "#**Denmark>#1234**" self.assertEqual( render_message_markdown(msg, content).rendered_content, - f'

#{denmark.name} > #1234

', + f'

#{denmark.name} > #1234

', ) def test_topic_multiple(self) -> None: denmark = get_stream("Denmark", get_realm("zulip")) scotland = get_stream("Scotland", get_realm("zulip")) sender_user_profile = self.example_user("othello") + first_message_id = self.send_stream_message( + sender_user_profile, "Denmark", topic_name="some topic", content="test" + ) msg = Message( sender=sender_user_profile, sending_client=get_client("test"), @@ -3140,7 +3163,7 @@ class MarkdownStreamMentionTests(ZulipTestCase): render_message_markdown(msg, content).rendered_content, "

This has two links: " f'' + f'href="/#narrow/channel/{denmark.id}-{denmark.name}/topic/some.20topic/with/{first_message_id}">' f"#{denmark.name} > some topic" " and " f'", ) + def test_topic_permalink(self) -> None: + realm = get_realm("zulip") + denmark = get_stream("Denmark", get_realm("zulip")) + sender_user_profile = self.example_user("othello") + first_message_id = self.send_stream_message( + sender_user_profile, "Denmark", topic_name="some topic", content="test" + ) + + msg = Message( + sender=sender_user_profile, + sending_client=get_client("test"), + realm=sender_user_profile.realm, + ) + + # test caching of topic data for user. + content = "#**Denmark>some topic**" + mention_backend = MentionBackend(realm.id) + mention_data = MentionData(mention_backend, content, message_sender=None) + render_message_markdown(msg, content, mention_data=mention_data) + + with ( + self.assert_database_query_count(1, keep_cache_warm=True), + self.assert_memcached_count(0), + ): + self.assertEqual( + render_message_markdown(msg, content, mention_data=mention_data).rendered_content, + f'

#{denmark.name} > some topic

', + ) + + # test topic linked doesn't have any message in it in case + # the topic mentioned doesn't have any messages. + content = "#**Denmark>random topic**" + self.assertEqual( + render_message_markdown(msg, content).rendered_content, + f'

#{denmark.name} > random topic

', + ) + + # Test when trying to render a topic link of a channel with shared + # history, if message_sender is None, topic link is permalink. + content = "#**Denmark>some topic**" + self.assertEqual( + markdown_convert_wrapper(content), + f'

#{denmark.name} > some topic

', + ) + + # test topic links for channel with protected history + core_stream = self.make_stream("core", realm, True, history_public_to_subscribers=False) + iago = self.example_user("iago") + hamlet = self.example_user("hamlet") + + self.subscribe(iago, "core") + msg_id = self.send_stream_message(iago, "core", topic_name="testing") + + msg = Message( + sender=iago, + sending_client=get_client("test"), + realm=realm, + ) + content = "#**core>testing**" + self.assertEqual( + render_message_markdown(msg, content).rendered_content, + f'

#{core_stream.name} > testing

', + ) + + # Test newly subscribed user to a channel with protected history + # won't have accessed to this message, and hence, the topic + # link would not be a permalink. + self.subscribe(hamlet, "core") + + msg = Message( + sender=hamlet, + sending_client=get_client("test"), + realm=realm, + ) + content = "#**core>testing**" + self.assertEqual( + render_message_markdown(msg, content).rendered_content, + f'

#{core_stream.name} > testing

', + ) + + # Test when trying to render a topic link of a channel with protected + # history, if message_sender is None, topic link is not permalink. + content = "#**core>testing**" + self.assertEqual( + markdown_convert_wrapper(content), + f'

#{core_stream.name} > testing

', + ) + def test_message_id_multiple(self) -> None: denmark = get_stream("Denmark", get_realm("zulip")) sender_user_profile = self.example_user("othello") diff --git a/zerver/tests/test_message_move_stream.py b/zerver/tests/test_message_move_stream.py index 5c7b78e63f..b09f273aaf 100644 --- a/zerver/tests/test_message_move_stream.py +++ b/zerver/tests/test_message_move_stream.py @@ -1201,7 +1201,7 @@ class MessageMoveStreamTest(ZulipTestCase): "iago", "test move stream", "new stream", "test" ) - with self.assert_database_query_count(57), self.assert_memcached_count(14): + with self.assert_database_query_count(59), self.assert_memcached_count(14): result = self.client_patch( f"/json/messages/{msg_id}", {