Merge lp:~elachuni/ubuntu-webcatalog/display-custom-screenshot into lp:ubuntu-webcatalog

[Anthony Lenton]

Display the custom screenshot_url in the application details page, if included.

Overview
========
A branch to display the custom screenshot_url in the application details page, if included.

Details
=======
The screenshot_url field was modified slightly so that it doesn't verify if the page actually exists. This is a bad idea for url fields in general (it expects us to allow arbitrary outbound connections from our DC), but in this case specifically it made even less sense as most of the time the field is populated by a script that doesn't commit typos.

While I was there, I found an issue with the import_ratings_stats command (at least when working against sqlite) when there are many ratings returned, so I fixed and added a test for that too.

Finally, I added a bit of css to the thumbnail screenshot to make it match the current USC widget: 225px wide, and with a slight white and gray padded border.

[Michael Nelson (michael.nelson)]

Ouch... what is the actual error returned for that query? Is it because the sql ends up way too large? Anyway, great fix!

Nice test for the verify_exists too. I've always wondered about that kind of thing... how far do we go when testing the configuration of our models? We need to check the configuration of our models (required fields, for example), or even valid data when we've customized what's valid and what's not (with custom clean, or a RegExField). Other times we add a test to ensure the configuration isn't reverted.

Great stuff :)

review: Approve

[Anthony Lenton (elachuni)]

Hi Michael,

> Ouch... what is the actual error returned for that query? Is it because the
> sql ends up way too large? Anyway, great fix!

It's not just the SQL ends up being too large, but it seems there's a limit to the amount of variables you can use in a query. Removing the fix makes the test fail like this: https://pastebin.canonical.com/51010/
Googling for the error ("too many SQL variables") this limit is 999: http://www.trash-factor.com/content/too-many-sql-variables-sqlite-error
And apparently each constant in the "AND package_name IN (.....)" clause counts towards that.

> Nice test for the verify_exists too. I've always wondered about that kind of
> thing... how far do we go when testing the configuration of our models? We
> need to check the configuration of our models (required fields, for example),
> or even valid data when we've customized what's valid and what's not (with
> custom clean, or a RegExField). Other times we add a test to ensure the
> configuration isn't reverted.

I'd usually avoid checking model field arguments like this (like checking max_length works on CharFields, which I agree is pointless), but I've seen verify_exists=True come back a couple of times on one or another field, so the test is to ensure that we don't happen to put it back to True at some point on this field at least, and to document the reasons which here aren't trivial. Hopefully if somebody happens to change this down the road, the test will fail, the developer will read the comment and put it back, or at least be aware of the issues at play :)

Thanks for the review!

=== modified file 'src/webcatalog/management/commands/import_ratings_stats.py'
--- src/webcatalog/management/commands/import_ratings_stats.py	2011-07-20 15:31:13 +0000
+++ src/webcatalog/management/commands/import_ratings_stats.py	2011-08-09 20:20:16 +0000
@@ -59,10 +59,12 @@
 
     def update_apps_with_stats(self, distroseries, stats):
         stats_dict = dict((stat.package_name, stat) for stat in stats)
-        apps = Application.objects.filter(distroseries=distroseries,
-            package_name__in=stats_dict.keys()).order_by('package_name')
+        apps = Application.objects.filter(distroseries=distroseries)
+        apps = apps.order_by('package_name')
 
         for app in apps:
+            if not app.package_name in stats_dict:
+                continue
             stat = stats_dict[app.package_name]
             app.ratings_average = stat.ratings_average
             app.ratings_total = stat.ratings_total

=== modified file 'src/webcatalog/models/applications.py'
--- src/webcatalog/models/applications.py	2011-07-27 16:52:06 +0000
+++ src/webcatalog/models/applications.py	2011-08-09 20:20:16 +0000
@@ -69,7 +69,7 @@
     comment = models.CharField(max_length=255, blank=True)
     popcon = models.IntegerField(null=True, blank=True)
     channel = models.CharField(max_length=255, blank=True)
-    screenshot_url = models.URLField(blank=True,
+    screenshot_url = models.URLField(blank=True, verify_exists=False,
       help_text="Only use this if it is other than the normal screenshot url.")
     mimetype = models.CharField(max_length=2048, blank=True)
     architectures = models.CharField(max_length=255, blank=True)

=== modified file 'src/webcatalog/static/css/webcatalog.css'
--- src/webcatalog/static/css/webcatalog.css	2011-07-27 09:13:49 +0000
+++ src/webcatalog/static/css/webcatalog.css	2011-08-09 20:20:16 +0000
@@ -56,6 +56,12 @@
     margin: 0 16px;
 }
 
