launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/maas-api-permissions into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/maas-api-permissions/+merge/100416

This branch changes how we identify a node in the view code from using node.id to using node.system_id.  This will unify the way we fetch nodes in the view code and in the API code.  I'm working on unifying (and fixing) the way we do permissions check in the view code and in the API code and this is a pre-requisite for that work.
-- 
https://code.launchpad.net/~rvb/maas/maas-api-permissions/+merge/100416
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-api-permissions into lp:maas.

=== modified file 'src/maasserver/templates/maasserver/node_edit.html'
--- src/maasserver/templates/maasserver/node_edit.html	2012-03-21 18:43:29 +0000
+++ src/maasserver/templates/maasserver/node_edit.html	2012-04-02 13:50:32 +0000
@@ -12,6 +12,6 @@
     {% endfor %}
     </ul>
     <input type="submit" value="Save node" class="right" />
-    <a class="link-button" href="{% url 'node-view' node.id %}">Cancel</a>
+    <a class="link-button" href="{% url 'node-view' node.system_id %}">Cancel</a>
   </form>
 {% endblock %}

=== modified file 'src/maasserver/templates/maasserver/node_list.html'
--- src/maasserver/templates/maasserver/node_list.html	2012-03-22 06:38:06 +0000
+++ src/maasserver/templates/maasserver/node_list.html	2012-04-02 13:50:32 +0000
@@ -39,7 +39,7 @@
         {% for node in node_list %}
           <tr class="node {% cycle 'even' 'odd' %}">
             <td>
-              <a href="{% url 'node-view' node.id %}">
+              <a href="{% url 'node-view' node.system_id %}">
                 {% for macaddress in node.macaddress_set.reverse %}
                   {{ macaddress }}{% if not forloop.last %},{% endif %}
                 {% endfor %}

=== modified file 'src/maasserver/templates/maasserver/node_view.html'
--- src/maasserver/templates/maasserver/node_view.html	2012-03-21 18:53:44 +0000
+++ src/maasserver/templates/maasserver/node_view.html	2012-04-02 13:50:32 +0000
@@ -9,7 +9,7 @@
   <div class="block size3">
     {% if can_edit %}
       <h4>Node details</h4>
-      <a href="{% url 'node-edit' node.id %}" class="button secondary">
+      <a href="{% url 'node-edit' node.system_id %}" class="button secondary">
         Edit node
       </a>
     {% endif%}

=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py	2012-03-29 11:40:39 +0000
+++ src/maasserver/tests/test_views.py	2012-04-02 13:50:32 +0000
@@ -470,13 +470,13 @@
     def test_node_list_contains_link_to_node_view(self):
         node = factory.make_node()
         response = self.client.get(reverse('node-list'))
-        node_link = reverse('node-view', args=[node.id])
+        node_link = reverse('node-view', args=[node.system_id])
         self.assertIn(node_link, get_content_links(response))
 
     def test_view_node_displays_node_info(self):
         # The node page features the basic information about the node.
         node = factory.make_node()
-        node_link = reverse('node-view', args=[node.id])
+        node_link = reverse('node-view', args=[node.system_id])
         response = self.client.get(node_link)
         doc = fromstring(response.content)
         content_text = doc.cssselect('#content')[0].text_content()
@@ -485,33 +485,33 @@
 
     def test_view_node_displays_link_to_edit_if_user_owns_node(self):
         node = factory.make_node(owner=self.logged_in_user)
-        node_link = reverse('node-view', args=[node.id])
+        node_link = reverse('node-view', args=[node.system_id])
         response = self.client.get(node_link)
-        node_edit_link = reverse('node-edit', args=[node.id])
+        node_edit_link = reverse('node-edit', args=[node.system_id])
         self.assertIn(node_edit_link, get_content_links(response))
 
     def test_view_node_no_link_to_edit_someonelses_node(self):
         node = factory.make_node(owner=factory.make_user())
-        node_link = reverse('node-view', args=[node.id])
+        node_link = reverse('node-view', args=[node.system_id])
         response = self.client.get(node_link)
