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

Minor IPAM improvements #1511

Merged
merged 16 commits into from Jun 8, 2017
Merged

Minor IPAM improvements #1511

merged 16 commits into from Jun 8, 2017

Conversation

emilhf
Copy link
Collaborator

@emilhf emilhf commented Apr 28, 2017

This fixes a couple of minor visual aspects of the IPAM, adding the description to the prefix listing itself (allowing for quick perusal) and providing more useful context in the tooltips (VLAN, description). This should make it more comfortable to crawl prefixes with many sub-allocations.

Also fixes a minor API bug where information about the top-level prefix is not returned when searching for available address space for delegation purposes. This allows us to draw more stuff in the prefix allocation viz, for example a tooltip displaying the VLAN id.

@jmbredal
Copy link
Collaborator

jmbredal commented May 31, 2017

IPAM

  • Prefix-dropdown goes immediately in search-mode when opening dropdown.
  • Should you do a search immediately after choosing a prefix? Lets try that.

Fix for these two things below

modified   htdocs/static/js/src/ipam/views/control.js
@@ -69,6 +69,7 @@ define(function(require, exports, module) {
       // Set up remote fetching of prefixes for form auto-completion
       var prefixSelect = this.$el.find("#prefix-search-box");
       prefixSelect.select2({
+        minimumInputLength: 1,
         ajax: {
           url: "/api/prefix/search/",
           dataType: 'json',
@@ -93,6 +94,7 @@ define(function(require, exports, module) {
           cache: true
         }
       });
+      prefixSelect.on('change', this.forceSearch.bind(this));
     },
 
     // If the user triggers a search by hitting enter

More serious is this though:

  • Searching with no prefix does not work because the database tries to search with empty string as prefix argument
DataError at /ipam/api/
invalid input syntax for type inet: ""
LINE 1: ..."vlanid" FROM "prefix" WHERE "prefix"."netaddr" = '' LIMIT 2...

This should be taken care of by the api.

On load I just need to know why this happens:
image
Are the 6 requests (some cancelled) for usage warranted?

Subnet Matrix

  1. Good choice of icon. I would prefer the filled one as it is used throughout NAV. It has the class 'fa-info-circle'.
  2. The nbsp; is ruining the centering. It is not needed and should be removed.

Allow clickthrough on icon:

modified   htdocs/static/js/src/subnetmatrix.js
@@ -88,7 +88,6 @@ require(['libs/underscore', 'libs/jquery.sparkline'], function() {
                 // Add link and text for usage if colspan is large enough
                 $element.append(this.usageString(result));
             }
-          console.log(result);
             this.createTooltipText($element, this.tooltipTemplateV4, result);
         },
 
@@ -197,18 +196,17 @@ require(['libs/underscore', 'libs/jquery.sparkline'], function() {
 
             // Actually show the tooltip only on click.
             this.container.on('click', function(event) {
-                var $target = $(event.target);
-
-                if ($target.hasClass('has-loaded')) {
+                var $cell = event.target.nodeName === 'TD' ? $(event.target) : $(event.target).closest('td');
+                if ($cell.hasClass('has-loaded')) {
                     // if for some reason the tooltip has not been created, do it now
-                    if (!$target.data('selector')) {
-                        self.createTooltip($target);
+                    if (!$cell.data('selector')) {
+                        self.createTooltip($cell);
                     }
 
-                    if ($target.hasClass('open')) {
+                    if ($cell.hasClass('open')) {
                         self.closeAllTips();
                     } else {
-                        self.openTip($target);
+                        self.openTip($cell);
                     }
                 } else {
                     // If we click outside the cells, remove all tooltips
@@ -249,7 +247,7 @@ require(['libs/underscore', 'libs/jquery.sparkline'], function() {
                 var request = $.getJSON($target.data('url'));
                 var sparkContainer = $('<div class="usage-sparkline">&nbsp;</div>');
                 sparkContainer.appendTo($toolTip);
-                
+
                 request.done(function(response) {
                     if (response.length > 0) {

@emilhf
Copy link
Collaborator Author

emilhf commented Jun 2, 2017

Yeah, that'll need to be sorted out. I've applied your fixes and also found the invariant error causing the API error when the prefix is set to an empty string. The usage requests you are seeing are individual requests for usage info on individual prefixes, so we shouldn't be firing unnecessary requests. Some of them are cancelled because their associated tree element (e.g. the prefix shown in the IPAM) are destroyed, usually because the DOM is being prepared to draw a new set of nodes and a new IP tree.

@emilhf
Copy link
Collaborator Author

emilhf commented Jun 2, 2017

Current status is that I'm currently struggling with some component lifecycle management, which is blocking the desired feature of being able to scroll to selected nodes in the allocation viz. I'm not entirely sure what the issue is caused by, but I should be able to figure it out by the end of the day.

@jmbredal
Copy link
Collaborator

jmbredal commented Jun 6, 2017

Looking great so far! 😄

Regarding the scrollto - it is set to only scrollto on scope prefixes. I trust you remember better than me that this was as wished from NTNU. Anyway, I don't find any reason to not include this in NAV 4.7. 👍

Copy link
Collaborator

@jmbredal jmbredal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, this looks ready for release.

@jmbredal jmbredal added this to the 4.7.0 milestone Jun 6, 2017
@lunkwill42 lunkwill42 added the ipam label Jun 6, 2017
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only reviewed Python code (not JS), looks good :-)

Testing functionality reveals that once a Prefix is selected in the search form, it cannot be removed without performing a full form reset. Will approve when fixed.

(There's also an issue that the defaults of the form do not correspond to the "default" search parameters. Opening IPAM and clicking Search without altering the form will yield a different result than just opening IPAM - but this is irrelevant to this PR and can be fixed in any other context)

@lunkwill42
Copy link
Member

@emilhf I am not able to confirm your latest commits actually work :-/ I can confirm that the files are updated and not cached by my browser, but the changes apparently have no effect.

I will still merge this for the release, as these are minor issues that can be reported as bugs.

@lunkwill42 lunkwill42 merged commit effdaed into Uninett:master Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants