From cf6e7840aeef2e93dca1392b7a717ff0badb81b3 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 14 May 2018 17:42:10 -0300 Subject: [PATCH 01/24] Facet "selected" key and toggle_url now toggles, refs #255 --- datasette/utils.py | 12 ++++++++++++ datasette/views/table.py | 23 ++++++++++++++++------- docs/facets.rst | 5 +++++ tests/test_api.py | 9 ++++++++- tests/test_utils.py | 13 +++++++++++++ 5 files changed, 54 insertions(+), 8 deletions(-) diff --git a/datasette/utils.py b/datasette/utils.py index 118d9bd7..0e0c388b 100644 --- a/datasette/utils.py +++ b/datasette/utils.py @@ -165,6 +165,18 @@ def path_with_added_args(request, args, path=None): return path + query_string +def path_with_removed_args(request, args, path=None): + path = path or request.path + current = [] + for key, value in urllib.parse.parse_qsl(request.query_string): + if key not in args: + current.append((key, value)) + query_string = urllib.parse.urlencode(current) + if query_string: + query_string = '?{}'.format(query_string) + return path + query_string + + def path_with_ext(request, ext): path = request.path path += ext diff --git a/datasette/views/table.py b/datasette/views/table.py index 5369825c..99075abe 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -13,6 +13,7 @@ from datasette.utils import ( is_url, path_from_row_pks, path_with_added_args, + path_with_removed_args, to_css_class, urlsafe_components ) @@ -505,17 +506,25 @@ class TableView(RowTableShared): facet_rows = await self.execute( name, facet_sql, params, truncate=False, custom_time_limit=200 ) - facet_results[column] = [ - { + facet_results[column] = [] + for row in facet_rows: + selected = other_args.get(column) == row["value"] + if selected: + toggle_path = path_with_removed_args( + request, {column: row["value"]} + ) + else: + toggle_path = path_with_added_args( + request, {column: row["value"]} + ) + facet_results[column].append({ "value": row["value"], "count": row["count"], "toggle_url": urllib.parse.urljoin( - request.url, - path_with_added_args(request, {column: row["value"]}), + request.url, toggle_path ), - } - for row in facet_rows - ] + "selected": selected, + }) except sqlite3.OperationalError: # Hit time limit pass diff --git a/docs/facets.rst b/docs/facets.rst index 40499e55..02ac8d55 100644 --- a/docs/facets.rst +++ b/docs/facets.rst @@ -23,11 +23,13 @@ This works for both the HTML interface and the ``.json`` view. When enabled, fac { "value": "CA", "count": 10, + "selected": false, "toggle_url": "http://...&state=CA" }, { "value": "MI", "count": 4, + "selected": false, "toggle_url": "http://...&state=MI" } ], @@ -35,16 +37,19 @@ This works for both the HTML interface and the ``.json`` view. When enabled, fac { "value": "San Francisco", "count": 6, + "selected": false, "toggle_url": "http://...=San+Francisco" }, { "value": "Detroit", "count": 4, + "selected": false, "toggle_url": "http://...&city=Detroit" }, { "value": "Los Angeles", "count": 4, + "selected": false, "toggle_url": "http://...=Los+Angeles" } ] diff --git a/tests/test_api.py b/tests/test_api.py index 3f91b075..da8c177c 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -897,11 +897,13 @@ def test_page_size_matching_max_returned_rows(app_client_returend_rows_matches_p { "value": "CA", "count": 10, + "selected": False, "toggle_url": "_facet=state&_facet=city&state=CA", }, { "value": "MI", "count": 4, + "selected": False, "toggle_url": "_facet=state&_facet=city&state=MI", }, ], @@ -909,16 +911,19 @@ def test_page_size_matching_max_returned_rows(app_client_returend_rows_matches_p { "value": "San Francisco", "count": 6, + "selected": False, "toggle_url": "_facet=state&_facet=city&city=San+Francisco", }, { "value": "Detroit", "count": 4, + "selected": False, "toggle_url": "_facet=state&_facet=city&city=Detroit", }, { "value": "Los Angeles", "count": 4, + "selected": False, "toggle_url": "_facet=state&_facet=city&city=Los+Angeles", }, ], @@ -930,13 +935,15 @@ def test_page_size_matching_max_returned_rows(app_client_returend_rows_matches_p { "value": "MI", "count": 4, - "toggle_url": "_facet=state&_facet=city&state=MI", + "selected": True, + "toggle_url": "_facet=state&_facet=city", }, ], "city": [ { "value": "Detroit", "count": 4, + "selected": False, "toggle_url": "_facet=state&_facet=city&state=MI&city=Detroit", }, ], diff --git a/tests/test_utils.py b/tests/test_utils.py index 3f51f87d..0d6bb2d1 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -45,6 +45,19 @@ def test_path_with_added_args(path, added_args, expected): assert expected == actual +@pytest.mark.parametrize('path,args,expected', [ + ('/foo?bar=1', {'bar'}, '/foo'), + ('/foo?bar=1&baz=2', {'bar'}, '/foo?baz=2'), +]) +def test_path_with_removed_args(path, args, expected): + request = Request( + path.encode('utf8'), + {}, '1.1', 'GET', None + ) + actual = utils.path_with_removed_args(request, args) + assert expected == actual + + @pytest.mark.parametrize('row,pks,expected_path', [ ({'A': 'foo', 'B': 'bar'}, ['A', 'B'], 'foo,bar'), ({'A': 'f,o', 'B': 'bar'}, ['A', 'B'], 'f%2Co,bar'), From 7e61a1f77bc99dcc9fce8e1d7708d85b0c51861b Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 14 May 2018 18:33:24 -0300 Subject: [PATCH 02/24] Facet toggling now works for integer columns, refs #255 --- datasette/views/table.py | 2 +- tests/fixtures.py | 32 ++++++++++++++------------- tests/test_api.py | 47 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 63 insertions(+), 18 deletions(-) diff --git a/datasette/views/table.py b/datasette/views/table.py index 99075abe..d80c9431 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -508,7 +508,7 @@ class TableView(RowTableShared): ) facet_results[column] = [] for row in facet_rows: - selected = other_args.get(column) == row["value"] + selected = str(other_args.get(column)) == str(row["value"]) if selected: toggle_path = path_with_removed_args( request, {column: row["value"]} diff --git a/tests/fixtures.py b/tests/fixtures.py index 2f2c32ff..99d686e4 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -269,25 +269,27 @@ INSERT INTO [select] VALUES ('group', 'having', 'and'); CREATE TABLE facetable ( pk integer primary key, + planet_id integer, state text, city text, neighborhood text ); -INSERT INTO facetable (state, city, neighborhood) VALUES - ('CA', 'San Francisco', 'Mission'), - ('CA', 'San Francisco', 'Dogpatch'), - ('CA', 'San Francisco', 'SOMA'), - ('CA', 'San Francisco', 'Tenderloin'), - ('CA', 'San Francisco', 'Bernal Heights'), - ('CA', 'San Francisco', 'Hayes Valley'), - ('CA', 'Los Angeles', 'Hollywood'), - ('CA', 'Los Angeles', 'Downtown'), - ('CA', 'Los Angeles', 'Los Feliz'), - ('CA', 'Los Angeles', 'Koreatown'), - ('MI', 'Detroit', 'Downtown'), - ('MI', 'Detroit', 'Greektown'), - ('MI', 'Detroit', 'Corktown'), - ('MI', 'Detroit', 'Mexicantown') +INSERT INTO facetable (planet_id, state, city, neighborhood) VALUES + (1, 'CA', 'San Francisco', 'Mission'), + (1, 'CA', 'San Francisco', 'Dogpatch'), + (1, 'CA', 'San Francisco', 'SOMA'), + (1, 'CA', 'San Francisco', 'Tenderloin'), + (1, 'CA', 'San Francisco', 'Bernal Heights'), + (1, 'CA', 'San Francisco', 'Hayes Valley'), + (1, 'CA', 'Los Angeles', 'Hollywood'), + (1, 'CA', 'Los Angeles', 'Downtown'), + (1, 'CA', 'Los Angeles', 'Los Feliz'), + (1, 'CA', 'Los Angeles', 'Koreatown'), + (1, 'MI', 'Detroit', 'Downtown'), + (1, 'MI', 'Detroit', 'Greektown'), + (1, 'MI', 'Detroit', 'Corktown'), + (1, 'MI', 'Detroit', 'Mexicantown'), + (2, 'MC', 'Memnonia', 'Arcadia Planitia') ; INSERT INTO simple_primary_key VALUES (1, 'hello'); diff --git a/tests/test_api.py b/tests/test_api.py index da8c177c..45b4b43c 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -105,9 +105,9 @@ def test_database_page(app_client): 'fts_table': None, 'primary_keys': ['pk'], }, { - 'columns': ['pk', 'state', 'city', 'neighborhood'], + 'columns': ['pk', 'planet_id', 'state', 'city', 'neighborhood'], 'name': 'facetable', - 'count': 14, + 'count': 15, 'foreign_keys': {'incoming': [], 'outgoing': []}, 'fts_table': None, 'hidden': False, @@ -906,6 +906,12 @@ def test_page_size_matching_max_returned_rows(app_client_returend_rows_matches_p "selected": False, "toggle_url": "_facet=state&_facet=city&state=MI", }, + { + "value": "MC", + "count": 1, + "selected": False, + "toggle_url": "_facet=state&_facet=city&state=MC", + }, ], "city": [ { @@ -926,6 +932,12 @@ def test_page_size_matching_max_returned_rows(app_client_returend_rows_matches_p "selected": False, "toggle_url": "_facet=state&_facet=city&city=Los+Angeles", }, + { + "value": "Memnonia", + "count": 1, + "selected": False, + "toggle_url": "_facet=state&_facet=city&city=Memnonia", + }, ], }, ), ( @@ -948,6 +960,37 @@ def test_page_size_matching_max_returned_rows(app_client_returend_rows_matches_p }, ], }, + ), ( + "/test_tables/facetable.json?_facet=planet_id", + { + "planet_id": [ + { + "value": 1, + "count": 14, + "selected": False, + "toggle_url": "_facet=planet_id&planet_id=1", + }, + { + "value": 2, + "count": 1, + "selected": False, + "toggle_url": "_facet=planet_id&planet_id=2", + }, + ], + }, + ), ( + # planet_id is an integer field: + "/test_tables/facetable.json?_facet=planet_id&planet_id=1", + { + "planet_id": [ + { + "value": 1, + "count": 14, + "selected": True, + "toggle_url": "_facet=planet_id", + } + ], + }, ) ]) def test_facets(app_client, path, expected_facet_results): From 2a365b6156f7651e1b795cc88e128f288e4130d6 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 14 May 2018 19:09:09 -0300 Subject: [PATCH 03/24] path_with_added_args now works with multiple existing args --- datasette/utils.py | 4 ++-- tests/test_utils.py | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/datasette/utils.py b/datasette/utils.py index 0e0c388b..e5bdf589 100644 --- a/datasette/utils.py +++ b/datasette/utils.py @@ -149,10 +149,10 @@ def path_with_added_args(request, args, path=None): path = path or request.path if isinstance(args, dict): args = args.items() - arg_keys = set(a[0] for a in args) + args_to_remove = {k for k, v in args if v is None} current = [] for key, value in urllib.parse.parse_qsl(request.query_string): - if key not in arg_keys: + if key not in args_to_remove: current.append((key, value)) current.extend([ (key, value) diff --git a/tests/test_utils.py b/tests/test_utils.py index 0d6bb2d1..bbae4f08 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -35,6 +35,9 @@ def test_urlsafe_components(path, expected): ('/?_facet=state&_facet=city&state=MI', ( ('city', 'Detroit'), ), '/?_facet=state&_facet=city&state=MI&city=Detroit'), + ('/?_facet=state&_facet=city', ( + ('_facet', 'planet_id'), + ), '/?_facet=state&_facet=city&_facet=planet_id'), ]) def test_path_with_added_args(path, added_args, expected): request = Request( From ce84d76fff2ffd0f15e268ff0208c8acf0048615 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 14 May 2018 19:09:42 -0300 Subject: [PATCH 04/24] Initial implementation of suggested facets Causes tests to break at the moment --- datasette/templates/table.html | 4 ++++ datasette/views/table.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/datasette/templates/table.html b/datasette/templates/table.html index ba582ba9..605962d2 100644 --- a/datasette/templates/table.html +++ b/datasette/templates/table.html @@ -91,6 +91,10 @@

This data as .json

+{% if suggested_facets %} +

Suggested facets: {% for facet in suggested_facets %}{{ facet.name }} {% endfor %} +{% endif %} + {% for facet_name, facet_values in facet_results.items() %}

{{ facet_name }}