feat: parent child interfaces#87
Open
marcinpsk wants to merge 142 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds OOB linkage handling, PortStack LAG pattern support, server-scoped sync and migration flows, and expanded regression coverage across import, sync, and UI paths. ChangesOOB linkage, PortStack patterns, and import validation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/librenms_api.py`:
- Around line 413-414: The call returning (True, data.get("mappings", []))
should validate that "mappings" is a list of dicts before reporting success; in
librenms_api.py replace the raw return in the function that parses the response
with logic that: ensure data.get("mappings") is a list, filter out any non-dict
entries (e.g., [m for m in mappings if isinstance(m, dict)]), and if the result
is missing/empty use an empty list and still return success=False or True as
appropriate for your function (prefer True with an empty list to match
get_device_inventory()/get_inventory_filtered() behavior); update the code paths
that call resolve_port_relationships() to rely on the validated list so callers
that call .get() on each item won't hit AttributeError/TypeError.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1918-1938: The fetch response is parsed unconditionally; update
the fetch chain inside the existing fetch(url, { ... }) call so you first check
the Response.ok (the returned r object) before calling r.json() — if !r.ok
attempt to parse JSON or text to pull an error message and treat it as an error
branch (set btn.disabled = false, btn.innerHTML to the alert icon and btn.title
to the backend error or status text); if r.ok continue to parse r.json() and
handle data.status === 'success' as now. Also enhance the .catch handler to
include the caught error.message in btn.title instead of the generic 'Request
failed'. Reference the existing variables btn, url, csrf, body and the current
then/catch chain when making the change.
- Around line 346-365: The delegated change handler that auto-selects LAG
members sets memberCheckbox.checked but doesn't trigger those checkboxes' own
change listeners, so bulk-action state isn't recomputed; inside the loop in the
change handler (the document.addEventListener('change', ...) block that checks
'input[name="select"]' and the 'autoSelectLagMembers' toggle and iterates
`document.querySelectorAll('tr[data-member-of-lag="' + portId + '"]')`), after
setting `memberCheckbox.checked = checkbox.checked`, dispatch a change event on
the memberCheckbox (e.g. memberCheckbox.dispatchEvent(new Event('change', {
bubbles: true }))) so those checkboxes' listeners run and the global bulk-action
state is updated.
In `@netbox_librenms_plugin/tests/test_coverage_actions.py`:
- Around line 4243-4319: Add thin view-level regression tests that exercise
AddAsOOBView and PromoteToHostView end-to-end rather than only calling
set_librenms_oob: craft a request/POST that goes through the view code paths
which would pass oob_type="oob" (simulate the same MagicMock device/CF data used
in current tests), call AddAsOOBView and PromoteToHostView (or use the test
client to POST to their endpoints) and assert the HTTP response is not 400 and
that the device custom_field_data was updated (via get_librenms_oob or
inspecting obj.custom_field_data) to contain type=="oob" or the saved oob id;
keep the existing util-level tests but add one view-level test per path
referencing AddAsOOBView, PromoteToHostView and set_librenms_oob so the
user-visible behavior is covered.
In `@netbox_librenms_plugin/tests/test_librenms_api.py`:
- Around line 1731-1781: The tests in TestGetPortStack rely on the pytest
fixture mock_librenms_api but that fixture isn't available at class scope; move
or register mock_librenms_api at module/collection scope so pytest can find it:
either place the mock_librenms_api fixture into your project's conftest.py
(preferred) or expose it via module-level pytest_plugins in the test module so
TestGetPortStack and its methods can use it; ensure the fixture name remains
mock_librenms_api and that tests like
TestGetPortStack::test_returns_mappings_list_on_success,
test_returns_false_on_404, and test_returns_empty_list_when_no_mappings_key
import/use the same fixture.
In `@netbox_librenms_plugin/urls.py`:
- Around line 936-940: The changelog route for PortStackLagPattern is missing
the model context: update the path that registers
PortStackLagPatternChangeLogView (name "portstacklagpattern_changelog") to pass
kwargs={"model": PortStackLagPattern} so the view receives the model like the
other *ChangeLogView routes; locate the path call for
PortStackLagPatternChangeLogView and add the kwargs parameter referencing the
PortStackLagPattern model.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 116-134: OOB links are appended using link.get("local_port")
directly which skips the configured interface_name_field resolution; update the
OOB branch (where get_librenms_oob(...) and
self.librenms_api.get_device_links(oob["id"]) are used) to fetch OOB ports via
self.librenms_api.get_ports(oob["id"]) and build an oob_local_ports_map keyed by
port id that resolves the display name based on the same interface_name_field
logic used for the main device (mirror how local_ports_map is built), then use
that map to populate "local_port" and "local_port_id" when appending to
links_data so OOB interface names respect the user's naming preference.
In `@netbox_librenms_plugin/views/base/interfaces_view.py`:
- Around line 436-462: The _has_lag_signals function currently re-queries
PortStackLagPattern.objects.all() and recompiles regexes on every call; change
it to reuse cached compiled patterns by retrieving a precompiled attribute if
present (e.g. PortStackLagPattern._compiled_pattern) or by compiling once and
storing it on the model instance (check for getattr(pat_obj,
"_compiled_pattern", None) and use that) so you avoid repeated DB reads and
regex.compile calls; alternatively move the pattern-compile logic into a small
helper (cached via functools.lru_cache or a module-level variable) that returns
compiled patterns from PortStackLagPattern.lag_name_pattern, and then have
_has_lag_signals iterate those compiled regexes instead of recompiling each
time.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 172-217: The update_fields branch in _save_device currently skips
model validation and only relies on DB errors; before calling
device.save(update_fields=update_fields) call device.full_clean(exclude=...) to
validate the updated fields only (build exclude as the set of model field names
not present in update_fields) and handle ValidationError the same way as the
other branch (use _err with the same message formatting). Keep existing
IntegrityError handling and HTMX-aware _err behavior; use the same
ValidationError/message_dict logic used when update_fields is None.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 512-546: _resolve_interface_by_port_id currently only searches
interfaces directly on the passed obj, so interfaces that were created on VC
member devices (via sync_interface using device_selection_*) are never found
when running sync from the VC master; update _resolve_interface_by_port_id to,
when obj is a VC master (or when a device selection is present), iterate the
actual selected member devices' Interface/VMInterface querysets in addition to
obj itself and attempt the same librenms_id/name_hint matching against each
member device's interfaces; ensure you reference the same get_librenms_device_id
matching logic and return the matching iface and None as before so the post-sync
LAG/parent update and single-item relationship endpoints succeed.
🪄 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: e79507fd-bbd2-4c3f-8fb9-b2ce8e5f66f4
⛔ Files ignored due to path filters (13)
docs/img/Netbox-librenms-plugin-device-sync-fields.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-import-page.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-module-sync-tab.pngis excluded by!**/*.pngdocs/img/carrier_auto_install_rules/list.pngis excluded by!**/*.pngdocs/img/device_type_mappings/list.pngis excluded by!**/*.pngdocs/img/inventory_ignore_rules/list.pngis excluded by!**/*.pngdocs/img/module_bay_mappings/list.pngis excluded by!**/*.pngdocs/img/module_type_mappings/add.pngis excluded by!**/*.pngdocs/img/module_type_mappings/list.pngis excluded by!**/*.pngdocs/img/normalization_rules/add.pngis excluded by!**/*.pngdocs/img/normalization_rules/list.pngis excluded by!**/*.pngdocs/img/platform_mappings/add.pngis excluded by!**/*.pngdocs/img/platform_mappings/list.pngis excluded by!**/*.png
📒 Files selected for processing (65)
docs/README.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/README.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/filters.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/migrations/0012_portstacklagpattern.pynetbox_librenms_plugin/migrations/0013_portstacklagpattern_data.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/mappings.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_port_stack_lag_pattern.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/migrate.py
This was referenced May 29, 2026
marcinpsk
commented
May 29, 2026
marcinpsk
pushed a commit
that referenced
this pull request
May 29, 2026
- librenms_api: guard against non-dict entries in port_stack iteration
- views/sync/interfaces: resolve LAG/parent interfaces across VC member devices
- urls: add missing kwargs={"model": PortStackLagPattern} to changelog route
- js: check response.ok before parsing JSON in LAG/parent sync fetch handler
- js: call updateBulkActionButton() after LAG cascade uncheck clears members
marcinpsk
pushed a commit
that referenced
this pull request
May 29, 2026
- librenms_api: guard against non-dict entries in port_stack iteration
- views/sync/interfaces: resolve LAG/parent interfaces across VC member devices
- urls: add missing kwargs={"model": PortStackLagPattern} to changelog route
- js: check response.ok before parsing JSON in LAG/parent sync fetch handler
- js: call updateBulkActionButton() after LAG cascade uncheck clears members
marcinpsk
force-pushed
the
feat/parent-child-interfaces
branch
from
eeb6bc2 to
4986913
Compare
marcinpsk
pushed a commit
that referenced
this pull request
May 29, 2026
- librenms_api: guard against non-dict entries in port_stack iteration
- views/sync/interfaces: resolve LAG/parent interfaces across VC member devices
- urls: add missing kwargs={"model": PortStackLagPattern} to changelog route
- js: check response.ok before parsing JSON in LAG/parent sync fetch handler
- js: call updateBulkActionButton() after LAG cascade uncheck clears members
marcinpsk
force-pushed
the
feat/parent-child-interfaces
branch
11 times, most recently
from
f0f9eef to
8ccd827
Compare
marcinpsk
force-pushed
the
feat/parent-child-interfaces
branch
3 times, most recently
from
7a2a5af to
432b7f8
Compare
Owner
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Owner
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
marcinpsk
force-pushed
the
feat/parent-child-interfaces
branch
from
432b7f8 to
7033069
Compare
Display names ('select') can collide in ifDescr mode, where the visible
interface name is not a unique identifier. Carry a stable LibreNMS port_id
('select_port_id') for cross-page parents auto-included by the JS, and key
the relationship sync on port_id so distinct ports sharing a display name are
no longer collapsed onto one.
…display name The single-interface verify path keyed the NetBox interface lookup on the display name (ifName/ifDescr). That name is a render-time preference with no canonical value, so a lookup breaks whenever the current naming mode differs from the one the interface was synced under. Resolve by the stored LibreNMS port_id and fall back to the name only for interfaces without a librenms_id.
…ixin The annotation was never read or enforced — _resolve_interface_by_port_id takes server_key as a parameter — so it was dead noise on the mixin.
…e-id match, select-all - get_port_stack(): catch ValueError from response.json() and return the usual (False, error) tuple instead of letting a non-JSON body escape. - interfaces_view: when _has_lag_signals() says the device has relationships but the port_stack fetch fails, warn the user that the Parent/LAG column may be incomplete (mirrors the OOB-incomplete handling) instead of dropping it silently. - _related_iface_matches: always attempt the stable librenms_id comparison (default server_key to 'default') so the default-server path never silently falls back to fragile name matching when a stable id is stored. - select-all: dispatch a bubbling change event per row so the cross-page parent / LAG-member auto-select logic runs for select-all, not just individual clicks.
Shift-click range selection set cb.checked directly, bypassing the change handler that auto-includes cross-page parent / LAG-member interfaces — the same gap just closed for select-all. Dispatch a bubbling change per ranged checkbox so range selection injects cross-page parents like single clicks do.
…, harden auto-select - SingleInterfaceVerifyView now enforces dcim.view_device (like SingleModuleVerifyView) so a user can't probe arbitrary device IDs for cached interface data. - Pick the cached verify row by the posted stable port_id (host rows only) before the display-name fallback, so colliding host/OOB names can't patch the wrong row. - Normalize all port_ids to int in lag/parent enrichment: a cache round-trip can stringify relationship-map keys while port_id values stay int, silently dropping LAG/parent context. - get_cached_ports_data / _get_cached_relationships resolve the same VC-scoped sync device the writer uses, fixing false cache misses on VC member pages that have their own id. - Run the delegated change flow for auto-selected LAG members, same-page parents, and shift-range selection so second-order selection (nested parents, member-as-child) applies. - Hide bulk 'Delete Selected Interfaces' in migrated mode (a winner exists). - Clarify resolve_port_relationships docstring re: aggregate detection by ifType/name.
… before lookup
- _interface_sync_content.html: convert the multiline {# #} (which test_template_comments
flags — Django renders it as literal text) to {% comment %}, and gate the bulk-delete
button on migrated_to_marker (not migrated_to_winner) so a donor marked migrated never
falls back to destructive delete when the winner can't be resolved.
- handleInterfaceChange: resolve the row from select.closest('tr') (unique) instead of
data-interface, which collides when a name appears in both the host and OOB datasets —
a name lookup could post the wrong row's port_id and repaint the wrong row.
- SingleInterfaceVerifyView: bind self.request and run the dcim.view_device gate before
get_object_or_404, so a user without permission can't probe device IDs via 404-vs-200.
…g as empty A null/missing mappings genuinely means 'no parent/LAG relationships' -> []. But a non-list payload (or a list with non-dict items) is a malformed LibreNMS response; returning (True, []) made it indistinguishable from 'no relationships' and silently skipped valid sync updates. Fail the call (logged) so the caller surfaces it via the port_stack-failure warning.
…not self.device CodeRabbit batch (pci-domain): - librenms_api.get_port_stack: a non-object top-level payload (list/string/null) is malformed, not 'no relationships'. Fail it instead of collapsing to (True, []), which would silently skip valid LAG/parent sync updates. - tables/interfaces: derive the relationship-sync button's object_type from the table subclass (sync_object_type) instead of a self.device.cluster probe that misclassifies a cluster-less VM as a device and POSTs to the wrong endpoint. - librenms_sync.js: the relationship-sync button now honors the row's live VC member-select value over the server-rendered data-object-id, so switching members targets the chosen member instead of the name-based default. Tests added for the non-object payload and the device/VM object_type derivation.
…bort stale verify CodeRabbit batch (PR 87, pci-domain): - interfaces sync: the LAG/parent name_hint fallback used hardcoded ifName. In ifDescr mode the NetBox interface name matches ifDescr, so the wrong-name lookup silently skipped the link. Use the active interface_name_field, falling back to ifName. - interfaces sync: the skip summary claimed every skipped interface was 'mapped to a different interface', but the list also includes ambiguous-port_id skips — make the reason generic. - librenms_sync.js: handleInterfaceChange now uses an AbortController (like handleModuleChange) so a stale /verify-interface/ response from a rapid VC-member change can't repaint the row after a newer selection.
…ship buttons during verify - interfaces_view: an OOB-controller port matching the LAG ifType/name heuristic could trigger get_port_stack(host_id) and the 'Parent/LAG may be incomplete' warning even when the main device has no such relationships. get_port_stack is host-scoped, so filter _source=="oob" rows out before _has_lag_signals() and resolve_port_relationships(). - librenms_sync.js: changing a VC member fires an async row verify; a LAG/parent sync click landing before it repaints would POST the new member's objectId with the previous member's stale relationship metadata. Disable the row's relationship sync buttons while the verify is in flight, re-enabling on settle (skipped on AbortError — the superseding verify owns state). - Test that OOB rows are excluded from the port_stack LAG-inference trigger.
…tton re-enable
CodeRabbit batch (pci-domain):
- librenms_api.get_port_stack: an error payload like {"status": "error", "message": ...}
omits 'mappings' just like a genuine "no relationships" answer, so it returned (True, []) and
masked the failure — skipping valid LAG/parent sync updates. Honor an explicit non-ok status
when mappings is absent; a real empty result (no status / "ok") still returns (True, []).
- sync/interfaces SyncInterfaceLagView / SyncInterfaceParentView: both are fetch() JSON
endpoints but gated with require_all_permissions, which returns the mixin's HTML/redirect on
denial and breaks the caller's JSON error path. Switch to require_all_permissions_json, like
the sibling DeleteNetBoxInterfacesView.
- librenms_sync.js: the verify-in-flight relationship-button lock could leave buttons stuck
disabled. On rapid VC-member changes the second handler captured an empty set (buttons already
disabled) and the first was aborted without re-enabling; a non-repainting settle then restored
nothing. Tag the buttons this flow disables with data-verify-locked and re-enable by
re-querying that marker on the live row, so whichever request settles last restores them — and
a button the sync-click handler disabled mid-POST (no marker) is never wrongly re-enabled.
Tests: port_stack error-status failure; verify-gate test patches the _json method.
- normalize the stored related-interface id before comparing to the LibreNMS port_id so a renamed LAG/parent still matches on the stable id - prefer the row-selected object for missing_nb relationship sync instead of the name-based VC heuristic, so the button targets the right device
…nship maps once - _interface_sync.html: forward the active server_key in the HTMX refresh hx-vals for both device and VM buttons so a multi-server refresh rebuilds the content from the correct LibreNMS namespace instead of the default. - interfaces_view: normalize port_id_to_lag/parent keys once at the cached-data build site instead of re-normalizing per non-OOB port inside _enrich_port_with_lag_parent.
Setting Interface.lag/parent and calling .save() directly bypassed model validation: _interfaces_same_owner only checks the device, so stale port_stack data or a crafted POST with port_id == lag/parent_port_id could persist a self-link (member == aggregate) that Interface.clean() forbids. Run full_clean() before each save at all four sites — the JSON endpoints return 409 with detail, the bulk loop logs and skips — mirroring the full_clean()+409 pattern already used by the migrate move endpoints.
…esponses CodeQL py/stack-trace-exposure flagged SyncInterfaceLagView and SyncInterfaceParentView for returning full_clean() ValidationError detail straight to the JSON client. Mirror the repo's established remediation: log the flattened detail server-side and return a fixed, safe message (the cross-device case is already rejected upstream, so this is the self-link/constraint case). The bulk-sync sites already only log the detail, so they were never flagged.
…uity, verify normalization) Three review findings in the LAG/parent relationship paths: - resolve_port_relationships: only feed ports with a usable port_id AND a non-empty string ifName into the lookup maps. A non-string ifName (malformed payload) would otherwise reach regex/':'/suffix string ops and raise at runtime. - _resolve_interface_by_port_id: collect all stored-id matches and fail on ambiguity instead of binding lag/parent to the first of several interfaces carrying the same stale librenms_id, mirroring _resolve_device_interface(). - SingleInterfaceVerifyView: normalize the cached relationship-map keys before enrichment (as the main table path does), so stringified port_id keys don't drop Parent/LAG context from the inline verify response.
… except, expose VM Parent/LAG - interfaces table: suppress the per-row LAG/parent sync button on migrated donor pages (the buttons POST directly, so the hidden bulk form alone is not enough); stamp migrated_to_marker from the main render and verify paths. - _resolve_interface_by_port_id: narrow the name-hint fallback's bare except to Interface/VMInterface DoesNotExist (swallow) and MultipleObjectsReturned (return ambiguity error); real runtime/DB faults now propagate instead of masking as not-found. - VM interface table: add the Parent/LAG column to Meta.sequence so VMInterface sub-interface relationship sync is reachable from the UI. - ip sync: regression test proving an already-bound IP is not unbound when no interface resolves (skip happens before the update branch).
…ale verify buttons - _interface_sync_content: in migrated-donor mode hide the bulk-select controls (select-all header + per-row checkboxes — dead now that bulk delete is hidden) and switch the modal copy to transfer-only 'Move' wording instead of delete. - librenms_sync.js: on a genuine (non-abort) verify failure, keep the LAG/parent sync buttons disabled. The row was not repainted for the newly-selected VC member, so re-enabling would let a retry combine the new member with the previous member's stale lag/parent port_id and sync the wrong relationship.
- resolve_port_relationships: key by_id on str(port_id) and look up str(high/low_port_id) so a str-vs-int discrepancy between the independent ports and port_stack payloads no longer silently drops valid relationships. - librenms_sync.js: move reenableRelationshipButtons() into the success branch so a 2xx response with status != 'success' (which does not repaint the row) leaves the verify-locked LAG/parent buttons disabled, matching the .catch() rationale — otherwise a retry could post the previous member's stale relationship metadata. - Test for str/int port_id normalization (both directions).
…rify dropdown on failure
- librenms_api.get_port_stack(): check an explicit non-ok status before consuming
'mappings', so an error payload that still carries mappings (e.g.
{status: error, mappings: []}) fails instead of being read as 'no relationships'
and silently skipping valid LAG/sub-interface sync.
- librenms_sync.js: track the last server-confirmed VC member per <select>; on a
failed verify (non-success payload or non-abort error) roll the dropdown back to
it and re-enable the relationship controls, so the user isn't stranded on an
unverified selection with locked buttons. Seeds from the rendered <option selected>.
- Test for the error-status-with-mappings case (no JS unit harness in the repo).
…member VC matches - _resolve_interface_by_port_id() takes expected_owner: the VC-wide librenms_id search can resolve a child/parent uniquely onto a different VC member that carries the same stale id, so callers now pin the (device_id, virtual_machine_id) the row was synced onto and a foreign-member match is rejected instead of writing lag/parent on the wrong interface. - Bulk sync derives the per-row owner via the extracted _resolve_row_target_device() (the same device_selection target used at interface-sync time); the JSON LAG/parent endpoints pin to the posted member device. - Tests converted off mocks to real VC/Device/Interface objects: a cross-member match is rejected (and accepted for the right owner), and the bulk linking outcomes (duplicate display names, stable-id selection, self-LAG full_clean rejection) are asserted end-to-end.
TestResolveInterfaceByPortId mocked Interface.objects.filter (returning lists of
MagicMock(spec=Interface)) and stubbed get_librenms_device_id, so it never verified
the real query or that the nested {server_key: port_id} cf is read correctly. Drive
it against real Device/Interface objects instead: the server-keyed lookup, the
ambiguity guard, and the name-hint fallback all run for real, and the VC-wide
name-ambiguity case uses a real two-member VirtualChassis.
Add a dependency-free make_virtual_chassis(name, *devices) conftest builder and
fold the already-real TestResolveInterfaceByPortIdExpectedOwner onto the shared
builders (dropping its hand-rolled Site/Manufacturer/DeviceType/DeviceRole quartet).
The unexpected-error propagation test stays a unit test (it injects a non-DB
RuntimeError into the lookup).
Convert TestInterfacesSameOwnerGuard off MagicMock(device_id=...) onto real Interface / VMInterface so the genuine device_id / virtual_machine_id attributes are compared (a MagicMock would also hide the function reading a wrong attribute). Fold TestSyncLagAndParentRelationships' hand-rolled _make_device/_iface onto the shared conftest builders, dropping its duplicated Site/Manufacturer/DeviceType/ DeviceRole quartet (the tests themselves were already real-DB).
TestInterfaceContextOOBRows mocked _build_interface_lookup_maps to hand back a MagicMock interface, so the netbox-only reconciliation never ran over a real Interface — a synthesized attribute could hide a bug in what the dedup reads. Drive it through the concrete DeviceInterfaceTableView against a real Device with a real idrac0 Interface: the real interface index, the real get_stored_librenms_id (a local custom-field/cache read, not HTTP), and the OOB-row dedup run end to end, asserting idrac0 still surfaces as netbox-only despite the same-named OOB row. A real LibreNMSAPI is built under a config patch (no network). Only the VLAN helpers, table construction, and cache stay mocked.
resolve_port_relationships stored the raw port_stack high/low ids in the sub_interfaces map while the LAG branch stored the port record's own port_id. port_stack and ports are independent LibreNMS payloads, so a str-vs-int type mismatch could make downstream port-id lookups miss the parent mapping and silently drop sub-interface relationships. Mirror the LAG branch and store low_port["port_id"]/high_port["port_id"]. Regression test asserts the map uses the port-record types, not the stack ids.
…e select On a verify failure the rollback assigned select.value directly, but the member control is TomSelect-enhanced, so the visible dropdown kept showing the rejected member while the backing value reverted — the next LAG/parent sync click would post a different member than the user sees. Route the rollback through select.tomselect.setValue(..., true) (silent, to avoid re-firing the change handler), mirroring the existing initializeVCMemberSelect pattern; fall back to select.value when the widget isn't present.
…nship name The Parent/LAG badge pre-escaped the related interface name with escape() before passing it to format_html(). That was redundant (escape() returns a SafeString that format_html's conditional_escape passes through, so no double-encoding occurred) but obscured intent. Drop the manual escape() so format_html is the single, obvious escape point. Adds a contract test asserting a name with & < > is escaped exactly once.
_has_lag_signals() only scanned ifName, but post() resolves an interface_name_field that may be ifDescr (or another field), and later enrichment renders/matches on that field. On an ifDescr-driven device a LAG/sub-interface signal present only in ifDescr was missed, so the lazy get_port_stack() fetch was skipped and the Parent/LAG column silently stayed empty. Scan ifName/ifDescr plus the selected field, and pass it from the call site. Tests: TestHasLagSignalsFieldSelection exercises the real method (DB-backed, empty pattern table) — proves the field parameter changes the outcome and the ifDescr case is detected. Also hardens test_post_lag_inference_excludes_oob_ports to prove the OOB row was actually fetched and merged before the host-only filter dropped it (otherwise the assertion passed vacuously).
marcinpsk
force-pushed
the
feat/parent-child-interfaces
branch
from
c74987c to
f86aedf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Briefly describe what this PR does in plain English, and provide as much of the following information as possible.
Motivation / Problem
What issue does this solve?
Link any related issues if applicable.
Scope of Change
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
Release Notes