Merge pull request 'fix(places): a locality collision must not break the sitemap' (#115) from fix/locality-collision-skip into main

Reviewed-on: #115

backend/app.py

@@ -209,27 +209,12 @@ def _place_url(place) -> str:
 
 
 def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]:
-    """A <url> per place, plus a phase variant wherever that phase clears the
-    threshold on its own.
-
-    Phase is part of the query — "primary schools in beccles" — so each
-    variant is its own indexable page. Submitting only the bare place URL left
-    ~950 of them reachable by nothing: absent from every sitemap, and not
-    linked from the place page either.
-    """
-    rows: list[str] = []
-    for p in sorted(get_place_registry().values(), key=lambda p: (p.kind, p.slug)):
-        if p.kind not in kinds:
-            continue
-        rows.append(_url_element(BASE_URL + _place_url(p)))
-        # Outcodes carry no phase variants: nobody searches "primary schools
-        # in SW11", so the routes do not exist to submit.
-        if p.kind == "outcode":
-            continue
-        for phase in ("primary", "secondary"):
-            if p.publishes_phase(phase):
-                rows.append(_url_element(f"{BASE_URL}{_place_url(p)}/{phase}"))
-    return rows
+    return [
+        _url_element(BASE_URL + _place_url(p))
+        for p in sorted(get_place_registry().values(),
+                        key=lambda p: (p.kind, p.slug))
+        if p.kind in kinds
+    ]
 
 
 def build_sitemaps() -> dict[str, str]:
@@ -1231,11 +1216,7 @@ async def get_place(request: Request, kind: str, slug: str,
     return {
         "place": {"kind": place.kind, "slug": place.slug, "name": place.name,
                   "count": len(place.urns),
-                  "parent_authority": place.parent_authority,
-                  # Only phases that clear the threshold, so the page links
-                  # variants that exist rather than 404s.
-                  "phases": [ph for ph in ("primary", "secondary")
-                             if place.publishes_phase(ph)]},
+                  "parent_authority": place.parent_authority},
         "schools": clean_for_json(rows[cols]),
         "averages": averages,
     }

backend/places.py

@@ -14,7 +14,7 @@ from __future__ import annotations
 
 import logging
 import re
-from dataclasses import dataclass, field
+from dataclasses import dataclass
 
 logger = logging.getLogger(__name__)
 
@@ -30,13 +30,6 @@ class Place:
     name: str
     urns: tuple[int, ...]
     parent_authority: str | None    # authority NAME, for the 301 target
-    # URNs per phase, so the per-phase threshold can be applied without
-    # re-querying. A place with 30 primaries and 2 secondaries publishes a
-    # primary variant and no secondary one.
-    phase_urns: dict[str, tuple[int, ...]] = field(default_factory=dict)
-
-    def publishes_phase(self, phase: str) -> bool:
-        return len(self.phase_urns.get(phase, ())) >= MIN_SCHOOLS
 
     @property
     def key(self) -> str:
@@ -53,24 +46,6 @@ def _publishable_urns(df) -> set[int]:
     return set(df.loc[df[cols].notna().any(axis=1), "urn"].astype(int))
 
 
