fix(compare): give every basket entry a real phase
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m13s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 17s
PR Checks / Build Frontend (no push) (pull_request) Canceled after 1m6s
PR Checks / Build Pipeline (no push) (pull_request) Canceled after 0s
PR Checks / AI Code Review (Claude) (pull_request) Canceled after 0s
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m13s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 17s
PR Checks / Build Frontend (no push) (pull_request) Canceled after 1m6s
PR Checks / Build Pipeline (no push) (pull_request) Canceled after 0s
PR Checks / AI Code Review (Claude) (pull_request) Canceled after 0s
Review of the per-phase limit found entries reaching the basket with no phase, and a phase-less entry holds a slot in both groups: - Nearby-school cards added without one. The API now returns each peer's own phase (its pool is a phase group, so an all-through school can sit beside a primary); the button passes it through, and an older API simply leaves the conservative both-groups count in place. - Baskets saved before this change were never migrated. The compare page now backfills missing phases from the data it already fetches, and never overwrites a phase the basket has. - "16 plus" counted against both groups; it is secondary, as the API's PHASE_GROUPS files it. Also rewraps the HomeView doc comment the previous commit left awkward. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
0a4c051ee5
commit
587cfe3f0b
11 files changed
+175
-6
No files matched your search
@@ -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<typeof useComparisonContext>;
|
||||
|
||||
function renderBasket(stored: Partial<School>[] = []) {
|
||||
window.localStorage.setItem('selectedSchools', JSON.stringify(stored));
|
||||
const ref: { current: Ctx | null } = { current: null };
|
||||
function Probe() {
|
||||
ref.current = useComparisonContext();
|
||||
return null;
|
||||
}
|
||||
render(
|
||||
<ComparisonProvider>
|
||||
<Probe />
|
||||
</ComparisonProvider>,
|
||||
);
|
||||
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(
|
||||
<ComparisonProvider>
|
||||
<Probe />
|
||||
<AddToCompareButton school={nearby} />
|
||||
</ComparisonProvider>,
|
||||
);
|
||||
fireEvent.click(screen.getByRole('button', { name: /Add to compare/ }));
|
||||
expect(ref.current?.selectedSchools[0].phase).toBe('All-through');
|
||||
});
|
||||
@@ -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', () => {
|
||||
|
||||
Reference in new issue
Block a user