From ed230961e0314f181b3eafcb3ee148ae18d05133 Mon Sep 17 00:00:00 2001 From: Joshua Date: Sat, 26 Sep 2026 12:31:10 -0400 Subject: [PATCH] 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)