diff --git a/apps/addons/models.py b/apps/addons/models.py index 1037e435e8..5965b9d309 100644 --- a/apps/addons/models.py +++ b/apps/addons/models.py @@ -794,7 +794,7 @@ class Addon(amo.models.OnChangeMixin, amo.models.ModelBase): def is_category_featured(self, app, lang): """Is add-on featured in any category for this app?""" - return app.id in self._creatured_apps + return (app.id, lang) in self._creatured_apps def has_full_profile(self): """Is developer profile public (completed)?""" @@ -1145,19 +1145,24 @@ class AddonCategory(caching.CachingMixin, models.Model): @classmethod def creatured_ids(cls): - """ Returns a list of creatured ids """ - qs = cls.objects.filter(feature=True) - f = lambda: list(qs.values_list('addon', 'category', - 'category__application', - 'feature_locales')) - return caching.cached_with(qs, f, 'creatured') + """Returns a list of creatured ids.""" + if settings.NEW_FEATURES: + from bandwagon.models import FeaturedCollection + return FeaturedCollection.objects.creatured_ids() + else: + # TODO(cvan): Remove this once new features are enabled. + qs = cls.objects.filter(feature=True) + f = lambda: list(qs.values_list('addon', 'category', + 'category__application', + 'feature_locales')) + return caching.cached_with(qs, f, 'creatured') @classmethod def creatured(cls): """Get a dict of {addon: [app,..]} for all creatured add-ons.""" rv = {} for addon, cat, catapp, locale in cls.creatured_ids(): - rv.setdefault(addon, []).append(catapp) + rv.setdefault(addon, []).append((catapp, locale)) return rv @classmethod diff --git a/apps/addons/tests/test_models.py b/apps/addons/tests/test_models.py index 47a959bc47..71fac2cd3f 100644 --- a/apps/addons/tests/test_models.py +++ b/apps/addons/tests/test_models.py @@ -24,6 +24,7 @@ from addons.models import (Addon, AddonCategory, AddonDependency, Category, Charity, Feature, FrozenAddon, Persona, Preview) from applications.models import Application, AppVersion +from bandwagon.models import CollectionAddon, FeaturedCollection from devhub.models import ActivityLog from files.models import File, Platform from files.tests.test_models import UploadTest @@ -42,13 +43,13 @@ class TestAddonManager(test_utils.TestCase): def setUp(self): set_user(None) - @patch.object(settings._wrapped, 'NEW_FEATURES', False) + @patch.object(settings, 'NEW_FEATURES', False) def test_featured(self): featured = Addon.objects.featured(amo.FIREFOX)[0] eq_(featured.id, 1) eq_(Addon.objects.featured(amo.FIREFOX).count(), 5) - @patch.object(settings._wrapped, 'NEW_FEATURES', True) + @patch.object(settings, 'NEW_FEATURES', True) def test_featured(self): featured = Addon.objects.featured(amo.FIREFOX)[0] eq_(featured.id, 1001) @@ -122,7 +123,7 @@ class TestAddonManagerFeatured(test_utils.TestCase): fixtures = ['addons/featured', 'bandwagon/featured_collections', 'base/collections', 'base/featured'] - @patch.object(settings._wrapped, 'NEW_FEATURES', True) + @patch.object(settings, 'NEW_FEATURES', True) def test_new_featured(self): f = Addon.objects.featured(amo.FIREFOX) eq_(f.count(), 6) @@ -134,6 +135,7 @@ class TestAddonManagerFeatured(test_utils.TestCase): class TestAddonModels(test_utils.TestCase): fixtures = ['base/apps', + 'base/collections', 'base/featured', 'base/users', 'base/addon_5299_gcal', @@ -145,7 +147,8 @@ class TestAddonModels(test_utils.TestCase): 'base/thunderbird', 'addons/featured', 'addons/invalid_latest_version', - 'addons/blacklisted'] + 'addons/blacklisted', + 'bandwagon/featured_collections'] def setUp(self): TranslationSequence.objects.create(id=99243) @@ -205,7 +208,7 @@ class TestAddonModels(test_utils.TestCase): a = Addon.objects.get(pk=5299) eq_(a.current_beta_version.id, 50000) - @patch.object(settings._wrapped, 'NEW_FEATURES', False) + @patch.object(settings, 'NEW_FEATURES', False) def test_current_version_mixed_statuses(self): """Mixed file statuses are evil (bug 558237).""" a = Addon.objects.get(pk=3895) @@ -355,21 +358,43 @@ class TestAddonModels(test_utils.TestCase): a.status = amo.STATUS_LISTED assert a.is_selfhosted(), 'listed add-on => is_selfhosted()' - @patch.object(settings._wrapped, 'NEW_FEATURES', False) def test_is_featured(self): """Test if an add-on is globally featured""" a = Addon.objects.get(pk=1003) assert a.is_featured(amo.FIREFOX, 'en-US'), ( 'globally featured add-on not recognized') - @patch.object(settings._wrapped, 'NEW_FEATURES', False) + @patch.object(settings, 'NEW_FEATURES', False) def test_is_category_featured(self): - """Test if an add-on is category featured""" + """Test if an add-on is category featured.""" Feature.objects.filter(addon=1001).delete() a = Addon.objects.get(pk=1001) - assert not a.is_featured(amo.FIREFOX, 'en-US') + assert not a.is_featured(amo.FIREFOX, 'en-US'), ( + "Expected add-on should not be in 'en-US' locale") - assert a.is_category_featured(amo.FIREFOX, 'en-US') + assert a.is_category_featured(amo.FIREFOX, None), ( + 'Expected add-on to have no locale') + assert not a.is_category_featured(amo.FIREFOX, 'fr'), ( + "Expected add-on to not be in 'fr' locale") + + @patch.object(settings, 'NEW_FEATURES', True) + def test_new_is_category_featured(self): + """Test if an add-on is category featured.""" + a = Addon.objects.get(pk=1001) + assert a.is_category_featured(amo.FIREFOX, None), ( + 'Expected add-on to have no locale') + assert not a.is_category_featured(amo.FIREFOX, 'fr'), ( + "Expected add-on to not be in 'fr' locale") + + fc = FeaturedCollection.objects.filter(collection__addons=1001)[0] + c = CollectionAddon.objects.filter(addon=a, + collection=fc.collection)[0] + c.delete() + assert not a.is_featured(amo.FIREFOX, 'en-US'), ( + "Expected add-on to be in 'en-US' locale") + + assert a.is_category_featured(amo.FIREFOX, None), ( + 'Expected add-on to have no locale') def test_has_full_profile(self): """Test if an add-on's developer profile is complete (public).""" @@ -1093,11 +1118,11 @@ class TestAddonModelsFeatured(test_utils.TestCase): f = Addon.featured_random(amo.SUNBIRD, 'en-US') eq_(f, []) - @patch.object(settings._wrapped, 'NEW_FEATURES', False) + @patch.object(settings, 'NEW_FEATURES', False) def test_featured_random(self): self._test_featured_random() - @patch.object(settings._wrapped, 'NEW_FEATURES', True) + @patch.object(settings, 'NEW_FEATURES', True) def test_new_featured_random(self): self._test_featured_random() @@ -1359,13 +1384,13 @@ REDIRECT_URL = 'http://outgoing.mozilla.org/v1/' class TestCharity(test_utils.TestCase): fixtures = ['base/charity.json'] - @patch.object(settings._wrapped, 'REDIRECT_URL', REDIRECT_URL) + @patch.object(settings, 'REDIRECT_URL', REDIRECT_URL) def test_url(self): charity = Charity(name="a", paypal="b", url="http://foo.com") charity.save() assert charity.outgoing_url.startswith(REDIRECT_URL) - @patch.object(settings._wrapped, 'REDIRECT_URL', REDIRECT_URL) + @patch.object(settings, 'REDIRECT_URL', REDIRECT_URL) def test_url_foundation(self): foundation = Charity.objects.get(pk=amo.FOUNDATION_ORG) assert not foundation.outgoing_url.startswith(REDIRECT_URL) diff --git a/apps/amo/fixtures/base/addon_3615_featuredcollection.json b/apps/amo/fixtures/base/addon_3615_featuredcollection.json new file mode 100644 index 0000000000..c6f22ab4f3 --- /dev/null +++ b/apps/amo/fixtures/base/addon_3615_featuredcollection.json @@ -0,0 +1,54 @@ +[ + { + "pk": 4, + "model": "bandwagon.featuredcollection", + "fields": { + "locale": "en-US", + "application": 1, + "modified": "2011-06-15 13:13:59", + "collection": 56448, + "created": "2011-06-15 13:13:59" + } + }, + { + "pk": 1004521, + "model": "bandwagon.collectionaddon", + "fields": { + "user": null, + "created": "2010-09-13 00:00:20", + "ordering": 0, + "downloads": 0, + "modified": "2010-09-13 00:00:20", + "comments": null, + "collection": 56448, + "addon": 3615 + } + }, + { + "pk": 56448, + "model": "bandwagon.collection", + "fields": { + "rating": 0.0, + "downvotes": 0, + "uuid": "78c35f98-a376-b2bf-cd66-3512b5078511", + "author": 55021, + "application": 1, + "listed": true, + "upvotes": 0, + "icontype": "", + "type": 0, + "default_locale": "en-US", + "monthly_subscribers": 0, + "downloads": 0, + "addon_index": null, + "subscribers": 0, + "nickname": null, + "slug": "78c35f98-a376-b2bf-cd66-3512b5", + "created": "2010-02-12 13:52:05", + "weekly_subscribers": 0, + "modified": "2010-09-13 00:39:36", + "addon_count": 1, + "all_personas": false + } + } +] diff --git a/apps/api/tests/test_legacy.py b/apps/api/tests/test_legacy.py index 9f16b8f6e7..6de49e5267 100644 --- a/apps/api/tests/test_legacy.py +++ b/apps/api/tests/test_legacy.py @@ -20,6 +20,8 @@ from addons.models import (Addon, AddonCategory, AppSupport, Category, Feature, Preview) from amo import helpers from amo.urlresolvers import reverse +from applications.models import Application +from bandwagon.models import Collection, CollectionAddon, FeaturedCollection from search.tests import SphinxTestCase from search.utils import stop_sphinx @@ -390,6 +392,7 @@ class APITest(TestCase): '%s' % Addon.objects.get(pk=5299).slug) + @patch.object(settings._wrapped, 'NEW_FEATURES', False) def test_is_featured(self): self.assertContains(make_call('addon/5299', version=1.5), '0') @@ -402,11 +405,29 @@ class APITest(TestCase): ('en-US', 'firefox', 0), ('ja', 'seamonkey', 0)]: # Clean out the special cache for feature. - delattr(Addon, '_feature') + if hasattr(Addon, '_feature'): + del Addon._feature self.assertContains(make_call('addon/5299', version=1.5, lang=lang, app=app), '%s' % result) + @patch.object(settings._wrapped, 'NEW_FEATURES', True) + def test_new_is_featured(self): + self.assertContains(make_call('addon/5299', version=1.5), + '0') + c = CollectionAddon.objects.create(addon=Addon.objects.get(id=5299), + collection=Collection.objects.create()) + FeaturedCollection.objects.create(locale='ja', + application=Application.objects.get(id=amo.FIREFOX.id), + collection=c.collection) + for lang, app, result in [('ja', 'firefox', 1), + ('en-US', 'firefox', 0), + ('ja', 'seamonkey', 0)]: + self.assertContains(make_call('addon/5299', version=1.5, + lang=lang, app=app), + '%s' % result) + + @patch.object(settings._wrapped, 'NEW_FEATURES', False) def test_is_category_featured(self): self.assertContains(make_call('addon/5299', version=1.5), '0') @@ -424,12 +445,29 @@ class APITest(TestCase): lang=lang, app=app), '%s' % result) + @patch.object(settings._wrapped, 'NEW_FEATURES', True) + def test_new_is_category_featured(self): + self.assertContains(make_call('addon/5299', version=1.5), + '0') + AddonCategory.objects.create(addon_id=5299, + category=Category.objects.all()[0]) + c = CollectionAddon.objects.create(addon=Addon.objects.get(id=5299), + collection=Collection.objects.create()) + FeaturedCollection.objects.create(locale='ja', + application=Application.objects.get(id=amo.FIREFOX.id), + collection=c.collection) + for lang, app, result in [('ja', 'firefox', 1), + ('en-US', 'firefox', 0), + ('ja', 'seamonkey', 0)]: + self.assertContains(make_call('addon/5299', version=1.5, + lang=lang, app=app), + '%s' % result) + def test_default_icon(self): addon = Addon.objects.get(pk=5299) addon.update(icon_type='') self.assertContains(make_call('addon/5299'), '') - def test_thumbnail_size(self): addon = Addon.objects.get(pk=5299) preview = Preview.objects.create(addon=addon) diff --git a/apps/bandwagon/models.py b/apps/bandwagon/models.py index 22ac72bebc..48045c8f62 100644 --- a/apps/bandwagon/models.py +++ b/apps/bandwagon/models.py @@ -549,7 +549,7 @@ class RecommendedCollection(Collection): class FeaturedCollectionManager(amo.models.ManagerBase): - def featured(self, app, lang): + def featured(self, app=None, lang=None): qs = self if app: qs = qs.filter(application__id=app.id) @@ -571,6 +571,19 @@ class FeaturedCollectionManager(amo.models.ManagerBase): """Returns add-ons from filtered collections.""" return Addon.objects.filter(id__in=self.addon_ids(app, lang)) + def creatured_ids(self, category=None, lang=None): + qs = self + if category: + qs = qs.filter(collection__addons__category=category) + else: + qs = qs.filter(collection__addons__category__isnull=False) + if lang: + qs = qs.filter(Q(locale=lang) | Q(locale__isnull=True)) + return list(qs.values_list('collection__addons', + 'collection__addons__category', + 'collection__addons__category__application', + 'locale').distinct()) + class FeaturedCollection(amo.models.ModelBase): application = models.ForeignKey(Application) diff --git a/apps/bandwagon/tests/test_models.py b/apps/bandwagon/tests/test_models.py index f81d5955a5..5dcd2a6a3f 100644 --- a/apps/bandwagon/tests/test_models.py +++ b/apps/bandwagon/tests/test_models.py @@ -6,10 +6,10 @@ import test_utils from nose.tools import eq_ import amo -from addons.models import Addon, AddonRecommendation -from bandwagon.models import (Collection, CollectionUser, CollectionWatcher, - SyncedCollection, RecommendedCollection, - FeaturedCollection) +from addons.models import Addon, AddonCategory, AddonRecommendation, Category +from bandwagon.models import (Collection, CollectionAddon, CollectionUser, + CollectionWatcher, SyncedCollection, + RecommendedCollection, FeaturedCollection) from devhub.models import ActivityLog from bandwagon import tasks from users.models import UserProfile @@ -194,6 +194,8 @@ class TestFeaturedCollectionManager(test_utils.TestCase): .addon_ids(**kw))) self.ids = [1001, 1003, 2464, 3481, 7661, 15679] self.default_ids = [1001, 1003, 2464, 7661, 15679] + self.c = (lambda **kw: sorted(FeaturedCollection.objects + .creatured_ids(**kw))) def test_addon_ids_apps(self): eq_(self.f(), self.ids) @@ -230,3 +232,46 @@ class TestFeaturedCollectionManager(test_utils.TestCase): eq_(ids(app=amo.FIREFOX), self.ids) eq_(ids(app=amo.FIREFOX, lang='en-US'), self.ids) eq_(ids(app=amo.FIREFOX, lang='fr'), self.default_ids) + + def test_creatured_ids(self): + cat = Addon.objects.get(id=1001).categories.all()[0] + expected = [(1001, cat.id, amo.FIREFOX.id, None)] + eq_(self.c(), expected) + eq_(self.c(category=999), []) + eq_(self.c(category=cat.id, lang=None), expected) + + # This should contain creatured add-ons from the default locale. + eq_(self.c(category=cat.id, lang='fr'), expected) + + def test_creatured_ids_new_addon_category(self): + """Creatured add-ons should contain those add-ons in a category.""" + cat = Category.objects.all()[0] + AddonCategory.objects.create(addon_id=1003, category=cat) + eq_(self.c(), [(1001, cat.id, amo.FIREFOX.id, None), + (1003, cat.id, amo.FIREFOX.id, None)]) + + def test_creatured_ids_remove_addon_category(self): + """Creatured add-ons should disappear if no longer in a category.""" + AddonCategory.objects.filter(addon__id=1001)[0].delete() + eq_(self.c(), []) + + def test_creatured_ids_new_locale_category(self): + """Creatured add-ons should change if we change featured locale.""" + c = CollectionAddon.objects.create(addon_id=1003, + collection=Collection.objects.create()) + FeaturedCollection.objects.create(locale='ja', + application_id=amo.FIREFOX.id, + collection=c.collection) + cat = Category.objects.create(pk=12, slug='burr', + type=amo.ADDON_EXTENSION, + application_id=amo.FIREFOX.id) + AddonCategory.objects.create(addon_id=1003, category=new_cat) + + # The 1003 is already featured for the default locale, so adding a + # category for this add-on will give us two creatures. + ja_creature = (1003, cat.id, amo.FIREFOX.id, 'ja') + eq_(self.c(), [(1001, 22, amo.FIREFOX.id, None), + (1003, cat.id, amo.FIREFOX.id, None), + ja_creature]) + eq_(self.c(lang='ja'), [ja_creature]) + eq_(self.c(category=new_cat.id, lang='ja'), [ja_creature]) diff --git a/apps/browse/tests.py b/apps/browse/tests.py index 55a81e9604..9d037c04ce 100644 --- a/apps/browse/tests.py +++ b/apps/browse/tests.py @@ -230,8 +230,15 @@ class TestThemes(test_utils.TestCase): class TestCategoryPages(test_utils.TestCase): - fixtures = ('base/apps', 'base/category', 'base/addon_3615', - 'base/featured', 'addons/featured', 'browse/nameless-addon') + fixtures = ['base/apps', 'base/category', 'base/addon_3615', + 'base/featured', 'addons/featured', 'browse/nameless-addon'] + + def setUp(self): + self._new_features = settings.NEW_FEATURES + settings.NEW_FEATURES = False + + def tearDown(self): + settings.NEW_FEATURES = self._new_features def test_browsing_urls(self): """Every browse page URL exists.""" @@ -317,6 +324,18 @@ class TestCategoryPages(test_utils.TestCase): assert 57132 not in [a.id for a in ids] +class NewTestCategoryPages(TestCategoryPages): + fixtures = (TestCategoryPages.fixtures + + ['base/addon_3615_featuredcollection']) + + def setUp(self): + self._new_features = settings.NEW_FEATURES + settings.NEW_FEATURES = True + + def tearDown(self): + settings.NEW_FEATURES = self._new_features + + class TestFeaturedLocale(test_utils.TestCase): fixtures = ['base/apps', 'base/category', 'base/addon_3615', 'base/featured', 'addons/featured', 'browse/nameless-addon'] @@ -560,7 +579,10 @@ class TestFeaturedLocale(test_utils.TestCase): class TestNewFeaturedLocale(TestFeaturedLocale): - fixtures = TestFeaturedLocale.fixtures + ['bandwagon/featured_collections'] + fixtures = (TestFeaturedLocale.fixtures + + ['base/collections', 'addons/featured', 'base/featured', + 'bandwagon/featured_collections', + 'base/addon_3615_featuredcollection']) # TODO(cvan): Merge with above once new featured add-ons are enabled. def setUp(self): @@ -574,6 +596,9 @@ class TestNewFeaturedLocale(TestFeaturedLocale): def test_featured_random_caching(self): raise SkipTest() # We're no longer caching `featured_random`. + def test_creatured_random_caching(self): + raise SkipTest() # We're no longer caching `creatured_random`. + def change_addon(self, addon, locale='es-ES'): fc = FeaturedCollection.objects.filter(collection__addons=addon.id)[0] feature = FeaturedCollection.objects.create(locale=locale, @@ -584,6 +609,33 @@ class TestNewFeaturedLocale(TestFeaturedLocale): c.collection = feature.collection c.save() + def change_addoncategory(self, addon, locale='es-ES'): + CollectionAddon.objects.filter(addon=addon).delete() + locales = (locale or '').split(',') + for locale in locales: + c = CollectionAddon.objects.create(addon=addon, + collection=Collection.objects.create()) + FeaturedCollection.objects.create(locale=locale, + application=Application.objects.get(id=amo.FIREFOX.id), + collection=c.collection) + + def test_featured_ids(self): + # TODO(cvan): Change the TestFeaturedLocale test + # accordingly after we switch over to the new features. + FeaturedCollection.objects.filter(collection__addons=3615)[0].delete() + super(TestNewFeaturedLocale, self).test_featured_ids() + + def test_homepage_order(self): + # TODO(cvan): Change the TestFeaturedLocale test + # accordingly after we switch over to the new features. + FeaturedCollection.objects.filter(collection__addons=3615)[0].delete() + super(TestNewFeaturedLocale, self).test_featured_ids() + + def test_creatured_locale_es_ES(self): + """Ensure 'en-US'-creatured add-ons do not exist for other locales.""" + res = self.client.get(self.url.replace('en-US', 'es-ES')) + assert self.addon not in res.context['addons'] + class TestListingByStatus(test_utils.TestCase): fixtures = ['base/apps', 'base/addon_3615'] diff --git a/settings.py b/settings.py index dcaa64d365..59ffd843ee 100644 --- a/settings.py +++ b/settings.py @@ -936,6 +936,8 @@ EXPOSE_VALIDATOR_TRACEBACKS = False # Feature flags SEARCH_EXCLUDE_PERSONAS = True UNLINK_SITE_STATS = True + +# Use the new featured add-ons system which makes use of featured collections. NEW_FEATURES = False # Set to True if we're allowed to use X-SENDFILE.