From 82777da67f980b7b18df0da9e4e48295ec4754f7 Mon Sep 17 00:00:00 2001 From: Giorgos Logiotatidis Date: Tue, 18 Jun 2019 14:57:34 +0300 Subject: [PATCH] [Issue #997] Migrate TargetedLocales to Locale --- snippets/base/admin/__init__.py | 3 +- snippets/base/admin/adminmodels.py | 27 +++--- snippets/base/feed.py | 19 ++-- snippets/base/managers.py | 28 +++--- .../migrations/0094_auto_20190619_1039.py | 33 +++++++ snippets/base/models.py | 28 ++++++ snippets/base/tests/__init__.py | 26 ++++-- snippets/base/tests/test_feed.py | 6 +- snippets/base/tests/test_forms.py | 6 +- snippets/base/tests/test_managers.py | 89 +++++++------------ snippets/base/tests/test_models.py | 10 ++- snippets/base/tests/test_validators.py | 13 ++- snippets/base/validators.py | 8 ++ snippets/settings.py | 2 +- 14 files changed, 191 insertions(+), 107 deletions(-) create mode 100644 snippets/base/migrations/0094_auto_20190619_1039.py diff --git a/snippets/base/admin/__init__.py b/snippets/base/admin/__init__.py index 9e2eb06d9..89a29c439 100644 --- a/snippets/base/admin/__init__.py +++ b/snippets/base/admin/__init__.py @@ -4,7 +4,7 @@ from .adminmodels import (AddonAdmin, ASRSnippetAdmin, CampaignAdmin, CategoryAdmin, ClientMatchRuleAdmin, LogEntryAdmin, IconAdmin, - SnippetTemplateAdmin, TargetAdmin) + SnippetTemplateAdmin, TargetAdmin, LocaleAdmin) from .legacy import (SnippetAdmin, SearchProviderAdmin, TargetedCountryAdmin, TargetedLocaleAdmin) @@ -25,3 +25,4 @@ admin.site.register(models.SnippetTemplate, SnippetTemplateAdmin) admin.site.register(models.Target, TargetAdmin) admin.site.register(models.Icon, IconAdmin) +admin.site.register(models.Locale, LocaleAdmin) diff --git a/snippets/base/admin/adminmodels.py b/snippets/base/admin/adminmodels.py index 030811ed8..f346562b1 100644 --- a/snippets/base/admin/adminmodels.py +++ b/snippets/base/admin/adminmodels.py @@ -465,13 +465,13 @@ class ASRSnippetAdmin(admin.ModelAdmin): 'id', 'name', 'status', - 'locale_list', + 'locale', 'modified', ) list_filter = ( filters.ModifiedFilter, filters.TemplateFilter, - ('locales', RelatedOnlyDropdownFilter), + ('locale', RelatedDropdownFilter), ('targets', RelatedOnlyDropdownFilter), 'status', filters.ChannelFilter, @@ -503,7 +503,6 @@ class ASRSnippetAdmin(admin.ModelAdmin): ) filter_horizontal = ( 'targets', - 'locales', ) save_on_top = True save_as = True @@ -541,7 +540,10 @@ class ASRSnippetAdmin(admin.ModelAdmin):
''' # noqa ), - 'fields': ('template_chooser',), + 'fields': ( + 'locale', + 'template_chooser', + ), 'classes': ('template-fieldset',) }), ('Publishing Options', { @@ -550,7 +552,6 @@ class ASRSnippetAdmin(admin.ModelAdmin): 'category', 'targets', ('publish_start', 'publish_end'), - 'locales', 'weight',) }), ('Other Info', { @@ -686,14 +687,6 @@ def has_global_publish_permission(self, request): ] ]) - def locale_list(self, obj): - num_locales = obj.locales.count() - locales = obj.locales.all()[:3] - active_locales = ', '.join([str(locale) for locale in locales]) - if num_locales > 3: - active_locales += ' and {0} more.'.format(num_locales - 3) - return active_locales - class CampaignAdmin(RelatedSnippetsMixin, admin.ModelAdmin): readonly_fields = [ @@ -897,3 +890,11 @@ def save_model(self, request, obj, form, change): obj.creator = request.user statsd.incr('save.target') super().save_model(request, obj, form, change) + + +class LocaleAdmin(admin.ModelAdmin): + list_display = ('name', 'code') + search_fields = ( + 'name', + 'code', + ) diff --git a/snippets/base/feed.py b/snippets/base/feed.py index 83d4b2a78..8219310d0 100644 --- a/snippets/base/feed.py +++ b/snippets/base/feed.py @@ -1,9 +1,11 @@ +import operator from datetime import timedelta from distutils.util import strtobool from textwrap import dedent from urllib.parse import urlparse from django.conf import settings +from django.db.models import Q import django_filters from django_ical.views import ICalFeed @@ -11,18 +13,25 @@ from snippets.base import models -class CharInFilter(django_filters.BaseInFilter, django_filters.CharFilter): - pass - - class ASRSnippetFilter(django_filters.FilterSet): name = django_filters.CharFilter(lookup_expr='icontains') - locale = CharInFilter(field_name='locales__code', lookup_expr='in') + locale = django_filters.CharFilter(method='filter_locale') only_scheduled = django_filters.ChoiceFilter( method='filter_scheduled', choices=(('true', 'Yes'), ('false', 'No'), ('all', 'All'))) + def filter_locale(self, queryset, name, value): + if not value: + return queryset + + locales = value.split(',') + return queryset.filter( + operator.or_( + *[Q(locale__code=',{},'.format(locale)) for locale in locales] + ) + ) + def filter_scheduled(self, queryset, name, value): if value == 'all': return queryset diff --git a/snippets/base/managers.py b/snippets/base/managers.py index d1da9afc7..6755336cc 100644 --- a/snippets/base/managers.py +++ b/snippets/base/managers.py @@ -1,6 +1,6 @@ from datetime import datetime -from django.db.models import Manager +from django.db.models import Manager, Q from django.db.models.query import QuerySet from product_details import product_details @@ -109,29 +109,23 @@ def filter_by_available(self): def match_client(self, client): from snippets.base.models import CHANNELS, ClientMatchRule, Target - snippet_filters = {} - target_filters = {} - # Retrieve the first channel that starts with the client's channel. # Allows things like "release-cck-mozilla14" to match "release". if client.channel == 'default': client_channel = 'nightly' else: - client_channel = first(CHANNELS, client.channel.startswith) - if client_channel: - target_filters.update(**{'on_{0}'.format(client_channel): True}) - targets = Target.objects.filter(**target_filters).distinct() + client_channel = first(CHANNELS, client.channel.startswith) or 'release' - # Only filter by locale if they pass a valid locale. - locales = list(filter(client.locale.lower().startswith, LANGUAGE_VALUES)) - if locales: - snippet_filters.update(locales__code__in=locales) - else: - # If the locale is invalid, only match snippets with no - # locales specified. - snippet_filters.update(locales__isnull=True) + targets = Target.objects.filter(**{'on_{0}'.format(client_channel): True}).distinct() + + # Include both Snippets targeted at the specific full locale (e.g. + # en-us) but also snippets targeted to all territories (en) + full_locale = ',{},'.format(client.locale.lower()) + splitted_locale = ',{},'.format(client.locale.lower().split('-', 1)[0]) + snippets = self.filter(Q(locale__code__contains=splitted_locale) | + Q(locale__code__contains=full_locale)) - snippets = self.filter(**snippet_filters).filter(targets__in=targets) + snippets = snippets.filter(targets__in=targets) # Filter based on ClientMatchRules passed_rules, failed_rules = (ClientMatchRule.objects diff --git a/snippets/base/migrations/0094_auto_20190619_1039.py b/snippets/base/migrations/0094_auto_20190619_1039.py new file mode 100644 index 000000000..d47f2ca52 --- /dev/null +++ b/snippets/base/migrations/0094_auto_20190619_1039.py @@ -0,0 +1,33 @@ +# Generated by Django 2.2.1 on 2019-06-19 10:39 + +import django.core.validators +from django.db import migrations, models +import django.db.models.deletion +import snippets.base.validators + + +class Migration(migrations.Migration): + + dependencies = [ + ('base', '0093_auto_20190618_1325'), + ] + + operations = [ + migrations.CreateModel( + name='Locale', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('name', models.CharField(max_length=100)), + ('code', models.CharField(max_length=255, unique=True, validators=[django.core.validators.RegexValidator(regex='^,?([A-Za-z-]+,?)+$')])), + ('translations', models.TextField(blank=True, default='{}', help_text='JSON dictionary with Template fields as keys and localized strings as values.', validators=[snippets.base.validators.validate_json_data])), + ], + options={ + 'ordering': ('name', 'code'), + }, + ), + migrations.AddField( + model_name='asrsnippet', + name='locale', + field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.PROTECT, to='base.Locale'), + ), + ] diff --git a/snippets/base/models.py b/snippets/base/models.py index 12c80a3b9..616a148a4 100644 --- a/snippets/base/models.py +++ b/snippets/base/models.py @@ -1373,6 +1373,33 @@ def get_rich_text_fields(self): return ['text'] +class Locale(models.Model): + name = models.CharField(max_length=100) + code = models.CharField( + max_length=255, unique=True, + validators=[django_validators.RegexValidator(regex=r'^,?([A-Za-z-]+,?)+$')], + ) + translations = models.TextField( + blank=True, validators=[validators.validate_json_data], default='{}', + help_text='JSON dictionary with Template fields as keys and localized strings as values.' + ) + + def save(self, *args, **kwargs): + # Make sure that code always stats and ends with `,` and it's always lowercase. + self.code = self.code.lower() + if self.code[0] != ',': + self.code = ',' + self.code + if self.code[-1] != ',': + self.code = self.code + ',' + super().save(*args, **kwargs) + + class Meta: + ordering = ('name', 'code') + + def __str__(self): + return self.name + + class ASRSnippet(models.Model): creator = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.PROTECT) created = models.DateTimeField(auto_now_add=True) @@ -1401,6 +1428,7 @@ class ASRSnippet(models.Model): 'See the current time in UTC')) locales = models.ManyToManyField('TargetedLocale', blank=True, verbose_name='Targeted Locales') + locale = models.ForeignKey('Locale', blank=False, null=True, on_delete=models.PROTECT) targets = models.ManyToManyField(Target, default=None, blank=True, related_name='snippets') weight = models.IntegerField( diff --git a/snippets/base/tests/__init__.py b/snippets/base/tests/__init__.py index 22fccd8e6..1371b2267 100644 --- a/snippets/base/tests/__init__.py +++ b/snippets/base/tests/__init__.py @@ -1,3 +1,6 @@ +import random +import string + from django.test import TransactionTestCase import factory @@ -188,16 +191,17 @@ def targets(self, create, extracted, **kwargs): self.targets.add(target) @factory.post_generation - def locales(self, create, extracted, **kwargs): + def locale(self, create, extracted, **kwargs): if not create: return - if extracted is None: - extracted = ['en-us'] - - locales = [models.TargetedLocale.objects.get_or_create(code=code, name=code)[0] - for code in extracted] - self.locales.add(*locales) + code = extracted or 'en-us' + if code[0] != ',': + code = ',' + code + if code[-1] != ',': + code = code + ',' + locale = models.Locale.objects.get_or_create(code=code, name=code)[0] + self.locale = locale class AddonFactory(factory.django.DjangoModelFactory): @@ -207,3 +211,11 @@ class AddonFactory(factory.django.DjangoModelFactory): class Meta: model = models.Addon + + +class LocaleFactory(factory.django.DjangoModelFactory): + name = factory.Sequence(lambda n: 'Locale {}'.format(n)) + code = factory.LazyAttribute(lambda o: ''.join(random.choices(string.ascii_lowercase, k=4))) + + class Meta: + model = models.Locale diff --git a/snippets/base/tests/test_feed.py b/snippets/base/tests/test_feed.py index db16aad56..d8493f659 100644 --- a/snippets/base/tests/test_feed.py +++ b/snippets/base/tests/test_feed.py @@ -48,9 +48,9 @@ def test_name(self): self.assertEqual(set([snippet1]), set(filtr.qs)) def test_locale(self): - snippet1 = ASRSnippetFactory.create(locales=['xx', 'de']) - snippet2 = ASRSnippetFactory.create(locales=['fr']) - ASRSnippetFactory.create(locales=['de']) + snippet1 = ASRSnippetFactory.create(locale='xx') + snippet2 = ASRSnippetFactory.create(locale='fr') + ASRSnippetFactory.create(locale='de') filtr = ASRSnippetFilter(QueryDict(query_string='locale=xx,fr'), queryset=models.ASRSnippet.objects.all()) self.assertEqual(set([snippet1, snippet2]), set(filtr.qs)) diff --git a/snippets/base/tests/test_forms.py b/snippets/base/tests/test_forms.py index c467cbecb..0c0321555 100644 --- a/snippets/base/tests/test_forms.py +++ b/snippets/base/tests/test_forms.py @@ -9,8 +9,8 @@ SnippetAdminForm, TargetAdminForm, TemplateDataWidget, TemplateSelect) from snippets.base.models import STATUS_CHOICES -from snippets.base.tests import (ASRSnippetFactory, SnippetFactory, - SnippetTemplateFactory, +from snippets.base.tests import (ASRSnippetFactory, LocaleFactory, + SnippetFactory, SnippetTemplateFactory, SnippetTemplateVariableFactory, TestCase, TargetFactory) @@ -265,6 +265,7 @@ def test_publish_permission_check_asr(self): user = User.objects.create_user(username='admin', email='foo@example.com', password='admin') + locale = LocaleFactory() perm_beta = Permission.objects.get( codename='publish_on_beta', @@ -292,6 +293,7 @@ def test_publish_permission_check_asr(self): data = { 'name': 'Test', 'weight': 100, + 'locale': locale.id, } # User should get an error trying to publish on Release diff --git a/snippets/base/tests/test_managers.py b/snippets/base/tests/test_managers.py index 371bddb0c..072c743a6 100644 --- a/snippets/base/tests/test_managers.py +++ b/snippets/base/tests/test_managers.py @@ -262,26 +262,28 @@ def test_match_client_base(self): self.assertEqual(set(snippets), set([snippet_1, snippet_2, snippet_3])) - @patch('snippets.base.managers.LANGUAGE_VALUES', ['en-us', 'fr']) def test_match_client(self): params = {} snippet = ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['en-us']) + locale='en-us') ASRSnippetFactory.create( targets=[TargetFactory(on_release=False, on_startpage_6=True)], - locales=['en-us']) + locale='en-us') self._assert_client_matches_snippets(params, [snippet]) - @patch('snippets.base.managers.LANGUAGE_VALUES', ['en-us', 'fr']) def test_match_client_not_matching_channel(self): params = {'channel': 'phantom'} + # When no matching channel, return release snippets snippet = ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['en-us']) + locale='en-us') + # For example don't include Beta + ASRSnippetFactory.create( + targets=[TargetFactory(on_release=False, on_beta=True, on_startpage_6=True)], + locale='en-us') self._assert_client_matches_snippets(params, [snippet]) - @patch('snippets.base.managers.LANGUAGE_VALUES', ['en-us', 'fr']) def test_match_client_match_channel_partially(self): """ Client channels like "release-cck-mozilla14" should match @@ -290,47 +292,30 @@ def test_match_client_match_channel_partially(self): params = {'channel': 'release-cck-mozilla14'} snippet = ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['en-us']) + locale='en-us') ASRSnippetFactory.create( targets=[TargetFactory(on_release=False, on_startpage_6=True)], - locales=['en-us']) - self._assert_client_matches_snippets(params, [snippet]) - - @patch('snippets.base.managers.LANGUAGE_VALUES', ['en-us', 'fr']) - def test_match_client_not_matching_startpage(self): - params = {'startpage_version': '0'} - snippet = ASRSnippetFactory.create( - targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['en-us']) - self._assert_client_matches_snippets(params, [snippet]) - - @patch('snippets.base.managers.LANGUAGE_VALUES', ['en-us', 'fr']) - def test_match_client_not_matching_name(self): - params = {'name': 'unicorn'} - snippet = ASRSnippetFactory.create() + locale='en-us') self._assert_client_matches_snippets(params, [snippet]) - @patch('snippets.base.managers.LANGUAGE_VALUES', ['en-us', 'fr']) def test_match_client_not_matching_locale(self): params = {'locale': 'en-US'} ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=[]) + locale='fr') self._assert_client_matches_snippets(params, []) - @patch('snippets.base.managers.LANGUAGE_VALUES', ['en-us', 'fr']) def test_match_client_match_locale(self): params = {} snippet = ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['en-us']) + locale='en-us') ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['fr']) + locale='fr') self._assert_client_matches_snippets(params, [snippet]) - @patch('snippets.base.managers.LANGUAGE_VALUES', ['es-mx', 'es', 'fr']) - def test_match_client_multiple_locales(self): + def test_match_client_multiple_snippets_for_client_locale(self): """ If there are multiple locales that should match the client's locale, include all of them. @@ -338,39 +323,33 @@ def test_match_client_multiple_locales(self): params = {'locale': 'es-mx'} snippet_1 = ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['es']) + locale='es') snippet_2 = ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['es-mx']) - self._assert_client_matches_snippets(params, [snippet_1, snippet_2]) - - @patch('snippets.base.managers.LANGUAGE_VALUES', ['es-mx', 'es', 'fr']) - def test_match_client_multiple_locales_distinct(self): - """ - If a snippet has multiple locales and a client matches more - than one of them, the snippet should only be included in the - queryset once. - """ - params = {'locale': 'es-mx'} - snippet = ASRSnippetFactory.create( + locale='es-mx') + # Don't include Spanish (Spain) + ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['es', 'es-mx']) - self._assert_client_matches_snippets(params, [snippet]) + locale='es-es') + self._assert_client_matches_snippets(params, [snippet_1, snippet_2]) - @patch('snippets.base.managers.LANGUAGE_VALUES', ['en-us', 'fr']) - def test_match_client_invalid_locale(self): - """ - If client sends invalid locale return snippets with no locales - specified. - """ - params = {'locale': 'foo'} - snippet = ASRSnippetFactory.create( + def test_match_client_locale_without_territory(self): + params = {'locale': 'es'} + snippet_1 = ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=[]) + locale='es') + # Don't include Spanish (Spain) ASRSnippetFactory.create( targets=[TargetFactory(on_release=True, on_startpage_6=True)], - locales=['en-us']) - self._assert_client_matches_snippets(params, [snippet]) + locale='es-es') + self._assert_client_matches_snippets(params, [snippet_1]) + + def test_match_locale_with_multiple_codes(self): + params = {'locale': 'es-mx'} + snippet_1 = ASRSnippetFactory.create( + targets=[TargetFactory(on_release=True, on_startpage_6=True)], + locale=',es-ar,es-cl,es-mx,') + self._assert_client_matches_snippets(params, [snippet_1]) def test_default_is_same_as_nightly(self): """ Make sure that default channel follows nightly. """ diff --git a/snippets/base/tests/test_models.py b/snippets/base/tests/test_models.py index 7276590ff..c51ecb814 100644 --- a/snippets/base/tests/test_models.py +++ b/snippets/base/tests/test_models.py @@ -10,6 +10,7 @@ from snippets.base.models import (STATUS_CHOICES, Client, Icon, + Locale, SimpleTemplate, _generate_filename) from snippets.base.util import fluent_link_extractor @@ -446,7 +447,6 @@ def test_duplicate(self): user = UserFactory.create() snippet = ASRSnippetFactory.create( status=STATUS_CHOICES['Published'], - locales=['en-us', 'fr'], ) duplicate_snippet = snippet.duplicate(user) snippet.refresh_from_db() @@ -454,7 +454,6 @@ def test_duplicate(self): for attr in ['id', 'creator', 'created', 'modified', 'name', 'uuid']: self.assertNotEqual(getattr(snippet, attr), getattr(duplicate_snippet, attr)) - self.assertEqual(set(snippet.locales.all()), set(duplicate_snippet.locales.all())) self.assertEqual(duplicate_snippet.status, STATUS_CHOICES['Draft']) self.assertNotEqual(snippet.template_ng.pk, duplicate_snippet.template_ng.pk) @@ -544,3 +543,10 @@ def test_modified_date_updates_when_target_updates(self): new_modified = snippet.modified self.assertNotEqual(old_modified, new_modified) + + +class LocaleTests(TestCase): + def test_code_commas_and_case(self): + locale = Locale(name='foo', code='Bar') + locale.save() + self.assertEqual(locale.code, ',bar,') diff --git a/snippets/base/tests/test_validators.py b/snippets/base/tests/test_validators.py index 6d3856604..848b18a98 100644 --- a/snippets/base/tests/test_validators.py +++ b/snippets/base/tests/test_validators.py @@ -9,7 +9,8 @@ from snippets.base.validators import (validate_as_router_fluent_variables, validate_xml_template, validate_xml_variables, - validate_regex, validate_image_format) + validate_regex, validate_image_format, + validate_json_data) from snippets.base.tests import TestCase @@ -97,3 +98,13 @@ def test_invalid_image(self): image = InMemoryUploadedFile(fle, 'ImageField', 'foo.jpg', 'image/jpeg', None, None) self.assertRaises(ValidationError, validate_image_format, image) + + +class ValidateJSONDataTests(TestCase): + def test_base(self): + data = '{"foo": 3}' + self.assertEqual(validate_json_data(data), data) + + def test_invalid_data(self): + data = '{"foo": 3' + self.assertRaises(ValidationError, validate_json_data, data) diff --git a/snippets/base/validators.py b/snippets/base/validators.py index 869a5b68a..cade89ddc 100644 --- a/snippets/base/validators.py +++ b/snippets/base/validators.py @@ -110,3 +110,11 @@ def validate_image_format(image): if img.format not in ['PNG']: raise ValidationError('Upload only PNG images.') return image + + +def validate_json_data(data): + try: + json.loads(data) + except ValueError: + raise ValidationError('Enter valid JSON string.') + return data diff --git a/snippets/settings.py b/snippets/settings.py index a10dfc534..01e520ae7 100644 --- a/snippets/settings.py +++ b/snippets/settings.py @@ -328,7 +328,7 @@ 'base.Target', 'base.Icon', 'base.Addon', - 'base.Template', + 'base.Locale', ] },