Merge lp:~elachuni/ubuntu-webcatalog/import-partner into lp:ubuntu-webcatalog

[Anthony Lenton]

Made import_app_install_data also sync apps from the partner repo

Overview
========
This branch refactors import_app_install_data, and makes it also sync apps from archive.canonical.com (using metadata from app-install-data-partner).

Details
=======
The biggest refactor change is to the Cache class, that is no longer a context manager. It didn't seem to be necessary, as it was only using the __exit__ method to clean up a temporary directory that shouldn't have been created in the first place.

Besides that, the changes are mainly so that the command also processes app-install-data-partner, like it does app-install-data.

The Application.section field now uses blank=True, because desktop files in app-install-data-partner don't provide the x-appinstall-section field, and the right department can be inferred from the category.

Finally, import_app_install_data.Command.get_screenshot_urls_from_server was patched out during tests so that it doesn't hit the wire at all.

[Danny Tamez (zematynnad)]

Nice. works great.

review: Approve

=== modified file 'src/webcatalog/management/commands/import_app_install_data.py'
--- src/webcatalog/management/commands/import_app_install_data.py	2012-03-28 12:45:14 +0000
+++ src/webcatalog/management/commands/import_app_install_data.py	2012-03-29 22:02:21 +0000
@@ -51,7 +51,12 @@
     ]
 
 
-class Cache(object):
+class CacheMaker(object):
+    sources = """
+deb http://archive.ubuntu.com/ubuntu {distroseries} main universe
+deb http://archive.ubuntu.com/ubuntu {distroseries}-updates main universe
+deb http://archive.canonical.com/ubuntu {distroseries} partner
+"""
 
     def __init__(self, distroseries):
         self.distroseries = distroseries
@@ -61,12 +66,8 @@
         sources_list = os.path.join(
             series_cache_location, 'etc', 'apt', 'sources.list')
         with open(sources_list, "w+") as sources_list_file:
-            sources_list_file.write(
-                'deb http://archive.ubuntu.com/ubuntu {distroseries}'
-                ' main universe\n'
-                'deb http://archive.ubuntu.com/ubuntu {distroseries}-updates'
-                ' main universe\n'
-                .format(distroseries=self.distroseries))
+            sources_list_file.write(self.sources.format(
+                distroseries=self.distroseries))
 
     def get_apt_cache(self):
         # Create on-disk cache for distroseries if it
@@ -76,20 +77,7 @@
         if not os.path.exists(series_cache_location):
             self.create_apt_cache(series_cache_location)
 
-        return apt.Cache(rootdir=series_cache_location)
-
-    def __enter__(self):
-        tmp_sources_dir = tempfile.mkdtemp()
-        self.tmp_sources_dir = tmp_sources_dir
-        os.makedirs(os.path.join(tmp_sources_dir, 'etc', 'apt'))
-        tmp_sources_list = os.path.join(
-            tmp_sources_dir, 'etc', 'apt', 'sources.list')
-        with open(tmp_sources_list, "w+") as sources_list:
-            sources_list.write(
-                'deb http://archive.ubuntu.com/ubuntu {0} main\n'.format(
-                    self.distroseries))
-
-        cache = self.get_apt_cache()
+        cache = apt.Cache(rootdir=series_cache_location)
         try:
             # XXX michaeln bug=879268 Lucid doesnt know about translations.
             # As long as our server is running on Lucid, we need to ensure
@@ -103,26 +91,39 @@
         cache.open()
         return cache
 
-    def __exit__(self, exc_type, exc_value, exc_traceback):
-        shutil.rmtree(self.tmp_sources_dir)
-
 
 class Command(LabelCommand):
 
