[sugar] review: Guillaume's pubsub-notif branch of Gadget

[Dafydd Harries]

> diff --git a/gadget/component.py b/gadget/component.py
> @@ -421,3 +435,24 @@ class GadgetService(component.Service):
>                  if jid.resource in room.members:
>                      del room.members[jid.resource]
>  
> +    def message_pubsub_notification(self, stanza, event):
> +        items = event.firstChildElement()
> +        if items['node'] == ns.BUDDY_PROP:
> +            self.message_buddy_properties_notification(stanza, items)
> +
> +    def message_buddy_properties_notification(self, stanza, items):
> +        from_ = stanza.getAttribute('from')
> +        for item in items.elements():
> +            if item.name != 'item':
> +                continue
> +            for properties in item.elements():
> +                if properties.name != 'properties':
> +                    continue

You should check the URIs in addition to the node names.

This is a common pattern in the code; perhaps we could use a utility function
like this:

  def filterElements(elem, uri, name):
      for child in elem.elements():
          if child.uri == uri and child.name == name:
              yield child

Then we could rewrite the above code as:

  for item in filterElements(item, ns.FOO, 'item'):
      for properties in filterElements(item, ns.BUDDY_PROP, 'properties'):
          props = parse_properties(properties)
          ...

Alternatively, we could use twisted.words.xish.xpath:

  from twisted.words.xish import xpath

  query = '{%s}item/{%s}properties' % (ns.FOO, ns.BUDDY_PROP)

  for properties in xpath.queryForNodes(elem, query):
      pass

That's if I remember the syntax correctly. Not sure which is better.

> +
> +                props = parse_properties(properties)
> +                try:
> +                    self.model.buddy_update(from_, props)
> +                except KeyError:
> +                    self.model.buddy_add(from_, props)
> +
> +                # FIXME: send notification messages to interested buddies

I would prefer:

  if self.model.has_buddy(from_):
      self.model.buddy_update(from_, props)
  else:
      self.model.buddy_add(from_, props)

> diff --git a/gadget/ns.py b/gadget/ns.py
> index 293d4dd..8700f04 100644
> --- a/gadget/ns.py
> +++ b/gadget/ns.py
> @@ -8,4 +8,6 @@ DISCO_INFO = 'http://jabber.org/protocol/disco#info'
>  MUC = 'http://jabber.org/protocol/muc'
>  MUC_USER = 'http://jabber.org/protocol/muc#user'
>  XMPP_STANZAS = 'urn:ietf:params:xml:ns:xmpp-stanzas'
> -
> +PUBSUB = 'http://jabber.org/protocol/pubsub'
> +PUBSUB_EVENT = 'http://jabber.org/protocol/pubsub#event'
> +CAPS = 'http://jabber.org/protocol/caps'

Can we keep this file in ASCII order?

> diff --git a/gadget/test_component.py b/gadget/test_component.py
> index 178424d..5379870 100644
> --- a/gadget/test_component.py
> +++ b/gadget/test_component.py
> @@ -948,3 +948,100 @@ class PropertiesToXmlTest(unittest.TestCase):
>          assert property['name'] == 'some_bytes'
>          child = property.children[0]
>          assert child == 'ABCDEF'
> +
> +class CapsTest(TestCase):
> +    def runTest(self):
> +        self.done = defer.Deferred()
> +        self.server.addOnetimeObserver('//presence', self.presence)
> +
> +        presence = domish.Element((ns.CLIENT, 'presence'))
> +        presence['from'] = 'bob at foo.org'
> +        presence['to'] = 'gadget.foor.org'

Typo?

> +        presence['type'] = 'probe'
> +        self.server.send(presence)
> +        return self.done
> +
> +    def presence(self, stanza):
> +        assert stanza.name == 'presence'
> +        c = stanza.firstChildElement()
> +        assert c.name == 'c'
> +        assert c.uri == ns.CAPS
> +
> +        self.server.addOnetimeObserver('//iq', self.iq)
> +        iq = IQ(self.server, "get")
> +        iq['from'] = 'bob at foo.org'
> +        iq['to'] = 'gadget.bar.org'
> +        query = iq.addElement((ns.DISCO_INFO, 'query'))
> +        query['node'] = c['node']
> +        self.server.send(iq)
> +
> +    def iq(self, stanza):
> +        query = stanza.firstChildElement()
> +        assert query.name == 'query'
> +        identities = set()
> +        features = set()
> +        for elt in query.children:
> +            assert elt.name in ('feature', 'identity')
> +            if elt.name == 'identity':
> +                identities.add((elt['category'], elt['type'], elt['name']))
> +            elif elt.name == 'feature':
> +                features.add(elt['var'])
> +        assert identities == set([('collaboration', 'gadget', 'OLPC Gadget')])
> +        assert features == set([ns.ACTIVITY, ns.BUDDY, "%s+notify" % ns.BUDDY_PROP])
> +
> +        self.done.callback(None)

