diff --git a/experimenter/experimenter/nimbus_ui/constants.py b/experimenter/experimenter/nimbus_ui/constants.py index bacd13679d..77f7f83356 100644 --- a/experimenter/experimenter/nimbus_ui/constants.py +++ b/experimenter/experimenter/nimbus_ui/constants.py @@ -406,7 +406,6 @@ class NimbusUIConstants: ERROR_ROLLOUT_PLAN_FIX_ERRORS = ( "Resolve the highlighted errors above before saving this plan." ) - ERROR_ROLLOUT_PHASE_LOCKED = "This rollout phase is locked and cannot be changed." ROLLOUT_APPLICATION_TOOLTIP = ( "Rollouts can only target one Application at a time. Application can " "not be changed after a rollout is created." diff --git a/experimenter/experimenter/nimbus_ui/new/forms.py b/experimenter/experimenter/nimbus_ui/new/forms.py index dfbc2ed136..b7aeb88052 100644 --- a/experimenter/experimenter/nimbus_ui/new/forms.py +++ b/experimenter/experimenter/nimbus_ui/new/forms.py @@ -1737,6 +1737,12 @@ class Meta: model = NimbusRolloutPhase fields = ("start_date", "end_date", "population_percent") + def clean_population_percent(self): + population_percent = self.cleaned_data.get("population_percent") + if population_percent is None: + return Decimal(0) + return population_percent + class RolloutScheduleForm(NimbusChangeLogFormMixin, forms.ModelForm): rollout_plan = forms.ChoiceField( @@ -1785,11 +1791,19 @@ def __init__(self, *args, **kwargs): form=RolloutPhaseForm, extra=0, ) + self.plans = self.available_plans() + annotated_phases = { + phase.id: phase for phase in self.instance.annotated_rollout_phases() + } + self.locked_phase_ids = { + phase_id + for phase_id, phase in annotated_phases.items() + if phase.card_status in NimbusUIConstants.RolloutPhaseStatus.LOCKED + } self.rollout_phases = self.NimbusRolloutPhaseFormSet( - data=self.data or None, + data=self.get_rollout_phases_data(), instance=self.instance, ) - self.plans = self.available_plans() self.fields["rollout_plan"].choices = [("", "None")] + [ (name, f"{name} ({NimbusRolloutPlanTemplate.summary(phases)})") for name, phases in self.plans.items() @@ -1807,16 +1821,13 @@ def __init__(self, *args, **kwargs): } ) - annotated_phases = { - phase.id: phase for phase in self.instance.annotated_rollout_phases() - } - self.locked_phase_ids = { - phase_id - for phase_id, phase in annotated_phases.items() - if phase.card_status in NimbusUIConstants.RolloutPhaseStatus.LOCKED - } not_started = NimbusUIConstants.RolloutPhaseStatus.NOT_STARTED + self.visible_phase_count = 0 for phase_form in self.rollout_phases.forms: + phase_form.is_deleted = bool(phase_form["DELETE"].value()) + if not phase_form.is_deleted: + self.visible_phase_count += 1 + phase_form.number = self.visible_phase_count phase = annotated_phases.get(phase_form.instance.pk) status = phase.card_status if phase else not_started phase_form.card_status = status @@ -1838,6 +1849,26 @@ def __init__(self, *args, **kwargs): for field_name in disabled_fields: phase_form.fields[field_name].disabled = True + def get_rollout_phases_data(self): + # Preview schedule changes in the formset data so they are only written to + # the database when the card is saved. + if not self.is_bound: + return None + + data = self.data.copy() + prefix = "rollout_phases" + total_forms = int(data[f"{prefix}-TOTAL_FORMS"]) + + for index in range(total_forms): + phase_id = data.get(f"{prefix}-{index}-id") + if phase_id and int(phase_id) in self.locked_phase_ids: + data.pop(f"{prefix}-{index}-DELETE", None) + + return self.preview_rollout_phases(data) + + def preview_rollout_phases(self, data): + return data + @staticmethod def available_plans(): plans = dict(NimbusUIConstants.ROLLOUT_TEMPLATE_PLANS) @@ -1859,58 +1890,49 @@ def get_changelog_message(self): class RolloutPhaseCreateForm(RolloutScheduleForm): - @transaction.atomic - def save(self, *args, **kwargs): - super().save(*args, **kwargs) - self.instance.rollout_phases.create() - return self.instance - - def get_changelog_message(self): - return f"{self.request.user} added a rollout phase" + def preview_rollout_phases(self, data): + prefix = "rollout_phases" + total_forms_key = f"{prefix}-TOTAL_FORMS" + total_forms = int(data[total_forms_key]) + data[f"{prefix}-{total_forms}-population_percent"] = "0" + data[total_forms_key] = str(total_forms + 1) + return data class RolloutPhaseDeleteForm(RolloutScheduleForm): - phase_id = forms.ModelChoiceField(queryset=NimbusRolloutPhase.objects.all()) - - class Meta: - model = NimbusExperiment - fields = ["phase_id"] - - def clean_phase_id(self): - phase = self.cleaned_data["phase_id"] - if phase.pk in self.locked_phase_ids: - raise forms.ValidationError(NimbusUIConstants.ERROR_ROLLOUT_PHASE_LOCKED) - return phase + def preview_rollout_phases(self, data): + prefix = "rollout_phases" + index = data.get("phase_index") + if not index: + return data + + phase_id = data.get(f"{prefix}-{index}-id") + if not (phase_id and int(phase_id) in self.locked_phase_ids): + data[f"{prefix}-{index}-DELETE"] = "on" + return data - @transaction.atomic - def save(self, *args, **kwargs): - super().save(*args, **kwargs) - self.cleaned_data["phase_id"].delete() - return self.instance - def get_changelog_message(self): - return f"{self.request.user} removed a rollout phase" +class RolloutPlanApplyForm(RolloutScheduleForm): + def preview_rollout_phases(self, data): + plan_name = data.get("rollout_plan") + if plan_name not in self.plans: + return data + prefix = "rollout_phases" + total_forms_key = f"{prefix}-TOTAL_FORMS" + total_forms = int(data[total_forms_key]) -class RolloutPlanApplyForm(RolloutScheduleForm): - @transaction.atomic - def apply_plan(self): - plan_name = self.data.get("rollout_plan") - if plan_name and plan_name in self.plans: - self.instance.rollout_phases.exclude(id__in=self.locked_phase_ids).delete() - for population_percent in self.plans[plan_name]: - self.instance.rollout_phases.create( - population_percent=Decimal(str(population_percent)) - ) + for index in range(total_forms): + phase_id = data.get(f"{prefix}-{index}-id") + if not (phase_id and int(phase_id) in self.locked_phase_ids): + data[f"{prefix}-{index}-DELETE"] = "on" - @transaction.atomic - def save(self, *args, **kwargs): - experiment = super().save(*args, **kwargs) - self.apply_plan() - return experiment + for population_percent in self.plans[plan_name]: + data[f"{prefix}-{total_forms}-population_percent"] = str(population_percent) + total_forms += 1 - def get_changelog_message(self): - return f"{self.request.user} applied a rollout plan" + data[total_forms_key] = str(total_forms) + return data class RolloutPlanCreateForm(RolloutScheduleForm): @@ -1934,19 +1956,17 @@ def clean(self): ) return cleaned_data - @transaction.atomic def save(self): - experiment = super().save() - phases = [ - float(phase.population_percent) for phase in experiment.rollout_phases.all() - ] NimbusRolloutPlanTemplate.objects.create( - name=self.cleaned_data["template_name"], phases=phases + name=self.cleaned_data["template_name"], + phases=[ + float(phase_form.cleaned_data["population_percent"]) + for phase_form in self.rollout_phases.forms + if phase_form.cleaned_data.get("population_percent") is not None + and not phase_form.cleaned_data.get("DELETE") + ], ) - return experiment - - def get_changelog_message(self): - return f"{self.request.user} created a rollout plan template" + return self.instance class SubscribeForm(NimbusChangeLogFormMixin, forms.ModelForm): diff --git a/experimenter/experimenter/nimbus_ui/new/views.py b/experimenter/experimenter/nimbus_ui/new/views.py index 8d00470e71..1b52846978 100644 --- a/experimenter/experimenter/nimbus_ui/new/views.py +++ b/experimenter/experimenter/nimbus_ui/new/views.py @@ -795,43 +795,47 @@ def get_context_data(self, **kwargs): context["rollout_phase_population_estimates"] = ( get_rollout_phase_population_estimates(self.object) ) - selected_plan = self.request.POST.get("template_name") or self.request.POST.get( - "rollout_plan" - ) - if selected_plan: - context["form"].initial["rollout_plan"] = selected_plan return context def can_edit(self): return self.object.is_draft or self.object.is_rolling_out +class RolloutSchedulePreviewMixin: + def form_valid(self, form): + return self.render_to_response(self.get_context_data(form=form)) + + class NewRolloutPhaseCreateView( - RenderParentDBResponseMixin, NewRolloutScheduleUpdateView + RolloutSchedulePreviewMixin, NewRolloutScheduleUpdateView ): form_class = RolloutPhaseCreateForm -class NewRolloutPhaseDeleteView(CardMutationMixin, NewRolloutScheduleUpdateView): +class NewRolloutPhaseDeleteView( + RolloutSchedulePreviewMixin, NewRolloutScheduleUpdateView +): form_class = RolloutPhaseDeleteForm - def mutate(self, form): - phase_id = self.request.POST.get("phase_id") - if not phase_id: - return - if int(phase_id) not in form.locked_phase_ids: - self.object.rollout_phases.filter(id=phase_id).delete() - -class NewRolloutPlanCreateView(RenderParentDBResponseMixin, NewRolloutScheduleUpdateView): - form_class = RolloutPlanCreateForm +class NewRolloutPlanApplyView(RolloutSchedulePreviewMixin, NewRolloutScheduleUpdateView): + form_class = RolloutPlanApplyForm -class NewRolloutPlanApplyView(CardMutationMixin, NewRolloutScheduleUpdateView): - form_class = RolloutPlanApplyForm +class NewRolloutPlanCreateView(NewRolloutScheduleUpdateView): + form_class = RolloutPlanCreateForm - def mutate(self, form): - form.apply_plan() + def form_valid(self, form): + form.save() + data = self.request.POST.copy() + data["rollout_plan"] = form.cleaned_data["template_name"] + return self.render_to_response( + self.get_context_data( + form=RolloutScheduleForm( + data=data, instance=self.object, request=self.request + ) + ) + ) class NewSubscribeView(NimbusExperimentViewMixin, RequestFormMixin, UpdateView): diff --git a/experimenter/experimenter/nimbus_ui/templates/new/rollouts/schedule/edit_form.html b/experimenter/experimenter/nimbus_ui/templates/new/rollouts/schedule/edit_form.html index 1299c0c45e..0fd36b098e 100644 --- a/experimenter/experimenter/nimbus_ui/templates/new/rollouts/schedule/edit_form.html +++ b/experimenter/experimenter/nimbus_ui/templates/new/rollouts/schedule/edit_form.html @@ -23,81 +23,86 @@ {{ form.rollout_phases.management_form }}
- {% if form.rollout_phases|length %} + {% if form.visible_phase_count %}
{{ NimbusUIConstants.ROLLOUT_SCHEDULE_DATES_NOTE }}
{% endif %} {% for phase_form in form.rollout_phases %} - {% with display=phase_form.card_status_display %} -
- {{ phase_form.id }} - {# Phase Number and Status #} -
- Phase {{ forloop.counter }} - {% include "new/rollouts/schedule/phase_status_badge.html" %} + {% if phase_form.is_deleted %} + {{ phase_form.id }} + {{ phase_form.DELETE.as_hidden }} + {% else %} + {% with display=phase_form.card_status_display %} +
+ {{ phase_form.id }} + {# Phase Number and Status #} +
+ Phase {{ phase_form.number }} + {% include "new/rollouts/schedule/phase_status_badge.html" %} -
-
- {# Population in % #} -
- -
- {{ phase_form.population_percent|add_error_class:"is-invalid" }} - % -
- {% if phase_form.population_percent.errors %} -
- {{ phase_form.population_percent.errors|join:", " }} -
- {% endif %}
- {# Duration #} -
- -
-
- {{ phase_form.start_date|add_error_class:"is-invalid" }} - {% if phase_form.start_date.errors %} -
- {{ phase_form.start_date.errors|join:", " }} -
- {% endif %} +
+ {# Population in % #} +
+ +
+ {{ phase_form.population_percent|add_error_class:"is-invalid" }} + %
-
- {{ phase_form.end_date|add_error_class:"is-invalid" }} - {% if phase_form.end_date.errors %} -
- {{ phase_form.end_date.errors|join:", " }} -
- {% endif %} + {% if phase_form.population_percent.errors %} +
+ {{ phase_form.population_percent.errors|join:", " }} +
+ {% endif %} +
+ {# Duration #} +
+ +
+
+ {{ phase_form.start_date|add_error_class:"is-invalid" }} + {% if phase_form.start_date.errors %} +
+ {{ phase_form.start_date.errors|join:", " }} +
+ {% endif %} +
+
+ {{ phase_form.end_date|add_error_class:"is-invalid" }} + {% if phase_form.end_date.errors %} +
+ {{ phase_form.end_date.errors|join:", " }} +
+ {% endif %} +
+ {% if not phase_form.is_locked %} + + {% endif %}
- {% if not phase_form.is_locked %} - - {% endif %}
-
- {% endwith %} + {% endwith %} + {% endif %} {% endfor %}
- {% if form.rollout_phases|length %} + {% if form.visible_phase_count %}
Reuse this schedule on other rollouts