Merge lp:~kai-mast/friends/fetch-twitter-images into lp:friends

[Kai Mast]

Maybe this was too easy and I did something wrong. But for me this small change resulted in twitter posts now properly showing images.

[Robert Bruce Park (robru)]

Please bring back the trailing comma at the end of url=permalink. It will make the diff nicer.

review: Needs Fixing

[lp:~kai-mast/friends/fetch-twitter-images]

246. By Kai Mast on 2013-12-03

Added trailing comma

[Kai Mast (kai-mast)]

Done :)

[Robert Bruce Park (robru)]

Please specify a default value for picture because python's None value i don't think will work. So:

  picture = url.get('media_url', '')

Oh and please add a test for this as well (look at the existing tests, you might have to add some extra json if there isn't already a media_url key present in the example data).

[lp:~kai-mast/friends/fetch-twitter-images]

247. By Kai Mast on 2013-12-04

Fixed most tests

248. By Kai Mast on 2013-12-04

Fixed final test

249. By Kai Mast on 2013-12-04

added a test for fetching the media_url

[Kai Mast (kai-mast)]

Tests added. The diff got a lot larger now because I changed the code to only adding the media_url if it exists (otherwise the tests break).

review: Needs Resubmitting

[Kai Mast (kai-mast)]

Btw, considering the second TODO I added. I was wondering, if I can just add a html tag at the protocol level? Or will this break internal processing?

[Robert Bruce Park (robru)]

What? No, I don't like your args dict creation at all. Put back the self._publish call the way it was, and just update the tests to reflect the new argument being called.

Also I'm not sure what you mean by "adding an html tag at the protocol level". The url field can only be one URL, because that's how friends-app interprets it. Putting HTML in there will break how friends-app displays it.

[Robert Bruce Park (robru)]

Also, why did you review as 'Resubmit'? Do you intend to create a new MP?

[Adolfo Jayme Barrientos (fitojb)]

> Also, why did you review as 'Resubmit'? Do you intend to create a new MP?
If Launchpad was less confusing... When I created my first MPs, I thought "Resubmit" was a way to indicate that a change was made to the MP - MP reviews display similarly to bug statuses in LP, so one can think they work the same way. </sorry_for_intruding>

[Robert Bruce Park (robru)]

Adolfo, are there any launchpad projects you don't subscribe to? ;-)

[Kai Mast (kai-mast)]

@Adolfo: yes that is exactly what I thought

Robert, if I don't use the args dictionary I woudl have to add link_picture='' to some of the tests. That was in my eyes a worse than adding the dictionary, but if you prefer that I will do so.

What I mean with the other post was the following: The t.co link is currently replaced with the long link instead of the pretty display link. The easiest way to solve this would be to directly insert an html tag at line 37.
But there seems to be some processing at a higer level that replaces links by HTML tags, so I don't know it if this would break in such a case...

[Kai Mast (kai-mast)]

I just tested the latter on my branch and it seems to work :)

So we would have:
<a href='http://twitter.com/yunorno/status/114080493036773378/photo/1'>pic.twitter.com/rJC5Pxsu</a>

Instead of:
<a href="http://twitter.com/yunorno/status/114080493036773378/photo/1">http://twitter.com/yunorno/status/114080493036773378/photo/1</a>

[Robert Bruce Park (robru)]

Oh right, it's starting to come back to me now, we do have some code in the linkifier regex that stops it linkifying links that are already linkified, so that just might work.

yes, please change back the args dict / _publish call and update all the tests to expect "link_picture=''". I find a dozen lines of "args args args args args args args" with the accompanying bracket notation really ugly. Strongly prefer direct kwargs on the _publish method invocation.

[lp:~kai-mast/friends/fetch-twitter-images]

250. By Kai Mast on 2013-12-04

Fixed displaying URLs

251. By Kai Mast on 2013-12-05

Removed args dictionary

252. By Kai Mast on 2013-12-05

Fixed indendation of publish call

253. By Kai Mast on 2013-12-05

Removed spaces in params

[Kai Mast (kai-mast)]

Args dictionary removed...

[lp:~kai-mast/friends/fetch-twitter-images]

254. By Kai Mast on 2013-12-05

Fixed a small issue that was not cached by the test

[Robert Bruce Park (robru)]

Ok, this is starting to look really good to me now ;-)

Please:

1. Strip trailing whitespace

2. Add trailing comma to last argument of self._publish() (adding it now means the diff won't be ugly later if we have to add another argument at the end)

3. "picture = ''; if 'media_url' in url: picture = url.get('media_url')" should just be "picture = url.get('media_url', '')"

4. "if display_url is not ''" is really ugly. At the very least, it should just be "if display_url:", however I don't like the two different "message = message[...]" lines, I think we can do better than this. Please try something like this instead:

expanded_url = url.get('expanded_url', '')
display_url = url.get('display_url', '')
other_url = url.get('url', '')
picture_url = url.get('media_url', '')

if None not in (begin, end):
    message = ''.join(
        message[:begin],
        '<a href="',
        (expanded_url or display_url or other_url),
        '">',
        (display_url or expanded_url or other_url),
        '</a>',
        message[end:])

This is a much more concise/pythonic way of expressing the same thought, and it's also a little bit more robust in the case of missing keys from the url dict.

review: Needs Fixing

[lp:~kai-mast/friends/fetch-twitter-images]

255. By Kai Mast on 2013-12-05

Formatted code as suggested

256. By Kai Mast on 2013-12-05

Removed trailing whitespace

257. By Kai Mast on 2013-12-05

Fixed wrong indentation

[Kai Mast (kai-mast)]

I hink I did what you said.

Its my first time touching reallife python code so I have no idea of coding style. Thanks for the feedback.

[Robert Bruce Park (robru)]

Ok, yes it looks very good to me now. I see you caught a bug I wrote, since I forgot to put the arguments to 'join' in a list ;-)

I will try to test this a bit more thoroughly later today. Thanks!

[Robert Bruce Park (robru)]

Ok, gave this a proper test, it looks really good! Thanks!

review: Approve

=== modified file 'friends/protocols/twitter.py'
--- friends/protocols/twitter.py	2013-07-23 08:17:16 +0000
+++ friends/protocols/twitter.py	2013-12-05 19:59:40 +0000
@@ -119,16 +119,29 @@
             tweet_id=tweet_id)
 
         message = tweet.get('text', '')
+        picture_url = ''
 
         # Resolve t.co links.
+        #TODO support more than one media file
         entities = tweet.get('entities', {})
         for url in (entities.get('urls', []) + entities.get('media', [])):
             begin, end = url.get('indices', (None, None))
-            destination = (url.get('expanded_url') or
-                           url.get('display_url') or
-                           url.get('url'))
-            if None not in (begin, end, destination):
-                message = message[:begin] + destination + message[end:]
+            
+            expanded_url = url.get('expanded_url', '')
+            display_url = url.get('display_url', '')
+            other_url = url.get('url', '')
+            picture_url = url.get('media_url', '')
+
+            # Friends has no notion of display URLs, so this is handled at the protocol level
+            if None not in (begin, end):
+                message = ''.join([
+                    message[:begin],
+                    '<a href="',
+                    (expanded_url or display_url or other_url),
+                    '">',
+                    (display_url or expanded_url or other_url),
+                    '</a>',
+                    message[end:]])
 
         self._publish(
             message_id=tweet_id,
@@ -142,6 +155,7 @@
             icon_uri=avatar_url.replace('_normal.', '.'),
             liked=tweet.get('favorited', False),
             url=permalink,
+            link_picture=picture_url,
             )
         return permalink
 
 
