diff --git a/custom_components/nestquest/dao_rules.py b/custom_components/nestquest/dao_rules.py index 672c064..2a7340c 100644 --- a/custom_components/nestquest/dao_rules.py +++ b/custom_components/nestquest/dao_rules.py @@ -329,6 +329,28 @@ def schedule_rule_from_storage(storage: ScheduleRuleStorage) -> ScheduleRule: ) +def schedule_rule_storage_from_record( + record: ScheduleRuleRecord, +) -> ScheduleRuleStorage: + """Copy a rule row's value columns into a storage mapping. + + ``ScheduleRuleRecord`` is the read shape (it carries ``id``); + :func:`schedule_rule_from_storage` consumes the value-only + :class:`ScheduleRuleStorage` shape, so callers re-reading a stored + rule convert through this helper. + """ + return ScheduleRuleStorage( + rule_type=record.rule_type, + interval=record.interval, + weekday_set=record.weekday_set, + day_of_month=record.day_of_month, + nth_weekday=record.nth_weekday, + month=record.month, + start_date=record.start_date, + end_date=record.end_date, + ) + + def _reject_forbidden_storage_fields( storage: ScheduleRuleStorage, shape: RuleType ) -> None: @@ -726,6 +748,105 @@ class QuestDefinitionsDao: windows=window_records, ) + async def edit_definition( + self, + definition_id: int, + *, + title: str | None | object = _UNSET, + description: str | None | object = _UNSET, + icon: str | None | object = _UNSET, + rule: ScheduleRuleStorage | object = _UNSET, + windows: list[tuple[str, str | None]] | object = _UNSET, + ) -> QuestDefinitionSnapshot: + """Edit a definition's metadata, rule and windows atomically. + + Mirrors :meth:`create_with_rule_and_windows`: everything runs + inside ONE transaction under the connection lock, and the + returned snapshot's assignees and windows are read back inside + that transaction. Arguments default to the module sentinel + ``_UNSET`` meaning "leave this field alone"; ``None`` writes SQL + NULL so optional metadata (description, icon) can be cleared. + + - ``title``/``description``/``icon`` update the definition row + in place; the definition-level ``due_time`` column is never + written (per-window due times supersede it, D-008). + - ``rule`` (a pre-validated :class:`ScheduleRuleStorage`) + overwrites the referenced ``schedule_rules`` row in place: the + definition keeps its ``schedule_rule_id``, so no new or + orphaned rule row appears. + - ``windows`` REPLACES the whole window set: the current rows + are deleted and the given set re-inserted, so missing windows + are added, changed due times updated and removed windows + dropped. + + Assignment is deliberately not settable here (multi-assignee is + managed by :meth:`add_assignee`/:meth:`remove_assignee`), and + activation is untouched. Raises ValueError when the definition + does not exist. Edits change future instances only: no + ``quest_instances`` or ``completion_events`` row is touched. + """ + async with _connection_lock(self._database): + async with self._database.transaction(): + definition = await self.get(definition_id) + if definition is None: + raise ValueError( + f"quest definition {definition_id} does not exist" + ) + assignments: list[str] = [] + parameters: list[object] = [] + for column, value in ( + ("title", title), + ("description", description), + ("icon", icon), + ): + if value is not _UNSET: + assignments.append(f"{column} = ?") + parameters.append(value) + if assignments: + parameters.append(definition_id) + await self._database.execute( + f"UPDATE quest_definitions SET " + f"{', '.join(assignments)} WHERE id = ?", + tuple(parameters), + ) + if rule is not _UNSET: + await ScheduleRulesDao(self._database).update( + definition.schedule_rule_id, + rule_type=rule.rule_type, + interval=rule.interval, + weekday_set=rule.weekday_set, + day_of_month=rule.day_of_month, + nth_weekday=rule.nth_weekday, + month=rule.month, + start_date=rule.start_date, + end_date=rule.end_date, + ) + if windows is not _UNSET: + await self._database.execute( + "DELETE FROM quest_definition_windows " + "WHERE definition_id = ?", + (definition_id,), + ) + for window, due_time in windows: + _validate_window(window) + if due_time is not None: + _validate_time(due_time, "due_time") + await self._database.execute( + "INSERT INTO quest_definition_windows " + "(definition_id, window, due_time) " + "VALUES (?, ?, ?)", + (definition_id, window, due_time), + ) + updated = await self.get(definition_id) + assignees = await self.list_assignees(definition_id) + window_records = await self.list_windows(definition_id) + assert updated is not None + return QuestDefinitionSnapshot( + definition=updated, + assignees=assignees, + windows=window_records, + ) + async def _validate_rule_exists(self, schedule_rule_id: int) -> None: """Raise ValueError unless the rule exists. diff --git a/custom_components/nestquest/quest_definitions.py b/custom_components/nestquest/quest_definitions.py index 28283e1..d326b2f 100644 --- a/custom_components/nestquest/quest_definitions.py +++ b/custom_components/nestquest/quest_definitions.py @@ -1,10 +1,11 @@ -"""Business layer for creating quest definitions (Feature 06). +"""Business layer for creating and editing quest definitions (Feature 06). Sits between the Feature 09 service gate and the typed :class:`~.dao_rules.QuestDefinitionsDao`: callers get validation and the -all-or-nothing create here, and never touch the DAO SQL directly. -:func:`create_quest_definition` is the create path; there is no -permission check here (that is Feature 09). +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 +check here (that is Feature 09). Validation policy mirrors :mod:`.children`: @@ -43,9 +44,13 @@ from .dao_rules import ( QuestDefinitionRecord, QuestDefinitionWindowRecord, QuestDefinitionsDao, + ScheduleRuleStorage, + ScheduleRulesDao, + _UNSET, _validate_time, _validate_window, schedule_rule_from_storage, + schedule_rule_storage_from_record, schedule_rule_to_storage, ) from .db import NestQuestDatabase @@ -54,7 +59,7 @@ from .recurrence import ScheduleRule @dataclass(frozen=True) class CreatedQuestDefinition: - """The persisted result of :func:`create_quest_definition`. + """The persisted result of a create or edit. ``definition`` is the stored row; ``rule`` is the schedule rule decoded back through the storage mapping; ``assignees`` and @@ -169,6 +174,18 @@ def _normalize_windows( return normalized +def _validate_definition_id(value: object) -> int: + """Reject non-int definition ids (bools included) before any lookup. + + SQLite binds Python bools as integers, so ``True`` would silently + address definition 1 and a float would round — malformed service + input must never mutate the wrong definition. + """ + if isinstance(value, bool) or not isinstance(value, int): + raise ValueError(f"definition_id must be an integer, got {value!r}") + return value + + async def create_quest_definition( database: NestQuestDatabase, title: str, @@ -226,3 +243,89 @@ async def create_quest_definition( assignees=snapshot.assignees, windows=snapshot.windows, ) + + +async def edit_quest_definition( + database: NestQuestDatabase, + definition_id: int, + *, + title: str | object = _UNSET, + description: str | None | object = _UNSET, + icon: str | None | object = _UNSET, + rule: ScheduleRule | object = _UNSET, + windows: list[str | tuple[str, str | None]] | object = _UNSET, +) -> CreatedQuestDefinition: + """Edit a quest definition's metadata, rule and windows atomically. + + Arguments default to the sentinel ``_UNSET`` meaning "leave this + field alone". A provided ``title`` is validated like create's + (required and trimmed); ``description`` and ``icon`` are optional + and may be cleared by passing ``None`` explicitly. A provided + ``rule`` must be an already-validated :class:`~.recurrence.ScheduleRule` + and REPLACES the definition's schedule rule IN PLACE — the existing + ``schedule_rules`` row is updated, never orphaned or duplicated. A + provided ``windows`` list REPLACES the whole window set (each entry + a ``const.QUEST_WINDOWS`` name, optionally with a strict HH:MM due + time), matching create's non-empty contract. Assignment is NOT + settable here. Raises ValueError when the definition does not + exist or any argument is rejected. + + Returns the updated definition together with its decoded rule, + assignees and windows — all read back inside the DAO's transaction, + so the returned bundle is a consistent edit snapshot. Edits change + future instances only; no ``quest_instances`` or + ``completion_events`` row is touched. + """ + _validate_definition_id(definition_id) + + title_value: str | None | object = _UNSET + if title is not _UNSET: + name = _validate_text(title, "title", required=True) + assert name is not None + title_value = name + + description_value: str | None | object = _UNSET + if description is not _UNSET: + description_value = _validate_text(description, "description") + + icon_value: str | None | object = _UNSET + if icon is not _UNSET: + icon_value = _validate_text(icon, "icon") + + rule_storage: ScheduleRuleStorage | object = _UNSET + if rule is not _UNSET: + if not isinstance(rule, ScheduleRule): + raise ValueError(f"rule must be a ScheduleRule, got {rule!r}") + rule_storage = schedule_rule_to_storage(rule) + + window_specs: list[tuple[str, str | None]] | object = _UNSET + if windows is not _UNSET: + window_specs = _normalize_windows(windows) + + dao = QuestDefinitionsDao(database) + snapshot = await dao.edit_definition( + definition_id, + title=title_value, + description=description_value, + icon=icon_value, + rule=rule_storage, + windows=window_specs, + ) + + if rule_storage is not _UNSET: + decoded_rule = schedule_rule_from_storage(rule_storage) + else: + rule_record = await ScheduleRulesDao(database).get( + snapshot.definition.schedule_rule_id + ) + assert rule_record is not None + decoded_rule = schedule_rule_from_storage( + schedule_rule_storage_from_record(rule_record) + ) + + return CreatedQuestDefinition( + definition=snapshot.definition, + rule=decoded_rule, + assignees=snapshot.assignees, + windows=snapshot.windows, + ) diff --git a/tests/test_quest_definitions.py b/tests/test_quest_definitions.py index 1f99808..97a2e48 100644 --- a/tests/test_quest_definitions.py +++ b/tests/test_quest_definitions.py @@ -17,6 +17,7 @@ from custom_components.nestquest.migrations import apply_migrations from custom_components.nestquest.quest_definitions import ( CreatedQuestDefinition, create_quest_definition, + edit_quest_definition, ) from custom_components.nestquest.recurrence import RuleType, ScheduleRule @@ -461,3 +462,317 @@ def test_post_write_failure_rolls_back_everything(tmp_path) -> None: return None _with_db(tmp_path, "post-write-rollback.db")(_body) + + +# --------------------------------------------------------------------------- +# edit_quest_definition: successful edits +# --------------------------------------------------------------------------- + + +def test_edit_title_description_icon(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id], + ["morning"], + description="Old description", + icon="mdi:old", + ) + result = await edit_quest_definition( + database, + created.definition.id, + title=" Floss ", + description=" New description ", + icon=" mdi:new ", + ) + assert result.definition.title == "Floss" + assert result.definition.description == "New description" + assert result.definition.icon == "mdi:new" + # Untouched fields survive the edit unchanged. + assert result.definition.id == created.definition.id + assert result.definition.schedule_rule_id == created.definition.schedule_rule_id + assert result.definition.due_time is None + assert result.definition.is_active is True + assert result.definition.created_at == created.definition.created_at + # Rule, assignees and windows are read back through the DAO. + assert result.rule == _daily_rule() + assert [c.id for c in result.assignees] == [child.id] + assert [w.window for w in result.windows] == ["morning"] + stored = await definitions.get(created.definition.id) + assert stored is not None + assert stored.title == "Floss" + assert stored.description == "New description" + assert stored.icon == "mdi:new" + return None + + _with_db(tmp_path, "edit-metadata.db")(_body) + + +def test_edit_omitted_fields_left_unchanged(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id], + ["morning"], + description="Keep me", + ) + result = await edit_quest_definition( + database, created.definition.id, title="Renamed" + ) + assert result.definition.title == "Renamed" + assert result.definition.description == "Keep me" + assert result.definition.icon is None + return None + + _with_db(tmp_path, "edit-omitted.db")(_body) + + +def test_edit_clears_optional_metadata(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id], + ["morning"], + description="Old", + icon="mdi:old", + ) + result = await edit_quest_definition( + database, + created.definition.id, + description=None, + icon=None, + ) + assert result.definition.description is None + assert result.definition.icon is None + return None + + _with_db(tmp_path, "edit-clear-metadata.db")(_body) + + +def test_edit_rule_updates_existing_row_in_place(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + rule_id = created.definition.schedule_rule_id + assert rule_id == 1 + new_rule = ScheduleRule( + rule_type=RuleType.WEEKLY, + weekday_set={1, 3}, + start_date="2026-09-14", + ) + result = await edit_quest_definition( + database, created.definition.id, rule=new_rule + ) + assert result.rule == new_rule + # The definition keeps the SAME rule row: no new or orphaned row. + assert result.definition.schedule_rule_id == rule_id + rule_row = await rules.get(rule_id) + assert rule_row is not None + assert rule_row.rule_type == "weekly" + assert rule_row.weekday_set == "1,3" + assert await rules.get(rule_id + 1) is None + return None + + _with_db(tmp_path, "edit-rule-in-place.db")(_body) + + +def test_edit_rule_shape_change_clears_stale_columns(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + weekly = ScheduleRule( + rule_type=RuleType.WEEKLY, + weekday_set={2}, + start_date="2026-09-14", + ) + created = await create_quest_definition( + database, "Chore", weekly, [child.id], ["morning"] + ) + rule_id = created.definition.schedule_rule_id + result = await edit_quest_definition( + database, created.definition.id, rule=_daily_rule() + ) + assert result.rule == _daily_rule() + rule_row = await rules.get(rule_id) + assert rule_row is not None + # The old weekly shape's weekday_set must be cleared, not left stale. + assert rule_row.rule_type == "daily" + assert rule_row.weekday_set is None + return None + + _with_db(tmp_path, "edit-rule-shape.db")(_body) + + +def test_edit_replaces_windows(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id], + [("morning", "07:00"), ("evening", "19:00")], + ) + result = await edit_quest_definition( + database, + created.definition.id, + windows=[("morning", "08:00"), ("afternoon", "12:00")], + ) + by_name = {w.window: w.due_time for w in result.windows} + # morning's due time changed, afternoon added, evening removed. + assert by_name == {"morning": "08:00", "afternoon": "12:00"} + persisted = { + w.window: w.due_time + for w in await definitions.list_windows(created.definition.id) + } + assert persisted == {"morning": "08:00", "afternoon": "12:00"} + return None + + _with_db(tmp_path, "edit-replace-windows.db")(_body) + + +def test_edit_preserves_assignees(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"] + ) + result = await edit_quest_definition( + database, + created.definition.id, + title="Renamed", + rule=ScheduleRule( + rule_type=RuleType.WEEKLY, + weekday_set={0}, + start_date="2026-09-14", + ), + windows=[("evening", "20:00")], + ) + assert sorted(c.id for c in result.assignees) == sorted([child.id, bo.id]) + persisted = [ + c.id for c in await definitions.list_assignees(created.definition.id) + ] + assert sorted(persisted) == sorted([child.id, bo.id]) + return None + + _with_db(tmp_path, "edit-preserves-assignees.db")(_body) + + +# --------------------------------------------------------------------------- +# edit_quest_definition: argument rejections +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("bad", ["", " ", None]) +def test_edit_rejects_empty_title(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="title is required"): + await edit_quest_definition( + database, created.definition.id, title=bad + ) + return None + + _with_db(tmp_path, "edit-bad-title.db")(_body) + + +@pytest.mark.parametrize("bad", [None, "weekly", 42, {"rule_type": "daily"}]) +def test_edit_rejects_non_schedule_rule(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="rule must be a ScheduleRule"): + await edit_quest_definition( + database, created.definition.id, rule=bad + ) + return None + + _with_db(tmp_path, "edit-bad-rule.db")(_body) + + +@pytest.mark.parametrize("bad", [["noon"], [("MORNING", None)], [("", None)]]) +def test_edit_rejects_invalid_window(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="window must be one of"): + await edit_quest_definition( + database, created.definition.id, windows=bad + ) + return None + + _with_db(tmp_path, "edit-bad-window.db")(_body) + + +@pytest.mark.parametrize("bad", ["9:30", "25:00", "09:60", "0900"]) +def test_edit_rejects_malformed_due_time(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="HH:MM"): + await edit_quest_definition( + database, created.definition.id, windows=[("morning", bad)] + ) + return None + + _with_db(tmp_path, "edit-bad-due-time.db")(_body) + + +@pytest.mark.parametrize("bad", [[]]) +def test_edit_rejects_empty_windows(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="windows must not be empty"): + await edit_quest_definition( + database, created.definition.id, windows=bad + ) + return None + + _with_db(tmp_path, "edit-empty-windows.db")(_body) + + +@pytest.mark.parametrize("bad", [("morning",), "morning", {"morning"}]) +def test_edit_rejects_non_list_windows(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="windows must be a list"): + await edit_quest_definition( + database, created.definition.id, windows=bad + ) + return None + + _with_db(tmp_path, "edit-non-list-windows.db")(_body) + + +def test_edit_rejects_nonexistent_definition(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match="does not exist"): + await edit_quest_definition(database, 999, title="X") + return None + + _with_db(tmp_path, "edit-missing-definition.db")(_body) + + +@pytest.mark.parametrize("bad", [True, 1.5, "1", None]) +def test_edit_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 edit_quest_definition(database, bad, title="X") + return None + + _with_db(tmp_path, "edit-bad-definition-id.db")(_body)