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

[Dan McGee]

On Tue, Aug 16, 2011 at 6:23 PM, Tom Willemsen <ryuslash at gmail.com> wrote:
> 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?
Yeah, I just did that for a sanity check on the updates to see when
new ISOs are added and removed- active is still updated as always, so
I wouldn't worry about the dates column.

>> > +                        {{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.
No problem. :)

>> > +    $(".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.
I'm here to help- this was great work, I just saw a few things we
could improve before it went live. Thanks!

-Dan