schooltool-developers team mailing list archive

[Justas <justas@xxxxxx>]

Hi,

On 03/03/2011 08:22 AM, Alan Elkner wrote:
> Hey Justas,
>
> After talking with Tom about what changes I would make to the
> intervention implementation, we agreed that there were two very
> practical tasks I could undertake in the short term.  I'd like your
> comments on these tasks to make sure I don't come up with the wrong
> time estimate for Tom.  Please look at interfaces.py and
> intervention.py to follow my points.
>
> The first one is a small one requiring no change to the data model.
> As is necessary in many of the views, I need to display the student's
> name.  Currently, I have a student attribute for all of the
> intervention objects that computationally returns the student's
> BasicPerson record.  I think you would prefer that I used the more
> standard approach of creating adapters from all the intervention
> object types to IBasicPerson and then get rid of the no longer needed
> property.  This would only take a couple of days to change the
> implementation and write the tests.

  Great!  Adapter to IPerson would be a bit more appropriate though.

> The second task is a much larger one, requiring a change to the data
> model, hence, an evolution script.  I know how much you hate having
> ids saved in objects, so I plan to replace the current lists of ids
> with relationships to the actual objects themselves.  This way, if a
> person is removed from the system, they will automatically disappear
> from the intervention objects, the advantage of using relationships.
> This would also eliminate the need for my helper functions like
> convertIdToNameWithSort which is already too complex and difficult to
> test.  The most difficult to understand part is the ':n' that I add to
> the student's id when I need to save a reference to one of the
> student's parents.  Instead, I'll carry relationship links to the
> contacts themselves, also magically disappearing if deleted at the
> source.
> Specifically, in the InterventionMessage object, I will be replacing
> the sender attribute, an id, with a sender_link attribute, a
> relationship.  I need the attribute name to change in order to handle
> the evolve step where I will be reading from one and writing to the
> other, right?  The recipients attribute will be replaced by two new
> attributes, recipients_persons and recipients_contacts.  I need to
> have two different lists there because relationships to teachers,
> administrators and the student is with BasicPerson objects where
> relationships to parents is with Contact objects.
  All BasicPerson objects have their contact information obtainable via 
IContact(person), so it would be more consistent to create relationships 
with contact objects only.  This way you would only need a single list 
for recipients, for example.   Also, contacts that are actual users in 
ST can be adapted back via IPerson(contact) - if you need to.

  As for evolution, there is no need to rename attributes in this case.

  You can just erase the old attribute from the dict.  Say, you changed 
recipients to a relationship property:

  class InterventionMessage(...):
      recipients = RelationshipProperty(...) # message <-> contact 
relationships, URIMessageRecipients
      ...

  Then in evolution script, you can:

  recipient_ids = []
  # Here you address the old data stored for the InterventionMessage 
instance
  if 'recipients' in message.__dict__:
      recipient_ids = message.__dict__['recipients']
      del message.__dict__['recipients']

  for recipient_id in recipient_ids:
      # Do the magic to get the contact object if ':' in id, or 
IContact(person) if this was the person id
      contact = getContactFromId(recipient_id)

      # And here you use the new relationship property
      intervention_message.recipients.add(contact)


  As for sender, a relationship may be overkill...  Then again - it 
will do for now.
  What we want here is a weak reference to some object, maybe via IntId 
or something.  Sadly, ST does not have a standard way to do this at the 
moment.  So a relationship it is.

> For InterventionGoal objects, creator will be replaced by
> creator_link, again the name change for evolution purposes.  The
> persons_responsible attribute will be broken up into
> persons_responsible_persons and person_responsible_contacts, where
> at_one_time_responsible will be broken up into
> at_one_time_responsible_persons and at_one_time_responsible_contacts.

  Again, you can keep the attribute names and - not split relationships 
to _persons/_contacts.  persons_responsible and at_one_time_responsible 
will suffice.

> One of the trickier parts of this change is how to rework the
> PersonListField widget which is designed to work off of the list of
> ids (:n included) that are found in the current implementation.  The
> widget is designed to list the teachers and administrators in the
> column labeled 'Staff' and the student and parents in the second
> column labeled 'Student/Parents'.  Also, in those rare cases where
> there is someone in the at_one_time_responsible list that is not in
> the persons_responsible list, those people appear in the second
> column, right below the student and parents with a 'Changed Status'
> heading above.  This is all what we agreed to at the Rhode Island
> sprint.
>
> In any event, the mapping of the one list to this complex widget would
> no longer be possible in the new implementation where we would be
> carrying lists of both persons and contacts.

  Right, this is a +1 to working with contacts only.  And using 
IPerson(contact, None) where needed.  Code will be simpler, and maybe 
you can keep the widget.

>    Would this be a case for
> a sub-form?  The need to render the two column list and process the
> request for which checkboxes the user clicked is found in four places,
> each of the add an edit views for goals and messages.  I suppose the
> same page template could be used for rendering the sub-form for goals
> and messages and the processing of the request could be handled by two
> different view classes, one for goals and one for messages.  The
> current goal and message add and edit views are old formlib views, not
> z3c.form, so there's that to deal with.  Also, even if I change the
> views to use z3c.form, we don't currently have a page macro in core
> that allows for sub-forms.  I suppose I could just have my own page
> templates for this and not worry about using page macros.  What do
> recommend?
  This is a very reasonable approach, and yes - don't worry about using 
page macros too much.

  Both approaches - a simple sub-form and keeping the old widget - look 
ok for me in this case, so in the end it's your call.

  Overall - a nice refactoring!  Looking forward to it :)

Cheers,
Justas