Merge lp:~mpontillo/maas/delete-fabric-cleanup into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Clean up "delete fabric" action.

 * UI no longer contains duplicate widgets.
 * Generates a nicer error message when subnets and/or interfaces
   are still present on the fabric.
 * Drive-by fix for an issue that caused VLANs with no subnets to
   be missing from the fabric details page.
 * Add missing test cases for fabric details page.

[Blake Rouse (blake-rouse)]

Looks good other than the missing test. Please add the test before landing, not going to block you on the missing test.

review: Approve

[Mike Pontillo (mpontillo)]

Yeah, this page doesn't have any tests at all for creating that table. We have some technical debt to pay off here. My fault because I reviewed the original page and didn't notice they were missing.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~mpontillo/maas/delete-fabric-cleanup into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [92.2 kB]
Hit:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:4 http://security.ubuntu.com/ubuntu xenial-security InRelease [92.2 kB]
Fetched 184 kB in 0s (381 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-bson python3-convoy python3-coverage python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-mock python3-netaddr python3-netifaces python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
bind9 is already the newest version (1:9.10.3.dfsg.P4-8).
bind9utils is already the newest version (1:9.10.3.dfsg.P4-8).
build-essential is already the newest version (12.1ubuntu2).
bzr is already the newest version (2.7.0-2ubuntu1).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
dnsutils is already the newest version (1:9.10.3.dfsg.P4-8).
firefox is already the newest version (45.0.2+build1-0ubuntu1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
isc-dhcp-common is already the newest version (4.3.3-5ubuntu12).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-f...

=== modified file 'src/maasserver/models/fabric.py'
--- src/maasserver/models/fabric.py	2016-04-11 16:23:26 +0000
+++ src/maasserver/models/fabric.py	2016-04-27 03:54:09 +0000
@@ -26,6 +26,7 @@
 from maasserver import DefaultMeta
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.interface import Interface
+from maasserver.models.subnet import Subnet
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.utils.orm import MAASQueriesMixin
 
@@ -193,11 +194,20 @@
         if self.is_default():
             raise ValidationError(
                 "This fabric is the default fabric, it cannot be deleted.")
-        # Circular imports.
+        if Subnet.objects.filter(vlan__fabric=self).exists():
+            subnets = Subnet.objects.filter(vlan__fabric=self).order_by(
+                'cidr')
+            descriptions = [str(subnet.cidr) for subnet in subnets]
+            raise ValidationError(
+                "Can't delete fabric; the following subnets are "
+                "still present: %s" % (", ".join(descriptions)))
         if Interface.objects.filter(vlan__fabric=self).exists():
+            interfaces = Interface.objects.filter(vlan__fabric=self).order_by(
+                'node', 'name')
+            descriptions = [iface.get_log_string() for iface in interfaces]
             raise ValidationError(
-                "Can't delete fabric: interfaces are connected to VLANs from "
-                "this fabric.")
+                "Can't delete fabric; the following interfaces are "
+                "still connected: %s" % (", ".join(descriptions)))
         super(Fabric, self).delete()
 
     def _create_default_vlan(self):

=== modified file 'src/maasserver/models/tests/test_fabric.py'
--- src/maasserver/models/tests/test_fabric.py	2016-04-11 16:23:26 +0000
+++ src/maasserver/models/tests/test_fabric.py	2016-04-27 03:54:09 +0000
@@ -27,7 +27,10 @@
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.matchers import MockCalledOnce
-from testtools.matchers import MatchesStructure
+from testtools.matchers import (
+    Contains,
+    MatchesStructure,
+)
 
 
 class TestFabricManagerGetFabricOr404(MAASServerTestCase):
@@ -239,10 +242,20 @@
             INTERFACE_TYPE.PHYSICAL,
             vlan=fabric.get_default_vlan())
         error = self.assertRaises(ValidationError, fabric.delete)
