Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
93a0a3c26f |
No files matched your search
+23
-49
@@ -81,12 +81,9 @@ def _phase_urns(group, publishable: set[int]) -> dict[str, tuple[int, ...]]:
|
||||
# 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
|
||||
_AUTHORITY_MAX_SHOWN = 3
|
||||
|
||||
|
||||
def _authorities(group) -> tuple[tuple[str, int], ...]:
|
||||
@@ -113,64 +110,43 @@ def _authorities(group) -> tuple[tuple[str, int], ...]:
|
||||
if str(name) not in EXCLUDED_FILTER_VALUES:
|
||||
return ((str(name), int(n)),)
|
||||
return ()
|
||||
return tuple(kept)
|
||||
return tuple(kept[:_AUTHORITY_MAX_SHOWN])
|
||||
|
||||
|
||||
def _parent_authority(authorities: tuple[tuple[str, int], ...]) -> str | None:
|
||||
"""The 301 target: the largest authority a place sits in.
|
||||
def _parent_authority(group) -> str | None:
|
||||
"""The most common authority in a group — the useful 301 target.
|
||||
|
||||
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,
|
||||
authorities=() if kind == "authority" else _authorities(group),
|
||||
phase_urns=_phase_urns(group, publishable),
|
||||
)
|
||||
out[place.key] = place
|
||||
@@ -203,10 +179,9 @@ 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),
|
||||
authorities=_authorities(group),
|
||||
phase_urns=_phase_urns(group, publishable))
|
||||
out[place.key] = place
|
||||
return out
|
||||
@@ -248,10 +223,9 @@ 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),
|
||||
authorities=_authorities(group),
|
||||
phase_urns=_phase_urns(group, publishable))
|
||||
out[place.key] = place
|
||||
return out
|
||||
|
||||
@@ -290,56 +290,3 @@ def test_a_place_always_names_at_least_one_authority():
|
||||
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"
|
||||
@@ -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 ?? [];
|
||||
|
||||
@@ -169,37 +169,6 @@ describe('PlaceView presentation', () => {
|
||||
});
|
||||
});
|
||||
|
||||
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,
|
||||
|
||||
@@ -144,24 +144,10 @@
|
||||
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. */
|
||||
|
||||
@@ -71,9 +71,7 @@ function SchoolTable({ schools, phase }: { schools: School[]; phase: PhaseKey })
|
||||
<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}>
|
||||
<th scope="col">
|
||||
<abbr className={styles.metricHead} title={metric.hint}>
|
||||
{metric.heading}
|
||||
</abbr>
|
||||
|
||||
Reference in new issue
Block a user