-        node_edit_link = reverse('node-edit', args=[node.id])
+        node_edit_link = reverse('node-edit', args=[node.system_id])
         self.assertNotIn(node_edit_link, get_content_links(response))
 
     def test_user_cannot_edit_someonelses_node(self):
         node = factory.make_node(owner=factory.make_user())
-        node_edit_link = reverse('node-edit', args=[node.id])
+        node_edit_link = reverse('node-edit', args=[node.system_id])
         response = self.client.get(node_edit_link)
         self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_user_can_access_the_edition_page_for_his_nodes(self):
         node = factory.make_node(owner=self.logged_in_user)
-        node_edit_link = reverse('node-edit', args=[node.id])
+        node_edit_link = reverse('node-edit', args=[node.system_id])
         response = self.client.get(node_edit_link)
         self.assertEqual(httplib.OK, response.status_code)
 
     def test_user_can_edit_his_nodes(self):
         node = factory.make_node(owner=self.logged_in_user)
-        node_edit_link = reverse('node-edit', args=[node.id])
+        node_edit_link = reverse('node-edit', args=[node.system_id])
         hostname = factory.getRandomString()
         after_commissioning_action = factory.getRandomEnum(
             NODE_AFTER_COMMISSIONING_ACTION)
@@ -533,7 +533,7 @@
 
     def test_admin_can_edit_nodes(self):
         node = factory.make_node(owner=factory.make_user())
-        node_edit_link = reverse('node-edit', args=[node.id])
+        node_edit_link = reverse('node-edit', args=[node.system_id])
         hostname = factory.getRandomString()
         power_type = factory.getRandomChoice(POWER_TYPE_CHOICES)
         after_commissioning_action = factory.getRandomEnum(

=== modified file 'src/maasserver/urls.py'
--- src/maasserver/urls.py	2012-03-28 16:00:26 +0000
+++ src/maasserver/urls.py	2012-04-02 13:50:32 +0000
@@ -82,8 +82,12 @@
         NodeListView.as_view(template_name="maasserver/index.html"),
         name='index'),
     url(r'^nodes/$', NodeListView.as_view(model=Node), name='node-list'),
-    url(r'^nodes/(?P<id>\d*)/view/$', NodeView.as_view(), name='node-view'),
-    url(r'^nodes/(?P<id>\d*)/edit/$', NodeEdit.as_view(), name='node-edit'),
+    url(
+        r'^nodes/(?P<system_id>[\w\-]+)/view/$', NodeView.as_view(),
+        name='node-view'),
+    url(
+        r'^nodes/(?P<system_id>[\w\-]+)/edit/$', NodeEdit.as_view(),
+        name='node-edit'),
     url(
         r'^nodes/create/$', NodesCreateView.as_view(), name='node-create'),
 )

=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py	2012-03-28 16:13:04 +0000
+++ src/maasserver/views.py	2012-04-02 13:50:32 +0000
@@ -108,8 +108,8 @@
     context_object_name = 'node'
 
     def get_object(self):
-        id = self.kwargs.get('id', None)
-        return get_object_or_404(Node, id=id)
+        system_id = self.kwargs.get('system_id', None)
+        return get_object_or_404(Node, system_id=system_id)
 
     def get_context_data(self, **kwargs):
         context = super(NodeView, self).get_context_data(**kwargs)
@@ -123,8 +123,8 @@
     template_name = 'maasserver/node_edit.html'
 
     def get_object(self):
-        id = self.kwargs.get('id', None)
-        node = get_object_or_404(Node, id=id)
+        system_id = self.kwargs.get('system_id', None)
+        node = get_object_or_404(Node, system_id=system_id)
         if not self.request.user.has_perm('edit', node):
             raise PermissionDenied()
         return node
@@ -136,7 +136,7 @@
             return UINodeEditForm
 
     def get_success_url(self):
-        return reverse('node-view', args=[self.get_object().id])
+        return reverse('node-view', args=[self.get_object().system_id])
 
 
 def get_longpoll_context():