[sugar] review: Guillaume's 'activity' branch of Gadget

[Guillaume Desmottes]

Le mardi 10 juin 2008 à 14:42 +0100, Dafydd Harries a écrit : 
> > diff --git a/gadget/component.py b/gadget/component.py
> > index b5fc702..dd85a03 100644
> > --- a/gadget/component.py
> > +++ b/gadget/component.py
> > @@ -92,8 +92,9 @@ class Room(object):
> >      def __init__(self, own_nick):
> >          self.own_nick = own_nick
> >          self.membership = None
> > -        self.members = {}
> > +        self.members = set()
> >          self.properties = {}
> > +        self.id = None
> >  
> 
> Would it be possible to just have a Room.activity property and use that to
> store members/id?
> 

I'll remove the Room object and move membership management to Activity
as we discussed yesterday.


> >  class GadgetService(component.Service):
> >      debug = False
> > @@ -333,7 +334,7 @@ class GadgetService(component.Service):
> >      def message(self, stanza):
> >          type = stanza.getAttribute('type', 'normal')
> >  
> > -        if type in ('chat', 'groupchat'):
> > +        if type in ('chat'):
> >              return
> 
> This doesn't do quite what you want. "()" doesn't create a tuple, "," does, so
> this is the same as "type in 'chat'", which will be true if type is a
> substring of "chat". Better to just say "==".
> 

Right. Forgot about that, good catch. Fixed.

> >  
> >          if type == 'normal':
> > @@ -343,12 +344,17 @@ class GadgetService(component.Service):
> >                      return
> >                  elif (elem.uri == ns.ACTIVITY_PROP and
> >                        elem.name == 'properties'):
> > -                    self.message_activity_properties(stanza, elem)
> > +                    self.message_pre_invite_activity_properties(stanza, elem)
> >                      return
> >                  elif elem.uri == ns.PUBSUB_EVENT and elem.name == 'event':
> >                      self.message_pubsub_notification(stanza, elem)
> >                      return
> >  
> > +        elif type == 'groupchat':
> > +            for elem in stanza.elements():
> > +                if elem.uri == ns.ACTIVITY_PROP and elem.name == 'properties':
> > +                    self.message_activity_properties(stanza, elem)
> 
> You should return here. Perhaps we should dispatch messages more like we
> dispatch IQs, i.e. using a dict.
> 

fixed. I added a FIXME suggestion a better dispatch system.

> > +
> >          # FIXME: handle close query stanza
> >          log.msg('got unhandled <message>')
> >  
> > @@ -357,7 +363,7 @@ class GadgetService(component.Service):
> >              if elem.uri == ns.MUC_USER and elem.name == 'invite':
> >                  self.message_muc_invite(stanza, elem)
> >  
> > -    def create_room(self, jid, properties):
> > +    def create_room(self, jid, properties, id):
> 
> Hmm, can we put the id before the properties? I think it's more aesthetically
> pleasing that way.
> 

done.

> >          if jid in self.mucs:
> >              # We already have a room by this name.
> >              return
> > @@ -366,9 +372,10 @@ class GadgetService(component.Service):
> >          # join it.
> >          room = Room('inspector')
> >          room.properties = properties
> > +        room.id = id
> >          self.mucs[jid] = room
> >  
> > -    def message_activity_properties(self, stanza, properties_elem):
> > +    def message_pre_invite_activity_properties(self, stanza, properties_elem):
> >          # XXX Restrictions on from address?
> >  
> >          try:
> > @@ -384,7 +391,7 @@ class GadgetService(component.Service):
> >          if not (properties and activity and room_jid):
> >              return
> >  
> > -        self.create_room(room_jid, properties)
> > +        self.create_room(room_jid, properties, activity)
> 
> Perhaps we should rename 'activity' to 'activity_id' for clarity.
> 

done.

> >      def message_muc_invite(self, stanza, invite):
> >          to = stanza.getAttribute('to')
> > @@ -502,17 +509,54 @@ class GadgetService(component.Service):
> >                  # Presence for our in-room self; we've finished joining the
> >                  # room.
> >                  room.membership = 'joined'
> > +
> > +                # activities are created and added to the model when the
> > +                # inspector actually joined the muc.
> 
> English nitpicks: Your tenses don't agree here. "are created" ... "joined".
> s/joined/join/. If you use a full stop, you should also capitalise the first
> letter.
> 

fixed.

> > +                # If we'll do it before, we won't be able to properly track
> > +                # activity's properties and members.
> 
> I don't understand this comment.
> 

Humm me neither actually :) Removed.

> > +                activity = self.model.activity_add(room.id, room_jid,
> > +                        room.properties)
> > +
> > +                # Activity and Room now share the same set() object
> > +                activity.members = room.members
> >                  return
> >  
> > +        item = xpath_query('/presence/x[@xmlns="%s"]/item' % ns.MUC_USER,
> > +                stanza)
> > +        if not item or item[0].getAttribute('jid') is None:
> > +            log.msg("full jid of %s missing. Can't update activity members"
> > +                    % jid.resource)
> 
> English nitpicks: you should capitalise "full" and have a full stop after
> the second sentence.
> 

fixed.

