From 0bbea9f3c8702816204738756448e5ff985de428 Mon Sep 17 00:00:00 2001 From: Mike Cooper Date: Thu, 18 Feb 2016 13:21:19 -0800 Subject: [PATCH] Make Recipe locale and country selection many-to-many Fixes bug 1248263. --- normandy/classifier/models.py | 13 +++-- normandy/classifier/tests/test_api.py | 16 ++++-- normandy/recipes/admin.py | 28 +++++++-- normandy/recipes/models.py | 81 +++++++++++++++++---------- normandy/recipes/storage.py | 6 ++ normandy/recipes/tests/__init__.py | 27 +++++++-- normandy/recipes/tests/test_models.py | 59 ++++++++++++++++++- 7 files changed, 178 insertions(+), 52 deletions(-) diff --git a/normandy/classifier/models.py b/normandy/classifier/models.py index 3de17da42..a492db2a5 100644 --- a/normandy/classifier/models.py +++ b/normandy/classifier/models.py @@ -2,23 +2,24 @@ import uuid -from django.utils.functional import cached_property - from normandy.classifier.geolocation import get_country_code from normandy.recipes.models import Recipe class Client(object): """A client attempting to fetch a set of recipes.""" - def __init__(self, request, locale=None, release_channel=None): + def __init__(self, request, locale=None, release_channel=None, country=None): self.request = request self.locale = locale self.release_channel = release_channel + self._country = country - @cached_property + @property def country(self): - ip_address = self.request.META.get('REMOTE_ADDR') - return get_country_code(ip_address) + if not self._country: + ip_address = self.request.META.get('REMOTE_ADDR') + self._country = get_country_code(ip_address) + return self._country @property def request_time(self): 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 bdb056565..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,8 @@ class RecipeAdmin(NonSortableParentAdmin): ['Delivery Rules', { 'fields': [ 'enabled', - 'locale', - 'country', + 'locales', + 'countries', 'release_channels', 'sample_rate', 'start_time', @@ -41,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): diff --git a/normandy/recipes/models.py b/normandy/recipes/models.py index 1d6f767a5..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,17 @@ 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""" @@ -40,6 +43,25 @@ class Meta: 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.""" @@ -48,8 +70,8 @@ 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) @@ -58,26 +80,31 @@ class Recipe(models.Model): 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})' - .format(self=self, client=client)) - return False - - if self.country and self.country != client.country: - self.log_rejection('recipe country ({self.country!r}) != ' - 'client country ({client.country!r})' - .format(self=self, client=client)) - 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 @@ -92,20 +119,14 @@ def matches(self, client): self.log_rejection('did not match sample') return False - recipe_channels = list(self.release_channels.all()) - if recipe_channels: - match = False - for channel in recipe_channels: - if channel.slug == client.release_channel: - match = True - break - if not match: - channels = ', '.join(c.slug for c in self.release_channels.all()) - self.log_rejection( - 'client channel ({client.release_channel}) not in ' - 'recipe channels ({channels})' - .format(channels=channels, client=client)) - 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 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 297cd5e28..1adc42be4 100644 --- a/normandy/recipes/tests/__init__.py +++ b/normandy/recipes/tests/__init__.py @@ -3,7 +3,7 @@ from django.template.defaultfilters import slugify from normandy.base.tests import FuzzyUnicode -from normandy.recipes.models import Action, Locale, Recipe, RecipeAction, ReleaseChannel +from normandy.recipes.models import Action, Country, Locale, Recipe, RecipeAction, ReleaseChannel class RecipeFactory(factory.DjangoModelFactory): @@ -14,12 +14,22 @@ 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 and isinstance(extracted, str): - self.locale, _ = Locale.objects.get_or_create(code=extracted) + 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): @@ -47,10 +57,19 @@ 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: diff --git a/normandy/recipes/tests/test_models.py b/normandy/recipes/tests/test_models.py index 556aba67c..cab466b05 100644 --- a/normandy/recipes/tests/test_models.py +++ b/normandy/recipes/tests/test_models.py @@ -1,7 +1,8 @@ import pytest from normandy.recipes.tests import ( - ActionFactory, RecipeActionFactory, RecipeFactory, ReleaseChannelFactory) + ActionFactory, CountryFactory, LocaleFactory, RecipeActionFactory, RecipeFactory, + ReleaseChannelFactory) from normandy.classifier.tests import ClientFactory @@ -62,3 +63,59 @@ def test_filter_by_channel_many(self): 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)