Merge pull request #1477 from diox/fix-collections-500

Hide non-public apps inside collections (bug 941337)

mkt/collections/models.py

@@ -5,7 +5,7 @@ from django.db import models
 
 import amo.models
 import mkt.regions
-from addons.models import Category, clean_slug
+from addons.models import Addon, Category, clean_slug
 from amo.decorators import use_master
 from amo.utils import to_language
 from mkt.webapps.models import Webapp
@@ -76,7 +76,15 @@ class Collection(amo.models.ModelBase):
                             'app_collection_%s.png' % (self.pk,))
 
     def apps(self):
-        return self._apps.order_by('collectionmembership')
+        """
+        Public apps on the collection, ordered by their position in the
+        CollectionMembership model.
+
+        Use this method everytime you want to display apps for a collection to
+        an user.
+        """
+        return self._apps.filter(disabled_by_user=False,
+            status=amo.STATUS_PUBLIC).order_by('collectionmembership')
 
     def add_app(self, app, order=None):
         """
@@ -159,5 +167,16 @@ class CollectionMembership(amo.models.ModelBase):
         ordering = ('order',)
 
 
+def remove_deleted_apps(*args, **kwargs):
+    instance = kwargs.get('instance')
+    CollectionMembership.objects.filter(app_id=instance.pk).delete()
+
+
+# Save translations when saving a Collection.
 models.signals.pre_save.connect(save_signal, sender=Collection,
                                 dispatch_uid='collection_translations')
+
+# Delete collection membership when deleting an app (sender needs to be Addon,
+# not Webapp, because that's the real model underneath).
+models.signals.post_delete.connect(remove_deleted_apps, sender=Addon,
+                                   dispatch_uid='apps_collections_cleanup')

mkt/collections/serializers.py

@@ -34,14 +34,15 @@ from .constants import COLLECTIONS_TYPE_FEATURED, COLLECTIONS_TYPE_OPERATOR
 
 class CollectionMembershipField(serializers.RelatedField):
     """
-    RelatedField subclass that serializes an M2M to CollectionMembership into
-    a list of apps, rather than a list of CollectionMembership objects.
+    RelatedField subclass that serializes apps in a Collection, taking into
+    account feature profile and optionally relying on ElasticSearch to find
+    the apps instead of making a DB query.
 
     Specifically created for use with CollectionSerializer; you probably don't
     want to use this elsewhere.
     """
     def to_native(self, value):
-        return AppSerializer(value.app, context=self.context).data
+        return AppSerializer(value, context=self.context).data
 
     def field_to_native(self, obj, field_name):
         if not hasattr(self, 'context') or not 'request' in self.context:
@@ -56,15 +57,15 @@ class CollectionMembershipField(serializers.RelatedField):
             and waffle.switch_is_active('collections-use-es-for-apps')):
             return self.field_to_native_es(obj, request)
 
-        value = get_component(obj, self.source)
+        qs = get_component(obj, self.source)
 
         # Filter apps based on feature profiles.
         profile = get_feature_profile(request)
         if profile:
-            value = value.filter(**profile.to_kwargs(
-                prefix='app___current_version__features__has_'))
+            qs = qs.filter(**profile.to_kwargs(
+                prefix='_current_version__features__has_'))
 
-        return [self.to_native(item) for item in value.all()]
+        return [self.to_native(app) for app in qs]
 
     def field_to_native_es(self, obj, request):
         """
@@ -114,8 +115,7 @@ class CollectionSerializer(serializers.ModelSerializer):
     description = TranslationSerializerField()
     slug = serializers.CharField(required=False)
     collection_type = serializers.IntegerField()
