522251 - In the review process, we should allow choices when marking invalid.

Status: RESOLVED FIXED

(Webtools :: ISPDB Server, defect, P1)

Description

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

Some sample choices, in addition to a freeform textfield, could be:
"Not available from our domain", or "Incorrect data".
(Perhaps we should have an "Other" choice, which enables the textfield, too.)

Comment 1

[Eric Henry]

I'll take this one.

Comment 2

[Eric Henry]

Attachment: fix (11.3.09)

Comment 3

[Eric Henry]

Attachment: fix (11.17.09)

now with added tests!

Comment 4

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

Comment on attachment 412895 [details] [diff] [review]
fix (11.17.09)

>diff --git a/.hgignore b/.hgignore
>new file mode 100644
>--- /dev/null
>+++ b/.hgignore

I don't think you want this as part of your diff.  :)

(I tend to hand-edit my diffs after I've created them, in cases like this.)

>+++ b/config/models.py
>@@ -153,16 +153,26 @@ class Config(models.Model):
>+class ApprovalComment(models.Model):
>+    user = models.CharField(max_length=100)
>+    status = models.CharField(max_length=7)
>+    comment = models.CharField(max_length=255)
>+    created_datetime = models.DateTimeField(auto_now_add=True)
>+    
>+    config = models.ForeignKey('Config', related_name="comments")
>+    

There are some trailing spaces on the two blank lines above.

>+    def __str__(self): return self.name

Should we be returning self.name or self.comment here?

>+++ b/config/views.py

(There are some more trailing spaces here, too.)

>@@ -1,11 +1,11 @@
>-from ispdb.config.models import Config, ConfigForm, Domain, DomainForm, UnclaimedDomain
>+from ispdb.config.models import Config, ConfigForm, Domain, DomainForm, UnclaimedDomain, ApprovalComment
> from django.shortcuts import get_object_or_404, render_to_response

I prefer to have one import per line, but since the rest of the code doesn't follow that, we might as well leave it.

>@@ -130,18 +130,34 @@ def approve(request, id):
>-        # XXX do something w/ the comment text

I love that you're removing this comment.  :)

>+        commentChoice = data.get('comment','')
>+        
>+        if commentChoice == 'Other - invalid':
>+            comment = data.get('comment_invalid','')
>+        elif commentChoice == 'Other - valid':
>+            comment = data.get('comment_valid','')

I'ld prefer it if we used "s instead of 's.

>+        else:
>+            comment = commentChoice
>+        
>+        comment = ApprovalComment(comment=comment, config=config, status=status, user=request.user.username)

We should break this line at 80 characters.  Maybe like:
        comment = ApprovalComment(comment=comment, config=config, status=status,
                                  user=request.user.username)

>diff --git a/ispdb b/ispdb
>deleted file mode 120000

Woo!  Did we all decide that we don't need this file anymore?

>--- a/ispdb
>+++ /dev/null
>@@ -1,1 +0,0 @@
>-.
>\ No newline at end of file

BTW, this didn't apply cleanly, but I know what you mean there.

>+++ b/templates/config/details.html
>@@ -10,17 +10,17 @@
>   $("#add").click(function() {
>     $(this).hide();
>     $(this).parent().children('.comment_form').show('fast');
>   })
>-
>+  

Ooh, seriously?  You just added trailing spaces there?  :)

>@@ -31,64 +31,76 @@
>   {% if config.approved %}
>-  <div>This configuration is APPROVED.</div>
>+  <div><p><span style="color: green; font-weight: bold;">This configuration is APPROVED.</span></p></div>

Instead of putting styles in the HTML, what about having:
<span class="approved">, and putting a span.approved style in the css?

>       {% if perms.config.can_approve %}
>         <div>Is there a reason it shouldn't be?</div>
>         <form action="{% url ispdb_approve id=config.id %}" method="POST">
>-          <div>
>-            <input type="text" size="100" name="comment" value="Always enter a comment here"></input>
>+          <div style="margin: 0.8em;">
>+            <input type="radio" name="comment" value="Not available from our domain" checked="checked"/> Not available from our domain<br />
>+            <input type="radio" name="comment" value="Incorrect data"/> Incorrect data<br />
>+            <input type="radio" name="comment" value="Other - invalid"/> Other &raquo; <input type="text" size="70" name="comment_invalid" value=""/>

