fix: code quality improvements and bug fixes - #48
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughPer-DB-alias custom-field bootstrapping with integer→json migration; centralized RQ-based job-cancellation helper used across bulk device/VM import flows; removed standalone vc_detection parameter in favor of sync_options; tightened API job lookup/error handling; assorted template, view, JS, and test updates to align with these changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant JobRunner
participant ImportUtils
participant RQ
participant Database
User->>JobRunner: enqueue ImportDevicesJob(sync_options)
JobRunner->>ImportUtils: run bulk_import_devices_shared(job, sync_options)
loop per checkpoint (first and every 5th)
ImportUtils->>RQ: fetch job (_is_job_cancelled)
alt RQ job found
RQ-->>ImportUtils: job state (failed/errored/stopped/completed)
ImportUtils->>ImportUtils: if cancelled -> break loop
else NoSuchJobError / RQ error
RQ-->>ImportUtils: NoSuchJobError / error
ImportUtils->>Database: (optional) inspect DB Job.status
Database-->>ImportUtils: job.status
ImportUtils->>ImportUtils: decide cancelled? (default: not cancelled)
end
end
ImportUtils->>Database: persist per-device import results
JobRunner-->>User: job finished / status updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/tests/test_init.py (1)
89-107: 🧹 Nitpick | 🔵 TrivialAdd a dedicated test for the integer→JSON migration branch.
The new behavior in
_ensure_librenms_id_custom_field(cf.type == "integer"→cf.save(using=alias, update_fields=["type"])) is not directly asserted yet. A targeted test will prevent regressions in the migration path.Proposed test addition
+ `@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_migrates_integer_field_to_json( + self, MockCustomField, MockContentType, mock_vm, mock_vmif, mock_device, mock_iface + ): + 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, using="default") + + mock_cf.save.assert_called_once_with(using="default", update_fields=["type"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_init.py` around lines 89 - 107, Add a test that covers the integer→JSON migration path in _ensure_librenms_id_custom_field by creating a MockCustomField instance (mock_cf) with mock_cf.type set to "integer" and arranging mocks like in existing tests (MockContentType/MockCustomField); call _ensure_librenms_id_custom_field(sender=None) and assert that mock_cf.save was called with the DB alias and update_fields=["type"] (e.g., mock_cf.save.assert_called_with(using=alias, update_fields=["type"])) to ensure the migration branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 103-112: The required_perms list in bulk_import_devices_shared()
incorrectly includes "virtualization.change_virtualmachine"; remove that entry
and update the surrounding comment so the permission check only requires
"virtualization.add_virtualmachine" (since the import only creates VMs and never
updates them), ensuring the permission enforcement uses the updated
required_perms symbol and does not block device-only imports that lack VM edit
rights.
- Around line 26-50: The cancellation logic in _is_job_cancelled should rely
solely on RQ/Redis flags and not fall back to the database status; update the
function to import get_queue and RQ Job via RQJob.fetch(str(job.job.job_id),
connection=queue.connection) and return rq_job.is_failed or rq_job.is_stopped,
and in the except/Redis-unavailable path do NOT inspect job.job.status or
refresh_from_db—instead return False (i.e., treat as not-cancelled) or re-raise
the connection-related error so database status is never used for cancellation;
ensure references to _is_job_cancelled, get_queue, RQJob.fetch, and
rq_job.is_failed/is_stopped are used to locate the change.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Line 465: The code currently reads vc_detection_enabled from request.GET which
loses the POSTed checkbox state from BulkImportConfirmView.post(); change the
logic that builds the payload (the "vc_detection_enabled" value) to read and
persist the flag from the POSTed form (use
request.POST.get("enable_vc_detection") or equivalent) when present (falling
back to GET only if POST is absent) so BulkImportDevicesView receives the actual
user selection; update the location where "vc_detection_enabled" is set and any
passing of that value between BulkImportConfirmView.post() and
BulkImportDevicesView to ensure the POSTed value is carried through.
---
Outside diff comments:
In `@netbox_librenms_plugin/tests/test_init.py`:
- Around line 89-107: Add a test that covers the integer→JSON migration path in
_ensure_librenms_id_custom_field by creating a MockCustomField instance
(mock_cf) with mock_cf.type set to "integer" and arranging mocks like in
existing tests (MockContentType/MockCustomField); call
_ensure_librenms_id_custom_field(sender=None) and assert that mock_cf.save was
called with the DB alias and update_fields=["type"] (e.g.,
mock_cf.save.assert_called_with(using=alias, update_fields=["type"])) to ensure
the migration branch is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cae0749-5e90-4f59-9f3f-fe7b4f5252a5
📒 Files selected for processing (17)
docs/usage_tips/custom_field.mdnetbox_librenms_plugin/__init__.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/paginator.htmlnetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_api2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_init.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/imports/actions.py
💤 Files with no reviewable changes (1)
- netbox_librenms_plugin/jobs.py
marcinpsk
added a commit
that referenced
this pull request
Mar 31, 2026
- 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
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 29-31: The docstring in
netbox_librenms_plugin/import_utils/bulk_import.py incorrectly claims the
function "falls back to the database status if Redis is unavailable"; update the
docstring to state the actual behavior: the function checks RQ/Redis state first
and, on Redis/Redis-py exceptions, returns False immediately (no DB fallback).
Locate the function whose docstring starts with "Checks RQ/Redis state first..."
and edit that text to accurately describe the exception path and return value.
In `@netbox_librenms_plugin/tests/test_vm_operations.py`:
- Around line 603-630: Rename the misleading test function
test_job_status_value_attribute_used_when_present to a name that reflects its
behavior (e.g., test_redis_unavailable_continues_loop or
test_is_job_cancelled_false_processes_all_vms) and update any references; the
test exercises bulk_import_vms with _is_job_cancelled patched to return False to
ensure the loop continues and both VMs are attempted, so ensure the new name
mentions Redis/unavailable or _is_job_cancelled returning False and adjust the
test function definition accordingly.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 63-111: Replace the duplicated local function
_resolve_naming_preferences with a call to the shared implementation in utils:
import resolve_naming_preferences from netbox_librenms_plugin.utils at the top
of the module, remove the entire local _resolve_naming_preferences definition,
and update any internal references to call resolve_naming_preferences(request)
(preserving its tuple return of (use_sysname, strip_domain)); ensure imports and
any variable names match the utils function signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 227a88d9-3fcf-4b04-abdf-37f549d53850
📒 Files selected for processing (6)
docs/development/testing.mdnetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_init.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/imports/actions.py
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
netbox_librenms_plugin/views/imports/actions.py (1)
281-282:⚠️ Potential issue | 🟠 MajorPass the parsed VC flag into confirm-time validation.
The new
vc_detection_enabledvalue is only computed for the template context.validate_device_for_import()has already run above with its defaultinclude_vc_detection=True, so the confirm modal still performs VC lookups and can show stack-specific validation even when the user filtered with VC detection off.validation["_vc_detection_enabled"]should come from the same parsed boolean.🔧 Suggested fix
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 = [] seen_ids = set() cache_expired_count = 0 @@ validation = validate_device_for_import( libre_device, import_as_vm=is_vm, api=self.librenms_api, + include_vc_detection=vc_detection_enabled, use_sysname=use_sysname, strip_domain=strip_domain, server_key=self.librenms_api.server_key, ) @@ - vc_requested = request.GET.get("enable_vc_detection") == "true" - validation["_vc_detection_enabled"] = vc_requested + validation["_vc_detection_enabled"] = vc_detection_enabled @@ context = { "devices": devices, "device_count": len(devices), "errors": errors, "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, }Also applies to: 316-323, 414-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/imports/actions.py` around lines 281 - 282, The confirm-time validation still uses the default include_vc_detection=True instead of the parsed VC flag; update the confirm-path to use the parsed boolean (vc_detection_enabled) by passing include_vc_detection=vc_detection_enabled into validate_device_for_import and by setting validation["_vc_detection_enabled"] = vc_detection_enabled (the same parsed value you compute for the template context). Apply the same change in the other confirm-time blocks referenced (around the validate_device_for_import calls at the 316-323 and 414-417 regions) so confirm modal validation and template context use the identical vc_detection_enabled flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 145-149: The current call to validate_device_for_import sets
include_vc_detection to True when sync_options is None or empty; change it so a
missing vc_detection_enabled flag is treated as off. Update the
include_vc_detection expression in the validate_device_for_import call (and any
similar calls in bulk_import_devices / bulk_import_devices_shared) to only be
True if sync_options exists and explicitly contains a truthy
"vc_detection_enabled" (e.g. include_vc_detection = bool(sync_options and
sync_options.get("vc_detection_enabled", False)) or check for the key presence),
so default/omitted sync_options behaves as disabled.
---
Duplicate comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 281-282: The confirm-time validation still uses the default
include_vc_detection=True instead of the parsed VC flag; update the confirm-path
to use the parsed boolean (vc_detection_enabled) by passing
include_vc_detection=vc_detection_enabled into validate_device_for_import and by
setting validation["_vc_detection_enabled"] = vc_detection_enabled (the same
parsed value you compute for the template context). Apply the same change in the
other confirm-time blocks referenced (around the validate_device_for_import
calls at the 316-323 and 414-417 regions) so confirm modal validation and
template context use the identical vc_detection_enabled flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb63cc65-9a9c-4582-8829-425b20119fb3
📒 Files selected for processing (5)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/imports/actions.py
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- __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
- 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
- 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
- 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)
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.
marcinpsk
force-pushed
the
pr/code-quality-fixes
branch
from
April 1, 2026 12:14
b0e610f to
bbfe3ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
netbox_librenms_plugin/views/imports/actions.py (1)
324-326:⚠️ Potential issue | 🟠 MajorReuse the normalized VC flag for confirm-time validation.
BulkImportConfirmView.post()now computesvc_detection_enabledfor the template context, but Line 330 still derivesvalidation["_vc_detection_enabled"]fromrequest.GETonly, and thevalidate_device_for_import(...)call above still omitsinclude_vc_detection. In the common POST path, the confirm modal can therefore render rows without the same VC state that the final import uses.🔧 Suggested fix
- use_sysname, strip_domain = resolve_naming_preferences(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") + ) @@ validation = validate_device_for_import( libre_device, import_as_vm=is_vm, api=self.librenms_api, + include_vc_detection=vc_detection_enabled, use_sysname=use_sysname, strip_domain=strip_domain, server_key=self.librenms_api.server_key, ) @@ - vc_requested = request.GET.get("enable_vc_detection") == "true" - validation["_vc_detection_enabled"] = vc_requested + validation["_vc_detection_enabled"] = vc_detection_enabled @@ - "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,Also applies to: 414-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/imports/actions.py` around lines 324 - 326, BulkImportConfirmView.post computes vc_detection_enabled for the template but validation["_vc_detection_enabled"] and the validate_device_for_import(...) calls still read from request.GET only; fix by reusing the normalized vc_detection_enabled: set validation["_vc_detection_enabled"] = vc_detection_enabled and pass include_vc_detection=vc_detection_enabled into each validate_device_for_import(...) call (the earlier call that currently omits include_vc_detection and the later call around the other confirm-path block that mirrors lines ~414-417) so the confirm modal uses the same VC detection state as the final import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/__init__.py`:
- Around line 107-114: The startup migration only converts cf.type == "integer"
but must handle any legacy non-JSON types; update the conditional in the
migration block that references cf and created to check cf.type != "json"
instead of cf.type == "integer", then call cf.save(using=db_alias,
update_fields=["type"]) and update the
logging.getLogger("netbox_librenms_plugin").info message to indicate a non-JSON
-> json migration (include cf.type if desired before changing it) so any
manually recreated "text" or other legacy types are fixed on post_migrate.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.html`:
- Line 135: The change listener for the background-job checkbox is currently
guarded by window.__bulkConfirmHandlersAttached, making it fire only once and
breaking after HTMX re-renders the modal; remove that one-shot guard and rebind
the listener every time the fragment is rendered (or use event delegation) so
that `#use-background-job-checkbox` change always updates
`#use-background-job-hidden` (and any related inputs like vc_detection_enabled)
after each HTMX replace; target the DOM elements by their IDs
(`#use-background-job-checkbox`, `#use-background-job-hidden`) and ensure you either
attach the handler unconditionally on render or safely remove and re-add the
listener rather than relying on window.__bulkConfirmHandlersAttached.
In `@netbox_librenms_plugin/tests/test_init.py`:
- Around line 191-209: Update the test_integer_field_migrated_to_json test to
exercise a non-default DB alias: call _ensure_librenms_id_custom_field with
sender=None and a non-default alias string (e.g. "other") so the per-alias guard
(_executed_aliases) path is exercised, mock the ORM lookups to expect
.objects.using(db_alias) and .objects.db_manager(db_alias) calls on
MockCustomField/MockContentType, and assert mock_cf.save was called with
using=db_alias and update_fields=["type"] instead of hard-coding "default";
ensure the test verifies the alias flows through
MockCustomField.objects.using/.db_manager and the save(using=...) call.
In `@netbox_librenms_plugin/tests/test_vm_operations.py`:
- Around line 603-630: Update the test_is_job_cancelled_false_processes_all_vms
test to explicitly verify that _is_job_cancelled was invoked: when patching
netbox_librenms_plugin.import_utils.vm_operations._is_job_cancelled, capture the
patch as a mock (e.g., mock_is_job_cancelled) with return_value=False and after
calling bulk_import_vms assert
mock_is_job_cancelled.assert_called_once_with(mock_job); this ensures the helper
(_is_job_cancelled) is actually consulted during bulk_import_vms execution and
protects against accidental removal of the cancellation check.
---
Duplicate comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 324-326: BulkImportConfirmView.post computes vc_detection_enabled
for the template but validation["_vc_detection_enabled"] and the
validate_device_for_import(...) calls still read from request.GET only; fix by
reusing the normalized vc_detection_enabled: set
validation["_vc_detection_enabled"] = vc_detection_enabled and pass
include_vc_detection=vc_detection_enabled into each
validate_device_for_import(...) call (the earlier call that currently omits
include_vc_detection and the later call around the other confirm-path block that
mirrors lines ~414-417) so the confirm modal uses the same VC detection state as
the final import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: e797966c-f9f4-4d41-9eae-98c32aaa82ac
📒 Files selected for processing (20)
docs/development/testing.mddocs/usage_tips/custom_field.mdnetbox_librenms_plugin/__init__.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_confirm.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/paginator.htmlnetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_api2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_init.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/imports/actions.py
💤 Files with no reviewable changes (1)
- netbox_librenms_plugin/jobs.py
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.
- 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.
This was referenced May 5, 2026
Closed
This was referenced May 16, 2026
Merged
Merged
Closed
Merged
This was referenced Jun 21, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bug fixes and code quality improvements to the import pipeline, job cancellation, API views, and templates. No new features or schema changes. Addresses issues found during review after the multi-server merge.
Motivation / Problem
_is_job_cancelledfell back to DB status when Redis was unavailable, which could produce false cancellationsvc_detection_enableddefaulted toTruewhensync_optionswas absent, silently enabling VC detection on all importschange_virtualmachineincorrectly included in required permissions — import only creates VMs, never updates themBulkImportDevicesViewreadvc_detection_enabledfrom GET params instead of the POSTed confirmation formpage.*instead oftable.page.*)device_validation_details.htmlhad wrong condition for existing-device check and VM model name__init__.pystartup migration forlibrenms_idcustom field didn't use per-alias DB routing (multi-DB setups)_is_job_cancelledextracted as a shared helper (was duplicated betweenbulk_importandvm_operations)_resolve_naming_preferencesduplicate inactions.pyremoved; now imports sharedresolve_naming_preferencesfromutilsapi/views.pynow sourced fromFilterDevicesJob/ImportDevicesJobMetarather than hardcoded stringsNoSuchJobErrorcaught explicitly in API views instead of bareExceptionScope of Change
How Was This Tested?
test_import_utils,test_vm_operations), permission checks, integer→JSON migration path (test_init), naming preference resolution (test_coverage_actions), and base view server key forwarding (test_coverage_base_views)Risk Assessment
vc_detection_enablednow defaults to off (was on) whensync_optionsis missing — safer, but callers that relied on the previous default will no longer get VC detection unless they explicitly pass the flagchange_virtualmachinefrom required permissions allows users with onlyadd_virtualmachineto run imports (correct behaviour)Backwards Compatibility
vc_detection_enableddefault flip is a behavioural change in the internal import call chain, not in the user-facing UI flowOther Notes
__init__.pynow tracks which DB aliases have already run the startup migration via_executed_aliasesto avoid duplicate execution in multi-DB setups — reviewers should verify theusing(db_alias)call pattern is correct for their NetBox deploymentSummary by CodeRabbit
Release Notes
Bug Fixes
Documentation
Improvements