From 1a0b9b1267e961d42be46c119ef1cb8f750858c0 Mon Sep 17 00:00:00 2001 From: Jared Kerim Date: Tue, 12 Mar 2019 19:09:22 -0400 Subject: [PATCH 1/3] Add normandy slug fixes #582 --- app/experimenter/experiments/admin.py | 1 + app/experimenter/experiments/forms.py | 3 ++ .../0031_experiment_normandy_slug.py | 18 +++++++ app/experimenter/experiments/models.py | 20 ++++++++ .../experiments/tests/test_forms.py | 18 ++++++- .../experiments/tests/test_models.py | 50 +++++++++++++++++++ 6 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 app/experimenter/experiments/migrations/0031_experiment_normandy_slug.py diff --git a/app/experimenter/experiments/admin.py b/app/experimenter/experiments/admin.py index b140e3baa6..fd436ca0d3 100644 --- a/app/experimenter/experiments/admin.py +++ b/app/experimenter/experiments/admin.py @@ -51,6 +51,7 @@ class ExperimentAdmin(admin.ModelAdmin): "proposed_enrollment", "proposed_duration", "bugzilla_id", + "normandy_slug", "data_science_bugzilla_url", "feature_bugzilla_url", "related_work", diff --git a/app/experimenter/experiments/forms.py b/app/experimenter/experiments/forms.py index bf5d450933..7dd4f9006a 100644 --- a/app/experimenter/experiments/forms.py +++ b/app/experimenter/experiments/forms.py @@ -812,6 +812,9 @@ def save(self, *args, **kwargs): and self.new_status == Experiment.STATUS_SHIP and experiment.bugzilla_id ): + experiment.normandy_slug = experiment.generate_normandy_slug() + experiment.save() + tasks.add_experiment_comment_task.delay( self.request.user.id, experiment.id ) diff --git a/app/experimenter/experiments/migrations/0031_experiment_normandy_slug.py b/app/experimenter/experiments/migrations/0031_experiment_normandy_slug.py new file mode 100644 index 0000000000..a555046848 --- /dev/null +++ b/app/experimenter/experiments/migrations/0031_experiment_normandy_slug.py @@ -0,0 +1,18 @@ +# Generated by Django 2.1.7 on 2019-03-13 16:05 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('experiments', '0030_experiment_engineering_owner'), + ] + + operations = [ + migrations.AddField( + model_name='experiment', + name='normandy_slug', + field=models.CharField(blank=True, max_length=255, null=True), + ), + ] diff --git a/app/experimenter/experiments/models.py b/app/experimenter/experiments/models.py index 1823ff4d6f..cfaf52c946 100644 --- a/app/experimenter/experiments/models.py +++ b/app/experimenter/experiments/models.py @@ -115,6 +115,7 @@ class Experiment(ExperimentConstants, models.Model): dashboard_image_url = models.URLField(blank=True, null=True) bugzilla_id = models.CharField(max_length=255, blank=True, null=True) + normandy_slug = models.CharField(max_length=255, blank=True, null=True) data_science_bugzilla_url = models.URLField(blank=True, null=True) feature_bugzilla_url = models.URLField(blank=True, null=True) @@ -236,6 +237,25 @@ def test_tube_url(self): "https://firefox-test-tube.herokuapp.com/experiments/{slug}/" ).format(slug=self.slug) + def generate_normandy_slug(self): + error_msg = ( + "The {field} must be set before a Normandy slug can be generated" + ) + + if not self.firefox_version: + raise ValueError(error_msg.format(field="Firefox version")) + + if not self.firefox_channel: + raise ValueError(error_msg.format(field="Firefox channel")) + + if not self.bugzilla_id: + raise ValueError(error_msg.format(field="Bugzilla ID")) + + return ( + f"{self.type}-{self.slug}-{self.firefox_channel}" + f"-{self.firefox_version}-bug-{self.bugzilla_id}" + ) + @property def has_external_urls(self): return ( diff --git a/app/experimenter/experiments/tests/test_forms.py b/app/experimenter/experiments/tests/test_forms.py index 0b1e953c5a..8aa35f9ec5 100644 --- a/app/experimenter/experiments/tests/test_forms.py +++ b/app/experimenter/experiments/tests/test_forms.py @@ -1047,17 +1047,31 @@ def test_sets_bugzilla_id_when_draft_becomes_review(self): self.user.id, experiment.id ) - def test_adds_bugzilla_comment_when_review_becomes_ship(self): + def test_adds_bugzilla_comment_and_normandy_slug_when_becomes_ship(self): experiment = ExperimentFactory.create_with_status( - Experiment.STATUS_REVIEW, bugzilla_id="12345" + target_status=Experiment.STATUS_REVIEW, + type=Experiment.TYPE_PREF, + name="Experiment Name", + slug="experiment-slug", + firefox_version="57.0", + firefox_channel=Experiment.CHANNEL_NIGHTLY, + bugzilla_id="12345", ) + self.assertEqual(experiment.normandy_slug, None) + form = ExperimentStatusForm( request=self.request, data={"status": experiment.STATUS_SHIP}, instance=experiment, ) + self.assertTrue(form.is_valid()) experiment = form.save() + + self.assertEqual( + experiment.normandy_slug, + "pref-experiment-slug-Nightly-57.0-bug-12345", + ) self.mock_tasks_add_comment.delay.assert_called_with( self.user.id, experiment.id ) diff --git a/app/experimenter/experiments/tests/test_models.py b/app/experimenter/experiments/tests/test_models.py index f306405123..36f2af7520 100644 --- a/app/experimenter/experiments/tests/test_models.py +++ b/app/experimenter/experiments/tests/test_models.py @@ -160,6 +160,56 @@ def test_has_external_urls_is_true_when_bugzilla_and_test_tube_urls(self): ) self.assertTrue(experiment.has_external_urls) + def test_generate_normandy_slug_raises_valueerror_without_version(self): + experiment = ExperimentFactory.create( + type=Experiment.TYPE_PREF, + slug="experiment-slug", + firefox_version="", + firefox_channel="Nightly", + bugzilla_id="12345", + ) + + with self.assertRaises(ValueError): + experiment.generate_normandy_slug() + + def test_generate_normandy_slug_raises_valueerror_without_channel(self): + experiment = ExperimentFactory.create( + type=Experiment.TYPE_PREF, + slug="experiment-slug", + firefox_version="57.0", + firefox_channel="", + bugzilla_id="12345", + ) + + with self.assertRaises(ValueError): + experiment.generate_normandy_slug() + + def test_generate_normandy_slug_raises_valueerror_without_bugzilla(self): + experiment = ExperimentFactory.create( + type=Experiment.TYPE_PREF, + slug="experiment-slug", + firefox_version="57.0", + firefox_channel="Nightly", + bugzilla_id="", + ) + + with self.assertRaises(ValueError): + experiment.generate_normandy_slug() + + def test_generate_normandy_slug_returns_expected_slug(self): + experiment = ExperimentFactory.create( + type=Experiment.TYPE_PREF, + slug="experiment-slug", + firefox_version="57.0", + firefox_channel="Nightly", + bugzilla_id="12345", + ) + + self.assertEqual( + experiment.generate_normandy_slug(), + "pref-experiment-slug-Nightly-57.0-bug-12345", + ) + def test_start_date_returns_proposed_start_date_if_change_is_missing(self): experiment = ExperimentFactory.create_with_variants() self.assertEqual(experiment.start_date, experiment.proposed_start_date) From 0ace451671fb92bc6656022225d8b69218da712d Mon Sep 17 00:00:00 2001 From: Jared Kerim Date: Wed, 13 Mar 2019 13:18:32 -0400 Subject: [PATCH 2/3] Code formatting --- .../migrations/0031_experiment_normandy_slug.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/experimenter/experiments/migrations/0031_experiment_normandy_slug.py b/app/experimenter/experiments/migrations/0031_experiment_normandy_slug.py index a555046848..80720129a7 100644 --- a/app/experimenter/experiments/migrations/0031_experiment_normandy_slug.py +++ b/app/experimenter/experiments/migrations/0031_experiment_normandy_slug.py @@ -5,14 +5,12 @@ class Migration(migrations.Migration): - dependencies = [ - ('experiments', '0030_experiment_engineering_owner'), - ] + dependencies = [("experiments", "0030_experiment_engineering_owner")] operations = [ migrations.AddField( - model_name='experiment', - name='normandy_slug', + model_name="experiment", + name="normandy_slug", field=models.CharField(blank=True, max_length=255, null=True), - ), + ) ] From fcd07d2b3a2ebc76d73b3a8c8f9f34785804015a Mon Sep 17 00:00:00 2001 From: Jared Kerim Date: Thu, 14 Mar 2019 16:06:04 -0400 Subject: [PATCH 3/3] Make slugs lower case --- app/experimenter/experiments/models.py | 2 +- app/experimenter/experiments/tests/test_forms.py | 2 +- app/experimenter/experiments/tests/test_models.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/experimenter/experiments/models.py b/app/experimenter/experiments/models.py index cfaf52c946..9c69bb7e53 100644 --- a/app/experimenter/experiments/models.py +++ b/app/experimenter/experiments/models.py @@ -254,7 +254,7 @@ def generate_normandy_slug(self): return ( f"{self.type}-{self.slug}-{self.firefox_channel}" f"-{self.firefox_version}-bug-{self.bugzilla_id}" - ) + ).lower() @property def has_external_urls(self): diff --git a/app/experimenter/experiments/tests/test_forms.py b/app/experimenter/experiments/tests/test_forms.py index 8aa35f9ec5..67417eaae5 100644 --- a/app/experimenter/experiments/tests/test_forms.py +++ b/app/experimenter/experiments/tests/test_forms.py @@ -1070,7 +1070,7 @@ def test_adds_bugzilla_comment_and_normandy_slug_when_becomes_ship(self): self.assertEqual( experiment.normandy_slug, - "pref-experiment-slug-Nightly-57.0-bug-12345", + "pref-experiment-slug-nightly-57.0-bug-12345", ) self.mock_tasks_add_comment.delay.assert_called_with( self.user.id, experiment.id diff --git a/app/experimenter/experiments/tests/test_models.py b/app/experimenter/experiments/tests/test_models.py index 36f2af7520..d57fffecde 100644 --- a/app/experimenter/experiments/tests/test_models.py +++ b/app/experimenter/experiments/tests/test_models.py @@ -207,7 +207,7 @@ def test_generate_normandy_slug_returns_expected_slug(self): self.assertEqual( experiment.generate_normandy_slug(), - "pref-experiment-slug-Nightly-57.0-bug-12345", + "pref-experiment-slug-nightly-57.0-bug-12345", ) def test_start_date_returns_proposed_start_date_if_change_is_missing(self):