[arch-releng] [PATCH] New page ISO Overview

[Dan McGee]

On Tue, Aug 16, 2011 at 2:24 PM, Tom Willemsen <ryuslash at gmail.com> wrote:
> ISO Overview shows a simple list of all the isos that are available
> and how many times they've been tested succesfully or have failed.
successfully, spelling
>
> Signed-off-by: Tom Willemsen <ryuslash at gmail.com>
> ---

Please go through the patch and make sure you are using "ISOs" and not
"isos" everywhere in the text. We should treat it as a proper noun.

>  releng/models.py                   |    9 +++++++
>  releng/urls.py                     |    1 +
>  releng/views.py                    |    7 +++++
>  templates/releng/iso_overview.html |   44 ++++++++++++++++++++++++++++++++++++
>  templates/releng/results.html      |    3 ++
>  templates/releng/thanks.html       |    4 ++-
>  6 files changed, 67 insertions(+), 1 deletions(-)
>  create mode 100644 templates/releng/iso_overview.html
>
> diff --git a/releng/models.py b/releng/models.py
> index 5510db6..3b50972 100644
> --- a/releng/models.py
> +++ b/releng/models.py
> @@ -50,6 +50,15 @@ class Iso(models.Model):
>     def __unicode__(self):
>         return self.name
>
> +    def get_test_result_count(self, success):
> +        return self.test_set.filter(success=success).count()
> +
> +    def get_success_count(self):
> +        return self.get_test_result_count(True)
> +
> +    def get_failure_count(self):
> +        return self.get_test_result_count(False)
> +
>  class Architecture(IsoOption):
>     pass
>
> diff --git a/releng/urls.py b/releng/urls.py
> index 4a125df..239ad02 100644
> --- a/releng/urls.py
> +++ b/releng/urls.py
> @@ -6,6 +6,7 @@
>     (r'^thanks/$',                       'submit_test_thanks', {}, 'releng-test-thanks'),
>     (r'^iso/(?P<iso_id>\d+)/$',          'test_results_iso', {}, 'releng-results-iso'),
>     (r'^(?P<option>.+)/(?P<value>\d+)/$','test_results_for', {}, 'releng-results-for'),
> +    (r'^iso/overview/$',                 'iso_overview', {}, 'releng-iso-overview'),
>  )
>
>  urlpatterns = patterns('',
> diff --git a/releng/views.py b/releng/views.py
> index 1d4a0b5..bf53781 100644
> --- a/releng/views.py
> +++ b/releng/views.py
> @@ -138,4 +138,11 @@ def test_results_for(request, option, value):
>  def submit_test_thanks(request):
>     return direct_to_template(request, "releng/thanks.html", None)
>
> +def iso_overview(request):
> +    isos = Iso.objects.all()
> +    context = {
> +        'isos': isos
> +    }
> +    return direct_to_template(request, 'releng/iso_overview.html', context)
> +
>  # vim: set ts=4 sw=4 et:
> diff --git a/templates/releng/iso_overview.html b/templates/releng/iso_overview.html
> new file mode 100644
> index 0000000..b786d6f
> --- /dev/null
> +++ b/templates/releng/iso_overview.html
> @@ -0,0 +1,44 @@
> +{% extends "base.html" %}
> +
> +{% block content %}
> +<div class="box">
> +    <h2>Failures and successes for testing isos</h2>
Title Case please:
Failures and Successes for Testing ISOs
> +
> +    <p><a href="{% url releng-test-overview %}">Go back to testing results</a></p>
> +
> +    <table id="releng-result" class="results">
> +        <thead>
> +            <tr>
> +                <th>Iso</th>
An "active" column might be good here. I might name it something more
user-friendly since that is our backend name, but you get the idea.
"Currently Available"?
> +                <th># Successes</th>
> +                <th># Failures</th>
> +            </tr>
> +        </thead>
> +        <tbody>
> +            {% for iso in isos %}
> +            <tr>
> +                <td>
> +                    <a href="{% url releng-results-iso iso.pk %}">
> +                        {{iso.name}}
I prefer spaces inside the {{ }} usages (and it is the usual
convention in the current code).
> +                    </a>
> +                </td>
> +                <td>
> +                    {{iso.get_success_count}}
> +                </td>
> +                <td>
> +                    {{iso.get_failure_count}}
> +                </td>
This kinda stinks- you're firing off 55 queries on the page at the
moment, and two more for every new ISO added to the system. This is
where Django annotation/aggregation should be used; that said, it
unfortunately isn't powerful enough to express what we want to do here
so we need to cheat a little bit. Make your original code something
like:

    from django.db.models import Count
    isos = Iso.objects.all().order_by('-pk')
    successes =
dict(Iso.objects.values_list('pk').filter(test__success=True).annotate(ct=Count('test')))
    failures = dict(Iso.objects.values_list('pk').filter(test__success=False).annotate(ct=Count('test')))
    for iso in isos:
        iso.success = successes.get(iso.pk, 0)
        iso.failure = failures.get(iso.pk, 0)

and then you can do iso.successes, iso.failures in the template. This
gets us down to 3 total queries, and the number of queries is no
longer dependent on the number of ISOs in the system.

> +            </tr>
> +            {% endfor %}
> +        </tbody>
> +    </table>
> +</div>
> +{% load cdn %}{% jquery %}
> +<script type="text/javascript" src="/media/jquery.tablesorter.min.js"></script>
> +<script type="text/javascript" src="/media/archweb.js"></script>
> +<script type="text/javascript">
> +$(document).ready(function() {
> +    $(".results:not(:has(tbody tr.empty))").tablesorter({widgets: ['zebra'], sortList: [[1,1],[2,0]]});
What was the goal here as far as default sort order? I would find it
far more intuitive to sort by id, e.g. creation date, and put the
newest ones at the top. You can always sort later, but showing some
year old ISO at the top just because it has the most feedback is a bit
misleading.

I also try to ensure the default sort order is done in the backend as
well so those that disable JS don't have a randomly-ordered list. So I
would add an .order_by('-pk') clause.

> +});
> +</script>
> +{% endblock %}
> diff --git a/templates/releng/results.html b/templates/releng/results.html
> index c3e7d99..4ff3c86 100644
> --- a/templates/releng/results.html
> +++ b/templates/releng/results.html
> @@ -13,6 +13,9 @@
>     if you have tested and used any ISOs. Both successful and failed results
>     are encouraged and welcome.</p>
>
> +    <p>For a overview of which ISOs tested best, have a look at
> +    the <a href="{% url releng-iso-overview %}">overview</a>.</p>
> +
>     <p>For more information, see the <a
>     href="https://wiki.archlinux.org/index.php/DeveloperWiki:releng_testimages_feedback">documentation
>     on the wiki</a>.</p>
> diff --git a/templates/releng/thanks.html b/templates/releng/thanks.html
> index b261426..80018b0 100644
> --- a/templates/releng/thanks.html
> +++ b/templates/releng/thanks.html
> @@ -8,6 +8,8 @@
>     <p>Thank you for taking the time to give us this information!
>     Your results have been succesfully added to our database.</p>
>     <p>You can now <a href="{% url releng-test-overview %}">go back to the results</a>,
> -    or <a href="{% url releng-test-submit %}">give more feedback</a>.</p>
> +    <a href="{% url releng-test-submit %}">give more feedback</a>, or
> +    have a look at the <a href="{% url releng-iso-overview %}">look at
> +    the ISO test overview</a>.</p>
>  </div>
>  {% endblock %}
> --
> 1.7.6
>
>