=== modified file 'friends/tests/data/twitter-home.dat'
--- friends/tests/data/twitter-home.dat	2012-10-13 01:27:15 +0000
+++ friends/tests/data/twitter-home.dat	2013-12-05 19:59:40 +0000
@@ -100,17 +100,18 @@
         "id_str": "240556426106372096",
         "in_reply_to_user_id_str": null,
         "entities": {
-          "urls": [
-            {
-              "expanded_url": "http://blogs.ischool.berkeley.edu/i290-abdt-s12/",
-              "url": "http://t.co/bfj7zkDJ",
-              "indices": [
-                79,
-                99
-              ],
-              "display_url": "blogs.ischool.berkeley.edu/i290-abdt-s12/"
-            }
-          ],
+          "media":[{"type":"photo",
+                "sizes":{
+                        "thumb":{"h":150, "resize":"crop", "w":150},
+                        "large":{"h":238, "resize":"fit", "w":226},
+                        "medium":{"h":238, "resize":"fit", "w":226},
+                        "small":{"h":238, "resize":"fit", "w":226}},
+                 "indices":[78,98], "url":"http://t.co/rJC5Pxsu",
+                 "media_url":"http://p.twimg.com/AZVLmp-CIAAbkyy.jpg",
+                 "display_url":"pic.twitter.com\/rJC5Pxsu",
+                 "id":114080493040967680, "id_str":"114080493040967680",
+                 "expanded_url": "http://twitter.com/yunorno/status/114080493036773378/photo/1",
+                 "media_url_https":"https://p.twimg.com/AZVLmp-CIAAbkyy.jpg"}],
           "hashtags": [
 
           ],
@@ -137,7 +138,7 @@
             }
           ]
         },
-        "text": "lecturing at the \"analyzing big data with twitter\" class at @cal with @othman  http://t.co/bfj7zkDJ",
+        "text": "lecturing at the \"analyzing big data with twitter\" class at @cal with @othman http://t.co/rJC5Pxsu",
         "contributors": null,
         "id": 240556426106372096,
         "retweet_count": 3,
 
=== modified file 'friends/tests/test_twitter.py'
--- friends/tests/test_twitter.py	2013-11-01 17:27:51 +0000
+++ friends/tests/test_twitter.py	2013-12-05 19:59:40 +0000
@@ -133,6 +133,7 @@
         self.assertEqual(3, TestModel.get_n_rows())
 
         # This test data was ripped directly from Twitter's API docs.
+        
         expected = [
             ['twitter', 88, '240558470661799936',
              'messages', 'OAuth Dancer', '119476949', 'oauth_dancer', False,
@@ -144,13 +145,13 @@
             ['twitter', 88, '240556426106372096',
              'messages', 'Raffi Krikorian', '8285392', 'raffi', False,
              '2012-08-28T21:08:15Z', 'lecturing at the "analyzing big data '
-             'with twitter" class at @cal with @othman  '
-             '<a href="http://blogs.ischool.berkeley.edu/i290-abdt-s12/">'
-             'http://blogs.ischool.berkeley.edu/i290-abdt-s12/</a>',
+             'with twitter" class at @cal with @othman '
+             '<a href="http://twitter.com/yunorno/status/114080493036773378/photo/1">'
+             'pic.twitter.com/rJC5Pxsu</a>', 
              'https://si0.twimg.com/profile_images/1270234259/'
              'raffi-headshot-casual.png',
              'https://twitter.com/raffi/status/240556426106372096',
-             0, False, '', '', '', '', '', '', '', 0.0, 0.0,
+             0, False, 'http://p.twimg.com/AZVLmp-CIAAbkyy.jpg', '', '', '', '', '', '', 0.0, 0.0,
              ],
             ['twitter', 88, '240539141056638977',
              'messages', 'Taylor Singletary', '819797', 'episod', False,
@@ -313,6 +314,7 @@
             liked=False, sender='Bob', stream='private',
             url='https://twitter.com/some_guy/status/1452456',
             icon_uri='https://example.com/bob.jpg',
+            link_picture='',
             sender_nick='some_guy', sender_id='', from_me=False,
             timestamp='2012-11-04T17:14:52Z', message='Does my avatar show up?',
             message_id='1452456')