Code review comment for lp:~godricglow/postorius/tests_forms

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*