[arch-releng] Iso tests

[Tom Willemsen]

On 10 Mar 10:04, Dan McGee wrote:
> Nice! I'd be glad to have this hosted on the main site (I'm the
> maintainer). Overall things look sound; sure there are some
> style/convention/etc. differences but nothing hard to fix up.
> 
> * View page: right now it is making 98 queries (one for each of those
> max values). Not that this page is probably going to see loads of
> traffic but we can surely improve upon that.
I would prefer to change that, yes, I was having a lot of trouble
figuring out a nice way to get this kind of structure in the page. So
if you or anyone else can point me in another direction it would be much
appreciated.

> * Add page: Using radio buttons for widgets here would probably be
> smarter than the dropdowns, otherwise field entry is excruciatingly
> painful. See the forms documentation for how to change the default
> widget.
I'll look at it, that does sound like it could be nicer.

> * models, Test.ms: totally un-descriptive field name, and you didn't
> give it a nice field name. Can't this just be "modules"?
Yes I'll do that.

> * models, Test: if you name things boot_type instead of boottype,
> you'll get nicer default labels on your view and entry screens. Same
> with the models themselves- Boottype -> BootType.
Ok I'll do that.

> * models, Iso: Likely need more than a date field here. We've double
> released, etc. It might just be worth having a CharField as no users
> are going to be entering these anyway, and also having a boolean
> "active" field or something to determine what actually shows up on the
> entry and view pages.
Sure, I didn't know what to put in it (or most of the models for that
matter) anyway.

> * models, get_success_test/get_failed_test: these can be abstracted
> into a common proxy model
> (http://docs.djangoproject.com/en/1.2/topics/db/models/#proxy-models)
> superclass, from which all these various ISO options inherit from.
Ah ok, I was thinking of using a base model for that, but in the
interest of being done as soon as possible I used this way first. I'll
look at this.

> * tests.py: yes, we should be writing tests, but at the least, get rid
> of the default garbage in this file.
Oh heh, I didn't write that file, I'll remove it.

> * admin.py: convention is not to use * imports; I know it works here
> and makes things cleaner but I've eradicated the codebase of them
> otherwise.
Ok I'll get rid of all of them.

> * models.py: "# Create your models here." -> not needed
You're right.

> * urls.py: You aren't using info_dict anymore; you have a blank second
> pattern definition?, and I'd prefer you follow indentation patterns of
> something like packages/urls.py.
Right again, I'll look at the indentation, I was thinking about that
anyway, it's not nice to not adhere to someone else's style rules, I
just sent first and wanted to clean up later.

> * views.py, add: A lot of extra comments. "Create your views here.",
> all the "form been submitted" trailing stuff. For the return, I've
> switched to using mostly direct_to_template to avoid some of the
> boilerplate- see the end of flag() in packages/views.py around line
> 373 for an example.
I'll look at that too.

> * views.py:, view: I see some things are _choices, some are _list- why
> the disparity?
Hehe, that's because in the beginning I thought it might be better to
use the choices parameters for some things, but then later I switched
and I apparently didn't want to rename the fields then, I think I was in
a rush at the time, sorry.

> * isotest/templates: why is this in a different directory from the
> standard layout?
Oh I'm sorry, that should have already been removed. That's there
because I read in the django documentation that templates _should_ also
be looked for in an app's directory, I thought it'd be nicer if I
completely seperated my app from the rest of archweb so it could have
been placed in any project if it wasn't going to be placed into archweb.
But I couldn't get that to work right away so moved on again.

> * Finally, I see 4 + 1 templates, but only two views- are these other
> ones old, unnecessary, not wired up yet?
They must be from tests with object_list generic functions and such.

> Great work! I know I just said a lot but these are mostly minor
> things, nothing to feel bad about.
Thank you! I appreciate your comments, I will get to work on everything
you've mentioned.

Tom