Release 0.4.7#308
Merged
Merged
Conversation
Add inventory/modules sync functionality: - Six mapping model types: DeviceTypeMapping, ModuleTypeMapping, ModuleBayMapping, NormalizationRule, InventoryIgnoreRule, PlatformMapping - Migration 0010 creating all mapping tables - Modules sync tab on Device/VM detail pages with ENTITY-MIB inventory data - Install, replace, and move module actions - Mapping CRUD views with YAML bulk export for all mapping models - LibreNMS API: get_device_transceivers() for transceiver data - Platform matching: PlatformMapping lookup before name-exact match - contrib/ YAML files with example mappings and rules - Comprehensive test coverage (test_sync_modules, test_modules_view, test_module_replace, test_platform_mapping, test_tables_modules)
… behavior _is_job_cancelled now returns False on RQ/Redis unavailability instead of falling back to DB status. Update 5 tests that were asserting the old DB-fallback behavior: - test_db_fallback_logs_via_module_logger_when_job_logger_none → test_rq_unavailable_does_not_cancel_import: RQ unavailable means processing continues, not cancelled - test_job_db_fallback_stopped/errored_before_validation_loop → test_job_cancelled_before_validation_loop_returns_empty / test_rq_unavailable_job_not_cancelled_in_preloop: patch _is_job_cancelled directly to test early-exit behavior - test_job_validation_loop_db_fallback_stop → test_job_cancelled_in_validation_loop_returns_empty: use _is_job_cancelled side_effect to simulate mid-loop cancellation - test_job_rq_check_exception_uses_db_status_and_exits → test_rq_fetch_exception_does_not_cancel_process_filters: assert result has 1 device (not []) when RQ unavailable
- Remove dead _resolve_naming_preferences from actions.py (never called) - Unify vc_detection_enabled parsing in BulkImportConfirmView (POST+GET) - Add ambiguity detection to get_module_types_indexed second loop - Fix get_validated_device_cache_key doctest (wrong e3b0 hash) - Add status=='ok' envelope check before transceivers shape validation - Strip netbox_bay_name in ModuleBayMapping.clean() - Use server_info.server_key in _module_sync.html refresh form - Guard module_mismatch_modal Update Serial form on serial_conflict/installed - Remove duplicate NoSuchJobError import in test_coverage_api2.py - Fix _is_job_cancelled side_effect count for in-loop cancellation test - Precompute sibling_counts in _build_table_rows to eliminate N+1 queries - Accept sibling_counts in has_nested_name_conflict (DB fallback preserved) - Update test_has_installable_children fake_build_row signature - Coerce entPhysicalParentRelPos to int in siblings sort (prevent TypeError) - Move get_queue inside try block in api/views.py sync_job_status - Replace MD5 with SHA256 for VC domain fingerprint (FIPS compliance) - Fix test_role_is_read_from_validation to test validation dict path - Add test_module_replace.py to docs/development/testing.md - Add platform_mappings.yaml row to contrib/README.md - Fix QSFP28-DD-2X100G-LR4 typo in module_type_mappings.yaml - Clarify librenms_id auto-create in docs/usage_tips/custom_field.md
- Unify librenms_id auto-create version reference to 0.4.3 in docs
- Move _LIBRENMS_JOB_NAMES to module-level constant in api/views.py
- Add status=='ok' envelope check in port handler in librenms_api.py
- Split mapping ambiguity tracking in get_module_types_indexed (separate
mapping_seen/mapping_ambiguous so explicit mappings always win over base
ModuleType ambiguity)
- Handle match_type=='ambiguous' in find_matching_platform caller with
dedicated warning message about conflicting platform mappings
- Move vc_requested parse before validate_device_for_import in
BulkImportConfirmView and pass include_vc_detection=vc_requested
- Guard Replace Module form with {% if installed_module %} in
module_mismatch_modal.html to prevent empty module_id submission
- Add mock_db_job.status/completed assertions to api2 test
- Extend cancellation test side_effects to 4 entries to target in-loop
check (lines 507, 534, 566, 574) in both RQ and _is_job_cancelled tests
- Assert success list in test_no_warning_when_cluster_found
…t, module bay normalization, BulkExportYAMLView permissions - test_coverage_sync_views2: patch get_object_or_404 to isolate from ORM - test_coverage_device_fields: assert save/full_clean not called on no-match - test_librenms_id: use exact tuple set comparison for Q branch children - test_init: assert DB alias used in custom field creation - utils.py: move PlatformMapping import to function scope (lazy); update all test patches to models.PlatformMapping; remove create=True - test_platform_mapping: patch require_object_permissions instead of require_write_permission - test_sync_modules: mock apply_normalization_rules in _match_module_bay tests - views/base/modules_view.py: apply NormalizationRule(scope=module_bay) to candidate names in _match_module_bay - views/mapping_views.py: BulkExportYAMLView uses NetBoxObjectPermissionMixin with view permission - views/object_sync/devices.py: precompute has_write_permission once in get_table()
- utils.py: add prefetch_related('interfacetemplates') to ModuleType query
and prefetch_related('netbox_module_type__interfacetemplates') to
ModuleTypeMapping query in get_module_types_indexed() to avoid N+1
queries in has_nested_name_conflict()
- views/mapping_views.py: add .order_by('pk') to BulkExportYAMLView filter
for deterministic YAML export; add select_related() to all 4 export
subclass querysets (DeviceTypeMapping, ModuleTypeMapping,
NormalizationRule, PlatformMapping)
- tests/test_utils.py: assert exact Platform.objects.get kwargs in 2 platform
tests; fix vc.master = None -> vc.master = master to exercise designated-
master-without-IP branch
- tests/test_platform_mapping.py: update mock chain for .order_by(); complete
test_returns_yaml_content_type with actual view call and content-type assertion
- tests/test_sync_modules.py: fix mock chain for .prefetch_related() in
TestGetModuleTypesIndexed
- utils.py: fix apply_normalization_rules() else-branch to filter manufacturer__isnull=True so callers without manufacturer context never have vendor-specific rules applied to their values - tests/test_sync_modules.py: add test_mapping_overrides_ambiguous_base_key to lock down the separate-ambiguous-sets behaviour in get_module_types_indexed(); fix test_regex_mapping_with_backreference to use 'Optics 0/0/0/5' (with space) so the assertion cannot pass via exact-name fallback — only the regex expansion path can produce a match - tests/test_platform_mapping.py: remove dangling assertions accidentally left inside test_returns_200_with_empty_selection (PlatformMapping import and existence check now live in test_all_mapping_bulk_export_yaml_views_exist)
- utils.py: add preload_normalization_rules() helper that preloads NormalizationRule rows for a (scope, manufacturer) combination into a dict keyed by (scope, manufacturer_pk_or_None); update apply_normalization_rules() to accept preloaded_rules kwarg and use preloaded lists when provided (skipping DB queries); update resolve_module_type() to accept norm_rules kwarg and thread it through to apply_normalization_rules — eliminates N+1 DB queries in _match_module_bay and _build_row loops - views/base/modules_view.py: call preload_normalization_rules() in _build_context for both 'module_bay' and 'module_type' scopes; pass preloaded rules via self._norm_rules_bay/_norm_rules_type to _match_module_bay and _build_row respectively - tests/test_platform_mapping.py: add test_multiple_platform_mappings_returns_ambiguous asserting PlatformMapping.MultipleObjectsReturned yields match_type='ambiguous' and that Platform.objects.get is never called - tests/test_sync_modules.py: fix test_mapping_overrides_ambiguous_base_key to use distinct mapping key 'SFP-1G-LX-EXPLICIT' so 'SFP-1G-LX' is absent and only the explicit key is present; fix test_uninstalled_bay_is_skipped to add grandparent bay with installed module (pk=99) and assert walk continues past empty bay to return 99; fix test_class_scoped_mapping_preferred to pass [m_generic, m_class] so priority logic must actively prefer class-scoped mapping; patch preload_normalization_rules in tests that call _build_context directly; update apply_normalization_rules lambda patches to accept **kw
- _fpc_slot_matches: convert match.group(1) and parent_bay.position to int before comparing (Python 3: '1' == 1 is False); handle ValueError with safe fallbacks - apply_normalization_rules docstring: clarify that manufacturer=None applies only unscoped (manufacturer__isnull=True) rules, not all scope rules - test_modules_view._run_build_context: patch load_bay_mappings and get_enabled_ignore_rules at utils level instead of patching model classes that _build_context never references directly; remove now-unused mock_ignore_qs variable
…match, transceiver ignore - actions.py sync_platform: add explicit elif for match_type='ambiguous' so users get a clear conflict message instead of generic 'not found' error when multiple PlatformMapping rows match the same OS string (works towards #51) - utils.py apply_normalization_rules: when preloaded_rules is provided, check key presence before using the dict; fall back to DB query when (scope, mfg_pk) or (scope, None) is absent, preventing silent omission of vendor rules for manufacturers not included in the preloaded dict - utils.py match_librenms_hardware_to_device_type: update Returns docstring to dict | None and document the MultipleObjectsReturned → None case - modules_view.py _apply_installed_status: drop nb_serial from the guard so a module with a LibreNMS serial is flagged as Serial Mismatch even when NetBox has no serial recorded (lnms_serial and lnms_serial != nb_serial) - modules_view.py _collect_top_items: apply ignore-rule check to transceiver- synthesised items before appending, so InventoryIgnoreRules can suppress optics from get_device_transceivers() - test_modules_view.py: rename test that expected the old nb_serial-required behavior; update assertions to Serial Mismatch + can_update_serial + can_replace - test_modules_view.py: wrap two early-return _detect_serial_conflicts tests in patch(dcim.models.Module) and assert filter was never called
…t, vc flag case - utils.py find_by_librenms_id: strip whitespace and canonicalize leading-zero string IDs before building Q filters; '042' and '42 ' now resolve to int_value=42 / canonical_str='42' and both forms are added to the query so they match records stored as numeric 42 or string '42' - modules_view.py _find_parent_container_name: use (... or '') pattern to guard against entPhysicalName being explicitly None in the ENTITY-MIB payload - modules_view.py _match_module_bay: same None guard for entPhysicalName, entPhysicalDescr, entPhysicalClass on the item dict - modules_view.py _collect_top_items: treat 'transparent' the same as 'skip' for transceiver-synthesised rows so transparent synthetic items are not added to top_items - actions.py BulkImportDevicesView: normalise vc_detection_enabled flag with .lower() before membership test so 'ON', 'True', 'TRUE' all parse correctly, consistent with BulkImportConfirmView
- bulk_import: check cancellation every iteration (not every 5th) - models: always call full_clean() on save, remove update_fields guard - tables/modules: add has_write_permission param to LibreNMSModuleTable, gate selection column and render_actions on it - modules_view: normalize placeholder model/serial strings in transceiver merge; filter placeholder serials from inv_serials set - api/views: skip DB status update when job is already in a terminal state - utils: add 'ambiguous' case to find_matching_platform docstring - utils: memoize DB fallback into preloaded_rules in apply_normalization_rules - utils: use try/except int() instead of isdigit() for +42 style IDs - devices: pass has_write_permission to LibreNMSModuleTable constructor - test_modules_view: use SimpleNamespace instead of MagicMock in _determine_status tests for unambiguous truthiness checks - test_sync_modules: assert checkbox HTML in selection cell; update test_install_module_view_not_in_base to assert public import path; add order-independent check for class-scoped mapping preference - test_tables_modules: set has_write_permission=True in _make_table helper; add no-write-permission test case - test_utils: assert exact kwargs on DeviceTypeMapping.objects.get and PlatformMapping.objects.get calls - test_vm_operations: patch _is_job_cancelled directly instead of mutating job.job.status via refresh_from_db side_effect - test_coverage_base_views2: rename test to reflect actual behavior (cache entry present but lacks port_id) - test_coverage_devices: update constructor assertion to include has_write_permission kwarg
- models: add FullCleanOnSaveMixin + clean() to InterfaceTypeMapping to enforce uniqueness for NULL-speed rows (SQL UNIQUE skips NULL=NULL) - utils: expand match_librenms_hardware_to_device_type docstring to document all three fail-closed None cases (mapping, part_number, model MultipleObjectsReturned), not just the mapping-table one - test_vm_operations: remove stale mock_job.job.status='running' from _run_bulk_with_mappings helper; patch _is_job_cancelled=False instead
…o_kbps docstring - DeviceTypeMapping.clean() and PlatformMapping.clean() now lowercase the stored value after stripping, preventing case-variant duplicates (e.g. 'IOS' and 'ios') that would cause MultipleObjectsReturned on __iexact lookups. Closes #51. - Fix convert_speed_to_kbps docstring: Returns section now reads 'int | None' to match the signature and implementation. - Add TestDeviceTypeMappingModel tests for DeviceTypeMapping.clean() (strip, lowercase, blank validation). - Add test_clean_normalizes_to_lowercase and test_clean_strips_and_lowercases to TestPlatformMappingModel.
- hideModal: remove all backdrops via querySelectorAll+forEach - htmx:afterSettle: derive label from aria-labelledby, fallback to id - initializeVlanModalSave: truncate/extract error body before display
… issues - utils.py: guard unsaved manufacturer (pk=None) in preload_normalization_rules and apply_normalization_rules to prevent ValueError on DB query - librenms_sync.js: add id="htmx-modal-label" to module mismatch modal header so aria-labelledby target is preserved after innerHTML replacement - librenms_sync.js: check response.ok before response.json() in deleteUrl fetch so HTTP errors surface their status instead of a parse error
…ener - utils.py: fix convert_speed_to_kbps parameter annotation to int|None - utils.py: guard non-positive numeric librenms_id (<=0) same as None; skip Q canonicalization for string '0'/'-1' after int parse - librenms_sync.js: extract updateHtmxModalLabel(), listen at document level for htmx:afterSettle, call from module-replace fetch completion
- modules_view.py: _apply_installed_status and _detect_serial_conflicts now use _PLACEHOLDER_VALUES set instead of only guarding against "-" so serials like 'unknown', 'n/a', 'na' are treated as absent - utils.py: update convert_speed_to_kbps Args docstring to int|None
…, ChainMap bay lookup
- device_fields.py: distinguish None (ambiguous) from failed match result;
surface a specific error for duplicate DeviceType mappings
- utils.py: find_matching_platform returns {found:False, match_type='ambiguous'}
on Platform.MultipleObjectsReturned instead of silently taking .first()
- modules_view.py: invalidate stale inventory cache before early-return renders
when librenms_id is falsy or inventory fetch fails
- modules_view.py: _lookup_regex_bay_mapping iterates all ChainMap scopes
so same-named bays in different scopes are all checked
- tests: update TestFindMatchingPlatformMultipleReturned to expect ambiguous
Move inline set literals SKIP_TYPES and _NON_HARDWARE_CLASSES out of their method bodies and into module scope, consistent with _PLACEHOLDER_VALUES and other module-level constants. Rename SKIP_TYPES to _SKIP_TRANSCEIVER_TYPES to clarify its domain.
…_id guard - utils.py: broaden find_matching_platform docstring to state 'ambiguous' applies both to multiple PlatformMapping entries and to duplicate exact-name Platform rows (Platform.MultipleObjectsReturned) - utils.py: add early-return guard in find_by_librenms_id for string librenms_id values that parse to <= 0 (e.g. '0', '-1', '000'), preventing them from reaching the Q clauses and matching stale/corrupted records
_check_ignore_rules: normalize item_serial, device_serial, and ancestor_serial against _PLACEHOLDER_VALUES so sentinels like 'unknown'/'n/a' are treated as absent and do not trigger or short-circuit serial_matches_device rules or require_serial_match_parent ancestor walks. _merge_transceiver_data: normalize txr_type against _PLACEHOLDER_VALUES (same as model/serial) so placeholder types like 'unknown' do not bypass the _SKIP_TRANSCEIVER_TYPES guard and produce synthetic rows with display_model set to a placeholder string.
… exact-bay fallback, has_write_permission in HTMX render - utils.py: treat whitespace-only librenms_id strings as absent (return None after strip() when cleaned == "") so they don't reach the Q-object builder - modules_view.py: extend transceiver backfill to also replace 'BUILTIN' model and serial values, not just those already in _PLACEHOLDER_VALUES - modules_view.py: exact-name bay fallback now iterates ChainMap scopes and calls _fpc_slot_matches() to avoid returning the wrong-scope bay when duplicate bay names exist across scopes (mirrors the regex path behaviour) - modules_view.py: add has_write_permission to all render() calls in post() so the Install Selected button is visible in HTMX-refreshed content
…xact-mapping ChainMap scope - utils.py: non-integer strings (e.g. 'abc') now return None in find_by_librenms_id() instead of falling through to build Q objects; changed 'except ValueError: pass' to 'except ValueError: return None' - modules_view.py: get_context_data() re-validates the device's current LibreNMS ID before serving cached inventory; clears cache and returns empty context when the mapping has been removed since the cache was written - modules_view.py: _lookup_exact_bay_mapping() now iterates ChainMap scopes and calls _fpc_slot_matches() before returning, matching the existing behaviour of _lookup_regex_bay_mapping() and the name-fallback path
…_id in inventory cache
- modules_view: lowercase _GENERIC_CONTAINER_MODELS set and apply .lower() at
all 5 comparison sites so values like 'builtin', 'default', 'n/a' received
from LibreNMS in any case are treated as generic containers
- modules_view: store {'inventory': data, 'librenms_id': id} in the inventory
cache instead of the raw list; get_context_data validates the embedded
librenms_id against the current mapping so remapped devices never serve stale
inventory (non-dict/legacy entries are treated as cache misses)
…silently overwriting When multiple Module objects share the same serial, the old loop would overwrite row["serial_conflict_module"] nondeterministically. Now we group conflicts by serial and only set the move target when exactly one candidate exists; multiple candidates set serial_conflict_ambiguous.
… from phys class - modules_view: get_context_data now calls cache.delete(cache_key) before returning when the cached payload is not the new dict format so pre-upgrade list-form entries are evicted and the next request regenerates fresh data - modules_view: collapse the two-check current-item filter into a single 'model in _GENERIC_CONTAINER_MODELS' test (removes the phys_class=='container' gate) so empty-model non-container items are also treated as generic - modules_view: remove the anc_class=='container' guard from the ancestor walk so any inventory-class ancestor with a generic model (e.g. a 'module' row with model='builtin') is treated as transparent instead of blocking its subtree
The rebase onto pr/code-quality-fixes dropped the consumer of vc_requested in favor of the hoisted vc_detection_enabled variable, leaving the assignment itself as an orphan that ruff F841 flags.
…h fail-closed behavior - virtual_chassis.py: only treat stack as 0-based when positions span 0 AND a positive value. When every entPhysicalParentRelPos is 0 the data is invalid and the shift produced colliding positions (all members → slot 1); fall through to the per-member idx+1 fallback instead. - test_coverage_bulk_import.py: PR #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.
…k result AddPlatformMappingView now fails closed (error response) when more than one PlatformMapping row exists for the same librenms_os string, both before the permission check (fast path) and inside the transaction.atomic() block after SELECT FOR UPDATE (to catch concurrent duplicates). bulk_import_vms now includes 'created_ips' in each success entry, populated from vm._librenms_created_ips, mirroring bulk_import_devices_shared so callers receive consistent IPAM creation feedback for VMs.
…stale-lookup guards, simplify auto_create_ipam coercion - Add hidden server_key input to device-type and platform mapping forms so multi-server POSTs resolve against the correct LibreNMS API/cache namespace (AddDeviceTypeMappingView/AddPlatformMappingView read request.POST[server_key]) - Guard the typeahead .catch handlers with seq !== requestSeq so a stale failed lookup can't hide the dropdown from a newer successful one - Drop redundant string truthy-parsing of auto_create_ipam in bulk_import_vms and import_single_device; the flag is already normalised to bool by resolve_auto_create_ipam() at the request boundary
Drops the unassigned-global-/32 pre-create behaviour from device/VM import. It added busy import side-effects, was stuck in the default VRF, and never actually set primary_ip (NetBox requires the IP be interface-assigned), so it only parked records for manual wiring. Replaced (next commit) by Primary-IP auto-match on the IP-sync tab. Removed: ip_helpers.py + tests, migration 0011 + the auto_create_ipam_default model/form field, resolve_auto_create_ipam, the import-utils re-export; the pre-create blocks + created_ips plumbing in device_operations/vm_operations/ bulk_import; the import-page and settings-page toggles + their JS; and Kept: PR #84 server_key inputs + .catch stale-lookup guards, the PlatformMapping duplicate guard, and the htmx_toasts success/failed/skipped mechanism. oob-sync still references the removed helper for OOB/promote; that is reworked to interface-assigned IPs in a later commit on feat/oob-sync.
Replaces the removed import-time auto-create with an interface-assigned model. When the 'Set as Primary IP' toggle is on, SyncIPAddressesView matches the synced IP against the device/VM's LibreNMS management IP (from get_device_info) and, if that IP ends up assigned to one of the object's interfaces, sets it as primary_ip4/primary_ip6. This satisfies NetBox's interface-assignment constraint for primary_ip and creates no unassigned global records. - utils.resolve_set_primary_ip: POST/GET toggle -> user pref -> False cascade - SaveUserPrefView: allow the set_primary_ip pref - SyncIPAddressesView: get_management_ip / _same_host / _set_primary_ip helpers; process_ip_sync sets primary + reports it; display_sync_results surfaces it - IP-sync tab: 'Set as Primary IP' toggle, pre-checked from pref, persisted via save_user_pref - tests for the cascade and the process_ip_sync primary-set behaviour
…s on Flags the IP-sync row whose IP equals the device/VM's LibreNMS management IP (enrich_ip_data -> data-mgmt-ip row attribute) and auto-ticks its select box when the 'Set Primary IP' toggle is enabled, so syncing it sets the Primary IP in one action. Mirrors the parent-child row-auto-select construct (data-* attribute + JS), keyed off the LibreNMS-mapped interface. - utils.same_host: shared host-equality helper (SyncIPAddressesView._same_host now delegates to it) - BaseIPAddressTableView._flag_management_ip: marks the matching enriched entry - IPAddressTable: data-mgmt-ip row attribute - _ipaddress_sync_content.html: toggle JS now also auto-checks the mgmt row - tests for _flag_management_ip
…erface
- Wrap each IP in its own savepoint so one bad address rolls back only itself
instead of poisoning the whole batch; log the exception and include the
reason in the failure toast (previously a bare 'Failed: <ip>').
- When 'Set Primary IP' is on and the management IP has no NetBox interface,
emit a clear warning ('Sync interfaces first, then re-run') instead of
silently skipping the primary assignment.
…e pref A 403/500 from save_user_pref was silently swallowed (fetch only rejects on network errors), so the toggle looked persisted when it wasn't. Addresses CodeRabbit review on PR #84.
filter(address=...) alone can match the same address in a different VRF and then rewrite its VRF on save, hijacking an unrelated IP. Scope the lookup to the selected VRF so a new IP is created in the target VRF instead. Addresses PR #79 CodeRabbit.
docs: inventory module, mapping rules, and updated screenshots
feat: set primary IP action
Add a minimal top-level 'permissions: contents: read' block to the test, lint-format and publish-pypi workflows to follow least-privilege and resolve CodeQL actions/missing-workflow-permissions alerts. The publish-to-pypi job keeps its own job-level 'id-token: write' permission required for PyPI trusted publishing; job-level permissions override the top-level default, so publishing is unaffected.
CodeQL could not trace the url_has_allowed_host_and_scheme sanitizer across the _get_safe_redirect_url helper boundary, flagging py/url-redirection on the redirect() calls in require_write_permission and require_object_permissions. Add a _safe_redirect_response helper that re-applies the allowed host/scheme guard inline, in the same scope as the redirect() sink, and route both permission-denied paths through it. Behaviour is unchanged; the guard is now local to the sink so the sanitizer barrier is traceable.
CodeQL py/stack-trace-exposure (alerts 10, 11, 12 on develop) flagged the IPAddress sync POST handler for returning str(exception) directly to the client. Replace these with safe, fixed messages in ip_addresses_view.post: - 'Invalid JSON payload' instead of echoing the JSONDecodeError - reconstructed 'Object with ID <id> not found' instead of the Http404 text - a fixed invalid-prefix message instead of the ValueError text Status codes and validation logic are unchanged.
ci: restrict GITHUB_TOKEN permissions in workflows
The previous _safe_redirect_response used a negative guard
('if not url_has_allowed_host_and_scheme(...): target = "/"') which
CodeQL's Django taint model did not recognise as a sanitizer barrier,
so it raised a fresh py/url-redirection alert on the helper.
Restructure to the canonical positive-guard pattern: validate the target
and place the redirect/HX-Redirect sinks inside the validated branch,
with a hard-coded '/' fallback otherwise. Behaviour is unchanged (a
relative request.path fallback still passes validation); only genuinely
unsafe targets now redirect to '/'.
fix: localise open-redirect guard to redirect sink
fix: avoid exposing exception details in IP address sync responses
Address maintainer review on PR #298: - send -> send_robust in get_module_template_interface_names so a buggy third-party receiver can't break module adoption (matches the hook's loosely-coupled design goal). Exception results are logged and skipped, preserving the 'last non-None wins' ordering. - Add logger.debug(exc_info=True) on both template.instantiate() swallow sites (get_module_template_interface_names + detect_vc_normalization_noop) so 'why is no report button showing?' is diagnosable. - Tests: failing receiver is isolated; sole failing receiver falls back to raw names.
PR #298 CR follow-up: verify the WARNING is emitted (not just that the exception is swallowed) via caplog scoped to netbox_librenms_plugin.utils.
…dict-hook Feat/module interface name predict hook
- Add Module Sync and Mapping Rules to mkdocs and SUMMARY navigation - Document the opt-in "Set Primary IP" toggle on IP Address Sync (#303) - Add developer Extension Points page for the predict_module_interface_names signal and VC normalization report (#298) - Note module-interface adoption in the Module Sync guide (#296) - Sync root README and docs README feature sections (Module/Inventory Sync, hardware-model filter, Platform Mappings link, multi-server)
docs: review and fill gaps for v0.4.7 release
WalkthroughAdds module/inventory sync across UI, templates, and JS; introduces mapping/rule models with migrations, serializers, viewsets, filters, forms, tables, and navigation; updates LibreNMS API client methods and cache keys for multi-server; refines device/VM import, VC handling, and IP primary setting; adds platform/device type mapping UIs; tightens CI workflow permissions; bumps version to 0.4.7; and expands documentation and tests accordingly. Sequence Diagram(s)Possibly related PRs
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (1)
1110-1121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the fallback toast for failed single-row imports.
The new early
returnrestores the previous selection, but it also skipsshowErrorToast()for programmatic one-row imports. A genuine 4xx/5xx now fails silently in that path.Suggested change
document.body.addEventListener('htmx:responseError', function (event) { if (event.target === bulkImportBtn && pendingRowImport) { restoreSelectionState(pendingRowImport.previousSelections); pendingRowImport = null; - return; } // Fallback for genuine 5xx / unexpected 4xx responses that bypass // the server-side _htmx_error_response helper (which returns 200 + // an OOB toast for expected validation errors). try {netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js (2)
100-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the manual modal fallback stack-safe.
This cleanup now removes every backdrop and always clears the body modal lock. If one fallback modal is closed while another is still open, the remaining dialog loses its backdrop and page scroll state.
Suggested fix
- document.querySelectorAll('.modal-backdrop').forEach((backdrop) => backdrop.remove()); - document.body.classList.remove('modal-open'); - document.body.style.removeProperty('padding-right'); - document.body.style.removeProperty('overflow'); + const otherOpenModals = Array.from(document.querySelectorAll('.modal.show')) + .filter((modal) => modal !== el); + if (otherOpenModals.length === 0) { + document.querySelectorAll('.modal-backdrop').forEach((backdrop) => backdrop.remove()); + document.body.classList.remove('modal-open'); + document.body.style.removeProperty('padding-right'); + document.body.style.removeProperty('overflow'); + }Based on learnings: nested modals are intentionally opened programmatically from HTMX-rendered fragments.
1405-1436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
updateInterfaceNameField()idempotent.
initializeScripts()runs after every HTMX swap, but this function adds a freshchangelistener on every pass. After a few swaps, one radio change will POST the preference multiple times and fire multiplehtmx:refreshevents.Suggested fix
function updateInterfaceNameField() { document.querySelectorAll('.interface-name-field').forEach(radio => { + if (radio.dataset.interfaceNameFieldInitialised) return; + radio.dataset.interfaceNameFieldInitialised = 'true'; + radio.addEventListener('change', function () { const url = new URL(window.location); url.searchParams.set('interface_name_field', this.value); window.history.pushState({}, '', url);Based on learnings: the master initializer
initializeScripts()must run on bothDOMContentLoadedandhtmx:afterSwapevents.
🧹 Nitpick comments (2)
netbox_librenms_plugin/models.py (1)
115-135: 💤 Low valueRemove redundant import of
ValidationError.
ValidationErroris already imported at module level (line 8). This local import on line 117 is unnecessary.def clean(self): """Enforce uniqueness for NULL-speed rows (SQL UNIQUE does not cover NULL = NULL).""" - from django.core.exceptions import ValidationError - super().clean()docs/usage_tips/mapping_rules.md (1)
3-3: 💤 Low valueConsider adding a comma for clarity.
The sentence would read more smoothly as: "...with them, you can cover vendor naming variations..."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: afdb6a8e-b9f0-4cbb-84ff-bdfe4933b6f4
⛔ Files ignored due to path filters (13)
docs/img/Netbox-librenms-plugin-device-sync-fields.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-import-page.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-module-sync-tab.pngis excluded by!**/*.pngdocs/img/carrier_auto_install_rules/list.pngis excluded by!**/*.pngdocs/img/device_type_mappings/list.pngis excluded by!**/*.pngdocs/img/inventory_ignore_rules/list.pngis excluded by!**/*.pngdocs/img/module_bay_mappings/list.pngis excluded by!**/*.pngdocs/img/module_type_mappings/add.pngis excluded by!**/*.pngdocs/img/module_type_mappings/list.pngis excluded by!**/*.pngdocs/img/normalization_rules/add.pngis excluded by!**/*.pngdocs/img/normalization_rules/list.pngis excluded by!**/*.pngdocs/img/platform_mappings/add.pngis excluded by!**/*.pngdocs/img/platform_mappings/list.pngis excluded by!**/*.png
📒 Files selected for processing (147)
.devcontainer/config/codespaces-configuration.py.github/copilot-instructions.md.github/instructions/background-jobs.instructions.md.github/instructions/frontend.instructions.md.github/instructions/release.instructions.md.github/instructions/testing.instructions.md.github/workflows/lint-format.yaml.github/workflows/publish-pypi.yaml.github/workflows/test.yamlREADME.mdcontrib/README.mdcontrib/carrier_auto_install_rules.yamlcontrib/device_type_mappings.yamlcontrib/interface_type_mappings.yamlcontrib/inventory_ignore_rules.yamlcontrib/module_bay_mappings.yamlcontrib/module_type_mappings.yamlcontrib/normalization_rules.yamlcontrib/platform_mappings.yamldocs/README.mddocs/SUMMARY.mddocs/changelog.mddocs/development/README.mddocs/development/extension_points.mddocs/development/testing.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/README.mddocs/usage_tips/custom_field.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/module_sync.mdmkdocs.ymlnetbox_librenms_plugin/__init__.pynetbox_librenms_plugin/api/serializers.pynetbox_librenms_plugin/api/urls.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/filters.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/cache.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0010_inventory_and_mapping_models.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/signals.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/__init__.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/mappings.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/carrierautoinstallrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_manage_icon.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/add_bay_template_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/create_platform_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/module_mismatch_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/vc_normalization_report.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inventoryignorerule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/platformmapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_api.pynetbox_librenms_plugin/tests/test_coverage_api2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_filters.pynetbox_librenms_plugin/tests/test_coverage_forms.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_init.pynetbox_librenms_plugin/tests/test_integration_virtual_chassis.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_module_replace.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_platform_mapping.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_interfaces.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_template_comments.pynetbox_librenms_plugin/tests/test_unique_constraints.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/__init__.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/modules.pypyproject.tomltests/e2e/__init__.pytests/e2e/conftest.pytests/e2e/test_device_type_mapping.pytests/e2e/test_module_install.py
💤 Files with no reviewable changes (1)
- netbox_librenms_plugin/tests/mock_librenms_server.py
Comment on lines
+22
to
+23
| # Development environment — logging config values is an accepted tradeoff here. | ||
| # CodeQL alert for this is dismissed intentionally. |
There was a problem hiding this comment.
Avoid codifying a security-alert dismissal without compensating controls.
These comments normalise ignoring the alert, but the file still uses ALLOWED_HOSTS = ["*"]. Even in dev containers, that broad host policy is a risky default and easy to propagate accidentally.
Suggested hardening
@@
- ALLOWED_HOSTS = [
- f"{codespace_name}-8000.{port_domain}",
- "localhost",
- "127.0.0.1",
- "*",
- ]
- # Development environment — logging config values is an accepted tradeoff here.
- # CodeQL alert for this is dismissed intentionally.
+ ALLOWED_HOSTS = [
+ f"{codespace_name}-8000.{port_domain}",
+ "localhost",
+ "127.0.0.1",
+ ]
+ # Development environment: keep diagnostics, but avoid broad host wildcards.
Comment on lines
+144
to
+170
| - librenms_name: "^PSM (\\d+)$" | ||
| librenms_class: "powerSupply" | ||
| netbox_bay_name: "PEM \\1" | ||
| is_regex: true | ||
| description: "Juniper chassis PSM N → PEM N" | ||
|
|
||
| # Juniper FPC container: "FPC: <description> @ N/*/*" → FPC N | ||
| - librenms_name: "^FPC: .+ @ (\\d+)/\\*/\\*$" | ||
| librenms_class: "container" | ||
| netbox_bay_name: "FPC \\1" | ||
| is_regex: true | ||
| description: "Juniper FPC container description → FPC N" | ||
|
|
||
| # Juniper transceivers: "<type> @ slot/pic/port" description → Transceiver slot/pic/port | ||
| - librenms_name: "^.+ @ (\\d+/\\d+/\\d+)$" | ||
| librenms_class: "port" | ||
| netbox_bay_name: "Transceiver \\1" | ||
| is_regex: true | ||
| description: "Juniper transceiver description → Transceiver slot/pic/port" | ||
|
|
||
| # Juniper fan trays: "Fan Tray N" → "Fan N" (ACX7100, etc.) | ||
| # Runs after exact match, so "Fan Tray 0" → "Fan Tray" (ACX7024) still works | ||
| - librenms_name: "^Fan Tray (\\d+)$" | ||
| librenms_class: "fan" | ||
| netbox_bay_name: "Fan \\1" | ||
| is_regex: true | ||
| description: "Juniper Fan Tray N → Fan N (ACX7100 etc.)" |
There was a problem hiding this comment.
Scope Juniper-specific regex rules to Juniper to prevent cross-vendor bay mis-matches.
These Juniper-labelled patterns are currently global. The broad forms (especially ^.+ @ (\\d+/\\d+/\\d+)$) can match other vendors and map to the wrong NetBox bay names.
Suggested patch
- librenms_name: "^PSM (\\d+)$"
librenms_class: "powerSupply"
+ manufacturer: "Juniper"
netbox_bay_name: "PEM \\1"
is_regex: true
description: "Juniper chassis PSM N → PEM N"
- librenms_name: "^FPC: .+ @ (\\d+)/\\*/\\*$"
librenms_class: "container"
+ manufacturer: "Juniper"
netbox_bay_name: "FPC \\1"
is_regex: true
description: "Juniper FPC container description → FPC N"
- librenms_name: "^.+ @ (\\d+/\\d+/\\d+)$"
librenms_class: "port"
+ manufacturer: "Juniper"
netbox_bay_name: "Transceiver \\1"
is_regex: true
description: "Juniper transceiver description → Transceiver slot/pic/port"
- librenms_name: "^Fan Tray (\\d+)$"
librenms_class: "fan"
+ manufacturer: "Juniper"
netbox_bay_name: "Fan \\1"
is_regex: true
description: "Juniper Fan Tray N → Fan N (ACX7100 etc.)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - librenms_name: "^PSM (\\d+)$" | |
| librenms_class: "powerSupply" | |
| netbox_bay_name: "PEM \\1" | |
| is_regex: true | |
| description: "Juniper chassis PSM N → PEM N" | |
| # Juniper FPC container: "FPC: <description> @ N/*/*" → FPC N | |
| - librenms_name: "^FPC: .+ @ (\\d+)/\\*/\\*$" | |
| librenms_class: "container" | |
| netbox_bay_name: "FPC \\1" | |
| is_regex: true | |
| description: "Juniper FPC container description → FPC N" | |
| # Juniper transceivers: "<type> @ slot/pic/port" description → Transceiver slot/pic/port | |
| - librenms_name: "^.+ @ (\\d+/\\d+/\\d+)$" | |
| librenms_class: "port" | |
| netbox_bay_name: "Transceiver \\1" | |
| is_regex: true | |
| description: "Juniper transceiver description → Transceiver slot/pic/port" | |
| # Juniper fan trays: "Fan Tray N" → "Fan N" (ACX7100, etc.) | |
| # Runs after exact match, so "Fan Tray 0" → "Fan Tray" (ACX7024) still works | |
| - librenms_name: "^Fan Tray (\\d+)$" | |
| librenms_class: "fan" | |
| netbox_bay_name: "Fan \\1" | |
| is_regex: true | |
| description: "Juniper Fan Tray N → Fan N (ACX7100 etc.)" | |
| - librenms_name: "^PSM (\\d+)$" | |
| librenms_class: "powerSupply" | |
| manufacturer: "Juniper" | |
| netbox_bay_name: "PEM \\1" | |
| is_regex: true | |
| description: "Juniper chassis PSM N → PEM N" | |
| # Juniper FPC container: "FPC: <description> @ N/*/*" → FPC N | |
| - librenms_name: "^FPC: .+ @ (\\d+)/\\*/\\*$" | |
| librenms_class: "container" | |
| manufacturer: "Juniper" | |
| netbox_bay_name: "FPC \\1" | |
| is_regex: true | |
| description: "Juniper FPC container description → FPC N" | |
| # Juniper transceivers: "<type> @ slot/pic/port" description → Transceiver slot/pic/port | |
| - librenms_name: "^.+ @ (\\d+/\\d+/\\d+)$" | |
| librenms_class: "port" | |
| manufacturer: "Juniper" | |
| netbox_bay_name: "Transceiver \\1" | |
| is_regex: true | |
| description: "Juniper transceiver description → Transceiver slot/pic/port" | |
| # Juniper fan trays: "Fan Tray N" → "Fan N" (ACX7100, etc.) | |
| # Runs after exact match, so "Fan Tray 0" → "Fan Tray" (ACX7024) still works | |
| - librenms_name: "^Fan Tray (\\d+)$" | |
| librenms_class: "fan" | |
| manufacturer: "Juniper" | |
| netbox_bay_name: "Fan \\1" | |
| is_regex: true | |
| description: "Juniper Fan Tray N → Fan N (ACX7100 etc.)" |
Comment on lines
+50
to
+60
| manufacturer_id = django_filters.ModelChoiceFilter( | ||
| field_name="manufacturer", | ||
| queryset=Manufacturer.objects.all(), | ||
| label="Manufacturer", | ||
| ) | ||
|
|
||
| class Meta: | ||
| """Meta options for ModuleTypeMappingFilterSet.""" | ||
|
|
||
| model = ModuleTypeMapping | ||
| fields = ["librenms_model", "description", "manufacturer_id"] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In django-filter, can FilterSet.Meta.fieldsinclude an alias filter name likemanufacturer_id = ModelChoiceFilter(field_name="manufacturer"), or must it only include real model field names?
💡 Result:
In django-filter, FilterSet.Meta.fields must only include real model field names (or relationship paths) [1][2]. You should not include declared filter names (aliases) in Meta.fields [1][2]. If you declare a custom filter in your FilterSet, such as: class ProductFilter(django_filters.FilterSet): manufacturer_id = django_filters.ModelChoiceFilter(field_name="manufacturer") class Meta: model = Product fields = ["price"] # Only include real model fields here You do not need to (and must not) include manufacturer_id in the Meta.fields list [1][2]. Including a declared filter alias in Meta.fields will raise a TypeError [3][1]. The primary purpose of Meta.fields is to tell django-filter which model fields to automatically generate filters for; since you have already explicitly defined manufacturer_id, it is already "present" and does not need to be registered in Meta.fields [1][2].
Citations:
- 1: https://django-filter.readthedocs.io/en/stable/ref/filterset.html
- 2: https://django-filter.readthedocs.io/en/latest/ref/filterset.html
- 3: Filter field rename in FilterSet class no longer works in 2.3 carltongibson/django-filter#1232
Fix FilterSet.Meta.fields to use model field names, not alias filters (manufacturer_id)
manufacturer_id is declared as a filter alias (ModelChoiceFilter(field_name="manufacturer")), but ModuleTypeMappingFilterSet.Meta.fields includes "manufacturer_id". django-filter’s Meta.fields must list real model field names (or relationship paths); including a declared filter alias can raise TypeError at class-build/import time.
Suggested fix
class ModuleTypeMappingFilterSet(django_filters.FilterSet):
@@
class Meta:
"""Meta options for ModuleTypeMappingFilterSet."""
model = ModuleTypeMapping
- fields = ["librenms_model", "description", "manufacturer_id"]
+ fields = ["librenms_model", "description", "manufacturer"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| manufacturer_id = django_filters.ModelChoiceFilter( | |
| field_name="manufacturer", | |
| queryset=Manufacturer.objects.all(), | |
| label="Manufacturer", | |
| ) | |
| class Meta: | |
| """Meta options for ModuleTypeMappingFilterSet.""" | |
| model = ModuleTypeMapping | |
| fields = ["librenms_model", "description", "manufacturer_id"] | |
| manufacturer_id = django_filters.ModelChoiceFilter( | |
| field_name="manufacturer", | |
| queryset=Manufacturer.objects.all(), | |
| label="Manufacturer", | |
| ) | |
| class Meta: | |
| """Meta options for ModuleTypeMappingFilterSet.""" | |
| model = ModuleTypeMapping | |
| fields = ["librenms_model", "description", "manufacturer"] |
Comment on lines
+252
to
+311
| function showErrorToast(source) { | ||
| if (!source) { | ||
| return; | ||
| } | ||
| const isXhr = typeof source === 'object' && 'responseText' in source; | ||
| const container = document.getElementById('django-messages'); | ||
| if (!container || typeof bootstrap === 'undefined' || !bootstrap.Toast) { | ||
| if (isXhr) { | ||
| console.error('LibreNMS plugin: server error', source.status, source.responseText); | ||
| } else { | ||
| console.error('LibreNMS plugin: server error', source); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Truncate very long error bodies (some Django validation traces are huge). | ||
| let raw; | ||
| if (isXhr) { | ||
| raw = (source.responseText || '').trim(); | ||
| if (!raw) { | ||
| raw = `Request failed with status ${source.status}`; | ||
| } | ||
| } else { | ||
| raw = String(source).trim() || 'Server error'; | ||
| } | ||
| const MAX = 600; | ||
| if (raw.length > MAX) { | ||
| raw = raw.slice(0, MAX) + '\u2026'; | ||
| } | ||
|
|
||
| const toast = document.createElement('div'); | ||
| toast.className = 'toast toast-dark border-0 shadow-sm'; | ||
| toast.setAttribute('role', 'alert'); | ||
| toast.setAttribute('aria-live', 'assertive'); | ||
| toast.setAttribute('aria-atomic', 'true'); | ||
| toast.setAttribute('data-bs-delay', '12000'); | ||
|
|
||
| const header = document.createElement('div'); | ||
| header.className = 'toast-header text-bg-danger'; | ||
| const icon = document.createElement('i'); | ||
| icon.className = 'mdi mdi-alert-circle me-1'; | ||
| header.appendChild(icon); | ||
| header.appendChild(document.createTextNode(' Error')); | ||
| const closeBtn = document.createElement('button'); | ||
| closeBtn.type = 'button'; | ||
| closeBtn.className = 'btn-close me-0 m-auto'; | ||
| closeBtn.setAttribute('data-bs-dismiss', 'toast'); | ||
| closeBtn.setAttribute('aria-label', 'Close'); | ||
| header.appendChild(closeBtn); | ||
|
|
||
| const body = document.createElement('div'); | ||
| body.className = 'toast-body'; | ||
| // Use textContent to keep server response untrusted-safe (no HTML injection). | ||
| body.textContent = raw; | ||
|
|
||
| toast.appendChild(header); | ||
| toast.appendChild(body); | ||
| container.appendChild(toast); | ||
| bootstrap.Toast.getOrCreateInstance(toast).show(); | ||
| } |
There was a problem hiding this comment.
Don't surface raw server error bodies in the toast.
showErrorToast() renders source.responseText straight into the UI. On a real 4xx/5xx that can be a full Django error page or exception text, which leaks server-side detail back to operators. Please keep the raw body in console.error() and show a generic toast message instead.
Suggested change
function showErrorToast(source) {
if (!source) {
return;
}
const isXhr = typeof source === 'object' && 'responseText' in source;
@@
- let raw;
+ let raw = 'Request failed. Please try again or contact an administrator.';
if (isXhr) {
- raw = (source.responseText || '').trim();
- if (!raw) {
- raw = `Request failed with status ${source.status}`;
- }
+ console.error('LibreNMS plugin: server error', source.status, source.responseText);
} else {
- raw = String(source).trim() || 'Server error';
+ raw = String(source).trim() || raw;
}
- const MAX = 600;
- if (raw.length > MAX) {
- raw = raw.slice(0, MAX) + '\u2026';
- }
@@
const body = document.createElement('div');
body.className = 'toast-body';
- // Use textContent to keep server response untrusted-safe (no HTML injection).
body.textContent = raw;
Comment on lines
+1956
to
+1965
| function updateHtmxModalLabel() { | ||
| const htmxModal = document.getElementById('htmx-modal'); | ||
| if (htmxModal) { | ||
| htmxModal.addEventListener('htmx:afterSettle', function () { | ||
| const header = htmxModal.querySelector('.modal-title, .modal-header h5, .modal-header h4'); | ||
| const label = document.getElementById('htmx-modal-label'); | ||
| if (header && label) { | ||
| label.textContent = header.textContent.trim(); | ||
| } | ||
| }); | ||
| if (!htmxModal) return; | ||
| const modalBody = htmxModal.querySelector('#htmx-modal-body') || htmxModal; | ||
| const header = modalBody.querySelector('.modal-title, .modal-header h5, .modal-header h4'); | ||
| const labelId = htmxModal.getAttribute('aria-labelledby'); | ||
| const label = (labelId && document.getElementById(labelId)) || document.getElementById('htmx-modal-label'); | ||
| if (header && label && header !== label) { | ||
| label.textContent = header.textContent.trim(); | ||
| } |
There was a problem hiding this comment.
Read the modal title from the wrapper, not #htmx-modal-body.
When a fragment replaces the full modal content with sibling header/body nodes, the current lookup searches inside #htmx-modal-body and never sees the new header. The shared modal then keeps the stale loading label for screen readers.
Suggested fix
function updateHtmxModalLabel() {
const htmxModal = document.getElementById('htmx-modal');
if (!htmxModal) return;
- const modalBody = htmxModal.querySelector('`#htmx-modal-body`') || htmxModal;
- const header = modalBody.querySelector('.modal-title, .modal-header h5, .modal-header h4');
+ const header = htmxModal.querySelector('.modal-header .modal-title, .modal-header h5, .modal-header h4');
const labelId = htmxModal.getAttribute('aria-labelledby');
const label = (labelId && document.getElementById(labelId)) || document.getElementById('htmx-modal-label');
if (header && label && header !== label) {
label.textContent = header.textContent.trim();
}
Comment on lines
+793
to
+833
| if record.get("installed_module_id") and isinstance(getattr(self.device, "virtual_chassis_id", None), int): | ||
| from dcim.models import Module | ||
| from django.db import DatabaseError | ||
|
|
||
| from netbox_librenms_plugin.utils import detect_vc_normalization_noop | ||
|
|
||
| installed_module = None | ||
| try: | ||
| installed_module = Module.objects.select_related( | ||
| "module_type", | ||
| "module_type__manufacturer", | ||
| "module_bay", | ||
| "device", | ||
| "device__device_type", | ||
| "device__virtual_chassis", | ||
| ).get(pk=record["installed_module_id"]) | ||
| except (Module.DoesNotExist, DatabaseError, RuntimeError): | ||
| # RuntimeError: pytest's "Database access not allowed" in unit-test | ||
| # contexts that supply self.device as a MagicMock with a real-looking | ||
| # virtual_chassis_id. Production rows wouldn't reach here. | ||
| installed_module = None | ||
|
|
||
| if installed_module is not None and detect_vc_normalization_noop(installed_module.device, installed_module): | ||
| report_url = reverse( | ||
| "plugins:netbox_librenms_plugin:vc_normalization_report", | ||
| kwargs={"pk": self.device.pk}, | ||
| ) | ||
| buttons.append( | ||
| format_html( | ||
| '<button type="button" class="btn btn-sm btn-outline-secondary ms-1 vc-report-btn"' | ||
| ' data-report-url="{}" data-module-id="{}" data-selected-device-id="{}"' | ||
| ' title="Report VC naming-convention issue — opens a copyable diagnostic' | ||
| " for a GitHub issue" | ||
| '">' | ||
| '<i class="mdi mdi-bug-outline"></i> Report VC issue' | ||
| "</button>", | ||
| report_url, | ||
| record["installed_module_id"], | ||
| record.get("selected_device_id") or self.device.pk, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Move the VC normalisation lookup out of render_actions().
This block does a Module.objects.select_related(...).get(...) per installed VC row while the table is rendering. On a populated chassis that becomes an N+1 query pattern, and format_module_data() will hit the same path again for row refreshes. Please compute the vc_normalization_noop flag upstream when building the records, or batch-load the referenced modules once on the table instance.
Based on learnings: Sync pipelines should follow the standard flow: fetch LibreNMS data (librenms_api.py), cache it (CacheMixin), build comparison tables (tables/), and render HTMX fragments (templates/netbox_librenms_plugin/htmx/).
Comment on lines
+112
to
+117
| var token = (document.querySelector('[name=csrfmiddlewaretoken]') || {}).value || ''; | ||
| fetch(url, { | ||
| method: 'POST', | ||
| credentials: 'same-origin', | ||
| headers: { 'Content-Type': 'application/json', 'X-CSRFToken': token }, | ||
| body: JSON.stringify({ key: 'set_primary_ip', value: cb.checked }) |
There was a problem hiding this comment.
Add CSRF cookie fallback for the preference POST.
The request currently falls back to an empty CSRF token; when the hidden input is unavailable, preference saves can fail with 403.
💡 Proposed fix
- var token = (document.querySelector('[name=csrfmiddlewaretoken]') || {}).value || '';
+ var tokenInput = document.querySelector('[name=csrfmiddlewaretoken]');
+ var token = tokenInput && tokenInput.value
+ ? tokenInput.value
+ : (typeof getCookie === 'function' ? getCookie('csrftoken') : '');As per coding guidelines netbox_librenms_plugin/static/**/*.js: all fetch() calls must use hidden-input CSRF with getCookie('csrftoken') as fallback.
5 tasks
Owner
Author
|
CodeRabbit review comments have been noted but are non blocking for this feature release PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Release 0.4.7 — merge into master for PyPI release.
Closes #217
Closes #279
Closes #284
Closes #280
Motivation / Problem
This release adds Module / Inventory Sync and its mapping rules (#261, #296), a Set Primary IP action (#303), and a module interface name prediction hook (#298), along with several bug fixes, security hardening, and documentation updates.
Scope of Change
Changes
DeviceTypeMapping,ModuleTypeMapping,ModuleBayMapping(regex + manufacturer scoping),NormalizationRule,InventoryIgnoreRule,CarrierAutoInstallRulewith bulk YAML import/export (feat: inventory module #261)port_idto interfacelibrenms_id, install modules with interface templates, adopt existing standalone interfaces (Follow up module sync interface matching and update logic #296)predict_module_interface_namessignal + VC normalization report (Feat/module interface name predict hook #298)VirtualMachine.clusteraccess on import page (fix(import): guard VirtualMachine.cluster access (#284) #287, closes 'NoneType' object has no attribute 'name' #284)GITHUB_TOKENpermissions (ci: restrict GITHUB_TOKEN permissions in workflows #304), open-redirect guard localisation (fix: localise open-redirect guard to redirect sink #305), avoid exception-detail exposure in IP sync responses (fix: avoid exposing exception details in IP address sync responses #306)How Was This Tested?
Each included PR carried its own unit tests and/or manual testing. The module sync feature added extensive unit tests; bug-fix PRs added regression tests; security PRs resolve CodeQL alerts. The version bump itself is changelog/metadata only.
Risk Assessment
Backwards Compatibility