From e08bf15682d8dbf05d1fa221b253807369547bb4 Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Tue, 28 Jan 2025 14:43:36 +0530 Subject: [PATCH] stream_topic_link: Add support for empty string topic in syntax. This commit adds support for empty string as a valid topic name in syntax for linking to topics. The server stores it after empty string is replaced with `realm_empty_topic_display_name` and wrapped with an tag. The web client parses the rendered_content and updates the topic_name part in the HTML with topic_name in user's language + wraps it in a tag with 'empty-topic-display' css class. --- web/src/markdown.ts | 7 ++- web/src/rendered_markdown.ts | 39 +++++++++++------ web/templates/topic_link.hbs | 15 ++++++- web/tests/markdown.test.cjs | 11 ++--- web/tests/rendered_markdown.test.cjs | 64 ++++++++++++++++++++++++++-- web/third/marked/lib/marked.cjs | 2 +- zerver/lib/markdown/__init__.py | 14 ++++-- zerver/tests/test_markdown.py | 6 +++ 8 files changed, 127 insertions(+), 31 deletions(-) diff --git a/web/src/markdown.ts b/web/src/markdown.ts index e8d5ae9d52..673d95a118 100644 --- a/web/src/markdown.ts +++ b/web/src/markdown.ts @@ -10,6 +10,8 @@ import render_topic_link from "../templates/topic_link.hbs"; import marked from "../third/marked/lib/marked.cjs"; import type {LinkifierMatch, ParseOptions, RegExpOrStub} from "../third/marked/lib/marked.cjs"; +import * as util from "./util.ts"; + // This contains zulip's frontend Markdown implementation; see // docs/subsystems/markdown.md for docs on our Markdown syntax. The other // main piece in rendering Markdown client-side is @@ -639,14 +641,15 @@ function handleStreamTopic({ stream_topic_hash: (stream_id: number, topic: string) => string; }): string | undefined { const stream = get_stream_by_name(stream_name); - if (stream === undefined || !topic) { + if (stream === undefined) { return undefined; } const href = stream_topic_hash(stream.stream_id, topic); return render_topic_link({ channel_id: stream.stream_id, channel_name: stream.name, - topic_display_name: topic, + topic_display_name: util.get_final_topic_display_name(topic), + is_empty_string_topic: topic === "", href, }); } diff --git a/web/src/rendered_markdown.ts b/web/src/rendered_markdown.ts index 24a7b2ee4d..1ba35ecf92 100644 --- a/web/src/rendered_markdown.ts +++ b/web/src/rendered_markdown.ts @@ -6,9 +6,11 @@ import assert from "minimalistic-assert"; import code_buttons_container from "../templates/code_buttons_container.hbs"; import render_markdown_timestamp from "../templates/markdown_timestamp.hbs"; import render_mention_content_wrapper from "../templates/mention_content_wrapper.hbs"; +import render_topic_link from "../templates/topic_link.hbs"; import * as blueslip from "./blueslip.ts"; import {show_copied_confirmation} from "./copied_tooltip.ts"; +import * as hash_util from "./hash_util.ts"; import {$t} from "./i18n.ts"; import * as message_store from "./message_store.ts"; import type {Message} from "./message_store.ts"; @@ -234,20 +236,29 @@ export const update_elements = ($content: JQuery): void => { }); $content.find("a.stream-topic").each(function (): void { - const stream_id_string = $(this).attr("data-stream-id"); - assert(stream_id_string !== undefined); - const stream_id = Number.parseInt(stream_id_string, 10); - if (stream_id && $(this).find(".highlight").length === 0) { - // Display the current name for stream if it is not - // being displayed in search highlight. - const stream_name = sub_store.maybe_get_stream_name(stream_id); - if (stream_name !== undefined) { - // If the stream has been deleted, - // sub_store.maybe_get_stream_name might return - // undefined. Otherwise, display the current stream name. - const text = $(this).text(); - $(this).text("#" + stream_name + text.slice(text.indexOf(" > "))); - } + const narrow_url = $(this).attr("href"); + assert(narrow_url !== undefined); + const channel_topic = hash_util.decode_stream_topic_from_url( + window.location.origin + narrow_url, + ); + assert(channel_topic !== null); + const channel_name = sub_store.maybe_get_stream_name(channel_topic.stream_id); + if (channel_name !== undefined && $(this).find(".highlight").length === 0) { + // Display the current channel name if it hasn't been deleted + // and not being displayed in search highlight. + // TODO: Ideally, we should NOT skip this if only topic is highlighted, + // but we are doing so currently. + const topic_name = channel_topic.topic_name; + assert(topic_name !== undefined); + const topic_display_name = util.get_final_topic_display_name(topic_name); + const topic_link_html = render_topic_link({ + channel_id: channel_topic.stream_id, + channel_name, + topic_display_name, + is_empty_string_topic: topic_name === "", + href: narrow_url, + }); + $(this).replaceWith($(topic_link_html)); } }); diff --git a/web/templates/topic_link.hbs b/web/templates/topic_link.hbs index 300d37ffcf..46bc8e2fb0 100644 --- a/web/templates/topic_link.hbs +++ b/web/templates/topic_link.hbs @@ -1,2 +1,13 @@ -#{{channel_name}} > {{topic_display_name}} -{{~!-- squash whitespace --~}} +{{#if is_empty_string_topic}} + + {{~!-- squash whitespace --~}} + #{{channel_name}} > {{topic_display_name}} + {{~!-- squash whitespace --~}} + +{{~else}} + + {{~!-- squash whitespace --~}} + #{{channel_name}} > {{topic_display_name}} + {{~!-- squash whitespace --~}} + +{{~/if}} diff --git a/web/tests/markdown.test.cjs b/web/tests/markdown.test.cjs index 90f854ffae..7dbacf239a 100644 --- a/web/tests/markdown.test.cjs +++ b/web/tests/markdown.test.cjs @@ -60,7 +60,8 @@ const stream_data = zrequire("stream_data"); const user_groups = zrequire("user_groups"); const {initialize_user_settings} = zrequire("user_settings"); -set_realm({}); +const REALM_EMPTY_TOPIC_DISPLAY_NAME = "general chat"; +set_realm({realm_empty_topic_display_name: REALM_EMPTY_TOPIC_DISPLAY_NAME}); const user_settings = {}; initialize_user_settings({user_settings}); @@ -411,15 +412,15 @@ test("marked", ({override}) => { expected: '