I think we can wrap these lines at 80 characters, and make the div "class=approvalComments" instead of "style=margin:0.8em".

>+      <p><span style="color: red; font-weight: bold;">This configuration has been determined INVALID.</span><br />

Ditto with the class comment above.

>+      We've looked at the data, and either it's not correct, or it can't be verified from outside the ISP's firewall, or the ISP has asked not to publish it.</p>

And the wrapping at 80 characters.

>-            <input type="text" size="100" name="comment" value="Always enter a comment here"></input>
>+            <input type="text" size="70" name="comment" value="Always enter a comment here"></input>

Why did we bump this down from 100 to 70?

>+++ b/tests/test_approve.py

This file has a whole bunch of ^Ms at the end when I look at it.  We should probably get rid of them.

>@@ -0,0 +1,132 @@
>+    def test_simple(self):
>+        request = HttpRequest()
>+             
>+        result = views.approve(request, "1")

In https://bugzilla.mozilla.org/attachment.cgi?id=411899&action=diff Jay has been using self.client.post instead of calling the view directly.  I think I would prefer that, since it feels to me like it will test more of the end-to-end path.

>+    def test_post_no_user(self):
>+        try:
>+            result = views.approve(request, "1")
>+            raise Exception("request.user was not specified, but did NOT fail")
>+        except AttributeError:
>+            pass

I think we can do this with:
  @raises(AttributeError)
which might be nicer.

Finally, it would be nice to see how well we handled international comments.  My favourite string to use is u"Iñtërnâtiônàlizætiøn", but don't forget to add the "# -*- coding: utf-8 -*-" line at the top of the file, and save it in utf-8.

Thanks,
Blake.

Comment 5

[Eric Henry]

> >-            <input type="text" size="100" name="comment" value="Always enter a comment here"></input>
> >+            <input type="text" size="70" name="comment" value="Always enter a comment here"></input>
> 
> Why did we bump this down from 100 to 70?

I thought it looked nicer...

Comment 6

[Eric Henry]

Attachment: fix (11.23.09)

Next attempt.  Should fix all issues listed by bwinton

Comment 7

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

Comment on attachment 414181 [details] [diff] [review]
fix (11.23.09)

>+++ b/config/models.py
>@@ -32,17 +32,17 @@ class UnclaimedDomain(models.Model):
>+class ApprovalComment(models.Model):
>+    user = models.CharField(max_length=100)

If this is meant to be a user, maybe we could make it a User somehow?  Something like:
user = models.ForeignKey(User, related_name=’comments’)
maybe?

>+    status = models.CharField(max_length=7)

What are the possible values this could take?  Should we perhaps make it a ChoiceField instead?

>+    comment = models.CharField(max_length=255)
>+    created_datetime = models.DateTimeField(auto_now_add=True)
>+
>+    config = models.ForeignKey('Config', related_name="comments")

Should that be 'Config' or Config?
Also, please use "s instead of 's.

>+    def __str__(self): return self.comment

I've seen a few places that recommend overriding __unicode__ instead of __str__, but I don't think we do that anywhere else.  At least, not yet.


>+++ b/config/views.py
>@@ -130,18 +130,40 @@ def approve(request, id):
>     config = Config.objects.filter(id=id)[0]
>     if request.method == 'POST': # If the form has been submitted...
>         data = request.POST
>+        status = ""
>         if data.get('approved', False):
>+            status = "Valid"
>+            config.invalid = False
>             config.approved = True
>         elif data.get('denied', False):
>+            status = "Invalid"
>             config.invalid = True
>             config.approved = False
>         else:
>             raise ValueError, "shouldn't get here"

