fix Top Free and Top Paid sorting on devhub dashboard (bug 705295)

apps/addons/models.py

@@ -97,6 +97,22 @@ class AddonManager(amo.models.ManagerBase):
             status = [amo.STATUS_PUBLIC]
         return self.filter(self.valid_q(status), appsupport__app=app.id)
 
+    def top_free(self, app, listed=True):
+        qs = (self.listed(app) if listed else
+              self.filter(appsupport__app=app.id))
+        return (qs.exclude(premium_type=amo.ADDON_PREMIUM)
+                .exclude(addonpremium__price__price__isnull=False)
+                .order_by('-weekly_downloads')
+                .with_index(addons='downloads_type_idx'))
+
+    def top_paid(self, app, listed=True):
+        qs = (self.listed(app) if listed else
+              self.filter(appsupport__app=app.id))
+        return (qs.filter(premium_type=amo.ADDON_PREMIUM,
+                          addonpremium__price__price__isnull=False)
+                .order_by('-weekly_downloads')
+                .with_index(addons='downloads_type_idx'))
+
     def valid_q(self, status=[], prefix=''):
         """
         Return a Q object that selects a valid Addon with the given statuses.

apps/addons/tests/test_models.py

@@ -132,6 +132,42 @@ class TestAddonManager(amo.tests.TestCase):
         addon.update(status=amo.STATUS_DISABLED)
         eq_(Addon.objects.valid_and_disabled().count(), before)
 
+    def test_top_free_public(self):
+        addons = list(Addon.objects.listed(amo.FIREFOX))
+        eq_(list(Addon.objects.top_free(amo.FIREFOX)),
+            sorted(addons, key=lambda x: x.weekly_downloads, reverse=True))
+        eq_(list(Addon.objects.top_free(amo.THUNDERBIRD)), [])
+
+    def test_top_free_all(self):
+        addons = list(Addon.objects.filter(appsupport__app=amo.FIREFOX.id)
+                     .exclude(premium_type=amo.ADDON_PREMIUM)
+                     .exclude(addonpremium__price__price__isnull=False))
+        eq_(list(Addon.objects.top_free(amo.FIREFOX, listed=False)),
+            sorted(addons, key=lambda x: x.weekly_downloads, reverse=True))
+        eq_(list(Addon.objects.top_free(amo.THUNDERBIRD, listed=False)), [])
+
+    def make_paid(self, addons):
+        price = Price.objects.create(price='1.00')
+        for addon in addons:
+            addon.update(premium_type=amo.ADDON_PREMIUM)
+            AddonPremium.objects.create(addon=addon, price=price)
+
+    def test_top_paid_public(self):
+        addons = list(Addon.objects.listed(amo.FIREFOX)[:3])
+        self.make_paid(addons)
+        eq_(list(Addon.objects.top_paid(amo.FIREFOX)),
+            sorted(addons, key=lambda x: x.weekly_downloads, reverse=True))
+        eq_(list(Addon.objects.top_paid(amo.THUNDERBIRD)), [])
+
+    def test_top_paid_all(self):
+        addons = list(Addon.objects.listed(amo.FIREFOX)[:3])
+        for addon in addons:
+            addon.update(status=amo.STATUS_LITE)
+        self.make_paid(addons)
+        eq_(list(Addon.objects.top_paid(amo.FIREFOX, listed=False)),
+            sorted(addons, key=lambda x: x.weekly_downloads, reverse=True))
+        eq_(list(Addon.objects.top_paid(amo.THUNDERBIRD, listed=False)), [])
+
 
 class TestAddonManagerFeatured(amo.tests.TestCase):
     # TODO(cvan): Merge with above once new featured add-ons are enabled.

apps/addons/views.py

@@ -271,14 +271,19 @@ class BaseFilter(object):
         return manual_order(self.model.objects, ids, 'addons.id')
 
     def filter_price(self):
-        return (self.model.objects.filter(premium_type=amo.ADDON_PREMIUM)
-                .order_by('addonpremium__price__price'))
+        return self.model.objects.order_by('addonpremium__price__price')
 
     def filter_free(self):
-        return Webapp.objects.top_free()
+        if self.model == Addon:
+            return self.model.objects.top_free(self.request.APP, listed=False)
+        else:
+            return self.model.objects.top_free(listed=False)
 
     def filter_paid(self):
-        return Webapp.objects.top_paid()
+        if self.model == Addon:
+            return self.model.objects.top_paid(self.request.APP, listed=False)
+        else:
+            return self.model.objects.top_paid(listed=False)
 
     def filter_popular(self):
         return (self.model.objects.order_by('-weekly_downloads')

apps/amo/fixtures/base/featured.json

@@ -43,7 +43,7 @@
             "trusted": 0,
             "binary": 0,
             "annoying": 0,
-            "weekly_downloads": 0,
+            "weekly_downloads": 8381,
             "paypal_id": "",
             "wants_contributions": 0,
             "average_daily_users": 3661,

apps/browse/tests.py

@@ -35,7 +35,7 @@ from versions.models import Version
 
 @nottest
 def test_listing_sort(self, sort, key=None, reverse=True, sel_class='opt'):
-    r = self.client.get(urlparams(self.url, sort=sort))
+    r = self.client.get(self.url, dict(sort=sort))
     eq_(r.status_code, 200)
     sel = pq(r.content)('#sorter ul > li.selected')
     eq_(sel.find('a').attr('class'), sel_class)
@@ -47,15 +47,15 @@ def test_listing_sort(self, sort, key=None, reverse=True, sel_class='opt'):
 
 
 @nottest
-def test_default_sort(self, sort, key=None):
+def test_default_sort(self, sort, key=None, reverse=True, sel_class='opt'):
     r = self.client.get(self.url)
     eq_(r.status_code, 200)
     eq_(r.context['sorting'], sort)
 
-    r = self.client.get(urlparams(self.url, sort='xxx'))
+    r = self.client.get(self.url, dict(sort='xxx'))
     eq_(r.status_code, 200)
     eq_(r.context['sorting'], sort)
-    test_listing_sort(self, sort, key)
+    test_listing_sort(self, sort, key, reverse, sel_class)
 
 
 class ExtensionTestCase(amo.tests.ESTestCase):

apps/devhub/templates/devhub/addons/dashboard.html

@@ -26,7 +26,6 @@
     </header>
   </section>
 
-
 {% if not addons %}
   <div class="island action-needed">
     <h2>{{ _('Welcome to the Developer Dashboard') }}</h2>
@@ -99,16 +98,12 @@
   </section>
   <section id="dashboard" class="primary" role="main">
     <div class="listing island hero c">
-      {% set url_base = url('devhub.apps') if webapp else url('devhub.addons') %}
-      {{ impala_addon_listing_header(url_base, sort_opts, sorting) }}
+      {{ impala_addon_listing_header(request.get_full_path(), search_filter=filter) }}
       <div class="items">
         {{ dev_addon_listing_items(addons.object_list) }}
-      </div>{# /items #}
-      {% if addons.paginator.num_pages > 1 %}
-        {{ addons|impala_paginator }}
-      {% endif %}
+      </div>
+      {{ addons|impala_paginator }}
     </div>
   </section>
-
 {% endif %}
 {% endblock %}

apps/devhub/tests/test_views.py

@@ -34,6 +34,7 @@ from addons.models import (Addon, AddonCategory, AddonUpsell, AddonUser,
                            Category, Charity)
 from addons.utils import ReverseNameLookup
 from applications.models import Application, AppVersion
+from browse.tests import test_listing_sort, test_default_sort
 from devhub.forms import ContribForm
 from devhub.models import ActivityLog, BlogPost, SubmitStep
 from devhub import tasks
@@ -46,6 +47,7 @@ from stats.models import Contribution
 from translations.models import Translation
 from users.models import UserProfile
 from versions.models import ApplicationsVersions, License, Version
+from webapps.models import Webapp
 
 
 class MetaTests(amo.tests.TestCase):
@@ -136,15 +138,11 @@ class TestNav(HubTest):
     def test_only_one_header(self):
         # For bug 682359.
         # Remove this test when we switch to Impala in the devhub!
-        url = reverse('devhub.addons')
-        r = self.client.get(url)
-        doc = pq(r.content)
-
-        # Make sure we're on a non-impala page
-        error = "This test should be run on a non-impala page"
-        assert doc('.is-impala').length == 0, error
-
-        assert doc('#header').length == 0, "Uh oh, there's two headers!"
+        doc = pq(self.client.get(reverse('devhub.addons')).content)
+        # Make sure we're on a non-impala page.
+        eq_(doc('.is-impala').length, 0,
+            'This test should be run on a non-impala page.')
+        eq_(doc('#header').length, 0, 'Uh oh, there are two headers!')
 
 
 class TestDashboard(HubTest):
@@ -201,7 +199,7 @@ class TestDashboard(HubTest):
 
         # Create 5 add-ons.
         self.clone_addon(5)
-        r = self.client.get(self.url + '?page=2')
+        r = self.client.get(self.url, dict(page=2))
         doc = pq(r.content)
         eq_(len(doc('.item .item-info')), 5)
         eq_(doc('nav.paginator').length, 1)
@@ -285,34 +283,80 @@ class TestAppDashboard(HubTest):
         self.url = reverse('devhub.apps')
         waffle.models.Flag.objects.create(name='accept-webapps', everyone=True)
 
-    def test_app_dashboard(self):
+    def test_dashboard(self):
         eq_(self.client.get(self.url).status_code, 200)
 
     def test_no_apps(self):
-        """Check that no apps are displayed for this user."""
         r = self.client.get(self.url)
-        doc = pq(r.content)
-        eq_(doc('.items .item').length, 0)
+        eq_(pq(r.content)('#dashboard .item').length, 0)
 
-    def test_app_pagination(self):
-        """Check that the correct info. is displayed for each app:
-        namely, that apps are paginated at 10 items per page, and that
-        when there is more than one page, the 'Sort by' header and pagination
-        footer appear.
-        """
+    def test_pagination(self):
         # Create 10 add-ons.
         self.clone_addon(10, addon_id=337141)
         r = self.client.get(self.url)
-        doc = pq(r.content)
-        eq_(len(doc('.item .item-info')), 10)
-        eq_(doc('nav.paginator').length, 0)
+        doc = pq(r.content)('#dashboard')
+        eq_(doc('.item').length, 10)
+        eq_(doc('#sorter').length, 0)
+        eq_(doc('.paginator').length, 0)
 
-        # Create 5 add-ons.
-        self.clone_addon(5, addon_id=337141)
-        r = self.client.get(self.url + '?page=2')
-        doc = pq(r.content)
-        eq_(len(doc('.item .item-info')), 5)
-        eq_(doc('nav.paginator').length, 1)
+
+class TestAppDashboardSorting(HubTest):
+
+    def setUp(self):
+        super(TestAppDashboardSorting, self).setUp()
+        self.clone_addon(11, addon_id=337141)
+        self.my_apps = self.user_profile.addons
+        self.url = reverse('devhub.apps')
+        waffle.models.Flag.objects.create(name='accept-webapps', everyone=True)
+
+    def test_pagination(self):
+        doc = pq(self.client.get(self.url).content)('#dashboard')
+        eq_(doc('.item').length, 10)
+        eq_(doc('#sorter').length, 1)
+        eq_(doc('.paginator').length, 1)
+
+        doc = pq(self.client.get(self.url, dict(page=2)).content)('#dashboard')
+        eq_(doc('.item').length, 1)
+        eq_(doc('#sorter').length, 1)
+        eq_(doc('.paginator').length, 1)
+
+    def test_default_sort(self):
+        test_default_sort(self, 'name', 'name', reverse=False,
+                          sel_class='extra-opt')
+
+    def test_free_sort(self):
+        for app in test_listing_sort(self, 'free', 'weekly_downloads'):
+            eq_(app.is_premium(), False)
+
+    def test_paid_sort(self):
+        apps = list(self.my_apps.all()[:3])
+        price = Price.objects.create(price='1.00')
+        for app in apps:
+            app.update(premium_type=amo.ADDON_PREMIUM)
+            AddonPremium.objects.create(addon=app, price=price)
+
+        for app in test_listing_sort(self, 'paid', 'weekly_downloads'):
+            eq_(app.is_premium(), True)
+
+    def test_price_sort(self):
+        apps = test_listing_sort(self, 'price', None, reverse=False,
+                                 sel_class='extra-opt')
+        eq_(apps,
+            list(self.my_apps.order_by('addonpremium__price__price')[:10]))
+
+    def test_rating_sort(self):
+        test_listing_sort(self, 'rating', 'bayesian_rating')
+
+    def test_newest_sort(self):
+        test_listing_sort(self, 'created', 'created', sel_class='extra-opt')
+
+    def test_name_sort(self):
+        test_listing_sort(self, 'name', 'name', reverse=False,
+                          sel_class='extra-opt')
+
+    def test_updated_sort(self):
+        test_listing_sort(self, 'updated', 'last_updated',
+                          sel_class='extra-opt')
 
 
 class TestUpdateCompatibility(amo.tests.TestCase):

apps/devhub/views.py

@@ -57,6 +57,7 @@ from stats.models import Contribution
 from translations.models import delete_translation
 from users.models import UserProfile
 from versions.models import Version
+from webapps.models import Webapp
 from webapps.views import AppFilter
 from zadmin.models import ValidationResult
 
@@ -82,10 +83,13 @@ def addon_listing(request, default='name', webapp=False):
     """Set up the queryset and filtering for addon listing for Dashboard."""
     Filter = AppFilter if webapp else AddonFilter
     if webapp:
-        qs = request.amo_user.addons.filter(type=amo.ADDON_WEBAPP)
+        qs = Webapp.objects.filter(
+            id__in=request.amo_user.addons.filter(type=amo.ADDON_WEBAPP))
+        model = Webapp
     else:
         qs = request.amo_user.addons.exclude(type=amo.ADDON_WEBAPP)
-    filter = Filter(request, qs, 'sort', default)
+        model = Addon
+    filter = Filter(request, qs, 'sort', default, model=model)
     return filter.qs, filter
 
 
@@ -105,7 +109,7 @@ def dashboard(request, webapp=False):
     addons, filter = addon_listing(request, webapp=webapp)
     addons = amo.utils.paginate(request, addons, per_page=10)
     blog_posts = _get_posts()
-    data = dict(addons=addons, sorting=filter.field,
+    data = dict(addons=addons, sorting=filter.field, filter=filter,
                 items=_get_items(None, request.amo_user.addons.all())[:4],
                 sort_opts=filter.opts, rss=_get_rss_feed(request),
                 blog_posts=blog_posts, timestamp=int(time.time()),

apps/webapps/models.py

@@ -39,19 +39,19 @@ class WebappManager(amo.models.ManagerBase):
         return self.reviewed().filter(_current_version__isnull=False,
                                       disabled_by_user=False)
 
-    def popular(self):
-        return self.listed().order_by('-weekly_downloads').with_index(
-            addons='downloads_type_idx')
+    def top_free(self, listed=True):
+        qs = self.listed() if listed else self
+        return (qs.exclude(premium_type=amo.ADDON_PREMIUM)
+                .exclude(addonpremium__price__price__isnull=False)
+                .order_by('-weekly_downloads')
+                .with_index(addons='downloads_type_idx'))
 
-    def top_free(self):
-        qs = self.exclude(premium_type=amo.ADDON_PREMIUM).filter(
-            addonpremium__price__price__isnull=True)
-        return qs & self.popular()
-
-    def top_paid(self):
-        qs = self.filter(premium_type=amo.ADDON_PREMIUM,
-                         addonpremium__price__price__isnull=False)
-        return qs & self.popular()
+    def top_paid(self, listed=True):
+        qs = self.listed() if listed else self
+        return (qs.filter(premium_type=amo.ADDON_PREMIUM,
+                          addonpremium__price__price__isnull=False)
+                .order_by('-weekly_downloads')
+                .with_index(addons='downloads_type_idx'))
 
     @skip_cache
     def pending(self):

apps/webapps/tests/test_views.py

@@ -12,10 +12,10 @@ import amo
 from amo.helpers import absolutify, numberfmt, page_title
 import amo.tests
 from amo.urlresolvers import reverse
-from addons.models import Addon, AddonUser, AddonPremium
+from addons.models import Addon, AddonUser
 from addons.tests.test_views import add_addon_author, test_hovercards
 from browse.tests import test_listing_sort, test_default_sort, TestMobileHeader
-from market.models import Price
+from market.models import AddonPremium, Price
 from sharing import SERVICES
 from translations.helpers import truncate
 from users.models import UserProfile
@@ -175,20 +175,17 @@ class TestListing(TestPremium):
         test_default_sort(self, 'downloads', 'weekly_downloads')
 
     def test_free_sort(self):
-        apps = test_listing_sort(self, 'free', 'weekly_downloads')
-        for a in apps:
-            eq_(a.is_premium(), False)
+        for app in test_listing_sort(self, 'free', 'weekly_downloads'):
+            eq_(app.is_premium(), False)
 
     def test_paid_sort(self):
-        apps = test_listing_sort(self, 'paid', 'weekly_downloads')
-        for a in apps:
-            eq_(a.is_premium(), True)
+        for app in test_listing_sort(self, 'paid', 'weekly_downloads'):
+            eq_(app.is_premium(), True)
 
     def test_price_sort(self):
         apps = test_listing_sort(self, 'price', None, reverse=False,
                                  sel_class='extra-opt')
         eq_(apps, list(Webapp.objects.listed()
-                       .filter(premium_type=amo.ADDON_PREMIUM)
                        .order_by('addonpremium__price__price')))
 
     def test_rating_sort(self):
@@ -205,9 +202,6 @@ class TestListing(TestPremium):
         test_listing_sort(self, 'updated', 'last_updated',
                           sel_class='extra-opt')
 
-    def test_upandcoming_sort(self):
-        test_listing_sort(self, 'hotness', 'hotness', sel_class='extra-opt')
-
 
 class TestDetail(WebappTest):
     fixtures = ['base/apps', 'base/addon_3615', 'base/addon_592', 'base/users']

apps/webapps/views.py

@@ -61,8 +61,7 @@ class AppFilter(addons.views.BaseFilter):
     extras = (('created', _lazy(u'Newest')),
               ('name', _lazy(u'Name')),
               ('price', loc(u'Price')),
-              ('updated', _lazy(u'Recently Updated')),
-              ('hotness', _lazy(u'Up & Coming')))
+              ('updated', _lazy(u'Recently Updated')))
 
 
 def app_listing(request):