[Code-review] review: Gadget members branch

[Dafydd Harries]

>+        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))

Maybe it should be a method on the activity class.

  message.addChild(activity.to_xml(False))

>+    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.

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

>+            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.

>+        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.

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.

-- 
Dafydd