Feat/oob sync - #78
Merged
Merged
Conversation
Add inventory/modules sync functionality: - Six mapping model types: DeviceTypeMapping, ModuleTypeMapping, ModuleBayMapping, NormalizationRule, InventoryIgnoreRule, PlatformMapping - Migration 0010 creating all mapping tables - Modules sync tab on Device/VM detail pages with ENTITY-MIB inventory data - Install, replace, and move module actions - Mapping CRUD views with YAML bulk export for all mapping models - LibreNMS API: get_device_transceivers() for transceiver data - Platform matching: PlatformMapping lookup before name-exact match - contrib/ YAML files with example mappings and rules - Comprehensive test coverage (test_sync_modules, test_modules_view, test_module_replace, test_platform_mapping, test_tables_modules)
… behavior _is_job_cancelled now returns False on RQ/Redis unavailability instead of falling back to DB status. Update 5 tests that were asserting the old DB-fallback behavior: - test_db_fallback_logs_via_module_logger_when_job_logger_none → test_rq_unavailable_does_not_cancel_import: RQ unavailable means processing continues, not cancelled - test_job_db_fallback_stopped/errored_before_validation_loop → test_job_cancelled_before_validation_loop_returns_empty / test_rq_unavailable_job_not_cancelled_in_preloop: patch _is_job_cancelled directly to test early-exit behavior - test_job_validation_loop_db_fallback_stop → test_job_cancelled_in_validation_loop_returns_empty: use _is_job_cancelled side_effect to simulate mid-loop cancellation - test_job_rq_check_exception_uses_db_status_and_exits → test_rq_fetch_exception_does_not_cancel_process_filters: assert result has 1 device (not []) when RQ unavailable
- Remove dead _resolve_naming_preferences from actions.py (never called) - Unify vc_detection_enabled parsing in BulkImportConfirmView (POST+GET) - Add ambiguity detection to get_module_types_indexed second loop - Fix get_validated_device_cache_key doctest (wrong e3b0 hash) - Add status=='ok' envelope check before transceivers shape validation - Strip netbox_bay_name in ModuleBayMapping.clean() - Use server_info.server_key in _module_sync.html refresh form - Guard module_mismatch_modal Update Serial form on serial_conflict/installed - Remove duplicate NoSuchJobError import in test_coverage_api2.py - Fix _is_job_cancelled side_effect count for in-loop cancellation test - Precompute sibling_counts in _build_table_rows to eliminate N+1 queries - Accept sibling_counts in has_nested_name_conflict (DB fallback preserved) - Update test_has_installable_children fake_build_row signature - Coerce entPhysicalParentRelPos to int in siblings sort (prevent TypeError) - Move get_queue inside try block in api/views.py sync_job_status - Replace MD5 with SHA256 for VC domain fingerprint (FIPS compliance) - Fix test_role_is_read_from_validation to test validation dict path - Add test_module_replace.py to docs/development/testing.md - Add platform_mappings.yaml row to contrib/README.md - Fix QSFP28-DD-2X100G-LR4 typo in module_type_mappings.yaml - Clarify librenms_id auto-create in docs/usage_tips/custom_field.md
- Unify librenms_id auto-create version reference to 0.4.3 in docs
- Move _LIBRENMS_JOB_NAMES to module-level constant in api/views.py
- Add status=='ok' envelope check in port handler in librenms_api.py
- Split mapping ambiguity tracking in get_module_types_indexed (separate
mapping_seen/mapping_ambiguous so explicit mappings always win over base
ModuleType ambiguity)
- Handle match_type=='ambiguous' in find_matching_platform caller with
dedicated warning message about conflicting platform mappings
- Move vc_requested parse before validate_device_for_import in
BulkImportConfirmView and pass include_vc_detection=vc_requested
- Guard Replace Module form with {% if installed_module %} in
module_mismatch_modal.html to prevent empty module_id submission
- Add mock_db_job.status/completed assertions to api2 test
- Extend cancellation test side_effects to 4 entries to target in-loop
check (lines 507, 534, 566, 574) in both RQ and _is_job_cancelled tests
- Assert success list in test_no_warning_when_cluster_found
…t, module bay normalization, BulkExportYAMLView permissions - test_coverage_sync_views2: patch get_object_or_404 to isolate from ORM - test_coverage_device_fields: assert save/full_clean not called on no-match - test_librenms_id: use exact tuple set comparison for Q branch children - test_init: assert DB alias used in custom field creation - utils.py: move PlatformMapping import to function scope (lazy); update all test patches to models.PlatformMapping; remove create=True - test_platform_mapping: patch require_object_permissions instead of require_write_permission - test_sync_modules: mock apply_normalization_rules in _match_module_bay tests - views/base/modules_view.py: apply NormalizationRule(scope=module_bay) to candidate names in _match_module_bay - views/mapping_views.py: BulkExportYAMLView uses NetBoxObjectPermissionMixin with view permission - views/object_sync/devices.py: precompute has_write_permission once in get_table()
- utils.py: add prefetch_related('interfacetemplates') to ModuleType query
and prefetch_related('netbox_module_type__interfacetemplates') to
ModuleTypeMapping query in get_module_types_indexed() to avoid N+1
queries in has_nested_name_conflict()
- views/mapping_views.py: add .order_by('pk') to BulkExportYAMLView filter
for deterministic YAML export; add select_related() to all 4 export
subclass querysets (DeviceTypeMapping, ModuleTypeMapping,
NormalizationRule, PlatformMapping)
- tests/test_utils.py: assert exact Platform.objects.get kwargs in 2 platform
tests; fix vc.master = None -> vc.master = master to exercise designated-
master-without-IP branch
- tests/test_platform_mapping.py: update mock chain for .order_by(); complete
test_returns_yaml_content_type with actual view call and content-type assertion
- tests/test_sync_modules.py: fix mock chain for .prefetch_related() in
TestGetModuleTypesIndexed
- utils.py: fix apply_normalization_rules() else-branch to filter manufacturer__isnull=True so callers without manufacturer context never have vendor-specific rules applied to their values - tests/test_sync_modules.py: add test_mapping_overrides_ambiguous_base_key to lock down the separate-ambiguous-sets behaviour in get_module_types_indexed(); fix test_regex_mapping_with_backreference to use 'Optics 0/0/0/5' (with space) so the assertion cannot pass via exact-name fallback — only the regex expansion path can produce a match - tests/test_platform_mapping.py: remove dangling assertions accidentally left inside test_returns_200_with_empty_selection (PlatformMapping import and existence check now live in test_all_mapping_bulk_export_yaml_views_exist)
- utils.py: add preload_normalization_rules() helper that preloads NormalizationRule rows for a (scope, manufacturer) combination into a dict keyed by (scope, manufacturer_pk_or_None); update apply_normalization_rules() to accept preloaded_rules kwarg and use preloaded lists when provided (skipping DB queries); update resolve_module_type() to accept norm_rules kwarg and thread it through to apply_normalization_rules — eliminates N+1 DB queries in _match_module_bay and _build_row loops - views/base/modules_view.py: call preload_normalization_rules() in _build_context for both 'module_bay' and 'module_type' scopes; pass preloaded rules via self._norm_rules_bay/_norm_rules_type to _match_module_bay and _build_row respectively - tests/test_platform_mapping.py: add test_multiple_platform_mappings_returns_ambiguous asserting PlatformMapping.MultipleObjectsReturned yields match_type='ambiguous' and that Platform.objects.get is never called - tests/test_sync_modules.py: fix test_mapping_overrides_ambiguous_base_key to use distinct mapping key 'SFP-1G-LX-EXPLICIT' so 'SFP-1G-LX' is absent and only the explicit key is present; fix test_uninstalled_bay_is_skipped to add grandparent bay with installed module (pk=99) and assert walk continues past empty bay to return 99; fix test_class_scoped_mapping_preferred to pass [m_generic, m_class] so priority logic must actively prefer class-scoped mapping; patch preload_normalization_rules in tests that call _build_context directly; update apply_normalization_rules lambda patches to accept **kw
- _fpc_slot_matches: convert match.group(1) and parent_bay.position to int before comparing (Python 3: '1' == 1 is False); handle ValueError with safe fallbacks - apply_normalization_rules docstring: clarify that manufacturer=None applies only unscoped (manufacturer__isnull=True) rules, not all scope rules - test_modules_view._run_build_context: patch load_bay_mappings and get_enabled_ignore_rules at utils level instead of patching model classes that _build_context never references directly; remove now-unused mock_ignore_qs variable
…match, transceiver ignore - actions.py sync_platform: add explicit elif for match_type='ambiguous' so users get a clear conflict message instead of generic 'not found' error when multiple PlatformMapping rows match the same OS string (works towards #51) - utils.py apply_normalization_rules: when preloaded_rules is provided, check key presence before using the dict; fall back to DB query when (scope, mfg_pk) or (scope, None) is absent, preventing silent omission of vendor rules for manufacturers not included in the preloaded dict - utils.py match_librenms_hardware_to_device_type: update Returns docstring to dict | None and document the MultipleObjectsReturned → None case - modules_view.py _apply_installed_status: drop nb_serial from the guard so a module with a LibreNMS serial is flagged as Serial Mismatch even when NetBox has no serial recorded (lnms_serial and lnms_serial != nb_serial) - modules_view.py _collect_top_items: apply ignore-rule check to transceiver- synthesised items before appending, so InventoryIgnoreRules can suppress optics from get_device_transceivers() - test_modules_view.py: rename test that expected the old nb_serial-required behavior; update assertions to Serial Mismatch + can_update_serial + can_replace - test_modules_view.py: wrap two early-return _detect_serial_conflicts tests in patch(dcim.models.Module) and assert filter was never called
…t, vc flag case - utils.py find_by_librenms_id: strip whitespace and canonicalize leading-zero string IDs before building Q filters; '042' and '42 ' now resolve to int_value=42 / canonical_str='42' and both forms are added to the query so they match records stored as numeric 42 or string '42' - modules_view.py _find_parent_container_name: use (... or '') pattern to guard against entPhysicalName being explicitly None in the ENTITY-MIB payload - modules_view.py _match_module_bay: same None guard for entPhysicalName, entPhysicalDescr, entPhysicalClass on the item dict - modules_view.py _collect_top_items: treat 'transparent' the same as 'skip' for transceiver-synthesised rows so transparent synthetic items are not added to top_items - actions.py BulkImportDevicesView: normalise vc_detection_enabled flag with .lower() before membership test so 'ON', 'True', 'TRUE' all parse correctly, consistent with BulkImportConfirmView
- bulk_import: check cancellation every iteration (not every 5th) - models: always call full_clean() on save, remove update_fields guard - tables/modules: add has_write_permission param to LibreNMSModuleTable, gate selection column and render_actions on it - modules_view: normalize placeholder model/serial strings in transceiver merge; filter placeholder serials from inv_serials set - api/views: skip DB status update when job is already in a terminal state - utils: add 'ambiguous' case to find_matching_platform docstring - utils: memoize DB fallback into preloaded_rules in apply_normalization_rules - utils: use try/except int() instead of isdigit() for +42 style IDs - devices: pass has_write_permission to LibreNMSModuleTable constructor - test_modules_view: use SimpleNamespace instead of MagicMock in _determine_status tests for unambiguous truthiness checks - test_sync_modules: assert checkbox HTML in selection cell; update test_install_module_view_not_in_base to assert public import path; add order-independent check for class-scoped mapping preference - test_tables_modules: set has_write_permission=True in _make_table helper; add no-write-permission test case - test_utils: assert exact kwargs on DeviceTypeMapping.objects.get and PlatformMapping.objects.get calls - test_vm_operations: patch _is_job_cancelled directly instead of mutating job.job.status via refresh_from_db side_effect - test_coverage_base_views2: rename test to reflect actual behavior (cache entry present but lacks port_id) - test_coverage_devices: update constructor assertion to include has_write_permission kwarg
- models: add FullCleanOnSaveMixin + clean() to InterfaceTypeMapping to enforce uniqueness for NULL-speed rows (SQL UNIQUE skips NULL=NULL) - utils: expand match_librenms_hardware_to_device_type docstring to document all three fail-closed None cases (mapping, part_number, model MultipleObjectsReturned), not just the mapping-table one - test_vm_operations: remove stale mock_job.job.status='running' from _run_bulk_with_mappings helper; patch _is_job_cancelled=False instead
…o_kbps docstring - DeviceTypeMapping.clean() and PlatformMapping.clean() now lowercase the stored value after stripping, preventing case-variant duplicates (e.g. 'IOS' and 'ios') that would cause MultipleObjectsReturned on __iexact lookups. Closes #51. - Fix convert_speed_to_kbps docstring: Returns section now reads 'int | None' to match the signature and implementation. - Add TestDeviceTypeMappingModel tests for DeviceTypeMapping.clean() (strip, lowercase, blank validation). - Add test_clean_normalizes_to_lowercase and test_clean_strips_and_lowercases to TestPlatformMappingModel.
- hideModal: remove all backdrops via querySelectorAll+forEach - htmx:afterSettle: derive label from aria-labelledby, fallback to id - initializeVlanModalSave: truncate/extract error body before display
… issues - utils.py: guard unsaved manufacturer (pk=None) in preload_normalization_rules and apply_normalization_rules to prevent ValueError on DB query - librenms_sync.js: add id="htmx-modal-label" to module mismatch modal header so aria-labelledby target is preserved after innerHTML replacement - librenms_sync.js: check response.ok before response.json() in deleteUrl fetch so HTTP errors surface their status instead of a parse error
…ener - utils.py: fix convert_speed_to_kbps parameter annotation to int|None - utils.py: guard non-positive numeric librenms_id (<=0) same as None; skip Q canonicalization for string '0'/'-1' after int parse - librenms_sync.js: extract updateHtmxModalLabel(), listen at document level for htmx:afterSettle, call from module-replace fetch completion
- modules_view.py: _apply_installed_status and _detect_serial_conflicts now use _PLACEHOLDER_VALUES set instead of only guarding against "-" so serials like 'unknown', 'n/a', 'na' are treated as absent - utils.py: update convert_speed_to_kbps Args docstring to int|None
…, ChainMap bay lookup
- device_fields.py: distinguish None (ambiguous) from failed match result;
surface a specific error for duplicate DeviceType mappings
- utils.py: find_matching_platform returns {found:False, match_type='ambiguous'}
on Platform.MultipleObjectsReturned instead of silently taking .first()
- modules_view.py: invalidate stale inventory cache before early-return renders
when librenms_id is falsy or inventory fetch fails
- modules_view.py: _lookup_regex_bay_mapping iterates all ChainMap scopes
so same-named bays in different scopes are all checked
- tests: update TestFindMatchingPlatformMultipleReturned to expect ambiguous
Move inline set literals SKIP_TYPES and _NON_HARDWARE_CLASSES out of their method bodies and into module scope, consistent with _PLACEHOLDER_VALUES and other module-level constants. Rename SKIP_TYPES to _SKIP_TRANSCEIVER_TYPES to clarify its domain.
…_id guard - utils.py: broaden find_matching_platform docstring to state 'ambiguous' applies both to multiple PlatformMapping entries and to duplicate exact-name Platform rows (Platform.MultipleObjectsReturned) - utils.py: add early-return guard in find_by_librenms_id for string librenms_id values that parse to <= 0 (e.g. '0', '-1', '000'), preventing them from reaching the Q clauses and matching stale/corrupted records
_check_ignore_rules: normalize item_serial, device_serial, and ancestor_serial against _PLACEHOLDER_VALUES so sentinels like 'unknown'/'n/a' are treated as absent and do not trigger or short-circuit serial_matches_device rules or require_serial_match_parent ancestor walks. _merge_transceiver_data: normalize txr_type against _PLACEHOLDER_VALUES (same as model/serial) so placeholder types like 'unknown' do not bypass the _SKIP_TRANSCEIVER_TYPES guard and produce synthetic rows with display_model set to a placeholder string.
… exact-bay fallback, has_write_permission in HTMX render - utils.py: treat whitespace-only librenms_id strings as absent (return None after strip() when cleaned == "") so they don't reach the Q-object builder - modules_view.py: extend transceiver backfill to also replace 'BUILTIN' model and serial values, not just those already in _PLACEHOLDER_VALUES - modules_view.py: exact-name bay fallback now iterates ChainMap scopes and calls _fpc_slot_matches() to avoid returning the wrong-scope bay when duplicate bay names exist across scopes (mirrors the regex path behaviour) - modules_view.py: add has_write_permission to all render() calls in post() so the Install Selected button is visible in HTMX-refreshed content
…xact-mapping ChainMap scope - utils.py: non-integer strings (e.g. 'abc') now return None in find_by_librenms_id() instead of falling through to build Q objects; changed 'except ValueError: pass' to 'except ValueError: return None' - modules_view.py: get_context_data() re-validates the device's current LibreNMS ID before serving cached inventory; clears cache and returns empty context when the mapping has been removed since the cache was written - modules_view.py: _lookup_exact_bay_mapping() now iterates ChainMap scopes and calls _fpc_slot_matches() before returning, matching the existing behaviour of _lookup_regex_bay_mapping() and the name-fallback path
…_id in inventory cache
- modules_view: lowercase _GENERIC_CONTAINER_MODELS set and apply .lower() at
all 5 comparison sites so values like 'builtin', 'default', 'n/a' received
from LibreNMS in any case are treated as generic containers
- modules_view: store {'inventory': data, 'librenms_id': id} in the inventory
cache instead of the raw list; get_context_data validates the embedded
librenms_id against the current mapping so remapped devices never serve stale
inventory (non-dict/legacy entries are treated as cache misses)
…silently overwriting When multiple Module objects share the same serial, the old loop would overwrite row["serial_conflict_module"] nondeterministically. Now we group conflicts by serial and only set the move target when exactly one candidate exists; multiple candidates set serial_conflict_ambiguous.
… from phys class - modules_view: get_context_data now calls cache.delete(cache_key) before returning when the cached payload is not the new dict format so pre-upgrade list-form entries are evicted and the next request regenerates fresh data - modules_view: collapse the two-check current-item filter into a single 'model in _GENERIC_CONTAINER_MODELS' test (removes the phys_class=='container' gate) so empty-model non-container items are also treated as generic - modules_view: remove the anc_class=='container' guard from the ancestor walk so any inventory-class ancestor with a generic model (e.g. a 'module' row with model='builtin') is treated as transparent instead of blocking its subtree
The rebase onto pr/code-quality-fixes dropped the consumer of vc_requested in favor of the hoisted vc_detection_enabled variable, leaving the assignment itself as an orphan that ruff F841 flags.
…h fail-closed behavior - virtual_chassis.py: only treat stack as 0-based when positions span 0 AND a positive value. When every entPhysicalParentRelPos is 0 the data is invalid and the shift produced colliding positions (all members → slot 1); fall through to the per-member idx+1 fallback instead. - test_coverage_bulk_import.py: PR bonzo81#257 fails stack imports fast when the user lacks dcim.add_virtualchassis. Update both VC-permission tests to assert failure + error logging instead of silent success.
When the user clicked 'Promote to host of ...', nothing happened and
the page console showed:
TypeError: Cannot read properties of null (reading 'hide')
at HTMLButtonElement.<anonymous> (netbox.js)
The trigger button used data-bs-toggle="modal"+data-bs-target. NetBox's
own bundled JS intercepts those clicks and crashes when the wrapping
modal (our HTMX validation modal) isn't a Bootstrap-managed instance.
The plugin's frontend instructions explicitly forbid reintroducing
data-bs-toggle for modals for exactly this reason.
Switch the trigger to a plain button with a data-modal-target attribute
and wire it up via a small JS handler at the end of the template that
opens the nested modal directly through bootstrap.Modal API (with a
classList fallback). Verified with Playwright: modal now opens cleanly
on eve-ng-02 / idrac-jhw6nc4.
The nested promote-to-host pick modal's Confirm button carried a
data-bs-dismiss="modal" attribute, and the global dismiss handler in
librenms_import.js called event.preventDefault() on ANY click matching
[data-bs-dismiss="modal"]. That intercepted the form submit and merely
closed the outer validation modal -- the promote POST never fired.
Fix:
* Scope the global dismiss handler to the outer modal only (compare
closest('.modal') to modalElement before preventing default + closing).
* Drop the stray data-bs-dismiss attribute from the Confirm Promote
submit button -- the new HX-Trigger flow handles modal closing.
* Switch PromoteToHostView and AddAsOOBView from emitting
HX-Trigger=closeModal to a structured validationRefresh event carrying
the device id. The new listener closes any nested modal inside
#htmx-modal-content (fallback DOM toggling -- window.bootstrap is not
exposed by NetBox) and dispatches a synthetic click on the row's
details button to re-fetch validation, leaving the user inside the
refreshed outer modal.
Verified end-to-end with Playwright on eve-ng-02 / idrac-jhw6nc4
(pk 85, LibreNMS 39):
* POST device_promote_to_host/39/ returns 200
* HX-Trigger header carries {"validationRefresh":{"deviceId":39}}
* Inner pick modal closes, outer validation modal stays open
* Validation auto-refreshes -- shows new link state
* Device renamed, librenms_id reshaped (host=39, OOB=25/idrac/IP)
445 targeted tests pass.
LibreNMSImportView._load_job_results was updated to accept a request kwarg, but two test_coverage_list assertions still pinned the old single-argument signature, breaking the suite. Update them to expect the (job_id, request=request) form and fix the side_effect helper signature accordingly.
When a NetBox device has already been promoted to host (with the OOB sub-key in its librenms_id custom field), the bulk-import table used to show its two halves with completely different styling: the OOB row got a blue 'View OOB details' button while the host row got a green 'ready' button, as if they were unrelated devices. Plumb existing_librenms_link through the librenms_id match path in validate_device_for_import (it was previously only set on the serial-match path), and add a 'paired host' branch in DeviceImportTable.render_actions that uses btn-outline-info + mdi-server-network + ' Host' label whenever the validation reports a paired_oob_id distinct from the host id. The OOB-linked branch now also labels itself ' OOB' with a paired-host tooltip so the two halves match visually.
Two issues from code review of cd88459: 1. XSS in paired-host tooltip: paired_oob_type is read from the user-editable librenms_id custom field with only a string-type check and no sanitisation, then interpolated raw into the title attribute. Wrap with escape() and force int() on the IDs for defence in depth. 2. ID-zero false negatives: _describe_existing_librenms_link accepts integer 0 as a valid id. Replace truthy checks with explicit 'is not None' so a LibreNMS device with id 0 still gets paired styling.
Test names already described the new behavior (`_returns_400`, `_returns_409`, `validation_error`, `integrity_error`); only the assertion bodies still checked the legacy 200 + HX-Reswap=none HTMX-toast contract. Updated to assert the actual status codes and error-message content.
Extract LibreNMSAPI._normalize_librenms_id into a public helper
utils.coerce_librenms_id and use it in _describe_existing_librenms_link
so legacy string-digit IDs are picked up alongside bare ints.
Why: _describe_existing_librenms_link only inspected `isinstance(int)`
at the top-level cf_value and per-server entry branches, silently
dropping string-digit legacy IDs. That degraded the OOB-pairing UX
(already_linked_elsewhere check, merge-candidate detection) for any
pre-migration device whose librenms_id custom field still stored a
string. The migration prompt path already treats string-digit as a
known legacy variant; this aligns the link-describer with it.
coerce_librenms_id only accepts int/str (rejects bool, None, dicts,
MagicMocks) — matches the original implicit isinstance() guard so
mock-based tests for serial/OOB heuristics still distinguish "no link"
from "linked to id N".
The inner dict-form branches ({"id": int, "oob": {"id": int}}) keep
their strict isinstance(int) check — those values are only written by
set_librenms_device_id, which always stores ints.
LibreNMSAPI._normalize_librenms_id becomes a back-compat shim that
delegates to the new helper.
The IP-path OOB inference treated IPv6-only devices as having no primary IP because the gate only inspected device.primary_ip4_id. A NetBox device with primary_ip6 set but no primary_ip4 would be offered as an OOB candidate even though it already has a host IP. Extract has_primary_ip = bool(primary_ip4_id or primary_ip6_id) so the gate matches the intent: only treat the matched IP as a likely OOB IP when the device has no host IP at all (either family) or the IP literally equals device.oob_ip_id.
The lookup in get_or_create_global_ip omitted any VRF filter even
though the helper documents itself as operating on the global scope.
In multi-VRF deployments an IPAddress can exist in vrf=None and one
or more customer VRFs at the same host; .first() with no ordering
could return a customer-scoped record and the caller would silently
treat it as global. Add vrf__isnull=True to both the pre-create
lookup and the post-IntegrityError re-query.
The previous broad `except Exception` also swallowed concurrent-create
races: when another process won the insert, this returned (None, False)
even though the record was successfully created. Narrow that to a
dedicated `except IntegrityError` branch that re-queries the global
record and returns it as (existing, False). The outer `except Exception`
stays to preserve the documented best-effort contract for validation
and other failures.
Skipped from the original suggestion: switching to
get_or_create(address=f"{ip}{mask}", ...). That would only match an
exact prefix length, breaking the deliberate "any prefix length is
acceptable" semantics of net_host — a host already covered by a
global /24 would then get a duplicate /32 inserted alongside it.
Both TestBulkImportConfirmCollisions tests inspected the mocked render() call args instead of the response object actually returned by BulkImportConfirmView.post. Today the render-mock's side_effect mirrors call args onto the returned object so the two paths see the same data — but post() also has earlier HttpResponse(...) direct- return branches (permission errors, validation). A regression that short-circuited via one of those would leave mock_render.call_args empty/stale and the test would silently inspect the wrong thing instead of failing on the actual response. Capture and assert on the returned response (status_code, template_name, context) so the tests validate what callers actually receive.
The auto-create IPAM checkbox was rendered on the import-settings form but its change-detection wasn't wired into updateImportSaveButton(), so toggling only that checkbox left the Save button disabled and the user couldn't persist the preference. Track it alongside the existing use_sysname / strip_domain checkboxes using the same capture-initial + compare-on-change pattern.
The Stage 2 merge-candidate block read the un-refreshed local `import_as_vm`, so a hostname match against a VirtualMachine (which sets result["import_as_vm"]=True at line 466) still entered the block and ran Device.objects.filter(...).exclude(pk=vm.pk).first() against a VM, then called _describe_existing_librenms_link with the VM. Move the existing local-variable refresh up to just before the merge block.
The DT and Platform mapping forms registered an unconditional
`document.addEventListener("click", ...)` inside their IIFE, so each
HTMX re-render (per row, per modal open) added another listener to
document — with the previous listeners holding references to detached
searchEl/dropdownEl nodes for the life of the page.
A container-scoped listener doesn't work here: outside-click detection
needs events from outside the container. Instead, give the handler a
name and have it self-remove the first time it fires after its form
has been detached from the DOM.
The platform-mapping include guarded on `libre_device.os` truthiness only, so a LibreNMS placeholder value of "-" still rendered the add- mapping form (which would then try to create a mapping for OS "-"). Three other guards in the same template already excluded "-"; align this one with them.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
netbox_librenms_plugin/import_utils/device_operations.py (1)
1007-1196:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
created_ipsin all return payloads for a stable contract.
created_ipsis only present on the success path (Line 1195). Error/early-return paths currently omit it, making the response shape inconsistent for callers.Proposed fix
return { "success": False, "device": None, "message": "", "error": f"Failed to retrieve device {device_id} from LibreNMS", "synced": {}, + "created_ips": [], } @@ return { "success": False, "device": validation["existing_device"], "message": "", "error": f"Device already exists: {validation['existing_device'].name}", "synced": {}, + "created_ips": [], } @@ return { "success": False, "device": None, "message": "", "error": str(e), "synced": {}, + "created_ips": [], }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 1007 - 1196, The function initializes created_ips but only includes it in the final success return, causing inconsistent response shapes; update every early/error return dict (the ones returning {"success": False, ...} after the get_device_info check, validation checks, and required-field checks) to include "created_ips": created_ips so callers always receive the same keys; locate the return statements near the calls/refs to get_device_info, validate_device_for_import, and the validation["existing_device"]/site/device_type/device_role checks and add the created_ips entry to each returned dict.netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html (1)
116-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
server_keyon everydevice_conflict_actionPOST.These forms only identify the LibreNMS row by numeric
device_idin the URL. With multi-serverlibrenms_idsupport, that is no longer globally unique, so the action has to fall back to the mutable selected server and can link/sync the wrong device. The migrate/OOB/promote flows in this same template already send a hiddenserver_key; thedevice_conflict_actionforms need the same treatment.Also applies to: 165-173, 184-190, 242-249, 297-303, 330-336, 597-604, 922-929
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html` around lines 116 - 123, The device_conflict_action POSTs only include the LibreNMS numeric device_id in the URL, which is ambiguous for multi-server setups; add a hidden input named "server_key" with value "{{ libre_device.server_key }}" to each form that posts to the URL named 'plugins:netbox_librenms_plugin:device_conflict_action' (e.g., the form containing hx-post="{% url 'plugins:netbox_librenms_plugin:device_conflict_action' device_id=libre_device.device_id %}" and the other forms at the ranges you listed) so the server_key is submitted with the POST; add the same hidden input to every corresponding form instance (lines referenced: the forms around startLine 116 and also at 165-173, 184-190, 242-249, 297-303, 330-336, 597-604, 922-929).netbox_librenms_plugin/models.py (1)
109-164:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAdd schema migrations for these model changes.
This file adds several database-backed models and changes
InterfaceTypeMappinguniqueness, but the only migration in the reviewed stack is0011_librenmssettings_auto_create_ipam_default.py. As-is, the ORM will be ahead of the database, so any page/query touching these models or constraints will fail at runtime with missing-table or stale-schema errors.Also applies to: 179-938
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/models.py` around lines 109 - 164, The models (e.g., InterfaceTypeMapping and other new DB-backed models introduced between lines ~179-938) changed the schema and added UniqueConstraints, but no migrations were added; run Django's makemigrations to create migrations that create the new tables and add/alter the unique constraints (include the wildcard constraint for librenms_speed IS NULL and the non-null constraint), add those migration files to the PR (ensure the new migration depends on 0011_librenmssettings_auto_create_ipam_default if appropriate), and document/apply them in CI by running migrate so the DB schema matches the ORM before deployment.
♻️ Duplicate comments (2)
netbox_librenms_plugin/import_utils/collisions.py (1)
41-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject boolean IDs before integer coercion.
At Line 43 and Line 115,
True/Falseare currently coerced to1/0, which can create false collision buckets instead of skipping invalid rows.Proposed fix
def _add(pk, name, role): + if isinstance(pk, bool): + return try: pk_int = int(pk) except (TypeError, ValueError): return @@ for entry in devices or []: validation = entry.get("validation") or {} + raw_libre_id = entry.get("device_id") + if isinstance(raw_libre_id, bool): + continue try: - libre_id = int(entry.get("device_id")) + libre_id = int(raw_libre_id) except (TypeError, ValueError): continueAlso applies to: 114-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/import_utils/collisions.py` around lines 41 - 45, The _add function currently coerces boolean pk values (True/False) into integers 1/0 during int(pk), causing incorrect collision buckets; update the validation in _add (and the analogous logic at lines handling pk around the second occurrence) to explicitly reject booleans before attempting integer coercion (e.g., if isinstance(pk, bool): return) so True/False are skipped rather than converted, then proceed with int(pk) for non-boolean values only.netbox_librenms_plugin/import_utils/ip_helpers.py (1)
69-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard non-string
ip_strbefore calling.strip().At Line 71, truthy non-string input can raise
AttributeError, which breaks the helper’s “never raise / return(None, False)” behavior on malformed input.Proposed fix
- if not ip_str: + if ip_str is None: return None, False + if not isinstance(ip_str, str): + logger.debug("get_or_create_global_ip: invalid non-string IP %r", ip_str) + return None, False ip_str = ip_str.strip() if not ip_str: return None, False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/import_utils/ip_helpers.py` around lines 69 - 73, The guard currently calls ip_str.strip() without ensuring ip_str is a string, so truthy non-string values can raise AttributeError; before calling .strip() (in the function handling ip_str in ip_helpers.py) add an explicit type check like isinstance(ip_str, str) and return (None, False) for any non-string values, preserving the existing None/False behavior for empty or malformed input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 1007-1196: The function initializes created_ips but only includes
it in the final success return, causing inconsistent response shapes; update
every early/error return dict (the ones returning {"success": False, ...} after
the get_device_info check, validation checks, and required-field checks) to
include "created_ips": created_ips so callers always receive the same keys;
locate the return statements near the calls/refs to get_device_info,
validate_device_for_import, and the
validation["existing_device"]/site/device_type/device_role checks and add the
created_ips entry to each returned dict.
In `@netbox_librenms_plugin/models.py`:
- Around line 109-164: The models (e.g., InterfaceTypeMapping and other new
DB-backed models introduced between lines ~179-938) changed the schema and added
UniqueConstraints, but no migrations were added; run Django's makemigrations to
create migrations that create the new tables and add/alter the unique
constraints (include the wildcard constraint for librenms_speed IS NULL and the
non-null constraint), add those migration files to the PR (ensure the new
migration depends on 0011_librenmssettings_auto_create_ipam_default if
appropriate), and document/apply them in CI by running migrate so the DB schema
matches the ORM before deployment.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 116-123: The device_conflict_action POSTs only include the
LibreNMS numeric device_id in the URL, which is ambiguous for multi-server
setups; add a hidden input named "server_key" with value "{{
libre_device.server_key }}" to each form that posts to the URL named
'plugins:netbox_librenms_plugin:device_conflict_action' (e.g., the form
containing hx-post="{% url
'plugins:netbox_librenms_plugin:device_conflict_action'
device_id=libre_device.device_id %}" and the other forms at the ranges you
listed) so the server_key is submitted with the POST; add the same hidden input
to every corresponding form instance (lines referenced: the forms around
startLine 116 and also at 165-173, 184-190, 242-249, 297-303, 330-336, 597-604,
922-929).
---
Duplicate comments:
In `@netbox_librenms_plugin/import_utils/collisions.py`:
- Around line 41-45: The _add function currently coerces boolean pk values
(True/False) into integers 1/0 during int(pk), causing incorrect collision
buckets; update the validation in _add (and the analogous logic at lines
handling pk around the second occurrence) to explicitly reject booleans before
attempting integer coercion (e.g., if isinstance(pk, bool): return) so
True/False are skipped rather than converted, then proceed with int(pk) for
non-boolean values only.
In `@netbox_librenms_plugin/import_utils/ip_helpers.py`:
- Around line 69-73: The guard currently calls ip_str.strip() without ensuring
ip_str is a string, so truthy non-string values can raise AttributeError; before
calling .strip() (in the function handling ip_str in ip_helpers.py) add an
explicit type check like isinstance(ip_str, str) and return (None, False) for
any non-string values, preserving the existing None/False behavior for empty or
malformed input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2ca905c-dda2-475d-9a92-3a4308b2e431
⛔ Files ignored due to path filters (13)
docs/img/Netbox-librenms-plugin-device-sync-fields.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-import-page.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-module-sync-tab.pngis excluded by!**/*.pngdocs/img/carrier_auto_install_rules/list.pngis excluded by!**/*.pngdocs/img/device_type_mappings/list.pngis excluded by!**/*.pngdocs/img/inventory_ignore_rules/list.pngis excluded by!**/*.pngdocs/img/module_bay_mappings/list.pngis excluded by!**/*.pngdocs/img/module_type_mappings/add.pngis excluded by!**/*.pngdocs/img/module_type_mappings/list.pngis excluded by!**/*.pngdocs/img/normalization_rules/add.pngis excluded by!**/*.pngdocs/img/normalization_rules/list.pngis excluded by!**/*.pngdocs/img/platform_mappings/add.pngis excluded by!**/*.pngdocs/img/platform_mappings/list.pngis excluded by!**/*.png
📒 Files selected for processing (51)
docs/README.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/README.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_forms.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/migrate.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX 2.x is the primary async layer. Table row updates must return
<tr hx-swap-oob="true">.Avoid
outerHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.Do not reintroduce
data-bs-toggleor duplicate modal IDs in modal implementations.Device import dropdowns must use TomSelect decorators as set up elsewhere; keep
<select class="device-role-select">markup stable to preserve JS hook-up.Styling assumes Tabler defaults.
Do not re-add
table-responsivewrappers; removing them was deliberate to prevent dropdown clipping.Templates must live in
templates/netbox_librenms_plugin/; reuse/includes must be placed underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
netbox_librenms_plugin/**/*.{js,html}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token, preferringdocument.querySelector('[name=csrfmiddlewaretoken]').valuefrom a hidden form input overgetCookie('csrftoken')for consistency.Modals must try Bootstrap 5 native (
bootstrap.Modal) first, falling back to manual DOM manipulation if unavailable viashowModal()/hideModal()helpers.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments must live in
templates/netbox_librenms_plugin/htmx/and include specific fragments:device_import_row.html,device_validation_details.html,device_vc_details.html, andbulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
**/import_utils/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
import_utils/is a package; the__init__.pymust re-export key functions so callers can usefrom import_utils import ...
Files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.py
**/import_utils/vm_operations.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
vm_operations.pymust implementbulk_import_vms(vm_imports, user, ...)
Files:
netbox_librenms_plugin/import_utils/vm_operations.py
netbox_librenms_plugin/templates/netbox_librenms_plugin/*_sync*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Sync pages must extend
librenms_sync_base.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
netbox_librenms_plugin/templates/**/_*_sync{,_content}.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Each sync resource must have two templates following a naming convention:
_<resource>_sync.html(tab wrapper, loaded once) and_<resource>_sync_content.html(HTMX-swappable inner fragment, refreshed on data changes).
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/templates/**/settings.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Settings page must use a split-form pattern: two separate Django forms (
ServerConfigForm+ImportSettingsForm) sharing one page, differentiated by a hiddenform_typefield with values"server_config"or"import_settings".The settings page test-connection button must be an HTMX POST to
TestLibreNMSConnectionView, returning an inline alert fragment.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/settings.html
**/import_utils/device_operations.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
device_operations.pymust implementvalidate_device_for_import(device, ...)andbulk_import_devices_shared(devices, user, ...)
Files:
netbox_librenms_plugin/import_utils/device_operations.py
netbox_librenms_plugin/static/**/*.js
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
The import page must use
ModalManagerclass andfilterModalManagerinstance reference in fetch callbacks, not undefinedmodalInstancevariables.Dismiss handlers (backdrop click,
data-bs-dismissbuttons) must be bound once per element to prevent stacking on repeatedshowModal()calls.The import filter form uses fetch with
Accept: application/json, text/html—JSON for background jobs, HTML for synchronous mode.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
netbox_librenms_plugin/**/*.js
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Always check
response.okbefore processing fetch responses to catch HTTP errors.In fetch catch blocks, show
error.messagefor debugging rather than generic messages.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
netbox_librenms_plugin/static/**/librenms_import.js
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
JavaScript in
librenms_import.jsmust be wrapped in an IIFE withwindow.LibreNMSImportInitializedguard to prevent re-initialization during HTMX swaps.The
ModalManagerclass must wrap Bootstrap 5 modal show/hide with fallback for the import page.The
pollJobStatus()function must poll/api/core/background-tasks/{jobId}/every 2s, update progress messages, handle cancel button, and redirect on completion.The
captureSelectionState()andrestoreSelectionState()functions must preserve checkbox state across HTMX content swaps.The
createCacheCountdown()function must provide a generic countdown timer for cache expiration display.The
initializeFilterForm()function must intercept form submit, detect JSON response (background job), and start polling.CSRF token in JavaScript must be extracted via
getCookie('csrftoken')(cookie-based) as the fallback method, with preference for hidden form input approach.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Hook into NetBox (Django 5) plugin APIs via `navigation.py`, `urls.py`, and `api/`, respecting NetBox plugin conventions
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Reuse the LibreNMS API client from `librenms_api.py` instead of making new `requests` calls; it handles multi-server configs via `LibreNMSSettings` model and caching
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Devices/VMs map to LibreNMS via the `librenms_id` custom field; always call `LibreNMSAPI.get_librenms_id` instead of touching the field directly
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Use exact-only matching for site, platform, device type, and role; do not add fuzzy matching. Rely on `utils.py` functions (`find_matching_site`, `match_librenms_hardware_to_device_type`, `find_matching_platform`)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Sync pipelines should follow the three-layer pattern: fetch LibreNMS data (`librenms_api.py`), cache it (`CacheMixin`), build comparison tables (`tables/`), and render HTMX fragments (`templates/netbox_librenms_plugin/htmx/`)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Use virtual chassis support via `get_virtual_chassis_member()` for port-to-member mapping and `get_librenms_sync_device()` for VC priority-based device selection
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: AJAX/JSON endpoints must use `require_all_permissions_json("POST")` instead of `require_all_permissions("POST")` to return JSON 403 responses on permission failure
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Background job polling requires superuser permission; non-superusers fall back to synchronous mode
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: REST endpoints for imports live in `views/imports/actions.py` (with the list view in `views/imports/list.py`) and surface via `urls.py`; keep server responses and HTMX targets in sync
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: `import_validation_helpers.py` centralizes validation state mutation during import (role/cluster/rack assignment, issue removal, status recalculation); reuse this module instead of duplicating validation logic
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Prefer the devcontainer commands (`netbox-run`, `netbox-run-bg`, `netbox-reload`, `netbox-logs`) described in `.devcontainer/README.md` for managing NetBox + plugin reloading
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Check docs in `docs/development/` for structure, view inheritance, mixins, and template conventions before introducing new patterns
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:04.719Z
Learning: Review existing sync views (e.g., `views/sync/interfaces.py`) as reference implementations for data flow and caching patterns
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:51.902Z
Learning: Tables emit HTMX-enabled columns and buttons via `tables/*.py` Python renderers; prefer updating the table renderer in Python rather than templates when changing row actions.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:51.902Z
Learning: When adding a new sync resource, create both the wrapper and content templates following the `_<resource>_sync.html` and `_<resource>_sync_content.html` pattern.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:51.902Z
Learning: Keep server responses and HTMX targets in sync when modifying HTMX fragments.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:59.768Z
Learning: Version bump commit message must be exactly `Bump version to X.Y.Z and update changelog` and include simultaneous updates to `__init__.py`, `pyproject.toml`, and `changelog.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:59.768Z
Learning: Create `release/X.Y.Z` branch from develop, then submit PR to develop with title `Bump version to X.Y.Z and update changelog` using the standard version bump PR body template
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:59.768Z
Learning: Submit Release PR from develop to master with title `Release X.Y.Z` using the standard release PR body template, which will auto-include all commits master is missing
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:59.768Z
Learning: Create Git tag `vX.Y.Z` on master branch and publish GitHub release with release notes following the standard template structure including release type, summary, changes with PR numbers, and upgrade notes
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:44:59.768Z
Learning: Both `develop` and `master` branches have branch protection enabled; all changes must go through pull requests and never be pushed directly
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:45:14.637Z
Learning: librenms_api.py module tests should be in test_librenms_api.py and test_librenms_api_helpers.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:45:14.637Z
Learning: import_utils package modules (filters.py, device_operations.py, vm_operations.py, cache.py, permissions.py, virtual_chassis.py), import_validation_helpers.py, and utils.py tests should be in test_import_utils.py, test_import_validation_helpers.py, and test_utils.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:45:14.637Z
Learning: jobs.py and views/imports/list.py module tests should be in test_background_jobs.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:45:14.637Z
Learning: import_utils/bulk_import.py module tests should be in test_coverage_bulk_import.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:45:14.637Z
Learning: Utility helpers (utils.py coverage tests) should be in test_coverage_utils.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:45:14.637Z
Learning: Permission mixins, API permissions, and constants tests should be in test_permissions.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:45:14.637Z
Learning: VLAN API, mode detection, comparison, and sync tests should be in test_vlan_sync.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:45:14.637Z
Learning: VlanAssignmentMixin and VLAN enrichment tests should be in test_interface_vlan_sync.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T21:45:14.637Z
Learning: Views in views/sync/, views/object_sync/, and views/imports/actions.py should be tested via the utility modules they call, not via HTTP requests; no dedicated test files yet
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/constants.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tests/test_coverage_forms.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/constants.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tests/test_coverage_forms.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/constants.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tests/test_coverage_forms.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-03-08T14:17:28.826Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/import_utils/virtual_chassis.py:73-80
Timestamp: 2026-03-08T14:17:28.826Z
Learning: In Python code, when a lookup returns None (including API failures), implement negative caching by storing an empty result with a configurable TTL (default 5 minutes). Document the TTL and ensure a force_refresh=True bypasses the cache for manual re-fetch actions. Do not treat caching None/empty results on API failure as a bug if this mirrors existing patterns (e.g., get_device_with_server caching None on not-found). Apply this guidance to files within netbox_librenms_plugin/import_utils where similar inventory/API lookups occur.
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-09T20:10:48.502Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/import_utils/device_operations.py:496-575
Timestamp: 2026-03-09T20:10:48.502Z
Learning: In netbox_librenms_plugin/import_utils/device_operations.py, in validate_device_for_import(), ensure both the cluster-required blocker (VM path) and the device_role-required blocker (device path) are guarded with if not result.get('existing_device') to ensure create-time prerequisites are only appended for new imports, not for link/update flows; keep available_roles and available_clusters populated for both new and existing-device cases so UI dropdowns function correctly on update views.
Applied to files:
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.py
📚 Learning: 2026-03-08T11:35:34.876Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 23
File: netbox_librenms_plugin/import_utils/vm_operations.py:61-69
Timestamp: 2026-03-08T11:35:34.876Z
Learning: In netbox_librenms_plugin/import_utils/vm_operations.py, do not flag the bare-int librenms_id in create_vm_from_librenms as a multi-server bug on the pr/librenms-pre-id-multi-server branch. This file is on a staging branch where per-server scoping migration (set_librenms_device_id(vm, device_id, server_key)) will occur in the next multi-server PR. Treat this as a branch-specific exception and plan to enforce the per-server convention only in the subsequent PR, not on this branch.
Applied to files:
netbox_librenms_plugin/import_utils/vm_operations.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_forms.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_forms.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_forms.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-03-09T20:16:13.590Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/tests/test_coverage_list.py:175-177
Timestamp: 2026-03-09T20:16:13.590Z
Learning: In netbox_librenms_plugin/tests/test_coverage_list.py, strengthen the _load_job_results() tests (around lines 175–177, 209–211, 239–241) to assert all arguments passed to get_validated_device_cache_key, not just device_id. Specifically verify server_key, vc_detection_enabled, use_sysname, and strip_domain are propagated in the mocks. Replace the current lambda mock that varies only by device_id with a mock or assertion that checks all discriminators. This is a concrete, future-proofable test improvement; note that this should be tracked as a backlog item and do not re-raise a missing full-kwarg assertion as a new finding.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_list.py
📚 Learning: 2026-03-12T20:27:53.873Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/librenms_api.py:703-709
Timestamp: 2026-03-12T20:27:53.873Z
Learning: In netbox_librenms_plugin/librenms_api.py, enforce that get_device_inventory() and get_inventory_filtered() always return a list of dicts. Validate as: inventory must be a list and every item must be a dict; if not, log a warning with the raw payload and return (False, error_message). Do not weaken the check to just verify a list type. This should prevent downstream AttributeError/TypeError when callers call .get() on items.
Applied to files:
netbox_librenms_plugin/librenms_api.py
📚 Learning: 2026-03-09T10:39:37.846Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:1039-1047
Timestamp: 2026-03-09T10:39:37.846Z
Learning: In netbox_librenms_plugin/tests/test_coverage_device_operations.py, fix test_no_hostname_adds_issue in both TestValidateDeviceForImportEdgeCases and TestValidateDeviceMoreEdgeCases. Do not patch _determine_device_name. Instead, call validate_device_for_import with sysName="" and hostname="" and assert that "no hostname" is NOT present in result.get("issues", []) because the fallback name (device-{id}) is used when both inputs are empty. Ensure the test directly verifies the absence of the blocker when inputs are empty, relying on the actual fallback behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_device_operations.py
🪛 HTMLHint (1.9.2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 22-22: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 27-27: Special characters must be escaped : [ > ].
(spec-char-escape)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 20-20: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 25-25: Special characters must be escaped : [ > ].
(spec-char-escape)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
[error] 656-656: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 661-661: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 665-665: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 670-670: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 679-679: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 680-680: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 733-733: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 734-734: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 815-815: Duplicate of attribute name [ new_name ] was found.
(attr-no-duplication)
[error] 918-918: Tag must be paired, no start tag: [ ]
(tag-pair)
🪛 LanguageTool
docs/usage_tips/README.md
[grammar] ~18-~18: Use a hyphen to join words.
Context: ...during device import and sync 4. [Multi Server Configuration](multi_server_confi...
(QB_NEW_EN_HYPHEN)
🪛 OpenGrep (1.21.0)
netbox_librenms_plugin/tables/modules.py
[WARNING] 695-695: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
[WARNING] 751-751: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
This was referenced May 22, 2026
Closed
Closed
Merged
Merged
This was referenced Jun 15, 2026
Merged
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
2 participants
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
Briefly describe what this PR does in plain English, and provide as much of the following information as possible.
Motivation / Problem
What issue does this solve?
Link any related issues if applicable.
Scope of Change
Delete items that don’t apply:
How Was This Tested?
Delete items that don’t apply and describe briefly.
Manual Test Steps (if applicable)
Risk Assessment
Explain briefly.
Backwards Compatibility
Other Notes
Anything the maintainer(s) should pay particular attention to?
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests