Add hybrid admin delete for quest definitions

Owner decision 2026-09-26: "delete if there's no history, retire/deactivate if there is". A definition with no completion event on any instance is hard-deleted (instances, assignees, windows, the row and its unreferenced rule) in one transaction under the connection lock; one with history is retired via set_quest_definition_active and everything is preserved (D-005). - core: QuestDefinitionsDao.delete_if_no_history (atomic history check + deletes) and quest_definitions.delete_quest_definition - api: DELETE /api/v1/admin/quest-definitions/{id} on the admin router - admin: Delete task control with a hybrid-explaining confirm in the edit sheet; the list removes or shows the row inactive - docs: ADMIN-SPEC Definitions section Task 3efb07b4-7bfa-45af-b36a-57d81249a39d Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-09-26 12:31:10 -04:00
parent b1c14ed7d9
commit ed230961e0
11 changed files with 792 additions and 41 deletions
@@ -13,8 +13,12 @@ Guardrail mapping:
 - ``schedule_rules`` deletion is delete-if-unreferenced: a rule still
  referenced by any quest definition is rejected (the done-condition),
  not cascaded.
- ``quest_definitions`` has no delete path at all: definitions are
-  deactivated, never hard-deleted, and history must survive.
+- ``quest_definitions`` has exactly one delete path,
+  :meth:`QuestDefinitionsDao.delete_if_no_history` (owner decision
+  2026-09-26): a definition with NO completion event on any of its
+  instances is hard-deleted with its instances, assignees, windows and
+  (if unreferenced) rule; a definition WITH history is never deleted —
+  it is retired by deactivation, so history always survives.
 - Assignment is multi-assignee (D-008): :meth:`add_assignee` and
  :meth:`remove_assignee` change future instances only — the DAO writes
  the assignee table and nothing else; assignment edits never rewrite
@@ -25,8 +29,8 @@ Guardrail mapping:
  :meth:`QuestDefinitionsDao.add_assignee` (active child); no
  permission checks here, that is the Feature 09 gate's job.

-Concurrency: ``delete_rule_if_unreferenced`` and the assignee
-mutations perform their checks and writes inside one transaction under
+Concurrency: ``delete_rule_if_unreferenced``,
+``delete_if_no_history`` and the assignee mutations perform their checks and writes inside one transaction under
 the connection-scoped lock shared with :mod:`.dao_children`, so a
 definition referencing the rule (or an assignee appearing) cannot slip
 in between check and write on this connection.
@@ -608,26 +612,42 @@ class ScheduleRulesDao:
        """
        async with _connection_lock(self._database):
            async with self._database.transaction():
-                row = await self._database.fetch_one(
-                    "SELECT 1 FROM quest_definitions "
-                    "WHERE schedule_rule_id = ? LIMIT 1",
-                    (rule_id,),
+                return await _delete_rule_if_unreferenced_in_transaction(
+                    self._database, rule_id
                )
-                if row is not None:
-                    return False
-                result = await self._database.execute(
-                    "DELETE FROM schedule_rules WHERE id = ?",
-                    (rule_id,),
-                )
-                return result.rowcount > 0
+
+
+async def _delete_rule_if_unreferenced_in_transaction(
+    database: NestQuestDatabase, rule_id: int
+) -> bool:
+    """Delete-if-unreferenced body; the CALLER holds lock + transaction.
+
+    Shared by :meth:`ScheduleRulesDao.delete_if_unreferenced` and
+    :meth:`QuestDefinitionsDao.delete_if_no_history`, so the rule's
+    reference check runs in whichever transaction removed the last
+    definition that could reference it.
+    """
+    row = await database.fetch_one(
+        "SELECT 1 FROM quest_definitions "
+        "WHERE schedule_rule_id = ? LIMIT 1",
+        (rule_id,),
+    )
+    if row is not None:
+        return False
+    result = await database.execute(
+        "DELETE FROM schedule_rules WHERE id = ?",
+        (rule_id,),
+    )
+    return result.rowcount > 0


 class QuestDefinitionsDao:
    """Typed async access to ``quest_definitions`` and its assignees.

-    No delete method exists: definitions are deactivated via
-    :meth:`set_active`, never hard-deleted, so completion history keeps
-    its references (feature guardrail).  Assignment is multi-assignee
+    The only delete method is :meth:`delete_if_no_history`: a
+    definition with completion history is never hard-deleted (it is
+    deactivated via :meth:`set_active` instead), so completion history
+    always keeps its references (feature guardrail, D-005).  Assignment is multi-assignee
    (D-008): the definition row carries no child; assignees live in
    ``quest_definition_assignees`` and are managed explicitly so the
    operations stay separately loggable and permission-gateable.
@@ -1140,6 +1160,54 @@ class QuestDefinitionsDao:
        )
        return result.rowcount

