Merge lp:~elachuni/ubuntu-webcatalog/distro-searches into lp:ubuntu-webcatalog

[Anthony Lenton]

Make searches distro-series specific

Overview
========
Make searches distro-series specific

Details
=======
Until now, search results would include apps for all distroseries, repeated and intermixed. So if an app is available for four different distroseries, you would get it repeated four times in the search results.
While coding the solution different options were considered:
 - Showing an app in search results only once, if it's available in any distroseries. This would require a largeish model change to separate an App from an AppInADistroSeries, with many AppInADistroSeries for each App. And you still never know what distroseries an app you see in your search results is available for.
 - Leave search results as is, but add a small label next to each app stating which distroseries it's for. This would still result in multiple instances of almost identical apps in your search results.
 - Group search results by distroseries. This is what packages.ubuntu.com does, but it doesn't play well with paging results.
 - The current solution, that's to have the search limit to a default distroseries, and allow the user to specify a different distroseries easily (using a right navigation bar in this case).

While I was there, I made distroseries show up in a reverse code_name order in our right sidebars, so that Oneiric is always at the top, and older distroseries are shown further down.

[Matthew Nuzum (newz)]

Looks good.

review: Approve

=== modified file 'src/webcatalog/models/applications.py'
--- src/webcatalog/models/applications.py	2011-07-07 14:15:04 +0000
+++ src/webcatalog/models/applications.py	2011-07-07 21:04:30 +0000
@@ -104,7 +104,7 @@
     def available_distroseries(self):
         """Return the set of distroseries for which this app is available"""
         return DistroSeries.objects.filter(
-            application__package_name=self.package_name).order_by('version')
+            application__package_name=self.package_name).order_by('-code_name')
 
     def update_departments(self):
         """Update the list of departments for this app"""

=== modified file 'src/webcatalog/templates/webcatalog/search_results.html'
--- src/webcatalog/templates/webcatalog/search_results.html	2011-06-28 15:03:12 +0000
+++ src/webcatalog/templates/webcatalog/search_results.html	2011-07-07 21:04:30 +0000
@@ -7,6 +7,23 @@
 {% block content %}
     {% include "webcatalog/breadcrumbs_snippet.html" %}
 
+    <div id="rightbar">
+        <div class="portlet">
+            <div class="portletheader">Search within</div>
+            <ul class="portletactions">
+                {% for ds in available_distroseries %}
+                <li class="item{% ifequal ds.code_name distroseries %} active{% endifequal %}{% if forloop.last %} last{% endif %}">
+                    {% ifequal ds.code_name distroseries %}
+                        <span>Ubuntu {{ ds.version }} ({{ ds.code_name }})</span>
+                    {% else %}
+                        <a href="{% url wc-search ds.code_name %}?q={{query}}">Ubuntu {{ ds.version }} ({{ ds.code_name }})</a>
+                    {% endifequal %}</li>
+                {% endfor %}
+            </ul>
+        </div>
+    </div>
+
+<div class="main-column">
 {% if query %}
 <h3>{% blocktrans %}Searched for "{{query}}"{% endblocktrans %}</h3>
 {% if page.object_list %}
@@ -21,4 +38,6 @@
 {% else %}
 <h3>{% trans "You didn't search for anything!" %}</h3>
 {% endif %}
+</div>
+
 {% endblock %}

=== modified file 'src/webcatalog/tests/test_views.py'
--- src/webcatalog/tests/test_views.py	2011-07-04 14:47:05 +0000
+++ src/webcatalog/tests/test_views.py	2011-07-07 21:04:30 +0000
@@ -212,8 +212,9 @@
 class SearchTestCase(TestCaseWithFactory):
     def test_no_search_retrieves_no_apps(self):
         # Ensure there's some data in the DB
-        self.factory.make_application()
-        url = reverse('wc-search')
+        distro = self.factory.make_distroseries(code_name='lucid')
+        self.factory.make_application(distroseries=distro)
+        url = reverse('wc-search', args=[distro.code_name])
         for data in [{'q': ''}, {}]:
 
             response = self.client.get(url, data=data)
@@ -222,10 +223,14 @@
             self.assertContains(response, "You didn't search for anything")
 
     def test_search_searches_package_name(self):
-        app1 = self.factory.make_application(package_name='foo')
-        app2 = self.factory.make_application(package_name='bar')
-        app3 = self.factory.make_application(package_name='foobar')
-        url = reverse('wc-search')
+        distro = self.factory.make_distroseries(code_name='lucid')
+        app1 = self.factory.make_application(package_name='foo',
+            distroseries=distro)
+        app2 = self.factory.make_application(package_name='bar',
+            distroseries=distro)
+        app3 = self.factory.make_application(package_name='foobar',
+            distroseries=distro)
+        url = reverse('wc-search', args=[distro.code_name])
         expected = {'foo': set([app1, app3]), 'bar': set([app2, app3])}
         for q in expected:
 
@@ -235,10 +240,14 @@
                 expected[q], set(response.context['page'].object_list))
 
     def test_search_searches_name(self):
-        app1 = self.factory.make_application(name='foo')
-        app2 = self.factory.make_application(name='bar')
-        app3 = self.factory.make_application(name='foobar')
-        url = reverse('wc-search')
+        distro = self.factory.make_distroseries(code_name='lucid')
+        app1 = self.factory.make_application(name='foo',
+            distroseries=distro)
+        app2 = self.factory.make_application(name='bar',
+            distroseries=distro)
+        app3 = self.factory.make_application(name='foobar',
+            distroseries=distro)
+        url = reverse('wc-search', args=[distro.code_name])
         expected = {'foo': set([app1, app3]), 'bar': set([app2, app3])}
         for q in expected:
 
@@ -248,43 +257,53 @@
                 expected[q], set(response.context['page'].object_list))
 
     def test_search_no_apps_found_says_so(self):
-        url = reverse('wc-search')
+        distro = self.factory.make_distroseries(code_name='lucid')
+        url = reverse('wc-search', args=[distro.code_name])
 
         response = self.client.get(url, data={'q': 'sarasa'})
 
         self.assertContains(response, "No applications found")
 
     def test_response_shows_query_if_no_apps_found(self):
+        distro = self.factory.make_distroseries(code_name='lucid')
         query = 'someimprobablequery'
-        url = reverse('wc-search')
+        url = reverse('wc-search', args=[distro.code_name])
 
         response = self.client.get(url, data={'q': query})
 
         self.assertContains(response, query)
 
     def test_response_shows_query_if_some_apps_found(self):
+        distro = self.factory.make_distroseries(code_name='lucid')
         query = 'someimprobablequery'
-        self.factory.make_application(package_name=query)
-        url = reverse('wc-search')
+        self.factory.make_application(package_name=query, distroseries=distro)
+        url = reverse('wc-search', args=[distro.code_name])
 
         response = self.client.get(url, data={'q': query})
 
         self.assertContains(response, query)
 
     def test_response_shows_number_of_apps_found(self):
-        app1 = self.factory.make_application(package_name='foo')
-        app2 = self.factory.make_application(package_name='foobar')
-        url = reverse('wc-search')
+        distro = self.factory.make_distroseries(code_name='lucid')
+        app1 = self.factory.make_application(package_name='foo',
+            distroseries=distro)
+        app2 = self.factory.make_application(package_name='foobar',
+            distroseries=distro)
+        url = reverse('wc-search', args=[distro.code_name])
 
         response = self.client.get(url, data={'q': 'foo'})
 
         self.assertContains(response, "1 to 2 of 2")
 
     def test_response_shows_paginated_results(self):
-        app1 = self.factory.make_application(name='foo')
-        app2 = self.factory.make_application(name='fobar')
-        app3 = self.factory.make_application(name='foobar')
-        url = reverse('wc-search')
+        distro = self.factory.make_distroseries(code_name='lucid')
+        app1 = self.factory.make_application(name='foo',
+            distroseries=distro)
+        app2 = self.factory.make_application(name='fobar',
+            distroseries=distro)
+        app3 = self.factory.make_application(name='foobar',
+            distroseries=distro)
+        url = reverse('wc-search', args=[distro.code_name])
 
         with patch_settings(PAGE_BATCH_SIZE=2):
             response = self.client.get(url, data={'q': 'fo'})
@@ -299,8 +318,9 @@
         self.assertContains(response, '<a href="?q=fo&page=2"', 2)
 
     def test_invalid_page_doesnt_error(self):
-        self.factory.make_application(name='foo')
-        url = reverse('wc-search')
+        distro = self.factory.make_distroseries(code_name='lucid')
+        self.factory.make_application(name='foo', distroseries=distro)
+        url = reverse('wc-search', args=[distro.code_name])
 
         response = self.client.get(url, data={'q': 'fo', 'page': 'aef8'})
         page = response.context['page']
@@ -314,6 +334,11 @@
         page = response.context['page']
         self.assertEqual(1, page.number)
 