-    help = "Update Application data from app-install-data package."
+    help = ("Update Application data from apt cache and "
+        "app-install-data(-partner) packages.")
     option_list = LabelCommand.option_list + (
-        make_option('--local-app-install-deb',
+        make_option('--local-app-install-data',
             action='store',
-            dest='local_app_install_deb',
+            dest='local_app_install_data',
             default='',
             help=('Use a local app-install-data deb package rather than '
                   'downloading from the archive.')),
+        make_option('--local-app-install-data-partner',
+            action='store',
+            dest='local_app_install_data_partner',
+            default='',
+            help=('Use a local app-install-data-partner deb package rather '
+                  'than downloading from the archive.')),
             )
     verbosity = 1
 
+    def set_cache(self, distroseries):
+        """Set self.cache.
+
+           Call set_cache at some point before accessing self.cache.
+           Calling more than once does no harm.
+        """
+        if not hasattr(self, 'cache'):
+            maker = CacheMaker(distroseries)
+            self.cache = maker.get_apt_cache()
+
     def handle_label(self, distroseries_name, **options):
         self.verbosity = int(options['verbosity'])
-        data_dir = tempfile.mkdtemp()
         # First create the apps based on the current cache.
         distroseries, created = DistroSeries.objects.get_or_create(
             code_name=distroseries_name)
@@ -130,32 +131,37 @@
             self.output(
                 "Created a DistroSeries record called '{0}'.\n".format(
                     distroseries_name), 1)
-        with Cache(distroseries_name) as cache:
-            self.update_from_cache(cache, distroseries_name)
-
-            # Download app install data when requested.
-            if options['local_app_install_deb'] == '':
-                deb_location = self.get_latest_app_data_for_series(
-                    distroseries_name, data_dir, cache)
-            else:
-                deb_location = options['local_app_install_deb']
-
-            # Extract and parse the deb archive.
-            deb_file = DebFile(deb_location)
-            self.output("Processing application data to update database...\n",
-                1)
-            deb_file.data.extractall(data_dir)
-            matcher = data_dir + '/usr/share/app-install/desktop/*.desktop'
-            icon_dir = data_dir + '/usr/share/app-install/icons/'
-
-            for desktop_file in iglob(matcher):
-                if self.verbosity > 1:
-                    self.output("Processing {0}\n".format(desktop_file), 1)
-                elif self.verbosity > 0:
-                    self.output(".", 1, flush=True)
-                self.process_desktop_file(desktop_file, icon_dir, distroseries)
-
-            shutil.rmtree(data_dir)
+        self.update_from_cache(distroseries_name)
+
+        for package in ('app-install-data', 'app-install-data-partner', ):
+            opt_name = 'local_{0}'.format(package.replace('-', '_'))
+            deb_location = options.get(opt_name, '')
+            self.update_from_app_data(package, distroseries, deb_location)
+
+    def update_from_app_data(self, package_name, distroseries,
+        deb_location=''):
+        data_dir = tempfile.mkdtemp()
+        # Download app install data if necessary
+        if deb_location == '':
+            deb_location = self.get_package_data_for_series(package_name,
+                distroseries.code_name, data_dir)
+
+        # Extract and parse the deb archive.
+        deb_file = DebFile(deb_location)
+        self.output("Processing {0} to update database...\n".format(
+            package_name), 1)
+        deb_file.data.extractall(data_dir)
+        matcher = data_dir + '/usr/share/app-install/desktop/*.desktop'
+        icon_dir = data_dir + '/usr/share/app-install/icons/'
+
+        for desktop_file in iglob(matcher):
+            if self.verbosity > 1:
+                self.output("Processing {0}\n".format(desktop_file), 1)
+            elif self.verbosity > 0:
+                self.output(".", 1, flush=True)
+            self.process_desktop_file(desktop_file, icon_dir, distroseries)
+
+        shutil.rmtree(data_dir)
 
     def get_screenshot_urls_from_server(self, app):
         url = urljoin(settings.SCREENSHOTS_BASE_URL,
@@ -167,7 +173,7 @@
                 for entry in data['screenshots']:
                     screenshot_url = entry['large_image_url']
 
-                    self.output("Adding {0} to {1}".format(
+                    self.output("Adding {0} to {1}\n".format(
                         screenshot_url,
                         app.package_name), 1)
                     app.applicationmedia_set.get_or_create(
@@ -178,31 +184,24 @@
         except (IOError, TypeError):
             pass
 
-    def get_latest_app_data_for_series(self, distroseries, working_dir,
-        cache=None):
-        # Write a temporary sources.list, update the cache and access:
-        # cache['app-install-data-ubuntu'].candidate.uri
-        install_data_url = self.get_app_data_uri_for_series(distroseries,
-        cache)
+    def get_package_data_for_series(self, package_name, distroseries,
+        working_dir):
+        install_data_url = self.get_package_uri_for_series(
+            package_name, distroseries)
         rest, sep, deb_filename = install_data_url.rpartition('/')
 
-        self.output(u"Downloading app-install-data from {0}...\n".format(
+        self.output(u"Downloading {0} from {1}...\n".format(package_name,
                     install_data_url), 1)
         deb_location = os.path.join(working_dir, deb_filename)
         urllib.urlretrieve(install_data_url, deb_location)
         self.output("Done.\n", 1)
         return deb_location
 
-    def get_app_data_uri_for_series(self, distroseries, cache=None):
+    def get_package_uri_for_series(self, package_name, distroseries):
         self.output("Determining URI for current version of "
-                "app-install-data in {0}.\n".format(distroseries), 1)
-        uri = None
-        if cache == None:
-            with Cache(distroseries) as cache:
-                uri = cache['app-install-data'].candidate.uri
-        else:
-            uri = cache['app-install-data'].candidate.uri
-        return uri
+                "{0} in {1}.\n".format(package_name, distroseries), 1)
+        self.set_cache(distroseries)
+        return self.cache[package_name].candidate.uri
 
     def process_desktop_file(self, desktop_file_path, icon_dir, distroseries):
         with open(desktop_file_path, 'r') as desktop_file:
@@ -262,7 +261,7 @@
                         app.name, icon_path_generic,
                         '|'.join(supported_extensions)).encode('utf-8'), 1)
 
-    def update_from_cache(self, cache, distroseries_name):
+    def update_from_cache(self, distroseries_name):
         distroseries, created = DistroSeries.objects.get_or_create(
             code_name=distroseries_name)
         if created:
@@ -272,9 +271,10 @@
         prefetched_apps = dict((app.package_name, app) for app in
             Application.objects.filter(distroseries=distroseries))
 
+        self.set_cache(distroseries_name)
         # For each package in the apt-cache, we update (or possibly
         # create) a matching db entry.
-        for package in cache:
+        for package in self.cache:
             if self.verbosity > 1:
                 self.output("Updating {0}\n".format(package.name), 1)
             elif self.verbosity > 0:

=== modified file 'src/webcatalog/models/applications.py'
--- src/webcatalog/models/applications.py	2012-03-23 20:14:03 +0000
+++ src/webcatalog/models/applications.py	2012-03-29 22:02:21 +0000
@@ -78,7 +78,7 @@
     architectures = models.CharField(max_length=255, blank=True)
     keywords = models.CharField(max_length=255, blank=True)
     app_type = models.CharField(max_length=32, blank=True)
-    section = models.CharField(max_length=32)
+    section = models.CharField(max_length=32, blank=True)
     categories = models.CharField(max_length=255, blank=True)
     departments = models.ManyToManyField('Department', blank=True)
     icon_name = models.CharField(max_length=255, blank=True)

=== modified file 'src/webcatalog/tests/test_commands.py'
--- src/webcatalog/tests/test_commands.py	2012-03-28 12:44:35 +0000
+++ src/webcatalog/tests/test_commands.py	2012-03-29 22:02:21 +0000
@@ -48,7 +48,6 @@
     ReviewStatsImport,
     )
 from webcatalog.management.commands.import_app_install_data import (
-    Cache as CacheContextManager,
     Command as ImportAppInstallCommand,
     )
 from webcatalog.management.commands.import_ratings_stats import (
@@ -131,6 +130,8 @@
     def setUp(self):
         """Create a mock apt-cache so we don't wait for the update."""
         super(ImportAppInstallTestCase, self).setUp()
+        self.deb_location = self.factory.get_test_path(
+            'app-install-data-test_all.deb')
 
         # We use a temporary on-disk cache location for our
         # tests.
@@ -139,6 +140,11 @@
             'DISK_APT_CACHE_LOCATION', self.tmp_apt_cache)
         self.patch_cache_setting.start()
 
+        self.patch_get_screenshot_urls = patch(
+            'webcatalog.management.commands.import_app_install_data.'
+            'Command.get_screenshot_urls_from_server')
+        self.patch_get_screenshot_urls.start()
+
         if self.use_mock_apt_cache:
             # The apt.Cache class is patched to return our own mock which
             # will be subscriptable with a pre-set dict with mock ock apt
@@ -150,6 +156,8 @@
 
     def tearDown(self):
         self.patch_cache_setting.stop()
+        if hasattr(self, 'patch_get_screenshot_urls'):
+            self.patch_get_screenshot_urls.stop()
         shutil.rmtree(self.tmp_apt_cache)
         if self.use_mock_apt_cache:
             self.patch_cache.stop()
@@ -158,41 +166,41 @@
     def test_app_data_downloaded_by_default(self):
         get_data_fn = ('webcatalog.management.commands.'
                        'import_app_install_data.Command.'
-                       'get_latest_app_data_for_series')
+                       'get_package_data_for_series')
         with patch(get_data_fn) as mock_get_data_fn:
-            mock_get_data_fn.return_value = self.factory.get_test_path(
-                'app-install-data-test_all.deb')
+            mock_get_data_fn.return_value = self.deb_location
             call_command('import_app_install_data', 'onion', verbosity=0)
 
-        self.assertEqual(1, mock_get_data_fn.call_count)
-        args = mock_get_data_fn.call_args[0]
-        self.assertEqual('onion', args[0])
-        self.assertTrue(args[1].startswith('/tmp/tmp'))
-        if self.use_mock_apt_cache:
-            self.assertEqual(self.mock_cache, args[2])
+        self.assertEqual(2, mock_get_data_fn.call_count)
+        for call_args, package_name in zip(mock_get_data_fn.call_args_list,
+            ('app-install-data', 'app-install-data-partner')):
+            args = call_args[0]
+            self.assertEqual(package_name, args[0])
+            self.assertEqual('onion', args[1])
+            self.assertTrue(args[2].startswith('/tmp/tmp'))
 
-    def test_get_latest_app_data_for_series(self):
+    def test_get_package_data_for_series(self):
         # Requested and downloaded from the server.
         tmp_dir = tempfile.mkdtemp()
         get_uri_fn = (
             'webcatalog.management.commands.import_app_install_data.'
-            'Command.get_app_data_uri_for_series')
+            'Command.get_package_uri_for_series')
         with patch(get_uri_fn) as mock_get_uri:
             mock_get_uri.return_value = 'http://example.com/my.deb'
             with patch('urllib.urlretrieve') as mock_urlretrieve:
-                ImportAppInstallCommand().get_latest_app_data_for_series(
-                    'natty', tmp_dir)
+                ImportAppInstallCommand().get_package_data_for_series(
+                    'app-install-data', 'natty', tmp_dir)
         shutil.rmtree(tmp_dir)
 
         mock_urlretrieve.assert_called_with(
             'http://example.com/my.deb', os.path.join(tmp_dir, 'my.deb'))
 
-    def test_local_app_install_deb(self):
+    def test_local_app_install_data(self):
         # We don't hit the network when provided a local app-install deb.
         with patch('urllib.urlretrieve') as mock_urlretrieve:
             call_command('import_app_install_data', 'natty',
-                local_app_install_deb=self.factory.get_test_path(
-                    'app-install-data-test_all.deb'),
+                local_app_install_data=self.deb_location,
+                local_app_install_data_partner=self.deb_location,
                 verbosity=0)
 
         self.assertEqual(0, mock_urlretrieve.call_count)
@@ -202,8 +210,8 @@
 
         call_command(
             'import_app_install_data', 'natty',
-            local_app_install_deb=self.factory.get_test_path(
-                'app-install-data-test_all.deb'),
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
             verbosity=0)
 
         self.assertEqual(4, Application.objects.count())
@@ -222,8 +230,8 @@
 
         call_command(
             'import_app_install_data', 'natty',
-            local_app_install_deb=self.factory.get_test_path(
-                'app-install-data-test_all.deb'),
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
             verbosity=0)
 
         app_reloaded = Application.objects.get(id=app.id)
@@ -240,8 +248,8 @@
 
         call_command(
             'import_app_install_data', 'oneric',
-            local_app_install_deb=self.factory.get_test_path(
-                'app-install-data-test_all.deb'),
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
             verbosity=0)
 
         app_reloaded = Application.objects.get(id=app.id)
@@ -252,8 +260,8 @@
     def test_distroseries_added_to_app(self):
         call_command(
             'import_app_install_data', 'natty',
-            local_app_install_deb=self.factory.get_test_path(
-                'app-install-data-test_all.deb'),
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
             verbosity=0)
 
         self.assertEqual(4, Application.objects.count())
@@ -263,8 +271,8 @@
     def test_icon_added_to_model(self):
         call_command(
             'import_app_install_data', 'natty',
-            local_app_install_deb=self.factory.get_test_path(
-                'app-install-data-test_all.deb'),
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
             verbosity=0)
 
         self.assertEqual(4, Application.objects.count())
@@ -275,6 +283,8 @@
 
     def test_screenshots_added_to_the_model(self):
         fn = 'webcatalog.management.commands.import_app_install_data.urllib'
+        self.patch_get_screenshot_urls.stop()
+        del self.patch_get_screenshot_urls
         with patch(fn) as mock_urllib:
             mock_response = Mock()
             mock_response.getcode.return_value = 200
@@ -285,18 +295,18 @@
             )
             mock_urllib.urlopen.return_value = mock_response
 
