launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/multifield-power_parameters into lp:maas with lp:~rvb/maas/multifield as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/multifield-power_parameters/+merge/108191

This branch simply updates src/maasserver/power_parameter.py to use DictCharField instead of a ad hoc structure.  Note that this simplifies the code and the tests because all the validation will be done using DictCharField's validation features (which in turns leverages the stock validation functionality present in Django).
-- 
https://code.launchpad.net/~rvb/maas/multifield-power_parameters/+merge/108191
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/multifield-power_parameters into lp:maas.

=== modified file 'src/maasserver/power_parameters.py'
--- src/maasserver/power_parameters.py	2012-05-25 16:13:50 +0000
+++ src/maasserver/power_parameters.py	2012-05-31 15:55:32 +0000
@@ -14,7 +14,7 @@
 power type with a set of power parameters.
 
 To define a new set of power parameters for a new power_type: create a new
-mapping between the new power type and a list of PowerParameter instances in
+mapping between the new power type and a DictCharField instance in
 `POWER_TYPE_PARAMETERS`.
 """
 
@@ -27,108 +27,48 @@
 __metaclass__ = type
 __all__ = [
     'POWER_TYPE_PARAMETERS',
-    'validate_power_parameters',
     ]
 
-from collections import namedtuple
-from operator import attrgetter
 
-from django.core.exceptions import ValidationError
+from django import forms
+from maasserver.config_forms import DictCharField
 from provisioningserver.enum import POWER_TYPE
 
 
-PowerParameter = namedtuple(
-    'PowerParameter',
-    [
-        # 'display' will be used in the UI as the title of the field.
-        'display',
-        # 'name' is the actual name of this parameter as used in the JSON
-        # structure (power parameters are stored as JSON dicts).
-        'name',
-    ])
-
-
 POWER_TYPE_PARAMETERS = {
     POWER_TYPE.WAKE_ON_LAN:
-        [
-            PowerParameter(
-                display='Address',
-                name='power_address',
-                ),
-        ],
+        DictCharField(
+            [
+                ('power_address', forms.CharField(label="Address")),
+            ],
+            required=False,
+            skip_check=True),
     POWER_TYPE.VIRSH:
-        [
-            PowerParameter(
-                display='Driver',
-                name='driver',
-                ),
-             PowerParameter(
-                display='Username',
-                name='username',
-                ),
-             PowerParameter(
-                display='Address',
-                name='power_address',
-                ),
-             PowerParameter(
-                display='power_id',
-                name='power_id',
-                ),
-         ],
+        DictCharField(
+            [
+                ('driver', forms.CharField(label="Driver")),
+                ('username', forms.CharField(label="Username")),
+                ('power_address', forms.CharField(label="Address")),
+                ('power_id', forms.CharField(label="Power ID")),
+            ],
+            required=False,
+            skip_check=True),
     POWER_TYPE.IPMI:
-        [
-            PowerParameter(
-                display='Address',
-                name='power_address',
-                ),
-            PowerParameter(
-                display='User',
-                name='power_user',
-                ),
-            PowerParameter(
-                display='Password',
-                name='power_pass',
-                ),
-         ],
+        DictCharField(
+            [
+                ('power_address', forms.CharField(label="Address")),
+                ('power_user', forms.CharField(label="User")),
+                ('power_pass', forms.CharField(label="Password")),
+            ],
+            required=False,
+            skip_check=True),
     POWER_TYPE.IPMI_LAN:
-        [
-            PowerParameter(
-                display='User',
-                name='power_user',
-                ),
-            PowerParameter(
-                display='Password',
-                name='power_pass',
-                ),
-            PowerParameter(
-                display='power_id',
-                name='power_id',
-                ),
-        ]
+        DictCharField(
+            [
+                ('power_user', forms.CharField(label="User")),
+                ('power_pass', forms.CharField(label="Password")),
+                ('power_id', forms.CharField(label="Power ID")),
+            ],
+            required=False,
+            skip_check=True),
     }
-
-
-def validate_power_parameters(power_parameters, power_type):
-    """Validate that the given power parameters:
-    - the given power_parameter argument must be a dictionary.
-    - the keys of the given power_parameter argument must be a subset of
-      the possible parameters for this power type.
-    If one of these assertions is not true, raise a ValidationError.
-    """
-    if not isinstance(power_parameters, dict):
-        raise ValidationError(
-            "The given power parameters should be a dictionary.")
-    # Fetch the expected power_parameter related to the power_type.  If the
-    # power_type is unknown, don't validate power_parameter.  We don't want
-    # to block things if one wants to use a custom power_type.
-    expected_power_parameters = map(attrgetter(
-        'name'), POWER_TYPE_PARAMETERS.get(power_type, []))
-    if len(expected_power_parameters) != 0:
-        unknown_fields = set(
-            power_parameters).difference(expected_power_parameters)
-        if len(unknown_fields) != 0:
-            raise ValidationError(
-                    "These field(s) are invalid for this power type: %s.  "
-                    "Allowed fields: %s." % (
-                        ', '.join(unknown_fields),
-                        ', '.join(expected_power_parameters)))

=== modified file 'src/maasserver/tests/test_power_parameters.py'
--- src/maasserver/tests/test_power_parameters.py	2012-05-25 16:13:50 +0000
+++ src/maasserver/tests/test_power_parameters.py	2012-05-31 15:55:32 +0000
@@ -12,22 +12,17 @@
 __metaclass__ = type
 __all__ = []
 
-from operator import attrgetter
-
-from django.core.exceptions import ValidationError
-from maasserver.power_parameters import (
-    POWER_TYPE_PARAMETERS,
-    PowerParameter,
-    validate_power_parameters,
-    )
-from maasserver.testing.factory import factory
+from maasserver.config_forms import DictCharField
+from maasserver.power_parameters import POWER_TYPE_PARAMETERS
 from maasserver.testing.testcase import TestCase
 from maasserver.utils import map_enum
 from maastesting.matchers import ContainsAll
 from provisioningserver.enum import POWER_TYPE
 from testtools.matchers import (
     AllMatch,
+    Equals,
     IsInstance,
+    MatchesStructure,
     )
 
 
@@ -38,48 +33,12 @@
         self.assertIsInstance(POWER_TYPE_PARAMETERS, dict)
         self.assertThat(power_types, ContainsAll(POWER_TYPE_PARAMETERS))
 
-    def test_POWER_TYPE_PARAMETERS_values_are_PowerParameter(self):
-        params = sum(POWER_TYPE_PARAMETERS.values(), [])
-        self.assertThat(params, AllMatch(IsInstance(PowerParameter)))
-
-
-class TestPowerParameterHelpers(TestCase):
-
-    def test_validate_power_parameters_requires_dict(self):
-        exception = self.assertRaises(
-            ValidationError, validate_power_parameters,
-            factory.getRandomString(), factory.getRandomString())
-        self.assertEqual(
-            ["The given power parameters should be a dictionary."],
-            exception.messages)
-
-    def test_validate_power_parameters_rejects_unknown_field(self):
-        # If power_type is a known power type, the fields in the provided
-        # power_parameters dict are checked.
-        power_parameters = {"invalid-power-type": factory.getRandomString()}
-        power_type = POWER_TYPE.WAKE_ON_LAN
-        expected_power_parameters = map(attrgetter(
-            'name'), POWER_TYPE_PARAMETERS.get(power_type, []))
-        exception = self.assertRaises(
-            ValidationError, validate_power_parameters,
-            power_parameters, power_type)
-        expected_message = (
-            "These field(s) are invalid for this power type: "
-            "invalid-power-type.  Allowed fields: %s." % ', '.join(
-                expected_power_parameters))
-        self.assertEqual([expected_message], exception.messages)
-
-    def test_validate_power_parameters_validates_if_unknown_power_type(self):
-        # If power_type is not a known power type, no check of the fields in
-        # power_parameter is performed.
-        power_parameters = {
-            factory.getRandomString(): factory.getRandomString()}
-        power_type = factory.getRandomString()
-        self.assertIsNone(
-            validate_power_parameters(power_parameters, power_type))
-
-    def test_validate_power_parameters_validates_with_power_type_info(self):
-        power_parameters = {'power_address': factory.getRandomString()}
-        power_type = POWER_TYPE.WAKE_ON_LAN
-        self.assertIsNone(
-            validate_power_parameters(power_parameters, power_type))
+    def test_POWER_TYPE_PARAMETERS_values_are_DictCharField(self):
+        self.assertThat(
+            POWER_TYPE_PARAMETERS.values(),
+            AllMatch(IsInstance(DictCharField)))
+
+    def test_POWER_TYPE_PARAMETERS_DictCharField_objects_have_skip_check(self):
+        self.assertThat(
+            POWER_TYPE_PARAMETERS.values(),
+            AllMatch(MatchesStructure(skip_check=Equals(True))))