Merge lp:~michael.nelson/ubuntu-webcatalog/777871-pull-other-apps-from-apt-cache-really into lp:ubuntu-webcatalog

[Michael Nelson]

Import apps from the apt-cache for the distroseries in addition to the app-install-data.

Overview
========
Imports and updates applications from the apt-cache before processing the desktop data in app-install-data. (bug 777871).

Details
=======
As well as providing the new update_from_cache method for use during import, this branch also ensures that when the desktop files are subsequently parsed, we don't overwrite values, like Application.description (that were set from the cache) with empty values from the desktop file.

While there, I simplified the process_desktop_file method by moving some form-related code into the form where it belongs.

Issues
======

As far as I can see, the only useful data I can grab from the apt-cache is: package name, description, section and summary. This leaves us with a few issues that we may either want to fix in this branch, or create bugs and fix soon:
1) Our templates assume that apps have a name (eg. we use Application.name when linking to package details), but for apps that are created from the cache only, I'm not sure what we should use (perhaps a template tag name_or_package_name)
2) We've got required fields, such as popcon on the Application model, which we don't get from the cache, so apps which are created from cache only won't have this (meaning records that can't be edited in the admin).

To test: follow the readme to bootstrap, then `fab test`

[Ricardo Kirkner (ricardokirkner)]

l. 113-129: this can be replaced by a call to get_or_create, avoiding the need of a try..except block

review: Needs Fixing

[Ricardo Kirkner (ricardokirkner)]

> l. 113-129: this can be replaced by a call to get_or_create, avoiding the need
> of a try..except block

I was mistaken. This cannot be done without saving twice.

review: Approve

[lp:~michael.nelson/ubuntu-webcatalog/777871-pull-other-apps-from-apt-cache-really]

28. By Michael Nelson on 2011-06-24

Ensure that apt-cache only entries split the cache entry summary into name/comment.

29. By Michael Nelson on 2011-06-24

Ensure we include updates in the cache, not just the release.

[Michael Nelson (michael.nelson)]

Hi, I've pushed revs 28 and 29, which addresses issue (1) that I noted above. It also ensures that we populate the apt-cache using not just main, but also universe for both the release and updates.

[Michael Foord (mfoord)]

Looks good.

review: Approve

=== modified file 'django_project/config/main.cfg'
--- django_project/config/main.cfg	2011-06-21 11:31:27 +0000
+++ django_project/config/main.cfg	2011-06-24 07:54:54 +0000
@@ -53,6 +53,7 @@
 template_dirs = django_project/templates/
 static_root = ./django_project/static/
 static_url = /assets/
+admin_media_prefix = /assets/admin/
 
 # Django-1.1 backwards compatibility
 database_engine = sqlite3

=== modified file 'src/webcatalog/forms.py'
--- src/webcatalog/forms.py	2011-06-17 16:21:59 +0000
+++ src/webcatalog/forms.py	2011-06-24 07:54:54 +0000
@@ -56,7 +56,7 @@
         exclude = ('distroseries','for_purchase', 'archive_id')
 
     @classmethod
-    def get_form_from_desktop_data(cls, str_data):
+    def get_form_from_desktop_data(cls, str_data, distroseries):
         parser = ConfigParser()
         parser.readfp(StringIO(str_data))
         data = dict(parser.items('Desktop Entry'))
@@ -65,4 +65,23 @@
                 data[app_key] = data[desktop_key]
                 del(data[desktop_key])
 
-        return cls(data=data)
+        try:
+            instance = Application.objects.get(
+                package_name=data['package_name'], distroseries=distroseries)
+        except Application.DoesNotExist:
+            instance = None
+
+        return cls(data=data, instance=instance)
+
+    def clean(self):
+        cleaned_data = super(ApplicationForm, self).clean()
+
+        # For all fields, ensure we don't overwrite valid data on an
+        # associated instance with blank fields.
+        if self.instance.id:
+            for key, value in cleaned_data.items():
+                if not value:
+                    instance_value = getattr(self.instance, key, False)
+                    cleaned_data[key] = instance_value or value
+
+        return cleaned_data

