Feat/bulk import - #115
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds bulk collision prechecks, fail-closed cache validation, dynamic import permissions, permission-scoped synchronization lookups, linkage refresh handling, and extensive database-backed regression coverage. ChangesImport validation and synchronization safeguards
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ImportView
participant NetBox
participant Precheck
participant LibreNMS
participant ImportJob
ImportView->>NetBox: Check import permissions
ImportView->>Precheck: Scan selected device and VM rows
Precheck->>LibreNMS: Reuse verified cache or fetch device data
Precheck->>NetBox: Validate targets and detect collisions
Precheck-->>ImportView: Return approved, unresolved, or blocked rows
ImportView->>ImportJob: Dispatch approved rows
ImportJob-->>ImportView: Record imports and row failures
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
marcinpsk
force-pushed
the
feat/bulk-import
branch
5 times, most recently
from
June 19, 2026 17:58
432f566 to
aece92b
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
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/tests/test_coverage_sync_views2.py (1)
1257-1282: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake this VM interface test fail if the port-id lookup regresses.
interface_nameis still"eth0", so the test passes even if_match_interface()ignoresport_idand falls back by name. Use a deliberately stale/non-matching name while keepingport_id=9so this assertion proves the LibreNMS ID path is used. Based on learnings, MagicMock setups should not return the same result for both the librenms_id lookup and name fallback when the test is meant to distinguish those paths.🧪 Proposed hardening
ip_data = { "ip_address": "10.0.0.5", "ip_with_mask": "10.0.0.5/24", "port_id": 9, - "interface_name": "eth0", + # Deliberately stale: the match must come from port_id, not name fallback. + "interface_name": "cached-old-name", }🤖 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/tests/test_coverage_sync_views2.py` around lines 1257 - 1282, The test currently uses interface_name "eth0" which could match by name fallback even if the port_id lookup is ignored, making the test unable to distinguish between the librenms_id path and name fallback path. Change the interface_name value in the ip_data dictionary to a deliberately stale or non-matching name (different from "eth0") while keeping port_id=9 unchanged. This ensures the test will only pass if the _match_interface() function correctly uses the port_id lookup and does not regress to relying solely on name matching.Source: Learnings
netbox_librenms_plugin/import_utils/bulk_import.py (1)
590-637:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer any
librenms_idhit before name fallbacks across models.The guard detects
cross_id_match, but_lookup_in_model(Model)still tries preferred-model name matches beforeCrossModelis considered. If the scanned LibreNMS ID is already linked to a VM while a Device happens to share the resolved/hostname, refresh will bind the Device by name instead of the ID-linked VM, disagreeing with validation and potentially rendering actions for the wrong object.🐛 Proposed fix to make ID matches win across both models
- def _lookup_in_model(m): - """Return (device, match_type) for model m, or (None, None).""" - if librenms_id is not None: - dev = find_by_librenms_id(m, librenms_id, server_key) - if dev: - return dev, "librenms_id" + def _lookup_by_name_in_model(m): + """Return (device, match_type) for name-based matches in model m, or (None, None).""" resolved_name = validation.get("resolved_name") if resolved_name: dev = m.objects.filter(name__iexact=resolved_name).first() if dev: return dev, "resolved_name" @@ if librenms_id is not None: model_id_match = find_by_librenms_id(Model, librenms_id, server_key) cross_id_match = find_by_librenms_id(CrossModel, librenms_id, server_key) if model_id_match and cross_id_match: raise AmbiguousLibreNMSIdError( f"LibreNMS ID {librenms_id} matches both {Model.__name__} and {CrossModel.__name__}" ) + if model_id_match: + new_device, match_type = model_id_match, "librenms_id" + elif cross_id_match: + new_device, match_type = cross_id_match, "librenms_id" + found_as_cross_model = True - new_device, match_type = _lookup_in_model(Model) + if not new_device: + new_device, match_type = _lookup_by_name_in_model(Model) if not new_device: # Try the opposite model: catches cross-model imports that happened # after the cache was built (e.g. LibreNMS device imported as VM). - new_device, match_type = _lookup_in_model(CrossModel) + new_device, match_type = _lookup_by_name_in_model(CrossModel) if new_device: found_as_cross_model = True🤖 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/bulk_import.py` around lines 590 - 637, The issue is that _lookup_in_model(Model) tries name-based matches (resolved_name, hostname, sysname) before considering CrossModel, so if a librenms_id is already linked to a VM but a Device shares the same hostname, the Device will be matched by name instead of the ID-linked VM. Reorganize the lookup logic to prioritize librenms_id matches across both Model and CrossModel before attempting any name-based fallbacks. Check if librenms_id exists in Model first, then CrossModel, and only if neither has an ID match should you proceed to the name-based matching in _lookup_in_model.
🤖 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 740-853: The issue is that when duplicate hostname or serial peers
are detected in the merge-candidates detection block (in the conditions checking
len() > 1), the code only appends a warning message to result["warnings"] but
leaves the stale existing_device match (originally set via .first()) still
populated in the result. This means subsequent logic can act on an arbitrary
device. To fix this, when the duplicate peer check finds multiple devices (len >
1 for either hostname or serial), clear or block the corresponding
result["existing_device"] and result["existing_match_type"] values so the stale
match doesn't propagate to later operations. This should happen in the same code
paths where you currently append the duplicate-device warnings (around the
multiple-peer detection conditions), ensuring the uniqueness validation happens
before the result is used by apply_merge_candidates or other downstream logic.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js`:
- Around line 1309-1313: The Escape key handler currently suppresses the outer
modal close when a nested modal is detected (via the eventStartedInNestedModal
check), but it doesn't close the nested modal itself when Bootstrap is
unavailable. Instead of simply returning when a nested modal is open, add a
fallback that manually closes the nested modal by removing the show class and
associated backdrop from the nested dialog element when Bootstrap is not
available. Follow the pattern of trying Bootstrap.Modal first and then falling
back to manual DOM manipulation to hide the nested modal.
In `@netbox_librenms_plugin/tables/device_status.py`:
- Around line 520-544: The guard condition evaluating whether to render the Host
state (starting at line 523) does not validate that the coerced paired_oob_id is
actually valid. When _coerce_pair_id(paired_oob_id) returns None for a malformed
ID, the condition still passes because None differs from a valid host ID,
causing bogus Host titles to render. Tighten the guard by adding an additional
check to ensure _coerce_pair_id(paired_oob_id) is not None (returning a valid
ID) before allowing the paired host styling to be applied, so that malformed
paired_oob_id values fall through to the generic details state instead.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html`:
- Around line 333-354: The checkboxes in this template lack accessible labels
for screen readers. Add an aria-label attribute to the select-all checkbox with
id "select-all-netbox-interfaces" to describe its function (selecting all NetBox
interfaces), and add an aria-label to each per-row checkbox with name
"interface_ids" to identify which interface row it corresponds to (you can
reference the interface.name or similar property in the label text).
- Around line 367-384: The confirmation dialog for the Move button in migrated
mode currently contains misleading language about permanent deletion, but should
instead accurately describe the action as moving or reassigning the interface.
Update the hx-confirm attribute on the button with class btn-outline-warning
within the migrated_to_winner conditional block to ensure the confirmation
message clearly indicates that the interface (and its associated
cables/IPs/MACs) is being moved/reassigned to the winner interface, not deleted.
Verify that the warning text matches the actual behavior of the
interface_move_to_winner action.
In `@netbox_librenms_plugin/tests/test_badge_contrast.py`:
- Around line 32-40: The regex pattern `_CLASS_ATTR` currently only matches
class attributes with double quotes on a single line, which causes false
negatives when class attributes use single quotes. Update the `_CLASS_ATTR`
regex pattern to match both single-quoted and double-quoted class attributes by
modifying the pattern to accept either quote type around the attribute value.
This ensures the _bare_badge_offenders function correctly identifies all
badge-related class attributes regardless of which quote style is used.
In `@netbox_librenms_plugin/tests/test_coverage_sync_views2.py`:
- Around line 1644-1653: The test is patching the `reverse()` function from
`netbox_librenms_plugin.views.sync.vlans`, which prevents it from validating
whether the actual URL name used in the VLAN sync redirect is correct. Remove
the patch for `reverse()` from the context manager in this test method and the
other similar success-path tests (the ones on lines around 1681-1690 and
1710-1719) so that the real `reverse()` function executes and validates that the
URL name passed to it matches a valid plugin route. This ensures the test fails
if the view attempts to redirect to an invalid URL name.
In `@netbox_librenms_plugin/tests/test_import_utils.py`:
- Around line 1294-1297: The mock setup in the duplicate-detection tests uses a
generic MagicMock chain that returns existing for any
Device.objects.filter().first() call regardless of the filter criteria applied.
This masks whether the implementation queries by the resolved name (core-switch)
or the raw hostname (10.0.0.1), so the test no longer verifies its documented
behavior. Modify the mock setup to be path-sensitive by configuring different
return values based on the specific filter arguments passed to
Device.objects.filter(), ensuring that only queries filtering by the resolved
name return existing while other filter queries behave differently. This
approach will make the test fail if the implementation queries the wrong
hostname variant.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 590-637: The issue is that _lookup_in_model(Model) tries
name-based matches (resolved_name, hostname, sysname) before considering
CrossModel, so if a librenms_id is already linked to a VM but a Device shares
the same hostname, the Device will be matched by name instead of the ID-linked
VM. Reorganize the lookup logic to prioritize librenms_id matches across both
Model and CrossModel before attempting any name-based fallbacks. Check if
librenms_id exists in Model first, then CrossModel, and only if neither has an
ID match should you proceed to the name-based matching in _lookup_in_model.
In `@netbox_librenms_plugin/tests/test_coverage_sync_views2.py`:
- Around line 1257-1282: The test currently uses interface_name "eth0" which
could match by name fallback even if the port_id lookup is ignored, making the
test unable to distinguish between the librenms_id path and name fallback path.
Change the interface_name value in the ip_data dictionary to a deliberately
stale or non-matching name (different from "eth0") while keeping port_id=9
unchanged. This ensures the test will only pass if the _match_interface()
function correctly uses the port_id lookup and does not regress to relying
solely on name matching.
🪄 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: 7203dc58-5623-46cb-b5d9-53341b2da513
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (89)
.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_badge_contrast.pynetbox_librenms_plugin/tests/test_cable_sync_content_template.pynetbox_librenms_plugin/tests/test_cable_verify.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_device_validation_details_template.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_module_sync_content_template.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
marcinpsk
force-pushed
the
feat/bulk-import
branch
2 times, most recently
from
June 20, 2026 05:35
b353b95 to
bcf9cf1
Compare
Owner
Author
|
@coderabbitai Both outside-diff findings are develop-inherited, so per our stacked-PR convention they’re handled on the develop-targeted branch (
|
|
Tip For best results, initiate chat on the files or code changes.
The stacked-PR convention makes sense — I'll keep it in mind for future reviews of this PR series. ✏️ Learnings added
|
marcinpsk
force-pushed
the
feat/bulk-import
branch
3 times, most recently
from
June 20, 2026 14:05
a82a5f8 to
df5eeeb
Compare
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (3)
netbox_librenms_plugin/views/base/interfaces_view.py (2)
261-265:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep internal LibreNMS IDs out of the UI warning.
The server log already includes
oob["id"]; the toast should stay generic so internal LibreNMS identifiers are not exposed in user-facing UI.🛡️ Proposed fix
messages.warning( request, - f"Interfaces refreshed, but OOB controller ports fetch failed (OOB id {oob['id']}); " + "Interfaces refreshed, but OOB controller ports fetch failed; " "showing host interfaces only. See server logs for details.", )🤖 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/base/interfaces_view.py` around lines 261 - 265, In the messages.warning call that displays the OOB controller ports fetch failure message, remove the internal LibreNMS identifier from the user-facing UI message by deleting the interpolated oob['id'] portion from the f-string. Keep the warning message generic for end users (such as "OOB controller ports fetch failed") without exposing internal IDs, while relying on server logs to capture the detailed identification information needed for debugging.
153-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve migrated mode when the posted server key is stale.
This branch redirects without the POSTed
server_keyand withoutbuild_migrated_context(). On a migrated donor whose marker is namespaced to the stale key, the HTMX refresh can come back without the migrated marker and re-enable normal interface sync controls.Based on learnings, POST handlers in base/sync views should propagate the POST-scoped
server_keyfor cache and redirect/tab state.🐛 Proposed fix
- post_server_key = self.rebind_api_for_server(request.POST.get("server_key")) + posted_server_key = request.POST.get("server_key") + post_server_key = self.rebind_api_for_server(posted_server_key) if post_server_key is None: messages.error(request, "Selected LibreNMS server is no longer configured.") - return redirect(self.get_redirect_url(obj)) + return render( + request, + self.partial_template_name, + { + "interface_sync": { + "object": obj, + "table": None, + "vlan_groups": [], + "last_fetched": None, + "cache_expiry": None, + "virtual_chassis_members": [], + "interface_name_field": interface_name_field, + "netbox_only_interfaces": [], + "server_key": None, + "oob_incomplete": False, + }, + "interface_name_field": interface_name_field, + **build_migrated_context(obj, posted_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/base/interfaces_view.py` around lines 153 - 156, When post_server_key is None (indicating a stale server key), the redirect to get_redirect_url(obj) does not preserve the migrated context. Pass the original stale server_key from request.POST.get("server_key") as a parameter to get_redirect_url() to maintain the migrated mode and ensure the HTMX refresh preserves the migrated marker that is namespaced to that stale key. This ensures the tab state and cache remain consistent for the migrated donor context.Source: Learnings
netbox_librenms_plugin/views/base/cables_view.py (1)
225-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail OOB cable refresh when OOB port metadata is unavailable.
If
get_ports(oob["id"])fails or returns malformed data, the maps stay empty but OOB link rows are still appended and cached as a successful snapshot. For LibreNMS link rows that only carrylocal_port_id, this dropslocal_portresolution and can overwrite a good cache with unverifiable OOB cable rows.🐛 Proposed fix
oob_ports_success, oob_ports_data = self.librenms_api.get_ports(oob["id"]) oob_local_ports_map = {} oob_local_ports_alt_map = {} - if oob_ports_success and isinstance(oob_ports_data, dict): - oob_ports = oob_ports_data.get("ports") - for port in oob_ports if isinstance(oob_ports, list) else []: + oob_ports_ok = ( + oob_ports_success + and isinstance(oob_ports_data, dict) + and isinstance(oob_ports_data.get("ports"), list) + and all(isinstance(port, dict) for port in oob_ports_data["ports"]) + ) + if not oob_ports_ok: + self._oob_links_fetch_failed = True + logger.warning( + "OOB ports fetch failed for device %s (OOB id %s): %s", + self.librenms_id, + oob["id"], + oob_ports_data, + ) + else: + for port in oob_ports_data["ports"]: # Skip non-dict rows (see the main-branch guard above) so a # malformed OOB ports payload can't 500 the refresh. - if not isinstance(port, dict): - continue raw_port_id = port.get("port_id") if raw_port_id is None: continue port_name = port.get(interface_name_field) if port_name is None: continue oob_local_ports_map[str(raw_port_id)] = port_name # Same alternate-field fallback as the main branch (issue `#88`). alt_name = port.get(alt_name_field) if alt_name and alt_name != port_name: oob_local_ports_alt_map[str(raw_port_id)] = alt_name + + # Only merge OOB links after the port-id -> name map is trustworthy. + oob_links = oob_data.get("links") + ...Also applies to: 263-283
🤖 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/base/cables_view.py` around lines 225 - 245, The issue is that when the get_ports API call fails (oob_ports_success is False) or returns malformed data (oob_ports_data is not a dict), the oob_local_ports_map and oob_local_ports_alt_map dictionaries remain empty, but the OOB cable rows are still appended and cached as a successful snapshot. To fix this, add a guard check after the if block that populates these maps: if oob_ports_success is False or oob_ports_data is not a dict, use continue to skip processing this OOB device entirely and move to the next OOB iteration. This prevents unverifiable OOB cable rows with empty port metadata from overwriting the cache. Apply the same fix to the similar code mentioned in the also applies to section at lines 263-283.
🤖 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 `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 225-245: The issue is that when the get_ports API call fails
(oob_ports_success is False) or returns malformed data (oob_ports_data is not a
dict), the oob_local_ports_map and oob_local_ports_alt_map dictionaries remain
empty, but the OOB cable rows are still appended and cached as a successful
snapshot. To fix this, add a guard check after the if block that populates these
maps: if oob_ports_success is False or oob_ports_data is not a dict, use
continue to skip processing this OOB device entirely and move to the next OOB
iteration. This prevents unverifiable OOB cable rows with empty port metadata
from overwriting the cache. Apply the same fix to the similar code mentioned in
the also applies to section at lines 263-283.
In `@netbox_librenms_plugin/views/base/interfaces_view.py`:
- Around line 261-265: In the messages.warning call that displays the OOB
controller ports fetch failure message, remove the internal LibreNMS identifier
from the user-facing UI message by deleting the interpolated oob['id'] portion
from the f-string. Keep the warning message generic for end users (such as "OOB
controller ports fetch failed") without exposing internal IDs, while relying on
server logs to capture the detailed identification information needed for
debugging.
- Around line 153-156: When post_server_key is None (indicating a stale server
key), the redirect to get_redirect_url(obj) does not preserve the migrated
context. Pass the original stale server_key from request.POST.get("server_key")
as a parameter to get_redirect_url() to maintain the migrated mode and ensure
the HTMX refresh preserves the migrated marker that is namespaced to that stale
key. This ensures the tab state and cache remain consistent for the migrated
donor context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4886672-b25c-41b5-9595-963ba0f7d71a
📒 Files selected for processing (62)
netbox_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/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/device_status.pynetbox_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/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_badge_contrast.pynetbox_librenms_plugin/tests/test_cable_verify.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_device_validation_details_template.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_ipaddress_sync_content_template.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_modules.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/sync/device_fields.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
marcinpsk
force-pushed
the
feat/bulk-import
branch
from
June 20, 2026 22:43
df5eeeb to
6ce26d8
Compare
Owner
Author
|
@coderabbitai — the three outside-diff (body-only) findings from the latest review on this PR:
|
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
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/base/vlan_table_view.py (1)
48-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the migrated-context fallback when the POST key is empty.
If
server_keyis missing or blank here,build_migrated_context()gets an empty value and the retry fragment can render without the donor/winner controls.Proposed fix
- **build_migrated_context(obj, request.POST.get("server_key")), + **build_migrated_context( + obj, + request.POST.get("server_key") or 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/base/vlan_table_view.py` around lines 48 - 57, The issue is that when the POST key "server_key" is missing or empty, the call to build_migrated_context(obj, request.POST.get("server_key")) receives a None or empty value, which prevents the donor/winner controls from being properly preserved in the error context. Fix this by providing a fallback value when request.POST.get("server_key") returns None or is empty. The fallback should extract the server_key from the object itself or another appropriate source to ensure the migrated-context flags are properly maintained when rendering the retry fragment.netbox_librenms_plugin/views/base/cables_view.py (1)
611-625:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFall back to the active server key when preserving migrated context.
Line 625 passes
posted_server_keydirectly tobuild_migrated_context(). When the POST omits or sends a blankserver_key, this becomesNone/blank and can miss the donor’s server-scoped_migrated_tomarker, re-enabling the normal cable-sync controls in the stale-server error partial.Proposed fix
- posted_server_key = request.POST.get("server_key") + posted_server_key = (request.POST.get("server_key") or "").strip() + migrated_server_key = ( + posted_server_key + or getattr(getattr(self, "_librenms_api", None), "server_key", None) + or "default" + ) # Rebind the API to the POSTed server so live link/port fetches hit the same # LibreNMS instance the cached rows are namespaced under (multi-server tabs). server_key = self.rebind_api_for_server(posted_server_key) @@ "cable_sync": {"object": obj, "table": None, "cache_expiry": None, "server_key": None}, - **build_migrated_context(obj, posted_server_key), + **build_migrated_context(obj, migrated_server_key), }, )Based on learnings, POST handlers in
views/baseshould keep server-scoped state using the POSTedserver_keywith a fallback to the activeself.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/base/cables_view.py` around lines 611 - 625, The issue is that on line 625, posted_server_key is passed directly to build_migrated_context() without a fallback, so when the POST omits or sends a blank server_key value (making it None or blank), the donor's server-scoped _migrated_to marker is missed. Fix this by modifying the call to build_migrated_context(obj, posted_server_key) to use a fallback pattern: pass posted_server_key if it is valid and non-empty, otherwise fall back to self.librenms_api.server_key to preserve the server-scoped migrated context correctly.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/tests/test_migrate_views.py`:
- Around line 881-890: Replace the hardcoded GATE template string in the
TestMigratedTransferIpDeviceOnlyGate class with the actual production template
or a reference to the real librenms_sync_base.html file. Update the _render
method to load and render the actual production template section containing the
transfer-IP controls instead of rendering the copied mini-template defined in
GATE. This ensures the regression test verifies the actual shipped template
source and will catch if someone removes or changes the device-only guard from
the real template.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 2849-2886: The current code locks existing_device first with
select_for_update(), then later calls find_by_librenms_id with
select_for_update=True to check for conflicts, which can cause a deadlock when
two concurrent promotions occur in opposite directions (each holding one device
lock and waiting for the other). Refactor to acquire both relevant device row
locks in a deterministic order (such as by primary key) first before performing
any validation logic, similar to how the merge flow handles this. This ensures
locks are always acquired in the same order across all concurrent operations,
preventing the deadlock scenario.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 611-625: The issue is that on line 625, posted_server_key is
passed directly to build_migrated_context() without a fallback, so when the POST
omits or sends a blank server_key value (making it None or blank), the donor's
server-scoped _migrated_to marker is missed. Fix this by modifying the call to
build_migrated_context(obj, posted_server_key) to use a fallback pattern: pass
posted_server_key if it is valid and non-empty, otherwise fall back to
self.librenms_api.server_key to preserve the server-scoped migrated context
correctly.
In `@netbox_librenms_plugin/views/base/vlan_table_view.py`:
- Around line 48-57: The issue is that when the POST key "server_key" is missing
or empty, the call to build_migrated_context(obj,
request.POST.get("server_key")) receives a None or empty value, which prevents
the donor/winner controls from being properly preserved in the error context.
Fix this by providing a fallback value when request.POST.get("server_key")
returns None or is empty. The fallback should extract the server_key from the
object itself or another appropriate source to ensure the migrated-context flags
are properly maintained when rendering the retry fragment.
🪄 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: 4a799f44-9fab-4a6b-bb50-bacaac8d5e33
📒 Files selected for processing (27)
netbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_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_device_operations.pynetbox_librenms_plugin/tests/test_device_validation_details_template.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.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_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/sync/migrate.py
marcinpsk
force-pushed
the
feat/bulk-import
branch
from
June 21, 2026 05:49
6ce26d8 to
2343552
Compare
Owner
Author
|
@coderabbitai — the outside-diff (body-only) findings from the latest review:
|
|
Tip For best results, initiate chat on the files or code changes.
Both are clean fixes to a subtle but important invariant (stale POST key must never be forwarded into migrated-context resolution). ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
cached_row_matches() compared coerced ids with ==, so two un-coercible ids matched as None == None and an invalid requested id was served any trusted cached row. Validate the requested id first: un-coercible never matches. The single-row live-fetch fallback in bulk_import_devices_shared() accepted any successful payload and wrote it into the shared cache without checking its device_id against the requested id. Extract the collision pre-check's strict rule into row_identity_matches() and apply it at both sites: a payload that isn't a dict carrying the requested device_id is treated as a failed retrieval and never cached.
NetBox treats MAX_PAGE_SIZE 0/None as 'no ceiling'. min(per_page, 0) silently returned a zero page size and min(per_page, None) raised TypeError; pass the requested per_page through unclamped when the ceiling is disabled.
The IP sync writes the IPAddress row and then saves the device (primary_ip4/6), while the migrate move views lock Device first and only then the IPAddress and its interface. Two concurrent transactions taking the two rows in opposite order close a deadlock cycle, which Postgres resolves by aborting one side. Take the device row lock before the address write on the primary-IP candidate row, so both paths order their locks Device -> IPAddress, and re-read the primary_ip ids from the locked row (a stale in-memory value would make _set_primary_ip skip a set it must perform). Only the management-address row pays for the extra lock; every other row is unchanged. The same-host test is now computed once and reused for the three places that asked for it.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/tests/test_validation_template_server_key.py`:
- Around line 45-46: Update the validation helpers around
_form_has_named_control so server_key validation only matches an input element
with type="hidden" and name="server_key", rather than any named control such as
a button. Preserve button matching for action validation, and use the
server-key-specific matcher at the checks on lines 73 and 83.
🪄 Autofix
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 Plus
Run ID: f4d713e4-adac-4072-84b9-0b869bf62a2a
📒 Files selected for processing (3)
netbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_vlan_sync_concurrency.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
🧠 Learnings (17)
📚 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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
📚 Learning: 2026-07-01T16:41:50.451Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tables/cables.py:62-76
Timestamp: 2026-07-01T16:41:50.451Z
Learning: When rendering Bootstrap/Tabler badges in this repo (netbox-librenms-plugin), always pair a solid background utility `bg-*` with an explicit text utility that provides appropriate contrast (e.g., `bg-danger` + `text-white`, `bg-warning` + `text-dark`, `bg-purple` + `text-white`). Follow the existing badge contrast convention enforced by `netbox_librenms_plugin/tests/test_badge_contrast.py`. Do not suggest Tabler’s semantic `text-*-fg` token classes (e.g., `text-purple-fg`), since they are not used in this codebase and are not covered by the contrast test.
Applied to files:
netbox_librenms_plugin/tests/test_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
📚 Learning: 2026-07-30T02:40:53.531Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 123
File: netbox_librenms_plugin/views/base/modules_view.py:0-0
Timestamp: 2026-07-30T02:40:53.531Z
Learning: In the NetBox LibreNMS plugin, normalize all LibreNMS serial values via `netbox_librenms_plugin.utils.normalize_serial()` before storing/comparing them. Treat only `None` as “absent”; for any other value (including numeric/falsey values like `0` or `False`), convert using `str(value).strip()` inside the normalizer. For identity/conflict checks against existing NetBox `Device` rows, compare using trimmed serial matching (i.e., compare normalized/stripped serial strings) so legacy device serials with surrounding whitespace still match normalized incoming serials.
Applied to files:
netbox_librenms_plugin/tests/test_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
📚 Learning: 2026-08-03T19:16:41.198Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 87
File: netbox_librenms_plugin/views/base/ip_addresses_view.py:502-502
Timestamp: 2026-08-03T19:16:41.198Z
Learning: For migration-marker handling in the NetBox LibreNMS plugin, use `get_migrated_to_marker()` and `mark_librenms_migrated()` as the centralized read/write chokepoints rather than duplicating marker logic. Normalize blank or `None` `server_key` values to `"default"`. Derive migration UI context with `build_migrated_context()` and pass it through `render_sync_partial()` to the interface, IP, cable, module, and VLAN sync partials.
Applied to files:
netbox_librenms_plugin/tests/test_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
📚 Learning: 2026-06-15T18:49:04.201Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_coverage_actions.py:1866-1870
Timestamp: 2026-06-15T18:49:04.201Z
Learning: When reviewing tests related to the LibreNMS device ID migration flow (e.g., `migrate_librenms_id` / `migrate_legacy_librenms_id`), do not require `validation["librenms_id_needs_migration"] == True` solely for test setup. That flag is only used for UI visibility in `device_status.py` / `device_validation_details.html`; the backend migration action is gated by the instance’s legacy raw value (`custom_field_data["librenms_id"]` matching the active LibreNMS device id) plus the `serial_confirmed` or `force` condition. If the test already pins/executes migration by asserting the migration function was called with the locked instance (or otherwise directly forces execution), it should be acceptable even when `librenms_id_needs_migration` is not set to True.
Applied to files:
netbox_librenms_plugin/tests/test_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
📚 Learning: 2026-06-17T07:31:54.849Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 104
File: netbox_librenms_plugin/tests/test_librenms_api.py:2258-2258
Timestamp: 2026-06-17T07:31:54.849Z
Learning: When reviewing Python test code in netbox_librenms_plugin/tests, treat “develop-owned” scaffold lines as off-limits for in-PR rewrites. A line is “develop-owned” if `git blame` for that line attributes it to a commit that is an ancestor of `origin/develop` (i.e., the commit is contained in `origin/develop`). For such lines, reviewers should acknowledge the findings as valid but defer the change by creating/using a follow-up issue targeting the `develop` branch (e.g., `#112`), rather than requesting modifications in the current feature/PR stack.
Applied to files:
netbox_librenms_plugin/tests/test_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
📚 Learning: 2026-06-19T14:03:09.440Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 113
File: netbox_librenms_plugin/tests/test_coverage_base_views2.py:555-567
Timestamp: 2026-06-19T14:03:09.440Z
Learning: In tests under netbox_librenms_plugin/tests, don’t rely on “pure” MagicMock setups that stub chained calls like `interfaces.filter.return_value.first.return_value` when the code under test is supposed to distinguish between (1) a librenms_id custom-field lookup and (2) a name-based fallback lookup. If the mock returns the same interface regardless of filter arguments, the test cannot detect which lookup path matched (renaming variables like `remote_port` doesn’t fix this). Use a real-DB hardening test instead: create/seed a `remote_port` value that is deliberately different from the actual interface name so only the librenms_id CF lookup can produce a match. If an existing MagicMock-masked test file is develop-inherited (identical on origin/develop), don’t modify it in feature PRs; add a new real-DB hardening test file (e.g., `test_enrich_remote_port_realdb.py`) on the develop-targeted branch.
Applied to files:
netbox_librenms_plugin/tests/test_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
📚 Learning: 2026-06-25T07:07:59.192Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 114
File: netbox_librenms_plugin/tests/test_oob_sync_review_fixes.py:0-0
Timestamp: 2026-06-25T07:07:59.192Z
Learning: When writing/adjusting tests (and any review/test helper code) that interact with NetBox’s custom User model, do not assume the user model has an `is_staff` field. If you need to check or set user privileges/eligibility, use fields that are known to exist on the NetBox User model (e.g., `is_superuser` and `is_active`) instead. Avoid setting `is_staff` (it can raise Django `FieldError` if that field doesn’t exist on the custom model).
Applied to files:
netbox_librenms_plugin/tests/test_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
📚 Learning: 2026-07-02T21:46:46.384Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_coverage_bulk_import.py:132-140
Timestamp: 2026-07-02T21:46:46.384Z
Learning: In netbox_librenms_plugin/tests, keep the `_stub_norm_preload` autouse fixture (which patches `netbox_librenms_plugin.import_utils.bulk_import.preload_normalization_rules` to return `{}`) intentionally duplicated per mock-based test class/module. Do NOT hoist it into a global `conftest.py` autouse fixture, because that would apply the patch repo-wide and mask the real `preload_normalization_rules` behavior in real-DB tests that are meant to exercise it (see issue `#90`). If you need this patch, scope it to the specific mock-based tests that should stub normalization rules.
Applied to files:
netbox_librenms_plugin/tests/test_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.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_vlan_sync_concurrency.pynetbox_librenms_plugin/tests/test_validation_template_server_key.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
🪛 ast-grep (0.45.0)
netbox_librenms_plugin/tests/test_validation_template_server_key.py
[warning] 27-27: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(name_pattern, tag, re.IGNORECASE)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🔇 Additional comments (3)
netbox_librenms_plugin/tests/test_vlan_sync_concurrency.py (1)
1-96: LGTM!netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py (2)
12-39: LGTM!Also applies to: 91-94, 104-107, 376-376, 393-403, 442-456, 474-491, 510-527, 547-574, 597-600, 627-629, 721-784, 847-848, 852-883
895-895: 📐 Maintainability & Code QualityNo change needed.
Comment on lines
+45
to
+46
| def _form_has_named_control(form_html, name): | ||
| return next(_named_control_tags(form_html, name), None) is not None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require a hidden input for server_key.
_form_has_named_control() accepts <button name="server_key">. A button submits its value only when that button is clicked. Lines 73 and 83 can then pass although another action does not post server_key.
Add a server-key-specific matcher that requires an <input type="hidden" name="server_key">. Keep button matching only for action.
🤖 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/tests/test_validation_template_server_key.py` around
lines 45 - 46, Update the validation helpers around _form_has_named_control so
server_key validation only matches an input element with type="hidden" and
name="server_key", rather than any named control such as a button. Preserve
button matching for action validation, and use the server-key-specific matcher
at the checks on lines 73 and 83.
This was referenced Aug 12, 2026
Closed
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
1 participant
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
Stacked on #114 — review only the delta over
feat/device-merge.Bulk-import collision detection. At refresh time each import row is re-checked against existing NetBox devices by serial, primary IP and
oob_ipbefore it can be imported, so a device that already exists under a different name can't be re-imported as new.Motivation / Problem
Feature / bug. Close a gap where a row whose
librenms_id/name link disappeared could flip to importable and duplicate an existing device.Scope of Change
How Was This Tested?
Risk Assessment
Makes the refresh re-check stricter (fail-closed); cannot cause new imports.
Backwards Compatibility
Summary by CodeRabbit
New Features
Bug Fixes
Tests