fix: consistent edit snapshot and rollback coverage (CQ-01, CQ-02)

custom_components/nestquest/dao_rules.py

@@ -907,66 +907,6 @@ class QuestDefinitionsDao:
         )
         return [_definition_from_row(row) for row in rows]
 
-    async def _snapshots_for(
-        self, definitions: list[QuestDefinitionRecord]
-    ) -> list[QuestDefinitionSnapshot]:
-        """Read each definition's rule, assignees and windows.
-
-        MUST be called inside the connection lock (and ideally a
-        transaction) so the definitions handed in and the rows this
-        reads back cannot be raced by a concurrent edit between list
-        and bundle.  Each rule is read through
-        :class:`ScheduleRulesDao` on the same connection, so the
-        snapshot's rule record is the one the definition references AT
-        the moment of the locked read.
-        """
-        rules_dao = ScheduleRulesDao(self._database)
-        snapshots: list[QuestDefinitionSnapshot] = []
-        for definition in definitions:
-            rule_record = await rules_dao.get(definition.schedule_rule_id)
-            assert rule_record is not None
-            snapshots.append(
-                QuestDefinitionSnapshot(
-                    definition=definition,
-                    rule=rule_record,
-                    assignees=await self.list_assignees(definition.id),
-                    windows=await self.list_windows(definition.id),
-                )
-            )
-        return snapshots
-
-    async def list_snapshots_active(self) -> list[QuestDefinitionSnapshot]:
-        """Return every active definition with its rule, assignees and
-        windows, all read inside one locked transaction.
-
-        The definitive list and each definition's linked rows are read
-        in a single BEGIN..COMMIT span under the connection lock, so a
-        concurrent atomic edit cannot interleave between the definition
-        list and the bundle read: the caller receives a coherent
-        snapshot (each definition's rule, assignees and windows as they
-        were at one instant), never a mixed old-rule/new-windows state.
-        """
-        async with _connection_lock(self._database):
-            async with self._database.transaction():
-                definitions = await self.list_active()
-                return await self._snapshots_for(definitions)
-
-    async def list_snapshots_by_child(
-        self, child_id: int
-    ) -> list[QuestDefinitionSnapshot]:
-        """Return each definition assigned to ``child_id`` with its rule,
-        assignees and windows, all read inside one locked transaction.
-
-        Mirrors :meth:`list_snapshots_active` but selects by assignment
-        (``list_by_child`` order: newest first).  Locks read-and-bundle
-        into one transaction so the bundle never mixes one edit's
-        definition with another's windows.
-        """
-        async with _connection_lock(self._database):
-            async with self._database.transaction():
-                definitions = await self.list_by_child(child_id)
-                return await self._snapshots_for(definitions)
-
     async def update(
         self,
         definition_id: int,
@@ -1022,18 +962,14 @@ class QuestDefinitionsDao:
 
     async def add_assignee(
         self, definition_id: int, child_id: int
-    ) -> list[ChildRecord]:
+    ) -> None:
         """Assign ``child_id`` to the definition (idempotent).
 
         Validates the assignee inside the same serialized transaction
         as the INSERT (existing and active AT ASSIGNMENT TIME), so a
         child deactivated concurrently cannot become an assignee.
         Assigning an already-assigned child is a no-op, mirroring the
-        composite primary key's storage-level guarantee.  Returns the
-        definition's assignees read back INSIDE the same transaction
-        that performed the write, so the returned roster is a
-        consistent post-assignment snapshot — a concurrent assign or
-        unassign cannot race it after the write commits.  Changes
+        composite primary key's storage-level guarantee.  Changes
         future instances only: already-generated instances and
         completion history are never rewritten here.
         """
@@ -1054,7 +990,6 @@ class QuestDefinitionsDao:
                     "ON CONFLICT (definition_id, child_id) DO NOTHING",
                     (definition_id, child_id),
                 )
