Merge lp:~rvb/maas/bug-1287310 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Fix the 'invalid choice' message for JSON-generated field so that the list of valid choices is included in the error message.

Drive-by fixes:
  - move the utility compose_invalid_choice_text into src/maasserver/utils/forms.py
  - fix the message generated by compose_invalid_choice_text so that the valid choices are quoted.

[Gavin Panella (allenap)]

Looks good, nice improvement.

[1]

146     +    return "'%s' is not a valid %s.  It should be one of: %s." % (
147     +        "%(value)s",
148     +        choice_of_what,
149     +        ", ".join("'%s'" % name for name, value in valid_choices),
150     +    )

Consider using englist_list() from Launchpad to produce an even better
string of choices. I suspect Django has something similar though.

[2]

148     +        choice_of_what,
149     +        ", ".join("'%s'" % name for name, value in valid_choices),
150     +    )

Is valid_choices sorted?

[3]

146     +    return "'%s' is not a valid %s.  It should be one of: %s." % (
147     +        "%(value)s",

Or double-up the percent symbol:

    return "'%%(value)s' is not a valid %s.  It should be one of: %s." % (

[4]

182     +        choices = [
183     +            (factory.make_name('value'), factory.make_name('key'))
184     +            for _ in range(2)]

I think value and key are the wrong way round here.

review: Approve

[Raphaël Badin (rvb)]

> Looks good, nice improvement.

Thanks for the review!

> [1]
>
> 146     +    return "'%s' is not a valid %s.  It should be one of: %s." % (
> 147     +        "%(value)s",
> 148     +        choice_of_what,
> 149     +        ", ".join("'%s'" % name for name, value in valid_choices),
> 150     +    )
>
> Consider using englist_list() from Launchpad to produce an even better
> string of choices. I suspect Django has something similar though.

Not that I can find right now. I'll keep it like this for now… we can always improve it later.

> [2]
>
> 148     +        choice_of_what,
> 149     +        ", ".join("'%s'" % name for name, value in valid_choices),
> 150     +    )
>
> Is valid_choices sorted?

No, but it's a list of tuples so it's better to just keep the ordering of that list.

> [3]
>
> 146     +    return "'%s' is not a valid %s.  It should be one of: %s." % (
> 147     +        "%(value)s",
>
> Or double-up the percent symbol:

Hum, didn't know that trick!

> [4]
>
> 182     +        choices = [
> 183     +            (factory.make_name('value'), factory.make_name('key'))
> 184     +            for _ in range(2)]
>
> I think value and key are the wrong way round here.

Well spotted, fixed.

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Julian Edwards (julian-edwards)]

On Tuesday 18 Mar 2014 06:38:44 you wrote:
> > Consider using englist_list() from Launchpad to produce an even better
> > string of choices. I suspect Django has something similar though.
>
> Not that I can find right now. I'll keep it like this for now… we can
> always improve it later.

He's just punting some code he wrote years ago in LP :)

(not that I am bitter that it was chosen in favour of mine because he used the
Oxford comma, apparently there were better things to worry about)

=== modified file 'src/maasserver/clusterrpc/power_parameters.py'
--- src/maasserver/clusterrpc/power_parameters.py	2014-03-11 06:55:34 +0000
+++ src/maasserver/clusterrpc/power_parameters.py	2014-03-18 06:39:09 +0000
@@ -40,6 +40,7 @@
 from maasserver.clusterrpc.utils import call_clusters
 from maasserver.config_forms import DictCharField
 from maasserver.fields import MACAddressFormField
+from maasserver.utils.forms import compose_invalid_choice_text
 from provisioningserver.power_schema import (
     JSON_POWER_TYPE_SCHEMA,
     POWER_TYPE_PARAMETER_FIELD_SCHEMA,
@@ -66,9 +67,13 @@
     field_class = FIELD_TYPE_MAPPINGS.get(
         json_field['field_type'], forms.CharField)
     if json_field['field_type'] == 'choice':
+        invalid_choice_message = compose_invalid_choice_text(
+            json_field['name'], json_field['choices'])
         extra_parameters = {
             'choices': json_field['choices'],
             'initial': json_field['default'],
+            'error_messages': {
+                'invalid_choice': invalid_choice_message},
             }
     else:
         extra_parameters = {}

=== modified file 'src/maasserver/clusterrpc/tests/test_power_parameters.py'
--- src/maasserver/clusterrpc/tests/test_power_parameters.py	2014-03-07 06:20:27 +0000
+++ src/maasserver/clusterrpc/tests/test_power_parameters.py	2014-03-18 06:39:09 +0000
@@ -33,6 +33,7 @@
     MAASServerTestCase,
     MAASTestCase,
     )
+from maasserver.utils.forms import compose_invalid_choice_text
 from maastesting.matchers import MockCalledOnceWith
 from mock import sentinel
 from provisioningserver.power.poweraction import PowerAction
@@ -142,6 +143,10 @@
         django_field = make_form_field(json_field)
         self.assertIsInstance(django_field, forms.ChoiceField)
         self.assertEqual(json_field['choices'], django_field.choices)
+        invalid_msg = compose_invalid_choice_text(
+            json_field['name'], json_field['choices'])
+        self.assertEqual(
+            invalid_msg, django_field.error_messages['invalid_choice'])
         self.assertEqual(json_field['default'], django_field.initial)
 
     def test_creates_mac_address_field_for_mac_addresses(self):

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-03-11 06:46:26 +0000
+++ src/maasserver/forms.py	2014-03-18 06:39:09 +0000
@@ -83,7 +83,6 @@
     NodeGroupFormField,
     )
 from maasserver.forms_settings import (
-    compose_invalid_choice_text,
     CONFIG_ITEMS_KEYS,
     get_config_field,
     INVALID_DISTRO_SERIES_MESSAGE,
@@ -109,6 +108,7 @@
     compile_node_actions,
     )
 from maasserver.utils import strip_domain
