Pr/librems id multi fixes

docs/development/testing.md

@@ -38,8 +38,24 @@ The test suite covers all major plugin functionality. Tests are organized by the
 | [test_coverage_device_fields.py](../../netbox_librenms_plugin/tests/test_coverage_device_fields.py) | Device field sync view—field update logic and device field mapping |
 | [test_coverage_list.py](../../netbox_librenms_plugin/tests/test_coverage_list.py) | Import list view—background job decision, job result loading, and GET handler |
 | [test_coverage_api.py](../../netbox_librenms_plugin/tests/test_coverage_api.py) | LibreNMS API client—malformed payload guards, error paths, and edge cases |
+| [test_coverage_api2.py](../../netbox_librenms_plugin/tests/test_coverage_api2.py) | API views—device status, background job management, VM status endpoints |
+| [test_coverage_base_views.py](../../netbox_librenms_plugin/tests/test_coverage_base_views.py) | Base view coverage tests—sync table views, context data, and data pipeline |
+| [test_coverage_base_views2.py](../../netbox_librenms_plugin/tests/test_coverage_base_views2.py) | Additional base view coverage—IP address sync, cable matching, edge cases |
+| [test_coverage_cache.py](../../netbox_librenms_plugin/tests/test_coverage_cache.py) | Import cache helpers—cache key generation, active search tracking, metadata |
+| [test_coverage_device_operations.py](../../netbox_librenms_plugin/tests/test_coverage_device_operations.py) | Device validation—type matching, serial handling, VC detection, role lookup |
+| [test_coverage_forms.py](../../netbox_librenms_plugin/tests/test_coverage_forms.py) | Import forms—filter form choices, background-job option guards, field validation |
+| [test_coverage_mixins.py](../../netbox_librenms_plugin/tests/test_coverage_mixins.py) | View mixins—VLAN group scope resolution, VlanAssignmentMixin, scope priority |
+| [test_coverage_sync_interfaces.py](../../netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py) | Interface sync view—port caching, attribute updates, MAC handling, VC member routing |
 | [test_coverage_sync_view.py](../../netbox_librenms_plugin/tests/test_coverage_sync_view.py) | Sync view base class—context preparation and tab rendering |
+| [test_coverage_sync_views.py](../../netbox_librenms_plugin/tests/test_coverage_sync_views.py) | Sync action views—cables, IP addresses, VLAN sync action handlers |
+| [test_coverage_sync_views2.py](../../netbox_librenms_plugin/tests/test_coverage_sync_views2.py) | Additional sync action view coverage—device fields, device name/type sync |
+| [test_coverage_sync_views3.py](../../netbox_librenms_plugin/tests/test_coverage_sync_views3.py) | Further sync action view coverage—location sync, VLAN assignment edge cases |
+| [test_coverage_actions.py](../../netbox_librenms_plugin/tests/test_coverage_actions.py) | Import action views—bulk import, device role/cluster/rack update, validation details |
 | [test_coverage_filters.py](../../netbox_librenms_plugin/tests/test_coverage_filters.py) | Import filter logic—filter form processing and device count helpers |
+| [test_coverage_tables.py](../../netbox_librenms_plugin/tests/test_coverage_tables.py) | Sync tables—column rendering, row data, interface and cable table helpers |
+| [test_coverage_utils.py](../../netbox_librenms_plugin/tests/test_coverage_utils.py) | Utility function coverage—name matching, speed conversion, site/platform lookup |
+| [test_coverage_virtual_chassis.py](../../netbox_librenms_plugin/tests/test_coverage_virtual_chassis.py) | Virtual chassis coverage—VC creation, position conflict handling, member naming |
+| [test_coverage_vlans_table.py](../../netbox_librenms_plugin/tests/test_coverage_vlans_table.py) | VLAN sync table—column rendering, group assignment, VLAN comparison rows |
 | [test_sync_modules.py](../../netbox_librenms_plugin/tests/test_sync_modules.py) | Module sync—inventory matching, module type resolution, and normalization rules |
 | [test_modules_view.py](../../netbox_librenms_plugin/tests/test_modules_view.py) | Module sync view—context preparation, table rendering, and module bay mapping |
 | [test_tables_modules.py](../../netbox_librenms_plugin/tests/test_tables_modules.py) | Module tables—column rendering, row formatting, and action buttons |
