Pr/inventory core - #76
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.
handleModuleChange fired off a fetch on every dropdown change with no cancellation, so a slow earlier response could clobber the row that a later, faster response had already rendered (and re-binding the row listeners on the wrong data). Track an AbortController per select; on re-entry, abort the previous in-flight request and ignore its AbortError in catch.
Every other regex entry in contrib/module_bay_mappings.yaml uses ^...$; the two Nokia transceiver patterns were the only ones missing anchors. Functionally identical because ModuleBayMapping lookup uses re.fullmatch(), but anchor them so the file is internally consistent and the patterns are unambiguous when read on their own.
![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
🤖 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.
Inline comments:
In `@contrib/module_bay_mappings.yaml`:
- Around line 93-103: The two connector-style regex mapping entries using
librenms_name "^(\\d+)/(\\d+)/(c\\d+)$" and "^(\\d+)/x(\\d+)/(\\d+)/(c\\d+)$"
are currently global; scope them to Nokia by adding manufacturer: "Nokia" to
each mapping entry (the blocks that include librenms_class: "port",
netbox_bay_name: "\\2/\\3" and netbox_bay_name: "\\3/\\4") so they only apply to
Nokia devices and won’t match other vendors exposing similar path shapes.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1697-1704: The code currently reads selectedDeviceId from
this.dataset.selectedDeviceId and always adds selected_device_id to the
URLSearchParams, which serializes undefined to the string "undefined"; change
the logic so you only append selected_device_id when selectedDeviceId is present
(e.g. not undefined/null/empty) before creating/setting the params; locate the
reference to this.dataset.selectedDeviceId and the URLSearchParams construction
(params using moduleId, entIndex, serverKey) and conditionally add
selected_device_id only when valid.
- Around line 1823-1832: The helper updateHtmxModalLabel currently only looks
inside `#htmx-modal-body` for the header, so when HTMX swaps a full fragment into
`#htmx-modal-content` the function keeps the stale accessible label; update the
DOM search to look across the entire modal content (e.g. prefer
htmxModal.querySelector('`#htmx-modal-body`') ||
htmxModal.querySelector('`#htmx-modal-content`') || htmxModal) and find the header
via a query on that resolved container (or directly on htmxModal) using the same
selectors ('.modal-title, .modal-header h5, .modal-header h4') before copying
text into the aria-labelledby element in updateHtmxModalLabel.
🪄 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: e7f1d990-5fe4-49ab-86c4-16a44c5d8954
📒 Files selected for processing (3)
contrib/module_bay_mappings.yamlnetbox_librenms_plugin/migrations/0010_inventory_and_mapping_models.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Reuse the LibreNMS client from
librenms_api.pyinstead of making newrequestscalls. The client handles multi-server configs viaLibreNMSSettingsmodel and theserversplugin config, plus caching via Django cache and custom fields.Always call
LibreNMSAPI.get_librenms_idinstead of directly touching thelibrenms_idcustom field to map Devices/VMs to LibreNMS via custom fields, then cache if absent.Use exact-only matching for site, platform, device type, and role in sync pipelines. Do not add fuzzy matching. See
utils.pyfunctions:find_matching_site,match_librenms_hardware_to_device_type,find_matching_platform.Virtual chassis support should use
get_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selection.
Files:
netbox_librenms_plugin/migrations/0010_inventory_and_mapping_models.py
**/librenms_sync.js
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/librenms_sync.js: JavaScript inlibrenms_sync.jsmust not be wrapped in an IIFE and must use a master initializerinitializeScripts()that runs on bothDOMContentLoadedandhtmx:afterSwapevents.
JavaScript checkbox management must include functionsinitializeTableCheckboxes()andupdateBulkActionButton()to handle multi-table checkbox selection and bulk action button state.
JavaScript TomSelect dropdown initialization must use aTOMSELECT_INIT_DELAY_MS = 100constant and implement delayed initialization after HTMX swaps. Required initializer functions:initializeVCMemberSelect(),initializeVRFSelects(),initializeVlanGroupSelects(),initializeVlanSyncGroupSelects().
JavaScript verification functions must includehandleInterfaceChange(),handleCableChange(),handleVRFChange()that POST to single-item verify endpoints to validate resource changes.
JavaScript VLAN modal functions must implementopenVlanDetailModal(),verifyVlanInGroup(),verifyVlanSyncGroup()for per-interface VLAN detail editing.
JavaScript bulk operations must include functionsinitializeBulkEditApply()anddeleteSelectedInterfaces()to handle bulk edit and delete actions.
JavaScript table filtering must implementinitializeTableFilters()andfilterTable()functions for client-side row filtering.
JavaScript URL and tab state management must implementinitializeTabs(),getDeviceIdFromUrl(), andsetInterfaceNameFieldFromURL()to maintain browser state and URL synchronization.
JavaScript cache countdown functionality must implementinitializeCountdown()andinitializeCountdowns()functions to display and manage cache expiration timers.
JavaScript CSRF token must be extracted viadocument.querySelector('[name=csrfmiddlewaretoken]').valuefor all POST requests.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
netbox_librenms_plugin/static/**/*.js
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/static/**/*.js: Modals should try Bootstrap 5 native (bootstrap.Modal) first, falling back to manual DOM manipulation if unavailable. UseshowModal()/hideModal()helper functions.
UseModalManagerclass reference andfilterModalManagerinstance in fetch callbacks; do not use undefinedmodalInstancevariables.
Bind dismiss handlers (backdrop click,data-bs-dismissbuttons) once per element to prevent stacking on repeatedshowModal()calls.
Always checkresponse.okbefore processing fetch responses to catch HTTP errors.
In fetch catch blocks, showerror.messagefor debugging rather than generic messages.
The import filter form uses fetch withAccept: application/json, text/html—JSON for background jobs, HTML for synchronous mode.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:24:55.461Z
Learning: All sync pipelines should fetch LibreNMS data from `librenms_api.py`, cache it using `CacheMixin`, build comparison tables in `tables/`, and render HTMX fragments in `templates/netbox_librenms_plugin/htmx/`. Follow this flow for new resources.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:24:55.461Z
Learning: Prefer the devcontainer commands (`netbox-run`, `netbox-run-bg`, `netbox-reload`, `netbox-logs`) described in `.devcontainer/README.md` for development workflow management.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:24:55.461Z
Learning: Plugin hooks should respect NetBox plugin APIs via `navigation.py`, `urls.py`, and `api/` to properly integrate with NetBox (Django 5) under the `netbox_librenms_plugin/` module.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:25:09.803Z
Learning: librenms_api.py should be tested in test_librenms_api.py and test_librenms_api_helpers.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:25:09.803Z
Learning: import_utils/ package (filters.py, device_operations.py, vm_operations.py, cache.py, permissions.py, virtual_chassis.py), import_validation_helpers.py, and utils.py should be tested 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-19T09:25:09.803Z
Learning: jobs.py and views/imports/list.py should be tested in test_background_jobs.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:25:09.803Z
Learning: import_utils/bulk_import.py should be tested in test_coverage_bulk_import.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:25:09.803Z
Learning: Utility helpers (utils.py coverage tests) should be tested in test_coverage_utils.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:25:09.803Z
Learning: Permission mixins, API permissions, and constants should be tested in test_permissions.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:25:09.803Z
Learning: VLAN API, mode detection, comparison, and sync should be tested in test_vlan_sync.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T09:25:09.803Z
Learning: VlanAssignmentMixin and VLAN enrichment should be tested in test_interface_vlan_sync.py
📚 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/0010_inventory_and_mapping_models.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/0010_inventory_and_mapping_models.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/0010_inventory_and_mapping_models.py
🪛 OpenGrep (1.21.0)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
[WARNING] 1087-1087: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1089-1089: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1090-1090: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1091-1091: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1092-1092: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1093-1093: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1105-1105: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1620-1620: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1739-1739: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
Comment on lines
+93
to
+103
| - librenms_name: "^(\\d+)/(\\d+)/(c\\d+)$" | ||
| librenms_class: "port" | ||
| netbox_bay_name: "\\2/\\3" | ||
| is_regex: true | ||
| description: "Nokia transceiver slot/mda/cN → mda-pos/cN" | ||
| # LibreNMS ifName "2/x1/1/c2" (slot/xiom/mda/connector) → NetBox bay "1/c2" | ||
| - librenms_name: "^(\\d+)/x(\\d+)/(\\d+)/(c\\d+)$" | ||
| librenms_class: "port" | ||
| netbox_bay_name: "\\3/\\4" | ||
| is_regex: true | ||
| description: "Nokia XIOM transceiver slot/xiom/mda/cN → mda-pos/cN" |
There was a problem hiding this comment.
Scope these connector-style regexes to Nokia.
These rows are documented as Nokia-specific, but they're global today. Because they match only on generic 1/1/c1 / 2/x1/1/c2 path shapes, importing this file can remap other vendors that expose the same naming style. Add manufacturer: "Nokia" to keep them from stealing cross-vendor matches.
Suggested fix
- librenms_name: "^(\\d+)/(\\d+)/(c\\d+)$"
librenms_class: "port"
netbox_bay_name: "\\2/\\3"
is_regex: true
+ manufacturer: "Nokia"
description: "Nokia transceiver slot/mda/cN → mda-pos/cN"
# LibreNMS ifName "2/x1/1/c2" (slot/xiom/mda/connector) → NetBox bay "1/c2"
- librenms_name: "^(\\d+)/x(\\d+)/(\\d+)/(c\\d+)$"
librenms_class: "port"
netbox_bay_name: "\\3/\\4"
is_regex: true
+ manufacturer: "Nokia"
description: "Nokia XIOM transceiver slot/xiom/mda/cN → mda-pos/cN"🤖 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 `@contrib/module_bay_mappings.yaml` around lines 93 - 103, The two
connector-style regex mapping entries using librenms_name
"^(\\d+)/(\\d+)/(c\\d+)$" and "^(\\d+)/x(\\d+)/(\\d+)/(c\\d+)$" are currently
global; scope them to Nokia by adding manufacturer: "Nokia" to each mapping
entry (the blocks that include librenms_class: "port", netbox_bay_name:
"\\2/\\3" and netbox_bay_name: "\\3/\\4") so they only apply to Nokia devices
and won’t match other vendors exposing similar path shapes.
Comment on lines
+1697
to
1704
| const selectedDeviceId = this.dataset.selectedDeviceId; | ||
|
|
||
| const params = new URLSearchParams({ | ||
| module_id: moduleId, | ||
| ent_index: entIndex, | ||
| server_key: serverKey, | ||
| selected_device_id: selectedDeviceId, | ||
| }); |
There was a problem hiding this comment.
Don't send "undefined" as selected_device_id.
URLSearchParams will serialize a missing dataset value as the literal string "undefined". That means rows without a chosen override can hit the preview endpoint with a bogus device ID instead of omitting the parameter.
Suggested fix
const params = new URLSearchParams({
module_id: moduleId,
ent_index: entIndex,
server_key: serverKey,
- selected_device_id: selectedDeviceId,
});
+ if (selectedDeviceId) {
+ params.set('selected_device_id', selectedDeviceId);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const selectedDeviceId = this.dataset.selectedDeviceId; | |
| const params = new URLSearchParams({ | |
| module_id: moduleId, | |
| ent_index: entIndex, | |
| server_key: serverKey, | |
| selected_device_id: selectedDeviceId, | |
| }); | |
| const selectedDeviceId = this.dataset.selectedDeviceId; | |
| const params = new URLSearchParams({ | |
| module_id: moduleId, | |
| ent_index: entIndex, | |
| server_key: serverKey, | |
| }); | |
| if (selectedDeviceId) { | |
| params.set('selected_device_id', selectedDeviceId); | |
| } |
🤖 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/static/netbox_librenms_plugin/js/librenms_sync.js`
around lines 1697 - 1704, The code currently reads selectedDeviceId from
this.dataset.selectedDeviceId and always adds selected_device_id to the
URLSearchParams, which serializes undefined to the string "undefined"; change
the logic so you only append selected_device_id when selectedDeviceId is present
(e.g. not undefined/null/empty) before creating/setting the params; locate the
reference to this.dataset.selectedDeviceId and the URLSearchParams construction
(params using moduleId, entIndex, serverKey) and conditionally add
selected_device_id only when valid.
Comment on lines
+1823
to
+1832
| function updateHtmxModalLabel() { | ||
| const htmxModal = document.getElementById('htmx-modal'); | ||
| if (htmxModal) { | ||
| htmxModal.addEventListener('htmx:afterSettle', function () { | ||
| const header = htmxModal.querySelector('.modal-title, .modal-header h5, .modal-header h4'); | ||
| const label = document.getElementById('htmx-modal-label'); | ||
| if (header && label) { | ||
| label.textContent = header.textContent.trim(); | ||
| } | ||
| }); | ||
| if (!htmxModal) return; | ||
| const modalBody = htmxModal.querySelector('#htmx-modal-body') || htmxModal; | ||
| const header = modalBody.querySelector('.modal-title, .modal-header h5, .modal-header h4'); | ||
| const labelId = htmxModal.getAttribute('aria-labelledby'); | ||
| const label = (labelId && document.getElementById(labelId)) || document.getElementById('htmx-modal-label'); | ||
| if (header && label && header !== label) { | ||
| label.textContent = header.textContent.trim(); | ||
| } |
There was a problem hiding this comment.
Search the whole modal content for the new title.
When HTMX swaps a full fragment into #htmx-modal-content, the title sits in .modal-header, not inside #htmx-modal-body. This helper then keeps the stale label, so the shared modal announces the wrong name.
Suggested fix
function updateHtmxModalLabel() {
const htmxModal = document.getElementById('htmx-modal');
if (!htmxModal) return;
- const modalBody = htmxModal.querySelector('`#htmx-modal-body`') || htmxModal;
- const header = modalBody.querySelector('.modal-title, .modal-header h5, .modal-header h4');
+ const modalContent = htmxModal.querySelector('`#htmx-modal-content`') || htmxModal;
+ const header = modalContent.querySelector('.modal-title, .modal-header h5, .modal-header h4');
const labelId = htmxModal.getAttribute('aria-labelledby');
const label = (labelId && document.getElementById(labelId)) || document.getElementById('htmx-modal-label');
if (header && label && header !== label) {
label.textContent = header.textContent.trim();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function updateHtmxModalLabel() { | |
| const htmxModal = document.getElementById('htmx-modal'); | |
| if (htmxModal) { | |
| htmxModal.addEventListener('htmx:afterSettle', function () { | |
| const header = htmxModal.querySelector('.modal-title, .modal-header h5, .modal-header h4'); | |
| const label = document.getElementById('htmx-modal-label'); | |
| if (header && label) { | |
| label.textContent = header.textContent.trim(); | |
| } | |
| }); | |
| if (!htmxModal) return; | |
| const modalBody = htmxModal.querySelector('#htmx-modal-body') || htmxModal; | |
| const header = modalBody.querySelector('.modal-title, .modal-header h5, .modal-header h4'); | |
| const labelId = htmxModal.getAttribute('aria-labelledby'); | |
| const label = (labelId && document.getElementById(labelId)) || document.getElementById('htmx-modal-label'); | |
| if (header && label && header !== label) { | |
| label.textContent = header.textContent.trim(); | |
| } | |
| function updateHtmxModalLabel() { | |
| const htmxModal = document.getElementById('htmx-modal'); | |
| if (!htmxModal) return; | |
| const modalContent = htmxModal.querySelector('`#htmx-modal-content`') || htmxModal; | |
| const header = modalContent.querySelector('.modal-title, .modal-header h5, .modal-header h4'); | |
| const labelId = htmxModal.getAttribute('aria-labelledby'); | |
| const label = (labelId && document.getElementById(labelId)) || document.getElementById('htmx-modal-label'); | |
| if (header && label && header !== label) { | |
| label.textContent = header.textContent.trim(); | |
| } | |
| } |
🤖 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/static/netbox_librenms_plugin/js/librenms_sync.js`
around lines 1823 - 1832, The helper updateHtmxModalLabel currently only looks
inside `#htmx-modal-body` for the header, so when HTMX swaps a full fragment into
`#htmx-modal-content` the function keeps the stale accessible label; update the
DOM search to look across the entire modal content (e.g. prefer
htmxModal.querySelector('`#htmx-modal-body`') ||
htmxModal.querySelector('`#htmx-modal-content`') || htmxModal) and find the header
via a query on that resolved container (or directly on htmxModal) using the same
selectors ('.modal-title, .modal-header h5, .modal-header h4') before copying
text into the aria-labelledby element in updateHtmxModalLabel.
- tests/test_unique_constraints.py: add found-flag sentinel so test_global_constraint_excludes_manufacturer_field fails if the expected constraint is absent rather than passing vacuously. - views/imports/actions.py (CreatePlatformFromImportView): stop swallowing exceptions from get_validated_device_with_selections; log via logger.exception and return an HTMX error response so the user is informed rather than silently getting a platform created without device assignment. - views/mapping_views.py (BulkExportYAMLView): reject empty YAML exports with 400 Bad Request when no PKs are supplied or none match the queryset, instead of returning a 200 with empty content. - views/object_sync/devices.py (ModuleVerifyView.post): wrap json.loads in try/except and return 400 on malformed JSON instead of letting a 500 bubble up.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
netbox_librenms_plugin/views/imports/actions.py (1)
735-739:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn an HTMX toast here instead of a raw 500.
This catch-all still sends plain text for HTMX requests, so an unexpected import failure can replace the modal target instead of surfacing the error through the toast/OOB path this module now standardizes on.
💡 Suggested fix
except Exception: # pragma: no cover - defensive guard logger.exception("Error during bulk import") if request.headers.get("HX-Request"): - return HttpResponse("Import failed. Please check server logs.", status=500) + return _htmx_error_response("Bulk import failed. Please check server logs.") messages.error(request, "Bulk import failed. Please check server logs.") return redirect("plugins:netbox_librenms_plugin:librenms_import")🤖 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/views/imports/actions.py` around lines 735 - 739, The catch-all except block currently logs via logger.exception and returns a plain HttpResponse when request.headers.get("HX-Request") is truthy; change this to return the standard HTMX OOB toast instead of raw text: keep logger.exception(...) and messages.error(request, "Bulk import failed. Please check server logs."), then for the HTMX branch replace the plain HttpResponse with a response that triggers the app's HTMX toast flow (e.g., return rendered toast markup or an HttpResponse with the appropriate HX-Trigger header/event that your frontend listens for) so the error is shown as an OOB toast rather than replacing the modal; update the branch that checks request.headers.get("HX-Request") accordingly in the same except block.
🤖 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.
Inline comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1725-1733: The code currently swallows IntegrityError when
creating the requested PlatformMapping (PlatformMapping.objects.create) which
can leave a newly created netbox Platform (variable platform) without its
mapping; instead make the creation of the PlatformMapping atomic with the
platform creation when create_mapping and librenms_os are set: either perform
both creations inside a transaction (use transaction.atomic) so a mapping
IntegrityError rolls back the platform, or if you must create the platform first
then catch IntegrityError and delete/rollback the created platform and
re-raise/log the error instead of passing; do not silently pass on
IntegrityError for PlatformMapping.objects.create.
---
Duplicate comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 735-739: The catch-all except block currently logs via
logger.exception and returns a plain HttpResponse when
request.headers.get("HX-Request") is truthy; change this to return the standard
HTMX OOB toast instead of raw text: keep logger.exception(...) and
messages.error(request, "Bulk import failed. Please check server logs."), then
for the HTMX branch replace the plain HttpResponse with a response that triggers
the app's HTMX toast flow (e.g., return rendered toast markup or an HttpResponse
with the appropriate HX-Trigger header/event that your frontend listens for) so
the error is shown as an OOB toast rather than replacing the modal; update the
branch that checks request.headers.get("HX-Request") accordingly in the same
except block.
🪄 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: 965c4644-d0dc-4029-8d2f-938490b22a7f
📒 Files selected for processing (4)
netbox_librenms_plugin/tests/test_unique_constraints.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/object_sync/devices.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use permission constants in
constants.py:PERM_VIEW_PLUGINandPERM_CHANGE_PLUGINinstead of hardcoding permission stringsUse
_get_safe_redirect_url(request)to validate referrer URLs in views to prevent open-redirect attacksWhen a view builds an
HttpResponsefrom Django-template-rendered HTML, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Only usemark_safe()when HTML comes from server-rendered Django views that auto-escape values; never pass untrusted user input tomark_safe()Do NOT use
# lgtm[py/reflected-xss]syntax — it is LGTM.com legacy and is not honoured by GitHub's modern CodeQL Action
Files:
netbox_librenms_plugin/tests/test_unique_constraints.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/imports/actions.py
**/views/object_sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Object sync view methods must create instances of concrete table views, copy the
requestobject, and callget_context_data(). VMs must skip cables and VLANs by returningNonefrom thoseget_*_context()methods.
Files:
netbox_librenms_plugin/views/object_sync/devices.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Hook into NetBox (Django 5) under `netbox_librenms_plugin/` directory; respect NetBox plugin APIs (`navigation.py`, `urls.py`, `api/`)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Reuse `librenms_api.py` client for LibreNMS communication instead of making new `requests` calls; it handles multi-server configs via `LibreNMSSettings` model and `servers` plugin config, plus caching
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Views follow a three-layer structure: Base views (`views/base/`), Object sync views (`views/object_sync/`), and Sync action views (`views/sync/`) with shared mixins in `views/mixins.py`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: All four sync resources (interfaces, cables, IP addresses, VLANs) follow the same three-layer pattern; new views should extend the closest base class and compose mixins
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: VLAN sync additionally uses `VlanAssignmentMixin` for VLAN group scope resolution (Rack → Location → Site → SiteGroup → Region → Global)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Tables (`tables/*.py`) and templates (`templates/netbox_librenms_plugin/`) drive the UI; follow HTMX, template, and styling conventions in `frontend.instructions.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Use `import_validation_helpers.py` to centralize validation state mutation during import (role/cluster/rack assignment, issue removal, status recalculation) instead of scattering validation logic
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Always call `LibreNMSAPI.get_librenms_id` to retrieve the `librenms_id` custom field instead of touching the field directly; Devices/VMs map to LibreNMS via this field and are cached if absent
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Use exact-only matching for site, platform, device type, and role via utility functions (`find_matching_site`, `match_librenms_hardware_to_device_type`, `find_matching_platform` in `utils.py`); do not add fuzzy matching
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Sync pipelines should fetch LibreNMS data (`librenms_api.py`), cache it (`CacheMixin`), build comparison tables (`tables/`), and render HTMX fragments (`templates/netbox_librenms_plugin/htmx/`); follow this flow for new resources
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Virtual chassis support must use `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-19T12:32:25.906Z
Learning: Use devcontainer commands (`netbox-run`, `netbox-run-bg`, `netbox-reload`, `netbox-logs`) described in `.devcontainer/README.md` instead of manual commands; they manage NetBox + plugin reloading
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Place static assets in `static/netbox_librenms_plugin/`; run NetBox's `collectstatic` when bundling, but the devcontainer handles this automatically
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: REST endpoints for imports live in `views/imports/actions.py` (with list view in `views/imports/list.py`) and surface via `urls.py`; emit HTMX fragments and keep server responses and HTMX targets in sync
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Register navigation and menu items in `navigation.py` for new sections so NetBox renders links correctly
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Use two-tier permissions via `LibreNMSSettings` model with `view_librenmssettings` (read) and `change_librenmssettings` (write) permissions as documented in `docs/development/permissions.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: All views must inherit `LibreNMSPermissionMixin` from `views/mixins.py`, which sets `permission_required = PERM_VIEW_PLUGIN` and provides `has_write_permission()`, `require_write_permission()`, and `require_write_permission_json()` methods
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Use `NetBoxObjectPermissionMixin` to add object-level permission checking for NetBox model operations (add/change/delete on Device, Interface, VLAN, etc.) via `required_object_permissions` dict
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Declare `required_object_permissions` dict mapping HTTP methods to `[(action, Model)]` tuples, e.g., `required_object_permissions = {"POST": [("add", VLAN), ("change", VLAN)]}` in sync views
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Some views set `required_object_permissions` dynamically per-request based on object type (e.g., `SyncInterfacesView` switches between `Interface` and `VMInterface`)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Background jobs run outside view context and cannot use view mixins; use standalone helpers from `import_utils/permissions.py` (`check_user_permissions`, `require_permissions`) instead. See `background-jobs.instructions.md` for details
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Background job polling requires superuser; non-superusers fall back to synchronous mode. Use permission helpers from `import_utils/permissions.py`. See `background-jobs.instructions.md` for details
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Navigation menu has 3 groups: **Settings** (Plugin Settings, Interface Mappings), **Import** (LibreNMS Import), **Status Check** (Site & Location Sync, Device Status, VM Status); all items use `permissions=[PERM_VIEW_PLUGIN]`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Check `docs/development/` for structure, view inheritance, mixins, and template conventions before introducing new patterns; review existing sync views (e.g., `views/sync/interfaces.py`) as reference implementations
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T12:32:25.906Z
Learning: Coordinate schema changes through Django migrations in `migrations/` directory and update `models.py` plus admin/pydantic representations accordingly
📚 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/tests/test_unique_constraints.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/imports/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/tests/test_unique_constraints.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/imports/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/tests/test_unique_constraints.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/imports/actions.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_unique_constraints.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_unique_constraints.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_unique_constraints.py
📚 Learning: 2026-03-08T08:57:43.392Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/object_sync/devices.py:83-100
Timestamp: 2026-03-08T08:57:43.392Z
Learning: In views under netbox_librenms_plugin/views/object_sync, server_key values come from admin-controlled PLUGINS_CONFIG dict keys (e.g., "default", "production") and are not user input. Therefore URL-encoding them via urlencode() is unnecessary defensiveness. Do not flag direct string interpolation of server_key into query strings as a URL-injection or encoding issue. This guidance should apply to similar views in the same directory.
Applied to files:
netbox_librenms_plugin/views/object_sync/devices.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-05T09:58:50.179Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/object_sync/devices.py:57-75
Timestamp: 2026-05-05T09:58:50.179Z
Learning: In object_sync view classes that pass Django/NetBox `request` into child table context helpers (e.g., for interfaces/cables/IPs/vlans/modules), ensure the child view stores `copy.copy(request)` rather than the original `request` object. Apply this consistently across similar sync views (such as the pattern used in `VMLibreNMSSyncView` in `vms.py`) to prevent cross-view request mutation when the child view modifies the request.
Applied to files:
netbox_librenms_plugin/views/object_sync/devices.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
🪛 OpenGrep (1.21.0)
netbox_librenms_plugin/views/imports/actions.py
[WARNING] 1528-1528: 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] 1546-1546: 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] 1750-1750: 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] 1759-1759: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
Comment on lines
+1725
to
+1733
| if create_mapping and librenms_os: | ||
| if not PlatformMapping.objects.filter(librenms_os__iexact=librenms_os).exists(): | ||
| try: | ||
| PlatformMapping.objects.create( | ||
| librenms_os=librenms_os.lower(), | ||
| netbox_platform=platform, | ||
| ) | ||
| except IntegrityError: | ||
| pass |
There was a problem hiding this comment.
Don't silently drop the requested PlatformMapping.
When create_mapping is checked, this branch can still commit the new Platform while skipping the OS mapping entirely. That turns a requested atomic action into a partial success and can leave the row unresolved or create an orphan platform if another mapping already exists or is inserted concurrently.
💡 Suggested fix
if create_mapping and librenms_os:
- if not PlatformMapping.objects.filter(librenms_os__iexact=librenms_os).exists():
- try:
- PlatformMapping.objects.create(
- librenms_os=librenms_os.lower(),
- netbox_platform=platform,
- )
- except IntegrityError:
- pass
+ if PlatformMapping.objects.filter(librenms_os__iexact=librenms_os).exists():
+ raise ValidationError(
+ {"librenms_os": f'Platform mapping for "{librenms_os}" already exists.'}
+ )
+ try:
+ PlatformMapping.objects.create(
+ librenms_os=librenms_os.lower(),
+ netbox_platform=platform,
+ )
+ except IntegrityError:
+ raise ValidationError(
+ {"librenms_os": f'Platform mapping for "{librenms_os}" was created concurrently. Please retry.'}
+ )🤖 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/views/imports/actions.py` around lines 1725 - 1733,
The code currently swallows IntegrityError when creating the requested
PlatformMapping (PlatformMapping.objects.create) which can leave a newly created
netbox Platform (variable platform) without its mapping; instead make the
creation of the PlatformMapping atomic with the platform creation when
create_mapping and librenms_os are set: either perform both creations inside a
transaction (use transaction.atomic) so a mapping IntegrityError rolls back the
platform, or if you must create the platform first then catch IntegrityError and
delete/rollback the created platform and re-raise/log the error instead of
passing; do not silently pass on IntegrityError for
PlatformMapping.objects.create.
mapping_views.py: use 'if not objects' instead of 'objects.exists()' so the guard works with both real QuerySets and list mocks in tests. test_platform_mapping.py: update test_returns_200_with_empty_selection to expect 400 (behaviour intentionally changed in previous commit). views/imports/actions.py: wrap PlatformMapping.objects.create in a nested transaction.atomic() savepoint so a concurrent-create IntegrityError rolls back only the savepoint and not the outer platform creation transaction. Without the savepoint, PostgreSQL marks the outer transaction as aborted the moment the IntegrityError is raised, even when caught.
Add module-level _parse_request_json(request) -> (data, error_response) helper that wraps json.loads in try/except (TypeError, ValueError) and returns a 400 JsonResponse on malformed input. Replace all five bare json.loads(request.body) calls in devices.py with the helper, including the one in SingleInterfaceVerifyView that was previously unguarded and the ad-hoc try/except added in ModuleVerifyView in the prior commit.
views/mixins.py: promote _parse_request_json to the shared module so it can be used by both base/ and object_sync/ views without layering issues. views/object_sync/devices.py: import _parse_request_json from mixins (removes the local definition added in the previous commit). views/base/cables_view.py: use _parse_request_json to guard the bare json.loads call in SingleCableVerifyView.post, returning 400 on malformed JSON instead of a 500. test_import_utils.py: rename three test methods that still said 'returns_400' but were asserting 200 + HX-Reswap=none, and update their docstrings to match the actual contract. test_coverage_sync_views2.py: replace tautological 'mock_msgs.error.call_count >= 0' with '>= 1' so the invalid-form test actually fails when no errors are shown; replace the weak 'or mock_vlan_cls.objects.get_or_create.called' grouped-VLAN assertion with assert_called_once_with exact kwargs.
For tests where the 200+HX-Reswap envelope hides which error branch ran, assert on the response body content too so the test fails if a different branch returns the same shell. Rename methods/docstrings that still claimed 400/404/409 to match the actual contract.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
netbox_librenms_plugin/tests/test_coverage_actions.py (1)
50-73: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider updating test method names to match the new assertions.
The test methods
test_validation_error_returns_400andtest_integrity_error_returns_409now assertstatus_code == 200withHX-Reswap: none, but their names still reference the old HTTP error codes. This could confuse future maintainers.📝 Suggested test name updates
- def test_validation_error_returns_400(self): + def test_validation_error_renders_htmx_error_toast(self): from django.core.exceptions import ValidationError from netbox_librenms_plugin.views.imports.actions import _save_device device = MagicMock() device.full_clean.side_effect = ValidationError({"name": ["This field is required."]}) response = _save_device(device) assert response.status_code == 200 assert response.headers.get("HX-Reswap") == "none" - def test_integrity_error_returns_409(self): + def test_integrity_error_renders_htmx_error_toast(self): from django.db import IntegrityError from netbox_librenms_plugin.views.imports.actions import _save_device device = MagicMock() device.full_clean.return_value = None device.save.side_effect = IntegrityError("duplicate key") response = _save_device(device) assert response.status_code == 200 assert response.headers.get("HX-Reswap") == "none"🤖 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/tests/test_coverage_actions.py` around lines 50 - 73, Rename the two tests whose names still reference 400/409 to reflect the current assertions that _save_device returns status_code 200 and sets HX-Reswap to "none"; specifically update test_validation_error_returns_400 -> something like test_validation_error_returns_200_with_hx_reswap_none and test_integrity_error_returns_409 -> test_integrity_error_returns_200_with_hx_reswap_none (or similar clearer names), and update any related docstrings/comments in the same test functions so the names match the assertions and avoid confusion when locating the _save_device behavior being validated.
🤖 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/tests/test_coverage_actions.py`:
- Around line 50-73: Rename the two tests whose names still reference 400/409 to
reflect the current assertions that _save_device returns status_code 200 and
sets HX-Reswap to "none"; specifically update test_validation_error_returns_400
-> something like test_validation_error_returns_200_with_hx_reswap_none and
test_integrity_error_returns_409 ->
test_integrity_error_returns_200_with_hx_reswap_none (or similar clearer names),
and update any related docstrings/comments in the same test functions so the
names match the assertions and avoid confusion when locating the _save_device
behavior being validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c90626ef-aa3e-4c2d-a3f9-89463929f2e8
📒 Files selected for processing (9)
netbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_platform_mapping.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
netbox_librenms_plugin/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Hook the plugin into NetBox (Django 5) under
netbox_librenms_plugin/and respect NetBox plugin APIs (navigation.py,urls.py,api/)Reuse the
librenms_api.pyclient for all LibreNMS communication instead of making newrequestscalls; it handles multi-server configs viaLibreNMSSettingsmodel and theserversplugin config, plus caching via Django cache and custom fieldsAlways call
LibreNMSAPI.get_librenms_idto retrieve thelibrenms_idcustom field mapping for Devices/VMs instead of touching the field directly; results are cached if absentUse only exact matching (no fuzzy matching) for site, platform, device type, and role via the helper functions in
utils.py(find_matching_site,match_librenms_hardware_to_device_type,find_matching_platform)Use
get_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selection in virtual chassis contextsUse the two-tier permission system:
view_librenmssettings(read) andchange_librenmssettings(write) via theLibreNMSSettingsmodel; use permission constantsPERM_VIEW_PLUGINandPERM_CHANGE_PLUGINfromconstants.pyBackground job polling requires superuser status; non-superusers fall back to synchronous mode (see
background-jobs.instructions.mdfor details)
Files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_platform_mapping.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_actions.py
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/cables_view.py
netbox_librenms_plugin/views/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Views should follow a three-layer structure: base views in
views/base/, concrete object sync views inviews/object_sync/registered via@register_model_view(), and POST-only sync action views inviews/sync/; new views should extend the closest base class and compose mixinsAll views must inherit
LibreNMSPermissionMixinfromviews/mixins.pyand setpermission_required = PERM_VIEW_PLUGIN; usehas_write_permission(),require_write_permission()(returns redirect/HTMX), orrequire_write_permission_json()(returns 403 JSON) for write access checksUse
NetBoxObjectPermissionMixinfor a second layer of permission checking on NetBox model operations (add/change/delete); declarerequired_object_permissionsas a dict mapping HTTP methods to[(action, Model)]tuples (e.g.,{"POST": [("add", VLAN), ("change", VLAN)]})Use
_get_safe_redirect_url(request)to validate referrer URLs and prevent open-redirect attacks in viewsWhen building HttpResponse from Django-template-rendered HTML via
.content.decode(), useformat_html()to compose the envelope andmark_safe()as a trust assertion on inner HTML to clear CodeQLpy/reflected-xssfalse positives; only usemark_safe()on server-rendered HTML, never on untrusted user input
Files:
netbox_librenms_plugin/views/base/cables_view.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T13:35:33.615Z
Learning: Sync pipelines should follow the flow: fetch LibreNMS data (`librenms_api.py`), cache it (`CacheMixin`), build comparison tables (`tables/`), and render HTMX fragments (`templates/netbox_librenms_plugin/htmx/`); follow this pattern for new resources
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T13:35:33.615Z
Learning: Prefer devcontainer commands (`netbox-run`, `netbox-run-bg`, `netbox-reload`, `netbox-logs`) for development as described in `.devcontainer/README.md` to manage NetBox and plugin reloading
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T13:35:33.615Z
Learning: Keep REST endpoint responses and HTMX targets in sync between import action views (`views/imports/actions.py`) and HTMX fragments (`templates/netbox_librenms_plugin/htmx/device_import_row.html`, etc.)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T13:35:47.982Z
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-19T13:35:47.982Z
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 should have tests 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-19T13:35:47.982Z
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-19T13:35:47.982Z
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-19T13:35:47.982Z
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-19T13:35:47.982Z
Learning: Permission mixins, API permissions, and constants should be tested in test_permissions.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T13:35:47.982Z
Learning: VLAN API, mode detection, comparison, and sync should be tested in test_vlan_sync.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-19T13:35:47.982Z
Learning: VlanAssignmentMixin and VLAN enrichment should be tested in test_interface_vlan_sync.py.
📚 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/tests/test_import_utils.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_platform_mapping.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.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/tests/test_import_utils.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_platform_mapping.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.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/tests/test_import_utils.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_platform_mapping.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_actions.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_import_utils.pynetbox_librenms_plugin/tests/test_platform_mapping.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.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_import_utils.pynetbox_librenms_plugin/tests/test_platform_mapping.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.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_import_utils.pynetbox_librenms_plugin/tests/test_platform_mapping.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-03-07T09:14:06.791Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/base/cables_view.py:324-331
Timestamp: 2026-03-07T09:14:06.791Z
Learning: In netbox_librenms_plugin/views/base/cables_view.py, do not treat cache.ttl() usage as a portability issue. NetBox requires Redis as the cache backend (since NetBox v2.6), so django-redis cache.ttl() and cache.pttl() extensions are available. Consider this as a project-specific guideline: cache.ttl() is intentional/safe in this codebase.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.py
🔇 Additional comments (8)
netbox_librenms_plugin/tests/test_import_utils.py (1)
2850-2860: LGTM!Also applies to: 2862-2889, 2934-2962
netbox_librenms_plugin/views/base/cables_view.py (1)
19-24: LGTM!Also applies to: 422-424
netbox_librenms_plugin/tests/test_coverage_sync_views2.py (1)
120-120: LGTM!Also applies to: 143-143, 149-150, 153-153, 172-173, 189-190, 196-197, 200-200, 472-478, 486-487, 497-498, 506-507, 515-516, 561-562, 605-606, 649-650, 697-698, 763-770, 771-786, 1508-1512
netbox_librenms_plugin/tests/test_platform_mapping.py (1)
337-340: ⚡ Quick winLikely an incorrect or invalid review comment.
netbox_librenms_plugin/tests/test_coverage_actions.py (4)
3699-3728: LGTM!
425-430: LGTM!Also applies to: 968-971
4015-4042: LGTM!
3361-3382: LGTM!
- librenms_api.add_device(): omit empty SNMPv3 credential keys instead of sending empty strings. The form now allows authpass/authalgo/ cryptopass/cryptoalgo to be blank at noAuthNoPriv / authNoPriv, but the API call was still pushing all six keys regardless of value, which LibreNMS rejects. - utils._get_librenms_sync_device(): reject 0, negative, and non-numeric librenms_id values when picking a VC member. The prior check only guarded against None / bool, letting invalid IDs win priority. - tables/__init__: re-export CarrierAutoInstallRuleTable for parity with the other mapping tables. - device_validation_details.html: add aria-label to the icon-only platform-create button (title alone is not a reliable accessible name). - test_coverage_actions: restore test_device_not_found_returns_404 on TestDeviceVCDetailsView — that test asserts an actual 404 and was swept up by a global rename meant only for HTMX-toast tests.
…ape tests _get_librenms_poller_group_choices() used to fall back to the unscoped "librenms_poller_group_choices" key when LibreNMSAPI() init raised, which could leak choices across servers. Now returns defaults on init failure without touching the cache. Also add regression tests for the malformed-payload branches in get_device_info(), get_device_transceivers(), get_device_vlans(), and get_port_vlan_details().
This was referenced May 26, 2026
Merged
Merged
Closed
This was referenced Jun 14, 2026
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
3 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
Bug Fixes
Documentation
Tests