-                return await self.list_assignees(definition_id)
 
     async def remove_assignee(
         self, definition_id: int, child_id: int

custom_components/nestquest/quest_definitions.py

@@ -3,11 +3,9 @@
 Sits between the Feature 09 service gate and the typed
 :class:`~.dao_rules.QuestDefinitionsDao`: callers get validation and the
 all-or-nothing create/edit here, and never touch the DAO SQL directly.
-:func:`create_quest_definition` is the create path,
-:func:`edit_quest_definition` the edit path, :func:`assign_child` and
-:func:`unassign_child` the assignment paths, and
-:func:`set_quest_definition_active` the deactivate/reactivate path.
-There is no permission check here (that is Feature 09).
+:func:`create_quest_definition` is the create path and
+:func:`edit_quest_definition` the edit path; there is no permission
+check here (that is Feature 09).
 
 Validation policy mirrors :mod:`.children`:
 
@@ -41,16 +39,13 @@ from __future__ import annotations
 import datetime
 from dataclasses import dataclass
 
-from .dao_children import ChildRecord, _connection_lock
+from .dao_children import ChildRecord
 from .dao_rules import (
     QuestDefinitionRecord,
-    QuestDefinitionSnapshot,
     QuestDefinitionWindowRecord,
     QuestDefinitionsDao,
     ScheduleRuleStorage,
-    ScheduleRulesDao,
     _UNSET,
-    _validate_date,
     _validate_time,
     _validate_window,
     schedule_rule_from_storage,
@@ -58,12 +53,12 @@ from .dao_rules import (
     schedule_rule_to_storage,
 )
 from .db import NestQuestDatabase
-from .recurrence import ScheduleRule, occurs_on
+from .recurrence import ScheduleRule
 
 
 @dataclass(frozen=True)
 class CreatedQuestDefinition:
-    """The persisted result of a create, edit or activation change.
+    """The persisted result of a create or edit.
 
     ``definition`` is the stored row; ``rule`` is the schedule rule
     decoded back through the storage mapping; ``assignees`` and
@@ -190,18 +185,6 @@ def _validate_definition_id(value: object) -> int:
     return value
 
 
-def _validate_child_id(value: object) -> int:
-    """Reject non-int child ids (bools included) before any lookup.
-
-    SQLite binds Python bools as integers, so ``True`` would silently
-    address child 1 and a float would round — malformed service input
-    must never mutate the wrong profile.
-    """
-    if isinstance(value, bool) or not isinstance(value, int):
-        raise ValueError(f"child_id must be an integer, got {value!r}")
-    return value
-
-
 async def create_quest_definition(
     database: NestQuestDatabase,
     title: str,
@@ -319,23 +302,14 @@ async def edit_quest_definition(
         window_specs = _normalize_windows(windows)
 
     dao = QuestDefinitionsDao(database)
-    try:
-        snapshot = await dao.edit_definition(
-            definition_id,
-            title=title_value,
-            description=description_value,
-            icon=icon_value,
-            rule=rule_storage,
-            windows=window_specs,
-        )
-    except ValueError as error:
-        # The DAO's missing-definition error names only the id; re-raise
-        # so the public contract names the field the caller passed (matching
-        # assign/unassign/deactivate).
-        message = str(error)
-        if message.startswith("quest definition "):
-            raise ValueError(f"definition_id: {message}") from error
-        raise
+    snapshot = await dao.edit_definition(
+        definition_id,
+        title=title_value,
+        description=description_value,
+        icon=icon_value,
+        rule=rule_storage,
+        windows=window_specs,
+    )
 
     decoded_rule = schedule_rule_from_storage(
         schedule_rule_storage_from_record(snapshot.rule)
@@ -347,205 +321,3 @@ async def edit_quest_definition(
         assignees=snapshot.assignees,
         windows=snapshot.windows,
     )
-
-
-async def assign_child(
-    database: NestQuestDatabase,
-    definition_id: int,
-    child_id: int,
-) -> list[ChildRecord]:
-    """Assign ``child_id`` to the definition (idempotent).
-
-    Validates that both ids are plain ints (bools and floats rejected)
-    and that the child exists and is active — the DAO enforces the
-    active/exists check inside its transaction, and also confirms the
-    definition exists.  Assigning an already-assigned child is a no-op
-    (no duplicate row).  Returns the definition's assignees read back
-    inside the DAO's same transaction, a consistent post-assignment
-    roster (a concurrent assign/unassign cannot race it after the
-    write).  Raises ValueError on any rejected argument.  Only the
-    ``quest_definition_assignees`` link is written; existing instances
-    and completion history are never touched.
-    """
-    _validate_definition_id(definition_id)
-    _validate_child_id(child_id)
-    dao = QuestDefinitionsDao(database)
-    try:
-        return await dao.add_assignee(definition_id, child_id)
-    except ValueError as error:
-        # The DAO names only the child/definition; re-raise so the
-        # public contract names the field the caller actually passed.
-        message = str(error)
-        if message.startswith("child "):
-            raise ValueError(f"child_id: {message}") from error
-        if message.startswith("quest definition "):
-            raise ValueError(f"definition_id: {message}") from error
-        raise
-
-
-async def unassign_child(
-    database: NestQuestDatabase,
-    definition_id: int,
-    child_id: int,
-) -> bool:
-    """Unassign ``child_id`` from the definition; True when removed.
-
-    Validates both ids are plain ints (bools and floats rejected) and
-    that the definition exists, then removes the (definition, child)
-    link.  Returns True when a row was removed and False when the child
-    was not currently assigned.  Raises ValueError on a rejected
-    argument.  Only the ``quest_definition_assignees`` link is written;
-    existing instances and completion history are never touched.
-    """
-    _validate_definition_id(definition_id)
-    _validate_child_id(child_id)
-    dao = QuestDefinitionsDao(database)
-    definition = await dao.get(definition_id)
-    if definition is None:
-        raise ValueError(
-            f"definition_id: quest definition {definition_id} does not exist"
-        )
-    return await dao.remove_assignee(definition_id, child_id)
-
-
-async def set_quest_definition_active(
-    database: NestQuestDatabase,
-    definition_id: int,
-    is_active: bool,
-) -> CreatedQuestDefinition:
-    """Deactivate or reactivate a definition; returns the updated view.
-
-    Flipping the flag is the only removal path (Feature 06 guardrail):
-    a deactivated definition stops future instance generation but its
-    row, schedule rule, assignees and windows all survive, and existing
-    instances plus completion history are never touched.  Reactivation
-    re-enables future generation; generating or removing instances is
-    Feature 07 and does not happen here.
-
-    ``definition_id`` must be a plain int (bools and floats rejected,
-    since SQLite would bind ``True`` onto definition 1) and
-    ``is_active`` must be a real bool — the DAO's ``int()`` would
-    otherwise silently coerce strings and numerics ("1", 0) into a
-    state the caller never asked for.  Raises ValueError naming the
-    offending field when the id is malformed, ``is_active`` is not a
-    bool, or the definition does not exist.
-
-    Atomicity: the existence check, the UPDATE and the readback of the
-    definition, its rule, assignees and windows all run inside the
-    connection-scoped lock, so the returned bundle is a consistent
-    snapshot that a concurrent mutation cannot race.
-    """
-    _validate_definition_id(definition_id)
-    if not isinstance(is_active, bool):
-        raise ValueError(f"is_active must be a real bool, got {is_active!r}")
-    async with _connection_lock(database):
-        dao = QuestDefinitionsDao(database)
-        if await dao.get(definition_id) is None:
-            raise ValueError(
-                f"definition_id: quest definition {definition_id} "
-                "does not exist"
-            )
-        await dao.set_active(definition_id, is_active)
-        updated = await dao.get(definition_id)
-        assert updated is not None
-        rule_record = await ScheduleRulesDao(database).get(
-            updated.schedule_rule_id
-        )
-        assert rule_record is not None
-        assignees = await dao.list_assignees(definition_id)
-        windows = await dao.list_windows(definition_id)
-    rule = schedule_rule_from_storage(
-        schedule_rule_storage_from_record(rule_record)
-    )
-    return CreatedQuestDefinition(
-        definition=updated,
-        rule=rule,
-        assignees=assignees,
-        windows=windows,
-    )
-
-
-def _bundle_snapshot(
-    snapshot: QuestDefinitionSnapshot,
-) -> CreatedQuestDefinition:
-    """Decode a DAO snapshot into the layer's rich view.
-
-    The rule record carried by the snapshot is the one the definition
-    referenced at read time, so decoding it here (rather than re-reading
-    through :class:`~.dao_rules.ScheduleRulesDao`) preserves the
-    coherence the DAO's locked snapshot guarantees: the rule, assignees
-    and windows all come from one atomic read, never three separate ones.
-    """
-    rule = schedule_rule_from_storage(
-        schedule_rule_storage_from_record(snapshot.rule)
-    )
-    return CreatedQuestDefinition(
-        definition=snapshot.definition,
-        rule=rule,
-        assignees=snapshot.assignees,
-        windows=snapshot.windows,
-    )
-
-
-async def list_active_definitions(
-    database: NestQuestDatabase,
-) -> list[CreatedQuestDefinition]:
-    """Return every ACTIVE definition with its decoded rule and links.
-
-    Ordered by rising definition id, matching the DAO's ``list_active``
-    order, so callers get a stable, repeatable sequence.  Each entry is
-    the same rich :class:`CreatedQuestDefinition` snapshot as the rest of
-    the layer: the stored row plus its decoded
-    :class:`~.recurrence.ScheduleRule`, assignees and windows.  The
-    definitions and their links are read by the DAO inside one locked
-    transaction, so each bundle is a coherent snapshot that a concurrent
-    edit cannot interleave.
-    """
-    dao = QuestDefinitionsDao(database)
-    snapshots = await dao.list_snapshots_active()
-    return [_bundle_snapshot(snapshot) for snapshot in snapshots]
-
-
-async def list_definitions_for_child(
-    database: NestQuestDatabase,
-    child_id: int,
-) -> list[CreatedQuestDefinition]:
-    """Return the definitions assigned to ``child_id`` (rich view).
-
-    ``child_id`` must be a plain int (bools and floats rejected, since
-    SQLite would bind ``True`` onto child 1).  Results are ordered by
-    rising definition id, matching ``list_active_definitions`` and
-    ``list_definitions_firing_on``.  Each entry is a
-    :class:`CreatedQuestDefinition` snapshot bundling the definition with
-    its decoded rule, assignees and windows, read atomically by the DAO.
-    """
-    _validate_child_id(child_id)
-    dao = QuestDefinitionsDao(database)
-    snapshots = await dao.list_snapshots_by_child(child_id)
-    bundles = [_bundle_snapshot(snapshot) for snapshot in snapshots]
-    return sorted(bundles, key=lambda bundle: bundle.definition.id)
-
-
-async def list_definitions_firing_on(
-    database: NestQuestDatabase,
-    date: str,
-) -> list[CreatedQuestDefinition]:
-    """Return the ACTIVE definitions whose rule fires on ``date``.
-
-    ``date`` must be a strict ISO calendar date (YYYY-MM-DD); a malformed
-    value raises ValueError naming the field.  Each active definition's
-    stored rule is decoded through the task-1 storage mapping and handed
-    to the recurrence engine's :func:`~.recurrence.occurs_on`; only the
-    definitions whose decoded rule fires on the date are returned, in
-    rising definition-id order, each as the same rich
-    :class:`CreatedQuestDefinition` snapshot as the rest of the layer.
-    The definitions and their rules are read inside one locked
-    transaction, so a bundle can never filter on one rule while
-    presenting another state's windows.
-    """
-    _validate_date(date, "date")
-    target = datetime.datetime.strptime(date, "%Y-%m-%d").date()
-    dao = QuestDefinitionsDao(database)
-    snapshots = await dao.list_snapshots_active()
-    bundles = [_bundle_snapshot(snapshot) for snapshot in snapshots]
-    return [bundle for bundle in bundles if occurs_on(bundle.rule, target)]