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/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") 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,