On Tue, Aug 16, 2011 at 6:23 PM, Tom Willemsen <ryuslash@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