526650 - Can't add more than 8-9 domains per entry

Status: RESOLVED FIXED

(Webtools :: ISPDB Server, defect)

Description

[Ben Bucksch (:BenB)]

Reproduction:
1. Add a new configuration
2. Add 15 domains to it, by clicking the "add another domain" link
   14 times and entering different domains in the fields
3. Submit
4. Go to "List"

Actual result:
Your config is listed but only with one domain, in the list and in the details.

Expected result:
All domains added, with 1 main domain and 14 additional domains.

Comment 1

[Ben Bucksch (:BenB)]

It works up to 7-9 or so domains.

Comment 2

[Jay Schmidek]

Attachment: testcase confirming bug

I'll look into why this is happening.

Comment 3

[Jay Schmidek]

Attachment: fix for bug

That was easy, add view was only looking at the first character of the number of forms.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 411826 [details] [diff] [review]
testcase confirming bug

>Index: tests/test_add.py
>+        res = self.client.post(reverse('ispdb_add'),
>+                         {'asking_or_adding':'adding',
>+                          'form-TOTAL_FORMS':str(num_domains),
>+                          'form-INITIAL_FORMS':'1',
>+                          'form-0-name':'test0.com',
>+                          'form-1-name':'test1.com',
>+                          'form-2-name':'test2.com',
>+                          'form-3-name':'test3.com',
>+                          'form-4-name':'test4.com',
>+                          'form-5-name':'test5.com',
>+                          'form-6-name':'test6.com',
>+                          'form-7-name':'test7.com',
>+                          'form-8-name':'test8.com',
>+                          'form-9-name':'test9.com',
[...]
>+                          'problems':'0'})

I think you could replace this with something like:
domain_form = {"asking_or_adding":"adding",
               "form-TOTAL_FORMS":str(num_domains),
               "form-INITIAL_FORMS':"1",
               [...]
               "problems":"0"}
for i in range(num_domains):
    domain_form["form-%d-name" % i] = "test%d.com" % i
res = self.client.post(reverse('ispdb_add'), domain_form)

And even better, what if you added a "populate_domain_form", which created the dictionary with some decent sample values, and then you could just override the ones that you were interested in for that test?

>+for i in range(num_domains):
>+  assert_true(models.Domain.objects.filter(name="test"+str(i)+".com"))

What about:
assert_true(models.Domain.objects.filter(name="test%d.com"%i))
instead?

Also, if you could put the two patches in one, that would be cool.  :)

Finally, what happens if someone enters "1000000" in the num_domains field?  Or "aaa"?  (Or "09"?  That's a tricky one sometimes.)

Thanks,
Blake.

Comment 5

[Jay Schmidek]

Attachment: combined, updated patch

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 411899 [details] [diff] [review]
combined, updated patch

>Index: config/views.py
>-        num_domains = int(data['form-TOTAL_FORMS'][0])
>+        num_domains = int(data['form-TOTAL_FORMS'])

Heh.  Good catch.

>         for i in range(num_domains):

I think we might want to assert that the number of domains is less than some value, so that we don't get some joker trying to add 10000000 domains, and hanging the server.  If you want to fix that in a separate bug, that would be okay with me.

>Index: tests/test_add.py
>@@ -6,293 +6,203 @@
>+success_code = 302
>+fail_code = 200

So, normally I would expect 200 to be a success, and 302 to be some sort of redirect.  Why is 200 a fail?  (I think I know why, but we should add a comment.)

Also, I think we might want to import httplib, and use httplib.FOUND and httplib.OK, to show that these are http status codes we're talking about.

> class AddTest(TestCase):
>     def test_ask(self):
>-        self.client.post(reverse('ispdb_add'),{'asking_or_adding':'asking',
>-                                               'form-TOTAL_FORMS':'1',
>-                                               'form-INITIAL_FORMS':'1',
>-                                               'form-0-name':'ty.com'})
>-        domain = models.UnclaimedDomain.objects.get(name='ty.com')
>+        asking_form = asking_domain_form()
>+        self.client.post(reverse("ispdb_add"), asking_form)

Since you're only using the asking_form here, you could inline that call, to:
self.client.post(reverse("ispdb_add"), asking_domain_form())

>+        assert_equal(domain.votes,1);

We don't usually put ;s at the end of Python statements.  There are 5 of them that I see.  (And I do the same thing, when I switch to Python from Java or Javascript…)

>         assert_equal(domain.votes,2);

Since you're changing these, it would be nice to have a space after the ,.

>     def test_add_internationalization(self):
>-        name = u'Iñtërnâtiônàlizætiøn'
>+        name = u"Iñtërnâtiônàlizætiøn"

Nice.

>+    def test_add_lots_of_domains(self):
>+        num_domains = 10

Yeah?  10 is "lots"?  :)
(It's fine, you don't need to change it.)

>+def asking_domain_form():
>+    return {"asking_or_adding":"asking",
>+            "form-TOTAL_FORMS":"1",
>+            "form-INITIAL_FORMS":"1",
>+            "form-0-name":"test.com"}
>+def adding_domain_form():

We could use a blank like before this function.

created_datetime=datetime.datetime.now())
>         config.save()
>-        d = models.Domain(name='test',votes=1,config=config)
>+        d = models.Domain(name="test",votes=1,config=config)

We could use some spaces after these commas.

So, other than the small things I've mentioned, I'm happy with this.  Your next step is to fix those, create a new patch, and ask, say, davida to review that one.

Later,
Blake.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

You appear to be working on this one, so I'm going to assign it to you.

Comment 8

[Jay Schmidek]

Attachment: updated patch

Comment 9

[David Ascher (:davida)]

Comment on attachment 414152 [details] [diff] [review]
updated patch

I suggest some comment in the test_add_09_domains to point out why this test is relevant.  (In particular, I suspect it's testing correct handling of the leading 0, which can trigger bugs due to octal notation in some systems).

Comment 10

[Jay Schmidek]

Attachment: good patch

Added a comment

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Checked in as: http://viewvc.svn.mozilla.org/vc?view=revision&revision=57910

Congrats!