Compare commits
2
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
12cbbda3c8 | ||
|
|
6f749ed21f |
No files matched your search
+26
-7
@@ -209,12 +209,27 @@ def _place_url(place) -> str:
|
||||
|
||||
|
||||
def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]:
|
||||
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
|
||||
]
|
||||
"""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
|
||||
|
||||
|
||||
def build_sitemaps() -> dict[str, str]:
|
||||
@@ -1216,7 +1231,11 @@ 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},
|
||||
"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)]},
|
||||
"schools": clean_for_json(rows[cols]),
|
||||
"averages": averages,
|
||||
}
|
||||
|
||||
+31
-3
@@ -14,7 +14,7 @@ from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import re
|
||||
from dataclasses import dataclass
|
||||
from dataclasses import dataclass, field
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -30,6 +30,13 @@ 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:
|
||||
@@ -46,6 +53,24 @@ 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.
|
||||
|
||||
@@ -79,6 +104,7 @@ 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
|
||||
@@ -111,7 +137,8 @@ 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))
|
||||
urns=urns, parent_authority=_parent_authority(group),
|
||||
phase_urns=_phase_urns(group, publishable))
|
||||
out[place.key] = place
|
||||
return out
|
||||
|
||||
@@ -153,7 +180,8 @@ 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))
|
||||
parent_authority=_parent_authority(group),
|
||||
phase_urns=_phase_urns(group, publishable))
|
||||
out[place.key] = place
|
||||
return out
|
||||
|
||||
|
||||
@@ -268,3 +268,23 @@ 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
|
||||
@@ -1842,3 +1842,46 @@ 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);
|
||||
}
|
||||
});
|
||||
@@ -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).toMatch(/schools in brentwood/i);
|
||||
expect((m.title as { absolute: string }).absolute).toMatch(/schools in brentwood/i);
|
||||
});
|
||||
|
||||
it('canonicalises to its own path on the www host', async () => {
|
||||
@@ -23,6 +23,18 @@ 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);
|
||||
|
||||
@@ -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' },
|
||||
parent_authority: 'Essex', phases: ['primary'] },
|
||||
schools: [
|
||||
{ urn: 1, school_name: 'Alpha Primary', rwm_expected_pct: 82,
|
||||
ofsted_grade: 1 } as never,
|
||||
@@ -91,3 +91,24 @@ 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();
|
||||
});
|
||||
});
|
||||
@@ -37,7 +37,7 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
|
||||
const word = phase === 'secondary' ? 'Secondary' : 'Primary';
|
||||
const { name } = detail.place;
|
||||
return {
|
||||
title: `${word} Schools in ${name} — Ranked | schoolcompare`,
|
||||
title: { absolute: `${word} Schools in ${name} — Ranked | schoolcompare` },
|
||||
description:
|
||||
`Every ${phase} school in ${name} ranked by results, with Ofsted grades and `
|
||||
+ `the local average against England.`,
|
||||
|
||||
@@ -53,7 +53,10 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
|
||||
|
||||
const { name, count } = detail.place;
|
||||
return {
|
||||
title: `Schools in ${name} — Compare ${count} Schools | schoolcompare`,
|
||||
// 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` },
|
||||
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.`,
|
||||
|
||||
@@ -43,7 +43,7 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
|
||||
|
||||
const { name, count } = detail.place;
|
||||
return {
|
||||
title: `Schools in ${name} — Local Authority | schoolcompare`,
|
||||
title: { absolute: `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.`,
|
||||
|
||||
@@ -38,7 +38,7 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
|
||||
|
||||
const { name, count } = detail.place;
|
||||
return {
|
||||
title: `Schools near ${name} | schoolcompare`,
|
||||
title: { absolute: `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.`,
|
||||
|
||||
@@ -106,3 +106,13 @@
|
||||
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;
|
||||
}
|
||||
@@ -89,6 +89,16 @@ 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{' '}
|
||||
|
||||
@@ -13,6 +13,9 @@ 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 {
|
||||
|
||||
Reference in new issue
Block a user