+.screenshot img {
+    width: 225px;
+    border: 1px solid gray;
+    padding: 3px;
+}
+
 .noscreenshot {
     padding: 41px 32px;
     border: 1px solid white;

=== modified file 'src/webcatalog/templates/webcatalog/application_detail.html'
--- src/webcatalog/templates/webcatalog/application_detail.html	2011-07-27 09:13:49 +0000
+++ src/webcatalog/templates/webcatalog/application_detail.html	2011-08-09 20:20:16 +0000
@@ -40,11 +40,11 @@
     </div>
     <div class="description">
       <div class="screenshot">
-        {% comment %}
-        Add a |thumbnail filter which will use either the custom screenshot_url
-        (if it's provided) or default to the thumbnail url below.
-        {% endcomment %}
+        {% if application.screenshot_url %}
+        <img src="{{ application.screenshot_url }}" />
+        {% else %}
         <img src="http://screenshots.ubuntu.com/thumbnail-with-version/{{ application.package_name }}/ignored" />
+        {% endif %}
       </div>
       <p>{{ application.description|htmlize_package_description }}</p>
     </div>

=== modified file 'src/webcatalog/tests/factory.py'
--- src/webcatalog/tests/factory.py	2011-07-29 15:57:17 +0000
+++ src/webcatalog/tests/factory.py	2011-08-09 20:20:16 +0000
@@ -92,7 +92,7 @@
     def make_application(self, package_name=None, name=None,
         comment=None, description=None, icon_name='', icon=None,
         distroseries=None, arch='i686', ratings_average=None,
-        ratings_total=None, ratings_histogram=''):
+        ratings_total=None, ratings_histogram='', screenshot_url=None):
         if name is None:
             name = self.get_unique_string(prefix='Readable Name')
         if package_name is None:
@@ -109,7 +109,7 @@
             description=description, popcon=999, icon=icon,
             icon_name=icon_name, distroseries=distroseries, architectures=arch,
             ratings_average=ratings_average, ratings_total=ratings_total,
-            ratings_histogram=ratings_histogram)
+            ratings_histogram=ratings_histogram, screenshot_url=screenshot_url)
 
     def make_department(self, name, parent=None):
         return Department.objects.create(name=name, parent=parent)

=== modified file 'src/webcatalog/tests/test_commands.py'
--- src/webcatalog/tests/test_commands.py	2011-07-20 11:07:59 +0000
+++ src/webcatalog/tests/test_commands.py	2011-08-09 20:20:16 +0000
@@ -528,3 +528,17 @@
         self.assertEqual('[0, 1, 0, 1, 2]', scribus.ratings_histogram)
         otherapp = Application.objects.get(id=otherapp.id)
         self.assertEqual(None, otherapp.ratings_total)
+
+    def test_update_works_with_many_stats(self):
+        """Ensure update_apps_with_stats doesn't fail with many ratings."""
+        natty = self.factory.make_distroseries(code_name='natty')
+        stats = [ReviewsStats.from_dict(dict(
+            package_name=self.factory.get_unique_string(prefix='package-'),
+            ratings_average='5.00', ratings_total=1,
+            histogram='[0, 0, 0, 0, 1]')) for x in range(3000)]
+        command = ImportRatingsStatsCommand()
+
+        # update_apps_with_stats returns None on success:
+        self.assertIsNone(command.update_apps_with_stats(natty, stats))
+
+        
\ No newline at end of file

=== modified file 'src/webcatalog/tests/test_forms.py'
--- src/webcatalog/tests/test_forms.py	2011-06-23 12:38:12 +0000
+++ src/webcatalog/tests/test_forms.py	2011-08-09 20:20:16 +0000
@@ -131,3 +131,13 @@
             form_key = desktop_field_mappings.get(key.lower(), key.lower())
             self.assertEqual(
                 extra_desktop_info[key], form.cleaned_data[form_key])
+
+    def test_screenshot_url_isnt_verified(self):
+        # Verifying if this URLField actually exists is a bad idea:
+        # - It expects us to allow arbitrary outgoing connections from the DC
+        # - It adds a significant delay to tests unless carefully mocked
+        # - It doesn't provide a real benefit as 99% of the time the field
+        #   is populated by a script that doesn't have typo issues.
+        form = ApplicationForm(dict(screenshot_url='http://foo.com:42/broken',
+            section='required', name='required', package_name='required'))
+        self.assertTrue(form.is_valid())

=== modified file 'src/webcatalog/tests/test_views.py'
--- src/webcatalog/tests/test_views.py	2011-07-27 09:13:49 +0000
+++ src/webcatalog/tests/test_views.py	2011-08-09 20:20:16 +0000
@@ -52,16 +52,17 @@
 class ApplicationDetailTestCase(TestCaseWithFactory):
 
     def get_app_and_response(self, code_name='natty', version='11.04',
-                                        arch='x86_64', name=None, comment=None,
-                                        description=None, detail_distro=None,
-                                        detail_package=None, for_purchase=False,
-                                        useragent=UBUNTU_USERAGENT):
+                             arch='x86_64', name=None, comment=None,
+                             description=None, detail_distro=None,
+                             detail_package=None, for_purchase=False,
+                             screenshot_url=None, useragent=UBUNTU_USERAGENT):
         series, created = DistroSeries.objects.get_or_create(
             code_name=code_name, version=version)
         app = self.factory.make_application(package_name='pkgfoo',
                                             distroseries=series, arch=arch,
                                             name=name, comment=comment,
-                                            description=description)
+                                            description=description,
+                                            screenshot_url=screenshot_url)
         if for_purchase:
             app.for_purchase = True
             app.save()
@@ -102,6 +103,12 @@
             response, '<img src="http://screenshots.ubuntu.com/'
                       'thumbnail-with-version/pkgfoo/ignored"')
 
+    def test_includes_custom_screenshot_url_if_provided(self):
+        expected = 'http://foo.com/%s.png' + self.factory.get_unique_string()
+        response, app = self.get_app_and_response(screenshot_url=expected)
+
+        self.assertContains(response, expected)
+
     def test_link_to_apt_package(self):
         response, app = self.get_app_and_response()