Merge lp:~godricglow/postorius/tests_forms into lp:postorius

[Pranjal Yadav]

Added tests for forms.py. Improving overall coverage

[Florian Fuchs (flo-fuchs)]

Hi Pranjal,

thanks for this merge proposal. It looks pretty good so far, I only have some minor comments (see below).

Florian

[Pranjal Yadav (godricglow)]

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/forms.py'
> > --- src/postorius/forms.py 2015-02-09 14:35:44 +0000
> > +++ src/postorius/forms.py 2015-03-04 13:56:28 +0000
> > @@ -17,7 +17,7 @@
> > # Postorius. If not, see <http://www.gnu.org/licenses/>.
> >
> > from django import forms
> > -from django.core.validators import validate_email, URLValidator
> > +from django.core.validators import validate_email, URLValidator,
> validate_slug
> > from django.utils.translation import gettext_lazy as _
> > from fieldset_forms import FieldsetForm
> > from django.forms.models import modelformset_factory
> > @@ -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=_('Description'),
> > required=True)
> >
> > +
> > def __init__(self, domain_choices, *args, **kwargs):
> > super(ListNew, self).__init__(*args, **kwargs)
> > self.fields["mail_host"] = forms.ChoiceField(
> > @@ -138,17 +140,20 @@
> > if len(domain_choices) < 2:
> > self.fields["mail_host"].help_text = _(
> > "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(self):
> > +
> > + cleaned_data = super(ListNew, self).clean()
> > + listname = cleaned_data.get('listname')
>
> For a single field's clean method, we don't need to call the parent's
> clean method. We can just access `self.cleaned_data.get('listname')`
> 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_email(self.cleaned_data['listname'] + '@
> example.net')
> > + validate_slug(listname)
>
> 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> on 2015-02-27

Adding new tests for forms.py

=== modified file 'src/postorius/forms.py'
--- src/postorius/forms.py	2015-02-09 14:35:44 +0000
+++ src/postorius/forms.py	2015-03-04 13:56:28 +0000
@@ -17,7 +17,7 @@
 # Postorius.  If not, see <http://www.gnu.org/licenses/>.
 
 from django import forms
-from django.core.validators import validate_email, URLValidator
+from django.core.validators import validate_email, URLValidator, validate_slug
 from django.utils.translation import gettext_lazy as _
 from fieldset_forms import FieldsetForm
 from django.forms.models import modelformset_factory
@@ -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=_('Description'),
         required=True)
 
+
     def __init__(self, domain_choices, *args, **kwargs):
         super(ListNew, self).__init__(*args, **kwargs)
         self.fields["mail_host"] = forms.ChoiceField(
@@ -138,17 +140,20 @@
         if len(domain_choices) < 2:
             self.fields["mail_host"].help_text = _(
                 "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(self):
+        
+        cleaned_data = super(ListNew, self).clean()
+        listname = cleaned_data.get('listname')
         try:
-            validate_email(self.cleaned_data['listname'] + '@example.net')
+            validate_slug(listname)
         except:
             raise forms.ValidationError(_("Please enter a valid listname"))
-        return self.cleaned_data['listname']
+        return listname
 
     class Meta:
 
@@ -827,14 +832,19 @@
         widget=forms.PasswordInput(render_value=False))
 
     def clean(self):
-        cleaned_data = self.cleaned_data
-        password = cleaned_data.get("password")
-        password_repeat = cleaned_data.get("password_repeat")
+        password = self.cleaned_data.get('password')
+        password_repeat = self.cleaned_data.get('password_repeat')
         if password != password_repeat:
             raise forms.ValidationError("Passwords must be identical.")
-
-        return cleaned_data
-
+        return password_repeat
+
+    def clean_email(self):
+        email = self.cleaned_data['email']
+        try:
+            validate_email(email)
+        except:
+            raise forms.ValidationError(_("Please enter a valid email ID"))
+        return email
 
 class UserSettings(FieldsetForm):
 

=== modified file 'src/postorius/tests/__init__.py'
--- src/postorius/tests/__init__.py	2015-02-10 13:56:47 +0000
+++ src/postorius/tests/__init__.py	2015-03-04 13:56:28 +0000
@@ -20,7 +20,6 @@
 
 from django.conf import settings
 
-
 TEST_ROOT = os.path.abspath(os.path.dirname(__file__))
 
 FIXTURES_DIR = getattr(

=== modified file 'src/postorius/tests/test_forms.py'
--- src/postorius/tests/test_forms.py	2015-02-09 14:35:44 +0000
+++ src/postorius/tests/test_forms.py	2015-03-04 13:56:28 +0000
@@ -15,9 +15,7 @@
 # You should have received a copy of the GNU General Public License along with
 # Postorius.  If not, see <http://www.gnu.org/licenses/>.
 from django.utils import unittest
-
-from postorius.forms import UserPreferences, DomainNew
-
+from postorius.forms import UserPreferences, DomainNew, ListNew, UserNew
 
 class UserPreferencesTest(unittest.TestCase):
 
@@ -32,20 +30,78 @@
 
 class DomainNewTest(unittest.TestCase):
 
+    def setUp(self):
+        self.form_data = {
+            'mail_host': 'mailman.most-desirable.org',
+            'web_host': 'http://mailman.most-desirable.org',
+            'description': 'The Most Desirable organization',
+            'contact_address': '[e-mail]',
+        }
+
     def test_form_fields_webhost(self):
-        form = DomainNew({
-            'mail_host': 'mailman.most-desirable.org',
-            'web_host': 'http://mailman.most-desirable.org',
-            'description': 'The Most Desirable organization',
-            'contact_address': '[e-mail]',
-        })
+        form = DomainNew(self.form_data)
         self.assertTrue(form.is_valid())
 
     def test_form_fields_webhost_invalid(self):
-        form = DomainNew({
+        self.form_data['web_host'] = 'mailman.most-desirable.org'
+        form = DomainNew(self.form_data)
+        self.assertFalse(form.is_valid())
+
+    def test_form_fields_mailhost(self):
+        form = DomainNew(self.form_data)
+        self.assertTrue(form.is_valid())
+
+    def test_form_fields_mailhost_invalid(self):
+        self.form_data['mail_host'] = 'mailman.most-desirable..org'
+        form = DomainNew(self.form_data)
+        self.assertFalse(form.is_valid())
+
+class ListNewTest(unittest.TestCase):
+
+    def setUp(self):
+        self.choosable_domains = [('mailman.most-desirable.org', 'mailman.most-desirable.org')]
+        
+        self.form_data = {
+            'listname' : 'test_list1',
             'mail_host': 'mailman.most-desirable.org',
-            'web_host': 'mailman.most-desirable.org',
+            'list_owner': '[e-mail]',
+            'advertised': 'True',
             'description': 'The Most Desirable organization',
-            'contact_address': '[e-mail]',
-        })
-        self.assertFalse(form.is_valid())
+        }
+
+    def test_form_fields_listname(self):
+        form = ListNew(self.choosable_domains,self.form_data)
+        self.assertTrue(form.is_valid())    
+
+    def test_form_fields_listname_invalid(self):
+        self.form_data['listname'] = 'test$list1'
+        form = ListNew(self.choosable_domains,self.form_data)
+        self.assertFalse(form.is_valid())
+
+class UserNewTest(unittest.TestCase): 
+
+    def setUp(self):
+        self.form_data = {
+            'display_name' : 'Pranjal Yadav',
+            'email' : '[e-mail]',
+            'password' : 'password123',                   
+            'password_repeat' : 'password123',
+        }
+
+    def test_form_fields_password(self):
+        form = UserNew(self.form_data)
+        self.assertTrue(form.is_valid())
+
+    def test_form_fields_password_invalid(self):
+        self.form_data['password_repeat'] = 'random'
+        form = UserNew(self.form_data)
+        self.assertFalse(form.is_valid())
+
+    def test_form_fields_email(self):
+        form = UserNew(self.form_data)
+        self.assertTrue(form.is_valid())
+
+    def test_form_fields_email_invalid(self):
+        self.form_data['email'] = 'pranjal@example..com'
+        form = UserNew(self.form_data)
+        self.assertFalse(form.is_valid())
\ No newline at end of file