From d65eb588834ee9b3625330aca339b59ecaf5f7ba Mon Sep 17 00:00:00 2001 From: Tudor Date: Mon, 14 Sep 2026 20:54:59 +0100 Subject: [PATCH] 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[]; } /**