-def _phase_urns(group, publishable: set[int]) -> dict[str, tuple[int, ...]]:
-    """URNs per phase. All-through schools count toward both, matching the
-    PHASE_GROUPS mapping the search filters already use."""
-    from backend.app import PHASE_GROUPS
-
-    if "phase" not in group.columns:
-        return {}
-    lowered = group["phase"].fillna("").str.lower()
-    out: dict[str, tuple[int, ...]] = {}
-    for phase in ("primary", "secondary"):
-        wanted = PHASE_GROUPS.get(phase, set())
-        subset = group[lowered.isin(wanted)]
-        urns = tuple(sorted({int(u) for u in subset["urn"]} & publishable))
-        if urns:
-            out[phase] = urns
-    return out
-
-
 def _parent_authority(group) -> str | None:
     """The most common authority in a group — the useful 301 target.
 
@@ -104,7 +79,6 @@ def _group(df, column: str, kind: str, publishable: set[int]) -> dict[str, Place
         place = Place(
             kind=kind, slug=slug, name=name, urns=urns,
             parent_authority=_parent_authority(group) if kind == "town" else None,
-            phase_urns=_phase_urns(group, publishable),
         )
         out[place.key] = place
     return out
@@ -137,8 +111,7 @@ def _outcode_places(df, publishable: set[int]) -> dict[str, Place]:
         if len(urns) < MIN_SCHOOLS:
             continue
         place = Place(kind="outcode", slug=str(oc).lower(), name=str(oc),
-                      urns=urns, parent_authority=_parent_authority(group),
-                      phase_urns=_phase_urns(group, publishable))
+                      urns=urns, parent_authority=_parent_authority(group))
         out[place.key] = place
     return out
 
@@ -180,8 +153,7 @@ def _locality_places(df, publishable: set[int],
                 slug, ", ".join(outcodes), len(urns), MIN_SCHOOLS)
             continue
         place = Place(kind="locality", slug=slug, name=name, urns=urns,
-                      parent_authority=_parent_authority(group),
-                      phase_urns=_phase_urns(group, publishable))
+                      parent_authority=_parent_authority(group))
         out[place.key] = place
     return out
 

backend/tests/test_sitemap.py

@@ -268,23 +268,3 @@ def test_place_urls_carry_no_priority_or_changefreq(place_sitemaps):
     for name in ("places-1.xml", "outcodes-1.xml"):
         assert "<priority>" not in place_sitemaps[name]
         assert "<changefreq>" not in place_sitemaps[name]
-
-
-def test_phase_variants_are_submitted_where_the_phase_clears_the_threshold(place_sitemaps):
-    # "primary schools in beccles" is the query shape the baseline showed, so
-    # each variant is its own page and has to be submitted. Emitting only the
-    # bare place URL left ~950 of them reachable by nothing.
-    xml = place_sitemaps["places-1.xml"]
-    assert "<loc>https://www.schoolcompare.co.uk/schools/brentwood/primary</loc>" in xml
-
-
-def test_a_phase_below_its_own_threshold_is_not_submitted(place_sitemaps):
-    # The fixture is six primaries and no secondaries.
-    xml = place_sitemaps["places-1.xml"]
-    assert "/schools/brentwood/secondary" not in xml
-
-
-def test_outcodes_get_no_phase_variants(place_sitemaps):
-    # Nobody searches "primary schools in CM13"; the routes do not exist.
-    xml = place_sitemaps["outcodes-1.xml"]
-    assert "/primary" not in xml and "/secondary" not in xml

e2e/tests/journeys.spec.ts

@@ -1842,46 +1842,3 @@ test('a place page states the local average against England', async ({ page }) =
   await page.goto(`/schools/${place.slug}`);
   await expect(page.getByTestId('local-vs-england')).toContainText(/across England/i);
 });
-
-test('a place page links its phase variants, and they resolve', async ({ page }) => {
-  // "primary schools in beccles" is the query shape the baseline showed. The
-  // first cut submitted only the bare place URL and linked nothing, leaving
-  // ~950 variant pages reachable by nothing at all.
-  const res = await page.request.get('/api/places');
-  const { places } = await res.json();
-  const town = places.find((p: { kind: string }) => p.kind === 'town');
-  expect(town).toBeTruthy();
-
-  const detail = await (await page.request.get(`/api/places/town/${town.slug}`)).json();
-  test.skip(!(detail.place.phases ?? []).length, 'no phase clears the threshold here');
-
-  await page.goto(`/schools/${town.slug}`);
-  const phase = detail.place.phases[0];
-  const link = page.locator(`a[href="/schools/${town.slug}/${phase}"]`).first();
-  await expect(link).toBeVisible();
-
-  await link.click();
-  await expect(page.locator('h1')).toContainText(new RegExp(`${phase} schools in`, 'i'));
-});
-
-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</);
-});
-
-test('no page title repeats the brand', async ({ page }) => {
-  // The root layout appends '| schoolcompare' to a plain-string title. Any
-  // route whose title already carries the brand must opt out with
-  // `absolute`, or it ships '... | schoolcompare | schoolcompare' — which is
-  // how ~2,600 place pages first went out.
-  const res = await page.request.get('/api/places');
-  const { places } = await res.json();
-  const town = places.find((p: { kind: string }) => p.kind === 'town');
-
-  for (const path of ['/', '/rankings', '/admissions', `/schools/${town.slug}`]) {
-    await page.goto(path);
-    const title = await page.title();
-    const brands = (title.match(/schoolcompare/gi) ?? []).length;
-    expect(brands, `${path} repeats the brand: ${title}`).toBeLessThanOrEqual(1);
-  }
-});

nextjs-app/__tests__/app/placeMetadata.test.ts

@@ -14,7 +14,7 @@ jest.mock('@/lib/places', () => ({
 describe('place page metadata', () => {
   it('titles the page the way the place is searched', async () => {
     const m = await placeMeta({ params: Promise.resolve({ place: 'brentwood' }) });
-    expect((m.title as { absolute: string }).absolute).toMatch(/schools in brentwood/i);
+    expect(m.title).toMatch(/schools in brentwood/i);
   });
 
   it('canonicalises to its own path on the www host', async () => {
@@ -23,18 +23,6 @@ describe('place page metadata', () => {
       .toBe('https://www.schoolcompare.co.uk/schools/brentwood');
   });
 
-  it('opts out of the layout template, which would double the brand', () => {
-    // The root layout appends '| schoolcompare' to a plain string title, and
-    // these titles already carry it — every place page shipped reading
-    // '... | schoolcompare | schoolcompare' until this was made absolute.
-    return placeMeta({ params: Promise.resolve({ place: 'brentwood' }) })
-      .then((m) => {
-        expect(typeof m.title).toBe('object');
-        expect((m.title as { absolute: string }).absolute)
-          .not.toMatch(/schoolcompare.*schoolcompare/);
-      });
-  });
-
   it('an unknown place gets a not-found title rather than inventing one', async () => {
     const m = await placeMeta({ params: Promise.resolve({ place: 'atlantis' }) });
     expect(m.title).toMatch(/not found/i);

nextjs-app/__tests__/components/PlaceView.test.tsx

@@ -4,7 +4,7 @@ import type { PlaceDetail } from '@/lib/places';
 
 const detail: PlaceDetail = {
   place: { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 29,
-           parent_authority: 'Essex', phases: ['primary'] },
+           parent_authority: 'Essex' },
   schools: [
     { urn: 1, school_name: 'Alpha Primary', rwm_expected_pct: 82,
       ofsted_grade: 1 } as never,
@@ -91,24 +91,3 @@ describe('PlaceView structured data', () => {
     }
   });
 });
-
-describe('PlaceView phase variants', () => {
-  it('links the phase variants that exist', () => {
-    render(<PlaceView detail={detail} englandAverage={61} neighbours={[]} />);
-    expect(screen.getByRole('link', { name: /Primary schools in Brentwood/i }))
-      .toHaveAttribute('href', '/schools/brentwood/primary');
-  });
-
-  it('links no variant for a phase below its own threshold', () => {
-    render(<PlaceView detail={detail} englandAverage={61} neighbours={[]} />);
-    expect(screen.queryByRole('link', { name: /Secondary schools in Brentwood/i }))
-      .not.toBeInTheDocument();
-  });
-
-  it('does not link sideways from a variant page to itself', () => {
-    render(<PlaceView detail={detail} phase="primary" englandAverage={61}
-      neighbours={[]} />);
-    expect(screen.queryByRole('link', { name: /Primary schools in Brentwood/i }))
-      .not.toBeInTheDocument();
-  });
-});

nextjs-app/app/schools/[place]/[phase]/page.tsx

@@ -37,7 +37,7 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
   const word = phase === 'secondary' ? 'Secondary' : 'Primary';
   const { name } = detail.place;
   return {
-    title: { absolute: `${word} Schools in ${name} — Ranked | schoolcompare` },
+    title: `${word} Schools in ${name} — Ranked | schoolcompare`,
     description:
       `Every ${phase} school in ${name} ranked by results, with Ofsted grades and `
       + `the local average against England.`,

nextjs-app/app/schools/[place]/page.tsx

@@ -53,10 +53,7 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
 
   const { name, count } = detail.place;
   return {
-    // absolute: the root layout's template appends '| schoolcompare' to a
-    // plain string, and this title already carries it. Without this every
-    // place title read '... | schoolcompare | schoolcompare'.
-    title: { absolute: `Schools in ${name} — Compare ${count} Schools | schoolcompare` },
+    title: `Schools in ${name} — Compare ${count} Schools | schoolcompare`,
     description:
       `Every school in ${name} ranked by SATs and GCSE results, with Ofsted grades, `
       + `the local average against England, and how close you had to live to get a place.`,

nextjs-app/app/schools/authority/[la]/page.tsx

@@ -43,7 +43,7 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
 
   const { name, count } = detail.place;
   return {
-    title: { absolute: `Schools in ${name} — Local Authority | schoolcompare` },
+    title: `Schools in ${name} — Local Authority | schoolcompare`,
     description:
       `All ${count} schools in the ${name} local authority, ranked by SATs and GCSE `
       + `results, with Ofsted grades and the authority average against England.`,

nextjs-app/app/schools/near/[outcode]/page.tsx

@@ -38,7 +38,7 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
 
   const { name, count } = detail.place;
   return {
-    title: { absolute: `Schools near ${name} | schoolcompare` },
+    title: `Schools near ${name} | schoolcompare`,
     description:
       `${count} schools in the ${name} postcode district, ranked by results, with `
       + `Ofsted grades and how close you had to live to get a place.`,

nextjs-app/components/places/PlaceView.module.css

@@ -106,13 +106,3 @@
   padding: 0;
   margin: 0;
 }
-
-/* Phase variants are separate indexable pages, so the bare place page has to
-   link them — a sitemap entry alone leaves them with no internal path in. */
-.phaseLinks {
-  display: flex;
-  flex-wrap: wrap;
-  gap: 0.5rem 1rem;
-  margin: 0 0 1.25rem;
-  font-size: 0.9375rem;
-}

nextjs-app/components/places/PlaceView.tsx

@@ -89,16 +89,6 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)
         </p>
       </header>
 
-      {!phase && (place.phases ?? []).length > 0 && (
-        <nav className={styles.phaseLinks} aria-label="By phase">
-          {(place.phases ?? []).map((ph) => (
-            <Link key={ph} href={`/schools/${place.slug}/${ph}`}>
-              {ph === 'secondary' ? 'Secondary schools' : 'Primary schools'} in {place.name}
-            </Link>
-          ))}
-        </nav>
-      )}
-
       {local != null && englandAverage != null && (
         <p className={styles.compare} data-testid="local-vs-england">
           {place.name} averages <strong>{Math.round(local)}</strong> against{' '}

nextjs-app/lib/places.ts

@@ -13,9 +13,6 @@ export interface PlaceSummary {
   slug: string;
   name: string;
   count: number;
-  /** Phases that clear the threshold on their own, so the page links
-   *  variants that exist rather than 404s. Absent on the registry listing. */
-  phases?: string[];
 }
 
 export interface PlaceDetail {