From ef3eacf622e69723d48ab1ad597645770a7361db Mon Sep 17 00:00:00 2001
From: Simon Willison <swillison@gmail.com>
Date: Thu, 23 Nov 2017 12:32:54 -0800
Subject: [PATCH] Select option for removing filters

---
 datasette/app.py               | 10 +++++++---
 datasette/templates/table.html |  1 +
 datasette/utils.py             | 34 +++++++++++++++++++---------------
 tests/test_app.py              | 20 +++++++++++++++++---
 4 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/datasette/app.py b/datasette/app.py
index e4475a85..3e2cf42a 100644
--- a/datasette/app.py
+++ b/datasette/app.py
@@ -58,8 +58,8 @@ class BaseView(HTTPMethodView):
             r.headers['Access-Control-Allow-Origin'] = '*'
         return r
 
-    def redirect(self, request, path):
-        if request.query_string and '?' not in path:
+    def redirect(self, request, path, forward_querystring=True):
+        if request.query_string and '?' not in path and forward_querystring:
             path = '{}?{}'.format(
                 path, request.query_string
             )
@@ -435,7 +435,11 @@ class TableView(BaseView):
         # Handle ?_filter_column and redirect, if present
         redirect_params = filters_should_redirect(special_args)
         if redirect_params:
-            return self.redirect(request, path_with_added_args(request, redirect_params))
+            return self.redirect(
+                request,
+                path_with_added_args(request, redirect_params),
+                forward_querystring=False
+            )
 
         filters = Filters(sorted(other_args.items()))
         where_clauses, params = filters.build_where_clauses()
diff --git a/datasette/templates/table.html b/datasette/templates/table.html
index 70860fce..06a6c71f 100644
--- a/datasette/templates/table.html
+++ b/datasette/templates/table.html
@@ -34,6 +34,7 @@
         <div class="filter-row">
             <div class="select-wrapper">
                 <select name="_filter_column_{{ loop.index }}">
+                    <option value="">- remove filter -</option>
                     {% for c in display_columns %}
                         {% if c != 'rowid' %}
                             <option{% if c == column %} selected{% endif %}>{{ c }}</option>
diff --git a/datasette/utils.py b/datasette/utils.py
index aac6eaad..5ef21488 100644
--- a/datasette/utils.py
+++ b/datasette/utils.py
@@ -91,7 +91,10 @@ def path_with_added_args(request, args):
         for key, value in args
         if value is not None
     ])
-    return request.path + '?' + urllib.parse.urlencode(sorted(current))
+    query_string = urllib.parse.urlencode(sorted(current))
+    if query_string:
+        query_string = '?{}'.format(query_string)
+    return request.path + query_string
 
 
 def path_with_ext(request, ext):
@@ -402,19 +405,19 @@ filter_column_re = re.compile(r'^_filter_column_\d+$')
 
 def filters_should_redirect(special_args):
     redirect_params = []
-    if '_filter_column' in special_args:
-        filter_column = special_args['_filter_column']
-        if filter_column:
-            filter_op = special_args.get('_filter_op') or ''
-            filter_value = special_args.get('_filter_value') or ''
-            if '__' in filter_op:
-                filter_op, filter_value = filter_op.split('__', 1)
-            redirect_params.extend([
-                ('{}__{}'.format(filter_column, filter_op), filter_value),
-                ('_filter_column', None),
-                ('_filter_op', None),
-                ('_filter_value', None),
-            ])
+    # Handle _filter_column=foo&_filter_op=exact&_filter_value=...
+    filter_column = special_args.get('_filter_column')
+    filter_op = special_args.get('_filter_op') or ''
+    filter_value = special_args.get('_filter_value') or ''
+    if '__' in filter_op:
+        filter_op, filter_value = filter_op.split('__', 1)
+    if filter_column:
+        redirect_params.append(
+            ('{}__{}'.format(filter_column, filter_op), filter_value)
+        )
+    for key in ('_filter_column', '_filter_op', '_filter_value'):
+        if key in special_args:
+            redirect_params.append((key, None))
     # Now handle _filter_column_1=name&_filter_op_1=contains&_filter_value_1=hello
     column_keys = [k for k in special_args if filter_column_re.match(k)]
     for column_key in column_keys:
@@ -424,8 +427,9 @@ def filters_should_redirect(special_args):
         value = special_args.get('_filter_value_{}'.format(number)) or ''
         if '__' in op:
             op, value = op.split('__', 1)
+        if column:
+            redirect_params.append(('{}__{}'.format(column, op), value))
         redirect_params.extend([
-            ('{}__{}'.format(column, op), value),
             ('_filter_column_{}'.format(number), None),
             ('_filter_op_{}'.format(number), None),
             ('_filter_value_{}'.format(number), None),
diff --git a/tests/test_app.py b/tests/test_app.py
index 70556252..2ea3b4a0 100644
--- a/tests/test_app.py
+++ b/tests/test_app.py
@@ -303,7 +303,7 @@ def test_add_filter_redirects(app_client):
 
 
 def test_existing_filter_redirects(app_client):
-    filter_args = urllib.parse.urlencode({
+    filter_args = {
         '_filter_column_1': 'name',
         '_filter_op_1': 'contains',
         '_filter_value_1': 'hello',
@@ -316,17 +316,31 @@ def test_existing_filter_redirects(app_client):
         '_filter_column_4': 'name',
         '_filter_op_4': 'contains',
         '_filter_value_4': 'world',
-    })
+    }
     path_base = app_client.get(
         '/test_tables/simple_primary_key', allow_redirects=False, gather_request=False
     ).headers['Location']
-    path = path_base + '?' + filter_args
+    path = path_base + '?' + urllib.parse.urlencode(filter_args)
     response = app_client.get(path, allow_redirects=False, gather_request=False)
     assert response.status == 302
     assert response.headers['Location'].endswith(
         '?age__gte=22&age__lt=30&name__contains=hello&name__contains=world'
     )
 
+    # Setting _filter_column_3 to empty string should remove *_3 entirely
+    filter_args['_filter_column_3'] = ''
+    path = path_base + '?' + urllib.parse.urlencode(filter_args)
+    response = app_client.get(path, allow_redirects=False, gather_request=False)
+    assert response.status == 302
+    assert response.headers['Location'].endswith(
+        '?age__gte=22&name__contains=hello&name__contains=world'
+    )
+
+    # ?_filter_op=exact should be removed if unaccompanied by _fiter_column
+    response = app_client.get(path_base + '?_filter_op=exact', allow_redirects=False, gather_request=False)
+    assert response.status == 302
+    assert '?' not in response.headers['Location']
+
 
 TABLES = '''
 CREATE TABLE simple_primary_key (