From 4606d7ddd0efa24a857c07788706b5edce92257b Mon Sep 17 00:00:00 2001 From: Joshua Date: Thu, 17 Sep 2026 09:21:21 -0400 Subject: [PATCH] Assign and unassign children on a quest definition (Feature 06, task 4) (#69) --- custom_components/nestquest/dao_rules.py | 9 +- .../nestquest/quest_definitions.py | 74 ++++- tests/test_quest_definitions.py | 270 ++++++++++++++++++ 3 files changed, 350 insertions(+), 3 deletions(-) diff --git a/custom_components/nestquest/dao_rules.py b/custom_components/nestquest/dao_rules.py index c15f585..d1338e4 100644 --- a/custom_components/nestquest/dao_rules.py +++ b/custom_components/nestquest/dao_rules.py @@ -962,14 +962,18 @@ class QuestDefinitionsDao: async def add_assignee( self, definition_id: int, child_id: int - ) -> None: + ) -> list[ChildRecord]: """Assign ``child_id`` to the definition (idempotent). Validates the assignee inside the same serialized transaction as the INSERT (existing and active AT ASSIGNMENT TIME), so a child deactivated concurrently cannot become an assignee. Assigning an already-assigned child is a no-op, mirroring the - composite primary key's storage-level guarantee. Changes + composite primary key's storage-level guarantee. Returns the + definition's assignees read back INSIDE the same transaction + that performed the write, so the returned roster is a + consistent post-assignment snapshot — a concurrent assign or + unassign cannot race it after the write commits. Changes future instances only: already-generated instances and completion history are never rewritten here. """ @@ -990,6 +994,7 @@ class QuestDefinitionsDao: "ON CONFLICT (definition_id, child_id) DO NOTHING", (definition_id, child_id), ) + return await self.list_assignees(definition_id) async def remove_assignee( self, definition_id: int, child_id: int diff --git a/custom_components/nestquest/quest_definitions.py b/custom_components/nestquest/quest_definitions.py index b574f08..b438a03 100644 --- a/custom_components/nestquest/quest_definitions.py +++ b/custom_components/nestquest/quest_definitions.py @@ -4,7 +4,8 @@ Sits between the Feature 09 service gate and the typed :class:`~.dao_rules.QuestDefinitionsDao`: callers get validation and the all-or-nothing create/edit here, and never touch the DAO SQL directly. :func:`create_quest_definition` is the create path and -:func:`edit_quest_definition` the edit path; there is no permission +:func:`edit_quest_definition` the edit path; :func:`assign_child` and +:func:`unassign_child` are the assignment paths. There is no permission check here (that is Feature 09). Validation policy mirrors :mod:`.children`: @@ -185,6 +186,18 @@ def _validate_definition_id(value: object) -> int: return value +def _validate_child_id(value: object) -> int: + """Reject non-int child ids (bools included) before any lookup. + + SQLite binds Python bools as integers, so ``True`` would silently + address child 1 and a float would round — malformed service input + must never mutate the wrong profile. + """ + if isinstance(value, bool) or not isinstance(value, int): + raise ValueError(f"child_id must be an integer, got {value!r}") + return value + + async def create_quest_definition( database: NestQuestDatabase, title: str, @@ -321,3 +334,62 @@ async def edit_quest_definition( assignees=snapshot.assignees, windows=snapshot.windows, ) + + +async def assign_child( + database: NestQuestDatabase, + definition_id: int, + child_id: int, +) -> list[ChildRecord]: + """Assign ``child_id`` to the definition (idempotent). + + Validates that both ids are plain ints (bools and floats rejected) + and that the child exists and is active — the DAO enforces the + active/exists check inside its transaction, and also confirms the + definition exists. Assigning an already-assigned child is a no-op + (no duplicate row). Returns the definition's assignees read back + inside the DAO's same transaction, a consistent post-assignment + roster (a concurrent assign/unassign cannot race it after the + write). Raises ValueError on any rejected argument. Only the + ``quest_definition_assignees`` link is written; existing instances + and completion history are never touched. + """ + _validate_definition_id(definition_id) + _validate_child_id(child_id) + dao = QuestDefinitionsDao(database) + try: + return await dao.add_assignee(definition_id, child_id) + except ValueError as error: + # The DAO names only the child/definition; re-raise so the + # public contract names the field the caller actually passed. + message = str(error) + if message.startswith("child "): + raise ValueError(f"child_id: {message}") from error + if message.startswith("quest definition "): + raise ValueError(f"definition_id: {message}") from error + raise + + +async def unassign_child( + database: NestQuestDatabase, + definition_id: int, + child_id: int, +) -> bool: + """Unassign ``child_id`` from the definition; True when removed. + + Validates both ids are plain ints (bools and floats rejected) and + that the definition exists, then removes the (definition, child) + link. Returns True when a row was removed and False when the child + was not currently assigned. Raises ValueError on a rejected + argument. Only the ``quest_definition_assignees`` link is written; + existing instances and completion history are never touched. + """ + _validate_definition_id(definition_id) + _validate_child_id(child_id) + dao = QuestDefinitionsDao(database) + definition = await dao.get(definition_id) + if definition is None: + raise ValueError( + f"definition_id: quest definition {definition_id} does not exist" + ) + return await dao.remove_assignee(definition_id, child_id) diff --git a/tests/test_quest_definitions.py b/tests/test_quest_definitions.py index 2f91b82..2206daa 100644 --- a/tests/test_quest_definitions.py +++ b/tests/test_quest_definitions.py @@ -16,8 +16,10 @@ from custom_components.nestquest.db import NestQuestDatabase from custom_components.nestquest.migrations import apply_migrations from custom_components.nestquest.quest_definitions import ( CreatedQuestDefinition, + assign_child, create_quest_definition, edit_quest_definition, + unassign_child, ) from custom_components.nestquest.recurrence import RuleType, ScheduleRule @@ -880,3 +882,271 @@ def test_edit_post_write_failure_rolls_back_everything(tmp_path) -> None: return None _with_db(tmp_path, "edit-post-write-rollback.db")(_body) + + +# --------------------------------------------------------------------------- +# assign_child / unassign_child: the assignment business-layer paths +# --------------------------------------------------------------------------- + + +def test_assign_child_multi_assignee_success(tmp_path) -> None: + """Assigning a second child leaves both as assignees (D-008).""" + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + bo = await children.create("Bo", NOW) + assignees = await assign_child(database, created.definition.id, bo.id) + assert sorted(c.id for c in assignees) == sorted([child.id, bo.id]) + return assignees + + _with_db(tmp_path, "assign-multi.db")(_body) + + +def test_assign_child_duplicate_is_noop(tmp_path) -> None: + """Assigning an already-assigned child adds no duplicate row.""" + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + assignees = await assign_child( + database, created.definition.id, child.id + ) + assert [c.id for c in assignees] == [child.id] + again = await assign_child( + database, created.definition.id, child.id + ) + assert [c.id for c in again] == [child.id] + return assignees + + _with_db(tmp_path, "assign-duplicate.db")(_body) + + +def test_assign_child_unknown_child_names_field(tmp_path) -> 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="child_id.*does not exist" + ): + await assign_child(database, created.definition.id, 999) + return None + + _with_db(tmp_path, "assign-unknown-child.db")(_body) + + +def test_assign_child_inactive_child_names_field(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + bo = await children.create("Bo", NOW) + await children.set_active(bo.id, False) + with pytest.raises(ValueError, match="child_id.*inactive"): + await assign_child(database, created.definition.id, bo.id) + # The failed assignment leaves the roster unchanged. + assert [c.id for c in await definitions.list_assignees( + created.definition.id + )] == [child.id] + return None + + _with_db(tmp_path, "assign-inactive-child.db")(_body) + + +def test_assign_child_nonexistent_definition_names_field(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises( + ValueError, match="definition_id.*does not exist" + ): + await assign_child(database, 999, child.id) + return None + + _with_db(tmp_path, "assign-missing-definition.db")(_body) + + +@pytest.mark.parametrize("bad", [True, 1.5, "1", None]) +def test_assign_child_rejects_non_int_definition_id(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises( + ValueError, match="definition_id must be an integer" + ): + await assign_child(database, bad, child.id) + return None + + _with_db(tmp_path, "assign-bad-definition-id.db")(_body) + + +@pytest.mark.parametrize("bad", [True, 1.5, "1", None]) +def test_assign_child_rejects_non_int_child_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="child_id must be an integer"): + await assign_child(database, created.definition.id, bad) + return None + + _with_db(tmp_path, "assign-bad-child-id.db")(_body) + + +def test_unassign_child_success_returns_true(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + bo = await children.create("Bo", NOW) + created = await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id, bo.id], + ["morning"], + ) + assert await unassign_child( + database, created.definition.id, bo.id + ) is True + assert [c.id for c in await definitions.list_assignees( + created.definition.id + )] == [child.id] + return None + + _with_db(tmp_path, "unassign-success.db")(_body) + + +def test_unassign_child_not_assigned_returns_false(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + assert await unassign_child( + database, created.definition.id, 999 + ) is False + return None + + _with_db(tmp_path, "unassign-not-assigned.db")(_body) + + +def test_unassign_child_nonexistent_definition_names_field(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises( + ValueError, match="definition_id.*does not exist" + ): + await unassign_child(database, 999, child.id) + return None + + _with_db(tmp_path, "unassign-missing-definition.db")(_body) + + +@pytest.mark.parametrize("bad", [True, 1.5, "1", None]) +def test_unassign_child_rejects_non_int_definition_id(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises( + ValueError, match="definition_id must be an integer" + ): + await unassign_child(database, bad, child.id) + return None + + _with_db(tmp_path, "unassign-bad-definition-id.db")(_body) + + +@pytest.mark.parametrize("bad", [True, 1.5, "1", None]) +def test_unassign_child_rejects_non_int_child_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="child_id must be an integer"): + await unassign_child(database, created.definition.id, bad) + return None + + _with_db(tmp_path, "unassign-bad-child-id.db")(_body) + + +def test_assign_and_unassign_leave_instances_and_history_alone( + tmp_path, +) -> None: + """Assign/unassign write the assignees table only. + + An already-generated instance keeps its generation-time child_id, and + no completion event appears — the assignment paths touch neither + ``quest_instances`` nor ``completion_events``. + """ + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + bo = await children.create("Bo", NOW) + await database.execute( + "INSERT INTO quest_instances (definition_id, child_id, " + "window, due_date, generated_at) VALUES (?, ?, 'morning', ?, ?)", + (created.definition.id, child.id, "2026-09-15", NOW), + ) + await assign_child(database, created.definition.id, bo.id) + await unassign_child(database, created.definition.id, child.id) + row = await database.fetch_one( + "SELECT child_id FROM quest_instances WHERE definition_id = ?", + (created.definition.id,), + ) + assert row == (child.id,) + events = await database.fetch_one( + "SELECT COUNT(*) FROM completion_events" + ) + assert events == (0,) + return None + + _with_db(tmp_path, "assign-unassign-history.db")(_body) + + +def test_concurrent_assign_unassign_returns_consistent_roster( + tmp_path, +) -> None: + """A concurrent unassign cannot strip the just-assigned child from + the roster that ``assign_child`` returns. + + ``assign_child`` returns the roster read inside the same locked + transaction as the INSERT, so the child it assigned is always present + in its result — even when an unassign of that same child races. The + pause hook holds ``list_assignees`` mid-flight so the unassign is + deterministically queued against the assignment's lock. + """ + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + bo = await children.create("Bo", NOW) + + import custom_components.nestquest.dao_rules as dao_rules + + original_list = dao_rules.QuestDefinitionsDao.list_assignees + started = asyncio.Event() + release = asyncio.Event() + + async def _pausing_list(self, definition_id): + if not started.is_set(): + started.set() + await release.wait() + return await original_list(self, definition_id) + + dao_rules.QuestDefinitionsDao.list_assignees = _pausing_list + try: + assign_task = asyncio.ensure_future( + assign_child(database, created.definition.id, bo.id) + ) + await started.wait() + unassign_task = asyncio.ensure_future( + unassign_child(database, created.definition.id, bo.id) + ) + await asyncio.sleep(0) + release.set() + assignees, removed = await asyncio.gather( + assign_task, unassign_task + ) + finally: + dao_rules.QuestDefinitionsDao.list_assignees = original_list + + assert bo.id in [c.id for c in assignees], ( + "assign_child must return a roster containing the child it " + "just assigned, even under a concurrent unassign" + ) + assert removed is True + return assignees + + _with_db(tmp_path, "assign-unassign-concurrent.db")(_body)