Merge lp:~zematynnad/ubuntu-webcatalog/duplicates_987826 into lp:ubuntu-webcatalog

[Danny Tamez]

Removes duplicate apps from top rated apps widget.

Overview
=========
This branch is a fix for http://pad.lv/987826 where duplicate apps were showing up in the top rated apps carousel.

Details
========
The problem was that apps for the same package but for different distroseries were being returned in the query. Since django doesn't do distinct on individual fields (at least not without a patch) the duplicates have to be filtered out a different way. I just used a dictionary with the packagename as the key to avoid the duplicates.

To Test
========
$fab bootstrap test

[Anthony Lenton (elachuni)]

Hi Danny,

I really don't think the code is doing what we want there. Comments about the changes in src/webcatalog/views.py:
 - You iterate over all of top_rated_apps (tens of thousands of apps) to keep just the first settings.NUMBER_TOP_RATED_APPS off the filtered list (tens of apps). See if there's a way to only filter enough apps as we need.
 - For each package name you currently keep the last item on the list that matches the name (as the dict is overridden each time you find a new Application instance with the same package name), so I you're effectively keeping the *lowest* rated app instance for each package name.
 - Finally, you're storing the apps in a dict and then asking for values(), so you can't expect any order in top_rated_apps.
 - And, something really minor, Count is being imported unnecessarily.

Kind regards,

achuni.

review: Needs Fixing

[lp:~zematynnad/ubuntu-webcatalog/duplicates_987826]

126. By Danny Tamez on 2012-05-23

Remove unused import.
Ensure that as few rows as necessary are iterated on.
Ensure that order is preserved in final result.

127. By Danny Tamez on 2012-05-23

Refactor to filter as few as needed.

[Anthony Lenton (elachuni)]

Thanks Danny!

review: Approve

=== modified file 'src/webcatalog/tests/test_views.py'
--- src/webcatalog/tests/test_views.py	2012-05-08 07:03:56 +0000
+++ src/webcatalog/tests/test_views.py	2012-05-23 22:34:23 +0000
@@ -697,13 +697,17 @@
         for link in links:
             self.assertContains(response, link)
 
-    def make_high_medium_and_low_apps(self):
+    def make_high_medium_and_low_apps(self, score_high=5.0, score_mid=4.0,
+                                      score_low=1.0):
         high = self.factory.make_application(package_name='high',
-                    wilson_score=5.0, ratings_average=5, ratings_total=500)
+                    wilson_score=score_high, ratings_average=5,
+                    ratings_total=500)
         mid = self.factory.make_application(package_name='mid',
-                    wilson_score=4.0, ratings_average=4, ratings_total=400)
+                    wilson_score=score_mid, ratings_average=4,
+                    ratings_total=400)
         low = self.factory.make_application(package_name='low',
-                    wilson_score=1.0, ratings_average=1, ratings_total=100)
+                    wilson_score=score_low, ratings_average=1,
+                    ratings_total=100)
         return (high, mid, low)
 
     def test_top_rated_apps_in_response(self):
@@ -712,7 +716,7 @@
         with patch_settings(NUMBER_TOP_RATED_APPS=1):
             response = self.client.get(reverse('wc-index'))
 
-        self.assertEqual(1, response.context[0]['top_rated_apps'].count())
+        self.assertEqual(1, len(response.context[0]['top_rated_apps']))
         self.assertEqual(high, response.context[0]['top_rated_apps'][0])
 
     def test_top_rated_apps_display_correctly(self):
@@ -746,6 +750,23 @@
         self.assertContains(response, expected, 2)
         self.assertNotContains(response, app_not_included.package_name)
 
+    def test_top_rated_apps_does_not_include_duplicate_packages(self):
+        high, mid, low = self.make_high_medium_and_low_apps(5.0, 4.0, 1.0)
+        high2, mid2, low2 = self.make_high_medium_and_low_apps(4.8, 3.8, 0.8)
+        high3, mid3, low3 = self.make_high_medium_and_low_apps(4.6, 3.6, 0.6)
+        high4, mid4, low4 = self.make_high_medium_and_low_apps(4.4, 3.4, 0.4)
+
+        with patch_settings(NUMBER_TOP_RATED_APPS=4):
+            response = self.client.get(reverse('wc-index'))
+
+        apps = response.context['top_rated_apps']
+        # should have only first high, first mid and first low
+        # and in that order
+        self.assertEqual(len(apps), 3)
+        self.assertEqual(apps[0], high)
+        self.assertEqual(apps[1], mid)
+        self.assertEqual(apps[2], low)
+
     def test_includes_cache_headers(self):
         with patch_settings(CACHE_MIDDLEWARE_SECONDS=600):
             response = self.client.get(reverse('wc-index'))

=== modified file 'src/webcatalog/views.py'
--- src/webcatalog/views.py	2012-05-08 07:03:56 +0000
+++ src/webcatalog/views.py	2012-05-23 22:34:23 +0000
@@ -116,15 +116,25 @@
     featured_apps = [Application.objects.find_best(package_name=app)
                      for app in settings.FEATURED_APPS]
     featured_apps = [x for x in featured_apps if x]
-    top_rated_apps = (Application.objects.filter(wilson_score__gt=0)
-                      .order_by('-wilson_score'))
-    num = settings.NUMBER_TOP_RATED_APPS
-    top_rated_apps = top_rated_apps[:num]
+    apps = (Application.objects.filter(wilson_score__gt=0)
+            .order_by('-wilson_score'))
+    # filter out apps that are top rated more than once because
+    # of different distroseries
+
+    seen = set()
+    top_rated = []
+    for app in apps:
+        if not app.package_name in seen:
+            top_rated.append(app)
+            seen.add(app.package_name)
+        if len(top_rated) == settings.NUMBER_TOP_RATED_APPS:
+            break
+
     context = RequestContext(request, dict={
         'depts': depts,
         'exhibits': exhibits,
         'featured_apps': featured_apps,
-        'top_rated_apps': top_rated_apps,
+        'top_rated_apps': top_rated,
     })
     return render_to_response('webcatalog/index.html',
         context_instance=context)