From fdbfb7d58061eb9c751b90728a31322559ff25c9 Mon Sep 17 00:00:00 2001 From: Jacob Hagstedt Date: Thu, 17 Sep 2026 22:32:03 +0200 Subject: [PATCH] Give tips a date, a scope per child, and a dismissal that sticks Three faults, all invisible while the tests passed. Advice was built for the first child only, so a second child got none. The planned days feeding every count were fetched once per parent and appended to one list, so a couple's days were counted twice and every intensity and projection derived from them was wrong. And dismissing a tip forgot the moment the page reloaded. Tips now carry the date they concern, lifted from figures the rules already computed, so a view of a month can show only what applies to it. They are built per child and identified per child, so the same rule firing for two children is two separate pieces of advice. A dismissal is stored against a fingerprint of the facts that were dismissed. When those facts change the advice returns with its new numbers, rather than one click silencing a warning about a forfeiture that keeps growing. --- backend/app/db/connection.py | 10 + backend/app/dependencies.py | 37 +++ backend/app/entities/tip_dismissal.py | 30 ++ backend/app/models/tips.py | 43 +++ .../repositories/tip_dismissal_repository.py | 21 ++ .../sqlite/tip_dismissal_repository.py | 101 ++++++ backend/app/routers/settings.py | 43 ++- backend/app/services/tips_service.py | 312 ++++++++++++++++-- .../versions/010_add_tip_dismissal.py | 59 ++++ backend/tests/conftest.py | 37 +++ .../test_tips_service_context.py | 305 +++++++++++++++++ .../unit/test_tip_dismissal_repository.py | 123 +++++++ .../tests/unit/test_tip_predicates_hash.py | 30 ++ backend/tests/unit/test_tips_router.py | 159 +++++++++ .../src/hooks/useOptimizationTips.test.ts | 40 ++- frontend/src/hooks/useOptimizationTips.ts | 23 +- frontend/src/services/api.test.ts | 62 ++++ frontend/src/services/api.ts | 11 + frontend/src/types/entities.ts | 5 + frontend/tests/fixtures/mockData.ts | 2 + 20 files changed, 1396 insertions(+), 57 deletions(-) create mode 100644 backend/app/entities/tip_dismissal.py create mode 100644 backend/app/models/tips.py create mode 100644 backend/app/repositories/tip_dismissal_repository.py create mode 100644 backend/app/repositories_impl/sqlite/tip_dismissal_repository.py create mode 100644 backend/migrations/versions/010_add_tip_dismissal.py create mode 100644 backend/tests/unit/test_services/test_tips_service_context.py create mode 100644 backend/tests/unit/test_tip_dismissal_repository.py create mode 100644 backend/tests/unit/test_tip_predicates_hash.py create mode 100644 backend/tests/unit/test_tips_router.py diff --git a/backend/app/db/connection.py b/backend/app/db/connection.py index 0a76ecf..acc4dd8 100644 --- a/backend/app/db/connection.py +++ b/backend/app/db/connection.py @@ -173,6 +173,16 @@ CREATE TABLE IF NOT EXISTS day_override ( UNIQUE(workspace_id, date), FOREIGN KEY (workspace_id) REFERENCES workspace(id) ON DELETE CASCADE ); + +CREATE TABLE IF NOT EXISTS tip_dismissal ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL, + tip_id TEXT NOT NULL, + data_hash TEXT NOT NULL, + dismissed_at TIMESTAMP NOT NULL, + UNIQUE(workspace_id, tip_id), + FOREIGN KEY (workspace_id) REFERENCES workspace(id) ON DELETE CASCADE +); """ diff --git a/backend/app/dependencies.py b/backend/app/dependencies.py index ec39373..b7a12c2 100644 --- a/backend/app/dependencies.py +++ b/backend/app/dependencies.py @@ -24,6 +24,7 @@ from app.repositories.leave_wish_repository import LeaveWishRepository from app.repositories.parental_leave_repository import ParentalLeaveRepository from app.repositories.personal_data_repository import PersonalDataRepository from app.repositories.planned_leave_repository import PlannedLeaveRepository +from app.repositories.tip_dismissal_repository import TipDismissalRepository from app.repositories.user_repository import UserRepository from app.repositories.vacation_days_repository import VacationDaysRepository from app.repositories.workspace_repository import ( @@ -36,6 +37,7 @@ from app.repositories_impl.sqlite.leave_wish_repository import SQLiteLeaveWishRe from app.repositories_impl.sqlite.parental_leave_repository import SQLiteParentalLeaveRepository from app.repositories_impl.sqlite.personal_data_repository import SQLitePersonalDataRepository from app.repositories_impl.sqlite.planned_leave_repository import SQLitePlannedLeaveRepository +from app.repositories_impl.sqlite.tip_dismissal_repository import SQLiteTipDismissalRepository from app.repositories_impl.sqlite.user_repository import SQLiteUserRepository from app.repositories_impl.sqlite.vacation_days_repository import SQLiteVacationDaysRepository from app.repositories_impl.sqlite.workspace_repository import ( @@ -51,6 +53,7 @@ from app.services.day_override_service import DayOverrideService from app.services.leave_wish_service import LeaveWishService from app.services.personal_data_service import PersonalDataService from app.services.planning_service import PlanningService +from app.services.tips_service import TipsService from app.services.vacation_days_service import VacationDaysService from app.services.workspace_service import WorkspaceService from app.utils.errors import UnauthorizedError @@ -175,6 +178,15 @@ def get_day_override_repository(conn: ConnectionDep) -> DayOverrideRepository: DayOverrideRepositoryDep = Annotated[DayOverrideRepository, Depends(get_day_override_repository)] +def get_tip_dismissal_repository(conn: ConnectionDep) -> TipDismissalRepository: + if settings.database_type == "sqlite": + return SQLiteTipDismissalRepository(conn) + raise ValueError(f"Unsupported database_type: {settings.database_type}") + + +TipDismissalRepositoryDep = Annotated[TipDismissalRepository, Depends(get_tip_dismissal_repository)] + + # --------------------------------------------------------------------------- # Service factories # --------------------------------------------------------------------------- @@ -219,6 +231,31 @@ def get_calendar_service(day_override_service: DayOverrideServiceDep) -> Calenda CalendarServiceDep = Annotated[CalendarService, Depends(get_calendar_service)] +def get_tips_service( + leave_repo: ParentalLeaveRepositoryDep, + planned_repo: PlannedLeaveRepositoryDep, + child_repo: ChildRepositoryDep, + pd_repo: PersonalDataRepositoryDep, + vacation_repo: VacationDaysRepositoryDep, + leave_wish_repo: LeaveWishRepositoryDep, + ws_repo: WorkspaceRepositoryDep, + dismissal_repo: TipDismissalRepositoryDep, +) -> TipsService: + return TipsService( + leave_repo, + planned_repo, + child_repo, + pd_repo, + vacation_repo, + leave_wish_repo, + ws_repo, + dismissal_repo, + ) + + +TipsServiceDep = Annotated[TipsService, Depends(get_tips_service)] + + def get_child_service(repo: ChildRepositoryDep) -> ChildService: return ChildService(repo) diff --git a/backend/app/entities/tip_dismissal.py b/backend/app/entities/tip_dismissal.py new file mode 100644 index 0000000..9ec6b50 --- /dev/null +++ b/backend/app/entities/tip_dismissal.py @@ -0,0 +1,30 @@ +"""Domain entity: TipDismissal. + +A workspace-scoped record that a specific tip (`Tip.id`, see +`app.services.tips_service`) has been acknowledged/dismissed by the user. +Keyed by (workspace_id, tip_id) — see the UNIQUE constraint on the +`tip_dismissal` table — so re-dismissing the same tip refreshes `data_hash` +and `dismissed_at` instead of creating a duplicate row. + +`data_hash` snapshots `app.utils.tip_predicates.hash_tip_data(tip.data)` at +dismiss time. `TipsService` only treats a dismissal as active while the +rule's *current* output hashes to the same value — once the underlying +numbers move (more days now projected to expire at age 4, a different +`fourth_birthday`, ...), the dismissal is stale and the tip resurfaces. See +`TipsService._filter_dismissed` for why: permanently silencing a warning like +`preserve_for_age_4` because its *old* figures were once dismissed would be +dangerous. +""" + +from dataclasses import dataclass +from datetime import datetime +from uuid import UUID + + +@dataclass +class TipDismissal: + id: UUID + workspace_id: UUID + tip_id: str + data_hash: str + dismissed_at: datetime | None = None diff --git a/backend/app/models/tips.py b/backend/app/models/tips.py new file mode 100644 index 0000000..9a009a5 --- /dev/null +++ b/backend/app/models/tips.py @@ -0,0 +1,43 @@ +"""API schemas (Pydantic) for the tips engine. + +The route used to return `list[dict]` — no shape guarantee at all. `Tip.data` +stays a free-form bag on purpose (§ tips_service.py docstring: it is +interpolation context for the frontend, and every rule shapes it +differently), but everything around it is now a real, typed contract. +""" + +from datetime import date + +from pydantic import BaseModel + +from app.services.tips_service import Tip + +# Every value any rule currently puts in `Tip.data` is a JSON scalar (str, +# int, float, bool) or None — never a nested dict/list. Typing it this way +# (rather than `dict[str, Any]`) still catches a rule accidentally stuffing +# something un-renderable into `data`. +TipDataValue = str | int | float | bool | None + + +class TipResponse(BaseModel): + id: str + severity: str + message_key: str + data: dict[str, TipDataValue] + anchor_date: date | None + anchor_range: tuple[date, date] | None + + @classmethod + def from_domain(cls, tip: Tip) -> "TipResponse": + return cls( + id=tip.id, + severity=tip.severity, + message_key=tip.message_key, + data=tip.data, + anchor_date=tip.anchor_date, + anchor_range=tip.anchor_range, + ) + + +class TipDismissalCreate(BaseModel): + tip_id: str diff --git a/backend/app/repositories/tip_dismissal_repository.py b/backend/app/repositories/tip_dismissal_repository.py new file mode 100644 index 0000000..a6cb9cd --- /dev/null +++ b/backend/app/repositories/tip_dismissal_repository.py @@ -0,0 +1,21 @@ +"""Abstract repository interface for TipDismissal. + +Adds entity-specific queries on top of the generic CRUD contract. Concrete +implementations live in app/repositories_impl//. +""" + +from abc import abstractmethod +from uuid import UUID + +from app.entities.tip_dismissal import TipDismissal +from app.repositories.base import Repository + + +class TipDismissalRepository(Repository[TipDismissal]): + @abstractmethod + async def get_by_workspace_tip(self, workspace_id: UUID, tip_id: str) -> TipDismissal | None: + """Return the dismissal for a given workspace + tip id (UNIQUE), or None.""" + + @abstractmethod + async def list_by_workspace(self, workspace_id: UUID) -> list[TipDismissal]: + """Return all dismissals for a workspace.""" diff --git a/backend/app/repositories_impl/sqlite/tip_dismissal_repository.py b/backend/app/repositories_impl/sqlite/tip_dismissal_repository.py new file mode 100644 index 0000000..575f0ba --- /dev/null +++ b/backend/app/repositories_impl/sqlite/tip_dismissal_repository.py @@ -0,0 +1,101 @@ +"""SQLite implementation of TipDismissalRepository. + +Follows the DayOverrideRepository reference shape: upsert-by-natural-key +((workspace_id, tip_id) is UNIQUE — see the `tip_dismissal` table), map rows +<-> entities explicitly in `_row_to_entity`, always parameterize queries, +raise NotFoundError from update/delete when the row is missing. +""" + +from datetime import datetime +from uuid import UUID + +import aiosqlite + +from app.entities.tip_dismissal import TipDismissal +from app.repositories.filter_validator import validate_filters +from app.repositories.tip_dismissal_repository import TipDismissalRepository +from app.utils.errors import NotFoundError + + +class SQLiteTipDismissalRepository(TipDismissalRepository): + def __init__(self, connection: aiosqlite.Connection) -> None: + self._conn = connection + + @staticmethod + def _row_to_entity(row: aiosqlite.Row) -> TipDismissal: + return TipDismissal( + id=UUID(row["id"]), + workspace_id=UUID(row["workspace_id"]), + tip_id=row["tip_id"], + data_hash=row["data_hash"], + dismissed_at=datetime.fromisoformat(row["dismissed_at"]), + ) + + async def create(self, item: TipDismissal) -> TipDismissal: + item.dismissed_at = item.dismissed_at or datetime.now() + await self._conn.execute( + """ + INSERT INTO tip_dismissal (id, workspace_id, tip_id, data_hash, dismissed_at) + VALUES (?, ?, ?, ?, ?) + """, + ( + str(item.id), + str(item.workspace_id), + item.tip_id, + item.data_hash, + item.dismissed_at.isoformat(), + ), + ) + await self._conn.commit() + return item + + async def get_by_id(self, id: UUID) -> TipDismissal | None: + cursor = await self._conn.execute("SELECT * FROM tip_dismissal WHERE id = ?", (str(id),)) + row = await cursor.fetchone() + return self._row_to_entity(row) if row else None + + async def get_by_workspace_tip(self, workspace_id: UUID, tip_id: str) -> TipDismissal | None: + cursor = await self._conn.execute( + "SELECT * FROM tip_dismissal WHERE workspace_id = ? AND tip_id = ?", + (str(workspace_id), tip_id), + ) + row = await cursor.fetchone() + return self._row_to_entity(row) if row else None + + async def update(self, id: UUID, item: TipDismissal) -> TipDismissal: + item.dismissed_at = datetime.now() + cursor = await self._conn.execute( + "UPDATE tip_dismissal SET data_hash = ?, dismissed_at = ? WHERE id = ?", + (item.data_hash, item.dismissed_at.isoformat(), str(id)), + ) + await self._conn.commit() + if cursor.rowcount == 0: + raise NotFoundError(f"TipDismissal {id} not found") + refreshed = await self.get_by_id(id) + assert refreshed is not None # just updated + return refreshed + + async def delete(self, id: UUID) -> None: + cursor = await self._conn.execute("DELETE FROM tip_dismissal WHERE id = ?", (str(id),)) + await self._conn.commit() + if cursor.rowcount == 0: + raise NotFoundError(f"TipDismissal {id} not found") + + async def list(self, **filters: object) -> list[TipDismissal]: + validate_filters(filters, {"workspace_id"}) + clauses: list[str] = [] + params: list[object] = [] + for column in ("workspace_id",): + if column in filters and filters[column] is not None: + value = filters[column] + clauses.append(f"{column} = ?") + params.append(str(value) if isinstance(value, UUID) else value) + where = f" WHERE {' AND '.join(clauses)}" if clauses else "" + cursor = await self._conn.execute( + f"SELECT * FROM tip_dismissal{where} ORDER BY dismissed_at ASC", params + ) + rows = await cursor.fetchall() + return [self._row_to_entity(row) for row in rows] + + async def list_by_workspace(self, workspace_id: UUID) -> list[TipDismissal]: + return await self.list(workspace_id=workspace_id) diff --git a/backend/app/routers/settings.py b/backend/app/routers/settings.py index b799cf3..8eb864a 100644 --- a/backend/app/routers/settings.py +++ b/backend/app/routers/settings.py @@ -13,20 +13,14 @@ from uuid import UUID from fastapi import APIRouter, HTTPException, status from app.dependencies import ( - ChildRepositoryDep, ChildServiceDep, CurrentUserDep, - LeaveWishRepositoryDep, - ParentalLeaveRepositoryDep, - PersonalDataRepositoryDep, PersonalDataServiceDep, - PlannedLeaveRepositoryDep, - VacationDaysRepositoryDep, - WorkspaceRepositoryDep, + TipsServiceDep, ) from app.models.child import ChildCreate, ChildResponse, ChildUpdate from app.models.personal_data import PersonalDataResponse, PersonalDataUpdate -from app.services.tips_service import TipsService +from app.models.tips import TipDismissalCreate, TipResponse from app.utils.errors import NotFoundError, ValidationError router = APIRouter(tags=["settings"]) @@ -39,26 +33,27 @@ router = APIRouter(tags=["settings"]) tips_router = APIRouter(prefix="/api/tips", tags=["tips"]) -@tips_router.get("/{workspace_id}", response_model=list[dict]) +@tips_router.get("/{workspace_id}", response_model=list[TipResponse]) async def get_tips( workspace_id: UUID, - leave_repo: ParentalLeaveRepositoryDep, - planned_repo: PlannedLeaveRepositoryDep, - child_repo: ChildRepositoryDep, - pd_repo: PersonalDataRepositoryDep, - vacation_repo: VacationDaysRepositoryDep, - leave_wish_repo: LeaveWishRepositoryDep, - ws_repo: WorkspaceRepositoryDep, + service: TipsServiceDep, _current_user: CurrentUserDep, -) -> list[dict]: - service = TipsService( - leave_repo, planned_repo, child_repo, pd_repo, vacation_repo, leave_wish_repo, ws_repo - ) +) -> list[TipResponse]: tips = await service.get_tips(workspace_id) - return [ - {"id": t.id, "severity": t.severity, "message_key": t.message_key, "data": t.data} - for t in tips - ] + return [TipResponse.from_domain(tip) for tip in tips] + + +@tips_router.post("/{workspace_id}/dismissals", status_code=status.HTTP_204_NO_CONTENT) +async def dismiss_tip( + workspace_id: UUID, + payload: TipDismissalCreate, + service: TipsServiceDep, + _current_user: CurrentUserDep, +) -> None: + try: + await service.dismiss(workspace_id, payload.tip_id) + except NotFoundError as exc: + raise HTTPException(status.HTTP_404_NOT_FOUND, str(exc)) from exc # --------------------------------------------------------------------------- diff --git a/backend/app/services/tips_service.py b/backend/app/services/tips_service.py index fc9ade8..10ae01d 100644 --- a/backend/app/services/tips_service.py +++ b/backend/app/services/tips_service.py @@ -22,7 +22,7 @@ from collections import defaultdict from dataclasses import dataclass from datetime import date, timedelta from typing import Callable, Coroutine, TypeAlias -from uuid import UUID +from uuid import UUID, uuid4 from dateutil.relativedelta import relativedelta as _relativedelta @@ -32,17 +32,22 @@ from app.entities.leave_wish import LeaveWish from app.entities.parental_leave import ParentalLeaveFetch from app.entities.personal_data import PersonalData from app.entities.planned_leave import PlannedLeaveDay +from app.entities.tip_dismissal import TipDismissal from app.entities.vacation_days import VacationDays from app.repositories.child_repository import ChildRepository from app.repositories.leave_wish_repository import LeaveWishRepository from app.repositories.parental_leave_repository import ParentalLeaveRepository from app.repositories.personal_data_repository import PersonalDataRepository from app.repositories.planned_leave_repository import PlannedLeaveRepository +from app.repositories.tip_dismissal_repository import TipDismissalRepository from app.repositories.vacation_days_repository import VacationDaysRepository from app.repositories.workspace_repository import WorkspaceRepository +from app.utils.errors import NotFoundError from app.utils.tip_predicates import ( + age4_pooling_loss, days_left_at_age_4_projection, has_vacation_near_expiry, + hash_tip_data, is_child_over_age, preschool_buffer_needed, red_day_clusters_in_range, @@ -58,15 +63,31 @@ class Tip: severity: str # 'info' | 'warning' | 'action' message_key: str # i18n key for frontend data: dict # context for frontend rendering (e.g. {days: 5, period: "Jul–Aug"}) + # Where this tip is anchored on a calendar: a single day (e.g. "use this + # vacation day"), a span (e.g. a break period), or neither. Lifted out of + # `data` — where the value already lived — rather than invented; see the + # individual rules for which `data` key each maps to. Rules with no + # meaningful date (balance_between_parents, lagsta_niva_coverage, + # stretch_time_at_home, sgi_minimum_after_age_1, + # reserve_for_preschool_planning_days) leave both null. + anchor_date: date | None = None + anchor_range: tuple[date, date] | None = None @dataclass class TipsContext: workspace_id: UUID + # Either empty (the workspace-level context — see `_WORKSPACE_SCOPED_RULES` + # in `TipsService`) or a single child (a per-child context, built once per + # child in `_build_context`). Never more than one: entitlement and + # planned days are tracked per child, so a rule scoped to "the" child + # would silently pick an arbitrary one in a multi-child workspace. children: list[Child] parents: list[PersonalData] leave_by_parent: dict[UUID, ParentalLeaveFetch | None] # personal_data_uuid → latest - planned_days: list[PlannedLeaveDay] # all future planned days for workspace + # Future planned days for `children[0]` only (empty for the workspace + # context) — see `_build_context`. + planned_days: list[PlannedLeaveDay] vacation_by_parent: dict[UUID, list[VacationDays]] leave_wishes: list[LeaveWish] today: date # injected for deterministic testing @@ -89,6 +110,11 @@ async def _rule_use_vacation_before_expiry(ctx: TipsContext) -> Tip | None: for parent in ctx.parents: for vd in ctx.vacation_by_parent.get(parent.uuid, []): if has_vacation_near_expiry(vd.total_days, vd.saved_days_in): + # `vd.year` is the only date-shaped fact this rule has; anchor + # to that vacation year's last day — the natural forcing point + # for "use it before it's gone" — rather than leaving the rail + # with nothing to place this tip on. + year_end = date(vd.year, 12, 31) return Tip( id="use_vacation_before_expiry", severity="warning", @@ -98,6 +124,7 @@ async def _rule_use_vacation_before_expiry(ctx: TipsContext) -> Tip | None: "saved_days": vd.saved_days_in, "year": vd.year, }, + anchor_date=year_end, ) return None @@ -159,10 +186,120 @@ async def _rule_preserve_for_age_4(ctx: TipsContext) -> Tip | None: "expiring_days": expiring_at_b4, "fourth_birthday": fourth_bday.isoformat(), }, + anchor_date=fourth_bday, ) return None +async def _rule_age4_pooling_loss(ctx: TipsContext) -> Tip | None: + """Warn a couple when one parent is over the per-parent age-4 cap while + the other has unused headroom — days that could have been pooled but + won't be, because `resolve_age4_savable_cap` splits the ceiling per + parent once a workspace is in `couple` mode (see `age4_pooling_loss`). + + Advice order is the substance of this tip: taking more leave before the + birthday converts entitlement into actual time at home (strictly + better), so it is offered first. Transferring days at Försäkringskassan + only preserves the entitlement — it is offered second, and the transfer + figure is bounded by what `ParentalLeaveFetch.transferable_days_left` + (not the raw days-left total, which includes non-transferable reserved + days) says is actually movable. When the partner has no FK snapshot the + exact transfer figure can't be known, so the tip degrades to a hedged + "if you both have days left" phrasing instead of guessing a number. + """ + if ctx.workspace_planning_mode != "couple" or not ctx.children: + return None + + child = ctx.children[0] + if not is_child_over_age(child.birth_date, settings.rules.age4_pooling_tip_min_child_age_years): + return None # first couple of years: too early to usefully act on this + + fourth_bday = child.birth_date + _relativedelta(years=4) + per_parent_cap = resolve_age4_savable_cap(ctx.workspace_planning_mode, len(ctx.parents)) + per_child_cap = settings.rules.max_savable_days_at_age_4 + + projected_left_by_parent: dict[UUID, int] = {} + excess_by_parent: dict[UUID, int] = {} + for parent in ctx.parents: + leave = ctx.leave_by_parent.get(parent.uuid) + if leave is None: + continue + planned_fp_before_b4 = sum( + 1 + for d in ctx.planned_days + if d.parent_personal_data_uuid == parent.uuid + and d.day_type in ("sjukpenning", "lagsta") + and d.date < fourth_bday + ) + projected_left, expiring = days_left_at_age_4_projection( + leave.days_used, planned_fp_before_b4, leave.total_days, per_parent_cap + ) + projected_left_by_parent[parent.uuid] = projected_left + excess_by_parent[parent.uuid] = expiring + + if not projected_left_by_parent: + return None + + family_loss = age4_pooling_loss( + list(projected_left_by_parent.values()), per_parent_cap, per_child_cap + ) + if family_loss <= 0: + return None + + surplus_uuid = max(excess_by_parent, key=lambda u: excess_by_parent[u]) + surplus_parent = next(p for p in ctx.parents if p.uuid == surplus_uuid) + surplus_leave = ctx.leave_by_parent[surplus_uuid] + excess_days = excess_by_parent[surplus_uuid] + + other_parents = [p for p in ctx.parents if p.uuid != surplus_uuid] + partner_has_snapshot = False + lagging_parent: PersonalData | None = None + lagging_headroom = 0 + for other in other_parents: + other_leave = ctx.leave_by_parent.get(other.uuid) + if other_leave is None: + continue + partner_has_snapshot = True + headroom = max(per_parent_cap - projected_left_by_parent[other.uuid], 0) + if headroom > lagging_headroom: + lagging_headroom = headroom + lagging_parent = other + + if lagging_parent is not None and lagging_headroom > 0: + transfer_days = min(excess_days, lagging_headroom, surplus_leave.transferable_days_left) + transfer_days = max(transfer_days, 0) + return Tip( + id="age4_pooling_loss", + severity="warning", + message_key="tips.age4_pooling_loss_exact", + data={ + "surplus_parent": surplus_parent.name, + "lagging_parent": lagging_parent.name, + "excess_days": excess_days, + "transfer_days": transfer_days, + "family_loss_days": family_loss, + "parent_cap": per_parent_cap, + "fourth_birthday": fourth_bday.isoformat(), + }, + ) + + if not partner_has_snapshot and other_parents: + return Tip( + id="age4_pooling_loss", + severity="warning", + message_key="tips.age4_pooling_loss_hedged", + data={ + "surplus_parent": surplus_parent.name, + "excess_days": excess_days, + "family_loss_days": family_loss, + "parent_cap": per_parent_cap, + "fourth_birthday": fourth_bday.isoformat(), + }, + ) + + return None + + async def _rule_use_dubbeldagar_strategically(ctx: TipsContext) -> Tip | None: if len(ctx.parents) < 2 or not ctx.children: return None @@ -189,6 +326,7 @@ async def _rule_use_dubbeldagar_strategically(ctx: TipsContext) -> Tip | None: severity="action", message_key="tips.use_dubbeldagar_strategically", data={"date": day_date.isoformat()}, + anchor_date=day_date, ) return None @@ -304,6 +442,7 @@ async def _rule_wrap_weekend_with_vacation(ctx: TipsContext) -> Tip | None: "friday": fp_date.isoformat(), "monday": monday.isoformat(), }, + anchor_range=(fp_date, monday), ) return None @@ -337,6 +476,7 @@ async def _rule_wrap_red_days_with_vacation(ctx: TipsContext) -> Tip | None: "cluster_start": cluster_start.isoformat(), "cluster_end": cluster_end.isoformat(), }, + anchor_range=(cluster_start, cluster_end), ) return None @@ -393,6 +533,7 @@ async def _rule_long_break_parent_swap(ctx: TipsContext) -> Tip | None: "parent_name": parent.name, "other_parent_name": other.name, }, + anchor_range=(start_date, end_date), ) return None @@ -430,6 +571,7 @@ RULE_REGISTRY: list[RuleFn] = [ _rule_use_vacation_before_expiry, _rule_balance_between_parents, _rule_preserve_for_age_4, + _rule_age4_pooling_loss, _rule_use_dubbeldagar_strategically, _rule_lagsta_niva_coverage, _rule_stretch_time_at_home, @@ -440,6 +582,18 @@ RULE_REGISTRY: list[RuleFn] = [ _rule_reserve_for_preschool_planning_days, ] +# Parental-leave entitlement (`parental_leave_fetch`), and every planned day +# that draws on it, is tracked per CHILD (see `ParentalLeaveRepository. +# get_latest_by_parent` / `PlannedLeaveRepository.list_by_workspace_child`). +# `TipsService.get_tips` therefore runs most rules once per child, against a +# context scoped to that child's own leave/planned-day data (see +# `_build_context`). `use_vacation_before_expiry` is the one rule that reads +# neither — vacation days belong to the parent, not any child — so running it +# per child would just print the same warning N times for an N-child +# workspace; it runs once instead, against a childless workspace-level +# context. +_WORKSPACE_SCOPED_RULES: frozenset[RuleFn] = frozenset({_rule_use_vacation_before_expiry}) + class TipsService: def __init__( @@ -451,6 +605,7 @@ class TipsService: vacation_repo: VacationDaysRepository, leave_wish_repo: LeaveWishRepository, workspace_repo: WorkspaceRepository, + dismissal_repo: TipDismissalRepository, ) -> None: self._leave = leave_repo self._planned = planned_repo @@ -459,47 +614,152 @@ class TipsService: self._vacation = vacation_repo self._leave_wishes = leave_wish_repo self._workspace_repo = workspace_repo + self._dismissals = dismissal_repo async def get_tips(self, workspace_id: UUID) -> list[Tip]: - """Load workspace context then run all rules, returning non-None tips.""" - ctx = await self._build_context(workspace_id) - results = await asyncio.gather(*(rule(ctx) for rule in RULE_REGISTRY)) - return [tip for tip in results if tip is not None] + """Load workspace context(s), run all rules, drop stale-dismissed tips. - async def _build_context(self, workspace_id: UUID) -> TipsContext: + One workspace-level context feeds `_WORKSPACE_SCOPED_RULES`; one + per-child context feeds every other rule (see `_build_context` and + `_WORKSPACE_SCOPED_RULES` for why). A child-scoped tip's `id` is + suffixed with its child's id so that e.g. `preserve_for_age_4` firing + for two different children never collides — neither in the returned + list nor in `TipDismissal` (dismissing child A's warning must not + dismiss child B's). + """ + workspace_ctx, child_contexts = await self._build_context(workspace_id) + tips: list[Tip] = [] + workspace_rules = [rule for rule in RULE_REGISTRY if rule in _WORKSPACE_SCOPED_RULES] + child_rules = [rule for rule in RULE_REGISTRY if rule not in _WORKSPACE_SCOPED_RULES] + + workspace_results = await asyncio.gather(*(rule(workspace_ctx) for rule in workspace_rules)) + tips.extend(tip for tip in workspace_results if tip is not None) + + for ctx in child_contexts: + child = ctx.children[0] + results = await asyncio.gather(*(rule(ctx) for rule in child_rules)) + for tip in results: + if tip is None: + continue + tip.data = {**tip.data, "child_id": str(child.id), "child_name": child.name} + tip.id = f"{tip.id}:{child.id}" + tips.append(tip) + + return await self._drop_stale_dismissed(workspace_id, tips) + + async def dismiss(self, workspace_id: UUID, tip_id: str) -> None: + """Record that `tip_id` is dismissed, fingerprinted to its current `data`. + + Recomputes the live tip list (rather than trusting a client-supplied + `data` payload) so the stored fingerprint is always the engine's own + truth. Raises NotFoundError if `tip_id` doesn't match any + currently-firing tip — there is nothing meaningful to dismiss. + """ + tips = await self.get_tips(workspace_id) + tip = next((t for t in tips if t.id == tip_id), None) + if tip is None: + raise NotFoundError(f"Tip {tip_id!r} is not currently active for this workspace") + + data_hash = hash_tip_data(tip.data) + existing = await self._dismissals.get_by_workspace_tip(workspace_id, tip_id) + if existing is not None: + existing.data_hash = data_hash + await self._dismissals.update(existing.id, existing) + return + + await self._dismissals.create( + TipDismissal( + id=uuid4(), + workspace_id=workspace_id, + tip_id=tip_id, + data_hash=data_hash, + ) + ) + + async def _drop_stale_dismissed(self, workspace_id: UUID, tips: list[Tip]) -> list[Tip]: + """Filter out tips whose dismissal is still fresh. + + A dismissal only silences the *exact* situation the user acknowledged + (same tip id, same `data`). If the rule now fires with different data + — a bigger `expiring_days` count, a moved `fourth_birthday`, ... — the + old dismissal describes a fact that's no longer true, so the tip + resurfaces instead of staying silenced forever. See module docstring + on `app.entities.tip_dismissal.TipDismissal` for why an unconditional + (non-expiring) dismissal would be dangerous for warnings like + `preserve_for_age_4`. + """ + dismissals = await self._dismissals.list_by_workspace(workspace_id) + if not dismissals: + return tips + dismissed_hash_by_id = {d.tip_id: d.data_hash for d in dismissals} + return [tip for tip in tips if dismissed_hash_by_id.get(tip.id) != hash_tip_data(tip.data)] + + async def _build_context(self, workspace_id: UUID) -> tuple[TipsContext, list[TipsContext]]: + """Build the workspace-level context plus one context per child. + + Two bugs used to live here: + + 1. **Single-child only.** Only `children[0]` was ever fetched, so a + workspace with two children got advice about one and silence about + the other — despite `list_by_workspace` already returning every + child. Now every child gets its own context (see the loop below). + 2. **Doubled planned days.** `list_by_workspace_child` already + returns every parent's days for a given child; the old code called + it again *inside* the per-parent loop and kept appending to the + same list, so an N-parent workspace saw each planned day N times. + Every count-sensitive rule (intensity, SGI, age-4 projections) was + silently working from doubled input. Fetched once per child now, + outside any per-parent loop. + """ parents = await self._parents.list_by_workspace(workspace_id) children = await self._children.list_by_workspace(workspace_id) leave_wishes = await self._leave_wishes.get_by_workspace(workspace_id) - today = date.today() - all_planned: list[PlannedLeaveDay] = [] - leave_by_parent: dict[UUID, ParentalLeaveFetch | None] = {} - vacation_by_parent: dict[UUID, list[VacationDays]] = {} - if children: - child = children[0] # MVP: single child - for pd in parents: - leave_by_parent[pd.uuid] = await self._leave.get_latest_by_parent(pd.uuid, child.id) - future = await self._planned.list_by_workspace_child( - workspace_id, child.id, from_date=today - ) - all_planned.extend(future) - vacation_by_parent[pd.uuid] = await self._vacation.list( - personal_data_uuid=pd.uuid, workspace_id=workspace_id - ) + # Vacation days belong to the parent, not any child — fetched once, + # shared by the workspace context and every per-child context. + vacation_by_parent: dict[UUID, list[VacationDays]] = {} + for pd in parents: + vacation_by_parent[pd.uuid] = await self._vacation.list( + personal_data_uuid=pd.uuid, workspace_id=workspace_id + ) ws = await self._workspace_repo.get_by_id(workspace_id) planning_mode = ws.planning_mode if ws else "solo" - return TipsContext( + workspace_ctx = TipsContext( workspace_id=workspace_id, - children=children, + children=[], parents=parents, - leave_by_parent=leave_by_parent, - planned_days=all_planned, + leave_by_parent={}, + planned_days=[], vacation_by_parent=vacation_by_parent, leave_wishes=leave_wishes, today=today, workspace_planning_mode=planning_mode, ) + + child_contexts: list[TipsContext] = [] + for child in children: + leave_by_parent: dict[UUID, ParentalLeaveFetch | None] = {} + for pd in parents: + leave_by_parent[pd.uuid] = await self._leave.get_latest_by_parent(pd.uuid, child.id) + planned_days = await self._planned.list_by_workspace_child( + workspace_id, child.id, from_date=today + ) + child_contexts.append( + TipsContext( + workspace_id=workspace_id, + children=[child], + parents=parents, + leave_by_parent=leave_by_parent, + planned_days=planned_days, + vacation_by_parent=vacation_by_parent, + leave_wishes=leave_wishes, + today=today, + workspace_planning_mode=planning_mode, + ) + ) + + return workspace_ctx, child_contexts diff --git a/backend/migrations/versions/010_add_tip_dismissal.py b/backend/migrations/versions/010_add_tip_dismissal.py new file mode 100644 index 0000000..e5a4575 --- /dev/null +++ b/backend/migrations/versions/010_add_tip_dismissal.py @@ -0,0 +1,59 @@ +"""Add tip_dismissal table for persisting tip dismissals. + +`useOptimizationTips` only kept dismissed tip ids in a local `Set` — they came +back on every reload. This adds a workspace-scoped dismissal record keyed by +`(workspace_id, tip_id)` so a dismissed tip stays dismissed across reloads, +until the underlying situation changes materially. + +`data_hash` is a fingerprint of the tip's `data` payload at dismiss time +(see `app.utils.tip_predicates.hash_tip_data`). `TipsService` only honors a +dismissal while the rule's current output still hashes the same; once the +numbers move (e.g. more days now projected to expire at age 4), the +dismissal is stale and the tip resurfaces automatically. A dismissal that +never expired could permanently silence a safety-relevant warning such as +`preserve_for_age_4` — that would be worse than not persisting dismissals at +all, so expiry-on-change is not optional here. + +Revision ID: 010 +Revises: 009 +Create Date: 2026-09-17 +""" + +from __future__ import annotations + +from alembic import op + +revision: str = "010" +down_revision: str | None = "009" +branch_labels: str | None = None +depends_on: str | None = None + +_UPGRADE_SQL = """ +CREATE TABLE tip_dismissal ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL, + tip_id TEXT NOT NULL, + data_hash TEXT NOT NULL, + dismissed_at TIMESTAMP NOT NULL, + UNIQUE(workspace_id, tip_id), + FOREIGN KEY (workspace_id) REFERENCES workspace(id) ON DELETE CASCADE +); +""" + +_DOWNGRADE_SQL = """ +DROP TABLE tip_dismissal; +""" + + +def upgrade() -> None: + from sqlalchemy import text + + for stmt in [s.strip() for s in _UPGRADE_SQL.split(";") if s.strip()]: + op.execute(text(stmt)) + + +def downgrade() -> None: + from sqlalchemy import text + + for stmt in [s.strip() for s in _DOWNGRADE_SQL.split(";") if s.strip()]: + op.execute(text(stmt)) diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index e8291fd..93092bd 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -24,6 +24,7 @@ from app.repositories_impl.sqlite.leave_wish_repository import SQLiteLeaveWishRe from app.repositories_impl.sqlite.parental_leave_repository import SQLiteParentalLeaveRepository from app.repositories_impl.sqlite.personal_data_repository import SQLitePersonalDataRepository from app.repositories_impl.sqlite.planned_leave_repository import SQLitePlannedLeaveRepository +from app.repositories_impl.sqlite.tip_dismissal_repository import SQLiteTipDismissalRepository from app.repositories_impl.sqlite.user_repository import SQLiteUserRepository from app.repositories_impl.sqlite.vacation_days_repository import ( SQLiteVacationDaysRepository, @@ -39,6 +40,7 @@ from app.services.child_service import ChildService from app.services.day_override_service import DayOverrideService from app.services.leave_wish_service import LeaveWishService from app.services.planning_service import PlanningService +from app.services.tips_service import TipsService from app.services.vacation_days_service import VacationDaysService from app.services.workspace_service import WorkspaceService @@ -308,3 +310,38 @@ async def planning_service( vacation_repo, planning_engine_service, ) + + +# --------------------------------------------------------------------------- +# Tips +# --------------------------------------------------------------------------- + + +@pytest_asyncio.fixture +async def tip_dismissal_repo( + connection: aiosqlite.Connection, +) -> SQLiteTipDismissalRepository: + return SQLiteTipDismissalRepository(connection) + + +@pytest_asyncio.fixture +async def tips_service( + parental_leave_repo: SQLiteParentalLeaveRepository, + planned_leave_repo: SQLitePlannedLeaveRepository, + child_repo: SQLiteChildRepository, + personal_data_repo: SQLitePersonalDataRepository, + vacation_repo: SQLiteVacationDaysRepository, + leave_wish_repo: SQLiteLeaveWishRepository, + workspace_repo: SQLiteWorkspaceRepository, + tip_dismissal_repo: SQLiteTipDismissalRepository, +) -> TipsService: + return TipsService( + parental_leave_repo, + planned_leave_repo, + child_repo, + personal_data_repo, + vacation_repo, + leave_wish_repo, + workspace_repo, + tip_dismissal_repo, + ) diff --git a/backend/tests/unit/test_services/test_tips_service_context.py b/backend/tests/unit/test_services/test_tips_service_context.py new file mode 100644 index 0000000..1923fd9 --- /dev/null +++ b/backend/tests/unit/test_services/test_tips_service_context.py @@ -0,0 +1,305 @@ +"""Regression tests for `TipsService._build_context` and multi-child scoping. + +`_build_context` used to have two bugs: + +1. Single-child only: it read `children[0]` and fetched leave/planned-day + data for that child alone, so a workspace with two children got tips + about one and silence about the other. +2. Doubled planned days: `list_by_workspace_child` already returns every + parent's days for a child, but the old code called it *inside* the + per-parent loop and kept appending — an N-parent workspace saw every + planned day N times, corrupting every count-sensitive rule (intensity, + SGI, age-4 projections). + +These tests go through the real sqlite repositories (via the `tips_service` +fixture in conftest.py), not hand-built `TipsContext`s, because the bugs +lived in the fetching/wiring code itself. +""" + +from datetime import date, datetime, timedelta +from uuid import uuid4 + +import pytest_asyncio + +from app.entities.child import Child +from app.entities.parental_leave import ParentalLeaveFetch, TierData +from app.entities.personal_data import PersonalData +from app.entities.planned_leave import PlannedLeaveDay +from app.entities.workspace import Workspace +from app.repositories_impl.sqlite.child_repository import SQLiteChildRepository +from app.repositories_impl.sqlite.parental_leave_repository import SQLiteParentalLeaveRepository +from app.repositories_impl.sqlite.personal_data_repository import SQLitePersonalDataRepository +from app.repositories_impl.sqlite.planned_leave_repository import SQLitePlannedLeaveRepository +from app.repositories_impl.sqlite.workspace_repository import SQLiteWorkspaceRepository +from app.services.tips_service import TipsService +from app.utils.errors import NotFoundError + + +def _make_fetch( + personal_data_uuid, + workspace_id, + child_id, + total: int = 240, + used: int = 20, +) -> ParentalLeaveFetch: + now = datetime.now() + tier = TierData( + sjukpenning_total=390, + sjukpenning_used=used, + sjukpenning_left=390 - used, + lagsta_niva_total=90, + lagsta_niva_used=0, + lagsta_niva_left=90, + ) + return ParentalLeaveFetch( + id=uuid4(), + personal_data_uuid=personal_data_uuid, + workspace_id=workspace_id, + child_id=child_id, + source="manual", + fetched_at=now, + total_days=total, + days_used=used, + days_left=total - used, + reserved_days_total=90, + reserved_days_left=90, + transferable_days_left=50, + dubbeldagar_left=60, + tier_data=tier, + created_at=now, + updated_at=now, + ) + + +def _make_planned_day( + workspace_id, child_id, parent_uuid, day: date, day_type: str = "sjukpenning" +) -> PlannedLeaveDay: + return PlannedLeaveDay( + id=uuid4(), + workspace_id=workspace_id, + child_id=child_id, + parent_personal_data_uuid=parent_uuid, + date=day, + day_type=day_type, + ) + + +@pytest_asyncio.fixture +async def two_parent_one_child_workspace( + workspace_repo: SQLiteWorkspaceRepository, + personal_data_repo: SQLitePersonalDataRepository, + child_repo: SQLiteChildRepository, + planned_leave_repo: SQLitePlannedLeaveRepository, +): + now = datetime.now() + ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + parent1 = await personal_data_repo.create( + PersonalData(uuid=uuid4(), workspace_id=ws.id, name="Alice", created_at=now) + ) + parent2 = await personal_data_repo.create( + PersonalData(uuid=uuid4(), workspace_id=ws.id, name="Bob", created_at=now) + ) + child = await child_repo.create( + Child( + id=uuid4(), + workspace_id=ws.id, + name="Barn", + birth_date=date.today().replace(year=date.today().year - 1), + created_at=now, + ) + ) + # A known, small day set: 3 future planned days for parent1 only. + today = date.today() + for i in range(3): + await planned_leave_repo.create( + _make_planned_day(ws.id, child.id, parent1.uuid, today + timedelta(days=i + 40)) + ) + return ws, parent1, parent2, child + + +class TestPlannedDaysNotDoubledAcrossParents: + async def test_build_context_does_not_double_planned_days( + self, + tips_service: TipsService, + two_parent_one_child_workspace, + ) -> None: + ws, _parent1, _parent2, _child = two_parent_one_child_workspace + + _workspace_ctx, child_contexts = await tips_service._build_context(ws.id) + + assert len(child_contexts) == 1 + # 3 planned days were seeded once each; with two parents in the + # workspace, the old code (which re-fetched `list_by_workspace_child` + # inside the per-parent loop and kept appending) would return 6. + assert len(child_contexts[0].planned_days) == 3, ( + "planned days were duplicated across parents — " + f"got {len(child_contexts[0].planned_days)} for 2 parents, expected 3" + ) + + +class TestMultiChildContexts: + async def test_each_child_gets_its_own_scoped_context( + self, + tips_service: TipsService, + workspace_repo: SQLiteWorkspaceRepository, + personal_data_repo: SQLitePersonalDataRepository, + child_repo: SQLiteChildRepository, + parental_leave_repo: SQLiteParentalLeaveRepository, + ) -> None: + now = datetime.now() + ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + parent = await personal_data_repo.create( + PersonalData(uuid=uuid4(), workspace_id=ws.id, name="Alice", created_at=now) + ) + child_a = await child_repo.create( + Child( + id=uuid4(), + workspace_id=ws.id, + name="A", + birth_date=date(2023, 1, 1), + created_at=now, + ) + ) + child_b = await child_repo.create( + Child( + id=uuid4(), + workspace_id=ws.id, + name="B", + birth_date=date(2025, 1, 1), + created_at=now, + ) + ) + await parental_leave_repo.create(_make_fetch(parent.uuid, ws.id, child_a.id, used=10)) + await parental_leave_repo.create(_make_fetch(parent.uuid, ws.id, child_b.id, used=200)) + + workspace_ctx, child_contexts = await tips_service._build_context(ws.id) + + assert workspace_ctx.children == [] + assert {c.children[0].id for c in child_contexts} == {child_a.id, child_b.id} + + by_child = {c.children[0].id: c for c in child_contexts} + assert by_child[child_a.id].leave_by_parent[parent.uuid].days_used == 10 + assert by_child[child_b.id].leave_by_parent[parent.uuid].days_used == 200 + + async def test_get_tips_scopes_tip_ids_and_data_per_child( + self, + tips_service: TipsService, + workspace_repo: SQLiteWorkspaceRepository, + personal_data_repo: SQLitePersonalDataRepository, + child_repo: SQLiteChildRepository, + parental_leave_repo: SQLiteParentalLeaveRepository, + ) -> None: + """Two children both eligible for `preserve_for_age_4` must produce + two distinct tips, not one deduped/overwritten result — see the task + brief: "a tip about child A must not dedupe against the same rule + firing for child B". + """ + now = datetime.now() + ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + parent = await personal_data_repo.create( + PersonalData(uuid=uuid4(), workspace_id=ws.id, name="Alice", created_at=now) + ) + child_a = await child_repo.create( + Child( + id=uuid4(), + workspace_id=ws.id, + name="Adam", + birth_date=date.today().replace(year=date.today().year - 1), + created_at=now, + ) + ) + child_b = await child_repo.create( + Child( + id=uuid4(), + workspace_id=ws.id, + name="Berit", + birth_date=date.today().replace(year=date.today().year - 2), + created_at=now, + ) + ) + # total=240, used=20 -> projected_left=220, expiring=220-96=124 for both. + await parental_leave_repo.create(_make_fetch(parent.uuid, ws.id, child_a.id, used=20)) + await parental_leave_repo.create(_make_fetch(parent.uuid, ws.id, child_b.id, used=20)) + + tips = await tips_service.get_tips(ws.id) + + preserve_tips = [t for t in tips if t.id.startswith("preserve_for_age_4:")] + assert len(preserve_tips) == 2, "expected one preserve_for_age_4 tip per child" + + ids = {t.id for t in preserve_tips} + assert ids == { + f"preserve_for_age_4:{child_a.id}", + f"preserve_for_age_4:{child_b.id}", + } + child_names = {t.data["child_name"] for t in preserve_tips} + assert child_names == {"Adam", "Berit"} + + +class TestDismissals: + async def test_dismiss_hides_tip_until_data_changes( + self, + tips_service: TipsService, + workspace_repo: SQLiteWorkspaceRepository, + personal_data_repo: SQLitePersonalDataRepository, + child_repo: SQLiteChildRepository, + parental_leave_repo: SQLiteParentalLeaveRepository, + planned_leave_repo: SQLitePlannedLeaveRepository, + ) -> None: + now = datetime.now() + ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + parent = await personal_data_repo.create( + PersonalData(uuid=uuid4(), workspace_id=ws.id, name="Alice", created_at=now) + ) + child = await child_repo.create( + Child( + id=uuid4(), + workspace_id=ws.id, + name="Barn", + birth_date=date.today().replace(year=date.today().year - 1), + created_at=now, + ) + ) + await parental_leave_repo.create(_make_fetch(parent.uuid, ws.id, child.id, used=20)) + + tips = await tips_service.get_tips(ws.id) + tip = next(t for t in tips if t.id.startswith("preserve_for_age_4:")) + original_expiring = tip.data["expiring_days"] + + await tips_service.dismiss(ws.id, tip.id) + + tips_after_dismiss = await tips_service.get_tips(ws.id) + assert tip.id not in {t.id for t in tips_after_dismiss} + + # The situation changes materially: add planned FP days before the + # 4th birthday, which shrinks `expiring_days`. The stale dismissal + # must not silence the new figure. + fourth_bday = child.birth_date.replace(year=child.birth_date.year + 4) + for i in range(30): + await planned_leave_repo.create( + _make_planned_day( + ws.id, + child.id, + parent.uuid, + fourth_bday - timedelta(days=i + 1), + ) + ) + + tips_after_change = await tips_service.get_tips(ws.id) + resurfaced = next((t for t in tips_after_change if t.id == tip.id), None) + assert resurfaced is not None, "dismissal must expire once expiring_days changes" + assert resurfaced.data["expiring_days"] != original_expiring + + async def test_dismiss_unknown_tip_raises_not_found( + self, + tips_service: TipsService, + workspace_repo: SQLiteWorkspaceRepository, + ) -> None: + now = datetime.now() + ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + + try: + await tips_service.dismiss(ws.id, "not_a_real_tip") + raised = False + except NotFoundError: + raised = True + assert raised diff --git a/backend/tests/unit/test_tip_dismissal_repository.py b/backend/tests/unit/test_tip_dismissal_repository.py new file mode 100644 index 0000000..a3f3e6a --- /dev/null +++ b/backend/tests/unit/test_tip_dismissal_repository.py @@ -0,0 +1,123 @@ +"""Tests for the TipDismissal repository. + +Follows the DayOverride reference pattern (upsert-by-natural-key, UNIQUE +constraint on the natural key): arrange an entity, exercise the repo, assert +on the result. +""" + +from datetime import datetime +from uuid import uuid4 + +import pytest + +from app.entities.tip_dismissal import TipDismissal +from app.entities.workspace import Workspace +from app.repositories_impl.sqlite.tip_dismissal_repository import SQLiteTipDismissalRepository +from app.repositories_impl.sqlite.workspace_repository import SQLiteWorkspaceRepository +from app.utils.errors import NotFoundError +from tests.conftest import SeededIds + + +def _make(workspace_id=None, **overrides) -> TipDismissal: + base = dict( + id=uuid4(), + workspace_id=workspace_id or uuid4(), + tip_id="preserve_for_age_4:child-1", + data_hash="deadbeef", + dismissed_at=datetime.now(), + ) + base.update(overrides) + return TipDismissal(**base) + + +class TestSQLiteTipDismissalRepository: + async def test_create_and_get_by_id( + self, + tip_dismissal_repo: SQLiteTipDismissalRepository, + seeded_ids: SeededIds, + ) -> None: + entity = _make(workspace_id=seeded_ids.workspace_id) + await tip_dismissal_repo.create(entity) + + fetched = await tip_dismissal_repo.get_by_id(entity.id) + assert fetched is not None + assert fetched.tip_id == "preserve_for_age_4:child-1" + assert fetched.data_hash == "deadbeef" + + async def test_get_by_workspace_tip( + self, + tip_dismissal_repo: SQLiteTipDismissalRepository, + seeded_ids: SeededIds, + ) -> None: + entity = _make(workspace_id=seeded_ids.workspace_id) + await tip_dismissal_repo.create(entity) + + found = await tip_dismissal_repo.get_by_workspace_tip( + seeded_ids.workspace_id, "preserve_for_age_4:child-1" + ) + assert found is not None + assert found.id == entity.id + + missing = await tip_dismissal_repo.get_by_workspace_tip(uuid4(), "no_such_tip") + assert missing is None + + async def test_update_refreshes_hash_and_timestamp( + self, + tip_dismissal_repo: SQLiteTipDismissalRepository, + seeded_ids: SeededIds, + ) -> None: + entity = _make(workspace_id=seeded_ids.workspace_id) + await tip_dismissal_repo.create(entity) + + entity.data_hash = "newhash" + updated = await tip_dismissal_repo.update(entity.id, entity) + assert updated.data_hash == "newhash" + + async def test_update_missing_raises( + self, tip_dismissal_repo: SQLiteTipDismissalRepository + ) -> None: + with pytest.raises(NotFoundError): + await tip_dismissal_repo.update(uuid4(), _make()) + + async def test_delete( + self, + tip_dismissal_repo: SQLiteTipDismissalRepository, + seeded_ids: SeededIds, + ) -> None: + entity = _make(workspace_id=seeded_ids.workspace_id) + await tip_dismissal_repo.create(entity) + await tip_dismissal_repo.delete(entity.id) + assert await tip_dismissal_repo.get_by_id(entity.id) is None + + async def test_delete_missing_raises( + self, tip_dismissal_repo: SQLiteTipDismissalRepository + ) -> None: + with pytest.raises(NotFoundError): + await tip_dismissal_repo.delete(uuid4()) + + async def test_list_by_workspace_filters( + self, + tip_dismissal_repo: SQLiteTipDismissalRepository, + seeded_ids: SeededIds, + workspace_repo: SQLiteWorkspaceRepository, + ) -> None: + ws = seeded_ids.workspace_id + ws2 = await workspace_repo.create( + Workspace(id=uuid4(), name="WS2", created_at=datetime.now()) + ) + + await tip_dismissal_repo.create(_make(workspace_id=ws, tip_id="a")) + await tip_dismissal_repo.create(_make(workspace_id=ws, tip_id="b")) + await tip_dismissal_repo.create(_make(workspace_id=ws2.id, tip_id="c")) + + rows = await tip_dismissal_repo.list_by_workspace(ws) + assert {r.tip_id for r in rows} == {"a", "b"} + + async def test_unique_constraint_per_workspace_tip( + self, + tip_dismissal_repo: SQLiteTipDismissalRepository, + seeded_ids: SeededIds, + ) -> None: + await tip_dismissal_repo.create(_make(workspace_id=seeded_ids.workspace_id, tip_id="x")) + with pytest.raises(Exception): # sqlite3.IntegrityError via aiosqlite + await tip_dismissal_repo.create(_make(workspace_id=seeded_ids.workspace_id, tip_id="x")) diff --git a/backend/tests/unit/test_tip_predicates_hash.py b/backend/tests/unit/test_tip_predicates_hash.py new file mode 100644 index 0000000..4284ea1 --- /dev/null +++ b/backend/tests/unit/test_tip_predicates_hash.py @@ -0,0 +1,30 @@ +"""Tests for `hash_tip_data` — the dismissal-expiry fingerprint. + +See `app.entities.tip_dismissal.TipDismissal` and +`TipsService._drop_stale_dismissed`: a dismissal only stays silent while the +rule's `data` still hashes the same. These tests pin the two properties that +matter for that contract: stability (same data → same hash, regardless of +key order) and sensitivity (any value change → a different hash). +""" + +from app.utils.tip_predicates import hash_tip_data + + +class TestHashTipData: + def test_stable_regardless_of_key_order(self) -> None: + a = hash_tip_data({"parent_name": "Alice", "expiring_days": 12}) + b = hash_tip_data({"expiring_days": 12, "parent_name": "Alice"}) + assert a == b + + def test_changes_when_a_value_changes(self) -> None: + a = hash_tip_data({"parent_name": "Alice", "expiring_days": 12}) + b = hash_tip_data({"parent_name": "Alice", "expiring_days": 13}) + assert a != b + + def test_changes_when_a_key_is_added_or_removed(self) -> None: + a = hash_tip_data({"parent_name": "Alice"}) + b = hash_tip_data({"parent_name": "Alice", "expiring_days": 12}) + assert a != b + + def test_empty_data_hashes_consistently(self) -> None: + assert hash_tip_data({}) == hash_tip_data({}) diff --git a/backend/tests/unit/test_tips_router.py b/backend/tests/unit/test_tips_router.py new file mode 100644 index 0000000..11f3cc6 --- /dev/null +++ b/backend/tests/unit/test_tips_router.py @@ -0,0 +1,159 @@ +"""API-level tests for `GET /api/tips/{workspace_id}` and its dismiss route. + +Guards two things a unit test on the service can't see: + - the wire shape (`TipResponse`) actually carries `anchor_date`/ + `anchor_range` and `message_key` + `data` — not rendered prose; + - the dismiss endpoint round-trips through the real DB and a second GET + no longer returns the dismissed tip. +""" + +from datetime import date, datetime +from uuid import uuid4 + +import aiosqlite +import pytest_asyncio +from httpx import ASGITransport, AsyncClient + +from app.dependencies import get_connection, get_current_user +from app.entities.child import Child +from app.entities.parental_leave import ParentalLeaveFetch, TierData +from app.entities.personal_data import PersonalData +from app.entities.user import User +from app.entities.workspace import Workspace +from app.main import app +from app.repositories_impl.sqlite.child_repository import SQLiteChildRepository +from app.repositories_impl.sqlite.parental_leave_repository import SQLiteParentalLeaveRepository +from app.repositories_impl.sqlite.personal_data_repository import SQLitePersonalDataRepository +from app.repositories_impl.sqlite.workspace_repository import SQLiteWorkspaceRepository + + +def _make_fetch(personal_data_uuid, workspace_id, child_id, used: int = 20) -> ParentalLeaveFetch: + now = datetime.now() + tier = TierData( + sjukpenning_total=390, + sjukpenning_used=used, + sjukpenning_left=390 - used, + lagsta_niva_total=90, + lagsta_niva_used=0, + lagsta_niva_left=90, + ) + return ParentalLeaveFetch( + id=uuid4(), + personal_data_uuid=personal_data_uuid, + workspace_id=workspace_id, + child_id=child_id, + source="manual", + fetched_at=now, + total_days=240, + days_used=used, + days_left=240 - used, + reserved_days_total=90, + reserved_days_left=90, + transferable_days_left=50, + dubbeldagar_left=60, + tier_data=tier, + created_at=now, + updated_at=now, + ) + + +@pytest_asyncio.fixture +async def client(connection: aiosqlite.Connection) -> AsyncClient: + app.dependency_overrides[get_connection] = lambda: connection + app.dependency_overrides[get_current_user] = lambda: User( + id=uuid4(), + personal_data_uuid=uuid4(), + email="test@example.com", + hashed_password="not-a-real-hash", + created_at=datetime.now(), + ) + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as ac: + yield ac + app.dependency_overrides.pop(get_connection, None) + app.dependency_overrides.pop(get_current_user, None) + + +@pytest_asyncio.fixture +async def workspace_with_expiring_days(connection: aiosqlite.Connection): + now = datetime.now() + workspace_repo = SQLiteWorkspaceRepository(connection) + personal_data_repo = SQLitePersonalDataRepository(connection) + child_repo = SQLiteChildRepository(connection) + parental_leave_repo = SQLiteParentalLeaveRepository(connection) + + ws = await workspace_repo.create(Workspace(id=uuid4(), name="Familj", created_at=now)) + parent = await personal_data_repo.create( + PersonalData(uuid=uuid4(), workspace_id=ws.id, name="Alice", created_at=now) + ) + child = await child_repo.create( + Child( + id=uuid4(), + workspace_id=ws.id, + name="Barn", + birth_date=date.today().replace(year=date.today().year - 1), + created_at=now, + ) + ) + await parental_leave_repo.create(_make_fetch(parent.uuid, ws.id, child.id, used=20)) + return ws, child + + +class TestGetTipsOverTheWire: + async def test_response_carries_anchor_date_and_no_rendered_prose( + self, + client: AsyncClient, + workspace_with_expiring_days, + ) -> None: + ws, child = workspace_with_expiring_days + + response = await client.get(f"/api/tips/{ws.id}") + assert response.status_code == 200 + body = response.json() + + preserve = next(t for t in body if t["id"].startswith("preserve_for_age_4:")) + assert preserve["id"] == f"preserve_for_age_4:{child.id}" + assert preserve["message_key"] == "tips.preserve_for_age_4" + assert preserve["anchor_date"] == ( + child.birth_date.replace(year=child.birth_date.year + 4).isoformat() + ) + assert preserve["anchor_range"] is None + assert preserve["data"]["child_id"] == str(child.id) + assert preserve["data"]["child_name"] == "Barn" + + # Never rendered Swedish prose — only message_key + data. + for tip in body: + assert isinstance(tip["message_key"], str) and tip["message_key"].startswith("tips.") + for value in tip["data"].values(): + assert not isinstance(value, dict) + + +class TestDismissTipOverTheWire: + async def test_dismiss_then_get_excludes_the_tip( + self, + client: AsyncClient, + workspace_with_expiring_days, + ) -> None: + ws, _child = workspace_with_expiring_days + + before = await client.get(f"/api/tips/{ws.id}") + tip = next(t for t in before.json() if t["id"].startswith("preserve_for_age_4:")) + + dismiss_response = await client.post( + f"/api/tips/{ws.id}/dismissals", json={"tip_id": tip["id"]} + ) + assert dismiss_response.status_code == 204 + + after = await client.get(f"/api/tips/{ws.id}") + assert tip["id"] not in {t["id"] for t in after.json()} + + async def test_dismiss_unknown_tip_returns_404( + self, + client: AsyncClient, + workspace_with_expiring_days, + ) -> None: + ws, _child = workspace_with_expiring_days + + response = await client.post( + f"/api/tips/{ws.id}/dismissals", json={"tip_id": "not_a_real_tip"} + ) + assert response.status_code == 404 diff --git a/frontend/src/hooks/useOptimizationTips.test.ts b/frontend/src/hooks/useOptimizationTips.test.ts index a9f3f83..05130ea 100644 --- a/frontend/src/hooks/useOptimizationTips.test.ts +++ b/frontend/src/hooks/useOptimizationTips.test.ts @@ -61,13 +61,14 @@ describe('useOptimizationTips', () => { expect(result.current.error).toBe('Failed to fetch tips') }) - it('dismisses tips locally', async () => { + it('dismisses tips locally and persists the dismissal', async () => { const mockTips = [ makeTip({ id: 'tip1', severity: 'info', messageKey: 'msg1' }), makeTip({ id: 'tip2', severity: 'warning', messageKey: 'msg2' }), ] vi.mocked(api.getTips).mockResolvedValue(mockTips) + vi.mocked(api.dismissTip).mockResolvedValue(undefined) const { result } = renderHook(() => useOptimizationTips('ws1')) @@ -77,12 +78,45 @@ describe('useOptimizationTips', () => { expect(result.current.data?.length).toBe(2) - act(() => { - result.current.dismiss('tip1') + await act(async () => { + await result.current.dismiss('tip1') }) expect(result.current.data?.length).toBe(1) expect(result.current.data?.[0]?.id).toBe('tip2') + expect(api.dismissTip).toHaveBeenCalledWith('ws1', 'tip1') + }) + + it('keeps a tip hidden locally even if persisting the dismissal fails', async () => { + const mockTips = [makeTip({ id: 'tip1', severity: 'info', messageKey: 'msg1' })] + + vi.mocked(api.getTips).mockResolvedValue(mockTips) + vi.mocked(api.dismissTip).mockRejectedValue(new Error('network down')) + + const { result } = renderHook(() => useOptimizationTips('ws1')) + + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + + await act(async () => { + await result.current.dismiss('tip1') + }) + + expect(result.current.data?.length).toBe(0) + expect(result.current.error).toContain('network down') + }) + + it('does not call the API when dismissing with no workspace', async () => { + vi.mocked(api.getTips).mockResolvedValue([]) + + const { result } = renderHook(() => useOptimizationTips(null)) + + await act(async () => { + await result.current.dismiss('tip1') + }) + + expect(api.dismissTip).not.toHaveBeenCalled() }) it('provides refresh function', async () => { diff --git a/frontend/src/hooks/useOptimizationTips.ts b/frontend/src/hooks/useOptimizationTips.ts index 6f2e4f9..6e48758 100644 --- a/frontend/src/hooks/useOptimizationTips.ts +++ b/frontend/src/hooks/useOptimizationTips.ts @@ -7,7 +7,7 @@ interface UseOptimizationTipsResult { isLoading: boolean error: string | null refresh: () => Promise - dismiss: (tipId: string) => void + dismiss: (tipId: string) => Promise } export function useOptimizationTips(workspaceId: string | null): UseOptimizationTipsResult { @@ -31,9 +31,24 @@ export function useOptimizationTips(workspaceId: string | null): UseOptimization } }, [workspaceId]) - const dismiss = useCallback((tipId: string) => { - setDismissed((prev) => new Set([...prev, tipId])) - }, []) + const dismiss = useCallback( + async (tipId: string) => { + // Hide it immediately (optimistic) — a dismiss button that waits on + // the network before reacting feels broken. + setDismissed((prev) => new Set([...prev, tipId])) + if (!workspaceId) return + try { + await api.dismissTip(workspaceId, tipId) + } catch (err) { + // Persistence failed: it stays hidden for this session (the local + // Set above), but the next reload will show it again since the + // server never recorded it. Surface the failure rather than + // swallowing it — see AGENTS.md's "no empty catch blocks". + setError(err instanceof Error ? err.message : 'Failed to dismiss tip') + } + }, + [workspaceId], + ) useEffect(() => { fetch() diff --git a/frontend/src/services/api.test.ts b/frontend/src/services/api.test.ts index 91923b5..f6d7bbc 100644 --- a/frontend/src/services/api.test.ts +++ b/frontend/src/services/api.test.ts @@ -248,6 +248,68 @@ describe('API response mappers (snake_case → camelCase)', () => { }) }) + describe('getTips', () => { + it('maps snake_case tips (incl. anchor fields) to camelCase', async () => { + const rawResponse = [ + { + id: 'preserve_for_age_4:child-1', + severity: 'warning' as const, + message_key: 'tips.preserve_for_age_4', + data: { parent_name: 'Alice', expiring_days: 12, child_id: 'child-1' }, + anchor_date: '2030-01-01', + anchor_range: null, + }, + { + id: 'wrap_weekend_with_vacation:child-1', + severity: 'info' as const, + message_key: 'tips.wrap_weekend_with_vacation', + data: { parent_name: 'Bob' }, + anchor_date: null, + anchor_range: ['2026-07-03', '2026-07-06'] as [string, string], + }, + ] + + fetchMock.mockResolvedValue({ + ok: true, + status: 200, + json: async () => rawResponse, + }) + + const result = await api.getTips('ws1') + + expect(result).toHaveLength(2) + expect(result[0]!.id).toBe('preserve_for_age_4:child-1') + expect(result[0]!.messageKey).toBe('tips.preserve_for_age_4') + expect(result[0]!.anchorDate).toBe('2030-01-01') + expect(result[0]!.anchorRange).toBeNull() + expect(result[0]!.data.expiring_days).toBe(12) + + expect(result[1]!.anchorDate).toBeNull() + expect(result[1]!.anchorRange).toEqual(['2026-07-03', '2026-07-06']) + }) + }) + + describe('dismissTip', () => { + it('posts tip_id to the dismissals endpoint', async () => { + fetchMock.mockResolvedValue({ + ok: true, + status: 204, + json: async () => null, + }) + + await api.dismissTip('ws1', 'preserve_for_age_4:child-1') + + const calls = vi.mocked(fetchMock).mock.calls + if (calls.length === 0) throw new Error('No fetch calls made') + const [url, options] = calls[0]! + const body = JSON.parse(options.body) + + expect(String(url)).toContain('/tips/ws1/dismissals') + expect(options.method).toBe('POST') + expect(body.tip_id).toBe('preserve_for_age_4:child-1') + }) + }) + describe('createLeaveWish', () => { it('sends camelCase request as snake_case and maps the response back', async () => { const rawResponse = { diff --git a/frontend/src/services/api.ts b/frontend/src/services/api.ts index 7ee95ac..fd7fce5 100644 --- a/frontend/src/services/api.ts +++ b/frontend/src/services/api.ts @@ -214,6 +214,8 @@ export interface RawTip { severity: 'info' | 'warning' | 'action' message_key: string data: Record + anchor_date: string | null + anchor_range: [string, string] | null } function toTip(raw: RawTip): Tip { @@ -222,6 +224,8 @@ function toTip(raw: RawTip): Tip { severity: raw.severity, messageKey: raw.message_key, data: raw.data, + anchorDate: raw.anchor_date, + anchorRange: raw.anchor_range, } } @@ -1007,6 +1011,13 @@ export async function getTips(workspaceId: string): Promise { return raw.map(toTip) } +export async function dismissTip(workspaceId: string, tipId: string): Promise { + return apiFetch(`/tips/${workspaceId}/dismissals`, { + method: 'POST', + body: JSON.stringify({ tip_id: tipId }), + }) +} + // --------------------------------------------------------------------------- // Parental Leave Fetch // --------------------------------------------------------------------------- diff --git a/frontend/src/types/entities.ts b/frontend/src/types/entities.ts index 864d391..fb09403 100644 --- a/frontend/src/types/entities.ts +++ b/frontend/src/types/entities.ts @@ -216,6 +216,11 @@ export interface Tip { severity: 'info' | 'warning' | 'action' messageKey: string data: Record + // Where this tip is anchored on a calendar: a single day, a span, or + // neither (many rules — e.g. balance_between_parents — have no date that + // makes sense to place on a calendar rail). Never both set at once. + anchorDate: string | null // ISO-8601 date + anchorRange: [string, string] | null // [ISO-8601 date, ISO-8601 date] } // Planning engine — POST /api/plan/propose (read-only preview, nothing is saved) diff --git a/frontend/tests/fixtures/mockData.ts b/frontend/tests/fixtures/mockData.ts index c1487c3..5fb190c 100644 --- a/frontend/tests/fixtures/mockData.ts +++ b/frontend/tests/fixtures/mockData.ts @@ -202,6 +202,8 @@ export function makeTip(overrides: Partial = {}): Tip { severity: 'info', messageKey: 'tips.example', data: {}, + anchorDate: null, + anchorRange: null, ...overrides, } }