508364 - Site page

Status: RESOLVED FIXED

(Webtools Graveyard :: Cesium, defect)

Description

[Ryan Doherty (:rdoherty)]

make changes to the site detail page per the spec.

http://people.mozilla.com/~chowse/drop/cesium/HiFid_Site.png

Remove:
* Page stats table
* Pass/fail images and text
* Load time

Comment 1

[Ryan Doherty (:rdoherty)]

Attachment: v1

let the reviewing begin!

Comment 2

[Ryan Doherty (:rdoherty)]

Attachment: v2

rebased

Comment 3

[Jeff Balogh (:jbalogh)]

Comment on attachment 402215 [details] [diff] [review]
v2

Overall, this looks pretty good.  Nice work.


>diff --git a/autoyslow/models.py b/autoyslow/models.py
>index 847ef57..5286ae0 100644
>--- a/autoyslow/models.py
>+++ b/autoyslow/models.py
>@@ -94,11 +94,52 @@ class Site(models.Model):
>             pages = self.page_set.filter(last_testrun__gte=self.last_testrun)
>             tests = Test.objects.filter(page__in=pages)
>             return int(tests.aggregate(models.Avg('score'))['score__avg'])
>+    
>+    @property
>+    def avg_page_weight(self):

Can we make the naming consistent with the score property above?

>+        if self.last_testrun is None:
>+            return 0
>+        else:
>+            pages = self.page_set.filter(last_testrun__gte=self.last_testrun)
>+            tests = Test.objects.filter(page__in=pages)

These 5 lines are in the score, weight, and request averaging functions.  Can we move them to their own function?

The helper function could probably take the name of the field you want to aggregate and go from there.

>+            return int(tests.aggregate(models.Avg('weight'))['weight__avg']/1024)
>+
>+    @property
>+    def avg_http_requests(self):
>+        if self.last_testrun is None:
>+            return 0
>+        else:
>+            pages = self.page_set.filter(last_testrun__gte=self.last_testrun)
>+            tests = Test.objects.filter(page__in=pages)
>+            return int(tests.aggregate(models.Avg('requests'))['requests__avg'])
> 
>     @property
>     def next_testrun(self):
>         return next_after(self.last_testrun)
> 
>+    def test_average(self, days=14):
>+       """Aggregates test data over the last N days, grouped by day."""
>+
>+       lower_bound = datetime.now() - timedelta(days=days)
>+       q = Test.objects.filter(page__in=self.page_set.all(),
>+                               time__gte=lower_bound)
>+
>+       # Add the grouped field to the SELECT for convenience.
>+       time_field = 'DATE(%s.time)' % Test._meta.db_table
>+       q = q.extra(select={'test_date': time_field})
>+
>+       # We're messing with undocumented internals here, so don't cry too hard
>+       # if it breaks.  The ORM currently turns this into 'GROUP BY DATE(..)'.
>+       q.query.group_by = [time_field]
>+
>+       # The Test data we're averaging.
>+       averages = {'score_avg': models.Avg('score'),
>+                   'weight_avg': models.Avg('weight'),
>+                   'requests_avg': models.Avg('requests')}
>+
>+       return [(x.test_date, dict((k, getattr(x, k)) for k in averages))
>+               for x in q.annotate(**averages)]

That's hot.

> 
> class Page(models.Model):
>     # 767 bytes is the max key length for an InnoDB unique constraint.
>@@ -130,6 +171,20 @@ class Page(models.Model):
>             return 0
> 
>     @property
>+    def weight(self):
>+        try:
>+            return self.test_set.latest('time').weight
>+        except Test.DoesNotExist:
>+            return 0
>+
>+    @property
>+    def requests(self):
>+        try:
>+            return self.test_set.latest('time').requests
>+        except Test.DoesNotExist:
>+            return 0

Eek, more duplication!

>+
>+    @property
>     def next_testrun(self):
>         return next_after(self.last_testrun)
> 
>diff --git a/autoyslow/templates/autoyslow/site_detail.html b/autoyslow/templates/autoyslow/site_detail.html
>index 81d2e2c..83eab11 100644
>--- a/autoyslow/templates/autoyslow/site_detail.html
>+++ b/autoyslow/templates/autoyslow/site_detail.html
>@@ -12,28 +12,11 @@
> 
>             <div class="heading">
>                 <h2>{{ site.base_url }}</h2>
>-                <a href="">{% trans "Remove this site" %}</a>
>+                <a href="{% url cesium.remove_site site.id %}" class="remove">{% trans "Remove this site" %}</a>

What did newlines ever do to you?

