Add region/category/carrier filtering on Collections (bug 903489)

Also, implement dynamic filters fallback : if the current set of
filters don't return results, remove one and try again till we have
exausted available filters or we have found some results.

django-filter 0.7 upgrade is necessary to fix a bug regarding filtering
on '0' values.

docs/api/topics/rocketfuel.rst

@@ -36,10 +36,21 @@ Create
 
     **Request**:
 
+    :param collection_type: the type of collection to create.
+    :type collection_type: int
     :param name: the name of the collection.
     :type name: string
     :param description: a description of the collection.
     :type description: string
+    :param category: the ID of the category to attach this collection to.
+    Defaults to ``null``.
+    :type collection_type: int|null
+    :param region: the ID of the region to attach this collection to.
+    Defaults to ``null``.
+    :type region: int|null
+    :param carrier: the ID of the carrier to attach this collection to.
+    Defaults to ``null``.
+    :type carrier: int|null
 
 
 Detail
@@ -63,10 +74,18 @@ Update
 
     **Request**:
 
+    :param collection_type: the type of the collection.
+    :type collection_type: int
     :param name: the name of the collection.
     :type name: string
     :param description: a description of the collection.
     :type description: string
+    :param category: the ID of the category to attach this collection to.
+    :type collection_type: int|null
+    :param region: the ID of the region to attach this collection to.
+    :type region: int|null
+    :param carrier: the ID of the carrier to attach this collection to.
+    :type carrier: int|null
 
     **Response**:
 

migrations/639-add-collections-carrier-region-cat.sql

@@ -0,0 +1,6 @@
+ALTER TABLE `app_collections` ADD COLUMN `region` integer UNSIGNED DEFAULT NULL;
+ALTER TABLE `app_collections` ADD COLUMN `carrier` integer UNSIGNED DEFAULT NULL;
+ALTER TABLE `app_collections` ADD COLUMN `category_id` integer;
+CREATE INDEX `app_collections_region_idx` ON `app_collections` (`region`);
+CREATE INDEX `app_collections_carrier_idx` ON `app_collections` (`carrier`);
+CREATE INDEX `app_collections_category_id_idx` ON `app_collections` (`category_id`);

mkt/collections/filters.py

@@ -0,0 +1,46 @@
+from django_filters.filterset import FilterSet
+
+from mkt.collections.models import Collection
+
+
+class CollectionFilterSetWithFallback(FilterSet):
+    class Meta:
+        model = Collection
+        fields = ['carrier', 'region', 'category']
+
+    # Fields that can be removed when filtering the queryset if there
+    # are no results for the currently applied filters, in the order
+    # that they'll be removed. See `next_fallback`.
+    fields_fallback_order = ('carrier', 'region', 'category')
+
+    def next_fallback(self):
+        """
+        Return the next field to remove from the filters if we didn't find any
+        results with the ones currently in use. It relies on
+        `fields_fallback_order`.
+
+        The `qs` property will call this method and use it to remove filters
+        from `self.data` till a result is returned by the queryset or
+        there are no filter left to remove.
+        """
+        for f in self.fields_fallback_order:
+            if f in self.data:
+                return f
+        return None
+
+    def __init__(self, *args, **kwargs):
+        super(CollectionFilterSetWithFallback, self).__init__(*args, **kwargs)
+        # Make self.data mutable for later.
+        self.data = self.data.copy()
+
+    @property
+    def qs(self):
+        qs = super(CollectionFilterSetWithFallback, self).qs
+        next_fallback = self.next_fallback()
+        if next_fallback and not qs.exists():
+            # FIXME: add current filter set to API-Filter in response.
+            self.data.pop(next_fallback)
+            del self._form
+            del self._qs
+            qs = self.qs
+        return qs

mkt/collections/models.py

@@ -2,6 +2,8 @@ from django.db import models
 from django.db.models import Max
 
 import amo.models
+import mkt.regions
+from addons.models import Category
 from mkt.webapps.models import Webapp
 from translations.fields import PurifiedField, save_signal
 
@@ -9,12 +11,22 @@ from .constants import COLLECTION_TYPES
 
 
 class Collection(amo.models.ModelBase):