=== modified file 'src/webcatalog/management/commands/import_app_install_data.py'
--- src/webcatalog/management/commands/import_app_install_data.py	2011-06-17 14:37:58 +0000
+++ src/webcatalog/management/commands/import_app_install_data.py	2011-06-24 07:54:54 +0000
@@ -59,8 +59,11 @@
             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 {0} main\n'.format(
-                    self.distroseries))
+                '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))
 
     def get_apt_cache(self):
         # Create on-disk cache for distroseries if it
@@ -121,6 +124,7 @@
                 self.stdout.write(
                     "Created a DistroSeries record with codename '{0}'.".format(
                         distroseries_name))
+            self.update_from_cache(cache, distroseries_name)
 
             # Download app install data when requested.
             if options['local_app_install_deb'] == '':
@@ -177,31 +181,11 @@
                              cache):
         with open(desktop_file_path, 'r') as desktop_file:
             data = desktop_file.read()
-        form = ApplicationForm.get_form_from_desktop_data(data)
+        form = ApplicationForm.get_form_from_desktop_data(data, distroseries)
 
         if form.is_valid():
-            # Check if the app already exists in our DB.
-            try:
-                app = Application.objects.get(
-                    package_name=form.cleaned_data['package_name'],
-                    distroseries=distroseries)
-                # Force the form to update the existing application.
-                # Simply setting the forms.instance doesn't work as the
-                # form constructs its instance during is_valid, and
-                # doesn't reconstruct it to save.
-                app = construct_instance(form, app)
-            except Application.DoesNotExist:
-                app = form.save(commit=False)
-                app.distroseries = distroseries
-
-            # we don't have the description - just the desktop data, so:
-            # use the package_name to lookup the app in the cache and get
-            # the descrption from there
-            try:
-                cached_app = cache[app.package_name]
-                app.description = cached_app.candidate.description
-            except KeyError:
-                pass
+            app = form.save(commit=False)
+            app.distroseries = distroseries
             app.save()
             app.update_departments()
             self.add_icon_to_app(app, icon_dir)
@@ -248,3 +232,44 @@
                     u"No icon found for {0} at {1}.({2}).\n".format(
                         app.name, icon_path_generic,
                         '|'.join(supported_extensions)).encode('utf-8'))
+
+    def update_from_cache(self, cache, distroseries_name):
+        distroseries, created = DistroSeries.objects.get_or_create(
+            code_name=distroseries_name)
+        if created and self.verbosity > 0:
+            self.stdout.write(
+                "Created a DistroSeries record with codename '{0}'.".format(
+                    distroseries_name))
+
+        # For each package in the apt-cache, we update (or possibly
+        # create) a matching db entry.
+        for package in cache:
+            candidate = package.candidate
+            # USC uses the first line of the summary as the
+            # application name and the rest as the comment..
+            summary_lines = candidate.summary.split('\n')
+            app_name = summary_lines[0]
+            app_comment = '\n'.join(summary_lines[1:])
+            if not app_comment:
+                app_comment = package.name
+
+            try:
+                app = Application.objects.get(
+                    package_name=package.name,
+                    distroseries=distroseries)
+                app.description = candidate.description
+                app.section = candidate.section
+                app.name = app_name
+                app.comment = app_comment
+                app.save()
+
+            except Application.DoesNotExist:
+                app = Application.objects.create(
+                    package_name=package.name,
+                    distroseries=distroseries,
+                    description=candidate.description,
+                    section=candidate.section,
+                    name=app_name,
+                    comment=app_comment,
+                    )
+        return distroseries

=== modified file 'src/webcatalog/tests/test_commands.py'
--- src/webcatalog/tests/test_commands.py	2011-06-17 21:27:33 +0000
+++ src/webcatalog/tests/test_commands.py	2011-06-24 07:54:54 +0000
@@ -30,11 +30,18 @@
 from django.core.management import call_command
 from mock import (
     patch,
+    MagicMock,
     Mock,
     )
 
-from webcatalog.models import Application
-from webcatalog.management.commands.import_app_install_data import Command
+from webcatalog.models import (
+    Application,
+    DistroSeries,
+    )
+from webcatalog.management.commands.import_app_install_data import (
+    Cache as CacheContextManager,
+    Command,
+    )
 from webcatalog.tests.factory import TestCaseWithFactory
 
 __metaclass__ = type
@@ -53,11 +60,14 @@
     # firefox and scribus in the cache.
     use_mock_apt_cache = True
 
