Merge lp:~rvb/maas/multifield into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Add DictCharField, a form field used to store/display a dictionary.

This branch adds a new (form) field used to expose and store a dictionary. This new field (DictCharField) will use a list of widgets to display the data. Each key/value will be associated with a widget.

I'll use that to change src/maasserver/power_parameters.py in a follow-up branch. A third branch will then update the API and the form code to use that to populate Node.power_parameters from the API.

= Notes =

* Improvement over power-param-api4 *
This is a much better version of what I've done in https://code.launchpad.net/~rvb/maas/power-param-api4/+merge/107661. Instead of putting the logic in the API code (as it was done in the branch power-param-api4, this branch creates a plain Django field that can be used in the API or in the UI in a completely normal fashion.

* DictCharField and DictCharWidget *
One might find that this is a little bit hacky. I confess I had to perform open heart surgery on forms.MultiValueField and forms.widgets.MultiWidget. Given the quality of the Django code, I had to adapt a more bigger portion of the code that was strictly required but note that: a) I tried to document what I've done very thoroughly, a) the amount of code is not that great. c) More importantly this is fairly *well encapsulated* as this creates a completely normal Django form field that can be reused in a very simple way:

MyForm(forms.Form)
    # A structured DictCharField which will create dicts like
    # {'sub_field1': 'value1', 'sub_field2': 'value2'}
    # or {'sub_field1': 'value1'} (Note that the second field is optional.
    constrained_multi_field = DictCharField(
        [
            ('sub_field1', forms.CharField(label="Sub Field 1")),
            ('sub_field2', forms.CharField(label="Sub Field 2"), required=False),
        ],
     # A completely un-constrained DictCharField.
     free_multi_field = DictCharField([], skip_check=True)

* Isolation *
Again, I want to state that this is fairly well isolated:
- this first branch only creates a completely independent module.
- the second branch simply updates the power_parameter module to use the newly created DictCharField.
https://code.launchpad.net/~rvb/maas/multifield-power_parameters/+merge/108191
- the third branch creates a new form used by the API, based on the one used in the UI to edit nodes and uses that form in the API. In this branch, the change to the API code is a one-liner and the change to forms.py is pretty minimal and fairly straightforward.
https://code.launchpad.net/~rvb/maas/multifield-api/+merge/108195

[Julian Edwards (julian-edwards)]

Thanks Raphers. This is needs-fixing because there's a few coding style violations and a test antipattern.

35 +class DictCharField(forms.MultiValueField):
36 + """A field to edit a dictionary of strings. Each entry in the
37 + dictionary correspond to a sub-field.

s/correspond/corresponds/

41 + 'skip_check' allows to store an arbitrary dictionary in the field,

s/to store/the storing of/

44 + For instance a DictCharField created with the following list:
45 + [
46 + ('field1', forms.CharField(label="Field 1"),
47 + ('field2', forms.CharField(label="Field 2"),
48 + ]
49 + Will produce dictionaries of the form:
50 + {'field1': 'subvalue1', 'field2': 'subvalue2'}

I think this example needs expanding to show how assignment works. It wasn't clear to me how you get from the first bit to the second bit, which suddenly has values.

55 + self.field_dict = OrderedDict(field_items)

Why ordered? Shouldn't that be an option?

60 + if skip_check:
61 + self.field_dict[SKIP_CHECK_NAME] = forms.BooleanField(
62 + required=False)

I think SKIP_CHECK_NAME needs to be more obscure; it's quite conceivable that someone will have a field named skip_check. Perhaps '__skip_check' ?

76 + def compress(self, data):
77 + if data:

Docstring missing.

91 + - the method is splitted into clean_global and

s/splitted/split/

92 + clean_individual_fields;

The code has 'clean_sub_fields' ...

114 + value = value if not self.skip_check else value[:-1]

How about:

if self.skip_check:
    value = value[:-1]

which is easier to read. We tend to eschew this logic shortcut constraint, it's hideously ugly and error prone.

111 + def clean_global(self, value):
112 + # Remove the value corresponding to the SKIP_CHECK_NAME boolean field
113 + # if required.

Missing docstring.

115 + if not value or isinstance(value, (list, tuple)):
116 + is_empty = (
117 + not value or
118 + not [v for v in value if v not in validators.EMPTY_VALUES])

*blink*

Firstly, our coding standards say to check explicitly for None or False, and not use "if not value".

Second, this really needs splitting up into something that's more understandable. I don't have any good suggestions because I can't tell what you're trying to do here. The code appears to be doing more than the comment suggests.

127 + raise ValidationError(self.error_messages['invalid'])

Is this "invalid" text something that Django needs or can we write a better error text here?

129 + def clean_sub_fields(self, value):
130 + clean_data = []

Missing docstring.

134 + fields = self.fields if not self.skip_check else self.fields[:-1]

    fields = self.fields
    if self.skip_check:
        fields = self.fields[:-1]

135 + for i, field in enumerate(fields):

Our coding standards say to use useful variable names. Perhaps "index" here.

163 +def get_all_prefixed_values(data, name):
164 + result = {}

Missing docstring.

185 + - DictCharWidget has the (optional) ability to skip all the validation
186 + are instead fetch all the values prefixed by 'main_widget_' in the
187 + input data.

s/are instead/and instead/ ?

194 + - 'value_from_datadict' which fetches the value of the data to be
195 + processed by thi...

review: Needs Fixing

[Julian Edwards (julian-edwards)]

Thank you LP for stripping all the formatting spaces on the code snippets >:(

[Raphaël Badin (rvb)]

Thanks a lot for the review Julian, I'll fix this when I'll be back on Tuesday. I had a quick glance at your comments and it seems that most of the weirdness that you pointed out to me are snippets of Django code that I had to embed in my own code because, like I said on the MP, the Django code is not sufficiently well written to allow me to override only the parts that I needed to override :(. eg:

** original Django code **
        if not value or isinstance(value, (list, tuple)):
            if not value or not [v for v in value if v not in validators.EMPTY_VALUES]:

** Semi-refactored Django code **
115 + if not value or isinstance(value, (list, tuple)):
116 + is_empty = (
117 + not value or
118 + not [v for v in value if v not in validators.EMPTY_VALUES])

But anyway, since this becomes our code, I'll fix this… thanks again for the thorough review.

[Julian Edwards (julian-edwards)]

On Sunday 03 June 2012 10:12:18 you wrote:
> Thanks a lot for the review Julian, I'll fix this when I'll be back on
> Tuesday. I had a quick glance at your comments and it seems that most of
> the weirdness that you pointed out to me are snippets of Django code that I
> had to embed in my own code because, like I said on the MP, the Django code
> is not sufficiently well written to allow me to override only the parts
> that I needed to override :(. eg:
>
> ** original Django code **
> if not value or isinstance(value, (list, tuple)):
> if not value or not [v for v in value if v not in
> validators.EMPTY_VALUES]:
>
> ** Semi-refactored Django code **
> 115 + if not value or isinstance(value, (list, tuple)):
> 116 + is_empty = (
> 117 + not value or
> 118 + not [v for v in value if v not in validators.EMPTY_VALUES])
>
> But anyway, since this becomes our code, I'll fix this… thanks again for the
> thorough review.

Ok, thanks. I knew there had to be a reason you'd not be writing code that
badly on purpose!

[Raphaël Badin (rvb)]

> Ok, thanks. I knew there had to be a reason you'd not be writing code that
> badly on purpose!

Yeah, I confess I was tempted to leave it like it was written originally to better identify what was "stolen" from Django and what was not… If I refactor heavily the Django code, what is our code and what is not will become less obvious.

[Raphaël Badin (rvb)]

> Thanks Raphers. This is needs-fixing because there's a few coding style
> violations and a test antipattern.

Thanks for the review. This is a first batch of fixes. Now I'm working on improving the test file.
>
> s/correspond/corresponds/
> s/to store/the storing of/

Done.

>
> 44 + For instance a DictCharField created with the following list:
> 45 + [
[...]
> 50 + {'field1': 'subvalue1', 'field2': 'subvalue2'}
>
> I think this example needs expanding to show how assignment works. It wasn't
> clear to me how you get from the first bit to the second bit, which suddenly
> has values.

Dood point, done

> 55 + self.field_dict = OrderedDict(field_items)
>
> Why ordered? Shouldn't that be an option?

No, I need self.fields and self.names to be ordered in the same way. But note that this is an implementation detail.

> 60 + if skip_check:
> 61 + self.field_dict[SKIP_CHECK_NAME] = forms.BooleanField(
> 62 + required=False)
>
> I think SKIP_CHECK_NAME needs to be more obscure; it's quite conceivable that
> someone will have a field named skip_check. Perhaps '__skip_check' ?

You've got a point, but since SKIP_CHECK_NAME is exposed to the user, I think it should be readable. Instead I've added a check to make sure that no user-defined subfield is named 'skip_check'. I've added a test for that (test_DictCharField_does_not_allow_subfield_named_skip_check).

> 76 + def compress(self, data):
> 77 + if data:
>
> Docstring missing.

Done.

> s/splitted/split/

Fixed.

> The code has 'clean_sub_fields' ...

Right… fixed.

> 114 + value = value if not self.skip_check else value[:-1]
>
> How about:
>
> if self.skip_check:
> value = value[:-1]
>
> which is easier to read. We tend to eschew this logic shortcut constraint,
> it's hideously ugly and error prone.

Done. I've also added a comment to explain why this is for.

> 111 + def clean_global(self, value):
> 112 + # Remove the value corresponding to the SKIP_CHECK_NAME boolean
> field
> 113 + # if required.
>
> Missing docstring.

Fixed.

> 115 + if not value or isinstance(value, (list, tuple)):
> 116 + is_empty = (
> 117 + not value or
> 118 + not [v for v in value if v not in validators.EMPTY_VALUES])
>
> *blink*
>
> Firstly, our coding standards say to check explicitly for None or False, and
> not use "if not value".
>
> Second, this really needs splitting up into something that's more
> understandable. I don't have any good suggestions because I can't tell what
> you're trying to do here. The code appears to be doing more than the comment
> suggests.

Done, and I've added a comment.

> 127 + raise ValidationError(self.error_messages['invalid'])
>
> Is this "invalid" text something that Django needs or can we write a better
> error text here?

This is precisely done so that you can override all the default error messages on a per-field basis:
e.g.:
name = forms.CharField(error_messages={'invalid': "Custom invalid message here"})

>
> 129 + def clean_sub_fields(self, value):
> 130 + clean_data = []
>
> Missing docstring.

Fixed.

> 134 + fields = self.field...

[Raphaël Badin (rvb)]

>
> 324 +class DictCharFieldTest(TestCase):
> 325 +
> 326 + def test_DictCharField_init(self):
> 327 + self.assertEqual(['field_a', 'field_b', 'field_c'],
> testField.names)
> 328 + self.assertEqual(
> 329 + ['field_a', 'field_b', 'field_c'], testField.widget.names)
> 330 + self.assertEqual(
> 331 + [field.widget for field in testField.field_dict.values()],
> 332 + testField.widget.widgets)
>
> This is a poor test because it's testing values that are set elsewhere in the
> test file, and is one of our testing antipatterns. Test values should be
> initialised in the same test. (i.e. the global "testField" is bad!)
>
> I'd fix this by having a factory for it so you can supply initialiser values
> in the test.

Fixed. I've not created a factory because I thought it wouldn't give us much considering that all the initialization code is in the creation of the list of the subfields. But now each test is isolated and much more readable.

[Julian Edwards (julian-edwards)]

 status approve

On Tuesday 05 Jun 2012 09:06:22 Raphaël Badin wrote:
> > I think SKIP_CHECK_NAME needs to be more obscure; it's quite conceivable
> > that someone will have a field named skip_check. Perhaps '__skip_check'
> > ?
> You've got a point, but since SKIP_CHECK_NAME is exposed to the user, I
> think it should be readable. Instead I've added a check to make sure that
> no user-defined subfield is named 'skip_check'. I've added a test for that
> (test_DictCharField_does_not_allow_subfield_named_skip_check).

This is good that you've checked its presence - however I'd make the error
message convey more information like:

"skip_check is a reserved name"

so the user knows why it's disallowed.

+ """Extract the values for each widget from a data dict (QueryDict).
+ :param data: The data dict (usually request.data or request.GET where
+ request is a django.http.HttpRequest).
+ :param data: dict
+ :param files: The files dict (usually request.FILES where request is
a
+ django.http.HttpRequest).
+ :param files: dict
+ :param name: The name of the widget.
+ :param name: basestring

Some of these s/param/type/

Everything else is much better! I particularly like the doctest-like nature
of some of the docstrings, thank you.

Approved with the change above!

review: Approve

[Raphaël Badin (rvb)]

> This is good that you've checked its presence - however I'd make the error
> message convey more information like:
> "skip_check is a reserved name"
> so the user knows why it's disallowed.

Done.

> Some of these s/param/type/

Done.

Thanks!

=== added file 'src/maasserver/config_forms.py'
--- src/maasserver/config_forms.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/config_forms.py	2012-06-05 10:17:20 +0000
@@ -0,0 +1,332 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Config forms utilities."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'DictCharField',
+    'DictCharWidget',
+    ]
+
+from collections import OrderedDict
+
+from django import forms
+from django.core import validators
+from django.core.exceptions import ValidationError
+from django.forms.fields import Field
+from django.forms.util import ErrorList
+from django.utils.safestring import mark_safe
+
+
+SKIP_CHECK_NAME = 'skip_check'
+
+
+class DictCharField(forms.MultiValueField):
+    """A field to edit a dictionary of strings.  Each entry in the
+    dictionary corresponds to a sub-field.
+
+    The field is constructed with a list of tuples containing the name of the
+    sub-fields and the sub-field themselves.  An optional parameter
+    'skip_check' allows the storing of an arbitrary dictionary in the field,
+    bypassing any validation made by the sub-fields.
+
+    For instance, if we create a form with a single DictCharField:
+
+    >>> from django import forms
+    >>> class ExampleForm(forms.Form)
+            example = DictCharField(
+                [
+                    ('field1', forms.CharField(label="Field 1"),
+                    ('field2', forms.CharField(label="Field 2"),
+                ])
+    >>> data = QueryDict('example_field1=subvalue1&example_field2=subvalue2')
+    >>> form = ExampleForm(data)
+    >>> # The 'cleaned_data' of the 'example' field is populated with the
+    >>> # values of the subfields.
+    >>> form.cleaned_data['example']
+    {'field1': 'subvalue1', 'field2': 'subvalue2'}
+    """
+
+    def __init__(self, field_items, skip_check=False, *args,
+                 **kwargs):
+        self.field_dict = OrderedDict(field_items)
+        self.skip_check = skip_check
+        # Make sure no subfield is named 'SKIP_CHECK_NAME'. If
+        # skip_check is True this field will clash with the addtional
+        # subfield added by the DictCharField constructor.  We perform
+        # this check even if skip_check=False because having a field named
+        # 'skip_check' that isn't used to actually skip the checks would be
+        # very confusing.
+        if SKIP_CHECK_NAME in self.field_dict.keys():
+            raise RuntimeError("No subfield can be named 'skip_check'.")
+        # if skip_check: add a BooleanField to the list of fields, this will
+        # be used to skip the validation of the fields and accept arbitrary
+        # data.
+        if skip_check:
+            self.field_dict[SKIP_CHECK_NAME] = forms.BooleanField(
+                required=False)
+        self.names = [name for name in self.field_dict.keys()]
+        # Create the DictCharWidget with init values from the list of fields.
+        self.fields = self.field_dict.values()
+        self.widget = DictCharWidget(
+            [field.widget for field in self.fields],
+            self.names,
+            [field.label for field in self.fields],
+            skip_check=skip_check,
+            )
+        # Upcall to Field and not MultiValueField to avoid setting all the
+        # subfields' 'required' attributes to False.
+        Field.__init__(self, *args, **kwargs)
+
+    def compress(self, data):
+        """Returns a single value for the given list of values."""
+        if data:
+            if isinstance(data, dict):
+                # If the data is a dict, this means that we're in the
+                # situation where skip_check was true and we simply
+                # return the dict.
+                return data
+            else:
+                # Here data is the list of the values of the subfields,
+                # return a dict with all the right keys:
+                # For instance, for a DictCharField created with two
+                # subfields 'field1' and 'field2', data will be
+                # ['value1', 'value2'] and we will return:
+                # {'field1': 'value1', 'field2': 'value2'}
+                return dict(zip(self.names, data))
+        return None
+
+    def clean(self, value):
+        """Validates every value in the given list. A value is validated
+        against the corresponding Field in self.fields.
+
+        This is an adapted version of Django's MultiValueField_ clean method.
+
+        The differences are:
+        - the method is split into clean_global and
+             clean_sub_fields;
+        - the field and value corresponding to the SKIP_CHECK_NAME boolean
+            field are removed;
+        - each individual field 'required' attribute is used instead of the
+            DictCharField's 'required' attribute.  This allows a more
+            fine-grained control of what's required and what's not required.
+
+        .. _MultiValueField: http://code.djangoproject.com/
+            svn/django/tags/releases/1.3.1/django/forms/fields.py
+        """
+        if isinstance(value, dict):
+            return value
+        else:
+            result = self.clean_global(value)
+            if result is None:
+                return None
+            else:
+                return self.clean_sub_fields(value)
+
+    def clean_global(self, value):
+        """Make sure the value is not empty and is thus suitable to be
+        feed to the sub fields' validators."""
+        # Remove the value corresponding to the SKIP_CHECK_NAME boolean field
+        # if required.
+        if self.skip_check:
+            # If self.skip_check: strip out the last value which
+            # corresponds to the value of the 'skip_check' boolean field.
+            value = value[:-1]
+        if not value or isinstance(value, (list, tuple)):
+            # value is considered empty if it is in
+            # validators.EMPTY_VALUES, or if each of the subvalues is in
+            # validators.EMPTY_VALUES.
+            is_empty = (
+                value in validators.EMPTY_VALUES or
+                len(filter(
+                    lambda x: x not in validators.EMPTY_VALUES, value)) == 0)
+            if is_empty:
+                if self.required:
+                    raise ValidationError(self.error_messages['required'])
+                else:
+                    return None
+            else:
+                return True
+        else:
+            raise ValidationError(self.error_messages['invalid'])
+
+    def clean_sub_fields(self, value):
+        """'value' being the list of the values of the subfields, validate
+        each subfield."""
+        clean_data = []
+        errors = ErrorList()
+        # Remove the field corresponding to the SKIP_CHECK_NAME boolean field
+        # if required.
+        fields = self.fields if not self.skip_check else self.fields[:-1]
+        for index, field in enumerate(fields):
+            try:
+                field_value = value[index]
+            except IndexError:
+                field_value = None
+            # Check the field's 'required' field instead of the global
+            # 'required' field to allow subfields to be required or not.
+            if field.required and field_value in validators.EMPTY_VALUES:
+                errors.append(
+                    '%s: %s' % (field.label, self.error_messages['required']))
+                continue
+            try:
+                clean_data.append(field.clean(field_value))
+            except ValidationError, e:
+                # Collect all validation errors in a single list, which we'll
+                # raise at the end of clean(), rather than raising a single
+                # exception for the first error we encounter.
+                errors.extend(
+                    ['%s: %s' % (field.label, message)
+                    for message in e.messages])
+        if errors:
+            raise ValidationError(errors)
+
+        out = self.compress(clean_data)
+        self.validate(out)
+        return out
+
+
+def get_all_prefixed_values(data, name):
+    """From a dictionary, extract a sub-dictionary of all the keys/values for
+    which the key starts with a particular prefix.  In the resulting
+    dictionary, strip the prefix from the keys.
+
+    >>> get_all_prefixed_values(
+        {'prefix_test': 'a', 'key': 'b'}, 'prefix_')
+    {'test': 'a'}
+    """
+    result = {}
+    for key, value in data.items():
+        if key.startswith(name):
+            new_key = key[len(name):]
+            if new_key != SKIP_CHECK_NAME:
+                result[new_key] = value
+    return result
+
+
+class DictCharWidget(forms.widgets.MultiWidget):
+    """A widget to display the content of a dictionary.  Each key will
+    correspond to a subwidget.  Although there is no harm in using this class
+    directly, note that this is mostly destined to be used internally
+    by DictCharField.
+
+    The customization compared to Django's MultiWidget_ are:
+    - DictCharWidget displays all the subwidgets inside a fieldset tag;
+    - DictCharWidget displays a label for each subwidget;
+    - DictCharWidget names each subwidget 'main_widget_sub_widget_name'
+        instead of 'main_widget_0';
+    - DictCharWidget has the (optional) ability to skip all the validation
+        and instead fetch all the values prefixed by 'main_widget_' in the
+        input data.
+
+    To achieve that, we customize:
+    - 'render' which returns the HTML code to display this widget;
+    - 'id_for_label' which return the HTML ID attribute for this widget
+        for use by a label.  This widget is composed of multiple widgets so
+        the id of the first widget is used;
+    - 'value_from_datadict' which fetches the value of the data to be
+        processed by this form to give a 'data' dictionary.  We need to
+        customize that because we've changed the way MultiWidget names
+        sub-widgets;
+    - 'decompress' which takes a single "compressed" value and returns a list
+        of values to be used by the widgets.
+
+    .. _MultiWidget: http://code.djangoproject.com/
+        svn/django/tags/releases/1.3.1/django/forms/widgets.py
+    """
+
+    def __init__(self, widgets, names, labels, skip_check=False, attrs=None):
+        self.names = names
+        self.labels = labels
+        self.skip_check = skip_check
+        super(DictCharWidget, self).__init__(widgets, attrs)
+
+    def render(self, name, value, attrs=None):
+        # value is a list of values, each corresponding to a widget
+        # in self.widgets.
+        if not isinstance(value, list):
+            value = self.decompress(value)
+        output = ['<fieldset>']
+        final_attrs = self.build_attrs(attrs)
+        id_ = final_attrs.get('id', None)
+        for index, widget in enumerate(self.widgets):
+            try:
+                widget_value = value[index]
+            except IndexError:
+                widget_value = None
+            if id_:
+                final_attrs = dict(
+                    final_attrs, id='%s_%s' % (id_, self.names[index]))
+            # Add label to each sub-field.
+            if id_:
+                label_for = ' for="%s"' % final_attrs['id']
+            else:
+                label_for = ''
+            output.append(
+                '<label%s>%s</label>' % (
+                    label_for, self.labels[index]))
+            output.append(
+                widget.render(
+                    '%s_%s' % (name, self.names[index]), widget_value,
+                    final_attrs))
+        output.append('</fieldset>')
+        return mark_safe(self.format_output(output))
+
+    def id_for_label(self, id_):
+        """Returns the HTML ID attribute of this Widget.  Since this is a
+        widget with multiple HTML elements, this method returns an ID
+        corresponding to the first ID in the widget's tags."""
+        # See the comment for RadioSelect.id_for_label()
+        if id_:
+            id_ += '_%s' % self.names[0]
+        return id_
+
+    def value_from_datadict(self, data, files, name):
+        """Extract the values for each widget from a data dict (QueryDict).
+        :param data: The data dict (usually request.data or request.GET where
+            request is a django.http.HttpRequest).
+        :param data: dict
+        :param files: The files dict (usually request.FILES where request is a
+            django.http.HttpRequest).
+        :param files: dict
+        :param name: The name of the widget.
+        :param name: basestring
+        :return: The extracted values.  If skip_check has been activated, the
+            extracted value will be a dictionary, if not, is will be a list.
+        :rtype: dict or list
+        """
+        # self.widgets[-1] is the 'skip_check' boolean widget.  If
+        # self.skip_check is true and the value (in data) corresponding
+        # to the 'skip_check' widget is true, extract all the values
+        # from data which correspond to this widget and return a
+        # dictionary of these keys/values.
+        skip_check = (
+            self.skip_check and
+            self.widgets[-1].value_from_datadict(
+                data, files, name + '_%s' % self.names[-1]))
+        if skip_check:
+            # If the skip_check option is on and the value of the boolean
+            # field is true: simply return all the values from 'data' which
+            # are prefixed by the name of this widget.
+            return get_all_prefixed_values(data, name + '_')
+        else:
+            return [
+                widget.value_from_datadict(
+                    data, files, name + '_%s' % self.names[i])
+                for i, widget in enumerate(self.widgets)]
+
+    def decompress(self, value):
+        """Returns a list of decompressed values for the given compressed
+        value.  The given value can be assumed to be valid, but not
+        necessarily non-empty."""
+        if value is not None:
+            return [value.get(name, None) for name in self.names]
+        else:
+            return [None] * len(self.names)