-    collection_type = models.IntegerField(choices=COLLECTION_TYPES, null=True)
+    collection_type = models.IntegerField(choices=COLLECTION_TYPES)
     description = PurifiedField()
     name = PurifiedField()
+    # FIXME: add better / composite indexes that matches the query we are
+    # going to make.
+    category = models.ForeignKey(Category, null=True, blank=True)
+    region = models.PositiveIntegerField(default=None, null=True, blank=True,
+        choices=mkt.regions.REGIONS_CHOICES_ID, db_index=True)
+    carrier = models.IntegerField(default=None, null=True, blank=True,
+        choices=mkt.carriers.CARRIER_CHOICES, db_index=True)
 
     class Meta:
         db_table = 'app_collections'
+        ordering = ('-id',)  # This will change soon since we'll need to be
+                             # able to order collections themselves, but this
+                             # helps tests for now.
 
     def __unicode__(self):
         return self.name.localized_string_clean

mkt/collections/serializers.py

@@ -19,6 +19,6 @@ class CollectionSerializer(serializers.ModelSerializer):
 
     class Meta:
         fields = ('collection_type', 'description', 'id', 'name',
-                  'apps')
+                  'apps', 'category', 'region', 'carrier')
         model = Collection
 

mkt/collections/tests/test_serializers.py

@@ -46,7 +46,8 @@ class TestCollectionSerializer(CollectionDataMixin, amo.tests.TestCase):
         for name, value in self.collection_data.iteritems():
             eq_(self.collection_data[name], data[name])
         self.assertSetEqual(data.keys(), ['id', 'name', 'description', 'apps',
-                                          'collection_type'])
+                                          'collection_type', 'category',
+                                          'region', 'carrier'])
         for order, app in enumerate(apps):
             eq_(data['apps'][order]['slug'], app.app_slug)
 

mkt/collections/tests/test_views.py

@@ -3,10 +3,14 @@ from random import shuffle
 
 from django.core.urlresolvers import reverse
 
+from nose import SkipTest
 from nose.tools import eq_
 from rest_framework.exceptions import PermissionDenied
 
+import amo
 import amo.tests
+import mkt
+from addons.models import Category
 from mkt.api.tests.test_oauth import RestOAuth
 from mkt.collections.constants import COLLECTIONS_TYPE_BASIC
 from mkt.collections.models import Collection
@@ -59,6 +63,41 @@ class TestCollectionViewSet(RestOAuth):
         for field, value in self.collection_data.iteritems():
             eq_(collection[field], self.collection_data[field])
 
+    def create_additional_data(self):
+        self.category = Category.objects.create(slug='Cat',
+                                                type=amo.ADDON_WEBAPP)
+        self.empty_category = Category.objects.create(slug='Empty Cat',
+                                                      type=amo.ADDON_WEBAPP)
+        eq_(Category.objects.count(), 2)
+
+        collection_data = {
+            'collection_type': COLLECTIONS_TYPE_BASIC,
+            'description': 'A collection of my favorite spanish games',
+            'name': 'My Favorite spanish games',
+            'region': mkt.regions.SPAIN.id,
+            'carrier': mkt.carriers.UNKNOWN_CARRIER.id,
+        }
+        self.collection2 = Collection.objects.create(**collection_data)
+
+        collection_data = {
+            'collection_type': COLLECTIONS_TYPE_BASIC,
+            'description': 'A collection of my favorite phone games',
+            'name': 'My Favorite phone games',
+            'region': mkt.regions.SPAIN.id,
+            'carrier': mkt.carriers.TELEFONICA.id,
+        }
+        self.collection3 = Collection.objects.create(**collection_data)
+
+        collection_data = {
+            'collection_type': COLLECTIONS_TYPE_BASIC,
+            'description': 'A collection of my favorite categorized games',
+            'name': 'My Favorite categorized games',
+            'region': mkt.regions.SPAIN.id,
+            'carrier': mkt.carriers.TELEFONICA.id,
+            'category': self.category
+        }
+        self.collection4 = Collection.objects.create(**collection_data)
+
     def test_listing(self):
         self.listing(self.anon)
 
@@ -69,6 +108,133 @@ class TestCollectionViewSet(RestOAuth):
         self.make_publisher()
         self.listing(self.client)
 
