Make Recipe locale and country selection many-to-many

Fixes bug 1248263.

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):

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'}

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):

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
 

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.

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:

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)