feat: inventory enhancement - #67
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
netbox_librenms_plugin/tables/modules.py (1)
336-339:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't return before the mapping/type-creation branches run.
This guard makes the later
Add Carrier Rule,Add Mapping, andAdd Module Typebranches unreachable for users who can curate those resources but do not have module add/change rights. Split the gating by action type instead of treating every button as a module mutation.🤖 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/tables/modules.py` around lines 336 - 339, The current early-return uses has_write_permission and a combined can_add_module/can_change_module check which prevents the later "Add Carrier Rule", "Add Mapping", and "Add Module Type" branches from rendering; modify the gating so only the module-mutation buttons are blocked by the can_add_module or can_change_module check while the mapping/type/carrier-rule buttons are gated by their own permissions (e.g., can_add_mapping, can_add_module_type, can_add_carrierrule) or by has_write_permission as appropriate; locate the permission checks around has_write_permission, can_add_module and can_change_module in the method that builds the action buttons and split them so each branch checks the correct permission symbol before returning/omitting its button.
🤖 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/tables/modules.py`:
- Around line 241-275: render_status() currently always returns clickable "Fix
Model"/"Fix Device Type" badges via _render_fix_bay_template_badge even for
users who lack permission to add ModuleBayTemplate; change render_status() to
consult a boolean flag (can_add_module_bay_template) passed into the table (or
set on the table instance) and, if that flag is false, return the plain status
text (status_html + possible_carrier_html) instead of calling
_render_fix_bay_template_badge. Apply the same check in the second block that
handles the device_type_incomplete path (the other occurrence noted). Ensure the
flag name is exactly can_add_module_bay_template and that render_status() uses
record.get(...) only for model/device fields while relying on the table-level
can_add_module_bay_template to decide whether to render a clickable badge.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 865-883: Re-run the serial-conflict query inside the transaction
that locks the target row: move the logic that computes conflict_module into the
transaction.atomic() block after acquiring the lock on installed_module and use
Module.objects.filter(serial=serial).exclude(pk=installed_module.pk).select_for_update().select_related(...)
to re-derive and lock the candidate rows; then only proceed to delete when the
locked queryset count is exactly 1 (otherwise abort/send message). Replace the
prior pre-transaction conflict_module usage with this re-queried, locked result
so you never delete a stale or concurrently-modified module.
---
Duplicate comments:
In `@netbox_librenms_plugin/tables/modules.py`:
- Around line 336-339: The current early-return uses has_write_permission and a
combined can_add_module/can_change_module check which prevents the later "Add
Carrier Rule", "Add Mapping", and "Add Module Type" branches from rendering;
modify the gating so only the module-mutation buttons are blocked by the
can_add_module or can_change_module check while the mapping/type/carrier-rule
buttons are gated by their own permissions (e.g., can_add_mapping,
can_add_module_type, can_add_carrierrule) or by has_write_permission as
appropriate; locate the permission checks around has_write_permission,
can_add_module and can_change_module in the method that builds the action
buttons and split them so each branch checks the correct permission symbol
before returning/omitting its button.
🪄 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: aa9982c6-a826-4a21-a405-14e664311e77
📒 Files selected for processing (8)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.htmlnetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/modules.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX 2.x is the primary async layer. Table row updates must return
<tr hx-swap-oob="true">.Avoid
outerHTMLswaps; use OOB (Out-of-Band) or targetedinnerHTMLswaps to keep table layout intact.Do not reintroduce
data-bs-toggleor duplicate modal IDs in modal implementations.Keep
<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.Styling assumes Tabler defaults. Do not re-add
table-responsivewrappers as they were deliberately removed to prevent dropdown clipping.Templates live in
templates/netbox_librenms_plugin/; reuse/includes should be placed underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
netbox_librenms_plugin/templates/**/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments in
templates/netbox_librenms_plugin/htmx/must keep server responses and HTMX targets in sync when modifying fragments likedevice_import_row.html,device_validation_details.html,device_vc_details.html, andbulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/modules.py
**/views/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
Custom sync endpoint
api/views.py::sync_job_status()syncs database Job status with RQ job status, needed because NetBox worker doesn't always update DB when jobs stop before processing starts
Files:
netbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/__init__.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/static/**/*.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]').valueover cookie-basedgetCookie('csrftoken').Modals should try Bootstrap 5 native (
bootstrap.Modal) first, falling back to manual DOM manipulation if unavailable. UseshowModal()/hideModal()helper functions for consistency.The import page must use
ModalManagerclass andfilterModalManagerinstance in fetch callbacks, not undefinedmodalInstancevariables.Dismiss handlers (backdrop click,
data-bs-dismissbuttons) must be bound once per element to prevent stacking on repeatedshowModal()calls.Always check
response.okbefore processing fetch responses to catch HTTP errors.In fetch catch blocks, show
error.messagefor debugging rather than generic messages.The import filter form should use fetch with
Accept: application/json, text/htmlheader—JSON for background jobs, HTML for synchronous mode.
ModalManagerclass must wrap Bootstrap 5 modal show/hide with fallback support.
pollJobStatus()function must poll/api/core/background-tasks/{jobId}/every 2s, update progress messages, handle cancel button, and redirect on completion.
captureSelectionState()andrestoreSelectionState()functions must preserve checkbox state across HTMX content swaps.
createCacheCountdown()function must be a generic countdown timer for cache expiration display.
initializeFilterForm()must intercept form submit, detect JSON response for background jobs, start polling for asynchronous mode, and return HTML for synchronous mode.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/modules.py
**/__init__.py
📄 CodeRabbit inference engine (.github/instructions/release.instructions.md)
Update
__version__toX.Y.Zin a single commit with messageBump version to X.Y.Z and update changelogalongsidepyproject.tomlandchangelog.mdupdates.
Files:
netbox_librenms_plugin/views/__init__.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Plugin hooks into NetBox (Django 5) under `netbox_librenms_plugin/`; respect NetBox plugin APIs (`navigation.py`, `urls.py`, `api/`).
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: LibreNMS communication lives in `librenms_api.py`; reuse this client instead of new `requests` calls. It handles multi-server configs via `LibreNMSSettings` model and the `servers` plugin config, plus caching via Django cache + custom fields.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: All four sync resources (interfaces, cables, IP addresses, VLANs) should follow the same three-layer pattern: base views, object sync views, sync action views.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: New views should extend the closest base class and compose mixins.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Devices/VMs map to LibreNMS via the `librenms_id` custom field. Always call `LibreNMSAPI.get_librenms_id` instead of touching the field directly.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Matching should be intentionally exact-only for site, platform, device type, and role. Do not add fuzzy matching.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
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/`).
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Virtual chassis support should 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-11T07:02:18.861Z
Learning: Prefer the devcontainer commands (`netbox-run`, `netbox-run-bg`, `netbox-reload`, `netbox-logs`) described in `.devcontainer/README.md`. They manage NetBox + plugin reloading.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Uses two-tier permissions via `LibreNMSSettings` model: `view_librenmssettings` (read) and `change_librenmssettings` (write). See `docs/development/permissions.md`.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: `NetBoxObjectPermissionMixin` should be used for a second layer of permission checking for NetBox model operations (add/change/delete on Device, Interface, VLAN, etc.).
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Some views should set `required_object_permissions` dynamically per-request (e.g., `SyncInterfacesView` switches between `Interface` and `VMInterface` based on object type).
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Background job polling requires superuser; non-superusers fall back to synchronous mode.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: `mark_safe()` is a trust assertion, not a sanitizer. Only use it when the HTML comes from a server-rendered Django view (whose templates auto-escape all user values). Never pass untrusted user input to `mark_safe()`.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Do NOT use `# lgtm[py/reflected-xss]` — that is LGTM.com legacy syntax and is NOT honoured by GitHub's modern CodeQL Action.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Check docs in `docs/development/` for structure, view inheritance, mixins, and template conventions before introducing new patterns.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Review existing sync views (e.g., `views/sync/interfaces.py`) as reference implementations for data flow and caching patterns.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:02:18.861Z
Learning: Coordinate schema changes through Django migrations in `migrations/` and update `models.py` + admin/pydantic representations accordingly.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:03.504Z
Learning: Prefer updating the table renderer in Python rather than templates when changing row actions, as tables emit HTMX-enabled columns and buttons from `tables/*.py`.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:11.820Z
Learning: Follow the standard release workflow: create `release/X.Y.Z` branch from develop, commit version bump to this branch, create PR to develop, then create PR from develop to master. Tag `vX.Y.Z` on master and create GitHub release.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:11.820Z
Learning: Never push directly to `develop` or `master` branches; all changes must go through pull requests due to branch protection rules.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:11.820Z
Learning: Version bump PR from `release/X.Y.Z` to `develop` should use title 'Bump version to X.Y.Z and update changelog' with body template including Summary, Motivation, Scope of Change, Testing, Risk Assessment, and Backwards Compatibility sections.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:11.820Z
Learning: Release PR from `develop` to `master` should use title 'Release X.Y.Z' with body template including Summary, Motivation, Scope of Change, Changes list, Testing, Risk Assessment, and Backwards Compatibility sections.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:11.820Z
Learning: GitHub release body for tag `vX.Y.Z` should include release type header, one-paragraph summary, categorized changes with PR references, upgrade notes, and 'All Changes' section listing only feature/fix PRs (excluding version bump and merge PRs).
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:28.706Z
Learning: librenms_api.py tests should go in test_librenms_api.py and test_librenms_api_helpers.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:28.706Z
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 tests should go 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-11T07:03:28.706Z
Learning: jobs.py and views/imports/list.py tests should go in test_background_jobs.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:28.706Z
Learning: import_utils/bulk_import.py tests should go in test_coverage_bulk_import.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:28.706Z
Learning: Utility helpers (utils.py coverage tests) should go in test_coverage_utils.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:28.706Z
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-11T07:03:28.706Z
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-11T07:03:28.706Z
Learning: VlanAssignmentMixin and VLAN enrichment should be tested in test_interface_vlan_sync.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T07:03:28.706Z
Learning: Views (views/sync/, views/object_sync/, views/imports/actions.py) should not have dedicated test files. Test business logic via the utility modules they call, not via HTTP requests.
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/urls.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/__init__.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/urls.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/__init__.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/urls.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/__init__.py
📚 Learning: 2026-03-07T13:12:59.182Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:445-448
Timestamp: 2026-03-07T13:12:59.182Z
Learning: Across netbox_librenms_plugin/views/sync/*.py, verify redirects and URL rewrites do not append ?server_key=. The active server context is determined from the global setting (settings.selected_server) via BaseLibreNMSSyncView.get(), so request.GET server_key is not used to preserve context. Ensure RemoveServerMappingView and ConvertLegacyLibreNMSIdView (and similar views) rely on the global setting instead of propagating server_key in redirects.
Applied to files:
netbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/modules.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/sync/modules.pynetbox_librenms_plugin/views/__init__.py
📚 Learning: 2026-03-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/modules.py
🪛 HTMLHint (1.9.2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🪛 OpenGrep (1.20.0)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
[WARNING] 1078-1078: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1080-1080: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1081-1081: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1082-1082: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1083-1083: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1084-1084: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1096-1096: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1610-1610: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 1729-1729: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
netbox_librenms_plugin/tables/modules.py
[WARNING] 647-647: 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] 700-700: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
CodeRabbit + Copilot review feedback on commit d0809b4: - views/sync/modules.py: re-derive serial conflict inside the locked atomic block using select_for_update() to close TOCTOU window where a concurrent write could change a module's serial between the check and the delete (CR Critical, comment 3216948214). Ambiguous conflicts raise an internal exception that aborts the transaction with a user-friendly error. - utils.py: drop librenms_name from load_bay_mappings() ordering so first-match-wins regex precedence preserves insertion order rather than depending on lexicographic pattern text (CR Major, 3215678490). - tables/modules.py: thread can_add_module_bay_template flag from view into LibreNMSModuleTable; suppress the 'Fix Model' / 'Fix Device Type' badge entirely when the viewer lacks dcim.add_modulebaytemplate rather than rendering a dead-end control (CR Minor, 3216948208). - views/object_sync/devices.py: pass can_add_module_bay_template at both LibreNMSModuleTable construction sites. - templates/.../device_validation_details.html: use dt.display directly in the device-type autocomplete dropdown, eliminating duplicated manufacturer text on selection (CR Minor, 3215678485). - views/sync/devices.py: invalid object_type now returns HTTP 400 with an escaped echo (matching device_fields.py convention) instead of raising Http404, which would mislead clients into thinking the resource is missing (Copilot Minor, 3215480447). - tests/e2e/conftest.py: replace misleading os.environ.pop call (which had no effect since pyproject.toml drives the setting) with an explanatory docstring noting -p no:django as the proper opt-out (Copilot Minor, 3215480453). Tests updated for the new flag and serial-conflict locking flow; new test added covering badge suppression when viewer lacks the add_modulebaytemplate permission. Full suite: 2896 passed.
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 `@netbox_librenms_plugin/tests/test_coverage_devices.py`:
- Around line 35-58: Update the test assertions to verify identity (not just
equality) so we catch regressions where the original request is reused: in
test_get_interface_context_delegates_to_interface_view replace the equality
check of the child view's request (currently using ==) with an identity check
that child_instance.request is not the original request and assert that the
second positional arg passed into DeviceInterfaceTableView.get_context_data is
the original request (is request) and the third is obj; apply the same
identity-based change to the analogous tests covering lines 59-79, 80-100,
101-121, and 122-141 so each confirms the child view stored copy.copy(request)
(i.e., child_instance.request is not request) rather than reusing the original
request object.
In `@netbox_librenms_plugin/views/object_sync/devices.py`:
- Around line 165-296: Add object-level permission enforcement to
SingleModuleVerifyView by declaring a required_object_permissions class
attribute mapping "POST" to [("view", Device)] and, in post(), after loading
selected_device (and before reading cache or formatting rows), call the view's
object-permission check method (e.g.
self.check_object_permissions(selected_device)) to enforce that the requesting
user can view that Device; ensure this check runs right after selected_device =
get_object_or_404(Device, pk=selected_device_id) and returns an appropriate
error/raises PermissionDenied if not permitted.
In `@netbox_librenms_plugin/views/sync/devices.py`:
- Around line 46-47: The POST sync handler currently calls
self.require_write_permission(), which does not enforce object-level checks;
replace that call with self.require_all_permissions("POST") in the handler to
enforce object-level permissions and remove or stop using
require_write_permission() there. Also add a required_object_permissions mapping
at the view class level (required_object_permissions = {"POST":
["dcim.change_device", "virtualization.change_virtualmachine"]} or the
appropriate NetBox permission strings) so NetBox can resolve the target
Device/VirtualMachine object for permission checks; ensure the handler uses
those class attributes and keep the require_all_permissions("POST") call to gate
the request.
🪄 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: 29e13340-3d41-479e-84d9-ab800f17eb88
📒 Files selected for processing (12)
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/modules.pytests/e2e/conftest.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Reuse the
LibreNMSAPIclient fromlibrenms_api.pyinstead of making new directrequestscalls; it handles multi-server configs viaLibreNMSSettingsmodel and theserversplugin config, plus caching via Django cache and custom fields.Always call
LibreNMSAPI.get_librenms_id()to fetch or cache thelibrenms_idcustom field instead of touching the field directly.Use exact-only matching for site, platform, device type, and role. Do not add fuzzy matching. Use the helper functions
find_matching_site(),match_librenms_hardware_to_device_type(), andfind_matching_platform()fromutils.py.Use virtual chassis support via
get_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selection.
Files:
tests/e2e/conftest.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/views/sync/modules.py
tests/**/*.py
📄 CodeRabbit inference engine (.github/instructions/testing.instructions.md)
Use plain pytest classes, not Django TestCase. Avoid importing TestCase from django.test.
Never use
@pytest.mark.django_dbfor unit tests—mock all database interactions with MagicMock instead.Use inline imports inside test methods to avoid Django initialization at module load time.
Mock NetBox models (Device, Job, User) with MagicMock() instead of creating real instances.
Use assert x == y syntax, not self.assertEqual(x, y) since there is no TestCase inheritance.
Patch deferred/inline imports at their source module (e.g., netbox_librenms_plugin.import_utils.process_device_filters), not the consuming module.
Patch cache where imported (e.g., netbox_librenms_plugin.views.imports.list.cache), not django.core.cache.cache.
Test view decision logic by setting view._filter_form_data = {...} directly, not via HTTP requests.
Never use RequestFactory—mock request objects directly or test method logic in isolation.
Cache key tests must patch get_validated_device_cache_key from import_utils.py; never hardcode key formats like job_123_device_1.
Reuse shared fixtures from tests/conftest.py instead of creating ad-hoc mocks.
Files:
tests/e2e/conftest.py
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.Sync POST handlers must call
require_all_permissions("POST")(not justrequire_write_permission()) and return early if it returns a response. AJAX/JSON endpoints userequire_all_permissions_json("POST").
Files:
netbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
**/views/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
New views should extend the closest base class and compose mixins from
views/mixins.py(e.g.,LibreNMSPermissionMixin,NetBoxObjectPermissionMixin,LibreNMSAPIMixin,CacheMixin,VlanAssignmentMixin).Use
LibreNMSPermissionMixinin all views; it setspermission_required = PERM_VIEW_PLUGINand provideshas_write_permission(),require_write_permission(), andrequire_write_permission_json()methods.Declare
required_object_permissionsdict in views mapping HTTP methods to[(action, Model)]tuples for NetBox object-level permission checking, e.g.,required_object_permissions = {"POST": [("add", VLAN), ("change", VLAN)]}.Use
_get_safe_redirect_url(request)to validate referrer URLs in views to prevent open-redirect attacks.Use
format_html()andmark_safe()together as a trust assertion when composing Django-template-rendered HTML for HTMX responses to fix CodeQL reflected-XSS false positives.mark_safe()is a trust assertion, not a sanitizer — only use it when HTML comes from server-rendered Django views whose templates auto-escape user values.
Files:
netbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/modules.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
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX 2.x is the primary async layer. Table row updates must return
<tr hx-swap-oob="true">.Avoid
outerHTMLswaps; use OOB or targetedinnerHTMLswaps to keep table layout intact.Modal buttons must target the
htmx-modal-contentelement and JavaScript must toggle the wrapper, not reintroducedata-bs-toggleor duplicate modal IDs.Keep
<select class="device-role-select">markup stable to preserve JavaScript hook-up with TomSelect decorators.Do not re-add
table-responsivewrappers as their removal was deliberate to prevent dropdown clipping.Templates must live in
templates/netbox_librenms_plugin/with reuse/includes underinc/.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
netbox_librenms_plugin/**/*.{js,html}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token usingdocument.querySelector('[name=csrfmiddlewaretoken]').valuefrom a hidden form input, withgetCookie('csrftoken')as a fallback.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments must live in
templates/netbox_librenms_plugin/htmx/and include:device_import_row.html,device_validation_details.html,device_vc_details.html,bulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/modules.py
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:19:56.691Z
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 flow for new resources.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:19:56.691Z
Learning: Respect NetBox plugin APIs in `navigation.py`, `urls.py`, and `api/` directories when hooking into NetBox (Django 5).
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:19:56.691Z
Learning: Coordinate any schema changes through Django migrations in `migrations/` and update `models.py` plus admin/pydantic representations accordingly.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:19:56.691Z
Learning: Keep server responses and HTMX targets in sync for REST endpoints in `views/imports/actions.py` and HTMX fragments in `templates/netbox_librenms_plugin/htmx/device_import_row.html`, etc.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:26.793Z
Learning: Job cancellation flow: (1) Call `/api/core/background-tasks/{uuid}/stop/` to stop RQ job, (2) Call plugin's sync endpoint `/api/plugins/librenms_plugin/jobs/{pk}/sync-status/` to update database, (3) Frontend polling detects status changes and redirects appropriately
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:26.793Z
Learning: For polling implementation: Poll `/api/core/background-tasks/{uuid}/` for real-time RQ status, update modal messages based on status values, handle all RQ status values explicitly to avoid infinite polling, and use `cancelInProgress` flag to prevent polling interference during cancellation
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:43.960Z
Learning: Prefer updating the table renderer in Python rather than templates when changing row actions, as tables emit HTMX-enabled columns and buttons from `tables/*.py`.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:43.960Z
Learning: Keep server responses and HTMX targets in sync when modifying HTMX fragments.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:43.960Z
Learning: Styling assumes Tabler defaults.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:52.512Z
Learning: Create a version bump commit with the exact message `Bump version to X.Y.Z and update changelog` that updates `__init__.py`, `pyproject.toml`, and `changelog.md` simultaneously
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:52.512Z
Learning: Follow the standard release branch strategy: create `release/X.Y.Z` branch from develop, create PR `release/X.Y.Z` → develop, then create PR `develop` → master, tag `vX.Y.Z` on master, and create GitHub release
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:52.512Z
Learning: Use the provided PR templates with specific sections (Summary, Motivation/Problem, Scope of Change, How Was This Tested, Risk Assessment, Backwards Compatibility) for version bump PRs and release PRs
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:52.512Z
Learning: Create GitHub release with tag `vX.Y.Z`, title `vX.Y.Z`, and body containing a release type section (Bug Fix Release, Feature Release, or Maintenance Release), a summary paragraph, categorized changes with PR numbers, upgrade notes, and an 'All Changes' section listing only feature/fix PRs
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:20:52.512Z
Learning: Both `develop` and `master` branches have branch protection enabled — all changes must be made through pull requests and never pushed directly
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:21:06.313Z
Learning: librenms_api.py modules should have tests in test_librenms_api.py and test_librenms_api_helpers.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:21:06.313Z
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 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-11T08:21:06.313Z
Learning: jobs.py and views/imports/list.py should have tests in test_background_jobs.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:21:06.313Z
Learning: import_utils/bulk_import.py should have tests in test_coverage_bulk_import.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:21:06.313Z
Learning: Utility helpers (utils.py coverage tests) should have tests in test_coverage_utils.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:21:06.313Z
Learning: Permission mixins, API permissions, and constants should have tests in test_permissions.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:21:06.313Z
Learning: VLAN API, mode detection, comparison, and sync should have tests in test_vlan_sync.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:21:06.313Z
Learning: VlanAssignmentMixin and VLAN enrichment should have tests in test_interface_vlan_sync.py.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T08:21:06.313Z
Learning: Views (views/sync/, views/object_sync/, views/imports/actions.py) should test business logic via utility modules they call, not via HTTP requests.
📚 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:
tests/e2e/conftest.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-27T01:35:32.609Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: tests/e2e/test_module_install.py:132-140
Timestamp: 2026-03-27T01:35:32.609Z
Learning: In Playwright-based E2E tests, avoid using fixed `time.sleep(...)` as the synchronization mechanism. Replace each sleep (e.g., after `btn.click()`, after navigation to the modules tab, and other workflow waiting points) with observable waits tied to real DOM/network transitions such as `page.wait_for_selector(...)`, `page.wait_for_response(...)`, or `page.wait_for_function(...)`. For `tests/e2e/test_module_install.py`, treat the existing fixed-sleep usage as an intentionally deferred improvement tracked in issue `#45`; do not raise it as a new review finding in that file.
Applied to files:
tests/e2e/conftest.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:
tests/e2e/conftest.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_tables_modules.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_module_replace.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/views/sync/modules.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_module_replace.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/views/sync/modules.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_module_replace.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_tables_modules.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_module_replace.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_tables_modules.py
📚 Learning: 2026-03-07T13:12:59.182Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:445-448
Timestamp: 2026-03-07T13:12:59.182Z
Learning: Across netbox_librenms_plugin/views/sync/*.py, verify redirects and URL rewrites do not append ?server_key=. The active server context is determined from the global setting (settings.selected_server) via BaseLibreNMSSyncView.get(), so request.GET server_key is not used to preserve context. Ensure RemoveServerMappingView and ConvertLegacyLibreNMSIdView (and similar views) rely on the global setting instead of propagating server_key in redirects.
Applied to files:
netbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/modules.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/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/modules.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-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-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
📚 Learning: 2026-03-07T10:32:06.242Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:107-117
Timestamp: 2026-03-07T10:32:06.242Z
Learning: In netbox_librenms_plugin/utils.py, keep the Priority 1 loop to guard against None and bool values when accessing raw_cf.get(server_key). Do not replace the Priority 1 condition with a full get_librenms_device_id call. The two-pass design is intentional: Priority 1 performs quick sanity checks, while Priority 2 handles string normalization and full validation by calling get_librenms_device_id(member, server_key, auto_save=False).
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-07T22:38:43.110Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:552-584
Timestamp: 2026-03-07T22:38:43.110Z
Learning: In netbox_librenms_plugin/utils.py, do not propose replacing 'obj.custom_field_data.get("librenms_id") or {}' with a None check. The code intentionally uses 'or {}' to handle falsey values; downstream type guards treat them equivalently since LibreNMS IDs start at 1, making 0 equivalent to 'not set'. Do not modify this logic; keep the existing behavior for all falsey values.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T08:55:46.317Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:532-595
Timestamp: 2026-03-08T08:55:46.317Z
Learning: In netbox_librenms_plugin/utils.py, do not modify set_librenms_device_id to call obj.save(). It is mutator-only and should only update in-memory obj.custom_field_data[...] without persisting. Ensure callers perform persistence: after mutation, run full_clean() and then save() (as seen in device_operations.py around lines ~864-866) or explicit obj.save() after set_librenms_device_id (as in librenms_api.py around lines ~261-262). This pattern prevents coupling mutation with persistence and preserves validation in between.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-09T19:15:13.104Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/utils.py:249-267
Timestamp: 2026-03-09T19:15:13.104Z
Learning: In netbox_librenms_plugin/utils.py, ensure match_librenms_hardware_to_device_type returns None when DeviceTypeMapping.MultipleObjectsReturned is raised (fail-closed per inline comment). Callers must guard for result is None separately from the normal result check (e.g., if result is None: handle; elif result.get('matched'): ... ). Note that the success path uses match_type='mapping' (not 'exact'), distinguishing it from standard part_number/model exact lookups. Consider adding a unit test that asserts None is returned on MultipleObjectsReturned and that callers properly handle both None and dict results.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/modules.py
🪛 OpenGrep (1.20.0)
netbox_librenms_plugin/tables/modules.py
[WARNING] 659-659: 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] 712-712: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (9)
tests/e2e/conftest.py (1)
1-16: Documentation-only conftest addition looks correct.Clear, accurate guidance for explicit E2E invocation and pytest-django loading behavior; no functional or security concerns in this segment.
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html (1)
201-306: Device-type mapping HTMX flow and autocomplete logic look solid.The added mapping form, conditional fallback behavior, debounced autocomplete, stale-response guard, and safe text rendering are all correctly wired for this modal fragment.
netbox_librenms_plugin/views/sync/devices.py (2)
30-41: Good move to strictobject_type-based lookup.This removes ambiguous PK fallback behavior and makes
DevicevsVirtualMachineresolution deterministic.
49-59: HTTP 400 + escaped echo for invalidobject_typeis the right behavior.Treating this as a client error (instead of 404) and escaping the reflected value is a solid correctness/security improvement.
netbox_librenms_plugin/tests/test_coverage_sync_views2.py (3)
120-153: Nice deterministic setup for the cables success-path test.The explicit
device_selection_port1plus matchinglocal_iface.device_idremoves VC-branch ambiguity and makes this path reliably test the intended branch.
495-495: Good isolation of form invalid-path behavior.Patching
_get_librenms_poller_group_choiceshere keeps this test focused on form validation instead of unrelated choice-loading behavior.
761-783: Theseget_object()contract tests are well aligned with the new view behavior.Asserting
Nonefor missing/bogusobject_typeandHttp404passthrough for missing typed objects gives clear coverage of the new control flow.netbox_librenms_plugin/tables/modules.py (1)
75-76: Hide selection column when bay-template-fix is the only available action.The selection column is hidden unless the user has
has_write_permission and can_add_module. That's consistent with the install/install-branch buttons but unrelated to the bay-template badge flow — minor, no action needed since unchecked rows simply skip the install path.netbox_librenms_plugin/views/sync/modules.py (1)
887-924: Race condition fix correctly applied.The serial conflict re-derivation now happens inside the atomic block under
select_for_update()(Lines 912-917), closing the TOCTOU window between conflict detection and deletion. Ambiguous conflicts properly abort via_SerialConflictAmbiguousto roll back the transaction before surfacing a user-facing error.
When the user opens the Add-Bay-Template modal from a 'No Bay' badge and picks a NetBox bay name that differs from the LibreNMS-supplied name (e.g. 'SFM 1' vs Nokia's 'Sfm 1'), bay matching still fails because matching is case-sensitive exact dict lookup. Offer to create a manufacturer-scoped ModuleBayMapping inside the same atomic transaction. Changes: * _derive_bay_template_suggestion now returns librenms_name + librenms_class. * The 'Fix Device Type' / 'Fix Model' badges thread those values into the modal-trigger URL. * AddBayTemplateView.get checks for an existing covering mapping (vendor-scoped or global) and only offers the checkbox when (a) a librenms_name is known, (b) no mapping exists, and (c) the user has add_modulebaymapping permission. * The modal renders the checkbox with inline JS that auto-toggles it based on whether the bay-name input differs from the LibreNMS name. * AddBayTemplateView.post creates the ModuleBayMapping (manufacturer scope = device.device_type.manufacturer, is_regex=False) inside the same transaction.atomic() that creates the bay template, with a TOCTOU re-check against existing mappings.
Extends the auto-mapping checkbox in the Add-Bay-Template modal so that,
when the LibreNMS name and chosen NetBox name share the same alternating
literal/digit skeleton (e.g. 'Sfm 1' vs 'SFM 1', or 'TenGigE0/0/0/0'),
the user can save a single regex mapping that covers every sibling bay
of the same family — instead of having to add one mapping per slot/port.
* New helper AddBayTemplateView._derive_mapping_pattern: conservative,
fires only when both names produce the same token sequence, all digit
runs match in value, literal tokens are equal under case-insensitive
comparison, and at least one digit run exists. Returns the parametrised
'^…(\d+)…$' pattern + '\1' replacement.
* New helper _existing_regex_mapping_covers iterates the (small) set of
vendor-scoped/global regex rows server-side to skip duplicates when a
prior regex already covers the LibreNMS name.
* AddBayTemplateView.get derives the pattern and threads
mapping_pattern + mapping_default_kind into the template.
* Modal renders a radio group ('All bays of this kind' (regex) /
'This bay only' (exact)) only when a pattern is derivable, with a live
preview of what will actually be stored. Default = regex when
derivable. Inline JS mirrors the Python helper for live re-derivation
while the user edits the bay name.
* AddBayTemplateView.post reads mapping_kind, re-derives + validates the
pattern server-side (falls back to exact if the rule no longer fires),
and stores ModuleBayMapping with is_regex/librenms_name/netbox_bay_name
set accordingly. Existing-mapping race check uses the right (exact vs
regex) lookup.
Tests:
* TestDeriveMappingPattern — 8 derivation cases incl. negatives.
* TestAddBayTemplateViewRegexMapping — GET context, POST regex/exact/
fallback/race-skip, 7 cases.
* All previously added mapping-checkbox tests still green.
Verified manually with Playwright: identical names hide the radio,
case-only rename shows regex preview, switching to exact updates the
preview, and a non-derivable rename hides the radio block again.
The old _derive_mapping_pattern required identical token sequences on
both sides — so 'Sfm 1' ↔ 'SFM 1' worked, but '0/FT0' ↔ 'Fan Tray 0'
fell through to exact-only and the user had to add one mapping per
fan tray. The user's chassis routinely produces this shape (libre
'N/FTN' vs NetBox 'Fan Tray N').
Relaxed algorithm:
* Walk libre tokens; assign a numbered group to each *distinct* digit
value, emit back-references for repeats. So '0/FT0' becomes
'^(\d+)/FT\1$' — pattern matches only when both fan-tray digits
agree, exactly the safety property we want.
* Walk NetBox tokens; literals go through verbatim (with backslash
escaping for the replacement string), digits route to the libre
group whose value matches. If a NetBox digit value is absent on the
libre side, derivation returns None — we won't invent a value we
can't extract.
* Keep the round-trip sanity check: pattern must compile, fullmatch
the libre name, and re.sub against the libre name must reproduce
the NetBox name exactly.
Side effects of the relaxation:
* 'Slot 1' / 'Card 1' now derives ('^Slot (\d+)$' → 'Card \1')
— literals on the two sides may legitimately differ in unrelated
ways, the safety constraint is on digit values, not literals.
* 'TenGigE0/0/0/0' (self-map) now collapses the four digit '0' runs
to a single group with three back-references, instead of four
independent groups — same matching set, more constrained, less
greedy.
* 'Slot 1' → 'Slot 2' still returns None (NetBox digit '2' nowhere on
libre side), and 'Slot 1' → 'Slot 1/0' still returns None (extra
NetBox digit '0' nowhere on libre side).
JS mirror in the modal template implements the same algorithm and
includes its own round-trip check (using JS replace() with $1
back-references, converted from the stored \1 form).
Tests:
* TestDeriveMappingPattern updated to match relaxed semantics:
- test_libre_and_nb_have_different_skeletons: new positive case for
'0/FT0' → 'Fan Tray 0'.
- test_literal_difference_with_shared_digit: 'Sfm 1' / 'Card 1' now
derives a pattern (was None before).
- test_multi_digit_skeleton_collapses_repeat_to_backref: documents
the new repeat-collapse behaviour for 'TenGigE0/0/0/0'.
- Existing negative cases (no digit, NetBox digit absent from libre,
extra NetBox digit run) still return None.
Verified manually with Playwright on device 54: clicking 'Fix Device
Type' on the 0/FT0 fan row and typing 'Fan Tray 0' now shows the
regex radio default-selected with preview
'^(\d+)/FT\1$' → 'Fan Tray \1'.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
netbox_librenms_plugin/views/sync/devices.py (1)
46-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse combined permission enforcement for this sync POST path.
At Line 46,
require_write_permission()bypasses object-level ACL checks forDevice/VirtualMachine. This should userequire_all_permissions("POST")and definerequired_object_permissionson the view.Suggested patch
from netbox_librenms_plugin.forms import AddToLIbreSNMPV1V2, AddToLIbreSNMPV3 -from netbox_librenms_plugin.views.mixins import LibreNMSAPIMixin, LibreNMSPermissionMixin +from netbox_librenms_plugin.views.mixins import ( + LibreNMSAPIMixin, + LibreNMSPermissionMixin, + NetBoxObjectPermissionMixin, +) -class AddDeviceToLibreNMSView(LibreNMSPermissionMixin, LibreNMSAPIMixin, View): +class AddDeviceToLibreNMSView(NetBoxObjectPermissionMixin, LibreNMSPermissionMixin, LibreNMSAPIMixin, View): """Add a NetBox device or VM to LibreNMS via the API.""" + required_object_permissions = { + "POST": [("view", Device), ("view", VirtualMachine)], + } @@ - if error := self.require_write_permission(): + if error := self.require_all_permissions("POST"): return errorAs per coding guidelines: "
**/views/sync/**/*.py: Sync POST handlers must callrequire_all_permissions(\"POST\")(not justrequire_write_permission())andViews declare required_object_permissions dict mapping HTTP methods to [(action, Model)] tuples.`"🤖 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/sync/devices.py` around lines 46 - 47, Replace the call to require_write_permission() in this sync POST handler with require_all_permissions("POST") and add a required_object_permissions dict on the view class that maps "POST" to the object-level permission tuples for Device and VirtualMachine (e.g. [( "change", Device ), ( "change", VirtualMachine )] or the appropriate action required by your ACLs); update the view class to declare required_object_permissions so the POST path enforces object-level ACLs rather than bypassing them (use the existing method name require_all_permissions("POST") and the required_object_permissions attribute to locate where to modify the code).netbox_librenms_plugin/tables/modules.py (1)
350-353:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't short-circuit the mapping-curation actions again.
These early returns still prevent the later
No Bay/No Typebranches from running, so users with plugin write access but without module add/change rights lose the "Add Mapping" workflow from this table.🤖 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/tables/modules.py` around lines 350 - 353, The early-return block in modules.py (checking self.has_write_permission, self.can_add_module, self.can_change_module) is preventing the "No Bay"/"No Type" add-mapping UI from rendering for users who have plugin write access but lack module add/change rights; remove or change the second early return so that you only skip module add/change-specific actions instead of returning "" for the whole actions cell. Concretely, keep the has_write_permission guard if needed, but remove or refactor the joint check "if not self.can_add_module and not self.can_change_module: return ''" in the method that builds the actions cell so that the code still reaches the branches that render "No Bay"/"No Type" and the Add Mapping workflow while hiding only the edit/change controls when self.can_add_module/self.can_change_module are false.netbox_librenms_plugin/views/object_sync/devices.py (1)
165-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce object-level
Devicepermissions in this verify endpoint.
device_idis caller-supplied and this returns device-specific module-sync HTML, but there's no NetBox object-permission check afterselected_device = get_object_or_404(...). A user with plugin access can probe devices they shouldn't be able to view. Please declarerequired_object_permissions = {"POST": [("view", Device)]}and fail early with the JSON permission helper before touching cache or formatting the row.As per coding guidelines,
Views declare required_object_permissions dict mapping HTTP methods to [(action, Model)] tuples.🤖 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/object_sync/devices.py` around lines 165 - 201, Add object-level permission enforcement to SingleModuleVerifyView by declaring required_object_permissions = {"POST": [("view", Device)]} on the class and, immediately after selected_device = get_object_or_404(Device, pk=selected_device_id), invoke the view/mixin's JSON object-permission helper to check the "view" permission for selected_device and return the JSON permission error if the user lacks access; only proceed to cache.get(self.get_cache_key(...)) and further processing after that check passes.
🤖 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/tables/modules.py`:
- Around line 640-659: The "Add Module Type" button is shown unconditionally and
can cause a 403 for users without the dcim.add_moduletype permission; before
building the params and appending to buttons, check a permission flag (e.g.,
self.can_add_module_bay_template) or the actual permission on the request
(self.request.user.has_perm("dcim.add_moduletype")) and only render the block
when true; update the module_type_create / record handling around the
buttons.append(...) branch to short-circuit when the flag/permission is false so
the link is suppressed for unauthorized users.
- Around line 328-331: The function _render_fix_bay_template_badge returns a
plain empty string when the viewer lacks dcim.add_modulebaytemplate, which
causes later concatenation (status_html + fix_html) to become an unsafe str and
be auto-escaped; change the early return to return mark_safe("") (and add the
import for django.utils.safestring.mark_safe if not present) so the empty value
preserves HTML safety, or alternatively ensure the final composition uses
format_html() to produce a SafeString.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html`:
- Around line 178-184: The preview currently assigns unescaped user-controlled
values (libreName, pat.pattern, pat.replacement) into preview.innerHTML,
enabling reflected XSS; update the preview rendering logic in the
refresh()/preview update code to build DOM nodes and set node.textContent (or
use document.createElement and appendText) instead of concatenating into
innerHTML, and ensure values coming from derivePattern and the
AddBayTemplateView.get (librenms_name) are treated as plain text; replace uses
of preview.innerHTML = ... with creation of elements (e.g., span/code elements)
and setting their textContent to libreName/current/pat.pattern/pat.replacement
so HTML metacharacters are not parsed.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 233-303: Add a brief user-facing error message to the autocomplete
dropdown when the fetch for device types fails: inside the fetch(...).catch
handler referenced around the fetch call and console.warn (use the same
requestSeq/seq logic and dropdownEl variable), set dropdownEl.innerHTML to a
small disabled "Error" item (similar to the "No results" item), make
dropdownEl.style.display = "block", and optionally clear that message or hide
the dropdown after a short timeout; keep using dropdownEl, searchEl, hiddenEl
and submitBtn as currently used so behavior remains consistent.
---
Duplicate comments:
In `@netbox_librenms_plugin/tables/modules.py`:
- Around line 350-353: The early-return block in modules.py (checking
self.has_write_permission, self.can_add_module, self.can_change_module) is
preventing the "No Bay"/"No Type" add-mapping UI from rendering for users who
have plugin write access but lack module add/change rights; remove or change the
second early return so that you only skip module add/change-specific actions
instead of returning "" for the whole actions cell. Concretely, keep the
has_write_permission guard if needed, but remove or refactor the joint check "if
not self.can_add_module and not self.can_change_module: return ''" in the method
that builds the actions cell so that the code still reaches the branches that
render "No Bay"/"No Type" and the Add Mapping workflow while hiding only the
edit/change controls when self.can_add_module/self.can_change_module are false.
In `@netbox_librenms_plugin/views/object_sync/devices.py`:
- Around line 165-201: Add object-level permission enforcement to
SingleModuleVerifyView by declaring required_object_permissions = {"POST":
[("view", Device)]} on the class and, immediately after selected_device =
get_object_or_404(Device, pk=selected_device_id), invoke the view/mixin's JSON
object-permission helper to check the "view" permission for selected_device and
return the JSON permission error if the user lacks access; only proceed to
cache.get(self.get_cache_key(...)) and further processing after that check
passes.
In `@netbox_librenms_plugin/views/sync/devices.py`:
- Around line 46-47: Replace the call to require_write_permission() in this sync
POST handler with require_all_permissions("POST") and add a
required_object_permissions dict on the view class that maps "POST" to the
object-level permission tuples for Device and VirtualMachine (e.g. [( "change",
Device ), ( "change", VirtualMachine )] or the appropriate action required by
your ACLs); update the view class to declare required_object_permissions so the
POST path enforces object-level ACLs rather than bypassing them (use the
existing method name require_all_permissions("POST") and the
required_object_permissions attribute to locate where to modify the code).
🪄 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: 59e24f33-0cb0-4703-9256-acc4e4b91275
📒 Files selected for processing (14)
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/modules.pytests/e2e/conftest.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Devices/VMs map to LibreNMS via the
librenms_idcustom field, then cached if absent. Always callLibreNMSAPI.get_librenms_idinstead of touching the field directlyMatching is intentionally exact-only for site, platform, device type, and role. See
utils.py(find_matching_site,match_librenms_hardware_to_device_type,find_matching_platform). Do not add fuzzy matchingVirtual chassis support uses
get_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selectionPermission constants in
constants.py:PERM_VIEW_PLUGINandPERM_CHANGE_PLUGINViews declare
required_object_permissionsdict mapping HTTP methods to[(action, Model)]tuples, e.g.{"POST": [("add", VLAN), ("change", VLAN)]}Sync POST handlers must call
require_all_permissions("POST")(not justrequire_write_permission()) and return early if it returns a responseAJAX/JSON endpoints use
require_all_permissions_json("POST")instead of redirect variantsUse
_get_safe_redirect_url(request)to validate referrer URLs to prevent open-redirect attacksBackground jobs run outside view context and cannot use view mixins. Use standalone helpers from
import_utils/permissions.py(check_user_permissions,require_permissions)Background job polling requires superuser — non-superusers fall back to synchronous mode
When a view builds an
HttpResponsefrom Django-template-rendered HTML (decoded via.content.decode()), useformat_html()to compose the envelope andmark_safe()as a trust assertion on the inner HTML to clear CodeQLpy/reflected-xssfalse positives
mark_safe()is a trust assertion, not a sanitizer — only use it when HTML comes from server-rendered Django views whose templates auto-escape all values. Never pass untrusted user input tomark_safe()Do NOT use
# lgtm[py/reflected-xss]— that is LGTM.com legacy syntax and is not honoured by GitHub's modern CodeQL ActionCoordinate any ...
Files:
tests/e2e/conftest.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_coverage_devices.py
tests/**/*.py
📄 CodeRabbit inference engine (.github/instructions/testing.instructions.md)
Use plain pytest classes, not Django TestCase. Avoid
from django.test import TestCase.Never use
@pytest.mark.django_dbfor unit tests—mock all database interactions with MagicMock.Use inline imports inside test methods to avoid Django initialization at module load time.
Mock NetBox models (Device, Job, User) with MagicMock() instead of creating real instances.
Use
assert x == ysyntax, notself.assertEqual(x, y)(no TestCase inheritance).Patch deferred/inline imports at their source module (e.g.,
netbox_librenms_plugin.import_utils.process_device_filters), not the consuming module.Patch
cachewhere imported:netbox_librenms_plugin.views.imports.list.cache, notdjango.core.cache.cache.Test view decision logic by setting
view._filter_form_data = {...}directly, not via HTTP requests.Never use RequestFactory—mock request objects directly or test method logic in isolation.
Cache key tests must patch
get_validated_device_cache_keyfromimport_utils.py; never hardcode key formats likejob_123_device_1.Reuse fixtures from
tests/conftest.pyinstead of creating ad-hoc mocks for Configuration, API client, NetBox objects, HTTP responses, and Import workflow.
Files:
tests/e2e/conftest.py
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX 2.x is the primary async layer for table row updates; return
<tr hx-swap-oob="true">for OOB swapsAvoid
outerHTMLswaps; use OOB or targetedinnerHTMLswaps to keep table layout intactDo not reintroduce
data-bs-toggleor duplicate modal IDs in modal implementationsKeep
<select class="device-role-select">markup stable to preserve JS hook-up with TomSelect decoratorsDo not re-add
table-responsivewrappers as they were deliberately removed to prevent dropdown clipping
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
netbox_librenms_plugin/templates/**/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments live in
templates/netbox_librenms_plugin/htmx/and include:device_import_row.html,device_validation_details.html,device_vc_details.html,bulk_import_confirm.htmlKeep server responses and HTMX targets in sync when modifying HTMX fragment templates
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
**/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
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/modules.py
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Plugin hooks into NetBox (Django 5) under `netbox_librenms_plugin/`; respect NetBox plugin APIs (`navigation.py`, `urls.py`, `api/`)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: LibreNMS communication lives in `librenms_api.py`; reuse this client instead of new `requests` calls. It handles multi-server configs via `LibreNMSSettings` model and the `servers` plugin config, plus caching via Django cache + custom fields
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Base views (`views/base/`) — abstract views for each sync resource (`BaseInterfaceTableView`, `BaseCableTableView`, `BaseIPAddressTableView`, `BaseVLANTableView`)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Object sync views (`views/object_sync/`) — concrete per-model views registered as tabs on NetBox's Device/VM detail pages via `register_model_view(Device, ...)`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Sync action views (`views/sync/`) — POST-only views that apply changes (add/change/delete NetBox objects). Includes `interfaces.py`, `cables.py`, `ip_addresses.py`, `vlans.py`, `devices.py`, `device_fields.py`, `locations.py`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Use shared mixins (`views/mixins.py`) — `LibreNMSPermissionMixin`, `NetBoxObjectPermissionMixin`, `LibreNMSAPIMixin`, `CacheMixin`, `VlanAssignmentMixin` in views
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: All four sync resources (interfaces, cables, IP addresses, VLANs) follow the same three-layer pattern. VLAN sync additionally uses `VlanAssignmentMixin` for VLAN group scope resolution
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: New views should extend the closest base class and compose mixins
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Tables (`tables/*.py`) and templates (`templates/netbox_librenms_plugin/`) drive the UI. See `frontend.instructions.md` for HTMX, template, and styling conventions
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Forms (`forms.py`) include dynamic LibreNMS API-populated choices (location dropdowns, poller groups) and a split-form pattern for settings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `import_validation_helpers.py` centralizes validation state mutation during import (role/cluster/rack assignment, issue removal, status recalculation)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Sync pipelines generally fetch LibreNMS data (`librenms_api.py`), cache it (`CacheMixin`), build comparison tables (`tables/`), and render HTMX fragments (`templates/netbox_librenms_plugin/htmx/`). Follow that flow for new resources
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Prefer the devcontainer commands (`netbox-run`, `netbox-run-bg`, `netbox-reload`, `netbox-logs`) described in `.devcontainer/README.md`. They manage NetBox + plugin reloading
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Static assets belong 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-11T15:36:09.754Z
Learning: REST endpoints for imports live in `views/imports/actions.py` (with the list view in `views/imports/list.py`) and surface via `urls.py`. Keep server responses and HTMX targets in sync
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: API serializers (`api/serializers.py`) mirror models for external consumption. Update serializers and `api/views.py` together to avoid contract drift
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Navigation and menu items are registered in `navigation.py`; extend there for new sections so NetBox renders links correctly
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Uses two-tier permissions via `LibreNMSSettings` model: `view_librenmssettings` (read) and `change_librenmssettings` (write). See `docs/development/permissions.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: All views inherit `LibreNMSPermissionMixin` from `views/mixins.py`, which sets `permission_required = PERM_VIEW_PLUGIN`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `has_write_permission()` method checks `PERM_CHANGE_PLUGIN`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `require_write_permission()` returns error response (HTMX `HX-Redirect` or standard redirect) if denied
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `require_write_permission_json()` returns `JsonResponse(403)` if denied (for AJAX endpoints)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `NetBoxObjectPermissionMixin` adds a **second layer** of permission checking for NetBox model operations (add/change/delete on Device, Interface, VLAN, etc.)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Some views set `required_object_permissions` dynamically per-request (e.g., `SyncInterfacesView` switches between `Interface` and `VMInterface` based on object type)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `check_object_permissions(method)` returns `(bool, missing_perms_list)`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `require_object_permissions(method)` redirects/shows HTMX on permission failure
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `require_object_permissions_json(method)` returns JSON 403 on failure
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `require_all_permissions(method)` performs combined plugin write + object perms check (redirect/HTMX on failure)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: `require_all_permissions_json(method)` performs combined plugin write + object perms check, JSON variant
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Check `docs/development/` for structure, view inheritance, mixins, and template conventions before introducing new patterns
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Review the existing sync views (e.g., `views/sync/interfaces.py`) as reference implementations for data flow and caching patterns
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Apply to `tests/**` context-specific instructions from `.github/instructions/testing.instructions.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Apply to templates and static files context-specific instructions from `.github/instructions/frontend.instructions.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Apply to `jobs.py`, import views, and import utilities context-specific instructions from `.github/instructions/background-jobs.instructions.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Apply to sync views, base views, tables, and sync JS context-specific instructions from `.github/instructions/sync.instructions.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:09.754Z
Learning: Apply to changelog, pyproject.toml, and `__init__.py` version bumps context-specific instructions from `.github/instructions/release.instructions.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:36.441Z
Learning: Job cancellation flow: (1) Call `/api/core/background-tasks/{uuid}/stop/` to stop RQ job, (2) Call plugin's sync endpoint `/api/plugins/librenms_plugin/jobs/{pk}/sync-status/` to update database, (3) Frontend polling detects status changes and redirects appropriately
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:36.441Z
Learning: Poll `/api/core/background-tasks/{uuid}/` for real-time RQ status and update modal messages based on status: 'Job queued...', 'Processing...', 'Job completed!'
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:36.441Z
Learning: Handle all RQ status values explicitly in polling logic to avoid infinite polling
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:36.441Z
Learning: Use `cancelInProgress` flag to prevent polling interference during cancellation
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:36.441Z
Learning: `api/views.py::sync_job_status()` syncs database Job status with RQ job status, needed because NetBox worker doesn't always update DB when jobs stop before processing starts
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:53.412Z
Learning: Styling assumes Tabler defaults
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:53.412Z
Learning: Templates live in `templates/netbox_librenms_plugin/`; reuse/includes under `inc/`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:36:53.412Z
Learning: Prefer updating the table renderer in Python rather than templates when changing row actions since tables emit HTMX-enabled columns and buttons from `tables/*.py`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:37:01.161Z
Learning: Update `__init__.py`, `pyproject.toml`, and `docs/changelog.md` together in a single commit with message `Bump version to X.Y.Z and update changelog`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:37:01.161Z
Learning: Create `release/X.Y.Z` branch from develop, then submit PR to develop for version bump, then submit PR from develop to master for release, then tag `vX.Y.Z` on master
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:37:01.161Z
Learning: Use PR title `Bump version to X.Y.Z and update changelog` for version bump pull requests
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:37:01.161Z
Learning: Use PR title `Release X.Y.Z` for release pull requests from develop to master
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T15:37:01.161Z
Learning: Create GitHub release with tag `vX.Y.Z`, title `vX.Y.Z`, and include release notes describing the release type (Bug Fix, Feature, or Maintenance), summary, changes by category, upgrade notes, and all PRs included
📚 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:
tests/e2e/conftest.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_coverage_devices.py
📚 Learning: 2026-03-27T01:35:32.609Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: tests/e2e/test_module_install.py:132-140
Timestamp: 2026-03-27T01:35:32.609Z
Learning: In Playwright-based E2E tests, avoid using fixed `time.sleep(...)` as the synchronization mechanism. Replace each sleep (e.g., after `btn.click()`, after navigation to the modules tab, and other workflow waiting points) with observable waits tied to real DOM/network transitions such as `page.wait_for_selector(...)`, `page.wait_for_response(...)`, or `page.wait_for_function(...)`. For `tests/e2e/test_module_install.py`, treat the existing fixed-sleep usage as an intentionally deferred improvement tracked in issue `#45`; do not raise it as a new review finding in that file.
Applied to files:
tests/e2e/conftest.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:
tests/e2e/conftest.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_coverage_devices.py
📚 Learning: 2026-03-07T13:12:59.182Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:445-448
Timestamp: 2026-03-07T13:12:59.182Z
Learning: Across netbox_librenms_plugin/views/sync/*.py, verify redirects and URL rewrites do not append ?server_key=. The active server context is determined from the global setting (settings.selected_server) via BaseLibreNMSSyncView.get(), so request.GET server_key is not used to preserve context. Ensure RemoveServerMappingView and ConvertLegacyLibreNMSIdView (and similar views) rely on the global setting instead of propagating server_key in redirects.
Applied to files:
netbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/modules.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/views/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_coverage_devices.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/views/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_coverage_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/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/modules.py
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
📚 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-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-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_tables_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_coverage_devices.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_tables_modules.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_coverage_devices.py
📚 Learning: 2026-03-07T10:32:06.242Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:107-117
Timestamp: 2026-03-07T10:32:06.242Z
Learning: In netbox_librenms_plugin/utils.py, keep the Priority 1 loop to guard against None and bool values when accessing raw_cf.get(server_key). Do not replace the Priority 1 condition with a full get_librenms_device_id call. The two-pass design is intentional: Priority 1 performs quick sanity checks, while Priority 2 handles string normalization and full validation by calling get_librenms_device_id(member, server_key, auto_save=False).
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-07T22:38:43.110Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:552-584
Timestamp: 2026-03-07T22:38:43.110Z
Learning: In netbox_librenms_plugin/utils.py, do not propose replacing 'obj.custom_field_data.get("librenms_id") or {}' with a None check. The code intentionally uses 'or {}' to handle falsey values; downstream type guards treat them equivalently since LibreNMS IDs start at 1, making 0 equivalent to 'not set'. Do not modify this logic; keep the existing behavior for all falsey values.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T08:55:46.317Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:532-595
Timestamp: 2026-03-08T08:55:46.317Z
Learning: In netbox_librenms_plugin/utils.py, do not modify set_librenms_device_id to call obj.save(). It is mutator-only and should only update in-memory obj.custom_field_data[...] without persisting. Ensure callers perform persistence: after mutation, run full_clean() and then save() (as seen in device_operations.py around lines ~864-866) or explicit obj.save() after set_librenms_device_id (as in librenms_api.py around lines ~261-262). This pattern prevents coupling mutation with persistence and preserves validation in between.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-09T19:15:13.104Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/utils.py:249-267
Timestamp: 2026-03-09T19:15:13.104Z
Learning: In netbox_librenms_plugin/utils.py, ensure match_librenms_hardware_to_device_type returns None when DeviceTypeMapping.MultipleObjectsReturned is raised (fail-closed per inline comment). Callers must guard for result is None separately from the normal result check (e.g., if result is None: handle; elif result.get('matched'): ... ). Note that the success path uses match_type='mapping' (not 'exact'), distinguishing it from standard part_number/model exact lookups. Consider adding a unit test that asserts None is returned on MultipleObjectsReturned and that callers properly handle both None and dict results.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/modules.py
🪛 HTMLHint (1.9.2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🪛 OpenGrep (1.20.0)
netbox_librenms_plugin/tables/modules.py
[WARNING] 661-661: 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] 714-714: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (6)
tests/e2e/conftest.py (1)
1-16: LGTM! Clear and helpful documentation.The docstring accurately explains the e2e test suite configuration, provides the correct invocation command, and clarifies the pytest-django interaction for developers. The technical details are correct for Playwright-based tests that drive NetBox over HTTP.
netbox_librenms_plugin/views/sync/devices.py (1)
49-59: Nice fix for invalidobject_typehandling.Returning HTTP 400 for invalid/missing
object_typewith escaped echo is a solid correctness and safety improvement.netbox_librenms_plugin/tests/test_coverage_sync_views2.py (2)
120-153: Deterministic VC-path test setup looks good.The updated request payload and interface/device-id alignment make these cable-sync branches deterministic and reduce flaky branch selection in unit tests.
Also applies to: 172-173, 196-200, 437-438
495-495: Good coverage additions for updatedget_object()contract.The new assertions for
Noneon invalid/missingobject_typeandHttp404propagation correctly lock in the new behavior.Also applies to: 761-783
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html (1)
201-231: LGTM! Clean HTMX form structure with proper accessibility.The device-type mapping form is well-structured:
- Conditional rendering checks for valid hardware before showing the form
- Proper CSRF token and HTMX attributes (
hx-post,hx-swap="none",hx-include)- Accessibility-compliant with
visually-hiddenlabel for screen readers- Unique element IDs scoped by
device_idprevent collisions in multi-device contexts- Submit button correctly starts disabled until a device type is selected
- Dropdown positioned with appropriate
z-index: 1050for modal overlaynetbox_librenms_plugin/views/sync/modules.py (1)
885-965: TOCTOU fix looks correct — conflict re-derivation is properly locked.The serial-conflict query is now executed inside
transaction.atomic()after the installed row is locked, usesselect_for_update()on the conflict candidates, and aborts cleanly via_SerialConflictAmbiguouswhen more than one match is locked. Capturingold_type_name/old_bay_namefrom the lockedinstalled_modulealso avoids the stale-snapshot pitfall from before. Nice closure of the previously flagged TOCTOU window.
The pattern/replacement strings shown in the modal preview are derived from operator-typed input. The previous implementation interpolated them directly into innerHTML, so a NetBox bay name containing HTML markup would be parsed as HTML in the preview — self-XSS only (a user can only inject into their own modal), but still avoidable. Replace the two innerHTML assignments with a small renderPreview() helper that builds the preview as text nodes + <code> elements, using textContent for the user-derived parts. No behavioural change. Flagged by the /review code-review pass on commits 4fd7111..4a6b0b7.
* AddDeviceToLibreNMSView: switch perm gate from
require_write_permission() to require_all_permissions("POST"). Object
is resolved first so required_object_permissions can be set
per-request to (change, Device) or (change, VirtualMachine) — the
view operates on either model via object_type.
* SingleModuleVerifyView: add NetBoxObjectPermissionMixin with
('view', Device) and a require_object_permissions_json('POST') gate
before any inventory data is surfaced.
* tables/modules.py: gate the No Type 'Add Module Type' CTA on
dcim.add_moduletype (new can_add_module_type table flag); thread the
flag from both module-table construction sites in
views/object_sync/devices.py.
* tables/modules.py: replace bare return "" in
_render_fix_bay_template_badge with mark_safe("") so the surrounding
status_html + fix_html + ... concatenation stays a SafeString.
* tests/test_coverage_devices.py: change five 'child_instance.request
== request' assertions to 'is not request' identity checks — the
delegation copies the request, but MagicMock() == MagicMock() is
True so equality silently passed even if the copy were dropped.
* tests/test_coverage_sync_views{,2}.py: update perm stubs to match
new require_all_permissions() call site; add a regression test for
the (Device-vs-VM) per-request mapping and a 400-without-perm-check
test for invalid object_type.
* tests/test_tables_modules.py: thread can_add_module_type through
the table fixture; add a negative test for the gating.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
netbox_librenms_plugin/tables/modules.py (1)
355-358:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t gate non-module CTAs behind module add/change permissions.
The early return at Lines 357-358 makes the later
Add Carrier Rule,Add Mapping, andAdd Module Typebranches unreachable for users who can manage those workflows but cannot add/changeModuleobjects. In particular, the newcan_add_module_typeflag never takes effect in that permission combination.Suggested fix
if not self.device: return "" if not self.has_write_permission: return "" - if not self.can_add_module and not self.can_change_module: - return "" # "Integrated" rows are duplicate SNMP entries for a single physical # card (parent + integrated child sharing serial+model) — there's # nothing to install, so no actions. if record.get("status") == "Integrated": return ""Also applies to: 531-649
🤖 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/tables/modules.py` around lines 355 - 358, The early return currently uses self.has_write_permission and then blocks when not (self.can_add_module or self.can_change_module), which prevents non-module CTAs from showing; change the gating logic so the initial check only returns when not self.has_write_permission, and replace the subsequent blanket module add/change check with targeted permission checks for each CTA branch (use self.can_add_carrier_rule for "Add Carrier Rule", self.can_add_mapping for "Add Mapping", and self.can_add_module_type for "Add Module Type") so those branches can be reached even when self.can_add_module/self.can_change_module are false; apply the same fix pattern to the corresponding block in the later range (the 531-649 region) to ensure each CTA is guarded by its specific permission flag rather than the module add/change flags.
🤖 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/object_sync/devices.py`:
- Around line 178-180: The POST handler runs the permission gate before
resolving the target Device, so move the permission check to immediately after
you obtain selected_device via get_object_or_404(...) in the post method;
register selected_device in the mixin’s per-request object map (so object-level
checks see it) and replace the early call to
require_object_permissions_json("POST") with
require_all_permissions_json("POST"), returning early if it yields a response;
apply the same change for the other POST checks in this file that currently call
require_object_permissions_json.
---
Duplicate comments:
In `@netbox_librenms_plugin/tables/modules.py`:
- Around line 355-358: The early return currently uses self.has_write_permission
and then blocks when not (self.can_add_module or self.can_change_module), which
prevents non-module CTAs from showing; change the gating logic so the initial
check only returns when not self.has_write_permission, and replace the
subsequent blanket module add/change check with targeted permission checks for
each CTA branch (use self.can_add_carrier_rule for "Add Carrier Rule",
self.can_add_mapping for "Add Mapping", and self.can_add_module_type for "Add
Module Type") so those branches can be reached even when
self.can_add_module/self.can_change_module are false; apply the same fix pattern
to the corresponding block in the later range (the 531-649 region) to ensure
each CTA is guarded by its specific permission flag rather than the module
add/change flags.
🪄 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: 29a3ffc3-8b1a-4c76-b3af-ebaf99012119
📒 Files selected for processing (8)
netbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.htmlnetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/devices.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/devices.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All views must inherit
LibreNMSPermissionMixinfromviews/mixins.pyand setpermission_required = PERM_VIEW_PLUGINObject-level permission checks must declare
required_object_permissionsdict mapping HTTP methods to[(action, Model)]tuples (e.g., {"POST": [("add", VLAN), ("change", VLAN)]})Sync POST handlers must call
require_all_permissions("POST")and return early if it returns a response; AJAX/JSON endpoints must userequire_all_permissions_json("POST")Validate referrer URLs using
_get_safe_redirect_url(request)to prevent open-redirect attacksBackground jobs must use standalone permission helpers from
import_utils/permissions.py(check_user_permissions,require_permissions) instead of view mixins, since jobs run outside view contextWhen building HttpResponse from Django-template-rendered HTML, use
format_html()to compose the envelope andmark_safe()as a trust assertion on the inner HTML to satisfy CodeQL's reflected-XSS checksDo not use
# lgtm[py/reflected-xss]comments; GitHub's modern CodeQL Action does not honour LGTM.com legacy syntax
Files:
netbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_tables_modules.py
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX 2.x is the primary async layer. Table row updates must return
<tr hx-swap-oob="true">instead of usingouterHTMLswaps to keep table layout intact.Do not reintroduce
data-bs-toggleor duplicate modal IDs in modal implementations.Keep
<select class="device-role-select">markup stable to preserve JavaScript hook-up with TomSelect decorators.Do not re-add
table-responsivewrappers, as they were deliberately removed to prevent dropdown clipping.Templates must live in
templates/netbox_librenms_plugin/with reuse/includes underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
netbox_librenms_plugin/templates/**/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments must live in
templates/netbox_librenms_plugin/htmx/directory and include:device_import_row.html,device_validation_details.html,device_vc_details.html, andbulk_import_confirm.html.Keep server responses and HTMX targets in sync when modifying HTMX fragments in
templates/netbox_librenms_plugin/htmx/.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
**/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
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/modules.py
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Hook into NetBox (Django 5) using plugin APIs; respect `navigation.py`, `urls.py`, and `api/` patterns
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: LibreNMS communication must use `librenms_api.py` client instead of new `requests` calls; reuse the client to handle multi-server configs via `LibreNMSSettings` model and `servers` plugin config
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Views follow a three-layer structure: Base views (`views/base/`), Object sync views (`views/object_sync/`), and Sync action views (`views/sync/`); new views should extend the closest base class and compose mixins
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Sync pipelines should fetch LibreNMS data, cache it, build comparison tables, and render HTMX fragments; follow this flow for new resources
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Always call `LibreNMSAPI.get_librenms_id` for device/VM mapping instead of touching the `librenms_id` custom field directly
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Matching is intentionally exact-only for site, platform, device type, and role; do not add fuzzy matching
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
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-11T19:10:59.366Z
Learning: Prefer the devcontainer commands (`netbox-run`, `netbox-run-bg`, `netbox-reload`, `netbox-logs`) for local development as described in `.devcontainer/README.md`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Static assets belong in `static/netbox_librenms_plugin/`; run NetBox's `collectstatic` when bundling
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
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 from `templates/netbox_librenms_plugin/htmx/` and keep server responses and HTMX targets in sync
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: API serializers in `api/serializers.py` must mirror models; update serializers and `api/views.py` together to avoid contract drift
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Navigation and menu items must be registered in `navigation.py`; extend there for new sections so NetBox renders links correctly
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Schema changes must be coordinated through Django migrations in `migrations/` and update `models.py` plus admin/pydantic representations accordingly
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Review documentation in `docs/development/` for structure, view inheritance, mixins, and template conventions before introducing new patterns
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:10:59.366Z
Learning: Coordinate with existing sync views (e.g., `views/sync/interfaces.py`) as reference implementations when implementing new resources
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:11:44.883Z
Learning: Prefer updating the table renderer in Python rather than templates when changing row actions, since tables emit HTMX-enabled columns and buttons from `tables/*.py`.
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:11:51.084Z
Learning: Version bump commit must update `__init__.py`, `pyproject.toml`, and `changelog.md` in a single commit with message `Bump version to X.Y.Z and update changelog`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:11:51.084Z
Learning: Create `release/X.Y.Z` branch from develop, submit PR to develop, then submit PR from develop to master for the release workflow
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:11:51.084Z
Learning: Tag releases as `vX.Y.Z` on master branch and create GitHub release with structured body including release type, summary, changes by category, and upgrade notes
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:12:07.391Z
Learning: librenms_api.py → test_librenms_api.py, test_librenms_api_helpers.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:12:07.391Z
Learning: import_utils/ package (filters.py, device_operations.py, vm_operations.py, cache.py, permissions.py, virtual_chassis.py), import_validation_helpers.py, utils.py → test_import_utils.py, test_import_validation_helpers.py, test_utils.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:12:07.391Z
Learning: jobs.py, views/imports/list.py → test_background_jobs.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:12:07.391Z
Learning: import_utils/bulk_import.py → test_coverage_bulk_import.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:12:07.391Z
Learning: Utility helpers (utils.py coverage tests) → test_coverage_utils.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:12:07.391Z
Learning: Permission mixins, API permissions, constants → test_permissions.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:12:07.391Z
Learning: VLAN API, mode detection, comparison, sync → test_vlan_sync.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:12:07.391Z
Learning: VlanAssignmentMixin, VLAN enrichment → test_interface_vlan_sync.py
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-11T19:12:07.391Z
Learning: Views (views/sync/, views/object_sync/, views/imports/actions.py) — no dedicated test files yet. Test business logic via the utility modules they call, not via HTTP requests.
📚 Learning: 2026-03-07T13:12:59.182Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:445-448
Timestamp: 2026-03-07T13:12:59.182Z
Learning: Across netbox_librenms_plugin/views/sync/*.py, verify redirects and URL rewrites do not append ?server_key=. The active server context is determined from the global setting (settings.selected_server) via BaseLibreNMSSyncView.get(), so request.GET server_key is not used to preserve context. Ensure RemoveServerMappingView and ConvertLegacyLibreNMSIdView (and similar views) rely on the global setting instead of propagating server_key in redirects.
Applied to files:
netbox_librenms_plugin/views/sync/devices.py
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/devices.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/views/sync/devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_tables_modules.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/views/sync/devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_tables_modules.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/views/sync/devices.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/tests/test_tables_modules.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/sync/devices.pynetbox_librenms_plugin/views/object_sync/devices.py
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_tables_modules.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_tables_modules.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_tables_modules.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-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-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/modules.py
🪛 HTMLHint (1.9.2)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🪛 OpenGrep (1.20.0)
netbox_librenms_plugin/tables/modules.py
[WARNING] 670-670: 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] 723-723: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (5)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.html (1)
97-213: LGTM — XSS-safe preview rendering is correctly implemented.The
renderPreviewhelper (lines 161-172) builds DOM nodes viadocument.createElement("code")+textContentanddocument.createTextNode, fully resolving the priorinnerHTMLsink for operator-controlledpat.pattern,pat.replacement,libreName, andcurrent. Other interpolations are appropriately contextualized:|escapejsat line 99 for the JS string literal, Django auto-escaping for attribute/HTML values (lines 15, 67, 219), andtextContentfor all dynamic summaries (lines 181, 185).derivePatternalso defensively returnsnullon round-trip mismatch (line 154), keeping unsafe replacement-string edge cases (e.g.,$, backslash tokens) on the safe exact-only fallback path.netbox_librenms_plugin/views/sync/devices.py (1)
54-75: Permission gating order and object-type scoping are correctly enforced.Resolving the target object before building
required_object_permissions, then gating withrequire_all_permissions("POST"), is the right fix for the Device/VirtualMachine branch.As per coding guidelines: "
**/*.py: Sync POST handlers must callrequire_all_permissions(\"POST\")and return early if it returns a response" and "**/*.py: Object-level permission checks must declarerequired_object_permissionsdict mapping HTTP methods to[(action, Model)]tuples".netbox_librenms_plugin/tests/test_coverage_sync_views.py (1)
563-595: Great coverage for the new invalid-object-type and permission-order behavior.These assertions lock in the intended contract: 400 on invalid
object_type, and permission checks only after successful model resolution.netbox_librenms_plugin/tests/test_coverage_sync_views2.py (2)
120-154: The cable sync tests now exercise a deterministic branch path.Aligning selected device IDs with mocked interface ownership removes ambiguity and makes these assertions more reliable.
Also applies to: 172-200
472-478: Add-device tests are now aligned with the updated permission and object-resolution flow.Good update to gate on
require_all_permissionsand to explicitly coverget_object()behavior for missing/invalid vs valid-not-found cases.Also applies to: 763-786
When the user adds a missing bay via the 'Fix Device Type' / 'Fix Model' modal, NetBox saves a ModuleBayTemplate but does not auto-create the corresponding ModuleBay on existing devices/modules of that type — bay instances are only auto-materialised at first-create time. The resolver then keeps reporting 'No matching bay' even though both the template and the ModuleBayMapping look correct, leaving the user with no obvious follow-up action. AddBayTemplateView.post now calls a new staticmethod _instantiate_template_on_existing(bay_template, target_kind, target) inside the same transaction. For device_type targets it walks every Device of that DeviceType; for module_type targets it walks every installed Module of that ModuleType. For each, it calls template.instantiate(...) and saves the resulting ModuleBay, skipping any device/module that already has a bay with the same name (so re-running the modal after a partial manual fix is safe). The success message now reports how many existing devices/modules received the new bay so it is obvious why the row in the modules tab now resolves.
The inventory-core feature set originally landed as six incremental
migrations on this branch (0010_inventory_models through
0015_moduletypemapping_manufacturer). Since none of them have ever
shipped to a release \u2014 they only existed in the devcontainer \u2014 we collapse
them into a single 0010_inventory_and_mapping_models migration that
matches the current model state.
The new migration also re-seeds the two default InventoryIgnoreRule rows
the modules-sync code expects (Cisco IOS-XR IDPROM duplicates and
embedded RP / fixed-chassis system boards). The wildcard-dedup data
migration from old 0011 is intentionally dropped: fresh installs have
nothing to dedupe.
Devcontainer recovery (for anyone who pulled before the squash):
python manage.py migrate netbox_librenms_plugin 0009 --fake
python manage.py migrate netbox_librenms_plugin --fake
Schema is identical end-to-end; only the migration history changes.
Move require_all_permissions_json() to after selected_device is resolved via get_object_or_404() so the gate runs with the correct request context. Also switch from require_object_permissions_json to require_all_permissions_json per coding guidelines for POST JSON endpoints.
This was referenced May 17, 2026
Merged
Merged
Closed
This was referenced May 25, 2026
Merged
Merged
This was referenced Jun 15, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 by CodeRabbit
New Features
Bug Fixes
Documentation
Tests