fix(destinations): the table said 'withheld' for a category that just doesn't apply
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 8s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 45s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 1m14s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 9s
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 8s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 45s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 1m14s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 9s
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BvdDKvFFSZuMVDH5fEyTob
This commit is contained in:
1 parent
2e9b5c83c5
commit
cbe3a9a772
3 files changed
+107
-11
No files matched your search
@@ -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(<DestinationsSection destinations={mixedPhase} />);
|
||||
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(<DestinationsSection destinations={mixedPhase} />);
|
||||
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(<DestinationsSection destinations={mixedPhase} />);
|
||||
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(<DestinationsSection destinations={mixedPhase} />);
|
||||
const row = rowFor(container, 'State-funded school sixth form');
|
||||
expect(row!.textContent).toMatch(/75/);
|
||||
expect(row!.textContent).toMatch(/42%/);
|
||||
});
|
||||
});
|
||||
@@ -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 <span className={styles.withheldMark}>withheld</span>;
|
||||
}
|
||||
const notApplicable = (
|
||||
<span className={styles.notApplicable} title="Does not apply to this school">
|
||||
—
|
||||
</span>
|
||||
);
|
||||
|
||||
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 [(
|
||||
<tr
|
||||
key={category}
|
||||
data-group={card ?? 'none'}
|
||||
data-status={cell.status}
|
||||
className={dimmed(card) ? styles.dim : ''}
|
||||
>
|
||||
<th scope="row" className={styles.rowName}>
|
||||
<span className={`${styles.swatch} ${styles[category]}`} />
|
||||
{CATEGORY_LABELS[category]}
|
||||
</th>
|
||||
<td>
|
||||
{isWithheld
|
||||
? <span className={styles.withheldMark}>withheld</span>
|
||||
: cell.pupils}
|
||||
</td>
|
||||
<td>
|
||||
{isWithheld || cell.percentage === null
|
||||
? <span className={styles.withheldMark}>withheld</span>
|
||||
: `${Math.round(cell.percentage)}%`}
|
||||
</td>
|
||||
<td>{cellValue(cell, group.cohort, 'pupils')}</td>
|
||||
<td>{cellValue(cell, group.cohort, 'share')}</td>
|
||||
</tr>
|
||||
)];
|
||||
})}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
Reference in new issue
Block a user