fix(smart-plugs): an edit can no longer save a plug that cannot be read back

CHANGELOG.md

@@ -162,6 +162,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **The frontend build no longer warns about `path` and `crypto` being externalized for the STEP previewer (#2976)** — `occt-import-js`, the Emscripten build behind STEP previews, requires both modules, but only inside its `ENVIRONMENT_IS_NODE` branches; in the browser it loads its `.wasm` from the URL the preview worker passes and draws randomness from `crypto.getRandomValues`. Vite still externalized both and printed two warnings on every build. `vite.config.ts` now drops exactly those two warnings for that one package through `build.rolldownOptions.onLog`, so an externalization anywhere else, or of any other module, still shows.
 
 ### Fixed
+- **Editing a smart plug can no longer save a plug that cannot be read back** — Adding a plug checks its values: a Tasmota plug needs an IPv4 address, a Home Assistant plug an entity of the right kind, an MQTT plug a topic, a REST plug an ON or OFF URL, and the name must not be empty. Editing one did not, so clearing such a field, switching the type without filling in the new one, or typing a hostname as a Tasmota address saved a plug that the smart plug list, the plug itself and the edit all failed on with a server error until it was fixed in the database. Such an edit is now refused with the same message as when adding a plug, and the plug stays as it was. Switching a plug to Tasmota on a printer that already has a Tasmota plug is refused as well (adding one already was); before, it gave the printer a second one, and adding a plug to that printer then failed with a server error.
 - **Importing a project from a file without a priority** — The import gave such a project the priority `0`. On PostgreSQL the import failed with a server error; on SQLite the project got a priority the project editor itself refuses. An imported project now keeps a valid priority (low, normal, high or urgent) and gets **normal** otherwise, from a file and from the JSON import alike.
 - **The "Password Reset" email template is used again** — Since password resets send a link instead of a new password, the reset email (self-service and admin) was always the built-in one: editing the "Password Reset" template under Notifications changed nothing, and its default text still promised a new password. The reset email is now built from that template, and Bambuddy adds the reset link at the end of it (and as a button); the template itself cannot place or contain the link. `{password}` and `{login_url}` are gone from this template. The default text is updated on upgrade unless you edited it; if you did, reword it for a link instead of a new password.
 - **A cost centre is charged for a print whose user was deleted while it ran** — When a user was deleted during their print, the print was never charged: the cost centre's ledger missed its cost, and its budget reservation stayed active for good, holding back that much of the budget. The print is now charged to its cost centre, also to the deleted user's own private one, which their personal prints go through, and its reservation is settled. A queue print is charged under the user who queued it, never under the owner of the archive it reprints. Ledger entries without a user now read "No user" instead of "Deleted user": with login turned off, prints on a cost centre are now charged too and never had a user. Deleting a user together with their items now also releases the budget their queued prints still held; a print of theirs still running is kept until it ends (without a user) and billed to its cost centre like any other. Only a print with no cost centre at all (started while billing was off) has nobody left to charge.

backend/app/api/routes/smart_plugs.py

@@ -3,7 +3,8 @@
 import logging
 
 from fastapi import APIRouter, Body, Depends, HTTPException
-from pydantic import BaseModel
+from fastapi.exceptions import RequestValidationError
+from pydantic import BaseModel, ValidationError
 from sqlalchemy import select
 from sqlalchemy.ext.asyncio import AsyncSession
 
@@ -28,6 +29,7 @@ from backend.app.schemas.smart_plug import (
     HATestConnectionResponse,
     RESTTestConnectionRequest,
     RESTTestConnectionResponse,
+    SmartPlugBase,
     SmartPlugControl,
     SmartPlugCreate,
     SmartPlugEnergy,
@@ -91,6 +93,21 @@ async def list_smart_plugs(
     return [_serialize_plug(plug, can_edit=can_edit) for plug in result.scalars().all()]
 
 
+async def _printer_has_other_tasmota_plug(
+    db: AsyncSession, printer_id: int, exclude_plug_id: int | None = None
+) -> bool:
+    """Whether the printer already has a Tasmota plug (other than this one).
+
+    One per printer: a Tasmota plug is the printer's power device, while HA
+    entities may be several (different automations). Tolerates printers that
+    already hold more than one from before every path checked this.
+    """
+    query = select(SmartPlug.id).where(SmartPlug.printer_id == printer_id, SmartPlug.plug_type == "tasmota")
+    if exclude_plug_id is not None:
+        query = query.where(SmartPlug.id != exclude_plug_id)
+    return (await db.execute(query.limit(1))).first() is not None
+
+
 @router.post("/", response_model=SmartPlugResponseWithSecret)
 async def create_smart_plug(
     data: SmartPlugCreate,
@@ -113,15 +130,8 @@ async def create_smart_plug(
         # Check if printer already has a plug assigned
         # Tasmota plugs: only one per printer (physical power device)
         # HA entities: allow multiple per printer (for different automations)
-        if data.plug_type == "tasmota":
-            result = await db.execute(
-                select(SmartPlug).where(
-                    SmartPlug.printer_id == data.printer_id,
-                    SmartPlug.plug_type == "tasmota",
-                )
-            )
-            if result.scalar_one_or_none():
-                raise HTTPException(400, "This printer already has a Tasmota plug assigned")
+        if data.plug_type == "tasmota" and await _printer_has_other_tasmota_plug(db, data.printer_id):
+            raise HTTPException(400, "This printer already has a Tasmota plug assigned")
 
     # For MQTT plugs, ensure MQTT broker is configured and service is connected
     if data.plug_type == "mqtt":
@@ -617,20 +627,31 @@ async def update_smart_plug(
         if not result.scalar_one_or_none():
             raise HTTPException(400, "Printer not found")
 
-        # Check if that printer already has a different Tasmota plug assigned
-        # Tasmota plugs: only one per printer (physical power device)
-        # HA entities: allow multiple per printer (for different automations)
-        new_plug_type = update_data.get("plug_type", plug.plug_type)
-        if new_plug_type == "tasmota":
-            result = await db.execute(
-                select(SmartPlug).where(
-                    SmartPlug.printer_id == new_printer_id,
-                    SmartPlug.id != plug_id,
-                    SmartPlug.plug_type == "tasmota",
-                )
-            )
-            if result.scalar_one_or_none():
-                raise HTTPException(400, "This printer already has a Tasmota plug assigned")
+    # One Tasmota plug per printer (it is the printer's power device), checked
+    # when this edit makes the plug a Tasmota plug on a printer, by its type or
+    # its printer: a type change alone cannot add a second, and an edit that
+    # changes neither is not refused for plugs stored earlier.
+    merged_type = update_data.get("plug_type", plug.plug_type)
+    merged_printer_id = update_data.get("printer_id", plug.printer_id)
+    becomes_tasmota_here = merged_type != plug.plug_type or merged_printer_id != plug.printer_id
+    if merged_type == "tasmota" and merged_printer_id and becomes_tasmota_here:
+        if await _printer_has_other_tasmota_plug(db, merged_printer_id, exclude_plug_id=plug_id):
+            raise HTTPException(400, "This printer already has a Tasmota plug assigned")
+
+    # The stored plug with the change applied must pass what create checks:
+    # a plug that fails it can no longer be read back (list, detail and this
+    # response all fail). The error names the field to fix, sent or stored,
+    # and carries no values: the stored plug holds write-only credentials.
+    merged = {field: update_data.get(field, getattr(plug, field, None)) for field in SmartPlugBase.model_fields}
+    try:
+        SmartPlugBase.model_validate(merged)
+    except ValidationError as exc:
+        raise RequestValidationError(
+            [
+                {"type": error["type"], "loc": ("body", *error["loc"]), "msg": error["msg"]}
+                for error in exc.errors(include_url=False, include_input=False, include_context=False)
+            ]
+        ) from None
 
     # Track old MQTT settings for comparison
     old_plug_type = plug.plug_type

backend/tests/integration/test_smart_plug_update_type_rules.py

@@ -0,0 +1,205 @@
+"""An update cannot leave a smart plug without what its type needs.
+
+Create checks that a Tasmota plug has an address, a Home Assistant plug an
+entity, an MQTT plug a topic and a REST plug a URL. An update applied the
+change without that check, so clearing the field (or switching the type
+without it) stored a plug every later read of it failed on: a 500 from the
+plug list, the plug detail and the update itself.
+"""
+
+from __future__ import annotations
+
+import pytest
+from httpx import AsyncClient
+
+
+async def _plug(async_client: AsyncClient, **fields) -> dict:
+    response = await async_client.post("/api/v1/smart-plugs/", json={"name": "Plug", **fields})
+    assert response.status_code == 200, response.text
+    return response.json()
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+@pytest.mark.parametrize(
+    ("created", "change", "message"),
+    [
+        ({"ip_address": "192.168.1.100"}, {"ip_address": None}, "ip_address is required for Tasmota plugs"),
+        # As on create: an empty address fails the address format first.
+        ({"ip_address": "192.168.1.100"}, {"ip_address": ""}, "String should match pattern"),
+        (
+            {"ip_address": "192.168.1.100"},
+            {"plug_type": "homeassistant"},
+            "ha_entity_id is required for Home Assistant plugs",
+        ),
+        (
+            {"plug_type": "homeassistant", "ha_entity_id": "switch.printer"},
+            {"ha_entity_id": None},
+            "ha_entity_id is required for Home Assistant plugs",
+        ),
+        (
+            {"plug_type": "mqtt", "mqtt_power_topic": "plug/power"},
+            {"mqtt_power_topic": None},
+            "At least one MQTT topic must be configured",
+        ),
+        (
+            {"plug_type": "rest", "rest_on_url": "http://192.168.1.80/on"},
+            {"rest_on_url": None},
+            "At least one of ON URL or OFF URL is required",
+        ),
+    ],
+)
+async def test_an_update_that_drops_a_required_field_is_refused(
+    async_client: AsyncClient, mock_mqtt_smart_plug_service, created, change, message
+):
+    plug = await _plug(async_client, **created)
+
+    response = await async_client.patch(f"/api/v1/smart-plugs/{plug['id']}", json=change)
+
+    assert response.status_code == 422, response.text
+    assert message in response.text
+    # Nothing was stored: the plug still reads back as it was.
+    after = await async_client.get(f"/api/v1/smart-plugs/{plug['id']}")
+    assert after.status_code == 200, after.text
+    listed = await async_client.get("/api/v1/smart-plugs/")
+    assert listed.status_code == 200, listed.text
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_an_update_that_keeps_the_type_complete_is_applied(async_client: AsyncClient):
+    plug = await _plug(async_client, ip_address="192.168.1.100")
+
+    response = await async_client.patch(
+        f"/api/v1/smart-plugs/{plug['id']}",
+        json={"plug_type": "homeassistant", "ha_entity_id": "switch.printer", "ip_address": None},
+    )
+
+    assert response.status_code == 200, response.text
+    assert response.json()["plug_type"] == "homeassistant"
+    assert response.json()["ha_entity_id"] == "switch.printer"
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_an_unrelated_update_still_works(async_client: AsyncClient):
+    plug = await _plug(async_client, ip_address="192.168.1.100")
+
+    response = await async_client.patch(f"/api/v1/smart-plugs/{plug['id']}", json={"name": "Renamed"})
+
+    assert response.status_code == 200, response.text
+    assert response.json()["name"] == "Renamed"
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+@pytest.mark.parametrize(
+    ("created", "change"),
+    [
+        ({"ip_address": "192.168.1.100"}, {"ip_address": "plug.local"}),
+        ({"ip_address": "192.168.1.100"}, {"name": ""}),
+        ({"ip_address": "192.168.1.100"}, {"name": "x" * 150}),
+        ({"ip_address": "192.168.1.100"}, {"plug_type": "homeassistant", "ha_entity_id": "Bad Entity"}),
+        ({"plug_type": "homeassistant", "ha_entity_id": "switch.printer"}, {"ha_power_entity": "switch.not_a_sensor"}),
+    ],
+)
+async def test_an_update_with_a_value_create_would_refuse_is_refused(async_client: AsyncClient, created, change):
+    """Values in the wrong form broke the read-back the same way."""
+    plug = await _plug(async_client, **created)
+
+    response = await async_client.patch(f"/api/v1/smart-plugs/{plug['id']}", json=change)
+
+    assert response.status_code == 422, response.text
+    assert (await async_client.get(f"/api/v1/smart-plugs/{plug['id']}")).status_code == 200
+    assert (await async_client.get("/api/v1/smart-plugs/")).status_code == 200
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_a_plug_stored_invalid_earlier_can_be_repaired(async_client: AsyncClient, db_session):
+    from backend.app.models.smart_plug import SmartPlug
+
+    plug = SmartPlug(name="Old", plug_type="tasmota", ip_address="plug.local")
+    db_session.add(plug)
+    await db_session.commit()
+
+    response = await async_client.patch(f"/api/v1/smart-plugs/{plug.id}", json={"ip_address": "192.168.1.5"})
+
+    assert response.status_code == 200, response.text
+    assert response.json()["ip_address"] == "192.168.1.5"
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_a_refused_update_does_not_echo_the_stored_plug(async_client: AsyncClient):
+    """The error names the field, nothing else: the stored plug carries its
+    write-only credentials."""
+    plug = await _plug(async_client, ip_address="192.168.1.100", username="admin", password="plug-pw-value-8")
+
+    # Changes that keep the address, so the stored password stays in the plug.
+    for change in ({"name": ""}, {"name": "x" * 150}):
+        response = await async_client.patch(f"/api/v1/smart-plugs/{plug['id']}", json=change)
+        assert response.status_code == 422, response.text
+        assert "plug-pw-value-8" not in response.text
+        assert set().union(*(error.keys() for error in response.json()["detail"])) <= {"type", "loc", "msg"}
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_switching_to_tasmota_keeps_one_tasmota_plug_per_printer(async_client: AsyncClient, printer_factory):
+    """Create allows one Tasmota plug per printer; an edit that only changes
+    the type must not add a second."""
+    printer = await printer_factory(name="P1")
+    await _plug(async_client, ip_address="192.168.1.100", printer_id=printer.id)
+    ha = await _plug(async_client, plug_type="homeassistant", ha_entity_id="switch.p1", printer_id=printer.id)
+
+    response = await async_client.patch(
+        f"/api/v1/smart-plugs/{ha['id']}", json={"plug_type": "tasmota", "ip_address": "192.168.1.101"}
+    )
+
+    assert response.status_code == 400, response.text
+    assert "already has a Tasmota plug" in response.text
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_a_printer_already_holding_two_tasmota_plugs_is_answered_not_crashed(
+    async_client: AsyncClient, db_session, printer_factory
+):
+    """Rows stored before the check covered type changes."""
+    from backend.app.models.smart_plug import SmartPlug
+
+    printer = await printer_factory(name="P2")
+    for ip in ("192.168.1.110", "192.168.1.111"):
+        db_session.add(SmartPlug(name=ip, plug_type="tasmota", ip_address=ip, printer_id=printer.id))
+    await db_session.commit()
+
+    created = await async_client.post(
+        "/api/v1/smart-plugs/", json={"name": "Third", "ip_address": "192.168.1.112", "printer_id": printer.id}
+    )
+
+    assert created.status_code == 400, created.text
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration
+async def test_plugs_on_an_already_doubled_printer_can_still_be_toggled(
+    async_client: AsyncClient, db_session, printer_factory
+):
+    """The check is for an edit that makes a plug the printer's second Tasmota
+    plug; a toggle on either of two stored ones changes neither."""
+    from backend.app.models.smart_plug import SmartPlug
+
+    printer = await printer_factory(name="P3")
+    plugs = [
+        SmartPlug(name=ip, plug_type="tasmota", ip_address=ip, printer_id=printer.id)
+        for ip in ("192.168.1.120", "192.168.1.121")
+    ]
+    db_session.add_all(plugs)
+    await db_session.commit()
+
+    for plug in plugs:
+        response = await async_client.patch(
+            f"/api/v1/smart-plugs/{plug.id}", json={"auto_off": False, "name": "Renamed"}
+        )
+        assert response.status_code == 200, response.text