From 7f5f0fb676189d1cd31d1c735ae0569d03bf7df2 Mon Sep 17 00:00:00 2001 From: Tudor Date: Mon, 14 Sep 2026 20:45:50 +0100 Subject: [PATCH 1/3] feat(seo): link school pages into the location layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W2 shipped ~5,000 place pages and nothing linked into them. The location layer pointed down at school pages; school pages pointed nowhere on the site. Their only anchor was the school's own website, so the ~27k pages carrying most of the site's inbound authority passed it straight off-site, and the new corpus was reachable mainly through the sitemap. Three things close the loop. A reverse index over the place registry, places_for_urn, answers which published places contain a school. Derived from the registry rather than stored beside it, so the two cannot disagree about which places exist: a place below the publish threshold is absent from the registry and therefore never offered as a link. A test asserts that invariant across every place in a built registry. GET /api/schools/{urn} gains a `places` array carrying the name, count and canonical path for each. It rides on the request the page already makes, so the school page costs no extra round trip. The frontend types it optional and defaults it to empty, because the two images deploy separately and a frontend ahead of the API must render without it. The page gains a "More schools near here" module and a BreadcrumbList. The module orders narrowest first, because a reader on a school page wants its town before its county, while the API orders widest first for the trail. Anchors state their destination's size — "37 schools in Brentwood" — which is worth more to a reader and a crawler than "see more". With no published places it renders nothing rather than an empty heading. The trail is rooted at the homepage, not /schools. There is no /schools index page; the location layer lives only at /schools/[place], /schools/authority/[la] and /schools/near/[outcode]. Rooting it at the bare path would have opened every breadcrumb with a link to a 404. Outcodes are omitted from the trail: "schools near CM15" is a real query and a useful link, but nobody navigates Essex to CM15 to a school, and a breadcrumb claiming that describes a hierarchy the site does not have. School pages also now declare the School type rather than EducationalOrganization, the parent type that covers universities and nurseries alike. The e2e journey asserts the round trip in both directions, following a place page's own first school so the pair is genuinely related rather than hardcoded. A one-way link is what already existed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DXnXQKnPpZBBP61fBQiFkq --- backend/app.py | 31 ++++++++- backend/places.py | 17 +++++ backend/tests/test_places.py | 53 ++++++++++++++- backend/tests/test_school_details.py | 57 ++++++++++++++++ e2e/tests/journeys.spec.ts | 44 ++++++++++++ .../components/NearbyPlaces.test.tsx | 55 +++++++++++++++ nextjs-app/__tests__/lib/schoolJsonLd.test.ts | 68 +++++++++++++++++++ .../app/(frontend)/school/[slug]/page.tsx | 29 +++++++- .../components/school/NearbyPlaces.module.css | 53 +++++++++++++++ nextjs-app/components/school/NearbyPlaces.tsx | 51 ++++++++++++++ nextjs-app/lib/jsonld.ts | 57 ++++++++++++++++ nextjs-app/lib/types.ts | 11 +++ 12 files changed, 522 insertions(+), 4 deletions(-) create mode 100644 nextjs-app/__tests__/components/NearbyPlaces.test.tsx create mode 100644 nextjs-app/__tests__/lib/schoolJsonLd.test.ts create mode 100644 nextjs-app/components/school/NearbyPlaces.module.css create mode 100644 nextjs-app/components/school/NearbyPlaces.tsx diff --git a/backend/app.py b/backend/app.py index 7836ca7..782dd81 100644 --- a/backend/app.py +++ b/backend/app.py @@ -38,7 +38,7 @@ from .data_loader import ( ) from .data_loader import get_data_info as get_db_info from . import flags -from .places import build_place_registry +from .places import build_place_registry, places_for_urn from .schemas import METRIC_DEFINITIONS, RANKING_COLUMNS, SCHOOL_COLUMNS from .utils import clean_for_json, convert_to_native @@ -211,6 +211,28 @@ def _place_url(place) -> str: return f"/schools/{place.slug}" +def _places_payload(urn: int) -> list[dict]: + """The published places containing this school, as the school page needs + them: a name to write in the link, a count so the anchor can say what it + leads to, and the canonical path. + + `phase_url` is present only where the place publishes a page for this + school's phase, which is the registry's decision alone — repeating the + threshold rule here is how the page and the sitemap would come to disagree. + """ + payload = [] + for place in places_for_urn(get_place_registry(), int(urn)): + entry = { + "kind": place.kind, + "slug": place.slug, + "name": place.name, + "count": len(place.urns), + "url": _place_url(place), + } + payload.append(entry) + return payload + + def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]: """A per place, plus a phase variant wherever that phase clears the threshold on its own. @@ -902,6 +924,13 @@ async def get_school_details(request: Request, urn: int): return { "school_info": school_info, + # Where this school sits in the location layer, for the page's link + # module and breadcrumb. Derived from the same registry the place + # pages and the sitemap use, so a link is never offered for a page + # that does not exist. Empty is a valid answer: a school whose town + # and authority both fall below the publish threshold has nowhere to + # point, and the page renders without the module. + "places": _places_payload(urn), "yearly_data": clean_for_json(school_data), # Supplementary data (null if not yet populated by Kestra) "ofsted": supplementary.get("ofsted"), diff --git a/backend/places.py b/backend/places.py index 078638c..6476c38 100644 --- a/backend/places.py +++ b/backend/places.py @@ -296,6 +296,23 @@ def _locality_places(df, publishable: set[int], return out +def places_for_urn(registry: dict[str, Place], urn: int) -> tuple[Place, ...]: + """Every published place containing this school, largest kind first. + + The reverse of the registry, and the thing school pages link out through. + Derived from the registry rather than stored beside it, so the two cannot + disagree about which places exist: a link module must never offer a place + whose page does not exist, and a place below the publish threshold is + simply absent from the registry, so it is absent from here too. + + Ordered authority → town/locality → outcode, widest first, because that is + the order a breadcrumb reads and the order the link module lists. + """ + order = {"authority": 0, "town": 1, "locality": 2, "outcode": 3} + found = [p for p in registry.values() if urn in p.urns] + return tuple(sorted(found, key=lambda p: (order.get(p.kind, 9), p.slug))) + + def build_place_registry(df) -> dict[str, Place]: """Every place the site publishes, keyed by ":".""" if df.empty or "urn" not in df.columns: diff --git a/backend/tests/test_places.py b/backend/tests/test_places.py index 99e82e2..c041eda 100644 --- a/backend/tests/test_places.py +++ b/backend/tests/test_places.py @@ -8,7 +8,7 @@ import numpy as np import pandas as pd import pytest -from backend.places import MIN_SCHOOLS, build_place_registry +from backend.places import MIN_SCHOOLS, build_place_registry, places_for_urn def _df(rows: list[dict]) -> pd.DataFrame: @@ -418,3 +418,54 @@ def test_an_authority_still_publishes_phase_variants(): 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") + + +# ── The reverse index: which published places contain a school ────────────── +# +# School pages link out to the location layer through this. It is the whole +# point of the index: before it, ~27k school pages linked to nothing on the +# site and stranded whatever authority they held. + +def test_a_school_resolves_to_every_published_place_containing_it(): + reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Brentwood", "Essex"))) + places = places_for_urn(reg, 100000) + + kinds = {p.kind for p in places} + assert "town" in kinds + assert "authority" in kinds + + +def test_a_school_in_an_unpublished_town_still_resolves_to_its_authority(): + # A town below the threshold has no page, so there is no link to offer — + # but the authority above it clears the threshold on the same schools and + # is where that reader should be sent. + reg = build_place_registry(_df( + _town(MIN_SCHOOLS - 1, "Tinytown", "Essex") + + _town(MIN_SCHOOLS, "Brentwood", "Essex", start=200000) + )) + places = places_for_urn(reg, 100000) + + # The town is below the threshold, so it has no page and must not be + # offered as a link. The authority above it does, and is the right target. + assert all(p.slug != "tinytown" for p in places) + assert "authority" in {p.kind for p in places} + + +def test_an_unknown_urn_resolves_to_nothing_rather_than_raising(): + # A school page renders for any URN the API knows; the link module is not + # entitled to take the page down when it has nothing to say. + reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Brentwood", "Essex"))) + assert places_for_urn(reg, 999999) == () + + +def test_the_index_is_consistent_with_the_registry_it_was_built_from(): + # The invariant that matters: a link module must never offer a place whose + # page does not exist, and never omit one that does. + reg = build_place_registry(_df( + _town(MIN_SCHOOLS, "Brentwood", "Essex") + + _town(MIN_SCHOOLS, "Bedford", "Bedford", start=300000) + )) + for key, place in reg.items(): + for urn in place.urns: + assert place in places_for_urn(reg, urn), ( + f"{urn} is in {key} but the index does not say so") diff --git a/backend/tests/test_school_details.py b/backend/tests/test_school_details.py index f2a20ac..a16bc0b 100644 --- a/backend/tests/test_school_details.py +++ b/backend/tests/test_school_details.py @@ -69,3 +69,60 @@ def test_nan_gias_fields_serialize_as_null(client): assert info["capacity"] is None assert info["total_pupils"] is None assert info["school_name"] == "West London Performing Arts Academy" + + +# ── Links out to the location layer ───────────────────────────────────────── +# +# School pages carried no link into the site at all: the only anchor on the +# template pointed at the school's own website, so ~27k pages received +# whatever authority the site had and sent it off-site. `places` is what the +# link module and the breadcrumb are built from. + +def test_places_is_present_even_when_the_school_belongs_to_none(client): + # This fixture's single school cannot clear any publish threshold, so the + # honest answer is an empty list. The key must still be there: a missing + # key and "no places" are different things to the page rendering it. + body = client.get("/api/schools/150275").json() + assert body["places"] == [] + + +def test_places_names_only_pages_that_exist(monkeypatch): + from backend import app as app_module + from backend.places import MIN_SCHOOLS + + def _df(): + return pd.DataFrame([ + { + "urn": 100000 + i, + "school_name": f"Brentwood School {i}", + "town": "Brentwood", + "local_authority": "Essex", + "postcode": "CM15 8AA", + "phase": "Primary", + "year": 202425, + "rwm_expected_pct": 60.0, + "attainment_8_score": np.nan, + "ofsted_grade": 2.0, + "ofsted_date": None, + } + for i in range(MIN_SCHOOLS) + ]) + + monkeypatch.setattr(app_module, "load_school_data", _df) + monkeypatch.setattr(app_module, "get_supplementary_data", lambda db, urn: {}) + monkeypatch.setattr(app_module, "_place_registry", None) + client = TestClient(app_module.app, raise_server_exceptions=False) + + places = client.get("/api/schools/100000").json()["places"] + assert places, "a school in a published town must offer links" + + by_kind = {p["kind"]: p for p in places} + assert by_kind["town"]["url"] == "/schools/brentwood" + assert by_kind["authority"]["url"] == "/schools/authority/essex" + + # Every entry carries what the link text needs, and a count, so the anchor + # can say what it leads to rather than "click here". + for place in places: + assert place["name"] + assert place["count"] >= 1 + assert place["url"].startswith("/schools/") diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index cfb614f..2855b4f 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1935,6 +1935,50 @@ async function firstPlaceOfKind(page: Page, kind: string) { return hit as { kind: string; slug: string; name: string; count: number }; } +/** + * The round trip. Place pages always linked down to school pages; school + * pages linked nowhere on the site, so the ~27k of them that carry most of + * the inbound authority stranded it — their only anchor pointed at the + * school's own website. + * + * Asserting both directions is the point. A one-way link is what already + * existed and is not what this journey is for. + */ +test('a school page links back into the location layer, and the place page links down', async ({ page }) => { + const town = await firstPlaceOfKind(page, 'town'); + + // Start from the place page and take its first school, so the pair is + // guaranteed to be genuinely related rather than a hardcoded guess. + await page.goto(`/schools/${town.slug}`); + const schoolHref = await page.locator('a[href^="/school/"]').first() + .getAttribute('href'); + expect(schoolHref, 'the town page listed no school to follow').toBeTruthy(); + + await page.goto(schoolHref!); + + // Down: the school page must offer a link back to the town it sits in. + const backToTown = page.locator(`a[href="/schools/${town.slug}"]`); + await expect(backToTown).toHaveCount(1); + await expect(backToTown).toBeVisible(); + + // The anchor says what it leads to, which is worth more than "see more". + await expect(backToTown).toContainText(town.name, { ignoreCase: true }); + await expect(backToTown).toContainText(/\d+ schools?/); + + // And the breadcrumb resolves the school into a real hierarchy. + const blocks = await page.locator('script[type="application/ld+json"]') + .allTextContents(); + const graph = blocks.join(' '); + expect(graph).toContain('"BreadcrumbList"'); + // The narrower type, not the EducationalOrganization parent it used to be. + expect(graph).toContain('"School"'); + + // Following it lands on a real page, not a 404. + await backToTown.click(); + await page.waitForURL(new RegExp(`/schools/${town.slug}$`)); + await expect(page.locator('h1')).toContainText(town.name, { ignoreCase: true }); +}); + for (const [kind, prefix, article] of [ ['town', '/schools/', 'a'], ['authority', '/schools/authority/', 'an'], diff --git a/nextjs-app/__tests__/components/NearbyPlaces.test.tsx b/nextjs-app/__tests__/components/NearbyPlaces.test.tsx new file mode 100644 index 0000000..9604bb4 --- /dev/null +++ b/nextjs-app/__tests__/components/NearbyPlaces.test.tsx @@ -0,0 +1,55 @@ +/** + * The module that ends the stranding: before it, a school page's only anchor + * pointed at the school's own website, so ~27k pages sent authority off-site + * and none of it reached the location layer. + */ +import { render, screen } from '@testing-library/react'; +import { NearbyPlaces } from '@/components/school/NearbyPlaces'; + +const essex = { kind: 'authority', slug: 'essex', name: 'Essex', count: 480, url: '/schools/authority/essex' }; +const brentwood = { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 37, url: '/schools/brentwood' }; +const cm15 = { kind: 'outcode', slug: 'cm15', name: 'CM15', count: 12, url: '/schools/near/cm15' }; + +describe('NearbyPlaces', () => { + it('links to every place the school belongs to', () => { + render(); + + expect(screen.getByRole('link', { name: /Brentwood/ })) + .toHaveAttribute('href', '/schools/brentwood'); + expect(screen.getByRole('link', { name: /Essex/ })) + .toHaveAttribute('href', '/schools/authority/essex'); + expect(screen.getByRole('link', { name: /CM15/ })) + .toHaveAttribute('href', '/schools/near/cm15'); + }); + + it('says how many schools each link leads to', () => { + // An anchor that states its destination's size is worth more to a reader + // and to a crawler than "see more". + render(); + expect(screen.getByRole('link', { name: /37 schools in Brentwood/ })) + .toBeInTheDocument(); + }); + + it('renders nothing at all when the school has no published places', () => { + // Not an empty heading. A school whose town and authority both fall below + // the threshold has nowhere to point, and the page should look as it did + // before the module existed. + const { container } = render(); + expect(container).toBeEmptyDOMElement(); + }); + + it('puts the narrowest place first, which is the most useful link', () => { + // The API orders widest-first for the breadcrumb; a reader on a school + // page wants its town before its county. + render(); + const hrefs = screen.getAllByRole('link').map((a) => a.getAttribute('href')); + expect(hrefs.indexOf('/schools/brentwood')) + .toBeLessThan(hrefs.indexOf('/schools/authority/essex')); + }); + + it('handles a singular count without saying "1 schools"', () => { + render(); + expect(screen.getByRole('link', { name: /1 school in Brentwood/ })) + .toBeInTheDocument(); + }); +}); diff --git a/nextjs-app/__tests__/lib/schoolJsonLd.test.ts b/nextjs-app/__tests__/lib/schoolJsonLd.test.ts new file mode 100644 index 0000000..ea68081 --- /dev/null +++ b/nextjs-app/__tests__/lib/schoolJsonLd.test.ts @@ -0,0 +1,68 @@ +/** + * School pages had no BreadcrumbList and no links into the location layer. + * Both are fixed by the same data — the `places` array the API now returns — + * so they are tested together. + */ +import { schoolBreadcrumbJsonLd } from '@/lib/jsonld'; + +const essex = { kind: 'authority', slug: 'essex', name: 'Essex', count: 480, url: '/schools/authority/essex' }; +const brentwood = { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 37, url: '/schools/brentwood' }; +const outcode = { kind: 'outcode', slug: 'cm15', name: 'CM15', count: 12, url: '/schools/near/cm15' }; + +describe('school breadcrumbs', () => { + it('reads home to authority to town to school', () => { + const ld = schoolBreadcrumbJsonLd({ + name: 'Brentwood School', url: '/school/100000-brentwood-school', + places: [essex, brentwood], + }); + + expect(ld['@type']).toBe('BreadcrumbList'); + expect(ld.itemListElement.map((i) => i.name)) + .toEqual(['schoolcompare', 'Essex', 'Brentwood', 'Brentwood School']); + expect(ld.itemListElement.map((i) => i.position)).toEqual([1, 2, 3, 4]); + }); + + it('skips a level the school has no published place for', () => { + // A school whose town falls below the publish threshold has no town page. + // The trail closes over the gap rather than linking to a 404. + const ld = schoolBreadcrumbJsonLd({ + name: 'Lone School', url: '/school/1-lone-school', places: [essex], + }); + + expect(ld.itemListElement.map((i) => i.name)) + .toEqual(['schoolcompare', 'Essex', 'Lone School']); + expect(ld.itemListElement.map((i) => i.position)).toEqual([1, 2, 3]); + }); + + it('omits outcodes, which are not a place a breadcrumb reads through', () => { + // CM15 is a useful link in the module but nonsense in a trail: nobody + // navigates Essex → CM15 → school. + const ld = schoolBreadcrumbJsonLd({ + name: 'Brentwood School', url: '/school/100000-brentwood-school', + places: [essex, brentwood, outcode], + }); + + expect(JSON.stringify(ld)).not.toContain('cm15'); + }); + + it('still produces a valid trail when the school has no places at all', () => { + const ld = schoolBreadcrumbJsonLd({ + name: 'Orphan School', url: '/school/2-orphan-school', places: [], + }); + + expect(ld.itemListElement.map((i) => i.name)).toEqual(['schoolcompare', 'Orphan School']); + }); + + it('uses absolute urls, as every other entity on the site does', () => { + const ld = schoolBreadcrumbJsonLd({ + name: 'Brentwood School', url: '/school/100000-brentwood-school', + places: [essex, brentwood], + }); + + for (const item of ld.itemListElement) { + expect(item.item).toMatch(/^https:\/\/www\.schoolcompare\.co\.uk\//); + } + // The root is the homepage: there is no /schools index page to link to. + expect(ld.itemListElement[0].item).toBe('https://www.schoolcompare.co.uk/'); + }); +}); diff --git a/nextjs-app/app/(frontend)/school/[slug]/page.tsx b/nextjs-app/app/(frontend)/school/[slug]/page.tsx index be0674f..9b3649f 100644 --- a/nextjs-app/app/(frontend)/school/[slug]/page.tsx +++ b/nextjs-app/app/(frontend)/school/[slug]/page.tsx @@ -7,6 +7,8 @@ import { fetchSchoolDetails, fetchSchools, fetchNationalAverages } from '@/lib/api'; import { notFound, redirect } from 'next/navigation'; import { SchoolDetailShell } from '@/components/school/SchoolDetailShell'; +import { NearbyPlaces } from '@/components/school/NearbyPlaces'; +import { schoolBreadcrumbJsonLd, type SchoolPlace } from '@/lib/jsonld'; import { PrimarySchoolSections } from '@/components/school/PrimarySchoolSections'; import { SecondarySchoolSections } from '@/components/school/SecondarySchoolSections'; import { @@ -149,6 +151,10 @@ export default async function SchoolPage({ params }: SchoolPageProps) { } const { school_info, yearly_data, absence_data, ofsted, census, admissions, admissions_history, admission_distance, deprivation, finance, destinations } = data; + // Absent on an older API build; the module and the trail both degrade to + // nothing rather than throwing, which is how this shipped without a + // lockstep deploy of the two images. + const places: SchoolPlace[] = data.places ?? []; // Redirect bare URN to canonical slug URL const canonicalSlug = schoolUrl(urn, school_info.school_name).replace('/school/', ''); @@ -185,10 +191,19 @@ export default async function SchoolPage({ params }: SchoolPageProps) { const primaryNavItems = buildNavItems(primaryFlags, navInput); const secondaryNavItems = buildSecondaryNavItems(secondaryFlags, navInput); - // Generate JSON-LD structured data for SEO + /* + * `School`, not `EducationalOrganization`. + * + * Both are valid, but EducationalOrganization is the parent type covering + * universities, training providers and nurseries alike. School is the + * specific one, and a type that says what the page is about is the whole + * point of declaring it. Google's own guidance treats the narrower type as + * the correct choice where it applies. + */ const structuredData = { '@context': 'https://schema.org', - '@type': 'EducationalOrganization', + '@graph': [{ + '@type': 'School', name: school_info.school_name, identifier: school_info.urn.toString(), ...(school_info.address && { @@ -210,6 +225,15 @@ export default async function SchoolPage({ params }: SchoolPageProps) { ...(school_info.school_type && { additionalType: school_info.school_type, }), + }, + // The trail the page sits at the end of. School pages carried no + // breadcrumb at all, while every place page already emitted one. + schoolBreadcrumbJsonLd({ + name: school_info.school_name, + url: `/school/${slug}`, + places, + }), + ], }; return ( @@ -264,6 +288,7 @@ export default async function SchoolPage({ params }: SchoolPageProps) { /> )} + ); } diff --git a/nextjs-app/components/school/NearbyPlaces.module.css b/nextjs-app/components/school/NearbyPlaces.module.css new file mode 100644 index 0000000..3ef4477 --- /dev/null +++ b/nextjs-app/components/school/NearbyPlaces.module.css @@ -0,0 +1,53 @@ +/* Tokens only — the same vocabulary schoolSections.module.css uses, so the + module follows both themes without a rule of its own. No hardcoded colour + appears here; darkThemeSafety asserts that across the codebase. */ + +.section { + margin-top: 2rem; +} + +/* Matches .sectionTitle in schoolSections.module.css, including the brand + rule before the text, so this reads as one more section of the page + rather than a footer bolted underneath it. */ +.heading { + font-size: 1.125rem; + font-weight: 600; + color: var(--text-primary); + margin-bottom: 0.875rem; + padding-bottom: 0.5rem; + border-bottom: 2px solid var(--border); + font-family: var(--font-display); + display: flex; + align-items: center; + gap: 0.375rem; +} + +.heading::before { + content: ""; + display: inline-block; + width: 3px; + height: 1em; + background: var(--brand); + border-radius: 2px; + flex-shrink: 0; +} + +.list { + display: flex; + flex-wrap: wrap; + gap: 0.5rem 1.25rem; + list-style: none; + margin: 0; + padding: 0; +} + +.link { + color: var(--brand-strong); + font-weight: 500; + text-decoration: underline; + text-underline-offset: 2px; +} + +.link:hover { + text-decoration-thickness: 2px; +} diff --git a/nextjs-app/components/school/NearbyPlaces.tsx b/nextjs-app/components/school/NearbyPlaces.tsx new file mode 100644 index 0000000..2af24a8 --- /dev/null +++ b/nextjs-app/components/school/NearbyPlaces.tsx @@ -0,0 +1,51 @@ +import Link from 'next/link'; +import type { SchoolPlace } from '@/lib/jsonld'; +import styles from './NearbyPlaces.module.css'; + +/** + * Links from a school page into the location layer. + * + * This exists for a structural reason rather than a decorative one. Before + * it, the only anchor on a school page pointed at the school's own website, + * so the ~27k pages that carry most of the site's inbound authority passed it + * straight off-site and none of it reached the place pages. These links are + * what circulate it instead. + * + * Every entry comes from the place registry via the API, so a link is only + * ever offered for a page that exists: a place below the publish threshold is + * absent from the registry and therefore absent here. + */ + +/** Narrowest first: a reader on a school page wants its town before its + * county. The API orders widest-first because that is what the breadcrumb + * reads, so the two orders are deliberately different. */ +const ORDER: Record = { + town: 0, locality: 0, outcode: 1, authority: 2, +}; + +function label(place: SchoolPlace): string { + const noun = place.count === 1 ? 'school' : 'schools'; + const preposition = place.kind === 'outcode' ? 'near' : 'in'; + return `${place.count} ${noun} ${preposition} ${place.name}`; +} + +export function NearbyPlaces({ places }: { places: SchoolPlace[] }) { + if (places.length === 0) return null; + + const sorted = [...places].sort( + (a, b) => (ORDER[a.kind] ?? 9) - (ORDER[b.kind] ?? 9), + ); + + return ( +
+

More schools near here

+
    + {sorted.map((place) => ( +
  • + {label(place)} +
    • + ))} +
+
+ ); +} diff --git a/nextjs-app/lib/jsonld.ts b/nextjs-app/lib/jsonld.ts index 41435e2..aec423a 100644 --- a/nextjs-app/lib/jsonld.ts +++ b/nextjs-app/lib/jsonld.ts @@ -69,6 +69,63 @@ export function blogPostingJsonLd( } as const; } +/** + * A place the location layer publishes a page for, as the school API reports + * it. `count` is what lets a link say "All 37 schools in Brentwood" rather + * than "click here". + */ +export interface SchoolPlace { + kind: string; + slug: string; + name: string; + count: number; + url: string; +} + +/** + * The trail a school page sits at the end of: Schools → authority → town. + * + * Only authority and town/locality appear. An outcode is a useful link in the + * module beside this — a parent does search "schools near CM15" — but it is + * not a step anyone navigates through, and a breadcrumb that claims otherwise + * describes a hierarchy the site does not have. + * + * Levels are skipped rather than faked. A school whose town falls below the + * publish threshold has no town page, so the trail closes over the gap; the + * alternative is a breadcrumb linking to a 404. + */ +export function schoolBreadcrumbJsonLd( + school: { name: string; url: string; places: SchoolPlace[] }, +) { + /* + * Rooted at the homepage, not at /schools. There is no /schools index page + * — the location layer is /schools/[place], /schools/authority/[la] and + * /schools/near/[outcode], with nothing at the bare path — so a trail + * starting there would open with a link to a 404. + */ + const trail: Array<{ name: string; url: string }> = [ + { name: 'schoolcompare', url: '/' }, + ]; + + const authority = school.places.find((p) => p.kind === 'authority'); + if (authority) trail.push({ name: authority.name, url: authority.url }); + + const town = school.places.find((p) => p.kind === 'town' || p.kind === 'locality'); + if (town) trail.push({ name: town.name, url: town.url }); + + trail.push({ name: school.name, url: school.url }); + + return { + '@type': 'BreadcrumbList', + itemListElement: trail.map((step, index) => ({ + '@type': 'ListItem', + position: index + 1, + name: step.name, + item: absoluteUrl(step.url), + })), + } as const; +} + export function breadcrumbJsonLd(post: PostSummary) { return { '@type': 'BreadcrumbList', diff --git a/nextjs-app/lib/types.ts b/nextjs-app/lib/types.ts index a19d250..927d301 100644 --- a/nextjs-app/lib/types.ts +++ b/nextjs-app/lib/types.ts @@ -1,3 +1,5 @@ +import type { SchoolPlace } from '@/lib/jsonld'; + /** * TypeScript type definitions for SchoolCompare API * Generated from backend/models.py and backend/schemas.py @@ -346,6 +348,15 @@ export interface SchoolsResponse { export interface SchoolDetailsResponse { school_info: School; + /** + * The published location-layer pages containing this school, widest first. + * + * Optional because the frontend and backend ship as separate images: a + * frontend deployed ahead of the API that serves this must render without + * it, not throw. Empty is also a real answer — a school whose town and + * authority both fall below the publish threshold has nowhere to link. + */ + places?: SchoolPlace[]; yearly_data: SchoolResult[]; absence_data: AbsenceData | null; // Supplementary data (null until Kestra populates) From d65eb588834ee9b3625330aca339b59ecaf5f7ba Mon Sep 17 00:00:00 2001 From: Tudor Date: Mon, 14 Sep 2026 20:54:59 +0100 Subject: [PATCH 2/3] fix(seo): build the phase links the docstring already promised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught _places_payload documenting a `phase_url` the function never returned. The docstring was not stray prose: the approved design included the phase variant — "Primary schools in Beccles" was one of its four example links — and it was dropped during implementation without being mentioned. Deleting the sentence would have closed the report while losing the feature, so the links are built instead. These are the pages that most needed them. ~950 phase variants were once reachable by nothing at all: absent from every sitemap and unlinked from the place page. "Primary schools in brentwood" is the query they exist to answer. Membership is read from the registry's own `phase_urns` rather than re-derived from the school's phase string. The registry already decides which phases a place publishes and which schools are listed on each, so asking it is both shorter and the only way the link cannot disagree with the page it points at. It also means outcodes need no special case: they carry empty `phase_urns` by design, because nobody searches "primary schools in SW11", so they report no phase links on their own. `phases` is a list rather than a single url. An all-through school is listed on both the primary and secondary pages, so there is no tie to break and no reason to invent one. Each entry renders directly after its own place, so "22 primary schools in Brentwood" reads as part of Brentwood rather than as an unrelated link further along the row. The e2e journey now follows a phase link where the town publishes one and asserts it resolves. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DXnXQKnPpZBBP61fBQiFkq --- backend/app.py | 29 +++++-- backend/tests/test_school_details.py | 86 +++++++++++++++++++ e2e/tests/journeys.spec.ts | 13 +++ .../components/NearbyPlaces.test.tsx | 43 +++++++++- nextjs-app/__tests__/lib/schoolJsonLd.test.ts | 6 +- nextjs-app/components/school/NearbyPlaces.tsx | 24 +++++- nextjs-app/lib/jsonld.ts | 12 +++ 7 files changed, 197 insertions(+), 16 deletions(-) diff --git a/backend/app.py b/backend/app.py index 782dd81..6c4e0e8 100644 --- a/backend/app.py +++ b/backend/app.py @@ -216,20 +216,37 @@ def _places_payload(urn: int) -> list[dict]: them: a name to write in the link, a count so the anchor can say what it leads to, and the canonical path. - `phase_url` is present only where the place publishes a page for this - school's phase, which is the registry's decision alone — repeating the - threshold rule here is how the page and the sitemap would come to disagree. + `phases` carries the phase variants this school actually appears on, which + is usually one and is two for an all-through school — it is listed on both + pages, so there is no tie to break. + + Membership is read straight from the registry's own `phase_urns` rather + than re-derived from the school's phase string. The registry is the one + place that decides which phases a place publishes and who is on them; + computing it a second time here is how a page comes to link a school to a + phase page that does not list it, or to a route that does not exist. That + is also why outcodes need no special case: they carry empty `phase_urns`, + so they report no phase links on their own. """ payload = [] for place in places_for_urn(get_place_registry(), int(urn)): - entry = { + phases = [ + { + "phase": phase, + "count": len(phase_urns), + "url": f"{_place_url(place)}/{phase}", + } + for phase, phase_urns in sorted(place.phase_urns.items()) + if int(urn) in phase_urns + ] + payload.append({ "kind": place.kind, "slug": place.slug, "name": place.name, "count": len(place.urns), "url": _place_url(place), - } - payload.append(entry) + "phases": phases, + }) return payload diff --git a/backend/tests/test_school_details.py b/backend/tests/test_school_details.py index a16bc0b..6b4ba55 100644 --- a/backend/tests/test_school_details.py +++ b/backend/tests/test_school_details.py @@ -126,3 +126,89 @@ def test_places_names_only_pages_that_exist(monkeypatch): assert place["name"] assert place["count"] >= 1 assert place["url"].startswith("/schools/") + + +def _brentwood_df(phase: str = "Primary", n: int = None): + from backend.places import MIN_SCHOOLS + n = n if n is not None else MIN_SCHOOLS + return lambda: pd.DataFrame([ + { + "urn": 100000 + i, + "school_name": f"Brentwood School {i}", + "town": "Brentwood", "local_authority": "Essex", + "postcode": "CM15 8AA", "phase": phase, "year": 202425, + "rwm_expected_pct": 60.0, "attainment_8_score": 50.0, + "ofsted_grade": 2.0, "ofsted_date": None, + } + for i in range(n) + ]) + + +def _places_for(monkeypatch, df_factory, urn: int): + from backend import app as app_module + monkeypatch.setattr(app_module, "load_school_data", df_factory) + monkeypatch.setattr(app_module, "get_supplementary_data", lambda db, urn: {}) + monkeypatch.setattr(app_module, "_place_registry", None) + client = TestClient(app_module.app, raise_server_exceptions=False) + return client.get(f"/api/schools/{urn}").json()["places"] + + +def test_a_place_offers_the_phase_page_this_school_appears_on(monkeypatch): + # "primary schools in brentwood" is the query the phase pages exist for, + # and ~950 of them were once reachable by nothing at all. + places = _places_for(monkeypatch, _brentwood_df("Primary"), 100000) + town = next(p for p in places if p["kind"] == "town") + + assert town["phases"], "a primary school in a published primary town has a link" + assert town["phases"][0]["url"] == "/schools/brentwood/primary" + assert town["phases"][0]["count"] >= 1 + + +def test_an_all_through_school_offers_both_phase_pages(monkeypatch): + # It genuinely appears on both, so there is no tie to break. + places = _places_for(monkeypatch, _brentwood_df("All-through"), 100000) + town = next(p for p in places if p["kind"] == "town") + + assert {p["phase"] for p in town["phases"]} == {"primary", "secondary"} + + +def test_outcodes_never_offer_a_phase_page(monkeypatch): + # The registry gives outcodes no phase route — nobody searches "primary + # schools in SW11" — and computing them anyway once put a link to a + # nonexistent route on all 1,720 outcode pages. + places = _places_for(monkeypatch, _brentwood_df("Primary"), 100000) + outcode = next((p for p in places if p["kind"] == "outcode"), None) + + if outcode is not None: + assert outcode["phases"] == [] + + +def test_a_school_absent_from_the_phase_page_is_not_linked_to_it(monkeypatch): + # The check is URN membership in the registry's own phase list, not a + # re-derivation of the phase mapping. A secondary school must not be sent + # to a primary phase page that does not list it. + from backend.places import MIN_SCHOOLS + + def df(): + rows = [ + {"urn": 100000 + i, "school_name": f"P{i}", "town": "Brentwood", + "local_authority": "Essex", "postcode": "CM15 8AA", + "phase": "Primary", "year": 202425, "rwm_expected_pct": 60.0, + "attainment_8_score": np.nan, "ofsted_grade": 2.0, + "ofsted_date": None} + for i in range(MIN_SCHOOLS) + ] + rows.append({ + "urn": 900000, "school_name": "Lone Secondary", "town": "Brentwood", + "local_authority": "Essex", "postcode": "CM15 8AA", + "phase": "Secondary", "year": 202425, "rwm_expected_pct": np.nan, + "attainment_8_score": 50.0, "ofsted_grade": 2.0, "ofsted_date": None, + }) + return pd.DataFrame(rows) + + places = _places_for(monkeypatch, df, 900000) + town = next(p for p in places if p["kind"] == "town") + + # The town publishes a primary page, but this secondary school is not on + # it, and there are too few secondaries for a secondary page. + assert town["phases"] == [] diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 2855b4f..cf77edb 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1973,6 +1973,19 @@ test('a school page links back into the location layer, and the place page links // The narrower type, not the EducationalOrganization parent it used to be. expect(graph).toContain('"School"'); + /* + * The phase variants are the pages this most needs to reach: ~950 of them + * were once reachable by nothing at all, absent from every sitemap and + * unlinked from the place page. Conditional because not every school sits + * in a town that publishes one. + */ + const phaseLink = page.locator(`a[href^="/schools/${town.slug}/"]`).first(); + if (await phaseLink.count()) { + const phaseHref = await phaseLink.getAttribute('href'); + expect((await page.request.get(phaseHref!)).status()).toBe(200); + await expect(phaseLink).toContainText(/primary|secondary/); + } + // Following it lands on a real page, not a 404. await backToTown.click(); await page.waitForURL(new RegExp(`/schools/${town.slug}$`)); diff --git a/nextjs-app/__tests__/components/NearbyPlaces.test.tsx b/nextjs-app/__tests__/components/NearbyPlaces.test.tsx index 9604bb4..cbce660 100644 --- a/nextjs-app/__tests__/components/NearbyPlaces.test.tsx +++ b/nextjs-app/__tests__/components/NearbyPlaces.test.tsx @@ -6,9 +6,9 @@ import { render, screen } from '@testing-library/react'; import { NearbyPlaces } from '@/components/school/NearbyPlaces'; -const essex = { kind: 'authority', slug: 'essex', name: 'Essex', count: 480, url: '/schools/authority/essex' }; -const brentwood = { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 37, url: '/schools/brentwood' }; -const cm15 = { kind: 'outcode', slug: 'cm15', name: 'CM15', count: 12, url: '/schools/near/cm15' }; +const essex = { kind: 'authority', slug: 'essex', name: 'Essex', count: 480, url: '/schools/authority/essex', phases: [] }; +const brentwood = { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 37, url: '/schools/brentwood', phases: [] }; +const cm15 = { kind: 'outcode', slug: 'cm15', name: 'CM15', count: 12, url: '/schools/near/cm15', phases: [] }; describe('NearbyPlaces', () => { it('links to every place the school belongs to', () => { @@ -52,4 +52,41 @@ describe('NearbyPlaces', () => { expect(screen.getByRole('link', { name: /1 school in Brentwood/ })) .toBeInTheDocument(); }); + + it('links the phase page the school appears on', () => { + // "primary schools in brentwood" is the query these pages exist for. + render(); + + expect(screen.getByRole('link', { name: /22 primary schools in Brentwood/ })) + .toHaveAttribute('href', '/schools/brentwood/primary'); + }); + + it('links both phase pages for an all-through school', () => { + render(); + + expect(screen.getByRole('link', { name: /22 primary schools/ })).toBeInTheDocument(); + expect(screen.getByRole('link', { name: /9 secondary schools/ })).toBeInTheDocument(); + }); + + it('keeps a phase link next to the place it belongs to', () => { + // Grouping matters: "22 primary schools in Brentwood" directly after + // "37 schools in Brentwood" reads as one place, not two unrelated links. + render(); + + const hrefs = screen.getAllByRole('link').map((a) => a.getAttribute('href')); + expect(hrefs.indexOf('/schools/brentwood/primary')) + .toBe(hrefs.indexOf('/schools/brentwood') + 1); + }); }); diff --git a/nextjs-app/__tests__/lib/schoolJsonLd.test.ts b/nextjs-app/__tests__/lib/schoolJsonLd.test.ts index ea68081..d5c910f 100644 --- a/nextjs-app/__tests__/lib/schoolJsonLd.test.ts +++ b/nextjs-app/__tests__/lib/schoolJsonLd.test.ts @@ -5,9 +5,9 @@ */ import { schoolBreadcrumbJsonLd } from '@/lib/jsonld'; -const essex = { kind: 'authority', slug: 'essex', name: 'Essex', count: 480, url: '/schools/authority/essex' }; -const brentwood = { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 37, url: '/schools/brentwood' }; -const outcode = { kind: 'outcode', slug: 'cm15', name: 'CM15', count: 12, url: '/schools/near/cm15' }; +const essex = { kind: 'authority', slug: 'essex', name: 'Essex', count: 480, url: '/schools/authority/essex', phases: [] }; +const brentwood = { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 37, url: '/schools/brentwood', phases: [] }; +const outcode = { kind: 'outcode', slug: 'cm15', name: 'CM15', count: 12, url: '/schools/near/cm15', phases: [] }; describe('school breadcrumbs', () => { it('reads home to authority to town to school', () => { diff --git a/nextjs-app/components/school/NearbyPlaces.tsx b/nextjs-app/components/school/NearbyPlaces.tsx index 2af24a8..a4160f6 100644 --- a/nextjs-app/components/school/NearbyPlaces.tsx +++ b/nextjs-app/components/school/NearbyPlaces.tsx @@ -1,5 +1,5 @@ import Link from 'next/link'; -import type { SchoolPlace } from '@/lib/jsonld'; +import type { SchoolPlace, SchoolPhasePage } from '@/lib/jsonld'; import styles from './NearbyPlaces.module.css'; /** @@ -29,6 +29,12 @@ function label(place: SchoolPlace): string { return `${place.count} ${noun} ${preposition} ${place.name}`; } +/** "22 primary schools in Brentwood" — the phrasing the query itself uses. */ +function phaseLabel(place: SchoolPlace, page: SchoolPhasePage): string { + const noun = page.count === 1 ? 'school' : 'schools'; + return `${page.count} ${page.phase} ${noun} in ${place.name}`; +} + export function NearbyPlaces({ places }: { places: SchoolPlace[] }) { if (places.length === 0) return null; @@ -40,11 +46,21 @@ export function NearbyPlaces({ places }: { places: SchoolPlace[] }) {