-        self.assertEqual(
-            "Can't delete fabric: interfaces are connected to VLANs from this "
-            "fabric.",
-            error.message)
+        self.assertThat(error.message, Contains(
+            "Can't delete fabric; the following interfaces are "
+            "still connected: "))
+
+    def test_cant_delete_fabric_if_connected_to_subnet(self):
+        fabric = factory.make_Fabric()
+        factory.make_Subnet(vlan=fabric.get_default_vlan())
+        factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL,
+            vlan=fabric.get_default_vlan())
+        error = self.assertRaises(ValidationError, fabric.delete)
+        self.assertThat(error.message, Contains(
+            "Can't delete fabric; the following subnets are "
+            "still present: "))
 
     def test_cant_delete_default_fabric(self):
         default_fabric = Fabric.objects.get_default_fabric()

=== modified file 'src/maasserver/static/js/angular/controllers/fabric_details.js'
--- src/maasserver/static/js/angular/controllers/fabric_details.js	2016-04-22 17:28:15 +0000
+++ src/maasserver/static/js/angular/controllers/fabric_details.js	2016-04-27 03:54:09 +0000
@@ -34,13 +34,15 @@
 
         // Called when the fabric has been loaded.
         function fabricLoaded(fabric) {
-            $scope.fabric = fabric;
-            $scope.loaded = true;
-            $scope.vlans = FabricsManager.getVLANs($scope.fabric);
-            $scope.racks = getRackControllers();
+            if(angular.isObject(fabric)) {
+                $scope.fabric = fabric;
+                $scope.loaded = true;
+                $scope.vlans = FabricsManager.getVLANs($scope.fabric);
+                $scope.racks = getRackControllers();
 
-            updateTitle();
-            updateVLANTable();
+                updateTitle();
+                updateVLANTable();
+            }
         }
 
         // Return the rack controller objects attached to this Fabric.  The
@@ -80,6 +82,18 @@
                         };
                         rows.push(row);
                     });
+                } else {
+                    // If there are no subnets, populate the row based on the
+                    // information we have (just the VLAN).
+                    var row = {
+                        vlan: vlan,
+                        vlan_name: VLANsManager.getName(vlan),
+                        subnet: null,
+                        subnet_name: null,
+                        space: null,
+                        space_name: null
+                    };
+                    rows.push(row);
                 }
             });
             $scope.rows = rows;
@@ -129,8 +143,8 @@
             FabricsManager.deleteFabric($scope.fabric).then(function() {
                 $scope.confirmingDelete = false;
                 $location.path("/fabrics");
-            }, function(error) {
-                $scope.error = $scope.convertPythonDictToErrorMsg(error);
+            }, function(reply) {
+                $scope.error = $scope.convertPythonDictToErrorMsg(reply.error);
             });
         };
 

=== modified file 'src/maasserver/static/js/angular/controllers/tests/test_fabric_details.js'
--- src/maasserver/static/js/angular/controllers/tests/test_fabric_details.js	2016-03-25 13:52:27 +0000
+++ src/maasserver/static/js/angular/controllers/tests/test_fabric_details.js	2016-04-27 03:54:09 +0000
@@ -16,7 +16,7 @@
             name: makeName("fabric")
         };
         FabricsManager._items.push(fabric);
-        return fabric;
+       return fabric;
     }
 
     // Grab the needed angular pieces.
@@ -51,14 +51,13 @@
 
     // Makes the NodesListController
     function makeController(loadManagerDefer) {
-        spyOn(UsersManager, "isSuperUser").and.returnValue(true);
+       spyOn(UsersManager, "isSuperUser").and.returnValue(true);
         var loadManagers = spyOn(ManagerHelperService, "loadManagers");
         if(angular.isObject(loadManagerDefer)) {
             loadManagers.and.returnValue(loadManagerDefer.promise);
         } else {
             loadManagers.and.returnValue($q.defer().promise);
         }
-
         // Create the controller.
         var controller = $controller("FabricDetailsController", {
             $scope: $scope,
@@ -74,8 +73,7 @@
             ManagerHelperService: ManagerHelperService,
             ErrorService: ErrorService
         });
-
-        return controller;
+       return controller;
     }
 
     // Make the controller and resolve the setActiveItem call.