+    def test_invalid_distroseries_returns_404(self):
+        response = self.client.get(reverse('wc-search', args=['zirconic']))
+
+        self.assertEqual(404, response.status_code)
+
 
 class OverviewTestCase(TestCaseWithFactory):
     def test_index_contains_links_to_departments(self):
@@ -413,4 +438,22 @@
         response = self.client.get(reverse('wc-department', args=[
             'amnesiac', dept.id]))
 
-        self.assertEqual(404, response.status_code)
\ No newline at end of file
+        self.assertEqual(404, response.status_code)
+
+    def test_distroseries_are_ordered_by_reverse_code_name(self):
+        dept = self.factory.make_department('bar')
+        default = self.factory.make_distroseries(
+            code_name=settings.DEFAULT_DISTRO, version='11.04')
+        lucid = self.factory.make_distroseries(code_name='lucid',
+            version='10.04')
+        maverick = self.factory.make_distroseries(code_name='maverick',
+            version='10.10')
+
+        response = self.client.get(reverse('wc-department', args=[
+            settings.DEFAULT_DISTRO, dept.id]))
+
+        url = reverse('wc-department', args=['lucid', dept.id])
+        lucid_pos = response.content.find('<a href="{0}">Ubuntu'.format(url))
+        url = reverse('wc-department', args=['maverick', dept.id])
+        maver_pos = response.content.find('<a href="{0}">Ubuntu'.format(url))
+        self.assertTrue(lucid_pos > maver_pos)

=== modified file 'src/webcatalog/urls.py'
--- src/webcatalog/urls.py	2011-07-01 16:18:58 +0000
+++ src/webcatalog/urls.py	2011-07-07 21:04:30 +0000
@@ -40,6 +40,7 @@
     url(r'^applications/(?P<package_name>[-.+\w]+)/$', 'application_detail',
         name="wc-package-detail"),
     url(r'^search/$', 'search', name="wc-search"),
+    url(r'^search/(?P<distro>[-.+\w]+)/$', 'search', name="wc-search"),
 
     (r'^api/', include('webcatalog.api.urls')),
 

=== modified file 'src/webcatalog/views.py'
--- src/webcatalog/views.py	2011-07-04 15:46:48 +0000
+++ src/webcatalog/views.py	2011-07-07 21:04:30 +0000
@@ -59,17 +59,30 @@
     return 1
 
 
-def search(request):
+def search(request, distro=None):
+    query = request.GET.get('q', '')
+    if distro is None:
+        useragent = UserAgentString(request.META.get('HTTP_USER_AGENT', ''))
+        # Check for the distroseries in the useragent, if we have it,
+        # redirect there.
+        if useragent.distroseries:
+            distro = useragent.distroseries
+        else:
+            distro = settings.DEFAULT_DISTRO
+        return HttpResponseRedirect(reverse('wc-search', args=[distro]) +
+            '?' + urlencode({'q': query}))
+
+    # Attempt to retrieve the DistroSeries to ensure that it actually exists
+    get_object_or_404(DistroSeries, code_name=distro)
+
     apps = []
-    query = ''
     if 'q' in request.GET:
-        query = request.GET.get('q', '')
         terms = query.split()
         if terms:
             ors = [Q(package_name__icontains=term) for term in terms]
             ors += [Q(name__icontains=term) for term in terms]
-            apps = Application.objects.filter(reduce(operator.or_, ors))
-            apps = apps.order_by('name')
+            apps = Application.objects.filter(distroseries__code_name=distro)
+            apps = apps.filter(reduce(operator.or_, ors)).order_by('name')
 
     crumbs = [{'name': 'Get Software', 'url': reverse('wc-index')},
               {'name': 'Search results', 'url': reverse('wc-search') + '?' +
@@ -83,6 +96,8 @@
         'query': query,
         'page': paginator.page(page_num),
         'breadcrumbs': crumbs,
+        'available_distroseries': DistroSeries.objects.order_by('-code_name'),
+        'distroseries': distro,
         })
     return render_to_response('webcatalog/search_results.html',
         context_instance=context)
@@ -125,7 +140,7 @@
         'subdepts': subdepts,
         'page': paginator.page(page_num),
         'breadcrumbs': dept.crumbs(),
-        'available_distroseries': DistroSeries.objects.all(),
+        'available_distroseries': DistroSeries.objects.order_by('-code_name'),
         'distroseries': distro,
         })
     return render_to_response('webcatalog/department_overview.html',