From 2a981e2ac1d13125973904b777d00ea75e8df4e6 Mon Sep 17 00:00:00 2001
From: Simon Willison <swillison@gmail.com>
Date: Wed, 11 Nov 2020 15:37:37 -0800
Subject: [PATCH] Blank foreign key labels now show as hyphens, closes #1086

---
 datasette/templates/table.html |  4 +-
 datasette/views/table.py       |  2 +-
 tests/fixtures.py              |  6 ++-
 tests/test_api.py              | 76 ++++++++++++++++++++++------------
 tests/test_csv.py              |  6 +--
 tests/test_html.py             | 30 +++++++++-----
 6 files changed, 80 insertions(+), 44 deletions(-)

diff --git a/datasette/templates/table.html b/datasette/templates/table.html
index 5034b62e..077332dc 100644
--- a/datasette/templates/table.html
+++ b/datasette/templates/table.html
@@ -156,9 +156,9 @@
                 <ul class="tight-bullets">
                     {% for facet_value in facet_info.results %}
                         {% if not facet_value.selected %}
-                            <li><a href="{{ facet_value.toggle_url }}">{{ (facet_value.label if facet_value.label is not none else "_") }}</a> {{ "{:,}".format(facet_value.count) }}</li>
+                            <li><a href="{{ facet_value.toggle_url }}">{{ (facet_value.label | string()) or "-" }}</a> {{ "{:,}".format(facet_value.count) }}</li>
                         {% else %}
-                            <li>{{ facet_value.label }} &middot; {{ "{:,}".format(facet_value.count) }} <a href="{{ facet_value.toggle_url }}" class="cross">&#x2716;</a></li>
+                            <li>{{ facet_value.label or "-" }} &middot; {{ "{:,}".format(facet_value.count) }} <a href="{{ facet_value.toggle_url }}" class="cross">&#x2716;</a></li>
                         {% endif %}
                     {% endfor %}
                     {% if facet_info.truncated %}
diff --git a/datasette/views/table.py b/datasette/views/table.py
index 65fe7f8b..d29ef201 100644
--- a/datasette/views/table.py
+++ b/datasette/views/table.py
@@ -195,7 +195,7 @@ class RowTableShared(DataView):
                             table=urllib.parse.quote_plus(other_table),
                             link_id=urllib.parse.quote_plus(str(value)),
                             id=str(jinja2.escape(value)),
-                            label=str(jinja2.escape(label)),
+                            label=str(jinja2.escape(label)) or "-",
                         )
                     )
                 elif value in ("", None):
diff --git a/tests/fixtures.py b/tests/fixtures.py
index a48cfb46..bd530398 100644
--- a/tests/fixtures.py
+++ b/tests/fixtures.py
@@ -386,8 +386,10 @@ CREATE INDEX idx_compound_three_primary_keys_content ON compound_three_primary_k
 CREATE TABLE foreign_key_references (
   pk varchar(30) primary key,
   foreign_key_with_label varchar(30),
+  foreign_key_with_blank_label varchar(30),
   foreign_key_with_no_label varchar(30),
   FOREIGN KEY (foreign_key_with_label) REFERENCES simple_primary_key(id),
+  FOREIGN KEY (foreign_key_with_blank_label) REFERENCES simple_primary_key(id),
   FOREIGN KEY (foreign_key_with_no_label) REFERENCES primary_key_multiple_columns(id)
 );
 
@@ -622,8 +624,8 @@ INSERT INTO simple_primary_key VALUES (4, 'RENDER_CELL_DEMO');
 INSERT INTO primary_key_multiple_columns VALUES (1, 'hey', 'world');
 INSERT INTO primary_key_multiple_columns_explicit_label VALUES (1, 'hey', 'world2');
 
-INSERT INTO foreign_key_references VALUES (1, 1, 1);
-INSERT INTO foreign_key_references VALUES (2, null, null);
+INSERT INTO foreign_key_references VALUES (1, 1, 3, 1);
+INSERT INTO foreign_key_references VALUES (2, null, null, null);
 
 INSERT INTO complex_foreign_keys VALUES (1, 1, 2, 1);
 INSERT INTO custom_foreign_key_label VALUES (1, 1);
diff --git a/tests/test_api.py b/tests/test_api.py
index 1a43e7f4..d6d683b7 100644
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -49,21 +49,21 @@ def test_homepage_sort_by_relationships(app_client):
     tables = [
         t["name"] for t in response.json["fixtures"]["tables_and_views_truncated"]
     ]
