models: Add `render_format_string` field in `RealmFilter` model. #18914

akshatdalton · 2021-06-18T20:46:50Z

Added some minor fixes following them I've added new changes to add the render_format_string field in the RealmFilter model. Then new changes are done in a series of commits; please do tell me if this needs some changes in the commit structure.

The new changes will help the user set the rendered text of the detected linkifier pattern if render_format_string is provided; else, the detected pattern text will be used to set the rendered text (like we do now). Please note that this still can't perform the Link Shortener task (Related issue: #17971) as I've not modified the linkifier pattern regex, which is used for validation purpose. I'll do so as part of my next commit, which will help to close the issue #17971.

akshatdalton · 2021-06-18T20:52:57Z

zerver/models.py

@@ -953,6 +953,16 @@ class RealmFilter(models.Model):
    realm: Realm = models.ForeignKey(Realm, on_delete=CASCADE)
    pattern: str = models.TextField()
    url_format_string: str = models.TextField(validators=[URLValidator(), filter_format_validator])
+    render_format_string: str = models.TextField(
+        default="",


I chose the default="" and not null=True as this may create some issue when the linkifier data will be used for comparison purpose (@ frontend), or in other cases, this may cause some unexpected problems like errors are thrown (@ frontend).

Can you explain the reasoning more, perhaps with an example problem you'd expect with null=True?

Oh, is the point just that "" is a simpler value to set in a web form?

We probably need to put some unnecessary check at (when null=True is used):

zulip/static/js/settings_linkifiers.js

Lines 103 to 104 in 4cff56a

item.pattern.toLowerCase().includes(value) ||

item.url_format.toLowerCase().includes(value)

.

zulip/static/js/settings_linkifiers.js

Line 30 in 4cff56a

if (x > y) {

.

Hence I found that empty strings would be much better rather than providing unnecessary checks.

timabbott · 2021-06-18T22:49:15Z

I merged the first two commits as the series ending with 4cff56a.

zerver/models.py

timabbott · 2021-06-18T23:05:03Z

zerver/models.py

+            # Report patterns missing in linkifier pattern.
+            #
+            # NOTE: We do not need to report the missing patterns in Render format string
+            # as some users may or may not want to render every patterns found in linkifer pattern.


I think most correctly, we should be changing the existing check to allow fields in the linkifier pattern that are not present in the linkifier but are present in render_format_string, i.e. be merging the two found_group_set values.

To do that we need to change the current error message:

zulip/zerver/models.py

Lines 985 to 987 in 8cd9fc7

raise ValidationError(

_("Group %(name)r in URL format string is not present in linkifier pattern."),

params={"name": name},

to: Group %(name)r is not present in linkifier pattern. since in such case we can't tell that the missing name in linkifier pattern was present in url_format_string or render_format_string.

timabbott · 2021-06-18T23:05:18Z

zerver/models.py

+        blank=True,
+        validators=[
+            RegexValidator(
+                regex=r"^([\.\/:\w#?=&;!@~-]*%\(([\w-]+)\)s)*[/\w#?=&;~@-]*$",


How did you select this validator string?

Since render_format_string and url_format_string will have the same format: foo/%(id)s/bar so I picked the validator string used by url_format_string:

zulip/zerver/models.py

Line 941 in 8cd9fc7

regex = re.compile(r"^([\.\/:a-zA-Z0-9#_?=&;~-]+%$([a-zA-Z0-9_-]+)$s)+[/a-zA-Z0-9#_?=&;~-]*$")

A slight difference is that I've added ! and @ as the allowed characters. Since GitLab users may want to use it as !1234 and for commits as zulip/zulip@abcdefg.
And since it is not necessary to use all the detected groups in the linkifier render_format_string, I've used the * qualifier rather than the + qualifier.

This is done as a prep commit for the upcoming new changes where this regex will also be used to find matches in `render_format_string`.

Using this field, users can set the rendered text of the linkifier pattern. This is an optional field, if not provided, detected the pattern will be used to set the rendered text. Note: I've not added/removed validation checks in the `pattern` field, so still, this cannot be used as `Link Shortener` (Related issue: zulip#17971). In the future commits, I'll do the same.

If `render_format_string` is present, it will be used to set the rendered text of the detected linkifier pattern in messages and topic names.

… to create linkifer. This will help the users to add the optional field `render_format_string` to the realm linkifiers. New tests are added for the same.

… to update linkifier. This will help the users to update the optional field `render_format_string` of the realm linkifiers. New tests are added for the same.

Added `render_format` as an `optional_keys` in `realm_linkifier_type`. New tests are added and zulip.yaml is updated as well.

akshatdalton · 2021-06-19T21:32:59Z

I've extracted group_match_regex as a class attribute. #18914 (comment)
I've addressed the feedback as well: #18914 (comment), #18914 (comment), #18914 (comment).

@timabbott, please review.

…ener. This commit only modifies the regex. In the next commit, linkifier priority number will be increased in Markdown processing so that we can use it as link shortener.

…rtener. In this commit, linkifier priority number is increased in Markdown processing so that we can use it as link shortener. This increment in priority number is required to process any link that matches the linkifier pattern before it is processed by `AutoLink`. Fixes zulip#17971.

akshatdalton · 2021-06-21T22:31:49Z

These new commits: b1d3953, 928fb38 may require some discussion as:

for topic links with linkifier configuration:
pattern: https://github.com/(?P<org>[\w\-]+)/(?P<repo>[\w\-]+)/issues/(?P<id>[0-9]+)
url_format_string: https://github.com/%(org)s/%(repo)s/issues/%(id)s
render_format_string: %(org)s/%(repo)s#%(id)s
we get:

While the expected was: zulip/zulip#17971; more over there are two icons present to open the link; expected was one.
The possible fixes are:

a nested loop searching in topic_links in zerver/lib/markdown/__init__.py to avoid double parsing for the same link.
or let's totally avoid using this third field render_format_string in topic_links.
or maintain a set in topic_links (not sure) to keep this feature and to avoid nested loops.

Probably we also need to update the regex in filter_format_validator in models.py as when I tested (this on frontend) for the case:
url_format_string: https://github.com/%(org)s/%(repo)s/commit/%(commit_id)s%(id)s it failed to find any match (Raised an error: Invalid URL format string.). I've added a test based on this condition in test_markdown.py; I don't know how that test is passing here? 🤔
(This condition was motivated for render_format_strings as: %(org)s/%(repo)s@%(commit_id)s).

@timabbott, waiting to hear from you before making any new changes for the above two cases.

zulipbot · 2021-09-07T16:55:44Z

Heads up @akshatdalton, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: XL label Jun 18, 2021

akshatdalton commented Jun 18, 2021

View reviewed changes

akshatdalton force-pushed the issue_#17971 branch 2 times, most recently from 076b833 to 1308749 Compare June 18, 2021 22:16

timabbott reviewed Jun 18, 2021

View reviewed changes

zerver/models.py Outdated Show resolved Hide resolved

timabbott reviewed Jun 18, 2021

View reviewed changes

akshatdalton force-pushed the issue_#17971 branch from 1308749 to 3fc91d5 Compare June 19, 2021 21:12

akshatdalton added 6 commits June 19, 2021 21:15

refactor: Extract group_match_regex as a class attribute.

7799e12

This is done as a prep commit for the upcoming new changes where this regex will also be used to find matches in `render_format_string`.

markdown: Add changes to support render_format_string in linkifers.

e42c090

If `render_format_string` is present, it will be used to set the rendered text of the detected linkifier pattern in messages and topic names.

realm_linkifiers: Add render_format_string as an optional parameter…

120ffcb

… to create linkifer. This will help the users to add the optional field `render_format_string` to the realm linkifiers. New tests are added for the same.

realm_linkifiers: Add render_format_string as an optional parameter…

8c640b3

… to update linkifier. This will help the users to update the optional field `render_format_string` of the realm linkifiers. New tests are added for the same.

event_schema: Add render_format as optional_keys.

cea25f6

Added `render_format` as an `optional_keys` in `realm_linkifier_type`. New tests are added and zulip.yaml is updated as well.

akshatdalton force-pushed the issue_#17971 branch from 3fc91d5 to cea25f6 Compare June 19, 2021 21:32

akshatdalton added 2 commits June 21, 2021 21:33

models: Update linkifier pattern regex to use linkifier as link short…

b1d3953

…ener. This commit only modifies the regex. In the next commit, linkifier priority number will be increased in Markdown processing so that we can use it as link shortener.

zulipbot added the has conflicts label Sep 7, 2021

timabbott force-pushed the main branch 2 times, most recently from 4ec3636 to 88b200c Compare August 18, 2023 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

models: Add `render_format_string` field in `RealmFilter` model. #18914

models: Add `render_format_string` field in `RealmFilter` model. #18914

akshatdalton commented Jun 18, 2021

akshatdalton Jun 18, 2021

timabbott Jun 18, 2021 •

edited

Loading

akshatdalton Jun 18, 2021

timabbott commented Jun 18, 2021

timabbott Jun 18, 2021

akshatdalton Jun 18, 2021

timabbott Jun 18, 2021

akshatdalton Jun 18, 2021

akshatdalton commented Jun 19, 2021

akshatdalton commented Jun 21, 2021 •

edited

Loading

zulipbot commented Sep 7, 2021

	item.pattern.toLowerCase().includes(value) \|\|
	item.url_format.toLowerCase().includes(value)

	raise ValidationError(
	_("Group %(name)r in URL format string is not present in linkifier pattern."),
	params={"name": name},

models: Add render_format_string field in RealmFilter model. #18914

Are you sure you want to change the base?

models: Add render_format_string field in RealmFilter model. #18914

Conversation

akshatdalton commented Jun 18, 2021

akshatdalton Jun 18, 2021

Choose a reason for hiding this comment

timabbott Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

akshatdalton Jun 18, 2021

Choose a reason for hiding this comment

timabbott commented Jun 18, 2021

timabbott Jun 18, 2021

Choose a reason for hiding this comment

akshatdalton Jun 18, 2021

Choose a reason for hiding this comment

timabbott Jun 18, 2021

Choose a reason for hiding this comment

akshatdalton Jun 18, 2021

Choose a reason for hiding this comment

akshatdalton commented Jun 19, 2021

akshatdalton commented Jun 21, 2021 • edited Loading

zulipbot commented Sep 7, 2021

models: Add `render_format_string` field in `RealmFilter` model. #18914

models: Add `render_format_string` field in `RealmFilter` model. #18914

timabbott Jun 18, 2021 •

edited

Loading

akshatdalton commented Jun 21, 2021 •

edited

Loading