From e6048c9ca60a1e6e3d8ea9464e5f96bdc3a9dbfd Mon Sep 17 00:00:00 2001 From: Tudor Date: Thu, 20 Aug 2026 22:05:00 +0100 Subject: [PATCH] docs(seo): implementation plan for W1, crawl hygiene and sitemap Five tasks: one canonical host, canonicals on every route, noindex on parameterised comparisons, a sitemap that drops dataless schools and invented priorities, and a per-family sitemap index. Planning turned up a fault the spec had missed: the apex 301s to www, but metadataBase, the school-page canonical, robots.txt's Sitemap: line and every sitemap named the apex. Task 1 fixes it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj --- .../plans/2026-08-20-w1-crawl-hygiene.md | 1233 +++++++++++++++++ 1 file changed, 1233 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md diff --git a/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md b/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md new file mode 100644 index 0000000..7e3c269 --- /dev/null +++ b/docs/superpowers/plans/2026-08-20-w1-crawl-hygiene.md @@ -0,0 +1,1233 @@ +# W1: Crawl Hygiene and Sitemap — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make every page declare one correct canonical URL, stop the homepage +competing with itself across eleven search params, and turn the sitemap from a +flat 27k-URL dump into a per-family index carrying honest `lastmod`. + +**Architecture:** Two independent halves. The frontend half is Next.js +`Metadata` — a single shared canonical-host constant, `alternates.canonical` on +every route, and `generateMetadata` on `/compare` so a parameterised comparison +goes `noindex`. The backend half is `build_sitemap()` in `backend/app.py`, +which grows a dataless-school filter and splits into a sitemap index with +child sitemaps, proxied through Next by a catch-all route. + +**Tech Stack:** Next.js 15 App Router (`Metadata` / `generateMetadata`), Jest + +jsdom for frontend unit tests, FastAPI + pandas for the sitemap, pytest with +`monkeypatch`-injected DataFrames for backend tests, Playwright for the e2e gate. + +**Spec:** `docs/superpowers/specs/2026-08-20-seo-programme-design.md` (workstream W1) + +## Global Constraints + +- **Canonical host is `https://www.schoolcompare.co.uk`.** The apex 301s to + `www` at Cloudflare (verified 2026-08-20). Every canonical, `metadataBase`, + sitemap `` and `robots.txt` `Sitemap:` line must use `www` so no + canonical points at a redirect. +- **Prerequisite:** branch `feat/england-only-corpus` must be merged first. + This plan's sitemap counts assume the England-only corpus of 25,193 schools. +- **Never push to `main`.** Feature branch and a PR, per `CLAUDE.md`. +- **User-facing behaviour changes extend `e2e/tests/journeys.spec.ts` in the + same PR** — the journeys gate staging and promotion. +- **Backend test command** (there is no local pytest; this builds an isolated env): + ```bash + uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests -q + ``` +- **Frontend test command:** `cd nextjs-app && npm test` +- **Do not start a local server to test** (`CLAUDE.md`). Backend behaviour is + tested through `TestClient`, frontend through Jest, integration through the + staging e2e run. + +--- + +### Task 1: One canonical host, used everywhere + +Today `metadataBase`, the school-page canonical, the backend `BASE_URL` and +`robots.ts` all say `https://schoolcompare.co.uk`, which 301s to `www`. A +canonical that points at a redirect is a wasted signal. This task introduces +one constant and routes every producer of an absolute URL through it. + +**Files:** +- Create: `nextjs-app/lib/site.ts` +- Create: `nextjs-app/__tests__/lib/site.test.ts` +- Modify: `nextjs-app/app/layout.tsx` (the `metadataBase` and `openGraph.url` keys) +- Modify: `nextjs-app/app/robots.ts` (the `sitemap` key) +- Modify: `nextjs-app/app/school/[slug]/page.tsx` (the two hardcoded + `https://schoolcompare.co.uk` template strings in `generateMetadata`) +- Modify: `backend/app.py:51` (`BASE_URL`) +- Test: `backend/tests/test_sitemap.py` (created here, extended in Tasks 4–5) + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: `SITE_URL: string` and `absoluteUrl(path: string): string` from + `@/lib/site`. Tasks 2 and 3 import both. `backend.app.BASE_URL` keeps its + name and type (`str`) so nothing else in `app.py` changes. + +- [ ] **Step 1: Write the failing test for the site-URL helper** + +Create `nextjs-app/__tests__/lib/site.test.ts`: + +```typescript +import { SITE_URL, absoluteUrl } from '@/lib/site'; + +describe('SITE_URL', () => { + it('is the www host, which is the one that serves a 200', () => { + // The apex 301s to www at Cloudflare. A canonical pointing at a redirect + // is a wasted signal, so every absolute URL we emit must already be www. + expect(SITE_URL).toBe('https://www.schoolcompare.co.uk'); + }); + + it('has no trailing slash, so joins never double up', () => { + expect(SITE_URL.endsWith('/')).toBe(false); + }); +}); + +describe('absoluteUrl', () => { + it('joins a rooted path', () => { + expect(absoluteUrl('/rankings')).toBe('https://www.schoolcompare.co.uk/rankings'); + }); + + it('joins a path missing its leading slash', () => { + expect(absoluteUrl('rankings')).toBe('https://www.schoolcompare.co.uk/rankings'); + }); + + it('maps the site root to a bare trailing slash', () => { + expect(absoluteUrl('/')).toBe('https://www.schoolcompare.co.uk/'); + }); +}); +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `cd nextjs-app && npm test -- __tests__/lib/site.test.ts` +Expected: FAIL — `Cannot find module '@/lib/site'`. + +- [ ] **Step 3: Write the helper** + +Create `nextjs-app/lib/site.ts`: + +```typescript +/** + * The one place the site's absolute origin is written down. + * + * The apex domain 301s to www at Cloudflare, so www is the host that actually + * serves a 200. Canonicals, og:url, sitemap entries and the robots.txt + * Sitemap: line must all agree with it — a canonical pointing at a redirect + * makes Google resolve the hop before it can consolidate the signal. + * + * backend/app.py holds the same value as BASE_URL for the sitemap. The two are + * asserted against each other by the e2e journeys rather than shared at build + * time, because the backend and frontend ship as separate images. + */ +export const SITE_URL = 'https://www.schoolcompare.co.uk'; + +/** Absolute URL for a site-relative path. Tolerates a missing leading slash. */ +export function absoluteUrl(path: string): string { + const rooted = path.startsWith('/') ? path : `/${path}`; + return `${SITE_URL}${rooted}`; +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `cd nextjs-app && npm test -- __tests__/lib/site.test.ts` +Expected: PASS (5 tests). + +- [ ] **Step 5: Point the frontend's absolute URLs at the helper** + +In `nextjs-app/app/layout.tsx`, add the import and replace the two hardcoded hosts: + +```typescript +import { SITE_URL } from '@/lib/site'; +``` + +```typescript + metadataBase: new URL(SITE_URL), +``` + +```typescript + openGraph: { + type: 'website', + title: 'schoolcompare | Compare School Performance', + description: 'Compare primary and secondary school SATs and GCSE performance across England', + url: SITE_URL, + siteName: 'schoolcompare', + }, +``` + +In `nextjs-app/app/robots.ts`: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript + sitemap: absoluteUrl('/sitemap.xml'), +``` + +In `nextjs-app/app/school/[slug]/page.tsx`, add the import and replace both +occurrences inside `generateMetadata`: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript + url: absoluteUrl(canonicalPath), +``` + +```typescript + alternates: { + canonical: absoluteUrl(canonicalPath), + }, +``` + +- [ ] **Step 6: Point the backend's BASE_URL at the same host** + +In `backend/app.py`, replace line 51: + +```python +# Must match SITE_URL in nextjs-app/lib/site.ts. The apex 301s to www, and a +# sitemap that redirects wastes a crawl on every URL it lists. +BASE_URL = "https://www.schoolcompare.co.uk" +``` + +- [ ] **Step 7: Write the backend test pinning the host** + +Create `backend/tests/test_sitemap.py`: + +```python +"""Tests for sitemap generation (spec 2026-08-20, workstream W1). + +The sitemap is built from the in-memory school DataFrame, so these inject a +small frame via monkeypatch rather than touching a database. +""" + +import numpy as np +import pandas as pd +import pytest + + +def _schools_df() -> pd.DataFrame: + """Two schools: one with results, one with neither results nor Ofsted.""" + base = { + "local_authority": "Testshire", + "school_type": "Academy", + "phase": "Primary", + "year": 202425, + "ofsted_date": None, + } + return pd.DataFrame( + [ + {**base, "urn": 100001, "school_name": "Alpha Primary", + "rwm_expected_pct": 62.0, "attainment_8_score": np.nan, + "ofsted_grade": 2.0}, + {**base, "urn": 100002, "school_name": "Ghost Primary", + "rwm_expected_pct": np.nan, "attainment_8_score": np.nan, + "ofsted_grade": np.nan}, + ] + ) + + +@pytest.fixture() +def sitemap(monkeypatch) -> str: + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + return app_module.build_sitemap() + + +def test_every_loc_uses_the_www_host(sitemap): + # The apex 301s to www. A that redirects burns a crawl per URL. + assert "https://www.schoolcompare.co.uk" in sitemap + assert "https://schoolcompare.co.uk" not in sitemap +``` + +- [ ] **Step 8: Run both suites to verify they pass** + +Run: +```bash +cd nextjs-app && npm test -- __tests__/lib/site.test.ts && cd .. +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests/test_sitemap.py -q +``` +Expected: frontend 5 passed; backend 1 passed. + +Note: `test_every_loc_uses_the_www_host` asserts absence of the apex string. +Because `https://www.schoolcompare.co.uk` contains `schoolcompare.co.uk` but +not `https://schoolcompare.co.uk`, the second assertion is precise as written. + +- [ ] **Step 9: Commit** + +```bash +git add nextjs-app/lib/site.ts nextjs-app/__tests__/lib/site.test.ts \ + nextjs-app/app/layout.tsx nextjs-app/app/robots.ts \ + "nextjs-app/app/school/[slug]/page.tsx" backend/app.py \ + backend/tests/test_sitemap.py +git commit -m "fix(seo): canonicalise on the www host, which is the one that serves 200 + +The apex 301s to www at Cloudflare, but metadataBase, the school-page +canonical, robots.txt's Sitemap: line and the sitemap's own entries all +named the apex. Every one of those pointed Google at a redirect." +``` + +--- + +### Task 2: A canonical on every route, and one homepage instead of eleven params + +`app/page.tsx` accepts eleven search params and sets no canonical, so every +filter combination is a crawlable near-duplicate of the page we most want to +rank. `/rankings` and `/admissions` set no canonical either. + +**Files:** +- Modify: `nextjs-app/app/page.tsx` (the `metadata` export) +- Modify: `nextjs-app/app/rankings/page.tsx` (the `metadata` export) +- Modify: `nextjs-app/app/admissions/page.tsx` (the `metadata` export) +- Test: `nextjs-app/__tests__/app/metadata.test.ts` (created here, extended in Task 3) + +**Interfaces:** +- Consumes: `absoluteUrl` from `@/lib/site` (Task 1). +- Produces: nothing later tasks import. Task 3 adds cases to the same test file. + +- [ ] **Step 1: Write the failing tests** + +Create `nextjs-app/__tests__/app/metadata.test.ts`: + +```typescript +import { metadata as homeMetadata } from '@/app/page'; +import { metadata as rankingsMetadata } from '@/app/rankings/page'; +import { metadata as admissionsMetadata } from '@/app/admissions/page'; + +describe('canonical URLs', () => { + it('the homepage canonicalises to the bare root', () => { + // page.tsx reads eleven search params. Without this, every filter + // combination is a crawlable near-duplicate of the one page we want to + // rank for "compare schools". + expect(homeMetadata.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/'); + }); + + it('rankings canonicalises to the bare path', () => { + expect(rankingsMetadata.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/rankings'); + }); + + it('admissions canonicalises to the bare path', () => { + expect(admissionsMetadata.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/admissions'); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cd nextjs-app && npm test -- __tests__/app/metadata.test.ts` +Expected: FAIL — all three receive `undefined`. + +- [ ] **Step 3: Add the canonicals** + +In `nextjs-app/app/page.tsx`, add the import and extend the metadata export: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript +export const metadata: Metadata = { + title: { absolute: 'schoolcompare | Compare every school in England' }, + description: 'Search and compare school performance across England', + // This page reads eleven search params. They filter a result set; they do + // not make a new document. Collapsing every combination onto "/" stops the + // homepage competing with itself for its own head terms. + alternates: { canonical: absoluteUrl('/') }, +}; +``` + +In `nextjs-app/app/rankings/page.tsx`: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript +export const metadata: Metadata = { + title: 'School Rankings', + description: 'Top-ranked schools by SATs and GCSE performance across England', + keywords: 'school rankings, top schools, best schools, KS2 rankings, KS4 rankings, school league tables', + // Param forms (?metric=&local_authority=&year=&phase=) collapse here for + // now. W3 replaces them with real indexable paths. + alternates: { canonical: absoluteUrl('/rankings') }, +}; +``` + +In `nextjs-app/app/admissions/page.tsx`: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +```typescript +export const metadata: Metadata = { + title: 'School Admissions Guide', + description: + 'Understand the Primary and Secondary school admissions process in England, with live countdowns to every key deadline and National Offer Day.', + alternates: { canonical: absoluteUrl('/admissions') }, +}; +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd nextjs-app && npm test -- __tests__/app/metadata.test.ts` +Expected: PASS (3 tests). + +- [ ] **Step 5: Add the e2e assertion** + +Append to `e2e/tests/journeys.spec.ts`: + +```typescript +/* + * Canonical URLs (spec 2026-08-20, W1). + * + * Every indexable route declares exactly one canonical, on the www host, with + * no query string. The homepage's eleven search params filter a result set + * rather than making a new document, so they all collapse onto "/". + */ +const CANONICAL_ROUTES: Array<[string, string]> = [ + ['/', 'https://www.schoolcompare.co.uk/'], + ['/rankings', 'https://www.schoolcompare.co.uk/rankings'], + ['/admissions', 'https://www.schoolcompare.co.uk/admissions'], +]; + +for (const [path, expected] of CANONICAL_ROUTES) { + test(`${path} declares exactly one canonical, on the www host`, async ({ page }) => { + await page.goto(path); + const hrefs = await page.locator('link[rel="canonical"]').evaluateAll( + (els) => els.map((e) => e.getAttribute('href'))); + expect(hrefs, `${path} should declare one canonical`).toHaveLength(1); + expect(hrefs[0]).toBe(expected); + }); +} + +test('a filtered homepage still canonicalises to the bare root', async ({ page }) => { + await page.goto('/?search=primary&phase=primary&sort=name&page=2'); + const href = await page.locator('link[rel="canonical"]').first() + .getAttribute('href'); + expect(href).toBe('https://www.schoolcompare.co.uk/'); +}); + +test('a school page canonicalises to its own slug on the www host', async ({ page }) => { + const res = await page.request.get('/api/schools?search=primary&per_page=1'); + expect(res.ok()).toBeTruthy(); + const [first] = (await res.json()).schools ?? []; + expect(first, 'no school available').toBeTruthy(); + + await page.goto(`/school/${first.urn}-x`); + const href = await page.locator('link[rel="canonical"]').first() + .getAttribute('href'); + expect(href).toMatch(/^https:\/\/www\.schoolcompare\.co\.uk\/school\/\d+-/); +}); +``` + +- [ ] **Step 6: Verify the e2e file still parses** + +Run: `cd e2e && npx playwright test --list` +Expected: the five new tests appear in the listing; total rises by 5. + +- [ ] **Step 7: Commit** + +```bash +git add nextjs-app/app/page.tsx nextjs-app/app/rankings/page.tsx \ + nextjs-app/app/admissions/page.tsx \ + nextjs-app/__tests__/app/metadata.test.ts e2e/tests/journeys.spec.ts +git commit -m "fix(seo): declare a canonical on every route + +The homepage read eleven search params and declared no canonical, so every +filter combination was a crawlable near-duplicate of the page we most want to +rank. Rankings and admissions declared none either." +``` + +--- + +### Task 3: A parameterised comparison is not a document + +`/compare?urns=…` is an unbounded parameter space — 25,193 schools make ~317 +million pairs, before triples. Bare `/compare` stays indexable: it is the +landing page for "compare schools", the head term in cluster C1. + +**Files:** +- Modify: `nextjs-app/app/compare/page.tsx` (replace the static `metadata` + export with `generateMetadata`) +- Test: `nextjs-app/__tests__/app/metadata.test.ts` (extend) + +**Interfaces:** +- Consumes: `absoluteUrl` from `@/lib/site` (Task 1). +- Produces: `generateMetadata({ searchParams }): Promise` exported + from `app/compare/page.tsx`, replacing the `metadata` const. `searchParams` + has the same `Promise<{ urns?: string; metric?: string }>` shape the default + export already declares as `ComparePageProps`. + +- [ ] **Step 1: Write the failing tests** + +Append to `nextjs-app/__tests__/app/metadata.test.ts`: + +```typescript +import { generateMetadata as compareMetadata } from '@/app/compare/page'; + +describe('/compare indexability', () => { + it('the bare compare page is indexable and canonical to itself', async () => { + // This is the landing page for the "compare schools" head term. + const meta = await compareMetadata({ searchParams: Promise.resolve({}) }); + expect(meta.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/compare'); + expect(meta.robots).toBeUndefined(); + }); + + it('a comparison of specific schools is noindex, follow', async () => { + // ~317 million pairs before triples. Indexing the parameter space would + // swamp everything else in the corpus. + const meta = await compareMetadata({ + searchParams: Promise.resolve({ urns: '100001,100002' }), + }); + expect(meta.robots).toEqual({ index: false, follow: true }); + }); + + it('a parameterised comparison still canonicalises to the bare path', async () => { + // follow:true plus a canonical means the outbound links to each school + // page still pass value even though this URL is not indexed. + const meta = await compareMetadata({ + searchParams: Promise.resolve({ urns: '100001,100002' }), + }); + expect(meta.alternates?.canonical) + .toBe('https://www.schoolcompare.co.uk/compare'); + }); +}); +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cd nextjs-app && npm test -- __tests__/app/metadata.test.ts` +Expected: FAIL — `compareMetadata is not a function`. + +- [ ] **Step 3: Replace the static metadata with generateMetadata** + +In `nextjs-app/app/compare/page.tsx`, add the import: + +```typescript +import { absoluteUrl } from '@/lib/site'; +``` + +Delete the whole `export const metadata: Metadata = { … };` block and put this +in its place: + +```typescript +/** + * Indexability depends on the query string, so this cannot be a static export. + * + * Bare /compare is the landing page for the "compare schools" head term and + * stays indexable. /compare?urns=… is an unbounded parameter space — 25,193 + * schools make ~317 million pairs — so it goes noindex. It stays `follow` and + * keeps a canonical to the bare path, so the links out to each school page + * still count. + */ +export async function generateMetadata( + { searchParams }: ComparePageProps, +): Promise { + const { urns } = await searchParams; + + const base: Metadata = { + title: 'Compare Schools', + description: + 'Compare schools in England side by side — Ofsted inspections, KS2 and GCSE results against the England average, admissions odds and school community.', + keywords: + 'school comparison, compare schools, Ofsted comparison, school admissions, KS2 comparison, primary school performance', + alternates: { canonical: absoluteUrl('/compare') }, + }; + + if (!urns) return base; + + return { ...base, robots: { index: false, follow: true } }; +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd nextjs-app && npm test -- __tests__/app/metadata.test.ts` +Expected: PASS (6 tests — 3 from Task 2, 3 from this task). + +- [ ] **Step 5: Add the e2e assertion** + +Append to `e2e/tests/journeys.spec.ts`: + +```typescript +test('a bare /compare is indexable, a parameterised one is not', async ({ page }) => { + await page.goto('/compare'); + await expect(page.locator('meta[name="robots"]')).toHaveCount(0); + + const [a, b] = await twoPrimaryUrns(page); + await page.goto(`/compare?urns=${a},${b}`); + const robots = await page.locator('meta[name="robots"]').first() + .getAttribute('content'); + expect(robots).toContain('noindex'); + expect(robots).toContain('follow'); + + // noindex but follow: the links out to each school page still count, so the + // canonical must still be present and point at the bare path. + const canonical = await page.locator('link[rel="canonical"]').first() + .getAttribute('href'); + expect(canonical).toBe('https://www.schoolcompare.co.uk/compare'); +}); +``` + +- [ ] **Step 6: Verify the e2e file parses** + +Run: `cd e2e && npx playwright test --list` +Expected: the new test appears; total rises by 1. + +- [ ] **Step 7: Commit** + +```bash +git add nextjs-app/app/compare/page.tsx \ + nextjs-app/__tests__/app/metadata.test.ts e2e/tests/journeys.spec.ts +git commit -m "fix(seo): noindex parameterised comparisons, keep bare /compare + +25,193 schools make ~317 million pairs. The bare page stays indexable as the +landing page for the head term; the parameter space goes noindex, follow so +its outbound links still count." +``` + +--- + +### Task 4: Drop dataless schools and invented priorities from the sitemap + +`build_sitemap()` lists every URN and attaches `priority` and `changefreq`, +both of which Google ignores, while omitting `lastmod`, which it reads. After +the England-only change, 3,927 of 25,193 schools still have no results and no +Ofsted — nothing for a search result to say. + +`lastmod` is set from each school's `ofsted_date` where one exists, and +omitted otherwise. An always-`now` `lastmod` is a claim Google learns to +distrust; an absent one honestly means "unknown". + +**Files:** +- Modify: `backend/app.py:73-108` (`build_sitemap`) +- Test: `backend/tests/test_sitemap.py` (extend) + +**Interfaces:** +- Consumes: `BASE_URL` (Task 1). +- Produces: `build_sitemap()` keeps its `() -> str` signature. Task 5 changes + what it returns; the helper `_school_sitemap_rows(df) -> list[str]` added + here is what Task 5 reuses. + +- [ ] **Step 1: Write the failing tests** + +Append to `backend/tests/test_sitemap.py`: + +```python +def test_school_with_results_is_listed(sitemap): + assert "/school/100001-alpha-primary" in sitemap + + +def test_school_with_no_results_and_no_ofsted_is_omitted(sitemap): + # Nothing for a search result to say about it. Submitting it spends crawl + # budget and drags the corpus-wide quality signal down. + assert "/school/100002" not in sitemap + + +def test_no_invented_priority_or_changefreq(sitemap): + # Google ignores both. They were noise dressed as signal. + assert "" not in sitemap + assert "" not in sitemap + + +def test_ofsted_date_becomes_lastmod(monkeypatch): + from backend import app as app_module + import datetime + + def _df(): + base = _schools_df() + base.loc[base["urn"] == 100001, "ofsted_date"] = datetime.date(2024, 3, 14) + return base + + monkeypatch.setattr(app_module, "load_school_data", _df) + xml = app_module.build_sitemap() + assert "2024-03-14" in xml + + +def test_no_lastmod_invented_when_date_unknown(monkeypatch): + # An always-now lastmod is a claim Google learns to distrust. Absent + # honestly means unknown. + from backend import app as app_module + + def _df(): + df = _schools_df() + df["ofsted_date"] = None + return df + + monkeypatch.setattr(app_module, "load_school_data", _df) + xml = app_module.build_sitemap() + assert "" not in xml + + +def test_static_routes_are_listed(sitemap): + for path in ("/", "/rankings", "/compare", "/admissions"): + assert f"https://www.schoolcompare.co.uk{path}" in sitemap +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: +```bash +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests/test_sitemap.py -q +``` +Expected: FAIL — the dataless school is still listed, `` is present, +`/admissions` is missing, and no `` is emitted. + +- [ ] **Step 3: Rewrite build_sitemap** + +Replace `build_sitemap` in `backend/app.py` (currently lines 73-108) with: + +```python +# Routes worth submitting that are not a school page. /admissions was missing +# from the sitemap entirely despite being a static, indexable guide. +STATIC_SITEMAP_PATHS = ("/", "/rankings", "/compare", "/admissions") + + +def _has_publishable_data(row) -> bool: + """True when a school page has something a search result could state. + + A school with no results in any year and no Ofsted grade renders an empty + page. Submitting it spends crawl budget and drags the corpus-wide quality + signal down, so it stays out of the sitemap. The page itself still resolves + for anyone who has the URL. + """ + for field in ("rwm_expected_pct", "attainment_8_score", "ofsted_grade"): + value = row.get(field) + if value is not None and not pd.isna(value): + return True + return False + + +def _url_element(loc: str, lastmod: str | None = None) -> str: + """One entry. No priority or changefreq — Google ignores both.""" + body = f"{loc}" + if lastmod: + body += f"{lastmod}" + return f" {body}" + + +def _school_sitemap_rows(df) -> list[str]: + """A element per school that has something to show, URN order. + + lastmod comes from the school's Ofsted date where there is one and is + omitted otherwise. An always-now lastmod is a claim Google learns to + distrust; an absent one honestly means "unknown". + """ + if df.empty or "urn" not in df.columns or "school_name" not in df.columns: + return [] + + rows: list[str] = [] + seen: set[int] = set() + + # Latest row per URN first, so a school's most recent Ofsted date wins. + ordered = df.sort_values("year", ascending=False) if "year" in df.columns else df + + for _, row in ordered.iterrows(): + urn = int(row["urn"]) + if urn in seen: + continue + seen.add(urn) + if not _has_publishable_data(row): + continue + + lastmod = None + ofsted_date = row.get("ofsted_date") + if ofsted_date is not None and not pd.isna(ofsted_date): + lastmod = pd.Timestamp(ofsted_date).date().isoformat() + + rows.append(_url_element( + BASE_URL + _school_url(urn, str(row["school_name"])), lastmod)) + + return rows + + +def build_sitemap() -> str: + """Generate sitemap XML from in-memory school data. Returns the XML string.""" + df = load_school_data() + + lines = ['', + ''] + lines.extend(_url_element(BASE_URL + path) for path in STATIC_SITEMAP_PATHS) + lines.extend(_school_sitemap_rows(df)) + lines.append("") + return "\n".join(lines) +``` + +Confirm `pandas` is imported in `backend/app.py` as `pd`. If it is not, add +`import pandas as pd` alongside the other imports at the top of the file. + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: +```bash +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests/test_sitemap.py -q +``` +Expected: PASS (7 tests). + +- [ ] **Step 5: Run the whole backend suite for regressions** + +Run: +```bash +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests -q +``` +Expected: PASS. Baseline before this plan was 54 passed; expect 61. + +- [ ] **Step 6: Commit** + +```bash +git add backend/app.py backend/tests/test_sitemap.py +git commit -m "fix(seo): submit only school pages that have something to show + +Drops the 3,927 schools with neither results nor an Ofsted grade, adds +/admissions which was never listed, replaces the invented priority and +changefreq with a lastmod taken from each school's Ofsted date." +``` + +--- + +### Task 5: Split the sitemap into a per-family index + +One flat file cannot tell you which page family Google is failing to index. +Search Console reports coverage per submitted sitemap, so splitting by family +is what makes W2's location pages measurable when they land. 25,193 URLs is +still under the 50,000 per-file limit, so this is for the diagnostics, not the +size. + +`/sitemap.xml` becomes the index. Children are `/sitemap-static.xml` and +`/sitemap-schools-{n}.xml`, 10,000 URLs each. + +**Files:** +- Modify: `backend/app.py` (`build_sitemap`, the `_sitemap_xml` cache, the + `/sitemap.xml` route, `/api/admin/regenerate-sitemap`) +- Create: `nextjs-app/lib/sitemapProxy.ts` +- Create: `nextjs-app/app/sitemap-[...parts]/route.ts` +- Modify: `nextjs-app/app/sitemap.xml/route.ts` (body moves to the shared proxy) +- Test: `backend/tests/test_sitemap.py` (extend) + +**Interfaces:** +- Consumes: `_school_sitemap_rows`, `_url_element`, `STATIC_SITEMAP_PATHS` (Task 4). +- Produces: `build_sitemaps() -> dict[str, str]` mapping a filename + (`"sitemap.xml"`, `"sitemap-static.xml"`, `"sitemap-schools-1.xml"`) to its + XML. The module-level cache `_sitemap_xml: str | None` is replaced by + `_sitemaps: dict[str, str] | None`. `build_sitemap()` is kept as a thin + wrapper returning `build_sitemaps()["sitemap.xml"]` so Task 4's tests and + the startup path in `lifespan` keep working unchanged. + +- [ ] **Step 1: Write the failing tests** + +Append to `backend/tests/test_sitemap.py`: + +```python +@pytest.fixture() +def sitemaps(monkeypatch) -> dict: + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + return app_module.build_sitemaps() + + +def test_index_lists_each_child(sitemaps): + index = sitemaps["sitemap.xml"] + assert " entries only; mixing in is invalid. + assert "" not in sitemaps["sitemap.xml"] + + +def test_static_child_holds_the_static_routes(sitemaps): + static = sitemaps["sitemap-static.xml"] + for path in ("/", "/rankings", "/compare", "/admissions"): + assert f"https://www.schoolcompare.co.uk{path}" in static + + +def test_school_child_holds_the_schools(sitemaps): + assert "/school/100001-alpha-primary" in sitemaps["sitemap-schools-1.xml"] + + +def test_children_are_chunked_under_the_limit(monkeypatch): + # Sitemaps cap at 50,000 URLs per file. Chunk at 10,000 so a child stays + # small enough to eyeball in Search Console. + from backend import app as app_module + import pandas as _pd + + rows = [ + {"urn": 200000 + i, "school_name": f"School {i}", "year": 202425, + "rwm_expected_pct": 60.0, "attainment_8_score": None, + "ofsted_grade": 2.0, "ofsted_date": None} + for i in range(10_001) + ] + monkeypatch.setattr(app_module, "load_school_data", lambda: _pd.DataFrame(rows)) + + maps = app_module.build_sitemaps() + assert maps["sitemap-schools-1.xml"].count("") == 10_000 + assert maps["sitemap-schools-2.xml"].count("") == 1 + + +def test_build_sitemap_still_returns_the_index(sitemap): + # lifespan and the admin endpoint call build_sitemap(); keep it working. + assert " XML. Populated on startup and by the +# admin regenerate endpoint after a pipeline run. +_sitemaps: dict[str, str] | None = None +``` + +Add below `build_sitemap`'s helpers from Task 4: + +```python +# Sitemaps cap at 50,000 URLs per file. 10,000 keeps a child small enough to +# scan by eye in Search Console, which is the point of splitting at all: +# coverage is reported per submitted sitemap, so one file per page family is +# what makes an indexation problem attributable to a family. +SITEMAP_CHUNK_SIZE = 10_000 + + +def build_sitemaps() -> dict[str, str]: + """Build the sitemap index and every child, keyed by filename.""" + df = load_school_data() + + def _urlset(rows: list[str]) -> str: + return "\n".join([ + '', + '', + *rows, + "", + ]) + + maps: dict[str, str] = { + "sitemap-static.xml": _urlset( + [_url_element(BASE_URL + path) for path in STATIC_SITEMAP_PATHS]), + } + + school_rows = _school_sitemap_rows(df) + # Always emit at least one school child, so the index shape is stable even + # on an empty database. + chunks = [school_rows[i:i + SITEMAP_CHUNK_SIZE] + for i in range(0, len(school_rows), SITEMAP_CHUNK_SIZE)] or [[]] + for n, chunk in enumerate(chunks, start=1): + maps[f"sitemap-schools-{n}.xml"] = _urlset(chunk) + + # On a sitemap index, lastmod means "when this sitemap file last changed", + # so generation time is the correct value here — unlike on a , where + # it would be a claim about content we cannot support. + generated = datetime.now(timezone.utc).date().isoformat() + index_rows = [ + f" {BASE_URL}/{name}" + f"{generated}" + for name in maps + ] + maps["sitemap.xml"] = "\n".join([ + '', + '', + *index_rows, + "", + ]) + return maps + + +def build_sitemap() -> str: + """The sitemap index. Kept for `lifespan` and the admin endpoint.""" + return build_sitemaps()["sitemap.xml"] +``` + +Delete the old `build_sitemap` body from Task 4 (the one assembling a single +``); `_url_element`, `_has_publishable_data`, `_school_sitemap_rows` +and `STATIC_SITEMAP_PATHS` all stay. + +Ensure `from datetime import datetime, timezone` is imported at the top of +`backend/app.py`; add it if absent. + +Replace the `/sitemap.xml` route and add the child route: + +```python +def _serve_sitemap(filename: str) -> Response: + global _sitemaps + if _sitemaps is None: + try: + _sitemaps = build_sitemaps() + except Exception as e: + raise HTTPException(status_code=503, detail=f"Sitemap unavailable: {e}") + if filename not in _sitemaps: + raise HTTPException(status_code=404, detail="No such sitemap") + return Response(content=_sitemaps[filename], media_type="application/xml") + + +@app.get("/sitemap.xml") +async def sitemap_xml(): + """Serve the sitemap index.""" + return _serve_sitemap("sitemap.xml") + + +@app.get("/sitemap-{name}.xml") +async def sitemap_child(name: str): + """Serve a child sitemap (static, or schools-N).""" + return _serve_sitemap(f"sitemap-{name}.xml") +``` + +Update `/api/admin/regenerate-sitemap`: + +```python +@app.post("/api/admin/regenerate-sitemap") +@limiter.limit("10/minute") +async def regenerate_sitemap( + request: Request, + _: bool = Depends(verify_admin_api_key), +): + """Rebuild and cache the sitemaps from current school data. Called by Airflow after data updates.""" + global _sitemaps + _sitemaps = build_sitemaps() + n = sum(x.count("") for x in _sitemaps.values()) + return {"status": "ok", "urls": n, "sitemaps": len(_sitemaps)} +``` + +In `lifespan`, replace the sitemap block: + +```python + try: + _sitemaps = build_sitemaps() + n = sum(x.count("") for x in _sitemaps.values()) + print(f"Sitemaps built: {len(_sitemaps)} files, {n} URLs.") + except Exception as e: + print(f"Warning: sitemap build failed on startup: {e}") +``` + +and change its `global _sitemap_xml` declaration to `global _sitemaps`. + +Update the six Task 4 tests to read from the right child. In +`backend/tests/test_sitemap.py`, change `test_school_with_results_is_listed`, +`test_school_with_no_results_and_no_ofsted_is_omitted`, +`test_ofsted_date_becomes_lastmod` and `test_no_lastmod_invented_when_date_unknown` +to assert against `build_sitemaps()["sitemap-schools-1.xml"]`, and +`test_static_routes_are_listed` against `build_sitemaps()["sitemap-static.xml"]`. +`test_no_invented_priority_or_changefreq` and `test_every_loc_uses_the_www_host` +should assert across every value in `build_sitemaps()`. + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: +```bash +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests/test_sitemap.py -q +``` +Expected: PASS (13 tests). + +- [ ] **Step 5: Replace the Next proxy with a catch-all** + +The index and the children need the same proxy, and a catch-all cannot match +`/sitemap.xml` itself, so both routes stay and share one handler. Extract it to +`nextjs-app/lib/sitemapProxy.ts`: + +```typescript +/** + * Runtime proxy for the sitemap family → the FastAPI backend. + * + * Like the /api/* proxy, this reads FASTAPI_URL at request time rather than + * baking the backend host into the build, so one image works in every + * environment. robots.ts points crawlers at /sitemap.xml, which is the index; + * the index names the children, which land on the same proxy. + */ +import { NextResponse } from 'next/server'; + +function backendOrigin(): string { + const base = process.env.FASTAPI_URL || process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8000/api'; + return base.replace(/\/api$/, ''); +} + +export async function proxySitemap(filename: string): Promise { + let upstream: Response; + try { + upstream = await fetch(`${backendOrigin()}/${filename}`, { cache: 'no-store' }); + } catch { + return new NextResponse('Sitemap temporarily unavailable', { status: 502 }); + } + + const body = await upstream.text(); + return new NextResponse(body, { + status: upstream.status, + headers: { 'content-type': upstream.headers.get('content-type') || 'application/xml' }, + }); +} +``` + +Replace the body of `nextjs-app/app/sitemap.xml/route.ts` with: + +```typescript +import { proxySitemap } from '@/lib/sitemapProxy'; + +export const dynamic = 'force-dynamic'; +export const runtime = 'nodejs'; + +export async function GET() { + return proxySitemap('sitemap.xml'); +} +``` + +Create `nextjs-app/app/sitemap-[...parts]/route.ts`: + +```typescript +import { NextResponse } from 'next/server'; +import { proxySitemap } from '@/lib/sitemapProxy'; + +export const dynamic = 'force-dynamic'; +export const runtime = 'nodejs'; + +/** + * Children are /sitemap-static.xml and /sitemap-schools-{n}.xml. The segment + * pattern is validated here rather than passed through, so this route cannot + * be used to reach arbitrary backend paths. + */ +const CHILD = /^(static|schools-\d+)\.xml$/; + +export async function GET( + _request: Request, + { params }: { params: Promise<{ parts: string[] }> }, +) { + const { parts } = await params; + const name = parts.join('/'); + if (!CHILD.test(name)) { + return new NextResponse('Not found', { status: 404 }); + } + return proxySitemap(`sitemap-${name}`); +} +``` + +- [ ] **Step 6: Add the e2e assertions** + +Replace the `the sitemap submits no Welsh or overseas school` test in +`e2e/tests/journeys.spec.ts` with this block, which keeps its assertions and +follows the index to its children: + +```typescript +test('the sitemap index names children that all resolve', async ({ page }) => { + const res = await page.request.get('/sitemap.xml'); + expect(res.ok()).toBeTruthy(); + const index = await res.text(); + + expect(index).toContain(' entries only; mixing in is invalid. + expect(index).not.toContain(''); + + const locs = [...index.matchAll(/([^<]+)<\/loc>/g)].map((m) => m[1]); + expect(locs.length).toBeGreaterThanOrEqual(2); + + for (const loc of locs) { + expect(loc.startsWith('https://www.schoolcompare.co.uk/')).toBeTruthy(); + const child = await page.request.get(new URL(loc).pathname); + expect(child.ok(), `${loc} should resolve`).toBeTruthy(); + expect(await child.text()).toContain(' { + const index = await (await page.request.get('/sitemap.xml')).text(); + const locs = [...index.matchAll(/([^<]+)<\/loc>/g)].map((m) => m[1]); + + let total = 0; + for (const loc of locs) { + const xml = await (await page.request.get(new URL(loc).pathname)).text(); + total += (xml.match(//g) ?? []).length; + // 401559 (Adamsdown, Cardiff) and 402426 (ACT Schools, Cardiff) were both + // submitted before the England-only filter landed. + expect(xml).not.toContain('/school/401559'); + expect(xml).not.toContain('/school/402426'); + } + expect(total, 'sitemap looks empty or truncated').toBeGreaterThan(1000); +}); + +test('the sitemap invents no priority, changefreq, or lastmod it cannot support', async ({ page }) => { + const index = await (await page.request.get('/sitemap.xml')).text(); + const firstChild = index.match(/([^<]+)<\/loc>/)?.[1]; + expect(firstChild).toBeTruthy(); + + const xml = await (await page.request.get(new URL(firstChild!).pathname)).text(); + expect(xml).not.toContain(''); + expect(xml).not.toContain(''); +}); +``` + +- [ ] **Step 7: Verify the e2e file parses and the whole backend suite is green** + +Run: +```bash +cd e2e && npx playwright test --list && cd .. +uv run --quiet --with-requirements requirements.txt --with pytest \ + --with "httpx==0.27.0" python -m pytest backend/tests -q +cd nextjs-app && npm test && npx next build --no-lint +``` +Expected: e2e listing parses; backend suite green; Jest green; Next build +succeeds with both sitemap routes present. + +- [ ] **Step 8: Commit** + +```bash +git add backend/app.py backend/tests/test_sitemap.py \ + nextjs-app/lib/sitemapProxy.ts nextjs-app/app/sitemap.xml/route.ts \ + "nextjs-app/app/sitemap-[...parts]/route.ts" e2e/tests/journeys.spec.ts +git commit -m "feat(seo): split the sitemap into a per-family index + +Search Console reports coverage per submitted sitemap, so one file per page +family is what will make W2's location pages measurable when they land. The +index's lastmod is generation time, which is the correct semantic there." +``` + +--- + +## After the plan + +1. Open a PR from the feature branch. Do not merge until the England-only PR + is in `main`, or the sitemap counts in the tests will not hold. +2. **Staging needs an Airflow run between the deploy and the e2e gate.** The + England-only filter is a dbt change and staging's marts must be rebuilt + before the journeys run, or the England-only tests fail and block promotion. +3. After staging is verified, resubmit `/sitemap.xml` in Search Console. The + old flat sitemap should be removed from the property so its coverage report + does not compete with the index's. +4. W0's Search Console baseline export should be captured **before** this + lands, so the before/after comparison has a clean cut. + +## Not in this plan + +W1 item 5 in the spec — a better template for the 3,927 English schools with +no data — is deliberately excluded. Task 4 stops submitting them, which is the +crawl-hygiene half. Giving them something worth showing is a product change, +not a crawl fix, and belongs with its own design.