Conversation
Test/ci workflow
Feature/snmpv1-support
- Remove trailing whitespace from templates, JS, docs, and config files - Add missing newlines at end of files - Applied via pre-commit hooks (first run after installation)
…ess control - Add LibreNMSPermissionMixin and NetBoxObjectPermissionMixin to all views - Enforce view/change permissions on LibreNMSSettings model - Add NetBox object permission checks on sync POST handlers - Auto-fallback to synchronous mode for non-superuser background jobs - Update navigation, API, templates, and import UI for permission gating - Add user-facing permissions documentation and mkdocs nav entry - Add test_permissions.py and update test_background_jobs.py
Updated lint and format workflow to updated action v4 and improved output formatting.
…ructions chore: update copilot instructions with permission and background job patterns
Validate Referer header with url_has_allowed_host_and_scheme before using it for HX-Redirect or redirect targets. Falls back to request.path when the referrer is external or missing.
Add dcim.add_virtualchassis to bulk import permission checks. Explicitly validate object_type in sync views, raising Http404 for invalid values instead of silently defaulting to VM permissions.
Replace inline HTML alert response with messages.error and HX-Redirect to match the permission denial pattern used elsewhere.
feat: add two-tier permission system with plugin and object-level access control
Add missing docstrings across the codebase for improved code documentation: - Model classes and Meta inner classes - Table classes, Meta classes, and render/configure methods - View handler methods (get, post) and helper methods - API serializers and viewsets - FilterSet classes and form Meta classes All changes are documentation-only with no functional impact.
Show informative empty state cards when no data is loaded for cable, interface, and IP address sync tabs. Cards display a sync-off icon and prompt users to click the refresh button.
- Fix bug in DeviceInterfaceTableView.get_redirect_url using vm_interface_sync instead of device_interface_sync - Remove incorrect Returns section from create_cable docstring - Clarify get_queryset docstring in import view - Fix verify_cable_creation_requirements docstring (checks device ID too) - Remove non-existent 'role' from VMStatusFilterSet.search docstring
docs: add docstrings to models, tables, views, forms, and API modules
Add comprehensive proxy configuration support for corporate networks with MITM proxies (Zscaler, BlueCoat, etc.). Container environment: - Pass proxy env vars (HTTP_PROXY, HTTPS_PROXY, NO_PROXY) and CA bundle vars (REQUESTS_CA_BUNDLE, SSL_CERT_FILE, CURL_CA_BUNDLE) through devcontainer.json and docker-compose.yml - Add proxy env var examples to .env.example Setup script: - Auto-configure apt proxy when HTTP_PROXY/HTTPS_PROXY are set - Install custom CA certificates from workspace ca-bundle.crt into the system trust store (split into individual certs for update-ca-certificates compatibility) - Configure pip global cert for isolated virtualenvs (e.g. pre-commit) - Support ALLOW_GIT_SSL_DISABLE opt-in for git SSL verification override (prefer CA bundle over disabling SSL) - Extract detect_plugin_workspace() helper for early workspace detection needed by CA cert installation Documentation: - Add detailed proxy configuration guide to README covering Docker client proxy, container runtime proxy, CA certificate setup, and common troubleshooting Also adds ca-bundle.crt and *.pem to .gitignore to keep proxy certificates out of version control.
Extract and consolidate devcontainer setup scripts for better maintainability and robustness. Setup script refactoring: - Extract detect_plugin_workspace() helper function for reusable workspace directory detection with pyproject.toml lookup - Replace inline workspace detection with function call - Replace devcontainer features (git, github-cli) with manual installation in setup.sh for better proxy/network compatibility - Add idempotent .bashrc guard with sentinel comment to prevent duplicate alias entries on setup.sh re-runs - Move all alias definitions from inline .bashrc block to sourcing the canonical load-aliases.sh file - Add git safe.directory config for workspace - Install git via apt-get alongside net-tools Alias improvements: - Move dev-help alias to load-aliases.sh with expanded command reference (rq-stats, rq-jobs, rq-recent) - Update help output echo message to include new commands - Source aliases in welcome.sh for postAttach terminal sessions Other fixes: - Fix typo in plugin-config.py.example (exampel -> example) - Update comment text (three -> example) - Add LibreNMS server configuration section to README - Add netbox-restart tip to welcome.sh - Use 'install -d' instead of 'mkdir -p -m' for keyrings directory
Previously, when a specific (non-default) server_key was requested but not found in the plugin configuration, the API would silently fall back to the first available server. This could cause operations to run against the wrong LibreNMS instance without any indication. Now, if a specific server_key is requested and not found: - Raises KeyError with the missing key and list of available servers - Only the 'default' key falls back to the first configured server (with an info log) This prevents silent misconfiguration when users reference a server that doesn't exist in their configuration.
Cover the two new init code paths introduced in this PR: - KeyError raised when a specific non-default server_key is not found - Graceful fallback to first configured server when 'default' key is missing
…yerror fix: raise KeyError for missing non-default server keys in LibreNMSAPI
…improvements - Escape LibreNMS API responses with django.utils.html.escape() to prevent XSS in settings_views.py (version, database, PHP version, error messages) - Add full_clean()/ValidationError/IntegrityError handling on all device.save() calls in device_fields.py (serial, device type, platform, VC serial assignment) - Use SAFE_METHODS instead of == 'GET' for REST API permission check - Use safe .get() for ifSpeed/ifType instead of direct dict access - Add exception handling for Device.objects.get(id=selected_device_id) - Fix DeleteNetBoxInterfacesView to use require_all_permissions_json (consolidate separate permission checks into single call) - Move interface_name variable initialization before delete to fix potential NameError in error path - Add str() cast on ifAdminStatus to handle non-string values safely - Fix create_cable() to return bool for success/failure tracking - Fix found_in_librenms flag in base sync view (was False when device had librenms_id but hostname mismatched) - Fix permission string formatting in permissions.md docs
Per review feedback, this is a logic change (not hardening) and should be handled in a separate PR for issue bonzo81#228.
fix(js): handle VLAN group persistence errors in librenms_sync refactor(tables): enhance VLAN group handling in interface table refactor(utils): add group-aware CSS class functions for VLANs fix(views): improve VLAN group verification logic in device sync fix(views): optimize VLAN sync logic to prevent redundant saves
feat: VLAN sync — Device VLAN sync with VLAN group selection and Interface sync VLAN data
…zo81#228) When a device has a librenms_id custom field set, trust it as the authoritative link even if the hostname/IP doesn't match. This fixes the issue where the sync page was completely blocked when the NetBox device name differed from the LibreNMS sysName. Changes: - Always set found_in_librenms=True when librenms_id resolves to a valid device, regardless of hostname/IP match - Strict full-name comparison (FQDN to FQDN, short to short) for mismatch detection — no short-to-FQDN fallback - Show an informational warning banner above the sync tabs when a name mismatch is detected, instead of blocking the entire page - Add Name and Serial rows to LibreNMS Status card with mismatch indicators - Remove the old blocking "Device Mismatch" alert Fixes bonzo81#228
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix: sync view mismatch
fix: security hardening — XSS escaping, input validation, and safety …
…factor refactor: devcontainer script improvements and alias consolidation
…e name field Add user preference system using NetBox's built-in user config: - New helpers: _get_user_pref(), _save_user_pref(), save_import_toggle_prefs() - use_sysname and strip_domain toggles persist per-user on change - interface_name_field persists per-user when explicitly selected - Preferences override server-level defaults, falling back when unset - New SaveUserPrefView endpoint for JS-driven preference updates - Settings page syncs user prefs when admin changes import defaults - JS handlers in librenms_import.html and librenms_sync.js persist toggle/radio state via fetch to save-user-pref endpoint - Tests for user pref loading and persistence in get_interface_name_field()
- Rename _get_user_pref/_save_user_pref to drop leading underscore (used across modules, not private) - Remove redundant POST save path (save_import_toggle_prefs) from BulkImportConfirmView; keep JS-only persistence via SaveUserPrefView - Extract inline savePref JS from librenms_import.html into librenms_import.js with data-save-pref-url attribute - Add LibreNMSPermissionMixin to SaveUserPrefView - Improve error handling in settings_views.py (catch specific exceptions, log unexpected ones with traceback) - Add tests for SaveUserPrefView (valid prefs, invalid key, invalid JSON, permission mixin inheritance) - Add help text on settings page explaining defaults vs user preferences - Document user preference sync behavior in import_settings.md
feat: per-user preference persistence for import toggles and interfac…
📝 WalkthroughWalkthroughThis PR introduces comprehensive VLAN synchronization between LibreNMS and NetBox, establishes a two-tier permission system for plugin access control, refactors devcontainer configuration for proxy support, and adds extensive documentation and test coverage across the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 54
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
.pre-commit-config.yaml (2)
3-3:⚠️ Potential issue | 🟡 MinorUpdate ruff-pre-commit to the latest version.
The version
v0.14.13exists and is valid, but the inline comment states "Use the latest version." The current latest release isv0.15.2(released February 19, 2026). Update the revision to match the stated intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml at line 3, Update the ruff-pre-commit rev to the actual latest release by replacing the current "rev: v0.14.13" in the ruff-pre-commit entry with "rev: v0.15.2" (reference the rev field for ruff-pre-commit); after updating the rev, run the pre-commit autoupdate or validate the YAML to ensure the change is applied and the comment still accurately reflects the intent.
3-9:⚠️ Potential issue | 🟡 MinorUpdate the
ruffhook ID toruff-check.The
ruffhook ID is deprecated. Currentruff-pre-commitdocumentation usesruff-checkas the linter hook ID. Update line 6 fromid: rufftoid: ruff-check.Note: The version
v0.14.13is a valid release and does not need to be changed. If you wish to upgrade to a newer version for other reasons,v0.15.2is the current latest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml around lines 3 - 9, Update the pre-commit hook id for the linter from "ruff" to "ruff-check": locate the hooks block where the linter is declared (the entry with id: ruff) and replace that id value with id: ruff-check so the hook matches the current ruff-pre-commit naming; keep the same args and version (rev: v0.14.13) unchanged.netbox_librenms_plugin/templates/netbox_librenms_plugin/settings.html (1)
296-336: 🧹 Nitpick | 🔵 TrivialInconsistent null-safety on DOM lookups in the save-button script.
useSysnameCheckboxandstripDomainCheckboxare null-guarded (ternary on lines 306–307, 320–321), butserverSelect,vcPatternInput,saveServerBtn, andsaveImportBtnare accessed unconditionally. If any of those elements are missing (e.g., due to future conditional rendering or a form being split out), the script will throw at lines 304–305 and 312/323 before any change listener is registered.♻️ Suggested fix — guard all four non-optional lookups
- const serverSelect = document.getElementById('id_selected_server'); - const vcPatternInput = document.getElementById('id_vc_member_name_pattern'); - const saveServerBtn = document.getElementById('save-server-btn'); - const saveImportBtn = document.getElementById('save-import-btn'); - const useSysnameCheckbox = document.getElementById('id_use_sysname_default'); - const stripDomainCheckbox = document.getElementById('id_strip_domain_default'); - - // Store the initial values to detect changes - const initialServerValue = serverSelect.value; - const initialVcPatternValue = vcPatternInput.value; + const serverSelect = document.getElementById('id_selected_server'); + const vcPatternInput = document.getElementById('id_vc_member_name_pattern'); + const saveServerBtn = document.getElementById('save-server-btn'); + const saveImportBtn = document.getElementById('save-import-btn'); + const useSysnameCheckbox = document.getElementById('id_use_sysname_default'); + const stripDomainCheckbox = document.getElementById('id_strip_domain_default'); + + if (!serverSelect || !vcPatternInput || !saveServerBtn || !saveImportBtn) return; + + // Store the initial values to detect changes + const initialServerValue = serverSelect.value; + const initialVcPatternValue = vcPatternInput.value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/templates/netbox_librenms_plugin/settings.html` around lines 296 - 336, The script currently assumes elements with IDs id_selected_server, id_vc_member_name_pattern, save-server-btn and save-import-btn always exist; add null-safety checks for serverSelect, vcPatternInput, saveServerBtn and saveImportBtn (similar to existing guards for useSysnameCheckbox/stripDomainCheckbox) and bail out early if any required element is missing or skip wiring that section; update updateServerSaveButton and updateImportSaveButton to handle possible nulls and only attach event listeners (serverSelect.addEventListener, vcPatternInput.addEventListener, etc.) when the corresponding DOM element is non-null.netbox_librenms_plugin/api/views.py (1)
72-72:⚠️ Potential issue | 🟠 MajorAdd explicit
rq >= 1.8.0constraint or implement defensive attribute check.The
is_stoppedattribute was introduced in RQ 1.8.0. While modern NetBox versions (v2.7+) transitively requirerq >= 1.14viadjango-rq, this plugin does not explicitly declare therqversion requirement. In older NetBox installations or custom environments, accessingis_stoppedon line 72 will raiseAttributeError, which gets silently caught by the broadexcept Exceptionhandler at line 82, incorrectly classifying the error as "job not found" rather than exposing the real problem.Either add
rq >= 1.8.0topyproject.tomldependencies, or use a defensive check likegetattr(rq_job, 'is_stopped', False)to handle olderrqversions gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/api/views.py` at line 72, The code accesses rq_job.is_stopped which exists only in RQ >= 1.8.0; either declare the runtime dependency rq >= 1.8.0 in pyproject.toml or make the attribute access defensive to avoid AttributeError being swallowed by the broad except that follows. Replace direct checks of rq_job.is_stopped (and similarly rq_job.is_failed) with a safe lookup such as using getattr(rq_job, 'is_stopped', False) and getattr(rq_job, 'is_failed', False) in the view handling (where rq_job is inspected) so older rq versions treat missing attributes as False and surface real errors correctly.netbox_librenms_plugin/views/sync/locations.py (2)
46-53:⚠️ Potential issue | 🟠 MajorDead code — the
"q"manual filter at lines 49-51 is unreachable.When
self.request.GETis truthy (line 46), the method returns early at line 47-48; when it is falsy,"q"cannot be in it. The manual"q"filter at lines 49-51 is never executed. If text search is still needed, it should be wired throughSiteLocationFilterSet, not duplicated here.🗑️ Proposed fix — remove dead code
sync_data = [self.create_sync_data(site, librenms_locations) for site in netbox_sites] if self.request.GET and self.filterset: return self.filterset(self.request.GET, queryset=sync_data).qs - if "q" in self.request.GET: - query = self.request.GET.get("q", "").lower() - sync_data = [item for item in sync_data if query in item.netbox_site.name.lower()] - return sync_data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/locations.py` around lines 46 - 53, The manual "q" filter block is dead because the method returns whenever self.request.GET is truthy; remove the unreachable lines that check if "q" in self.request.GET and the subsequent list comprehension filtering sync_data; instead ensure text-search behavior is implemented in the existing filterset (e.g., SiteLocationFilterSet) or wired through self.filterset so all GET-based filtering happens in one place; update or add a filter field in the filterset if needed and delete the redundant code referencing query, item.netbox_site.name, and the manual sync_data reassignment.
119-130:⚠️ Potential issue | 🟠 MajorMissing
Noneguard before callingbuild_location_dataincreate_librenms_location.
build_location_datacallsstr(site.latitude)/str(site.longitude)without a None check. If either coordinate is unset, the string"None"is sent to the LibreNMS API as the coordinate value.update_librenms_location(line 134-139) correctly guards against this;create_librenms_locationshould do the same.🛡️ Proposed fix
def create_librenms_location(self, request, site): """Create a new location in LibreNMS from the given site.""" + if site.latitude is None or site.longitude is None: + messages.warning( + request, + f"Latitude and/or longitude is missing. Cannot create location '{site.name}' in LibreNMS.", + ) + return redirect("plugins:netbox_librenms_plugin:site_location_sync") location_data = self.build_location_data(site)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/locations.py` around lines 119 - 130, create_librenms_location currently calls build_location_data without guarding site.latitude/site.longitude, which causes "None" strings to be sent; mirror update_librenms_location's behavior by adding a None-check before building the payload: ensure site.latitude and site.longitude are not None (or sanitize them) before calling build_location_data, and if either is None, handle appropriately (e.g., skip creating the location or build a payload without coordinates) so that build_location_data never receives None values for site.latitude/site.longitude.netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html (1)
289-296:⚠️ Potential issue | 🟡 MinorHTML tag mismatch:
<td>opened but closed with</th>.Line 291 opens a
<td>element, but Line 296 closes it with</th>. While this appears to be in existing code (not marked as changed), it's adjacent to changes and could cause rendering issues in some browsers.Proposed fix
- </th> + </td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html` around lines 289 - 296, In the "Serial Number" row of the librenms_sync_base.html template there is an HTML tag mismatch: a <td> is opened but closed with </th>; update the closing tag to </td> for the table cell that contains "Serial Number" and the conditional badge (the Serial Number Row block) so the <td>...{% if object.virtual_chassis and vc_inventory_serials %}...{% endif %}</td> is correctly balanced.netbox_librenms_plugin/views/sync/cables.py (1)
115-124:⚠️ Potential issue | 🟡 MinorVerify:
transaction.atomic()will roll back all cables if any creation raises an exception, butcreate_cableswallows exceptions.
create_cable(Line 53) catchesExceptionand returnsFalseinstead of re-raising. This means thetransaction.atomic()block at Line 119 will never actually roll back on cable creation failure — failed cables just return"invalid"status while previously created cables persist. If all-or-nothing semantics are intended,create_cableshould re-raise. If partial success is acceptable (as the current UX messaging suggests), consider removingtransaction.atomic()to avoid confusion about the intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/cables.py` around lines 115 - 124, The transaction.atomic() in process_interface_sync and the swallowing of exceptions in create_cable conflict: either make cable creation truly transactional by letting create_cable re-raise exceptions (remove the broad try/except or re-raise the caught Exception in create_cable) so transaction.atomic() will roll back on failure, or accept partial success by removing the transaction.atomic() block in process_interface_sync so successes persist while failures are reported; update the logic in create_cable (the function referenced by process_single_interface/process_interface_sync) or remove the atomic context accordingly to reflect the intended all‑or‑nothing vs partial success behavior..devcontainer/scripts/setup.sh (1)
189-226:⚠️ Potential issue | 🟠 MajorHardcoded paths in injected Python config ignore
$PLUGIN_WS_DIR.Lines 189, 203, and 216 hardcode
/workspaces/netbox-librenms-plugin/for the plugin-config, extra-configuration, and codespaces-configuration paths. However,detect_plugin_workspacemay resolve a different workspace directory (e.g., via thefindfallback at Line 22). If the workspace is at a different path, these config files won't be found.Consider using
$PLUGIN_WS_DIRinstead:Proposed fix (showing one of three paths)
- echo "_pc_path = '/workspaces/netbox-librenms-plugin/.devcontainer/config/plugin-config.py'"; + echo "_pc_path = '$PLUGIN_WS_DIR/.devcontainer/config/plugin-config.py'";Apply the same pattern for
_xc_path(Line 203) and_cs_path(Line 216).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/scripts/setup.sh around lines 189 - 226, The script hardcodes '/workspaces/netbox-librenms-plugin/' when building _pc_path, _xc_path, and _cs_path so injected Python configs can be missed if detect_plugin_workspace found a different PLUGIN_WS_DIR; change those three constructions to use the PLUGIN_WS_DIR variable (the same variable set by detect_plugin_workspace) instead of the literal path so _pc_path, _xc_path and _cs_path point to "${PLUGIN_WS_DIR}/.devcontainer/config/..." (apply the same substitution for plugin-config, extra-configuration and codespaces-configuration).netbox_librenms_plugin/import_utils.py (1)
1303-1308:⚠️ Potential issue | 🟠 MajorBulk import cancellation checks should use RQ job status, not database polling
These loops poll
job.job.statusfrom the database viarefresh_from_db(), which can lag behind actual RQ stop/fail state and miss cancellation requests. This code is unreliable because the NetBox worker doesn't always update the database when jobs stop before they begin processing.The same file already uses RQ-aware cancellation detection elsewhere (lines 2162–2168 pattern); apply that approach here:
Required change
Replace database polling with RQ job status checks:
- job.job.refresh_from_db() - job_status = job.job.status - status_value = job_status.value if hasattr(job_status, "value") else job_status - if status_value in (JobStatusChoices.STATUS_FAILED, "failed", "errored"): + from django_rq import get_queue + from rq.job import Job as RQJob + queue = get_queue("default") + rq_job = RQJob.fetch(str(job.job.job_id), connection=queue.connection) + if rq_job.is_failed or rq_job.is_stopped:Apply the same fix to line 1667–1671.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils.py` around lines 1303 - 1308, The code currently polls the DB via job.job.refresh_from_db() and job.job.status (and compares to JobStatusChoices) which can miss RQ-level cancellations; replace that DB polling with an RQ-aware status check by calling the RQ Job API on job.job (e.g., use job.job.get_status() or the RQ status properties) and compare the returned RQ status string to RQ terminal states like "failed", "stopped" (and "canceled"/"cancelled" if applicable), then branch on that instead of the DB-derived status_value; apply the same replacement for the second occurrence of this pattern near the other bulk-import loop so both checks use RQ job.get_status()/RQ status comparisons rather than refresh_from_db()/JobStatusChoices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/devcontainer.json:
- Around line 44-46: The devcontainer currently unconditionally injects
REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE using ${localEnv:...}, which can pass
empty strings and break Python HTTPS requests; instead, remove these two from
the containerEnv and implement a conditional export in setup.sh (or environment
bootstrap) that only exports REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE when the
corresponding host vars are non-empty (check with -n or equivalent), leaving
SSL_CERT_FILE untouched since requests ignores it; this ensures
REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE are only set inside the container when
valid values exist.
In @.devcontainer/docker-compose.yml:
- Around line 32-34: The compose file currently exports REQUESTS_CA_BUNDLE,
SSL_CERT_FILE, and CURL_CA_BUNDLE with empty-string defaults using ${VAR:-},
which forces empty values into the container and breaks TLS; change those
environment entries to use bare variable references (e.g.,
${REQUESTS_CA_BUNDLE}, ${SSL_CERT_FILE}, ${CURL_CA_BUNDLE}) so Docker Compose
will inherit them from the host if set and omit them entirely if unset,
preserving normal requests/certifi behavior; update the lines referencing
REQUESTS_CA_BUNDLE, SSL_CERT_FILE, and CURL_CA_BUNDLE in
.devcontainer/docker-compose.yml accordingly.
In @.github/copilot-instructions.md:
- Around line 38-70: Add missing blank lines around Markdown headings and the
fenced code block: insert a blank line before and after the "Permission System",
"Plugin-Level Permissions", "Object-Level Permissions", "Permission Helpers for
Background Jobs", and "API & Navigation Permissions" headings, and ensure there
is a blank line both before and after the fenced code block containing the
required_object_permissions example (the block starting with ```python and the
VLAN example). This will satisfy MD022/MD031 by spacing headings and fenced code
blocks correctly.
- Around line 12-18: The description incorrectly calls this a "three-layer
structure" but then enumerates four items; update the header text to accurately
reflect the structure (e.g., change "Three-layer structure" to "Four-layer
structure" or "Three-layer plus shared mixins") and ensure the surrounding text
and examples (references to views/base/, views/object_sync/, views/sync/, and
views/mixins.py as well as mixins LibreNMSPermissionMixin,
NetBoxObjectPermissionMixin, LibreNMSAPIMixin, CacheMixin, VlanAssignmentMixin)
remain consistent with the chosen phrasing.
In @.github/instructions/background-jobs.instructions.md:
- Around line 30-84: Several Markdown headings in this document (e.g.,
"Superuser Requirement for Background Jobs", "Import Jobs", "Shared Cache Key
Pattern", "Permission Checks in Jobs", "Custom Sync Endpoint", "Import Page
Flow", "Import Action Views (`views/imports/actions.py`)", and "Key Import
Utilities (`import_utils.py`)") have no blank line following the heading which
triggers MD022; edit the file to insert a single blank line immediately after
each of those headings so the heading is followed by an empty line before the
next paragraph or list, preserving the existing content and indentation.
In @.github/instructions/sync.instructions.md:
- Line 8: Add missing blank lines after the affected headings (e.g.,
"Three-Layer View Architecture" and the other flagged headings) and ensure
fenced code blocks are surrounded by blank lines; specifically, insert a blank
line immediately after each heading and add one blank line both before and after
each ``` fenced block (the python example block and any other fenced blocks
flagged) so the document satisfies MD022 and MD031.
In @.github/instructions/testing.instructions.md:
- Around line 24-50: The markdown has MD022 violations because four headings
lack a blank line after them; open the file and insert a single blank line
immediately after each offending heading: "## Test File Naming", "## Test
Coverage by Module", "## Permission Test Patterns", and "## Shared Fixtures" so
that each heading is followed by an empty line before the next paragraph or
list.
In @.github/workflows/test.yaml:
- Line 67: The ln command in the workflow uses an unquoted command substitution
source path (ln -s
$(pwd)/../netbox-librenms-plugin/media/configuration.testing.py
netbox/netbox/configuration.py) which can trigger SC2046/word-splitting; fix it
by quoting the symlink source (wrap the
$(pwd)/../netbox-librenms-plugin/media/configuration.testing.py portion in
double quotes) so the shell treats the expanded path as a single argument while
leaving the target argument unchanged.
- Around line 41-55: The workflow uses floating refs for actions
(actions/checkout@main and actions/setup-python@main) which is
non-deterministic; update the action references to pinned major releases (for
example replace actions/checkout@main with actions/checkout@v4 and
actions/setup-python@main with actions/setup-python@v4 or the latest stable
major you support) so the steps "Checkout code", "Set up Python", and "Checkout
NetBox" reference immutable, reviewed tags instead of main.
In @.gitignore:
- Line 290: The ignore rule '*.pem' is too broad; restrict it to the actual CA
directory by replacing the global pattern with a scoped one that only ignores
PEM files under the devcontainer CA location (e.g., use a pattern targeting
'.devcontainer/*.pem' or '.devcontainer/**/*.pem' depending on whether nested
files exist) so that other .pem files in the repo remain tracked; update the
.gitignore entry accordingly.
- Line 290: The glob pattern "*.pem" in .gitignore is too broad; narrow it to
only ignore pem files in the devcontainer folder by replacing the bare pattern
with a scoped pattern that targets the devcontainer directory (e.g., change
"*.pem" to "devcontainer/*.pem" or the appropriate devcontainer subpath),
ensuring legitimate .pem files elsewhere (tests, docs, fixtures) remain tracked;
update the .gitignore entry near the existing devcontainer comments to keep
intent clear.
In `@docs/development/testing.md`:
- Around line 29-31: The Test Structure table in docs/development/testing.md
omitted the new test_permissions.py entry; update the table to include an entry
for test_permissions.py (referencing the filename test_permissions.py) with a
short description like "Permission system tests—two-tier permission checks and
access control logic" so the docs match the codebase and the new
permission-related tests are documented alongside test_vlan_sync.py and
test_interface_vlan_sync.py.
In `@docs/README.md`:
- Around line 56-58: The "### VLAN Sync" heading lacks a blank line below it
which violates MD022; open the README content around the "### VLAN Sync" heading
and insert a single empty line immediately after the "### VLAN Sync" line so the
subsequent bullet list starts after a blank line (i.e., ensure there is a
newline between the heading "### VLAN Sync" and the "- Create VLAN objects..."
list item).
In `@docs/usage_tips/permissions.md`:
- Around line 22-29: Update the ordered list numbering so all items use the "1."
style to satisfy markdownlint MD029: change the "2. **Tier 2: Object
permission**" entry to "1. **Tier 2: Object permission**" (the sections titled
"Tier 1: Plugin permission" and "Tier 2: Object permission") and ensure any
subsequent ordered entries also use "1." so the renderer auto-numbers correctly.
In `@docs/usage_tips/README.md`:
- Line 64: Update the duplicated phrasing in the docs: change the text "Exclude
columns to exclude from interface sync" (in the interface-management step 2
entry) to the suggested concise version "Exclude columns from interface sync" to
remove the repetition.
In `@docs/usage_tips/suggested_workflow.md`:
- Around line 52-54: Add a blank line after the "## 6. Sync VLAN" heading and
insert a bolded **Why** paragraph (matching the style of sections 1–5 and 7–9)
that succinctly explains why VLAN sync is placed here in the workflow; update
the block containing "## 6. Sync VLAN" and its two bullet points so it reads
with a separating blank line and a short bolded rationale sentence before or
after the bullets as the other sections do.
In `@netbox_librenms_plugin/api/views.py`:
- Around line 29-32: The permission check in has_permission (used with
SAFE_METHODS, PERM_VIEW_PLUGIN and PERM_CHANGE_PLUGIN) doesn't explicitly detect
unauthenticated users, causing DRF to return 403 instead of 401; update
has_permission to first verify request.user.is_authenticated (e.g., if not
getattr(request, "user", None) or not request.user.is_authenticated: return
False) before checking has_perm for PERM_VIEW_PLUGIN / PERM_CHANGE_PLUGIN so
unauthenticated requests yield a 401 from DRF's authentication layer.
- Around line 22-27: Update the docstring in api/views.py to use the full
permission strings instead of the short names: replace "view_librenmssettings"
with "netbox_librenms_plugin.view_librenmssettings" and
"change_librenmssettings" with "netbox_librenms_plugin.change_librenmssettings"
(the permissions referenced by the view class handling GET vs other methods,
matching the constants in constants.py and tests).
In `@netbox_librenms_plugin/jobs.py`:
- Around line 217-219: The call to bulk_import_vms passes both job=self and
user=self.job.user redundantly; remove the explicit user=self.job.user argument
so bulk_import_vms can auto-resolve the user from the provided job (leave
vm_result = bulk_import_vms(vm_imports, api, sync_options, libre_devices_cache,
job=self) intact and remove the user= parameter).
In `@netbox_librenms_plugin/librenms_api.py`:
- Around line 1014-1019: Normalize VLAN IDs from vlans_data before using them:
in the loop that processes vlan_entry (and the other block at the same pattern),
read vlan_entry.get("vlan") into vlan_raw, attempt to coerce to an int (e.g.,
try: vlan_id = int(vlan_raw) except (TypeError, ValueError): continue to skip
invalid/null entries), then use that integer for classification into
untagged_vlan or tagged_vlans and later call sorted(tagged_vlans). Ensure you
update both the vlan processing loop (references: vlan_entry, vlans_data,
tagged_vlans, untagged_vlan) and the other similar location so all
appended/compared VLAN IDs are integers.
- Around line 354-356: The unconditional addition of params["with"] = "vlans"
when with_vlans is true can break older LibreNMS servers; update the API call
that sets params based on with_vlans so that on a failing response (HTTP error
or API error indicating unknown/unsupported 'with' value) it retries the same
request without VLAN expansion (remove params["with"] or call again with
with_vlans=False). Locate the code that sets params["with"]="vlans" (the
with_vlans flag and the request/send function in librenms_api.py), wrap the
request in a try/inspect response block, and if the first attempt fails due to
unsupported VLAN expansion, perform a fallback retry without the VLAN param and
return that result.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 443-448: The fetch calls in librenms_sync.js directly dereference
document.querySelector('[name=csrfmiddlewaretoken]').value which throws if the
input is missing; add a safe CSRF resolver (e.g., a small helper like
getCsrfToken/resolveCsrfToken) that queries for the element, checks for null,
and returns element.value or an empty string/fallback, then use that helper in
the POST headers for the VLAN verify/save fetches (the call shown and the
similar one at lines 615-620) to avoid runtime exceptions.
In `@netbox_librenms_plugin/tables/interfaces.py`:
- Around line 109-287: render_vlans is doing too many tasks; extract logic into
focused private helpers and call them from render_vlans to improve readability
and testability. Create helper methods like _build_vlan_summary(self, all_vlans,
vlan_group_map, netbox_...), _build_vlan_tooltip(self, all_vlans,
vlan_group_map, missing_vlans), _build_hidden_inputs(self, all_vlans,
vlan_group_map, interface_name, safe_name), and _build_vlan_modal_json(self,
all_vlans, vlan_group_map, missing_vlans, netbox_...), move the corresponding
loops/formatting/JSON creation into these methods, return simple strings/JSON
from each helper, and replace the inlined blocks in render_vlans with calls to
these new helpers (keeping existing symbols like render_vlans, _parse_group_id,
get_untagged_vlan_css_class, get_tagged_vlan_css_class,
check_vlan_group_matches, vlan_groups and device) so behavior is unchanged.
- Around line 154-155: The current sanitization of interface_name into safe_name
only replaces "/" and ":" and can miss dots, spaces, and other problematic
characters; update the logic where interface_name is read
(record.get(self.interface_name_field, "")) and safe_name is computed so it
normalizes more comprehensively by replacing any character not allowed in HTML
name attributes (e.g., anything other than [A-Za-z0-9_.-]) with an underscore
and collapsing repeated underscores, and ensure the result never starts with a
digit if that matters for downstream CSS/JS consumers; locate the safe_name
assignment and replace the simple .replace calls with a regex-based
whitelist-replacement approach and optional trimming/collapse of underscores to
produce a consistent, safe identifier.
In `@netbox_librenms_plugin/tables/vlans.py`:
- Around line 128-141: The option labels are built by concatenating raw
group.name and group.scope into strings and then mark_safe is used, which can
allow XSS; update the loop that builds options (the code referencing
vlan_groups, selected_group_id, options, group.name, group.scope) to escape
group.name and group.scope before inserting into the option HTML (or use
Django's format_html to build each option safely) and only mark the
fully-assembled safe HTML once all parts have been escaped/constructed; ensure
the select_html call (format_html(..., mark_safe("".join(options)), ...))
receives already-escaped option content instead of raw group values.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html`:
- Around line 43-47: The badge color classes in the VLAN sync template are using
text-* utilities (e.g., the spans with "badge text-success text-white", "badge
text-warning text-white", "badge text-danger text-white") which causes invisible
text; update these to use background utility classes instead (replace
text-success/text-warning/text-danger with bg-success/bg-warning/bg-danger and
remove the conflicting text-white where appropriate or keep text-white only when
paired with bg-*) so the badges have proper filled colors; apply the same
replacement in the corresponding spans in _interface_sync_content.html (the
badge block around lines ~94-98) to keep styling consistent.
In `@netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync.html`:
- Around line 1-30: Move the reusable template _vlan_sync.html into the inc/
subdirectory (templates/netbox_librenms_plugin/inc/_vlan_sync.html) and update
any references to it (for example the include tag that currently uses
'netbox_librenms_plugin/_vlan_sync_content.html' or any place that includes
'_vlan_sync.html') to point to the new path under inc/; ensure the file name and
the include usage remain consistent (e.g., include
'netbox_librenms_plugin/inc/_vlan_sync.html' or update callers to reference the
moved partial).
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.html`:
- Around line 20-22: The HTMX out-of-band attribute on the element with id
"django-messages" uses hx-swap-oob="true" which triggers an outerHTML
replacement; change it to hx-swap-oob="innerHTML" (i.e., replace the attribute
value) so the toast container's contents are cleared without replacing the
container element itself, preserving layout and following project guidelines.
- Around line 20-22: Change the out-of-band swap on the messages container from
an outerHTML swap to an innerHTML swap: update the div with id "django-messages"
(which currently has hx-swap-oob="true") to use hx-swap-oob="innerHTML" so only
the container's contents are replaced/cleared rather than the entire element.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html`:
- Around line 211-218: Add a static id to the disabled checkbox input and
reference it from the label's for attribute so they are programmatically
associated; specifically, update the <input> for the use_background_job checkbox
to include an id (e.g., id="id_use_background_job") and add
for="id_use_background_job" to the corresponding <label> that renders {{
filter_form.use_background_job.label }}, leaving the disabled state and title
unchanged.
In `@netbox_librenms_plugin/tests/test_interface_vlan_sync.py`:
- Line 231: Remove the class-level pytest_plugins assignments (the
pytest_plugins = ["tests.test_librenms_api_helpers"] declarations) inside the
test classes in this file; pytest only honors module-level pytest_plugins and
the correct module string is already declared at the top as
"netbox_librenms_plugin.tests.test_librenms_api_helpers", so delete those two
dead class-level lines to avoid confusion.
- Around line 233-234: The tests test_parse_port_vlan_data_access_port,
test_parse_port_vlan_data_trunk_port, and
test_parse_port_vlan_data_uses_interface_name_field are unnecessarily decorated
with `@patch`("requests.get") and accept an unused mock_get parameter; remove the
`@patch`("requests.get") decorator from each test and delete the unused mock_get
argument from their signatures so they only take the needed mock_librenms_config
(and any other real params), keeping the tests focused on the pure data
transformation in parse_port_vlan_data.
In `@netbox_librenms_plugin/tests/test_sync_view_mismatch.py`:
- Around line 166-174: Update the docstring in the test in
netbox_librenms_plugin/tests/test_sync_view_mismatch.py to reflect that this
scenario is a mismatch: explain that LibreNMS provides a domain-stripped short
name 'sw01' while NetBox retains the full FQDN 'sw01.example.net', so despite
sharing the short label the identities do not match and the test asserts a
mismatch; reference the test's docstring/comments near the failing assertions in
this file (the docstring currently stating “matches” must be changed to describe
the mismatch behavior).
In `@netbox_librenms_plugin/tests/test_vlan_sync.py`:
- Around line 301-334: Add an integration-style test that calls
BaseVLANTableView.compare_vlans() directly using the existing fixtures (e.g.,
create_mock_vlan for NetBox entries and the librenms_vlan dict shape) instead of
only testing Python operators; construct a netbox_vlans mapping and one or more
librenms_vlan dicts, invoke BaseVLANTableView.compare_vlans(netbox_vlans,
librenms_vlan) and assert the returned structure flags presence/absence and name
equality (presence when vlan id exists, missing when it doesn't, and name_match
true/false when names match/differ). Ensure the test references
BaseVLANTableView.compare_vlans, create_mock_vlan, and uses the same keys
("vlan_vlan", "vlan_name") so it will catch wrong dict keys or missing fields.
In `@netbox_librenms_plugin/urls.py`:
- Around line 158-162: The URL accepts any object_type but
SyncVLANsView.get_object only supports "device", causing 404s for other types;
fix by either (A) restricting the URL to device only: change the route to
"device/<int:object_id>/sync-vlans/" and update the name if needed so
SyncVLANsView always receives a device, or (B) implement VM support in
SyncVLANsView: extend get_object to handle "virtualmachine" by resolving the
VirtualMachine model and return it (and complete the dead branch in _redirect to
build VM redirect URLs), ensuring both get_object and _redirect consistently
handle the same object_type values (use the class name SyncVLANsView, methods
get_object and _redirect to locate the code).
In `@netbox_librenms_plugin/views/__init__.py`:
- Line 59: Add a noqa for the unused-import false positive: update the import
line "from .sync.vlans import SyncVLANsView" in this re-export __init__.py so it
appends "# noqa: F401" to suppress the Flake8 F401 error (SyncVLANsView is
re-exported and consumed by urls.py).
- Line 9: Remove the unnecessary re-export of BaseVLANTableView from the package
public surface: delete the import line "from .base.vlan_table_view import
BaseVLANTableView" in the package __init__ and rely on its internal use as the
base class for DeviceVLANTableView, or if you intentionally want to keep it
public for future extension, mark the import with "# noqa: F401" to silence
Flake8; reference the BaseVLANTableView symbol and DeviceVLANTableView to locate
the related code.
In `@netbox_librenms_plugin/views/base/interfaces_view.py`:
- Around line 324-335: The loop over all_vids assumes override_group_id (from
vlan_group_overrides) is an integer string and calling int(override_group_id)
may raise ValueError for corrupted cache data; update the block in the function
handling vlan_group_map to defensively validate/convert override_group_id (e.g.,
check .isdigit() or wrap int(...) in try/except) before using it to index
override_groups_by_id, and skip or log and continue on invalid values so the
context building (variables: all_vids, vlan_group_overrides, override_group_id,
override_groups_by_id, vlan_group_map) does not crash.
In `@netbox_librenms_plugin/views/base/vlan_table_view.py`:
- Around line 107-109: The code uses timezone.timedelta which doesn't exist;
import datetime and use datetime.timedelta instead: add "import datetime" (or
"from datetime import timedelta") and replace timezone.timedelta(...) with
datetime.timedelta(...) in vlan_table_view.py (affecting cache_ttl/cache_expiry
computation using self.get_cache_key and cache.ttl), and apply the identical
change in interfaces_view.py (around the cache_expiry at interfaces handling),
cables_view.py, and ip_addresses_view.py where the same pattern appears.
- Around line 107-108: The call to cache.ttl(...) is not portable across Django
cache backends; update the assignment where cache_ttl is computed (e.g.,
cache_ttl = cache.ttl(self.get_cache_key(obj, "vlans"))) to call cache.ttl
inside a try/except AttributeError block and set cache_ttl = None (or a sensible
default) when AttributeError is raised, optionally logging a debug message;
apply the same defensive change to the other usages of cache.ttl in the codebase
so all occurrences (the lines that call cache.ttl with self.get_cache_key(...)
or similar keys) follow this pattern.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 765-779: The POST handler in SaveUserPrefView (post method)
performs a write by calling save_user_pref but does not enforce write
permissions; call require_write_permission_json(request) at the start of post
(before parsing input and before save_user_pref) to block view-only users,
keeping the existing validation of key in ALLOWED_PREFS and the JsonResponse
error flows intact.
In `@netbox_librenms_plugin/views/mixins.py`:
- Around line 597-599: The code currently falls back to selecting vlans[0] from
vid_to_vlans for an ambiguous VID which can nondeterministically pick the wrong
VLAN; change the fallback so that if vid_to_vlans.get(vid) yields more than one
VLAN you do NOT return the first match but instead treat it as ambiguous (return
None or raise an explicit error) and optionally log or record the ambiguity;
update the logic around vid_to_vlans, vlans and the return so only a single
unambiguous VLAN is returned (i.e., return the VLAN only when len(vlans) == 1).
- Line 25: The current code returns getattr(request, "path", "/") which falls
back to the current POST path when HTTP_REFERER is missing; change the logic to
first read request.META.get("HTTP_REFERER") and only if that is empty fall back
to a safe GET route (e.g. reverse("home") or settings.LOGIN_REDIRECT_URL or "/")
instead of request.path; update the function that returns getattr(request,
"path", "/") to use this HTTP_REFERER-first, safe-GET fallback approach so
redirects never target a POST-only endpoint.
In `@netbox_librenms_plugin/views/object_sync/devices.py`:
- Around line 149-153: Guard the JSON decoding in the view POST handlers by
wrapping the json.loads(request.body) call inside a try/except that catches
json.JSONDecodeError (and ValueError for older Python if desired) and returns a
400 response instead of letting a 500 occur; update the post(self, request)
method shown (and the other similar POST handlers at the regions around lines
275-279 and 325-331) to perform this try/except, log or include a short error
message, and return an HttpResponseBadRequest/JsonResponse with a clear error
payload when decoding fails.
- Around line 292-297: The current check only queries
VLAN.objects.filter(vid=vid, group=vlan_group) when vlan_group_id is present,
which misses existing global VLANs; update the logic in the verification branch
that uses vlan_group_id/vlan_group to accept either the selected group or global
VLANs by querying VLAN where vid==vid and (group==vlan_group OR
group__isnull==True) (use Django Q objects or an OR queryset) so netbox_vlan can
match group-specific or global VLANs just like the sibling verification flow.
- Around line 340-342: The code calls cache.ttl(...) (e.g.,
cache.ttl(self.get_cache_key(device, "ports")) in the device view) which is
backend-specific and will raise AttributeError on standard Django caches; update
every site that uses cache.ttl (the usages in devices.py, base/cables_view.py,
base/interfaces_view.py, base/vlan_table_view.py, base/ip_addresses_view.py) to
guard the call—either check hasattr(cache, "ttl") before calling and fall back
to None (or a sensible default), or wrap the call in a try/except AttributeError
and set ports_ttl (or the equivalent variable) to None when ttl is unavailable;
ensure you reference the same cache key logic (get_cache_key) and preserve
existing behavior when ttl is present.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 233-254: Wrap the platform creation and device assignment/save
inside a single database transaction so a validation or integrity error on the
device rolls back the newly created Platform; specifically, import
django.db.transaction and enclose the sequence that calls
Platform.objects.create(...), sets device.platform, calls device.full_clean()
and device.save() in a with transaction.atomic(): block, keep the existing
handling for IntegrityError from the Platform.create to show the slug-collision
message, and let device ValidationError/IntegrityError trigger rollback (catch
to show the existing error message but do not leave the platform created outside
the transaction).
In `@netbox_librenms_plugin/views/sync/devices.py`:
- Around line 24-29: The get_object method currently catches Device.DoesNotExist
but lets VirtualMachine.DoesNotExist propagate, causing a 500; update get_object
(the Device/VirtualMachine lookup) to use Django's get_object_or_404 for the
fallback VirtualMachine lookup (or wrap the second lookup in a try/except that
raises Http404) so that when neither Device nor VirtualMachine exists a proper
404 is returned instead of an unhandled exception.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 364-366: The except block that appends errors.append(f"Error
deleting interface {interface_name or interface_id}: {str(exc)}") exposes raw
exception text; change it to append a generic client-safe message (e.g.,
errors.append(f"Error deleting interface {interface_name or interface_id}:
internal server error")) and log the actual exception server-side using the
module logger (or processLogger) with exc_info=True so the stack trace is
preserved for debugging; update the except block around the deletion logic
(where errors, interface_name, interface_id are used) to remove str(exc) from
responses and call logger.error(...) with the exception.
- Around line 267-287: The current safe_name normalization (safe_name =
interface_name.replace("/", "_").replace(":", "_")) can produce collisions and
cause VLAN group selections to bleed across interfaces; update the form key
strategy used when reading POST values (the f"vlan_group_{safe_name}_{vid}"
lookup in the loop that builds vlan_group_map) so keys are uniquely derived from
interface identifiers—for example, generate and use a deterministic,
collision-resistant token per interface (e.g., an encoded or hashed
interface_name like sha256/interface_id) both when rendering the form and when
reading POST (so safe_name is replaced by that token), and update any code that
constructs the POST keys to use the same token (touch points: safe_name,
interface_name, vlan_group_map population and the request.POST lookup
f"vlan_group_{...}_{vid}").
- Around line 167-175: The current assignment to interface.enabled uses
librenms_interface.get("ifAdminStatus") being None to default to True, which
overwrites NetBox state when LibreNMS omits the field; change the logic so you
only set interface.enabled when ifAdminStatus is present (and not None) in
librenms_interface: check presence with librenms_interface.get("ifAdminStatus")
is not None (or "ifAdminStatus" in librenms_interface and
librenms_interface["ifAdminStatus"] is not None) and then set interface.enabled
to librenms_interface["ifAdminStatus"].lower() == "up" if it's a str else
bool(librenms_interface["ifAdminStatus"]); otherwise leave interface.enabled
untouched.
In `@netbox_librenms_plugin/views/sync/locations.py`:
- Around line 30-35: get_context_data is calling self.get_queryset() again
causing a duplicate remote call to get_librenms_locations; change it to reuse
the already-populated self.object_list (set by
django_tables2.SingleTableView.get()) or cache the queryset on the instance.
Concretely, inside get_context_data replace the direct call to
self.get_queryset() with something like: use self.object_list if it exists,
otherwise call self.get_queryset() once and assign it to self.object_list (or an
instance variable) before creating the filter form (referencing
get_context_data, get_queryset, and get_librenms_locations to locate the code).
In `@netbox_librenms_plugin/views/sync/vlans.py`:
- Around line 48-53: The get_object method in SyncVLANsView currently only
accepts object_type == "device" and raises Http404 for others; update
SyncVLANsView.get_object to also accept "virtualmachine" (matching
SyncInterfacesView/_redirect behavior) by returning
get_object_or_404(VirtualMachine, pk=object_id) when object_type ==
"virtualmachine" and ensure VirtualMachine is imported; keep the existing device
branch and only raise Http404 for unrecognized object_type values.
In `@README.md`:
- Around line 51-54: Add a blank line immediately after the "### VLAN Sync"
heading to satisfy markdownlint rule MD022; edit the README content around the
"### VLAN Sync" header (the heading string "### VLAN Sync") so there is an empty
line between the heading and the subsequent list items.
---
Outside diff comments:
In @.devcontainer/scripts/setup.sh:
- Around line 189-226: The script hardcodes
'/workspaces/netbox-librenms-plugin/' when building _pc_path, _xc_path, and
_cs_path so injected Python configs can be missed if detect_plugin_workspace
found a different PLUGIN_WS_DIR; change those three constructions to use the
PLUGIN_WS_DIR variable (the same variable set by detect_plugin_workspace)
instead of the literal path so _pc_path, _xc_path and _cs_path point to
"${PLUGIN_WS_DIR}/.devcontainer/config/..." (apply the same substitution for
plugin-config, extra-configuration and codespaces-configuration).
In @.pre-commit-config.yaml:
- Line 3: Update the ruff-pre-commit rev to the actual latest release by
replacing the current "rev: v0.14.13" in the ruff-pre-commit entry with "rev:
v0.15.2" (reference the rev field for ruff-pre-commit); after updating the rev,
run the pre-commit autoupdate or validate the YAML to ensure the change is
applied and the comment still accurately reflects the intent.
- Around line 3-9: Update the pre-commit hook id for the linter from "ruff" to
"ruff-check": locate the hooks block where the linter is declared (the entry
with id: ruff) and replace that id value with id: ruff-check so the hook matches
the current ruff-pre-commit naming; keep the same args and version (rev:
v0.14.13) unchanged.
In `@netbox_librenms_plugin/api/views.py`:
- Line 72: The code accesses rq_job.is_stopped which exists only in RQ >= 1.8.0;
either declare the runtime dependency rq >= 1.8.0 in pyproject.toml or make the
attribute access defensive to avoid AttributeError being swallowed by the broad
except that follows. Replace direct checks of rq_job.is_stopped (and similarly
rq_job.is_failed) with a safe lookup such as using getattr(rq_job, 'is_stopped',
False) and getattr(rq_job, 'is_failed', False) in the view handling (where
rq_job is inspected) so older rq versions treat missing attributes as False and
surface real errors correctly.
In `@netbox_librenms_plugin/import_utils.py`:
- Around line 1303-1308: The code currently polls the DB via
job.job.refresh_from_db() and job.job.status (and compares to JobStatusChoices)
which can miss RQ-level cancellations; replace that DB polling with an RQ-aware
status check by calling the RQ Job API on job.job (e.g., use
job.job.get_status() or the RQ status properties) and compare the returned RQ
status string to RQ terminal states like "failed", "stopped" (and
"canceled"/"cancelled" if applicable), then branch on that instead of the
DB-derived status_value; apply the same replacement for the second occurrence of
this pattern near the other bulk-import loop so both checks use RQ
job.get_status()/RQ status comparisons rather than
refresh_from_db()/JobStatusChoices.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html`:
- Around line 289-296: In the "Serial Number" row of the librenms_sync_base.html
template there is an HTML tag mismatch: a <td> is opened but closed with </th>;
update the closing tag to </td> for the table cell that contains "Serial Number"
and the conditional badge (the Serial Number Row block) so the <td>...{% if
object.virtual_chassis and vc_inventory_serials %}...{% endif %}</td> is
correctly balanced.
In `@netbox_librenms_plugin/templates/netbox_librenms_plugin/settings.html`:
- Around line 296-336: The script currently assumes elements with IDs
id_selected_server, id_vc_member_name_pattern, save-server-btn and
save-import-btn always exist; add null-safety checks for serverSelect,
vcPatternInput, saveServerBtn and saveImportBtn (similar to existing guards for
useSysnameCheckbox/stripDomainCheckbox) and bail out early if any required
element is missing or skip wiring that section; update updateServerSaveButton
and updateImportSaveButton to handle possible nulls and only attach event
listeners (serverSelect.addEventListener, vcPatternInput.addEventListener, etc.)
when the corresponding DOM element is non-null.
In `@netbox_librenms_plugin/views/sync/cables.py`:
- Around line 115-124: The transaction.atomic() in process_interface_sync and
the swallowing of exceptions in create_cable conflict: either make cable
creation truly transactional by letting create_cable re-raise exceptions (remove
the broad try/except or re-raise the caught Exception in create_cable) so
transaction.atomic() will roll back on failure, or accept partial success by
removing the transaction.atomic() block in process_interface_sync so successes
persist while failures are reported; update the logic in create_cable (the
function referenced by process_single_interface/process_interface_sync) or
remove the atomic context accordingly to reflect the intended all‑or‑nothing vs
partial success behavior.
In `@netbox_librenms_plugin/views/sync/locations.py`:
- Around line 46-53: The manual "q" filter block is dead because the method
returns whenever self.request.GET is truthy; remove the unreachable lines that
check if "q" in self.request.GET and the subsequent list comprehension filtering
sync_data; instead ensure text-search behavior is implemented in the existing
filterset (e.g., SiteLocationFilterSet) or wired through self.filterset so all
GET-based filtering happens in one place; update or add a filter field in the
filterset if needed and delete the redundant code referencing query,
item.netbox_site.name, and the manual sync_data reassignment.
- Around line 119-130: create_librenms_location currently calls
build_location_data without guarding site.latitude/site.longitude, which causes
"None" strings to be sent; mirror update_librenms_location's behavior by adding
a None-check before building the payload: ensure site.latitude and
site.longitude are not None (or sanitize them) before calling
build_location_data, and if either is None, handle appropriately (e.g., skip
creating the location or build a payload without coordinates) so that
build_location_data never receives None values for site.latitude/site.longitude.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (106)
.devcontainer/.env.example.devcontainer/README.md.devcontainer/config/plugin-config.py.example.devcontainer/devcontainer.json.devcontainer/docker-compose.yml.devcontainer/scripts/load-aliases.sh.devcontainer/scripts/setup.sh.devcontainer/scripts/start-netbox.sh.devcontainer/scripts/welcome.sh.github/copilot-instructions.md.github/instructions/background-jobs.instructions.md.github/instructions/frontend.instructions.md.github/instructions/sync.instructions.md.github/instructions/testing.instructions.md.github/workflows/lint-format.yaml.github/workflows/test.yaml.gitignore.pre-commit-config.yamlLICENSEREADME.mddocs/README.mddocs/SUMMARY.mddocs/development/README.mddocs/development/mixins.mddocs/development/structure.mddocs/development/templates.mddocs/development/testing.mddocs/development/views.mddocs/feature_list.mddocs/librenms_import/import_process.mddocs/librenms_import/import_settings.mddocs/usage_tips/README.mddocs/usage_tips/custom_field.mddocs/usage_tips/permissions.mddocs/usage_tips/suggested_workflow.mdmedia/configuration.testing.pymkdocs.ymlnetbox_librenms_plugin/api/serializers.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/constants.pynetbox_librenms_plugin/filters.pynetbox_librenms_plugin/filtersets.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/VM_status.pynetbox_librenms_plugin/tables/__init__.pynetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/locations.pynetbox_librenms_plugin/tables/mappings.pynetbox_librenms_plugin/tables/vlans.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/interfacetypemapping_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/settings.htmlnetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_interface_vlan_sync.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/__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/vlan_table_view.pynetbox_librenms_plugin/views/imports/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.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/object_sync/vms.pynetbox_librenms_plugin/views/settings_views.pynetbox_librenms_plugin/views/status_check.pynetbox_librenms_plugin/views/sync/cables.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/locations.pynetbox_librenms_plugin/views/sync/vlans.pypyproject.toml
💤 Files with no reviewable changes (2)
- docs/development/README.md
- docs/librenms_import/import_process.md
Comment on lines
+44
to
+46
| "REQUESTS_CA_BUNDLE": "${localEnv:REQUESTS_CA_BUNDLE}", | ||
| "SSL_CERT_FILE": "${localEnv:SSL_CERT_FILE}", | ||
| "CURL_CA_BUNDLE": "${localEnv:CURL_CA_BUNDLE}", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python requests library REQUESTS_CA_BUNDLE empty string environment variable SSL error
💡 Result:
If REQUESTS_CA_BUNDLE is set, requests treats it as the path to a CA bundle file (or directory). If you set it to the empty string (""), you’re effectively telling requests to verify TLS using “a CA bundle at path ''”, which is not a valid file path—so you typically get errors like:
OSError: Could not find a suitable TLS CA certificate bundle, invalid path: ...- or an
SSLErrorcascading from the failed CA loading. [1]
Fix
-
Unset the variable (recommended)
- macOS/Linux:
unset REQUESTS_CA_BUNDLE unset CURL_CA_BUNDLE
- Windows (PowerShell):
Remove-Item Env:REQUESTS_CA_BUNDLE Remove-Item Env:CURL_CA_BUNDLE
Unsetting is a common resolution when the variable was set incorrectly. [1]
- macOS/Linux:
-
Or set it to a real CA bundle path (e.g., your corporate root bundle, or a certifi-derived bundle you’ve extended):
REQUESTS_CA_BUNDLE=/path/to/bundle.pem. [2][3]
Notes
requestssupports bothREQUESTS_CA_BUNDLEandCURL_CA_BUNDLEfor overriding the trusted CA bundle;CURL_CA_BUNDLEis used as a fallback ifREQUESTS_CA_BUNDLEisn’t set. [2][4]- Some guides suggest using an empty string to “disable verification”, but that’s not a safe or reliable approach with
requests; if you truly must bypass verification, do it explicitly per call (verify=False) and understand the security risk. [5]
Sources:
[1] Stack Overflow: fix by unsetting REQUESTS_CA_BUNDLE when it causes “invalid path” TLS bundle errors. (stackoverflow.com)
[2] Conda docs: REQUESTS_CA_BUNDLE is the setting requests uses for non-standard certs. (docs.conda.io)
[3] hvac docs (quoting requests behavior): REQUESTS_CA_BUNDLE should point to a CA bundle file. (python-hvac.org)
[4] “SSL Yes Verify” docs: mentions REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE override. (ssl-yes-verify.dev)
[5] Stack Overflow discussion includes advice around empty env vars and fallback behavior; use with caution. (stackoverflow.com)
Citations:
- 1: https://stackoverflow.com/questions/56738345/could-not-install-packages-due-to-an-environmenterror-could-not-find-a-suitable?utm_source=openai
- 2: https://docs.conda.io/projects/conda/en/stable/user-guide/configuration/non-standard-certs.html?utm_source=openai
- 3: https://python-hvac.org/en/v0.8.2/advanced_usage.html?utm_source=openai
- 4: https://ssl-yes-verify.dev/docs/python/requests/?utm_source=openai
- 5: https://stackoverflow.com/questions/15445981/how-do-i-disable-the-security-certificate-check-in-pythons-requests?utm_source=openai
🌐 Web query:
devcontainer.json localEnv variable substitution default value
💡 Result:
In devcontainer.json, you can provide a default for a missing host env var by adding a 2nd : segment:
{
"build": {
"args": {
"REMOTE_UID": "${localEnv:REMOTE_UID:1000}"
}
},
"containerEnv": {
"IMAGE_TAG": "${localEnv:IMAGE_TAG:latest}"
}
}${localEnv:VARIABLE_NAME}resolves to the host env var value; if unset it’s blank.${localEnv:VARIABLE_NAME:default_value}usesdefault_valuewhen the host env var is not set.- Same pattern also applies to
${containerEnv:VARIABLE_NAME:default_value}. [1]
Sources: Dev Container spec “Variables in devcontainer.json”. [1]
[1] (containers.dev)
Citations:
🌐 Web query:
Python requests library verify SSL_CERT_FILE empty string handling
💡 Result:
requestsdoes not look atSSL_CERT_FILE(the OpenSSL/Python-stdlib env var). It only supportsREQUESTS_CA_BUNDLE, withCURL_CA_BUNDLEas a fallback. So if you setSSL_CERT_FILE=""(empty string), it generally has no effect onrequestsunless some other layer in your environment is influencing trust configuration. [1][2]- What
SSL_CERT_FILEis: it’s the OpenSSL default environment key name for overriding the CA file used by OpenSSL/Python’sssldefaults (i.e., things usingSSLContext.set_default_verify_paths()/ssl.get_default_verify_paths()), not something Requests promises to honor. [3][4] - Empty-string env var behavior in Requests (important regression fix): historically, setting
CURL_CA_BUNDLE=""could disable verification in Requests; this was fixed in Requests 2.28.0 (so an empty string should no longer “turn off” verification). [5] - If you pass an empty string directly to Requests (e.g.,
requests.get(url, verify="")), Requests treats it as a CA-bundle path, and TLS setup will typically fail because""isn’t a valid CA file path (expect anSSLError/ underlying file/path error). [1]
Sources:
[1] Requests docs (SSL verification + supported env vars) (docs.python-requests.org)
[2] Requests issue requesting SSL_CERT_FILE support (shows it isn’t supported) (github.com)
[3] Python ssl.get_default_verify_paths() docs (mentions OpenSSL env keys like SSL_CERT_FILE) (docs.python.org)
[4] OpenSSL docs (X509_get_default_cert_file_env / env var mechanism) (docs.openssl.org)
[5] StackOverflow answer/comment citing Requests 2.28.0 fix for CURL_CA_BUNDLE="" disabling verification (stackoverflow.com)
Citations:
- 1: https://docs.python-requests.org/en/master/user/advanced/%23ssl-cert-verification?utm_source=openai
- 2: requests: Support SSL_CERT_FILE environment variable psf/requests#2899
- 3: https://docs.python.org/3.12/library/ssl.html?utm_source=openai
- 4: https://docs.openssl.org/3.6/man3/X509_get_default_cert_file/?utm_source=openai
- 5: https://stackoverflow.com/questions/15445981/how-do-i-disable-the-security-certificate-check-in-pythons-requests?utm_source=openai
Empty REQUESTS_CA_BUNDLE / CURL_CA_BUNDLE will break HTTPS.
${localEnv:VAR} (no default) evaluates to an empty string when the host variable is not set. Passing REQUESTS_CA_BUNDLE="" or CURL_CA_BUNDLE="" into the container causes the Python requests library to treat it as an explicit (invalid) CA bundle path, raising SSL errors like "Could not find a suitable TLS CA certificate bundle, invalid path" for all HTTPS calls.
Note: SSL_CERT_FILE is not used by the requests library (it only honors REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE); only guard the latter two.
The safeguard using : as a default (e.g., ${localEnv:REQUESTS_CA_BUNDLE:}) still results in an empty string and does not prevent this issue. Instead, conditionally set these variables inside setup.sh only when the host variables are non-empty, or omit them from containerEnv entirely if not strictly required.
Conditional approach (setup.sh)
if [ -n "$REQUESTS_CA_BUNDLE" ]; then
export REQUESTS_CA_BUNDLE
fi
if [ -n "$CURL_CA_BUNDLE" ]; then
export CURL_CA_BUNDLE
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/devcontainer.json around lines 44 - 46, The devcontainer
currently unconditionally injects REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE using
${localEnv:...}, which can pass empty strings and break Python HTTPS requests;
instead, remove these two from the containerEnv and implement a conditional
export in setup.sh (or environment bootstrap) that only exports
REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE when the corresponding host vars are
non-empty (check with -n or equivalent), leaving SSL_CERT_FILE untouched since
requests ignores it; this ensures REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE are only
set inside the container when valid values exist.
Comment on lines
+32
to
+34
| REQUESTS_CA_BUNDLE: ${REQUESTS_CA_BUNDLE:-} | ||
| SSL_CERT_FILE: ${SSL_CERT_FILE:-} | ||
| CURL_CA_BUNDLE: ${CURL_CA_BUNDLE:-} |
There was a problem hiding this comment.
Empty-string defaults for CA bundle env vars will silently break or disable TLS verification.
Setting REQUESTS_CA_BUNDLE to an empty string "effectively disables SSL verification for all requests made using the requests library." Additionally, REQUESTS_CA_BUNDLE= with session.verify = True raises an SSLError (certificate verify failed against the empty path). The behavior varies by requests version — in either case it's wrong.
The ${VAR:-} syntax unconditionally exports these variables as "" into the container when the host hasn't set them. This directly contradicts the guidance in .env.example which says "Normally you SHOULD NOT set REQUESTS_CA_BUNDLE, SSL_CERT_FILE, or CURL_CA_BUNDLE here".
Every developer without a custom CA bundle will have these exported as empty strings, breaking TLS for all requests calls inside the container.
🛠️ Proposed fix — omit the variables when not explicitly set
Use bare variable references (no default): Docker Compose will inherit them from the host if set, and leave them absent from the container environment if not set on the host.
- REQUESTS_CA_BUNDLE: ${REQUESTS_CA_BUNDLE:-}
- SSL_CERT_FILE: ${SSL_CERT_FILE:-}
- CURL_CA_BUNDLE: ${CURL_CA_BUNDLE:-}
+ REQUESTS_CA_BUNDLE: ${REQUESTS_CA_BUNDLE}
+ SSL_CERT_FILE: ${SSL_CERT_FILE}
+ CURL_CA_BUNDLE: ${CURL_CA_BUNDLE}With this change, Docker Compose will omit the variable from the container environment when it is unset on the host, preserving requests' default certifi CA resolution.
📝 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
| REQUESTS_CA_BUNDLE: ${REQUESTS_CA_BUNDLE:-} | |
| SSL_CERT_FILE: ${SSL_CERT_FILE:-} | |
| CURL_CA_BUNDLE: ${CURL_CA_BUNDLE:-} | |
| REQUESTS_CA_BUNDLE: ${REQUESTS_CA_BUNDLE} | |
| SSL_CERT_FILE: ${SSL_CERT_FILE} | |
| CURL_CA_BUNDLE: ${CURL_CA_BUNDLE} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/docker-compose.yml around lines 32 - 34, The compose file
currently exports REQUESTS_CA_BUNDLE, SSL_CERT_FILE, and CURL_CA_BUNDLE with
empty-string defaults using ${VAR:-}, which forces empty values into the
container and breaks TLS; change those environment entries to use bare variable
references (e.g., ${REQUESTS_CA_BUNDLE}, ${SSL_CERT_FILE}, ${CURL_CA_BUNDLE}) so
Docker Compose will inherit them from the host if set and omit them entirely if
unset, preserving normal requests/certifi behavior; update the lines referencing
REQUESTS_CA_BUNDLE, SSL_CERT_FILE, and CURL_CA_BUNDLE in
.devcontainer/docker-compose.yml accordingly.
Comment on lines
+12
to
+18
| - Views follow a three-layer structure: | ||
| - **Base views** (`views/base/`) — abstract views for each sync resource (`BaseInterfaceTableView`, `BaseCableTableView`, `BaseIPAddressTableView`, `BaseVLANTableView`). | ||
| - **Object sync views** (`views/object_sync/`) — concrete per-model views registered as tabs on NetBox's Device/VM detail pages via `@register_model_view(Device, ...)`. These wire base views to models. | ||
| - **Sync action views** (`views/sync/`) — POST-only views that apply changes (add/change/delete NetBox objects). Includes `interfaces.py`, `cables.py`, `ip_addresses.py`, `vlans.py`, `devices.py`, `device_fields.py`, `locations.py`. | ||
| - **Shared mixins** (`views/mixins.py`) — `LibreNMSPermissionMixin`, `NetBoxObjectPermissionMixin`, `LibreNMSAPIMixin`, `CacheMixin`, `VlanAssignmentMixin`. | ||
| - All four sync resources (interfaces, cables, IP addresses, VLANs) follow the same three-layer pattern. VLAN sync additionally uses `VlanAssignmentMixin` for VLAN group scope resolution (Rack → Location → Site → SiteGroup → Region → Global). | ||
| - New views should extend the closest base class and compose mixins. |
There was a problem hiding this comment.
"Three-layer structure" description lists four items.
Line 12 says "three-layer structure" but the subsequent bullets describe four items: Base views, Object sync views, Sync action views, and Shared mixins. Either update to "four-layer" (or "three-layer plus shared mixins") to match the content.
🧰 Tools
🪛 LanguageTool
[grammar] ~14-~14: Use a hyphen to join words.
Context: ...ter_model_view(Device, ...)`. These wire base views to models. - **Sync action ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/copilot-instructions.md around lines 12 - 18, The description
incorrectly calls this a "three-layer structure" but then enumerates four items;
update the header text to accurately reflect the structure (e.g., change
"Three-layer structure" to "Four-layer structure" or "Three-layer plus shared
mixins") and ensure the surrounding text and examples (references to
views/base/, views/object_sync/, views/sync/, and views/mixins.py as well as
mixins LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, LibreNMSAPIMixin,
CacheMixin, VlanAssignmentMixin) remain consistent with the chosen phrasing.
Comment on lines
+38
to
+70
| ## Permission System | ||
| - Uses two-tier permissions via `LibreNMSSettings` model: `view_librenmssettings` (read) and `change_librenmssettings` (write). See `docs/development/permissions.md`. | ||
| - Permission constants in `constants.py`: `PERM_VIEW_PLUGIN` and `PERM_CHANGE_PLUGIN`. | ||
|
|
||
| ### Plugin-Level Permissions | ||
| - All views inherit `LibreNMSPermissionMixin` from `views/mixins.py`, which sets `permission_required = PERM_VIEW_PLUGIN` and provides: | ||
| - `has_write_permission()` — checks `PERM_CHANGE_PLUGIN`. | ||
| - `require_write_permission()` — returns error response (HTMX `HX-Redirect` or standard redirect) if denied. | ||
| - `require_write_permission_json()` — returns `JsonResponse(403)` if denied (for AJAX endpoints). | ||
|
|
||
| ### Object-Level Permissions | ||
| - `NetBoxObjectPermissionMixin` adds a **second layer** of permission checking for NetBox model operations (add/change/delete on Device, Interface, VLAN, etc.). | ||
| - Views declare `required_object_permissions` dict mapping HTTP methods to `[(action, Model)]` tuples, e.g.: | ||
| ```python | ||
| required_object_permissions = {"POST": [("add", VLAN), ("change", VLAN)]} | ||
| ``` | ||
| - Some views set `required_object_permissions` dynamically per-request (e.g., `SyncInterfacesView` switches between `Interface` and `VMInterface` based on object type). | ||
| - Provides: | ||
| - `check_object_permissions(method)` → `(bool, missing_perms_list)` | ||
| - `require_object_permissions(method)` — redirect/HTMX on failure. | ||
| - `require_object_permissions_json(method)` — JSON 403 on failure. | ||
| - `require_all_permissions(method)` — combined plugin write + object perms check (redirect/HTMX). | ||
| - `require_all_permissions_json(method)` — combined check, JSON variant. | ||
| - **Sync POST handlers** must call `require_all_permissions("POST")` (not just `require_write_permission()`) and return early if it returns a response. AJAX/JSON endpoints use `require_all_permissions_json("POST")`. | ||
| - `_get_safe_redirect_url(request)` validates referrer URLs to prevent open-redirect attacks. | ||
|
|
||
| ### Permission Helpers for Background Jobs | ||
| - Background jobs run outside view context and cannot use view mixins. Use standalone helpers from `import_utils.py` (`check_user_permissions`, `require_permissions`). See `background-jobs.instructions.md` for details. | ||
|
|
||
| ### API & Navigation Permissions | ||
| - API endpoints use `LibreNMSPluginPermission` class in `api/views.py` (GET=view, others=change). | ||
| - Navigation menu (`navigation.py`) has 3 groups: **Settings** (Plugin Settings, Interface Mappings), **Import** (LibreNMS Import), **Status Check** (Site & Location Sync, Device Status, VM Status). All items use `permissions=[PERM_VIEW_PLUGIN]`. | ||
| - **Background job polling requires superuser** — non-superusers fall back to synchronous mode. See `background-jobs.instructions.md` for details. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Markdown lint: add blank lines around headings and fenced code blocks.
Multiple headings (Lines 38, 42, 48, 64, 67) and the fenced code block (Lines 51-53) are missing required surrounding blank lines per MD022/MD031. This can cause rendering issues in some Markdown parsers.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 38-38: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 42-42: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 48-48: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 53-53: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 64-64: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 67-67: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/copilot-instructions.md around lines 38 - 70, Add missing blank
lines around Markdown headings and the fenced code block: insert a blank line
before and after the "Permission System", "Plugin-Level Permissions",
"Object-Level Permissions", "Permission Helpers for Background Jobs", and "API &
Navigation Permissions" headings, and ensure there is a blank line both before
and after the fenced code block containing the required_object_permissions
example (the block starting with ```python and the VLAN example). This will
satisfy MD022/MD031 by spacing headings and fenced code blocks correctly.
Comment on lines
+30
to
+84
| ## Superuser Requirement for Background Jobs | ||
| - NetBox's `/api/core/background-tasks/` endpoint requires **superuser** (`IsSuperuser` in `BaseRQViewSet`). | ||
| - Non-superuser users cannot poll job status; they get 403 Forbidden. | ||
| - The plugin automatically falls back to synchronous mode for non-superusers—see `should_use_background_job()` in `list.py` and `actions.py`. | ||
| - This is a NetBox core design decision, not a plugin limitation. No amount of permissions (including `core.view_job`) bypasses it. | ||
|
|
||
| ## Import Jobs | ||
| - **`FilterDevicesJob`** — background device filtering with VC detection. `job.data` keys: `device_ids`, `total_processed`, `filters`, `server_key`, `vc_detection_enabled`, `cache_timeout`, `cached_at`, `completed`. Devices are cached individually via shared cache keys from `get_validated_device_cache_key()`. | ||
| - **`ImportDevicesJob`** — background device/VM import. Calls `bulk_import_devices_shared()` for devices and `bulk_import_vms()` for VMs. `job.data` keys: `imported_device_pks`, `imported_vm_pks`, `imported_libre_device_ids`, `imported_libre_vm_ids`, `server_key`, `total`, `success_count`, `failed_count`, `skipped_count`, `virtual_chassis_created`, `errors`, `completed`. | ||
|
|
||
| ## Shared Cache Key Pattern | ||
| - Both synchronous and background modes use `get_validated_device_cache_key()` from `import_utils.py` to generate cache keys. This ensures `_load_job_results()` in the list view can retrieve devices regardless of which mode produced them. | ||
| - `get_active_cached_searches()` manages multi-search cache to let users run and switch between searches. | ||
| - Never hardcode cache key formats; always use the helper functions. | ||
|
|
||
| ## Permission Checks in Jobs | ||
| - Background jobs run outside view context, so they cannot use view mixins. | ||
| - Use standalone helpers from `import_utils.py` for permission checks inside job code: | ||
| - `check_user_permissions(user, permissions)` → `(bool, missing_list)` | ||
| - `require_permissions(user, permissions, action_description)` — raises `PermissionDenied`. | ||
|
|
||
| ## Custom Sync Endpoint | ||
| `api/views.py::sync_job_status()` syncs database Job status with RQ job status, needed because NetBox worker doesn't always update DB when jobs stop before processing starts. | ||
|
|
||
| ## Import Page Flow | ||
| The import page (`LibreNMSImportView` in `views/imports/list.py`) supports two modes: | ||
|
|
||
| 1. **Synchronous** — calls `process_device_filters()` directly, renders results inline. | ||
| 2. **Background** — enqueues `FilterDevicesJob`, returns `JsonResponse` with `job_id`/`job_pk`/`poll_url`. Frontend polls and redirects to `?job_id={pk}` on completion. | ||
|
|
||
| Result loading: `_load_job_results(job_id)` reads `job.data["device_ids"]`, reconstructs devices from per-device cache using `get_validated_device_cache_key()`. | ||
|
|
||
| Filter fields: `librenms_location`, `librenms_type`, `librenms_os`, `librenms_hostname`, `librenms_sysname`, `librenms_hardware`, `enable_vc_detection`, `show_disabled`, `exclude_existing`. | ||
|
|
||
| ## Import Action Views (`views/imports/actions.py`) | ||
| - **`DeviceImportHelperMixin`** — provides `get_validated_device_with_selections()` and `render_device_row()` for HTMX row rendering. Shared by update views. | ||
| - **`BulkImportConfirmView`** (POST) — renders confirmation modal with selected device list. Returns `htmx/bulk_import_confirm.html`. | ||
| - **`BulkImportDevicesView`** (POST) — executes import. Background mode enqueues `ImportDevicesJob`; sync mode calls `bulk_import_devices()` + `bulk_import_vms()` and returns OOB row swaps with `HX-Trigger: closeModal`. | ||
| - **`DeviceValidationDetailsView`** (GET) — renders expandable validation details via `htmx/device_validation_details.html`. | ||
| - **`DeviceVCDetailsView`** (GET) — renders VC member details via `htmx/device_vc_details.html`. | ||
| - **`DeviceRoleUpdateView`**, **`DeviceClusterUpdateView`**, **`DeviceRackUpdateView`** (POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row via `render_device_row()`. | ||
|
|
||
| ## Key Import Utilities (`import_utils.py`) | ||
| - `process_device_filters(filters, ...)` — fetches and validates devices from LibreNMS, returns list. | ||
| - `validate_device_for_import(device, ...)` — core validation function, produces validation state dict. | ||
| - `bulk_import_devices_shared(devices, user, ...)` — shared implementation between sync and background import. | ||
| - `bulk_import_vms(vm_imports, user, ...)` — VM import implementation. | ||
| - `fetch_device_with_cache(device_id, ...)` — retrieves/caches individual device data. | ||
| - Cache key functions: `get_validated_device_cache_key()`, `get_cache_metadata_key()`, `get_active_cached_searches()`, `get_import_device_cache_key()`. | ||
|
|
||
| ## Validation Helpers (`import_validation_helpers.py`) | ||
| Centralizes validation state mutation used by the role/cluster/rack update views: | ||
| - `apply_role_to_validation()`, `apply_cluster_to_validation()`, `apply_rack_to_validation()` — update validation state when user selects a role/cluster/rack. | ||
| - `remove_validation_issue()`, `recalculate_validation_status()` — maintain issue list and overall status. | ||
| - `fetch_model_by_id()`, `extract_device_selections()` — helpers for reading form data. |
There was a problem hiding this comment.
Multiple headings violate MD022 (missing blank line below heading).
Lines 30, 36, 40, 45, 54, 64, 72, and 80 all have content immediately following the heading without a blank line. Add a blank line after each heading to satisfy markdownlint-cli2.
Example pattern for each:
## Superuser Requirement for Background Jobs
+
- NetBox's `/api/core/background-tasks/` endpoint...🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 30-30: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 36-36: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 54-54: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 64-64: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 72-72: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 80-80: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/instructions/background-jobs.instructions.md around lines 30 - 84,
Several Markdown headings in this document (e.g., "Superuser Requirement for
Background Jobs", "Import Jobs", "Shared Cache Key Pattern", "Permission Checks
in Jobs", "Custom Sync Endpoint", "Import Page Flow", "Import Action Views
(`views/imports/actions.py`)", and "Key Import Utilities (`import_utils.py`)")
have no blank line following the heading which triggers MD022; edit the file to
insert a single blank line immediately after each of those headings so the
heading is followed by an empty line before the next paragraph or list,
preserving the existing content and indentation.
Comment on lines
+324
to
+335
| for vid in all_vids: | ||
| vid_str = str(vid) | ||
| if vid_str in vlan_group_overrides: | ||
| override_group_id = vlan_group_overrides[vid_str] | ||
| if override_group_id: | ||
| group = override_groups_by_id.get(int(override_group_id)) | ||
| if group: | ||
| vlan_group_map[vid] = { | ||
| "group_id": str(group.pk), | ||
| "group_name": group.name, | ||
| "is_ambiguous": False, | ||
| } |
There was a problem hiding this comment.
int(override_group_id) can raise ValueError on corrupted cache data.
override_group_id comes from cache and could contain a non-integer string. This would crash the entire context data preparation.
🛡️ Proposed defensive fix
for vid in all_vids:
vid_str = str(vid)
if vid_str in vlan_group_overrides:
override_group_id = vlan_group_overrides[vid_str]
if override_group_id:
- group = override_groups_by_id.get(int(override_group_id))
+ try:
+ group = override_groups_by_id.get(int(override_group_id))
+ except (ValueError, TypeError):
+ group = None
if group:📝 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
| for vid in all_vids: | |
| vid_str = str(vid) | |
| if vid_str in vlan_group_overrides: | |
| override_group_id = vlan_group_overrides[vid_str] | |
| if override_group_id: | |
| group = override_groups_by_id.get(int(override_group_id)) | |
| if group: | |
| vlan_group_map[vid] = { | |
| "group_id": str(group.pk), | |
| "group_name": group.name, | |
| "is_ambiguous": False, | |
| } | |
| for vid in all_vids: | |
| vid_str = str(vid) | |
| if vid_str in vlan_group_overrides: | |
| override_group_id = vlan_group_overrides[vid_str] | |
| if override_group_id: | |
| try: | |
| group = override_groups_by_id.get(int(override_group_id)) | |
| except (ValueError, TypeError): | |
| group = None | |
| if group: | |
| vlan_group_map[vid] = { | |
| "group_id": str(group.pk), | |
| "group_name": group.name, | |
| "is_ambiguous": False, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/base/interfaces_view.py` around lines 324 - 335,
The loop over all_vids assumes override_group_id (from vlan_group_overrides) is
an integer string and calling int(override_group_id) may raise ValueError for
corrupted cache data; update the block in the function handling vlan_group_map
to defensively validate/convert override_group_id (e.g., check .isdigit() or
wrap int(...) in try/except) before using it to index override_groups_by_id, and
skip or log and continue on invalid values so the context building (variables:
all_vids, vlan_group_overrides, override_group_id, override_groups_by_id,
vlan_group_map) does not crash.
Comment on lines
+107
to
+109
| # Calculate cache TTL | ||
| cache_ttl = cache.ttl(self.get_cache_key(obj, "vlans")) | ||
| cache_expiry = timezone.now() + timezone.timedelta(seconds=cache_ttl) if cache_ttl else None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Django version and timezone module exports
python3 << 'EOF'
try:
from django.utils import timezone
print("=== django.utils.timezone attributes ===")
print(f"Has timedelta: {hasattr(timezone, 'timedelta')}")
print(f"Module attributes: {[attr for attr in dir(timezone) if not attr.startswith('_')][:20]}")
print()
print("=== datetime module ===")
from datetime import timedelta
print(f"timedelta available from datetime: True")
except Exception as e:
print(f"Error: {e}")
EOFRepository: marcinpsk/netbox-librenms-plugin
Length of output: 107
🏁 Script executed:
# Find and examine the actual file
fd -t f 'vlan_table_view.py' | head -5Repository: marcinpsk/netbox-librenms-plugin
Length of output: 128
🏁 Script executed:
# Search for cache.ttl usage in the codebase
rg -n 'cache\.ttl' --type=pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 687
🏁 Script executed:
# Examine the imports and line 109 of the vlan_table_view.py file
fd -t f 'vlan_table_view.py' -x head -120 {} | grep -A 10 -B 5 'timezone.timedelta\|cache\.ttl'Repository: marcinpsk/netbox-librenms-plugin
Length of output: 670
🏁 Script executed:
# Check the imports at the top of vlan_table_view.py
head -20 netbox_librenms_plugin/views/base/vlan_table_view.pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 769
🏁 Script executed:
# Check full context around the problematic lines
sed -n '1,120p' netbox_librenms_plugin/views/base/vlan_table_view.py | tail -40Repository: marcinpsk/netbox-librenms-plugin
Length of output: 1526
🏁 Script executed:
# Search for any timedelta import in the file
rg -n 'import.*timedelta|from.*timedelta' netbox_librenms_plugin/views/base/vlan_table_view.pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 58
🏁 Script executed:
# Check if datetime is imported anywhere in this file
rg -n 'import datetime|from datetime' netbox_librenms_plugin/views/base/vlan_table_view.pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 58
🏁 Script executed:
# Let me check Django's actual source to understand timezone module behavior
# by looking at common Django cache backend patterns in the repo
rg -n 'from django.core.cache|import cache' netbox_librenms_plugin/views/base/vlan_table_view.py -A 2Repository: marcinpsk/netbox-librenms-plugin
Length of output: 206
🏁 Script executed:
# Check other similar files to see the pattern of timedelta usage
rg -n 'timezone\.now.*timedelta|timezone\.timedelta' --type=py netbox_librenms_plugin/Repository: marcinpsk/netbox-librenms-plugin
Length of output: 679
🏁 Script executed:
# Check the imports in interfaces_view.py which also uses cache.ttl
head -25 netbox_librenms_plugin/views/base/interfaces_view.py | grep -E '^(import|from)'Repository: marcinpsk/netbox-librenms-plugin
Length of output: 369
🏁 Script executed:
# Check if timedelta is used anywhere else in the codebase correctly
rg -n 'timedelta' --type=py netbox_librenms_plugin/ -B 1 -A 1Repository: marcinpsk/netbox-librenms-plugin
Length of output: 1368
🏁 Script executed:
# Let me verify if django.utils.timezone re-exports timedelta by checking Django source or documentation patterns
# Check if there's a requirements file or setup.py that shows Django version
fd -t f 'requirements*.txt|setup.py|pyproject.toml' | xargs cat 2>/dev/null | grep -i djangoRepository: marcinpsk/netbox-librenms-plugin
Length of output: 118
🏁 Script executed:
# Check django source code documentation or comments that might clarify timezone.timedelta
rg -n 'timedelta' netbox_librenms_plugin/views/base/ --type=py -B 3 -A 1 | head -50Repository: marcinpsk/netbox-librenms-plugin
Length of output: 2125
🏁 Script executed:
# Search for Django documentation or source about timezone module exports
# Let me check if there are any test files that use timezone.timedelta
rg -n 'timezone\.timedelta' --type=py netbox_librenms_plugin/Repository: marcinpsk/netbox-librenms-plugin
Length of output: 679
🏁 Script executed:
# Check if there's any documentation or comments about timezone usage in the project
rg -n 'django.utils.timezone' --type=py netbox_librenms_plugin/ | head -20Repository: marcinpsk/netbox-librenms-plugin
Length of output: 58
🏁 Script executed:
# Let me also verify by checking Django's actual behavior with a simple check
# of how many times timezone is imported and used
rg -n 'from django.utils import timezone' --type=py netbox_librenms_plugin/ | wc -lRepository: marcinpsk/netbox-librenms-plugin
Length of output: 77
Import datetime.timedelta — timezone.timedelta does not exist in Django and will raise AttributeError at runtime.
django.utils.timezone does not expose timedelta. Use datetime.timedelta instead.
Fix required
+from datetime import timedelta
from django.contrib import messages
from django.core.cache import cache
from django.shortcuts import get_object_or_404, render
from django.utils import timezone
from django.views import View # Calculate cache TTL
cache_ttl = cache.ttl(self.get_cache_key(obj, "vlans"))
- cache_expiry = timezone.now() + timezone.timedelta(seconds=cache_ttl) if cache_ttl else None
+ cache_expiry = timezone.now() + timedelta(seconds=cache_ttl) if cache_ttl else NoneThis pattern also appears in interfaces_view.py:248, cables_view.py:287, and ip_addresses_view.py:259 — all require the same fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/base/vlan_table_view.py` around lines 107 - 109,
The code uses timezone.timedelta which doesn't exist; import datetime and use
datetime.timedelta instead: add "import datetime" (or "from datetime import
timedelta") and replace timezone.timedelta(...) with datetime.timedelta(...) in
vlan_table_view.py (affecting cache_ttl/cache_expiry computation using
self.get_cache_key and cache.ttl), and apply the identical change in
interfaces_view.py (around the cache_expiry at interfaces handling),
cables_view.py, and ip_addresses_view.py where the same pattern appears.
Comment on lines
+107
to
+108
| # Calculate cache TTL | ||
| cache_ttl = cache.ttl(self.get_cache_key(obj, "vlans")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if django-redis is a dependency
echo "=== Checking for django-redis in dependency files ==="
find . -maxdepth 3 \( -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" -o -name "Pipfile" \) -type f -exec grep -l "django-redis\|django.redis" {} \; 2>/dev/null || echo "No django-redis found in common dependency files"
echo ""
echo "=== Checking CACHES configuration in Python files ==="
rg -n 'CACHES' --type=py -A10 -B2 || echo "No CACHES configuration found"
echo ""
echo "=== Checking cache.ttl usage in the codebase ==="
rg -n 'cache\.ttl' --type=py
echo ""
echo "=== Examining the target file ==="
if [ -f "netbox_librenms_plugin/views/base/vlan_table_view.py" ]; then
echo "File exists. Showing lines 100-120:"
sed -n '100,120p' netbox_librenms_plugin/views/base/vlan_table_view.py
fiRepository: marcinpsk/netbox-librenms-plugin
Length of output: 1776
🏁 Script executed:
#!/bin/bash
echo "=== Checking for setup.py or pyproject.toml ==="
find . -maxdepth 2 -type f \( -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" \) | head -5
echo ""
echo "=== Reading setup.py if it exists ==="
if [ -f "setup.py" ]; then
cat setup.py
fi
echo ""
echo "=== Reading pyproject.toml if it exists ==="
if [ -f "pyproject.toml" ]; then
cat pyproject.toml
fi
echo ""
echo "=== Checking how cache is imported in the problematic files ==="
rg -n 'from django\.core\.cache\|from django_redis\|import cache' --type=py -B2 netbox_librenms_plugin/views/base/vlan_table_view.py netbox_librenms_plugin/views/object_sync/devices.py | head -40
echo ""
echo "=== Check for any cache compatibility shim or wrapper ==="
fd -name "*cache*" --type fRepository: marcinpsk/netbox-librenms-plugin
Length of output: 2247
🏁 Script executed:
#!/bin/bash
echo "=== Finding all requirements files ==="
find . -name "requirements*.txt" -o -name "Pipfile" -o -name "poetry.lock"
echo ""
echo "=== Checking imports in vlan_table_view.py ==="
head -30 netbox_librenms_plugin/views/base/vlan_table_view.py
echo ""
echo "=== Checking imports in devices.py (another file using cache.ttl) ==="
head -30 netbox_librenms_plugin/views/object_sync/devices.py
echo ""
echo "=== Searching for cache import patterns ==="
rg 'from django' netbox_librenms_plugin/views/base/vlan_table_view.py | head -20
echo ""
echo "=== Check if cache object is created/imported ==="
rg 'cache\s*=' netbox_librenms_plugin/views/base/vlan_table_view.py | head -10Repository: marcinpsk/netbox-librenms-plugin
Length of output: 2465
🏁 Script executed:
#!/bin/bash
echo "=== Checking requirements_dev.txt ==="
cat requirements_dev.txt
echo ""
echo "=== Verify all files using cache.ttl ==="
rg -l 'cache\.ttl' --type=pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 438
cache.ttl() is not available on all Django cache backends and is used across multiple files.
cache.ttl(key) is provided by django-redis but is not part of Django's built-in cache API. If the project's NetBox environment uses LocMemCache, FileBasedCache, or DatabaseCache, this will raise AttributeError. This issue affects five files:
netbox_librenms_plugin/views/base/vlan_table_view.py:108netbox_librenms_plugin/views/object_sync/devices.py:340netbox_librenms_plugin/views/base/interfaces_view.py:247netbox_librenms_plugin/views/base/cables_view.py:285netbox_librenms_plugin/views/base/ip_addresses_view.py:257
Wrap cache.ttl() calls in a try/except or document the django-redis backend requirement consistently across all usages.
Proposed defensive approach
# Calculate cache TTL
- cache_ttl = cache.ttl(self.get_cache_key(obj, "vlans"))
- cache_expiry = timezone.now() + timedelta(seconds=cache_ttl) if cache_ttl else None
+ try:
+ cache_ttl = cache.ttl(self.get_cache_key(obj, "vlans"))
+ except AttributeError:
+ cache_ttl = None
+ cache_expiry = timezone.now() + timedelta(seconds=cache_ttl) if cache_ttl else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/base/vlan_table_view.py` around lines 107 - 108,
The call to cache.ttl(...) is not portable across Django cache backends; update
the assignment where cache_ttl is computed (e.g., cache_ttl =
cache.ttl(self.get_cache_key(obj, "vlans"))) to call cache.ttl inside a
try/except AttributeError block and set cache_ttl = None (or a sensible default)
when AttributeError is raised, optionally logging a debug message; apply the
same defensive change to the other usages of cache.ttl in the codebase so all
occurrences (the lines that call cache.ttl with self.get_cache_key(...) or
similar keys) follow this pattern.
Comment on lines
30
to
35
| def get_context_data(self, **kwargs): | ||
| """Return context with filter form for site-location sync.""" | ||
| context = super().get_context_data(**kwargs) | ||
| queryset = self.get_queryset() | ||
| context["filter_form"] = self.filterset(self.request.GET, queryset=queryset).form | ||
| return context |
There was a problem hiding this comment.
get_context_data calls self.get_queryset() explicitly, causing a double LibreNMS API call per request.
django_tables2.SingleTableView.get() stores self.object_list = self.get_queryset() before calling get_context_data. The explicit queryset = self.get_queryset() on line 33 triggers a second call to get_librenms_locations() (a remote HTTP request). Use self.object_list instead if already set, or cache the result in an instance variable.
⚡ Proposed fix
def get_context_data(self, **kwargs):
"""Return context with filter form for site-location sync."""
context = super().get_context_data(**kwargs)
- queryset = self.get_queryset()
+ queryset = getattr(self, "object_list", None) or self.get_queryset()
context["filter_form"] = self.filterset(self.request.GET, queryset=queryset).form
return context🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/locations.py` around lines 30 - 35,
get_context_data is calling self.get_queryset() again causing a duplicate remote
call to get_librenms_locations; change it to reuse the already-populated
self.object_list (set by django_tables2.SingleTableView.get()) or cache the
queryset on the instance. Concretely, inside get_context_data replace the direct
call to self.get_queryset() with something like: use self.object_list if it
exists, otherwise call self.get_queryset() once and assign it to
self.object_list (or an instance variable) before creating the filter form
(referencing get_context_data, get_queryset, and get_librenms_locations to
locate the code).
Comment on lines
+51
to
+54
| ### VLAN Sync | ||
| - Create VLAN objects in NetBox from LibreNMS device VLAN data | ||
| - Per-VLAN group assignment with scope-aware auto-selection | ||
|
|
There was a problem hiding this comment.
Add a blank line after the heading to satisfy markdownlint MD022.
Line 51 should be followed by a blank line before the list.
📝 Proposed fix
### VLAN Sync
+
- Create VLAN objects in NetBox from LibreNMS device VLAN data
- Per-VLAN group assignment with scope-aware auto-selection📝 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
| ### VLAN Sync | |
| - Create VLAN objects in NetBox from LibreNMS device VLAN data | |
| - Per-VLAN group assignment with scope-aware auto-selection | |
| ### VLAN Sync | |
| - Create VLAN objects in NetBox from LibreNMS device VLAN data | |
| - Per-VLAN group assignment with scope-aware auto-selection | |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 51 - 54, Add a blank line immediately after the "###
VLAN Sync" heading to satisfy markdownlint rule MD022; edit the README content
around the "### VLAN Sync" header (the heading string "### VLAN Sync") so there
is an empty line between the heading and the subsequent list items.
Comment on lines
+154
to
+155
| interface_name = record.get(self.interface_name_field, "") | ||
| safe_name = interface_name.replace("/", "_").replace(":", "_") |
There was a problem hiding this comment.
safe_name replacement may miss other special characters in interface names.
Only / and : are replaced. Interface names can contain dots (e.g., Gi0/0/0.100), spaces, or other characters that are invalid in HTML name attributes or could break CSS selectors. Consider a more comprehensive sanitization.
Proposed fix
- safe_name = interface_name.replace("/", "_").replace(":", "_")
+ import re
+ safe_name = re.sub(r'[^a-zA-Z0-9_-]', '_', interface_name)📝 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
| interface_name = record.get(self.interface_name_field, "") | |
| safe_name = interface_name.replace("/", "_").replace(":", "_") | |
| interface_name = record.get(self.interface_name_field, "") | |
| import re | |
| safe_name = re.sub(r'[^a-zA-Z0-9_-]', '_', interface_name) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/tables/interfaces.py` around lines 154 - 155, The
current sanitization of interface_name into safe_name only replaces "/" and ":"
and can miss dots, spaces, and other problematic characters; update the logic
where interface_name is read (record.get(self.interface_name_field, "")) and
safe_name is computed so it normalizes more comprehensively by replacing any
character not allowed in HTML name attributes (e.g., anything other than
[A-Za-z0-9_.-]) with an underscore and collapsing repeated underscores, and
ensure the result never starts with a digit if that matters for downstream
CSS/JS consumers; locate the safe_name assignment and replace the simple
.replace calls with a regex-based whitelist-replacement approach and optional
trimming/collapse of underscores to produce a consistent, safe identifier.
Comment on lines
+128
to
+141
| options = ['<option value="">-- No Group (Global) --</option>'] | ||
| for group in self.vlan_groups: | ||
| selected = "selected" if group.pk == selected_group_id else "" | ||
| scope_info = f" ({group.scope})" if group.scope else "" | ||
| options.append(f'<option value="{group.pk}" {selected}>{group.name}{scope_info}</option>') | ||
|
|
||
| select_html = format_html( | ||
| '<select name="vlan_group_{}" class="form-select form-select-sm vlan-sync-group-select"' | ||
| ' data-vlan-id="{}" data-vlan-name="{}" style="min-width: 180px;">{}</select>', | ||
| vlan_id, | ||
| vlan_id, | ||
| record.get("name", ""), | ||
| mark_safe("".join(options)), | ||
| ) |
There was a problem hiding this comment.
Escape VLAN group labels before marking HTML as safe
group.name and group.scope are interpolated into raw <option> strings and then passed through mark_safe, which allows stored XSS payloads to render.
🔒 Suggested fix
- options = ['<option value="">-- No Group (Global) --</option>']
- for group in self.vlan_groups:
- selected = "selected" if group.pk == selected_group_id else ""
- scope_info = f" ({group.scope})" if group.scope else ""
- options.append(f'<option value="{group.pk}" {selected}>{group.name}{scope_info}</option>')
+ options = [format_html('<option value="">{}</option>', "-- No Group (Global) --")]
+ for group in self.vlan_groups:
+ scope_info = f" ({group.scope})" if group.scope else ""
+ if group.pk == selected_group_id:
+ options.append(
+ format_html(
+ '<option value="{}" selected>{}{}</option>',
+ group.pk,
+ group.name,
+ scope_info,
+ )
+ )
+ else:
+ options.append(
+ format_html(
+ '<option value="{}">{}{}</option>',
+ group.pk,
+ group.name,
+ scope_info,
+ )
+ )
@@
- mark_safe("".join(options)),
+ mark_safe("".join(str(o) for o in options)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/tables/vlans.py` around lines 128 - 141, The option
labels are built by concatenating raw group.name and group.scope into strings
and then mark_safe is used, which can allow XSS; update the loop that builds
options (the code referencing vlan_groups, selected_group_id, options,
group.name, group.scope) to escape group.name and group.scope before inserting
into the option HTML (or use Django's format_html to build each option safely)
and only mark the fully-assembled safe HTML once all parts have been
escaped/constructed; ensure the select_html call (format_html(...,
mark_safe("".join(options)), ...)) receives already-escaped option content
instead of raw group values.
Comment on lines
+765
to
+779
| def post(self, request): | ||
| """Persist a user preference toggle value.""" | ||
| try: | ||
| data = json.loads(request.body) | ||
| except (json.JSONDecodeError, ValueError): | ||
| return JsonResponse({"error": "Invalid JSON"}, status=400) | ||
|
|
||
| key = data.get("key") | ||
| value = data.get("value") | ||
|
|
||
| if key not in self.ALLOWED_PREFS: | ||
| return JsonResponse({"error": "Invalid preference key"}, status=400) | ||
|
|
||
| save_user_pref(request, self.ALLOWED_PREFS[key], value) | ||
| return JsonResponse({"status": "ok"}) |
There was a problem hiding this comment.
Enforce write permission on preference persistence endpoint
SaveUserPrefView mutates persisted user config but does not call require_write_permission_json(), so view-only users can still perform writes through this endpoint.
🔐 Suggested fix
def post(self, request):
"""Persist a user preference toggle value."""
+ if error := self.require_write_permission_json():
+ return error
+
try:
data = json.loads(request.body)
except (json.JSONDecodeError, ValueError):
return JsonResponse({"error": "Invalid JSON"}, status=400)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/imports/actions.py` around lines 765 - 779, The
POST handler in SaveUserPrefView (post method) performs a write by calling
save_user_pref but does not enforce write permissions; call
require_write_permission_json(request) at the start of post (before parsing
input and before save_user_pref) to block view-only users, keeping the existing
validation of key in ALLOWED_PREFS and the JsonResponse error flows intact.
| require_https=request.is_secure(), | ||
| ): | ||
| return referrer | ||
| return getattr(request, "path", "/") |
There was a problem hiding this comment.
Avoid redirect fallback to the current POST path
When HTTP_REFERER is missing, falling back to request.path can send users to a POST-only endpoint and produce a 405 on redirect. Use a safe GET route fallback instead.
Proposed fix
- return getattr(request, "path", "/")
+ # Avoid redirecting to action endpoints (often POST-only)
+ return "/"📝 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
| return getattr(request, "path", "/") | |
| # Avoid redirecting to action endpoints (often POST-only) | |
| return "/" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/mixins.py` at line 25, The current code returns
getattr(request, "path", "/") which falls back to the current POST path when
HTTP_REFERER is missing; change the logic to first read
request.META.get("HTTP_REFERER") and only if that is empty fall back to a safe
GET route (e.g. reverse("home") or settings.LOGIN_REDIRECT_URL or "/") instead
of request.path; update the function that returns getattr(request, "path", "/")
to use this HTTP_REFERER-first, safe-GET fallback approach so redirects never
target a POST-only endpoint.
Comment on lines
+597
to
+599
| # Fallback: first matching VLAN | ||
| vlans = vid_to_vlans.get(vid, []) | ||
| return vlans[0] if vlans else None |
There was a problem hiding this comment.
Do not silently choose an arbitrary VLAN on ambiguous VID matches
If a VID exists in multiple VLANs and no explicit group is selected, returning the first match can assign the wrong VLAN nondeterministically.
Proposed fix
- # Fallback: first matching VLAN
- vlans = vid_to_vlans.get(vid, [])
- return vlans[0] if vlans else None
+ # Fallback: only auto-select when unambiguous
+ vlans = vid_to_vlans.get(vid, [])
+ if len(vlans) == 1:
+ return vlans[0]
+ return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/mixins.py` around lines 597 - 599, The code
currently falls back to selecting vlans[0] from vid_to_vlans for an ambiguous
VID which can nondeterministically pick the wrong VLAN; change the fallback so
that if vid_to_vlans.get(vid) yields more than one VLAN you do NOT return the
first match but instead treat it as ambiguous (return None or raise an explicit
error) and optionally log or record the ambiguity; update the logic around
vid_to_vlans, vlans and the return so only a single unambiguous VLAN is returned
(i.e., return the VLAN only when len(vlans) == 1).
Comment on lines
24
to
29
| def get_object(self, object_id): | ||
| """Return the Device or VirtualMachine for the given ID.""" | ||
| try: | ||
| return Device.objects.get(pk=object_id) | ||
| except Device.DoesNotExist: | ||
| return VirtualMachine.objects.get(pk=object_id) |
There was a problem hiding this comment.
Unhandled VirtualMachine.DoesNotExist will result in a 500 error.
If object_id doesn't match any Device or VirtualMachine, the VirtualMachine.DoesNotExist exception propagates unhandled. Consider using get_object_or_404 for the fallback lookup to return a proper 404 response.
🐛 Proposed fix
def get_object(self, object_id):
"""Return the Device or VirtualMachine for the given ID."""
try:
return Device.objects.get(pk=object_id)
except Device.DoesNotExist:
- return VirtualMachine.objects.get(pk=object_id)
+ return get_object_or_404(VirtualMachine, pk=object_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/devices.py` around lines 24 - 29, The
get_object method currently catches Device.DoesNotExist but lets
VirtualMachine.DoesNotExist propagate, causing a 500; update get_object (the
Device/VirtualMachine lookup) to use Django's get_object_or_404 for the fallback
VirtualMachine lookup (or wrap the second lookup in a try/except that raises
Http404) so that when neither Device nor VirtualMachine exists a proper 404 is
returned instead of an unhandled exception.
Comment on lines
167
to
175
| interface.enabled = ( | ||
| True | ||
| if librenms_interface["ifAdminStatus"] is None | ||
| if librenms_interface.get("ifAdminStatus") is None | ||
| else ( | ||
| librenms_interface["ifAdminStatus"].lower() == "up" | ||
| if isinstance(librenms_interface["ifAdminStatus"], str) | ||
| else bool(librenms_interface["ifAdminStatus"]) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Do not default enabled=True when admin status is missing
When LibreNMS omits ifAdminStatus, this logic overwrites existing NetBox state to enabled. Preserve current value unless status is present.
Proposed fix
- if "enabled" not in exclude_columns:
- interface.enabled = (
- True
- if librenms_interface.get("ifAdminStatus") is None
- else (
- librenms_interface["ifAdminStatus"].lower() == "up"
- if isinstance(librenms_interface["ifAdminStatus"], str)
- else bool(librenms_interface["ifAdminStatus"])
- )
- )
+ if "enabled" not in exclude_columns:
+ admin_status = librenms_interface.get("ifAdminStatus")
+ if admin_status is not None:
+ interface.enabled = (
+ admin_status.lower() == "up"
+ if isinstance(admin_status, str)
+ else bool(admin_status)
+ )📝 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
| interface.enabled = ( | |
| True | |
| if librenms_interface["ifAdminStatus"] is None | |
| if librenms_interface.get("ifAdminStatus") is None | |
| else ( | |
| librenms_interface["ifAdminStatus"].lower() == "up" | |
| if isinstance(librenms_interface["ifAdminStatus"], str) | |
| else bool(librenms_interface["ifAdminStatus"]) | |
| ) | |
| ) | |
| admin_status = librenms_interface.get("ifAdminStatus") | |
| if admin_status is not None: | |
| interface.enabled = ( | |
| admin_status.lower() == "up" | |
| if isinstance(admin_status, str) | |
| else bool(admin_status) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 167 - 175, The
current assignment to interface.enabled uses
librenms_interface.get("ifAdminStatus") being None to default to True, which
overwrites NetBox state when LibreNMS omits the field; change the logic so you
only set interface.enabled when ifAdminStatus is present (and not None) in
librenms_interface: check presence with librenms_interface.get("ifAdminStatus")
is not None (or "ifAdminStatus" in librenms_interface and
librenms_interface["ifAdminStatus"] is not None) and then set interface.enabled
to librenms_interface["ifAdminStatus"].lower() == "up" if it's a str else
bool(librenms_interface["ifAdminStatus"]); otherwise leave interface.enabled
untouched.
Comment on lines
+267
to
+287
| safe_name = interface_name.replace("/", "_").replace(":", "_") | ||
|
|
||
| # Build VLAN data from port | ||
| vlan_data = { | ||
| "untagged_vlan": librenms_port.get("untagged_vlan"), | ||
| "tagged_vlans": librenms_port.get("tagged_vlans", []), | ||
| } | ||
|
|
||
| class DeleteNetBoxInterfacesView(CacheMixin, View): | ||
| # Build per-VLAN group map from POST data | ||
| vlan_group_map = {} | ||
| all_vids = [] | ||
| if vlan_data["untagged_vlan"]: | ||
| all_vids.append(str(vlan_data["untagged_vlan"])) | ||
| for vid in vlan_data.get("tagged_vlans", []): | ||
| all_vids.append(str(vid)) | ||
|
|
||
| for vid in all_vids: | ||
| group_id = self.request.POST.get(f"vlan_group_{safe_name}_{vid}", "") | ||
| if group_id: | ||
| vlan_group_map[vid] = group_id | ||
|
|
There was a problem hiding this comment.
Form key normalization for interface names is collision-prone
Replacing only / and : can map different interface names to the same key, causing VLAN group selections to bleed across interfaces.
Proposed fix
- # Get per-VLAN group selections from form (safely handle special chars in name)
- safe_name = interface_name.replace("/", "_").replace(":", "_")
+ # Prefer stable unique key to avoid collisions between interface names
+ interface_key = str(librenms_port.get("port_id") or interface_name)
...
- group_id = self.request.POST.get(f"vlan_group_{safe_name}_{vid}", "")
+ group_id = self.request.POST.get(f"vlan_group_{interface_key}_{vid}", "")Based on learnings: keep server responses and HTMX targets in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 267 - 287, The
current safe_name normalization (safe_name = interface_name.replace("/",
"_").replace(":", "_")) can produce collisions and cause VLAN group selections
to bleed across interfaces; update the form key strategy used when reading POST
values (the f"vlan_group_{safe_name}_{vid}" lookup in the loop that builds
vlan_group_map) so keys are uniquely derived from interface identifiers—for
example, generate and use a deterministic, collision-resistant token per
interface (e.g., an encoded or hashed interface_name like sha256/interface_id)
both when rendering the form and when reading POST (so safe_name is replaced by
that token), and update any code that constructs the POST keys to use the same
token (touch points: safe_name, interface_name, vlan_group_map population and
the request.POST lookup f"vlan_group_{...}_{vid}").
Comment on lines
364
to
366
| except Exception as exc: # pragma: no cover - defensive | ||
| errors.append(f"Error deleting interface {interface_name}: {str(exc)}") | ||
| errors.append(f"Error deleting interface {interface_name or interface_id}: {str(exc)}") | ||
| continue |
There was a problem hiding this comment.
Avoid returning raw exception text in API responses
Including str(exc) in client-facing errors can leak internal details. Return a generic message and log the exception server-side.
Proposed fix
- except Exception as exc: # pragma: no cover - defensive
- errors.append(f"Error deleting interface {interface_name or interface_id}: {str(exc)}")
+ except Exception: # pragma: no cover - defensive
+ errors.append(f"Error deleting interface {interface_name or interface_id}")📝 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
| except Exception as exc: # pragma: no cover - defensive | |
| errors.append(f"Error deleting interface {interface_name}: {str(exc)}") | |
| errors.append(f"Error deleting interface {interface_name or interface_id}: {str(exc)}") | |
| continue | |
| except Exception: # pragma: no cover - defensive | |
| errors.append(f"Error deleting interface {interface_name or interface_id}") | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/interfaces.py` around lines 364 - 366, The
except block that appends errors.append(f"Error deleting interface
{interface_name or interface_id}: {str(exc)}") exposes raw exception text;
change it to append a generic client-safe message (e.g., errors.append(f"Error
deleting interface {interface_name or interface_id}: internal server error"))
and log the actual exception server-side using the module logger (or
processLogger) with exc_info=True so the stack trace is preserved for debugging;
update the except block around the deletion logic (where errors, interface_name,
interface_id are used) to remove str(exc) from responses and call
logger.error(...) with the exception.
Comment on lines
+48
to
+53
| def get_object(self, object_type: str, object_id: int): | ||
| """Get the target object (Device or VM).""" | ||
| if object_type == "device": | ||
| return get_object_or_404(Device, pk=object_id) | ||
| raise Http404("Invalid object type.") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find URL configuration files that might route SyncVLANsView
fd -e py -path '*/urls.py' | head -20Repository: marcinpsk/netbox-librenms-plugin
Length of output: 244
🏁 Script executed:
# Search for SyncVLANsView routing and definition
rg -n 'SyncVLANsView' --type=pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 417
🏁 Script executed:
# Look for the full vlans.py file and its class structure
find . -name 'vlans.py' -type f | xargs wc -lRepository: marcinpsk/netbox-librenms-plugin
Length of output: 185
🏁 Script executed:
# Get the full vlans.py view file
find . -name 'vlans.py' -type f -path '*/sync/*' | head -1 | xargs catRepository: marcinpsk/netbox-librenms-plugin
Length of output: 6093
🏁 Script executed:
# Get the full urls.py to see how SyncVLANsView is routed
cat netbox_librenms_plugin/urls.pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 9453
🏁 Script executed:
# Check how SyncInterfacesView handles object_type to see if there's a pattern
sed -n '/class SyncInterfacesView/,/^class /p' netbox_librenms_plugin/views/sync/interfaces.py | head -100Repository: marcinpsk/netbox-librenms-plugin
Length of output: 4368
🏁 Script executed:
# Check how SyncIPAddressesView handles object_type
sed -n '/class SyncIPAddressesView/,/^class /p' netbox_librenms_plugin/views/sync/ipaddresses.py | head -100Repository: marcinpsk/netbox-librenms-plugin
Length of output: 167
🏁 Script executed:
# Find where SyncVLANsView is invoked/called from (templates, forms)
rg -n 'sync-vlans' --type-listRepository: marcinpsk/netbox-librenms-plugin
Length of output: 465
🏁 Script executed:
# Search for sync-vlans in all file types
rg -n 'sync-vlans'Repository: marcinpsk/netbox-librenms-plugin
Length of output: 167
🏁 Script executed:
# Check SyncIPAddressesView location
find . -name '*.py' -path '*/sync/*' | head -20Repository: marcinpsk/netbox-librenms-plugin
Length of output: 465
🏁 Script executed:
# Look for how object_type is determined (likely "device" vs "virtualmachine")
rg -n 'object_type.*device|object_type.*vm' --type=py -A 2Repository: marcinpsk/netbox-librenms-plugin
Length of output: 3885
🏁 Script executed:
# Check what object_type values are actually used in templates and how URLs are built
rg -n 'sync-vlans' -B 2 -A 2 | head -50Repository: marcinpsk/netbox-librenms-plugin
Length of output: 393
🏁 Script executed:
# Look at templates to see how sync-vlans URL is invoked
find . -name '*.html' -o -name '*.jinja*' | xargs grep -l 'sync-vlans' 2>/dev/nullRepository: marcinpsk/netbox-librenms-plugin
Length of output: 60
🏁 Script executed:
# Check the base IP addresses view pattern
sed -n '310,330p' netbox_librenms_plugin/views/base/ip_addresses_view.pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 935
🏁 Script executed:
# Search for where sync-vlans URL is invoked/built in Python code
rg -n 'sync_selected_vlans|sync-vlans' --type=py -B 2 -A 2Repository: marcinpsk/netbox-librenms-plugin
Length of output: 504
🏁 Script executed:
# Check if there's a template that builds the sync-vlans URL
find . -type f \( -name '*.html' -o -name '*.jinja' \) 2>/dev/null | head -10Repository: marcinpsk/netbox-librenms-plugin
Length of output: 895
🏁 Script executed:
# Look for reverse() calls to sync_selected_vlans
rg -n 'sync_selected_vlans' --type=pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 146
🏁 Script executed:
# Check the VLAN sync template to see how the URL is invoked
cat netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync.htmlRepository: marcinpsk/netbox-librenms-plugin
Length of output: 1161
🏁 Script executed:
# Also check the content template
cat netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlRepository: marcinpsk/netbox-librenms-plugin
Length of output: 2630
SyncVLANsView.get_object() rejects VM object types despite _redirect() handling them
The get_object() method only accepts object_type == "device" and raises Http404 for all others, but _redirect() explicitly handles VM routes via "plugins:netbox_librenms_plugin:vm_librenms_sync". While the template currently only exposes VLAN sync for devices, the URL pattern and form action use object_type=model_name (which would be "virtualmachine" for VMs), creating a functional mismatch. If a VM object reaches this view—either through the form or API—it will hard-fail with 404 instead of gracefully redirecting.
Update get_object() to handle "virtualmachine" object type, matching the pattern used in SyncInterfacesView:
Suggested fix
from dcim.models import Device
+from virtualization.models import VirtualMachine
def get_object(self, object_type: str, object_id: int):
"""Get the target object (Device or VM)."""
if object_type == "device":
return get_object_or_404(Device, pk=object_id)
+ if object_type == "virtualmachine":
+ return get_object_or_404(VirtualMachine, pk=object_id)
raise Http404("Invalid object type.")📝 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
| def get_object(self, object_type: str, object_id: int): | |
| """Get the target object (Device or VM).""" | |
| if object_type == "device": | |
| return get_object_or_404(Device, pk=object_id) | |
| raise Http404("Invalid object type.") | |
| from dcim.models import Device | |
| from virtualization.models import VirtualMachine | |
| def get_object(self, object_type: str, object_id: int): | |
| """Get the target object (Device or VM).""" | |
| if object_type == "device": | |
| return get_object_or_404(Device, pk=object_id) | |
| if object_type == "virtualmachine": | |
| return get_object_or_404(VirtualMachine, pk=object_id) | |
| raise Http404("Invalid object type.") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/sync/vlans.py` around lines 48 - 53, The
get_object method in SyncVLANsView currently only accepts object_type ==
"device" and raises Http404 for others; update SyncVLANsView.get_object to also
accept "virtualmachine" (matching SyncInterfacesView/_redirect behavior) by
returning get_object_or_404(VirtualMachine, pk=object_id) when object_type ==
"virtualmachine" and ensure VirtualMachine is imported; keep the existing device
branch and only raise Http404 for unrecognized object_type values.
marcinpsk
added a commit
that referenced
this pull request
Mar 29, 2026
- forms.py: ModuleBayMappingFilterForm.is_regex changed to NullBooleanField with tri-state select (allows 'not set' filter) - views/base/modules_view.py: add inv_serials.add(serial) after backfilling entPhysicalSerialNum to keep dedup set consistent (#6) - views/object_sync/devices.py: set request on DeviceIPAddressTableView instance before calling get_context_data, matching other context methods (#8b) - tests/test_coverage_forms.py: patch _get_librenms_poller_group_choices in AddToLibreSNMPV3 tests to avoid network calls (#4) - tests/test_sync_modules.py: assert messages.success called before cache.delete assertion in Install{Module,Branch,Selected}View tests to confirm success path ran (#5) - tests/test_coverage_devices.py: assert status_code==200 and exact css_class value in vlan_group tests; assert child_instance.request is request in ip_context test (#8a, #8b)
marcinpsk
added a commit
that referenced
this pull request
May 4, 2026
- 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
marcinpsk
added a commit
that referenced
this pull request
May 7, 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: 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.
---------
Co-authored-by: Andy Norwood <2754635+bonzo81@users.noreply.github.com>
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…
marcinpsk
added a commit
that referenced
this pull request
Jun 1, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
marcinpsk
added a commit
that referenced
this pull request
Jun 1, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
marcinpsk
added a commit
that referenced
this pull request
Jun 1, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
marcinpsk
added a commit
that referenced
this pull request
Jun 1, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
marcinpsk
added a commit
that referenced
this pull request
Jun 2, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Briefly describe what this PR does in plain English, and provide as much of the following information as possible.
Motivation / Problem
What issue does this solve?
Link any related issues if applicable.
Scope of Change
Tick all that apply:
How Was This Tested?
Tick all that apply and describe briefly.
Manual Test Steps (if applicable)
Risk Assessment
Explain briefly.
Backwards Compatibility
Other Notes
Anything the maintainer(s) should pay particular attention to?
Summary by CodeRabbit
New Features
Improvements