Compare commits

...
Author SHA1 Message Date
TudorandClaude Opus 5.5 68452681f8 fix(search): match primary phases by their words, not exact labels
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m14s
PR Checks / Backend Smoke (pull_request) Successful in 10s
PR Checks / Build Backend (no push) (pull_request) Successful in 17s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m20s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 12s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 18s
The secondary-only filters were hidden for an exact set of lowercased
labels, so a change of case, hyphen or spacing in a GIAS phase label
would have left them showing. Phases are now matched on their words, as
compareGroups already reads "primary".

The E2E journey no longer counts school types: it asserts that choosing
one leaves the same list on offer, whatever the data holds. A test now
pins that a filter with no options is left out, not shown with only its
"any" option.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-02 10:28:34 +01:00
TudorandClaude Opus 5.5 0cc4f52816 fix(search): offer every filter option, not only those in the results
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m17s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 19s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m22s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 15s
School type, gender and admissions took their options from the result
set, which the filter had already narrowed: choose "Girls" and only
"Girls" was offered, so switching to "Boys" meant clearing first. They
now offer the full lists from /api/filters, as phase already did. Local
authority stays scoped to the results, so a postcode search offers the
councils nearby rather than all 153.

Whether gender, sixth form and admissions show was also decided by the
results (any secondary school in them). It is now decided by the phase
alone: hidden for Primary, Nursery and Middle deemed primary, shown
otherwise. Choosing one of those phases clears the three filters, which
would otherwise stay applied with no control showing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-02 10:22:39 +01:00
tudor eb13ab0b5e Merge pull request 'feat(search): filter on phones through one sheet' (#168) from feat/mobile-filter-sheet into main
Stage (build -> staging -> E2E gate) / prepare (push) Successful in 1s
Stage (build -> staging -> E2E gate) / Build Backend (FastAPI) (push) Successful in 44s
Stage (build -> staging -> E2E gate) / Build Frontend (Next.js) (push) Successful in 1m25s
Stage (build -> staging -> E2E gate) / Build Pipeline (Meltano + dbt + Airflow) (push) Successful in 2m14s
Stage (build -> staging -> E2E gate) / Deploy to Staging (push) Successful in 4s
Stage (build -> staging -> E2E gate) / E2E Journeys against Staging (push) Failing after 3m4s
Reviewed-on: #168
2026-10-02 08:51:17 +00:00
4 changed files with 185 additions and 16 deletions

No files matched your search

+1
View File
@@ -44,6 +44,7 @@ Their previous implementations remain recoverable from Git history.
| `nextjs-app/components/SchoolCard.tsx` and its CSS | Imported by its own tests, not application code. HomeView uses SchoolRow/SecondarySchoolRow. | Decide whether to retire the card design; if removed, remove its dedicated tests as well. Passing tests do not establish runtime use. |
| `backend/database.py: get_db`, `get_db_session` | No remaining callers after removing the importer. Current code creates SessionLocal directly. | Either adopt these helpers during session-lifecycle cleanup or remove them; do not rewrite active sessions in a documentation change. |
| `backend/schemas.py: COLUMN_MAPPINGS`, `NULL_VALUES`, `LA_CODE_TO_NAME` | No remaining Python consumers found after importer removal. Other constants in this module are active. | Remove individual constants after checking external data utilities; retain the module. |
| `backend/app.py: result_filters` keys `school_types`, `phases`, `genders`, `admissions_policies` | Since 2026-10-02 FilterBar offers these from `/api/filters`, because options scoped to the results left only the chosen value on offer. Only `local_authorities` is still read. | Stop computing the four keys in a focused API change; keep `local_authorities`. |
| `backend/config.py: data_dir`, `max_page_size`, `rate_limit_burst` | No active consumers found. `default_page_size` appears only in a branch that expects None, although the route supplies a concrete default. | Reconcile settings with route validation in a focused API change. |
## Legacy/manual paths requiring operational verification
+19
View File
@@ -286,6 +286,25 @@ test('the phase filter switches straight from secondary to primary', async ({ pa
await expect(phase).toHaveValue('primary');
});
test('school type and gender switch straight to another value', async ({ page }) => {
// Their options came from the result set, which the filter had already
// narrowed, so with one value chosen it was the only one on offer.
await page.goto('/?search=school&gender=girls');
// An applied gender filter opens More filters by itself.
const gender = page.getByRole('combobox', { name: 'Gender', exact: true });
await expect(gender).toHaveValue('girls', { timeout: 15_000 });
await gender.selectOption('boys');
await expect(page).toHaveURL(/[?&]gender=boys(&|$)/);
// Whatever types the data holds, choosing one leaves the same list on offer.
const type = page.getByRole('combobox', { name: 'School type', exact: true });
const offered = await type.locator('option').allTextContents();
expect(offered.length, 'no school type to choose').toBeGreaterThan(1);
await type.selectOption(offered[1]);
await expect(page).toHaveURL(/[?&]school_type=/);
await expect.poll(() => type.locator('option').allTextContents()).toEqual(offered);
});
test('a phase outside primary/secondary filters to that phase, not to everything', async ({ page }) => {
// The search page offers every GIAS phase, but the API only knew the grouped
// ones and silently dropped the rest — so "Nursery" returned primaries.
@@ -0,0 +1,125 @@
import { fireEvent, render, screen, within } from '@testing-library/react';
import { FilterBar } from '@/components/FilterBar';
import type { ResultFilters } from '@/lib/types';
/*
* A filter's options must not come from the results it is filtering, or
* choosing one leaves only that one on offer: pick "Girls" and "Boys" is gone
* until the filter is cleared. School type, gender and admissions offer the
* full lists, as phase already did. Local authority stays scoped to the
* results, so a postcode search offers the councils nearby rather than 153.
*
* Gender, sixth form and admissions show unless the phase chosen is a primary
* one, so what the results happen to contain never decides which filters
* there are.
*/
let params = new URLSearchParams('postcode=SW196AR&radius=1');
const push = jest.fn();
jest.mock('next/navigation', () => ({
useSearchParams: () => params,
usePathname: () => '/',
useRouter: () => ({ push, replace: jest.fn(), prefetch: jest.fn() }),
}));
jest.mock('@/lib/analytics', () => ({ track: jest.fn() }));
const filters = {
local_authorities: ['Merton', 'Wandsworth'],
school_types: ['Academy converter', 'Community school'], years: [],
phases: ['Middle deemed primary', 'Nursery', 'Primary', 'Secondary', 'All-through'],
genders: ['Boys', 'Girls', 'Mixed'],
admissions_policies: ['Non-selective', 'Selective'],
};
// What the results came back with once narrowed by the chosen filters.
const narrowed: ResultFilters = {
local_authorities: ['Wandsworth'], school_types: ['Community school'],
phases: ['Secondary'], genders: ['Girls'], admissions_policies: ['Non-selective'],
};
const pushedParams = () => new URLSearchParams(push.mock.calls.at(-1)![0].split('?')[1]);
const openSheet = () => {
fireEvent.click(screen.getByRole('button', { name: /^Filters/ }));
return screen.getByRole('dialog', { name: 'Filters' });
};
const optionsOf = (sheet: HTMLElement, name: string) =>
within(within(sheet).getByRole('combobox', { name }))
.getAllByRole('option').map((o) => o.textContent);
beforeEach(() => {
params = new URLSearchParams('postcode=SW196AR&radius=1');
push.mockClear();
});
describe('filter options', () => {
it('offer every school type, gender and admissions policy, whatever the results hold', () => {
params = new URLSearchParams('postcode=SW196AR&radius=1&school_type=Community+school&gender=girls');
render(<FilterBar filters={filters} resultFilters={narrowed} />);
const sheet = openSheet();
expect(optionsOf(sheet, 'School type')).toEqual(['Any school type', 'Academy converter', 'Community school']);
expect(optionsOf(sheet, 'Gender')).toEqual(['Boys, Girls & Mixed', 'Boys', 'Girls', 'Mixed']);
expect(optionsOf(sheet, 'Admissions')).toEqual(['All admissions types', 'Non-selective', 'Selective']);
});
it('keep local authority to the councils in the results', () => {
render(<FilterBar filters={filters} resultFilters={narrowed} />);
expect(optionsOf(openSheet(), 'Local authority')).toEqual(['All Local Authorities', 'Wandsworth']);
});
it('offer the full school types in the desktop row too', () => {
render(<FilterBar filters={filters} resultFilters={narrowed} />);
const row = screen.getByRole('group', { name: 'Filters' });
expect(within(within(row).getByRole('combobox', { name: 'School type' }))
.getAllByRole('option')).toHaveLength(3);
});
});
describe('the secondary-only filters', () => {
const secondaryOnly = ['Gender', 'Sixth form', 'Admissions'];
const shown = (sheet: HTMLElement) =>
secondaryOnly.filter((name) => within(sheet).queryByRole('combobox', { name }));
it('show with any phase, even when no secondary school is in the results', () => {
const primariesOnly = { ...narrowed, phases: ['Primary'], genders: [], admissions_policies: [] };
render(<FilterBar filters={filters} resultFilters={primariesOnly} />);
expect(shown(openSheet())).toEqual(secondaryOnly);
});
it.each(['primary', 'nursery', 'middle deemed primary', 'Middle-deemed Primary'])(
'hide for the %s phase', (phase) => {
params = new URLSearchParams(`postcode=SW196AR&radius=1&phase=${encodeURIComponent(phase)}`);
render(<FilterBar filters={filters} />);
expect(shown(openSheet())).toEqual([]);
});
it('leave out a filter with no options, rather than show only its "any"', () => {
render(<FilterBar filters={{ ...filters, genders: [], admissions_policies: [] }} />);
expect(shown(openSheet())).toEqual(['Sixth form']);
});
it.each(['secondary', 'all-through', 'middle deemed secondary', '16 plus'])('show for the %s phase', (phase) => {
params = new URLSearchParams(`postcode=SW196AR&radius=1&phase=${phase}`);
render(<FilterBar filters={filters} />);
expect(shown(openSheet())).toEqual(secondaryOnly);
});
it('are cleared by choosing a primary phase, rather than left applied and hidden', () => {
params = new URLSearchParams(
'postcode=SW196AR&radius=1&phase=secondary&gender=girls&has_sixth_form=yes&admissions_policy=selective&school_type=Community+school');
render(<FilterBar filters={filters} />);
fireEvent.change(within(openSheet()).getByRole('combobox', { name: 'Phase' }), { target: { value: 'primary' } });
const next = pushedParams();
expect(next.get('phase')).toBe('primary');
for (const key of ['gender', 'has_sixth_form', 'admissions_policy']) expect(next.get(key)).toBeNull();
expect(next.get('school_type')).toBe('Community school');
});
it('are kept when the new phase still has them', () => {
params = new URLSearchParams('postcode=SW196AR&radius=1&phase=secondary&gender=girls');
render(<FilterBar filters={filters} />);
fireEvent.change(within(openSheet()).getByRole('combobox', { name: 'Phase' }), { target: { value: 'all-through' } });
expect(pushedParams().get('gender')).toBe('girls');
});
});
+40 -16
View File
@@ -66,6 +66,23 @@ const FILTER_KEYS = [
] as const;
type FilterKey = (typeof FILTER_KEYS)[number];
/** Filters that only mean something for schools teaching beyond primary. */
const SECONDARY_ONLY_KEYS = ["gender", "has_sixth_form", "admissions_policy"] as const;
/**
* False for a phase with no secondary-age pupils, for which those filters are
* hidden: Primary, Nursery and Middle deemed primary. Matched on the words,
* not exact labels, so a change of case, hyphen or spacing in the GIAS label
* cannot leave them showing. The same reading of "primary" as
* compareGroups in lib/compareLogic.ts. The API's PHASE_GROUPS answers a
* different question (which phases a phase filter returns: primary includes
* all-through), so it is not this list.
*/
function hasSecondaryFilters(phase: string): boolean {
const p = phase.toLowerCase().replace(/[^a-z]+/g, " ").trim();
return !(p === "nursery" || (p.includes("primary") && !p.includes("secondary")));
}
function SlidersIcon() {
return (
<svg
@@ -326,7 +343,13 @@ export function FilterBar({
};
const handleFilterChange = (key: string, value: string) => {
updateURL({ [key]: value });
// A primary phase hides the secondary-only filters, so it clears them
// too: left applied, they would empty the list with no control showing.
const cleared =
key === "phase" && !hasSecondaryFilters(value)
? Object.fromEntries(SECONDARY_ONLY_KEYS.map((k) => [k, ""]))
: {};
updateURL({ ...cleared, [key]: value });
};
// Every filter at once, keeping the search and its distance: what "Clear
@@ -353,20 +376,24 @@ export function FilterBar({
currentAdmissionsPolicy ||
currentHasSixthForm;
// Use result-scoped filter values when available, fall back to global
/*
* A filter's options come from the full lists, not from the results: the
* results have already been narrowed by that filter, so scoping to them
* would leave only the chosen value on offer, and switching (Girls to Boys,
* one school type to another) would need clearing first.
*
* Local authority is the exception, scoped to the results so a postcode
* search offers the councils nearby rather than all of England's.
*/
const laOptions =
resultFilters?.local_authorities ?? filters.local_authorities;
const typeOptions = resultFilters?.school_types ?? filters.school_types;
// Phase is the exception: always the full list. The result set has already
// been narrowed by the phase filter, so scoping to it would leave only the
// chosen phase on offer and switching phase would need "Any phase" first.
const typeOptions = filters.school_types;
const phaseOptions = filters.phases ?? [];
const genderOptions = resultFilters?.genders ?? filters.genders ?? [];
const admissionsPolicyOptions =
resultFilters?.admissions_policies ?? filters.admissions_policies ?? [];
const genderOptions = filters.genders ?? [];
const admissionsPolicyOptions = filters.admissions_policies ?? [];
const isSecondaryMode =
currentPhase === "secondary" || genderOptions.length > 0;
// Set by the phase chosen, never by what the results happen to contain.
const isSecondaryMode = hasSecondaryFilters(currentPhase);
// A select that is narrowing the results carries a sage tint; the class is
// only ever additive, so the control's behaviour is untouched.
@@ -409,11 +436,8 @@ export function FilterBar({
// rest are the names themselves.
const named: Partial<Record<FilterKey, string[]>> = {
phase: phaseOptions,
gender: [...genderOptions, ...(filters.genders ?? [])],
admissions_policy: [
...admissionsPolicyOptions,
...(filters.admissions_policies ?? []),
],
gender: genderOptions,
admissions_policy: admissionsPolicyOptions,
};
return (named[key] ?? []).find((o) => o.toLowerCase() === value.toLowerCase()) ?? value;
};