Merge lp:~robru/friends/model-cleanup into lp:friends

[Robert Bruce Park]

Thread replies properly, and fix up likes/unlikes to modify model data upon success.

Alright Ken, just like you asked, replies are threading and likes/unlikes are updating the model contents as you'd expect. Not sure how we overlooked this originally ;-)

Anyway, I did the model alteration stuff with a fairly simple API in the Base class, rather than having to import the Model into the protocol classes, which is a bit hacky.

As usual, excellent test coverage is included.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:178
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robru/friends/model-cleanup/+merge/156704/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/friends-ci/16/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/friends-raring-amd64-ci/16

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

review: Needs Fixing (continuous-integration)

[Ken VanDine (ken-vandine)]

Looks good and does fix the issue. To test it I connected debug_slave.py to row-changed and called the like/unlike APIs. I consistently saw the value of likes get incremented/decremented along with the success callback, very quickly. However the value of liked gets changed separately, and a good 20 to 40 seconds later, which seems weird. The value does get changed though, just not seen on the model quickly.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

[Robert Bruce Park (robru)]

That is quite strange. I wonder if that might actually be a dbus synchronization issue rather than anything specific to the code I wrote. Because the way I wrote it, it changes the one value right after the other, although not in an atomic way.

=== modified file 'friends/protocols/facebook.py'
--- friends/protocols/facebook.py	2013-03-16 00:54:45 +0000
+++ friends/protocols/facebook.py	2013-04-02 21:50:28 +0000
@@ -226,6 +226,8 @@
         This includes messages, statuses, wall posts, events, etc.
         """
         self._like(obj_id, 'POST')
+        self._inc_cell(obj_id, 'likes')
+        self._set_cell(obj_id, 'liked', True)
         return obj_id
 
     @feature
@@ -235,9 +237,11 @@
         This includes messages, statuses, wall posts, events, etc.
         """
         self._like(obj_id, 'DELETE')
+        self._dec_cell(obj_id, 'likes')
+        self._set_cell(obj_id, 'liked', False)
         return obj_id
 
-    def _send(self, obj_id, message, endpoint):
+    def _send(self, obj_id, message, endpoint, stream='messages'):
         url = API_BASE.format(id=obj_id) + endpoint
         token = self._get_access_token()
 
@@ -251,7 +255,9 @@
 
         url = API_BASE.format(id=new_id)
         entry = Downloader(url, params=dict(access_token=token)).get_json()
-        return self._publish_entry(entry)
+        return self._publish_entry(
+            stream=stream,
+            entry=entry)
 
     @feature
     def send(self, message, obj_id='me'):
@@ -270,7 +276,8 @@
         obj_id can be the id of any Facebook object that supports being
         commented on, which will generally be Posts.
         """
-        return self._send(obj_id, message, '/comments')
+        return self._send(obj_id, message, '/comments',
+                          stream='reply_to/{}'.format(obj_id))
 
     @feature
     def delete(self, obj_id):
 
=== modified file 'friends/protocols/twitter.py'
--- friends/protocols/twitter.py	2013-03-20 01:24:15 +0000
+++ friends/protocols/twitter.py	2013-04-02 21:50:28 +0000
@@ -248,9 +248,10 @@
     def send_thread(self, message_id, message):
         """Send a reply message to message_id.
 
