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

[Guillaume Desmottes]

Le lundi 26 mai 2008 ? 14:08 +0100, Dafydd Harries a ?crit :
> > 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.
> 

I don't think we need to invent our own function to do that. I modified
the code to use xpath.

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

done.

> > 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?
> 

sure.

> > 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?
> 

fixed.

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

Because this test check if the presence contains the right capa and
gadget send it when the buddy turns online.


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

Yeah, that's the only way I found to be sure that the message have been
processed by Gadget.

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

right.

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

good idea. I wasn't very happy with these names too.

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

done.

> > +
> > +    features.sort()
> > +    for feature in features:
> 
> You can simplify this too.
> 

done.

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

I pushed my changes. Thanks for the review.


	G.