From 22d69945e9e73ea4b01cdaee88b6e2bd6f8779b0 Mon Sep 17 00:00:00 2001 From: Mike Cooper Date: Fri, 19 Feb 2016 17:36:34 -0800 Subject: [PATCH] wip --- normandy/classifier/models.py | 13 ++-- normandy/classifier/tests/test_api.py | 16 +++-- .../migrations/0016_auto_20160218_2024.py | 55 ---------------- normandy/recipes/migrations/0017_countries.py | 49 -------------- normandy/recipes/models.py | 64 +++++++++++-------- normandy/recipes/tests/__init__.py | 27 ++++++-- normandy/recipes/tests/test_models.py | 59 ++++++++++++++++- 7 files changed, 134 insertions(+), 149 deletions(-) delete mode 100644 normandy/recipes/migrations/0016_auto_20160218_2024.py delete mode 100644 normandy/recipes/migrations/0017_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/migrations/0016_auto_20160218_2024.py b/normandy/recipes/migrations/0016_auto_20160218_2024.py deleted file mode 100644 index 47bf1130c..000000000 --- a/normandy/recipes/migrations/0016_auto_20160218_2024.py +++ /dev/null @@ -1,55 +0,0 @@ -# -*- 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', '0015_auto_20160217_1819'), - ] - - 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/0017_countries.py b/normandy/recipes/migrations/0017_countries.py deleted file mode 100644 index 67050e7f1..000000000 --- a/normandy/recipes/migrations/0017_countries.py +++ /dev/null @@ -1,49 +0,0 @@ -# -*- 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') - english = Locale.objects.get(code='en-US') - english.order = 0 - english.save() - - -def noop(apps, schema_editor): - pass - - -class Migration(migrations.Migration): - - dependencies = [ - ('recipes', '0016_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 ed781c4dc..96a849494 100644 --- a/normandy/recipes/models.py +++ b/normandy/recipes/models.py @@ -20,7 +20,7 @@ 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() + order = models.IntegerField(default=100) class Meta: ordering = ['order', 'code'] @@ -28,6 +28,9 @@ class Meta: 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,12 +43,15 @@ 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() + order = models.IntegerField(default=100) class Meta: ordering = ['order', 'name'] @@ -53,6 +59,9 @@ class Meta: 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.""" @@ -71,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 @@ -105,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/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)