-    assert [
+    assert tables == [
         "simple_primary_key",
+        "foreign_key_references",
         "complex_foreign_keys",
         "roadside_attraction_characteristics",
         "searchable_tags",
-        "foreign_key_references",
-    ] == tables
+    ]
 
 
 def test_database_page(app_client):
     response = app_client.get("/fixtures.json")
     assert response.status == 200
     data = response.json
-    assert "fixtures" == data["database"]
-    assert [
+    assert data["database"] == "fixtures"
+    assert data["tables"] == [
         {
             "name": "123_starts_with_digits",
             "columns": ["content"],
@@ -232,7 +232,12 @@ def test_database_page(app_client):
         },
         {
             "name": "foreign_key_references",
-            "columns": ["pk", "foreign_key_with_label", "foreign_key_with_no_label"],
+            "columns": [
+                "pk",
+                "foreign_key_with_label",
+                "foreign_key_with_blank_label",
+                "foreign_key_with_no_label",
+            ],
             "primary_keys": ["pk"],
             "count": 2,
             "hidden": False,
@@ -245,6 +250,11 @@ def test_database_page(app_client):
                         "column": "foreign_key_with_no_label",
                         "other_column": "id",
                     },
+                    {
+                        "other_table": "simple_primary_key",
+                        "column": "foreign_key_with_blank_label",
+                        "other_column": "id",
+                    },
                     {
                         "other_table": "simple_primary_key",
                         "column": "foreign_key_with_label",
@@ -403,6 +413,11 @@ def test_database_page(app_client):
             "fts_table": None,
             "foreign_keys": {
                 "incoming": [
+                    {
+                        "other_table": "foreign_key_references",
+                        "column": "id",
+                        "other_column": "foreign_key_with_blank_label",
+                    },
                     {
                         "other_table": "foreign_key_references",
                         "column": "id",
@@ -548,7 +563,7 @@ def test_database_page(app_client):
             "foreign_keys": {"incoming": [], "outgoing": []},
             "private": False,
         },
-    ] == data["tables"]
+    ]
 
 
 def test_no_files_uses_memory_database(app_client_no_files):
@@ -1203,32 +1218,38 @@ def test_row_foreign_key_tables(app_client):
         "/fixtures/simple_primary_key/1.json?_extras=foreign_key_tables"
     )
     assert response.status == 200
-    assert [
+    assert response.json["foreign_key_tables"] == [
         {
-            "column": "id",
-            "count": 1,
-            "other_column": "foreign_key_with_label",
             "other_table": "foreign_key_references",
-        },
-        {
-            "column": "id",
-            "count": 1,
-            "other_column": "f3",
-            "other_table": "complex_foreign_keys",
-        },
-        {
             "column": "id",
+            "other_column": "foreign_key_with_blank_label",
             "count": 0,
-            "other_column": "f2",
-            "other_table": "complex_foreign_keys",
         },
         {
+            "other_table": "foreign_key_references",
             "column": "id",
+            "other_column": "foreign_key_with_label",
             "count": 1,
-            "other_column": "f1",
-            "other_table": "complex_foreign_keys",
         },
-    ] == response.json["foreign_key_tables"]
+        {
+            "other_table": "complex_foreign_keys",
+            "column": "id",
+            "other_column": "f3",
+            "count": 1,
+        },
+        {
+            "other_table": "complex_foreign_keys",
+            "column": "id",
+            "other_column": "f2",
+            "count": 0,
+        },
+        {
+            "other_table": "complex_foreign_keys",
+            "column": "id",
+            "other_column": "f1",
+            "count": 1,
+        },
+    ]
 
 
 def test_unit_filters(app_client):
@@ -1593,13 +1614,14 @@ def test_expand_label(app_client):
         "/fixtures/foreign_key_references.json?_shape=object"
         "&_label=foreign_key_with_label&_size=1"
     )
-    assert {
+    assert response.json == {
         "1": {
             "pk": "1",
             "foreign_key_with_label": {"value": "1", "label": "hello"},
+            "foreign_key_with_blank_label": "3",
             "foreign_key_with_no_label": "1",
         }
-    } == response.json
+    }
 
 
 @pytest.mark.parametrize(
@@ -1790,11 +1812,13 @@ def test_null_foreign_keys_are_not_expanded(app_client):
         {
             "pk": "1",
             "foreign_key_with_label": {"value": "1", "label": "hello"},
+            "foreign_key_with_blank_label": {"value": "3", "label": ""},
             "foreign_key_with_no_label": {"value": "1", "label": "1"},
         },
         {
             "pk": "2",
             "foreign_key_with_label": None,
+            "foreign_key_with_blank_label": None,
             "foreign_key_with_no_label": None,
         },
     ] == response.json
diff --git a/tests/test_csv.py b/tests/test_csv.py
index 3e91fb04..209bce2b 100644
--- a/tests/test_csv.py
+++ b/tests/test_csv.py
@@ -42,9 +42,9 @@ pk,created,planet_int,on_earth,state,city_id,city_id_label,neighborhood,tags,com
 )
 
 EXPECTED_TABLE_WITH_NULLABLE_LABELS_CSV = """
-pk,foreign_key_with_label,foreign_key_with_label_label,foreign_key_with_no_label,foreign_key_with_no_label_label
-1,1,hello,1,1
-2,,,,
+pk,foreign_key_with_label,foreign_key_with_label_label,foreign_key_with_blank_label,foreign_key_with_blank_label_label,foreign_key_with_no_label,foreign_key_with_no_label_label
+1,1,hello,3,,1,1
+2,,,,,,
 """.lstrip().replace(
     "\n", "\r\n"
 )
