From ed230961e0314f181b3eafcb3ee148ae18d05133 Mon Sep 17 00:00:00 2001 From: Joshua Date: Sat, 26 Sep 2026 12:31:10 -0400 Subject: [PATCH 1/3] 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 --- admin/src/api/definitions.ts | 19 ++ admin/src/definitions/DefinitionsTab.css | 20 ++ admin/src/definitions/DefinitionsTab.test.tsx | 89 ++++++++- admin/src/definitions/DefinitionsTab.tsx | 102 +++++++++- api/routes_admin.py | 72 ++++++- custom_components/nestquest/core/dao_rules.py | 104 ++++++++-- .../nestquest/core/quest_definitions.py | 80 +++++++- design/ADMIN-SPEC.md | 14 +- tests/test_api_admin_questdef.py | 138 +++++++++++++ tests/test_dao_rules.py | 9 +- tests/test_quest_definitions.py | 186 ++++++++++++++++++ 11 files changed, 792 insertions(+), 41 deletions(-) diff --git a/admin/src/api/definitions.ts b/admin/src/api/definitions.ts index ce7a513..7c6405f 100644 --- a/admin/src/api/definitions.ts +++ b/admin/src/api/definitions.ts @@ -147,6 +147,25 @@ export async function updateDefinition( return readJson(await sendJson(`${DEFINITIONS_PATH}/${id}`, "PATCH", body)); } +/** `DELETE /quest-definitions/{id}`: exactly one of `deleted` / `retired` is true. */ +export interface DefinitionDeleteResult { + definition_id: number; + deleted: boolean; + retired: boolean; + /** The retired (now inactive) definition; null when it was deleted. */ + definition: QuestDefinition | null; +} + +/** + * Delete a task if it has no completion history; otherwise the API retires + * (deactivates) it and everything is preserved. + */ +export async function deleteDefinition(id: number): Promise { + return readJson( + await apiFetch(`${DEFINITIONS_PATH}/${id}`, { method: "DELETE" }), + ); +} + export const OCCURRENCES_PREVIEW_PATH = `${DEFINITIONS_PATH}/occurrences-preview`; export interface OccurrencesPreviewOptions { diff --git a/admin/src/definitions/DefinitionsTab.css b/admin/src/definitions/DefinitionsTab.css index bd47f8a..7442c83 100644 --- a/admin/src/definitions/DefinitionsTab.css +++ b/admin/src/definitions/DefinitionsTab.css @@ -569,6 +569,26 @@ color: #fff; } +.defs-save--danger { + background: var(--nq-a-danger); +} + +.defs-delete { + display: block; + width: 100%; + margin-top: 10px; + padding: 11px 0; + border: 1px solid var(--nq-a-danger-bg); + border-radius: var(--nq-a-density-radius); + background: var(--nq-a-surface); + color: var(--nq-a-danger-strong); + font-family: inherit; + font-size: 13px; + font-weight: 700; + cursor: pointer; +} + +.defs-delete:disabled, .defs-cancel:disabled, .defs-save:disabled { opacity: 0.6; diff --git a/admin/src/definitions/DefinitionsTab.test.tsx b/admin/src/definitions/DefinitionsTab.test.tsx index 7ee2a0d..3a24638 100644 --- a/admin/src/definitions/DefinitionsTab.test.tsx +++ b/admin/src/definitions/DefinitionsTab.test.tsx @@ -121,6 +121,8 @@ function fakeApi(initial: QuestDefinition[]) { const calls: Call[] = []; let nextWrite: Response | null = null; let preview: PreviewHandler = async () => jsonResponse({ dates: [] }); + // Definitions with completion history: the API retires these on DELETE. + const withHistory = new Set(); const fetchMock = vi.fn(async (input: RequestInfo | URL, init: RequestInit = {}) => { const path = new URL(String(input), "http://localhost").pathname; const method = init.method ?? "GET"; @@ -187,6 +189,16 @@ function fakeApi(initial: QuestDefinition[]) { ); return jsonResponse(definitions.find((d) => d.id === id)); } + if (method === "DELETE" && match) { + const id = Number(match[1]); + if (withHistory.has(id)) { + definitions = definitions.map((d) => (d.id === id ? { ...d, is_active: false } : d)); + const retired = definitions.find((d) => d.id === id); + return jsonResponse({ definition_id: id, deleted: false, retired: true, definition: retired }); + } + definitions = definitions.filter((d) => d.id !== id); + return jsonResponse({ definition_id: id, deleted: true, retired: false, definition: null }); + } return jsonResponse({ detail: "unexpected" }, 500); }); return { @@ -203,6 +215,9 @@ function fakeApi(initial: QuestDefinition[]) { failNextWrite: (response: Response) => { nextWrite = response; }, + markHistory: (id: number) => { + withHistory.add(id); + }, }; } @@ -578,14 +593,15 @@ describe("DefinitionsTab", () => { fireEvent.click(screen.getByTestId("definition-10")); const s = sheet(); const title = within(s).getByRole("textbox", { name: "Title" }); - const save = within(s).getByRole("button", { name: "Save changes" }); + // The edit sheet's last focusable control is its Delete button. + const last = within(s).getByRole("button", { name: "Delete task" }); - save.focus(); + last.focus(); fireEvent.keyDown(s, { key: "Tab" }); expect(document.activeElement).toBe(title); fireEvent.keyDown(s, { key: "Tab", shiftKey: true }); - expect(document.activeElement).toBe(save); + expect(document.activeElement).toBe(last); }); it("Escape does not close the sheet while saving", async () => { @@ -694,6 +710,73 @@ describe("DefinitionsTab", () => { ); }); + it("the Delete control is only offered for an existing task", async () => { + await renderReady(); + fireEvent.click(screen.getByRole("button", { name: "New" })); + expect(within(sheet()).queryByRole("button", { name: "Delete task" })).toBeNull(); + fireEvent.click(within(sheet()).getByRole("button", { name: "Cancel" })); + fireEvent.click(screen.getByTestId("definition-10")); + expect(within(sheet()).getByRole("button", { name: "Delete task" })).toBeTruthy(); + }); + + it("Delete asks first, explaining delete-vs-retire; Go back writes nothing", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-10")); + fireEvent.click(within(sheet()).getByRole("button", { name: "Delete task" })); + const confirm = screen.getByTestId("delete-confirm"); + expect(confirm.textContent).toContain("If it has no completion history it is deleted"); + expect(confirm.textContent).toContain("otherwise it is retired"); + expect(api.writes()).toHaveLength(0); + fireEvent.click(within(confirm).getByRole("button", { name: "Go back" })); + expect(screen.queryByTestId("delete-confirm")).toBeNull(); + expect(api.writes()).toHaveLength(0); + }); + + it("confirming Delete on a task without history removes its row", async () => { + await renderReady(); + fireEvent.click(screen.getByTestId("definition-10")); + fireEvent.click(within(sheet()).getByRole("button", { name: "Delete task" })); + fireEvent.click( + within(screen.getByTestId("delete-confirm")).getByRole("button", { name: "Delete task" }), + ); + await waitFor(() => expect(screen.queryByTestId("definition-10")).toBeNull()); + expect(screen.queryByRole("dialog")).toBeNull(); + expect(api.writes()).toEqual([ + { method: "DELETE", path: "/api/v1/admin/quest-definitions/10", body: null }, + ]); + expect(screen.getByTestId("definition-11")).toBeTruthy(); + }); + + it("confirming Delete on a task with history shows it retired (inactive)", async () => { + await renderReady(); + api.markHistory(10); + fireEvent.click(screen.getByTestId("definition-10")); + fireEvent.click(within(sheet()).getByRole("button", { name: "Delete task" })); + fireEvent.click( + within(screen.getByTestId("delete-confirm")).getByRole("button", { name: "Delete task" }), + ); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + const row = screen.getByTestId("definition-10"); + expect(row.className).toContain("defs-row--inactive"); + expect(meta(row)).toContain("· inactive"); + expect(api.writes().map((c) => c.method)).toEqual(["DELETE"]); + }); + + it("a failed Delete shows the API's detail and keeps the sheet open", async () => { + await renderReady(); + api.failNextWrite(jsonResponse({ detail: "Quest definition not found" }, 404)); + fireEvent.click(screen.getByTestId("definition-10")); + fireEvent.click(within(sheet()).getByRole("button", { name: "Delete task" })); + fireEvent.click( + within(screen.getByTestId("delete-confirm")).getByRole("button", { name: "Delete task" }), + ); + expect((await screen.findByTestId("sheet-error")).textContent).toBe( + "Quest definition not found", + ); + expect(screen.getByRole("dialog")).toBeTruthy(); + expect(screen.getByTestId("definition-10")).toBeTruthy(); + }); + it("scroll column carries the 92px bottom padding", async () => { await renderReady(); expect(screen.getByTestId("definitions-scroll").style.paddingBottom).toBe("92px"); diff --git a/admin/src/definitions/DefinitionsTab.tsx b/admin/src/definitions/DefinitionsTab.tsx index 4ac7748..a3ff02a 100644 --- a/admin/src/definitions/DefinitionsTab.tsx +++ b/admin/src/definitions/DefinitionsTab.tsx @@ -13,12 +13,14 @@ import { ApiForbiddenError } from "../api/client"; import { ApiRequestError, createDefinition, + deleteDefinition, fetchChildren, fetchDefinitions, previewOccurrences, updateDefinition, type AdminChild, type DefinitionCreateBody, + type DefinitionDeleteResult, type DefinitionEditBody, type DefinitionRule, type QuestDefinition, @@ -273,6 +275,12 @@ function saveErrorMessage(error: unknown): string { return "The task could not be saved. Check your connection and try again."; } +function deleteErrorMessage(error: unknown): string { + if (error instanceof ApiForbiddenError) return error.message; + if (error instanceof ApiRequestError && error.detail) return error.detail; + return "The task could not be deleted. Check your connection and try again."; +} + function toggle(list: T[], value: T): T[] { return list.includes(value) ? list.filter((item) => item !== value) : [...list, value]; } @@ -449,11 +457,13 @@ function EditSheet({ activeChildren, onCancel, onSaved, + onDeleted, }: { target: "new" | QuestDefinition; activeChildren: AdminChild[]; onCancel: () => void; onSaved: () => void; + onDeleted: (result: DefinitionDeleteResult) => void; }) { const [draft, setDraft] = useState(() => target === "new" ? newDraft(activeChildren) : draftFrom(target, activeChildren), @@ -473,6 +483,7 @@ function EditSheet({ const [pickerOpen, setPickerOpen] = useState(false); const [saving, setSaving] = useState(false); const [confirming, setConfirming] = useState(false); + const [confirmingDelete, setConfirmingDelete] = useState(false); const [error, setError] = useState(null); // A ref, not state: two taps in one frame must not both see `saving === false`. const inFlight = useRef(false); @@ -490,8 +501,8 @@ function EditSheet({ }, []); useEffect(() => { - if (confirming) confirmRef.current?.focus(); - }, [confirming]); + if (confirming || confirmingDelete) confirmRef.current?.focus(); + }, [confirming, confirmingDelete]); function onKeyDown(event: ReactKeyboardEvent) { if (event.key === "Escape") { @@ -559,6 +570,21 @@ function EditSheet({ } } + async function remove() { + if (target === "new" || inFlight.current) return; + setConfirmingDelete(false); + inFlight.current = true; + setSaving(true); + setError(null); + try { + onDeleted(await deleteDefinition(target.id)); + } catch (err) { + setError(deleteErrorMessage(err)); + inFlight.current = false; + setSaving(false); + } + } + return (
- ) : ( -
- - + ) : confirmingDelete ? ( +
+

+ Delete this task? If it has no completion history it is deleted; otherwise it is + retired (deactivated) and its history is kept. +

+
+ + +
+ ) : ( + <> +
+ + +
+ {target !== "new" ? ( + + ) : null} + )}
@@ -1038,6 +1105,21 @@ export default function DefinitionsTab() { setSheet(null); setAttempt((n) => n + 1); }} + onDeleted={(result) => { + setSheet(null); + setState((current) => + current.kind !== "ready" + ? current + : { + ...current, + definitions: result.definition + ? current.definitions.map((d) => + d.id === result.definition_id ? result.definition! : d, + ) + : current.definitions.filter((d) => d.id !== result.definition_id), + }, + ); + }} /> ) : null} diff --git a/api/routes_admin.py b/api/routes_admin.py index c67a269..0dc64c2 100644 --- a/api/routes_admin.py +++ b/api/routes_admin.py @@ -200,6 +200,19 @@ integration's rollover listener shares: and the read-only-over-the-domain-tables guarantee all live in the core; the response reports how many transitions were published. +The quest-definition DELETE route (task 3efb07b4) is the hybrid admin +delete (owner decision 2026-09-26: "delete if there's no history, +retire/deactivate if there is"): + +- ``DELETE /quest-definitions/{definition_id}`` is a thin adapter over + :func:`nestquest_core.quest_definitions.delete_quest_definition`. A + definition with NO completion event on any instance is hard-deleted + (instances, assignees, windows, the row and its unreferenced rule, in + one core transaction); one WITH history is retired by deactivation + and everything is preserved. The response reports which happened + (``deleted`` / ``retired``) and carries the retired definition, or + ``null`` when deleted. An unknown id is 404. + The occurrence-preview route (task 5f843564) lets the admin PWA's Definitions edit sheet show the next dates a rule fires on WITHOUT reimplementing the recurrence rules client-side: @@ -453,6 +466,19 @@ class AdminQuestDefinitionListResponse(BaseModel): definitions: list[AdminQuestDefinitionResponse] +class AdminQuestDefinitionDeleteResponse(BaseModel): + """The body ``DELETE /api/v1/admin/quest-definitions/{id}`` answers. + + Exactly one of ``deleted`` / ``retired`` is true. ``definition`` is + the retired (now inactive) definition, or null when it was deleted. + """ + + definition_id: int + deleted: bool + retired: bool + definition: AdminQuestDefinitionResponse | None + + class AdminChildCreateRequest(BaseModel): """The body of ``POST /api/v1/admin/children``. @@ -1611,16 +1637,16 @@ async def admin_set_quest_definition_active( body: AdminQuestDefinitionActiveRequest, request: Request, ) -> AdminQuestDefinitionResponse: - """Deactivate (or reactivate) a definition; it is NEVER deleted. + """Deactivate (or reactivate) a definition; it is never deleted here. Requires a valid admin JWT (the router's shared :func:`~api.auth.require_admin` dependency — the ONE check; this handler performs NO auth of its own). Thin adapter: ``is_active`` goes straight to :func:`nestquest_core.quest_definitions.set_quest_definition_active` - — deactivation is the only removal path (the row, rule, assignees - and windows all survive, so completion history keeps its - references). An unknown definition id is mapped to 404 + — the row, rule, assignees and windows all survive, so completion + history keeps its references (the hybrid DELETE route is the only + hard-delete path, and only for a definition without history). An unknown definition id is mapped to 404 (:func:`_raise_quest_definition_error`); the body model already makes the core's real-bool check unreachable from this route. """ @@ -1634,6 +1660,44 @@ async def admin_set_quest_definition_active( return _quest_definition_response(bundle) +@router.delete( + "/quest-definitions/{definition_id}", + summary="Delete a quest definition, or retire it if it has history", + response_model=AdminQuestDefinitionDeleteResponse, +) +async def admin_delete_quest_definition( + definition_id: int, request: Request +) -> AdminQuestDefinitionDeleteResponse: + """Hard-delete a history-free definition; retire one with history. + + Requires a valid admin JWT (the router's shared + :func:`~api.auth.require_admin` dependency — the ONE check; this + handler performs NO auth of its own). Thin adapter: the id goes + straight to + :func:`nestquest_core.quest_definitions.delete_quest_definition`, + which decides delete-vs-retire atomically in the core. An unknown + definition id is mapped to 404 + (:func:`_raise_quest_definition_error`). + """ + state: DatabaseState = request.app.state.db + try: + result = await core_quest_definitions.delete_quest_definition( + state.database, definition_id + ) + except ValueError as error: + _raise_quest_definition_error(error) + return AdminQuestDefinitionDeleteResponse( + definition_id=result.definition_id, + deleted=result.deleted, + retired=result.retired, + definition=( + None + if result.definition is None + else _quest_definition_response(result.definition) + ), + ) + + @router.get( "/children/{child_id}/presence-patterns", summary="List a child's repeating presence patterns", diff --git a/custom_components/nestquest/core/dao_rules.py b/custom_components/nestquest/core/dao_rules.py index c4a5472..38d0043 100644 --- a/custom_components/nestquest/core/dao_rules.py +++ b/custom_components/nestquest/core/dao_rules.py @@ -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]: diff --git a/custom_components/nestquest/core/quest_definitions.py b/custom_components/nestquest/core/quest_definitions.py index 58903eb..1e07692 100644 --- a/custom_components/nestquest/core/quest_definitions.py +++ b/custom_components/nestquest/core/quest_definitions.py @@ -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: diff --git a/design/ADMIN-SPEC.md b/design/ADMIN-SPEC.md index 797668b..77cab54 100644 --- a/design/ADMIN-SPEC.md +++ b/design/ADMIN-SPEC.md @@ -64,7 +64,8 @@ Header `Tasks` / `7 definitions · 3 children` + gradient `New` button. Filter c (All / one per child): active `#18181D` fill white text, inactive white with border. Rows: glyph 17px · title 13.5px/700 · meta 11px/600 ` · · ` · `chevron-right`. An inactive definition renders the whole row at -`--nq-a-ink-tertiary` with `· inactive` in the meta — never hard-deleted (glossary). +`--nq-a-ink-tertiary` with `· inactive` in the meta. A definition with completion +history is never hard-deleted (glossary, D-017): Delete retires it to this inactive state. The row being edited takes `--nq-a-selected` with brand-blue glyph, `Editing` meta and `chevron-down`. @@ -83,6 +84,17 @@ Bottom sheet, `--nq-a-radius-sheet`, `--nq-a-sheet-shadow`, 38×4px grabber, pad Footer: `Cancel` (flex 1, bordered) + `Save changes` (flex 2, gradient), both 13px vertical padding. Sheet enters over the dimmed list in `--nq-dur-modal`. +Delete (editing an existing definition only, never on `New`): a full-width bordered +`Delete task` button below the footer, danger-ink text. Tapping it swaps the footer for a +danger-tinted confirm — "Delete this task? If it has no completion history it is deleted; +otherwise it is retired (deactivated) and its history is kept." — with `Go back` and a +danger-filled `Delete task`. Confirming calls `DELETE /api/v1/admin/quest-definitions/{id}` +(admin plane only; the kids' panel has no equivalent control). Hybrid outcome (D-017): no +completion event on any instance → the definition, its instances, assignees, windows and +unreferenced rule are hard-deleted and the row leaves the list; otherwise nothing is +deleted, the definition is deactivated and the row stays, shown inactive. An error keeps +the sheet open with the API's detail. + ### 3.3 Icon picker Each definition carries one optional icon, shown on both surfaces. The picker offers three sources: diff --git a/tests/test_api_admin_questdef.py b/tests/test_api_admin_questdef.py index 8850b43..a27bc8c 100644 --- a/tests/test_api_admin_questdef.py +++ b/tests/test_api_admin_questdef.py @@ -817,3 +817,141 @@ async def test_set_active_malformed_body_is_422( headers=_admin_headers(), ) assert response.status_code == 422 + + +# --- delete: hybrid (delete if no history, retire if there is) -------------- + +_dao_instances = importlib.import_module("nestquest_core.dao_instances") + + +def test_delete_route_lives_on_the_admin_router() -> None: + """The DELETE route is on the admin router, which carries the ONE + require_admin dependency — no second auth check is added.""" + from api.auth import require_admin + from api.routes_admin import router as admin_router + + assert require_admin in [d.dependency for d in admin_router.dependencies] + matches = [ + route + for route in admin_router.routes + if route.path == "/api/v1/admin/quest-definitions/{definition_id}" + and "DELETE" in route.methods + ] + assert len(matches) == 1 + + +async def test_delete_refuses_non_admin_and_absent( + admin_questdefs: SimpleNamespace, +) -> None: + """A non-admin JWT is 403 and an absent credential 401: nothing is + deleted or retired.""" + client = admin_questdefs.client + ada = (await _create_children(client, ("Ada",)))[0] + definition_id = (await _create_definition(client, [ada])).json()["id"] + non_admin = make_token(groups=("some-other-group",)) + + forbidden = await client.delete( + f"/api/v1/admin/quest-definitions/{definition_id}", + headers={"Authorization": f"Bearer {non_admin}"}, + ) + assert forbidden.status_code == 403 + unauthenticated = await client.delete( + f"/api/v1/admin/quest-definitions/{definition_id}" + ) + assert unauthenticated.status_code == 401 + + record = await _dao_rules.QuestDefinitionsDao( + admin_questdefs.database + ).get(definition_id) + assert record is not None + assert record.is_active is True + + +async def test_delete_without_history_hard_deletes( + admin_questdefs: SimpleNamespace, +) -> None: + """No completion history: 200 ``deleted`` and the row and its + unreferenced rule are gone.""" + client = admin_questdefs.client + ada = (await _create_children(client, ("Ada",)))[0] + definition_id = (await _create_definition(client, [ada])).json()["id"] + dao = _dao_rules.QuestDefinitionsDao(admin_questdefs.database) + rule_id = (await dao.get(definition_id)).schedule_rule_id + + response = await client.delete( + f"/api/v1/admin/quest-definitions/{definition_id}", + headers=_admin_headers(), + ) + + assert response.status_code == 200 + assert response.json() == { + "definition_id": definition_id, + "deleted": True, + "retired": False, + "definition": None, + } + assert await dao.get(definition_id) is None + assert await _dao_rules.ScheduleRulesDao( + admin_questdefs.database + ).get(rule_id) is None + + +async def test_delete_with_history_retires( + admin_questdefs: SimpleNamespace, +) -> None: + """Completion history present: 200 ``retired`` with the now-inactive + definition; the row, instance and history all remain.""" + client = admin_questdefs.client + database = admin_questdefs.database + ada = (await _create_children(client, ("Ada",)))[0] + definition_id = (await _create_definition(client, [ada])).json()["id"] + instance = await _dao_instances.QuestInstancesDao(database).upsert( + definition_id, + ada, + datetime.date.today().isoformat(), + "2026-09-14T12:00:00+00:00", + window="morning", + ) + events = _dao_instances.CompletionEventsDao(database) + event = await events.append( + instance.id, + ada, + "completed", + "user", + "2026-09-14T12:00:00+00:00", + True, + actor_user_id="admin-1", + ) + + response = await client.delete( + f"/api/v1/admin/quest-definitions/{definition_id}", + headers=_admin_headers(), + ) + + assert response.status_code == 200 + payload = response.json() + assert payload["definition_id"] == definition_id + assert payload["deleted"] is False + assert payload["retired"] is True + assert payload["definition"]["id"] == definition_id + assert payload["definition"]["is_active"] is False + assert payload["definition"]["title"] == "Tidy the den" + record = await _dao_rules.QuestDefinitionsDao(database).get(definition_id) + assert record is not None + assert record.is_active is False + assert await _dao_instances.QuestInstancesDao(database).get_by_id( + instance.id + ) == instance + assert await events.list_by_instance(instance.id) == [event] + + +async def test_delete_unknown_definition_is_404( + admin_questdefs: SimpleNamespace, +) -> None: + """A DELETE naming a definition that does not exist is 404.""" + response = await admin_questdefs.client.delete( + "/api/v1/admin/quest-definitions/999", + headers=_admin_headers(), + ) + assert response.status_code == 404 + assert response.json()["detail"] == "Quest definition not found" diff --git a/tests/test_dao_rules.py b/tests/test_dao_rules.py index 5986dbf..d2a8deb 100644 --- a/tests/test_dao_rules.py +++ b/tests/test_dao_rules.py @@ -996,8 +996,9 @@ def test_definition_set_active_missing_returns_zero(tmp_path) -> None: assert _with_db(tmp_path, "definition-set-active-missing.db")(_body) == 0 -def test_definition_no_delete_method_exists(tmp_path) -> None: - """Guardrail: definitions are deactivated, never hard-deleted.""" +def test_definition_only_delete_method_is_history_guarded(tmp_path) -> None: + """Guardrail: the ONLY definition delete path is the history-guarded + one (owner decision 2026-09-26); there is no unconditional delete.""" async def _body(database, rules, definitions, children, child): import inspect @@ -1008,7 +1009,9 @@ def test_definition_no_delete_method_exists(tmp_path) -> None: ) } assert "delete" not in methods - assert not any(name.startswith("delete") for name in methods) + assert { + name for name in methods if name.startswith("delete") + } == {"delete_if_no_history"} return methods _with_db(tmp_path, "definition-no-delete.db")(_body) diff --git a/tests/test_quest_definitions.py b/tests/test_quest_definitions.py index c344a1a..ee7f54c 100644 --- a/tests/test_quest_definitions.py +++ b/tests/test_quest_definitions.py @@ -22,8 +22,10 @@ from custom_components.nestquest.core.db import NestQuestDatabase from custom_components.nestquest.core.migrations import apply_migrations from custom_components.nestquest.core.quest_definitions import ( CreatedQuestDefinition, + DeletedQuestDefinition, assign_child, create_quest_definition, + delete_quest_definition, edit_quest_definition, list_active_definitions, list_definitions_firing_on, @@ -2126,3 +2128,187 @@ def test_wrappers_forward_horizon_days(tmp_path, mode) -> None: return None _with_db(tmp_path, f"wrapper-horizon-{mode}.db")(_body) + + +# --------------------------------------------------------------------------- +# delete_quest_definition: hybrid delete (owner decision 2026-09-26) +# --------------------------------------------------------------------------- + + +async def _count(database, sql, definition_id) -> int: + row = await database.fetch_one(sql, (definition_id,)) + return row[0] + + +def test_delete_without_history_hard_deletes_everything(tmp_path) -> None: + """No completion event on any instance: the definition, its + instances, assignees, windows and unreferenced rule are all gone.""" + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id], + ["morning", "evening"], + ) + definition_id = created.definition.id + rule_id = created.definition.schedule_rule_id + instances_sql = ( + "SELECT COUNT(*) FROM quest_instances WHERE definition_id = ?" + ) + # Create materialized future instances, so the delete has + # instance rows to remove (not just an empty table). + assert await _count(database, instances_sql, definition_id) > 0 + + result = await delete_quest_definition(database, definition_id) + + assert result == DeletedQuestDefinition( + definition_id=definition_id, + deleted=True, + retired=False, + definition=None, + ) + assert await definitions.get(definition_id) is None + assert await rules.get(rule_id) is None + assert await _count(database, instances_sql, definition_id) == 0 + for table in ( + "quest_definition_assignees", + "quest_definition_windows", + ): + assert await _count( + database, + f"SELECT COUNT(*) FROM {table} WHERE definition_id = ?", + definition_id, + ) == 0 + # The child itself is untouched. + assert (await children.get(child.id)) is not None + return None + + _with_db(tmp_path, "delete-no-history.db")(_body) + + +def test_delete_without_history_keeps_a_shared_rule(tmp_path) -> None: + """The rule is deleted only when no other definition references it.""" + async def _body(database, children, rules, definitions, child): + rule = await rules.create("daily", "2026-09-14") + first = await definitions.create( + "First", rule.id, NOW, assignee_child_ids=[child.id] + ) + second = await definitions.create( + "Second", rule.id, NOW, assignee_child_ids=[child.id] + ) + + result = await delete_quest_definition(database, first.id) + + assert result.deleted is True + assert await definitions.get(first.id) is None + assert await definitions.get(second.id) is not None + assert await rules.get(rule.id) is not None + return None + + _with_db(tmp_path, "delete-shared-rule.db")(_body) + + +def test_delete_with_history_retires_and_preserves_everything( + tmp_path, +) -> None: + """A completion event on ANY instance: nothing is deleted; the + definition is deactivated and its row, rule, assignees, windows, + instance and completion history all remain.""" + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + definition_id = created.definition.id + rule_id = created.definition.schedule_rule_id + instances = QuestInstancesDao(database) + events = CompletionEventsDao(database) + instance = await instances.upsert( + definition_id, child.id, _today_iso(), NOW, window="morning" + ) + event = await events.append( + instance.id, + child.id, + "completed", + "user", + NOW, + True, + actor_user_id="admin-1", + ) + + result = await delete_quest_definition(database, definition_id) + + assert result.deleted is False + assert result.retired is True + assert result.definition_id == definition_id + assert isinstance(result.definition, CreatedQuestDefinition) + assert result.definition.definition.is_active is False + stored = await definitions.get(definition_id) + assert stored is not None + assert stored.is_active is False + assert await rules.get(rule_id) is not None + assert [c.id for c in await definitions.list_assignees( + definition_id + )] == [child.id] + assert [w.window for w in await definitions.list_windows( + definition_id + )] == ["morning"] + assert await instances.get_by_id(instance.id) == instance + assert await events.list_by_instance(instance.id) == [event] + return None + + _with_db(tmp_path, "delete-with-history.db")(_body) + + +def test_delete_with_uncompleted_history_still_retires(tmp_path) -> None: + """ANY completion event counts as history — even when the latest + event is an un-completion, the definition is retired, not deleted.""" + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + definition_id = created.definition.id + instances = QuestInstancesDao(database) + events = CompletionEventsDao(database) + instance = await instances.upsert( + definition_id, child.id, _today_iso(), NOW, window="morning" + ) + await events.append( + instance.id, child.id, "completed", "user", NOW, True, + actor_user_id="admin-1", + ) + await events.append( + instance.id, child.id, "uncompleted", "user", NOW, None, + actor_user_id="admin-1", + ) + + result = await delete_quest_definition(database, definition_id) + + assert result.retired is True + assert await instances.get_by_id(instance.id) == instance + return None + + _with_db(tmp_path, "delete-uncompleted-history.db")(_body) + + +def test_delete_nonexistent_definition_names_field(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match=r"^definition_id:"): + await delete_quest_definition(database, 999) + return None + + _with_db(tmp_path, "delete-missing.db")(_body) + + +@pytest.mark.parametrize("bad", [True, 1.0, "1", None]) +def test_delete_rejects_non_int_definition_id(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + with pytest.raises(ValueError, match="definition_id"): + await delete_quest_definition(database, bad) + assert await definitions.get(created.definition.id) is not None + return None + + _with_db(tmp_path, "delete-bad-id.db")(_body) -- 2.55.0 From ba0b2032cc8271be2f45b0a4582ed4a229dd0878 Mon Sep 17 00:00:00 2001 From: Joshua Date: Sat, 26 Sep 2026 12:40:44 -0400 Subject: [PATCH 2/3] Correct retirement docs: deactivation clears future open instances Retirement preserves the definition row, schedule rule, assignees, windows and all durable completion history, but like any deactivation it regenerates, so future uncompleted instances are cleared and none are recreated while inactive. Fixes the "deletes nothing / all instances preserved" wording in the core docstrings, the admin DELETE route description, ADMIN-SPEC and the retire test's docstring (review finding NQ-262-1). No behavioural change. Co-Authored-By: Claude Opus 5.5 --- api/routes_admin.py | 10 ++++++-- .../nestquest/core/quest_definitions.py | 24 +++++++++++++------ design/ADMIN-SPEC.md | 11 ++++++--- tests/test_quest_definitions.py | 7 +++--- 4 files changed, 37 insertions(+), 15 deletions(-) diff --git a/api/routes_admin.py b/api/routes_admin.py index 0dc64c2..6a00d48 100644 --- a/api/routes_admin.py +++ b/api/routes_admin.py @@ -208,8 +208,14 @@ retire/deactivate if there is"): :func:`nestquest_core.quest_definitions.delete_quest_definition`. A definition with NO completion event on any instance is hard-deleted (instances, assignees, windows, the row and its unreferenced rule, in - one core transaction); one WITH history is retired by deactivation - and everything is preserved. The response reports which happened + one core transaction); one WITH history is retired by deactivation. + Retirement preserves the definition row, schedule rule, assignees, + windows and ALL durable completion history; like any deactivation it + regenerates, so future UNCOMPLETED instances are cleared (rolling, + regenerable rows) and none are recreated while the definition is + inactive — the retired task stops generating and shows no open work. + Durable instance history lives only in the append-only + ``completion_events``. The response reports which happened (``deleted`` / ``retired``) and carries the retired definition, or ``null`` when deleted. An unknown id is 404. diff --git a/custom_components/nestquest/core/quest_definitions.py b/custom_components/nestquest/core/quest_definitions.py index 1e07692..b4764a5 100644 --- a/custom_components/nestquest/core/quest_definitions.py +++ b/custom_components/nestquest/core/quest_definitions.py @@ -525,10 +525,15 @@ async def set_quest_definition_active( 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 - re-enables future generation; generating or removing instances is - Feature 07 and does not happen here. + row, schedule rule, assignees and windows all survive, and its + durable completion history (the append-only ``completion_events``) + is never touched. Like every Feature 06 config change, the flip is + followed by :func:`~.materialize.regenerate_for_definition`: the + definition's future (today onward) UNCOMPLETED instances are cleared + — they are rolling, regenerable rows — and, while the definition is + inactive, none are re-materialized. Completed and past instances + are kept. Reactivation re-enables generation, and the same + regeneration re-materializes the rolling horizon. ``definition_id`` must be a plain int (bools and floats rejected, since SQLite would bind ``True`` onto definition 1) and @@ -598,9 +603,14 @@ async def delete_quest_definition( :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. + the definition is retired through :func:`set_quest_definition_active` + (``False``). Retirement preserves the definition row, schedule + rule, assignees, windows and ALL durable completion history. Like + any deactivation it regenerates, so future UNCOMPLETED instances are + cleared (they are rolling, regenerable rows) and none are recreated + while the definition is inactive — the retired task stops generating + and no longer shows open work. Durable instance history lives only + in the append-only ``completion_events``. ``definition_id`` must be a plain int (bools and floats rejected). Raises ValueError naming the field when the id is malformed or the diff --git a/design/ADMIN-SPEC.md b/design/ADMIN-SPEC.md index 77cab54..b337c07 100644 --- a/design/ADMIN-SPEC.md +++ b/design/ADMIN-SPEC.md @@ -91,9 +91,14 @@ otherwise it is retired (deactivated) and its history is kept." — with `Go bac danger-filled `Delete task`. Confirming calls `DELETE /api/v1/admin/quest-definitions/{id}` (admin plane only; the kids' panel has no equivalent control). Hybrid outcome (D-017): no completion event on any instance → the definition, its instances, assignees, windows and -unreferenced rule are hard-deleted and the row leaves the list; otherwise nothing is -deleted, the definition is deactivated and the row stays, shown inactive. An error keeps -the sheet open with the API's detail. +unreferenced rule are hard-deleted and the row leaves the list; otherwise the definition +is retired (deactivated) and the row stays, shown inactive. Retirement preserves the +definition row, schedule rule, assignees, windows and all durable completion history. Like +any deactivation it regenerates, so future uncompleted instances are cleared (they are +rolling, regenerable rows) and none are recreated while the definition is inactive — the +retired task stops generating and no longer shows open work. Durable instance history lives +only in the append-only `completion_events`. An error keeps the sheet open with the API's +detail. ### 3.3 Icon picker Each definition carries one optional icon, shown on both surfaces. The picker offers three diff --git a/tests/test_quest_definitions.py b/tests/test_quest_definitions.py index ee7f54c..65bcc69 100644 --- a/tests/test_quest_definitions.py +++ b/tests/test_quest_definitions.py @@ -2212,9 +2212,10 @@ def test_delete_without_history_keeps_a_shared_rule(tmp_path) -> None: def test_delete_with_history_retires_and_preserves_everything( tmp_path, ) -> None: - """A completion event on ANY instance: nothing is deleted; the - definition is deactivated and its row, rule, assignees, windows, - instance and completion history all remain.""" + """A completion event on ANY instance: the definition is retired + (deactivated) and its row, rule, assignees, windows and completion + history all remain. The completed instance survives; only future + UNCOMPLETED instances are cleared by the deactivation's regeneration.""" async def _body(database, children, rules, definitions, child): created = await create_quest_definition( database, "Brush teeth", _daily_rule(), [child.id], ["morning"] -- 2.55.0 From 6cb62b02605ff520ff937f70cfa310d6a27bd8a1 Mon Sep 17 00:00:00 2001 From: Joshua Date: Sat, 26 Sep 2026 12:44:45 -0400 Subject: [PATCH 3/3] Correct deleteDefinition doc: retirement clears future open instances Replace the over-broad "everything is preserved" with the precise hybrid contract (NQ-262-2). Comment-only change. Co-Authored-By: Claude Opus 5.5 --- admin/src/api/definitions.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/admin/src/api/definitions.ts b/admin/src/api/definitions.ts index 7c6405f..838a43d 100644 --- a/admin/src/api/definitions.ts +++ b/admin/src/api/definitions.ts @@ -158,7 +158,11 @@ export interface DefinitionDeleteResult { /** * Delete a task if it has no completion history; otherwise the API retires - * (deactivates) it and everything is preserved. + * (deactivates) it. Retirement preserves the definition row, schedule rule, + * assignees, windows and ALL durable completion history. Like any + * deactivation it regenerates, so future UNCOMPLETED instances are cleared + * (rolling, regenerable rows) and not recreated while inactive; durable + * history lives only in the append-only `completion_events`. */ export async function deleteDefinition(id: number): Promise { return readJson( -- 2.55.0