Fixed BGP Remote peer link and graph in Routing Overview, or just static text when not a device in LibreNMS #15535
Conversation
This file contains 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
…tic text when not a device in LibreNMS
…dates Graph for local peer)
…dates Graph for local peer), replaced with just a DeviceLinks
|
Hi @tim427 |
|
(that being said, I tested this PR and it looks better indeed) :) |
murrant
reviewed
Nov 1, 2023
| $peeraddresslink = '<span class=list-large>' . \LibreNMS\Util\Url::overlibLink($overlib_link, $peer_addr, \LibreNMS\Util\Url::graphTag($graph_array_zoom)) . '</span>'; | ||
| $peer_device = \App\Models\Device::whereHas('bgppeers', fn ($q) => $q->where('bgpLocalAddr', $peer['bgpPeerIdentifier']))->first(); | ||
| if (! empty($peer_device)) { | ||
| $peeraddresslink = '<span class=list-large>' . \LibreNMS\Util\Url::deviceLink($peer_device, $peer_addr, vars: ['tab' => 'routing', 'proto' => 'bgp']) . '</span>'; |
There was a problem hiding this comment.
Hmm, looking at deviceLink, I think it should return the second argument if the first is not a device.
Also, you don't need the name (vars:) for the third argument here. I only put it in last time so I could omit the second argument.
There was a problem hiding this comment.
Fixed, hopefully I didn't break it.
There was a problem hiding this comment.
Nice fix/addition :)!
|
If we are interested in redoing this whole page. The SQL could be optimized a lot like this (basic query, and the peer relationship doesn't exist yet): $bgpPeers = \App\Models\BgpPeers::with(['device', 'peer'])->get();Another option would be to turn it into an ajax table so it works well if there are a large amount of peers. |
…efault to '' Import some of the things to improve readability.
murrant
previously approved these changes
Nov 2, 2023
murrant
approved these changes
Nov 2, 2023
|
This pull request has been mentioned on LibreNMS Community. There might be relevant details there: |
gdepeyrot
pushed a commit
to gdepeyrot/librenms
that referenced
this pull request
Nov 29, 2023
…tic text when not a device in LibreNMS (librenms#15535) * Fixed BGP Remote peer link and graph in Routing Overview, or just static text when not a device in LibreNMS * Added extra space * Reverted back to overlibLink (due to BGP update graphs on hover) * Added extra space * Remove graph for remote peer, as it is falsely (showed earlier the Updates Graph for local peer) * Removed unnecessary/unused code * Remove graph for remote peer, as it is falsely (showed earlier the Updates Graph for local peer), replaced with just a DeviceLinks * Change Url::deviceLink to return $text if the device doesn't exist. Default to '' Import some of the things to improve readability. * update phpdoc --------- Co-authored-by: Tony Murray <murraytony@gmail.com>
gdepeyrot
pushed a commit
to gdepeyrot/librenms
that referenced
this pull request
Nov 29, 2023
…tic text when not a device in LibreNMS (librenms#15535) * Fixed BGP Remote peer link and graph in Routing Overview, or just static text when not a device in LibreNMS * Added extra space * Reverted back to overlibLink (due to BGP update graphs on hover) * Added extra space * Remove graph for remote peer, as it is falsely (showed earlier the Updates Graph for local peer) * Removed unnecessary/unused code * Remove graph for remote peer, as it is falsely (showed earlier the Updates Graph for local peer), replaced with just a DeviceLinks * Change Url::deviceLink to return $text if the device doesn't exist. Default to '' Import some of the things to improve readability. * update phpdoc --------- Co-authored-by: Tony Murray <murraytony@gmail.com>
gunkaaa
pushed a commit
to gunkaaa/librenms
that referenced
this pull request
Jan 8, 2024
…tic text when not a device in LibreNMS (librenms#15535) * Fixed BGP Remote peer link and graph in Routing Overview, or just static text when not a device in LibreNMS * Added extra space * Reverted back to overlibLink (due to BGP update graphs on hover) * Added extra space * Remove graph for remote peer, as it is falsely (showed earlier the Updates Graph for local peer) * Removed unnecessary/unused code * Remove graph for remote peer, as it is falsely (showed earlier the Updates Graph for local peer), replaced with just a DeviceLinks * Change Url::deviceLink to return $text if the device doesn't exist. Default to '' Import some of the things to improve readability. * update phpdoc --------- Co-authored-by: Tony Murray <murraytony@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaced "overlibLink" with "deviceLink" and using the RemotePeer-device, instead of LocalPeer-device.
When the device isn't monitored by LibreNMS, then just a static text with the address is shown.
DO NOT DELETE THE UNDERLYING TEXT
Please note
Testers
If you would like to test this pull request then please run:
./scripts/github-apply <pr_id>, i.e./scripts/github-apply 5926After you are done testing, you can remove the changes with
./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.