feat: inventory core - modules/inventory sync with mapping rules - #63
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: 1
🤖 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/migrations/0011_interfacetypemapping_wildcard_constraint.py`:
- Around line 6-16: The migration's remove_wildcard_duplicates function
currently uses the default DB; change reads and deletes to the migration DB
alias by calling
InterfaceTypeMapping.objects.using(schema_editor.connection.alias).filter(...).order_by("id")
and call row.delete(using=schema_editor.connection.alias) so both the queryset
and the delete operate against schema_editor.connection.alias; reference
remove_wildcard_duplicates and InterfaceTypeMapping in your change.
🪄 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: c5c14b06-679d-4514-9a71-a3df739f27ff
📒 Files selected for processing (1)
netbox_librenms_plugin/migrations/0011_interfacetypemapping_wildcard_constraint.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuselibrenms_api.pyclient for all LibreNMS communication; do not make newrequestscalls. The client handles multi-server configs viaLibreNMSSettingsmodel,serversplugin config, and caching via Django cache + custom fields.
Always callLibreNMSAPI.get_librenms_idto map Devices/VMs to LibreNMS; do not touch thelibrenms_idcustom field directly.
Use exact-only matching for site, platform, device type, and role viautils.pyhelpers (find_matching_site,match_librenms_hardware_to_device_type,find_matching_platform). Do not add fuzzy matching.
Useget_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selection.
Files:
netbox_librenms_plugin/migrations/0011_interfacetypemapping_wildcard_constraint.py
🔇 Additional comments (1)
netbox_librenms_plugin/migrations/0011_interfacetypemapping_wildcard_constraint.py (1)
37-52: Conditional uniqueness split is implemented correctly.The two constraints correctly model SQL NULL semantics: one for concrete speeds and one wildcard constraint for
librenms_speed IS NULL.
- Remove InventoryIgnoreRule.check_match() — dead code never called anywhere; the real matching logic with require_serial_match_parent lives in _check_ignore_rules() in modules_view.py - Close add→change permission race in AddDeviceTypeMappingView: use select_for_update() inside transaction.atomic() and re-check change permission if a concurrent request created the mapping in the window between the upfront filter() and the write - Use schema_editor.connection.alias for ORM reads/deletes in remove_wildcard_duplicates migration so multi-DB deployments clean up duplicates against the correct database
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1463-1495: The code currently re-checks "change" when a mapping
appears between the upfront read and the select_for_update, but does not
re-check "add" when a mapping present at the upfront read is deleted before the
lock; to fix, inside the transaction after obtaining locked (the block using
DeviceTypeMapping.select_for_update()), add a symmetric guard: if not locked and
existing_mapping is truthy then set self.required_object_permissions = {"POST":
[("add", DeviceTypeMapping)]} and call if error :=
self.require_object_permissions("POST"): return error so a user who only had
"change" cannot create a new DeviceTypeMapping; this uses existing symbols
locked, existing_mapping, require_object_permissions, DeviceTypeMapping and the
same "POST" permission flow.
🪄 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: 013cee3b-a4a2-4fd9-b012-68eb316e2824
📒 Files selected for processing (3)
netbox_librenms_plugin/migrations/0011_interfacetypemapping_wildcard_constraint.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/views/imports/actions.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuselibrenms_api.pyclient for all LibreNMS communication; do not make newrequestscalls. The client handles multi-server configs viaLibreNMSSettingsmodel,serversplugin config, and caching via Django cache + custom fields.
Always callLibreNMSAPI.get_librenms_idto map Devices/VMs to LibreNMS; do not touch thelibrenms_idcustom field directly.
Use exact-only matching for site, platform, device type, and role viautils.pyhelpers (find_matching_site,match_librenms_hardware_to_device_type,find_matching_platform). Do not add fuzzy matching.
Useget_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selection.
Files:
netbox_librenms_plugin/migrations/0011_interfacetypemapping_wildcard_constraint.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/models.py
**/*import*/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
import_validation_helpers.pyto centralize validation state mutation during import (role/cluster/rack assignment, issue removal, status recalculation).
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (5)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/migrations/0011_interfacetypemapping_wildcard_constraint.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/models.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/migrations/0011_interfacetypemapping_wildcard_constraint.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/models.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/migrations/0011_interfacetypemapping_wildcard_constraint.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/models.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 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/imports/actions.py
If existing_mapping was found upfront (change permission granted) but the row was deleted before select_for_update(), the else branch would create a new row without ever requiring add permission. Add a symmetric guard: if existing_mapping and not locked, re-check add permission before the create path runs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1474-1501: Concurrent creates on absent rows still race because
select_for_update() doesn't lock missing rows; wrap the
DeviceTypeMapping.objects.create(...) call in a try/except that catches
django.db.IntegrityError, re-query
DeviceTypeMapping.objects.filter(librenms_hardware=hardware.lower()).first() to
decide whether to update the existing row or return the same 409 conflict
response used elsewhere, and ensure you import IntegrityError; keep this
handling inside the existing transaction.atomic() block around the create to
convert the current generic 500 into a graceful 409 (or update the row if
appropriate).
🪄 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: 68a7e47d-a58b-4687-b192-7610b17e6776
📒 Files selected for processing (1)
netbox_librenms_plugin/views/imports/actions.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuselibrenms_api.pyclient for all LibreNMS communication; do not make newrequestscalls. The client handles multi-server configs viaLibreNMSSettingsmodel,serversplugin config, and caching via Django cache + custom fields.
Always callLibreNMSAPI.get_librenms_idto map Devices/VMs to LibreNMS; do not touch thelibrenms_idcustom field directly.
Use exact-only matching for site, platform, device type, and role viautils.pyhelpers (find_matching_site,match_librenms_hardware_to_device_type,find_matching_platform). Do not add fuzzy matching.
Useget_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selection.
Files:
netbox_librenms_plugin/views/imports/actions.py
**/*import*/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
import_validation_helpers.pyto centralize validation state mutation during import (role/cluster/rack assignment, issue removal, status recalculation).
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (5)
📚 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/imports/actions.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 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/imports/actions.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/views/imports/actions.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/imports/actions.py
🔇 Additional comments (3)
netbox_librenms_plugin/views/imports/actions.py (3)
458-458: Good: each validation error is escaped before rendering.This closes the reflected-HTML path in the “No valid devices selected” response.
701-705: Good: bulk-import failures no longer expose exception text.Both the HTMX and redirect paths now keep details in server logs and return a stable user-facing message.
1281-1290: Good ambiguous-platform guard.Returning
400here is safer than silently picking one of multiple exact matches and mutating the wrong platform.
…gView select_for_update() cannot lock absent rows, so two concurrent requests both seeing no existing mapping can both attempt .create(). Catch IntegrityError inside the transaction and return 409 so the client retries; the second attempt will find the now-existing row and take the update path with correct change-permission checking. Also move IntegrityError import to top-level (was a deferred local import inside _save_device).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1527-1538: The rendered device row isn't marked out-of-band, so
HTMX won't apply the OOB swap; after getting row_html from render_device_row (in
the block using get_validated_device_with_selections and variable row_html)
inject an hx-swap-oob attribute into the rendered <tr id="device-row-..."> (e.g.
by replacing the first "<tr " with "<tr hx-swap-oob " or using a small regex
that targets the tr with id="device-row-") before wrapping with
"<table><tbody>...</tbody></table>" so the response contains an OOB-marked
element and HTMX will replace the background row immediately.
🪄 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: 8d700e97-7f74-47d3-8761-864876c0ce17
📒 Files selected for processing (1)
netbox_librenms_plugin/views/imports/actions.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuselibrenms_api.pyclient for all LibreNMS communication; do not make newrequestscalls. The client handles multi-server configs viaLibreNMSSettingsmodel,serversplugin config, and caching via Django cache + custom fields.
Always callLibreNMSAPI.get_librenms_idto map Devices/VMs to LibreNMS; do not touch thelibrenms_idcustom field directly.
Use exact-only matching for site, platform, device type, and role viautils.pyhelpers (find_matching_site,match_librenms_hardware_to_device_type,find_matching_platform). Do not add fuzzy matching.
Useget_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selection.
Files:
netbox_librenms_plugin/views/imports/actions.py
**/*import*/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
import_validation_helpers.pyto centralize validation state mutation during import (role/cluster/rack assignment, issue removal, status recalculation).
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (5)
📚 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/imports/actions.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 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/imports/actions.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/views/imports/actions.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/imports/actions.py
…ponse
The <table><tbody> wrapper was already in place to keep the <tr> in a valid
HTML context for the browser parser, but the hx-swap-oob attribute was never
injected, so HTMX had no instruction to OOB-swap the background row. Without
it the row stays stale until a manual refresh.
String-replace the known <tr id="device-row-{id}"> prefix (count=1) before
wrapping, so both the modal and the row are updated in a single response.
CodeQL flagged reflected XSS on AddDeviceTypeMappingView because device_id was <str:device_id>, making it a user-controlled string embedded in the HTML response. LibreNMS device IDs are always integers; switching to <int:device_id> on all seven device-import/* routes gives Django's URL router built-in type validation and removes the taint path.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
…esponse Both oob_modal and row_html are rendered by Django template views, which auto-escape all user-supplied values. CodeQL cannot model Django's template engine as a sanitizer and flags all request → template-render → HttpResponse paths as reflected XSS. Add lgtm[py/reflected-xss] suppression with a comment explaining why these paths are safe.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1461-1494: The concurrent-create branch currently always escalates
to "change" permission when locked and not existing_mapping; before calling
require_object_permissions("POST") there, short-circuit the identical-target
no-op by checking if locked.netbox_device_type_id == device_type_id (and/or
locked.netbox_device_type equals device_type) and if so skip permission
escalation and treat the operation as a successful no-op (return the same
response/None as the early-return path); otherwise proceed to set
self.required_object_permissions = {"POST": [("change", DeviceTypeMapping)]} and
call require_object_permissions as before. Ensure you update the block
referencing locked, existing_mapping, device_type_id, device_type and
require_object_permissions accordingly.
- Around line 1517-1546: The current code re-fetches the LibreNMS device by
calling DeviceValidationDetailsView.get() and
get_validated_device_with_selections() after saving the mapping, which makes
success responses depend on a second external lookup; instead, reuse the
already-loaded libre_device instance you obtained earlier in this flow (the
in-scope libre_device variable) to re-run the local validation logic (the same
validation/selection computation used by get_validated_device_with_selections,
invoked locally rather than via that helper or
DeviceValidationDetailsView.get()) to produce validation and selections, build
modal_html and row_html from those local objects, then clear or repopulate the
cache (cache.delete or cache.set) only after the HTML is assembled so the client
gets a stable success response even if LibreNMS is temporarily unavailable.
🪄 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: 8817c533-e304-4a63-af05-670dc595f3b2
📒 Files selected for processing (2)
netbox_librenms_plugin/urls.pynetbox_librenms_plugin/views/imports/actions.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuselibrenms_api.pyclient for all LibreNMS communication; do not make newrequestscalls. The client handles multi-server configs viaLibreNMSSettingsmodel,serversplugin config, and caching via Django cache + custom fields.
Always callLibreNMSAPI.get_librenms_idto map Devices/VMs to LibreNMS; do not touch thelibrenms_idcustom field directly.
Use exact-only matching for site, platform, device type, and role viautils.pyhelpers (find_matching_site,match_librenms_hardware_to_device_type,find_matching_platform). Do not add fuzzy matching.
Useget_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selection.
Files:
netbox_librenms_plugin/urls.pynetbox_librenms_plugin/views/imports/actions.py
**/*import*/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
import_validation_helpers.pyto centralize validation state mutation during import (role/cluster/rack assignment, issue removal, status recalculation).
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (5)
📚 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/imports/actions.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/urls.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/urls.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 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/imports/actions.py
…MappingView The previous `# lgtm[py/reflected-xss]` suppression is LGTM.com legacy syntax and is not honored by GitHub's modern CodeQL action, so the alert returned. The remaining taint path is request -> detail_view.get(request, device_id) / render_device_row(request, ...) -> .content.decode() -> f-string into HttpResponse; the inner views auto-escape via Django templates, but CodeQL does not credit template rendering as a sanitizer at this composition site. Compose the OOB envelope with format_html() and pass the rendered inner HTML through mark_safe(). Both are recognized by CodeQL's Django XSS taint model as sanitizers, and the assertion is accurate -- the inner HTML originates from auto-escaping render() calls. Drop the stale comment block and the ineffective lgtm suppression.
When a concurrent request creates a DeviceTypeMapping between our upfront read and the select_for_update() lock, and the locked row already maps to the same device_type_id, the write block is a no-op (lines 1492-1495 skip save). Escalating to change permission in that case rejects callers who hold only add permission even though nothing would be mutated. Short-circuit: only require change permission when locked.netbox_device_type_id != device_type_id (i.e. we will actually overwrite). Also add CodeQL XSS suppression pattern to copilot-instructions.md.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/copilot-instructions.md:
- Around line 78-90: The guidance misstates mark_safe() as a sanitizer; update
the text around the example (references: mark_safe, format_html, modal_html,
some_view.get, HttpResponse) to explicitly state that mark_safe() is only a
trust assertion that disables auto-escaping and must only be used when
modal_html is provably trusted (e.g., server-rendered HTML from a trusted
source), and that untrusted user content must be sanitized (e.g., escape/bleach
or use format_html with escaped parts) before being wrapped — do not present
mark_safe() as a sanitizer and add a short note recommending sanitization
alternatives for untrusted input.
🪄 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: 9e1d9cae-989c-4d1c-969d-2178386a6900
📒 Files selected for processing (2)
.github/copilot-instructions.mdnetbox_librenms_plugin/views/imports/actions.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always call
LibreNMSAPI.get_librenms_idinstead of touching thelibrenms_idcustom field directly for device/VM mapping
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-07T06:42:07.493Z
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-07T06:42:07.493Z
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-07T06:42:07.493Z
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-07T06:42:07.493Z
Learning: Matching should be intentionally exact-only for site, platform, device type, and role via utility functions (`find_matching_site`, `match_librenms_hardware_to_device_type`, `find_matching_platform`). Do not add fuzzy matching
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-07T06:42:07.493Z
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/`)
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-07T06:42:07.493Z
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-07T06:42:07.493Z
Learning: Prefer devcontainer commands (`netbox-run`, `netbox-run-bg`, `netbox-reload`, `netbox-logs`) from `.devcontainer/README.md` instead of manual NetBox management
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin
Timestamp: 2026-05-07T06:42:07.493Z
Learning: Tables should live in `tables/*.py` and templates in `templates/netbox_librenms_plugin/` and drive the UI following HTMX, template, and styling conventions from `frontend.instructions.md`
📚 Learning: 2026-03-09T20:06:43.432Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: .github/pull_request_template.md:1-49
Timestamp: 2026-03-09T20:06:43.432Z
Learning: Do not flag markdownlint style issues (MD022, MD041, blank lines around headings, first-line H1) in repository-level documentation Markdown files such as .github/pull_request_template.md. These files are cosmetic templates and out of scope for PR review linting; consider excluding .github Markdown files from style checks or scope reviews for consistent contributor experience.
Applied to files:
.github/copilot-instructions.md
📚 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/imports/actions.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 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/imports/actions.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/views/imports/actions.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/imports/actions.py
🪛 markdownlint-cli2 (0.22.1)
.github/copilot-instructions.md
[warning] 75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 79-79: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 86-86: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 89-89: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 OpenGrep (1.20.0)
netbox_librenms_plugin/views/imports/actions.py
[WARNING] 1534-1534: 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] 1555-1555: 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 (2)
netbox_librenms_plugin/views/imports/actions.py (2)
1521-1559: ⚡ Quick winSuccess path still depends on two follow-up LibreNMS lookups after the write.
After the mapping is committed, line 1523 deletes the device cache and the response is then rebuilt through
DeviceValidationDetailsView.get()(line 1531) andget_validated_device_with_selections()(line 1539). With the cache freshly deleted, both paths fall throughfetch_device_with_cache()to a live LibreNMS call. If LibreNMS is transiently unavailable or the lookup misses, the mapping is persisted but the client receives an empty/stale OOB swap (modal returns the 404 fragment, the row branch hits theelseat 1557 and emits emptyrow_html) instead of the success state.Reuse the in-scope
libre_deviceyou already loaded at line 1425 to recompute validation locally for both fragments, and clear (or repopulate) the cache after the HTML is assembled — same approach raised previously for this block.
1462-1519: Race-window handling looks solid.The composition is correct: pre-resolve mapping → set least-privilege
required_object_permissions(addvschange) → enforce upfront → re-acquire underselect_for_update()insidetransaction.atomic()with symmetric guards for both the create-after-read race (line 1480, with the no-op short-circuit at 1485) and the delete-after-read race (line 1489), plus anIntegrityError → 409fallback (line 1505) for the absent-row insert race thatselect_for_update()cannot cover. Permission escalations are skipped on identical-target updates so callers with onlyaddare not falsely rejected on a concurrent no-op.
CodeRabbit correctly flagged that the prior wording described mark_safe() as a sanitizer. Rewrote the CodeQL section to be explicit: mark_safe() only asserts that the caller has verified the string is safe; it must only be used on server-rendered Django view output, never on raw user input.
marcinpsk
added a commit
that referenced
this pull request
May 12, 2026
* feat(inventory): modules/inventory sync tab with mapping rules
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)
* fix tests: update db-fallback tests to match new RQ-only cancellation 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
* fix: updated tests
* Apply CR findings: code quality, test, docs, and template fixes
- 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
* Apply second batch of CR findings on pr/inventory-core
- 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
* Apply CR findings batch 3: test hardening, PlatformMapping lazy import, 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()
* cr: batch 4 — prefetch_related, deterministic export, test improvements
- 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
* cr: batch 5 — normalization scoping bug, test fixes
- 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)
* cr: batch 6 — normalization rule caching, test correctness
- 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
* fix: FPC slot int/str comparison, stale docstring, test patch targets
- _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
* fix: surface ambiguous platform, preloaded-rules fallback, serial mismatch, 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
* fix: canonical librenms_id lookup, None guard, transceiver transparent, 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
* Apply CR batch 10 fixes
- 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
* Apply CR batch 11 fixes
- 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
* fix: normalize librenms_hardware/os to lowercase, fix convert_speed_to_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.
* fix(js): address PR #258 review findings
- hideModal: remove all backdrops via querySelectorAll+forEach
- htmx:afterSettle: derive label from aria-labelledby, fallback to id
- initializeVlanModalSave: truncate/extract error body before display
* fix: handle unsaved manufacturer in normalization, fix JS modal/fetch 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
* fix: type annotation, non-positive librenms_id guard, htmx label listener
- 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
* fix: use _PLACEHOLDER_VALUES for serial normalization, fix docstring
- 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
* Fix CR batch: device_type ambiguity, platform MOR, cache invalidation, 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
* Promote SKIP_TYPES and _NON_HARDWARE_CLASSES to module-level constants
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.
* Fix find_matching_platform docstring and non-positive string librenms_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
* fix: normalize placeholder serials and txr_type in modules_view
_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.
* Fix whitespace-only librenms_id, BUILTIN model placeholder, FPC-scope 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
* Fix non-integer librenms_id passthrough, stale cache on missing ID, exact-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
* fix: normalize _GENERIC_CONTAINER_MODELS to lowercase; embed librenms_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)
* fix: treat duplicate-serial module conflicts as ambiguous instead of 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.
* fix: clear stale cache on legacy payload; ungate generic-model filter 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
* Remove dead vc_requested assignment left by rebase
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.
* fix: VC zero-based detection all-zeros guard; align VC-perm tests with 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 #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.
* fix: CR batch 12 — serial normalization, conflict disambiguation, modal guard, test fixes
- sync/modules.py: replace all 'serial == "-"' guards with _PLACEHOLDER_VALUES
normalization (import from modules_view) for consistency across InstallModuleView,
InstallBranchView, PreviewModuleReplaceView, ReplaceModuleView
- sync/modules.py: PreviewModuleReplaceView — use count() instead of .first() to
detect ambiguous serial conflicts; pass serial_conflict_ambiguous=True to template
- sync/modules.py: ReplaceModuleView — use count() to detect ambiguous conflicts;
return error redirect when count > 1 instead of silently picking one
- module_mismatch_modal.html: add serial_conflict_ambiguous branch (warning alert);
gate 'Update Serial Only' and 'Replace Module' buttons when conflict is ambiguous
- tables/mappings.py: render em-dash for serial_matches_device rules in
render_require_serial_match_parent (flag has no effect for that match type)
- test_coverage_bulk_import.py: remove xfail marker — TTL refresh is now implemented
- test_coverage_utils.py: assert PlatformMapping.objects.get called with librenms_os__iexact
- test_modules_view.py: assert row.get('serial_conflict_module') is None (not 'not in row')
- test_module_replace.py: add count.return_value to mock chain for new count() calls
- tests/e2e/test_module_install.py: extend os.environ instead of replacing in subprocess
* fix: address deferred CR findings from issues #53-#56
where can_move_from and serial_conflict_module are set. Button posts to
move_module view with conflict_module_id and target_bay_id.
valid dict from fetch_device_with_cache mock instead of None, so the
test exercises the full sync code path including cache dict population.
- save.assert_called_once() → assert_called_once_with(update_fields=
['status', 'completed']) in test_does_not_overwrite_completed_when_not_in_rq
- Use distinct server_key 'prod-server' in DeviceModuleTableView tests
instead of 'default' to catch accidental default fallback
- Use permission-specific has_perm side_effect instead of return_value=True
per name (dict[str, list]) instead of keeping only the first. Resolve
mapping-based lookups by checking which bay has an installed_module when
duplicates exist — return only if unambiguous (exactly one occupied bay).
Closes #53
Closes #54
Closes #55
Closes #56
* fix: address 3 remaining CR findings from PR #50
test_coverage_actions.py: Fix false-positive test — wrong POST key
'vc_detection_enabled' replaced with the actual key 'enable_vc_detection'
that _resolve_vc_detection_enabled() reads. Previously the mock returned
None for every key and the assertion passed via the default=False fallback.
views/sync/modules.py: Unwrap cached inventory dict payload before use.
The inventory cache stores {"inventory": [...], "librenms_id": ...} but all
4 sync action views (InstallBranchView, InstallSelectedView,
ModuleMismatchPreviewView, ReplaceModuleView) iterated the raw payload as
a list, causing item.get("entPhysicalIndex") to iterate dict keys instead
of inventory rows. Added _extract_inventory_list() helper that unwraps the
dict payload and tolerates legacy list payloads.
views/sync/modules.py: Apply ignore rules to root item in _collect_branch().
Previously only children were filtered by ignore rules; the root/parent
item was always enqueued. Now _collect_branch() checks the root item:
'skip' → return []; 'transparent' → collect children only, not root.
Also updated InstallSelectedView to filter both 'skip' and 'transparent'
(was only 'skip') to match table view parity.
* refactor: drop _extract_inventory_list legacy-list fallback
The "legacy" list payload referenced in e96b3f3 never existed in any
released or upstream code — the bare-list writer was replaced inside
this same branch (7dfd436) before any consumer shipped, so no
deployed cache can hold one. The fallback also contradicted
BaseModuleTableView.get_context_data, which evicts non-dict payloads
as cache misses.
Drop the `or []` non-dict branch so the helper matches the canonical
reader, and update the inventory-cache stubs in test_module_replace.py
and test_sync_modules.py to use the dict format produced by the writer.
* fix: race conditions, class-aware mappings, stale snapshot in module sync
_install_single: Lock target bay with select_for_update() before checking
occupancy. Previously two concurrent requests could both observe the bay
as empty, causing an IntegrityError that rolled back the entire batch.
Now consistent with InstallModuleView's locking pattern.
_find_parent_module_id: Make ancestor mapping resolution class-aware.
Exact mappings are now indexed by (librenms_name, librenms_class) with
class-specific preference and class-empty fallback. Regex mappings also
prefer class-matching rules before falling back to class-empty rules,
consistent with _lookup_regex_bay_mapping() in the base view.
ReplaceModuleView: Move target_bay/old_type_name/old_bay_name reads
after select_for_update() so they reflect the locked row's current state.
Previously these were captured from the pre-lock snapshot, which could
be stale if another request moved or replaced the module first.
MoveModuleView: Lock target bay with select_for_update() inside the
atomic block instead of using the pre-lock get_object_or_404 snapshot.
Tests updated for select_for_update chains and librenms_class on mock
mapping helpers.
* fix: normalize placeholder serials in UpdateModuleSerialView, fix regex fallback in _find_parent_module_id
UpdateModuleSerialView: add placeholder normalization consistent with all
other serial-handling paths (InstallModuleView, _install_single,
ReplaceModuleView, ModuleMismatchPreviewView).
_find_parent_module_id: replace 'class_matches or fallback_matches' with
'class_matches + fallback_matches' so empty-class regex rules are still
tried when class-specific rules exist but none match the name — matching
the base view's _lookup_regex_bay_mapping pattern.
* fix: lock module row before updating serial in UpdateModuleSerialView
Move Module fetch inside transaction.atomic() with select_for_update()
to prevent stale-instance writes when a concurrent replace/move changes
the module between the read and the save. Consistent with locking pattern
used by InstallModuleView, ReplaceModuleView, and MoveModuleView.
Update test to mock Module.objects.select_for_update() chain instead of
get_object_or_404 for the module.
* fix: use fullmatch+expand in _find_parent_module_id regex matching
Mirror base view's _lookup_regex_bay_mapping behavior:
- Use rm._compiled_pattern.fullmatch(name) instead of re.search()
- Use match.expand(rm.netbox_bay_name) for capture group substitution
- Prevents false-positive partial matches and enables regex backrefs
Update test helpers to set _compiled_pattern on mock regex mappings.
* fix: address CR review batch - regex, tests, docs
- Tighten Nokia regex: \w → [0-9] for part number digits, [A-Z0-9]
for transceiver cleanup pattern
- Use real dict for request.GET mock in test_background_jobs.py
- Make devcontainer discovery deterministic: env var override,
error on multiple matches
- Fix brittle '156 objects' banner filter in e2e tests
- Fix typos in virtual_chassis.md (NEtbox → NetBox, dispalys)
- Make README install snippet idempotent with grep guard
* revert: restore original docs/usage_tips/virtual_chassis.md
Revert grammar/typo edits to virtual_chassis.md — these changes
do not belong in this PR. Any docs updates should go through the
upstream repository directly.
* fix: revert bulk_import cancellation, VC comment, VM docstring, sync template improvements
- Restore periodic job cancellation checks (every 5th iteration)
- Improve VC zero-based position detection comment clarity
- Simplify VM create docstring, reorder server_key parameter
- Use resolved_name instead of sysName in sync template
- Move VC serial count badge to button label
- Add name check tooltip
* fix: restore naming resolution and VC logic lost during rebase
Rebase regression stripped resolve_naming_preferences(),
_determine_device_name(), and _generate_vc_member_name() from both
UpdateDeviceNameView and the sync base view's get_librenms_device_info().
Restored:
- resolved_name computation using user naming preferences (sysName vs
hostname, strip domain) in librenms_sync_view.py
- VC member name generation for virtual chassis devices
- VC sync device lookup for members without their own librenms_id
- resolved_name template context variable (used by sync_base template)
- Proper early bailout when LibreNMS has no usable name
Updated test mocks to set virtual_chassis=None and patch the restored
naming functions.
* revert: restore original README.md and virtual_chassis.md
These files should not be modified in this PR — docs and README
changes belong in upstream repository directly.
* fix: improve ambiguity test assertion and error message wording
- Rename test to test_match_none_returns_ambiguous_error and assert
'Ambiguous' appears in the error message to detect regressions
- Broaden error message to cover both DeviceTypeMapping and DeviceType
ambiguity (match_librenms_hardware_to_device_type returns None for
either case)
* fix: CR review - modal close, test line refs, generic e2e
- librenms_sync.js: replace closeBtn.click() with hideModal() in VLAN
modal save handler (aligns with upstream fix 7bf533f)
- test_coverage_bulk_import/devices/list: strip hardcoded line-number
references from docstrings to avoid drift
- tests/e2e/test_module_install: make device-agnostic — auto-detect
device, discover modules from table, generic assertions
* docs: update instruction files for accuracy after v0.4.4-v0.4.6 changes
* fix: three bugs in module inventory sync reported in PR #261 review
- select_for_update(of=("self",)) avoids FOR UPDATE on the nullable side
of the installed_module outer join, fixing the Install Selected crash
- add has_write_permission to full-page context so Install Selected form
renders on page load, not only after an htmx Refresh Modules swap
- guard htmx.process() with typeof check to prevent 'htmx is not defined'
error when the Replace modal fetches its preview fragment
* fix: improve module table readability in dark mode
Remove table-success row highlight from installed modules — the Installed
badge is sufficient to identify state, and the green row background makes
linked text unreadable in dark theme.
Add text-white/text-dark contrast classes to all badge colors so they
remain legible in both light and dark themes.
* feat: split Mappings into its own navigation group
Move all mapping items (Interface, Device Type, Module Type, Module Bay,
Normalization Rules, Inventory Ignore Rules, Platform) out of the
Settings group into a dedicated Mappings group for clearer navigation.
* refactor: reorder navigation groups to Import, Status Check, Mappings, Settings
* fix: remove table-light from mismatch modal thead for dark mode
table-light forces a white background regardless of theme. Removing it
lets Bootstrap 5.3 theme-aware styling apply to the header row.
* fix: replace table-danger/warning cell backgrounds with text colors in mismatch modal
table-danger/table-warning produce a pinkish/yellow background that
clashes with Bootstrap's link color in dark mode. Switch to text-danger
and text-warning with fw-semibold — pure text color works on any
background and reads correctly in both light and dark themes.
* fix: remove all row background highlighting from module sync table
table-warning and table-danger row classes cause unreadable contrast in
dark mode. The status badge is sufficient to identify each row state.
Remove the row_class field entirely along with the dead row_attrs entry.
* refactor: reorder Mappings group — all mappings first, then Ignore Rules, Normalization Rules
* fix: update tests to reflect row_class removal
Replace assertions on table-success/danger/warning row_class values
with assert "row_class" not in row now that row highlighting is gone.
* fix: auto-generate slug when creating platform from sync page (#279)
Platform() without a slug fails full_clean() in NetBox 4.x. Derive the
slug from the platform name via slugify so the modal submit succeeds.
* Revert "fix: auto-generate slug when creating platform from sync page (#279)"
This reverts commit 3029cd3c2fc7ef054104e721a663ccde2109bdbb.
* fix: generate slug when creating Platform via CreateAndAssignPlatformView
Platform requires a slug field; omitting it caused full_clean() to raise
a ValidationError before the platform could be saved. Fixes #279.
* test: assert Platform constructor receives slug in CreateAndAssignPlatformView
Regression test for #279 — verifies slugify(name) is passed to the
Platform constructor so the missing-slug error cannot recur undetected.
* fix: generate slug when creating Platform via CreateAndAssignPlatformView
Platform requires a slug field; omitting it caused full_clean() to raise
a ValidationError before the platform could be saved. Fixes #279.
* test: assert Platform constructor receives slug in CreateAndAssignPlatformView
Regression test for #279 — verifies slugify(name) is passed to the
Platform constructor so the missing-slug error cannot recur undetected.
* fix: wrap OOB row in table to survive HTMX HTML parsing
When AddDeviceTypeMappingView returns a combined response with both
the modal OOB (a <div>) and the row OOB (a <tr>), HTMX wraps the
response in a <template> for parsing. A <tr> following a <div> in
that context is invalid HTML and gets silently dropped by the browser
parser, so the row never updates.
Wrap row_html in <table><tbody>...</tbody></table> before appending
to ensure the <tr> element survives parsing. The <div id='django-messages'>
inside is foster-parented outside the table by the parser, so both
OOBs are found and applied correctly.
* fix(imports): add HTTP status codes, remove redundant full_clean, fix JS Content-Type case-sensitivity
- AddDeviceTypeMappingView: return 400 for missing/invalid device_type_id,
404 for DeviceType.DoesNotExist, 500 for unexpected exceptions
- Remove redundant mapping.full_clean() before save() — DeviceTypeMapping
inherits FullCleanOnSaveMixin which calls full_clean() inside save()
- librenms_sync.js: lowercase Content-Type header before .includes() check
to handle case variants (e.g. 'Application/JSON')
* fix(js): extract JSON error message in delete-interfaces handler; guard updateHtmxModalLabel self-assignment
- DeleteNetBoxInterfacesView fetch: parse JSON error body (error/message/detail)
instead of throwing raw JSON string, consistent with other fetch handlers
- updateHtmxModalLabel: skip textContent assignment when header === label to
prevent stripping icon markup from the modal title element
* refactor(js): extract fetchErrorMessage() helper to unify fetch error handling
inline text/JSON handling. Helper extracts error/message/detail from
JSON responses, falls back to raw text, and truncates at 300 chars.
Eliminates drift between handlers and simplifies future additions.
* feat: exact-name-first platform lookup; optional mapping on platform create
- find_matching_platform(): try exact case-insensitive name match first,
fall back to PlatformMapping only when no direct name match exists;
update docstring accordingly
- _get_platform_info(): delegate to find_matching_platform() instead of
inline Platform.objects.get(); use 'or "-"' for null LibreNMS fields
- CreateAndAssignPlatformView: read 'librenms_os' + 'create_mapping' POST
params; create PlatformMapping(librenms_os, platform) when checkbox
checked and no existing mapping for that OS
- librenms_sync_base.html: add librenms_os hidden input, 'Add platform
mapping' checkbox, and JS warning when platform name differs from OS
- device_validation_details.html: inline device-type search form when
no matching device type
- urls.py + views/__init__.py: register and export AddDeviceTypeMappingView
- tests: align mocks and assertions to new lookup order
* fix(pr#50): address CR feedback on AddDeviceTypeMapping form and platform-mapping creation
- device_validation_details.html: add visually-hidden <label> for the
device-type search input (HTMLHint a11y)
- device_validation_details.html: replace hardcoded "/api/dcim/device-types/"
with {% url 'dcim-api:devicetype-list' %} so NetBox BASE_PATH deployments
work correctly
- CreateAndAssignPlatformView: surface PlatformMapping creation failures
to the user via messages.warning, and inform when an existing mapping
was found (no longer fails silently)
* fix(pr#50): isolate PlatformMapping save; ignore stale autocomplete results
- CreateAndAssignPlatformView: wrap PlatformMapping save in nested
transaction.atomic() so an IntegrityError on the unique librenms_os
constraint only rolls back the mapping savepoint, not the outer
device-platform-assignment transaction (which was previously left in
needs_rollback state, so messages.success would fire after a discarded
device save)
- device_validation_details.html: add monotonically increasing requestSeq
to the device-type autocomplete so out-of-order fetch responses can no
longer overwrite the dropdown with results for an older query
* fix(pr#50): check add_platformmapping permission when create_mapping is requested
CreateAndAssignPlatformView now reads 'create_mapping' from POST before the
permission check and dynamically adds ('add', PlatformMapping) to
required_object_permissions so require_all_permissions() enforces it.
Without this, a user with only add_platform + change_device could silently
create PlatformMapping objects through the checkbox.
* fix: address CodeQL security scan findings
- XSS (#18): escape() user-controlled error strings in bulk confirm HTML response
- XSS (#19,#20): escape() object_type param in device_fields.py HTTP responses
- Stack trace (#8): replace str(exc) with generic message in bulk import HTMX error
- Stack trace (#9): replace str(exc) with generic message in interfaces transaction error
- Stack trace (#13): replace catch-all str(e) with generic message in ip_addresses_view
- JS XSS (#4,#5): fix innerHTML spinner — store/restore full innerHTML, use DOM
for label to avoid reinterpreting textContent as HTML
- Workflow permissions (#1-#3): add permissions: contents: read to all three workflows;
publish-pypi job-level permissions also gains contents: read alongside id-token: write
- Devcontainer logging (#17): stop printing raw codespace name / CSRF / allowed-host
values to stdout in devcontainer config
- URL redirect (#6,#7): add comment — _get_safe_redirect_url already validates via
url_has_allowed_host_and_scheme; CodeQL false positive
- Lint: fix E741 ambiguous variable name in e2e test
* revert: remove workflow permissions additions (tracked separately)
* Fix PR review findings: available_roles, CSRF, test quality
- bulk_import.py: preserve available_roles in elif-not-actual_is_vm branch
(matches pattern of other clearing paths at lines 363, 384)
- librenms_sync.js: remove getCookie('csrftoken') fallback from two fetch
call sites; align with hidden-input-only CSRF convention used elsewhere
- test_vm_operations.py: strengthen log assertion to assert_any_call with
expected checkpoint messages (VM 1 and VM 5 of 10)
- test_vm_operations.py: delete duplicate test_job_cancellation_with_errored_status
(identical logic already covered by test_job_cancellation_breaks_loop)
- Skip mock-target change: apply_cluster/role_to_validation are lazy (in-function)
imports, so patching import_validation_helpers.X is correct — patching
vm_operations.X would fail as those names don't exist at module level
* Fix PR #58 review findings (batch 2)
- librenms_sync_view.py: Fix inverted comment on find_matching_platform order
- actions.py: Replace str(exc) with generic message in non-HTMX bulk import error handler
- actions.py: AddDeviceTypeMappingView — honour server_key, add NetBoxObjectPermissionMixin
with DeviceTypeMapping add/change permissions
- device_validation_details.html: Add hidden server_key input to dt-mapping form
- interfaces.py: Add logger.exception in DeleteNetBoxInterfacesView catch-all
- test_sync_devices.py: Strengthen VM type assertion to assert_called_once_with
including VirtualMachine class and pk kwarg
* Fix PR #58 review findings (batch 3)
- device_validation_details.html: Disable 'Add Mapping' button until device type
is selected from dropdown; enable on selection, re-disable on input clear
- devices.py: Use copy.copy(request) for child table view instances, consistent
with vms.py pattern
- test_coverage_devices.py: Update request assertions to use == (copy equality)
instead of 'is' identity to match new copy.copy behaviour
* Fix #59–#62: YAML anchor, DynamicModelChoiceField, ToggleColumn, write perms
Closes #59: Anchor Juniper MX regex pattern (^...$) so generic transceiver
pattern sorts first and MX-specific fallback works correctly.
Closes #60: Replace plain ModelChoiceField with DynamicModelChoiceField in
DeviceTypeMappingForm and ModuleTypeMappingForm for API-backed typeahead.
Closes #61: Add 'pk' to fields/default_columns in all 7 mapping tables so
NetBoxTable's built-in ToggleColumn renders the bulk-select checkbox.
Closes #62: Add LibreNMSWritePermissionMixin (permission_required =
PERM_CHANGE_PLUGIN) to mixins.py and apply it to all 35 mutation views
(Create, Edit, Delete, BulkImport, BulkDelete) in mapping_views.py.
* Fix PR #63 review findings: ordering, error handling, modal title, click listener cleanup
- utils.py: add .order_by('pk') to get_enabled_ignore_rules() for deterministic
first-match-wins behavior in _check_ignore_rules
- utils.py: add ambiguity_source key to find_matching_platform ambiguous returns
('platform' or 'mapping') to distinguish which source caused the conflict
- modules_view.py: _apply_installed_status else branch returns 'No Type' instead
of 'Installed' when matched_type is None
- actions.py: AddDeviceTypeMappingView exception handler uses logger.exception
and returns generic message instead of leaking str(exc) to the client
- device_fields.py: separate IntegrityError from ValidationError in
CreateAndAssignPlatformView — IntegrityError on PlatformMapping insert treated
as mapping_existed (concurrent insert) rather than a creation failure
- librenms_sync.js: updateHtmxModalLabel scopes .modal-title query to
#htmx-modal-body to avoid matching the outer #htmx-modal-label element
- device_validation_details.html: replace anonymous accumulating document click
listener with a named handleDocumentClick function that removes itself when
the dropdown element is detached from the DOM (HTMX fragment replacement)
- test_coverage_forms.py: use _make_form helper in test methods instead of
duplicating the patch setup inline
- test_platform_mapping.py: update ambiguous assertion to include ambiguity_source
- test_sync_modules.py: fix InventoryIgnoreRule mock to support .order_by() chain
- contrib/platform_mappings.yaml: create example file referenced in README
* Fix replacement template validation and deterministic bay lookup
Closes #64, Closes #65
Issue #64: ModuleBayMapping.clean() and NormalizationRule.clean() validated
replacement templates against test strings that didn't guarantee a regex match
(empty string, or pattern text itself), so invalid back-references like \2
on a single-group pattern silently passed. Add _validate_replacement_template()
helper that builds a synthetic guaranteed-match pattern with identical group
structure (named groups preserved), exercising all back-references.
Issue #65: _build_table_rows() used ChainMap(device_bays, *module_scoped_bays.values())
for transceiver items, whose iteration order was non-deterministic (dict insertion
order = queryset order). Replace with a dedicated _compute_all_bays() static
method that sorts module IDs by PK for stable first-match-wins collision
resolution, logs collisions at DEBUG level, and always lets device-level bays
win. Remove the now-unused 'from collections import ChainMap' import.
* Fix wildcard constraint, ambiguity message, and scoped permissions
Add DB-level UniqueConstraint for wildcard InterfaceTypeMapping rows
(librenms_speed IS NULL) to prevent concurrent duplicate inserts that
bypass the in-process clean() check. The existing constraint for
non-NULL rows gains an explicit condition so both are partial indexes.
Migration 0011 handles the rename + addition atomically.
Update sync_platform ambiguous-match error message to inspect
ambiguity_source ('platform' vs 'mapping') and direct users to either
the Platforms screen or the Platform Mappings screen as appropriate.
Restructure AddDeviceTypeMappingView.post() so only the plugin write
permission is checked upfront (cheap). The API call and hardware string
extraction happen first, then an existence check on DeviceTypeMapping
determines whether 'add' or 'change' object permission is required
before the actual get_or_create.
* devcontainer: restore full debug output in codespaces-configuration.py
Development-only file; logging config values (CSRF origins, allowed hosts)
is an accepted tradeoff. CodeQL alert dismissed intentionally.
* fix(migration): add preflight dedup before wildcard UniqueConstraint
Before enforcing unique_interface_type_mapping_wildcard, remove any
duplicate librenms_speed IS NULL rows (keeping lowest PK per
librenms_type). Guards against deployments that applied 0010 and hit
the race window before 0011 was available.
* fix: remove dead check_match, close TOCTOU race, fix migration db alias
- Remove InventoryIgnoreRule.check_match() — dead code never called
anywhere; the real matching logic with require_serial_match_parent
lives in _check_ignore_rules() in modules_view.py
- Close add→change permission race in AddDeviceTypeMappingView: use
select_for_update() inside transaction.atomic() and re-check change
permission if a concurrent request created the mapping in the window
between the upfront filter() and the write
- Use schema_editor.connection.alias for ORM reads/deletes in
remove_wildcard_duplicates migration so multi-DB deployments clean
up duplicates against the correct database
* fix: guard symmetric delete race in AddDeviceTypeMappingView
If existing_mapping was found upfront (change permission granted) but
the row was deleted before select_for_update(), the else branch would
create a new row without ever requiring add permission. Add a symmetric
guard: if existing_mapping and not locked, re-check add permission
before the create path runs.
* fix: catch IntegrityError on concurrent create in AddDeviceTypeMappingView
select_for_update() cannot lock absent rows, so two concurrent requests
both seeing no existing mapping can both attempt .create(). Catch
IntegrityError inside the transaction and return 409 so the client
retries; the second attempt will find the now-existing row and take the
update path with correct change-permission checking.
Also move IntegrityError import to top-level (was a deferred local
import inside _save_device).
* fix: inject hx-swap-oob on device row in AddDeviceTypeMappingView response
The <table><tbody> wrapper was already in place to keep the <tr> in a valid
HTML context for the browser parser, but the hx-swap-oob attribute was never
injected, so HTMX had no instruction to OOB-swap the background row. Without
it the row stays stale until a manual refresh.
String-replace the known <tr id="device-row-{id}"> prefix (count=1) before
wrapping, so both the modal and the row are updated in a single response.
* fix: use int:device_id converter on all device-import URL patterns
CodeQL flagged reflected XSS on AddDeviceTypeMappingView because device_id
was <str:device_id>, making it a user-controlled string embedded in the
HTML response. LibreNMS device IDs are always integers; switching to
<int:device_id> on all seven device-import/* routes gives Django's URL
router built-in type validation and removes the taint path.
* fix: suppress false-positive CodeQL XSS on AddDeviceTypeMappingView response
Both oob_modal and row_html are rendered by Django template views, which
auto-escape all user-supplied values. CodeQL cannot model Django's template
engine as a sanitizer and flags all request → template-render → HttpResponse
paths as reflected XSS. Add lgtm[py/reflected-xss] suppression with a
comment explaining why these paths are safe.
* fix: use format_html + mark_safe to clear CodeQL XSS on AddDeviceTypeMappingView
The previous `# lgtm[py/reflected-xss]` suppression is LGTM.com legacy syntax
and is not honored by GitHub's modern CodeQL action, so the alert returned.
The remaining taint path is request -> detail_view.get(request, device_id) /
render_device_row(request, ...) -> .content.decode() -> f-string into
HttpResponse; the inner views auto-escape via Django templates, but CodeQL
does not credit template rendering as a sanitizer at this composition site.
Compose the OOB envelope with format_html() and pass the rendered inner HTML
through mark_safe(). Both are recognized by CodeQL's Django XSS taint model
as sanitizers, and the assertion is accurate -- the inner HTML originates
from auto-escaping render() calls. Drop the stale comment block and the
ineffective lgtm suppression.
* fix: skip change-permission escalation in concurrent-create no-op path
When a concurrent request creates a DeviceTypeMapping between our upfront
read and the select_for_update() lock, and the locked row already maps to
the same device_type_id, the write block is a no-op (lines 1492-1495 skip
save). Escalating to change permission in that case rejects callers who
hold only add permission even though nothing would be mutated.
Short-circuit: only require change permission when locked.netbox_device_type_id
!= device_type_id (i.e. we will actually overwrite).
Also add CodeQL XSS suppression pattern to copilot-instructions.md.
* docs: clarify mark_safe is a trust assertion, not a sanitizer
CodeRabbit correctly flagged that the prior wording described mark_safe()
as a sanitizer. Rewrote the CodeQL section to be explicit: mark_safe() only
asserts that the caller has verified the string is safe; it must only be
used on server-rendered Django view output, never on raw user input.
* feat: add VC-aware module sync
* test: align VC module sync expectations
* fix: use all ancestor names as bay-mapping candidates
Previously _match_module_bay only used the nearest named ancestor as a
candidate for ModuleBayMapping lookups. For deeply nested ports (e.g.
GigabitEthernet3/11 inside a CVR-X2-SFP adapter on a WS-X4908-10GE)
the immediate parent is Port Container 3/11, which the existing contrib
regex ^Port Container (\d+)/(\d+)$ resolves to X2 Port 11 — a bay that
does not exist. The grandparent Port Container 3/2 would resolve
correctly to X2 Port 2, but was never tried.
Replace _find_parent_container_name (nearest-only) with
_find_all_ancestor_names which returns every named ancestor nearest
first. All ancestors beyond the immediate parent are appended to
candidate_names after item_name/item_descr so that the existing contrib
regex already handles this case without any new mapping entries.
The contrib description for the Port Container regex is updated to note
the CVR-X2-SFP use-case.
* Revert "fix: use all ancestor names as bay-mapping candidates"
This reverts commit 216fb84358b347326e8983cb87f046c0fda8b7c4.
* test: add prod-shape WS-X4908 bay-matching coverage
The existing _linecard_inventory fixture uses synthetic container names
that already match NetBox bays directly ("Slot 3", "X2 Port 2", "SFP slot")
and never exercises the contrib regex paths or the positional fallback
on real LibreNMS naming. As a result, no test covered the actual prod
data shape, and a flawed "walk all ancestors as bay candidates" fix
(216fb84, since reverted) passed all tests despite landing transceivers
in the wrong bay.
Capture the real shape from a Cisco WS-X4908-10GE linecard:
chassis "Switch System"
container "Slot 3" [no model]
module "Linecard(slot 3)" [WS-X4908-10GE]
container "Port Container 3/2"
other "Converter 3/2" [CVR-X2-SFP]
container "Port Container 3/11"
port "GigabitEthernet3/11" [GLC-TE]
container "Port Container 3/12"
port "GigabitEthernet3/12" [GLC-T]
Tests assert each level resolves correctly:
- linecard via `^Linecard\(slot (\d+)\)$` regex -> device-bay "Slot 3"
- converter via parent `^Port Container (\d+)/(\d+)$` regex -> "X2 Port 2"
- GE inside CVR via positional fallback -> "SFP 1" / "SFP 2"
- GE shows "No Bay" when CVR is matched but uninstalled in NetBox
A separate regression test uses a no-Converter-entry hierarchy
(Linecard -> PC 3/2 -> PC 3/11 -> GE3/11) where bay scope bubbles to
the linecard's bays. In this scope, ancestor-walking would resolve
the grandparent `Port Container 3/2` to `X2 Port 2` and incorrectly
land the transceiver in the parent module's slot. This test fails
if 216fb84-style logic is re-introduced.
Add a `_load_contrib_bay_mappings` helper that loads the contrib YAML
as fake mapping objects and a `bay_mappings=` kwarg on `_run_build_context`
so tests can opt into the real mapping set instead of the empty default.
* fix: bail _match_bay_by_position on non-container scaffolding
Cisco IOS-XR exposes deep entity-MIB scaffolding under each linecard:
modules with class="module" and model="N/A" (Motherboard, Slice 0,
EZChip, "Slice 0 SFP Port Module #N"). The positional walk previously
treated every model-less ancestor as a walk-through container, kept
reassigning container_idx until it found the linecard's real model,
and then took the position of the deepest ancestor among the linecard's
direct children.
On a real ASR-9904 (NetBox device prod-lab03d-ra1.lab) every TenGigE
port descended through the same Motherboard chain, so they all reported
container_idx=Motherboard and resolved to position=1 -> Slot 1 on the
chassis. The chassis Slot 1 holds the RSP line card; clicking install
on a TenGigE row would write an SFP module into the RSP bay, then any
subsequent install rejected with "Module bay 'Slot 1' already has a
module installed".
Restrict the walk to ENTITY-MIB containers (entPhysicalClass="container").
A modelless ancestor of any other class is hierarchical scaffolding, not a
bay position; walking past it silently collapses sibling counts. Bail
when we hit one — the row drops to "No Bay" instead of confidently
mismatching.
Tests:
- TestPositionalMatchScaffoldingChain (new): captures the ASR-9904
shape and asserts (1) TenGigE rows do NOT match Slot 1, (2) status
is "No Bay", (3) sibling rows resolve independently rather than
collapsing to a single bay.
- TestMatchBayByPosition (updated): existing tests omitted
entPhysicalClass on synthetic containers; add it explicitly so the
fixtures match real LibreNMS data shape and the positional walk's
class check passes.
Verified live: all 11 TenGigE0/0/0/N rows on device 54 now show
"No Bay" instead of collapsing to "Slot 1". RSP0/RSP1 and power
supplies still match correctly via their own positional paths.
* fix: restrict serial_matches_device rule to chassis-level entries
The 'Embedded RP / fixed-chassis system board' ignore rule
(match_type=serial_matches_device, action=transparent) targets fixed-form
routers like Cisco 8201-SYS / 8100, where the system board is reported as
an ENTITY-MIB entry whose serial equals the device record's serial.
Marking it transparent hides the row and promotes its children
(transceivers, fans, PSUs) to device-level bay matching.
The match criterion was just "item.serial == device.serial" with no
location check. On chassis devices like the ASR-9904, line cards can
share the device serial when the operator set Device.serial to the
linecard's serial (LibreNMS doesn't necessarily report the chassis serial
as the device serial). The rule fired on the linecard, hid it, and
promoted every TenGigE child to chassis-level matching.
Combined with the previous positional-walk bug, this collapsed every
TenGigE0/0/0/N row to the same chassis Slot 1 bay; in isolation it would
silently land transceivers in chassis line-card bays.
Tighten the criterion: only fire when the item is at chassis level —
either it has no parent (top-level entity) or its direct parent has
entPhysicalClass="chassis". System boards on fixed-form routers satisfy
this; line cards inside slot containers don't.
Tests cover three new shapes:
- parent is chassis -> rule fires (fixed-form router)
- parent is container -> rule does NOT fire (ASR-9904 linecard)
- parent is module -> rule does NOT fire (nested submodule)
Verified on live device 54: the 0/0 linecard now appears as its own
row instead of being hidden, and its TenGigE descendants resolve to
"No Bay" instead of collapsing to a chassis slot.
* fix: class-aware positional fallback + model gap warnings
The positional fallback in _match_bay_by_position previously tried
[SFP N, Slot N, Bay N, Port N] for every item regardless of hardware
class. On chassis devices whose NetBox model defines only line-card
slots (Slot 0..3) but no Fan Tray / PSU bays, fans and power supplies
landed in line-card "Slot N" bays. Example on ASR-9904 device 54:
- 0/FT0 (fan) -> Slot 3
- 0/PT0-PM0 (powerSupply) -> Slot 2
- 0/PT0-PM1 (powerSupply) -> Slot 3
Pick patterns appropriate for the item class:
- fan -> Fan Tray N / Fan N / FT N
- powerSupply -> Power Supply N / PSU N / PEM N / PM N
- module / port / ioModule / cpmModule / mdaModule / fabricModule
/ xioModule -> Slot N / SFP N / Bay N / Port N
- other classes (sensor, etc.) -> no positional guess
Items whose class has no matching bay name in scope drop to No Bay
rather than confidently mismatching.
Surface NetBox-model gaps: each No Bay / No Type row now carries a
model_warning field describing the most likely missing piece:
- empty bay scope -> parent module type has no bay templates
- class-specific -> add bay templates with the expected names
- missing type -> No NetBox ModuleType matches '<model>'
The modules-table render_status surfaces this as a tooltip on the
status badge plus an alert icon, mirroring the existing
name_conflict_warning rendering.
Tests:
- TestPositionalMatchClassAware: 6 cases covering fan/PSU/module
behavior plus unknown-class fallback to None.
- TestNoBayWarningHints / TestNoTypeWarningHints: helper output
distinguishes the three causes.
- TestBuildRowModelWarning: integration check that _build_row
populates model_warning on the right rows.
- test_tables_modules.py: render_status surfaces model_warning as
a tooltip with the alert icon.
Verified on device 54: 0/FT0 and 0/PT0-PMx now show No Bay with
class-appropriate hints instead of landing in chassis line-card slots.
* feat: ModuleBayMapping suggestions + Add Mapping button on No Bay rows
When a row resolves to "No Bay" because the LibreNMS name doesn't match
any bay in scope, propose a regex ModuleBayMapping covering the whole
slot family rather than one entry per slot. Heuristic: when the item's
name ends with a number and a bay in scope ends with the same number,
suggest "^<prefix>(\d+)$ -> <bay-prefix>\1" with the item's class as
filter. Example: item "0/0" + bay "Slot 0" yields
"^0/(\d+)$ -> Slot \1, class=module".
Suppressed in three cases:
- scope_preserved=True (scope inherited from an unmatched ancestor)
- Class/bay mismatch (fans only match Fan/FT bays, etc.)
- scope_uninstalled (warning already redirects to install parent first)
UI:
- render_status surfaces the suggestion in the badge tooltip.
- render_actions adds an "Add Mapping" button on No Bay rows with
model_suggestion. Opens ModuleBayMapping create form pre-filled via
NetBox ObjectEditView GET-param initial. return_url is captured from
configure(request) for round-trip.
Plumbing:
- _build_row gains scope_uninstalled and scope_preserved kwargs.
- The iteration loop tracks both states alongside bays_by_depth.
Defaults fall back to top-level state so first sub-item iteration
inherits correct semantics.
- _build_no_bay_warning gains a scope_uninstalled branch ("install the
parent module first") and appends suggestion when provided.
Verified live on device 54 (ASR-9904):
- 0/0 (No Bay, top-level) -> SUGGEST ^0/(\d+)$ -> Slot \1
- TenGigE0/0/0/N (preserved scope) -> no suggestion
- 0/FT0 (fan, no fan bays) -> no suggestion (class filter)
- 0/PT0-PMx (powerSupply) -> no suggestion (class filter)
* fix: address valid code-review findings
- testing.instructions.md: add test_coverage_bulk_import.py and
test_coverage_utils.py to coverage-by-module table
- models.py InterfaceTypeMapping.clean(): normalize/strip librenms_type
and reject blank values before wildcard uniqueness check
- librenms_sync.js: always clear device_selection cell on row update,
even when backend returns empty, so stale VC dropdowns are removed
- tables/modules.py VCModuleTable: hide device_selection column by
default (visible=False); guard format_module_data against missing VC
- utils.py load_bay_mappings(): use explicit order_by for deterministic
regex precedence
- modules_view.py: log raw LibreNMS errors server-side and show generic
message to users (inventory fetch failure + transceiver fetch warning)
- test_coverage_filters.py: make cache-key assertions order-independent
- test_vm_operations.py: add missing existing_device key to mock dict
* feat: remove {module} conflict warning, add Generic manufacturer fallback for module type matching
* fix: replace {module} conflict tooltip with Name Conflict status badge; add Generic manufacturer fallback
- Remove the warning tooltip about {module} causing non-unique interface
names; instead show a 'Name Conflict' status badge (bg-warning text-dark)
when has_nested_name_conflict() detects sibling name collisions
- has_nested_name_conflict() and sibling_counts tracking are preserved
- Add get_generic_module_types_indexed() and generic_fallback support in
resolve_module_type() so 'Generic' manufacturer matches are tried when
no vendor-specific ModuleType is found
- Pre-fetch generic module types once per request in _get_generic_module_types()
- render_status() no longer reads name_conflict_warning (never set); reads
model_warning only for the alert-icon tooltip
- Restore has_nest…
marcinpsk
added a commit
that referenced
this pull request
May 19, 2026
* feat(inventory): modules/inventory sync tab with mapping rules
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)
* fix tests: update db-fallback tests to match new RQ-only cancellation 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
* fix: updated tests
* Apply CR findings: code quality, test, docs, and template fixes
- 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
* Apply second batch of CR findings on pr/inventory-core
- 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
* Apply CR findings batch 3: test hardening, PlatformMapping lazy import, 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()
* cr: batch 4 — prefetch_related, deterministic export, test improvements
- 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
* cr: batch 5 — normalization scoping bug, test fixes
- 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)
* cr: batch 6 — normalization rule caching, test correctness
- 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
* fix: FPC slot int/str comparison, stale docstring, test patch targets
- _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
* fix: surface ambiguous platform, preloaded-rules fallback, serial mismatch, 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
* fix: canonical librenms_id lookup, None guard, transceiver transparent, 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
* Apply CR batch 10 fixes
- 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
* Apply CR batch 11 fixes
- 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
* fix: normalize librenms_hardware/os to lowercase, fix convert_speed_to_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.
* fix(js): address PR #258 review findings
- hideModal: remove all backdrops via querySelectorAll+forEach
- htmx:afterSettle: derive label from aria-labelledby, fallback to id
- initializeVlanModalSave: truncate/extract error body before display
* fix: handle unsaved manufacturer in normalization, fix JS modal/fetch 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
* fix: type annotation, non-positive librenms_id guard, htmx label listener
- 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
* fix: use _PLACEHOLDER_VALUES for serial normalization, fix docstring
- 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
* Fix CR batch: device_type ambiguity, platform MOR, cache invalidation, 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
* Promote SKIP_TYPES and _NON_HARDWARE_CLASSES to module-level constants
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.
* Fix find_matching_platform docstring and non-positive string librenms_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
* fix: normalize placeholder serials and txr_type in modules_view
_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.
* Fix whitespace-only librenms_id, BUILTIN model placeholder, FPC-scope 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
* Fix non-integer librenms_id passthrough, stale cache on missing ID, exact-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
* fix: normalize _GENERIC_CONTAINER_MODELS to lowercase; embed librenms_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)
* fix: treat duplicate-serial module conflicts as ambiguous instead of 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.
* fix: clear stale cache on legacy payload; ungate generic-model filter 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
* Remove dead vc_requested assignment left by rebase
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.
* fix: VC zero-based detection all-zeros guard; align VC-perm tests with 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 #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.
* fix: CR batch 12 — serial normalization, conflict disambiguation, modal guard, test fixes
- sync/modules.py: replace all 'serial == "-"' guards with _PLACEHOLDER_VALUES
normalization (import from modules_view) for consistency across InstallModuleView,
InstallBranchView, PreviewModuleReplaceView, ReplaceModuleView
- sync/modules.py: PreviewModuleReplaceView — use count() instead of .first() to
detect ambiguous serial conflicts; pass serial_conflict_ambiguous=True to template
- sync/modules.py: ReplaceModuleView — use count() to detect ambiguous conflicts;
return error redirect when count > 1 instead of silently picking one
- module_mismatch_modal.html: add serial_conflict_ambiguous branch (warning alert);
gate 'Update Serial Only' and 'Replace Module' buttons when conflict is ambiguous
- tables/mappings.py: render em-dash for serial_matches_device rules in
render_require_serial_match_parent (flag has no effect for that match type)
- test_coverage_bulk_import.py: remove xfail marker — TTL refresh is now implemented
- test_coverage_utils.py: assert PlatformMapping.objects.get called with librenms_os__iexact
- test_modules_view.py: assert row.get('serial_conflict_module') is None (not 'not in row')
- test_module_replace.py: add count.return_value to mock chain for new count() calls
- tests/e2e/test_module_install.py: extend os.environ instead of replacing in subprocess
* fix: address deferred CR findings from issues #53-#56
where can_move_from and serial_conflict_module are set. Button posts to
move_module view with conflict_module_id and target_bay_id.
valid dict from fetch_device_with_cache mock instead of None, so the
test exercises the full sync code path including cache dict population.
- save.assert_called_once() → assert_called_once_with(update_fields=
['status', 'completed']) in test_does_not_overwrite_completed_when_not_in_rq
- Use distinct server_key 'prod-server' in DeviceModuleTableView tests
instead of 'default' to catch accidental default fallback
- Use permission-specific has_perm side_effect instead of return_value=True
per name (dict[str, list]) instead of keeping only the first. Resolve
mapping-based lookups by checking which bay has an installed_module when
duplicates exist — return only if unambiguous (exactly one occupied bay).
Closes #53
Closes #54
Closes #55
Closes #56
* fix: address 3 remaining CR findings from PR #50
test_coverage_actions.py: Fix false-positive test — wrong POST key
'vc_detection_enabled' replaced with the actual key 'enable_vc_detection'
that _resolve_vc_detection_enabled() reads. Previously the mock returned
None for every key and the assertion passed via the default=False fallback.
views/sync/modules.py: Unwrap cached inventory dict payload before use.
The inventory cache stores {"inventory": [...], "librenms_id": ...} but all
4 sync action views (InstallBranchView, InstallSelectedView,
ModuleMismatchPreviewView, ReplaceModuleView) iterated the raw payload as
a list, causing item.get("entPhysicalIndex") to iterate dict keys instead
of inventory rows. Added _extract_inventory_list() helper that unwraps the
dict payload and tolerates legacy list payloads.
views/sync/modules.py: Apply ignore rules to root item in _collect_branch().
Previously only children were filtered by ignore rules; the root/parent
item was always enqueued. Now _collect_branch() checks the root item:
'skip' → return []; 'transparent' → collect children only, not root.
Also updated InstallSelectedView to filter both 'skip' and 'transparent'
(was only 'skip') to match table view parity.
* refactor: drop _extract_inventory_list legacy-list fallback
The "legacy" list payload referenced in e96b3f3 never existed in any
released or upstream code — the bare-list writer was replaced inside
this same branch (7dfd436) before any consumer shipped, so no
deployed cache can hold one. The fallback also contradicted
BaseModuleTableView.get_context_data, which evicts non-dict payloads
as cache misses.
Drop the `or []` non-dict branch so the helper matches the canonical
reader, and update the inventory-cache stubs in test_module_replace.py
and test_sync_modules.py to use the dict format produced by the writer.
* fix: race conditions, class-aware mappings, stale snapshot in module sync
_install_single: Lock target bay with select_for_update() before checking
occupancy. Previously two concurrent requests could both observe the bay
as empty, causing an IntegrityError that rolled back the entire batch.
Now consistent with InstallModuleView's locking pattern.
_find_parent_module_id: Make ancestor mapping resolution class-aware.
Exact mappings are now indexed by (librenms_name, librenms_class) with
class-specific preference and class-empty fallback. Regex mappings also
prefer class-matching rules before falling back to class-empty rules,
consistent with _lookup_regex_bay_mapping() in the base view.
ReplaceModuleView: Move target_bay/old_type_name/old_bay_name reads
after select_for_update() so they reflect the locked row's current state.
Previously these were captured from the pre-lock snapshot, which could
be stale if another request moved or replaced the module first.
MoveModuleView: Lock target bay with select_for_update() inside the
atomic block instead of using the pre-lock get_object_or_404 snapshot.
Tests updated for select_for_update chains and librenms_class on mock
mapping helpers.
* fix: normalize placeholder serials in UpdateModuleSerialView, fix regex fallback in _find_parent_module_id
UpdateModuleSerialView: add placeholder normalization consistent with all
other serial-handling paths (InstallModuleView, _install_single,
ReplaceModuleView, ModuleMismatchPreviewView).
_find_parent_module_id: replace 'class_matches or fallback_matches' with
'class_matches + fallback_matches' so empty-class regex rules are still
tried when class-specific rules exist but none match the name — matching
the base view's _lookup_regex_bay_mapping pattern.
* fix: lock module row before updating serial in UpdateModuleSerialView
Move Module fetch inside transaction.atomic() with select_for_update()
to prevent stale-instance writes when a concurrent replace/move changes
the module between the read and the save. Consistent with locking pattern
used by InstallModuleView, ReplaceModuleView, and MoveModuleView.
Update test to mock Module.objects.select_for_update() chain instead of
get_object_or_404 for the module.
* fix: use fullmatch+expand in _find_parent_module_id regex matching
Mirror base view's _lookup_regex_bay_mapping behavior:
- Use rm._compiled_pattern.fullmatch(name) instead of re.search()
- Use match.expand(rm.netbox_bay_name) for capture group substitution
- Prevents false-positive partial matches and enables regex backrefs
Update test helpers to set _compiled_pattern on mock regex mappings.
* fix: address CR review batch - regex, tests, docs
- Tighten Nokia regex: \w → [0-9] for part number digits, [A-Z0-9]
for transceiver cleanup pattern
- Use real dict for request.GET mock in test_background_jobs.py
- Make devcontainer discovery deterministic: env var override,
error on multiple matches
- Fix brittle '156 objects' banner filter in e2e tests
- Fix typos in virtual_chassis.md (NEtbox → NetBox, dispalys)
- Make README install snippet idempotent with grep guard
* revert: restore original docs/usage_tips/virtual_chassis.md
Revert grammar/typo edits to virtual_chassis.md — these changes
do not belong in this PR. Any docs updates should go through the
upstream repository directly.
* fix: revert bulk_import cancellation, VC comment, VM docstring, sync template improvements
- Restore periodic job cancellation checks (every 5th iteration)
- Improve VC zero-based position detection comment clarity
- Simplify VM create docstring, reorder server_key parameter
- Use resolved_name instead of sysName in sync template
- Move VC serial count badge to button label
- Add name check tooltip
* fix: restore naming resolution and VC logic lost during rebase
Rebase regression stripped resolve_naming_preferences(),
_determine_device_name(), and _generate_vc_member_name() from both
UpdateDeviceNameView and the sync base view's get_librenms_device_info().
Restored:
- resolved_name computation using user naming preferences (sysName vs
hostname, strip domain) in librenms_sync_view.py
- VC member name generation for virtual chassis devices
- VC sync device lookup for members without their own librenms_id
- resolved_name template context variable (used by sync_base template)
- Proper early bailout when LibreNMS has no usable name
Updated test mocks to set virtual_chassis=None and patch the restored
naming functions.
* revert: restore original README.md and virtual_chassis.md
These files should not be modified in this PR — docs and README
changes belong in upstream repository directly.
* fix: improve ambiguity test assertion and error message wording
- Rename test to test_match_none_returns_ambiguous_error and assert
'Ambiguous' appears in the error message to detect regressions
- Broaden error message to cover both DeviceTypeMapping and DeviceType
ambiguity (match_librenms_hardware_to_device_type returns None for
either case)
* fix: CR review - modal close, test line refs, generic e2e
- librenms_sync.js: replace closeBtn.click() with hideModal() in VLAN
modal save handler (aligns with upstream fix 7bf533f)
- test_coverage_bulk_import/devices/list: strip hardcoded line-number
references from docstrings to avoid drift
- tests/e2e/test_module_install: make device-agnostic — auto-detect
device, discover modules from table, generic assertions
* fix: three bugs in module inventory sync reported in PR #261 review
- select_for_update(of=("self",)) avoids FOR UPDATE on the nullable side
of the installed_module outer join, fixing the Install Selected crash
- add has_write_permission to full-page context so Install Selected form
renders on page load, not only after an htmx Refresh Modules swap
- guard htmx.process() with typeof check to prevent 'htmx is not defined'
error when the Replace modal fetches its preview fragment
* fix: improve module table readability in dark mode
Remove table-success row highlight from installed modules — the Installed
badge is sufficient to identify state, and the green row background makes
linked text unreadable in dark theme.
Add text-white/text-dark contrast classes to all badge colors so they
remain legible in both light and dark themes.
* feat: split Mappings into its own navigation group
Move all mapping items (Interface, Device Type, Module Type, Module Bay,
Normalization Rules, Inventory Ignore Rules, Platform) out of the
Settings group into a dedicated Mappings group for clearer navigation.
* refactor: reorder navigation groups to Import, Status Check, Mappings, Settings
* fix: remove table-light from mismatch modal thead for dark mode
table-light forces a white background regardless of theme. Removing it
lets Bootstrap 5.3 theme-aware styling apply to the header row.
* fix: replace table-danger/warning cell backgrounds with text colors in mismatch modal
table-danger/table-warning produce a pinkish/yellow background that
clashes with Bootstrap's link color in dark mode. Switch to text-danger
and text-warning with fw-semibold — pure text color works on any
background and reads correctly in both light and dark themes.
* fix: remove all row background highlighting from module sync table
table-warning and table-danger row classes cause unreadable contrast in
dark mode. The status badge is sufficient to identify each row state.
Remove the row_class field entirely along with the dead row_attrs entry.
* refactor: reorder Mappings group — all mappings first, then Ignore Rules, Normalization Rules
* fix: update tests to reflect row_class removal
Replace assertions on table-success/danger/warning row_class values
with assert "row_class" not in row now that row highlighting is gone.
* fix: auto-generate slug when creating platform from sync page (#279)
Platform() without a slug fails full_clean() in NetBox 4.x. Derive the
slug from the platform name via slugify so the modal submit succeeds.
* Revert "fix: auto-generate slug when creating platform from sync page (#279)"
This reverts commit 3029cd3c2fc7ef054104e721a663ccde2109bdbb.
* fix: generate slug when creating Platform via CreateAndAssignPlatformView
Platform requires a slug field; omitting it caused full_clean() to raise
a ValidationError before the platform could be saved. Fixes #279.
* test: assert Platform constructor receives slug in CreateAndAssignPlatformView
Regression test for #279 — verifies slugify(name) is passed to the
Platform constructor so the missing-slug error cannot recur undetected.
* fix: generate slug when creating Platform via CreateAndAssignPlatformView
Platform requires a slug field; omitting it caused full_clean() to raise
a ValidationError before the platform could be saved. Fixes #279.
* test: assert Platform constructor receives slug in CreateAndAssignPlatformView
Regression test for #279 — verifies slugify(name) is passed to the
Platform constructor so the missing-slug error cannot recur undetected.
* fix: wrap OOB row in table to survive HTMX HTML parsing
When AddDeviceTypeMappingView returns a combined response with both
the modal OOB (a <div>) and the row OOB (a <tr>), HTMX wraps the
response in a <template> for parsing. A <tr> following a <div> in
that context is invalid HTML and gets silently dropped by the browser
parser, so the row never updates.
Wrap row_html in <table><tbody>...</tbody></table> before appending
to ensure the <tr> element survives parsing. The <div id='django-messages'>
inside is foster-parented outside the table by the parser, so both
OOBs are found and applied correctly.
* fix(imports): add HTTP status codes, remove redundant full_clean, fix JS Content-Type case-sensitivity
- AddDeviceTypeMappingView: return 400 for missing/invalid device_type_id,
404 for DeviceType.DoesNotExist, 500 for unexpected exceptions
- Remove redundant mapping.full_clean() before save() — DeviceTypeMapping
inherits FullCleanOnSaveMixin which calls full_clean() inside save()
- librenms_sync.js: lowercase Content-Type header before .includes() check
to handle case variants (e.g. 'Application/JSON')
* fix(js): extract JSON error message in delete-interfaces handler; guard updateHtmxModalLabel self-assignment
- DeleteNetBoxInterfacesView fetch: parse JSON error body (error/message/detail)
instead of throwing raw JSON string, consistent with other fetch handlers
- updateHtmxModalLabel: skip textContent assignment when header === label to
prevent stripping icon markup from the modal title element
* refactor(js): extract fetchErrorMessage() helper to unify fetch error handling
inline text/JSON handling. Helper extracts error/message/detail from
JSON responses, falls back to raw text, and truncates at 300 chars.
Eliminates drift between handlers and simplifies future additions.
* feat: exact-name-first platform lookup; optional mapping on platform create
- find_matching_platform(): try exact case-insensitive name match first,
fall back to PlatformMapping only when no direct name match exists;
update docstring accordingly
- _get_platform_info(): delegate to find_matching_platform() instead of
inline Platform.objects.get(); use 'or "-"' for null LibreNMS fields
- CreateAndAssignPlatformView: read 'librenms_os' + 'create_mapping' POST
params; create PlatformMapping(librenms_os, platform) when checkbox
checked and no existing mapping for that OS
- librenms_sync_base.html: add librenms_os hidden input, 'Add platform
mapping' checkbox, and JS warning when platform name differs from OS
- device_validation_details.html: inline device-type search form when
no matching device type
- urls.py + views/__init__.py: register and export AddDeviceTypeMappingView
- tests: align mocks and assertions to new lookup order
* fix(pr#50): address CR feedback on AddDeviceTypeMapping form and platform-mapping creation
- device_validation_details.html: add visually-hidden <label> for the
device-type search input (HTMLHint a11y)
- device_validation_details.html: replace hardcoded "/api/dcim/device-types/"
with {% url 'dcim-api:devicetype-list' %} so NetBox BASE_PATH deployments
work correctly
- CreateAndAssignPlatformView: surface PlatformMapping creation failures
to the user via messages.warning, and inform when an existing mapping
was found (no longer fails silently)
* fix(pr#50): isolate PlatformMapping save; ignore stale autocomplete results
- CreateAndAssignPlatformView: wrap PlatformMapping save in nested
transaction.atomic() so an IntegrityError on the unique librenms_os
constraint only rolls back the mapping savepoint, not the outer
device-platform-assignment transaction (which was previously left in
needs_rollback state, so messages.success would fire after a discarded
device save)
- device_validation_details.html: add monotonically increasing requestSeq
to the device-type autocomplete so out-of-order fetch responses can no
longer overwrite the dropdown with results for an older query
* fix(pr#50): check add_platformmapping permission when create_mapping is requested
CreateAndAssignPlatformView now reads 'create_mapping' from POST before the
permission check and dynamically adds ('add', PlatformMapping) to
required_object_permissions so require_all_permissions() enforces it.
Without this, a user with only add_platform + change_device could silently
create PlatformMapping objects through the checkbox.
* fix: address CodeQL security scan findings
- XSS (#18): escape() user-controlled error strings in bulk confirm HTML response
- XSS (#19,#20): escape() object_type param in device_fields.py HTTP responses
- Stack trace (#8): replace str(exc) with generic message in bulk import HTMX error
- Stack trace (#9): replace str(exc) with generic message in interfaces transaction error
- Stack trace (#13): replace catch-all str(e) with generic message in ip_addresses_view
- JS XSS (#4,#5): fix innerHTML spinner — store/restore full innerHTML, use DOM
for label to avoid reinterpreting textContent as HTML
- Workflow permissions (#1-#3): add permissions: contents: read to all three workflows;
publish-pypi job-level permissions also gains contents: read alongside id-token: write
- Devcontainer logging (#17): stop printing raw codespace name / CSRF / allowed-host
values to stdout in devcontainer config
- URL redirect (#6,#7): add comment — _get_safe_redirect_url already validates via
url_has_allowed_host_and_scheme; CodeQL false positive
- Lint: fix E741 ambiguous variable name in e2e test
* revert: remove workflow permissions additions (tracked separately)
* Fix PR review findings: available_roles, CSRF, test quality
- bulk_import.py: preserve available_roles in elif-not-actual_is_vm branch
(matches pattern of other clearing paths at lines 363, 384)
- librenms_sync.js: remove getCookie('csrftoken') fallback from two fetch
call sites; align with hidden-input-only CSRF convention used elsewhere
- test_vm_operations.py: strengthen log assertion to assert_any_call with
expected checkpoint messages (VM 1 and VM 5 of 10)
- test_vm_operations.py: delete duplicate test_job_cancellation_with_errored_status
(identical logic already covered by test_job_cancellation_breaks_loop)
- Skip mock-target change: apply_cluster/role_to_validation are lazy (in-function)
imports, so patching import_validation_helpers.X is correct — patching
vm_operations.X would fail as those names don't exist at module level
* Fix PR #58 review findings (batch 2)
- librenms_sync_view.py: Fix inverted comment on find_matching_platform order
- actions.py: Replace str(exc) with generic message in non-HTMX bulk import error handler
- actions.py: AddDeviceTypeMappingView — honour server_key, add NetBoxObjectPermissionMixin
with DeviceTypeMapping add/change permissions
- device_validation_details.html: Add hidden server_key input to dt-mapping form
- interfaces.py: Add logger.exception in DeleteNetBoxInterfacesView catch-all
- test_sync_devices.py: Strengthen VM type assertion to assert_called_once_with
including VirtualMachine class and pk kwarg
* Fix PR #58 review findings (batch 3)
- device_validation_details.html: Disable 'Add Mapping' button until device type
is selected from dropdown; enable on selection, re-disable on input clear
- devices.py: Use copy.copy(request) for child table view instances, consistent
with vms.py pattern
- test_coverage_devices.py: Update request assertions to use == (copy equality)
instead of 'is' identity to match new copy.copy behaviour
* Fix #59–#62: YAML anchor, DynamicModelChoiceField, ToggleColumn, write perms
Closes #59: Anchor Juniper MX regex pattern (^...$) so generic transceiver
pattern sorts first and MX-specific fallback works correctly.
Closes #60: Replace plain ModelChoiceField with DynamicModelChoiceField in
DeviceTypeMappingForm and ModuleTypeMappingForm for API-backed typeahead.
Closes #61: Add 'pk' to fields/default_columns in all 7 mapping tables so
NetBoxTable's built-in ToggleColumn renders the bulk-select checkbox.
Closes #62: Add LibreNMSWritePermissionMixin (permission_required =
PERM_CHANGE_PLUGIN) to mixins.py and apply it to all 35 mutation views
(Create, Edit, Delete, BulkImport, BulkDelete) in mapping_views.py.
* Fix PR #63 review findings: ordering, error handling, modal title, click listener cleanup
- utils.py: add .order_by('pk') to get_enabled_ignore_rules() for deterministic
first-match-wins behavior in _check_ignore_rules
- utils.py: add ambiguity_source key to find_matching_platform ambiguous returns
('platform' or 'mapping') to distinguish which source caused the conflict
- modules_view.py: _apply_installed_status else branch returns 'No Type' instead
of 'Installed' when matched_type is None
- actions.py: AddDeviceTypeMappingView exception handler uses logger.exception
and returns generic message instead of leaking str(exc) to the client
- device_fields.py: separate IntegrityError from ValidationError in
CreateAndAssignPlatformView — IntegrityError on PlatformMapping insert treated
as mapping_existed (concurrent insert) rather than a creation failure
- librenms_sync.js: updateHtmxModalLabel scopes .modal-title query to
#htmx-modal-body to avoid matching the outer #htmx-modal-label element
- device_validation_details.html: replace anonymous accumulating document click
listener with a named handleDocumentClick function that removes itself when
the dropdown element is detached from the DOM (HTMX fragment replacement)
- test_coverage_forms.py: use _make_form helper in test methods instead of
duplicating the patch setup inline
- test_platform_mapping.py: update ambiguous assertion to include ambiguity_source
- test_sync_modules.py: fix InventoryIgnoreRule mock to support .order_by() chain
- contrib/platform_mappings.yaml: create example file referenced in README
* Fix replacement template validation and deterministic bay lookup
Closes #64, Closes #65
Issue #64: ModuleBayMapping.clean() and NormalizationRule.clean() validated
replacement templates against test strings that didn't guarantee a regex match
(empty string, or pattern text itself), so invalid back-references like \2
on a single-group pattern silently passed. Add _validate_replacement_template()
helper that builds a synthetic guaranteed-match pattern with identical group
structure (named groups preserved), exercising all back-references.
Issue #65: _build_table_rows() used ChainMap(device_bays, *module_scoped_bays.values())
for transceiver items, whose iteration order was non-deterministic (dict insertion
order = queryset order). Replace with a dedicated _compute_all_bays() static
method that sorts module IDs by PK for stable first-match-wins collision
resolution, logs collisions at DEBUG level, and always lets device-level bays
win. Remove the now-unused 'from collections import ChainMap' import.
* Fix wildcard constraint, ambiguity message, and scoped permissions
Add DB-level UniqueConstraint for wildcard InterfaceTypeMapping rows
(librenms_speed IS NULL) to prevent concurrent duplicate inserts that
bypass the in-process clean() check. The existing constraint for
non-NULL rows gains an explicit condition so both are partial indexes.
Migration 0011 handles the rename + addition atomically.
Update sync_platform ambiguous-match error message to inspect
ambiguity_source ('platform' vs 'mapping') and direct users to either
the Platforms screen or the Platform Mappings screen as appropriate.
Restructure AddDeviceTypeMappingView.post() so only the plugin write
permission is checked upfront (cheap). The API call and hardware string
extraction happen first, then an existence check on DeviceTypeMapping
determines whether 'add' or 'change' object permission is required
before the actual get_or_create.
* devcontainer: restore full debug output in codespaces-configuration.py
Development-only file; logging config values (CSRF origins, allowed hosts)
is an accepted tradeoff. CodeQL alert dismissed intentionally.
* fix(migration): add preflight dedup before wildcard UniqueConstraint
Before enforcing unique_interface_type_mapping_wildcard, remove any
duplicate librenms_speed IS NULL rows (keeping lowest PK per
librenms_type). Guards against deployments that applied 0010 and hit
the race window before 0011 was available.
* fix: remove dead check_match, close TOCTOU race, fix migration db alias
- Remove InventoryIgnoreRule.check_match() — dead code never called
anywhere; the real matching logic with require_serial_match_parent
lives in _check_ignore_rules() in modules_view.py
- Close add→change permission race in AddDeviceTypeMappingView: use
select_for_update() inside transaction.atomic() and re-check change
permission if a concurrent request created the mapping in the window
between the upfront filter() and the write
- Use schema_editor.connection.alias for ORM reads/deletes in
remove_wildcard_duplicates migration so multi-DB deployments clean
up duplicates against the correct database
* fix: guard symmetric delete race in AddDeviceTypeMappingView
If existing_mapping was found upfront (change permission granted) but
the row was deleted before select_for_update(), the else branch would
create a new row without ever requiring add permission. Add a symmetric
guard: if existing_mapping and not locked, re-check add permission
before the create path runs.
* fix: catch IntegrityError on concurrent create in AddDeviceTypeMappingView
select_for_update() cannot lock absent rows, so two concurrent requests
both seeing no existing mapping can both attempt .create(). Catch
IntegrityError inside the transaction and return 409 so the client
retries; the second attempt will find the now-existing row and take the
update path with correct change-permission checking.
Also move IntegrityError import to top-level (was a deferred local
import inside _save_device).
* fix: inject hx-swap-oob on device row in AddDeviceTypeMappingView response
The <table><tbody> wrapper was already in place to keep the <tr> in a valid
HTML context for the browser parser, but the hx-swap-oob attribute was never
injected, so HTMX had no instruction to OOB-swap the background row. Without
it the row stays stale until a manual refresh.
String-replace the known <tr id="device-row-{id}"> prefix (count=1) before
wrapping, so both the modal and the row are updated in a single response.
* fix: use int:device_id converter on all device-import URL patterns
CodeQL flagged reflected XSS on AddDeviceTypeMappingView because device_id
was <str:device_id>, making it a user-controlled string embedded in the
HTML response. LibreNMS device IDs are always integers; switching to
<int:device_id> on all seven device-import/* routes gives Django's URL
router built-in type validation and removes the taint path.
* fix: suppress false-positive CodeQL XSS on AddDeviceTypeMappingView response
Both oob_modal and row_html are rendered by Django template views, which
auto-escape all user-supplied values. CodeQL cannot model Django's template
engine as a sanitizer and flags all request → template-render → HttpResponse
paths as reflected XSS. Add lgtm[py/reflected-xss] suppression with a
comment explaining why these paths are safe.
* fix: use format_html + mark_safe to clear CodeQL XSS on AddDeviceTypeMappingView
The previous `# lgtm[py/reflected-xss]` suppression is LGTM.com legacy syntax
and is not honored by GitHub's modern CodeQL action, so the alert returned.
The remaining taint path is request -> detail_view.get(request, device_id) /
render_device_row(request, ...) -> .content.decode() -> f-string into
HttpResponse; the inner views auto-escape via Django templates, but CodeQL
does not credit template rendering as a sanitizer at this composition site.
Compose the OOB envelope with format_html() and pass the rendered inner HTML
through mark_safe(). Both are recognized by CodeQL's Django XSS taint model
as sanitizers, and the assertion is accurate -- the inner HTML originates
from auto-escaping render() calls. Drop the stale comment block and the
ineffective lgtm suppression.
* fix: skip change-permission escalation in concurrent-create no-op path
When a concurrent request creates a DeviceTypeMapping between our upfront
read and the select_for_update() lock, and the locked row already maps to
the same device_type_id, the write block is a no-op (lines 1492-1495 skip
save). Escalating to change permission in that case rejects callers who
hold only add permission even though nothing would be mutated.
Short-circuit: only require change permission when locked.netbox_device_type_id
!= device_type_id (i.e. we will actually overwrite).
Also add CodeQL XSS suppression pattern to copilot-instructions.md.
* docs: clarify mark_safe is a trust assertion, not a sanitizer
CodeRabbit correctly flagged that the prior wording described mark_safe()
as a sanitizer. Rewrote the CodeQL section to be explicit: mark_safe() only
asserts that the caller has verified the string is safe; it must only be
used on server-rendered Django view output, never on raw user input.
* feat: add VC-aware module sync
* test: align VC module sync expectations
* fix: use all ancestor names as bay-mapping candidates
Previously _match_module_bay only used the nearest named ancestor as a
candidate for ModuleBayMapping lookups. For deeply nested ports (e.g.
GigabitEthernet3/11 inside a CVR-X2-SFP adapter on a WS-X4908-10GE)
the immediate parent is Port Container 3/11, which the existing contrib
regex ^Port Container (\d+)/(\d+)$ resolves to X2 Port 11 — a bay that
does not exist. The grandparent Port Container 3/2 would resolve
correctly to X2 Port 2, but was never tried.
Replace _find_parent_container_name (nearest-only) with
_find_all_ancestor_names which returns every named ancestor nearest
first. All ancestors beyond the immediate parent are appended to
candidate_names after item_name/item_descr so that the existing contrib
regex already handles this case without any new mapping entries.
The contrib description for the Port Container regex is updated to note
the CVR-X2-SFP use-case.
* Revert "fix: use all ancestor names as bay-mapping candidates"
This reverts commit 216fb84358b347326e8983cb87f046c0fda8b7c4.
* test: add prod-shape WS-X4908 bay-matching coverage
The existing _linecard_inventory fixture uses synthetic container names
that already match NetBox bays directly ("Slot 3", "X2 Port 2", "SFP slot")
and never exercises the contrib regex paths or the positional fallback
on real LibreNMS naming. As a result, no test covered the actual prod
data shape, and a flawed "walk all ancestors as bay candidates" fix
(216fb84, since reverted) passed all tests despite landing transceivers
in the wrong bay.
Capture the real shape from a Cisco WS-X4908-10GE linecard:
chassis "Switch System"
container "Slot 3" [no model]
module "Linecard(slot 3)" [WS-X4908-10GE]
container "Port Container 3/2"
other "Converter 3/2" [CVR-X2-SFP]
container "Port Container 3/11"
port "GigabitEthernet3/11" [GLC-TE]
container "Port Container 3/12"
port "GigabitEthernet3/12" [GLC-T]
Tests assert each level resolves correctly:
- linecard via `^Linecard\(slot (\d+)\)$` regex -> device-bay "Slot 3"
- converter via parent `^Port Container (\d+)/(\d+)$` regex -> "X2 Port 2"
- GE inside CVR via positional fallback -> "SFP 1" / "SFP 2"
- GE shows "No Bay" when CVR is matched but uninstalled in NetBox
A separate regression test uses a no-Converter-entry hierarchy
(Linecard -> PC 3/2 -> PC 3/11 -> GE3/11) where bay scope bubbles to
the linecard's bays. In this scope, ancestor-walking would resolve
the grandparent `Port Container 3/2` to `X2 Port 2` and incorrectly
land the transceiver in the parent module's slot. This test fails
if 216fb84-style logic is re-introduced.
Add a `_load_contrib_bay_mappings` helper that loads the contrib YAML
as fake mapping objects and a `bay_mappings=` kwarg on `_run_build_context`
so tests can opt into the real mapping set instead of the empty default.
* fix: bail _match_bay_by_position on non-container scaffolding
Cisco IOS-XR exposes deep entity-MIB scaffolding under each linecard:
modules with class="module" and model="N/A" (Motherboard, Slice 0,
EZChip, "Slice 0 SFP Port Module #N"). The positional walk previously
treated every model-less ancestor as a walk-through container, kept
reassigning container_idx until it found the linecard's real model,
and then took the position of the deepest ancestor among the linecard's
direct children.
On a real ASR-9904 (NetBox device prod-lab03d-ra1.lab) every TenGigE
port descended through the same Motherboard chain, so they all reported
container_idx=Motherboard and resolved to position=1 -> Slot 1 on the
chassis. The chassis Slot 1 holds the RSP line card; clicking install
on a TenGigE row would write an SFP module into the RSP bay, then any
subsequent install rejected with "Module bay 'Slot 1' already has a
module installed".
Restrict the walk to ENTITY-MIB containers (entPhysicalClass="container").
A modelless ancestor of any other class is hierarchical scaffolding, not a
bay position; walking past it silently collapses sibling counts. Bail
when we hit one — the row drops to "No Bay" instead of confidently
mismatching.
Tests:
- TestPositionalMatchScaffoldingChain (new): captures the ASR-9904
shape and asserts (1) TenGigE rows do NOT match Slot 1, (2) status
is "No Bay", (3) sibling rows resolve independently rather than
collapsing to a single bay.
- TestMatchBayByPosition (updated): existing tests omitted
entPhysicalClass on synthetic containers; add it explicitly so the
fixtures match real LibreNMS data shape and the positional walk's
class check passes.
Verified live: all 11 TenGigE0/0/0/N rows on device 54 now show
"No Bay" instead of collapsing to "Slot 1". RSP0/RSP1 and power
supplies still match correctly via their own positional paths.
* fix: restrict serial_matches_device rule to chassis-level entries
The 'Embedded RP / fixed-chassis system board' ignore rule
(match_type=serial_matches_device, action=transparent) targets fixed-form
routers like Cisco 8201-SYS / 8100, where the system board is reported as
an ENTITY-MIB entry whose serial equals the device record's serial.
Marking it transparent hides the row and promotes its children
(transceivers, fans, PSUs) to device-level bay matching.
The match criterion was just "item.serial == device.serial" with no
location check. On chassis devices like the ASR-9904, line cards can
share the device serial when the operator set Device.serial to the
linecard's serial (LibreNMS doesn't necessarily report the chassis serial
as the device serial). The rule fired on the linecard, hid it, and
promoted every TenGigE child to chassis-level matching.
Combined with the previous positional-walk bug, this collapsed every
TenGigE0/0/0/N row to the same chassis Slot 1 bay; in isolation it would
silently land transceivers in chassis line-card bays.
Tighten the criterion: only fire when the item is at chassis level —
either it has no parent (top-level entity) or its direct parent has
entPhysicalClass="chassis". System boards on fixed-form routers satisfy
this; line cards inside slot containers don't.
Tests cover three new shapes:
- parent is chassis -> rule fires (fixed-form router)
- parent is container -> rule does NOT fire (ASR-9904 linecard)
- parent is module -> rule does NOT fire (nested submodule)
Verified on live device 54: the 0/0 linecard now appears as its own
row instead of being hidden, and its TenGigE descendants resolve to
"No Bay" instead of collapsing to a chassis slot.
* fix: class-aware positional fallback + model gap warnings
The positional fallback in _match_bay_by_position previously tried
[SFP N, Slot N, Bay N, Port N] for every item regardless of hardware
class. On chassis devices whose NetBox model defines only line-card
slots (Slot 0..3) but no Fan Tray / PSU bays, fans and power supplies
landed in line-card "Slot N" bays. Example on ASR-9904 device 54:
- 0/FT0 (fan) -> Slot 3
- 0/PT0-PM0 (powerSupply) -> Slot 2
- 0/PT0-PM1 (powerSupply) -> Slot 3
Pick patterns appropriate for the item class:
- fan -> Fan Tray N / Fan N / FT N
- powerSupply -> Power Supply N / PSU N / PEM N / PM N
- module / port / ioModule / cpmModule / mdaModule / fabricModule
/ xioModule -> Slot N / SFP N / Bay N / Port N
- other classes (sensor, etc.) -> no positional guess
Items whose class has no matching bay name in scope drop to No Bay
rather than confidently mismatching.
Surface NetBox-model gaps: each No Bay / No Type row now carries a
model_warning field describing the most likely missing piece:
- empty bay scope -> parent module type has no bay templates
- class-specific -> add bay templates with the expected names
- missing type -> No NetBox ModuleType matches '<model>'
The modules-table render_status surfaces this as a tooltip on the
status badge plus an alert icon, mirroring the existing
name_conflict_warning rendering.
Tests:
- TestPositionalMatchClassAware: 6 cases covering fan/PSU/module
behavior plus unknown-class fallback to None.
- TestNoBayWarningHints / TestNoTypeWarningHints: helper output
distinguishes the three causes.
- TestBuildRowModelWarning: integration check that _build_row
populates model_warning on the right rows.
- test_tables_modules.py: render_status surfaces model_warning as
a tooltip with the alert icon.
Verified on device 54: 0/FT0 and 0/PT0-PMx now show No Bay with
class-appropriate hints instead of landing in chassis line-card slots.
* feat: ModuleBayMapping suggestions + Add Mapping button on No Bay rows
When a row resolves to "No Bay" because the LibreNMS name doesn't match
any bay in scope, propose a regex ModuleBayMapping covering the whole
slot family rather than one entry per slot. Heuristic: when the item's
name ends with a number and a bay in scope ends with the same number,
suggest "^<prefix>(\d+)$ -> <bay-prefix>\1" with the item's class as
filter. Example: item "0/0" + bay "Slot 0" yields
"^0/(\d+)$ -> Slot \1, class=module".
Suppressed in three cases:
- scope_preserved=True (scope inherited from an unmatched ancestor)
- Class/bay mismatch (fans only match Fan/FT bays, etc.)
- scope_uninstalled (warning already redirects to install parent first)
UI:
- render_status surfaces the suggestion in the badge tooltip.
- render_actions adds an "Add Mapping" button on No Bay rows with
model_suggestion. Opens ModuleBayMapping create form pre-filled via
NetBox ObjectEditView GET-param initial. return_url is captured from
configure(request) for round-trip.
Plumbing:
- _build_row gains scope_uninstalled and scope_preserved kwargs.
- The iteration loop tracks both states alongside bays_by_depth.
Defaults fall back to top-level state so first sub-item iteration
inherits correct semantics.
- _build_no_bay_warning gains a scope_uninstalled branch ("install the
parent module first") and appends suggestion when provided.
Verified live on device 54 (ASR-9904):
- 0/0 (No Bay, top-level) -> SUGGEST ^0/(\d+)$ -> Slot \1
- TenGigE0/0/0/N (preserved scope) -> no suggestion
- 0/FT0 (fan, no fan bays) -> no suggestion (class filter)
- 0/PT0-PMx (powerSupply) -> no suggestion (class filter)
* fix: address valid code-review findings
- testing.instructions.md: add test_coverage_bulk_import.py and
test_coverage_utils.py to coverage-by-module table
- models.py InterfaceTypeMapping.clean(): normalize/strip librenms_type
and reject blank values before wildcard uniqueness check
- librenms_sync.js: always clear device_selection cell on row update,
even when backend returns empty, so stale VC dropdowns are removed
- tables/modules.py VCModuleTable: hide device_selection column by
default (visible=False); guard format_module_data against missing VC
- utils.py load_bay_mappings(): use explicit order_by for deterministic
regex precedence
- modules_view.py: log raw LibreNMS errors server-side and show generic
message to users (inventory fetch failure + transceiver fetch warning)
- test_coverage_filters.py: make cache-key assertions order-independent
- test_vm_operations.py: add missing existing_device key to mock dict
* feat: remove {module} conflict warning, add Generic manufacturer fallback for module type matching
* fix: replace {module} conflict tooltip with Name Conflict status badge; add Generic manufacturer fallback
- Remove the warning tooltip about {module} causing non-unique interface
names; instead show a 'Name Conflict' status badge (bg-warning text-dark)
when has_nested_name_conflict() detects sibling name collisions
- has_nested_name_conflict() and sibling_counts tracking are preserved
- Add get_generic_module_types_indexed() and generic_fallback support in
resolve_module_type() so 'Generic' manufacturer matches are tried when
no vendor-specific ModuleType is found
- Pre-fetch generic module types once per request in _get_generic_module_types()
- render_status() no longer reads name_conflict_warning (never set); reads
model_warning only for the alert-icon tooltip
- Restore has_nested_name_conflict patches in test_modules_view.py and
test_sync_modules.py; add sibling…
This was referenced May 23, 2026
Closed
Merged
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continuation of #58.
Summary
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests