fix(api): treat "16 plus" as secondary, the way PHASE_GROUPS already does
PR Checks / Frontend Typecheck + Tests (pull_request) Canceled after 9s
PR Checks / Backend Smoke (pull_request) Canceled after 0s
PR Checks / Build Backend (no push) (pull_request) Canceled after 0s
PR Checks / Build Frontend (no push) (pull_request) Canceled after 0s
PR Checks / Build Pipeline (no push) (pull_request) Canceled after 0s
PR Checks / AI Code Review (Claude) (pull_request) Canceled after 0s

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 <sixth form college>" 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 <noreply@anthropic.com>
This commit is contained in:
TudorandClaude Opus 5 committed 2026-09-22 06:41:49 +01:00
1 parent bd2a6c385b
commit 2175dccb7c
7 files changed
+121 -14

No files matched your search

+7 -6
View File
@@ -41,7 +41,7 @@ from .data_loader import get_data_info as get_db_info
from . import flags from . import flags
from .places import build_place_index, build_place_registry, places_for_urn from .places import build_place_index, build_place_registry, places_for_urn
from .schemas import METRIC_DEFINITIONS, PHASE_GROUPS, RANKING_COLUMNS, SCHOOL_COLUMNS 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 from .utils import clean_for_json, convert_to_native
# Values to exclude from filter dropdowns (empty strings, non-applicable labels) # 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. section simply does not render.
""" """
try: try:
phase_text = (phase or "").lower() # Decided in similar_schools, beside the PHASE_GROUPS bucket it selects
# All-through schools render with the primary template, which is what # from, so the two cannot drift. A substring test for "secondary" here
# decides the metric, so they are not secondary here. # would miss "16 plus" and hand a sixth-form college the primary bucket.
is_secondary = phase_text != "all-through" and "secondary" in phase_text return select_similar(
return select_similar(load_latest_school_data(), int(urn), is_secondary) load_latest_school_data(), int(urn), is_secondary_phase(phase)
)
except Exception: except Exception:
import logging import logging
+18
View File
@@ -89,6 +89,24 @@ def phase_label(phase: str | None) -> str:
return f"{text.capitalize()} school" 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]: def _phase_group(is_secondary: bool) -> set[str]:
return PHASE_GROUPS["secondary" if is_secondary else "primary"] return PHASE_GROUPS["secondary" if is_secondary else "primary"]
+33 -1
View File
@@ -11,7 +11,7 @@ set, never far enough to fill the last of the six slots.
import numpy as np import numpy as np
import pandas as pd 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. # Roughly 0.7 miles apart in latitude at this longitude.
BASE_LAT, BASE_LON = 51.5000, -0.1000 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)} 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(): def test_chips_state_only_what_the_tier_earned():
frame = _frame( frame = _frame(
_row(100001, "Subject", phase="Secondary", gender="Mixed", _row(100001, "Subject", phase="Secondary", gender="Mixed",
@@ -8,6 +8,7 @@
import { render, screen } from '@testing-library/react'; import { render, screen } from '@testing-library/react';
import { import {
nearbyNoun,
SimilarSchoolsSection, SimilarSchoolsSection,
shouldRenderSimilar, shouldRenderSimilar,
} from '@/components/school/SimilarSchoolsSection'; } from '@/components/school/SimilarSchoolsSection';
@@ -46,7 +47,7 @@ function renderSection(similar: SimilarSchool[]) {
<SimilarSchoolsSection <SimilarSchoolsSection
urn={100001} urn={100001}
schoolName="Meadowbrook Primary School" schoolName="Meadowbrook Primary School"
phaseNoun="primary" phase="Primary"
thisMetricValue={72} thisMetricValue={72}
similar={similar} similar={similar}
/>, />,
@@ -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(
<SimilarSchoolsSection
urn={100001}
schoolName="Barnet Sixth Form College"
phase="16 plus"
thisMetricValue={null}
similar={[school(), school({ urn: 100003 })]}
/>,
);
expect(screen.getByText(/Other schools and colleges near Barnet Sixth Form College/)).toBeInTheDocument();
expect(screen.queryByText(/primary schools/)).not.toBeInTheDocument();
});
});
describe('cards', () => { describe('cards', () => {
it('links each school to its canonical slug', () => { it('links each school to its canonical slug', () => {
renderSection([school(), school({ urn: 100003, school_name: 'Oakfield Primary School' })]); renderSection([school(), school({ urn: 100003, school_name: 'Oakfield Primary School' })]);
@@ -155,7 +155,7 @@ export function PrimarySchoolSections({
<SimilarSchoolsSection <SimilarSchoolsSection
urn={schoolInfo.urn} urn={schoolInfo.urn}
schoolName={schoolInfo.school_name} schoolName={schoolInfo.school_name}
phaseNoun={flags.isAllThrough ? 'all-through' : 'primary'} phase={schoolInfo.phase}
thisMetricValue={flags.latestResults?.rwm_expected_pct ?? null} thisMetricValue={flags.latestResults?.rwm_expected_pct ?? null}
similar={similarSchools} similar={similarSchools}
/> />
@@ -149,7 +149,7 @@ export function SecondarySchoolSections({
<SimilarSchoolsSection <SimilarSchoolsSection
urn={schoolInfo.urn} urn={schoolInfo.urn}
schoolName={schoolInfo.school_name} schoolName={schoolInfo.school_name}
phaseNoun="secondary" phase={schoolInfo.phase}
thisMetricValue={flags.latestResults?.attainment_8_score ?? null} thisMetricValue={flags.latestResults?.attainment_8_score ?? null}
similar={similarSchools} similar={similarSchools}
/> />
@@ -28,6 +28,28 @@ export function shouldRenderSimilar(similar?: SimilarSchool[] | null): boolean {
return (similar?.length ?? 0) >= MINIMUM; 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 <sixth form college>" 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 { function metricLabel(key: string): string {
return key === 'attainment_8_score' ? 'Attainment 8' : 'Reading, writing & maths'; 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({ export function SimilarSchoolsSection({
urn, urn,
schoolName, schoolName,
phaseNoun, phase,
thisMetricValue, thisMetricValue,
similar, similar,
}: { }: {
urn: number; urn: number;
schoolName: string; schoolName: string;
phaseNoun: string; /** The school's own GIAS phase, not the template it renders with. */
phase: string | null | undefined;
thisMetricValue: number | null; thisMetricValue: number | null;
similar?: SimilarSchool[] | null; similar?: SimilarSchool[] | null;
}) { }) {
@@ -57,6 +80,7 @@ export function SimilarSchoolsSection({
// shares an intake with this school. // shares an intake with this school.
const loosest = Math.max(...schools.map((s) => s.tier)); const loosest = Math.max(...schools.map((s) => s.tier));
const metricKey = schools[0].metric_key; const metricKey = schools[0].metric_key;
const noun = nearbyNoun(phase);
return ( return (
<Section id="similar"> <Section id="similar">
@@ -70,8 +94,8 @@ export function SimilarSchoolsSection({
</h2> </h2>
<p className={styles.lede}> <p className={styles.lede}>
{loosest >= 3 {loosest >= 3
? `Other ${phaseNoun} schools near ${schoolName}.` ? `Other ${noun} near ${schoolName}.`
: `Other ${phaseNoun} schools near ${schoolName}, with a similar intake.`} : `Other ${noun} near ${schoolName}, with a similar intake.`}
</p> </p>
</div> </div>
} }