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[]; } /**