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/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/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;