Skip to content

perf(nimbus): compute rollout review errors once per render #17152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jaredlockhart wants to merge 2 commits into
base: main
Choose a base branch
from
Open

perf(nimbus): compute rollout review errors once per render #17152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
557435e
perf(nimbus): compute rollout review errors once per render
jaredlockhart Sep 4, 2026
9dc20ac
Merge branch 'main' into 17151
jaredlockhart Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions experimenter/experimenter/experiments/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1403,18 +1403,6 @@ def next_rollout_phase(self):
next_index = current_index + 1
return phases[next_index] if next_index < len(phases) else None

@property
def has_rollout_review_errors(self):
from experimenter.experiments.api.v5.serializers import (
NimbusRolloutReviewSerializer,
)

if not self.is_rollout:
return False
return bool(
self.get_invalid_fields_errors(serializer_class=NimbusRolloutReviewSerializer)
)

@property
def next_rollout_phase_number(self):
next_phase = self.next_rollout_phase
Expand Down
4 changes: 0 additions & 4 deletions experimenter/experimenter/experiments/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8110,10 +8110,6 @@ def test_next_rollout_phase_returns_following_phase_with_zero_population(self):
experiment.save()
self.assertEqual(experiment.next_rollout_phase, phases[1])

def test_has_rollout_review_errors_false_when_not_rollout(self):
experiment = NimbusExperimentFactory.create(is_rollout=False)
self.assertFalse(experiment.has_rollout_review_errors)

def test_next_rollout_phase_number_returns_position_of_next_phase(self):
experiment = self.live_rollout()
phases = [
Expand Down
16 changes: 10 additions & 6 deletions experimenter/experimenter/nimbus_ui/new/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,12 +293,14 @@ def split_branch_screenshot_errors(self, field_errors):

def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
review_errors = self.object.get_invalid_fields_errors(
serializer_class=NimbusRolloutReviewSerializer
)
context["has_rollout_review_errors"] = self.object.is_rollout and bool(
review_errors
)
field_errors = self.split_branch_screenshot_errors(
self.drop_documentation_link_title_errors(
self.object.get_invalid_fields_errors(
serializer_class=NimbusRolloutReviewSerializer
)
)
self.drop_documentation_link_title_errors(review_errors)
)
cards = NimbusUIConstants.ROLLOUT_CARD_FIELDS

Expand Down Expand Up @@ -875,7 +877,9 @@ def post(self, *args, **kwargs):
return response


class NewToggleArchiveView(NimbusExperimentViewMixin, RequestFormMixin, UpdateView):
class NewToggleArchiveView(
RolloutSetupProgressMixin, NimbusExperimentViewMixin, RequestFormMixin, UpdateView
):
form_class = ToggleArchiveForm
template_name = "new/common/base.html"

Expand Down
22 changes: 11 additions & 11 deletions experimenter/experimenter/nimbus_ui/templates/new/common/sidebar_rollout_actions.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
{% if experiment.next_rollout_phase %}
{# Disabled to Live #}
<span class="d-inline-block w-100"
{% if not reenable_supported %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REENABLE_UNSUPPORTED_MESSAGE }}"{% elif experiment.has_rollout_review_errors %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_HAS_ISSUES_TOOLTIP }}"{% elif transition_pending %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REVIEW_PENDING_TOOLTIP }}"{% endif %}>
{% if not reenable_supported %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REENABLE_UNSUPPORTED_MESSAGE }}"{% elif has_rollout_review_errors %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_HAS_ISSUES_TOOLTIP }}"{% elif transition_pending %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REVIEW_PENDING_TOOLTIP }}"{% endif %}>
<button type="button"
id="rollout-resume-btn"
class="btn btn-primary btn-sm w-100 d-flex align-items-center justify-content-center gap-2{% if transition_pending or experiment.has_rollout_review_errors or not reenable_supported %} opacity-50{% else %} sidebar-action-button{% endif %}"
{% if transition_pending or experiment.has_rollout_review_errors or not reenable_supported %} disabled aria-disabled="true" {% else %} hx-post="{% url 'nimbus-ui-new-disabled-to-live-rollout' experiment.slug %}" hx-headers='{"X-CSRFToken": "{{ csrf_token }}"}' hx-disabled-elt=".sidebar-action-button" {% endif %}>
class="btn btn-primary btn-sm w-100 d-flex align-items-center justify-content-center gap-2{% if transition_pending or has_rollout_review_errors or not reenable_supported %} opacity-50{% else %} sidebar-action-button{% endif %}"
{% if transition_pending or has_rollout_review_errors or not reenable_supported %} disabled aria-disabled="true" {% else %} hx-post="{% url 'nimbus-ui-new-disabled-to-live-rollout' experiment.slug %}" hx-headers='{"X-CSRFToken": "{{ csrf_token }}"}' hx-disabled-elt=".sidebar-action-button" {% endif %}>
{% include "new/common/sidebar_transition_spinner.html" %}

