From 4fdc55b88406088b03419a6b54a66d57213a7bbf Mon Sep 17 00:00:00 2001 From: Kouki Ojima Date: Mon, 28 Sep 2026 17:21:46 +0900 Subject: [PATCH] Let the two stock alert toggles reach the database (issue #2945) (#2956) --- .../tests/_fixtures/notification_toggles.py | 66 +++++++++++++++++ .../integration/test_notifications_api.py | 47 +++++++----- ...test_notification_provider_field_parity.py | 73 +++++++++++++++++++ 3 files changed, 168 insertions(+), 18 deletions(-) create mode 100644 backend/tests/_fixtures/notification_toggles.py create mode 100644 backend/tests/unit/test_notification_provider_field_parity.py diff --git a/backend/tests/_fixtures/notification_toggles.py b/backend/tests/_fixtures/notification_toggles.py new file mode 100644 index 000000000..9c92772f8 --- /dev/null +++ b/backend/tests/_fixtures/notification_toggles.py @@ -0,0 +1,66 @@ +"""The per-event notification toggles, derived from the model's own columns (#2945). + +A hardcoded list only covers the toggles someone remembered to add to it, so it +carries the same hazard as the field maps it is meant to police, one layer up. +Derived, a toggle added tomorrow is exercised the day its column lands. + +This lives here rather than in either test module because the unit parity checks +and the integration round-trips need the same two answers, and a derivation +duplicated across files is a list with extra steps. +""" + +from __future__ import annotations + +from sqlalchemy import Boolean +from sqlalchemy.sql.schema import Column + +from backend.app.models.notification import NotificationProvider + +# The Boolean check is not decoration. An on_* column of some other type would +# turn `{field: True}` into a 422 and break every test parametrised off this +# list, in whichever unrelated PR happened to add that column -- and a spurious +# failure is how a derivation gets edited until it passes. +_EVENT_TOGGLES: list[Column] = [ + column + for column in NotificationProvider.__table__.columns + if column.name.startswith("on_") and isinstance(column.type, Boolean) +] + +EVENT_TOGGLE_COLUMNS: list[str] = sorted(column.name for column in _EVENT_TOGGLES) + +# Every column a client is meant to be able to set. The hazard #2945 describes is +# per-column -- a field that exists everywhere except one of the maps it has to +# cross -- and `enabled`, the quiet-hours and digest fields cross the same two +# maps as the on_* toggles. The prefix is a naming convention, not the boundary. +# What is left out is what the server owns: the key, the timestamps, and the +# delivery status it records itself. +_SERVER_OWNED = {"id", "created_at", "updated_at", "last_success", "last_error", "last_error_at"} + +SETTABLE_COLUMNS: list[str] = sorted( + column.name for column in NotificationProvider.__table__.columns if column.name not in _SERVER_OWNED +) + + +def _model_default(column: Column) -> bool: + """The value a row gets for this column when the caller sends nothing. + + Reads the Python-side default only. That is exact while every toggle + declares one, which ``test_every_event_column_has_a_python_default`` holds + in place: a column added with only a ``server_default`` would read as False + here, get a target of True, and for a server-default-true column the round + trip would send True and assert True -- vacuous again, and silently. + """ + if column.default is None: + return False + return bool(column.default.arg) + + +# The value each toggle has to be driven to for the assertion to mean anything. +# +# Nine of these columns default to True on the model and on the response schema, +# so a test that sends True and asserts True is answered by the default alone: +# a field dropped by a hand-maintained map still reads back True, and the +# mutation that would prove the map is load-bearing survives. Driving each +# toggle to whatever its default is not makes the round trip the only thing that +# can supply the answer. +TOGGLE_TARGET: dict[str, bool] = {column.name: not _model_default(column) for column in _EVENT_TOGGLES} diff --git a/backend/tests/integration/test_notifications_api.py b/backend/tests/integration/test_notifications_api.py index e9596c16d..f03b991b8 100644 --- a/backend/tests/integration/test_notifications_api.py +++ b/backend/tests/integration/test_notifications_api.py @@ -7,6 +7,8 @@ import pytest from httpx import AsyncClient from sqlalchemy import text +from backend.tests._fixtures.notification_toggles import EVENT_TOGGLE_COLUMNS, TOGGLE_TARGET + class TestNotificationsAPI: """Integration tests for /api/v1/notifications/ endpoints.""" @@ -500,7 +502,7 @@ class TestNotificationsAPI: response = await async_client.get(f"/api/v1/notifications/{provider.id}") assert response.json()["on_billing_charge_failed"] is False - # Per-event toggles that live only in these hand-maintained field maps. + # Every per-event toggle, across the hand-maintained field maps. # # These have to be exercised through the route, not the ORM: both # directions of notifications.py are hand-maintained field-by-field maps, @@ -518,52 +520,61 @@ class TestNotificationsAPI: # the toggles could not be turned on at all. @pytest.mark.asyncio @pytest.mark.integration - @pytest.mark.parametrize( - "field", - ["on_ha_sensor_alert", "on_location_ha_sensor_alert", "on_stock_reorder_alert", "on_stock_break_alert"], - ) + @pytest.mark.parametrize("field", EVENT_TOGGLE_COLUMNS) async def test_create_persists_and_returns_the_toggle(self, async_client: AsyncClient, field: str): + # Driven to whatever the column does not default to. Nine of these + # default to True on the model and on the response schema, so sending + # True and asserting True is answered by the default alone -- a field + # dropped by the create constructor still reads back True, and the + # mutation that would prove the constructor load-bearing survives. + target = TOGGLE_TARGET[field] + response = await async_client.post( "/api/v1/notifications/", json={ "name": "Sensor Alert Test", "provider_type": "ntfy", "config": {"server": "https://ntfy.sh", "topic": "test"}, - field: True, + field: target, }, ) assert response.status_code == 200 - assert response.json()[field] is True + assert response.json()[field] is target # Re-read it: a value dropped by the create constructor but echoed # from the request body would still pass the assertion above. provider_id = response.json()["id"] response = await async_client.get(f"/api/v1/notifications/{provider_id}") - assert response.json()[field] is True + assert response.json()[field] is target @pytest.mark.asyncio @pytest.mark.integration - @pytest.mark.parametrize( - "field", - ["on_ha_sensor_alert", "on_location_ha_sensor_alert", "on_stock_reorder_alert", "on_stock_break_alert"], - ) + @pytest.mark.parametrize("field", EVENT_TOGGLE_COLUMNS) async def test_patch_is_reflected_by_every_read_route( self, async_client: AsyncClient, notification_provider_factory, field: str ): - """PATCH already persisted (generic setattr loop) — the reads were the broken half.""" - provider = await notification_provider_factory(**{field: False}) + """PATCH already persisted (generic setattr loop) — the reads were the broken half. - response = await async_client.patch(f"/api/v1/notifications/{provider.id}", json={field: True}) + Seeded at the target's opposite and driven to the target, so the value + asserted is never the one the column would have supplied on its own. + With ``True`` on both sides the nine True-default columns could not see + the half this test exists for: deleting a column from the read map left + them passing. + """ + target = TOGGLE_TARGET[field] + provider = await notification_provider_factory(**{field: not target}) + + response = await async_client.patch(f"/api/v1/notifications/{provider.id}", json={field: target}) assert response.status_code == 200 - assert response.json()[field] is True + assert response.json()[field] is target response = await async_client.get(f"/api/v1/notifications/{provider.id}") - assert response.json()[field] is True + assert response.json()[field] is target response = await async_client.get("/api/v1/notifications/") listed = next(p for p in response.json() if p["id"] == provider.id) - assert listed[field] is True + assert listed[field] is target class TestNotificationTemplatesAPI: diff --git a/backend/tests/unit/test_notification_provider_field_parity.py b/backend/tests/unit/test_notification_provider_field_parity.py new file mode 100644 index 000000000..bcfe40412 --- /dev/null +++ b/backend/tests/unit/test_notification_provider_field_parity.py @@ -0,0 +1,73 @@ +"""Every client-settable notification provider column must reach the API (#2945). + +The defect this pins is not a wrong value, it is a field that exists everywhere +except the modules a toggle has to cross. `on_stock_reorder_alert` and +`on_stock_break_alert` had model columns, service producers, templates, UI +toggles and a frontend test asserting the PATCH — and no schema field, so +Pydantic dropped them, the PATCH answered 200, and nothing was written. #1184 +introduced that gap and every layer it did touch worked, which is why it went +unnoticed for months. + +These are structural checks of structural facts: a column that is absent from a +schema cannot be sent at all, whatever the routes do with it afterwards. The +behaviour behind them — create, both read routes and PATCH, over the same +derived column list — is covered through the API in +`backend/tests/integration/test_notifications_api.py`. + +The two schemas here are the whole of it. The update route needs nothing +beyond `NotificationProviderUpdate`, because it applies changes with a generic +`model_dump(exclude_unset=True)` + `setattr` loop rather than a third +hand-maintained map: once the field survives the schema, it is written. +""" + +from __future__ import annotations + +import pytest + +from backend.app.models.notification import NotificationProvider +from backend.app.schemas.notification import ( + NotificationProviderCreate, + NotificationProviderUpdate, +) +from backend.tests._fixtures.notification_toggles import ( + EVENT_TOGGLE_COLUMNS, + SETTABLE_COLUMNS, + TOGGLE_TARGET, +) + + +def test_there_are_event_columns_to_check() -> None: + """Guard the guard: an empty enumeration would make every test below vacuous.""" + assert len(EVENT_TOGGLE_COLUMNS) > 20 + + +def test_the_targets_cover_both_directions() -> None: + """Guard the other guard, the one the integration round-trips lean on. + + ``TOGGLE_TARGET`` exists so a test drives each toggle to whatever its + default is not. If every column defaulted the same way -- or if the default + lookup quietly started returning one constant -- the targets would collapse + to a single value and the round-trips would be answered by the default again + without anything failing. Both values have to appear. + """ + assert set(TOGGLE_TARGET.values()) == {True, False} + + +@pytest.mark.parametrize("column", EVENT_TOGGLE_COLUMNS) +def test_every_event_column_has_a_python_default(column: str) -> None: + """``TOGGLE_TARGET`` derives each target from the Python-side default, so a + toggle that only has a ``server_default`` would get the wrong target and + its round trip would pass on the default alone. Fail here instead.""" + assert NotificationProvider.__table__.columns[column].default is not None + + +@pytest.mark.parametrize("column", SETTABLE_COLUMNS) +def test_every_settable_column_is_settable_on_create(column: str) -> None: + """Absent from the Create schema, the field is silently dropped from the POST.""" + assert column in NotificationProviderCreate.model_fields + + +@pytest.mark.parametrize("column", SETTABLE_COLUMNS) +def test_every_settable_column_is_settable_on_update(column: str) -> None: + """This is the one the report hit: the PATCH succeeds and writes nothing.""" + assert column in NotificationProviderUpdate.model_fields