fix(search): tag only what the register records, and count the whole school #176

Merged
tudor merged 3 commits from fix/search-row-facts into main 2026-10-02 22:05:56 +00:00
7 changed files with 144 additions and 12 deletions
Showing only changes of commit 5f9caad7f4 - Show all commits

No files matched your search

+15 -2
View File
@@ -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,
}
+67
View File
@@ -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
@@ -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: () => <div data-testid="performance-chart" />,
}));
jest.mock('@/components/SatsChart', () => ({
__esModule: true,
default: () => <div data-testid="sats-chart" />,
}));
jest.mock('@/components/AdmissionsTrendChart', () => ({
__esModule: true,
default: () => <div data-testid="admissions-trend-chart" />,
}));
jest.mock('@/components/SchoolHeroMap', () => ({
SchoolHeroMap: () => <div data-testid="hero-map" />,
__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();
});
});
@@ -39,7 +39,6 @@ export function renderSchoolDetail(fixture: any) {
withProviders(
<SchoolDetailShell
schoolInfo={fixture.schoolInfo}
yearlyData={fixture.yearlyData}
census={fixture.census}
navItems={navItems}
>
@@ -67,7 +66,6 @@ export function renderSecondarySchoolDetail(fixture: any) {
withProviders(
<SchoolDetailShell
schoolInfo={fixture.schoolInfo}
yearlyData={fixture.yearlyData}
census={fixture.census}
navItems={navItems}
>
@@ -249,7 +249,6 @@ export default async function SchoolPage({ params }: SchoolPageProps) {
{isSecondary ? (
<SchoolDetailShell
schoolInfo={school_info}
yearlyData={yearly_data}
census={census ?? null}
navItems={secondaryNavItems}
>
@@ -273,7 +272,6 @@ export default async function SchoolPage({ params }: SchoolPageProps) {
) : (
<SchoolDetailShell
schoolInfo={school_info}
yearlyData={yearly_data}
census={census ?? null}
navItems={primaryNavItems}
>
@@ -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({
</span>
)}
{(() => {
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 (
<span className={styles.headerDetail}>
@@ -54,7 +54,8 @@ export function WellbeingSection({
</div>
)}
{(() => {
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;