Merge lp:~rvb/maas/cluster-own-ui4 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

This branch makes the cluster listings deal gracefully with a large number of clusters. Additionally, it makes the cluster listing pages more accessible.

In details:
- Move the cluster listings to their own url (instead of displaying them on the 'settings' page).
- Split the cluster listings into 3 separate listings (one page for each possible cluster status).
- Add a link to the cluster listing page next to 'Nodes' at the top of the page.
- Add pagination to all the listings.

There is a live canonistack instance available with the change from this branch available at http://162.213.35.95:5240/clusters/ (access it with the usual credentials).

On the face if it, this branch appears to be huge but please consider that a lot of it is tests/code being simply moved.

Things that where brought up by people during the design phase and that I left for later (because I wanted to get the main change landed first):
- Add a notification in the 'Clusters' tab when a MAAS instance contains clusters that need attention.
- Add a warning for pending/rejected clusters that are linked to nodes.

[Jeroen T. Vermeulen (jtv)]

Wonderful! And of course we've gone over the design in depth already.

Unfortunately I don't have time for a full review right now; I'll just do what I can and hit the button when I run out of time.

.

A nitpick: why not make status_link an OrderedDict?

.

Might it help clarity to say in get_view_title's docstring that the title basically implements tabs?

.

Also in get_view_title, line 401 of the diff:

                title_chunk = '<a href=%s>%s</a>' % (link, title_chunk)

Shouldn't the value for the href attribute be quoted? This may be a consequence of the method being slightly too big for comfortable detailed testing.

.

It may be helpful, therefore, to extract get_view_title's loop body: “make me a title entry for this category of cluster.” Then get_view_title would just

        return mark_safe(
            ' / '.join(
                self.make_title_entry(status, link_name))
                for status, link_name in self.status_links)

(Of course if status_links were an OrderedDict, then the new function could just take a status argument and look up the link name for itself.)

.

In get_context_data, lines 410—411 of the diff:

        # Do no display a warning for each cluster for which the images
        # are missing unless this is the display of the 'accepted' clusters.

There's a typo in the “not,” but more importantly, this could be simpler without the double negation. How about "Display warnings for clusters that have no images, but only for the display of 'accepted' clusters”?

.

In ClusterListView.post, since each stanza is meant to return, I wonder if it might be clearer to structure the ‘if’ blocks as if/elif blocks. The final return could do with a comment.

.

I'm not too comfortable about the bulk accept/reject buttons applying to all pending clusters... somebody could attempt to register a malicious cluster after the admin loaded the page, but before they hit the “accept all” button. Or conversely, though less harmfully I guess, a good cluster could present itself after the admin loaded the page but just before they hit the “reject all” button.

.

The scenarios list for ClusterListingTest is pretty daunting... why not compute the ‘url’ value in a method? The definition in the scenarios list looks completely regular.

.

In test_listing_is_paginated you use self.assertEquals(1, len(...)). I *finally* bothered to check: testtools has a HasLength matcher! Might make for nicer test output.

[Raphaël Badin (rvb)]

> Wonderful! And of course we've gone over the design in depth already.

Thanks a lot for the review!.

> [...]
>
> Also in get_view_title, line 401 of the diff:
>
> title_chunk = '<a href=%s>%s</a>' % (link, title_chunk)
>
> Shouldn't the value for the href attribute be quoted? This may be a
> consequence of the method being slightly too big for comfortable detailed
> testing.

That's correct, I'm splitting that method into smaller bit now.

> I'm not too comfortable about the bulk accept/reject buttons applying to all
> pending clusters... somebody could attempt to register a malicious cluster
> after the admin loaded the page, but before they hit the “accept all” button.
> Or conversely, though less harmfully I guess, a good cluster could present
> itself after the admin loaded the page but just before they hit the “reject
> all” button.

It's a bit dangerous indeed. This is not something this branch changed though. Let's fix this later.

> [...]

All your other remarks: done.

Thanks again.

[Raphaël Badin (rvb)]

> This may be a consequence of the method being slightly too big for comfortable detailed testing.

All done now. I've added a new utility method which is unit-tested.

[Jeroen T. Vermeulen (jtv)]

The change in the man page is a good catch! I just wrote that, and I hadn't made the connection. In general, thanks for the changes you've made. I know I'm being difficult as always. :) I'll make up for that by letting you have your way with the en-masse accept/reject actions.

In no-bootimages-warning.html, pre-existing text says “cluster's listing page” — should that apostrophe be there?

Could you give make_title_entry a quick docstring? Doesn't have to be much, just enough to place it in context.

In the ‘post’ method, maybe move the comments about what case each ‘if’ block handles, into the block? You'd get something like:

        if 'mass_accept_submit' in request.POST:
            # Accept clusters en masse.
            number = NodeGroup.objects.accept_all_pending()
            messages.info(request, "Accepted %d cluster(s)." % number)
            return HttpResponseRedirect(reverse('cluster-list'))

This way the if/elif/else structure comes out more clearly, because those statements are the only thing at their indentation level. It also means that the reader doesn't have to add a mental “if applicable” proviso to each comment. Once you're past the ‘if,’ the situation is clear and unambiguous.

Finally, I don't actually see the unit tests for make_title_entry. Missing commit? Not pushed? Diff not updated..?

review: Approve

[Raphaël Badin (rvb)]

> The change in the man page is a good catch! I just wrote that, and I hadn't
> made the connection. In general, thanks for the changes you've made. I know
> I'm being difficult as always. :) I'll make up for that by letting you have
> your way with the en-masse accept/reject actions.
>
> In no-bootimages-warning.html, pre-existing text says “cluster's listing page”
> — should that apostrophe be there?

Right, changed to "Cluster listing page."

> Could you give make_title_entry a quick docstring? Doesn't have to be much,
> just enough to place it in context.

