Skip to content

Commit d9ad3f9

Browse files
feat(nimbus): Approval and rejection flow (#12090)
Because - We want to add approval and rejection flow on the new summary page This commit - Add option to approve and Launch - Add the option to reject the experiment with the message - Shows the timeout message Fixes #12072 Note: This currently does not show the rejection reason-which I will add in the separate PR Approval flow https://github.com/user-attachments/assets/84731825-e241-43fd-b9d0-f9dc2922c85c Rejection flow https://github.com/user-attachments/assets/9d201e88-cfc7-49f3-b6ba-3fd110488237 Timeout message <img width="1687" alt="Screenshot 2025-01-22 at 10 27 54 AM" src="https://github.com/user-attachments/assets/b2d07515-15d2-40d5-882e-edbf67032cda" /> --------- Co-authored-by: Jared Lockhart <[email protected]>
1 parent c1510ea commit d9ad3f9

File tree

9 files changed

+322
-1
lines changed

9 files changed

+322
-1
lines changed

experimenter/experimenter/experiments/models.py

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -673,6 +673,17 @@ def is_review(self):
673673
and self.publish_status == self.PublishStatus.REVIEW
674674
)
675675

676+
@property
677+
def is_review_timeline(self):
678+
return self.status in {
679+
self.Status.DRAFT,
680+
self.Status.PREVIEW,
681+
} and self.publish_status in {
682+
self.PublishStatus.REVIEW,
683+
self.PublishStatus.APPROVED,
684+
self.PublishStatus.WAITING,
685+
}
686+
676687
@property
677688
def is_preview(self):
678689
return self.status == self.Status.PREVIEW
@@ -709,6 +720,16 @@ def can_preview_to_draft(self):
709720
def can_preview_to_review(self):
710721
return self.is_preview
711722

723+
def should_show_remote_settings_pending(self, reviewer):
724+
return self.publish_status in (
725+
self.PublishStatus.APPROVED,
726+
self.PublishStatus.WAITING,
727+
) and self.can_review(reviewer)
728+
729+
@property
730+
def should_show_timeout_message(self):
731+
return self.changes.latest_timeout()
732+
712733
@property
713734
def draft_date(self):
714735
if change := self.changes.all().order_by("changed_on").first():
@@ -925,7 +946,7 @@ def timeline(self):
925946
{
926947
"label": self.PublishStatus.REVIEW,
927948
"date": self.review_date,
928-
"is_active": self.is_review,
949+
"is_active": self.is_review_timeline,
929950
"days": self.computed_review_days,
930951
"tooltip": NimbusUIConstants.TIMELINE_TOOLTIPS["Review"],
931952
},

experimenter/experimenter/nimbus_ui_new/forms.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@
1212
NimbusExperimentBranchThroughExcluded,
1313
NimbusExperimentBranchThroughRequired,
1414
)
15+
from experimenter.kinto.tasks import (
16+
nimbus_check_kinto_push_queue_by_collection,
17+
nimbus_synchronize_preview_experiments_in_kinto,
18+
)
1519
from experimenter.nimbus_ui_new.constants import NimbusUIConstants
1620
from experimenter.outcomes import Outcomes
1721
from experimenter.projects.models import Project
@@ -546,6 +550,12 @@ class DraftToPreviewForm(UpdateStatusForm):
546550
def get_changelog_message(self):
547551
return f"{self.request.user} launched experiment to Preview"
548552

553+
def save(self, commit=True):
554+
experiment = super().save(commit=commit)
555+
experiment.allocate_bucket_range()
556+
nimbus_synchronize_preview_experiments_in_kinto.apply_async(countdown=5)
557+
return experiment
558+
549559

550560
class DraftToReviewForm(UpdateStatusForm):
551561
status = NimbusExperiment.Status.DRAFT
@@ -573,6 +583,11 @@ class PreviewToDraftForm(UpdateStatusForm):
573583
def get_changelog_message(self):
574584
return f"{self.request.user} moved the experiment back to Draft"
575585

586+
def save(self, commit=True):
587+
experiment = super().save(commit=commit)
588+
nimbus_synchronize_preview_experiments_in_kinto.apply_async(countdown=5)
589+
return experiment
590+
576591

