diff --git a/api/routes_admin.py b/api/routes_admin.py index 7664e4d..6fc3009 100644 --- a/api/routes_admin.py +++ b/api/routes_admin.py @@ -251,7 +251,7 @@ import datetime from typing import Annotated, Literal, NoReturn from fastapi import APIRouter, Depends, HTTPException, Request, Response -from pydantic import BaseModel, field_validator, model_validator +from pydantic import BaseModel, StrictBool, field_validator, model_validator from api import transitions as api_transitions from api.auth import require_admin @@ -464,7 +464,7 @@ class AdminQuestDefinitionCreateRequest(BaseModel): windows: list[str | tuple[str, str | None]] description: str | None = None icon: str | None = None - skip_on_away: bool = True + skip_on_away: StrictBool = True class AdminQuestDefinitionEditRequest(BaseModel): @@ -488,7 +488,7 @@ class AdminQuestDefinitionEditRequest(BaseModel): icon: str | None = None rule: AdminRuleRequest | None = None windows: list[str | tuple[str, str | None]] | None = None - skip_on_away: bool | None = None + skip_on_away: StrictBool | None = None class AdminQuestDefinitionActiveRequest(BaseModel): diff --git a/custom_components/nestquest/core/dao_rules.py b/custom_components/nestquest/core/dao_rules.py index 5525c03..669569d 100644 --- a/custom_components/nestquest/core/dao_rules.py +++ b/custom_components/nestquest/core/dao_rules.py @@ -105,6 +105,19 @@ def _validate_window(window: str) -> None: ) +def _validate_skip_on_away(value: object) -> int: + """Return ``value`` as 0/1, raising ValueError for anything else. + + The fresh-schema CHECK constraint enforces this in SQLite, but a + migrated database gained the column through ``ADD COLUMN`` without + it, so every DAO write path validates here instead. Accepts a bool + or the ints 0/1; floats, strings and other ints are rejected. + """ + if not isinstance(value, int) or value not in (0, 1): + raise ValueError(f"skip_on_away must be 0 or 1, got {value!r}") + return int(value) + + @dataclass(frozen=True) class ScheduleRuleRecord: """One row of ``schedule_rules``.""" @@ -647,6 +660,7 @@ class QuestDefinitionsDao: empty list or None creates an unassigned definition, which callers may fill via :meth:`add_assignee`. """ + skip_on_away_value = _validate_skip_on_away(skip_on_away) async with _connection_lock(self._database): async with self._database.transaction(): await self._validate_rule_exists(schedule_rule_id) @@ -665,7 +679,7 @@ class QuestDefinitionsDao: due_time, int(is_active), created_at, - int(skip_on_away), + skip_on_away_value, ), ) definition_id = result.lastrowid @@ -719,6 +733,7 @@ class QuestDefinitionsDao: _validate_date(schedule_rule.start_date, "start_date") if schedule_rule.end_date is not None: _validate_date(schedule_rule.end_date, "end_date") + skip_on_away_value = _validate_skip_on_away(skip_on_away) async with _connection_lock(self._database): async with self._database.transaction(): for child_id in assignee_child_ids: @@ -737,7 +752,7 @@ class QuestDefinitionsDao: None, 1, created_at, - int(skip_on_away), + skip_on_away_value, ), ) definition_id = result.lastrowid @@ -810,6 +825,8 @@ class QuestDefinitionsDao: does not exist. Edits change future instances only: no ``quest_instances`` or ``completion_events`` row is touched. """ + if skip_on_away is not _UNSET: + skip_on_away = _validate_skip_on_away(skip_on_away) async with _connection_lock(self._database): async with self._database.transaction(): definition = await self.get(definition_id) @@ -829,7 +846,7 @@ class QuestDefinitionsDao: parameters.append(value) if skip_on_away is not _UNSET: assignments.append("skip_on_away = ?") - parameters.append(int(skip_on_away)) + parameters.append(skip_on_away) if assignments: parameters.append(definition_id) await self._database.execute( @@ -1030,13 +1047,16 @@ class QuestDefinitionsDao: description: str | None | object = _UNSET, icon: str | None | object = _UNSET, due_time: str | None | object = _UNSET, + skip_on_away: bool | None = None, ) -> int: """Update the given fields; returns rows updated (0 if absent). Arguments default to the module sentinel ``_UNSET`` meaning "leave this column alone"; passing ``None`` explicitly writes SQL NULL, so callers can remove optional metadata - (description, icon, due_time). Assignment and activation are + (description, icon, due_time). ``skip_on_away`` is NOT NULL, + so its ``None`` default leaves it unchanged; a supplied value + must be 0/1 (a bool). Assignment and activation are deliberately NOT settable here: they have their own explicit operations (:meth:`add_assignee`, :meth:`remove_assignee`, :meth:`set_active`) so they stay separately loggable and @@ -1053,6 +1073,9 @@ class QuestDefinitionsDao: if value is not _UNSET: assignments.append(f"{column} = ?") parameters.append(value) + if skip_on_away is not None: + assignments.append("skip_on_away = ?") + parameters.append(_validate_skip_on_away(skip_on_away)) if not assignments: return 0 parameters.append(definition_id) diff --git a/custom_components/nestquest/core/schema.py b/custom_components/nestquest/core/schema.py index 0ada459..880ae36 100644 --- a/custom_components/nestquest/core/schema.py +++ b/custom_components/nestquest/core/schema.py @@ -258,7 +258,7 @@ QUEST_DEFINITIONS_TABLE_SQL = """( due_time TEXT, is_active INTEGER NOT NULL DEFAULT 1 CHECK (typeof(is_active) = 'integer' AND is_active IN (0, 1)), created_at TEXT NOT NULL, - skip_on_away INTEGER NOT NULL DEFAULT 1 + skip_on_away INTEGER NOT NULL DEFAULT 1 CHECK (skip_on_away IN (0, 1)) )""" SCHEMA_V1_QUEST_DEFINITIONS_DDL: list[str] = [ diff --git a/tests/test_api_admin_questdef.py b/tests/test_api_admin_questdef.py index 174b773..9111fb2 100644 --- a/tests/test_api_admin_questdef.py +++ b/tests/test_api_admin_questdef.py @@ -523,6 +523,75 @@ async def test_edit_rejections_are_422_and_change_nothing( } +@pytest.mark.parametrize("coercible", [0, 1, "true", "false"]) +async def test_create_rejects_coercible_skip_on_away_and_persists_nothing( + admin_questdefs: SimpleNamespace, coercible: object +) -> None: + """A numeric or string skip_on_away is 422, not coerced to a bool.""" + client = admin_questdefs.client + dao = _dao_rules.QuestDefinitionsDao(admin_questdefs.database) + ada = (await _create_children(client, ("Ada",)))[0] + existing = await client.post( + "/api/v1/admin/quest-definitions", + json={ + "title": "Feed the cat", + "rule": _weekly_rule_body(), + "assignee_child_ids": [ada], + "windows": ["morning"], + "skip_on_away": False, + }, + headers=_admin_headers(), + ) + assert existing.status_code == 201 + existing_id = existing.json()["id"] + + response = await client.post( + "/api/v1/admin/quest-definitions", + json={ + "title": "Tidy the den", + "rule": _weekly_rule_body(), + "assignee_child_ids": [ada], + "windows": ["morning"], + "skip_on_away": coercible, + }, + headers=_admin_headers(), + ) + assert response.status_code == 422 + assert set(await _definitions_by_id(admin_questdefs.database)) == { + existing_id + } + assert (await dao.get(existing_id)).skip_on_away is False + + +@pytest.mark.parametrize("coercible", [0, 1, "true", "false"]) +async def test_edit_rejects_coercible_skip_on_away_and_changes_nothing( + admin_questdefs: SimpleNamespace, coercible: object +) -> None: + """A numeric or string skip_on_away edit is 422; the flag is kept.""" + client = admin_questdefs.client + dao = _dao_rules.QuestDefinitionsDao(admin_questdefs.database) + ada = (await _create_children(client, ("Ada",)))[0] + definition_id = (await _create_definition(client, [ada])).json()["id"] + # Start from each stored value so a coerced write would be visible. + for stored in (True, False): + set_flag = await client.patch( + f"/api/v1/admin/quest-definitions/{definition_id}", + json={"skip_on_away": stored}, + headers=_admin_headers(), + ) + assert set_flag.status_code == 200 + + response = await client.patch( + f"/api/v1/admin/quest-definitions/{definition_id}", + json={"skip_on_away": coercible, "title": "Renamed"}, + headers=_admin_headers(), + ) + assert response.status_code == 422 + stored_definition = await dao.get(definition_id) + assert stored_definition.skip_on_away is stored + assert stored_definition.title == "Tidy the den" + + # --- set_active ------------------------------------------------------------- diff --git a/tests/test_dao_rules.py b/tests/test_dao_rules.py index 7047d86..5986dbf 100644 --- a/tests/test_dao_rules.py +++ b/tests/test_dao_rules.py @@ -847,6 +847,7 @@ def test_definition_update_cannot_change_assignee_or_active( "description", "icon", "due_time", + "skip_on_away", } assert await definitions.update( definition.id, title="A2" @@ -862,6 +863,72 @@ def test_definition_update_cannot_change_assignee_or_active( _with_db(tmp_path, "definition-update-shape.db")(_body) +def test_definition_update_toggles_skip_on_away_and_omission_keeps_it( + tmp_path, +) -> None: + async def _body(database, rules, definitions, children, child): + rule = await rules.create("daily", "2026-09-14") + definition = await definitions.create("A", rule.id, NOW) + assert definition.skip_on_away is True + assert await definitions.update( + definition.id, skip_on_away=False + ) == 1 + assert (await definitions.get(definition.id)).skip_on_away is False + # Omitting the flag (None) leaves the stored False untouched. + assert await definitions.update(definition.id, title="A2") == 1 + kept = await definitions.get(definition.id) + assert kept.title == "A2" + assert kept.skip_on_away is False + assert await definitions.update(definition.id) == 0 + assert await definitions.update( + definition.id, skip_on_away=True + ) == 1 + assert (await definitions.get(definition.id)).skip_on_away is True + + _with_db(tmp_path, "definition-update-skip-on-away.db")(_body) + + +@pytest.mark.parametrize("bad", [2, -1, 0.0, 1.5, "1", "true", b"\x01"]) +def test_skip_on_away_rejected_unless_zero_or_one_at_every_write( + tmp_path, bad +) -> None: + """Every DAO entry point taking the flag rejects non-0/1 values. + + This is the only guard on migrated databases, whose column was + added without the fresh-schema CHECK constraint. + """ + async def _body(database, rules, definitions, children, child): + rule = await rules.create("daily", "2026-09-14") + definition = await definitions.create("A", rule.id, NOW) + with pytest.raises(ValueError, match="skip_on_away must be 0 or 1"): + await definitions.create("B", rule.id, NOW, skip_on_away=bad) + with pytest.raises(ValueError, match="skip_on_away must be 0 or 1"): + await definitions.create_with_rule_and_windows( + "C", + schedule_rule_to_storage( + ScheduleRule( + rule_type=RuleType.DAILY, start_date="2026-09-14" + ) + ), + NOW, + [child.id], + [("morning", None)], + skip_on_away=bad, + ) + with pytest.raises(ValueError, match="skip_on_away must be 0 or 1"): + await definitions.edit_definition( + definition.id, title="A2", skip_on_away=bad + ) + with pytest.raises(ValueError, match="skip_on_away must be 0 or 1"): + await definitions.update( + definition.id, title="A2", skip_on_away=bad + ) + stored = await definitions.list_all() + assert [(d.title, d.skip_on_away) for d in stored] == [("A", True)] + + _with_db(tmp_path, "definition-skip-on-away-invalid.db")(_body) + + def test_definition_update_no_fields_returns_zero(tmp_path) -> None: async def _body(database, rules, definitions, children, child): rule = await rules.create("daily", "2026-09-14") diff --git a/tests/test_schema.py b/tests/test_schema.py index cb26e62..86c6eaf 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -634,6 +634,30 @@ def test_schedule_rules_rule_type_check_enforced(tmp_path) -> None: _run(database.close()) +@pytest.mark.parametrize("bad_flag", [2, -1]) +def test_quest_definitions_skip_on_away_check_enforced( + tmp_path, bad_flag +) -> None: + database = _open_db(tmp_path / f"skip-on-away-{bad_flag}.db") + try: + _apply(database) + _child(database) + _insert_rule(database) + with pytest.raises(sqlite3.IntegrityError, match="CHECK"): + _insert_definition(database, skip_on_away=bad_flag) + definition_id = _insert_definition(database, skip_on_away=0) + with pytest.raises(sqlite3.IntegrityError, match="CHECK"): + _run( + database.execute( + "UPDATE quest_definitions SET skip_on_away = ? " + "WHERE id = ?", + (bad_flag, definition_id), + ) + ) + finally: + _run(database.close()) + + def test_schedule_rules_interval_zero_fails(tmp_path) -> None: database = _open_db(tmp_path / "interval-zero.db") try: