diff --git a/normandy/classifier/models.py b/normandy/classifier/models.py index f812b95a8..85dac40c5 100644 --- a/normandy/classifier/models.py +++ b/normandy/classifier/models.py @@ -10,9 +10,12 @@ class Client(object): """A client attempting to fetch a set of recipes.""" - def __init__(self, request, locale=None): + def __init__(self, request, locale=None, release_channel=None, country=None): self.request = request self.locale = locale + self.release_channel = release_channel + if country is not None: + self.country = country @cached_property def country(self): diff --git a/normandy/classifier/tests/__init__.py b/normandy/classifier/tests/__init__.py index 27c8c94de..884d812a2 100644 --- a/normandy/classifier/tests/__init__.py +++ b/normandy/classifier/tests/__init__.py @@ -1,8 +1,17 @@ import factory -from normandy.classifier.models import Bundle +from django.test import RequestFactory + +from normandy.classifier.models import Bundle, Client class BundleFactory(factory.Factory): class Meta: model = Bundle + + +class ClientFactory(factory.Factory): + class Meta: + model = Client + + request = factory.LazyAttribute(lambda o: RequestFactory().get('/')) diff --git a/normandy/classifier/tests/test_api.py b/normandy/classifier/tests/test_api.py index 9238b1a9f..e0446df87 100644 --- a/normandy/classifier/tests/test_api.py +++ b/normandy/classifier/tests/test_api.py @@ -2,7 +2,7 @@ from rest_framework.reverse import reverse from normandy.base.tests import Whatever -from normandy.recipes.tests import RecipeActionFactory, RecipeFactory +from normandy.recipes.tests import RecipeActionFactory, RecipeFactory, LocaleFactory @pytest.mark.django_db @@ -38,13 +38,17 @@ def test_it_serves_recipes(self, client): }] def test_it_filters_by_locale(self, client): - RecipeFactory(name='english', enabled=True, locale='en-US') - RecipeFactory(name='german', enabled=True, locale='de') - RecipeFactory(name='any', enabled=True, locale='') - RecipeFactory(name='disabled', enabled=False, locale='de') + english = LocaleFactory(code='en-US') + german = LocaleFactory(code='de') + + RecipeFactory(name='english', enabled=True, locales=[english]) + RecipeFactory(name='german', enabled=True, locales=[german]) + RecipeFactory(name='any', enabled=True, locales=[]) + RecipeFactory(name='both', enabled=True, locales=[english, german]) + RecipeFactory(name='disabled', enabled=False, locales=[german]) res = client.post('/api/v1/fetch_bundle/', {'locale': 'de'}) assert res.status_code == 200 recipe_names = set(r['name'] for r in res.data['recipes']) - assert recipe_names == {'german', 'any'} + assert recipe_names == {'german', 'any', 'both'} diff --git a/normandy/recipes/admin.py b/normandy/recipes/admin.py index 1dd1a90a4..87db9a860 100644 --- a/normandy/recipes/admin.py +++ b/normandy/recipes/admin.py @@ -19,10 +19,16 @@ class RecipeActionInline(SortableTabularInline): @admin.register(models.Recipe) class RecipeAdmin(NonSortableParentAdmin): - list_display = ['name', 'enabled', 'locale', 'country', 'start_time', 'end_time'] - list_filter = ['enabled', 'locale', 'country'] - search_fields = ['name', 'locale', 'country'] + list_display = ['name', 'enabled', 'get_locales', 'get_countries', 'start_time', 'end_time'] + search_fields = ['name', 'locales', 'countries'] inlines = [RecipeActionInline] + filter_horizontal = ['locales', 'countries'] + + list_filter = [ + ('enabled', admin.BooleanFieldListFilter), + ('locales', admin.RelatedOnlyFieldListFilter), + ('countries', admin.RelatedOnlyFieldListFilter), + ] fieldsets = [ [None, { @@ -31,8 +37,9 @@ class RecipeAdmin(NonSortableParentAdmin): ['Delivery Rules', { 'fields': [ 'enabled', - 'locale', - 'country', + 'locales', + 'countries', + 'release_channels', 'sample_rate', 'start_time', 'end_time', @@ -40,6 +47,18 @@ class RecipeAdmin(NonSortableParentAdmin): }], ] + def get_locales(self, obj): + val = ', '.join(l.code for l in obj.locales.all()) + if not val: + val = self.get_empty_value_display() + return val + + def get_countries(self, obj): + val = ', '.join(l.name for l in obj.countries.all()) + if not val: + val = self.get_empty_value_display() + return val + @admin.register(models.Action) class ActionAdmin(admin.ModelAdmin): @@ -74,3 +93,8 @@ def in_use(self, action): """ return action.in_use in_use.boolean = True + + +@admin.register(models.ReleaseChannel) +class ReleaseChannelAdmin(admin.ModelAdmin): + fields = ['name', 'slug'] diff --git a/normandy/recipes/migrations/0016_auto_20160219_0101.py b/normandy/recipes/migrations/0016_auto_20160219_0101.py new file mode 100644 index 000000000..84f64fc63 --- /dev/null +++ b/normandy/recipes/migrations/0016_auto_20160219_0101.py @@ -0,0 +1,31 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.9 on 2016-02-19 01:01 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('recipes', '0015_auto_20160217_1819'), + ] + + operations = [ + migrations.CreateModel( + name='ReleaseChannel', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('slug', models.SlugField(max_length=255, unique=True)), + ('name', models.CharField(max_length=255)), + ], + options={ + 'ordering': ['slug'], + }, + ), + migrations.AddField( + model_name='recipe', + name='release_channels', + field=models.ManyToManyField(blank=True, to='recipes.ReleaseChannel'), + ), + ] diff --git a/normandy/recipes/migrations/0017_auto_20160218_2024.py b/normandy/recipes/migrations/0017_auto_20160218_2024.py new file mode 100644 index 000000000..3c8c1c173 --- /dev/null +++ b/normandy/recipes/migrations/0017_auto_20160218_2024.py @@ -0,0 +1,55 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.9 on 2016-02-18 20:24 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('recipes', '0016_auto_20160219_0101'), + ] + + operations = [ + migrations.CreateModel( + name='Country', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('code', models.CharField(max_length=255, unique=True)), + ('name', models.CharField(max_length=255)), + ('order', models.IntegerField()), + ], + options={ + 'ordering': ['order', 'name'], + }, + ), + migrations.AddField( + model_name='locale', + name='order', + field=models.IntegerField(default=100), + preserve_default=False, + ), + migrations.RemoveField( + model_name='recipe', + name='country', + ), + migrations.RemoveField( + model_name='recipe', + name='locale', + ), + migrations.AddField( + model_name='recipe', + name='locales', + field=models.ManyToManyField(blank=True, to='recipes.Locale'), + ), + migrations.AddField( + model_name='recipe', + name='countries', + field=models.ManyToManyField(blank=True, to='recipes.Country'), + ), + migrations.AlterModelOptions( + name='locale', + options={'ordering': ['order', 'code']}, + ), + ] diff --git a/normandy/recipes/migrations/0018_countries.py b/normandy/recipes/migrations/0018_countries.py new file mode 100644 index 000000000..3173bcfd4 --- /dev/null +++ b/normandy/recipes/migrations/0018_countries.py @@ -0,0 +1,52 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.9 on 2016-02-18 19:03 +from __future__ import unicode_literals + +from django.db import migrations + +from django_countries import countries + + +def add_countries(apps, schema_editor): + Country = apps.get_model('recipes', 'Country') + for (code, name) in countries: + if code == 'US': + order = 0 + else: + order = 100 + + Country.objects.update_or_create(code=code, defaults={ + 'name': name, + 'order': order, + }) + + +def remove_countries(apps, schema_editor): + Country = apps.get_model('recipes', 'Country') + Country.objects.all().delete() + + +def set_locale_sort_order(apps, schema_editor): + Locale = apps.get_model('recipes', 'Locale') + try: + english = Locale.objects.get(code='en-US') + english.order = 0 + english.save() + except Locale.DoesNotExist: + pass + + +def noop(apps, schema_editor): + pass + + +class Migration(migrations.Migration): + + dependencies = [ + ('recipes', '0017_auto_20160218_2024'), + ] + + operations = [ + migrations.RunPython(add_countries, remove_countries), + migrations.RunPython(set_locale_sort_order, noop), + ] diff --git a/normandy/recipes/models.py b/normandy/recipes/models.py index 82d6767f6..96a849494 100644 --- a/normandy/recipes/models.py +++ b/normandy/recipes/models.py @@ -5,7 +5,6 @@ from django.db import models from adminsortable.models import SortableMixin -from django_countries.fields import CountryField from rest_framework.reverse import reverse from normandy.recipes import utils @@ -21,13 +20,48 @@ class Locale(models.Model): code = models.CharField(max_length=255, unique=True) english_name = models.CharField(max_length=255, blank=True) native_name = models.CharField(max_length=255, blank=True) + order = models.IntegerField(default=100) class Meta: - ordering = ['code'] + ordering = ['order', 'code'] def __str__(self): return '{self.code} ({self.english_name})'.format(self=self) + def matches(self, other): + return self.code.lower() == other.lower() + + +class ReleaseChannel(models.Model): + """Release channel of Firefox""" + slug = models.SlugField(max_length=255, unique=True) + name = models.CharField(max_length=255) + + class Meta: + ordering = ['slug'] + + def __str__(self): + return self.name + + def matches(self, other): + return self.slug == other or self.name == other + + +class Country(models.Model): + """Database table for countries from django_countries.""" + code = models.CharField(max_length=255, unique=True) + name = models.CharField(max_length=255) + order = models.IntegerField(default=100) + + class Meta: + ordering = ['order', 'name'] + + def __str__(self): + return '{self.name} ({self.code})'.format(self=self) + + def matches(self, other): + return self.code == other or self.name == other + class Recipe(models.Model): """A set of actions to be fetched and executed by users.""" @@ -36,33 +70,41 @@ class Recipe(models.Model): # Fields that determine who this recipe is sent to. enabled = models.BooleanField(default=False) - locale = models.ForeignKey(Locale, blank=True, null=True) - country = CountryField(blank=True, null=True, default=None) + locales = models.ManyToManyField(Locale, blank=True) + countries = models.ManyToManyField(Country, blank=True) start_time = models.DateTimeField(blank=True, null=True, default=None) end_time = models.DateTimeField(blank=True, null=True, default=None) sample_rate = PercentField(default=100) + release_channels = models.ManyToManyField(ReleaseChannel, blank=True) def log_rejection(self, msg): logger.debug('{} rejected: {}'.format(self, msg)) + def check_many(self, name, field_qs, client_val): + field_vals = list(field_qs.all()) + + if field_vals == []: + return True + + for val in field_vals: + if val.matches(client_val): + return True + + field_vals_str = ', '.join(str(o) for o in field_vals) + self.log_rejection('client {name} ({client_val}) does not match recipe ' + '(choices are {field_vals})' + .format(name=name, client_val=client_val, field_vals=field_vals_str)) + return False + def matches(self, client): """ Return whether this Recipe should be sent to the given client. """ + # This should be ordered roughly by performance cost if not self.enabled: self.log_rejection('not enabled') return False - if self.locale and client.locale and self.locale.code.lower() != client.locale.lower(): - self.log_rejection('recipe locale ({self.locale!r}) != ' - 'client locale ({client.locale!r})') - return False - - if self.country and self.country != client.country: - self.log_rejection('recipe country ({self.country!r}) != ' - 'client country ({client.country!r})') - return False - if self.start_time and self.start_time > client.request_time: self.log_rejection('start time not met ({})'.format(self.start_time)) return False @@ -77,6 +119,15 @@ def matches(self, client): self.log_rejection('did not match sample') return False + if not self.check_many('locale', self.locales, client.locale): + return False + + if not self.check_many('country', self.countries, client.country): + return False + + if not self.check_many('channel', self.release_channels, client.release_channel): + return False + return True def __repr__(self): diff --git a/normandy/recipes/storage.py b/normandy/recipes/storage.py index 15c51cea3..fe5652808 100644 --- a/normandy/recipes/storage.py +++ b/normandy/recipes/storage.py @@ -21,9 +21,15 @@ def update(self, name, content, last_modified): if name == 'languages.json': languages = json.loads(content) for locale_code, names in languages.items(): + if locale_code == 'en-US': + order = 0 + else: + order = 100 + Locale.objects.update_or_create(code=locale_code, defaults={ 'english_name': names['English'], 'native_name': names['native'], + 'order': order, }) # Remove obsolete locales. diff --git a/normandy/recipes/tests/__init__.py b/normandy/recipes/tests/__init__.py index c610bc365..1adc42be4 100644 --- a/normandy/recipes/tests/__init__.py +++ b/normandy/recipes/tests/__init__.py @@ -1,7 +1,9 @@ import factory +from django.template.defaultfilters import slugify + from normandy.base.tests import FuzzyUnicode -from normandy.recipes.models import Action, Locale, Recipe, RecipeAction +from normandy.recipes.models import Action, Country, Locale, Recipe, RecipeAction, ReleaseChannel class RecipeFactory(factory.DjangoModelFactory): @@ -12,12 +14,31 @@ class Meta: enabled = True @factory.post_generation - def locale(self, create, extracted, **kwargs): + def countries(self, create, extracted, **kwargs): + if not create: + return + + if extracted: + for country in extracted: + self.countries.add(country) + + @factory.post_generation + def locales(self, create, extracted, **kwargs): + if not create: + return + + if extracted: + for locale in extracted: + self.locales.add(locale) + + @factory.post_generation + def release_channels(self, create, extracted, **kwargs): if not create: return - if extracted and isinstance(extracted, str): - self.locale, _ = Locale.objects.get_or_create(code=extracted) + if extracted: + for channel in extracted: + self.release_channels.add(channel) class ActionFactory(factory.DjangoModelFactory): @@ -36,6 +57,23 @@ class Meta: recipe = factory.SubFactory(RecipeFactory) +class CountryFactory(factory.DjangoModelFactory): + class Meta: + model = Country + + code = factory.fuzzy.FuzzyText(length=3) + + class LocaleFactory(factory.DjangoModelFactory): class Meta: model = Locale + + code = factory.fuzzy.FuzzyText(length=2) + + +class ReleaseChannelFactory(factory.DjangoModelFactory): + class Meta: + model = ReleaseChannel + + name = FuzzyUnicode() + slug = factory.LazyAttribute(lambda o: slugify(o.name)) diff --git a/normandy/recipes/tests/test_models.py b/normandy/recipes/tests/test_models.py index 0cb222db6..cab466b05 100644 --- a/normandy/recipes/tests/test_models.py +++ b/normandy/recipes/tests/test_models.py @@ -1,6 +1,9 @@ import pytest -from normandy.recipes.tests import ActionFactory, RecipeActionFactory +from normandy.recipes.tests import ( + ActionFactory, CountryFactory, LocaleFactory, RecipeActionFactory, RecipeFactory, + ReleaseChannelFactory) +from normandy.classifier.tests import ClientFactory @pytest.mark.django_db @@ -29,3 +32,90 @@ def test_in_use(self): RecipeActionFactory(action=action, recipe__enabled=True) assert action.in_use + + +@pytest.mark.django_db +class TestRecipe(object): + def test_filter_by_channel_empty(self): + recipe = RecipeFactory(release_channels=[]) + client = ClientFactory(release_channel='release') + assert recipe.matches(client) + + def test_filter_by_channel_one(self): + beta = ReleaseChannelFactory(slug='beta') + recipe = RecipeFactory(release_channels=[beta]) + + release_client = ClientFactory(release_channel='release') + beta_client = ClientFactory(release_channel='beta') + + assert not recipe.matches(release_client) + assert recipe.matches(beta_client) + + def test_filter_by_channel_many(self): + release = ReleaseChannelFactory(slug='release') + beta = ReleaseChannelFactory(slug='beta') + recipe = RecipeFactory(release_channels=[release, beta]) + + release_client = ClientFactory(release_channel='release') + beta_client = ClientFactory(release_channel='beta') + aurora_client = ClientFactory(release_channel='aurora') + + assert recipe.matches(release_client) + assert recipe.matches(beta_client) + assert not recipe.matches(aurora_client) + + def test_filter_by_locale_none(self): + recipe = RecipeFactory(locales=[]) + client = ClientFactory(locale='en-US') + assert recipe.matches(client) + + def test_filter_by_locale_one(self): + locale1 = LocaleFactory() + locale2 = LocaleFactory() + recipe = RecipeFactory(locales=[locale1]) + client1 = ClientFactory(locale=locale1.code) + client2 = ClientFactory(locale=locale2.code) + + assert recipe.matches(client1) + assert not recipe.matches(client2) + + def test_filter_by_locale_many(self): + locale1 = LocaleFactory() + locale2 = LocaleFactory() + locale3 = LocaleFactory() + recipe = RecipeFactory(locales=[locale1, locale2]) + client1 = ClientFactory(locale=locale1.code) + client2 = ClientFactory(locale=locale2.code) + client3 = ClientFactory(locale=locale3.code) + + assert recipe.matches(client1) + assert recipe.matches(client2) + assert not recipe.matches(client3) + + def test_filter_by_country_none(self): + recipe = RecipeFactory(countries=[]) + client = ClientFactory(country='US') + assert recipe.matches(client) + + def test_filter_by_country_one(self): + country1 = LocaleFactory() + country2 = LocaleFactory() + recipe = RecipeFactory(locales=[country1]) + client1 = ClientFactory(locale=country1.code) + client2 = ClientFactory(locale=country2.code) + + assert recipe.matches(client1) + assert not recipe.matches(client2) + + def test_filter_by_country_many(self): + country1 = CountryFactory() + country2 = CountryFactory() + country3 = CountryFactory() + recipe = RecipeFactory(countries=[country1, country2]) + client1 = ClientFactory(country=country1.code) + client2 = ClientFactory(country=country2.code) + client3 = ClientFactory(country=country3.code) + + assert recipe.matches(client1) + assert recipe.matches(client2) + assert not recipe.matches(client3)