+    def test_listing_no_filtering(self):
+        self.create_additional_data()
+        self.make_publisher()
+
+        res = self.client.get(self.list_url)
+        eq_(res.status_code, 200)
+        data = json.loads(res.content)
+        collections = data['objects']
+        eq_(len(collections), 4)
+
+    def test_listing_filtering_region(self):
+        self.create_additional_data()
+        self.make_publisher()
+
+        res = self.client.get(self.list_url, {'region': mkt.regions.SPAIN.id})
+        eq_(res.status_code, 200)
+        data = json.loads(res.content)
+        collections = data['objects']
+        eq_(len(collections), 3)
+        eq_(collections[0]['id'], self.collection4.id)
+        eq_(collections[1]['id'], self.collection3.id)
+        eq_(collections[2]['id'], self.collection2.id)
+
+    def test_listing_filtering_carrier(self):
+        self.create_additional_data()
+        self.make_publisher()
+
+        res = self.client.get(self.list_url, 
+            {'carrier': mkt.carriers.TELEFONICA.id})
+        eq_(res.status_code, 200)
+        data = json.loads(res.content)
+        collections = data['objects']
+        eq_(len(collections), 2)
+        eq_(collections[0]['id'], self.collection4.id)
+        eq_(collections[1]['id'], self.collection3.id)
+
+    def test_listing_filtering_carrier_0(self):
+        self.create_additional_data()
+        self.make_publisher()
+
+        # Test filtering on carrier with value 0 (which is valid, and different
+        # from the default, which is null). There used to be a bug with falsy
+        # values in django-filter 0.6, make sure it doesn't come back.
+        res = self.client.get(self.list_url, {'carrier': 0})
+        eq_(res.status_code, 200)
+        data = json.loads(res.content)
+        collections = data['objects']
+        eq_(len(collections), 1)
+        eq_(collections[0]['id'], self.collection2.id)
+
+    def test_listing_filtering_category(self):
+        self.create_additional_data()
+        self.make_publisher()
+
+        res = self.client.get(self.list_url, {'category': self.category.pk})
+        eq_(res.status_code, 200)
+        data = json.loads(res.content)
+        collections = data['objects']
+        eq_(len(collections), 1)
+        eq_(collections[0]['id'], self.collection4.id)
+
+    def test_listing_filtering_category_region_carrier(self):
+        self.create_additional_data()
+        self.make_publisher()
+
+        res = self.client.get(self.list_url, {
+            'category': self.category.pk,
+            'region': mkt.regions.SPAIN.id,
+            'carrier': mkt.carriers.SPRINT.id
+        })
+        eq_(res.status_code, 200)
+        data = json.loads(res.content)
+        collections = data['objects']
+        eq_(len(collections), 1)
+        eq_(collections[0]['id'], self.collection4.id)
+
+    def test_listing_filterig_category_region_carrier_fallback(self):
+        self.create_additional_data()
+        self.make_publisher()
+
+        # Test filtering with a non-existant category + region + carrier.
+        # It should fall back on region+category filtering only, not find
+        # anything either, then fall back to category only, then remove
+        # all filters because of the order in which filters are dropped.
+        res = self.client.get(self.list_url, {
+            'category': self.empty_category.pk,
+            'region': mkt.regions.SPAIN.id,
+            'carrier': mkt.carriers.SPRINT.id
+        })
+        eq_(res.status_code, 200)
+        data = json.loads(res.content)
+        collections = data['objects']
+        eq_(len(collections), 4)
+
+    def test_listing_filtering_nonexistant_carrier(self):
+        self.create_additional_data()
+        self.make_publisher()
+
+        # Test filtering with a non-existant carrier. It should fall back on
+        # region filtering only.
+        res = self.client.get(self.list_url, {
+            'region': mkt.regions.SPAIN.id,
+            'carrier': mkt.carriers.SPRINT.id
+        })
+        eq_(res.status_code, 200)
+        data = json.loads(res.content)
+        collections = data['objects']
+        eq_(len(collections), 3)
+        eq_(collections[0]['id'], self.collection4.id)
+        eq_(collections[1]['id'], self.collection3.id)
+        eq_(collections[2]['id'], self.collection2.id)
+
+    def test_listing_filtering_nonexistant_carrier_and_region(self):
+        self.create_additional_data()
+        self.make_publisher()
+
+        # Test filtering with a non-existant carrier and region. It should
+        # fall back on no filtering.
+        res = self.client.get(self.list_url, {
+            'region': mkt.regions.UK.id,
+            'carrier': mkt.carriers.SPRINT.id
+        })
+        eq_(res.status_code, 200)
+        data = json.loads(res.content)
+        collections = data['objects']
+        eq_(len(collections), 4)
+
     def detail(self, client):
         apps = self.apps[:2]
         for app in apps:
@@ -114,6 +280,12 @@ class TestCollectionViewSet(RestOAuth):
         res, data = self.create(self.client)
         eq_(res.status_code, 201)
 
+    def test_create_has_perms_no_type(self):
+        self.make_publisher()
+        self.collection_data.pop('collection_type')
+        res, data = self.create(self.client)
+        eq_(res.status_code, 400)
+
     def add_app(self, client, app_id=None):
         if app_id is None:
             app_id = self.apps[0].pk
@@ -220,15 +392,50 @@ class TestCollectionViewSet(RestOAuth):
 
     def test_edit_collection_has_perms(self):
         self.make_publisher()
+        cat = Category.objects.create(type=amo.ADDON_WEBAPP, slug='grumpy-cat')
         updates = {
             'name': 'clouserw soundboard',
-            'description': 'Get off my lawn!'
+            'description': 'Get off my lawn!',
+            'region': mkt.regions.SPAIN.id,
+            'category': cat.pk,
         }
         res, data = self.edit_collection(self.client, **updates)
         eq_(res.status_code, 200)
         for key, value in updates.iteritems():
             eq_(data[key], value)
 
+    def test_edit_collection_invalid_carrier(self):
+        self.make_publisher()
+        # Invalid carrier.
+        updates = {'carrier': 1576}
+        res, data = self.edit_collection(self.client, **updates)
+        eq_(res.status_code, 400)
+
+    def test_edit_collection_invalid_region_0(self):
+        # 0 is an invalid region. Unfortunately, because django bug #18724 is
+        # fixed in django 1.5 but not 1.4, '0' values are accepted.
+        # Unskip this test when using django 1.5.
+        raise SkipTest('Test that needs django 1.5 to pass')
+        self.make_publisher()
+        updates = {'region': 0}
+        res, data = self.edit_collection(self.client, **updates)
+        eq_(res.status_code, 400)
+
+    def test_edit_collection_invalid_region(self):
+        self.make_publisher()
+        # Invalid region id.
+        updates = {'region': max(mkt.regions.REGION_IDS) + 1}
+        res, data = self.edit_collection(self.client, **updates)
+        eq_(res.status_code, 400)
+
+    def test_edit_collection_category(self):
+        self.make_publisher()
+        eq_(Category.objects.count(), 0)
+        # Invalid (non-existant) category.
+        updates = {'category': 1}
+        res, data = self.edit_collection(self.client, **updates)
+        eq_(res.status_code, 400)
+
     def reorder(self, client, order=None):
         if order is None:
             order = {}

mkt/collections/views.py

@@ -11,6 +11,7 @@ from mkt.api.base import CORSMixin
 from mkt.webapps.models import Webapp
 
 from .authorization import PublisherAuthorization
+from .filters import CollectionFilterSetWithFallback
 from .models import Collection
 from .serializers import CollectionMembershipField, CollectionSerializer
 
@@ -22,6 +23,7 @@ class CollectionViewSet(CORSMixin, viewsets.ModelViewSet):
     permission_classes = [PublisherAuthorization]
     authentication_classes = [RestOAuthAuthentication,
                               RestAnonymousAuthentication]
+    filter_class = CollectionFilterSetWithFallback
 
     exceptions = {
         'not_provided': '`app` was not provided.',

requirements/prod.txt

@@ -21,7 +21,7 @@ django-celery==3.0.17
 django-cronjobs==0.2.3
 django_csp==1.0.2
 django-extensions==0.9
-django-filter==0.6
+django-filter==0.7
 django-memcached-pool==0.4.1
 django-multidb-router==0.5
 django-mysql-pool==0.2