Merge lp:~godricglow/postorius/tests_forms into lp:postorius
- tests_forms
- Merge into trunk
Status: | Needs review |
---|---|
Proposed branch: | lp:~godricglow/postorius/tests_forms |
Merge into: | lp:postorius |
Diff against target: |
196 lines (+90/-25) 3 files modified
src/postorius/forms.py (+20/-10) src/postorius/tests/__init__.py (+0/-1) src/postorius/tests/test_forms.py (+70/-14) |
To merge this branch: | bzr merge lp:~godricglow/postorius/tests_forms |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Florian Fuchs | Pending | ||
Review via email: mp+251740@code.launchpad.net |
Commit message
Description of the change
Added tests for forms.py. Improving overall coverage
Pranjal Yadav (godricglow) wrote : | # |
Hi Florian
Thanks for you reply, I checked all you comments carefully and have replied
below.
On Tue, Mar 10, 2015 at 2:43 PM, Florian Fuchs <email address hidden> wrote:
> Hi Pranjal,
>
> thanks for this merge proposal. It looks pretty good so far, I only have
> some minor comments (see below).
>
> Florian
>
> Diff comments:
>
> > === modified file 'src/postorius/
> > --- src/postorius/
> > +++ src/postorius/
> > @@ -17,7 +17,7 @@
> > # Postorius. If not, see <http://
> >
> > from django import forms
> > -from django.
> > +from django.
> validate_slug
> > from django.
> > from fieldset_forms import FieldsetForm
> > from django.forms.models import modelformset_
> > @@ -102,6 +102,7 @@
> > Form fields to add a new list. Languages are hard coded which should
> > be replaced by a REST lookup of available languages.
> > """
> > +
> > listname = forms.CharField(
> > label=_('List Name'),
> > required=True,
> > @@ -126,6 +127,7 @@
> > label=_
> > required=True)
> >
> > +
> > def __init__(self, domain_choices, *args, **kwargs):
> > super(ListNew, self)._
> > self.fields[
> > @@ -138,17 +140,20 @@
> > if len(domain_choices) < 2:
> > self.fields[
> > "Site admin has not created any domains")
> > - # if len(choices) < 2:
> > + #if len(choices) < 2:
> > # help_text=_("No domains available: " +
> > # "The site admin must create new domains " +
> > # "before you will be able to create a list")
> >
> > def clean_listname(
> > +
> > + cleaned_data = super(ListNew, self).clean()
> > + listname = cleaned_
>
> For a single field's clean method, we don't need to call the parent's
> clean method. We can just access `self.cleaned_
> directly.
>
Good point but I already tried doing that, it seems __init__ method creates
some problem otherwise when I don't call the parent's clean method. I
talked to abhilash and he too said that cleaning through parent seems to
work. I need your advice on this part.
>
> > try:
> > - validate_
> example.net')
> > + validate_
>
> I think `validate_slug` is too restrictive here, because the local part of
> an email address has more valid characters than validate_slug allows.
>
> I guess there is some confusion here, I'm validating the email with
> 'validate_email' only. I'm using validate_slug for listname only and I
> think listname need to have that restriction which again is not my decision
> but my choice as per sample data from various tests, Please tell me If I
> need to do otherwise.
> > except:
> > ...
Unmerged revisions
- 204. By Pranjal Yadav <email address hidden>
-
Adding new tests for forms.py
Preview Diff
1 | === modified file 'src/postorius/forms.py' | |||
2 | --- src/postorius/forms.py 2015-02-09 14:35:44 +0000 | |||
3 | +++ src/postorius/forms.py 2015-03-04 13:56:28 +0000 | |||
4 | @@ -17,7 +17,7 @@ | |||
5 | 17 | # Postorius. If not, see <http://www.gnu.org/licenses/>. | 17 | # Postorius. If not, see <http://www.gnu.org/licenses/>. |
6 | 18 | 18 | ||
7 | 19 | from django import forms | 19 | from django import forms |
9 | 20 | from django.core.validators import validate_email, URLValidator | 20 | from django.core.validators import validate_email, URLValidator, validate_slug |
10 | 21 | from django.utils.translation import gettext_lazy as _ | 21 | from django.utils.translation import gettext_lazy as _ |
11 | 22 | from fieldset_forms import FieldsetForm | 22 | from fieldset_forms import FieldsetForm |
12 | 23 | from django.forms.models import modelformset_factory | 23 | from django.forms.models import modelformset_factory |
13 | @@ -102,6 +102,7 @@ | |||
14 | 102 | Form fields to add a new list. Languages are hard coded which should | 102 | Form fields to add a new list. Languages are hard coded which should |
15 | 103 | be replaced by a REST lookup of available languages. | 103 | be replaced by a REST lookup of available languages. |
16 | 104 | """ | 104 | """ |
17 | 105 | |||
18 | 105 | listname = forms.CharField( | 106 | listname = forms.CharField( |
19 | 106 | label=_('List Name'), | 107 | label=_('List Name'), |
20 | 107 | required=True, | 108 | required=True, |
21 | @@ -126,6 +127,7 @@ | |||
22 | 126 | label=_('Description'), | 127 | label=_('Description'), |
23 | 127 | required=True) | 128 | required=True) |
24 | 128 | 129 | ||
25 | 130 | |||
26 | 129 | def __init__(self, domain_choices, *args, **kwargs): | 131 | def __init__(self, domain_choices, *args, **kwargs): |
27 | 130 | super(ListNew, self).__init__(*args, **kwargs) | 132 | super(ListNew, self).__init__(*args, **kwargs) |
28 | 131 | self.fields["mail_host"] = forms.ChoiceField( | 133 | self.fields["mail_host"] = forms.ChoiceField( |
29 | @@ -138,17 +140,20 @@ | |||
30 | 138 | if len(domain_choices) < 2: | 140 | if len(domain_choices) < 2: |
31 | 139 | self.fields["mail_host"].help_text = _( | 141 | self.fields["mail_host"].help_text = _( |
32 | 140 | "Site admin has not created any domains") | 142 | "Site admin has not created any domains") |
34 | 141 | # if len(choices) < 2: | 143 | #if len(choices) < 2: |
35 | 142 | # help_text=_("No domains available: " + | 144 | # help_text=_("No domains available: " + |
36 | 143 | # "The site admin must create new domains " + | 145 | # "The site admin must create new domains " + |
37 | 144 | # "before you will be able to create a list") | 146 | # "before you will be able to create a list") |
38 | 145 | 147 | ||
39 | 146 | def clean_listname(self): | 148 | def clean_listname(self): |
40 | 149 | |||
41 | 150 | cleaned_data = super(ListNew, self).clean() | ||
42 | 151 | listname = cleaned_data.get('listname') | ||
43 | 147 | try: | 152 | try: |
45 | 148 | validate_email(self.cleaned_data['listname'] + '@example.net') | 153 | validate_slug(listname) |
46 | 149 | except: | 154 | except: |
47 | 150 | raise forms.ValidationError(_("Please enter a valid listname")) | 155 | raise forms.ValidationError(_("Please enter a valid listname")) |
49 | 151 | return self.cleaned_data['listname'] | 156 | return listname |
50 | 152 | 157 | ||
51 | 153 | class Meta: | 158 | class Meta: |
52 | 154 | 159 | ||
53 | @@ -827,14 +832,19 @@ | |||
54 | 827 | widget=forms.PasswordInput(render_value=False)) | 832 | widget=forms.PasswordInput(render_value=False)) |
55 | 828 | 833 | ||
56 | 829 | def clean(self): | 834 | def clean(self): |
60 | 830 | cleaned_data = self.cleaned_data | 835 | password = self.cleaned_data.get('password') |
61 | 831 | password = cleaned_data.get("password") | 836 | password_repeat = self.cleaned_data.get('password_repeat') |
59 | 832 | password_repeat = cleaned_data.get("password_repeat") | ||
62 | 833 | if password != password_repeat: | 837 | if password != password_repeat: |
63 | 834 | raise forms.ValidationError("Passwords must be identical.") | 838 | raise forms.ValidationError("Passwords must be identical.") |
67 | 835 | 839 | return password_repeat | |
68 | 836 | return cleaned_data | 840 | |
69 | 837 | 841 | def clean_email(self): | |
70 | 842 | email = self.cleaned_data['email'] | ||
71 | 843 | try: | ||
72 | 844 | validate_email(email) | ||
73 | 845 | except: | ||
74 | 846 | raise forms.ValidationError(_("Please enter a valid email ID")) | ||
75 | 847 | return email | ||
76 | 838 | 848 | ||
77 | 839 | class UserSettings(FieldsetForm): | 849 | class UserSettings(FieldsetForm): |
78 | 840 | 850 | ||
79 | 841 | 851 | ||
80 | === modified file 'src/postorius/tests/__init__.py' | |||
81 | --- src/postorius/tests/__init__.py 2015-02-10 13:56:47 +0000 | |||
82 | +++ src/postorius/tests/__init__.py 2015-03-04 13:56:28 +0000 | |||
83 | @@ -20,7 +20,6 @@ | |||
84 | 20 | 20 | ||
85 | 21 | from django.conf import settings | 21 | from django.conf import settings |
86 | 22 | 22 | ||
87 | 23 | |||
88 | 24 | TEST_ROOT = os.path.abspath(os.path.dirname(__file__)) | 23 | TEST_ROOT = os.path.abspath(os.path.dirname(__file__)) |
89 | 25 | 24 | ||
90 | 26 | FIXTURES_DIR = getattr( | 25 | FIXTURES_DIR = getattr( |
91 | 27 | 26 | ||
92 | === modified file 'src/postorius/tests/test_forms.py' | |||
93 | --- src/postorius/tests/test_forms.py 2015-02-09 14:35:44 +0000 | |||
94 | +++ src/postorius/tests/test_forms.py 2015-03-04 13:56:28 +0000 | |||
95 | @@ -15,9 +15,7 @@ | |||
96 | 15 | # You should have received a copy of the GNU General Public License along with | 15 | # You should have received a copy of the GNU General Public License along with |
97 | 16 | # Postorius. If not, see <http://www.gnu.org/licenses/>. | 16 | # Postorius. If not, see <http://www.gnu.org/licenses/>. |
98 | 17 | from django.utils import unittest | 17 | from django.utils import unittest |
102 | 18 | 18 | from postorius.forms import UserPreferences, DomainNew, ListNew, UserNew | |
100 | 19 | from postorius.forms import UserPreferences, DomainNew | ||
101 | 20 | |||
103 | 21 | 19 | ||
104 | 22 | class UserPreferencesTest(unittest.TestCase): | 20 | class UserPreferencesTest(unittest.TestCase): |
105 | 23 | 21 | ||
106 | @@ -32,20 +30,78 @@ | |||
107 | 32 | 30 | ||
108 | 33 | class DomainNewTest(unittest.TestCase): | 31 | class DomainNewTest(unittest.TestCase): |
109 | 34 | 32 | ||
110 | 33 | def setUp(self): | ||
111 | 34 | self.form_data = { | ||
112 | 35 | 'mail_host': 'mailman.most-desirable.org', | ||
113 | 36 | 'web_host': 'http://mailman.most-desirable.org', | ||
114 | 37 | 'description': 'The Most Desirable organization', | ||
115 | 38 | 'contact_address': 'contact@mailman.most-desirable.org', | ||
116 | 39 | } | ||
117 | 40 | |||
118 | 35 | def test_form_fields_webhost(self): | 41 | def test_form_fields_webhost(self): |
125 | 36 | form = DomainNew({ | 42 | form = DomainNew(self.form_data) |
120 | 37 | 'mail_host': 'mailman.most-desirable.org', | ||
121 | 38 | 'web_host': 'http://mailman.most-desirable.org', | ||
122 | 39 | 'description': 'The Most Desirable organization', | ||
123 | 40 | 'contact_address': 'contact@mailman.most-desirable.org', | ||
124 | 41 | }) | ||
126 | 42 | self.assertTrue(form.is_valid()) | 43 | self.assertTrue(form.is_valid()) |
127 | 43 | 44 | ||
128 | 44 | def test_form_fields_webhost_invalid(self): | 45 | def test_form_fields_webhost_invalid(self): |
130 | 45 | form = DomainNew({ | 46 | self.form_data['web_host'] = 'mailman.most-desirable.org' |
131 | 47 | form = DomainNew(self.form_data) | ||
132 | 48 | self.assertFalse(form.is_valid()) | ||
133 | 49 | |||
134 | 50 | def test_form_fields_mailhost(self): | ||
135 | 51 | form = DomainNew(self.form_data) | ||
136 | 52 | self.assertTrue(form.is_valid()) | ||
137 | 53 | |||
138 | 54 | def test_form_fields_mailhost_invalid(self): | ||
139 | 55 | self.form_data['mail_host'] = 'mailman.most-desirable..org' | ||
140 | 56 | form = DomainNew(self.form_data) | ||
141 | 57 | self.assertFalse(form.is_valid()) | ||
142 | 58 | |||
143 | 59 | class ListNewTest(unittest.TestCase): | ||
144 | 60 | |||
145 | 61 | def setUp(self): | ||
146 | 62 | self.choosable_domains = [('mailman.most-desirable.org', 'mailman.most-desirable.org')] | ||
147 | 63 | |||
148 | 64 | self.form_data = { | ||
149 | 65 | 'listname' : 'test_list1', | ||
150 | 46 | 'mail_host': 'mailman.most-desirable.org', | 66 | 'mail_host': 'mailman.most-desirable.org', |
152 | 47 | 'web_host': 'mailman.most-desirable.org', | 67 | 'list_owner': 'james@example.com', |
153 | 68 | 'advertised': 'True', | ||
154 | 48 | 'description': 'The Most Desirable organization', | 69 | 'description': 'The Most Desirable organization', |
158 | 49 | 'contact_address': 'contact@mailman.most-desirable.org', | 70 | } |
159 | 50 | }) | 71 | |
160 | 51 | self.assertFalse(form.is_valid()) | 72 | def test_form_fields_listname(self): |
161 | 73 | form = ListNew(self.choosable_domains,self.form_data) | ||
162 | 74 | self.assertTrue(form.is_valid()) | ||
163 | 75 | |||
164 | 76 | def test_form_fields_listname_invalid(self): | ||
165 | 77 | self.form_data['listname'] = 'test$list1' | ||
166 | 78 | form = ListNew(self.choosable_domains,self.form_data) | ||
167 | 79 | self.assertFalse(form.is_valid()) | ||
168 | 80 | |||
169 | 81 | class UserNewTest(unittest.TestCase): | ||
170 | 82 | |||
171 | 83 | def setUp(self): | ||
172 | 84 | self.form_data = { | ||
173 | 85 | 'display_name' : 'Pranjal Yadav', | ||
174 | 86 | 'email' : 'pranjal@example.com', | ||
175 | 87 | 'password' : 'password123', | ||
176 | 88 | 'password_repeat' : 'password123', | ||
177 | 89 | } | ||
178 | 90 | |||
179 | 91 | def test_form_fields_password(self): | ||
180 | 92 | form = UserNew(self.form_data) | ||
181 | 93 | self.assertTrue(form.is_valid()) | ||
182 | 94 | |||
183 | 95 | def test_form_fields_password_invalid(self): | ||
184 | 96 | self.form_data['password_repeat'] = 'random' | ||
185 | 97 | form = UserNew(self.form_data) | ||
186 | 98 | self.assertFalse(form.is_valid()) | ||
187 | 99 | |||
188 | 100 | def test_form_fields_email(self): | ||
189 | 101 | form = UserNew(self.form_data) | ||
190 | 102 | self.assertTrue(form.is_valid()) | ||
191 | 103 | |||
192 | 104 | def test_form_fields_email_invalid(self): | ||
193 | 105 | self.form_data['email'] = 'pranjal@example..com' | ||
194 | 106 | form = UserNew(self.form_data) | ||
195 | 107 | self.assertFalse(form.is_valid()) | ||
196 | 52 | \ No newline at end of file | 108 | \ No newline at end of file |
Hi Pranjal,
thanks for this merge proposal. It looks pretty good so far, I only have some minor comments (see below).
Florian