Feat/oob sync#79
Open
marcinpsk wants to merge 105 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR adds OOB controller detection, OOB-aware import validation and actions, migrated-donor move-to-winner flows, server-key-scoped sync handling across views, updated sync and import UI states, collision detection for bulk import, and matching tests and documentation. ChangesOOB import, sync, and migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/utils.py (1)
875-908:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer host-ID matches before OOB-ID matches.
This query now treats
librenms_id[server_key].id == Xandlibrenms_id[server_key].oob.id == Xas the same lookup and then returns.first(). If one NetBox row owns host ID42and a different row stores OOB ID42, the selected object is DB-order dependent.Suggested fix
- q = Q(**{f"custom_field_data__librenms_id__{server_key}": librenms_id}) - # Also match when the namespaced value was stored as a string (e.g. {"production": "42"}). - q |= Q(**{f"custom_field_data__librenms_id__{server_key}": str(librenms_id)}) - # Match when value stored as {"id": librenms_id, "oob": {...}} — new dict-with-id form. - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": librenms_id}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": str(librenms_id)}) - # Match when librenms_id is the OOB device id — so re-import recognises merged device. - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": librenms_id}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": str(librenms_id)}) + host_q = Q(**{f"custom_field_data__librenms_id__{server_key}": librenms_id}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}": str(librenms_id)}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": librenms_id}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": str(librenms_id)}) @@ - q |= Q(**{f"custom_field_data__librenms_id__{server_key}": canonical_str}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}": int_value}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": canonical_str}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": int_value}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": canonical_str}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": int_value}) - q |= Q(custom_field_data__librenms_id=canonical_str) - q |= Q(custom_field_data__librenms_id=int_value) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}": canonical_str}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}": int_value}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": canonical_str}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": int_value}) + host_q |= Q(custom_field_data__librenms_id=canonical_str) + host_q |= Q(custom_field_data__librenms_id=int_value) - return model.objects.filter(q).first() + host_match = model.objects.filter(host_q).first() + if host_match: + return host_match + + oob_q = Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": librenms_id}) + oob_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": str(librenms_id)}) + if isinstance(librenms_id, str): + cleaned = librenms_id.strip() + try: + int_value = int(cleaned) + if int_value > 0: + canonical_str = str(int_value) + oob_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": canonical_str}) + oob_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": int_value}) + except ValueError: + pass + return model.objects.filter(oob_q).first()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/utils.py` around lines 875 - 908, The current combined Q (variable q) treats host-ID (__id) and OOB-ID (__oob__id) matches equally and returns model.objects.filter(q).first(), making the result DB-order dependent when different rows match host vs OOB; split the logic into two prioritized queries: build a host_q containing all host-ID forms (namespaced __{server_key} and legacy bare values, including the canonicalized numeric/string variants) and an oob_q containing only the OOB-ID forms, then return model.objects.filter(host_q).first() if present, otherwise model.objects.filter(oob_q).first(); keep the existing canonicalization and legacy fallbacks but move legacy bare-ID checks into host_q so host-ID matches are always preferred over OOB matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/usage_tips/module_sync.md`:
- Line 38: The docs use two different labels for the same carrier-action —
"Suggest Carrier" here versus "Install Carrier" elsewhere — which is confusing;
pick one canonical label (e.g., "Install Carrier") and update this line in
module_sync.md along with any references to the CarrierAutoInstallRule, the
mapping rules guide, and any UI/button label text so all occurrences use the
chosen term consistently.
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 257-264: Add a default "serial_role_choice_available": False entry
to the base result dict (the same dict that contains "serial_action",
"serial_confirmed", "serial_duplicate", etc.) so the function
(device_operations.py where that result is built/returned) always returns a
stable schema; ensure this key is initialized alongside the other defaults and
not only set inside the single serial-match branch.
- Around line 43-77: _describe_existing_librenms_link currently only accepts
literal ints for "id" and nested oob "id", so dicts like
{"id":"42","oob":{"id":"7"}} are treated as unlinked; change the strict
isinstance(int) checks to normalize using the existing coerce_librenms_id helper
(the same normalisation used by find_by_librenms_id/get_librenms_device_id) —
call coerce_librenms_id(entry.get("id")) and coerce_librenms_id(oob.get("id"))
and if the result is not None (and not a bool) assign
info["host_id"]/info["oob_id"] accordingly while leaving the oob_type string
check as-is.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js`:
- Around line 354-357: The client is sending the preference key
'auto_create_ipam' from librenms_import.js via savePref(...) but the backend
rejects it because SaveUserPrefView.ALLOWED_PREFS does not include that key;
update SaveUserPrefView.ALLOWED_PREFS to include 'auto_create_ipam' (or
alternatively change the client to an existing allowed key) so the POST from the
handler tied to document.getElementById('auto-create-ipam-toggle') (and the
savePref call) will be accepted and persist.
In `@netbox_librenms_plugin/tables/device_status.py`:
- Around line 520-534: The code casts paired_oob_id to int directly when
building btn_title (in the branch where match_type == "librenms_id") which can
raise and break rendering; change this to safely coerce/validate paired_oob_id
before interpolating—e.g., check isdigit()/numeric or wrap int(paired_oob_id) in
a try/except and fall back to a safe placeholder or the original escaped value
on error, then use that safe value when constructing btn_title (leave
escape(paired_oob_type or '') as-is).
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html`:
- Line 343: The hx-vals for the move action is using a fallback literal
"default" which can choose the wrong server context; change the hx-vals to
always post the migration marker's server_key (use "{{ interface_sync.server_key
}}") so the view can use CacheMixin.get_cache_key() to load server-scoped cache
correctly for the move action; update the template fragment containing
hx-vals='{"server_key": "{{ interface_sync.server_key|default:"default" }}"}' to
stop using the "default" fallback and send the actual interface_sync.server_key
value.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html`:
- Around line 12-15: The HTMX form posting to the add_device_type_mapping view
omits the auto-create IPAM toggle from its hx-include list, so include the
element id "`#auto-create-ipam-toggle`" in the hx-include attribute of the form
(the form with hx-post="{% url
'plugins:netbox_librenms_plugin:add_device_type_mapping'
device_id=libre_device.device_id %}" and class "mt-1") so the current
auto-create IPAM preference is sent with role_{{ libre_device.device_id }},
rack_{{ libre_device.device_id }}, cluster_{{ libre_device.device_id }},
`#use-sysname-toggle` and `#strip-domain-toggle` during mapping revalidation/update
flows.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html`:
- Around line 10-13: The HTMX form that posts to the
'plugins:netbox_librenms_plugin:add_platform_mapping' endpoint is missing the
auto-create IPAM toggle in its hx-include list; update the form element (the
<form> with hx-post and hx-include currently listing [name=role_{{
libre_device.device_id }}], [name=rack_{{ libre_device.device_id }}],
[name=cluster_{{ libre_device.device_id }}], `#use-sysname-toggle`,
`#strip-domain-toggle`) to also include the selector `#auto-create-ipam-toggle` so
the current import preference is submitted with platform-mapping updates.
In `@netbox_librenms_plugin/tests/test_import_utils.py`:
- Around line 1760-1763: The test currently asserts result["serial_action"],
result["existing_match_type"], and that result.get("oob_candidate") is not None
but misses the promised second payload; update the test to also assert that the
role-choice payload "promote_to_host" is present (e.g., assert
result.get("promote_to_host") is not None) so the contract exposing both
oob_candidate and promote_to_host is enforced (locate the assertion block that
references result in the failing test in tests/test_import_utils.py).
In `@netbox_librenms_plugin/utils.py`:
- Around line 658-675: coerce_librenms_id currently accepts 0 and negative
integers (and string-digit equivalents) as valid IDs which conflicts with
callers like _normalize_librenms_id that treat <= 0 as invalid; update
coerce_librenms_id so that after handling bools it only returns a positive int
(> 0): for an int return it only if value > 0 else None, and for a str convert
to int and return it only if the parsed value > 0, otherwise return None (keep
existing bool-rejection behavior intact).
- Around line 971-995: Validate oob_device_id before storing by ensuring it is
an integer (not a bool) and a positive value consistent with the main device-ID
rules; in the block that constructs oob = {"id": int(oob_device_id), "type":
normalized_type} (and where entry["oob"] is set), first check
isinstance(oob_device_id, int) and not isinstance(oob_device_id, bool) and
oob_device_id > 0 (or coerce safely then validate), and if the check fails log a
warning via logger and skip setting entry["oob"] (or raise ValueError) so we
don't persist boolean/zero/negative IDs that later match real devices.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 300-307: The loop that applies _OOB_OFFSET to entPhysicalIndex and
entPhysicalContainedIn should coerce those fields to integers before doing
arithmetic and before comparing to 0: in the for-loop over oob_inventory (the
block referencing _OOB_OFFSET, item["entPhysicalIndex"], and
item["entPhysicalContainedIn"]), parse item.get("entPhysicalIndex") and
item.get("entPhysicalContainedIn") with int() (or a safe int conversion), handle
non-numeric or missing values by skipping the offset or leaving the original
value, and then replace the fields with the adjusted integer values so that idx
+ _OOB_OFFSET and parent + _OOB_OFFSET never attempt string concatenation or
compare a string to 0.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 571-578: The collision branch currently returns HTMX modal HTML
with status=409 which breaks the project's 200-based HTMX pattern enforced by
_htmx_error_response; update the branch that calls detect_bulk_collisions and
render(...) (the block that assigns collisions and returns the
"netbox_librenms_plugin/htmx/bulk_import_collision.html" view) to return a 2xx
response (remove or change the status argument to 200) so the collision modal is
delivered as a normal HTMX swap instead of triggering error handling.
In `@netbox_librenms_plugin/views/sync/migrate.py`:
- Around line 136-149: The precondition checks (e.g., the
Interface.objects.filter(...).exists() winner-name check and similar
winner/donor guards used in TransferDeviceIPView) must be executed after
acquiring row locks to avoid races: move those checks inside the with
transaction.atomic() block immediately after the select_for_update() call that
locks both Device rows (the block that uses ordered = sorted({donor.pk,
winner.pk}) and Device.objects.select_for_update()), then re-run the same
existence/winner checks and abort with the same _fail response if they now fail
before performing interface.device = winner and interface.save(); apply the same
change to the other similar blocks referenced (the blocks around the other
ranges noted).
---
Outside diff comments:
In `@netbox_librenms_plugin/utils.py`:
- Around line 875-908: The current combined Q (variable q) treats host-ID (__id)
and OOB-ID (__oob__id) matches equally and returns
model.objects.filter(q).first(), making the result DB-order dependent when
different rows match host vs OOB; split the logic into two prioritized queries:
build a host_q containing all host-ID forms (namespaced __{server_key} and
legacy bare values, including the canonicalized numeric/string variants) and an
oob_q containing only the OOB-ID forms, then return
model.objects.filter(host_q).first() if present, otherwise
model.objects.filter(oob_q).first(); keep the existing canonicalization and
legacy fallbacks but move legacy bare-ID checks into host_q so host-ID matches
are always preferred over OOB matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 566bbda4-aa1d-478a-b3b9-6f7617d591eb
⛔ Files ignored due to path filters (13)
docs/img/Netbox-librenms-plugin-device-sync-fields.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-import-page.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-module-sync-tab.pngis excluded by!**/*.pngdocs/img/carrier_auto_install_rules/list.pngis excluded by!**/*.pngdocs/img/device_type_mappings/list.pngis excluded by!**/*.pngdocs/img/inventory_ignore_rules/list.pngis excluded by!**/*.pngdocs/img/module_bay_mappings/list.pngis excluded by!**/*.pngdocs/img/module_type_mappings/add.pngis excluded by!**/*.pngdocs/img/module_type_mappings/list.pngis excluded by!**/*.pngdocs/img/normalization_rules/add.pngis excluded by!**/*.pngdocs/img/normalization_rules/list.pngis excluded by!**/*.pngdocs/img/platform_mappings/add.pngis excluded by!**/*.pngdocs/img/platform_mappings/list.pngis excluded by!**/*.png
📒 Files selected for processing (49)
docs/README.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/README.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/migrate.py
marcinpsk
added a commit
that referenced
this pull request
May 22, 2026
docs: fix carrier button label 'Suggest Carrier' -> 'Install Carrier'
The tables/modules.py and carrierautoinstallrule_list.html both use
'Install Carrier'; the module_sync.md usage tip was inconsistent.
fix(device_operations): normalize string-digit IDs in dict-form CF values
_describe_existing_librenms_link() used strict isinstance(int) checks
for the new dict-form {"id": ...} path, so string-digit values like
{"id": "42"} were silently treated as unlinked. Switch to
coerce_librenms_id() which handles str->int, plus reject <= 0.
fix(device_operations): add serial_role_choice_available default to result schema
fix(actions): add auto_create_ipam to SaveUserPrefView.ALLOWED_PREFS
JS savePref('auto_create_ipam', ...) always returned 400 because the
backend whitelist didn't include the key.
fix(device_status): guard paired_oob_id int() coerce against malformed data
fix(template): use migrated_to_marker.server_key for move-to-winner action
fix(templates): propagate auto-create-ipam toggle in mapping form HTMX posts
fix(utils): reject non-positive IDs in coerce_librenms_id()
fix(utils): validate oob_device_id before storing in set_librenms_oob_device_id
test: fix docstring for test_serial_match_diff_hostname_offers_role_choice
There was a problem hiding this comment.
Pull request overview
This PR expands the NetBox LibreNMS plugin’s import and sync workflows to better handle “host + out-of-band (OOB) controller” scenarios, adds bulk-import collision detection, and introduces optional IPAM auto-creation for LibreNMS-known IPs. It also adds Stage 2b “migrated donor” actions for incrementally moving interfaces/IPs to the merge winner, plus supporting UI, settings, and documentation updates.
Changes:
- Add OOB linking + detection across import, sync (interfaces/cables/modules), and status tables, including UI badges and role-toggle flows.
- Add Stage 2 device merge flow enhancements: bulk collision blocking and Stage 2b “move-to-winner / transfer IP” endpoints + migrated-mode UI.
- Add IPAM auto-create option (settings + per-import toggle) and supporting import utilities/tests/docs.
Reviewed changes
Copilot reviewed 49 out of 62 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| netbox_librenms_plugin/views/sync/migrate.py | New Stage 2b endpoints to move interfaces/IPs and transfer device IP FKs from donor to winner. |
| netbox_librenms_plugin/views/imports/list.py | Preserves the auto-create IPAM toggle state when loading cached background-job results. |
| netbox_librenms_plugin/views/base/modules_view.py | Merges OOB controller inventory into module sync and tags rows with _source. |
| netbox_librenms_plugin/views/base/librenms_sync_view.py | Adds migrated-donor context (migrated_to_marker/migrated_to_winner) to sync pages. |
| netbox_librenms_plugin/views/base/interfaces_view.py | Merges OOB ports into interface sync, tags _source, and flags shared-LOM MAC conflicts. |
| netbox_librenms_plugin/views/base/cables_view.py | Merges OOB LLDP links into cable sync and tags _source. |
| netbox_librenms_plugin/views/init.py | Re-exports newly added import/sync views. |
| netbox_librenms_plugin/utils.py | Adds OOB helpers, migrated marker helpers, dict-with-id parsing, collision-resistant find, and auto-create IPAM resolver. |
| netbox_librenms_plugin/urls.py | Adds routes for OOB attach/promote/merge and Stage 2b move/transfer endpoints. |
| netbox_librenms_plugin/tests/test_migrate_views.py | New tests for migrated marker + Stage 2b per-row move/transfer endpoints. |
| netbox_librenms_plugin/tests/test_librenms_id.py | Extends librenms_id tests for dict-with-id/OOB and merge/migrate helpers. |
| netbox_librenms_plugin/tests/test_ip_helpers.py | New tests for IPAM auto-create helper behavior. |
| netbox_librenms_plugin/tests/test_import_utils.py | Updates serial-match behavior expectations to the new host/OOB role choice flow. |
| netbox_librenms_plugin/tests/test_coverage_list.py | Updates coverage tests for _load_job_results(..., request=...) signature. |
| netbox_librenms_plugin/tests/test_coverage_device_operations.py | Adds coverage for OOB detection + merge-candidate detection paths. |
| netbox_librenms_plugin/tests/test_coverage_actions.py | Updates action tests for new error statuses and adds collision + sentinel regression tests. |
| netbox_librenms_plugin/tests/test_collisions.py | New unit tests for bulk collision grouping logic. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/settings.html | Adds plugin setting toggle for auto_create_ipam_default. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html | Adds migrated-donor warning banner and transfer-IP buttons. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html | Adds import-page “Auto-create IPAM” toggle and includes it in bulk actions. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html | Major import modal updates: OOB/host role toggle, merge UI, mapping shortcuts, and new fragments. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.html | Removes message-container wipe; relies on view-level OOB message attachment. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html | New modal fragment to block bulk import on NetBox-device collisions. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html | New reusable platform-mapping typeahead form (used in validation modal). |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html | New reusable device-type-mapping typeahead form (used in validation modal). |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html | Adds per-row “Move to winner” action column in migrated-donor mode. |
| netbox_librenms_plugin/tables/modules.py | Adds OOB badge rendering in module inventory rows. |
| netbox_librenms_plugin/tables/interfaces.py | Adds OOB + Shared LOM badges to interface names. |
| netbox_librenms_plugin/tables/device_status.py | Refines action button rendering for OOB-linked/paired host rows and OOB candidates. |
| netbox_librenms_plugin/tables/cables.py | Adds OOB badge rendering for local port rows. |
| netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js | Persists auto-create IPAM preference and supports in-place validation modal refresh + nested modal handling. |
| netbox_librenms_plugin/models.py | Adds auto_create_ipam_default setting field. |
| netbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.py | Migration for the new auto_create_ipam_default setting. |
| netbox_librenms_plugin/librenms_api.py | Reuses shared coerce_librenms_id() for ID normalization. |
| netbox_librenms_plugin/import_utils/vm_operations.py | Adds optional IPAM auto-create during VM import (best-effort, no primary assignment). |
| netbox_librenms_plugin/import_utils/ip_helpers.py | New helper for get-or-create of global-scope /32 or /128 IPs with opt-in. |
| netbox_librenms_plugin/import_utils/device_operations.py | Adds OOB detection/role selection + merge-candidate detection and existing link description. |
| netbox_librenms_plugin/import_utils/collisions.py | New collision grouping logic for bulk import confirmation. |
| netbox_librenms_plugin/import_utils/bulk_import.py | Propagates created_ips into bulk import success results. |
| netbox_librenms_plugin/import_utils/init.py | Re-exports IP helpers and collision detector. |
| netbox_librenms_plugin/forms.py | Adds auto_create_ipam_default form field to settings UI. |
| netbox_librenms_plugin/constants.py | Adds OOB detection constants and normalize_oob_type(). |
| docs/usage_tips/README.md | Links new mapping/module sync docs. |
| docs/usage_tips/module_sync.md | New user guide for module/inventory sync. |
| docs/usage_tips/mapping_rules.md | New mapping rules guide (platform/device type/module types/bays/etc.). |
| docs/README.md | Highlights module sync and platform mapping documentation. |
| docs/librenms_import/validation.md | Updates validation docs to reference mapping rules and platform mapping behavior. |
| docs/feature_list.md | Expands feature list to include mapping rules + module sync capabilities. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/views/sync/migrate.py (2)
151-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLock and narrow the
Interfacewrite.Only the
Devicerows are locked here; theInterfacerow is still a stale in-memory object, andinterface.save()writes every column. A concurrent rename or field edit can be silently overwritten even though this action only intends to changedevice_id.🔧 Suggested fix
with transaction.atomic(): # Lock both devices in pk order to avoid cross-merge deadlocks. ordered = sorted({donor.pk, winner.pk}) - list(Device.objects.select_for_update().filter(pk__in=ordered).order_by("pk")) + locked = { + d.pk: d for d in Device.objects.select_for_update().filter(pk__in=ordered).order_by("pk") + } + donor = locked[donor.pk] + winner = locked[winner.pk] + interface = Interface.objects.select_for_update().get(pk=interface.pk) # Re-check under the lock to close the TOCTOU window. if Interface.objects.filter(device=winner, name=interface.name).exists(): return self._fail( request, f"Winner device '{winner.name}' already has an interface named '{interface.name}'. " "Rename or remove the existing interface first.", status=409, ) interface.device = winner - interface.save() + interface.save(update_fields=["device"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/sync/migrate.py` around lines 151 - 164, You're only locking Device rows but then calling interface.save() which will overwrite all Interface columns from a stale in-memory instance; instead, re-fetch and lock the specific Interface row (e.g., with Interface.objects.select_for_update().get(pk=interface.pk) inside the same transaction) or perform an atomic queryset update to only change device_id (e.g., Interface.objects.filter(pk=interface.pk).update(device=winner)) so concurrent edits to other fields aren't clobbered; keep the existence check (Interface.objects.filter(device=winner, name=interface.name).exists()) under the same transaction/lock and return via _fail as before.
55-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hard-code
"default"as the fallback server key.If the POST omits
server_key, all three actions resolve_migrated_toin the"default"namespace, so migrations started from a non-default LibreNMS server can incorrectly fail with “Donor device is not marked as migrated.” Fall back to the active API server key instead of a literal default.🔧 Suggested fix
-def _server_key_from_request(request, default="default"): +def _server_key_from_request(request, default): """Extract the LibreNMS server key from the POST body (form field).""" sk = request.POST.get("server_key") or default return sk if isinstance(sk, str) and sk else default- server_key = _server_key_from_request(request) + server_key = _server_key_from_request(request, self.librenms_api.server_key)Based on learnings: "In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key ..."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/views/sync/migrate.py` around lines 55 - 58, The helper _server_key_from_request currently hard-codes "default" as the fallback which breaks namespace scoping; change _server_key_from_request to take a default_server_key (or default=None) parameter instead of the literal "default", use that provided default when request.POST lacks a non-empty server_key, and update callers (e.g. the migration views/actions that call _server_key_from_request) to pass self.librenms_api.server_key as the default_server_key so the active API server key is used for cache namespace scoping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/constants.py`:
- Around line 11-12: OOB_TYPES contains "oob" but OOB_TYPE_PATTERN won't match
it, so normalize_oob_type() can never return "oob"; update the regex
OOB_TYPE_PATTERN to include "oob" (e.g., add "oob" to the alternation) so that
inputs matching "oob" are normalized, or alternatively remove "oob" from
OOB_TYPES if you deliberately don't want to normalize that value; adjust
OOB_TYPE_PATTERN and verify normalize_oob_type() returns values from OOB_TYPES
accordingly.
In `@netbox_librenms_plugin/utils.py`:
- Around line 1198-1209: The code only sets winner_entry["oob"] when
OOB_TYPE_PATTERN matches donor.name, dropping donor_id when no vendor-specific
type is found; update the logic in the block that calls coerce_librenms_id
(symbols: coerce_librenms_id, OOB_TYPE_PATTERN, winner_entry["oob"], donor.name)
so that if OOB_TYPE_PATTERN.search(donor.name or "") returns no match you
fallback to inferred_type = "oob" (lowercase) and still create demoted = {"id":
_coerced_donor, "type": inferred_type} and assign winner_entry["oob"] = demoted,
preserving the donor LibreNMS id as a generic oob link.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 317-322: The current computation of main_max_idx/_OOB_OFFSET uses
only inventory_data but _merge_transceiver_data() may append synthetic rows with
entity_physical_index values, causing OOB remapped indices to collide with those
synthetic main indices; to fix, compute the max index after merging by calling
_merge_transceiver_data(inventory_data, ...) (or otherwise obtaining the merged
list) and then compute main_max_idx = max(... for item in merged_inventory) and
set _OOB_OFFSET from that value so the OOB namespace is always above both real
and synthetic main transceiver indices (references: main_max_idx, _OOB_OFFSET,
_merge_transceiver_data, inventory_data, entity_physical_index, index_map).
In `@netbox_librenms_plugin/views/sync/migrate.py`:
- Around line 228-241: The IPAddress row (`ip`) must be re-locked and
re-validated inside the existing transaction to avoid overwriting concurrent
updates: inside the transaction.atomic() block, re-fetch the IPAddress using
select_for_update() (e.g. filter(pk=ip.pk).select_for_update().first()), confirm
its current assigned_object still refers to the donor/interface (compare to the
donor's Interface or `assigned`), and if it changed return self._fail(...) with
an appropriate 409; if still valid, update only the assignment columns (set
assigned_object to `winner_iface`) and call save(update_fields=[...]) so other
fields are not clobbered. Ensure you reference `ip`, `IPAddress`, `Interface`,
`assigned`, `donor`, `winner`, and `_fail` when locating and modifying the code.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/sync/migrate.py`:
- Around line 151-164: You're only locking Device rows but then calling
interface.save() which will overwrite all Interface columns from a stale
in-memory instance; instead, re-fetch and lock the specific Interface row (e.g.,
with Interface.objects.select_for_update().get(pk=interface.pk) inside the same
transaction) or perform an atomic queryset update to only change device_id
(e.g., Interface.objects.filter(pk=interface.pk).update(device=winner)) so
concurrent edits to other fields aren't clobbered; keep the existence check
(Interface.objects.filter(device=winner, name=interface.name).exists()) under
the same transaction/lock and return via _fail as before.
- Around line 55-58: The helper _server_key_from_request currently hard-codes
"default" as the fallback which breaks namespace scoping; change
_server_key_from_request to take a default_server_key (or default=None)
parameter instead of the literal "default", use that provided default when
request.POST lacks a non-empty server_key, and update callers (e.g. the
migration views/actions that call _server_key_from_request) to pass
self.librenms_api.server_key as the default_server_key so the active API server
key is used for cache namespace scoping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e393373-a59a-49c0-bbc9-b9bbba6c23b1
📒 Files selected for processing (18)
docs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/migrate.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (python)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.14)
- GitHub Check: test-netbox (3.13)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.py
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Do not reintroducedata-bs-toggleor duplicate modal IDs in modal implementation.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.
Do not re-addtable-responsivewrappers as their removal was deliberate to prevent dropdown clipping.
Templates live intemplates/netbox_librenms_plugin/; reuse and includes go underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/**/*.{html,css}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Styling assumes Tabler defaults for the netbox_librenms_plugin frontend.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments live in
templates/netbox_librenms_plugin/htmx/including:device_import_row.html,device_validation_details.html,device_vc_details.html,bulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/modules_view.py
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/device_status.py
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/migrate.py
**/import_utils/device_operations.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
device_operations.pymust export:validate_device_for_import(device, ...)andbulk_import_devices_shared(devices, user, ...)
Files:
netbox_librenms_plugin/import_utils/device_operations.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (23)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-03-08T14:17:28.826Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/import_utils/virtual_chassis.py:73-80
Timestamp: 2026-03-08T14:17:28.826Z
Learning: In Python code, when a lookup returns None (including API failures), implement negative caching by storing an empty result with a configurable TTL (default 5 minutes). Document the TTL and ensure a force_refresh=True bypasses the cache for manual re-fetch actions. Do not treat caching None/empty results on API failure as a bug if this mirrors existing patterns (e.g., get_device_with_server caching None on not-found). Apply this guidance to files within netbox_librenms_plugin/import_utils where similar inventory/API lookups occur.
Applied to files:
netbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-03-09T20:10:48.502Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/import_utils/device_operations.py:496-575
Timestamp: 2026-03-09T20:10:48.502Z
Learning: In netbox_librenms_plugin/import_utils/device_operations.py, in validate_device_for_import(), ensure both the cluster-required blocker (VM path) and the device_role-required blocker (device path) are guarded with if not result.get('existing_device') to ensure create-time prerequisites are only appended for new imports, not for link/update flows; keep available_roles and available_clusters populated for both new and existing-device cases so UI dropdowns function correctly on update views.
Applied to files:
netbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-05-22T19:37:46.167Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_import_utils.py:1763-1766
Timestamp: 2026-05-22T19:37:46.167Z
Learning: In `validate_device_for_import` (device operations / serial-diff name-resolution logic), preserve the following result contract so the UI/test expectations remain stable:
- If `serial_action` is determined via a serial match AND the existing device has NO OOB/LibreNMS link, set `serial_action` to `"oob_candidate"` and ensure `promote_to_host` is NOT present in the returned dict.
- Populate `promote_to_host` only when the existing device already has an OOB/LibreNMS link (i.e., a host id is available to inherit from); otherwise omit the key.
- Always include `serial_role_choice_available` in the returned dict, defaulting to `False` (baseline) even when other resolution outcomes do not enable it.
Applied to files:
netbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_librenms_id.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_librenms_id.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_librenms_id.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/device_status.py
📚 Learning: 2026-03-07T10:32:06.242Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:107-117
Timestamp: 2026-03-07T10:32:06.242Z
Learning: In netbox_librenms_plugin/utils.py, keep the Priority 1 loop to guard against None and bool values when accessing raw_cf.get(server_key). Do not replace the Priority 1 condition with a full get_librenms_device_id call. The two-pass design is intentional: Priority 1 performs quick sanity checks, while Priority 2 handles string normalization and full validation by calling get_librenms_device_id(member, server_key, auto_save=False).
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-07T22:38:43.110Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:552-584
Timestamp: 2026-03-07T22:38:43.110Z
Learning: In netbox_librenms_plugin/utils.py, do not propose replacing 'obj.custom_field_data.get("librenms_id") or {}' with a None check. The code intentionally uses 'or {}' to handle falsey values; downstream type guards treat them equivalently since LibreNMS IDs start at 1, making 0 equivalent to 'not set'. Do not modify this logic; keep the existing behavior for all falsey values.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T08:55:46.317Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:532-595
Timestamp: 2026-03-08T08:55:46.317Z
Learning: In netbox_librenms_plugin/utils.py, do not modify set_librenms_device_id to call obj.save(). It is mutator-only and should only update in-memory obj.custom_field_data[...] without persisting. Ensure callers perform persistence: after mutation, run full_clean() and then save() (as seen in device_operations.py around lines ~864-866) or explicit obj.save() after set_librenms_device_id (as in librenms_api.py around lines ~261-262). This pattern prevents coupling mutation with persistence and preserves validation in between.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-09T19:15:13.104Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/utils.py:249-267
Timestamp: 2026-03-09T19:15:13.104Z
Learning: In netbox_librenms_plugin/utils.py, ensure match_librenms_hardware_to_device_type returns None when DeviceTypeMapping.MultipleObjectsReturned is raised (fail-closed per inline comment). Callers must guard for result is None separately from the normal result check (e.g., if result is None: handle; elif result.get('matched'): ... ). Note that the success path uses match_type='mapping' (not 'exact'), distinguishing it from standard part_number/model exact lookups. Consider adding a unit test that asserts None is returned on MultipleObjectsReturned and that callers properly handle both None and dict results.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-09T10:39:37.846Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:1039-1047
Timestamp: 2026-03-09T10:39:37.846Z
Learning: In netbox_librenms_plugin/tests/test_coverage_device_operations.py, fix test_no_hostname_adds_issue in both TestValidateDeviceForImportEdgeCases and TestValidateDeviceMoreEdgeCases. Do not patch _determine_device_name. Instead, call validate_device_for_import with sysName="" and hostname="" and assert that "no hostname" is NOT present in result.get("issues", []) because the fallback name (device-{id}) is used when both inputs are empty. Ensure the test directly verifies the absence of the blocker when inputs are empty, relying on the actual fallback behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_device_operations.py
📚 Learning: 2026-03-07T13:12:59.182Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:445-448
Timestamp: 2026-03-07T13:12:59.182Z
Learning: Across netbox_librenms_plugin/views/sync/*.py, verify redirects and URL rewrites do not append ?server_key=. The active server context is determined from the global setting (settings.selected_server) via BaseLibreNMSSyncView.get(), so request.GET server_key is not used to preserve context. Ensure RemoveServerMappingView and ConvertLegacyLibreNMSIdView (and similar views) rely on the global setting instead of propagating server_key in redirects.
Applied to files:
netbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
🔇 Additional comments (19)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html (1)
12-12: LGTM!Also applies to: 34-38, 45-45, 53-53, 62-65, 81-81
docs/usage_tips/module_sync.md (1)
38-38: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html (1)
14-14: LGTM!Also applies to: 36-40, 47-47, 55-55, 64-67, 83-83
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (1)
343-343: LGTM!netbox_librenms_plugin/import_utils/collisions.py (1)
84-84: LGTM!Also applies to: 89-94
netbox_librenms_plugin/tests/test_import_utils.py (1)
1733-1738: LGTM!netbox_librenms_plugin/tests/test_coverage_actions.py (1)
4191-4320: LGTM!netbox_librenms_plugin/tests/test_collisions.py (1)
6-184: LGTM!netbox_librenms_plugin/tests/test_migrate_views.py (1)
261-266: LGTM!Also applies to: 323-327
netbox_librenms_plugin/views/base/modules_view.py (1)
50-60: LGTM!netbox_librenms_plugin/tables/device_status.py (1)
532-538: LGTM!netbox_librenms_plugin/utils.py (1)
253-301: LGTM!Also applies to: 658-675, 975-989, 1005-1005, 1128-1197
netbox_librenms_plugin/tests/test_coverage_device_operations.py (1)
1956-1961: LGTM!netbox_librenms_plugin/views/sync/migrate.py (1)
30-52: LGTM!Also applies to: 306-327
netbox_librenms_plugin/tests/test_librenms_id.py (1)
564-595: LGTM!Also applies to: 662-675, 731-751
netbox_librenms_plugin/import_utils/device_operations.py (1)
63-74: LGTM!Also applies to: 261-265, 575-579
netbox_librenms_plugin/views/imports/actions.py (3)
573-582: LGTM!
2305-2328: LGTM!
2574-2574: LGTM!
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
netbox_librenms_plugin/import_utils/device_operations.py (1)
382-385:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize the stored OOB id before refining to
librenms_oob.This branch only recognizes an OOB match when the stored nested id has the same runtime type as
librenms_id. A value like{"oob": {"id": "42"}}will still match viafind_by_librenms_id()and then fall through as"librenms_id"here, which renders the wrong UI path.🔧 Minimal fix
- if _existing_oob and _existing_oob.get("id") == librenms_id: + if _existing_oob and coerce_librenms_id(_existing_oob.get("id")) == coerce_librenms_id(librenms_id): result["existing_match_type"] = "librenms_oob"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 382 - 385, The existing check in device_operations.py using get_librenms_oob(existing_device, server_key=server_key) only matches when the stored nested id and librenms_id share the same runtime type; normalize both IDs (e.g., cast to string or perform a safe int cast) before comparison so values like {"oob": {"id": "42"}} correctly match; update the conditional around _existing_oob.get("id") == librenms_id (used to set result["existing_match_type"] = "librenms_oob") to compare normalized values (reference symbols: get_librenms_oob, existing_device, server_key, librenms_id, result["existing_match_type"]) and ensure the normalization handles None/invalid values safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 263-265: The response payload currently always includes the
"promote_to_host" key set to None which changes the validation shape; change the
logic that builds the result dict in device_operations.py so that
"promote_to_host" is only added when an existing LibreNMS host link can be
inherited (i.e., when the existing device's link info contains a host id — the
same condition you use to detect an existing_librenms_link / existing_libre_id);
otherwise do not include the "promote_to_host" key at all and leave
"oob_candidate" and "existing_librenms_link" handling unchanged.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Line 119: The HTMX form omits unchecked checkboxes (so auto_create_ipam is
never sent when off); to fix, add an explicit hidden input with the same name
and a false/off value immediately before the corresponding checkbox(s) so a
value is always submitted (e.g. add <input type="hidden" name="auto_create_ipam"
value="off"> immediately before the checkbox with id "auto-create-ipam-toggle");
apply the same pattern for the other toggles referenced (use-sysname-toggle,
strip-domain-toggle, etc.) or alternatively normalize missing keys in the view,
but the preferred fix is the hidden-input-before-checkbox change in the template
so HTMX posts an explicit false when unchecked.
In `@netbox_librenms_plugin/utils.py`:
- Around line 996-1003: The code drops numeric-string host IDs (e.g., "42") when
preparing the CF entry; detect and preserve numeric strings by treating them
like ints: add a branch that if entry is a str and entry.isdigit() then set
entry = {"id": int(entry)}, keep the existing dict branch (entry = dict(entry))
so shapes like {"default": "42"} are not reset to {}, and only fall back to {}
for truly non-numeric/non-dict values; update the logic around the
cf_value.get(server_key) handling (the variable entry and the surrounding
branch) so get_librenms_device_id, find_by_librenms_id and set_librenms_oob see
preserved numeric IDs.
- Around line 1168-1180: The normalization misses server IDs represented as
string or as dicts like {"default":"99"} so those get treated as empty; update
the handling of winner_entry and donor_entry (lookups from winner_cf/donor_cf
using server_key) to also normalize string numeric values and dicts with a
"default" key by converting them into {"id": int(value)} (or {"id": value} if
non-numeric string is expected) before the existing dict/int branches, and apply
the same normalization logic for both winner_entry and donor_entry so merges do
not lose valid IDs.
In `@netbox_librenms_plugin/views/__init__.py`:
- Around line 111-117: The F401 noqa is on the closing parenthesis so flake8
still flags the re-exports; move the "# noqa: F401" to the actual import
statement line (for example immediately after "from .imports.actions import (")
or add "# noqa: F401" on each imported name line (AddAsOOBView,
AddDeviceTypeMappingView, AddPlatformMappingView, MergeNetBoxDevicesView,
PromoteToHostView) so the unused-import ignore applies to those re-exported
symbols in netbox_librenms_plugin/views/__init__.py.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 113-133: The merged LLDP entries appended to links_data include a
"_source" key (see links_data entries added after get_librenms_oob and
self.librenms_api.get_device_links) but later cache re-enrichment/sanitization
strips unknown keys and drops "_source"; update the cache
re-enrichment/sanitization path to preserve or reapply the "_source" field for
cached links (or whitelist "_source" in the sanitization logic) so that entries
coming from OOB vs main remain source-aware after cache round-trips.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1930-1945: existing_mapping is checked before the transaction but
get_or_create() may return an object created concurrently, so after
get_or_create() you must re-check the change permission before mutating an
existing record: after mapping, created =
PlatformMapping.objects.get_or_create(...) and before assigning
mapping.netbox_platform or calling mapping.save(), call the permission check
(via self.require_object_permissions("POST") or an explicit check for ("change",
PlatformMapping)) and abort/return the error if present so callers who only
passed the initial "add" gate cannot update an existing mapping.
In `@netbox_librenms_plugin/views/sync/migrate.py`:
- Around line 265-270: The code assumes Device.objects.select_for_update()
returns both donor and winner and indexes locked[...] directly; instead, after
building locked = {d.pk: d ...}, verify that both donor.pk and winner.pk are
present (e.g. if not set(ordered) <= set(locked.keys()) or by using
locked.get(pk) and checking for None) and handle the missing case by returning
the same HTTP 410/409 response flow used elsewhere rather than letting a
KeyError propagate; apply the same presence-check + response logic to the other
similar block around the second select_for_update() call (the block related to
_resolve_winner_for_donor and the ip =
IPAddress.objects.select_for_update().get(...) sequence).
- Around line 192-207: The Interface instance was loaded before the transaction
so re-fetch it under the same lock and validate ownership before updating:
inside the transaction.atomic() block, after locking devices
(Device.objects.select_for_update()) call
Interface.objects.select_for_update().filter(pk=interface.pk) to re-retrieve the
row, check that the re-fetched interface exists and its device == donor (handle
missing or different owner by returning the same failure response), and only
then perform the update (e.g. update(device=winner)) so you don’t move a
stale/deleted/reattached row.
---
Duplicate comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 382-385: The existing check in device_operations.py using
get_librenms_oob(existing_device, server_key=server_key) only matches when the
stored nested id and librenms_id share the same runtime type; normalize both IDs
(e.g., cast to string or perform a safe int cast) before comparison so values
like {"oob": {"id": "42"}} correctly match; update the conditional around
_existing_oob.get("id") == librenms_id (used to set
result["existing_match_type"] = "librenms_oob") to compare normalized values
(reference symbols: get_librenms_oob, existing_device, server_key, librenms_id,
result["existing_match_type"]) and ensure the normalization handles None/invalid
values safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e234ae2-4712-4c4a-b353-03652fcf6e1f
⛔ Files ignored due to path filters (13)
docs/img/Netbox-librenms-plugin-device-sync-fields.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-import-page.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-module-sync-tab.pngis excluded by!**/*.pngdocs/img/carrier_auto_install_rules/list.pngis excluded by!**/*.pngdocs/img/device_type_mappings/list.pngis excluded by!**/*.pngdocs/img/inventory_ignore_rules/list.pngis excluded by!**/*.pngdocs/img/module_bay_mappings/list.pngis excluded by!**/*.pngdocs/img/module_type_mappings/add.pngis excluded by!**/*.pngdocs/img/module_type_mappings/list.pngis excluded by!**/*.pngdocs/img/normalization_rules/add.pngis excluded by!**/*.pngdocs/img/normalization_rules/list.pngis excluded by!**/*.pngdocs/img/platform_mappings/add.pngis excluded by!**/*.pngdocs/img/platform_mappings/list.pngis excluded by!**/*.png
📒 Files selected for processing (49)
docs/README.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/README.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.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_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/migrate.py
marcinpsk
commented
May 23, 2026
marcinpsk
added a commit
that referenced
this pull request
May 23, 2026
…ixes Phase 3 — AddAsOOBView, PromoteToHostView, MergeNetBoxDevicesView actions and the full UI (import validation modal, OOB badge, promote button). Phase 4 — Stage-2 'Move to winner' views: MoveInterfaceToWinnerView, MoveIPAddressToWinnerView, TransferDeviceIPView (migrate.py). Utilities — set_librenms_oob, clear_librenms_oob, get_librenms_oob, merge_librenms_links, mark_librenms_migrated, coerce_librenms_id, get_migrated_to_marker, detect_bulk_collisions, validate_device_for_import OOB/promote/host-serial path. Bug fixes addressed in PR #79 review (batches 1-5): - Accept generic 'oob' sentinel in set_librenms_oob - String-ID branches in set_librenms_oob and merge_librenms_links - Conditional filter(device=donor) guard on interface queryset update - locked.get() guards in MoveIPAddressToWinnerView and TransferDeviceIPView - _fail() unified OOB toast (HTTP 200 + HX-Reswap:none) across all views - _server_key_from_request default_server_key parameter rename - promote_to_host key omitted from default result dict when not applicable - noqa:F401 moved to from-line in views/__init__.py - _source preserved in cables_view._raw_keys on both strip paths - AddPlatformMappingView concurrent get_or_create permission re-check - Toggle checkboxes wrapped in span containers with hidden off inputs - TOCTOU race guards in conflict checks before atomic writes - entPhysicalIndex OOB offset changed from hardcoded 1_000_000 to dynamic - Bulk-import collision modal rendered at 200 not 409 - Various test fixes for new mock patterns and db access markers
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
… migrate tests Address CodeRabbit findings on PR #79: - ip_addresses_view._prepare_context: the fresh-refresh guard validated only the container shape, so a dict row missing the address/prefix or port_id fields _create_base_ip_entry() reads would KeyError mid-enrichment and 500. Validate the per-row schema and fail closed. Regression test added (fails pre-fix, passes post). - test_server_key_in_redirects: emit offender paths relative to the views package instead of the bare basename, so a failure is unambiguous when two scoped files share a name. - test_migrate_views: skip-path test now also asserts the donor keeps its IP (donor.primary_ip4_id == ip.pk), and the IP-move happy path is driven against real DB so the reassignment landing on the winner's same-named interface proves the donor re-lock and winner lookup used the correct device/name.
marcinpsk
commented
Jun 14, 2026
…s_data A malformed LibreNMS/cache ports payload (rows that are strings or null) caused port.get() to raise AttributeError and 500 the cables refresh, on both the main-device and OOB ports loops. Guard each row with isinstance(port, dict) (and the main loop's ports container with a list check, mirroring the OOB branch). Regression tests feed malformed rows and assert the valid row still resolves.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.playwright-mcp/page-2026-06-10T16-26-28-926Z.yml (1)
1-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove transient Playwright capture artifacts from the PR.
Line 1 introduces a raw UI snapshot artifact. This file (and the sibling
.playwright-mcp/page-2026-06-11T17-13-50-216Z.yml,.playwright-mcp/page-2026-06-11T17-26-52-669Z.yml,.playwright-mcp/page-2026-06-11T17-35-22-939Z.yml) embeds environment metadata (hostnames/URLs/user labels/timestamps) and creates high-churn, non-functional diffs. Please remove these from source control (or sanitize and keep only intentional, stable fixtures).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.playwright-mcp/page-2026-06-10T16-26-28-926Z.yml around lines 1 - 16, Remove the transient Playwright capture artifact files that contain environment-specific metadata and timestamps from the PR. These auto-generated UI snapshot files (with timestamps in their filenames in the .playwright-mcp directory) should not be committed to source control as they create high-churn, non-functional diffs. Either delete them entirely or add them to .gitignore to prevent their inclusion, keeping only intentional and stable test fixtures if any are needed for the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.playwright-mcp/page-2026-06-10T16-26-28-926Z.yml:
- Around line 1-16: Remove the transient Playwright capture artifact files that
contain environment-specific metadata and timestamps from the PR. These
auto-generated UI snapshot files (with timestamps in their filenames in the
.playwright-mcp directory) should not be committed to source control as they
create high-churn, non-functional diffs. Either delete them entirely or add them
to .gitignore to prevent their inclusion, keeping only intentional and stable
test fixtures if any are needed for the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f377e2b-045f-42ef-a4c7-02c53b0d5e79
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.playwright-mcp/page-2026-06-10T16-26-28-926Z.yml.playwright-mcp/page-2026-06-11T17-13-50-216Z.yml.playwright-mcp/page-2026-06-11T17-14-59-780Z.yml.playwright-mcp/page-2026-06-11T17-15-25-325Z.yml.playwright-mcp/page-2026-06-11T17-24-14-171Z.yml.playwright-mcp/page-2026-06-11T17-26-52-669Z.yml.playwright-mcp/page-2026-06-11T17-35-22-939Z.yml.playwright-mcp/page-2026-06-11T17-36-11-585Z.yml.playwright-mcp/page-2026-06-11T21-51-03-627Z.ymlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.14)
- GitHub Check: test-netbox (3.13)
🧰 Additional context used
📓 Path-based instructions (5)
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Do not reintroducedata-bs-toggleor duplicate modal IDs in modal implementation.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.
Do not re-addtable-responsivewrappers as their removal was deliberate to prevent dropdown clipping.
Templates live intemplates/netbox_librenms_plugin/; reuse and includes go underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html
netbox_librenms_plugin/**/*.{html,css}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Styling assumes Tabler defaults for the netbox_librenms_plugin frontend.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_migrate_views.py
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/cables_view.py
🧠 Learnings (18)
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html
📚 Learning: 2026-06-01T20:22:57.975Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:695-700
Timestamp: 2026-06-01T20:22:57.975Z
Learning: Do not recommend adding or propagating the removed `auto_create_ipam` toggle/preference via HTMX (e.g., `hx-include="`#auto-create-ipam-toggle`"`) or by introducing hidden `auto_create_ipam` inputs in out-of-band (OOB) / “promote” POST forms. Since the `auto_create_ipam` feature has been removed from the import page, any review suggestions attempting to wire it into `device_validation_details.html` or other import-flow templates should be ignored.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_migrate_views.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_migrate_views.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_migrate_views.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_migrate_views.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_migrate_views.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_migrate_views.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_migrate_views.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_migrate_views.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_migrate_views.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-06-01T15:12:26.824Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/ip_addresses.py:94-103
Timestamp: 2026-06-01T15:12:26.824Z
Learning: For any redirect/tab URL building in netbox_librenms_plugin/views/sync, views/base, and views/object_sync, propagate the active multi-server `server_key` as a `?server_key=<key>` query parameter so users return to the same server’s tab after POST actions. When handling POST requests, read the POST-scoped `server_key` from `request.POST` and store it (e.g., `self._post_server_key`) with a fallback to `self.librenms_api.server_key`; use this POST-scoped key for both cache-key scoping and for constructing the redirect/tab URLs. Treat this as the intentional codebase-wide convention—do not flag the presence/usage of the `server_key` query parameter (or the corresponding POST-scoped `_post_server_key` pattern) in these views as an error.
Applied to files:
netbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-06-05T07:19:49.079Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/base/interfaces_view.py:158-165
Timestamp: 2026-06-05T07:19:49.079Z
Learning: When building OOB relationships from interface/device view code, call get_librenms_oob() using the resolved sync device (e.g., `lookup_device = get_librenms_sync_device(obj, server_key=...) or obj; oob = get_librenms_oob(lookup_device, ...)`) rather than calling get_librenms_oob(obj, ... ) directly. For VC members, OOB data (including shared-LOM markers) is stored on the resolved sync device, so resolving first is required to avoid dropping OOB rows.
Applied to files:
netbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-03-07T09:14:06.791Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/base/cables_view.py:324-331
Timestamp: 2026-03-07T09:14:06.791Z
Learning: In netbox_librenms_plugin/views/base/cables_view.py, do not treat cache.ttl() usage as a portability issue. NetBox requires Redis as the cache backend (since NetBox v2.6), so django-redis cache.ttl() and cache.pttl() extensions are available. Consider this as a project-specific guideline: cache.ttl() is intentional/safe in this codebase.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.py
🔇 Additional comments (11)
netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html (1)
28-34: LGTM!netbox_librenms_plugin/tests/test_coverage_base_views2.py (1)
1163-1185: LGTM!netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
310-324: LGTM!.playwright-mcp/page-2026-06-11T17-36-11-585Z.yml (1)
1-384: LGTM!.playwright-mcp/page-2026-06-11T21-51-03-627Z.yml (1)
1-384: LGTM!netbox_librenms_plugin/tests/test_coverage_base_views.py (1)
228-295: LGTM!netbox_librenms_plugin/tests/test_migrate_views.py (1)
692-729: LGTM!Also applies to: 806-809
netbox_librenms_plugin/tests/test_server_key_in_redirects.py (1)
36-36: LGTM!Also applies to: 44-51
netbox_librenms_plugin/tests/test_vlan_sync.py (1)
13-14: LGTM!Also applies to: 488-658
netbox_librenms_plugin/views/base/cables_view.py (1)
33-64: LGTM!Also applies to: 85-96, 98-155, 197-201, 449-523, 533-587, 641-660
netbox_librenms_plugin/views/base/interfaces_view.py (1)
63-80: LGTM!Also applies to: 132-273, 299-309, 320-461
In migrated mode the VLAN sync form is replaced by a plain <div> so a migrated
donor cannot POST. The CSRF token and POST-only hidden inputs (server_key,
action) were still rendered inside that div — inert dead markup. Gate them on
{% if not migrated_to_marker %} so they only render with the real form. Adds a
render test asserting the migrated div emits no form/CSRF/hidden inputs while
normal mode still does.
…te path The docstring claimed the OOB-ports-fetch-failure path drops the partial cache entry and renders via fresh_data. That path was reworked to cache the host-only snapshot tagged oob_incomplete=True and render from cache (banner surfaces the missing OOB rows); no caller passes fresh_data anymore. Describe fresh_data as the render-from-snapshot escape hatch it is and stop describing removed behavior. Documentation-only; no behavior change.
The .playwright-mcp/ directory (page snapshots + console logs written by the Playwright MCP server during local UI verification) was committed by accident. Remove it from tracking and add it to .gitignore so these local artifacts no longer land in the repo.
The merge-candidate detection paired the hostname-matched device with a serial peer via .first(). Serial is not unique in NetBox (and device names are only unique per site), so .first() could pick an arbitrary same-serial row and surface the wrong merge target. Fetch up to two peers and only pair on exactly one; with more than one, skip the suggestion and warn. Applied symmetrically to the serial-matched/hostname-peer branch. DB-backed tests cover the multi-peer (warn, no suggestion) and unique-peer cases.
The three migrate/transfer reject-path tests stopped at the response assertion, so a regression that partially mutated the interface/IP/FK and then returned the same error toast would still pass. Add no-mutation guards: the collision reject keeps the interface on the donor and saves nothing; the winner-already-has-field reject leaves donor.primary_ip4 intact and saves neither device; the winner-lacks-interface reject leaves the IP on the donor interface and never persists it.
The _sync_redirect server_key tests covered allowlisted/spoofed/fallback handling but never the url_has_allowed_host_and_scheme=False path that _sync_url already pins. Add a test that an allowlisted server_key is still dropped when the redirect URL fails validation, so a regression reflecting a known key into a rejected URL is caught.
marcinpsk
commented
Jun 14, 2026
… return The interface refresh POST returned early on a missing librenms_id before the cache invalidation ran, so a failed refresh on a previously-synced device left the old ports snapshot in cache — the redirected sync tab (and downstream sync actions that load via CacheMixin.get_cache_key) would then serve/consume stale interface data. Move the cache.delete() calls ahead of the librenms_id lookup; they only depend on the already-resolved lookup_device/server_key. Regression test asserts the snapshot is cleared on the missing-id path (fails against the old ordering).
…-vals server_key
The interface table still renders interactive relationship/VC-member dropdowns in
migrated mode, and their verify-interface POST reads the token via
document.querySelector('[name=csrfmiddlewaretoken]'). Dropping the POST form in
migrated mode removed the only guaranteed token on the tab, so those JS requests hit
a null token (TypeError/403). Emit a standalone hidden csrfmiddlewaretoken in the
migrated <div> (a bare input never auto-submits, so it doesn't reintroduce the
live-form problem the form-drop avoided).
Also escapejs the server_key embedded in the migrated-mode hx-vals JSON so a key with
quotes/backslashes can't break JSON parsing and block the transfer/move action.
Adds a DB-backed render test asserting the token survives in migrated mode while the
form and form-only server_key input do not (and the form path still emits all three).
- migrate reject-path tests now assert the _fail() HTMX contract (HX-Reswap:none and no HX-Refresh) so a regression that returned HX-Refresh=true — refreshing as if the move/transfer succeeded — is caught. - _attach_messages_oob render-error test captures the storage and asserts used is False, pinning that a render failure doesn't leave queued messages consumed. - refresh-existing-device log test checks all positional args for the pk instead of only the format string, so a switch to parameterized logging stays green when correct.
…payloads, and non-Redis caches - cables: an OOB-only sync device has no host LibreNMS id, so the host LLDP fetch fails. Don't return None on that failure — fall through so the OOB merge runs and OOB-only devices still render their cable rows (returns None only when nothing at all is found). - interfaces: validate the OOB get_ports() payload (dict with a list of dict rows) before enriching/merging; a malformed-but-truthy response now follows the host-only oob_incomplete warning path instead of 500-ing. - interfaces + vlans: guard cache.ttl() with getattr — ttl() is Redis-specific and not part of the Django cache API, so other backends raised AttributeError at render (mirrors ip_addresses_view / modules_view). - vlans: evict the server-scoped VLAN snapshot on both refresh-failure exits so a failed refresh after a prior success can't leave a stale table for the next GET to render.
The outer-modal dismiss guard correctly stops nested data-bs-dismiss buttons from closing the outer HTMX modal, but when Bootstrap's modal JS is unavailable those nested buttons did nothing, leaving the user stuck in the child dialog. Manually hide just the nested modal in that fallback (Bootstrap still handles it natively when present).
…unique-peer merge - merge-peer discovery slices the queryset (exclude(...)[:2]); update the two skip-case mocks to stub the slice (__getitem__) instead of the unused .first() so the intended branch runs. - the single-serial-peer test only asserted absence of the multi-peer warning, which passes even with merge detection disabled. Give the peer a LibreNMS link (so the conservative guard fires) and assert the positive outcome: serial_action == merge_netbox_devices + candidates.
_detect_oob_type_from_name used a bare OOB_TYPE_PATTERN.search(), which returns the first token — so a name like 'leaf01-oob-idrac9' resolved to the generic 'oob' instead of the vendor-specific 'idrac'. Route it through normalize_oob_type() (same path the os/hardware detection uses), where a vendor token wins over the generic 'oob' regardless of order, so existing_oob_type / oob_candidate['type'] keep the more specific hint. Unit tests cover the generic-before-vendor case plus vendor-only / generic-only / none / empty.
… terminal - When the existing match is via the OOB sub-key (existing_match_type == 'librenms_oob'), the incoming LibreNMS payload is the OOB controller's, so the host serial-drift and device-type mismatch comparisons compare unrelated records and surface bogus replacement/conflict warnings on a correctly-linked row. Gate both on existing_match_type != 'librenms_oob'. - An ambiguous librenms_id (matches >1 NetBox record) is now the terminal blocker: skip the new-import site/device_type/role/cluster validation instead of piling unrelated 'must select' blockers onto a row whose only real problem is the duplicate id (mirrors bulk_import.py).
…n failure get_links_data returned None whenever it collected no rows, so a successful empty refresh (or a host-success/OOB-failure refresh) was mislabeled 'No links found' in _prepare_context() and the OOB-fetch warning was dropped. Return None only when nothing was collected AND a fetch error was recorded; otherwise return links_data (empty list flows through; any host/OOB/serial rows survive).
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js`:
- Around line 1288-1297: The Escape key handler in the document keydown listener
has a race condition where Bootstrap can remove the `.show` class from a nested
modal before the keydown listener executes, causing the querySelector check to
fail and allowing the outer modal to close when it shouldn't. Add an additional
check to gate the handler on whether the event originated inside a nested modal
element using event.composedPath() or by checking if event.target is contained
within a nested modal selector, since event origin is unaffected by Bootstrap's
timing of when it removes the `.show` class from nested modals.
In `@netbox_librenms_plugin/tests/test_coverage_tables.py`:
- Around line 1348-1384: The test_existing_paired_host_shows_host_button test
claims to verify OOB-type escaping but uses oob_type="idrac" which contains no
characters requiring HTML escaping, so the assertion would pass even if escaping
were not performed. Change the oob_type value in the existing_librenms_link
dictionary to a string with HTML special characters that require escaping (such
as characters like <, >, &, or quotes). Then add an additional assertion to
verify that the unescaped raw HTML version of this string is absent from the
result, making the test properly adversarial and catching cases where escaping
is omitted.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 124-134: The code currently records fetch errors in
_links_fetch_error when host LLDP fetch fails, but when OOB merge returns rows,
the success message is displayed without warning users about the partial
failure. In the success path (around the area where you check
_oob_links_fetch_failed), add an additional check to also verify if
_links_fetch_error is set. If either _links_fetch_error or
_oob_links_fetch_failed indicates a partial failure (one succeeded with data
while the other failed), emit a warning message instead of a generic success
message to inform users that cable data is incomplete. Apply this same check at
both affected locations in cables_view.py where the refresh completion message
is generated.
In `@netbox_librenms_plugin/views/base/interfaces_view.py`:
- Around line 223-227: The ports list on line 227 merges both enriched_ports and
oob_enriched without a way to disambiguate them during sync operations. When
SyncInterfacesView syncs selected rows using only
port.get(interface_name_field), rows with the same interface name from main and
OOB sources become indistinguishable, causing the later row to potentially
overwrite the NetBox interface with incorrect data. Either keep oob_enriched
rows separate from the cached ports list and handle them in a non-syncable way,
or add a unique identifier to each row combining source and port_id, then update
the sync matching logic in SyncInterfacesView to use this unique key instead of
interface_name_field alone.
In `@netbox_librenms_plugin/views/sync/migrate.py`:
- Around line 605-611: The code is saving the winner device first while the
donor device still references the same IP address, violating the unique
constraint on device IP FKs. Reverse the order of the save calls so that
donor.save(update_fields=[field]) is executed first to clear the donor's FK to
None, and then winner.save(update_fields=[field]) is executed second. This
allows the donor to release the IP address before the winner claims it, avoiding
the uniqueness constraint violation.
In `@netbox_librenms_plugin/views/sync/modules.py`:
- Around line 52-56: Preserve the computed fallback server_key value when
calling _modules_redirect_response() across all affected code paths. The
_modules_redirect_response() method only falls back to POST/GET lookups, so any
code that computes server_key with a fallback like server_key =
request.POST.get("server_key") or self.librenms_api.server_key must pass that
computed server_key value as a parameter to _modules_redirect_response() calls.
Update the _modules_redirect_response() method signature to accept an optional
server_key parameter, then modify all calls to this method (including those
around lines 52-56, 1274-1282, 1644-1665, and 1799-1815) to pass the computed
server_key so the redirect URL and cache scoping preserve the correct server
context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa674022-34e4-4f4b-8599-58ac28f67182
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (84)
.gitignoredocs/SUMMARY.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/custom_field.mddocs/usage_tips/oob_management.mdmkdocs.ymlnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/modules.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_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_oob_interface_select.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/create_platform_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_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/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
…nsfer device.primary_ip4/6 / oob_ip are UNIQUE per address; the direct transfer path saved the winner (claiming the address) while the donor still held it, tripping the unique constraint. Save the donor (release) first, then the winner — mirroring _reconcile_donor_device_ip_fks. Test pins the save ordering.
…esh succeed A host LLDP failure no longer aborts the refresh (OOB/serial rows can still surface it), so the success path now warns when _links_fetch_error is set and a host id was queried — otherwise host cables are silently omitted under a success banner. Skipped for OOB-only devices (librenms_id is None), where a host fetch failure is expected.
… overwrite the host OOB-controller rows are merged into the host interface list only for context (shared-LOM) and are never routed to a real target device. sync_selected_interfaces matched by name only, so a main+OOB 'eth0' collision processed both and the OOB row could overwrite the host interface with the wrong port_id/attrs. Skip _source=='oob' rows in the sync loop.
…directs Two module-sync handlers compute server_key = POST or self.librenms_api.server_key but called _modules_redirect_response(request, sync_url) without it on several return sites, so the helper fell back to POST/GET only and dropped the active-server context when the POST field was absent. Pass the computed server_key at every return site in those handlers.
…in a nested modal Bootstrap can strip .show from a nested modal before the keydown listener runs, so the '.modal.show' guard missed and Escape tore down the outer validation modal. Also gate on the event origin (event.target.closest of a nested modal), which is unaffected by that timing.
The test used oob_type='idrac' (no chars needing escaping), so it would pass even if render_actions() emitted the value unescaped. Use '<idrac>' and assert the escaped form is present and the raw value absent.
…failure An OOB-only mapping has no host librenms_id, so the host get_device_links() call always records _links_fetch_error even though no host fetch was meaningfully attempted. When the OOB controller validly returned no links, the empty-result guard mislabeled that as a failure and returned None, so _prepare_context() skipped caching the empty snapshot and stale OOB cable rows lingered after a genuine empty refresh. Return [] when the only reason for the recorded error is the absent host mapping on an OOB-scoped device.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Briefly describe what this PR does in plain English, and provide as much of the following information as possible.
Motivation / Problem
What issue does this solve?
Link any related issues if applicable.
Scope of Change
Delete items that don’t apply:
How Was This Tested?
Delete items that don’t apply and describe briefly.
Manual Test Steps (if applicable)
Risk Assessment
Explain briefly.
Backwards Compatibility
Other Notes
Anything the maintainer(s) should pay particular attention to?
Summary by CodeRabbit
New Features
Improvements
Documentation
librenms_idOOB JSON format details.