-    apps = CollectionMembershipField(many=True,
-                                     source='collectionmembership_set')
+    apps = CollectionMembershipField(many=True, source='apps')
     image = HyperlinkedRelatedOrNullField(
         source='*',
         view_name='collection-image-detail',

mkt/collections/tests/test_models.py

@@ -64,6 +64,53 @@ class TestCollection(amo.tests.TestCase):
         eq_(list(CollectionMembership.objects.values_list('order', flat=True)),
             [0, 1, 2, 3])
 
+    def test_app_deleted(self):
+        collection = self.collection
+        app = amo.tests.app_factory()
+        collection.add_app(app)
+        self.assertSetEqual(collection.apps(), [app])
+        self.assertSetEqual(collection.collectionmembership_set.all(),
+            [CollectionMembership.objects.get(collection=collection, app=app)])
+
+        app.delete()
+
+        self.assertSetEqual(collection.apps(), [])
+        self.assertSetEqual(collection.collectionmembership_set.all(), [])
+
+    def test_app_disabled_by_user(self):
+        collection = self.collection
+        app = amo.tests.app_factory()
+        collection.add_app(app)
+        self.assertSetEqual(collection.apps(), [app])
+        self.assertSetEqual(collection.collectionmembership_set.all(),
+            [CollectionMembership.objects.get(collection=collection, app=app)])
+
+        app.update(disabled_by_user=True)
+
+        self.assertSetEqual(collection.apps(), [])
+
+        # The collection membership still exists here, the app is not deleted,
+        # only disabled.
+        self.assertSetEqual(collection.collectionmembership_set.all(),
+            [CollectionMembership.objects.get(collection=collection, app=app)])
+
+    def test_app_pending(self):
+        collection = self.collection
+        app = amo.tests.app_factory()
+        collection.add_app(app)
+        self.assertSetEqual(collection.apps(), [app])
+        self.assertSetEqual(collection.collectionmembership_set.all(),
+            [CollectionMembership.objects.get(collection=collection, app=app)])
+
+        app.update(status=amo.STATUS_PENDING)
+
+        self.assertSetEqual(collection.apps(), [])
+
+        # The collection membership still exists here, the app is not deleted,
+        # just not public.
+        self.assertSetEqual(collection.collectionmembership_set.all(),
+            [CollectionMembership.objects.get(collection=collection, app=app)])
+
     def test_mixed_ordering(self):
         self._generate_apps()
 

mkt/collections/tests/test_serializers.py

@@ -54,7 +54,7 @@ class BaseTestCollectionMembershipField(object):
         resource = AppSerializer(self.app)
         resource.context = {'request': request}
         self.field.context['request'] = request
-        native = self.field.to_native(self.membership)
+        native = self.field.to_native(self.collection.apps()[0])
         for key, value in native.iteritems():
             if key == 'resource_uri':
                 eq_(value, self.app.get_api_url(pk=self.app.pk))
@@ -64,11 +64,10 @@ class BaseTestCollectionMembershipField(object):
     def _field_to_native_profile(self, profile='0.0'):
         request = self.get_request({'pro': profile, 'dev': 'firefoxos'})
         self.field.parent = self.collection
-        self.field.source = 'collectionmembership_set'
+        self.field.source = 'apps'
         self.field.context['request'] = request
 
-        return self.field.field_to_native(self.collection,
-                                          'collectionmembership_set')
+        return self.field.field_to_native(self.collection, 'apps')
 
     def test_ordering(self):
         self.app2 = amo.tests.app_factory()
@@ -78,6 +77,21 @@ class BaseTestCollectionMembershipField(object):
         eq_(int(result[0]['id']), self.app2.id)
         eq_(int(result[1]['id']), self.app.id)
 
+    def test_app_delete(self):
+        self.app.delete()
+        result = self._field_to_native_profile()
+        eq_(len(result), 0)
+
+    def test_app_disable(self):
+        self.app.update(disabled_by_user=True)
+        result = self._field_to_native_profile()
+        eq_(len(result), 0)
+
+    def test_app_pending(self):
+        self.app.update(status=amo.STATUS_PENDING)
+        result = self._field_to_native_profile()
+        eq_(len(result), 0)
+
     def test_field_to_native_profile(self):
         result = self._field_to_native_profile(self.profile)
         eq_(len(result), 1)
@@ -135,6 +149,24 @@ class TestCollectionMembershipFieldES(BaseTestCollectionMembershipField,
         eq_(int(result[0]['id']), self.app2.id)
         eq_(int(result[1]['id']), self.app.id)
 
+    def test_app_delete(self):
+        self.app.delete()
+        self.refresh('webapp')
+        result = self._field_to_native_profile()
+        eq_(len(result), 0)
+
+    def test_app_disable(self):
+        self.app.update(disabled_by_user=True)
+        self.refresh('webapp')
+        result = self._field_to_native_profile()
+        eq_(len(result), 0)
+
+    def test_app_pending(self):
+        self.app.update(status=amo.STATUS_PENDING)
+        self.refresh('webapp')
+        result = self._field_to_native_profile()
+        eq_(len(result), 0)
+
 
 class TestCollectionSerializer(CollectionDataMixin, amo.tests.TestCase):
 

mkt/collections/views.py

@@ -193,18 +193,18 @@ class CollectionViewSet(CORSMixin, SlugOrIdMixin, viewsets.ModelViewSet):
         """
         Reorder the specified collection.
         """
-        collection = self.get_object()
         def membership(app):
             f = CollectionMembershipField()
             f.context = {'request': request}
             return f.to_native(app)
+
+        collection = self.get_object()
         try:
             collection.reorder(request.DATA)
         except ValueError:
             return Response({
                 'detail': self.exceptions['app_mismatch'],
-                'apps': [membership(a) for a in
-                         collection.collectionmembership_set.all()]
+                'apps': [membership(a) for a in collection.apps()]
             }, status=status.HTTP_400_BAD_REQUEST, exception=True)
         return self.return_updated(status.HTTP_200_OK)
 

mkt/webapps/models.py

@@ -1506,8 +1506,11 @@ class WebappIndexer(MappingType, Indexable):
         d['app_type'] = obj.app_type_id
         d['author'] = obj.developer_name
         d['category'] = list(obj.categories.values_list('slug', flat=True))
-        d['collection'] = [{'id': cms.collection_id, 'order': cms.order}
-                           for cms in obj.collectionmembership_set.all()]
+        if obj.is_public:
+            d['collection'] = [{'id': cms.collection_id, 'order': cms.order}
+                               for cms in obj.collectionmembership_set.all()]
+        else:
+            d['collection'] = []
         d['content_ratings'] = (obj.get_content_ratings_by_region(es=True) or
                                 None)
         d['content_descriptors'] = obj.get_descriptors(es=True)