>             </div>
> 
>             {% score_overview _("Site Score") site %}
> 
>-            <div id="overview">
>-                <div class="module">
>-                    <h4>Site Score</h4>
>-                    <p class="scoreA score">100 (A)</p>
>-                </div>
>-                <div class="module run">
>-                    <h4>Last Test Run</h4>
>-                    <p>Yesterday @ 12:00</p>
>-                </div>
>-
>-                <div class="module run">
>-                    <h4>Next Test Run</h4>
>-                    <p>Tomorrow @ 12:00</p>
>-                </div>
>-            </div>
>-
>-
>             <div class="module">
>                 <div class="subheading">
>                     <h3>Site Statistics</h3>
>@@ -42,28 +25,31 @@
> 
>                 <table>
>                     <tr>
>-                        <td>Average Score</td>
>-                        <td>92 (A)</td>
>+                        <td>{% trans "Average Score" %}</td>
>+                        <td>{% grade site.score %}</td>
>                     </tr>
>                     <tr>
>-                        <td>Average Page Weight</td>
>-                        <td>389K</td>
>+                        <td>{% trans "Average Page Weight" %}</td>
>+                        <td>{{ site.avg_page_weight }}K</td>
>                     </tr>
>                     <tr>
>-                        <td>Average HTTP Requests</td>
>-                        <td>23</td>
>+                        <td>{% trans "Average HTTP Requests" %}</td>
>+                        <td>{{ site.avg_http_requests }}</td>
>                     </tr>
>                 </table>
>             </div>
>             <div class="module secondary">
>                 <div class="subheading">
>-
>                     <ul class="subnav">
>                         <li><a href="#">Score</a></li>
>                         <li><a href="#">Size</a></li>
>                         <li class="active"><a href="#">Requests</a></li>
>                     </ul>
>                 </div>
>+
>+                <div id="graph-score" class="graph"></div>
>+                <div id="graph-requests" class="graph"></div>
>+                <div id="graph-weight" class="graph"></div>

How much of this can be shared with page_detail?  The graphs are going to look pretty much the same, right?

>             </div>
> 
>             <div class="subheading">
>@@ -74,20 +60,38 @@
>             {% if site.page_set.count %}
>                 <table>
>                     <tr>
>-                        <th>Page</th>
>-                        <th>Score</th>
>-                        <th>Size (KB)</th>
>-                        <th>HTTP Requests</th>
>+                        <th>{% trans "Page" %}</th>
>+                        <th>{% trans "Score" %}</th>
>+                        <th>{% trans "Size" %}</th>
>+                        <th class="requests">{% trans "HTTP Requests" %}</th>
>                     </tr>
> 
>                     {% for page in site.page_set.all %}
>                         <tr>
>-                            <td><a href="#">{{page.url}}</a></td>
>-                            <td class="score scoreA"><p>98</p> <div class="scoreBarB" style="width:106px;"></div></td>
>-                            <td class="size"><p>32.3</p> <div class="scoreBarB" style="width:106px;"></div></td>
>-                            <td class="requests"><p>23</p><div class="scoreBarB" style="width:106px;"></div></td>
>+                            <td><a href="{% url cesium.page_detail page.id %}">{{page.url}}</a></td>
>+                            <td>
>+                                {% score page.score %} 
>+                                {% score_bar page.score %} 
>+                            </td>
>+                            <td class="size">
>+                                <p>{{page.weight|filesizeformat}}</p>

The Django style guide says to do {{ xxx }}, not {{xxx}}.

>+                            </td>
>+                            <td class="requests"><p>{{page.requests}}</p></td>
>                         </tr>
>                     {% endfor %}
>                 </table>
>             {% endif%}
> {% endblock content %}
>+
>+{% block scripts %} 
>+
>+  <script src="{{ MEDIA_URL }}js/flot/jquery.flot.js"></script>
>+  <script type="text/javascript">
>+    var graphs = {{ graphs|safe }},
>+        options = {xaxis: {mode: 'time'}},
>+        keys = ['score', 'requests', 'weight'];
>+    $.each(keys, function(i, val) {
>+        $.plot($('#graph-' + val), [graphs[val]], options)
>+    });
>+  </script>
>+{% endblock scripts %}

That part can be shared with page_detail too, right?