@@ -87,7 +85,9 @@
         var controller = makeController(defer);
         $routeParams.fabric_id = fabric.id;
 
+        $rootScope.$digest();
         defer.resolve();
+
         $rootScope.$digest();
         setActiveDefer.resolve(fabric);
         $rootScope.$digest();
@@ -174,6 +174,39 @@
         expect($rootScope.title).toBe(fabric.name);
     });
 
+    it("updates $scope.rows with VLANs containing subnet(s)", function() {
+        var spaces = [{ id: 0, name: "space-0" }];
+        var vlans = [{ id: 1, name: "vlan4", vid: 4, fabric: fabric.id }];
+        var subnets = [
+            { id: 0, name:"subnet1", vlan: 1, space: 0, cidr: "10.20.0.0/16" }
+        ];
+        fabric.vlan_ids = [1];
+        spaces[0].subnet_ids = [0];
+        vlans[0].subnet_ids = [0];
+        SpacesManager._items.push(spaces[0]);
+        VLANsManager._items.push(vlans[0]);
+        SubnetsManager._items.push(subnets[0]);
+        var controller = makeControllerResolveSetActiveItem();
+        $scope.$apply();
+        $rootScope.$digest();
+        var rows = $scope.rows;
+        expect(rows[0].vlan).toBe(vlans[0]);
+        expect(rows[0].subnet_name).toEqual("10.20.0.0/16 (subnet1)");
+        expect(rows[0].space_name).toEqual("space-0");
+    });
+
+    it("updates $scope.rows with VLANs containing no subnet(s)", function() {
+        var vlans = [ { id: 1, name: "vlan4", vid: 4, fabric: fabric.id } ];
+        fabric.vlan_ids = [1];
+        VLANsManager._items.push(vlans[0]);
+        var controller = makeControllerResolveSetActiveItem();
+        $rootScope.$digest();
+        var rows = $scope.rows;
+        expect(rows[0].vlan).toBe(vlans[0]);
+        expect(rows[0].subnet_name).toBe(null);
+        expect(rows[0].space_name).toBe(null);
+    });
+
     describe("canBeDeleted", function() {
 
         it("returns false if fabric is null", function() {

=== modified file 'src/maasserver/static/js/angular/factories/fabrics.js'
--- src/maasserver/static/js/angular/factories/fabrics.js	2016-03-28 13:54:47 +0000
+++ src/maasserver/static/js/angular/factories/fabrics.js	2016-04-27 03:54:09 +0000
@@ -35,12 +35,14 @@
         // instead you should watch this function.
         FabricsManager.prototype.getVLANs = function(fabric) {
             var vlans = [];
-            angular.forEach(fabric.vlan_ids, function(vlan_id) {
-                var vlan = VLANsManager.getItemFromList(vlan_id);
-                if(angular.isObject(vlan)) {
-                    vlans.push(vlan);
-                }
-            });
+            if(angular.isObject(fabric) && angular.isObject(fabric.vlan_ids)) {
+                angular.forEach(fabric.vlan_ids, function(vlan_id) {
+                    var vlan = VLANsManager.getItemFromList(vlan_id);
+                    if(angular.isObject(vlan)) {
+                        vlans.push(vlan);
+                    }
+                });
+            }
             return vlans;
         };
 

=== modified file 'src/maasserver/static/js/angular/services/managerhelper.js'
--- src/maasserver/static/js/angular/services/managerhelper.js	2016-03-28 13:54:47 +0000
+++ src/maasserver/static/js/angular/services/managerhelper.js	2016-04-27 03:54:09 +0000
@@ -78,7 +78,7 @@
         // Returns a printable version of the specified dictionary (useful
         // for displaying an error to the user).
         this.getPrintableString = function(dict, showNames) {
-           var result = '';
+            var result = '';
             angular.forEach(dict, function(value, key) {
                 var error = dict[key];
                 if(showNames === true) {

=== modified file 'src/maasserver/static/partials/fabric-details.html'
--- src/maasserver/static/partials/fabric-details.html	2016-03-21 22:46:23 +0000
+++ src/maasserver/static/partials/fabric-details.html	2016-04-27 03:54:09 +0000
@@ -44,30 +44,6 @@
     </div>
 </header>
 <div data-ng-show="!loading">
-    <div class="page-header__dropdown border ng-hide" data-ng-show="confirmingDelete">
-        <div class="page-header__feedback twelve-col no-margin-bottom">
-            <div class="twelve-col ng-hide" data-ng-show="error">
-                <p class="page-header__feedback-message info">{$ error $}</p>
-            </div>
-            <div class="inner-wrapper">
-                <div class="twelve-col">
-                    <div class="table">
-                        <div class="table__row table__row--no-hover">
-                            <div class="form-inline">
-                                <div class="left">
-                                    Are you sure you want to delete this fabric?
-                                </div>
-                                <div class="right">
-                                    <a data-ng-click="cancelDeleteButton()">Cancel</a>
-                                    <button class="cta-ubuntu secondary" data-ng-click="deleteConfirmButton()">Confirm</button>
-                                </div>
-                            </div>
-                        </div>
-                    </div>
-                </div>
-            </div>
-        </div>
-    </div>
     <section class="row">
         <div class="inner-wrapper">
             <div class="tweleve-col">

=== modified file 'src/maasserver/static/partials/subnet-details.html'
--- src/maasserver/static/partials/subnet-details.html	2016-04-22 17:28:15 +0000
+++ src/maasserver/static/partials/subnet-details.html	2016-04-27 03:54:09 +0000
@@ -265,7 +265,8 @@
                 <table class="table-listing">
                     <thead>
                         <tr class="table-listing__row">
-                            <th class="table-listing__header table-col--40">IP Address</th>
+                            <th class="table-listing__header table-col--20">IP Address</th>
+                            <th class="table-listing__header table-col--20">Node</th>
                             <th class="table-listing__header table-col--10">Owner</th>
                             <th class="table-listing__header table-col--10">Usage</th>
                             <th class="table-listing__header table-col--15">Type</th>
@@ -284,7 +285,15 @@
                     </thead>
                     <tbody>
                         <tr data-ng-repeat="ip in subnet.ip_addresses">
-                            <td class="table-col--40">{$ ip.ip $}</td>
+                            <td class="table-col--20">{$ ip.ip $}</td>
+                            <td class="table-col--20" data-ng-switch="ip.node_summary.node_type">
+                                <span data-ng-switch-when="0"><a href="#/node/{$ ip.node_summary.system_id $}">{$ ip.node_summary.fqdn $}</a></span>
+                                <span data-ng-switch-when="1">{$ ip.node_summary.fqdn $}</span>
+                                <span data-ng-switch-when="2"><a href="#/node/controller/{$ ip.node_summary.system_id $}">{$ ip.node_summary.fqdn $}</a></span>
+                                <span data-ng-switch-when="3"><a href="#/node/controller/{$ ip.node_summary.system_id $}">{$ ip.node_summary.fqdn $}</a></span>
+                                <span data-ng-switch-when="4"><a href="#/node/controller/{$ ip.node_summary.system_id $}">{$ ip.node_summary.fqdn $}</a></span>
+                                <span data-ng-switch-default>Unknown</span>
+                            </td>
                             <td class="table-col--10">{$ ip.user ? ip.user : "MAAS" $}</td>
                             <td class="table-col--10" data-ng-switch="ip.node_summary.node_type">
                                 <span data-ng-switch-when="0">Machine</span>