Custom realm filters #2261

TigorC · 2016-11-09T06:11:55Z

This is cleanup and rebase of #544

smarx · 2016-11-09T06:11:56Z

Automated message from Dropbox CLA bot

@TigorC, it looks like you've already signed the Dropbox CLA. Thanks!

timabbott · 2016-11-15T21:59:02Z

frontend_tests/node_tests/templates.js

@@ -129,6 +129,29 @@ function render(template_name, args) {
    assert.equal(emoji_url.attr('src'), 'http://emojipedia-us.s3.amazonaws.com/cache/46/7f/467fe69069c408e07517621f263ea9b5.png');
 }());

+(function admin_filter_list() {
+    // global.use_template('admin_filter_list');


global.use_template no longer exists (the tracking is done automatically); you can delete this line entirely.

timabbott · 2016-11-15T22:02:26Z

zerver/views/realm_filters.py

+def create_filter(request, user_profile):
+    # type: (HttpRequest, UserProfile) -> HttpResponse
+    pattern = request.POST.get('pattern', None)
+    url_format_string = request.POST.get('url_format_string', None)


these should be using the has_request_variables / REQ decorators. See http://zulip.readthedocs.io/en/latest/writing-views.html#writing-api-rest-endpoints for details.

timabbott

Cool, this is looking great @TigorC! I posted a bunch of small comments. There are 2 larger things we should change:

It needs more extensive backend tests; we should exercise the API fully (including triggering all error conditions) to get full backend test coverage on the new code, as well as having a nice set of tests for all the validation code.
I'm not totally sold on the ALLOWED_FILTER_PREFIXES model; I think we ultimately should allow the following model for patterns:

<PREFIX>(<CAPTURE>) where <PREFIX> is an arbitrary string not containing any special characters of meaning to regular expressions and (<CAPTURE>) is something of the form (?P<STR>[CHARSET]+), where STR is a

example:
ZUL-123 via pattern ZUL-(?P<id>[0-9]+)

I don't think we need to fully implement that for merging this, though; maybe for right now we should just eliminate the ALLOWED_FILTER_PREFIXES variable and just validate that the prefix checking that it has just a string of no-special-characters before the (? That seems really easy and would cover most of the issue.

Thanks @TigorC!

timabbott · 2016-11-15T22:05:02Z

zerver/lib/actions.py

+
+def do_remove_realm_filter(realm, pattern=None, id=None):
+    # type: (Realm, text_type, int) -> None
+    if pattern:


use is not None stylistically. Also, in mypy one should use Optional[text_type], Optional[int] for the other arguments.

timabbott · 2016-11-15T22:05:59Z

zerver/models.py

    for realm_filter in RealmFilter.objects.all():
-       filters[realm_filter.realm.domain].append((realm_filter.pattern, realm_filter.url_format_string))
+        filters[realm_filter.realm.domain].append((realm_filter.pattern, realm_filter.url_format_string, realm_filter.pk))


use realm_filter.id, not realm_filter.pk -- as a project, we prefer to be explicit about which field is the primary key being used.

timabbott · 2016-11-15T22:06:07Z

zerver/tests/test_bugdown.py

@@ -444,7 +444,7 @@ def test_realm_patterns(self):
                                   url_format_string=url_format_string)
        realm_filter.save()
        self.assertEqual(
-            str(realm_filter),
+            realm_filter.__unicode__(),


Why is this change needed?

str(realm_filter) - this doesn't work with python 3
We need to use python_2_unicode_compatible decorator, but it requires
from __future__ import unicode_literals
and this will require many changes in models.py, for this reason I added this hack

Cool, makes sense. This is probably fine given that it's test code.

timabbott · 2016-11-15T22:10:18Z

zerver/models.py

+
+def filter_format_validator(value):
+    # type: (str) -> None
+    regex = re.compile(r'.*(%\(\w+\))')


this regex seems like it could use some work to check the entire string to be of the format suggested in the error message below. I think basically we want to ensure that it contains no regex special characters other than a single instance of %([a-zA-Z0-9_0]+)s.

timabbott · 2016-11-15T22:11:40Z

static/templates/settings/realm-filter-settings-admin.handlebars

@@ -0,0 +1,35 @@
+<div id="filter-settings" class="settings-section">
+  <div class="settings-section-title"><i class="icon-vector-filter settings-section-icon"></i>Custom realm filters</div>
+  <div class="admin-table-wrapper">


This should add a description explaining how "realm filters" work -- i.e. that it is a set of regular expressions that will be automatically linkified when used in Zulip message bodies or topics.

timabbott · 2016-11-15T22:13:07Z

static/js/admin.js

+
+        channel.put({
+            url: "/json/realm/filters",
+            data: $(this).serialize(),


This code seems to have a bug where it doesn't strip whitespace properly; I accidentally had some trailing whitespace in some entries I created via the web UI and got the following filters (they don't work because of the \t characters):

$ ./manage.py realm_filters --r zulip.com zulip.com: [('#(?P<id>[0-9]+)\t', 'https://trac.example.com/ticket/%(id)s', 2), ('T(?P<id>[0-9]+)\t', 'https://trac.example.com/ticket/%(id)s', 3)]

timabbott · 2016-11-15T22:14:17Z

zerver/views/realm_filters.py

+    filters = realm_filters_for_domain(user_profile.realm.domain)
+    return json_success({'filters': filters})
+
+def create_filter(request, user_profile):


This (and the delete one) should use the @require_realm_admin decorator -- otherwise non-admins will be able to modify these.

timabbott · 2016-11-15T22:14:57Z

zerver/views/realm_filters.py

+
+def delete_filter(request, user_profile, filter_id):
+    # type: (HttpRequest, UserProfile, int) -> HttpResponse
+    do_remove_realm_filter(realm=user_profile.realm, id=filter_id)


This should check the filter exists and report an appropriate error if it doesn't.

timabbott · 2016-11-15T22:15:36Z

static/js/admin.js

+            url: "/json/realm/filters",
+            data: $(this).serialize(),
+            success: function (data) {
+                filter.id = data.id;


Is this line needed?

timabbott · 2016-11-15T22:25:31Z

zerver/models.py

+        raise ValidationError('Invalid filter pattern')
+
+    if not regex.match(pattern):
+        raise ValidationError('Pattern format string must be in the following format: (?P<\w+>.+)')


This should probably come before the re.compile test.

TigorC · 2016-11-16T15:40:55Z

@timabbott

example:
ZUL-123 via pattern ZUL-(?P<id>[0-9]+)

- this is a special character in regular expression, maybe allow some characters like -#?

timabbott · 2016-11-16T19:39:37Z

Yeah, we can allow some that aren't special without other characters (e.g. - has this property; you need [] for it to do anything); probably just -_# is good for now.

timabbott

Thanks for working on this @TigorC! I fixed several issues (noted in the comments for your reference for future contributions) and merged this. Thanks @TigorC!

timabbott · 2016-11-18T00:29:52Z

static/templates/settings/realm-filter-settings-admin.handlebars

+      </div>
+    </div>
+  </form>
+</div>


This is missing the CSS class to make the add-new-filter-box green; we should match styling with the "add-new-realm-emoji" form. (We'll be changing what this standard style looks like eventually, but consistency is ideal for now).

timabbott · 2016-11-18T00:57:09Z

static/templates/admin_filter_list.handlebars

+  </td>
+  <td>
+      <button class="btn delete btn-danger" data-filter-id="{{id}}">
+        Delete


this should have been tagged for translation

timabbott · 2016-11-18T00:57:15Z

static/templates/settings/realm-filter-settings-admin.handlebars

+      <tbody id="admin_filters_table">
+        <th>Pattern</th>
+        <th>URL format</th>
+        <th>Actions</th>


these should have been tagged for translation

timabbott · 2016-11-18T00:57:38Z

static/templates/settings/realm-filter-settings-admin.handlebars

+        <div class="alert" id="admin-filter-status"></div>
+        <div class="control-group">
+          <label for="filter_pattern" class="control-label">Filter pattern</label>
+          <input type="text" id="filter_pattern" name="pattern" placeholder="{{t '#(?P<id>[0-9]+)' }}" />


This should not be tagged for translation; instead one should have added this file to the exclude list for that linter rule. See tools/lint-all

timabbott · 2016-11-18T00:58:56Z

zproject/urls.py

+    # realm/filters -> zerver.views.realm_filters
+    url(r'^realm/filters$', rest_dispatch,
+        {'GET': 'zerver.views.realm_filters.list_filters',
+         'PUT': 'zerver.views.realm_filters.create_filter'}),


This should be using POST, not PUT. See e.g. http://restcookbook.com/HTTP%20Methods/put-vs-post/.

Looks like we have some existing code doing this wrong as well to fix.

TigorC force-pushed the custom-realm-filters branch from eaf7494 to 600df1a Compare November 10, 2016 07:21

timabbott reviewed Nov 15, 2016

View reviewed changes

blablacio and others added 2 commits November 17, 2016 09:39

Add initial implementation of custom realm filters

4a3c7e3

Cleanup and rebase Custom realm filters

420e43f

TigorC force-pushed the custom-realm-filters branch from 600df1a to 420e43f Compare November 17, 2016 04:43

timabbott reviewed Nov 18, 2016

View reviewed changes

timabbott mentioned this pull request Nov 18, 2016

Custom realm filters #544

Closed

timabbott closed this Nov 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom realm filters #2261

Custom realm filters #2261

TigorC commented Nov 9, 2016

smarx commented Nov 9, 2016

timabbott Nov 15, 2016

timabbott Nov 15, 2016

timabbott left a comment

timabbott Nov 15, 2016

timabbott Nov 15, 2016

timabbott Nov 15, 2016

TigorC Nov 16, 2016

timabbott Nov 16, 2016

timabbott Nov 15, 2016

timabbott Nov 15, 2016

timabbott Nov 15, 2016

timabbott Nov 15, 2016

timabbott Nov 15, 2016

timabbott Nov 15, 2016

timabbott Nov 15, 2016

TigorC commented Nov 16, 2016

timabbott commented Nov 16, 2016

timabbott left a comment

timabbott Nov 18, 2016

timabbott Nov 18, 2016

timabbott Nov 18, 2016

timabbott Nov 18, 2016

timabbott Nov 18, 2016

Custom realm filters #2261

Custom realm filters #2261

Conversation

TigorC commented Nov 9, 2016

smarx commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TigorC commented Nov 16, 2016

timabbott commented Nov 16, 2016

timabbott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment