Merge pull request 'fix(places): a locality collision must not break the sitemap' (#115) from fix/locality-collision-skip into main

Reviewed-on: #115

backend/localities.py

@@ -20,6 +20,18 @@ A locality whose outcodes hold fewer than MIN_SCHOOLS schools is not
 published, so a typo produces no page rather than an empty one. Places that
 fail that check are logged at startup, because a locality you meant to publish
 quietly not appearing is the failure worth hearing about.
+
+Two rules for anything added here.
+
+**Sub-borough districts only.** A London borough is a local authority and
+already has a page at /schools/authority/[la] covering all of its schools; a
+locality defined by two or three outcodes would be a partial, near-duplicate
+subset of it. Hackney, Islington, Greenwich and Ealing were all in the first
+draft for that reason and have been removed.
+
+**The slug must not match a GIAS town.** "Richmond" did — GIAS has a Richmond
+in North Yorkshire with 37 schools — so the London one could never publish.
+The registry skips any locality that collides and logs it.
 """
 
 # slug -> (display name, outcodes)
@@ -30,16 +42,11 @@ LOCALITY_OUTCODES: dict[str, tuple[str, tuple[str, ...]]] = {
     "shoreditch":    ("Shoreditch",    ("EC2A", "E1")),
     "peckham":       ("Peckham",       ("SE15",)),
     "brixton":       ("Brixton",       ("SW2", "SW9")),
-    "hackney":       ("Hackney",       ("E5", "E8", "E9")),
-    "islington":     ("Islington",     ("N1", "N5", "N7")),
     "camden-town":   ("Camden Town",   ("NW1",)),
-    "greenwich":     ("Greenwich",     ("SE10",)),
     "wimbledon":     ("Wimbledon",     ("SW19",)),
     "putney":        ("Putney",        ("SW15",)),
     "fulham":        ("Fulham",        ("SW6",)),
     "chiswick":      ("Chiswick",      ("W4",)),
-    "ealing":        ("Ealing",        ("W5", "W13")),
-    "richmond":      ("Richmond",      ("TW9", "TW10")),
     "stratford":     ("Stratford",     ("E15",)),
     "walthamstow":   ("Walthamstow",   ("E17",)),
     "tooting":       ("Tooting",       ("SW17",)),

backend/places.py

@@ -128,10 +128,19 @@ def _locality_places(df, publishable: set[int],
     out: dict[str, Place] = {}
     for slug, (name, outcodes) in LOCALITY_OUTCODES.items():
         if slug in town_slugs:
-            raise ValueError(
-                f"locality {slug!r} collides with a published town of the same "
-                "slug; publishing both would shadow the town silently"
-            )
+            # Skip, do not raise. The guard exists so a locality never
+            # silently shadows a town — skipping achieves that, and the error
+            # log makes it loud.
+            #
+            # Raising here took down sitemap generation for all 25,000 school
+            # pages when "richmond" met the GIAS town Richmond in North
+            # Yorkshire. Worse, GIAS town names change without any code change,
+            # so a raise means curated data can break the site spontaneously.
+            # A curation mistake must cost one page, not the sitemap.
+            logger.error(
+                "locality %r collides with the published town of the same "
+                "slug and has been skipped; rename it or remove it", slug)
+            continue
         group = working[working["_oc"].isin(outcodes)]
         urns = tuple(sorted({int(u) for u in group["urn"]} & publishable))
         if len(urns) < MIN_SCHOOLS:

backend/tests/test_places.py

@@ -108,16 +108,42 @@ def test_locality_below_the_threshold_is_not_published(monkeypatch):
     assert "locality:nowhere" not in reg
 
 
-def test_a_locality_may_not_shadow_a_viable_town(monkeypatch):
-    # Silently shadowing a town would lose a page carrying real demand.
+def test_a_locality_may_not_shadow_a_viable_town(monkeypatch, caplog):
+    """A colliding locality is skipped loudly, and the town survives.
+
+    This used to raise, which took down sitemap generation for all 25,000
+    school pages the first time a curated slug met a real GIAS town. Curated
+    data must not be able to break the site — and GIAS town names change with
+    no code change at all, so the raise could fire spontaneously.
+    """
+    import logging
+
     from backend import localities
     monkeypatch.setattr(localities, "LOCALITY_OUTCODES",
                         {"brentwood": ("Brentwood", ("CM13",))})
     rows = _town(MIN_SCHOOLS, "Brentwood", "Essex")
     for r in rows:
         r["postcode"] = "CM13 1AA"
-    with pytest.raises(ValueError, match="brentwood"):
-        build_place_registry(_df(rows))
+
+    with caplog.at_level(logging.ERROR):
+        reg = build_place_registry(_df(rows))
+
+    assert "locality:brentwood" not in reg      # skipped
+    assert "town:brentwood" in reg              # the town is untouched
+    assert "brentwood" in caplog.text           # and it was loud about it
+
+
+def test_a_locality_collision_does_not_break_the_rest_of_the_registry(monkeypatch):
+    # The whole point of skipping rather than raising.
+    from backend import localities
+    monkeypatch.setattr(localities, "LOCALITY_OUTCODES",
+                        {"brentwood": ("Brentwood", ("CM13",))})
+    rows = _town(MIN_SCHOOLS, "Brentwood", "Essex")
+    for r in rows:
+        r["postcode"] = "CM13 1AA"
+    reg = build_place_registry(_df(rows))
+    assert "authority:essex" in reg
+    assert "outcode:cm13" in reg
 
 
 def test_outcode_places_are_built_from_postcodes():
@@ -174,3 +200,32 @@ def test_the_pipeline_seed_mirrors_the_canonical_module():
         for row in csv.DictReader(seed_path.open())
     }
     assert seed == LOCALITY_OUTCODES
+
+
+def test_no_curated_locality_names_a_london_borough():
+    """Boroughs are authorities and already have a page.
+
+    A locality defined by two or three outcodes inside a borough would be a
+    partial, near-duplicate subset of that authority page — the exact
+    thin-content failure the two-namespace design exists to avoid. Hackney,
+    Islington, Greenwich and Ealing were all in the first draft.
+
+    Hardcoded rather than read from the corpus because this must fail in CI,
+    where there is no database.
+    """
+    from backend.localities import LOCALITY_OUTCODES
+
+    boroughs = {
+        "barking-and-dagenham", "barnet", "bexley", "brent", "bromley",
+        "camden", "croydon", "ealing", "enfield", "greenwich", "hackney",
+        "hammersmith-and-fulham", "haringey", "harrow", "havering",
+        "hillingdon", "hounslow", "islington", "kensington-and-chelsea",
+        "kingston-upon-thames", "lambeth", "lewisham", "merton", "newham",
+        "redbridge", "richmond-upon-thames", "southwark", "sutton",
+        "tower-hamlets", "waltham-forest", "wandsworth", "westminster",
+    }
+    named = boroughs & set(LOCALITY_OUTCODES)
+    assert not named, (
+        f"these are boroughs, not districts: {sorted(named)} - they already "
+        "have an authority page covering every school"
+    )

pipeline/transform/seeds/locality_outcodes.csv

@@ -5,16 +5,11 @@ clapham,Clapham,SW4,London
 shoreditch,Shoreditch,EC2A|E1,London
 peckham,Peckham,SE15,London
 brixton,Brixton,SW2|SW9,London
-hackney,Hackney,E5|E8|E9,London
-islington,Islington,N1|N5|N7,London
 camden-town,Camden Town,NW1,London
-greenwich,Greenwich,SE10,London
 wimbledon,Wimbledon,SW19,London
 putney,Putney,SW15,London
 fulham,Fulham,SW6,London
 chiswick,Chiswick,W4,London
-ealing,Ealing,W5|W13,London
-richmond,Richmond,TW9|TW10,London
 stratford,Stratford,E15,London
 walthamstow,Walthamstow,E17,London
 tooting,Tooting,SW17,London