From cbe3a9a772efbb17a0e7061412c6ee06a0ac2251 Mon Sep 17 00:00:00 2001 From: Tudor Date: Sun, 30 Aug 2026 21:48:39 +0100 Subject: [PATCH] fix(destinations): the table said 'withheld' for a category that just doesn't apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Share column keyed off `percentage === null`, which is true for not_applicable as well as suppressed, so a destination that does not apply to the school was labelled as one DfE withheld — while the Pupils column in the same row rendered blank. Two columns, one row, disagreeing about what the row was, and one of them making a claim about DfE that wasn't true. Both columns now derive from `status`, which is the distinction the mart, the SQLAlchemy model and the serialiser all preserve deliberately: published shows the figure, suppressed shows the withheld badge, not_applicable shows an em-dash with a title saying so. A published count with no published percentage now derives its share from the cohort rather than falling through to a marker — both halves are published, so nothing withheld is involved, and it is the same derivation the bar widths already use. Verified the new tests fail against the old logic before keeping them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BvdDKvFFSZuMVDH5fEyTob --- .../components/DestinationsSection.test.tsx | 58 +++++++++++++++++++ .../components/school/DestinationsView.tsx | 52 +++++++++++++---- .../components/school/destinations.module.css | 8 +++ 3 files changed, 107 insertions(+), 11 deletions(-) diff --git a/nextjs-app/__tests__/components/DestinationsSection.test.tsx b/nextjs-app/__tests__/components/DestinationsSection.test.tsx index 11002e7..d744423 100644 --- a/nextjs-app/__tests__/components/DestinationsSection.test.tsx +++ b/nextjs-app/__tests__/components/DestinationsSection.test.tsx @@ -89,3 +89,61 @@ describe('DestinationsSection', () => { expect(container.firstChild).toBeNull(); }); }); + +describe('the detail table keeps the three statuses apart', () => { + // 'suppressed' and 'not_applicable' are different claims, and the mart, the + // SQLAlchemy model and the serialiser all preserve the difference. The table + // used to key its Share column off `percentage === null`, which is true for + // both, so a category that simply does not apply was labelled "withheld" — + // while the Pupils column beside it rendered blank. + const mixedPhase: DestinationPhase = { + cohort_year: '2022/23', + groups: { + all: { + cohort: 180, + categories: [ + cell('school_sixth_form', 75), + cell('sixth_form_college', null, 'suppressed'), + cell('further_education', null, 'suppressed'), + cell('apprenticeship', null, 'not_applicable'), + ], + }, + }, + }; + + const rowFor = (container: HTMLElement, category: string) => + Array.from(container.querySelectorAll('tbody tr')) + .find(tr => tr.textContent?.includes(category)); + + it('never labels a not-applicable category as withheld', () => { + const { container } = render(); + const row = rowFor(container, 'Apprenticeship'); + expect(row).toBeTruthy(); + expect(row!.textContent).not.toMatch(/withheld/i); + }); + + it('labels a genuinely suppressed category as withheld in both columns', () => { + const { container } = render(); + const row = rowFor(container, 'Sixth-form college'); + expect(row).toBeTruthy(); + expect(row!.querySelectorAll('td')).toHaveLength(2); + Array.from(row!.querySelectorAll('td')).forEach(td => + expect(td.textContent).toMatch(/withheld/i)); + }); + + it('the two columns of a row never disagree about what the row is', () => { + const { container } = render(); + Array.from(container.querySelectorAll('tbody tr')).forEach(tr => { + const cells = Array.from(tr.querySelectorAll('td')) + .map(td => /withheld/i.test(td.textContent ?? '')); + expect(new Set(cells).size).toBe(1); + }); + }); + + it('shows a published category its real figures', () => { + const { container } = render(); + const row = rowFor(container, 'State-funded school sixth form'); + expect(row!.textContent).toMatch(/75/); + expect(row!.textContent).toMatch(/42%/); + }); +}); diff --git a/nextjs-app/components/school/DestinationsView.tsx b/nextjs-app/components/school/DestinationsView.tsx index 2dae294..f538d57 100644 --- a/nextjs-app/components/school/DestinationsView.tsx +++ b/nextjs-app/components/school/DestinationsView.tsx @@ -47,6 +47,44 @@ function cellsFor(group: DestinationGroup, card: CardGroup): DestinationCell[] { return group.cells.filter(c => wanted.has(c.category)); } +/** + * One cell of the detail table. + * + * The three statuses are three different statements and the table has to keep + * them apart, because the whole pipeline does — the mart, the SQLAlchemy model + * and the serialiser all preserve the difference deliberately: + * + * published the figure + * suppressed DfE withheld it to protect a small number of pupils + * not_applicable this destination does not apply to this school at all + * + * An earlier version keyed the share column off `percentage === null`, which is + * also true for not_applicable, so a category that simply does not apply was + * labelled "withheld" — while the pupils column beside it rendered blank. Both + * columns now derive from `status`, so they cannot disagree. + */ +function cellValue( + cell: DestinationCell, cohort: number, kind: 'pupils' | 'share', +) { + if (cell.status === 'suppressed') { + return withheld; + } + const notApplicable = ( + + — + + ); + + if (cell.status !== 'published' || cell.pupils === null) return notApplicable; + if (kind === 'pupils') return cell.pupils; + + // Percentages come from the mart, but a published count with no published + // percentage is recoverable from the cohort — both halves are published, so + // nothing withheld is involved. Same derivation the bar widths use. + const share = cell.percentage ?? (cohort > 0 ? (cell.pupils / cohort) * 100 : null); + return share === null ? notApplicable : `${Math.round(share)}%`; +} + export function DestinationsView({ destinations, phase, }: { destinations: DestinationPhase; phase: 'ks4' | 'ks5' }) { @@ -193,27 +231,19 @@ export function DestinationsView({ const cell = group.cells.find(c => c.category === category); if (!cell) return []; const card = cardGroupFor(category); - const isWithheld = cell.status === 'suppressed'; return [( {CATEGORY_LABELS[category]} - - {isWithheld - ? withheld - : cell.pupils} - - - {isWithheld || cell.percentage === null - ? withheld - : `${Math.round(cell.percentage)}%`} - + {cellValue(cell, group.cohort, 'pupils')} + {cellValue(cell, group.cohort, 'share')} )]; })} diff --git a/nextjs-app/components/school/destinations.module.css b/nextjs-app/components/school/destinations.module.css index 0985196..28896f3 100644 --- a/nextjs-app/components/school/destinations.module.css +++ b/nextjs-app/components/school/destinations.module.css @@ -297,3 +297,11 @@ font-size: var(--step--2); color: var(--text-muted); } + +/* A destination that does not apply to this school. Deliberately not the + withheld badge: "we are not told" and "there is nothing to tell" are + different statements, and the rest of the pipeline keeps them apart. */ +.notApplicable { + color: var(--text-muted); + cursor: help; +}