Merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:~maas-committers/maas/trunk

Proposed by Raphaël Badin
Status: Merged
Approved by: Raphaël Badin
Approved revision: no longer in the source branch.
Merged at revision: 2284
Proposed branch: lp:~rvb/maas/form-error-msg-bug-1308292
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 360 lines (+159/-24)
8 files modified
src/maasserver/api.py (+2/-2)
src/maasserver/fields.py (+43/-9)
src/maasserver/forms.py (+34/-5)
src/maasserver/tests/test_api_enlistment.py (+5/-2)
src/maasserver/tests/test_api_macaddress.py (+1/-1)
src/maasserver/tests/test_api_node.py (+3/-3)
src/maasserver/tests/test_fields.py (+31/-0)
src/maasserver/tests/test_forms.py (+40/-2)
To merge this branch: bzr merge lp:~rvb/maas/form-error-msg-bug-1308292
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+216416@code.launchpad.net

Commit message

Make NodeWithMACAddressesForm/AdminNodeWithMACAddressesForm return more meaningful errors when validation fails.

Description of the change

I had to create custom VerboseRegexValidator/VerboseRegexField because the the ones Django provides (RegexValidator/RegexField) don't include the value of the value checked in the error message that is returned when validation fails.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :
Download full text (7.1 KiB)

Nice improvement, thanks! Notes follow:

src/maasserver/fields.py
------------------------

This follows the pattern as it already was, so not a fault of this branch per se, but:

    mac_error_msg = "'%(value)s' is an invalid MAC address."

I often forget this, but I think we were supposed to stick to upper-case names for globals.

Could you make the message say “is not a valid MAC address” instead of “is an invalid MAC address”? The only reason to call the value a MAC address at all here is that the user, mistakenly, presented it as one. Echoing a part of their incorrect belief back to them while contradicting another part implies partial affirmation: “Yes, this is a MAC address like you said, but it's not a valid one.” That could mean “the vendor ID is a reserved value,” or who knows, “there's another NIC with the same MAC out there.” It will all work out in the end, but there is room for confusion.

Of course that does raise questions about the “One or more MAC addresses is invalid” elsewhere in the code. Not as serious, I think, because it's clear that the message doesn't go into the technical details. How about “Malformed value in MAC addresses”?

.

What is VerboseRegexValidator.code for? It looks mysterious. Comment?

.

The docstring for VerboseRegexValidator.__call__ says:

        Validates that the input matches the regular expression.

We normally put the first line of docstring right after the opening quotes, and if it's a single-line docstring and it will fit, we put the closing quotes on the same line. Did you mean to do some more work on this docstring before proposing the branch for merging? Was it originally written for __init__?

Also, this is a function docstring — that sentence should be in the imperative.

.

VerboseRegexField.__init__ has this docstring:

        regex can be either a string or a compiled regular expression object.
        error_message is an optional error message to use, if
        'Enter a valid value' is too generic for you.

That's not very easy to read, for several reasons. The parameter names make it impossible to start the sentences with capitals, so why not use the standard :param: fields? Single non-directional quotes look strange in running text, and resemble apostrophes. The ‘error_message’ parameter is actually just called ‘message.’ And, you're explaining ‘message’ in terms of an obscure default that is hidden away in Django somewhere, irrelevant to this function and, as you point out, uselessly generic. You did the right thing in not supporting the default; now remove it from the docstring!

.

src/maasserver/forms.py
-----------------------

It's a bit odd to have MAX_NUMBER_OF_MESSAGES be so verbose about “number of,” even while it abbreviates “maximum.” Maybe just call it MAX_MESSAGES?

.

I like merge_error_messages, and I don't want to slow you down too much on this, but manipulating text like this is always tricky. It's easy to produce gibberish; you'll know what I mean if you've ever seen MS-DOS or Windows 95 boot in a language other than English.

If you want to concatenate independently produced, stand-alone sentences or even brief paragraphs, they need very cl...

Read more...

review: Approve
Revision history for this message
Raphaël Badin (rvb) wrote :
Download full text (7.9 KiB)

