diff --git a/experimenter/experimenter/nimbus_ui/new/forms.py b/experimenter/experimenter/nimbus_ui/new/forms.py index b0b7a10fc..ae9ac88f0 100644 --- a/experimenter/experimenter/nimbus_ui/new/forms.py +++ b/experimenter/experimenter/nimbus_ui/new/forms.py @@ -278,14 +278,13 @@ def __init__(self, *args, **kwargs): self.instance.feature_config.slug ) + feature_config = ( + self.instance.feature_config if self.instance.feature_config_id else None + ) + if ( - self.instance.id is not None - and self.instance.feature_config - and ( - schema := self.instance.feature_config.schemas.filter( - version=None - ).first() - ) + feature_config + and (schema := feature_config.schemas.filter(version=None).first()) and schema is not None and schema.schema is not None ): @@ -303,10 +302,34 @@ def clean_value(self): return value +class RolloutBranchFeatureValueForm(NimbusBranchFeatureValueForm): + class Meta: + model = NimbusBranchFeatureValue + fields = ("feature_config", "value") + widgets = {"feature_config": forms.HiddenInput()} + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + if ( + self.is_bound + and not self.instance.feature_config_id + and (feature_config_id := self.data.get(f"{self.prefix}-feature_config")) + ): + self.instance.feature_config = NimbusFeatureConfig.objects.filter( + id=feature_config_id + ).first() + + def clean_feature_config(self): + return self.cleaned_data.get("feature_config") or ( + self.instance.feature_config if self.instance.feature_config_id else None + ) + + RolloutBranchFeatureValueFormSet = inlineformset_factory( NimbusBranch, NimbusBranchFeatureValue, - form=NimbusBranchFeatureValueForm, + form=RolloutBranchFeatureValueForm, extra=0, ) @@ -693,7 +716,7 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.branch_feature_values = RolloutBranchFeatureValueFormSet( - data=self.data or None, + data=self.get_branch_feature_values_data(), instance=self.reference_branch, prefix="branch-feature-value", ) @@ -717,6 +740,47 @@ def __init__(self, *args, **kwargs): # will remain unused as rollouts donot have results data self.fields["rollout_experience"].initial = self.instance.takeaways_summary + def get_branch_feature_values_data(self): + # Add temporary formset rows so newly selected, unsaved features get JSON + # editors during the HTMX preview before Save persists them. + if not self.is_bound: + return None + + data = self.data.copy() + prefix = "branch-feature-value" + total_forms_key = "branch-feature-value-TOTAL_FORMS" + total_forms = int(data[total_forms_key]) + + selected_feature_config_values = ( + data.getlist("feature_configs") + if hasattr(data, "getlist") + else data.get("feature_configs", []) + ) + selected_feature_config_ids = [ + int(feature_config_id) + for feature_config_id in selected_feature_config_values + if feature_config_id + ] + submitted_feature_config_ids = { + int(feature_config_id) + for index in range(total_forms) + if (feature_config_id := data.get(f"{prefix}-{index}-feature_config")) + } + + new_feature_config_ids = [ + feature_config_id + for feature_config_id in selected_feature_config_ids + if feature_config_id not in submitted_feature_config_ids + ] + + for feature_config_id in new_feature_config_ids: + data[f"{prefix}-{total_forms}-feature_config"] = feature_config_id + data[f"{prefix}-{total_forms}-value"] = "{}" + total_forms += 1 + + data[total_forms_key] = str(total_forms) + return data + @property def errors(self): errors = super().errors diff --git a/experimenter/experimenter/nimbus_ui/new/views.py b/experimenter/experimenter/nimbus_ui/new/views.py index 3957f86c7..68edde82a 100644 --- a/experimenter/experimenter/nimbus_ui/new/views.py +++ b/experimenter/experimenter/nimbus_ui/new/views.py @@ -159,17 +159,16 @@ class NewRolloutFeaturesUpdateView(CardMixin, NewCardUpdateView): display_template = "new/rollouts/rollout_features/card.html" template_name = "new/rollouts/rollout_features/edit_form.html" - def render_valid_response(self): - self.object.refresh_from_db() - + def form_valid(self, form): # If the request came from the explicit Save button, return the read-only card # view so the UI swaps back to the card. When the form is posted for intermediate # updates (e.g. feature_configs changed via hx-post on change), return the # editable form so the user can continue editing. + if "save" in self.request.POST: - return super().render_valid_response() + return super().form_valid(form) - return self.render_to_response(self.get_context_data()) + return self.render_to_response(self.get_context_data(form=form)) class NewQAUpdateView(CardMixin, NewCardUpdateView): diff --git a/experimenter/experimenter/nimbus_ui/templates/new/rollouts/rollout_features/edit_form.html b/experimenter/experimenter/nimbus_ui/templates/new/rollouts/rollout_features/edit_form.html index f0be66bef..921c067dd 100644 --- a/experimenter/experimenter/nimbus_ui/templates/new/rollouts/rollout_features/edit_form.html +++ b/experimenter/experimenter/nimbus_ui/templates/new/rollouts/rollout_features/edit_form.html @@ -35,6 +35,7 @@ {{ branch_feature_values.management_form }} {% for branch_feature_values_form in branch_feature_values %} {{ branch_feature_values_form.id }} + {{ branch_feature_values_form.feature_config }}
{% block extrascripts %} {% if branch_form_data %}{{ branch_form_data|json_script:"branch-form-data" }}{% endif %} - + {% endblock extrascripts %} diff --git a/experimenter/experimenter/nimbus_ui/tests/test_new_forms.py b/experimenter/experimenter/nimbus_ui/tests/test_new_forms.py index 052a87755..46305beab 100644 --- a/experimenter/experimenter/nimbus_ui/tests/test_new_forms.py +++ b/experimenter/experimenter/nimbus_ui/tests/test_new_forms.py @@ -678,6 +678,105 @@ def test_form_sets_initial_rollout_experience_and_filters_feature_configs(self): ) ) + def test_selected_feature_without_value_gets_temporary_form(self): + feature_config = NimbusFeatureConfigFactory.create( + application=NimbusExperiment.Application.DESKTOP, + slug="rollout-feature-preview", + ) + experiment = NimbusExperimentFactory.create_with_lifecycle( + NimbusExperimentFactory.Lifecycles.CREATED, + application=NimbusExperiment.Application.DESKTOP, + feature_configs=[], + ) + + form = RolloutFeaturesForm( + instance=experiment, + data={ + "rollout_experience": "Updated rollout experience", + "feature_configs": [feature_config.id], + "branch-feature-value-TOTAL_FORMS": "0", + "branch-feature-value-INITIAL_FORMS": "0", + "branch-feature-value-MIN_NUM_FORMS": "0", + "branch-feature-value-MAX_NUM_FORMS": "1000", + }, + request=self.request, + ) + + self.assertEqual(len(form.branch_feature_values.forms), 1) + feature_value_form = form.branch_feature_values.forms[0] + self.assertEqual(feature_value_form.instance.feature_config, feature_config) + self.assertEqual(feature_value_form["value"].value(), "{}") + self.assertEqual(experiment.feature_configs.count(), 0) + + def test_temporary_feature_value_form_saves_typed_value(self): + feature_config = NimbusFeatureConfigFactory.create( + application=NimbusExperiment.Application.DESKTOP, + slug="rollout-feature-preview-save", + ) + experiment = NimbusExperimentFactory.create_with_lifecycle( + NimbusExperimentFactory.Lifecycles.CREATED, + application=NimbusExperiment.Application.DESKTOP, + feature_configs=[], + ) + + form = RolloutFeaturesForm( + instance=experiment, + data={ + "rollout_experience": "Updated rollout experience", + "feature_configs": [feature_config.id], + "branch-feature-value-TOTAL_FORMS": "1", + "branch-feature-value-INITIAL_FORMS": "0", + "branch-feature-value-MIN_NUM_FORMS": "0", + "branch-feature-value-MAX_NUM_FORMS": "1000", + "branch-feature-value-0-feature_config": feature_config.id, + "branch-feature-value-0-value": '{"enabled": true}', + }, + request=self.request, + ) + + self.assertTrue(form.is_valid(), form.errors) + experiment = form.save() + experiment.refresh_from_db() + + feature_value = experiment.reference_branch.feature_values.get( + feature_config=feature_config + ) + self.assertEqual(feature_value.value, '{"enabled": true}') + + def test_selected_feature_without_submitted_value_saves_default_value(self): + feature_config = NimbusFeatureConfigFactory.create( + application=NimbusExperiment.Application.DESKTOP, + slug="rollout-feature-default-value", + ) + experiment = NimbusExperimentFactory.create_with_lifecycle( + NimbusExperimentFactory.Lifecycles.CREATED, + application=NimbusExperiment.Application.DESKTOP, + feature_configs=[], + ) + + form = RolloutFeaturesForm( + instance=experiment, + data={ + "rollout_experience": "Updated rollout experience", + "feature_configs": [feature_config.id], + "branch-feature-value-TOTAL_FORMS": "0", + "branch-feature-value-INITIAL_FORMS": "0", + "branch-feature-value-MIN_NUM_FORMS": "0", + "branch-feature-value-MAX_NUM_FORMS": "1000", + }, + request=self.request, + ) + + self.assertTrue(form.is_valid(), form.errors) + form.branch_feature_values.save = lambda: None + experiment = form.save() + experiment.refresh_from_db() + + feature_value = experiment.reference_branch.feature_values.get( + feature_config=feature_config + ) + self.assertEqual(feature_value.value, "{}") + def test_form_reports_branch_feature_value_errors(self): feature_config = NimbusFeatureConfigFactory.create( application=NimbusExperiment.Application.DESKTOP, @@ -699,6 +798,7 @@ def test_form_reports_branch_feature_value_errors(self): "branch-feature-value-INITIAL_FORMS": "1", "branch-feature-value-MIN_NUM_FORMS": "0", "branch-feature-value-MAX_NUM_FORMS": "1000", + "branch-feature-value-0-feature_config": feature_config.id, "branch-feature-value-0-value": "{}", }, request=self.request, @@ -787,6 +887,7 @@ def test_form_deletes_removed_reference_branch_feature_values(self): "branch-feature-value-MIN_NUM_FORMS": "0", "branch-feature-value-MAX_NUM_FORMS": "1000", "branch-feature-value-0-id": reference_feature_value.id, + "branch-feature-value-0-feature_config": feature_config.id, "branch-feature-value-0-value": reference_feature_value.value, }, request=self.request, diff --git a/experimenter/experimenter/nimbus_ui/tests/test_new_views.py b/experimenter/experimenter/nimbus_ui/tests/test_new_views.py index c4d3fe0b6..18d613b35 100644 --- a/experimenter/experimenter/nimbus_ui/tests/test_new_views.py +++ b/experimenter/experimenter/nimbus_ui/tests/test_new_views.py @@ -12,6 +12,7 @@ from experimenter.experiments.tests.factories import ( NimbusDocumentationLinkFactory, NimbusExperimentFactory, + NimbusFeatureConfigFactory, TagFactory, ) from experimenter.nimbus_ui.new.forms import ( @@ -356,16 +357,22 @@ def test_post_valid_saves_and_returns_display_card(self): self.assertTrue(response.context["hx_swap_oob"]) def test_post_change_returns_edit_form(self): + feature_config = NimbusFeatureConfigFactory.create( + application=NimbusExperiment.Application.DESKTOP, + slug="rollout-feature", + ) experiment = NimbusExperimentFactory.create_with_lifecycle( NimbusExperimentFactory.Lifecycles.CREATED, + application=NimbusExperiment.Application.DESKTOP, feature_configs=[], + takeaways_summary="Original rollout experience", ) response = self.client.post( reverse(self.url_name, kwargs={"slug": experiment.slug}), { "rollout_experience": "Updated rollout experience", - "feature_configs": [], + "feature_configs": [feature_config.id], "branch-feature-value-TOTAL_FORMS": "0", "branch-feature-value-INITIAL_FORMS": "0", }, @@ -373,8 +380,11 @@ def test_post_change_returns_edit_form(self): self.assertEqual(response.status_code, 200) self.assertTemplateUsed(response, "new/rollouts/rollout_features/edit_form.html") + self.assertContains(response, "rollout-feature") + self.assertContains(response, "value-editor") experiment.refresh_from_db() - self.assertEqual(experiment.takeaways_summary, "Updated rollout experience") + self.assertEqual(experiment.takeaways_summary, "Original rollout experience") + self.assertEqual(experiment.feature_configs.count(), 0) class TestNewQAUpdateView(NewViewTestMixin, AuthTestCase):