From ab0a320f3fa6fad6bc88fa55ccc4da01bb8f51c0 Mon Sep 17 00:00:00 2001 From: Joshua Date: Wed, 23 Sep 2026 20:31:05 -0400 Subject: [PATCH 1/2] Persist skip_on_away on quest definitions Add a skip_on_away flag to quest_definitions (migration 8, additive column defaulting to 1 so existing rows keep today's behaviour). The admin create/edit routes accept it as an optional boolean (create defaults true, an omitted edit field keeps the stored value) and the definition payload returns it. Materialisation generates instances on absent dates when the flag is false. Task: ded75ab8-0219-4d52-aefa-82c09e0b5e6b Co-Authored-By: Claude Opus 5.5 --- api/routes_admin.py | 13 +- custom_components/nestquest/core/dao_rules.py | 35 ++++- .../nestquest/core/materialize.py | 12 +- .../nestquest/core/migrations.py | 31 ++++- .../nestquest/core/quest_definitions.py | 24 +++- custom_components/nestquest/core/schema.py | 3 +- tests/test_api_admin_questdef.py | 54 ++++++++ tests/test_api_admin_questdef_list.py | 49 +++++++ tests/test_materialize.py | 127 ++++++++++++++++++ tests/test_migrations.py | 95 ++++++++++++- tests/test_schema.py | 1 + 11 files changed, 424 insertions(+), 20 deletions(-) diff --git a/api/routes_admin.py b/api/routes_admin.py index 1a94e60..7664e4d 100644 --- a/api/routes_admin.py +++ b/api/routes_admin.py @@ -363,6 +363,7 @@ class AdminQuestDefinitionResponse(BaseModel): description: str | None icon: str | None is_active: bool + skip_on_away: bool rule: AdminRuleResponse assignees: list[AdminAssigneeResponse] windows: list[AdminDefinitionWindowResponse] @@ -450,8 +451,9 @@ class AdminQuestDefinitionCreateRequest(BaseModel): ``title``, ``rule``, ``assignee_child_ids`` and ``windows`` are required (the core rejects an empty title, a non-list or empty assignee/window list, and unknown window names); ``description`` - and ``icon`` are optional and default to NULL through the core. - ``windows`` entries are a window name (``morning``) or a two-item + and ``icon`` are optional and default to NULL through the core, and + ``skip_on_away`` is optional and defaults to true (no instance on a + date the child is absent). ``windows`` entries are a window name (``morning``) or a two-item ``[window, due_time]`` pair whose due time is a strict 24-hour HH:MM (or ``null`` for none) — both forms the core normalizes. """ @@ -462,6 +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 class AdminQuestDefinitionEditRequest(BaseModel): @@ -476,7 +479,8 @@ class AdminQuestDefinitionEditRequest(BaseModel): ``rule`` or ``windows`` is passed and rejected by the core. A supplied ``windows`` list REPLACES the whole window set, and ``assignees`` are deliberately absent — assignment is not settable - through this route. + through this route. An omitted (or ``null``) ``skip_on_away`` + keeps the stored value. """ title: str | None = None @@ -484,6 +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 class AdminQuestDefinitionActiveRequest(BaseModel): @@ -963,6 +968,7 @@ def _quest_definition_response( description=bundle.definition.description, icon=bundle.definition.icon, is_active=bundle.definition.is_active, + skip_on_away=bundle.definition.skip_on_away, rule=_rule_response(bundle.rule), assignees=[ AdminAssigneeResponse(id=child.id, display_name=child.display_name) @@ -1234,6 +1240,7 @@ async def admin_create_quest_definition( body.windows, description=body.description, icon=body.icon, + skip_on_away=body.skip_on_away, ) except ValueError as error: _raise_quest_definition_error(error) diff --git a/custom_components/nestquest/core/dao_rules.py b/custom_components/nestquest/core/dao_rules.py index 6770eb2..5525c03 100644 --- a/custom_components/nestquest/core/dao_rules.py +++ b/custom_components/nestquest/core/dao_rules.py @@ -152,6 +152,7 @@ class QuestDefinitionRecord: due_time: str | None is_active: bool created_at: str + skip_on_away: bool @dataclass(frozen=True) @@ -187,7 +188,7 @@ _RULE_COLUMNS = ( ) _DEFINITION_COLUMNS = ( "id, title, description, icon, schedule_rule_id, due_time, " - "is_active, created_at" + "is_active, created_at, skip_on_away" ) _WINDOW_COLUMNS = "definition_id, window, due_time" @@ -216,6 +217,7 @@ def _definition_from_row(row: tuple) -> QuestDefinitionRecord: due_time=row[5], is_active=bool(row[6]), created_at=row[7], + skip_on_away=bool(row[8]), ) @@ -632,6 +634,7 @@ class QuestDefinitionsDao: due_time: str | None = None, is_active: bool = True, assignee_child_ids: list[int] | None = None, + skip_on_away: bool = True, ) -> QuestDefinitionRecord: """Insert one definition and return the record as stored. @@ -652,7 +655,8 @@ class QuestDefinitionsDao: result = await self._database.execute( "INSERT INTO quest_definitions (title, description, " "icon, schedule_rule_id, due_time, is_active, " - "created_at) VALUES (?, ?, ?, ?, ?, ?, ?)", + "created_at, skip_on_away) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?)", ( title, description, @@ -661,6 +665,7 @@ class QuestDefinitionsDao: due_time, int(is_active), created_at, + int(skip_on_away), ), ) definition_id = result.lastrowid @@ -684,6 +689,7 @@ class QuestDefinitionsDao: *, description: str | None = None, icon: str | None = None, + skip_on_away: bool = True, ) -> QuestDefinitionSnapshot: """Insert a rule, definition, assignees and windows atomically. @@ -721,8 +727,18 @@ class QuestDefinitionsDao: result = await self._database.execute( "INSERT INTO quest_definitions (title, description, " "icon, schedule_rule_id, due_time, is_active, " - "created_at) VALUES (?, ?, ?, ?, ?, ?, ?)", - (title, description, icon, rule_id, None, 1, created_at), + "created_at, skip_on_away) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + ( + title, + description, + icon, + rule_id, + None, + 1, + created_at, + int(skip_on_away), + ), ) definition_id = result.lastrowid for child_id in assignee_child_ids: @@ -764,6 +780,7 @@ class QuestDefinitionsDao: icon: str | None | object = _UNSET, rule: ScheduleRuleStorage | object = _UNSET, windows: list[tuple[str, str | None]] | object = _UNSET, + skip_on_away: bool | object = _UNSET, ) -> QuestDefinitionSnapshot: """Edit a definition's metadata, rule and windows atomically. @@ -774,9 +791,10 @@ class QuestDefinitionsDao: ``_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). + - ``title``/``description``/``icon``/``skip_on_away`` 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 @@ -809,6 +827,9 @@ class QuestDefinitionsDao: if value is not _UNSET: assignments.append(f"{column} = ?") parameters.append(value) + if skip_on_away is not _UNSET: + assignments.append("skip_on_away = ?") + parameters.append(int(skip_on_away)) if assignments: parameters.append(definition_id) await self._database.execute( diff --git a/custom_components/nestquest/core/materialize.py b/custom_components/nestquest/core/materialize.py index c32d3d9..fee785d 100644 --- a/custom_components/nestquest/core/materialize.py +++ b/custom_components/nestquest/core/materialize.py @@ -18,7 +18,8 @@ The walk is deterministic and idempotent: :class:`~.presence.PresenceEngine` from that same coherent read. - For each date in the range, each definition whose decoded rule :func:`~.recurrence.occurs_on` on that date, each assignee who is - ``is_present`` on that date and each declared window, one instance is + ``is_present`` on that date (or every assignee, when the definition's + ``skip_on_away`` is off) and each declared window, one instance is written through :class:`~.dao_instances.QuestInstancesDao.upsert_if_valid` with a ``generated_at`` stamp shared by the whole batch (one coherent time per run). @@ -208,7 +209,8 @@ async def materialize( definitions = [ (snapshot.definition.id, _decode_rule(snapshot), - snapshot.assignees, snapshot.windows) + snapshot.assignees, snapshot.windows, + snapshot.definition.skip_on_away) for snapshot in snapshots ] @@ -229,13 +231,15 @@ async def materialize( cursor = start while cursor <= end: iso = cursor.isoformat() - for definition_id, rule, assignees, windows in definitions: + for ( + definition_id, rule, assignees, windows, skip_on_away + ) in definitions: if not occurs_on(rule, cursor): continue for child in assignees: if child_filter is not None and child.id not in child_filter: continue - if not engine.is_present(child.id, cursor): + if skip_on_away and not engine.is_present(child.id, cursor): continue for window in windows: written = await instances.upsert_if_valid( diff --git a/custom_components/nestquest/core/migrations.py b/custom_components/nestquest/core/migrations.py index cf40a7a..fba7c50 100644 --- a/custom_components/nestquest/core/migrations.py +++ b/custom_components/nestquest/core/migrations.py @@ -30,8 +30,9 @@ widened (definition_id, child_id, due_date, window) key, and migration 6 rebuilds ``completion_events`` with the D-008 ``actor_child_id`` column. Migration 7 adds the generic ``nestquest_meta_state`` key/value table for integration-owned bookkeeping (Feature 11's -missed-sweep watermark). Each callable migration runs inside its own -transaction +missed-sweep watermark). Migration 8 adds the ``skip_on_away`` +column to ``quest_definitions`` (Feature 22), defaulting to 1. Each +callable migration runs inside its own transaction together with its version stamp, so a crash mid-step rolls the statements and the stamp back together. @@ -400,6 +401,28 @@ async def _add_actor_child_id( await database.execute("PRAGMA foreign_keys = ON") +async def _add_skip_on_away( + database: NestQuestDatabase, target_version: int +) -> None: + """Add the ``skip_on_away`` column to ``quest_definitions``. + + Strictly additive: ``ALTER TABLE ... ADD COLUMN`` with a NOT NULL + DEFAULT 1, so existing rows read as 1 (skip absent dates — the + behaviour before the flag existed). Fresh databases already carry + the column from the canonical DDL, so the ALTER is guarded and the + step only stamps there. + """ + async with database.transaction(): + if not await _table_has_column( + database, "quest_definitions", "skip_on_away" + ): + await database.execute( + "ALTER TABLE quest_definitions ADD COLUMN " + "skip_on_away INTEGER NOT NULL DEFAULT 1" + ) + await stamp_schema_version(database, target_version) + + #: The ordered migration list. Append-only: never edit an applied #: entry, add the next one instead. (Migration 1's content was #: rewritten pre-release per D-007 — no version 1 database shipped.) @@ -423,6 +446,10 @@ MIGRATIONS: Sequence[MigrationStep] = [ # fresh databases (which already made it in migration 1) and # pre-v7 databases. Strictly additive; no existing table changes. list(SCHEMA_V7_META_STATE_DDL), + # Migration 8 (Feature 22): the skip_on_away flag on + # quest_definitions — an additive column defaulting to 1, so every + # pre-existing definition keeps today's skip-when-absent behaviour. + _add_skip_on_away, ] diff --git a/custom_components/nestquest/core/quest_definitions.py b/custom_components/nestquest/core/quest_definitions.py index 643c511..9e347e3 100644 --- a/custom_components/nestquest/core/quest_definitions.py +++ b/custom_components/nestquest/core/quest_definitions.py @@ -191,6 +191,17 @@ def _validate_definition_id(value: object) -> int: return value +def _validate_skip_on_away(value: object) -> bool: + """Reject a non-bool ``skip_on_away`` before any write. + + The DAO's ``int()`` would otherwise silently coerce strings and + numerics ("0", 1) into a state the caller never asked for. + """ + if not isinstance(value, bool): + raise ValueError(f"skip_on_away must be a real bool, got {value!r}") + return value + + def _validate_child_id(value: object) -> int: """Reject non-int child ids (bools included) before any lookup. @@ -212,6 +223,7 @@ async def create_quest_definition( *, description: str | None = None, icon: str | None = None, + skip_on_away: bool = True, ) -> CreatedQuestDefinition: """Create a quest definition (rule + assignees + windows) atomically. @@ -231,6 +243,7 @@ async def create_quest_definition( window_specs = _normalize_windows(windows) description_value = _validate_text(description, "description") icon_value = _validate_text(icon, "icon") + skip_on_away_value = _validate_skip_on_away(skip_on_away) storage = schedule_rule_to_storage(rule) dao = QuestDefinitionsDao(database) @@ -243,6 +256,7 @@ async def create_quest_definition( window_specs, description=description_value, icon=icon_value, + skip_on_away=skip_on_away_value, ) except ValueError as error: # The DAO's assignee check names only the child; re-raise so the @@ -271,6 +285,7 @@ async def edit_quest_definition( icon: str | None | object = _UNSET, rule: ScheduleRule | object = _UNSET, windows: list[str | tuple[str, str | None]] | object = _UNSET, + skip_on_away: bool | None = None, today: datetime.date | None = None, horizon_days: int | None = None, ) -> CreatedQuestDefinition: @@ -285,7 +300,9 @@ async def edit_quest_definition( ``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 + time), matching create's non-empty contract. A provided + ``skip_on_away`` (a real bool) replaces the stored flag; ``None`` + leaves it unchanged. Assignment is NOT settable here. Raises ValueError when the definition does not exist or any argument is rejected. @@ -331,6 +348,10 @@ async def edit_quest_definition( if windows is not _UNSET: window_specs = _normalize_windows(windows) + skip_on_away_value: bool | object = _UNSET + if skip_on_away is not None: + skip_on_away_value = _validate_skip_on_away(skip_on_away) + dao = QuestDefinitionsDao(database) try: snapshot = await dao.edit_definition( @@ -340,6 +361,7 @@ async def edit_quest_definition( icon=icon_value, rule=rule_storage, windows=window_specs, + skip_on_away=skip_on_away_value, ) except ValueError as error: # The DAO's missing-definition error names only the id; re-raise diff --git a/custom_components/nestquest/core/schema.py b/custom_components/nestquest/core/schema.py index b3c3223..0ada459 100644 --- a/custom_components/nestquest/core/schema.py +++ b/custom_components/nestquest/core/schema.py @@ -257,7 +257,8 @@ QUEST_DEFINITIONS_TABLE_SQL = """( schedule_rule_id INTEGER NOT NULL REFERENCES schedule_rules(id), 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 + created_at TEXT NOT NULL, + skip_on_away INTEGER NOT NULL DEFAULT 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 0e378fd..174b773 100644 --- a/tests/test_api_admin_questdef.py +++ b/tests/test_api_admin_questdef.py @@ -215,6 +215,7 @@ async def test_create_persists_definition_with_assignees_and_windows( "description", "icon", "is_active", + "skip_on_away", "rule", "assignees", "windows", @@ -223,6 +224,7 @@ async def test_create_persists_definition_with_assignees_and_windows( assert payload["description"] == "Put everything back where it belongs." assert payload["icon"] == "toy-box" assert payload["is_active"] is True + assert payload["skip_on_away"] is True assert payload["rule"] == _weekly_rule_dict() assert {a["id"]: a["display_name"] for a in payload["assignees"]} == { ada: "Ada", @@ -524,6 +526,58 @@ async def test_edit_rejections_are_422_and_change_nothing( # --- set_active ------------------------------------------------------------- +async def test_edit_toggles_skip_on_away_and_omission_keeps_it( + admin_questdefs: SimpleNamespace, +) -> None: + """A supplied skip_on_away persists; an edit omitting it keeps it.""" + client = admin_questdefs.client + dao = _dao_rules.QuestDefinitionsDao(admin_questdefs.database) + ada = (await _create_children(client, ("Ada",)))[0] + created = await _create_definition(client, [ada]) + assert created.status_code == 201 + definition_id = created.json()["id"] + assert created.json()["skip_on_away"] is True + + toggled_off = await client.patch( + f"/api/v1/admin/quest-definitions/{definition_id}", + json={"skip_on_away": False}, + headers=_admin_headers(), + ) + assert toggled_off.status_code == 200 + assert toggled_off.json()["skip_on_away"] is False + assert (await dao.get(definition_id)).skip_on_away is False + + # An edit that omits the flag leaves the stored False untouched. + renamed = await client.patch( + f"/api/v1/admin/quest-definitions/{definition_id}", + json={"title": "Tidy the den properly"}, + headers=_admin_headers(), + ) + assert renamed.status_code == 200 + assert renamed.json()["skip_on_away"] is False + stored = await dao.get(definition_id) + assert stored.title == "Tidy the den properly" + assert stored.skip_on_away is False + + toggled_on = await client.patch( + f"/api/v1/admin/quest-definitions/{definition_id}", + json={"skip_on_away": True}, + headers=_admin_headers(), + ) + assert toggled_on.status_code == 200 + assert toggled_on.json()["skip_on_away"] is True + assert (await dao.get(definition_id)).skip_on_away is True + + # A non-boolean value is a malformed body: 422, nothing changes. + rejected = await client.patch( + f"/api/v1/admin/quest-definitions/{definition_id}", + json={"skip_on_away": "sometimes"}, + headers=_admin_headers(), + ) + assert rejected.status_code == 422 + assert (await dao.get(definition_id)).skip_on_away is True + + async def test_set_active_toggles_the_flag_and_the_definition_survives( admin_questdefs: SimpleNamespace, ) -> None: diff --git a/tests/test_api_admin_questdef_list.py b/tests/test_api_admin_questdef_list.py index 69d022d..0584695 100644 --- a/tests/test_api_admin_questdef_list.py +++ b/tests/test_api_admin_questdef_list.py @@ -137,6 +137,7 @@ async def test_admin_lists_active_and_inactive_definitions_with_full_shape( "description": "Put everything back where it belongs.", "icon": "toy-box", "is_active": True, + "skip_on_away": True, "rule": { "rule_type": "weekly", "interval": 1, @@ -160,6 +161,7 @@ async def test_admin_lists_active_and_inactive_definitions_with_full_shape( "description": None, "icon": None, "is_active": False, + "skip_on_away": True, "rule": { "rule_type": "daily", "interval": 1, @@ -209,6 +211,53 @@ async def test_list_items_match_the_create_response_shape( assert response.json() == {"definitions": [created.json()]} +async def test_skip_on_away_round_trips_true_and_false_through_list( + admin_client: object, +) -> None: + """skip_on_away posted true, false, or omitted lists back as stored.""" + child = await admin_client.post( + "/api/v1/admin/children", + json={"display_name": "Cleo"}, + headers=_admin_headers(), + ) + child_id = child.json()["id"] + + async def create(title: str, extra: dict[str, object]) -> dict: + response = await admin_client.post( + LIST_PATH, + json={ + "title": title, + "rule": { + "rule_type": "daily", + "interval": 1, + "start_date": "2026-03-01", + }, + "assignee_child_ids": [child_id], + "windows": ["morning"], + **extra, + }, + headers=_admin_headers(), + ) + assert response.status_code == 201 + return response.json() + + skipping = await create("Skip when away", {"skip_on_away": True}) + keeping = await create("Keep when away", {"skip_on_away": False}) + defaulted = await create("Default", {}) + assert skipping["skip_on_away"] is True + assert keeping["skip_on_away"] is False + assert defaulted["skip_on_away"] is True + + response = await admin_client.get(LIST_PATH, headers=_admin_headers()) + + assert response.status_code == 200 + listed = {item["id"]: item for item in response.json()["definitions"]} + assert listed[skipping["id"]]["skip_on_away"] is True + assert listed[keeping["id"]]["skip_on_away"] is False + assert listed[defaulted["id"]]["skip_on_away"] is True + assert response.json() == {"definitions": [skipping, keeping, defaulted]} + + async def test_empty_household_lists_no_definitions( admin_client: object, ) -> None: diff --git a/tests/test_materialize.py b/tests/test_materialize.py index e9df73a..39a5667 100644 --- a/tests/test_materialize.py +++ b/tests/test_materialize.py @@ -657,6 +657,133 @@ def test_materialize_honours_overrides(tmp_path) -> None: _with_db(tmp_path, "materialize-override.db")(_body) +async def _absent_midweek_setup(database, skip_on_away: bool): + """One child absent on days 2..4 of a future week, one daily quest. + + Returns ``(child_id, definition_id, start, end_iso, absent_dates)``. + """ + from custom_components.nestquest.core.dao_presence import ( + PresenceOverridesDao, + ) + + child = await ChildrenDao(database).create("Ada", NOW) + start = _future_monday() + end_iso = (start + datetime.timedelta(days=6)).isoformat() + absent = { + (start + datetime.timedelta(days=offset)).isoformat() + for offset in (2, 3, 4) + } + await PresenceOverridesDao(database).create( + child.id, min(absent), max(absent), False, note="away" + ) + created = await create_quest_definition( + database, + "Daily chore", + ScheduleRule(rule_type=RuleType.DAILY, start_date=start.isoformat()), + [child.id], + ["morning"], + skip_on_away=skip_on_away, + ) + assert created.definition.skip_on_away is skip_on_away + return child.id, created.definition.id, start, end_iso, absent + + +def test_materialize_skip_on_away_true_skips_absent_dates(tmp_path) -> None: + async def _body(database): + child_id, _, start, end_iso, absent = await _absent_midweek_setup( + database, skip_on_away=True + ) + count = await materialize(database, start.isoformat(), end_iso) + records = await QuestInstancesDao(database).list_by_date_range( + child_id, start.isoformat(), end_iso + ) + due_dates = {record.due_date for record in records} + assert count == 4 + assert due_dates.isdisjoint(absent) + assert len(due_dates) == 4 + + _with_db(tmp_path, "skip-on-away-true.db")(_body) + + +def test_materialize_skip_on_away_false_generates_absent_dates( + tmp_path, +) -> None: + async def _body(database): + child_id, _, start, end_iso, absent = await _absent_midweek_setup( + database, skip_on_away=False + ) + count = await materialize(database, start.isoformat(), end_iso) + records = await QuestInstancesDao(database).list_by_date_range( + child_id, start.isoformat(), end_iso + ) + due_dates = {record.due_date for record in records} + assert count == 7 + assert absent <= due_dates + assert len(due_dates) == 7 + + _with_db(tmp_path, "skip-on-away-false.db")(_body) + + +def test_edit_skip_on_away_regenerates_absent_dates(tmp_path) -> None: + """Toggling the flag through the edit path rebuilds future instances.""" + async def _body(database): + child_id, definition_id, start, end_iso, absent = ( + await _absent_midweek_setup(database, skip_on_away=True) + ) + await materialize(database, start.isoformat(), end_iso) + dao = QuestInstancesDao(database) + + async def due_dates() -> set[str]: + records = await dao.list_by_date_range( + child_id, start.isoformat(), end_iso + ) + return {record.due_date for record in records} + + assert (await due_dates()).isdisjoint(absent) + + edited = await edit_quest_definition( + database, definition_id, skip_on_away=False + ) + assert edited.definition.skip_on_away is False + assert absent <= await due_dates() + + # None leaves the stored flag unchanged. + unchanged = await edit_quest_definition( + database, definition_id, title="Daily chore (renamed)" + ) + assert unchanged.definition.skip_on_away is False + + edited = await edit_quest_definition( + database, definition_id, skip_on_away=True + ) + assert edited.definition.skip_on_away is True + assert (await due_dates()).isdisjoint(absent) + + _with_db(tmp_path, "skip-on-away-edit.db")(_body) + + +def test_skip_on_away_rejects_non_bool(tmp_path) -> None: + async def _body(database): + child = await ChildrenDao(database).create("Ada", NOW) + rule = ScheduleRule(rule_type=RuleType.DAILY, start_date="2026-01-01") + with pytest.raises(ValueError, match="skip_on_away must be a real bool"): + await create_quest_definition( + database, "Chore", rule, [child.id], ["morning"], + skip_on_away=1, + ) + created = await create_quest_definition( + database, "Chore", rule, [child.id], ["morning"] + ) + with pytest.raises(ValueError, match="skip_on_away must be a real bool"): + await edit_quest_definition( + database, created.definition.id, skip_on_away="false" + ) + stored = await QuestDefinitionsDao(database).get(created.definition.id) + assert stored.skip_on_away is True + + _with_db(tmp_path, "skip-on-away-validation.db")(_body) + + def test_materialize_rejects_malformed_or_inverted_bounds(tmp_path) -> None: async def _body(database): start = (_future_monday()).isoformat() diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 0bfca68..855164a 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -153,7 +153,9 @@ def test_migration_list_shape() -> None: migration 3 brings pre-D-008 definitions to the multi-assignee model, migration 4 adds the windows table, migration 5 rebuilds quest_instances onto the widened (definition, child, date, window) - key, and migration 6 adds actor_child_id to completion_events. + key, migration 6 adds actor_child_id to completion_events, + migration 7 adds the meta_state table and migration 8 adds + skip_on_away to quest_definitions. """ assert MIGRATIONS[0] == SCHEMA_V1_STATEMENTS assert callable(MIGRATIONS[1]) @@ -162,7 +164,8 @@ def test_migration_list_shape() -> None: assert callable(MIGRATIONS[4]) assert callable(MIGRATIONS[5]) assert MIGRATIONS[6] == SCHEMA_V7_META_STATE_DDL - assert len(MIGRATIONS) == 7 + assert callable(MIGRATIONS[7]) + assert len(MIGRATIONS) == 8 # --------------------------------------------------------------------------- @@ -564,6 +567,13 @@ def test_v2_definitions_rebuilt_to_multi_assignee(tmp_path) -> None: database.fetch_all("PRAGMA table_info(quest_definitions)") ) assert "child_id" not in {c[1] for c in columns} + # The rebuilt row reads the pre-flag behaviour: skip on away. + skip = _run( + database.fetch_one( + "SELECT skip_on_away FROM quest_definitions WHERE id = 1" + ) + ) + assert skip == (1,) # The single assignee moved into the assignees table. assignees = _run( database.fetch_all( @@ -637,6 +647,87 @@ def test_v2_definitions_rebuilt_to_multi_assignee(tmp_path) -> None: _run(database.close()) +# --------------------------------------------------------------------------- +# Migration 8: skip_on_away on quest_definitions (Feature 22) +# --------------------------------------------------------------------------- + + +def _seed_v7_database(database) -> None: + """Build a genuine version-7 database holding one definition. + + Migrations 1..7 run first; the fresh v1 DDL already carries + ``skip_on_away`` (canonical shape), so the column is dropped to + reproduce the exact pre-migration-8 table a shipped v7 file holds, + and a definition row is written through that old column list. + """ + assert _migrate(database, MIGRATIONS[:7]) == 7 + _run( + database.execute( + "ALTER TABLE quest_definitions DROP COLUMN skip_on_away" + ) + ) + columns = _run(database.fetch_all("PRAGMA table_info(quest_definitions)")) + assert "skip_on_away" not in {c[1] for c in columns} + _run( + database.execute( + "INSERT INTO schedule_rules (id, rule_type, interval, " + "start_date) VALUES (1, 'daily', 1, '2026-09-01')" + ) + ) + _run( + database.execute( + "INSERT INTO quest_definitions (id, title, schedule_rule_id, " + "is_active, created_at) VALUES (1, 'Dishes', 1, 1, " + "'2026-09-01T00:00:00+00:00')" + ) + ) + + +def test_v7_definitions_gain_skip_on_away_defaulting_true(tmp_path) -> None: + from custom_components.nestquest.core.dao_rules import ( + QuestDefinitionsDao, + ) + + database = _open_db(tmp_path / "v7-upgrade.db") + try: + _seed_v7_database(database) + before = _version(database) + assert before == 7 + + final = _migrate(database) + + assert final == before + 1 + assert _version(database) == before + 1 + columns = { + c[1]: c + for c in _run( + database.fetch_all("PRAGMA table_info(quest_definitions)") + ) + } + assert "skip_on_away" in columns + # (cid, name, type, notnull, dflt_value, pk) + assert columns["skip_on_away"][2:5] == ("INTEGER", 1, "1") + record = _run(QuestDefinitionsDao(database).get(1)) + assert record is not None + assert record.title == "Dishes" + assert record.skip_on_away is True + finally: + _run(database.close()) + + +def test_skip_on_away_migration_is_noop_on_fresh_schema(tmp_path) -> None: + """A fresh file already has the column; step 8 only stamps.""" + database = _open_db(tmp_path / "fresh-v8.db") + try: + assert _migrate(database) == len(MIGRATIONS) + columns = _run( + database.fetch_all("PRAGMA table_info(quest_definitions)") + ) + assert [c[1] for c in columns].count("skip_on_away") == 1 + finally: + _run(database.close()) + + # --------------------------------------------------------------------------- # Idempotence: second start is a no-op # --------------------------------------------------------------------------- diff --git a/tests/test_schema.py b/tests/test_schema.py index 4a526de..cb26e62 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -322,6 +322,7 @@ def test_quest_definitions_columns_types_and_constraints(tmp_path) -> None: ("due_time", "TEXT", 0, None, 0), ("is_active", "INTEGER", 1, "1", 0), ("created_at", "TEXT", 1, None, 0), + ("skip_on_away", "INTEGER", 1, "1", 0), ] finally: _run(database.close()) -- 2.55.0 From e8ae25e5ebb3554907a2c38400cb2134fc456f53 Mon Sep 17 00:00:00 2001 From: Joshua Date: Wed, 23 Sep 2026 20:40:22 -0400 Subject: [PATCH 2/2] Reject coerced and out-of-range skip_on_away values Review remediation for PR #194: - Admin create/edit request models use StrictBool so JSON 0/1 and "true"/"false" are 422 instead of being coerced to a bool. - Fresh-schema quest_definitions gains CHECK (skip_on_away IN (0, 1)); migrated databases (column added without the CHECK) are guarded by a DAO validator applied at every write path that takes the flag. - QuestDefinitionsDao.update() accepts skip_on_away (None leaves it unchanged), validated like the other write paths. Task: ded75ab8-0219-4d52-aefa-82c09e0b5e6b Co-Authored-By: Claude Opus 5.5 --- api/routes_admin.py | 6 +- custom_components/nestquest/core/dao_rules.py | 31 +++++++-- custom_components/nestquest/core/schema.py | 2 +- tests/test_api_admin_questdef.py | 69 +++++++++++++++++++ tests/test_dao_rules.py | 67 ++++++++++++++++++ tests/test_schema.py | 24 +++++++ 6 files changed, 191 insertions(+), 8 deletions(-) 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: -- 2.55.0