Support "file:///" as hyperlink #508
Conversation
|
Automated message from Dropbox CLA bot @kevchentw, it looks like you've already signed the Dropbox CLA. Thanks! |
|
Travis CI seems to failed often? |
|
Travis CI is having some issues with its apt mirrors, and thus everyone's Travis CI builds are failing somewhat often right now. For Zulip it's mostly the "production" tests, which tests the dependencies and installation process, since that makes heavy use of apt... |
|
Should I reopened the pr to run the CI again? |
| @@ -634,7 +634,6 @@ def sanitize_url(url): | |||
| except ValueError: | |||
| # Bad url - so bad it couldn't be parsed. | |||
| return '' | |||
|
|
|||
There was a problem hiding this comment.
probably should remove this whitespace change from the commit.
|
Well, this change needs tests for the new behavior, so why don't you add those, and when you push your updates to add tests, it'll rerun the CI anyway. |
|
(tests probably belong in |
| @@ -159,6 +159,7 @@ def get_secret(key): | |||
| 'REMOTE_POSTGRES_SSLMODE': '', | |||
| 'GOOGLE_CLIENT_ID': '', | |||
| 'DBX_APNS_CERT_FILE': None, | |||
| 'ENABLE_FILE_LINKS' : False, | |||
There was a problem hiding this comment.
You should also add a patch to zproject/local_settings_template.py documenting the feature and providing a clear place to set this new setting.
There was a problem hiding this comment.
cool, looks like you did this one.
| @@ -999,7 +999,7 @@ def content_has_image(content): | |||
|
|
|||
| @staticmethod | |||
| def content_has_link(content): | |||
| return 'http://' in content or 'https://' in content or '/user_uploads' in content | |||
| return 'http://' in content or 'https://' in content or '/user_uploads' in content or 'file:///' in content | |||
There was a problem hiding this comment.
hmm probably this or case should be conditional on settings.ENABLE_FILE_LINKS as well, right?
|
Thanks @kevchentw, and sorry for the slow review! I posted a couple more comments on this, and also asked the original reporter to take a look and see if this does what they want. Finally, you should squash the commits to match the Zulip commit style (http://zulip.readthedocs.org/en/latest/code-style.html#version-control) Oh, and update the commit message summary to make clear this is a new setting. |
| @@ -112,6 +112,10 @@ | |||
| # a link to an image is referenced in a message. | |||
| INLINE_IMAGE_PREVIEW = True | |||
|
|
|||
| # Controls whether or not Zulip will parse links starting with | |||
| # "file:///" as a hyperlink. | |||
| ENABLE_FILE_LINKS = False | |||
There was a problem hiding this comment.
Maybe expand this comment a bit more to discuss the use case, e.g.
# Controls whether or not Zulip will parse links starting with
# "file:///" as a hyperlink (useful if you have e.g. an NFS share).
|
@kevchentw just wanted to follow up on this pull request! |
|
@kevchentw How is this pull request going? Are there questions you'd perhaps like to talk about in realtime? We could chat about it during our office hours on October 3rd. |
|
This was merged as #2216. |
fixed #380
Add the support of
file:///as hyperlink. It is default to be off.Enable by setting
ENABLE_FILE_LINKS = Trueinzproject/settings.pyNotice that the browser will block the link start with
file:///and need to install extension to fix it.