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:
> >              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.
> I will correct this one right away, It was meant to return the whole
> 'cleaned_data', I may have mis-typed, sorry :)
> > +
> > +    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': '<email address hidden>',
> > +        }
> > +
> >      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': '<email address hidden>',
> > -        })
> > +        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': '<email address hidden>',
> > +            'advertised': 'True',
> >              'description': 'The Most Desirable organization',
> > -            'contact_address': '<email address hidden>',
> > -        })
> > -        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' : '<email address hidden>',
> > +            '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'] = '<email address hidden>'
> > +        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
> You are the owner of lp:~godricglow/postorius/tests_forms.
>



-- 

*Pranjal Yadav*

*IIT Kharagpur*