launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/maas-dont-delete-inuse-nodes-ui into lp:maas with lp:~rvb/maas/maas-dont-delete-inuse-nodes as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/maas-dont-delete-inuse-nodes-ui/+merge/100806

Allocated cannot be deleted.  The delete button is 'disabled' in this case.  It's better than to have is simply gone when it's not possible to delete the node because it will let you understand why it is that this node cannot be deleted.  fwiw the not-allowed cursor is only working on FF.
-- 
https://code.launchpad.net/~rvb/maas/maas-dont-delete-inuse-nodes-ui/+merge/100806
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-dont-delete-inuse-nodes-ui into lp:maas.

=== modified file 'src/maasserver/static/css/forms.css'
--- src/maasserver/static/css/forms.css	2012-04-04 04:41:04 +0000
+++ src/maasserver/static/css/forms.css	2012-04-04 14:48:21 +0000
@@ -127,6 +127,18 @@
     background-image: -webkit-linear-gradient(bottom, rgb(51,51,51) 0%, rgb(90,90,90) 100%);
     background-image: -ms-linear-gradient(bottom, rgb(51,51,51) 0%, rgb(90,90,90) 100%);
     }
+/* Disabled buttons */
+.yui3-button.disabled,
+.button.disabled,
+button.disabled {
+    cursor: not-allowed;
+    border-color: #999;
+    background-image: linear-gradient(bottom, rgb(200,200,200) 0%, rgb(150,150,150) 100%);
+    background-image: -o-linear-gradient(bottom, rgb(200,200,200) 0%, rgb(150,150,150) 100%);
+    background-image: -moz-linear-gradient(bottom, rgb(200,200,200) 0%, rgb(150,150,150) 100%);
+    background-image: -webkit-linear-gradient(bottom, rgb(200,200,200) 0%, rgb(150,150,150) 100%);
+    background-image: -ms-linear-gradient(bottom, rgb(200,200,200) 0%, rgb(150,150,150) 100%);
+    }
 .link-button {
     display: inline-block;
     padding: 6px 0;

=== modified file 'src/maasserver/templates/maasserver/node_view.html'
--- src/maasserver/templates/maasserver/node_view.html	2012-04-04 06:51:17 +0000
+++ src/maasserver/templates/maasserver/node_view.html	2012-04-04 14:48:21 +0000
@@ -18,9 +18,18 @@
     <div class="block size3">
     <h4>Actions</h4>
     {% if can_delete %}
-      <a href="{% url 'node-delete' node.system_id %}" class="button secondary">
-        Delete node
-      </a>
+      {% if node.owner %}
+        <a href="#"
+           title="You cannot delete this node because it's in use."
+           class="disabled button">
+          Delete node
+        </a>
+      {% else %}
+        <a href="{% url 'node-delete' node.system_id %}"
+           class="button secondary">
+          Delete node
+        </a>
+      {% endif %}
     {% endif %}
     {% for action in form.action_buttons %}
       {% if forloop.first %}

=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py	2012-04-04 09:49:49 +0000
+++ src/maasserver/tests/test_views.py	2012-04-04 14:48:21 +0000
@@ -526,7 +526,7 @@
 
     def test_view_node_shows_link_to_delete_node_for_admin(self):
         self.become_admin()
-        node = factory.make_node(owner=factory.make_user())
+        node = factory.make_node()
         node_link = reverse('node-view', args=[node.system_id])
         response = self.client.get(node_link)
         node_delete_link = reverse('node-delete', args=[node.system_id])
@@ -534,12 +534,35 @@
 
     def test_admin_can_delete_nodes(self):
         self.become_admin()
-        node = factory.make_node(owner=factory.make_user())
+        node = factory.make_node()
         node_delete_link = reverse('node-delete', args=[node.system_id])
         response = self.client.post(node_delete_link, {'post': 'yes'})
         self.assertEqual(httplib.FOUND, response.status_code)
         self.assertFalse(User.objects.filter(id=node.id).exists())
 
+    def test_allocated_node_view_page_says_node_cannot_be_deleted(self):
+        self.become_admin()
+        node = factory.make_node(
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+        node_view_link = reverse('node-view', args=[node.system_id])
+        response = self.client.get(node_view_link)
+        node_delete_link = reverse('node-delete', args=[node.system_id])
+
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertNotIn(node_delete_link, get_content_links(response))
+        self.assertIn(
+            "You cannot delete this node because it's in use.",
+            response.content)
+
+    def test_allocated_node_cannot_be_deleted(self):
+        self.become_admin()
+        node = factory.make_node(
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+        node_delete_link = reverse('node-delete', args=[node.system_id])
+        response = self.client.get(node_delete_link)
+
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
     def test_user_cannot_view_someone_elses_node(self):
         node = factory.make_node(owner=factory.make_user())
         node_view_link = reverse('node-view', args=[node.system_id])

=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py	2012-04-04 07:10:49 +0000
+++ src/maasserver/views.py	2012-04-04 14:48:21 +0000
@@ -47,6 +47,7 @@
     login as dj_login,
     logout as dj_logout,
     )
+from django.core.exceptions import PermissionDenied
 from django.core.urlresolvers import reverse
 from django.http import (
     HttpResponse,
@@ -90,6 +91,7 @@
 from maasserver.models import (
     Node,
     NODE_PERMISSION,
+    NODE_STATUS,
     SSHKey,
     UserProfile,
     )
@@ -169,6 +171,8 @@
         node = Node.objects.get_node_or_404(
             system_id=system_id, user=self.request.user,
             perm=NODE_PERMISSION.ADMIN)
+        if node.status == NODE_STATUS.ALLOCATED:
+            raise PermissionDenied()
         return node
 
     def get_next_url(self):