diff --git a/backend/nearby_schools.py b/backend/nearby_schools.py index c4f22bd..a633ec9 100644 --- a/backend/nearby_schools.py +++ b/backend/nearby_schools.py @@ -250,6 +250,10 @@ def select_nearby(frame: pd.DataFrame, urn: int) -> list[dict]: "distance_miles": float(row["distance_miles"]), "school_type": _native(row.get("school_type")), "age_range": _native(row.get("age_range")), + # Each peer's own phase, not the subject's: the pool is a phase + # group, so an all-through school can sit beside a primary. The + # compare basket counts it against both of its tabs. + "phase": _native(row.get("phase")), "shared": _shared(subject, row, is_secondary), "metric_value": _native(row.get(metric_key)), "metric_key": metric_key, diff --git a/backend/tests/test_nearby_schools.py b/backend/tests/test_nearby_schools.py index a096d9b..31f4477 100644 --- a/backend/tests/test_nearby_schools.py +++ b/backend/tests/test_nearby_schools.py @@ -163,6 +163,18 @@ def test_secondary_reaches_further_than_primary(): assert {s["urn"] for s in select_nearby(frame, 100001)} == {100002, 100003} +def test_each_card_carries_its_own_phase(): + # The compare basket limits each phase separately, so an all-through peer + # must not inherit the subject's "Primary". + frame = _frame( + _row(100001, "Subject"), + _row(100002, "A", latitude=_at(0.5)), + _row(100003, "B", phase="All-through", age_range="4-18", latitude=_at(0.6)), + ) + phases = {s["urn"]: s["phase"] for s in select_nearby(frame, 100001)} + assert phases == {100002: "Primary", 100003: "All-through"} + + def test_the_cap_follows_the_phase(): assert radius_miles("Primary") == 2.0 assert radius_miles("Middle deemed primary") == 2.0 diff --git a/nextjs-app/__tests__/context/ComparisonProvider.test.tsx b/nextjs-app/__tests__/context/ComparisonProvider.test.tsx new file mode 100644 index 0000000..0dd6ad6 --- /dev/null +++ b/nextjs-app/__tests__/context/ComparisonProvider.test.tsx @@ -0,0 +1,114 @@ +/** + * The basket limit is five per phase group, not five overall, and the + * provider is where it is enforced for every add path. + * + * Entries without a phase count against both groups. Baskets saved before + * phases were recorded hold such entries, so the compare page backfills them + * once it has fetched each school. + */ + +import { act, render, screen, fireEvent } from '@testing-library/react'; + +import { AddToCompareButton } from '@/components/school/AddToCompareButton'; +import { ComparisonProvider } from '@/context/ComparisonProvider'; +import { useComparisonContext } from '@/context/ComparisonContext'; +import type { NearbySchool, School } from '@/lib/types'; + +type Ctx = ReturnType; + +function renderBasket(stored: Partial[] = []) { + window.localStorage.setItem('selectedSchools', JSON.stringify(stored)); + const ref: { current: Ctx | null } = { current: null }; + function Probe() { + ref.current = useComparisonContext(); + return null; + } + render( + + + , + ); + return ref as { current: Ctx }; +} + +const school = (urn: number, phase: string | null) => + ({ urn, school_name: `School ${urn}`, phase }) as School; + +beforeEach(() => { + window.localStorage.clear(); + jest.spyOn(window, 'alert').mockImplementation(() => {}); +}); + +afterEach(() => jest.restoreAllMocks()); + +it('holds five primary and five secondary schools, and no more of either', () => { + const ctx = renderBasket(); + act(() => { + for (let i = 0; i < 6; i++) ctx.current.addSchool(school(100000 + i, 'Primary')); + for (let i = 0; i < 6; i++) ctx.current.addSchool(school(200000 + i, 'Secondary')); + }); + expect(ctx.current.selectedSchools).toHaveLength(10); + expect(window.alert).toHaveBeenCalledWith(expect.stringMatching(/5 primary schools/)); + expect(window.alert).toHaveBeenCalledWith(expect.stringMatching(/5 secondary schools/)); +}); + +it('frees the second group once a stored entry learns its phase', () => { + // Three phase-less entries from an older basket plus two primaries: the + // primary group reads as full although only two are really primary. + const ctx = renderBasket([ + school(100001, null), + school(100002, null), + school(100003, null), + school(100004, 'Primary'), + school(100005, 'Primary'), + ]); + expect(ctx.current.fullGroupFor({ phase: 'Primary' })).toBe('primary'); + + act(() => { + ctx.current.backfillPhases({ 100001: 'Secondary', 100002: 'Secondary', 100003: 'Secondary' }); + }); + + expect(ctx.current.fullGroupFor({ phase: 'Primary' })).toBeNull(); + expect(ctx.current.selectedSchools.map((s) => s.phase)).toEqual([ + 'Secondary', + 'Secondary', + 'Secondary', + 'Primary', + 'Primary', + ]); +}); + +it('never overwrites a phase the basket already has', () => { + const ctx = renderBasket([school(100001, 'Primary')]); + act(() => ctx.current.backfillPhases({ 100001: 'All-through' })); + expect(ctx.current.selectedSchools[0].phase).toBe('Primary'); +}); + +it('a nearby card adds its own phase, not an unknown one', () => { + const nearby = { + urn: 100009, + school_name: 'Nearby', + distance_miles: 0.4, + school_type: 'Academy', + age_range: '4-18', + phase: 'All-through', + shared: [], + metric_value: null, + metric_key: 'rwm_expected_pct', + metric_year: null, + } as NearbySchool; + + const ref: { current: Ctx | null } = { current: null }; + function Probe() { + ref.current = useComparisonContext(); + return null; + } + render( + + + + , + ); + fireEvent.click(screen.getByRole('button', { name: /Add to compare/ })); + expect(ref.current?.selectedSchools[0].phase).toBe('All-through'); +}); diff --git a/nextjs-app/__tests__/lib/compareLogic.test.ts b/nextjs-app/__tests__/lib/compareLogic.test.ts index 5051b5c..1437deb 100644 --- a/nextjs-app/__tests__/lib/compareLogic.test.ts +++ b/nextjs-app/__tests__/lib/compareLogic.test.ts @@ -328,6 +328,7 @@ describe('basket limits per comparison group', () => { expect(compareGroups('Middle deemed primary')).toEqual(['primary']); expect(compareGroups('Secondary')).toEqual(['secondary']); expect(compareGroups('Middle deemed secondary')).toEqual(['secondary']); + expect(compareGroups('16 plus')).toEqual(['secondary']); }); it('counts schools that could land in either tab against both', () => { diff --git a/nextjs-app/components/ComparisonView.tsx b/nextjs-app/components/ComparisonView.tsx index 9b23799..a0ba996 100644 --- a/nextjs-app/components/ComparisonView.tsx +++ b/nextjs-app/components/ComparisonView.tsx @@ -53,7 +53,8 @@ export function ComparisonView({ const router = useRouter(); const pathname = usePathname(); const searchParams = useSearchParams(); - const { selectedSchools, removeSchool, replaceSchools, isInitialized } = useComparison(); + const { selectedSchools, removeSchool, replaceSchools, backfillPhases, isInitialized } = + useComparison(); const [selectedMetric, setSelectedMetric] = useState(initialMetric); const [isModalOpen, setIsModalOpen] = useState(false); @@ -157,6 +158,17 @@ export function ComparisonView({ }; }, [urnKey, isInitialized]); + useEffect(() => { + if (!comparisonData) return; + backfillPhases( + Object.fromEntries( + Object.values(comparisonData) + .filter((d) => d?.school_info) + .map((d) => [d.school_info.urn, d.school_info.phase]), + ), + ); + }, [comparisonData, backfillPhases]); + const primarySchools = selectedSchools.filter((school) => { const info = comparisonData?.[school.urn]?.school_info; const hasPrimaryData = diff --git a/nextjs-app/components/HomeView.tsx b/nextjs-app/components/HomeView.tsx index 90ee1c8..b49f8fc 100644 --- a/nextjs-app/components/HomeView.tsx +++ b/nextjs-app/components/HomeView.tsx @@ -150,10 +150,11 @@ interface ValueProp { * * Every claim here must name something the product actually does. Two of the * four previously did not: "up to three schools" contradicted the basket limit - * (now MAX_PER_GROUP = 5 per phase, in lib/compareLogic.ts) (and the card further down the page, which - * correctly said five), and "class sizes" described data the codebase has never - * held — grep for it and this line was the only hit. Both are corrected below - * against the real fields, which live in components/school/InclusionSection.tsx. + * of five (and the card further down the page, which correctly said five), and + * "class sizes" described data the codebase has never held — grep for it and + * this line was the only hit. Both are corrected below against the real + * fields, which live in components/school/InclusionSection.tsx. + * The limit is now five per phase: MAX_PER_GROUP in lib/compareLogic.ts. */ const VALUE_PROPS: ValueProp[] = [ { diff --git a/nextjs-app/components/school/AddToCompareButton.tsx b/nextjs-app/components/school/AddToCompareButton.tsx index 51bdd81..b97349c 100644 --- a/nextjs-app/components/school/AddToCompareButton.tsx +++ b/nextjs-app/components/school/AddToCompareButton.tsx @@ -28,6 +28,9 @@ export function AddToCompareButton({ school }: { school: NearbySchool }) { school_name: school.school_name, school_type: school.school_type, age_range: school.age_range, + // The basket limits each phase separately. Missing (an older API) + // counts against both groups, which is safe, just stricter. + phase: school.phase ?? null, } as School); }; diff --git a/nextjs-app/context/ComparisonContext.tsx b/nextjs-app/context/ComparisonContext.tsx index 52446d3..7eaf378 100644 --- a/nextjs-app/context/ComparisonContext.tsx +++ b/nextjs-app/context/ComparisonContext.tsx @@ -17,6 +17,8 @@ interface ComparisonContextType { addSchool: (school: School) => void; removeSchool: (urn: number) => void; replaceSchools: (schools: School[]) => void; + /** Fill in phases missing from stored entries; never overwrites one. */ + backfillPhases: (phases: Record) => void; clearAll: () => void; isSelected: (urn: number) => boolean; /** The comparison group with no room for this school, or null. */ diff --git a/nextjs-app/context/ComparisonProvider.tsx b/nextjs-app/context/ComparisonProvider.tsx index cbe0bbb..8c1c4c0 100644 --- a/nextjs-app/context/ComparisonProvider.tsx +++ b/nextjs-app/context/ComparisonProvider.tsx @@ -72,6 +72,22 @@ export function ComparisonProvider({ children }: { children: React.ReactNode }) setSelectedSchools(fitToGroupLimits(schools)); }, []); + // Baskets saved before phases were recorded (or added from a path that + // lacked one) count against both groups. Fill the gaps once the compare + // page has fetched each school, so they stop holding a slot they don't need. + const backfillPhases = useCallback((phases: Record) => { + setSelectedSchools((prev) => { + let changed = false; + const next = prev.map((s) => { + const phase = phases[s.urn]; + if (s.phase || !phase) return s; + changed = true; + return { ...s, phase }; + }); + return changed ? next : prev; + }); + }, []); + const clearAll = useCallback(() => { setSelectedSchools([]); }, []); @@ -99,6 +115,7 @@ export function ComparisonProvider({ children }: { children: React.ReactNode }) addSchool, removeSchool, replaceSchools, + backfillPhases, clearAll, isSelected, fullGroupFor: fullGroupForSchool, diff --git a/nextjs-app/lib/compareLogic.ts b/nextjs-app/lib/compareLogic.ts index 7cae75c..a10e1a7 100644 --- a/nextjs-app/lib/compareLogic.ts +++ b/nextjs-app/lib/compareLogic.ts @@ -309,7 +309,8 @@ export const MAX_PER_GROUP = 5; export function compareGroups(phase?: string | null): CompareGroup[] { const p = (phase ?? '').toLowerCase(); // "Middle deemed secondary" / "Middle deemed primary" match here too. - if (p.includes('secondary')) return ['secondary']; + // "16 plus" is secondary, as the API's PHASE_GROUPS files it. + if (p.includes('secondary') || p === '16 plus') return ['secondary']; if (p.includes('primary')) return ['primary']; return ['primary', 'secondary']; } diff --git a/nextjs-app/lib/types.ts b/nextjs-app/lib/types.ts index d2fdc97..00f6a75 100644 --- a/nextjs-app/lib/types.ts +++ b/nextjs-app/lib/types.ts @@ -360,6 +360,8 @@ export interface NearbySchool { distance_miles: number; school_type: string | null; age_range: string | null; + /** Optional: a frontend can ship ahead of the API that serves it. */ + phase?: string | null; shared: string[]; metric_value: number | null; metric_key: string;