From 2175dccb7c91385c5c9e7595ee53bcef99019c40 Mon Sep 17 00:00:00 2001 From: Tudor Date: Tue, 22 Sep 2026 06:41:49 +0100 Subject: [PATCH] fix(api): treat "16 plus" as secondary, the way PHASE_GROUPS already does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GIAS phase 6 is "16 plus", and PHASE_GROUPS deliberately files it in the secondary group. The payload helper decided the same question with `"secondary" in phase_text`, which that value does not satisfy — so a sixth-form college was handed the primary bucket and offered infant schools as its peers, with the KS2 metric key to label them. No crash; just a page confidently showing the wrong schools. The decision now lives in similar_schools.is_secondary_phase, beside the PHASE_GROUPS bucket it selects from, so the two cannot drift again. A test pins them together. The same binary assumption had a second output. computeSchoolFlags tests for the substring too, so a 16-plus school renders the primary template, and the composer was labelling the section from the template: "Other primary schools near " above a row of secondaries. The section now derives its noun from the school's own phase, which also removes the duplicated wording from both composers. A 16-plus school's candidates span the whole secondary group, so no single noun fits and it gets the honest general one. Reported in review on #150. Co-Authored-By: Claude Opus 5 --- backend/app.py | 13 +++---- backend/similar_schools.py | 18 ++++++++++ backend/tests/test_similar_schools.py | 34 ++++++++++++++++++- .../components/SimilarSchoolsSection.test.tsx | 34 ++++++++++++++++++- .../school/PrimarySchoolSections.tsx | 2 +- .../school/SecondarySchoolSections.tsx | 2 +- .../school/SimilarSchoolsSection.tsx | 32 ++++++++++++++--- 7 files changed, 121 insertions(+), 14 deletions(-) diff --git a/backend/app.py b/backend/app.py index cc79cbd..23826c2 100644 --- a/backend/app.py +++ b/backend/app.py @@ -41,7 +41,7 @@ from .data_loader import get_data_info as get_db_info from . import flags from .places import build_place_index, build_place_registry, places_for_urn from .schemas import METRIC_DEFINITIONS, PHASE_GROUPS, RANKING_COLUMNS, SCHOOL_COLUMNS -from .similar_schools import select_similar +from .similar_schools import is_secondary_phase, select_similar from .utils import clean_for_json, convert_to_native # Values to exclude from filter dropdowns (empty strings, non-applicable labels) @@ -274,11 +274,12 @@ def _similar_schools_payload(urn: int, phase: str | None) -> list[dict]: section simply does not render. """ try: - phase_text = (phase or "").lower() - # All-through schools render with the primary template, which is what - # decides the metric, so they are not secondary here. - is_secondary = phase_text != "all-through" and "secondary" in phase_text - return select_similar(load_latest_school_data(), int(urn), is_secondary) + # Decided in similar_schools, beside the PHASE_GROUPS bucket it selects + # from, so the two cannot drift. A substring test for "secondary" here + # would miss "16 plus" and hand a sixth-form college the primary bucket. + return select_similar( + load_latest_school_data(), int(urn), is_secondary_phase(phase) + ) except Exception: import logging diff --git a/backend/similar_schools.py b/backend/similar_schools.py index 72b3318..0e75467 100644 --- a/backend/similar_schools.py +++ b/backend/similar_schools.py @@ -89,6 +89,24 @@ def phase_label(phase: str | None) -> str: return f"{text.capitalize()} school" +def is_secondary_phase(phase: str | None) -> bool: + """Whether this phase takes the secondary side: secondary group membership, + minus all-through. + + Membership is read from PHASE_GROUPS rather than tested with `"secondary" in + phase`, because that substring misses "16 plus" — GIAS phase 6, which + PHASE_GROUPS deliberately files as secondary. The substring version fails + silently rather than loudly: a sixth-form college is simply handed the + primary bucket and offered infant schools as peers. + + All-through is the exception. PHASE_GROUPS lists it on both sides because it + belongs on both phases' place pages, but the detail page renders it with the + primary template, and the metric follows the template. + """ + text = (phase or "").strip().lower() + return text != "all-through" and text in PHASE_GROUPS["secondary"] + + def _phase_group(is_secondary: bool) -> set[str]: return PHASE_GROUPS["secondary" if is_secondary else "primary"] diff --git a/backend/tests/test_similar_schools.py b/backend/tests/test_similar_schools.py index 356902a..ad4aca7 100644 --- a/backend/tests/test_similar_schools.py +++ b/backend/tests/test_similar_schools.py @@ -11,7 +11,7 @@ set, never far enough to fill the last of the six slots. import numpy as np import pandas as pd -from backend.similar_schools import select_similar +from backend.similar_schools import is_secondary_phase, select_similar # Roughly 0.7 miles apart in latitude at this longitude. BASE_LAT, BASE_LON = 51.5000, -0.1000 @@ -203,6 +203,38 @@ def test_all_through_is_offered_on_both_phase_sides(): assert 100002 in {s["urn"] for s in select_similar(secondary, 100010, is_secondary=True)} +def test_sixteen_plus_is_matched_against_secondary_not_primary(): + """GIAS phase 6 is "16 plus", and PHASE_GROUPS puts it in the secondary + group — a sixth-form college's peers are secondaries and other colleges, + never primary schools. A substring test for "secondary" misses it silently: + no crash, just a page offering infant schools to a sixth form.""" + frame = _frame( + _row(100001, "Sixth Form College", phase="16 plus", age_range="16-19"), + _row(100002, "Nearby Secondary", phase="Secondary", latitude=_at(0.5), + attainment_8_score=52.0), + _row(100003, "Nearby College", phase="16 plus", latitude=_at(0.6), + attainment_8_score=np.nan), + _row(100004, "Nearby Primary", phase="Primary", latitude=_at(0.1)), + ) + result = select_similar(frame, 100001, is_secondary=is_secondary_phase("16 plus")) + urns = {s["urn"] for s in result} + assert 100004 not in urns, "a primary school is not a peer for a sixth form" + assert urns == {100002, 100003} + assert all(s["metric_key"] == "attainment_8_score" for s in result) + + +def test_is_secondary_phase_agrees_with_the_phase_groups_it_selects_from(): + """The two must not drift: whatever this calls secondary decides which + PHASE_GROUPS bucket the candidates come from.""" + for phase in ("Secondary", "Middle deemed secondary", "16 plus"): + assert is_secondary_phase(phase) is True, phase + for phase in ("Primary", "Middle deemed primary", "Nursery", "", None): + assert is_secondary_phase(phase) is False, phase + # In PHASE_GROUPS an all-through school is on both sides, but it renders + # with the primary template, and the metric follows the template. + assert is_secondary_phase("All-through") is False + + def test_chips_state_only_what_the_tier_earned(): frame = _frame( _row(100001, "Subject", phase="Secondary", gender="Mixed", diff --git a/nextjs-app/__tests__/components/SimilarSchoolsSection.test.tsx b/nextjs-app/__tests__/components/SimilarSchoolsSection.test.tsx index 5eda120..646ad89 100644 --- a/nextjs-app/__tests__/components/SimilarSchoolsSection.test.tsx +++ b/nextjs-app/__tests__/components/SimilarSchoolsSection.test.tsx @@ -8,6 +8,7 @@ import { render, screen } from '@testing-library/react'; import { + nearbyNoun, SimilarSchoolsSection, shouldRenderSimilar, } from '@/components/school/SimilarSchoolsSection'; @@ -46,7 +47,7 @@ function renderSection(similar: SimilarSchool[]) { , @@ -85,6 +86,37 @@ describe('the claim the lede makes', () => { }); }); +describe('what the lede calls the set', () => { + it.each([ + ['Primary', 'primary schools'], + ['Middle deemed primary', 'primary schools'], + ['Secondary', 'secondary schools'], + ['Middle deemed secondary', 'secondary schools'], + ['All-through', 'all-through schools'], + // GIAS phase 6. Its candidates span the whole secondary group, so no + // single noun fits and it takes the honest general one. + ['16 plus', 'schools and colleges'], + ['', 'schools'], + [null, 'schools'], + ])('calls a %s school\'s neighbours "%s"', (phase, expected) => { + expect(nearbyNoun(phase)).toBe(expected); + }); + + it('never calls a sixth form college\'s neighbours primary schools', () => { + render( + , + ); + expect(screen.getByText(/Other schools and colleges near Barnet Sixth Form College/)).toBeInTheDocument(); + expect(screen.queryByText(/primary schools/)).not.toBeInTheDocument(); + }); +}); + describe('cards', () => { it('links each school to its canonical slug', () => { renderSection([school(), school({ urn: 100003, school_name: 'Oakfield Primary School' })]); diff --git a/nextjs-app/components/school/PrimarySchoolSections.tsx b/nextjs-app/components/school/PrimarySchoolSections.tsx index 4898c78..18bf76c 100644 --- a/nextjs-app/components/school/PrimarySchoolSections.tsx +++ b/nextjs-app/components/school/PrimarySchoolSections.tsx @@ -155,7 +155,7 @@ export function PrimarySchoolSections({ diff --git a/nextjs-app/components/school/SecondarySchoolSections.tsx b/nextjs-app/components/school/SecondarySchoolSections.tsx index 6cc2a25..a660e1f 100644 --- a/nextjs-app/components/school/SecondarySchoolSections.tsx +++ b/nextjs-app/components/school/SecondarySchoolSections.tsx @@ -149,7 +149,7 @@ export function SecondarySchoolSections({ diff --git a/nextjs-app/components/school/SimilarSchoolsSection.tsx b/nextjs-app/components/school/SimilarSchoolsSection.tsx index b9a8e9b..76db7e1 100644 --- a/nextjs-app/components/school/SimilarSchoolsSection.tsx +++ b/nextjs-app/components/school/SimilarSchoolsSection.tsx @@ -28,6 +28,28 @@ export function shouldRenderSimilar(similar?: SimilarSchool[] | null): boolean { return (similar?.length ?? 0) >= MINIMUM; } +/** + * What the lede calls the set of schools it is showing. + * + * Derived from the school's own GIAS phase rather than the template it renders + * with, because those disagree for "16 plus" (GIAS phase 6): a sixth-form + * college renders the primary template — computeSchoolFlags tests for the + * substring "secondary" — while the backend correctly matches it against the + * secondary group. Taking the noun from the template would print "Other primary + * schools near " above a row of secondaries. + * + * A 16-plus school's candidates span the whole secondary group, so no single + * noun fits and it gets the honest general one. + */ +export function nearbyNoun(phase: string | null | undefined): string { + const text = (phase ?? '').trim().toLowerCase(); + if (text === 'all-through') return 'all-through schools'; + if (text === '16 plus') return 'schools and colleges'; + if (text.includes('secondary')) return 'secondary schools'; + if (text.includes('primary')) return 'primary schools'; + return 'schools'; +} + function metricLabel(key: string): string { return key === 'attainment_8_score' ? 'Attainment 8' : 'Reading, writing & maths'; } @@ -40,13 +62,14 @@ function formatMetric(value: number | null, key: string): string { export function SimilarSchoolsSection({ urn, schoolName, - phaseNoun, + phase, thisMetricValue, similar, }: { urn: number; schoolName: string; - phaseNoun: string; + /** The school's own GIAS phase, not the template it renders with. */ + phase: string | null | undefined; thisMetricValue: number | null; similar?: SimilarSchool[] | null; }) { @@ -57,6 +80,7 @@ export function SimilarSchoolsSection({ // shares an intake with this school. const loosest = Math.max(...schools.map((s) => s.tier)); const metricKey = schools[0].metric_key; + const noun = nearbyNoun(phase); return (
@@ -70,8 +94,8 @@ export function SimilarSchoolsSection({

{loosest >= 3 - ? `Other ${phaseNoun} schools near ${schoolName}.` - : `Other ${phaseNoun} schools near ${schoolName}, with a similar intake.`} + ? `Other ${noun} near ${schoolName}.` + : `Other ${noun} near ${schoolName}, with a similar intake.`}

}