Merge lp:~robru/friends/simplify-contacts into lp:friends

[Robert Bruce Park]

More explicit call signature for Base._push_to_eds.

So, re-reviewing the code after the last merge, I realized that I could take it even a step further in simplification ;-)

Now instead of passing an obscure dict as the argument to _push_to_eds, I've broken that down into explicit arguments, so that the call signature is significantly more scrutible. Note how the three protocols that call _push_to_eds now have to supply significantly less boilerplate, and it reads much more clearly.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:237
http://jenkins.qa.ubuntu.com/job/friends-ci/51/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/friends-saucy-amd64-ci/8

Click here to trigger a rebuild:
http://s-jenkins:8080/job/friends-ci/51/rebuild

review: Approve (continuous-integration)

=== modified file 'friends/protocols/facebook.py'
--- friends/protocols/facebook.py	2013-07-20 02:21:49 +0000
+++ friends/protocols/facebook.py	2013-07-23 08:22:30 +0000
@@ -328,30 +328,26 @@
 
     @feature
     def contacts(self):
+        access_token=self._get_access_token()
         contacts = self._follow_pagination(
             url=ME_URL + '/friends',
-            params=dict(access_token=self._get_access_token(), limit=1000),
+            params=dict(access_token=access_token, limit=1000),
             limit=1000)
         log.debug('Found {} contacts'.format(len(contacts)))
 
         for contact in contacts:
             contact_id = contact.get('id')
-            if contact_id is None or self._previously_stored_contact(contact_id):
-                continue
-            full_contact = Downloader(
-                url=API_BASE.format(id=contact_id),
-                params=dict(access_token=self._get_access_token())).get_json()
-            self._push_to_eds({
-                'facebook-id':   contact_id,
-                'facebook-name': full_contact.get('name'),
-                'facebook-nick': full_contact.get('username'),
-                'X-URIS':        full_contact.get('link'),
-                'X-GENDER':      full_contact.get('gender'),
-                'X-FOLKS-WEB-SERVICES-IDS': {
-                    'jabber': '-{}@chat.facebook.com'.format(contact_id),
-                    'remote-full-name': full_contact.get('name'),
-                    'facebook-id': contact_id,
-                }})
+            if not self._previously_stored_contact(contact_id):
+                full_contact = Downloader(
+                    url=API_BASE.format(id=contact_id),
+                    params=dict(access_token=access_token)).get_json()
+                self._push_to_eds(
+                    uid=contact_id,
+                    name=full_contact.get('name'),
+                    nick=full_contact.get('username'),
+                    link=full_contact.get('link'),
+                    gender=full_contact.get('gender'),
+                    jabber='-{}@chat.facebook.com'.format(contact_id))
 
         return len(contacts)
 

=== modified file 'friends/protocols/linkedin.py'
--- friends/protocols/linkedin.py	2013-07-20 02:21:49 +0000
+++ friends/protocols/linkedin.py	2013-07-23 08:22:30 +0000
@@ -113,17 +113,13 @@
         ).get_json().get('values', [])
 
         for connection in connections:
-            connection_id = connection.get('id', 'private')
+            uid = connection.get('id', 'private')
             fullname = make_fullname(**connection)
-            if connection_id != 'private' and not self._previously_stored_contact(connection_id):
-                self._push_to_eds({
-                    'linkedin-id': connection_id,
-                    'linkedin-name': fullname,
-                    'X-URIS': connection.get(
-                        'siteStandardProfileRequest', {}).get('url'),
-                    'X-FOLKS-WEB-SERVICES-IDS': {
-                        'remote-full-name': fullname,
-                        'linkedin-id': connection_id,
-                    }})
+            if uid != 'private' and not self._previously_stored_contact(uid):
+                self._push_to_eds(
+                    uid=uid,
+                    name=fullname,
+                    link=connection.get(
+                        'siteStandardProfileRequest', {}).get('url'))
 
         return len(connections)

