launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08422
[Merge] lp:~rvb/maas/multifield into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/multifield into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/multifield/+merge/108188
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
--
https://code.launchpad.net/~rvb/maas/multifield/+merge/108188
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/multifield into lp:maas.
=== 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-05-31 15:52:42 +0000
@@ -0,0 +1,261 @@
+# 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 correspond 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 to store an arbitrary dictionary in the field,
+ bypassing any validation made by the sub-fields.
+
+ For instance a DictCharField created with the following list:
+ [
+ ('field1', forms.CharField(label="Field 1"),
+ ('field2', forms.CharField(label="Field 2"),
+ ]
+ Will produce dictionaries of the form:
+ {'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
+ # 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):
+ if data:
+ if isinstance(data, dict):
+ return data
+ else:
+ 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 splitted into clean_global and
+ clean_individual_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):
+ # Remove the value corresponding to the SKIP_CHECK_NAME boolean field
+ # if required.
+ value = value if not self.skip_check else value[:-1]
+ if not value or isinstance(value, (list, tuple)):
+ is_empty = (
+ not value or
+ not [v for v in value if v not in validators.EMPTY_VALUES])
+ 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):
+ 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 i, field in enumerate(fields):
+ try:
+ field_value = value[i]
+ 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):
+ result = {}
+ prefix = name + '_'
+ for key, value in data.items():
+ if key.startswith(prefix):
+ new_key = key[len(prefix):]
+ 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
+ are 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 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 i, widget in enumerate(self.widgets):
+ try:
+ widget_value = value[i]
+ except IndexError:
+ widget_value = None
+ if id_:
+ final_attrs = dict(
+ final_attrs, id='%s_%s' % (id_, self.names[i]))
+ # Add label to each sub-field.
+ label_for = ' for="%s"' % final_attrs['id'] if id_ else ''
+ output.append(
+ '<label%s>%s</label>' % (
+ label_for, self.labels[i]))
+ output.append(
+ widget.render(
+ name + '_%s' % self.names[i], widget_value, final_attrs))
+ output.append('</fieldset>')
+ return mark_safe(self.format_output(output))
+
+ def id_for_label(self, id_):
+ # 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)."""
+ 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 thie 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):
+ 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-05-31 15:52:42 +0000
@@ -0,0 +1,194 @@
+# 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,
+ )
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase
+
+
+testField = 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)),
+ ])
+
+
+class TestForm(forms.Form):
+ multi_field = testField
+
+
+# A form where the DictCharField instance is constructed with skip_check=True.
+class TestFormSkip(forms.Form):
+ multi_field = DictCharField(
+ [('field_a', forms.CharField(label='Field a', max_length=3))],
+ skip_check=True)
+
+
+# A form where the DictCharField instance is constructed with required=False.
+class TestFormRequiredFalse(forms.Form):
+ multi_field = DictCharField(
+ [('field_a', forms.CharField(label='Field a'))],
+ required=False)
+ char_field = forms.CharField(label='Field a')
+
+
+class DictCharFieldTest(TestCase):
+
+ def test_DictCharField_init(self):
+ 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)
+
+
+class FormWithDictCharFieldTest(TestCase):
+
+ def test_DictCharField_processes_QueryDict_into_a_dict(self):
+ 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 = TestForm(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):
+ # 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 = TestForm(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.
+ fielda_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_field_a=%s&multi_field_skip_check=true' % (
+ fielda_value))
+ form = TestFormSkip(data)
+
+ self.assertTrue(form.is_valid())
+ self.assertEqual(
+ {'field_a': fielda_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.
+ fielda_value = factory.getRandomString(10)
+ # Force the check with multi_field_skip_check=false.
+ data = QueryDict(
+ 'multi_field_field_a=%s&multi_field_skip_check=false' % (
+ fielda_value))
+ form = TestFormSkip(data)
+
+ self.assertFalse(form.is_valid())
+ self.assertEqual(
+ {
+ 'multi_field': [
+ "Field a: Ensure this value has at most 3 characters "
+ "(it has 10)."]
+ },
+ form.errors)
+
+ def test_DictCharField_accepts_required_false(self):
+ char_value = factory.getRandomString(10)
+ data = QueryDict('char_field=%s' % (char_value))
+ form = TestFormRequiredFalse(data)
+ self.assertTrue(form.is_valid())
+ self.assertEqual(
+ {'char_field': char_value, 'multi_field': None},
+ form.cleaned_data)
+
+
+class DictCharWidgetTest(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))