> Nice improvement, thanks! Notes follow:

Thanks for the review!

>
> src/maasserver/fields.py
> ------------------------
>
> This follows the pattern as it already was, so not a fault of this branch per
> se, but:
>
> mac_error_msg = "'%(value)s' is an invalid MAC address."
>
> I often forget this, but I think we were supposed to stick to upper-case names
> for globals.
>
> Could you make the message say “is not a valid MAC address” instead of “is an
> invalid MAC address”? The only reason to call the value a MAC address at all
> here is that the user, mistakenly, presented it as one. Echoing a part of
> their incorrect belief back to them while contradicting another part implies
> partial affirmation: “Yes, this is a MAC address like you said, but it's not a
> valid one.” That could mean “the vendor ID is a reserved value,” or who
> knows, “there's another NIC with the same MAC out there.” It will all work
> out in the end, but there is room for confusion.
>
> Of course that does raise questions about the “One or more MAC addresses is
> invalid” elsewhere in the code. Not as serious, I think, because it's clear
> that the message doesn't go into the technical details. How about “Malformed
> value in MAC addresses”?

I think I prefer “is not a valid MAC address”. And I turned mac_error_msg into MAC_ERROR_MSG and mac_re into MAC_RE.

> What is VerboseRegexValidator.code for? It looks mysterious. Comment?

Done.

>
> .
>
> The docstring for VerboseRegexValidator.__call__ says:
>
> Validates that the input matches the regular expression.
>
> We normally put the first line of docstring right after the opening quotes,
> and if it's a single-line docstring and it will fit, we put the closing quotes
> on the same line. Did you mean to do some more work on this docstring before
> proposing the branch for merging? Was it originally written for __init__?
>
> Also, this is a function docstring — that sentence should be in the
> imperative.

Yeah, I've been lazy, this is a comment I copied straight from Django's code. Fixed.

> .
>
> VerboseRegexField.__init__ has this docstring:
>
> regex can be either a string or a compiled regular expression object.
> error_message is an optional error message to use, if
> 'Enter a valid value' is too generic for you.
>
> That's not very easy to read, for several reasons. The parameter names make
> it impossible to start the sentences with capitals, so why not use the
> standard :param: fields? Single non-directional quotes look strange in
> running text, and resemble apostrophes. The ‘error_message’ parameter is
> actually just called ‘message.’ And, you're explaining ‘message’ in terms of
> an obscure default that is hidden away in Django somewhere, irrelevant to this
> function and, as you point out, uselessly generic. You did the right thing in
> not supporting the default; now remove it from the docstring!

Same here (comment copied from Django), fixed.

> src/maasserver/forms.py
> -----------------------
>
> It's a bit odd to have MAX_NUMBER_OF_MESSAGES be so verbose about “number of,”
> even while it abbreviates “maximum.” Maybe just call i...

Read more...

Revision history for this message
Raphaël Badin (rvb) wrote :

> (Pop text-processing quiz: If x is an all-ASCII unicode string, is x.lower()
> guaranteed to be an all-ASCII unicode string? All code is broken. :-)

Looks like it, what am I missing? http://paste.ubuntu.com/7306338/

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

The Turkish language has two letters “i”: with the dot and without. As I understand it, and it looks very sensible to me, the lower-case version of ‘I’ is ‘ı’ whereas the upper-case version of ‘i’ is ‘İ’.

So presumably, with a Turkish locale, "HI".lower() == "hı"!

Revision history for this message
Raphaël Badin (rvb) wrote :

> The Turkish language has two letters “i”: with the dot and without. As I
> understand it, and it looks very sensible to me, the lower-case version of ‘I’
> is ‘ı’ whereas the upper-case version of ‘i’ is ‘İ’.
>
> So presumably, with a Turkish locale, "HI".lower() == "hı"!

Fail: http://paste.ubuntu.com/7306718/

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

You didn't actually print 'hI'.lower(), just 'hI'.