=== modified file 'friends/protocols/twitter.py'
--- friends/protocols/twitter.py	2013-07-20 02:55:28 +0000
+++ friends/protocols/twitter.py	2013-07-23 08:22:30 +0000
@@ -364,17 +364,12 @@
                 # https://dev.twitter.com/docs/api/1.1/get/users/show
                 full_contact = self._get_url(url=self._api_base.format(
                     endpoint='users/show') + '?user_id=' + contact_id)
-                user_fullname = full_contact.get('name')
-                user_nickname = full_contact.get('screen_name')
-                self._push_to_eds({
-                    '{}-id'.format(self._name): contact_id,
-                    '{}-name'.format(self._name): user_fullname,
-                    '{}-nick'.format(self._name): user_nickname,
-                    'X-URIS': self._user_home.format(user_id=user_nickname),
-                    'X-FOLKS-WEB-SERVICES-IDS': {
-                        'remote-full-name': user_fullname,
-                        '{}-id'.format(self._name): contact_id,
-                    }})
+                user_nickname = full_contact.get('screen_name', '')
+                self._push_to_eds(
+                    uid=contact_id,
+                    name=full_contact.get('name'),
+                    nick=user_nickname,
+                    link=self._user_home.format(user_id=user_nickname))
         return len(contacts)
 
 

=== modified file 'friends/tests/test_facebook.py'
--- friends/tests/test_facebook.py	2013-07-20 03:23:41 +0000
+++ friends/tests/test_facebook.py	2013-07-23 08:22:30 +0000
@@ -26,7 +26,7 @@
 import unittest
 import shutil
 
-from gi.repository import GLib, EDataServer
+from gi.repository import GLib, EDataServer, EBookContacts
 from pkg_resources import resource_filename
 
 from friends.protocols.facebook import Facebook
@@ -501,69 +501,61 @@
                        params={'access_token': 'broken'})])
         self.assertEqual(
             push.call_args_list,
-            [mock.call({
-                'X-FOLKS-WEB-SERVICES-IDS': {
-                    'jabber': '[e-mail]',
-                    'facebook-id': 'contact1',
-                    'remote-full-name': 'Joe Blow'},
-                'X-GENDER': 'male',
-                'facebook-name': 'Joe Blow',
-                'facebook-id': 'contact1',
-                'X-URIS': 'example.com',
-                'facebook-nick': 'jblow'}),
-             mock.call({
-                 'X-FOLKS-WEB-SERVICES-IDS': {
-                     'jabber': '[e-mail]',
-                     'facebook-id': 'contact2',
-                     'remote-full-name': 'Joe Blow'},
-                 'X-GENDER': 'male',
-                 'facebook-name': 'Joe Blow',
-                 'facebook-id': 'contact2',
-                 'X-URIS': 'example.com',
-                 'facebook-nick': 'jblow'})])
+            [mock.call(gender='male', jabber='[e-mail]',
+                       nick='jblow', link='example.com', name='Joe Blow',
+                       uid='contact1'),
+             mock.call(gender='male', jabber='[e-mail]',
+                       nick='jblow', link='example.com', name='Joe Blow',
+                       uid='contact2')])
 
     def test_create_contact(self, *mocks):
         # Receive the users friends.
-        bare_contact = {
-            'facebook-id':   '555555555',
-            'facebook-name': 'Lucy Baron',
-            'facebook-nick': 'lucy.baron5',
-            'X-URIS':        'http:www.facebook.com/lucy.baron5',
-            'X-GENDER':      'female',
-            'X-FOLKS-WEB-SERVICES-IDS': {
-                'jabber': '[e-mail]',
-                'remote-full-name': 'Lucy Baron',
-                'facebook-id': '555555555',
-            }}
-        eds_contact = self.protocol._create_contact(bare_contact)
+        eds_contact = self.protocol._create_contact(
+            uid='555555555',
+            name='Lucy Baron',
+            nick='lucy.baron5',
+            gender='female',
+            link='http:www.facebook.com/lucy.baron5',
+            jabber='[e-mail]')
         facebook_id_attr = eds_contact.get_attribute('facebook-id')
         self.assertEqual(facebook_id_attr.get_value(), '555555555')