577592
class ReviewToDraftForm(UpdateStatusForm):
578593
status = NimbusExperiment.Status.DRAFT
@@ -581,3 +596,33 @@ class ReviewToDraftForm(UpdateStatusForm):
581596

582597
def get_changelog_message(self):
583598
return f"{self.request.user} cancelled the review"
599+
600+
601+
class ReviewToApproveForm(UpdateStatusForm):
602+
status = NimbusExperiment.Status.DRAFT
603+
status_next = NimbusExperiment.Status.LIVE
604+
publish_status = NimbusExperiment.PublishStatus.APPROVED
605+
606+
def get_changelog_message(self):
607+
return f"{self.request.user} approved the review."
608+
609+
def save(self, commit=True):
610+
experiment = super().save(commit=commit)
611+
experiment.allocate_bucket_range()
612+
nimbus_check_kinto_push_queue_by_collection.apply_async(
613+
countdown=5, args=[experiment.kinto_collection]
614+
)
615+
return experiment
616+
617+
618+
class ReviewToRejectForm(UpdateStatusForm):
619+
status = NimbusExperiment.Status.DRAFT
620+
status_next = None
621+
publish_status = NimbusExperiment.PublishStatus.IDLE
622+
changelog_message = forms.CharField(
623+
required=True, label="Reason for Rejection", max_length=1000
624+
)
625+
626+
def get_changelog_message(self):
627+
changelog_message = self.cleaned_data.get("changelog_message", "")
628+
return f"{self.request.user} rejected the review with reason: {changelog_message}"

experimenter/experimenter/nimbus_ui_new/static/js/review_controls.js

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,32 @@ window.toggleSubmitButton = function () {
1212
const submitButton = document.getElementById("request-launch-button");
1313
submitButton.disabled = !(checkbox1.checked && checkbox2.checked);
1414
};
15+
16+
function initializeRejectApproveListeners() {
17+
const rejectButton = document.getElementById("reject-button");
18+
const reviewControls = document.getElementById("review-controls");
19+
const rejectFormContainer = document.getElementById("reject-form-container");
20+
const cancelReject = document.getElementById("cancel-reject");
21+
22+
if (rejectButton) {
23+
rejectButton.addEventListener("click", () => {
24+
reviewControls.classList.add("d-none");
25+
rejectFormContainer.classList.remove("d-none");
26+
});
27+
}
28+
29+
if (cancelReject) {
30+
cancelReject.addEventListener("click", () => {
31+
rejectFormContainer.classList.add("d-none");
32+
reviewControls.classList.remove("d-none");
33+
});
34+
}
35+
}
36+
37+
// Initialize listeners on initial load
38+
document.addEventListener("DOMContentLoaded", initializeRejectApproveListeners);
39+
40+
// Reinitialize listeners after HTMX content swaps
41+
document.body.addEventListener("htmx:afterSwap", () => {
42+
initializeRejectApproveListeners();
43+
});

