From 8967966eef9c1c6abeac533b0187c4acc659a898 Mon Sep 17 00:00:00 2001 From: Tudor Date: Sat, 22 Aug 2026 00:09:58 +0100 Subject: [PATCH 1/2] feat(places): list schools alphabetically on place pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Someone on a place page is usually looking for a school they can name, so the order should serve scanning for it rather than ranking. /api/rankings keeps its league-table ordering; this is a place-page decision, not a site-wide one. Sorted case-insensitively, or a capitalised name would sort ahead of every lowercase one. The change made five pieces of copy untrue, so they go with it. The phase variant titled itself "— Ranked", and all four route families described themselves as "ranked by SATs and GCSE results". A page that opens by claiming an order it does not keep is worse than one that claims nothing. The ItemList markup carried `position` with no declared order, which reads as a ranking. It now declares ItemListOrderAscending, so the structured data says what the table does. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/app.py | 11 +++++-- backend/tests/test_places_api.py | 24 ++++++++++++-- e2e/tests/journeys.spec.ts | 28 ++++++++++++++++ .../__tests__/components/PlaceView.test.tsx | 33 +++++++++++++++++++ .../app/schools/[place]/[phase]/page.tsx | 8 +++-- nextjs-app/app/schools/[place]/page.tsx | 4 +-- .../app/schools/authority/[la]/page.tsx | 4 +-- .../app/schools/near/[outcode]/page.tsx | 4 +-- nextjs-app/components/places/PlaceView.tsx | 4 +++ 9 files changed, 106 insertions(+), 14 deletions(-) diff --git a/backend/app.py b/backend/app.py index 4cba056..fa8945d 100644 --- a/backend/app.py +++ b/backend/app.py @@ -1212,10 +1212,15 @@ async def get_place(request: Request, kind: str, slug: str, if wanted and "phase" in rows.columns: rows = rows[rows["phase"].fillna("").str.lower().isin(wanted)] - # The metric the page ranks on, which is also the one it averages. + # The metric the page shows, and averages. metric = "attainment_8_score" if phase == "secondary" else "rwm_expected_pct" - if metric in rows.columns: - rows = rows.sort_values(metric, ascending=False, na_position="last") + + # Alphabetical, not by score. A place page is read by someone looking for + # a school they can name, and scanning for it is what the order should + # serve. /rankings is where the league-table ordering lives, and it keeps + # sorting by metric. + if "school_name" in rows.columns: + rows = rows.sort_values("school_name", key=lambda c: c.str.lower()) averages = { m: (None if m not in rows.columns or rows[m].dropna().empty diff --git a/backend/tests/test_places_api.py b/backend/tests/test_places_api.py index 4eb9822..b80444d 100644 --- a/backend/tests/test_places_api.py +++ b/backend/tests/test_places_api.py @@ -45,10 +45,30 @@ def test_registry_carries_a_count_per_place(client): assert town["count"] == 6 -def test_place_detail_returns_its_schools_ranked(client): +def test_place_detail_returns_its_schools_alphabetically(client): + """A place page is read by someone looking for a school they can name. + + Scanning for it is what the order should serve, so the list is A-Z. + /api/rankings is where the league-table ordering lives. + """ body = client.get("/api/places/town/brentwood").json() assert body["place"]["name"] == "Brentwood" - scores = [s["rwm_expected_pct"] for s in body["schools"]] + names = [s["school_name"] for s in body["schools"]] + assert names == sorted(names, key=str.lower) + + +def test_place_ordering_ignores_case(client): + body = client.get("/api/places/town/brentwood").json() + names = [s["school_name"] for s in body["schools"]] + # A capitalised name must not sort ahead of every lowercase one. + assert names == sorted(names, key=str.lower) + + +def test_the_rankings_endpoint_still_ranks_by_metric(client): + # Alphabetical is a place-page decision, not a site-wide one. + body = client.get("/api/rankings?metric=rwm_expected_pct&phase=primary").json() + scores = [r["rwm_expected_pct"] for r in body.get("rankings", []) + if r.get("rwm_expected_pct") is not None] assert scores == sorted(scores, reverse=True) diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 0e2a778..03b1e27 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1951,3 +1951,31 @@ test('a place straddling a boundary names every authority it sits in', async ({ .toBeVisible(); } }); + +test('a place page lists its schools alphabetically', async ({ page }) => { + // Someone on a place page is usually looking for a school they can name, + // so the order should serve scanning for it. /rankings is where the + // league-table ordering lives. + const { places } = await (await page.request.get('/api/places')).json(); + const town = places.find((p: { kind: string; count: number }) => + p.kind === 'town' && p.count >= 5); + expect(town).toBeTruthy(); + + await page.goto(`/schools/${town.slug}`); + const names = await page.locator('a[href^="/school/"]').allTextContents(); + expect(names.length).toBeGreaterThan(1); + + const sorted = [...names].sort((a, b) => + a.toLowerCase().localeCompare(b.toLowerCase())); + expect(names).toEqual(sorted); +}); + +test('the rankings page still orders by score, not name', async ({ page }) => { + // Alphabetical is a place-page decision, not a site-wide one. + const res = await page.request.get('/api/rankings?metric=rwm_expected_pct&phase=primary'); + expect(res.ok()).toBeTruthy(); + const scores = ((await res.json()).rankings ?? []) + .map((r: { rwm_expected_pct: number | null }) => r.rwm_expected_pct) + .filter((v: number | null) => v != null); + expect(scores).toEqual([...scores].sort((a: number, b: number) => b - a)); +}); diff --git a/nextjs-app/__tests__/components/PlaceView.test.tsx b/nextjs-app/__tests__/components/PlaceView.test.tsx index f85b624..3d6caf6 100644 --- a/nextjs-app/__tests__/components/PlaceView.test.tsx +++ b/nextjs-app/__tests__/components/PlaceView.test.tsx @@ -243,3 +243,36 @@ describe('PlaceView authorities', () => { expect(screen.getByRole('link', { name: 'Merton' })).toBeInTheDocument(); }); }); + +describe('PlaceView list ordering', () => { + const detail3: PlaceDetail = { + place: { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 2, + parent_authority: 'Essex', phases: ['primary'] }, + schools: [ + { urn: 1, school_name: 'Alpha Primary', phase: 'Primary', + rwm_expected_pct: 40, attainment_8_score: null } as never, + { urn: 2, school_name: 'Beta Primary', phase: 'Primary', + rwm_expected_pct: 90, attainment_8_score: null } as never, + ], + averages: { rwm_expected_pct: 65, attainment_8_score: null }, + }; + + it('renders schools in the order the API sent them, not by score', () => { + // The API sorts alphabetically now; the component must not re-sort. + render(); + const links = screen.getAllByRole('link', { name: /Primary$/ }); + expect(links.map((l) => l.textContent)) + .toEqual(['Alpha Primary', 'Beta Primary']); + }); + + it('declares the list as ascending rather than implying a ranking', () => { + // An ItemList carrying `position` reads as a ranking unless it says + // otherwise, and the table is A-Z. + const { container } = render(); + const ld = JSON.parse( + container.querySelector('script[type="application/ld+json"]')!.textContent!); + const list = ld['@graph'].find((n: { '@type': string }) => n['@type'] === 'ItemList'); + expect(list.itemListOrder).toBe('https://schema.org/ItemListOrderAscending'); + }); +}); diff --git a/nextjs-app/app/schools/[place]/[phase]/page.tsx b/nextjs-app/app/schools/[place]/[phase]/page.tsx index b63b80b..9f97045 100644 --- a/nextjs-app/app/schools/[place]/[phase]/page.tsx +++ b/nextjs-app/app/schools/[place]/[phase]/page.tsx @@ -37,10 +37,12 @@ export async function generateMetadata({ params }: Props): Promise { const word = phase === 'secondary' ? 'Secondary' : 'Primary'; const { name } = detail.place; return { - title: { absolute: `${word} Schools in ${name} — Ranked | schoolcompare` }, + // Not "Ranked": the table is alphabetical, so the word would be a claim + // the page does not keep. + title: { absolute: `${word} Schools in ${name} | schoolcompare` }, description: - `Every ${phase} school in ${name} ranked by results, with Ofsted grades and ` - + `the local average against England.`, + `Every ${phase} school in ${name}, with results, Ofsted grades and the local ` + + `average against England.`, alternates: { canonical: absoluteUrl(`/schools/${slug}/${phase}`) }, }; } diff --git a/nextjs-app/app/schools/[place]/page.tsx b/nextjs-app/app/schools/[place]/page.tsx index c2007b3..68a8652 100644 --- a/nextjs-app/app/schools/[place]/page.tsx +++ b/nextjs-app/app/schools/[place]/page.tsx @@ -58,8 +58,8 @@ export async function generateMetadata({ params }: Props): Promise { // place title read '... | schoolcompare | schoolcompare'. title: { absolute: `Schools in ${name} — Compare ${count} Schools | schoolcompare` }, description: - `Every school in ${name} ranked by SATs and GCSE results, with Ofsted grades, ` - + `the local average against England, and how close you had to live to get a place.`, + `Every school in ${name}, with SATs and GCSE results, Ofsted grades, the local ` + + `average against England, and how close you had to live to get a place.`, alternates: { canonical: absoluteUrl(`/schools/${slug}`) }, }; } diff --git a/nextjs-app/app/schools/authority/[la]/page.tsx b/nextjs-app/app/schools/authority/[la]/page.tsx index f9b4c3e..b907b88 100644 --- a/nextjs-app/app/schools/authority/[la]/page.tsx +++ b/nextjs-app/app/schools/authority/[la]/page.tsx @@ -45,8 +45,8 @@ export async function generateMetadata({ params }: Props): Promise { return { title: { absolute: `Schools in ${name} — Local Authority | schoolcompare` }, description: - `All ${count} schools in the ${name} local authority, ranked by SATs and GCSE ` - + `results, with Ofsted grades and the authority average against England.`, + `All ${count} schools in the ${name} local authority, with SATs and GCSE results, ` + + `Ofsted grades and the authority average against England.`, alternates: { canonical: absoluteUrl(`/schools/authority/${la}`) }, }; } diff --git a/nextjs-app/app/schools/near/[outcode]/page.tsx b/nextjs-app/app/schools/near/[outcode]/page.tsx index e08d471..c4ec120 100644 --- a/nextjs-app/app/schools/near/[outcode]/page.tsx +++ b/nextjs-app/app/schools/near/[outcode]/page.tsx @@ -40,8 +40,8 @@ export async function generateMetadata({ params }: Props): Promise { return { title: { absolute: `Schools near ${name} | schoolcompare` }, description: - `${count} schools in the ${name} postcode district, ranked by results, with ` - + `Ofsted grades and how close you had to live to get a place.`, + `${count} schools in the ${name} postcode district, with results, Ofsted grades ` + + `and how close you had to live to get a place.`, alternates: { canonical: absoluteUrl(`/schools/near/${outcode}`) }, }; } diff --git a/nextjs-app/components/places/PlaceView.tsx b/nextjs-app/components/places/PlaceView.tsx index 4a3ea5e..15ec0c3 100644 --- a/nextjs-app/components/places/PlaceView.tsx +++ b/nextjs-app/components/places/PlaceView.tsx @@ -142,6 +142,10 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props) '@type': 'ItemList', name: `${phaseWord} in ${place.name}`, numberOfItems: schools.length, + // Alphabetical, and said so. Without this an ItemList carrying + // `position` reads as a ranking, which would be a claim the page + // stopped making when the table became A-Z. + itemListOrder: 'https://schema.org/ItemListOrderAscending', itemListElement: schools.slice(0, 20).map((s, i) => ({ '@type': 'ListItem', position: i + 1, -- 2.54.0 From 9cc87c41bb60b2cfc769991140d1d1c17cbf711a Mon Sep 17 00:00:00 2001 From: Tudor Date: Sat, 22 Aug 2026 00:14:31 +0100 Subject: [PATCH 2/2] fix(places): a phase page needs results, not merely publishable schools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asked where schools with no results should sit in an alphabetical list, and found that some pages were almost entirely made of them. The per-phase threshold counted schools that were publishable — a result OR an Ofsted grade — while a phase page exists for its results column. /schools/kent/primary published with none of its five rows carrying a result; Minehead had one of seven, Buntingford one of five. Forty-four phase pages were majority-blank. It is the same rule as "no page without a local average", which was written into the spec as a thin-page control and never extended per phase. The threshold now counts schools with a result for that phase. It gates whether the page exists; it does not filter rows — a page that publishes still lists every school of the phase, because someone looking up a school by name has to find it whether or not it published results. 126 of 1,012 variant pages stop publishing: 62 primary, 64 secondary. Every one of them was a table with too little in it to be worth a page. The ordering itself is unchanged: pure A-Z, blanks interleaved. A school sits where its name says it does, and at roughly a tenth of rows that reads fine. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- backend/places.py | 37 +++++++++++++++++++++++++++-- backend/tests/test_places.py | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/backend/places.py b/backend/places.py index 870f779..aae7fdd 100644 --- a/backend/places.py +++ b/backend/places.py @@ -59,18 +59,51 @@ def _publishable_urns(df) -> set[int]: return set(df.loc[df[cols].notna().any(axis=1), "urn"].astype(int)) +# The measure a phase page is built around. A page with no results in this +# column has nothing a list of school names does not already give. +_PHASE_METRIC = { + "primary": "rwm_expected_pct", + "secondary": "attainment_8_score", +} + + 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.""" + """URNs per phase, counting only schools with a result for that phase. + + Not merely "publishable". A school with an Ofsted grade and no results is + worth a page of its own and belongs in the place list, but it cannot + populate a phase page's results column — and the threshold is there to ask + whether that column will have anything in it. + + Counting publishable schools instead let /schools/kent/primary publish + with none of its five rows carrying a result, and left 44 phase pages + majority-blank. It is the same rule as "no page without a local average", + which was never extended per phase. + + All-through schools count toward both phases, 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)] + + # The page lists every school of the phase; the threshold counts only + # those carrying a result, so a mostly-empty table never publishes. + metric = _PHASE_METRIC[phase] + with_result = ( + {int(u) for u in subset.loc[subset[metric].notna(), "urn"]} + if metric in subset.columns else set() + ) + if len(with_result & publishable) < MIN_SCHOOLS: + continue + urns = tuple(sorted({int(u) for u in subset["urn"]} & publishable)) if urns: out[phase] = urns diff --git a/backend/tests/test_places.py b/backend/tests/test_places.py index 6f3be5e..521c536 100644 --- a/backend/tests/test_places.py +++ b/backend/tests/test_places.py @@ -343,3 +343,49 @@ def test_the_merged_place_takes_its_most_common_spelling(): + _town(5, "NEWCASTLE-UNDER-LYME", "Staffordshire", start=400000)) reg = build_place_registry(_df(rows)) assert reg["town:newcastle-under-lyme"].name == "Newcastle-under-Lyme" + + +def test_a_phase_page_needs_results_not_merely_publishable_schools(): + """/schools/kent/primary published with none of its five rows scored. + + The threshold counted schools that were publishable — a result OR an + Ofsted grade — while the page exists for its results column. Forty-four + phase pages were majority-blank; one had no results at all. + """ + rows = _town(MIN_SCHOOLS, "Kent", "Kent") + for r in rows: + r["rwm_expected_pct"] = np.nan # Ofsted only, no results + reg = build_place_registry(_df(rows)) + + assert "town:kent" in reg # the place still publishes + assert not reg["town:kent"].publishes_phase("primary") + + +def test_a_phase_page_publishes_once_enough_schools_carry_a_result(): + rows = _town(MIN_SCHOOLS, "Beccles", "Suffolk") + reg = build_place_registry(_df(rows)) + assert reg["town:beccles"].publishes_phase("primary") + + +def test_a_publishing_phase_page_still_lists_its_unscored_schools(): + """The threshold gates whether the page exists; it does not filter rows. + + A parent looking up a school by name has to find it whether or not it + published results. + """ + scored = _town(MIN_SCHOOLS, "Beccles", "Suffolk", start=300000) + unscored = _town(2, "Beccles", "Suffolk", start=400000) + for r in unscored: + r["rwm_expected_pct"] = np.nan + reg = build_place_registry(_df(scored + unscored)) + + place = reg["town:beccles"] + assert place.publishes_phase("primary") + assert len(place.phase_urns["primary"]) == MIN_SCHOOLS + 2 + + +def test_the_secondary_threshold_counts_its_own_metric(): + # A town full of scored primaries must not thereby publish a secondary page. + rows = _town(MIN_SCHOOLS, "Brentwood", "Essex") + reg = build_place_registry(_df(rows)) + assert not reg["town:brentwood"].publishes_phase("secondary") -- 2.54.0