-        Note that you have to @mention the message_id owner's screen name in
-        order for Twitter to actually accept this as a reply.  Otherwise it
-        will just be an ordinary tweet.
+        This method takes care to prepend the @mention to the start of
+        your tweet if you forgot it. Without this, Twitter will just
+        consider it a regular message, and it won't be part of any
+        conversation.
         """
         try:
             sender = '@{}'.format(self._fetch_cell(message_id, 'sender_nick'))
@@ -261,7 +262,8 @@
         url = self._api_base.format(endpoint='statuses/update')
         tweet = self._get_url(url, dict(in_reply_to_status_id=message_id,
                                         status=message))
-        return self._publish_tweet(tweet)
+        return self._publish_tweet(
+            tweet, stream='reply_to/{}'.format(message_id))
 
 # https://dev.twitter.com/docs/api/1.1/post/statuses/destroy/%3Aid
     @feature
@@ -303,10 +305,9 @@
         """Announce to the world your undying love for a tweet."""
         url = self._api_base.format(endpoint='favorites/create')
         self._get_url(url, dict(id=message_id))
+        self._inc_cell(message_id, 'likes')
+        self._set_cell(message_id, 'liked', True)
         return message_id
-        # I don't think we need to publish this tweet because presumably the
-        # user has clicked the 'favorite' button on the message that's already
-        # in the stream.
 
 # https://dev.twitter.com/docs/api/1.1/post/favorites/destroy
     @feature
@@ -314,6 +315,8 @@
         """Renounce your undying love for a tweet."""
         url = self._api_base.format(endpoint='favorites/destroy')
         self._get_url(url, dict(id=message_id))
+        self._dec_cell(message_id, 'likes')
+        self._set_cell(message_id, 'liked', False)
         return message_id
 
 # https://dev.twitter.com/docs/api/1.1/get/search/tweets
 
=== modified file 'friends/tests/test_facebook.py'
--- friends/tests/test_facebook.py	2013-03-16 00:54:45 +0000
+++ friends/tests/test_facebook.py	2013-04-02 21:50:28 +0000
@@ -269,7 +269,8 @@
             'http://facebook.com/post_id')
 
         token.assert_called_once_with()
-        publish.assert_called_with({'id': 'post_id'})
+        publish.assert_called_with(entry={'id': 'post_id'},
+                                   stream='messages')
         self.assertEqual(
             dload.mock_calls,
             [mock.call(),
@@ -298,7 +299,8 @@
             'http://facebook.com/new_post_id')
 
         token.assert_called_once_with()
-        publish.assert_called_with({'id': 'post_id'})
+        publish.assert_called_with(entry={'id': 'post_id'},
+                                   stream='messages')
         self.assertEqual(
             dload.mock_calls,
             [mock.call(),
@@ -327,7 +329,8 @@
             'http://facebook.com/private_message_id')
 
         token.assert_called_once_with()
-        publish.assert_called_with({'id': 'comment_id'})
+        publish.assert_called_with(entry={'id': 'comment_id'},
+                                   stream='reply_to/post_id')
         self.assertEqual(
             dload.mock_calls,
             [mock.call(),
@@ -427,9 +430,13 @@
         dload().get_json.return_value = True
         token = self.protocol._get_access_token = mock.Mock(
             return_value='face')
+        inc_cell = self.protocol._inc_cell = mock.Mock()
+        set_cell = self.protocol._set_cell = mock.Mock()
 
         self.assertEqual(self.protocol.like('post_id'), 'post_id')
 
+        inc_cell.assert_called_once_with('post_id', 'likes')
+        set_cell.assert_called_once_with('post_id', 'liked', True)
         token.assert_called_once_with()
         dload.assert_called_with(
             'https://graph.facebook.com/post_id/likes',
@@ -441,9 +448,13 @@
         dload.get_json.return_value = True
         token = self.protocol._get_access_token = mock.Mock(
             return_value='face')
+        dec_cell = self.protocol._dec_cell = mock.Mock()
+        set_cell = self.protocol._set_cell = mock.Mock()
 
         self.assertEqual(self.protocol.unlike('post_id'), 'post_id')
 
+        dec_cell.assert_called_once_with('post_id', 'likes')
+        set_cell.assert_called_once_with('post_id', 'liked', False)
         token.assert_called_once_with()
         dload.assert_called_once_with(
             'https://graph.facebook.com/post_id/likes',
 
=== modified file 'friends/tests/test_identica.py'
--- friends/tests/test_identica.py	2013-03-14 19:14:03 +0000
+++ friends/tests/test_identica.py	2013-04-02 21:50:28 +0000
@@ -150,7 +150,7 @@
             '1234',
             'Why yes, I would love to respond to your tweet @pumpichank!')
 
-        publish.assert_called_with('tweet')
+        publish.assert_called_with('tweet', stream='reply_to/1234')
         get_url.assert_called_with(
             'http://identi.ca/api/statuses/update.json',
             dict(status=
 
=== modified file 'friends/tests/test_protocols.py'
--- friends/tests/test_protocols.py	2013-03-19 00:43:13 +0000
+++ friends/tests/test_protocols.py	2013-04-02 21:50:28 +0000
@@ -370,6 +370,55 @@
         self.assertEqual(TestModel.get_row(1)[COLUMN_INDICES['sender']],
                          'tedtholomew')
 
+    @mock.patch('friends.utils.base.Model', TestModel)
+    @mock.patch('friends.utils.base._seen_ids', {})
+    def test_inc_cell(self):
+        base = Base(FakeAccount())
+        self.assertEqual(0, TestModel.get_n_rows())
+        self.assertTrue(base._publish(message_id='1234', likes=10, liked=True))
+        base._inc_cell('1234', 'likes')
+        row = TestModel.get_row(0)
+        self.assertEqual(
+            list(row),
+            ['base', 88, '1234', '', '', '', '', False, '', '', '', '', 11,
+             True, '', '', '', '', '', '', '', 0.0, 0.0])
+
+    @mock.patch('friends.utils.base.Model', TestModel)
+    @mock.patch('friends.utils.base._seen_ids', {})
+    def test_dec_cell(self):
+        base = Base(FakeAccount())
+        self.assertEqual(0, TestModel.get_n_rows())
+        self.assertTrue(base._publish(message_id='1234', likes=10, liked=True))
+        base._dec_cell('1234', 'likes')
+        row = TestModel.get_row(0)
+        self.assertEqual(
+            list(row),
+            ['base', 88, '1234', '', '', '', '', False, '', '', '', '', 9,
+             True, '', '', '', '', '', '', '', 0.0, 0.0])
+
+    @mock.patch('friends.utils.base.Model', TestModel)
+    @mock.patch('friends.utils.base._seen_ids', {})
+    def test_set_cell(self):
+        base = Base(FakeAccount())
+        self.assertEqual(0, TestModel.get_n_rows())
+        self.assertTrue(base._publish(message_id='1234', likes=10, liked=True))
+        base._set_cell('1234', 'likes', 500)
+        base._set_cell('1234', 'liked', False)
+        base._set_cell('1234', 'message_id', '5678')
+        row = TestModel.get_row(0)
+        self.assertEqual(
+            list(row),
+            ['base', 88, '5678', '', '', '', '', False, '', '', '', '', 500,
+             False, '', '', '', '', '', '', '', 0.0, 0.0])
+
+    @mock.patch('friends.utils.base.Model', TestModel)
+    @mock.patch('friends.utils.base._seen_ids', {})
+    def test_fetch_cell(self):
+        base = Base(FakeAccount())
+        self.assertEqual(0, TestModel.get_n_rows())
+        self.assertTrue(base._publish(message_id='1234', likes=10, liked=True))
+        self.assertEqual(base._fetch_cell('1234', 'likes'), 10)
+
     def test_basic_login(self):
         # Try to log in twice.  The second login attempt returns False because
         # it's already logged in.
 
=== modified file 'friends/tests/test_twitter.py'
--- friends/tests/test_twitter.py	2013-03-19 04:17:58 +0000
+++ friends/tests/test_twitter.py	2013-04-02 21:50:28 +0000
@@ -383,7 +383,7 @@
                 'Why yes, I would love to respond to your tweet @pumpichank!'),
             'tweet permalink')
 
-        publish.assert_called_with('tweet')
+        publish.assert_called_with('tweet', stream='reply_to/1234')
         get_url.assert_called_with(
             'https://api.twitter.com/1.1/statuses/update.json',
             dict(status='Why yes, I would love to respond to your '
@@ -471,18 +471,26 @@
 
     def test_like(self):
         get_url = self.protocol._get_url = mock.Mock()
+        inc_cell = self.protocol._inc_cell = mock.Mock()
+        set_cell = self.protocol._set_cell = mock.Mock()
 
         self.assertEqual(self.protocol.like('1234'), '1234')
 
+        inc_cell.assert_called_once_with('1234', 'likes')
+        set_cell.assert_called_once_with('1234', 'liked', True)
         get_url.assert_called_with(
             'https://api.twitter.com/1.1/favorites/create.json',
             dict(id='1234'))
 
     def test_unlike(self):
         get_url = self.protocol._get_url = mock.Mock()
+        dec_cell = self.protocol._dec_cell = mock.Mock()
+        set_cell = self.protocol._set_cell = mock.Mock()
 
         self.assertEqual(self.protocol.unlike('1234'), '1234')
 
+        dec_cell.assert_called_once_with('1234', 'likes')
+        set_cell.assert_called_once_with('1234', 'liked', False)
         get_url.assert_called_with(
             'https://api.twitter.com/1.1/favorites/destroy.json',
             dict(id='1234'))
 
=== modified file 'friends/utils/base.py'
--- friends/utils/base.py	2013-03-20 01:19:39 +0000
+++ friends/utils/base.py	2013-04-02 21:50:28 +0000
@@ -503,15 +503,33 @@
             message = None
         raise FriendsError(message or str(error))
 
+    def _calculate_row_cell(self, message_id, column_name):
+        """Find x,y coords in the model based on message_id and column_name."""
+        row_id = _seen_ids.get(message_id)
+        col_idx = COLUMN_INDICES.get(column_name)
+        if None in (row_id, col_idx):
+            raise FriendsError('Cell could not be found.')
+        return row_id, col_idx
+
     def _fetch_cell(self, message_id, column_name):
         """Find a column value associated with a specific message_id."""
-        row_id = _seen_ids.get(message_id)
-        col_idx = COLUMN_INDICES.get(column_name)
-        if None not in (row_id, col_idx):
-            row = Model.get_row(row_id)
-            return row[col_idx]
-        else:
-            raise FriendsError('Value could not be found.')
+        row_id, col_idx = self._calculate_row_cell(message_id, column_name)
+        return Model.get_row(row_id)[col_idx]
+
+    def _set_cell(self, message_id, column_name, value):
+        """Set a column value associated with a specific message_id."""
+        row_id, col_idx = self._calculate_row_cell(message_id, column_name)
+        Model.get_row(row_id)[col_idx] = value
+
+    def _inc_cell(self, message_id, column_name):
+        """Increment a column value associated with a specific message_id."""
+        row_id, col_idx = self._calculate_row_cell(message_id, column_name)
+        Model.get_row(row_id)[col_idx] += 1
+
+    def _dec_cell(self, message_id, column_name):
+        """Decrement a column value associated with a specific message_id."""
+        row_id, col_idx = self._calculate_row_cell(message_id, column_name)
+        Model.get_row(row_id)[col_idx] -= 1
 
     def _new_book_client(self, source):
         client = EBook.BookClient.new(source)