experimenter/experimenter/nimbus_ui_new/templates/nimbus_experiments/launch_controls.html

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
{% load nimbus_extras %}
2+
13
<div id="launch-controls">
24
<form>
35
{% csrf_token %}
@@ -116,6 +118,45 @@
116118
</div>
117119
<!-- Review Mode Controls -->
118120
{% elif experiment.is_review %}
121+
{% if experiment.should_show_timeout_message %}
122+
<div class="alert alert-danger" role="alert">
123+
<p>
124+
<span role="img" aria-label="red X emoji"></span>
125+
Remote Settings request has timed out, please go through the approval flow to launch this experiment again.
126+
</p>
127+
</div>
128+
{% endif %}
129+
<div id="review-controls" class="alert alert-primary">
130+
<p>
131+
<strong>{{ experiment.changes.latest_review_request.changed_by.email }}</strong> requested to Launch this experiment.
132+
</p>
133+
<button type="button"
134+
class="btn btn-success"
135+
hx-post="{% url 'nimbus-new-review-to-approve' slug=experiment.slug %}"
136+
hx-select="#content"
137+
hx-target="#content"
138+
hx-swap="outerHTML">Approve and Launch Experiment</button>
139+
<button type="button" class="btn btn-danger" id="reject-button">Reject</button>
140+
</div>
141+
<!-- Rejection Form -->
142+
<div id="reject-form-container" class="d-none alert alert-warning">
143+
<label for="changelog_message">
144+
<strong>You are rejecting the request to launch this experiment.</strong> Please add some comments:
145+
</label>
146+
<textarea id="changelog_message"
147+
name="changelog_message"
148+
class="form-control"
149+
rows="4"
150+
required></textarea>
151+
<button type="submit"
152+
class="btn btn-danger mt-2"
153+
hx-post="{% url 'nimbus-new-review-to-reject' slug=experiment.slug %}"
154+
hx-select="#content"
155+
hx-target="#content"
156+
hx-swap="outerHTML"
157+
hx-include="#changelog_message">Submit</button>
158+
<button type="button" class="btn btn-secondary mt-2" id="cancel-reject">Cancel</button>
159+
</div>
119160
<div class="alert alert-warning">
120161
<p>The experiment is currently under review. If you wish to cancel the review, click the button below:</p>
121162
<button type="button"
@@ -125,6 +166,16 @@
125166
hx-target="#content"
126167
hx-swap="outerHTML">Cancel Review</button>
127168
</div>
169+
{% elif experiment|should_show_remote_settings_pending:user %}
170+
<div class="alert alert-primary">
171+
<p>
172+
<strong>Action required:</strong> Please review this change in Remote Settings to launch this experiment.
173+
</p>
174+
<a href="{{ experiment.review_url }}"
175+
class="btn btn-primary"
176+
target="_blank"
177+
rel="noopener noreferrer">Open Remote Settings</a>
178+
</div>
128179
{% endif %}
129180
</form>
130181
</div>

experimenter/experimenter/nimbus_ui_new/templatetags/nimbus_extras.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,3 +81,8 @@ def format_json(value):
8181
f'<pre class="text-monospace" style="white-space: pre-wrap; '
8282
f'word-wrap: break-word;">{parsed_json}</pre>'
8383
)
84+
85+
86+
@register.filter
87+
def should_show_remote_settings_pending(experiment, reviewer):
88+
return experiment.should_show_remote_settings_pending(reviewer)

experimenter/experimenter/nimbus_ui_new/tests/test_forms.py

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
from unittest.mock import patch
2+
13
from django.test import RequestFactory, TestCase
24
from django.urls import reverse
35