This is a #Denmark > some topic stream_topic link

', }, + { + input: "This is a #**Denmark>** stream_topic link with empty string topic.", + expected: `

This is a #Denmark > translated: ${REALM_EMPTY_TOPIC_DISPLAY_NAME} stream_topic link with empty string topic.

`, + }, { input: "This has two links: #**Denmark>some topic** and #**social>other topic**.", expected: '

This has two links: #Denmark > some topic and #social > other topic.

', }, - { - input: "This is not a #**Denmark>** stream_topic link", - expected: "

This is not a #**Denmark>** stream_topic link

", - }, { input: "Look at #**Denmark>message_link@100**", expected: diff --git a/web/tests/rendered_markdown.test.cjs b/web/tests/rendered_markdown.test.cjs index ad4a302bf8..787f989fd8 100644 --- a/web/tests/rendered_markdown.test.cjs +++ b/web/tests/rendered_markdown.test.cjs @@ -35,7 +35,8 @@ mock_esm("../src/settings_data", { const {set_realm} = zrequire("state_data"); const {initialize_user_settings} = zrequire("user_settings"); -const realm = {}; +const REALM_EMPTY_TOPIC_DISPLAY_NAME = "general chat"; +const realm = {realm_empty_topic_display_name: REALM_EMPTY_TOPIC_DISPLAY_NAME}; set_realm(realm); const user_settings = {}; initialize_user_settings({user_settings}); @@ -407,19 +408,33 @@ run_test("user-group-mention (error)", () => { assert.ok(!$group.hasClass("user-mention-me")); }); -run_test("stream-links", () => { +run_test("stream-links", ({mock_template}) => { // Setup const $content = get_content_element(); const $stream = $.create("a.stream"); $stream.set_find_results(".highlight", false); $stream.attr("data-stream-id", stream.stream_id); + const $stream_topic = $.create("a.stream-topic"); $stream_topic.set_find_results(".highlight", false); - $stream_topic.attr("data-stream-id", stream.stream_id); + $stream_topic.attr( + "href", + `/#narrow/channel/${stream.stream_id}-random/topic/topic.20name.20.3E.20still.20the.20topic.20name`, + ); + $stream_topic.replaceWith = noop; $stream_topic.text("#random > topic name > still the topic name"); + $content.set_find_results("a.stream", $array([$stream])); $content.set_find_results("a.stream-topic", $array([$stream_topic])); + let topic_link_context; + let topic_link_rendered_html; + mock_template("topic_link.hbs", true, (data, html) => { + topic_link_context = data; + topic_link_rendered_html = html; + return html; + }); + // Initial asserts assert.equal($stream.text(), "never-been-set"); assert.equal($stream_topic.text(), "#random > topic name > still the topic name"); @@ -428,7 +443,48 @@ run_test("stream-links", () => { // Final asserts assert.equal($stream.text(), `#${stream.name}`); - assert.equal($stream_topic.text(), `#${stream.name} > topic name > still the topic name`); + assert.deepEqual(topic_link_context, { + channel_id: stream.stream_id, + channel_name: stream.name, + topic_display_name: "topic name > still the topic name", + is_empty_string_topic: false, + href: `/#narrow/channel/${stream.stream_id}-random/topic/topic.20name.20.3E.20still.20the.20topic.20name`, + }); + assert.ok(!topic_link_rendered_html.includes("empty-topic-display")); +}); + +run_test("topic-link (empty string topic)", ({mock_template}) => { + // Setup + const $content = get_content_element(); + const $channel_topic = $.create("a.stream-topic(empty-string-topic)"); + $channel_topic.set_find_results(".highlight", false); + $channel_topic.attr("href", `/#narrow/channel/${stream.stream_id}-random/topic/`); + $channel_topic.replaceWith = noop; + $channel_topic.html(`#random > ${REALM_EMPTY_TOPIC_DISPLAY_NAME}`); + $content.set_find_results("a.stream-topic", $array([$channel_topic])); + + let topic_link_context; + let topic_link_rendered_html; + mock_template("topic_link.hbs", true, (data, html) => { + topic_link_context = data; + topic_link_rendered_html = html; + return html; + }); + + // Initial assert + assert.equal($channel_topic.html(), "#random > general chat"); + + rm.update_elements($content); + + // Final assert + assert.deepEqual(topic_link_context, { + channel_id: stream.stream_id, + channel_name: stream.name, + topic_display_name: `translated: ${REALM_EMPTY_TOPIC_DISPLAY_NAME}`, + is_empty_string_topic: true, + href: `/#narrow/channel/${stream.stream_id}-random/topic/`, + }); + assert.ok(topic_link_rendered_html.includes("empty-topic-display")); }); run_test("timestamp without time", () => { diff --git a/web/third/marked/lib/marked.cjs b/web/third/marked/lib/marked.cjs index e0fbe9349d..38e579fc02 100644 --- a/web/third/marked/lib/marked.cjs +++ b/web/third/marked/lib/marked.cjs @@ -546,7 +546,7 @@ inline.zulip = merge({}, inline.breaks, { usermention: /^@(_?)(?:\*\*([^\*]+)\*\*)/, // Match potentially multi-word string between @** ** groupmention: /^@(_?)(?:\*([^\*]+)\*)/, // Match multi-word string between @* * stream_topic_message: /^#\*\*([^\*>]+)>([^\*]+)@(\d+)\*\*/, - stream_topic: /^#\*\*([^\*>]+)>([^\*]+)\*\*/, + stream_topic: /^#\*\*([^\*>]+)>([^\*]*)\*\*/, stream: /^#\*\*([^\*]+)\*\*/, tex: /^(\$\$([^\n_$](\\\$|[^\n$])*)\$\$(?!\$))\B/, timestamp: /^]+)>/, diff --git a/zerver/lib/markdown/__init__.py b/zerver/lib/markdown/__init__.py index 4c2be3f37b..fd13d5705e 100644 --- a/zerver/lib/markdown/__init__.py +++ b/zerver/lib/markdown/__init__.py @@ -177,7 +177,7 @@ STREAM_TOPIC_LINK_REGEX = rf""" \#\*\* # and after hash sign followed by double asterisks (?P[^\*>]+) # stream name can contain anything except > > # > acts as separator - (?P[^\*]+) # topic name can contain anything + (?P[^\*]*) # topic name can be an empty string or contain anything \*\* # ends by double asterisks """ @@ -2066,8 +2066,16 @@ class StreamTopicPattern(StreamTopicMessageProcessor): topic_url = hash_util_encode(topic_name) link = f"/#narrow/channel/{stream_url}/topic/{topic_url}" el.set("href", link) - text = f"#{stream_name} > {topic_name}" - el.text = markdown.util.AtomicString(text) + + if topic_name == "": + topic_el = Element("em") + topic_el.text = Message.EMPTY_TOPIC_FALLBACK_NAME + el.text = markdown.util.AtomicString(f"#{stream_name} > ") + el.append(topic_el) + else: + text = f"#{stream_name} > {topic_name}" + el.text = markdown.util.AtomicString(text) + return el, m.start(), m.end() diff --git a/zerver/tests/test_markdown.py b/zerver/tests/test_markdown.py index 65db8a5dea..2dc7c24f58 100644 --- a/zerver/tests/test_markdown.py +++ b/zerver/tests/test_markdown.py @@ -3097,6 +3097,12 @@ class MarkdownStreamMentionTests(ZulipTestCase): render_message_markdown(msg, content).rendered_content, f'

#{denmark.name} > some topic

', ) + # Empty string as topic name. + content = "#**Denmark>**" + self.assertEqual( + render_message_markdown(msg, content).rendered_content, + f'

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

', + ) def test_topic_atomic_string(self) -> None: realm = get_realm("zulip")