+from maasserver.utils.forms import compose_invalid_choice_text
 from maasserver.utils.network import make_network
 from metadataserver.fields import Bin
 from metadataserver.models import CommissioningScript

=== modified file 'src/maasserver/forms_settings.py'
--- src/maasserver/forms_settings.py	2014-02-27 14:37:46 +0000
+++ src/maasserver/forms_settings.py	2014-03-18 06:39:09 +0000
@@ -13,7 +13,6 @@
 
 __metaclass__ = type
 __all__ = [
-    'compose_invalid_choice_text',
     'CONFIG_ITEMS',
     'CONFIG_ITEMS_KEYS',
     'get_config_field',
@@ -29,26 +28,7 @@
     DISTRO_SERIES,
     DISTRO_SERIES_CHOICES,
     )
-
-
-def compose_invalid_choice_text(choice_of_what, valid_choices):
-    """Compose an "invalid choice" string for form error messages.
-
-    This returns a template string that is intended to be used as the
-    argument to the 'error_messages' parameter in a Django form.
-
-    :param choice_of_what: The name for what the selected item is supposed
-        to be, to be inserted into the error string.
-    :type choice_of_what: unicode
-    :param valid_choices: Valid choices, in Django choices format:
-        (name, value).
-    :type valid_choices: sequence
-    """
-    return "%s is not a valid %s.  It should be one of: %s." % (
-        "%(value)s",
-        choice_of_what,
-        ", ".join(name for name, value in valid_choices),
-    )
+from maasserver.utils.forms import compose_invalid_choice_text
 
 
 INVALID_URL_MESSAGE = "Enter a valid url (e.g. http://host.example.com)."

=== added file 'src/maasserver/utils/forms.py'
--- src/maasserver/utils/forms.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/utils/forms.py	2014-03-18 06:39:09 +0000
@@ -0,0 +1,37 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Form utilities."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+)
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    'compose_invalid_choice_text',
+]
+
+
+def compose_invalid_choice_text(choice_of_what, valid_choices):
+    """Compose an "invalid choice" string for form error messages.
+
+    This returns a template string that is intended to be used as the
+    argument to the 'error_messages' parameter in a Django form.
+
+    :param choice_of_what: The name for what the selected item is supposed
+        to be, to be inserted into the error string.
+    :type choice_of_what: unicode
+    :param valid_choices: Valid choices, in Django choices format:
+        (name, value).
+    :type valid_choices: sequence
+    """
+    return "'%s' is not a valid %s.  It should be one of: %s." % (
+        "%(value)s",
+        choice_of_what,
+        ", ".join("'%s'" % name for name, value in valid_choices),
+    )

=== added file 'src/maasserver/utils/tests/test_forms.py'
--- src/maasserver/utils/tests/test_forms.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/utils/tests/test_forms.py	2014-03-18 06:39:09 +0000
@@ -0,0 +1,33 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for forms helpers."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+
+from maasserver.testing.factory import factory
+from maasserver.utils.forms import compose_invalid_choice_text
+from maastesting.testcase import MAASTestCase
+from testtools.matchers import ContainsAll
+
+
+class TestComposeInvalidChoiceText(MAASTestCase):
+
+    def test_map_enum_includes_all_enum_values(self):
+        choices = [
+            (factory.make_name('key'), factory.make_name('value'))
+            for _ in range(2)]
+        msg = compose_invalid_choice_text(factory.make_name(), choices)
+        self.assertThat(
+            msg,
+            ContainsAll(["'%s'" % key for key, val in choices]))