Merge lp:~rvb/maas/bad-msg-bug-1301465 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Fix NodeForm's is_valid() method so that it uses Django's way of setting errors on forms instead of putting text in self.errors['architecture'].

[Jeroen T. Vermeulen (jtv)]

Nice touch, thanks.

There's one change I'd like to ask for: error_key looks mysterious, as if you're giving up on encapsulation there and punting responsibility for explanation to Django. Would it be better called field_name?

I suppose there would be no point in checking for pre-existing errors on the same field, and appending instead of setting — although Django's API for this does look as if it was designed that way, sort of.

review: Approve

[Raphaël Badin (rvb)]

> Nice touch, thanks.
>
> There's one change I'd like to ask for: error_key looks mysterious, as if
> you're giving up on encapsulation there and punting responsibility for
> explanation to Django. Would it be better called field_name?

Good point, done.

> I suppose there would be no point in checking for pre-existing errors on the
> same field, and appending instead of setting — although Django's API for this
> does look as if it was designed that way, sort of.

Not really, this is meant to "highjack" the error message. I've improved the docstring to be really clear about this.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-05-20 14:16:58 +0000
+++ src/maasserver/forms.py	2014-05-26 14:15:15 +0000
@@ -122,6 +122,17 @@
 BLANK_CHOICE = ('', '-------')
 
 
+def set_form_error(form, field_name, error_value):
+    """Set an error on a form's field.
+
+    This utility method encapsulates Django's arguably awkward way
+    of settings errors inside a form's clean()/is_valid() method.  This
+    method will override any previously-registered error for 'field_name'.
+    """
+    # Hey Django devs, this is a crap API to set errors.
+    form.errors[field_name] = form.error_class([error_value])
+
+
 def remove_None_values(data):
     """Return a new dictionary without the keys corresponding to None values.
     """
@@ -253,8 +264,8 @@
     def is_valid(self):
         is_valid = super(NodeForm, self).is_valid()
         if len(list_all_usable_architectures()) == 0:
-            self.errors['architecture'] = (
-                [NO_ARCHITECTURES_AVAILABLE])
+            set_form_error(
+                self, "architecture", NO_ARCHITECTURES_AVAILABLE)
             is_valid = False
         return is_valid
 
@@ -424,9 +435,8 @@
             try:
                 get_power_types([self._get_nodegroup()])
             except ClusterUnavailable as e:
-                # Hey Django devs, this is a crap API to set errors.
-                self._errors["power_type"] = self.error_class(
-                    [CLUSTER_NOT_AVAILABLE + e.args[0]])
+                set_form_error(
+                    self, "power_type", CLUSTER_NOT_AVAILABLE + e.args[0])
         # If power_type is not set and power_parameters_skip_check is not
         # on, reset power_parameters (set it to the empty string).
         no_power_type = cleaned_data.get('power_type', '') == ''