So, it looks like config.invalid is always the opposite of config.approved.  Is that true?
Also, status looks like it depends on config.invalid (or config.approved), so do we need it?

>+++ b/fixtures/xml_testdata.json

I think I might prefer it if you put these changes into "approve.json", because they're being used by the ApproveTest.

>+++ b/media/css/details.css
>@@ -1,8 +1,26 @@
>+.approved {
>+    color: green;
>+    font-weight: bold;
>+}

This rule interacts poorly with the line:
>+++ b/templates/config/details.html
>@@ -26,69 +26,101 @@
> <div class="configstatus {% if config.approved %} approved {% else %} pending {% endif %}">

(If the config is approved, then everything in it is in green.)

>+++ b/templates/config/details.html
>@@ -26,69 +26,101 @@
>-{% ifnotequal config.display_name config.display_short_name %} ({{config.display_short_name}} for short) {% endifnotequal %}
>-</h1>
>+{% ifnotequal config.display_name config.display_short_name %}
>+({{config.display_short_name}} for short)
>+{% endifnotequal %}</h1>

I love this change.  What do you think about indenting the middle line by two spaces?

>+            <input type="radio" name="comment"
>+            value="Not available from our domain" checked="checked"/>

I think this line should be indented by another two spaces.

>     <div>
>-      This configuration has been determined INVALID.  We've looked at the
>-      data, and either it's not correct, or it can't be verified from outside
>-      the ISP's firewall, or the ISP has asked not to publish it.
>+      <p><span class="denied">This configuration has been determined INVALID.

Why did you add the extra <p> here?

>-            <input type="text" size="100" name="comment" value="Always enter a comment here"></input>
>+            <input type="text" size="70" name="comment"
>+            value="Always enter a comment here" />

Ditto with the two more spaces.

>-        Can you confirm that this is a good configuration according to our <a href="{% url ispdb_policy %}">policies</a>?
>+        <p><b>Can you confirm that this is a good configuration according to our

Ditto with the extra <p>.

>+  {% if config.comments.all %}
>+      <h4>Approval History</h4>
>+      <div class="approvalComments">
>+      {% for comment in config.comments.all reversed %}
>+        <h5>{{ comment.status }} [{{ comment.created_datetime|date:"M j Y,  H:i O" }}]</h5>

Having said what I said above, it does look kinda neat.  Perhaps we should put the comment.status in <span class="{{ comment.status }}>{{ comment.status }}</span>, so that we can see when it switched from approved to invalid…

>+++ b/tests/test_approve.py
>@@ -0,0 +1,145 @@
>+# -*- coding: utf-8 -*-

That's some strange stuff at the start of this file.
Did you insert a UTF-8 BOM by mistake?

>+from django.test import TestCase
>+from django.test.client import Client
>+from django.http import HttpRequest, QueryDict
>+from django.contrib.auth.models import User
>+from ispdb.config import views, models
>+from ispdb.config.models import Config
>+
>+import nose.tools

I think these should be in a slightly different order:
-----------------
from django.contrib.auth.models import User
from django.test import TestCase
from django.test.client import Client

import nose.tools

from ispdb.config import views
from ispdb.config import models
-----------------

>+class ApproveTest(TestCase):
>+    fixtures = ['xml_testdata']
>+
>+    def test_simple(self):
>+        client = Client()
>+             

Trailing spaces...  :)

>+        response = client.get("/approve/1");
>+        
>+        print response

I don't think we actually want to print the response here.
But, if you changed that to "nose.tools.set_trace()", then when you ran the test, it would drop you into a debugger (called "pdb") from which you could print out various things.

>+        assert response['Location'].count('/details/1')

We can also check the response.template[0].name to see if you got the right template, and response.context[0] to see the data that was passed in.

Oh, and I think we get better failure messages if we use nose.tools.assert_equal instead of just assert.

>+        #there should only be the comment we made
>+        assert config.comments.count() == 1

Do you have a test for more than one comment, and the order they're returned in?

>+        config = Config.objects.filter(id=1)[0]
>+        assert config.invalid == False

