From d3c63ccc6d975583195c4ae6397557b9ed386a56 Mon Sep 17 00:00:00 2001 From: Tudor Date: Fri, 21 Aug 2026 19:06:55 +0100 Subject: [PATCH] fix(places): a locality collision must not break the sitemap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sitemap regeneration failed on staging. 'richmond' in the curated locality list collides with the GIAS town Richmond in North Yorkshire (37 schools), the registry raised, and the admin endpoint 500d — taking down sitemap generation for all 25,000 school pages over one bad row of curated data. The guard now skips the colliding locality and logs an error. Skipping still achieves what the guard was for — a locality never silently shadows a town — without letting curated data break the site. That matters beyond this bug: GIAS town names change with no code change here, so a raise could fire spontaneously in production later. Also removes four localities that were London boroughs rather than districts. Hackney, Islington, Greenwich and Ealing are local authorities with 104, 72, 108 and 115 schools and already have authority pages; a locality defined by two or three outcodes would have been a partial near-duplicate of one — the thin-content failure the two-namespace design exists to avoid. A test now guards the whole borough list. Validated against the live corpus: 15 localities, no town collisions, no authority duplicates, all 15 clear the threshold. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/localities.py | 17 +++-- backend/places.py | 17 +++-- backend/tests/test_places.py | 63 +++++++++++++++++-- .../transform/seeds/locality_outcodes.csv | 5 -- 4 files changed, 84 insertions(+), 18 deletions(-) diff --git a/backend/localities.py b/backend/localities.py index ecd3077..0d6369e 100644 --- a/backend/localities.py +++ b/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",)), diff --git a/backend/places.py b/backend/places.py index 12eb84a..5cab181 100644 --- a/backend/places.py +++ b/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: diff --git a/backend/tests/test_places.py b/backend/tests/test_places.py index bcd7019..af14671 100644 --- a/backend/tests/test_places.py +++ b/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" + ) diff --git a/pipeline/transform/seeds/locality_outcodes.csv b/pipeline/transform/seeds/locality_outcodes.csv index 968f6ce..11dfc8d 100644 --- a/pipeline/transform/seeds/locality_outcodes.csv +++ b/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 -- 2.54.0