From 8fc808f8f9e677d26790701ad04ea66be408c27e Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Tue, 31 Mar 2026 22:31:08 +0200 Subject: [PATCH 1/9] fix: code quality improvements and bug fixes - __init__.py: per-alias execution tracking (_executed_aliases set), startup migration for integer->json librenms_id field, use using(db_alias) for DB ops - api/views.py: job names from FilterDevicesJob/ImportDevicesJob Meta, add filterset_class to InterfaceTypeMappingViewSet, catch NoSuchJobError explicitly, update LibreNMSPluginPermission docstring, scope sync_job_status to owner - import_utils/bulk_import.py: extract _is_job_cancelled helper, remove vc_detection_enabled param (handled via sync_options), add virtualization.change_virtualmachine to required perms, move _safe_disabled import from filters module - import_utils/vm_operations.py: use _is_job_cancelled helper (dedup) - jobs.py: remove vc_detection_enabled param from ImportDevicesJob - views/base/cables_view.py: table=None init before conditional, docstring cleanup, explicit for-loop instead of list comprehension - views/imports/actions.py: inline _resolve_naming_preferences, move _DEVICE_ONLY_ACTIONS to module level, fix resolve_naming_preferences import - tests: update test_init for _executed_aliases/using(db_alias), update test_coverage_api2 to raise NoSuchJobError, fix patch paths for get_user_pref --- netbox_librenms_plugin/__init__.py | 40 +++-- netbox_librenms_plugin/api/views.py | 31 +++- .../import_utils/bulk_import.py | 145 ++++++------------ .../import_utils/vm_operations.py | 25 +-- netbox_librenms_plugin/jobs.py | 3 - .../tests/test_coverage_actions.py | 6 +- .../tests/test_coverage_api2.py | 11 +- .../tests/test_import_utils.py | 2 +- netbox_librenms_plugin/tests/test_init.py | 43 ++++-- .../views/base/cables_view.py | 36 +++-- .../views/imports/actions.py | 81 ++++++++-- 11 files changed, 230 insertions(+), 193 deletions(-) diff --git a/netbox_librenms_plugin/__init__.py b/netbox_librenms_plugin/__init__.py index 299ceadd3e..6676229613 100644 --- a/netbox_librenms_plugin/__init__.py +++ b/netbox_librenms_plugin/__init__.py @@ -70,25 +70,28 @@ 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. + Auto-create (or migrate) the 'librenms_id' custom field. Runs after migrations via post_migrate signal to ensure tables exist. Uses dispatch_uid to avoid duplicate connections. + + librenms_id stores a per-server JSON mapping {"server_key": device_id}. + Legacy installations may have this field typed as 'integer'; we upgrade it + to 'json' automatically so the UI and API accept the dict format. """ - # 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): + # Track per-alias execution so each database alias is bootstrapped exactly once. + db_alias = kwargs.get("using") or "default" + executed_aliases = getattr(_ensure_librenms_id_custom_field, "_executed_aliases", set()) + if db_alias in executed_aliases: return - _ensure_librenms_id_custom_field._executed = True # not reset; see comment above + + import logging try: from django.contrib.contenttypes.models import ContentType from extras.models import CustomField - cf, created = CustomField.objects.get_or_create( + cf, created = CustomField.objects.using(db_alias).get_or_create( name="librenms_id", defaults={ "type": "json", @@ -101,6 +104,15 @@ def _ensure_librenms_id_custom_field(sender, **kwargs): }, ) + # Migrate legacy integer-typed field to JSON so the multi-server + # dict format {"server_key": device_id} is accepted by the UI/API. + if not created and cf.type == "integer": + cf.type = "json" + cf.save(using=db_alias, update_fields=["type"]) + logging.getLogger("netbox_librenms_plugin").info( + "Migrated 'librenms_id' custom field type from integer to json" + ) + # Ensure the field is assigned to the required object types from dcim.models import Device, Interface from virtualization.models import VirtualMachine, VMInterface @@ -109,21 +121,21 @@ def _ensure_librenms_id_custom_field(sender, **kwargs): current_types = set(cf.object_types.values_list("pk", flat=True)) for model in required_models: - ct = ContentType.objects.get_for_model(model) + ct = ContentType.objects.db_manager(db_alias).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" ) + + # Mark this alias as executed after successful completion to allow retry on failure. + executed_aliases.add(db_alias) + _ensure_librenms_id_custom_field._executed_aliases = executed_aliases 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) diff --git a/netbox_librenms_plugin/api/views.py b/netbox_librenms_plugin/api/views.py index 768c67f5fe..a5d440b8fb 100644 --- a/netbox_librenms_plugin/api/views.py +++ b/netbox_librenms_plugin/api/views.py @@ -8,9 +8,12 @@ from netbox.api.viewsets import NetBoxModelViewSet from rest_framework.decorators import api_view, permission_classes from rest_framework.permissions import BasePermission, SAFE_METHODS +from rq.exceptions import NoSuchJobError from rq.job import Job as RQJob from netbox_librenms_plugin.constants import PERM_CHANGE_PLUGIN, PERM_VIEW_PLUGIN +from netbox_librenms_plugin.filters import InterfaceTypeMappingFilterSet +from netbox_librenms_plugin.jobs import FilterDevicesJob, ImportDevicesJob from netbox_librenms_plugin.models import InterfaceTypeMapping from .serializers import InterfaceTypeMappingSerializer @@ -22,8 +25,8 @@ class LibreNMSPluginPermission(BasePermission): """ Permission class for LibreNMS plugin API endpoints. - - GET requests require view_librenmssettings - - All other requests require change_librenmssettings + - Safe requests (GET, HEAD, OPTIONS) require netbox_librenms_plugin.view_librenmssettings + - All other requests require netbox_librenms_plugin.change_librenmssettings """ def has_permission(self, request, view): @@ -36,6 +39,7 @@ class InterfaceTypeMappingViewSet(NetBoxModelViewSet): """API viewset for InterfaceTypeMapping CRUD operations.""" permission_classes = [LibreNMSPluginPermission] + filterset_class = InterfaceTypeMappingFilterSet queryset = InterfaceTypeMapping.objects.all() serializer_class = InterfaceTypeMappingSerializer @@ -50,6 +54,8 @@ def sync_job_status(request, job_pk): This is needed because NetBox's worker doesn't always update the database when a job is stopped before it starts processing. + Only allows users to sync their own LibreNMS jobs. + Args: request: Django request job_pk: Primary key of the Job to sync @@ -57,8 +63,9 @@ def sync_job_status(request, job_pk): Returns: JsonResponse with updated status """ + _LIBRENMS_JOB_NAMES = (FilterDevicesJob.Meta.name, ImportDevicesJob.Meta.name) try: - job = Job.objects.get(pk=job_pk) + job = Job.objects.get(pk=job_pk, user=request.user, name__in=_LIBRENMS_JOB_NAMES) except Job.DoesNotExist: return JsonResponse({"error": "Job not found"}, status=404) @@ -74,18 +81,26 @@ def sync_job_status(request, job_pk): if not job.completed: job.completed = timezone.now() job.save(update_fields=["status", "completed"]) - logger.info(f"Synced Job #{job.pk}: DB status updated to failed (RQ: {rq_status})") + logger.info("Synced Job #%s: DB status updated to failed (RQ: %s)", job.pk, rq_status) return JsonResponse({"status": "updated", "db_status": job.status, "rq_status": rq_status}) else: # Job still active in RQ return JsonResponse({"status": "no_change", "db_status": job.status, "rq_status": rq_status}) - except Exception as e: - # Job not in RQ queue - mark as failed - logger.warning(f"Job #{job.pk} not found in RQ: {e}") - if job.status == JobStatusChoices.STATUS_RUNNING: + except NoSuchJobError: + # Job not in RQ queue — mark any non-terminal DB job as failed + logger.warning("Job #%s not found in RQ (NoSuchJobError)", job.pk) + terminal_states = { + JobStatusChoices.STATUS_COMPLETED, + JobStatusChoices.STATUS_FAILED, + JobStatusChoices.STATUS_ERRORED, + } + if job.status not in terminal_states: job.status = JobStatusChoices.STATUS_FAILED if not job.completed: job.completed = timezone.now() job.save(update_fields=["status", "completed"]) return JsonResponse({"status": "updated", "db_status": job.status, "rq_status": "not_found"}) return JsonResponse({"status": "no_change", "db_status": job.status, "rq_status": "not_found"}) + except Exception as e: + logger.exception("Unexpected error fetching RQ job for Job #%s: %s", job.pk, e) + return JsonResponse({"error": "Failed to fetch RQ job status"}, status=500) diff --git a/netbox_librenms_plugin/import_utils/bulk_import.py b/netbox_librenms_plugin/import_utils/bulk_import.py index a01bc7562f..8541f6022c 100644 --- a/netbox_librenms_plugin/import_utils/bulk_import.py +++ b/netbox_librenms_plugin/import_utils/bulk_import.py @@ -12,7 +12,7 @@ from ..utils import find_by_librenms_id from .cache import get_cache_metadata_key, get_import_device_cache_key, get_validated_device_cache_key from .device_operations import import_single_device, validate_device_for_import -from .filters import get_librenms_devices_for_import +from .filters import _safe_disabled, get_librenms_devices_for_import from .permissions import check_user_permissions, require_permissions from .virtual_chassis import ( create_virtual_chassis_with_members, @@ -23,27 +23,31 @@ logger = logging.getLogger(__name__) -def _safe_disabled(device: dict) -> int: +def _is_job_cancelled(job) -> bool: """ - Return 1 if the device is disabled, 0 otherwise. + Return True if a background job has been stopped or cancelled. - Handles None, booleans, numeric strings, and common truthy/falsy tokens - (e.g. "true"/"yes"/"on" → 1, "false"/"no"/"off" → 0) without raising. + Checks RQ/Redis state first (reflects stop API calls immediately); + falls back to the database status if Redis is unavailable. """ - val = device.get("disabled", 0) - if isinstance(val, bool): - return int(val) - if isinstance(val, str): - normalized = val.strip().lower() - if normalized in ("1", "true", "yes", "on"): - return 1 - if normalized in ("0", "false", "no", "off", ""): - return 0 try: - int_val = int(val) - return 1 if int_val else 0 - except (TypeError, ValueError): - return 0 + from django_rq import get_queue + from rq.job import Job as RQJob + + queue = get_queue("default") + rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) + return rq_job.is_failed or rq_job.is_stopped + except Exception: + job.job.refresh_from_db() + job_status = job.job.status + status_value = job_status.value if hasattr(job_status, "value") else job_status + return status_value in ( + JobStatusChoices.STATUS_FAILED, + JobStatusChoices.STATUS_ERRORED, + "failed", + "errored", + "stopped", + ) def bulk_import_devices_shared( @@ -52,7 +56,6 @@ def bulk_import_devices_shared( sync_options: dict = None, manual_mappings_per_device: dict = None, libre_devices_cache: dict = None, - vc_detection_enabled: bool = False, job=None, user=None, ) -> dict: @@ -70,8 +73,6 @@ def bulk_import_devices_shared( Example: {1179: {'device_role_id': 5}, 1180: {'device_role_id': 3}} libre_devices_cache: Optional dict mapping device_id to pre-fetched device data to avoid redundant API calls. Example: {123: {...device_data...}} - vc_detection_enabled: Whether to enable virtual chassis detection during import. - Should match the flag used during the filter/preview step for consistency. job: Optional JobRunner instance for progress logging and cancellation checks user: User performing the import (for permission checks). If job is provided, user is extracted from job.job.user if not explicitly passed. @@ -101,11 +102,13 @@ def bulk_import_devices_shared( # 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. + # change_device is needed for VC master/member updates; change_virtualmachine is + # needed when refreshing an existing VM (e.g. setting librenms_id on a re-import). required_perms = [ "dcim.add_device", "dcim.change_device", "virtualization.add_virtualmachine", + "virtualization.change_virtualmachine", ] require_permissions(user, required_perms, "import devices") @@ -122,35 +125,13 @@ def bulk_import_devices_shared( for idx, device_id in enumerate(device_ids, start=1): # Check for job cancellation on first iteration and every 5th thereafter. - # Check RQ/Redis state first (reflects stop API immediately); fall back to DB. - if job and (idx == 1 or idx % 5 == 0): - try: - from django_rq import get_queue - from rq.job import Job as RQJob - - queue = get_queue("default") - rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) - if rq_job.is_failed or rq_job.is_stopped: - if job.logger: - job.logger.warning( - f"Import job stopped at device {idx} of {total} (RQ status: {rq_job.get_status()})" - ) - else: - logger.warning(f"Import cancelled at device {idx} of {total}") - _cancelled = True - break - except Exception: - # Fall back to DB check if RQ is unavailable - job.job.refresh_from_db() - job_status = job.job.status - status_value = job_status.value if hasattr(job_status, "value") else job_status - if status_value in (JobStatusChoices.STATUS_FAILED, "failed", "errored"): - if job.logger: - job.logger.warning(f"Import job cancelled at device {idx} of {total}") - else: - logger.warning(f"Import cancelled at device {idx} of {total}") - _cancelled = True - break + if job and (idx == 1 or idx % 5 == 0) and _is_job_cancelled(job): + if job.logger: + job.logger.warning(f"Import job stopped at device {idx} of {total}") + else: + logger.warning(f"Import cancelled at device {idx} of {total}") + _cancelled = True + break try: # Use cached device data if available to avoid redundant API calls @@ -174,10 +155,10 @@ def bulk_import_devices_shared( validation = validate_device_for_import( libre_device, api=api, + include_vc_detection=(sync_options.get("vc_detection_enabled", True) if sync_options else True), use_sysname=use_sysname_opt, strip_domain=strip_domain_opt, server_key=api.server_key, - include_vc_detection=vc_detection_enabled, ) # Build manual mappings from validation + any provided overrides @@ -315,7 +296,6 @@ def bulk_import_devices( sync_options: dict = None, manual_mappings_per_device: dict = None, libre_devices_cache: dict = None, - vc_detection_enabled: bool = False, user=None, ) -> dict: """ @@ -332,7 +312,6 @@ def bulk_import_devices( Example: {1179: {'device_role_id': 5}, 1180: {'device_role_id': 3}} libre_devices_cache: Optional dict mapping device_id to pre-fetched device data to avoid redundant API calls. Example: {123: {...device_data...}} - vc_detection_enabled: Whether to enable virtual chassis detection during import. user: User performing the import (for permission checks) Returns: @@ -354,7 +333,6 @@ def bulk_import_devices( sync_options=sync_options, manual_mappings_per_device=manual_mappings_per_device, libre_devices_cache=libre_devices_cache, - vc_detection_enabled=vc_detection_enabled, job=None, # No job context for synchronous imports user=user, ) @@ -524,6 +502,9 @@ def process_device_filters( # Fetch devices from LibreNMS if job: job.logger.info(f"Fetching devices with filters: {filters}") + if _is_job_cancelled(job): + job.logger.warning("Job was stopped before fetching devices") + return _empty_return(return_cache_status) else: logger.info(f"Fetching devices with filters: {filters}") @@ -547,6 +528,11 @@ def process_device_filters( else: logger.info(f"Found {len(libre_devices)} devices") + # Check for early cancellation before the expensive VC prefetch + if job and _is_job_cancelled(job): + job.logger.warning("Job was stopped before VC pre-fetch") + return _empty_return(return_cache_status) + # Pre-warm VC cache if needed if vc_detection_enabled and libre_devices: device_ids = [d["device_id"] for d in libre_devices] @@ -575,52 +561,17 @@ def process_device_filters( if job: job.logger.info(f"Starting validation of {total} devices") - # Initial check if job was already terminated before we even started - try: - from django_rq import get_queue - from rq.job import Job as RQJob - - queue = get_queue("default") - rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) - - if rq_job.is_failed or rq_job.is_stopped: - job.logger.warning("Job was already stopped before validation started") - return _empty_return(return_cache_status) - except Exception: - # Fall back to DB check if RQ check fails - job.job.refresh_from_db() - if job.job.status in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED): - job.logger.warning("Job was stopped before validation started") - return _empty_return(return_cache_status) + if _is_job_cancelled(job): + job.logger.warning("Job was already stopped before validation started") + return _empty_return(return_cache_status) else: logger.info(f"Validating {total} devices") for idx, device in enumerate(libre_devices, 1): - # Check for job termination or client disconnect periodically - if idx % 5 == 0 or idx == 1: # Check more frequently (every 5 devices + first device) - if job: - # Check if job was terminated via stop API - # CRITICAL: Check the RQ job status in Redis, not just the DB model - # NetBox's stop endpoint marks the RQ job as failed in Redis - try: - from django_rq import get_queue - from rq.job import Job as RQJob - - queue = get_queue("default") - rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) - - # Check if RQ job is in a stopped state - if rq_job.is_failed or rq_job.is_stopped: - job.logger.info( - f"Job stopped at device {idx}/{total} (RQ status: {rq_job.get_status()}). Exiting gracefully." - ) - return _empty_return(return_cache_status) - except Exception: - # If we can't check RQ status, fall back to DB status check - job.job.refresh_from_db() - if job.job.status in (JobStatusChoices.STATUS_FAILED, JobStatusChoices.STATUS_ERRORED): - job.logger.info(f"Job stopped at device {idx}/{total}. Exiting gracefully.") - return _empty_return(return_cache_status) + # Check for job termination periodically + if (idx % 5 == 0 or idx == 1) and job and _is_job_cancelled(job): + job.logger.info(f"Job stopped at device {idx}/{total}. Exiting gracefully.") + return _empty_return(return_cache_status) # Drop any cached validation/meta keys before recomputing device.pop("_validation", None) diff --git a/netbox_librenms_plugin/import_utils/vm_operations.py b/netbox_librenms_plugin/import_utils/vm_operations.py index e1c43cc434..a630b55632 100644 --- a/netbox_librenms_plugin/import_utils/vm_operations.py +++ b/netbox_librenms_plugin/import_utils/vm_operations.py @@ -8,6 +8,7 @@ from virtualization.models import Cluster from ..librenms_api import LibreNMSAPI +from .bulk_import import _is_job_cancelled from .device_operations import _determine_device_name, fetch_device_with_cache, validate_device_for_import from .permissions import require_permissions @@ -152,26 +153,10 @@ def bulk_import_vms( for idx, vm_id in enumerate(vm_ids, start=1): # Check for job cancellation before first VM and every 5 thereafter - if job and (idx == 1 or idx % 5 == 0): - cancelled = False - try: - from django_rq import get_queue - from rq.job import Job as RQJob - - queue = get_queue("default") - rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) - if rq_job.is_failed or rq_job.is_stopped: - cancelled = True - except Exception: - job.job.refresh_from_db() - job_status = job.job.status - status_value = job_status.value if hasattr(job_status, "value") else job_status - if status_value in ("failed", "errored", "stopped"): - cancelled = True - if cancelled: - log.warning(f"Job cancelled at VM {idx} of {len(vm_ids)}") - break - log.info(f"Processing VM {idx} of {len(vm_ids)}") + if job and (idx == 1 or idx % 5 == 0) and _is_job_cancelled(job): + log.warning(f"Job cancelled at VM {idx} of {len(vm_ids)}") + break + log.info(f"Processing VM {idx} of {len(vm_ids)}") try: # Fetch device data (uses cache helper) diff --git a/netbox_librenms_plugin/jobs.py b/netbox_librenms_plugin/jobs.py index d8aa638c73..781deb28c2 100644 --- a/netbox_librenms_plugin/jobs.py +++ b/netbox_librenms_plugin/jobs.py @@ -166,7 +166,6 @@ def run( vm_imports, server_key=None, sync_options=None, - vc_detection_enabled=False, manual_mappings_per_device=None, libre_devices_cache=None, **kwargs, @@ -179,7 +178,6 @@ def run( vm_imports: Dict mapping device_id to cluster/role info for VM imports server_key: Optional LibreNMS server key for multi-server setups sync_options: Dict with sync_interfaces, sync_cables, sync_ips, use_sysname, strip_domain - vc_detection_enabled: Whether VC detection was enabled during the filter step. manual_mappings_per_device: Dict mapping device_id to manual_mappings dict libre_devices_cache: Optional dict mapping device_id to pre-fetched device data **kwargs: Additional job parameters @@ -214,7 +212,6 @@ def run( sync_options=sync_options, manual_mappings_per_device=manual_mappings_per_device, libre_devices_cache=libre_devices_cache, - vc_detection_enabled=vc_detection_enabled, job=self, # Pass job context for logging and cancellation user=self.job.user, # Pass user for permission checks ) diff --git a/netbox_librenms_plugin/tests/test_coverage_actions.py b/netbox_librenms_plugin/tests/test_coverage_actions.py index 0aed82706e..ba91d85204 100644 --- a/netbox_librenms_plugin/tests/test_coverage_actions.py +++ b/netbox_librenms_plugin/tests/test_coverage_actions.py @@ -129,7 +129,7 @@ def test_user_pref_used_when_no_post_get(self): from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences request = _make_request() - with patch("netbox_librenms_plugin.utils.get_user_pref") as mock_pref: + with patch("netbox_librenms_plugin.views.imports.actions.get_user_pref") as mock_pref: mock_pref.return_value = False with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None @@ -140,7 +140,7 @@ def test_settings_fallback_when_no_pref(self): from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences request = _make_request() - with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): + with patch("netbox_librenms_plugin.views.imports.actions.get_user_pref", return_value=None): with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: settings_obj = MagicMock() settings_obj.use_sysname_default = False @@ -154,7 +154,7 @@ def test_no_settings_defaults_to_true_false(self): from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences request = _make_request() - with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): + with patch("netbox_librenms_plugin.views.imports.actions.get_user_pref", return_value=None): with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None use_sysname, strip_domain = _resolve_naming_preferences(request) diff --git a/netbox_librenms_plugin/tests/test_coverage_api2.py b/netbox_librenms_plugin/tests/test_coverage_api2.py index 3fe7aa4005..65260421e6 100644 --- a/netbox_librenms_plugin/tests/test_coverage_api2.py +++ b/netbox_librenms_plugin/tests/test_coverage_api2.py @@ -242,10 +242,11 @@ class _DoesNotExist(Exception): class TestSyncJobStatusRQJobNotInQueue: - """Test sync_job_status when RQ.fetch raises an exception.""" + """Test sync_job_status when RQ.fetch raises NoSuchJobError.""" def test_updates_db_to_failed_when_running_and_not_in_rq(self): from core.choices import JobStatusChoices + from rq.exceptions import NoSuchJobError mock_db_job = MagicMock() mock_db_job.pk = 5 @@ -260,7 +261,7 @@ class _DoesNotExist(Exception): mock_job_cls.objects.get.return_value = mock_db_job mock_rq_cls = MagicMock() - mock_rq_cls.fetch.side_effect = Exception("not found in redis") + mock_rq_cls.fetch.side_effect = NoSuchJobError("not found in redis") mock_queue = MagicMock() mock_queue_fn = MagicMock(return_value=mock_queue) @@ -282,6 +283,7 @@ class _DoesNotExist(Exception): def test_no_change_when_not_running_and_not_in_rq(self): from core.choices import JobStatusChoices + from rq.exceptions import NoSuchJobError mock_db_job = MagicMock() mock_db_job.pk = 6 @@ -296,7 +298,7 @@ class _DoesNotExist(Exception): mock_job_cls.objects.get.return_value = mock_db_job mock_rq_cls = MagicMock() - mock_rq_cls.fetch.side_effect = Exception("not found in redis") + mock_rq_cls.fetch.side_effect = NoSuchJobError("not found in redis") mock_queue = MagicMock() mock_queue_fn = MagicMock(return_value=mock_queue) @@ -316,6 +318,7 @@ class _DoesNotExist(Exception): def test_does_not_overwrite_completed_when_not_in_rq(self): from core.choices import JobStatusChoices + from rq.exceptions import NoSuchJobError mock_db_job = MagicMock() mock_db_job.pk = 7 @@ -330,7 +333,7 @@ class _DoesNotExist(Exception): mock_job_cls.objects.get.return_value = mock_db_job mock_rq_cls = MagicMock() - mock_rq_cls.fetch.side_effect = Exception("gone") + mock_rq_cls.fetch.side_effect = NoSuchJobError("gone") mock_queue = MagicMock() mock_queue_fn = MagicMock(return_value=mock_queue) diff --git a/netbox_librenms_plugin/tests/test_import_utils.py b/netbox_librenms_plugin/tests/test_import_utils.py index e0c7880550..837a585d6b 100644 --- a/netbox_librenms_plugin/tests/test_import_utils.py +++ b/netbox_librenms_plugin/tests/test_import_utils.py @@ -5123,7 +5123,7 @@ def test_user_pref_used_when_no_toggle_in_request(self): from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences request = self._make_request() - with patch("netbox_librenms_plugin.utils.get_user_pref") as mock_pref: + with patch("netbox_librenms_plugin.views.imports.actions.get_user_pref") as mock_pref: mock_pref.side_effect = lambda req, key: False if "use_sysname" in key else True use_sysname, strip_domain = _resolve_naming_preferences(request) assert use_sysname is False diff --git a/netbox_librenms_plugin/tests/test_init.py b/netbox_librenms_plugin/tests/test_init.py index 2be541e200..adf7bc18c7 100644 --- a/netbox_librenms_plugin/tests/test_init.py +++ b/netbox_librenms_plugin/tests/test_init.py @@ -15,10 +15,18 @@ class TestEnsureLibreNMSIdCustomField: """Test _ensure_librenms_id_custom_field signal handler.""" def setup_method(self): - """Reset the _executed flag before each test for consistent isolation.""" + """Reset per-alias execution tracking before each test.""" from netbox_librenms_plugin import _ensure_librenms_id_custom_field - _ensure_librenms_id_custom_field._executed = False + _ensure_librenms_id_custom_field._executed_aliases = set() + + def _setup_cf_mock(self, MockCustomField, mock_cf, created): + """Wire MockCustomField so that objects.using(alias).get_or_create(...) returns (mock_cf, created).""" + MockCustomField.objects.using.return_value.get_or_create.return_value = (mock_cf, created) + + def _setup_ct_mock(self, MockContentType, mock_ct): + """Wire MockContentType so that objects.db_manager(alias).get_for_model(...) returns mock_ct.""" + MockContentType.objects.db_manager.return_value.get_for_model.return_value = mock_ct @patch("dcim.models.Interface", new_callable=MagicMock) @patch("dcim.models.Device", new_callable=MagicMock) @@ -34,16 +42,16 @@ def test_creates_custom_field_when_missing( mock_cf = MagicMock() mock_cf.object_types.values_list.return_value = [] - MockCustomField.objects.get_or_create.return_value = (mock_cf, True) + self._setup_cf_mock(MockCustomField, mock_cf, True) mock_ct = MagicMock() mock_ct.pk = 1 - MockContentType.objects.get_for_model.return_value = mock_ct + self._setup_ct_mock(MockContentType, mock_ct) with patch("logging.getLogger") as mock_get_logger: _ensure_librenms_id_custom_field(sender=None) - MockCustomField.objects.get_or_create.assert_called_once_with( + MockCustomField.objects.using.return_value.get_or_create.assert_called_once_with( name="librenms_id", defaults={ "type": "json", @@ -63,14 +71,14 @@ def test_creates_custom_field_when_missing( mock_get_logger.assert_called_with("netbox_librenms_plugin") def test_skips_when_already_executed(self): - """Handler is a no-op on second invocation (per-migrate dedup).""" + """Handler is a no-op on second invocation for the same DB alias.""" from netbox_librenms_plugin import _ensure_librenms_id_custom_field - _ensure_librenms_id_custom_field._executed = True + _ensure_librenms_id_custom_field._executed_aliases = {"default"} with patch("extras.models.CustomField") as MockCustomField: _ensure_librenms_id_custom_field(sender=None) - MockCustomField.objects.get_or_create.assert_not_called() + MockCustomField.objects.using.assert_not_called() @patch("dcim.models.Interface", new_callable=MagicMock) @patch("dcim.models.Device", new_callable=MagicMock) @@ -86,11 +94,11 @@ def test_existing_field_not_recreated( mock_cf = MagicMock() mock_cf.object_types.values_list.return_value = [1, 2, 3, 4] - MockCustomField.objects.get_or_create.return_value = (mock_cf, False) + self._setup_cf_mock(MockCustomField, mock_cf, False) mock_ct = MagicMock() mock_ct.pk = 1 - MockContentType.objects.get_for_model.return_value = mock_ct + self._setup_ct_mock(MockContentType, mock_ct) _ensure_librenms_id_custom_field(sender=None) @@ -111,13 +119,18 @@ def test_adds_missing_content_types( mock_cf = MagicMock() mock_cf.object_types.values_list.return_value = [1, 2] - MockCustomField.objects.get_or_create.return_value = (mock_cf, False) + self._setup_cf_mock(MockCustomField, mock_cf, False) ct_existing = MagicMock() ct_existing.pk = 1 ct_new = MagicMock() ct_new.pk = 99 - MockContentType.objects.get_for_model.side_effect = [ct_existing, ct_existing, ct_new, ct_new] + MockContentType.objects.db_manager.return_value.get_for_model.side_effect = [ + ct_existing, + ct_existing, + ct_new, + ct_new, + ] _ensure_librenms_id_custom_field(sender=None) @@ -129,7 +142,7 @@ def test_exception_does_not_propagate(self, MockCustomField): """Exceptions during custom field creation are caught and logged.""" from netbox_librenms_plugin import _ensure_librenms_id_custom_field - MockCustomField.objects.get_or_create.side_effect = Exception("DB not ready") + MockCustomField.objects.using.return_value.get_or_create.side_effect = Exception("DB not ready") with patch("logging.getLogger") as mock_get_logger: # Should not raise @@ -155,11 +168,11 @@ def test_no_log_when_field_already_exists( mock_cf = MagicMock() mock_cf.object_types.values_list.return_value = [1, 2, 3, 4] - MockCustomField.objects.get_or_create.return_value = (mock_cf, False) + self._setup_cf_mock(MockCustomField, mock_cf, False) mock_ct = MagicMock() mock_ct.pk = 1 - MockContentType.objects.get_for_model.return_value = mock_ct + self._setup_ct_mock(MockContentType, mock_ct) with patch("logging.getLogger") as mock_get_logger: _ensure_librenms_id_custom_field(sender=None) diff --git a/netbox_librenms_plugin/views/base/cables_view.py b/netbox_librenms_plugin/views/base/cables_view.py index 03732ad70e..a599022b80 100644 --- a/netbox_librenms_plugin/views/base/cables_view.py +++ b/netbox_librenms_plugin/views/base/cables_view.py @@ -75,7 +75,7 @@ def get_ports_data(self, obj): return data def get_links_data(self, obj): - """Fetch links data from LibreNMS, including local_port names for the current request.""" + """Fetch links data from LibreNMS for the device and add local port names.""" self.librenms_id = self.librenms_api.get_librenms_id(obj) success, data = self.librenms_api.get_device_links(self.librenms_id) if not success or "error" in data: @@ -95,17 +95,20 @@ def get_links_data(self, obj): local_ports_map[port_id] = port_name links = data.get("links", []) - return [ - { - "local_port": local_ports_map.get(str(link.get("local_port_id"))), - "local_port_id": link.get("local_port_id"), - "remote_port": link.get("remote_port"), - "remote_device": link.get("remote_hostname"), - "remote_port_id": link.get("remote_port_id"), - "remote_device_id": link.get("remote_device_id"), - } - for link in links - ] + links_data = [] + for link in links: + local_port_name = local_ports_map.get(str(link.get("local_port_id"))) + links_data.append( + { + "local_port": local_port_name, + "local_port_id": link.get("local_port_id"), + "remote_port": link.get("remote_port"), + "remote_device": link.get("remote_hostname"), + "remote_port_id": link.get("remote_port_id"), + "remote_device_id": link.get("remote_device_id"), + } + ) + return links_data def get_device_by_id_or_name(self, remote_device_id, hostname, server_key=None): """Try to find device in NetBox first by librenms_id custom field, then by name""" @@ -304,6 +307,7 @@ def get_table(self, data, obj): def _prepare_context(self, request, obj, fetch_fresh=False): """Helper method to prepare the context data for cable sync views.""" + table = None cache_expiry = None server_key = self.librenms_api.server_key # For VC devices, cache under the sync device's key so SingleCableVerifyView reads the same entry. @@ -357,10 +361,12 @@ def _prepare_context(self, request, obj, fetch_fresh=False): cache_ttl = cache.ttl(cache_key) if cache_ttl is not None and cache_ttl > 0: cache_expiry = timezone.now() + timezone.timedelta(seconds=cache_ttl) - + # Generate the table table = self.get_table(links_data, obj) + table.configure(request) + # Prepare and return the context return { "table": table, "object": obj, @@ -414,7 +420,9 @@ def post(self, request): selected_device_id = data.get("device_id") local_port_id = data.get("local_port_id") # Read server_key from POST so we use the exact server the user was viewing - server_key = data.get("server_key") or self.librenms_api.server_key + server_key = data.get("server_key") + if not server_key: + server_key = self.librenms_api.server_key formatted_row = { "local_port": "", diff --git a/netbox_librenms_plugin/views/imports/actions.py b/netbox_librenms_plugin/views/imports/actions.py index 8adcd025a4..07f2b88f23 100644 --- a/netbox_librenms_plugin/views/imports/actions.py +++ b/netbox_librenms_plugin/views/imports/actions.py @@ -32,11 +32,7 @@ fetch_model_by_id, ) from netbox_librenms_plugin.tables.device_status import DeviceImportTable -from netbox_librenms_plugin.utils import ( - resolve_naming_preferences as _resolve_naming_preferences, - save_user_pref, - set_librenms_device_id, -) +from netbox_librenms_plugin.utils import get_user_pref, save_user_pref, set_librenms_device_id from netbox_librenms_plugin.views.mixins import LibreNMSAPIMixin, LibreNMSPermissionMixin, NetBoxObjectPermissionMixin logger = logging.getLogger(__name__) @@ -44,6 +40,9 @@ # Actions that require the force checkbox when a device-type mismatch is detected. _FORCE_REQUIRED_ACTIONS = frozenset({"link", "update", "update_serial", "update_type"}) +# Actions that operate on Device-only fields and cannot be applied to VMs. +_DEVICE_ONLY_ACTIONS = frozenset({"link", "update", "update_serial", "update_type", "sync_serial", "sync_device_type"}) + def _save_device(device) -> HttpResponse | None: """Call full_clean() then save(). Return an HttpResponse on failure, None on success.""" @@ -61,6 +60,57 @@ def _save_device(device) -> HttpResponse | None: return None +def _resolve_naming_preferences(request) -> tuple[bool, bool]: + """Resolve use_sysname/strip_domain: POST/GET data → user pref → plugin settings.""" + from netbox_librenms_plugin.models import LibreNMSSettings + + settings = None + + # Check POST first (form submissions), then GET (HTMX hx-include on hx-get). + # Support hyphenated ("use-sysname-toggle"), underscored ("use_sysname-toggle"), + # and plain canonical ("use_sysname") key variants for compatibility across + # different form/hidden-input implementations. + _USE_SYSNAME_KEYS = ("use-sysname-toggle", "use_sysname-toggle", "use_sysname") + _STRIP_DOMAIN_KEYS = ("strip-domain-toggle", "strip_domain-toggle", "strip_domain") + _TRUTHY = frozenset({"on", "true", "1"}) + + def _is_truthy(val): + return val.lower() in _TRUTHY if val is not None else False + + _use_sysname_post = next((request.POST.get(k) for k in _USE_SYSNAME_KEYS if k in request.POST), None) + _use_sysname_get = next((request.GET.get(k) for k in _USE_SYSNAME_KEYS if k in request.GET), None) + + if _use_sysname_post is not None: + use_sysname = _is_truthy(_use_sysname_post) + elif _use_sysname_get is not None: + use_sysname = _is_truthy(_use_sysname_get) + else: + pref = get_user_pref(request, "plugins.netbox_librenms_plugin.use_sysname") + if pref is not None: + use_sysname = pref + else: + settings = LibreNMSSettings.objects.first() + use_sysname = getattr(settings, "use_sysname_default", True) if settings else True + + _strip_domain_post = next((request.POST.get(k) for k in _STRIP_DOMAIN_KEYS if k in request.POST), None) + _strip_domain_get = next((request.GET.get(k) for k in _STRIP_DOMAIN_KEYS if k in request.GET), None) + + if _strip_domain_post is not None: + strip_domain = _is_truthy(_strip_domain_post) + elif _strip_domain_get is not None: + strip_domain = _is_truthy(_strip_domain_get) + else: + pref = get_user_pref(request, "plugins.netbox_librenms_plugin.strip_domain") + if pref is not None: + strip_domain = pref + else: + if settings is None: + settings = LibreNMSSettings.objects.first() + strip_domain = getattr(settings, "strip_domain_default", False) if settings else False + + return use_sysname, strip_domain + + def _get_hostname_for_action(request, validation: dict, libre_device: dict) -> str: """ Return the resolved hostname to use when updating a device during a conflict action. @@ -166,7 +216,9 @@ def get_validated_device_with_selections(self, device_id: int, request) -> tuple strip_domain=strip_domain, server_key=self.librenms_api.server_key, ) - validation["import_as_vm"] = is_vm + # Recompute is_vm from validate_device_for_import's own detection + # (it may have found an existing VM via hostname/IP lookup) + is_vm = bool(validation.get("import_as_vm")) # Apply user selections (cluster, role, rack) to validation _apply_user_selections_to_validation(validation, selections, is_vm) @@ -320,6 +372,9 @@ def post(self, request): strip_domain=strip_domain, server_key=self.librenms_api.server_key, ) + # Recompute is_vm from validation result — the function may have + # detected an existing VM via hostname/IP lookup + is_vm = bool(validation.get("import_as_vm")) # Mark validation with VC detection flag for proper URL generation in table # Bulk confirm should respect the initial filter's VC detection preference @@ -407,6 +462,7 @@ def post(self, request): "use_sysname": use_sysname, "strip_domain": strip_domain, "server_key": self.librenms_api.server_key, + "vc_detection_enabled": request.GET.get("enable_vc_detection") == "true", } return render( @@ -464,11 +520,11 @@ def post(self, request): # noqa: PLR0912 - branching keeps responses explicit return HttpResponse("Invalid device identifier", status=400) use_sysname, strip_domain = _resolve_naming_preferences(request) - vc_detection_enabled = request.POST.get("enable_vc_detection") in ("on", "true", "1", "True") sync_options = { "sync_interfaces": request.POST.get("sync_interfaces") == "on", "sync_cables": request.POST.get("sync_cables") == "on", "sync_ips": request.POST.get("sync_ips") == "on", + "vc_detection_enabled": request.POST.get("vc_detection_enabled") in ("on", "true", "1", "True"), "use_sysname": use_sysname, "strip_domain": strip_domain, } @@ -550,7 +606,6 @@ def post(self, request): # noqa: PLR0912 - branching keeps responses explicit vm_imports=vm_imports, server_key=self.librenms_api.server_key, sync_options=sync_options, - vc_detection_enabled=vc_detection_enabled, manual_mappings_per_device=manual_mappings_per_device, libre_devices_cache=libre_devices_cache, ) @@ -613,7 +668,6 @@ def post(self, request): # noqa: PLR0912 - branching keeps responses explicit sync_options=sync_options, manual_mappings_per_device=manual_mappings_per_device, # type: ignore libre_devices_cache=libre_devices_cache_sync, - vc_detection_enabled=vc_detection_enabled, user=request.user, # Pass user for permission checks ) @@ -843,7 +897,7 @@ def _build_sync_info(libre_device, existing_device): device_type_synced = False elif hw_match.get("matched"): librenms_device_type = hw_match["device_type"] - if not netbox_device_type or netbox_device_type.pk != librenms_device_type.pk: + if netbox_device_type is None or netbox_device_type.pk != librenms_device_type.pk: device_type_synced = False else: device_type_synced = False @@ -966,11 +1020,10 @@ def post(self, request, device_id): if not action or not existing_device_id: return HttpResponse("Missing action or existing_device_id", status=400) - # VirtualMachine supports migrate_librenms_id, sync_name, and sync_platform; all - # other actions operate on Device-specific fields (serial, device_type) and remain Device-only. - _VM_SUPPORTED_ACTIONS = frozenset({"migrate_librenms_id", "sync_name", "sync_platform"}) + # VirtualMachine supports migrate_librenms_id, sync_name, and sync_platform. + # Device-only actions (serial, device_type, legacy link/update) are rejected. if existing_device_type == "virtualmachine": - if action not in _VM_SUPPORTED_ACTIONS: + if action in _DEVICE_ONLY_ACTIONS: return HttpResponse( f"Action '{escape(action)}' is not supported for virtual machines", status=400, From 0d68ca63e5bb3560f31bd061041810dd44ab0ab6 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Tue, 31 Mar 2026 23:44:01 +0200 Subject: [PATCH 2/9] fix: import validation, template bugs, pagination fix, doc update - device_operations: skip site/device-type issues for existing devices on re-import - bulk_import_confirm.html: pass vc_detection_enabled through bulk import form - device_validation_details.html: fix existing-device condition and VM model name check - paginator.html: fix table.page.previous/next_page_number (was page.*) - docs/usage_tips/custom_field.md: remove 'Legacy' label, update 0.4.2+ note - test_coverage_base_views.py: add test for non-default server_key forwarded to enrich_links_data --- docs/usage_tips/custom_field.md | 4 +- .../import_utils/device_operations.py | 13 ++++--- .../htmx/bulk_import_confirm.html | 1 + .../htmx/device_validation_details.html | 7 +--- .../netbox_librenms_plugin/inc/paginator.html | 4 +- .../tests/test_coverage_base_views.py | 39 +++++++++++++++++++ 6 files changed, 54 insertions(+), 14 deletions(-) diff --git a/docs/usage_tips/custom_field.md b/docs/usage_tips/custom_field.md index 1474d6826e..4e9951fc7c 100644 --- a/docs/usage_tips/custom_field.md +++ b/docs/usage_tips/custom_field.md @@ -18,10 +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. -## Manual Custom Field Setup (Legacy) +## Manual Custom Field Setup !!! 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. + On 0.4.2+, rerun migrations first (`manage.py migrate`). If you need to recreate the field manually on current releases, use the JSON schema below. Pre-0.4.2 releases used an Integer field — do not use Integer for new entries. Follow these steps to create the `librenms_id` custom field in NetBox: diff --git a/netbox_librenms_plugin/import_utils/device_operations.py b/netbox_librenms_plugin/import_utils/device_operations.py index 2b1e5c416e..152414998b 100644 --- a/netbox_librenms_plugin/import_utils/device_operations.py +++ b/netbox_librenms_plugin/import_utils/device_operations.py @@ -519,7 +519,8 @@ def validate_device_for_import( result["site"] = site_match if not site_match["found"]: - result["issues"].append(f"No matching site found for location: '{location}'") + if not result.get("existing_device"): + result["issues"].append(f"No matching site found for location: '{location}'") # Get alternative suggestions if location: all_sites = Site.objects.all()[:10] # Limit for performance @@ -533,9 +534,10 @@ def validate_device_for_import( result["device_type"]["found"] = False result["device_type"]["device_type"] = None result["device_type"]["match_type"] = "ambiguous" - result["issues"].append( - f"Multiple device types match hardware '{hardware}' — resolve the ambiguity in NetBox." - ) + if not result.get("existing_device"): + result["issues"].append( + f"Multiple device types match hardware '{hardware}' — resolve the ambiguity in NetBox." + ) else: # Chassis inventory fallback: when hardware doesn't match, # try the chassis entPhysicalModelName as an additional lookup source @@ -553,7 +555,8 @@ def validate_device_for_import( if not result["device_type"]["found"] and result["device_type"].get("match_type") != "ambiguous": result["device_type"]["found"] = False - result["issues"].append(f"No matching device type found for hardware: '{hardware}'") + if not result.get("existing_device"): + result["issues"].append(f"No matching device type found for hardware: '{hardware}'") # Get some device types for user to choose from all_device_types = DeviceType.objects.all()[:10] result["device_type"]["suggestions"] = [ diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html index da8ba2ef65..c003201bd6 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html @@ -132,6 +132,7 @@
hx-swap="none"> {% csrf_token %} + {% for entry in devices %} {% if entry.role %}{% endif %} diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html index fbaa3a7597..dc69bcf0ee 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html @@ -135,7 +135,6 @@
{# Show only for non-VM contexts: true when no existing device (new import, not VM) OR when existing device is not a VM. #} {% if not validation.existing_device and not validation.import_as_vm or validation.existing_device and existing_device_model_name != "virtualmachine" %} - {# Site row #} Site @@ -279,7 +278,6 @@
{{ validation.existing_device.platform }} {% if sync_info and not sync_info.platform_synced %} {% if sync_info.platform_info.platform_exists %} - {% if not validation.import_as_vm and not validation.existing_device.cluster %}
@@ -291,7 +289,6 @@
- {% endif %} {% else %} @@ -305,7 +302,7 @@
{% elif sync_info and sync_info.platform_info.platform_exists %} Not set - {% if validation.existing_device and not validation.import_as_vm and not validation.existing_device.cluster %} + {% if validation.existing_device %}
@@ -464,7 +461,7 @@
Import blocked: The incoming serial number is already assigned to another device in NetBox. Resolve the duplicate serial before linking. - {% elif validation.import_as_vm %} + {% elif validation.import_as_vm or existing_device_model_name == "virtualmachine" %}
Hostname match found for a VM — use the import action to proceed. diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/paginator.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/paginator.html index ece9d2c129..306e356cd5 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/paginator.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/paginator.html @@ -9,7 +9,7 @@
    {% if table.page.has_previous %}
  • - +
    • @@ -27,7 +27,7 @@ {% if table.page.has_next %}
  • - +
    • diff --git a/netbox_librenms_plugin/tests/test_coverage_base_views.py b/netbox_librenms_plugin/tests/test_coverage_base_views.py index 2fa16f08a1..94b18d4381 100644 --- a/netbox_librenms_plugin/tests/test_coverage_base_views.py +++ b/netbox_librenms_plugin/tests/test_coverage_base_views.py @@ -613,6 +613,45 @@ def test_cache_hit_re_enriches_and_returns_context(self): assert result is not None assert result["object"] is obj + def test_non_default_server_key_forwarded_to_enrich(self): + """When librenms_api.server_key is non-default, _prepare_context passes it to enrich_links_data.""" + view = self._make_view() + view._librenms_api.server_key = "non-default" + obj = _mock_obj() + + links = [ + { + "local_port": "Gi0/0", + "local_port_id": "1", + "remote_device": None, + "remote_port": None, + "remote_port_id": None, + "remote_device_id": None, + } + ] + mock_table = MagicMock() + mock_table.configure = MagicMock() + + with ( + patch.object(view, "get_links_data", return_value=links), + patch.object(view, "enrich_links_data", return_value=links) as mock_enrich, + patch.object(view, "get_cache_key", return_value="cable-key"), + patch.object(view, "get_table", return_value=mock_table), + patch("netbox_librenms_plugin.views.base.cables_view.get_librenms_sync_device", return_value=None), + patch("netbox_librenms_plugin.views.base.cables_view.cache") as mock_cache, + patch("netbox_librenms_plugin.views.base.cables_view.timezone") as mock_tz, + ): + mock_cache.ttl.return_value = 300 + mock_tz.now.return_value = MagicMock() + mock_tz.timedelta.return_value = MagicMock() + result = view._prepare_context(view.request, obj, fetch_fresh=True) + + assert result is not None + # enrich_links_data must be called with the non-default server_key + mock_enrich.assert_called_once() + _, enrich_kwargs = mock_enrich.call_args + assert enrich_kwargs.get("server_key") == "non-default" + class TestBaseCableTableViewGetContextData: """Tests for BaseCableTableView.get_context_data.""" From 04ad361484e28f76c2f4454d8a5a2b1500d56194 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Wed, 1 Apr 2026 00:26:42 +0200 Subject: [PATCH 3/9] Fix PR #48 review findings - bulk_import: remove change_virtualmachine from required_perms (import only creates VMs, never updates them) - bulk_import: _is_job_cancelled except branch returns False instead of checking DB status (Redis unavailable = not cancelled) - bulk_import: remove now-unused JobStatusChoices import - actions: read vc_detection_enabled from POST first, GET as fallback, with safe empty-string default to avoid AttributeError - test_init: add test for integer->JSON field migration branch - test_import_utils: replace DB-fallback cancellation tests with RQ-unavailable returns-False test - test_vm_operations: update cancellation tests to use patched _is_job_cancelled instead of removed DB-fallback path --- .../import_utils/bulk_import.py | 17 +----- .../tests/test_import_utils.py | 18 ++---- netbox_librenms_plugin/tests/test_init.py | 26 ++++++++ .../tests/test_vm_operations.py | 59 +++++++------------ .../views/imports/actions.py | 5 +- 5 files changed, 61 insertions(+), 64 deletions(-) diff --git a/netbox_librenms_plugin/import_utils/bulk_import.py b/netbox_librenms_plugin/import_utils/bulk_import.py index 8541f6022c..abaa6041bb 100644 --- a/netbox_librenms_plugin/import_utils/bulk_import.py +++ b/netbox_librenms_plugin/import_utils/bulk_import.py @@ -4,7 +4,6 @@ import logging from typing import List -from core.choices import JobStatusChoices from django.core.cache import cache from ..import_validation_helpers import apply_role_to_validation, recalculate_validation_status, remove_validation_issue @@ -38,16 +37,8 @@ def _is_job_cancelled(job) -> bool: rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) return rq_job.is_failed or rq_job.is_stopped except Exception: - job.job.refresh_from_db() - job_status = job.job.status - status_value = job_status.value if hasattr(job_status, "value") else job_status - return status_value in ( - JobStatusChoices.STATUS_FAILED, - JobStatusChoices.STATUS_ERRORED, - "failed", - "errored", - "stopped", - ) + # Redis unavailable — treat as not cancelled to avoid false positives. + return False def bulk_import_devices_shared( @@ -102,13 +93,11 @@ def bulk_import_devices_shared( # 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; change_virtualmachine is - # needed when refreshing an existing VM (e.g. setting librenms_id on a re-import). + # change_device is needed for VC master/member updates. required_perms = [ "dcim.add_device", "dcim.change_device", "virtualization.add_virtualmachine", - "virtualization.change_virtualmachine", ] require_permissions(user, required_perms, "import devices") diff --git a/netbox_librenms_plugin/tests/test_import_utils.py b/netbox_librenms_plugin/tests/test_import_utils.py index 837a585d6b..b9894182d9 100644 --- a/netbox_librenms_plugin/tests/test_import_utils.py +++ b/netbox_librenms_plugin/tests/test_import_utils.py @@ -4724,19 +4724,13 @@ def test_rq_failed_cancels_import_loop(self): assert count == 0 assert result.get("cancelled") is True - def test_rq_unavailable_falls_back_to_db_check(self): - """When RQ is unavailable, DB status check is used as fallback.""" + def test_rq_unavailable_treats_as_not_cancelled(self): + """When RQ is unavailable, _is_job_cancelled returns False so import continues.""" # mock_rq_job=None triggers the side_effect=Exception path - count, result = self._run_bulk_import(mock_rq_job=None, db_status="failed", device_ids=[1]) - # With DB status "failed", import should not run - assert count == 0 - assert result.get("cancelled") is True - - def test_db_errored_status_also_terminates_loop(self): - """When DB job status is 'errored', import loop should terminate early.""" - count, result = self._run_bulk_import(mock_rq_job=None, db_status="errored", device_ids=[1, 2, 3]) - assert count == 0 - assert result.get("cancelled") is True + count, result = self._run_bulk_import(mock_rq_job=None, db_status="failed", device_ids=[1, 2, 3]) + # Redis unavailable → not cancelled → all devices processed + assert count == 3 + assert result.get("cancelled") is False def test_healthy_job_runs_all_devices(self): """When job is healthy, all devices should be imported.""" diff --git a/netbox_librenms_plugin/tests/test_init.py b/netbox_librenms_plugin/tests/test_init.py index adf7bc18c7..efac03069a 100644 --- a/netbox_librenms_plugin/tests/test_init.py +++ b/netbox_librenms_plugin/tests/test_init.py @@ -181,3 +181,29 @@ def test_no_log_when_field_already_exists( # asserting getLogger was never called, which is fragile. logger_instance = mock_get_logger.return_value logger_instance.info.assert_not_called() + + @patch("dcim.models.Interface", new_callable=MagicMock) + @patch("dcim.models.Device", new_callable=MagicMock) + @patch("virtualization.models.VMInterface", new_callable=MagicMock) + @patch("virtualization.models.VirtualMachine", new_callable=MagicMock) + @patch("django.contrib.contenttypes.models.ContentType") + @patch("extras.models.CustomField") + def test_integer_field_migrated_to_json( + self, MockCustomField, MockContentType, mock_vm, mock_vmif, mock_device, mock_iface + ): + """When existing field has type='integer', it is migrated to 'json' and saved.""" + from netbox_librenms_plugin import _ensure_librenms_id_custom_field + + mock_cf = MagicMock() + mock_cf.type = "integer" + mock_cf.object_types.values_list.return_value = [1, 2, 3, 4] + self._setup_cf_mock(MockCustomField, mock_cf, False) + + mock_ct = MagicMock() + mock_ct.pk = 1 + self._setup_ct_mock(MockContentType, mock_ct) + + _ensure_librenms_id_custom_field(sender=None) + + assert mock_cf.type == "json" + mock_cf.save.assert_called_once_with(using="default", update_fields=["type"]) diff --git a/netbox_librenms_plugin/tests/test_vm_operations.py b/netbox_librenms_plugin/tests/test_vm_operations.py index 7aaaf95b09..8eac7d132b 100644 --- a/netbox_librenms_plugin/tests/test_vm_operations.py +++ b/netbox_librenms_plugin/tests/test_vm_operations.py @@ -437,7 +437,7 @@ def test_exception_in_inner_loop_added_to_failed(self): assert "Connection error" in result["failed"][0]["error"] def test_job_cancellation_breaks_loop(self): - """Loop exits early when job status becomes 'failed' at the 5th-iteration check.""" + """Loop exits early when _is_job_cancelled returns True at idx=1 check.""" from netbox_librenms_plugin.import_utils.vm_operations import bulk_import_vms mock_api = MagicMock() @@ -445,22 +445,19 @@ def test_job_cancellation_breaks_loop(self): mock_job = MagicMock() mock_job.logger = MagicMock() - # Start as "running"; switches to "failed" on the 2nd refresh (idx=5 check) - mock_job.job.status = "running" - refresh_calls = [0] - def _refresh(): - refresh_calls[0] += 1 - if refresh_calls[0] >= 2: - mock_job.job.status = "failed" + # 5 VMs; _is_job_cancelled returns False for first check (idx=1), True for second (idx=5) + cancel_calls = [0] - mock_job.job.refresh_from_db.side_effect = _refresh + def _cancelled(job): + cancel_calls[0] += 1 + return cancel_calls[0] >= 2 - # 5 VMs: idx=1 check fires (running → no cancel); idx=5 fires (failed → cancel) vm_imports = {i: {} for i in range(1, 6)} with ( patch("netbox_librenms_plugin.import_utils.vm_operations.require_permissions"), + patch("netbox_librenms_plugin.import_utils.vm_operations._is_job_cancelled", side_effect=_cancelled), patch( "netbox_librenms_plugin.import_utils.vm_operations.fetch_device_with_cache", return_value=None, # VMs 1-4 → failed; VM-5 never reached @@ -472,7 +469,7 @@ def _refresh(): assert len(result["failed"]) == 4 def test_job_cancellation_with_errored_status(self): - """Loop also exits for 'errored' job status.""" + """Loop also exits when _is_job_cancelled returns True (rq_job.is_failed).""" from netbox_librenms_plugin.import_utils.vm_operations import bulk_import_vms mock_api = MagicMock() @@ -480,21 +477,18 @@ def test_job_cancellation_with_errored_status(self): mock_job = MagicMock() mock_job.logger = MagicMock() - # Start as "running"; switches to "errored" on the 2nd refresh (idx=5 check) - mock_job.job.status = "running" - refresh_calls = [0] - def _refresh(): - refresh_calls[0] += 1 - if refresh_calls[0] >= 2: - mock_job.job.status = "errored" + cancel_calls = [0] - mock_job.job.refresh_from_db.side_effect = _refresh + def _cancelled(job): + cancel_calls[0] += 1 + return cancel_calls[0] >= 2 vm_imports = {i: {} for i in range(1, 6)} with ( patch("netbox_librenms_plugin.import_utils.vm_operations.require_permissions"), + patch("netbox_librenms_plugin.import_utils.vm_operations._is_job_cancelled", side_effect=_cancelled), patch( "netbox_librenms_plugin.import_utils.vm_operations.fetch_device_with_cache", return_value=None, @@ -607,7 +601,7 @@ def test_no_cluster_id_skips_cluster_lookup(self): mock_apply_cluster.assert_not_called() def test_job_status_value_attribute_used_when_present(self): - """Status enum .value is used for cancellation check when present.""" + """Redis unavailable (exception in _is_job_cancelled) returns False so loop continues.""" from netbox_librenms_plugin.import_utils.vm_operations import bulk_import_vms mock_api = MagicMock() @@ -615,26 +609,16 @@ def test_job_status_value_attribute_used_when_present(self): mock_job = MagicMock() mock_job.logger = MagicMock() - # Status object with .value attribute (simulates Django choices enum) - # Initially "running", switches to "failed" on 2nd refresh (idx=5 check) - mock_running = MagicMock() - mock_running.value = "running" - mock_failed = MagicMock() - mock_failed.value = "failed" - mock_job.job.status = mock_running - refresh_calls = [0] - def _refresh(): - refresh_calls[0] += 1 - if refresh_calls[0] >= 2: - mock_job.job.status = mock_failed - - mock_job.job.refresh_from_db.side_effect = _refresh - - vm_imports = {i: {} for i in range(1, 6)} + vm_imports = {i: {} for i in range(1, 3)} with ( patch("netbox_librenms_plugin.import_utils.vm_operations.require_permissions"), + # Simulate Redis unavailable → _is_job_cancelled returns False (not cancelled) + patch( + "netbox_librenms_plugin.import_utils.vm_operations._is_job_cancelled", + return_value=False, + ), patch( "netbox_librenms_plugin.import_utils.vm_operations.fetch_device_with_cache", return_value=None, @@ -642,7 +626,8 @@ def _refresh(): ): result = bulk_import_vms(vm_imports, mock_api, job=mock_job) - assert len(result["failed"]) == 4 + # Both VMs should be attempted (not cancelled) → both failed (fetch returned None) + assert len(result["failed"]) == 2 def test_job_log_info_when_not_cancelled_at_checkpoint(self): """log.info is called at a non-cancelling 5-iteration checkpoint.""" diff --git a/netbox_librenms_plugin/views/imports/actions.py b/netbox_librenms_plugin/views/imports/actions.py index 07f2b88f23..7db616dac0 100644 --- a/netbox_librenms_plugin/views/imports/actions.py +++ b/netbox_librenms_plugin/views/imports/actions.py @@ -462,7 +462,10 @@ def post(self, request): "use_sysname": use_sysname, "strip_domain": strip_domain, "server_key": self.librenms_api.server_key, - "vc_detection_enabled": request.GET.get("enable_vc_detection") == "true", + "vc_detection_enabled": ( + request.POST.get("enable_vc_detection") or request.GET.get("enable_vc_detection") or "" + ).lower() + in ("on", "true", "1"), } return render( From b1341ac22dbf9e694b9989b61c3dde8e52f14a07 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Wed, 1 Apr 2026 00:27:34 +0200 Subject: [PATCH 4/9] docs: add test_init.py to testing.md --- docs/development/testing.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/development/testing.md b/docs/development/testing.md index ca861ea502..1e1290a25f 100644 --- a/docs/development/testing.md +++ b/docs/development/testing.md @@ -52,6 +52,7 @@ The test suite covers all major plugin functionality. Tests are organized by the | [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_init.py](../../netbox_librenms_plugin/tests/test_init.py) | Plugin startup—`_ensure_librenms_id_custom_field` creation, type migration, and multi-DB alias handling | | [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 | From a2fbfc25f13348713f6e90995f80e0c43e234a0e Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Wed, 1 Apr 2026 08:20:21 +0200 Subject: [PATCH 5/9] Fix stale docstring, test name, and duplicate function - bulk_import: fix _is_job_cancelled docstring to reflect that Redis-unavailable returns False with no DB fallback - test_vm_operations: rename test_job_status_value_attribute_used_when_present to test_is_job_cancelled_false_processes_all_vms - actions: remove local _resolve_naming_preferences duplicate; import and use the shared resolve_naming_preferences from utils instead - test_coverage_actions, test_import_utils: update patch targets and inline imports to reference utils.resolve_naming_preferences and utils.get_user_pref (no longer in actions module) --- .../import_utils/bulk_import.py | 5 +- .../tests/test_coverage_actions.py | 96 +++++++++---------- .../tests/test_import_utils.py | 38 ++++---- .../tests/test_vm_operations.py | 4 +- .../views/imports/actions.py | 63 ++---------- 5 files changed, 78 insertions(+), 128 deletions(-) diff --git a/netbox_librenms_plugin/import_utils/bulk_import.py b/netbox_librenms_plugin/import_utils/bulk_import.py index abaa6041bb..6679a9e7b2 100644 --- a/netbox_librenms_plugin/import_utils/bulk_import.py +++ b/netbox_librenms_plugin/import_utils/bulk_import.py @@ -26,8 +26,9 @@ def _is_job_cancelled(job) -> bool: """ Return True if a background job has been stopped or cancelled. - Checks RQ/Redis state first (reflects stop API calls immediately); - falls back to the database status if Redis is unavailable. + Checks RQ/Redis state only (reflects stop API calls immediately). + On any Redis/redis-py exception (e.g., connection refused), returns False + immediately without any database fallback, to avoid false cancellation. """ try: from django_rq import get_queue diff --git a/netbox_librenms_plugin/tests/test_coverage_actions.py b/netbox_librenms_plugin/tests/test_coverage_actions.py index ba91d85204..7a7fef0423 100644 --- a/netbox_librenms_plugin/tests/test_coverage_actions.py +++ b/netbox_librenms_plugin/tests/test_coverage_actions.py @@ -82,93 +82,93 @@ def test_success_returns_none(self): class TestResolveNamingPreferences: - """Tests for _resolve_naming_preferences (lines 60-106).""" + """Tests for resolve_naming_preferences (utils.resolve_naming_preferences).""" def test_post_use_sysname_toggle_truthy(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request(post={"use-sysname-toggle": "on"}) with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) assert use_sysname is True def test_post_use_sysname_underscored_key(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request(post={"use_sysname-toggle": "on"}) with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None - use_sysname, _ = _resolve_naming_preferences(request) + use_sysname, _ = resolve_naming_preferences(request) assert use_sysname is True def test_post_use_sysname_plain_key(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request(post={"use_sysname": "true"}) with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None - use_sysname, _ = _resolve_naming_preferences(request) + use_sysname, _ = resolve_naming_preferences(request) assert use_sysname is True def test_get_fallback_when_no_post(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request(get={"use_sysname": "on"}) request.POST = {} with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None - use_sysname, _ = _resolve_naming_preferences(request) + use_sysname, _ = resolve_naming_preferences(request) assert use_sysname is True def test_user_pref_used_when_no_post_get(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request() - with patch("netbox_librenms_plugin.views.imports.actions.get_user_pref") as mock_pref: + with patch("netbox_librenms_plugin.utils.get_user_pref") as mock_pref: mock_pref.return_value = False with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None - use_sysname, _ = _resolve_naming_preferences(request) + use_sysname, _ = resolve_naming_preferences(request) assert use_sysname is False def test_settings_fallback_when_no_pref(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request() - with patch("netbox_librenms_plugin.views.imports.actions.get_user_pref", return_value=None): + with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: settings_obj = MagicMock() settings_obj.use_sysname_default = False settings_obj.strip_domain_default = True MockSettings.objects.first.return_value = settings_obj - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) assert use_sysname is False assert strip_domain is True def test_no_settings_defaults_to_true_false(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request() - with patch("netbox_librenms_plugin.views.imports.actions.get_user_pref", return_value=None): + with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) assert use_sysname is True assert strip_domain is False def test_strip_domain_post_toggle(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request(post={"strip-domain-toggle": "on"}) with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None - _, strip_domain = _resolve_naming_preferences(request) + _, strip_domain = resolve_naming_preferences(request) assert strip_domain is True @@ -203,7 +203,7 @@ def test_invalid_device_id_skipped(self): view = self._make_view() with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch("netbox_librenms_plugin.views.imports.actions.fetch_device_with_cache", return_value=None): request = _make_request(post={"select": ["not-an-int"]}) @@ -215,7 +215,7 @@ def test_all_cache_expired_returns_400_with_expiry_message(self): view = self._make_view() with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch("netbox_librenms_plugin.views.imports.actions.fetch_device_with_cache", return_value=None): request = _make_request(post={"select": ["1", "2"]}) @@ -237,7 +237,7 @@ def test_valid_devices_renders_confirm_template(self, mock_render): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.fetch_device_with_cache", return_value=libre_device @@ -342,7 +342,7 @@ def test_get_validated_device_returns_data_when_found(self): return_value={"cluster_id": None, "role_id": None, "rack_id": None}, ): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False), ): with patch( @@ -407,7 +407,7 @@ def test_get_with_existing_device_adds_sync_info(self, mock_render): with patch.object(view, "get_validated_device_with_selections", return_value=(libre_device, validation, {})): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch.object(view, "_build_sync_info", return_value={"serial_synced": True}): with patch.object(view, "_build_id_server_info", return_value=None): @@ -908,25 +908,25 @@ def test_serial_empty_treated_as_not_set(self): class TestResolveTruthyPreferences: - """Tests for _resolve_naming_preferences truthy parsing via integration.""" + """Tests for resolve_naming_preferences truthy parsing via integration.""" def test_on_value_resolves_to_true(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request(post={"use_sysname": "on", "strip_domain": "on"}) with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) assert use_sysname is True assert strip_domain is True def test_false_value_resolves_to_false(self): - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = _make_request(post={"use_sysname": "false", "strip_domain": "0"}) with patch("netbox_librenms_plugin.models.LibreNMSSettings", create=True) as MockSettings: MockSettings.objects.first.return_value = None - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) assert use_sysname is False assert strip_domain is False @@ -1318,7 +1318,7 @@ def test_duplicate_device_id_is_skipped(self): return_value=validation, ): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False), ): with patch( @@ -1343,7 +1343,7 @@ def test_device_not_in_cache_adds_error(self): "netbox_librenms_plugin.views.imports.actions.fetch_device_with_cache", return_value=None ): # Not in cache with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False), ): with patch( @@ -1386,7 +1386,7 @@ def test_vc_stack_updates_suggested_names(self): return_value=validation, ): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False), ): with patch( @@ -2182,7 +2182,7 @@ def test_vm_with_cluster_and_role_applies_both(self): return_value=validation, ): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False), ): with patch( @@ -2234,7 +2234,7 @@ def test_device_with_role_and_rack_applies_both(self): return_value=validation, ): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False), ): with patch( @@ -3297,7 +3297,7 @@ def fetch_side_effect(device_id, *args, **kwargs): return_value=validation, ): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False), ): with patch( @@ -3359,7 +3359,7 @@ def test_sync_mode_import_runs(self): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", return_value=import_result @@ -3430,7 +3430,7 @@ def test_invalid_cluster_value_logs_warning(self): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3469,7 +3469,7 @@ def get_side_effect(k, d=None): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3507,7 +3507,7 @@ def get_side_effect(k, d=None): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3540,7 +3540,7 @@ def test_import_with_success_messages(self): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3583,7 +3583,7 @@ def get_side_effect(k, d=None): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3619,7 +3619,7 @@ def test_htmx_request_returns_oob_rows(self): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3672,7 +3672,7 @@ def test_permission_denied_during_import_redirects(self): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3699,7 +3699,7 @@ def test_background_no_workers_falls_back_to_sync(self): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3753,7 +3753,7 @@ def get_side_effect(k, d=None): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3790,7 +3790,7 @@ def test_permission_denied_htmx_returns_htmx_redirect(self): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch( "netbox_librenms_plugin.views.imports.actions.bulk_import_devices", @@ -3823,7 +3823,7 @@ def get_side_effect(k, d=None): with patch.object(view, "require_write_permission", return_value=None): with patch( - "netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences", return_value=(True, False) + "netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences", return_value=(True, False) ): with patch("utilities.rqworker.get_workers_for_queue", return_value=2): with patch( diff --git a/netbox_librenms_plugin/tests/test_import_utils.py b/netbox_librenms_plugin/tests/test_import_utils.py index b9894182d9..c8ba3ca50b 100644 --- a/netbox_librenms_plugin/tests/test_import_utils.py +++ b/netbox_librenms_plugin/tests/test_import_utils.py @@ -2567,7 +2567,7 @@ def _create_request(self, action, existing_device_id, use_sysname=False, strip_d """ request = MagicMock() request.user.has_perm.return_value = True - # Always include both toggles so _resolve_naming_preferences never falls through + # Always include both toggles so resolve_naming_preferences never falls through # to the user-pref/settings DB path, which would hit the real database. post_data = { "action": action, @@ -5049,7 +5049,7 @@ def test_falls_back_to_determine_device_name(self): validation = {} # no resolved_name libre_device = {"hostname": "host.example.com", "sysName": "host"} - with patch("netbox_librenms_plugin.views.imports.actions._resolve_naming_preferences") as mock_prefs: + with patch("netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences") as mock_prefs: mock_prefs.return_value = (False, False) # use_sysname=False, strip_domain=False with patch("netbox_librenms_plugin.views.imports.actions._determine_device_name") as mock_name: mock_name.return_value = "host.example.com" @@ -5061,12 +5061,12 @@ def test_falls_back_to_determine_device_name(self): # --------------------------------------------------------------------------- -# Tests for _resolve_naming_preferences underscore-variant key support +# Tests for resolve_naming_preferences underscore-variant key support # --------------------------------------------------------------------------- class TestResolveNamingPreferencesKeys: - """Test that _resolve_naming_preferences handles both hyphenated and underscored keys.""" + """Test that resolve_naming_preferences handles both hyphenated and underscored keys.""" def _make_request(self, post=None, get=None): from unittest.mock import MagicMock @@ -5080,11 +5080,11 @@ def _make_request(self, post=None, get=None): def test_hyphenated_post_key_use_sysname(self): from unittest.mock import patch - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = self._make_request(post={"use-sysname-toggle": "on", "strip-domain-toggle": "off"}) with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) assert use_sysname is True assert strip_domain is False @@ -5092,70 +5092,70 @@ def test_underscored_post_key_use_sysname(self): """Underscore variant 'use_sysname-toggle' should also be recognised.""" from unittest.mock import patch - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = self._make_request(post={"use_sysname-toggle": "on", "strip_domain-toggle": "on"}) with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) assert use_sysname is True assert strip_domain is True def test_get_key_used_when_not_in_post(self): from unittest.mock import patch - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = self._make_request(get={"use-sysname-toggle": "off", "strip-domain-toggle": "on"}) with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) assert use_sysname is False assert strip_domain is True def test_user_pref_used_when_no_toggle_in_request(self): from unittest.mock import patch - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = self._make_request() - with patch("netbox_librenms_plugin.views.imports.actions.get_user_pref") as mock_pref: + with patch("netbox_librenms_plugin.utils.get_user_pref") as mock_pref: mock_pref.side_effect = lambda req, key: False if "use_sysname" in key else True - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) assert use_sysname is False assert strip_domain is True def test_post_takes_precedence_over_user_pref(self): from unittest.mock import patch - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences request = self._make_request(post={"use-sysname-toggle": "off"}) # user_pref would say True — POST should win with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=True): - use_sysname, _ = _resolve_naming_preferences(request) + use_sysname, _ = resolve_naming_preferences(request) assert use_sysname is False def test_truthy_string_true_value(self): """'true' and '1' (in addition to 'on') should be treated as True.""" from unittest.mock import patch - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences for truthy_val in ("true", "True", "TRUE", "1"): request = self._make_request(post={"use-sysname-toggle": truthy_val, "strip-domain-toggle": "off"}) with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): - use_sysname, _ = _resolve_naming_preferences(request) + use_sysname, _ = resolve_naming_preferences(request) assert use_sysname is True, f"Expected True for value {truthy_val!r}" def test_falsy_string_false_value(self): """Unrecognised strings should be treated as False.""" from unittest.mock import patch - from netbox_librenms_plugin.views.imports.actions import _resolve_naming_preferences + from netbox_librenms_plugin.utils import resolve_naming_preferences for falsy_val in ("off", "false", "0", "", "no"): request = self._make_request(post={"use-sysname-toggle": falsy_val, "strip-domain-toggle": "off"}) with patch("netbox_librenms_plugin.utils.get_user_pref", return_value=None): - use_sysname, _ = _resolve_naming_preferences(request) + use_sysname, _ = resolve_naming_preferences(request) assert use_sysname is False, f"Expected False for value {falsy_val!r}" diff --git a/netbox_librenms_plugin/tests/test_vm_operations.py b/netbox_librenms_plugin/tests/test_vm_operations.py index 8eac7d132b..2000d00413 100644 --- a/netbox_librenms_plugin/tests/test_vm_operations.py +++ b/netbox_librenms_plugin/tests/test_vm_operations.py @@ -600,8 +600,8 @@ def test_no_cluster_id_skips_cluster_lookup(self): mock_cluster_cls.objects.filter.assert_not_called() mock_apply_cluster.assert_not_called() - def test_job_status_value_attribute_used_when_present(self): - """Redis unavailable (exception in _is_job_cancelled) returns False so loop continues.""" + def test_is_job_cancelled_false_processes_all_vms(self): + """_is_job_cancelled returning False lets loop process all VMs.""" from netbox_librenms_plugin.import_utils.vm_operations import bulk_import_vms mock_api = MagicMock() diff --git a/netbox_librenms_plugin/views/imports/actions.py b/netbox_librenms_plugin/views/imports/actions.py index 7db616dac0..8b569664ce 100644 --- a/netbox_librenms_plugin/views/imports/actions.py +++ b/netbox_librenms_plugin/views/imports/actions.py @@ -32,7 +32,7 @@ fetch_model_by_id, ) from netbox_librenms_plugin.tables.device_status import DeviceImportTable -from netbox_librenms_plugin.utils import get_user_pref, save_user_pref, set_librenms_device_id +from netbox_librenms_plugin.utils import resolve_naming_preferences, save_user_pref, set_librenms_device_id from netbox_librenms_plugin.views.mixins import LibreNMSAPIMixin, LibreNMSPermissionMixin, NetBoxObjectPermissionMixin logger = logging.getLogger(__name__) @@ -60,57 +60,6 @@ def _save_device(device) -> HttpResponse | None: return None -def _resolve_naming_preferences(request) -> tuple[bool, bool]: - """Resolve use_sysname/strip_domain: POST/GET data → user pref → plugin settings.""" - from netbox_librenms_plugin.models import LibreNMSSettings - - settings = None - - # Check POST first (form submissions), then GET (HTMX hx-include on hx-get). - # Support hyphenated ("use-sysname-toggle"), underscored ("use_sysname-toggle"), - # and plain canonical ("use_sysname") key variants for compatibility across - # different form/hidden-input implementations. - _USE_SYSNAME_KEYS = ("use-sysname-toggle", "use_sysname-toggle", "use_sysname") - _STRIP_DOMAIN_KEYS = ("strip-domain-toggle", "strip_domain-toggle", "strip_domain") - _TRUTHY = frozenset({"on", "true", "1"}) - - def _is_truthy(val): - return val.lower() in _TRUTHY if val is not None else False - - _use_sysname_post = next((request.POST.get(k) for k in _USE_SYSNAME_KEYS if k in request.POST), None) - _use_sysname_get = next((request.GET.get(k) for k in _USE_SYSNAME_KEYS if k in request.GET), None) - - if _use_sysname_post is not None: - use_sysname = _is_truthy(_use_sysname_post) - elif _use_sysname_get is not None: - use_sysname = _is_truthy(_use_sysname_get) - else: - pref = get_user_pref(request, "plugins.netbox_librenms_plugin.use_sysname") - if pref is not None: - use_sysname = pref - else: - settings = LibreNMSSettings.objects.first() - use_sysname = getattr(settings, "use_sysname_default", True) if settings else True - - _strip_domain_post = next((request.POST.get(k) for k in _STRIP_DOMAIN_KEYS if k in request.POST), None) - _strip_domain_get = next((request.GET.get(k) for k in _STRIP_DOMAIN_KEYS if k in request.GET), None) - - if _strip_domain_post is not None: - strip_domain = _is_truthy(_strip_domain_post) - elif _strip_domain_get is not None: - strip_domain = _is_truthy(_strip_domain_get) - else: - pref = get_user_pref(request, "plugins.netbox_librenms_plugin.strip_domain") - if pref is not None: - strip_domain = pref - else: - if settings is None: - settings = LibreNMSSettings.objects.first() - strip_domain = getattr(settings, "strip_domain_default", False) if settings else False - - return use_sysname, strip_domain - - def _get_hostname_for_action(request, validation: dict, libre_device: dict) -> str: """ Return the resolved hostname to use when updating a device during a conflict action. @@ -122,7 +71,7 @@ def _get_hostname_for_action(request, validation: dict, libre_device: dict) -> s resolved = validation.get("resolved_name") if resolved: return resolved - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) return _determine_device_name(libre_device, use_sysname=use_sysname, strip_domain=strip_domain) @@ -205,7 +154,7 @@ def get_validated_device_with_selections(self, device_id: int, request) -> tuple enable_vc = not is_vm and self._should_enable_vc_detection(device_id, request) # Extract naming preferences: POST data (hx-include) → user pref → plugin settings. - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) validation = validate_device_for_import( libre_device, @@ -329,7 +278,7 @@ def post(self, request): status=400, ) - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) devices = [] errors = [] @@ -522,7 +471,7 @@ def post(self, request): # noqa: PLR0912 - branching keeps responses explicit messages.error(request, "Invalid device identifier supplied") return HttpResponse("Invalid device identifier", status=400) - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) sync_options = { "sync_interfaces": request.POST.get("sync_interfaces") == "on", "sync_cables": request.POST.get("sync_cables") == "on", @@ -830,7 +779,7 @@ def get(self, request, device_id): status=404, ) - use_sysname, strip_domain = _resolve_naming_preferences(request) + use_sysname, strip_domain = resolve_naming_preferences(request) context = { "libre_device": libre_device, From 73c8bc97a6b6bd7102146803699cc368ab77118f Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Wed, 1 Apr 2026 08:40:47 +0200 Subject: [PATCH 6/9] Fix vc_detection_enabled default in validate_device_for_import call When sync_options is None or missing the key, default to False (off) rather than True so omitted options don't silently enable VC detection. --- netbox_librenms_plugin/import_utils/bulk_import.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netbox_librenms_plugin/import_utils/bulk_import.py b/netbox_librenms_plugin/import_utils/bulk_import.py index 6679a9e7b2..e01159c7dd 100644 --- a/netbox_librenms_plugin/import_utils/bulk_import.py +++ b/netbox_librenms_plugin/import_utils/bulk_import.py @@ -145,7 +145,7 @@ def bulk_import_devices_shared( validation = validate_device_for_import( libre_device, api=api, - include_vc_detection=(sync_options.get("vc_detection_enabled", True) if sync_options else True), + include_vc_detection=bool(sync_options and sync_options.get("vc_detection_enabled", False)), use_sysname=use_sysname_opt, strip_domain=strip_domain_opt, server_key=api.server_key, From bbfe3edcde0b45c80cd6496b8cdaeb256a0124ea Mon Sep 17 00:00:00 2001 From: Marcin Zieba <49913098+marcinpsk@users.noreply.github.com> Date: Wed, 1 Apr 2026 13:36:55 +0200 Subject: [PATCH 7/9] Update custom_field.md --- docs/usage_tips/custom_field.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage_tips/custom_field.md b/docs/usage_tips/custom_field.md index 4e9951fc7c..c0328734ee 100644 --- a/docs/usage_tips/custom_field.md +++ b/docs/usage_tips/custom_field.md @@ -21,7 +21,7 @@ For the Interface object, the plugin will automatically populate the LibreNMS ID ## Manual Custom Field Setup !!! note - On 0.4.2+, rerun migrations first (`manage.py migrate`). If you need to recreate the field manually on current releases, use the JSON schema below. Pre-0.4.2 releases used an Integer field — do not use Integer for new entries. + On 0.4.3+, rerun migrations first (`manage.py migrate`). If you need to recreate the field manually on current releases, use the JSON schema below. Pre-0.4.2 releases used an Integer field — do not use Integer for new entries. Follow these steps to create the `librenms_id` custom field in NetBox: From 878f5b1fe0bc8b6133915bac379ee33ef753584f Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Tue, 14 Apr 2026 09:29:07 +0200 Subject: [PATCH 8/9] Address PR #259 review comments Catch specific Redis/RQ exceptions in _is_job_cancelled and log unexpected ones, so real bugs aren't masked. Document vc_detection_enabled in ImportDevicesJob.run sync_options docstring. --- .../import_utils/bulk_import.py | 16 ++++++++++------ netbox_librenms_plugin/jobs.py | 3 ++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/netbox_librenms_plugin/import_utils/bulk_import.py b/netbox_librenms_plugin/import_utils/bulk_import.py index e01159c7dd..af51d6e23a 100644 --- a/netbox_librenms_plugin/import_utils/bulk_import.py +++ b/netbox_librenms_plugin/import_utils/bulk_import.py @@ -27,18 +27,22 @@ def _is_job_cancelled(job) -> bool: Return True if a background job has been stopped or cancelled. Checks RQ/Redis state only (reflects stop API calls immediately). - On any Redis/redis-py exception (e.g., connection refused), returns False - immediately without any database fallback, to avoid false cancellation. + On Redis connectivity issues or a missing RQ job, returns False to avoid + false cancellation. Unexpected exceptions are logged and also return False. """ - try: - from django_rq import get_queue - from rq.job import Job as RQJob + from django_rq import get_queue + from redis.exceptions import RedisError + from rq.exceptions import NoSuchJobError + from rq.job import Job as RQJob + try: queue = get_queue("default") rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) return rq_job.is_failed or rq_job.is_stopped + except (RedisError, NoSuchJobError): + return False except Exception: - # Redis unavailable — treat as not cancelled to avoid false positives. + logger.warning("Unexpected error checking RQ job cancellation state", exc_info=True) return False diff --git a/netbox_librenms_plugin/jobs.py b/netbox_librenms_plugin/jobs.py index 781deb28c2..678a367824 100644 --- a/netbox_librenms_plugin/jobs.py +++ b/netbox_librenms_plugin/jobs.py @@ -177,7 +177,8 @@ def run( device_ids: List of LibreNMS device IDs to import as Devices vm_imports: Dict mapping device_id to cluster/role info for VM imports server_key: Optional LibreNMS server key for multi-server setups - sync_options: Dict with sync_interfaces, sync_cables, sync_ips, use_sysname, strip_domain + sync_options: Dict with sync_interfaces, sync_cables, sync_ips, + use_sysname, strip_domain, and vc_detection_enabled manual_mappings_per_device: Dict mapping device_id to manual_mappings dict libre_devices_cache: Optional dict mapping device_id to pre-fetched device data **kwargs: Additional job parameters From d41ad0b1875e33d817db5c3bba5de0108fecacdc Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Tue, 14 Apr 2026 11:23:52 +0200 Subject: [PATCH 9/9] Address follow-up PR #259 review findings - bulk_import_confirm.html: delegate the background-job checkbox change listener on document so it keeps working after HTMX re-renders the modal. - test_init.py: exercise the per-alias path by migrating an integer field on a non-default DB alias and assert .using/.db_manager/save use it. - test_vm_operations.py: capture the _is_job_cancelled patch and assert it was invoked, guarding the cancellation check from accidental removal. - views/imports/actions.py: normalize vc_detection_enabled once and reuse it for validate_device_for_import, validation["_vc_detection_enabled"], and the template context so the confirm modal matches the final import. --- .../htmx/bulk_import_confirm.html | 19 +++++++++++-------- netbox_librenms_plugin/tests/test_init.py | 13 ++++++++++--- .../tests/test_vm_operations.py | 5 ++++- .../views/imports/actions.py | 13 ++++++------- 4 files changed, 31 insertions(+), 19 deletions(-) diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html index c003201bd6..349080eb71 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html @@ -163,6 +163,8 @@
    window.__bulkConfirmHandlersAttached = true; const formId = 'bulk-import-confirm-form'; + const checkboxId = 'use-background-job-checkbox'; + const hiddenId = 'use-background-job-hidden'; function toggleButtonState(isLoading) { const form = document.getElementById(formId); @@ -184,14 +186,15 @@
    } } - // Sync checkbox with hidden input - const checkbox = document.getElementById('use-background-job-checkbox'); - const hiddenInput = document.getElementById('use-background-job-hidden'); - if (checkbox && hiddenInput) { - checkbox.addEventListener('change', function() { - hiddenInput.value = checkbox.checked ? 'on' : 'off'; - }); - } + // Delegated so it survives HTMX re-renders of the modal fragment. + document.addEventListener('change', function(event) { + if (event.target && event.target.id === checkboxId) { + const hiddenInput = document.getElementById(hiddenId); + if (hiddenInput) { + hiddenInput.value = event.target.checked ? 'on' : 'off'; + } + } + }); document.addEventListener('htmx:beforeRequest', function(event) { if (event.target && event.target.id === formId) { diff --git a/netbox_librenms_plugin/tests/test_init.py b/netbox_librenms_plugin/tests/test_init.py index efac03069a..fe2274fc95 100644 --- a/netbox_librenms_plugin/tests/test_init.py +++ b/netbox_librenms_plugin/tests/test_init.py @@ -191,9 +191,11 @@ def test_no_log_when_field_already_exists( def test_integer_field_migrated_to_json( self, MockCustomField, MockContentType, mock_vm, mock_vmif, mock_device, mock_iface ): - """When existing field has type='integer', it is migrated to 'json' and saved.""" + """When existing field has type='integer', it is migrated to 'json' and saved on the given alias.""" from netbox_librenms_plugin import _ensure_librenms_id_custom_field + db_alias = "other" + mock_cf = MagicMock() mock_cf.type = "integer" mock_cf.object_types.values_list.return_value = [1, 2, 3, 4] @@ -203,7 +205,12 @@ def test_integer_field_migrated_to_json( mock_ct.pk = 1 self._setup_ct_mock(MockContentType, mock_ct) - _ensure_librenms_id_custom_field(sender=None) + _ensure_librenms_id_custom_field(sender=None, using=db_alias) assert mock_cf.type == "json" - mock_cf.save.assert_called_once_with(using="default", update_fields=["type"]) + # Alias must flow through both the CustomField lookup and the ContentType lookup, + # exercising the per-alias guard path (_executed_aliases). + MockCustomField.objects.using.assert_called_with(db_alias) + MockContentType.objects.db_manager.assert_called_with(db_alias) + mock_cf.save.assert_called_once_with(using=db_alias, update_fields=["type"]) + assert db_alias in _ensure_librenms_id_custom_field._executed_aliases diff --git a/netbox_librenms_plugin/tests/test_vm_operations.py b/netbox_librenms_plugin/tests/test_vm_operations.py index 2000d00413..6a64950e89 100644 --- a/netbox_librenms_plugin/tests/test_vm_operations.py +++ b/netbox_librenms_plugin/tests/test_vm_operations.py @@ -618,7 +618,7 @@ def test_is_job_cancelled_false_processes_all_vms(self): patch( "netbox_librenms_plugin.import_utils.vm_operations._is_job_cancelled", return_value=False, - ), + ) as mock_is_job_cancelled, patch( "netbox_librenms_plugin.import_utils.vm_operations.fetch_device_with_cache", return_value=None, @@ -626,6 +626,9 @@ def test_is_job_cancelled_false_processes_all_vms(self): ): result = bulk_import_vms(vm_imports, mock_api, job=mock_job) + # The cancellation helper must be consulted at least once (idx==1 checkpoint); + # protects against accidental removal of the check. + mock_is_job_cancelled.assert_called_once_with(mock_job) # Both VMs should be attempted (not cancelled) → both failed (fetch returned None) assert len(result["failed"]) == 2 diff --git a/netbox_librenms_plugin/views/imports/actions.py b/netbox_librenms_plugin/views/imports/actions.py index 8b569664ce..5f98473c1c 100644 --- a/netbox_librenms_plugin/views/imports/actions.py +++ b/netbox_librenms_plugin/views/imports/actions.py @@ -279,6 +279,9 @@ def post(self, request): ) use_sysname, strip_domain = resolve_naming_preferences(request) + vc_detection_enabled = ( + request.POST.get("enable_vc_detection") or request.GET.get("enable_vc_detection") or "" + ).lower() in ("on", "true", "1") devices = [] errors = [] @@ -320,15 +323,14 @@ def post(self, request): use_sysname=use_sysname, strip_domain=strip_domain, server_key=self.librenms_api.server_key, + include_vc_detection=vc_detection_enabled, ) # Recompute is_vm from validation result — the function may have # detected an existing VM via hostname/IP lookup is_vm = bool(validation.get("import_as_vm")) # Mark validation with VC detection flag for proper URL generation in table - # Bulk confirm should respect the initial filter's VC detection preference - vc_requested = request.GET.get("enable_vc_detection") == "true" - validation["_vc_detection_enabled"] = vc_requested + validation["_vc_detection_enabled"] = vc_detection_enabled device_name = validation.get("resolved_name") or f"device-{device_id}" @@ -411,10 +413,7 @@ def post(self, request): "use_sysname": use_sysname, "strip_domain": strip_domain, "server_key": self.librenms_api.server_key, - "vc_detection_enabled": ( - request.POST.get("enable_vc_detection") or request.GET.get("enable_vc_detection") or "" - ).lower() - in ("on", "true", "1"), + "vc_detection_enabled": vc_detection_enabled, } return render(