Compare commits

..
Author SHA1 Message Date
TudorandClaude Opus 5 12cbbda3c8 fix(places): stop the place titles doubling the brand
Every place page shipped as 'Schools in Brentwood - Compare 27 Schools |
schoolcompare | schoolcompare'. The root layout's title template appends
'| schoolcompare' to any plain-string title, and all four place routes already
carried the brand. W8 opted the other routes out with an absolute title; the
place routes were written afterwards and did not inherit the lesson.

~2,600 titles affected, and the repetition pushed them past Google's
truncation point, so the doubled brand displaced real words in the result.

An e2e journey now asserts no title repeats the brand, across the static
routes and a place page, so this cannot come back on a route added later.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
2026-08-21 21:43:12 +01:00
13 changed files with 90 additions and 842 deletions

No files matched your search

+3 -15
View File
@@ -1212,15 +1212,10 @@ async def get_place(request: Request, kind: str, slug: str,
if wanted and "phase" in rows.columns:
rows = rows[rows["phase"].fillna("").str.lower().isin(wanted)]
# The metric the page shows, and averages.
# The metric the page ranks on, which is also the one it averages.
metric = "attainment_8_score" if phase == "secondary" else "rwm_expected_pct"
# Alphabetical, not by score. A place page is read by someone looking for
# a school they can name, and scanning for it is what the order should
# serve. /rankings is where the league-table ordering lives, and it keeps
# sorting by metric.
if "school_name" in rows.columns:
rows = rows.sort_values("school_name", key=lambda c: c.str.lower())
if metric in rows.columns:
rows = rows.sort_values(metric, ascending=False, na_position="last")
averages = {
m: (None if m not in rows.columns or rows[m].dropna().empty
@@ -1237,13 +1232,6 @@ async def get_place(request: Request, kind: str, slug: str,
"place": {"kind": place.kind, "slug": place.slug, "name": place.name,
"count": len(place.urns),
"parent_authority": place.parent_authority,
# Every authority the place meaningfully sits in. SW19 is
# mostly Merton but partly Wandsworth; naming one asserts
# something false.
"authorities": [
{"name": name, "slug": _slugify(name), "count": n}
for name, n in place.authorities
],
# Only phases that clear the threshold, so the page links
# variants that exist rather than 404s.
"phases": [ph for ph in ("primary", "secondary")
+20 -124
View File
@@ -30,12 +30,6 @@ class Place:
name: str
urns: tuple[int, ...]
parent_authority: str | None # authority NAME, for the 301 target
# Every authority the place meaningfully sits in, largest first. A quarter
# of outcodes and a third of towns straddle a boundary — SW19 is mostly
# Merton but partly Wandsworth — so naming only one asserts something
# false. parent_authority stays single because a redirect needs one
# target; this is what the page shows.
authorities: tuple[tuple[str, int], ...] = ()
# 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.
@@ -59,151 +53,57 @@ def _publishable_urns(df) -> set[int]:
return set(df.loc[df[cols].notna().any(axis=1), "urn"].astype(int))
# The measure a phase page is built around. A page with no results in this
# column has nothing a list of school names does not already give.
_PHASE_METRIC = {
"primary": "rwm_expected_pct",
"secondary": "attainment_8_score",
}
def _phase_urns(group, publishable: set[int]) -> dict[str, tuple[int, ...]]:
"""URNs per phase, counting only schools with a result for that phase.
Not merely "publishable". A school with an Ofsted grade and no results is
worth a page of its own and belongs in the place list, but it cannot
populate a phase page's results column — and the threshold is there to ask
whether that column will have anything in it.
Counting publishable schools instead let /schools/kent/primary publish
with none of its five rows carrying a result, and left 44 phase pages
majority-blank. It is the same rule as "no page without a local average",
which was never extended per phase.
All-through schools count toward both phases, matching the PHASE_GROUPS
mapping the search filters already use.
"""
"""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)]
# The page lists every school of the phase; the threshold counts only
# those carrying a result, so a mostly-empty table never publishes.
metric = _PHASE_METRIC[phase]
with_result = (
{int(u) for u in subset.loc[subset[metric].notna(), "urn"]}
if metric in subset.columns else set()
)
if len(with_result & publishable) < MIN_SCHOOLS:
continue
urns = tuple(sorted({int(u) for u in subset["urn"]} & publishable))
if urns:
out[phase] = urns
return out
# A place is described by an authority when it holds at least a tenth of the
# schools, and at least two. GIAS carries occasional postcode errors — EN6
# lists two Shropshire schools among fourteen in Hertfordshire — and a bare
# "any authority present" rule would print those as though they were real.
# There is deliberately no cap on how many are named. An earlier cut stopped
# at three, which silently dropped the fourth in exactly the case where the
# information matters most — a genuinely fragmented place. The share rule is
# the only limit, and it already bounds the list at ten.
_AUTHORITY_MIN_SHARE = 0.10
_AUTHORITY_MIN_SCHOOLS = 2
def _parent_authority(group) -> str | None:
"""The most common authority in a group — the useful 301 target.
def _authorities(group) -> tuple[tuple[str, int], ...]:
"""Authorities this place meaningfully sits in, largest first."""
from backend.app import EXCLUDED_FILTER_VALUES
if "local_authority" not in group.columns:
return ()
counts = group["local_authority"].dropna().value_counts()
total = int(counts.sum())
if not total:
return ()
kept = [
(str(name), int(n)) for name, n in counts.items()
if str(name) not in EXCLUDED_FILTER_VALUES
and n >= _AUTHORITY_MIN_SCHOOLS
and n / total >= _AUTHORITY_MIN_SHARE
]
# A place too small or too fragmented for the share rule still names its
# largest authority, or the page would say nothing about where it is.
if not kept:
for name, n in counts.items():
if str(name) not in EXCLUDED_FILTER_VALUES:
return ((str(name), int(n)),)
return ()
return tuple(kept)
def _parent_authority(authorities: tuple[tuple[str, int], ...]) -> str | None:
"""The 301 target: the largest authority a place sits in.
Derived from `authorities` rather than computed separately. The first cut
used `mode()` here while `authorities` used `value_counts()`, and on an
exact tie pandas does not guarantee the two pick the same name — so the
redirect could have pointed somewhere other than the authority the page
named first. One computation, one answer.
Deriving it also inherits the sentinel filter, so a place can no longer
redirect to /schools/authority/does-not-apply.
A town spanning several authorities has no single parent, so the mode is
the honest answer rather than an arbitrary first row.
"""
return authorities[0][0] if authorities else None
if "local_authority" not in group.columns:
return None
top = group["local_authority"].dropna()
return str(top.mode().iloc[0]) if not top.empty else None
def _group(df, column: str, kind: str, publishable: set[int]) -> dict[str, Place]:
"""One Place per distinct SLUG in `column` that clears the threshold.
Grouped by slug, not by raw value, because GIAS spells the same place
several ways and they all resolve to one URL. Five town slugs come from
more than one spelling: "London" (1,819 schools) and "LONDON" (12) both
slugify to `london`; Weston-super-Mare is split 14/19 across two
spellings; Newcastle-under-Lyme across three.
Grouping by raw value meant the later group simply overwrote the earlier
one in this dict — so /schools/london could have shown twelve schools
instead of 1,819, silently and depending on row order.
The display name is the most common spelling, which is the one a reader
expects to see.
"""
"""One Place per distinct value of `column` that clears the threshold."""
from backend.app import _slugify
if column not in df.columns:
return {}
working = df.assign(_slug=df[column].map(
lambda v: _slugify(str(v).strip()) if isinstance(v, str) and v.strip() else None))
working = working[working["_slug"].notna() & (working["_slug"] != "")]
out: dict[str, Place] = {}
for slug, group in working.groupby("_slug"):
slug = str(slug)
for name, group in df.groupby(column, dropna=True):
name = str(name).strip()
if not name:
continue
urns = tuple(sorted({int(u) for u in group["urn"]} & publishable))
if len(urns) < MIN_SCHOOLS:
continue
spellings = group[column].dropna().value_counts()
if spellings.empty:
slug = _slugify(name)
if not slug:
continue
name = str(spellings.index[0]).strip()
authorities = () if kind == "authority" else _authorities(group)
place = Place(
kind=kind, slug=slug, name=name, urns=urns,
parent_authority=_parent_authority(authorities),
authorities=authorities,
parent_authority=_parent_authority(group) if kind == "town" else None,
phase_urns=_phase_urns(group, publishable),
)
out[place.key] = place
@@ -236,10 +136,8 @@ def _outcode_places(df, publishable: set[int]) -> dict[str, Place]:
urns = tuple(sorted({int(u) for u in group["urn"]} & publishable))
if len(urns) < MIN_SCHOOLS:
continue
authorities = _authorities(group)
place = Place(kind="outcode", slug=str(oc).lower(), name=str(oc),
urns=urns, parent_authority=_parent_authority(authorities),
authorities=authorities,
urns=urns, parent_authority=_parent_authority(group),
phase_urns=_phase_urns(group, publishable))
out[place.key] = place
return out
@@ -281,10 +179,8 @@ def _locality_places(df, publishable: set[int],
"threshold of %d - not published",
slug, ", ".join(outcodes), len(urns), MIN_SCHOOLS)
continue
authorities = _authorities(group)
place = Place(kind="locality", slug=slug, name=name, urns=urns,
parent_authority=_parent_authority(authorities),
authorities=authorities,
parent_authority=_parent_authority(group),
phase_urns=_phase_urns(group, publishable))
out[place.key] = place
return out
-160
View File
@@ -229,163 +229,3 @@ def test_no_curated_locality_names_a_london_borough():
f"these are boroughs, not districts: {sorted(named)} - they already "
"have an authority page covering every school"
)
def test_a_place_names_every_authority_it_straddles():
"""SW19 is mostly Merton but partly Wandsworth.
A quarter of viable outcodes and a third of viable towns cross an
authority boundary, so naming only the largest asserts something false.
"""
rows = (_town(26, "London", "Merton", start=300000)
+ _town(7, "London", "Wandsworth", start=400000))
for r in rows:
r["postcode"] = "SW19 1AA"
reg = build_place_registry(_df(rows))
names = [n for n, _ in reg["outcode:sw19"].authorities]
assert names == ["Merton", "Wandsworth"] # largest first
assert dict(reg["outcode:sw19"].authorities)["Wandsworth"] == 7
def test_the_redirect_target_stays_a_single_authority():
# parent_authority and authorities do different jobs: a 301 needs one
# target, the page needs the truth.
rows = (_town(26, "London", "Merton", start=300000)
+ _town(7, "London", "Wandsworth", start=400000))
for r in rows:
r["postcode"] = "SW19 1AA"
reg = build_place_registry(_df(rows))
assert reg["outcode:sw19"].parent_authority == "Merton"
def test_a_stray_authority_below_the_share_threshold_is_not_named():
# GIAS carries postcode errors — EN6 lists two Shropshire schools among
# fourteen in Hertfordshire. Printing those as though real would be worse
# than omitting them.
rows = (_town(30, "Barnet", "Hertfordshire", start=300000)
+ _town(1, "Barnet", "Shropshire", start=400000))
for r in rows:
r["postcode"] = "EN6 1AA"
reg = build_place_registry(_df(rows))
assert [n for n, _ in reg["outcode:en6"].authorities] == ["Hertfordshire"]
def test_a_sentinel_authority_is_never_named():
rows = (_town(20, "London", "Merton", start=300000)
+ _town(6, "London", "Does not apply", start=400000))
for r in rows:
r["postcode"] = "SW19 1AA"
reg = build_place_registry(_df(rows))
assert [n for n, _ in reg["outcode:sw19"].authorities] == ["Merton"]
def test_a_place_always_names_at_least_one_authority():
# Even when every authority is below the share threshold, the page has to
# say where the place is.
rows = []
for i, la in enumerate(["A", "B", "C", "D", "E", "F", "G"]):
rows += _town(1, "Fragmented", la, start=300000 + i * 100)
reg = build_place_registry(_df(rows))
place = reg.get("town:fragmented")
assert place is not None
assert len(place.authorities) == 1
def test_every_qualifying_authority_is_named_with_no_cap():
"""An earlier cut stopped at three, dropping the fourth silently.
That truncation bit exactly where the information matters most — a
genuinely fragmented place — and nothing recorded it.
"""
rows = []
for i, la in enumerate(["Hackney", "Lambeth", "Westminster", "Lewisham"]):
rows += _town(3, "Fourway", la, start=300000 + i * 100)
reg = build_place_registry(_df(rows))
assert len(reg["town:fourway"].authorities) == 4
def test_the_redirect_target_is_the_authority_named_first():
"""They were computed separately — mode() against value_counts() — and on
an exact tie pandas does not guarantee the two agree."""
rows = (_town(26, "London", "Merton", start=300000)
+ _town(7, "London", "Wandsworth", start=400000))
for r in rows:
r["postcode"] = "SW19 1AA"
place = build_place_registry(_df(rows))["outcode:sw19"]
assert place.parent_authority == place.authorities[0][0]
def test_a_place_never_redirects_to_a_sentinel_authority():
# Deriving the parent from `authorities` inherits its sentinel filter.
rows = (_town(6, "Someplace", "Does not apply", start=300000)
+ _town(5, "Someplace", "Essex", start=400000))
reg = build_place_registry(_df(rows))
assert reg["town:someplace"].parent_authority == "Essex"
def test_spellings_of_one_place_are_merged_not_overwritten():
"""GIAS spells the same place several ways, and they share a URL.
"London" (1,819 schools) and "LONDON" (12) both slugify to `london`.
Grouping by raw value let the later group overwrite the earlier one, so
the page could have shown twelve schools instead of 1,819 — silently, and
depending on row order.
"""
rows = (_town(6, "Weston-super-Mare", "North Somerset", start=300000)
+ _town(5, "Weston-Super-Mare", "North Somerset", start=400000))
reg = build_place_registry(_df(rows))
assert len(reg["town:weston-super-mare"].urns) == 11
def test_the_merged_place_takes_its_most_common_spelling():
rows = (_town(9, "Newcastle-under-Lyme", "Staffordshire", start=300000)
+ _town(5, "NEWCASTLE-UNDER-LYME", "Staffordshire", start=400000))
reg = build_place_registry(_df(rows))
assert reg["town:newcastle-under-lyme"].name == "Newcastle-under-Lyme"
def test_a_phase_page_needs_results_not_merely_publishable_schools():
"""/schools/kent/primary published with none of its five rows scored.
The threshold counted schools that were publishable — a result OR an
Ofsted grade — while the page exists for its results column. Forty-four
phase pages were majority-blank; one had no results at all.
"""
rows = _town(MIN_SCHOOLS, "Kent", "Kent")
for r in rows:
r["rwm_expected_pct"] = np.nan # Ofsted only, no results
reg = build_place_registry(_df(rows))
assert "town:kent" in reg # the place still publishes
assert not reg["town:kent"].publishes_phase("primary")
def test_a_phase_page_publishes_once_enough_schools_carry_a_result():
rows = _town(MIN_SCHOOLS, "Beccles", "Suffolk")
reg = build_place_registry(_df(rows))
assert reg["town:beccles"].publishes_phase("primary")
def test_a_publishing_phase_page_still_lists_its_unscored_schools():
"""The threshold gates whether the page exists; it does not filter rows.
A parent looking up a school by name has to find it whether or not it
published results.
"""
scored = _town(MIN_SCHOOLS, "Beccles", "Suffolk", start=300000)
unscored = _town(2, "Beccles", "Suffolk", start=400000)
for r in unscored:
r["rwm_expected_pct"] = np.nan
reg = build_place_registry(_df(scored + unscored))
place = reg["town:beccles"]
assert place.publishes_phase("primary")
assert len(place.phase_urns["primary"]) == MIN_SCHOOLS + 2
def test_the_secondary_threshold_counts_its_own_metric():
# A town full of scored primaries must not thereby publish a secondary page.
rows = _town(MIN_SCHOOLS, "Brentwood", "Essex")
reg = build_place_registry(_df(rows))
assert not reg["town:brentwood"].publishes_phase("secondary")
+2 -22
View File
@@ -45,30 +45,10 @@ def test_registry_carries_a_count_per_place(client):
assert town["count"] == 6
def test_place_detail_returns_its_schools_alphabetically(client):
"""A place page is read by someone looking for a school they can name.
Scanning for it is what the order should serve, so the list is A-Z.
/api/rankings is where the league-table ordering lives.
"""
def test_place_detail_returns_its_schools_ranked(client):
body = client.get("/api/places/town/brentwood").json()
assert body["place"]["name"] == "Brentwood"
names = [s["school_name"] for s in body["schools"]]
assert names == sorted(names, key=str.lower)
def test_place_ordering_ignores_case(client):
body = client.get("/api/places/town/brentwood").json()
names = [s["school_name"] for s in body["schools"]]
# A capitalised name must not sort ahead of every lowercase one.
assert names == sorted(names, key=str.lower)
def test_the_rankings_endpoint_still_ranks_by_metric(client):
# Alphabetical is a place-page decision, not a site-wide one.
body = client.get("/api/rankings?metric=rwm_expected_pct&phase=primary").json()
scores = [r["rwm_expected_pct"] for r in body.get("rankings", [])
if r.get("rwm_expected_pct") is not None]
scores = [s["rwm_expected_pct"] for s in body["schools"]]
assert scores == sorted(scores, reverse=True)
+3 -97
View File
@@ -1649,29 +1649,13 @@ const CANONICAL_ROUTES: Array<[string, string]> = [
['/admissions', 'https://www.schoolcompare.co.uk/admissions'],
];
/**
* Next normalises canonical URLs against `trailingSlash: false`, so the root
* ships as `https://www.schoolcompare.co.uk` with no slash while every other
* route keeps its path. Both forms address the same document, and which one
* Next emits is its business, not something worth pinning a test to.
*
* The first cut hardcoded the slash and failed only on the homepage — the
* same gap as the doubled brand: it asserted the metadata object rather than
* what the page actually renders.
*/
function sameUrl(a: string | null, b: string): boolean {
const strip = (u: string) => u.replace(/\/+$/, '');
return strip(a ?? '') === strip(b);
}
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(sameUrl(hrefs[0], expected),
`${path} canonical was ${hrefs[0]}, expected ${expected}`).toBe(true);
expect(hrefs[0]).toBe(expected);
});
}
@@ -1679,8 +1663,7 @@ 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(sameUrl(href, 'https://www.schoolcompare.co.uk/'),
`filtered homepage canonical was ${href}`).toBe(true);
expect(href).toBe('https://www.schoolcompare.co.uk/');
});
test('a school page canonicalises to its own slug on the www host', async ({ page }) => {
@@ -1731,33 +1714,10 @@ test('staging answers noindex, and stays crawlable so the noindex is seen', asyn
// The other half, and the reason this is one test rather than two: a
// Disallow would stop Google fetching the page at all, so it would never
// see the noindex above. The two only work together.
//
// Scoped to the `*` group. The first cut matched `Disallow: /` anywhere in
// the file and tripped over the AI-crawler groups Cloudflare injects —
// ClaudeBot, GPTBot, Amazonbot and friends all carry a blanket disallow,
// deliberately, and none of them is Googlebot.
const robots = await (await page.request.get('/robots.txt')).text();
expect(blocksEverything(robots, '*'),
'the * group must not disallow the whole site, or the noindex is never seen')
.toBe(false);
expect(robots).not.toMatch(/^\s*Disallow:\s*\/\s*$/mi);
});
/** True when `agent`'s group in a robots.txt disallows the entire site. */
function blocksEverything(robots: string, agent: string): boolean {
let current: string | null = null;
let blocked = false;
for (const raw of robots.split('\n')) {
const line = raw.split('#')[0].trim();
if (!line) continue;
const [key, ...rest] = line.split(':');
const value = rest.join(':').trim();
const k = key.trim().toLowerCase();
if (k === 'user-agent') current = value;
else if (current === agent && k === 'disallow' && value === '/') blocked = true;
}
return blocked;
}
test('a school page on staging is noindexed too, not just the homepage', async ({ page }) => {
const list = await page.request.get('/api/schools?search=primary&per_page=1');
const [first] = (await list.json()).schools ?? [];
@@ -1925,57 +1885,3 @@ test('no page title repeats the brand', async ({ page }) => {
expect(brands, `${path} repeats the brand: ${title}`).toBeLessThanOrEqual(1);
}
});
test('a place straddling a boundary names every authority it sits in', async ({ page }) => {
// A quarter of outcodes and a third of towns cross an authority boundary —
// SW19 is mostly Merton but partly Wandsworth. Naming only the largest
// asserts something false about the place.
const { places } = await (await page.request.get('/api/places')).json();
const outcode = places.find((p: { kind: string }) => p.kind === 'outcode');
expect(outcode).toBeTruthy();
// Find any place the registry reports as straddling.
let straddling: { kind: string; slug: string } | null = null;
for (const p of places.filter((p: { kind: string }) => p.kind === 'outcode').slice(0, 40)) {
const d = await (await page.request.get(`/api/places/outcode/${p.slug}`)).json();
if ((d.place.authorities ?? []).length > 1) { straddling = p; break; }
}
test.skip(!straddling, 'no straddling outcode found in the sample');
const detail = await (await page.request.get(
`/api/places/outcode/${straddling!.slug}`)).json();
await page.goto(`/schools/near/${straddling!.slug}`);
for (const a of detail.place.authorities) {
await expect(page.locator(`a[href="/schools/authority/${a.slug}"]`).first())
.toBeVisible();
}
});
test('a place page lists its schools alphabetically', async ({ page }) => {
// Someone on a place page is usually looking for a school they can name,
// so the order should serve scanning for it. /rankings is where the
// league-table ordering lives.
const { places } = await (await page.request.get('/api/places')).json();
const town = places.find((p: { kind: string; count: number }) =>
p.kind === 'town' && p.count >= 5);
expect(town).toBeTruthy();
await page.goto(`/schools/${town.slug}`);
const names = await page.locator('a[href^="/school/"]').allTextContents();
expect(names.length).toBeGreaterThan(1);
const sorted = [...names].sort((a, b) =>
a.toLowerCase().localeCompare(b.toLowerCase()));
expect(names).toEqual(sorted);
});
test('the rankings page still orders by score, not name', async ({ page }) => {
// Alphabetical is a place-page decision, not a site-wide one.
const res = await page.request.get('/api/rankings?metric=rwm_expected_pct&phase=primary');
expect(res.ok()).toBeTruthy();
const scores = ((await res.json()).rankings ?? [])
.map((r: { rwm_expected_pct: number | null }) => r.rwm_expected_pct)
.filter((v: number | null) => v != null);
expect(scores).toEqual([...scores].sort((a: number, b: number) => b - a));
});
@@ -7,9 +7,9 @@ const detail: PlaceDetail = {
parent_authority: 'Essex', phases: ['primary'] },
schools: [
{ urn: 1, school_name: 'Alpha Primary', rwm_expected_pct: 82,
ofsted_grade: 1, phase: 'Primary' } as never,
ofsted_grade: 1 } as never,
{ urn: 2, school_name: 'Beta Primary', rwm_expected_pct: 44,
ofsted_grade: 3, phase: 'Primary' } as never,
ofsted_grade: 3 } as never,
],
averages: { rwm_expected_pct: 63, attainment_8_score: null },
};
@@ -112,167 +112,3 @@ describe('PlaceView phase variants', () => {
.not.toBeInTheDocument();
});
});
describe('PlaceView presentation', () => {
// /schools/brentwood shipped with 8 of 27 rows blank: an unphased page shows
// one primary-only measure for a list that also holds secondaries.
const mixed: PlaceDetail = {
place: { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 4,
parent_authority: 'Essex', phases: ['primary', 'secondary'] },
schools: [
{ urn: 1, school_name: 'Alpha Primary', phase: 'Primary',
rwm_expected_pct: 82, attainment_8_score: null } as never,
{ urn: 2, school_name: 'Beta High', phase: 'Secondary',
rwm_expected_pct: null, attainment_8_score: 47 } as never,
],
averages: { rwm_expected_pct: 63, attainment_8_score: 45 },
};
it('gives each phase its own table rather than one column of blanks', () => {
render(<PlaceView detail={mixed} englandAverage={61} neighbours={[]} />);
expect(screen.getByRole('heading', { name: /^Primary schools/ })).toBeInTheDocument();
expect(screen.getByRole('heading', { name: /^Secondary schools/ })).toBeInTheDocument();
expect(screen.getByText('82%')).toBeInTheDocument();
expect(screen.getByText('47')).toBeInTheDocument();
});
it('names the measure in plain words, not jargon', () => {
// The first cut said "RWM expected", which appears nowhere else on the site.
render(<PlaceView detail={mixed} englandAverage={61} neighbours={[]} />);
expect(screen.getByText('Reading, writing & maths')).toBeInTheDocument();
expect(screen.getByText('Attainment 8')).toBeInTheDocument();
expect(screen.queryByText(/RWM expected/i)).not.toBeInTheDocument();
});
it('says a missing result is unpublished rather than showing a bare dash', () => {
const noResult: PlaceDetail = {
...mixed,
schools: [{ urn: 3, school_name: 'New Primary', phase: 'Primary',
rwm_expected_pct: null, attainment_8_score: null } as never],
};
render(<PlaceView detail={noResult} englandAverage={61} neighbours={[]} />);
expect(screen.getByText('Not published')).toBeInTheDocument();
});
it('styles school links to the site convention rather than browser default', () => {
const { container } = render(<PlaceView detail={mixed} englandAverage={61}
neighbours={[]} />);
const link = container.querySelector('a[href^="/school/"]');
expect(link?.className).toBeTruthy();
});
it('a phased page shows one table and no phase headings', () => {
render(<PlaceView detail={mixed} phase="primary" englandAverage={61}
neighbours={[]} />);
expect(screen.queryByRole('heading', { name: /^Secondary schools/ }))
.not.toBeInTheDocument();
});
});
describe('PlaceView table alignment', () => {
const aligned: PlaceDetail = {
place: { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 2,
parent_authority: 'Essex', phases: ['primary'] },
schools: [
{ urn: 1, school_name: 'Alpha Primary', phase: 'Primary',
rwm_expected_pct: 82, attainment_8_score: null } as never,
],
averages: { rwm_expected_pct: 63, attainment_8_score: null },
};
it('aligns the measure heading and its values with the same class', () => {
// They were aligned by two different selectors whose specificity did not
// match: `.table th:last-child` (0,2,1) won and went right, while `.num`
// (0,1,0) lost to `.table td` (0,1,1) and stayed left. Sharing one class
// is what makes them impossible to drift apart.
const { container } = render(<PlaceView detail={aligned} englandAverage={61}
neighbours={[]} />);
const th = container.querySelectorAll('th')[1];
const td = container.querySelectorAll('tbody td')[1];
expect(th.className).toBeTruthy();
expect(td.className).toBe(th.className);
});
it('leaves the school-name column unclassed so it takes the spare width', () => {
const { container } = render(<PlaceView detail={aligned} englandAverage={61}
neighbours={[]} />);
expect(container.querySelectorAll('th')[0].className).toBe('');
});
});
describe('PlaceView authorities', () => {
const straddling: PlaceDetail = {
place: { kind: 'outcode', slug: 'sw19', name: 'SW19', count: 33,
parent_authority: 'Merton', phases: ['primary'],
authorities: [
{ name: 'Merton', slug: 'merton', count: 26 },
{ name: 'Wandsworth', slug: 'wandsworth', count: 7 },
] },
schools: [
{ urn: 1, school_name: 'Alpha Primary', phase: 'Primary',
rwm_expected_pct: 82, attainment_8_score: null } as never,
],
averages: { rwm_expected_pct: 63, attainment_8_score: null },
};
it('names every authority the place straddles, not just the largest', () => {
// SW19 is mostly Merton but partly Wandsworth. Naming one asserts
// something false about a quarter of outcodes.
render(<PlaceView detail={straddling} englandAverage={61} neighbours={[]} />);
expect(screen.getByRole('link', { name: 'Merton' }))
.toHaveAttribute('href', '/schools/authority/merton');
expect(screen.getByRole('link', { name: 'Wandsworth' }))
.toHaveAttribute('href', '/schools/authority/wandsworth');
});
it('joins them readably rather than as a bare list', () => {
// Asserted on the summary line's whole text: a loose /and/ matcher also
// hits "Wandsworth".
const { container } = render(<PlaceView detail={straddling}
englandAverage={61} neighbours={[]} />);
const summary = container.querySelector('header p');
expect(summary?.textContent).toContain('Merton and Wandsworth');
});
it('falls back to the single parent when the field is absent', () => {
// A cached API response predating the authorities field must not blank
// the line entirely.
const legacy = { ...straddling,
place: { ...straddling.place, authorities: undefined } };
render(<PlaceView detail={legacy} englandAverage={61} neighbours={[]} />);
expect(screen.getByRole('link', { name: 'Merton' })).toBeInTheDocument();
});
});
describe('PlaceView list ordering', () => {
const detail3: PlaceDetail = {
place: { kind: 'town', slug: 'brentwood', name: 'Brentwood', count: 2,
parent_authority: 'Essex', phases: ['primary'] },
schools: [
{ urn: 1, school_name: 'Alpha Primary', phase: 'Primary',
rwm_expected_pct: 40, attainment_8_score: null } as never,
{ urn: 2, school_name: 'Beta Primary', phase: 'Primary',
rwm_expected_pct: 90, attainment_8_score: null } as never,
],
averages: { rwm_expected_pct: 65, attainment_8_score: null },
};
it('renders schools in the order the API sent them, not by score', () => {
// The API sorts alphabetically now; the component must not re-sort.
render(<PlaceView detail={detail3} englandAverage={61} neighbours={[]} />);
const links = screen.getAllByRole('link', { name: /Primary$/ });
expect(links.map((l) => l.textContent))
.toEqual(['Alpha Primary', 'Beta Primary']);
});
it('declares the list as ascending rather than implying a ranking', () => {
// An ItemList carrying `position` reads as a ranking unless it says
// otherwise, and the table is A-Z.
const { container } = render(<PlaceView detail={detail3} englandAverage={61}
neighbours={[]} />);
const ld = JSON.parse(
container.querySelector('script[type="application/ld+json"]')!.textContent!);
const list = ld['@graph'].find((n: { '@type': string }) => n['@type'] === 'ItemList');
expect(list.itemListOrder).toBe('https://schema.org/ItemListOrderAscending');
});
});
@@ -37,12 +37,10 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
const word = phase === 'secondary' ? 'Secondary' : 'Primary';
const { name } = detail.place;
return {
// Not "Ranked": the table is alphabetical, so the word would be a claim
// the page does not keep.
title: { absolute: `${word} Schools in ${name} | schoolcompare` },
title: { absolute: `${word} Schools in ${name} — Ranked | schoolcompare` },
description:
`Every ${phase} school in ${name}, with results, Ofsted grades and the local `
+ `average against England.`,
`Every ${phase} school in ${name} ranked by results, with Ofsted grades and `
+ `the local average against England.`,
alternates: { canonical: absoluteUrl(`/schools/${slug}/${phase}`) },
};
}
+2 -2
View File
@@ -58,8 +58,8 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
// place title read '... | schoolcompare | schoolcompare'.
title: { absolute: `Schools in ${name} — Compare ${count} Schools | schoolcompare` },
description:
`Every school in ${name}, with SATs and GCSE results, Ofsted grades, the local `
+ `average against England, and how close you had to live to get a place.`,
`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.`,
alternates: { canonical: absoluteUrl(`/schools/${slug}`) },
};
}
@@ -45,8 +45,8 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
return {
title: { absolute: `Schools in ${name} — Local Authority | schoolcompare` },
description:
`All ${count} schools in the ${name} local authority, with SATs and GCSE results, `
+ `Ofsted grades and the authority average against England.`,
`All ${count} schools in the ${name} local authority, ranked by SATs and GCSE `
+ `results, with Ofsted grades and the authority average against England.`,
alternates: { canonical: absoluteUrl(`/schools/authority/${la}`) },
};
}
@@ -40,8 +40,8 @@ export async function generateMetadata({ params }: Props): Promise<Metadata> {
return {
title: { absolute: `Schools near ${name} | schoolcompare` },
description:
`${count} schools in the ${name} postcode district, with results, Ofsted grades `
+ `and how close you had to live to get a place.`,
`${count} schools in the ${name} postcode district, ranked by results, with `
+ `Ofsted grades and how close you had to live to get a place.`,
alternates: { canonical: absoluteUrl(`/schools/near/${outcode}`) },
};
}
+15 -107
View File
@@ -1,8 +1,4 @@
/* Tokens only — see globals.css. Follows RankingsView's conventions, and in
particular its link treatment: table links take --text-primary with no
underline and a brand-coloured hover, not the browser default. The first
cut used bare <Link> with no class at all, which rendered as default blue
underlined links and read as unstyled beside the rest of the site. */
/* Tokens only — see globals.css. Matches RankingsView's conventions. */
.container {
width: 100%;
min-width: 0;
@@ -28,44 +24,6 @@
line-height: 1.6;
}
/* Links in running copy: brand colour, underline on hover only. */
.inlineLink {
color: var(--brand);
text-decoration: none;
transition: color 0.2s ease;
}
.inlineLink:hover {
color: var(--brand-strong);
text-decoration: underline;
}
/* 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 0.75rem;
margin: 0 0 1.25rem;
}
.phaseLink {
display: inline-block;
padding: 0.4rem 0.875rem;
border: 1px solid var(--border-strong);
border-radius: 999px;
font-size: 0.875rem;
font-weight: 500;
color: var(--text-primary);
text-decoration: none;
transition: border-color 0.2s ease, color 0.2s ease;
}
.phaseLink:hover {
border-color: var(--brand);
color: var(--brand-strong);
}
/* The one number a list cannot give you, so it gets its own band. */
.compare {
background: var(--bg-secondary);
@@ -88,30 +46,6 @@
color: var(--text-secondary);
}
.group {
margin-bottom: 2rem;
}
.groupHeading {
display: flex;
align-items: baseline;
gap: 0.625rem;
font-size: 1.25rem;
font-weight: 600;
color: var(--text-primary);
font-family: var(--font-display);
margin: 0 0 0.75rem;
}
.groupCount {
font-size: 0.8125rem;
font-weight: 500;
color: var(--text-secondary);
background: var(--bg-secondary);
border-radius: 999px;
padding: 0.125rem 0.5rem;
}
/* Wide content scrolls in its own container so the page body never does. */
.tableWrap {
overflow-x: auto;
@@ -138,54 +72,18 @@
color: var(--text-secondary);
font-weight: 600;
font-size: 0.8125rem;
text-transform: uppercase;
letter-spacing: 0.04em;
}
.table tbody tr:last-child td {
border-bottom: none;
}
/*
* Header and value share one class and one rule, so they cannot drift apart.
*
* The first cut aligned them with two different selectors: `.table th:last-child`
* at (0,2,1) beat the element rule and went right, while `.num` at (0,1,0) lost
* to `.table td` at (0,1,1) and stayed left. The heading and its numbers sat on
* opposite edges of the column.
*
* width:1% with nowrap makes the measure column hug its content so the school
* name takes the remaining width — without it the two columns split evenly and
* the gap between heading and value reads as misalignment on a wide screen.
*/
.table th.num,
.table td.num {
.table th:last-child,
.num {
text-align: right;
font-variant-numeric: tabular-nums;
width: 1%;
white-space: nowrap;
}
/* The measure is spelled out; the tooltip carries the definition. */
.metricHead {
text-decoration: none;
cursor: help;
border-bottom: 1px dotted var(--border-strong);
}
/* Table links: site convention is body colour, brand on hover. */
.schoolLink {
color: var(--text-primary);
text-decoration: none;
transition: color 0.2s ease;
}
.schoolLink:hover {
color: var(--brand-strong);
}
/* "Not published" is a fact about the school, not an error. */
.noData {
color: var(--text-muted);
font-size: 0.8125rem;
}
.neighbours {
@@ -208,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;
}
+35 -128
View File
@@ -12,7 +12,6 @@
import Link from 'next/link';
import type { PlaceDetail, PlaceSummary } from '@/lib/places';
import { placeUrl, authoritySlug } from '@/lib/places';
import type { School } from '@/lib/types';
import { schoolUrl } from '@/lib/utils';
import { absoluteUrl } from '@/lib/site';
import styles from './PlaceView.module.css';
@@ -31,110 +30,19 @@ const OFSTED_LABELS: Array<[number, string]> = [
[3, 'Requires improvement'], [4, 'Inadequate'],
];
/**
* Column headings, taken from the site's own metric dictionary rather than
* invented here — see METRIC_DEFINITIONS in backend/schemas.py, surfaced at
* /api/metrics. The first cut said "RWM expected", which is jargon that
* appears nowhere else on the site.
*/
const METRICS = {
primary: {
key: 'rwm_expected_pct' as const,
heading: 'Reading, writing & maths',
hint: '% meeting the expected standard in reading, writing and maths',
unit: '%',
},
secondary: {
key: 'attainment_8_score' as const,
heading: 'Attainment 8',
hint: "Average grade across a pupil's best 8 GCSEs, including English and maths",
unit: '',
},
};
type PhaseKey = keyof typeof METRICS;
/** All-through schools sit in both phases, matching the search filters. */
function isPhase(school: School, phase: PhaseKey): boolean {
const p = (school.phase ?? '').toLowerCase();
if (p === 'all-through') return true;
return phase === 'secondary'
? p.includes('secondary') || p === '16 plus'
: p.includes('primary') || p.includes('middle');
}
function SchoolTable({ schools, phase }: { schools: School[]; phase: PhaseKey }) {
const metric = METRICS[phase];
return (
<div className={styles.tableWrap}>
<table className={styles.table}>
<thead>
<tr>
<th scope="col">School</th>
{/* Same class as the value cell below: one rule aligns both, so
they cannot drift apart. */}
<th scope="col" className={styles.num}>
<abbr className={styles.metricHead} title={metric.hint}>
{metric.heading}
</abbr>
</th>
</tr>
</thead>
<tbody>
{schools.map((s) => {
const value = s[metric.key];
return (
<tr key={s.urn}>
<td>
<Link href={schoolUrl(s.urn, s.school_name)} className={styles.schoolLink}>
{s.school_name}
</Link>
</td>
<td className={styles.num}>
{value == null
? <span className={styles.noData}>Not published</span>
: `${Math.round(Number(value))}${metric.unit}`}
</td>
</tr>
);
})}
</tbody>
</table>
</div>
);
}
export function PlaceView({ detail, phase, englandAverage, neighbours }: Props) {
const { place, schools, averages } = detail;
// Fall back to the single parent when the API predates the authorities
// field, so a stale cache never blanks the line entirely.
const authorities = place.authorities?.length
? place.authorities
: place.parent_authority
? [{ name: place.parent_authority, slug: authoritySlug(place.parent_authority), count: 0 }]
: [];
const local = averages[METRICS[phase ?? 'primary'].key];
const metric = phase === 'secondary' ? 'attainment_8_score' : 'rwm_expected_pct';
const local = averages[metric];
const phaseWord = phase === 'secondary' ? 'Secondary schools'
: phase === 'primary' ? 'Primary schools' : 'Schools';
const graded = OFSTED_LABELS
.map(([grade, label]) => [label, schools.filter((s) => s.ofsted_grade === grade).length] as const)
.filter(([, n]) => n > 0);
/*
* An unphased page holds both primaries and secondaries, and they are
* scored on different measures — a percentage and a 0-90 score. Showing one
* column for both left 30% of rows blank on /schools/brentwood and put two
* incomparable scales in one column when it did not.
*
* So the phases get a table each. A blank cell inside one now means the
* school genuinely has no published result, which is worth saying.
*/
const groups: Array<[PhaseKey, School[]]> = phase
? [[phase, schools]]
: (['primary', 'secondary'] as PhaseKey[])
.map((p) => [p, schools.filter((s) => isPhase(s, p))] as [PhaseKey, School[]])
.filter(([, list]) => list.length > 0);
// ItemList tells Google this page is a ranked set rather than prose;
// BreadcrumbList puts the place in a hierarchy. Capped at 20 because that
// is what the page shows above the fold and what the markup should mirror.
const jsonLd = {
'@context': 'https://schema.org',
'@graph': [
@@ -142,10 +50,6 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)
'@type': 'ItemList',
name: `${phaseWord} in ${place.name}`,
numberOfItems: schools.length,
// Alphabetical, and said so. Without this an ItemList carrying
// `position` reads as a ranking, which would be a claim the page
// stopped making when the table became A-Z.
itemListOrder: 'https://schema.org/ItemListOrderAscending',
itemListElement: schools.slice(0, 20).map((s, i) => ({
'@type': 'ListItem',
position: i + 1,
@@ -156,7 +60,8 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)
{
'@type': 'BreadcrumbList',
itemListElement: [
{ '@type': 'ListItem', position: 1, name: 'Schools', item: absoluteUrl('/') },
{ '@type': 'ListItem', position: 1, name: 'Schools',
item: absoluteUrl('/') },
{ '@type': 'ListItem', position: 2, name: place.name },
],
},
@@ -169,25 +74,16 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)
type="application/ld+json"
dangerouslySetInnerHTML={{ __html: JSON.stringify(jsonLd) }}
/>
<header className={styles.header}>
<h1>{phaseWord} in {place.name}</h1>
<p className={styles.summary}>
{place.count} schools
{authorities.length > 0 && (
{place.parent_authority && (
<>
{' · '}
{/* Every authority, not just the largest. A quarter of outcodes
and a third of towns cross a boundary: SW19 is mostly Merton
but partly Wandsworth, and naming one asserts otherwise. */}
{authorities.map((a, i) => (
<span key={a.slug}>
{i > 0 && (i === authorities.length - 1 ? ' and ' : ', ')}
<Link href={`/schools/authority/${a.slug}`} className={styles.inlineLink}>
{a.name}
</Link>
</span>
))}
<Link href={`/schools/authority/${authoritySlug(place.parent_authority)}`}>
{place.parent_authority}
</Link>
</>
)}
</p>
@@ -196,7 +92,7 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)
{!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}`} className={styles.phaseLink}>
<Link key={ph} href={`/schools/${place.slug}/${ph}`}>
{ph === 'secondary' ? 'Secondary schools' : 'Primary schools'} in {place.name}
</Link>
))}
@@ -218,17 +114,28 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)
</ul>
)}
{groups.map(([p, list]) => (
<section key={p} className={styles.group}>
{groups.length > 1 && (
<h2 className={styles.groupHeading}>
{p === 'secondary' ? 'Secondary schools' : 'Primary schools'}
<span className={styles.groupCount}>{list.length}</span>
</h2>
)}
<SchoolTable schools={list} phase={p} />
</section>
))}
<div className={styles.tableWrap}>
<table className={styles.table}>
<thead>
<tr>
<th>School</th>
<th>{phase === 'secondary' ? 'Attainment 8' : 'RWM expected'}</th>
</tr>
</thead>
<tbody>
{schools.map((s) => (
<tr key={s.urn}>
<td>
<Link href={schoolUrl(s.urn, s.school_name)}>{s.school_name}</Link>
</td>
<td className={styles.num}>
{s[metric] == null ? '—' : Math.round(Number(s[metric]))}
</td>
</tr>
))}
</tbody>
</table>
</div>
{neighbours.length > 0 && (
<nav className={styles.neighbours} aria-label="Nearby places">
@@ -236,7 +143,7 @@ export function PlaceView({ detail, phase, englandAverage, neighbours }: Props)
<ul>
{neighbours.map((n) => (
<li key={n.kind + n.slug}>
<Link href={placeUrl(n.kind, n.slug)} className={styles.inlineLink}>{n.name}</Link>
<Link href={placeUrl(n.kind, n.slug)}>{n.name}</Link>
</li>
))}
</ul>
+1 -12
View File
@@ -18,19 +18,8 @@ export interface PlaceSummary {
phases?: string[];
}
export interface PlaceAuthority {
name: string;
slug: string;
count: number;
}
export interface PlaceDetail {
place: PlaceSummary & {
parent_authority: string | null;
/** Every authority the place meaningfully sits in, largest first. SW19 is
* mostly Merton but partly Wandsworth. */
authorities?: PlaceAuthority[];
};
place: PlaceSummary & { parent_authority: string | null };
schools: School[];
averages: {
rwm_expected_pct: number | null;