diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index e953520..d7065fa 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -1304,6 +1304,52 @@ test('with the distance feature off, the section is absent rather than empty', a .toHaveCount(0); }); +/** + * A secondary school carrying an EES admissions row, which is what makes its + * Admissions section render while the distance feature is dark. + */ +async function secondarySchoolWithAdmissions(page: Page) { + const list = await page.request.get('/api/schools?phase=secondary&page_size=40'); + if (!list.ok()) return null; + const body = await list.json(); + for (const s of (body?.schools ?? []).slice(0, 25)) { + const res = await page.request.get(`/api/schools/${s.urn}`); + if (!res.ok()) continue; + const detail = await res.json(); + if (detail?.admissions == null) continue; + return { urn: s.urn as number }; + } + return null; +} + +test('with the distance feature off, a secondary page makes no claim about publication', async ({ page }) => { + /* + * Shipping dark must not put words in the council's mouth. The secondary + * template is the only one that words the absence, and "X has not published + * a cut-off distance for this school" is false wherever X does publish and + * we are simply withholding it. + * + * This is why the API omits the key rather than sending null: absent means + * "cut-offs are not published at all", null means "this school has none". + * Only the second is a fact about the school, and only the second is sayable. + */ + test.skip(await distanceFeatureIsOn(page), + 'the admission_distance flag is on in this environment'); + + const found = await secondarySchoolWithAdmissions(page); + test.skip(found === null, 'no secondary school in the sample has an admissions row'); + + await page.goto(`/school/${found!.urn}`); + await expect(page.locator('h1').first()).toBeVisible({ timeout: 15_000 }); + + // The Admissions section is still there — this is not a test that the whole + // section vanished, which would pass for the wrong reason. + await expect(page.locator('#admissions')).toHaveCount(1); + + await expect(page.getByText(/has not published a cut-off distance/)).toHaveCount(0); + await expect(page.getByText(/Contact the admissions authority/)).toHaveCount(0); +}); + test('/api/flags is not reachable from the public internet', async ({ page }) => { // It names every unreleased feature and whether it is on. Next reads it // server-side over the Docker network; the public proxy must deny it. diff --git a/nextjs-app/__tests__/components/RouteTrail.test.tsx b/nextjs-app/__tests__/components/RouteTrail.test.tsx new file mode 100644 index 0000000..cc95c9e --- /dev/null +++ b/nextjs-app/__tests__/components/RouteTrail.test.tsx @@ -0,0 +1,38 @@ +/** + * The trail has to be written by something, and it has to be written on every + * route — not only the ones that happen to track an event. + */ +import { render } from '@testing-library/react'; + +const recordVisitedPath = jest.fn(); +let pathname = '/schools/brentwood'; + +jest.mock('next/navigation', () => ({ usePathname: () => pathname })); +jest.mock('@/lib/analytics', () => ({ + recordVisitedPath: (p: string) => recordVisitedPath(p), +})); + +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { RouteTrail } = require('@/components/RouteTrail'); + +describe('RouteTrail', () => { + beforeEach(() => recordVisitedPath.mockClear()); + + it('records the page it is mounted on', () => { + render(); + expect(recordVisitedPath).toHaveBeenCalledWith('/schools/brentwood'); + }); + + it('records each new route as the user moves through the app', () => { + const { rerender } = render(); + pathname = '/school/115429-brentwood-school'; + rerender(); + expect(recordVisitedPath).toHaveBeenLastCalledWith( + '/school/115429-brentwood-school'); + }); + + it('renders nothing, so it can sit anywhere in the layout', () => { + const { container } = render(); + expect(container).toBeEmptyDOMElement(); + }); +}); diff --git a/nextjs-app/__tests__/components/darkThemeSafety.test.ts b/nextjs-app/__tests__/components/darkThemeSafety.test.ts index 3d6d889..b33ba70 100644 --- a/nextjs-app/__tests__/components/darkThemeSafety.test.ts +++ b/nextjs-app/__tests__/components/darkThemeSafety.test.ts @@ -32,10 +32,17 @@ function stylesheets(dir: string): string[] { } /** Innermost `selector { body }` pairs. Nested at-rules never match as rules, - * because their body contains braces. */ + * because their body contains braces. + * + * Comments are stripped before matching rather than after, so that the whole + * selector survives. Taking only its last line — which is what stripping a + * leading comment used to require — silently discarded every selector in a + * grouped rule but the final one, and a safety guard that cannot see half its + * input fails open. */ function rules(css: string): Array<{ selector: string; body: string }> { - return Array.from(css.matchAll(/([^{}]+)\{([^{}]*)\}/g), (m) => ({ - selector: m[1].trim().split('\n').pop()!.trim(), + const bare = css.replace(/\/\*[\s\S]*?\*\//g, ''); + return Array.from(bare.matchAll(/([^{}]+)\{([^{}]*)\}/g), (m) => ({ + selector: m[1].trim().replace(/\s*\n\s*/g, ' '), body: m[2], })); } @@ -45,6 +52,15 @@ const THEMED_COLOR = /(?:^|[^-])color:\s*var\(--/; const files = stylesheets(COMPONENTS); +/** Component sources, for the third-party-surface rule below. */ +function sources(dir: string): string[] { + return fs.readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) return sources(full); + return entry.name.endsWith('.tsx') ? [full] : []; + }); +} + describe('dark-theme safety', () => { it('finds stylesheets to check', () => { expect(files.length).toBeGreaterThan(0); @@ -77,6 +93,76 @@ describe('dark-theme safety', () => { }); }); +/** + * The same defect one stylesheet further out. + * + * The rules above scan our own CSS modules. They cannot see a surface painted + * by a third-party sheet: leaflet.css hardcodes `background: white` on + * `.leaflet-popup-content-wrapper` and `.leaflet-popup-tip`, and + * LeafletMapInner builds its popup as an HTML string with inline + * `color: var(--text-primary)`. Neither half lives in a .module.css, so the + * module scan passed while dark mode rendered #E9EEF0 on #FFFFFF — 1.17:1, + * with the school name and the headline figure effectively invisible. + * + * globals.css already pulls the rest of Leaflet's chrome onto the tokens (the + * attribution bar, the zoom controls) for exactly this reason. The popup was + * simply missed. + */ +describe('third-party surfaces under themed text', () => { + const GLOBALS = path.join(__dirname, '..', '..', 'app', 'globals.css'); + + /** Leaflet surfaces our own code writes token-coloured text onto. */ + const LEAFLET_POPUP_SURFACES = [ + '.leaflet-popup-content-wrapper', + '.leaflet-popup-tip', + ]; + + it('still finds a component painting themed text into a Leaflet popup', () => { + // Guards the rule below against passing vacuously if the popups are ever + // rewritten as React components rather than HTML strings. + const themed = sources(COMPONENTS).filter((file) => { + const src = fs.readFileSync(file, 'utf8'); + return /bindPopup\(/.test(src) && /color:var\(--|color: var\(--/.test(src); + }); + + expect(themed.length).toBeGreaterThan(0); + }); + + it('themes the Leaflet popup surface, because the text on it is themed', () => { + const globals = rules(fs.readFileSync(GLOBALS, 'utf8')); + + const unthemed = LEAFLET_POPUP_SURFACES.filter((surface) => { + const rule = globals.find((r) => r.selector.includes(surface)); + return !rule || !/background[^;]*var\(--/.test(rule.body); + }); + + // Leaflet's white is not a colour this site owns. Either the surface + // follows the theme or the text on it must be literal — and the text is + // already themed. + expect(unthemed).toEqual([]); + }); + + it('never puts a literal white label on a themed fill', () => { + /* + * The mirror image of the module-CSS rule above, and the half of the popup + * that theming the card does not reach. "View Details" is + * `background:var(--status-above);color:white`; --status-above is #36743F + * in light but #7FCB8A in dark, so the label went from 5.63:1 to 1.94:1. + * + * --text-inverse is the token for ink on a saturated fill — #FFFFFF in + * light, #111A20 in dark — and the popup's Ofsted badge already uses it. + */ + const offenders = sources(COMPONENTS).flatMap((file) => { + const src = fs.readFileSync(file, 'utf8'); + return Array.from( + src.matchAll(/background:\s*var\(--[^;"']*;[^"']*?color:\s*(white|#fff\b|#ffffff\b)/gi), + () => path.relative(COMPONENTS, file)); + }); + + expect(offenders).toEqual([]); + }); +}); + /** * Destination measures add the first new colour family since the palette was * set. The tokens have to exist in both blocks or the section renders one diff --git a/nextjs-app/__tests__/components/lastDistanceOffered.test.tsx b/nextjs-app/__tests__/components/lastDistanceOffered.test.tsx index 9f16df5..3f9efa1 100644 --- a/nextjs-app/__tests__/components/lastDistanceOffered.test.tsx +++ b/nextjs-app/__tests__/components/lastDistanceOffered.test.tsx @@ -98,6 +98,17 @@ describe('secondary detail page', () => { expect(screen.getByText(/has not published a cut-off distance/)).toBeInTheDocument(); }); + + it('makes no claim about publication when the feature is switched off', () => { + // Absent, not null. The API omits the key entirely while the + // admission_distance flag is off, and "Islington has not published a + // cut-off distance" is then a statement about us, not about Islington — + // false wherever the authority does publish one. + renderSecondarySchoolDetail({ ...secondaryFixture, admissionDistance: undefined }); + + expect(screen.queryByText(/has not published a cut-off distance/)).not.toBeInTheDocument(); + expect(screen.queryByText(/Contact the admissions authority/)).not.toBeInTheDocument(); + }); }); // ── The Distance section ─────────────────────────────────────────────── diff --git a/nextjs-app/__tests__/lib/analytics.test.ts b/nextjs-app/__tests__/lib/analytics.test.ts index 22af2d7..96dd0fb 100644 --- a/nextjs-app/__tests__/lib/analytics.test.ts +++ b/nextjs-app/__tests__/lib/analytics.test.ts @@ -62,3 +62,97 @@ describe('getNavigationSource', () => { expect(getNavigationSource()).toBe('direct'); }); }); + +/* + * The defect the existing suite could not see. + * + * Every test above sets document.referrer, which the browser writes only when + * a *document* loads. Every internal navigation in this app is an App Router + * soft navigation — history.pushState, no new document — so document.referrer + * keeps naming whatever opened the tab for the whole session. Verified on + * staging: /schools/brentwood → click a school → URL changes to /school/… + * and document.referrer is still "". + * + * So `from` reported 'direct' for essentially every in-app journey, and the + * suite passed because it only ever exercised the full-page-load path. + */ +function freshAnalytics() { + let mod!: typeof import('@/lib/analytics'); + jest.isolateModules(() => { + mod = require('@/lib/analytics'); + }); + return mod; +} + +function at(path: string) { + window.history.pushState({}, '', path); +} + +describe('getNavigationSource across a soft navigation', () => { + afterEach(() => { + referrer(''); + at('/'); + }); + + it('attributes a school view to the place page the user actually came from', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + at('/schools/brentwood'); + recordVisitedPath('/schools/brentwood'); + + at('/school/115429-brentwood-school'); + recordVisitedPath('/school/115429-brentwood-school'); + + expect(source()).toBe('place'); + }); + + it('does not depend on whether the new path was recorded first', () => { + // The trail is written by a layout-level effect and read by a page-level + // one. React orders those by tree position, which is not a contract worth + // resting a measurement on, so the answer must be the same either way. + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + recordVisitedPath('/rankings'); + at('/school/115429-brentwood-school'); + + expect(source()).toBe('rankings'); + }); + + it('names the previous page, not the current one, when both are schools', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + at('/school/100010-brecknock-primary-school'); + recordVisitedPath('/school/100010-brecknock-primary-school'); + + at('/school/115429-brentwood-school'); + recordVisitedPath('/school/115429-brentwood-school'); + + expect(source()).toBe('detail'); + }); + + it('looks past a return visit to the page the user came back from', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + for (const p of ['/schools/brentwood', '/school/115429-brentwood-school', + '/schools/brentwood']) { + at(p); + recordVisitedPath(p); + } + expect(source()).toBe('detail'); + }); + + it('falls back to the referrer on a real document load, where it is true', () => { + // A fresh module is a fresh document: nothing has been recorded, and + // document.referrer is meaningful again. + const { getNavigationSource: source } = freshAnalytics(); + at('/school/115429-brentwood-school'); + referrer(`${ORIGIN}/schools/barnet`); + + expect(source()).toBe('place'); + }); + + it('still reads an arrival from outside as direct', () => { + const { recordVisitedPath, getNavigationSource: source } = freshAnalytics(); + at('/schools/brentwood'); + recordVisitedPath('/schools/brentwood'); + referrer('https://www.google.com/search?q=schools+in+brentwood'); + + expect(source()).toBe('direct'); + }); +}); diff --git a/nextjs-app/app/globals.css b/nextjs-app/app/globals.css index 18ad53c..327e68c 100644 --- a/nextjs-app/app/globals.css +++ b/nextjs-app/app/globals.css @@ -616,6 +616,35 @@ html .leaflet-bar a:hover { color: var(--text-primary); } +/* + * The popup, which leaflet.css paints `background: white; color: #333` on both + * the card and its tip. The content LeafletMapInner binds into it is themed — + * the school name and the headline figure are `var(--text-primary)` — so in + * dark mode that was #E9EEF0 on #FFFFFF, a contrast ratio of 1.17:1. The name + * and the number were the two least readable things on the page. + * + * Moving the surface onto --bg-card fixes every foreground at once rather than + * one at a time: the muted phase line goes 2.90:1 -> 5.45:1, the vs-national + * delta 1.94:1 -> 8.14:1, the Ofsted badge 1.74:1 -> 9.11:1. In light mode + * --bg-card is #FFFFFF, so the popup looks as it always did. + */ +html .leaflet-popup-content-wrapper, +html .leaflet-popup-tip { + background: var(--bg-card); + color: var(--text-primary); +} + +/* Leaflet's own selector is `.leaflet-container a.leaflet-popup-close-button` + at 0,2,1 — an `html` prefix alone would lose to it. */ +html .leaflet-container a.leaflet-popup-close-button { + color: var(--text-muted); +} + +html .leaflet-container a.leaflet-popup-close-button:hover, +html .leaflet-container a.leaflet-popup-close-button:focus { + color: var(--text-primary); +} + /* Main content column */ .main { max-width: 1400px; diff --git a/nextjs-app/app/layout.tsx b/nextjs-app/app/layout.tsx index 2f888b9..96a2be4 100644 --- a/nextjs-app/app/layout.tsx +++ b/nextjs-app/app/layout.tsx @@ -4,6 +4,7 @@ import Script from 'next/script'; import { Navigation } from '@/components/Navigation'; import { Footer } from '@/components/Footer'; import { ComparisonToast } from '@/components/ComparisonToast'; +import { RouteTrail } from '@/components/RouteTrail'; import { ComparisonProvider } from '@/context/ComparisonProvider'; import { SITE_URL } from '@/lib/site'; import './globals.css'; @@ -114,6 +115,10 @@ export default function RootLayout({ /> + {/* Records every route so funnel attribution has a previous page to + name. document.referrer cannot: a soft navigation creates no + document, so the browser never updates it. */} + Skip to main content diff --git a/nextjs-app/app/school/[slug]/page.tsx b/nextjs-app/app/school/[slug]/page.tsx index 6320aa4..be0674f 100644 --- a/nextjs-app/app/school/[slug]/page.tsx +++ b/nextjs-app/app/school/[slug]/page.tsx @@ -233,7 +233,7 @@ export default async function SchoolPage({ params }: SchoolPageProps) { census={census ?? null} admissions={admissions ?? null} admissionsHistory={admissions_history ?? []} - admissionDistance={admission_distance ?? null} + admissionDistance={admission_distance} deprivation={deprivation ?? null} finance={finance ?? null} nationalAvg={nationalAvg} diff --git a/nextjs-app/components/LeafletMapInner.tsx b/nextjs-app/components/LeafletMapInner.tsx index 5bfe7db..9132b43 100644 --- a/nextjs-app/components/LeafletMapInner.tsx +++ b/nextjs-app/components/LeafletMapInner.tsx @@ -184,7 +184,7 @@ export default function LeafletMapInner({ schools, center, zoom, referencePoint, ${phaseLabel}${school.local_authority ? ` · ${escapeHtml(school.local_authority)}` : ''}${distanceStr} ${metricHtml} - View Details → + View Details → `; marker.bindPopup(popupContent); diff --git a/nextjs-app/components/RouteTrail.tsx b/nextjs-app/components/RouteTrail.tsx new file mode 100644 index 0000000..4531693 --- /dev/null +++ b/nextjs-app/components/RouteTrail.tsx @@ -0,0 +1,27 @@ +/** + * Writes the in-app navigation trail that funnel attribution reads. + * + * Renders nothing. It exists because document.referrer cannot answer "which + * page did they come from" in an App Router app: a soft navigation creates no + * document, so the browser never updates it. See the trail comment in + * lib/analytics.ts. + * + * Mounted once in the root layout, so every route is recorded — including the + * ones that fire no event of their own, which are still somebody else's + * previous page. + */ +'use client'; + +import { useEffect } from 'react'; +import { usePathname } from 'next/navigation'; +import { recordVisitedPath } from '@/lib/analytics'; + +export function RouteTrail() { + const pathname = usePathname(); + + useEffect(() => { + recordVisitedPath(pathname); + }, [pathname]); + + return null; +} diff --git a/nextjs-app/components/school/DistanceSection.tsx b/nextjs-app/components/school/DistanceSection.tsx index 8990960..45651b5 100644 --- a/nextjs-app/components/school/DistanceSection.tsx +++ b/nextjs-app/components/school/DistanceSection.tsx @@ -24,7 +24,7 @@ export function DistanceSection({ admissionDistance, schoolInfo, }: { - admissionDistance: SchoolAdmissionDistance | null; + admissionDistance: SchoolAdmissionDistance | null | undefined; schoolInfo: School; }) { // Without a figure there is nothing to compare against, and without diff --git a/nextjs-app/components/school/SecondaryAdmissionsSection.tsx b/nextjs-app/components/school/SecondaryAdmissionsSection.tsx index 73a8e36..b787a03 100644 --- a/nextjs-app/components/school/SecondaryAdmissionsSection.tsx +++ b/nextjs-app/components/school/SecondaryAdmissionsSection.tsx @@ -21,10 +21,16 @@ export function SecondaryAdmissionsSection({ published cut-off and no EES admissions row. */ admissions: SchoolAdmissions | null; admissionsHistory: SchoolAdmissions[]; - admissionDistance: SchoolAdmissionDistance | null; + admissionDistance: SchoolAdmissionDistance | null | undefined; schoolInfo: School; }) { const cutoff = describeCutoff(admissionDistance); + /* Absent means cut-offs are not being published at all; null means this + school has no published cut-off. Only the second is a fact about the + school, and only the second can be stated. Saying "X has not published a + cut-off" while the feature is dark describes us, and is false wherever the + authority does publish one. */ + const featureOn = admissionDistance !== undefined; // Moved with this section from SecondarySchoolDetailView, its only consumer. const admissionsTag = (() => { const policy = schoolInfo.admissions_policy?.toLowerCase() ?? ''; @@ -101,7 +107,7 @@ export function SecondaryAdmissionsSection({ {CUTOFF_NOTE} {CUTOFF_MEASUREMENT_NOTE} {cutoff.routeNote && <> {cutoff.routeNote}}

- ) : ( + ) : featureOn ? (

{describeCutoffAbsence({ localAuthority: schoolInfo.local_authority, @@ -109,7 +115,7 @@ export function SecondaryAdmissionsSection({ admissionsHistory, })}

- )} + ) : null} ); diff --git a/nextjs-app/components/school/SecondarySchoolSections.tsx b/nextjs-app/components/school/SecondarySchoolSections.tsx index f3542ef..68f8ba9 100644 --- a/nextjs-app/components/school/SecondarySchoolSections.tsx +++ b/nextjs-app/components/school/SecondarySchoolSections.tsx @@ -39,7 +39,10 @@ export interface SecondarySchoolSectionsProps { /** Needed to tell a year with no published cut-off apart from a year the * school simply was not oversubscribed. */ admissionsHistory: SchoolAdmissions[]; - admissionDistance: SchoolAdmissionDistance | null; + /** Absent — not null — while the admission_distance flag is off. The two + * mean different things to the reader and must stay distinguishable: + * see SecondaryAdmissionsSection, which words the absence. */ + admissionDistance: SchoolAdmissionDistance | null | undefined; deprivation: SchoolDeprivation | null; finance: SchoolFinance | null; nationalAvg: NationalAverages | null; diff --git a/nextjs-app/lib/analytics.ts b/nextjs-app/lib/analytics.ts index bd05ba1..591ed7d 100644 --- a/nextjs-app/lib/analytics.ts +++ b/nextjs-app/lib/analytics.ts @@ -60,22 +60,77 @@ export function track(name: EventName, data?: Payload): void { export type NavigationSource = 'search' | 'rankings' | 'compare' | 'detail' | 'place' | 'direct'; +/* + * The in-app trail. + * + * document.referrer is written by the browser only when a *document* loads. + * Every internal navigation here is an App Router soft navigation — + * history.pushState, no new document — so document.referrer goes on naming + * whatever opened the tab (usually nothing, or a search engine) for the whole + * session. Reading it to answer "which page did they come from" therefore + * returned 'direct' for essentially every in-app journey, including the one + * the location layer exists to produce. + * + * Verified on staging: /schools/brentwood, click a school, the URL becomes + * /school/… and document.referrer is still "". + * + * A module-level trail is the counterpart with exactly the right lifetime. It + * survives soft navigation, and it dies on a real document load — which is + * precisely when document.referrer becomes meaningful again, so the two cover + * each other with no overlap. + */ +const TRAIL_LIMIT = 4; +const trail: string[] = []; + +/** Record a path the user is now on. Called by RouteTrail on every route. */ +export function recordVisitedPath(path: string): void { + if (trail[trail.length - 1] === path) return; + trail.push(path); + if (trail.length > TRAIL_LIMIT) trail.shift(); +} + +/** + * The most recent path that is not the one being viewed. + * + * Skipping the current path rather than taking trail[length - 2] is what + * makes the answer independent of ordering: the trail is written by a + * layout-level effect and read by a page-level one, and React orders those by + * tree position — not a contract worth resting a measurement on. It also + * gives the right answer when the user goes back to a page they came from. + */ +function previousInAppPath(): string | null { + if (typeof window === 'undefined') return null; + const current = window.location.pathname; + for (let i = trail.length - 1; i >= 0; i -= 1) { + if (trail[i] !== current) return trail[i]; + } + return null; +} + +function classifyPath(p: string): NavigationSource { + if (p === '/' || p === '') return 'search'; + if (p.startsWith('/rankings')) return 'rankings'; + if (p.startsWith('/compare')) return 'compare'; + // `/schools/` before `/school/`: they differ by one letter and mean + // different things — the location layer versus a single school. Checked + // first so the narrower-looking prefix cannot shadow it if either string + // is ever edited. + if (p.startsWith('/schools/')) return 'place'; + if (p.startsWith('/school/')) return 'detail'; + return 'direct'; +} + export function getNavigationSource(): NavigationSource { + const internal = previousInAppPath(); + if (internal) return classifyPath(internal); + + // No trail means this is the first page of the document, so the referrer is + // the only witness — and an honest one. if (typeof window === 'undefined' || !document.referrer) return 'direct'; try { const ref = new URL(document.referrer); if (ref.origin !== window.location.origin) return 'direct'; - const p = ref.pathname; - if (p === '/' || p === '') return 'search'; - if (p.startsWith('/rankings')) return 'rankings'; - if (p.startsWith('/compare')) return 'compare'; - // `/schools/` before `/school/`: they differ by one letter and mean - // different things — the location layer versus a single school. Checked - // first so the narrower-looking prefix cannot shadow it if either string - // is ever edited. - if (p.startsWith('/schools/')) return 'place'; - if (p.startsWith('/school/')) return 'detail'; - return 'direct'; + return classifyPath(ref.pathname); } catch { return 'direct'; }