I don't see the connection between the presence probe and the disco query
here. Don't we already have a test for the latter?

> +class BuddyPropertiesChangeTest(TestCase):
> +    def runTest(self):
> +        self.done = defer.Deferred()
> +        self.service.model.buddy_add('bob at foo.org', {})
> +
> +        message = domish.Element((None, 'message'))
> +        message['from'] = 'bob at foo.org'
> +        message['to'] = 'gadget.foo.org'
> +        event = message.addElement((ns.PUBSUB_EVENT, 'event'))
> +        items = event.addElement((None, 'items'))
> +        items['node'] = ns.BUDDY_PROP
> +        item = items.addElement((None, 'item'))
> +        properties = item.addElement((ns.BUDDY_PROP, 'properties'))
> +        property = properties.addElement((None, 'property'))
> +        property['name'] = 'color'
> +        property['type'] = 'str'
> +        property.addContent('red')
> +
> +        self.server.send(message)

I think this should be synchronous, so we shouldn't need to iterate the
reactor, but in some cases the loopback transport doesn't behave as I expect
it to. I guess it behaved oddly for you here too?

> +        reactor.callLater(0, self.later1)
> +
> +        return self.done
> +
> +    def later1(self):
> +        buddy = self.service.model.buddy_by_id('bob at foo.org')
> +        assert buddy.properties == {'color': ('str', 'red')}
> +
> +        message = domish.Element((None, 'message'))
> +        message['from'] = 'bob at foo.org'
> +        message['to'] = 'gadget.foo.org'
> +        event = message.addElement((ns.PUBSUB_EVENT, 'event'))
> +        items = event.addElement((None, 'items'))
> +        items['node'] = ns.BUDDY_PROP
> +        item = items.addElement((None, 'item'))
> +        properties = item.addElement((ns.BUDDY_PROP, 'properties'))
> +        property = properties.addElement((None, 'property'))
> +        property['type'] = 'str'
> +        property['name'] = 'key'
> +        property.addContent('my key')
> +        property = properties.addElement((None, 'property'))
> +        property['type'] = 'str'
> +        property['name'] = 'color'
> +        property.addContent('blue')
> +
> +        self.server.send(message)
> +        reactor.callLater(0, self.later2)
> +        return self.done

I don't think you need to return anything here.

> +
> +    def later2(self):
> +        buddy = self.service.model.buddy_by_id('bob at foo.org')
> +        assert buddy.properties == {'color': ('str', 'blue'),
> +            'key': ('str', 'my key')}
> +
> +        self.done.callback(None)
> diff --git a/gadget/util.py b/gadget/util.py
> index ac8638d..f342085 100644
> --- a/gadget/util.py
> +++ b/gadget/util.py
> @@ -1,3 +1,9 @@
> +import base64
> +try:
> +    from hashlib import sha1
> +except ImportError:
> +    # Python < 2.5
> +    from sha import new as sha1
>  
>  from twisted.words.xish import domish
>  
> @@ -51,6 +57,28 @@ def xml_pformat(e, indent=0, width=80):
>  
>      return ret
>  
> +def capa_hash(S):

I think we can give these functions more descriptive names; perhaps we can
rename capa_hash to something like _hash_capabilities and gen_capa_ver to
hash_capabilities. Although the result of gen_capa_ver is used in the 'ver'
field, what it does is perform a hash.

> +    r"""
> +    >>> capa_hash('client/pc//Exodus 0.9.1<http://jabber.org/protocol/caps<http://jabber.org/protocol/disco#info<http://jabber.org/protocol/disco#items<http://jabber.org/protocol/muc<')
> +    'QgayPKawpkPSDYmwT/WM94uAlu0=\n'
> +    """
> +    return base64.encodestring(sha1(S).digest())
> +
> +def gen_capa_ver(identities, features):
> +    r"""
> +    >>> gen_capa_ver([('client', 'pc', 'Exodus 0.9.1')], ["http://jabber.org/protocol/disco#info", "http://jabber.org/protocol/disco#items", "http://jabber.org/protocol/muc", "http://jabber.org/protocol/caps"])
> +    'QgayPKawpkPSDYmwT/WM94uAlu0=\n'
> +    """
> +    S = ""
> +    identities.sort()
> +    for identity in identities:
> +        S += "%s/%s//%s<" % identity

You can write:

  for identity in sorted(identities):

This is shorter and has the advantage of not modifying data that the caller
passes you.

> +
> +    features.sort()
> +    for feature in features:

You can simplify this too.

> +        S += "%s<" % feature
> +    return capa_hash(S)
> +
>  if __name__ == '__main__':
>      import doctest
>      doctest.testmod()

Otherwise, fine.

-- 
Dafydd