-    def make_mock_apt_package(self, description, uri=''):
+    def make_mock_apt_package(self, name, description, uri='', summary=''):
         """Helper to DRY up creating a mock apt package."""
         mock_package = Mock(spec=apt.package.Package)
         mock_package.candidate.description = description
         mock_package.candidate.uri = uri
+        mock_package.name = name
+        mock_package.candidate.section = ''
+        mock_package.candidate.summary = summary
         return mock_package
 
     def make_mock_apt_cache(self):
@@ -65,16 +75,17 @@
 
         And keeping the setUp readable.
         """
-        mock_cache = Mock()
-        mock_apt_firefox = self.make_mock_apt_package(
-            description = "Firefox description")
-        mock_apt_scribus = self.make_mock_apt_package(
-            description = "Scribus description")
-        mock_other_app = self.make_mock_apt_package(
-            description = "Otherapp description")
-        mock_app_install_data = self.make_mock_apt_package(
-            description = "App install desc.",
-            uri = 'http://example.com/app-install-1.01.deb')
+        mock_cache = MagicMock()
+        mock_apt_firefox = self.make_mock_apt_package('firefox',
+            description="Firefox description")
+        mock_apt_scribus=self.make_mock_apt_package('scribus',
+            description="Scribus description")
+        mock_other_app=self.make_mock_apt_package('otherapp',
+            description="Otherapp description",
+            summary="Otherapp the Internet\nA tagline for Otherapp")
+        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')
         cache_dict = {
             'firefox': mock_apt_firefox,
             'scribus': mock_apt_scribus,
@@ -84,8 +95,15 @@
 
         def getitem(self, key):
             return cache_dict.get(key, None)
-
         mock_cache.__getitem__ = getitem
+
+        # XXX 2011-06-23 michaeln Can't assign a bound method to a magic
+        # mock method. If the following issue is fixed, we'll be able to
+        # do `mock_cache.__iter__ = cache_dict.itervalues` without a
+        # MagicMock.
+        # https://code.google.com/p/mock/issues/detail?id=98
+        mock_cache.__iter__.return_value = cache_dict.itervalues()
+
         return mock_cache
 
     def setUp(self):
@@ -108,7 +126,6 @@
             self.mock_cache = self.make_mock_apt_cache()
             self.mock_cache_class.return_value = self.mock_cache
 
-
     def tearDown(self):
         self.patch_cache_setting.stop()
         shutil.rmtree(self.tmp_apt_cache)
@@ -166,7 +183,7 @@
                 'app-install-data-test_all.deb'),
             verbosity=0)
 
-        self.assertEqual(2, Application.objects.count())
+        self.assertEqual(4, Application.objects.count())
         scribus = Application.objects.get(package_name='scribus')
         self.assertEqual("Scribus (Stable)", scribus.name)
         self.assertEqual(
@@ -216,7 +233,7 @@
                 'app-install-data-test_all.deb'),
             verbosity=0)
 
-        self.assertEqual(2, Application.objects.count())
+        self.assertEqual(4, Application.objects.count())
         scribus = Application.objects.get(package_name='scribus')
         self.assertEqual('natty', scribus.distroseries.code_name)
 
@@ -227,7 +244,7 @@
                 'app-install-data-test_all.deb'),
             verbosity=0)
 
-        self.assertEqual(2, Application.objects.count())
+        self.assertEqual(4, Application.objects.count())
         scribus = Application.objects.get(package_name='scribus')
         firefox = Application.objects.get(package_name='firefox')
         self.assertEqual(
@@ -251,11 +268,15 @@
                 'app-install-data-test_all.deb'),
             verbosity=0)
 
-        self.assertEqual(2, Application.objects.count())
+        self.assertEqual(4, Application.objects.count())
         firefox = Application.objects.get(package_name='firefox')
         scribus = Application.objects.get(package_name='scribus')
         self.assertEqual("Firefox description", firefox.description)
         self.assertEqual("Scribus description", scribus.description)
+        # The name and comment come directly from the desktop file when
+        # it exists.
+        self.assertEqual("Firefox Web Browser", firefox.name)
+        self.assertEqual("Browse the World Wide Web", firefox.comment)
 
     def test_description_updated_in_app(self):
         app = self.factory.make_application(package_name='scribus',
@@ -282,8 +303,16 @@
             verbosity=0)
 
         natty_cache = os.path.join(self.tmp_apt_cache, 'natty')
-        self.assertTrue(os.path.exists(os.path.join(
-            natty_cache, 'etc', 'apt', 'sources.list')))
+        sources_list_path = os.path.join(natty_cache, 'etc', 'apt',
+            'sources.list')
+        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',
+            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)
@@ -311,6 +340,32 @@
             self.mock_cache_class.assert_called_with(rootdir=series_cache)
             self.assertEqual(1, self.mock_cache.update.call_count)
 
+    def test_update_from_cache_creates_distroseries_when_required(self):
+        self.assertEqual(0, DistroSeries.objects.filter(
+            code_name='natty').count())
+
+        with CacheContextManager('natty') as cache:
+            Command().update_from_cache(cache, 'natty')
+
+        self.assertEqual(1, DistroSeries.objects.filter(
+            code_name='natty').count())
+
+    def test_apt_cache_apps_used_also(self):
+        self.assertEqual(0, Application.objects.count())
+
+        with CacheContextManager('natty') as cache:
+            Command().update_from_cache(cache, 'natty')
+
+        # There are 4 packages in our mock_cache which is patched in the
+        # setUp.
+        self.assertEqual(4, Application.objects.count())
+        # As cache entries don't include the app name or comment, the
+        # summary is split to create those fields.
+        otherapp = Application.objects.get(package_name='otherapp')
+        self.assertEqual('Otherapp the Internet', otherapp.name)
+        self.assertEqual('A tagline for Otherapp', otherapp.comment)
+
+
 class ImportForPurchaseAppsTestCase(TestCaseWithFactory):
 
     def setUp(self):

=== modified file 'src/webcatalog/tests/test_forms.py'
--- src/webcatalog/tests/test_forms.py	2011-04-12 15:22:54 +0000
+++ src/webcatalog/tests/test_forms.py	2011-06-24 07:54:54 +0000
@@ -81,12 +81,30 @@
             }
         desktop_entry = self.get_desktop_entry(self.get_desktop_data(data))
 
-        form = ApplicationForm.get_form_from_desktop_data(desktop_entry)
+        form = ApplicationForm.get_form_from_desktop_data(desktop_entry,
+            distroseries=self.factory.make_distroseries())
 
         self.assertTrue(form.is_valid())
         self.assertEqual('My Package', form.cleaned_data['name'])
         self.assertEqual('mypkg', form.cleaned_data['package_name'])
 
+    def test_get_form_from_desktop_data_with_existing(self):
+        # If the desktop data contains some blanks, then the initial
+        # data from any existing instance should be used instead.
+        app = self.factory.make_application(package_name='mypkg',
+            description='MyPkg description')
+        data = {
+            'Name': 'My Package',
+            'X-AppInstall-Package': 'mypkg',
+            }
+        desktop_entry = self.get_desktop_entry(self.get_desktop_data(data))
+
+        form = ApplicationForm.get_form_from_desktop_data(desktop_entry,
+            distroseries=app.distroseries)
+
+        self.assertTrue(form.is_valid())
+        self.assertEqual('MyPkg description', form.cleaned_data['description'])
+
     def test_extra_fields(self):
         # There are a bunch of extra fields which are also supported by
         # the model form.
@@ -96,7 +114,7 @@
             'X-AppInstall-Screenshot-Url': 'http://example.com/screenshot',
             'Type': 'Application',
             'X-AppInstall-Description': 'A description',
-            'X-AppInstall-Architectures': 'i386r;amd64',
+            'X-AppInstall-Architectures': 'i386;amd64',
             'X-AppInstall-Popcon': 9876,
             'X-AppInstall-Keywords': 'jazz,rock,country',
             'Categories': 'Graphics;Jazz;Publishing',
@@ -105,7 +123,8 @@
         desktop_entry = self.get_desktop_entry(self.get_desktop_data(
             extra_desktop_info))
 
-        form = ApplicationForm.get_form_from_desktop_data(desktop_entry)
+        form = ApplicationForm.get_form_from_desktop_data(desktop_entry,
+            distroseries=self.factory.make_distroseries())
 
         self.assertTrue(form.is_valid())
         for key, value in extra_desktop_info.items():