[sugar] review: Guillaume's pubsub-notif branch of Gadget
Dafydd Harries
dafydd.harries at collabora.co.uk
Mon May 26 09:08:43 EDT 2008
> 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
More information about the Sugar
mailing list