More schools near here

    - {sorted.map((place) => ( + {sorted.flatMap((place) => [
  • {label(place)} -
    • - ))} + , + /* Immediately after its own place, so "22 primary schools in + Brentwood" reads as part of Brentwood rather than as an + unrelated link further down the row. */ + ...place.phases.map((page) => ( +
  • + + {phaseLabel(place, page)} + +
    • + )), + ])}
); diff --git a/nextjs-app/lib/jsonld.ts b/nextjs-app/lib/jsonld.ts index aec423a..d801918 100644 --- a/nextjs-app/lib/jsonld.ts +++ b/nextjs-app/lib/jsonld.ts @@ -74,12 +74,24 @@ export function blogPostingJsonLd( * it. `count` is what lets a link say "All 37 schools in Brentwood" rather * than "click here". */ +export interface SchoolPhasePage { + phase: string; + count: number; + url: string; +} + export interface SchoolPlace { kind: string; slug: string; name: string; count: number; url: string; + /** + * The phase variants this school is actually listed on: usually one, two + * for an all-through school, none for an outcode, which publishes no phase + * route. Decided by the place registry, never re-derived here. + */ + phases: SchoolPhasePage[]; } /** From b0d5334e0664dc8afbe541af8fa06f0d40d3b59f Mon Sep 17 00:00:00 2001 From: Tudor Date: Mon, 14 Sep 2026 21:22:39 +0100 Subject: [PATCH 3/3] perf(places): index the reverse lookup, and isolate the registry in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both confirmed before fixing. The client fixture in test_school_details.py patched load_school_data but not _place_registry, which is a module-level cache. A probe settled it rather than an argument: poisoning the global with a registry built from data the fixture never saw, then issuing the fixture's own request, returned that other dataset's places. So the new `places == []` assertion was satisfied by a stale registry exactly as well as by the fixture's own data, and proved nothing. Every other test that touches place data already reset it; the fixture predates places existing and was never updated. It resets it now. places_for_urn walked every place in the registry and did a tuple membership test against each, on /api/schools/{urn}, the site's highest-traffic endpoint. It now reads a dict built once per registry. Measured against a synthetic corpus of 27k schools in 1,650 places: 0.118ms per request becomes 0.0001ms, with the index built once in 21ms. Production carries ~5,000 places, so the scan there is larger again. The absolute saving per request is small; the point is that it is repeated on every school page view and costs nothing to remove. The index is cached against the registry by identity rather than behind a second flag. Anything that drops _place_registry — every test that touches place data does — gets a fresh registry object, which no longer matches what the index was built from, so the index rebuilds with it. A separate _place_index = None would be one more thing to forget, and a stale reverse index is precisely the first finding's bug wearing a different hat. That invalidation has its own test, and the test was checked by breaking the identity check: five tests fail without it, so three existing ones were already relying on it too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DXnXQKnPpZBBP61fBQiFkq --- backend/app.py | 26 +++++++++++++-- backend/places.py | 47 +++++++++++++++++++++------- backend/tests/test_places.py | 36 ++++++++++++++++++--- backend/tests/test_school_details.py | 33 +++++++++++++++++++ 4 files changed, 124 insertions(+), 18 deletions(-) diff --git a/backend/app.py b/backend/app.py index 6c4e0e8..caa3c21 100644 --- a/backend/app.py +++ b/backend/app.py @@ -38,7 +38,7 @@ from .data_loader import ( ) from .data_loader import get_data_info as get_db_info from . import flags -from .places import build_place_registry, places_for_urn +from .places import build_place_index, build_place_registry, places_for_urn from .schemas import METRIC_DEFINITIONS, RANKING_COLUMNS, SCHOOL_COLUMNS from .utils import clean_for_json, convert_to_native @@ -65,6 +65,10 @@ _sitemaps: dict[str, str] | None = None # Built from the same DataFrame the sitemap uses, so places and sitemap can # never describe different corpora. Reset by the same admin endpoint. _place_registry: dict | None = None +# Cached beside the registry, and invalidated by identity against it — see +# get_place_index. Never cleared independently. +_place_index: dict | None = None +_place_index_source: dict | None = None VALID_PLACE_KINDS = ("town", "locality", "authority", "outcode") @@ -188,6 +192,24 @@ def get_place_registry() -> dict: return _place_registry +def get_place_index() -> dict: + """URN → its published places, cached against the registry it came from. + + Invalidation is an identity check rather than a second flag to remember to + clear. Anything that drops `_place_registry` — the tests all do — gets a + fresh registry object here, which no longer matches the one the index was + built from, so the index rebuilds with it. A separate `_place_index = None` + would be one more thing to forget, and a stale reverse index is exactly the + bug that would put links to another dataset's places on a school page. + """ + global _place_index, _place_index_source + registry = get_place_registry() + if _place_index is None or _place_index_source is not registry: + _place_index = build_place_index(registry) + _place_index_source = registry + return _place_index + + def _urlset(rows: list[str]) -> str: return "\n".join([ '', @@ -229,7 +251,7 @@ def _places_payload(urn: int) -> list[dict]: so they report no phase links on their own. """ payload = [] - for place in places_for_urn(get_place_registry(), int(urn)): + for place in places_for_urn(get_place_index(), int(urn)): phases = [ { "phase": phase, diff --git a/backend/places.py b/backend/places.py index 6476c38..60055e8 100644 --- a/backend/places.py +++ b/backend/places.py @@ -296,21 +296,46 @@ def _locality_places(df, publishable: set[int], return out -def places_for_urn(registry: dict[str, Place], urn: int) -> tuple[Place, ...]: - """Every published place containing this school, largest kind first. +# Ordered authority → town/locality → outcode, widest first, because that is +# the order a breadcrumb reads. The link module re-sorts for its own purposes. +_PLACE_ORDER = {"authority": 0, "town": 1, "locality": 2, "outcode": 3} + + +def build_place_index(registry: dict[str, Place]) -> dict[int, tuple[Place, ...]]: + """URN → the published places containing it, built once per registry. The reverse of the registry, and the thing school pages link out through. - Derived from the registry rather than stored beside it, so the two cannot - disagree about which places exist: a link module must never offer a place - whose page does not exist, and a place below the publish threshold is - simply absent from the registry, so it is absent from here too. + Derived from the registry rather than maintained beside it, so the two + cannot disagree about which places exist: a place below the publish + threshold is absent from the registry, so it is absent from here too, and + a link is never offered for a page that does not exist. - Ordered authority → town/locality → outcode, widest first, because that is - the order a breadcrumb reads and the order the link module lists. + Built as an index rather than scanned per call because /api/schools/{urn} + is the site's highest-traffic endpoint. Scanning meant walking every place + and doing a tuple membership test against each — on the order of 10^5 + comparisons per request, repeated for every school page view. One pass at + registry-build time replaces all of it with a dict lookup. """ - order = {"authority": 0, "town": 1, "locality": 2, "outcode": 3} - found = [p for p in registry.values() if urn in p.urns] - return tuple(sorted(found, key=lambda p: (order.get(p.kind, 9), p.slug))) + grouped: dict[int, list[Place]] = {} + for place in registry.values(): + for urn in place.urns: + grouped.setdefault(int(urn), []).append(place) + + return { + urn: tuple(sorted(places, + key=lambda p: (_PLACE_ORDER.get(p.kind, 9), p.slug))) + for urn, places in grouped.items() + } + + +def places_for_urn(index: dict[int, tuple[Place, ...]], urn: int) -> tuple[Place, ...]: + """The published places containing this school, widest first. + + Empty is a real answer, not a failure: a school whose town and authority + both fall below the publish threshold has nowhere to link, and the page + renders without the module. + """ + return index.get(int(urn), ()) def build_place_registry(df) -> dict[str, Place]: diff --git a/backend/tests/test_places.py b/backend/tests/test_places.py index c041eda..2bb4ae3 100644 --- a/backend/tests/test_places.py +++ b/backend/tests/test_places.py @@ -8,7 +8,8 @@ import numpy as np import pandas as pd import pytest -from backend.places import MIN_SCHOOLS, build_place_registry, places_for_urn +from backend.places import (MIN_SCHOOLS, build_place_index, + build_place_registry, places_for_urn) def _df(rows: list[dict]) -> pd.DataFrame: @@ -428,7 +429,7 @@ def test_an_authority_still_publishes_phase_variants(): def test_a_school_resolves_to_every_published_place_containing_it(): reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Brentwood", "Essex"))) - places = places_for_urn(reg, 100000) + places = places_for_urn(build_place_index(reg), 100000) kinds = {p.kind for p in places} assert "town" in kinds @@ -443,7 +444,7 @@ def test_a_school_in_an_unpublished_town_still_resolves_to_its_authority(): _town(MIN_SCHOOLS - 1, "Tinytown", "Essex") + _town(MIN_SCHOOLS, "Brentwood", "Essex", start=200000) )) - places = places_for_urn(reg, 100000) + places = places_for_urn(build_place_index(reg), 100000) # The town is below the threshold, so it has no page and must not be # offered as a link. The authority above it does, and is the right target. @@ -455,7 +456,7 @@ def test_an_unknown_urn_resolves_to_nothing_rather_than_raising(): # A school page renders for any URN the API knows; the link module is not # entitled to take the page down when it has nothing to say. reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Brentwood", "Essex"))) - assert places_for_urn(reg, 999999) == () + assert places_for_urn(build_place_index(reg), 999999) == () def test_the_index_is_consistent_with_the_registry_it_was_built_from(): @@ -465,7 +466,32 @@ def test_the_index_is_consistent_with_the_registry_it_was_built_from(): _town(MIN_SCHOOLS, "Brentwood", "Essex") + _town(MIN_SCHOOLS, "Bedford", "Bedford", start=300000) )) + index = build_place_index(reg) for key, place in reg.items(): for urn in place.urns: - assert place in places_for_urn(reg, urn), ( + assert place in places_for_urn(index, urn), ( f"{urn} is in {key} but the index does not say so") + + +def test_the_index_holds_no_school_the_registry_does_not(): + # The reverse direction of the invariant above. An index entry for a URN + # no published place contains would put a link on a page for a place that + # does not list that school. + reg = build_place_registry(_df( + _town(MIN_SCHOOLS, "Brentwood", "Essex") + + _town(MIN_SCHOOLS - 1, "Tinytown", "Essex", start=400000) + )) + index = build_place_index(reg) + + for urn, places in index.items(): + for place in places: + assert urn in place.urns + assert place.key in reg + + +def test_the_index_preserves_the_widest_first_order(): + # The breadcrumb reads authority then town, and takes this order as given. + reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Brentwood", "Essex"))) + kinds = [p.kind for p in places_for_urn(build_place_index(reg), 100000)] + + assert kinds.index("authority") < kinds.index("town") diff --git a/backend/tests/test_school_details.py b/backend/tests/test_school_details.py index 6b4ba55..c7fc398 100644 --- a/backend/tests/test_school_details.py +++ b/backend/tests/test_school_details.py @@ -56,6 +56,11 @@ def client(monkeypatch): monkeypatch.setattr( app_module, "get_supplementary_data", lambda db, urn: {} ) + # The place registry is a module-level cache, so without this the endpoint + # answers from whatever registry an earlier test happened to leave behind + # — and a `places == []` assertion is satisfied by a stale registry just + # as well as by this fixture's own data, which makes it prove nothing. + monkeypatch.setattr(app_module, "_place_registry", None) return TestClient(app_module.app, raise_server_exceptions=False) @@ -212,3 +217,31 @@ def test_a_school_absent_from_the_phase_page_is_not_linked_to_it(monkeypatch): # The town publishes a primary page, but this secondary school is not on # it, and there are too few secondaries for a secondary page. assert town["phases"] == [] + + +def test_the_place_index_rebuilds_when_the_registry_is_replaced(monkeypatch): + """The reverse index is cached; a stale one would put another dataset's + places on a school page. Invalidation is an identity check against the + registry rather than a second flag, so this asserts the check works.""" + from backend import app as app_module + + monkeypatch.setattr(app_module, "_place_registry", None) + monkeypatch.setattr(app_module, "_place_index", None) + monkeypatch.setattr(app_module, "_place_index_source", None) + monkeypatch.setattr(app_module, "load_school_data", _brentwood_df("Primary")) + + first = app_module.get_place_index() + assert 100000 in first + + # Same registry object, so the index is reused rather than rebuilt. + assert app_module.get_place_index() is first + + # Drop the registry the way every test that touches place data does. The + # index must follow it, not survive it. + app_module._place_registry = None + monkeypatch.setattr(app_module, "load_school_data", + _brentwood_df("Primary", n=0)) + + rebuilt = app_module.get_place_index() + assert rebuilt is not first + assert 100000 not in rebuilt, "the index outlived the registry it came from"