diff --git a/docs/LEGACY_CODE.md b/docs/LEGACY_CODE.md index 165eb09..26e5725 100644 --- a/docs/LEGACY_CODE.md +++ b/docs/LEGACY_CODE.md @@ -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 diff --git a/e2e/tests/journeys.spec.ts b/e2e/tests/journeys.spec.ts index b5f7f9e..c168add 100644 --- a/e2e/tests/journeys.spec.ts +++ b/e2e/tests/journeys.spec.ts @@ -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. diff --git a/nextjs-app/__tests__/components/FilterBarOptions.test.tsx b/nextjs-app/__tests__/components/FilterBarOptions.test.tsx new file mode 100644 index 0000000..35355b4 --- /dev/null +++ b/nextjs-app/__tests__/components/FilterBarOptions.test.tsx @@ -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(); + 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(); + expect(optionsOf(openSheet(), 'Local authority')).toEqual(['All Local Authorities', 'Wandsworth']); + }); + + it('offer the full school types in the desktop row too', () => { + render(); + 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(); + 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(); + expect(shown(openSheet())).toEqual([]); + }); + + it('leave out a filter with no options, rather than show only its "any"', () => { + render(); + 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(); + 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(); + 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(); + fireEvent.change(within(openSheet()).getByRole('combobox', { name: 'Phase' }), { target: { value: 'all-through' } }); + expect(pushedParams().get('gender')).toBe('girls'); + }); +}); diff --git a/nextjs-app/components/FilterBar.tsx b/nextjs-app/components/FilterBar.tsx index 036b7fa..4f406c3 100644 --- a/nextjs-app/components/FilterBar.tsx +++ b/nextjs-app/components/FilterBar.tsx @@ -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 ( { - 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> = { 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; };