fix(develop): overall pass - #123
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR centralizes serial and IP normalization, strengthens import refresh and conflict-action validation, adds advisory-lock serial assignment, improves permission and API edge-case handling, adds migration support for existing serials and indexing, and expands regression coverage. Documentation also gains screenshot alt text and uninstall rollback guidance. ChangesNormalization and serial persistence
Conflict actions and refresh validation
Compatibility and edge cases
Documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceConflictActionView
participant PostgreSQL
participant Device
DeviceConflictActionView->>PostgreSQL: Acquire advisory lock for normalized serial
DeviceConflictActionView->>Device: Check serial ownership without row lock
DeviceConflictActionView->>Device: Persist or reject serial assignment
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
marcinpsk
force-pushed
the
fix/develop-hardening-2
branch
from
July 17, 2026 10:27
76e68c2 to
8bcc4ed
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
netbox_librenms_plugin/import_utils/device_operations.py (1)
456-482: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNormalize serial before comparing and storing.
The trim only applies to the serial-matching branch; the drift checks still compare the raw value, andimport_single_device()saves it raw. A padded LibreNMS serial can still be persisted padded and miss the next trimmedfilter(serial=...), recreating duplicates.🤖 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 456 - 482, Normalize the LibreNMS serial once before all matching, drift-check, and persistence logic, not only inside the serial-matching branch. Update the relevant flow around import_single_device() and its serial comparisons to use the trimmed serial value, and ensure the normalized value is stored on the imported device so subsequent Device.objects.filter(serial=...) lookups remain consistent.
🤖 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/import_utils/device_operations.py`:
- Around line 456-482: Normalize the LibreNMS serial once before all matching,
drift-check, and persistence logic, not only inside the serial-matching branch.
Update the relevant flow around import_single_device() and its serial
comparisons to use the trimmed serial value, and ensure the normalized value is
stored on the imported device so subsequent Device.objects.filter(serial=...)
lookups remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc3b3184-a131-425f-9e7d-c9a80df7ab50
📒 Files selected for processing (15)
docs/usage_tips/interface_mappings.mdnetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/imports/actions.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test-netbox (3.14)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.12)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
**/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/cables_view.py
**/*.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/views/base/cables_view.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/import_utils/bulk_import.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
**/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
🧠 Learnings (33)
📚 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
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/import_utils/bulk_import.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/views/base/cables_view.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/import_utils/bulk_import.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/views/base/cables_view.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/import_utils/bulk_import.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/cables_view.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/cables_view.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-07-02T13:36:15.226Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/interfaces_view.py:226-226
Timestamp: 2026-07-02T13:36:15.226Z
Learning: When reviewing netbox-librenms-plugin view code that handles POSTed `server_key`, treat the plugin-wide convention as intentional: if the posted `server_key` is not present in `LibreNMSAPI.get_available_servers()`, the request should fall back to the currently configured default/active server key (not reject/fail-closed and not treat it as an error for that single view). Do not flag individual instances of this fallback pattern as incorrect “invalid server_key” validation. Any change to fail-closed behavior must be a coordinated cross-cutting change applied uniformly across all affected sites/views, not a one-view patch.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/imports/actions.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/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/cables_view.py
📚 Learning: 2026-06-26T09:04:49.793Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_verify_views.py:281-343
Timestamp: 2026-06-26T09:04:49.793Z
Learning: When implementing per-object permission resolution in verify views (e.g., like `SingleIPAddressVerifyView`), `_required_perms_for_object` should: (1) if `object_type` is explicit, gate on the exact model permission for that target type; (2) if `object_type` is not explicit, resolve the object id to its model without reading the object’s data (avoid fetching the object just to determine permissions); and (3) in ambiguous cases, fail closed by requiring all relevant view permissions (deny unless both applicable permissions are satisfied). Add/extend DB-backed tests to cover allow/deny paths and the “no `object_type`” case.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-03-12T20:27:53.873Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/librenms_api.py:703-709
Timestamp: 2026-03-12T20:27:53.873Z
Learning: In netbox_librenms_plugin/librenms_api.py, enforce that get_device_inventory() and get_inventory_filtered() always return a list of dicts. Validate as: inventory must be a list and every item must be a dict; if not, log a warning with the raw payload and return (False, error_message). Do not weaken the check to just verify a list type. This should prevent downstream AttributeError/TypeError when callers call .get() on items.
Applied to files:
netbox_librenms_plugin/librenms_api.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-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.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_librenms_api.pynetbox_librenms_plugin/tests/test_verify_views.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 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/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.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/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.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/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.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
🪛 ast-grep (0.44.1)
netbox_librenms_plugin/utils.py
[warning] 144-144: 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.compile(value)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
netbox_librenms_plugin/tests/test_librenms_api.py
[info] 1005-1005: no timeout was given on call to external resource
Context: patch("netbox_librenms_plugin.librenms_api.requests.get")
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
[info] 1018-1018: no timeout was given on call to external resource
Context: patch("netbox_librenms_plugin.librenms_api.requests.get")
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
[info] 1049-1049: no timeout was given on call to external resource
Context: patch("netbox_librenms_plugin.librenms_api.requests.get")
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
netbox_librenms_plugin/tests/test_verify_views.py
[info] 51-51: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 303-303: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 426-433: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"device_id": device.pk,
"ip_address": "1.2.3.4/24",
"object_type": "device",
"server_key": "evil-namespace", # not a configured server
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 509-509: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 722-729: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"device_id": out_of_scope.pk,
"interface_name": "Gi0/0",
"interface_name_field": "ifName",
"server_key": self.SERVER_KEY,
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 797-797: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"device_id": device_pk, "vid_group_map": {"10": 3}, "server_key": self.SERVER_KEY})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 863-863: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"device_id": 999_999_999, "ent_physical_index": 1, "server_key": "default"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
netbox_librenms_plugin/tests/test_coverage_device_operations.py
[warning] 1873-1873: Do not make http calls without encryption
Context: "http://d"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 1900-1900: Do not make http calls without encryption
Context: "http://d"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[info] 1880-1883: no timeout was given on call to external resource
Context: patch(
"netbox_librenms_plugin.librenms_api.requests.get",
side_effect=RequestsConnectionError("offline"),
)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
[info] 1905-1908: no timeout was given on call to external resource
Context: patch(
"netbox_librenms_plugin.librenms_api.requests.get",
side_effect=RequestsConnectionError("offline"),
)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
netbox_librenms_plugin/tests/test_coverage_base_views2.py
[info] 728-728: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"device_id": device.pk, "local_port_id": 10, "server_key": "ghost"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
netbox_librenms_plugin/tests/test_cable_verify.py
[info] 57-57: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 234-234: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 367-367: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"device_id": device.pk, "local_port_id": "1", "server_key": "prod"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 360-360: Do not make http calls without encryption
Context: "http://prod.example"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
netbox_librenms_plugin/tests/test_coverage_actions.py
[info] 866-866: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 4757-4757: Do not make http calls without encryption
Context: "http://p"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 4774-4774: Do not make http calls without encryption
Context: "http://p"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 4793-4793: Do not make http calls without encryption
Context: "http://d"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🔇 Additional comments (19)
docs/usage_tips/interface_mappings.md (1)
32-32: LGTM!Also applies to: 113-113, 122-122
netbox_librenms_plugin/utils.py (1)
165-171: LGTM!netbox_librenms_plugin/tests/test_utils.py (1)
1490-1509: LGTM!netbox_librenms_plugin/import_utils/bulk_import.py (1)
9-14: LGTM!Also applies to: 125-128, 145-154, 177-177, 432-444, 462-479, 497-511, 512-526
netbox_librenms_plugin/tests/test_coverage_bulk_import.py (1)
132-140: LGTM!Also applies to: 954-957, 1050-1069, 1082-1085, 1087-1088, 1103-1108, 2335-2365, 2399-2458
netbox_librenms_plugin/views/base/cables_view.py (1)
279-283: LGTM!netbox_librenms_plugin/tests/test_coverage_base_views2.py (1)
17-18: LGTM!Also applies to: 44-69, 138-155, 555-563, 712-717, 718-751, 752-764, 826-826, 847-852, 853-869, 898-898, 920-936, 969-969, 1017-1041, 1202-1218, 1219-1287, 1385-1443, 1504-1506, 1749-1754, 1939-1944, 1945-1961, 1993-1993, 2041-2046, 2047-2061, 2079-2079, 2096-2097, 2114-2115
netbox_librenms_plugin/tests/test_cable_verify.py (1)
4-18: LGTM!Also applies to: 20-49, 51-64, 66-79, 81-126, 127-162, 164-195, 196-224, 225-241, 243-254, 255-284, 286-334, 336-383, 385-406, 407-420, 421-429
netbox_librenms_plugin/librenms_api.py (1)
12-17: LGTM!Also applies to: 33-48, 71-100, 101-122, 212-219, 404-427, 446-448, 745-754
netbox_librenms_plugin/tests/test_librenms_api.py (1)
60-107: LGTM!Also applies to: 141-183, 184-245, 988-1018, 1019-1049, 1320-1340
netbox_librenms_plugin/tests/test_verify_views.py (2)
11-58: LGTM!Also applies to: 63-175, 180-258, 263-404, 406-465, 468-602, 604-632, 635-665, 667-751, 753-848
853-888: LGTM!
TestSingleModuleVerifyPermissionOrdercorrectly exercises the gate-before-lookup contract: a real non-superuser with nodcim.view_devicegrant posting a non-existentdevice_idgets403(notHttp404), matchingSingleModuleVerifyView.post()callingrequire_object_permissions_json("POST")beforerestrict_object_or_404(Device, ...). This mirrors the sibling ordering hardening inSingleInterfaceVerifyViewandSingleCableVerifyView.netbox_librenms_plugin/views/imports/actions.py (3)
1324-1332: LGTM!
1028-1043: LGTM!
1928-1931: LGTM!netbox_librenms_plugin/tests/test_coverage_actions.py (2)
3201-3268: LGTM!
859-869: LGTM!netbox_librenms_plugin/tests/test_coverage_device_operations.py (2)
1818-1851: LGTM!
1866-1892: LGTM!
marcinpsk
commented
Jul 21, 2026
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/import_utils/device_operations.py`:
- Around line 351-353: Defensively normalize serial values to strings before
stripping them: update the incoming serial extraction near lines 351-353 and the
serial extraction near lines 925-928 in
netbox_librenms_plugin/import_utils/device_operations.py to cast the retrieved
value with str(...), preserving the existing empty-value and "-" normalization
behavior. Reuse a single normalized value where appropriate to avoid repeating
the extraction across related serial handling paths.
🪄 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: 58bcd465-f038-4fe1-b3b8-9e5620951181
📒 Files selected for processing (2)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_device_operations.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Analyze (python)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.14)
🧰 Additional context used
📓 Path-based instructions (2)
**/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
**/*.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/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_device_operations.py
🧠 Learnings (19)
📚 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/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_device_operations.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/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_device_operations.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/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_device_operations.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/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_device_operations.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/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_device_operations.py
📚 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/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/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/device_operations.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-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_device_operations.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_device_operations.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_device_operations.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_coverage_device_operations.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_coverage_device_operations.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_coverage_device_operations.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_coverage_device_operations.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_coverage_device_operations.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_device_operations.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_device_operations.py
🔇 Additional comments (2)
netbox_librenms_plugin/tests/test_coverage_device_operations.py (1)
1818-1930: LGTM!netbox_librenms_plugin/import_utils/device_operations.py (1)
138-140: 📐 Maintainability & Code Quality
bulk_import_devices_sharedis not part ofdevice_operations.py. It lives innetbox_librenms_plugin/import_utils/bulk_import.pyand is re-exported fromnetbox_librenms_plugin/import_utils/__init__.py.> Likely an incorrect or invalid review comment.
marcinpsk
commented
Jul 21, 2026
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
_refresh_existing_device()'s generic exception handler only logged, so a transient failure (e.g. a DB error inside the librenms_id/name lookups) left a previously-cached "importable" row importable — letting a duplicate import slip through exactly when the duplicate check couldn't run. Every other failure branch of this function already fails closed (vanished link, deleted device). The handler now forces can_import/is_ready False and appends a blocking issue message (issues, not just a warning — recalculate would silently re-enable anything weaker). Red->green via the extended exception-path test.
… match _refresh_existing_device()'s newly-imported-device recheck populated the device_role display from the matched device but left validation["cluster"] untouched on a VM match, so the existing-match UI kept showing a stale or empty cluster for a VM that in fact has one. Mirror the device-role handling: apply the matched VM's cluster, or reset the cluster display when the VM has none. Import gating is unaffected — the row stays force-blocked either way.
The three screenshots in interface_mappings.md rendered with empty alt text (MD045), which screen readers announce as bare images.
The link/update/update_serial actions all select_for_update() the row that already owns the incoming serial before deciding; sync_serial's re-check ran unlocked, so a concurrent action re-pointing that row's serial could race the guard. Lock it the same way so the check serializes when a conflicting row exists (the no-DB-unique-constraint design otherwise stands). Real-DB test drives DeviceConflictActionView.post end-to-end and asserts the conflict lookup runs FOR UPDATE via CaptureQueriesContext.
A LibreNMS port row can lack the selected name field entirely (port.get(interface_name_field) -> None) or carry a non-string value; re.match() raises TypeError on those, which the except tuple deliberately doesn't cover — so one nameless row aborted the whole VC interface render with a 500. Guard non-string names up front and fall back to the viewed device like any other unmatchable name.
…VerifyView
SingleModuleVerifyView.post() resolved the Device with get_object_or_404
before running require_object_permissions_json("POST"), so a caller
without dcim.view_device could tell an existing device (403 after the
lookup) from a missing one (404) — a device-enumeration oracle. Move the
object-permission check ahead of the lookup, matching the sibling verify
views, so any device_id yields 403 for unauthorized callers.
enrich_remote_port() returned link only inside the 'if remote_port' block, so an LLDP/CDP neighbor that resolves to a NetBox device but advertises no remote port made it return None. process_remote_device() propagated that None, and enrich_links_data() then called link.get(...) on it, raising AttributeError and taking down the whole Cables tab. Return link outside the conditional so enrichment is skipped but the link always survives.
… failure
get_device_ips() let an HTTP 404 fall into the generic RequestException handler and
returned (False, ...), so a device with no IP addresses in LibreNMS surfaced as a red
"Failed to fetch IP addresses from LibreNMS" error instead of just an empty table.
LibreNMS 404s /devices/{id}/ip for a device that simply has no IPs (the same way /links
404s for a device with no LLDP neighbours). Catch HTTPError and map 404 to (True, []),
mirroring get_device_links; any other status remains a genuine failure.
The serial-identity duplicate check queried Device.objects.filter(serial=...) with the raw LibreNMS serial, but SNMP-sourced serials often carry trailing/leading whitespace, so an existing device that stored the trimmed serial was missed and the import would mint a duplicate. Strip the incoming serial before matching, mirroring the _serial_now normalization the validator already computes downstream. Real-DB regression test.
…and persist The earlier serial-strip only trimmed the identity-match branch: the drift checks still compared the raw incoming serial and import_single_device persisted it raw. So a padded LibreNMS serial produced a spurious "serial differs" warning and was stored padded, missing the next trimmed filter(serial=...) and recreating duplicates. Strip the incoming serial at every use — the match lookup, both drift checks, and the persisted value — so a match, a comparison, and the stored serial can't disagree on whitespace. Real-DB drift and persistence regression tests.
The serial-normalization strip assumed a string, but an all-digit serial can arrive as an int from LibreNMS JSON — (42 or "").strip() then raises AttributeError, which the validator swallows into a partial result and misses the match. Cast to str() at every serial normalization site before .strip(). Real-DB regression test with a numeric serial.
NetBox 4.5 added a str-tolerant branch to IPAddress.family; on 4.4 the
property is just self.address.version, and a freshly constructed
IPAddress(address="10.0.0.1/24") keeps the plain str in memory even
after save(), which never runs to_python coercion. Two flows broke on
NetBox 4.4 only:
- _set_primary_ip read ip_obj.family directly. AttributeError ('str'
object has no attribute 'version') failed the whole IP-sync row for
every newly created address.
- set_device_ip_fk's family guard read getattr(ip, "family", None).
getattr swallows the AttributeError raised inside the property, so
the family read as None and the guard refused valid same-family
addresses with the non-IPv4/non-IPv6 message.
New ip_family() helper mirrors the >= 4.5 property (str parsed with
netaddr, else address.version, empty means None); both sites use it.
Red->green via tests that pin NetBox 4.4's family property
implementation, plus direct helper tests.
…ocking conflict-row lock Two review findings on the serial hardening: - The interactive write paths (update / update_serial / sync_serial) and the _build_sync_info drift display still used the raw LibreNMS serial while validate/import_single_device compare and persist it trimmed. A padded serial was stored padded (so the next trimmed filter(serial=...) missed it and could mint a duplicate), missed trimmed-stored conflicts, and reported permanent drift in the modal. All four sites now use str(...).strip(), matching the match/drift/persist normalization. - The conflict guard select_for_update()'d the conflicting-serial row while the target row was already locked. Two swap-direction requests (A gets B's serial while B gets A's) lock their targets first and then each other's conflict row - a PostgreSQL deadlock that surfaces as a 500. The guard now serializes on a transaction-scoped pg_advisory_xact_lock(hashtext(serial)) taken before an unlocked conflict lookup: same-serial writers fully serialize (also closing the both-pass race the row lock never covered while the serial was still unassigned), and without a second row lock there is no lock cycle. Red->green: real-DB action tests for trimmed persist/conflict handling and the advisory-lock mechanism; the old row-lock mechanism test is rewritten to pin the new guard. Mock-based action tests retarget the conflict-lookup chain and stub the advisory helper, which needs a real connection.
DeviceConflictActionView and AddAsOOBView resolved the POSTed existing_device_id
against the plain manager, and their ("change", Model) gate only asks the
model-level permission with no instance. A user holding a constrained grant
(site- or pk-scoped change_device) therefore cleared the gate and could link,
update or OOB-attach any device by raw pk.
Both now gate first, then resolve through restricted_queryset(model, "change") —
the same fail-closed pattern the sync/verify views already use — so an
out-of-scope pk reports "Existing device not found" like a nonexistent one, while
a caller with no change permission at all still gets the named-permission error.
Generalises the reviewer-flagged class from the promote/merge endpoints to the
develop-owned twins in the same file.
LibreNMS returns an all-digit serial as a JSON number, so `.strip()` on the raw value raises AttributeError. Five reads still lacked the cast that cf06fbe added elsewhere: - device_operations `_serial_now` (Stage-1 duplicate guard) — the validator swallows the crash into a generic "Validation error" issue and blocks the row. - device_operations `_serial_for_pair` (Stage-2 merge pairing) — the crash is caught by the defensive handler, so the merge suggestion silently disappears. - bulk_import `_refresh_existing_device` serial fallback — the refresh aborts and the row is blocked as a transient lookup failure instead of rebinding. - modules_view transceiver merge and device_fields' legacy-id serial gate. Tests cover the four sites a real flow reaches; each fails against the uncast code with the AttributeError or its swallowed symptom.
…ion test NetBox 4.4's pre_delete `clear_primary_ip` builds its field name from `instance.family`, and 4.4's `family` dereferences `self.address.version` without the str guard later versions have. The instance here still carries the str it was constructed with, so the delete raised `AttributeError: 'str' object has no attribute 'version'` on the 4.4 CI job. Refreshing from the DB first coerces `address` to an IPNetwork; the test's intent is unchanged. Reproduced locally by patching `IPAddress.family` to the 4.4 shape. No production code deletes an IPAddress instance built in-flow, so nothing else needs the same treatment.
The update, update_serial and sync_serial actions each repeated the same sequence: take the advisory lock on the serial value, look for another device already holding it, return an HTMX conflict toast, otherwise assign. Three copies meant three places to keep in step — sync_serial had already grown a warning log the other two lacked. _apply_conflict_checked_serial() now owns that sequence. Each branch keeps its own update_fields bookkeeping and sync_serial still passes its re-fetched locked row, so the lock shape and messages are unchanged; the existing advisory-lock and conflict-guard tests pass untouched.
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 (1)
netbox_librenms_plugin/tests/test_coverage_actions.py (1)
3756-3757: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThese comments now contradict the behavior they annotate.
Both still claim the conflict lookup runs under
select_for_update(), but the stubs below them were changed to the unlockedfilter().exclude().first()chain precisely because_apply_conflict_checked_serialdeliberately does not row-lock the conflicting row — that's the documented deadlock-avoidance rationale (actions.pylines 231-232), andtest_sync_serial_conflict_guard_uses_advisory_lock_not_row_lockasserts it. Leaving the stale wording invites someone to "restore" the row lock and reintroduce the A→B/B→A deadlock.📝 Proposed comment corrections
- # The conflict lookup runs under select_for_update() too (row lock on the conflicting device). + # The conflict lookup is NOT row-locked: the advisory lock on the serial value serializes + # same-serial writers instead (see _apply_conflict_checked_serial). MockDevice.objects.filter.return_value.exclude.return_value.first.return_value = conflict_device- # The conflict lookup runs under select_for_update() too (row lock on the conflicting device). + # The conflict lookup is NOT row-locked (advisory lock on the serial value instead). MockDevice.objects.filter.return_value.exclude.return_value.first.return_value = NoneAlso applies to: 3785-3786
🤖 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_actions.py` around lines 3756 - 3757, Update the comments above the conflict lookup stubs in the relevant test cases, including the block near test_sync_serial_conflict_guard_uses_advisory_lock_not_row_lock, to state that the lookup intentionally uses the unlocked filter().exclude().first() chain and does not use select_for_update(), preserving the documented deadlock-avoidance behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 1336-1338: Update the serial normalization in the
transaction-processing flow to treat only None from txr.get("serial") as
missing, preserving falsey numeric values such as 0 before converting and
stripping the string. Keep the existing _PLACEHOLDER_VALUES check unchanged.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 273-276: Update the serial-conflict response in the relevant
import action to pass an escaped value of conflict_device.name when constructing
the message for _htmx_error_response. Match the nearby OOB conflict messages’
existing escape() usage while leaving the rest of the conflict text unchanged.
---
Outside diff comments:
In `@netbox_librenms_plugin/tests/test_coverage_actions.py`:
- Around line 3756-3757: Update the comments above the conflict lookup stubs in
the relevant test cases, including the block near
test_sync_serial_conflict_guard_uses_advisory_lock_not_row_lock, to state that
the lookup intentionally uses the unlocked filter().exclude().first() chain and
does not use select_for_update(), preserving the documented deadlock-avoidance
behavior.
🪄 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 Plus
Run ID: 1e48e52e-d941-4701-a5f9-bdc3d404b625
📒 Files selected for processing (10)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/device_fields.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: NetBox main / Python 3.13
- GitHub Check: NetBox main / Python 3.14
- GitHub Check: NetBox v4.6.5 / Python 3.12
- GitHub Check: NetBox v4.6.5 / Python 3.13
- GitHub Check: NetBox v4.4.0 / Python 3.12
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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_device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/modules_view.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/device_fields.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 (30)
📚 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_device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_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/tests/test_coverage_device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_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/tests/test_coverage_device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-03-12T12:14:03.173Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/tests/test_coverage_device_fields.py:643-673
Timestamp: 2026-03-12T12:14:03.173Z
Learning: In netbox_librenms_plugin/tests/test_coverage_device_fields.py, strengthen the CreateAndAssignPlatformView success-path tests (test_manufacturer_not_found around lines 642–672 and the adjacent test around lines 674–697) by asserting that the newly created Platform instance is assigned to the locked device object (mock_locked.platform is mock_platform_instance) and that mock_locked.save() is called. This validates the FK assignment and persistence, rather than only checking that messages.success() was invoked. This improvement is a known backlog item (low priority); do not re-raise as a new finding.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_device_fields.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_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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_device_fields.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/device_fields.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/device_fields.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-07-02T13:36:15.226Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/interfaces_view.py:226-226
Timestamp: 2026-07-02T13:36:15.226Z
Learning: When reviewing netbox-librenms-plugin view code that handles POSTed `server_key`, treat the plugin-wide convention as intentional: if the posted `server_key` is not present in `LibreNMSAPI.get_available_servers()`, the request should fall back to the currently configured default/active server key (not reject/fail-closed and not treat it as an error for that single view). Do not flag individual instances of this fallback pattern as incorrect “invalid server_key” validation. Any change to fail-closed behavior must be a coordinated cross-cutting change applied uniformly across all affected sites/views, not a one-view patch.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/imports/actions.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/modules_view.pynetbox_librenms_plugin/views/sync/device_fields.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/modules_view.py
📚 Learning: 2026-06-25T07:14:19.587Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/modules_view.py:1073-1074
Timestamp: 2026-06-25T07:14:19.587Z
Learning: In netbox_librenms_plugin/views/base/modules_view.py and netbox_librenms_plugin/views/sync/modules.py, treat LibreNMS `entPhysicalIndex` as an end-to-end integer invariant (it originates from an LibreNMS int DB column and is preserved as an int through the module inventory/sync pipeline). When reviewing code, do not flag mixed string/int `entPhysicalIndex` handling or request additional `int()` normalization solely as a defensive measure against string indices. Only recommend `int()` conversion/normalization if there is concrete evidence in the code path that values are actually being converted to strings (e.g., explicit casts, JSON serialization/deserialization steps that coerce to strings, or external inputs known to provide strings).
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-06-26T09:04:49.793Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_verify_views.py:281-343
Timestamp: 2026-06-26T09:04:49.793Z
Learning: When implementing per-object permission resolution in verify views (e.g., like `SingleIPAddressVerifyView`), `_required_perms_for_object` should: (1) if `object_type` is explicit, gate on the exact model permission for that target type; (2) if `object_type` is not explicit, resolve the object id to its model without reading the object’s data (avoid fetching the object just to determine permissions); and (3) in ambiguous cases, fail closed by requiring all relevant view permissions (deny unless both applicable permissions are satisfied). Add/extend DB-backed tests to cover allow/deny paths and the “no `object_type`” case.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.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/device_fields.py
📚 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/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.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/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.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/device_operations.pynetbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.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
🪛 ast-grep (0.45.0)
netbox_librenms_plugin/tests/test_coverage_actions.py
[error] 6964-6964: Lack of sanitization of user data
Context: HttpResponse(b"row-ok")
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(http-response-from-request)
[error] 6993-6993: Lack of sanitization of user data
Context: HttpResponse(b"row-ok")
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(http-response-from-request)
🔇 Additional comments (17)
netbox_librenms_plugin/import_utils/bulk_import.py (4)
9-14: LGTM!
792-792: LGTM!
861-870: LGTM!
904-916: LGTM!netbox_librenms_plugin/tests/test_modules_view.py (1)
131-145: LGTM!netbox_librenms_plugin/views/imports/actions.py (4)
1401-1402: LGTM!
1598-1611: LGTM!
1706-1707: LGTM!Also applies to: 1727-1728, 1771-1773
2469-2479: 🔒 Security & PrivacyWorth confirming the VC sync-device redirection stays inside the granted scope.
The restricted lookup scopes
existing_device, but line 2503 then resolvessync_device = get_librenms_sync_device(existing_device, ...), and that (possibly different) VC member is what gets locked and saved. A pk-constrained grant covering only the selected member would still write to the sibling sync member. Not raising this as a defect — per learnings, per-object scoping gaps in this plugin are an intentional plugin-wide item deferred to a dedicated PR, and this change narrows the surface rather than widening it. Flagging only so the VC case is a conscious decision here.Based on learnings:
check_object_permissions/NetBoxObjectPermissionMixinenforce only model-level permissions and per-object scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.Source: Learnings
netbox_librenms_plugin/import_utils/device_operations.py (1)
730-732: LGTM!Also applies to: 753-754, 824-825, 853-858, 917-917, 987-987
netbox_librenms_plugin/views/sync/device_fields.py (1)
949-949: LGTM!netbox_librenms_plugin/tests/test_coverage_device_fields.py (1)
2167-2198: LGTM!netbox_librenms_plugin/tests/test_coverage_device_operations.py (1)
1390-1394: LGTM!Also applies to: 1405-1426, 1979-1981, 2505-2527, 2824-2824
netbox_librenms_plugin/tests/test_coverage_actions.py (3)
3802-3863: LGTM!
6908-7059: LGTM!
1273-1273: LGTM!Also applies to: 1695-1695, 2180-2180, 2225-2225, 3138-3143, 3352-3357, 3590-3595, 3678-3683, 5501-5501, 5548-5548, 5791-5791
netbox_librenms_plugin/tests/test_import_utils.py (1)
2696-2701: LGTM!Also applies to: 2793-2795, 2840-2840, 2884-2886, 2935-2937, 2975-2977, 3019-3021, 3060-3062, 3101-3103, 3135-3137, 3178-3180, 3233-3235, 3288-3290, 3323-3325, 3363-3365, 3399-3401
str(value or "").strip() silently dropped the falsey-but-real JSON-number serial 0 while keeping the string form "0", so the same device matched or not depending on how LibreNMS serialized it. New normalize_serial() helper (None -> "", else str(value).strip()) replaces the hand-rolled idiom at all 16 serial read sites, so the next fix can't land on some copies and miss others again. Call sites keep their own "-" placeholder guards.
…mments
The conflict toast interpolates the conflicting device's name via
_htmx_error_response, which substitutes the message through
format_html('{}', ...) — Django escapes substitution args, so a marked-up
device name renders escaped and adding escape() at the call site would
double-escape. Pin that with a test.
Two test comments still claimed the conflict lookup runs under
select_for_update(); it is deliberately unlocked (advisory lock on the
serial value) to avoid the swap-direction row-lock deadlock — reword them
so nobody 'restores' the lock.
…rvives fc133ed's sweep matched the .get("serial") call sites but missed this local helper (12 call sites in the VC detection path), which still dropped the falsey-but-real JSON-number serial 0 via str(s or ""). Delegate to normalize_serial and keep the '-' placeholder handling.
Rows imported before the serial normalization landed persisted the raw LibreNMS value, so they can store space-padded serials. The trimmed lookups introduced with the normalization can't exact-match those rows: the identity match treats the hardware as new (duplicate risk) and the conflict guards pass a padded conflict row. New filter_by_trimmed_serial() helper (annotate TRIM(serial), compare to the trimmed needle) replaces the exact filter at the six Device-serial lookup sites: identity match, both drift-conflict lookups, the Stage-2 pair check, the bulk refresh fallback, the VC pre-import exists() check, and the conflict-checked assignment helper. Module-serial lookups are untouched (different flow, no legacy padding evidence).
…_by_trimmed_serial filter_by_trimmed_serial() turned the Device serial lookups into Device.objects.all().annotate(TRIM(serial)).filter(_serial_trimmed=...). On a MagicMock manager that forks a fresh auto-mock instead of reaching each test's filter pin, so every no-conflict case saw a truthy row and reported a serial conflict; kwargs-inspecting side effects stopped recognising the lookup because the key is now _serial_trimmed; and the real-DB query-count assertion matched a predicate the TRIM form no longer emits. 23 tests were failing. collapse_queryset_chain() folds .all()/.annotate() back onto the mocked manager the way a real queryset clones and returns, so the existing pins keep applying on the trimmed path; _is_serial_lookup() accepts either kwarg form; the SQL matcher looks for the trimmed predicate.
The POSTed existing_device_id is resolved through the restricted change queryset, but the OOB link is written to the VC sync device that get_librenms_sync_device() derives from it — resolved, locked and saved from the plain manager. A grant covering only the member the OOB candidate matched therefore attached the link (and, on the IP path, an oob_ip) to the sibling that holds the chassis' librenms_id and that the grant excludes. Verify the derived row against the same restricted queryset before the write, reusing the flow's "Existing device not found" surface. The check is skipped when the sync device is the selected row, so the non-VC path adds no query.
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/import_utils/device_operations.py`:
- Around line 755-763: Normalize both sides of serial comparisons using the
existing serial-normalization helper. In device_operations.py ranges 755-763 and
828-834, normalize existing_device.serial before the librenms_id-matched and
hostname-matched conflict checks. In actions.py range 1401-1410, normalize the
netbox_serial value in _build_sync_info before computing serial_synced; apply
the corresponding changes at all three sites while preserving current comparison
behavior.
🪄 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 Plus
Run ID: 2355c596-57ca-4f8d-878c-e227a3c44bfb
📒 Files selected for processing (13)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/device_fields.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: NetBox v4.4.0 / Python 3.12
- GitHub Check: NetBox v4.6.5 / Python 3.13
- GitHub Check: NetBox v4.6.5 / Python 3.12
- GitHub Check: NetBox main / Python 3.14
- GitHub Check: NetBox main / Python 3.13
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/conftest.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/device_fields.py
**/import_utils/virtual_chassis.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
virtual_chassis.pymust export:create_virtual_chassis_with_members()and_sync_module_bay_counter()
Files:
netbox_librenms_plugin/import_utils/virtual_chassis.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/modules_view.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 (33)
📚 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/conftest.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/conftest.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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/device_fields.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/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.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/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-07-02T13:36:15.226Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/interfaces_view.py:226-226
Timestamp: 2026-07-02T13:36:15.226Z
Learning: When reviewing netbox-librenms-plugin view code that handles POSTed `server_key`, treat the plugin-wide convention as intentional: if the posted `server_key` is not present in `LibreNMSAPI.get_available_servers()`, the request should fall back to the currently configured default/active server key (not reject/fail-closed and not treat it as an error for that single view). Do not flag individual instances of this fallback pattern as incorrect “invalid server_key” validation. Any change to fail-closed behavior must be a coordinated cross-cutting change applied uniformly across all affected sites/views, not a one-view patch.
Applied to files:
netbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.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/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.py
📚 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/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.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/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.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/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.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_modules_view.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_import_utils.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-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/modules_view.py
📚 Learning: 2026-06-25T07:14:19.587Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/modules_view.py:1073-1074
Timestamp: 2026-06-25T07:14:19.587Z
Learning: In netbox_librenms_plugin/views/base/modules_view.py and netbox_librenms_plugin/views/sync/modules.py, treat LibreNMS `entPhysicalIndex` as an end-to-end integer invariant (it originates from an LibreNMS int DB column and is preserved as an int through the module inventory/sync pipeline). When reviewing code, do not flag mixed string/int `entPhysicalIndex` handling or request additional `int()` normalization solely as a defensive measure against string indices. Only recommend `int()` conversion/normalization if there is concrete evidence in the code path that values are actually being converted to strings (e.g., explicit casts, JSON serialization/deserialization steps that coerce to strings, or external inputs known to provide strings).
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-06-26T09:04:49.793Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_verify_views.py:281-343
Timestamp: 2026-06-26T09:04:49.793Z
Learning: When implementing per-object permission resolution in verify views (e.g., like `SingleIPAddressVerifyView`), `_required_perms_for_object` should: (1) if `object_type` is explicit, gate on the exact model permission for that target type; (2) if `object_type` is not explicit, resolve the object id to its model without reading the object’s data (avoid fetching the object just to determine permissions); and (3) in ambiguous cases, fail closed by requiring all relevant view permissions (deny unless both applicable permissions are satisfied). Add/extend DB-backed tests to cover allow/deny paths and the “no `object_type`” case.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.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
🔇 Additional comments (14)
netbox_librenms_plugin/views/sync/device_fields.py (1)
27-27: LGTM!Also applies to: 950-950
netbox_librenms_plugin/views/base/modules_view.py (1)
19-19: LGTM!Also applies to: 1337-1337
netbox_librenms_plugin/tests/test_modules_view.py (1)
147-162: LGTM!netbox_librenms_plugin/tests/test_coverage_virtual_chassis.py (1)
8-9: LGTM!Also applies to: 64-64, 115-115, 162-162, 207-207, 253-253, 292-292, 391-411
netbox_librenms_plugin/import_utils/bulk_import.py (2)
9-22: LGTM!Also applies to: 264-268, 794-801
904-918: LGTM!netbox_librenms_plugin/import_utils/device_operations.py (1)
730-739: LGTM!Also applies to: 859-886, 923-945, 993-1037, 1623-1628
netbox_librenms_plugin/import_utils/virtual_chassis.py (1)
11-11: LGTM!Also applies to: 407-410, 523-556
netbox_librenms_plugin/tests/test_coverage_device_operations.py (1)
1960-2023: LGTM!Also applies to: 2029-2079, 2546-2568, 3162-3197
netbox_librenms_plugin/views/imports/actions.py (1)
46-47: LGTM!Also applies to: 171-201, 246-280, 1563-1785, 2450-2745
netbox_librenms_plugin/tests/test_coverage_actions.py (1)
8-15: LGTM!Also applies to: 2706-2714, 2962-3010, 3144-3357, 3598-3878, 6839-7146
netbox_librenms_plugin/tests/conftest.py (1)
201-212: LGTM!netbox_librenms_plugin/tests/test_import_utils.py (1)
12-13: LGTM!Also applies to: 30-37, 1786-1786, 1802-2261, 2280-2308, 2709-2714, 2778-3438
netbox_librenms_plugin/utils.py (1)
1153-1184: LGTM!Also applies to: 1880-1965
marcinpsk
commented
Jul 30, 2026
| with stack: | ||
| MockDevice.objects.select_for_update.return_value.get.return_value = locked_device | ||
| # The conflict lookup is deliberately UNLOCKED (advisory lock on the serial value instead); | ||
| # a second row lock would deadlock two swap-direction requests (A→B / B→A). |
Owner
Author
There was a problem hiding this comment.
Resolution for the review-body finding (comments still claiming the conflict lookup runs under select_for_update()): fixed in 609039c — both comments now state the lookup is deliberately unlocked (advisory lock on the serial value) to avoid the swap-direction row-lock deadlock. The earlier outside-diff str-cast finding was fixed in 3275b4a (see the inline thread).
@coderabbitai
…arison
The incoming LibreNMS serial was normalized while the NetBox-stored value was
read raw, so a legacy whitespace-padded stored serial reported false drift
("Hardware may have been replaced") on the librenms_id-matched and
hostname-matched paths, and as unsynced in the validation-details modal.
The stored value is normalized at comparison time only; nothing is rewritten.
Also coerces LibreNMS model/type values in the transceiver merge the same way
serial already was: an all-digit value arrives as a JSON number, so a bare
.strip() raised AttributeError and 500'd the modules refresh, and the
`(value or "")` form dropped a real serial of 0.
pg_advisory_xact_lock is transaction-scoped: called in autocommit it is taken and released within the same statement, so the read-then-write conflict guard it serializes would silently run unprotected. Fail loudly instead.
There was a problem hiding this comment.
Actionable comments posted: 1
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/tests/test_coverage_device_operations.py (1)
2938-2946: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest helper still drops falsey serials instead of using the shared
normalize_serial().
_role()'sserial = str(libre_device.get("serial") or "").strip()reintroduces the exact "or ""drops real0" pattern this PR eliminates everywhere else (seenormalize_serial()'s docstring). It only feeds_detect_serial_match_role()'s warning-text formatting today, so nothing currently breaks, but it's an inconsistency in a test module dedicated to hardening this contract — a future test passing a zero/int serial through this helper would silently diverge from production behavior.♻️ Proposed fix
- existing_link = _describe_existing_librenms_link(existing_device, server_key) - serial = str(libre_device.get("serial") or "").strip() + from netbox_librenms_plugin.utils import normalize_serial + + existing_link = _describe_existing_librenms_link(existing_device, server_key) + serial = normalize_serial(libre_device.get("serial")) return _detect_serial_match_role(existing_device, existing_link, hostname, serial, libre_device, 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/tests/test_coverage_device_operations.py` around lines 2938 - 2946, Update the test helper method _role to obtain the serial through the shared normalize_serial() utility instead of using str(... or "").strip(). Preserve the normalized value when passing it to _detect_serial_match_role, including falsey serials such as numeric 0.
🤖 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/utils.py`:
- Around line 1153-1168: Add a database functional index for TRIM(serial) on the
serial-bearing Device model, with the corresponding migration, so
filter_by_trimmed_serial() can use an indexed equality lookup during bulk
imports while preserving its legacy padded-serial matching behavior.
---
Outside diff comments:
In `@netbox_librenms_plugin/tests/test_coverage_device_operations.py`:
- Around line 2938-2946: Update the test helper method _role to obtain the
serial through the shared normalize_serial() utility instead of using str(... or
"").strip(). Preserve the normalized value when passing it to
_detect_serial_match_role, including falsey serials such as numeric 0.
🪄 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 Plus
Run ID: c350bed7-1009-4d64-85d3-d90ece8b1ce3
📒 Files selected for processing (13)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/device_fields.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: NetBox main / Python 3.13
- GitHub Check: NetBox v4.6.5 / Python 3.12
- GitHub Check: NetBox v4.6.5 / Python 3.13
- GitHub Check: NetBox v4.4.0 / Python 3.12
- GitHub Check: NetBox main / Python 3.14
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/conftest.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.py
**/import_utils/virtual_chassis.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
virtual_chassis.pymust export:create_virtual_chassis_with_members()and_sync_module_bay_counter()
Files:
netbox_librenms_plugin/import_utils/virtual_chassis.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/device_fields.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/modules_view.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
**/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
🧠 Learnings (34)
📚 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/conftest.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_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/tests/conftest.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_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/tests/conftest.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.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/conftest.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 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/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.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/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.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/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.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/device_fields.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/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.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/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-07-02T13:36:15.226Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/interfaces_view.py:226-226
Timestamp: 2026-07-02T13:36:15.226Z
Learning: When reviewing netbox-librenms-plugin view code that handles POSTed `server_key`, treat the plugin-wide convention as intentional: if the posted `server_key` is not present in `LibreNMSAPI.get_available_servers()`, the request should fall back to the currently configured default/active server key (not reject/fail-closed and not treat it as an error for that single view). Do not flag individual instances of this fallback pattern as incorrect “invalid server_key” validation. Any change to fail-closed behavior must be a coordinated cross-cutting change applied uniformly across all affected sites/views, not a one-view patch.
Applied to files:
netbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.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/sync/device_fields.pynetbox_librenms_plugin/views/base/modules_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/modules_view.py
📚 Learning: 2026-06-25T07:14:19.587Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/modules_view.py:1073-1074
Timestamp: 2026-06-25T07:14:19.587Z
Learning: In netbox_librenms_plugin/views/base/modules_view.py and netbox_librenms_plugin/views/sync/modules.py, treat LibreNMS `entPhysicalIndex` as an end-to-end integer invariant (it originates from an LibreNMS int DB column and is preserved as an int through the module inventory/sync pipeline). When reviewing code, do not flag mixed string/int `entPhysicalIndex` handling or request additional `int()` normalization solely as a defensive measure against string indices. Only recommend `int()` conversion/normalization if there is concrete evidence in the code path that values are actually being converted to strings (e.g., explicit casts, JSON serialization/deserialization steps that coerce to strings, or external inputs known to provide strings).
Applied to files:
netbox_librenms_plugin/views/base/modules_view.py
📚 Learning: 2026-06-26T09:04:49.793Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/tests/test_verify_views.py:281-343
Timestamp: 2026-06-26T09:04:49.793Z
Learning: When implementing per-object permission resolution in verify views (e.g., like `SingleIPAddressVerifyView`), `_required_perms_for_object` should: (1) if `object_type` is explicit, gate on the exact model permission for that target type; (2) if `object_type` is not explicit, resolve the object id to its model without reading the object’s data (avoid fetching the object just to determine permissions); and (3) in ambiguous cases, fail closed by requiring all relevant view permissions (deny unless both applicable permissions are satisfied). Add/extend DB-backed tests to cover allow/deny paths and the “no `object_type`” case.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.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-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_virtual_chassis.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.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
🔇 Additional comments (19)
netbox_librenms_plugin/utils.py (2)
1153-1168: 🗄️ Data Integrity & Integration
filter_by_trimmed_serial()trims only spaces at the DB level, whilenormalize_serial()strips all whitespace in Python.Django's
Trim()compiles to plain SQLTRIM(...), which by default removes only the ASCII space character on PostgreSQL/MySQL/SQL Server/SQLite — it does not strip tabs, newlines, or other whitespace the way Python'sstr.strip()does. If a legacy device serial were padded with a non-space whitespace character (e.g. a stray tab from an old import),normalize_serial()would trim it in Python butfilter_by_trimmed_serial()would not consider it equal at the DB level, silently missing the "legacy padded serial" match this helper exists to catch. Given serials are typically plain alphanumeric, this is a narrow edge case, but worth confirming isn't hit in practice (e.g. via any known vendors emitting tab-padded serials).
1171-1183: LGTM!normalize_serial()correctly treats onlyNoneas absent while coercing numeric/falsey values (including0) viastr(value).strip(), matching the documented contract.netbox_librenms_plugin/import_utils/device_operations.py (1)
24-25: LGTM! Serial normalization is now consistently two-sided (incoming and stored) across the librenms_id-matched drift check, hostname-matched conflict check, strong serial-identity match, dedup/merge-candidate detection, and persisted value inimport_single_device(). This closes the previously-flagged "one-sided normalization" gap and is backed by solid regression tests (TestValidateSerialMatchStripsWhitespace,TestImportPersistsTrimmedSerial).Also applies to: 732-746, 756-782, 830-859, 862-896, 918-950, 992-1091, 1628-1633
netbox_librenms_plugin/import_utils/virtual_chassis.py (1)
11-11: LGTM!_norm_serial()now correctly layers on the sharednormalize_serial()(preserving numeric0, coercing non-str types) while keeping its own"-"placeholder handling, and the duplicate-serial guard now usesfilter_by_trimmed_serial()to catch legacy padded serials during VC member creation.Also applies to: 407-410, 527-527, 552-552
netbox_librenms_plugin/views/base/modules_view.py (1)
19-19: LGTM!_clean_librenms_value()correctly centralizes normalize+placeholder-blank logic and fixes a real latent crash: the prior raw.strip()onentPhysicalModelName/entPhysicalSerialNumwould have raisedAttributeErrorfor a numeric (int) ENTITY-MIB value; this is now safely coerced vianormalize_serial()first, matching the new test coverage (test_numeric_entity_values_on_the_existing_item_are_coerced,test_zero_transceiver_serial_is_preserved).Also applies to: 60-76, 236-236, 1323-1323, 1344-1346, 1358-1362
netbox_librenms_plugin/import_utils/bulk_import.py (1)
9-22: LGTM! The stack-dedup serial fingerprint, refresh-time serial fallback, matched-VM cluster mirroring, and fail-closed exception handling are all correctly implemented and consistent with the shared normalization contract and the PR's stated fail-closed intent.Also applies to: 267-267, 794-796, 863-872, 906-918
netbox_librenms_plugin/tests/test_coverage_device_operations.py (1)
1906-2159: LGTM! Thorough real-DB regression coverage for whitespace-padded incoming/stored serials, numeric (int) serials,0-serial preservation, and VC member-name generation — directly validates the fixes indevice_operations.py.Also applies to: 2626-2648, 3242-3277
netbox_librenms_plugin/views/sync/device_fields.py (1)
27-27: LGTM!Also applies to: 950-950
netbox_librenms_plugin/tests/test_modules_view.py (1)
147-207: LGTM!Also applies to: 5096-5135
netbox_librenms_plugin/tests/conftest.py (1)
201-210: LGTM!netbox_librenms_plugin/tests/test_coverage_virtual_chassis.py (1)
8-9: LGTM!Also applies to: 64-64, 115-115, 162-162, 207-207, 253-253, 292-292, 391-411
netbox_librenms_plugin/tests/test_import_utils.py (1)
30-38: LGTM!Also applies to: 1786-1786, 1812-2233, 2295-2295
netbox_librenms_plugin/views/imports/actions.py (3)
46-47: LGTM!Also applies to: 226-248, 251-285
1405-1415: LGTM!Also applies to: 1709-1715, 1731-1736, 1771-1781
2512-2519: Correctly closes a VC-sync-device authorization gap.The new gate authorizes the derived
sync_device(which may differ from the user-selectedexisting_devicefor VC members) before it is locked and written to, preventing a grant scoped only to the selected member from writing an OOB link onto an unauthorized VC sibling. Well covered by the newTestConflictActionsObjectScopeVC-pair tests intest_coverage_actions.py.netbox_librenms_plugin/tests/test_coverage_actions.py (4)
8-15: LGTM!Also applies to: 991-1002
3009-3009: LGTM!Also applies to: 3113-3113, 3206-3206, 3415-3415, 3646-3646, 3723-3723, 3781-3782, 3811-3812, 3930-3930
6923-6963: LGTM!
7117-7169: LGTM!
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)
netbox_librenms_plugin/import_utils/bulk_import.py (1)
905-917: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear the transient blocker after a successful retry.
This issue is never removed. A later successful refresh still leaves it in
issues, so recalculation keeps the row blocked despite the “Refresh to retry” message. Remove the prior marker before rerunning the lookup, then re-add it only if the retry fails.Proposed fix
try: + retry_failure = "Duplicate re-check failed (transient lookup error); import blocked. Refresh to retry." + validation["issues"] = [ + issue for issue in validation.get("issues", []) if issue != retry_failure + ] + from dcim.models import Device from virtualization.models import VirtualMachine ... except Exception as e: logger.error(f"Failed to check for newly imported device: {e}") validation["can_import"] = False validation["is_ready"] = False - message = "Duplicate re-check failed (transient lookup error); import blocked. Refresh to retry." + message = retry_failure🤖 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 905 - 917, The duplicate re-check failure marker must be cleared before retrying validation so a successful refresh can restore importability. In the surrounding duplicate lookup/recalculation flow, remove the exact “Duplicate re-check failed (transient lookup error); import blocked. Refresh to retry.” entry from validation["issues"] before rerunning the lookup, then retain the existing deduplicated append only when the retry fails.
🤖 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/import_utils/bulk_import.py`:
- Around line 905-917: The duplicate re-check failure marker must be cleared
before retrying validation so a successful refresh can restore importability. In
the surrounding duplicate lookup/recalculation flow, remove the exact “Duplicate
re-check failed (transient lookup error); import blocked. Refresh to retry.”
entry from validation["issues"] before rerunning the lookup, then retain the
existing deduplicated append only when the retry fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0567cfcc-9cdc-4dbf-9718-e668b5e4f86f
📒 Files selected for processing (13)
netbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/device_fields.py
💤 Files with no reviewable changes (2)
- netbox_librenms_plugin/utils.py
- netbox_librenms_plugin/tests/test_coverage_virtual_chassis.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Analyze (python)
- GitHub Check: NetBox v4.6.5 / Python 3.13
- GitHub Check: NetBox v4.6.5 / Python 3.12
- GitHub Check: NetBox main / Python 3.14
- GitHub Check: NetBox main / Python 3.13
- GitHub Check: NetBox v4.4.0 / Python 3.12
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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/device_fields.py
**/import_utils/virtual_chassis.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
virtual_chassis.pymust export:create_virtual_chassis_with_members()and_sync_module_bay_counter()
Files:
netbox_librenms_plugin/import_utils/virtual_chassis.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 (28)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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/device_fields.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/sync/device_fields.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/sync/device_fields.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-07-02T13:36:15.226Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 116
File: netbox_librenms_plugin/views/base/interfaces_view.py:226-226
Timestamp: 2026-07-02T13:36:15.226Z
Learning: When reviewing netbox-librenms-plugin view code that handles POSTed `server_key`, treat the plugin-wide convention as intentional: if the posted `server_key` is not present in `LibreNMSAPI.get_available_servers()`, the request should fall back to the currently configured default/active server key (not reject/fail-closed and not treat it as an error for that single view). Do not flag individual instances of this fallback pattern as incorrect “invalid server_key” validation. Any change to fail-closed behavior must be a coordinated cross-cutting change applied uniformly across all affected sites/views, not a one-view patch.
Applied to files:
netbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/imports/actions.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/sync/device_fields.py
📚 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/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.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/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.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/virtual_chassis.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-03-12T12:14:03.173Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/tests/test_coverage_device_fields.py:643-673
Timestamp: 2026-03-12T12:14:03.173Z
Learning: In netbox_librenms_plugin/tests/test_coverage_device_fields.py, strengthen the CreateAndAssignPlatformView success-path tests (test_manufacturer_not_found around lines 642–672 and the adjacent test around lines 674–697) by asserting that the newly created Platform instance is assigned to the locked device object (mock_locked.platform is mock_platform_instance) and that mock_locked.save() is called. This validates the FK assignment and persistence, rather than only checking that messages.success() was invoked. This improvement is a known backlog item (low priority); do not re-raise as a new finding.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_device_fields.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_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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_coverage_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.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_device_fields.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_actions.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-06-01T13:32:29.984Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/imports/list.py:0-0
Timestamp: 2026-06-01T13:32:29.984Z
Learning: When reviewing the NetBox LibreNMS plugin’s import/IPAM code (including files under netbox_librenms_plugin/**/imports/), do not flag issues about missing propagation of the `auto_create_ipam` flag (or `resolve_auto_create_ipam()` usage) into cache keys or `FilterDevicesJob` payloads. The `auto_create_ipam` feature was removed entirely during the IPAM rework merged to `develop` (PR `#303`), so the absence of this flag/payload data in current code is expected.
Applied to files:
netbox_librenms_plugin/views/imports/actions.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
🔇 Additional comments (11)
netbox_librenms_plugin/import_utils/device_operations.py (1)
24-24: LGTM!Also applies to: 731-738, 755-763, 827-834, 859-868, 921-921, 991-1034, 1619-1622
netbox_librenms_plugin/import_utils/virtual_chassis.py (1)
11-11: LGTM!Also applies to: 408-410, 527-527, 552-552
netbox_librenms_plugin/views/sync/device_fields.py (1)
27-27: LGTM!Also applies to: 185-185, 687-687, 950-950
netbox_librenms_plugin/migrations/0012_normalize_device_serials.py (1)
1-52: LGTM!netbox_librenms_plugin/tests/test_utils.py (1)
1449-1494: LGTM!Also applies to: 1624-1691
netbox_librenms_plugin/tests/test_coverage_device_fields.py (1)
329-361: LGTM!Also applies to: 1419-1442, 2226-2256
netbox_librenms_plugin/tests/test_coverage_device_operations.py (1)
1405-1425: LGTM!Also applies to: 1905-2171, 2636-2657, 2953-2975, 3303-3306
netbox_librenms_plugin/tests/test_import_utils.py (1)
28-30: LGTM!Also applies to: 1803-1809, 2702-2705, 2770-3406
netbox_librenms_plugin/views/imports/actions.py (1)
46-46: LGTM!Also applies to: 250-283, 1405-1415, 1605-1617, 1708-1714, 1730-1735, 1770-1781, 2476-2518
netbox_librenms_plugin/tests/test_coverage_actions.py (1)
991-1000: LGTM!Also applies to: 1695-1724, 2223-2252, 3813-3884, 5516-5523, 5775-5819, 6844-6978, 7135-7148
netbox_librenms_plugin/import_utils/bulk_import.py (1)
9-21: LGTM!Also applies to: 266-266, 793-871
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_librenms_plugin/migrations/0012_normalize_device_serials.py`:
- Around line 31-94: Guard the entire catalog inspection, conditional drop, and
concurrent index creation sequence in ensure_device_serial_index with a
PostgreSQL advisory lock, using the existing advisory-lock pattern from the
serial assignment code. Acquire the lock before reading the index state and
release it reliably after the operation, ensuring concurrent migrate runs
serialize without changing the existing validation or retry behavior.
- Around line 73-89: Add focused migration tests for ensure_device_serial_index
covering an existing matching index with indisvalid or indisready false,
asserting it is dropped and rebuilt, and an existing index with an incompatible
definition, asserting RuntimeError. Reuse the existing test fixtures and index
metadata setup from test_preexisting_valid_serial_index_is_reused, and verify
both retry-guard branches without changing production behavior.
🪄 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 Plus
Run ID: 8c304ba6-9b15-4b58-b6dd-392917cf8536
📒 Files selected for processing (2)
netbox_librenms_plugin/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tests/test_utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: NetBox main / Python 3.14
- GitHub Check: NetBox v4.6.5 / Python 3.12
- GitHub Check: NetBox v4.6.5 / Python 3.13
- GitHub Check: NetBox main / Python 3.13
- GitHub Check: NetBox v4.4.0 / Python 3.12
🧰 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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tests/test_utils.py
🧠 Learnings (16)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tests/test_utils.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tests/test_utils.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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tests/test_utils.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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tests/test_utils.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/migrations/0012_normalize_device_serials.pynetbox_librenms_plugin/tests/test_utils.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_utils.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_utils.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_utils.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_utils.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_utils.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_utils.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_utils.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_utils.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_utils.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_utils.py
🪛 OpenGrep (1.26.0)
netbox_librenms_plugin/migrations/0012_normalize_device_serials.py
[ERROR] 89-89: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 100-100: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🔇 Additional comments (2)
netbox_librenms_plugin/tests/test_utils.py (1)
1494-1513: LGTM!Also applies to: 1644-1663, 1665-1689, 1692-1711
netbox_librenms_plugin/migrations/0012_normalize_device_serials.py (1)
7-7: LGTM!Also applies to: 97-101, 121-124
Comment on lines
+31
to
+94
| def ensure_device_serial_index(apps, schema_editor): | ||
| """Create the serial index, reusing a valid copy or repairing an interrupted build.""" | ||
| del apps | ||
| connection = schema_editor.connection | ||
|
|
||
| # IF NOT EXISTS also accepts an invalid index left by a failed concurrent build. | ||
| # Inspect the catalog so retries can repair that state without trusting a wrong definition. | ||
| with connection.cursor() as cursor: | ||
| cursor.execute( | ||
| """ | ||
| SELECT | ||
| index_state.indisvalid, | ||
| index_state.indisready, | ||
| index_state.indrelid = to_regclass(%s), | ||
| index_state.indisunique, | ||
| index_state.indisprimary, | ||
| index_state.indpred IS NULL, | ||
| index_state.indnatts = index_state.indnkeyatts, | ||
| access_method.amname, | ||
| ARRAY( | ||
| SELECT attribute.attname | ||
| FROM unnest(index_state.indkey) WITH ORDINALITY AS key(attnum, position) | ||
| JOIN pg_attribute AS attribute | ||
| ON attribute.attrelid = index_state.indrelid | ||
| AND attribute.attnum = key.attnum | ||
| WHERE key.position <= index_state.indnkeyatts | ||
| ORDER BY key.position | ||
| ) | ||
| FROM pg_index AS index_state | ||
| JOIN pg_class AS index_class | ||
| ON index_class.oid = index_state.indexrelid | ||
| JOIN pg_namespace AS index_namespace | ||
| ON index_namespace.oid = index_class.relnamespace | ||
| JOIN pg_am AS access_method | ||
| ON access_method.oid = index_class.relam | ||
| WHERE index_class.relname = %s | ||
| AND index_namespace.nspname = current_schema() | ||
| """, | ||
| [_SERIAL_TABLE, _SERIAL_INDEX], | ||
| ) | ||
| existing = cursor.fetchone() | ||
|
|
||
| if existing is not None: | ||
| valid, ready, on_device, unique, primary, unfiltered, no_includes, method, columns = existing | ||
| expected_shape = ( | ||
| on_device | ||
| and not unique | ||
| and not primary | ||
| and unfiltered | ||
| and no_includes | ||
| and method == "btree" | ||
| and columns == ["serial"] | ||
| ) | ||
| if not expected_shape: | ||
| raise RuntimeError(f"Existing index {_SERIAL_INDEX!r} has an incompatible definition") | ||
| if valid and ready: | ||
| return | ||
|
|
||
| schema_editor.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {schema_editor.quote_name(_SERIAL_INDEX)}") | ||
|
|
||
| schema_editor.execute( | ||
| f"CREATE INDEX CONCURRENTLY {schema_editor.quote_name(_SERIAL_INDEX)} " | ||
| f"ON {schema_editor.quote_name(_SERIAL_TABLE)} ({schema_editor.quote_name('serial')})" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
Consider guarding against concurrent migrate runs.
The catalog-inspect → conditionally DROP INDEX CONCURRENTLY → CREATE INDEX CONCURRENTLY sequence isn't itself transactional (by necessity, since CONCURRENTLY can't run inside a transaction). If migrate is invoked from multiple replicas/pods simultaneously (common in rolling k8s deployments), two processes could both observe a stale/invalid index and race on the drop+rebuild, causing one to fail with a "relation already exists" or lock-wait error. The design is already retry-safe on a subsequent solo run, so this is a resilience nicety rather than a correctness bug, but wrapping the whole function body in a pg_advisory_lock/pg_try_advisory_lock (similar to the advisory-lock pattern used elsewhere in this PR for serial assignment) would make concurrent deploys fail-safe instead of relying on a retry.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 89-89: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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/migrations/0012_normalize_device_serials.py` around
lines 31 - 94, Guard the entire catalog inspection, conditional drop, and
concurrent index creation sequence in ensure_device_serial_index with a
PostgreSQL advisory lock, using the existing advisory-lock pattern from the
serial assignment code. Acquire the lock before reading the index state and
release it reliably after the operation, ensuring concurrent migrate runs
serialize without changing the existing validation or retry behavior.
Comment on lines
+73
to
+89
| if existing is not None: | ||
| valid, ready, on_device, unique, primary, unfiltered, no_includes, method, columns = existing | ||
| expected_shape = ( | ||
| on_device | ||
| and not unique | ||
| and not primary | ||
| and unfiltered | ||
| and no_includes | ||
| and method == "btree" | ||
| and columns == ["serial"] | ||
| ) | ||
| if not expected_shape: | ||
| raise RuntimeError(f"Existing index {_SERIAL_INDEX!r} has an incompatible definition") | ||
| if valid and ready: | ||
| return | ||
|
|
||
| schema_editor.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {schema_editor.quote_name(_SERIAL_INDEX)}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether tests exercise the repair / incompatible-shape branches of ensure_device_serial_index
rg -n "indisvalid|indisready|incompatible definition|RuntimeError" netbox_librenms_plugin/tests/test_utils.pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate relevant files =="
fd -a '0012_normalize_device_serials.py|test_utils.py' . | sed 's#^\./##'
echo
echo "== Migration relevant section =="
migration="$(fd '0012_normalize_device_serials.py' . | head -n1)"
wc -l "$migration"
sed -n '1,140p' "$migration" | cat -n
echo
echo "== Test references to ensure_device_serial_index / device serial indexes =="
rg -n "ensure_device_serial_index|_SERIAL_INDEX|device_serial|serial.*index|indisvalid|indisready|pg_index|pg_indexes|incompatible|RuntimeError" netbox_librenms_plugin/tests netbox_librenms_plugin/migrations -g '*.py'Repository: marcinpsk/netbox-librenms-plugin
Length of output: 17005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = next(Path('.').rglob('0012_normalize_device_serials.py'))
text = p.read_text()
print("Migration path:", p)
for needle in ["def ensure_device_serial_index", "class _SerialIndexCheckerMixin", "RuntimeError", "ensure_device_serial_index"]:
print(needle, "count=", text.count(needle))
PY
python3 - <<'PY'
from pathlib import Path
import re
migration = next(Path('.').rglob('0012_normalize_device_serials.py'))
migration_text = migration.read_text()
test_text = Path('netbox_librenms_plugin/tests/test_utils.py').read_text()
checks = [
("ensure_device_serial_index call", re.search(r"\bensure_device_serial_index\b", migration_text)),
("SerialIndexCheckerMixin or _SerialIndexCheckerMixin import/class", re.search(r"_?SerialIndexCheckerMixin", migration_text + test_text)),
("incompatible definition in migration", re.search(r"incompatible definition", migration_text)),
("RuntimeError in migration with incompatible definition", re.search(r"raise RuntimeError\(.*incompatible definition", migration_text)),
("PG status columns in migration or tests", re.search(r"indisvalid|indisready|pg_index|pg_indexes", migration_text + test_text)),
("Same migration and test references count", lambda: (migration_text.count("ensure_device_serial_index"), test_text.count("ensure_device_serial_index"))),
]
for name, pat in checks:
try:
print(name, bool(pat), pat.group(0) if hasattr(pat, 'group') else pat)
except Exception as e:
print(name, "ERROR", repr(e))
PYRepository: marcinpsk/netbox-librenms-plugin
Length of output: 882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 0012 migration tests exact boundary =="
sed -n '1450,1520p' netbox_librenms_plugin/tests/test_utils.py | cat -n
echo
echo "== Migration implementation imports relevant for PG cursor shape =="
sed -n '38,94p' netbox_librenms_plugin/migrations/0012_normalize_device_serials.py | cat -n
echo
echo "== Deterministic schema-selection check for the serial-index SELECT =="
python3 - <<'PY'
import ast
from pathlib import Path
path = next(Path('.').rglob('0012_normalize_device_serials.py'))
tree = ast.parse(path.read_text())
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.name == "ensure_device_serial_index":
break
else:
raise SystemExit("missing ensure_device_serial_index")
sql = None
for stmt in node.body:
if isinstance(stmt, ast.With):
for withitem in stmt.withitems:
if isinstance(withitem.optional_vars, ast.Name) and withitem.optional_vars.id == "cursor":
call = withitem.context_expr.func
if isinstance(call, ast.Attribute) and call.attr == "execute":
sql = ast.get_source_segment(path.read_text(), call.args[0])
break
if sql:
break
print(sql)
print("uses_current_schema_filter=", "current_schema()" in sql)
print("uses_schema_filter=", "index_namespace.nspname" in sql and "current_schema()" in sql)
PYRepository: marcinpsk/netbox-librenms-plugin
Length of output: 6888
Add migration-edge-case coverage for ensure_device_serial_index.
test_preexisting_valid_serial_index_is_reused covers missing-index and valid-index reuse, but not the retry-eligible branches: existing index with indisvalid/indisready = false (drop/rebuild) and existing same-schema index with an incompatible shape (RuntimeError). Add targeted cases for both so the production DDL retry-guard path cannot regress unnoticed.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 89-89: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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/migrations/0012_normalize_device_serials.py` around
lines 73 - 89, Add focused migration tests for ensure_device_serial_index
covering an existing matching index with indisvalid or indisready false,
asserting it is dropped and rebuilt, and an existing index with an incompatible
definition, asserting RuntimeError. Reuse the existing test fixtures and index
metadata setup from test_preexisting_valid_serial_index_is_reused, and verify
both retry-guard branches without changing production behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/README.md`:
- Line 275: Revise the documentation sentence around the plugin migration
rollback guidance to avoid characterizing NetBox’s generic removal steps as
table-only. State that running migrate ... zero reverses this plugin’s
migrations, including cross-app schema changes, without making claims about the
scope of NetBox’s subsequent generic cleanup.
- Around line 271-272: Update the removal command block in the NetBox
instructions to change into the NetBox project directory after activating the
virtual environment and before running `python manage.py migrate`, using the
official removal procedure’s directory-change command.
- Line 270: Specify the fenced code block language in the documentation by
adding the bash language identifier to the fence at the referenced README
location, preserving the block’s existing contents.
🪄 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 Plus
Run ID: 04cf8594-9994-4d20-a80b-5d78fd370c26
📒 Files selected for processing (1)
docs/README.md
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: NetBox v4.4.0 / Python 3.12
- GitHub Check: NetBox v4.6.5 / Python 3.12
- GitHub Check: NetBox v4.6.5 / Python 3.13
- GitHub Check: Analyze (python)
- GitHub Check: NetBox main / Python 3.14
- GitHub Check: NetBox main / Python 3.13
🧰 Additional context used
🪛 markdownlint-cli2 (0.23.1)
docs/README.md
[warning] 270-270: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| See [the instructions for uninstalling plugins](https://netboxlabs.com/docs/netbox/en/stable/plugins/removal/). | ||
| To cleanly uninstall the plugin and reverse its migrations, **while the plugin is still installed and enabled**, do the following: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced block language.
Add bash to satisfy Markdown linting and improve rendered syntax highlighting.
Proposed fix
-```
+```bash📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 270-270: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/README.md` at line 270, Specify the fenced code block language in the
documentation by adding the bash language identifier to the fence at the
referenced README location, preserving the block’s existing contents.
Source: Linters/SAST tools
Comment on lines
+271
to
+272
| source /opt/netbox/venv/bin/activate | ||
| python manage.py migrate netbox_librenms_plugin zero |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Change to NetBox’s directory before invoking manage.py.
This block is not self-contained: python manage.py fails unless the shell is already in the NetBox project directory. The official removal instructions explicitly change directories before running management commands. (netboxlabs.com)
Proposed fix
source /opt/netbox/venv/bin/activate
+cd /opt/netbox/netbox/
python manage.py migrate netbox_librenms_plugin zero📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| source /opt/netbox/venv/bin/activate | |
| python manage.py migrate netbox_librenms_plugin zero | |
| source /opt/netbox/venv/bin/activate | |
| cd /opt/netbox/netbox/ | |
| python manage.py migrate netbox_librenms_plugin zero |
🤖 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 `@docs/README.md` around lines 271 - 272, Update the removal command block in
the NetBox instructions to change into the NetBox project directory after
activating the virtual environment and before running `python manage.py
migrate`, using the official removal procedure’s directory-change command.
| python manage.py migrate netbox_librenms_plugin zero | ||
| ``` | ||
|
|
||
| This is the supported way to undo everything the plugin's migrations created: its own tables and migration records, plus any objects it adds to NetBox's own tables (for example indexes on `dcim_device`). Those cross-app changes are **not** covered by NetBox's generic removal steps — which only look for tables named `netbox_librenms_plugin_*` — so reversing the migrations first ensures nothing is left behind. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid describing NetBox’s generic removal steps as table-only.
The current NetBox guidance also covers reindexing, migration records, and stale content types/permissions. Reword this sentence to say that migrate ... zero reverses this plugin’s migrations and cross-app schema changes, without inaccurately characterizing the subsequent generic cleanup. (netboxlabs.com)
🤖 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 `@docs/README.md` at line 275, Revise the documentation sentence around the
plugin migration rollback guidance to avoid characterizing NetBox’s generic
removal steps as table-only. State that running migrate ... zero reverses this
plugin’s migrations, including cross-app schema changes, without making claims
about the scope of NetBox’s subsequent generic cleanup.
Merged
This was referenced Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_refresh_existing_device()'s generic exception handler only logged, so a transient failure (e.g. a DB error inside the librenms_id/name lookups) left a previously-cached "importable" row importable — letting a duplicate import slip through exactly when the duplicate check couldn't run. Every other failure branch of this function already fails closed (vanished link, deleted device). The handler now forces can_import/is_ready False and appends a blocking issue message (issues, not just a warning — recalculate would silently re-enable anything weaker). Red->green via the extended exception-path test.
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
Bug Fixes
"0"preserved) across import, sync, virtual chassis, and device updates.Documentation