Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

markdown: Remove unnecessary checks for zulip_message. #19553

Merged
merged 1 commit into from Aug 31, 2021

Conversation

garg3133
Copy link
Member

This commits removes some unnecessary checks for self.md.zulip_message,
which were put there historically, as earlier we used to add the additional
properties like mentions_user_ids, alert_words, etc. to Message dict
only. These were later moved to MessageRenderingResult class in commit
75cea32 but the checks weren't removed.

This is important because while rendering the messages imported from
other chat tools (like Rocket.Chat), the Message dict is not passed to
the markdown, due to which the checks for self.md.zerver_message fails
and hence, things like user mentions, stream/topic mentions are not
rendered in the imported messages properly.

This commits removes some unnecessary checks for `self.md.zulip_message`,
which were put there historically, as earlier we used to add the additional
properties like mentions_user_ids, alert_words, etc. to Message dict
only. These were later moved to MessageRenderingResult class in commit
75cea32 but the checks weren't removed.

This is important because while rendering the messages imported from
other chat tools (like Rocket.Chat), the Message dict is not passed to
the markdown, due to which the checks for `self.md.zerver_message` fails
and hence, things like user mentions, stream/topic mentions are not
rendered in the imported messages properly.
@garg3133
Copy link
Member Author

Other than the messages, I think these changes will also affect the markdown in realm//stream descriptions. But I think we should support realm emojis, stream-topic mentions and maybe even user mentions in realm/stream descriptions also.

Other than realm/stream descriptions, I couldn't find any other place where we use this markdown processor. Is there any?

@garg3133
Copy link
Member Author

@PIG208 Can you please take a look at this PR? And also at this CZO topic so that I can proceed further with that.

@PIG208
Copy link
Member

PIG208 commented Aug 14, 2021

I have looked at the PR. I wasn't aware of the possibility of extending the markdown processor during the refactor.

However, I'm not sure if directly rendering stream/user mentions or realm-specific emoji from a third-part platform would work just fine. We support this feature with this converter in zerver.lib.import_realm.fix_message_rendered_content, where we update the render_content of the message records directly.

For supporting stream/user mentions and etc. in descriptions, I think we will need to add some tests to validate this. Probably in zerver/tests/test_markdown.py?

@PIG208
Copy link
Member

PIG208 commented Aug 14, 2021

Regarding the CZO issue, I guess the right direction for us here might be completely remove it from the markdown processor.

@garg3133
Copy link
Member Author

However, I'm not sure if directly rendering stream/user mentions or realm-specific emoji from a third-part platform would work just fine.

We actually convert the markdown while importing messages from other chat platforms to Zulip, so that it's compatible with Zulip markdown (like converting @user to @**user**), but the problem is that since we do not pass a Message instance while calling the markdown converter in zerver.lib.import_realm.fix_message_rendered_content, the message is not rendered properly (user mentions are not linked to users, stream/topic mentions are not linked to corresponding stream/topic, etc.).

But after your refactor in 75cea32, I think we don't actually need to check if self.md.zulip_message is present or not, as we no longer use it for carrying other properties. And if we can remove it, our problems with the markdown render of messages imported from other chat softwares will be solved.

So, I was just wondering if there would be any other consequences of removing checks for self.md.zulip_message, apart from these things being rendered in realm/stream descriptions, which is a topic of separate discussion?

@PIG208
Copy link
Member

PIG208 commented Aug 16, 2021

I don't think there is any other than zerver.lib.import_realm.fix_message_rendered_content, zerver.lib.realm_description.get_realm_rendered_description and zerver.lib.streams.render_stream_description. AFAIK, these are the only code paths where we invoke the convertor without the message object.

@timabbott timabbott merged commit 1e51c23 into zulip:master Aug 31, 2021
@timabbott
Copy link
Sponsor Member

This lgtm, merged, thanks @garg3133 and @PIG208!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants