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>
This commit is contained in:
TudorandClaude Opus 5.5 committed 2026-10-02 10:22:39 +01:00
1 parent eb13ab0b5e
commit 0cc4f52816
4 files changed
+168 -16

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. | | `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/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/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. | | `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 ## Legacy/manual paths requiring operational verification
+17
View File
@@ -286,6 +286,23 @@ test('the phase filter switches straight from secondary to primary', async ({ pa
await expect(phase).toHaveValue('primary'); 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(&|$)/);
const type = page.getByRole('combobox', { name: 'School type', exact: true });
const [first] = (await type.locator('option').allTextContents()).slice(1);
await type.selectOption(first);
await expect(page).toHaveURL(/[?&]school_type=/);
await expect.poll(() => type.locator('option').count()).toBeGreaterThan(2);
});
test('a phase outside primary/secondary filters to that phase, not to everything', async ({ page }) => { 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 // The search page offers every GIAS phase, but the API only knew the grouped
// ones and silently dropped the rest — so "Nursery" returned primaries. // ones and silently dropped the rest — so "Nursery" returned primaries.
@@ -0,0 +1,119 @@
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'])('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.each(['secondary', 'all-through'])('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');
});
});
+31 -16
View File
@@ -66,6 +66,14 @@ const FILTER_KEYS = [
] as const; ] as const;
type FilterKey = (typeof FILTER_KEYS)[number]; 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;
/** Phases with no secondary-age pupils, for which those filters are hidden. */
const PRIMARY_PHASES = new Set(["primary", "nursery", "middle deemed primary"]);
const hasSecondaryFilters = (phase: string) => !PRIMARY_PHASES.has(phase.toLowerCase());
function SlidersIcon() { function SlidersIcon() {
return ( return (
<svg <svg
@@ -326,7 +334,13 @@ export function FilterBar({
}; };
const handleFilterChange = (key: string, value: string) => { 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 // Every filter at once, keeping the search and its distance: what "Clear
@@ -353,20 +367,24 @@ export function FilterBar({
currentAdmissionsPolicy || currentAdmissionsPolicy ||
currentHasSixthForm; 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 = const laOptions =
resultFilters?.local_authorities ?? filters.local_authorities; resultFilters?.local_authorities ?? filters.local_authorities;
const typeOptions = resultFilters?.school_types ?? filters.school_types; const typeOptions = 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 phaseOptions = filters.phases ?? []; const phaseOptions = filters.phases ?? [];
const genderOptions = resultFilters?.genders ?? filters.genders ?? []; const genderOptions = filters.genders ?? [];
const admissionsPolicyOptions = const admissionsPolicyOptions = filters.admissions_policies ?? [];
resultFilters?.admissions_policies ?? filters.admissions_policies ?? [];
const isSecondaryMode = // Set by the phase chosen, never by what the results happen to contain.
currentPhase === "secondary" || genderOptions.length > 0; const isSecondaryMode = hasSecondaryFilters(currentPhase);
// A select that is narrowing the results carries a sage tint; the class is // A select that is narrowing the results carries a sage tint; the class is
// only ever additive, so the control's behaviour is untouched. // only ever additive, so the control's behaviour is untouched.
@@ -409,11 +427,8 @@ export function FilterBar({
// rest are the names themselves. // rest are the names themselves.
const named: Partial<Record<FilterKey, string[]>> = { const named: Partial<Record<FilterKey, string[]>> = {
phase: phaseOptions, phase: phaseOptions,
gender: [...genderOptions, ...(filters.genders ?? [])], gender: genderOptions,
admissions_policy: [ admissions_policy: admissionsPolicyOptions,
...admissionsPolicyOptions,
...(filters.admissions_policies ?? []),
],
}; };
return (named[key] ?? []).find((o) => o.toLowerCase() === value.toLowerCase()) ?? value; return (named[key] ?? []).find((o) => o.toLowerCase() === value.toLowerCase()) ?? value;
}; };