-        facebook_name_attr = eds_contact.get_attribute('facebook-name')
-        self.assertEqual(facebook_name_attr.get_value(), 'Lucy Baron')
         web_service_addrs = eds_contact.get_attribute('X-FOLKS-WEB-SERVICES-IDS')
         params= web_service_addrs.get_params()
-
-        self.assertEqual(len(params), 3)
-
-        test_jabber = False
-        test_remote_name = False
-        test_facebook_id = False
-
-        for p in params:
-            if p.get_name() == 'jabber':
-                self.assertEqual(len(p.get_values()), 1)
-                self.assertEqual(p.get_values()[0], '[e-mail]')
-                test_jabber = True
-            if p.get_name() == 'remote-full-name':
-                self.assertEqual(len(p.get_values()), 1)
-                self.assertEqual(p.get_values()[0], 'Lucy Baron')
-                test_remote_name = True
-            if p.get_name() == 'facebook-id':
-                self.assertEqual(len(p.get_values()), 1)
-                self.assertEqual(p.get_values()[0], '555555555')
-                test_facebook_id = True
-        # Finally test to ensure all key value pairs were tested
-        self.assertTrue(test_jabber and test_remote_name and test_facebook_id)
+        self.assertEqual(len(params), 5)
+
+        # Can't compare the vcard string directly because it is sorted randomly...
+        vcard = eds_contact.to_string(
+            EBookContacts.VCardFormat(1)).replace('\r\n ', '')
+        self.assertIn(
+            'social-networking-attributes.X-URIS:http:www.facebook.com/lucy.baron5',
+            vcard)
+        self.assertIn(
+            'social-networking-attributes.X-GENDER:female',
+            vcard)
+        self.assertIn(
+            'social-networking-attributes.facebook-id:555555555',
+            vcard)
+        self.assertIn(
+            'FN:Lucy Baron',
+            vcard)
+        self.assertIn(
+            'NICKNAME:lucy.baron5',
+            vcard)
+        self.assertIn(
+            'social-networking-attributes.X-FOLKS-WEB-SERVICES-IDS;',
+            vcard)
+        self.assertIn(
+            'remote-full-name="Lucy Baron"',
+            vcard)
+        self.assertIn(
+            'facebook-id=555555555',
+            vcard)
+        self.assertIn(
+            'jabber="[e-mail]"',
+            vcard)
+        self.assertIn(
+            'facebook-nick="lucy.baron5"',
+            vcard)
 
     @mock.patch('friends.utils.base.Base._prepare_eds_connections',
                 return_value=True)
@@ -571,21 +563,21 @@
                 return_value=EDSBookClientMock())
     def test_successfull_push_to_eds(self, *mocks):
         bare_contact = {'name': 'Lucy Baron',
-                        'id': '555555555',
-                        'username': 'lucy.baron5',
+                        'uid': '555555555',
+                        'nick': 'lucy.baron5',
                         'link': 'http:www.facebook.com/lucy.baron5'}
         self.protocol._address_book = 'test-address-book'
         client = self.protocol._book_client = mock.Mock()
         client.add_contact_sync.return_value = True
         # Implicitely fail test if the following raises any exceptions
-        self.protocol._push_to_eds(bare_contact)
+        self.protocol._push_to_eds(**bare_contact)
 
     @mock.patch('friends.utils.base.Base._prepare_eds_connections',
                 return_value=None)
     def test_unsuccessfull_push_to_eds(self, *mocks):
         bare_contact = {'name': 'Lucy Baron',
-                        'id': '555555555',
-                        'username': 'lucy.baron5',
+                        'uid': '555555555',
+                        'nick': 'lucy.baron5',
                         'link': 'http:www.facebook.com/lucy.baron5'}
         self.protocol._address_book = 'test-address-book'
         client = self.protocol._book_client = mock.Mock()
