Merge lp:~rvb/maas/ui-power-parameters into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Add power_type and power_parameters to the add node UI.

This branch add the fields power_type and power_parameters to the "add node" UI (if the user is an admin).

= Notes =

- I've changed DictCharField to return the empty string instead of '<fieldset></fieldset>' when the number of sub-fields is zero. I've done that because, for a reason I cannot fathom, http://paste.ubuntu.com/1035380/ does not validate. I suppose <fieldset> must contain a tag or something like that. Anyway, returning the empty string in that case is better anyway.

- I've added power_parameters.js and enums.js in js-conf.html (i.e. globally).

- no need for the "architecture" snippet (in snippet.html) to be a snippet on its own, I've included it in the general snippet "add-node".

[Gavin Panella (allenap)]

[1]

+ setupPowerParameterField: function() {
+ if (Y.Lang.isValue(Y.one('#id_power_type'))) {
...
+ widget.bindTo(Y.one('#id_power_type'), 'change');

Saving the result of this lookup will be more efficient and be clearer
to read:

    setupPowerParameterField: function() {
        var power_type_node = Y.one('#id_power_type');
        if (Y.Lang.isValue(power_type_node)) {
            ...
            widget.bindTo(power_type_node, 'change');

[2]

+ setUpPowerTypeEnum: function() {
+ // Override module.POWER_TYPE_ENUM for the sake of testing.
+ this.POWER_TYPE_ENUM = module.POWER_TYPE_ENUM;
+ module.POWER_TYPE_ENUM = ENUM;
+ },

Not for this branch, but it would be cool to have a this.patch()
method, like in the Python test suite, that arranges a clean-up hook.

[3]

+ <input type="text" maxlength="30" name="hostname" id="id_hostname" />
...
+ this.add_node_template = Y.one('#add-node').getContent();
+ this.add_node_admin_snippet = Y.one('#add-node-admin').getContent();

Why do some IDs have hyphens and some underscores? Is this Django
versus YUI?

[4]

+ self.assertEqual(
+ 1, len(snippets),
+ "The snippet '%s' does not exist." % template_selector)

There could be two.

[5]

+ def _assertTemplateExistsAndContains(self, content, template_selector,
+ contains_selector, reverse=False):
...
+ if reverse:
+ self.assertEqual(
+ 0, len(selects),
+ "The element '%s' does exist." % contains_selector)
+ else:
+ self.assertEqual(
+ 1, len(selects),
+ "The element '%s' does not exist." % contains_selector,)
+
+ def assertTemplateExistsAndContains(self, content, template_selector,
...
+ self._assertTemplateExistsAndContains(
+ content, template_selector, contains_selector)
+
+ def assertTemplateExistsAndDoesNotContain(self, content, template_selector,
...
+ self._assertTemplateExistsAndContains(
+ content, template_selector, contains_selector, reverse=True)

The above is a bit of an anti-pattern. The `reverse` condition should
ideally be moved elsewhere, like up into the calling functions:

    def findTemplate(self, content, selector):
        snippets = fromstring(content).cssselect(template_selector)
        self.assertEqual(
            1, len(snippets),
            "The snippet '%s' does not exist." % selector) # XXX: See [4]
        return fromstring(snippets[0].text)

    def assertTemplateExistsAndContains(self, content, template_selector,
        ...
        template = self.findTemplate(content, template_selector)
        self.assertNotEqual(
            0, len(template.cssselect(contains_selector)),
            "The element '%s' does not exist." % contains_selector)

    def assertTemplateExistsAndDoesNotContain(self, content, template_selector,
        ...
        template = self.findTemplate(content, template_selector)
        self.assertEqual(
            0, len(template.cssselect(contains_selector)),
            "The element '%s' should not exist." % contains_se...

review: Approve

[Raphaël Badin (rvb)]

Thanks for the review Gavin!

> [1]
> [...]
> Saving the result of this lookup will be more efficient and be clearer
> to read:
> [...]

Done.

> [2]
> Not for this branch, but it would be cool to have a this.patch()
> method, like in the Python test suite, that arranges a clean-up hook.
> [...]

Indeed. We clearly lack our python test arsenal in Javascript…

> [3]
>
> + <input type="text" maxlength="30" name="hostname" id="id_hostname" />
> ...
> + this.add_node_template = Y.one('#add-node').getContent();
> + this.add_node_admin_snippet = Y.one('#add-node-admin').getContent();
>
> Why do some IDs have hyphens and some underscores? Is this Django
> versus YUI?

That's right, it's just a convention I use to remind me of what comes from Django and what does not.

> [4]
>
> + self.assertEqual(
> + 1, len(snippets),
> + "The snippet '%s' does not exist." % template_selector)
>
> There could be two.

Right, I've changed the error message.

> [5]
> [...]
> The above is a bit of an anti-pattern. The `reverse` condition should
> ideally be moved elsewhere, like up into the calling functions:
> [...]

Good point, I confess I was a little bit lazy on this one… fixed!

=== modified file 'src/maasserver/config_forms.py'
--- src/maasserver/config_forms.py	2012-06-07 15:57:27 +0000
+++ src/maasserver/config_forms.py	2012-06-11 13:08:19 +0000
@@ -279,6 +279,9 @@
             widgets = self.widgets
         if not isinstance(value, list):
             value = self.decompress(value)
+        if len(widgets) == 0:
+            return mark_safe(self.format_output(''))
+
         output = ['<fieldset>']
         final_attrs = self.build_attrs(attrs)
         id_ = final_attrs.get('id', None)

=== modified file 'src/maasserver/context_processors.py'
--- src/maasserver/context_processors.py	2012-06-06 16:01:55 +0000
+++ src/maasserver/context_processors.py	2012-06-11 13:08:19 +0000
@@ -16,7 +16,7 @@
 
 from django.conf import settings
 from maasserver.components import get_persistent_errors
-from maasserver.forms import NodeForm
+from maasserver.forms import get_node_edit_form
 from maasserver.models import Config
 from maasserver.power_parameters import POWER_TYPE_PARAMETERS
 from provisioningserver.enum import POWER_TYPE
@@ -34,7 +34,7 @@
 def global_options(context):
     return {
         'persistent_errors': get_persistent_errors(),
-        'node_form': NodeForm(),
+        'node_form': get_node_edit_form(context.user)(),
         'POWER_TYPE_PARAMETERS_FIELDS':
             [(power_type, field.widget.render('power_parameters', []))
                 for power_type, field in POWER_TYPE_PARAMETERS.items()

=== modified file 'src/maasserver/static/js/node_add.js'
--- src/maasserver/static/js/node_add.js	2012-06-11 07:34:55 +0000
+++ src/maasserver/static/js/node_add.js	2012-06-11 13:08:19 +0000
@@ -25,6 +25,10 @@
 
 AddNodeWidget.NAME = 'node-add-widget';
 
+module.POWER_TYPE_ENUM = Y.maas.enums.POWER_TYPE;
+
+module.POWER_PARAM_TEMPLATE_PREFIX = '#power-param-form-';
+
 AddNodeWidget.ATTRS = {
 
    /**
@@ -141,7 +145,6 @@
             .append(global_error)
             .append(operation)
             .append(Y.Node.create(this.add_node))
-            .append(Y.Node.create(this.add_architecture))
             .append(Y.Node.create(this.add_macaddress))
             .append(macaddress_add_link)
             .append(buttons);
@@ -207,16 +210,36 @@
     renderUI: function() {
         // Load form snippets.
         this.add_macaddress = Y.one('#add-macaddress').getContent();
-        this.add_architecture = Y.one('#add-architecture').getContent();
         this.add_node = Y.one('#add-node').getContent();
         // Create panel's content.
         var heading = Y.Node.create('<h2 />')
             .set('text', "Add node");
         this.get('srcNode').append(heading).append(this.createForm());
+        this.setupPowerParameterField();
         this.initializeNodes();
     },
 
     /**
+     * If the 'power_type' field is present, setup the linked
+     * 'power_parameter' field.
+     *
+     * @method setupPowerParameterField
+     */
+    setupPowerParameterField: function() {
+        if (Y.Lang.isValue(Y.one('#id_power_type'))) {
+            // If the 'power_type' field is present, setup the linked
+            // field 'power_parameters'.
+            var widget = new Y.maas.power_parameters.LinkedContentWidget({
+                srcNode: '.power_parameters',
+                driverEnum: module.POWER_TYPE_ENUM,
+                templatePrefix: module.POWER_PARAM_TEMPLATE_PREFIX
+                });
+            widget.bindTo(Y.one('#id_power_type'), 'change');
+            widget.render();
+        }
+    },
+
+    /**
      * Show the widget
      *
      * @method showWidget
@@ -379,5 +402,5 @@
 };
 
 }, '0.1', {'requires': ['io', 'node', 'widget', 'event', 'event-custom',
-                        'maas.morph']}
+                        'maas.morph', 'maas.enums', 'maas.power_parameters']}
 );

=== modified file 'src/maasserver/static/js/tests/test_node_add.html'
--- src/maasserver/static/js/tests/test_node_add.html	2012-03-29 04:21:34 +0000
+++ src/maasserver/static/js/tests/test_node_add.html	2012-06-11 13:08:19 +0000
@@ -9,6 +9,8 @@
     <script type="text/javascript" src="../testing/testrunner.js"></script>
     <script type="text/javascript" src="../testing/testing.js"></script>
     <script type="text/javascript" src="../morph.js"></script>
+    <script type="text/javascript" src="../power_parameters.js"></script>
+    <script type="text/javascript" src="../enums.js"></script>
     <!-- The module under test -->
     <script type="text/javascript" src="../node_add.js"></script>
     <!-- The test suite -->
@@ -22,30 +24,47 @@
         nodes_handler: '/api/nodes/'
       }
     };
+    var ENUM = {
+        "DEFAULT": "",
+        "value1": "value1"
+    };
     // -->
     </script>
   </head>
   <body>
+  <script type="text/x-template" id="power-param-form-">
+  </script>
+  <script type="text/x-template" id="power-param-form-value1">
+  </script>
   <script type="text/x-template" id="add-macaddress">
     <p></p>
   </script>
-  <script type="text/x-template" id="add-architecture">
-    <p>
-    <label for="id_architecture">Architecture
-    </label>
-    <select id="id_architecture" name="architecture">
-      <option value="amd64">amd64</option>
-      <option value="i386">i386</option>
-    </select>
-    <input type="text" maxlength="30" name="hostname" id="id_hostname" />
-    </p>
-  </script>
   <script type="text/x-template" id="add-node">
     <p>
-    <label for="id_hostname">Hostname
-      <span class="form-optional">(optional)</span>
-    </label>
-    <input type="text" maxlength="30" name="hostname" id="id_hostname" />
+      <label for="id_hostname">Hostname
+        <span class="form-optional">(optional)</span>
+      </label>
+      <input type="text" maxlength="30" name="hostname" id="id_hostname" />
+    </p>
+    <p>
+      <label for="id_architecture">Architecture</label>
+      <select id="id_architecture" name="architecture">
+        <option value="amd64">amd64</option>
+        <option value="i386">i386</option>
+      </select>
+      <input type="text" maxlength="30" name="hostname" id="id_hostname" />
+    </p>
+  </script>
+  <script type="text/x-template" id="add-node-admin">
+    <p>
+      <label for="id_power_type">Power type</label>
+      <select name="power_type" id="id_power_type">
+        <option value="" selected="selected">---------</option>
+        <option value="value1">value1</option>
+      </select>
+    </p>
+    <p class="power_parameters">
+      <label for="id_power_parameters">Power parameters</label>
     </p>
   </script>
   <div id="target_node"></div>

=== modified file 'src/maasserver/static/js/tests/test_node_add.js'
--- src/maasserver/static/js/tests/test_node_add.js	2012-04-02 05:21:40 +0000
+++ src/maasserver/static/js/tests/test_node_add.js	2012-06-11 13:08:19 +0000
@@ -211,6 +211,70 @@
 
 }));
 
+suite.add(new Y.maas.testing.TestCase({
+    name: 'test-add-node-widget-add-node-admin',
+
+    setUp: function() {
+        this.setUpAdminTemplate();
+        this.setUpPowerTypeEnum();
+    },
+
+    setUpPowerTypeEnum: function() {
+        // Override module.POWER_TYPE_ENUM for the sake of testing.
+        this.POWER_TYPE_ENUM = module.POWER_TYPE_ENUM;
+        module.POWER_TYPE_ENUM = ENUM;
+    },
+
+    setUpAdminTemplate: function() {
+        // Append the snippet that will be seen by an admin user to the
+        // general template.
+        this.add_node_template = Y.one('#add-node').getContent();
+        this.add_node_admin_snippet = Y.one('#add-node-admin').getContent();
+        Y.one('#add-node').set(
+            'innerHTML',
+            this.add_node_template + this.add_node_admin_snippet);
+    },
+
+    tearDown: function() {
+        this.tearDownAdminTemplate();
+        this.tearDownPowerTypeEnum();
+    },
+
+    tearDownAdminTemplate: function() {
+        Y.one('#add-node').set('innerHTML', this.add_node_template);
+    },
+
+    tearDownPowerTypeEnum: function() {
+        module.POWER_TYPE_ENUM = this.POWER_TYPE_ENUM;
+    },
+
+    testFormContainsPowerType: function() {
+        module.showAddNodeWidget({targetNode: '#target_node', animate: false});
+        Y.Assert.isNotNull(find_widget().one('#id_power_type'));
+    },
+
+    testPowerParameterHidden: function() {
+        // The power_parameter field starts hidden.
+        module.showAddNodeWidget({targetNode: '#target_node', animate: false});
+        Y.Assert.isTrue(
+            find_widget().one('.power_parameters').hasClass('hidden'));
+    },
+
+    testPowerParameterDynamicallyLinked: function() {
+        // When the option of the 'select' tag changes to a non empty
+        // string, the 'power_parameters' field is shown.
+        module.showAddNodeWidget({targetNode: '#target_node', animate: false});
+        var select = Y.one('#id_power_type');
+        select.set('value', 'value1');
+        select.simulate('change');
+        Y.Assert.isFalse(
+            find_widget().one('.power_parameters').hasClass('hidden'));
+    }
+
+
+}));
+
+
 namespace.suite = suite;
 
 }, '0.1', {'requires': [

=== modified file 'src/maasserver/templates/maasserver/js-conf.html'
--- src/maasserver/templates/maasserver/js-conf.html	2012-03-16 09:02:13 +0000
+++ src/maasserver/templates/maasserver/js-conf.html	2012-06-11 13:08:19 +0000
@@ -30,3 +30,5 @@
 <script type="text/javascript" src="{{ STATIC_URL }}js/utils.js"></script>
 <script type="text/javascript" src="{{ STATIC_URL }}js/node_views.js"></script>
 <script type="text/javascript" src="{{ STATIC_URL }}js/longpoll.js"></script>
+<script type="text/javascript" src="{{ STATIC_URL }}js/enums.js"></script>
+<script type="text/javascript" src="{{ STATIC_URL }}js/power_parameters.js"></script>

=== modified file 'src/maasserver/templates/maasserver/node_edit.html'
--- src/maasserver/templates/maasserver/node_edit.html	2012-06-08 10:05:57 +0000
+++ src/maasserver/templates/maasserver/node_edit.html	2012-06-11 13:08:19 +0000
@@ -8,8 +8,6 @@
 {% endblock %}
 
 {% block head %}
-  <script type="text/javascript" src="{{ STATIC_URL }}js/power_parameters.js"></script>
-  <script type="text/javascript" src="{{ STATIC_URL }}js/enums.js"></script>
   <script type="text/javascript">
   <!--
   YUI().use(

=== modified file 'src/maasserver/templates/maasserver/snippets.html'
--- src/maasserver/templates/maasserver/snippets.html	2012-06-06 16:01:55 +0000
+++ src/maasserver/templates/maasserver/snippets.html	2012-06-11 13:08:19 +0000
@@ -6,12 +6,6 @@
     <div class="field-help">e.g AA:BB:CC:DD:EE:FF</div>
   </p>
 </script>
-<script type="text/x-template" id="add-architecture">
-  <p>
-    <label for="id_architecture">Architecture</label>
-    {{ node_form.architecture }}
-  </p>
-</script>
 <script type="text/x-template" id="add-node">
   <p>
     <label for="id_hostname">Hostname
@@ -25,6 +19,20 @@
     <label for="id_after_commissioning_action">After commissioning</label>
     {{ node_form.after_commissioning_action }}
   </p>
+  <p>
+    <label for="id_architecture">Architecture</label>
+    {{ node_form.architecture }}
+  </p>
+  {% if node_form.power_type %}
+    <p>
+      <label for="id_power_type">Power type</label>
+      {{ node_form.power_type }}
+    </p>
+    <p class="power_parameters">
+      <label for="id_power_parameters">Power parameters</label>
+      {{ node_form.power_parameters }}
+    </p>
+  {% endif %}
 </script>
 {% for power_type, fieldset in POWER_TYPE_PARAMETERS_FIELDS %}
   <script type="text/x-template" id="power-param-form-{{ power_type}}">

=== modified file 'src/maasserver/tests/test_config_forms.py'
--- src/maasserver/tests/test_config_forms.py	2012-06-07 14:34:27 +0000
+++ src/maasserver/tests/test_config_forms.py	2012-06-11 13:08:19 +0000
@@ -216,6 +216,11 @@
                 ),
             widget.render(name, values))
 
+    def test_empty_DictCharWidget_renders_as_empty_string(self):
+        widget = DictCharWidget(
+            [widgets.CheckboxInput], [], [], skip_check=True)
+        self.assertEqual('', widget.render(factory.getRandomString(), ''))
+
     def test_DictCharWidget_value_from_datadict_values_from_data(self):
         # 'value_from_datadict' extracts the values corresponding to the
         # field as a dictionary.

=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py	2012-04-30 16:26:38 +0000
+++ src/maasserver/tests/test_views.py	2012-06-11 13:08:19 +0000
@@ -31,9 +31,9 @@
     LoggedInTestCase,
     TestCase,
     )
+from maasserver.utils import map_enum
 from maasserver.views import HelpfulDeleteView
 from maasserver.views.nodes import NodeEdit
-from maasserver.utils import map_enum
 from maastesting.matchers import ContainsAll
 from provisioningserver.enum import PSERV_FAULT
 
@@ -70,24 +70,47 @@
 
 class TestSnippets(LoggedInTestCase):
 
-    def assertTemplateExistsAndContains(self, content, template_selector,
-                                        contains_selector):
-        """Assert that the provided html 'content' contains a snippet as
-        selected by 'template_selector' which in turn contains an element
-        selected by 'contains_selector'.
-        """
+    def _assertTemplateExistsAndContains(self, content, template_selector,
+                                         contains_selector, reverse=False):
         doc = fromstring(content)
         snippets = doc.cssselect(template_selector)
         # The snippet exists.
-        self.assertEqual(1, len(snippets))
+        self.assertEqual(
+            1, len(snippets),
+            "The snippet '%s' does not exist." % template_selector)
         # It contains the required element.
         selects = fromstring(snippets[0].text).cssselect(contains_selector)
-        self.assertEqual(1, len(selects))
+        if reverse:
+            self.assertEqual(
+                0, len(selects),
+                "The element '%s' does exist." % contains_selector)
+        else:
+            self.assertEqual(
+                1, len(selects),
+                "The element '%s' does not exist." % contains_selector,)
+
+    def assertTemplateExistsAndContains(self, content, template_selector,
+                                        contains_selector, reverse=False):
+        """Assert that the provided html 'content' contains a snippet as
+        selected by 'template_selector' which in turn contains an element
+        selected by 'contains_selector'.
+        """
+        self._assertTemplateExistsAndContains(
+            content, template_selector, contains_selector)
+
+    def assertTemplateExistsAndDoesNotContain(self, content, template_selector,
+                                              contains_selector):
+        """Assert that the provided html 'content' contains a snippet as
+        selected by 'template_selector' which does not contains an element
+        selected by 'contains_selector'.
+        """
+        self._assertTemplateExistsAndContains(
+            content, template_selector, contains_selector, reverse=True)
 
     def test_architecture_snippet(self):
         response = self.client.get('/')
         self.assertTemplateExistsAndContains(
-            response.content, '#add-architecture', 'select#id_architecture')
+            response.content, '#add-node', 'select#id_architecture')
 
     def test_hostname(self):
         response = self.client.get('/')
@@ -100,6 +123,19 @@
             response.content, '#add-node',
             'select#id_after_commissioning_action')
 
+    def test_power_type_does_not_exist_if_not_admin(self):
+        response = self.client.get('/')
+        self.assertTemplateExistsAndDoesNotContain(
+            response.content, '#add-node',
+            'select#id_power_type')
+
+    def test_power_type_exists_if_admin(self):
+        self.become_admin()
+        response = self.client.get('/')
+        self.assertTemplateExistsAndContains(
+            response.content, '#add-node',
+            'select#id_power_type')
+
 
 class FakeDeletableModel:
     """A fake model class, with a delete method."""