@@ -15,6 +17,10 @@
1517
NimbusDocumentationLinkFactory,
1618
NimbusExperimentFactory,
1719
)
20+
from experimenter.kinto.tasks import (
21+
nimbus_check_kinto_push_queue_by_collection,
22+
nimbus_synchronize_preview_experiments_in_kinto,
23+
)
1824
from experimenter.nimbus_ui_new.constants import NimbusUIConstants
1925
from experimenter.nimbus_ui_new.forms import (
2026
AudienceForm,
@@ -28,7 +34,9 @@
2834
PreviewToDraftForm,
2935
PreviewToReviewForm,
3036
QAStatusForm,
37+
ReviewToApproveForm,
3138
ReviewToDraftForm,
39+
ReviewToRejectForm,
3240
SignoffForm,
3341
SubscribeForm,
3442
TakeawaysForm,
@@ -337,6 +345,18 @@ def setUp(self):
337345
super().setUp()
338346
self.experiment = NimbusExperimentFactory.create()
339347

348+
self.mock_preview_task = patch.object(
349+
nimbus_synchronize_preview_experiments_in_kinto, "apply_async"
350+
).start()
351+
self.mock_push_task = patch.object(
352+
nimbus_check_kinto_push_queue_by_collection, "apply_async"
353+
).start()
354+
self.mock_allocate_bucket_range = patch.object(
355+
NimbusExperiment, "allocate_bucket_range"
356+
).start()
357+
358+
self.addCleanup(patch.stopall)
359+
340360
def test_draft_to_preview_form(self):
341361
self.experiment.status = NimbusExperiment.Status.DRAFT
342362
self.experiment.status_next = None
@@ -353,6 +373,8 @@ def test_draft_to_preview_form(self):
353373
changelog = experiment.changes.latest("changed_on")
354374
self.assertEqual(changelog.changed_by, self.user)
355375
self.assertIn("launched experiment to Preview", changelog.message)
376+
self.mock_preview_task.assert_called_once_with(countdown=5)
377+
self.mock_allocate_bucket_range.assert_called_once()
356378

357379
def test_draft_to_review_form(self):
358380
self.experiment.status = NimbusExperiment.Status.DRAFT
@@ -376,6 +398,7 @@ def test_preview_to_review_form(self):
376398
self.experiment.status_next = NimbusExperiment.Status.PREVIEW
377399
self.experiment.publish_status = NimbusExperiment.PublishStatus.IDLE
378400
self.experiment.save()
401+
379402
form = PreviewToReviewForm(
380403
data={}, instance=self.experiment, request=self.request
381404
)
@@ -395,6 +418,7 @@ def test_preview_to_draft_form(self):
395418
self.experiment.status_next = NimbusExperiment.Status.PREVIEW
396419
self.experiment.publish_status = NimbusExperiment.PublishStatus.IDLE
397420
self.experiment.save()
421+
398422
form = PreviewToDraftForm(data={}, instance=self.experiment, request=self.request)
399423
self.assertTrue(form.is_valid(), form.errors)
400424

@@ -406,6 +430,7 @@ def test_preview_to_draft_form(self):
406430
changelog = experiment.changes.latest("changed_on")
407431
self.assertEqual(changelog.changed_by, self.user)
408432
self.assertIn("moved the experiment back to Draft", changelog.message)
433+
self.mock_preview_task.assert_called_once_with(countdown=5)
409434

410435
def test_review_to_draft_form(self):
411436
self.experiment.status = NimbusExperiment.Status.DRAFT
@@ -425,6 +450,57 @@ def test_review_to_draft_form(self):
425450
self.assertEqual(changelog.changed_by, self.user)
426451
self.assertIn("cancelled the review", changelog.message)
427452

453+
def test_review_to_approve_form(self):
454+
self.experiment.status = NimbusExperiment.Status.DRAFT
455+
self.experiment.status_next = NimbusExperiment.Status.LIVE
456+
self.experiment.publish_status = NimbusExperiment.PublishStatus.REVIEW
457+
self.experiment.save()
458+
459+
form = ReviewToApproveForm(
460+
data={}, instance=self.experiment, request=self.request
461+
)
462+
self.assertTrue(form.is_valid(), form.errors)
463+
464+
experiment = form.save()
465+
self.assertEqual(experiment.status, NimbusExperiment.Status.DRAFT)
466+
self.assertEqual(experiment.status_next, NimbusExperiment.Status.LIVE)
467+
self.assertEqual(
468+
experiment.publish_status, NimbusExperiment.PublishStatus.APPROVED
469+
)
470+
471+
changelog = experiment.changes.latest("changed_on")
472+
self.assertEqual(changelog.changed_by, self.user)
473+
self.assertIn(f"{self.user} approved the review.", changelog.message)
474+
self.mock_push_task.assert_called_once_with(
475+
countdown=5, args=[experiment.kinto_collection]
476+
)
477+
self.mock_allocate_bucket_range.assert_called_once()
478+
479+
def test_review_to_reject_form_with_reason(self):
480+
self.experiment.status = NimbusExperiment.Status.DRAFT
481+
self.experiment.status_next = NimbusExperiment.Status.LIVE
482+
self.experiment.publish_status = NimbusExperiment.PublishStatus.REVIEW
483+
self.experiment.save()
484+
485+
form = ReviewToRejectForm(
486+
data={"changelog_message": "Needs more work."},
487+
instance=self.experiment,
488+
request=self.request,
489+
)
490+
self.assertTrue(form.is_valid(), form.errors)
491+
492+
experiment = form.save()
493+
self.assertEqual(experiment.status, NimbusExperiment.Status.DRAFT)
494+
self.assertEqual(experiment.status_next, None)
495+
self.assertEqual(experiment.publish_status, NimbusExperiment.PublishStatus.IDLE)
496+
497+
changelog = experiment.changes.latest("changed_on")
498+
self.assertEqual(changelog.changed_by, self.user)
499+
self.assertIn(
500+
f"{self.user} rejected the review with reason: Needs more work.",
501+
changelog.message,
502+
)
503+
428504

429505
class TestOverviewForm(RequestFormTestCase):
430506
def test_valid_form_saves(self):

0 commit comments

Comments
 (0)