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

RESOLVED FIXED

Status

Webtools
ISPDB Server
--
major
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: BenB, Assigned: Jay Schmidek)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
It works up to 7-9 or so domains.
(Assignee)

Comment 2

8 years ago
Created attachment 411826 [details] [diff] [review]
testcase confirming bug

I'll look into why this is happening.
Attachment #411826 - Flags: review?(bwinton)
(Assignee)

Comment 3

8 years ago
Created attachment 411830 [details] [diff] [review]
fix for bug

That was easy, add view was only looking at the first character of the number of forms.
Attachment #411830 - Flags: review?(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.
(Assignee)

Comment 5

8 years ago
Created attachment 411899 [details] [diff] [review]
combined, updated patch
Attachment #411826 - Attachment is obsolete: true
Attachment #411830 - Attachment is obsolete: true
Attachment #411899 - Flags: review?(bwinton)
Attachment #411826 - Flags: review?(bwinton)
Attachment #411830 - Flags: review?(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.
Attachment #411899 - Flags: review?(bwinton) → review+
You appear to be working on this one, so I'm going to assign it to you.
Assignee: nobody → jschmidek
Status: NEW → ASSIGNED
(Assignee)

Comment 8

8 years ago
Created attachment 414152 [details] [diff] [review]
updated patch
Attachment #411899 - Attachment is obsolete: true
Attachment #414152 - Flags: review?(david.ascher)
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).
Attachment #414152 - Flags: review?(david.ascher) → review+
(Assignee)

Comment 10

8 years ago
Created attachment 417735 [details] [diff] [review]
good patch

Added a comment
Attachment #414152 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Checked in as: http://viewvc.svn.mozilla.org/vc?view=revision&revision=57910

Congrats!
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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.