From 8967966eef9c1c6abeac533b0187c4acc659a898 Mon Sep 17 00:00:00 2001 From: Tudor Date: Sat, 22 Aug 2026 00:09:58 +0100 Subject: [PATCH] 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,