@@ -593,7 +585,7 @@
         self.assertRaises(
             ContactsError,
             self.protocol._push_to_eds,
-            bare_contact,
+            **bare_contact
             )
 
     @mock.patch('gi.repository.EBook.BookClient.connect_sync',

=== modified file 'friends/tests/test_identica.py'
--- friends/tests/test_identica.py	2013-07-20 03:23:41 +0000
+++ friends/tests/test_identica.py	2013-07-23 08:22:30 +0000
@@ -263,17 +263,7 @@
             [mock.call('1'), mock.call('2')])
         self.assertEqual(
             push.call_args_list,
-            [mock.call({'identica-id': '1',
-                        'X-FOLKS-WEB-SERVICES-IDS': {
-                            'identica-id': '1',
-                            'remote-full-name': 'Bob'},
-                        'X-URIS': 'https://identi.ca/bobby',
-                        'identica-name': 'Bob',
-                        'identica-nick': 'bobby'}),
-             mock.call({'identica-id': '2',
-                        'X-FOLKS-WEB-SERVICES-IDS': {
-                            'identica-id': '2',
-                            'remote-full-name': 'Bob'},
-                        'X-URIS': 'https://identi.ca/bobby',
-                        'identica-name': 'Bob',
-                        'identica-nick': 'bobby'})])
+            [mock.call(link='https://identi.ca/bobby', nick='bobby',
+                       uid='1', name='Bob'),
+             mock.call(link='https://identi.ca/bobby', nick='bobby',
+                       uid='2', name='Bob')])

=== modified file 'friends/tests/test_linkedin.py'
--- friends/tests/test_linkedin.py	2013-07-20 01:38:59 +0000
+++ friends/tests/test_linkedin.py	2013-07-23 08:22:30 +0000
@@ -122,31 +122,7 @@
         self.assertEqual(self.protocol.contacts(), 4)
         self.assertEqual(
             push.mock_calls,
-            [mock.call(
-                {'X-URIS': 'https://www.linkedin.com',
-                 'X-FOLKS-WEB-SERVICES-IDS': {
-                     'linkedin-id': 'IFDI',
-                     'remote-full-name': 'H A'},
-                 'linkedin-name': 'H A',
-                 'linkedin-id': 'IFDI'}),
-             mock.call(
-                 {'X-URIS': 'https://www.linkedin.com',
-                  'X-FOLKS-WEB-SERVICES-IDS': {
-                      'linkedin-id': 'AefF',
-                      'remote-full-name': 'C A'},
-                  'linkedin-name': 'C A',
-                  'linkedin-id': 'AefF'}),
-             mock.call(
-                 {'X-URIS': 'https://www.linkedin.com',
-                  'X-FOLKS-WEB-SERVICES-IDS': {
-                      'linkedin-id': 'DFdV',
-                      'remote-full-name': 'R A'},
-                  'linkedin-name': 'R A',
-                  'linkedin-id': 'DFdV'}),
-             mock.call(
-                 {'X-URIS': 'https://www.linkedin.com',
-                  'X-FOLKS-WEB-SERVICES-IDS': {
-                      'linkedin-id': 'xkBU',
-                      'remote-full-name': 'A Z'},
-                  'linkedin-name': 'A Z',
-                  'linkedin-id': 'xkBU'})])
+            [mock.call(link='https://www.linkedin.com', name='H A', uid='IFDI'),
+             mock.call(link='https://www.linkedin.com', name='C A', uid='AefF'),
+             mock.call(link='https://www.linkedin.com', name='R A', uid='DFdV'),
+             mock.call(link='https://www.linkedin.com', name='A Z', uid='xkBU')])

