Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Title to Prefix Utilization to create Tooltip #5321

Closed
wants to merge 17 commits into from
Closed

Adds Title to Prefix Utilization to create Tooltip #5321

wants to merge 17 commits into from

Conversation

jvanderaa
Copy link
Contributor

Fixes: #4611

Adds a multi-line tooltip to include:

  • Addresses Used count

  • Total available addresses

  • Modifies the return of the get_utilization function to include the count used, and total available calculated

  • Updates the utilization utility function to pass these values into the template

  • Update the template to include a title to create a tooltip

Screen Shot 2020-11-07 at 21 29 31

netbox/ipam/models.py Outdated Show resolved Hide resolved
netbox/ipam/tests/test_models.py Outdated Show resolved Hide resolved
netbox/ipam/tests/test_models.py Outdated Show resolved Hide resolved
netbox/utilities/templatetags/helpers.py Outdated Show resolved Hide resolved
netbox/utilities/templatetags/helpers.py Outdated Show resolved Hide resolved
netbox/dcim/models/racks.py Outdated Show resolved Hide resolved
netbox/utilities/templatetags/helpers.py Outdated Show resolved Hide resolved
@candlerb
Copy link
Contributor

candlerb commented Feb 2, 2021

The image shows "Used: 24, Available: 254", and I think that's ambiguous: it could imply that 254 more addresses are available for allocation.

I suggest "Used: 24, Total: 254", or simply "Used: 24 / 254"

EDIT: I see the actual patch says "Used: ... Total Count: ..." which is fine.

@jvanderaa
Copy link
Contributor Author

@jeremystretch @DanSheps this should be all set to go and is ready for merging.

@jeremystretch
Copy link
Member

@jvanderaa while I appreciate the work you've put into this, I can't accept the level of complexity that has been introduced merely to display a count of IP addresses. There also remain some questionable choices, such as using namedtuple where it's not needed and accessing elements by numeric index within a template.

It would probably make more sense to introduce a separate table column to convey this information, rather than trying to inject it into the utilization graph. That would avoid the need to intermingle logic and rendering with the utilization graph.

@jvanderaa
Copy link
Contributor Author

Agreed on the numeric index on the template. Seeing that, it is definitely a mistake there. I will get the update in that is this:

                    <td>Space Utilization</td>
                    <td>{% utilization_graph object.get_utilization %}</td>
                </tr>
                <tr>
                    <td>Power Utilization</td>
                    <td>{% utilization_graph object.get_power_utilization %}</td>

@jeremystretch
Copy link
Member

@jvanderaa the much larger issue is the overall complexity being introduced: it's just trying to do too much at once. As I said, I'm not able to accept this PR, however I sincerely appreciate the effort.

Are you interested in pursuing an alternative approach using a separate table column? If so, I'll leave #4611 assigned to you. I'm afraid we have to close out this PR though.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tally of remaining Available IPs in web UI views
4 participants