+    async def delete_if_no_history(self, definition_id: int) -> bool | None:
+        """Hard-delete a definition that has NO completion history.
+
+        Returns None when the definition does not exist, False when any
+        of its instances has a completion event (nothing is touched —
+        the caller retires it by deactivation instead), and True when it
+        was deleted: its ``quest_instances``, assignee and window rows,
+        the definition row, and its schedule rule if no other definition
+        references it.
+
+        The history check and every DELETE run inside ONE transaction
+        under the connection lock, so a completion recorded concurrently
+        cannot land between the check and the delete (D-005: an instance
+        with a completion event is never deleted).
+        """
+        async with _connection_lock(self._database):
+            async with self._database.transaction():
+                row = await self._database.fetch_one(
+                    "SELECT schedule_rule_id FROM quest_definitions "
+                    "WHERE id = ?",
+                    (definition_id,),
+                )
+                if row is None:
+                    return None
+                schedule_rule_id = row[0]
+                history = await self._database.fetch_one(
+                    "SELECT 1 FROM completion_events "
+                    "JOIN quest_instances "
+                    "ON quest_instances.id = completion_events.instance_id "
+                    "WHERE quest_instances.definition_id = ? LIMIT 1",
+                    (definition_id,),
+                )
+                if history is not None:
+                    return False
+                for statement in (
+                    "DELETE FROM quest_instances WHERE definition_id = ?",
+                    "DELETE FROM quest_definition_assignees "
+                    "WHERE definition_id = ?",
+                    "DELETE FROM quest_definition_windows "
+                    "WHERE definition_id = ?",
+                    "DELETE FROM quest_definitions WHERE id = ?",
+                ):
+                    await self._database.execute(statement, (definition_id,))
+                await _delete_rule_if_unreferenced_in_transaction(
+                    self._database, schedule_rule_id
+                )
+                return True
+
    async def add_assignee(
        self, definition_id: int, child_id: int
    ) -> list[ChildRecord]:
@@ -7,7 +7,9 @@ all-or-nothing create/edit here, and never touch the DAO SQL directly.
 :func:`edit_quest_definition` the edit path (which may also replace the
 whole assignee set), :func:`assign_child` and
 :func:`unassign_child` the single-child assignment paths, and
-:func:`set_quest_definition_active` the deactivate/reactivate path.
+:func:`set_quest_definition_active` the deactivate/reactivate path,
+and :func:`delete_quest_definition` the hybrid admin delete (hard-delete
+when there is no completion history, retire by deactivation otherwise).
 There is no permission check here (that is Feature 09).

 Validation policy mirrors :mod:`.children`:
@@ -80,6 +82,21 @@ class CreatedQuestDefinition:
    windows: list[QuestDefinitionWindowRecord]


+@dataclass(frozen=True)
+class DeletedQuestDefinition:
+    """What :func:`delete_quest_definition` actually did.
+
+    Exactly one of ``deleted`` / ``retired`` is True.  ``definition`` is
+    the retired (deactivated) definition's updated view, or None when
+    the definition was hard-deleted and no longer exists.
+    """
+
+    definition_id: int
+    deleted: bool
+    retired: bool
+    definition: CreatedQuestDefinition | None
+
+
 def _now_stamp() -> str:
    """Return the module's strict UTC second-precision stamp."""
    return datetime.datetime.now(datetime.timezone.utc).isoformat(
@@ -504,7 +521,9 @@ async def set_quest_definition_active(
 ) -> CreatedQuestDefinition:
    """Deactivate or reactivate a definition; returns the updated view.

-    Flipping the flag is the only removal path (Feature 06 guardrail):
+    Flipping the flag is the only removal path for a definition with
+    completion history (Feature 06 guardrail; a history-free definition
+    may instead be hard-deleted by :func:`delete_quest_definition`):
    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
@@ -562,6 +581,63 @@ async def set_quest_definition_active(
    )


+async def delete_quest_definition(
+    database: NestQuestDatabase,
+    definition_id: int,
+    *,
+    today: datetime.date | None = None,
+    horizon_days: int | None = None,
+) -> DeletedQuestDefinition:
+    """Delete a definition if it has no history; otherwise retire it.
+
+    Owner decision (2026-09-26): "delete if there's no history,
+    retire/deactivate if there is".  When NO instance of the definition
+    has a completion event, the definition, its instances, assignees,
+    windows and (if no other definition references it) its schedule
+    rule are hard-deleted in ONE transaction by
+    :meth:`~.dao_rules.QuestDefinitionsDao.delete_if_no_history` — the
+    history check is inside that transaction, so a completion recorded
+    concurrently can never be deleted (D-005).  When history exists,
+    nothing is deleted: the definition is retired through
+    :func:`set_quest_definition_active` (``False``), preserving the row,
+    rule, assignees, windows, instances and completion history.
+
+    ``definition_id`` must be a plain int (bools and floats rejected).
+    Raises ValueError naming the field when the id is malformed or the
+    definition does not exist.  ``today`` / ``horizon_days`` are threaded
+    into the retire path's regeneration, as in
+    :func:`set_quest_definition_active`.
+    """
+    _validate_definition_id(definition_id)
+    deleted = await QuestDefinitionsDao(database).delete_if_no_history(
+        definition_id
+    )
+    if deleted is None:
+        raise ValueError(
+            f"definition_id: quest definition {definition_id} does not exist"
+        )
+    if deleted:
+        return DeletedQuestDefinition(
+            definition_id=definition_id,
+            deleted=True,
+            retired=False,
+            definition=None,
+        )
+    retired = await set_quest_definition_active(
+        database,
+        definition_id,
+        False,
+        today=today,
+        horizon_days=horizon_days,
+    )
+    return DeletedQuestDefinition(
+        definition_id=definition_id,
+        deleted=False,
+        retired=True,
+        definition=retired,
+    )
+
+
 def _bundle_snapshot(
    snapshot: QuestDefinitionSnapshot,
 ) -> CreatedQuestDefinition: