Merge lp:~phanimahesh/friends/friends into lp:friends

[J Phani Mahesh]

Proposed fix for LP:#1185684

Corrects the url on posts from facebook feed to point at the permalink instead of the timeline.

[Robert Bruce Park (robru)]

Hi, thanks for submitting this patch. It looks nice and simple, but I really need to see some test coverage with it. Make sure you run the test suite and update the tests in test_facebook.py in order to reflect this change.

review: Needs Fixing

[J Phani Mahesh (phanimahesh)]

I'm not familiar with 'mock', which seems to be used for the unit tests. I've noticed that the tests fail, but was unable to identify what caused the failure. I'll give it a try once again.
Do you have any tips that might help in finding the cause of failure?

I've patched up my installation ( dog-fooding? :P) and it works great for me.

[Robert Bruce Park (robru)]

I had a quick look at the tests, and what I saw was that the mock data we
use in the testsuite doesn't have an underscore, so it's raising IndexError
because the call to split is ineffective. So you'll have to read the
traceback, and then follow the code path backwards to see where the mock
data is being defined (usually within each test it should declare some
dummy data). then you'll have to change the value to have the expected
number of underscores in it, with some arbitrary letters or numbers in
between. once you have that in place, then what'll happen is that the tests
will fail, because they expect the old timeline URL and not the new page
URL you're implementing. so just update the actual assertions to start
expecting the new URL, and that should pretty much do it.

On Mon, Jun 10, 2013 at 11:40 PM, J Phani Mahesh <
<email address hidden>> wrote:

> I'm not familiar with 'mock', which seems to be used for the unit tests.
> I've noticed that the tests fail, but was unable to identify what caused
> the failure. I'll give it a try once again.
> Do you have any tips that might help in finding the cause of failure?
>
> I've patched up my installation ( dog-fooding? :P) and it works great for
> me.
> --
> https://code.launchpad.net/~phanimahesh/friends/friends/+merge/166449
> You are reviewing the proposed merge of lp:~phanimahesh/friends/friends
> into lp:friends.
>

[J Phani Mahesh (phanimahesh)]

Hello Robert!

Thanks for the pointers. That was easy. please find the tests updated.

[Robert Bruce Park (robru)]

Great work, thanks!

review: Approve

=== modified file 'friends/protocols/facebook.py'
--- friends/protocols/facebook.py	2013-04-02 21:30:43 +0000
+++ friends/protocols/facebook.py	2013-06-12 06:40:33 +0000
@@ -39,7 +39,7 @@
 API_BASE = URL_BASE.format(subdomain='graph') + '{id}'
 ME_URL = API_BASE.format(id='me')
 FACEBOOK_ADDRESS_BOOK = 'friends-facebook-contacts'
-
+STORY_PERMALINK = PERMALINK + '/posts/{post_id}'
 
 TEN_DAYS = 864000 # seconds
 
@@ -93,12 +93,15 @@
         if from_record is not None:
             args['sender'] = from_record.get('name', '')
             args['sender_id'] = sender_id = from_record.get('id', '')
-            args['url'] = PERMALINK.format(id=sender_id)
             args['icon_uri'] = Avatar.get_image(
                 API_BASE.format(id=sender_id) + '/picture?width=840&height=840')
             args['sender_nick'] = from_record.get('name', '')
             args['from_me'] = (sender_id == self._account.user_id)
 
+        # Fix for LP:1185684 - JPM
+        post_id= message_id.split('_')[1]
+        args['url'] = STORY_PERMALINK.format(id=sender_id,post_id=post_id)
+
         # Normalize the timestamp.
         timestamp = entry.get('updated_time', entry.get('created_time'))
         if timestamp is not None:

=== modified file 'friends/tests/data/facebook-full.dat'
--- friends/tests/data/facebook-full.dat	2013-03-16 00:54:45 +0000
+++ friends/tests/data/facebook-full.dat	2013-06-12 06:40:33 +0000
@@ -1,7 +1,7 @@
 {
   "data": [
     {
-      "id": "fake_id",
+      "id": "userid_postid1",
       "from": {
         "name": "Yours Truly",
         "id": "56789"
@@ -10,11 +10,11 @@
       "actions": [
         {
           "name": "Comment",
-          "link": "https://www.facebook.com/fake/posts/id"
+          "link": "https://www.facebook.com/userid/posts/postid1"
         },
         {
           "name": "Like",
-          "link": "https://www.facebook.com/fake/posts/id"
+          "link": "https://www.facebook.com/userid/posts/postid1"
         }
       ],
       "privacy": {
@@ -46,7 +46,7 @@
       "comments": {
         "data": [
           {
-            "id": "fake as a snake",
+            "id": "postid1_commentid1",
             "from": {
               "name": "Grandma",
               "id": "9876"
@@ -55,7 +55,7 @@
             "created_time": "2013-03-12T22:56:17+0000"
           },
           {
-            "id": "faker than cake!",
+            "id": "postid1_commentid2",
             "from": {
               "name": "Father",
               "id": "234"
@@ -64,7 +64,7 @@
             "created_time": "2013-03-12T23:29:45+0000"
           },
           {
-            "id": "still fake",
+            "id": "postid1_commentid3",
             "from": {
               "name": "Mother",
               "id": "456"
@@ -73,7 +73,7 @@
             "created_time": "2013-03-13T02:20:27+0000"
           },
           {
-            "id": "this one is real",
+            "id": "postid1_commentid4",
             "from": {
               "name": "Yours Truly",
               "id": "56789"

=== modified file 'friends/tests/test_facebook.py'
--- friends/tests/test_facebook.py	2013-04-02 21:54:24 +0000
+++ friends/tests/test_facebook.py	2013-06-12 06:40:33 +0000
@@ -119,7 +119,7 @@
         self.assertEqual(list(TestModel.get_row(0)), [
             'facebook',
             88,
-            'fake_id',
+            'userid_postid1',
             'mentions',
             'Yours Truly',
             '56789',
@@ -131,7 +131,7 @@
             'to test with, that\'d be super.',
             GLib.get_user_cache_dir() +
             '/friends/avatars/5c4e74c64b1a09343558afc1046c2b1d176a2ba2',
-            'https://www.facebook.com/56789',
+            'https://www.facebook.com/56789/posts/postid1',
             1,
             False,
             '',
@@ -147,8 +147,8 @@
         self.assertEqual(list(TestModel.get_row(2)), [
             'facebook',
             88,
-            'faker than cake!',
-            'reply_to/fake_id',
+            'postid1_commentid2',
+            'reply_to/userid_postid1',
             'Father',
             '234',
             'Father',
@@ -157,7 +157,7 @@
             'don\'t know how',
             GLib.get_user_cache_dir() +
             '/friends/avatars/9b9379ccc7948e4804dff7914bfa4c6de3974df5',
-            'https://www.facebook.com/234',
+            'https://www.facebook.com/234/posts/commentid2',
             0,
             False,
             '',
@@ -188,7 +188,7 @@
             'producer, Sean Keegan. Stanley is in the lobby.',
             GLib.get_user_cache_dir() +
             '/friends/avatars/5b2d70e788df790b9c8db4c6a138fc4a1f433ec9',
-            'https://www.facebook.com/161247843901324',
+            'https://www.facebook.com/161247843901324/posts/629147610444676',
             84,
             False,
             'https://fbcdn-photos-a.akamaihd.net/hphotos-ak-snc7/' +
@@ -215,7 +215,7 @@
             'Guy Frenchie did some things with some stuff.',
             GLib.get_user_cache_dir() +
             '/friends/avatars/3f5e276af0c43f6411d931b829123825ede1968e',
-            'https://www.facebook.com/1244414',
+            'https://www.facebook.com/1244414/posts/100085049977',
             3,
             False,
             '',