But yes, it looks like Python doesn't do Turkish upper/lower-casing! Try it using e.g. ‘awk’ and you'll get different results... All software is still broken. :-)

Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (2.7 MiB)

The attempt to merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Get:2 http://security.ubuntu.com trusty-security Release [58.5 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [58.5 kB]
Get:5 http://security.ubuntu.com trusty-security/main Sources [970 B]
Get:6 http://security.ubuntu.com trusty-security/universe Sources [14 B]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [977 B]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [14 B]
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [3,405 B]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [648 B]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [10.1 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [1,602 B]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 137 kB in 0s (669 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschem...

Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (2.7 MiB)

The attempt to merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 933 B in 0s (5,962 B/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-netifaces python-oauth python-oops python-oops-amqp python-oops-dated...

Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (54.2 KiB)

The attempt to merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-netifaces python-oauth python-oops python-oops-amqp python-oops-datedir-repo python-oops-twisted python-oops-ws...

Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (54.4 KiB)

The attempt to merge lp:~rvb/maas/form-error-msg-bug-1308292 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:2 http://security.ubuntu.com trusty-security Release [58.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [58.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:5 http://security.ubuntu.com trusty-security/main Sources [972 B]
Get:6 http://security.ubuntu.com trusty-security/universe Sources [14 B]
Get:7 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [3,407 B]
Get:8 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [648 B]
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [10.1 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [1,602 B]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Get:11 http://security.ubuntu.com trusty-security/main amd64 Packages [971 B]
Get:12 http://security.ubuntu.com trusty-security/universe amd64 Packages [14 B]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 137 kB in 0s (712 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschem...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api.py'
2--- src/maasserver/api.py 2014-04-14 23:32:50 +0000
3+++ src/maasserver/api.py 2014-04-23 07:31:18 +0000
4@@ -172,7 +172,7 @@
5 Unauthorized,
6 )
7 from maasserver.fields import (
8- mac_re,
9+ MAC_RE,
10 validate_mac,
11 )
12 from maasserver.forms import (
13@@ -785,7 +785,7 @@
14 match_macs = get_optional_list(request.GET, 'mac_address')
15 if match_macs is not None:
16 invalid_macs = [
17- mac for mac in match_macs if mac_re.match(mac) is None]
18+ mac for mac in match_macs if MAC_RE.match(mac) is None]
19 if len(invalid_macs) != 0:
20 raise ValidationError(
21 "Invalid MAC address(es): %s" % ", ".join(invalid_macs))
22
23=== modified file 'src/maasserver/fields.py'
24--- src/maasserver/fields.py 2014-02-21 06:59:04 +0000
25+++ src/maasserver/fields.py 2014-04-23 07:31:18 +0000
26@@ -17,6 +17,7 @@
27 "MACAddressField",
28 "MACAddressFormField",
29 "register_mac_type",
30+ "VerboseRegexValidator",
31 ]
32
33 from copy import deepcopy
34@@ -33,20 +34,40 @@
35 SubfieldBase,
36 )
37 from django.forms import (
38+ CharField,
39 ModelChoiceField,
40- RegexField,
41 )
42+from django.utils.encoding import force_text
43 from maasserver.utils.orm import get_one
44 import psycopg2.extensions
45 from south.modelsinspector import add_introspection_rules
46
47
48-mac_re = re.compile(r'^\s*([0-9a-fA-F]{2}[:-]){5}[0-9a-fA-F]{2}\s*$')
49-
50-
51-mac_error_msg = "Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF)."
52-
53-mac_validator = RegexValidator(regex=mac_re, message=mac_error_msg)
54+MAC_RE = re.compile(r'^\s*([0-9a-fA-F]{2}[:-]){5}[0-9a-fA-F]{2}\s*$')
55+
56+
57+MAC_ERROR_MSG = "'%(value)s' is not a valid MAC address."
58+
59+
60+class VerboseRegexValidator(RegexValidator):
61+ """A verbose `RegexValidator`.
62+
63+ This `RegexValidator` includes the checked value in the rendered error
64+ message when the validation fails.
65+ """
66+ # Set a bugus code to circumvent Django's attempt to re-interpret a
67+ # validator's error message using the field's message it is attached
68+ # to.
69+ code = 'bogus-code'
70+
71+ def __call__(self, value):
72+ """Validates that the input matches the regular expression."""
73+ if not self.regex.search(force_text(value)):
74+ raise ValidationError(
75+ self.message % {'value': value}, code=self.code)
76+
77+
78+mac_validator = VerboseRegexValidator(regex=MAC_RE, message=MAC_ERROR_MSG)
79
80
81 def validate_mac(value):
82@@ -157,12 +178,25 @@
83 return nodegroup
84
85
86-class MACAddressFormField(RegexField):
87+class VerboseRegexField(CharField):
88+
89+ def __init__(self, regex, message, *args, **kwargs):
90+ """A field that validates its value with a regular expression.
91+
92+ :param regex: Either a string or a compiled regular expression object.
93+ :param message: Error message to use when the validation fails.
94+ """
95+ super(VerboseRegexField, self).__init__(*args, **kwargs)
96+ self.validators.append(
97+ VerboseRegexValidator(regex=regex, message=message))
98+
99+
100+class MACAddressFormField(VerboseRegexField):
101 """Form field type: MAC address."""
102
103 def __init__(self, *args, **kwargs):
104 super(MACAddressFormField, self).__init__(
105- regex=mac_re, error_message=mac_error_msg, *args, **kwargs)
106+ regex=MAC_RE, message=MAC_ERROR_MSG, *args, **kwargs)
107
108
109 class MACAddressField(Field):
110
111=== modified file 'src/maasserver/forms.py'
112--- src/maasserver/forms.py 2014-04-10 20:21:24 +0000
113+++ src/maasserver/forms.py 2014-04-23 07:31:18 +0000
114@@ -532,6 +532,33 @@
115
116 IP_BASED_HOSTNAME_REGEXP = re.compile('\d{1,3}-\d{1,3}-\d{1,3}-\d{1,3}')
117
118+MAX_MESSAGES = 10
119+
120+
121+def merge_error_messages(summary, errors, limit=MAX_MESSAGES):
122+ """Merge a collection of errors into a summary message of limited size.
123+
124+ :param summary: The message summarizing the error.
125+ :type summary: unicode
126+ :param errors: The list of errors to merge.
127+ :type errors: iterable
128+ :param limit: The maximum number of individual error messages to include in
129+ the summary error message.
130+ :type limit: int
131+ """
132+ ellipsis_msg = ''
133+ if len(errors) > limit:
134+ nb_errors = len(errors) - limit
135+ ellipsis_msg = (
136+ " and %d more error%s" % (
137+ nb_errors,
138+ 's' if nb_errors > 1 else ''))
139+ return "%s (%s%s)" % (
140+ summary,
141+ ' \u2014 '.join(errors[:limit]),
142+ ellipsis_msg
143+ )
144+
145
146 class WithMACAddressesMixin:
147 """A form mixin which dynamically adds a MultipleMACAddressField to the
148@@ -558,17 +585,19 @@
149 self.errors.get('mac_addresses', None) is not None and
150 len(self.data['mac_addresses']) > 1)
151 if reformat_mac_address_error:
152- self.errors['mac_addresses'] = (
153- ['One or more MAC addresses is invalid.'])
154+ self.errors['mac_addresses'] = [merge_error_messages(
155+ "One or more MAC addresses is invalid.",
156+ self.errors['mac_addresses'])]
157 return valid
158
159 def clean_mac_addresses(self):
160 data = self.cleaned_data['mac_addresses']
161+ errors = []
162 for mac in data:
163 if MACAddress.objects.filter(mac_address=mac.lower()).exists():
164- raise ValidationError(
165- {'mac_addresses': [
166- 'Mac address %s already in use.' % mac]})
167+ errors.append('MAC address %s already in use.' % mac)
168+ if errors:
169+ raise ValidationError({'mac_addresses': errors})
170 return data
171
172 def save(self):
173
174=== modified file 'src/maasserver/tests/test_api_enlistment.py'
175--- src/maasserver/tests/test_api_enlistment.py 2014-03-27 04:15:45 +0000
176+++ src/maasserver/tests/test_api_enlistment.py 2014-04-23 07:31:18 +0000
177@@ -280,7 +280,7 @@
178 self.assertEqual(httplib.BAD_REQUEST, response.status_code)
179 self.assertIn('application/json', response['Content-Type'])
180 self.assertEqual(
181- ["Mac address %s already in use." % mac],
182+ ["MAC address %s already in use." % mac],
183 parsed_result['mac_addresses'])
184
185 def test_POST_fails_with_bad_operation(self):
186@@ -316,7 +316,10 @@
187 self.assertEqual(httplib.BAD_REQUEST, response.status_code)
188 self.assertIn('application/json', response['Content-Type'])
189 self.assertEqual(
190- ["One or more MAC addresses is invalid."],
191+ [
192+ "One or more MAC addresses is invalid. "
193+ "('invalid' is not a valid MAC address.)"
194+ ],
195 parsed_result['mac_addresses'])
196
197 def test_POST_invalid_architecture_returns_bad_request(self):
198
199=== modified file 'src/maasserver/tests/test_api_macaddress.py'
200--- src/maasserver/tests/test_api_macaddress.py 2014-02-20 10:29:15 +0000
201+++ src/maasserver/tests/test_api_macaddress.py 2014-04-23 07:31:18 +0000
202@@ -120,7 +120,7 @@
203 self.assertEqual(400, response.status_code)
204 self.assertEqual(['mac_address'], list(parsed_result))
205 self.assertEqual(
206- ["Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF)."],
207+ ["'invalid-mac' is not a valid MAC address."],
208 parsed_result['mac_address'])
209
210 def test_macs_DELETE_mac(self):
211
212=== modified file 'src/maasserver/tests/test_api_node.py'
213--- src/maasserver/tests/test_api_node.py 2014-04-03 16:12:28 +0000
214+++ src/maasserver/tests/test_api_node.py 2014-04-23 07:31:18 +0000
215@@ -29,7 +29,7 @@
216 )
217 from maasserver.fields import (
218 MAC,
219- mac_error_msg,
220+ MAC_ERROR_MSG,
221 )
222 from maasserver.models import Node
223 from maasserver.testing import (
224@@ -574,11 +574,11 @@
225 response = self.client_put(
226 self.get_node_uri(node),
227 {'power_parameters_mac_address': new_power_address})
228-
229+ error_msg = MAC_ERROR_MSG % {'value': new_power_address}
230 self.assertEqual(
231 (
232 httplib.BAD_REQUEST,
233- {'power_parameters': ["MAC Address: %s" % mac_error_msg]},
234+ {'power_parameters': ["MAC Address: %s" % error_msg]},
235 ),
236 (response.status_code, json.loads(response.content)))
237
238
239=== modified file 'src/maasserver/tests/test_fields.py'
240--- src/maasserver/tests/test_fields.py 2014-02-21 03:49:40 +0000
241+++ src/maasserver/tests/test_fields.py 2014-04-23 07:31:18 +0000
242@@ -28,6 +28,8 @@
243 NodeGroupFormField,
244 register_mac_type,
245 validate_mac,
246+ VerboseRegexField,
247+ VerboseRegexValidator,
248 )
249 from maasserver.models import (
250 MACAddress,
251@@ -222,6 +224,35 @@
252 pass
253
254
255+class TestVerboseRegexValidator(MAASServerTestCase):
256+
257+ def test_VerboseRegexValidator_validates_value(self):
258+ validator = VerboseRegexValidator(
259+ regex="test", message="Unknown value")
260+ self.assertIsNone(validator('test'))
261+
262+ def test_VerboseRegexValidator_validation_error_includes_value(self):
263+ message = "Unknown value: %(value)s"
264+ validator = VerboseRegexValidator(regex="test", message=message)
265+ value = factory.make_name('value')
266+ error = self.assertRaises(ValidationError, validator, value)
267+ self.assertEqual(message % {'value': value}, error.message)
268+
269+
270+class TestVerboseRegexField(MAASServerTestCase):
271+
272+ def test_VerboseRegexField_accepts_valid_value(self):
273+ field = VerboseRegexField(regex="test", message="Unknown value")
274+ self.assertEqual('test', field.clean('test'))
275+
276+ def test_VerboseRegexField_validation_error_includes_value(self):
277+ message = "Unknown value: %(value)s"
278+ field = VerboseRegexField(regex="test", message=message)
279+ value = factory.make_name('value')
280+ error = self.assertRaises(ValidationError, field.clean, value)
281+ self.assertEqual([message % {'value': value}], error.messages)
282+
283+
284 class TestMACAddressField(MAASServerTestCase):
285
286 def test_mac_address_is_stored_normalized_and_loaded(self):
287
288=== modified file 'src/maasserver/tests/test_forms.py'
289--- src/maasserver/tests/test_forms.py 2014-04-10 15:30:11 +0000
290+++ src/maasserver/tests/test_forms.py 2014-04-23 07:31:18 +0000
291@@ -47,6 +47,8 @@
292 INTERFACES_VALIDATION_ERROR_MESSAGE,
293 list_all_usable_architectures,
294 MACAddressForm,
295+ MAX_MESSAGES,
296+ merge_error_messages,
297 NetworkForm,
298 NewUserCreationForm,
299 NO_ARCHITECTURES_AVAILABLE,
300@@ -274,7 +276,7 @@
301 self.assertFalse(form.is_valid())
302 self.assertEqual(['mac_addresses'], list(form.errors))
303 self.assertEqual(
304- ['Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF).'],
305+ ["'invalid' is not a valid MAC address."],
306 form.errors['mac_addresses'])
307
308 def test_NodeWithMACAddressesForm_multiple_invalid(self):
309@@ -287,7 +289,11 @@
310 self.assertFalse(form.is_valid())
311 self.assertEqual(['mac_addresses'], list(form.errors))
312 self.assertEqual(
313- ['One or more MAC addresses is invalid.'],
314+ [
315+ "One or more MAC addresses is invalid. "
316+ "('invalid_1' is not a valid MAC address. \u2014"
317+ " 'invalid_2' is not a valid MAC address.)"
318+ ],
319 form.errors['mac_addresses'])
320
321 def test_NodeWithMACAddressesForm_empty(self):
322@@ -782,6 +788,38 @@
323 list(form.fields))
324
325
326+class TestMergeErrorMessages(MAASServerTestCase):
327+
328+ def test_merge_error_messages_returns_summary_message(self):
329+ summary = factory.make_name('summary')
330+ errors = [factory.make_name('error') for _ in range(2)]
331+ result = merge_error_messages(summary, errors, 5)
332+ self.assertEqual(
333+ "%s (%s)" % (summary, ' \u2014 '.join(errors)), result)
334+
335+ def test_merge_error_messages_includes_limited_number_of_msgs(self):
336+ summary = factory.make_name('summary')
337+ errors = [
338+ factory.make_name('error')
339+ for _ in range(MAX_MESSAGES + 2)]
340+ result = merge_error_messages(summary, errors)
341+ self.assertEqual(
342+ "%s (%s and 2 more errors)" % (
343+ summary, ' \u2014 '.join(errors[:MAX_MESSAGES])),
344+ result)
345+
346+ def test_merge_error_messages_with_one_more_error(self):
347+ summary = factory.make_name('summary')
348+ errors = [
349+ factory.make_name('error')
350+ for _ in range(MAX_MESSAGES + 1)]
351+ result = merge_error_messages(summary, errors)
352+ self.assertEqual(
353+ "%s (%s and 1 more error)" % (
354+ summary, ' \u2014 '.join(errors[:MAX_MESSAGES])),
355+ result)
356+
357+
358 class TestMACAddressForm(MAASServerTestCase):
359
360 def test_MACAddressForm_creates_mac_address(self):