diff --git a/backend/app/services/allocation_service.py b/backend/app/services/allocation_service.py index 56331ba..3e98393 100644 --- a/backend/app/services/allocation_service.py +++ b/backend/app/services/allocation_service.py @@ -2392,7 +2392,7 @@ class _Ledger: warnings: list[PlanWarning] = [] warnings.extend(self._age4_warnings(all_days)) warnings.extend(self._sgi_warnings(all_days)) - warnings.extend(self._vacation_expiry_warnings()) + warnings.extend(self._vacation_expiry_warnings(all_days)) return warnings def _age4_projections( @@ -2683,7 +2683,7 @@ class _Ledger: ) return warnings - def _vacation_expiry_warnings(self) -> list[PlanWarning]: + def _vacation_expiry_warnings(self, all_days: list[ProposedDay]) -> list[PlanWarning]: """§Task 2 — the real forfeiture figure (`AllocationInput. vacation_forfeiting`, sourced from the same FIFO carry-forward simulation the Analytics tab uses) replaces the old @@ -2691,20 +2691,43 @@ class _Ledger: total + large carry-in" and could both under- and over-warn relative to what actually gets lost. - This is a forecast, not a post-plan recomputation: `vacation_ - forfeiting` is computed by the loader from already-committed usage, - before this run's own placements (including `_assign_tier3`'s - forfeiture bias above) are added. A (parent, year) this run funds - with extra vacation days over a forecast forfeiture may end up - losing fewer days than warned here — re-simulating the bank with - this run's own placements would mean re-deriving `AnalyticsService`'s - full carry-forward algorithm a second time inside the pure `_Ledger`, - which is deliberately not duplicated here. + `vacation_forfeiting` itself is a forecast: the loader computes it + from already-committed usage, before this run's own placements + (including `_assign_tier3`'s forfeiture-avoidance bias — see its + docstring above) are added. Left unadjusted, that forecast double- + counts — `_assign_tier3` deliberately spends at-risk vacation days + first specifically to avoid this forfeiture, so the naive figure + warns about days the plan is actively saving. + + This method corrects for that here, at the one call site that sees + every placed day, rather than re-deriving `AnalyticsService`'s full + carry-forward/FIFO bank algorithm inside this pure `_Ledger` (still + deliberately not duplicated — see `_assign_tier3`'s docstring): for + each (parent, year) forecast to forfeit, it subtracts the number of + `all_days` entries with `day_type == "vacation"` that this run + placed for that parent with a `date.year` matching that year, and + clamps the result at 0. This is sound *because* the bank is FIFO — + a vacation day spent in year Y is always drawn from the oldest + entry in that parent's bank, which is exactly the entry about to + expire. So each vacation day placed in year Y retires one + forecast-expiring day for that year one-for-one, up to the + forecast itself; the clamp stops the count going negative once + placements cover (or exceed) the forecast, and any placements + beyond that draw from newer, non-expiring entries and are ignored. """ + placed_vacation_by_parent_year: dict[tuple[UUID, int], int] = {} + for d in all_days: + if d.day_type != "vacation": + continue + key = (d.parent_personal_data_uuid, d.date.year) + placed_vacation_by_parent_year[key] = placed_vacation_by_parent_year.get(key, 0) + 1 + warnings: list[PlanWarning] = [] - for (parent_uuid, year), expiring in sorted( + for (parent_uuid, year), forecast in sorted( self._input.vacation_forfeiting.items(), key=lambda kv: (str(kv[0][0]), kv[0][1]) ): + placed = placed_vacation_by_parent_year.get((parent_uuid, year), 0) + expiring = max(0, forecast - placed) if expiring <= 0: continue warnings.append( diff --git a/backend/tests/unit/test_services/test_allocation_service_branches.py b/backend/tests/unit/test_services/test_allocation_service_branches.py index 7e5901c..3b199f8 100644 --- a/backend/tests/unit/test_services/test_allocation_service_branches.py +++ b/backend/tests/unit/test_services/test_allocation_service_branches.py @@ -328,6 +328,11 @@ class TestWarnings: # `has_vacation_near_expiry` heuristic, so a hand-built pure # `AllocationInput` supplies that figure directly rather than a # `VacationBudget` shape the heuristic used to infer it from. + # + # Also the "structurally unresolvable" regression case for the + # post-allocation subtraction added below: no wishes means this run + # places zero vacation days anywhere, so nothing can reduce the + # forecast and the warning must still fire for the full figure. parent = _parent() alloc_input = _build_input( today=date(2027, 1, 1), @@ -347,6 +352,96 @@ class TestWarnings: assert expiry_warnings[0].data["expiring_days"] == 5 assert expiry_warnings[0].data["year"] == 2027 + def test_vacation_expiring_warning_reduced_by_placed_vacation_days(self) -> None: + # §Task 2 follow-up — `_assign_tier3` (family-wide, 0-children wishes + # are vacation-only regardless of its bias) spends vacation to fund + # this window, so the pre-placement forecast must be reduced by + # however many vacation days THIS run actually placed in that same + # year before the warning is shown — the FIFO argument in + # `_vacation_expiry_warnings`'s docstring. + parent = _parent() + wish = _wish(child_id=None, start=(2, 1), end=(2, 7), weeks=1) + alloc_input = _build_input( + today=date(2027, 1, 1), + horizon_end=date(2027, 12, 31), + children=[], + parents=[parent], + pools={}, + vacation={(parent.uuid, 2027): VacationBudget(25, 8, 0)}, + wishes=[wish], + vacation_forfeiting={(parent.uuid, 2027): 8}, + ) + + proposal = Allocator().propose(alloc_input) + + placed_vacation_2027 = sum( + 1 for d in proposal.days if d.day_type == "vacation" and d.date.year == 2027 + ) + assert placed_vacation_2027 == 5 # Mon 1 Feb - Fri 5 Feb, the whole eligible window + + expiry_warnings = [w for w in proposal.warnings if w.kind == "vacation_expiring"] + assert len(expiry_warnings) == 1 + assert expiry_warnings[0].data["expiring_days"] == 3 # 8 forecast - 5 placed + assert expiry_warnings[0].data["year"] == 2027 + + def test_vacation_expiring_warning_suppressed_when_fully_covered(self) -> None: + # M >= N: the plan places at least as many vacation days in the + # forfeiting year as were forecast to expire, so no warning at all. + parent = _parent() + wish = _wish(child_id=None, start=(2, 1), end=(2, 7), weeks=1) + alloc_input = _build_input( + today=date(2027, 1, 1), + horizon_end=date(2027, 12, 31), + children=[], + parents=[parent], + pools={}, + vacation={(parent.uuid, 2027): VacationBudget(25, 5, 0)}, + wishes=[wish], + vacation_forfeiting={(parent.uuid, 2027): 5}, + ) + + proposal = Allocator().propose(alloc_input) + + placed_vacation_2027 = sum( + 1 for d in proposal.days if d.day_type == "vacation" and d.date.year == 2027 + ) + assert placed_vacation_2027 == 5 + + expiry_warnings = [w for w in proposal.warnings if w.kind == "vacation_expiring"] + assert expiry_warnings == [] + + def test_vacation_expiring_warning_unaffected_by_placements_in_other_year(self) -> None: + # The plan places vacation only in 2027; the forfeiture forecast is + # for 2028, a year this run places no vacation in at all, so the + # figure must pass through unchanged. + parent = _parent() + wish = _wish(child_id=None, start=(2, 1), end=(2, 7), weeks=1, first_year=2027) + alloc_input = _build_input( + today=date(2027, 1, 1), + horizon_end=date(2027, 12, 31), + children=[], + parents=[parent], + pools={}, + vacation={ + (parent.uuid, 2027): VacationBudget(25, 0, 0), + (parent.uuid, 2028): VacationBudget(25, 0, 0), + }, + wishes=[wish], + vacation_forfeiting={(parent.uuid, 2028): 4}, + ) + + proposal = Allocator().propose(alloc_input) + + placed_vacation_2027 = sum( + 1 for d in proposal.days if d.day_type == "vacation" and d.date.year == 2027 + ) + assert placed_vacation_2027 == 5 + + expiry_warnings = [w for w in proposal.warnings if w.kind == "vacation_expiring"] + assert len(expiry_warnings) == 1 + assert expiry_warnings[0].data["expiring_days"] == 4 + assert expiry_warnings[0].data["year"] == 2028 + class TestVacationForfeitureBias: """§Task 2 — `_assign_tier3` spends vacation before FP in a year where diff --git a/frontend/src/context/PlanSyncProvider.test.tsx b/frontend/src/context/PlanSyncProvider.test.tsx index c396da9..afca432 100644 --- a/frontend/src/context/PlanSyncProvider.test.tsx +++ b/frontend/src/context/PlanSyncProvider.test.tsx @@ -11,6 +11,8 @@ import { makeEvictionPreview, makeEvictionCandidate, makeResourceShortage, + makePlanProposal, + makePlanWarning, } from '../../tests/fixtures' vi.mock('@/services/api') @@ -197,6 +199,45 @@ describe('PlanSyncProvider', () => { expect(result.current.pendingEvictionPreview).toBeNull() }) + it('starts with no proposal and version 0 before any sync completes', () => { + // A sync is already dispatched by the mount effect (see the `beforeEach` + // workspace mock) but its promise hasn't resolved on this synchronous + // first render yet. + const { result } = renderHook(() => usePlanSyncContext(), { wrapper }) + expect(result.current.latestProposal).toBeNull() + expect(result.current.latestProposalVersion).toBe(0) + }) + + it('exposes the synced proposal and bumps its version exactly once per completed sync', async () => { + const proposal = makePlanProposal({ warnings: [makePlanWarning()] }) + vi.mocked(api.syncPlan).mockResolvedValueOnce(makePlanSyncResponse({ proposal })) + + const { result, rerender } = renderHook(() => usePlanSyncContext(), { wrapper }) + + await waitFor(() => expect(result.current.latestProposal).toEqual(proposal)) + expect(result.current.latestProposalVersion).toBe(1) + + const nextProposal = makePlanProposal({ warnings: [] }) + vi.mocked(api.syncPlan).mockResolvedValueOnce(makePlanSyncResponse({ proposal: nextProposal })) + mockWorkspace({ workspaceId: 'ws-1', planVersion: 1, bumpPlanVersion }) + rerender() + + await waitFor(() => expect(result.current.latestProposal).toEqual(nextProposal)) + expect(result.current.latestProposalVersion).toBe(2) + }) + + it('still stores the proposal from a sync that evicted (wrote) nothing', async () => { + const proposal = makePlanProposal() + const preview = makeEvictionPreview({ wouldEvict: true }) + vi.mocked(api.syncPlan).mockResolvedValueOnce(makePlanSyncResponse({ proposal, evictionPreview: preview })) + + const { result } = renderHook(() => usePlanSyncContext(), { wrapper }) + + await waitFor(() => expect(result.current.pendingEvictionPreview).toEqual(preview)) + expect(result.current.latestProposal).toEqual(proposal) + expect(result.current.latestProposalVersion).toBe(1) + }) + it('throws when used outside a PlanSyncProvider', () => { expect(() => renderHook(() => usePlanSyncContext())).toThrow( 'usePlanSyncContext must be used inside ', diff --git a/frontend/src/context/PlanSyncProvider.tsx b/frontend/src/context/PlanSyncProvider.tsx index 16d8683..7b1b537 100644 --- a/frontend/src/context/PlanSyncProvider.tsx +++ b/frontend/src/context/PlanSyncProvider.tsx @@ -2,7 +2,7 @@ import { createContext, useCallback, useContext, useEffect, useRef, useState } f import * as api from '@/services/api' import { useWorkspace } from './WorkspaceContext' import { isApiError } from '@/services/errors' -import type { EvictionPreview } from '@/types' +import type { EvictionPreview, PlanProposal } from '@/types' interface PlanSyncContextValue { /** Set when the workspace's most recent background sync came back with @@ -15,6 +15,20 @@ interface PlanSyncContextValue { pendingEvictionPreview: EvictionPreview | null /** Call once a consumer has shown/handled `pendingEvictionPreview`. */ clearPendingEvictionPreview: () => void + /** The `PlanProposal` returned by the most recently *completed* background + * sync (read-only — this provider never calls `/plan/propose` itself, it + * just forwards what `syncPlan` already computed). `null` until the first + * sync resolves. Lets any consumer (e.g. Dashboard's warnings/surplus + * advice/outcomes, via `usePlanProposal`) reflect the live plan without + * its own extra `/propose` round-trip — see that hook's doc comment. */ + latestProposal: PlanProposal | null + /** Bumped by exactly 1 every time `latestProposal` is replaced. A + * monotonic counter, not `latestProposal !== null`/object identity: two + * distinct proposals can be reference-different only in a field a naive + * `!==` check might not care about, and a consumer needs to tell "a newer + * sync landed" apart from "the object reference changed for some other + * reason". Starts at 0 (no sync has completed yet). */ + latestProposalVersion: number } const PlanSyncContext = createContext(null) @@ -45,6 +59,8 @@ const PlanSyncContext = createContext(null) export function PlanSyncProvider({ children }: { children: React.ReactNode }) { const { workspaceId, planVersion } = useWorkspace() const [pendingEvictionPreview, setPendingEvictionPreview] = useState(null) + const [latestProposal, setLatestProposal] = useState(null) + const [latestProposalVersion, setLatestProposalVersion] = useState(0) // In-flight/pending refs, not state — flipping them must never itself // cause a re-render (that's exactly the kind of feedback loop the @@ -73,6 +89,14 @@ export function PlanSyncProvider({ children }: { children: React.ReactNode }) { // consumer (Calendar) can ask the user on its next visit. A sync that // did write (or evicted nothing) clears any earlier pending preview. setPendingEvictionPreview(result.evictionPreview.wouldEvict ? result.evictionPreview : null) + // `syncPlan`'s response already carries a fresh `PlanProposal` — stored + // here (read-only state; see the interface doc comment) so + // `usePlanProposal` can adopt it instead of Dashboard/etc. issuing a + // second, redundant `/plan/propose` call just to see current warnings. + // Stored even when `wouldEvict` above is true (nothing was written) — + // the proposal itself is still a valid, freshly-computed read. + setLatestProposal(result.proposal) + setLatestProposalVersion((v) => v + 1) } catch (err) { // No toast exists for a background sync — this is the minimum bar // from the module doc comment: never an empty catch, always logged @@ -112,7 +136,14 @@ export function PlanSyncProvider({ children }: { children: React.ReactNode }) { }, []) return ( - + {children} ) @@ -123,3 +154,14 @@ export function usePlanSyncContext(): PlanSyncContextValue { if (!ctx) throw new Error('usePlanSyncContext must be used inside ') return ctx } + +/** Same as `usePlanSyncContext`, but returns `null` instead of throwing when + * no `` is mounted above the caller, rather than requiring + * every consumer to be wrapped in one. In the real app every page is always + * inside `WorkspaceProvider`'s `` (see `WorkspaceContext.tsx`), + * so this only returns `null` in an isolated test (e.g. `usePlanProposal`'s + * own unit tests, or a page test that auto-mocks `@/context` wholesale) that + * renders a hook/page standalone. */ +export function usePlanSyncContextOptional(): PlanSyncContextValue | null { + return useContext(PlanSyncContext) +} diff --git a/frontend/src/context/index.ts b/frontend/src/context/index.ts index 2903707..ab62e85 100644 --- a/frontend/src/context/index.ts +++ b/frontend/src/context/index.ts @@ -1,3 +1,3 @@ export { useAuth } from './AuthContext' export { useWorkspace } from './WorkspaceContext' -export { usePlanSyncContext } from './PlanSyncProvider' +export { usePlanSyncContext, usePlanSyncContextOptional } from './PlanSyncProvider' diff --git a/frontend/src/hooks/usePlanProposal.test.ts b/frontend/src/hooks/usePlanProposal.test.ts index cc743b6..de47dbe 100644 --- a/frontend/src/hooks/usePlanProposal.test.ts +++ b/frontend/src/hooks/usePlanProposal.test.ts @@ -2,9 +2,17 @@ import { describe, it, expect, beforeEach, vi } from 'vitest' import { renderHook, act } from '@testing-library/react' import { usePlanProposal } from './usePlanProposal' import * as api from '@/services/api' -import { makePlanProposal } from '../../tests/fixtures' +import { usePlanSyncContextOptional } from '@/context' +import { makePlanProposal, makePlanSyncContextValue, makeSurplusAdvice } from '../../tests/fixtures' vi.mock('@/services/api') +// Auto-mocked (not given a factory) — `usePlanSyncContextOptional()` then +// defaults to returning `undefined` until a test calls `mockReturnValue`. +// The hook treats that exactly like the real "no above +// this render" case (`null`): `syncContext?.x ?? …` handles both the same +// way, so the pre-existing tests below (none of which mock this) keep +// exercising the pre-sync, `propose()`-only behaviour unchanged. +vi.mock('@/context') const mockProposal = makePlanProposal() @@ -69,4 +77,54 @@ describe('usePlanProposal', () => { expect(result.current.error).toBe('Failed to compute plan proposal') expect(result.current.data).toBeNull() }) + + describe('preferring PlanSyncProvider\'s proposal', () => { + it('falls back to its own propose() result before any sync has completed', async () => { + vi.mocked(usePlanSyncContextOptional).mockReturnValue( + makePlanSyncContextValue({ latestProposal: null, latestProposalVersion: 0 }), + ) + vi.mocked(api.proposePlan).mockResolvedValue(mockProposal) + + const { result } = renderHook(() => usePlanProposal('ws1')) + await act(async () => { + await result.current.propose() + }) + + expect(result.current.data).toEqual(mockProposal) + }) + + it('prefers the sync context\'s proposal once a background sync has completed', () => { + const syncedProposal = makePlanProposal({ warnings: [] }) + vi.mocked(usePlanSyncContextOptional).mockReturnValue( + makePlanSyncContextValue({ latestProposal: syncedProposal, latestProposalVersion: 1 }), + ) + + const { result } = renderHook(() => usePlanProposal('ws1')) + + expect(result.current.data).toEqual(syncedProposal) + expect(api.proposePlan).not.toHaveBeenCalled() + }) + + it('prefers a newer sync proposal over a stale value from an earlier propose() call', async () => { + const ownProposal = makePlanProposal({ surplusAdvice: [] }) + vi.mocked(api.proposePlan).mockResolvedValue(ownProposal) + vi.mocked(usePlanSyncContextOptional).mockReturnValue( + makePlanSyncContextValue({ latestProposal: null, latestProposalVersion: 0 }), + ) + + const { result, rerender } = renderHook(() => usePlanProposal('ws1')) + await act(async () => { + await result.current.propose() + }) + expect(result.current.data).toEqual(ownProposal) + + const syncedProposal = makePlanProposal({ surplusAdvice: [makeSurplusAdvice()] }) + vi.mocked(usePlanSyncContextOptional).mockReturnValue( + makePlanSyncContextValue({ latestProposal: syncedProposal, latestProposalVersion: 1 }), + ) + rerender() + + expect(result.current.data).toEqual(syncedProposal) + }) + }) }) diff --git a/frontend/src/hooks/usePlanProposal.ts b/frontend/src/hooks/usePlanProposal.ts index 9c070e0..b79b3b3 100644 --- a/frontend/src/hooks/usePlanProposal.ts +++ b/frontend/src/hooks/usePlanProposal.ts @@ -1,5 +1,6 @@ import { useCallback, useState } from 'react' import * as api from '@/services/api' +import { usePlanSyncContextOptional } from '@/context' import type { PlanProposal } from '@/types' interface UsePlanProposalResult { @@ -10,13 +11,36 @@ interface UsePlanProposalResult { } /** - * Triggers `POST /api/plan/propose` on demand (not on mount) — the user asks - * for a proposal by clicking "Föreslå plan"; nothing is persisted server-side. + * `data` is the freshest `PlanProposal` available, from either of two + * sources: + * - `propose()`, triggered on demand (not on mount) — historically the only + * source, still used by callers (e.g. `Dashboard.tsx`'s mount effect) for + * the very first load, before any background sync has run. + * - `PlanSyncProvider` (mounted app-wide — see its doc comment), which + * fires `POST /api/plan/sync` on every `planVersion` bump and already + * gets a fresh `PlanProposal` back in that same response. + * + * Once at least one sync has completed, its proposal is always preferred + * over this hook's own `propose()` result: `PlanSyncProvider` syncs on + * mount and on every `planVersion` bump, a superset of whatever would make + * a caller invoke `propose()` again, so it can never be staler. "At least + * one sync has completed" is read off `latestProposalVersion`, a monotonic + * counter the provider bumps once per completed sync — not off + * `latestProposal !== null`/object identity, which can't distinguish "no + * sync yet" from a (currently impossible, but not type-guaranteed) proposal + * value of `null`, and not off wall-clock time, which a fast `propose()` + * and a slower in-flight sync could resolve out of order against. + * + * `usePlanSyncContextOptional` (not the throwing `usePlanSyncContext`) so + * this hook keeps working when rendered outside a `` — + * e.g. this file's own unit tests, or a page test that auto-mocks + * `@/context` wholesale — falling back to `propose()`-only behaviour. */ export function usePlanProposal(workspaceId: string | null): UsePlanProposalResult { const [data, setData] = useState(null) const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) + const syncContext = usePlanSyncContextOptional() const propose = useCallback(async () => { if (!workspaceId) return @@ -33,5 +57,8 @@ export function usePlanProposal(workspaceId: string | null): UsePlanProposalResu } }, [workspaceId]) - return { data, isLoading, error, propose } + const hasSynced = (syncContext?.latestProposalVersion ?? 0) > 0 + const freshest = hasSynced ? (syncContext?.latestProposal ?? null) : data + + return { data: freshest, isLoading, error, propose } } diff --git a/frontend/src/i18n/en/settings.ts b/frontend/src/i18n/en/settings.ts index fdd6959..711ddcf 100644 --- a/frontend/src/i18n/en/settings.ts +++ b/frontend/src/i18n/en/settings.ts @@ -83,6 +83,12 @@ export const settings = { 'settings.preschoolPlanningDaysLabel': 'Preschool planning days per year', 'settings.profileHelp': 'Your salary type and employment status are used to estimate your SGI and plan your leave.', 'settings.profileSectionTitle': 'Your profile', + 'settings.recomputeButton': 'Recompute plan', + 'settings.recomputeEvicted': 'The recompute would have to remove already-planned days to fit everything — nothing was saved. Adjust balances or wishes and try again.', + 'settings.recomputeHelp': 'The plan normally recomputes automatically whenever you change something. Use this button to recompute it right away, without waiting.', + 'settings.recomputeSuccess': 'The plan was recomputed.', + 'settings.recomputeTimeout': 'Recomputing the plan took too long. Try again in a little while.', + 'settings.recomputing': 'Recomputing…', 'settings.redDaysHelp': 'A list of public holidays under the Swedish calendar, together with your own exceptions — four years at a time.', 'settings.removeBlockerLeaveWishes': 'leave wishes', 'settings.removeBlockerParentalLeave': 'recorded parental leave', diff --git a/frontend/src/i18n/sv/settings.ts b/frontend/src/i18n/sv/settings.ts index 25b8efd..59ffa53 100644 --- a/frontend/src/i18n/sv/settings.ts +++ b/frontend/src/i18n/sv/settings.ts @@ -83,6 +83,12 @@ export const settings = { 'settings.preschoolPlanningDaysLabel': 'Förskolans planeringsdagar per år', 'settings.profileHelp': 'Din lönetyp och sysselsättning används för att uppskatta din SGI och planera din ledighet.', 'settings.profileSectionTitle': 'Din profil', + 'settings.recomputeButton': 'Räkna om planen', + 'settings.recomputeEvicted': 'Omräkningen skulle ta bort redan inplanerade dagar för att få plats — inget sparades. Justera saldon eller önskemål och försök igen.', + 'settings.recomputeHelp': 'Planen räknas normalt om automatiskt när du ändrar något. Använd den här knappen för att räkna om den direkt, utan att vänta.', + 'settings.recomputeSuccess': 'Planen räknades om.', + 'settings.recomputeTimeout': 'Det tog för lång tid att räkna om planen. Försök igen om en stund.', + 'settings.recomputing': 'Räknar om…', 'settings.redDaysHelp': 'En lista över röda dagar enligt den svenska kalendern, tillsammans med dina egna undantag — fyra år i taget.', 'settings.removeBlockerLeaveWishes': 'önskemål om ledighet', 'settings.removeBlockerParentalLeave': 'registrerad föräldrapenning', diff --git a/frontend/src/pages/Dashboard.test.tsx b/frontend/src/pages/Dashboard.test.tsx index ee10ff5..43d62ee 100644 --- a/frontend/src/pages/Dashboard.test.tsx +++ b/frontend/src/pages/Dashboard.test.tsx @@ -510,6 +510,35 @@ describe('Dashboard', () => { expect(screen.queryByTestId('dashboard-warnings')).not.toBeInTheDocument() }) + it('a vacation_expiring warning disappears once a planVersion bump\'s sync returns a proposal with no warnings', () => { + // `usePlanProposal` is mocked wholesale here (see the top of this + // file) — this simulates what it looks like from Dashboard's side + // once `PlanSyncProvider`'s background sync (triggered elsewhere by + // a `bumpPlanVersion()`, e.g. Settings' new recompute button or a + // partner add/remove) completes and `usePlanProposal` starts + // reflecting its fresher proposal instead of the stale one. + vi.mocked(hooks.usePlanProposal).mockReturnValue( + makePlanProposalResult({ + data: makePlanProposal({ + warnings: [makePlanWarning({ kind: 'vacation_expiring', messageKey: 'planWarnings.vacation_expiring', data: {} })], + }), + }), + ) + const { rerender } = renderWithRouter() + expect(screen.getByTestId('dashboard-warnings')).toBeInTheDocument() + + vi.mocked(hooks.usePlanProposal).mockReturnValue( + makePlanProposalResult({ data: makePlanProposal({ warnings: [] }) }), + ) + rerender( + + + , + ) + + expect(screen.queryByTestId('dashboard-warnings')).not.toBeInTheDocument() + }) + it('renders each warning above the tips section, with the parent name resolved', () => { vi.mocked(hooks.useOptimizationTips).mockReturnValue( makeOptimizationTipsResult({ data: [makeTip({ id: 'tip-1', messageKey: 'tips.use_vacation_before_expiry' })] }), diff --git a/frontend/src/pages/Dashboard.tsx b/frontend/src/pages/Dashboard.tsx index 26a2391..b15071a 100644 --- a/frontend/src/pages/Dashboard.tsx +++ b/frontend/src/pages/Dashboard.tsx @@ -76,8 +76,24 @@ export function Dashboard(): ReactNode { // absent year, which is the correct fallback either way. const { data: preschoolPlanningDays } = usePreschoolPlanningDaysList(selectedChildId ?? null) const { data: members, isLoading: membersLoading, refresh: refreshMembers } = useWorkspaceMembers(workspace?.id ?? null) + // Unlike `usePlanProposal` below, `useOptimizationTips` still only fetches + // once (no `planVersion` dependency) — a plan-changing write elsewhere + // (Settings, a bumped partner add/remove, PlanSyncProvider's own + // background sync) never refreshes the tips shown here. Same staleness + // shape as the bug `usePlanProposal` was just fixed for; left alone here, + // deliberately out of scope for this change. const { data: tips, dismiss: dismissTip } = useOptimizationTips(workspace?.id ?? null) const { data: wishes, isLoading: wishesLoading } = useLeaveWishes(workspace?.id ?? null) + // `proposal` (`data` from `usePlanProposal`) is already the freshest + // proposal available — that hook itself prefers `PlanSyncProvider`'s + // latest synced proposal over its own one-shot `propose()` result once a + // sync has completed (see `usePlanProposal.ts`'s doc comment). No + // `planVersion` dependency is needed *here* the way `useAnalytics.ts` + // has one: `usePlanProposal` calls `usePlanSyncContextOptional()` + // internally, a React Context read, so this component re-renders + // whenever `PlanSyncProvider`'s state changes — the same plumbing + // `planVersion` deps elsewhere rely on, just via Context instead of a + // dependency array. const { data: proposal, isLoading: isProposing, error: proposalError, propose } = usePlanProposal(workspace?.id ?? null) const orderedMembers = orderSelfFirst(members, personalDataUuid) @@ -103,10 +119,12 @@ export function Dashboard(): ReactNode { wasAwaitingFirstChildRef.current = children.length === 0 }, [children.length, refreshMembers]) - // The plan proposal (`POST /api/plan/propose`, read-only — persists - // nothing) is the only source for wished-vs-funded per year and the - // surplus advice, so it is requested automatically once wishes exist. - // Stops after one failure rather than retrying on every render. + // Initial-load-only fetch: this is what supplies `proposal` before + // `PlanSyncProvider`'s own background sync has completed even once (e.g. + // right after login). Once that first sync lands, `usePlanProposal` + // switches to its result and this effect's own `propose()` calls stop + // mattering — see `usePlanProposal.ts`. `proposalError === null` still + // guards it so a failed `propose()` never becomes a per-render retry loop. useEffect(() => { if (!wishesLoading && wishes.length > 0 && proposal === null && !isProposing && proposalError === null) { void propose() @@ -231,6 +249,12 @@ export function Dashboard(): ReactNode { data: leaves[member.uuid] ?? null, })) + // `proposal` (and therefore these, and `outcomes` passed to `YearRows` + // below) is always the freshest one available — see the comment by + // `usePlanProposal(...)` above. A partner add/remove, a Settings save, or + // the new manual recompute button (Settings.tsx) all eventually bump + // `planVersion`, which `PlanSyncProvider` turns into a background sync, + // which lands here without this component doing anything sync-specific. const surplusAdvice = proposal?.surplusAdvice ?? [] const warnings = proposal?.warnings ?? [] const hasTips = (tips?.length ?? 0) > 0 || surplusAdvice.length > 0 diff --git a/frontend/src/pages/Settings.test.tsx b/frontend/src/pages/Settings.test.tsx index 722e1d0..0f6bd8c 100644 --- a/frontend/src/pages/Settings.test.tsx +++ b/frontend/src/pages/Settings.test.tsx @@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import { render, screen, fireEvent, waitFor, act, within } from '@testing-library/react' import { Settings } from './Settings' import * as hooks from '@/hooks' -import { useWorkspace } from '@/context' +import { useWorkspace, usePlanSyncContextOptional } from '@/context' import * as api from '@/services/api' import { ApiRequestError } from '@/services/errors' import { LanguageProvider } from '@/i18n' @@ -22,6 +22,8 @@ import { makeAge4Projection, makeParentYearBalance, makeVacationYearBalance, + makePlanSyncContextValue, + makeEvictionPreview, } from '../../tests/fixtures' vi.mock('@/hooks') @@ -169,6 +171,10 @@ describe('Settings', () => { bumpPlanVersion: mockBumpPlanVersion, }) ) + // Default: no sync in flight / completed yet — the recompute button's + // own tests override this to simulate `PlanSyncProvider`'s background + // sync landing. + vi.mocked(usePlanSyncContextOptional).mockReturnValue(makePlanSyncContextValue()) // Default: solo workspace (only the logged-in user) — the add-partner // form should be visible. Tests that need a linked partner override this. vi.mocked(hooks.useWorkspaceMembers).mockReturnValue( @@ -515,6 +521,24 @@ describe('Settings', () => { await waitFor(() => expect(mockRefreshMembers).toHaveBeenCalled()) }) + it('bumps planVersion after adding a partner, triggering a sync (a new partner changes who the allocator can assign days to)', async () => { + renderSettings() + fireEvent.change(screen.getByPlaceholderText('Partnerns namn'), { target: { value: 'Partner Name' } }) + fireEvent.click(screen.getByTestId('add-partner-button')) + + await waitFor(() => expect(mockBumpPlanVersion).toHaveBeenCalledTimes(1)) + }) + + it('does not bump planVersion when adding a partner fails', async () => { + vi.mocked(api.addPartner).mockRejectedValue(new Error('boom')) + renderSettings() + fireEvent.change(screen.getByPlaceholderText('Partnerns namn'), { target: { value: 'Partner Name' } }) + fireEvent.click(screen.getByTestId('add-partner-button')) + + await screen.findByTestId('save-message') + expect(mockBumpPlanVersion).not.toHaveBeenCalled() + }) + it('shows an add-partner error without a raw message_key when the name is rejected', async () => { vi.mocked(api.addPartner).mockRejectedValue( new ApiRequestError(422, { @@ -626,6 +650,34 @@ describe('Settings', () => { await waitFor(() => expect(mockRefreshMembers).toHaveBeenCalled()) }) + it('bumps planVersion after removing a partner, triggering a sync (fewer parents changes who the allocator can assign days to)', async () => { + renderSettings() + fireEvent.click(screen.getByTestId('remove-button-pd-2')) + fireEvent.click(screen.getByTestId('remove-confirm-pd-2')) + + await waitFor(() => expect(mockBumpPlanVersion).toHaveBeenCalledTimes(1)) + }) + + it('does not bump planVersion when removing a partner fails', async () => { + vi.mocked(api.removeWorkspaceParent).mockRejectedValue(new Error('boom')) + renderSettings() + fireEvent.click(screen.getByTestId('remove-button-pd-2')) + fireEvent.click(screen.getByTestId('remove-confirm-pd-2')) + + await screen.findByTestId('member-action-error-pd-2') + expect(mockBumpPlanVersion).not.toHaveBeenCalled() + }) + + it('does not bump planVersion for a rename — cosmetic, nothing for a re-plan to fix', async () => { + renderSettings() + fireEvent.click(screen.getByTestId('rename-button-pd-2')) + fireEvent.change(screen.getByTestId('rename-input-pd-2'), { target: { value: 'Samuel' } }) + fireEvent.click(screen.getByTestId('rename-save-pd-2')) + + await waitFor(() => expect(mockRefreshMembers).toHaveBeenCalled()) + expect(mockBumpPlanVersion).not.toHaveBeenCalled() + }) + it('shows the has-login refusal without a raw message_key', async () => { vi.mocked(api.removeWorkspaceParent).mockRejectedValue( new ApiRequestError(422, { @@ -883,6 +935,69 @@ describe('Settings', () => { }) }) + describe('manual recompute button', () => { + it('bumps planVersion (never calling api.syncPlan directly) and shows a spinner label while waiting', () => { + renderSettings() + + fireEvent.click(screen.getByTestId('recompute-plan-button')) + + expect(mockBumpPlanVersion).toHaveBeenCalledTimes(1) + expect(api.syncPlan).not.toHaveBeenCalled() + expect(screen.getByTestId('recompute-plan-button')).toHaveTextContent('Räknar om…') + expect(screen.getByTestId('recompute-plan-button')).toBeDisabled() + }) + + it('reports success once PlanSyncProvider\'s triggered sync lands with a newer proposal', async () => { + const { rerender } = renderSettings() + + fireEvent.click(screen.getByTestId('recompute-plan-button')) + + // Simulates `PlanSyncProvider`'s background sync (triggered by the + // `bumpPlanVersion()` call above) completing: its context value now + // carries a newer `latestProposalVersion` and no eviction. + vi.mocked(usePlanSyncContextOptional).mockReturnValue( + makePlanSyncContextValue({ latestProposalVersion: 1, pendingEvictionPreview: null }), + ) + rerender() + + const message = await screen.findByTestId('save-message') + expect(message).toHaveTextContent('Planen räknades om.') + expect(screen.getByTestId('recompute-plan-button')).not.toBeDisabled() + expect(screen.getByTestId('recompute-plan-button')).toHaveTextContent('Räkna om planen') + }) + + it('reports that nothing was saved when the triggered sync would evict', async () => { + const { rerender } = renderSettings() + + fireEvent.click(screen.getByTestId('recompute-plan-button')) + + vi.mocked(usePlanSyncContextOptional).mockReturnValue( + makePlanSyncContextValue({ + latestProposalVersion: 1, + pendingEvictionPreview: makeEvictionPreview({ wouldEvict: true }), + }), + ) + rerender() + + const message = await screen.findByTestId('save-message') + expect(message).toHaveTextContent('Omräkningen skulle ta bort redan inplanerade dagar') + }) + + it('ignores a version bump/eviction from an unrelated sync while idle', () => { + const { rerender } = renderSettings() + + // No recompute click happened — `latestProposalVersion` moving on its + // own (e.g. a vacation-day autosave elsewhere bumped `planVersion`) + // must not surface a message this button never triggered. + vi.mocked(usePlanSyncContextOptional).mockReturnValue( + makePlanSyncContextValue({ latestProposalVersion: 1, pendingEvictionPreview: null }), + ) + rerender() + + expect(screen.queryByTestId('save-message')).not.toBeInTheDocument() + }) + }) + describe('Undantag & röda dagar panel — CalendarExceptions wiring', () => { it('passes the fetched calendar window and override handlers through', () => { renderSettings() diff --git a/frontend/src/pages/Settings.tsx b/frontend/src/pages/Settings.tsx index 974552b..cddd81e 100644 --- a/frontend/src/pages/Settings.tsx +++ b/frontend/src/pages/Settings.tsx @@ -1,5 +1,5 @@ import { ReactNode, useEffect, useMemo, useRef, useState } from 'react' -import { useWorkspace } from '@/context' +import { useWorkspace, usePlanSyncContextOptional } from '@/context' import { useVacationDays, useShowWeekNumbers, @@ -108,6 +108,13 @@ function computeVacationYearRange(birthDate: string | undefined, currentYear: nu const PRESCHOOL_AUTOSAVE_DELAY_MS = 800 +// How long the recompute button (Kalender & språk panel) waits for +// `PlanSyncProvider`'s triggered sync to land before giving up and +// reporting a failure — a sync that errors instead of resolving is only +// ever logged (see `PlanSyncProvider.tsx`), so without a ceiling here the +// button would spin forever on that path. +const RECOMPUTE_TIMEOUT_MS = 10_000 + /** * One (child, year) row letting the user record the true number of * preschool planning days actually taken that year — the child-level @@ -258,12 +265,58 @@ export function Settings(): ReactNode { // and vice versa. const [isSavingFamily, setIsSavingFamily] = useState(false) const [isSavingRedDayPref, setIsSavingRedDayPref] = useState(false) + const [isRecomputing, setIsRecomputing] = useState(false) const [newPartnerName, setNewPartnerName] = useState('') const [renamingUuid, setRenamingUuid] = useState(null) const [renameValue, setRenameValue] = useState('') const [removeConfirmUuid, setRemoveConfirmUuid] = useState(null) const [memberActionError, setMemberActionError] = useState<{ uuid: string; text: string } | null>(null) + // Optional: in the real app `Settings` is always mounted inside + // `WorkspaceProvider` (and therefore `PlanSyncProvider`), but this page's + // own test suite renders it with `@/context` auto-mocked wholesale — the + // optional variant keeps the recompute button below testable without + // requiring a real `PlanSyncProvider` in every existing test. See + // `PlanSyncProvider.tsx`. + const planSyncContext = usePlanSyncContextOptional() + const latestProposalVersion = planSyncContext?.latestProposalVersion ?? 0 + const pendingEvictionPreview = planSyncContext?.pendingEvictionPreview ?? null + // The `latestProposalVersion` the recompute button's own triggered sync + // should land past — `null` while no recompute is in flight, so a sync + // started by something else entirely (e.g. a vacation-day autosave + // elsewhere on this same page) never flips this button's spinner off. + const recomputeBaselineRef = useRef(null) + const recomputeTimeoutRef = useRef | null>(null) + + // Detects the recompute button's own triggered sync landing. + // `bumpPlanVersion()` (in `handleRecompute` below) doesn't itself return + // anything to await — the sync it causes runs inside `PlanSyncProvider`, + // entirely outside this component. `latestProposalVersion` moving past + // the baseline captured when the button was clicked is this component's + // only signal that *that* sync (as opposed to some unrelated one) has + // completed. A `setTimeout` fallback covers a sync that errors instead of + // resolving (`PlanSyncProvider` only logs that case, see its doc comment) + // — without it a failed sync would leave this button spinning forever. + useEffect(() => { + if (recomputeBaselineRef.current === null) return + if (latestProposalVersion <= recomputeBaselineRef.current) return + recomputeBaselineRef.current = null + if (recomputeTimeoutRef.current !== null) { + clearTimeout(recomputeTimeoutRef.current) + recomputeTimeoutRef.current = null + } + setIsRecomputing(false) + if (pendingEvictionPreview) { + // `wouldEvict === true` means the sync this button triggered wrote + // NOTHING (see `EvictionPreview`'s doc comment) — never report that + // as a success. + setSaveMessage({ type: 'error', text: t('settings.recomputeEvicted') }) + } else { + setSaveMessage({ type: 'success', text: t('settings.recomputeSuccess') }) + } + setTimeout(() => setSaveMessage(null), 3000) + }, [latestProposalVersion, pendingEvictionPreview, t]) + // Settings isn't scoped to a single child the way Onboarding/Dashboard are // — the balance flow needs somewhere to attach a saved balance even before // a child is chosen/created elsewhere in the app. @@ -447,6 +500,10 @@ export function Settings(): ReactNode { setSaveMessage(null) await api.addPartner(workspace.id, newPartnerName) await refreshMembers() + // A new partner changes which parent days the allocator can assign + // leave to — re-type the rest of the plan's unpinned days. See + // `WorkspaceContext.planVersion`. + bumpPlanVersion() setNewPartnerName('') showMessage('success', t('settings.addPartnerSuccess')) } catch (err) { @@ -481,6 +538,9 @@ export function Settings(): ReactNode { if (uuid === personalDataUuid) { await refreshContext() } + // No `bumpPlanVersion()` here, unlike add/remove below — a rename is + // cosmetic and doesn't change which days the allocator can assign to + // which parent, so there's nothing for a re-plan to fix. setRenamingUuid(null) setRenameValue('') } catch (err) { @@ -508,6 +568,10 @@ export function Settings(): ReactNode { await api.removeWorkspaceParent(workspace.id, uuid) setRemoveConfirmUuid(null) await refreshMembers() + // Removing a partner changes which parent days the allocator can + // assign leave to — re-type the rest of the plan's unpinned days. + // See `WorkspaceContext.planVersion`. + bumpPlanVersion() } catch (err) { setMemberActionError({ uuid, text: describePartnerError(err, t, t('settings.removePartnerError')) }) } finally { @@ -532,6 +596,28 @@ export function Settings(): ReactNode { } } + // Manual recompute: bumping `planVersion` is the whole implementation — + // `PlanSyncProvider` (mounted app-wide) turns the bump into a + // `POST /api/plan/sync` itself. Calling `api.syncPlan` directly here + // instead would bypass that provider's burst-coalescing and its + // `pendingEvictionPreview` handling. See the effect above for how the + // outcome (success vs. the sync writing nothing) gets reported. + const handleRecompute = async () => { + recomputeBaselineRef.current = latestProposalVersion + setIsRecomputing(true) + setSaveMessage(null) + if (recomputeTimeoutRef.current !== null) { + clearTimeout(recomputeTimeoutRef.current) + } + recomputeTimeoutRef.current = setTimeout(() => { + recomputeBaselineRef.current = null + recomputeTimeoutRef.current = null + setIsRecomputing(false) + showMessage('error', t('settings.recomputeTimeout')) + }, RECOMPUTE_TIMEOUT_MS) + bumpPlanVersion() + } + return (
@@ -819,6 +905,19 @@ export function Settings(): ReactNode { + +

{t('settings.recomputeHelp')}

+
+ +
diff --git a/frontend/tests/fixtures/hooks.ts b/frontend/tests/fixtures/hooks.ts index d6d12c2..f5a16d2 100644 --- a/frontend/tests/fixtures/hooks.ts +++ b/frontend/tests/fixtures/hooks.ts @@ -12,6 +12,7 @@ import { vi } from 'vitest' import type { useWorkspace } from '@/context/WorkspaceContext' import type { useAuth } from '@/context/AuthContext' +import type { usePlanSyncContext } from '@/context/PlanSyncProvider' import type { useAnalytics } from '@/hooks/useAnalytics' import type { useParentalLeave } from '@/hooks/useParentalLeave' import type { usePlannedDays } from '@/hooks/usePlannedDays' @@ -63,6 +64,18 @@ export function makeWorkspaceContextValue( } } +export function makePlanSyncContextValue( + overrides: Partial> = {}, +): ReturnType { + return { + pendingEvictionPreview: null, + clearPendingEvictionPreview: vi.fn(), + latestProposal: null, + latestProposalVersion: 0, + ...overrides, + } +} + export function makeAuthContextValue( overrides: Partial> = {}, ): ReturnType {