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 && (