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

[Tom Willemsen]

On 16 Aug 15:32, Dan McGee wrote:
> 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.

I did and all occurrences, apart from urls and url names and such,
should match now.

> > +    <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"?

Sounds good.
I've just noticed though that a removed field has been added to the Iso
model, should I do something with this as well? Ignore these or
something?

> > +                        {{iso.name}}
> I prefer spaces inside the {{ }} usages (and it is the usual
> convention in the current code).

Gah, of course... I should have remembered.

> > +                <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.

You're right, that does stink, I basically copied it from the
get_last_success and get_last_failure functions, I'd forgotten that
those weren't actually very query-friendly.

Thanks for the code, it would've taken me quite some time to figure that
out. I read about annotations the last time I worked on this project and
I seem to remember barely understanding half of how they work.

> > +    $(".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.

Right, I was thinking that, for picking which ISO to use for my install
I'd like to know which one had the best scores. Since old ISOs are
thrown away anyway, for users they'd be useless. But since this isn't
just for users I see your point.

> 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.

As I see you already did in your bit of code.

Thanks for your feedback. I've changed the patch and am going to try
again right after this. I hope it's better this time.