=== added file 'src/maasserver/tests/test_config_forms.py'
--- src/maasserver/tests/test_config_forms.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_config_forms.py	2012-06-05 10:17:20 +0000
@@ -0,0 +1,240 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test config forms utilities."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from django import forms
+from django.forms import widgets
+from django.http import QueryDict
+from maasserver.config_forms import (
+    DictCharField,
+    DictCharWidget,
+    get_all_prefixed_values,
+    )
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase
+
+
+class TestDictCharField(TestCase):
+
+    def test_DictCharField_init(self):
+        testField = DictCharField(
+            [
+                ('field_a', forms.CharField(label='Field a')),
+                ('field_b', forms.CharField(label='Field b')),
+                ('field_c', forms.CharField(label='Field c')),
+            ])
+        self.assertEqual(['field_a', 'field_b', 'field_c'], testField.names)
+        self.assertEqual(
+            ['field_a', 'field_b', 'field_c'], testField.widget.names)
+        self.assertEqual(
+            [field.widget for field in testField.field_dict.values()],
+            testField.widget.widgets)
+
+    def test_DictCharField_does_not_allow_subfield_named_skip_check(self):
+        # Creating a DictCharField with a subfield named 'skip_check' is not
+        # allowed.
+        self.assertRaises(
+            RuntimeError, DictCharField,
+            [('skip_check', forms.CharField(label='Skip Check'))])
+
+
+class TestFormWithDictCharField(TestCase):
+
+    def test_DictCharField_processes_QueryDict_into_a_dict(self):
+        class FakeForm(forms.Form):
+            multi_field = DictCharField(
+                [
+                    ('field_a', forms.CharField(label='Field a')),
+                    ('field_b', forms.CharField(
+                        label='Field b', required=False, max_length=3)),
+                    ('field_c', forms.CharField(
+                        label='Field c', required=False)),
+                ])
+
+        fielda_value = factory.getRandomString()
+        fieldc_value = factory.getRandomString()
+        data = QueryDict(
+            'multi_field_field_a=%s&multi_field_field_c=%s' % (
+                fielda_value, fieldc_value))
+
+        form = FakeForm(data)
+
+        self.assertTrue(form.is_valid())
+        self.assertEqual(
+            {
+                'field_a': fielda_value,
+                'field_b': '',
+                'field_c': fieldc_value,
+            },
+            form.cleaned_data['multi_field'])
+
+    def test_DictCharField_honors_field_constraint(self):
+        class FakeForm(forms.Form):
+            multi_field = DictCharField(
+                [
+                    ('field_a', forms.CharField(label='Field a')),
+                    ('field_b', forms.CharField(
+                        label='Field b', required=False, max_length=3)),
+                ])
+
+        # Create a value that will fail validation because it's too long.
+        fielda_value = factory.getRandomString(10)
+        data = QueryDict('multi_field_field_b=%s' % fielda_value)
+        form = FakeForm(data)
+
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            {'multi_field': [
+                'Field a: This field is required.',
+                'Field b: Ensure this value has at '
+                'most 3 characters (it has 10).']},
+            form.errors)
+
+    def test_DictCharField_skip_check_true_skips_validation(self):
+        # Create a value that will fail validation because it's too long.
+        field_name = factory.getRandomString(10)
+        field_value = factory.getRandomString(10)
+        # multi_field_skip_check=true will make the form accept the value
+        # even if it's not valid.
+        data = QueryDict(
+            'multi_field_%s=%s&multi_field_skip_check=true' % (
+                field_name, field_value))
+
+        class FakeFormSkip(forms.Form):
+            multi_field = DictCharField(
+                [(field_name, forms.CharField(label='Unused', max_length=3))],
+            skip_check=True)
+        form = FakeFormSkip(data)
+
+        self.assertTrue(form.is_valid())
+        self.assertEqual(
+            {field_name: field_value},
+            form.cleaned_data['multi_field'])
+
+    def test_DictCharField_skip_check_false(self):
+        # Create a value that will fail validation because it's too long.
+        field_value = factory.getRandomString(10)
+        field_name = factory.getRandomString(10)
+        field_label = factory.getRandomString(10)
+        # Force the check with multi_field_skip_check=false.
+        data = QueryDict(
+            'multi_field_%s=%s&multi_field_skip_check=false' % (
+                field_name, field_value))
+
+        class FakeFormSkip(forms.Form):
+            multi_field = DictCharField(
+                [(field_name, forms.CharField(
+                    label=field_label, max_length=3))],
+            skip_check=True)
+        form = FakeFormSkip(data)
+
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            {
+                'multi_field': [
+                    "%s: Ensure this value has at most 3 characters "
+                    "(it has 10)." % field_label]
+            },
+            form.errors)
+
+    def test_DictCharField_accepts_required_false(self):
+        # A form where the DictCharField instance is constructed with
+        # required=False.
+        class FakeFormRequiredFalse(forms.Form):
+            multi_field = DictCharField(
+                [('field_a', forms.CharField(label='Field a'))],
+                required=False)
+            char_field = forms.CharField(label='Field a')
+
+        char_value = factory.getRandomString(10)
+        data = QueryDict('char_field=%s' % (char_value))
+        form = FakeFormRequiredFalse(data)
+        self.assertTrue(form.is_valid())
+        self.assertEqual(
+            {'char_field': char_value, 'multi_field': None},
+            form.cleaned_data)
+
+
+class TestUtilities(TestCase):
+
+    def test_get_all_prefixed_values_returns_sub_dict(self):
+        inputs = [
+            {'prefix_test': 'a', 'key': 'b', 'prefix_2': 'c'},
+            {},
+            {'b': factory.getRandomString()},
+            ]
+        prefix = 'prefix_'
+        expected = [
+            {'test': 'a', '2': 'c'},
+            {},
+            {},
+            ]
+        self.assertEqual(
+            expected,
+            map(lambda data: get_all_prefixed_values(data, prefix), inputs))
+
+
+class TestDictCharWidget(TestCase):
+
+    def test_DictCharWidget_id_for_label_uses_first_fields_name(self):
+        names = [factory.getRandomString()]
+        labels = [factory.getRandomString()]
+        widget = DictCharWidget(
+            [widgets.TextInput, widgets.TextInput], names, labels)
+        self.assertEqual(
+            ' _%s' % names[0],
+            widget.id_for_label(' '))
+
+    def test_DictCharWidget_renders_fieldset_with_label_and_field_names(self):
+        names = [factory.getRandomString(), factory.getRandomString()]
+        labels = [factory.getRandomString(), factory.getRandomString()]
+        values = [factory.getRandomString(), factory.getRandomString()]
+        widget = DictCharWidget(
+            [widgets.TextInput, widgets.TextInput], names, labels)
+        name = factory.getRandomString()
+        self.assertEqual(
+            '<fieldset>'
+            '<label>%s</label>'
+            '<input type="text" name="%s" value="%s" />'
+            '<label>%s</label>'
+            '<input type="text" name="%s" value="%s" />'
+            '</fieldset>' %
+                (
+                    labels[0],
+                     '%s_%s' % (name, names[0]), values[0],
+                     labels[1],
+                     '%s_%s' % (name, names[1]), values[1],
+                ),
+            widget.render(name, values))
+
+    def test_DictCharWidget_value_from_datadict_values_from_data(self):
+        # 'value_from_datadict' extracts the value of the fields from a
+        # QueryDict and returns them in the sub widgets' order.
+        names = [factory.getRandomString(), factory.getRandomString()]
+        labels = [factory.getRandomString(), factory.getRandomString()]
+        name = factory.getRandomString()
+        field_1_value = factory.getRandomString()
+        field_2_value = factory.getRandomString()
+        # Create a query string with the field2 before the field1 and another
+        # (unknown) value.
+        data = QueryDict(
+            '%s_%s=%s&%s_%s=%s&%s=%s' % (
+                name, names[1], field_2_value,
+                name, names[0], field_1_value,
+                factory.getRandomString(), factory.getRandomString())
+            )
+        widget = DictCharWidget(
+            [widgets.TextInput, widgets.TextInput], names, labels)
+        self.assertEqual(
+            [field_1_value, field_2_value],
+            widget.value_from_datadict(data, None, name))