diff --git a/backend/app.py b/backend/app.py index 934f0e4..1eb0837 100644 --- a/backend/app.py +++ b/backend/app.py @@ -209,12 +209,27 @@ def _place_url(place) -> str: def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]: - return [ - _url_element(BASE_URL + _place_url(p)) - for p in sorted(get_place_registry().values(), - key=lambda p: (p.kind, p.slug)) - if p.kind in kinds - ] + """A per place, plus a phase variant wherever that phase clears the + threshold on its own. + + Phase is part of the query — "primary schools in beccles" — so each + variant is its own indexable page. Submitting only the bare place URL left + ~950 of them reachable by nothing: absent from every sitemap, and not + linked from the place page either. + """ + rows: list[str] = [] + for p in sorted(get_place_registry().values(), key=lambda p: (p.kind, p.slug)): + 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 + for phase in ("primary", "secondary"): + if p.publishes_phase(phase): + rows.append(_url_element(f"{BASE_URL}{_place_url(p)}/{phase}")) + return rows def build_sitemaps() -> dict[str, str]: @@ -1216,7 +1231,11 @@ async def get_place(request: Request, kind: str, slug: str, return { "place": {"kind": place.kind, "slug": place.slug, "name": place.name, "count": len(place.urns), - "parent_authority": place.parent_authority}, + "parent_authority": place.parent_authority, + # Only phases that clear the threshold, so the page links + # variants that exist rather than 404s. + "phases": [ph for ph in ("primary", "secondary") + if place.publishes_phase(ph)]}, "schools": clean_for_json(rows[cols]), "averages": averages, } diff --git a/backend/places.py b/backend/places.py index 5cab181..a9bb3e6 100644 --- a/backend/places.py +++ b/backend/places.py @@ -14,7 +14,7 @@ from __future__ import annotations import logging import re -from dataclasses import dataclass +from dataclasses import dataclass, field logger = logging.getLogger(__name__) @@ -30,6 +30,13 @@ class Place: name: str urns: tuple[int, ...] parent_authority: str | None # authority NAME, for the 301 target + # 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. + phase_urns: dict[str, tuple[int, ...]] = field(default_factory=dict) + + def publishes_phase(self, phase: str) -> bool: + return len(self.phase_urns.get(phase, ())) >= MIN_SCHOOLS @property def key(self) -> str: @@ -46,6 +53,24 @@ def _publishable_urns(df) -> set[int]: return set(df.loc[df[cols].notna().any(axis=1), "urn"].astype(int)) +def _phase_urns(group, publishable: set[int]) -> dict[str, tuple[int, ...]]: + """URNs per phase. All-through schools count toward both, matching the + PHASE_GROUPS mapping the search filters already use.""" + from backend.app import PHASE_GROUPS + + if "phase" not in group.columns: + return {} + lowered = group["phase"].fillna("").str.lower() + out: dict[str, tuple[int, ...]] = {} + for phase in ("primary", "secondary"): + wanted = PHASE_GROUPS.get(phase, set()) + subset = group[lowered.isin(wanted)] + urns = tuple(sorted({int(u) for u in subset["urn"]} & publishable)) + if urns: + out[phase] = urns + return out + + def _parent_authority(group) -> str | None: """The most common authority in a group — the useful 301 target. @@ -79,6 +104,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, + phase_urns=_phase_urns(group, publishable), ) out[place.key] = place return out @@ -111,7 +137,8 @@ def _outcode_places(df, publishable: set[int]) -> dict[str, Place]: if len(urns) < MIN_SCHOOLS: continue place = Place(kind="outcode", slug=str(oc).lower(), name=str(oc), - urns=urns, parent_authority=_parent_authority(group)) + urns=urns, parent_authority=_parent_authority(group), + phase_urns=_phase_urns(group, publishable)) out[place.key] = place return out @@ -153,7 +180,8 @@ def _locality_places(df, publishable: set[int], slug, ", ".join(outcodes), len(urns), MIN_SCHOOLS) continue place = Place(kind="locality", slug=slug, name=name, urns=urns, - parent_authority=_parent_authority(group)) + parent_authority=_parent_authority(group), + phase_urns=_phase_urns(group, publishable)) out[place.key] = place return out diff --git a/backend/tests/test_sitemap.py b/backend/tests/test_sitemap.py index 21d44ff..142b572 100644 --- a/backend/tests/test_sitemap.py +++ b/backend/tests/test_sitemap.py @@ -268,3 +268,23 @@ def test_place_urls_carry_no_priority_or_changefreq(place_sitemaps): for name in ("places-1.xml", "outcodes-1.xml"): assert "" not in place_sitemaps[name] assert "" not in place_sitemaps[name] + + +def test_phase_variants_are_submitted_where_the_phase_clears_the_threshold(place_sitemaps): + # "primary schools in beccles" is the query shape the baseline showed, so + # each variant is its own page and has to be submitted. Emitting only the + # bare place URL left ~950 of them reachable by nothing. + xml = place_sitemaps["places-1.xml"] + assert "https://www.schoolcompare.co.uk/schools/brentwood/primary" in xml + + +def test_a_phase_below_its_own_threshold_is_not_submitted(place_sitemaps): + # The fixture is six primaries and no secondaries. + xml = place_sitemaps["places-1.xml"] + assert "/schools/brentwood/secondary" not in xml + + +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 diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index f6b1538..430fb5e 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1842,3 +1842,29 @@ test('a place page states the local average against England', async ({ page }) = await page.goto(`/schools/${place.slug}`); await expect(page.getByTestId('local-vs-england')).toContainText(/across England/i); }); + +test('a place page links its phase variants, and they resolve', async ({ page }) => { + // "primary schools in beccles" is the query shape the baseline showed. The + // first cut submitted only the bare place URL and linked nothing, leaving + // ~950 variant pages reachable by nothing at all. + const res = await page.request.get('/api/places'); + const { places } = await res.json(); + const town = places.find((p: { kind: string }) => p.kind === 'town'); + expect(town).toBeTruthy(); + + const detail = await (await page.request.get(`/api/places/town/${town.slug}`)).json(); + test.skip(!(detail.place.phases ?? []).length, 'no phase clears the threshold here'); + + await page.goto(`/schools/${town.slug}`); + const phase = detail.place.phases[0]; + const link = page.locator(`a[href="/schools/${town.slug}/${phase}"]`).first(); + await expect(link).toBeVisible(); + + await link.click(); + await expect(page.locator('h1')).toContainText(new RegExp(`${phase} schools in`, 'i')); +}); + +test('phase variants are submitted in the places sitemap', async ({ page }) => { + const xml = await (await page.request.get('/sitemaps/places-1.xml')).text(); + expect(xml).toMatch(/\/schools\/[a-z0-9-]+\/primary { } }); }); + +describe('PlaceView phase variants', () => { + it('links the phase variants that exist', () => { + render(); + expect(screen.getByRole('link', { name: /Primary schools in Brentwood/i })) + .toHaveAttribute('href', '/schools/brentwood/primary'); + }); + + it('links no variant for a phase below its own threshold', () => { + render(); + expect(screen.queryByRole('link', { name: /Secondary schools in Brentwood/i })) + .not.toBeInTheDocument(); + }); + + it('does not link sideways from a variant page to itself', () => { + render(); + expect(screen.queryByRole('link', { name: /Primary schools in Brentwood/i })) + .not.toBeInTheDocument(); + }); +}); diff --git a/nextjs-app/components/places/PlaceView.module.css b/nextjs-app/components/places/PlaceView.module.css index 326d778..5aec3a3 100644 --- a/nextjs-app/components/places/PlaceView.module.css +++ b/nextjs-app/components/places/PlaceView.module.css @@ -106,3 +106,13 @@ padding: 0; margin: 0; } + +/* Phase variants are separate indexable pages, so the bare place page has to + link them — a sitemap entry alone leaves them with no internal path in. */ +.phaseLinks { + display: flex; + flex-wrap: wrap; + gap: 0.5rem 1rem; + margin: 0 0 1.25rem; + font-size: 0.9375rem; +} diff --git a/nextjs-app/components/places/PlaceView.tsx b/nextjs-app/components/places/PlaceView.tsx index 0e063ac..db37692 100644 --- a/nextjs-app/components/places/PlaceView.tsx +++ b/nextjs-app/components/places/PlaceView.tsx @@ -89,6 +89,16 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)

+ {!phase && (place.phases ?? []).length > 0 && ( + + )} + {local != null && englandAverage != null && (

{place.name} averages {Math.round(local)} against{' '} diff --git a/nextjs-app/lib/places.ts b/nextjs-app/lib/places.ts index 2c6b392..d1be648 100644 --- a/nextjs-app/lib/places.ts +++ b/nextjs-app/lib/places.ts @@ -13,6 +13,9 @@ export interface PlaceSummary { slug: string; name: string; count: number; + /** Phases that clear the threshold on their own, so the page links + * variants that exist rather than 404s. Absent on the registry listing. */ + phases?: string[]; } export interface PlaceDetail {