<i class="fa-regular fa-circle-play"></i>
Expand All @@ -22,11 +22,11 @@
{# Disabled to Live #}
<div class="collapse show rollout-resume-swap" id="rollout-resume-trigger">
<span class="d-inline-block w-100"
{% if not reenable_supported %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REENABLE_UNSUPPORTED_MESSAGE }}"{% elif experiment.has_rollout_review_errors %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_HAS_ISSUES_TOOLTIP }}"{% elif transition_pending %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REVIEW_PENDING_TOOLTIP }}"{% endif %}>
{% if not reenable_supported %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REENABLE_UNSUPPORTED_MESSAGE }}"{% elif has_rollout_review_errors %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_HAS_ISSUES_TOOLTIP }}"{% elif transition_pending %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REVIEW_PENDING_TOOLTIP }}"{% endif %}>
<button type="button"
id="rollout-resume-btn"
class="btn btn-primary btn-sm w-100 d-flex align-items-center justify-content-center gap-2{% if transition_pending or experiment.has_rollout_review_errors or not reenable_supported %} opacity-50{% else %} sidebar-action-button{% endif %}"
{% if transition_pending or experiment.has_rollout_review_errors or not reenable_supported %} disabled aria-disabled="true" {% else %} data-bs-toggle="collapse" data-bs-target=".rollout-resume-swap" aria-expanded="false" aria-controls="rollout-duplicate-phase-confirm" {% endif %}>
class="btn btn-primary btn-sm w-100 d-flex align-items-center justify-content-center gap-2{% if transition_pending or has_rollout_review_errors or not reenable_supported %} opacity-50{% else %} sidebar-action-button{% endif %}"
{% if transition_pending or has_rollout_review_errors or not reenable_supported %} disabled aria-disabled="true" {% else %} data-bs-toggle="collapse" data-bs-target=".rollout-resume-swap" aria-expanded="false" aria-controls="rollout-duplicate-phase-confirm" {% endif %}>
<i class="fa-regular fa-circle-play"></i>
Start next phase
</button>
Expand All @@ -48,8 +48,8 @@
</button>
<button type="button"
id="rollout-duplicate-phase-accept-btn"
class="btn btn-primary btn-sm w-100 d-flex align-items-center justify-content-center gap-2{% if transition_pending or experiment.has_rollout_review_errors or not reenable_supported %} opacity-50{% else %} sidebar-action-button{% endif %}"
{% if transition_pending or experiment.has_rollout_review_errors or not reenable_supported %} disabled aria-disabled="true" {% else %} hx-post="{% url 'nimbus-ui-new-disabled-to-live-duplicate-phase-rollout' experiment.slug %}" hx-headers='{"X-CSRFToken": "{{ csrf_token }}"}' hx-disabled-elt=".sidebar-action-button" {% endif %}>
class="btn btn-primary btn-sm w-100 d-flex align-items-center justify-content-center gap-2{% if transition_pending or has_rollout_review_errors or not reenable_supported %} opacity-50{% else %} sidebar-action-button{% endif %}"
{% if transition_pending or has_rollout_review_errors or not reenable_supported %} disabled aria-disabled="true" {% else %} hx-post="{% url 'nimbus-ui-new-disabled-to-live-duplicate-phase-rollout' experiment.slug %}" hx-headers='{"X-CSRFToken": "{{ csrf_token }}"}' hx-disabled-elt=".sidebar-action-button" {% endif %}>
{% include "new/common/sidebar_transition_spinner.html" %}

