feat: skip completed instances during materialization

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) "

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
 

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
 
@@ -207,15 +211,33 @@ def test_materialize_is_idempotent(tmp_path) -> None:
             [child.id],
             ["morning"],
         )
+        dao = QuestInstancesDao(database)
         first = await materialize(database, start_iso, end_iso)
-        second = await materialize(database, start_iso, end_iso)
-        assert first == second == 7
-        # Running twice refreshes the SAME seven rows: exactly one
-        # physical row per (definition_id, child_id, due_date, window)
-        # key, never a duplicate.
-        records = await QuestInstancesDao(database).list_by_date_range(
-            child.id, start_iso, end_iso
+        assert first == 7
+        first_ids = sorted(
+            r.id
+            for r in await dao.list_by_date_range(child.id, start_iso, end_iso)
         )
+        assert len(first_ids) == 7
+        assert len(set(first_ids)) == 7
+
+        # Re-running over the same range refreshes the SAME physical rows:
+        # the ON CONFLICT path updates the snapshot columns in place and
+        # never delete + reinserts, so every primary-key id is stable.
+        for _ in range(2):
+            count = await materialize(database, start_iso, end_iso)
+            assert count == 7
+            run_ids = sorted(
+                r.id
+                for r in await dao.list_by_date_range(
+                    child.id, start_iso, end_iso
+                )
+            )
+            assert run_ids == first_ids
+
+        # And the widened key never duplicated: exactly one physical row
+        # per (definition_id, child_id, due_date, window).
+        records = await dao.list_by_date_range(child.id, start_iso, end_iso)
         keys = [
             (r.definition_id, r.child_id, r.due_date, r.window)
             for r in records
@@ -227,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: