diff --git a/backend/app/api/routes/scan.py b/backend/app/api/routes/scan.py index 33c03df..9b1d8d2 100644 --- a/backend/app/api/routes/scan.py +++ b/backend/app/api/routes/scan.py @@ -20,6 +20,34 @@ from app.services.zigbee_service import build_zigbee_properties _ZIGBEE_TYPES = {"zigbee_coordinator", "zigbee_router", "zigbee_enddevice"} +def build_mac_property(mac: str | None) -> list[dict[str, Any]]: + """Build a NodeProperty list carrying a device MAC address. + + Shape matches the frontend ``NodeProperty`` type + (``{key, value, icon, visible}``). Hidden by default — the user opts in to + showing it on the canvas card from the right panel. Returns an empty list + when no MAC is known. + """ + if not mac: + return [] + return [{"key": "MAC", "value": mac, "icon": None, "visible": False}] + + +def merge_mac_property( + props: list[dict[str, Any]] | None, mac: str | None +) -> list[dict[str, Any]]: + """Append a MAC NodeProperty to ``props`` unless one is already present. + + Preserves any user-supplied properties (and an existing MAC row's + visibility) untouched. Used on approve so the scanned MAC is not lost. + """ + out = [dict(p) for p in (props or [])] + if not mac or any(p.get("key") == "MAC" for p in out): + return out + out.append({"key": "MAC", "value": mac, "icon": None, "visible": False}) + return out + + class BulkActionRequest(BaseModel): device_ids: list[str] @@ -134,13 +162,14 @@ async def bulk_approve_devices( label=device.hostname or device.friendly_name or device.ip or "device", type=node_type, ip=device.ip, + mac=device.mac, hostname=device.hostname, status="online" if is_zigbee else "unknown", services=device.services or [], ieee_address=device.ieee_address, properties=build_zigbee_properties( device.ieee_address, device.vendor, device.model, device.lqi - ) if is_zigbee else [], + ) if is_zigbee else build_mac_property(device.mac), # Default to ping so the status checker actually polls the new node. # Without this the scheduler skips it (check_method NULL → no check). check_method="none" if is_zigbee else ("ping" if device.ip else None), @@ -234,17 +263,21 @@ async def approve_device( raise HTTPException(status_code=409, detail="Device already processed") device.status = "approved" _is_zigbee = node_data.type in _ZIGBEE_TYPES + # 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 node = Node( label=node_data.label, type=node_data.type, ip=node_data.ip, + mac=_mac, hostname=node_data.hostname, status="online" if _is_zigbee else node_data.status, services=node_data.services or [], ieee_address=device.ieee_address, properties=build_zigbee_properties( device.ieee_address, device.vendor, device.model, device.lqi - ) if _is_zigbee else (node_data.properties or []), + ) if _is_zigbee else merge_mac_property(node_data.properties, _mac), check_method="none" if _is_zigbee else (node_data.check_method or ("ping" if node_data.ip else None)), check_target=None if _is_zigbee else node_data.check_target, ) diff --git a/backend/tests/test_scan.py b/backend/tests/test_scan.py index 7199ea9..6feb0b9 100644 --- a/backend/tests/test_scan.py +++ b/backend/tests/test_scan.py @@ -698,6 +698,144 @@ async def test_bulk_approve_zigbee_populates_properties( assert node.check_method == "none" +# --- MAC address propagation on approve (issue #168) --- + +def test_build_mac_property_returns_hidden_row(): + from app.api.routes.scan import build_mac_property + + assert build_mac_property("aa:bb:cc:dd:ee:ff") == [ + {"key": "MAC", "value": "aa:bb:cc:dd:ee:ff", "icon": None, "visible": False} + ] + + +def test_build_mac_property_empty_when_no_mac(): + from app.api.routes.scan import build_mac_property + + assert build_mac_property(None) == [] + assert build_mac_property("") == [] + + +def test_merge_mac_property_appends_when_absent(): + from app.api.routes.scan import merge_mac_property + + existing = [{"key": "Custom", "value": "x", "icon": None, "visible": True}] + merged = merge_mac_property(existing, "aa:bb:cc:dd:ee:ff") + assert {"key": "MAC", "value": "aa:bb:cc:dd:ee:ff", "icon": None, "visible": False} in merged + # Existing prop preserved untouched. + assert existing[0] in merged + + +def test_merge_mac_property_idempotent_and_preserves_visibility(): + from app.api.routes.scan import merge_mac_property + + existing = [{"key": "MAC", "value": "aa:bb:cc:dd:ee:ff", "icon": None, "visible": True}] + merged = merge_mac_property(existing, "aa:bb:cc:dd:ee:ff") + # No duplicate MAC row; user's visible=True choice kept. + macs = [p for p in merged if p["key"] == "MAC"] + assert len(macs) == 1 + assert macs[0]["visible"] is True + + +def test_merge_mac_property_noop_without_mac(): + from app.api.routes.scan import merge_mac_property + + existing = [{"key": "Custom", "value": "x", "icon": None, "visible": True}] + assert merge_mac_property(existing, None) == existing + + +@pytest.mark.asyncio +async def test_approve_device_copies_mac_to_node_and_properties( + client: AsyncClient, headers, pending_device, db_session +): + """Approving a scanned device must carry its MAC onto the node + properties.""" + from sqlalchemy import select + + from app.db.models import Node as NodeModel + # Payload intentionally omits mac — it must come from the pending device. + res = await client.post( + f"/api/v1/scan/pending/{pending_device.id}/approve", + json={"label": "My Server", "type": "server", "ip": "192.168.1.100", "status": "unknown", "services": []}, + headers=headers, + ) + assert res.status_code == 200 + node = ( + await db_session.execute(select(NodeModel).where(NodeModel.ip == "192.168.1.100")) + ).scalar_one() + assert node.mac == "aa:bb:cc:dd:ee:ff" + mac_props = [p for p in node.properties if p["key"] == "MAC"] + assert mac_props == [ + {"key": "MAC", "value": "aa:bb:cc:dd:ee:ff", "icon": None, "visible": False} + ] + + +@pytest.mark.asyncio +async def test_approve_device_does_not_duplicate_mac_property( + client: AsyncClient, headers, pending_device, db_session +): + """If the approve payload already carries a MAC prop, don't add a second one.""" + from sqlalchemy import select + + from app.db.models import Node as NodeModel + res = await client.post( + f"/api/v1/scan/pending/{pending_device.id}/approve", + json={ + "label": "My Server", + "type": "server", + "ip": "192.168.1.100", + "status": "unknown", + "services": [], + "properties": [ + {"key": "MAC", "value": "aa:bb:cc:dd:ee:ff", "icon": None, "visible": True} + ], + }, + headers=headers, + ) + assert res.status_code == 200 + node = ( + await db_session.execute(select(NodeModel).where(NodeModel.ip == "192.168.1.100")) + ).scalar_one() + mac_props = [p for p in node.properties if p["key"] == "MAC"] + assert len(mac_props) == 1 + # User's visibility choice is preserved. + assert mac_props[0]["visible"] is True + + +@pytest.mark.asyncio +async def test_bulk_approve_copies_mac_to_node_and_properties( + client: AsyncClient, headers, db_session +): + """Bulk approve must also propagate the scanned MAC to node + properties.""" + from sqlalchemy import select + + from app.db.models import Node as NodeModel + device = PendingDevice( + id=str(uuid.uuid4()), + ip="192.168.1.55", + mac="11:22:33:44:55:66", + hostname="host-mac", + services=[], + suggested_type="generic", + status="pending", + ) + db_session.add(device) + await db_session.commit() + + res = await client.post( + "/api/v1/scan/pending/bulk-approve", + json={"device_ids": [device.id]}, + headers=headers, + ) + assert res.status_code == 200 + node = ( + await db_session.execute(select(NodeModel).where(NodeModel.ip == "192.168.1.55")) + ).scalar_one() + assert node.mac == "11:22:33:44:55:66" + mac_props = [p for p in node.properties if p["key"] == "MAC"] + assert mac_props == [ + {"key": "MAC", "value": "11:22:33:44:55:66", "icon": None, "visible": False} + ] + + @pytest.mark.asyncio async def test_bulk_approve_sets_default_check_method(client: AsyncClient, headers, two_pending_devices, db_session): """Approved devices with an IP must default to ping; otherwise scheduler skips them.""" diff --git a/frontend/src/components/modals/PendingDevicesModal.tsx b/frontend/src/components/modals/PendingDevicesModal.tsx index 973b492..1f770ed 100644 --- a/frontend/src/components/modals/PendingDevicesModal.tsx +++ b/frontend/src/components/modals/PendingDevicesModal.tsx @@ -10,6 +10,7 @@ import { toast } from 'sonner' import { PendingDeviceModal, type PendingDevice } from '@/components/modals/PendingDeviceModal' import type { NodeType, ServiceInfo } from '@/types' import { buildZigbeeProperties, isZigbeeType } from '@/utils/zigbeeProperties' +import { buildMacProperty } from '@/utils/macProperty' interface PendingDevicesModalProps { open: boolean @@ -255,11 +256,12 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus const fallbackLabel = deviceLabel(device) const type = (device.suggested_type ?? 'generic') as NodeType const zigbee = isZigbeeType(type) - const properties = zigbee ? buildZigbeeProperties(device) : [] + const properties = zigbee ? buildZigbeeProperties(device) : buildMacProperty(device.mac) const nodeData = { label: fallbackLabel, type, ip: device.ip ?? undefined, + mac: device.mac ?? undefined, hostname: device.hostname ?? undefined, status: zigbee ? 'online' : 'unknown', services: (device.services ?? []) as ServiceInfo[], @@ -325,10 +327,11 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus label: deviceLabel(d), type, ip: d.ip ?? undefined, + mac: d.mac ?? undefined, hostname: d.hostname ?? undefined, status: zigbee ? ('online' as const) : ('unknown' as const), services: (d.services ?? []) as ServiceInfo[], - properties: zigbee ? buildZigbeeProperties(d) : [], + properties: zigbee ? buildZigbeeProperties(d) : buildMacProperty(d.mac), }, }) }) diff --git a/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx b/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx index 6164950..4cbcf97 100644 --- a/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx +++ b/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx @@ -13,6 +13,7 @@ const mockApprove = vi.fn() const mockHide = vi.fn() const mockPending = vi.fn() const mockHidden = vi.fn() +const mockAddNode = vi.fn() vi.mock('@/api/client', () => ({ scanApi: { @@ -69,7 +70,7 @@ const DEVICE_ZIGBEE = { beforeEach(() => { vi.clearAllMocks() vi.mocked(useCanvasStore).mockReturnValue({ - addNode: vi.fn(), + addNode: mockAddNode, scanEventTs: 0, } as unknown as ReturnType) // setState is used by injectAutoEdges @@ -174,6 +175,34 @@ describe('PendingDevicesModal', () => { await waitFor(() => expect(mockBulkApprove).toHaveBeenCalledWith(['dev-a', 'dev-b'])) }) + it('bulk approve carries the scanned MAC onto the canvas node (#168)', async () => { + render() + await waitFor(() => expect(screen.getByTestId('pending-card-dev-a')).toBeInTheDocument()) + fireEvent.click(screen.getByRole('button', { name: 'Select mode' })) + fireEvent.click(screen.getByTestId('pending-card-dev-a')) + fireEvent.click(screen.getByTestId('pending-card-dev-b')) + fireEvent.click(screen.getByRole('button', { name: /Approve \(2\)/ })) + await waitFor(() => expect(mockAddNode).toHaveBeenCalledTimes(2)) + + // dev-a is an IP device with a MAC → node carries mac + a MAC property row. + const ipNode = mockAddNode.mock.calls + .map((c) => c[0]) + .find((n) => n.id === 'n1') + expect(ipNode.data.mac).toBe('aa:bb:cc:dd:ee:01') + expect(ipNode.data.properties).toContainEqual({ + key: 'MAC', + value: 'aa:bb:cc:dd:ee:01', + icon: null, + visible: false, + }) + + // dev-b is zigbee with no MAC → no MAC property row. + const zbNode = mockAddNode.mock.calls + .map((c) => c[0]) + .find((n) => n.id === 'n2') + expect(zbNode.data.properties.some((p: { key: string }) => p.key === 'MAC')).toBe(false) + }) + it('bulk hide calls API with selected ids', async () => { render() await waitFor(() => expect(screen.getByTestId('pending-card-dev-a')).toBeInTheDocument()) diff --git a/frontend/src/utils/__tests__/macProperty.test.ts b/frontend/src/utils/__tests__/macProperty.test.ts new file mode 100644 index 0000000..166f3fb --- /dev/null +++ b/frontend/src/utils/__tests__/macProperty.test.ts @@ -0,0 +1,16 @@ +import { describe, it, expect } from 'vitest' +import { buildMacProperty } from '../macProperty' + +describe('buildMacProperty', () => { + it('returns a hidden MAC property row for a MAC', () => { + expect(buildMacProperty('aa:bb:cc:dd:ee:ff')).toEqual([ + { key: 'MAC', value: 'aa:bb:cc:dd:ee:ff', icon: null, visible: false }, + ]) + }) + + it('returns an empty array when MAC is null/undefined/empty', () => { + expect(buildMacProperty(null)).toEqual([]) + expect(buildMacProperty(undefined)).toEqual([]) + expect(buildMacProperty('')).toEqual([]) + }) +}) diff --git a/frontend/src/utils/macProperty.ts b/frontend/src/utils/macProperty.ts new file mode 100644 index 0000000..969da81 --- /dev/null +++ b/frontend/src/utils/macProperty.ts @@ -0,0 +1,9 @@ +import type { NodeProperty } from '@/types' + +/** Build the MAC address property row shown in the right panel. + * Hidden by default — the user opts in to showing it on the canvas card. + * Matches backend `build_mac_property`. Returns an empty array when no MAC. */ +export function buildMacProperty(mac?: string | null): NodeProperty[] { + if (!mac) return [] + return [{ key: 'MAC', value: mac, icon: null, visible: false }] +}