It would also be nice to use self.client.get(details) to see what comes back.

>+++ b/urls.py
>@@ -10,27 +10,27 @@ unclaimed_dict = {
>     'queryset': UnclaimedDomain.objects.all(),
> }
> 
> 
> urlpatterns = patterns('',
>     (r'^admin/(.*)', admin.site.root),
>     (r'^comments/', include('django.contrib.comments.urls')),
>     (r'^openid/', include('django_openid_auth.urls')),
>-    url(r'^logout/', 'ispdb.config.views.logout_view', name='ispdb_logout'),
>-    url(r'^admin_login/', 'ispdb.config.views.admin_login', name="ispdb_login"),
>-    url(r'^$', 'ispdb.config.views.intro', name='ispdb_index'),
>-    url(r'^list', 'ispdb.config.views.list', name='ispdb_list'),
>-    url(r'^details/(?P<id>\d*)', 'ispdb.config.views.details', name='ispdb_details'),
>-    url(r'^export_xml/(?P<id>\d*)', 'ispdb.config.views.export_xml', name='ispdb_export_xml'),
>-    url(r'^add/(?P<domain>.*)', 'ispdb.config.views.add', name='ispdb_add'),
>-    url(r'^add/', 'ispdb.config.views.add', name='ispdb_add'),
>-    url(r'^queue$', 'ispdb.config.views.queue', name='ispdb_queue'),
>-    url(r'^policy$', 'ispdb.config.views.policy', name='ispdb_policy'),
>-    url(r'^approve/(?P<id>\d*)', 'ispdb.config.views.approve', name='ispdb_approve'),
>+    url(r'^logout/',                'ispdb.config.views.logout_view', name='ispdb_logout'),
>+    url(r'^admin_login/',           'ispdb.config.views.admin_login', name="ispdb_login"),
>+    url(r'^$',                      'ispdb.config.views.intro',       name='ispdb_index'),
>+    url(r'^list',                   'ispdb.config.views.list',        name='ispdb_list'),
>+    url(r'^details/(?P<id>\d*)',    'ispdb.config.views.details',     name='ispdb_details'),
>+    url(r'^export_xml/(?P<id>\d*)', 'ispdb.config.views.export_xml',  name='ispdb_export_xml'),
>+    url(r'^add/(?P<domain>.*)',     'ispdb.config.views.add',         name='ispdb_add'),
>+    url(r'^add/',                   'ispdb.config.views.add',         name='ispdb_add'),
>+    url(r'^queue$',                 'ispdb.config.views.queue',       name='ispdb_queue'),
>+    url(r'^policy$',                'ispdb.config.views.policy',      name='ispdb_policy'),
>+    url(r'^approve/(?P<id>\d*)',    'ispdb.config.views.approve',     name='ispdb_approve'),
> )

Those are _way_ longer than 80 characters, and I don't really think they look any better with all the extra spaces.  Please revert this change.

I think this patch is very close to being ready.  Thank you for your hard work on it.

Later,
Blake.

Comment 8

[Eric Henry]

