From 912e372c4eb2346198245502ac92a71c45e05883 Mon Sep 17 00:00:00 2001 From: Rohitt Vashishtha Date: Tue, 7 Jul 2020 02:31:38 +0530 Subject: [PATCH] markdown: Remove !avatar() and !gravatar() syntax. This particular commit has been a long time coming. For reference, !avatar(email) was an undocumented syntax that simply rendered an inline 50px avatar for a user in a message, essentially allowing you to create a user pill like: `!avatar(alice@example.com) Alice: hey!` --- Reimplementation If we decide to reimplement this or a similar feature in the future, we could use something like `` syntax which is more in line with creating links in markdown. Even then, it would not be a good idea to add this instead of supporting inline images directly. Since any usecases of such a syntax are in automation, we do not need to make it userfriendly and something like the following is a better implementation that doesn't need a custom syntax: `![avatar for Alice](/avatar/1234?s=50) Alice: hey!` --- History We initially added this syntax back in 2012 and it was 'deprecated' from the get go. Here's what the original commit had to say about the new syntax: > We'll use this internally for the commit bot. We might eventually > disable it for external users. We eventually did start using this for our github integrations in 2013 but since then, those integrations have been neglected in favor of our GitHub webhooks which do not use this syntax. When we copied `!gravatar` to add the `!avatar` syntax, we also noted that we want to deprecate the `!gravatar` syntax entirely - in 2013! Since then, we haven't advertised either of these syntaxes anywhere in our docs, and the only two places where this syntax remains is our game bots that could easily do without these, and the git commit integration that we have deprecated anyway. We do not have any evidence of someone asking about this syntax on chat.zulip.org when developing an integration and rightfully so- only the people who work on Zulip (and specifically, markdown) are likely to stumble upon it and try it out. This is also the only peice of code due to which we had to look up emails -> userid mapping in our backend markdown. By removing this, we entirely remove the backend markdown's dependency on user emails to render messages. --- Relevant commits: - Oct 2012, Initial commit c31462c2782a33886e737cf33424a36a95c81f97 - Nov 2013, Update commit bot 968c393826f8846065c5c880427328f6e534c2f5 - Nov 2013, Add avatar syntax 761c0a0266669aca82d134716a4d6b6e33d541fc - Sep 2017, Avoid email use c3032a7fe8ed49b011e0d242f4b8a7d756b9f647 - Apr 2019, Remove from webhook 674fcfcce1fcf35bdc57031a1025ef169d495d36 --- frontend_tests/node_tests/markdown.js | 8 --- static/js/markdown.js | 7 -- static/styles/rendered_markdown.scss | 7 -- static/third/marked/lib/marked.js | 33 +--------- templates/zerver/api/changelog.md | 5 ++ .../zerver/api/incoming-webhooks-overview.md | 2 +- version.py | 2 +- zerver/lib/markdown/__init__.py | 64 +------------------ .../tests/fixtures/markdown_test_cases.json | 24 ------- zerver/tests/test_markdown.py | 37 ----------- zproject/urls.py | 3 +- 11 files changed, 13 insertions(+), 179 deletions(-) diff --git a/frontend_tests/node_tests/markdown.js b/frontend_tests/node_tests/markdown.js index 486b3d11e7..171e3cdcdf 100644 --- a/frontend_tests/node_tests/markdown.js +++ b/frontend_tests/node_tests/markdown.js @@ -214,8 +214,6 @@ run_test('markdown_detection', () => { "User Mention @**leo with some name**", "Group Mention @*hamletcharacters*", "Stream #**Verona**", - "This contains !gravatar(leo@zulip.com)", - "And an avatar !avatar(leo@zulip.com) is here", ]; const markup = [ @@ -353,10 +351,6 @@ run_test('marked', () => { expected: '

A pattern written as #1234is not a realm filter.

'}, {input: 'This is a realm filter with ZGROUP_123:45 groups', expected: '

This is a realm filter with ZGROUP_123:45 groups

'}, - {input: 'This is an !avatar(cordelia@zulip.com) of Cordelia Lear', - expected: '

This is an cordelia@zulip.com of Cordelia Lear

'}, - {input: 'This is a !gravatar(cordelia@zulip.com) of Cordelia Lear', - expected: '

This is a cordelia@zulip.com of Cordelia Lear

'}, {input: 'Test *italic*', expected: '

Test italic

'}, {input: 'T\n#**Denmark**', @@ -396,8 +390,6 @@ run_test('marked', () => { expected: '

@**<h1>The Rogue One</h1>**

'}, {input: '#**

The Rogue One

**', expected: '

#**<h1>The Rogue One</h1>**

'}, - {input: '!avatar(

The Rogue One

)', - expected: '

<h1>The Rogue One</h1>

'}, {input: ':

