From 1d4646c1f463128a626f8615d13f1a7a383d9f6a Mon Sep 17 00:00:00 2001 From: Mike Cooper Date: Fri, 19 Feb 2016 10:10:18 -0800 Subject: [PATCH 1/3] Filter by release channel Fixes bug 1248265. --- normandy/classifier/models.py | 3 +- normandy/classifier/tests/__init__.py | 11 +++++- normandy/recipes/admin.py | 6 ++++ .../migrations/0016_auto_20160219_0101.py | 31 ++++++++++++++++ normandy/recipes/models.py | 34 ++++++++++++++++-- normandy/recipes/tests/__init__.py | 21 ++++++++++- normandy/recipes/tests/test_models.py | 35 ++++++++++++++++++- 7 files changed, 135 insertions(+), 6 deletions(-) create mode 100644 normandy/recipes/migrations/0016_auto_20160219_0101.py diff --git a/normandy/classifier/models.py b/normandy/classifier/models.py index f812b95a8..3de17da42 100644 --- a/normandy/classifier/models.py +++ b/normandy/classifier/models.py @@ -10,9 +10,10 @@ 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): self.request = request self.locale = locale + self.release_channel = release_channel @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/recipes/admin.py b/normandy/recipes/admin.py index 1dd1a90a4..bdb056565 100644 --- a/normandy/recipes/admin.py +++ b/normandy/recipes/admin.py @@ -33,6 +33,7 @@ class RecipeAdmin(NonSortableParentAdmin): 'enabled', 'locale', 'country', + 'release_channels', 'sample_rate', 'start_time', 'end_time', @@ -74,3 +75,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/models.py b/normandy/recipes/models.py index 82d6767f6..1d6f767a5 100644 --- a/normandy/recipes/models.py +++ b/normandy/recipes/models.py @@ -29,6 +29,18 @@ def __str__(self): return '{self.code} ({self.english_name})'.format(self=self) +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 + + class Recipe(models.Model): """A set of actions to be fetched and executed by users.""" name = models.CharField(max_length=255, unique=True) @@ -41,6 +53,7 @@ class Recipe(models.Model): 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)) @@ -55,12 +68,14 @@ def matches(self, client): 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})') + '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})') + 'client country ({client.country!r})' + .format(self=self, client=client)) return False if self.start_time and self.start_time > client.request_time: @@ -77,6 +92,21 @@ 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 + return True def __repr__(self): diff --git a/normandy/recipes/tests/__init__.py b/normandy/recipes/tests/__init__.py index c610bc365..297cd5e28 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, Locale, Recipe, RecipeAction, ReleaseChannel class RecipeFactory(factory.DjangoModelFactory): @@ -19,6 +21,15 @@ def locale(self, create, extracted, **kwargs): if extracted and isinstance(extracted, str): self.locale, _ = Locale.objects.get_or_create(code=extracted) + @factory.post_generation + def release_channels(self, create, extracted, **kwargs): + if not create: + return + + if extracted: + for channel in extracted: + self.release_channels.add(channel) + class ActionFactory(factory.DjangoModelFactory): class Meta: @@ -39,3 +50,11 @@ class Meta: class LocaleFactory(factory.DjangoModelFactory): class Meta: model = Locale + + +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..556aba67c 100644 --- a/normandy/recipes/tests/test_models.py +++ b/normandy/recipes/tests/test_models.py @@ -1,6 +1,8 @@ import pytest -from normandy.recipes.tests import ActionFactory, RecipeActionFactory +from normandy.recipes.tests import ( + ActionFactory, RecipeActionFactory, RecipeFactory, ReleaseChannelFactory) +from normandy.classifier.tests import ClientFactory @pytest.mark.django_db @@ -29,3 +31,34 @@ 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) From 060dae782c3507444402a8c120d963272c925bdf Mon Sep 17 00:00:00 2001 From: Mike Cooper Date: Thu, 18 Feb 2016 13:21:19 -0800 Subject: [PATCH 2/3] 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 +++++-- .../migrations/0017_auto_20160218_2024.py | 55 +++++++++++++ normandy/recipes/migrations/0018_countries.py | 52 ++++++++++++ normandy/recipes/models.py | 81 ++++++++++++------- normandy/recipes/storage.py | 6 ++ normandy/recipes/tests/__init__.py | 27 ++++++- normandy/recipes/tests/test_models.py | 59 +++++++++++++- 9 files changed, 285 insertions(+), 52 deletions(-) create mode 100644 normandy/recipes/migrations/0017_auto_20160218_2024.py create mode 100644 normandy/recipes/migrations/0018_countries.py 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/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 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) From d3180532a4ebe41ac75295517ebfa4e173982549 Mon Sep 17 00:00:00 2001 From: Mike Cooper Date: Wed, 24 Feb 2016 16:21:00 -0800 Subject: [PATCH 3/3] Feedback --- normandy/classifier/models.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/normandy/classifier/models.py b/normandy/classifier/models.py index a492db2a5..85dac40c5 100644 --- a/normandy/classifier/models.py +++ b/normandy/classifier/models.py @@ -2,6 +2,8 @@ import uuid +from django.utils.functional import cached_property + from normandy.classifier.geolocation import get_country_code from normandy.recipes.models import Recipe @@ -12,14 +14,13 @@ 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 + if country is not None: + self.country = country - @property + @cached_property def country(self): - if not self._country: - ip_address = self.request.META.get('REMOTE_ADDR') - self._country = get_country_code(ip_address) - return self._country + ip_address = self.request.META.get('REMOTE_ADDR') + return get_country_code(ip_address) @property def request_time(self):