Done.

> In the ‘post’ method, maybe move the comments about what case each ‘if’ block
> handles, into the block? You'd get something like:
>
> if 'mass_accept_submit' in request.POST:
> # Accept clusters en masse.
> number = NodeGroup.objects.accept_all_pending()
> messages.info(request, "Accepted %d cluster(s)." % number)
> return HttpResponseRedirect(reverse('cluster-list'))

Good point, done.

> Finally, I don't actually see the unit tests for make_title_entry. Missing
> commit? Not pushed? Diff not updated..?

I guess I forgot to push the revision, done now.

Thanks again for the review.

=== modified file 'docs/man/maas-import-pxe-files.8.rst'
--- docs/man/maas-import-pxe-files.8.rst	2014-04-01 07:59:47 +0000
+++ docs/man/maas-import-pxe-files.8.rst	2014-04-03 11:20:25 +0000
@@ -20,7 +20,7 @@
 An easier way to run the script is to trigger it from the MAAS web user
 interface.  To do that, log in to your MAAS as an administrator using a
 web browser, click the cogwheel icon in the top right of the page to go
-to the Settings page, and click "Import boot images."  This will start
+to the Clusters page, and click "Import boot images."  This will start
 imports on all cluster controllers simultaneously.  The same thing can
 also be done through the region-controller API, or through the
 command-line interface.

=== modified file 'man/maas-import-pxe-files.8'
--- man/maas-import-pxe-files.8	2014-04-01 07:59:47 +0000
+++ man/maas-import-pxe-files.8	2014-04-03 11:20:25 +0000
@@ -1,6 +1,6 @@
 .\" Man page generated from reStructuredText.
 .
-.TH "MAAS-IMPORT-PXE-FILES" "8" "April 01, 2014" "1.5" "MAAS"
+.TH "MAAS-IMPORT-PXE-FILES" "8" "April 03, 2014" "1.5" "MAAS"
 .SH NAME
 maas-import-pxe-files \- MAAS helper script
 .
@@ -46,7 +46,7 @@
 An easier way to run the script is to trigger it from the MAAS web user
 interface.  To do that, log in to your MAAS as an administrator using a
 web browser, click the cogwheel icon in the top right of the page to go
-to the Settings page, and click "Import boot images."  This will start
+to the Clusters page, and click "Import boot images."  This will start
 imports on all cluster controllers simultaneously.  The same thing can
 also be done through the region\-controller API, or through the
 command\-line interface.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2014-03-27 17:24:02 +0000
+++ src/maasserver/api.py	2014-04-03 11:20:25 +0000
@@ -2492,8 +2492,7 @@
     nodegroups_without_images = nodegroups_without_images.filter(
         status=NODEGROUP_STATUS.ACCEPTED)
     if nodegroups_without_images.exists():
