From 60383bee6403faf4bf2efb5e8e3832f14faa92ef Mon Sep 17 00:00:00 2001 From: Pouzor Date: Fri, 3 Jul 2026 01:52:56 +0200 Subject: [PATCH 1/4] fix: tolerate same device on multiple canvases in zigbee/zwave import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zigbee and Z-Wave imports looked up canvas nodes by ieee_address with scalar_one_or_none(), assuming one node per IEEE globally. A device placed on two designs (one Node per canvas — a supported feature) made re-import crash with MultipleResultsFound. - Both mesh imports now refresh properties on every matching node instead of a single row (loop over .scalars().all()). - approve_device guards against a true duplicate: same IEEE already on the SAME design reuses that node instead of inserting a second one. - New node_dedupe service: loss-free repair keyed on (ieee, design_id). Collapses only genuine same-canvas duplicates (merges properties/services/ missing fields, re-points edges + parent_id, drops self-loops/parallel edges). Cross-design placements preserved. Runs at start of both imports and bulk-approve. No-op on healthy DBs. - IP correlation path already handled multiple nodes; left unchanged. Tests: dedupe unit tests (collapse, cross-design preservation, edge/parent re-point, idempotent), zigbee + zwave multi-canvas regression, approve no-dupe guard. ha-relevant: yes --- backend/app/api/routes/scan.py | 38 ++++++- backend/app/api/routes/zigbee.py | 46 +++++--- backend/app/api/routes/zwave.py | 45 +++++--- backend/app/services/node_dedupe.py | 161 ++++++++++++++++++++++++++++ backend/tests/test_node_dedupe.py | 117 ++++++++++++++++++++ backend/tests/test_scan.py | 38 +++++++ backend/tests/test_zigbee_router.py | 37 +++++++ backend/tests/test_zwave_router.py | 35 ++++++ 8 files changed, 485 insertions(+), 32 deletions(-) create mode 100644 backend/app/services/node_dedupe.py create mode 100644 backend/tests/test_node_dedupe.py diff --git a/backend/app/api/routes/scan.py b/backend/app/api/routes/scan.py index 1b3fd06..2b778d2 100644 --- a/backend/app/api/routes/scan.py +++ b/backend/app/api/routes/scan.py @@ -15,8 +15,12 @@ from app.db.database import AsyncSessionLocal, get_db from app.db.models import Design, Edge, Node, PendingDevice, PendingDeviceLink, ScanRun from app.schemas.nodes import NodeCreate from app.schemas.scan import PendingDeviceResponse, ScanRunResponse +from app.services.node_dedupe import dedupe_nodes_by_ieee from app.services.scanner import DeepScanOptions, _valid_port_range, request_cancel, run_scan -from app.services.zigbee_service import build_zigbee_properties +from app.services.zigbee_service import ( + build_zigbee_properties, + merge_zigbee_properties, +) from app.services.zwave_service import build_zwave_properties _ZIGBEE_TYPES = {"zigbee_coordinator", "zigbee_router", "zigbee_enddevice"} @@ -308,6 +312,9 @@ async def bulk_approve_devices( db: AsyncSession = Depends(get_db), _: str = Depends(get_current_user), ) -> dict[str, Any]: + # Repair any legacy same-canvas duplicate nodes before placing more. + await dedupe_nodes_by_ieee(db) + # Target the design the user is on; fall back to the first design. default_design_id = payload.design_id if default_design_id is None: @@ -469,6 +476,35 @@ async def approve_device( raise HTTPException(status_code=409, detail="Device already processed") device.status = "approved" wireless = _is_wireless(node_data.type) + + # Guard against a true duplicate: same IEEE already placed on THIS design. + # (The same device on a *different* design is valid — one Node per canvas.) + if device.ieee_address is not None: + dup = ( + await db.execute( + select(Node).where( + Node.ieee_address == device.ieee_address, + Node.design_id == node_design_id, + ) + ) + ).scalars().first() + if dup is not None: + dup.properties = merge_zigbee_properties( + dup.properties, + _wireless_properties( + node_data.type, device.ieee_address, + device.vendor, device.model, device.lqi, + ) if wireless else [], + ) + edges = await _resolve_pending_links_for_ieee(db, device.ieee_address) + await db.commit() + return { + "approved": True, + "node_id": dup.id, + "edges_created": len(edges), + "edges": edges, + } + # Prefer the MAC discovered during the scan (stored on the pending device); # fall back to whatever the approve payload carried. _mac = device.mac or node_data.mac diff --git a/backend/app/api/routes/zigbee.py b/backend/app/api/routes/zigbee.py index 9e69af1..dd62039 100644 --- a/backend/app/api/routes/zigbee.py +++ b/backend/app/api/routes/zigbee.py @@ -23,6 +23,7 @@ from app.schemas.zigbee import ( ZigbeeTestConnectionRequest, ZigbeeTestConnectionResponse, ) +from app.services.node_dedupe import dedupe_nodes_by_ieee from app.services.zigbee_service import ( build_zigbee_properties, fetch_networkmap, @@ -138,6 +139,10 @@ async def _persist_pending_import( Coordinator auto-approves to a canvas Node. Other devices upsert by IEEE. All zigbee-source links are wiped and re-inserted from the new map. """ + # Repair any pre-existing duplicate nodes (same IEEE) before upserting, so + # the by-IEEE lookups below resolve to a single row. + await dedupe_nodes_by_ieee(db) + # Determine target design (use first design as fallback) first_design = (await db.execute(select(Design).order_by(Design.created_at).limit(1))).scalar() default_design_id = first_design.id if first_design else None @@ -156,15 +161,22 @@ async def _persist_pending_import( ) if n.get("device_type") == "Coordinator": - existing = await db.execute(select(Node).where(Node.ieee_address == ieee)) - existing_node = existing.scalar_one_or_none() - if existing_node: - existing_node.properties = merge_zigbee_properties( - existing_node.properties, props + # The coordinator may sit on several canvases (one Node per design). + # Refresh props on every matching node, not just one. + existing_nodes = ( + await db.execute( + select(Node).where(Node.ieee_address == ieee).order_by(Node.id) ) + ).scalars().all() + if existing_nodes: + for existing_node in existing_nodes: + existing_node.properties = merge_zigbee_properties( + existing_node.properties, props + ) + first = existing_nodes[0] coordinator_out = ZigbeeCoordinatorOut( - id=existing_node.id, - label=existing_node.label, + id=first.id, + label=first.label, ieee_address=ieee, ) coordinator_existed = True @@ -188,16 +200,18 @@ async def _persist_pending_import( continue # If the device has already been approved as a canvas Node, refresh - # its properties and skip creating a pending row (keeps approved - # devices out of pending/hidden modals on re-import). - existing_node_q = await db.execute( - select(Node).where(Node.ieee_address == ieee) - ) - existing_node = existing_node_q.scalar_one_or_none() - if existing_node: - existing_node.properties = merge_zigbee_properties( - existing_node.properties, props + # its properties on every canvas it sits on and skip creating a pending + # row (keeps approved devices out of pending/hidden modals on re-import). + existing_nodes = ( + await db.execute( + select(Node).where(Node.ieee_address == ieee).order_by(Node.id) ) + ).scalars().all() + if existing_nodes: + for existing_node in existing_nodes: + existing_node.properties = merge_zigbee_properties( + existing_node.properties, props + ) continue result = await db.execute( diff --git a/backend/app/api/routes/zwave.py b/backend/app/api/routes/zwave.py index 241dc38..56fc355 100644 --- a/backend/app/api/routes/zwave.py +++ b/backend/app/api/routes/zwave.py @@ -23,6 +23,7 @@ from app.schemas.zwave import ( ZwaveTestConnectionRequest, ZwaveTestConnectionResponse, ) +from app.services.node_dedupe import dedupe_nodes_by_ieee from app.services.zwave_service import ( build_zwave_properties, fetch_zwave_network, @@ -134,6 +135,10 @@ async def _persist_pending_import( Coordinator auto-approves to a canvas Node. Other devices upsert by Z-Wave identity. All zwave-source links are wiped and re-inserted from the new map. """ + # Repair any pre-existing same-canvas duplicate nodes before upserting, so + # the by-IEEE lookups below resolve cleanly. + await dedupe_nodes_by_ieee(db) + first_design = (await db.execute(select(Design).order_by(Design.created_at).limit(1))).scalar() default_design_id = first_design.id if first_design else None @@ -149,15 +154,22 @@ async def _persist_pending_import( props = build_zwave_properties(ieee, n.get("vendor"), n.get("model")) if n.get("type") == "zwave_coordinator": - existing = await db.execute(select(Node).where(Node.ieee_address == ieee)) - existing_node = existing.scalar_one_or_none() - if existing_node: - existing_node.properties = merge_zwave_properties( - existing_node.properties, props + # The coordinator may sit on several canvases (one Node per design). + # Refresh props on every matching node, not just one. + existing_nodes = ( + await db.execute( + select(Node).where(Node.ieee_address == ieee).order_by(Node.id) ) + ).scalars().all() + if existing_nodes: + for existing_node in existing_nodes: + existing_node.properties = merge_zwave_properties( + existing_node.properties, props + ) + first = existing_nodes[0] coordinator_out = ZwaveCoordinatorOut( - id=existing_node.id, - label=existing_node.label, + id=first.id, + label=first.label, ieee_address=ieee, ) coordinator_existed = True @@ -180,15 +192,18 @@ async def _persist_pending_import( ) continue - # Already approved as a canvas Node → refresh props, skip pending row. - existing_node_q = await db.execute( - select(Node).where(Node.ieee_address == ieee) - ) - existing_node = existing_node_q.scalar_one_or_none() - if existing_node: - existing_node.properties = merge_zwave_properties( - existing_node.properties, props + # Already approved as a canvas Node → refresh props on every canvas it + # sits on, skip pending row. + existing_nodes = ( + await db.execute( + select(Node).where(Node.ieee_address == ieee).order_by(Node.id) ) + ).scalars().all() + if existing_nodes: + for existing_node in existing_nodes: + existing_node.properties = merge_zwave_properties( + existing_node.properties, props + ) continue result = await db.execute( diff --git a/backend/app/services/node_dedupe.py b/backend/app/services/node_dedupe.py new file mode 100644 index 0000000..76fa1cd --- /dev/null +++ b/backend/app/services/node_dedupe.py @@ -0,0 +1,161 @@ +"""Collapse *true* duplicate canvas nodes: same ``ieee_address`` **and** same +``design_id`` (i.e. the same device placed twice on the *same* canvas). + +The same device legitimately appears on multiple canvases — placing a device on +another design creates a second :class:`Node` for that IEEE by design (see the +per-design guard in ``bulk_approve_devices``). Those cross-design rows are NOT +duplicates and must be preserved. Only two rows sharing both the IEEE *and* the +design are corrupt, and only those are collapsed here. + +This module provides an idempotent, **loss-free** repair: per ``(ieee, +design_id)`` group with >1 node it keeps the oldest as canonical, merges the +extras' data into it (properties, missing scalar fields, services), re-points +every edge and ``parent_id`` reference onto the canonical node, then deletes the +extras. Edges are de-duplicated and self-loops dropped after re-pointing so no +dangling or redundant links remain. +""" + +from __future__ import annotations + +import logging +from typing import Any + +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from app.db.models import Edge, Node +from app.services.zigbee_service import merge_zigbee_properties + +logger = logging.getLogger(__name__) + +# Scalar Node fields worth carrying over from a duplicate when the canonical +# node has no value. Positions, design, parent and identity fields are left on +# the canonical node untouched (that's the row the user actually placed). +_FILLABLE_FIELDS = ( + "hostname", + "ip", + "mac", + "os", + "check_method", + "check_target", + "notes", + "cpu_count", + "cpu_model", + "ram_gb", + "disk_gb", + "custom_icon", + "last_seen", + "last_scan", +) + + +def _merge_services( + a: list[Any] | None, b: list[Any] | None +) -> list[Any]: + """Union two service lists, de-duplicated, order-stable.""" + out: list[Any] = list(a or []) + seen = {repr(s) for s in out} + for s in b or []: + if repr(s) not in seen: + out.append(s) + seen.add(repr(s)) + return out + + +def _merge_into_canonical(canonical: Node, dup: Node) -> None: + """Fold ``dup``'s data into ``canonical`` in place (no field lost).""" + canonical.properties = merge_zigbee_properties( + canonical.properties, dup.properties or [] + ) + canonical.services = _merge_services(canonical.services, dup.services) + for field in _FILLABLE_FIELDS: + if getattr(canonical, field, None) in (None, "") and getattr(dup, field, None) not in (None, ""): + setattr(canonical, field, getattr(dup, field)) + # Prefer a human label over a bare IEEE/hex fallback. + if (not canonical.label or canonical.label == canonical.ieee_address) and dup.label: + canonical.label = dup.label + + +async def dedupe_nodes_by_ieee(db: AsyncSession) -> int: + """Merge duplicate nodes sharing an ``ieee_address`` AND ``design_id``. + + Returns the number of nodes removed. Idempotent: a no-op when every + ``(ieee, design)`` pair maps to at most one node. Nodes with the same IEEE + on *different* designs are left untouched (valid cross-canvas placement). + Does not commit — the caller owns the transaction. + """ + rows = ( + await db.execute( + select(Node) + .where(Node.ieee_address.is_not(None)) + .order_by(Node.ieee_address, Node.created_at, Node.id) + ) + ).scalars().all() + + groups: dict[tuple[str, str | None], list[Node]] = {} + for node in rows: + groups.setdefault((node.ieee_address, node.design_id), []).append(node) # type: ignore[arg-type] + + removed = 0 + for (ieee, _design), nodes in groups.items(): + if len(nodes) < 2: + continue + canonical, *dups = nodes # oldest first (ordered above) + dup_ids = {d.id for d in dups} + + for dup in dups: + _merge_into_canonical(canonical, dup) + + # Re-point edges + parents, then drop self-loops / duplicates. + edges = ( + await db.execute( + select(Edge).where( + Edge.source.in_(dup_ids) | Edge.target.in_(dup_ids) + ) + ) + ).scalars().all() + for edge in edges: + if edge.source in dup_ids: + edge.source = canonical.id + if edge.target in dup_ids: + edge.target = canonical.id + + # Re-point children whose parent was a duplicate. + children = ( + await db.execute(select(Node).where(Node.parent_id.in_(dup_ids))) + ).scalars().all() + for child in children: + child.parent_id = canonical.id + + # Collapse self-loops and now-redundant parallel edges. + all_edges = ( + await db.execute( + select(Edge).where( + (Edge.source == canonical.id) | (Edge.target == canonical.id) + ) + ) + ).scalars().all() + seen_pairs: set[tuple[str, str, str]] = set() + for edge in all_edges: + if edge.source == edge.target: + await db.delete(edge) + continue + key = (edge.source, edge.target, edge.type) + if key in seen_pairs: + await db.delete(edge) + continue + seen_pairs.add(key) + + await db.flush() + for dup in dups: + await db.delete(dup) + removed += 1 + + logger.info( + "Deduped IEEE %s: merged %d duplicate node(s) into %s", + ieee, len(dups), canonical.id, + ) + + if removed: + await db.flush() + return removed diff --git a/backend/tests/test_node_dedupe.py b/backend/tests/test_node_dedupe.py new file mode 100644 index 0000000..be702f1 --- /dev/null +++ b/backend/tests/test_node_dedupe.py @@ -0,0 +1,117 @@ +"""Tests for the same-canvas node dedupe repair (app.services.node_dedupe).""" + +import pytest +from sqlalchemy import select + +from app.db.models import Design, Edge, Node +from app.services.node_dedupe import dedupe_nodes_by_ieee + + +async def _design(db, name="d1"): + d = Design(name=name) + db.add(d) + await db.flush() + return d + + +@pytest.mark.asyncio +async def test_collapses_same_ieee_same_design(db_session): + d = await _design(db_session) + keep = Node( + label="Sensor", type="zigbee_enddevice", design_id=d.id, + ieee_address="0xAAA", properties=[{"key": "IEEE", "value": "0xAAA", "visible": True}], + pos_x=100, pos_y=200, + ) + db_session.add(keep) + await db_session.flush() + dup = Node( + label="Sensor", type="zigbee_enddevice", design_id=d.id, + ieee_address="0xAAA", ip="10.0.0.5", + properties=[{"key": "LQI", "value": "88", "visible": False}], + ) + db_session.add(dup) + await db_session.flush() + + removed = await dedupe_nodes_by_ieee(db_session) + assert removed == 1 + + nodes = (await db_session.execute(select(Node).where(Node.ieee_address == "0xAAA"))).scalars().all() + assert len(nodes) == 1 + survivor = nodes[0] + assert survivor.id == keep.id # oldest kept + assert survivor.pos_x == 100 # canvas position preserved + assert survivor.ip == "10.0.0.5" # missing field filled from dup + keys = {p["key"] for p in survivor.properties} + assert keys == {"IEEE", "LQI"} # properties merged + + +@pytest.mark.asyncio +async def test_preserves_same_ieee_across_designs(db_session): + """Same device on two canvases is valid — must NOT be merged.""" + d1 = await _design(db_session, "d1") + d2 = await _design(db_session, "d2") + for d in (d1, d2): + db_session.add(Node(label="S", type="zigbee_enddevice", design_id=d.id, ieee_address="0xBBB")) + await db_session.flush() + + removed = await dedupe_nodes_by_ieee(db_session) + assert removed == 0 + + nodes = (await db_session.execute(select(Node).where(Node.ieee_address == "0xBBB"))).scalars().all() + assert len(nodes) == 2 + + +@pytest.mark.asyncio +async def test_repoints_edges_and_drops_dupes(db_session): + d = await _design(db_session) + keep = Node(label="A", type="server", design_id=d.id, ieee_address="0xCCC") + dup = Node(label="A", type="server", design_id=d.id, ieee_address="0xCCC") + other = Node(label="B", type="server", design_id=d.id) + db_session.add_all([keep, other]) + await db_session.flush() + db_session.add(dup) + await db_session.flush() + + # keep<->other and dup<->other (parallel after repoint), plus dup<->keep (self-loop). + db_session.add_all([ + Edge(source=keep.id, target=other.id, type="ethernet", design_id=d.id), + Edge(source=dup.id, target=other.id, type="ethernet", design_id=d.id), + Edge(source=dup.id, target=keep.id, type="ethernet", design_id=d.id), + ]) + await db_session.flush() + + removed = await dedupe_nodes_by_ieee(db_session) + assert removed == 1 + + edges = (await db_session.execute(select(Edge))).scalars().all() + # self-loop dropped, parallel edge collapsed -> a single keep<->other edge + assert len(edges) == 1 + e = edges[0] + assert {e.source, e.target} == {keep.id, other.id} + + +@pytest.mark.asyncio +async def test_repoints_child_parent(db_session): + d = await _design(db_session) + keep = Node(label="Host", type="proxmox", design_id=d.id, ieee_address="0xDDD") + dup = Node(label="Host", type="proxmox", design_id=d.id, ieee_address="0xDDD") + db_session.add(keep) + await db_session.flush() + db_session.add(dup) + await db_session.flush() + child = Node(label="VM", type="vm", design_id=d.id, parent_id=dup.id) + db_session.add(child) + await db_session.flush() + + await dedupe_nodes_by_ieee(db_session) + await db_session.refresh(child) + assert child.parent_id == keep.id + + +@pytest.mark.asyncio +async def test_idempotent_noop_when_unique(db_session): + d = await _design(db_session) + db_session.add(Node(label="X", type="server", design_id=d.id, ieee_address="0xEEE")) + await db_session.flush() + assert await dedupe_nodes_by_ieee(db_session) == 0 + assert await dedupe_nodes_by_ieee(db_session) == 0 diff --git a/backend/tests/test_scan.py b/backend/tests/test_scan.py index 2114bb1..8087e20 100644 --- a/backend/tests/test_scan.py +++ b/backend/tests/test_scan.py @@ -311,6 +311,44 @@ async def test_approve_device(client: AsyncClient, headers, pending_device): assert inventory[0]["status"] == "approved" +@pytest.mark.asyncio +async def test_approve_device_no_dupe_same_ieee_same_design( + client: AsyncClient, headers, db_session +): + """Approving a device whose IEEE is already on the target design must reuse + the existing node, not create a duplicate (source of the crash bug).""" + design = Design(name="d1") + db_session.add(design) + await db_session.flush() + existing = Node( + label="sensor", type="zigbee_enddevice", ieee_address="0xZZZ", + services=[], design_id=design.id, + ) + db_session.add(existing) + device = PendingDevice( + id=str(uuid.uuid4()), ieee_address="0xZZZ", suggested_type="zigbee_enddevice", + status="pending", discovery_source="zigbee", + ) + db_session.add(device) + await db_session.commit() + + res = await client.post( + f"/api/v1/scan/pending/{device.id}/approve", + json={ + "label": "sensor", "type": "zigbee_enddevice", + "status": "online", "services": [], "design_id": design.id, + }, + headers=headers, + ) + assert res.status_code == 200 + assert res.json()["node_id"] == existing.id # reused, not new + + nodes = ( + await db_session.execute(select(Node).where(Node.ieee_address == "0xZZZ")) + ).scalars().all() + assert len(nodes) == 1 # no duplicate created + + @pytest.mark.asyncio async def test_approve_nonexistent_device(client: AsyncClient, headers): node_payload = { diff --git a/backend/tests/test_zigbee_router.py b/backend/tests/test_zigbee_router.py index 5096531..7467f15 100644 --- a/backend/tests/test_zigbee_router.py +++ b/backend/tests/test_zigbee_router.py @@ -618,6 +618,43 @@ async def test_persist_pending_import_refreshes_existing_coordinator_properties( assert by_key["Model"]["visible"] is False +@pytest.mark.asyncio +async def test_persist_pending_import_device_on_multiple_canvases( + db_session, +) -> None: + """Regression: a device approved onto TWO designs (one Node each) must not + crash re-import with MultipleResultsFound — props refresh on both nodes.""" + from sqlalchemy import select + + from app.api.routes.zigbee import _persist_pending_import + from app.db.models import Design, Node + + d1 = Design(name="d1") + d2 = Design(name="d2") + db_session.add_all([d1, d2]) + await db_session.flush() + for d in (d1, d2): + db_session.add(Node( + label="router_1", type="zigbee_router", status="online", + check_method="none", ieee_address="0xR1", services=[], + properties=[], design_id=d.id, + )) + await db_session.commit() + + bumped = [dict(n) for n in _PENDING_NODES] + bumped[1]["lqi"] = 240 + # Must not raise. + await _persist_pending_import(db_session, bumped, _PENDING_EDGES) + + nodes = ( + await db_session.execute(select(Node).where(Node.ieee_address == "0xR1")) + ).scalars().all() + assert len(nodes) == 2 # both canvas placements preserved + for n in nodes: + lqi = {p["key"]: p["value"] for p in n.properties}.get("LQI") + assert lqi == "240" # refreshed on every canvas + + @pytest.mark.asyncio async def test_import_pending_requires_auth(client: AsyncClient) -> None: res = await client.post( diff --git a/backend/tests/test_zwave_router.py b/backend/tests/test_zwave_router.py index 6974738..3aa7130 100644 --- a/backend/tests/test_zwave_router.py +++ b/backend/tests/test_zwave_router.py @@ -445,3 +445,38 @@ async def test_persist_keeps_hidden_hidden(db_session) -> None: ) ).scalar_one() assert still_hidden.status == "hidden" + + +@pytest.mark.asyncio +async def test_persist_device_on_multiple_canvases(db_session) -> None: + """Regression: a device on TWO designs (one Node each) must not crash + re-import with MultipleResultsFound — props refresh on both nodes.""" + from sqlalchemy import select + + from app.api.routes.zwave import _persist_pending_import + from app.db.models import Design, Node + + d1 = Design(name="d1") + d2 = Design(name="d2") + db_session.add_all([d1, d2]) + await db_session.flush() + for d in (d1, d2): + db_session.add(Node( + label="Wall Plug", type="zwave_router", status="online", + check_method="none", ieee_address="zwave-0xh-2", services=[], + properties=[], design_id=d.id, + )) + await db_session.commit() + + bumped = [dict(n) for n in _PENDING_NODES] + bumped[1]["model"] = "ZW200" + # Must not raise. + await _persist_pending_import(db_session, bumped, _PENDING_EDGES) + + nodes = ( + await db_session.execute(select(Node).where(Node.ieee_address == "zwave-0xh-2")) + ).scalars().all() + assert len(nodes) == 2 # both canvas placements preserved + for n in nodes: + model = {p["key"]: p["value"] for p in n.properties}.get("Model") + assert model == "ZW200" # refreshed on every canvas From 3f6e9b00f728828ccab58b21b556d28d8dbfd6e5 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Fri, 3 Jul 2026 02:43:42 +0200 Subject: [PATCH 2/4] fix: send zigbee/zwave coordinator to pending inventory, not auto-canvas The coordinator was special-cased in both mesh imports to auto-create a canvas Node, so it never appeared in the pending inventory and users could not approve/hide/type it like every other device. Remove the auto-placement: the coordinator now flows through the shared pending path (upsert into pending_devices with suggested_type zigbee_coordinator / zwave_coordinator). An already-approved coordinator Node still gets its properties refreshed on re-import via the shared approved-node path. The response's coordinator/coordinator_already_existed fields are retained (now always unset) for backward-compatible shape; the frontend already ignored them. Tests updated: coordinator lands in pending (counts include it), no Node auto-created, pending metadata carried, approved coordinator refresh + no pending re-list. ha-relevant: yes --- backend/app/api/routes/zigbee.py | 49 +++---------------- backend/app/api/routes/zwave.py | 48 +++---------------- backend/tests/test_zigbee_router.py | 73 ++++++++++++++++++++--------- backend/tests/test_zwave_router.py | 43 ++++++++++++----- 4 files changed, 93 insertions(+), 120 deletions(-) diff --git a/backend/app/api/routes/zigbee.py b/backend/app/api/routes/zigbee.py index dd62039..d59bfaf 100644 --- a/backend/app/api/routes/zigbee.py +++ b/backend/app/api/routes/zigbee.py @@ -11,7 +11,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.api.deps import get_current_user from app.db.database import AsyncSessionLocal, get_db -from app.db.models import Design, Node, PendingDevice, PendingDeviceLink, ScanRun +from app.db.models import Node, PendingDevice, PendingDeviceLink, ScanRun from app.schemas.scan import ScanRunResponse from app.schemas.zigbee import ( ZigbeeCoordinatorOut, @@ -143,10 +143,8 @@ async def _persist_pending_import( # the by-IEEE lookups below resolve to a single row. await dedupe_nodes_by_ieee(db) - # Determine target design (use first design as fallback) - first_design = (await db.execute(select(Design).order_by(Design.created_at).limit(1))).scalar() - default_design_id = first_design.id if first_design else None - + # Coordinator is no longer auto-placed, so the response's coordinator fields + # stay unset — retained for backward-compatible response shape. coordinator_out: ZigbeeCoordinatorOut | None = None coordinator_existed = False pending_created = 0 @@ -160,44 +158,9 @@ async def _persist_pending_import( ieee, n.get("vendor"), n.get("model"), n.get("lqi") ) - if n.get("device_type") == "Coordinator": - # The coordinator may sit on several canvases (one Node per design). - # Refresh props on every matching node, not just one. - existing_nodes = ( - await db.execute( - select(Node).where(Node.ieee_address == ieee).order_by(Node.id) - ) - ).scalars().all() - if existing_nodes: - for existing_node in existing_nodes: - existing_node.properties = merge_zigbee_properties( - existing_node.properties, props - ) - first = existing_nodes[0] - coordinator_out = ZigbeeCoordinatorOut( - id=first.id, - label=first.label, - ieee_address=ieee, - ) - coordinator_existed = True - continue - label = n.get("friendly_name") or ieee - node = Node( - label=label, - type=n.get("type") or "zigbee_coordinator", - status="online", - check_method="none", - ieee_address=ieee, - services=[], - properties=props, - design_id=default_design_id, - ) - db.add(node) - await db.flush() - coordinator_out = ZigbeeCoordinatorOut( - id=node.id, label=label, ieee_address=ieee - ) - continue + # The coordinator is no longer auto-placed on the canvas — it flows to + # the pending inventory like every other device, so the user approves it + # explicitly. Only the shared paths below run for it. # If the device has already been approved as a canvas Node, refresh # its properties on every canvas it sits on and skip creating a pending diff --git a/backend/app/api/routes/zwave.py b/backend/app/api/routes/zwave.py index 56fc355..9f20241 100644 --- a/backend/app/api/routes/zwave.py +++ b/backend/app/api/routes/zwave.py @@ -11,7 +11,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.api.deps import get_current_user from app.db.database import AsyncSessionLocal, get_db -from app.db.models import Design, Node, PendingDevice, PendingDeviceLink, ScanRun +from app.db.models import Node, PendingDevice, PendingDeviceLink, ScanRun from app.schemas.scan import ScanRunResponse from app.schemas.zwave import ( ZwaveCoordinatorOut, @@ -139,9 +139,8 @@ async def _persist_pending_import( # the by-IEEE lookups below resolve cleanly. await dedupe_nodes_by_ieee(db) - first_design = (await db.execute(select(Design).order_by(Design.created_at).limit(1))).scalar() - default_design_id = first_design.id if first_design else None - + # Coordinator is no longer auto-placed, so the response's coordinator fields + # stay unset — retained for backward-compatible response shape. coordinator_out: ZwaveCoordinatorOut | None = None coordinator_existed = False pending_created = 0 @@ -153,44 +152,9 @@ async def _persist_pending_import( continue props = build_zwave_properties(ieee, n.get("vendor"), n.get("model")) - if n.get("type") == "zwave_coordinator": - # The coordinator may sit on several canvases (one Node per design). - # Refresh props on every matching node, not just one. - existing_nodes = ( - await db.execute( - select(Node).where(Node.ieee_address == ieee).order_by(Node.id) - ) - ).scalars().all() - if existing_nodes: - for existing_node in existing_nodes: - existing_node.properties = merge_zwave_properties( - existing_node.properties, props - ) - first = existing_nodes[0] - coordinator_out = ZwaveCoordinatorOut( - id=first.id, - label=first.label, - ieee_address=ieee, - ) - coordinator_existed = True - continue - label = n.get("friendly_name") or ieee - node = Node( - label=label, - type=n.get("type") or "zwave_coordinator", - status="online", - check_method="none", - ieee_address=ieee, - services=[], - properties=props, - design_id=default_design_id, - ) - db.add(node) - await db.flush() - coordinator_out = ZwaveCoordinatorOut( - id=node.id, label=label, ieee_address=ieee - ) - continue + # The coordinator is no longer auto-placed on the canvas — it flows to + # the pending inventory like every other device, so the user approves it + # explicitly. Only the shared paths below run for it. # Already approved as a canvas Node → refresh props on every canvas it # sits on, skip pending row. diff --git a/backend/tests/test_zigbee_router.py b/backend/tests/test_zigbee_router.py index 7467f15..a6f2160 100644 --- a/backend/tests/test_zigbee_router.py +++ b/backend/tests/test_zigbee_router.py @@ -338,20 +338,36 @@ async def test_import_pending_endpoint_creates_zigbee_scan_run( @pytest.mark.asyncio -async def test_persist_pending_import_creates_coordinator_and_pending( +async def test_persist_pending_import_coordinator_goes_to_pending( db_session, ) -> None: + """Coordinator is no longer auto-placed — it lands in pending like the rest.""" + from sqlalchemy import select + from app.api.routes.zigbee import _persist_pending_import + from app.db.models import Node, PendingDevice result = await _persist_pending_import(db_session, _PENDING_NODES, _PENDING_EDGES) assert result.device_count == 3 - assert result.pending_created == 2 + assert result.pending_created == 3 # coordinator included now assert result.pending_updated == 0 - assert result.coordinator is not None - assert result.coordinator.ieee_address == "0xCOORD" + assert result.coordinator is None # not auto-placed assert result.coordinator_already_existed is False assert result.links_recorded == 2 + # No canvas Node auto-created for the coordinator. + nodes = (await db_session.execute(select(Node))).scalars().all() + assert nodes == [] + # Coordinator sits in the pending inventory. + coord = ( + await db_session.execute( + select(PendingDevice).where(PendingDevice.ieee_address == "0xCOORD") + ) + ).scalar_one() + assert coord.status == "pending" + assert coord.suggested_type == "zigbee_coordinator" + assert coord.device_subtype == "Coordinator" + @pytest.mark.asyncio async def test_persist_pending_import_idempotent_updates_existing( @@ -366,8 +382,8 @@ async def test_persist_pending_import_idempotent_updates_existing( result = await _persist_pending_import(db_session, bumped, _PENDING_EDGES) assert result.pending_created == 0 - assert result.pending_updated == 2 - assert result.coordinator_already_existed is True + assert result.pending_updated == 3 # coordinator upserts too + assert result.coordinator_already_existed is False assert result.links_recorded == 2 @@ -389,12 +405,12 @@ async def test_persist_pending_import_replaces_links(db_session) -> None: @pytest.mark.asyncio -async def test_persist_pending_import_sets_coordinator_properties(db_session) -> None: - """Coordinator Node is created with IEEE/Vendor/Model/LQI in properties.""" +async def test_persist_pending_import_sets_coordinator_pending_fields(db_session) -> None: + """Coordinator lands in pending carrying its vendor/model metadata.""" from sqlalchemy import select from app.api.routes.zigbee import _persist_pending_import - from app.db.models import Node + from app.db.models import PendingDevice nodes_with_meta = [dict(n) for n in _PENDING_NODES] nodes_with_meta[0]["vendor"] = "TI" @@ -403,12 +419,13 @@ async def test_persist_pending_import_sets_coordinator_properties(db_session) -> await _persist_pending_import(db_session, nodes_with_meta, _PENDING_EDGES) coord = ( - await db_session.execute(select(Node).where(Node.ieee_address == "0xCOORD")) + await db_session.execute( + select(PendingDevice).where(PendingDevice.ieee_address == "0xCOORD") + ) ).scalar_one() - keys = {p["key"]: p["value"] for p in coord.properties} - assert keys == {"IEEE": "0xCOORD", "Vendor": "TI", "Model": "CC2652"} - # New zigbee props default to hidden — user opts in from the right panel. - assert all(p["visible"] is False for p in coord.properties) + assert coord.vendor == "TI" + assert coord.model == "CC2652" + assert coord.suggested_type == "zigbee_coordinator" @pytest.mark.asyncio @@ -501,8 +518,8 @@ async def test_persist_pending_import_revives_orphaned_approved_device( ) ).scalar_one() assert revived.status == "pending" - # End device 0xE1 is brand new → created as pending; router was updated. - assert result.pending_created == 1 + # Coordinator + end device 0xE1 are brand new → created; router was revived. + assert result.pending_created == 2 assert result.pending_updated == 1 # It is now visible to the Pending list (status filter == "pending"). @@ -511,7 +528,7 @@ async def test_persist_pending_import_revives_orphaned_approved_device( select(PendingDevice).where(PendingDevice.status == "pending") ) ).scalars().all() - assert {p.ieee_address for p in listed} == {"0xR1", "0xE1"} + assert {p.ieee_address for p in listed} == {"0xCOORD", "0xR1", "0xE1"} @pytest.mark.asyncio @@ -591,15 +608,22 @@ async def test_persist_pending_import_preserves_user_visibility(db_session) -> N @pytest.mark.asyncio -async def test_persist_pending_import_refreshes_existing_coordinator_properties( +async def test_persist_pending_import_refreshes_approved_coordinator_node( db_session, ) -> None: + """An already-approved coordinator Node still gets its props refreshed on + re-import (via the shared approved-node path), and stays out of pending.""" from sqlalchemy import select from app.api.routes.zigbee import _persist_pending_import - from app.db.models import Node + from app.db.models import Node, PendingDevice - await _persist_pending_import(db_session, _PENDING_NODES, _PENDING_EDGES) + # User previously approved the coordinator onto the canvas. + db_session.add(Node( + label="Coordinator", type="zigbee_coordinator", status="online", + check_method="none", ieee_address="0xCOORD", services=[], properties=[], + )) + await db_session.commit() bumped = [dict(n) for n in _PENDING_NODES] bumped[0]["vendor"] = "TI" @@ -612,10 +636,15 @@ async def test_persist_pending_import_refreshes_existing_coordinator_properties( keys = {p["key"]: p["value"] for p in coord.properties} assert keys["Vendor"] == "TI" assert keys["Model"] == "CC2652" - # Newly added keys on re-import default to hidden. by_key = {p["key"]: p for p in coord.properties} assert by_key["Vendor"]["visible"] is False - assert by_key["Model"]["visible"] is False + # Approved coordinator must NOT reappear in pending. + pendings = ( + await db_session.execute( + select(PendingDevice).where(PendingDevice.ieee_address == "0xCOORD") + ) + ).scalars().all() + assert pendings == [] @pytest.mark.asyncio diff --git a/backend/tests/test_zwave_router.py b/backend/tests/test_zwave_router.py index 3aa7130..5b275c9 100644 --- a/backend/tests/test_zwave_router.py +++ b/backend/tests/test_zwave_router.py @@ -297,18 +297,31 @@ async def test_import_pending_requires_auth(client: AsyncClient) -> None: @pytest.mark.asyncio -async def test_persist_creates_coordinator_and_pending(db_session) -> None: +async def test_persist_coordinator_goes_to_pending(db_session) -> None: + """Coordinator is no longer auto-placed — it lands in pending like the rest.""" + from sqlalchemy import select + from app.api.routes.zwave import _persist_pending_import + from app.db.models import Node, PendingDevice result = await _persist_pending_import(db_session, _PENDING_NODES, _PENDING_EDGES) assert result.device_count == 3 - assert result.pending_created == 2 + assert result.pending_created == 3 # coordinator included now assert result.pending_updated == 0 - assert result.coordinator is not None - assert result.coordinator.ieee_address == "zwave-0xh-1" + assert result.coordinator is None # not auto-placed assert result.coordinator_already_existed is False assert result.links_recorded == 2 + nodes = (await db_session.execute(select(Node))).scalars().all() + assert nodes == [] + coord = ( + await db_session.execute( + select(PendingDevice).where(PendingDevice.ieee_address == "zwave-0xh-1") + ) + ).scalar_one() + assert coord.status == "pending" + assert coord.suggested_type == "zwave_coordinator" + @pytest.mark.asyncio async def test_persist_idempotent_updates_existing(db_session) -> None: @@ -319,8 +332,8 @@ async def test_persist_idempotent_updates_existing(db_session) -> None: bumped[1]["model"] = "ZW111" result = await _persist_pending_import(db_session, bumped, _PENDING_EDGES) assert result.pending_created == 0 - assert result.pending_updated == 2 - assert result.coordinator_already_existed is True + assert result.pending_updated == 3 # coordinator upserts too + assert result.coordinator_already_existed is False @pytest.mark.asyncio @@ -339,22 +352,25 @@ async def test_persist_replaces_links(db_session) -> None: @pytest.mark.asyncio -async def test_persist_sets_coordinator_properties(db_session) -> None: +async def test_persist_sets_coordinator_pending_fields(db_session) -> None: + """Coordinator lands in pending carrying its vendor/model metadata.""" from sqlalchemy import select from app.api.routes.zwave import _persist_pending_import - from app.db.models import Node + from app.db.models import PendingDevice nodes = [dict(n) for n in _PENDING_NODES] nodes[0]["vendor"] = "Aeotec" nodes[0]["model"] = "ZW090" await _persist_pending_import(db_session, nodes, _PENDING_EDGES) coord = ( - await db_session.execute(select(Node).where(Node.ieee_address == "zwave-0xh-1")) + await db_session.execute( + select(PendingDevice).where(PendingDevice.ieee_address == "zwave-0xh-1") + ) ).scalar_one() - keys = {p["key"]: p["value"] for p in coord.properties} - assert keys == {"Z-Wave ID": "zwave-0xh-1", "Vendor": "Aeotec", "Model": "ZW090"} - assert all(p["visible"] is False for p in coord.properties) + assert coord.vendor == "Aeotec" + assert coord.model == "ZW090" + assert coord.suggested_type == "zwave_coordinator" @pytest.mark.asyncio @@ -416,7 +432,8 @@ async def test_persist_revives_orphaned_approved_device(db_session) -> None: ) ).scalar_one() assert revived.status == "pending" - assert result.pending_created == 1 + # Coordinator + end device are brand new → created; router was revived. + assert result.pending_created == 2 assert result.pending_updated == 1 From 1de6e91ba3951694d883a19517c7537af47953aa Mon Sep 17 00:00:00 2001 From: Pouzor Date: Fri, 3 Jul 2026 11:07:48 +0200 Subject: [PATCH 3/4] feat: colour device-inventory role badge by node-type accent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The role/type badge on each inventory tile was always flat grey (bg-[#21262d] text-muted-foreground), which read as disabled and gave no visual cue about the device kind. Colour it with the same per-type accent the node uses on the canvas — resolved through the active theme / style section via resolveNodeColors — so a zigbee_coordinator, zwave_router, etc. gets its style-section colour (translucent background + solid text). Applies to every source (IP, zigbee, zwave) since all node types carry an accent in the theme. Test: role badge renders with the node-type accent colour, not the grey muted class. ha-relevant: yes --- .../src/components/modals/PendingDevicesModal.tsx | 14 ++++++++++++-- .../modals/__tests__/PendingDevicesModal.test.tsx | 12 ++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/frontend/src/components/modals/PendingDevicesModal.tsx b/frontend/src/components/modals/PendingDevicesModal.tsx index 926c947..6d3af0d 100644 --- a/frontend/src/components/modals/PendingDevicesModal.tsx +++ b/frontend/src/components/modals/PendingDevicesModal.tsx @@ -7,6 +7,8 @@ import { Dialog, DialogClose, DialogContent, DialogHeader, DialogTitle } from '@ import { scanApi } from '@/api/client' import { useCanvasStore } from '@/stores/canvasStore' import { useDesignStore } from '@/stores/designStore' +import { useThemeStore } from '@/stores/themeStore' +import { resolveNodeColors } from '@/utils/nodeColors' import { toast } from 'sonner' import { PendingDeviceModal, type PendingDevice } from '@/components/modals/PendingDeviceModal' import type { NodeType, ServiceInfo } from '@/types' @@ -657,7 +659,12 @@ interface DeviceCardProps { function DeviceCard({ device, selected, selectMode, highlighted, onClick, cardRef }: DeviceCardProps) { const source = inferSource(device) - const Icon = TYPE_ICONS[device.suggested_type ?? 'generic'] ?? Circle + const roleType = (device.suggested_type ?? 'generic') as NodeType + const Icon = TYPE_ICONS[roleType] ?? Circle + const activeTheme = useThemeStore((s) => s.activeTheme) + // Colour the role badge with the same accent the node uses on the canvas + // (from the active theme / style section), instead of a flat grey. + const roleColor = resolveNodeColors({ type: roleType, custom_colors: undefined }, activeTheme).border const label = deviceLabel(device) const sourceColor = source === 'zigbee' ? '#00d4ff' : source === 'zwave' ? '#ff6e00' : '#a855f7' const sourceLabel = @@ -734,7 +741,10 @@ function DeviceCard({ device, selected, selectMode, highlighted, onClick, cardRe {sourceLabel} {device.suggested_type && ( - + {device.suggested_type} )} diff --git a/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx b/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx index 25b27be..ae322fc 100644 --- a/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx +++ b/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx @@ -153,6 +153,18 @@ describe('PendingDevicesModal', () => { expect(screen.getByText('Z-WAVE')).toBeInTheDocument() }) + it('colours the role badge with the node-type accent, not flat grey', async () => { + mockPending.mockResolvedValue({ data: [DEVICE_ZWAVE] }) + render() + const card = await waitFor(() => screen.getByTestId('pending-card-dev-c')) + // zwave_router accent from the default theme = #e3b341 (amber), applied to + // both the text colour and a translucent background. + const badge = within(card).getByText('zwave_router') + // #e3b341 → rgb(227, 179, 65) once jsdom normalises the inline colour. + expect(badge).toHaveStyle({ color: 'rgb(227, 179, 65)' }) + expect(badge.className).not.toContain('text-muted-foreground') + }) + it('filters by source (zwave only)', async () => { mockPending.mockResolvedValue({ data: [DEVICE_IP, DEVICE_ZWAVE] }) render() From 6f82c15c69e940623007211af27f33644a936206 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Fri, 3 Jul 2026 12:08:53 +0200 Subject: [PATCH 4/4] fix: backfill inventory row for on-canvas mesh devices missing one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A device already on a canvas (Node exists) but with no pending_devices row never showed in the discovery inventory — the inventory lists pending_devices, not nodes. This stranded legacy auto-placed coordinators: on a canvas yet invisible in the inventory list. Both mesh imports now, in the already-approved-node branch, ensure an inventory row exists: create one as status="approved" when missing, or refresh its metadata (preserving status) when present. New/unplaced devices still land as status="pending"; hidden rows stay hidden. Tests updated: approved-node path now backfills an approved inventory row (zigbee + zwave), plus a regression that a hidden inventory row is not revived. ha-relevant: yes --- backend/app/api/routes/zigbee.py | 40 +++++++++++++++-- backend/app/api/routes/zwave.py | 36 ++++++++++++++- backend/tests/test_zigbee_router.py | 68 ++++++++++++++++++++++------- backend/tests/test_zwave_router.py | 11 +++-- 4 files changed, 131 insertions(+), 24 deletions(-) diff --git a/backend/app/api/routes/zigbee.py b/backend/app/api/routes/zigbee.py index d59bfaf..d5d47cb 100644 --- a/backend/app/api/routes/zigbee.py +++ b/backend/app/api/routes/zigbee.py @@ -162,9 +162,11 @@ async def _persist_pending_import( # the pending inventory like every other device, so the user approves it # explicitly. Only the shared paths below run for it. - # If the device has already been approved as a canvas Node, refresh - # its properties on every canvas it sits on and skip creating a pending - # row (keeps approved devices out of pending/hidden modals on re-import). + # If the device has already been approved as a canvas Node, refresh its + # properties on every canvas it sits on. Still ensure the discovery + # inventory carries a row for it (status="approved") so it shows in the + # inventory list with an "In N canvas" badge — legacy auto-placed + # coordinators never got a pending row, which is why they went missing. existing_nodes = ( await db.execute( select(Node).where(Node.ieee_address == ieee).order_by(Node.id) @@ -175,6 +177,38 @@ async def _persist_pending_import( existing_node.properties = merge_zigbee_properties( existing_node.properties, props ) + inv = ( + await db.execute( + select(PendingDevice).where(PendingDevice.ieee_address == ieee) + ) + ).scalar_one_or_none() + if inv is None: + db.add( + PendingDevice( + ieee_address=ieee, + friendly_name=n.get("friendly_name"), + hostname=n.get("friendly_name"), + suggested_type=n.get("type"), + device_subtype=n.get("device_type"), + model=n.get("model"), + vendor=n.get("vendor"), + lqi=n.get("lqi"), + status="approved", + discovery_source="zigbee", + ) + ) + pending_created += 1 + else: + # Refresh metadata but never change the row's status (an approved + # device stays approved; a hidden one stays hidden). + inv.friendly_name = n.get("friendly_name") or inv.friendly_name + inv.suggested_type = n.get("type") or inv.suggested_type + inv.device_subtype = n.get("device_type") or inv.device_subtype + inv.model = n.get("model") or inv.model + inv.vendor = n.get("vendor") or inv.vendor + if n.get("lqi") is not None: + inv.lqi = n.get("lqi") + pending_updated += 1 continue result = await db.execute( diff --git a/backend/app/api/routes/zwave.py b/backend/app/api/routes/zwave.py index 9f20241..add3e9f 100644 --- a/backend/app/api/routes/zwave.py +++ b/backend/app/api/routes/zwave.py @@ -157,7 +157,10 @@ async def _persist_pending_import( # explicitly. Only the shared paths below run for it. # Already approved as a canvas Node → refresh props on every canvas it - # sits on, skip pending row. + # sits on. Still ensure the discovery inventory carries a row for it + # (status="approved") so it shows in the inventory list with an + # "In N canvas" badge — legacy auto-placed coordinators never got a + # pending row, which is why they went missing. existing_nodes = ( await db.execute( select(Node).where(Node.ieee_address == ieee).order_by(Node.id) @@ -168,6 +171,37 @@ async def _persist_pending_import( existing_node.properties = merge_zwave_properties( existing_node.properties, props ) + inv = ( + await db.execute( + select(PendingDevice).where(PendingDevice.ieee_address == ieee) + ) + ).scalar_one_or_none() + if inv is None: + db.add( + PendingDevice( + ieee_address=ieee, + friendly_name=n.get("friendly_name"), + hostname=n.get("friendly_name"), + suggested_type=n.get("type"), + device_subtype=n.get("device_type"), + model=n.get("model"), + vendor=n.get("vendor"), + lqi=n.get("lqi"), + status="approved", + discovery_source="zwave", + ) + ) + pending_created += 1 + else: + # Refresh metadata but never change the row's status. + inv.friendly_name = n.get("friendly_name") or inv.friendly_name + inv.suggested_type = n.get("type") or inv.suggested_type + inv.device_subtype = n.get("device_type") or inv.device_subtype + inv.model = n.get("model") or inv.model + inv.vendor = n.get("vendor") or inv.vendor + if n.get("lqi") is not None: + inv.lqi = n.get("lqi") + pending_updated += 1 continue result = await db.execute( diff --git a/backend/tests/test_zigbee_router.py b/backend/tests/test_zigbee_router.py index a6f2160..a2e9cda 100644 --- a/backend/tests/test_zigbee_router.py +++ b/backend/tests/test_zigbee_router.py @@ -429,19 +429,20 @@ async def test_persist_pending_import_sets_coordinator_pending_fields(db_session @pytest.mark.asyncio -async def test_persist_pending_import_skips_pending_for_approved_node( +async def test_persist_pending_import_backfills_inventory_for_approved_node( db_session, ) -> None: - """A device already approved as a canvas Node must not reappear in pending. - - Its properties must still be refreshed with the latest Vendor/Model/LQI. + """A device on canvas but missing its inventory row gets one backfilled + (status="approved"), so it shows in the inventory list. Node props still + refresh with the latest Vendor/Model/LQI. """ from sqlalchemy import select from app.api.routes.zigbee import _persist_pending_import from app.db.models import Node, PendingDevice - # Simulate: router was approved earlier → exists as a canvas Node. + # Simulate: router was approved earlier → exists as a canvas Node, but with + # no matching pending_devices row (e.g. a legacy auto-placed device). approved = Node( label="router_1", type="zigbee_router", @@ -458,13 +459,15 @@ async def test_persist_pending_import_skips_pending_for_approved_node( bumped[1]["lqi"] = 250 # new LQI from re-import await _persist_pending_import(db_session, bumped, _PENDING_EDGES) - # No PendingDevice row was created for the approved router. - pendings = ( + # An inventory row is backfilled as "approved" (it is on a canvas). + inv = ( await db_session.execute( select(PendingDevice).where(PendingDevice.ieee_address == "0xR1") ) - ).scalars().all() - assert pendings == [] + ).scalar_one() + assert inv.status == "approved" + assert inv.suggested_type == "zigbee_router" + assert inv.device_subtype == "Router" # Node properties got refreshed. refreshed = ( @@ -476,6 +479,37 @@ async def test_persist_pending_import_skips_pending_for_approved_node( assert all(p["visible"] is False for p in refreshed.properties) +@pytest.mark.asyncio +async def test_persist_pending_import_preserves_hidden_inventory_for_approved_node( + db_session, +) -> None: + """If the on-canvas device already has a hidden inventory row, re-import + refreshes its metadata but must NOT flip it back to approved/visible.""" + from sqlalchemy import select + + from app.api.routes.zigbee import _persist_pending_import + from app.db.models import Node, PendingDevice + + db_session.add(Node( + label="router_1", type="zigbee_router", status="online", + check_method="none", ieee_address="0xR1", services=[], properties=[], + )) + db_session.add(PendingDevice( + ieee_address="0xR1", friendly_name="router_1", suggested_type="zigbee_router", + device_subtype="Router", status="hidden", discovery_source="zigbee", + )) + await db_session.commit() + + await _persist_pending_import(db_session, _PENDING_NODES, _PENDING_EDGES) + + inv = ( + await db_session.execute( + select(PendingDevice).where(PendingDevice.ieee_address == "0xR1") + ) + ).scalar_one() + assert inv.status == "hidden" # stays hidden + + @pytest.mark.asyncio async def test_persist_pending_import_revives_orphaned_approved_device( db_session, @@ -611,14 +645,15 @@ async def test_persist_pending_import_preserves_user_visibility(db_session) -> N async def test_persist_pending_import_refreshes_approved_coordinator_node( db_session, ) -> None: - """An already-approved coordinator Node still gets its props refreshed on - re-import (via the shared approved-node path), and stays out of pending.""" + """An already-approved coordinator Node gets its props refreshed on + re-import, and a backfilled inventory row (approved) so it shows in the + inventory list — this is the legacy auto-placed coordinator scenario.""" from sqlalchemy import select from app.api.routes.zigbee import _persist_pending_import from app.db.models import Node, PendingDevice - # User previously approved the coordinator onto the canvas. + # Legacy: coordinator auto-placed as a canvas Node, no pending_devices row. db_session.add(Node( label="Coordinator", type="zigbee_coordinator", status="online", check_method="none", ieee_address="0xCOORD", services=[], properties=[], @@ -638,13 +673,14 @@ async def test_persist_pending_import_refreshes_approved_coordinator_node( assert keys["Model"] == "CC2652" by_key = {p["key"]: p for p in coord.properties} assert by_key["Vendor"]["visible"] is False - # Approved coordinator must NOT reappear in pending. - pendings = ( + # Inventory row backfilled as approved → now visible in the inventory list. + inv = ( await db_session.execute( select(PendingDevice).where(PendingDevice.ieee_address == "0xCOORD") ) - ).scalars().all() - assert pendings == [] + ).scalar_one() + assert inv.status == "approved" + assert inv.suggested_type == "zigbee_coordinator" @pytest.mark.asyncio diff --git a/backend/tests/test_zwave_router.py b/backend/tests/test_zwave_router.py index 5b275c9..3ccc442 100644 --- a/backend/tests/test_zwave_router.py +++ b/backend/tests/test_zwave_router.py @@ -374,7 +374,9 @@ async def test_persist_sets_coordinator_pending_fields(db_session) -> None: @pytest.mark.asyncio -async def test_persist_skips_pending_for_approved_node(db_session) -> None: +async def test_persist_backfills_inventory_for_approved_node(db_session) -> None: + """On-canvas device missing its inventory row gets one backfilled + (status="approved"); Node props still refresh.""" from sqlalchemy import select from app.api.routes.zwave import _persist_pending_import @@ -394,12 +396,13 @@ async def test_persist_skips_pending_for_approved_node(db_session) -> None: await _persist_pending_import(db_session, _PENDING_NODES, _PENDING_EDGES) - pendings = ( + inv = ( await db_session.execute( select(PendingDevice).where(PendingDevice.ieee_address == "zwave-0xh-2") ) - ).scalars().all() - assert pendings == [] + ).scalar_one() + assert inv.status == "approved" + assert inv.suggested_type == "zwave_router" refreshed = ( await db_session.execute(select(Node).where(Node.ieee_address == "zwave-0xh-2")) ).scalar_one()