From 9a1f56c431f0809dead7b5853a79717954a69d89 Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 27 Aug 2026 08:49:14 +0100 Subject: [PATCH] feat(places): say what each school is, not only how it scored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The location tables carried one column: a percentage. A parent shortlisting from a town page is asking a different question first — does it take my child's age, is it a faith school, does it have a nursery — and the page could not answer any of it. Primary tables gain Ages, Religious character, Nursery and Constituency; secondary tables the same minus Nursery, which is a question about a different intake. An all-through school renders in both groups, so its nursery shows under primary alone. The measure moves to the second column rather than the last. Six columns overflow a phone and .tableWrap turns that into a horizontal swipe; with the measure last, the one number the page exists for is the one scrolled off the screen. Cell rules are the ones the school page already uses, so the two surfaces cannot disagree about the same school: "Does not apply", "None" and "Not applicable" all read as no religious character, and the en-dash age normalisation moves into formatAgeSpan, which formatAgeRange now delegates to. Backend: nursery_provision and parliamentary_constituency were not in the place response. Both are optional GIAS mart columns that data_loader degrades to NULL, and the `in rows.columns` guard keeps a mart the pipeline has not rebuilt working. Also fixes a live bug on the same line: SCHOOL_COLUMNS already ends with latitude and longitude, and the endpoint concatenated them again, so pandas dropped one of every duplicated pair and warned "columns are not unique" on each request. Ordered de-duplication removes the warning and the silent drop. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FuPUioHpxtaiDNagQvjxyM --- backend/app.py | 19 ++- backend/tests/test_places_api.py | 46 ++++++ e2e/tests/journeys.spec.ts | 53 +++++++ .../__tests__/components/PlaceView.test.tsx | 131 ++++++++++++++++++ nextjs-app/__tests__/lib/utils.test.ts | 26 ++++ .../components/places/PlaceView.module.css | 28 ++++ nextjs-app/components/places/PlaceView.tsx | 45 +++++- nextjs-app/lib/utils.ts | 14 +- 8 files changed, 356 insertions(+), 6 deletions(-) diff --git a/backend/app.py b/backend/app.py index 9e60c1d..a513aea 100644 --- a/backend/app.py +++ b/backend/app.py @@ -1337,9 +1337,22 @@ async def get_place(request: Request, kind: str, slug: str, for m in ("rwm_expected_pct", "attainment_8_score") } - cols = [c for c in SCHOOL_COLUMNS + ["latitude", "longitude", "phase", - "rwm_expected_pct", "attainment_8_score", - "total_pupils"] + # dict.fromkeys, not a list: SCHOOL_COLUMNS already ends with latitude and + # longitude, so concatenating them again selected each twice and pandas + # dropped one of every duplicated pair with a "columns are not unique" + # warning. Ordered de-duplication keeps the column order and the warning + # cannot come back. + # + # nursery_provision and parliamentary_constituency are not in + # SCHOOL_COLUMNS and the place table shows both. The `in rows.columns` + # guard is what keeps a mart the pipeline has not rebuilt working: those + # two are the optional GIAS columns data_loader degrades to NULL. + cols = [c for c in dict.fromkeys( + SCHOOL_COLUMNS + ["latitude", "longitude", "phase", + "nursery_provision", + "parliamentary_constituency", + "rwm_expected_pct", "attainment_8_score", + "total_pupils"]) if c in rows.columns] return { diff --git a/backend/tests/test_places_api.py b/backend/tests/test_places_api.py index 1d8f701..716bc3a 100644 --- a/backend/tests/test_places_api.py +++ b/backend/tests/test_places_api.py @@ -137,3 +137,49 @@ def test_an_authority_without_a_page_is_named_but_carries_no_slug(straddling_cli by_name = {a["name"]: a for a in body["place"]["authorities"]} assert by_name["Essex"]["slug"] == "essex" assert by_name["Isles Of Scilly"]["slug"] is None + + +def _attributed_df() -> pd.DataFrame: + """The same town, with the four attributes the place table now shows.""" + df = _schools_df() + df["age_range"] = "4-11" + df["religious_denomination"] = "Church of England" + df["nursery_provision"] = True + df["parliamentary_constituency"] = "Brentwood and Ongar" + return df + + +@pytest.fixture() +def attributed_client(monkeypatch): + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _attributed_df) + monkeypatch.setattr(app_module, "load_latest_school_data", _attributed_df) + monkeypatch.setattr(app_module, "_place_registry", None) + return TestClient(app_module.app, raise_server_exceptions=False) + + +def test_place_detail_carries_the_attributes_the_table_shows(attributed_client): + """age_range and religious_denomination ride in on SCHOOL_COLUMNS. + + nursery_provision and parliamentary_constituency do not, and the place + table needs all four — a column the response cannot fill is a column of + dashes on ~3,900 pages. + """ + body = attributed_client.get("/api/places/town/brentwood").json() + school = body["schools"][0] + assert school["age_range"] == "4-11" + assert school["religious_denomination"] == "Church of England" + assert school["nursery_provision"] is True + assert school["parliamentary_constituency"] == "Brentwood and Ongar" + + +def test_place_detail_survives_a_mart_without_the_optional_columns(client): + """The base fixture has neither column, as an unrebuilt mart does not. + + data_loader degrades those to NULL rather than failing the load, so the + endpoint must not assume they are present. + """ + res = client.get("/api/places/town/brentwood") + assert res.status_code == 200 + assert "nursery_provision" not in res.json()["schools"][0] diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index 1a55540..966f2ab 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1978,6 +1978,59 @@ test('a place page links its phase variants, and they resolve', async ({ page }) await expect(page.locator('h1')).toContainText(new RegExp(`${phase} schools in`, 'i')); }); +/* + * The table shipped with one column of scores. A parent shortlisting from a + * town page needs to know whether a school takes their child's age, whether + * it is a faith school, and — for a primary — whether it has a nursery, + * before a percentage means anything. + * + * These assert the column headings rather than the values: nursery_provision + * and parliamentary_constituency are optional mart columns, and on an + * environment whose pipeline has not rebuilt them the API degrades them to + * absent. A value assertion would then fail for a data reason, not a code one. + */ +async function phasedPlace(page: Page, phase: 'primary' | 'secondary') { + const place = await firstPlaceOfKind(page, 'town'); + const detail = await (await page.request.get(`/api/places/town/${place.slug}`)).json(); + test.skip(!(detail.place.phases ?? []).includes(phase), + `no ${phase} page clears the threshold here`); + return place; +} + +test('a primary place page names each school as well as scoring it', async ({ page }) => { + const place = await phasedPlace(page, 'primary'); + await page.goto(`/schools/${place.slug}/primary`); + for (const heading of ['Ages', 'Religious character', 'Nursery', 'Constituency']) { + await expect(page.getByRole('columnheader', { name: heading, exact: true })) + .toBeVisible(); + } + // age_range rides in on SCHOOL_COLUMNS and predates the optional columns, + // so it is the one attribute safe to assert a value for anywhere. + await expect(page.locator('table tbody td').filter({ hasText: /^\d+–\d+$/ }).first()) + .toBeVisible(); +}); + +test('a secondary place page does not ask about nurseries', async ({ page }) => { + const place = await phasedPlace(page, 'secondary'); + await page.goto(`/schools/${place.slug}/secondary`); + await expect(page.getByRole('columnheader', { name: 'Ages', exact: true })) + .toBeVisible(); + await expect(page.getByRole('columnheader', { name: 'Nursery', exact: true })) + .toHaveCount(0); +}); + +test('the measure stays beside the school name, not behind a swipe', async ({ page }) => { + // Six columns overflow a phone; .tableWrap turns that into a horizontal + // scroll. With the measure last, the number the page exists for is the one + // off the screen. + const place = await phasedPlace(page, 'primary'); + await page.setViewportSize({ width: 390, height: 844 }); + await page.goto(`/schools/${place.slug}/primary`); + const second = page.locator('table thead th').nth(1); + await expect(second).toContainText(/reading, writing/i); + await expect(second).toBeInViewport(); +}); + test('phase variants are submitted in the places sitemap', async ({ page }) => { const xml = await (await page.request.get('/sitemaps/places-1.xml')).text(); expect(xml).toMatch(/\/schools\/[a-z0-9-]+\/primary { .toContain('Isles Of Scilly'); }); }); + +describe('PlaceView school attributes', () => { + /* + * The table shipped with one column of scores, which answers "how did they + * do" and nothing about whether the school is one a family could use. Age + * range, faith, nursery and constituency are the four facts a parent + * filters on before they look at a number at all. + */ + const withAttributes: PlaceDetail = { + place: { kind: 'town', slug: 'chelmsford', name: 'Chelmsford', count: 3, + parent_authority: 'Essex', phases: ['primary', 'secondary'] }, + schools: [ + { urn: 1, school_name: 'Alpha Primary', phase: 'Primary', + rwm_expected_pct: 82, attainment_8_score: null, + age_range: '4-11', religious_denomination: 'Church of England', + nursery_provision: true, + parliamentary_constituency: 'Chelmsford' } as never, + { urn: 2, school_name: 'Beta High', phase: 'Secondary', + rwm_expected_pct: null, attainment_8_score: 47, + age_range: '11-16', religious_denomination: 'Does not apply', + nursery_provision: false, + parliamentary_constituency: 'Witham' } as never, + ], + averages: { rwm_expected_pct: 63, attainment_8_score: 45 }, + }; + + function headings(container: HTMLElement, table = 0): string[] { + return Array.from(container.querySelectorAll('table')[table] + .querySelectorAll('thead th')).map((th) => th.textContent ?? ''); + } + + it('heads a primary table with all four attributes', () => { + const { container } = render(); + expect(headings(container)).toEqual([ + 'School', 'Reading, writing & maths', + 'Ages', 'Religious character', 'Nursery', 'Constituency', + ]); + }); + + it('omits nursery from a secondary table, where it does not apply', () => { + const { container } = render(); + expect(headings(container, 1)).toEqual([ + 'School', 'Attainment 8', 'Ages', 'Religious character', 'Constituency', + ]); + }); + + it('keeps the measure beside the school name, where a phone can see it', () => { + // Six columns overflow a phone and .tableWrap turns that into a swipe. + // With the measure last, the one number the page exists for is the one + // scrolled off the screen. + const { container } = render(); + expect(headings(container)[1]).toBe('Reading, writing & maths'); + }); + + it('shows the age range without repeating the column heading', () => { + render(); + expect(screen.getByText('4–11')).toBeInTheDocument(); + expect(screen.queryByText('Ages 4–11')).not.toBeInTheDocument(); + }); + + it('names the faith of a faith school', () => { + render(); + expect(screen.getByText('Church of England')).toBeInTheDocument(); + }); + + it('reads "Does not apply" as no religious character, not as a value', () => { + // GIAS spells the absence of a faith as "Does not apply", which is a + // database answer rather than an English one. The school page already + // suppresses it; the two must not disagree about the same school. + const { container } = render(); + const secondary = container.querySelectorAll('table')[1] + .querySelectorAll('tbody td'); + expect(secondary[3].textContent).toBe('—'); + expect(screen.queryByText(/Does not apply/)).not.toBeInTheDocument(); + }); + + it('marks a nursery as such and a school without one as not', () => { + const { container } = render(); + const cells = container.querySelectorAll('table')[0] + .querySelectorAll('tbody td'); + expect(cells[4].textContent).toBe('Yes'); + }); + + it('names the constituency of each school', () => { + render(); + expect(screen.getByText('Chelmsford', { selector: 'td' })).toBeInTheDocument(); + expect(screen.getByText('Witham', { selector: 'td' })).toBeInTheDocument(); + }); + + it('dashes an attribute the data does not carry', () => { + // nursery_provision and parliamentary_constituency are absent from marts + // the pipeline has not rebuilt, and the API degrades them to null rather + // than failing. A row must survive that. + const bare: PlaceDetail = { + ...withAttributes, + schools: [{ urn: 3, school_name: 'Gamma Primary', phase: 'Primary', + rwm_expected_pct: 70 } as never], + }; + const { container } = render(); + const cells = Array.from(container.querySelectorAll('tbody td')) + .map((td) => td.textContent); + expect(cells.slice(2)).toEqual(['—', '—', '—', '—']); + }); + + it('gives an all-through school its nursery under primary only', () => { + // All-through schools render in both groups. Nursery belongs to the + // primary reading of the same school, not the secondary one. + const allThrough: PlaceDetail = { + ...withAttributes, + schools: [{ urn: 4, school_name: 'Delta Academy', phase: 'All-through', + rwm_expected_pct: 66, attainment_8_score: 51, + age_range: '4-18', religious_denomination: 'None', + nursery_provision: true, + parliamentary_constituency: 'Chelmsford' } as never], + }; + const { container } = render(); + const tables = container.querySelectorAll('table'); + expect(tables[0].textContent).toContain('Yes'); + expect(tables[1].textContent).not.toContain('Yes'); + }); +}); diff --git a/nextjs-app/__tests__/lib/utils.test.ts b/nextjs-app/__tests__/lib/utils.test.ts index 38e3b78..b5f74a4 100644 --- a/nextjs-app/__tests__/lib/utils.test.ts +++ b/nextjs-app/__tests__/lib/utils.test.ts @@ -13,6 +13,8 @@ import { metricKind, shortName, computeYBounds, + formatAgeRange, + formatAgeSpan, } from '@/lib/utils'; describe('formatPercentage', () => { @@ -320,3 +322,27 @@ describe('shortName', () => { expect(shortName('A'.repeat(30), 10)).toBe('AAAAAAAAA…'); }); }); + +describe('formatAgeSpan', () => { + it('normalises a hyphenated range to an en dash, without a label', () => { + // The place table carries "Ages" in the column heading, so repeating it + // in every cell is noise. formatAgeRange keeps the label for the contexts + // that have no heading to hang it on. + expect(formatAgeSpan('4-11')).toBe('4–11'); + }); + + it('leaves a range it does not recognise alone rather than mangling it', () => { + expect(formatAgeSpan('3-19 (SEN)')).toBe('3-19 (SEN)'); + }); + + it('returns an empty string for a missing range', () => { + expect(formatAgeSpan(null)).toBe(''); + expect(formatAgeSpan(undefined)).toBe(''); + }); +}); + +describe('formatAgeRange', () => { + it('keeps its label, so the two helpers stay distinguishable', () => { + expect(formatAgeRange('4-11')).toBe('Ages 4–11'); + }); +}); diff --git a/nextjs-app/components/places/PlaceView.module.css b/nextjs-app/components/places/PlaceView.module.css index b304b37..6a8d4af 100644 --- a/nextjs-app/components/places/PlaceView.module.css +++ b/nextjs-app/components/places/PlaceView.module.css @@ -164,6 +164,34 @@ white-space: nowrap; } +/* + * Attribute columns. Muted, because they qualify the row rather than compete + * with the measure for it, and hugging their content so the school name keeps + * the spare width — the same width:1% trick as .num, which is what stops six + * columns from splitting evenly and squeezing the names into two lines each. + * + * .attr never wraps: "4–11" and "Yes" broken across lines read as two values. + * .attrWide may — "Church of England" and some constituency names are long + * enough that forcing one line would push the measure off a phone screen. + */ +.table th.attr, +.table td.attr, +.table th.attrWide, +.table td.attrWide { + color: var(--text-secondary); + width: 1%; +} + +.table th.attr, +.table td.attr { + white-space: nowrap; +} + +.table th.attrWide, +.table td.attrWide { + min-width: 8rem; +} + /* The measure is spelled out; the tooltip carries the definition. */ .metricHead { text-decoration: none; diff --git a/nextjs-app/components/places/PlaceView.tsx b/nextjs-app/components/places/PlaceView.tsx index 3aa90ac..b27c935 100644 --- a/nextjs-app/components/places/PlaceView.tsx +++ b/nextjs-app/components/places/PlaceView.tsx @@ -13,7 +13,7 @@ import Link from 'next/link'; import type { PlaceDetail, PlaceSummary } from '@/lib/places'; import { placeUrl, authoritySlug } from '@/lib/places'; import type { School } from '@/lib/types'; -import { schoolUrl } from '@/lib/utils'; +import { schoolUrl, formatAgeSpan } from '@/lib/utils'; import { absoluteUrl } from '@/lib/site'; import { TrackPlaceView } from './TrackPlaceView'; import styles from './PlaceView.module.css'; @@ -64,8 +64,31 @@ function isPhase(school: School, phase: PhaseKey): boolean { : p.includes('primary') || p.includes('middle'); } +/* + * GIAS spells the absence of a faith as "Does not apply", and sometimes + * "None" or "Not applicable" — database answers, not English ones. The school + * page and the comparison already suppress all three; this is the same rule, + * so the two surfaces cannot disagree about the same school. + */ +const NO_FAITH = /^(none|does not apply|not applicable)$/i; + +/** An attribute the data does not carry. Distinct from the measure's "Not + * published": four of those per row would drown the row it qualifies. */ +const NO_VALUE = '—'; + +function faithOf(school: School): string { + const denom = school.religious_denomination ?? ''; + return denom && !NO_FAITH.test(denom) ? denom : NO_VALUE; +} + function SchoolTable({ schools, phase }: { schools: School[]; phase: PhaseKey }) { const metric = METRICS[phase]; + /* + * Nursery is a primary question. An all-through school renders in both + * groups, and its nursery belongs to the primary reading of it — under + * "Secondary schools" the column would be a fact about a different intake. + */ + const showNursery = phase === 'primary'; return (
@@ -79,6 +102,13 @@ function SchoolTable({ schools, phase }: { schools: School[]; phase: PhaseKey }) {metric.heading} + {/* The measure sits second, not last. Six columns overflow a + phone and .tableWrap turns that into a swipe; last would put + the one number the page exists for off the screen. */} + + + {showNursery && } + @@ -96,6 +126,19 @@ function SchoolTable({ schools, phase }: { schools: School[]; phase: PhaseKey }) ? Not published : `${Math.round(Number(value))}${metric.unit}`} + + + {showNursery && ( + + )} + ); })} diff --git a/nextjs-app/lib/utils.ts b/nextjs-app/lib/utils.ts index 2d7e931..42864ef 100644 --- a/nextjs-app/lib/utils.ts +++ b/nextjs-app/lib/utils.ts @@ -82,11 +82,21 @@ export function shortName(name: string, maxLength = 32): string { * Display-only — leaves the raw `age_range` field (used for sixth-form * detection) untouched. Falls back to the raw value if it's not a plain range. */ -export function formatAgeRange(ageRange: string | null | undefined): string { +export function formatAgeSpan(ageRange: string | null | undefined): string { if (!ageRange) return ''; const match = ageRange.match(/^\s*(\d+)\s*[-–]\s*(\d+)\s*$/); if (!match) return ageRange; - return `Ages ${match[1]}–${match[2]}`; + return `${match[1]}–${match[2]}`; +} + +/** + * The same span, labelled — for the places that show it with no column + * heading to carry the word "Ages". Delegates so the en-dash normalisation + * lives in one place. + */ +export function formatAgeRange(ageRange: string | null | undefined): string { + const span = formatAgeSpan(ageRange); + return /^\d+–\d+$/.test(span) ? `Ages ${span}` : span; } // ============================================================================ -- 2.54.0
AgesReligious characterNurseryConstituency
{formatAgeSpan(s.age_range) || NO_VALUE}{faithOf(s)} + {/* Undefined is a mart the pipeline has not rebuilt, and + false is a school without one. Neither is a "Yes", and + neither is worth two different words. */} + {s.nursery_provision ? 'Yes' : NO_VALUE} + + {s.parliamentary_constituency || NO_VALUE} +