[Code-review] review: Gadget members branch

[Guillaume Desmottes]

Le lundi 11 août 2008 à 14:57 +0100, Dafydd Harries a écrit :
> >+        activity_elem = message.addElement((ns.ACTIVITY, 'activity'))
> >+        activity_elem['id'] = str(activity.id)
> >+        activity_elem['room'] = activity.room
> >+        activity_elem['view'] = self.id
> 
> This seems like something we should factor out. Then we can just write:
> 
>   message.addChild(activity_to_xml(activity))
> 
> Perhaps we need two variants, depending on whether we want to include the properties or not.
> 
>   message.addChild(activity_to_xml(activity, False))
> 

Actually I didn't have an activity_to_xml method in the component class.
I adapted it to use it in all cases.

> Maybe it should be a method on the activity class.
> 
>   message.addChild(activity.to_xml(False))

I do like to have the model completely XMPP ignorant.

> >+    def buddy_properties_to_xml(self, buddy):
> >+        if not self.model.has_buddy(buddy):
> >+            log.msg("unknown member %s; can't add their properties"
> >+                    "to query reply" % buddy)
> >+            return None
> >+
> >+        buddy = self.model.buddy_by_id(buddy)
> >+        properties = domish.Element((ns.BUDDY_PROP, 'properties'))
> >+
> >+        for node in properties_to_xml(buddy.properties):
> >+            properties.addChild(node)
> >+
> >+        # FIXME: We should send buddy properties only if the user
> >+        # doesn't know this JID yet (he doesn't have any open view
> >+        # containing it).
> >+
> >+        return properties
> >+
> 
> This refactoring looks a bit strange: it mixes together the job of checking
> that the buddy ID is valid with converting the data to XML. I think it would
> be better for this code to take a buddy object, and for the calling function
> to be responsible for ensuring it's dealing with a valid ID. Also, the comment
> doesn't really make sense in this context.

You're right. I dropped  this function and used properties_to_xml
directly.

> Why might we have an invalid buddy ID to begin with?

If a contact joins the activity room but is not subscribed to Gadget.
Then Gadget doesn't know his properties.


> >+            log.msg("can't update 
> activity members: full jid of %s is missing"
> >+                    % nick)
> 
> This message is a bit misleading: better to say "real" instead of "full".
> Also, "JID" should be capitalised.

fixed.


> >+        if type is None:
> >+            log.msg('room %s has member %s' % (room_jid, nick))
> >+            activity.members.add(real_jid)
> 
> This check should be an early return at the beginning of the function. It
> would be very strange if the type was not None while we were joining the room.
> 
done.

> We should probably do:
> 
>   type = presence.getAttribute('type', 'available')
> 
> And then compare against that. But that's an aesthetic change.
> 
> >+        self.server.addOnetimeObserver('//message', self.message3)
> 
> I wonder if we could automate this.
> 
>   class StanzaSeriesTest(unittest.TestCase):
>       def __init__(self):
>           # Chain stanza callbacks together.
>           callbacks = [getattr(self, name)
>               for name in dir(self)
>               if name.startswith('stanza')]
> 
>           def wrap(callback, next):
>               def wrapped(elem):
>                   callback(elem)
>                   self.addOnetimeObserver('//*', next)
> 
>               return wrapped
> 
>           for callback, next in zip(callbacks[:-1], callbacks[1:]):
>               self.__dict__[f.__name__] = wrap(f)
> 
> This is untested, but you see the principle. One disadvantage is that callback
> names might be less self-descriptive.


I'm not convince about this. Current code make things clearer and the
test workflow is easier to understand (which is more important than
code's size for tests IMHO).


	G.