schooltool-developers team mailing list archive

[Justas <justas@xxxxxx>]

On 03/14/2011 06:59 AM, Alan Elkner wrote:
> Hey Justas,
>
> Could you please look at the last two commits I made to my refactoring
> branch, lp:~aelkner/schooltool.intervention/test_coverage, the first
> being for fixing the views and the second for evolution.  I'd like to
> discuss them in detail at the meeting.
>
> For the view changes, they mostly have to do with updating the widget
> to work with the new attributes.  I moved the getPersonsResponsible
> method out of schooltool/intervention/intervention.py into the widget
> __init__ method itself since it was only being used there anyway.  The
> widget code is cleaner now.
  It would look better if you moved it to 
PersonListWidget.getPersonsResponsible.

  It could use more refactoring like that.  For example move code 
"# set student from field, view type" to .getStudent, or even a 
@property def student(...).

  "# build responsible, notified and changed lists of contacts" part 
could also be remove from __init__ for starters.  I just like my 
__init__'s without heavy logic...

  But that's personal preferences - just wanted to share.

> I thought it was a good idea to replace my redundant form key logic
> with the supplied adapter from the contact package, but that does lead
> to a problem with testing in that the form keys are not easily
> dissected for their parts.  Perhaps we should have another '.' in the
> 'persons.%s%x' to allow us to get the user id easily using split('.').
>   I had to write a new method in my tests called choiceControl that
> finds the checkbox control based on the user id and parent index, but
> it has to use startswith() which breaks if I have ids like 'teacher'
> and teacher2', one starting with the other.  Fortunately I don't have
> such ids, but it is a vulnerability for anyone changing the tests in
> the future.
  One thing that may help you is using labels in your page template.  
For checbox

<input type="checkbox" name="remove_item.contacts.Contact-2-..." 
id="remove_item.contacts.Contact-2-..." />

  Also render a label instead of span

<label for="remove_item.contacts.Contact-2-...">Peter Peterson</label>

  Then you can just
>>> browser.getControl('Peter Peterson').click()

  This is not 100% solution, but it might ease your testing.

> Another thing that would be nice is if we had an adapter that goes
> from the form key back to the contact.  I just loop through all the
> possible contacts in my widget to find a match which works but is less
> efficient.
  You can actually do that now.  Internals of index are kind of exposed 
for usage at the moment, as in - we don't have nice wrappers.

   catalog = ICatalog(self.context)
   index = catalog['form_keys']
   contact_ids = set(index.values_to_documents.get(your_form_key, ()))

  Here len(contact_ids) should be exactly 1.

> One thing that I left out of the evolution is the persons_responsible
> catalog which is now keyed by the relationship property rather than
> the list of ids as it was before.  I didn't write that evolve step yet
> because I wanted to discuss it first to make sure it was okay to have
> the index carrying the relationship properties in the first place.
> Doesn't that cause the problem of having to visit too many objects
> while looping though the index?  Isn't that why we try to use strings
> like ids in an index when possible?  If so, what strings could we
> carry there, InitIds?  You can look at my commit from earlier in the
> week for the catalog changes and also the InterventionFilterWidget
> class that uses the catalog for comparison.

  Well, current interventions catalog has some issues.

  Being Catalog(FilterExtent(FilterImplementing(IInterventionMarker))), 
it updates only when objects implementing the marker change.

  This means that person names won't be updated (because IContact 
changes, not IInterventionMarker).
  With current table formatter implementation, we can use only one 
catalog, so just put a fat XXX there.

  Same theoretically goes for schoolYearId;  but as it's cataloged 
value not used AFAIK, I'd just remove it from catalog.

  As for relationships, yes, you can store object ids for now.  Yet 
another thing that needs work in core...

  Also, this is a good time to replace the old catalog style with a 
newer one, see changes too schooltool/contact/* and the evolution script 
here:  
http://bazaar.launchpad.net/~justas-pov/schooltool/catalogs/revision/2646

> Anyway, besides the small catalog evolution step which I can add to
> evolve7 after we decide what to do, I think evolve7 looks good and is
> well tested.  Let me know if you think otherwise.  I tested it
> manually as well with a database I created using a checkout of trunk,
> and it worked well there, too.  Only thing that didn't work was the
> intervention tab view which will work better after we resolve the
> catalog issue.

  Just a small note - you are actually doing three tests there.  Please 
split to three separate doctests.

Cheers,
Justas