@@ -76,7 +92,7 @@ pytest netbox_librenms_plugin/tests/test_librenms_api.py::TestLibreNMSAPIConnect
 
 ```bash
 # API client tests
-pytest netbox_librenms_plugin/tests/test_librenms_api.py -v
+pytest netbox_librenms_plugin/tests/test_librenms_api.py netbox_librenms_plugin/tests/test_coverage_api.py netbox_librenms_plugin/tests/test_coverage_api2.py -v
 
 # Import and validation tests
 pytest netbox_librenms_plugin/tests/test_import_utils.py netbox_librenms_plugin/tests/test_import_validation_helpers.py netbox_librenms_plugin/tests/test_utils.py -v

docs/usage_tips/custom_field.md

@@ -4,6 +4,9 @@
 
 To enhance device identification and synchronization between NetBox and LibreNMS, this plugin supports using a custom field `librenms_id` on Device, Virtual Machine and Interface objects. While the plugin works without it, using this custom field is recommended for LibreNMS API lookups, and to assist with matching the remote device and remote interfaces for cable creation in Netbox. It can also be entered manually if no primary IP or FQDN is available.
 
+!!! info "Automatic Creation"
+    As of version 0.4.2, the plugin **automatically creates** the `librenms_id` custom field when migrations are run. You no longer need to create it manually. The field is created for Device, Virtual Machine, Interface, and VM Interface objects.
+
 For the Device and Virtual Machine objects the plugin will automatically populate the LibreNMS ID custom field when opening the LibreNMS Sync page if the device has been found in LibreNMS.
 
 For the Interface object, the plugin will automatically populate the LibreNMS ID custom field when the interface data is synced from LibreNMS.
@@ -15,7 +18,10 @@ For the Interface object, the plugin will automatically populate the LibreNMS ID
 - **Efficient Synchronization:** Enhances the reliability of API lookups.
 - **Cable creation:** Allows better device identification for the creation of cables between NetBox devices.
 
-## Suggested Custom Field Setup
+## Manual Custom Field Setup (Legacy)
+
+!!! note
+    This section is only needed if you are running an older version of the plugin that does not auto-create the field, or if you need to recreate it after deletion.
 
 Follow these steps to create the `librenms_id` custom field in NetBox:
 

netbox_librenms_plugin/__init__.py

@@ -28,6 +28,7 @@ def ready(self):
         super().ready()
 
         from django.conf import settings
+        from django.db.models.signals import post_migrate
 
         plugin_config = getattr(settings, "PLUGINS_CONFIG", {}).get(self.name, {})
 
@@ -37,6 +38,12 @@ def ready(self):
         else:
             self._validate_legacy_config(plugin_config)
 
+        # Auto-create the librenms_id custom field after migrations complete
+        post_migrate.connect(
+            _ensure_librenms_id_custom_field,
+            dispatch_uid="netbox_librenms_plugin_ensure_cf",
+        )
+
     def _validate_multi_server_config(self, servers_config):
         """Validate multi-server configuration."""
         if not servers_config or not isinstance(servers_config, dict):
@@ -61,4 +68,63 @@ def _validate_legacy_config(self, plugin_config):
                 )
 
 
+def _ensure_librenms_id_custom_field(sender, **kwargs):
+    """
+    Auto-create the 'librenms_id' custom field if it doesn't exist.
+    Runs after migrations via post_migrate signal to ensure tables exist.
+    Uses dispatch_uid to avoid duplicate connections.
+    """
+    # Only run once per migrate invocation (post_migrate fires per-app).
+    # The _executed flag is intentionally never reset: migrations are expected to
+    # run in short-lived CLI processes (manage.py migrate) where the flag is
+    # naturally cleared on exit.  Long-running processes (e.g. gunicorn workers)
+    # should not rely on this handler re-executing after startup.
+    if getattr(_ensure_librenms_id_custom_field, "_executed", False):
+        return
+    _ensure_librenms_id_custom_field._executed = True  # not reset; see comment above
+
+    try:
+        from django.contrib.contenttypes.models import ContentType
+
+        from extras.models import CustomField
+
+        cf, created = CustomField.objects.get_or_create(
+            name="librenms_id",
+            defaults={
+                "type": "json",
+                "label": "LibreNMS ID",
+                "description": "LibreNMS Device ID for synchronization (auto-created by plugin)",
+                "required": False,
+                "ui_visible": "if-set",
+                "ui_editable": "yes",
+                "is_cloneable": False,
+            },
+        )
+
+        # Ensure the field is assigned to the required object types
+        from dcim.models import Device, Interface
+        from virtualization.models import VirtualMachine, VMInterface
+
+        required_models = [Device, VirtualMachine, Interface, VMInterface]
+        current_types = set(cf.object_types.values_list("pk", flat=True))
+
+        for model in required_models:
+            ct = ContentType.objects.get_for_model(model)
+            if ct.pk not in current_types:
+                cf.object_types.add(ct)
+
+        if created:
+            import logging
+
+            logging.getLogger("netbox_librenms_plugin").info(
+                "Auto-created 'librenms_id' custom field for Device, VirtualMachine, Interface, VMInterface"
+            )
+    except Exception as e:
+        # Don't break startup if custom field creation fails (e.g., during initial migration),
+        # but log the error so it's not silently swallowed.
+        import logging
+
+        logging.getLogger("netbox_librenms_plugin").exception("Failed to auto-create 'librenms_id' custom field: %s", e)
+
+
 config = LibreNMSSyncConfig

netbox_librenms_plugin/import_utils/bulk_import.py

@@ -99,13 +99,13 @@ def bulk_import_devices_shared(
     if user is None and job is not None:
         user = getattr(job.job, "user", None)
 
-    # Check permissions at start of bulk operation — both device and VM perms are
+    # Check permissions at start of bulk operation — device and VM add perms are
     # required because any device may be flagged as import_as_vm during validation.
+    # change_device is needed for VC master/member updates; VMs are only created, not changed.
     required_perms = [
         "dcim.add_device",
         "dcim.change_device",
         "virtualization.add_virtualmachine",
-        "virtualization.change_virtualmachine",
     ]
     require_permissions(user, required_perms, "import devices")
 
@@ -239,51 +239,50 @@ def bulk_import_devices_shared(
                             for m in vc_data.get("members", [])
                         )
                         if member_parts:
-                            fingerprint = hashlib.md5((f"{device_id}," + ",".join(member_parts)).encode()).hexdigest()[
-                                :12
-                            ]
+                            fingerprint = hashlib.md5(",".join(member_parts).encode()).hexdigest()[:12]
                             vc_domain = f"librenms-stack-{fingerprint}"
                         else:
                             vc_domain = f"librenms-{device_id}"
 
-                    # Guard VC creation with its own permission check — the upfront check
-                    # only covers add_device/change_device; VirtualChassis needs a separate perm.
-                    has_vc_perm, missing_vc_perms = check_user_permissions(user, ["dcim.add_virtualchassis"])
-                    if not has_vc_perm:
-                        warn_msg = (
-                            f"Skipping VC creation for device {device_id}: "
-                            f"missing permissions: {', '.join(missing_vc_perms)}"
-                        )
-                        if job and job.logger:
-                            job.logger.warning(warn_msg)
-                        else:
-                            logger.warning(warn_msg)
                     # Only create VC if we haven't processed this stack yet
-                    # Add to set BEFORE attempting creation to prevent race condition
-                    elif vc_domain not in processed_vc_domains:
-                        processed_vc_domains.add(vc_domain)
-                        try:
-                            vc = create_virtual_chassis_with_members(
-                                result["device"],
-                                vc_data["members"],
-                                libre_device,
-                                server_key=api.server_key,
+                    if vc_domain not in processed_vc_domains:
+                        # Guard VC creation with its own permission check — the upfront check
+                        # only covers add_device/change_device; VirtualChassis needs a separate perm.
+                        has_vc_perm, missing_vc_perms = check_user_permissions(user, ["dcim.add_virtualchassis"])
+                        if not has_vc_perm:
+                            warn_msg = (
+                                f"Skipping VC creation for device {device_id}: "
+                                f"missing permissions: {', '.join(missing_vc_perms)}"
                             )
-                            vc_created_count += 1
-                            log_msg = f"Created VC '{vc.name}' during bulk import for device {device_id}"
-                            if job and job.logger:
-                                job.logger.info(log_msg)
-                            else:
-                                logger.info(log_msg)
-                        except Exception as vc_error:
-                            # Remove from set on failure so retry is possible
-                            processed_vc_domains.discard(vc_domain)
-                            warn_msg = f"Failed to create VC for device {device_id}: {vc_error}"
                             if job and job.logger:
                                 job.logger.warning(warn_msg)
                             else:
                                 logger.warning(warn_msg)
-                            # Don't fail the import, just log the warning
+                        else:
+                            # Add to set BEFORE attempting creation to prevent race condition
+                            processed_vc_domains.add(vc_domain)
+                            try:
+                                vc = create_virtual_chassis_with_members(
+                                    result["device"],
+                                    vc_data["members"],
+                                    libre_device,
+                                    server_key=api.server_key,
+                                )
+                                vc_created_count += 1
+                                log_msg = f"Created VC '{vc.name}' during bulk import for device {device_id}"
+                                if job and job.logger:
+                                    job.logger.info(log_msg)
+                                else:
+                                    logger.info(log_msg)
+                            except Exception as vc_error:
+                                # Remove from set on failure so retry is possible
+                                processed_vc_domains.discard(vc_domain)
+                                warn_msg = f"Failed to create VC for device {device_id}: {vc_error}"
+                                if job and job.logger:
+                                    job.logger.warning(warn_msg)
+                                else:
+                                    logger.warning(warn_msg)
+                                # Don't fail the import, just log the warning
 
             elif result.get("device"):  # Device exists
                 skipped_list.append({"device_id": device_id, "reason": result["error"]})
@@ -386,8 +385,11 @@ def _refresh_existing_device(validation: dict, libre_device: dict = None, server
                 elif not validation.get("import_as_vm"):
                     validation["device_role"] = {"found": False, "role": None}
                     remove_validation_issue(validation, "role")
-                    validation.setdefault("issues", []).append("Device role must be manually selected before import")
                 recalculate_validation_status(validation, is_vm=bool(validation.get("import_as_vm")))
+                # Re-assert non-importable state: recalculate bases can_import on
+                # issues alone, but an existing matched device must never be import-ready.
+                validation["can_import"] = False
+                validation["is_ready"] = False
                 return
             else:
                 # Device was deleted since caching — recompute readiness to match
@@ -403,7 +405,7 @@ def _refresh_existing_device(validation: dict, libre_device: dict = None, server
         except Exception as e:
             existing_id = getattr(existing, "pk", "unknown") if existing else "none"
             logger.error(f"Failed to refresh existing device (pk={existing_id}): {e}")
-        return
+            return
 
     # existing_device was None at cache time — check if device was imported since
     if not libre_device:
@@ -468,9 +470,10 @@ def _lookup_in_model(m):
             actual_is_vm = found_as_cross_model != import_as_vm  # XOR: cross flips the flag
             validation["import_as_vm"] = actual_is_vm  # Update so future refreshes query correct model
             if not actual_is_vm and hasattr(new_device, "role") and new_device.role:
-                validation["device_role"] = {"found": True, "role": new_device.role}
+                apply_role_to_validation(validation, new_device.role, is_vm=False)
             elif not actual_is_vm:
-                validation.setdefault("device_role", {}).update({"found": False, "role": None})
+                validation["device_role"] = {"found": False, "role": None}
+            recalculate_validation_status(validation, is_vm=actual_is_vm)
     except Exception as e:
         logger.error(f"Failed to check for newly imported device: {e}")