The Rogue One

:', expected: '

:<h1>The Rogue One</h1>:

'}, {input: '@**O\'Connell**', diff --git a/static/js/markdown.js b/static/js/markdown.js index 537b8bed3c..829d1ac4bd 100644 --- a/static/js/markdown.js +++ b/static/js/markdown.js @@ -283,12 +283,6 @@ function handleEmoji(emoji_name) { return alt_text; } -function handleAvatar(email) { - return '' + email + ''; -} - function handleTimestamp(time) { let timeobject; if (isNaN(time)) { @@ -519,7 +513,6 @@ exports.initialize = function (realm_filters, helper_config) { smartypants: false, zulip: true, emojiHandler: handleEmoji, - avatarHandler: handleAvatar, unicodeEmojiHandler: handleUnicodeEmoji, streamHandler: handleStream, streamTopicHandler: handleStreamTopic, diff --git a/static/styles/rendered_markdown.scss b/static/styles/rendered_markdown.scss index 1d14258e9c..0462a84da3 100644 --- a/static/styles/rendered_markdown.scss +++ b/static/styles/rendered_markdown.scss @@ -134,13 +134,6 @@ background-color: hsla(102, 85%, 57%, 0.5); } - .message_body_gravatar { - width: 20px; - height: 20px; - margin: 2px 2px 2px 0px; - vertical-align: text-bottom; - } - /* Timestamps */ time { background: hsl(0, 0%, 93%); diff --git a/static/third/marked/lib/marked.js b/static/third/marked/lib/marked.js index b80e7fab18..12b69d4179 100644 --- a/static/third/marked/lib/marked.js +++ b/static/third/marked/lib/marked.js @@ -482,9 +482,7 @@ var inline = { usermention: noop, groupmention: noop, stream: noop, - avatar: noop, tex: noop, - gravatar: noop, timestamp: noop, realm_filters: [], text: /^[\s\S]+?(?=[\\]+)>([^\*]+)\*\*/, stream: /^#\*\*([^\*]+)\*\*/, - avatar: /^!avatar\(([^)]+)\)/, - gravatar: /^!gravatar\(([^)]+)\)/, tex: /^(\$\$([^\n_$](\\\$|[^\n$])*)\$\$(?!\$))\B/, timestamp: /^]+)>/, realm_filters: [], @@ -821,17 +817,10 @@ InlineLexer.prototype.output = function(src) { continue; } - // user avatar - if (cap = this.rules.avatar.exec(src)) { + // timestamp + if (cap = this.rules.timestamp.exec(src)) { src = src.substring(cap[0].length); - out += this.userAvatar(cap[1]); - continue; - } - - // user gravatar - if (cap = this.rules.gravatar.exec(src)) { - src = src.substring(cap[0].length); - out += this.userGravatar(cap[1]); + out += this.timestamp(cap[1]); continue; } @@ -895,20 +884,6 @@ InlineLexer.prototype.tex = function (tex, fullmatch) { return this.options.texHandler(tex, fullmatch); }; -InlineLexer.prototype.userAvatar = function (email) { - email = escape(email); - if (typeof this.options.avatarHandler !== 'function') - return '!avatar(' + email + ')'; - return this.options.avatarHandler(email); -}; - -InlineLexer.prototype.userGravatar = function (email) { - email = escape(email); - if (typeof this.options.avatarHandler !== 'function') - return '!gravatar(' + email + ')'; - return this.options.avatarHandler(email); -}; - InlineLexer.prototype.timestamp = function (time) { if (typeof this.options.timestampHandler !== 'function') return '<time:' + time + '>'; @@ -1537,8 +1512,6 @@ marked.defaults = { gfm: true, emoji: false, unicodeemoji: false, - avatar: false, - gravatar: false, timestamp: true, tables: true, breaks: false, diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 41e865f444..365fc03d75 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -10,6 +10,11 @@ below features are supported. ## Changes in Zulip 3.0 +**Feature level 24** + +* The `!avatar()` and `!gravatar()` markdown syntax, which was never + documented and rarely used, was removed. + **Feature level 23** * `GET/PUT/POST /users/me/pointer`: Eliminated. We eliminated diff --git a/templates/zerver/api/incoming-webhooks-overview.md b/templates/zerver/api/incoming-webhooks-overview.md index 30d85e5949..49cb348a7f 100644 --- a/templates/zerver/api/incoming-webhooks-overview.md +++ b/templates/zerver/api/incoming-webhooks-overview.md @@ -109,7 +109,7 @@ below are for a webhook named `MyWebHook`. * Consider using our Zulip markup to make the output from your integration especially attractive or useful (e.g. emoji, markdown - emphasis, @-mentions, or `!avatar(email)`). + emphasis or @-mentions). * Use topics effectively to ensure sequential messages about the same thing are threaded together; this makes for much better consumption diff --git a/version.py b/version.py index d899a72bc3..bc8a6ed23c 100644 --- a/version.py +++ b/version.py @@ -29,7 +29,7 @@ DESKTOP_WARNING_VERSION = "5.2.0" # # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md. -API_FEATURE_LEVEL = 23 +API_FEATURE_LEVEL = 24 # 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 7a646c74d1..bf84d0677b 100644 --- a/zerver/lib/markdown/__init__.py +++ b/zerver/lib/markdown/__init__.py @@ -103,8 +103,6 @@ version = 1 _T = TypeVar('_T') ElementStringNone = Union[Element, Optional[str]] -AVATAR_REGEX = r'!avatar\((?P[^)]*)\)' -GRAVATAR_REGEX = r'!gravatar\((?P[^)]*)\)' EMOJI_REGEX = r'(?P:[\w\-\+]+:)' def verbose_compile(pattern: str) -> Any: @@ -1198,35 +1196,6 @@ class InlineInterestingLinkProcessor(markdown.treeprocessors.Treeprocessor): if title: found_url.family.child.text = title -class Avatar(markdown.inlinepatterns.Pattern): - def handleMatch(self, match: Match[str]) -> Optional[Element]: - img = Element('img') - email_address = match.group('email') - email = email_address.strip().lower() - profile_id = None - - db_data = self.md.zulip_db_data - if db_data is not None: - user_dict = db_data['email_info'].get(email) - if user_dict is not None: - profile_id = user_dict['id'] - - img.set('class', 'message_body_gravatar') - img.set('src', f'/avatar/{profile_id or email}?s=30') - img.set('title', email) - img.set('alt', email) - return img - -def possible_avatar_emails(content: str) -> Set[str]: - emails = set() - for REGEX in [AVATAR_REGEX, GRAVATAR_REGEX]: - matches = re.findall(REGEX, content) - for email in matches: - if email: - emails.add(email) - - return emails - class Timestamp(markdown.inlinepatterns.Pattern): def handleMatch(self, match: Match[str]) -> Optional[Element]: time_input_string = match.group('time') @@ -1940,10 +1909,7 @@ class Markdown(markdown.Markdown): reg.register(Tex(r'\B(?[^\n_$](\\\$|[^$\n])*)\$\$(?!\$)\B'), 'tex', 90) reg.register(StreamTopicPattern(get_compiled_stream_topic_link_regex(), self), 'topic', 87) reg.register(StreamPattern(get_compiled_stream_link_regex(), self), 'stream', 85) - reg.register(Avatar(AVATAR_REGEX, self), 'avatar', 80) reg.register(Timestamp(r'[^>]*?)>'), 'timestamp', 75) - # Note that !gravatar syntax should be deprecated long term. - reg.register(Avatar(GRAVATAR_REGEX, self), 'gravatar', 70) reg.register(UserGroupMentionPattern(mention.user_group_mentions, self), 'usergroupmention', 65) reg.register(LinkInlineProcessor(markdown.inlinepatterns.LINK_RE, self), 'link', 60) reg.register(AutoLink(get_web_link_regex(), self), 'autolink', 55) @@ -2107,30 +2073,6 @@ _privacy_re = re.compile('\\w', flags=re.UNICODE) def privacy_clean_markdown(content: str) -> str: return repr(_privacy_re.sub('x', content)) -def get_email_info(realm_id: int, emails: Set[str]) -> Dict[str, FullNameInfo]: - if not emails: - return dict() - - q_list = { - Q(email__iexact=email.strip().lower()) - for email in emails - } - - rows = UserProfile.objects.filter( - realm_id=realm_id, - ).filter( - functools.reduce(lambda a, b: a | b, q_list), - ).values( - 'id', - 'email', - ) - - dct = { - row['email'].strip().lower(): row - for row in rows - } - return dct - def get_possible_mentions_info(realm_id: int, mention_texts: Set[str]) -> List[FullNameInfo]: if not mention_texts: return list() @@ -2318,7 +2260,7 @@ def do_convert(content: str, if message_realm is not None: # Here we fetch the data structures needed to render - # mentions/avatars/stream mentions from the database, but only + # mentions/stream mentions from the database, but only # if there is syntax in the message that might use them, since # the fetches are somewhat expensive and these types of syntax # are uncommon enough that it's a useful optimization. @@ -2326,9 +2268,6 @@ def do_convert(content: str, if mention_data is None: mention_data = MentionData(message_realm.id, content) - emails = possible_avatar_emails(content) - email_info = get_email_info(message_realm.id, emails) - stream_names = possible_linked_stream_names(content) stream_name_info = get_stream_name_info(message_realm, stream_names) @@ -2339,7 +2278,6 @@ def do_convert(content: str, _md_engine.zulip_db_data = { 'realm_alert_words_automaton': realm_alert_words_automaton, - 'email_info': email_info, 'mention_data': mention_data, 'active_realm_emoji': active_realm_emoji, 'realm_uri': message_realm.uri, diff --git a/zerver/tests/fixtures/markdown_test_cases.json b/zerver/tests/fixtures/markdown_test_cases.json index 17a1c80763..c9b8a08e26 100644 --- a/zerver/tests/fixtures/markdown_test_cases.json +++ b/zerver/tests/fixtures/markdown_test_cases.json @@ -722,30 +722,6 @@ "expected_output": "

gonna take a break for a bit--all yours if you want to play around too. what I did:

\n
    \n
  • install cmake
    • \n
  • git clone zulip desktop
    • \n
  • run cmake-gui .. in c:\\zulip\\zulip-desktop\\msvcbuild
    • \n
  • hit configure/generate until it generated the msvc project (had to make a fix to some cmake files)
    • \n
  • opened vs2013
    • \n
  • tried to build
    • \n
", "text_content": "gonna take a break for a bit--all yours if you want to play around too. what I did:\n\ninstall cmake\ngit clone zulip desktop\nrun cmake-gui .. in c:\\zulip\\zulip-desktop\\msvcbuild\nhit configure\/generate until it generated the msvc project (had to make a fix to some cmake files)\nopened vs2013 \ntried to build\n" }, - { - "name": "avatar", - "input": "!avatar(username@example.com)", - "expected_output": "

\"username@example.com\"

", - "text_content": "username@example.com" - }, - { - "name": "gravatar", - "input": "!gravatar(username@example.com)", - "expected_output": "

\"username@example.com\"

", - "text_content": "username@example.com" - }, - { - "name": "avatar_escaped", - "input": "`!avatar(username@example.com)`", - "expected_output": "

!avatar(username@example.com)

", - "text_content": "!avatar(username@example.com)" - }, - { - "name": "gravatar_escaped", - "input": "`!gravatar(username@example.com)`", - "expected_output": "

!gravatar(username@example.com)

", - "text_content": "!gravatar(username@example.com)" - }, { "name": "timestamp_backend_markdown_only", "input": "", diff --git a/zerver/tests/test_markdown.py b/zerver/tests/test_markdown.py index 28c6a3b3f2..03ecfc1eca 100644 --- a/zerver/tests/test_markdown.py +++ b/zerver/tests/test_markdown.py @@ -30,7 +30,6 @@ from zerver.lib.markdown import ( image_preview_enabled, markdown_convert, maybe_update_markdown_engines, - possible_avatar_emails, possible_linked_stream_names, topic_links, url_embed_preview_enabled, @@ -2166,39 +2165,3 @@ class MarkdownErrorTests(ZulipTestCase): result = processor.run(markdown_input) self.assertEqual(result, expected) - -class MarkdownAvatarTestCase(ZulipTestCase): - def test_possible_avatar_emails(self) -> None: - content = ''' - hello !avatar(foo@example.com) my email is ignore@ignore.com - !gravatar(bar@yo.tv) - - smushing!avatar(hamlet@example.org) is allowed - ''' - self.assertEqual( - possible_avatar_emails(content), - {'foo@example.com', 'bar@yo.tv', 'hamlet@example.org'}, - ) - - def test_avatar_with_id(self) -> None: - sender_user_profile = self.example_user('othello') - message = Message(sender=sender_user_profile, sending_client=get_client("test")) - - user_profile = self.example_user('hamlet') - msg = f'!avatar({user_profile.email})' - converted = markdown_convert(msg, message=message) - values = {'email': user_profile.email, 'id': user_profile.id} - self.assertEqual( - converted, - '

{email}

'.format(**values)) - - def test_avatar_of_unregistered_user(self) -> None: - sender_user_profile = self.example_user('othello') - message = Message(sender=sender_user_profile, sending_client=get_client("test")) - - email = 'fakeuser@example.com' - msg = f'!avatar({email})' - converted = markdown_convert(msg, message=message) - self.assertEqual( - converted, - '

{0}

'.format(email)) diff --git a/zproject/urls.py b/zproject/urls.py index f3b0a06a3d..6f55cf4ac0 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -621,7 +621,8 @@ urls += [ path('thumbnail', rest_dispatch, {'GET': ('zerver.views.thumbnail.backend_serve_thumbnail', {'override_api_url_scheme'})}), - # Avatars have the same constraint due to `!avatar` syntax. + # Avatars have the same constraint because their URLs are included + # in API data structures used by both the mobile and web clients. re_path(r'^avatar/(?P[\S]+)/(?P[\S]+)?$', rest_dispatch, {'GET': ('zerver.views.users.avatar',