Merge lp:~michael.nelson/ubuntu-webcatalog/987838-no-screenshots-for-extras-2 into lp:ubuntu-webcatalog

[Michael Nelson]

Extract Screenshot-Url from packages metadata when present.

Overview
========

The first branch for this bug fixed an apparently unrelated issue, this branch fixes the actual issue that we were not pulling the screenshot url out of the package (which is where it is defined for packages in extra).

`fab test`

Details
=======
I switched off ImportAppInstallTestCase.use_mock_apt_cache and ran a test to find out where i could find the screenshot url... I didn't see a key for it on package or package.candidate but grabbed it from package.candidateRecord [1]

Once coded, I've run the test (for precise) with use_mock_apt_cache=False to ensure the screenshot is set for the 'leds' package. The only uncertainty that i need to check is that package.candidateRecord exists on lucid (Edit: it's available on lucid also [2]).

You can demonstrate this working locally [3].

[1] https://pastebin.canonical.com/65727/
[2] https://pastebin.canonical.com/65733/
[3] https://pastebin.canonical.com/65734/

[Natalia Bidart (nataliabidart)]

Branch look pretty awesome.

One tiny note, instead of:

if 'Screenshot-Url' in package.candidateRecord:
    screenshot_url = package.candidateRecord['Screenshot-Url']

I usually prefer something like:

screenshot_url = package.candidateRecord.get('Screenshot-Url')
if screenshot_url is not None and screenshot_url not in app.screenshots:
    app.applicationmedia_set.create(media_type='screenshot', url=screenshot_url)

The benefits I see in the second option are:

* you access the package.candidateRecord dict only once
* you do not duplicate the literal 'Screenshot-Url' and thus avoid risk of typos
* you avoid an if (and then another inner level in the logic)

So, if you think is worth it, feel free to tweak the code in this MP. I'll be approving anyways since this looks great.

review: Approve

[Michael Nelson (michael.nelson)]

On Wed, May 9, 2012 at 4:42 PM, Natalia Bidart
<email address hidden> wrote:
> One tiny note, instead of:
>
> if 'Screenshot-Url' in package.candidateRecord:
>    screenshot_url = package.candidateRecord['Screenshot-Url']
>
> I usually prefer something like:
>
> screenshot_url = package.candidateRecord.get('Screenshot-Url')
> if screenshot_url is not None and screenshot_url not in app.screenshots:
>    app.applicationmedia_set.create(media_type='screenshot', url=screenshot_url)
>
> The benefits I see in the second option are:
>
> * you access the package.candidateRecord dict only once
> * you do not duplicate the literal 'Screenshot-Url' and thus avoid risk of typos
> * you avoid an if (and then another inner level in the logic)
>
> So, if you think is worth it, feel free to tweak the code in this MP. I'll be approving anyways since this looks great.

Indeed - thanks! pushed with r123.

[lp:~michael.nelson/ubuntu-webcatalog/987838-no-screenshots-for-extras-2]

123. By Michael Nelson on 2012-05-09

REFACTOR: improve screenshot_url detection after review.

=== modified file 'src/webcatalog/management/commands/import_app_install_data.py'
--- src/webcatalog/management/commands/import_app_install_data.py	2012-04-20 16:57:50 +0000
+++ src/webcatalog/management/commands/import_app_install_data.py	2012-05-09 15:34:21 +0000
@@ -313,6 +313,13 @@
                     version=version,
                     comment=app_comment,
                     )
+
+            screenshot_url = package.candidateRecord.get('Screenshot-Url')
+            if screenshot_url is not None and (
+                    screenshot_url not in app.screenshots):
+                app.applicationmedia_set.create(
+                    media_type='screenshot', url=screenshot_url)
+
             self.fetch_icon_from_extras(app, candidate)
         if self.verbosity == 1:
             self.output("\n", 1)

=== modified file 'src/webcatalog/tests/test_commands.py'
--- src/webcatalog/tests/test_commands.py	2012-05-03 12:13:00 +0000
+++ src/webcatalog/tests/test_commands.py	2012-05-09 15:34:21 +0000
@@ -89,7 +89,8 @@
     # firefox and scribus in the cache.
     use_mock_apt_cache = True
 
-    def make_mock_apt_package(self, name, description, uri='', summary=''):
+    def make_mock_apt_package(self, name, description, uri='', summary='',
+            screenshot_url=''):
         """Helper to DRY up creating a mock apt package."""
         mock_package = Mock(spec=apt.package.Package)
         mock_package.candidate.description = description
@@ -98,6 +99,10 @@
         mock_package.candidate.section = ''
         mock_package.candidate.summary = summary
         mock_package.candidate.version = '1.2.3-4ubuntu1~dev'
+        apt_record = {}
+        if screenshot_url:
+            apt_record['Screenshot-Url'] = screenshot_url
+        mock_package.candidateRecord = apt_record
         return mock_package
 
     def make_mock_apt_cache(self):
@@ -112,7 +117,8 @@
             description="Scribus description")
         mock_other_app = self.make_mock_apt_package('otherapp',
             description="Otherapp description",
-            summary="Otherapp the Internet\nA tagline for Otherapp")
+            summary="Otherapp the Internet\nA tagline for Otherapp",
+            screenshot_url="http://example.com/screenshot_for_otherapp.png")
         mock_app_install_data = self.make_mock_apt_package('app-install-data',
             description="App install desc.",
             uri='http://example.com/app-install-1.01.deb')
@@ -283,6 +289,21 @@
         self.assertTrue(firefox.icon.size > 1)
         self.assertTrue(scribus.icon.size > 1)
 
+    def test_screenshot_added_to_model(self):
+        call_command(
+            'import_app_install_data', 'natty',
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
+            verbosity=0)
+
+        otherapp = Application.objects.get(package_name='otherapp')
+        self.assertEqual(1, otherapp.applicationmedia_set.count())
+        app_media = otherapp.applicationmedia_set.all()[0]
+        self.assertEqual('screenshot', app_media.media_type)
+        self.assertEqual(
+            "http://example.com/screenshot_for_otherapp.png",
+            app_media.url)
+
     def get_package_uri_for_series(self):
         uri = import_app_install_data.Command().get_package_uri_for_series(
             'app-install-data', 'natty')