fix: approve devices onto the active design + Z-Wave node fields
Approve (single + bulk) created the canvas Node under the first design instead of the design the user is viewing, so approved devices were invisible on the active canvas and got wiped on the next save — and a re-approve returned "0 approved" because the rows were already approved. - bulk-approve now accepts design_id; the UI sends the active design. - single approve already honoured design_id; the UI now sends it too. - generalize the wireless branch (status=online, mesh props, no ICMP check) to Z-Wave as well as Zigbee, using build_zwave_properties. ha-relevant: yes
This commit is contained in:
@@ -16,8 +16,28 @@ from app.schemas.nodes import NodeCreate
|
|||||||
from app.schemas.scan import PendingDeviceResponse, ScanRunResponse
|
from app.schemas.scan import PendingDeviceResponse, ScanRunResponse
|
||||||
from app.services.scanner import DeepScanOptions, _valid_port_range, request_cancel, run_scan
|
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
|
||||||
|
from app.services.zwave_service import build_zwave_properties
|
||||||
|
|
||||||
_ZIGBEE_TYPES = {"zigbee_coordinator", "zigbee_router", "zigbee_enddevice"}
|
_ZIGBEE_TYPES = {"zigbee_coordinator", "zigbee_router", "zigbee_enddevice"}
|
||||||
|
_ZWAVE_TYPES = {"zwave_coordinator", "zwave_router", "zwave_enddevice"}
|
||||||
|
|
||||||
|
|
||||||
|
def _is_wireless(node_type: str | None) -> bool:
|
||||||
|
"""Zigbee + Z-Wave mesh devices share online status / no ICMP check."""
|
||||||
|
return node_type in _ZIGBEE_TYPES or node_type in _ZWAVE_TYPES
|
||||||
|
|
||||||
|
|
||||||
|
def _wireless_properties(
|
||||||
|
node_type: str | None,
|
||||||
|
ieee: str | None,
|
||||||
|
vendor: str | None,
|
||||||
|
model: str | None,
|
||||||
|
lqi: int | None,
|
||||||
|
) -> list[dict[str, Any]]:
|
||||||
|
"""Build the right property rows for a mesh device (Z-Wave has no LQI)."""
|
||||||
|
if node_type in _ZWAVE_TYPES:
|
||||||
|
return build_zwave_properties(ieee, vendor, model)
|
||||||
|
return build_zigbee_properties(ieee, vendor, model, lqi)
|
||||||
|
|
||||||
|
|
||||||
def build_mac_property(mac: str | None) -> list[dict[str, Any]]:
|
def build_mac_property(mac: str | None) -> list[dict[str, Any]]:
|
||||||
@@ -50,6 +70,10 @@ def merge_mac_property(
|
|||||||
|
|
||||||
class BulkActionRequest(BaseModel):
|
class BulkActionRequest(BaseModel):
|
||||||
device_ids: list[str]
|
device_ids: list[str]
|
||||||
|
# Target design for approved nodes. Falls back to the first design when
|
||||||
|
# omitted (keeps older clients working), but the UI should send the active
|
||||||
|
# design so approved devices land on the canvas the user is looking at.
|
||||||
|
design_id: str | None = None
|
||||||
|
|
||||||
|
|
||||||
def _check_port_ranges(v: list[str]) -> list[str]:
|
def _check_port_ranges(v: list[str]) -> list[str]:
|
||||||
@@ -248,9 +272,11 @@ async def bulk_approve_devices(
|
|||||||
db: AsyncSession = Depends(get_db),
|
db: AsyncSession = Depends(get_db),
|
||||||
_: str = Depends(get_current_user),
|
_: str = Depends(get_current_user),
|
||||||
) -> dict[str, Any]:
|
) -> dict[str, Any]:
|
||||||
# Determine target design (use first design as fallback)
|
# Target the design the user is on; fall back to the first design.
|
||||||
first_design = (await db.execute(select(Design).order_by(Design.created_at).limit(1))).scalar()
|
default_design_id = payload.design_id
|
||||||
default_design_id = first_design.id if first_design else None
|
if default_design_id is None:
|
||||||
|
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
|
||||||
|
|
||||||
result = await db.execute(
|
result = await db.execute(
|
||||||
select(PendingDevice).where(
|
select(PendingDevice).where(
|
||||||
@@ -263,22 +289,22 @@ async def bulk_approve_devices(
|
|||||||
for device in devices:
|
for device in devices:
|
||||||
device.status = "approved"
|
device.status = "approved"
|
||||||
node_type = device.suggested_type or "generic"
|
node_type = device.suggested_type or "generic"
|
||||||
is_zigbee = node_type in _ZIGBEE_TYPES
|
is_wireless = _is_wireless(node_type)
|
||||||
node = Node(
|
node = Node(
|
||||||
label=device.hostname or device.friendly_name or device.ip or "device",
|
label=device.hostname or device.friendly_name or device.ip or "device",
|
||||||
type=node_type,
|
type=node_type,
|
||||||
ip=device.ip,
|
ip=device.ip,
|
||||||
mac=device.mac,
|
mac=device.mac,
|
||||||
hostname=device.hostname,
|
hostname=device.hostname,
|
||||||
status="online" if is_zigbee else "unknown",
|
status="online" if is_wireless else "unknown",
|
||||||
services=device.services or [],
|
services=device.services or [],
|
||||||
ieee_address=device.ieee_address,
|
ieee_address=device.ieee_address,
|
||||||
properties=build_zigbee_properties(
|
properties=_wireless_properties(
|
||||||
device.ieee_address, device.vendor, device.model, device.lqi
|
node_type, device.ieee_address, device.vendor, device.model, device.lqi
|
||||||
) if is_zigbee else build_mac_property(device.mac),
|
) if is_wireless else build_mac_property(device.mac),
|
||||||
# Default to ping so the status checker actually polls the new node.
|
# Default to ping so the status checker actually polls the new node.
|
||||||
# Without this the scheduler skips it (check_method NULL → no check).
|
# Without this the scheduler skips it (check_method NULL → no check).
|
||||||
check_method="none" if is_zigbee else ("ping" if device.ip else None),
|
check_method="none" if is_wireless else ("ping" if device.ip else None),
|
||||||
design_id=default_design_id,
|
design_id=default_design_id,
|
||||||
)
|
)
|
||||||
db.add(node)
|
db.add(node)
|
||||||
@@ -375,7 +401,7 @@ async def approve_device(
|
|||||||
if device.status != "pending":
|
if device.status != "pending":
|
||||||
raise HTTPException(status_code=409, detail="Device already processed")
|
raise HTTPException(status_code=409, detail="Device already processed")
|
||||||
device.status = "approved"
|
device.status = "approved"
|
||||||
_is_zigbee = node_data.type in _ZIGBEE_TYPES
|
wireless = _is_wireless(node_data.type)
|
||||||
# Prefer the MAC discovered during the scan (stored on the pending device);
|
# Prefer the MAC discovered during the scan (stored on the pending device);
|
||||||
# fall back to whatever the approve payload carried.
|
# fall back to whatever the approve payload carried.
|
||||||
_mac = device.mac or node_data.mac
|
_mac = device.mac or node_data.mac
|
||||||
@@ -385,14 +411,14 @@ async def approve_device(
|
|||||||
ip=node_data.ip,
|
ip=node_data.ip,
|
||||||
mac=_mac,
|
mac=_mac,
|
||||||
hostname=node_data.hostname,
|
hostname=node_data.hostname,
|
||||||
status="online" if _is_zigbee else node_data.status,
|
status="online" if wireless else node_data.status,
|
||||||
services=node_data.services or [],
|
services=node_data.services or [],
|
||||||
ieee_address=device.ieee_address,
|
ieee_address=device.ieee_address,
|
||||||
properties=build_zigbee_properties(
|
properties=_wireless_properties(
|
||||||
device.ieee_address, device.vendor, device.model, device.lqi
|
node_data.type, device.ieee_address, device.vendor, device.model, device.lqi
|
||||||
) if _is_zigbee else merge_mac_property(node_data.properties, _mac),
|
) if wireless 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_method="none" if wireless else (node_data.check_method or ("ping" if node_data.ip else None)),
|
||||||
check_target=None if _is_zigbee else node_data.check_target,
|
check_target=None if wireless else node_data.check_target,
|
||||||
design_id=node_design_id,
|
design_id=node_design_id,
|
||||||
)
|
)
|
||||||
db.add(node)
|
db.add(node)
|
||||||
|
|||||||
@@ -1357,3 +1357,76 @@ async def test_update_scan_config_persists_deep_scan(client: AsyncClient, header
|
|||||||
)
|
)
|
||||||
assert res.status_code == 200
|
assert res.status_code == 200
|
||||||
assert saved == {"http_ranges": ["9000-9100"], "probe": True}
|
assert saved == {"http_ranges": ["9000-9100"], "probe": True}
|
||||||
|
|
||||||
|
|
||||||
|
# --- Z-Wave approve: active design targeting + wireless props (regression) ---
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_bulk_approve_targets_requested_design(client, headers, db_session):
|
||||||
|
"""bulk-approve must place nodes on the design_id sent by the UI, not the
|
||||||
|
first design — otherwise approved devices land on the wrong canvas."""
|
||||||
|
first = await _add_design(db_session, "Default") # first design (fallback)
|
||||||
|
active = await _add_design(db_session, "zwave") # the design the user is on
|
||||||
|
dev = PendingDevice(
|
||||||
|
id=str(uuid.uuid4()),
|
||||||
|
ieee_address="zwave-H-2",
|
||||||
|
friendly_name="Living Room Plug",
|
||||||
|
suggested_type="zwave_router",
|
||||||
|
device_subtype="Router",
|
||||||
|
vendor="Aeotec",
|
||||||
|
model="ZW096",
|
||||||
|
status="pending",
|
||||||
|
discovery_source="zwave",
|
||||||
|
)
|
||||||
|
db_session.add(dev)
|
||||||
|
await db_session.commit()
|
||||||
|
|
||||||
|
res = await client.post(
|
||||||
|
"/api/v1/scan/pending/bulk-approve",
|
||||||
|
json={"device_ids": [dev.id], "design_id": active},
|
||||||
|
headers=headers,
|
||||||
|
)
|
||||||
|
assert res.status_code == 200
|
||||||
|
assert res.json()["approved"] == 1
|
||||||
|
|
||||||
|
node = (
|
||||||
|
await db_session.execute(select(Node).where(Node.ieee_address == "zwave-H-2"))
|
||||||
|
).scalar_one()
|
||||||
|
assert node.design_id == active
|
||||||
|
assert node.design_id != first
|
||||||
|
# Z-Wave device → online + Z-Wave property rows, no ICMP check.
|
||||||
|
assert node.status == "online"
|
||||||
|
assert node.check_method == "none"
|
||||||
|
assert {p["key"] for p in node.properties} == {"Z-Wave ID", "Vendor", "Model"}
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_single_approve_zwave_sets_wireless_fields(client, headers, db_session):
|
||||||
|
active = await _add_design(db_session, "zwave")
|
||||||
|
dev = PendingDevice(
|
||||||
|
id=str(uuid.uuid4()),
|
||||||
|
ieee_address="zwave-H-9",
|
||||||
|
friendly_name="Door Sensor",
|
||||||
|
suggested_type="zwave_enddevice",
|
||||||
|
vendor="Aeotec",
|
||||||
|
model="ZW120",
|
||||||
|
status="pending",
|
||||||
|
discovery_source="zwave",
|
||||||
|
)
|
||||||
|
db_session.add(dev)
|
||||||
|
await db_session.commit()
|
||||||
|
|
||||||
|
res = await client.post(
|
||||||
|
f"/api/v1/scan/pending/{dev.id}/approve",
|
||||||
|
json={"label": "Door Sensor", "type": "zwave_enddevice", "design_id": active},
|
||||||
|
headers=headers,
|
||||||
|
)
|
||||||
|
assert res.status_code == 200
|
||||||
|
|
||||||
|
node = (
|
||||||
|
await db_session.execute(select(Node).where(Node.ieee_address == "zwave-H-9"))
|
||||||
|
).scalar_one()
|
||||||
|
assert node.design_id == active
|
||||||
|
assert node.status == "online"
|
||||||
|
assert node.check_method == "none"
|
||||||
|
assert any(p["key"] == "Z-Wave ID" for p in node.properties)
|
||||||
|
|||||||
@@ -186,7 +186,9 @@ describe('api/client', () => {
|
|||||||
mod.scanApi.ignore('d1')
|
mod.scanApi.ignore('d1')
|
||||||
expect(api.post).toHaveBeenCalledWith('/scan/pending/d1/ignore')
|
expect(api.post).toHaveBeenCalledWith('/scan/pending/d1/ignore')
|
||||||
mod.scanApi.bulkApprove(['a', 'b'])
|
mod.scanApi.bulkApprove(['a', 'b'])
|
||||||
expect(api.post).toHaveBeenCalledWith('/scan/pending/bulk-approve', { device_ids: ['a', 'b'] })
|
expect(api.post).toHaveBeenCalledWith('/scan/pending/bulk-approve', { device_ids: ['a', 'b'], design_id: undefined })
|
||||||
|
mod.scanApi.bulkApprove(['a'], 'design-9')
|
||||||
|
expect(api.post).toHaveBeenCalledWith('/scan/pending/bulk-approve', { device_ids: ['a'], design_id: 'design-9' })
|
||||||
mod.scanApi.bulkHide(['a'])
|
mod.scanApi.bulkHide(['a'])
|
||||||
expect(api.post).toHaveBeenCalledWith('/scan/pending/bulk-hide', { device_ids: ['a'] })
|
expect(api.post).toHaveBeenCalledWith('/scan/pending/bulk-hide', { device_ids: ['a'] })
|
||||||
mod.scanApi.restore('d1')
|
mod.scanApi.restore('d1')
|
||||||
|
|||||||
@@ -81,7 +81,7 @@ export const scanApi = {
|
|||||||
}>(`/scan/pending/${id}/approve`, nodeData),
|
}>(`/scan/pending/${id}/approve`, nodeData),
|
||||||
hide: (id: string) => api.post(`/scan/pending/${id}/hide`),
|
hide: (id: string) => api.post(`/scan/pending/${id}/hide`),
|
||||||
ignore: (id: string) => api.post(`/scan/pending/${id}/ignore`),
|
ignore: (id: string) => api.post(`/scan/pending/${id}/ignore`),
|
||||||
bulkApprove: (ids: string[]) =>
|
bulkApprove: (ids: string[], designId?: string | null) =>
|
||||||
api.post<{
|
api.post<{
|
||||||
approved: number
|
approved: number
|
||||||
node_ids: string[]
|
node_ids: string[]
|
||||||
@@ -89,7 +89,7 @@ export const scanApi = {
|
|||||||
edges_created: number
|
edges_created: number
|
||||||
edges: { id: string; source: string; target: string }[]
|
edges: { id: string; source: string; target: string }[]
|
||||||
skipped: number
|
skipped: number
|
||||||
}>('/scan/pending/bulk-approve', { device_ids: ids }),
|
}>('/scan/pending/bulk-approve', { device_ids: ids, design_id: designId ?? undefined }),
|
||||||
bulkHide: (ids: string[]) => api.post<{ hidden: number; skipped: number }>('/scan/pending/bulk-hide', { device_ids: ids }),
|
bulkHide: (ids: string[]) => api.post<{ hidden: number; skipped: number }>('/scan/pending/bulk-hide', { device_ids: ids }),
|
||||||
restore: (id: string) => api.post<{ restored: boolean; device_id: string }>(`/scan/pending/${id}/restore`),
|
restore: (id: string) => api.post<{ restored: boolean; device_id: string }>(`/scan/pending/${id}/restore`),
|
||||||
bulkRestore: (ids: string[]) => api.post<{ restored: number; skipped: number }>('/scan/pending/bulk-restore', { device_ids: ids }),
|
bulkRestore: (ids: string[]) => api.post<{ restored: number; skipped: number }>('/scan/pending/bulk-restore', { device_ids: ids }),
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import {
|
|||||||
import { Dialog, DialogClose, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/dialog'
|
import { Dialog, DialogClose, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/dialog'
|
||||||
import { scanApi } from '@/api/client'
|
import { scanApi } from '@/api/client'
|
||||||
import { useCanvasStore } from '@/stores/canvasStore'
|
import { useCanvasStore } from '@/stores/canvasStore'
|
||||||
|
import { useDesignStore } from '@/stores/designStore'
|
||||||
import { toast } from 'sonner'
|
import { toast } from 'sonner'
|
||||||
import { PendingDeviceModal, type PendingDevice } from '@/components/modals/PendingDeviceModal'
|
import { PendingDeviceModal, type PendingDevice } from '@/components/modals/PendingDeviceModal'
|
||||||
import type { NodeType, ServiceInfo } from '@/types'
|
import type { NodeType, ServiceInfo } from '@/types'
|
||||||
@@ -127,6 +128,7 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus
|
|||||||
// Optionally restrict to devices that have at least one detected service.
|
// Optionally restrict to devices that have at least one detected service.
|
||||||
const [withServicesOnly, setWithServicesOnly] = useState(false)
|
const [withServicesOnly, setWithServicesOnly] = useState(false)
|
||||||
const { addNode, scanEventTs } = useCanvasStore()
|
const { addNode, scanEventTs } = useCanvasStore()
|
||||||
|
const activeDesignId = useDesignStore((s) => s.activeDesignId)
|
||||||
const highlightRef = useRef<HTMLButtonElement>(null)
|
const highlightRef = useRef<HTMLButtonElement>(null)
|
||||||
|
|
||||||
const load = useCallback(async () => {
|
const load = useCallback(async () => {
|
||||||
@@ -283,6 +285,8 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus
|
|||||||
status: wireless ? 'online' : 'unknown',
|
status: wireless ? 'online' : 'unknown',
|
||||||
services: (device.services ?? []) as ServiceInfo[],
|
services: (device.services ?? []) as ServiceInfo[],
|
||||||
properties,
|
properties,
|
||||||
|
// Approve onto the design the user is viewing, not the first design.
|
||||||
|
design_id: activeDesignId ?? undefined,
|
||||||
}
|
}
|
||||||
const res = await scanApi.approve(device.id, nodeData)
|
const res = await scanApi.approve(device.id, nodeData)
|
||||||
const nodeId = res.data.node_id
|
const nodeId = res.data.node_id
|
||||||
@@ -327,7 +331,7 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus
|
|||||||
const ids = [...selectedIds]
|
const ids = [...selectedIds]
|
||||||
if (ids.length === 0) return
|
if (ids.length === 0) return
|
||||||
try {
|
try {
|
||||||
const res = await scanApi.bulkApprove(ids)
|
const res = await scanApi.bulkApprove(ids, activeDesignId)
|
||||||
const deviceToNode: Record<string, string> = {}
|
const deviceToNode: Record<string, string> = {}
|
||||||
res.data.device_ids.forEach((did, i) => { deviceToNode[did] = res.data.node_ids[i] })
|
res.data.device_ids.forEach((did, i) => { deviceToNode[did] = res.data.node_ids[i] })
|
||||||
const approvedDevices = devices.filter((d) => ids.includes(d.id))
|
const approvedDevices = devices.filter((d) => ids.includes(d.id))
|
||||||
|
|||||||
@@ -213,7 +213,7 @@ describe('PendingDevicesModal', () => {
|
|||||||
fireEvent.click(screen.getByTestId('pending-card-dev-a'))
|
fireEvent.click(screen.getByTestId('pending-card-dev-a'))
|
||||||
fireEvent.click(screen.getByTestId('pending-card-dev-b'))
|
fireEvent.click(screen.getByTestId('pending-card-dev-b'))
|
||||||
fireEvent.click(screen.getByRole('button', { name: /Approve \(2\)/ }))
|
fireEvent.click(screen.getByRole('button', { name: /Approve \(2\)/ }))
|
||||||
await waitFor(() => expect(mockBulkApprove).toHaveBeenCalledWith(['dev-a', 'dev-b']))
|
await waitFor(() => expect(mockBulkApprove).toHaveBeenCalledWith(['dev-a', 'dev-b'], null))
|
||||||
})
|
})
|
||||||
|
|
||||||
it('bulk approve carries the scanned MAC onto the canvas node (#168)', async () => {
|
it('bulk approve carries the scanned MAC onto the canvas node (#168)', async () => {
|
||||||
@@ -290,7 +290,7 @@ describe('PendingDevicesModal', () => {
|
|||||||
fireEvent.click(screen.getByRole('button', { name: 'Select mode' }))
|
fireEvent.click(screen.getByRole('button', { name: 'Select mode' }))
|
||||||
fireEvent.click(screen.getByTestId('pending-card-dev-a'))
|
fireEvent.click(screen.getByTestId('pending-card-dev-a'))
|
||||||
fireEvent.keyDown(window, { key: 'Enter' })
|
fireEvent.keyDown(window, { key: 'Enter' })
|
||||||
await waitFor(() => expect(mockBulkApprove).toHaveBeenCalledWith(['dev-a']))
|
await waitFor(() => expect(mockBulkApprove).toHaveBeenCalledWith(['dev-a'], null))
|
||||||
expect(mockBulkRestore).not.toHaveBeenCalled()
|
expect(mockBulkRestore).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user