-            call_command(
-                'import_app_install_data', 'natty',
-                    local_app_install_deb=self.factory.get_test_path(
-                    'app-install-data-test_all.deb'),
+            call_command('import_app_install_data', 'natty',
+                local_app_install_data=self.deb_location,
+                local_app_install_data_partner=self.deb_location,
                 verbosity=0)
 
         scribus = Application.objects.get(package_name='scribus')
 
         self.assertEqual(scribus.screenshots, ["http://example.com/1.png"])
 
-    def test_get_app_data_uri_for_series(self):
-        uri = ImportAppInstallCommand().get_app_data_uri_for_series('natty')
+    def get_package_uri_for_series(self):
+        uri = ImportAppInstallCommand().get_package_uri_for_series(
+            'app-install-data', 'natty')
 
         self.assertEqual('http://example.com/app-install-1.01.deb', uri)
 
@@ -305,8 +315,8 @@
 
         call_command(
             'import_app_install_data', 'natty',
-            local_app_install_deb=self.factory.get_test_path(
-                'app-install-data-test_all.deb'),
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
             verbosity=0)
 
         self.assertEqual(4, Application.objects.count())
@@ -325,8 +335,8 @@
 
         call_command(
             'import_app_install_data', 'natty',
-            local_app_install_deb=self.factory.get_test_path(
-                'app-install-data-test_all.deb'),
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
             verbosity=0)
 
         app_reloaded = Application.objects.get(id=app.id)
@@ -335,12 +345,12 @@
             app_reloaded.comment)
         self.assertEqual("Scribus description", app_reloaded.description)
 
