Merge pull request #1473 from glogiotatidis/issue-1446

[Fix #1446] Move Channel targeting in the browser.

docs/architecture/decisions/0006-release-jobs-in-nightly-bundles.md

@@ -4,7 +4,7 @@ Date: 2020-03-31
 
 ## Status
 
-Accepted
+Replaced by ADR 0007.
 
 ## Context
 

docs/architecture/decisions/0007-move-channel-targeting-to-browser.md

@@ -0,0 +1,29 @@
+# 7. Move Channel targeting to browser.
+
+Date: 2020-11-12
+
+## Status
+
+Accepted
+
+## Context
+
+We want to be able to create more complex Targets, specifically targets that
+will evaluate to true for Profiles older than X weeks with X being different for
+each channel.
+
+The requirement comes from an initiative to reduce the number of active Jobs per week and thus reduce the programming and analyzing time required. With this change we will be able to schedule one Job for multiple channels while maintaining different targeting for each channel.
+
+
+## Decision
+
+We decide to move Channel targeting from the server to the browser. To accomplish this we will take advantage of `browser.update.channel` JEXL attribute to target snippets based on channel and remove any server side code that does channel targeting.
+
+We will generate one bundle for each locale, instead of one bundle for each locale, channel combination. 
+
+This ADR replaces 0006 since all Jobs for locale will be included in all channels.
+
+
+## Consequences
+
+CDN traffic is expected to increase since bundles are going to include Job from all channels of a locale. The increase is not expected to be significant because traditionally most Jobs are on Release channel and it's the Release that generates from of the traffic.

requirements.txt

@@ -540,3 +540,8 @@ docutils==0.15 \
     --hash=sha256:54a349c622ff31c91cbec43b0b512f113b5b24daf00e2ea530bb1bd9aac14849 \
     --hash=sha256:ba4584f9107571ced0d2c7f56a5499c696215ba90797849c92d395979da68521 \
     --hash=sha256:d2ddba74835cb090a1b627d3de4e7835c628d07ee461f7b4480f51af2fe4d448
+pyjexl==0.2.3 \
+    --hash=sha256:1369c08c3f1f6931bc3c3089f24e79902680b6022412ccfc5e818441cf0dca52 \
+    --hash=sha256:56eb1ab1bd78eb12d3c514b6e2f2c93fe7f2fdd00bd821873e3f089706452c51
+parsimonious==0.8.1 \
+    --hash=sha256:3add338892d580e0cb3b1a39e4a1b427ff9f687858fdd61097053742391a9f6b

snippets/base/admin/adminmodels.py

@@ -931,20 +931,13 @@ class TargetAdmin(RelatedJobsMixin, admin.ModelAdmin):
     ]
     fieldsets_custom = [
         ('ID', {'fields': ('name',)}),
-        ('Product channels', {
-            'description': 'What channels will this snippet be available in?',
-            'fields': (('on_release', 'on_beta', 'on_aurora', 'on_nightly', 'on_esr'),)
-        }),
         ('Targeting', {'fields': ('jexl_expr',)}),
     ]
     fieldsets_full = [
         ('ID', {'fields': ('name',)}),
-        ('Product channels', {
-            'description': 'What channels will this snippet be available in?',
-            'fields': (('on_release', 'on_beta', 'on_aurora', 'on_nightly', 'on_esr'),)
-        }),
         ('Targeting', {
             'fields': (
+                'filtr_channel',
                 'filtr_is_default_browser',
                 'filtr_needs_update',
                 'filtr_updates_enabled',

snippets/base/admin/fields.py

@@ -1,6 +1,7 @@
 from django.core.exceptions import ValidationError
-from django.forms import (ChoiceField, ModelChoiceField, ModelMultipleChoiceField,
-                          MultiValueField, MultipleChoiceField)
+from django.forms import (ChoiceField, ModelChoiceField,
+                          ModelMultipleChoiceField, MultipleChoiceField,
+                          MultiValueField)
 
 from snippets.base.models import Addon, TargetedCountry
 
@@ -14,7 +15,7 @@ class MultipleChoiceFieldCSV(MultipleChoiceField):
 
     def prepare_value(self, value):
         value = super(MultipleChoiceFieldCSV, self).prepare_value(value)
-        if not isinstance(value, list):
+        if value and not isinstance(value, list):
             value = value.split(';')
         return value
 
@@ -31,6 +32,30 @@ def to_jexl(self, value):
         return None
 
 
+class JEXLMultipleChoiceField(JEXLBaseField, MultipleChoiceFieldCSV):
+    def __init__(self, attr_name, *args, **kwargs):
+        self.attr_name = attr_name
+        super().__init__(*args, **kwargs)
+
+    def to_jexl(self, value):
+        if value:
+            return f'{self.attr_name} in {[x for x in value.split(";")]}'
+        return None
+
+
+class JEXLChannelField(JEXLMultipleChoiceField):
+    def to_jexl(self, value):
+        if not value:
+            return None
+        values = value.split(';')
+
+        # When `release` is selected, also add `default`
+        if 'release' in values:
+            values.append('default')
+
+        return f'{self.attr_name} in {[x for x in values]}'
+
+
 class JEXLChoiceField(JEXLBaseField, ChoiceField):
     def __init__(self, attr_name, *args, **kwargs):
         self.attr_name = attr_name

snippets/base/admin/filters.py

@@ -46,20 +46,20 @@ class ChannelFilter(admin.SimpleListFilter):
 
     def lookups(self, request, model_admin):
         return (
-            ('on_release', 'Release'),
-            ('on_esr', 'ESR'),
-            ('on_beta', 'Beta'),
-            ('on_aurora', 'Dev (Aurora)'),
-            ('on_nightly', 'Nightly'),
+            ('release', 'Release'),
+            ('esr', 'ESR'),
+            ('beta', 'Beta'),
+            ('aurora', 'Dev (Aurora)'),
+            ('nightly', 'Nightly'),
         )
 
     def queryset(self, request, queryset):
         if self.value() is None:
             return queryset
 
         if hasattr(queryset.model, 'jobs'):
-            return queryset.filter(**{f'jobs__targets__{self.value()}': True}).distinct()
-        return queryset.filter(**{f'targets__{self.value()}': True}).distinct()
+            return queryset.filter(jobs__targets__filtr_channels__contains=self.value()).distinct()
+        return queryset.filter(targets__filtr_channels__contains=self.value()).distinct()
 
 
 class TemplateFilter(admin.SimpleListFilter):

snippets/base/bundles.py

@@ -14,7 +14,7 @@
 from snippets.base import models
 
 
-def generate_bundles(timestamp=None, limit_to_locale=None, limit_to_channel=None,
+def generate_bundles(timestamp=None, limit_to_locale=None,
                      limit_to_distribution_bundle=None, save_to_disk=True,
                      stdout=StringIO()):
     if not timestamp:
@@ -30,17 +30,14 @@ def generate_bundles(timestamp=None, limit_to_locale=None, limit_to_channel=None
         ).distinct()
 
     stdout.write('Processing bundles…')
-    if limit_to_locale and limit_to_channel:
-        combinations_to_process = [
-            (limit_to_channel, limit_to_locale)
+    if limit_to_locale:
+        all_locales_to_process = [
+            limit_to_locale,
         ]
     else:
-        combinations_to_process = set(
+        all_locales_to_process = set(
             itertools.chain.from_iterable(
-                itertools.product(
-                    job.channels,
-                    job.snippet.locale.code.strip(',').split(',')
-                )
+                job.snippet.locale.code.strip(',').split(',')
                 for job in total_jobs
             )
         )
@@ -56,34 +53,19 @@ def generate_bundles(timestamp=None, limit_to_locale=None, limit_to_channel=None
     for distribution_bundle in distribution_bundles_to_process:
         distributions = distribution_bundle.distributions.all()
 
-        for channel, locale in combinations_to_process:
-            additional_jobs = []
-            if channel == 'nightly' and settings.NIGHTLY_INCLUDES_RELEASE:
-                additional_jobs = models.Job.objects.filter(
-                    status=models.Job.PUBLISHED).filter(**{
-                        'targets__on_release': True,
-                        'distribution__in': distributions,
-                    })
-
-            channel_jobs = models.Job.objects.filter(
-                status=models.Job.PUBLISHED).filter(
-                    Q(**{
-                        'targets__on_{}'.format(channel): True,
-                        'distribution__in': distributions,
-                    }))
-
-            all_jobs = models.Job.objects.filter(
-                Q(id__in=additional_jobs) | Q(id__in=channel_jobs)
-            )
+        for locale in all_locales_to_process:
+
+            all_jobs = (models.Job.objects
+                        .filter(status=models.Job.PUBLISHED)
+                        .filter(distribution__in=distributions))
 
             locales_to_process = [
                 key.lower() for key in product_details.languages.keys()
                 if key.lower().startswith(locale)
             ]
 
             for locale_to_process in locales_to_process:
-                filename = 'Firefox/{channel}/{locale}/{distribution}.json'.format(
-                    channel=channel,
+                filename = 'Firefox/{locale}/{distribution}.json'.format(
                     locale=locale_to_process,
                     distribution=distribution_bundle.code_name,
                 )
@@ -95,29 +77,22 @@ def generate_bundles(timestamp=None, limit_to_locale=None, limit_to_channel=None
                     Q(snippet__locale__code__contains=full_locale)).distinct()
 
                 # If DistributionBundle is not enabled, or if there are no
-                # Published Jobs for the channel / locale / distribution
+                # Published Jobs for the locale / distribution
                 # combination, delete the current bundle file if it exists.
                 if save_to_disk and not distribution_bundle.enabled or not bundle_jobs.exists():
                     if default_storage.exists(filename):
                         stdout.write('Removing {}'.format(filename))
                         default_storage.delete(filename)
                     continue
 
-                data = []
-                channel_job_ids = list(channel_jobs.values_list('id', flat=True))
-                for job in bundle_jobs:
-                    if job.id in channel_job_ids:
-                        render = job.render()
-                    else:
-                        render = job.render(always_eval_to_false=True)
-                    data.append(render)
-
+                data = [
+                    job.render() for job in bundle_jobs
+                ]
                 bundle_content = json.dumps({
                     'messages': data,
                     'metadata': {
                         'generated_at': datetime.utcnow().isoformat(),
                         'number_of_snippets': len(data),
-                        'channel': channel,
                         'locale': locale_to_process,
                         'distribution_bundle': distribution_bundle.code_name,
                     }
@@ -149,7 +124,6 @@ def generate_bundles(timestamp=None, limit_to_locale=None, limit_to_channel=None
                 'metadata': {
                     'generated_at': datetime.utcnow().isoformat(),
                     'number_of_snippets': 0,
-                    'channel': limit_to_channel,
                     'locale': limit_to_locale,
                     'distribution_bundle': limit_to_distribution_bundle,
                 }

snippets/base/forms.py

@@ -250,10 +250,22 @@ def save(self, *args, **kwargs):
 class TargetAdminCustomForm(forms.ModelForm):
     class Meta:
         model = models.Target
-        fields = ['name', 'jexl_expr', 'on_release', 'on_esr', 'on_beta', 'on_aurora', 'on_nightly']
+        fields = ['name', 'jexl_expr']
 
 
 class TargetAdminForm(forms.ModelForm):
+    filtr_channels = fields.JEXLChannelField(
+        'browserSettings.update.channel',
+        choices=[
+            ('release', 'Release'),
+            ('esr', 'ESR'),
+            ('beta', 'Beta'),
+            ('aurora', 'Dev'),
+            ('nightly', 'Nightly')
+        ],
+        widget=forms.CheckboxSelectMultiple(),
+        required=False,
+    )
     filtr_is_default_browser = fields.JEXLChoiceField(
         'isDefaultBrowser',
         choices=((None, "I don't care"),
@@ -443,16 +455,17 @@ class Meta:
         model = models.Target
         exclude = ['creator', 'created', 'modified']
 
-    def save(self, *args, **kwargs):
+    def generate_jexl_expr(self, data):
         jexl_expr_array = []
-
         for name, field in self.fields.items():
             if name.startswith('filtr_'):
-                value = self.cleaned_data[name]
+                value = data[name]
                 if value:
                     jexl_expr_array.append(field.to_jexl(value))
-        self.instance.jexl_expr = ' && '.join([x for x in jexl_expr_array if x])
+        return ' && '.join([x for x in jexl_expr_array if x])
 
+    def save(self, *args, **kwargs):
+        self.instance.jexl_expr = self.generate_jexl_expr(self.cleaned_data)
         return super().save(*args, **kwargs)
 
 

snippets/base/migrations/0044_target_filtr_channel.py

@@ -0,0 +1,18 @@
+# Generated by Django 2.2.13 on 2020-10-26 10:28
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('base', '0043_auto_20201104_0811'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='target',
+            name='filtr_channels',
+            field=models.CharField(default='release;', max_length=255),
+        ),
+    ]

snippets/base/migrations/0045_auto_20201026_1255.py

@@ -0,0 +1,36 @@
+# Generated by Django 2.2.13 on 2020-10-26 12:55
+
+from django.db import migrations
+from django.db.models import Q
+
+def forwards(apps, schema_editor):
+    Target = apps.get_model('base', 'Target')
+    from snippets.base.forms import TargetAdminForm
+    for target in Target.objects.filter(
+            Q(on_release=True) | \
+            Q(on_esr=True) | \
+            Q(on_beta=True) | \
+            Q(on_aurora=True) | \
+            Q(on_nightly=True)):
+
+        value = ''
+        for key in ['release', 'esr', 'beta', 'aurora', 'nightly']:
+            if getattr(target, f'on_{key}', False):
+                value += f'{key};'
+        value = value.strip(';')
+
+        target.filtr_channels = value
+        f = TargetAdminForm(instance=target)
+        target.jexl_expr = f.generate_jexl_expr(f.initial)
+        target.save()
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('base', '0044_target_filtr_channel'),
+    ]
+
+    operations = [
+        migrations.RunPython(forwards),
+    ]