diff --git a/custom_components/nestquest/dao_instances.py b/custom_components/nestquest/dao_instances.py index 841c1a7..8c9e852 100644 --- a/custom_components/nestquest/dao_instances.py +++ b/custom_components/nestquest/dao_instances.py @@ -314,9 +314,13 @@ class QuestInstancesDao: Returns the stored instance, or ``None`` when a precondition failed — the tuple no longer materializable, which the walk SKIPS rather than raising, so the rest of the batch is unaffected. The - no-past rule and the immutable-completed-instance refusal are - preserved from :meth:`upsert` (those are not configuration - invalidations and still raise ValueError). + no-past rule (instances are never generated in the past) is + preserved from :meth:`upsert` and still raises ValueError. An + instance that ALREADY has a completion event is also skipped + (returns ``None``) rather than raised: it is immutable, so the + walk leaves it untouched instead of failing the batch — the + generic :meth:`upsert` keeps its hard refusal for the + delete/regenerate path. """ _validate_date(due_date, "due_date") _validate_window(window) @@ -359,12 +363,12 @@ class QuestInstancesDao: (definition_id, child_id, due_date, window), ) if existing is not None: - raise ValueError( - f"instance for (definition {definition_id}, " - f"child {child_id}, due_date {due_date!r}, " - f"window {window!r}) already has a completion " - "event and is immutable; it cannot be regenerated" - ) + # The instance is already completed and therefore + # immutable: the walk SKIPS it (no-op) rather than + # rewriting its snapshot columns or failing the + # batch. This is the materialization walk's skip, + # distinct from the generic :meth:`upsert` refusal. + return None await self._database.execute( "INSERT INTO quest_instances (definition_id, child_id, " "window, due_date, due_time, generated_at) " diff --git a/custom_components/nestquest/materialize.py b/custom_components/nestquest/materialize.py index 5c4de5b..577533c 100644 --- a/custom_components/nestquest/materialize.py +++ b/custom_components/nestquest/materialize.py @@ -3,8 +3,8 @@ :func:`materialize` turns the schedule rules + custody presence of the active quest definitions into concrete ``quest_instances`` rows over a date range. It is the pure walk over definitions x dates x assignees x -windows; HA scheduling (day-rollover triggers) and skip-completed -generation are separate later tasks and live elsewhere. +windows; HA scheduling (day-rollover triggers) is a separate later +task and lives elsewhere. The walk is deterministic and idempotent: @@ -32,9 +32,11 @@ live assignment, declared window, and its CURRENT ``due_time``) inside the SAME transaction as the INSERT, so the check and the write cannot be split by a racing config edit. -The upsert is idempotent on (definition_id, child_id, due_date, window), -so re-running the materialization over the same range never duplicates a -row; skipped-completed handling is deliberately NOT done here. +The upsert is idempotent on (definition_id, child_id, due_date, +window), so re-running the materialization over the same range never +duplicates a row; a tuple whose instance is already completed is SKIPPED +(never rewritten, never raised) by +:meth:`~.dao_instances.QuestInstancesDao.upsert_if_valid`. """ from __future__ import annotations diff --git a/tests/test_materialize.py b/tests/test_materialize.py index 48c5c06..bd5025d 100644 --- a/tests/test_materialize.py +++ b/tests/test_materialize.py @@ -7,7 +7,10 @@ import datetime import pytest from custom_components.nestquest.dao_children import ChildrenDao -from custom_components.nestquest.dao_instances import QuestInstancesDao +from custom_components.nestquest.dao_instances import ( + CompletionEventsDao, + QuestInstancesDao, +) from custom_components.nestquest.dao_presence import PresenceSchedulesDao from custom_components.nestquest.dao_rules import QuestDefinitionsDao from custom_components.nestquest.db import NestQuestDatabase @@ -15,6 +18,7 @@ from custom_components.nestquest.materialize import materialize from custom_components.nestquest.migrations import apply_migrations from custom_components.nestquest.quest_definitions import ( create_quest_definition, + edit_quest_definition, ) from custom_components.nestquest.recurrence import RuleType, ScheduleRule @@ -245,6 +249,65 @@ def test_materialize_is_idempotent(tmp_path) -> None: _with_db(tmp_path, "materialize-idempotent.db")(_body) +def test_materialize_skips_completed_instance_on_re_run(tmp_path) -> None: + async def _body(database): + children = ChildrenDao(database) + child = await children.create("Ada", NOW) + start = _future_monday() + start_iso = start.isoformat() + end_iso = (start + datetime.timedelta(days=2)).isoformat() + created = await create_quest_definition( + database, + "Daily chore", + ScheduleRule(rule_type=RuleType.DAILY, start_date=start_iso), + [child.id], + [("morning", "09:00")], + ) + + await materialize(database, start_iso, end_iso) + dao = QuestInstancesDao(database) + records = await dao.list_by_date_range(child.id, start_iso, end_iso) + assert len(records) == 3 + completed = records[0] + + await CompletionEventsDao(database).append( + completed.id, + child.id, + "completed", + "user", + f"{start_iso}T08:00:00+00:00", + True, + actor_user_id="user-1", + ) + + # Change the definition so the tuple would otherwise regenerate: + # the morning window's due time moves from 09:00 to 10:15. + await edit_quest_definition( + database, + created.definition.id, + windows=[("morning", "10:15")], + ) + + # Re-running must skip the completed instance (no exception) and + # leave it untouched, while the open days regenerate to 10:15. + count = await materialize(database, start_iso, end_iso) + assert count == 2 + + refreshed = await dao.list_by_date_range(child.id, start_iso, end_iso) + by_due_date = {r.due_date: r for r in refreshed} + survived = by_due_date[completed.due_date] + assert survived.id == completed.id + assert survived.due_time == "09:00" + assert survived.generated_at == completed.generated_at + + others = [r for r in refreshed if r.id != completed.id] + assert len(others) == 2 + assert {r.due_time for r in others} == {"10:15"} + return count + + _with_db(tmp_path, "materialize-skip-completed.db")(_body) + + async def _materialize_with_change( database, start_iso, end_iso, change ) -> int: