Merge pull request 'fix(admissions): flag-off pages must not speak for the council' (#135) from fix/distance-flag-off-absence-copy into main

Reviewed-on: #135

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.

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(<RouteTrail />);
+    expect(recordVisitedPath).toHaveBeenCalledWith('/schools/brentwood');
+  });
+
+  it('records each new route as the user moves through the app', () => {
+    const { rerender } = render(<RouteTrail />);
+    pathname = '/school/115429-brentwood-school';
+    rerender(<RouteTrail />);
+    expect(recordVisitedPath).toHaveBeenLastCalledWith(
+      '/school/115429-brentwood-school');
+  });
+
+  it('renders nothing, so it can sit anywhere in the layout', () => {
+    const { container } = render(<RouteTrail />);
+    expect(container).toBeEmptyDOMElement();
+  });
+});

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 ───────────────────────────────────────────────

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');
+  });
+});

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({
         />
       </head>
       <body>
+        {/* 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. */}
+        <RouteTrail />
         <ComparisonProvider>
           <a href="#main-content" className="skip-link">Skip to main content</a>
           <Navigation />

nextjs-app/app/school/[slug]/page.tsx

@@ -232,7 +232,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}

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

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

nextjs-app/components/school/SecondaryAdmissionsSection.tsx

@@ -21,11 +21,17 @@ 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;
   hasSixthForm: boolean;
 }) {
   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() ?? '';
@@ -102,7 +108,7 @@ export function SecondaryAdmissionsSection({
           {CUTOFF_NOTE} {CUTOFF_MEASUREMENT_NOTE}
           {cutoff.routeNote && <> {cutoff.routeNote}</>}
         </p>
-      ) : (
+      ) : featureOn ? (
         <p className={styles.sectionSubtitle} style={{ marginTop: '1rem' }}>
           {describeCutoffAbsence({
             localAuthority: schoolInfo.local_authority,
@@ -110,7 +116,7 @@ export function SecondaryAdmissionsSection({
             admissionsHistory,
           })}
         </p>
-      )}
+      ) : null}
 
       {hasSixthForm && (
         <div className={styles.sixthFormNote}>

nextjs-app/components/school/SecondarySchoolSections.tsx

@@ -36,7 +36,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;

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;
+    return classifyPath(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';
   } catch {
     return 'direct';
   }