filtering featured by the locale of either addoncategory or feature as appropriate (bug 567250)

apps/addons/helpers.py

@@ -1,3 +1,5 @@
+import random
+
 import jinja2
 
 from jingo import register, env
@@ -101,6 +103,17 @@ def tags_box(context, addon, tags=None):
     return c
 
 
+@register.filter
+@jinja2.contextfilter
+def shuffle_boundary(context, addons, boundary=None):
+    if not boundary:
+        return addons
+    split = addons[:boundary], addons[boundary:]
+    for s in split:
+        random.shuffle(s)
+    return split[0] + split[1]
+
+
 @register.inclusion_tag('addons/listing/items.html')
 @jinja2.contextfunction
 def addon_listing_items(context, addons, show_date=False, src=None,
@@ -110,8 +123,10 @@ def addon_listing_items(context, addons, show_date=False, src=None,
 
 @register.inclusion_tag('addons/listing/items_compact.html')
 @jinja2.contextfunction
-def addon_listing_items_compact(context, addons, show_date=False,
-                                src=None):
+def addon_listing_items_compact(context, addons, boundary=0,
+                                show_date=False, src=None):
+    if show_date == 'featured' and boundary:
+        addons = shuffle_boundary(context, list(addons), boundary)
     return new_context(**locals())
 
 

apps/addons/models.py

@@ -63,15 +63,50 @@ class AddonManager(amo.models.ManagerBase):
         """Get valid, enabled add-ons only"""
         return self.filter(self.valid_q(amo.LISTED_STATUSES))
 
-    def featured(self, app):
+    def featured(self, app, by_locale=False):
         """
-        Filter for all featured add-ons for an application in all locales.
+        Filter for all featured add-ons for an application. Specify
+        by_locale to have it filtered by the current locale.
         """
-        return self.valid().filter(feature__application=app.id)
+        qs = Q(feature__application=app.id)
+        if by_locale:
+            locales = []
+            for locale in by_locale():
+                locale = to_language(locale) if locale else locale
+                locales.append(Q(feature__locale=locale))
 
-    def category_featured(self):
-        """Get all category-featured add-ons for ``app`` in all locales."""
-        return self.filter(addoncategory__feature=True)
+            qs = qs & Q(feature__isnull=False) & reduce(operator.or_, locales)
+            return (self.valid().filter(qs).order_by('-feature__locale'))
+
+        return self.valid().filter(qs)
+
+    def category_featured(self, category=None, by_locale=False):
+        """
+        Filter for all featured addons for a category. Specify
+        by_locale to have it filtered by the current locale.
+        """
+        qs = Q(addoncategory__feature=True)
+
+        if by_locale:
+            locales = []
+            for locale in by_locale():
+                prefix = 'addoncategory__feature_locales'
+                if locale:
+                    locale = to_language(locale)
+                    prefix += '__contains'
+                else:
+                    # Sadly looks like theres '' and NULL in the column,
+                    # hopefully the new admin tools will clean this out.
+                    locales.append(Q(**{prefix: ''}))
+                locales.append(Q(**{prefix: locale}))
+
+            qs = (qs & Q(addoncategory__category=category) &
+                       reduce(operator.or_, locales))
+            return self.filter(qs).order_by('-addoncategory__feature_locales')
+
+        if category:
+            qs = qs & Q(addoncategory__category=category)
+        return self.filter(qs)
 
     def listed(self, app, *status):
         """

apps/addons/templates/addons/home.html

@@ -80,8 +80,8 @@
       <div class="addon-listing addon-listing-{{ filter.field }}">
         {% for key, addons in addon_sets.items() %}
           <div id="list-{{ key }}">
-            {{ addon_listing_items_compact(addons, show_date=key,
-                                           src='homepagebrowse') }}
+            {{ addon_listing_items_compact(addons, boundary=boundary,
+                                           show_date=key, src='homepagebrowse') }}
             <div class="listing-footer">
               <a class="subscribe" href="{{ view_all[key]['feed'] }}">
                 {{ _('Subscribe', 'addons_home_browse_subscribe') }}</a>

apps/addons/views.py

@@ -256,7 +256,8 @@ class HomepageFilter(BaseFilter):
 
     def filter_featured(self):
         # It's ok to cache this for a while...it'll expire eventually.
-        return Addon.objects.featured(self.request.APP).order_by('?')
+        return Addon.objects.featured(self.request.APP,
+                                      by_locale=amo.LOCALE_ALL)
 
 
 def home(request):
@@ -264,6 +265,8 @@ def home(request):
     base = Addon.objects.listed(request.APP).exclude(type=amo.ADDON_PERSONA)
     filter = HomepageFilter(request, base, key='browse', default='featured')
     addon_sets = dict((key, qs[:4]) for key, qs in filter.all().items())
+    boundary = (Addon.objects.featured(request.APP,
+                                       by_locale=amo.LOCALE_CURRENT).count())
 
     # Collections.
     q = Collection.objects.filter(listed=True, application=request.APP.id)
@@ -284,7 +287,8 @@ def home(request):
     return jingo.render(request, 'addons/home.html',
                         {'downloads': downloads, 'top_tags': top_tags,
                          'filter': filter, 'addon_sets': addon_sets,
-                         'collections': collections, 'promobox': promobox})
+                         'collections': collections, 'promobox': promobox,
+                         'boundary': boundary})
 
 
 @dec.mobilized(home)

apps/amo/__init__.py

@@ -10,6 +10,7 @@ from tower import ugettext_lazy as _
 
 from licenses import license_text
 from .log import LOG, LOG_BY_ID, LOG_KEEP, log
+from django.utils.translation import trans_real as translation
 
 logger_log = commonware.log.getLogger('z.amo')
 
@@ -211,6 +212,9 @@ ADDON_ICON_SIZES = [32, 48, 64]
 # Preview upload sizes [thumb, full]
 ADDON_PREVIEW_SIZES = [(200, 150), (700, 525)]
 
+LOCALE_CURRENT = lambda: (translation.get_language(),)
+LOCALE_ALL = lambda: (None, translation.get_language())
+
 # These types don't maintain app compatibility in the db.  Instead, we look at
 # APP.types and APP_TYPE_SUPPORT to figure out where they are compatible.
 NO_COMPAT = (ADDON_SEARCH, ADDON_PERSONA)

apps/browse/templates/browse/category_landing.html

@@ -28,7 +28,7 @@
         {% cache addons %}
         {# The featured add-ons for a category should be small, so we get them all
            with the if.  Shuffling here to take advantage of the cached fragment. #}
-          {% for addon in (addons|list|shuffle)[:3] %}
+          {% for addon in (addons|list|shuffle_boundary(boundary))[:3] %}
           <li class="addon item">
             <h4>
               <a href="{{ addon.get_url_path() }}">

apps/browse/templates/browse/creatured.html

@@ -14,7 +14,7 @@
 <div class="featured listing">
   <div class="featured-inner">
     {% if addons %}
-      {{ addon_listing_items(addons, src='creatured') }}
+      {{ addon_listing_items(addons|list|shuffle_boundary(boundary), src='creatured') }}
     {% else %}
       <p class="no-results">{{ _('No featured add-ons in {0}.')|f(category.name) }}</p>
     {% endif %}

apps/browse/templates/browse/featured.html

@@ -28,7 +28,7 @@
   {% cache addons %}
   <div class="featured listing">
     <div class="featured-inner">
-      {{ addon_listing_items(addons|list|shuffle, src="recommended") }}
+      {{ addon_listing_items(addons|list|shuffle_boundary(boundary), src="recommended") }}
     </div>
   </div>
   {% endcache %}

apps/browse/templates/browse/personas/category_landing.html

@@ -27,11 +27,9 @@
       <div class="featured-inner">
         {% set addons = addon_sets[special][:16] %}
         <ul class="personas-slider">
-          {% cache addons %}
           {% for addon in addons %}
             <li>{{ persona_preview(addon.persona, size='small') }}</li>
           {% endfor %}
-          {% endcache %}
         </ul>
         {{ breadcrumbs([(None, _('Personas'))]) }}
         <h2>{{ _('Personas') }}</h2>
@@ -44,13 +42,13 @@
       </div>
     </div>
 
-    {% set addons = featured[:6] %}
+    {% set addons = featured|list|shuffle_boundary(boundary) %}
     {% if addons %}
       <div class="personas-featured">
         <h3>{{ _('Featured Personas') }}</h3>
         <ul class="persona-list-3col">
           {% cache addons %}
-            {% for addon in addons %}
+            {% for addon in addons[:6] %}
               <li>{{ persona_preview(addon.persona, size='small') }}</li>
             {% endfor %}
           {% endcache %}

apps/browse/tests.py

@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 import datetime
 from datetime import timedelta
+import re
 from urlparse import urlparse
 
 from django import http
@@ -252,6 +253,173 @@ class TestCategoryPages(test_utils.TestCase):
         eq_(len(doc('.item')), 1)
 
 
+class TestFeaturedLocale(test_utils.TestCase):
+    fixtures = ('base/apps', 'base/category', 'base/addon_3615',
+                'base/featured', 'addons/featured', 'browse/nameless-addon')
+
+    def setUp(self):
+        self.addon = Addon.objects.get(pk=3615)
+        self.persona = Addon.objects.get(pk=15679)
+        self.extension = Addon.objects.get(pk=2464)
+        self.category = Category.objects.get(slug='bookmarks')
+
+        self.url = reverse('browse.creatured', args=['bookmarks'])
+
+    def change_addoncategory(self, addon, locale='es-ES'):
+        ac = addon.addoncategory_set.all()[0]
+        ac.feature_locales = locale
+        ac.save()
+
+    def change_addon(self, addon, locale='es-ES'):
+        feature = addon.feature_set.all()[0]
+        feature.locale = locale
+        feature.save()
+
+    def list_featured(self, content):
+        # Not sure we want to get into testing randomness
+        # between multiple executions of a page, but if this is a quick
+        # way to print out the results and check yourself that they
+        # are changing.
+        doc = pq(content)
+        ass = doc('.featured-inner .item a')
+        rx = re.compile('/(en-US|es-ES)/firefox/addon/(\d+)/$')
+        for a in ass:
+            mtch = rx.match(a.attrib['href'])
+            if mtch:
+                print mtch.group(2)
+
+    def test_featured(self):
+        addons = (Addon.objects.featured(app=amo.FIREFOX,
+                                         by_locale=amo.LOCALE_ALL))
+        eq_(addons.count(), 6)
+
+    def test_category_featured(self):
+        cat = Category.objects.get(id=22)
+        addons = (Addon.objects.category_featured(category=cat,
+                                                  by_locale=amo.LOCALE_ALL))
+        eq_(addons.count(), 2)
+
+    def test_creatured_locale_en_US(self):
+        res = self.client.get(self.url)
+        assert self.addon in res.context['addons']
+
+    def test_creatured_locale_nones(self):
+        self.change_addoncategory(self.addon, '')
+        res = self.client.get(self.url)
+        assert self.addon in res.context['addons']
+
+        self.change_addoncategory(self.addon, None)
+        res = self.client.get(self.url)
+        assert self.addon in res.context['addons']
+
+    def test_creatured_locale_many(self):
+        self.change_addoncategory(self.addon, 'en-US,es-ES')
+        res = self.client.get(self.url)
+        assert self.addon in res.context['addons']
+
+        res = self.client.get(self.url.replace('en-US', 'es-ES'))
+        assert self.addon in res.context['addons']
+
+    def test_creatured_locale_not_en_US(self):
+        self.change_addoncategory(self.addon, 'es-ES')
+        res = self.client.get(self.url)
+        assert self.addon not in res.context['addons']
+
+    def test_creatured_locale_es_ES(self):
+        res = self.client.get(self.url.replace('en-US', 'es-ES'))
+        assert self.addon in res.context['addons']
+
+    def test_featured_locale_en_US(self):
+        res = self.client.get(reverse('browse.featured'))
+        assert self.extension in res.context['addons']
+
+    def test_featured_locale_es_ES(self):
+        addon = self.extension
+        self.change_addon(addon, 'es-ES')
+        url = reverse('browse.featured')
+        res = self.client.get(url)
+        assert addon not in res.context['addons']
+
+        res = self.client.get(url.replace('en-US', 'es-ES'))
+        assert addon in res.context['addons']
+
+    def test_featured_extensions_no_category_en_US(self):
+        addon = self.extension
+        res = self.client.get(reverse('browse.extensions'))
+        assert addon in res.context['addons'].object_list
+
+    def test_featured_extensions_with_category_es_ES(self):
+        addon = self.addon
+        res = self.client.get(reverse('browse.extensions', args=['bookmarks']))
+        assert addon in res.context['filter'].all()['featured']
+
+        self.change_addoncategory(addon, 'es-ES')
+        res = self.client.get(reverse('browse.extensions', args=['bookmarks']))
+        assert addon not in res.context['filter'].all()['featured']
+
+    def test_featured_persona_no_category_en_US(self):
+        addon = self.persona
+        url = reverse('browse.personas')
+        res = self.client.get(url)
+        assert addon in res.context['featured']
+
+        self.change_addon(addon, 'es-ES')
+        res = self.client.get(url)
+        assert addon not in res.context['featured']
+
+        res = self.client.get(url.replace('en-US', 'es-ES'))
+        assert addon in res.context['featured']
+
+    def test_featured_persona_category_en_US(self):
+        addon = self.persona
+        category = Category.objects.get(id=22)
+        category.update(type=amo.ADDON_PERSONA)
+
+        addon.addoncategory_set.create(category=category, feature=True)
+        url = reverse('browse.personas', args=[category.slug])
+        res = self.client.get(url)
+        assert addon in res.context['featured']
+
+        self.change_addoncategory(addon, 'es-ES')
+        res = self.client.get(url)
+        assert addon not in res.context['featured']
+
+        res = self.client.get(url.replace('en-US', 'es-ES'))
+        assert addon in res.context['featured']
+
+    def test_homepage(self):
+        addon = self.persona
+        url = reverse('home')
+        res = self.client.get(url)
+        assert addon in res.context['filter'].filter_featured()
+
+        self.change_addon(addon, 'es-ES')
+        res = self.client.get(url)
+        assert addon not in res.context['filter'].filter_featured()
+
+        res = self.client.get(url.replace('en-US', 'es-ES'))
+        assert addon in res.context['filter'].filter_featured()
+
+    def test_homepage_order(self):
+        another = Addon.objects.get(id=1003)
+        self.change_addon(another, 'en-US')
+        url = reverse('home')
+        res = self.client.get(url)
+        items = res.context['filter'].filter_featured()
+        # The order should be random within those boundaries.
+        eq_([1003, 3481], sorted([i.pk for i in items[0:2]]))
+        eq_([1001, 2464, 7661, 15679], sorted([i.pk for i in items[2:]]))
+
+        res = self.client.get(url.replace('en-US', 'es-ES'))
+        items = res.context['filter'].filter_featured()
+        eq_([1001, 2464, 7661, 15679], sorted([i.pk for i in items]))
+
+        self.change_addon(another, 'es-ES')
+        items = res.context['filter'].filter_featured()
+        assert items[0].pk == another.pk
+        eq_([1001, 2464, 7661, 15679], sorted([i.pk for i in items[1:]]))
+
+
 class TestListingByStatus(test_utils.TestCase):
     fixtures = ['base/apps', 'base/addon_3615']
 
@@ -609,7 +777,6 @@ class TestFeaturedPage(test_utils.TestCase):
 
     def test_featured_addons(self):
         """Make sure that only featured add-ons are shown"""
-
         response = self.client.get(reverse('browse.featured'))
         eq_([1001, 1003], sorted(a.id for a in response.context['addons']))
 

apps/browse/views.py

@@ -178,8 +178,8 @@ class CategoryLandingFilter(BaseFilter):
                                                     default)
 
     def filter_featured(self):
-        return Addon.objects.filter(addoncategory__feature=True,
-                                    addoncategory__category=self.category)
+        return Addon.objects.category_featured(category=self.category,
+                                              by_locale=amo.LOCALE_ALL)
 
 
 def category_landing(request, category):
@@ -187,8 +187,11 @@ def category_landing(request, category):
             .filter(categories__id=category.id))
     filter = CategoryLandingFilter(request, base, category,
                                    key='browse', default='featured')
+    boundary = (Addon.objects.category_featured(category=category,
+                              by_locale=amo.LOCALE_CURRENT).count())
     return jingo.render(request, 'browse/category_landing.html',
                         {'category': category, 'filter': filter,
+                         'boundary': boundary,
                          'search_cat': '%s,0' % category.type})
 
 
@@ -196,10 +199,14 @@ def creatured(request, category):
     TYPE = amo.ADDON_EXTENSION
     q = Category.objects.filter(application=request.APP.id, type=TYPE)
     category = get_object_or_404(q, slug=category)
-    addons = Addon.objects.public().filter(addoncategory__feature=True,
-                                           addoncategory__category=category)
+    addons = (Addon.objects.public() &
+              Addon.objects.category_featured(category=category,
+                                              by_locale=amo.LOCALE_ALL))
+    boundary = (Addon.objects.category_featured(category=category,
+                              by_locale=amo.LOCALE_CURRENT).count())
     return jingo.render(request, 'browse/creatured.html',
-                        {'addons': addons, 'category': category})
+                        {'addons': addons, 'category': category,
+                         'boundary': boundary})
 
 
 class PersonasFilter(BaseFilter):
@@ -259,13 +266,24 @@ def personas(request, category=None):
         template = 'category_landing.html'
 
     addons = amo.utils.paginate(request, filter.qs, 30, count=count)
+    boundary = 0
+    if category:
+        featured = base & Addon.objects.category_featured(category=category,
+                                by_locale=amo.LOCALE_ALL)
+        boundary = (Addon.objects.category_featured(category=category,
+                          by_locale=amo.LOCALE_CURRENT).count())
+    else:
+        featured = base & Addon.objects.featured(request.APP,
+                                by_locale=amo.LOCALE_ALL)
+        boundary = (Addon.objects.featured(request.APP,
+                          by_locale=amo.LOCALE_CURRENT).count())
 
-    featured = base & Addon.objects.featured(request.APP)
     is_homepage = category is None and 'sort' not in request.GET
     return jingo.render(request, 'browse/personas/' + template,
                         {'categories': categories, 'category': category,
                          'filter': filter, 'addons': addons,
-                         'featured': featured, 'is_homepage': is_homepage,
+                         'featured': featured,
+                         'boundary': boundary, 'is_homepage': is_homepage,
                          'search_cat': 'personas'})
 
 
@@ -381,5 +399,8 @@ def search_tools(request, category=None):
 
 
 def featured(request, category=None):
-    addons = Addon.objects.featured(request.APP)
-    return jingo.render(request, 'browse/featured.html', {'addons': addons})
+    count = (Addon.objects.featured(request.APP, by_locale=amo.LOCALE_CURRENT)
+                                    .count())
+    addons = Addon.objects.featured(request.APP, by_locale=amo.LOCALE_ALL)
+    return jingo.render(request, 'browse/featured.html',
+                        {'addons': addons, 'boundary': count})