From d1358cc00f83d4f8b22e448758ebf2a522590733 Mon Sep 17 00:00:00 2001 From: Tudor Date: Sat, 22 Aug 2026 17:22:11 +0100 Subject: [PATCH] fix(places): phase links must stay in their own namespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every place page built its phase links as /schools/[slug]/[phase], the shape that belongs to towns alone. On an authority page that pointed into the town namespace. For 87 of the 151 authorities the target does not exist and the link 404s; for the other 64 it resolves to the town of the same name — a different set of schools, which is precisely the near-duplicate the two namespaces were introduced to prevent. On an outcode page it 404s outright. Two causes behind it, both a rule written twice and inherited by only one of the places that needed it. The authority phase route was in the spec and dropped by the plan, which built the three bare routes and no fourth. The sitemap is generated from the place registry, which was right about them all along, so 302 authority phase URLs have been submitted to Google and every one 404s. Adding the route makes the sitemap true and serves a real query — admissions are authority-run, so "primary schools in Kent" is how a parent searches before they have settled on a town. The outcode variants were the opposite: the registry computed phases for outcodes although the spec gives them no route, and the sitemap knew to skip them while the API did not. The registry now decides alone, and the sitemap's duplicate of that rule is gone. Also: an authority under the five-school threshold has no page, so the API sends a null slug for it and the page names it without linking. Two English authorities are in that position. It was unreachable in today's data — verified across the EC and TR outcodes — but the thin place redirect would have sent a reader to a 404 the year it isn't. The e2e journey now walks every /schools link a page of each family emits and requires a 200, which is the check that was missing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 22 ++++-- backend/places.py | 12 +++- backend/tests/test_places.py | 29 ++++++++ backend/tests/test_places_api.py | 48 +++++++++++++ backend/tests/test_sitemap.py | 16 +++++ e2e/tests/journeys.spec.ts | 62 +++++++++++++++- .../__tests__/components/PlaceView.test.tsx | 70 +++++++++++++++++++ nextjs-app/app/schools/[place]/page.tsx | 11 ++- .../schools/authority/[la]/[phase]/page.tsx | 65 +++++++++++++++++ nextjs-app/components/places/PlaceView.tsx | 21 ++++-- nextjs-app/lib/places.ts | 19 ++++- 11 files changed, 353 insertions(+), 22 deletions(-) create mode 100644 nextjs-app/app/schools/authority/[la]/[phase]/page.tsx diff --git a/backend/app.py b/backend/app.py index fa8945d..2284ac8 100644 --- a/backend/app.py +++ b/backend/app.py @@ -222,10 +222,10 @@ def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]: if p.kind not in kinds: continue rows.append(_url_element(BASE_URL + _place_url(p))) - # Outcodes carry no phase variants: nobody searches "primary schools - # in SW11", so the routes do not exist to submit. - if p.kind == "outcode": - continue + # Which phases a place publishes is the registry's decision alone — + # outcodes report none, because the spec gives them no phase route. + # Repeating that rule here was how the page and the sitemap came to + # disagree about which URLs exist. for phase in ("primary", "secondary"): if p.publishes_phase(phase): rows.append(_url_element(f"{BASE_URL}{_place_url(p)}/{phase}")) @@ -1200,7 +1200,8 @@ async def get_place(request: Request, kind: str, slug: str, if kind not in VALID_PLACE_KINDS: raise HTTPException(status_code=404, detail="No such place") - place = get_place_registry().get(f"{kind}:{slug}") + registry = get_place_registry() + place = registry.get(f"{kind}:{slug}") if place is None: raise HTTPException(status_code=404, detail="No such place") @@ -1240,8 +1241,17 @@ async def get_place(request: Request, kind: str, slug: str, # Every authority the place meaningfully sits in. SW19 is # mostly Merton but partly Wandsworth; naming one asserts # something false. + # + # The slug is null where that authority has no page of its + # own: City of London and the Isles of Scilly hold fewer + # schools than the threshold. Naming them is still right; + # linking them would be a 404. "authorities": [ - {"name": name, "slug": _slugify(name), "count": n} + {"name": name, + "slug": (_slugify(name) + if f"authority:{_slugify(name)}" in registry + else None), + "count": n} for name, n in place.authorities ], # Only phases that clear the threshold, so the page links diff --git a/backend/places.py b/backend/places.py index aae7fdd..078638c 100644 --- a/backend/places.py +++ b/backend/places.py @@ -224,7 +224,14 @@ def _outcode(postcode) -> str | None: def _outcode_places(df, publishable: set[int]) -> dict[str, Place]: """One Place per postcode district clearing the threshold. - These carry no phase variants: nobody searches "primary schools in SW11". + These carry no phase variants: nobody searches "primary schools in SW11", + so the spec gives them no /primary or /secondary route. `phase_urns` is + left empty rather than computed and then filtered downstream — the + registry is the one place that decides which phases a place publishes, + and the page links whatever it reports. + + Computing them here put a link to a route that does not exist on every one + of the 1,720 outcode pages. """ if "postcode" not in df.columns: return {} @@ -239,8 +246,7 @@ def _outcode_places(df, publishable: set[int]) -> dict[str, Place]: authorities = _authorities(group) place = Place(kind="outcode", slug=str(oc).lower(), name=str(oc), urns=urns, parent_authority=_parent_authority(authorities), - authorities=authorities, - phase_urns=_phase_urns(group, publishable)) + authorities=authorities) out[place.key] = place return out diff --git a/backend/tests/test_places.py b/backend/tests/test_places.py index 521c536..99e82e2 100644 --- a/backend/tests/test_places.py +++ b/backend/tests/test_places.py @@ -389,3 +389,32 @@ def test_the_secondary_threshold_counts_its_own_metric(): rows = _town(MIN_SCHOOLS, "Brentwood", "Essex") reg = build_place_registry(_df(rows)) assert not reg["town:brentwood"].publishes_phase("secondary") + + +def test_an_outcode_publishes_no_phase_variants(): + """There is no /schools/near/[outcode]/[phase] route, by design. + + Nobody searches "primary schools in SW11", so the spec gives outcodes no + phase variants. The registry computed them anyway, and the place page — + which links whatever phases the registry reports — put two 404s on every + outcode page in the site. + + This is the single rule now: a kind with no phase route reports no phases, + so neither the page nor the sitemap can offer one. + """ + rows = [{"urn": 500000 + i, "school_name": f"SW11 School {i}", + "town": "London", "local_authority": "Wandsworth", + "postcode": "SW11 1AA"} for i in range(MIN_SCHOOLS + 3)] + reg = build_place_registry(_df(rows)) + + place = reg["outcode:sw11"] + assert place.phase_urns == {} + assert not place.publishes_phase("primary") + assert not place.publishes_phase("secondary") + + +def test_an_authority_still_publishes_phase_variants(): + """Authorities keep theirs — "primary schools in Kent" is a real query, + and /schools/authority/[la]/[phase] is the route that serves it.""" + reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Maidstone", "Kent"))) + assert reg["authority:kent"].publishes_phase("primary") diff --git a/backend/tests/test_places_api.py b/backend/tests/test_places_api.py index b80444d..1d8f701 100644 --- a/backend/tests/test_places_api.py +++ b/backend/tests/test_places_api.py @@ -89,3 +89,51 @@ def test_unknown_place_404s(client): def test_unknown_kind_404s(client): assert client.get("/api/places/planet/mars").status_code == 404 + + +def _straddling_df() -> pd.DataFrame: + """Eight schools in CM13: six in Essex, which has a page, and two in an + authority too small to have one. + + Two, not one: the registry ignores an authority holding a single school in + a place, because GIAS carries occasional postcode errors.""" + df = _schools_df() + extra = df.iloc[:2].copy() + extra["urn"] = [200000, 200001] + extra["school_name"] = ["Scilly School 0", "Scilly School 1"] + extra["local_authority"] = "Isles Of Scilly" + return pd.concat([df, extra], ignore_index=True) + + +@pytest.fixture() +def straddling_client(monkeypatch): + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _straddling_df) + monkeypatch.setattr(app_module, "load_latest_school_data", _straddling_df) + monkeypatch.setattr(app_module, "_place_registry", None) + return TestClient(app_module.app, raise_server_exceptions=False) + + +def test_an_outcode_reports_no_phases_because_it_has_no_phase_route(client): + body = client.get("/api/places/outcode/cm13").json() + assert body["place"]["phases"] == [] + + +def test_an_authority_reports_the_phases_it_publishes(client): + body = client.get("/api/places/authority/essex").json() + assert body["place"]["phases"] == ["primary"] + + +def test_an_authority_without_a_page_is_named_but_carries_no_slug(straddling_client): + """Two English authorities — City of London and the Isles of Scilly — hold + fewer than the five schools a page needs, so they have no page. + + Naming them is still right: the page says where the place is. Linking them + would not be. A null slug is what tells the page to print the name plainly + rather than invent a URL that 404s. + """ + body = straddling_client.get("/api/places/outcode/cm13").json() + by_name = {a["name"]: a for a in body["place"]["authorities"]} + assert by_name["Essex"]["slug"] == "essex" + assert by_name["Isles Of Scilly"]["slug"] is None diff --git a/backend/tests/test_sitemap.py b/backend/tests/test_sitemap.py index 142b572..f45cf45 100644 --- a/backend/tests/test_sitemap.py +++ b/backend/tests/test_sitemap.py @@ -288,3 +288,19 @@ def test_outcodes_get_no_phase_variants(place_sitemaps): # Nobody searches "primary schools in CM13"; the routes do not exist. xml = place_sitemaps["outcodes-1.xml"] assert "/primary" not in xml and "/secondary" not in xml + + +def test_authority_phase_variants_are_submitted_in_their_own_namespace(place_sitemaps): + """302 of these were already in the sitemap, and every one 404'd. + + The spec gives authorities a phase route; the plan built the bare + authority route and dropped it. Nothing noticed because the sitemap was + written from the registry, which was right, while the routes were written + by hand. This test fails if the URL ever leaves the sitemap; the e2e + journey fails if the route ever leaves the app. + """ + xml = place_sitemaps["places-1.xml"] + assert ("https://www.schoolcompare.co.uk" + "/schools/authority/essex/primary") in xml + # And never in the town namespace, which is a different set of schools. + assert "/schools/essex/primary" not in xml diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 03b1e27..7a81e60 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1909,6 +1909,57 @@ test('phase variants are submitted in the places sitemap', async ({ page }) => { expect(xml).toMatch(/\/schools\/[a-z0-9-]+\/primary { + // 302 of these were in the sitemap for weeks and every one 404'd: the spec + // called for the route, the plan built the bare authority page and dropped + // it, and the sitemap — written from the registry — kept submitting them. + const xml = await (await page.request.get('/sitemaps/places-1.xml')).text(); + expect(xml).toMatch(/\/schools\/authority\/[a-z0-9-]+\/primary { + /* + * The guard that was missing. Each family built its own links, so a URL + * shape belonging to one namespace was used by all four: an authority page + * offered "Primary schools in Barnet" pointing at /schools/barnet/primary, + * the *town*. For 87 of 151 authorities that 404'd; for the other 64 it + * quietly served a different set of schools under the same name. + * + * Only /schools links are followed. The per-school links are the same + * component the school-page journeys already cover, and there are hundreds + * of them on a page. + */ + for (const kind of ['town', 'authority', 'outcode'] as const) { + const place = await firstPlaceOfKind(page, kind); + const prefix = kind === 'authority' ? '/schools/authority/' + : kind === 'outcode' ? '/schools/near/' : '/schools/'; + await page.goto(`${prefix}${place.slug}`); + + const hrefs = [...new Set( + await page.locator('a[href^="/schools"]').evaluateAll( + (els) => els.map((e) => e.getAttribute('href')!)))]; + expect(hrefs.length, `${kind} page links no other place`).toBeGreaterThan(0); + + for (const href of hrefs) { + const res = await page.request.get(href); + expect(res.status(), `${kind} page links ${href}`).toBe(200); + } + } +}); + +test('an outcode page offers no phase link, because no such page exists', async ({ page }) => { + // Nobody searches "primary schools in SW11", so the spec gives outcodes no + // phase route. The registry computed the variants anyway and the page + // linked them, putting two 404s on each of 1,720 outcode pages. + const place = await firstPlaceOfKind(page, 'outcode'); + const detail = await (await page.request.get( + `/api/places/outcode/${place.slug}`)).json(); + expect(detail.place.phases).toEqual([]); + + await page.goto(`/schools/near/${place.slug}`); + await expect(page.getByRole('navigation', { name: 'By phase' })).toHaveCount(0); +}); + test('no page title repeats the brand', async ({ page }) => { // The root layout appends '| schoolcompare' to a plain-string title. Any // route whose title already carries the brand must opt out with @@ -1947,8 +1998,15 @@ test('a place straddling a boundary names every authority it sits in', async ({ 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(); + if (a.slug) { + await expect(page.locator(`a[href="/schools/authority/${a.slug}"]`).first()) + .toBeVisible(); + } else { + // No page of its own — City of London and the Isles of Scilly are + // under the threshold. Named, deliberately not linked. + await expect(page.locator('header p')).toContainText(a.name); + await expect(page.getByRole('link', { name: a.name })).toHaveCount(0); + } } }); diff --git a/nextjs-app/__tests__/components/PlaceView.test.tsx b/nextjs-app/__tests__/components/PlaceView.test.tsx index 3d6caf6..96bed7d 100644 --- a/nextjs-app/__tests__/components/PlaceView.test.tsx +++ b/nextjs-app/__tests__/components/PlaceView.test.tsx @@ -276,3 +276,73 @@ describe('PlaceView list ordering', () => { expect(list.itemListOrder).toBe('https://schema.org/ItemListOrderAscending'); }); }); + +describe('PlaceView phase links', () => { + const authority: PlaceDetail = { + place: { kind: 'authority', slug: 'barnet', name: 'Barnet', count: 156, + parent_authority: null, phases: ['primary', 'secondary'] }, + 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('keeps an authority phase link in the authority namespace', () => { + // The link was built as `/schools/${slug}/${phase}` for every kind, so an + // authority page pointed into the town namespace. For 87 of 151 + // authorities that 404'd; for the other 64 it silently landed on the town + // page of the same name — a different set of schools, and exactly the + // duplicate the two namespaces exist to prevent. Barnet is one of the 64. + render(); + expect(screen.getByRole('link', { name: /^Primary schools in Barnet$/ })) + .toHaveAttribute('href', '/schools/authority/barnet/primary'); + expect(screen.getByRole('link', { name: /^Secondary schools in Barnet$/ })) + .toHaveAttribute('href', '/schools/authority/barnet/secondary'); + }); + + it('still uses the bare namespace for a town', () => { + render(); + expect(screen.getByRole('link', { name: /^Primary schools in Brentwood$/ })) + .toHaveAttribute('href', '/schools/brentwood/primary'); + }); + + it('offers no phase link when the place publishes none', () => { + // Outcodes are the case: no phase route exists for them, so the registry + // reports no phases and the nav does not render. + const outcode = { ...detail, + place: { ...detail.place, kind: 'outcode', slug: 'cm13', name: 'CM13', + phases: [] } }; + render(); + expect(screen.queryByRole('navigation', { name: 'By phase' })) + .not.toBeInTheDocument(); + }); +}); + +describe('PlaceView unlinkable authorities', () => { + const withUnpublished: PlaceDetail = { + place: { kind: 'outcode', slug: 'tr21', name: 'TR21', count: 8, + parent_authority: 'Cornwall', phases: [], + authorities: [ + { name: 'Cornwall', slug: 'cornwall', count: 6 }, + { name: 'Isles Of Scilly', slug: null, count: 2 }, + ] }, + 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 an authority with no page without linking it', () => { + // City of London and the Isles of Scilly hold fewer schools than a page + // needs. Saying where the place is stays right; linking there would 404. + const { container } = render(); + expect(screen.getByRole('link', { name: 'Cornwall' })).toBeInTheDocument(); + expect(screen.queryByRole('link', { name: 'Isles Of Scilly' })) + .not.toBeInTheDocument(); + expect(container.querySelector('header p')?.textContent) + .toContain('Isles Of Scilly'); + }); +}); diff --git a/nextjs-app/app/schools/[place]/page.tsx b/nextjs-app/app/schools/[place]/page.tsx index 68a8652..08a6727 100644 --- a/nextjs-app/app/schools/[place]/page.tsx +++ b/nextjs-app/app/schools/[place]/page.tsx @@ -74,9 +74,14 @@ export default async function PlacePage({ params }: Props) { // defers to its authority rather than publishing a thin page. if (detail.averages.rwm_expected_pct == null && detail.averages.attainment_8_score == null) { - if (detail.place.parent_authority) { - redirect(`/schools/authority/${authoritySlug(detail.place.parent_authority)}`); - } + // The API's own slug, which is null when that authority is itself under + // the threshold and has no page. Re-slugifying the name here would send + // the reader to a 404 instead of telling them the place has no page. + const target = detail.place.authorities?.[0]?.slug + ?? (detail.place.parent_authority + ? authoritySlug(detail.place.parent_authority) + : null); + if (target) redirect(`/schools/authority/${target}`); notFound(); } diff --git a/nextjs-app/app/schools/authority/[la]/[phase]/page.tsx b/nextjs-app/app/schools/authority/[la]/[phase]/page.tsx new file mode 100644 index 0000000..21cc736 --- /dev/null +++ b/nextjs-app/app/schools/authority/[la]/[phase]/page.tsx @@ -0,0 +1,65 @@ +/** + * Phase variants of an authority page. + * + * The spec called for these; the plan built the bare authority route and + * dropped them. Nothing caught it, because the sitemap is written from the + * place registry — which was right about them all along — while the routes + * were written by hand. 302 authority phase URLs were submitted to Google and + * every one 404'd, and every authority page linked to a phase page in the + * *town* namespace, which is a different set of schools entirely. + * + * "Primary schools in Kent" is the query these serve, and it is a real one: + * admissions are authority-run, so the authority is the unit a parent thinks + * in when they have not settled on a town. + */ +import { notFound } from 'next/navigation'; +import type { Metadata } from 'next'; +import { fetchPlace } from '@/lib/places'; +import { fetchNationalAverages } from '@/lib/api'; +import { PlaceView } from '@/components/places/PlaceView'; +import { absoluteUrl } from '@/lib/site'; + +interface Props { params: Promise<{ la: string; phase: string }> } + +export const revalidate = 604800; +export const dynamicParams = true; + +const PHASES = ['primary', 'secondary'] as const; +type Phase = (typeof PHASES)[number]; + +const isPhase = (v: string): v is Phase => (PHASES as readonly string[]).includes(v); + +export async function generateMetadata({ params }: Props): Promise { + const { la, phase } = await params; + if (!isPhase(phase)) return { title: 'Place Not Found' }; + const detail = await fetchPlace('authority', la, phase); + if (!detail || detail.schools.length === 0) return { title: 'Place Not Found' }; + + const word = phase === 'secondary' ? 'Secondary' : 'Primary'; + const { name } = detail.place; + return { + // "Local Authority" stays in the title for the same reason it is on the + // bare authority page: 67 town names collide with an authority name, and + // a reader landing on both needs to know which set each covers. + title: { absolute: `${word} Schools in ${name} — Local Authority | schoolcompare` }, + description: + `Every ${phase} school in the ${name} local authority, with results, Ofsted ` + + `grades and the authority average against England.`, + alternates: { canonical: absoluteUrl(`/schools/authority/${la}/${phase}`) }, + }; +} + +export default async function AuthorityPhasePage({ params }: Props) { + const { la, phase } = await params; + if (!isPhase(phase)) notFound(); + const detail = await fetchPlace('authority', la, phase); + if (!detail || detail.schools.length === 0) notFound(); + + const national = await fetchNationalAverages().catch(() => null); + const englandAverage = phase === 'secondary' + ? national?.secondary?.attainment_8_score ?? null + : national?.primary?.rwm_expected_pct ?? null; + + return ; +} diff --git a/nextjs-app/components/places/PlaceView.tsx b/nextjs-app/components/places/PlaceView.tsx index 15ec0c3..20fc831 100644 --- a/nextjs-app/components/places/PlaceView.tsx +++ b/nextjs-app/components/places/PlaceView.tsx @@ -181,11 +181,18 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props) 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} - + {/* No slug means no page: City of London and the Isles of + Scilly hold too few schools for one. Saying where the + place is stays right; linking there would 404. */} + {a.slug + ? ( + + {a.name} + + ) + : a.name} ))} @@ -196,7 +203,11 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props) {!phase && (place.phases ?? []).length > 0 && (