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

Florian Fuchs f at florianfuchs.com
Tue Mar 10 10:13:08 CET 2015


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.

>          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.

>          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

The `clean` method needs to either return the whole `cleaned_data` dictionary or None (Django >= 1.7), but not just a single field.

> +
> +    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': 'contact at mailman.most-desirable.org',
> +        }
> +
>      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': 'contact at mailman.most-desirable.org',
> -        })
> +        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': 'james at example.com',
> +            'advertised': 'True',
>              'description': 'The Most Desirable organization',
> -            'contact_address': 'contact at mailman.most-desirable.org',
> -        })
> -        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' : 'pranjal at example.com',
> +            '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 at example..com'
> +        form = UserNew(self.form_data)
> +        self.assertFalse(form.is_valid())
> \ No newline at end of file
> 


-- 
https://code.launchpad.net/~godricglow/postorius/tests_forms/+merge/251740
Your team Mailman Coders is requested to review the proposed merge of lp:~godricglow/postorius/tests_forms into lp:postorius.


More information about the Mailman-coders mailing list