diff --git a/tests/test_html.py b/tests/test_html.py
index 006c223d..7fca8a68 100644
--- a/tests/test_html.py
+++ b/tests/test_html.py
@@ -464,7 +464,7 @@ def test_facet_display(app_client):
                 ],
             }
         )
-    assert [
+    assert actual == [
         {
             "name": "city_id",
             "items": [
@@ -520,7 +520,7 @@ def test_facet_display(app_client):
                 },
             ],
         },
-    ] == actual
+    ]
 
 
 def test_facets_persist_through_filter_form(app_client):
@@ -801,37 +801,47 @@ def test_table_html_foreign_key_links(app_client):
     response = app_client.get("/fixtures/foreign_key_references")
     assert response.status == 200
     table = Soup(response.body, "html.parser").find("table")
-    expected = [
+    actual = [[str(td) for td in tr.select("td")] for tr in table.select("tbody tr")]
+    assert actual == [
         [
             '<td class="col-pk type-pk"><a href="/fixtures/foreign_key_references/1">1</a></td>',
             '<td class="col-foreign_key_with_label type-str"><a href="/fixtures/simple_primary_key/1">hello</a>\xa0<em>1</em></td>',
+            '<td class="col-foreign_key_with_blank_label type-str"><a href="/fixtures/simple_primary_key/3">-</a>\xa0<em>3</em></td>',
             '<td class="col-foreign_key_with_no_label type-str"><a href="/fixtures/primary_key_multiple_columns/1">1</a></td>',
         ],
         [
             '<td class="col-pk type-pk"><a href="/fixtures/foreign_key_references/2">2</a></td>',
             '<td class="col-foreign_key_with_label type-none">\xa0</td>',
+            '<td class="col-foreign_key_with_blank_label type-none">\xa0</td>',
             '<td class="col-foreign_key_with_no_label type-none">\xa0</td>',
         ],
     ]
-    assert expected == [
-        [str(td) for td in tr.select("td")] for tr in table.select("tbody tr")
-    ]
+
+
+def test_table_html_foreign_key_facets(app_client):
+    response = app_client.get(
+        "/fixtures/foreign_key_references?_facet=foreign_key_with_blank_label"
+    )
+    assert response.status == 200
+    assert (
+        '<li><a href="http://localhost/fixtures/foreign_key_references?_facet=foreign_key_with_blank_label&amp;foreign_key_with_blank_label=3">'
+        "-</a> 1</li>"
+    ) in response.text
 
 
 def test_table_html_disable_foreign_key_links_with_labels(app_client):
     response = app_client.get("/fixtures/foreign_key_references?_labels=off&_size=1")
     assert response.status == 200
     table = Soup(response.body, "html.parser").find("table")
-    expected = [
+    actual = [[str(td) for td in tr.select("td")] for tr in table.select("tbody tr")]
+    assert actual == [
         [
             '<td class="col-pk type-pk"><a href="/fixtures/foreign_key_references/1">1</a></td>',
             '<td class="col-foreign_key_with_label type-str">1</td>',
+            '<td class="col-foreign_key_with_blank_label type-str">3</td>',
             '<td class="col-foreign_key_with_no_label type-str">1</td>',
         ]
     ]
-    assert expected == [
-        [str(td) for td in tr.select("td")] for tr in table.select("tbody tr")
-    ]
 
 
 def test_table_html_foreign_key_custom_label_column(app_client):