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; } // ============================================================================
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} +