diff --git a/backend/places.py b/backend/places.py index 443acdd..870f779 100644 --- a/backend/places.py +++ b/backend/places.py @@ -81,9 +81,12 @@ def _phase_urns(group, publishable: set[int]) -> dict[str, tuple[int, ...]]: # schools, and at least two. GIAS carries occasional postcode errors — EN6 # lists two Shropshire schools among fourteen in Hertfordshire — and a bare # "any authority present" rule would print those as though they were real. +# There is deliberately no cap on how many are named. An earlier cut stopped +# at three, which silently dropped the fourth in exactly the case where the +# information matters most — a genuinely fragmented place. The share rule is +# the only limit, and it already bounds the list at ten. _AUTHORITY_MIN_SHARE = 0.10 _AUTHORITY_MIN_SCHOOLS = 2 -_AUTHORITY_MAX_SHOWN = 3 def _authorities(group) -> tuple[tuple[str, int], ...]: @@ -110,43 +113,64 @@ def _authorities(group) -> tuple[tuple[str, int], ...]: if str(name) not in EXCLUDED_FILTER_VALUES: return ((str(name), int(n)),) return () - return tuple(kept[:_AUTHORITY_MAX_SHOWN]) + return tuple(kept) -def _parent_authority(group) -> str | None: - """The most common authority in a group — the useful 301 target. +def _parent_authority(authorities: tuple[tuple[str, int], ...]) -> str | None: + """The 301 target: the largest authority a place sits in. - A town spanning several authorities has no single parent, so the mode is - the honest answer rather than an arbitrary first row. + Derived from `authorities` rather than computed separately. The first cut + used `mode()` here while `authorities` used `value_counts()`, and on an + exact tie pandas does not guarantee the two pick the same name — so the + redirect could have pointed somewhere other than the authority the page + named first. One computation, one answer. + + Deriving it also inherits the sentinel filter, so a place can no longer + redirect to /schools/authority/does-not-apply. """ - if "local_authority" not in group.columns: - return None - top = group["local_authority"].dropna() - return str(top.mode().iloc[0]) if not top.empty else None + return authorities[0][0] if authorities else None def _group(df, column: str, kind: str, publishable: set[int]) -> dict[str, Place]: - """One Place per distinct value of `column` that clears the threshold.""" + """One Place per distinct SLUG in `column` that clears the threshold. + + Grouped by slug, not by raw value, because GIAS spells the same place + several ways and they all resolve to one URL. Five town slugs come from + more than one spelling: "London" (1,819 schools) and "LONDON" (12) both + slugify to `london`; Weston-super-Mare is split 14/19 across two + spellings; Newcastle-under-Lyme across three. + + Grouping by raw value meant the later group simply overwrote the earlier + one in this dict — so /schools/london could have shown twelve schools + instead of 1,819, silently and depending on row order. + + The display name is the most common spelling, which is the one a reader + expects to see. + """ from backend.app import _slugify if column not in df.columns: return {} + working = df.assign(_slug=df[column].map( + lambda v: _slugify(str(v).strip()) if isinstance(v, str) and v.strip() else None)) + working = working[working["_slug"].notna() & (working["_slug"] != "")] + out: dict[str, Place] = {} - for name, group in df.groupby(column, dropna=True): - name = str(name).strip() - if not name: - continue + for slug, group in working.groupby("_slug"): + slug = str(slug) urns = tuple(sorted({int(u) for u in group["urn"]} & publishable)) if len(urns) < MIN_SCHOOLS: continue - slug = _slugify(name) - if not slug: + spellings = group[column].dropna().value_counts() + if spellings.empty: continue + name = str(spellings.index[0]).strip() + authorities = () if kind == "authority" else _authorities(group) place = Place( kind=kind, slug=slug, name=name, urns=urns, - parent_authority=_parent_authority(group) if kind == "town" else None, - authorities=() if kind == "authority" else _authorities(group), + parent_authority=_parent_authority(authorities), + authorities=authorities, phase_urns=_phase_urns(group, publishable), ) out[place.key] = place @@ -179,9 +203,10 @@ def _outcode_places(df, publishable: set[int]) -> dict[str, Place]: urns = tuple(sorted({int(u) for u in group["urn"]} & publishable)) if len(urns) < MIN_SCHOOLS: continue + authorities = _authorities(group) place = Place(kind="outcode", slug=str(oc).lower(), name=str(oc), - urns=urns, parent_authority=_parent_authority(group), - authorities=_authorities(group), + urns=urns, parent_authority=_parent_authority(authorities), + authorities=authorities, phase_urns=_phase_urns(group, publishable)) out[place.key] = place return out @@ -223,9 +248,10 @@ def _locality_places(df, publishable: set[int], "threshold of %d - not published", slug, ", ".join(outcodes), len(urns), MIN_SCHOOLS) continue + authorities = _authorities(group) place = Place(kind="locality", slug=slug, name=name, urns=urns, - parent_authority=_parent_authority(group), - authorities=_authorities(group), + parent_authority=_parent_authority(authorities), + authorities=authorities, phase_urns=_phase_urns(group, publishable)) out[place.key] = place return out diff --git a/backend/tests/test_places.py b/backend/tests/test_places.py index 96283f4..6f3be5e 100644 --- a/backend/tests/test_places.py +++ b/backend/tests/test_places.py @@ -290,3 +290,56 @@ def test_a_place_always_names_at_least_one_authority(): place = reg.get("town:fragmented") assert place is not None assert len(place.authorities) == 1 + + +def test_every_qualifying_authority_is_named_with_no_cap(): + """An earlier cut stopped at three, dropping the fourth silently. + + That truncation bit exactly where the information matters most — a + genuinely fragmented place — and nothing recorded it. + """ + rows = [] + for i, la in enumerate(["Hackney", "Lambeth", "Westminster", "Lewisham"]): + rows += _town(3, "Fourway", la, start=300000 + i * 100) + reg = build_place_registry(_df(rows)) + assert len(reg["town:fourway"].authorities) == 4 + + +def test_the_redirect_target_is_the_authority_named_first(): + """They were computed separately — mode() against value_counts() — and on + an exact tie pandas does not guarantee the two agree.""" + rows = (_town(26, "London", "Merton", start=300000) + + _town(7, "London", "Wandsworth", start=400000)) + for r in rows: + r["postcode"] = "SW19 1AA" + place = build_place_registry(_df(rows))["outcode:sw19"] + assert place.parent_authority == place.authorities[0][0] + + +def test_a_place_never_redirects_to_a_sentinel_authority(): + # Deriving the parent from `authorities` inherits its sentinel filter. + rows = (_town(6, "Someplace", "Does not apply", start=300000) + + _town(5, "Someplace", "Essex", start=400000)) + reg = build_place_registry(_df(rows)) + assert reg["town:someplace"].parent_authority == "Essex" + + +def test_spellings_of_one_place_are_merged_not_overwritten(): + """GIAS spells the same place several ways, and they share a URL. + + "London" (1,819 schools) and "LONDON" (12) both slugify to `london`. + Grouping by raw value let the later group overwrite the earlier one, so + the page could have shown twelve schools instead of 1,819 — silently, and + depending on row order. + """ + rows = (_town(6, "Weston-super-Mare", "North Somerset", start=300000) + + _town(5, "Weston-Super-Mare", "North Somerset", start=400000)) + reg = build_place_registry(_df(rows)) + assert len(reg["town:weston-super-mare"].urns) == 11 + + +def test_the_merged_place_takes_its_most_common_spelling(): + rows = (_town(9, "Newcastle-under-Lyme", "Staffordshire", start=300000) + + _town(5, "NEWCASTLE-UNDER-LYME", "Staffordshire", start=400000)) + reg = build_place_registry(_df(rows)) + assert reg["town:newcastle-under-lyme"].name == "Newcastle-under-Lyme"