From 1cb5314c536ef0ebabd1612784c3c34a843efc10 Mon Sep 17 00:00:00 2001 From: Tudor Date: Fri, 21 Aug 2026 22:19:23 +0100 Subject: [PATCH 1/2] feat(places): name every authority a place sits in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SW19 is mostly Merton but partly Wandsworth, and the page said only Merton. The cause was one field doing two jobs: _parent_authority takes the modal authority, which is right for a 301 target and wrong as a statement about where a place is. This is not a corner case. A quarter of viable outcodes (425 of 1,760) and a third of viable towns (263 of 783) cross an authority boundary — Bedford the town spans Bedford and Central Bedfordshire. Place now carries `authorities`, every authority holding at least a tenth of the schools and at least two of them, largest first. parent_authority stays single and unchanged, because a redirect still needs one target. The share threshold exists because GIAS carries 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. A place too small or too fragmented to clear the threshold still names its largest, so the page never goes silent about where it is. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 7 +++ backend/places.py | 45 ++++++++++++++ backend/tests/test_places.py | 61 +++++++++++++++++++ e2e/tests/journeys.spec.ts | 26 ++++++++ .../__tests__/components/PlaceView.test.tsx | 44 +++++++++++++ nextjs-app/components/places/PlaceView.tsx | 24 ++++++-- nextjs-app/lib/places.ts | 13 +++- 7 files changed, 214 insertions(+), 6 deletions(-) diff --git a/backend/app.py b/backend/app.py index 1eb0837..4cba056 100644 --- a/backend/app.py +++ b/backend/app.py @@ -1232,6 +1232,13 @@ async def get_place(request: Request, kind: str, slug: str, "place": {"kind": place.kind, "slug": place.slug, "name": place.name, "count": len(place.urns), "parent_authority": place.parent_authority, + # Every authority the place meaningfully sits in. SW19 is + # mostly Merton but partly Wandsworth; naming one asserts + # something false. + "authorities": [ + {"name": name, "slug": _slugify(name), "count": n} + for name, n in place.authorities + ], # Only phases that clear the threshold, so the page links # variants that exist rather than 404s. "phases": [ph for ph in ("primary", "secondary") diff --git a/backend/places.py b/backend/places.py index a9bb3e6..443acdd 100644 --- a/backend/places.py +++ b/backend/places.py @@ -30,6 +30,12 @@ class Place: name: str urns: tuple[int, ...] parent_authority: str | None # authority NAME, for the 301 target + # Every authority the place meaningfully sits in, largest first. A quarter + # of outcodes and a third of towns straddle a boundary — SW19 is mostly + # Merton but partly Wandsworth — so naming only one asserts something + # false. parent_authority stays single because a redirect needs one + # target; this is what the page shows. + authorities: tuple[tuple[str, int], ...] = () # URNs per phase, so the per-phase threshold can be applied without # re-querying. A place with 30 primaries and 2 secondaries publishes a # primary variant and no secondary one. @@ -71,6 +77,42 @@ def _phase_urns(group, publishable: set[int]) -> dict[str, tuple[int, ...]]: return out +# A place is described by an authority when it holds at least a tenth of the +# 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. +_AUTHORITY_MIN_SHARE = 0.10 +_AUTHORITY_MIN_SCHOOLS = 2 +_AUTHORITY_MAX_SHOWN = 3 + + +def _authorities(group) -> tuple[tuple[str, int], ...]: + """Authorities this place meaningfully sits in, largest first.""" + from backend.app import EXCLUDED_FILTER_VALUES + + if "local_authority" not in group.columns: + return () + counts = group["local_authority"].dropna().value_counts() + total = int(counts.sum()) + if not total: + return () + + kept = [ + (str(name), int(n)) for name, n in counts.items() + if str(name) not in EXCLUDED_FILTER_VALUES + and n >= _AUTHORITY_MIN_SCHOOLS + and n / total >= _AUTHORITY_MIN_SHARE + ] + # A place too small or too fragmented for the share rule still names its + # largest authority, or the page would say nothing about where it is. + if not kept: + for name, n in counts.items(): + if str(name) not in EXCLUDED_FILTER_VALUES: + return ((str(name), int(n)),) + return () + return tuple(kept[:_AUTHORITY_MAX_SHOWN]) + + def _parent_authority(group) -> str | None: """The most common authority in a group — the useful 301 target. @@ -104,6 +146,7 @@ def _group(df, column: str, kind: str, publishable: set[int]) -> dict[str, Place 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), phase_urns=_phase_urns(group, publishable), ) out[place.key] = place @@ -138,6 +181,7 @@ def _outcode_places(df, publishable: set[int]) -> dict[str, Place]: continue place = Place(kind="outcode", slug=str(oc).lower(), name=str(oc), urns=urns, parent_authority=_parent_authority(group), + authorities=_authorities(group), phase_urns=_phase_urns(group, publishable)) out[place.key] = place return out @@ -181,6 +225,7 @@ def _locality_places(df, publishable: set[int], continue place = Place(kind="locality", slug=slug, name=name, urns=urns, parent_authority=_parent_authority(group), + authorities=_authorities(group), 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 af14671..96283f4 100644 --- a/backend/tests/test_places.py +++ b/backend/tests/test_places.py @@ -229,3 +229,64 @@ def test_no_curated_locality_names_a_london_borough(): f"these are boroughs, not districts: {sorted(named)} - they already " "have an authority page covering every school" ) + + +def test_a_place_names_every_authority_it_straddles(): + """SW19 is mostly Merton but partly Wandsworth. + + A quarter of viable outcodes and a third of viable towns cross an + authority boundary, so naming only the largest asserts something false. + """ + rows = (_town(26, "London", "Merton", start=300000) + + _town(7, "London", "Wandsworth", start=400000)) + for r in rows: + r["postcode"] = "SW19 1AA" + reg = build_place_registry(_df(rows)) + + names = [n for n, _ in reg["outcode:sw19"].authorities] + assert names == ["Merton", "Wandsworth"] # largest first + assert dict(reg["outcode:sw19"].authorities)["Wandsworth"] == 7 + + +def test_the_redirect_target_stays_a_single_authority(): + # parent_authority and authorities do different jobs: a 301 needs one + # target, the page needs the truth. + rows = (_town(26, "London", "Merton", start=300000) + + _town(7, "London", "Wandsworth", start=400000)) + for r in rows: + r["postcode"] = "SW19 1AA" + reg = build_place_registry(_df(rows)) + assert reg["outcode:sw19"].parent_authority == "Merton" + + +def test_a_stray_authority_below_the_share_threshold_is_not_named(): + # GIAS carries postcode errors — EN6 lists two Shropshire schools among + # fourteen in Hertfordshire. Printing those as though real would be worse + # than omitting them. + rows = (_town(30, "Barnet", "Hertfordshire", start=300000) + + _town(1, "Barnet", "Shropshire", start=400000)) + for r in rows: + r["postcode"] = "EN6 1AA" + reg = build_place_registry(_df(rows)) + assert [n for n, _ in reg["outcode:en6"].authorities] == ["Hertfordshire"] + + +def test_a_sentinel_authority_is_never_named(): + rows = (_town(20, "London", "Merton", start=300000) + + _town(6, "London", "Does not apply", start=400000)) + for r in rows: + r["postcode"] = "SW19 1AA" + reg = build_place_registry(_df(rows)) + assert [n for n, _ in reg["outcode:sw19"].authorities] == ["Merton"] + + +def test_a_place_always_names_at_least_one_authority(): + # Even when every authority is below the share threshold, the page has to + # say where the place is. + rows = [] + for i, la in enumerate(["A", "B", "C", "D", "E", "F", "G"]): + rows += _town(1, "Fragmented", la, start=300000 + i * 100) + reg = build_place_registry(_df(rows)) + place = reg.get("town:fragmented") + assert place is not None + assert len(place.authorities) == 1 diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 35b6049..60b561d 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1885,3 +1885,29 @@ test('no page title repeats the brand', async ({ page }) => { expect(brands, `${path} repeats the brand: ${title}`).toBeLessThanOrEqual(1); } }); + +test('a place straddling a boundary names every authority it sits in', async ({ page }) => { + // A quarter of outcodes and a third of towns cross an authority boundary — + // SW19 is mostly Merton but partly Wandsworth. Naming only the largest + // asserts something false about the place. + const { places } = await (await page.request.get('/api/places')).json(); + const outcode = places.find((p: { kind: string }) => p.kind === 'outcode'); + expect(outcode).toBeTruthy(); + + // Find any place the registry reports as straddling. + let straddling: { kind: string; slug: string } | null = null; + for (const p of places.filter((p: { kind: string }) => p.kind === 'outcode').slice(0, 40)) { + const d = await (await page.request.get(`/api/places/outcode/${p.slug}`)).json(); + if ((d.place.authorities ?? []).length > 1) { straddling = p; break; } + } + test.skip(!straddling, 'no straddling outcode found in the sample'); + + const detail = await (await page.request.get( + `/api/places/outcode/${straddling!.slug}`)).json(); + await page.goto(`/schools/near/${straddling!.slug}`); + + for (const a of detail.place.authorities) { + await expect(page.locator(`a[href="/schools/authority/${a.slug}"]`).first()) + .toBeVisible(); + } +}); diff --git a/nextjs-app/__tests__/components/PlaceView.test.tsx b/nextjs-app/__tests__/components/PlaceView.test.tsx index aeec09e..f85b624 100644 --- a/nextjs-app/__tests__/components/PlaceView.test.tsx +++ b/nextjs-app/__tests__/components/PlaceView.test.tsx @@ -199,3 +199,47 @@ describe('PlaceView table alignment', () => { expect(container.querySelectorAll('th')[0].className).toBe(''); }); }); + +describe('PlaceView authorities', () => { + const straddling: PlaceDetail = { + place: { kind: 'outcode', slug: 'sw19', name: 'SW19', count: 33, + parent_authority: 'Merton', phases: ['primary'], + authorities: [ + { name: 'Merton', slug: 'merton', count: 26 }, + { name: 'Wandsworth', slug: 'wandsworth', count: 7 }, + ] }, + schools: [ + { urn: 1, school_name: 'Alpha Primary', phase: 'Primary', + rwm_expected_pct: 82, attainment_8_score: null } as never, + ], + averages: { rwm_expected_pct: 63, attainment_8_score: null }, + }; + + it('names every authority the place straddles, not just the largest', () => { + // SW19 is mostly Merton but partly Wandsworth. Naming one asserts + // something false about a quarter of outcodes. + render(); + expect(screen.getByRole('link', { name: 'Merton' })) + .toHaveAttribute('href', '/schools/authority/merton'); + expect(screen.getByRole('link', { name: 'Wandsworth' })) + .toHaveAttribute('href', '/schools/authority/wandsworth'); + }); + + it('joins them readably rather than as a bare list', () => { + // Asserted on the summary line's whole text: a loose /and/ matcher also + // hits "Wandsworth". + const { container } = render(); + const summary = container.querySelector('header p'); + expect(summary?.textContent).toContain('Merton and Wandsworth'); + }); + + it('falls back to the single parent when the field is absent', () => { + // A cached API response predating the authorities field must not blank + // the line entirely. + const legacy = { ...straddling, + place: { ...straddling.place, authorities: undefined } }; + render(); + expect(screen.getByRole('link', { name: 'Merton' })).toBeInTheDocument(); + }); +}); diff --git a/nextjs-app/components/places/PlaceView.tsx b/nextjs-app/components/places/PlaceView.tsx index a8efc5d..4a3ea5e 100644 --- a/nextjs-app/components/places/PlaceView.tsx +++ b/nextjs-app/components/places/PlaceView.tsx @@ -106,6 +106,13 @@ function SchoolTable({ schools, phase }: { schools: School[]; phase: PhaseKey }) export function PlaceView({ detail, phase, englandAverage, neighbours }: Props) { const { place, schools, averages } = detail; + // Fall back to the single parent when the API predates the authorities + // field, so a stale cache never blanks the line entirely. + const authorities = place.authorities?.length + ? place.authorities + : place.parent_authority + ? [{ name: place.parent_authority, slug: authoritySlug(place.parent_authority), count: 0 }] + : []; const local = averages[METRICS[phase ?? 'primary'].key]; const phaseWord = phase === 'secondary' ? 'Secondary schools' : phase === 'primary' ? 'Primary schools' : 'Schools'; @@ -163,13 +170,20 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)

{phaseWord} in {place.name}

{place.count} schools - {place.parent_authority && ( + {authorities.length > 0 && ( <> {' · '} - - {place.parent_authority} - + {/* Every authority, not just the largest. A quarter of outcodes + and a third of towns cross a boundary: SW19 is mostly Merton + but partly Wandsworth, and naming one asserts otherwise. */} + {authorities.map((a, i) => ( + + {i > 0 && (i === authorities.length - 1 ? ' and ' : ', ')} + + {a.name} + + + ))} )}

diff --git a/nextjs-app/lib/places.ts b/nextjs-app/lib/places.ts index d1be648..183efb2 100644 --- a/nextjs-app/lib/places.ts +++ b/nextjs-app/lib/places.ts @@ -18,8 +18,19 @@ export interface PlaceSummary { phases?: string[]; } +export interface PlaceAuthority { + name: string; + slug: string; + count: number; +} + export interface PlaceDetail { - place: PlaceSummary & { parent_authority: string | null }; + place: PlaceSummary & { + parent_authority: string | null; + /** Every authority the place meaningfully sits in, largest first. SW19 is + * mostly Merton but partly Wandsworth. */ + authorities?: PlaceAuthority[]; + }; schools: School[]; averages: { rwm_expected_pct: number | null; -- 2.54.0 From bb2f7a58418451cc4d8429d1e09b4cadfdeb1499 Mon Sep 17 00:00:00 2001 From: Tudor Date: Fri, 21 Aug 2026 22:42:18 +0100 Subject: [PATCH 2/2] fix(places): address review, and merge places GIAS spells more than one way MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from review on #121, plus a third the review prompted. The cap at three authorities silently dropped the fourth in exactly the case where the information matters most — a genuinely fragmented place — and contradicted the stated goal of naming every authority a place sits in. It is gone. The share rule was always the real limit and already bounds the list at ten. Measured against the live corpus, one town would have been truncated today: LONDON, split evenly between Hackney, Lambeth, Westminster and Lewisham. parent_authority used mode() while authorities used value_counts(), and on an exact tie pandas does not guarantee the two pick the same name, so the 301 could have pointed somewhere other than the authority named first on the page. The parent is now derived from authorities[0]: one computation, one answer. It also inherits the sentinel filter, so a place can no longer redirect to /schools/authority/does-not-apply. Chasing the truncation case surfaced a worse bug. Places were grouped by raw town value, but the registry is keyed by slug, and GIAS spells the same place several ways. Five town slugs come from more than one spelling: "London" (1,819 schools) and "LONDON" (12) both slugify to `london`, so the later group simply overwrote the earlier one — /schools/london could have shown twelve schools, silently, depending on row order. Weston-super-Mare was split 14/19 across two spellings and Newcastle-under-Lyme across three. Grouping is now by slug, and the display name is the most common spelling. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/places.py | 72 ++++++++++++++++++++++++------------ backend/tests/test_places.py | 53 ++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 23 deletions(-) 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" -- 2.54.0