(In reply to comment #7)
> (From update of attachment 414181 [details] [diff] [review])
> >+++ b/config/models.py
> >@@ -32,17 +32,17 @@ class UnclaimedDomain(models.Model):
> >+class ApprovalComment(models.Model):
> >+    user = models.CharField(max_length=100)
> 
> If this is meant to be a user, maybe we could make it a User somehow? 
> Something like:
> user = models.ForeignKey(User, related_name=’comments’)
> maybe?

Actually, all I wanted there was the username so you could see who made the comment.

> 
> >+    def __str__(self): return self.comment
> 
> I've seen a few places that recommend overriding __unicode__ instead of
> __str__, but I don't think we do that anywhere else.  At least, not yet.
> 

I went ahead and left this one as is, so we have more consistency (I have also seen those recommendations).

Comment 9

[Eric Henry]

Attachment: fix (11.29.09)

Comment 10

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

Comment on attachment 415037 [details] [diff] [review]
fix (11.29.09)

I believe you've got a new diff for me for this bug…

(If I'm wrong, reset the r- to an r?, and I'll review it ASAP.)

Thanks,
Blake.

Comment 11

[Eric Henry]

Attachment: patch (2010-06-11)

this patch should be applied after the patch for bug 530721.

Comment 12

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

Comment on attachment 450577 [details] [diff] [review]
patch (2010-06-11)

>+++ b/ispdb/config/models.py	Thu Jun 10 23:28:06 2010 -0400
>@@ -65,7 +65,7 @@
>-  invalid = models.BooleanField(default=False)
>+  pending = models.BooleanField(default=True)

So, I don't mind this change, but models.py still refers to the "invalid" field, and details.html still uses "config.invalid", as does list.html.

>@@ -154,6 +154,16 @@
>+class ApprovalComment(models.Model):
>+    user = models.CharField(max_length=100)
> If this is meant to be a user, maybe we could make it a User somehow? 
> Something like:
> user = models.ForeignKey(User, related_name=’comments’)
> maybe?
Actually, all I wanted there was the username so you could see who made the
comment.

Sure, but if you make it an actual user, you can grab the name, but then we can also link the user to all the comments they made, which might be interesting information.  :)  And we're not going to let people without User objects in the system approve or deny comments…

>+    def __str__(self): return self.comment
>+ 

There's a trailing space on this line.

>+++ b/ispdb/config/views.py	Thu Jun 10 23:28:06 2010 -0400
>@@ -167,8 +167,8 @@
>-    pending_configs = Config.objects.filter(approved=False, invalid=False)
>-    invalid_configs = Config.objects.filter(invalid=True)
>+    pending_configs = Config.objects.filter(approved=False, pending=False)
>+    invalid_configs = Config.objects.filter(pending=True)

So, I would have thought that pending_configs would really have the pending attribute set to True.  And invalid_configs could have the approved attribute set to False.

>@@ -185,13 +185,31 @@
>     config = Config.objects.filter(id=id)[0]
>     if request.method == 'POST': # If the form has been submitted...
>         data = request.POST
>+        config.pending = False
[snip…]
>             raise ValueError, "shouldn't get here"
[snip…]
>         config.save()

Do we want to set config.pending this early?  What if there's an error in the input?  (I guess then the config wouldn't get saved, so it might be okay.)

>+++ b/ispdb/fixtures/approval_testdata.json	Thu Jun 10 23:28:06 2010 -0400

I see a bunch of dos-style newlines in this file.  Please remove them.

>+++ b/ispdb/templates/config/details.html	Thu Jun 10 23:28:06 2010 -0400
>@@ -31,59 +31,97 @@
>-  <div>This configuration is APPROVED.</div>
>+  <div><span class="approvedText">This configuration is APPROVED.</span><p/>

You could just put the class on the div, and save yourself an element.  ;)

>+          <div class="commentChoices">
>+            <input type="radio" name="comment"
>+              value="Not available from our domain" checked="checked"/>
>+            Not available from our domain<br />
>+            <input type="radio" name="comment" value="Incorrect data"/>
>+            Incorrect data<br />
>+            <input type="radio" name="comment" value="Other - invalid"/>
>+            Other &raquo;
>+            <input type="text" size="70" name="comment_invalid" value=""/>
>           </div>

So, there's no way to get this list of choices from the model?  Or would you like to file a separate bug for that?  :)

>+++ b/ispdb/tests/test_approve.py	Thu Jun 10 23:28:06 2010 -0400
>@@ -0,0 +1,137 @@

dos-style newlines here, too.

So, other than that, I think I'm happy.  Fix those things, and let's see what bugs the second reviewer finds.  :)

(But, to be fair, I'm just running the tests now, and so I might switch this to an r-, if they fail.)

Thanks,
Blake.

Comment 13

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

Well, it looks like they failed, a lot.

But you didn't add any failures, so I guess that's a win?

Thanks,
Blake.

Comment 14

[Eric Henry]

I'm assuming this is fixed from my patch, and that checkin is actually ready...