<i class="fa-regular fa-circle-check"></i>
Expand All @@ -74,11 +74,11 @@
</span>
{# Live to Live phase advance #}
<span class="d-inline-block w-100"
{% if experiment.has_rollout_review_errors %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_HAS_ISSUES_TOOLTIP }}"{% elif transition_pending %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REVIEW_PENDING_TOOLTIP }}"{% elif not experiment.next_rollout_phase %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_NO_NEXT_PHASE_TOOLTIP }}"{% endif %}>
{% if has_rollout_review_errors %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_HAS_ISSUES_TOOLTIP }}"{% elif transition_pending %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_REVIEW_PENDING_TOOLTIP }}"{% elif not experiment.next_rollout_phase %}data-bs-toggle="tooltip" title="{{ NimbusUIConstants.ROLLOUT_NO_NEXT_PHASE_TOOLTIP }}"{% endif %}>
<button type="button"
id="rollout-next-phase-btn"
class="btn btn-primary btn-sm w-100 d-flex align-items-center justify-content-center gap-2{% if transition_pending or not experiment.next_rollout_phase or experiment.has_rollout_review_errors %} opacity-50{% else %} sidebar-action-button{% endif %}"
{% if transition_pending or not experiment.next_rollout_phase or experiment.has_rollout_review_errors %} disabled aria-disabled="true" {% else %} hx-post="{% url 'nimbus-ui-new-advance-phase-review-rollout' experiment.slug %}" hx-headers='{"X-CSRFToken": "{{ csrf_token }}"}' hx-disabled-elt=".sidebar-action-button" {% endif %}>
class="btn btn-primary btn-sm w-100 d-flex align-items-center justify-content-center gap-2{% if transition_pending or not experiment.next_rollout_phase or has_rollout_review_errors %} opacity-50{% else %} sidebar-action-button{% endif %}"
{% if transition_pending or not experiment.next_rollout_phase or has_rollout_review_errors %} disabled aria-disabled="true" {% else %} hx-post="{% url 'nimbus-ui-new-advance-phase-review-rollout' experiment.slug %}" hx-headers='{"X-CSRFToken": "{{ csrf_token }}"}' hx-disabled-elt=".sidebar-action-button" {% endif %}>
{% include "new/common/sidebar_transition_spinner.html" %}

