Add required signoffs for message type fixes #2496

mozilla · Apr 27, 2020 · a6228e5 · a6228e5
1 parent cc67ead
commit a6228e5
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 22 deletions.
diff --git a/app/experimenter/experiments/constants.py b/app/experimenter/experiments/constants.py
@@ -315,6 +315,34 @@ def FEATURE_TYPE_CHOICES(cls):  # pragma: no cover
         ],
     }
 
+    SIGNOFF_DEFAULTS = (
+        "review_science",
+        "review_advisory",
+        "review_engineering",
+        "review_qa_requested",
+        "review_intent_to_ship",
+        "review_bugzilla",
+        "review_qa",
+        "review_relman",
+    )
+
+    SIGNOFF_TYPE_DEFAULTS = {
+        TYPE_ROLLOUT: (
+            "review_advisory",
+            "review_qa_requested",
+            "review_intent_to_ship",
+            "review_qa",
+            "review_relman",
+        ),
+        TYPE_MESSAGE: (
+            "review_science",
+            "review_qa_requested",
+            "review_intent_to_ship",
+            "review_qa",
+            "review_ux",
+        ),
+    }
+
     RISK_LABELS = {
         "risk_partner_related": RISK_PARTNER_RELATED_LABEL,
         "risk_brand": RISK_BRAND_LABEL,

diff --git a/app/experimenter/experiments/forms.py b/app/experimenter/experiments/forms.py
@@ -697,7 +697,9 @@ class ExperimentReviewForm(ExperimentConstants, ChangeLogMixin, forms.ModelForm)
         help_text=Experiment.REVIEW_GENERAL_HELP_TEXT,
     )
     review_ux = forms.BooleanField(
-        required=False, label="UX Review", help_text=Experiment.REVIEW_GENERAL_HELP_TEXT
+        required=False,
+        label="Copy/UX Review",
+        help_text=Experiment.REVIEW_GENERAL_HELP_TEXT,
     )
     review_security = forms.BooleanField(
         required=False,

diff --git a/app/experimenter/experiments/models.py b/app/experimenter/experiments/models.py
@@ -767,40 +767,24 @@ def completed_testing(self):
     def _conditional_required_reviews_mapping(self):
         return {
             "review_vp": any(
-                [
+                (
                     self.risk_partner_related,
                     self.risk_brand,
                     self.risk_fast_shipped,
                     self.risk_confidential,
                     self.risk_release_population,
                     self.risk_revenue,
-                ]
+                )
             ),
-            "review_legal": any([self.risk_partner_related, self.risk_data_category]),
+            "review_legal": any((self.risk_partner_related, self.risk_data_category)),
             "review_impacted_teams": self.risk_external_team_impact,
             "review_data_steward": self.risk_telemetry_data,
             "review_ux": self.risk_ux,
             "review_security": self.risk_security,
         }
 
     def _default_required_reviews(self):
-        reviews = [
-            "review_science",
-            "review_advisory",
-            "review_engineering",
-            "review_qa_requested",
-            "review_intent_to_ship",
-            "review_bugzilla",
-            "review_qa",
-            "review_relman",
-        ]
-
-        rollout_exclusions = ["review_science", "review_bugzilla", "review_engineering"]
-
-        if self.is_rollout:
-            return [review for review in reviews if review not in rollout_exclusions]
-
-        return reviews
+        return list(self.SIGNOFF_TYPE_DEFAULTS.get(self.type, self.SIGNOFF_DEFAULTS))
 
     def get_all_required_reviews(self):
         required_reviews = self._default_required_reviews()
@@ -814,7 +798,7 @@ def get_all_required_reviews(self):
     def completed_required_reviews(self):
         required_reviews = self.get_all_required_reviews()
 
-        if not self.is_rollout:
+        if not self.is_rollout and "review_advisory" in required_reviews:
             # review advisory is an exception that is not required
             required_reviews.remove("review_advisory")
 

diff --git a/app/experimenter/experiments/tests/test_models.py b/app/experimenter/experiments/tests/test_models.py
@@ -1092,6 +1092,7 @@ def test_completed_required_reviews_false_when_reviews_not_complete(self):
 
     def test_completed_required_reviews_true_when_reviews_complete(self):
         experiment = ExperimentFactory.create(
+            type=Experiment.TYPE_PREF,
             review_science=True,
             review_engineering=True,
             review_qa_requested=True,
@@ -1128,6 +1129,17 @@ def test_required_reviews_for_rollout(self):
         )
         self.assertTrue(experiment.completed_required_reviews)
 
+    def test_required_reviews_for_message(self):
+        experiment = ExperimentFactory.create(
+            type=Experiment.TYPE_MESSAGE,
+            review_science=True,
+            review_qa_requested=True,
+            review_intent_to_ship=True,
+            review_qa=True,
+            review_ux=True,
+        )
+        self.assertTrue(experiment.completed_required_reviews)
+
     def test_completed_all_sections_false_when_incomplete(self):
         experiment = ExperimentFactory.create()
         self.assertFalse(experiment.completed_all_sections)
@@ -1193,6 +1205,7 @@ def test_completed_all_sections_true_for_generic_with_design(self):
     def test_is_ready_to_launch_true_when_reviews_and_sections_complete(self):
         experiment = ExperimentFactory.create_with_status(
             Experiment.STATUS_REVIEW,
+            type=Experiment.TYPE_PREF,
             review_science=True,
             review_engineering=True,
             review_qa_requested=True,