-        accepted_clusters_url = (
-            "%s#accepted-clusters" % absolute_reverse("settings"))
+        accepted_clusters_url = absolute_reverse("cluster-list")
         warning = dedent("""\
             Some cluster controllers are missing boot images.  Either the
             import task has not been initiated (for each cluster, the task

=== modified file 'src/maasserver/templates/maasserver/base.html'
--- src/maasserver/templates/maasserver/base.html	2014-02-25 09:57:24 +0000
+++ src/maasserver/templates/maasserver/base.html	2014-04-03 11:20:25 +0000
@@ -72,6 +72,11 @@
                 <li class="{% block nav-active-node-list %}{% endblock %}">
                   <a href="{% url 'node-list' %}">Nodes</a>
                 </li>
+                {% if user.is_superuser %}
+                  <li class="{% block nav-active-cluster-list %}{% endblock %}">
+                    <a href="{% url 'cluster-list' %}">Clusters</a>
+                  </li>
+                {% endif %}
                 <li class="{% block nav-active-zone-list %}{% endblock %}">
                   <a href="{% url 'zone-list' %}">Zones</a>
                 </li>

=== renamed file 'src/maasserver/templates/maasserver/settings_cluster_listing.html' => 'src/maasserver/templates/maasserver/cluster_listing.html'
--- src/maasserver/templates/maasserver/settings_cluster_listing.html	2012-11-07 10:39:19 +0000
+++ src/maasserver/templates/maasserver/cluster_listing.html	2014-04-03 11:20:25 +0000
@@ -1,58 +1,54 @@
-<h2 id="accepted-clusters">Cluster controllers</h2>
-<table class="list" id="accepted-clusters-list">
-  <tbody>
-    {% for cluster in accepted_clusters %}
-      {% cycle 'even' 'odd' as cycle silent %}
-      {% include "maasserver/settings_cluster_listing_row.html" with cycle=cycle %}
-    {% endfor %}
-  </tbody>
-</table>
-<div>
-  <form method="POST"
-        action="{% url 'settings' %}">
-    {% csrf_token %}
-    <input type="hidden" name="import_all_boot_images" value="1" />
-    <input type="submit" class="button right" value="Import boot images" />
-  </form>
-</div>
-<div class="clear"></div>
-{% if pending_clusters %}
-<h3 id="pending-clusters">Pending clusters</h3>
-<table class="list" id="pending-clusters-list">
-  <tbody>
-    {% for cluster in pending_clusters %}
-      {% cycle 'even' 'odd' as cycle silent %}
-      {% include "maasserver/settings_cluster_listing_row.html" with cycle=cycle %}
-    {% endfor %}
-  </tbody>
-</table>
-<div>
-  <form id="reject_all_pending_nodegroups"
-        method="POST"
-        action="{% url 'settings' %}">
-    {% csrf_token %}
-    <input type="hidden" name="mass_accept_submit" value="1" />
-    <input type="submit" class="button right" value="Accept all" />
-  </form>
-  <form id="accept_all_pending_nodegroups"
-        method="POST"
-        action="{% url 'settings' %}">
-    {% csrf_token %}
-    <input type="hidden" name="mass_reject_submit" value="1" />
-    <input type="submit" class="button right space-right-small"
-           value="Reject all" />
-  </form>
-</div>
-<div class="clear"></div>
-{% endif %}
-{% if rejected_clusters %}
-<h3 id="rejected-clusters">Rejected clusters</h3>
-<table class="list" id="rejected-clusters-list">
-  <tbody>
-    {% for cluster in rejected_clusters %}
-      {% cycle 'even' 'odd' as cycle silent %}
-      {% include "maasserver/settings_cluster_listing_row.html" with cycle=cycle %}
-    {% endfor %}
-  </tbody>
-</table>
-{% endif %}
+{% extends "maasserver/base.html" %}
+
+{% block nav-active-cluster-list %}active{% endblock %}
+{% block title %}{{ status_name }} clusters{% endblock %}
+{% block page-title %}{{ current_count }}{% if input_query %} matching{% endif %} {{ status_name|lower }} cluster{{ current_count|pluralize }} in {% include "maasserver/site_title.html" %}{% endblock %}
+{% block site-switcher %}{% endblock %}
+{% block header-search %}{% endblock %}
+
+{% block content %}
+<div id="clusters" style="position: relative;">
+
+<h2 id="clusters">{{ title }}</h2>
+<table class="list" id="clusters-list">
+  {% include "maasserver/cluster_listing_head.html" %}
+  <tbody>
+    {% for cluster in cluster_list %}
+      {% cycle 'even' 'odd' as cycle silent %}
+      {% include "maasserver/cluster_listing_row.html" with cycle=cycle warning_no_images=warn_no_images %}
+    {% endfor %}
+  </tbody>
+</table>
+{% include "maasserver/pagination.html" %}
+<div>
+  {% if status == statuses.ACCEPTED %}
+    <form method="POST"
+          action="{% url 'cluster-list' %}">
+      {% csrf_token %}
+      <input type="hidden" name="import_all_boot_images" value="1" />
+      <input type="submit" class="button right" value="Import boot images" />
+    </form>
+  {% endif %}
+  {% if status == statuses.PENDING %}
+    <form id="reject_all_pending_nodegroups"
+          method="POST"
+          action="{% url 'cluster-list' %}">
+      {% csrf_token %}
+      <input type="hidden" name="mass_accept_submit" value="1" />
+      <input type="submit" class="button right" value="Accept all" />
+    </form>
+    <form id="accept_all_pending_nodegroups"
+          method="POST"
+          action="{% url 'cluster-list' %}">
+      {% csrf_token %}
+      <input type="hidden" name="mass_reject_submit" value="1" />
+      <input type="submit" class="button right space-right-small"
+             value="Reject all" />
+    </form>
+  {% endif %}
+
+</div>
+<div class="clear"></div>
+
+
+{% endblock %}
\ No newline at end of file

=== added file 'src/maasserver/templates/maasserver/cluster_listing_head.html'
--- src/maasserver/templates/maasserver/cluster_listing_head.html	1970-01-01 00:00:00 +0000
+++ src/maasserver/templates/maasserver/cluster_listing_head.html	2014-04-03 11:20:25 +0000
@@ -0,0 +1,10 @@
+<thead>
+  <tr>
+    <th>Name</th>
+    <th>Managed interfaces</th>
+    <th>Boot images</th>
+    <th>Nodes</th>
+    {% comment %} Action buttons {% endcomment %}
+    <th></th>
+  </tr>
+</thead>
\ No newline at end of file

=== renamed file 'src/maasserver/templates/maasserver/settings_cluster_listing_row.html' => 'src/maasserver/templates/maasserver/cluster_listing_row.html'
--- src/maasserver/templates/maasserver/settings_cluster_listing_row.html	2012-12-06 04:22:51 +0000
+++ src/maasserver/templates/maasserver/cluster_listing_row.html	2014-04-03 11:20:25 +0000
@@ -1,11 +1,24 @@
-<tr class="cluster {{ cycle }}"
+<tr class="cluster {{ cycle }} {% if warning_no_images and cluster.bootimage_set.count == 0 %}warning{% endif %}"
     id="{{ cluster.uuid }}">
   <td>
-    {{ cluster.cluster_name }}
+    <a href="{% url 'cluster-edit' cluster.uuid %}">{{ cluster.cluster_name }}</a>
+  </td>
+  <td>
+  {{ cluster.get_managed_interfaces|length }}
+  </td>
+  <td>
+  {% if warning_no_images and cluster.bootimage_set.count == 0 %}
+    0 <img src="{{ STATIC_URL }}img/warning.png" title="Warning: this cluster has no boot images."/>
+  {% else %}
     {% if cluster.bootimage_set.count == 0 %}
-      <span class="warning">(Warning: this cluster has no boot images.)</span>
+      0
+    {% else %}
+      <a title="View boot images"
+         href="{% url 'cluster-bootimages-list' cluster.uuid %}">{{ cluster.bootimage_set.count }}</a>
     {% endif %}
+  {% endif %}
   </td>
+  <td>{{ cluster.node_set.count }}</td>
   <td class="icon-controls">
     <a href="{% url 'cluster-edit' cluster.uuid %}"
        class="icon"

=== modified file 'src/maasserver/templates/maasserver/no-bootimages-warning.html'
--- src/maasserver/templates/maasserver/no-bootimages-warning.html	2014-03-27 10:51:28 +0000
+++ src/maasserver/templates/maasserver/no-bootimages-warning.html	2014-04-03 11:20:25 +0000
@@ -5,5 +5,5 @@
   <span class="console">/etc/maas/bootresources.yaml</span> on the cluster's
   machine) suits the configuration of the machines this cluster will have to
   handle and then run the import script by clicking the "Import boot images"
-  button on the  <a href="{% url 'settings' %}#clusters">cluster's listing page</a>.
+  button on the  <a href="{% url 'cluster-list' %}">cluster listing page</a>.
 </p>

=== modified file 'src/maasserver/templates/maasserver/nodegroup_confirm_delete.html'
--- src/maasserver/templates/maasserver/nodegroup_confirm_delete.html	2012-10-08 12:50:32 +0000
+++ src/maasserver/templates/maasserver/nodegroup_confirm_delete.html	2014-04-03 11:20:25 +0000
@@ -17,7 +17,7 @@
             <input type="hidden" name="post" value="yes" />
             <input type="submit" value="Delete cluster controller"
                    class="right" />
-            <a href="{% url 'settings' %}">Cancel</a>
+            <a href="{% url 'cluster-list' %}">Cancel</a>
           </form>
         </p>
     </div>

=== modified file 'src/maasserver/templates/maasserver/nodegroup_edit.html'
--- src/maasserver/templates/maasserver/nodegroup_edit.html	2014-03-25 10:35:14 +0000
+++ src/maasserver/templates/maasserver/nodegroup_edit.html	2014-04-03 11:20:25 +0000
@@ -15,7 +15,7 @@
       </ul>
       <input type="submit" value="Save cluster controller"
              class="button right" />
-      <a class="link-button" href="{% url 'settings' %}">Cancel</a>
+      <a class="link-button" href="{% url 'cluster-list' %}">Cancel</a>
       <h2>Interfaces</h2>
       {% with nb_interfaces=cluster.nodegroupinterface_set.count %}
       This cluster controller has {{ nb_interfaces }}

=== modified file 'src/maasserver/templates/maasserver/settings.html'
--- src/maasserver/templates/maasserver/settings.html	2012-11-30 17:21:28 +0000
+++ src/maasserver/templates/maasserver/settings.html	2014-04-03 11:20:25 +0000
@@ -70,11 +70,6 @@
       <div class="clear"></div>
     </div>
     <div class="divider"></div>
-    <div id="clusters" class="block size11 first">
-      {% include "maasserver/settings_cluster_listing.html" %}
-      <div class="clear"></div>
-    </div>
-    <div class="divider"></div>
     <div id="commissioning_scripts" class="block size11 first">
       {% include "maasserver/settings_commissioning_scripts.html" %}
       <div class="clear"></div>

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2014-03-19 13:54:20 +0000
+++ src/maasserver/tests/test_api.py	2014-04-03 11:20:25 +0000
@@ -53,6 +53,7 @@
 from maasserver.testing.oauthclient import OAuthAuthenticatedClient
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.tests.test_forms import make_interface_settings
+from maasserver.utils import absolute_reverse
 from maasserver.utils.orm import get_one
 from maastesting.djangotestcase import TransactionTestCase
 from maastesting.matchers import MockCalledOnceWith
@@ -754,7 +755,8 @@
             [args[0][0] for args in recorder.call_args_list])
         # The persistent error message links to the clusters listing.
         self.assertIn(
-            "/settings/#accepted-clusters", recorder.call_args_list[0][0][1])
+            absolute_reverse("cluster-list"),
+            recorder.call_args_list[0][0][1])
 
     def test_warns_if_any_nodegroup_has_no_images(self):
         factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)

=== modified file 'src/maasserver/urls.py'
--- src/maasserver/urls.py	2014-03-25 10:35:14 +0000
+++ src/maasserver/urls.py	2014-04-03 11:20:25 +0000
@@ -21,12 +21,22 @@
     url,
     )
 from django.contrib.auth.decorators import user_passes_test
+from maasserver.enum import NODEGROUP_STATUS
 from maasserver.models import Node
 from maasserver.views import TextTemplateView
 from maasserver.views.account import (
     login,
     logout,
     )
+from maasserver.views.clusters import (
+    BootImagesListView,
+    ClusterDelete,
+    ClusterEdit,
+    ClusterInterfaceCreate,
+    ClusterInterfaceDelete,
+    ClusterInterfaceEdit,
+    ClusterListView,
+    )
 from maasserver.views.networks import (
     NetworkAdd,
     NetworkDelete,
@@ -60,14 +70,6 @@
     AccountsView,
     settings,
     )
-from maasserver.views.settings_clusters import (
-    BootImagesListView,
-    ClusterDelete,
-    ClusterEdit,
-    ClusterInterfaceCreate,
-    ClusterInterfaceDelete,
-    ClusterInterfaceEdit,
-    )
 from maasserver.views.settings_commissioning_scripts import (
     CommissioningScriptCreate,
     CommissioningScriptDelete,
@@ -116,7 +118,6 @@
         r'^account/prefs/sshkey/delete/(?P<keyid>\d*)/$',
         SSHKeyDeleteView.as_view(), name='prefs-delete-sshkey'),
     )
-
 # Logout view.
 urlpatterns += patterns(
     'maasserver.views',
@@ -159,6 +160,18 @@
 urlpatterns += patterns(
     'maasserver.views',
     adminurl(
+        r'^clusters/$',
+        ClusterListView.as_view(status=NODEGROUP_STATUS.ACCEPTED),
+        name='cluster-list'),
+    adminurl(
+        r'^clusters/pending/$',
+        ClusterListView.as_view(status=NODEGROUP_STATUS.PENDING),
+        name='cluster-list-pending'),
+    adminurl(
+        r'^clusters/rejected/$',
+        ClusterListView.as_view(status=NODEGROUP_STATUS.REJECTED),
+        name='cluster-list-rejected'),
+    adminurl(
         r'^clusters/(?P<uuid>[\w\-]+)/edit/$', ClusterEdit.as_view(),
         name='cluster-edit'),
     adminurl(

=== renamed file 'src/maasserver/views/settings_clusters.py' => 'src/maasserver/views/clusters.py'
--- src/maasserver/views/settings_clusters.py	2014-03-25 10:35:14 +0000
+++ src/maasserver/views/clusters.py	2014-04-03 11:20:25 +0000
@@ -1,7 +1,7 @@
 # Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Cluster Settings views."""
+"""Cluster views."""
 
 from __future__ import (
     absolute_import,
@@ -19,17 +19,29 @@
     "ClusterInterfaceCreate",
     "ClusterInterfaceDelete",
     "ClusterInterfaceEdit",
+    "ClusterListView",
     ]
 
+from collections import OrderedDict
+
 from django.contrib import messages
 from django.core.urlresolvers import reverse
 from django.http import HttpResponseRedirect
 from django.shortcuts import get_object_or_404
+from django.utils.safestring import mark_safe
 from django.views.generic import (
     CreateView,
     DeleteView,
     UpdateView,
     )
+from django.views.generic.edit import (
+    FormMixin,
+    ProcessFormView,
+    )
+from maasserver.enum import (
+    NODEGROUP_STATUS,
+    NODEGROUP_STATUS_CHOICES,
+    )
 from maasserver.forms import (
     NodeGroupEdit,
     NodeGroupInterfaceForm,
@@ -41,6 +53,99 @@
 from maasserver.views import PaginatedListView
 
 
+class ClusterListView(PaginatedListView, FormMixin, ProcessFormView):
+    template_name = 'maasserver/cluster_listing.html'
+    context_object_name = "cluster_list"
+    status = None
+
+    def get_queryset(self):
+        return NodeGroup.objects.filter(
+            status=self.status).order_by('cluster_name')
+
+    # A record of the urls used to reach the clusters of different
+    # statuses.
+    status_links = OrderedDict((
+        (NODEGROUP_STATUS.ACCEPTED, 'cluster-list'),
+        (NODEGROUP_STATUS.PENDING, 'cluster-list-pending'),
+        (NODEGROUP_STATUS.REJECTED, 'cluster-list-rejected'),
+    ))
+
+    def make_title_entry(self, status, link_name):
+        """Generate an entry as used by make_cluster_listing_title().
+
+        This is a utility method only used by make_cluster_listing_title.
+        It is a separate method for clarity and to help testing."""
+        link = reverse(link_name)
+        status_name = NODEGROUP_STATUS_CHOICES[status][1]
+        nb_clusters = NodeGroup.objects.filter(
+            status=status).count()
+        entry = "%d %s cluster%s" % (
+            nb_clusters,
+            status_name.lower(),
+            's' if nb_clusters != 1 else '')
+        if nb_clusters != 0 and status != self.status:
+            entry = '<a href="%s">%s</a>' % (link, entry)
+        return entry
+
+    def make_cluster_listing_title(self):
+        """Generate this view's title with "tabs" for each cluster status.
+
+        Generate a title for this view with the number of clusters for each
+        possible status.  The title includes the links to the other listings
+        (i.e. if this is the listing for the accepted clusters, include links
+        to the listings of the pending/rejected clusters).  The title will be
+        of the form: "3 accepted clusters / 1 pending cluster / 2 rejected
+        clusters". (This is simpler to do in the view rather than write this
+        using the template language.)
+        """
+        return mark_safe(
+            ' / '.join(
+                self.make_title_entry(status, link_name)
+                for status, link_name in self.status_links.items()))
+
+    def get_context_data(self, **kwargs):
+        context = super(ClusterListView, self).get_context_data(**kwargs)
+        context['current_count'] = NodeGroup.objects.filter(
+            status=self.status).count()
+        context['title'] = self.make_cluster_listing_title()
+        # Display warnings for clusters that have no images, but only for the
+        # display of 'accepted' clusters.
+        context['warn_no_images'] = self.status == NODEGROUP_STATUS.ACCEPTED
+        context['status'] = self.status
+        context['statuses'] = NODEGROUP_STATUS
+        context['status_name'] = NODEGROUP_STATUS_CHOICES[self.status][1]
+        return context
+
+    def post(self, request, *args, **kwargs):
+        """Handle a POST request."""
+        if 'mass_accept_submit' in request.POST:
+            # Process accept clusters en masse.
+            number = NodeGroup.objects.accept_all_pending()
+            messages.info(request, "Accepted %d cluster(s)." % number)
+            return HttpResponseRedirect(reverse('cluster-list'))
+
+        elif 'mass_reject_submit' in request.POST:
+            # Process reject clusters en masse.
+            number = NodeGroup.objects.reject_all_pending()
+            messages.info(request, "Rejected %d cluster(s)." % number)
+            return HttpResponseRedirect(reverse('cluster-list'))
+
+        elif 'import_all_boot_images' in request.POST:
+            # Import PXE files for all the accepted clusters.
+            NodeGroup.objects.import_boot_images_accepted_clusters()
+            message = (
+                "Import of boot images started on all cluster controllers.  "
+                "Importing the boot images can take a long time depending on "
+                "the available bandwidth.")
+            messages.info(request, message)
+            return HttpResponseRedirect(reverse('cluster-list'))
+
+        else:
+            # Unknown action: redirect to the cluster listing page (this
+            # shouldn't happen).
+            return HttpResponseRedirect(reverse('cluster-list'))
+
+
 class ClusterEdit(UpdateView):
     model = NodeGroup
     template_name = 'maasserver/nodegroup_edit.html'
@@ -54,7 +159,7 @@
         return context
 
     def get_success_url(self):
-        return reverse('settings')
+        return reverse('cluster-list')
 
     def get_object(self):
         uuid = self.kwargs.get('uuid', None)
@@ -75,7 +180,7 @@
         return get_object_or_404(NodeGroup, uuid=uuid)
 
     def get_next_url(self):
-        return reverse('settings')
+        return reverse('cluster-list')
 
     def delete(self, request, *args, **kwargs):
         cluster = self.get_object()

=== modified file 'src/maasserver/views/settings.py'
--- src/maasserver/views/settings.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/views/settings.py	2014-04-03 11:20:25 +0000
@@ -38,7 +38,6 @@
 from django.views.generic.base import TemplateView
 from django.views.generic.detail import SingleObjectTemplateResponseMixin
 from django.views.generic.edit import ModelFormMixin
-from maasserver.enum import NODEGROUP_STATUS
 from maasserver.exceptions import CannotDeleteUserException
 from maasserver.forms import (
     CommissioningForm,
@@ -48,10 +47,7 @@
     NewUserCreationForm,
     UbuntuForm,
     )
-from maasserver.models import (
-    NodeGroup,
-    UserProfile,
-    )
+from maasserver.models import UserProfile
 from maasserver.views import process_form
 from metadataserver.models import CommissioningScript
 
@@ -186,36 +182,6 @@
     if response is not None:
         return response
 
-    # Process accept clusters en masse.
-    if 'mass_accept_submit' in request.POST:
-        number = NodeGroup.objects.accept_all_pending()
-        messages.info(request, "Accepted %d cluster(s)." % number)
-        return HttpResponseRedirect(reverse('settings'))
-
-    # Process reject clusters en masse.
-    if 'mass_reject_submit' in request.POST:
-        number = NodeGroup.objects.reject_all_pending()
-        messages.info(request, "Rejected %d cluster(s)." % number)
-        return HttpResponseRedirect(reverse('settings'))
-
-    # Import PXE files for all the accepted clusters.
-    if 'import_all_boot_images' in request.POST:
-        NodeGroup.objects.import_boot_images_accepted_clusters()
-        message = (
-            "Import of boot images started on all cluster controllers.  "
-            "Importing the boot images can take a long time depending on "
-            "the available bandwidth.")
-        messages.info(request, message)
-        return HttpResponseRedirect(reverse('settings'))
-
-    # Cluster listings.
-    accepted_clusters = NodeGroup.objects.filter(
-        status=NODEGROUP_STATUS.ACCEPTED).order_by('cluster_name')
-    pending_clusters = NodeGroup.objects.filter(
-        status=NODEGROUP_STATUS.PENDING).order_by('cluster_name')
-    rejected_clusters = NodeGroup.objects.filter(
-        status=NODEGROUP_STATUS.REJECTED).order_by('cluster_name')
-
     # Commissioning scripts.
     commissioning_scripts = CommissioningScript.objects.all()
 
@@ -224,9 +190,6 @@
         {
             'user_list': user_list,
             'commissioning_scripts': commissioning_scripts,
-            'accepted_clusters': accepted_clusters,
-            'pending_clusters': pending_clusters,
-            'rejected_clusters': rejected_clusters,
             'maas_and_network_form': maas_and_network_form,
             'commissioning_form': commissioning_form,
             'ubuntu_form': ubuntu_form,

=== modified file 'src/maasserver/views/tests/test_boot_image_list.py'
--- src/maasserver/views/tests/test_boot_image_list.py	2014-03-27 07:39:38 +0000
+++ src/maasserver/views/tests/test_boot_image_list.py	2014-04-03 11:20:25 +0000
@@ -21,7 +21,7 @@
 from lxml.html import fromstring
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from maasserver.views.settings_clusters import BootImagesListView
+from maasserver.views.clusters import BootImagesListView
 from testtools.matchers import ContainsAll
 
 

=== renamed file 'src/maasserver/views/tests/test_settings_clusters.py' => 'src/maasserver/views/tests/test_clusters.py'
--- src/maasserver/views/tests/test_settings_clusters.py	2014-03-25 12:53:32 +0000
+++ src/maasserver/views/tests/test_clusters.py	2014-04-03 11:20:25 +0000
@@ -20,10 +20,12 @@
 from lxml.html import fromstring
 from maasserver.enum import (
     NODEGROUP_STATUS,
+    NODEGROUP_STATUS_CHOICES,
     NODEGROUPINTERFACE_MANAGEMENT,
     )
 from maasserver.models import (
     NodeGroup,
+    nodegroup as nodegroup_module,
     NodeGroupInterface,
     )
 from maasserver.testing import (
@@ -33,25 +35,42 @@
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.utils import map_enum
+from maasserver.views.clusters import ClusterListView
+from mock import (
+    ANY,
+    call,
+    )
 from testtools.matchers import (
     AllMatch,
     Contains,
     ContainsAll,
     Equals,
+    HasLength,
     MatchesStructure,
     )
 
 
 class ClusterListingTest(MAASServerTestCase):
 
-    def test_settings_contains_links_to_edit_and_delete_clusters(self):
+    scenarios = [
+        ('accepted-clusters', {'status': NODEGROUP_STATUS.ACCEPTED}),
+        ('pending-clusters', {'status': NODEGROUP_STATUS.PENDING}),
+        ('rejected-clusters', {'status': NODEGROUP_STATUS.REJECTED}),
+    ]
+
+    def get_url(self):
+        """Return the listing url used in this scenario."""
+        return reverse(ClusterListView.status_links[
+            self.status])
+
+    def test_cluster_listing_contains_links_to_manipulate_clusters(self):
         self.client_log_in(as_admin=True)
         nodegroups = {
-            factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED),
-            factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
-            factory.make_node_group(status=NODEGROUP_STATUS.REJECTED),
+            factory.make_node_group(status=self.status)
+            for _ in range(3)
             }
-        links = get_content_links(self.client.get(reverse('settings')))
+        links = get_content_links(self.client.get(self.get_url()))
         nodegroup_edit_links = [
             reverse('cluster-edit', args=[nodegroup.uuid])
             for nodegroup in nodegroups]
@@ -62,6 +81,166 @@
             links,
             ContainsAll(nodegroup_edit_links + nodegroup_delete_links))
 
+    def make_listing_view(self, status):
+        view = ClusterListView()
+        view.status = status
+        return view
+
+    def test_make_title_entry_returns_link_for_other_status(self):
+        # If the entry's status is different from the view's status,
+        # the returned entry is a link.
+        other_status = factory.getRandomChoice(
+            NODEGROUP_STATUS_CHOICES, but_not=[self.status])
+        factory.make_node_group(status=other_status)
+        link_name = ClusterListView.status_links[other_status]
+        view = self.make_listing_view(self.status)
+        entry = view.make_title_entry(other_status, link_name)
+        status_name = NODEGROUP_STATUS_CHOICES[other_status][1]
+        self.assertEqual(
+            '<a href="%s">1 %s cluster</a>' % (
+                reverse(link_name), status_name.lower()),
+            entry)
+
+    def test_make_title_entry_returns_title_if_no_cluster(self):
+        # If no cluster correspond to the entry's status, the returned
+        # entry is not a link: it's a simple mention '0 <status> clusters'.
+        other_status = factory.getRandomChoice(
+            NODEGROUP_STATUS_CHOICES, but_not=[self.status])
+        link_name = ClusterListView.status_links[other_status]
+        view = self.make_listing_view(self.status)
+        entry = view.make_title_entry(other_status, link_name)
+        status_name = NODEGROUP_STATUS_CHOICES[other_status][1]
+        self.assertEqual(
+            '0 %s clusters' % status_name.lower(), entry)
+
+    def test_title_displays_number_of_clusters(self):
+        for _ in range(3):
+            factory.make_node_group(status=self.status)
+        view = self.make_listing_view(self.status)
+        status_name = NODEGROUP_STATUS_CHOICES[self.status][1]
+        title = view.make_cluster_listing_title()
+        self.assertIn("3 %s clusters" % status_name.lower(), title)
+
+    def test_title_contains_links_to_other_listings(self):
+        view = self.make_listing_view(self.status)
+        other_statuses = []
+        # Compute a list with the statuses of the clusters not being
+        # displayed by the 'view'.  Create clusters with these statuses.
+        for status in map_enum(NODEGROUP_STATUS).values():
+            if status != self.status:
+                other_statuses.append(status)
+                factory.make_node_group(status=status)
+        for status in other_statuses:
+            link_name = ClusterListView.status_links[status]
+            title = view.make_cluster_listing_title()
+            self.assertIn(reverse(link_name), title)
+
+    def test_listing_is_paginated(self):
+        self.patch(ClusterListView, "paginate_by", 2)
+        self.client_log_in(as_admin=True)
+        for _ in range(3):
+            factory.make_node_group(status=self.status)
+        response = self.client.get(self.get_url())
+        self.assertEqual(httplib.OK, response.status_code)
+        doc = fromstring(response.content)
+        self.assertThat(
+            doc.cssselect('div.pagination'),
+            HasLength(1),
+            "Couldn't find pagination tag.")
+
+
+class ClusterListingAccess(MAASServerTestCase):
+
+    def test_admin_sees_cluster_tab(self):
+        self.client_log_in(as_admin=True)
+        links = get_content_links(
+            self.client.get(reverse('index')), element='#main-nav')
+        self.assertIn(reverse('cluster-list'), links)
+
+    def test_non_admin_doesnt_see_cluster_tab(self):
+        self.client_log_in(as_admin=False)
+        links = get_content_links(
+            self.client.get(reverse('index')), element='#main-nav')
+        self.assertNotIn(reverse('cluster-list'), links)
+
+
+class ClusterPendingListingTest(MAASServerTestCase):
+
+    def test_pending_listing_contains_form_to_accept_all_nodegroups(self):
+        self.client_log_in(as_admin=True)
+        factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
+        response = self.client.get(reverse('cluster-list-pending'))
+        doc = fromstring(response.content)
+        forms = doc.cssselect('form#accept_all_pending_nodegroups')
+        self.assertEqual(1, len(forms))
+
+    def test_pending_listing_contains_form_to_reject_all_nodegroups(self):
+        self.client_log_in(as_admin=True)
+        factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
+        response = self.client.get(reverse('cluster-list-pending'))
+        doc = fromstring(response.content)
+        forms = doc.cssselect('form#reject_all_pending_nodegroups')
+        self.assertEqual(1, len(forms))
+
+    def test_pending_listing_accepts_all_pending_nodegroups_POST(self):
+        self.client_log_in(as_admin=True)
+        nodegroups = {
+            factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
+            factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
+        }
+        response = self.client.post(
+            reverse('cluster-list-pending'), {'mass_accept_submit': 1})
+        self.assertEqual(httplib.FOUND, response.status_code)
+        self.assertEqual(
+            [reload_object(nodegroup).status for nodegroup in nodegroups],
+            [NODEGROUP_STATUS.ACCEPTED] * 2)
+
+    def test_pending_listing_rejects_all_pending_nodegroups_POST(self):
+        self.client_log_in(as_admin=True)
+        nodegroups = {
+            factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
+            factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
+        }
+        response = self.client.post(
+            reverse('cluster-list-pending'), {'mass_reject_submit': 1})
+        self.assertEqual(httplib.FOUND, response.status_code)
+        self.assertEqual(
+            [reload_object(nodegroup).status for nodegroup in nodegroups],
+            [NODEGROUP_STATUS.REJECTED] * 2)
+
+
+class ClusterAcceptedListingTest(MAASServerTestCase):
+
+    def test_accepted_listing_import_boot_images_calls_tasks(self):
+        self.client_log_in(as_admin=True)
+        recorder = self.patch(nodegroup_module, 'import_boot_images')
+        accepted_nodegroups = [
+            factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED),
+            factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED),
+        ]
+        response = self.client.post(
+            reverse('cluster-list'), {'import_all_boot_images': 1})
+        self.assertEqual(httplib.FOUND, response.status_code)
+        calls = [
+            call(queue=nodegroup.work_queue, kwargs=ANY)
+            for nodegroup in accepted_nodegroups
+        ]
+        self.assertItemsEqual(calls, recorder.apply_async.call_args_list)
+
+    def test_a_warning_is_displayed_if_the_cluster_has_no_boot_images(self):
+        self.client_log_in(as_admin=True)
+        nodegroup = factory.make_node_group(
+            status=NODEGROUP_STATUS.ACCEPTED)
+        response = self.client.get(reverse('cluster-list'))
+        document = fromstring(response.content)
+        nodegroup_row = document.xpath("//tr[@id='%s']" % nodegroup.uuid)[0]
+        self.assertIn('warning', nodegroup_row.get('class'))
+        warning_elems = (
+            nodegroup_row.xpath(
+                "//img[@title='Warning: this cluster has no boot images.']"))
+        self.assertEqual(
+            1, len(warning_elems), "No warning about missing boot images.")
+
 
 class ClusterDeleteTest(MAASServerTestCase):
 
