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 <f...@florianfuchs.com> 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': 'cont...@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': 'cont...@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': 'ja...@example.com', > > + 'advertised': 'True', > > 'description': 'The Most Desirable organization', > > - 'contact_address': 'cont...@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' : 'pran...@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'] = 'pran...@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 > You are the owner of lp:~godricglow/postorius/tests_forms. > -- *Pranjal Yadav* *IIT Kharagpur* https://code.launchpad.net/~godricglow/postorius/tests_forms/+merge/251740 Your team Mailman Coders is subscribed to branch lp:postorius. _______________________________________________ Mailman-coders mailing list Mailman-coders@python.org https://mail.python.org/mailman/listinfo/mailman-coders