>diff --git a/autoyslow/templates/autoyslow/tags/score_overview.html b/autoyslow/templates/autoyslow/tags/score_overview.html
>index 304b673..a88286c 100644
>--- a/autoyslow/templates/autoyslow/tags/score_overview.html
>+++ b/autoyslow/templates/autoyslow/tags/score_overview.html
>@@ -8,10 +8,10 @@
>   </div>
>   <div class="module run">
>     <h4>{% trans "Last Test Run" %}</h4>
>-    <p>{{ object.last_testrun|datetime }}</p>
>+    <p class="date-run">{{ object.last_testrun|datetime }}</p>
>   </div>
>   <div class="module run">
>     <h4>{% trans "Next Test Run" %}</h4>
>-    <p>{{ object.next_testrun|howsoon }}</p>
>+    <p class="date-run">{{ object.next_testrun|howsoon }}</p>
>   </div>
> </div>
>diff --git a/autoyslow/templatetags/autoyslow.py b/autoyslow/templatetags/autoyslow.py
>index 8819b5e..3402684 100644
>--- a/autoyslow/templatetags/autoyslow.py
>+++ b/autoyslow/templatetags/autoyslow.py
>@@ -1,34 +1,45 @@
> from datetime import datetime, timedelta
> 
> from django import template
>+from django.template import Context, Template
> from django.utils.translation import ugettext as _, ungettext as n_
> 
> 
> register = template.Library()
> 
> 
>+def insert_letter_grade(template, score):
>+    grades = {90: 'A', 80: 'B', 70: 'C', 60: 'D'}
>+    for number, letter in sorted(grades.items(), reverse=True):
>+        if score >= number:
>+            return template.format(letter=letter)
>+    else:
>+        return template.format(letter='F')

I'd rather see a function that takes a number and returns a letter.  It would be more obvious what's going on.  This feels a lot like we're mutating variables, even though it's really not.

>+
> @register.inclusion_tag('autoyslow/tags/score_overview.html')
> def score_overview(title, obj):
>     return {'title': title, 'object': obj}
> 
>-
> @register.simple_tag
> def grade(score, show_letter=True):
>     inner = ('%s ({letter})' if show_letter else '%s') % score
>     template = '<p class="score score{letter}">%s</p>' % inner
>-    grades = {90: 'A', 80: 'B', 70: 'C', 60: 'D'}
>-    for number, letter in sorted(grades.items(), reverse=True):
>-        if score >= number:
>-            return template.format(letter=letter)
>-    else:
>-        return template.format(letter='F')
>-
>+    return insert_letter_grade(template, score)
> 
> @register.simple_tag
>+def score_bar(score):
>+    string = """<div class=\"scoreBar{letter}\" 
>+                style=\"width:{%% widthratio %s 100 80 %%}px;\">
>+                </div>""" % score

You don't need to escape quotes inside a triple-quoted string.  Isn't it beautiful?

>+    t = Template(string)
>+    c = Context({})

I'd put this all in one line, like Template(string).render(Context({})

>+    template = t.render(c) 
>+    return insert_letter_grade(template, score) 
>+     
>[e-mail]_tag
> def score(number):
>     return grade(number, show_letter=False)
> 
>-
> @register.filter('datetime')
> def datetime_filter(time):
>     now = datetime.now()
>diff --git a/autoyslow/views.py b/autoyslow/views.py
>index 38b6b29..15f6015 100644
>--- a/autoyslow/views.py
>+++ b/autoyslow/views.py
>@@ -27,11 +27,20 @@ def site_list(request):
> @login_required
> def site_detail(request, site_id):
>     site = Site.objects.for_user(request.user).get(pk=site_id)
>-    site_info = {}
>-    site_info['site'] = site
>+
>+    graph_data = site.test_average(30) 

Can you say test_average(days=30)?  I don't know what 30 means here without it.

Comment 4

[Ryan Doherty (:rdoherty)]

Attachment: v3 (finally)

v2 finally, didn't make a templatetag for the stats table, they weren't similar enough for me.

Comment 5

[Jeff Balogh (:jbalogh)]

Comment on attachment 427998 [details] [diff] [review]
v3 (finally)


>diff --git a/autoyslow/models.py b/autoyslow/models.py
>index 847ef57..e4e3b1e 100644
>--- a/autoyslow/models.py
>+++ b/autoyslow/models.py
>@@ -86,19 +86,61 @@ class Site(models.Model):
>         except AttributeError:
>             return False
> 
>-    @property
>-    def score(self):
>+    def average_for(self, field):
>         if self.last_testrun is None:
>             return 0
>         else:
>             pages = self.page_set.filter(last_testrun__gte=self.last_testrun)
>             tests = Test.objects.filter(page__in=pages)
>-            return int(tests.aggregate(models.Avg('score'))['score__avg'])
>+            val = None
>+            if field == 'score':
>+                val = int(tests.aggregate(models.Avg('score'))['score__avg'])
>+            elif field == 'weight':
>+                val = int(tests.aggregate(models.Avg('weight'))['weight__avg']/1024)
>+            elif field == 'requests':
>+                val = int(tests.aggregate(models.Avg('requests'))['requests__avg'])
>+
>+            return val

It seems like you could condense this to ``return int(tests.aggregate(val=Avg(field))['val'])``.

And you can handle the weight division in the weight property.  If this is internalish, I'd prefix it with an underscore.

Comment 6

[Ryan Doherty (:rdoherty)]

r64159

I'd rather stick with one unified method of getting an average for the site than splitting it out into 2 different functions.