Closed Bug 522250 Opened 10 years ago Closed 10 years ago

New config entry validation.

Categories

(Webtools :: ISPDB Server, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0rc1

People

(Reporter: bwinton, Assigned: jschmidek)

Details

Attachments

(1 file, 4 obsolete files)

There are a set of things which could go wrong when people enter new config entries, for instance, the domain could already be in the DB, or the values in the config could be wrong.

We should perform some validation of the configs as people enter them.
Flags: blocking-thunderbird3?
I'll take on this bug.
Assignee: nobody → jschmidek
ispdb as a whole isn't a release blocker for TB3 (we can hand-edit XML files)
Flags: blocking-thunderbird3? → blocking-thunderbird3-
Attached patch WIP config entry validation (obsolete) — Splinter Review
Added validation for port numbers and for uniqueness of domain names. Everything is checked when the form is submitted. However, for the domain name the user would probably want to know if it's already in the db before they enter all the information for it; when they press "I think so". I could make an ajax call there to check that before the rest of the form is shown but then there would be a delay. I'm not sure exactly what is desired here so feedback is appreciated.
Attachment #406974 - Flags: review?(bwinton)
Attached patch config entry validation patch (obsolete) — Splinter Review
Attachment #406974 - Attachment is obsolete: true
Attachment #411810 - Flags: review?(bwinton)
Attachment #406974 - Flags: review?(bwinton)
Comment on attachment 411810 [details] [diff] [review]
config entry validation patch

>Index: config/models.py
>@@ -1,10 +1,16 @@
> from django.db import models
>-from django.forms import ModelForm, RadioSelect, ChoiceField
>+from django.contrib import admin
>+from django.utils.safestring import mark_safe
>+from django.core.urlresolvers import reverse
>+from django.forms import ModelForm
>+from django.forms import RadioSelect
>+from django.forms import ChoiceField
>+from django.forms import ValidationError
> import ispdb.audit as audit

It would be nice to alphabetize these.  (utils and db are out of place, I believe.)

>@@ -37,7 +43,7 @@
>   """
>-  
>+

Nice.  I hates me some trailing whitespace.  :)

>@@ -114,7 +120,7 @@
>-  incoming_port = models.IntegerField(max_length=5)
>+  incoming_port = models.PositiveIntegerField()

So, why did we lose the max_length here?

>@@ -126,9 +132,9 @@
>-  outgoing_port = models.IntegerField(max_length=5)
>+  outgoing_port = models.PositiveIntegerField()

And here.

> class DomainForm(ModelForm):
>+    def clean_name(self):
>+        data = self.cleaned_data["name"]
>+        dom = Domain.objects.filter(name=data)
>+        if dom:
>+            msg = "Domain configuration already exists <a href=\""\
>+            + reverse('ispdb_details',kwargs={'id':dom[0].config.id})\
>+            + "\">here</a>."

I think I would prefer this to be:
            msg = 'Domain configuration already exists <a href="%s">here</a>.' %
            (reverse("ispdb_details", kwargs={"id" : dom[0].config.id}),)


>+            raise ValidationError(mark_safe(msg))
>+        return data

>+def clean_port(self,field):
>+        data = self.cleaned_data[field]
>+        if data > 99999:
>+            raise ValidationError("Port number cannot be larger than 99999")
>+        return data

http://www.iana.org/assignments/port-numbers says:
The Well Known Ports are those from 0 through 1023.
The Registered Ports are those from 1024 through 49151
The Dynamic and/or Private Ports are those from 49152 through 65535

So perhaps we should use 65535 as the number to check against.

And the method has too much indentation.

>Index: config/views.py
>@@ -88,7 +100,8 @@
>-            if config_form.is_valid(): # All validation rules pass
>+            formset = DomainFormSet(request.POST,request.FILES)
>+            if config_form.is_valid() and formset.is_valid(): # All validation rules pass

I think we should move this comment up above the line, to keep it under 80 characters wide.

>Index: templates/config/enter_config.html
>@@ -34,16 +42,33 @@
>+      $("#yes,#no").attr("disabled","disabled");

Why did you put two "disabled"s here?

>+      $.ajax({
>+        url: "{%url ispdb_check_domain%}/"+$("#id_form-0-name").val(),

Spaces around the +, please.

>+        type: "GET",
>+        dataType: 'json',
>+        success: function(data){
>+            // no error messages
>+            if(!data.name){
>+                showForm();
>+            }else{
>+                $(".errorlist").remove();
>+                list = $("<ul class='errorlist'></ul>");
>+                list.append("<li>"+data.name+"</li>");
>+                $("#domain_list td").prepend(list);
>+            }
>+        },

Add spaces before the {s, please.
And strangely enough, the else goes on its own line, -ish.
The coding style for this would be
            if (!data.name) {
                showForm();
            }
            else {
                $(".errorlist").remove();

Uh, except we should also be using 2-space indents, not 4.

>     $("#no").click(function(event) {
>-      // the yes button is in a form, and we want to stop the
>+      // the no button is in a form, and we want to stop the

Good catch.  :)

Thanks,
Blake.
Attachment #411810 - Flags: review?(bwinton) → review-
Oh, and where are the tests?
Status: NEW → ASSIGNED
(In reply to comment #5)
> >@@ -114,7 +120,7 @@
> >-  incoming_port = models.IntegerField(max_length=5)
> >+  incoming_port = models.PositiveIntegerField()
> 
> So, why did we lose the max_length here?
> 
> >@@ -126,9 +132,9 @@
> >-  outgoing_port = models.IntegerField(max_length=5)
> >+  outgoing_port = models.PositiveIntegerField()
> 
> And here.
> 
IntegerField doesn't have a max_length option so it wasn't doing anything.
(In reply to comment #7)
> (In reply to comment #5)
> > >@@ -114,7 +120,7 @@
> > >-  incoming_port = models.IntegerField(max_length=5)
> > >+  incoming_port = models.PositiveIntegerField()
> > So, why did we lose the max_length here?
> IntegerField doesn't have a max_length option so it wasn't doing anything.

Well, that's a pretty good answer.  :)
(In reply to comment #5)
> >Index: templates/config/enter_config.html
> >@@ -34,16 +42,33 @@
> >+      $("#yes,#no").attr("disabled","disabled");
> 
> Why did you put two "disabled"s here?

I'm setting the disabled attribute on the buttons to "disabled" :) You can probably do .attr("disabled", true) but the jQuery docs seem to support doing it this way so I've stuck with it. http://docs.jquery.com/Frequently_Asked_Questions#How_do_I_disable.2Fenable_an_element.3F
Attached patch updated patch, with tests (obsolete) — Splinter Review
Attachment #411810 - Attachment is obsolete: true
Attachment #417863 - Flags: review?(bwinton)
Comment on attachment 417863 [details] [diff] [review]
updated patch, with tests

First off, there still seem to be a bunch of ^M's in the file.  (I can work around them, but they make my life a little harder.  :)

And I think I'm happy with the rest of it.

Let's see what the next reviewer asks for.  :)

Thanks,
Blake.
Attachment #417863 - Flags: review?(bwinton) → review+
Where are the ^M's ? I wasn't aware there were any.
They're mostly on the svn diff lines, which I think we can ignore.

Aside from that, tests/test_check_domain.py has one on every line, and that's all I can see.

Thanks,
Blake.
Attached patch latest svn patch (obsolete) — Splinter Review
Updated to work with the latest svn revision.
Attachment #417863 - Attachment is obsolete: true
Attachment #418591 - Flags: review?(david.ascher)
Comment on attachment 418591 [details] [diff] [review]
latest svn patch

>Index: config/models.py
>+    def clean_incoming_port(self):
>+        return clean_port(self,'incoming_port')
>+    def clean_outgoing_port(self):
>+        return clean_port(self,'outgoing_port')
>+

We're using " for strings in these files, right?


>Index: config/views.py

>+# tests if the given domain name is valid, it is not already in the db

For clarity, can you replace /, / with / and that /? 

Any reason not to put that as a function docstring?


Look good, haven't tested it.
Attachment #418591 - Flags: review?(david.ascher) → review+
Attachment #418591 - Attachment is obsolete: true
Keywords: checkin-needed
Checked in as http://viewvc.svn.mozilla.org/vc?view=revision&revision=58707
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Component: ispdb → ISPDB Server
Product: Mozilla Messaging → Webtools
You need to log in before you can comment on or make changes to this bug.