diff --git a/app/experimenter/experiments/forms.py b/app/experimenter/experiments/forms.py index 742105606e..6c1040ecf7 100644 --- a/app/experimenter/experiments/forms.py +++ b/app/experimenter/experiments/forms.py @@ -566,48 +566,68 @@ class RadioWidget(forms.widgets.RadioSelect): class ExperimentRisksForm(ChangeLogMixin, forms.ModelForm): RADIO_OPTIONS = ((False, "No"), (True, "Yes")) + def coerce_truthy(value): + if value.lower() == "true": + return True + elif value.lower() == "false": + return False + # Radio Buttons - risk_internal_only = forms.ChoiceField( + risk_internal_only = forms.TypedChoiceField( label=Experiment.RISK_INTERNAL_ONLY_LABEL, help_text=Experiment.RISK_INTERNAL_ONLY_HELP_TEXT, choices=RADIO_OPTIONS, widget=RadioWidget, + coerce=coerce_truthy, + empty_value=None, ) - risk_partner_related = forms.ChoiceField( + risk_partner_related = forms.TypedChoiceField( label=Experiment.RISK_PARTNER_RELATED_LABEL, help_text=Experiment.RISK_PARTNER_RELATED_HELP_TEXT, choices=RADIO_OPTIONS, widget=RadioWidget, + coerce=coerce_truthy, + empty_value=None, ) - risk_brand = forms.ChoiceField( + risk_brand = forms.TypedChoiceField( label=Experiment.RISK_BRAND_LABEL, help_text=Experiment.RISK_BRAND_HELP_TEXT, choices=RADIO_OPTIONS, widget=RadioWidget, + coerce=coerce_truthy, + empty_value=None, ) - risk_fast_shipped = forms.ChoiceField( + risk_fast_shipped = forms.TypedChoiceField( label=Experiment.RISK_FAST_SHIPPED_LABEL, help_text=Experiment.RISK_FAST_SHIPPED_HELP_TEXT, choices=RADIO_OPTIONS, widget=RadioWidget, + coerce=coerce_truthy, + empty_value=None, ) - risk_confidential = forms.ChoiceField( + risk_confidential = forms.TypedChoiceField( label=Experiment.RISK_CONFIDENTIAL_LABEL, help_text=Experiment.RISK_CONFIDENTIAL_HELP_TEXT, choices=RADIO_OPTIONS, widget=RadioWidget, + coerce=coerce_truthy, + empty_value=None, ) - risk_release_population = forms.ChoiceField( + risk_release_population = forms.TypedChoiceField( label=Experiment.RISK_RELEASE_POPULATION_LABEL, help_text=Experiment.RISK_RELEASE_POPULATION_HELP_TEXT, choices=RADIO_OPTIONS, widget=RadioWidget, + coerce=coerce_truthy, + empty_value=None, ) - risk_technical = forms.ChoiceField( + risk_technical = forms.TypedChoiceField( label=Experiment.RISK_TECHNICAL_LABEL, help_text=Experiment.RISK_TECHNICAL_HELP_TEXT, choices=RADIO_OPTIONS, widget=RadioWidget, + coerce=coerce_truthy, + empty_value=None, ) # Optional Risk Descriptions @@ -690,6 +710,27 @@ class Meta: "qa_status", ) + def clean(self): + cleaned_data = super().clean() + if ( + "risk_technical" in cleaned_data + and "risk_technical_description" in cleaned_data + ): + # Both checked, now we need to do an invariance check on these + # two. This is to match what's done with jQuery in the form: + # the 'risk_technical_description' needs to be required + # if 'risk_technical' is ``True``. + if cleaned_data.get("risk_technical"): + if not cleaned_data["risk_technical_description"]: + msg = ( + f"This is required if " + f"'{Experiment.RISK_TECHNICAL_LABEL}' is true." + ) + raise forms.ValidationError( + {"risk_technical_description": msg} + ) + return cleaned_data + class ExperimentReviewForm( ExperimentConstants, ChangeLogMixin, forms.ModelForm diff --git a/app/experimenter/experiments/tests/test_forms.py b/app/experimenter/experiments/tests/test_forms.py index 702eec94dc..d2865b916c 100644 --- a/app/experimenter/experiments/tests/test_forms.py +++ b/app/experimenter/experiments/tests/test_forms.py @@ -1041,26 +1041,27 @@ def test_form_saves_objectives(self): class TestExperimentRisksForm(MockRequestMixin, TestCase): + valid_data = { + "risk_internal_only": True, + "risk_partner_related": True, + "risk_brand": True, + "risk_fast_shipped": True, + "risk_confidential": True, + "risk_release_population": True, + "risk_technical": True, + "risk_technical_description": "It's complicated", + "risks": "There are some risks", + "testing": "Always be sure to test!", + "test_builds": "Latest build", + "qa_status": "It ain't easy being green", + } + def test_form_saves_risks(self): created_experiment = ExperimentFactory.create_with_status( Experiment.STATUS_DRAFT ) - data = { - "risk_internal_only": True, - "risk_partner_related": True, - "risk_brand": True, - "risk_fast_shipped": True, - "risk_confidential": True, - "risk_release_population": True, - "risk_technical": True, - "risk_technical_description": "It's complicated", - "risks": "There are some risks", - "testing": "Always be sure to test!", - "test_builds": "Latest build", - "qa_status": "It ain't easy being green", - } - + data = self.valid_data.copy() form = ExperimentRisksForm( request=self.request, data=data, instance=created_experiment ) @@ -1083,6 +1084,34 @@ def test_form_saves_risks(self): self.assertEqual(experiment.test_builds, data["test_builds"]) self.assertEqual(experiment.qa_status, data["qa_status"]) + def test_risk_technical_description_empty(self): + created_experiment = ExperimentFactory.create_with_status( + Experiment.STATUS_DRAFT + ) + + data = self.valid_data.copy() + data["risk_technical_description"] = "" + + form = ExperimentRisksForm( + request=self.request, data=data, instance=created_experiment + ) + self.assertFalse(form.is_valid()) + self.assertIn("risk_technical_description", form.errors) + + def test_risk_technical_description_empty_not_risk_technical(self): + created_experiment = ExperimentFactory.create_with_status( + Experiment.STATUS_DRAFT + ) + + data = self.valid_data.copy() + data["risk_technical"] = False + data["risk_technical_description"] = "" + + form = ExperimentRisksForm( + request=self.request, data=data, instance=created_experiment + ) + self.assertTrue(form.is_valid()) + class TestExperimentReviewForm( MockRequestMixin, MockBugzillaMixin, MockTasksMixin, TestCase