<i class="fa-regular fa-circle-play"></i>
Expand Down
115 changes: 115 additions & 0 deletions experimenter/experimenter/nimbus_ui/tests/test_new_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,94 @@ def audience_data(


class TestNimbusRolloutDetailView(AuthTestCase):
@parameterized.expand([(True, False), (None, True)])
def test_setup_issues_disable_resume_button(self, risk_brand, expect_disabled):
experiment = NimbusExperimentFactory.create(
is_rollout=True,
status=NimbusExperiment.Status.DISABLED,
publish_status=NimbusExperiment.PublishStatus.IDLE,
application=NimbusExperiment.Application.DESKTOP,
firefox_min_version=NimbusExperiment.Version.FIREFOX_156,
risk_brand=risk_brand,
)
NimbusRolloutPhaseFactory.create(experiment=experiment, population_percent=10)
resume_url = reverse(
"nimbus-ui-new-disabled-to-live-rollout", kwargs={"slug": experiment.slug}
)

response = self.client.get(
reverse("new-nimbus-ui-rollout-detail", kwargs={"slug": experiment.slug})
)

self.assertContains(response, 'id="rollout-resume-btn"')
if expect_disabled:
self.assertNotContains(response, resume_url)
self.assertContains(response, NimbusUIConstants.ROLLOUT_HAS_ISSUES_TOOLTIP)
else:
self.assertContains(response, resume_url)

@parameterized.expand([(True, False), (None, True)])
def test_setup_issues_disable_duplicate_phase_button(
self, risk_brand, expect_disabled
):
experiment = NimbusExperimentFactory.create(
is_rollout=True,
status=NimbusExperiment.Status.DISABLED,
publish_status=NimbusExperiment.PublishStatus.IDLE,
application=NimbusExperiment.Application.DESKTOP,
firefox_min_version=NimbusExperiment.Version.FIREFOX_156,
risk_brand=risk_brand,
)
final_phase = NimbusRolloutPhaseFactory.create(
experiment=experiment, population_percent=10
)
experiment.rollout_phase = final_phase
experiment.save()
duplicate_phase_url = reverse(
"nimbus-ui-new-disabled-to-live-duplicate-phase-rollout",
kwargs={"slug": experiment.slug},
)

response = self.client.get(
reverse("new-nimbus-ui-rollout-detail", kwargs={"slug": experiment.slug})
)

self.assertContains(response, 'id="rollout-duplicate-phase-accept-btn"')
if expect_disabled:
self.assertNotContains(response, duplicate_phase_url)
self.assertContains(response, NimbusUIConstants.ROLLOUT_HAS_ISSUES_TOOLTIP)
else:
self.assertContains(response, duplicate_phase_url)

@parameterized.expand([(True, False), (None, True)])
def test_setup_issues_disable_advance_phase_button(self, risk_brand, expect_disabled):
experiment = NimbusExperimentFactory.create_with_lifecycle(
NimbusExperimentFactory.Lifecycles.LIVE_ENROLLING,
is_rollout=True,
firefox_min_version=NimbusExperiment.Version.FIREFOX_156,
risk_brand=risk_brand,
)
current_phase = NimbusRolloutPhaseFactory.create(
experiment=experiment, population_percent=10
)
NimbusRolloutPhaseFactory.create(experiment=experiment, population_percent=20)
experiment.rollout_phase = current_phase
experiment.save()
advance_url = reverse(
"nimbus-ui-new-advance-phase-review-rollout", kwargs={"slug": experiment.slug}
)

response = self.client.get(
reverse("new-nimbus-ui-rollout-detail", kwargs={"slug": experiment.slug})
)

self.assertContains(response, 'id="rollout-next-phase-btn"')
if expect_disabled:
self.assertNotContains(response, advance_url)
self.assertContains(response, NimbusUIConstants.ROLLOUT_HAS_ISSUES_TOOLTIP)
else:
self.assertContains(response, advance_url)

@mock.patch.object(NimbusExperiment, "get_invalid_fields_errors", return_value={})
def test_ready_rollout_shows_preview_and_launch_actions(self, _mock_errors):
experiment = NimbusExperimentFactory.create(
Expand Down Expand Up @@ -3502,6 +3590,33 @@ def test_toggle_archive_status_to_unarchive(self):
updated_experiment = NimbusExperiment.objects.get(slug=self.experiment.slug)
self.assertFalse(updated_experiment.is_archived)

@parameterized.expand([(True, False), (None, True)])
def test_sidebar_resume_button_reflects_setup_issues(
self, risk_brand, expect_disabled
):
experiment = NimbusExperimentFactory.create(
is_rollout=True,
status=NimbusExperiment.Status.DISABLED,
publish_status=NimbusExperiment.PublishStatus.IDLE,
application=NimbusExperiment.Application.DESKTOP,
firefox_min_version=NimbusExperiment.Version.FIREFOX_156,
risk_brand=risk_brand,
)
NimbusRolloutPhaseFactory.create(experiment=experiment, population_percent=10)
resume_url = reverse(
"nimbus-ui-new-disabled-to-live-rollout", kwargs={"slug": experiment.slug}
)

response = self.client.get(
reverse("nimbus-ui-new-toggle-archive", kwargs={"slug": experiment.slug})
)

self.assertContains(response, 'id="rollout-resume-btn"')
if expect_disabled:
self.assertNotContains(response, resume_url)
else:
self.assertContains(response, resume_url)

def test_detail_page_renders_archive_button(self):
experiment = NimbusExperimentFactory.create_with_lifecycle(
NimbusExperimentFactory.Lifecycles.CREATED,
Expand Down
Loading