diff --git a/app/experimenter/experiments/constants.py b/app/experimenter/experiments/constants.py index 4c9b14fb37..4ee1e9feac 100644 --- a/app/experimenter/experiments/constants.py +++ b/app/experimenter/experiments/constants.py @@ -324,6 +324,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_intent_to_ship", + "review_qa_requested", + "review_ux", + "review_qa", + ), + } + 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 index 290f982d09..cd68c40455 100644 --- 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 index f616a39267..85619dfb0d 100644 --- a/app/experimenter/experiments/models.py +++ b/app/experimenter/experiments/models.py @@ -774,16 +774,16 @@ 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, @@ -791,23 +791,7 @@ def _conditional_required_reviews_mapping(self): } 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() @@ -821,7 +805,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 index c319653300..977cdcbec5 100644 --- 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,