@@ -71,7 +250,7 @@
         delete_link = reverse('cluster-delete', args=[nodegroup.uuid])
         response = self.client.post(delete_link, {'post': 'yes'})
         self.assertEqual(
-            (httplib.FOUND, reverse('settings')),
+            (httplib.FOUND, reverse('cluster-list')),
             (response.status_code, extract_redirect(response)))
         self.assertFalse(
             NodeGroup.objects.filter(uuid=nodegroup.uuid).exists())

=== modified file 'src/maasserver/views/tests/test_settings.py'
--- src/maasserver/views/tests/test_settings.py	2014-04-01 06:50:01 +0000
+++ src/maasserver/views/tests/test_settings.py	2014-04-03 11:20:25 +0000
@@ -23,11 +23,9 @@
 from maasserver.enum import (
     COMMISSIONING_DISTRO_SERIES_CHOICES,
     DISTRO_SERIES,
-    NODEGROUP_STATUS,
     )
 from maasserver.models import (
     Config,
-    nodegroup as nodegroup_module,
     UserProfile,
     )
 from maasserver.testing import (
@@ -37,10 +35,6 @@
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from mock import (
-    ANY,
-    call,
-    )
 
 
 class SettingsTest(MAASServerTestCase):
@@ -185,73 +179,6 @@
             new_kernel_opts,
             Config.objects.get_config('kernel_opts'))
 
