diff --git a/app/experimenter/experiments/forms.py b/app/experimenter/experiments/forms.py index 8031777959..91cfd6fec3 100644 --- a/app/experimenter/experiments/forms.py +++ b/app/experimenter/experiments/forms.py @@ -267,9 +267,11 @@ def clean(self): form._errors["name"] = ["All branches must have a unique name"] -class ExperimentVariantsPrefFormSet(BaseInlineFormSet): +class ExperimentVariantsPrefFormSet(ExperimentVariantsFormSet): def clean(self): + super().clean() + alive_forms = [ form for form in self.forms diff --git a/app/experimenter/experiments/tests/test_forms.py b/app/experimenter/experiments/tests/test_forms.py index c5fb2d808b..8955d607b6 100644 --- a/app/experimenter/experiments/tests/test_forms.py +++ b/app/experimenter/experiments/tests/test_forms.py @@ -4,9 +4,9 @@ from django import forms from django.core.exceptions import ValidationError -from django.forms import inlineformset_factory from django.test import TestCase from django.utils import timezone +from parameterized import parameterized_class from experimenter.experiments.forms import ( BugzillaURLField, @@ -21,7 +21,6 @@ ExperimentVariantAddonForm, ExperimentVariantPrefForm, ExperimentVariantsAddonForm, - ExperimentVariantsFormSet, ExperimentVariantsPrefForm, JSONField, ) @@ -256,17 +255,27 @@ def test_empty_slug_raises_error(self): self.assertFalse(form.is_valid()) -class TestExperimentVariantsFormSet(TestCase): +@parameterized_class( + ["form_class"], + [[ExperimentVariantsAddonForm], [ExperimentVariantsPrefForm]], +) +class TestExperimentVariantsFormSet(MockRequestMixin, TestCase): def setUp(self): - self.FormSet = inlineformset_factory( - formset=ExperimentVariantsFormSet, - model=ExperimentVariant, - parent_model=Experiment, - fields=["is_control", "ratio", "name", "slug", "description"], + super().setUp() + + self.experiment = ExperimentFactory.create_with_status( + Experiment.STATUS_DRAFT, num_variants=0 ) self.data = { + "population_percent": "10", + "firefox_version": Experiment.VERSION_CHOICES[-1][0], + "firefox_channel": Experiment.CHANNEL_NIGHTLY, + "client_matching": "en-us only please", + "pref_key": "browser.test.example", + "pref_type": Experiment.PREF_TYPE_STR, + "pref_branch": Experiment.PREF_BRANCH_DEFAULT, "variants-TOTAL_FORMS": "3", "variants-INITIAL_FORMS": "0", "variants-MIN_NUM_FORMS": "0", @@ -274,52 +283,69 @@ def setUp(self): "variants-0-is_control": True, "variants-0-ratio": "34", "variants-0-name": "control name", - "variants-0-slug": "control-slug", "variants-0-description": "control desc", + "variants-0-value": '"control value"', "variants-1-is_control": False, "variants-1-ratio": "33", "variants-1-name": "branch 1 name", - "variants-1-slug": "branch-1-slug", "variants-1-description": "branch 1 desc", + "variants-1-value": '"branch 1 value"', "variants-2-is_control": False, "variants-2-ratio": "33", "variants-2-name": "branch 2 name", - "variants-2-slug": "branch-2-slug", "variants-2-description": "branch 2 desc", + "variants-2-value": '"branch 2 value"', } - def test_formset_valid_if_sizes_sum_to_100(self): - formset = self.FormSet(data=self.data) - self.assertTrue(formset.is_valid()) + def test_form_valid_if_sizes_sum_to_100(self): + self.data["variants-0-ratio"] = "34" + self.data["variants-1-ratio"] = "33" + self.data["variants-2-ratio"] = "33" + form = self.form_class( + request=self.request, data=self.data, instance=self.experiment + ) + self.assertTrue(form.is_valid()) - def test_formset_invalid_if_sizes_sum_to_less_than_100(self): - self.data["variants-0-ratio"] = 30 - formset = self.FormSet(data=self.data) - self.assertFalse(formset.is_valid()) + def test_form_invalid_if_sizes_sum_to_less_than_100(self): + self.data["variants-0-ratio"] = "33" + self.data["variants-1-ratio"] = "33" + self.data["variants-2-ratio"] = "33" + form = self.form_class( + request=self.request, data=self.data, instance=self.experiment + ) + self.assertFalse(form.is_valid()) - for form in formset.forms: + for form in form.variants_formset.forms: self.assertIn("ratio", form.errors) - def test_formset_invalid_if_sizes_sum_to_more_than_100(self): - self.data["variants-0-ratio"] = 40 - formset = self.FormSet(data=self.data) - self.assertFalse(formset.is_valid()) + def test_form_invalid_if_sizes_sum_to_more_than_100(self): + self.data["variants-0-ratio"] = "35" + self.data["variants-1-ratio"] = "33" + self.data["variants-2-ratio"] = "33" + form = self.form_class( + request=self.request, data=self.data, instance=self.experiment + ) + self.assertFalse(form.is_valid()) - for form in formset.forms: + for form in form.variants_formset.forms: self.assertIn("ratio", form.errors) - def test_formset_invalid_if_duplicate_slugs_appear(self): - self.data["variants-0-slug"] = self.data["variants-1-slug"] - formset = self.FormSet(data=self.data) - self.assertFalse(formset.is_valid()) + def test_form_invalid_if_duplicate_names_appear(self): + self.data["variants-0-name"] = self.data["variants-1-name"] + form = self.form_class( + request=self.request, data=self.data, instance=self.experiment + ) + self.assertFalse(form.is_valid()) - for form in formset.forms: + for form in form.variants_formset.forms: self.assertIn("name", form.errors) def test_empty_branch_size_raises_validation_error(self): del self.data["variants-0-ratio"] - formset = self.FormSet(data=self.data) - self.assertFalse(formset.is_valid()) + form = self.form_class( + request=self.request, data=self.data, instance=self.experiment + ) + self.assertFalse(form.is_valid()) class TestExperimentVariantsAddonForm(MockRequestMixin, TestCase): diff --git a/app/requirements/default.txt b/app/requirements/default.txt index 0498bcea08..e38836ae23 100644 --- a/app/requirements/default.txt +++ b/app/requirements/default.txt @@ -138,3 +138,6 @@ requests[security]==2.20.0 \ whitenoise==4.1.2 \ --hash=sha256:118ab3e5f815d380171b100b05b76de2a07612f422368a201a9ffdeefb2251c1 \ --hash=sha256:42133ddd5229eeb6a0c9899496bdbe56c292394bf8666da77deeb27454c0456a +parameterized==0.7.0 \ + --hash=sha256:020343a281efcfe9b71b9028a91817f981202c14d72104b5a2fbe401dee25a18 \ + --hash=sha256:d8c8837fb677ed2d5a93b9e2308ce0da3aeb58cf513120d501e0b7af14da78d5