Merge pull request #1392 from glogiotatidis/distribution-updates

[Fix 1391] Allow disabling DistributionBundles.

snippets/base/admin/__init__.py

@@ -26,6 +26,6 @@
 admin.site.register(models.Locale, adminmodels.LocaleAdmin)
 admin.site.register(models.Job, adminmodels.JobAdmin)
 admin.site.register(models.Distribution, adminmodels.DistributionAdmin)
-admin.site.register(models.DistributionBundle)
+admin.site.register(models.DistributionBundle, adminmodels.DistributionBundleAdmin)
 admin.site.register(models.JobDailyPerformance, adminmodels.JobDailyPerformanceAdmin)
 admin.site.register(models.DailyImpressions, adminmodels.DailyImpressionsAdmin)

snippets/base/admin/adminmodels.py

@@ -1023,6 +1023,7 @@ class LocaleAdmin(admin.ModelAdmin):
 
 
 class JobAdmin(admin.ModelAdmin):
+    form = forms.JobAdminForm
     save_on_top = True
     preserve_filters = True
     filter_horizontal = [
@@ -1368,6 +1369,31 @@ def action_delete_job(self, request, queryset):
 class DistributionAdmin(admin.ModelAdmin):
     save_on_top = True
 
+    list_display = [
+        'id',
+        'name',
+    ]
+
+
+class DistributionBundleAdmin(admin.ModelAdmin):
+    save_on_top = True
+    list_display = [
+        'name',
+        'code_name',
+        'enabled',
+        'distribution_list',
+    ]
+
+    def distribution_list(self, obj):
+        return mark_safe(
+            '<ul>' +
+            ''.join([
+                f'<li> {dist}' for dist in obj.distributions.values_list('name', flat=True)
+            ]) +
+            '</ul>'
+        )
+    distribution_list.short_description = 'Distributions'
+
 
 class JobDailyPerformanceAdmin(admin.ModelAdmin):
     list_display = [

snippets/base/forms.py

@@ -850,3 +850,14 @@ def save(self, *args, **kwargs):
         self.instance.jexl_expr = ' && '.join([x for x in jexl_expr_array if x])
 
         return super().save(*args, **kwargs)
+
+
+class JobAdminForm(forms.ModelForm):
+    distribution = forms.ModelChoiceField(
+        queryset=models.Distribution.objects.filter(
+            distributionbundle__enabled=True
+        ).distinct(),
+        empty_label='Select Distribution',
+        help_text=('Set a Distribution for this Job. It should be normally '
+                   'left to Default. Useful for running Normandy experiments.'),
+    )

snippets/base/management/commands/generate_bundles.py

@@ -53,7 +53,7 @@ def handle(self, *args, **options):
         )
         distribution_bundles_to_process = DistributionBundle.objects.filter(
             distributions__jobs__in=total_jobs
-        ).distinct()
+        ).distinct().order_by('id')
 
         for distribution_bundle in distribution_bundles_to_process:
             distributions = distribution_bundle.distributions.all()
@@ -96,10 +96,10 @@ def handle(self, *args, **options):
                         Q(snippet__locale__code__contains=splitted_locale) |
                         Q(snippet__locale__code__contains=full_locale)).distinct()
 
-                    # If there 're no Published Jobs for the channel / locale /
-                    # distribution combination, delete the current bundle file if
-                    # it exists.
-                    if not bundle_jobs.exists():
+                    # If DistributionBundle is not enabled, or if there are no
+                    # Published Jobs for the channel / locale / distribution
+                    # combination, delete the current bundle file if it exists.
+                    if not distribution_bundle.enabled or not bundle_jobs.exists():
                         if default_storage.exists(filename):
                             self.stdout.write('Removing {}'.format(filename))
                             default_storage.delete(filename)

snippets/base/migrations/0037_auto_20200604_1218.py

@@ -0,0 +1,24 @@
+# Generated by Django 2.2.10 on 2020-06-04 12:18
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('base', '0036_auto_20200513_0724'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='distributionbundle',
+            name='enabled',
+            field=models.BooleanField(default=True),
+        ),
+        migrations.AlterField(
+            model_name='job',
+            name='distribution',
+            field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, related_name='jobs', to='base.Distribution'),
+        ),
+    ]

snippets/base/models.py

@@ -1851,8 +1851,7 @@ class Job(models.Model):
         'Distribution',
         on_delete=models.PROTECT,
         related_name='jobs',
-        help_text=('Set a Distribution for this Job. It should be normally '
-                   'left to Default. Useful for running Normandy experiments.'),
+        # A terrible hack to get `distinct` working.
     )
 
     client_limit_lifetime = models.PositiveIntegerField(
@@ -2232,6 +2231,8 @@ class DistributionBundle(models.Model):
         'Distribution'
     )
 
+    enabled = models.BooleanField(default=True)
+
     def __str__(self):
         return self.name
 

snippets/base/tests/test_commands.py

@@ -282,7 +282,7 @@ def test_base(self):
         self.assertEqual(job_block_limit_not_reached_just_created.status, models.Job.PUBLISHED)
 
 
-class GenerateBundles(TestCase):
+class GenerateBundlesTests(TestCase):
     def setUp(self):
         self.distribution = DistributionFactory.create(name='Default')
         self.distribution_bundle = DistributionBundleFactory.create(name='Default',
@@ -456,6 +456,13 @@ def test_no_brotli(self):
 
     @override_settings(MEDIA_BUNDLES_PREGEN_ROOT='pregen')
     def test_delete(self):
+        distribution = DistributionFactory()
+        distribution_bundle = DistributionBundleFactory(
+            code_name='foo',
+            enabled=False,
+        )
+        distribution_bundle.distributions.add(distribution)
+
         target = TargetFactory(
             on_release=False, on_beta=False, on_nightly=True, on_esr=False, on_aurora=False
         )
@@ -465,14 +472,23 @@ def test_delete(self):
             targets=[target],
         )
 
+        # Still published, but belongs to a disabled distribution
+        JobFactory(
+            status=models.Job.PUBLISHED,
+            snippet__locale=',fr,',
+            targets=[target],
+            distribution=distribution,
+        )
+
         with patch('snippets.base.management.commands.generate_bundles.default_storage') as ds_mock:
             # Test that only removes if file exists.
             ds_mock.exists.return_value = True
             call_command('generate_bundles', stdout=Mock())
 
-        ds_mock.delete.assert_called_once_with(
-            'pregen/Firefox/nightly/fr/default.json'
-        )
+        ds_mock.delete.assert_has_calls([
+            call('pregen/Firefox/nightly/fr/default.json'),
+            call('pregen/Firefox/nightly/fr/foo.json')
+        ])
         ds_mock.save.assert_not_called()
 
     def test_delete_does_not_exist(self):