-    def test_cache_context_manager_creates_sources_list(self):
+    def test_cache_maker_creates_sources_list(self):
         # If the on-disk cache doesn't yet exist, it is created.
         call_command(
             'import_app_install_data', 'natty',
-            local_app_install_deb=self.factory.get_test_path(
-                'app-install-data-test_all.deb'),
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
             verbosity=0)
 
         natty_cache = os.path.join(self.tmp_apt_cache, 'natty')
@@ -349,16 +359,17 @@
         self.assertTrue(os.path.exists(os.path.join(sources_list_path)))
         with open(sources_list_path) as content:
             sources_list_content = content.read()
-        self.assertEqual(
-            'deb http://archive.ubuntu.com/ubuntu natty main universe\n'
-            'deb http://archive.ubuntu.com/ubuntu '
-            'natty-updates main universe\n',
+        self.assertEqual("""
+deb http://archive.ubuntu.com/ubuntu natty main universe
+deb http://archive.ubuntu.com/ubuntu natty-updates main universe
+deb http://archive.canonical.com/ubuntu natty partner
+""",
             sources_list_content)
         if self.use_mock_apt_cache:
             self.mock_cache_class.assert_called_with(rootdir=natty_cache)
             self.assertEqual(1, self.mock_cache.update.call_count)
 
-    def test_cache_context_manager_doesnt_overwrite_existing(self):
+    def test_cache_maker_doesnt_overwrite_existing(self):
         # If an on-disk cache for the distroseries already exists, a
         # new one is *not* created.
         series_cache = os.path.join(self.tmp_apt_cache, 'testwebcatseries')
@@ -368,12 +379,12 @@
 
         create_apt_cache_fn = (
             'webcatalog.management.commands.import_app_install_data.'
-            'Cache.create_apt_cache')
+            'CacheMaker.create_apt_cache')
         with patch(create_apt_cache_fn) as mock_create_apt_cache:
             call_command(
                 'import_app_install_data', 'testwebcatseries',
-                local_app_install_deb=self.factory.get_test_path(
-                    'app-install-data-test_all.deb'),
+                local_app_install_data=self.deb_location,
+                local_app_install_data_partner=self.deb_location,
                 verbosity=0)
 
         self.assertEqual(0, mock_create_apt_cache.call_count)
@@ -385,8 +396,7 @@
         self.assertEqual(0, DistroSeries.objects.filter(
             code_name='natty').count())
 
-        with CacheContextManager('natty') as cache:
-            ImportAppInstallCommand().update_from_cache(cache, 'natty')
+        ImportAppInstallCommand().update_from_cache('natty')
 
         self.assertEqual(1, DistroSeries.objects.filter(
             code_name='natty').count())
@@ -394,8 +404,7 @@
     def test_apt_cache_apps_used_also(self):
         self.assertEqual(0, Application.objects.count())
 
-        with CacheContextManager('natty') as cache:
-            ImportAppInstallCommand().update_from_cache(cache, 'natty')
+        ImportAppInstallCommand().update_from_cache('natty')
 
         # There are 4 packages in our mock_cache which is patched in the
         # setUp.
@@ -415,8 +424,8 @@
 
         call_command(
             'import_app_install_data', 'precise',
-            local_app_install_deb=self.factory.get_test_path(
-                'app-install-data-test_all.deb'),
+            local_app_install_data=self.deb_location,
+            local_app_install_data_partner=self.deb_location,
             verbosity=0)
 
         firefox = Application.objects.get(package_name='firefox')