From a873a59c10ce069a352b584c8c23e9a6b55e0a8d Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Wed, 4 Mar 2020 13:43:31 -0800 Subject: [PATCH 1/5] removed unused design forms fixes #1808 (#2351) --- app/experimenter/experiments/forms.py | 264 +----- .../experiments/tests/test_forms.py | 815 +----------------- 2 files changed, 3 insertions(+), 1076 deletions(-) diff --git a/app/experimenter/experiments/forms.py b/app/experimenter/experiments/forms.py index a643d70e11..c7cc297609 100644 --- a/app/experimenter/experiments/forms.py +++ b/app/experimenter/experiments/forms.py @@ -5,9 +5,6 @@ from django.conf import settings from django.contrib import messages from django.contrib.auth import get_user_model -from django.db import transaction -from django.forms import BaseInlineFormSet -from django.forms import inlineformset_factory from django.forms.models import ModelChoiceIterator from django.utils import timezone from django.utils.html import strip_tags @@ -19,11 +16,7 @@ from experimenter.experiments import tasks from experimenter.experiments.changelog_utils import generate_change_log from experimenter.experiments.constants import ExperimentConstants -from experimenter.experiments.models import ( - Experiment, - ExperimentComment, - ExperimentVariant, -) +from experimenter.experiments.models import Experiment, ExperimentComment from experimenter.experiments.serializers.entities import ChangeLogSerializer from experimenter.notifications.models import Notification @@ -221,122 +214,6 @@ def clean(self): return cleaned_data -class ExperimentVariantGenericForm(forms.ModelForm): - - experiment = forms.ModelChoiceField(queryset=Experiment.objects.all(), required=False) - is_control = forms.BooleanField(required=False) - ratio = forms.IntegerField( - label="Branch Size", - initial="50", - min_value=1, - max_value=100, - help_text=Experiment.CONTROL_RATIO_HELP_TEXT, - widget=forms.TextInput(attrs={"class": "form-control"}), - ) - name = forms.CharField( - label="Name", - help_text=Experiment.CONTROL_NAME_HELP_TEXT, - widget=forms.TextInput(attrs={"class": "form-control"}), - ) - description = forms.CharField( - label="Description", - help_text=Experiment.CONTROL_DESCRIPTION_HELP_TEXT, - widget=forms.Textarea(attrs={"class": "form-control", "rows": 3}), - ) - slug = forms.CharField(required=False) - - class Meta: - model = ExperimentVariant - fields = ["description", "experiment", "is_control", "name", "ratio", "slug"] - - def clean_name(self): - name = self.cleaned_data["name"] - slug = slugify(name) - - if not slug: - raise forms.ValidationError( - "This name must include non-punctuation characters." - ) - - return name - - def clean(self): - cleaned_data = super().clean() - - name = cleaned_data.get("name") - cleaned_data["slug"] = slugify(name) - - return cleaned_data - - -class ExperimentVariantPrefForm(ExperimentVariantGenericForm): - - value = forms.CharField( - label="Pref Value", - help_text=Experiment.CONTROL_VALUE_HELP_TEXT, - widget=forms.TextInput(attrs={"class": "form-control"}), - ) - - class Meta: - model = ExperimentVariant - fields = [ - "description", - "experiment", - "is_control", - "name", - "ratio", - "slug", - "value", - ] - - -class ExperimentVariantsFormSet(BaseInlineFormSet): - - def clean(self): - alive_forms = [form for form in self.forms if not form.cleaned_data["DELETE"]] - - total_percentage = sum( - [form.cleaned_data.get("ratio", 0) for form in alive_forms] - ) - - if total_percentage != 100: - for form in alive_forms: - form._errors["ratio"] = ["The size of all branches must add up to 100"] - - if all([f.is_valid() for f in alive_forms]): - unique_names = set( - form.cleaned_data["name"] - for form in alive_forms - if form.cleaned_data.get("name") - ) - - if not len(unique_names) == len(alive_forms): - for form in alive_forms: - form._errors["name"] = ["All branches must have a unique name"] - - -class ExperimentVariantsPrefFormSet(ExperimentVariantsFormSet): - - def clean(self): - super().clean() - - alive_forms = [ - form - for form in self.forms - if form.is_valid() and not form.cleaned_data["DELETE"] - ] - - forms_by_value = {} - for form in alive_forms: - value = form.cleaned_data["value"] - forms_by_value.setdefault(value, []).append(form) - - for dupe_forms in forms_by_value.values(): - if len(dupe_forms) > 1: - for form in dupe_forms: - form.add_error("value", "All branches must have a unique pref value") - - class CustomModelChoiceIterator(ModelChoiceIterator): def __iter__(self): @@ -551,145 +428,6 @@ def save(self, *args, **kwargs): return experiment -class ExperimentDesignBaseForm(ChangeLogMixin, forms.ModelForm): - - class Meta: - model = Experiment - fields = [] - - def __init__(self, *args, **kwargs): - data = kwargs.pop("data", None) - instance = kwargs.pop("instance", None) - - extra = 0 - if instance and instance.variants.count() == 0: - extra = 2 - - FormSet = inlineformset_factory( - can_delete=True, - extra=extra, - form=self.FORMSET_FORM_CLASS, - formset=self.FORMSET_CLASS, - model=ExperimentVariant, - parent_model=Experiment, - ) - - self.variants_formset = FormSet(data=data, instance=instance) - super().__init__(data=data, instance=instance, *args, **kwargs) - - def is_valid(self): - return super().is_valid() and self.variants_formset.is_valid() - - @transaction.atomic - def save(self, *args, **kwargs): - self.variants_formset.save() - return super().save(*args, **kwargs) - - -class ExperimentDesignGenericForm(ExperimentDesignBaseForm): - - FORMSET_FORM_CLASS = ExperimentVariantGenericForm - FORMSET_CLASS = ExperimentVariantsFormSet - - design = forms.CharField( - required=False, - label="Design", - help_text=Experiment.DESIGN_HELP_TEXT, - widget=forms.Textarea(attrs={"class": "form-control", "rows": 10}), - ) - - class Meta: - model = Experiment - fields = ["design"] - - -class ExperimentDesignAddonForm(ExperimentDesignBaseForm): - - FORMSET_FORM_CLASS = ExperimentVariantGenericForm - FORMSET_CLASS = ExperimentVariantsFormSet - - addon_experiment_id = forms.CharField( - empty_value=None, - max_length=settings.NORMANDY_SLUG_MAX_LEN, - required=False, - label="Active Experiment Name", - help_text=Experiment.ADDON_EXPERIMENT_ID_HELP_TEXT, - ) - addon_release_url = forms.URLField( - required=False, - label="Signed Release URL", - help_text=Experiment.ADDON_RELEASE_URL_HELP_TEXT, - ) - - class Meta: - model = Experiment - fields = ExperimentDesignBaseForm.Meta.fields + [ - "addon_experiment_id", - "addon_release_url", - ] - - -class ExperimentDesignPrefForm(ExperimentDesignBaseForm): - - FORMSET_FORM_CLASS = ExperimentVariantPrefForm - FORMSET_CLASS = ExperimentVariantsPrefFormSet - - pref_name = forms.CharField( - label="Pref Name", - help_text=Experiment.PREF_NAME_HELP_TEXT, - widget=forms.TextInput(attrs={"class": "form-control"}), - ) - pref_type = forms.ChoiceField( - label="Pref Type", - help_text=Experiment.PREF_TYPE_HELP_TEXT, - choices=Experiment.PREF_TYPE_CHOICES, - widget=forms.Select(attrs={"class": "form-control"}), - ) - pref_branch = forms.ChoiceField( - label="Pref Branch", - help_text=Experiment.PREF_BRANCH_HELP_TEXT, - choices=Experiment.PREF_BRANCH_CHOICES, - widget=forms.Select(attrs={"class": "form-control"}), - ) - - class Meta: - model = Experiment - fields = ExperimentDesignBaseForm.Meta.fields + [ - "pref_name", - "pref_type", - "pref_branch", - ] - - def clean(self): - cleaned_data = super().clean() - expected_type_mapping = expected_type = { - Experiment.PREF_TYPE_BOOL: bool, - Experiment.PREF_TYPE_INT: int, - Experiment.PREF_TYPE_STR: str, - } - - # Check that each pref value matches the global pref type of the form. - pref_type = cleaned_data["pref_type"] - expected_type = expected_type_mapping.get(pref_type, None) - if pref_type != Experiment.PREF_TYPE_STR: - - for form in self.variants_formset.forms: - try: - if form.is_valid(): - found_type = type(json.loads(form.cleaned_data["value"])) - - # type validation only for non json type - if expected_type and found_type != expected_type: - raise ValueError - - except (json.JSONDecodeError, ValueError): - form.add_error( - "value", f"Unexpected value type (should be {pref_type})" - ) - - return cleaned_data - - class RadioWidget(forms.widgets.RadioSelect): template_name = "experiments/radio_widget.html" diff --git a/app/experimenter/experiments/tests/test_forms.py b/app/experimenter/experiments/tests/test_forms.py index 2a804a93b6..1d861d4fbd 100644 --- a/app/experimenter/experiments/tests/test_forms.py +++ b/app/experimenter/experiments/tests/test_forms.py @@ -7,9 +7,7 @@ from django.core.exceptions import ValidationError from django.test import TestCase, override_settings from django.utils import timezone -from django.utils.text import slugify from faker import Factory as FakerFactory -from parameterized import parameterized_class from django.contrib.auth.models import Permission from django.contrib.contenttypes.models import ContentType @@ -25,29 +23,15 @@ ExperimentRisksForm, ExperimentStatusForm, ExperimentSubscribedForm, - ExperimentVariantGenericForm, - ExperimentVariantPrefForm, - ExperimentDesignGenericForm, - ExperimentDesignAddonForm, - ExperimentDesignPrefForm, ExperimentResultsForm, JSONField, NormandyIdForm, ExperimentTimelinePopulationForm, ExperimentOrderingForm, ) -from experimenter.experiments.models import ( - Experiment, - ExperimentVariant, - ExperimentChangeLog, -) +from experimenter.experiments.models import Experiment from experimenter.base.tests.factories import CountryFactory, LocaleFactory -from experimenter.experiments.tests.factories import ( - ExperimentFactory, - UserFactory, - ExperimentVariantFactory, - VariantPreferencesFactory, -) +from experimenter.experiments.tests.factories import ExperimentFactory, UserFactory from experimenter.bugzilla.tests.mixins import MockBugzillaMixin from experimenter.experiments.tests.mixins import MockTasksMixin, MockRequestMixin @@ -96,94 +80,6 @@ def test_rejects_non_bugzilla_url(self): field.clean("www.example.com") -class TestExperimentVariantGenericForm(TestCase): - - def setUp(self): - self.experiment = ExperimentFactory.create() - self.data = { - "description": "Its the control! So controlly.", - "experiment": self.experiment.id, - "is_control": True, - "name": "The Control Variant", - "ratio": 50, - } - - def test_form_creates_variant(self): - form = ExperimentVariantGenericForm(self.data) - - self.assertTrue(form.is_valid()) - - saved_variant = form.save() - variant = ExperimentVariant.objects.get(id=saved_variant.id) - - self.assertEqual(variant.experiment.id, self.experiment.id) - self.assertTrue(variant.is_control) - self.assertEqual(variant.description, self.data["description"]) - self.assertEqual(variant.name, self.data["name"]) - self.assertEqual(variant.ratio, self.data["ratio"]) - self.assertEqual(variant.slug, "the-control-variant") - - def test_ratio_must_be_greater_than_0(self): - self.data["ratio"] = 0 - form = ExperimentVariantGenericForm(self.data) - - self.assertFalse(form.is_valid()) - self.assertIn("ratio", form.errors) - - def test_ratio_must_be_less_than_or_equal_to_100(self): - self.data["ratio"] = 101 - form = ExperimentVariantGenericForm(self.data) - - self.assertFalse(form.is_valid()) - self.assertIn("ratio", form.errors) - - def test_checks_empty_slug(self): - self.data["name"] = "!" - form = ExperimentVariantGenericForm(self.data) - - self.assertFalse(form.is_valid()) - self.assertIn("name", form.errors) - - def test_updates_variant_slug(self): - variant = ExperimentVariantFactory.create(slug="the-treatment-variant") - form = ExperimentVariantGenericForm(self.data, instance=variant) - - self.assertTrue(form.is_valid()) - - saved_variant = form.save() - self.assertEqual(saved_variant.slug, "the-control-variant") - - -class TestExperimentVariantPrefForm(TestCase): - - def test_form_creates_variant(self): - experiment = ExperimentFactory.create() - - data = { - "description": "Its the control! So controlly.", - "experiment": experiment.id, - "is_control": True, - "name": "The Control Variant", - "ratio": 50, - "value": "true", - } - - form = ExperimentVariantPrefForm(data) - - self.assertTrue(form.is_valid()) - - saved_variant = form.save() - variant = ExperimentVariant.objects.get(id=saved_variant.id) - - self.assertEqual(variant.experiment.id, experiment.id) - self.assertTrue(variant.is_control) - self.assertEqual(variant.description, data["description"]) - self.assertEqual(variant.name, data["name"]) - self.assertEqual(variant.ratio, data["ratio"]) - self.assertEqual(variant.slug, "the-control-variant") - self.assertEqual(variant.value, "true") - - class TestChangeLogMixin(MockRequestMixin, TestCase): def test_mixin_creates_change_log_with_request_user_on_save(self): @@ -337,149 +233,6 @@ def test_changelog_values(self): self.assertEqual(expected_data, latest_changes.changed_values) - def test_changelog_values_with_prev_log(self): - experiment = ExperimentFactory.create_with_variants( - type=Experiment.TYPE_PREF, - num_variants=0, - pref_type=Experiment.PREF_TYPE_INT, - pref_branch=Experiment.PREF_BRANCH_DEFAULT, - ) - addon_url = "https://www.example.com/old-branch-1-name-release.xpi" - - variant = ExperimentVariantFactory( - name="old branch 1 name", - slug="old-branch-1-name", - ratio=50, - value=8, - is_control=True, - description="old branch 1 desc", - experiment=experiment, - addon_release_url=addon_url, - ) - - VariantPreferencesFactory.create( - variant=variant, - pref_name="p1", - pref_type=Experiment.PREF_TYPE_INT, - pref_branch=Experiment.PREF_BRANCH_DEFAULT, - pref_value=5, - ) - - changed_variant_pref_value = { - "pref_name": "p1", - "pref_type": "integer", - "pref_branch": "default", - "pref_value": "5", - } - changed_values = { - "variants": { - "old_value": None, - "new_value": [ - { - "name": "old branch 1 name", - "slug": "old-branch-1-name", - "ratio": 50, - "value": "8", - "is_control": True, - "description": "old branch 1 desc", - "addon_release_url": addon_url, - "preferences": [changed_variant_pref_value], - } - ], - "display_name": "Branches", - } - } - - ExperimentChangeLog.objects.create( - experiment=experiment, - changed_by=self.request.user, - old_status=Experiment.STATUS_ACCEPTED, - new_status=Experiment.STATUS_DRAFT, - changed_values=changed_values, - message="", - ) - - data = { - "pref_name": experiment.pref_name, - "pref_type": Experiment.PREF_TYPE_INT, - "pref_branch": Experiment.PREF_BRANCH_DEFAULT, - "addon_experiment_id": experiment.addon_experiment_id, - "addon_release_url": experiment.addon_release_url, - "variants-TOTAL_FORMS": "2", - "variants-INITIAL_FORMS": "0", - "variants-MIN_NUM_FORMS": "0", - "variants-MAX_NUM_FORMS": "1000", - "variants-0-is_control": True, - "variants-0-ratio": "50", - "variants-0-name": "variant 0 name", - "variants-0-description": "variant 0 desc", - "variants-0-value": 5, - "variants-1-is_control": False, - "variants-1-ratio": "50", - "variants-1-name": "branch 1 name", - "variants-1-description": "branch 1 desc", - "variants-1-value": 8, - } - - form = ExperimentDesignPrefForm( - request=self.request, data=data, instance=experiment - ) - self.assertTrue(form.is_valid()) - experiment = form.save() - latest_changes = experiment.changes.latest() - - old_value = [ - { - "description": "old branch 1 desc", - "is_control": True, - "name": "old branch 1 name", - "ratio": 50, - "slug": "old-branch-1-name", - "value": "8", - "addon_release_url": addon_url, - "preferences": [changed_variant_pref_value], - } - ] - - new_value = [ - { - "description": "branch 1 desc", - "is_control": False, - "name": "branch 1 name", - "ratio": 50, - "slug": "branch-1-name", - "value": "8", - "addon_release_url": None, - "preferences": [], - }, - { - "description": "variant 0 desc", - "is_control": True, - "name": "variant 0 name", - "ratio": 50, - "slug": "variant-0-name", - "value": "5", - "addon_release_url": None, - "preferences": [], - }, - { - "description": "old branch 1 desc", - "is_control": True, - "name": "old branch 1 name", - "ratio": 50, - "slug": "old-branch-1-name", - "value": "8", - "addon_release_url": addon_url, - "preferences": [changed_variant_pref_value], - }, - ] - - variant_changes = latest_changes.changed_values["variants"] - - self.assertEqual(variant_changes["display_name"], "Branches") - self.assertEqual(variant_changes["old_value"], old_value) - self.assertCountEqual(variant_changes["new_value"], new_value) - @override_settings(BUGZILLA_HOST="https://bugzilla.mozilla.org") class TestExperimentOverviewForm(MockRequestMixin, TestCase): @@ -925,570 +678,6 @@ def test_form_is_valid_if_firefox_max_is_equal_to_min(self): self.assertTrue(form.is_valid()) -@parameterized_class( - ["form_class"], - [ - [ExperimentDesignGenericForm], - [ExperimentDesignAddonForm], - [ExperimentDesignPrefForm], - ], -) -class TestExperimentVariantFormSet(MockRequestMixin, TestCase): - - def setUp(self): - super().setUp() - - self.experiment = ExperimentFactory.create_with_status( - Experiment.STATUS_DRAFT, num_variants=0 - ) - - self.data = get_design_form_data() - - def test_formset_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"] = "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 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"] = "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 form.variants_formset.forms: - self.assertIn("ratio", form.errors) - - def test_formset_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 form.variants_formset.forms: - self.assertIn("name", form.errors) - - def test_empty_branch_size_raises_validation_error(self): - del self.data["variants-0-ratio"] - form = self.form_class( - request=self.request, data=self.data, instance=self.experiment - ) - self.assertFalse(form.is_valid()) - - -@parameterized_class( - ["form_class"], - [ - [ExperimentDesignGenericForm], - [ExperimentDesignAddonForm], - [ExperimentDesignPrefForm], - ], -) -class TestExperimentDesignBaseForm(MockRequestMixin, TestCase): - - def setUp(self): - super().setUp() - - self.experiment = ExperimentFactory.create_with_status( - Experiment.STATUS_DRAFT, num_variants=0, countries=[], locales=[] - ) - - self.data = get_design_form_data() - - def test_formset_saves_new_variants(self): - form = self.form_class( - request=self.request, data=self.data, instance=self.experiment - ) - - self.assertTrue(form.is_valid()) - - self.assertEqual(self.experiment.variants.count(), 0) - experiment = form.save() - - self.assertEqual(experiment.variants.count(), 3) - - branch0 = experiment.variants.get(name=self.data["variants-0-name"]) - self.assertTrue(branch0.is_control) - self.assertTrue(branch0.slug, "control-name") - self.assertEqual(branch0.ratio, 34) - self.assertEqual(branch0.description, self.data["variants-0-description"]) - - branch1 = experiment.variants.get(name=self.data["variants-1-name"]) - self.assertFalse(branch1.is_control) - self.assertEqual(branch1.slug, "branch-1-name") - self.assertEqual(branch1.ratio, 33) - self.assertEqual(branch1.description, self.data["variants-1-description"]) - - branch2 = experiment.variants.get(name=self.data["variants-2-name"]) - self.assertFalse(branch2.is_control) - self.assertEqual(branch2.slug, "branch-2-name") - self.assertEqual(branch2.ratio, 33) - self.assertEqual(branch2.description, self.data["variants-2-description"]) - - def test_formset_edits_existing_variants(self): - form = self.form_class( - request=self.request, data=self.data, instance=self.experiment - ) - - self.assertTrue(form.is_valid()) - - self.assertEqual(self.experiment.variants.count(), 0) - - experiment = form.save() - - self.assertEqual(experiment.variants.count(), 3) - - self.data["variants-INITIAL_FORMS"] = 3 - - branch0 = experiment.variants.get(name=self.data["variants-0-name"]) - self.data["variants-0-id"] = branch0.id - self.data["variants-0-DELETE"] = False - self.data["variants-0-name"] = "new branch 0 name" - self.data["variants-0-description"] = "new branch 0 description" - - branch1 = experiment.variants.get(name=self.data["variants-1-name"]) - self.data["variants-1-id"] = branch1.id - self.data["variants-1-DELETE"] = False - self.data["variants-1-name"] = "new branch 1 name" - self.data["variants-1-description"] = "new branch 1 description" - - branch2 = experiment.variants.get(name=self.data["variants-2-name"]) - self.data["variants-2-id"] = branch2.id - self.data["variants-2-DELETE"] = False - self.data["variants-2-name"] = "new branch 2 name" - self.data["variants-2-description"] = "new branch 2 description" - - form = self.form_class(request=self.request, data=self.data, instance=experiment) - - self.assertTrue(form.is_valid()) - - experiment = form.save() - - self.assertEqual(experiment.variants.count(), 3) - - branch0 = experiment.variants.get(name=self.data["variants-0-name"]) - self.assertTrue(branch0.is_control) - self.assertEqual(branch0.ratio, 34) - self.assertEqual(branch0.description, self.data["variants-0-description"]) - - branch1 = experiment.variants.get(name=self.data["variants-1-name"]) - self.assertFalse(branch1.is_control) - self.assertEqual(branch1.ratio, 33) - self.assertEqual(branch1.description, self.data["variants-1-description"]) - - branch2 = experiment.variants.get(name=self.data["variants-2-name"]) - self.assertFalse(branch2.is_control) - self.assertEqual(branch2.ratio, 33) - self.assertEqual(branch2.description, self.data["variants-2-description"]) - - def test_formset_adds_new_variant(self): - form = self.form_class( - request=self.request, data=self.data, instance=self.experiment - ) - - self.assertTrue(form.is_valid()) - - self.assertEqual(self.experiment.variants.count(), 0) - - experiment = form.save() - - self.assertEqual(experiment.variants.count(), 3) - - self.data["variants-INITIAL_FORMS"] = 3 - self.data["variants-TOTAL_FORMS"] = 4 - - branch0 = experiment.variants.get(name=self.data["variants-0-name"]) - self.data["variants-0-id"] = branch0.id - self.data["variants-0-ratio"] = 25 - - branch1 = experiment.variants.get(name=self.data["variants-1-name"]) - self.data["variants-1-id"] = branch1.id - self.data["variants-1-ratio"] = 25 - - branch2 = experiment.variants.get(name=self.data["variants-2-name"]) - self.data["variants-2-id"] = branch2.id - self.data["variants-2-ratio"] = 25 - - self.data["variants-3-DELETE"] = False - self.data["variants-3-name"] = "new branch 0 name" - self.data["variants-3-description"] = "new branch 0 description" - self.data["variants-3-ratio"] = 25 - self.data["variants-3-value"] = '"new branch 3 value"' - - form = self.form_class(request=self.request, data=self.data, instance=experiment) - - self.assertTrue(form.is_valid()) - - experiment = form.save() - - self.assertEqual(experiment.variants.count(), 4) - - branch0 = experiment.variants.get(name=self.data["variants-0-name"]) - self.assertEqual(branch0.ratio, 25) - - branch1 = experiment.variants.get(name=self.data["variants-1-name"]) - self.assertEqual(branch1.ratio, 25) - - branch2 = experiment.variants.get(name=self.data["variants-2-name"]) - self.assertEqual(branch2.ratio, 25) - - branch3 = experiment.variants.get(name=self.data["variants-3-name"]) - self.assertEqual(branch3.ratio, 25) - self.assertEqual(branch3.description, self.data["variants-3-description"]) - - def test_formset_removes_variant(self): - form = self.form_class( - request=self.request, data=self.data, instance=self.experiment - ) - - self.assertTrue(form.is_valid()) - - self.assertEqual(self.experiment.variants.count(), 0) - - experiment = form.save() - - self.assertEqual(experiment.variants.count(), 3) - - self.data["variants-INITIAL_FORMS"] = 3 - self.data["variants-TOTAL_FORMS"] = 3 - - branch0 = experiment.variants.get(name=self.data["variants-0-name"]) - self.data["variants-0-id"] = branch0.id - self.data["variants-0-ratio"] = 50 - - branch1 = experiment.variants.get(name=self.data["variants-1-name"]) - self.data["variants-1-id"] = branch1.id - self.data["variants-1-DELETE"] = True - - branch2 = experiment.variants.get(name=self.data["variants-2-name"]) - self.data["variants-2-id"] = branch2.id - self.data["variants-2-ratio"] = 50 - - form = self.form_class(request=self.request, data=self.data, instance=experiment) - - self.assertTrue(form.is_valid()) - - experiment = form.save() - - self.assertEqual(experiment.variants.count(), 2) - - self.assertTrue( - experiment.variants.filter(name=self.data["variants-0-name"]).exists() - ) - self.assertFalse( - experiment.variants.filter(name=self.data["variants-1-name"]).exists() - ) - self.assertTrue( - experiment.variants.filter(name=self.data["variants-2-name"]).exists() - ) - - def test_formset_checks_uniqueness_for_single_experiment_not_all(self): - # We want to make sure that all the branches in a single - # experiment have unique names/slugs but not that they're - # unique across all experiments. This behaviour was accidentally - # introduced in 30c210183cd1568850d54132633b0f23e3e56c98 - # so I'm adding a test for it. - experiment1 = ExperimentFactory.create_with_status( - Experiment.STATUS_DRAFT, num_variants=0 - ) - self.data["addon_experiment_id"] = "addon-experiment-1" - form1 = self.form_class( - request=self.request, data=self.data, instance=experiment1 - ) - self.assertTrue(form1.is_valid()) - experiment1 = form1.save() - self.assertEqual(experiment1.variants.count(), 3) - - experiment2 = ExperimentFactory.create_with_status( - Experiment.STATUS_DRAFT, num_variants=0 - ) - self.data["addon_experiment_id"] = "addon-experiment-2" - form2 = self.form_class( - request=self.request, data=self.data, instance=experiment2 - ) - self.assertTrue(form2.is_valid()) - experiment2 = form2.save() - self.assertEqual(experiment2.variants.count(), 3) - - -def get_variants_form_data(): - return { - "variants-TOTAL_FORMS": "3", - "variants-INITIAL_FORMS": "0", - "variants-MIN_NUM_FORMS": "0", - "variants-MAX_NUM_FORMS": "1000", - "variants-0-is_control": True, - "variants-0-ratio": "34", - "variants-0-name": "control name", - "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-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-description": "branch 2 desc", - "variants-2-value": '"branch 2 value"', - } - - -def get_design_form_data(): - data = get_variants_form_data() - data.update( - { - "pref_name": "browser.test.example", - "pref_type": Experiment.PREF_TYPE_STR, - "pref_branch": Experiment.PREF_BRANCH_DEFAULT, - "addon_experiment_id": slugify(faker.catch_phrase()), - "addon_release_url": "https://www.example.com/release.xpi", - "design": "Design", - } - ) - return data - - -class TestExperimentDesignGenericForm(MockRequestMixin, TestCase): - - def setUp(self): - super().setUp() - self.data = get_variants_form_data() - self.data.update({"design": "Design"}) - - def test_form_saves_design_information(self): - experiment = ExperimentFactory.create( - type=Experiment.TYPE_GENERIC, design=Experiment.DESIGN_DEFAULT - ) - - form = ExperimentDesignGenericForm( - request=self.request, data=self.data, instance=experiment - ) - - self.assertTrue(form.is_valid()) - - experiment = form.save() - - self.assertEqual(experiment.design, self.data["design"]) - - -class TestExperimentDesignAddonForm(MockRequestMixin, TestCase): - - def setUp(self): - super().setUp() - self.data = get_variants_form_data() - self.data.update( - { - "addon_experiment_id": slugify(faker.catch_phrase()), - "addon_release_url": "https://www.example.com/release.xpi", - } - ) - - def test_minimum_required_fields(self): - experiment = ExperimentFactory.create( - addon_experiment_id=None, addon_release_url=None - ) - - data = get_variants_form_data() - data["addon_release_url"] = "https://www.example.com/release.xpi" - - form = ExperimentDesignAddonForm( - request=self.request, data=data, instance=experiment - ) - - self.assertTrue(form.is_valid()) - - experiment = form.save() - - self.assertEqual(experiment.addon_release_url, self.data["addon_release_url"]) - - def test_addon_experiment_id_is_unique(self): - experiment1 = ExperimentFactory.create( - addon_experiment_id=None, addon_release_url=None - ) - - form = ExperimentDesignAddonForm( - request=self.request, data=self.data, instance=experiment1 - ) - self.assertTrue(form.is_valid()) - experiment1 = form.save() - - self.assertEqual( - experiment1.addon_experiment_id, self.data["addon_experiment_id"] - ) - - experiment2 = ExperimentFactory.create( - addon_experiment_id=None, addon_release_url=None - ) - - form = ExperimentDesignAddonForm( - request=self.request, data=self.data, instance=experiment2 - ) - self.assertFalse(form.is_valid()) - self.assertIn("addon_experiment_id", form.errors) - - def test_addon_experiment_id_is_within_normandy_slug_max_len(self): - experiment = ExperimentFactory.create( - addon_experiment_id=None, addon_release_url=None - ) - - self.data["addon_experiment_id"] = "-" * (settings.NORMANDY_SLUG_MAX_LEN + 1) - - form = ExperimentDesignAddonForm( - request=self.request, data=self.data, instance=experiment - ) - - self.assertFalse(form.is_valid()) - self.assertIn("addon_experiment_id", form.errors) - - def test_addon_experiment_id_allows_duplicate_empty_values(self): - ExperimentFactory.create(addon_experiment_id="") - experiment = ExperimentFactory.create(addon_experiment_id=None) - - self.data["addon_experiment_id"] = "" - - form = ExperimentDesignAddonForm( - request=self.request, data=self.data, instance=experiment - ) - - self.assertTrue(form.is_valid()) - - -class TestExperimentDesignPrefForm(MockRequestMixin, TestCase): - - def setUp(self): - super().setUp() - self.data = get_variants_form_data() - self.data.update( - { - "pref_name": "browser.test.example", - "pref_type": Experiment.PREF_TYPE_STR, - "pref_branch": Experiment.PREF_BRANCH_DEFAULT, - } - ) - - def test_minimum_required_fields(self): - experiment = ExperimentFactory.create( - pref_name=None, pref_type=None, pref_branch=None - ) - - form = ExperimentDesignPrefForm( - request=self.request, data=self.data, instance=experiment - ) - - self.assertTrue(form.is_valid()) - - experiment = form.save() - - self.assertEqual(experiment.pref_name, self.data["pref_name"]) - self.assertEqual(experiment.pref_type, self.data["pref_type"]) - self.assertEqual(experiment.pref_branch, self.data["pref_branch"]) - - def test_form_is_invalid_if_branches_have_duplicate_pref_values(self): - self.data["variants-0-value"] = self.data["variants-1-value"] - form = ExperimentDesignPrefForm(request=self.request, data=self.data) - self.assertFalse(form.is_valid()) - self.assertIn("value", form.variants_formset.errors[0]) - self.assertIn("value", form.variants_formset.errors[1]) - self.assertNotIn("value", form.variants_formset.errors[2]) - - def test_form_is_invalid_if_pref_value_do_not_match_pref_type(self): - self.data["pref_type"] = Experiment.PREF_TYPE_INT - self.data["variants-0-value"] = "hello" # str - self.data["variants-1-value"] = "true" # bool - self.data["variants-2-value"] = "5" # int - form = ExperimentDesignPrefForm(request=self.request, data=self.data) - self.assertFalse(form.is_valid()) - self.assertIn("value", form.variants_formset.errors[0]) - self.assertIn("value", form.variants_formset.errors[1]) - self.assertNotIn("value", form.variants_formset.errors[2]) - - def test_form_is_valid_if_pref_type_is_string(self): - self.data["variants-0-value"] = "abc" - form = ExperimentDesignPrefForm(request=self.request, data=self.data) - self.assertTrue(form.is_valid()) - self.assertNotIn("value", form.variants_formset.errors[0]) - self.assertNotIn("value", form.variants_formset.errors[1]) - self.assertNotIn("value", form.variants_formset.errors[2]) - - def test_form_is_valid_if_pref_type_is_bool(self): - - self.data["pref_type"] = Experiment.PREF_TYPE_BOOL - - # remove the extra variant for uniqueness bool constraint - self.data.pop("variants-2-value") - self.data.pop("variants-2-is_control") - self.data.pop("variants-2-ratio") - self.data.pop("variants-2-name") - self.data.pop("variants-2-description") - - # modify remaining values to accomodate for only two variants - self.data["variants-TOTAL_FORMS"] = 2 - self.data["variants-0-ratio"] = 40 - self.data["variants-1-ratio"] = 60 - self.data["variants-0-value"] = "true" - self.data["variants-1-value"] = "false" - - form = ExperimentDesignPrefForm(request=self.request, data=self.data) - - self.assertTrue(form.is_valid()) - self.assertNotIn("value", form.variants_formset.errors[0]) - self.assertNotIn("value", form.variants_formset.errors[1]) - - def test_form_is_valid_if_pref_type_is_int(self): - self.data["pref_type"] = Experiment.PREF_TYPE_INT - self.data["variants-0-value"] = "20" - self.data["variants-1-value"] = "55" - self.data["variants-2-value"] = "75" - form = ExperimentDesignPrefForm(request=self.request, data=self.data) - self.assertTrue(form.is_valid()) - self.assertNotIn("value", form.variants_formset.errors[0]) - self.assertNotIn("value", form.variants_formset.errors[1]) - self.assertNotIn("value", form.variants_formset.errors[2]) - - def test_form_is_valid_if_pref_type_is_json_string(self): - self.data["pref_type"] = Experiment.PREF_TYPE_JSON_STR - self.data["variants-0-value"] = "{}" - self.data["variants-1-value"] = "[1,2,3,4]" - self.data["variants-2-value"] = '{"variant":[1,2,3,4]}' - form = ExperimentDesignPrefForm(request=self.request, data=self.data) - self.assertTrue(form.is_valid()) - self.assertNotIn("value", form.variants_formset.errors[0]) - self.assertNotIn("value", form.variants_formset.errors[1]) - self.assertNotIn("value", form.variants_formset.errors[2]) - - def test_form_is_invalid_with_invalid_json(self): - self.data["pref_type"] = Experiment.PREF_TYPE_JSON_STR - self.data["variants-0-value"] = "{]" - self.data["variants-1-value"] = '{5: "something"}' - self.data["variants-2-value"] = "hi" - form = ExperimentDesignPrefForm(request=self.request, data=self.data) - self.assertFalse(form.is_valid()) - self.assertIn("value", form.variants_formset.errors[0]) - self.assertIn("value", form.variants_formset.errors[1]) - self.assertIn("value", form.variants_formset.errors[2]) - - class TestExperimentObjectivesForm(MockRequestMixin, TestCase): def test_no_fields_required(self): From 6a3dbc232bfce98e96f4a0e2792a64403f8be3cd Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Wed, 4 Mar 2020 13:58:28 -0800 Subject: [PATCH 2/5] separate public and private apis fixes #2250 (#2353) * separate public and private apis fixes #2250 * removed unused imports * added missing slash --- .../base/management/commands/generate-docs.py | 23 ++- app/experimenter/docs/openapi-schema.json | 145 ++++++++++++----- app/experimenter/docs/swagger-ui.html | 146 +++++++++++++----- .../{api_urls.py => private_api_urls.py} | 14 -- .../experiments/public_api_urls.py | 22 +++ app/experimenter/static/js/utils/api.js | 2 +- app/experimenter/urls.py | 5 +- 7 files changed, 262 insertions(+), 95 deletions(-) rename app/experimenter/experiments/{api_urls.py => private_api_urls.py} (79%) create mode 100644 app/experimenter/experiments/public_api_urls.py diff --git a/app/experimenter/base/management/commands/generate-docs.py b/app/experimenter/base/management/commands/generate-docs.py index 9655a6b9d7..62c5dac87c 100644 --- a/app/experimenter/base/management/commands/generate-docs.py +++ b/app/experimenter/base/management/commands/generate-docs.py @@ -1,13 +1,13 @@ -import io import os import logging import json from django.conf import settings -from django.core.management import call_command from django.template.loader import render_to_string from django.core.management.base import BaseCommand +from rest_framework.schemas.openapi import SchemaGenerator + logger = logging.getLogger() @@ -22,15 +22,22 @@ def handle(self, *args, **options): self.generate_docs(options) @staticmethod - def read_json_doc(): - output = io.StringIO() - call_command("generateschema", "--format=openapi-json", stdout=output) - output.seek(0) - return output.read() + def generateSchema(): + generator = SchemaGenerator(title="Experimenter API") + schema = generator.get_schema() + paths = schema["paths"] + for path in paths: + if "/api/v1/" in path: + for method in paths[path]: + paths[path][method]["tags"] = ["public"] + elif "/api/v2/" in path: + for method in paths[path]: + paths[path][method]["tags"] = ["private"] + return json.dumps(schema, indent=2) @staticmethod def generate_docs(options): - api_json = Command.read_json_doc() + api_json = Command.generateSchema() docs_dir = os.path.join(settings.BASE_DIR, "docs") schema_json_path = os.path.join(docs_dir, "openapi-schema.json") swagger_html_path = os.path.join(docs_dir, "swagger-ui.html") diff --git a/app/experimenter/docs/openapi-schema.json b/app/experimenter/docs/openapi-schema.json index 108dcdc1ca..33d4d0daa3 100644 --- a/app/experimenter/docs/openapi-schema.json +++ b/app/experimenter/docs/openapi-schema.json @@ -1,7 +1,7 @@ { "openapi": "3.0.2", "info": { - "title": "", + "title": "Experimenter API", "version": "" }, "paths": { @@ -59,7 +59,10 @@ }, "description": "" } - } + }, + "tags": [ + "public" + ] } }, "/api/v1/experiments/{slug}/": { @@ -457,7 +460,10 @@ }, "description": "" } - } + }, + "tags": [ + "public" + ] } }, "/api/v1/experiments/": { @@ -858,10 +864,13 @@ }, "description": "" } - } + }, + "tags": [ + "public" + ] } }, - "/api/v1/experiments/{slug}/design-addon": { + "/api/v2/experiments/{slug}/design-addon": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -931,7 +940,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -1153,7 +1165,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -1360,10 +1375,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-pref": { + "/api/v2/experiments/{slug}/design-pref": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -1445,7 +1463,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -1715,7 +1736,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -1964,10 +1988,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-multi-pref": { + "/api/v2/experiments/{slug}/design-multi-pref": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -2063,7 +2090,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -2389,7 +2419,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -2703,10 +2736,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-generic": { + "/api/v2/experiments/{slug}/design-generic": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -2769,7 +2805,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -2963,7 +3002,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -3148,10 +3190,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-branched-addon": { + "/api/v2/experiments/{slug}/design-branched-addon": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -3221,7 +3266,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -3443,7 +3491,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -3653,10 +3704,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-rollout": { + "/api/v2/experiments/{slug}/design-rollout": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -3722,7 +3776,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -3928,7 +3985,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -4125,10 +4185,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/intent-to-ship-email": { + "/api/v2/experiments/{slug}/intent-to-ship-email": { "put": { "operationId": "UpdateExperiment", "description": "", @@ -5625,7 +5688,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -7081,10 +7147,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/clone": { + "/api/v2/experiments/{slug}/clone": { "put": { "operationId": "UpdateExperiment", "description": "", @@ -7165,7 +7234,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -7238,8 +7310,11 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } } } -} +} \ No newline at end of file diff --git a/app/experimenter/docs/swagger-ui.html b/app/experimenter/docs/swagger-ui.html index de7edb8325..d7727b79da 100644 --- a/app/experimenter/docs/swagger-ui.html +++ b/app/experimenter/docs/swagger-ui.html @@ -13,7 +13,7 @@ const spec = { "openapi": "3.0.2", "info": { - "title": "", + "title": "Experimenter API", "version": "" }, "paths": { @@ -71,7 +71,10 @@ }, "description": "" } - } + }, + "tags": [ + "public" + ] } }, "/api/v1/experiments/{slug}/": { @@ -469,7 +472,10 @@ }, "description": "" } - } + }, + "tags": [ + "public" + ] } }, "/api/v1/experiments/": { @@ -870,10 +876,13 @@ }, "description": "" } - } + }, + "tags": [ + "public" + ] } }, - "/api/v1/experiments/{slug}/design-addon": { + "/api/v2/experiments/{slug}/design-addon": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -943,7 +952,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -1165,7 +1177,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -1372,10 +1387,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-pref": { + "/api/v2/experiments/{slug}/design-pref": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -1457,7 +1475,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -1727,7 +1748,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -1976,10 +2000,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-multi-pref": { + "/api/v2/experiments/{slug}/design-multi-pref": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -2075,7 +2102,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -2401,7 +2431,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -2715,10 +2748,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-generic": { + "/api/v2/experiments/{slug}/design-generic": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -2781,7 +2817,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -2975,7 +3014,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -3160,10 +3202,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-branched-addon": { + "/api/v2/experiments/{slug}/design-branched-addon": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -3233,7 +3278,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -3455,7 +3503,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -3665,10 +3716,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/design-rollout": { + "/api/v2/experiments/{slug}/design-rollout": { "get": { "operationId": "RetrieveExperiment", "description": "", @@ -3734,7 +3788,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "put": { "operationId": "UpdateExperiment", @@ -3940,7 +3997,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -4137,10 +4197,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/intent-to-ship-email": { + "/api/v2/experiments/{slug}/intent-to-ship-email": { "put": { "operationId": "UpdateExperiment", "description": "", @@ -5637,7 +5700,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -7093,10 +7159,13 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } }, - "/api/v1/experiments/{slug}/clone": { + "/api/v2/experiments/{slug}/clone": { "put": { "operationId": "UpdateExperiment", "description": "", @@ -7177,7 +7246,10 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] }, "patch": { "operationId": "PartialUpdateExperiment", @@ -7250,12 +7322,14 @@ }, "description": "" } - } + }, + "tags": [ + "private" + ] } } } -} -; +}; const ui = SwaggerUIBundle({ spec: spec, dom_id: "#swagger-ui", diff --git a/app/experimenter/experiments/api_urls.py b/app/experimenter/experiments/private_api_urls.py similarity index 79% rename from app/experimenter/experiments/api_urls.py rename to app/experimenter/experiments/private_api_urls.py index 9d5b573555..4850d42e56 100644 --- a/app/experimenter/experiments/api_urls.py +++ b/app/experimenter/experiments/private_api_urls.py @@ -8,9 +8,6 @@ ExperimentDesignMultiPrefView, ExperimentDesignPrefView, ExperimentDesignRolloutView, - ExperimentDetailView, - ExperimentListView, - ExperimentRecipeView, ExperimentSendIntentToShipEmailView, ) @@ -21,17 +18,6 @@ ExperimentSendIntentToShipEmailView.as_view(), name="experiments-api-send-intent-to-ship-email", ), - url( - r"^(?P[\w-]+)/recipe/$", - ExperimentRecipeView.as_view(), - name="experiments-api-recipe", - ), - url( - r"^(?P[\w-]+)/$", - ExperimentDetailView.as_view(), - name="experiments-api-detail", - ), - url(r"^$", ExperimentListView.as_view(), name="experiments-api-list"), url( r"^(?P[\w-]+)/clone", ExperimentCloneView.as_view(), diff --git a/app/experimenter/experiments/public_api_urls.py b/app/experimenter/experiments/public_api_urls.py new file mode 100644 index 0000000000..c394851476 --- /dev/null +++ b/app/experimenter/experiments/public_api_urls.py @@ -0,0 +1,22 @@ +from django.conf.urls import url + +from experimenter.experiments.api_views import ( + ExperimentDetailView, + ExperimentListView, + ExperimentRecipeView, +) + + +urlpatterns = [ + url( + r"^(?P[\w-]+)/recipe/$", + ExperimentRecipeView.as_view(), + name="experiments-api-recipe", + ), + url( + r"^(?P[\w-]+)/$", + ExperimentDetailView.as_view(), + name="experiments-api-detail", + ), + url(r"^$", ExperimentListView.as_view(), name="experiments-api-list"), +] diff --git a/app/experimenter/static/js/utils/api.js b/app/experimenter/static/js/utils/api.js index 7bf75b52b0..4dfe36de0e 100644 --- a/app/experimenter/static/js/utils/api.js +++ b/app/experimenter/static/js/utils/api.js @@ -1,6 +1,6 @@ import { request } from "experimenter/utils/request.js"; -const API_ROOT = "/api/v1/"; +const API_ROOT = "/api/v2/"; export function makeApiRequest(endpoint, options = {}, extraHeaders = {}) { return request(`${API_ROOT}${endpoint}`, options, extraHeaders); diff --git a/app/experimenter/urls.py b/app/experimenter/urls.py index b0e60acd5a..9d1b579356 100644 --- a/app/experimenter/urls.py +++ b/app/experimenter/urls.py @@ -7,7 +7,10 @@ urlpatterns = [ - re_path(r"^api/v1/experiments/", include("experimenter.experiments.api_urls")), + re_path(r"^api/v1/experiments/", include("experimenter.experiments.public_api_urls")), + re_path( + r"^api/v2/experiments/", include("experimenter.experiments.private_api_urls") + ), re_path(r"^admin/", admin.site.urls), re_path(r"^experiments/", include("experimenter.experiments.web_urls")), re_path(r"^$", ExperimentListView.as_view(), name="home"), From e315afc8aa0c6b6592b59de95b16679103f87e67 Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Wed, 4 Mar 2020 14:11:00 -0800 Subject: [PATCH 3/5] stop changelog serializer from convering json string -> string fixes #2346 (#2347) Co-authored-by: Jared Lockhart <119884+jaredlockhart@users.noreply.github.com> --- app/experimenter/experiments/serializers/entities.py | 1 - .../experiments/tests/serializers/test_entities.py | 5 ++--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/experimenter/experiments/serializers/entities.py b/app/experimenter/experiments/serializers/entities.py index b09fa59ce8..17f37c4a4f 100644 --- a/app/experimenter/experiments/serializers/entities.py +++ b/app/experimenter/experiments/serializers/entities.py @@ -61,7 +61,6 @@ class ChangeLogSerializer(serializers.ModelSerializer): variants = ExperimentVariantSerializer(many=True, required=False) locales = LocaleSerializer(many=True, required=False) countries = CountrySerializer(many=True, required=False) - pref_type = PrefTypeField() class Meta: model = Experiment diff --git a/app/experimenter/experiments/tests/serializers/test_entities.py b/app/experimenter/experiments/tests/serializers/test_entities.py index 8a85b56960..dccf0e6173 100644 --- a/app/experimenter/experiments/tests/serializers/test_entities.py +++ b/app/experimenter/experiments/tests/serializers/test_entities.py @@ -185,8 +185,7 @@ def test_serializer_outputs_expected_schema(self): serializer = ChangeLogSerializer(experiment) risk_tech_description = experiment.risk_technical_description - # ensure expected_data has "string" if pref_type is json string - pref_type = PrefTypeField().to_representation(experiment.pref_type) + expected_data = { "type": experiment.type, "owner": experiment.owner.id, @@ -201,7 +200,7 @@ def test_serializer_outputs_expected_schema(self): "addon_experiment_id": experiment.addon_experiment_id, "addon_release_url": experiment.addon_release_url, "pref_name": experiment.pref_name, - "pref_type": pref_type, + "pref_type": experiment.pref_type, "pref_branch": experiment.pref_branch, "public_name": experiment.public_name, "public_description": experiment.public_description, From c13f803589f8decdfb65b4474505402f7e263325 Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Wed, 4 Mar 2020 14:18:14 -0800 Subject: [PATCH 4/5] edits to make reviews list in specific order fixes #2325 (#2327) * edits to make reviews list in specific order fixes #2325 * removed comment and added exp type for test --- app/experimenter/experiments/models.py | 9 +++++-- .../experiments/tests/test_models.py | 27 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/app/experimenter/experiments/models.py b/app/experimenter/experiments/models.py index 300bf9e834..035ffe1f6b 100644 --- a/app/experimenter/experiments/models.py +++ b/app/experimenter/experiments/models.py @@ -675,15 +675,20 @@ 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", ] - if not self.is_rollout: - reviews += ["review_science", "review_bugzilla", "review_engineering"] + 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 diff --git a/app/experimenter/experiments/tests/test_models.py b/app/experimenter/experiments/tests/test_models.py index cb74ec48ef..f23a4f0800 100644 --- a/app/experimenter/experiments/tests/test_models.py +++ b/app/experimenter/experiments/tests/test_models.py @@ -1260,6 +1260,33 @@ def test_is_ready_to_launch_is_false_without_conditional_review(self): self.assertFalse(experiment.is_ready_to_launch) + def test_review_order_is_correct_for_experiment(self): + experiment = ExperimentFactory.create(type=Experiment.TYPE_PREF) + expected_reviews = [ + "review_science", + "review_advisory", + "review_engineering", + "review_qa_requested", + "review_intent_to_ship", + "review_bugzilla", + "review_qa", + "review_relman", + ] + reviews = experiment.get_all_required_reviews() + self.assertEqual(expected_reviews, reviews) + + def test_review_order_is_correct_for_rollout(self): + experiment = ExperimentFactory.create(type=Experiment.TYPE_ROLLOUT) + expected_reviews = [ + "review_advisory", + "review_qa_requested", + "review_intent_to_ship", + "review_qa", + "review_relman", + ] + reviews = experiment.get_all_required_reviews() + self.assertEqual(expected_reviews, reviews) + def test_completed_results_returns_true_if_any_results(self): experiment = ExperimentFactory.create( results_initial="The results here were great." From aad9f950e15d05ea2bcc27bf33a3b921ac03b993 Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Wed, 4 Mar 2020 14:27:08 -0800 Subject: [PATCH 5/5] black_fix and eslint_fix to use compose file instead of compose_test fixes #2322 (#2324) Co-authored-by: Jared Lockhart <119884+jaredlockhart@users.noreply.github.com> --- Makefile | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index e06c845523..d94c14a6ed 100644 --- a/Makefile +++ b/Makefile @@ -43,9 +43,6 @@ testfast: test_build eslint: test_build $(COMPOSE_TEST) run app sh -c "$(ESLINT)" -eslint_fix: test_build - $(COMPOSE_TEST) run app sh -c "$(ESLINT_FIX)" - py_test: test_build $(COMPOSE_TEST) run app sh -c "$(WAIT_FOR_DB) $(PYTHON_TEST)" @@ -58,9 +55,6 @@ flake8: test_build black_check: test_build $(COMPOSE_TEST) run app sh -c "$(BLACK_CHECK)" -black_fix: test_build - $(COMPOSE_TEST) run app sh -c "$(BLACK_FIX)" - check_migrations: test_build $(COMPOSE_TEST) run app sh -c "$(WAIT_FOR_DB) $(PYTHON_CHECK_MIGRATIONS)" @@ -94,6 +88,12 @@ up: compose_kill compose_build generate_docs: compose_build $(COMPOSE) run app sh -c "$(GENERATE_DOCS)" +eslint_fix: test_build + $(COMPOSE) run app sh -c "$(ESLINT_FIX)" + +black_fix: test_build + $(COMPOSE) run app sh -c "$(BLACK_FIX)" + code_format: compose_build $(COMPOSE) run app sh -c "$(BLACK_FIX)&&$(ESLINT_FIX)"