> > +            return
> > +
> > +        full_jid = JID(item[0]['jid']).userhost()
> 
> Hmm, "full_jid" seems like a bad name, since it implies a JID with a resource.
> Perhaps it should be "real_jid" or just "jid".
> 

renamed to real_jid

> > +
> > +        activities = self.model.activity_by_room(room_jid)
> > +
> >          if type is None:
> >              log.msg('room %s has member %s' % (room_jid, jid.resource))
> > -            room.members[jid.resource] = True
> > +            room.members.add(full_jid)
> > +
> >          elif type == 'unavailable':
> >              if room_jid in self.mucs:
> >                  room = self.mucs[room_jid]
> >  
> > -                if jid.resource in room.members:
> > -                    del room.members[jid.resource]
> > +                if full_jid in room.members:
> > +                    room.members.remove(full_jid)
> > +
> > +                    if len(room.members) == 0:
> > +                        self.leave_activity(room_jid, room.id)
> > +
> > +    def leave_activity(self, room_jid, activity_id):
> 
> If we have Room.activity you won't need to pass around the ID here.

> > +        log.msg('leave room %s' % room_jid)
> > +
> 
> Should be "leaving room".
> 

fixed.

> > +        presence = domish.Element((None, 'presence'))
> > +        presence['to'] = '%s/inspector' % room_jid
> 
> You should use the JID class to generate the JID, and use Room.own_nick for
> the resource.
> 

done.

> > +        presence['type'] = 'unavailable'
> > +        x = presence.addElement((ns.MUC_USER, 'x'))
> > +        self.send(presence)
> > +
> > +        self.model.activity_remove(activity_id)
> > +        del self.mucs[room_jid]
> >  
> >      def message_pubsub_notification(self, stanza, event):
> >          items = event.firstChildElement()
> > @@ -533,3 +577,33 @@ class GadgetService(component.Service):
> >                  self.model.buddy_add(jid, props)
> >  
> >              # FIXME: send notification messages to interested buddies
> > +
> > +    def message_activity_properties(self, stanza, properties_elem):
> > +        id = properties_elem.getAttribute('activity')
> > +        room_jid = properties_elem.getAttribute('room')
> > +        jid = JID(stanza['from'])
> > +
> > +        if room_jid != jid.userhost():
> > +            log.msg("Ignored activity properties message from %s trying to change properties for another room"
> > +                    % stanza['from'])
> 
> Can you wrap this to 79 characters? You can just split the string literal in
> two.
> 
done.

> > +            return
> > +
> > +        try:
> > +            properties = parse_properties(properties_elem)
> > +        except PropertyTypeError:
> > +            log.msg("Ignored activity properties message from %s containing an invalid property"
> > +                    % stanza['from'])
> 
> Same here.
> 
done.

> > +            return
> > +
> > +        if not self.model.has_activity(id):
> > +            log.msg("Ignored activity properties message from %s about an invalid activity: %s "
> > +                    % stanza['from'], id)
> 
> And here.
> 
done.

> > +            return
> > +
> > +        # is activity private?
> > +        if properties.get('private') in (('bool', '1'), ('bool', 'true')):
> 
> Perhaps we should do some kind of normalisation in parse_properties, and maybe
> ignore malformed values.
> 

We already check this:
        if type == 'bool' and value not in ['1', '0', 'true', 'false']:
            raise PropertyTypeError(type, elems.uri)

Do you think we should check that the 'private' properties is always of
type 'bool'?

> > +            log.msg('Activity %s is not private. Leave it' % id)
> 
> s/Leave/Leaving/.
> 
fixed.



> > @@ -214,7 +214,7 @@ class RoomMembershipTest(TestCase):
> >          self.done = defer.Deferred()
> >          self.server.addOnetimeObserver('//presence', self.presence)
> >          # Simulate invitation process.
> > -        self.service.create_room('chat at conf.foo.org', {})
> > +        self.service.create_room('chat at conf.foo.org', {}, 'activity1')
> >          self.service.invited('inspector at gadget.bar.org', 'chat at conf.foo.org')
> >          return self.done
> >  
> > @@ -222,11 +222,15 @@ class RoomMembershipTest(TestCase):
> >          presence = domish.Element((ns.CLIENT, 'presence'))
> >          presence['from'] = 'chat at conf.foo.org/amy'
> >          x = presence.addElement((ns.MUC_USER, 'x'))
> > +        item = x.addElement('item')
> > +        item['jid'] = 'amy at foo.org/resource'
> >          self.server.send(presence)
> >  
> >          presence = domish.Element((ns.CLIENT, 'presence'))
> >          presence['from'] = 'chat at conf.foo.org/bob'
> >          x = presence.addElement((ns.MUC_USER, 'x'))
> > +        item = x.addElement('item')
> > +        item['jid'] = 'bob at foo.org/resource'
> >          self.server.send(presence)
> 
> We should probably have a test that includes a MUC presence message without a
> jid.

Good idea. Added.

> Perhaps we should factor out the code for creating MUC presence messages here.

done. I created a send_muc_presence method in TestCase as lot of tests need it.


I fixed all the typo in tests.


Thanks for the review,


	G.