From a6228e52c13d2d4e108117fd2085215f74b75005 Mon Sep 17 00:00:00 2001 From: Jared Lockhart <119884+jaredlockhart@users.noreply.github.com> Date: Mon, 27 Apr 2020 17:25:32 -0400 Subject: [PATCH] Add required signoffs for message type fixes #2496 --- app/experimenter/experiments/constants.py | 28 +++++++++++++++++++ app/experimenter/experiments/forms.py | 4 ++- app/experimenter/experiments/models.py | 26 ++++------------- .../experiments/tests/test_models.py | 13 +++++++++ 4 files changed, 49 insertions(+), 22 deletions(-) diff --git a/app/experimenter/experiments/constants.py b/app/experimenter/experiments/constants.py index a0498fb223b..72612e2f059 100644 --- 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 index 290f982d09a..cd68c404557 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 351ad41a75e..b0b0d610258 100644 --- a/app/experimenter/experiments/models.py +++ b/app/experimenter/experiments/models.py @@ -767,16 +767,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, @@ -784,23 +784,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() @@ -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 index c3196533005..977cdcbec5e 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,