diff --git a/backend/app.py b/backend/app.py index a054a6b..15bf139 100644 --- a/backend/app.py +++ b/backend/app.py @@ -621,6 +621,19 @@ def verify_admin_api_key(x_api_key: str = Header(None)) -> bool: return True +def _with_whole_school_pupils(rows: pd.DataFrame, source: pd.DataFrame) -> pd.DataFrame: + """Set total_pupils to the size of the school. + + fact_performance's total_pupils is the cohort a year's results were + measured on. For a secondary that is the GCSE year group alone (Burntwood: + 245 against 1,462 on roll). Cards, map popups and place rows label it + "pupils", so they take the register's whole-school count instead, and + nothing when the register has none. + """ + whole = source["gias_total_pupils"] if "gias_total_pupils" in source.columns else None + return rows.assign(total_pupils=whole) + + # Input validation helpers def _names_in_group(names: pd.Series, in_group) -> set: """The distinct names in a column that a group predicate accepts. @@ -859,7 +872,7 @@ async def get_schools( if c in df_latest.columns ] # fact_performance guarantees one row per (urn, year); df_latest has one row per urn. - schools_df = df_latest[available_cols] + schools_df = _with_whole_school_pupils(df_latest[available_cols], df_latest) # Location-based search (uses pre-geocoded data from database) search_coords = None @@ -1545,7 +1558,7 @@ async def get_place(request: Request, kind: str, slug: str, # variants that exist rather than 404s. "phases": [ph for ph in ("primary", "secondary") if place.publishes_phase(ph)]}, - "schools": clean_for_json(rows[cols]), + "schools": clean_for_json(_with_whole_school_pupils(rows[cols], rows)), "averages": averages, } diff --git a/backend/tests/test_whole_school_pupils.py b/backend/tests/test_whole_school_pupils.py new file mode 100644 index 0000000..8e9e8a7 --- /dev/null +++ b/backend/tests/test_whole_school_pupils.py @@ -0,0 +1,67 @@ +"""Cards, map popups and place rows label total_pupils "pupils". + +fact_performance's total_pupils is the cohort a year's results were measured +on. For a secondary that is the GCSE year group alone: Burntwood showed 245 in +search against 1,462 on roll. The list and place payloads therefore carry the +register's whole-school count, and nothing when the register has none. +""" + +import numpy as np +import pandas as pd +import pytest +from fastapi.testclient import TestClient + + +def _schools_df() -> pd.DataFrame: + base = { + "local_authority": "Essex", "school_type": "Academy converter", + "year": 202425, "ofsted_grade": 2.0, "ofsted_date": None, + "town": "Brentwood", "postcode": "CM13 1AA", "status": "Open", + "address": "1 Test Street", "latitude": 51.6, "longitude": 0.3, + "gender": "Mixed", "rwm_expected_pct": np.nan, "attainment_8_score": 50.0, + } + rows = [ + # Secondary: results cohort 245, register 1,462. + {**base, "urn": 100001, "school_name": "Alpha High", "phase": "Secondary", + "total_pupils": 245, "gias_total_pupils": 1462}, + # Register count missing: no count, never the cohort. + {**base, "urn": 100002, "school_name": "Beta High", "phase": "Secondary", + "total_pupils": 180, "gias_total_pupils": np.nan}, + ] + # Enough schools in one town for it to have a place page. + rows += [ + {**base, "urn": 100010 + i, "school_name": f"Gamma High {i}", "phase": "Secondary", + "total_pupils": 200, "gias_total_pupils": 1000 + i} + for i in range(5) + ] + return pd.DataFrame(rows) + + +@pytest.fixture() +def client(monkeypatch): + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_latest_school_data", _schools_df) + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + monkeypatch.setattr(app_module, "_place_registry", None) + return TestClient(app_module.app, raise_server_exceptions=False) + + +def _pupils(schools: list[dict]) -> dict[int, object]: + return {s["urn"]: s.get("total_pupils") for s in schools} + + +def test_the_list_carries_the_whole_school_count(client): + resp = client.get("/api/schools?page_size=50") + assert resp.status_code == 200, resp.text + pupils = _pupils(resp.json()["schools"]) + assert pupils[100001] == 1462 + assert pupils[100002] is None + + +def test_a_place_page_carries_the_whole_school_count(client): + resp = client.get("/api/places/town/brentwood") + assert resp.status_code == 200, resp.text + pupils = _pupils(resp.json()["schools"]) + assert pupils[100001] == 1462 + assert pupils[100002] is None diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 13ebecf..4e09915 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -53,7 +53,7 @@ async function settledScrollLeft(scroller: Locator): Promise { * whatever primaries the environment holds. */ async function twoPrimaryUrns(page: Page): Promise<[string, string]> { - const res = await page.request.get('/api/schools?search=primary&per_page=50'); + const res = await page.request.get('/api/schools?search=primary&page_size=50'); expect(res.ok()).toBeTruthy(); const body = await res.json(); const urns: string[] = (body.schools ?? []) @@ -66,7 +66,7 @@ async function twoPrimaryUrns(page: Page): Promise<[string, string]> { } async function twoSecondaryUrns(page: Page): Promise<[string, string]> { - const res = await page.request.get('/api/schools?search=school&per_page=100'); + const res = await page.request.get('/api/schools?search=school&page_size=100'); expect(res.ok()).toBeTruthy(); const body = await res.json(); const urns: string[] = (body.schools ?? []) @@ -355,6 +355,61 @@ test('school type groups and the faith filter narrow to what they name', async ( } }); +/* + * Search rows printed tags the register does not hold. Every non-selective + * secondary was "Selective" ("non-selective" contains "selective"), and a + * school with no religious character got "Faith priority" or a bare "None" + * chip, because only "Does not apply" was excluded. Data-invariant: each test + * picks its school from the API and reads only that school's row. + */ +async function rowTags(page: Page, school: { urn: number; school_name: string }) { + await searchByName(page, school.school_name); + const link = page.locator(`a[href^="/school/${school.urn}-"]`).first(); + await expect(link).toBeVisible({ timeout: 15_000 }); + return link.locator('xpath=ancestor::div[contains(@class, "__rowContent")][1]') + .locator('[class*="__line2"]'); +} + +test('a non-selective secondary is not tagged Selective in search', async ({ page }) => { + const res = await page.request.get( + '/api/schools?search=school&phase=secondary&admissions_policy=non-selective&page_size=1'); + expect(res.ok()).toBeTruthy(); + const [school] = (await res.json()).schools ?? []; + test.skip(!school, 'no non-selective secondary in this environment'); + expect(school.admissions_policy, 'the admissions filter was ignored').toBe('Non-selective'); + + const tags = await rowTags(page, school); + await expect(tags).toBeVisible(); + await expect(tags.getByText('Selective', { exact: true })).toHaveCount(0); +}); + +test('a school with no religious character carries no faith tag in search', async ({ page }) => { + const res = await page.request.get('/api/schools?search=school&faith=none&page_size=100'); + expect(res.ok()).toBeTruthy(); + // Not a selective school: the Selective tag would win and hide the bug. + const school = ((await res.json()).schools ?? []).find( + (s: { religious_denomination?: string; admissions_policy?: string }) => + s.religious_denomination === 'None' && !/selective/i.test(s.admissions_policy ?? '')); + test.skip(!school, 'no school recorded with religious character "None" here'); + + const tags = await rowTags(page, school); + await expect(tags).toBeVisible(); + await expect(tags.getByText('Faith priority', { exact: true })).toHaveCount(0); + await expect(tags.getByText('None', { exact: true })).toHaveCount(0); +}); + +test('search and the school page agree on how many pupils a secondary has', async ({ page }) => { + // Search showed the GCSE year group as "pupils": Burntwood had 245 in + // search and 1,462 on its page. Both now carry the register's count. + const res = await page.request.get('/api/schools?search=school&phase=secondary&page_size=20'); + expect(res.ok()).toBeTruthy(); + const school = ((await res.json()).schools ?? []).find( + (s: { total_pupils?: number | null }) => s.total_pupils != null); + test.skip(!school, 'no secondary with a pupil count in this environment'); + const detail = await (await page.request.get(`/api/schools/${school.urn}`)).json(); + expect(school.total_pupils).toBe(detail.school_info.total_pupils); +}); + test('a phase outside primary/secondary filters to that phase, not to everything', async ({ page }) => { // The search page offers every GIAS phase, but the API only knew the grouped // ones and silently dropped the rest — so "Nursery" returned primaries. @@ -544,7 +599,7 @@ test('school with no performance data still gets a working detail page', async ( const candidates: number[] = []; for (const q of ['post 16', 'specialist college', 'sixth form']) { const resp = await page.request.get( - `/api/schools?search=${encodeURIComponent(q)}&per_page=20` + `/api/schools?search=${encodeURIComponent(q)}&page_size=20` ); if (!resp.ok()) continue; const body = await resp.json(); @@ -1123,7 +1178,7 @@ test('compare metric-help popover stays within the mobile viewport', async ({ pa test('admissions year/trend toggle still switches views after the server/client split', async ({ page }) => { // Find a school with at least two years carrying an offer rate — the toggle // only appears then. Data-invariant: uses whatever the environment holds. - const res = await page.request.get('/api/schools?search=primary&per_page=50'); + const res = await page.request.get('/api/schools?search=primary&page_size=50'); expect(res.ok()).toBeTruthy(); const candidates: number[] = ((await res.json()).schools ?? []).map((s: { urn: number }) => s.urn); @@ -2083,7 +2138,7 @@ test('English schools with Welsh postcodes are kept', async ({ page }) => { test('a Welsh school URL 404s while an English one still resolves', async ({ page }) => { // Paired on purpose: the Welsh assertion alone would also pass if the whole // site were down, which is the failure this test most needs to distinguish. - const english = await page.request.get('/api/schools?search=primary&per_page=1'); + const english = await page.request.get('/api/schools?search=primary&page_size=1'); expect(english.ok()).toBeTruthy(); const [first] = (await english.json()).schools ?? []; expect(first, 'no English school available to compare against').toBeTruthy(); @@ -2193,7 +2248,7 @@ test('a filtered homepage still canonicalises to the bare root', async ({ page } }); test('a school page canonicalises to its own slug on the www host', async ({ page }) => { - const res = await page.request.get('/api/schools?search=primary&per_page=1'); + const res = await page.request.get('/api/schools?search=primary&page_size=1'); expect(res.ok()).toBeTruthy(); const [first] = (await res.json()).schools ?? []; expect(first, 'no school available').toBeTruthy(); @@ -2268,7 +2323,7 @@ function blocksEverything(robots: string, agent: string): boolean { } test('a school page on staging is noindexed too, not just the homepage', async ({ page }) => { - const list = await page.request.get('/api/schools?search=primary&per_page=1'); + const list = await page.request.get('/api/schools?search=primary&page_size=1'); const [first] = (await list.json()).schools ?? []; expect(first, 'no school available').toBeTruthy(); @@ -2879,7 +2934,7 @@ test('with autosuggest off, the search box is a plain input', async ({ page }) = async function secondaryWithDestinations(page: Page): Promise<{ urn: string; destinations: any; }> { - const res = await page.request.get('/api/schools?search=school&per_page=100'); + const res = await page.request.get('/api/schools?search=school&page_size=100'); expect(res.ok()).toBeTruthy(); const body = await res.json(); const urns: string[] = (body.schools ?? []) @@ -2975,7 +3030,7 @@ test('switching to disadvantaged pupils never reveals a withheld figure', async }); test('a school with no sixth form has no post-16 destinations section', async ({ page }) => { - const res = await page.request.get('/api/schools?search=school&per_page=100'); + const res = await page.request.get('/api/schools?search=school&page_size=100'); const body = await res.json(); const noSixthForm = (body.schools ?? []) .filter((s: { phase?: string; has_sixth_form?: boolean }) => diff --git a/nextjs-app/__tests__/components/SchoolRow.test.tsx b/nextjs-app/__tests__/components/SchoolRow.test.tsx new file mode 100644 index 0000000..dcdab0e --- /dev/null +++ b/nextjs-app/__tests__/components/SchoolRow.test.tsx @@ -0,0 +1,36 @@ +/** + * SchoolRow (primary search results): line 2 prints the religious character + * only when the school has one. The register's "None" was printed as a chip. + */ + +import '@testing-library/jest-dom'; +import { render, screen } from '@testing-library/react'; +import { SchoolRow } from '@/components/SchoolRow'; +import type { School } from '@/lib/types'; + +const base = { + urn: 100001, + school_name: 'Alpha Primary School', + local_authority: 'Testshire', + school_type: 'Free schools', + phase: 'Primary', + gender: 'Mixed', + age_range: '4-11', + rwm_expected_pct: 70, +} as unknown as School; + +describe('SchoolRow religious character', () => { + it.each(['None', 'Does not apply', ''])( + 'prints nothing when the register says %p', + (religious_denomination) => { + render(); + expect(screen.queryByText('None')).not.toBeInTheDocument(); + expect(screen.queryByText('Does not apply')).not.toBeInTheDocument(); + }, + ); + + it('prints a religious character the school has', () => { + render(); + expect(screen.getByText('Church of England')).toBeInTheDocument(); + }); +}); diff --git a/nextjs-app/__tests__/components/SecondarySchoolRow.test.tsx b/nextjs-app/__tests__/components/SecondarySchoolRow.test.tsx index a868256..e3a25ac 100644 --- a/nextjs-app/__tests__/components/SecondarySchoolRow.test.tsx +++ b/nextjs-app/__tests__/components/SecondarySchoolRow.test.tsx @@ -65,3 +65,38 @@ describe('SecondarySchoolRow proposed-to-close tag', () => { expect(screen.queryByText(/Proposed to close/)).not.toBeInTheDocument(); }); }); + +describe('SecondarySchoolRow admissions tag', () => { + it('tags a selective school', () => { + render(); + expect(screen.getByText('Selective')).toBeInTheDocument(); + }); + + it('does not tag a non-selective school as selective', () => { + // "Non-selective" contains "selective": a substring test tagged every + // comprehensive (Burntwood, Graveney) as Selective. + render(); + expect(screen.queryByText('Selective')).not.toBeInTheDocument(); + }); + + it.each(['None', 'Does not apply', '', null])( + 'gives no faith tag when the religious character is %p', + (religious_denomination) => { + render( + , + ); + expect(screen.queryByText('Faith priority')).not.toBeInTheDocument(); + }, + ); + + it('tags a school with a religious character', () => { + render( + , + ); + expect(screen.getByText('Faith priority')).toBeInTheDocument(); + }); +}); diff --git a/nextjs-app/__tests__/components/schoolPupilCount.test.tsx b/nextjs-app/__tests__/components/schoolPupilCount.test.tsx new file mode 100644 index 0000000..d5d792f --- /dev/null +++ b/nextjs-app/__tests__/components/schoolPupilCount.test.tsx @@ -0,0 +1,56 @@ +/** + * "Pupils" on a school page is the size of the school. + * + * A year's results row carries the cohort its figures were measured on. For a + * secondary that is the GCSE year group alone (Burntwood: 245, against 1,462 + * on roll), so it must never stand in for the whole-school count. + */ + +import { screen } from '@testing-library/react'; +import { secondaryFixture } from '../support/schoolFixtures'; +import { renderSecondarySchoolDetail } from '../support/renderSchoolDetail'; + +jest.mock('@/lib/analytics', () => ({ + track: jest.fn(), + getNavigationSource: () => 'direct', +})); +jest.mock('@/components/PerformanceChart', () => ({ + PerformanceChart: () =>
, +})); +jest.mock('@/components/SatsChart', () => ({ + __esModule: true, + default: () =>
, +})); +jest.mock('@/components/AdmissionsTrendChart', () => ({ + __esModule: true, + default: () =>
, +})); +jest.mock('@/components/SchoolHeroMap', () => ({ + SchoolHeroMap: () =>
, + __esModule: true, +})); + +function withoutCensus(schoolTotal: number | null) { + const yearlyData = secondaryFixture.yearlyData.map((r) => ({ ...r, total_pupils: 245 })); + return { + ...secondaryFixture, + census: null, + yearlyData, + schoolInfo: { ...secondaryFixture.schoolInfo, total_pupils: schoolTotal }, + }; +} + +describe('pupil count without a census record', () => { + it('uses the register count in the header, not the results cohort', () => { + renderSecondarySchoolDetail(withoutCensus(1462)); + const pupils = screen.getByText('Pupils:').parentElement!; + expect(pupils).toHaveTextContent('1,462'); + expect(pupils).not.toHaveTextContent('245'); + }); + + it('shows no count rather than the results cohort when the register has none', () => { + renderSecondarySchoolDetail(withoutCensus(null)); + expect(screen.queryByText('Pupils:')).not.toBeInTheDocument(); + expect(screen.queryByText('Total pupils')).not.toBeInTheDocument(); + }); +}); diff --git a/nextjs-app/__tests__/support/renderSchoolDetail.tsx b/nextjs-app/__tests__/support/renderSchoolDetail.tsx index acc657d..5ab2b60 100644 --- a/nextjs-app/__tests__/support/renderSchoolDetail.tsx +++ b/nextjs-app/__tests__/support/renderSchoolDetail.tsx @@ -39,7 +39,6 @@ export function renderSchoolDetail(fixture: any) { withProviders( @@ -67,7 +66,6 @@ export function renderSecondarySchoolDetail(fixture: any) { withProviders( diff --git a/nextjs-app/app/(frontend)/school/[slug]/page.tsx b/nextjs-app/app/(frontend)/school/[slug]/page.tsx index 838ab67..a083de4 100644 --- a/nextjs-app/app/(frontend)/school/[slug]/page.tsx +++ b/nextjs-app/app/(frontend)/school/[slug]/page.tsx @@ -249,7 +249,6 @@ export default async function SchoolPage({ params }: SchoolPageProps) { {isSecondary ? ( @@ -273,7 +272,6 @@ export default async function SchoolPage({ params }: SchoolPageProps) { ) : ( diff --git a/nextjs-app/components/SchoolRow.tsx b/nextjs-app/components/SchoolRow.tsx index 306363a..fe167b0 100644 --- a/nextjs-app/components/SchoolRow.tsx +++ b/nextjs-app/components/SchoolRow.tsx @@ -9,7 +9,7 @@ */ import type { School } from '@/lib/types'; -import { formatPercentage, calculateTrend, getPhaseStyle, schoolUrl, buildOfstedListBadge, formatAgeRange, isProposedToClose, isSpecialSchool, listRwmValue } from '@/lib/utils'; +import { formatPercentage, calculateTrend, getPhaseStyle, schoolUrl, buildOfstedListBadge, formatAgeRange, hasReligiousCharacter, isProposedToClose, isSpecialSchool, listRwmValue } from '@/lib/utils'; import styles from './SchoolRow.module.css'; interface SchoolRowProps { @@ -34,9 +34,7 @@ export function SchoolRow({ const ofstedBadge = buildOfstedListBadge(school); const showGender = school.gender && school.gender.toLowerCase() !== 'mixed'; - const showDenomination = - school.religious_denomination && - school.religious_denomination !== 'Does not apply'; + const showDenomination = hasReligiousCharacter(school.religious_denomination); // The school's OWN figure and its year-over-year trend are same-school // measures — shown whenever there's a real value (not the all-zero diff --git a/nextjs-app/components/SecondarySchoolRow.tsx b/nextjs-app/components/SecondarySchoolRow.tsx index e10ca0a..762937d 100644 --- a/nextjs-app/components/SecondarySchoolRow.tsx +++ b/nextjs-app/components/SecondarySchoolRow.tsx @@ -11,14 +11,14 @@ 'use client'; import type { School } from '@/lib/types'; -import { buildOfstedListBadge, getPhaseStyle, schoolUrl, formatAgeRange, isProposedToClose, isSpecialSchool } from '@/lib/utils'; +import { buildOfstedListBadge, getPhaseStyle, schoolUrl, formatAgeRange, hasReligiousCharacter, isProposedToClose, isSpecialSchool } from '@/lib/utils'; import styles from './SecondarySchoolRow.module.css'; function detectAdmissionsTag(school: School): string | null { - const policy = school.admissions_policy?.toLowerCase() ?? ''; - if (policy.includes('selective')) return 'Selective'; - const denom = school.religious_denomination ?? ''; - if (denom && denom !== 'Does not apply') return 'Faith priority'; + // Exact match: "Non-selective" contains "selective", so a substring test + // tagged every comprehensive as Selective. + if (school.admissions_policy?.trim().toLowerCase() === 'selective') return 'Selective'; + if (hasReligiousCharacter(school.religious_denomination)) return 'Faith priority'; return null; } diff --git a/nextjs-app/components/school/SchoolDetailShell.tsx b/nextjs-app/components/school/SchoolDetailShell.tsx index ff031ca..2e0bd88 100644 --- a/nextjs-app/components/school/SchoolDetailShell.tsx +++ b/nextjs-app/components/school/SchoolDetailShell.tsx @@ -34,8 +34,6 @@ import styles from './SchoolDetailShell.module.css'; */ export interface SchoolDetailShellProps { schoolInfo: School; - /** Only for the header's pupil-count fallback. */ - yearlyData: SchoolResult[]; census: SchoolCensus | null; /** Section list for the sticky nav, computed on the server. */ navItems: NavItem[]; @@ -44,7 +42,7 @@ export interface SchoolDetailShellProps { } export function SchoolDetailShell({ - schoolInfo, yearlyData, census, navItems, children, + schoolInfo, census, navItems, children, }: SchoolDetailShellProps) { const router = useRouter(); const { addSchool, removeSchool, isSelected } = useComparison(); @@ -126,7 +124,6 @@ export function SchoolDetailShell({ // once on the server (lib/schoolSections) and consumed by the section // composers; recomputing them here would duplicate that work for values // this component never renders. - const latestResults = yearlyData.length > 0 ? yearlyData[yearlyData.length - 1] : null; const phase = schoolInfo.phase ?? ''; const isAllThrough = phase.toLowerCase() === 'all-through'; const hasLocation = schoolInfo.latitude != null && schoolInfo.longitude != null; @@ -283,7 +280,9 @@ export function SchoolDetailShell({ )} {(() => { - const total = census?.total_pupils ?? latestResults?.total_pupils ?? null; + // Never latestResults.total_pupils: that is the results + // cohort, which for a secondary is the GCSE year group alone. + const total = census?.total_pupils ?? schoolInfo.total_pupils ?? null; if (total == null) return null; return ( diff --git a/nextjs-app/components/school/WellbeingSection.tsx b/nextjs-app/components/school/WellbeingSection.tsx index 93fef14..7a1e035 100644 --- a/nextjs-app/components/school/WellbeingSection.tsx +++ b/nextjs-app/components/school/WellbeingSection.tsx @@ -54,7 +54,8 @@ export function WellbeingSection({
)} {(() => { - const total = census?.total_pupils ?? schoolInfo.total_pupils ?? latestResults?.total_pupils ?? null; + // Not latestResults.total_pupils: that is the GCSE year group. + const total = census?.total_pupils ?? schoolInfo.total_pupils ?? null; if (total == null) return null; const female = census?.female_pupils ?? null; const male = census?.male_pupils ?? null; diff --git a/nextjs-app/lib/utils.ts b/nextjs-app/lib/utils.ts index 3422edb..09a76c4 100644 --- a/nextjs-app/lib/utils.ts +++ b/nextjs-app/lib/utils.ts @@ -902,6 +902,15 @@ export function isSpecialSchool(school: { school_type?: string | null }): boolea return /\bspecial\b/.test(t) || /pupil referral/.test(t) || /alternative provision/.test(t); } +/** + * Whether GIAS records a religious character. "None" and "Does not apply" are + * the register's two ways of saying it has none, and neither is a faith. + */ +export function hasReligiousCharacter(value: string | null | undefined): boolean { + const v = value?.trim().toLowerCase() ?? ''; + return v !== '' && v !== 'none' && v !== 'does not apply'; +} + /** * The school's combined Reading, Writing & Maths figure, or null when there is * no real one to show.