-    def test_settings_contains_form_to_accept_all_nodegroups(self):
-        self.client_log_in(as_admin=True)
-        factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
-        response = self.client.get(reverse('settings'))
-        doc = fromstring(response.content)
-        forms = doc.cssselect('form#accept_all_pending_nodegroups')
-        self.assertEqual(1, len(forms))
-
-    def test_settings_contains_form_to_reject_all_nodegroups(self):
-        self.client_log_in(as_admin=True)
-        factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
-        response = self.client.get(reverse('settings'))
-        doc = fromstring(response.content)
-        forms = doc.cssselect('form#reject_all_pending_nodegroups')
-        self.assertEqual(1, len(forms))
-
-    def test_settings_accepts_all_pending_nodegroups_POST(self):
-        self.client_log_in(as_admin=True)
-        nodegroups = {
-            factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
-            factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
-        }
-        response = self.client.post(
-            reverse('settings'), {'mass_accept_submit': 1})
-        self.assertEqual(httplib.FOUND, response.status_code)
-        self.assertEqual(
-            [reload_object(nodegroup).status for nodegroup in nodegroups],
-            [NODEGROUP_STATUS.ACCEPTED] * 2)
-
-    def test_settings_rejects_all_pending_nodegroups_POST(self):
-        self.client_log_in(as_admin=True)
-        nodegroups = {
-            factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
-            factory.make_node_group(status=NODEGROUP_STATUS.PENDING),
-        }
-        response = self.client.post(
-            reverse('settings'), {'mass_reject_submit': 1})
-        self.assertEqual(httplib.FOUND, response.status_code)
-        self.assertEqual(
-            [reload_object(nodegroup).status for nodegroup in nodegroups],
-            [NODEGROUP_STATUS.REJECTED] * 2)
-
-    def test_settings_import_boot_images_calls_tasks(self):
-        self.client_log_in(as_admin=True)
-        recorder = self.patch(nodegroup_module, 'import_boot_images')
-        accepted_nodegroups = [
-            factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED),
-            factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED),
-        ]
-        response = self.client.post(
-            reverse('settings'), {'import_all_boot_images': 1})
-        self.assertEqual(httplib.FOUND, response.status_code)
-        calls = [
-            call(queue=nodegroup.work_queue, kwargs=ANY)
-            for nodegroup in accepted_nodegroups
-        ]
-        self.assertItemsEqual(calls, recorder.apply_async.call_args_list)
-
-    def test_cluster_no_boot_images_message_displayed_if_no_boot_images(self):
-        self.client_log_in(as_admin=True)
-        nodegroup = factory.make_node_group(
-            status=NODEGROUP_STATUS.ACCEPTED)
-        response = self.client.get(reverse('settings'))
-        document = fromstring(response.content)
-        nodegroup_row = document.xpath("//tr[@id='%s']" % nodegroup.uuid)[0]
-        self.assertIn('no boot images', nodegroup_row.text_content())
-
 
 class NonAdminSettingsTest(MAASServerTestCase):