=== modified file 'friends/tests/test_twitter.py'
--- friends/tests/test_twitter.py	2013-07-20 03:23:41 +0000
+++ friends/tests/test_twitter.py	2013-07-23 08:22:30 +0000
@@ -714,17 +714,7 @@
             [mock.call('1'), mock.call('2')])
         self.assertEqual(
             push.call_args_list,
-            [mock.call({'twitter-id': '1',
-                        'X-FOLKS-WEB-SERVICES-IDS': {
-                            'twitter-id': '1',
-                            'remote-full-name': 'Bob'},
-                        'X-URIS': 'https://twitter.com/bobby',
-                        'twitter-name': 'Bob',
-                        'twitter-nick': 'bobby'}),
-             mock.call({'twitter-id': '2',
-                        'X-FOLKS-WEB-SERVICES-IDS': {
-                            'twitter-id': '2',
-                            'remote-full-name': 'Bob'},
-                        'X-URIS': 'https://twitter.com/bobby',
-                        'twitter-name': 'Bob',
-                        'twitter-nick': 'bobby'})])
+            [mock.call(link='https://twitter.com/bobby', uid='1',
+                       name='Bob', nick='bobby'),
+             mock.call(link='https://twitter.com/bobby', uid='2',
+                       name='Bob', nick='bobby')])

=== modified file 'friends/utils/base.py'
--- friends/utils/base.py	2013-07-20 02:21:49 +0000
+++ friends/utils/base.py	2013-07-23 08:22:30 +0000
@@ -568,48 +568,58 @@
         if self._eds_source is not None:
             self._book_client = EBook.BookClient.connect_sync(self._eds_source, None)
 
-    def _push_to_eds(self, contact):
+    def _push_to_eds(self, **contact_details):
         self._prepare_eds_connections()
-        contact = self._create_contact(contact)
+        contact = self._create_contact(**contact_details)
         if not self._book_client.add_contact_sync(contact, None):
             raise ContactsError('Failed to save contact {!r}'.format(contact))
 
     def _previously_stored_contact(self, search_term):
         self._prepare_eds_connections()
         query = EBookContacts.BookQuery.vcard_field_test(
-            '{}-id'.format(self._name), EBookContacts.BookQueryTest.IS, search_term)
+            self._name + '-id', EBookContacts.BookQueryTest.IS, search_term)
         success, result = self._book_client.get_contacts_sync(query.to_string(), None)
         if not success:
             raise ContactsError(
                 'Id field is missing in {} address book.'.format(self._Name))
         return len(result) > 0
 
-    def _create_contact(self, info):
+    def _create_contact(self, uid, name, nick=None, link=None, gender=None, **folks):
         """Build a VCard based on a dict representation of a contact."""
         contact = EBookContacts.Contact.new()
 
-        for key, value in info.items():
+        folks.update({
+            'remote-full-name': name,
+            self._name + '-id': uid,
+            self._name + '-name': name,
+            self._name + '-nick': nick,
+        })
+
+        def add_attr(key, value=None, **params):
             attr = EBookContacts.VCardAttribute.new(
                 'social-networking-attributes', key)
-            if hasattr(value, 'items'):
-                for subkey, subval in value.items():
+            if value is not None:
+                attr.add_value(value)
+            elif params:
+                for subkey, subval in params.items():
                     if subval is not None:
                         param = EBookContacts.VCardAttributeParam.new(subkey)
                         param.add_value(subval)
                         attr.add_param(param);
-            elif value is not None:
-                attr.add_value(value)
             else:
-                continue
+                return
             contact.add_attribute(attr)
 
-        properties = (('full-name', '{}-name'),
-                      ('nickname', '{}-nick'))
-        for prop, key in properties:
-            value = info.get(key.format(self._name))
-            if value is not None:
-                contact.set_property(prop, value)
-                log.debug('New contact got {}: {}'.format(prop, value))
+        add_attr(self._name + '-id', uid)
+        add_attr('X-GENDER', gender)
+        add_attr('X-URIS', link)
+        add_attr('X-FOLKS-WEB-SERVICES-IDS', **folks)
+
+        contact.set_property('full-name', name)
+        if nick:
+            contact.set_property('nickname', nick)
+        # X-TREME debugging!
+        # print(contact.to_string(EBookContacts.VCardFormat(1)))
         return contact
 
     @feature